Re: [PATCH next v2 4/4] rockchip: rk3399: move sysreset-gpio logic to TPL

2024-11-12 Thread Dragan Simic

Hello Kever,

On 2024-11-08 01:50, Kever Yang wrote:

On 2024/11/7 20:27, Paul Kocialkowski wrote:

Le Thu 07 Nov 24, 12:04, Quentin Schulz a écrit :

On 11/7/24 8:14 AM, Kever Yang wrote:

Yes, this movement is reasonable and needed for this workaround,
although still not

understand why puma board need this.

Me neither, this predates me joining the company, c.f.:
https://source.denx.de/u-boot/u-boot/-/commit/ae0d33a7291a164a11ae034bcf4f71226b2bef48
https://source.denx.de/u-boot/u-boot/-/commit/5f104178bf713615dc404fdfcf0fb53d89c66a07
https://source.denx.de/u-boot/u-boot/-/commit/07586ee4322abca01db52624b925e5218538f259

What I can tell you is that it seems this is required as Paul (in Cc) 
is

trying to add support for it for the Firefly ROC-RK3399-PC[1] and the
ROCKPro64[2], so it seems it's useful for **some** purpose.
The initial issue I was seeing was that one of the MMC controllers 
showed some
errors and failed to read data after reboot, while it always worked on 
cold

boot. With this feature enabled, it worked reliably after reboot.

I didn't investigate exactly which component (maybe clocks, maybe 
regulators)

caused the problem, but it clearly wasn't correctly reset.


Are you able to check, how is the 'reboot' happen, does it use the
system_reset API from the ATF?

Because there are some operations for clock and PD before trigger the
global reset in ATF driver and the code is very stable and used for
millions of devices.


The Linux kernel performs resets using PSCI_0_2_FN_SYSTEM_RESET
made available by TF-A, which ends up using GLB_SRST_FST_CFG_VAL,
also known as the RK3390's global software reset 1, which I'll
describe a bit further later in my response.

In this particular case and to the best of my knowledge, it isn't
up to the way the SoC reset is initiated and performed, but up to
the way RK3399 (mis)performs the global software reset 1.

Here's a quotation from arch/arm/mach-rockchip/rk3399/rk3399.c
that explains this issue a bit further:

  /*
   * The RK3399 resets only 'almost all logic' (see also in the
   * TRM "3.9.4 Global software reset"), when issuing a software
   * reset. This may cause issues during boot-up for some
   * configurations of the application software stack.
   *
   * To work around this, we test whether the last reset reason
   * was a power-on reset and (if not) issue an overtemp-reset to
   * reset the entire module.
   *
   * While this was previously fixed by modifying the various
   * places that could generate a software reset (e.g. U-Boot's
   * sysreset driver, the ATF or Linux), we now have it here to
   * ensure that we no longer have to track this through the
   * various components.
   */

The logic associated with this comment, currently residing in
arch/arm/mach-rockchip/rk3399/rk3399.c, was actually introduced
in the commit ae0d33a7291a (rockchip: rk3399-puma: add code to
allow forcing a power-on reset, 2017-11-28).

As described in section 3.4 of the RK3399 TRM, part 1 of the
version 1.1, the RK3399 provides two global software reset types,
out of which TF-A, as already described, properly performs the
global software reset 1.  Alas, section 3.9.4 of the RK3399 TRM
specifies that "almost all logic" gets reset, which is the source
of all troubles, resulting in the need to perform hardware resets
using (or better said, abusing a bit) the PMIC's overtemperature
protection (OTP) interface/pin.

I hope all this makes sense.

[1] 
https://lore.kernel.org/u-boot/20240926183111.1324284-1-pa...@sys-base.io/
[2] 
https://lore.kernel.org/u-boot/20240926183111.1324284-2-pa...@sys-base.io/


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

"""

So with this patch set, we can see the TPL banner twice ?


PS: We are able to merge to master instead of next before next 
branch is

open, because we still have

enough time to debug before next release.

My understanding is that once -rc1 is out, we should only do bug 
fixing in
master. BUT at the same time, next branch isn't actually open until 
-rc2.


Up to you! It's not really urgent, Puma was migrated to TPL only in 
v2023.01

and we've lived without sysreset-gpio in TPL since then :)


Re: [PATCH next v2 4/4] rockchip: rk3399: move sysreset-gpio logic to TPL

2024-11-07 Thread Kever Yang



On 2024/11/6 19:29, Quentin Schulz wrote:

From: Quentin Schulz 

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. Because there's currently only one user of sysreset-gpio logic,
and TPL is enabled on that board, so let's migrate the logic and that
board to do it in TPL.

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 

Reviewed-by: Kever Yang 

Thanks,
- Kever

---
  arch/arm/mach-rockchip/rk3399/rk3399.c | 15 ++-
  configs/puma-rk3399_defconfig  |  3 +++
  2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-rockchip/rk3399/rk3399.c 
b/arch/arm/mach-rockchip/rk3399/rk3399.c
index 
7b6a822ed04b8151a5da147056dbf73ffdafd149..0c28241c603e343c5140322f2778678e95fa84fd
 100644
--- a/arch/arm/mach-rockchip/rk3399/rk3399.c
+++ b/arch/arm/mach-rockchip/rk3399/rk3399.c
@@ -169,7 +169,8 @@ void board_debug_uart_init(void)
  }
  #endif
  
-#if defined(CONFIG_XPL_BUILD) && !defined(CONFIG_TPL_BUILD)

+#if defined(CONFIG_XPL_BUILD)
+#if defined(CONFIG_TPL_BUILD)
  static void rk3399_force_power_on_reset(void)
  {
const struct rockchip_cru *cru = rockchip_get_cru();
@@ -195,9 +196,9 @@ static void rk3399_force_power_on_reset(void)
if (cru->glb_rst_st == 0)
return;
  
-	if (!IS_ENABLED(CONFIG_SPL_GPIO)) {

+   if (!IS_ENABLED(CONFIG_TPL_GPIO)) {
debug("%s: trying to force a power-on reset but no GPIO "
- "support in SPL!\n", __func__);
+ "support in TPL!\n", __func__);
return;
}
  
@@ -218,6 +219,11 @@ static void rk3399_force_power_on_reset(void)

dm_gpio_set_value(&sysreset_gpio, 1);
  }
  
+void tpl_board_init(void)

+{
+   rk3399_force_power_on_reset();
+}
+# else
  void __weak led_setup(void)
  {
  }
@@ -225,7 +231,6 @@ void __weak led_setup(void)
  void spl_board_init(void)
  {
led_setup();
-
-   rk3399_force_power_on_reset();
  }
  #endif
+#endif
diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig
index 
67c0ee72c925cdd49066980b0fde4131c86a99a8..7a180b1413036234d834773778f6c0f0a7e85380
 100644
--- a/configs/puma-rk3399_defconfig
+++ b/configs/puma-rk3399_defconfig
@@ -30,6 +30,7 @@ CONFIG_SPL_I2C=y
  CONFIG_SPL_POWER=y
  CONFIG_SPL_SPI_LOAD=y
  CONFIG_TPL=y
+CONFIG_TPL_GPIO=y
  # CONFIG_BOOTM_NETBSD is not set
  # CONFIG_BOOTM_PLAN9 is not set
  # CONFIG_BOOTM_RTEMS is not set
@@ -78,6 +79,8 @@ CONFIG_ETH_DESIGNWARE=y
  CONFIG_GMAC_ROCKCHIP=y
  CONFIG_PHY_ROCKCHIP_INNO_USB2=y
  CONFIG_PHY_ROCKCHIP_TYPEC=y
+CONFIG_TPL_PINCTRL=y
+CONFIG_TPL_PINCTRL_FULL=y
  CONFIG_DM_PMIC_FAN53555=y
  CONFIG_PMIC_RK8XX=y
  CONFIG_SPL_PMIC_RK8XX=y



Re: [PATCH next v2 4/4] rockchip: rk3399: move sysreset-gpio logic to TPL

2024-11-07 Thread Kever Yang

Hi Paul,

On 2024/11/7 20:27, Paul Kocialkowski wrote:

Hi,

Le Thu 07 Nov 24, 12:04, Quentin Schulz a écrit :

On 11/7/24 8:14 AM, Kever Yang wrote:

Yes, this movement is reasonable and needed for this workaround,
although still not

understand why puma board need this.

Me neither, this predates me joining the company, c.f.:
https://source.denx.de/u-boot/u-boot/-/commit/ae0d33a7291a164a11ae034bcf4f71226b2bef48
https://source.denx.de/u-boot/u-boot/-/commit/5f104178bf713615dc404fdfcf0fb53d89c66a07
https://source.denx.de/u-boot/u-boot/-/commit/07586ee4322abca01db52624b925e5218538f259

What I can tell you is that it seems this is required as Paul (in Cc) is
trying to add support for it for the Firefly ROC-RK3399-PC[1] and the
ROCKPro64[2], so it seems it's useful for **some** purpose.

The initial issue I was seeing was that one of the MMC controllers showed some
errors and failed to read data after reboot, while it always worked on cold
boot. With this feature enabled, it worked reliably after reboot.

I didn't investigate exactly which component (maybe clocks, maybe regulators)
caused the problem, but it clearly wasn't correctly reset.


Are you able to check, how is the 'reboot' happen, does it use the 
system_reset API from the ATF?


Because there are some operations for clock and PD before trigger the 
global reset in ATF driver


and the code is very stable and used for millions of devices.


Thanks,
- Kever


Cheers,

Paul


[1]
https://lore.kernel.org/u-boot/20240926183111.1324284-1-pa...@sys-base.io/
[2]
https://lore.kernel.org/u-boot/20240926183111.1324284-2-pa...@sys-base.io/


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

So with this patch set, we can see the TPL banner twice ?


PS: We are able to merge to master instead of next before next branch is
open, because we still have

enough time to debug before next release.


My understanding is that once -rc1 is out, we should only do bug fixing in
master. BUT at the same time, next branch isn't actually open until -rc2.

Up to you! It's not really urgent, Puma was migrated to TPL only in v2023.01
and we've lived without sysreset-gpio in TPL since then :)

Cheers,
Quentin


Re: [PATCH next v2 4/4] rockchip: rk3399: move sysreset-gpio logic to TPL

2024-11-07 Thread Paul Kocialkowski
Hi,

Le Wed 06 Nov 24, 12:29, Quentin Schulz a écrit :
> From: Quentin Schulz 
> 
> 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. Because there's currently only one user of sysreset-gpio logic,
> and TPL is enabled on that board, so let's migrate the logic and that
> board to do it in TPL.
> 
> 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 

Thanks for keeping it only in TPL!

Reviewed-by: Paul Kocialkowski 

With just one small nit below:

> ---
>  arch/arm/mach-rockchip/rk3399/rk3399.c | 15 ++-
>  configs/puma-rk3399_defconfig  |  3 +++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/rk3399/rk3399.c 
> b/arch/arm/mach-rockchip/rk3399/rk3399.c
> index 
> 7b6a822ed04b8151a5da147056dbf73ffdafd149..0c28241c603e343c5140322f2778678e95fa84fd
>  100644
> --- a/arch/arm/mach-rockchip/rk3399/rk3399.c
> +++ b/arch/arm/mach-rockchip/rk3399/rk3399.c
> @@ -169,7 +169,8 @@ void board_debug_uart_init(void)
>  }
>  #endif
>  
> -#if defined(CONFIG_XPL_BUILD) && !defined(CONFIG_TPL_BUILD)
> +#if defined(CONFIG_XPL_BUILD)
> +#if defined(CONFIG_TPL_BUILD)
>  static void rk3399_force_power_on_reset(void)
>  {
>   const struct rockchip_cru *cru = rockchip_get_cru();
> @@ -195,9 +196,9 @@ static void rk3399_force_power_on_reset(void)
>   if (cru->glb_rst_st == 0)
>   return;
>  
> - if (!IS_ENABLED(CONFIG_SPL_GPIO)) {
> + if (!IS_ENABLED(CONFIG_TPL_GPIO)) {
>   debug("%s: trying to force a power-on reset but no GPIO "
> -   "support in SPL!\n", __func__);
> +   "support in TPL!\n", __func__);
>   return;
>   }
>  
> @@ -218,6 +219,11 @@ static void rk3399_force_power_on_reset(void)
>   dm_gpio_set_value(&sysreset_gpio, 1);
>  }
>  
> +void tpl_board_init(void)
> +{
> + rk3399_force_power_on_reset();
> +}
> +# else

No whitespace before "else" here.

>  void __weak led_setup(void)
>  {
>  }
> @@ -225,7 +231,6 @@ void __weak led_setup(void)
>  void spl_board_init(void)
>  {
>   led_setup();
> -
> - rk3399_force_power_on_reset();
>  }
>  #endif
> +#endif
> diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig
> index 
> 67c0ee72c925cdd49066980b0fde4131c86a99a8..7a180b1413036234d834773778f6c0f0a7e85380
>  100644
> --- a/configs/puma-rk3399_defconfig
> +++ b/configs/puma-rk3399_defconfig
> @@ -30,6 +30,7 @@ CONFIG_SPL_I2C=y
>  CONFIG_SPL_POWER=y
>  CONFIG_SPL_SPI_LOAD=y
>  CONFIG_TPL=y
> +CONFIG_TPL_GPIO=y
>  # CONFIG_BOOTM_NETBSD is not set
>  # CONFIG_BOOTM_PLAN9 is not set
>  # CONFIG_BOOTM_RTEMS is not set
> @@ -78,6 +79,8 @@ CONFIG_ETH_DESIGNWARE=y
>  CONFIG_GMAC_ROCKCHIP=y
>  CONFIG_PHY_ROCKCHIP_INNO_USB2=y
>  CONFIG_PHY_ROCKCHIP_TYPEC=y
> +CONFIG_TPL_PINCTRL=y
> +CONFIG_TPL_PINCTRL_FULL=y
>  CONFIG_DM_PMIC_FAN53555=y
>  CONFIG_PMIC_RK8XX=y
>  CONFIG_SPL_PMIC_RK8XX=y
> 
> -- 
> 2.47.0
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Specialist in multimedia, graphics and embedded hardware support with Linux.


signature.asc
Description: PGP signature


Re: [PATCH next v2 4/4] rockchip: rk3399: move sysreset-gpio logic to TPL

2024-11-07 Thread Paul Kocialkowski
Hi,

Le Thu 07 Nov 24, 12:04, Quentin Schulz a écrit :
> On 11/7/24 8:14 AM, Kever Yang wrote:
> > Yes, this movement is reasonable and needed for this workaround,
> > although still not
> > 
> > understand why puma board need this.
> 
> Me neither, this predates me joining the company, c.f.:
> https://source.denx.de/u-boot/u-boot/-/commit/ae0d33a7291a164a11ae034bcf4f71226b2bef48
> https://source.denx.de/u-boot/u-boot/-/commit/5f104178bf713615dc404fdfcf0fb53d89c66a07
> https://source.denx.de/u-boot/u-boot/-/commit/07586ee4322abca01db52624b925e5218538f259
> 
> What I can tell you is that it seems this is required as Paul (in Cc) is
> trying to add support for it for the Firefly ROC-RK3399-PC[1] and the
> ROCKPro64[2], so it seems it's useful for **some** purpose.

The initial issue I was seeing was that one of the MMC controllers showed some
errors and failed to read data after reboot, while it always worked on cold
boot. With this feature enabled, it worked reliably after reboot.

I didn't investigate exactly which component (maybe clocks, maybe regulators)
caused the problem, but it clearly wasn't correctly reset.

Cheers,

Paul

> [1]
> https://lore.kernel.org/u-boot/20240926183111.1324284-1-pa...@sys-base.io/
> [2]
> https://lore.kernel.org/u-boot/20240926183111.1324284-2-pa...@sys-base.io/
> 
> > > - 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)
> > > """
> > So with this patch set, we can see the TPL banner twice ?
> > 
> > 
> > PS: We are able to merge to master instead of next before next branch is
> > open, because we still have
> > 
> > enough time to debug before next release.
> > 
> 
> My understanding is that once -rc1 is out, we should only do bug fixing in
> master. BUT at the same time, next branch isn't actually open until -rc2.
> 
> Up to you! It's not really urgent, Puma was migrated to TPL only in v2023.01
> and we've lived without sysreset-gpio in TPL since then :)
> 
> Cheers,
> Quentin

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Specialist in multimedia, graphics and embedded hardware support with Linux.


signature.asc
Description: PGP signature


Re: [PATCH next v2 4/4] rockchip: rk3399: move sysreset-gpio logic to TPL

2024-11-07 Thread Quentin Schulz

Hi Kever,

On 11/7/24 8:14 AM, Kever Yang wrote:

Hi Quentin,

On 2024/11/6 19:29, Quentin Schulz wrote:

From: Quentin Schulz 

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. Because there's currently only one user of sysreset-gpio logic,
and TPL is enabled on that board, so let's migrate the logic and that
board to do it in TPL.

There are three reasons for moving this earlier:


Yes, this movement is reasonable and needed for this workaround, 
although still not


understand why puma board need this.



Me neither, this predates me joining the company, c.f.:
https://source.denx.de/u-boot/u-boot/-/commit/ae0d33a7291a164a11ae034bcf4f71226b2bef48
https://source.denx.de/u-boot/u-boot/-/commit/5f104178bf713615dc404fdfcf0fb53d89c66a07
https://source.denx.de/u-boot/u-boot/-/commit/07586ee4322abca01db52624b925e5218538f259

What I can tell you is that it seems this is required as Paul (in Cc) is 
trying to add support for it for the Firefly ROC-RK3399-PC[1] and the 
ROCKPro64[2], so it seems it's useful for **some** purpose.


[1] 
https://lore.kernel.org/u-boot/20240926183111.1324284-1-pa...@sys-base.io/
[2] 
https://lore.kernel.org/u-boot/20240926183111.1324284-2-pa...@sys-base.io/



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

So with this patch set, we can see the TPL banner twice ?


PS: We are able to merge to master instead of next before next branch is 
open, because we still have


enough time to debug before next release.



My understanding is that once -rc1 is out, we should only do bug fixing 
in master. BUT at the same time, next branch isn't actually open until -rc2.


Up to you! It's not really urgent, Puma was migrated to TPL only in 
v2023.01 and we've lived without sysreset-gpio in TPL since then :)


Cheers,
Quentin


Re: [PATCH next v2 4/4] rockchip: rk3399: move sysreset-gpio logic to TPL

2024-11-06 Thread Kever Yang

Hi Quentin,

On 2024/11/6 19:29, Quentin Schulz wrote:

From: Quentin Schulz 

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. Because there's currently only one user of sysreset-gpio logic,
and TPL is enabled on that board, so let's migrate the logic and that
board to do it in TPL.

There are three reasons for moving this earlier:


Yes, this movement is reasonable and needed for this workaround, 
although still not


understand why puma board need this.


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

So with this patch set, we can see the TPL banner twice ?


PS: We are able to merge to master instead of next before next branch is 
open, because we still have


enough time to debug before next release.


Thanks,
- Kever


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 
---
  arch/arm/mach-rockchip/rk3399/rk3399.c | 15 ++-
  configs/puma-rk3399_defconfig  |  3 +++
  2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-rockchip/rk3399/rk3399.c 
b/arch/arm/mach-rockchip/rk3399/rk3399.c
index 
7b6a822ed04b8151a5da147056dbf73ffdafd149..0c28241c603e343c5140322f2778678e95fa84fd
 100644
--- a/arch/arm/mach-rockchip/rk3399/rk3399.c
+++ b/arch/arm/mach-rockchip/rk3399/rk3399.c
@@ -169,7 +169,8 @@ void board_debug_uart_init(void)
  }
  #endif
  
-#if defined(CONFIG_XPL_BUILD) && !defined(CONFIG_TPL_BUILD)

+#if defined(CONFIG_XPL_BUILD)
+#if defined(CONFIG_TPL_BUILD)
  static void rk3399_force_power_on_reset(void)
  {
const struct rockchip_cru *cru = rockchip_get_cru();
@@ -195,9 +196,9 @@ static void rk3399_force_power_on_reset(void)
if (cru->glb_rst_st == 0)
return;
  
-	if (!IS_ENABLED(CONFIG_SPL_GPIO)) {

+   if (!IS_ENABLED(CONFIG_TPL_GPIO)) {
debug("%s: trying to force a power-on reset but no GPIO "
- "support in SPL!\n", __func__);
+ "support in TPL!\n", __func__);
return;
}
  
@@ -218,6 +219,11 @@ static void rk3399_force_power_on_reset(void)

dm_gpio_set_value(&sysreset_gpio, 1);
  }
  
+void tpl_board_init(void)

+{
+   rk3399_force_power_on_reset();
+}
+# else
  void __weak led_setup(void)
  {
  }
@@ -225,7 +231,6 @@ void __weak led_setup(void)
  void spl_board_init(void)
  {
led_setup();
-
-   rk3399_force_power_on_reset();
  }
  #endif
+#endif
diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig
index 
67c0ee72c925cdd49066980b0fde4131c86a99a8..7a180b1413036234d834773778f6c0f0a7e85380
 100644
--- a/configs/puma-rk3399_defconfig
+++ b/configs/puma-rk3399_defconfig
@@ -30,6 +30,7 @@ CONFIG_SPL_I2C=y
  CONFIG_SPL_POWER=y
  CONFIG_SPL_SPI_LOAD=y
  CONFIG_TPL=y
+CONFIG_TPL_GPIO=y
  # CONFIG_BOOTM_NETBSD is not set
  # CONFIG_BOOTM_PLAN9 is not set
  # CONFIG_BOOTM_RTEMS is not set
@@ -78,6 +79,8 @@ CONFIG_ETH_DESIGNWARE=y
  CONFIG_GMAC_ROCKCHIP=y
  CONFIG_PHY_ROCKCHIP_INNO_USB2=y
  CONFIG_PHY_ROCKCHIP_TYPEC=y
+CONFIG_TPL_PINCTRL=y
+CONFIG_TPL_PINCTRL_FULL=y
  CONFIG_DM_PMIC_FAN53555=y
  CONFIG_PMIC_RK8XX=y
  CONFIG_SPL_PMIC_RK8XX=y