[PATCH RFC] dt-bindings: pinctrl: support specifying pins

[PATCH RFC] dt-bindings: pinctrl: support specifying pins

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Add support for "pins" node with pin@ subnodes. This allows specifying
all pins (and their names) at DT level.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
While working with pinctrl in Linux I started wondering if we could
start specifying pins in DT instead of Linux drivers. When working with
DT we usually avoid hardcoding hardware description in drivers so it
isn't clear to me why it doesn't apply to pins.

Please let me know if this makes sense. If by some chance I'm correct I
think that specifying groups and functions could follow too.

FWIW: I didn't start working on Linux reading pins from DT yet.
---
 .../bindings/pinctrl/brcm,ns-pinmux.yaml      | 12 +++++++++-
 .../devicetree/bindings/pinctrl/pinctrl.yaml  | 23 +++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
index 8d1e5b1cdd5f..92a86b0822d6 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
@@ -74,7 +74,7 @@ required:
   - reg
   - reg-names
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
@@ -83,6 +83,16 @@ examples:
         reg = <0x1800c1c0 0x24>;
         reg-names = "cru_gpio_control";
 
+        pins {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            pin@0 {
+                reg = <0>;
+                label = "spi_clk";
+            };
+        };
+
         spi-pins {
             function = "spi";
             groups = "spi_grp";
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
index d471563119a9..d2f105e9570d 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
@@ -42,4 +42,27 @@ properties:
       This property can be set either globally for the pin controller or in
       child nodes for individual pin group control.
 
+  pins:
+    type: object
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      "^pin@[0-9a-z]+$":
+        type: object
+
+        properties:
+          reg:
+            description: Pin number
+
+          label:
+            description: Pin name
+            $ref: /schemas/types.yaml#/definitions/string
+
+        additionalProperties: false
+
 additionalProperties: true
-- 
2.31.1

Re: [PATCH RFC] dt-bindings: pinctrl: support specifying pins

[Linus Walleij]

On Thu, Nov 11, 2021 at 12:14 AM Rafał Miłecki <zajec5@gmail.com> wrote:

> While working with pinctrl in Linux I started wondering if we could
> start specifying pins in DT instead of Linux drivers. When working with
> DT we usually avoid hardcoding hardware description in drivers so it
> isn't clear to me why it doesn't apply to pins.

Historically this is what pinctrl-single.c does.
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt

At the time we created pin control there was a bit back-and-forth and
the conclusion was that there is not one-size-fits all for pin defines.

The reason TI (Tony) wanted to push the information into DT
was that what he gets is a number of unprocessed ASIC datasets,
that are then turned into tables with a script. Header files or DTS
source alike, but some kind of tables.

At the time (2011?) it was unclear what kind of data should go into
e.g. header and data files in the kernel (modules) and what should
go into the DT. So the approach to put pin information into the DT
was allowed for pinctrl-single.

The way I have understood it, DT maintainers have since gotten
a bit wary about (ab)using the DT as a container for "anything data"
and prefer that drivers contain details and derive these from
compatible strings.

As of today, IIUC the DT maintainers are against this scheme.

That said, the topic is open in a way. Some people are also annoyed
that some graphics drivers just ask Torvalds to pull 100.000+ lines
of register defnes in some merge windows. The data has to go
somewhere.

Yours,
Linus Walleij

Re: [PATCH RFC] dt-bindings: pinctrl: support specifying pins

[Rob Herring]

On Thu, Nov 11, 2021 at 04:31:28PM +0100, Linus Walleij wrote:
> On Thu, Nov 11, 2021 at 12:14 AM Rafał Miłecki <zajec5@gmail.com> wrote:
> 
> > While working with pinctrl in Linux I started wondering if we could
> > start specifying pins in DT instead of Linux drivers. When working with
> > DT we usually avoid hardcoding hardware description in drivers so it
> > isn't clear to me why it doesn't apply to pins.

I thought we were...

> Historically this is what pinctrl-single.c does.
> Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> 
> At the time we created pin control there was a bit back-and-forth and
> the conclusion was that there is not one-size-fits all for pin defines.
> 
> The reason TI (Tony) wanted to push the information into DT
> was that what he gets is a number of unprocessed ASIC datasets,
> that are then turned into tables with a script. Header files or DTS
> source alike, but some kind of tables.
> 
> At the time (2011?) it was unclear what kind of data should go into
> e.g. header and data files in the kernel (modules) and what should
> go into the DT. So the approach to put pin information into the DT
> was allowed for pinctrl-single.
> 
> The way I have understood it, DT maintainers have since gotten
> a bit wary about (ab)using the DT as a container for "anything data"
> and prefer that drivers contain details and derive these from
> compatible strings.
> 
> As of today, IIUC the DT maintainers are against this scheme.

The question is more can the data be static (complete) and correct? That 
doesn't work for quirk properties nor entire clock trees with a node per 
clock.

Pins? Maybe. That's a flat thing and you should know all the pins up 
front.

Rob

Re: [PATCH RFC] dt-bindings: pinctrl: support specifying pins

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [211111 15:32]:
> At the time (2011?) it was unclear what kind of data should go into
> e.g. header and data files in the kernel (modules) and what should
> go into the DT. So the approach to put pin information into the DT
> was allowed for pinctrl-single.
> 
> The way I have understood it, DT maintainers have since gotten
> a bit wary about (ab)using the DT as a container for "anything data"
> and prefer that drivers contain details and derive these from
> compatible strings.
> 
> As of today, IIUC the DT maintainers are against this scheme.

We have some newish tools now compared 2011 though with #pinctrl-cells.
And we now have also GENERIC_PINCTRL_GROUPS, GENERIC_PINMUX_FUNCTIONS
and GENERIC_PINCONF :)

The problem with the pinctrl-single binding is that it uses the hardware
specific mux values in addition to the mux register offsets. IMO the
values should use Linux generic pinctrl defines instead. Just like we
do for the gpio and interrupt bindings. And then the generic pinctrl
binding would be very similar to the interrupts-extended binding for
example.

And with a generic pinctrl binding pinctrl-single could be updated to
parse the generic binding naturally too in addition to the legacy
binding.

> That said, the topic is open in a way. Some people are also annoyed
> that some graphics drivers just ask Torvalds to pull 100.000+ lines
> of register defnes in some merge windows. The data has to go
> somewhere.

Yes and the amount of SoC specific LOC under drivers/pinctrl is pretty
staggering already.

With all that SoC specific data built into the kernel, it's like going
camping with all your pants stuffed into your car instead of just the
pants you need :)

We only need the SoC specific data for the booted SoC, so devicetree
and loadable modules makes more sense there compared to the current
built-in setup.

Regards,

Tony

Re: [PATCH RFC] dt-bindings: pinctrl: support specifying pins

[Andy Shevchenko]

On Fri, Nov 12, 2021 at 12:16 PM Tony Lindgren <tony@atomide.com> wrote:
> * Linus Walleij <linus.walleij@linaro.org> [211111 15:32]:
> > At the time (2011?) it was unclear what kind of data should go into
> > e.g. header and data files in the kernel (modules) and what should
> > go into the DT. So the approach to put pin information into the DT
> > was allowed for pinctrl-single.
> >
> > The way I have understood it, DT maintainers have since gotten
> > a bit wary about (ab)using the DT as a container for "anything data"
> > and prefer that drivers contain details and derive these from
> > compatible strings.
> >
> > As of today, IIUC the DT maintainers are against this scheme.
>
> We have some newish tools now compared 2011 though with #pinctrl-cells.
> And we now have also GENERIC_PINCTRL_GROUPS, GENERIC_PINMUX_FUNCTIONS
> and GENERIC_PINCONF :)
>
> The problem with the pinctrl-single binding is that it uses the hardware
> specific mux values in addition to the mux register offsets. IMO the
> values should use Linux generic pinctrl defines instead. Just like we
> do for the gpio and interrupt bindings. And then the generic pinctrl
> binding would be very similar to the interrupts-extended binding for
> example.
>
> And with a generic pinctrl binding pinctrl-single could be updated to
> parse the generic binding naturally too in addition to the legacy
> binding.
>
> > That said, the topic is open in a way. Some people are also annoyed
> > that some graphics drivers just ask Torvalds to pull 100.000+ lines
> > of register defnes in some merge windows. The data has to go
> > somewhere.
>
> Yes and the amount of SoC specific LOC under drivers/pinctrl is pretty
> staggering already.
>
> With all that SoC specific data built into the kernel, it's like going
> camping with all your pants stuffed into your car instead of just the
> pants you need :)
>
> We only need the SoC specific data for the booted SoC, so devicetree
> and loadable modules makes more sense there compared to the current
> built-in setup.

I'm against putting that into DT and here is why.

DT is the thing that describes the _platform_. While it's fine to put
GPIO expander thingy (and we actually do this with labeling schema for
GPIOs, right?), the SoC level of things is a _hardware_ and with all
flexibility the DT gives us we will definitely have a deviations on
_different_ platforms with _the same_ SoC! To work around this we must
have a validation of the pin names and their functions in many places.

And last but not least the copying it in tons of DT feels like a
duplication effort.,

AFAIU the topic, the pin control lacks labeling schema that will
provide the view from the platform perspective, while driver provides
from HW perspective.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC] dt-bindings: pinctrl: support specifying pins

[Tony Lindgren]

* Andy Shevchenko <andy.shevchenko@gmail.com> [211112 11:22]:
> > We only need the SoC specific data for the booted SoC, so devicetree
> > and loadable modules makes more sense there compared to the current
> > built-in setup.
> 
> I'm against putting that into DT and here is why.
> 
> DT is the thing that describes the _platform_. While it's fine to put
> GPIO expander thingy (and we actually do this with labeling schema for
> GPIOs, right?), the SoC level of things is a _hardware_ and with all
> flexibility the DT gives us we will definitely have a deviations on
> _different_ platforms with _the same_ SoC! To work around this we must
> have a validation of the pin names and their functions in many places.

I think you are misunderstanding what I mean here. Certainly the driver
needs to know how to deal with the SoC specific hardware. And that we
can easily do that in quite easily already. The device tree data I'm
describing would be similar to the interrupts with instance offset and
generic mux flags.

See for example the driver for drivers/pinctrl/ti/pinctrl-ti-iodelay.c.
For that driver we have the instance and picosecond iodelay values in
the devicetree, and with #nr-pinctrl cells there could be some generic
pinctrl mux flags. We are missing the generic pinctrl flags part AFAIK.

> And last but not least the copying it in tons of DT feels like a
> duplication effort.,

Hmm I don't think we have any of that for what I'm describing. But
please take a look at the iodelay example above, maybe I'm not
following.

> AFAIU the topic, the pin control lacks labeling schema that will
> provide the view from the platform perspective, while driver provides
> from HW perspective.

Agreed we need a generic labeling schema.

Regards,

Tony

Re: [PATCH RFC] dt-bindings: pinctrl: support specifying pins

[Andy Shevchenko]

On Fri, Nov 12, 2021 at 2:16 PM Tony Lindgren <tony@atomide.com> wrote:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [211112 11:22]:
> > > We only need the SoC specific data for the booted SoC, so devicetree
> > > and loadable modules makes more sense there compared to the current
> > > built-in setup.
> >
> > I'm against putting that into DT and here is why.
> >
> > DT is the thing that describes the _platform_. While it's fine to put
> > GPIO expander thingy (and we actually do this with labeling schema for
> > GPIOs, right?), the SoC level of things is a _hardware_ and with all
> > flexibility the DT gives us we will definitely have a deviations on
> > _different_ platforms with _the same_ SoC! To work around this we must
> > have a validation of the pin names and their functions in many places.
>
> I think you are misunderstanding what I mean here.

Ah, okay. Thanks for explaining!

>  Certainly the driver
> needs to know how to deal with the SoC specific hardware. And that we
> can easily do that in quite easily already. The device tree data I'm
> describing would be similar to the interrupts with instance offset and
> generic mux flags.
>
> See for example the driver for drivers/pinctrl/ti/pinctrl-ti-iodelay.c.
> For that driver we have the instance and picosecond iodelay values in
> the devicetree, and with #nr-pinctrl cells there could be some generic
> pinctrl mux flags. We are missing the generic pinctrl flags part AFAIK.
>
> > And last but not least the copying it in tons of DT feels like a
> > duplication effort.,
>
> Hmm I don't think we have any of that for what I'm describing. But
> please take a look at the iodelay example above, maybe I'm not
> following.
>
> > AFAIU the topic, the pin control lacks labeling schema that will
> > provide the view from the platform perspective, while driver provides
> > from HW perspective.
>
> Agreed we need a generic labeling schema.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC] dt-bindings: pinctrl: support specifying pins

[Rob Herring]

On Thu, Nov 11, 2021 at 12:14:36AM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Add support for "pins" node with pin@ subnodes. This allows specifying
> all pins (and their names) at DT level.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> While working with pinctrl in Linux I started wondering if we could
> start specifying pins in DT instead of Linux drivers. When working with
> DT we usually avoid hardcoding hardware description in drivers so it
> isn't clear to me why it doesn't apply to pins.
> 
> Please let me know if this makes sense. If by some chance I'm correct I
> think that specifying groups and functions could follow too.
> 
> FWIW: I didn't start working on Linux reading pins from DT yet.
> ---
>  .../bindings/pinctrl/brcm,ns-pinmux.yaml      | 12 +++++++++-
>  .../devicetree/bindings/pinctrl/pinctrl.yaml  | 23 +++++++++++++++++++
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
> index 8d1e5b1cdd5f..92a86b0822d6 100644
> --- a/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
> @@ -74,7 +74,7 @@ required:
>    - reg
>    - reg-names
>  
> -additionalProperties: false
> +unevaluatedProperties: false
>  
>  examples:
>    - |
> @@ -83,6 +83,16 @@ examples:
>          reg = <0x1800c1c0 0x24>;
>          reg-names = "cru_gpio_control";
>  
> +        pins {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            pin@0 {
> +                reg = <0>;

Where does 'reg' value come from?

> +                label = "spi_clk";
> +            };

If you just want a list of pins names, then why not just a list of 
names?

Rob