lkml.org 
[lkml]   [2013]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V3] irqchip: Add TB10x interrupt controller driver
    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2013-06-13 11:21    [W:2.039 / U:0.108 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site