Re: [PATCH] xen/arm: debug-pl011.inc: Use macros instead of hardcoded values

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 16/11/2022 18:05, Michal Orzel wrote:
> On 16/11/2022 16:56, Julien Grall wrote:
> > On 16/11/2022 14:45, Michal Orzel wrote:
> > > Hi Julien,
> > Hi Michal,
> >
> > > > > and use it in the pl011-debug files (+ there is a question whether we should 
> > > > > define WLEN_7-5 for completeness).
> > > > I would not define WLEN_7-5. That said, I wonder if we really need to
> > > > set the baud rate & co here?
> > > >
> > > > AFAICT the runtime driver never touch them. The reasoning is the
> > > > firmware is responsible to configure the serial. Therefore, I would
> > > > consider to drop the code (setting UARTCR might still be necessary).
> > > I do not really agree because the current behavior was done on purpose.
> > EARLY_UART_PL011_BAUD_RATE is only used for very early debugging (this
> > is protected by CONFIG_DEBUG and CONFIG_EXPERT). This is not a
> > production ready code.
> I am fully aware of it. I just found it useful but I understand the global 
> reasoning.
>
> > > At the moment early_uart_init is called only if EARLY_UART_PL011_BAUD_RATE is 
> > > set to a value != 0.
> > > This is done in order to have flexibility to either stick to what 
> > > firmware/bootloader configured or to change this
> > > configuration by specifying the EARLY_UART_PL011_BAUD_RATE (useful when you do 
> > > not know what
> > > the firmware configured).
> > The chances are that you want to use the baud rate that was configured
> > by the firmware. Otherwise, you would need to change the configuration
> > of minicom (or whatever you used) to get proper output for the firmware
> > and then Xen.
> >
> > Furthermore, as I wrote before, the runtime driver doesn't configure the
> > baud rate. This was removed in Xen 4.7 (see commit 2048e17ca9df
> > "drivers/pl011: Don't configure baudrate") because it was buggy and this
> > code is not simple.
> >
> > So it makes no sense to configure the baud rate when using early printk
> > but not the runtime driver.
> Ok, so we will get rid of EARLY_UART_PL011_BAUD_RATE config and setting the bd
> in the early uart code. Now, what about setting "8n1"? The runtime driver sets 
> them
> as well as the early code. It can also be set to a different value from the 
> firmware
> (unlikely but it can happen I think). In any case, if we decide to do what the 
> runtime driver
> does, I reckon setting LCR_H should be kept in early code.
Good question. I think, you would end up with the same issue I mentioned 
above if the firmware and Xen have different line control registers 
(tools like minicom/screen would ask for it).
So I am on the fence here. In one way, it seems pointless keep it. But 
on the other hand, Xen has always set it. So I have no data to prove 
this will be fine everywhere.
Cheers,

--
Julien Grall