Re: [PATCH] arm: am33xx: Fix early debug UART initialization

[Felix Brack <fb@ltec.ch>]

Hello Simon,

On 23.02.22 23:58, Simon Glass wrote:
> Hi Felix,
> 
> On Mon, 14 Feb 2022 at 09:57, Felix Brack <fb@ltec.ch> wrote:
>>
>> The changes from commit 0dba45864b2a ("arm: Init the debug UART")
>> prevent the early debug UART from being initialized correctly.
>> By adding a new SoC specific early UART initialization function this
> 
> SoC-specific
> 
> You need the hyphen for this to make sense, since you are creating an adjective.
> 
>> patch provides a generic location to add the required code. This code
>> has to be SoC and not board specific.
> 
> board-specific
> 
>> For the am33xx SoCs the fix consist of configuring early clocks.
>>
>> Signed-off-by: Felix Brack <fb@ltec.ch>
>> ---
>>
>>  arch/arm/mach-omap2/am33xx/board.c |  7 +++++++
>>  drivers/serial/Kconfig             | 13 +++++++++++++
>>  include/debug_uart.h               | 15 +++++++++++++++
>>  3 files changed, 35 insertions(+)
> 
> More nits below.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
>>
>> diff --git a/arch/arm/mach-omap2/am33xx/board.c b/arch/arm/mach-omap2/am33xx/board.c
>> index c44667668e..a7f0445b93 100644
>> --- a/arch/arm/mach-omap2/am33xx/board.c
>> +++ b/arch/arm/mach-omap2/am33xx/board.c
>> @@ -604,3 +604,10 @@ int arch_cpu_init_dm(void)
>>  #endif
>>         return 0;
>>  }
>> +
>> +#if IS_ENABLED(CONFIG_DEBUG_UART_SOC_INIT)
>> +void soc_debug_uart_init(void)
>> +{
>> +       setup_early_clocks();
>> +}
>> +#endif
>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>> index 345d1881f5..3da4064d35 100644
>> --- a/drivers/serial/Kconfig
>> +++ b/drivers/serial/Kconfig
>> @@ -490,6 +490,19 @@ config DEBUG_UART_SHIFT
>>           value. Use this value to specify the shift to use, where 0=byte
>>           registers, 2=32-bit word registers, etc.
>>
>> +config DEBUG_UART_SOC_INIT
>> +       bool "Enable SoC-specific debug UART init"
>> +       depends on DEBUG_UART
>> +       help
>> +         Boards using the same SoC might require some common settings before
>> +         the debug UART can be used. The board specific board_debug_uart_init()
>> +         is not the right place for such common settings as they would apply
>> +         to a specific board only instead of all boards using the same SoC.
>> +         When this option is enabled, the SoC specific function
>> +         soc_debug_uart_init() will be called when debug_uart_init() is called.
>> +         You can put any code here that is needed to set up the UART ready for
>> +         use.
> 
> Please mention the order in which the board and SoC functions are called.
> 
>> +
>>  config DEBUG_UART_BOARD_INIT
>>         bool "Enable board-specific debug UART init"
>>         depends on DEBUG_UART
>> diff --git a/include/debug_uart.h b/include/debug_uart.h
>> index 714b369e6f..ebc84c44cd 100644
>> --- a/include/debug_uart.h
>> +++ b/include/debug_uart.h
>> @@ -42,6 +42,12 @@
>>   * - Immediately afterwards, add DEBUG_UART_FUNCS to define the rest of the
>>   *     functionality (printch(), etc.)
>>   *
>> + * If your SoC needs additional init for the UART to work, enable
>> + * CONFIG_DEBUG_UART_SOC_INIT and write a function called
>> + * soc_debug_uart_init() to perform that init. When debug_uart_init() is
>> + * called, the init will happen automatically. Board specific code does not
>> + * go here, see board_debug_uart_init() below.
> 
> Again please fix your specifics.
> 
> 
>> + *
>>   * If your board needs additional init for the UART to work, enable
>>   * CONFIG_DEBUG_UART_BOARD_INIT and write a function called
>>   * board_debug_uart_init() to perform that init. When debug_uart_init() is
>> @@ -61,6 +67,14 @@
>>   */
>>  void debug_uart_init(void);
>>
>> +#ifdef CONFIG_DEBUG_UART_SOC_INIT
>> +void soc_debug_uart_init(void);
>> +#else
>> +static inline void soc_debug_uart_init(void)
>> +{
>> +}
>> +#endif
>> +
>>  #ifdef CONFIG_DEBUG_UART_BOARD_INIT
>>  void board_debug_uart_init(void);
>>  #else
>> @@ -192,6 +206,7 @@ void printdec(unsigned int value);
>>  \
>>         void debug_uart_init(void) \
>>         { \
>> +               soc_debug_uart_init(); \
>>                 board_debug_uart_init(); \
>>                 _debug_uart_init(); \
>>                 _DEBUG_UART_ANNOUNCE \
>> --
>> 2.25.1
>>
Thanks for the review! However it looks like this patch will never get
applied. If I'm wrong please let me know and I'll be happy to send a
fixed version.
-- 
Regards, Felix