[12/19] rockchip: rk3588: Update bootph props

After the commit aca95282c1b7 ("Makefile: Use the fdtgrep -u flag")
bootph props is propagating to parent nodes.

Update bootph props to ensure eMMC, SD-card and SPI flash is available
in SPL and U-Boot proper pre-reloc phase also remove unneeded bootph
props that automatically is propagated to parent nodes.

Also adjust pinctrl nodes to only be included in boot phases where they
are needed and add any missing pinctrl node needed in SPL.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 .../arm/dts/rk3588-coolpi-cm5-evb-u-boot.dtsi |  5 ++-
 arch/arm/dts/rk3588-generic.dts               |  1 +
 arch/arm/dts/rk3588-nanopc-t6-u-boot.dtsi     |  9 +++--
 .../dts/rk3588-orangepi-5-plus-u-boot.dtsi    |  6 ++--
 arch/arm/dts/rk3588-rock-5b-u-boot.dtsi       |  5 ++-
 arch/arm/dts/rk3588-turing-rk1-u-boot.dtsi    |  6 +++-
 arch/arm/dts/rk3588s-coolpi-4b-u-boot.dtsi    |  5 ++-
 arch/arm/dts/rk3588s-orangepi-5-u-boot.dtsi   |  8 ++---
 arch/arm/dts/rk3588s-u-boot.dtsi              | 36 ++++++++++++-------
 configs/evb-rk3588_defconfig                  |  4 ++-
 10 files changed, 48 insertions(+), 37 deletions(-)

Hi Jonas,

On 3/29/24 20:01, Jonas Karlman wrote:

[...]

> diff --git a/arch/arm/dts/rk3588s-u-boot.dtsi b/arch/arm/dts/rk3588s-u-boot.dtsi
> index e4171bd24d2a..a502a82fae6a 100644
> --- a/arch/arm/dts/rk3588s-u-boot.dtsi
> +++ b/arch/arm/dts/rk3588s-u-boot.dtsi
> @@ -121,31 +121,35 @@
>   };
>   
>   &cru {
> -	bootph-pre-ram;
> +	bootph-all;
>   };
>   
>   &emmc_bus8 {
> -	bootph-all;
> +	bootph-pre-ram;
>   };
>   
>   &emmc_clk {
> -	bootph-all;
> +	bootph-pre-ram;
>   };
>   
>   &emmc_cmd {
> -	bootph-all;
> +	bootph-pre-ram;
>   };
>   
>   &emmc_data_strobe {
> -	bootph-all;
> +	bootph-pre-ram;
>   };
>   
>   &emmc_rstnout {
> -	bootph-all;
> +	bootph-pre-ram;
>   };
>   
>   &ioc {
> -	bootph-pre-ram;
> +	bootph-all;
> +};
> +
> +&pcfg_pull_down {
> +	bootph-all;
>   };
>   
>   &pcfg_pull_none {
> @@ -157,6 +161,10 @@
>   };
>   
>   &pcfg_pull_up_drv_level_2 {
> +	bootph-pre-ram;
> +};
> +
> +&php_grf {
>   	bootph-all;
>   };
>   
> @@ -189,19 +197,23 @@
>   };
>   
>   &sdmmc_bus4 {
> -	bootph-all;
> +	bootph-pre-ram;
>   };
>   
>   &sdmmc_clk {
> -	bootph-all;
> +	bootph-pre-ram;
>   };
>   
>   &sdmmc_cmd {
> -	bootph-all;
> +	bootph-pre-ram;
>   };
>   
>   &sdmmc_det {
> -	bootph-all;
> +	bootph-pre-ram;
> +};
> +

Please add bootph-some-ram to all nodes related to eMMC/SD card 
otherwise I assume some boards won't work anymore (e.g. the ones that 
need to find MMC devices through DT in arch_env_get_location, e.g. 
Theobroma's Jaguar (and soon Tiger)).

c.f. 
https://source.denx.de/u-boot/u-boot/-/commit/70f9212d61fe79c605b805c6eb0764b29f8ae3b6

It was not easy to have this merged, so I'd prefer to avoid having to go 
through it again to fix my board(s) :)

It'd be nice to split this into multiple commits so we can have some 
individual justification of why such a change is made, so that we know 
if we need to revert/update it in the future.

Cheers,
Quentin

Hi Quentin,

On 2024-04-02 13:20, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 3/29/24 20:01, Jonas Karlman wrote:
> 
> [...]
> 
>> diff --git a/arch/arm/dts/rk3588s-u-boot.dtsi b/arch/arm/dts/rk3588s-u-boot.dtsi
>> index e4171bd24d2a..a502a82fae6a 100644
>> --- a/arch/arm/dts/rk3588s-u-boot.dtsi
>> +++ b/arch/arm/dts/rk3588s-u-boot.dtsi
>> @@ -121,31 +121,35 @@
>>   };
>>   
>>   &cru {
>> -	bootph-pre-ram;
>> +	bootph-all;
>>   };
>>   
>>   &emmc_bus8 {
>> -	bootph-all;
>> +	bootph-pre-ram;
>>   };
>>   
>>   &emmc_clk {
>> -	bootph-all;
>> +	bootph-pre-ram;
>>   };
>>   
>>   &emmc_cmd {
>> -	bootph-all;
>> +	bootph-pre-ram;
>>   };
>>   
>>   &emmc_data_strobe {
>> -	bootph-all;
>> +	bootph-pre-ram;
>>   };
>>   
>>   &emmc_rstnout {
>> -	bootph-all;
>> +	bootph-pre-ram;
>>   };
>>   
>>   &ioc {
>> -	bootph-pre-ram;
>> +	bootph-all;
>> +};
>> +
>> +&pcfg_pull_down {
>> +	bootph-all;
>>   };
>>   
>>   &pcfg_pull_none {
>> @@ -157,6 +161,10 @@
>>   };
>>   
>>   &pcfg_pull_up_drv_level_2 {
>> +	bootph-pre-ram;
>> +};
>> +
>> +&php_grf {
>>   	bootph-all;
>>   };
>>   
>> @@ -189,19 +197,23 @@
>>   };
>>   
>>   &sdmmc_bus4 {
>> -	bootph-all;
>> +	bootph-pre-ram;
>>   };
>>   
>>   &sdmmc_clk {
>> -	bootph-all;
>> +	bootph-pre-ram;
>>   };
>>   
>>   &sdmmc_cmd {
>> -	bootph-all;
>> +	bootph-pre-ram;
>>   };
>>   
>>   &sdmmc_det {
>> -	bootph-all;
>> +	bootph-pre-ram;
>> +};
>> +
> 
> Please add bootph-some-ram to all nodes related to eMMC/SD card 
> otherwise I assume some boards won't work anymore (e.g. the ones that 
> need to find MMC devices through DT in arch_env_get_location, e.g. 
> Theobroma's Jaguar (and soon Tiger)).

Sure I will add them in a v2.

When I tested your arch_env_get_location() it did not seem to require
any pinctrl nodes at pre-reloc stage.

Ideally we should be able to skip use of DM, env and serial at pre-reloc
stage for Rockchip. Serial and pinctrl already gets configured in SPL, and
use of DM slows down boot by 200-700ms, but that is for another series ;-)

> 
> c.f. 
> https://source.denx.de/u-boot/u-boot/-/commit/70f9212d61fe79c605b805c6eb0764b29f8ae3b6
> 
> It was not easy to have this merged, so I'd prefer to avoid having to go 
> through it again to fix my board(s) :)
> 
> It'd be nice to split this into multiple commits so we can have some 
> individual justification of why such a change is made, so that we know 
> if we need to revert/update it in the future.

Will try to split this in v2.

In summary some pinctrl or nodes referenced was not enabled for SPL
stage, and some was enabled for TPL that should never be needed in TPL,
if U-Boot TPL will be used in future.

TPL: uart node + dmc/ram and any node required/referenced
SPL: TPL + sdmmc/sdhci/spi-flash nodes and related pinctrl
pre-reloc: TPL + sdmmc/sdhci/spi-flash nodes

I created a python script [1] that can check the built tpl/spl/proper
dtb for missing nodes and some config options for all soc targets.

[1] https://gist.github.com/Kwiboo/34c099fb42eb6ae5ed515a04275a7ed7

Regards,
Jonas

> 
> Cheers,
> Quentin

Hi Jonas,

On 4/2/24 15:36, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2024-04-02 13:20, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 3/29/24 20:01, Jonas Karlman wrote:
>>
>> [...]
>>
>>> diff --git a/arch/arm/dts/rk3588s-u-boot.dtsi b/arch/arm/dts/rk3588s-u-boot.dtsi
>>> index e4171bd24d2a..a502a82fae6a 100644
>>> --- a/arch/arm/dts/rk3588s-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3588s-u-boot.dtsi
>>> @@ -121,31 +121,35 @@
>>>    };
>>>    
>>>    &cru {
>>> -	bootph-pre-ram;
>>> +	bootph-all;
>>>    };
>>>    
>>>    &emmc_bus8 {
>>> -	bootph-all;
>>> +	bootph-pre-ram;
>>>    };
>>>    
>>>    &emmc_clk {
>>> -	bootph-all;
>>> +	bootph-pre-ram;
>>>    };
>>>    
>>>    &emmc_cmd {
>>> -	bootph-all;
>>> +	bootph-pre-ram;
>>>    };
>>>    
>>>    &emmc_data_strobe {
>>> -	bootph-all;
>>> +	bootph-pre-ram;
>>>    };
>>>    
>>>    &emmc_rstnout {
>>> -	bootph-all;
>>> +	bootph-pre-ram;
>>>    };
>>>    
>>>    &ioc {
>>> -	bootph-pre-ram;
>>> +	bootph-all;
>>> +};
>>> +
>>> +&pcfg_pull_down {
>>> +	bootph-all;
>>>    };
>>>    
>>>    &pcfg_pull_none {
>>> @@ -157,6 +161,10 @@
>>>    };
>>>    
>>>    &pcfg_pull_up_drv_level_2 {
>>> +	bootph-pre-ram;
>>> +};
>>> +
>>> +&php_grf {
>>>    	bootph-all;
>>>    };
>>>    
>>> @@ -189,19 +197,23 @@
>>>    };
>>>    
>>>    &sdmmc_bus4 {
>>> -	bootph-all;
>>> +	bootph-pre-ram;
>>>    };
>>>    
>>>    &sdmmc_clk {
>>> -	bootph-all;
>>> +	bootph-pre-ram;
>>>    };
>>>    
>>>    &sdmmc_cmd {
>>> -	bootph-all;
>>> +	bootph-pre-ram;
>>>    };
>>>    
>>>    &sdmmc_det {
>>> -	bootph-all;
>>> +	bootph-pre-ram;
>>> +};
>>> +
>>
>> Please add bootph-some-ram to all nodes related to eMMC/SD card
>> otherwise I assume some boards won't work anymore (e.g. the ones that
>> need to find MMC devices through DT in arch_env_get_location, e.g.
>> Theobroma's Jaguar (and soon Tiger)).
> 
> Sure I will add them in a v2.
> 
> When I tested your arch_env_get_location() it did not seem to require
> any pinctrl nodes at pre-reloc stage.
> 

Mmmmmm... I assume it is because the device checked in 
arch_env_get_location() in U-Boot proper pre-reloc is necessarily the 
same as the one used to load U-Boot proper from SPL, which means the SPL 
will have set the mux correctly (because of bootph-pre-ram). I don't 
think U-Boot proper pre-reloc has a separate DTB from U-Boot proper, so 
I think it checks only for boopth-some-ram property in the MMC 
controller node, and maybe not for pinctrl nodes?

> Ideally we should be able to skip use of DM, env and serial at pre-reloc
> stage for Rockchip. Serial and pinctrl already gets configured in SPL, and
> use of DM slows down boot by 200-700ms, but that is for another series ;-)
> 

Agreed :)

>>
>> c.f.
>> https://source.denx.de/u-boot/u-boot/-/commit/70f9212d61fe79c605b805c6eb0764b29f8ae3b6
>>
>> It was not easy to have this merged, so I'd prefer to avoid having to go
>> through it again to fix my board(s) :)
>>
>> It'd be nice to split this into multiple commits so we can have some
>> individual justification of why such a change is made, so that we know
>> if we need to revert/update it in the future.
> 
> Will try to split this in v2.
> 
> In summary some pinctrl or nodes referenced was not enabled for SPL
> stage, and some was enabled for TPL that should never be needed in TPL,
> if U-Boot TPL will be used in future.
> 
> TPL: uart node + dmc/ram and any node required/referenced
> SPL: TPL + sdmmc/sdhci/spi-flash nodes and related pinctrl

For the <soc>-u-boot.dtsi, I guess this is good enough, we may need more 
though for some boards (e.g. emmc-reset, gpios, etc...).

> pre-reloc: TPL + sdmmc/sdhci/spi-flash nodes
> 
> I created a python script [1] that can check the built tpl/spl/proper
> dtb for missing nodes and some config options for all soc targets.
> 
> [1] https://gist.github.com/Kwiboo/34c099fb42eb6ae5ed515a04275a7ed7
> 

Ooooooh this seems really neat. I'm always very careful when adding 
support for a new board and I'm always afraid to miss a few nodes, I'll 
try to not forget to have a look at this for adding support for Tiger 
RK3588 :)

Do you have any plan of submitting this on the ML? I think this could be 
beneficial to the project!

Cheers,
Quentin