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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 18/11/2022 10:58, Julien Grall wrote:
> 
> 
> On 17/11/2022 12:52, Michal Orzel wrote:
>>
>>
>> On 17/11/2022 10:53, Michal Orzel wrote:
>>>
>>>
>>> Hi Julien,
>>>
>>> On 17/11/2022 10:29, Julien Grall 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.
>>> I'm not in favor of this approach either. That said, I will prepare patches 
>>> to remove
>>> CONFIG_EARLY_UART_PL011_BAUD_RATE and its usage in early printk code as we 
>>> agreed earlier.
>>> As for the LCR setting, I will keep it in early printk code to maintain the 
>>> same behavior as
>>> runtime driver who sets them.
>> Actually, there is one more thing to consider.
>> early_uart_init, even though it also sets LCR apart from the baud rate, is 
>> called when CONFIG_EARLY_UART_INIT is set.
>> The latter depends on EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0.
>> If we remove EARLY_UART_PL011_BAUD_RATE, we need to decide when do we want 
>> early_uart_init to be called. It is defined only for pl011
>> (it is also defined for meson but this is an unreachable code, as 
>> EARLY_UART_PL011 is 0 for meson), so we have the following options:
> 
> Good spot. I am not sure why the function was defined for Meson if it
> does nothing. I would drop it but keep the comment explaining why we
> don't have the helper.
Hmm, other drivers do not have such a comment but I can leave it there if you 
want.
Although, it should then be slightly modified to avoid ambiguity after removing 
macro:
/*
 * No need for early_uart_init, as UART has already been initialized
 * by Firmware, for instance by TF-A.
 */

> 
>> 2. Keep CONFIG_EARLY_UART_INIT so that future drivers can use it (?) and 
>> mark it as n by default
> 
> Based on the discussion with Bertrand, I would keep
> CONFIG_EARLY_UART_INIT in case someone wants to use a different UART for
> Xen and the firmware output.
> 
> Also, I would like to revise my opinion from earlier. I now think we
> should keep the baud rate part in early PL011 code even this means
> inconsistency between the early and runtime driver.
Ok, in this case I will just create a series containing this patch without any 
modifications,
the meson fix + macro for 8n1 to be used in early code as agreed earlier.

> 
> Cheers,
> 
> --
> Julien Grall

~Michal