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

From: Matthias Brugger <matthias.bgg@gmail.com>
To: Lukas Wunner <lukas@wunner.de>, matthias.bgg@kernel.org
Cc: gregkh@linuxfoundation.org, jslaby@suse.com,
	nsaenzjulienne@suse.de, linux-kernel@vger.kernel.org,
	linux-serial@vger.kernel.org,
	linux-rpi-kernel@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	Matthias Brugger <mbrugger@suse.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Ray Jui <rjui@broadcom.com>,
	Scott Branden <sbranden@broadcom.com>,
	Stephen Boyd <swboyd@chromium.org>,
	bcm-kernel-feedback-list@broadcom.com
Subject: Re: [PATCH] serial: 8250_early: Add earlycon for BCM2835 aux uart
Date: Sun, 26 Jan 2020 21:20:33 +0100	[thread overview]
Message-ID: <88af0fca-90d1-58ff-406d-73f185eb8b7e@gmail.com> (raw)
In-Reply-To: <20200126131238.c65zj3wo3srafa7z@wunner.de>

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
> 