Message-ID: <f95f6d61-8809-e668-0458-453a8dfbe641@marcan.st>
Date:   Thu, 7 Oct 2021 00:52:07 +0900
From:   Hector Martin <marcan@...can.st>
To:     Rob Herring <robh+dt@...nel.org>
Cc:     linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Marc Zyngier <maz@...nel.org>, Arnd Bergmann <arnd@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mark Kettenis <mark.kettenis@...all.nl>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        devicetree@...r.kernel.org,
        "open list:THERMAL" <linux-pm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>
Subject: Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

On 06/10/2021 09.58, Rob Herring wrote:
> On Tue, Oct 5, 2021 at 10:59 AM Hector Martin <marcan@...can.st> wrote:
>> Future SoCs are expected to use backwards compatible registers, and the
>> "apple,pmgr-pwrstate" represents any such interfaces (possibly with
>> additional features gated by the more specific compatible), allowing
>> them to be bound without driver updates. If a backwards incompatible
>> change is introduced in future SoCs, it will require a new compatible,
>> such as "apple,pmgr-pwrstate-v2".
> 
> Is that because past SoCs used the same registers? I don't see how
> else you have any insight to what future SoCs will do.
> 
> Normally we don't do 1 node per register type bindings, so I'm a bit
> leery about doing 1 node per domain.

Yes, we can trace a pretty clear lineage from past SoCs. Plus Apple 
folks themselves have confirmed that this is an explicit policy:

https://twitter.com/stuntpants/status/1442276493669724160

Already within this SoC we have two PMGR instances with different 
register sets (one covers all devices that stay on during system sleep), 
although I am only instantiating one in our devicetree at the moment. 
And of course there is the A14, which is the same generation as the M1 
and has exactly the same register format, but a different set of domains.

Having sub-nodes describing individual power domains has some prior art 
(e.g. power/rockchip,power-controller.yaml). In that case the nodes are 
all managed by the parent node, and use the hierarchical format, but the 
hierarchical format cannot handle multi-parent nodes (which we do have, 
or at least Apple describes them that way). Since we really have no 
"top-level" implementation specifics to worry about, I think it makes 
sense to just bind to single nodes at this point, which makes the driver 
very simple since it doesn't have to perform any bookkeeping for 
multiple domains.

I realize this is all kind of "not the way things are usually done", but 
I don't want to pass up on the opportunity to have one driver last us 
multiple SoCs if we have the chance, and it's looking like it should :-)

Note that as new features are implemented (e.g. auto-PM, which I will 
add to this driver later), that also naturally lends itself to 
forwards-compat, as SoCs without those features at all simply wouldn't 
request them in the DT. In this case an "apple,auto-pm" flag would 
enable that for domains where we want it, and those that don't support 
it (or a hypothetical past SoC without the feature at all) would simply 
not use it.

>> +properties:
>> +  $nodename:
>> +    pattern: "^power-controller@[0-9a-f]+$"
> 
> Drop this and define this node in the syscon schema with a $ref to this schema.

Ack, makes sense.

>> +  apple,domain-name:
>> +    description: |
>> +      Specifies the name of the SoC device being controlled. This is used to
>> +      name the power/reset domains.
>> +    $ref: /schemas/types.yaml#/definitions/string
> 
> No other power domain binding needs this, why do you?

Because they all hardcode the domain names in the drivers for every SoC :-)

Without a name of some sort in the devicetree, all our genpds would have 
to use numeric register offsets or the like, which seems quite ugly.

> I prefer 1 complete example in the MFD schema rather than piecemeal examples.

Sure. Would we leave this schema without examples then?

> As the child nodes are memory mapped devices, size should be 1. Then
> address translation works (though Linux doesn't care (currently)).

This requires all the reg properties to also declare the reg size, right?

One thing I wonder is whether it would make sense to allow 
#power-domain-cells = <1> and then be able to declare consecutive ranges 
of related power registers in one node. This would be useful for e.g. 
the 9 UARTs, the 4 SPI controllers, the 6 MCAs, the 5 I2C controllers, 
the 5 PWM controllers, etc (which all have uniform parents and features 
and are consecutive, so could be described together). I'm not sure if 
it's worth it, thoughts?

-- 
Hector Martin (marcan@...can.st)
Public Key: https://mrcn.st/pub

Powered by blists - more mailing lists