All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	linux-tegra@vger.kernel.org,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains
Date: Tue, 25 Sep 2018 11:33:15 +0200	[thread overview]
Message-ID: <20180925093315.GB7097@ulmo> (raw)
In-Reply-To: <CACRpkdYnrf7FknvnZyFSgfT5aCrcWT4hxSnQo0UqxYfR6Fkqwg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 5149 bytes --]

On Tue, Sep 25, 2018 at 10:11:06AM +0200, Linus Walleij wrote:
> Hi Thierry!
> 
> Thanks for the patch!
> 
> I am a bit ignorant about irqdomains so I will probably need an ACK
> from some irq maintainer before I can apply this.
> 
> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Hierarchical IRQ domains can be used to stack different IRQ controllers
> > on top of each other. One specific use-case where this can be useful is
> > if a power management controller has top-level controls for wakeup
> > interrupts. In such cases, the power management controller can be a
> > parent to other interrupt controllers and program additional registers
> > when an IRQ has its wake capability enabled or disabled.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> While I think it is really important that we start supporting hierarchical
> irqdomains in the gpiolib core, I want a more complete approach,
> so that drivers that need hierarchical handling of irqdomains
> can get the same support from gpiolib as they get for simple
> domains.
> 
> > @@ -1918,7 +1918,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
> >                 type = IRQ_TYPE_NONE;
> >         }
> >
> > -       gpiochip->to_irq = gpiochip_to_irq;
> > +       if (!gpiochip->to_irq)
> > +               gpiochip->to_irq = gpiochip_to_irq;
> 
> So here you let the drivers override the .to_irq() function and that
> I think gets confusing as we are asking gpiolib to handle our
> irqchip.
> 
> 
> > -       gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> > -                                                    gpiochip->irq.first,
> > -                                                    ops, gpiochip);
> > +       if (gpiochip->irq.parent_domain)
> > +               gpiochip->irq.domain = irq_domain_add_hierarchy(gpiochip->irq.parent_domain,
> > +                                                               0, gpiochip->ngpio,
> > +                                                               np, ops, gpiochip);
> > +       else
> > +               gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> > +                                                            gpiochip->irq.first,
> > +                                                            ops, gpiochip);
> 
> So this part is great: if we pass in a parent domain the core helps us
> create the hierarchy.
> 
> But the stuff in .to_irq() should also be handled in the gpiolib core:
> the irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec) for
> example. That way you should not need any external .to_irq() function.
> 
> I can't see if you need to pull more stuff into the core to accomplish
> that, but I think in essence the core gpiolib needs to be more helpful
> with hierarchies.

This is not as trivial as it sounds. I think we could probably provide a
simple helper in the core that may work for the majority of GPIO
controllers, and would be similar to irq_domain_xlate_twocell(). The
problem is that ->gpio_to_irq() effectively needs to convert the offset
of the pin in the GPIO controller to an IRQ specifier. If the IRQ domain
can use irq_domain_xlate_twocell(), that should be easy, but if it does
not, then we likely need a custom implementation as well.

For example, as you may remember, the Tegra186 GPIO controller is
somewhat quirky in that it has a number of banks, each of which can have
any number of pins up to 8. However, in order to prevent users from
attempting to use one of the non-existent GPIOs, we resorted to
compacting the GPIO number space so that the GPIO specifier uses
basically a (bank, pin) pair that is converted into a GPIO offset. The
same is done for interrupt specifiers.

Since there is no 1:1 relationship between the value in the specifier
and the GPIO offset, we can't use irq_domain_xlate_twocell(). Similarly
we won't be able to use the standard gpiochip_to_irq() counterpart for
two cell specifiers to construct the IRQ specifier, but instead we'll
have to convert it based on the offset and the number of pins per bank
(that is, the inverse of what we do in tegra186_gpio_of_xlate()).

I think we can probably just implement the simple two-cell version in
gpiochip_to_irq() directly and leave it up to drivers that require
something more to override ->to_irq().

Another alternative that I had pondered about was to add another level
of indirection and have a generic implementation in gpiochip_to_irq()
that calls back into a new ->to_irq_fwspec() function that drivers can
implement to fill in the struct irq_fwspec as required by the
irq_domain_alloc_irqs() function. We could still provide a default
implementation for the common two-cell case, so most drivers wouldn't
have to know about it.

I don't think any of the above has massive advantages over the other.
The latter will be slightly more compact, I would assume, but the former
gives us more flexibility if we need it.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2018-09-25  9:33 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-21 10:25 [PATCH 0/9] Implement wake event support on Tegra186 and later Thierry Reding
2018-09-21 10:25 ` [PATCH 1/9] dt-bindings: tegra186-pmc: Add interrupt controller properties Thierry Reding
2018-10-15 14:47   ` Rob Herring
2018-10-15 14:47     ` Rob Herring
2018-09-21 10:25 ` [PATCH 2/9] soc/tegra: pmc: Add Tegra194 support Thierry Reding
2018-09-21 10:25 ` [PATCH 3/9] soc/tegra: pmc: Add wake event support Thierry Reding
2018-09-21 10:35   ` Mikko Perttunen
2018-09-21 10:25 ` [PATCH 4/9] soc/tegra: pmc: Add initial Tegra186 wake events Thierry Reding
2018-09-21 10:25 ` [PATCH 5/9] soc/tegra: pmc: Add initial Tegra194 " Thierry Reding
2018-09-21 10:25 ` [PATCH 6/9] gpio: Add support for hierarchical IRQ domains Thierry Reding
2018-09-25  8:11   ` Linus Walleij
2018-09-25  8:11     ` Linus Walleij
2018-09-25  9:33     ` Thierry Reding [this message]
2018-09-25  9:33       ` Thierry Reding
2018-09-25 10:33       ` Linus Walleij
2018-09-25 10:33         ` Linus Walleij
2018-09-25 11:17         ` Thierry Reding
2018-09-25 11:17           ` Thierry Reding
2018-10-03  7:52           ` Linus Walleij
2018-10-03  7:52             ` Linus Walleij
2018-09-21 10:25 ` [PATCH 7/9] dt-bindings: tegra186-gpio: Add wakeup parent support Thierry Reding
2018-09-21 10:37   ` Mikko Perttunen
2018-10-15 14:46   ` Rob Herring
2018-11-28 10:44     ` Thierry Reding
2018-09-21 10:25 ` [PATCH 8/9] gpio: tegra186: Rename flow variable to type Thierry Reding
2018-09-21 10:25 ` [PATCH 9/9] gpio: tegra186: Implement wake event support Thierry Reding
2018-09-25  8:48 ` [PATCH 0/9] Implement wake event support on Tegra186 and later Linus Walleij
2018-09-25  8:48   ` Linus Walleij
2018-09-25  9:57   ` Thierry Reding
2018-09-25  9:57     ` Thierry Reding
2018-09-25 17:16     ` Lina Iyer
2018-09-25 17:16       ` Lina Iyer
2018-09-25 17:16       ` Lina Iyer
2018-10-08  7:14       ` Stephen Boyd
2018-10-08  7:14         ` Stephen Boyd
2018-10-09 12:58         ` Marc Zyngier
2018-10-09 12:58           ` Marc Zyngier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180925093315.GB7097@ulmo \
    --to=thierry.reding@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.