2022-07-14 12:30:19

by Tomer Maimon

[permalink] [raw]
Subject: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX
pinmux and GPIO controller.

Signed-off-by: Tomer Maimon <[email protected]>
---
.../pinctrl/nuvoton,npcm845-pinctrl.yaml | 213 ++++++++++++++++++
1 file changed, 213 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
new file mode 100644
index 000000000000..104766f7acc5
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
@@ -0,0 +1,213 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NPCM845 Pin Controller and GPIO
+
+maintainers:
+ - Tomer Maimon <[email protected]>
+
+description:
+ The Nuvoton BMC NPCM8XX Pin Controller multi-function routed through
+ the multiplexing block, Each pin supports GPIO functionality (GPIOx)
+ and multiple functions that directly connect the pin to different
+ hardware blocks.
+
+properties:
+ compatible:
+ const: nuvoton,npcm845-pinctrl
+
+ ranges:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 1
+
+ nuvoton,sysgcr:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: a phandle to access GCR registers.
+
+patternProperties:
+ "^gpio@":
+ type: object
+
+ description:
+ Eight GPIO banks that each contain between 32 GPIOs.
+
+ properties:
+ gpio-controller: true
+
+ '#gpio-cells':
+ const: 2
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ gpio-ranges:
+ maxItems: 1
+
+ required:
+ - gpio-controller
+ - '#gpio-cells'
+ - reg
+ - interrupts
+ - gpio-ranges
+
+ "-mux":
+ $ref: pinmux-node.yaml#
+
+ properties:
+ groups:
+ description:
+ One or more groups of pins to mux to a certain function
+ items:
+ enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi,
+ smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b,
+ smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21,
+ smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1,
+ spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2,
+ bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen,
+ rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4,
+ fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11,
+ fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3,
+ r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1,
+ rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5,
+ smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9,
+ smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8,
+ pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout,
+ serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1,
+ spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b,
+ smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12,
+ smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3,
+ hgpio4, hgpio5, hgpio6, hgpio7 ]
+
+ function:
+ description:
+ The function that a group of pins is muxed to
+ enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi,
+ smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b,
+ smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21,
+ smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1,
+ spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2,
+ bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen,
+ rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4,
+ fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11,
+ fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3,
+ r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1,
+ rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5,
+ smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9,
+ smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8,
+ pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout,
+ serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1,
+ spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b,
+ smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12,
+ smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3,
+ hgpio4, hgpio5, hgpio6, hgpio7 ]
+
+ dependencies:
+ groups: [ function ]
+ function: [ groups ]
+
+ additionalProperties: false
+
+ "^pin":
+ $ref: pincfg-node.yaml#
+
+ properties:
+ pins:
+ description:
+ A list of pins to configure in certain ways, such as enabling
+ debouncing
+
+ bias-disable: true
+
+ bias-pull-up: true
+
+ bias-pull-down: true
+
+ input-enable: true
+
+ output-low: true
+
+ output-high: true
+
+ drive-push-pull: true
+
+ drive-open-drain: true
+
+ input-debounce:
+ description:
+ Debouncing periods in microseconds, one period per interrupt
+ bank found in the controller
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 1
+ maxItems: 4
+
+ slew-rate:
+ description: |
+ 0: Low rate
+ 1: High rate
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1]
+
+ drive-strength:
+ enum: [ 0, 1, 2, 4, 8, 12 ]
+
+ additionalProperties: false
+
+allOf:
+ - $ref: "pinctrl.yaml#"
+
+required:
+ - compatible
+ - ranges
+ - '#address-cells'
+ - '#size-cells'
+ - nuvoton,sysgcr
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ pinctrl: pinctrl@f0800000 {
+ compatible = "nuvoton,npcm845-pinctrl";
+ ranges = <0x0 0x0 0xf0010000 0x8000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ nuvoton,sysgcr = <&gcr>;
+
+ gpio0: gpio@f0010000 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ reg = <0x0 0xB0>;
+ interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>;
+ gpio-ranges = <&pinctrl 0 0 32>;
+ };
+
+ fanin0_pin: fanin0-mux {
+ groups = "fanin0";
+ function = "fanin0";
+ };
+
+ pin34_slew: pin34-slew {
+ pins = "GPIO34/I3C4_SDA";
+ bias-disable;
+ };
+ };
+ };
+
--
2.33.0


2022-07-18 21:18:47

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

On Thu, Jul 14, 2022 at 03:23:21PM +0300, Tomer Maimon wrote:
> Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX
> pinmux and GPIO controller.
>
> Signed-off-by: Tomer Maimon <[email protected]>
> ---
> .../pinctrl/nuvoton,npcm845-pinctrl.yaml | 213 ++++++++++++++++++
> 1 file changed, 213 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> new file mode 100644
> index 000000000000..104766f7acc5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> @@ -0,0 +1,213 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton NPCM845 Pin Controller and GPIO
> +
> +maintainers:
> + - Tomer Maimon <[email protected]>
> +
> +description:
> + The Nuvoton BMC NPCM8XX Pin Controller multi-function routed through
> + the multiplexing block, Each pin supports GPIO functionality (GPIOx)
> + and multiple functions that directly connect the pin to different
> + hardware blocks.
> +
> +properties:
> + compatible:
> + const: nuvoton,npcm845-pinctrl
> +
> + ranges:
> + maxItems: 1
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 1
> +
> + nuvoton,sysgcr:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: a phandle to access GCR registers.
> +
> +patternProperties:
> + "^gpio@":
> + type: object
> +
> + description:
> + Eight GPIO banks that each contain between 32 GPIOs.

'each contain between 32'?

> +
> + properties:
> + gpio-controller: true
> +
> + '#gpio-cells':
> + const: 2
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + gpio-ranges:
> + maxItems: 1
> +
> + required:
> + - gpio-controller
> + - '#gpio-cells'
> + - reg
> + - interrupts
> + - gpio-ranges
> +
> + "-mux":

'-mux$'? Something like 'foo-muxbar' is needed?

> + $ref: pinmux-node.yaml#
> +
> + properties:
> + groups:
> + description:
> + One or more groups of pins to mux to a certain function
> + items:
> + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi,
> + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b,
> + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21,
> + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1,
> + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2,
> + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen,
> + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4,
> + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11,
> + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3,
> + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1,
> + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5,
> + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9,
> + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8,
> + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout,
> + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1,
> + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b,
> + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12,
> + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3,
> + hgpio4, hgpio5, hgpio6, hgpio7 ]
> +
> + function:
> + description:
> + The function that a group of pins is muxed to
> + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi,
> + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b,
> + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21,
> + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1,
> + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2,
> + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen,
> + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4,
> + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11,
> + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3,
> + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1,
> + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5,
> + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9,
> + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8,
> + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout,
> + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1,
> + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b,
> + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12,
> + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3,
> + hgpio4, hgpio5, hgpio6, hgpio7 ]
> +
> + dependencies:
> + groups: [ function ]
> + function: [ groups ]
> +
> + additionalProperties: false
> +
> + "^pin":
> + $ref: pincfg-node.yaml#
> +
> + properties:
> + pins:
> + description:
> + A list of pins to configure in certain ways, such as enabling
> + debouncing
> +
> + bias-disable: true
> +
> + bias-pull-up: true
> +
> + bias-pull-down: true
> +
> + input-enable: true
> +
> + output-low: true
> +
> + output-high: true
> +
> + drive-push-pull: true
> +
> + drive-open-drain: true
> +
> + input-debounce:
> + description:
> + Debouncing periods in microseconds, one period per interrupt
> + bank found in the controller
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 1
> + maxItems: 4
> +
> + slew-rate:
> + description: |
> + 0: Low rate
> + 1: High rate
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1]
> +
> + drive-strength:
> + enum: [ 0, 1, 2, 4, 8, 12 ]
> +
> + additionalProperties: false
> +
> +allOf:
> + - $ref: "pinctrl.yaml#"
> +
> +required:
> + - compatible
> + - ranges
> + - '#address-cells'
> + - '#size-cells'
> + - nuvoton,sysgcr
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/gpio/gpio.h>
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + pinctrl: pinctrl@f0800000 {
> + compatible = "nuvoton,npcm845-pinctrl";
> + ranges = <0x0 0x0 0xf0010000 0x8000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + nuvoton,sysgcr = <&gcr>;
> +
> + gpio0: gpio@f0010000 {

gpio@0

Is this really a child block of the pinctrl? Doesn't really look like it
based on addressess. Where are the pinctrl registers? In the sysgcr? If
so, then pinctrl should be a child of it. But that doesn't really work
too well with gpio child nodes...

Rob

2022-09-18 18:50:31

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

Hi Rob,

Thanks for your comment and sorry for the late reply.

On Tue, 19 Jul 2022 at 00:10, Rob Herring <[email protected]> wrote:
>
> On Thu, Jul 14, 2022 at 03:23:21PM +0300, Tomer Maimon wrote:
> > Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX
> > pinmux and GPIO controller.
> >
> > Signed-off-by: Tomer Maimon <[email protected]>
> > ---
> > .../pinctrl/nuvoton,npcm845-pinctrl.yaml | 213 ++++++++++++++++++
> > 1 file changed, 213 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> > new file mode 100644
> > index 000000000000..104766f7acc5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> > @@ -0,0 +1,213 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Nuvoton NPCM845 Pin Controller and GPIO
> > +
> > +maintainers:
> > + - Tomer Maimon <[email protected]>
> > +
> > +description:
> > + The Nuvoton BMC NPCM8XX Pin Controller multi-function routed through
> > + the multiplexing block, Each pin supports GPIO functionality (GPIOx)
> > + and multiple functions that directly connect the pin to different
> > + hardware blocks.
> > +
> > +properties:
> > + compatible:
> > + const: nuvoton,npcm845-pinctrl
> > +
> > + ranges:
> > + maxItems: 1
> > +
> > + '#address-cells':
> > + const: 1
> > +
> > + '#size-cells':
> > + const: 1
> > +
> > + nuvoton,sysgcr:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: a phandle to access GCR registers.
> > +
> > +patternProperties:
> > + "^gpio@":
> > + type: object
> > +
> > + description:
> > + Eight GPIO banks that each contain between 32 GPIOs.
>
> 'each contain between 32'?
will be fixed net version.
>
> > +
> > + properties:
> > + gpio-controller: true
> > +
> > + '#gpio-cells':
> > + const: 2
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + gpio-ranges:
> > + maxItems: 1
> > +
> > + required:
> > + - gpio-controller
> > + - '#gpio-cells'
> > + - reg
> > + - interrupts
> > + - gpio-ranges
> > +
> > + "-mux":
>
> '-mux$'? Something like 'foo-muxbar' is needed?
No.
>
> > + $ref: pinmux-node.yaml#
> > +
> > + properties:
> > + groups:
> > + description:
> > + One or more groups of pins to mux to a certain function
> > + items:
> > + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi,
> > + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b,
> > + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21,
> > + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1,
> > + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2,
> > + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen,
> > + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4,
> > + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11,
> > + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3,
> > + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1,
> > + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5,
> > + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9,
> > + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8,
> > + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout,
> > + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1,
> > + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b,
> > + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12,
> > + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3,
> > + hgpio4, hgpio5, hgpio6, hgpio7 ]
> > +
> > + function:
> > + description:
> > + The function that a group of pins is muxed to
> > + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi,
> > + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b,
> > + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21,
> > + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1,
> > + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2,
> > + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen,
> > + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4,
> > + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11,
> > + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3,
> > + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1,
> > + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5,
> > + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9,
> > + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8,
> > + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout,
> > + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1,
> > + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b,
> > + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12,
> > + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3,
> > + hgpio4, hgpio5, hgpio6, hgpio7 ]
> > +
> > + dependencies:
> > + groups: [ function ]
> > + function: [ groups ]
> > +
> > + additionalProperties: false
> > +
> > + "^pin":
> > + $ref: pincfg-node.yaml#
> > +
> > + properties:
> > + pins:
> > + description:
> > + A list of pins to configure in certain ways, such as enabling
> > + debouncing
> > +
> > + bias-disable: true
> > +
> > + bias-pull-up: true
> > +
> > + bias-pull-down: true
> > +
> > + input-enable: true
> > +
> > + output-low: true
> > +
> > + output-high: true
> > +
> > + drive-push-pull: true
> > +
> > + drive-open-drain: true
> > +
> > + input-debounce:
> > + description:
> > + Debouncing periods in microseconds, one period per interrupt
> > + bank found in the controller
> > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > + minItems: 1
> > + maxItems: 4
> > +
> > + slew-rate:
> > + description: |
> > + 0: Low rate
> > + 1: High rate
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [0, 1]
> > +
> > + drive-strength:
> > + enum: [ 0, 1, 2, 4, 8, 12 ]
> > +
> > + additionalProperties: false
> > +
> > +allOf:
> > + - $ref: "pinctrl.yaml#"
> > +
> > +required:
> > + - compatible
> > + - ranges
> > + - '#address-cells'
> > + - '#size-cells'
> > + - nuvoton,sysgcr
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/gpio/gpio.h>
> > +
> > + soc {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > +
> > + pinctrl: pinctrl@f0800000 {
> > + compatible = "nuvoton,npcm845-pinctrl";
> > + ranges = <0x0 0x0 0xf0010000 0x8000>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + nuvoton,sysgcr = <&gcr>;
> > +
> > + gpio0: gpio@f0010000 {
>
> gpio@0
>
> Is this really a child block of the pinctrl? Doesn't really look like it
> based on addressess. Where are the pinctrl registers? In the sysgcr? If
> so, then pinctrl should be a child of it. But that doesn't really work
> too well with gpio child nodes...
the pin controller mux is handled by sysgcr this is why the sysgcr in
the mother node,
and the pin configuration are handled by the GPIO registers. each
GPIO bank (child) contains 32 GPIO.
this is why the GPIO is the child node.

>
> Rob

Best regards,

Tomer

2022-09-19 08:02:17

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

On 18/09/2022 20:28, Tomer Maimon wrote:
> Hi Rob,
>
> Thanks for your comment and sorry for the late reply.

Two months... we are out of the context and this will not help your
patchset.

>
> On Tue, 19 Jul 2022 at 00:10, Rob Herring <[email protected]> wrote:
>>

(...)

>>> +examples:
>>> + - |
>>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> + #include <dt-bindings/gpio/gpio.h>
>>> +
>>> + soc {
>>> + #address-cells = <2>;
>>> + #size-cells = <2>;
>>> +
>>> + pinctrl: pinctrl@f0800000 {
>>> + compatible = "nuvoton,npcm845-pinctrl";
>>> + ranges = <0x0 0x0 0xf0010000 0x8000>;
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> + nuvoton,sysgcr = <&gcr>;
>>> +
>>> + gpio0: gpio@f0010000 {
>>
>> gpio@0
>>
>> Is this really a child block of the pinctrl? Doesn't really look like it
>> based on addressess. Where are the pinctrl registers? In the sysgcr? If
>> so, then pinctrl should be a child of it. But that doesn't really work
>> too well with gpio child nodes...
> the pin controller mux is handled by sysgcr this is why the sysgcr in
> the mother node,
> and the pin configuration are handled by the GPIO registers. each
> GPIO bank (child) contains 32 GPIO.
> this is why the GPIO is the child node.

Then maybe pinctrl should be the sysgcr and expose regmap for other devices?


Best regards,
Krzysztof

2022-09-19 14:50:07

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

Hi Krzysztof,

Sorry but I didn't understand,

On Mon, 19 Sept 2022 at 09:56, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 18/09/2022 20:28, Tomer Maimon wrote:
> > Hi Rob,
> >
> > Thanks for your comment and sorry for the late reply.
>
> Two months... we are out of the context and this will not help your
> patchset.
>
> >
> > On Tue, 19 Jul 2022 at 00:10, Rob Herring <[email protected]> wrote:
> >>
>
> (...)
>
> >>> +examples:
> >>> + - |
> >>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> >>> + #include <dt-bindings/gpio/gpio.h>
> >>> +
> >>> + soc {
> >>> + #address-cells = <2>;
> >>> + #size-cells = <2>;
> >>> +
> >>> + pinctrl: pinctrl@f0800000 {
> >>> + compatible = "nuvoton,npcm845-pinctrl";
> >>> + ranges = <0x0 0x0 0xf0010000 0x8000>;
> >>> + #address-cells = <1>;
> >>> + #size-cells = <1>;
> >>> + nuvoton,sysgcr = <&gcr>;
> >>> +
> >>> + gpio0: gpio@f0010000 {
> >>
> >> gpio@0
> >>
> >> Is this really a child block of the pinctrl? Doesn't really look like it
> >> based on addressess. Where are the pinctrl registers? In the sysgcr? If
> >> so, then pinctrl should be a child of it. But that doesn't really work
> >> too well with gpio child nodes...
> > the pin controller mux is handled by sysgcr this is why the sysgcr in
> > the mother node,
> > and the pin configuration are handled by the GPIO registers. each
> > GPIO bank (child) contains 32 GPIO.
> > this is why the GPIO is the child node.
>
> Then maybe pinctrl should be the sysgcr and expose regmap for other devices?
The pin controller using the sysgcr to handle the pinmux, this is why
the sysgcr is in the mother node, is it problematic?

>
>
> Best regards,
> Krzysztof

Best regards,

Tomer

2022-09-19 16:47:17

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

On 19/09/2022 16:31, Tomer Maimon wrote:
>>>>> +examples:
>>>>> + - |
>>>>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>> + #include <dt-bindings/gpio/gpio.h>
>>>>> +
>>>>> + soc {
>>>>> + #address-cells = <2>;
>>>>> + #size-cells = <2>;
>>>>> +
>>>>> + pinctrl: pinctrl@f0800000 {
>>>>> + compatible = "nuvoton,npcm845-pinctrl";
>>>>> + ranges = <0x0 0x0 0xf0010000 0x8000>;
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <1>;
>>>>> + nuvoton,sysgcr = <&gcr>;
>>>>> +
>>>>> + gpio0: gpio@f0010000 {
>>>>
>>>> gpio@0
>>>>
>>>> Is this really a child block of the pinctrl? Doesn't really look like it
>>>> based on addressess. Where are the pinctrl registers? In the sysgcr? If
>>>> so, then pinctrl should be a child of it. But that doesn't really work
>>>> too well with gpio child nodes...
>>> the pin controller mux is handled by sysgcr this is why the sysgcr in
>>> the mother node,
>>> and the pin configuration are handled by the GPIO registers. each
>>> GPIO bank (child) contains 32 GPIO.
>>> this is why the GPIO is the child node.
>>
>> Then maybe pinctrl should be the sysgcr and expose regmap for other devices?
> The pin controller using the sysgcr to handle the pinmux, this is why
> the sysgcr is in the mother node, is it problematic?

You said pin-controller mux registers are in sysgcr, so it should not be
used via syscon.

Please provide address map description to convince us that this is
correct HW representation.

Best regards,
Krzysztof

2022-09-20 08:16:11

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

On Mon, 19 Sept 2022 at 19:06, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 19/09/2022 16:31, Tomer Maimon wrote:
> >>>>> +examples:
> >>>>> + - |
> >>>>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> >>>>> + #include <dt-bindings/gpio/gpio.h>
> >>>>> +
> >>>>> + soc {
> >>>>> + #address-cells = <2>;
> >>>>> + #size-cells = <2>;
> >>>>> +
> >>>>> + pinctrl: pinctrl@f0800000 {
> >>>>> + compatible = "nuvoton,npcm845-pinctrl";
> >>>>> + ranges = <0x0 0x0 0xf0010000 0x8000>;
> >>>>> + #address-cells = <1>;
> >>>>> + #size-cells = <1>;
> >>>>> + nuvoton,sysgcr = <&gcr>;
> >>>>> +
> >>>>> + gpio0: gpio@f0010000 {
> >>>>
> >>>> gpio@0
> >>>>
> >>>> Is this really a child block of the pinctrl? Doesn't really look like it
> >>>> based on addressess. Where are the pinctrl registers? In the sysgcr? If
> >>>> so, then pinctrl should be a child of it. But that doesn't really work
> >>>> too well with gpio child nodes...
> >>> the pin controller mux is handled by sysgcr this is why the sysgcr in
> >>> the mother node,
> >>> and the pin configuration are handled by the GPIO registers. each
> >>> GPIO bank (child) contains 32 GPIO.
> >>> this is why the GPIO is the child node.
> >>
> >> Then maybe pinctrl should be the sysgcr and expose regmap for other devices?
> > The pin controller using the sysgcr to handle the pinmux, this is why
> > the sysgcr is in the mother node, is it problematic?
>
> You said pin-controller mux registers are in sysgcr, so it should not be
> used via syscon.
Sorry but maybe I missed something.
the sysgcr is used for miscellaneous features and not only for the pin
controller mux, this is why it used syscon and defined in the dtsi:
gcr: system-controller@f0800000 {
compatible = "nuvoton,npcm845-gcr", "syscon";
reg = <0x0 0xf0800000 0x0 0x1000>;
};
>
> Please provide address map description to convince us that this is
> correct HW representation.
GCR (sysgcr) registers 0xf0800000-0xf0801000 - used for miscellaneous
features, not only pin mux.
GPIO0 0xf0010000-0xf0011000
GPIO1 0xf0011000-0xf0012000
...
GPIO7 0xf0017000-0xf0018000
>
> Best regards,
> Krzysztof

Best regards,

Tomer

2022-09-20 08:55:28

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

On 20/09/2022 09:59, Tomer Maimon wrote:
>>>>>>> + pinctrl: pinctrl@f0800000 {
>>>>>>> + compatible = "nuvoton,npcm845-pinctrl";
>>>>>>> + ranges = <0x0 0x0 0xf0010000 0x8000>;
>>>>>>> + #address-cells = <1>;
>>>>>>> + #size-cells = <1>;
>>>>>>> + nuvoton,sysgcr = <&gcr>;
>>>>>>> +
>>>>>>> + gpio0: gpio@f0010000 {
>>>>>>
>>>>>> gpio@0
>>>>>>
>>>>>> Is this really a child block of the pinctrl? Doesn't really look like it
>>>>>> based on addressess. Where are the pinctrl registers? In the sysgcr? If
>>>>>> so, then pinctrl should be a child of it. But that doesn't really work
>>>>>> too well with gpio child nodes...
>>>>> the pin controller mux is handled by sysgcr this is why the sysgcr in
>>>>> the mother node,
>>>>> and the pin configuration are handled by the GPIO registers. each
>>>>> GPIO bank (child) contains 32 GPIO.
>>>>> this is why the GPIO is the child node.
>>>>
>>>> Then maybe pinctrl should be the sysgcr and expose regmap for other devices?
>>> The pin controller using the sysgcr to handle the pinmux, this is why
>>> the sysgcr is in the mother node, is it problematic?
>>
>> You said pin-controller mux registers are in sysgcr, so it should not be
>> used via syscon.
> Sorry but maybe I missed something.
> the sysgcr is used for miscellaneous features and not only for the pin
> controller mux, this is why it used syscon and defined in the dtsi:
> gcr: system-controller@f0800000 {
> compatible = "nuvoton,npcm845-gcr", "syscon";
> reg = <0x0 0xf0800000 0x0 0x1000>;
> };
>>
>> Please provide address map description to convince us that this is
>> correct HW representation.
> GCR (sysgcr) registers 0xf0800000-0xf0801000 - used for miscellaneous
> features, not only pin mux.
> GPIO0 0xf0010000-0xf0011000
> GPIO1 0xf0011000-0xf0012000
> ...
> GPIO7 0xf0017000-0xf0018000
>>

Then why your pinctrl is in sysgcr IO range? (pinctrl@f0800000)

Your map looks quite different from what you described in example.

Best regards,
Krzysztof

2022-09-20 09:02:57

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

On 20/09/2022 10:32, Tomer Maimon wrote:
> On Tue, 20 Sept 2022 at 11:21, Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 20/09/2022 09:59, Tomer Maimon wrote:
>>>>>>>>> + pinctrl: pinctrl@f0800000 {
>>>>>>>>> + compatible = "nuvoton,npcm845-pinctrl";
>>>>>>>>> + ranges = <0x0 0x0 0xf0010000 0x8000>;
>>>>>>>>> + #address-cells = <1>;
>>>>>>>>> + #size-cells = <1>;
>>>>>>>>> + nuvoton,sysgcr = <&gcr>;
>>>>>>>>> +
>>>>>>>>> + gpio0: gpio@f0010000 {
>>>>>>>>
>>>>>>>> gpio@0
>>>>>>>>
>>>>>>>> Is this really a child block of the pinctrl? Doesn't really look like it
>>>>>>>> based on addressess. Where are the pinctrl registers? In the sysgcr? If
>>>>>>>> so, then pinctrl should be a child of it. But that doesn't really work
>>>>>>>> too well with gpio child nodes...
>>>>>>> the pin controller mux is handled by sysgcr this is why the sysgcr in
>>>>>>> the mother node,
>>>>>>> and the pin configuration are handled by the GPIO registers. each
>>>>>>> GPIO bank (child) contains 32 GPIO.
>>>>>>> this is why the GPIO is the child node.
>>>>>>
>>>>>> Then maybe pinctrl should be the sysgcr and expose regmap for other devices?
>>>>> The pin controller using the sysgcr to handle the pinmux, this is why
>>>>> the sysgcr is in the mother node, is it problematic?
>>>>
>>>> You said pin-controller mux registers are in sysgcr, so it should not be
>>>> used via syscon.
>>> Sorry but maybe I missed something.
>>> the sysgcr is used for miscellaneous features and not only for the pin
>>> controller mux, this is why it used syscon and defined in the dtsi:
>>> gcr: system-controller@f0800000 {
>>> compatible = "nuvoton,npcm845-gcr", "syscon";
>>> reg = <0x0 0xf0800000 0x0 0x1000>;
>>> };
>>>>
>>>> Please provide address map description to convince us that this is
>>>> correct HW representation.
>>> GCR (sysgcr) registers 0xf0800000-0xf0801000 - used for miscellaneous
>>> features, not only pin mux.
>>> GPIO0 0xf0010000-0xf0011000
>>> GPIO1 0xf0011000-0xf0012000
>>> ...
>>> GPIO7 0xf0017000-0xf0018000
>>>>
>>
>> Then why your pinctrl is in sysgcr IO range? (pinctrl@f0800000)
> you suggest using pinctrl@0 or pinctrl@f0010000 and not
> pinctrl@f0800000 because 0xf0800000 is the GCR address that serve
> miscellaneous features and not only pinmux controller ?

If you have a map like you pasted, then DTS like this:

syscon@f0800000 {}
pinctrl@f0800000 {
gpio@f0010000 {}
}

Is quite weird, don't you think? You have two devices on the same unit
address which is not allowed. You have child of pinctrl with entirely
different unit address, so how is it its child?

Best regards,
Krzysztof

2022-09-20 09:26:13

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

On Tue, 20 Sept 2022 at 11:21, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 20/09/2022 09:59, Tomer Maimon wrote:
> >>>>>>> + pinctrl: pinctrl@f0800000 {
> >>>>>>> + compatible = "nuvoton,npcm845-pinctrl";
> >>>>>>> + ranges = <0x0 0x0 0xf0010000 0x8000>;
> >>>>>>> + #address-cells = <1>;
> >>>>>>> + #size-cells = <1>;
> >>>>>>> + nuvoton,sysgcr = <&gcr>;
> >>>>>>> +
> >>>>>>> + gpio0: gpio@f0010000 {
> >>>>>>
> >>>>>> gpio@0
> >>>>>>
> >>>>>> Is this really a child block of the pinctrl? Doesn't really look like it
> >>>>>> based on addressess. Where are the pinctrl registers? In the sysgcr? If
> >>>>>> so, then pinctrl should be a child of it. But that doesn't really work
> >>>>>> too well with gpio child nodes...
> >>>>> the pin controller mux is handled by sysgcr this is why the sysgcr in
> >>>>> the mother node,
> >>>>> and the pin configuration are handled by the GPIO registers. each
> >>>>> GPIO bank (child) contains 32 GPIO.
> >>>>> this is why the GPIO is the child node.
> >>>>
> >>>> Then maybe pinctrl should be the sysgcr and expose regmap for other devices?
> >>> The pin controller using the sysgcr to handle the pinmux, this is why
> >>> the sysgcr is in the mother node, is it problematic?
> >>
> >> You said pin-controller mux registers are in sysgcr, so it should not be
> >> used via syscon.
> > Sorry but maybe I missed something.
> > the sysgcr is used for miscellaneous features and not only for the pin
> > controller mux, this is why it used syscon and defined in the dtsi:
> > gcr: system-controller@f0800000 {
> > compatible = "nuvoton,npcm845-gcr", "syscon";
> > reg = <0x0 0xf0800000 0x0 0x1000>;
> > };
> >>
> >> Please provide address map description to convince us that this is
> >> correct HW representation.
> > GCR (sysgcr) registers 0xf0800000-0xf0801000 - used for miscellaneous
> > features, not only pin mux.
> > GPIO0 0xf0010000-0xf0011000
> > GPIO1 0xf0011000-0xf0012000
> > ...
> > GPIO7 0xf0017000-0xf0018000
> >>
>
> Then why your pinctrl is in sysgcr IO range? (pinctrl@f0800000)
you suggest using pinctrl@0 or pinctrl@f0010000 and not
pinctrl@f0800000 because 0xf0800000 is the GCR address that serve
miscellaneous features and not only pinmux controller ?
>
> Your map looks quite different from what you described in example.
>
> Best regards,
> Krzysztof

Best regards,

Tomer

2022-09-20 09:52:07

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

On Tue, 20 Sept 2022 at 11:47, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 20/09/2022 10:32, Tomer Maimon wrote:
> > On Tue, 20 Sept 2022 at 11:21, Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 20/09/2022 09:59, Tomer Maimon wrote:
> >>>>>>>>> + pinctrl: pinctrl@f0800000 {
> >>>>>>>>> + compatible = "nuvoton,npcm845-pinctrl";
> >>>>>>>>> + ranges = <0x0 0x0 0xf0010000 0x8000>;
> >>>>>>>>> + #address-cells = <1>;
> >>>>>>>>> + #size-cells = <1>;
> >>>>>>>>> + nuvoton,sysgcr = <&gcr>;
> >>>>>>>>> +
> >>>>>>>>> + gpio0: gpio@f0010000 {
> >>>>>>>>
> >>>>>>>> gpio@0
> >>>>>>>>
> >>>>>>>> Is this really a child block of the pinctrl? Doesn't really look like it
> >>>>>>>> based on addressess. Where are the pinctrl registers? In the sysgcr? If
> >>>>>>>> so, then pinctrl should be a child of it. But that doesn't really work
> >>>>>>>> too well with gpio child nodes...
> >>>>>>> the pin controller mux is handled by sysgcr this is why the sysgcr in
> >>>>>>> the mother node,
> >>>>>>> and the pin configuration are handled by the GPIO registers. each
> >>>>>>> GPIO bank (child) contains 32 GPIO.
> >>>>>>> this is why the GPIO is the child node.
> >>>>>>
> >>>>>> Then maybe pinctrl should be the sysgcr and expose regmap for other devices?
> >>>>> The pin controller using the sysgcr to handle the pinmux, this is why
> >>>>> the sysgcr is in the mother node, is it problematic?
> >>>>
> >>>> You said pin-controller mux registers are in sysgcr, so it should not be
> >>>> used via syscon.
> >>> Sorry but maybe I missed something.
> >>> the sysgcr is used for miscellaneous features and not only for the pin
> >>> controller mux, this is why it used syscon and defined in the dtsi:
> >>> gcr: system-controller@f0800000 {
> >>> compatible = "nuvoton,npcm845-gcr", "syscon";
> >>> reg = <0x0 0xf0800000 0x0 0x1000>;
> >>> };
> >>>>
> >>>> Please provide address map description to convince us that this is
> >>>> correct HW representation.
> >>> GCR (sysgcr) registers 0xf0800000-0xf0801000 - used for miscellaneous
> >>> features, not only pin mux.
> >>> GPIO0 0xf0010000-0xf0011000
> >>> GPIO1 0xf0011000-0xf0012000
> >>> ...
> >>> GPIO7 0xf0017000-0xf0018000
> >>>>
> >>
> >> Then why your pinctrl is in sysgcr IO range? (pinctrl@f0800000)
> > you suggest using pinctrl@0 or pinctrl@f0010000 and not
> > pinctrl@f0800000 because 0xf0800000 is the GCR address that serve
> > miscellaneous features and not only pinmux controller ?
>
> If you have a map like you pasted, then DTS like this:
>
> syscon@f0800000 {}
> pinctrl@f0800000 {
> gpio@f0010000 {}
> }
>
> Is quite weird, don't you think? You have two devices on the same unit
> address which is not allowed. You have child of pinctrl with entirely
O.K.
> different unit address, so how is it its child?
The pinctrl node name will modify the pinctrl@f0010000 the same as the
range property and the start of the child registers,is it fine?
> Best regards,
> Krzysztof

Best regards,

Tomer

2022-09-20 15:28:01

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

On 20/09/2022 11:27, Tomer Maimon wrote:
> On Tue, 20 Sept 2022 at 11:47, Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 20/09/2022 10:32, Tomer Maimon wrote:
>>> On Tue, 20 Sept 2022 at 11:21, Krzysztof Kozlowski
>>> <[email protected]> wrote:
>>>>
>>>> On 20/09/2022 09:59, Tomer Maimon wrote:
>>>>>>>>>>> + pinctrl: pinctrl@f0800000 {
>>>>>>>>>>> + compatible = "nuvoton,npcm845-pinctrl";
>>>>>>>>>>> + ranges = <0x0 0x0 0xf0010000 0x8000>;
>>>>>>>>>>> + #address-cells = <1>;
>>>>>>>>>>> + #size-cells = <1>;
>>>>>>>>>>> + nuvoton,sysgcr = <&gcr>;
>>>>>>>>>>> +
>>>>>>>>>>> + gpio0: gpio@f0010000 {
>>>>>>>>>>
>>>>>>>>>> gpio@0
>>>>>>>>>>
>>>>>>>>>> Is this really a child block of the pinctrl? Doesn't really look like it
>>>>>>>>>> based on addressess. Where are the pinctrl registers? In the sysgcr? If
>>>>>>>>>> so, then pinctrl should be a child of it. But that doesn't really work
>>>>>>>>>> too well with gpio child nodes...
>>>>>>>>> the pin controller mux is handled by sysgcr this is why the sysgcr in
>>>>>>>>> the mother node,
>>>>>>>>> and the pin configuration are handled by the GPIO registers. each
>>>>>>>>> GPIO bank (child) contains 32 GPIO.
>>>>>>>>> this is why the GPIO is the child node.
>>>>>>>>
>>>>>>>> Then maybe pinctrl should be the sysgcr and expose regmap for other devices?
>>>>>>> The pin controller using the sysgcr to handle the pinmux, this is why
>>>>>>> the sysgcr is in the mother node, is it problematic?
>>>>>>
>>>>>> You said pin-controller mux registers are in sysgcr, so it should not be
>>>>>> used via syscon.
>>>>> Sorry but maybe I missed something.
>>>>> the sysgcr is used for miscellaneous features and not only for the pin
>>>>> controller mux, this is why it used syscon and defined in the dtsi:
>>>>> gcr: system-controller@f0800000 {
>>>>> compatible = "nuvoton,npcm845-gcr", "syscon";
>>>>> reg = <0x0 0xf0800000 0x0 0x1000>;
>>>>> };
>>>>>>
>>>>>> Please provide address map description to convince us that this is
>>>>>> correct HW representation.
>>>>> GCR (sysgcr) registers 0xf0800000-0xf0801000 - used for miscellaneous
>>>>> features, not only pin mux.
>>>>> GPIO0 0xf0010000-0xf0011000
>>>>> GPIO1 0xf0011000-0xf0012000
>>>>> ...
>>>>> GPIO7 0xf0017000-0xf0018000
>>>>>>
>>>>
>>>> Then why your pinctrl is in sysgcr IO range? (pinctrl@f0800000)
>>> you suggest using pinctrl@0 or pinctrl@f0010000 and not
>>> pinctrl@f0800000 because 0xf0800000 is the GCR address that serve
>>> miscellaneous features and not only pinmux controller ?
>>
>> If you have a map like you pasted, then DTS like this:
>>
>> syscon@f0800000 {}
>> pinctrl@f0800000 {
>> gpio@f0010000 {}
>> }
>>
>> Is quite weird, don't you think? You have two devices on the same unit
>> address which is not allowed. You have child of pinctrl with entirely
> O.K.
>> different unit address, so how is it its child?
> The pinctrl node name will modify the pinctrl@f0010000 the same as the
> range property and the start of the child registers,is it fine?

We are all busy, so I don't have that much bandwidth to review each of
your many solutions and instead poking me with every possible solution,
I would prefer if you think a bit how this all should work and look.

I don't know if it is fine. Why you should have two devices like this:
pinctrl@f0010000 {
gpio@f0010000 {}
}

???
Instead of one device? Answer such questions to yourself before asking
me. Please come with reasonable DTS describing the hardware.

Best regards,
Krzysztof

2022-09-20 18:11:58

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

On Tue, 20 Sept 2022 at 18:16, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 20/09/2022 11:27, Tomer Maimon wrote:
> > On Tue, 20 Sept 2022 at 11:47, Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 20/09/2022 10:32, Tomer Maimon wrote:
> >>> On Tue, 20 Sept 2022 at 11:21, Krzysztof Kozlowski
> >>> <[email protected]> wrote:
> >>>>
> >>>> On 20/09/2022 09:59, Tomer Maimon wrote:
> >>>>>>>>>>> + pinctrl: pinctrl@f0800000 {
> >>>>>>>>>>> + compatible = "nuvoton,npcm845-pinctrl";
> >>>>>>>>>>> + ranges = <0x0 0x0 0xf0010000 0x8000>;
> >>>>>>>>>>> + #address-cells = <1>;
> >>>>>>>>>>> + #size-cells = <1>;
> >>>>>>>>>>> + nuvoton,sysgcr = <&gcr>;
> >>>>>>>>>>> +
> >>>>>>>>>>> + gpio0: gpio@f0010000 {
> >>>>>>>>>>
> >>>>>>>>>> gpio@0
> >>>>>>>>>>
> >>>>>>>>>> Is this really a child block of the pinctrl? Doesn't really look like it
> >>>>>>>>>> based on addressess. Where are the pinctrl registers? In the sysgcr? If
> >>>>>>>>>> so, then pinctrl should be a child of it. But that doesn't really work
> >>>>>>>>>> too well with gpio child nodes...
> >>>>>>>>> the pin controller mux is handled by sysgcr this is why the sysgcr in
> >>>>>>>>> the mother node,
> >>>>>>>>> and the pin configuration are handled by the GPIO registers. each
> >>>>>>>>> GPIO bank (child) contains 32 GPIO.
> >>>>>>>>> this is why the GPIO is the child node.
> >>>>>>>>
> >>>>>>>> Then maybe pinctrl should be the sysgcr and expose regmap for other devices?
> >>>>>>> The pin controller using the sysgcr to handle the pinmux, this is why
> >>>>>>> the sysgcr is in the mother node, is it problematic?
> >>>>>>
> >>>>>> You said pin-controller mux registers are in sysgcr, so it should not be
> >>>>>> used via syscon.
> >>>>> Sorry but maybe I missed something.
> >>>>> the sysgcr is used for miscellaneous features and not only for the pin
> >>>>> controller mux, this is why it used syscon and defined in the dtsi:
> >>>>> gcr: system-controller@f0800000 {
> >>>>> compatible = "nuvoton,npcm845-gcr", "syscon";
> >>>>> reg = <0x0 0xf0800000 0x0 0x1000>;
> >>>>> };
> >>>>>>
> >>>>>> Please provide address map description to convince us that this is
> >>>>>> correct HW representation.
> >>>>> GCR (sysgcr) registers 0xf0800000-0xf0801000 - used for miscellaneous
> >>>>> features, not only pin mux.
> >>>>> GPIO0 0xf0010000-0xf0011000
> >>>>> GPIO1 0xf0011000-0xf0012000
> >>>>> ...
> >>>>> GPIO7 0xf0017000-0xf0018000
> >>>>>>
> >>>>
> >>>> Then why your pinctrl is in sysgcr IO range? (pinctrl@f0800000)
> >>> you suggest using pinctrl@0 or pinctrl@f0010000 and not
> >>> pinctrl@f0800000 because 0xf0800000 is the GCR address that serve
> >>> miscellaneous features and not only pinmux controller ?
> >>
> >> If you have a map like you pasted, then DTS like this:
> >>
> >> syscon@f0800000 {}
> >> pinctrl@f0800000 {
> >> gpio@f0010000 {}
> >> }
> >>
> >> Is quite weird, don't you think? You have two devices on the same unit
> >> address which is not allowed. You have child of pinctrl with entirely
> > O.K.
> >> different unit address, so how is it its child?
> > The pinctrl node name will modify the pinctrl@f0010000 the same as the
> > range property and the start of the child registers,is it fine?
>
> We are all busy, so I don't have that much bandwidth to review each of
> your many solutions and instead poking me with every possible solution,
> I would prefer if you think a bit how this all should work and look.
>
> I don't know if it is fine. Why you should have two devices like this:
> pinctrl@f0010000 {
> gpio@f0010000 {}
> }
>
> ???
> Instead of one device? Answer such questions to yourself before asking
> me. Please come with reasonable DTS describing the hardware.
>
Will do, thanks.
> Best regards,
> Krzysztof

Best regards,

Tomer