Re: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support

[Marc Zyngier]

On 05/09/14 11:13, Arnd Bergmann wrote:
> On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
>>>
>>> I still prefer being explicit here for the same reason I mentioned earlier:
>>> I want it to be very clear that we don't support arbitrary irqchips other
>>> than the ones in the APCI specification. The infrastructure exists on DT
>>> because we have to support a large number of incompatible irqchips.
>>
>> I'm not suggesting that we should support more than the ACPI spec says.
>> And that's certainly the whole point of a spec, isn't it? ACPI says what
>> we support, and we're not going any further. I'm just saying that we
>> shouldn't make the maintenance burden heavier, and the code nothing
>> short of disgusting. Using our current infrastructure doesn't mean we're
>> going to support GIcv2.37.
> 
> Ok
> 
>>> In particular, the ACPI tables describing the irqchip have no way to
>>> identify the GIC at all, if I read the spec correctly, you have to
>>> parse the tables, ioremap the registers and then read the ID to know
>>> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
>>> for the GIC that a hypothetical probe function would be based on.
>>
>> This is not the way I read the spec. Table 5-46 (Interrupt Controller
>> Structure) tells you if you have a CPU interface (GICv1/v2) or a
>> redistributor (GICv3/v4). That's enough to know whether or not you
>> should carry on probing a particular controller.
> 
> Ah, good. I missed that.
> 
>> The various GIC versions don't really have a unified memory map anyway
>> (well, none that you can rely on), and you really have to rely on ACPI
>> to tell you what you have.
> 
> So we are back to needing to support two different irqchip drivers
> (v1/v2/v2m and v3/v4), instead of five or more, right?

As long as we make sure that the variants are directly probed from the
"canonical" driver (v2m from v2, ITS and v4 from v3), then we're OK.

>>> It does seem wrong to parse the tables in the irq-gic.c file though:
>>> that part can well be common across the various gic versions and then
>>> call into either irq-gic.c or irq-gic-v3.c for the version specific
>>> parts. Whether we put that common code into drivers/irqchip/irqchip.c,
>>> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
>>> drivers/acpi/irq-gic.c I don't care at all.
>>
>> I don't think so you can make that common code very easily. The
>> information required by both drivers is organized differently.
>> If it was, I'd have done that for the DT binding.
> 
> I see, and that's also what Tomasz just explained. So can we just
> have one an irqchip_init() function doing this:?
> 
> if (dt)
> 	of_irq_init(__irqchip_of_table);
> else if (acpi) {
> 	read cpu-interface and redistributor address from acpi tables
> 	if (cpu-interface)
> 		gic_v2_acpi_init(table);
> 	else if(redistributor)
> 		gic_v3_acpi_init(table)
> }

That'd be better, but that only considers the basic architecture. GICv2m
and GICv3's ITS bring additional complexity (and their own table
parsing). At that stage, I'm not sure how much commonality we're left with.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
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/