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

[Marc Zyngier]

On 04/11/14 14:20, Suravee Suthikulpanit wrote:
> 
> 
> On 11/4/14 04:06, Thomas Gleixner wrote:
>> On Mon, 3 Nov 2014, Suravee Suthikulanit wrote:
>>> On 11/3/2014 4:51 PM, Thomas Gleixner wrote:
>>>> On Mon, 3 Nov 2014, suravee.suthikulpanit at amd.com wrote:
>>>>> +	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.
>>
>>> 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.
> 
>> [...]
>> 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...