Re: [PATCH v9 2/2] ARM: dts: aspeed: ventura2: Add Meta ventura2 BMC

Hi Kyle,

> Ventura2 is a Rack Management Controller. It is a modular
> device designed to manage liquid cooling systems and monitor hardware
> states within an IT rack. The system uses an AST2600 BMC for management.
> 
> RMCv2 serves several critical roles:
> - Detects liquid leakage at both tray and rack levels.
> - Communicates with and controls liquid cooling equipment.
> - Manages leakage events and executes system recovery protocols.
> 
> Key hardware features include:
> - An extensive I2C and IO expander topology to support comprehensive
>   sensor monitoring and backward compatibility with legacy trays.
> - MCTP over I2C support for asynchronous device communications.
> - A dual-flash design for BMC firmware redundancy.
> 
> Signed-off-by: Kyle Hsieh <[email protected]>

*snip*

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-ventura2.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-ventura2.dts
> new file mode 100644
> index 000000000000..f1273ca31bf8
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-ventura2.dts
> @@ -0,0 +1,3005 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2023 Facebook Inc.
> +/dts-v1/;
> +
> +#include "aspeed-g6.dtsi"
> +#include <dt-bindings/i2c/i2c.h>
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +
> +/ {
> +	compatible = "facebook,ventura2-rmc", "aspeed,ast2600";
> +	model = "Facebook Ventura2 RMC";
> +
> +	aliases {
> +		/*
> +		* Pre-allocate I2C bus aliases for userspace predictability.
> +		* Several I2C channels are intentionally left empty in this
> +		* DTS as they are strictly reserved for future hardware
> +		* feature expansions and add-on boards that will interface
> +		* with these busses.
> +		*/
> +		/*
> +		* i2c switch 0-0077, pca9548, 8 child channels assigned
> +		* with bus number 16-23.
> +		*/

This alignment is one of the things that seem to be a false-positive for
dt-check-style in strict mode (value-whitespace). See [0] where Krzysztof said
that it may report false-positives, and "Review and investigate warnings from
'strict' mode, but do not fix them automatically without thinking".

@Krzysztof wdyt about this one?

> +		i2c16 = &i2c0mux0ch0;
> +		i2c17 = &i2c0mux0ch1;
> +		i2c18 = &i2c0mux0ch2;
> +		i2c19 = &i2c0mux0ch3;
> +		i2c20 = &i2c0mux0ch4;
> +		i2c21 = &i2c0mux0ch5;
> +		i2c22 = &i2c0mux0ch6;
> +		i2c23 = &i2c0mux0ch7;

*snip*

> +	spi {
> +		compatible = "spi-gpio";
> +		#address-cells = <1>;
> +		cs-gpios = <&gpio0 ASPEED_GPIO(Z, 0) GPIO_ACTIVE_LOW>;
> +		miso-gpios = <&gpio0 ASPEED_GPIO(Z, 5) GPIO_ACTIVE_HIGH>;
> +		mosi-gpios = <&gpio0 ASPEED_GPIO(Z, 4) GPIO_ACTIVE_HIGH>;
> +		num-chipselects = <1>;
> +		sck-gpios = <&gpio0 ASPEED_GPIO(Z, 3) GPIO_ACTIVE_HIGH>;
> +		#size-cells = <0>;

Shouldn't #address-cells and #size-cells be ordered together? If reported by
dt-check-style as issue, then it might be a false-positive.

> +
> +		tpm@0 {
> +			compatible = "infineon,slb9670", "tcg,tpm_tis-spi";
> +			reg = <0>;
> +			spi-max-frequency = <33000000>;
> +		};
> +	};
> +};
> +
> +&adc0 {
> +	pinctrl-0 =
> +		<&pinctrl_adc0_default &pinctrl_adc1_default
> +		&pinctrl_adc2_default &pinctrl_adc3_default
> +		&pinctrl_adc4_default &pinctrl_adc5_default
> +		&pinctrl_adc6_default &pinctrl_adc7_default>;
> +	pinctrl-names = "default";
> +	vref-supply = <&p1v8_bmc_aux>;
> +
> +	status = "okay";
> +};
> +
> +&adc1 {
> +	pinctrl-0 = <&pinctrl_adc8_default &pinctrl_adc10_default>;
> +	pinctrl-names = "default";
> +	vref-supply = <&p2v5_bmc_aux>;
> +
> +	status = "okay";
> +};
> +
> +&ehci0 {
> +	status = "okay";
> +};
> +
> +&ehci1 {
> +	status = "okay";
> +};
> +
> +&fmc {
> +	status = "okay";
> +
> +	flash@0 {
> +		label = "bmc";
> +		spi-max-frequency = <50000000>;
> +		m25p,fast-read;
> +
> +		status = "okay";
> +		#include "openbmc-flash-layout-128.dtsi"
> +	};
> +
> +	flash@1 {
> +		label = "alt-bmc";
> +		spi-max-frequency = <50000000>;
> +		m25p,fast-read;
> +
> +		status = "okay";

Missing '#include "openbmc-flash-layout-128-alt.dtsi"'?

> +	};
> +};

*snip*

> +&mac2 {
> +	phy-mode = "rmii";
> +	pinctrl-0 = <&pinctrl_rmii3_default>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +
> +	/*
> +	* The Marvell 88E6393X is initialized at boot via EEPROM
> +	* configuration and hardware straps.
> +	* The BMC connects via an RMII fixed-link; link parameters are fixed
> +	* by board design.
> +	*/

This kind of writing gives me the suspicion that it was written with assistance
by an LLM, but I might be wrong.

Either way, if you have used AI assistance for your series (or only for this
patch), even if you have only used it for the commit message, then you must
declare it via the "Assisted-by" trailer [1][2].

> +	fixed-link {
> +		full-duplex;
> +		speed = <100>;
> +	};
> +};
> +
> +&mac3 {
> +	pinctrl-0 = <&pinctrl_rmii4_default>;
> +	pinctrl-names = "default";
> +	use-ncsi;
> +
> +	status = "okay";
> +};
> +
> +&peci0 {
> +	status = "okay";
> +};

You mentioned that this is a rack management controller. Does it really require
PECI?

> +
> +&sgpiom0 {
> +	bus-frequency = <100000>;
> +	gpio-line-names =
> +	/*"input pin","output pin"*/
> +	/*A0 - A7*/
> +		"power-chassis-good","FM_PLD_HEARTBEAT_LVC3_R",
> +		"host0-ready","R_BMC_PTH_RST_BTN_L",
> +		"CONTROL_VT2_SUPPLY1_CLOSE","FM_MDIO_SW_SEL_PLD",
> +		"CONTROL_VT2_SUPPLY2_CLOSE","FM_88E6393X_BIN_UPDATE_EN_N",
> +		"CONTROL_VT2_SUPPLY3_CLOSE","Sequence_TransFR_Alert",

Why's "Sequence_TransFR_Alert" written in camelcase?

Tan

[0] https://lore.kernel.org/linux-aspeed/[email protected]/
[1] https://docs.kernel.org/process/submitting-patches.html#using-assisted-by
[2] https://docs.kernel.org/process/coding-assistants.html

-- 
Tan Siewert <[email protected]>