Re: [PATCH v9 3/4] arm64: dts: aspeed: Add initial AST27xx SoC device tree

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

Hi Ryan,

On Tue, 2026-06-09 at 05:36 +0000, Ryan Chen wrote:
> > Subject: Re: [PATCH v9 3/4] arm64: dts: aspeed: Add initial AST27xx SoC device
> > tree
> > 
> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to
> > consider:
> > - [High] INTC1 maps its summary interrupts to INTC0's TSP routing range,
> > preventing SOC1 interrupts from reaching the Cortex-A35 GIC.
> > - [Low] The device tree defines ad-hoc top-level nodes ('secondary', 'tertiary',
> > 'bootmcu') that violate standard naming schemas, and embeds direct Linux
> > kernel source URLs to justify '#size-cells' behavior.
> > - [Low] Duplicate label definitions in the pinctrl device tree include for multiple
> > pinctrl groups in SOC1 will cause a hard DTC build failure.
> > --
> > 
> > commit 92031db9a3c255cefbfcc33df4ca0cb9780dd226
> > Author: Ryan Chen <ryan_chen@aspeedtech.com>
> > 
> > arm64: dts: aspeed: Add initial AST27xx SoC device tree
> > 
> > Add initial device tree support for the ASPEED AST27xx family, the
> > 8th-generation Baseboard Management Controller (BMC) SoCs.
> > 
> > > diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7-a35.dtsi
> > > b/arch/arm64/boot/dts/aspeed/aspeed-g7-a35.dtsi
> > > new file mode 100644
> > > index 0000000000000..ef283d95649a3
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7-a35.dtsi
> > [ ... ]
> > > @@ -82,10 +82,10 @@ l2: l2-cache0 {
> > >  	};
> > > 
> > >  	secondary {
> > >  		#address-cells = <2>;
> > >  		/*
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/
> > of/address.c?h=v6.16#n491 */
> > >  		#size-cells = <0>;
> > >  		/*
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre
> > > e/drivers/of/address.c?h=v6.16#n430 */
> > > 
> > 
> > [Severity: Low]
> > Are these OS-specific comments and ad-hoc root nodes ('secondary', 'tertiary',
> > 'bootmcu') appropriate here?
> > 
> > Device trees typically provide an OS-agnostic hardware description using
> > generic nodes (like 'bus', 'soc', or 'remoteproc'). Embedding Linux kernel
> > source URLs suggests the hardware is being modeled around a specific Linux
> > parser implementation rather than providing an independent hardware
> > description.
> 
> The concern is valid and acknowledged. The sub-processor interrupt
> controllers (ssp_nvic for SSP/Cortex-M4, tsp_nvic for TSP/Cortex-M4,
> bootmcu_hlic for BootMCU/RISC-V) cannot be placed under the soc1 bus
> because their registers reside in each sub-processor's private address
> space and are not accessible from the Cortex-A35 cores.
> 
> The 2-cell address scheme (#address-cells = <2>, #size-cells = <0>)
> encodes a <cpu-index reg-base> tuple as suggested by Matt Johnston [1].

That was me, not Matt.

Further, that two cell scheme was part of a tentative sketch for
discussion from quite some time ago and wasn't intended to be
copy/pasted without further thought. We should remove the index now.

> The Linux kernel source URLs in the comments are admittedly
> Linux-specific and will be removed in v11; the cover letter explains
> the rationale instead.
> 
> All sub-processor nodes carry status = "disabled". They are present
> solely to provide phandle references for the aspeed,interrupt-ranges
> properties in intc0 and intc1, which the interrupt controller drivers
> use to build the interrupt domain hierarchy at boot time.
> 
> [1] https://lore.kernel.org/all/1a2ca78746e00c2ec4bfc2953a897c48376ed36f.camel@codeconstruct.com.au/
> 
> > 
> > >  		ssp_nvic: interrupt-controller@1,e000e100 {
> > [ ... ]
> > > diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7-soc1-pinctrl.dtsi
> > > b/arch/arm64/boot/dts/aspeed/aspeed-g7-soc1-pinctrl.dtsi
> > > new file mode 100644
> > > index 0000000000000..72d93323593d5
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7-soc1-pinctrl.dtsi
> > [ ... ]
> > > @@ -475,10 +475,10 @@ pinctrl_hvi3c15_default: hvi3c15-default-state {
> > >  		function = "I3C15";
> > >  		groups = "HVI3C15";
> > >  	};
> > > 
> > >  	pinctrl_tach0_default: tach0-default-state {
> > >  		function = "TACH0";
> > >  		groups = "TACH0";
> > >  	};
> > > 
> > 
> > [Severity: Low]
> > Is this label duplicated? There is another pinctrl_tach0_default definition
> > earlier in this file.
> > 
> > Defining the same pinctrl label multiple times (such as pinctrl_tach0_default
> > through pinctrl_tach15_default, and pinctrl_ncts5_default) in the same device
> > tree will cause a fatal syntax error in the Device Tree Compiler.
> 
> Will fixed in v11. The labels pinctrl_tach0_default through
> pinctrl_tach15_default were duplicated in aspeed-g7-soc1-pinctrl.dtsi
> due to a copy-paste error. The second block has been removed.

Please send me fixes for these, and avoid sending patches under review
to soc@ in the future:

> Note that the soc@kernel.org is usually not the place to discuss the
> patches, thus work sent to this address should be already considered
> as acceptable by the community.

https://docs.kernel.org/process/maintainer-soc.html#submitting-patches-to-the-main-soc-maintainers

Andrew