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@xxxxxxxxxx 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