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

[sashiko-bot@kernel.org]

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/tree/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.

>  		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.

>  	pinctrl_tach1_default: tach1-default-state {
>  		function = "TACH1";
>  		groups = "TACH1";
>  	};
[ ... ]
> diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7-soc1.dtsi b/arch/arm64/boot/dts/aspeed/aspeed-g7-soc1.dtsi
> new file mode 100644
> index 0000000000000..bc990f79e693f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7-soc1.dtsi
[ ... ]
> @@ -396,10 +396,10 @@ sgpiom1: sgpiom@14c0d000 {
>  		bus-frequency = <12000000>;
>  		status = "disabled";
>  	};
>  
>  	intc1: interrupt-controller@14c18000 {
>  		compatible = "aspeed,ast2700-intc1";
>  		reg = <0 0x14c18000 0 0x400>;
>  		interrupt-controller;
>  		interrupt-parent = <&intc0>;
>  		#interrupt-cells = <1>;
>  		aspeed,interrupt-ranges =
>  			<0 6 &intc0 480>,   /* M0  ~ M5  */
>  			<10 6 &intc0 490>,   /* M10 ~ M15  */
>  			<20 6 &intc0 500>,   /* M20 ~ M25  */

[Severity: High]
Does this incorrectly route SOC1 interrupts to the TSP instead of the
Cortex-A35 GIC?

Looking at intc0 in aspeed-g7-soc0.dtsi, interrupts 426 through 553 are
linearly mapped to the Tertiary Service Processor (tsp_nvic):

    <426 128 &tsp_nvic 0 0 >, /* linear to TSP */

Since intc1 maps its cascaded interrupts to intc0 in the 480-520 range,
won't this break interrupt delivery for all SOC1 peripherals (like uart12,
SDHCI, FMC, and SPI) on the main CPU by routing them to the TSP instead?

>  			<30 6 &intc0 510>,   /* M30 ~ M35  */
>  			<40 6 &intc0 520>,   /* M40 ~ M45  */
>  			<50 1 &bootmcu_hlic 11>; /* only 1 pin to BootMCU */
>  	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-upstream_ast2700-v9-0-f631752f0cb1@aspeedtech.com?part=3