linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Lina Iyer <ilina@codeaurora.org>
Cc: Stephen Boyd <swboyd@chromium.org>,
	Evan Green <evgreen@chromium.org>, Marc Zyngier <maz@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	MSM <linux-arm-msm@vger.kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	mkshah@codeaurora.org,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>
Subject: Re: [PATCH RFC v2 10/14] drivers: pinctrl: msm: setup GPIO chip in hierarchy
Date: Thu, 3 Oct 2019 14:17:39 +0200	[thread overview]
Message-ID: <CACRpkdav_BFubQ4-RWAN+uxBoExi7qfgdFhDVKfgtbXEOB5uvA@mail.gmail.com> (raw)
In-Reply-To: <1568411962-1022-11-git-send-email-ilina@codeaurora.org>

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"

> @@ -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!)

> +       /*
> +        * 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.

Other than that it looks fine!

Yours,
Linus Walleij

  reply	other threads:[~2019-10-03 12:17 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-13 21:59 [PATCH RFC v2 00/14] Support wakeup capable GPIOs Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 01/14] irqdomain: add bus token DOMAIN_BUS_WAKEUP Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 02/14] drivers: irqchip: qcom-pdc: update max PDC interrupts Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 03/14] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 04/14] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs Lina Iyer
2019-11-08 21:21   ` Doug Anderson
2019-11-08 21:54     ` Lina Iyer
2019-11-08 22:16       ` Lina Iyer
2019-11-08 22:57         ` Doug Anderson
2019-11-08 23:14           ` Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 05/14] of: irq: document properties for wakeup interrupt parent Lina Iyer
2019-10-03 12:02   ` Linus Walleij
2019-11-08 21:29   ` Doug Anderson
2019-09-13 21:59 ` [PATCH RFC v2 06/14] dt-bindings/interrupt-controller: pdc: add SPI config register Lina Iyer
2019-09-30 22:14   ` Rob Herring
2019-09-30 22:33   ` Stephen Boyd
2019-10-16  6:27     ` Stephen Boyd
2019-11-05 20:58       ` Lina Iyer
2019-11-06  0:53         ` Stephen Boyd
2019-11-11 18:37           ` Lina Iyer
2019-11-12 11:52             ` Marc Zyngier
2019-09-13 21:59 ` [PATCH RFC v2 07/14] drivers: irqchip: pdc: additionally set type in SPI config registers Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 08/14] genirq: Introduce irq_chip_get/set_parent_state calls Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 09/14] drivers: irqchip: pdc: Add irqchip set/get state calls Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 10/14] drivers: pinctrl: msm: setup GPIO chip in hierarchy Lina Iyer
2019-10-03 12:17   ` Linus Walleij [this message]
2019-11-13 18:35     ` Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 11/14] drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs Lina Iyer
2019-10-03 12:18   ` Linus Walleij
2019-09-13 21:59 ` [PATCH RFC v2 12/14] arm64: dts: qcom: add PDC interrupt controller for SDM845 Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 13/14] arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845 Lina Iyer
2019-09-13 21:59 ` [PATCH RFC v2 14/14] arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845 Lina Iyer

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=CACRpkdav_BFubQ4-RWAN+uxBoExi7qfgdFhDVKfgtbXEOB5uvA@mail.gmail.com \
    --to=linus.walleij@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=evgreen@chromium.org \
    --cc=ilina@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=mkshah@codeaurora.org \
    --cc=swboyd@chromium.org \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).