Re: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Quentin Schulz]

Sascha, Robin,

On 9/5/23 11:03, Robin Murphy wrote:
> [You don't often get email from robin.murphy@xxxxxxx. Learn why this is 
> important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On 2023-09-04 12:58, Sascha Hauer wrote:
> > Add rockchip,io-domains property to the Rockchip pinctrl driver. This
> > list of phandles points to the IO domain device(s) the pins of the
> > pinctrl driver are supplied from.
> >
> > Also a rockchip,io-domain-boot-on property is added to pin groups
> > which can be used for pin groups which themselves are needed to access
> > the regulators an IO domain is driven from.
> >
> > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > ---
> >   .../bindings/pinctrl/rockchip,pinctrl.yaml          | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml 
> > b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> > index 10c335efe619e..92075419d29cf 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> > +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> > @@ -62,6 +62,11 @@ properties:
> >         Required for at least rk3188 and rk3288. On the rk3368 this 
> > should
> >         point to the PMUGRF syscon.
> >
> > +  rockchip,io-domains:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    description:
> > +      Phandles to io domains
> > +
> >     "#address-cells":
> >       enum: [1, 2]
> >
> > @@ -137,7 +142,13 @@ additionalProperties:
> >               - description:
> >                   The phandle of a node contains the generic pinconfig 
> > options
> >                   to use as described in pinctrl-bindings.txt.
> > -
> > +      rockchip,io-domain-boot-on:
>
> I don't think "on" is a particularly descriptive or useful property name
> for something that has no "off" state. Furthermore it's no help at all
> if the DT consumer *is* the bootloader that's expected to configure this
> in the first place. IMO it would seem a lot more sensible to have an
> integer (or enum) property which describes the actual value for the
> initial I/O domain setting. Then Linux can choose to assume the presence
> of the property at all implies that the bootloader should have set it up
> already, but also has the option of actively enforcing it as well if we
> want to.

This is actually highly misleading. Whether the bootloader handles IO 
domains for pinctrl or not absolutely doesn't matter to the kernel since 
the kernel is required to handle IO domain for pinctrl as well. They're 
not exclusive, they are not complementary.

The point is that the voltage of the IO domain **can** change at 
runtime, at any point in time. We could theoretically have the 
bootloader require the regulator to be running at 1.8V and then the 
kernel at 3.3V, both should work (can't think of anything that would 
work like that but why not, the kernel IO domain driver is supposed to 
handle that within the kernel runtime even).

The issue here is that we want to avoid a cyclic dependency, where the 
pinctrl is needed for the IO domain regulator that we're trying to add 
as a dependency of the same pinctrl. There needs to be either some smart 
detection or a property to specify that the IO domain dependency needs 
to be ignored. This seems unfortunately to be for working around how 
Linux handles dependencies between devices and doesn't allow cyclic 
dependencies. At the same time, I do not know if there's anyway to not 
work around it?

> > +        type: boolean
> > +        description:
> > +          If true assume that the io domain needed for this pin group 
> > has been
> > +          configured correctly by the bootloader. This is needed to 
> > break cyclic
> > +          dependencies introduced when a io domain needs a regulator 
> > that can be
> > +          accessed through pins configured here.
>
> This is describing a Linux implementation detail, not the binding
> itself. There's no technical reason a DT consumer couldn't already
> figure this much out from the existing topology (by observing that the
> pinctrl consumer is a grandparent of the I/O domain's supply).

I am guessing you're suggesting to have some complex handling in the 
driver to detect those cyclic dependencies and ignore the IO domain 
dependency for the pinctrl pins where this happens?

This can actually be quite difficult to detect reliably:
We need to go through the phandle in pinctrl to the IO domain DT node, 
then check all phandles there to other DT node (likely regulators), then 
we need to look into the pinctrl-0 (actually, the one for "default" 
maybe, but what about the other states of pinctrl?) phandles and then 
parse the pinctrt DT nodes to see if they're pointing to the same DT 
node as the one we're trying to use. Here, we also do not know if the 
regulator DT node has other dependencies that needs to be accounted for.

I haven't put too much thoughts into it so maybe it's easier/harder than 
what I'm saying here (or maybe I'm completely off too...).

One of the issues we're having here too is that we lose granularity. 
There are multiple domains inside an IO domain device and here we make 
the whole pinctrl device depend on all domains from one IO domain device 
(there can be multiple ones) while it is factually (on the HW level) 
only dependent on one domain. Considering (if I remember correctly) 
Heiko highly suggested we think about adding child nodes to the IO 
domain devices to have a DT node per domain in the IO domain device, how 
would this work with the suggested DT binding?

Cheers,
Quentin