[v4,1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

Commit Message

Yixun Lan Jan. 21, 2025, 3:38 a.m. UTC

The GPIO controller of K1 support basic functions as input/output,
all pins can be used as interrupt which route to one IRQ line,
trigger type can be select between rising edge, failing edge, or both.
There are four GPIO ports, each consisting of 32 pins.

Signed-off-by: Yixun Lan <[e-mail]>
---
 .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
 1 file changed, 116 insertions(+)

Comments

Olof Johansson Jan. 22, 2025, 8:03 p.m. UTC | #1

Hi,

On Tue, Jan 21, 2025 at 11:38:11AM +0800, Yixun Lan wrote:
> The GPIO controller of K1 support basic functions as input/output,
> all pins can be used as interrupt which route to one IRQ line,
> trigger type can be select between rising edge, failing edge, or both.
> There are four GPIO ports, each consisting of 32 pins.
> 
> Signed-off-by: Yixun Lan <[e-mail]>
> ---
>  .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
>  1 file changed, 116 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..dd9459061aecfcba84e6a3c5052fbcddf6c61150
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> @@ -0,0 +1,116 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/spacemit,k1-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SpacemiT K1 GPIO controller
> +
> +maintainers:
> +  - Yixun Lan <[e-mail]>
> +
> +description:
> +  The controller's registers are organized as sets of eight 32-bit
> +  registers with each set of port controlling 32 pins.  A single
> +  interrupt line is shared for all of the pins by the controller.
> +  Each port will be represented as child nodes with the generic
> +  GPIO-controller properties in this bindings file.

There's only one interrupt line for all ports, but you have a binding that
duplicates them for every set of ports. That seems overly complicated,
doesn't it? They'd all bind the same handler, so there's no benefit in
providing the flexibility,.

> +properties:
> +  $nodename:
> +    pattern: "^gpio@[0-9a-f]+$"
> +
> +  compatible:
> +    const: spacemit,k1-gpio
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^gpio-port@[0-9a-f]+$":
> +    type: object
> +    properties:
> +      compatible:
> +        const: spacemit,k1-gpio-port
> +
> +      reg:
> +        maxItems: 1
> +
> +      gpio-controller: true
> +
> +      "#gpio-cells":
> +        const: 2
> +
> +      gpio-ranges: true
> +
> +      interrupts:
> +        maxItems: 1
> +
> +      interrupt-controller: true
> +
> +      "#interrupt-cells":
> +        const: 2
> +        description:
> +          The first cell is the GPIO number, the second should specify interrupt
> +          flag. The controller does not support level interrupts, so flags of
> +          IRQ_TYPE_LEVEL_HIGH, IRQ_TYPE_LEVEL_LOW should not be used.
> +          Refer <dt-bindings/interrupt-controller/irq.h> for valid flags.

Same here, since there's no real flexibility between the banks, it might
make sense to consider a 3-cell GPIO specifier instead, and having
the first cell indicate bank. I could see this argument go in either
direction, but I'm not sure I understand why to provide a gpio-controller
per bank.

Comparing to say Rockchip, where each bank has a separate interrupt line
-- so there the granularity makes sense.


-Olof

Yixun Lan Jan. 23, 2025, 11:30 a.m. UTC | #2

Hi Olof:
 thanks for your reivew

On 12:03 Wed 22 Jan     , Olof Johansson wrote:
> Hi,
> 
> On Tue, Jan 21, 2025 at 11:38:11AM +0800, Yixun Lan wrote:
> > The GPIO controller of K1 support basic functions as input/output,
> > all pins can be used as interrupt which route to one IRQ line,
> > trigger type can be select between rising edge, failing edge, or both.
> > There are four GPIO ports, each consisting of 32 pins.
> > 
> > Signed-off-by: Yixun Lan <[e-mail]>
> > ---
> >  .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
> >  1 file changed, 116 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..dd9459061aecfcba84e6a3c5052fbcddf6c61150
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > @@ -0,0 +1,116 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/spacemit,k1-gpio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SpacemiT K1 GPIO controller
> > +
> > +maintainers:
> > +  - Yixun Lan <[e-mail]>
> > +
> > +description:
> > +  The controller's registers are organized as sets of eight 32-bit
> > +  registers with each set of port controlling 32 pins.  A single
> > +  interrupt line is shared for all of the pins by the controller.
> > +  Each port will be represented as child nodes with the generic
> > +  GPIO-controller properties in this bindings file.
> 
> There's only one interrupt line for all ports, but you have a binding that
> duplicates them for every set of ports. That seems overly complicated,
> doesn't it? They'd all bind the same handler, so there's no benefit in
> providing the flexibility,.
> 
yes, all ports share same interrupt line, but each port has its own
irq related handling register, so it make sense to describe as per gpio irqchip

also see comments below

> > +properties:
> > +  $nodename:
> > +    pattern: "^gpio@[0-9a-f]+$"
> > +
> > +  compatible:
> > +    const: spacemit,k1-gpio
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^gpio-port@[0-9a-f]+$":
> > +    type: object
> > +    properties:
> > +      compatible:
> > +        const: spacemit,k1-gpio-port
> > +
> > +      reg:
> > +        maxItems: 1
> > +
> > +      gpio-controller: true
> > +
> > +      "#gpio-cells":
> > +        const: 2
> > +
> > +      gpio-ranges: true
> > +
> > +      interrupts:
> > +        maxItems: 1
> > +
> > +      interrupt-controller: true
> > +
> > +      "#interrupt-cells":
> > +        const: 2
> > +        description:
> > +          The first cell is the GPIO number, the second should specify interrupt
> > +          flag. The controller does not support level interrupts, so flags of
> > +          IRQ_TYPE_LEVEL_HIGH, IRQ_TYPE_LEVEL_LOW should not be used.
> > +          Refer <dt-bindings/interrupt-controller/irq.h> for valid flags.
> 
> Same here, since there's no real flexibility between the banks, it might
> make sense to consider a 3-cell GPIO specifier instead, and having
how to handle the fourth gpio port? I would like to have uniform driver for all ports

> the first cell indicate bank. I could see this argument go in either
> direction, but I'm not sure I understand why to provide a gpio-controller
> per bank.
> 

IIUC, your suggestion here was same as the implementation of patch v3 of this driver[1],
 while combining all four ports into one irqchip, which NACKed by maintainer[2].
 I tend to agree having a gpio-controller per bank provide more flexibility,
 easy to leverage generic gpio framework, even each port can be disabled or enabled,
 and IMO having shared irq handler isn't really a problem..

[1] https://lore.kernel.org/r/[e-mail]
[2] https://lore.kernel.org/r/[e-mail]
https://lore.kernel.org/r/CACRpkdYgGho=[e-mail]


> Comparing to say Rockchip, where each bank has a separate interrupt line
> -- so there the granularity makes sense.
> 
> 
> -Olof

Olof Johansson Jan. 23, 2025, 11:19 p.m. UTC | #3

On Thu, Jan 23, 2025 at 11:30:42AM +0000, Yixun Lan wrote:
> Hi Olof:
>  thanks for your reivew
> 
> On 12:03 Wed 22 Jan     , Olof Johansson wrote:
> > Hi,
> > 
> > On Tue, Jan 21, 2025 at 11:38:11AM +0800, Yixun Lan wrote:
> > > The GPIO controller of K1 support basic functions as input/output,
> > > all pins can be used as interrupt which route to one IRQ line,
> > > trigger type can be select between rising edge, failing edge, or both.
> > > There are four GPIO ports, each consisting of 32 pins.
> > > 
> > > Signed-off-by: Yixun Lan <[e-mail]>
> > > ---
> > >  .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
> > >  1 file changed, 116 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..dd9459061aecfcba84e6a3c5052fbcddf6c61150
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > > @@ -0,0 +1,116 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/gpio/spacemit,k1-gpio.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: SpacemiT K1 GPIO controller
> > > +
> > > +maintainers:
> > > +  - Yixun Lan <[e-mail]>
> > > +
> > > +description:
> > > +  The controller's registers are organized as sets of eight 32-bit
> > > +  registers with each set of port controlling 32 pins.  A single
> > > +  interrupt line is shared for all of the pins by the controller.
> > > +  Each port will be represented as child nodes with the generic
> > > +  GPIO-controller properties in this bindings file.
> > 
> > There's only one interrupt line for all ports, but you have a binding that
> > duplicates them for every set of ports. That seems overly complicated,
> > doesn't it? They'd all bind the same handler, so there's no benefit in
> > providing the flexibility,.
> > 
> yes, all ports share same interrupt line, but each port has its own
> irq related handling register, so it make sense to describe as per gpio irqchip
> 
> also see comments below
> 
> > > +properties:
> > > +  $nodename:
> > > +    pattern: "^gpio@[0-9a-f]+$"
> > > +
> > > +  compatible:
> > > +    const: spacemit,k1-gpio
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  "#address-cells":
> > > +    const: 1
> > > +
> > > +  "#size-cells":
> > > +    const: 0
> > > +
> > > +patternProperties:
> > > +  "^gpio-port@[0-9a-f]+$":
> > > +    type: object
> > > +    properties:
> > > +      compatible:
> > > +        const: spacemit,k1-gpio-port
> > > +
> > > +      reg:
> > > +        maxItems: 1
> > > +
> > > +      gpio-controller: true
> > > +
> > > +      "#gpio-cells":
> > > +        const: 2
> > > +
> > > +      gpio-ranges: true
> > > +
> > > +      interrupts:
> > > +        maxItems: 1
> > > +
> > > +      interrupt-controller: true
> > > +
> > > +      "#interrupt-cells":
> > > +        const: 2
> > > +        description:
> > > +          The first cell is the GPIO number, the second should specify interrupt
> > > +          flag. The controller does not support level interrupts, so flags of
> > > +          IRQ_TYPE_LEVEL_HIGH, IRQ_TYPE_LEVEL_LOW should not be used.
> > > +          Refer <dt-bindings/interrupt-controller/irq.h> for valid flags.
> > 
> > Same here, since there's no real flexibility between the banks, it might
> > make sense to consider a 3-cell GPIO specifier instead, and having
> how to handle the fourth gpio port? I would like to have uniform driver for all ports
> 
> > the first cell indicate bank. I could see this argument go in either
> > direction, but I'm not sure I understand why to provide a gpio-controller
> > per bank.
> > 
> 
> IIUC, your suggestion here was same as the implementation of patch v3 of this driver[1],
>  while combining all four ports into one irqchip, which NACKed by maintainer[2].
>  I tend to agree having a gpio-controller per bank provide more flexibility,
>  easy to leverage generic gpio framework, even each port can be disabled or enabled,
>  and IMO having shared irq handler isn't really a problem..
> 
> [1] https://lore.kernel.org/r/[e-mail]
> [2] https://lore.kernel.org/r/[e-mail]
> https://lore.kernel.org/r/CACRpkdYgGho=[e-mail]

Hmm, I don't understand the reasoning there, but it's not my subsystem.

It seems worse to me to misdescribe the hardware as separate blocks
with a device-tree binding that no longer describes the actual hardware,
but it's not up to me.

Let's get the platform support merged, ignore my feedback here -- we need more
SoCs supported upstream and this code is good enough to go in as-is.


-Olof

Rob Herring Jan. 27, 2025, 6:17 p.m. UTC | #4

On Thu, Jan 23, 2025 at 03:19:18PM -0800, Olof Johansson wrote:
> On Thu, Jan 23, 2025 at 11:30:42AM +0000, Yixun Lan wrote:
> > Hi Olof:
> >  thanks for your reivew
> > 
> > On 12:03 Wed 22 Jan     , Olof Johansson wrote:
> > > Hi,
> > > 
> > > On Tue, Jan 21, 2025 at 11:38:11AM +0800, Yixun Lan wrote:
> > > > The GPIO controller of K1 support basic functions as input/output,
> > > > all pins can be used as interrupt which route to one IRQ line,
> > > > trigger type can be select between rising edge, failing edge, or both.
> > > > There are four GPIO ports, each consisting of 32 pins.
> > > > 
> > > > Signed-off-by: Yixun Lan <[e-mail]>
> > > > ---
> > > >  .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
> > > >  1 file changed, 116 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..dd9459061aecfcba84e6a3c5052fbcddf6c61150
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > > > @@ -0,0 +1,116 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/gpio/spacemit,k1-gpio.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: SpacemiT K1 GPIO controller
> > > > +
> > > > +maintainers:
> > > > +  - Yixun Lan <[e-mail]>
> > > > +
> > > > +description:
> > > > +  The controller's registers are organized as sets of eight 32-bit
> > > > +  registers with each set of port controlling 32 pins.  A single
> > > > +  interrupt line is shared for all of the pins by the controller.
> > > > +  Each port will be represented as child nodes with the generic
> > > > +  GPIO-controller properties in this bindings file.
> > > 
> > > There's only one interrupt line for all ports, but you have a binding that
> > > duplicates them for every set of ports. That seems overly complicated,
> > > doesn't it? They'd all bind the same handler, so there's no benefit in
> > > providing the flexibility,.
> > > 
> > yes, all ports share same interrupt line, but each port has its own
> > irq related handling register, so it make sense to describe as per gpio irqchip
> > 
> > also see comments below
> > 
> > > > +properties:
> > > > +  $nodename:
> > > > +    pattern: "^gpio@[0-9a-f]+$"
> > > > +
> > > > +  compatible:
> > > > +    const: spacemit,k1-gpio
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  "#address-cells":
> > > > +    const: 1
> > > > +
> > > > +  "#size-cells":
> > > > +    const: 0
> > > > +
> > > > +patternProperties:
> > > > +  "^gpio-port@[0-9a-f]+$":
> > > > +    type: object
> > > > +    properties:
> > > > +      compatible:
> > > > +        const: spacemit,k1-gpio-port
> > > > +
> > > > +      reg:
> > > > +        maxItems: 1
> > > > +
> > > > +      gpio-controller: true
> > > > +
> > > > +      "#gpio-cells":
> > > > +        const: 2
> > > > +
> > > > +      gpio-ranges: true
> > > > +
> > > > +      interrupts:
> > > > +        maxItems: 1
> > > > +
> > > > +      interrupt-controller: true
> > > > +
> > > > +      "#interrupt-cells":
> > > > +        const: 2
> > > > +        description:
> > > > +          The first cell is the GPIO number, the second should specify interrupt
> > > > +          flag. The controller does not support level interrupts, so flags of
> > > > +          IRQ_TYPE_LEVEL_HIGH, IRQ_TYPE_LEVEL_LOW should not be used.
> > > > +          Refer <dt-bindings/interrupt-controller/irq.h> for valid flags.
> > > 
> > > Same here, since there's no real flexibility between the banks, it might
> > > make sense to consider a 3-cell GPIO specifier instead, and having
> > how to handle the fourth gpio port? I would like to have uniform driver for all ports
> > 
> > > the first cell indicate bank. I could see this argument go in either
> > > direction, but I'm not sure I understand why to provide a gpio-controller
> > > per bank.
> > > 
> > 
> > IIUC, your suggestion here was same as the implementation of patch v3 of this driver[1],
> >  while combining all four ports into one irqchip, which NACKed by maintainer[2].
> >  I tend to agree having a gpio-controller per bank provide more flexibility,
> >  easy to leverage generic gpio framework, even each port can be disabled or enabled,
> >  and IMO having shared irq handler isn't really a problem..
> > 
> > [1] https://lore.kernel.org/r/[e-mail]
> > [2] https://lore.kernel.org/r/[e-mail]
> > https://lore.kernel.org/r/CACRpkdYgGho=[e-mail]
> 
> Hmm, I don't understand the reasoning there, but it's not my subsystem.
> 
> It seems worse to me to misdescribe the hardware as separate blocks
> with a device-tree binding that no longer describes the actual hardware,
> but it's not up to me.

I agree. It's clearly 1 block given the first 3 banks are interleaved.

If Linux can't handle 1 node for N gpio_chip's, then that's a Linux 
problem. Maybe it can, IDK. The lookup from a DT node to gpio_chip just 
needs to match on more than just DT node pointer, but look at the node 
ptr and arg cells.

Rob

Yixun Lan Jan. 28, 2025, 3:17 a.m. UTC | #5

Hi Rob:

On 12:17 Mon 27 Jan     , Rob Herring wrote:
> On Thu, Jan 23, 2025 at 03:19:18PM -0800, Olof Johansson wrote:
> > On Thu, Jan 23, 2025 at 11:30:42AM +0000, Yixun Lan wrote:
> > > Hi Olof:
> > >  thanks for your reivew
> > > 
> > > On 12:03 Wed 22 Jan     , Olof Johansson wrote:
> > > > Hi,
> > > > 
> > > > On Tue, Jan 21, 2025 at 11:38:11AM +0800, Yixun Lan wrote:
> > > > > The GPIO controller of K1 support basic functions as input/output,
> > > > > all pins can be used as interrupt which route to one IRQ line,
> > > > > trigger type can be select between rising edge, failing edge, or both.
> > > > > There are four GPIO ports, each consisting of 32 pins.
> > > > > 
> > > > > Signed-off-by: Yixun Lan <[e-mail]>
> > > > > ---
> > > > >  .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
> > > > >  1 file changed, 116 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > > > > new file mode 100644
> > > > > index 0000000000000000000000000000000000000000..dd9459061aecfcba84e6a3c5052fbcddf6c61150
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > > > > @@ -0,0 +1,116 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/gpio/spacemit,k1-gpio.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: SpacemiT K1 GPIO controller
> > > > > +
> > > > > +maintainers:
> > > > > +  - Yixun Lan <[e-mail]>
> > > > > +
> > > > > +description:
> > > > > +  The controller's registers are organized as sets of eight 32-bit
> > > > > +  registers with each set of port controlling 32 pins.  A single
> > > > > +  interrupt line is shared for all of the pins by the controller.
> > > > > +  Each port will be represented as child nodes with the generic
> > > > > +  GPIO-controller properties in this bindings file.
> > > > 
> > > > There's only one interrupt line for all ports, but you have a binding that
> > > > duplicates them for every set of ports. That seems overly complicated,
> > > > doesn't it? They'd all bind the same handler, so there's no benefit in
> > > > providing the flexibility,.
> > > > 
> > > yes, all ports share same interrupt line, but each port has its own
> > > irq related handling register, so it make sense to describe as per gpio irqchip
> > > 
> > > also see comments below
> > > 
> > > > > +properties:
> > > > > +  $nodename:
> > > > > +    pattern: "^gpio@[0-9a-f]+$"
> > > > > +
> > > > > +  compatible:
> > > > > +    const: spacemit,k1-gpio
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  "#address-cells":
> > > > > +    const: 1
> > > > > +
> > > > > +  "#size-cells":
> > > > > +    const: 0
> > > > > +
> > > > > +patternProperties:
> > > > > +  "^gpio-port@[0-9a-f]+$":
> > > > > +    type: object
> > > > > +    properties:
> > > > > +      compatible:
> > > > > +        const: spacemit,k1-gpio-port
> > > > > +
> > > > > +      reg:
> > > > > +        maxItems: 1
> > > > > +
> > > > > +      gpio-controller: true
> > > > > +
> > > > > +      "#gpio-cells":
> > > > > +        const: 2
> > > > > +
> > > > > +      gpio-ranges: true
> > > > > +
> > > > > +      interrupts:
> > > > > +        maxItems: 1
> > > > > +
> > > > > +      interrupt-controller: true
> > > > > +
> > > > > +      "#interrupt-cells":
> > > > > +        const: 2
> > > > > +        description:
> > > > > +          The first cell is the GPIO number, the second should specify interrupt
> > > > > +          flag. The controller does not support level interrupts, so flags of
> > > > > +          IRQ_TYPE_LEVEL_HIGH, IRQ_TYPE_LEVEL_LOW should not be used.
> > > > > +          Refer <dt-bindings/interrupt-controller/irq.h> for valid flags.
> > > > 
> > > > Same here, since there's no real flexibility between the banks, it might
> > > > make sense to consider a 3-cell GPIO specifier instead, and having
> > > how to handle the fourth gpio port? I would like to have uniform driver for all ports
> > > 
> > > > the first cell indicate bank. I could see this argument go in either
> > > > direction, but I'm not sure I understand why to provide a gpio-controller
> > > > per bank.
> > > > 
> > > 
> > > IIUC, your suggestion here was same as the implementation of patch v3 of this driver[1],
> > >  while combining all four ports into one irqchip, which NACKed by maintainer[2].
> > >  I tend to agree having a gpio-controller per bank provide more flexibility,
> > >  easy to leverage generic gpio framework, even each port can be disabled or enabled,
> > >  and IMO having shared irq handler isn't really a problem..
> > > 
> > > [1] https://lore.kernel.org/r/[e-mail]
> > > [2] https://lore.kernel.org/r/[e-mail]
> > > https://lore.kernel.org/r/CACRpkdYgGho=[e-mail]
> > 
> > Hmm, I don't understand the reasoning there, but it's not my subsystem.
> > 
> > It seems worse to me to misdescribe the hardware as separate blocks
> > with a device-tree binding that no longer describes the actual hardware,
> > but it's not up to me.
> 
> I agree. It's clearly 1 block given the first 3 banks are interleaved.
> 
yes, it's kind of weird hardware design, the first 3 gpio are register interleaved,
while the 4th has independent space

> If Linux can't handle 1 node for N gpio_chip's, then that's a Linux 
> problem. Maybe it can, IDK. 
I haven't seen somthing like this to register 1 node for multi gpio_chips..
To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?

>The lookup from a DT node to gpio_chip just 
> needs to match on more than just DT node pointer, but look at the node 
> ptr and arg cells.
> 
IIUC, are you suggesting to add a index cells to match additional gpio bank?
so the underlying driver can still register 4 gpio chips?

               gpio: gpio@d4019000 {
                        compatible = "spacemit,k1-gpio";
                        reg = <0x0 0xd4019000 0x0 0x800>;
                        interrupt-controller;
			#interrupt-cells = <3>; // additional cell
                        gpio-controller;
                        #gpio-cells = <3>; // additional cell
			...
		};

on comsumer side, it will be something like this:
		gpios = <&gpio INDEX0 0 GPIO_ACTIVE_HIGH>;
		interrupts = <&gpio INDEX0 0 IRQ_TYPE_EDGE_RISING>;
(INDEX0 possiblely can be numeric vale (0, 1, 2, 3) or register base: 0x0 0x4 0x8 0x100)

one thing I'm not sure about how to map the pinctrl pins via "gpio-ranges" to each gpio_chip,
currently, in v4 verion, this info is populated via sub node of gpios (port1, port2 ...)

I will investigate more on this.. but need suggestion to know if I proceed at right direction

Linus Walleij Jan. 28, 2025, 3:47 p.m. UTC | #6

On Mon, Jan 27, 2025 at 7:17 PM Rob Herring <[e-mail]> wrote:
> [Olof]
> > It seems worse to me to misdescribe the hardware as separate blocks
> > with a device-tree binding that no longer describes the actual hardware,
> > but it's not up to me.
>
> I agree. It's clearly 1 block given the first 3 banks are interleaved.
>
> If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
> problem. Maybe it can, IDK. The lookup from a DT node to gpio_chip just
> needs to match on more than just DT node pointer, but look at the node
> ptr and arg cells.

Any operating system benefits from modeling the GPIOs such that
one set of 32bit registers [r0, r1 .. rn] becomes a discrete entity for
the OS.

Reasoning: any OS will want to be able to control several lines in
a single hardware operation, such as a register write, for example
to shake a clock and data line with a single write_to_register()
operation. If the hardware is described in chunks of 32 bit registers,
this is easy - Data Out Register, Data In Register, Direction
Register n bits, if an multiple-write/read operation hits this entity, we
know it can be handled with a single register write or read.

Yes, the same can be achieved by hardcoding this split into the
driver. But having the binding like such encourages it.

foo-gpios = <&gpio2 0>, <&gpio2 7>;

both need to be set high at outset, well they are in the same
entity and controlled by a single register, so (+/- overhead):

fooreg = fooreg | (1 << 0) | (1 << 7);

I agree this hardware is harder to classify as such since the blocks
share a single IRQ line - if they had individual IRQ lines it would be
a done deal, they are subblocks - yet shared IRQ lines are not *that*
uncommon.

Does this modeling reflect how the hardware actually looks? Likely.

The way GPIOs are built up from silicon io-cells are not that complex.
All the 32 bits from the set of registers will be routed to consecutive
pins, looking at the pin layout of the SoC would confirm if subsequent
bits map to subsequent pins.

Yours,
Linus Walleij

Linus Walleij Jan. 28, 2025, 4:03 p.m. UTC | #7

On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <[e-mail]> wrote:

> [Rob]
> > If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
> > problem. Maybe it can, IDK.
>
> I haven't seen somthing like this to register 1 node for multi gpio_chips..
> To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?

For Linux we can call bgpio_init() three times and
devm_gpiochip_add_data() three times on the result and if we use the
approach with three cells (where the second is instance 0,1,2 and the
last one the offset 0..31) then it will work all just the same I guess?

foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;

for offset 7 on block 2 for example.

We need a custom xlate function I suppose.

It just has not been done that way before, everybody just did
2-cell GPIOs.

Yours,
Linus Walleij

Rob Herring Jan. 28, 2025, 4:58 p.m. UTC | #8

On Tue, Jan 28, 2025 at 10:03 AM Linus Walleij <[e-mail]> wrote:
>
> On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <[e-mail]> wrote:
>
> > [Rob]
> > > If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
> > > problem. Maybe it can, IDK.
> >
> > I haven't seen somthing like this to register 1 node for multi gpio_chips..
> > To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?
>
> For Linux we can call bgpio_init() three times and
> devm_gpiochip_add_data() three times on the result and if we use the
> approach with three cells (where the second is instance 0,1,2 and the
> last one the offset 0..31) then it will work all just the same I guess?
>
> foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
>
> for offset 7 on block 2 for example.
>
> We need a custom xlate function I suppose.
>
> It just has not been done that way before, everybody just did
> 2-cell GPIOs.

You can do either 3 cells or 2 cells splitting the 1st cell into
<bank><index>. I'm pretty sure we have some cases of the latter.

Rob

Samuel Holland Jan. 28, 2025, 6:50 p.m. UTC | #9

On 2025-01-28 10:58 AM, Rob Herring wrote:
> On Tue, Jan 28, 2025 at 10:03 AM Linus Walleij <[e-mail]> wrote:
>>
>> On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <[e-mail]> wrote:
>>
>>> [Rob]
>>>> If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
>>>> problem. Maybe it can, IDK.
>>>
>>> I haven't seen somthing like this to register 1 node for multi gpio_chips..
>>> To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?
>>
>> For Linux we can call bgpio_init() three times and
>> devm_gpiochip_add_data() three times on the result and if we use the
>> approach with three cells (where the second is instance 0,1,2 and the
>> last one the offset 0..31) then it will work all just the same I guess?
>>
>> foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
>>
>> for offset 7 on block 2 for example.
>>
>> We need a custom xlate function I suppose.
>>
>> It just has not been done that way before, everybody just did
>> 2-cell GPIOs.
> 
> You can do either 3 cells or 2 cells splitting the 1st cell into
> <bank><index>. I'm pretty sure we have some cases of the latter.

There is also at least one example of 3-cell GPIOs:

Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml

It supports controllers with varying numbers of pins per bank and banks in each
instance. Compared to the design described above, it shares a single gpio_chip
across all banks in a controller instance.

Regards,
Samuel

Patch

diff --git a/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..dd9459061aecfcba84e6a3c5052fbcddf6c61150
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
@@ -0,0 +1,116 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/spacemit,k1-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K1 GPIO controller
+
+maintainers:
+  - Yixun Lan <[e-mail]>
+
+description:
+  The controller's registers are organized as sets of eight 32-bit
+  registers with each set of port controlling 32 pins.  A single
+  interrupt line is shared for all of the pins by the controller.
+  Each port will be represented as child nodes with the generic
+  GPIO-controller properties in this bindings file.
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    const: spacemit,k1-gpio
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^gpio-port@[0-9a-f]+$":
+    type: object
+    properties:
+      compatible:
+        const: spacemit,k1-gpio-port
+
+      reg:
+        maxItems: 1
+
+      gpio-controller: true
+
+      "#gpio-cells":
+        const: 2
+
+      gpio-ranges: true
+
+      interrupts:
+        maxItems: 1
+
+      interrupt-controller: true
+
+      "#interrupt-cells":
+        const: 2
+        description:
+          The first cell is the GPIO number, the second should specify interrupt
+          flag. The controller does not support level interrupts, so flags of
+          IRQ_TYPE_LEVEL_HIGH, IRQ_TYPE_LEVEL_LOW should not be used.
+          Refer <dt-bindings/interrupt-controller/irq.h> for valid flags.
+
+    required:
+      - compatible
+      - reg
+      - gpio-controller
+      - "#gpio-cells"
+
+    dependencies:
+      interrupt-controller: [ interrupts ]
+
+    additionalProperties: false
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+examples:
+  - |
+    gpio: gpio@d4019000 {
+      compatible = "spacemit,k1-gpio";
+      reg = <0xd4019000 0x800>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      port0: gpio-port@0 {
+        compatible = "spacemit,k1-gpio-port";
+        reg = <0>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        interrupt-parent = <&plic>;
+        interrupts = <58>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        gpio-ranges = <&pinctrl 0 0 32>;
+      };
+
+      port1: gpio-port@4 {
+        compatible = "spacemit,k1-gpio-port";
+        reg = <4>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        interrupt-parent = <&plic>;
+        interrupts = <58>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        gpio-ranges = <&pinctrl 0 32 32>;
+      };
+    };
+...