Re: [PATCH u-boot] ARM: dts: aspeed: Add Nvidia VR-NVL BMC

Hi Tan,

On 6/25/26 10:37 AM, Tan Siewert wrote:
>> +	ast2600-vr-bmc-nvidia.dtb \
>
> Sort alphabetically (i.e. after ast2600-intel.dtb)

Will do. Sorted alphabetically in v2, placed between ast2600-tacoma.dtb
and ast2600-x4tf.dtb.

>> +		mmc0 = &emmc_slot0;
>> +		mmc1 = &sdhci_slot0;
>> +		mmc2 = &sdhci_slot1;
>
> Do you use those on real hardware? If so, then you must enable them too.

No, eMMC/SD are not used on this board. The kernel dts doesn't enable
them either, and the BMC boots and runs from SPI-NOR. I'll drop the
mmc0/1/2 aliases in v2.

>> +		ethernet1 = &mac1;
>> +		ethernet2 = &mac2;
>> +		ethernet3 = &mac3;
>
> ethernet1, 2, and 3 are not enabled. No need to give them an alias.

Right, only mac0 is used. Dropping ethernet1/2/3 in v2.

>> +	aspeed,ecc-size-mb = <0>;
>
> aspeed,ecc-size-mb defaults to 0, no need to set it explicitly:

Dropping it in v2; aspeed,ecc-enabled is kept.

>> +		compatible = "spi-flash", "sst,w25q256";
>
> Compatible is unnecessary for all flash nodes. It defaults to
> "jedec,spi-nor" which is also a valid compatible in sf_probe.c.
> Also, sst,w25q256 does not exist as compatible in this u-boot tree.

Thanks, I missed that. Removing the compatible from all flash nodes in
v2 so they fall back to jedec,spi-nor.

>> +	flash@1 {
>> +		compatible = "spi-flash", "sst,w25q256";
>> +		status = "disabled";
>
> This node is disabled by default. No need to define it then.

Agreed, removing the disabled flash@1 node in v2.

Thanks for the review!

Best regards,
Jacky