On 03.09.2014 16: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.
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.
Yes, it is just one table with bunch of different subtables types. Since
GIC identification register is at different offset across GICv1/2 and
GICv3, the probe logic seems to be slightly different. IMO, we should
look into our MADT and try GICv3 fist and then GICv2, that means
presence of redistributor entries suggests GICv3/4. Its absence means we
need to look for GICC subtables and then call GICv2 init.
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.
We already had tried making MADT parser common for all GICs in the
separate file (outside GIC driver), it did not end up well. In the light
of above comment, the logic will be complex and some GIC local strutures
need to be exported:
1. Check redistributor entries.
2. If none exist, as fallback, check GICC entries, there are
redistributor base address assigned to CPU. Unfortunately it has no
register set size so ioremap only two pages (RD + SGI) check if we
support VLPI and extend ioremap if true. We cannot operate on GIC
register outside GIC driver so that requires another exported GICv3
function.
3. Jump to redistributor part if we are OK so far, if not, then we start
playing with GICC enries and look for cpu base addresses.
Honestly, apart from distributor parsing logic, there is no common code.
As you can see, current gic_v2_acpi_init() logic is quite easy to
follow. And gic_v3_acpi_init() inside of GICv3 driver is easy too. I
might not see other solutions but I am open to other suggestions, so
please correct me in that case.
Regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
