| Message ID | 1447982995-30231-2-git-send-email-vz@mleia.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 20, 2015 at 03:29:52AM +0200, Vladimir Zapolskiy wrote: > For the purpose of better description of NXP LPC32xx GPIO controller > hardware in device tree format, extend the existing description with > device tree subnodes, which represent 6 GPIO banks within the > controller. > > Note, client interface to the GPIO controller is untouched. > > Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> > --- > .../devicetree/bindings/gpio/gpio_lpc32xx.txt | 121 ++++++++++++++++++++- > 1 file changed, 120 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt > index 4981936..d2da63c 100644 > --- a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt > +++ b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt > @@ -15,7 +15,43 @@ Required properties: > 2) pin number > 3) optional parameters: > - bit 0 specifies polarity (0 for normal, 1 for inverted) > -- reg: Index of the GPIO group > +- #address-cells: should be 2, which stands for GPIO bank id and > + physical base address of this GPIO bank. Now you need special code to do address translation. I'd really think twice about doing this. Why do you need the bank number? > +- #size-cells: should be 1, total size of GPIO bank registers. > + > +The NXP LPC32xx SoC GPIO controller device node must contain a list > +of device nodes representing GPIO banks and their descriptions. > + > +The format of subnodes should follow the description below. > + > +Required properties: > +- reg: should contain 3 integer values: > + 1) GPIO bank id from 0 to 5, > + 2) physical base address of this GPIO bank, > + 3) total size of the GPIO bank registers. > + > +Optional properties: > +- gpio-bank-name: human readable name of a GPIO bank, > +- gpio-no-output-state: property of P2 bank, which has special, > + mapping of its control registers, > +- gpio-offset: property of P3/GPIO bank, offset of bits representing > + GPIO lines in output and direction registers, Seems like nr-gpios should have been a mask instead... > +- gpios: number of GPIO lines per GPIO bank, if this property is > + omitted, then gpio-input-mask must be present, "gpios" is already the property name for the client interface. > +- gpio-input-mask: should contain two bitmasks, the first bitmask is > + the mapping of GPIO lines to input status register, the second > + bitmask should be a subset of the first bitmask and it represents > + input GPIO lines, which may serve as an interrupt source, > + if gpio-input-mask roperty is omitted, gpios property should be > + present, > +- interrupts: list of parent interrupts mapped to input GPIO lines, > +- interrupts-extended: list of parent interrupts mapped to input GPIO > + lines, used if parent interrupts are provided by more than one > + interrupt controller, this option is used by GPI bank, > +- interrupt-controller: indicates that GPIO bank may serve as an > + interrupt controller, > +- #interrupt-cells: if interrupt-controller property is present, > + it should be 2, interrupt id and its flags. > > Example: > > @@ -24,6 +60,89 @@ Example: > reg = <0x40028000 0x1000>; > gpio-controller; > #gpio-cells = <3>; /* bank, pin, flags */ Can't bank and pin be encoded into one cell as the gpio core binding suggests. > + > + ranges = <0 0x0 0x40028000 0x00001000>, > + <1 0x0 0x40028000 0x00001000>, > + <2 0x0 0x40028000 0x00001000>, > + <3 0x0 0x40028000 0x00001000>, > + <4 0x0 0x40028000 0x00001000>, > + <5 0x0 0x40028000 0x00001000>; > + #address-cells = <2>; > + #size-cells = <1>; > + > + gpio_p0: gpio-controller@0 { > + reg = <0 0x40 0x1C>; > + gpio-bank-name = "p0"; > + gpios = <8>; > + > + interrupt-parent = <&sic2>; > + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; > + }; > + > + gpio_p1: gpio-controller@1 { > + reg = <1 0x60 0x1C>; > + gpio-bank-name = "p1"; > + gpios = <24>; > + > + interrupt-parent = <&sic2>; > + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; > + }; > + > + gpio_p2: gpio-controller@2 { > + reg = <2 0x10 0x18>; > + gpio-bank-name = "p2"; > + gpios = <13>; > + gpio-no-output-state; > + }; > + > + gpio_gpio: gpio-controller@3 { > + reg = <3 0x00 0x1C>; This overlaps with bank 2. > + gpio-bank-name = "gpio"; > + gpio-offset = <25>; > + gpio-input-mask = <0x01007c00>, <0x01007c00>; > + > + interrupt-parent = <&sic2>; > + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>, > + <1 IRQ_TYPE_LEVEL_HIGH>, > + <2 IRQ_TYPE_LEVEL_HIGH>, > + <3 IRQ_TYPE_LEVEL_HIGH>, > + <4 IRQ_TYPE_LEVEL_HIGH>, > + <5 IRQ_TYPE_LEVEL_HIGH>; > + > + interrupt-controller; > + #interrupt-cells = <2>; > + }; > + > + gpio_gpi: gpio-controller@4 { > + reg = <4 0x00 0x04>; > + gpio-bank-name = "gpi"; > + gpio-input-only; > + gpio-input-mask = <0x1aff83ff>, <0x100803ff>; > + > + interrupts-extended = > + <&sic2 22 IRQ_TYPE_LEVEL_HIGH>, > + <&sic2 23 IRQ_TYPE_LEVEL_HIGH>, > + <&sic2 24 IRQ_TYPE_LEVEL_HIGH>, > + <&sic2 25 IRQ_TYPE_LEVEL_HIGH>, > + <&sic2 26 IRQ_TYPE_LEVEL_HIGH>, > + <&sic2 27 IRQ_TYPE_LEVEL_HIGH>, > + <&sic2 28 IRQ_TYPE_LEVEL_HIGH>, > + <&sic2 15 IRQ_TYPE_LEVEL_HIGH>, > + <&sic2 9 IRQ_TYPE_LEVEL_HIGH>, > + <&sic2 10 IRQ_TYPE_LEVEL_HIGH>, > + <&sic2 11 IRQ_TYPE_LEVEL_HIGH>, > + <&sic1 4 IRQ_TYPE_LEVEL_HIGH>; > + > + interrupt-controller; > + #interrupt-cells = <2>; > + }; > + > + gpio_gpo: gpio-controller@5 { > + reg = <5 0x00 0x10>; > + gpio-bank-name = "gpo"; > + gpios = <24>; > + gpio-output-only; > + }; > }; > > leds { > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20.11.2015 16:13, Rob Herring wrote: > On Fri, Nov 20, 2015 at 03:29:52AM +0200, Vladimir Zapolskiy wrote: >> For the purpose of better description of NXP LPC32xx GPIO controller >> hardware in device tree format, extend the existing description with >> device tree subnodes, which represent 6 GPIO banks within the >> controller. >> >> Note, client interface to the GPIO controller is untouched. >> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> >> --- >> .../devicetree/bindings/gpio/gpio_lpc32xx.txt | 121 ++++++++++++++++++++- >> 1 file changed, 120 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt >> index 4981936..d2da63c 100644 >> --- a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt >> +++ b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt >> @@ -15,7 +15,43 @@ Required properties: >> 2) pin number >> 3) optional parameters: >> - bit 0 specifies polarity (0 for normal, 1 for inverted) >> -- reg: Index of the GPIO group >> +- #address-cells: should be 2, which stands for GPIO bank id and >> + physical base address of this GPIO bank. > > Now you need special code to do address translation. I'd really think > twice about doing this. Correct, address translation code is needed here... > Why do you need the bank number? Only one reason -- backward compatibility in sense of referencing a GPIO line on client's side. This API design is broken, I agree. Honestly I would prefer to get rid of this "feature", new code allows to reference on client's side either a parent GPIO controller device node, or bank nodes, probably the improvement can be done in a few steps? - this change, - convert clients to reference a GPIO bank directly, - remove root GPIO controller (e.g. make it "simple-bus") and convert GPIO banks to "gpio-controller"s. Can an evolution like this happen? >> +- #size-cells: should be 1, total size of GPIO bank registers. >> + >> +The NXP LPC32xx SoC GPIO controller device node must contain a list >> +of device nodes representing GPIO banks and their descriptions. >> + >> +The format of subnodes should follow the description below. >> + >> +Required properties: >> +- reg: should contain 3 integer values: >> + 1) GPIO bank id from 0 to 5, >> + 2) physical base address of this GPIO bank, >> + 3) total size of the GPIO bank registers. >> + >> +Optional properties: >> +- gpio-bank-name: human readable name of a GPIO bank, >> +- gpio-no-output-state: property of P2 bank, which has special, >> + mapping of its control registers, >> +- gpio-offset: property of P3/GPIO bank, offset of bits representing >> + GPIO lines in output and direction registers, > > Seems like nr-gpios should have been a mask instead... > >> +- gpios: number of GPIO lines per GPIO bank, if this property is >> + omitted, then gpio-input-mask must be present, > > "gpios" is already the property name for the client interface. > >> +- gpio-input-mask: should contain two bitmasks, the first bitmask is >> + the mapping of GPIO lines to input status register, the second >> + bitmask should be a subset of the first bitmask and it represents >> + input GPIO lines, which may serve as an interrupt source, >> + if gpio-input-mask roperty is omitted, gpios property should be >> + present, >> +- interrupts: list of parent interrupts mapped to input GPIO lines, >> +- interrupts-extended: list of parent interrupts mapped to input GPIO >> + lines, used if parent interrupts are provided by more than one >> + interrupt controller, this option is used by GPI bank, >> +- interrupt-controller: indicates that GPIO bank may serve as an >> + interrupt controller, >> +- #interrupt-cells: if interrupt-controller property is present, >> + it should be 2, interrupt id and its flags. >> >> Example: >> >> @@ -24,6 +60,89 @@ Example: >> reg = <0x40028000 0x1000>; >> gpio-controller; >> #gpio-cells = <3>; /* bank, pin, flags */ > > Can't bank and pin be encoded into one cell as the gpio core binding > suggests. Please see the comment above, the proposed change does not modify this legacy part. >> + >> + ranges = <0 0x0 0x40028000 0x00001000>, >> + <1 0x0 0x40028000 0x00001000>, >> + <2 0x0 0x40028000 0x00001000>, >> + <3 0x0 0x40028000 0x00001000>, >> + <4 0x0 0x40028000 0x00001000>, >> + <5 0x0 0x40028000 0x00001000>; >> + #address-cells = <2>; >> + #size-cells = <1>; >> + >> + gpio_p0: gpio-controller@0 { >> + reg = <0 0x40 0x1C>; >> + gpio-bank-name = "p0"; >> + gpios = <8>; >> + >> + interrupt-parent = <&sic2>; >> + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; >> + }; >> + >> + gpio_p1: gpio-controller@1 { >> + reg = <1 0x60 0x1C>; >> + gpio-bank-name = "p1"; >> + gpios = <24>; >> + >> + interrupt-parent = <&sic2>; >> + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; >> + }; >> + >> + gpio_p2: gpio-controller@2 { >> + reg = <2 0x10 0x18>; >> + gpio-bank-name = "p2"; >> + gpios = <13>; >> + gpio-no-output-state; >> + }; >> + >> + gpio_gpio: gpio-controller@3 { >> + reg = <3 0x00 0x1C>; > > This overlaps with bank 2. Yes, it is. Thousand thanks to hardware designers. -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 20, 2015 at 08:27:35PM +0200, Vladimir Zapolskiy wrote: > On 20.11.2015 16:13, Rob Herring wrote: > > On Fri, Nov 20, 2015 at 03:29:52AM +0200, Vladimir Zapolskiy wrote: > >> For the purpose of better description of NXP LPC32xx GPIO controller > >> hardware in device tree format, extend the existing description with > >> device tree subnodes, which represent 6 GPIO banks within the > >> controller. > >> > >> Note, client interface to the GPIO controller is untouched. > >> > >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> > >> --- > >> .../devicetree/bindings/gpio/gpio_lpc32xx.txt | 121 ++++++++++++++++++++- > >> 1 file changed, 120 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt > >> index 4981936..d2da63c 100644 > >> --- a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt > >> +++ b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt > >> @@ -15,7 +15,43 @@ Required properties: > >> 2) pin number > >> 3) optional parameters: > >> - bit 0 specifies polarity (0 for normal, 1 for inverted) > >> -- reg: Index of the GPIO group > >> +- #address-cells: should be 2, which stands for GPIO bank id and > >> + physical base address of this GPIO bank. > > > > Now you need special code to do address translation. I'd really think > > twice about doing this. > > Correct, address translation code is needed here... > > > Why do you need the bank number? > > Only one reason -- backward compatibility in sense of referencing a GPIO > line on client's side. This API design is broken, I agree. What client exactly? Between the dtb and kernel or kernel and userspace or ...? > Honestly I would prefer to get rid of this "feature", new code allows to > reference on client's side either a parent GPIO controller device node, > or bank nodes, probably the improvement can be done in a few steps? > > - this change, > - convert clients to reference a GPIO bank directly, > - remove root GPIO controller (e.g. make it "simple-bus") and convert > GPIO banks to "gpio-controller"s. > > Can an evolution like this happen? You generally don't want bindings to evolve. > >> + gpio_p2: gpio-controller@2 { > >> + reg = <2 0x10 0x18>; > >> + gpio-bank-name = "p2"; > >> + gpios = <13>; > >> + gpio-no-output-state; > >> + }; > >> + > >> + gpio_gpio: gpio-controller@3 { > >> + reg = <3 0x00 0x1C>; > > > > This overlaps with bank 2. > > Yes, it is. Thousand thanks to hardware designers. Then you might want to split these into 2 regions. The problem is request_resource does not work with overlapping resources. Or just don't do subnodes. If there is not a lot of variations in the subnode data, then just leave that information in the kernel. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 20, 2015 at 2:29 AM, Vladimir Zapolskiy <vz@mleia.com> wrote: > For the purpose of better description of NXP LPC32xx GPIO controller > hardware in device tree format, extend the existing description with > device tree subnodes, which represent 6 GPIO banks within the > controller. > > Note, client interface to the GPIO controller is untouched. > > Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> (...) > + > +Required properties: > +- reg: should contain 3 integer values: > + 1) GPIO bank id from 0 to 5, > + 2) physical base address of this GPIO bank, > + 3) total size of the GPIO bank registers. If each bank has a unique physical address, it should be a unique device with a compatible string. Below: rethink this. All are named gpio-* which is the generic GPIO namespace and should be documented in Documentation/devicetree/bindings/gpio/gpio.txt so figure out if what you're adding is generic or not. > +Optional properties: > +- gpio-bank-name: human readable name of a GPIO bank, The node name is unique enough and is what we use in device trees. Get rid of this. > +- gpio-no-output-state: property of P2 bank, which has special, > + mapping of its control registers, Looks like it should be nxp,no-output-state > +- gpio-offset: property of P3/GPIO bank, offset of bits representing > + GPIO lines in output and direction registers, Explain more. I think this should probably be generic and described together with ngpios. > +- gpios: number of GPIO lines per GPIO bank, if this property is > + omitted, then gpio-input-mask must be present, Use the generic ngpios, also one does not exclude the other, e.g if there is 32 gpios, offset it 8 ngpios is 8, we know that bits 8..15 are GPIO lines. > +- gpio-input-mask: should contain two bitmasks, the first bitmask is > + the mapping of GPIO lines to input status register, the second > + bitmask should be a subset of the first bitmask and it represents > + input GPIO lines, which may serve as an interrupt source, > + if gpio-input-mask roperty is omitted, gpios property should be > + present, This is really hopeless to understand. This document needs a detailed description about how this GPIO block works so we have the background for this. > +- interrupts: list of parent interrupts mapped to input GPIO lines, > +- interrupts-extended: list of parent interrupts mapped to input GPIO > + lines, used if parent interrupts are provided by more than one > + interrupt controller, this option is used by GPI bank, > +- interrupt-controller: indicates that GPIO bank may serve as an > + interrupt controller, > +- #interrupt-cells: if interrupt-controller property is present, > + it should be 2, interrupt id and its flags. You need to add a description of how the block maps IRQs to GPIO lines. It seems like it is doing some kind of phone-exchange type of thing and just latches the GPIO line out to an IRQ line on the interrupt controller, and that is why even setting up trigger type has to percolate up to the parent? Explain this in this document. > + gpio-output-only; You forgot to documen this property. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On 30.11.2015 12:40, Linus Walleij wrote: > On Fri, Nov 20, 2015 at 2:29 AM, Vladimir Zapolskiy <vz@mleia.com> wrote: > >> For the purpose of better description of NXP LPC32xx GPIO controller >> hardware in device tree format, extend the existing description with >> device tree subnodes, which represent 6 GPIO banks within the >> controller. >> >> Note, client interface to the GPIO controller is untouched. >> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> > > (...) > >> + >> +Required properties: >> +- reg: should contain 3 integer values: >> + 1) GPIO bank id from 0 to 5, >> + 2) physical base address of this GPIO bank, >> + 3) total size of the GPIO bank registers. > > If each bank has a unique physical address, it should be a > unique device with a compatible string. thank you for review, this is the most broken GPIO controller I've ever seen, so many common generalized concepts present in the kernel are not applicable, unfortunately. Here is just a short description of "the best hw design practices" from the SoC documentation: * 6 GPIO banks (similar properties of GPIO lines within a bank): P0 (8 lines), P1 (24 lines), P2 (13 lines), GPIO (6 lines), GPO (24 lines, output only), GPI (22 lines, input only), - gpio-output-only property is for GPO, - gpio-input-only property is for GPI, * but 4 mixed register spaces: P0 (holds controls of P0), P1 (controls of P1), P2 (controls of P2 and GPIO), P3 (controls of GPIO, GPO and GPI), Here is the first answer to your questions, not each bank has a unique physical address, GPIO bank controls are in two register spaces and one register space contains controls of 3 banks. So there must be another way to address banks, I reused an existing one from the legacy code, banks are enumerated from 0 to 5. Also this "bank address" has been already used by the existing GPIO consumers, I'd be glad to have one device per bank (and that's actually in my plans for future), but this requires an update of all GPIO clients in DTS files, I deliberately separate this task from the GPIO controller driver update task. * only P2 bank has controls to set output value, but has no register to read out the set output value, - gpio-no-output-state property is for P2, * due to the fact that P2 register space has GPIO bank controls, one more property is introduced: - gpio-offset property for GPIO, * GPIO names in GPI bank are discrete (that caused a problem you've recently applied a fix for), all other banks has continuous GPIO names, * GPIO - 6 lines, each is capable to produce an interrupt, interrupt on GPIO line can be IRQ_TYPE_EDGE_BOTH if the driver reads a line state out, but IRQ chip itself does not support edge-both interrupts, this bank potentially can be converted to use GPIOLIB_IRQCHIP, but please see the next point, * GPI - 22 lines, but only 12 can produce an interrupt, so GPIOLIB_IRQCHIP helper can not be used here, to keep the code smaller (one less exception for the banks) I've shared the same mechanism of assigning GPIO lines to IRQs from GPI bank to GPIO bank. - gpio-input-mask property of GPIO and GPI banks, mapping between lines and interrupts > Below: rethink this. All are named gpio-* which is the generic > GPIO namespace and should be documented in > Documentation/devicetree/bindings/gpio/gpio.txt > so figure out if what you're adding is generic or not. You may see that adding of 5 bank specific properties mentioned above allow to generalize all 6 banks, any of the banks now can be converted to a separate GPIO controller device with the same compatible in one step, but this is postponed at the moment. >> +Optional properties: >> +- gpio-bank-name: human readable name of a GPIO bank, > > The node name is unique enough and is what we use in device > trees. Get rid of this. The node name in most cases and in the example below is "gpio-controller", at least in this particular case I find it insufficiently informative, P0 bank is sensibly different from P2, as well as it is different from GPI, GPIO or GPO. A good mechanism to understand in userspace what particular bank is involved is wanted here, probably I can add a "label" property? Or should I replace gpio-controller@xxx device node names with p0@xxx etc.? >> +- gpio-no-output-state: property of P2 bank, which has special, >> + mapping of its control registers, > > Looks like it should be nxp,no-output-state Agree. >> +- gpio-offset: property of P3/GPIO bank, offset of bits representing >> + GPIO lines in output and direction registers, > > Explain more. I think this should probably be generic and > described together with ngpios. The necessity comes from the fact that there is an intersection of register spaces for banks P2 and GPIO, when it concerns GPIO, DIR_CLR/DIR_STATE registers has a bit offset to control GPIO bank lines. >> +- gpios: number of GPIO lines per GPIO bank, if this property is >> + omitted, then gpio-input-mask must be present, > > Use the generic ngpios, also one does not exclude the other, e.g > if there is 32 gpios, offset it 8 ngpios is 8, we know that > bits 8..15 are GPIO lines. Ok, will use ngpios. Offset feature won't help, because there is no uniform offset (except 0) for any of the banks... >> +- gpio-input-mask: should contain two bitmasks, the first bitmask is >> + the mapping of GPIO lines to input status register, the second >> + bitmask should be a subset of the first bitmask and it represents >> + input GPIO lines, which may serve as an interrupt source, >> + if gpio-input-mask roperty is omitted, gpios property should be >> + present, > > This is really hopeless to understand. This document needs a > detailed description about how this GPIO block works so we > have the background for this. I'll add more info, shortly if you once find LPC32xx User's Manual (it is public), this property contains two values, the first one repeats mapping of P3_INP_STATE bits to bank specific lines, the second one (subset of the first) lists input lines, which can produce an interrupt. >> +- interrupts: list of parent interrupts mapped to input GPIO lines, >> +- interrupts-extended: list of parent interrupts mapped to input GPIO >> + lines, used if parent interrupts are provided by more than one >> + interrupt controller, this option is used by GPI bank, >> +- interrupt-controller: indicates that GPIO bank may serve as an >> + interrupt controller, >> +- #interrupt-cells: if interrupt-controller property is present, >> + it should be 2, interrupt id and its flags. > > You need to add a description of how the block maps IRQs > to GPIO lines. It seems like it is doing some kind of > phone-exchange type of thing and just latches the GPIO line > out to an IRQ line on the interrupt controller, and that is > why even setting up trigger type has to percolate up to > the parent? Explain this in this document. Will extend this description. Generally your understanding is correct, a requested GPIO interrupt is propagated to an IRQ chip interrupt, the handled IRQ chip interrupt is cascaded to the GPIO interrupt, but some specific handling of both edges type of interrupt is done on GPIO interrupt controller side, because IRQ chip interrupt can not support edge-both type of interrupts. >> + gpio-output-only; > > You forgot to documen this property. Ok. Thanks for review. If you find that conceptually any other scheme of device tree node properties or bank layout is more applicable in this particular case, please let me know. -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 30, 2015 at 1:13 PM, Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote: > [Me] >> The node name is unique enough and is what we use in device >> trees. Get rid of this. > > The node name in most cases and in the example below is "gpio-controller", > at least in this particular case I find it insufficiently informative, P0 > bank is sensibly different from P2, as well as it is different from GPI, > GPIO or GPO. A good mechanism to understand in userspace what particular > bank is involved is wanted here, probably I can add a "label" property? Or > should I replace gpio-controller@xxx device node names with p0@xxx etc.? Userspace is supposed to use the whole filepath in sysfs to identify a device. That should be enough, no matter what name the chip has. Also: see the ongoing discussion and proposed patch for a new userspace ABI based on a character device. We're not too happy about new userspace usecases for GPIO right now, we need to fix the ABI. >>> +- gpio-offset: property of P3/GPIO bank, offset of bits representing >>> + GPIO lines in output and direction registers, >> >> Explain more. I think this should probably be generic and >> described together with ngpios. > > The necessity comes from the fact that there is an intersection of register > spaces for banks P2 and GPIO, when it concerns GPIO, DIR_CLR/DIR_STATE > registers has a bit offset to control GPIO bank lines. So do we think that other hardware will have this property too or is it an nxp,* thing? >>> +- gpios: number of GPIO lines per GPIO bank, if this property is >>> + omitted, then gpio-input-mask must be present, >> >> Use the generic ngpios, also one does not exclude the other, e.g >> if there is 32 gpios, offset it 8 ngpios is 8, we know that >> bits 8..15 are GPIO lines. > > Ok, will use ngpios. Offset feature won't help, because there is no uniform > offset (except 0) for any of the banks... OK let's look closer at it. >>> +- gpio-input-mask: should contain two bitmasks, the first bitmask is >>> + the mapping of GPIO lines to input status register, the second >>> + bitmask should be a subset of the first bitmask and it represents >>> + input GPIO lines, which may serve as an interrupt source, >>> + if gpio-input-mask roperty is omitted, gpios property should be >>> + present, >> >> This is really hopeless to understand. This document needs a >> detailed description about how this GPIO block works so we >> have the background for this. > > I'll add more info, shortly if you once find LPC32xx User's Manual (it is > public), this property contains two values, the first one repeats mapping of > P3_INP_STATE bits to bank specific lines, the second one (subset of the > first) lists input lines, which can produce an interrupt. OK, and should it be an nxp,* property? >>> +- interrupts: list of parent interrupts mapped to input GPIO lines, >>> +- interrupts-extended: list of parent interrupts mapped to input GPIO >>> + lines, used if parent interrupts are provided by more than one >>> + interrupt controller, this option is used by GPI bank, >>> +- interrupt-controller: indicates that GPIO bank may serve as an >>> + interrupt controller, >>> +- #interrupt-cells: if interrupt-controller property is present, >>> + it should be 2, interrupt id and its flags. >> >> You need to add a description of how the block maps IRQs >> to GPIO lines. It seems like it is doing some kind of >> phone-exchange type of thing and just latches the GPIO line >> out to an IRQ line on the interrupt controller, and that is >> why even setting up trigger type has to percolate up to >> the parent? Explain this in this document. > > Will extend this description. Generally your understanding is correct, a > requested GPIO interrupt is propagated to an IRQ chip interrupt, the handled > IRQ chip interrupt is cascaded to the GPIO interrupt, but some specific > handling of both edges type of interrupt is done on GPIO interrupt > controller side, because IRQ chip interrupt can not support edge-both type > of interrupts. OMG that sounds so weird. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt index 4981936..d2da63c 100644 --- a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +++ b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt @@ -15,7 +15,43 @@ Required properties: 2) pin number 3) optional parameters: - bit 0 specifies polarity (0 for normal, 1 for inverted) -- reg: Index of the GPIO group +- #address-cells: should be 2, which stands for GPIO bank id and + physical base address of this GPIO bank. +- #size-cells: should be 1, total size of GPIO bank registers. + +The NXP LPC32xx SoC GPIO controller device node must contain a list +of device nodes representing GPIO banks and their descriptions. + +The format of subnodes should follow the description below. + +Required properties: +- reg: should contain 3 integer values: + 1) GPIO bank id from 0 to 5, + 2) physical base address of this GPIO bank, + 3) total size of the GPIO bank registers. + +Optional properties: +- gpio-bank-name: human readable name of a GPIO bank, +- gpio-no-output-state: property of P2 bank, which has special, + mapping of its control registers, +- gpio-offset: property of P3/GPIO bank, offset of bits representing + GPIO lines in output and direction registers, +- gpios: number of GPIO lines per GPIO bank, if this property is + omitted, then gpio-input-mask must be present, +- gpio-input-mask: should contain two bitmasks, the first bitmask is + the mapping of GPIO lines to input status register, the second + bitmask should be a subset of the first bitmask and it represents + input GPIO lines, which may serve as an interrupt source, + if gpio-input-mask roperty is omitted, gpios property should be + present, +- interrupts: list of parent interrupts mapped to input GPIO lines, +- interrupts-extended: list of parent interrupts mapped to input GPIO + lines, used if parent interrupts are provided by more than one + interrupt controller, this option is used by GPI bank, +- interrupt-controller: indicates that GPIO bank may serve as an + interrupt controller, +- #interrupt-cells: if interrupt-controller property is present, + it should be 2, interrupt id and its flags. Example: @@ -24,6 +60,89 @@ Example: reg = <0x40028000 0x1000>; gpio-controller; #gpio-cells = <3>; /* bank, pin, flags */ + + ranges = <0 0x0 0x40028000 0x00001000>, + <1 0x0 0x40028000 0x00001000>, + <2 0x0 0x40028000 0x00001000>, + <3 0x0 0x40028000 0x00001000>, + <4 0x0 0x40028000 0x00001000>, + <5 0x0 0x40028000 0x00001000>; + #address-cells = <2>; + #size-cells = <1>; + + gpio_p0: gpio-controller@0 { + reg = <0 0x40 0x1C>; + gpio-bank-name = "p0"; + gpios = <8>; + + interrupt-parent = <&sic2>; + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; + }; + + gpio_p1: gpio-controller@1 { + reg = <1 0x60 0x1C>; + gpio-bank-name = "p1"; + gpios = <24>; + + interrupt-parent = <&sic2>; + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; + }; + + gpio_p2: gpio-controller@2 { + reg = <2 0x10 0x18>; + gpio-bank-name = "p2"; + gpios = <13>; + gpio-no-output-state; + }; + + gpio_gpio: gpio-controller@3 { + reg = <3 0x00 0x1C>; + gpio-bank-name = "gpio"; + gpio-offset = <25>; + gpio-input-mask = <0x01007c00>, <0x01007c00>; + + interrupt-parent = <&sic2>; + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>, + <1 IRQ_TYPE_LEVEL_HIGH>, + <2 IRQ_TYPE_LEVEL_HIGH>, + <3 IRQ_TYPE_LEVEL_HIGH>, + <4 IRQ_TYPE_LEVEL_HIGH>, + <5 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + + gpio_gpi: gpio-controller@4 { + reg = <4 0x00 0x04>; + gpio-bank-name = "gpi"; + gpio-input-only; + gpio-input-mask = <0x1aff83ff>, <0x100803ff>; + + interrupts-extended = + <&sic2 22 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 23 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 24 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 25 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 26 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 27 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 28 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 15 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 9 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 10 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 11 IRQ_TYPE_LEVEL_HIGH>, + <&sic1 4 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + + gpio_gpo: gpio-controller@5 { + reg = <5 0x00 0x10>; + gpio-bank-name = "gpo"; + gpios = <24>; + gpio-output-only; + }; }; leds {
For the purpose of better description of NXP LPC32xx GPIO controller hardware in device tree format, extend the existing description with device tree subnodes, which represent 6 GPIO banks within the controller. Note, client interface to the GPIO controller is untouched. Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> --- .../devicetree/bindings/gpio/gpio_lpc32xx.txt | 121 ++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-)