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

Hi Tan,

Thanks for the review.

On Fri, Aug 21, 2026 at 2:04 AM Tan Siewert <[email protected]> wrote:
>
> 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?

You're right, and this is a regression I introduced in v9. In v8 these comment
blocks used the standard kernel style. While running dt-check-style, I applied
the strict-mode output too mechanically and dropped the space before '*' in all
38 comment blocks in the file. I will restore to the standard style in v10.

>
> > +             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.

I will put the two properties back together in v10. Like the comment
alignment above, I'll list this as a strict-mode warning I'm not acting on.

>
> > +
> > +             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"'?

I will add it in v10.

>
> > +     };
> > +};
>
> *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].

You're right; I used an AI assistant to write this comment. I'll add the
Assisted-by trailer in v10.

>
> > +     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?

It does. Ventura2 can be populated with a COM Express module, and &peci0,
&kcs3 and &lpc_ctrl describe that path, so I'd like to keep them. I will add
a comment in v10 explaining this, so it's clear from the DTS itself.

For the record, the v5 changelog claimed &peci0 had been removed. That was
incorrect; the node was never actually dropped. Apologies for the confusion
that may have caused when reading through the series.
>
> > +
> > +&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?

An oversight; it should be consistent with the surrounding names. I
will renamed to
"SEQUENCE_TRANSFR_ALERT" in v10.

Thanks again for the thorough review.

Kyle Hsieh