pinctrl: nuvoton: add pinmux and GPIO driver for NPCM8XX

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

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +30 -0

new file mode 100644
index 000000000000..104766f7acc5
@@ -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
+

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +14 -0

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

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +120 -0

+    $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>;

Quoted 49 lines · Tomer Maimon

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

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +34 -0

new file mode 100644
index 000000000000..104766f7acc5
@@ -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.
+

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 40

Quoted 6 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6 -0

+patternProperties:
+  "^gpio@":
+    type: object
+
+    description:
+      Eight GPIO banks that each contain between 32 GPIOs.

'each contain between 32'?

Quoted 19 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +18 -0

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

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 64

Quoted 6 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6 -0

+      - '#gpio-cells'
+      - reg
+      - interrupts
+      - gpio-ranges
+
+  "-mux":

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

Quoted 121 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +120 -0

+    $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>;

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 194

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10 -0

+      #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

Hi Rob,

Thanks for your comment and sorry for the late reply.

Quoted 1 line

Quoted 20 lines

Quoted 7 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +7 -0

+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    soc {
+      #address-cells = <2>;

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 194

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10 -0

+      #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

Quoted 4 lines

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.

Quoted 3 lines · Tomer Maimon

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.

Quoted 3 lines · Rob Herring

Quoted 1 line

(...)

Quoted 24 lines

Quoted 24 lines

Quoted 7 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +7 -0

+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    soc {
+      #address-cells = <2>;

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 194

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10 -0

+      #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...

Quoted 5 lines

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

Quoted 30 lines

Quoted 29 lines

Quoted 20 lines

Quoted 7 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +7 -0

+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    soc {
+      #address-cells = <2>;

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 194

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10 -0

+      #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

Quoted 4 lines

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.

Quoted 1 line

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?

Quoted 30 lines · Tomer Maimon

Quoted 30 lines

Quoted 29 lines

Quoted 20 lines

Quoted 7 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +7 -0

+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    soc {
+      #address-cells = <2>;

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 194

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10 -0

+      #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

Quoted 4 lines

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.

Quoted 3 lines

Quoted 1 line

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

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+      pinctrl: pinctrl@f0800000 {
+        compatible = "nuvoton,npcm845-pinctrl";
+        ranges = <0x0 0x0 0xf0010000 0x8000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        nuvoton,sysgcr = <&gcr>;
+
+        gpio0: gpio@f0010000 {

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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>;
                };

Quoted 1 line

Quoted 2 lines

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

Quoted 1 line

Quoted 35 lines · Tomer Maimon

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+      pinctrl: pinctrl@f0800000 {
+        compatible = "nuvoton,npcm845-pinctrl";
+        ranges = <0x0 0x0 0xf0010000 0x8000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        nuvoton,sysgcr = <&gcr>;
+
+        gpio0: gpio@f0010000 {

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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>;
                };

Quoted 1 line

Quoted 9 lines

Quoted 2 lines

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

Quoted 1 line

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

Quoted 47 lines · Krzysztof Kozlowski

Quoted 44 lines · Tomer Maimon

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+      pinctrl: pinctrl@f0800000 {
+        compatible = "nuvoton,npcm845-pinctrl";
+        ranges = <0x0 0x0 0xf0010000 0x8000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        nuvoton,sysgcr = <&gcr>;
+
+        gpio0: gpio@f0010000 {

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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>;
                };

Quoted 3 lines

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

Quoted 1 line

Quoted 1 line

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 ?

Quoted 49 lines · Tomer Maimon

Quoted 47 lines · Krzysztof Kozlowski

Quoted 44 lines · Tomer Maimon

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+      pinctrl: pinctrl@f0800000 {
+        compatible = "nuvoton,npcm845-pinctrl";
+        ranges = <0x0 0x0 0xf0010000 0x8000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        nuvoton,sysgcr = <&gcr>;
+
+        gpio0: gpio@f0010000 {

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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>;
                };

Quoted 3 lines

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

Quoted 1 line

Quoted 4 lines

Quoted 1 line

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

Quoted 63 lines · Krzysztof Kozlowski

Quoted 49 lines · Tomer Maimon

Quoted 47 lines · Krzysztof Kozlowski

Quoted 44 lines · Tomer Maimon

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+      pinctrl: pinctrl@f0800000 {
+        compatible = "nuvoton,npcm845-pinctrl";
+        ranges = <0x0 0x0 0xf0010000 0x8000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        nuvoton,sysgcr = <&gcr>;
+
+        gpio0: gpio@f0010000 {

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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>;
                };

Quoted 3 lines

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

Quoted 1 line

Quoted 4 lines

Quoted 1 line

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 {}
}

Quoted 2 lines

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.

Quoted 1 line

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?

Quoted 65 lines · Tomer Maimon

Quoted 63 lines · Krzysztof Kozlowski

Quoted 49 lines · Tomer Maimon

Quoted 47 lines · Krzysztof Kozlowski

Quoted 44 lines · Tomer Maimon

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+      pinctrl: pinctrl@f0800000 {
+        compatible = "nuvoton,npcm845-pinctrl";
+        ranges = <0x0 0x0 0xf0010000 0x8000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        nuvoton,sysgcr = <&gcr>;
+
+        gpio0: gpio@f0010000 {

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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>;
                };

Quoted 3 lines

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

Quoted 1 line

Quoted 4 lines

Quoted 1 line

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 {}
}

Quoted 6 lines

Quoted 2 lines

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.

Quoted 1 line

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

Add pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC.

Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the pin mux mapping difference the NPCM8XX GPIO supports adjust debounce period time.

Signed-off-by Tomer Maimon <[email protected]>

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +7 -0

+
+static int npcm8xx_pinctrl_probe(struct platform_device *pdev)
+{
+	struct npcm8xx_pinctrl *pctrl;
+	int ret;
+
+	pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);

On 14/07/2022 14:23, Tomer Maimon wrote:

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2473

Quoted 2 lines

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2 -0

+static int npcm8xx_pinctrl_probe(struct platform_device *pdev)
+{

Quoted 6 lines

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +6 -0

+     struct npcm8xx_pinctrl *pctrl;
+     int ret;
+
+     pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
+     if (!pctrl)
+             return -ENOMEM;

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2487

Quoted 8 lines

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +8 -0

+
+     pctrl->dev = &pdev->dev;
+     dev_set_drvdata(&pdev->dev, pctrl);
+
+     pctrl->gcr_regmap =
+             syscon_regmap_lookup_by_phandle(pctrl->dev->of_node, "nuvoton,sysgcr");
+     if (IS_ERR(pctrl->gcr_regmap))
+             return dev_err_probe(pctrl->dev, PTR_ERR(pctrl->gcr_regmap), "Failed to find nuvoton,sysgcr property\n");

Please wrap your code according to Linux coding style - @80.

Add pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC.

Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the pin mux mapping difference the NPCM8XX GPIO supports adjust debounce period time.

+ depends on (ARCH_NPCM || COMPILE_TEST) && OF

+#include <linux/regmap.h>

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +7 -0

+#define NPCM8XX_GCR_MFSEL1     0x260
+#define NPCM8XX_GCR_MFSEL2     0x264
+#define NPCM8XX_GCR_MFSEL3     0x268
+#define NPCM8XX_GCR_MFSEL4     0x26C
+#define NPCM8XX_GCR_MFSEL5     0x270
+#define NPCM8XX_GCR_MFSEL6     0x274
+#define NPCM8XX_GCR_MFSEL7     0x278

+/* GPIO registers */

+#define NPCM8XX_DEBOUNCE_NANOSEC 40

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +11 -0

+       struct debounce_time    debounce;
+       int                     irqbase;
+       int                     irq;
+       struct irq_chip         irq_chip;
+       u32                     pinctrl_id;
+       int (*direction_input)(struct gpio_chip *chip, unsigned int offset);
+       int (*direction_output)(struct gpio_chip *chip, unsigned int offset,
+                               int value);
+       int (*request)(struct gpio_chip *chip, unsigned int offset);
+       void (*free)(struct gpio_chip *chip, unsigned int offset);
+};

+}

+ for_each_set_bit(bit, (const void *)&sts, NPCM8XX_GPIO_PER_BANK)

+ generic_handle_domain_irq(gc->irq.domain, bit);

+ unsigned int gpio = BIT(d->hwirq);

+ if (type & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +3 -0

+               npcm_gpio_set(&bank->gc, bank->base + NPCM8XX_GP_N_EVTYP, gpio);
+               irq_set_handler_locked(d, handle_edge_irq);
+       }

+ unsigned int gpio = d->hwirq;

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5 -0

+                       return 0;
+               default:
+                       return -EINVAL;
+               }
+       }

+ int gpio = (pin % bank->gc.ngpio);

+ u32 ds = 0;

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +7 -0

+               val = ioread32(bank->base + NPCM8XX_GP_N_ODSC) & pinmask;
+               ds = val ? DSHI(flg) : DSLO(flg);
+               dev_dbg(bank->gc.parent, "pin %d strength %d = %d\n", pin, val, ds);
+               return ds;
+       }
+
+       return -EINVAL;

+ v = (pincfg[pin].flag & DRIVE_STRENGTH_MASK);

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5 -0

+static int npcm8xx_gpio_request_enable(struct pinctrl_dev *pctldev,
+                                      struct pinctrl_gpio_range *range,
+                                      unsigned int offset)
+{
+       struct npcm8xx_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5 -0

+
+       npcm8xx_setfunc(npcm->gcr_regmap, &offset, 1, fn_gpio);
+
+       return 0;
+}

+ int debounce_select;

+ dbncp_val_mod = dbncp_val & 0xF;

+ if (dbncp_val_mod > 0x7)

+ int ret = 0;

drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +8 -0

+               }
+
+               return ret;
+       }
+
+       npcm_gpio_clr(&bank->gc, bank->base + NPCM8XX_GP_N_DBNC, gpio);
+
+       return 0;

+static int npcm8xx_gpio_of(struct npcm8xx_pinctrl *pctrl)

+ char gpioirqname[30];

+ dev_set_drvdata(&pdev->dev, pctrl);