RE: [PATCH] gpio: dwapb: Add support for 32 interrupts

2018-04-11 Thread Phil Edworthy
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

2018-04-10 Thread Phil Edworthy
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

2018-04-10 Thread Geert Uytterhoeven
Hi Phil,

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

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

2018-04-10 Thread Phil Edworthy
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

2018-04-10 Thread Phil Edworthy
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

2018-04-06 Thread Phil Edworthy
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

2018-04-06 Thread Geert Uytterhoeven
Hi Phil,

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.

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

2018-04-05 Thread Phil Edworthy
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

2018-03-30 Thread Andy Shevchenko
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?

> 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

2018-03-29 Thread Phil Edworthy
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;
> +