|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/arm: debug-pl011.inc: Use macros instead of hardcoded values
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. > >> >> [1] It is not stated anywhere in our docs. > > Our docs are not perfect. Patches are welcomed for improvement. > Although, I think the statement should only be for driver where we don't > set the baud rate. For the others, we should leave it as is unless you > can prove this is not necessary (we don't want to break existing setup). > >> >> [2] BTW: our docs/misc/arm/booting contains invalid links to the kernel >> docs. I guess >> this wants to be fixed. > > Patches are welcomed. > > [3] I do have a large list of more critical bugs that I will be happy to > share if you are looking for improving Xen. That is cool and such list would be great for everyone having some spare time (+ newcomers). Taking the opportunity of having a GitLab CI epics, I think it is worth adding such work items here: https://gitlab.com/groups/xen-project/-/epics?state=opened&page=1&sort=start_date_desc > > -- > Julien Grall ~Michal
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |