Re: [PATCH 1/2 V4] irqchip: gic: Add supports for ARM GICv2m MSI(-X)

[Suravee Suthikulpanit]

On 08/15/2014 09:03 AM, Marc Zyngier wrote:
> > +
> > +static struct irq_chip gicv2m_chip;
> > +
> > +#ifdef CONFIG_OF
>
> Is there any reason why this should be guarded by CONFIG_OF? Surely the
> v2m capability should only be enabled if OF is.

[Suravee]
We are also planning to support ACPI in the future also, which will be 
using a different init function.  Also, there is the same #ifdef in the 
irq-gic.c for the gic_of_init().  So, I am just trying to be consistent 
here.


> > +
> > +       memcpy(&gicv2m_chip, gic->irq_chip, sizeof(struct irq_chip));
> > +       gicv2m_chip.name = "GICv2m",
> > +       gicv2m_chip.irq_mask = gicv2m_mask_irq;
> > +       gicv2m_chip.irq_unmask = gicv2m_unmask_irq;
> > +       gic->irq_chip = &gicv2m_chip;
>
> I liked it until this last line. You're overriding the irq_chip for the
> whole GIC. I was expecting you'd only use it for the MSI range
> (basically return a range to the caller, together with your brand new
> irq_chip).

[Suravee]
I'm not sure if I understand you point here. Actually, I don't see the 
whole point of the need to have a whole different irq_chip for v2m 
stuff.  All I need is just a way to overwrite the irq_chip.irq_mask() 
and irq_chip.irq_unmask() with the v2m version which should check for 
MSI before calling mask/unmask_msi_irq(). I should be able to just do:

	gic->irq_chip.irq_mask = gicv2m_mask_irq;
	gic->irq_chip.irq_unmask = gicv2m_unmask_irq;


> > @@ -768,19 +768,21 @@ void __init gic_init_physaddr(struct device_node *node)
> >   static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
> >                                  irq_hw_number_t hw)
> >   {
> > +       struct gic_chip_data *gic = d->host_data;
> > +
> >          if (hw < 32) {
> >                  irq_set_percpu_devid(irq);
> > -               irq_set_chip_and_handler(irq, &gic_chip,
> > +               irq_set_chip_and_handler(irq, gic->irq_chip,
> >                                           handle_percpu_devid_irq);
> >                  set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
> >          } else {
> > -               irq_set_chip_and_handler(irq, &gic_chip,
> > +               irq_set_chip_and_handler(irq, gic->irq_chip,
> >                                           handle_fasteoi_irq);
>
> And here you should discriminate on whether this is MSI or not, based on
> the range you got from above.

[Suravee]
From above, since we only use one irq_chip (i.e. the gic_chip), there 
is no need to differentiate here, and I don't need to make these two 
line changes.

> > @@ -1009,6 +1012,16 @@ gic_of_init(struct device_node *node, struct device_node *parent)
> >          if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
> >                  percpu_offset = 0;
> >
> > +       gic_data[gic_cnt].irq_chip = &gic_chip;
> > +
> > +       /* Currently, we only support one v2m subnode. */
> > +       child = of_get_child_by_name(node, "v2m");
>
> If you only support one v2m node, then you should also enforce it for
> potential secondaty GICs (just probing it for gic_cnt == 0 should be
> enough).

[Suravee]
Actually, if we have multiple (N) GICs, we should be able to also 
support multiple (N) V2Ms with the followings entries.
	gic0 {
		.....
		v2m {
			....
		}
	}

	gic1 {
		.....
		v2m {
			....
		}
	}

What I am not trying to support at this point is the following:

	gic0 {
		....
		v2m {
			....
		}
		v2m {
			....
		}
	}


> > diff --git a/drivers/irqchip/irq-gic.h b/drivers/irqchip/irq-gic.h
> > new file mode 100644
> > index 0000000..2ec6bc3
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-gic.h
> > @@ -0,0 +1,48 @@
> > +#ifndef _IRQ_GIC_H_
> > +#define _IRQ_GIC_H_
> > +
> > +#include <linux/msi.h>
> > +
> > +union gic_base {
> > +       void __iomem *common_base;
> > +       void __percpu * __iomem *percpu_base;
> > +};
> > +
> > +#ifdef CONFIG_ARM_GIC_V2M
> > +struct v2m_data {
> > +       spinlock_t msi_cnt_lock;
> > +       struct msi_chip msi_chip;
> > +       struct resource res;      /* GICv2m resource */
> > +       void __iomem *base;       /* GICv2m virt address */
> > +       unsigned int spi_start;   /* The SPI number that MSIs start */
> > +       unsigned int nr_spis;     /* The number of SPIs for MSIs */
> > +       unsigned long *bm;        /* MSI vector bitmap */
> > +};
> > +#endif
>
> So if you put the #ifdef/#endif *inside* the v2m_data structure...

[Suravee] Are you suggesting an empty struct v2m_data{}; Hm.. I guess I 
can do that.

Thanks,

Suravee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/