lkml.org 
[lkml]   [2019]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC v2 10/14] drivers: pinctrl: msm: setup GPIO chip in hierarchy
On Thu, Oct 03 2019 at 06:17 -0600, Linus Walleij wrote:
>On Fri, Sep 13, 2019 at 11:59 PM Lina Iyer <ilina@codeaurora.org> wrote:
>
>> Some GPIOs are marked as wakeup capable and are routed to another
>> interrupt controller that is an always-domain and can detect interrupts
>> even most of the SoC is powered off. The wakeup interrupt controller
>> wakes up the GIC and replays the interrupt at the GIC.
>>
>> Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller
>> and ensure the wakeup GPIOs are handled correctly.
>>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ----
>> Changes in RFC v2:
>> - Define irq_domain_qcom_handle_wakeup()
>> - Rebase on top of GPIO hierarchy support in linux-next
>> - Set the chained irq handler for summary line
>
>This is looking better every time I look at it, it's really complex
>but alas the problem is hard to solve so it requires complex solutions.
>
>> @@ -1006,6 +1091,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>> struct gpio_irq_chip *girq;
>> int ret;
>> unsigned ngpio = pctrl->soc->ngpios;
>> + struct device_node *dn;
>
>I usually call the variable "np"
>
Will change.

>> @@ -1021,17 +1107,40 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>
>> pctrl->irq_chip.name = "msmgpio";
>> pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
>> + pctrl->irq_chip.irq_disable = msm_gpio_irq_disable;
>> pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
>> pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
>> pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
>> + pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
>
>This part and the functions called seem fine!
>
>> + dn = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
>> + if (dn) {
>> + int i;
>> + bool skip;
>> + unsigned int gpio;
>> +
>> + chip->irq.parent_domain = irq_find_matching_host(dn,
>> + DOMAIN_BUS_WAKEUP);
>> + of_node_put(dn);
>> + if (!chip->irq.parent_domain)
>> + return -EPROBE_DEFER;
>> + chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq;
>> +
>> + skip = irq_domain_qcom_handle_wakeup(chip->irq.parent_domain);
>> + for (i = 0; skip && i < pctrl->soc->nwakeirq_map; i++) {
>> + gpio = pctrl->soc->wakeirq_map[i].gpio;
>> + set_bit(gpio, pctrl->skip_wake_irqs);
>> + }
>> + }
>
>OK I guess this is how we should do it, maybe add a comment to clarify
>that we are checking the parent irqdomain of the chained IRQ to see
>if we need to avoid disabling the irq as it is used for wakeup. (IIUC
>what the code does!)
>
Okay.

>> + /*
>> + * Since we are chained to the GIC using the TLMM summary line
>> + * and in hierarchy with the wakeup parent interrupt controller,
>> + * explicitly set the chained summary line. We need to do this because
>> + * the summary line is not routed to the wakeup parent but directly
>> + * to the GIC.
>> + */
>> + gpiochip_set_chained_irqchip(chip, &pctrl->irq_chip, pctrl->irq,
>> + msm_gpio_irq_handler);
>
>I don't think this part is needed, we already have:
>
>girq->parent_handler = msm_gpio_irq_handler;
>girq->num_parents = 1;
>girq->parents = devm_kcalloc(pctrl->dev, 1, sizeof(*girq->parents),
> GFP_KERNEL);
>if (!girq->parents)
> return -ENOMEM;
>girq->default_type = IRQ_TYPE_NONE;
>girq->handler = handle_bad_irq;
>girq->parents[0] = pctrl->irq;
>
>This will make the irq chain when calling gpiochip_add_data(), so
>just delete this and see if everything works as before.
>
I thought it didn't work without this change and I am not sure why it
started working after I did. May be it was a bad set of patches that I
pulled in.

>Other than that it looks fine!
Thanks for your review.

--Lina

\
 
 \ /
  Last update: 2019-11-13 19:35    [W:0.069 / U:2.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site