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

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

[Felix Brack]

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 \
-- 
2.25.1

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

[Tom Rini]

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

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

-- 
Tom

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

Re: [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@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

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

[Tom Rini]

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

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

[Simon Glass]

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
>

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

[Felix Brack]

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

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

[Simon Glass]

Hi Felix,

On Thu, 24 Feb 2022 at 03:01, Felix Brack <fb@ltec.ch> wrote:
>
> 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.

OK, I suppose it can wait until another board comes along that needs it.

Regards,
Simon