|
[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 Nov 2022, at 10:11, Julien Grall <julien@xxxxxxx> wrote: > > Hi, > > On 17/11/2022 09:53, Michal Orzel wrote: >> 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 > > I already have a Trello board I created a few years ago when I left Arm [1] > with 30+ issues. I have another 30+ in my private board. > > I can try to clean-up the one I have in my private board. But I will need > some help for transfer everything to gitlab. Do not hesitate to ping me if you want some help on that :-) Cheers Bertrand > > Cheers, > > [1] > https://trello.com/invite/b/L54vXoqM/ATTI99c86a2718dec17b3b08f0de35fb3b5bC8729E45/xen > > -- > Julien Grall
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |