| Message ID | 20231005025843.508689-6-takahiro.akashi@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | gpio: add pinctrl based generic gpio driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
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
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.
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 >
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
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
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. 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
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
On Mon, Oct 09, 2023 at 04:08:13PM +0100, Cristian Marussi wrote: > 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 Just in case, gpio-ranges = <&scmi_pinctrl 0 10 5>; means that SCMI *pin* range [10..(10+5-1)] are mapped to this driver's gpio range [0..(5-1)]. So any consumer driver can access a gpio pin as: foo-gpios = <&gpio0 3>; will refer to gpio pin#3 that is actually SCMI's 13. gpio-ranges = <&scmi_pinctrl 5 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)). This way, we (consumer drivers) don't care what is the underlying pin controller. -Takahiro Akashi > > > 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
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: > > > > 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? null (or '_' as dummy) if the dt schema allows such a value as a trivial case? > 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. Thanks, -Takahiro Akashi > Yours, > Linus Walleij
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 >
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
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
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
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
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
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
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
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
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
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
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
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
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"; + };
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