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 <[email protected]>
---
.../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:
+ 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.
examples:
- |
#include <dt-bindings/interrupt-controller/arm-gic.h>
--
2.39.2
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 <[email protected]>
> ---
> .../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.
> + 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).
Thanks,
Robin.
> examples:
> - |
> #include <dt-bindings/interrupt-controller/arm-gic.h>
On 06/09/2023 11:19 am, Sascha Hauer wrote:
> On Wed, Sep 06, 2023 at 10:20:26AM +0200, Quentin Schulz wrote:
>> Sascha, Robin,
>>
>> On 9/5/23 11:03, Robin Murphy wrote:
>>> [You don't often get email from [email protected]. Learn why this is
>>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>>> +ￜￜￜￜￜￜￜ 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?
>
> I haven't read this as a suggestion, but only as an argument to make it
> clear that I should describe the binding rather than anticipating
> how it should be used.
>
> I may have misunderstood it though.
Indeed it was more about the definition itself - an extra property isn't
*needed* to break the cycle since the cycle is already fully described
in DT, so anyone who can parse parents and phandles already has
sufficient information to detect it and break it at any point they
choose. However, as mentioned subsequently, breaking the cycle alone
isn't enough to guarantee that things will actually work in general.
AFAICS what we fundamentally need to know is the initial voltage of the
supply regulator, to be able to short-circuit requiring the I/O domain
in order to query it from the regulator itself, and instead just
initialise the I/O domain directly. However that would still represent a
bunch of fiddly extra DT parsing, so for practical purposes it seems
reasonable to then short-cut that into directly describing the initial
setting of the I/O domain on the node itself, such that the consumer of
the binding can easily handle it all in a self-contained manner.
Cheers,
Robin
>> 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?
>
> I started implementing that. I have moved the IO domains into subnodes
> of the IO domain controller and started adding phandles from the pin
> groups in rk3568-pinctrl.dtsi to the corresponding IO domains. After a
> couple of hours I had phandles for around a quarter of the existing
> groups of only one SoC, so doing this for all SoCs would really be a
> cumbersome job.
>
> In the end I realized this doesn't solve any problem. Also adding the
> properties I suggested doesn't prevent us from adding the more specific
> dependencies from the pins to their actual IO domains later.
>
> Sascha
>