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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
