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

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

On Thu, Oct 23, 2025, at 09:37, Ryan Chen wrote:
>
>> > +	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.
>
> Understood. I'll move the aliases block from the SoC dtsi into the
> EVB board dts. For the EVB, UART12 is used as the default console,
> and the board labels match the SoC numbering, so I plan to keep:
>
> Does that look acceptable?
> ast2700-evb.dts
> 	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;
> }

I think this would be broken for the defconfig if the consol is
on serial12. I would recommend using serial0 as the console, like

aliases {
       serial0 = &uart12;
}

in this case. If additional uarts are enabled, add those as
further aliases.

>> 
>> > +	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?
>
> The AST2700 is two soc connection with a property bus.
> Sharing some decode registers. Each have it own ahb bus.

I don't understand your explanation, 

>> 
>> > +
>> > +		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.
>
> Thanks, will submit ast2700-mdio. 
> Question, should I add in here patch series?
> Or go for another patch thread?

Since there is no corresponding driver change, I would keep the binding
change as a patch in this series.

>> > +
>> > +		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?
>
> Yes, it have difference register define in each scu-ic#. That is 
> compatible with design.
> https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-aspeed-scu-ic.c#L45-L48

Right, if the driver already has this design, it does make sense to
not change it for the new generation. For a newly added driver I would
probably do it differently.

>> Also you claim to be compatible with "syscon" but nothing actually refers to the
>> syscon node in that form?
>
> There is another submit ongoing in pinctrl which will use syscon. 
> https://lwn.net/ml/all/20250829073030.2749482-2-billy_tsai@aspeedtech.com/
>
> Could I keep it? or I should remove it?

The version of the driver you are linking does not appear to use
syscon, maybe this is an artifact from a previous version?

If so, you can drop it. On the other hand, this does seem to
be a classic syscon device and keeping it marked that way is not
harmful, just redundant if you actually use the more specific
compatible string.


     Arnd