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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5399 bytes --]

On Tue, Feb 15, 2022 at 11:53:41AM +0100, Felix Brack wrote:
> 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@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.

Ah, OK, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]