[PATCH v2 1/2] board: atmel: sama5d2_wlsom1_ek: Fix spurious serial output

[PATCH v2 1/2] board: atmel: sama5d2_wlsom1_ek: Fix spurious serial output

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

Currently, there are always some spurious characters showing up
right before SPL banner is shown:

RomBOOT
���=������
U-Boot SPL 2025.04-rc5-00023-g9ed4e2c45f25 (Apr 03 2025 - 23:23:17 -0300)
Trying to boot from MMC1

<debug_uart>
...

The reason for the spurious characters is that the UART0 clock is
being enabled too early, prior to adjusting its correct frequency.

Fix the problem by removing the early UART0 clock enablement in C code.

SPL DM code will adjust the correct UART0 clock frequency and then enable
it at the appropriate time.

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- Make the minimal change to fix the spurious characters. Improve
the explanation in the commit log.

 board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c b/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
index 04de1257eca0..ff5e51ee3926 100644
--- a/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
+++ b/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
@@ -45,8 +45,6 @@ static void board_uart0_hw_init(void)
 {
 	atmel_pio4_set_c_periph(AT91_PIO_PORTB, 26, ATMEL_PIO_PUEN_MASK);	/* URXD0 */
 	atmel_pio4_set_c_periph(AT91_PIO_PORTB, 27, 0);				/* UTXD0 */
-
-	at91_periph_clk_enable(ATMEL_ID_UART0);
 }
 
 void board_debug_uart_init(void)
-- 
2.34.1

[PATCH v2 2/2] board: atmel: sama5d2_wlsom1_ek: Remove UART pinctrl initialization

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

The UART0 pinctrl initialization is already done by DM via
CONFIG_PINCTRL_AT91PIO4=y.

Remove the unnecessary UART0 pinctrl initialization done in board code.

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- Newly introduced.

 board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c b/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
index ff5e51ee3926..a0ecc4f42a90 100644
--- a/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
+++ b/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
@@ -41,15 +41,8 @@ int board_late_init(void)
 #endif
 
 #ifdef CONFIG_DEBUG_UART_BOARD_INIT
-static void board_uart0_hw_init(void)
-{
-	atmel_pio4_set_c_periph(AT91_PIO_PORTB, 26, ATMEL_PIO_PUEN_MASK);	/* URXD0 */
-	atmel_pio4_set_c_periph(AT91_PIO_PORTB, 27, 0);				/* UTXD0 */
-}
-
 void board_debug_uart_init(void)
 {
-	board_uart0_hw_init();
 }
 #endif
 
-- 
2.34.1

Re: [PATCH v2 1/2] board: atmel: sama5d2_wlsom1_ek: Fix spurious serial output

[Eugen Hristev]

On 4/4/25 14:39, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
> 
> Currently, there are always some spurious characters showing up
> right before SPL banner is shown:
> 
> RomBOOT
> ���=������
> U-Boot SPL 2025.04-rc5-00023-g9ed4e2c45f25 (Apr 03 2025 - 23:23:17 -0300)
> Trying to boot from MMC1
> 
> <debug_uart>
> ...
> 
> The reason for the spurious characters is that the UART0 clock is
> being enabled too early, prior to adjusting its correct frequency.
> 
> Fix the problem by removing the early UART0 clock enablement in C code.

I am not convinced.
The purpose of this early UART would be to get output *before* the DM
code would enable clocks, pins, etc, for exactly that purpose, early
debug output.
So to remove all this and make the initialization rely on the SPL DM
code would defeat the purpose.

> 
> SPL DM code will adjust the correct UART0 clock frequency and then enable
> it at the appropriate time.
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - Make the minimal change to fix the spurious characters. Improve
> the explanation in the commit log.
> 
>  board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c b/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
> index 04de1257eca0..ff5e51ee3926 100644
> --- a/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
> +++ b/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
> @@ -45,8 +45,6 @@ static void board_uart0_hw_init(void)
>  {
>  	atmel_pio4_set_c_periph(AT91_PIO_PORTB, 26, ATMEL_PIO_PUEN_MASK);	/* URXD0 */
>  	atmel_pio4_set_c_periph(AT91_PIO_PORTB, 27, 0);				/* UTXD0 */
> -
> -	at91_periph_clk_enable(ATMEL_ID_UART0);
>  }
>  
>  void board_debug_uart_init(void)

Re: [PATCH v2 1/2] board: atmel: sama5d2_wlsom1_ek: Fix spurious serial output

[Fabio Estevam]

On Mon, Apr 7, 2025 at 3:39 AM Eugen Hristev <eugen.hristev@linaro.org> wrote:

> I am not convinced.
> The purpose of this early UART would be to get output *before* the DM
> code would enable clocks, pins, etc, for exactly that purpose, early
> debug output.
> So to remove all this and make the initialization rely on the SPL DM
> code would defeat the purpose.

Ok, but if we have CONFIG_DEBUG_UART enabled, shouldn't we then
disable CONFIG_SPL_SERIAL?

The change below fixes the serial garbage shown in SPL:

--- a/configs/sama5d27_wlsom1_ek_mmc_defconfig
+++ b/configs/sama5d27_wlsom1_ek_mmc_defconfig
@@ -16,7 +16,6 @@ CONFIG_OF_LIBFDT_OVERLAY=y
 CONFIG_DM_RESET=y
 CONFIG_SYS_MONITOR_LEN=524288
 CONFIG_SPL_MMC=y
-CONFIG_SPL_SERIAL=y
 CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL_STACK=0x218000
 CONFIG_SPL_TEXT_BASE=0x200000

I suspect CONFIG_DEBUG_UART and CONFIG_SPL_SERIAL conflict with each other.

Is the change above acceptable? If not, what would you suggest?

Re: [PATCH v2 1/2] board: atmel: sama5d2_wlsom1_ek: Fix spurious serial output

[Eugen Hristev]

On 4/8/25 02:52, Fabio Estevam wrote:
> On Mon, Apr 7, 2025 at 3:39 AM Eugen Hristev <eugen.hristev@linaro.org> wrote:
> 
>> I am not convinced.
>> The purpose of this early UART would be to get output *before* the DM
>> code would enable clocks, pins, etc, for exactly that purpose, early
>> debug output.
>> So to remove all this and make the initialization rely on the SPL DM
>> code would defeat the purpose.
> 
> Ok, but if we have CONFIG_DEBUG_UART enabled, shouldn't we then
> disable CONFIG_SPL_SERIAL?
> 
> The change below fixes the serial garbage shown in SPL:
> 
> --- a/configs/sama5d27_wlsom1_ek_mmc_defconfig
> +++ b/configs/sama5d27_wlsom1_ek_mmc_defconfig
> @@ -16,7 +16,6 @@ CONFIG_OF_LIBFDT_OVERLAY=y
>  CONFIG_DM_RESET=y
>  CONFIG_SYS_MONITOR_LEN=524288
>  CONFIG_SPL_MMC=y
> -CONFIG_SPL_SERIAL=y
>  CONFIG_SPL_DRIVERS_MISC=y
>  CONFIG_SPL_STACK=0x218000
>  CONFIG_SPL_TEXT_BASE=0x200000
> 
> I suspect CONFIG_DEBUG_UART and CONFIG_SPL_SERIAL conflict with each other.

If they conflict, it means one of them is wrongly configured, since it's
the same hardware serial used for both.
So my feeling is that there is a wrong clock config in one of the
serials that causes the bad characters. Wrong clock config because it
appears to have a wrong computed divider to get to the desired baud.
So , disabling one of them is not really fixing the problem.

> 
> Is the change above acceptable? If not, what would you suggest?