[PATCH v2] ARM: dts: aspeed: Add device tree for Nvidia's GB200NVL BMC

[Willie Thai]

>> The GB200NVL BMC is an Aspeed Ast2600 based BMC
>> for Nvidia Blackwell GB200NVL platform.
>> Reference to Ast2600 SOC [1].
>> Reference to Blackwell GB200NVL Platform [2].
> 
> Missing blank line
> 

Thanks Krzysztof for the comments !
Will fix in the v3.

>> Co-developed-by: Mars Yang <maryang@xxxxxxxxxx>
>> Signed-off-by: Mars Yang <maryang@xxxxxxxxxx>
>> Cc: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
>> Cc: Andrew Lunn <andrew@xxxxxxx>
>> Cc: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
>> Link: Reference to Ast2600 SOC: https://www.aspeedtech.com/server_ast2600/ [1]
>> Link: Reference to Blackwell GB200NVL Platform: https://nvdam.widen.net/s/wwnsxrhm2w/blackwell-datasheet-3384703 [2]
> 
> Links do not have text, I think. Just link.
> 

Will fix in the v3.

>> Signed-off-by: Willie Thai <wthai@xxxxxxxxxx>
>> ---
>> Changes in v2:
>>   - Fix the SOB name [Krzysztof]
>>   - Fix warnings from scripts/checkpatch.pl run [Krzysztof]
>>   - Fix DTS coding style [Krzysztof]
>>   - Move pinctrl override to the bottom [Krzysztof]
>>   - Drop bootargs [Krzysztof]
>>   - Follow DTS coding style and change naming for leds node [Krzysztof]
>>   - Change flash 0 status property [Krzysztof]
>>   - Change the phy-mode to rgmii [Andrew]
>>   - Remove the max-speed in mac0 [Andrew]
>> ---
>> ---
>>  arch/arm/boot/dts/aspeed/Makefile             |    1 +
>>  .../aspeed/aspeed-bmc-nvidia-gb200nvl-bmc.dts | 1229 +++++++++++++++++
>>  2 files changed, 1230 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-nvidia-gb200nvl-bmc.dts
>> 
>> diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile
>> index 2e5f4833a073..20fd357a1ee9 100644
>> --- a/arch/arm/boot/dts/aspeed/Makefile
>> +++ b/arch/arm/boot/dts/aspeed/Makefile
>> @@ -50,6 +50,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>  	aspeed-bmc-lenovo-hr630.dtb \
>>  	aspeed-bmc-lenovo-hr855xg2.dtb \
>>  	aspeed-bmc-microsoft-olympus.dtb \
>> +	aspeed-bmc-nvidia-gb200nvl-bmc.dtb \
>>  	aspeed-bmc-opp-lanyang.dtb \
>>  	aspeed-bmc-opp-mowgli.dtb \
>>  	aspeed-bmc-opp-nicole.dtb \
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-nvidia-gb200nvl-bmc.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-nvidia-gb200nvl-bmc.dts
>> new file mode 100644
>> index 000000000000..eeec3704a43b
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-nvidia-gb200nvl-bmc.dts
>> @@ -0,0 +1,1229 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/dts-v1/;
>> +
>> +#include "aspeed-g6.dtsi"
>> +#include <dt-bindings/gpio/aspeed-gpio.h>
>> +#include <dt-bindings/leds/common.h>
>> +
>> +/ {
>> +	model = "AST2600 GB200NVL BMC";
>> +	compatible = "nvidia,gb200nvl-bmc", "aspeed,ast2600";
> 
> Missing bindings.
> 
> Please run scripts/checkpatch.pl and fix reported warnings. After that,
> run also `scripts/checkpatch.pl --strict` and (probably) fix more
> warnings. Some warnings can be ignored, especially from --strict run,
> but the code here looks like it needs a fix. Feel free to get in touch
> if the warning is not clear.
> 

Will add the binding to dt-bindings in patch v3.

> 
> ...
> 
>> +	leds {
>> +		compatible = "gpio-leds";
>> +		led-0{
> 
> Missing space befre {
> 
> This applies everywhere.
> 
>> +			label = "uid_led";
>> +			gpios = <&sgpiom0 27 GPIO_ACTIVE_LOW>;
>> +		};
>> +		led-1{
>> +			label = "fault_led";
>> +			gpios = <&sgpiom0 29 GPIO_ACTIVE_LOW>;
>> +		};
>> +		led-2{
>> +			label = "power_led";
>> +			gpios = <&sgpiom0 31 GPIO_ACTIVE_LOW>;
>> +		};
>> +
>> +	};
>> +
>> +	buttons {
>> +		button-power {
>> +			label = "power-btn";
>> +			gpio = <&sgpiom0 156 GPIO_ACTIVE_LOW>;
>> +		};
>> +		button-uid {
>> +			label = "uid-btn";
>> +			gpio = <&sgpiom0 154 GPIO_ACTIVE_LOW>;
>> +		};
>> +	};
>> +
>> +};
>> +
>> +// Enable Primary flash on FMC for bring up activity
>> +&fmc {
>> +	status = "okay";
>> +	flash@0 {
>> +		status = "okay";
> 
> Nothing improved.
> 
> Respond to comment instead of ignoring it.
> 

The property was disabled here: https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi#L172

> 
> 
> ...
> 
> 
>> +
>> +		imux33: i2c@1 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			reg = <1>;
>> +			pca9555@21 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> You already got this comment. You must apply such feedback to entire
> file instead of fixing only one issue and relying on us to find all
> instances. It's your task to find all of the instances.
> 

Sure, will modify node names to generic

Thanks !