[PATCH] board: rockchip: Add FriendlyElec NanoPi R6C

Fri Jun 7 10:23:09 CEST 2024

Hi Sebastian,

On 6/6/24 6:25 PM, Sebastian Kropatsch wrote:
> [You don't often get email from seb-dev at mail.de. Learn why this is 
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hi Quentin,
> 
> thanks for your feedback!
> 
> Am 06.06.2024 um 16:53 schrieb Quentin Schulz:
>> Hi Sebastian,
>>
>> On 6/5/24 5:36 PM, Sebastian Kropatsch wrote:
>>> [You don't often get email from seb-dev at mail.de. Learn why this is
>>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> The NanoPi R6C is a SBC by FriendlyElec based on the Rockchip RK3588s.
>>> It comes with 4GB or 8GB of RAM, a microSD card slot, optional 32GB eMMC
>>> storage, one M.2 M-Key connector, one RTL8211F 1GbE and one RTL8125
>>> 2.5GbE Ethernet port, one USB 2.0 Type-A and one USB 3.0 Type-A port, a
>>> HDMI port, a 30-pin GPIO header as well as multiple buttons and LEDs.
>>>
>>> Add initial support for this board using the upstream devicetree 
>>> sources.
>>>
>>> Tests in U-Boot proper:
>>> - Booting from eMMC works
>>> - 1GbE Ethernet works using the eth_eqos driver (tested by ping)
>>> - 2.5GbE Ethernet works using the eth_rtl8169 driver (tested by ping),
>>>    but the status LEDs on this specific port currently aren't working
>>> - NVMe SSD in M.2 socket does get recognized (tested with `nvme scan`
>>>    followed by `nvme details`)
>>>
>>> Kernel commit:
>>> d5f1d7437451 ("arm64: dts: rockchip: Add support for NanoPi R6C")
>>>
>>> Signed-off-by: Sebastian Kropatsch <seb-dev at mail.de>
>>> ---
>>>
>>> Hello!
>>>
>>> The Ethernet status LEDs which sit directly on the 2.5GbE port using the
>>> RTL8169 driver don't light up when connected and I couldn't figure out
>>> why. The other port with a RTL8211F has no problems with the LEDs.
>>> Have there been occurrences like this in combination with the RTL8125?
>>> I'm trying to figure out if this is something that could be solved in
>>> the devicetree or if this is a potential driver bug.
>>>
>>> Secondly, the default active network device in U-Boot is the 1GbE one.
>>> I believe it would make sense to make the 2.5GbE device the default
>>> active one since this one is labeled "LAN", whereas the 1GbE is labeled
>>> "WAN". However, since the 2.5GbE device is PCIe-based, it only shows up
>>> in U-Boot proper after using the `pci enum` command (shows up as in gets
>>> listed in `net list` and `dm tree`).
>>> Do you have any tips on the preferred approach to handle this switch of
>>> the default active net device? Is this even a sensible thing to include
>>> in U-Boot in your opinion?
>>>
>>> Thanks for your feedback!
>>>
>>> Best regards,
>>> Sebastian
>>>
>>> ---
>>>   arch/arm/dts/rk3588s-nanopi-r6c-u-boot.dtsi   |  3 +
>>>   arch/arm/mach-rockchip/rk3588/Kconfig         | 13 +++
>>>   board/friendlyelec/nanopi-r6c-rk3588s/Kconfig | 12 +++
>>>   .../nanopi-r6c-rk3588s/MAINTAINERS            |  7 ++
>>>   configs/nanopi-r6c-rk3588s_defconfig          | 83 +++++++++++++++++++
>>>   doc/board/rockchip/rockchip.rst               |  1 +
>>>   include/configs/nanopi-r6c-rk3588s.h          | 12 +++
>>>   7 files changed, 131 insertions(+)
>>>   create mode 100644 arch/arm/dts/rk3588s-nanopi-r6c-u-boot.dtsi
>>>   create mode 100644 board/friendlyelec/nanopi-r6c-rk3588s/Kconfig
>>>   create mode 100644 board/friendlyelec/nanopi-r6c-rk3588s/MAINTAINERS
>>>   create mode 100644 configs/nanopi-r6c-rk3588s_defconfig
>>>   create mode 100644 include/configs/nanopi-r6c-rk3588s.h
>>>
>>> diff --git a/arch/arm/dts/rk3588s-nanopi-r6c-u-boot.dtsi
>>> b/arch/arm/dts/rk3588s-nanopi-r6c-u-boot.dtsi
>>> new file mode 100644
>>> index 0000000000..853ed58cfe
>>> --- /dev/null
>>> +++ b/arch/arm/dts/rk3588s-nanopi-r6c-u-boot.dtsi
>>> @@ -0,0 +1,3 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>> +
>>> +#include "rk3588s-u-boot.dtsi"
>>> diff --git a/arch/arm/mach-rockchip/rk3588/Kconfig
>>> b/arch/arm/mach-rockchip/rk3588/Kconfig
>>> index 820e979abb..0e232ddfb9 100644
>>> --- a/arch/arm/mach-rockchip/rk3588/Kconfig
>>> +++ b/arch/arm/mach-rockchip/rk3588/Kconfig
>>> @@ -78,6 +78,18 @@ config TARGET_NANOPCT6_RK3588
>>>            Power: 5.5*2.1mm DC Jack, 12VDC input
>>>            Dimensions: 110x80x1.6mm (without case) / 86x114.5x30mm
>>> (with case)
>>>
>>> +config TARGET_NANOPI_R6C_RK3588
>>
>> Please add the S at the end of the symbol.
> 
> u-boot-next currently has two other RK3588s boards, which are Rock5A
> and Indiedroid Nova. Both do not have the S at the end of the symbol:
> 
>         config TARGET_ROCK5A_RK3588
>         [...]
>         config TARGET_NOVA_RK3588
> 
> Which is why I haven't included the S. Should we update those as well?
> I suppose in this case I should also update '/include/configs/nanopi-
> r6c-rk3588s.h' which currently reads:
> 

Ah well, this went through the review process :) It's not a big deal 
anyway, so we can do a search and replace later if necessary.

>> diff --git a/include/configs/nanopi-r6c-rk3588s.h 
>> b/include/configs/nanopi-r6c-rk3588s.h
>> new file mode 100644
>> index 0000000000..e05856f3ce
>> --- /dev/null
>> +++ b/include/configs/nanopi-r6c-rk3588s.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +
>> +#ifndef __NANOPI_R6C_RK3588_H
>> +#define __NANOPI_R6C_RK3588_H
>> +
>> +#define ROCKCHIP_DEVICE_SETTINGS \
>> +             "stdout=serial,vidconsole\0" \
>> +             "stderr=serial,vidconsole\0"
>> +
>> +#include <configs/rk3588_common.h>
>> +
>> +#endif /* __NANOPI_R6C_RK3588_H */
> 
> (end '/include/configs/nanopi-r6c-rk3588s.h')
> If this should not be updated to add an S, let me know. I'll send a v2
> when I know :)
> 

Nah, this is a local define, don't care too much if there's a typo in 
there as the only purpose is that no other file actually define it :)

>>
>> Otherwise looking good to me. I'm wondering if we shouldn't do something
>> for boards which do not need a -u-boot.dtsi. Here it's needed because
>> U-Boot would include rk3588-u-boot.dtsi automatically (because of
>> CONFIG_SYS_SOC being rk3588) instead of rk3588s-u-boot.dtsi. Maybe it's
>> too much headache for the benefit :)
>>
>> Cheers,
>> Quentin
> 
> Ah yes, I was wondering if my mostly empty rk3588s-nanopi-r6c-
> u-boot.dtsi was even needed, since I don't know much about the inner
> workings of U-Boot. I believe it would be nice to not need this file,
> but I have no idea how complex and maintainable an implementation would
> be. If it's complex I'd say no, since having those *u-boot.dtsi files
> don't hurt that much in my opinion :)
> 

This whole rk3588 is an rk3588s++ but actually we use "rk3588" for 
everything but actually no, the Device Tree base is "rk3588s" is a bit 
of a pain to deal with in different places :) Probably a similar issue 
with the RK3566 vs RK3568 I guess. And it's probably not going to be 
much more fun with the RK3582 which is an RK3588s--.

A few options right now tht comes to mind:
- add a new CONFIG_SYS_SOC for rk3588s and add symlinks wherever 
necessary to point to rk3588 implementations
- add a new config symbol for use here: 
https://elixir.bootlin.com/u-boot/latest/source/scripts/Makefile.lib#L162
- make U-Boot try recursively a right dash-stripped verison of the dts, 
e.g.: rk3588s-nanopi-r6c would also try rk3588s-nanopi-u-boot.dtsi and 
rk3588s-u-boot.dtsi

I think we're good anyway, so

Reviewed-by: Quentin Schulz <quentin.schulz at cherry.de>

Thanks!
Quentin