Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable ARCH_BCM2835 for downstream use #1099

Merged
merged 10 commits into from
Aug 22, 2015
Merged

Conversation

notro
Copy link
Contributor

@notro notro commented Aug 15, 2015

This PR makes ARCH_BCM2835 functionally equivalent to ARCH_BCM2708.

spi-bcm2708 is dropped now that spi-bcm2835 is the default on 4.1.
i2c-bcm2708 is kept, because i2c-bcm2835 has not seen any downstream use yet and it doesn't work with a I2C OLED display I have.

The Device Tree files are copied from their 2708 counterparts.

Issues

Onboard audio is disabled by default regardless of whether the module is loaded or not.

apb_pclk and uart0_pclk are defined both in bcm2708_common.dtsi and in clk-bcm2835 resulting in an error in the boot messages from the latter.
I haven't found a way to disable a DT clock, the status property is not checked and changing the compatible string results in no serial output.
This is a purely cosmetic issue.

[    0.212758] apb_pclk not registered
[    0.212849] uart0_pclk not registered
[    0.212900] uart0_pclk alias not registered

Test details

Serial & Revision

$ cat /proc/cpuinfo
...
Hardware        : BCM2835
Revision        : 0010
Serial          : 00000000a1725dd1

Thermal

$ cat /sys/class/thermal/thermal_zone0/temp && vcgencmd measure_temp
39007
temp=39.0'C

cpufreq

$ head -1 /boot/config.txt
arm_freq=900
$ dmesg | grep cpufreq
[    1.893675] bcm2835-cpufreq: min=700000 max=900000

vc_mem

$ sudo vcdbg log msg 2>&1 | grep Loading
000915.390: Loading 'kernel.img' from SD card
001159.040: Loading 'bcm2835-rpi-b-plus.dtb' from SD card

/dev/vcio

$ sudo /opt/vc/src/hello_pi/hello_fft/hello_fft.bin 12
rel_rms_err = 7.8e-07, usecs = 259, k = 0

bcm2708_fb Colour palette

# Audio is necessary: /boot/config.txt
# dtparam=audio=on

$ ./ch-egg-port-sdl-source_v1.1/ch-egg.elf

bcm2708_fb Ioctl: FBIOPAN_DISPLAY and FBIO_WAITFORVSYNC

$ ./fbtestXIV

bcm2708_fb Blanking

$ setterm -blank 1

SPI is tested with a PiScreen display.

Uart1 is tested.

@notro
Copy link
Contributor Author

notro commented Aug 15, 2015

@pelwell The mmc drivers get a slightly different build on ARCH_BCM2835. Is it possible to have the same build on all?

@pelwell
Copy link
Contributor

pelwell commented Aug 16, 2015

It was never clear to me why the builds would handle SDIO interrupts differently:

  • On 270x builds, the non-threaded IRQ handler calls mmc_signal_sdio_irq that wakes up the MMC-framework-provided sdio_irq_thread, that then calls process_sdio_pending_irqs.
  • On 2835 builds, the non-threaded IRQ handler defers to the threaded IRQ handler, which calls sdio_run_irqs that in turn calls process_sdio_pending_irqs directly.

I found this patch (bf3b5ec) that introduced sdio_run_irqs, but I don't know why the use int the mmc driver is conditional on the architecture. The differences in the sdhost driver are academic since we don't (and probably never will) use the SDIO capabilities - that code has never been run. The 2835 configuration seems to be the more up-to-date since it is using the new facility, so perhaps I could try making the change unconditionally. Unfortunately I'm not in a position to test SDIO functionality, which makes me reluctant to commit a change like that.

@notro
Copy link
Contributor Author

notro commented Aug 16, 2015

There is only one in-kernel user of this new code, and that's the sdhci subsystem: mmc: sdhci: convert to new SDIO IRQ handling
It looks like the code in bcm2835-mmc was copied from that patch.
However this statement is missing in our code, but it seem to only have impact on some resume path: mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;

I added a printk to bcm2835_mmc_thread_irq(), but it didn't fire.

So I suggest we just rip out this conditional code. It's been a year since the patch was added and there has been no other drivers converting to it, so it can't be that important.
Even though Russel had this to say:

...
rather than allowing the SDIO core code to buggily spawn its own
thread.

I think it's important that the mmc driver behaves in the same way on all architectures if possible.

@pelwell
Copy link
Contributor

pelwell commented Aug 17, 2015

See #1102

@pelwell
Copy link
Contributor

pelwell commented Aug 19, 2015

I've succeeded in getting the upstream interrupt controller to work for DT-enabled Pi 1 builds. Without DT it falls back to the old armctrl code. This patch (https://gist.github.com/pelwell/d41ec7fb7db4d1136eb5) should be applied after all of the others in this PR - perhaps you could append it, @notro?

@pelwell
Copy link
Contributor

pelwell commented Aug 19, 2015

For Pi 2 there will be a race between the upstreaming of the BCM2836 support and our removal of non-DT support. If BCM2836 wins then we can produce a similar patch for Pi 2, otherwise we just remove the old armctrl driver and switch wholesale.

Tested on Pi 1 and Pi 2, with and without Device Tree.

@pelwell
Copy link
Contributor

pelwell commented Aug 20, 2015

@notro Can you rebase against 4.2-rc7, and consider appending my patch?

@notro
Copy link
Contributor Author

notro commented Aug 20, 2015

Building fails on ARCH_BCM2835:

drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c: In function ‘hcd_init_fiq’:
drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c:453:13: error: ‘INTERRUPT_VC_USB’ undeclared (first use in this function)
  enable_fiq(INTERRUPT_VC_USB);
             ^
drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c:453:13: note: each undeclared identifier is reported only once for each function it appears in

Why was this change necessary:

-   init_FIQ(FIQ_START);
+   init_FIQ(NUMBER_IRQS);

I'm restarting the build now. I'll let you know when I have tried it.

@pelwell
Copy link
Contributor

pelwell commented Aug 20, 2015

Why was this change necessary:

Because of lines like this (https://github.com/notro/linux/commit/bab25463bb5842694f73de42bdf7edec600f2b7c#diff-b412d3553bc228bafd652cac486b891fR107):

static inline unsigned int hwirq_to_fiq(unsigned long hwirq)
{
    hwirq -= NUMBER_IRQS;

and this (https://github.com/notro/linux/commit/bab25463bb5842694f73de42bdf7edec600f2b7c#diff-b412d3553bc228bafd652cac486b891fR206):

            irq = irq_create_mapping(intc.domain,
                    MAKE_HWIRQ(b, i) + NUMBER_IRQS);

i.e. You have mapped the FIQs starting NUMBER_IRQS after the IRQs, and init_FIQ indicates where the FIQs start. Without this change it can't associate the arriving FIQ interrupts with the correct device.

@notro
Copy link
Contributor Author

notro commented Aug 20, 2015

The irq-bcm2835.c change doesn't work on ARCH_BCM2835. Lots of:

[    7.305673] Transfer to device 4 endpoint 0x1 frame 769 failed - FIQ timed out. Data may have been lost.

Reverting the change made it work again.

I remember that getting the fiq patch working was trial and error, so it's hard to come up with the rationale for this now :-)

I guess you needed this change to make it work on ARCH_BCM2708?

@pelwell
Copy link
Contributor

pelwell commented Aug 20, 2015

Yes, it was necessary. I'll try the 2835 build tomorrow.

notro added 9 commits August 20, 2015 20:52
Add a duplicate irq range with an offset on the hwirq's so the
driver can detect that enable_fiq() is used.
Tested with downstream dwc_otg USB controller driver.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Currently poweroff/halt results in a reboot on the Raspberry Pi.
The firmware uses the RSTS register to know which partiton to
boot from. The partiton value is spread into bits
0, 2, 4, 6, 8, 10. Partiton 63 is a special partition used by
the firmware to indicate halt.

The firmware made this change in 19 Aug 2013 and was matched
by the downstream commit:
Changes for new NOOBS multi partition booting from gsh

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Tested-by: Stephen Warren <swarren@wwwdotorg.org>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
These commands where used to make this commit:

: Get changed and new config values from a merge
./scripts/diffconfig -m arch/arm/configs/bcm2835_defconfig arch/arm/configs/bcmrpi_defconfig > merge.cfg

: Remove these options
cat << EOF > filter
CONFIG_ARCH_BCM2708
CONFIG_BCM2708_DT
CONFIG_ARM_PATCH_PHYS_VIRT
CONFIG_PHYS_OFFSET
CONFIG_CMDLINE
CONFIG_BCM2708_WDT
CONFIG_HW_RANDOM_BCM2708
CONFIG_SPI_BCM2708
EOF

: Apply filter
grep -F -v -f filter merge.cfg > filtered.cfg

: Add these options
: watchdog contains the restart/poweroff code.
cat << EOF > added.cfg
CONFIG_WATCHDOG=y
CONFIG_BCM2835_WDT=y
CONFIG_MISC_FILESYSTEMS=y
CONFIG_I2C_BCM2835=m
CONFIG_SND_BCM2835_SOC_I2S=m
EOF

: Create new config
ARCH=arm scripts/kconfig/merge_config.sh arch/arm/configs/bcm2835_defconfig filtered.cfg added.cfg
: Verify
ARCH=arm make oldconfig

: Update bcm2835_defconfig
ARCH=arm make savedefconfig
cp defconfig arch/arm/configs/bcm2835_defconfig

: Clean up
rm merge.cfg filter filtered.cfg added.cfg defconfig

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
This is a hack until a proper solution is agreed upon.
Martin Sperl is doing some work in this area.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
@notro
Copy link
Contributor Author

notro commented Aug 20, 2015

Rebased

@pelwell
Copy link
Contributor

pelwell commented Aug 21, 2015

I have an updated patch here that builds and works on ARCH_BCM2835 as well, and without changing any of the upstream code.

There were two related problems:

  1. The FIQ offset (the parameter to init_FIQ()) is in Linux IRQ numbers (not the underlying hardware IRQ values). This should be the number of registered IRQs (see 2), which is why FIQ_START was correct for BCM2835 builds.

  2. The IRQ map used by irq-bcm2835 puts the ARMCTRL interrupts (BANK0) at 0, and the other banks at 32 and 64. Since BANK0 only includes 8 interrupt sources, this leaves a hole in the hardware IRQ numbers between BANK0 and BANK1. (The downstream code puts the short bank at the end, so no gap).
    CONFIG_SPARSE_IRQ is set on ARCH_BCM2835, which causes 16 legacy IRQs to be allocated and no gaps to appear in the mapped IRQs:

IRQ BANK0: 16-23
IRQ BANK1: 24-55
IRQ BANK2: 56-87
FIQ BANK0: 88-95
FIQ BANK1: 96-127
FIQ BANK2: 128-159

You will see there is a constant offset of 72 between corresponding FIQs and IRQs, and 72 is FIQ_START.

On MACH_BCM2708, CONFIG_SPARSE_IRQ is not set, so there are no legacy IRQs and it tries to use the identity mapping for IRQ numbers. The allocator avoids 0 as an illegal value, so maps BANK0 to 1-8. But it isn't necessary to skip 32 or 64, so the mapping ends up looking like this:

IRQ BANK0: 1-8
IRQ BANK1: 32-63
IRQ BANK2: 64-95
FIQ BANK0: 96-103
FIQ BANK1: 128-159
FIQ BANK2: 160-191

Here the FIQ->IRQ offset is 95 for BANK0 and 96 for BANK1 and BANK2. This explains why NUMBER_IRQs (96) worked as the argument to init_FIQ, but it wouldn't have worked for the BANK0 FIQs and breaks ARCH_BCM2835 builds.

The workaround I have found is to provide an implementation of arch_dynirq_lower_bound (a weak function) that pushes the IRQ BANK0 numbers up, pulls the FIQ BANK1+2 numbers down to 104-167, and leaves the other numbers alone:

IRQ BANK0: 24-31
IRQ BANK1: 32-63
IRQ BANK2: 64-95
FIQ BANK0: 96-103
FIQ BANK1: 104-135
FIQ BANK2: 136-167

This restores the fixed offset, and keeps it at 72, matching ARCH_BCM2835 and removing the need to change irq-bcm2835.

@notro
Copy link
Contributor Author

notro commented Aug 21, 2015

Thanks for the detailed explanation.
I have added your commit.
Tested on ARCH_BCM2708 and ARCH_BCM2835

@pelwell
Copy link
Contributor

pelwell commented Aug 21, 2015

I'm happy with this. @popcornmix?

@popcornmix
Copy link
Collaborator

This PR breaks booting for me with bcm2709_defconfig. NFS boot fails. See:
http://pastebin.com/UgUHpcCL

@popcornmix
Copy link
Collaborator

@pelwell looks like "BCM2708: Use upstream interrupt driver on all Pi1's" is the commit that breaks booting on Pi2.

@pelwell
Copy link
Contributor

pelwell commented Aug 22, 2015

There's a squash patch here that should theoretically restore normal service.

@popcornmix
Copy link
Collaborator

squash patch does fix the bcm2709_defconfig build.

Although the aim is to delete the old armctrl driver, that can't happen
while non-DT configurations are still supported. This commit enables
use of the new FIQ-enabled upstream irqchip driver when DT is enabled
on all BCM2835-based RPi platforms (Models A, B, A+, B+ & CM).

BCM2836-based platforms (Pi 2) will get the same treatment at a later
date, unless non-DT support has already been withdrawn.
@notro
Copy link
Contributor Author

notro commented Aug 22, 2015

Squashed and tested on ARCH_BCM2708.

pelwell added a commit that referenced this pull request Aug 22, 2015
Enable ARCH_BCM2835 for downstream use
@pelwell pelwell merged commit 615c263 into raspberrypi:rpi-4.2.y Aug 22, 2015
@notro notro deleted the noatags branch August 31, 2015 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants