BCM270x_DT: Configure UART using DT

[pelwell]

See: raspberrypi/firmware#393

[pelwell]

@notro - any comments?

[notro]

Any reason why you haven't just copied the node from bcm2835.dtsi?

See here for why arm,primecell-periphid is needed: https://lkml.org/lkml/2013/5/21/20

Looks like you missed bcm2708.c

[pelwell]

Just copying bcm2835.dtsi didn't work - I needed to add the clock information, but it was an iterative process to get there, during which I accumulated some cruft. I then pruned it back until I got the minimum changeset that still worked. It does work without that line, at least sometimes.

I've corrected those issues.

[notro]

I see now why bcm2835.dtsi doesn't contain clocks, they are added in the clock driver: http://lxr.free-electrons.com/source/drivers/clk/clk-bcm2835.c

The binding document states that when specified it should be 2 clocks: https://www.kernel.org/doc/Documentation/devicetree/bindings/serial/pl011.txt

What's the purpose of the reg-shift property?

[pelwell]

The reg-shift property is supposed to specify the spacing between registers on the bus. It is used by many device drivers, and it used for pl011 nodes in many ARM .dtsi files, but it isn't mentioned in the bindings and it isn't clear that the pl011 driver uses it.

I'll review the clocking in the morning.

Phil

On 24/03/2015 20:09, notro wrote:

I see now why bcm2835.dtsi doesn't contain clocks, they are added in the clock driver: http://lxr.free-electrons.com/source/drivers/clk/clk-bcm2835.c

The binding document states that when specified it should be 2 clocks: https://www.kernel.org/doc/Documentation/devicetree/bindings/serial/pl011.txt

What's the purpose of the reg-shift property?

—
Reply to this email directly or view it on GitHub #910 (comment).

[notro]

reg-shift

If I remove DT files and files not related to serial/tty, I'm left with these:

$ grep -Ir "reg-shift" .
drivers/tty/serial/8250/8250_dw.c:        if (!of_property_read_u32(np, "reg-shift", &val))
drivers/tty/serial/of_serial.c:   if (of_property_read_u32(np, "reg-shift", &prop) == 0)
[...]
arch/powerpc/kernel/legacy_serial.c:      rs = of_get_property(np, "reg-shift", NULL);
[...]
Documentation/devicetree/bindings/serial/nvidia,tegra20-hsuart.txt:       reg-shift = <2>;
Documentation/devicetree/bindings/serial/of-serial.txt:- reg-shift : quantity to shift the register offsets by.
Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt:- reg-shift : quantity to shift the register offsets by.  If this property is
[...]
Documentation/devicetree/bindings/xilinx.txt:       - reg-shift : A value of 2 is required

I can't find anything that should tie pl011 and reg-shift.
It could be that the DT files that have reg-shift also have one of the of_serial.c devices in it's compatible string.

BTW arm,primecell triggers some special handling:
of_platform_populate -> of_platform_bus_create -> of_amba_device_create
This is where arm,primecell-periphid is read.

[pelwell]

This patch is targeting 3.18, where the binding documentation (https://github.com/raspberrypi/linux/blob/rpi-3.18.y/Documentation/devicetree/bindings/serial/pl011.txt) says:

* ARM AMBA Primecell PL011 serial UART

Required properties:
- compatible: must be "arm,primecell", "arm,pl011"
- reg: exactly one register range with length 0x1000
- interrupts: exactly one interrupt specifier

Optional properties:
- pinctrl: When present, must have one state named "sleep"
       and one state named "default"
- clocks:  When present, must refer to exactly one clock named
       "apb_pclk"
- dmas:    When present, may have one or two dma channels.
       The first one must be named "rx", the second one
       must be named "tx".

See also bindings/arm/primecell.txt

I've removed the reference to reg-shift, swapped the compatible strings and pushed again.

[notro]

The 3.18 binding doc is wrong as this commit states: ARM: dt: fix up PL011 device tree bindings:

Make the map match the reality, the current binding text is
nonsense:

- The clock required for the clocking of the serial port
  must come first and is not optional (as the driver will
  otherwise proceed to grab and use the apb_pclk as uartclk),
  and the apb_pclk that clocks the logic must come second
  as the code will retrieve the first clock by index,
  whereas the PrimeCell but will explicitly look for
  "apb_pclk" so this can be specified later, as it is
  looked up by name.

I haven't understood the purpose of the clk-bcm2835 driver. Why not just put these clocks in the Device Tree?
But this PR has forced me to look closer into this, and I'm starting to think that it actually started as a stub driver for clocks, but later fixed DT clocks was used instead.
When the BCM2835 clock spec is released, this is where the code will go to control the clocks I guess.
(some details about amba and apb_pclk: https://lists.ozlabs.org/pipermail/devicetree-discuss/2011-May/005577.html)

So, since we want to close in on mainline, we can enable this driver and get the clocks that way.
Something like:

-obj-$(CONFIG_ARCH_BCM2835)     += clk-bcm2835.o
+obj-$(CONFIG_COMMON_CLK_BCM2835)   += clk-bcm2835.o

And then select COMMON_CLK_BCM2835 in the ARCH_BCM2xxx definitions.

Then we can just use the same uart node as in bcm2835.dtsi.
And maybe in the near future, we can include bcm2835.dtsi in bcm2708.dtsi and thus use more of the mainline code.

It's easier to add clocks to the DT, but at some point we will have to take this step, unless of course if the clocks will always be fixed (no clock control interface exposed in the future).

Another thing is that bcm2708.dtsi and bcm2709.dtsi are nearly identical, so the common part could be put in a file of it's own.

[notro]

It seems the clk-bcm2835 driver is a relic of the past:

ARM: bcm2835: add stub clock driver

This patch adds a minimal stub clock driver for the BCM2835. Its sole
purpose is to allow the PL011 AMBA clk_get() API calls to provide
something that looks enough like a clock that the driver probes and
operates correctly.

[PATCH 9/9] ARM: BCM283x: Register fixed clocks for uart in the DT

We were previously relying on the fixed clock registration in
clk-bcm2835, but there doesn't seem to be any real reason to not just
define it in the DT (and for the 2836 port, I would have needed to
change the clock's physical address in clk-bcm2835.c).  Also, because
we weren't registering the apb_pclk in clk-bcm2835 as a clock device,
we were picking up the uart clock node as apb_pclk by accident.

Reply by Stephen Warren:

Actually, to maintain DT ABI (new kernels working with old or new DT),
we probably need to keep the code in clk-bcm2835.c, but make it
conditional upon whether there's a clock node in the DT file.

[notro]

Based on what is going on upstream:

• [PATCH 4/7] ARM: bcm2835: Drop never-used clock-frequency property of uart0.
• [PATCH 6/7] ARM: bcm2835: Use the RPi firmware clocks for uart.

I have this suggestion:

diff --git a/arch/arm/boot/dts/bcm2708_common.dtsi b/arch/arm/boot/dts/bcm2708_common.dtsi
index 4bf6960..0fcc8ad 100644
--- a/arch/arm/boot/dts/bcm2708_common.dtsi
+++ b/arch/arm/boot/dts/bcm2708_common.dtsi
@@ -61,6 +61,16 @@
            #interrupt-cells = <2>;
        };

+       uart0: uart@7e201000 {
+           compatible = "brcm,bcm2835-pl011", "arm,pl011", "arm,primecell";
+           reg = <0x7e201000 0x1000>;
+           interrupts = <2 25>;
+           clocks = <&clk_uart0>, <&clk_apb_p>;
+           clock-names = "uartclk", "apb_pclk";
+//         arm,primecell-periphid = <0x00241011>;
+           status = "disabled";
+       };
+
        mmc: mmc@7e300000 {
            compatible = "brcm,bcm2835-mmc";
            reg = <0x7e300000 0x100>;
@@ -156,5 +166,21 @@
            clock-output-names = "spi";
            clock-frequency = <250000000>;
        };
+
+       clk_uart0: clock@3 {
+           compatible = "fixed-clock";
+           reg = <3>;
+           #clock-cells = <0>;
+           clock-output-names = "uart0_pclk";
+           clock-frequency = <3000000>;
+       };
+
+       clk_apb_p: clock@4 {
+           compatible = "fixed-clock";
+           reg = <4>;
+           #clock-cells = <0>;
+           clock-output-names = "apb_pclk";
+           clock-frequency = <126000000>;
+       };
    };
 };

About periphid

This is what we normally get without DT (rev3):

[    0.136106] dev:f1: ttyAMA0 at MMIO 0x20201000 (irq = 83, base_baud = 0) is a PL011 rev3

This is with arm,primecell-periphid = <0x00241011> (rev2):

[    0.136815] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 83, base_baud = 0) is a PL011 rev2

This is DT without the arm,primecell-periphid property (rev3):

[    0.136840] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 83, base_baud = 0) is a PL011 rev3

I don't know why mainline wants rev2.
Maybe this discussion is relevant: [PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

ID probing happens in amba_device_add
DT: of_amba_device_create

About the uart alias you add, is it for the overlay machinery?
I ask because pl011_probe_dt_alias uses serial: of_alias_get_id(np, "serial");

[notro]

Here's the reason for rev2:
ARM: bcm2835: override the HW UART periphid

Stephen Warren reported the recent commit 78506f2 (add support for
extended FIFO-size of PL011-r1p5) breaks the serial port on the
BCM2835 ARM SoC.

A UART compatible with the ARM PL011-r1p5 should have 32-deep FIFOs.
The BCM2835 UART just looks like an ARM PL011-r1p5, but has 16-deep
FIFOs just like PL011-r1p4 or earlier revisions. As a workaround for
this compatibility issue, this patch overrides the HW UART periphid
register values with the actually compatible UART periphid 0x00241011
(r1p3 or r1p4).

[notro]

@pelwell This is the last device to be moved (except uart1). It would be nice to get this done before bumping the kernel release version, as this would (hopefully) give us a stable Device Tree where we don't break the ABI anymore.

If you have other thing on your plate I'll be happy to make a PR, but we need to address the revision/primecell-periphid issue first. Mainline uses a different setup/revison than the one we're using today and I don't have the necessary knowledge to say what we should choose. Both seem to work, but I haven't tried to flood the port, I have only used the console.

@popcornmix I have seen that you have worked with some patches for the uart, what's your opinion?

I side effect of getting this in DT, is that we should be able to address #887 by using dt-blob.bin to set the uart pins to inputs, and then in a DT overlay we use pinctrl to set the pins back to uart. This should stop the 'Uncompressing Linux..' message.

[pelwell]

Superseded by PR #1005