Re: [PATCH 1/1] board: rockchip: Add support for rk3588 GenBook

[Dragan Simic <dsimic@manjaro.org>]

On 2024-08-12 02:37, Andy Yan wrote:
> At 2024-08-12 05:26:36, "Dragan Simic" <dsimic@manjaro.org> wrote:
>> Hello Andy,
>> 
>> Please see a few comments below.
>> 
>> On 2024-08-11 12:02, Andy Yan wrote:
>>> Add support for Cool Pi GenBook.
>>> 
>>> Specification:
>>> - Rockchip RK3588
>>> - LPDDR5X 8/32 GB
>>> - eMMC 64 GB
>>> - HDMI Type A out x 1
>>> - USB 3.0 Host x 1
>>> - USB-C 3.0 with DisplayPort AltMode
>>> - PCIE M.2 E Key for RTL8852BE Wireless connection
>>> - PCIE M.2 M Key for NVME connection
>>> - eDP panel with 1920x1080
>> 
>> It should be mentioned that it actually uses CM5 SoM.
> 
> Okay，will do in v2

Great, thanks.

[snip]

>>> diff --git a/arch/arm/mach-rockchip/rk3588/Kconfig
>>> b/arch/arm/mach-rockchip/rk3588/Kconfig
>>> index e751d64e1a1..dc53d559ed9 100644
>>> --- a/arch/arm/mach-rockchip/rk3588/Kconfig
>>> +++ b/arch/arm/mach-rockchip/rk3588/Kconfig
>>> @@ -6,6 +6,12 @@ config TARGET_EVB_RK3588
>>>  	help
>>>  	  RK3588 EVB is a evaluation board for Rockchp RK3588.
>>> 
>>> +config TARGET_GENBOOK_RK3588
>> 
>> It should be better to use "TARGET_GENBOOK_CM5_RK3588" instead,
>> to leave room for support for any possible future SoMs that might
>> be made available for the GenBook at some point.
> 
> Considering the name of dts is rk3588-coolpi-cm5-genbook.dts , should
> we  use TARGET_CM5_GENBOOK_RK3588  instead of
> TARGET_GENBOOK_CM5_RK3588 here ?

That's a good question, and I already asked myself the same, before
sending over my comments.  It's all about the way U-Boot names the
things internally, i.e. we've got it like "<carrier_board>-<SoM>-<SoC>"
this time, meaning that the "dependency graph" seemingly gets usually
represented that way in U-Boot.