[PATCH] ns16550: Fix DM serial operation with non-DM SPL

[PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Andre Przywara]

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

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Sergei Antonov]

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.

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Tom Rini]

[-- 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 --]

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Tom Rini]

[-- 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 --]

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Andre Przywara]

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

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Andre Przywara]

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

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Tom Rini]

[-- 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 --]

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Tom Rini]

[-- 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 --]

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Andre Przywara]

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.
> 

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Tom Rini]

[-- 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 --]

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Andre Przywara]

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

> 

Re: [PATCH] ns16550: Fix DM serial operation with non-DM SPL

[Sergei Antonov]

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!