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 11:22 PM, FUKAUMI Naoki wrote:
> Hi,
> 
> On 10/30/24 19:39, Quentin Schulz wrote:
>> 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).
> 
> maybe it's time to update this patch.
> 
> https://eur02.safelinks.protection.outlook.com/? 
> url=https%3A%2F%2Fpatchwork.kernel.org%2Fproject%2Flinux- 
> rockchip%2Fpatch%2F20230913064505.77393-2- 
> naoki%40radxa.com%2F&data=05%7C02%7Cquentin.schulz%40cherry.de%7Cf853dffb859d43c4309f08dcf9315af8%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638659237600855259%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=S1k4mVpoTXIf2S4npvbQCoUWTgNWdWae2ELb7PvEwQI%3D&reserved=0
> 
> rock-5a-base.dtsi (common part)
> rock-5a.dts (same as current one)
> rock-5a-spi-dts
> 

Nice, we'll need a cherry-pick for those whenever they land in 
https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git 
(doesn't seem to be the case right now though...).

THen a -u-boot.dtsi per variant (rock-5a.dts and rock-5a-spi.dts).

>>>> 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?
> 
> I'm thinking doing same for ROCK 5C...
> 
> https://eur02.safelinks.protection.outlook.com/? 
> url=https%3A%2F%2Fgithub.com%2FRadxaNaoki%2Fu- 
> boot%2Fcommits%2Frock-5c%2F&data=05%7C02%7Cquentin.schulz%40cherry.de%7Cf853dffb859d43c4309f08dcf9315af8%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638659237600880322%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=R7VN8bJSvmKT14QQDYFbGmymxh5vsxzKLE3lEIwFCOw%3D&reserved=0
> 
>> Keeping all configs in sync for essentially same board with small 
>> differences may be difficult, maybe using config fragments will help?
> 
> any "config fragments" doc/example?
> 

https://docs.u-boot.org/en/latest/board/phytec/phycore-am64x.html

3.1.1

for how to use it.

I don't know how would one generate it. But if you figure it out, it's 
probably worth adding to the docs ;)

Cheers,
Quentin