diff mbox series

[next,4/5] rockchip: rk3399: allow to handle sysreset-gpio in TPL

Message ID 20241105-rk3399-sysreset-gpio-tpl-v1-4-12caff07a4e4@cherry.de
State New
Headers show
Series rockchip: rk3399: allow to trigger sysreset in TPL | expand

Commit Message

Quentin Schulz Nov. 5, 2024, 3:36 p.m. UTC 
From: Quentin Schulz <quentin.schulz@cherry.de>

If TPL_GPIO and TPL_PINCTRL_ROCKCHIP are enabled and a sysreset-gpio is
provided in the TPL Device Tree, this will trigger a system reset
similar to what's currently been done in SPL whenever the RK3399 "warm"
boots.

There are three reasons for moving this earlier:
- faster boot time as we don't need to reach SPL to be able to reset the
  system on a condition we know is already met in TPL,
- have less code to be impacted by the issue this system reset works
  around (that is, "unclean" SoC registers after a reboot),
- less confusion around the reason for restarting. Indeed when done from
  SPL, the following log can be observed:

"""
U-Boot TPL 2025.01-rc1-00165-gd79216ca9878-dirty (Nov 05 2024 - 15:31:45)
Channel 0: DDR3, 666MHz
BW=32 Col=10 Bk=8 CS0 Row=16 CS=1 Die BW=16 Size=2048MB
Channel 1: DDR3, 666MHz
BW=32 Col=10 Bk=8 CS0 Row=16 CS=1 Die BW=16 Size=2048MB
256B stride
Trying to boot from BOOTROM
Returning to boot ROM...

U-Boot SPL 2025.01-rc1-00165-gd79216ca9878-dirty (Nov 05 2024 - 15:31:45 +0100)
Trying to boot from MMC2

U-Boot TPL 2025.01-rc1-00165-gd79216ca9878-dirty (Nov 05 2024 - 15:31:45)
"""

possibly hinting at an issue within the SPL when loading the fitImage
from MMC2 instead of the normal course of events (a system reset).

Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 arch/arm/mach-rockchip/rk3399/rk3399.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Paul Kocialkowski Nov. 5, 2024, 6:40 p.m. UTC | #1 
Hi,

Le Tue 05 Nov 24, 16:36, Quentin Schulz a écrit :
> From: Quentin Schulz <quentin.schulz@cherry.de>
> 
> If TPL_GPIO and TPL_PINCTRL_ROCKCHIP are enabled and a sysreset-gpio is
> provided in the TPL Device Tree, this will trigger a system reset
> similar to what's currently been done in SPL whenever the RK3399 "warm"
> boots.
> 
> There are three reasons for moving this earlier:
> - faster boot time as we don't need to reach SPL to be able to reset the
>   system on a condition we know is already met in TPL,
> - have less code to be impacted by the issue this system reset works
>   around (that is, "unclean" SoC registers after a reboot),
> - less confusion around the reason for restarting. Indeed when done from
>   SPL, the following log can be observed:
> 
> """
> U-Boot TPL 2025.01-rc1-00165-gd79216ca9878-dirty (Nov 05 2024 - 15:31:45)
> Channel 0: DDR3, 666MHz
> BW=32 Col=10 Bk=8 CS0 Row=16 CS=1 Die BW=16 Size=2048MB
> Channel 1: DDR3, 666MHz
> BW=32 Col=10 Bk=8 CS0 Row=16 CS=1 Die BW=16 Size=2048MB
> 256B stride
> Trying to boot from BOOTROM
> Returning to boot ROM...
> 
> U-Boot SPL 2025.01-rc1-00165-gd79216ca9878-dirty (Nov 05 2024 - 15:31:45 +0100)
> Trying to boot from MMC2
> 
> U-Boot TPL 2025.01-rc1-00165-gd79216ca9878-dirty (Nov 05 2024 - 15:31:45)
> """
> 
> possibly hinting at an issue within the SPL when loading the fitImage
> from MMC2 instead of the normal course of events (a system reset).
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>

Reviewed-by: Paul Kocialkowski <paulk@sys-base.io>

See a nit below.

Cheers,

Paul

> ---
>  arch/arm/mach-rockchip/rk3399/rk3399.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/rk3399/rk3399.c b/arch/arm/mach-rockchip/rk3399/rk3399.c
> index 7b6a822ed04b8151a5da147056dbf73ffdafd149..26fd04691699d81f5fecd282964c7b3aff999717 100644
> --- a/arch/arm/mach-rockchip/rk3399/rk3399.c
> +++ b/arch/arm/mach-rockchip/rk3399/rk3399.c
> @@ -169,7 +169,7 @@ void board_debug_uart_init(void)
>  }
>  #endif
>  
> -#if defined(CONFIG_XPL_BUILD) && !defined(CONFIG_TPL_BUILD)
> +#if defined(CONFIG_XPL_BUILD)
>  static void rk3399_force_power_on_reset(void)
>  {
>  	const struct rockchip_cru *cru = rockchip_get_cru();
> @@ -195,9 +195,9 @@ static void rk3399_force_power_on_reset(void)
>  	if (cru->glb_rst_st == 0)
>  		return;
>  
> -	if (!IS_ENABLED(CONFIG_SPL_GPIO)) {
> +	if (!CONFIG_IS_ENABLED(GPIO)) {
>  		debug("%s: trying to force a power-on reset but no GPIO "
> -		      "support in SPL!\n", __func__);
> +		      "support in " PHASE_NAME "!\n", __func__);
>  		return;
>  	}
>  
> @@ -218,6 +218,7 @@ static void rk3399_force_power_on_reset(void)
>  	dm_gpio_set_value(&sysreset_gpio, 1);
>  }
>  
> +# if !defined(CONFIG_TPL_BUILD)

Coding style in the file doesn't put a space after # (even when nested).

>  void __weak led_setup(void)
>  {
>  }
> @@ -228,4 +229,10 @@ void spl_board_init(void)
>  
>  	rk3399_force_power_on_reset();
>  }
> -#endif
> +# else  // !defined(CONFIG_TPL_BUILD)

Ditto and also doesn't have such comments.

> +void tpl_board_init(void)
> +{
> +	rk3399_force_power_on_reset();
> +}
> +# endif // !defined(CONFIG_TPL_BUILD)

Ditto.

> +#endif // defined(CONFIG_XPL_BUILD)
> 
> -- 
> 2.47.0
>
Paul Kocialkowski Nov. 5, 2024, 6:44 p.m. UTC | #2 
Hi again,

Le Tue 05 Nov 24, 16:36, Quentin Schulz a écrit :
> From: Quentin Schulz <quentin.schulz@cherry.de>
> 
> If TPL_GPIO and TPL_PINCTRL_ROCKCHIP are enabled and a sysreset-gpio is
> provided in the TPL Device Tree, this will trigger a system reset
> similar to what's currently been done in SPL whenever the RK3399 "warm"
> boots.
> 
> There are three reasons for moving this earlier:
> - faster boot time as we don't need to reach SPL to be able to reset the
>   system on a condition we know is already met in TPL,
> - have less code to be impacted by the issue this system reset works
>   around (that is, "unclean" SoC registers after a reboot),
> - less confusion around the reason for restarting. Indeed when done from
>   SPL, the following log can be observed:

Just one other thing here: you could as well remove the call in spl_board_init.
There's just one user of this currently (the Puma board) so I don't think we
need to keep backwards-compatibility in the SPL.

Cheers,

Paul

> """
> U-Boot TPL 2025.01-rc1-00165-gd79216ca9878-dirty (Nov 05 2024 - 15:31:45)
> Channel 0: DDR3, 666MHz
> BW=32 Col=10 Bk=8 CS0 Row=16 CS=1 Die BW=16 Size=2048MB
> Channel 1: DDR3, 666MHz
> BW=32 Col=10 Bk=8 CS0 Row=16 CS=1 Die BW=16 Size=2048MB
> 256B stride
> Trying to boot from BOOTROM
> Returning to boot ROM...
> 
> U-Boot SPL 2025.01-rc1-00165-gd79216ca9878-dirty (Nov 05 2024 - 15:31:45 +0100)
> Trying to boot from MMC2
> 
> U-Boot TPL 2025.01-rc1-00165-gd79216ca9878-dirty (Nov 05 2024 - 15:31:45)
> """
> 
> possibly hinting at an issue within the SPL when loading the fitImage
> from MMC2 instead of the normal course of events (a system reset).
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  arch/arm/mach-rockchip/rk3399/rk3399.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/rk3399/rk3399.c b/arch/arm/mach-rockchip/rk3399/rk3399.c
> index 7b6a822ed04b8151a5da147056dbf73ffdafd149..26fd04691699d81f5fecd282964c7b3aff999717 100644
> --- a/arch/arm/mach-rockchip/rk3399/rk3399.c
> +++ b/arch/arm/mach-rockchip/rk3399/rk3399.c
> @@ -169,7 +169,7 @@ void board_debug_uart_init(void)
>  }
>  #endif
>  
> -#if defined(CONFIG_XPL_BUILD) && !defined(CONFIG_TPL_BUILD)
> +#if defined(CONFIG_XPL_BUILD)
>  static void rk3399_force_power_on_reset(void)
>  {
>  	const struct rockchip_cru *cru = rockchip_get_cru();
> @@ -195,9 +195,9 @@ static void rk3399_force_power_on_reset(void)
>  	if (cru->glb_rst_st == 0)
>  		return;
>  
> -	if (!IS_ENABLED(CONFIG_SPL_GPIO)) {
> +	if (!CONFIG_IS_ENABLED(GPIO)) {
>  		debug("%s: trying to force a power-on reset but no GPIO "
> -		      "support in SPL!\n", __func__);
> +		      "support in " PHASE_NAME "!\n", __func__);
>  		return;
>  	}
>  
> @@ -218,6 +218,7 @@ static void rk3399_force_power_on_reset(void)
>  	dm_gpio_set_value(&sysreset_gpio, 1);
>  }
>  
> +# if !defined(CONFIG_TPL_BUILD)
>  void __weak led_setup(void)
>  {
>  }
> @@ -228,4 +229,10 @@ void spl_board_init(void)
>  
>  	rk3399_force_power_on_reset();
>  }
> -#endif
> +# else  // !defined(CONFIG_TPL_BUILD)
> +void tpl_board_init(void)
> +{
> +	rk3399_force_power_on_reset();
> +}
> +# endif // !defined(CONFIG_TPL_BUILD)
> +#endif // defined(CONFIG_XPL_BUILD)
> 
> -- 
> 2.47.0
>
diff mbox series

Patch

diff --git a/arch/arm/mach-rockchip/rk3399/rk3399.c b/arch/arm/mach-rockchip/rk3399/rk3399.c
index 7b6a822ed04b8151a5da147056dbf73ffdafd149..26fd04691699d81f5fecd282964c7b3aff999717 100644
--- a/arch/arm/mach-rockchip/rk3399/rk3399.c
+++ b/arch/arm/mach-rockchip/rk3399/rk3399.c
@@ -169,7 +169,7 @@  void board_debug_uart_init(void)
 }
 #endif
 
-#if defined(CONFIG_XPL_BUILD) && !defined(CONFIG_TPL_BUILD)
+#if defined(CONFIG_XPL_BUILD)
 static void rk3399_force_power_on_reset(void)
 {
 	const struct rockchip_cru *cru = rockchip_get_cru();
@@ -195,9 +195,9 @@  static void rk3399_force_power_on_reset(void)
 	if (cru->glb_rst_st == 0)
 		return;
 
-	if (!IS_ENABLED(CONFIG_SPL_GPIO)) {
+	if (!CONFIG_IS_ENABLED(GPIO)) {
 		debug("%s: trying to force a power-on reset but no GPIO "
-		      "support in SPL!\n", __func__);
+		      "support in " PHASE_NAME "!\n", __func__);
 		return;
 	}
 
@@ -218,6 +218,7 @@  static void rk3399_force_power_on_reset(void)
 	dm_gpio_set_value(&sysreset_gpio, 1);
 }
 
+# if !defined(CONFIG_TPL_BUILD)
 void __weak led_setup(void)
 {
 }
@@ -228,4 +229,10 @@  void spl_board_init(void)
 
 	rk3399_force_power_on_reset();
 }
-#endif
+# else  // !defined(CONFIG_TPL_BUILD)
+void tpl_board_init(void)
+{
+	rk3399_force_power_on_reset();
+}
+# endif // !defined(CONFIG_TPL_BUILD)
+#endif // defined(CONFIG_XPL_BUILD)