| Message ID | 1522246950-9110-1-git-send-email-phil.edworthy@renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: dwapb: Add support for 32 interrupts | expand |
Hi, On 28 March 2018 15:23, Phil Edworthy wrote: > The DesignWare GPIO IP can be configured for either 1 or 32 interrupts, > but the driver currently only supports 1 interrupt. See the DesignWare > DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter. > > This change allows the driver to work with up to 32 interrupts, it will > get as many interrupts as specified in the DT 'interrupts' property. > It doesn't do anything clever with the different interrupts, it just calls > the same handler used for single interrupt hardware. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > Note: There are a few lines over 80 chars, but this is just guidance, right? > Especially as there are already some lines over 80 chars. > --- > .../devicetree/bindings/gpio/snps-dwapb-gpio.txt | 10 ++++- > drivers/gpio/gpio-dwapb.c | 44 +++++++++++++++++----- > include/linux/platform_data/gpio-dwapb.h | 3 +- > 3 files changed, 45 insertions(+), 12 deletions(-) This patch triggers a build error for Quark MFD driver, which is the only user of the structure outside of the driver. I will fix that with an additional patch, but I'll wait to see what other comments I get first. Thanks Phil > diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > index 4a75da7..e343581 100644 > --- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > @@ -26,8 +26,14 @@ controller. > the second encodes the triger flags encoded as described in > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > - interrupt-parent : The parent interrupt controller. > -- interrupts : The interrupt to the parent controller raised when GPIOs > - generate the interrupts. > +- interrupts : The interrupts to the parent controller raised when GPIOs > + generate the interrupts. If the controller provides one combined interrupt > + for all GPIOs, specify a single interrupt. If the controller provides one > + interrupt for each GPIO, provide a list of interrupts that correspond to each > + of the GPIO pins. When specifying multiple interrupts, if any of the GPIOs > are > + not connected to an interrupt, use the interrupt-mask property. > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts in the list > + of interrupts is valid, bit is 1 for a valid irq. > - snps,nr-gpios : The number of pins in the port, a single cell. > - resets : Reset line for the controller. > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 226977f..47d82f9 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct > dwapb_gpio *gpio, > irq_gc->chip_types[1].handler = handle_edge_irq; > > if (!pp->irq_shared) { > - irq_set_chained_handler_and_data(pp->irq, > dwapb_irq_handler, > - gpio); > + int i; > + > + for (i = 0; i < pp->ngpio; i++) { > + if (pp->irq[i]) > + irq_set_chained_handler_and_data(pp- > >irq[i], > + dwapb_irq_handler, gpio); > + } > } else { > /* > * Request a shared IRQ since where MFD would have > devices > * using the same irq pin > */ > - err = devm_request_irq(gpio->dev, pp->irq, > + err = devm_request_irq(gpio->dev, pp->irq[0], > dwapb_irq_handler_mfd, > IRQF_SHARED, "gpio-dwapb-mfd", gpio); > if (err) { > @@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio > *gpio, > if (pp->idx == 0) > port->gc.set_config = dwapb_gpio_set_config; > > - if (pp->irq) > + if (pp->has_irq) > dwapb_configure_irqs(gpio, port, pp); > > err = gpiochip_add_data(&port->gc, port); > @@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio > *gpio, > port->is_registered = true; > > /* Add GPIO-signaled ACPI event support */ > - if (pp->irq) > + if (pp->has_irq) > acpi_gpiochip_request_interrupts(&port->gc); > > return err; > @@ -601,13 +606,34 @@ dwapb_gpio_get_pdata(struct device *dev) > if (dev->of_node && pp->idx == 0 && > fwnode_property_read_bool(fwnode, > "interrupt-controller")) { > - pp->irq = > irq_of_parse_and_map(to_of_node(fwnode), 0); > - if (!pp->irq) > + struct device_node *np = to_of_node(fwnode); > + u32 irq_mask = 0xFFFFFFFF; > + int j; > + > + /* Optional irq mask */ > + fwnode_property_read_u32(fwnode, "interrupt- > mask", &irq_mask); > + > + /* > + * The IP has configuration options to allow a single > + * combined interrupt or one per gpio. If one per > gpio, > + * some might not be used. > + */ > + for (j = 0; j < pp->ngpio; j++) { > + if (irq_mask & BIT(j)) { > + pp->irq[j] = > irq_of_parse_and_map(np, j); > + if (pp->irq[j]) > + pp->has_irq = true; > + } > + } > + if (!pp->has_irq) > dev_warn(dev, "no irq for port%d\n", pp- > >idx); > } > > - if (has_acpi_companion(dev) && pp->idx == 0) > - pp->irq = > platform_get_irq(to_platform_device(dev), 0); > + if (has_acpi_companion(dev) && pp->idx == 0) { > + pp->irq[0] = > platform_get_irq(to_platform_device(dev), 0); > + if (pp->irq[0]) > + pp->has_irq = true; > + } > > pp->irq_shared = false; > pp->gpio_base = -1; > diff --git a/include/linux/platform_data/gpio-dwapb.h > b/include/linux/platform_data/gpio-dwapb.h > index 2dc7f4a..5a52d69 100644 > --- a/include/linux/platform_data/gpio-dwapb.h > +++ b/include/linux/platform_data/gpio-dwapb.h > @@ -19,7 +19,8 @@ struct dwapb_port_property { > unsigned int idx; > unsigned int ngpio; > unsigned int gpio_base; > - unsigned int irq; > + unsigned int irq[32]; > + bool has_irq; > bool irq_shared; > }; > > -- > 2.7.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 Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy <phil.edworthy@renesas.com> wrote: > The DesignWare GPIO IP can be configured for either 1 or 32 interrupts, 1 to 32, or just a choice between two? > but the driver currently only supports 1 interrupt. See the DesignWare > DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter. Will see after holiday and perhaps make more comments. Here is just a brief review. > +- interrupts : The interrupts to the parent controller raised when GPIOs > + generate the interrupts. If the controller provides one combined interrupt > + for all GPIOs, specify a single interrupt. If the controller provides one > + interrupt for each GPIO, provide a list of interrupts that correspond to each > + of the GPIO pins. When specifying multiple interrupts, if any of the GPIOs are > + not connected to an interrupt, use the interrupt-mask property. > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts in the list > + of interrupts is valid, bit is 1 for a valid irq. So, but why one will need that in practice? GPIO driver usually provides a pin based IRQ chip which maps each pin to the corresponding offset inside specific IRQ domain. > + struct device_node *np = to_of_node(fwnode); > + u32 irq_mask = 0xFFFFFFFF; Why? Shouldn't it be dependent to the amount of actual pins / ports? Intel Quark has only 8 AFAIR. > + int j; > + > + /* Optional irq mask */ > + fwnode_property_read_u32(fwnode, "interrupt-mask", &irq_mask); > + > + /* > + * The IP has configuration options to allow a single > + * combined interrupt or one per gpio. If one per gpio, > + * some might not be used. > + */ > + for (j = 0; j < pp->ngpio; j++) { > + if (irq_mask & BIT(j)) { for_each_set_bit() is in kernel for ages! > + pp->irq[j] = irq_of_parse_and_map(np, j); > + if (pp->irq[j]) > + pp->has_irq = true; > + } > + } So, on the first glance the patch looks either superfluous or taking wrong approach. Please, elaborate more why it's done in this way and what the case for all this in practice.
Hi Andy, On 30 March 2018 22:26 Andy Shevchenko wrote: > On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy wrote: > > The DesignWare GPIO IP can be configured for either 1 or 32 > > interrupts, > > 1 to 32, or just a choice between two? Just a choice of 1 or 32. Note that by 'configured' I am talking about the hardware being configured in RTL prior to manufacturing a device. Once made, you cannot change it. This configuration affects the number of output interrupt signals from the GPIO Controller block that are connected to an interrupt controller. > > but the driver currently only supports 1 interrupt. See the DesignWare > > DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter. > > Will see after holiday and perhaps make more comments. Here is just a brief > review. > > > +- interrupts : The interrupts to the parent controller raised when > > +GPIOs > > + generate the interrupts. If the controller provides one combined > > +interrupt > > + for all GPIOs, specify a single interrupt. If the controller > > +provides one > > + interrupt for each GPIO, provide a list of interrupts that > > +correspond to each > > + of the GPIO pins. When specifying multiple interrupts, if any of > > +the GPIOs are > > + not connected to an interrupt, use the interrupt-mask property. > > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts > > +in the list > > + of interrupts is valid, bit is 1 for a valid irq. > > So, but why one will need that in practice? GPIO driver usually provides a pin > based IRQ chip which maps each pin to the corresponding offset inside > specific IRQ domain. On an ARM device we have this GPIO block connected to the GIC interrupt controller, i.e. the Synopsys GPIO controller interrupts can* have a 1 to 1 mapping to the GIC interrupts. At the moment, the GPIO driver only allows a single irq signal to specified. * this is not strictly accurate on the device I am working on, there is another block of IP between the two, but that doesn't matter in this case. > > + struct device_node *np = to_of_node(fwnode); > > + u32 irq_mask = 0xFFFFFFFF; > > Why? Shouldn't it be dependent to the amount of actual pins / ports? > Intel Quark has only 8 AFAIR. It's just a default which can be overridden via device tree. For Quark, since you currently only use a single irq, I guess the HW was configured that way. In which case, you wouldn't use any of this. > > + int j; > > + > > + /* Optional irq mask */ > > + fwnode_property_read_u32(fwnode, > > + "interrupt-mask", &irq_mask); > > + > > + /* > > + * The IP has configuration options to allow a single > > + * combined interrupt or one per gpio. If one per gpio, > > + * some might not be used. > > + */ > > > + for (j = 0; j < pp->ngpio; j++) { > > + if (irq_mask & BIT(j)) { > > for_each_set_bit() is in kernel for ages! There's lot of stuff in the kernel for ages that I can't remember! I'll fix this :) > > + pp->irq[j] = irq_of_parse_and_map(np, j); > > + if (pp->irq[j]) > > + pp->has_irq = true; > > + } > > + } > > > So, on the first glance the patch looks either superfluous or taking wrong > approach. Please, elaborate more why it's done in this way and what the > case for all this in practice. Hopefully I have explained it a bit better above. Thanks for your comments Phil
Hi Phil, On Thu, Apr 5, 2018 at 11:42 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote: > On 30 March 2018 22:26 Andy Shevchenko wrote: >> On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy wrote: >> > The DesignWare GPIO IP can be configured for either 1 or 32 >> > interrupts, >> >> 1 to 32, or just a choice between two? > Just a choice of 1 or 32. > Note that by 'configured' I am talking about the hardware being configured in > RTL prior to manufacturing a device. Once made, you cannot change it. > This configuration affects the number of output interrupt signals from the GPIO > Controller block that are connected to an interrupt controller. Differentiating between different versions of an IP block using DT properties is usually a bad idea, for several reasons: - What if you discover another difference later? - You cannot add differentiating properties retroactively, because of backwards compatibility with old DTBS. Hence I think you should introduce a new compatible value instead. Gr{oetje,eeting}s, Geert
Hi Geert, On 06 April 2018 10:57 Geert Uytterhoeven wrote: > On Thu, Apr 5, 2018 at 11:42 AM, Phil Edworthy wrote: > > On 30 March 2018 22:26 Andy Shevchenko wrote: > >> On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy wrote: > >> > The DesignWare GPIO IP can be configured for either 1 or 32 > >> > interrupts, > >> > >> 1 to 32, or just a choice between two? > > Just a choice of 1 or 32. > > Note that by 'configured' I am talking about the hardware being > > configured in RTL prior to manufacturing a device. Once made, you cannot > change it. > > This configuration affects the number of output interrupt signals from > > the GPIO Controller block that are connected to an interrupt controller. > > Differentiating between different versions of an IP block using DT properties > is usually a bad idea, for several reasons: > - What if you discover another difference later? > - You cannot add differentiating properties retroactively, because of > backwards > compatibility with old DTBS. > > Hence I think you should introduce a new compatible value instead. This is not a different version of the IP, just a different configuration option. Most IP blocks have a huge number of knobs that can be twiddled by the HW people, such as cache size, UART fifo depth. I think this is no different. Thanks Phil
On Wed, Mar 28, 2018 at 03:22:30PM +0100, Phil Edworthy wrote: > The DesignWare GPIO IP can be configured for either 1 or 32 interrupts, > but the driver currently only supports 1 interrupt. See the DesignWare > DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter. Someday h/w designers will realize this does nothing to optimize interrupt handling... > This change allows the driver to work with up to 32 interrupts, it will > get as many interrupts as specified in the DT 'interrupts' property. > It doesn't do anything clever with the different interrupts, it just calls > the same handler used for single interrupt hardware. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > Note: There are a few lines over 80 chars, but this is just guidance, right? > Especially as there are already some lines over 80 chars. Code, yes, but not for paragraphs of text in DT bindings. > --- > .../devicetree/bindings/gpio/snps-dwapb-gpio.txt | 10 ++++- > drivers/gpio/gpio-dwapb.c | 44 +++++++++++++++++----- > include/linux/platform_data/gpio-dwapb.h | 3 +- > 3 files changed, 45 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > index 4a75da7..e343581 100644 > --- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > @@ -26,8 +26,14 @@ controller. > the second encodes the triger flags encoded as described in > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > - interrupt-parent : The parent interrupt controller. > -- interrupts : The interrupt to the parent controller raised when GPIOs > - generate the interrupts. > +- interrupts : The interrupts to the parent controller raised when GPIOs > + generate the interrupts. If the controller provides one combined interrupt > + for all GPIOs, specify a single interrupt. If the controller provides one > + interrupt for each GPIO, provide a list of interrupts that correspond to each > + of the GPIO pins. When specifying multiple interrupts, if any of the GPIOs are > + not connected to an interrupt, use the interrupt-mask property. > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts in the list > + of interrupts is valid, bit is 1 for a valid irq. This is not a standard property and would need a vendor prefix. However, I'd prefer you just skip any not connected interrupts with an invalid interrupt number. Then the GPIO number is the index into "interrupts". > - snps,nr-gpios : The number of pins in the port, a single cell. This BTW should be deprecated to use "nr-gpios" instead, but that's another patch. > - resets : Reset line for the controller. > -- 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 Rob, On 09 April 2018 20:20 Rob Herring wrote: > On Wed, Mar 28, 2018 at 03:22:30PM +0100, Phil Edworthy wrote: > > The DesignWare GPIO IP can be configured for either 1 or 32 > > interrupts, but the driver currently only supports 1 interrupt. See > > the DesignWare DW_apb_gpio Databook description of the > 'GPIO_INTR_IO' parameter. > > Someday h/w designers will realize this does nothing to optimize interrupt > handling... I can imagine some software where the isr is written to handle a specific GPIO interrupt _could_ be faster, though no sane software would be designed like that. > > This change allows the driver to work with up to 32 interrupts, it > > will get as many interrupts as specified in the DT 'interrupts' property. > > It doesn't do anything clever with the different interrupts, it just > > calls the same handler used for single interrupt hardware. > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > --- > > Note: There are a few lines over 80 chars, but this is just guidance, right? > > Especially as there are already some lines over 80 chars. > > Code, yes, but not for paragraphs of text in DT bindings. Good, that's what I did. > > --- > > .../devicetree/bindings/gpio/snps-dwapb-gpio.txt | 10 ++++- > > drivers/gpio/gpio-dwapb.c | 44 +++++++++++++++++----- > > include/linux/platform_data/gpio-dwapb.h | 3 +- > > 3 files changed, 45 insertions(+), 12 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > > b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > > index 4a75da7..e343581 100644 > > --- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > > +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > > @@ -26,8 +26,14 @@ controller. > > the second encodes the triger flags encoded as described in > > > > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > > - interrupt-parent : The parent interrupt controller. > > -- interrupts : The interrupt to the parent controller raised when > > GPIOs > > - generate the interrupts. > > +- interrupts : The interrupts to the parent controller raised when > > +GPIOs > > + generate the interrupts. If the controller provides one combined > > +interrupt > > + for all GPIOs, specify a single interrupt. If the controller > > +provides one > > + interrupt for each GPIO, provide a list of interrupts that > > +correspond to each > > + of the GPIO pins. When specifying multiple interrupts, if any of > > +the GPIOs are > > + not connected to an interrupt, use the interrupt-mask property. > > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts > > +in the list > > + of interrupts is valid, bit is 1 for a valid irq. > > This is not a standard property and would need a vendor prefix. However, I'd > prefer you just skip any not connected interrupts with an invalid interrupt > number. Then the GPIO number is the index into "interrupts". Makes sense, I'll rework it to do this. > > - snps,nr-gpios : The number of pins in the port, a single cell. > > This BTW should be deprecated to use "nr-gpios" instead, but that's another > patch. Thanks for your comments, Phil -- 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 Rob, On 10 April 2018 07:24 Phil Edworthy wrote: > On 09 April 2018 20:20 Rob Herring wrote: > > On Wed, Mar 28, 2018 at 03:22:30PM +0100, Phil Edworthy wrote: [...] > > > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts > > > +in the list > > > + of interrupts is valid, bit is 1 for a valid irq. > > > > This is not a standard property and would need a vendor prefix. However, > I'd > > prefer you just skip any not connected interrupts with an invalid interrupt > > number. Then the GPIO number is the index into "interrupts". > Makes sense, I'll rework it to do this. Err, what would an invalid interrupt number be? If I use -1, I get a DT parsing error and 0 is certainly valid. If the number is larger than the valid interrupt range I get errors during probe. Thanks Phil -- 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 Phil, On Tue, Apr 10, 2018 at 4:23 PM, Phil Edworthy <phil.edworthy@renesas.com> wrote: > On 10 April 2018 07:24 Phil Edworthy wrote: >> On 09 April 2018 20:20 Rob Herring wrote: >> > On Wed, Mar 28, 2018 at 03:22:30PM +0100, Phil Edworthy wrote: > [...] >> > > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts >> > > +in the list >> > > + of interrupts is valid, bit is 1 for a valid irq. >> > >> > This is not a standard property and would need a vendor prefix. However, >> I'd >> > prefer you just skip any not connected interrupts with an invalid interrupt >> > number. Then the GPIO number is the index into "interrupts". >> Makes sense, I'll rework it to do this. > Err, what would an invalid interrupt number be? > If I use -1, I get a DT parsing error and 0 is certainly valid. If the number is > larger than the valid interrupt range I get errors during probe. Perhaps using interrupts-extended instead of interrupts? E.g. interrupts-extended = <&intc1 5 1>, <0>, <&intc2 1 0>; Gr{oetje,eeting}s, Geert
SGkgR2VlcnQsDQoNCk9uIDEwIEFwcmlsIDIwMTggMTU6MjkgR2VlcnQgVXl0dGVyaG9ldmVuIHdy b3RlOg0KPiBPbiBUdWUsIEFwciAxMCwgMjAxOCBhdCA0OjIzIFBNLCBQaGlsIEVkd29ydGh5IHdy b3RlOg0KPiA+IE9uIDEwIEFwcmlsIDIwMTggMDc6MjQgUGhpbCBFZHdvcnRoeSB3cm90ZToNCj4g Pj4gT24gMDkgQXByaWwgMjAxOCAyMDoyMCBSb2IgSGVycmluZyB3cm90ZToNCj4gPj4gPiBPbiBX ZWQsIE1hciAyOCwgMjAxOCBhdCAwMzoyMjozMFBNICswMTAwLCBQaGlsIEVkd29ydGh5IHdyb3Rl Og0KPiA+IFsuLi5dDQo+ID4+ID4gPiArLSBpbnRlcnJ1cHQtbWFzayA6IGEgMzItYml0IGJpdCBt YXNrIHRoYXQgc3BlY2lmaWVzIHdoaWNoDQo+ID4+ID4gPiAraW50ZXJydXB0cyBpbiB0aGUgbGlz dA0KPiA+PiA+ID4gKyAgb2YgaW50ZXJydXB0cyBpcyB2YWxpZCwgYml0IGlzIDEgZm9yIGEgdmFs aWQgaXJxLg0KPiA+PiA+DQo+ID4+ID4gVGhpcyBpcyBub3QgYSBzdGFuZGFyZCBwcm9wZXJ0eSBh bmQgd291bGQgbmVlZCBhIHZlbmRvciBwcmVmaXguDQo+ID4+ID4gSG93ZXZlciwNCj4gPj4gSSdk DQo+ID4+ID4gcHJlZmVyIHlvdSBqdXN0IHNraXAgYW55IG5vdCBjb25uZWN0ZWQgaW50ZXJydXB0 cyB3aXRoIGFuIGludmFsaWQNCj4gPj4gPiBpbnRlcnJ1cHQgbnVtYmVyLiBUaGVuIHRoZSBHUElP IG51bWJlciBpcyB0aGUgaW5kZXggaW50byAiaW50ZXJydXB0cyIuDQo+ID4+IE1ha2VzIHNlbnNl LCBJJ2xsIHJld29yayBpdCB0byBkbyB0aGlzLg0KPiA+IEVyciwgd2hhdCB3b3VsZCBhbiBpbnZh bGlkIGludGVycnVwdCBudW1iZXIgYmU/DQo+ID4gSWYgSSB1c2UgLTEsIEkgZ2V0IGEgRFQgcGFy c2luZyBlcnJvciBhbmQgMCBpcyBjZXJ0YWlubHkgdmFsaWQuIElmIHRoZQ0KPiA+IG51bWJlciBp cyBsYXJnZXIgdGhhbiB0aGUgdmFsaWQgaW50ZXJydXB0IHJhbmdlIEkgZ2V0IGVycm9ycyBkdXJp bmcgcHJvYmUuDQo+IA0KPiBQZXJoYXBzIHVzaW5nIGludGVycnVwdHMtZXh0ZW5kZWQgaW5zdGVh ZCBvZiBpbnRlcnJ1cHRzPw0KPiANCj4gRS5nLg0KPiANCj4gICAgIGludGVycnVwdHMtZXh0ZW5k ZWQgPSA8JmludGMxIDUgMT4sIDwwPiwgPCZpbnRjMiAxIDA+Ow0KDQpUaGFua3MgZm9yIHRoZSBw b2ludGVyLCBJJ2xsIGhhdmUgYSBsb29rLg0KUGhpbA0K -- 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 Andy, On 05 April 2018 10:43, Phil Edworthy wrote: > On 30 March 2018 22:26 Andy Shevchenko wrote: > > On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy wrote: > > > The DesignWare GPIO IP can be configured for either 1 or 32 > > > interrupts, > > > > 1 to 32, or just a choice between two? > Just a choice of 1 or 32. Sorry, I was wrong about this... the manual does not say 1 or 32. It says: "Port A can be configured to generate multiple interrupt signals or a single combined interrupt flag. When set to 1, the component generates a single combined interrupt flag." There is no other text describing this option, but I believe all GPIOs on port A will have an interrupt. In our case we have 32 GPIOs on port A and 32 interrupts connected to them. Thanks Phil
diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt index 4a75da7..e343581 100644 --- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt @@ -26,8 +26,14 @@ controller. the second encodes the triger flags encoded as described in Documentation/devicetree/bindings/interrupt-controller/interrupts.txt - interrupt-parent : The parent interrupt controller. -- interrupts : The interrupt to the parent controller raised when GPIOs - generate the interrupts. +- interrupts : The interrupts to the parent controller raised when GPIOs + generate the interrupts. If the controller provides one combined interrupt + for all GPIOs, specify a single interrupt. If the controller provides one + interrupt for each GPIO, provide a list of interrupts that correspond to each + of the GPIO pins. When specifying multiple interrupts, if any of the GPIOs are + not connected to an interrupt, use the interrupt-mask property. +- interrupt-mask : a 32-bit bit mask that specifies which interrupts in the list + of interrupts is valid, bit is 1 for a valid irq. - snps,nr-gpios : The number of pins in the port, a single cell. - resets : Reset line for the controller. diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 226977f..47d82f9 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, irq_gc->chip_types[1].handler = handle_edge_irq; if (!pp->irq_shared) { - irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler, - gpio); + int i; + + for (i = 0; i < pp->ngpio; i++) { + if (pp->irq[i]) + irq_set_chained_handler_and_data(pp->irq[i], + dwapb_irq_handler, gpio); + } } else { /* * Request a shared IRQ since where MFD would have devices * using the same irq pin */ - err = devm_request_irq(gpio->dev, pp->irq, + err = devm_request_irq(gpio->dev, pp->irq[0], dwapb_irq_handler_mfd, IRQF_SHARED, "gpio-dwapb-mfd", gpio); if (err) { @@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, if (pp->idx == 0) port->gc.set_config = dwapb_gpio_set_config; - if (pp->irq) + if (pp->has_irq) dwapb_configure_irqs(gpio, port, pp); err = gpiochip_add_data(&port->gc, port); @@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, port->is_registered = true; /* Add GPIO-signaled ACPI event support */ - if (pp->irq) + if (pp->has_irq) acpi_gpiochip_request_interrupts(&port->gc); return err; @@ -601,13 +606,34 @@ dwapb_gpio_get_pdata(struct device *dev) if (dev->of_node && pp->idx == 0 && fwnode_property_read_bool(fwnode, "interrupt-controller")) { - pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); - if (!pp->irq) + struct device_node *np = to_of_node(fwnode); + u32 irq_mask = 0xFFFFFFFF; + int j; + + /* Optional irq mask */ + fwnode_property_read_u32(fwnode, "interrupt-mask", &irq_mask); + + /* + * The IP has configuration options to allow a single + * combined interrupt or one per gpio. If one per gpio, + * some might not be used. + */ + for (j = 0; j < pp->ngpio; j++) { + if (irq_mask & BIT(j)) { + pp->irq[j] = irq_of_parse_and_map(np, j); + if (pp->irq[j]) + pp->has_irq = true; + } + } + if (!pp->has_irq) dev_warn(dev, "no irq for port%d\n", pp->idx); } - if (has_acpi_companion(dev) && pp->idx == 0) - pp->irq = platform_get_irq(to_platform_device(dev), 0); + if (has_acpi_companion(dev) && pp->idx == 0) { + pp->irq[0] = platform_get_irq(to_platform_device(dev), 0); + if (pp->irq[0]) + pp->has_irq = true; + } pp->irq_shared = false; pp->gpio_base = -1; diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h index 2dc7f4a..5a52d69 100644 --- a/include/linux/platform_data/gpio-dwapb.h +++ b/include/linux/platform_data/gpio-dwapb.h @@ -19,7 +19,8 @@ struct dwapb_port_property { unsigned int idx; unsigned int ngpio; unsigned int gpio_base; - unsigned int irq; + unsigned int irq[32]; + bool has_irq; bool irq_shared; };
The DesignWare GPIO IP can be configured for either 1 or 32 interrupts, but the driver currently only supports 1 interrupt. See the DesignWare DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter. This change allows the driver to work with up to 32 interrupts, it will get as many interrupts as specified in the DT 'interrupts' property. It doesn't do anything clever with the different interrupts, it just calls the same handler used for single interrupt hardware. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- Note: There are a few lines over 80 chars, but this is just guidance, right? Especially as there are already some lines over 80 chars. --- .../devicetree/bindings/gpio/snps-dwapb-gpio.txt | 10 ++++- drivers/gpio/gpio-dwapb.c | 44 +++++++++++++++++----- include/linux/platform_data/gpio-dwapb.h | 3 +- 3 files changed, 45 insertions(+), 12 deletions(-)