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
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
