Re: [PATCH v2 2/3] ARM: dts: aspeed: add Intel AVCRP BMC

[Dawid Glazik <dawid.glazik@linux.intel.com>]

On 10/12/2026 7:19 PM, Tan Siewert wrote:
> On 10/12/26 7:39 PM, Dawid Glazik wrote:
>> Add the initial AST2600 device tree for the Intel AVCRP BMC platform.
>>
>> Signed-off-by: Dawid Glazik <dawid.glazik@linux.intel.com>
> 
> You got Review trailers from Andrew Lunn in v1. Unless you think Andrew 
> has to re-review it (which should be then reasoned), please keep them in 
> new revisions.
> 
>  From the kernel docs [0]:
> 
> "Both Tested-by and Reviewed-by tags, once received on mailing list from 
> tester or reviewer, should be added by author to the applicable patches 
> when sending next versions. However if the patch has changed 
> substantially in following version, these tags might not be applicable 
> anymore and thus should be removed. Usually removal of someone’s Acked- 
> by, Tested-by or Reviewed-by tags should be mentioned in the patch 
> changelog with an explanation (after the ‘---’ separator)."
> 
>> ---
>> v2:
>>   - Replace bootargs console=ttyS4,115200n8 with stdout-path = &uart5
>>   - Remove the unreferenced video_memory reserved-memory node
>> ---
>>   arch/arm/boot/dts/aspeed/Makefile             |   1 +
>>   .../dts/aspeed/aspeed-bmc-intel-avcrp.dts     | 315 ++++++++++++++++++
>>   2 files changed, 316 insertions(+)
>>   create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-intel-avcrp.dts
>>
>> diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/ 
>> aspeed/Makefile
>> index 767f7c7652d5..ea4645ef9872 100644
>> --- a/arch/arm/boot/dts/aspeed/Makefile
>> +++ b/arch/arm/boot/dts/aspeed/Makefile
>> @@ -54,6 +54,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>       aspeed-bmc-ibm-rainier-4u.dtb \
>>       aspeed-bmc-ibm-sbp1.dtb \
>>       aspeed-bmc-ibm-system1.dtb \
>> +    aspeed-bmc-intel-avcrp.dtb \
>>       aspeed-bmc-intel-s2600wf.dtb \
>>       aspeed-bmc-inspur-fp5280g2.dtb \
>>       aspeed-bmc-inspur-nf5280m6.dtb \
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-intel-avcrp.dts b/ 
>> arch/arm/boot/dts/aspeed/aspeed-bmc-intel-avcrp.dts
>> new file mode 100644
>> index 000000000000..7c088c89d417
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-intel-avcrp.dts
>> @@ -0,0 +1,315 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright 2026 Intel Corporation
>> +
>> +/dts-v1/;
>> +
>> +#include "aspeed-g6.dtsi"
>> +#include <dt-bindings/gpio/aspeed-gpio.h>
>> +
>> +/ {
>> +    model = "Intel AVCRP BMC";
>> +    compatible = "intel,avcrp-bmc", "aspeed,ast2600";
>> +
>> +    aliases {
>> +        serial4 = &uart5;
>> +    };
>> +
>> +    chosen {
>> +        stdout-path = &uart5;
>> +        bootargs = "earlycon";
>> +    };
>> +
>> +    memory@80000000 {
>> +        device_type = "memory";
>> +        reg = <0x80000000 0x40000000>;
>> +    };
>> +
>> +    reserved-memory {
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges;
>> +
>> +        video_engine_memory: jpegbuffer {
>> +            size = <0x02000000>; /* 32M */
>> +            alignment = <0x01000000>;
>> +            compatible = "shared-dma-pool";
>> +            reusable;
>> +        };
>> +    };
>> +
>> +};
>> +
>> +&rtc {
>> +    status = "okay";
>> +};
> 
> Please sort your nodes, as described in the device tree design guideline 
> [1]. There's also a tool called `scripts/dtc/dt-check-style` that can 
> help with this.
> Note on that: we usually run `--mode strict` but it is known to report 
> false-positives, so please check any reported errors twice ^^
> 
>> +
>> +&fmc {
>> +    status = "okay";
>> +    flash@0 {
>> +        status = "okay";
>> +        m25p,fast-read;
>> +        label = "bmc";
>> +        spi-rx-bus-width = <4>;
>> +        spi-max-frequency = <50000000>;
>> +#include "openbmc-flash-layout-64.dtsi"
>> +    };
>> +};
>> +
>> +&uart1 {
>> +    status = "okay";
>> +};
>> +
>> +&uart3 {
>> +    status = "okay";
>> +};
>> +
>> +&uart4 {
>> +    status = "okay";
>> +};
>> +
>> +&uart5 {
>> +    status = "okay";
>> +};
>> +
>> +&uart_routing {
>> +    status = "okay";
>> +};
>> +
>> +&mdio1 {
>> +    status = "okay";
>> +
>> +    ethphy1: ethernet-phy@0 {
>> +        compatible = "ethernet-phy-id001c.c916";
>> +        reg = <0>;
>> +        reset-gpios = <&gpio0 ASPEED_GPIO(C, 1) GPIO_ACTIVE_LOW>;
>> +        reset-assert-us = <10000>;
>> +        reset-deassert-us = <50000>;
>> +    };
>> +};
>> +
>> +&mac1 {
>> +    status = "okay";
>> +    pinctrl-names = "default";
>> +    pinctrl-0 = <&pinctrl_rgmii2_default>;
>> +    clocks = <&syscon ASPEED_CLK_GATE_MAC2CLK>,
>> +         <&syscon ASPEED_CLK_MAC2RCLK>;
>> +    clock-names = "MACCLK", "RCLK";
>> +    phy-mode = "rgmii-id";
>> +    phy-handle = <&ethphy1>;
>> +};
>> +
>> +&lpc {
>> +    lpc_snoop: lpc-snoop@80 {
>> +        snoop-ports = <0x80>;
>> +        status = "okay";
>> +    };
> 
> Why's this named if you don't use it as reference anywhere.
> 
>> +};
>> +
>> +&i3c_global {
>> +    status = "okay";
>> +};
> 
> That's enabled by default.
> 
>> +
>> +&vhub {
>> +    status = "okay";
>> +};
>> +
>> +&video {
>> +    status = "okay";
>> +    memory-region = <&video_engine_memory>;
>> +};
>> +
>> +&gpio0 {
>> +    gpio-line-names =
>> +        /*A0-A7*/ "", "", "", "", "SMB_CPU_PIROM_SCL", 
>> "SMB_CPU_PIROM_SDA",
>> +                "SMB_IPMB_STBY_LVC3_SCL", "SMB_IPMB_STBY_LVC3_SDA",
>> +        /*B0-B7*/ "", "", "", "FM_CPU1_ERR2_LVT3_N",
>> +                "RGMII_BMC_RMM4_LVC3_R_MDC", 
>> "RGMII_BMC_RMM4_LVC3_R_MDIO",
> 
> Why do you name reserved GPIO lines (in this case the RGMII lines)? 
> They're used by the kernel anyways.
> 
> Tan
> 
> [0] https://docs.kernel.org/process/submitting-patches.html#using- 
> reported-by-tested-by-reviewed-by-suggested-by-and-fixes
> [1] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
Hi Tan,

Thanks for the instructions. I'll implement them in the new revision

Dawid