Re: [PATCH 2/2] ARM: dts: aspeed: Add NVIDIA MGX Vera C2 BMC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ARM: dts: aspeed: vr-c2: missing i2c-mux-idle-disconnect on i2c4 muxes
- [Medium] ARM: dts: aspeed: duplicate gpio-line-names in vr-c2-bmc
--

commit a1ef1efffcf5e5e21a0a7edd3526a31cbe156f2d
Author: Jacky Huang <jackyhuang@nvidia.com>

ARM: dts: aspeed: Add NVIDIA MGX Vera C2 BMC

This commit adds the device tree for the NVIDIA MGX Vera C2 BMC platform
based on the ASPEED AST2600 SoC.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-nvidia-vr-c2-bmc.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-nvidia-vr-c2-bmc.dts
> new file mode 100644
> index 0000000000000..7e8ae8afea088
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-nvidia-vr-c2-bmc.dts
> @@ -0,0 +1,1321 @@

[ ... ]

> +&gpio0 {
> +	gpio-line-names =

[ ... ]

> +	/*V0-V7*/
> +	"AP_EROT_REQ-O",
> +	"EROT_AP_GNT-I",
> +	"",
> +	"",
> +	"PCB_TEMP_ALERT-I",
> +	"", "", "",

[ ... ]

> +};
> +
> +&gpio1 {
> +	gpio-line-names =
> +	/*A0-A7*/
> +	"", "", "", "", "", "", "", "",
> +
> +	/*B0-B7*/
> +	"", "", "", "",
> +	"AP_EROT_REQ-O",
> +	"EROT_AP_GNT-I",
> +	"IO_EXPANDER_INT_L-I",
> +	"",
> +
> +	/*C0-C7*/
> +	"", "", "", "", "", "", "", "",
> +
> +	/*D0-D7*/
> +	"", "", "", "", "", "", "",
> +	"I2C_SSIF_ALERT_L-I",
> +
> +	/*E0-E7*/
> +	"", "", "", "", "", "", "", "";
> +};

[Severity: Medium]
Are "AP_EROT_REQ-O" and "EROT_AP_GNT-I" intentionally defined on both &gpio0
(pins V0-V1) and &gpio1 (pins B4-B5)?

Because both AST2600 GPIO controllers are active at the same time and &gpio0
probes before &gpio1, name-based GPIO lookups via gpio_name_to_desc() or
libgpiod will always resolve to &gpio0 and shadow the lines on &gpio1.

Similarly, further down in this file, the pca6408@20 expanders on
&i2c_mcio_c0_south0 and &i2c_mcio_c1_south0 define identical "GPU1_*"
gpio-line-names, and the pca6408@20 expanders on &i2c_mcio_c0c1_north3 and
&i2c_mcio_c0c1_north6 define identical "E1S_*" gpio-line-names. Should those
line names be disambiguated (for example with C1/C2 prefixes) so that
consumers can address both instances by name?

[ ... ]

> +/* MCIO expansion muxes */
> +&i2c4 {
> +	multi-master;
> +
> +	status = "okay";
> +
> +	mctp@18 {
> +		compatible = "mctp-i2c-controller";
> +		reg = <(0x18 | I2C_OWN_SLAVE_ADDRESS)>;
> +	};
> +
> +	i2c-mux@70 {
> +		compatible = "nxp,pca9548";
> +		reg = <0x70>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		vdd-supply = <&standby_power_regulator>;

[ ... ]

> +	};
> +
> +	i2c-mux@71 {
> +		compatible = "nxp,pca9548";
> +		reg = <0x71>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		vdd-supply = <&standby_power_regulator>;

[ ... ]

> +	};
> +
> +	i2c-mux@75 {
> +		compatible = "nxp,pca9548";
> +		reg = <0x75>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		vdd-supply = <&standby_power_regulator>;

[ ... ]

> +	};
> +};

[ ... ]

> +/* GPU1 GPIO expander (C1) */
> +&i2c_mcio_c0_south0 {
> +	/* GPU 1, Expander @0x20 */
> +	pca6408@20 {
> +		compatible = "nxp,pca6408";
> +		reg = <0x20>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		vcc-supply = <&standby_power_regulator>;
> +		gpio-line-names =
> +		"GPU1_GLOBAL_WP-O",
> +		"GPU1_OOB_RST_N-O",
> +		"GPU1_OOB_RECOVERY-O",
> +		"GPU1_MCU_RECOVERY_N-O",
> +		"GPU1_MCU_RST_N-O",
> +		"GPU1_MCU_BYPASS_N-O",
> +		"GPU1_SMBUS_FRU_EEPROM_WP-O",
> +		"";
> +	};
> +};
> +
> +/* GPU0 GPIO expander */
> +&i2c_mcio_c0_south2 {
> +	/* GPU 0, Expander @0x20 */
> +	pca6408@20 {
> +		compatible = "nxp,pca6408";
> +		reg = <0x20>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		vcc-supply = <&standby_power_regulator>;
> +		gpio-line-names =
> +		"GPU0_GLOBAL_WP-O",
> +		"GPU0_OOB_RST_N-O",
> +		"GPU0_OOB_RECOVERY-O",
> +		"GPU0_MCU_RECOVERY_N-O",
> +		"GPU0_MCU_RST_N-O",
> +		"GPU0_MCU_BYPASS_N-O",
> +		"GPU0_SMBUS_FRU_EEPROM_WP-O",
> +		"";
> +	};
> +};
> +
> +/* E1.S backplane GPIO expander (C1) */
> +&i2c_mcio_c0c1_north3 {
> +	/* E1S Backplane, Expander @0x20 */
> +	pca6408@20 {
> +		compatible = "nxp,pca6408";
> +		reg = <0x20>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		vcc-supply = <&standby_power_regulator>;
> +		gpio-line-names =
> +		"E1S_MCU_RESET_L-O",
> +		"E1S_MCU_RECOVERY_L-O",
> +		"",
> +		"",
> +		"",
> +		"",
> +		"E1S_LOWER_PERST-O",
> +		"E1S_UPPER_PERST-O";
> +	};
> +};
> +
> +/* E1.S backplane GPIO expander (C2) */
> +&i2c_mcio_c0c1_north6 {
> +	/* E1S Backplane, Expander @0x20 */
> +	pca6408@20 {
> +		compatible = "nxp,pca6408";
> +		reg = <0x20>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		vcc-supply = <&standby_power_regulator>;
> +		gpio-line-names =
> +		"E1S_MCU_RESET_L-O",
> +		"E1S_MCU_RECOVERY_L-O",
> +		"",
> +		"",
> +		"",
> +		"",
> +		"E1S_LOWER_PERST-O",
> +		"E1S_UPPER_PERST-O";
> +	};
> +};
> +
> +/* GPU1 GPIO expander (C2) */
> +&i2c_mcio_c1_south0 {
> +	/* GPU 1, Expander @0x20 */
> +	pca6408@20 {
> +		compatible = "nxp,pca6408";
> +		reg = <0x20>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		vcc-supply = <&standby_power_regulator>;
> +		gpio-line-names =
> +		"GPU1_GLOBAL_WP-O",
> +		"GPU1_OOB_RST_N-O",
> +		"GPU1_OOB_RECOVERY-O",
> +		"GPU1_MCU_RECOVERY_N-O",
> +		"GPU1_MCU_RST_N-O",
> +		"GPU1_MCU_BYPASS_N-O",
> +		"GPU1_SMBUS_FRU_EEPROM_WP-O",
> +		"";
> +	};
> +};

[Severity: High]
Should i2c-mux@70, i2c-mux@71, and i2c-mux@75 on &i2c4 include the
i2c-mux-idle-disconnect property (like the pca9546 muxes on &i2c6 and &i2c9)?

In pca954x_probe() and pca954x_deselect_mux(), when neither idle-state nor
i2c-mux-idle-disconnect is specified, data->idle_state defaults to
MUX_IDLE_AS_IS and each PCA9548 leaves its last selected downstream channel
connected to &i2c4 after transfers complete.

Because five pca6408@20 GPIO expanders sit behind these three switches on
&i2c4 (&i2c_mcio_c0_south0, &i2c_mcio_c0_south2, &i2c_mcio_c0c1_north3,
&i2c_mcio_c0c1_north6, and &i2c_mcio_c1_south0) at the same slave address
0x20, leaving channels connected across the three switches bridges multiple
pca6408@20 devices onto &i2c4 simultaneously. Wouldn't this cause I2C address
contention at 0x20, false-positive probing of unpopulated C1/C2 expanders,
and cross-writes to the wrong GPU or E1.S reset/recovery/PERST GPIOs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793243187.git.jackyhuang@nvidia.com?part=2