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

[Vineet Gupta]

On 06/01/2013 04:31 PM, Christian Ruppert wrote:
> On Fri, May 31, 2013 at 11:18:14PM +0100, Grant Likely wrote:
>> On Fri, 31 May 2013 19:32:34 +0200 (CEST), Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>>> On Fri, 31 May 2013, Christian Ruppert wrote:
>>>
>>>> The SOC interrupt controller driver for the Abilis Systems TB10x series of
>>>> SOCs based on ARC700 CPUs.
>>>>
>>>> This patch depends on commits eb76bdd407d8a90e59a06cb0158886df390e5d1c and
>>>> 712bc93df9e7f14b8a163148d2aa7c778e151627 from branch irq/for-arm of
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git.
>>> That branch can be pulled into ARC as well. It only contains the
>>> changes, which are necessary for the irq domain support of the generic
>>> irq chip.
> Vineet, what do you think about this? For the moment I have pulled the
> patch set into our local branch and to me it doesn't matter, we just
> have to make sure to respect this dependency when merging everything
> together.

Yes what you did is right. I don't have to back-merge any of this into my tree -
given that I'm not testing the v3 intc here. In 3.11 all respective changes will
go in via the respective trees anyways.

>
>> (I don't know why someone would design an interrupt controller that way,
>> but that's another issue).
> There are several technical reasons for this front-end. The one that
> concerns us most in the kernel is that the TB10x front-end does the
> translation from all kinds of interrupt trigger modes to the level
> triggered interrupts natively understood by the ARC CPU built-in
> controller.

As I mentioned in prev email,  arc in-core intc always exists, providing
reasonable functionality so that simpler SoCs don't have to do that themselves.


>> 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.
> This is exactly the intention. I haven't found an easier way to do this
> either but I'm open to suggestions.

But the child intc must only uplink to one parent intc IRQ line. In that case why
do we need to enumerate the rest of them in code. Is that a limitation of
framework or the parent intc (with it's legacy domain instantiation) is not doing
something correct.

>  Btw, I have noticed that the parent
> controller interrupts from this loop are not listed in /proc/interrupts.
> I'm not sure if what is done in the loop is sufficient or if I should
> add something else (the naive option of using request_irq doesn't work,
> the kernel saying something in the lines of "irq XX triggered but noone
> cares").
>
>> 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?
> The parent controller is part of the CPU itself, see
> arch/arc/kernel/irq.c. This controller is maintained by Vineet and IMHO
> we should keep it separate from the TB10x one since it is implicitly
> used in all ARC-based platforms whereas the TB10x controller is used in
> Abilis chips only.

Ofcourse they have to separate. The question is, what do we need to do to avoid
clunkiness in code for the cascaded intc.

>> 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.
> To complicate things even further, some ARC CPU built-in peripherals
> (e.g. timers) generate interrupts directly to the ARC built-in interrupt
> controller (without going through the TB10x front-end), hence the
> "goofy" list of interrupts in the TB10x DT node.

But I presume that this is common-place for cascaded intc setups - some
peripherals hooking up directly to topmost intc would be normal. But it seems I'm
not as educated in area as I really should be.

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