Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Lina Iyer <ilina@codeaurora.org>]

Hi Linus,

On Fri, Jun 28 2019 at 03:15 -0600, Linus Walleij wrote:
>Hi Lina,
>
>thanks for your comments!
>
>On Wed, Jun 26, 2019 at 10:09 PM Lina Iyer <ilina@codeaurora.org> wrote:
>
>> Thanks for the patch Linus. I was running into the warning in
>> gpiochip_set_irq_hooks(), because it was called from two places.
>> Hopefully, this will fix that as well. I will give it a try.
>
>That's usually when creating two irqchips from a static config.
>The most common solution is to put struct irq_chip into the
>driver state container and assign all functions dynamically so
>the irqchip is a "live" struct if you see how I mean. This is
>needed because the gpiolib irqchip core will augment some
>of the pointers in the irqchip, so if that is done twice, it feels
>a bit shaky.
>
Yeah, I realized what was happening. I have fixed it in my drivers.

>> On Mon, Jun 24 2019 at 07:29 -0600, Linus Walleij wrote:
>
>> >+  girq->num_parents = 1;
>> >+  girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
>> >+                               GFP_KERNEL);
>>
>> Could this be folded into the gpiolib?
>
>It is part of the existing API for providing an irq_chip along
>with the gpio_chip but you are right, it's kludgy. I do have
>a patch like this, making the parents a static sized field
>simply:
>https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=devel-gpio-irqchip
>
>So I might go on this approach. (In a separate patch.)
>
>> >+  /* Get a pointer to the gpio_irq_chip */
>> >+  girq = &g->gc.irq;
>> >+  girq->chip = &g->irq;
>> >+  girq->default_type = IRQ_TYPE_NONE;
>> >+  girq->handler = handle_bad_irq;
>> >+  girq->fwnode = g->fwnode;
>> >+  girq->parent_domain = parent;
>> >+  girq->child_to_parent_hwirq = my_gpio_child_to_parent_hwirq;
>> >+
>> Should be the necessary, if the driver implements it's own .alloc?
>
>The idea is that when using GPIOLIB_IRQCHIP and
>IRQ_DOMAIN_HIERARCHY together, the drivers utilizing
>GPIOLIB_IRQCHIP will rely on the .alloc() and .translate()
>implementations in gpiolib so the ambition is that these should
>cover all hierarchical use cases.
>
>> >+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
>> >+{
>> >+      if (!gc->irq.parent_domain) {
>> >+              chip_err(gc, "missing parent irqdomain\n");
>> >+              return -EINVAL;
>> >+      }
>> >+
>> >+      if (!gc->irq.parent_domain ||
>> >+          !gc->irq.child_to_parent_hwirq ||
>>
>> This should probably be validated if the .ops have not been set.
>
>Yeah the idea here is a pretty imperialistic one: the gpiolib
>always provide the ops for hierarchical IRQ. The library
>implementation should cover all needs of all consumers,
>for the hierarchical case.
>
>I might be wrong, but then I need to see some example
>of hierarchies that need something more than what the
>gpiolib core is providing and idiomatic enough that it can't
>be rewritten and absolutely must have its own ops.
>
Here is an example of what I am working on [1]. The series is based on
this patch. What I want to point out is the .alloc function. The TLMM
irqchip's parent could be a PDC or a MPM depending on the QCOM SoC
architecture. They behave differently. The PDC takes over for the GPIO
and handles the monitoring etc, while the MPM comes into play only after
the SoC is in low power therefore TLMM needs to do its job. The way to
cleanly support both of themis to have our own .alloc functions to help
understand the the wakeup-parent irqchip's behavior.

Since I need my own .ops, it makes the function below irrelevant to
gpiolib. While I would still need a function to translate to parent
hwirq, I don't see it any beneficial to gpiolib.

thanks,
Lina

>> >+      int (*child_to_parent_hwirq)(struct gpio_chip *chip,
>> >+                                   unsigned int child_hwirq,
>> >+                                   unsigned int child_type,
>> >+                                   unsigned int *parent_hwirq,
>> >+                                   unsigned int *parent_type);
>>
>> Would irq_fwspec(s) be better than passing all these arguments around?
>
>I looked over these three drivers that I patched in the series
>and they all seemed to need pretty much these arguments,
>so wrapping it into fwspec would probably just make all
>drivers have to unwrap them to get child (I guess not parent)
>back out.
>
>But we can patch it later if it proves this is too much arguments
>for some drivers. Its the right amount for those I changed,
>AFAICT.
>
>Yours,
>Linus Walleij

[1]. https://github.com/i-lina/linux/commits/gpio2-2