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

[Felix Brack]

Hello Tom,

On 14.02.22 23:45, Tom Rini wrote:
> On Mon, Feb 14, 2022 at 05:56:52PM +0100, Felix Brack 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
>> patch provides a generic location to add the required code. This code
>> has to be SoC and not board specific.
>> For the am33xx SoCs the fix consist of configuring early clocks.
>>
>> Signed-off-by: Felix Brack <fb at ltec.ch>
>> ---
>>
>>  arch/arm/mach-omap2/am33xx/board.c |  7 +++++++
>>  drivers/serial/Kconfig             | 13 +++++++++++++
>>  include/debug_uart.h               | 15 +++++++++++++++
>>  3 files changed, 35 insertions(+)
>>
>> 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.
>> +
>>  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.
>> + *
>>   * 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 \
> 
> I'd be inclined to just re-use board_debug_uart_init on am33xx like
> other "board" function we have in the SoC level board.c file.  That's
> likely what other platforms are doing as well where this is more SoC
> than board dependent?  Thanks for digging in here BTW, I think I never
> saw this myself when I needed the UART because I have mine set for UART
> boot first (and then it does SD boot).
> 
On the PDU001 board the board specific board_debug_uart_init() is (for
now) responsible for the pin configuration of UART3 (the debug UART).
>From a discussion with Simon I concluded that other boards based on the
AM33XX SoCs are also affected from the problem introduced with commit
0dba45864b2a.
The function board_debug_uart_init() can not be moved to the SoC level
since some boards (like the PDU001) require it. Hence the idea of a SoC
specific soc_debug_uart_init() function.
However I now realize that such a "one fits it all" solution is probably
not the right way to fix things. It might be better to fix this on board
level especially since there are some boards that are not affected at all.
Hence I think it is best to drop my patch. I will send a new patch
fixing the problem for the PDU001 board specifically.
-- 
Regards, Felix