Message-ID: <459f84c56a5010910ecbf8b445c092674f060691.camel@codeconstruct.com.au>
Date: Fri, 23 Jan 2026 17:03:43 +1030
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Billy Tsai <billy_tsai@...eedtech.com>, Lee Jones <lee@...nel.org>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley	 <conor+dt@...nel.org>, Joel Stanley <joel@....id.au>, Linus Walleij
	 <linusw@...nel.org>, Bartosz Golaszewski <brgl@...nel.org>
Cc: Andrew Jeffery <andrew@...id.au>, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-aspeed@...ts.ozlabs.org, 
	linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org, 
	linux-gpio@...r.kernel.org, bmc-sw@...eedtech.com
Subject: Re: [PATCH v3 1/3] Add compatible strings for AST2700 pinctrl to
 the SCU binding.

On Tue, 2026-01-20 at 19:43 +0800, Billy Tsai wrote:
> AST2700 consists of two interconnected SoC instances. Each SoC has
> its own pinctrl register block, which needs to be described
> independently in the device tree.
> 
> Introduce "aspeed,ast2700-soc0-pinctrl" for the SoC0 pinctrl, which
> follows the same usage model as the existing AST2600 pinctrl.
> 
> The SoC1 pinctrl registers follow a regular and predictable layout,
> which allows describing them using an existing generic pinctrl
> binding without introducing a new SoC-specific compatible string.
> 
> Signed-off-by: Billy Tsai <billy_tsai@...eedtech.com>
> ---
>  Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
> index da1887d7a8fe..ff6cf8f63cbc 100644
> --- a/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
> +++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
> @@ -87,6 +87,7 @@ patternProperties:
>              - aspeed,ast2400-pinctrl
>              - aspeed,ast2500-pinctrl
>              - aspeed,ast2600-pinctrl
> +            - aspeed,ast2700-soc0-pinctrl
>  
>      required:
>        - compatible

So, in addition to Krzysztof's concern about the patch subject, I think
it's worth mulling over some further concerns that we might address
with a clean break for the AST2700. Specifically, whether we can tidy
up historical baggage by exploiting auxiliary bus[0] instead continuing
to pretend the SCU is an simple-mfd.

[0]: https://docs.kernel.org/driver-api/auxiliary_bus.html

The original AST2400 SCU bindings were cooked up in darker times and
have been a regular source of regret (same for the LPC bindings - we
can consider them later). The use of simple-mfd unnecessarily invokes a
bunch of machinery in the driver core to avoid explicit iteration of
subnodes in the driver(s), and causes tension between the need to
specify regs and the typically haphazard register layout of the SCUs.

Related, this entry from "DOs and DON’Ts for designing and writing
Devicetree bindings"[1] stands out:

 * DON’T create nodes just for the sake of instantiating drivers.
   Multi-function devices only need child nodes when the child nodes
   have their own DT resources. A single node can be multiple providers
   (e.g. clocks and resets).

[1]: https://docs.kernel.org/devicetree/bindings/writing-bindings.html#overall-design

We have a collection of all sorts of odd-ball bits of functionality,
and all of it can largely be implied by the SCU compatible. Avoiding
specifying things that are implied is also touched on by[2]:

 * DON’T add properties to avoid a specific compatible. DON’T add
   properties if they are implied by (deducible from) the compatible.

[2]: https://docs.kernel.org/devicetree/bindings/writing-bindings.html#properties

So, can we use auxiliary bus instead, and avoid extending the regret in
the devicetree?

Well, the interesting news is that auxiliary bus is already on the way
by way of the AST2700 clock and reset drivers[3].

[3]: https://lore.kernel.org/all/20250917020539.3690324-1-ryan_chen@aspeedtech.com/

A noteworthy observation is the auxiliary bus approach for pinctrl was
already used by (at least) the mobileye eyeq5 platform:

- Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
- arch/mips/boot/dts/mobileye/eyeq5.dtsi
- arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi
- drivers/clk/clk-eyeq.c
- drivers/pinctrl/pinctrl-eyeq5.c

So, we're not inventing anything new here.

Going down this path for pinctrl to fix the SCU situation will require
some rework of what's already merged for the AST2700. However, we've
not yet merged either a DTS or DTSI using the compatibles (and by
extension, aren't using the AST2700 support from the drivers), so I
hope that allows us to do a course-correction without too much
collateral damage.

A possible path forward is to:

 * Move AST2700 definitions out of mfd/aspeed,ast2x00-scu.yaml into one
   of:
    - soc/aspeed/aspeed,ast2700-scu.yaml: Follow the example of
      mobileye,eyeq5-olb?
    - arm/aspeed/aspeed,ast2700-scu.yaml: We already have e.g. the secure
      boot controller documented under arm/aspeed
 * Retain compatible strings but require simple-mfd is not specified
 * Rework AST2700 support introduced in:
    - drivers/irqchip/irq-aspeed-scu-ic.c
    - drivers/soc/aspeed/aspeed-socinfo.c

This is my preference, at least for the moment. The change to make the
SoC0 pinctrl driver compatible with auxiliary bus shouldn't take much
overall. Unwinding the binding situation is a little more involved, but
not too much. I think it just needs consensus on the direction from the
devicetree maintainers.

Andrew

Powered by blists - more mailing lists