RE: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver bindings
From: Fabrizio Castro
Date: Wed Dec 14 2022 - 13:27:04 EST
Hi Rob,
Thanks for your feedback!
> From: Rob Herring <robh@xxxxxxxxxx>
> Sent: 14 December 2022 16:11
> To: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> Subject: Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver
> bindings
>
> On Tue, Dec 13, 2022 at 10:43:06PM +0000, Fabrizio Castro wrote:
> > Add dt-bindings document for the RZ/V2M PWC GPIO driver.
>
> Bindings are for h/w blocks/devices, not a specific driver.
Apologies, I will reword the changelog in v2.
>
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > ---
> > .../bindings/gpio/renesas,rzv2m-pwc-gpio.yaml | 62 +++++++++++++++++++
> > 1 file changed, 62 insertions(+)
> > create mode 100644
> Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
> gpio.yaml b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
> gpio.yaml
> > new file mode 100644
> > index 000000000000..ecc034d53259
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
> > @@ -0,0 +1,62 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre
> e.org%2Fschemas%2Fgpio%2Frenesas%2Crzv2m-pwc-
> gpio.yaml%23&data=05%7C01%7Cfabrizio.castro.jz%40renesas.com%7C603623c
> 766f4421b85bd08daddedcb8c%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638
> 066310628408926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz
> IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=o46ncDZK8YK5HYJ
> ZYDXuq3yfEA34vnaxEsIDBlcroc0%3D&reserved=0
> > +$schema:
> https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre
> e.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C01%7Cfabrizio.castro.jz%40renesas.com
> %7C603623c766f4421b85bd08daddedcb8c%7C53d82571da1947e49cb4625a166a4a2a%7C0
> %7C0%7C638066310628408926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=VoWvV
> pW782DVH2zdTKIesyzqm6sjiFyacbl833%2BjRis%3D&reserved=0
> > +
> > +title: Renesas RZ/V2M External Power Sequence Controller (PWC) GPIO
> > +
> > +description: |+
> > + The PWC IP found in the RZ/V2M family of chips comes with General-
> Purpose
> > + Output pins, alongside the below functions
> > + - external power supply on/off sequence generation
> > + - on/off signal generation for the LPDDR4 core power supply (LPVDD)
> > + - key input signals processing
> > + This node uses syscon to map the register used to control the GPIOs
> > + (the register map is retrieved from the parent dt-node), and the node
> should
> > + be represented as a sub node of a "syscon", "simple-mfd" node.
> > +
> > +maintainers:
> > + - Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - renesas,r9a09g011-pwc-gpio # RZ/V2M
> > + - renesas,r9a09g055-pwc-gpio # RZ/V2MA
> > + - const: renesas,rzv2m-pwc-gpio
> > +
> > + offset:
>
> Too generic of a name. We want any given property name (globally) to
> have 1 type. With the below comment, this should be replaced with 'reg'
> instead if you have child nodes.
My understanding is that syscon subnodes normally use this name for exactly
the same purpose, for example:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/power/reset/syscon-poweroff.yaml#L27
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml#L30
What am I missing?
>
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: |
> > + Offset in the register map for controlling the GPIOs (in bytes).
> > +
> > + regmap:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: Phandle to the register map node.
>
> Looks like GPIO is a sub-function of some other block. Define the
> binding for that entire block.
That's defined in patch 3 from this series.
I have sent it as patch 3 because that document references:
* /schemas/gpio/renesas,rzv2m-pwc-gpio.yaml
* /schemas/power/reset/renesas,rzv2m-pwc-poweroff.yaml
Which are defined in this patch and in patch 2 in the series.
Do you want me to move patch 3 to patch 1 in v2?
> GPIO can be either either a function of
> that node (just add GPIO provider properties) or you can have GPIO child
> nodes. Depends on what the entire block looks like to decide. Do you
> have multiple instances of the GPIO block would be one reason to have
> child nodes.
>From a pure HW point of view, this GPIO block is contained inside the PWC block
(as PWC is basically a MFD device), and it only deals with 2 General-Purpose
Output pins, both controlled by 1 (and the same) register, therefore the GPIO
block is only 1 child.
If possible, I would like to keep the functionality offered by the sub-blocks of
PWC contained in separated drivers and DT nodes (either non-child nodes or child
nodes).
My understanding is that simple-mfd will automatically probe the children of the
simple-mfd node, and also hierarchically it makes sense to me to have the DT nodes
of the PWC sub-blocks as children of the "syscon", "simple-mfd" node. I have found
other instances of this same architecture in the kernel already (plenty from NXP/Freescale),
for example:
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mm.dtsi#L585
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mn.dtsi#L586
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mp.dtsi#L451
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mq.dtsi#L616
https://github.com/torvalds/linux/blob/master/arch/mips/boot/dts/mti/sead3.dts#L93
etc...
Something like the below could also work, but I don't think it would represent the
HW accurately:
pwc: pwc@a3700000 {
compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc",
"syscon", "simple-mfd";
reg = <0 0xa3700000 0 0x800>;
};
pwc-gpio {
compatible = "renesas,r9a09g011-pwc-gpio",
"renesas,rzv2m-pwc-gpio";
regmap = <&pwc>;
gpio-controller;
#gpio-cells = <2>;
};
pwc-poweroff {
compatible = "renesas,r9a09g011-pwc-poweroff",
"renesas,rzv2m-pwc-poweroff";
regmap = <&pwc>;
};
I think the below describes things better:
pwc: pwc@a3700000 {
compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc",
"syscon", "simple-mfd";
reg = <0 0xa3700000 0 0x800>;
gpio {
compatible = "renesas,r9a09g011-pwc-gpio",
"renesas,rzv2m-pwc-gpio";
regmap = <&pwc>;
offset = <0x80>;
gpio-controller;
#gpio-cells = <2>;
};
poweroff {
compatible = "renesas,r9a09g011-pwc-poweroff",
"renesas,rzv2m-pwc-poweroff";
regmap = <&pwc>;
};
};
Do you think the bindings I have sent out are causing confusion here?
What else can I improve?
Thanks,
Fab
>
> > +
> > + gpio-controller: true
> > +
> > + '#gpio-cells':
> > + const: 2
> > +
> > +required:
> > + - compatible
> > + - regmap
> > + - offset
> > + - gpio-controller
> > + - '#gpio-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + gpio {
> > + compatible = "renesas,r9a09g011-pwc-gpio",
> > + "renesas,rzv2m-pwc-gpio";
> > + regmap = <®mapnode>;
> > + offset = <0x80>;
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + };
> > --
> > 2.34.1
> >
> >