On 2023-05-16 21:13, Eugen Hristev wrote:
> On 5/16/23 20:38, Jonas Karlman wrote:
>> Hi Eugen,
>>
>> On 2023-05-16 17:06, Eugen Hristev wrote:
>>> Hi Jonas,
>>>
>>> On 5/9/23 14:16, Kever Yang wrote:
>>>>
>>>> On 2023/4/22 09:23, Jonas Karlman wrote:
>>>>> Add sfc and flash node to device tree and config options to enable
>>>>> support for booting from SPI NOR flash on Radxa ROCK 5 Model B.
>>>>>
>>>>> Signed-off-by: Jonas Karlman <jo...@kwiboo.se>
>>>> Reviewed-by: Kever Yang <kever.y...@rock-chips.com>
>>>>
>>>> Thanks,
>>>> - Kever
>>>>> ---
>>>>> arch/arm/dts/rk3588-rock-5b-u-boot.dtsi | 24 ++++++++++++++++++++++++
>>>>> arch/arm/dts/rk3588s-u-boot.dtsi | 20 ++++++++++++++++++++
>>>>> arch/arm/mach-rockchip/rk3588/rk3588.c | 1 +
>>>>> configs/rock5b-rk3588_defconfig | 10 ++++++++++
>>>>> 4 files changed, 55 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/dts/rk3588-rock-5b-u-boot.dtsi
>>>>> b/arch/arm/dts/rk3588-rock-5b-u-boot.dtsi
>>>>> index 4bbc19058c90..b63dd40deb6d 100644
>>>>> --- a/arch/arm/dts/rk3588-rock-5b-u-boot.dtsi
>>>>> +++ b/arch/arm/dts/rk3588-rock-5b-u-boot.dtsi
>>>>> @@ -11,6 +11,7 @@
>>>>> / {
>>>>> aliases {
>>>>> mmc1 = &sdmmc;
>>>>> + spi0 = &sfc;
>>>>> };
>>>>> chosen {
>>>>> @@ -43,6 +44,25 @@
>>>>> pinctrl-0 = <&emmc_bus8 &emmc_clk &emmc_cmd &emmc_data_strobe
>>>>> &emmc_rstnout>;
>>>>> };
>>>>> +&sfc {
>>>>> + bootph-pre-ram;
>>>
>>> Any reason why the sfc and flash are pre-ram and the pins bootph-all ?
>>
>> This used to be u-boot,dm-spl and was later replaced with bootph-pre-ram.
>>
>
> Right... but bootph-pre-ram means that the node is available in SPL
> right ? But it will not be available 'post-ram' ?
I was under the assumption that a node would be available at any later
stage, re-reading the dt-schema and checking Makefile I can now see that
nodes will only be available in:
TPL: nodes having bootph-all or bootph-pre-sram
SPL: nodes having bootph-all or bootph-pre-ram
U-Boot proper: all nodes regardless of any bootph prop
>
>> For fspim2_pins I used the same bootph as the other pins. Not sure why
>> the pins use bootph-all to begin with, sdmmc and sdhci use bootph-pre-ram
>> in rk3588s-u-boot.dtsi.
>>
>
> Because out of my understanding, the pins should be available in all
> bootstages, hence 'bootph-all' , they are needed all the time.
Fully agree and will have to change a few to bootph-all for RK3568 in a
v2 series.
>
> Please correct me if I am wrong, I am not very familiar with these new
> names yet.
>
> But 'pre-ram' suggests (at least to me) availability only in the initial
> stage (SPL)
>
> Otherwise what's the difference between bootph-all and bootph-pre-ram ?
> Maybe that difference would enlighten me
After reading up on the bootph props I think the following could be good
use for the Rockchip platform, where TPL main purpose is to init DRAM and
SPL main purpose is to load U-Boot proper/TF-A from storage:
- bootph-all for e.g. clock, ram, pinctrl and debug uart related nodes.
- bootph-pre-ram for any remaining node related to where
U-Boot proper/TF-A could be loaded from.
>
> Sorry for the long noise
>
>
>>>
>>>>> + u-boot,spl-sfc-no-dma;
>>>>> + pinctrl-names = "default";
>>>>> + pinctrl-0 = <&fspim2_pins>;
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <0>;
>>>>> + status = "okay";
>>>>> +
>>>>> + flash@0 {
>>>>> + bootph-pre-ram;
>>>>> + compatible = "jedec,spi-nor";
>>>>> + reg = <0>;
>>>>> + spi-max-frequency = <24000000>;
>>>>> + spi-rx-bus-width = <4>;
>>>>> + spi-tx-bus-width = <1>;
>>>>> + };
>>>>> +};
>>>>> +
>>>>> &pinctrl {
>>>>> bootph-all;
>>>>> @@ -69,6 +89,10 @@
>>>>> bootph-all;
>>>>> };
>>>>> +&fspim2_pins {
>>>>> + bootph-all;
>>>>> +};
>>>>> +
>>>>> &sdmmc_bus4 {
>>>>> bootph-all;
>>>>> };
>>>>> diff --git a/arch/arm/dts/rk3588s-u-boot.dtsi
>>>>> b/arch/arm/dts/rk3588s-u-boot.dtsi
>>>>> index cd7e6cb50ee2..d8a471a37fd1 100644
>>>>> --- a/arch/arm/dts/rk3588s-u-boot.dtsi
>>>>> +++ b/arch/arm/dts/rk3588s-u-boot.dtsi
>>>>> @@ -104,6 +104,15 @@
>>>>> };
>>>>> };
>>>>> + sfc: spi@fe2b0000 {
>>>>> + compatible = "rockchip,sfc";
>>>>> + reg = <0x0 0xfe2b0000 0x0 0x4000>;
>>>>> + interrupts = <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH 0>;
>>>>> + clocks = <&cru SCLK_SFC>, <&cru HCLK_SFC>;
>>>>> + clock-names = "clk_sfc", "hclk_sfc";
>>>>> + status = "disabled";
>>>>> + };
>>>>> +
>>>>> otp: nvmem@fecc0000 {
>>>>> compatible = "rockchip,rk3588-otp";
>>>>> reg = <0x0 0xfecc0000 0x0 0x400>;
>>>>> @@ -164,3 +173,14 @@
>>>>> &ioc {
>>>>> bootph-pre-ram;
>>>>> };
>>>>> +
>>>>> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
>>>>> +&binman {
>>>>> + simple-bin-spi {
>>>>> + mkimage {
>>>>> + args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
>>>>> + offset = <0x8000>;
>>>
>>> What is this offset referring to ?
>>
>> This offset is referring to the normal mmc 32 KiB offset that idbloader
>> is normally written to. I used the offset prop so that the
>> u-boot-rockchip-spi.bin can be written to offset 0 of spi similar as is
>> currently done for rk3399.
>
> Oh. I get it. thanks !
> So it's the offset of 64 sectors that the rockchip tool needs.
>
> I tried to write the u-boot-rockchip-spi.bin at offset 64 sectors, and
> it boots into the SPL, but cannot read the FIT.
> I tried to write it at offset 0, and then it boots correctly.
> Interesting behavior.
Possible other offsets then 64 sectors could be used, having idbloader
at sector 0 did not seem to work on RK3568 but matching the mmc offset
seemed to get my rk3568 device booting :-)
I think bootrom look for the idbloader header at multiple different
offsets. So with u-boot-rockchip-spi.bin written at 64 sector offset
bootrom will find and load TPL+SPL but SPL will not find FIT at the
configured offset, CONFIG_SYS_SPI_U_BOOT_OFFS.
>
> Do you have to specify here the offset for the FIT, in binman ?
> Or is it taken from CONFIG_SYS_SPI_U_BOOT_OFFS automatically ?
Correct, binman fit/u-boot-img node is using CONFIG_SYS_SPI_U_BOOT_OFFS
for the offset in u-boot-rockchip-spi.bin.
arch/arm/dts/rockchip-u-boot.dtsi:
offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
>>
>>>
>>>>> + };
>>>>> + };
>>>>> +};
>>>>> +#endif
>>>>> diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c
>>>>> b/arch/arm/mach-rockchip/rk3588/rk3588.c
>>>>> index 18e67b5ca9b2..0e85893e0096 100644
>>>>> --- a/arch/arm/mach-rockchip/rk3588/rk3588.c
>>>>> +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c
>>>>> @@ -41,6 +41,7 @@ const char * const boot_devices[BROM_LAST_BOOTSOURCE
>>>>> + 1] = {
>>>>> [BROM_BOOTSOURCE_EMMC] = "/mmc@fe2e0000",
>>>>> [BROM_BOOTSOURCE_SPINOR] = "/spi@fe2b0000/flash@0",
>>>>> [BROM_BOOTSOURCE_SD] = "/mmc@fe2c0000",
>>>>> + [6] = "/spi@fe2b0000/flash@0",
>>>
>>> Is this '6' meaning something in particular ? or just the next number in
>>> line ?
>>
>> The bootrom on rk3588 use value 6 when booting from the spi flash on my
>> ROCK 5B, normally bootrom have used value 3 (BROM_BOOTSOURCE_SPINOR) on
>> rk356x and earlier socs.
>>
>> I have no idea what we should call this BOOTSOURCE, could not find any
>> define in vendor u-boot to help give this a proper name.
>
> Either this is a BOOTSOURCE_SPINOR_RK3588 specific, or the RK3588 calls
> some other device SPINOR, and this one (the sfc) something else.
> I remember seeing some other flash controllers in the dtsi of the SoC,
> maybe those are also bootable ?
True, could be that there are some more bootable devices on RK3588,
will add and use BROM_BOOTSOURCE_SPINOR_RK3588 for v2.
>
>>
>> Kever: Any insights into what this value should be called?
>>
>>>
>>>>> };
>>>>> static struct mm_region rk3588_mem_map[] = {
>>>>> diff --git a/configs/rock5b-rk3588_defconfig
>>>>> b/configs/rock5b-rk3588_defconfig
>>>>> index 2f0a74ee5559..e6a903853fb7 100644
>>>>> --- a/configs/rock5b-rk3588_defconfig
>>>>> +++ b/configs/rock5b-rk3588_defconfig
>>>>> @@ -8,15 +8,20 @@ CONFIG_SPL_LIBGENERIC_SUPPORT=y
>>>>> CONFIG_NR_DRAM_BANKS=2
>>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0xc00000
>>>>> +CONFIG_SF_DEFAULT_SPEED=24000000
>>>>> +CONFIG_SF_DEFAULT_MODE=0x2000
>>>
>>> Any reason for changing the default mode ?
>>
>> Default speed and mode is changed to match the speed and mode used in
>> the device tree. SPL use these Kconfig instead of reading the speed and
>> mode from fdt blob.
>
> SPL should have its own blob, is the read from fdt not yet implemented ?
I had an issue on RK3568 where clk driver need an exact clock rate to
configure fspi clock. Noticed that fspi was probed with Kconfig values
instead of the values used in device tree. With default speed configured
booting from spi flash started working on RK3568. Setting the default
mode that would be used by U-Boot proper also seem appropriate.
Did same for RK3588 even if it may not strictly be needed.
common/spl/spl_spi.c:
spi_flash_probe(sf_bus, sf_cs, CONFIG_SF_DEFAULT_SPEED,
CONFIG_SF_DEFAULT_MODE);
>
>>
>>>
>>>>> CONFIG_DEFAULT_DEVICE_TREE="rk3588-rock-5b"
>>>>> CONFIG_ROCKCHIP_RK3588=y
>>>>> CONFIG_SPL_ROCKCHIP_COMMON_BOARD=y
>>>>> +CONFIG_ROCKCHIP_SPI_IMAGE=y
>>>>> CONFIG_SPL_SERIAL=y
>>>>> CONFIG_SPL_STACK_R_ADDR=0x600000
>>>>> CONFIG_TARGET_ROCK5B_RK3588=y
>>>>> CONFIG_SPL_STACK=0x400000
>>>>> CONFIG_DEBUG_UART_BASE=0xFEB50000
>>>>> CONFIG_DEBUG_UART_CLOCK=24000000
>>>>> +CONFIG_SPL_SPI_FLASH_SUPPORT=y
>>>>> +CONFIG_SPL_SPI=y
>>>>> CONFIG_SYS_LOAD_ADDR=0xc00800
>>>>> CONFIG_DEBUG_UART=y
>>>>> CONFIG_FIT=y
>>>>> @@ -35,6 +40,8 @@ CONFIG_SPL_BSS_MAX_SIZE=0x4000
>>>>> # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
>>>>> # CONFIG_SPL_SHARES_INIT_SP_ADDR is not set
>>>>> CONFIG_SPL_STACK_R=y
>>>>> +CONFIG_SPL_SPI_LOAD=y
>>>>> +CONFIG_SYS_SPI_U_BOOT_OFFS=0x60000
>>>
>>> I have a feeling the default is 0x80000, do you have any reason for the
>>> change to 0x60000 ?
>>
>> The default is probably set for rk3399 that requires double space for
>> idbloader because bootrom only read first 2 KiB of every 4 KiB page.
>> To not waste usable space I used the same offset that was used by
>> multiple rk3399 configs:
>>
>> u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */
>>
>> SPI flash layout would look something like:
>>
>> idbloader: @ 32 KiB
>> - TPL: current ddr init blob use around 54 KiB
>> - SPL: max 256 KiB
>> FIT payload: @ 384 KiB
>>
>> Should mean we have at least 96 KiB for TPL.
>
> Looks good.
> I am only interested if this is different than from other devices, such
> that we keep a similar layout, and not diverge.
>From what I could tell this is matching at least some rk3399 devices,
it is also matching a few rk356x devices I have prepared in my tree.
Regards,
Jonas
>
>>
>> Regards,
>> Jonas
>>
>>>
>>>>> CONFIG_SPL_ATF=y
>>>>> CONFIG_CMD_GPIO=y
>>>>> CONFIG_CMD_GPT=y
>>>>> @@ -59,6 +66,8 @@ CONFIG_MMC_SDHCI=y
>>>>> CONFIG_MMC_SDHCI_SDMA=y
>>>>> # CONFIG_SPL_MMC_SDHCI_SDMA is not set
>>>>> CONFIG_MMC_SDHCI_ROCKCHIP=y
>>>>> +CONFIG_SPI_FLASH_MACRONIX=y
>>>>> +CONFIG_SPI_FLASH_XTX=y
>>>>> CONFIG_ETH_DESIGNWARE=y
>>>>> CONFIG_GMAC_ROCKCHIP=y
>>>>> CONFIG_PHY_ROCKCHIP_INNO_USB2=y
>>>>> @@ -69,6 +78,7 @@ CONFIG_SPL_RAM=y
>>>>> CONFIG_BAUDRATE=1500000
>>>>> CONFIG_DEBUG_UART_SHIFT=2
>>>>> CONFIG_SYS_NS16550_MEM32=y
>>>>> +CONFIG_ROCKCHIP_SFC=y
>>>>> CONFIG_SYSRESET=y
>>>>> CONFIG_USB=y
>>>>> CONFIG_USB_EHCI_HCD=y
>>>
>>
>
