Re: [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip

[Thierry Reding <thierry.reding@gmail.com>]

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

On Sat, Dec 05, 2020 at 10:33:24PM +0100, Linus Walleij wrote:
> On Fri, Nov 27, 2020 at 3:09 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Convert the Tegra GPIO driver to use the gpio_irq_chip infrastructure.
> > This allows a bit of boiler plate to be removed and while at it enables
> > support for hierarchical domains, which is useful to support PMC wake
> > events on Tegra210 and earlier.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> The patch didn't apply to my "devel" branch for some reason
> so have a look at that, seems gpio-tegra.c has some changes not
> in my tree.
> 
> >  struct tegra_gpio_soc_config {
> > @@ -93,12 +91,12 @@ struct tegra_gpio_soc_config {
> >  struct tegra_gpio_info {
> >         struct device                           *dev;
> >         void __iomem                            *regs;
> > -       struct irq_domain                       *irq_domain;
> >         struct tegra_gpio_bank                  *bank_info;
> >         const struct tegra_gpio_soc_config      *soc;
> >         struct gpio_chip                        gc;
> >         struct irq_chip                         ic;
> >         u32                                     bank_count;
> > +       unsigned int                            *irqs;
> 
> So this is hierarchical with several IRQs.
> 
> >  static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> >  {
> >         unsigned int gpio = d->hwirq, port = GPIO_PORT(gpio), lvl_type;
> > -       struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> > -       struct tegra_gpio_info *tgi = bank->tgi;
> > +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> > +       struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> > +       struct tegra_gpio_bank *bank;
> >         unsigned long flags;
> > -       u32 val;
> >         int ret;
> > +       u32 val;
> > +
> > +       bank = &tgi->bank_info[GPIO_BANK(d->hwirq)];
> 
> So the general idea is to look up the bank from the IRQ offset.
> 
> But...
> 
> > -       return 0;
> > +       if (d->parent_data)
> > +               ret = irq_chip_set_type_parent(d, type);
> > +
> > +       return ret;
> 
> I don't quite get this. This makes sense if there is one parent IRQ
> per interrupt, but if one of the users of a GPIO in a bank sets the
> IRQ type to edge and then another one comes in and set another
> of the lines to level and then the function comes here, what type
> gets set on the parent? Whichever comes last?
> 
> Normally with banked GPIOs collecting several lines in a cascaded
> fashion, the GPIO out of the bank toward the GIC is level triggered.
> 
> I don't understand how this GPIO controller can be hierarchical,
> it looks cascaded by the definition of the document
> Documentation/driver-api/gpio/driver.rst

This is basically the same implementation that we've used in the
gpio-tegra186 driver. The goal here is to support wake events, which are
a mechanism for the PMC (which, among other things control wakeup of the
CPU complex from sleep). Wake events are a somewhat non-trivial concept
and I keep second-guessing this myself everytime I look at it...

So basically with these wake events we have a selected number of GPIOs
that are routed to the PMC and which can wake the system from sleep. To
make this work, the PMC IRQ domain becomes the parent of the GPIO IRQ
domain, so what we're forwarding the ->set_type() and ->set_wake()
operations to here is the PMC parent, rather than the parent IRQs which
are, I suppose, somewhat unfortunately named for this particular use-
case.

I suppose given the definition in the documentation the GPIO controller
is both hierarchical (it's a child of the PMC IRQ domain) and cascaded
(sets of GPIOs routed to a number of "parent" interrupts).

What usually helps me in understanding this better is to look at GPIO
and IRQ functionality as separate things. The GPIO controller is
cascaded from the point of view of the GPIOs and how the Linux virtual
interrupts are mapped to physical interrupts. On the other hand the GPIO
controller is hierarchical from an IRQ domain point of view because some
of the GPIO interrupts also have a parent interrupt in the PMC.

I hope that clarifies things a little bit. More specifically the
irq_chip_set_type_parent() isn't actually going to cause the type to be
set on the cascaded interrupts that go to the GIC, but on the parent
interrupts within the PMC (i.e. the wake events) which have separate
registers to program the type and wake enables.

Note that because not all GPIOs may have a corresponding wake event
(i.e. no parent, hierarchically speaking) that's also why we first
check for data->parent_data. See this email thread for a bit more
background information from Marc, who added proper support for this
recently:

	https://lore.kernel.org/lkml/20201007124544.1397322-1-maz@kernel.org/

Thierry

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