Re: [PATCH V3] irqchip: Add TB10x interrupt controller driver

From: Vineet Gupta
Date: Mon Jun 03 2013 - 00:05:45 EST


On 06/01/2013 03:48 AM, Grant Likely wrote:
> On Fri, 31 May 2013 19:32:34 +0200 (CEST), Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>> irq chip.
>>> +static void tb10x_irq_cascade(unsigned int irq, struct irq_desc *desc)
>>> +{
>>> + struct irq_domain *domain = irq_desc_get_handler_data(desc);
>>> +
>>> + generic_handle_irq(irq_find_mapping(domain, irq));
>>> +}
>> ...
>>
>>> + for (i = 0; i < nrirqs; i++) {
>>> + unsigned int irq = irq_of_parse_and_map(ictl, i);
>>> +
>>> + irq_set_handler_data(irq, domain);
>>> + irq_set_chained_handler(irq, tb10x_irq_cascade);
>>> + }
>> I might be completely confused, but this does not make any sense at
>> all.
>>
>> You allocate a linear domain and then map the interrupts in the
>> domain. The mapping function retrieves the hardware interrupt number
>> and creates a virtual interrupt number, installs the chip and the
>> handler for the interrupt and finally returns the virtual interrupt
>> number.
>>
>> Now you take that virtual interrupt number and install
>> tb10x_irq_cascade as the handler. irq_set_chained_handler() will
>> startup (unmask) the interrupt right away.
>>
>> In the cascade handler you take the virtual interrupt number, which
>> you get as argument, and find the mapping, i.e. the matching VIRTUAL
>> interrupt number for the VIRTUAL interrupt number and then call the
>> handler.
>>
>> How is this supposed to work?
> I think what is going on here is that the tb10x interrupt controller
> appears to be more of a front-end to another interrupt controller with
> each input wired up 1:1 to the interrupt inputs of the other controller.
> (I don't know why someone would design an interrupt controller that way,
> but that's another issue).

Actually ARC700 core has an integrated intc with 32 lines and ability to
mask/unmask each of the lines, priority/level per line etc. Simpler SoCs don't
need to have anymore.

> The loop above is mapping each of the
> interrupt inputs on the parent controller so that each child controller
> can be chained to it as an input. I can't think of how else it could be
> set up with the current code if the drivers were kept separate.
>
> Christian, what is the parent interrupt controller for this SoC? It
> really feels like the tb10x-ictl belongs as part of the parent
> controller. I went and looked at the parent node, and I saw this:
>
> intc: interrupt-controller {
> compatible = "snps,arc700-intc";
> interrupt-controller;
> #interrupt-cells = <1>;
> };
>
> I noticed the conspicuous absence of a reg property. Is this something
> architectural?

Indeed, the intc is not memory mapped. It is accessed via the separate AUX address
space (and separate r/w instructions) similar to x86 I/O address space.

> If I were working on this system I'd drop the
> snps,arc700-intc node entirely and have a single abilis,tb10x-intc that
> encapsulated the properties of both (you would of course want to share
> handler functions for the 'normal' inputs without the custom features).
> That would eliminate the goofyness of listing 27 separate interrupts in
> the abilis,tb10x-ictl interrupts property.

But how is this different from other systems with a primary in-core intc and a
cascaded external intc. How do they do it. I guess I need to read up more on this.

-Vineet
--
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/