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

[Vineet Gupta <Vineet.Gupta1@synopsys.com>]

On 06/01/2013 03:48 AM, Grant Likely wrote:
> On Fri, 31 May 2013 19:32:34 +0200 (CEST), Thomas Gleixner <tglx@linutronix.de> 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