Re: [PATCH v2 07/23] rockchip: rk35xx: Enable random generator

[Dragan Simic <dsimic@manjaro.org>]

On 2024-04-15 10:03, Jonas Karlman wrote:
> On 2024-04-15 08:37, Dragan Simic wrote:
>> Please see a couple of comments below.
>> 
>> On 2024-04-13 20:13, Jonas Karlman wrote:
>>> The RK35xx SoCs contain a crypto engine block that can generate 
>>> random
>>> numbers.
>>> 
>>> Enable rng node in soc u-boot.dtsi and enable Kconfig options to take
>>> advantage of the random generator.
>>> 
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>> v2: No change
>>> ---
>>>  arch/arm/dts/rk3566-anbernic-rgxx3-u-boot.dtsi | 6 ------
>>>  arch/arm/dts/rk356x-u-boot.dtsi                | 5 +++++
>>>  arch/arm/dts/rk3588s-u-boot.dtsi               | 1 -
>>>  arch/arm/mach-rockchip/Kconfig                 | 4 ++++
>>>  configs/anbernic-rgxx3-rk3566_defconfig        | 2 --
>>>  5 files changed, 9 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/arch/arm/dts/rk3566-anbernic-rgxx3-u-boot.dtsi
>>> b/arch/arm/dts/rk3566-anbernic-rgxx3-u-boot.dtsi
>>> index 791f16b206f2..793ed4ae8ae0 100644
>>> --- a/arch/arm/dts/rk3566-anbernic-rgxx3-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3566-anbernic-rgxx3-u-boot.dtsi
>>> @@ -6,12 +6,6 @@
>>>  	chosen {
>>>  		u-boot,spl-boot-order = "same-as-spl", &sdmmc1, &sdmmc0;
>>>  	};
>>> -
>>> -	rng: rng@fe388000 {
>>> -		compatible = "rockchip,cryptov2-rng";
>>> -		reg = <0x0 0xfe388000 0x0 0x2000>;
>>> -		status = "okay";
>>> -	};
>>>  };
>>> 
>>>  &dsi_dphy0 {
>>> diff --git a/arch/arm/dts/rk356x-u-boot.dtsi
>>> b/arch/arm/dts/rk356x-u-boot.dtsi
>>> index d347080577d9..05367216e118 100644
>>> --- a/arch/arm/dts/rk356x-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk356x-u-boot.dtsi
>>> @@ -21,6 +21,11 @@
>>>  		bootph-all;
>>>  	};
>>> 
>>> +	rng: rng@fe388000 {
>>> +		compatible = "rockchip,cryptov2-rng";
>>> +		reg = <0x0 0xfe388000 0x0 0x2000>;
>> 
>> Shouldn't
>> 
>>   +		status = "okay";
>> 
>> also be specified here?
> 
> It is not needed as "okay" is the implied status when status prop is
> undefined, and my understanding is that DT maintainers prefer not to
> include status = "okay" where the initial node is defined.
> 
> More correctly if status prop is defined it must explicitly have "okay"
> anything else result in a disabled node, removing the status prop
> entirely has the same effect as status = "okay" in a board file.

I see, thanks for the clarification.  In that case, there seem to be
a few redundant instances of 'status = "okay";' in various U-Boot dtsi
files, which should probably be deleted.

>>> +	};
>>> +
>>>  	otp: nvmem@fe38c000 {
>>>  		compatible = "rockchip,rk3568-otp";
>>>  		reg = <0x0 0xfe38c000 0x0 0x4000>;
>>> diff --git a/arch/arm/dts/rk3588s-u-boot.dtsi
>>> b/arch/arm/dts/rk3588s-u-boot.dtsi
>>> index ac67c777adea..233eb79d9ba2 100644
>>> --- a/arch/arm/dts/rk3588s-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3588s-u-boot.dtsi
>>> @@ -91,7 +91,6 @@
>>>  	rng: rng@fe378000 {
>>>  		compatible = "rockchip,trngv1";
>>>  		reg = <0x0 0xfe378000 0x0 0x200>;
>>> -		status = "disabled";
>> 
>> Shouldn't it be enabled instead?
> 
> Same as above, not defining any status prop result in node being 
> enabled.
> 
> Board u-boot.dtsi-files could disable the node with status = "disabled"
> if they absolutely do not want to have the rng node in U-Boot.

Thanks once again for the clarification.  Please include this tag:

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

>> 
>>>  	};
>>> 
>>>  	usbdp_phy0: phy@fed80000 {
>>> diff --git a/arch/arm/mach-rockchip/Kconfig
>>> b/arch/arm/mach-rockchip/Kconfig
>>> index 649c22618f36..247b9a3146c2 100644
>>> --- a/arch/arm/mach-rockchip/Kconfig
>>> +++ b/arch/arm/mach-rockchip/Kconfig
>>> @@ -305,9 +305,11 @@ config ROCKCHIP_RK3568
>>>  	select BOARD_LATE_INIT
>>>  	select DM_REGULATOR_FIXED
>>>  	select DM_RESET
>>> +	imply DM_RNG
>>>  	imply MISC_INIT_R
>>>  	imply MMC_HS200_SUPPORT if MMC_SDHCI_ROCKCHIP
>>>  	imply OF_LIBFDT_OVERLAY
>>> +	imply RNG_ROCKCHIP
>>>  	imply ROCKCHIP_COMMON_BOARD
>>>  	imply ROCKCHIP_OTP
>>>  	imply SPL_ATF_NO_PLATFORM_PARAM if SPL_ATF
>>> @@ -334,9 +336,11 @@ config ROCKCHIP_RK3588
>>>  	select DM_RESET
>>>  	imply BOOTSTD_FULL
>>>  	imply CLK_SCMI
>>> +	imply DM_RNG
>>>  	imply MISC_INIT_R
>>>  	imply MMC_HS200_SUPPORT if MMC_SDHCI_ROCKCHIP
>>>  	imply OF_LIBFDT_OVERLAY
>>> +	imply RNG_ROCKCHIP
>>>  	imply ROCKCHIP_COMMON_BOARD
>>>  	imply ROCKCHIP_OTP
>>>  	imply SCMI_FIRMWARE
>>> diff --git a/configs/anbernic-rgxx3-rk3566_defconfig
>>> b/configs/anbernic-rgxx3-rk3566_defconfig
>>> index 24b050c59b53..110237e798f9 100644
>>> --- a/configs/anbernic-rgxx3-rk3566_defconfig
>>> +++ b/configs/anbernic-rgxx3-rk3566_defconfig
>>> @@ -68,8 +68,6 @@ CONFIG_REGULATOR_RK8XX=y
>>>  CONFIG_PWM_ROCKCHIP=y
>>>  CONFIG_SPL_RAM=y
>>>  # CONFIG_RAM_ROCKCHIP_DEBUG is not set
>>> -CONFIG_DM_RNG=y
>>> -CONFIG_RNG_ROCKCHIP=y
>>>  # CONFIG_RNG_SMCCC_TRNG is not set
>>>  CONFIG_BAUDRATE=1500000
>>>  CONFIG_DEBUG_UART_SHIFT=2