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

BCM270X_DT: Use bcm283x.dtsi, bcm2835.dtsi and bcm2836.dtsi #1664

Merged
merged 1 commit into from Oct 10, 2016

Conversation

@notro
Copy link
Contributor

commented Oct 3, 2016

The mainline Device Tree files are quite close to downstream now.
This is a proposal to take advantage of that and take yet another "mainline" step.

I have made a diff file: bcm270x.dtsi instead of patching bcm283x.dtsi directly. I don't know what's the best maintainable solution. One benefit is that it is overly clear how we differ from mainline. The changes could also be included in bcm2708-rpi.dtsi as well, but IMO that's not so good since we loose the distinction made in mainline.

I haven't followed downstream so close the last months, so I don't know if this has any side effects on the overlays.
There's some more details in the commit message.

This is the diff introduced by this patch that applies to all dts. I have removed phandle changes that are due to node order changes and lines without change.

@@ -34,7 +34,6 @@
    aliases {
-       clocks = "/clocks";
-       leds = "/soc/leds";
+       leds = "/leds";
-       spi2 = "/soc/spi@7e2150C0";
+       spi2 = "/soc/spi@7e2150c0";
@@ -65,19 +64,31 @@
-   clocks: clocks {
+   clocks {
-       clk_osc: clock@6 {
+       clk_osc: clock@3 {
-           reg = <0x6>;
+           reg = <0x3>;
+       };
+   };
+
+   leds: leds {
+       compatible = "gpio-leds";
+
+       act_led: act {
+           gpios = <0x8 0x10 0x1>;
+           label = "led0";
+           linux,default-trigger = "mmc0";
+           linux,phandle = <0x19>;
+           phandle = <0x19>;
        };
    };
@@ -110,12 +121,12 @@
-       cprman: cprman@7e101000 {
+       cprman: clocks: cprman@7e101000 {
@@ -179,53 +190,52 @@
        gpiomem {
-           status = "okay";
        };

-       gpu: gpu {
+       gpu: vc4: gpu {
@@ -234,15 +244,17 @@ hdmi
+           interrupts = <0x2 0x8 0x2 0x9>;
        };

        hvs: hvs@7e400000 {
+           interrupts = <0x2 0x1>;
        };
@@ -313,18 +325,6 @@
-       leds: leds {
-           compatible = "gpio-leds";
-
-           act_led: act {
-               gpios = <0x7 0x10 0x1>;
-               label = "led0";
-               linux,default-trigger = "mmc0";
-               linux,phandle = <0x19>;
-               phandle = <0x19>;
-           };
-       };
-
@@ -347,18 +347,21 @@
        pixelvalve0: pixelvalve@7e206000 {
+           interrupts = <0x2 0xd>;
        };

        pixelvalve1: pixelvalve@7e207000 {
+           interrupts = <0x2 0xe>;
        };

        pixelvalve2: pixelvalve@7e807000 {
+           interrupts = <0x2 0xa>;
        };
@@ -386,7 +389,6 @@ random
-           status = "okay";
        };
@@ -444,18 +446,18 @@
-       spi0: spi@7e204000 {
+       spi0: spi: spi@7e204000 {
@@ -480,17 +482,17 @@
-       spi2: spi@7e2150C0 {
+       spi2: spi@7e2150c0 {
@@ -510,6 +512,8 @@
        usb: usb@7e980000 {
+           #address-cells = <0x1>;
+           #size-cells = <0x0>;
@@ -517,6 +521,7 @@
        v3d: v3d@7ec00000 {
+           interrupts = <0x1 0xa>;
        };
@@ -536,7 +541,6 @@ watchdog
-           status = "okay";
        };

The diff is produced like this, the resulting diff is sorted:

ARCH=arm ./scripts/dtc/dtx_diff arch/arm/boot/dts/bcm2708-rpi-b.dts > ../bcm283x-dtsi/org/bcm2708-rpi-b.dts
<patch>
ARCH=arm ./scripts/dtc/dtx_diff arch/arm/boot/dts/bcm2708-rpi-b.dts > ../bcm283x-dtsi/new/bcm2708-rpi-b.dts
./scripts/dtc/dtx_diff ../bcm283x-dtsi/org/bcm2708-rpi-b.dts ../bcm283x-dtsi/new/bcm2708-rpi-b.dts > ../bcm283x-dtsi/bcm2708-rpi-b.diff

Resulting full diffs:

I have only boot tested on Pi1 B+.

One thing I discovered is that we set timer clock-frequency in bcm2708.dtsi and bcm2709.dtsi, but apparently this should be avoided:

Documentation/devicetree/bindings/arm/arch_timer.txt

** CP15 Timer node properties:

- compatible : Should at least contain one of
        "arm,armv7-timer"
        "arm,armv8-timer"

- clock-frequency : The frequency of the main counter, in Hz. Should be present
  only where necessary to work around broken firmware which does not configure
  CNTFRQ on all CPUs to a uniform correct value. Use of this property is
  strongly discouraged; fix your firmware unless absolutely impossible.
@popcornmix

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2016

We do set CNTFRQ here:
https://github.com/raspberrypi/tools/blob/master/armstubs/armstub7.S#L82
https://github.com/raspberrypi/tools/blob/master/armstubs/armstub8.S#L72

but that doesn't exist on 2708 (no arch timers).
So perhaps it can be removed from bcm2709.dtsi but not bcm2708.dtsi?

@notro

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

Sorry I messed up: we set it in bcm2709.dtsi and bcm2710.dtsi.
Yeah, it looks like we can remove it.

I'll wait and see what @pelwell thinks about this PR before I spend more time on it.

@pelwell

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2016

I think the changes look good. All overlays still apply. It's just a shame that the multiple inclusions make it harder to follow.

If the bindings say not to include "clock-frequency" then we should take it out.

@notro

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2016

It's just a shame that the multiple inclusions make it harder to follow.

Do you see a better/different way of doing it?
Would you prefer that bcm270x.dtsi was instead a patch on bcm283x.dtsi? Perhaps it's better...

@pelwell

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2016

Without trying a few options I wasn't going to suggest there was a better way, just observing that we've spread the information over more levels in the hierarchy, which is good for avoiding replication and less good for legibility.

@notro notro force-pushed the notro:bcm283x-dtsi-1 branch from 48b6f74 to 5e073e5 Oct 5, 2016
@notro

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2016

Version 2

Removed clock-frequency property on the bcm2709/bcm2710.dtsi timer nodes.

@pelwell

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2016

Looks good to me.

@popcornmix popcornmix force-pushed the raspberrypi:rpi-4.8.y branch from ee19734 to c018644 Oct 9, 2016
@popcornmix

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2016

No objection here. Can you rebase?

The mainline Device Tree files are quite close to downstream now.
Let's use bcm283x.dtsi, bcm2835.dtsi and bcm2836.dtsi as base files
for our dts files.

Mainline dts files are based on these files:

          bcm2835-rpi.dtsi
  bcm2835.dtsi    bcm2836.dtsi
          bcm283x.dtsi

Current downstream are based on these:

  bcm2708.dtsi    bcm2709.dtsi    bcm2710.dtsi
             bcm2708_common.dtsi

This patch introduces this dependency:

  bcm2708.dtsi    bcm2709.dtsi
          bcm2708-rpi.dtsi
          bcm270x.dtsi
  bcm2835.dtsi    bcm2836.dtsi
          bcm283x.dtsi

And:
          bcm2710.dtsi
          bcm2708-rpi.dtsi
          bcm270x.dtsi
          bcm283x.dtsi

bcm270x.dtsi contains the downstream bcm283x.dtsi diff.
bcm2708-rpi.dtsi is the downstream version of bcm2835-rpi.dtsi.

Other changes:
- The led node has moved from /soc/leds to /leds. This is not a problem
  since the label is used to reference it.
- The clk_osc reg property changes from 6 to 3.
- The gpu nodes has their interrupt property set in the base file.
- the clocks label does not point to the /clocks node anymore, but
  points to the cprman node. This is not a problem since the overlays
  that use the clock node refer to it directly: target-path = "/clocks";
- some nodes now have 2 labels since mainline and downstream differs in
  this respect: cprman/clocks, spi0/spi, gpu/vc4.
- some nodes doesn't have an explicit status = "okay" since they're not
  disabled in the base file: watchdog and random.
- gpiomem doesn't need an explicit status = "okay".
- bcm2708-rpi-cm.dts got the hpd-gpios property from bcm2708_common.dtsi,
  it's now set directly in that file.
- bcm2709-rpi-2-b.dts has the timer node moved from /soc/timer to /timer.
- Removed clock-frequency property on the bcm{2709,2710}.dtsi timer nodes.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
@notro notro force-pushed the notro:bcm283x-dtsi-1 branch from 5e073e5 to 6e8485a Oct 10, 2016
@notro

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2016

Rebased

@popcornmix popcornmix merged commit 554c668 into raspberrypi:rpi-4.8.y Oct 10, 2016
@popcornmix

This comment has been minimized.

Copy link
Collaborator

commented Oct 11, 2016

@MilhouseVH has reported an issue when building LibreELEC with this commit:

Hi, got a problem with the latest RPi builds - if I include 6e8485a in the kernel, then kodi crashes at startup: http://sprunge.us/OADS
vcdbg log msg 2>&1| pastebinit => http://sprunge.us/bEfI
dmesg | pastebinit => http://sprunge.us/jRfb (edited)
Maybe a clue?

rpi22:~ # /usr/lib/kodi/kodi.bin --standalone -fs --lircdev /run/lirc/lircd
failed to add service - already in use?
Segmentation fault (core dumped)

I've also tried with a completely vanilla /flash/config.txt - same result. If I revert the linux commit, then Kodi works as normal
This is a crashlog from RPi1 showing same error when the linux commit is included: http://sprunge.us/UHAZ - it boots fine without it

Any ideas?

@notro

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2016

At first glance, this doesn't look good:

[    1.908542] sdhost-bcm2835 3f202000.sdhost: could not get clk
[    1.910784] mmc-bcm2835 3f300000.mmc: could not get clk

I also have the sdhost message, have to dig further

@notro

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2016

Those messages were just deferred probing.

I don't get this one on Pi2:

[    0.047431] uart-pl011 3f201000.serial: could not find pctldev for node /soc/gpio@7e200000/uart0_pins, deferring probe

The node ordering in the DT has probably changed due to this patch. But I thought that driver load order was the important thing, since the devices are already present when we get to module loading.

Do you know what kodi is trying to do when it crashes?

@popcornmix

This comment has been minimized.

Copy link
Collaborator

commented Oct 11, 2016

Yes, open display and init GL.
The firmware normally disables these features when it detects the nodes from the vc4-kms-v3d driver. It seems the new device tree is falsely triggering this.
@pelwell is just fixing the firmware to check more explicitly.

hello_triangle shows the failure more simply.

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Oct 11, 2016
firmware: arm_dt: Only mask interrupts for enabled DT nodes
See: raspberrypi/linux#1664
@popcornmix

This comment has been minimized.

Copy link
Collaborator

commented Oct 11, 2016

Firmware has been pushed that fixes this.

@MilhouseVH

This comment has been minimized.

Copy link

commented Oct 11, 2016

Thanks all, confirming LibreELEC now boots and starts Kodi on RPi1 and RPi2/3.

leeminghao pushed a commit to yudatun/vendor_raspberrypi_firmware that referenced this pull request Oct 18, 2016
firmware: arm_dt: Only mask interrupts for enabled DT nodes
See: raspberrypi/linux#1664
mkreisl added a commit to xbianonpi/xbian-package-firmware that referenced this pull request Oct 19, 2016
- vcimage: Fix detection of coherent addresses after IS_ALIAS_L1L2_NONALLOCATING change
  See: http://forum.kodi.tv/showthread.php?tid=269814&pid=2435907#pid2435907

- firmware: platform: Remove max_usb_current and default to enabled

- firmware: arm_dt: Only mask interrupts for enabled DT nodes
  See: raspberrypi/linux#1664

- firmware: ISP tuner: Lower rate at fast fps

- firmware: resize: Fix for no padding giving incorrect pitch
  See: https://www.raspberrypi.org/forums/viewtopic.php?f=66&t=162349

- firmware: arm_dt: Populate the /serial-number property
  See: raspberrypi/linux#1670

- firmware: deinterlace: Provide a mode where frame flags are exclusively used

- firmware: arm_loader: do not allow qpu usage when arm owns the 3d
  See: #669

- firmware: vcinclude: Fix macro for IS_ALIAS_L1L2_NONALLOCATING

- firmware: platform: Don't set kernel name explicitly for recovery.elf

- firmware: resize: Add a queue of input images to avoid dropped frames with opaque input

- firmware: resize: Set the direct_input flag when using mmal opaque mode

- firmware: arm_loader: Restrict automatic loading to LOWMEM
  See: raspberrypi/linux#1641

- firmware: RaspiVid: Make open_filename() unified for all outputs (video, imv, pts)
  See: raspberrypi/userland#338
neuschaefer pushed a commit to neuschaefer/raspi-binary-firmware that referenced this pull request Feb 27, 2017
firmware: arm_dt: Only mask interrupts for enabled DT nodes
See: raspberrypi/linux#1664
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.