gpio: dwapb: Add support for 32 interrupts

Message ID 1522246950-9110-1-git-send-email-phil.edworthy@renesas.com
State New
Headers show
Series
  • gpio: dwapb: Add support for 32 interrupts
Related show

Commit Message

Phil Edworthy March 28, 2018, 2:22 p.m.
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(-)

Comments

Phil Edworthy March 29, 2018, 9:39 a.m. | #1
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
Andy Shevchenko March 30, 2018, 9:26 p.m. | #2
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.
Phil Edworthy April 5, 2018, 9:42 a.m. | #3
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
Geert Uytterhoeven April 6, 2018, 9:57 a.m. | #4
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
Phil Edworthy April 6, 2018, 10:20 a.m. | #5
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
Rob Herring April 9, 2018, 7:20 p.m. | #6
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
Phil Edworthy April 10, 2018, 6:24 a.m. | #7
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
Phil Edworthy April 10, 2018, 2:23 p.m. | #8
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
Geert Uytterhoeven April 10, 2018, 2:29 p.m. | #9
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
Phil Edworthy April 10, 2018, 3 p.m. | #10
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
Phil Edworthy April 11, 2018, 10:17 a.m. | #11
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

Patch

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;
 };