| Message ID | 20221213224310.543243-2-fabrizio.castro.jz@renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Driver support for RZ/V2M PWC | expand |
On 14/12/2022 19:26, Fabrizio Castro wrote: > Hi Rob, > > Thanks for your feedback! > >> From: Rob Herring <robh@kernel.org> >> Sent: 14 December 2022 16:11 >> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com> >> 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@renesas.com> >>> --- >>> .../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@renesas.com> >>> + >>> +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? These are generic drivers, so they need offset as they do not match any specific programming model. You are not making a generic device. Address offsets are not suitable in most cases for DTS. There are of course exceptions so you can present reasons why this one is exception. > >> >>> + $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? We do not want regmap, but proper definition of entire hardware. > >> 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). Driver do not matter for bindings. We talk about regmap field which - as you explained above - is not needed. > > 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: I don't understand. You do not have here simple-mfd and it still does not explain Rob's comment and regmap. > 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>; You speak about two different things. So again - drop regmap. You do not need it. > offset = <0x80>; > gpio-controller; > #gpio-cells = <2>; > }; > > poweroff { > compatible = "renesas,r9a09g011-pwc-poweroff", > "renesas,rzv2m-pwc-poweroff"; > regmap = <&pwc>; Drop regmap. > }; > }; > Best regards, Krzysztof
Hi Rob, Thanks for the feedback. > From: Rob Herring <robh@kernel.org> > Sent: 14 December 2022 16:11 > To: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > 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. Right, I'll be more careful next time. > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > --- > > .../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 > > +--- > > + > > +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@renesas.com> > > + > > +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. I'll take it out > > > + $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. 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. I'll take out this child node. 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 > > > >
Hi Krzysztof, Thanks for your feedback. > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: 15 December 2022 09:49 > To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Rob Herring > <robh@kernel.org> > Subject: Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver > bindings > > On 14/12/2022 19:26, Fabrizio Castro wrote: > > Hi Rob, > > > > Thanks for your feedback! > > > >> From: Rob Herring <robh@kernel.org> > >> Sent: 14 December 2022 16:11 > >> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > >> 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@renesas.com> > >>> --- > >>> .../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 > >>> +--- > >>> + > >>> +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@renesas.com> > >>> + > >>> +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: > > > > > > What am I missing? > > These are generic drivers, so they need offset as they do not match any > specific programming model. You are not making a generic device. Address > offsets are not suitable in most cases for DTS. There are of course > exceptions so you can present reasons why this one is exception. Thanks for the explanation > > > >> > >>> + $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? > > We do not want regmap, but proper definition of entire hardware. Will do. I'll drop regmap. > > > > >> 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). > > Driver do not matter for bindings. We talk about regmap field which - as > you explained above - is not needed. Okay, I'll rework, and I'll send v2. Thanks, Fab > > > > > > 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: > > I don't understand. You do not have here simple-mfd and it still does > not explain Rob's comment and regmap. > > > > > 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>; > > You speak about two different things. So again - drop regmap. You do not > need it. > > > offset = <0x80>; > > gpio-controller; > > #gpio-cells = <2>; > > }; > > > > poweroff { > > compatible = "renesas,r9a09g011-pwc-poweroff", > > "renesas,rzv2m-pwc-poweroff"; > > regmap = <&pwc>; > > Drop regmap. > > > }; > > }; > > > > Best regards, > Krzysztof
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: http://devicetree.org/schemas/gpio/renesas,rzv2m-pwc-gpio.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +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@renesas.com> + +properties: + compatible: + items: + - enum: + - renesas,r9a09g011-pwc-gpio # RZ/V2M + - renesas,r9a09g055-pwc-gpio # RZ/V2MA + - const: renesas,rzv2m-pwc-gpio + + offset: + $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. + + 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>; + };
Add dt-bindings document for the RZ/V2M PWC GPIO driver. Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> --- .../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