[PATCH v3 5/5] rockchip: Add support for Radxa ROCK 5C

[Quentin Schulz]

Hi Naoki,

On 12/15/24 2:37 AM, FUKAUMI Naoki wrote:
> Hi Quentin,
> 
> Thank you very much for your review!
> 
> On 12/13/24 22:11, Quentin Schulz wrote:
>> Hi Naoki,
>>
>> On 12/11/24 4:39 AM, FUKAUMI Naoki wrote:
>>> Radxa ROCK 5C[1] is a Rockchip RK3588S2 based single board computer.
>>>
>>> [1] https://eur02.safelinks.protection.outlook.com/? 
>>> url=https%3A%2F%2Fradxa.com%2Fproducts%2Frock5%2F5c&data=05%7C02%7Cquentin.schulz%40cherry.de%7Cbe7d810ae88f4203af8208dd1ca921c6%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638698234938145886%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=8TcHBC2dV1LzouPSR8%2FICOBVshq9RndgMQS3m3XLwFs%3D&reserved=0
>>>
>>> Signed-off-by: FUKAUMI Naoki <naoki at radxa.com>
>>> ---
>>> Changes in v3:
>>> - fix compile error
>>> Changes in v2:
>>> - arch/arm/dts/rk3588s-rock-5-u-boot.dtsi: remove unused node
>>> - include/configs/rock-5-rk3588s.h: fix include order
>>> ---
>>>   arch/arm/dts/rk3588s-rock-5-u-boot.dtsi     | 16 ++++
>>>   arch/arm/dts/rk3588s-rock-5c-u-boot.dtsi    |  6 ++
>>>   arch/arm/mach-rockchip/rk3588/Kconfig       |  7 ++
>>>   board/radxa/rock-5-rk3588s/Kconfig          | 12 +++
>>>   board/radxa/rock-5-rk3588s/MAINTAINERS      |  8 ++
>>>   board/radxa/rock-5-rk3588s/Makefile         |  3 +
>>>   board/radxa/rock-5-rk3588s/rock-5-rk3588s.c | 77 +++++++++++++++++
>>>   configs/rock-5-rk3588s_defconfig            | 94 +++++++++++++++++++++
>>>   doc/board/rockchip/rockchip.rst             |  1 +
>>>   include/configs/rock-5-rk3588s.h            | 15 ++++
>>>   10 files changed, 239 insertions(+)
>>>   create mode 100644 arch/arm/dts/rk3588s-rock-5-u-boot.dtsi
>>>   create mode 100644 arch/arm/dts/rk3588s-rock-5c-u-boot.dtsi
>>>   create mode 100644 board/radxa/rock-5-rk3588s/Kconfig
>>>   create mode 100644 board/radxa/rock-5-rk3588s/MAINTAINERS
>>>   create mode 100644 board/radxa/rock-5-rk3588s/Makefile
>>>   create mode 100644 board/radxa/rock-5-rk3588s/rock-5-rk3588s.c
>>>   create mode 100644 configs/rock-5-rk3588s_defconfig
>>>   create mode 100644 include/configs/rock-5-rk3588s.h
>>>
>>> diff --git a/arch/arm/dts/rk3588s-rock-5-u-boot.dtsi b/arch/arm/dts/ 
>>> rk3588s-rock-5-u-boot.dtsi
>>> new file mode 100644
>>> index 000000000000..be1a2f9ae7bb
>>> --- /dev/null
>>> +++ b/arch/arm/dts/rk3588s-rock-5-u-boot.dtsi
>>> @@ -0,0 +1,16 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>> +/*
>>> + * Copyright (c) 2024 Radxa Computer (Shenzhen) Co., Ltd.
>>> + */
>>> +
>>> +#include "rk3588s-u-boot.dtsi"
>>> +
>>> +&saradc {
>>> +    bootph-pre-ram;
>>> +    vdd-microvolts = <1800000>;
>>> +};
>>> +
>>> +&sdhci {
>>> +    cap-mmc-highspeed;
>>> +    mmc-hs200-1_8v;
>>> +};
>>> diff --git a/arch/arm/dts/rk3588s-rock-5c-u-boot.dtsi b/arch/arm/dts/ 
>>> rk3588s-rock-5c-u-boot.dtsi
>>> new file mode 100644
>>> index 000000000000..bb1cc9e4a279
>>> --- /dev/null
>>> +++ b/arch/arm/dts/rk3588s-rock-5c-u-boot.dtsi
>>> @@ -0,0 +1,6 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>> +/*
>>> + * Copyright (c) 2024 Radxa Computer (Shenzhen) Co., Ltd.
>>> + */
>>> +
>>> +#include "rk3588s-rock-5-u-boot.dtsi"
>>> diff --git a/arch/arm/mach-rockchip/rk3588/Kconfig b/arch/arm/mach- 
>>> rockchip/rk3588/Kconfig
>>> index b5a0e624a532..e1487ecb0464 100644
>>> --- a/arch/arm/mach-rockchip/rk3588/Kconfig
>>> +++ b/arch/arm/mach-rockchip/rk3588/Kconfig
>>> @@ -260,6 +260,12 @@ config TARGET_ROCK_5_ITX_RK3588
>>>         Front-panel connectors for audio and case-power, -leds
>>>         Powered by either 12V, ATX power-supply or PoE
>>> +config TARGET_ROCK_5_RK3588S
>>
>> This should be TARGET_ROCK_5C_RK3588S?
> 
> rock-pi-4-rk3399_defconfig uses CONFIG_TARGET_ROCKPI4_RK3399.
> I'm doing same thing.
> 

config TARGET_ROCKPI4_RK3399
    help
      Support for ROCK Pi 4 board family by Radxa.

Here you say this option is for ROCK 5C only.

While this could be a good idea, anything odd needs to be justified in 
the commit log so that we can follow your train of thoughts.

This patch series only adds support for the ROCK 5C so nothing hints at 
this being usable/used for other boards from Radxa. Which boards are you 
planning to support with this option? If the ROCK 5B/ROCK 5 ITX aren't 
going to be part of it, why and how do I know which of the ROCK 5 is 
supported by this option?

This could also be started with a "normal" config option which is named 
after the board and for the next board support you want to reuse this 
config option for, you rename it to something more appropriate. In any 
case, please document the reasons for doing that :)

Cheers,
Quentin