RE: [PATCH] gpio: dwapb: Add support for 32 interrupts
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
RE: [PATCH] gpio: dwapb: Add support for 32 interrupts
Hi Geert, On 10 April 2018 15:29 Geert Uytterhoeven wrote: > On Tue, Apr 10, 2018 at 4:23 PM, Phil Edworthy 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 = < 5 1>, <0>, < 1 0>; Thanks for the pointer, I'll have a look. Phil
Re: [PATCH] gpio: dwapb: Add support for 32 interrupts
Hi Phil, On Tue, Apr 10, 2018 at 4:23 PM, Phil Edworthywrote: > 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 = < 5 1>, <0>, < 1 0>; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
RE: [PATCH] gpio: dwapb: Add support for 32 interrupts
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
RE: [PATCH] gpio: dwapb: Add support for 32 interrupts
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> > --- > > 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
RE: [PATCH] gpio: dwapb: Add support for 32 interrupts
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
Re: [PATCH] gpio: dwapb: Add support for 32 interrupts
Hi Phil, On Thu, Apr 5, 2018 at 11:42 AM, Phil Edworthywrote: > 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 -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
RE: [PATCH] gpio: dwapb: Add support for 32 interrupts
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 = 0x; > > 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", _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
Re: [PATCH] gpio: dwapb: Add support for 32 interrupts
On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthywrote: > 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 = 0x; 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", > _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. -- With Best Regards, Andy Shevchenko
RE: [PATCH] gpio: dwapb: Add support for 32 interrupts
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> --- > 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(>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(>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 = 0x; > +