| Message ID | 20250121-03-k1-gpio-v4-1-4641c95c0194@gentoo.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/4] dt-bindings: gpio: spacemit: add support for K1 SoC | expand |
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 <dlan@gentoo.org> > > --- > > .../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 <dlan@gentoo.org> > > + > > +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/20241225-03-k1-gpio-v3-0-27bb7b441d62@gentoo.org [2] https://lore.kernel.org/r/CACRpkdZPD2C2iPwOX_kW1Ug8jVkdHhhc7iFycHtzj5LQ0XWNgQ@mail.gmail.com https://lore.kernel.org/r/CACRpkdYgGho=VQabonq4HccEiXBH2qM76K45oDaV1Jyi0xZ-YA@mail.gmail.com > Comparing to say Rockchip, where each bank has a separate interrupt line > -- so there the granularity makes sense. > > > -Olof
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 <dlan@gentoo.org> > > > --- > > > .../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 <dlan@gentoo.org> > > > + > > > +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/20241225-03-k1-gpio-v3-0-27bb7b441d62@gentoo.org > [2] https://lore.kernel.org/r/CACRpkdZPD2C2iPwOX_kW1Ug8jVkdHhhc7iFycHtzj5LQ0XWNgQ@mail.gmail.com > https://lore.kernel.org/r/CACRpkdYgGho=VQabonq4HccEiXBH2qM76K45oDaV1Jyi0xZ-YA@mail.gmail.com 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
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 <dlan@gentoo.org> > > > > --- > > > > .../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 <dlan@gentoo.org> > > > > + > > > > +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/20241225-03-k1-gpio-v3-0-27bb7b441d62@gentoo.org > > [2] https://lore.kernel.org/r/CACRpkdZPD2C2iPwOX_kW1Ug8jVkdHhhc7iFycHtzj5LQ0XWNgQ@mail.gmail.com > > https://lore.kernel.org/r/CACRpkdYgGho=VQabonq4HccEiXBH2qM76K45oDaV1Jyi0xZ-YA@mail.gmail.com > > 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
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 <dlan@gentoo.org> > > > > > --- > > > > > .../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 <dlan@gentoo.org> > > > > > + > > > > > +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/20241225-03-k1-gpio-v3-0-27bb7b441d62@gentoo.org > > > [2] https://lore.kernel.org/r/CACRpkdZPD2C2iPwOX_kW1Ug8jVkdHhhc7iFycHtzj5LQ0XWNgQ@mail.gmail.com > > > https://lore.kernel.org/r/CACRpkdYgGho=VQabonq4HccEiXBH2qM76K45oDaV1Jyi0xZ-YA@mail.gmail.com > > > > 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
On Mon, Jan 27, 2025 at 7:17 PM Rob Herring <robh@kernel.org> 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
On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> 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
On Tue, Jan 28, 2025 at 10:03 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> 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
On 2025-01-28 10:58 AM, Rob Herring wrote: > On Tue, Jan 28, 2025 at 10:03 AM Linus Walleij <linus.walleij@linaro.org> wrote: >> >> On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> 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
Hi Yixun, On Tue, Jan 28, 2025 at 5:03 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> 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. does this approach work for you? I think it's the most diplomatic. I'm sorry about the hopeless back-and-forth with the bindings, also for contributing to the messy debate. I do want developers to feel encouraged to contribute and not get stuck in too long debates. Yours, Linus Walleij
hi Linus Thanks for the ping.. On 10:18 Thu 06 Feb , Linus Walleij wrote: > Hi Yixun, > > On Tue, Jan 28, 2025 at 5:03 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> 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 yes, even I've already done this in v4 > > 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? > > agree, I just need to connect dots.. parse dts & adjust the driver code > > 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. > > does this approach work for you? I think it's the most diplomatic. > I like the approach which make sense > I'm sorry about the hopeless back-and-forth with the bindings, also > for contributing to the messy debate. I do want developers to feel > encouraged to contribute and not get stuck in too long debates. > no problem, thanks for the encouragement.. I planed to go for the implementation, and raise any actual problem I may find, but it turns out taking more time than I expected (some reason to due long chinese new year holiday..) > Yours, > Linus Walleij
Hi Linus and DT maintainers: On 10:18 Thu 06 Feb , Linus Walleij wrote: > Hi Yixun, > > On Tue, Jan 28, 2025 at 5:03 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> 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? > > both bgpio_init() and devm_gpiochip_add_data() operate on per "struct gpio_chip" bias, which mean they need to request three independent gpio chips.. > > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>; if we model the dts as above, then "&gpio" will register itself as one sole "struct gpio_chip", which mean one gpio chip combine three banks.. I've looked at the sunxi driver which Samuel pointed, imply same example as this. if taking "one gpio chip support multi banks" direction, then it will be reverted back as patch V1, then, even the three gpio-cells model is unnecessary needed, as we can map gpio number to the <bank, offset> array in the underlying gpio driver the v4 patch is very similar to drivers/gpio/gpio-dwapb.c If had to choose the direction between v1 and v4, I personally would favor the latter, as from hw perspective, each gpio bank is quite indepedent - has its own io/irq registers, merely has interleaved io memory space, one shared IRQ line.. also the patch v4 leverage lots underlying generic gpio APIs, result in much simplified/clean code base.. > > > > 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. > > does this approach work for you? I think it's the most diplomatic. > > I'm sorry about the hopeless back-and-forth with the bindings, also > for contributing to the messy debate. I do want developers to feel > encouraged to contribute and not get stuck in too long debates. > > Yours, > Linus Walleij
On Thu, Feb 6, 2025 at 2:32 PM Yixun Lan <dlan@gentoo.org> wrote: > > > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>; > > if we model the dts as above, then "&gpio" will register itself as one sole "struct gpio_chip", > which mean one gpio chip combine three banks.. Not really: the fact that there is just one gpio node in the device tree does not mean that it needs to correspond to one single gpio_chip instance inside the Linux kernel. It's just what the current existing bindings and the code in the GPIO subsystem assumes. It does not have to assume that: we can change it. I'm sorry if this is not entirely intuitive :( One node can very well spawn three gpio_chip instances, but it requires some core changes. But I think it's the most elegant. > if taking "one gpio chip support multi banks" direction, then it will be reverted back as patch V1, > then, even the three gpio-cells model is unnecessary needed, as we can map gpio number > to the <bank, offset> array in the underlying gpio driver > > the v4 patch is very similar to drivers/gpio/gpio-dwapb.c > > If had to choose the direction between v1 and v4, I personally would favor the latter, > as from hw perspective, each gpio bank is quite indepedent - has its own io/irq registers, > merely has interleaved io memory space, one shared IRQ line.. also the patch v4 leverage > lots underlying generic gpio APIs, result in much simplified/clean code base.. So what I would suggest is a combination of the two. One gpio node in the device tree, like the DT maintainers want it. Three struct gpio_chip instances inside the driver, all three spawn from that single gpio device, and from that single platform_device. What we are suggesting is a three-cell phandle in the device tree: foo-gpios = <&gpio 0 7 GPIO_ACTIVE_HIGH>; bar-gpios = <&gpio 2 31 GPIO_ACTIVE_HIGH>; Notice the new first cell which is 0 or 2. The first one is what was previously called gpio 7. The second one is what was 2*32+31 = gpio 95. So internally in the driver it is easy to use the first cell (0 or 2) to map to the right struct gpio_chip if you have it in your driver something like this: struct spacemit_gpio { struct gpio_chip gcs[3]; ... }; struct spacemit_gpio *sg; struct gpio_chip *gc; int ret; for (i = 0; i++; i < 3) { ret = devm_gpiochip_add_data(dev, &sg->gcs[i], sg); if (ret) return ret; gc = sg->gcs[i]; .... do stuff with this instance .... } Callbacks etc should work as before. Then these phandles needs to be properly translated, which is done with the struct gpio_chip .of_xlate() callback. (If you look inside gpiolib-of.c you will see that chip->of_xlate() is called to map the phandle cells to a certain GPIO line). In most cases, drivers do not assign the chip->of_xlate callback (one exception is gpio-pxa.c) and then it is default-assigned to of_gpio_simple_xlate() which you can find in gpiolib-of.c as well. You need to copy this callback to your driver and augment it properly. The xlate callback is used to locate the struct gpio_chip and struct gpio_device as well, by just calling the xlate callback, so if you code up the right xlate callback, everything should just work by itself. this is a guess on what it would look like (just dry coding, but hopefully the idea works!): static int spacemit_gpio_xlate(struct gpio_chip *gc, const struct of_phandle_args *gpiospec, u32 *flags) { struct spacemit_gpio *sg = gpiochip_get_data(gc); int i; if (gc->of_gpio_n_cells != 3) return -EINVAL; if (gpiospec->args_count < gc->of_gpio_n_cells) return -EINVAL; /* We support maximum 3 gpio_chip instances */ i = gpiospec->args[0]; if (i >= 3) return -EINVAL; /* OK is this the right gpio_chip out of the three ? */ if (gc != sg->gcs[i]) return -EINVAL; /* Are we in range for this GPIO chip */ if (gpiospec->args[1] >= gc->ngpio) return -EINVAL; if (flags) *flags = gpiospec->args[2]; /* Return the hw index */ return gpiospec->args[1]; } ... gc->of_gpio_n_cells = 3; gc->of_xlate = spacemit_gpio_xlate; If it works as I hope, this will make the code in gpiolib-of.c in of_find_gpio_device_by_xlate() calling gpio_device_find() (which will iterate over all registered gpio_chips and then of_gpiochip_match_node_and_xlate() will call this custom function to see if it's the right one and return > 0 when we have the right chip. This should work for gpios *only*. When we then come to irqs, these assume (see gpiolib.c) that we are using irq_domain_xlate_twocell() when using GPIOLIB_IRQCHIP, so you either need to roll your own irqchip code or we should fix the core (I can help with this if the above works). Several gpio chips use their own domain translation outside of the gpiolib so you can use this as an intermediate step: git grep irq_domain_ops drivers/gpio/ ... but if you get here, let's patch the core to deal with custom irqdomain xlate functions in the same manner as above. I hope this isn't terribly unclear or complicated? Otherwise tell me and I will try to ... explain more or give up and say you can use a single 96-pin gpio_chip. Yours, Linus Walleij
Hi Linus: On 14:07 Thu 13 Feb , Linus Walleij wrote: > On Thu, Feb 6, 2025 at 2:32 PM Yixun Lan <dlan@gentoo.org> wrote: > > > > > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>; > > > > if we model the dts as above, then "&gpio" will register itself as one sole "struct gpio_chip", > > which mean one gpio chip combine three banks.. > > Not really: the fact that there is just one gpio node in the device > tree does not > mean that it needs to correspond to one single gpio_chip instance inside the > Linux kernel. > > It's just what the current existing bindings and the code in the GPIO subsystem > assumes. It does not have to assume that: we can change it. > > I'm sorry if this is not entirely intuitive :( > > One node can very well spawn three gpio_chip instances, but it requires > some core changes. But I think it's the most elegant. > > > if taking "one gpio chip support multi banks" direction, then it will be reverted back as patch V1, > > then, even the three gpio-cells model is unnecessary needed, as we can map gpio number > > to the <bank, offset> array in the underlying gpio driver > > > > the v4 patch is very similar to drivers/gpio/gpio-dwapb.c > > > > If had to choose the direction between v1 and v4, I personally would favor the latter, > > as from hw perspective, each gpio bank is quite indepedent - has its own io/irq registers, > > merely has interleaved io memory space, one shared IRQ line.. also the patch v4 leverage > > lots underlying generic gpio APIs, result in much simplified/clean code base.. > > So what I would suggest is a combination of the two. > > One gpio node in the device tree, like the DT maintainers want it. > > Three struct gpio_chip instances inside the driver, all three spawn from > that single gpio device, and from that single platform_device. > > What we are suggesting is a three-cell phandle in the device tree: > > foo-gpios = <&gpio 0 7 GPIO_ACTIVE_HIGH>; > bar-gpios = <&gpio 2 31 GPIO_ACTIVE_HIGH>; > > Notice the new first cell which is 0 or 2. > > The first one is what was previously called gpio 7. > The second one is what was 2*32+31 = gpio 95. > > So internally in the driver it is easy to use the first cell (0 or 2) to map to > the right struct gpio_chip if you have it in your driver something like this: > > struct spacemit_gpio { > struct gpio_chip gcs[3]; > ... > }; > > struct spacemit_gpio *sg; > struct gpio_chip *gc; > int ret; > > for (i = 0; i++; i < 3) { > ret = devm_gpiochip_add_data(dev, &sg->gcs[i], sg); > if (ret) > return ret; > gc = sg->gcs[i]; > .... do stuff with this instance .... > } > > Callbacks etc should work as before. > > Then these phandles needs to be properly translated, which is done with the > struct gpio_chip .of_xlate() callback. (If you look inside gpiolib-of.c > you will see that chip->of_xlate() is called to map the phandle cells > to a certain GPIO line). > > In most cases, drivers do not assign the chip->of_xlate callback > (one exception is gpio-pxa.c) and then it is default-assigned to > of_gpio_simple_xlate() which you can find in gpiolib-of.c as well. > > You need to copy this callback to your driver and augment it > properly. > > The xlate callback is used to locate the struct gpio_chip and > struct gpio_device as well, by just calling the xlate callback, so if > you code up the right xlate callback, everything should just > work by itself. > > this is a guess on what it would look like (just dry coding, > but hopefully the idea works!): > > static int spacemit_gpio_xlate(struct gpio_chip *gc, > const struct of_phandle_args *gpiospec, > u32 *flags) > { > struct spacemit_gpio *sg = gpiochip_get_data(gc); > int i; > > if (gc->of_gpio_n_cells != 3) > return -EINVAL; > > if (gpiospec->args_count < gc->of_gpio_n_cells) > return -EINVAL; > > /* We support maximum 3 gpio_chip instances */ > i = gpiospec->args[0]; > if (i >= 3) > return -EINVAL; > > /* OK is this the right gpio_chip out of the three ? */ > if (gc != sg->gcs[i]) > return -EINVAL; > > /* Are we in range for this GPIO chip */ > if (gpiospec->args[1] >= gc->ngpio) > return -EINVAL; > > if (flags) > *flags = gpiospec->args[2]; > > /* Return the hw index */ > return gpiospec->args[1]; > } > thanks for this very detail prototype! it works mostly, with one problem: how to map gpio correctly to the pin from pinctrl subsystem? for example, I specify gpio-ranges in dts, then gpio0: gpio@d4019000 { compatible = "spacemit,k1-gpio"; reg = <0x0 0xd4019000 0x0 0x100>; ... gpio-ranges = <&pinctrl 0 0 96>; }; foo-gpios = <&gpio0 2 28 GPIO_ACTIVE_LOW>; It should get GPIO_92 ( 92 = 2 * 32 + 28), but turns out GPIO_28 Probably there is something I missed... > ... > gc->of_gpio_n_cells = 3; > gc->of_xlate = spacemit_gpio_xlate; > > If it works as I hope, this will make the code in gpiolib-of.c in > of_find_gpio_device_by_xlate() calling gpio_device_find() > (which will iterate over all registered gpio_chips and then > of_gpiochip_match_node_and_xlate() will call this custom function > to see if it's the right one and return > 0 when we have the right > chip. > > This should work for gpios *only*. When we then come to irqs, > these assume (see gpiolib.c) that we are using > irq_domain_xlate_twocell() when using GPIOLIB_IRQCHIP, so > you either need to roll your own irqchip code or we should fix Sounds I should implement something like irq_domain_xlate_threecell()? > the core (I can help with this if the above works). > > Several gpio chips use their own domain translation outside > of the gpiolib so you can use this as an intermediate step: > git grep irq_domain_ops drivers/gpio/ .. > ... but if you get here, let's patch the core to deal with custom > irqdomain xlate functions in the same manner as above. > I like this direction, but how we should proceed? > I hope this isn't terribly unclear or complicated? > Otherwise tell me and I will try to ... explain more or give > up and say you can use a single 96-pin gpio_chip. > Let's try first, sounds it's a feasible way. Many thanks! > Yours, > Linus Walleij
Hi Linus: On 11:54 Fri 14 Feb , Yixun Lan wrote: > Hi Linus: > > On 14:07 Thu 13 Feb , Linus Walleij wrote: > > On Thu, Feb 6, 2025 at 2:32 PM Yixun Lan <dlan@gentoo.org> wrote: > > > > > > > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>; > > > > > > if we model the dts as above, then "&gpio" will register itself as one sole "struct gpio_chip", > > > which mean one gpio chip combine three banks.. > > > > Not really: the fact that there is just one gpio node in the device > > tree does not > > mean that it needs to correspond to one single gpio_chip instance inside the > > Linux kernel. > > > > It's just what the current existing bindings and the code in the GPIO subsystem > > assumes. It does not have to assume that: we can change it. > > > > I'm sorry if this is not entirely intuitive :( > > > > One node can very well spawn three gpio_chip instances, but it requires > > some core changes. But I think it's the most elegant. > > > > > if taking "one gpio chip support multi banks" direction, then it will be reverted back as patch V1, > > > then, even the three gpio-cells model is unnecessary needed, as we can map gpio number > > > to the <bank, offset> array in the underlying gpio driver > > > > > > the v4 patch is very similar to drivers/gpio/gpio-dwapb.c > > > > > > If had to choose the direction between v1 and v4, I personally would favor the latter, > > > as from hw perspective, each gpio bank is quite indepedent - has its own io/irq registers, > > > merely has interleaved io memory space, one shared IRQ line.. also the patch v4 leverage > > > lots underlying generic gpio APIs, result in much simplified/clean code base.. > > > > So what I would suggest is a combination of the two. > > > > One gpio node in the device tree, like the DT maintainers want it. > > > > Three struct gpio_chip instances inside the driver, all three spawn from > > that single gpio device, and from that single platform_device. > > > > What we are suggesting is a three-cell phandle in the device tree: > > > > foo-gpios = <&gpio 0 7 GPIO_ACTIVE_HIGH>; > > bar-gpios = <&gpio 2 31 GPIO_ACTIVE_HIGH>; > > > > Notice the new first cell which is 0 or 2. > > > > The first one is what was previously called gpio 7. > > The second one is what was 2*32+31 = gpio 95. > > > > So internally in the driver it is easy to use the first cell (0 or 2) to map to > > the right struct gpio_chip if you have it in your driver something like this: > > > > struct spacemit_gpio { > > struct gpio_chip gcs[3]; > > ... > > }; > > > > struct spacemit_gpio *sg; > > struct gpio_chip *gc; > > int ret; > > > > for (i = 0; i++; i < 3) { > > ret = devm_gpiochip_add_data(dev, &sg->gcs[i], sg); > > if (ret) > > return ret; > > gc = sg->gcs[i]; > > .... do stuff with this instance .... > > } > > > > Callbacks etc should work as before. > > > > Then these phandles needs to be properly translated, which is done with the > > struct gpio_chip .of_xlate() callback. (If you look inside gpiolib-of.c > > you will see that chip->of_xlate() is called to map the phandle cells > > to a certain GPIO line). > > > > In most cases, drivers do not assign the chip->of_xlate callback > > (one exception is gpio-pxa.c) and then it is default-assigned to > > of_gpio_simple_xlate() which you can find in gpiolib-of.c as well. > > > > You need to copy this callback to your driver and augment it > > properly. > > > > The xlate callback is used to locate the struct gpio_chip and > > struct gpio_device as well, by just calling the xlate callback, so if > > you code up the right xlate callback, everything should just > > work by itself. > > > > this is a guess on what it would look like (just dry coding, > > but hopefully the idea works!): > > > > static int spacemit_gpio_xlate(struct gpio_chip *gc, > > const struct of_phandle_args *gpiospec, > > u32 *flags) > > { > > struct spacemit_gpio *sg = gpiochip_get_data(gc); > > int i; > > > > if (gc->of_gpio_n_cells != 3) > > return -EINVAL; > > > > if (gpiospec->args_count < gc->of_gpio_n_cells) > > return -EINVAL; > > > > /* We support maximum 3 gpio_chip instances */ > > i = gpiospec->args[0]; > > if (i >= 3) > > return -EINVAL; > > > > /* OK is this the right gpio_chip out of the three ? */ > > if (gc != sg->gcs[i]) > > return -EINVAL; > > > > /* Are we in range for this GPIO chip */ > > if (gpiospec->args[1] >= gc->ngpio) > > return -EINVAL; > > > > if (flags) > > *flags = gpiospec->args[2]; > > > > /* Return the hw index */ > > return gpiospec->args[1]; > > } > > > thanks for this very detail prototype! it works mostly, with one problem: > > how to map gpio correctly to the pin from pinctrl subsystem? > > for example, I specify gpio-ranges in dts, then > gpio0: gpio@d4019000 { > compatible = "spacemit,k1-gpio"; > reg = <0x0 0xd4019000 0x0 0x100>; > ... > gpio-ranges = <&pinctrl 0 0 96>; > }; > > foo-gpios = <&gpio0 2 28 GPIO_ACTIVE_LOW>; > > It should get GPIO_92 ( 92 = 2 * 32 + 28), but turns out GPIO_28 > > Probably there is something I missed... to make the gpio part work, we need additional custom gpio-ranges parser, which should similar to of_gpiochip_add_pin_range() in gpiolib-of.c (at least gpio core need to adjust to call custom this function)
On Fri, Feb 14, 2025 at 12:54 PM Yixun Lan <dlan@gentoo.org> wrote: > thanks for this very detail prototype! it works mostly, with one problem: > > how to map gpio correctly to the pin from pinctrl subsystem? > > for example, I specify gpio-ranges in dts, then > gpio0: gpio@d4019000 { > compatible = "spacemit,k1-gpio"; > reg = <0x0 0xd4019000 0x0 0x100>; > ... > gpio-ranges = <&pinctrl 0 0 96>; > }; > > foo-gpios = <&gpio0 2 28 GPIO_ACTIVE_LOW>; > > It should get GPIO_92 ( 92 = 2 * 32 + 28), but turns out GPIO_28 > > Probably there is something I missed... No it's just me missing the complexity! > to make the gpio part work, we need additional custom gpio-ranges parser, > which should similar to of_gpiochip_add_pin_range() in gpiolib-of.c > (at least gpio core need to adjust to call custom this function) Let me send a patch set to bring threecell into the core instead, and see if it works for you! I will post it real soon. Yours, Linus Walleij
Hi Linus: On 10:44 Tue 18 Feb , Linus Walleij wrote: > On Fri, Feb 14, 2025 at 12:54 PM Yixun Lan <dlan@gentoo.org> wrote: > > > thanks for this very detail prototype! it works mostly, with one problem: > > > > how to map gpio correctly to the pin from pinctrl subsystem? > > > > for example, I specify gpio-ranges in dts, then > > gpio0: gpio@d4019000 { > > compatible = "spacemit,k1-gpio"; > > reg = <0x0 0xd4019000 0x0 0x100>; > > ... > > gpio-ranges = <&pinctrl 0 0 96>; > > }; > > > > foo-gpios = <&gpio0 2 28 GPIO_ACTIVE_LOW>; > > > > It should get GPIO_92 ( 92 = 2 * 32 + 28), but turns out GPIO_28 > > > > Probably there is something I missed... > > No it's just me missing the complexity! > > > to make the gpio part work, we need additional custom gpio-ranges parser, > > which should similar to of_gpiochip_add_pin_range() in gpiolib-of.c > > (at least gpio core need to adjust to call custom this function) > > Let me send a patch set to bring threecell into the core instead, > and see if it works for you! > > I will post it real soon. > can you check the v5 of the patch here [1]? which I just sent out yesterday it does 1) implement xlate() 2) instroduce custom add_pin_page() the gpio part works as I tested, the gpio irq probably need more testing > Yours, > Linus Walleij [1] https://lore.kernel.org/spacemit/20250217-03-k1-gpio-v5-0-2863ec3e7b67@gentoo.org/
On Tue, Feb 18, 2025 at 10:55 AM Yixun Lan <dlan@gentoo.org> wrote: > > I will post it real soon. > > > can you check the v5 of the patch here [1]? which I just sent out yesterday > it does 1) implement xlate() 2) instroduce custom add_pin_page() > the gpio part works as I tested, the gpio irq probably need more testing Ah nice! I have the same idea, but I just bring all the stuff you need to reimplement in your driver into the core instead. Your driver and bindings will look the same, you will just do not need to reimplement the translation functions (if my code works as I intended...) Yours, Linus Walleij
Hi Linus: On 11:17 Tue 18 Feb , Linus Walleij wrote: > On Tue, Feb 18, 2025 at 10:55 AM Yixun Lan <dlan@gentoo.org> wrote: > > > > I will post it real soon. > > > > > can you check the v5 of the patch here [1]? which I just sent out yesterday > > it does 1) implement xlate() 2) instroduce custom add_pin_page() > > the gpio part works as I tested, the gpio irq probably need more testing > > Ah nice! I have the same idea, but I just bring all the stuff you > need to reimplement in your driver into the core instead. > > Your driver and bindings will look the same, you will just do > not need to reimplement the translation functions (if my code > works as I intended...) > great! I will test and let you know if it works, many thanks.. > Yours, > Linus Walleij
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 <dlan@gentoo.org> + +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>; + }; + }; +...
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 <dlan@gentoo.org> --- .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++ 1 file changed, 116 insertions(+)