Re: [PATCH v2 2/2] rockchip: add support for Radxa ROCK 5A with SPI NOR flash module

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Naoki,

On 10/30/24 4:09 AM, FUKAUMI Naoki wrote:
> Hi,
> 
> could you review this patch, anyone?
> 
> Best regards,
> 
> -- 
> FUKAUMI Naoki
> Radxa Computer (Shenzhen) Co., Ltd.
> 
> On 8/25/24 07:33, FUKAUMI Naoki wrote:
>> on Radxa ROCK 5A, sdhci(eMMC) and fspim0(SPI NOR flash) share pins
>> (i.e. eMMC and SPI NOR flash are exclusive), new defconfig and dts
>> specifically for SPI NOR flash is required.
>>
>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>> ---
>> Changes in v2
>> - fix subject
>> ---
>>   arch/arm/dts/rk3588s-rock-5a-spi-u-boot.dtsi | 24 ++++++
>>   arch/arm/dts/rk3588s-rock-5a-spi.dts         |  4 +
>>   board/radxa/rock5a-rk3588s/MAINTAINERS       |  5 +-
>>   configs/rock5a-spi-rk3588s_defconfig         | 83 ++++++++++++++++++++
>>   4 files changed, 113 insertions(+), 3 deletions(-)
>>   create mode 100644 arch/arm/dts/rk3588s-rock-5a-spi-u-boot.dtsi
>>   create mode 100644 arch/arm/dts/rk3588s-rock-5a-spi.dts
>>   create mode 100644 configs/rock5a-spi-rk3588s_defconfig
>>
>> diff --git a/arch/arm/dts/rk3588s-rock-5a-spi-u-boot.dtsi b/arch/arm/ 
>> dts/rk3588s-rock-5a-spi-u-boot.dtsi
>> new file mode 100644
>> index 00000000000..5cd131d3cb1
>> --- /dev/null
>> +++ b/arch/arm/dts/rk3588s-rock-5a-spi-u-boot.dtsi
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (c) 2023 Collabora Ltd.
>> + */
>> +
>> +#include "rk3588s-u-boot.dtsi"
>> +
>> +&fspim0_pins {
>> +    bootph-pre-ram;
>> +    bootph-some-ram;
>> +};
>> +
>> +&sdhci {
>> +    status = "disabled";
>> +};
>> +
>> +&sfc {
>> +    status = "okay";
>> +
>> +    flash@0 {
>> +        bootph-pre-ram;
>> +        bootph-some-ram;
>> +    };
>> +};

I assume the board can only be fitted with an SPI NOR or an eMMC and not 
both at the same time? If that's the case, then the status = disabled 
and status = okay should be in the dts. I assume we want this to be in 
the Linux kernel first too, either as a separate DTS or with a DTSO (not 
sure what they will want).

>> diff --git a/arch/arm/dts/rk3588s-rock-5a-spi.dts b/arch/arm/dts/ 
>> rk3588s-rock-5a-spi.dts
>> new file mode 100644
>> index 00000000000..780e90d041b
>> --- /dev/null
>> +++ b/arch/arm/dts/rk3588s-rock-5a-spi.dts
>> @@ -0,0 +1,4 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +
>> +/dts-v1/;
>> +#include "rk3588s-rock-5a.dts"
>> diff --git a/board/radxa/rock5a-rk3588s/MAINTAINERS b/board/radxa/ 
>> rock5a-rk3588s/MAINTAINERS
>> index a569efa74e3..06ebc9829f4 100644
>> --- a/board/radxa/rock5a-rk3588s/MAINTAINERS
>> +++ b/board/radxa/rock5a-rk3588s/MAINTAINERS
>> @@ -4,6 +4,5 @@ R:    Jonas Karlman <jonas@kwiboo.se>
>>   S:    Maintained
>>   F:    board/radxa/rock5a-rk3588s
>>   F:    include/configs/rock5a-rk3588s.h
>> -F:    configs/rock5a-rk3588s_defconfig
>> -F:    arch/arm/dts/rk3588s-rock-5a.dts
>> -F:    arch/arm/dts/rk3588s-rock-5a-u-boot.dtsi
>> +F:    configs/rock5a*
>> +F:    arch/arm/dts/rk3588s-rock-5a*
>> diff --git a/configs/rock5a-spi-rk3588s_defconfig b/configs/rock5a- 
>> spi-rk3588s_defconfig
>> new file mode 100644
>> index 00000000000..297278c7a06
>> --- /dev/null
>> +++ b/configs/rock5a-spi-rk3588s_defconfig

If Radxa starts having many such options, maybe it won't make a lot of 
sense to duplicate configs but rather have config fragments to change 
the default DT and add a few symbols that differ from the base (I assume 
we may have something similar needed for rock 5b+ compared to rock5b for 
example?

Keeping all configs in sync for essentially same board with small 
differences may be difficult, maybe using config fragments will help?

>> @@ -0,0 +1,83 @@
>> +CONFIG_ARM=y
>> +CONFIG_SKIP_LOWLEVEL_INIT=y
>> +CONFIG_COUNTER_FREQUENCY=24000000
>> +CONFIG_ARCH_ROCKCHIP=y
>> +CONFIG_SF_DEFAULT_SPEED=24000000
>> +CONFIG_SF_DEFAULT_MODE=0x2000
>> +CONFIG_DEFAULT_DEVICE_TREE="rk3588s-rock-5a-spi"
>> +CONFIG_ROCKCHIP_RK3588=y
>> +CONFIG_ROCKCHIP_SPI_IMAGE=y
>> +CONFIG_SPL_SERIAL=y
>> +CONFIG_TARGET_ROCK5A_RK3588=y
>> +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
>> +CONFIG_FIT_VERBOSE=y
>> +CONFIG_SPL_FIT_SIGNATURE=y
>> +CONFIG_SPL_LOAD_FIT=y
>> +CONFIG_LEGACY_IMAGE_FORMAT=y
>> +CONFIG_DEFAULT_FDT_FILE="rockchip/rk3588s-rock-5a.dtb"
>> +# CONFIG_DISPLAY_CPUINFO is not set
>> +CONFIG_DISPLAY_BOARDINFO_LATE=y
>> +CONFIG_SPL_MAX_SIZE=0x40000
>> +CONFIG_SPL_PAD_TO=0x7f8000
>> +# CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
>> +CONFIG_SPL_SPI_LOAD=y
>> +CONFIG_SYS_SPI_U_BOOT_OFFS=0x60000
>> +CONFIG_SPL_ATF=y
>> +CONFIG_CMD_GPIO=y
>> +CONFIG_CMD_GPT=y
>> +CONFIG_CMD_I2C=y
>> +CONFIG_CMD_MMC=y
>> +CONFIG_CMD_USB=y
>> +# CONFIG_CMD_SETEXPR is not set
>> +CONFIG_CMD_REGULATOR=y
>> +# CONFIG_SPL_DOS_PARTITION is not set
>> +CONFIG_SPL_OF_CONTROL=y
>> +CONFIG_OF_LIVE=y
>> +# CONFIG_OF_UPSTREAM is not set
>> +CONFIG_OF_SPL_REMOVE_PROPS="clock-names interrupt-parent assigned- 
>> clocks assigned-clock-rates assigned-clock-parents"
>> +CONFIG_SPL_DM_SEQ_ALIAS=y
>> +CONFIG_SPL_REGMAP=y
>> +CONFIG_SPL_SYSCON=y
>> +CONFIG_SPL_CLK=y
>> +CONFIG_ROCKCHIP_GPIO=y
>> +CONFIG_SYS_I2C_ROCKCHIP=y
>> +CONFIG_MISC=y
>> +CONFIG_SUPPORT_EMMC_RPMB=y
>> +CONFIG_MMC_DW=y
>> +CONFIG_MMC_DW_ROCKCHIP=y
>> +CONFIG_MMC_SDHCI=y
>> +CONFIG_MMC_SDHCI_SDMA=y
>> +CONFIG_MMC_SDHCI_ROCKCHIP=y

I assume out of the MMC_DW_ROCKCHIP and MMC_SDHCI_ROCKCHIP drivers, one 
isn't needed anymore as it's the one for the eMMC and the other for the 
SD card?

Cheers,
Quentin