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

On Wed, Jun 10, 2026 at 7:15 PM Andrew Jeffery
<[email protected]> wrote:
>
> Hi Kyle,
>
> On Mon, 2026-06-08 at 10:42 +0800, Kyle Hsieh wrote:
> > Add linux device tree entry related to the Meta(Facebook) rmc-node.
>
> This is redundant as it is evident from the patch itself. Can you
> please remove it?
>
> > The system use an AT2600 BMC.
> > This node is named "ventura2".
>
> Can you provide some more detail about the platform in the commit
> message? What's it's purpose? Can you describe some interesting
> features or details about its design?
>
Hi Andrew,

To briefly answer your question: Ventura2 is Rack Management
Controller. It is a modular device primarily designed to manage liquid
cooling systems and monitor rack-level hardware states. Its key
hardware features include an extensive I2C/GPIO topology for
tray-level and rack-level liquid leakage detection, as well as MCTP
over I2C support for asynchronous device communications.

I will remove the redundant sentence and include this detailed
description of the platform's purpose and architecture in the commit
message for v7.
> >
> > Signed-off-by: Kyle Hsieh <[email protected]>
> > ---
> >  arch/arm/boot/dts/aspeed/Makefile                  |    1 +
> >  .../dts/aspeed/aspeed-bmc-facebook-ventura2.dts    | 2888 ++++++++++++++++++++
> >  2 files changed, 2889 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile
> > index 9adf9278dc94..6b96997629d4 100644
> > --- a/arch/arm/boot/dts/aspeed/Makefile
> > +++ b/arch/arm/boot/dts/aspeed/Makefile
> > @@ -32,6 +32,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
> >       aspeed-bmc-facebook-minipack.dtb \
> >       aspeed-bmc-facebook-santabarbara.dtb \
> >       aspeed-bmc-facebook-tiogapass.dtb \
> > +     aspeed-bmc-facebook-ventura2.dtb \
> >       aspeed-bmc-facebook-wedge40.dtb \
> >       aspeed-bmc-facebook-wedge100.dtb \
> >       aspeed-bmc-facebook-wedge400-data64.dtb \
> > 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..9bf7d6e52e40
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-ventura2.dts
> > @@ -0,0 +1,2888 @@
> > +// 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>
> > +
> > +/ {
> > +     model = "Facebook Ventura2 RMC";
> > +     compatible = "facebook,ventura2-rmc", "aspeed,ast2600";
> >
>
> ...
>
> > +};
> > +
>
> ...
>
> > +&gpio1 {
> > +     gpio-line-names =
> > +     /*18A0-18A7*/   "","","","","","","","",
> > +     /*18B0-18B7*/   "","","","",
> > +                     "FM_BOARD_BMC_REV_ID0","FM_BOARD_BMC_REV_ID1",
> > +                     "FM_BOARD_BMC_REV_ID2","",
> > +     /*18C0-18C7*/   "SPI_BMC_BIOS_ROM_IRQ0_R_N","","","","","","","",
> > +     /*18D0-18D7*/   "","","","","","","","",
> > +     /*18E0-18E3*/   "FM_BMC_PROT_LS_EN","AC_PWR_BMC_BTN_R_N","","";
> > +};
> > +
> > +&i2c0 {
> > +     status = "okay";
> > +
> > +     i2c-mux@77 {
> > +             compatible = "nxp,pca9548";
> > +             reg = <0x77>;
> > +             #address-cells = <1>;
> > +             #size-cells = <0>;
> > +             i2c-mux-idle-disconnect;
> > +
> > +             i2c0mux0ch0: i2c@0 {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     reg = <0>;
> > +             };
> > +
> > +             i2c0mux0ch1: i2c@1 {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     reg = <1>;
> > +             };
> > +
> > +             i2c0mux0ch2: i2c@2 {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     reg = <2>;
> > +             };
> > +
> > +             i2c0mux0ch3: i2c@3 {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     reg = <3>;
> > +                     status = "okay";
> > +             };
> > +
> > +             i2c0mux0ch4: i2c@4 {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     reg = <4>;
> > +                     status = "okay";
> > +             };
> > +
> > +             i2c0mux0ch5: i2c@5 {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     reg = <5>;
> > +                     status = "okay";
> > +
> > +                     eeprom@56 {
> > +                             compatible = "atmel,24c128";
> > +                             reg = <0x56>;
> > +                     };
> > +             };
> > +
> > +             i2c0mux0ch6: i2c@6 {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     reg = <6>;
> > +
> > +                     eeprom@56 {
> > +                             compatible = "atmel,24c128";
> > +                             reg = <0x56>;
> > +                     };
> > +
> > +                     fan_io_expander0: gpio@20 {
> > +                             compatible = "nxp,pca9555";
> > +                             reg = <0x20>;
> > +                             gpio-controller;
> > +                             #gpio-cells = <2>;
> > +                     };
> > +
> > +                     fan_io_expander1: gpio@21 {
> > +                             compatible = "nxp,pca9555";
> > +                             reg = <0x21>;
> > +                             gpio-controller;
> > +                             #gpio-cells = <2>;
> > +                     };
> > +
> > +                     adc@1d {
> > +                             compatible = "ti,adc128d818";
> > +                             reg = <0x1d>;
> > +                             ti,mode = /bits/ 8 <1>;
> > +                     };
> > +
> > +                     adc@35 {
> > +                             compatible = "maxim,max11617";
> > +                             reg = <0x35>;
> > +                     };
> > +             };
> > +
> > +             i2c0mux0ch7: i2c@7 {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     reg = <7>;
> > +
> > +                     fanctl0: fan-controller@20 {
> > +                             compatible = "maxim,max31790";
> > +                             reg = <0x20>;
> > +                             #address-cells = <1>;
> > +                             #size-cells = <0>;
> > +                             channel@2 {
>
> Can you make sure that you consistently use a blank line to separate
> child nodes from each other and from properties in their parent?
>
> Please fix throughout.

I will recheck the whole file to satisfy the rule in the next patch v7, thanks.
>
> > +                                     reg = <2>;
> > +                                     sensor-type = "TACH";
> > +                             };
> > +                             channel@5 {
> > +                                     reg = <5>;
> > +                                     sensor-type = "TACH";
> > +                             };
> > +                     };
> > +
> > +                     fanctl1: fan-controller@23 {
> > +                             compatible = "nuvoton,nct7363";
> > +                             reg = <0x23>;
> > +                             #pwm-cells = <2>;
> > +
> > +                             fan-9 {
> > +                                     pwms = <&fanctl1 0 20000>;
> > +                                     tach-ch = /bits/ 8 <0x09>;
> > +                             };
> > +
> > +                             fan-11 {
> > +                                     pwms = <&fanctl1 0 20000>;
> > +                                     tach-ch = /bits/ 8 <0x0B>;
> > +                             };
> > +
> > +                             fan-10 {
> > +                                     pwms = <&fanctl1 4 20000>;
> > +                                     tach-ch = /bits/ 8 <0x0A>;
> > +                             };
> > +
> > +                             fan-13 {
> > +                                     pwms = <&fanctl1 4 20000>;
> > +                                     tach-ch = /bits/ 8 <0x0D>;
> > +                             };
> > +
> > +                             fan-15 {
> > +                                     pwms = <&fanctl1 6 20000>;
> > +                                     tach-ch = /bits/ 8 <0x0F>;
> > +                             };
> > +
> > +                             fan-1 {
>
> Can you please sort the fan nodes in ascending order?
I will reorder the fan node in next patch v7.
>
> > +                                     pwms = <&fanctl1 6 20000>;
> > +                                     tach-ch = /bits/ 8 <0x01>;
> > +                             };
> > +
> > +                             fan-0 {
> > +                                     pwms = <&fanctl1 10 20000>;
> > +                                     tach-ch = /bits/ 8 <0x00>;
> > +                             };
> > +
> > +                             fan-3 {
> > +                                     pwms = <&fanctl1 10 20000>;
> > +                                     tach-ch = /bits/ 8 <0x03>;
> > +                             };
> > +                     };
> > +             };
> > +     };
> > +};
> >
>
> ...
>
> > +
> > +     // Marvell 88E6393X EEPROM
>
> Please try to be consistent with the comment style (prefer /* */).
I will fix this in the next patch v7, thanks for your suggestion.
>
> > +     eeprom@50 {
> > +             compatible = "atmel,24c64";
> > +             reg = <0x50>;
> > +     };
> > +
> > +     rtc@51 {
> > +             compatible = "nxp,pcf8563";
> > +             reg = <0x51>;
> > +     };
> > +};
> > +