Re: [PATCH 13/15] rockchip: rk3328: Add support to build bootable SPI image

[Dragan Simic <dsimic@manjaro.org>]

On 2024-02-10 20:30, Jonas Karlman wrote:
> On 2024-02-10 18:18, Dragan Simic wrote:
>> On 2024-02-08 22:54, Jonas Karlman wrote:
>>> On 2024-02-08 04:53, Dragan Simic wrote:
>>>> On 2024-02-07 01:02, Jonas Karlman wrote:
>>>>> Similar to RK35xx the BootRom in RK3328 can read all data and look
>>>>> for
>>>>> idbloader at 0x8000, same as on SD and eMMC.
>>>>> 
>>>>> Use the rksd format and modify the mkimage offset to generate a
>>>>> bootable
>>>>> u-boot-rockchip-spi.bin that can be written to 0x0 of SPI NOR 
>>>>> flash.
>>>>> 
>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> 
>>>> Could you, please, clarify a bit why the "rkspi" format isn't used
>>>> instead of "rksd"?
>>> 
>>> Not really sure how I can clarify this.
>>> 
>>> As stated in commit message, the bootrom will read all bytes and use
>>> same boot offset as the sd/emmc-format. The "legacy" spi-format only
>>> read first 2kb of each 4kb of storage, see tools/rkspi.c.
>>> 
>>> A few bootrom versions:
>>> - rk3288: 320A 20131116 V100
>>> - rk3399: 330C 20160118 V100
>>> - rk3328: 320C 20161117 V100
>>> - rk3308: 330E 20180203 V100
>>> 
>>> I am not sure if the rkspi format must be used for all bootrom prior 
>>> to
>>> and including rk3399 and all versions after that should use the rksd
>>> format. I have only ever used boot from spi flash on rk3399, rk3328 
>>> and
>>> rk35xx. And for those that has been true.
>> 
>> Ah, thank you very much for refreshing my memory.  I somehow forgot
>> about the bug that the RK3399 BROM suffers from.
>> 
>> Please see also one small suggestion below.
>> 
>>>>> ---
>>>>>  arch/arm/dts/rk3328-u-boot.dtsi        | 11 +++++++++++
>>>>>  arch/arm/mach-rockchip/rk3328/rk3328.c |  1 +
>>>>>  2 files changed, 12 insertions(+)
>>>>> 
>>>>> diff --git a/arch/arm/dts/rk3328-u-boot.dtsi
>>>>> b/arch/arm/dts/rk3328-u-boot.dtsi
>>>>> index b90d78878d77..2a5dca97dd4b 100644
>>>>> --- a/arch/arm/dts/rk3328-u-boot.dtsi
>>>>> +++ b/arch/arm/dts/rk3328-u-boot.dtsi
>>>>> @@ -120,3 +120,14 @@
>>>>>  &usb20_otg {
>>>>>  	hnp-srp-disable;
>>>>>  };
>>>>> +
>>>>> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
>>>>> +&binman {
>>>>> +	simple-bin-spi {
>>>>> +		mkimage {
>>>>> +			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
>> 
>> Perhaps there's no need to use CONFIG_SYS_SOC there, because this is 
>> an
>> U-Boot SoC dtsi specific to the RK3328.  Furthermore, I find it much
>> more readable (and grep-able, which is also important) when the exact
>> name of the SoC is specified here.
>> 
>> This note also applies to the other U-Boot SoC dtsi files.
> 
> Consistency is my main concern so I would rather keep using SYS_SOC, it
> is already being used at the two other override locations.

I agree that consistency is good.  Just checking, are you referring
to the use of CONFIG_SYS_SOC in arch/arm/dts/rk356x-u-boot.dtsi and
arch/arm/dts/rk3588s-u-boot.dtsi?  Frankly, I'd much rather see rk3568
and rk3588 there, which would be much more readable.

I mean, is CONFIG_SYS_SOC in each of the U-Boot SoC dtsi files ever
going to be substituted with something else than the already known
"rkxxxx" value?  If so, IMHO there's no valid point in not using the
known value directly.

> If we can get confirmation that it is brom in SoCs chronologically up 
> to
> and including rk3399 that is affected by the read 2kb per 4kb of spi
> issue we should be able to rework this into a more proper and future
> proof way.

It would be nice to have some kind of an official confirmation for that.

>>>>> +			offset = <0x8000>;
>>>>> +		};
>>>>> +	};
>>>>> +};
>>>>> +#endif
>>>>> diff --git a/arch/arm/mach-rockchip/rk3328/rk3328.c
>>>>> b/arch/arm/mach-rockchip/rk3328/rk3328.c
>>>>> index b591d38fe412..b82b209de9e2 100644
>>>>> --- a/arch/arm/mach-rockchip/rk3328/rk3328.c
>>>>> +++ b/arch/arm/mach-rockchip/rk3328/rk3328.c
>>>>> @@ -36,6 +36,7 @@
>>>>> 
>>>>>  const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
>>>>>  	[BROM_BOOTSOURCE_EMMC] = "/mmc@ff520000",
>>>>> +	[BROM_BOOTSOURCE_SPINOR] = "/spi@ff190000/flash@0",
>>>>>  	[BROM_BOOTSOURCE_SD] = "/mmc@ff500000",
>>>>>  };