From: Stephen Boyd <swboyd@chromium.org>
To: Lina Iyer <ilina@codeaurora.org>,
bjorn.andersson@linaro.org, linus.walleij@linaro.org,
maz@kernel.org
Cc: evgreen@chromium.org, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, mkshah@codeaurora.org,
linux-gpio@vger.kernel.org, agross@kernel.org,
dianders@chromium.org, Lina Iyer <ilina@codeaurora.org>
Subject: Re: [PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy
Date: Fri, 15 Nov 2019 11:33:25 -0800 [thread overview]
Message-ID: <5dcefd86.1c69fb81.c72c8.07a1@mx.google.com> (raw)
In-Reply-To: <1573756521-27373-9-git-send-email-ilina@codeaurora.org>
Quoting Lina Iyer (2019-11-14 10:35:17)
> 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
even when?
> 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>
Is it Co-developed-by for Maulik?
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Some minor comments. Shouldn't be hard to fix and resend quickly I hope.
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 763da0b..c245686 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -44,6 +46,7 @@
> * @enabled_irqs: Bitmap of currently enabled irqs.
> * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
> * detection.
> + * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt contrroller
s/contrroller/controller/
> * @soc; Reference to soc_data of platform specific data.
> * @regs: Base addresses for the TLMM tiles.
> */
> @@ -778,10 +794,37 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
>
> static void msm_gpio_irq_enable(struct irq_data *d)
> {
> + /*
> + * Clear the interrupt that may be pending before we enable
> + * the line.
> + * This is especially a problem with the GPIOs routed to the
> + * PDC. These GPIOs are direct-connect interrupts to the GIC.
> + * Disabling the interrupt line at the PDC does not prevent
> + * the interrupt from being latched at the GIC. The state at
> + * GIC needs to be cleared before enabling.
> + */
> + if (d->parent_data) {
> + irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
> + irq_chip_enable_parent(d);
> + }
>
> msm_gpio_irq_clear_unmask(d, true);
> }
>
> +static void msm_gpio_irq_disable(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +
> + if (d->parent_data)
> + irq_chip_disable_parent(d);
> +
> + if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
> + return;
> +
> + msm_gpio_irq_mask(d);
Why not
if (!test_bit(...)
msm_gpio_irq_mask(d);
> +}
> +
> static void msm_gpio_irq_unmask(struct irq_data *d)
> {
> msm_gpio_irq_clear_unmask(d, false);
> @@ -912,6 +964,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
> struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> unsigned long flags;
>
> + if (d->parent_data)
> + irq_chip_set_wake_parent(d, on);
> +
> + /*
> + * While they may not wake up when the TLMM is powered off,
> + * some GPIOs would like to wakeup the system from suspend
> + * when TLMM is powered on. To allow that, enable the GPIO
> + * summary line to be wakeup capable at GIC.
> + */
Can this comment go above the irq_set_irq_wake() line below instead of
this spinlock?
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> irq_set_irq_wake(pctrl->irq, on);
> @@ -990,6 +1051,30 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> +static int msm_gpio_wakeirq(struct gpio_chip *gc,
> + unsigned int child,
> + unsigned int child_type,
> + unsigned int *parent,
> + unsigned int *parent_type)
> +{
> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> + const struct msm_gpio_wakeirq_map *map;
> + int i;
> +
> + *parent = GPIO_NO_WAKE_IRQ;
> + *parent_type = IRQ_TYPE_EDGE_RISING;
> +
> + for (i = 0; i < pctrl->soc->nwakeirq_map; i++) {
> + map = &pctrl->soc->wakeirq_map[i];
> + if (map->gpio == child) {
> + *parent = map->wakeirq;
> + break;
> + }
> + }
> +
> + return 0;
Shouldn't we return -EINVAL if we can't translate the gpio irq to the
parent domain? I would expect to see return -EINVAL here and the above
if condition to return 0 instead of break.
> +}
> +
> static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
> {
> if (pctrl->soc->reserved_gpios)
> @@ -1004,6 +1089,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 *np;
>
> if (WARN_ON(ngpio > MAX_NR_GPIO))
> return -EINVAL;
> @@ -1020,17 +1106,44 @@ 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;
> pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
> pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
> pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
> pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
>
> + np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
> + if (np) {
> + int i;
> + bool skip;
> + unsigned int gpio;
Can these be placed at the top of this function instead of buried
halfway down here?
> +
> + chip->irq.parent_domain = irq_find_matching_host(np,
> + DOMAIN_BUS_WAKEUP);
> + of_node_put(np);
> + if (!chip->irq.parent_domain)
> + return -EPROBE_DEFER;
> + chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq;
> +
> + /*
> + * Let's skip handling the GPIOs, if the parent irqchip
> + * handling the direct connect IRQ of the GPIO.
is handling?
> + */
> + 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);
> + }
> + }
> +
> girq = &chip->irq;
> girq->chip = &pctrl->irq_chip;
> girq->parent_handler = msm_gpio_irq_handler;
> + girq->fwnode = pctrl->dev->fwnode;
> girq->num_parents = 1;
> girq->parents = devm_kcalloc(pctrl->dev, 1, sizeof(*girq->parents),
> GFP_KERNEL);
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 48569cda8..1547020 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -5,6 +5,8 @@
> #ifndef __PINCTRL_MSM_H__
> #define __PINCTRL_MSM_H__
>
> +#include <linux/gpio/driver.h>
What is this include for?
> +
> struct pinctrl_pin_desc;
>
> /**
> @@ -101,6 +113,8 @@ struct msm_pingroup {
> * @ngroups: The numbmer of entries in @groups.
> * @ngpio: The number of pingroups the driver should expose as GPIOs.
> * @pull_no_keeper: The SoC does not support keeper bias.
> + * @wakeirq_map: The map of wakeup capable GPIOs and the pin at PDC/MPM
> + * @nwakeirq_map: The number of entries in @hierarchy_map
Is it 'number of entries in @wakeirq_map"?
> */
> struct msm_pinctrl_soc_data {
> const struct pinctrl_pin_desc *pins;
> @@ -114,6 +128,8 @@ struct msm_pinctrl_soc_data {
> const char *const *tiles;
> unsigned int ntiles;
> const int *reserved_gpios;
> + const struct msm_gpio_wakeirq_map *wakeirq_map;
> + unsigned int nwakeirq_map;
> };
>
> extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
> index 637c0bf..e01391c 100644
> --- a/include/linux/soc/qcom/irq.h
> +++ b/include/linux/soc/qcom/irq.h
> @@ -18,4 +18,17 @@
> #define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 0)
> #define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 1)
>
> +/**
> + * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt
> + * configuration
> + * @d: irq domain
> + *
> + * This QCOM specific irq domain call returns if the interrupt controller
> + * requires the interrupt be masked at the child interrupt controller.
> + */
> +static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *d)
could be const irq_domain here.
> +{
> + return (d->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP);
> +}
> +
next prev parent reply other threads:[~2019-11-15 19:33 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-14 18:35 [PATCH 00/12] Support wakeup capable GPIOs Lina Iyer
2019-11-14 18:35 ` [PATCH 01/12] irqdomain: add bus token DOMAIN_BUS_WAKEUP Lina Iyer
2019-11-15 18:48 ` Stephen Boyd
2019-11-14 18:35 ` [PATCH 02/12] drivers: irqchip: qcom-pdc: update max PDC interrupts Lina Iyer
2019-11-15 18:48 ` Stephen Boyd
2019-11-14 18:35 ` [PATCH 03/12] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask Lina Iyer
2019-11-15 18:49 ` Stephen Boyd
2019-11-14 18:35 ` [PATCH 04/12] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs Lina Iyer
2019-11-15 19:03 ` Stephen Boyd
2019-11-14 18:35 ` [PATCH 05/12] of: irq: document properties for wakeup interrupt parent Lina Iyer
2019-11-15 19:03 ` Stephen Boyd
2019-11-14 18:35 ` [PATCH 06/12] genirq: Introduce irq_chip_get/set_parent_state calls Lina Iyer
2019-11-15 19:04 ` Stephen Boyd
2019-11-14 18:35 ` [PATCH 07/12] drivers: irqchip: pdc: Add irqchip set/get state calls Lina Iyer
2019-11-15 19:05 ` Stephen Boyd
2019-11-14 18:35 ` [PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy Lina Iyer
2019-11-14 18:43 ` Linus Walleij
2019-11-14 18:57 ` Lina Iyer
2019-11-15 19:33 ` Stephen Boyd [this message]
2019-11-15 20:55 ` Lina Iyer
2019-11-15 21:57 ` Lina Iyer
2019-11-15 22:09 ` Stephen Boyd
2019-11-15 22:13 ` Lina Iyer
2019-11-14 18:35 ` [PATCH 09/12] drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs Lina Iyer
2019-11-15 19:33 ` Stephen Boyd
2019-11-14 18:35 ` [PATCH 10/12] arm64: dts: qcom: add PDC interrupt controller for SDM845 Lina Iyer
2019-11-15 19:34 ` Stephen Boyd
2019-11-14 18:35 ` [PATCH 11/12] arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845 Lina Iyer
2019-11-15 19:34 ` Stephen Boyd
2019-11-14 18:35 ` [PATCH 12/12] arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845 Lina Iyer
2019-11-15 19:34 ` Stephen Boyd
2019-11-15 10:08 ` [PATCH 00/12] Support wakeup capable GPIOs Marc Zyngier
2019-11-15 16:20 ` Lina Iyer
2019-11-15 19:35 ` Stephen Boyd
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=5dcefd86.1c69fb81.c72c8.07a1@mx.google.com \
--to=swboyd@chromium.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=dianders@chromium.org \
--cc=evgreen@chromium.org \
--cc=ilina@codeaurora.org \
--cc=linus.walleij@linaro.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 \
/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).