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

[Christian Ruppert <christian.ruppert@abilis.com>]

On Sat, Jun 01, 2013 at 01:01:33PM +0200, 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@linutronix.de> 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.
> 
> > > > +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.
> 
> Exactly. The TB10x interrupt controller is a front-end for the ARC CPU
> built-in interrupt controller.
> 
> > (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.
> 
> > 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. 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.
> 
> > 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.

Hello Thomas,

Any news about this one?

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates