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

[Julien Grall <julien@xxxxxxx>]

On 17/11/2022 09:44, Bertrand Marquis wrote:
> Hi,
>
> > On 17 Nov 2022, at 09:29, Julien Grall <julien@xxxxxxx> wrote:
> >
> >
> >
> > On 17/11/2022 08:34, Michal Orzel wrote:
> > > Hi Julien,
> > > On 16/11/2022 19:37, Julien Grall wrote:
> > > > 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.
> > > If we are relying on the firmware[1] to configure the baud rate, it is not very 
> > > wise
> > > not to rely on it to configure the LCR. Looking at the other serial drivers in 
> > > Xen,
> > > we have a real mismatch in what is being configured. Some of the drivers (omap, 
> > > imx),
> > > apart from setting 8n1 also set the baud rate explicitly to 115200 and almost 
> > > all of them
> > > do set 8n1. In that case we will not benefit too much from fixing just pl011.
> > It is not great that Xen hardcode the baud rate (I can't remember whether there 
> > was a reason), but I don't think the consistency is necessary here (see more 
> > below).
> >
> > > On the other hand, Xen follows the zImage/Image protocols for ARM [2] which do 
> > > not
> > > state that serial port initializing is something mandatory. This could indicate 
> > > that
> > > the firmware does not really need to configure the serial.
> > The firmware doesn't need to configure the serial and yes in theory Xen should 
> > configure the baud rate and parity based on the firmware table.
> >
> > However, this is a trade off between complexity and benefits. The patch I 
> > mentioned earlier has been removed nearly 6 years ago and I haven't seen anyone 
> > reporting any issues.
> >
> > Hence why I think for the PL011 it is not worth looking [3] at the baud rate 
> > and instead removing it completely in the early PL011 code as well.
> >
> > That said, if you feel strongly adding support for baud rate then I will be 
> > happy to review the patch.
> Just one remark here: In some cases the firmware might use a different console 
> than the serial one (for example a graphical one) and Xen could be the first to 
> use the serial console.
> So there might be cases where if we do not set a default value, the console 
> will not be configured at all.
Fair point.

> I think the best solution here would be to have a CONFIG setting so that someone compiling Xen could choose 
> between “keeping firmware config” or “force a config”.
I don't like the idea of the CONFIG to force the baud rate at runtime. 
If you want to give the choice to the user, then this should be read 
from the firmware tables.
In fact, the information should already be there.

Cheers,

--
Julien Grall