Re: [PATCH] arm64: dts: aspeed: Fix duplicate pinctrl labels and address scheme

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

Hi Ryan,

On Thu, 2026-06-11 at 14:50 +0800, Ryan Chen wrote:
> Fix duplicate pinctrl_tach{0-15} and pinctrl_n{cts,dcd,dsr,ri}5 labels
> in aspeed-g7-soc1-pinctrl.dtsi.
> 
> Drop the cpu-index from secondary/tertiary container nodes: reduce the
> "#address-cells" from 2 to 1 and update ssp_nvic/tsp_nvic unit-address
> and reg accordingly. Also remove URL comments from the DTS.
> 
> Suggested-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> Fixes: e77bb5dc5759 ("arm64: dts: aspeed: Add initial AST27xx SoC device tree")
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
> This series contains follow-up fixes for the AST27xx DTS support that
> was merged into linux-next (e77bb5dc5759).
> 
> Two issues were identified after merge by Andrew Jeffery during review
> of the pending v11 series:

These were identified by the sashiko bot, not so much by me, as I
hadn't got around to looking at the patches at the time. I did comment
in the replies though:

https://lore.kernel.org/all/20260609025708.ADBFE1F00893@smtp.kernel.org/

Separately, the series at hand was v9, so any subsequent revision would
have been v10, not v11. This isn't significant on its own, but it is
another contribution to the collection of small errors that are
accumulating at this point, which concerns me. Please take care.

> 
> 1. Duplicate pinctrl state labels in aspeed-g7-soc1-pinctrl.dtsi caused
>    dtc to abort with fatal label-redefinition errors.

However, it didn't. soc/dt @ 564edaca1486 ("Merge tag 'sunxi-dt-for-
7.2-2' of https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux
into soc/dt"), which includes the v9 patches at e77bb5dc5759 ("arm64:
dts: aspeed: Add initial AST27xx SoC device tree"), builds without
error.

Why? Well, the report from sashiko appears misleading. Usually
duplicate labels do cause an error, for example:

   $ cat dle.dts
   /dts-v1/;
   / {
           inner: test1 {
                   prop-inner;
           };
           inner: test1 {
                   prop-inner;
           };
   };
   $ dtc -o /dev/null dle.dts
   dle.dts:6.15-8.4: ERROR (duplicate_node_names): /test1: Duplicate node name
   ERROR: Input tree has errors, aborting (use -f to force output)
   
   $ cat dle-1.dts
   /dts-v1/;
   / { };
   &{/} {
           inner: test0 {
                   prop-inner;
           };
           inner: test1 {
                   prop-inner;
           };
   };
   $ dtc -o /dev/null dle-1.dts
   dle-1.dts:8.15-10.4: ERROR (duplicate_label): /test1: Duplicate label 'inner' on /test1 and /test0
   ERROR: Input tree has errors, aborting (use -f to force output)

However, a relatively minimal reproduction of the case at hand is:

   $ cat dlu.dts
   /dts-v1/;
   / { };
   &{/} {
           inner: test1 {
                   prop-inner;
           };
           inner: test1 {
                   prop-inner;
           };
   };
   $ dtc -o /dev/null dlu.dts
   $

This doesn't error out. I recommend not assuming reports from the bot
are entirely accurate. Please test that its claims make sense before
proceeding.

While it's not good that there were duplicate nodes and labels, it is
good that you've tidied them up.

If there are modifications to the aspeed-g7-soc*-pinctrl.dtsi files in
the future, I ask that you them sorted first so we can minimise the
chance of falling into this trap again. The current order seems fairly
haphazard and likely contributed to the oversight.

> 
> 2. The synthetic container nodes (secondary, tertiary) for sub-processor

I'm not sure synthetic is the right word here. We're still describing
the hardware, just components that have their own distinct address
spaces.

On a separate note, if you feel the need to make a list when describing
the change (e.g. in the commit message or patch notes) it's usually an
indicator that the change should be split into separate commits. Please
keep this in mind for future changes.

>    interrupt controllers used a 2-cell address scheme to encode a
>    <cpu-index reg-base> tuple.  Since the cpu-index adds no value for
>    nodes that are purely phandle anchors, Andrew requested we drop it
>    and use the bare register address instead.
> ---
>  arch/arm64/boot/dts/aspeed/aspeed-g7-a35.dtsi      |  14 ++-
>  .../boot/dts/aspeed/aspeed-g7-soc1-pinctrl.dtsi    | 102 ---------------------
>  2 files changed, 6 insertions(+), 110 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7-a35.dtsi b/arch/arm64/boot/dts/aspeed/aspeed-g7-a35.dtsi
> index ef283d95649a..58193c3c3696 100644
> --- a/arch/arm64/boot/dts/aspeed/aspeed-g7-a35.dtsi
> +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7-a35.dtsi
> @@ -84,32 +84,30 @@ 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 */
> +		#address-cells = <1>;
>  		#size-cells = <0>;
> -		/* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/address.c?h=v6.16#n430 */
>  
> -		ssp_nvic: interrupt-controller@1,e000e100 {
> +		ssp_nvic: interrupt-controller@e000e100 {
>  			compatible = "arm,v7m-nvic";
>  			#interrupt-cells = <2>;
>  			#address-cells = <0>;
>  			interrupt-controller;
> -			reg = <1 0xe000e100>;
> +			reg = <0xe000e100>;

Some other cleanups to consider are ensuring the property ordering
conforms to the DTS coding style:

   https://docs.kernel.org/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node

The following grep is likely helpful:

   git grep -C1 -F 'compatible =' arch/arm64/boot/dts/aspeed

Andrew