Re: [bootwrapper PATCH v2 08/13] Announce boot-wrapper mode / exception level

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Jan 17, 2022 at 02:39:13PM +0000, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:56:48 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> Hi Mark,
> 
> > When something goes wrong within the boot-wrapper, it can be very
> > helpful to know where we started from. Add an arch_announce() function
> > to log this early in the boot process. More information can be added
> > here in future.
> > 
> > This is logged ot the serial console as:
> > 
> > | Boot-wrapper v0.2
> > | Entered at EL3
> 
> I like that one, and apart from the (already existing) UART issue below,
> this looks fine:
> 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks!

> > diff --git a/common/platform.c b/common/platform.c
> > index 47bf547..80d0562 100644
> > --- a/common/platform.c
> > +++ b/common/platform.c
> > @@ -31,21 +31,25 @@
> >  #define V2M_SYS(reg)	((void *)SYSREGS_BASE + V2M_SYS_##reg)
> >  #endif
> >  
> > -void print_string(const char *str)
> > +void print_char(char c)
> >  {
> >  	uint32_t flags;
> >  
> > -	while (*str) {
> > -		do
> > -			flags = raw_readl(PL011(UARTFR));
> > -		while (flags & PL011_UARTFR_FIFO_FULL);
> > +	do {
> > +		flags = raw_readl(PL011(UARTFR));
> > +	} while (flags & PL011_UARTFR_FIFO_FULL);
> > +
> > +	raw_writel(c, PL011(UARTDR));
> >  
> > -		raw_writel(*str++, PL011(UARTDR));
> > +	do {
> > +		flags = raw_readl(PL011(UARTFR));
> > +	} while (flags & PL011_UARTFR_BUSY);
> 
> Apologies if that appears to be nitpicking over a totally pointless issue
> (given the nature of the *emulated* PL011 in the model), but:
> 
> I understand that this code has not changed, but this loop basically
> renders the FIFOs ineffective. Is this intended? 

Hmm... AFAICT, we started to enable the FIFOs in commit:

  26a17ad59544f026 ("bootwrapper: improve UART initialisation")

... but back then the boot-wrapper didn't print anything to the UART, so
that was purely about putting the UART into a sane state for the OS.

We only started to write to the UART in commit:

  7ff3872adb33b068 ("Rewrite platform initialisation in C")

... where we said nothing about the FIFOs.

Therefore, I think there was no intent either way to use/avoid the
FIFOs within the boot-wrapper.

> I mean we explicitly enable the FIFOs in init_uart(), but then poll
> here until the transmit FIFO becomes empty, after *every* character
> pushed. I see that we probably want the output to be synchronous, for
> debug reasons, but I wonder if this should be achieved via an extra
> uart_flush() routine, once per line output? So call uart_flush() just
> in print_string(), but not in print_char(), for instance?

Hmm... we also use print_char() directly from the aarch64 init code when
logging the exception level, so we'd need an explicit sync there, if we
do want that output before other things can go wrong.

Otherwise, I don't have strong feelings either way, but I would like to
have the output completed before print_*() return. We could add an
internal print_char_nosync() that print_{char,string}() use to handle
that.

I'm happy to take a follow-up patch for that, but for now I'm going to
leave this as-is.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel