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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux