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
