|
[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
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: 1. Redefine CONFIG_EARLY_UART_INIT to be CONFIG_EARLY_UART_PL011_INIT and mark it as n by default 2. Keep CONFIG_EARLY_UART_INIT so that future drivers can use it (?) and mark it as n by default 2. Completely remove early_uart_init > >> >>> >>> [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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.com%2Fgroups%2Fxen-project%2F-%2Fepics%3Fstate%3Dopened%26page%3D1%26sort%3Dstart_date_desc&data=05%7C01%7Cmichal.orzel%40amd.com%7Ce2f65ddb895243bdb9cc08dac881acba%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638042756535884326%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5Hf%2BXW3nogjzasoTQ821OPAjJLJVVofyGpb0LNxRAto%3D&reserved=0 > >> >> -- >> Julien Grall > > ~Michal > ~Michal
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |