| Message ID | 20221024100536.12874-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: debug-pl011.inc: Use macros instead of hardcoded values | expand |
> On 24 Oct 2022, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote: > > Make use of the macros defined in asm/pl011-uart.h instead of hardcoding > the values. Also, take the opportunity to fix the file extension in a > top-level comment. > > No functional change intended. > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- Hi Michal, Seems good to me! Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Hi Michal, On 24/10/2022 11:05, Michal Orzel wrote: > Make use of the macros defined in asm/pl011-uart.h instead of hardcoding > the values. Also, take the opportunity to fix the file extension in a > top-level comment. > > No functional change intended. > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> With one comment below: Acked-by: Julien Grall <jgrall@amazon.com> > --- > xen/arch/arm/arm64/debug-pl011.inc | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc > index 1928a2e3ffbb..d82f2f1de197 100644 > --- a/xen/arch/arm/arm64/debug-pl011.inc > +++ b/xen/arch/arm/arm64/debug-pl011.inc > @@ -1,5 +1,5 @@ > /* > - * xen/arch/arm/arm64/debug-pl011.S > + * xen/arch/arm/arm64/debug-pl011.inc > * > * PL011 specific debug code > * > @@ -16,6 +16,8 @@ > * GNU General Public License for more details. > */ > > + #include <asm/pl011-uart.h> > + > /* > * PL011 UART initialization > * xb: register which containts the UART base address > @@ -23,13 +25,13 @@ > */ > .macro early_uart_init xb, c > mov x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16) > - strh w\c, [\xb, #0x28] /* -> UARTFBRD (Baud divisor fraction) */ > + strh w\c, [\xb, #FBRD] /* -> UARTFBRD (Baud divisor fraction) */ > mov x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16) > - strh w\c, [\xb, #0x24] /* -> UARTIBRD (Baud divisor integer) */ > + strh w\c, [\xb, #IBRD] /* -> UARTIBRD (Baud divisor integer) */ > mov x\c, #0x60 /* 8n1 */ Can we introduce macro/define for 0x60? > - str w\c, [\xb, #0x2C] /* -> UARTLCR_H (Line control) */ > - ldr x\c, =0x00000301 /* RXE | TXE | UARTEN */ > - str w\c, [\xb, #0x30] /* -> UARTCR (Control Register) */ > + str w\c, [\xb, #LCR_H] /* -> UARTLCR_H (Line control) */ > + ldr x\c, =(RXE | TXE | UARTEN) > + str w\c, [\xb, #CR] /* -> UARTCR (Control Register) */ > .endm Cheers,
Hi Julien, On 16/11/2022 00:10, Julien Grall wrote: > > > Hi Michal, > > On 24/10/2022 11:05, Michal Orzel wrote: >> Make use of the macros defined in asm/pl011-uart.h instead of hardcoding >> the values. Also, take the opportunity to fix the file extension in a >> top-level comment. >> >> No functional change intended. >> >> Signed-off-by: Michal Orzel <michal.orzel@amd.com> > > With one comment below: > > Acked-by: Julien Grall <jgrall@amazon.com> > >> --- >> xen/arch/arm/arm64/debug-pl011.inc | 20 +++++++++++--------- >> 1 file changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc >> index 1928a2e3ffbb..d82f2f1de197 100644 >> --- a/xen/arch/arm/arm64/debug-pl011.inc >> +++ b/xen/arch/arm/arm64/debug-pl011.inc >> @@ -1,5 +1,5 @@ >> /* >> - * xen/arch/arm/arm64/debug-pl011.S >> + * xen/arch/arm/arm64/debug-pl011.inc >> * >> * PL011 specific debug code >> * >> @@ -16,6 +16,8 @@ >> * GNU General Public License for more details. >> */ >> >> + #include <asm/pl011-uart.h> >> + >> /* >> * PL011 UART initialization >> * xb: register which containts the UART base address >> @@ -23,13 +25,13 @@ >> */ >> .macro early_uart_init xb, c >> mov x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16) >> - strh w\c, [\xb, #0x28] /* -> UARTFBRD (Baud divisor fraction) */ >> + strh w\c, [\xb, #FBRD] /* -> UARTFBRD (Baud divisor fraction) */ >> mov x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16) >> - strh w\c, [\xb, #0x24] /* -> UARTIBRD (Baud divisor integer) */ >> + strh w\c, [\xb, #IBRD] /* -> UARTIBRD (Baud divisor integer) */ >> mov x\c, #0x60 /* 8n1 */ > > Can we introduce macro/define for 0x60? We could but I think this should not be part of this patch. The reason being, the arm32 code also uses hardcoded 0x60 so it should be changed as well. I can create a prerequisite patch introducing the macro and changing the arm32 code first unless you prefer to have everything in a single patch. As for the macro itself, because 8n1 only requires setting bits for WLEN (1 stop bit and no parity are 0 by default), we can do the following in pl011-uart.h: #define WLEN_8 0x60 and use it in the pl011-debug files (+ there is a question whether we should define WLEN_7-5 for completeness). > >> - str w\c, [\xb, #0x2C] /* -> UARTLCR_H (Line control) */ >> - ldr x\c, =0x00000301 /* RXE | TXE | UARTEN */ >> - str w\c, [\xb, #0x30] /* -> UARTCR (Control Register) */ >> + str w\c, [\xb, #LCR_H] /* -> UARTLCR_H (Line control) */ >> + ldr x\c, =(RXE | TXE | UARTEN) >> + str w\c, [\xb, #CR] /* -> UARTCR (Control Register) */ >> .endm > > Cheers, > > -- > Julien Grall ~Michal
On 16/11/2022 08:05, Michal Orzel wrote: > On 16/11/2022 00:10, Julien Grall wrote: >> >> >> Hi Michal, >> >> On 24/10/2022 11:05, Michal Orzel wrote: >>> Make use of the macros defined in asm/pl011-uart.h instead of hardcoding >>> the values. Also, take the opportunity to fix the file extension in a >>> top-level comment. >>> >>> No functional change intended. >>> >>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >> >> With one comment below: >> >> Acked-by: Julien Grall <jgrall@amazon.com> >> >>> --- >>> xen/arch/arm/arm64/debug-pl011.inc | 20 +++++++++++--------- >>> 1 file changed, 11 insertions(+), 9 deletions(-) >>> >>> diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc >>> index 1928a2e3ffbb..d82f2f1de197 100644 >>> --- a/xen/arch/arm/arm64/debug-pl011.inc >>> +++ b/xen/arch/arm/arm64/debug-pl011.inc >>> @@ -1,5 +1,5 @@ >>> /* >>> - * xen/arch/arm/arm64/debug-pl011.S >>> + * xen/arch/arm/arm64/debug-pl011.inc >>> * >>> * PL011 specific debug code >>> * >>> @@ -16,6 +16,8 @@ >>> * GNU General Public License for more details. >>> */ >>> >>> + #include <asm/pl011-uart.h> >>> + >>> /* >>> * PL011 UART initialization >>> * xb: register which containts the UART base address >>> @@ -23,13 +25,13 @@ >>> */ >>> .macro early_uart_init xb, c >>> mov x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16) >>> - strh w\c, [\xb, #0x28] /* -> UARTFBRD (Baud divisor fraction) */ >>> + strh w\c, [\xb, #FBRD] /* -> UARTFBRD (Baud divisor fraction) */ >>> mov x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16) >>> - strh w\c, [\xb, #0x24] /* -> UARTIBRD (Baud divisor integer) */ >>> + strh w\c, [\xb, #IBRD] /* -> UARTIBRD (Baud divisor integer) */ >>> mov x\c, #0x60 /* 8n1 */ >> >> Can we introduce macro/define for 0x60? > We could but I think this should not be part of this patch. > The reason being, the arm32 code also uses hardcoded 0x60 so it should be changed as well. > I can create a prerequisite patch introducing the macro and changing the arm32 code first unless you prefer to have everything in a single patch. I am fine with either prerequisite or a follow-up to define a macro and use it in both arm32/arm64. > > As for the macro itself, because 8n1 only requires setting bits for WLEN (1 stop bit and no parity are 0 by default), we can do > the following in pl011-uart.h: > #define WLEN_8 0x60 I think it would be clearer and easier to check the spec if the value is (_AC(0x3, U) << 5). > 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). Cheers,
Hi Julien, On 16/11/2022 14:41, Julien Grall wrote: > > > On 16/11/2022 08:05, Michal Orzel wrote: >> On 16/11/2022 00:10, Julien Grall wrote: >>> >>> >>> Hi Michal, >>> >>> On 24/10/2022 11:05, Michal Orzel wrote: >>>> Make use of the macros defined in asm/pl011-uart.h instead of hardcoding >>>> the values. Also, take the opportunity to fix the file extension in a >>>> top-level comment. >>>> >>>> No functional change intended. >>>> >>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>> >>> With one comment below: >>> >>> Acked-by: Julien Grall <jgrall@amazon.com> >>> >>>> --- >>>> xen/arch/arm/arm64/debug-pl011.inc | 20 +++++++++++--------- >>>> 1 file changed, 11 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc >>>> index 1928a2e3ffbb..d82f2f1de197 100644 >>>> --- a/xen/arch/arm/arm64/debug-pl011.inc >>>> +++ b/xen/arch/arm/arm64/debug-pl011.inc >>>> @@ -1,5 +1,5 @@ >>>> /* >>>> - * xen/arch/arm/arm64/debug-pl011.S >>>> + * xen/arch/arm/arm64/debug-pl011.inc >>>> * >>>> * PL011 specific debug code >>>> * >>>> @@ -16,6 +16,8 @@ >>>> * GNU General Public License for more details. >>>> */ >>>> >>>> + #include <asm/pl011-uart.h> >>>> + >>>> /* >>>> * PL011 UART initialization >>>> * xb: register which containts the UART base address >>>> @@ -23,13 +25,13 @@ >>>> */ >>>> .macro early_uart_init xb, c >>>> mov x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16) >>>> - strh w\c, [\xb, #0x28] /* -> UARTFBRD (Baud divisor fraction) */ >>>> + strh w\c, [\xb, #FBRD] /* -> UARTFBRD (Baud divisor fraction) */ >>>> mov x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16) >>>> - strh w\c, [\xb, #0x24] /* -> UARTIBRD (Baud divisor integer) */ >>>> + strh w\c, [\xb, #IBRD] /* -> UARTIBRD (Baud divisor integer) */ >>>> mov x\c, #0x60 /* 8n1 */ >>> >>> Can we introduce macro/define for 0x60? >> We could but I think this should not be part of this patch. >> The reason being, the arm32 code also uses hardcoded 0x60 so it should be changed as well. >> I can create a prerequisite patch introducing the macro and changing the arm32 code first unless you prefer to have everything in a single patch. > > I am fine with either prerequisite or a follow-up to define a macro and > use it in both arm32/arm64. I would then prefer to add a follow-up patch as this one is already acked and created for a different reason. > >> >> As for the macro itself, because 8n1 only requires setting bits for WLEN (1 stop bit and no parity are 0 by default), we can do >> the following in pl011-uart.h: >> #define WLEN_8 0x60 > > I think it would be clearer and easier to check the spec if the value is > (_AC(0x3, U) << 5). sounds good. > >> 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. 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). > > Cheers, > > -- > Julien Grall ~Michal
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. > 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. So we have two choices: 1) Remove the baud rate setting in the early uart code 2) Support the baud rate in the runtime driver I strongly prefer 1 so far because there are not any practical use to have a different baud rate for Xen and the firmware. Cheers,
Hi Julien, 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. > > So we have two choices: > 1) Remove the baud rate setting in the early uart code > 2) Support the baud rate in the runtime driver > > I strongly prefer 1 so far because there are not any practical use to > have a different baud rate for Xen and the firmware. > > Cheers, > > -- > Julien Grall ~Michal
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. Cheers,
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. 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. [1] It is not stated anywhere in our docs. [2] BTW: our docs/misc/arm/booting contains invalid links to the kernel docs. I guess this wants to be fixed. > > Cheers, > > -- > Julien Grall ~Michal
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. > > [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.
Hi, > On 17 Nov 2022, at 09:29, Julien Grall <julien@xen.org> 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. Just one remark here: In some cases the firmware might use a different console than the serial one (for example a graphical one) and Xen could be the first to use the serial console. So there might be cases where if we do not set a default value, the console will not be configured at all. I think the best solution here would be to have a CONFIG setting so that someone compiling Xen could choose between “keeping firmware config” or “force a config”. Cheers Bertrand > >> [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. > > -- > Julien Grall
On 17/11/2022 09:44, Bertrand Marquis wrote: > Hi, > >> On 17 Nov 2022, at 09:29, Julien Grall <julien@xen.org> 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. > > Just one remark here: In some cases the firmware might use a different console than the serial one (for example a graphical one) and Xen could be the first to use the serial console. > So there might be cases where if we do not set a default value, the console will not be configured at all. Fair point. > > I think the best solution here would be to have a CONFIG setting so that someone compiling Xen could choose between “keeping firmware config” or “force a config”. I don't like the idea of the CONFIG to force the baud rate at runtime. If you want to give the choice to the user, then this should be read from the firmware tables. In fact, the information should already be there. Cheers,
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
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. Cheers, [1] https://trello.com/invite/b/L54vXoqM/ATTI99c86a2718dec17b3b08f0de35fb3b5bC8729E45/xen
Hi Julien, > On 17 Nov 2022, at 10:11, Julien Grall <julien@xen.org> 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
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
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. > 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. Cheers,
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
diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc index 1928a2e3ffbb..d82f2f1de197 100644 --- a/xen/arch/arm/arm64/debug-pl011.inc +++ b/xen/arch/arm/arm64/debug-pl011.inc @@ -1,5 +1,5 @@ /* - * xen/arch/arm/arm64/debug-pl011.S + * xen/arch/arm/arm64/debug-pl011.inc * * PL011 specific debug code * @@ -16,6 +16,8 @@ * GNU General Public License for more details. */ + #include <asm/pl011-uart.h> + /* * PL011 UART initialization * xb: register which containts the UART base address @@ -23,13 +25,13 @@ */ .macro early_uart_init xb, c mov x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16) - strh w\c, [\xb, #0x28] /* -> UARTFBRD (Baud divisor fraction) */ + strh w\c, [\xb, #FBRD] /* -> UARTFBRD (Baud divisor fraction) */ mov x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16) - strh w\c, [\xb, #0x24] /* -> UARTIBRD (Baud divisor integer) */ + strh w\c, [\xb, #IBRD] /* -> UARTIBRD (Baud divisor integer) */ mov x\c, #0x60 /* 8n1 */ - str w\c, [\xb, #0x2C] /* -> UARTLCR_H (Line control) */ - ldr x\c, =0x00000301 /* RXE | TXE | UARTEN */ - str w\c, [\xb, #0x30] /* -> UARTCR (Control Register) */ + str w\c, [\xb, #LCR_H] /* -> UARTLCR_H (Line control) */ + ldr x\c, =(RXE | TXE | UARTEN) + str w\c, [\xb, #CR] /* -> UARTCR (Control Register) */ .endm /* @@ -39,8 +41,8 @@ */ .macro early_uart_ready xb, c 1: - ldrh w\c, [\xb, #0x18] /* <- UARTFR (Flag register) */ - tst w\c, #0x8 /* Check BUSY bit */ + ldrh w\c, [\xb, #FR] /* <- UARTFR (Flag register) */ + tst w\c, #BUSY /* Check BUSY bit */ b.ne 1b /* Wait for the UART to be ready */ .endm @@ -50,7 +52,7 @@ * wt: register which contains the character to transmit */ .macro early_uart_transmit xb, wt - strb \wt, [\xb] /* -> UARTDR (Data Register) */ + strb \wt, [\xb, #DR] /* -> UARTDR (Data Register) */ .endm /*
Make use of the macros defined in asm/pl011-uart.h instead of hardcoding the values. Also, take the opportunity to fix the file extension in a top-level comment. No functional change intended. Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- xen/arch/arm/arm64/debug-pl011.inc | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)