[V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)

On 04/11/14 14:20, Suravee Suthikulpanit wrote:

On 11/4/14 04:06, Thomas Gleixner wrote:

quoted

On Mon, 3 Nov 2014, Suravee Suthikulanit wrote:

quoted

On 11/3/2014 4:51 PM, Thomas Gleixner wrote:

quoted

On Mon, 3 Nov 2014, suravee.suthikulpanit at amd.com wrote:

quoted

+	irq_domain_set_hwirq_and_chip(v2m->domain, virq, hwirq,
+				&v2m_chip, v2m);
+
+	irq_set_msi_desc(hwirq, desc);
+	irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING);

Sure both calls work perfectly fine as long as virq == hwirq, right?

I was running into an issue when calling the irq_domain_alloc_irq_parent(), it
requires of_phandle_args pointer to be passed in. However, this does not work
for GICv2m since it does not have interrupt information in the device tree.
So, I decided at first to use direct (virq == hwirq) mapping, which simplifies
the code a bit, but might not be ideal solution, as you pointed out.

It's not only far from ideal. It's not a solution at all. Simply
because there is no guarantee for virq == hwirq.

quoted

An alternative would be to create a temporary struct of_phandle_args, and
populate it with the interrupt information for the requested MSI. Then pass it
to:
   --> irq_domain_alloc_irq_parent
    |--> gic_irq_domain_alloc
      |--> gic_irq_domain_xlate
      |--> gic_irq_domain_map

However, this would still not be ideal if we want to support ACPI. Another

Neither device tree nor ACPI has anything to do with MSI interrupts at
runtime.

All they do is to tell that there is a MSI controller and where the
registers are and in the worst case fixups for a borked MSI_TYPER
register.

So either the TYPER reg or DT/ACPI gives you a fixed hwirq range which
is reserved for MSI. And that's all you need, right?

Right, I get that part. Figuring out the fixed hwirq range for MSI is 
not the point I am trying to make here.

quoted

[...]
All you need is to pick one hwirq out of the existing fixed range and
associate it to a newly allocated virq. That's the only information
the underlying gic domain has to know about, because it needs to
translate from the hwirq to the virq in the low level entry handler
gic_handle_irq().

And that's what I am trying to do here except that GIC is expecting that 
information to be passed to it via irq_domain_alloc_irqs(..., args) 
where args is struct of_phandle_args (e.g. in the kernel/irqdomain.c: 
irq_create_of_mapping). This works fine when specifying interrupt from 
DT, but that is not always the case.

Currently, I can just create a fake of_phandle_args just to pass the 
hwirq information to GIC.

     --> gicv2m_setup_msi_irq()
      |    struct of_phandle_args phan;
      |    phan.np = NULL;
      |    phan.args_count = 3;
      |    phan.args[0] = 0;
      |    phan.args[1] = hwirq - 32;
      |    phan.args[2] = IRQ_TYPE_EDGE_RISING;
      |--> irq_domain_alloc_irqs(d, 1, NUMA_NO_NODE, &phan);
       |--> gicv2m_domain_alloc(d, virq, nr_irqs, arg)
	|--> irq_domain_alloc_irqs_parent(d, virq, nr_irqs, arg);

I am trying to figure out what would be a common data structure for this 
purpose that would work for both Dt and non-DT case (e.g. GICv2m MSI). 
Unless you think this is ok.

I think of_phandle_args is the only thing we can use, whether this is DT
or this other non-DT thing. This is how we represent a GIC HW interrupt
outside of the GIC itself.

Of course, this creates a dependency between the different domains (a
child domain *must* know how the parent domain represents its
interrupt). Things like the v2m widget are completely fused with the GIC
anyway, so it makes sense.

Things like generic secondary irqchips don't fit that model, but they
can continue to use the existing framework.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...