Re: [PATCHv2 05/19] dm: ns16550: Restore how we define UART_REG

[Andre Przywara <andre.przywara@arm.com>]

On Tue, 17 Jan 2023 17:10:50 -0500
Tom Rini <trini@konsulko.com> wrote:

> Prior to commit 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to
> Kconfig") we had defined CONFIG_SYS_NS16550_REG_SIZE to -1 with
> DM_SERIAL such that we would then have a size 0 character array. This
> resulted in functionally no padding. The confusion on my part came from
> dealing with the constraints around platforms that do not use DM_SERIAL
> in SPL/TPL. After Andre Przywara reported that sunxi was broken, I've
> re-read the code and comments again and thought on this harder. What we
> want I believe is what this patch does now.
> 
> If DM_SERIAL is defined for this stage, regardless of
> CONFIG_SYS_NS16550_REG_SIZE then we will dynamically handle reg shifts
> and 'struct ns16550' needs no padding (which is functionally what
> unsigned char foo[0] provides). This is the same case as NS16550_DYNAMIC
> and DEBUG_UART. Expand the existing comment here slightly.
> 
> Otherwise, we will have CONFIG_SYS_NS16550_REG_SIZE set to a non-zero
> value, and handle padding within the struct.
> 
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Sergei Antonov <saproj@gmail.com>
> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Fixes: 9591b63531fa ("Convert CONFIG_SYS_NS16550_MEM32 et al to Kconfig")
> Signed-off-by: Tom Rini <trini@konsulko.com>

Thanks, that indeed fixes the issue for me, booted on LicheePi Nano and
Pine64-LTS:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Tested-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  include/ns16550.h | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/include/ns16550.h b/include/ns16550.h
> index 243226fc3d94..e7e68663d030 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -26,15 +26,13 @@
>  
>  #include <linux/types.h>
>  
> -#if CONFIG_IS_ENABLED(DM_SERIAL) && !defined(CONFIG_SYS_NS16550_REG_SIZE)
> +#if CONFIG_IS_ENABLED(DM_SERIAL) ||  defined(CONFIG_NS16550_DYNAMIC) || \
> +	defined(CONFIG_DEBUG_UART)
>  /*
>   * For driver model we always use one byte per register, and sort out the
> - * differences in the driver
> + * differences in the driver. In the case of CONFIG_NS16550_DYNAMIC we do
> + * similar, and CONFIG_DEBUG_UART is responsible for shifts in its own manner.
>   */
> -#define CONFIG_SYS_NS16550_REG_SIZE (-1)
> -#endif
> -
> -#if 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)