[PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

Define the OF early console for BCM2835 aux UART, which can be enabled
by passing "earlycon" on the boot command line. This UART is found on
BCM283x and BCM27xx SoCs, a.k.a. Raspberry Pi in its variants.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>

---

 drivers/tty/serial/8250/8250_bcm2835aux.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index 8ce700c1a7fc..6769cea2964a 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -135,6 +135,24 @@ static struct platform_driver bcm2835aux_serial_driver = {
 };
 module_platform_driver(bcm2835aux_serial_driver);
 
+#ifdef CONFIG_SERIAL_8250_CONSOLE
+
+static int __init early_bcm2835aux_setup(struct earlycon_device *device,
+					const char *options)
+{
+	if (!device->port.membase)
+		return -ENODEV;
+
+	device->port.iotype = UPIO_MEM32;
+	device->port.regshift = 2;
+
+	return early_serial8250_setup(device, NULL);
+}
+
+OF_EARLYCON_DECLARE(bcm2835aux, "brcm,bcm2835-aux-uart",
+		    early_bcm2835aux_setup);
+#endif
+
 MODULE_DESCRIPTION("BCM2835 auxiliar UART driver");
 MODULE_AUTHOR("Martin Sperl <kernel@martin.sperl.org>");
 MODULE_LICENSE("GPL v2");
-- 
2.24.1

Re: [PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[Lukas Wunner]

On Sun, Jan 26, 2020 at 01:33:14PM +0100, matthias.bgg@kernel.org wrote:
> +#ifdef CONFIG_SERIAL_8250_CONSOLE
> +
> +static int __init early_bcm2835aux_setup(struct earlycon_device *device,
> +					const char *options)
> +{
> +	if (!device->port.membase)
> +		return -ENODEV;
> +
> +	device->port.iotype = UPIO_MEM32;
> +	device->port.regshift = 2;
> +
> +	return early_serial8250_setup(device, NULL);
> +}
> +
> +OF_EARLYCON_DECLARE(bcm2835aux, "brcm,bcm2835-aux-uart",
> +		    early_bcm2835aux_setup);
> +#endif

Does this really work?  I also tried to get it working recently and
the system just hung on boot.  Looking at it with a JTAG debugger
showed that the bcm2835aux registers were inaccessible because
the mini UART wasn't enabled in the AUXENB register.

Maybe if you use OF_EARLYCON_DECLARE, the firmware recognizes that
serial1 is set as stdout-path and performs enablement of the mini UART?
Or are you using U-Boot which perhaps does the enablement?

I also saw in the JTAG debugger that the uartclk member contained
an incorrect value, so I'd expect that it has to be set as well in
early_bcm2835aux_setup().

I'll see to it that I give this patch a whirl when I return to the
office next week.

Thanks,

Lukas

Re: [PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[Matthias Brugger]

On 26/01/2020 14:12, Lukas Wunner wrote:
> On Sun, Jan 26, 2020 at 01:33:14PM +0100, matthias.bgg@kernel.org wrote:
>> +#ifdef CONFIG_SERIAL_8250_CONSOLE
>> +
>> +static int __init early_bcm2835aux_setup(struct earlycon_device *device,
>> +					const char *options)
>> +{
>> +	if (!device->port.membase)
>> +		return -ENODEV;
>> +
>> +	device->port.iotype = UPIO_MEM32;
>> +	device->port.regshift = 2;
>> +
>> +	return early_serial8250_setup(device, NULL);
>> +}
>> +
>> +OF_EARLYCON_DECLARE(bcm2835aux, "brcm,bcm2835-aux-uart",
>> +		    early_bcm2835aux_setup);
>> +#endif
> 
> Does this really work?  I also tried to get it working recently and
> the system just hung on boot.  Looking at it with a JTAG debugger
> showed that the bcm2835aux registers were inaccessible because
> the mini UART wasn't enabled in the AUXENB register.
> 
> Maybe if you use OF_EARLYCON_DECLARE, the firmware recognizes that
> serial1 is set as stdout-path and performs enablement of the mini UART?
> Or are you using U-Boot which perhaps does the enablement?

Yes I'm using U-Boot which enables the console for me. My understanding is that
the early console is thought as a re-use of the console the boot FW used for
logging. AFAIK for example it does not enable any needed clocks but expects
these to be enabled already.

Looking on the source code of U-Boot [1] I don't see that the AUXENB is written
somewhere, so I suppose that the FW should already has enabled the aux-uart.

I any case if it's just to set one bit, I think we can do that in
early_bcm2835aux_setup().

[1]
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/serial/serial_bcm283x_mu.c

> 
> I also saw in the JTAG debugger that the uartclk member contained
> an incorrect value, so I'd expect that it has to be set as well in
> early_bcm2835aux_setup().

In my case the clock was set by U-Boot already.

Regards,
Matthias

> 
> I'll see to it that I give this patch a whirl when I return to the
> office next week.
> 
> Thanks,
> 
> Lukas
> 

Re: [PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[Nicolas Saenz Julienne]

On Sun Jan 26, 2020 at 9:20 PM, Matthias Brugger wrote:
>
>
> On 26/01/2020 14:12, Lukas Wunner wrote:
> > On Sun, Jan 26, 2020 at 01:33:14PM +0100, matthias.bgg@kernel.org wrote:
> >> +#ifdef CONFIG_SERIAL_8250_CONSOLE
> >> +
> >> +static int __init early_bcm2835aux_setup(struct earlycon_device *device,
> >> +					const char *options)
> >> +{
> >> +	if (!device->port.membase)
> >> +		return -ENODEV;
> >> +
> >> +	device->port.iotype = UPIO_MEM32;
> >> +	device->port.regshift = 2;
> >> +
> >> +	return early_serial8250_setup(device, NULL);
> >> +}
> >> +
> >> +OF_EARLYCON_DECLARE(bcm2835aux, "brcm,bcm2835-aux-uart",
> >> +		    early_bcm2835aux_setup);
> >> +#endif
> > 
> > Does this really work?  I also tried to get it working recently and
> > the system just hung on boot.  Looking at it with a JTAG debugger
> > showed that the bcm2835aux registers were inaccessible because
> > the mini UART wasn't enabled in the AUXENB register.
> > 
> > Maybe if you use OF_EARLYCON_DECLARE, the firmware recognizes that
> > serial1 is set as stdout-path and performs enablement of the mini UART?
> > Or are you using U-Boot which perhaps does the enablement?
>
> Yes I'm using U-Boot which enables the console for me. My understanding
> is that
> the early console is thought as a re-use of the console the boot FW used
> for
> logging. AFAIK for example it does not enable any needed clocks but
> expects
> these to be enabled already.
>
> Looking on the source code of U-Boot [1] I don't see that the AUXENB is
> written
> somewhere, so I suppose that the FW should already has enabled the
> aux-uart.
>
> I any case if it's just to set one bit, I think we can do that in
> early_bcm2835aux_setup().
>
> [1]
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/serial/serial_bcm283x_mu.c
>
> > 
> > I also saw in the JTAG debugger that the uartclk member contained
> > an incorrect value, so I'd expect that it has to be set as well in
> > early_bcm2835aux_setup().
>
> In my case the clock was set by U-Boot already.

I'm testing this by booting directly from RPi4's bootloader. And it
works as long as I add this to config.txt:

enable_uart=1
gpu_freq=500

Which AFAIK blocks frequency scalin on the GPU and fixes the clock to a
point where the serial is set at 115200 bauds.

Ideally it'd be nice to be able to query the clock frequency, and
recalculate the divisors based on that. But I don't know if it's
feasible at that point in the boot process.

We could maybe leave a comment explaining what is expected from the
bootloader, given the different options around.

Regards,
Nicolas

Re: [PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[Lukas Wunner]

On Tue, Jan 28, 2020 at 01:42:21PM +0100, Nicolas Saenz Julienne wrote:
> I'm testing this by booting directly from RPi4's bootloader. And it
> works as long as I add this to config.txt:
> 
> enable_uart=1
> gpu_freq=500
> 
> Which AFAIK blocks frequency scalin on the GPU and fixes the clock to a
> point where the serial is set at 115200 bauds.
> 
> Ideally it'd be nice to be able to query the clock frequency, and
> recalculate the divisors based on that. But I don't know if it's
> feasible at that point in the boot process.

Well, we don't even support adjusting the baudrate *after* the kernel
has booted.

The problem is that in mainline, bcm2835_defconfig contains:
CONFIG_CPU_FREQ_DEFAULT_GOV_CONSERVATIVE=y

Likewise in the Foundation's downstream tree, bcmrpi_defconfig as well
as bcm2711_defconfig and bcm2709_defconfig contain:
CONFIG_CPU_FREQ_DEFAULT_GOV_POWERSAVE=y

In contrast to this, we set the following on Revolution Pi devices:
CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y

Downclocking influences not only the uart1 baud rate but also the
spi0 clock.  We attach Ethernet chips to spi0, throughput was
significantly worse with the ondemand governor (which is what we
used previously).  We felt that maximum Ethernet performance
outweighs the relatively small powersaving gains.

Thanks,

Lukas

Re: [PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[Nicolas Saenz Julienne]

On Tue Jan 28, 2020 at 3:19 PM, Lukas Wunner wrote:
> On Tue, Jan 28, 2020 at 01:42:21PM +0100, Nicolas Saenz Julienne wrote:
> > I'm testing this by booting directly from RPi4's bootloader. And it
> > works as long as I add this to config.txt:
> > 
> > enable_uart=1
> > gpu_freq=500
> > 
> > Which AFAIK blocks frequency scalin on the GPU and fixes the clock to a
> > point where the serial is set at 115200 bauds.
> > 
> > Ideally it'd be nice to be able to query the clock frequency, and
> > recalculate the divisors based on that. But I don't know if it's
> > feasible at that point in the boot process.
>
> Well, we don't even support adjusting the baudrate *after* the kernel
> has booted.

Don't 8250 uarts have a clk divisor? I've seen other 8250 earlycon
drivers do this (see 8250_ingenic.c), that said I think it's a lost
cause for us.

BTW did you had the oportunity to have a go at the patch?

> The problem is that in mainline, bcm2835_defconfig contains:
> CONFIG_CPU_FREQ_DEFAULT_GOV_CONSERVATIVE=y
>
> Likewise in the Foundation's downstream tree, bcmrpi_defconfig as well
> as bcm2711_defconfig and bcm2709_defconfig contain:
> CONFIG_CPU_FREQ_DEFAULT_GOV_POWERSAVE=y
>
> In contrast to this, we set the following on Revolution Pi devices:
> CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y
>
> Downclocking influences not only the uart1 baud rate but also the
> spi0 clock. We attach Ethernet chips to spi0, throughput was
> significantly worse with the ondemand governor (which is what we
> used previously). We felt that maximum Ethernet performance
> outweighs the relatively small powersaving gains.

In that regard I suggest you use the upstream cpufreq driver which
behaves properly in that regard. It disables GPU freq scaling, so as to
change CPU frequencies without SPI/I2C/UART issues.

Regards,
Nicolas

Re: [PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[Lukas Wunner]

On Thu, Jan 30, 2020 at 05:11:55PM +0100, Nicolas Saenz Julienne wrote:
> BTW did you had the oportunity to have a go at the patch?

I've just performed a quick test and it doesn't work for me.
If I add stdout-path = "serial1:115200n8"; to the chosen node,
I only get a regular console with this patch, not an earlycon.


> > The problem is that in mainline, bcm2835_defconfig contains:
> > CONFIG_CPU_FREQ_DEFAULT_GOV_CONSERVATIVE=y
> >
> > Likewise in the Foundation's downstream tree, bcmrpi_defconfig as well
> > as bcm2711_defconfig and bcm2709_defconfig contain:
> > CONFIG_CPU_FREQ_DEFAULT_GOV_POWERSAVE=y
> >
> > In contrast to this, we set the following on Revolution Pi devices:
> > CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y
> >
> > Downclocking influences not only the uart1 baud rate but also the
> > spi0 clock. We attach Ethernet chips to spi0, throughput was
> > significantly worse with the ondemand governor (which is what we
> > used previously). We felt that maximum Ethernet performance
> > outweighs the relatively small powersaving gains.
> 
> In that regard I suggest you use the upstream cpufreq driver which
> behaves properly in that regard. It disables GPU freq scaling, so as to
> change CPU frequencies without SPI/I2C/UART issues.

Okay, I'll take a look.  But the uart1 baudrate will still be wrong
if the firmware downclocks because of overheating, right?

Thanks,

Lukas

Re: [PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 2130 bytes --]

On Fri, 2020-01-31 at 16:24 +0100, Lukas Wunner wrote:
> On Thu, Jan 30, 2020 at 05:11:55PM +0100, Nicolas Saenz Julienne wrote:
> > BTW did you had the oportunity to have a go at the patch?
> 
> I've just performed a quick test and it doesn't work for me.
> If I add stdout-path = "serial1:115200n8"; to the chosen node,
> I only get a regular console with this patch, not an earlycon.

That's surprising, you're using u-boot isn't it? and the upstream device tree?

> > > The problem is that in mainline, bcm2835_defconfig contains:
> > > CONFIG_CPU_FREQ_DEFAULT_GOV_CONSERVATIVE=y
> > > 
> > > Likewise in the Foundation's downstream tree, bcmrpi_defconfig as well
> > > as bcm2711_defconfig and bcm2709_defconfig contain:
> > > CONFIG_CPU_FREQ_DEFAULT_GOV_POWERSAVE=y
> > > 
> > > In contrast to this, we set the following on Revolution Pi devices:
> > > CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y
> > > 
> > > Downclocking influences not only the uart1 baud rate but also the
> > > spi0 clock. We attach Ethernet chips to spi0, throughput was
> > > significantly worse with the ondemand governor (which is what we
> > > used previously). We felt that maximum Ethernet performance
> > > outweighs the relatively small powersaving gains.
> > 
> > In that regard I suggest you use the upstream cpufreq driver which
> > behaves properly in that regard. It disables GPU freq scaling, so as to
> > change CPU frequencies without SPI/I2C/UART issues.
> 
> Okay, I'll take a look.  But the uart1 baudrate will still be wrong
> if the firmware downclocks because of overheating, right?

You're right, it might happen. The way I understand it you're bound to leave
the GPU at it's lower frequency if you want to use those peripherals and for
them to be reliable. You could technically try to empirically fine tune the CPU
thermal trip point so as to make sure the upstream kernel cpufreq downclock
always kicks in before videocore's one. I'd actually like to see someone try
that. However short of using an RT kernel It think you'll never be 100% sure it
can never happen.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[Lukas Wunner]

On Mon, Feb 03, 2020 at 08:10:21PM +0100, Nicolas Saenz Julienne wrote:
> On Fri, 2020-01-31 at 16:24 +0100, Lukas Wunner wrote:
> > On Thu, Jan 30, 2020 at 05:11:55PM +0100, Nicolas Saenz Julienne wrote:
> > > BTW did you had the oportunity to have a go at the patch?
> > 
> > I've just performed a quick test and it doesn't work for me.
> > If I add stdout-path = "serial1:115200n8"; to the chosen node,
> > I only get a regular console with this patch, not an earlycon.
> 
> That's surprising, you're using u-boot isn't it? and the upstream device tree?

My apologies for the delay.

We boot the kernel directly from the Foundation's bootloader without
U-Boot as intermediary, and we use the downstream DT and kernel
(with RT & custom patches).

As far as I could see, the 8250_bcm2835aux UART wasn't even found
when the kernel searched for available earlycons.  The compatible
string in the DT does match the one in Matthias' patch.

I notice that Greg has already applied the patch to tty/next.
I'm currently stretched a little thin but I'll debug this further
once the dust has settled here.

Thanks,

Lukas

Re: [PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart

[Matthias Brugger]

On 12/02/2020 14:28, Lukas Wunner wrote:
> On Mon, Feb 03, 2020 at 08:10:21PM +0100, Nicolas Saenz Julienne wrote:
>> On Fri, 2020-01-31 at 16:24 +0100, Lukas Wunner wrote:
>>> On Thu, Jan 30, 2020 at 05:11:55PM +0100, Nicolas Saenz Julienne wrote:
>>>> BTW did you had the oportunity to have a go at the patch?
>>>
>>> I've just performed a quick test and it doesn't work for me.
>>> If I add stdout-path = "serial1:115200n8"; to the chosen node,
>>> I only get a regular console with this patch, not an earlycon.
>>
>> That's surprising, you're using u-boot isn't it? and the upstream device tree?
> 
> My apologies for the delay.
> 
> We boot the kernel directly from the Foundation's bootloader without
> U-Boot as intermediary, and we use the downstream DT and kernel
> (with RT & custom patches).
> 
> As far as I could see, the 8250_bcm2835aux UART wasn't even found
> when the kernel searched for available earlycons.  The compatible
> string in the DT does match the one in Matthias' patch.
> 

Can you provide your device tree please. I suspect that we find the culprit there.

Regards,
Matthias

> I notice that Greg has already applied the patch to tty/next.
> I'm currently stretched a little thin but I'll debug this further
> once the dust has settled here.
> 
> Thanks,
> 
> Lukas
>