mbox series

[RFC,v2,0/5] gpio: add pinctrl based generic gpio driver

Message ID 20231005025843.508689-1-takahiro.akashi@linaro.org
Headers show
Series gpio: add pinctrl based generic gpio driver | expand

Message

AKASHI Takahiro Oct. 5, 2023, 2:58 a.m. UTC
This is a revised version of my previous RFC[1]. Although I modified
the commits to make them look SCMI-independent, they are still posted
as RFC because I have never tested them on real hardware.

(background)
I'm currently working on implementing SCMI pinctrl/gpio drivers
on U-Boot[2]. Although the pinctrl driver for the kernel[3] was submitted
by EPAM, it doesn't contain the gpio driver and I believe that we should
discuss a couple of points on the kernel side to finalize my design for
U-Boot. 

So this RFC is intended for reviews, especially to raise some issues.

1) how to obtain a value on an input pin
   All the existing gpio drivers are set to obtain a value on an input
   pin by accessing the hardware directly. In SCMI case, however, this is
   just impossible in its nature and must be supported via a protocol
   using "Input-value" configuration type. (See the spec[4], table-23.)

   The current pinconf framework is missing the feature (the pinconf
   parameter and a helper function). See patch#1, #2 and #3.

   Please note that there is an issue around the pin configuration in
   EPAM's current pinctrl driver as I commented[5].

2) DT bindings
   I would like to propose a generic binding for pinctrl based gpio driver.
   This allows a "consumer" driver to handle gpio pins like as other
   normal gpio controllers support. (patch#5)

3) generic GPIO driver
   Based on (2), I tried to prototype a generic driver in patch#4.
   Thanks to a set of existing pinctrl_gpio helper functions, except (1),
   It seems that the driver can be implemented not relying on pin controller
   specific code, at least for SCMI pinctrl.

I will appreciate any comments.

-Takahiro Akashi

[1] https://lkml.iu.edu//hypermail/linux/kernel/2310.0/00362.html
[2] https://lists.denx.de/pipermail/u-boot/2023-September/529765.html
[3] https://lkml.iu.edu/hypermail/linux/kernel/2308.1/01082.html
[4] https://developer.arm.com/documentation/den0056/
[5] https://lkml.iu.edu/hypermail/linux/kernel/2308.2/07483.html

AKASHI Takahiro (5):
  pinctrl: define PIN_CONFIG_INPUT
  pinctrl: always export pin_config_get_for_pin()
  pinctrl: add pinctrl_gpio_get_config()
  gpio: add pinctrl based generic gpio driver
  dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

 .../bindings/gpio/pin-control-gpio.yaml       |  55 ++++++
 drivers/gpio/Kconfig                          |   7 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-by-pinctrl.c                | 165 ++++++++++++++++++
 drivers/pinctrl/core.c                        |  19 ++
 drivers/pinctrl/pinconf.h                     |  10 +-
 include/linux/pinctrl/consumer.h              |   8 +
 include/linux/pinctrl/pinconf-generic.h       |   5 +
 8 files changed, 268 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
 create mode 100644 drivers/gpio/gpio-by-pinctrl.c

Comments

Krzysztof Kozlowski Oct. 5, 2023, 7:48 p.m. UTC | #1
On 05/10/2023 04:58, AKASHI Takahiro wrote:
> A dt binding for pin controller based generic gpio driver is defined in
> this commit. One usable device is Arm's SCMI.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio0: gpio@0 {

No reg, so no unit address.

Drop also unused label.


> +        compatible = "pin-control-gpio";
> +        gpio-controller;

Best regards,
Krzysztof
Rob Herring Oct. 6, 2023, 1:18 p.m. UTC | #2
On Thu, 05 Oct 2023 11:58:43 +0900, AKASHI Takahiro wrote:
> A dt binding for pin controller based generic gpio driver is defined in
> this commit. One usable device is Arm's SCMI.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> RFC v2 (Oct 5, 2023)
> * rename the binding to pin-control-gpio
> * add the "description"
> * remove nodename, hog properties, and a consumer example
> RFC (Oct 2, 2023)
> ---
>  .../bindings/gpio/pin-control-gpio.yaml       | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/gpio/pin-control-gpio.example.dts:18.23-26.11: Warning (unit_address_vs_reg): /example-0/gpio@0: node has a unit name, but no reg or ranges property

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231005025843.508689-6-takahiro.akashi@linaro.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Rob Herring Oct. 6, 2023, 1:23 p.m. UTC | #3
On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:
> A dt binding for pin controller based generic gpio driver is defined in
> this commit. One usable device is Arm's SCMI.

You don't need a "generic" binding to have a generic driver. Keep the 
binding specific and then decide in the OS to whether to use a generic 
or specific driver. That decision could change over time, but the 
binding can't. For example, see simple-panel.


> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> RFC v2 (Oct 5, 2023)
> * rename the binding to pin-control-gpio
> * add the "description"
> * remove nodename, hog properties, and a consumer example
> RFC (Oct 2, 2023)
> ---
>  .../bindings/gpio/pin-control-gpio.yaml       | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml b/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> new file mode 100644
> index 000000000000..bc935dbd7edb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/pin-control-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Pin control based generic GPIO controller
> +
> +description:
> +  The pin control-based GPIO will facilitate a pin controller's ability
> +  to drive electric lines high/low and other generic properties of a
> +  pin controller to perform general-purpose one-bit binary I/O.
> +
> +maintainers:
> +  - AKASHI Takahiro <akashi.takahiro@linaro.org>
> +
> +properties:
> +  compatible:
> +    const: pin-control-gpio
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-ranges: true
> +
> +  gpio-ranges-group-names: true
> +
> +patternProperties:
> +  "^.+-hog(-[0-9]+)?$":
> +    type: object
> +
> +    required:
> +      - gpio-hog
> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio0: gpio@0 {
> +        compatible = "pin-control-gpio";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> +                      <&scmi_pinctrl 5 0 0>;
> +        gpio-ranges-group-names = "",
> +                                  "pinmux_gpio";
> +    };
> -- 
> 2.34.1
>
Linus Walleij Oct. 9, 2023, 7:49 a.m. UTC | #4
On Fri, Oct 6, 2023 at 3:23 PM Rob Herring <robh@kernel.org> wrote:
> On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:

> > A dt binding for pin controller based generic gpio driver is defined in
> > this commit. One usable device is Arm's SCMI.
>
> You don't need a "generic" binding to have a generic driver. Keep the
> binding specific and then decide in the OS to whether to use a generic
> or specific driver. That decision could change over time, but the
> binding can't. For example, see simple-panel.

What you say is true for simple-panel (a word like "simple" should
always cause red flags).

This case is more like mfd/syscon.yaml, where the singular
compatible = "syscon"; is in widespread use:

$ git grep 'compatible = \"syscon\";' |wc -l
50

I would accept adding a tuple compatible if you insist, so:

compatible = "foo-silicon", "pin-contro-gpio";

One case will be something like:

compatible = "optee-scmi-pin-control", "pin-control-gpio";

In this case I happen to know that we have the problem of
this being standardization work ahead of implementation on
actual hardware, and that is driven by the will known firmware
ambition to be completely abstract. It is supposed to sit on
top of pin control, or as part of pin control. Which leads me to
this thing (which I didn't think of before...)

> +    gpio0: gpio@0 {
> +        compatible = "pin-control-gpio";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> +                      <&scmi_pinctrl 5 0 0>;
> +        gpio-ranges-group-names = "",
> +                                  "pinmux_gpio";
> +    };

Maybe we should require that the pin-control-gpio node actually
be *inside* the pin control node, in this case whatever the label
&scmi_pinctrl is pointing to?

We can probably mandate that this has to be inside a pin controller
since it is a first.

Yours,
Linus Walleij
Cristian Marussi Oct. 9, 2023, 9:08 a.m. UTC | #5
On Mon, Oct 09, 2023 at 09:49:33AM +0200, Linus Walleij wrote:
> On Fri, Oct 6, 2023 at 3:23 PM Rob Herring <robh@kernel.org> wrote:
> > On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:
> 

Hi Linus and all,

> > > A dt binding for pin controller based generic gpio driver is defined in
> > > this commit. One usable device is Arm's SCMI.
> >
> > You don't need a "generic" binding to have a generic driver. Keep the
> > binding specific and then decide in the OS to whether to use a generic
> > or specific driver. That decision could change over time, but the
> > binding can't. For example, see simple-panel.
> 
> What you say is true for simple-panel (a word like "simple" should
> always cause red flags).
> 
> This case is more like mfd/syscon.yaml, where the singular
> compatible = "syscon"; is in widespread use:
> 
> $ git grep 'compatible = \"syscon\";' |wc -l
> 50
> 
> I would accept adding a tuple compatible if you insist, so:
> 
> compatible = "foo-silicon", "pin-contro-gpio";
> 
> One case will be something like:
> 
> compatible = "optee-scmi-pin-control", "pin-control-gpio";
> 
> In this case I happen to know that we have the problem of
> this being standardization work ahead of implementation on
> actual hardware, and that is driven by the will known firmware
> ambition to be completely abstract. It is supposed to sit on
> top of pin control, or as part of pin control. Which leads me to
> this thing (which I didn't think of before...)
> 
> > +    gpio0: gpio@0 {
> > +        compatible = "pin-control-gpio";
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > +                      <&scmi_pinctrl 5 0 0>;
> > +        gpio-ranges-group-names = "",
> > +                                  "pinmux_gpio";
> > +    };
> 

Assuming the above &scmi_pinctrl refers to the protocol node as we
usually do,  I am a bit puzzled by this example in this RFC series, because
usually in SCMI we DO refer to some resources using the phandle and the
domain IDs as in:

	scmi_sensor: protocol@15 {
		reg = <15>;
		#thermal-sensors-cells = <1>;
	};

	...

	thermal_zones {
		pmic {
			thermal-sensor = <&scmi_sensor 0>;
		};
	};
 
BUT in the SCMI Pinctrl case the current v4 Oleksii series takes advantage
of the existing Pinctrl bindings and naming to describe and refer to
pin/groups/functions, indeed #pinctrl-cells is defined as '0' in the
upcoming SCMI DT protocol node @19 in Oleksii v4, since indeed all the
parsing/matching is done by resource-names following the Picntrl
framework conventions. (AFAIU)

Moreover, due to how the SCMI Pinctrl protocol defines and describes the
pins/groups/functions using a tuple like (<TYPE>, <ID>) , with TYPE
being pin/group/function, a generic binding like the above would have to
be defined by at least 2 cells to be able to identify an SCMI PinCtrl
resource by index. (if that is the aim here...)

Am I right to think that such a generic SCMI PinCtrl binding is still to
be defined somewhere on the SCMI side, if needed as such by this GPIO driver ?

... or I am missing something ?

Thanks,
Cristian
Cristian Marussi Oct. 9, 2023, 3:08 p.m. UTC | #6
On Mon, Oct 09, 2023 at 03:13:24PM +0200, Linus Walleij wrote:
> On Mon, Oct 9, 2023 at 11:08 AM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> 
> > > > +    gpio0: gpio@0 {
> > > > +        compatible = "pin-control-gpio";
> > > > +        gpio-controller;
> > > > +        #gpio-cells = <2>;
> > > > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > > > +                      <&scmi_pinctrl 5 0 0>;
> > > > +        gpio-ranges-group-names = "",
> > > > +                                  "pinmux_gpio";
> > > > +    };
> > >
> >
> > Assuming the above &scmi_pinctrl refers to the protocol node as we
> > usually do,
> 
> No it does not, it is a three-layer cake.
> 
> scmi <-> scmi_pinctrl <-> scmi_gpio
> 
> it refers to the scmi_pinctrl node.
> 

Thanks, this explains a lot.
Cristian

> There is no SCMI GPIO protocol, instead SCMI is using the
> operations already available in the pin controller to exercise
> GPIO. Generic pin control has operations to drive lines for
> example, and Takahiro is adding the ability for a generic pin
> controller to also read a line.


> 
> Yours,
> Linus Walleij
AKASHI Takahiro Oct. 12, 2023, 1:15 a.m. UTC | #7
On Thu, Oct 05, 2023 at 09:48:09PM +0200, Krzysztof Kozlowski wrote:
> On 05/10/2023 04:58, AKASHI Takahiro wrote:
> > A dt binding for pin controller based generic gpio driver is defined in
> > this commit. One usable device is Arm's SCMI.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> > +
> > +required:
> > +  - compatible
> > +  - gpio-controller
> > +  - "#gpio-cells"
> > +  - gpio-ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    gpio0: gpio@0 {
> 
> No reg, so no unit address.

My intention was to allow for multiple nodes (instances) of
pinctrl based gpio devices. But I don't care the naming.

> Drop also unused label.

Okay, I already dropped an example consumer device and have no need
for the label any longer.

-Takahiro Akashi

> 
> > +        compatible = "pin-control-gpio";
> > +        gpio-controller;
> 
> Best regards,
> Krzysztof
>
Linus Walleij Oct. 12, 2023, 7:25 a.m. UTC | #8
On Tue, Oct 10, 2023 at 7:25 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> > We can probably mandate that this has to be inside a pin controller
> > since it is a first.
>
> Yeah, my U-Boot implementation tentatively supports both (inside and
> outside pin controller). But it is not a user's choice, but we should
> decide which way to go.

OK I have decided we are going to put it inside the pin control node,
as a subnode. (I don't expect anyone to object.)

It makes everything easier and clearer for users I think.

Yours,
Linus Walleij
Krzysztof Kozlowski Oct. 12, 2023, 7:27 a.m. UTC | #9
On 12/10/2023 03:15, AKASHI Takahiro wrote:

>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    gpio0: gpio@0 {
>>
>> No reg, so no unit address.
> 
> My intention was to allow for multiple nodes (instances) of
> pinctrl based gpio devices. But I don't care the naming.

How can you have unit address without reg? This causes warnings.

Best regards,
Krzysztof
AKASHI Takahiro Oct. 17, 2023, 2:32 a.m. UTC | #10
Hi Linus,

On Thu, Oct 12, 2023 at 09:25:20AM +0200, Linus Walleij wrote:
> On Tue, Oct 10, 2023 at 7:25???AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > > We can probably mandate that this has to be inside a pin controller
> > > since it is a first.
> >
> > Yeah, my U-Boot implementation tentatively supports both (inside and
> > outside pin controller). But it is not a user's choice, but we should
> > decide which way to go.
> 
> OK I have decided we are going to put it inside the pin control node,
> as a subnode. (I don't expect anyone to object.)

While I'm still thinking of how I can modify my current implementation
to fit into 'inside' syntax, there are a couple of concerns:

1) invoke gpiochip_add_data() at probe function
Probably we no longer need "compatible" property, but instead we need to
call gpiochip_add_data() explicitly in SCMI pin controller's probe
as follows:

scmi_pinctrl_probe()
    ...
    devm_pinctrl_register_and_init(dev, ..., pctrldev);
    pinctrl_enable(pctrldev);

    device_for_each_child_node(dev, fwnode)
        if (fwnode contains "gpio-controller") {
            /* what pin_control_gpio_probe() does */
            gc->get_direction = ...;
            ...
            devm_gpiochip_data_add(dev, gc, ...);
        }

2) gpio-by-pinctrl.c
While this file is SCMI-independent now, due to a change at (1),
it would be better to move the whole content inside SCMI pin controller
driver (because there is no other user for now).

3) Then, pin-control-gpio.yaml may also be put into SCMI binding
(i.e. firmware/arm,scmi.yaml). Can we leave the gpio binding outside?

4) phandle in "gpio-ranges" property
(As you mentioned)
The first element in a tuple of "gpio-ranges" is a phandle to a pin
controller node. Now that the gpio node is a sub node of pin controller,
the phandle is trivial. But there is no easier way to represent it
than using an explicit label:
(My U-Boot implementation does this.)

scmi {
    ...
    scmi_pinctrl: protocol@19 {
        ...
        gpio {
            gpio-controller;
            ...
            gpio-ranges = <&scmi_pinctrl ... >;
        }
    }
}

I tried:
    gpio-ranges = <0 ...>; // dtc passed, but '0' might be illegal by spec.
    gpio-ranges = <(-1) ...>; // dtc passed, but ...
    gpio-ranges = <&{..} ...>; // dtc error because it's not a full path.

Do you have any other idea? Otherwise, I will modify my RFC
with the changes above.

-Takahiro Akashi


> It makes everything easier and clearer for users I think.
> 
> Yours,
> Linus Walleij
Andy Shevchenko Oct. 19, 2023, 9:27 p.m. UTC | #11
Thu, Oct 05, 2023 at 11:58:38AM +0900, AKASHI Takahiro kirjoitti:
> This is a revised version of my previous RFC[1]. Although I modified
> the commits to make them look SCMI-independent, they are still posted
> as RFC because I have never tested them on real hardware.
> 
> (background)
> I'm currently working on implementing SCMI pinctrl/gpio drivers
> on U-Boot[2]. Although the pinctrl driver for the kernel[3] was submitted
> by EPAM, it doesn't contain the gpio driver and I believe that we should
> discuss a couple of points on the kernel side to finalize my design for
> U-Boot. 
> 
> So this RFC is intended for reviews, especially to raise some issues.
> 
> 1) how to obtain a value on an input pin
>    All the existing gpio drivers are set to obtain a value on an input
>    pin by accessing the hardware directly. In SCMI case, however, this is
>    just impossible in its nature and must be supported via a protocol
>    using "Input-value" configuration type. (See the spec[4], table-23.)
> 
>    The current pinconf framework is missing the feature (the pinconf
>    parameter and a helper function). See patch#1, #2 and #3.
> 
>    Please note that there is an issue around the pin configuration in
>    EPAM's current pinctrl driver as I commented[5].
> 
> 2) DT bindings
>    I would like to propose a generic binding for pinctrl based gpio driver.
>    This allows a "consumer" driver to handle gpio pins like as other
>    normal gpio controllers support. (patch#5)
> 
> 3) generic GPIO driver
>    Based on (2), I tried to prototype a generic driver in patch#4.
>    Thanks to a set of existing pinctrl_gpio helper functions, except (1),
>    It seems that the driver can be implemented not relying on pin controller
>    specific code, at least for SCMI pinctrl.
> 
> I will appreciate any comments.

Any comment here: I'm listed as a designated reviewer of GPIO patches, why am I
not Cc'ed on this? I definitely have some comments against the code (no DT,
though). Please, use (up-to-date) MAINTAINERS in your v3.
AKASHI Takahiro Oct. 20, 2023, 12:21 a.m. UTC | #12
Hi Andy,

On Fri, Oct 20, 2023 at 12:27:58AM +0300, andy.shevchenko@gmail.com wrote:
> Thu, Oct 05, 2023 at 11:58:38AM +0900, AKASHI Takahiro kirjoitti:
> > This is a revised version of my previous RFC[1]. Although I modified
> > the commits to make them look SCMI-independent, they are still posted
> > as RFC because I have never tested them on real hardware.
> > 
> > (background)
> > I'm currently working on implementing SCMI pinctrl/gpio drivers
> > on U-Boot[2]. Although the pinctrl driver for the kernel[3] was submitted
> > by EPAM, it doesn't contain the gpio driver and I believe that we should
> > discuss a couple of points on the kernel side to finalize my design for
> > U-Boot. 
> > 
> > So this RFC is intended for reviews, especially to raise some issues.
> > 
> > 1) how to obtain a value on an input pin
> >    All the existing gpio drivers are set to obtain a value on an input
> >    pin by accessing the hardware directly. In SCMI case, however, this is
> >    just impossible in its nature and must be supported via a protocol
> >    using "Input-value" configuration type. (See the spec[4], table-23.)
> > 
> >    The current pinconf framework is missing the feature (the pinconf
> >    parameter and a helper function). See patch#1, #2 and #3.
> > 
> >    Please note that there is an issue around the pin configuration in
> >    EPAM's current pinctrl driver as I commented[5].
> > 
> > 2) DT bindings
> >    I would like to propose a generic binding for pinctrl based gpio driver.
> >    This allows a "consumer" driver to handle gpio pins like as other
> >    normal gpio controllers support. (patch#5)
> > 
> > 3) generic GPIO driver
> >    Based on (2), I tried to prototype a generic driver in patch#4.
> >    Thanks to a set of existing pinctrl_gpio helper functions, except (1),
> >    It seems that the driver can be implemented not relying on pin controller
> >    specific code, at least for SCMI pinctrl.
> > 
> > I will appreciate any comments.
> 
> Any comment here: I'm listed as a designated reviewer of GPIO patches, why am I
> not Cc'ed on this?

My apologies. I will add you in Cc.

> I definitely have some comments against the code (no DT,
> though). Please, use (up-to-date) MAINTAINERS in your v3.

Please don't hesitate to make comments here on v2 so that I can
include your reviews in v3.

Thanks,
-Takahiro Akashi


> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Linus Walleij Oct. 23, 2023, 8:12 a.m. UTC | #13
Hi Takashi,

sorry for slow response :(

On Tue, Oct 17, 2023 at 4:32 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> > > > We can probably mandate that this has to be inside a pin controller
> > > > since it is a first.
> > >
> > > Yeah, my U-Boot implementation tentatively supports both (inside and
> > > outside pin controller). But it is not a user's choice, but we should
> > > decide which way to go.
> >
> > OK I have decided we are going to put it inside the pin control node,
> > as a subnode. (I don't expect anyone to object.)
>
> While I'm still thinking of how I can modify my current implementation
> to fit into 'inside' syntax, there are a couple of concerns:
>
> 1) invoke gpiochip_add_data() at probe function
> Probably we no longer need "compatible" property,

The DT binding people made it clear to me that they really
like compatibles for this kind of stuff so we should probably
keep it.

> but instead we need to
> call gpiochip_add_data() explicitly in SCMI pin controller's probe
> as follows:
>
> scmi_pinctrl_probe()
>     ...
>     devm_pinctrl_register_and_init(dev, ..., pctrldev);
>     pinctrl_enable(pctrldev);
>
>     device_for_each_child_node(dev, fwnode)
>         if (fwnode contains "gpio-controller") {
>             /* what pin_control_gpio_probe() does */
>             gc->get_direction = ...;
>             ...
>             devm_gpiochip_data_add(dev, gc, ...);
>         }

I think it is better of the pin controller just parse and add any
subdevices (GPIO or other) using of_platform_default_populate()
(just grep for this function and you will see how many device
drivers use that).

What is good with this approach is that if you place this call
last in the probe() we know the GPIO driver has all resources
it needs when it probes so it won't defer.

> 2) gpio-by-pinctrl.c
> While this file is SCMI-independent now, due to a change at (1),
> it would be better to move the whole content inside SCMI pin controller
> driver (because there is no other user for now).

That works, too. I have no strong opinion on this subject.

> 3) Then, pin-control-gpio.yaml may also be put into SCMI binding
> (i.e. firmware/arm,scmi.yaml). Can we leave the gpio binding outside?

There is no clear pattern whether to put subdevice bindings into
the parent device binding or not. Maybe? A lot of MFD devices does
this for sure.

> 4) phandle in "gpio-ranges" property
> (As you mentioned)
> The first element in a tuple of "gpio-ranges" is a phandle to a pin
> controller node. Now that the gpio node is a sub node of pin controller,
> the phandle is trivial. But there is no easier way to represent it
> than using an explicit label:
> (My U-Boot implementation does this.)
>
> scmi {
>     ...
>     scmi_pinctrl: protocol@19 {
>         ...
>         gpio {
>             gpio-controller;
>             ...
>             gpio-ranges = <&scmi_pinctrl ... >;
>         }
>     }
> }
>
> I tried:
>     gpio-ranges = <0 ...>; // dtc passed, but '0' might be illegal by spec.
>     gpio-ranges = <(-1) ...>; // dtc passed, but ...
>     gpio-ranges = <&{..} ...>; // dtc error because it's not a full path.
>
> Do you have any other idea? Otherwise, I will modify my RFC
> with the changes above.

If you have the GPIO node inside the pin controller node
and have all the details of the existing ranges available, there
is no need to put that into the device tree at all, just omit it?

Instead just call gpiochip_add_pin_range() directly in Linux
after adding the pin controller and gpio_chip.
C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
doing this. In this case the SX150X is hot-plugged (on a slow
bus) so it needs to figure out all ranges at runtime anyway.

Yours,
Linus Walleij
AKASHI Takahiro Oct. 24, 2023, 7:12 a.m. UTC | #14
Hi Linus,

On Mon, Oct 23, 2023 at 10:12:21AM +0200, Linus Walleij wrote:
> Hi Takashi,
> 
> sorry for slow response :(

Thank you for your feedback.

> On Tue, Oct 17, 2023 at 4:32???AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > > > > We can probably mandate that this has to be inside a pin controller
> > > > > since it is a first.
> > > >
> > > > Yeah, my U-Boot implementation tentatively supports both (inside and
> > > > outside pin controller). But it is not a user's choice, but we should
> > > > decide which way to go.
> > >
> > > OK I have decided we are going to put it inside the pin control node,
> > > as a subnode. (I don't expect anyone to object.)
> >
> > While I'm still thinking of how I can modify my current implementation
> > to fit into 'inside' syntax, there are a couple of concerns:
> >
> > 1) invoke gpiochip_add_data() at probe function
> > Probably we no longer need "compatible" property,
> 
> The DT binding people made it clear to me that they really
> like compatibles for this kind of stuff so we should probably
> keep it.

Okay, I will assume this in the following discussion.

> > but instead we need to
> > call gpiochip_add_data() explicitly in SCMI pin controller's probe
> > as follows:
> >
> > scmi_pinctrl_probe()
> >     ...
> >     devm_pinctrl_register_and_init(dev, ..., pctrldev);
> >     pinctrl_enable(pctrldev);
> >
> >     device_for_each_child_node(dev, fwnode)
> >         if (fwnode contains "gpio-controller") {
> >             /* what pin_control_gpio_probe() does */
> >             gc->get_direction = ...;
> >             ...
> >             devm_gpiochip_data_add(dev, gc, ...);
> >         }
> 
> I think it is better of the pin controller just parse and add any
> subdevices (GPIO or other) using of_platform_default_populate()
> (just grep for this function and you will see how many device
> drivers use that).

IICU, then, we will have to add a "compatible" to pinctrl node
to make of_platform_default_populate() work as expected. That is:

scmi {
    ...
    protocol@19 {
        compatible = "simple-bus"; // <- added
        reg = <0x19>;

        ... // a couple of pinconf nodes

        scmi_gpio {
            compatible = "pin-control-gpio";
            ...
        }
    }
}

Is this what you meant?
In this case, however, "protocol@19" has a mixture of sub-nodes,
most are pinconf definitions which are the properties of the pin
controller, while "scmi_gpio" is a separate device.

The code will work, but is it sane from DT binding pov?

> What is good with this approach is that if you place this call
> last in the probe() we know the GPIO driver has all resources
> it needs when it probes so it won't defer.
> 
> > 2) gpio-by-pinctrl.c
> > While this file is SCMI-independent now, due to a change at (1),
> > it would be better to move the whole content inside SCMI pin controller
> > driver (because there is no other user for now).
> 
> That works, too. I have no strong opinion on this subject.

I assumed that "compatible" had been removed here.
If we retain "compatible" property, I don't care either way.

> > 3) Then, pin-control-gpio.yaml may also be put into SCMI binding
> > (i.e. firmware/arm,scmi.yaml). Can we leave the gpio binding outside?
> 
> There is no clear pattern whether to put subdevice bindings into
> the parent device binding or not. Maybe? A lot of MFD devices does
> this for sure.

The same as above.

> > 4) phandle in "gpio-ranges" property
> > (As you mentioned)
> > The first element in a tuple of "gpio-ranges" is a phandle to a pin
> > controller node. Now that the gpio node is a sub node of pin controller,
> > the phandle is trivial. But there is no easier way to represent it
> > than using an explicit label:
> > (My U-Boot implementation does this.)
> >
> > scmi {
> >     ...
> >     scmi_pinctrl: protocol@19 {
> >         ...
> >         gpio {
> >             gpio-controller;
> >             ...
> >             gpio-ranges = <&scmi_pinctrl ... >;
> >         }
> >     }
> > }
> >
> > I tried:
> >     gpio-ranges = <0 ...>; // dtc passed, but '0' might be illegal by spec.
> >     gpio-ranges = <(-1) ...>; // dtc passed, but ...
> >     gpio-ranges = <&{..} ...>; // dtc error because it's not a full path.
> >
> > Do you have any other idea? Otherwise, I will modify my RFC
> > with the changes above.
> 
> If you have the GPIO node inside the pin controller node
> and have all the details of the existing ranges available, there
> is no need to put that into the device tree at all, just omit it?

Then, of_gpiochip_add_data() (hence, of_gpiochip_add()/gpiochip_add_data())
won't work because it always assume phandle + 3 arguments. Right?

In this case, "gpio-ranges" here may have different semantics from
the other pinctrl-based gpio drivers. Doesn't matter from DT pov?

> Instead just call gpiochip_add_pin_range() directly in Linux
> after adding the pin controller and gpio_chip.
> C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
> doing this. In this case the SX150X is hot-plugged (on a slow
> bus) so it needs to figure out all ranges at runtime anyway.

Are you suggesting implementing a custom function for parsing "gpio-ranges"
and calling it in pin_control_gpio_probe() instead of a generic helper?

Or do you want to always map all the pin controller's pins to
gpio pins as sx150x does?

-Takahiro Akashi

> Yours,
> Linus Walleij
Linus Walleij Oct. 24, 2023, 9:40 a.m. UTC | #15
Hi Takahiro,

On Tue, Oct 24, 2023 at 9:12 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> > I think it is better of the pin controller just parse and add any
> > subdevices (GPIO or other) using of_platform_default_populate()
> > (just grep for this function and you will see how many device
> > drivers use that).
>
> IICU, then, we will have to add a "compatible" to pinctrl node
> to make of_platform_default_populate() work as expected. That is:
>
> scmi {
>     ...
>     protocol@19 {
>         compatible = "simple-bus"; // <- added

Hm right, but you could also use
of_platform_populate(np, NULL, NULL, dev);

Then the compatible match is of no concern.

Sorry for my lack of attention to details :/

If you want to restrict the population to a few select compatibles
(maybe only "pin-control-gpio") then you can do
that with

const struct of_device_id of_scmi_protocol_19_match_table[] = {
        { .compatible = "pin-control-gpio", },
        {}
};
of_platform_populate(np, of_scmi_protocol_19_match_table, NULL, dev);

> Is this what you meant?
> In this case, however, "protocol@19" has a mixture of sub-nodes,
> most are pinconf definitions which are the properties of the pin
> controller, while "scmi_gpio" is a separate device.

That looks good to me, it makes sense to have the GPIO as a subnode
here and mandate it with a compatible to match.

> The code will work, but is it sane from DT binding pov?

Let's let the DT people jump in on that.

> > Instead just call gpiochip_add_pin_range() directly in Linux
> > after adding the pin controller and gpio_chip.
> > C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
> > doing this. In this case the SX150X is hot-plugged (on a slow
> > bus) so it needs to figure out all ranges at runtime anyway.
>
> Are you suggesting implementing a custom function for parsing "gpio-ranges"
> and calling it in pin_control_gpio_probe() instead of a generic helper?

The generic helper will always be attempted but if there are
no ranges in the device tree, it will just continue without adding
any ranges. I suggest putting *no* ranges into the device tree.

> Or do you want to always map all the pin controller's pins to
> gpio pins as sx150x does?

I think since the SCMI firmware knows about the available line
and pins etc, it makes sense that the driver comes up with the
applicable ranges on its own (derived from the information from
the SCMI firmware) and add them, instead of trying to put that
information into the device tree at all. Just omit it, and make your
own ranges, and add them in the Linux driver with
gpiochip_add_pin_range() without involving DT at all when defining
the ranges.

I'm sorry if I'm unclear sometimes.

Yours,
Linus Walleij
Cristian Marussi Oct. 24, 2023, 10:55 a.m. UTC | #16
On Tue, Oct 24, 2023 at 11:40:00AM +0200, Linus Walleij wrote:
> Hi Takahiro,
> 

Hi,

> On Tue, Oct 24, 2023 at 9:12 AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > > I think it is better of the pin controller just parse and add any
> > > subdevices (GPIO or other) using of_platform_default_populate()
> > > (just grep for this function and you will see how many device
> > > drivers use that).
> >
> > IICU, then, we will have to add a "compatible" to pinctrl node
> > to make of_platform_default_populate() work as expected. That is:
> >
> > scmi {
> >     ...
> >     protocol@19 {
> >         compatible = "simple-bus"; // <- added
> 
> Hm right, but you could also use
> of_platform_populate(np, NULL, NULL, dev);
> 
> Then the compatible match is of no concern.
> 
> Sorry for my lack of attention to details :/
> 
> If you want to restrict the population to a few select compatibles
> (maybe only "pin-control-gpio") then you can do
> that with
> 
> const struct of_device_id of_scmi_protocol_19_match_table[] = {
>         { .compatible = "pin-control-gpio", },
>         {}
> };
> of_platform_populate(np, of_scmi_protocol_19_match_table, NULL, dev);
> 
> > Is this what you meant?
> > In this case, however, "protocol@19" has a mixture of sub-nodes,
> > most are pinconf definitions which are the properties of the pin
> > controller, while "scmi_gpio" is a separate device.
> 
> That looks good to me, it makes sense to have the GPIO as a subnode
> here and mandate it with a compatible to match.
> 
> > The code will work, but is it sane from DT binding pov?
> 
> Let's let the DT people jump in on that.
> 
> > > Instead just call gpiochip_add_pin_range() directly in Linux
> > > after adding the pin controller and gpio_chip.
> > > C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
> > > doing this. In this case the SX150X is hot-plugged (on a slow
> > > bus) so it needs to figure out all ranges at runtime anyway.
> >
> > Are you suggesting implementing a custom function for parsing "gpio-ranges"
> > and calling it in pin_control_gpio_probe() instead of a generic helper?
> 
> The generic helper will always be attempted but if there are
> no ranges in the device tree, it will just continue without adding
> any ranges. I suggest putting *no* ranges into the device tree.
> 
> > Or do you want to always map all the pin controller's pins to
> > gpio pins as sx150x does?
> 
> I think since the SCMI firmware knows about the available line
> and pins etc, it makes sense that the driver comes up with the
> applicable ranges on its own (derived from the information froms
> the SCMI firmware) and add them, instead of trying to put that
> information into the device tree at all. Just omit it, and make your
> own ranges, and add them in the Linux driver with
> gpiochip_add_pin_range() without involving DT at all when defining
> the ranges.
> 
> I'm sorry if I'm unclear sometimes.

...a maybe dumb question from my side, BUT does the SCMI Pinctrl carry
enough information as it stands for the driver to derive autonomously
and efficently the possible/applicable gpio ranges ?

Are they (GPIOs) all the remaining unassociated pins ?
If this is the case note that the SCMI Pinctrl lets you query the
associations in groups or functions and this is generally now done
only lazily on-demand when specific pins/groups/funcs are requested
by the parsed DT confs: IOW, in order to derive GPIOs from the set
of unassociated ones, you will have to at first add some new full-lookup
pinctrl_ops to query upfront all existing associations (avoiding, at will,
the lazy querying adopted now) and then singling-out the non-associated
ones from the lists of all possible group/funcs associations.

Moreover, should we allow anyway the optional possibility to forcibly
restrict the available gpios from the DT, or we can just assume that
those un-available (map out as above) wont just be exposed by the
SCMI server ?

..again sorry if I am missing something crucial here and just talking
non-sense but I have limited familiarity with Pinctrl/GPIOs usage.

Thanks
Cristian
AKASHI Takahiro Oct. 24, 2023, 11:09 a.m. UTC | #17
Hi Linus,

On Tue, Oct 24, 2023 at 11:40:00AM +0200, Linus Walleij wrote:
> Hi Takahiro,
> 
> On Tue, Oct 24, 2023 at 9:12???AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > > I think it is better of the pin controller just parse and add any
> > > subdevices (GPIO or other) using of_platform_default_populate()
> > > (just grep for this function and you will see how many device
> > > drivers use that).
> >
> > IICU, then, we will have to add a "compatible" to pinctrl node
> > to make of_platform_default_populate() work as expected. That is:
> >
> > scmi {
> >     ...
> >     protocol@19 {
> >         compatible = "simple-bus"; // <- added
> 
> Hm right, but you could also use
> of_platform_populate(np, NULL, NULL, dev);
> 
> Then the compatible match is of no concern.
> 
> Sorry for my lack of attention to details :/
> 
> If you want to restrict the population to a few select compatibles
> (maybe only "pin-control-gpio") then you can do
> that with
> 
> const struct of_device_id of_scmi_protocol_19_match_table[] = {
>         { .compatible = "pin-control-gpio", },
>         {}
> };
> of_platform_populate(np, of_scmi_protocol_19_match_table, NULL, dev);
> 
> > Is this what you meant?
> > In this case, however, "protocol@19" has a mixture of sub-nodes,
> > most are pinconf definitions which are the properties of the pin
> > controller, while "scmi_gpio" is a separate device.
> 
> That looks good to me, it makes sense to have the GPIO as a subnode
> here and mandate it with a compatible to match.
> 
> > The code will work, but is it sane from DT binding pov?
> 
> Let's let the DT people jump in on that.
> 
> > > Instead just call gpiochip_add_pin_range() directly in Linux
> > > after adding the pin controller and gpio_chip.
> > > C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
> > > doing this. In this case the SX150X is hot-plugged (on a slow
> > > bus) so it needs to figure out all ranges at runtime anyway.
> >
> > Are you suggesting implementing a custom function for parsing "gpio-ranges"
> > and calling it in pin_control_gpio_probe() instead of a generic helper?
> 
> The generic helper will always be attempted but if there are
> no ranges in the device tree, it will just continue without adding
> any ranges. I suggest putting *no* ranges into the device tree.
> 
> > Or do you want to always map all the pin controller's pins to
> > gpio pins as sx150x does?
> 
> I think since the SCMI firmware knows about the available line
> and pins etc, it makes sense that the driver comes up with the
> applicable ranges on its own (derived from the information from
> the SCMI firmware) and add them, instead of trying to put that
> information into the device tree at all. Just omit it, and make your
> own ranges, and add them in the Linux driver with
> gpiochip_add_pin_range() without involving DT at all when defining
> the ranges.

As far as I understand, there is only one way for SCMI gpio driver
to know which pins are actually GPIO pins; Calling PINCNTRL_CONFIG_GET
command with "Input-mode" or "Output-mode" configuration type
against *every* pin-controller's pins.
(Here I assume that the command would fail with INVALID_PARAMETERS or
NOT_SUPPORTED if configuring the given pin as a GPIO input or output
is not possible. But the specification seems to be a bit ambiguous.)

It means that, if SCMI firmware has 100 pinctrl pins, the driver needs
to call the command 200 times in order to get the answer.

It is possible but I believe that it is clunky and painful for the driver
initialization.
I'd like to see explicit "gpio-ranges" property in a device tree.

Thanks,
-Takahiro Akashi



> I'm sorry if I'm unclear sometimes.
> 
> Yours,
> Linus Walleij
Linus Walleij Oct. 24, 2023, 1:01 p.m. UTC | #18
On Tue, Oct 24, 2023 at 12:55 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:

> ...a maybe dumb question from my side, BUT does the SCMI Pinctrl carry
> enough information as it stands for the driver to derive autonomously
> and efficently the possible/applicable gpio ranges ?

I don't know, that's part of the problem I suppose. But if the
pin controller can report functions supported by certain pins
or groups of pins, then certainly "gpio" should be one of those
functions or else the pin cannot be used for GPIO at all?

Then maybe that function is just a name convention, such
as "all pins are members of a 1-pin group named 'gpioN'
where N is the pin number" then you need to switch the pin
into this function in order to use the pin as a GPIO line.
Pins that do not have this group associated with them
cannot be used for GPIO.

This is incidentally exactly the method used by the Qualcomm
pin control driver (IIRC).

If the SCMI protocol has not though about GPIO as a special
function, or mentioned anything about group name
conventions for the GPIO function, then there is a hole
in the specification, and this is likely best filled by creating
one-pin groups as per above and feed this back to the
spec.

If the GPIO usecase isn't even considered a function by SCMI,
or (more likely) "nobody thought about that" then this is
a good time to send it back to the drawing board for
specification, right? It's normal for specs to run into a bit
of friction when confronted with the real world.

Yours,
Linus Walleij
Linus Walleij Oct. 24, 2023, 1:12 p.m. UTC | #19
On Tue, Oct 24, 2023 at 1:10 PM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> As far as I understand, there is only one way for SCMI gpio driver
> to know which pins are actually GPIO pins; Calling PINCNTRL_CONFIG_GET
> command with "Input-mode" or "Output-mode" configuration type
> against *every* pin-controller's pins.
> (Here I assume that the command would fail with INVALID_PARAMETERS or
> NOT_SUPPORTED if configuring the given pin as a GPIO input or output
> is not possible. But the specification seems to be a bit ambiguous.)

As I also wrote in the reply to Christian, I expect the SCMI firmware
to consider GPIO a function on the pins, and either individual pins
(groups with just one pin in it) or entire groups of pins can be switched
to perform the "gpio function". ("gpio function" is akin to "i2c function"
or "HDMI function" etc.)

If the SCMI protocol considers GPIO usage to be something else
than a function of a pin, that is going to be a problem. Then the SCMI
protocol need some other way of determining that the pin is in
GPIO mode, and perhaps something would need to be added to
the protocol for that.

The reason is that in practice a lot of hardware has to decouple
the pin from any internal function in order to use it as GPIO, and
connect it to the GPIO block that can drive the line high and low.
And we don't select that as a function, how is the firmware going
to know that it needs to do this? Implicitly from the call requesting
the line to be output perhaps. But if the firmware can be altered
to do that, the firmware can just as well be altered to present
GPIO as a proper function.

Using a function makes most sense, because the board firmware
knows which pins are GPIO lines and what they are used for
(such as a LED or camera flash) and at boot certainly put them
into GPIO function and drive them high/low or set them as
input (high impedance).

> It means that, if SCMI firmware has 100 pinctrl pins, the driver needs
> to call the command 200 times in order to get the answer.

I think we should only need to check which function each pin
has and those that are in the GPIO function we put into the ranges.

Yours,
Linus Walleij
AKASHI Takahiro Oct. 24, 2023, 1:42 p.m. UTC | #20
On Tue, Oct 24, 2023 at 03:12:51PM +0200, Linus Walleij wrote:
> On Tue, Oct 24, 2023 at 1:10???PM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > As far as I understand, there is only one way for SCMI gpio driver
> > to know which pins are actually GPIO pins; Calling PINCNTRL_CONFIG_GET
> > command with "Input-mode" or "Output-mode" configuration type
> > against *every* pin-controller's pins.
> > (Here I assume that the command would fail with INVALID_PARAMETERS or
> > NOT_SUPPORTED if configuring the given pin as a GPIO input or output
> > is not possible. But the specification seems to be a bit ambiguous.)
> 
> As I also wrote in the reply to Christian, I expect the SCMI firmware
> to consider GPIO a function on the pins, and either individual pins
> (groups with just one pin in it) or entire groups of pins can be switched
> to perform the "gpio function". ("gpio function" is akin to "i2c function"
> or "HDMI function" etc.)

First of all, there is no pre-defined naming convention either for
pins, groups or functions. SCMI firmware can give them any names.

Secondly, What you said in the above is already implemented in
my RFC patch. Please remember the example that I gave:

>     gpio-ranges = <&scmi_pinctrl 6 0 0>;
>     gpio-ranges-group-names = "pinmux_gpio";
> 
> means that SCMI *group*, "pinmux_gpio", are mapped to this driver's
> gpio range which starts with 5. If "pinmux_gpio" indicates SCMI *pin*
> range [20..24],
> 
>     baa-gpios = <&gpio0 7>;
> will refer to gpio pin#7 that is actually SCMI's 22 (=20 + (7-5)).

Given the fact there is no naming convention, we need to explicitly
specify an associated group name in "gpio-ranges-group-names" in any way.

What my driver doesn't care for now is the case where a group of GPIO pins
are multiplexed with other functions (UART, I2C or whatever else).
In this case, we need to configure "pinconf" setup prior to using those
pins as GPIO anyway. Simply, it is out of scope of my driver.
(We can still use existing generic GPIO interfaces to operate them once
set up, though.)

After all, I still believe we need "gpio-ranges" property in most of
all use cases (The only exception is, as I mentioned, to unconditionally
map all pinctrl's pins to GPIO (if possible) when SCMI firmware provides
only GPIO function for all pins. I think it is a simple and yet likely
use case.

Thanks,
-Takahiro Akashi


> 
> If the SCMI protocol considers GPIO usage to be something else
> than a function of a pin, that is going to be a problem. Then the SCMI
> protocol need some other way of determining that the pin is in
> GPIO mode, and perhaps something would need to be added to
> the protocol for that.
> 
> The reason is that in practice a lot of hardware has to decouple
> the pin from any internal function in order to use it as GPIO, and
> connect it to the GPIO block that can drive the line high and low.
> And we don't select that as a function, how is the firmware going
> to know that it needs to do this? Implicitly from the call requesting
> the line to be output perhaps. But if the firmware can be altered
> to do that, the firmware can just as well be altered to present
> GPIO as a proper function.
> 
> Using a function makes most sense, because the board firmware
> knows which pins are GPIO lines and what they are used for
> (such as a LED or camera flash) and at boot certainly put them
> into GPIO function and drive them high/low or set them as
> input (high impedance).
> 
> > It means that, if SCMI firmware has 100 pinctrl pins, the driver needs
> > to call the command 200 times in order to get the answer.
> 
> I think we should only need to check which function each pin
> has and those that are in the GPIO function we put into the ranges.
> 
> Yours,
> Linus Walleij
Linus Walleij Nov. 5, 2023, 10:15 p.m. UTC | #21
Hi Takahiro,

On Tue, Oct 24, 2023 at 3:43 PM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> First of all, there is no pre-defined naming convention either for
> pins, groups or functions. SCMI firmware can give them any names.

OK maybe that should be added to the spec?

[NB: I poked the pinctrl implementers in a separate mail, you
are on CC.]

Otherwise I think this is one of those cases where firmware
authors will simply start to use a certain naming convention if
the Linux driver requires it.

> Secondly, What you said in the above is already implemented in
> my RFC patch. Please remember the example that I gave:
>
> >     gpio-ranges = <&scmi_pinctrl 6 0 0>;
> >     gpio-ranges-group-names = "pinmux_gpio";
> >
> > means that SCMI *group*, "pinmux_gpio", are mapped to this driver's
> > gpio range which starts with 5. If "pinmux_gpio" indicates SCMI *pin*
> > range [20..24],
> >
> >     baa-gpios = <&gpio0 7>;
> > will refer to gpio pin#7 that is actually SCMI's 22 (=20 + (7-5)).

Right! I am so unused to the gpio-ranges-group-names that
I didn't parse that properly :(

> After all, I still believe we need "gpio-ranges" property in most of
> all use cases (The only exception is, as I mentioned, to unconditionally
> map all pinctrl's pins to GPIO (if possible) when SCMI firmware provides
> only GPIO function for all pins. I think it is a simple and yet likely
> use case.

I suppose it is a bit of placement question.

The device tree GPIO ranges will have to duplicate more information
that the SCMI firmware already knows (what ranges are GPIOs, the
name of the GPIO mux function), that is my main concern.
And when we have information in two places that need to be matched,
invariably we get mismatches.

I'm trying to figure out what is the best way forward here but I think
we need some feedback from the pinctrl driver authors.

Yours,
Linus Walleij