BCM270x: Make uart1 work with Device Tree

[notro]

Add uart1 to Device Tree. Enable it in AUXENB when it's used.
Remove old platform device.

This is how I have tested this on Pi1 and Pi2:

config

CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_SERIAL_OF_PLATFORM=y

DT overlay

/dts-v1/;
/plugin/;

/ {
    compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";

    fragment@0 {
        target = <&gpio>;
        __overlay__ {
            uart1_pins: uart1_pins {
                brcm,pins = <14 15>;
                brcm,function = <2>; /* alt5 */
            };
        };
    };

    fragment@1 {
        target = <&uart1>;
        __overlay__ {
            pinctrl-names = "default";
            pinctrl-0 = <&uart1_pins>;
            status = "okay";
        };
    };
};

Add to /boot/cmdline.txt

console=ttyS0,115200

dmesg

[    0.260750] bcm2708: Mini UART enabled
[...]
[    0.278795] Serial: AMBA PL011 UART driver
[    0.283324] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 83, base_baud = 0) is a PL011 rev2
[    0.292376] console [ttyAMA0] enabled
[...]
[    1.191862] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[    1.201851] uart-pl011 20201000.uart: no DMA platform data
[    1.209983] console [ttyS0] disabled
[    1.215464] 20215040.uart: ttyS0 at MMIO 0x20215040 (irq = 29, base_baud = 31250000) is a 16550
[    1.901232] console [ttyS0] enabled

To get a login prompt on ttyS0, /etc/inittab would need some attention.

[popcornmix]

Looks okay to me. @pelwell ?

[dpslwk]

Would it be possible to now have the 8250 kernel drive enabled in bcmrpi_defconfig?

I believe that would be very useful as currently even with this change to allow uart1 to work with DT a user still needs to custom compile the kernel and modules to make use of it.

Im not sure if this can be done via a module option (I think these are correct)

CONFIG_SERIAL_8250=m
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_SERIAL_OF_PLATFORM=y

or if it should be just built in using the options given by @notro

[popcornmix]

Can you try rpi-update. The options should be in.

[dpslwk]

The update works a treat and i have successfully used the port with minicom.
However one think i have noticed is that the /dev/ttyS0 is showing up even with out an overlay to enable the device, I'm not sure if this is the behaviour of the kernel compile options of the lower down DT settings.
I'm also not sure if this is a good change or not? As in /dev/ttyS0 start appearing in all pi distro's.
There are a lot of people out there that will have created /dev/ttyS0<>/dev/ttyAMA0 symlinks for use with the Arduino IDE
I intended to try compiling the kernel with a few different option for the 8250 driver and see what behaviour I can get

[pelwell]

I was just about to ask if you could help out ondrej1024 on the forum, but I see you have done that already. Thank you.

If you would like to submit a pull request with your overlay and updates to the Makefile and README then I'll happily merge it.

I'll need to look into the automatic creation of /dev/ttyS0 even without the overlay - it is not the desired behaviour.

[pelwell]

I now understand why ttyS0 is being created without an overlay, and I've got it working(ish).

The value of CONFIG_SERIAL_8250_RUNTIME_UARTS (currently 1) is the number of uarts initially configured at boot time, and causes /dev/ttyS0 to be created (but with what properties?) regardless of the DT content. Fortunately this config value can be overridden by a module parameter, so it may be necessary to use that as well as DT to enable the interface, unless we decide that having a (presumably) non-functioning ttyS0 is preferable to having to manipulate cmdline.txt.

The other remaining issue is that the baudrate seems to be out by a factor of two - I'm looking at clock rates now.

[dpslwk]

So i just attached the logic analyser on my setup here (Compute in OpenPi) and the baud rate is correct for 9600 and 115200
I know the baud rate is tied to the 250MHz system clock, so if thats different you would need to override the (in the uart1 DT section)

clock-frequency = <500000000>;

I've not tried on the Pi2 and don't know if that has a different system clock speed?

[pelwell]

Yes, the baud rate discrepancy was caused by using the core clock directly, which didn't take account of the non-standard clock divisor. Future firmware releases will correctly patch the uart1 clock value, but be aware that if you are overclocking the core you will need to set force_turbo=1 to get a sensible baud rate.

[popcornmix]

rpi-update firmware should contain @pelwell's fix

[dpslwk]

New overlay works for me, getting good baudrates out of it and the pin override works

So that just leave the creation of /dev/ttyS0 question

[pelwell]

The /dev/ttyS0 creation is triggered by the config parameter SERIAL_8250_RUNTIME_UARTS, which we are setting to 1. Making this a 0 stops the device being created without and with the DT overlay.

There are two parameters - SERIAL_8250_NR_UARTS and SERIAL_8250_RUNTIME_UARTS. The former is simply the size of an array, and the latter says how many entries in the array will be used. It seems odd to have a "RUNTIME" value like that compiled in, but it makes (slightly) more sense when you know that it can be overridden using the "nr_uarts" module parameter.

Creating the platform device from DT only has the effect of initialising one of the pre-defined UART ports, so in order to get the behaviour we want, i.e. either one configured UART or no UARTs, it is necessary to:

1. change the config so that SERIAL_8250_RUNTIME_UARTS is 0, and
2. load the overlay and add "8250.nr_uarts=1" to the kernel command line.

That sounds tedious, but I have a trick up my sleeve. When you write to the "/chosen/bootargs" DT property from an overlay the new value is appended to the kernel command line (plus, this happens before the extra settings supplied by the firmware). This makes it possible to set the module parameter from the overlay.

I've pushed an update to the rpi-4.0.y branch.

[notro]

@pelwell I'm just amazed by what your overlay mechanism can accomplish. It solves one problem after another with ease.

[dpslwk]

@pelwell that looks like a very nice little work around

[pelwell]

Oddly, applying the same patch (specifically setting SERIAL_8250_RUNTIME_UARTS=0) to the rpi-4.1.y branch causes it to crash before the serial console comes up. I'll have to return to this later.

[pelwell]

The 4.1 8250 driver has a bug that causes it to crash if nr_uarts is zero. With this bug patched, the overlay works as expected. Both are now on rpi-4.1.y.

[notro]

Yes, the baud rate discrepancy was caused by using the core clock directly, which didn't take account of the non-standard clock divisor. Future firmware releases will correctly patch the uart1 clock value, but be aware that if you are overclocking the core you will need to set force_turbo=1 to get a sensible baud rate.

What does this mean?
Should clock-frequency on uart1 really be 250MHz?

The reason I ask is because Eric has a proposal for the aux enable problem using gated clocks.
And as I understand it, it will return 250MHz if we use it for uart1.
http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-September/002261.html

[pelwell]

This means that that UART1 isn't an exact clone of a 8250 UART. In particular, the clock divisor is calculated differently. A standard 8250 derives the baud rate as clock/(divisor*16), whereas the BCM2835 mini UART uses clock/(divisor*8). This means that if you want to use the standard driver then you need to lie about the clock frequency, providing a value twice the real value, in order for a suitable divisor to be calculated.

The mini UART clock is tied (through a gate) to the core clock. It will therefore normally be 250MHz, but that will change if the VPU is overclocked. The current Pi firmware uses the uart1_clkrate DT parameter, passing in double the value of core_freq. Unless Eric's cprman driver (or the 8250 driver) can be modified in some way, it sounds like UART1 will run twice as quickly as it should.

[notro]

Maybe we can solve that problem by inserting a "fixed-factor-clock" to double the frequency?

  clocks: cprman@7e101000 {
    compatible = "brcm,bcm2835-cprman";
    #clock-cells = <1>;
    reg = <0x7e101000 0x2000>;
    clocks = <&clk_osc>;
  };

  aux_clocks: aux-clocks@0x7e215004 {
    compatible = "brcm,bcm2835-aux-clock";
    #clock-cells = <1>;
    reg = 0x7e215004 0x4>;
    clocks = <&clk_osc>;
    clocks = <&clocks BCM2835_CLOCK_VPU>;
  };

  uart1: uart@7e215040 {
    compatible = "brcm,bcm2835-aux-uart", "ns16550";
    reg = <0x7e215040 0x40>;
    interrupts = <1 29>;
//    clock-frequency = <500000000>;
    clocks = <&clk_uart1>;
    reg-shift = <2>;
    no-loopback-test;
    status = "disabled";
        };

  clocks: clocks {
    clk_uart1: clock@6 {
      compatible = "fixed-factor-clock";
      clocks = <&aux_clocks BCM2835_AUX_CLOCK_UART>;
      #clock-cells = <0>;
      clock-div = <1>;
      clock-mult = <2>;
    };
  };

http://lxr.free-electrons.com/source/drivers/clk/clk-fixed-factor.c
https://www.kernel.org/doc/Documentation/devicetree/bindings/clock/fixed-factor-clock.txt

[pelwell]

That's neat (although I'd have called it "fixed-scaled-clock").

[pelwell]

This patch seems to do the right thing, and removes the need for the uart1_clkrate parameter:

diff --git a/arch/arm/boot/dts/bcm2708_common.dtsi b/arch/arm/boot/dts/bcm2708_c
index f096f45..4d1cc67 100644
--- a/arch/arm/boot/dts/bcm2708_common.dtsi
+++ b/arch/arm/boot/dts/bcm2708_common.dtsi
@@ -165,7 +165,7 @@
                        compatible = "brcm,bcm2835-aux-uart", "ns16550";
                        reg = <0x7e215040 0x40>;
                        interrupts = <1 29>;
-                       clock-frequency = <500000000>;
+                       clocks = <&clk_uart1>;
                        reg-shift = <2>;
                        no-loopback-test;
                        status = "disabled";
@@ -294,6 +294,14 @@
                         clock-output-names = "pwm";
                         clock-frequency = <100000000>;
                };
+
+               clk_uart1: clock@6 {
+                       compatible = "fixed-factor-clock";
+                       clocks = <&clk_core>;
+                       #clock-cells = <0>;
+                       clock-div = <1>;
+                       clock-mult = <2>;
+               };
        };

        __overrides__ {

[notro]

I didn't think this far, that we could benefit from it already.

This is maybe the best way to work around this discrepency in the almost-like 8250 uart.
I think it's better to do this explicitly like this than to let the bcm2835 clock driver lie about the frequence by doubling it just to please a driver.
And writing a new driver will generate a lot of code I'm afraid. The 16 divisor was in a lot of places in serial_core.c. So a new driver would probably just double the clock and be done with it and use the serial core as-is.

[msperl]

Note that the drivers/tty/serial/8250/8250_bcm2835aux.c driver (which essentially just doubles clock and is accepted into https://git.kernel.org/cgit/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=bdc5f300958062a766518b81d5378f837149d5c1 but not merged into upstream yet) is a candidate to use in the future. Note that it relies on drivers/clk/bcm/clk-bcm2835-aux.c and drivers/clk/bcm/clk-bcm2835.c, some of which we may already use with the openGL framework...

But while trying to see if setting the console works on ttyS0 I found that there is a clock issue when using the aux uart and then enabling the ttyAMA0 device the system freezes.

So I wonder if this behavior has been seen during development/testing of this downstream solution?

[pelwell]

Apart from the fact that miniUART/UART1/ttyS0 uses the core clock which changes in turbo mode without force_turbo=1, I've not seen any downstream problems with UARTs and clocks.

If you can reproduce the freeze with the downstream kernel then please create a new issue.

[msperl]

thanks - so you have not seen it...

[dpslwk]

It’s know that the system will freeze any time the port is opened and there is not a device connected to pins for it to talk to,
and if not a device at least an external pul up on the RXD1_pin
but i’ve not had any issue using both ports at the same time during normal OS use.
I have not tried using uart1 as a console.

‘RepRap’ Matt

On 10 Feb 2016, at 11:56, msperl notifications@github.com wrote:

thanks - so you have not seen it...

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

[msperl]

as for VPU: i wonder if there is any means to get the clock framework to issue a notification when the
VPU clock is getting changed - but I guess that is something left right now to the firmware, so there is no notification of the kernel anyway... We may get there eventually.

But that is also similar to the SPI clock which also relies on VPU clock, but there the impact is smaller, as spi recalculates the divider on each spi_message, so it only impacts the current in-flight transaction...

Thanks for the info - I will build latest 4.5 and then report to the corresponding lists/Eric.