* [PATCH] ns16550: Fix DM serial operation with non-DM SPL @ 2023-01-17 12:09 Andre Przywara 2023-01-17 13:12 ` Sergei Antonov 2023-01-17 13:15 ` Tom Rini 0 siblings, 2 replies; 12+ messages in thread From: Andre Przywara @ 2023-01-17 12:09 UTC (permalink / raw) To: Simon Glass, Tom Rini; +Cc: u-boot, linux-sunxi, Samuel Holland Commit 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") moved some NS16550 configuration variables into Kconfig. Among those there is CONFIG_SYS_NS16550_REG_SIZE, which used to be only defined for SPL build runs, but now is always set (thanks for Kconfig). However this breaks the gating logic in ns16550.h, where we *override* this variable for DM build, as we learn this setting from the DT instead. As a consequence, we did the register shift twice: once when building the register struct (as required for non-DM SPL builds), but then also again in the driver after we parsed the reg-shift DT property. Change the logic to match what the comment says: only observe CONFIG_SYS_NS16550_REG_SIZE when not using DM, and ignore it otherwise. This fixes U-Boot proper for all sunxi boards, since they are relying on this driver being build non-DM for the SPL, but DM for U-Boot proper. Fixes: 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- Hi, this is admittedly a quick fix, to get sunxi booting again. This whole code around the register access looks somewhat bonkers, to be honest, but at the moment I don't have time to rework this. Another possible "quicker fix" would be use a separate variable for the register shift, so that we don't have to redefine a Kconfig variable. Another idea would be to get rid of the struct for the registers altogether, to remove the hacks about when to shift. Cheers, Andre include/ns16550.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/ns16550.h b/include/ns16550.h index 243226fc3d9..e9e2aeedd16 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -26,11 +26,12 @@ #include <linux/types.h> -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) +#if CONFIG_IS_ENABLED(DM_SERIAL) /* * For driver model we always use one byte per register, and sort out the * differences in the driver */ +#undef CONFIG_SYS_NS16550_REG_SIZE #define CONFIG_SYS_NS16550_REG_SIZE (-1) #endif -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 12:09 [PATCH] ns16550: Fix DM serial operation with non-DM SPL Andre Przywara @ 2023-01-17 13:12 ` Sergei Antonov 2023-01-17 16:13 ` Andre Przywara 2023-01-17 16:16 ` Tom Rini 2023-01-17 13:15 ` Tom Rini 1 sibling, 2 replies; 12+ messages in thread From: Sergei Antonov @ 2023-01-17 13:12 UTC (permalink / raw) To: Andre Przywara; +Cc: Simon Glass, Tom Rini, u-boot, linux-sunxi, Samuel Holland On Tue, 17 Jan 2023 at 15:10, Andre Przywara <andre.przywara@arm.com> wrote: > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > +#if CONFIG_IS_ENABLED(DM_SERIAL) > /* > * For driver model we always use one byte per register, and sort out the > * differences in the driver > */ > +#undef CONFIG_SYS_NS16550_REG_SIZE > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > #endif What if I have DM_SERIAL and a 16550 UART with 32-bit registers? Before 9591b63531fa the register size of 8 was enforced for DM_SERIAL. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 13:12 ` Sergei Antonov @ 2023-01-17 16:13 ` Andre Przywara 2023-01-18 13:08 ` Sergei Antonov 2023-01-17 16:16 ` Tom Rini 1 sibling, 1 reply; 12+ messages in thread From: Andre Przywara @ 2023-01-17 16:13 UTC (permalink / raw) To: Sergei Antonov; +Cc: Simon Glass, Tom Rini, u-boot, linux-sunxi, Samuel Holland On Tue, 17 Jan 2023 16:12:54 +0300 Sergei Antonov <saproj@gmail.com> wrote: Hi Sergei, > On Tue, 17 Jan 2023 at 15:10, Andre Przywara <andre.przywara@arm.com> wrote: > > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > > +#if CONFIG_IS_ENABLED(DM_SERIAL) > > /* > > * For driver model we always use one byte per register, and sort out the > > * differences in the driver > > */ > > +#undef CONFIG_SYS_NS16550_REG_SIZE > > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > > #endif > > What if I have DM_SERIAL and a 16550 UART with 32-bit registers? So does that break for you? Because it should still work, I think, since (as the comment says) we just use the struct to get the register *number*. The actual register size is worked out from the DT, and multiplied in later. > Before 9591b63531fa the register size of 8 was enforced for DM_SERIAL. 8? Or 4? What does your reg-shift DT property say? Is that in your DT? Cheers, Andre ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 16:13 ` Andre Przywara @ 2023-01-18 13:08 ` Sergei Antonov 0 siblings, 0 replies; 12+ messages in thread From: Sergei Antonov @ 2023-01-18 13:08 UTC (permalink / raw) To: Andre Przywara; +Cc: Simon Glass, Tom Rini, u-boot, linux-sunxi, Samuel Holland On Tue, 17 Jan 2023 at 19:14, Andre Przywara <andre.przywara@arm.com> wrote: > > On Tue, 17 Jan 2023 16:12:54 +0300 > Sergei Antonov <saproj@gmail.com> wrote: > > Hi Sergei, > > > On Tue, 17 Jan 2023 at 15:10, Andre Przywara <andre.przywara@arm.com> wrote: > > > > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > > > +#if CONFIG_IS_ENABLED(DM_SERIAL) > > > /* > > > * For driver model we always use one byte per register, and sort out the > > > * differences in the driver > > > */ > > > +#undef CONFIG_SYS_NS16550_REG_SIZE > > > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > > > #endif > > > > What if I have DM_SERIAL and a 16550 UART with 32-bit registers? > > So does that break for you? > Because it should still work, I think, since (as the comment says) we just > use the struct to get the register *number*. The actual register size is > worked out from the DT, and multiplied in later. > > > Before 9591b63531fa the register size of 8 was enforced for DM_SERIAL. > > 8? Or 4? Oh, I meant 8-bit. So under DM_SERIAL: CONFIG_SYS_NS16550_REG_SIZE is -1 UART_REG(x) is unsigned char x > What does your reg-shift DT property say? Is that in your DT? I did not have "reg-shift" property. Thanks for telling about it. Now that I have these properties in DT my UART works. reg-io-width = <4>; reg-shift = <2>; I also had to add this config parameter to make debug uart work: CONFIG_DEBUG_UART_SHIFT=2 Thanks! ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 13:12 ` Sergei Antonov 2023-01-17 16:13 ` Andre Przywara @ 2023-01-17 16:16 ` Tom Rini 1 sibling, 0 replies; 12+ messages in thread From: Tom Rini @ 2023-01-17 16:16 UTC (permalink / raw) To: Sergei Antonov Cc: Andre Przywara, Simon Glass, u-boot, linux-sunxi, Samuel Holland [-- Attachment #1: Type: text/plain, Size: 944 bytes --] On Tue, Jan 17, 2023 at 04:12:54PM +0300, Sergei Antonov wrote: > On Tue, 17 Jan 2023 at 15:10, Andre Przywara <andre.przywara@arm.com> wrote: > > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > > +#if CONFIG_IS_ENABLED(DM_SERIAL) > > /* > > * For driver model we always use one byte per register, and sort out the > > * differences in the driver > > */ > > +#undef CONFIG_SYS_NS16550_REG_SIZE > > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > > #endif > > What if I have DM_SERIAL and a 16550 UART with 32-bit registers? > Before 9591b63531fa the register size of 8 was enforced for DM_SERIAL. Note that right now, there are some broken cases still, which is why I made: https://patchwork.ozlabs.org/project/uboot/patch/20230110161946.3816866-5-trini@konsulko.com/ but that doesn't fix _this_ case, either. Can you test the above patch on your platform however? Thanks! -- Tom [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 12:09 [PATCH] ns16550: Fix DM serial operation with non-DM SPL Andre Przywara 2023-01-17 13:12 ` Sergei Antonov @ 2023-01-17 13:15 ` Tom Rini 2023-01-17 15:55 ` Tom Rini 2023-01-17 16:09 ` Andre Przywara 1 sibling, 2 replies; 12+ messages in thread From: Tom Rini @ 2023-01-17 13:15 UTC (permalink / raw) To: Andre Przywara; +Cc: Simon Glass, u-boot, linux-sunxi, Samuel Holland [-- Attachment #1: Type: text/plain, Size: 2449 bytes --] On Tue, Jan 17, 2023 at 12:09:38PM +0000, Andre Przywara wrote: > Commit 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > moved some NS16550 configuration variables into Kconfig. > Among those there is CONFIG_SYS_NS16550_REG_SIZE, which used to be only > defined for SPL build runs, but now is always set (thanks for Kconfig). > However this breaks the gating logic in ns16550.h, where we *override* > this variable for DM build, as we learn this setting from the DT instead. > > As a consequence, we did the register shift twice: once when building > the register struct (as required for non-DM SPL builds), but then also > again in the driver after we parsed the reg-shift DT property. > > Change the logic to match what the comment says: only observe > CONFIG_SYS_NS16550_REG_SIZE when not using DM, and ignore it otherwise. > > This fixes U-Boot proper for all sunxi boards, since they are relying > on this driver being build non-DM for the SPL, but DM for U-Boot proper. > > Fixes: 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Hi, > > this is admittedly a quick fix, to get sunxi booting again. This whole > code around the register access looks somewhat bonkers, to be honest, > but at the moment I don't have time to rework this. > Another possible "quicker fix" would be use a separate variable for the > register shift, so that we don't have to redefine a Kconfig variable. > Another idea would be to get rid of the struct for the registers > altogether, to remove the hacks about when to shift. > > Cheers, > Andre > > include/ns16550.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/ns16550.h b/include/ns16550.h > index 243226fc3d9..e9e2aeedd16 100644 > --- a/include/ns16550.h > +++ b/include/ns16550.h > @@ -26,11 +26,12 @@ > > #include <linux/types.h> > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > +#if CONFIG_IS_ENABLED(DM_SERIAL) > /* > * For driver model we always use one byte per register, and sort out the > * differences in the driver > */ > +#undef CONFIG_SYS_NS16550_REG_SIZE > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > #endif Does: https://patchwork.ozlabs.org/project/uboot/patch/20230110161946.3816866-5-trini@konsulko.com/ work for you as well? -- Tom [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 13:15 ` Tom Rini @ 2023-01-17 15:55 ` Tom Rini 2023-01-17 16:09 ` Andre Przywara 1 sibling, 0 replies; 12+ messages in thread From: Tom Rini @ 2023-01-17 15:55 UTC (permalink / raw) To: Andre Przywara; +Cc: Simon Glass, u-boot, linux-sunxi, Samuel Holland [-- Attachment #1: Type: text/plain, Size: 2752 bytes --] On Tue, Jan 17, 2023 at 08:15:18AM -0500, Tom Rini wrote: > On Tue, Jan 17, 2023 at 12:09:38PM +0000, Andre Przywara wrote: > > > Commit 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > moved some NS16550 configuration variables into Kconfig. > > Among those there is CONFIG_SYS_NS16550_REG_SIZE, which used to be only > > defined for SPL build runs, but now is always set (thanks for Kconfig). > > However this breaks the gating logic in ns16550.h, where we *override* > > this variable for DM build, as we learn this setting from the DT instead. > > > > As a consequence, we did the register shift twice: once when building > > the register struct (as required for non-DM SPL builds), but then also > > again in the driver after we parsed the reg-shift DT property. > > > > Change the logic to match what the comment says: only observe > > CONFIG_SYS_NS16550_REG_SIZE when not using DM, and ignore it otherwise. > > > > This fixes U-Boot proper for all sunxi boards, since they are relying > > on this driver being build non-DM for the SPL, but DM for U-Boot proper. > > > > Fixes: 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > Hi, > > > > this is admittedly a quick fix, to get sunxi booting again. This whole > > code around the register access looks somewhat bonkers, to be honest, > > but at the moment I don't have time to rework this. > > Another possible "quicker fix" would be use a separate variable for the > > register shift, so that we don't have to redefine a Kconfig variable. > > Another idea would be to get rid of the struct for the registers > > altogether, to remove the hacks about when to shift. > > > > Cheers, > > Andre > > > > include/ns16550.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/include/ns16550.h b/include/ns16550.h > > index 243226fc3d9..e9e2aeedd16 100644 > > --- a/include/ns16550.h > > +++ b/include/ns16550.h > > @@ -26,11 +26,12 @@ > > > > #include <linux/types.h> > > > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > > +#if CONFIG_IS_ENABLED(DM_SERIAL) > > /* > > * For driver model we always use one byte per register, and sort out the > > * differences in the driver > > */ > > +#undef CONFIG_SYS_NS16550_REG_SIZE > > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > > #endif > > Does: > https://patchwork.ozlabs.org/project/uboot/patch/20230110161946.3816866-5-trini@konsulko.com/ > work for you as well? This got me to double check why my pine64 wasn't in CI, again, and now I see that no, this patch doesn't solve that case. -- Tom [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 13:15 ` Tom Rini 2023-01-17 15:55 ` Tom Rini @ 2023-01-17 16:09 ` Andre Przywara 2023-01-17 16:14 ` Tom Rini 1 sibling, 1 reply; 12+ messages in thread From: Andre Przywara @ 2023-01-17 16:09 UTC (permalink / raw) To: Tom Rini; +Cc: Simon Glass, u-boot, linux-sunxi, Samuel Holland On Tue, 17 Jan 2023 08:15:18 -0500 Tom Rini <trini@konsulko.com> wrote: Hi Tom, > On Tue, Jan 17, 2023 at 12:09:38PM +0000, Andre Przywara wrote: > > > Commit 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > moved some NS16550 configuration variables into Kconfig. > > Among those there is CONFIG_SYS_NS16550_REG_SIZE, which used to be only > > defined for SPL build runs, but now is always set (thanks for Kconfig). > > However this breaks the gating logic in ns16550.h, where we *override* > > this variable for DM build, as we learn this setting from the DT instead. > > > > As a consequence, we did the register shift twice: once when building > > the register struct (as required for non-DM SPL builds), but then also > > again in the driver after we parsed the reg-shift DT property. > > > > Change the logic to match what the comment says: only observe > > CONFIG_SYS_NS16550_REG_SIZE when not using DM, and ignore it otherwise. > > > > This fixes U-Boot proper for all sunxi boards, since they are relying > > on this driver being build non-DM for the SPL, but DM for U-Boot proper. > > > > Fixes: 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > Hi, > > > > this is admittedly a quick fix, to get sunxi booting again. This whole > > code around the register access looks somewhat bonkers, to be honest, > > but at the moment I don't have time to rework this. > > Another possible "quicker fix" would be use a separate variable for the > > register shift, so that we don't have to redefine a Kconfig variable. > > Another idea would be to get rid of the struct for the registers > > altogether, to remove the hacks about when to shift. > > > > Cheers, > > Andre > > > > include/ns16550.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/include/ns16550.h b/include/ns16550.h > > index 243226fc3d9..e9e2aeedd16 100644 > > --- a/include/ns16550.h > > +++ b/include/ns16550.h > > @@ -26,11 +26,12 @@ > > > > #include <linux/types.h> > > > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > > +#if CONFIG_IS_ENABLED(DM_SERIAL) > > /* > > * For driver model we always use one byte per register, and sort out the > > * differences in the driver > > */ > > +#undef CONFIG_SYS_NS16550_REG_SIZE > > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > > #endif > > Does: > https://patchwork.ozlabs.org/project/uboot/patch/20230110161946.3816866-5-trini@konsulko.com/ > work for you as well? Unfortunately not. This changed condition there does not trigger for sunxi. If I hack it that it applies, but only to the proper build, it works, though. But I don't feel like adding to this #if even more ;-) Cheers, Andre ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 16:09 ` Andre Przywara @ 2023-01-17 16:14 ` Tom Rini 2023-01-17 22:15 ` Andre Przywara 0 siblings, 1 reply; 12+ messages in thread From: Tom Rini @ 2023-01-17 16:14 UTC (permalink / raw) To: Andre Przywara; +Cc: Simon Glass, u-boot, linux-sunxi, Samuel Holland [-- Attachment #1: Type: text/plain, Size: 3363 bytes --] On Tue, Jan 17, 2023 at 04:09:14PM +0000, Andre Przywara wrote: > On Tue, 17 Jan 2023 08:15:18 -0500 > Tom Rini <trini@konsulko.com> wrote: > > Hi Tom, > > > On Tue, Jan 17, 2023 at 12:09:38PM +0000, Andre Przywara wrote: > > > > > Commit 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > > moved some NS16550 configuration variables into Kconfig. > > > Among those there is CONFIG_SYS_NS16550_REG_SIZE, which used to be only > > > defined for SPL build runs, but now is always set (thanks for Kconfig). > > > However this breaks the gating logic in ns16550.h, where we *override* > > > this variable for DM build, as we learn this setting from the DT instead. > > > > > > As a consequence, we did the register shift twice: once when building > > > the register struct (as required for non-DM SPL builds), but then also > > > again in the driver after we parsed the reg-shift DT property. > > > > > > Change the logic to match what the comment says: only observe > > > CONFIG_SYS_NS16550_REG_SIZE when not using DM, and ignore it otherwise. > > > > > > This fixes U-Boot proper for all sunxi boards, since they are relying > > > on this driver being build non-DM for the SPL, but DM for U-Boot proper. > > > > > > Fixes: 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > --- > > > Hi, > > > > > > this is admittedly a quick fix, to get sunxi booting again. This whole > > > code around the register access looks somewhat bonkers, to be honest, > > > but at the moment I don't have time to rework this. > > > Another possible "quicker fix" would be use a separate variable for the > > > register shift, so that we don't have to redefine a Kconfig variable. > > > Another idea would be to get rid of the struct for the registers > > > altogether, to remove the hacks about when to shift. > > > > > > Cheers, > > > Andre > > > > > > include/ns16550.h | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/ns16550.h b/include/ns16550.h > > > index 243226fc3d9..e9e2aeedd16 100644 > > > --- a/include/ns16550.h > > > +++ b/include/ns16550.h > > > @@ -26,11 +26,12 @@ > > > > > > #include <linux/types.h> > > > > > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > > > +#if CONFIG_IS_ENABLED(DM_SERIAL) > > > /* > > > * For driver model we always use one byte per register, and sort out the > > > * differences in the driver > > > */ > > > +#undef CONFIG_SYS_NS16550_REG_SIZE > > > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > > > #endif > > > > Does: > > https://patchwork.ozlabs.org/project/uboot/patch/20230110161946.3816866-5-trini@konsulko.com/ > > work for you as well? > > Unfortunately not. This changed condition there does not trigger for > sunxi. If I hack it that it applies, but only to the proper build, it > works, though. But I don't feel like adding to this #if even more ;-) I've got a version that does work, now, on my pine64 plus, and I've asked Quentin to test his Rockchip platform that was also broken, before I post a new version there. A better long term fix would likely start by seeing which non-DM_SERIAL cases we have, and what they need, still. -- Tom [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 16:14 ` Tom Rini @ 2023-01-17 22:15 ` Andre Przywara 2023-01-17 22:23 ` Tom Rini 0 siblings, 1 reply; 12+ messages in thread From: Andre Przywara @ 2023-01-17 22:15 UTC (permalink / raw) To: Tom Rini; +Cc: Simon Glass, u-boot, linux-sunxi, Samuel Holland On Tue, 17 Jan 2023 11:14:35 -0500 Tom Rini <trini@konsulko.com> wrote: > On Tue, Jan 17, 2023 at 04:09:14PM +0000, Andre Przywara wrote: > > On Tue, 17 Jan 2023 08:15:18 -0500 > > Tom Rini <trini@konsulko.com> wrote: > > > > Hi Tom, > > > > > On Tue, Jan 17, 2023 at 12:09:38PM +0000, Andre Przywara wrote: > > > > > > > Commit 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > > > moved some NS16550 configuration variables into Kconfig. > > > > Among those there is CONFIG_SYS_NS16550_REG_SIZE, which used to be only > > > > defined for SPL build runs, but now is always set (thanks for Kconfig). > > > > However this breaks the gating logic in ns16550.h, where we *override* > > > > this variable for DM build, as we learn this setting from the DT instead. > > > > > > > > As a consequence, we did the register shift twice: once when building > > > > the register struct (as required for non-DM SPL builds), but then also > > > > again in the driver after we parsed the reg-shift DT property. > > > > > > > > Change the logic to match what the comment says: only observe > > > > CONFIG_SYS_NS16550_REG_SIZE when not using DM, and ignore it otherwise. > > > > > > > > This fixes U-Boot proper for all sunxi boards, since they are relying > > > > on this driver being build non-DM for the SPL, but DM for U-Boot proper. > > > > > > > > Fixes: 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > --- > > > > Hi, > > > > > > > > this is admittedly a quick fix, to get sunxi booting again. This whole > > > > code around the register access looks somewhat bonkers, to be honest, > > > > but at the moment I don't have time to rework this. > > > > Another possible "quicker fix" would be use a separate variable for the > > > > register shift, so that we don't have to redefine a Kconfig variable. > > > > Another idea would be to get rid of the struct for the registers > > > > altogether, to remove the hacks about when to shift. > > > > > > > > Cheers, > > > > Andre > > > > > > > > include/ns16550.h | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/ns16550.h b/include/ns16550.h > > > > index 243226fc3d9..e9e2aeedd16 100644 > > > > --- a/include/ns16550.h > > > > +++ b/include/ns16550.h > > > > @@ -26,11 +26,12 @@ > > > > > > > > #include <linux/types.h> > > > > > > > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > > > > +#if CONFIG_IS_ENABLED(DM_SERIAL) > > > > /* > > > > * For driver model we always use one byte per register, and sort out the > > > > * differences in the driver > > > > */ > > > > +#undef CONFIG_SYS_NS16550_REG_SIZE > > > > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > > > > #endif > > > > > > Does: > > > https://patchwork.ozlabs.org/project/uboot/patch/20230110161946.3816866-5-trini@konsulko.com/ > > > work for you as well? > > > > Unfortunately not. This changed condition there does not trigger for > > sunxi. If I hack it that it applies, but only to the proper build, it > > works, though. But I don't feel like adding to this #if even more ;-) > > I've got a version that does work, now, on my pine64 plus, and I've > asked Quentin to test his Rockchip platform that was also broken, before > I post a new version there. Is it something like this: diff --git a/include/ns16550.h b/include/ns16550.h index 243226fc3d9..74e619e0614 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -26,15 +26,12 @@ #include <linux/types.h> -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) /* * For driver model we always use one byte per register, and sort out the * differences in the driver */ -#define CONFIG_SYS_NS16550_REG_SIZE (-1) -#endif - -#if defined(CONFIG_NS16550_DYNAMIC) || defined(CONFIG_DEBUG_UART) +#if CONFIG_IS_ENABLED(DM_SERIAL) || defined(CONFIG_NS16550_DYNAMIC) || \ + defined(CONFIG_DEBUG_UART) #define UART_REG(x) unsigned char x #else #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0) Cheers, Andre > A better long term fix would likely start by seeing which non-DM_SERIAL > cases we have, and what they need, still. > ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 22:15 ` Andre Przywara @ 2023-01-17 22:23 ` Tom Rini 2023-01-17 23:35 ` Andre Przywara 0 siblings, 1 reply; 12+ messages in thread From: Tom Rini @ 2023-01-17 22:23 UTC (permalink / raw) To: Andre Przywara; +Cc: Simon Glass, u-boot, linux-sunxi, Samuel Holland [-- Attachment #1: Type: text/plain, Size: 4611 bytes --] On Tue, Jan 17, 2023 at 10:15:46PM +0000, Andre Przywara wrote: > On Tue, 17 Jan 2023 11:14:35 -0500 > Tom Rini <trini@konsulko.com> wrote: > > > On Tue, Jan 17, 2023 at 04:09:14PM +0000, Andre Przywara wrote: > > > On Tue, 17 Jan 2023 08:15:18 -0500 > > > Tom Rini <trini@konsulko.com> wrote: > > > > > > Hi Tom, > > > > > > > On Tue, Jan 17, 2023 at 12:09:38PM +0000, Andre Przywara wrote: > > > > > > > > > Commit 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > > > > moved some NS16550 configuration variables into Kconfig. > > > > > Among those there is CONFIG_SYS_NS16550_REG_SIZE, which used to be only > > > > > defined for SPL build runs, but now is always set (thanks for Kconfig). > > > > > However this breaks the gating logic in ns16550.h, where we *override* > > > > > this variable for DM build, as we learn this setting from the DT instead. > > > > > > > > > > As a consequence, we did the register shift twice: once when building > > > > > the register struct (as required for non-DM SPL builds), but then also > > > > > again in the driver after we parsed the reg-shift DT property. > > > > > > > > > > Change the logic to match what the comment says: only observe > > > > > CONFIG_SYS_NS16550_REG_SIZE when not using DM, and ignore it otherwise. > > > > > > > > > > This fixes U-Boot proper for all sunxi boards, since they are relying > > > > > on this driver being build non-DM for the SPL, but DM for U-Boot proper. > > > > > > > > > > Fixes: 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > > --- > > > > > Hi, > > > > > > > > > > this is admittedly a quick fix, to get sunxi booting again. This whole > > > > > code around the register access looks somewhat bonkers, to be honest, > > > > > but at the moment I don't have time to rework this. > > > > > Another possible "quicker fix" would be use a separate variable for the > > > > > register shift, so that we don't have to redefine a Kconfig variable. > > > > > Another idea would be to get rid of the struct for the registers > > > > > altogether, to remove the hacks about when to shift. > > > > > > > > > > Cheers, > > > > > Andre > > > > > > > > > > include/ns16550.h | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/include/ns16550.h b/include/ns16550.h > > > > > index 243226fc3d9..e9e2aeedd16 100644 > > > > > --- a/include/ns16550.h > > > > > +++ b/include/ns16550.h > > > > > @@ -26,11 +26,12 @@ > > > > > > > > > > #include <linux/types.h> > > > > > > > > > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > > > > > +#if CONFIG_IS_ENABLED(DM_SERIAL) > > > > > /* > > > > > * For driver model we always use one byte per register, and sort out the > > > > > * differences in the driver > > > > > */ > > > > > +#undef CONFIG_SYS_NS16550_REG_SIZE > > > > > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > > > > > #endif > > > > > > > > Does: > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230110161946.3816866-5-trini@konsulko.com/ > > > > work for you as well? > > > > > > Unfortunately not. This changed condition there does not trigger for > > > sunxi. If I hack it that it applies, but only to the proper build, it > > > works, though. But I don't feel like adding to this #if even more ;-) > > > > I've got a version that does work, now, on my pine64 plus, and I've > > asked Quentin to test his Rockchip platform that was also broken, before > > I post a new version there. > > Is it something like this: > > diff --git a/include/ns16550.h b/include/ns16550.h > index 243226fc3d9..74e619e0614 100644 > --- a/include/ns16550.h > +++ b/include/ns16550.h > @@ -26,15 +26,12 @@ > > #include <linux/types.h> > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && > !defined(CONFIG_SYS_NS16550_REG_SIZE) /* > * For driver model we always use one byte per register, and sort out > the > * differences in the driver > */ > -#define CONFIG_SYS_NS16550_REG_SIZE (-1) > -#endif > - > -#if defined(CONFIG_NS16550_DYNAMIC) || defined(CONFIG_DEBUG_UART) > +#if CONFIG_IS_ENABLED(DM_SERIAL) || defined(CONFIG_NS16550_DYNAMIC) > || \ > + defined(CONFIG_DEBUG_UART) > #define UART_REG(x) unsigned char x > #else > #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || > (CONFIG_SYS_NS16550_REG_SIZE == 0) Yes, I just cc'd you on it (crossed messages I do believe) with a comment. -- Tom [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL 2023-01-17 22:23 ` Tom Rini @ 2023-01-17 23:35 ` Andre Przywara 0 siblings, 0 replies; 12+ messages in thread From: Andre Przywara @ 2023-01-17 23:35 UTC (permalink / raw) To: Tom Rini; +Cc: Simon Glass, u-boot, linux-sunxi, Samuel Holland On Tue, 17 Jan 2023 17:23:57 -0500 Tom Rini <trini@konsulko.com> wrote: > On Tue, Jan 17, 2023 at 10:15:46PM +0000, Andre Przywara wrote: > > On Tue, 17 Jan 2023 11:14:35 -0500 > > Tom Rini <trini@konsulko.com> wrote: > > > > > On Tue, Jan 17, 2023 at 04:09:14PM +0000, Andre Przywara wrote: > > > > On Tue, 17 Jan 2023 08:15:18 -0500 > > > > Tom Rini <trini@konsulko.com> wrote: > > > > > > > > Hi Tom, > > > > > > > > > On Tue, Jan 17, 2023 at 12:09:38PM +0000, Andre Przywara wrote: > > > > > > > > > > > Commit 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > > > > > moved some NS16550 configuration variables into Kconfig. > > > > > > Among those there is CONFIG_SYS_NS16550_REG_SIZE, which used to be only > > > > > > defined for SPL build runs, but now is always set (thanks for Kconfig). > > > > > > However this breaks the gating logic in ns16550.h, where we *override* > > > > > > this variable for DM build, as we learn this setting from the DT instead. > > > > > > > > > > > > As a consequence, we did the register shift twice: once when building > > > > > > the register struct (as required for non-DM SPL builds), but then also > > > > > > again in the driver after we parsed the reg-shift DT property. > > > > > > > > > > > > Change the logic to match what the comment says: only observe > > > > > > CONFIG_SYS_NS16550_REG_SIZE when not using DM, and ignore it otherwise. > > > > > > > > > > > > This fixes U-Boot proper for all sunxi boards, since they are relying > > > > > > on this driver being build non-DM for the SPL, but DM for U-Boot proper. > > > > > > > > > > > > Fixes: 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig") > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > > > --- > > > > > > Hi, > > > > > > > > > > > > this is admittedly a quick fix, to get sunxi booting again. This whole > > > > > > code around the register access looks somewhat bonkers, to be honest, > > > > > > but at the moment I don't have time to rework this. > > > > > > Another possible "quicker fix" would be use a separate variable for the > > > > > > register shift, so that we don't have to redefine a Kconfig variable. > > > > > > Another idea would be to get rid of the struct for the registers > > > > > > altogether, to remove the hacks about when to shift. > > > > > > > > > > > > Cheers, > > > > > > Andre > > > > > > > > > > > > include/ns16550.h | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/include/ns16550.h b/include/ns16550.h > > > > > > index 243226fc3d9..e9e2aeedd16 100644 > > > > > > --- a/include/ns16550.h > > > > > > +++ b/include/ns16550.h > > > > > > @@ -26,11 +26,12 @@ > > > > > > > > > > > > #include <linux/types.h> > > > > > > > > > > > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE) > > > > > > +#if CONFIG_IS_ENABLED(DM_SERIAL) > > > > > > /* > > > > > > * For driver model we always use one byte per register, and sort out the > > > > > > * differences in the driver > > > > > > */ > > > > > > +#undef CONFIG_SYS_NS16550_REG_SIZE > > > > > > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > > > > > > #endif > > > > > > > > > > Does: > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230110161946.3816866-5-trini@konsulko.com/ > > > > > work for you as well? > > > > > > > > Unfortunately not. This changed condition there does not trigger for > > > > sunxi. If I hack it that it applies, but only to the proper build, it > > > > works, though. But I don't feel like adding to this #if even more ;-) > > > > > > I've got a version that does work, now, on my pine64 plus, and I've > > > asked Quentin to test his Rockchip platform that was also broken, before > > > I post a new version there. > > > > Is it something like this: > > > > diff --git a/include/ns16550.h b/include/ns16550.h > > index 243226fc3d9..74e619e0614 100644 > > --- a/include/ns16550.h > > +++ b/include/ns16550.h > > @@ -26,15 +26,12 @@ > > > > #include <linux/types.h> > > > > -#if CONFIG_IS_ENABLED(DM_SERIAL) && > > !defined(CONFIG_SYS_NS16550_REG_SIZE) /* > > * For driver model we always use one byte per register, and sort out > > the > > * differences in the driver > > */ > > -#define CONFIG_SYS_NS16550_REG_SIZE (-1) > > -#endif > > - > > -#if defined(CONFIG_NS16550_DYNAMIC) || defined(CONFIG_DEBUG_UART) > > +#if CONFIG_IS_ENABLED(DM_SERIAL) || defined(CONFIG_NS16550_DYNAMIC) > > || \ > > + defined(CONFIG_DEBUG_UART) > > #define UART_REG(x) unsigned char x > > #else > > #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || > > (CONFIG_SYS_NS16550_REG_SIZE == 0) > > Yes, I just cc'd you on it (crossed messages I do believe) with a > comment. Yeah, that's the five minutes I wrote and tested this ;-) Looks good, replied there. Thanks! Andre > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-01-18 13:08 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-17 12:09 [PATCH] ns16550: Fix DM serial operation with non-DM SPL Andre Przywara 2023-01-17 13:12 ` Sergei Antonov 2023-01-17 16:13 ` Andre Przywara 2023-01-18 13:08 ` Sergei Antonov 2023-01-17 16:16 ` Tom Rini 2023-01-17 13:15 ` Tom Rini 2023-01-17 15:55 ` Tom Rini 2023-01-17 16:09 ` Andre Przywara 2023-01-17 16:14 ` Tom Rini 2023-01-17 22:15 ` Andre Przywara 2023-01-17 22:23 ` Tom Rini 2023-01-17 23:35 ` Andre Przywara
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.