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

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

On Fri, Dec 18, 2020 at 3:49 PM Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
>
> >
> > 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:
>

It's clear to me that I need to finally educate myself more on
hierarchical IRQs. I don't want to block this patch though until that
happens. I trust that Thierry knows what he's doing here and so I've
applied it for next.

Bart