On 03/09/14 15:57, Arnd Bergmann wrote:
On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:
On 02.09.2014 15:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) void __init init_IRQ(void) { irqchip_init();
- if (!handle_arch_irq)
acpi_gic_init();
Why isn't this called from irqchip_init? It would seem like the logical spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing we support so far. I don't think duplicating the kernel infrastructure "because we're different" is the right way.
There is no reason for your probing structure to be artificially different (you're parsing the same information, at the same time). Just put in place a similar probing mechanism, and this will look a lot better.
Having only GICv2, it would work. Considering we would do the same for GICv3 (arm-gic-v3.h) there will be register name conflicts for both headers inclusion:
[...] #include <linux/irqchip/arm-gic.h> #include <linux/irqchip/arm-gic-v3.h> [...] err = gic_v3_acpi_init(table); if (err) err = gic_v2_acpi_init(table); if (err) pr_err("Failed to initialize GIC IRQ controller"); [...] So instead of changing register names prefix, I choose new header will be less painfully.
Yes, and this is exactly why I pushed back on that last time. I'll continue saying that interrupt controllers should be self-probing, with ACPI as they are with DT.
Even with the restrictions of ACPI and SBSA, we end-up with at least 2 main families of interrupt controllers (GICv2 and GICv3), both with a number of "interesting" variations (GICv2m and GICv4, to only mention those I'm directly involved with).
I can safely predict that the above will become a tangled mess within 18 months, and the idea of littering the arch code with a bunch of hardcoded "if (blah())" doesn't fill me with joy and confidence.
In summary: we have the infrastructure already, just use it.
We had that discussion but I see we still don't have consensus here. It would be good to know our direction before we prepare next patch version. Arnd any comments on this from you side?
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.
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.
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.
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.
Thanks,
M.