Re: [PATCH v6 4/6] arm64: dts: aspeed: Add initial AST2700 SoC device tree

["Arnd Bergmann" <arnd@arndb.de>]

On Wed, Oct 22, 2025, at 09:05, Ryan Chen wrote:
> Add initial device tree for the ASPEED 8th BMC SoC family.
>
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>

I think this is the place where you'd want to put some information
about the chip itself. I know what it is, but others may not
know anything about it.
 
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;
> +		serial3 = &uart3;
> +		serial4 = &uart4;
> +		serial5 = &uart5;
> +		serial6 = &uart6;
> +		serial7 = &uart7;
> +		serial8 = &uart8;
> +		serial9 = &uart9;
> +		serial10 = &uart10;
> +		serial11 = &uart11;
> +		serial12 = &uart12;
> +		serial13 = &uart13;
> +		serial14 = &uart14;
> +	};

This looks like you just list all the uarts that are present
on the chip, which is not how the aliases are meant to be
used. Move this block into the board specific file and
only list the ones that are actually enabled on that particular
board.

In particular, the alias names are meant to be local to the
board and don't usually correspond to the numbering inside
of the chip. In the defconfig, we currently set
CONFIG_SERIAL_8250_NR_UARTS=8, which is enough for any
board we support so far, but that means only the first
8 aliases in the list will actually work.

> +	soc0: soc@10000000 {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges = <0x0 0x0 0x0 0x10000000 0x0 0x4000000>;
> +
> +		intc0: interrupt-controller@12100000 {
> +			compatible = "aspeed,ast2700-intc0";
> +			reg = <0 0x12100000 0 0x4000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges = <0x0 0x0 0x12100000 0x4000>;

This doesn't seem to add up: you define a local register
space for the soc from 0x0 to 0x4000000, but the registers of
the child devices are above 0x4000000.

I suspect that you forgot to adjust all the addresses in
the child devices to be inside of that range.

> +	soc1: soc@14000000 {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges = <0x0 0x0 0x0 0x14000000 0x0 0x10000000>;

This probably needs some explanation: why are there two 'soc@...'
devices? Is this literally two chips in the system, or are you
describing two buses inside of the same SoC?

> +
> +		mdio0: mdio@14040000 {
> +			compatible = "aspeed,ast2600-mdio";
> +			reg = <0 0x14040000 0 0x8>;
> +			resets = <&syscon1 SCU1_RESET_MII>;
> +			status = "disabled";
> +		};

I see that you use the old compatible="aspeed,ast2600-mdio" string
exclusively here. While this works, I would suggest you list both
a more specific "aspeed,ast2700-mdio" string to refer to the version
in this chip as well as the fallback "aspeed,ast2600-mdio" string
as the generic identifier.

The binding obviously has to describe both in that case, but the
driver does not need to be modified as long as both behave the
same way.

> +
> +		syscon1: syscon@14c02000 {
> +			compatible = "aspeed,ast2700-scu1", "syscon", "simple-mfd";
> +			reg = <0x0 0x14c02000 0x0 0x1000>;
> +			ranges = <0x0 0x0 0x14c02000 0x1000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +
> +			scu_ic2: interrupt-controller@100 {
> +				compatible = "aspeed,ast2700-scu-ic2";
> +				reg = <0x100 0x8>;
> +				#interrupt-cells = <1>;
> +				interrupts-extended = <&intc1_5 0>;
> +				interrupt-controller;
> +			};
> +
> +			scu_ic3: interrupt-controller@108 {
> +				compatible = "aspeed,ast2700-scu-ic3";
> +				reg = <0x108 0x8>;
> +				#interrupt-cells = <1>;
> +				interrupts-extended = <&intc1_5 26>;
> +				interrupt-controller;
> +			};

This looks a bit silly to be honest: you have two separate devices
that each have a single register and a different compatible string?

Also you claim to be compatible with "syscon" but nothing actually
refers to the syscon node in that form?

       Arnd