Re: [PATCH v8 2/2] ARM: dts: aspeed: Add Device Tree for Facebook Anacapa BMC

[Andrew Jeffery <andrew@codeconstruct.com.au>]

Hi Peter,

A few follow-ups below on further inspection. Apologies for not
addressing more of these in your earlier submissions.

As a bit of a nit I'd prefer we use "devicetree" instead of "Device
Tree" in the subject. That said, it's also implied by the "dts:" part
of the prefix, so even better would be to drop it altogether:

ARM: dts: aspeed: Add Facebook Anacapa platform

On Tue, 2025-11-25 at 05:21 +0800, Peter Shen wrote:
> Add the initial device tree source file for the Facebook Anacapa BMC
> platform, based on the Aspeed AST2600 SoC.
> 
> This device tree configures the platform-specific peripherals and
> aliases for OpenBMC usage.

This describes what we can see in the commit itself. Can you please
rather describe the purpose of the platform? Why does the design exist?

> 
> Signed-off-by: Peter Shen <sjg168@gmail.com>
> ---
>  arch/arm/boot/dts/aspeed/Makefile             |    1 +
>  .../aspeed/aspeed-bmc-facebook-anacapa.dts    | 1044 +++++++++++++++++
>  2 files changed, 1045 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa.dts
> 
> diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile
> index 0f0b5b707654..e1b2fc7b8c08 100644
> --- a/arch/arm/boot/dts/aspeed/Makefile
> +++ b/arch/arm/boot/dts/aspeed/Makefile
> @@ -17,6 +17,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>  	aspeed-bmc-asus-x4tf.dtb \
>  	aspeed-bmc-bytedance-g220a.dtb \
>  	aspeed-bmc-delta-ahe50dc.dtb \
> +	aspeed-bmc-facebook-anacapa.dtb \
>  	aspeed-bmc-facebook-bletchley.dtb \
>  	aspeed-bmc-facebook-catalina.dtb \
>  	aspeed-bmc-facebook-clemente.dtb \
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa.dts
> new file mode 100644
> index 000000000000..a9bed728339b
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-anacapa.dts
> @@ -0,0 +1,1044 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/dts-v1/;
> +#include "aspeed-g6.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +#include <dt-bindings/i2c/i2c.h>
> +
> +/ {
> +	model = "Facebook Anacapa BMC";
> +	compatible = "facebook,anacapa-bmc", "aspeed,ast2600";
> +
> +	aliases {
> +		serial0 = &uart1;
> +		serial2 = &uart3;

Aliases should be defined sequentially. Is there a strong reason to
skip serial1 here?

> +		serial3 = &uart4;
> +		serial4 = &uart5;
> +		i2c16 = &i2c0mux0ch0;
> +		i2c17 = &i2c0mux0ch1;

...

> +
> +&adc0 {
> +	aspeed,int-vref-microvolt = <2500000>;
> +	pinctrl-names = "default";
> +	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>;
> +	status = "okay";
> +};
> +
> +&adc1 {
> +	aspeed,int-vref-microvolt = <2500000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_adc10_default>;
> +	status = "okay";
> +};
> +
> +&ehci1 {
> +	status = "okay";
> +};
> +
> +&uhci {
> +	status = "okay";
> +};
> +
> +&fmc {

Can you please order all label references alphabetically? There are
other options in the DTS style guide, but alphabetical order is the
easiest to enforce by simple inspection.

Andrew

> +	status = "okay";
> +
> +	flash@0 {
> +		status = "okay";
> +		m25p,fast-read;
> +		label = "bmc";
> +		spi-max-frequency = <50000000>;
> +#include "openbmc-flash-layout-128.dtsi"
> +	};
> +
> +	flash@1 {
> +		status = "okay";
> +		m25p,fast-read;
> +		label = "alt-bmc";
> +		spi-max-frequency = <50000000>;
> +	};
> +};
> +

...