diff mbox series

[RFC,3/3] serial: mxc: restore booting for imx8mn_bsh_smm_s2

Message ID 20250515131246.784206-4-dario.binacchi@amarulasolutions.com
State New
Headers
Series Restore imx8mn_bsh_smm_s2 properly booting |

Commit Message

Dario Binacchi May 15, 2025, 1:12 p.m. UTC 
The commit dda454e933c6 ("serial: mxc: Support bulk enabling clocks")
breaks the booting of the BSH SMM S2 board.
Restore proper booting of the board even in case of failure of either
clk_get_bulk() or clk_enable_bulk().

Fixes: dda454e933c6 ("serial: mxc: Support bulk enabling clocks")
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

 drivers/serial/serial_mxc.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

Fabio Estevam May 15, 2025, 7:45 p.m. UTC | #1 
On Thu, May 15, 2025 at 10:12 AM Dario Binacchi
<dario.binacchi@amarulasolutions.com> wrote:
>
> The commit dda454e933c6 ("serial: mxc: Support bulk enabling clocks")
> breaks the booting of the BSH SMM S2 board.
> Restore proper booting of the board even in case of failure of either
> clk_get_bulk() or clk_enable_bulk().

We need a better understanding of why the boot fails.

Does the failure come from clk_get_bulk() or clk_enable_bulk()?

Please investigate.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Michael Trimarchi May 15, 2025, 8:12 p.m. UTC | #2 
Hi Fabio

On Thu, May 15, 2025 at 9:45 PM Fabio Estevam <festevam@gmail.com> wrote:

> On Thu, May 15, 2025 at 10:12 AM Dario Binacchi
> <dario.binacchi@amarulasolutions.com> wrote:
> >
> > The commit dda454e933c6 ("serial: mxc: Support bulk enabling clocks")
> > breaks the booting of the BSH SMM S2 board.
> > Restore proper booting of the board even in case of failure of either
> > clk_get_bulk() or clk_enable_bulk().
>
> We need a better understanding of why the boot fails.
>
>
The patch keeps the same functionality for the working board and fixes the
no booting one. Agree
that we need to understand but even we need working boards


> Does the failure come from clk_get_bulk() or clk_enable_bulk()?
>

The serial is not up at that time so we need to buffer the error and print
later. Right now
we are working on other fixes

Michael


>
> Please investigate.
>
Fabio Estevam May 17, 2025, 7:56 p.m. UTC | #3 
Hi Michael,

On Thu, May 15, 2025 at 5:12 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:

> The serial is not up at that time so we need to buffer the error and print later. Right now
> we are working on other fixes

Please share more details about the boot problem on the imx8mn_bsh_smm_s2 board.

Does a hang occur at SPL? Do you get anything in the console?

The mx8mn_bsh_smm_s2 board uses UART4 as the console.

The other i.MX8MN boards where we tested this patch used UART2.

UART4 does not have U-Boot/Linux permission by default.

TF-A enables the other UART port permission access at:
https://github.com/ARM-software/arm-trusted-firmware/blob/v2.12/plat/imx/imx8m/imx8mn/imx8mn_bl31_setup.c#L124-L143

Is the UART clock access falling in your case?

Is clk_enable_bulk() the one that fails?

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Michael Trimarchi May 17, 2025, 8:14 p.m. UTC | #4 
Hi Fabio

On Sat, May 17, 2025 at 9:56 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Michael,
>
> On Thu, May 15, 2025 at 5:12 PM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
>
> > The serial is not up at that time so we need to buffer the error and print later. Right now
> > we are working on other fixes
>
> Please share more details about the boot problem on the imx8mn_bsh_smm_s2 board.
>
> Does a hang occur at SPL? Do you get anything in the console?
>
> The mx8mn_bsh_smm_s2 board uses UART4 as the console.
>
> The other i.MX8MN boards where we tested this patch used UART2.
>

Can you please point me to an example of a tested board?

> UART4 does not have U-Boot/Linux permission by default.
>

TF-A is not in the game, the board is working fine.

> TF-A enables the other UART port permission access at:
> https://github.com/ARM-software/arm-trusted-firmware/blob/v2.12/plat/imx/imx8m/imx8mn/imx8mn_bl31_setup.c#L124-L143

I think we have upstream the fix there. Anyway...

>
> Is the UART clock access falling in your case?
>

If the uart is failing you don't have the console

> Is clk_enable_bulk() the one that fails?

I think that something connected to -u-boot.dtsi if you have tested
with some imx8mn

Michael
Fabio Estevam May 17, 2025, 8:27 p.m. UTC | #5 
On Sat, May 17, 2025 at 5:15 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:

> Can you please point me to an example of a tested board?

I have just tested the top-of-tree U-Boot on an imx8mn evk board:

U-Boot SPL 2025.07-rc2-00018-g126a88d49bca (May 17 2025 - 17:21:14 -0300)
WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
SEC0:  RNG instantiated
Normal Boot
Trying to boot from BOOTROM
Boot Stage: Primary boot
image offset 0x8000, pagesize 0x200, ivt offset 0x0
NOTICE:  Do not release JR0 to NS as it can be used by HAB
NOTICE:  BL31: v2.12.0(release):v2.12.0
NOTICE:  BL31: Built : 17:16:49, May 17 2025


U-Boot 2025.07-rc2-00018-g126a88d49bca (May 17 2025 - 17:21:14 -0300)

CPU:   NXP i.MX8MNano Quad Rev1.0 A53 at 1200 MHz
CPU:   Consumer temperature grade (0C to 95C) at 32C
Model: NXP i.MX8MNano DDR4 EVK board
DRAM:  2 GiB
Core:  180 devices, 23 uclasses, devicetree: separate
WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
MMC:   FSL_SDHC: 1, FSL_SDHC: 2
Loading Environment from MMC... Reading from MMC(1)... OK
In:    serial@30890000
Out:   serial@30890000
Err:   serial@30890000
SEC0:  RNG instantiated
Net:   eth0: ethernet@30be0000
Hit any key to stop autoboot:  0
u-boot=>

Make sure you are using the most recent U-Boot. We had some imx8m
clock drive hickups after 2025.04.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Michael Trimarchi May 17, 2025, 8:44 p.m. UTC | #6 
Hi

On Sat, May 17, 2025 at 10:28 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Sat, May 17, 2025 at 5:15 PM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
>
> > Can you please point me to an example of a tested board?
>
> I have just tested the top-of-tree U-Boot on an imx8mn evk board:
>
> U-Boot SPL 2025.07-rc2-00018-g126a88d49bca (May 17 2025 - 17:21:14 -0300)
> WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
> SEC0:  RNG instantiated
> Normal Boot
> Trying to boot from BOOTROM
> Boot Stage: Primary boot
> image offset 0x8000, pagesize 0x200, ivt offset 0x0
> NOTICE:  Do not release JR0 to NS as it can be used by HAB
> NOTICE:  BL31: v2.12.0(release):v2.12.0
> NOTICE:  BL31: Built : 17:16:49, May 17 2025
>

I was on master except this commit

commit 128d997a8772cc174f38d529d8b25f90b3aa8ad8
Author: Jonas Karlman <jonas@kwiboo.se>
Date:   Sat May 10 15:32:01 2025 +0000

    clk: Fix clk_set_parent() regression

    The commit ac30d90f3367 ("clk: Ensure the parent clocks are enabled
    while reparenting") add a call to clk_enable() for the parent clock.

    For clock drivers that do not implement the enable() ops, like most
    Rockchip clock drivers, this now cause the set_parent() ops to never
    be called when CLK_CCF=n (default for Rockchip).


I can not try today but if you can, is this the commit that could break my boot?

Michael
>
> U-Boot 2025.07-rc2-00018-g126a88d49bca (May 17 2025 - 17:21:14 -0300)
>
> CPU:   NXP i.MX8MNano Quad Rev1.0 A53 at 1200 MHz
> CPU:   Consumer temperature grade (0C to 95C) at 32C
> Model: NXP i.MX8MNano DDR4 EVK board
> DRAM:  2 GiB
> Core:  180 devices, 23 uclasses, devicetree: separate
> WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
> MMC:   FSL_SDHC: 1, FSL_SDHC: 2
> Loading Environment from MMC... Reading from MMC(1)... OK
> In:    serial@30890000
> Out:   serial@30890000
> Err:   serial@30890000
> SEC0:  RNG instantiated
> Net:   eth0: ethernet@30be0000
> Hit any key to stop autoboot:  0
> u-boot=>
>
> Make sure you are using the most recent U-Boot. We had some imx8m
> clock drive hickups after 2025.04.
Michael Trimarchi May 17, 2025, 8:46 p.m. UTC | #7 
Hi Fabio

On Sat, May 17, 2025 at 10:44 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi
>
> On Sat, May 17, 2025 at 10:28 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > On Sat, May 17, 2025 at 5:15 PM Michael Nazzareno Trimarchi
> > <michael@amarulasolutions.com> wrote:
> >
> > > Can you please point me to an example of a tested board?
> >
> > I have just tested the top-of-tree U-Boot on an imx8mn evk board:
> >
> > U-Boot SPL 2025.07-rc2-00018-g126a88d49bca (May 17 2025 - 17:21:14 -0300)
> > WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
> > SEC0:  RNG instantiated
> > Normal Boot
> > Trying to boot from BOOTROM
> > Boot Stage: Primary boot
> > image offset 0x8000, pagesize 0x200, ivt offset 0x0
> > NOTICE:  Do not release JR0 to NS as it can be used by HAB
> > NOTICE:  BL31: v2.12.0(release):v2.12.0
> > NOTICE:  BL31: Built : 17:16:49, May 17 2025
> >
>
> I was on master except this commit
>
> commit 128d997a8772cc174f38d529d8b25f90b3aa8ad8
> Author: Jonas Karlman <jonas@kwiboo.se>
> Date:   Sat May 10 15:32:01 2025 +0000
>
>     clk: Fix clk_set_parent() regression
>
>     The commit ac30d90f3367 ("clk: Ensure the parent clocks are enabled
>     while reparenting") add a call to clk_enable() for the parent clock.
>
>     For clock drivers that do not implement the enable() ops, like most
>     Rockchip clock drivers, this now cause the set_parent() ops to never
>     be called when CLK_CCF=n (default for Rockchip).
>
>

BTW is really needed now?

git grep init_uart_clk board/freescale/imx8mn_evk/
board/freescale/imx8mn_evk/spl.c:       init_uart_clk(1);


Michael

> I can not try today but if you can, is this the commit that could break my boot?
>
> Michael
> >
> > U-Boot 2025.07-rc2-00018-g126a88d49bca (May 17 2025 - 17:21:14 -0300)
> >
> > CPU:   NXP i.MX8MNano Quad Rev1.0 A53 at 1200 MHz
> > CPU:   Consumer temperature grade (0C to 95C) at 32C
> > Model: NXP i.MX8MNano DDR4 EVK board
> > DRAM:  2 GiB
> > Core:  180 devices, 23 uclasses, devicetree: separate
> > WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
> > MMC:   FSL_SDHC: 1, FSL_SDHC: 2
> > Loading Environment from MMC... Reading from MMC(1)... OK
> > In:    serial@30890000
> > Out:   serial@30890000
> > Err:   serial@30890000
> > SEC0:  RNG instantiated
> > Net:   eth0: ethernet@30be0000
> > Hit any key to stop autoboot:  0
> > u-boot=>
> >
> > Make sure you are using the most recent U-Boot. We had some imx8m
> > clock drive hickups after 2025.04.
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com
Fabio Estevam May 17, 2025, 8:50 p.m. UTC | #8 
On Sat, May 17, 2025 at 5:46 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:

> > I was on master except this commit
> >
> > commit 128d997a8772cc174f38d529d8b25f90b3aa8ad8
> > Author: Jonas Karlman <jonas@kwiboo.se>
> > Date:   Sat May 10 15:32:01 2025 +0000
> >
> >     clk: Fix clk_set_parent() regression
> >
> >     The commit ac30d90f3367 ("clk: Ensure the parent clocks are enabled
> >     while reparenting") add a call to clk_enable() for the parent clock.
> >
> >     For clock drivers that do not implement the enable() ops, like most
> >     Rockchip clock drivers, this now cause the set_parent() ops to never
> >     be called when CLK_CCF=n (default for Rockchip).

CLK_CCF=y in your case, so this does not help for you.
>
> BTW is really needed now?
>
> git grep init_uart_clk board/freescale/imx8mn_evk/
> board/freescale/imx8mn_evk/spl.c:       init_uart_clk(1);

This can be removed. I tested without this line, and it boots fine.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Michael Trimarchi May 17, 2025, 8:52 p.m. UTC | #9 
Hi

On Sat, May 17, 2025 at 10:50 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Sat, May 17, 2025 at 5:46 PM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
>
> > > I was on master except this commit
> > >
> > > commit 128d997a8772cc174f38d529d8b25f90b3aa8ad8
> > > Author: Jonas Karlman <jonas@kwiboo.se>
> > > Date:   Sat May 10 15:32:01 2025 +0000
> > >
> > >     clk: Fix clk_set_parent() regression
> > >
> > >     The commit ac30d90f3367 ("clk: Ensure the parent clocks are enabled
> > >     while reparenting") add a call to clk_enable() for the parent clock.
> > >
> > >     For clock drivers that do not implement the enable() ops, like most
> > >     Rockchip clock drivers, this now cause the set_parent() ops to never
> > >     be called when CLK_CCF=n (default for Rockchip).
>
> CLK_CCF=y in your case, so this does not help for you.
> >
> > BTW is really needed now?
> >
> > git grep init_uart_clk board/freescale/imx8mn_evk/
> > board/freescale/imx8mn_evk/spl.c:       init_uart_clk(1);
>
> This can be removed. I tested without this line, and it boots fine.

Are you sending a patch? or I will send it

Michael
Fabio Estevam May 17, 2025, 8:57 p.m. UTC | #10 
On Sat, May 17, 2025 at 5:52 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:

> Are you sending a patch? or I will send it

Feel free to send it when you have a chance. Thanks!

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
diff mbox series

Patch

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 28f4435d01dd..b09a5fe0f7a4 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -310,22 +310,31 @@  int mxc_serial_setbrg(struct udevice *dev, int baudrate)
 	return 0;
 }
 
-static int mxc_serial_probe(struct udevice *dev)
+#if CONFIG_IS_ENABLED(CLK_CCF)
+static void mxc_serial_enable_clk_bulk(struct udevice *dev)
 {
 	struct mxc_serial_plat *plat = dev_get_plat(dev);
-#if CONFIG_IS_ENABLED(CLK_CCF)
 	int ret;
 
 	ret = clk_get_bulk(dev, &plat->clks);
 	if (ret)
-		return ret;
+		return;
 
-	ret = clk_enable_bulk(&plat->clks);
-	if (ret)
-		return ret;
+	clk_enable_bulk(&plat->clks);
+}
+#else
+static mxc_serial_enable_clk_bulk(struct udevice *dev)
+{
+}
 #endif
-	_mxc_serial_init(plat->reg, plat->use_dte);
 
+static int mxc_serial_probe(struct udevice *dev)
+{
+	struct mxc_serial_plat *plat = dev_get_plat(dev);
+
+	mxc_serial_enable_clk_bulk(dev);
+
+	_mxc_serial_init(plat->reg, plat->use_dte);
 	return 0;
 }