| Message ID | 20180824200157.9993-1-ilina@codeaurora.org |
|---|---|
| Headers | show |
| Series | Wakeup GPIO support for SDM845 SoC | expand |
Hi Lina, On Fri, Aug 24, 2018 at 02:01:53PM -0600, Lina Iyer wrote: > QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on > domain can wakeup the SoC, when interrupts and GPIOs are routed to the > its interrupt controller. Only select GPIOs that are deemed wakeup wording nit: "are routed to the|its interrupt controller" > capable are routed to specific PDC pins. During low power state, the > pinmux interrupt controller may be non-functional but the PDC would be. > The PDC can detect the wakeup GPIO is triggered and bring the TLMM to an > operational state. > > Interrupts that are level triggered will be detected at the TLMM when > the controller becomes operational. Edge interrupts however need to be > replayed again. > > Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ, > but keep it disabled. During suspend, we can enable the PDC IRQ instead > of the GPIO IRQ, which may or not be detected. > > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > --- > Changes in v2: > - Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ > Changes in v1: > - Trigger GPIO in h/w from PDC IRQ handler > - Avoid big tables for GPIO-PDC map, pick from DT instead > - Use handler_data > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 96 ++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 0e22f52b2a19..b675ea56a4ff 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) > const struct msm_pingroup *g; > unsigned long flags; > u32 val; > + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); > > g = &pctrl->soc->groups[d->hwirq]; > > raw_spin_lock_irqsave(&pctrl->lock, flags); > > + if (pdc_irqd) > + irq_set_irq_type(pdc_irqd->irq, type); > + > /* > * For hw without possibility of detecting both edges > */ > @@ -779,9 +783,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > unsigned long flags; > + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); > > raw_spin_lock_irqsave(&pctrl->lock, flags); > > + if (pdc_irqd) > + irq_set_irq_wake(pdc_irqd->irq, on); > + > irq_set_irq_wake(pctrl->irq, on); > > raw_spin_unlock_irqrestore(&pctrl->lock, flags); > @@ -863,6 +871,92 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) > return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0; > } > > +static irqreturn_t wake_irq_gpio_handler(int irq, void *data) > +{ > + struct irq_data *irqd = data; > + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd); > + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > + const struct msm_pingroup *g; > + unsigned long flags; > + u32 val; > + > + if (!irqd_is_level_type(irqd)) { > + g = &pctrl->soc->groups[irqd->hwirq]; > + raw_spin_lock_irqsave(&pctrl->lock, flags); > + val = BIT(g->intr_status_bit); > + writel(val, pctrl->regs + g->intr_status_reg); > + raw_spin_unlock_irqrestore(&pctrl->lock, flags); > + } > + > + return IRQ_HANDLED; > +} > + > +static int msm_gpio_pdc_pin_request(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > + struct platform_device *pdev = to_platform_device(pctrl->dev); > + const char *pin_name; > + int irq; > + int ret; > + > + pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq); > + if (!pin_name) > + return -ENOMEM; > + > + irq = platform_get_irq_byname(pdev, pin_name); > + if (irq < 0) { > + kfree(pin_name); > + return 0; Do I understand correctly that this is the case where the pin isn't routed to the PDC? > + } > + > + ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d), > + pin_name, d); > + if (ret) { > + pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq); '\n' is missing > + kfree(pin_name); > + return ret; > + } > + > + irq_set_handler_data(d->irq, irq_get_irq_data(irq)); > + disable_irq(irq); > + > + return 0; > +} > + > +static int msm_gpio_pdc_pin_release(struct irq_data *d) > +{ > + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); > + > + if (pdc_irqd) { > + irq_set_handler_data(d->irq, NULL); > + free_irq(pdc_irqd->irq, d); You need to free 'pin_name' allocated in msm_gpio_pdc_pin_request(). IIUC it should be available in irq_desc->action->name. Cheers Matthias
Hi Lina, On Fri, Aug 24, 2018 at 02:01:55PM -0600, Lina Iyer wrote: > During suspend the system may power down some of the system rails. As a > result, the TLMM hw block may not be operational anymore and wakeup > capable GPIOs will not be detected. The PDC however will be operational > and the GPIOs that are routed to the PDC as IRQs can wake the system up. > > To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a > GPIO trips, use TLMM for active and switch to PDC for suspend. When > entering suspend, disable the TLMM wakeup interrupt and instead enable > the PDC IRQ and revert upon resume. > > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > --- > Changes in v2: > - Fix PDC IRQ max port, 126 is the max supported in h/w > - Use PDC hwirq in bitmap, linux numbers could be large > - Setup DISABLE_UNLAZY for both TLMM and PDC IRQs > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 70 +++++++++++++++++++++++++++++- > drivers/pinctrl/qcom/pinctrl-msm.h | 3 ++ > 2 files changed, 72 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index b675ea56a4ff..a880cefbd248 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -37,6 +37,7 @@ > #include "../pinctrl-utils.h" > > #define MAX_NR_GPIO 300 > +#define MAX_PDC_HWIRQ 126 > #define PS_HOLD_OFFSET 0x820 > > /** > @@ -51,6 +52,7 @@ > * @enabled_irqs: Bitmap of currently enabled irqs. > * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge > * detection. > + * @pdc_hwirqs: Bitmap of wakeup capable irqs. > * @soc; Reference to soc_data of platform specific data. > * @regs: Base address for the TLMM register map. > */ > @@ -68,11 +70,15 @@ struct msm_pinctrl { > > DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO); > DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO); > + DECLARE_BITMAP(pdc_hwirqs, MAX_PDC_HWIRQ); > > const struct msm_pinctrl_soc_data *soc; > void __iomem *regs; > + struct irq_domain *pdc_irq_domain; > }; > > +static bool in_suspend; > + > static int msm_get_groups_count(struct pinctrl_dev *pctldev) > { > struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > @@ -787,8 +793,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > > raw_spin_lock_irqsave(&pctrl->lock, flags); > > - if (pdc_irqd) > + if (pdc_irqd && !in_suspend) { > irq_set_irq_wake(pdc_irqd->irq, on); > + if (on) > + set_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs); > + else > + clear_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs); > + } > > irq_set_irq_wake(pctrl->irq, on); > > @@ -919,7 +930,12 @@ static int msm_gpio_pdc_pin_request(struct irq_data *d) > } > > irq_set_handler_data(d->irq, irq_get_irq_data(irq)); > + irq_set_handler_data(irq, d); > + irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY); > + irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY); > disable_irq(irq); > + if (!pctrl->pdc_irq_domain) > + pctrl->pdc_irq_domain = irq_get_irq_data(irq)->domain; > > return 0; > } > @@ -1069,6 +1085,58 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) > } > } > > +int __maybe_unused msm_pinctrl_suspend_late(struct device *dev) > +{ > + struct msm_pinctrl *pctrl = dev_get_drvdata(dev); > + struct irq_data *irqd; > + unsigned int irq; > + int i; > + > + in_suspend = true; > + for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) { > + irq = irq_find_mapping(pctrl->pdc_irq_domain, i); > + irqd = irq_get_handler_data(irq); > + /* > + * We don't know if the TLMM will be functional > + * or not, during the suspend. If its functional, > + * we do not want duplicate interrupts from PDC. > + * Hence disable the GPIO IRQ and enable PDC IRQ. > + */ > + if (irqd_is_wakeup_set(irqd)) { > + disable_irq_wake(irqd->irq); > + disable_irq(irqd->irq); > + enable_irq(irq); > + } Would it make sense to limit this to edge triggered interrupts since the interrupt handler does nothing for level triggered ones? Cheers Matthias
On Fri 24 Aug 13:01 PDT 2018, Lina Iyer wrote: > Enable TLMM IRQs to be sensed by PDC when we enter suspend. It is > possible that the TLMM may be powered off and not detect GPIOs that are > configured as wake up interrupts. By hooking into suspend callbacks, we > allow PDC IRQs to take over and wake up the system if wakeup interrupts > are triggered. > > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > --- > drivers/pinctrl/qcom/pinctrl-sdm845.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c > index 2ab7a8885757..cc333b8afb99 100644 > --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c > +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c > @@ -1297,10 +1297,16 @@ static const struct of_device_id sdm845_pinctrl_of_match[] = { > { }, > }; > > +static const struct dev_pm_ops msm_pinctrl_dev_pm_ops = { > + SET_LATE_SYSTEM_SLEEP_PM_OPS(msm_pinctrl_suspend_late, > + msm_pinctrl_resume_late) > +}; > + I expect these four lines to be duplicated in every platform file, so I think it would be better to just move it to pinctrl-msm.c and extern declare it in pinctrl-msm.h. > static struct platform_driver sdm845_pinctrl_driver = { > .driver = { > .name = "sdm845-pinctrl", > .of_match_table = sdm845_pinctrl_of_match, > + .pm = &msm_pinctrl_dev_pm_ops, > }, > .probe = sdm845_pinctrl_probe, > .remove = msm_pinctrl_remove, Regards, Bjorn
On Mon, Aug 27 2018 at 18:31 -0600, Bjorn Andersson wrote: >On Fri 24 Aug 13:01 PDT 2018, Lina Iyer wrote: > >> Enable TLMM IRQs to be sensed by PDC when we enter suspend. It is >> possible that the TLMM may be powered off and not detect GPIOs that are >> configured as wake up interrupts. By hooking into suspend callbacks, we >> allow PDC IRQs to take over and wake up the system if wakeup interrupts >> are triggered. >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> drivers/pinctrl/qcom/pinctrl-sdm845.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c >> index 2ab7a8885757..cc333b8afb99 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c >> +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c >> @@ -1297,10 +1297,16 @@ static const struct of_device_id sdm845_pinctrl_of_match[] = { >> { }, >> }; >> >> +static const struct dev_pm_ops msm_pinctrl_dev_pm_ops = { >> + SET_LATE_SYSTEM_SLEEP_PM_OPS(msm_pinctrl_suspend_late, >> + msm_pinctrl_resume_late) >> +}; >> + > >I expect these four lines to be duplicated in every platform file, so I >think it would be better to just move it to pinctrl-msm.c and extern >declare it in pinctrl-msm.h. > Sure. >> static struct platform_driver sdm845_pinctrl_driver = { >> .driver = { >> .name = "sdm845-pinctrl", >> .of_match_table = sdm845_pinctrl_of_match, >> + .pm = &msm_pinctrl_dev_pm_ops, >> }, >> .probe = sdm845_pinctrl_probe, >> .remove = msm_pinctrl_remove, > >Regards, >Bjorn
On Mon, Aug 27 2018 at 16:57 -0600, Matthias Kaehlcke wrote: >Hi Lina, > >On Fri, Aug 24, 2018 at 02:01:55PM -0600, Lina Iyer wrote: >> During suspend the system may power down some of the system rails. As a >> result, the TLMM hw block may not be operational anymore and wakeup >> capable GPIOs will not be detected. The PDC however will be operational >> and the GPIOs that are routed to the PDC as IRQs can wake the system up. >> >> To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a >> GPIO trips, use TLMM for active and switch to PDC for suspend. When >> entering suspend, disable the TLMM wakeup interrupt and instead enable >> the PDC IRQ and revert upon resume. >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> Changes in v2: >> - Fix PDC IRQ max port, 126 is the max supported in h/w >> - Use PDC hwirq in bitmap, linux numbers could be large >> - Setup DISABLE_UNLAZY for both TLMM and PDC IRQs >> --- >> drivers/pinctrl/qcom/pinctrl-msm.c | 70 +++++++++++++++++++++++++++++- >> drivers/pinctrl/qcom/pinctrl-msm.h | 3 ++ >> 2 files changed, 72 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> index b675ea56a4ff..a880cefbd248 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> @@ -37,6 +37,7 @@ >> #include "../pinctrl-utils.h" >> >> #define MAX_NR_GPIO 300 >> +#define MAX_PDC_HWIRQ 126 >> #define PS_HOLD_OFFSET 0x820 >> >> /** >> @@ -51,6 +52,7 @@ >> * @enabled_irqs: Bitmap of currently enabled irqs. >> * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge >> * detection. >> + * @pdc_hwirqs: Bitmap of wakeup capable irqs. >> * @soc; Reference to soc_data of platform specific data. >> * @regs: Base address for the TLMM register map. >> */ >> @@ -68,11 +70,15 @@ struct msm_pinctrl { >> >> DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO); >> DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO); >> + DECLARE_BITMAP(pdc_hwirqs, MAX_PDC_HWIRQ); >> >> const struct msm_pinctrl_soc_data *soc; >> void __iomem *regs; >> + struct irq_domain *pdc_irq_domain; >> }; >> >> +static bool in_suspend; >> + >> static int msm_get_groups_count(struct pinctrl_dev *pctldev) >> { >> struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); >> @@ -787,8 +793,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) >> >> raw_spin_lock_irqsave(&pctrl->lock, flags); >> >> - if (pdc_irqd) >> + if (pdc_irqd && !in_suspend) { >> irq_set_irq_wake(pdc_irqd->irq, on); >> + if (on) >> + set_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs); >> + else >> + clear_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs); >> + } >> >> irq_set_irq_wake(pctrl->irq, on); >> >> @@ -919,7 +930,12 @@ static int msm_gpio_pdc_pin_request(struct irq_data *d) >> } >> >> irq_set_handler_data(d->irq, irq_get_irq_data(irq)); >> + irq_set_handler_data(irq, d); >> + irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY); >> + irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY); >> disable_irq(irq); >> + if (!pctrl->pdc_irq_domain) >> + pctrl->pdc_irq_domain = irq_get_irq_data(irq)->domain; >> >> return 0; >> } >> @@ -1069,6 +1085,58 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) >> } >> } >> >> +int __maybe_unused msm_pinctrl_suspend_late(struct device *dev) >> +{ >> + struct msm_pinctrl *pctrl = dev_get_drvdata(dev); >> + struct irq_data *irqd; >> + unsigned int irq; >> + int i; >> + >> + in_suspend = true; >> + for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) { >> + irq = irq_find_mapping(pctrl->pdc_irq_domain, i); >> + irqd = irq_get_handler_data(irq); >> + /* >> + * We don't know if the TLMM will be functional >> + * or not, during the suspend. If its functional, >> + * we do not want duplicate interrupts from PDC. >> + * Hence disable the GPIO IRQ and enable PDC IRQ. >> + */ >> + if (irqd_is_wakeup_set(irqd)) { >> + disable_irq_wake(irqd->irq); >> + disable_irq(irqd->irq); >> + enable_irq(irq); >> + } > >Would it make sense to limit this to edge triggered interrupts since >the interrupt handler does nothing for level triggered ones? > Sure. -- Lina
On Mon, Aug 27 2018 at 16:35 -0600, Matthias Kaehlcke wrote: >Hi Lina, > >On Fri, Aug 24, 2018 at 02:01:53PM -0600, Lina Iyer wrote: >> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on >> domain can wakeup the SoC, when interrupts and GPIOs are routed to the >> its interrupt controller. Only select GPIOs that are deemed wakeup > >wording nit: "are routed to the|its interrupt controller" > Okay. >> capable are routed to specific PDC pins. During low power state, the >> pinmux interrupt controller may be non-functional but the PDC would be. >> The PDC can detect the wakeup GPIO is triggered and bring the TLMM to an >> operational state. >> >> Interrupts that are level triggered will be detected at the TLMM when >> the controller becomes operational. Edge interrupts however need to be >> replayed again. >> >> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ, >> but keep it disabled. During suspend, we can enable the PDC IRQ instead >> of the GPIO IRQ, which may or not be detected. >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> Changes in v2: >> - Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ >> Changes in v1: >> - Trigger GPIO in h/w from PDC IRQ handler >> - Avoid big tables for GPIO-PDC map, pick from DT instead >> - Use handler_data >> --- >> drivers/pinctrl/qcom/pinctrl-msm.c | 96 ++++++++++++++++++++++++++++++ >> 1 file changed, 96 insertions(+) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> index 0e22f52b2a19..b675ea56a4ff 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> @@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) >> const struct msm_pingroup *g; >> unsigned long flags; >> u32 val; >> + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); >> >> g = &pctrl->soc->groups[d->hwirq]; >> >> raw_spin_lock_irqsave(&pctrl->lock, flags); >> >> + if (pdc_irqd) >> + irq_set_irq_type(pdc_irqd->irq, type); >> + >> /* >> * For hw without possibility of detecting both edges >> */ >> @@ -779,9 +783,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) >> struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >> unsigned long flags; >> + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); >> >> raw_spin_lock_irqsave(&pctrl->lock, flags); >> >> + if (pdc_irqd) >> + irq_set_irq_wake(pdc_irqd->irq, on); >> + >> irq_set_irq_wake(pctrl->irq, on); >> >> raw_spin_unlock_irqrestore(&pctrl->lock, flags); >> @@ -863,6 +871,92 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) >> return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0; >> } >> >> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data) >> +{ >> + struct irq_data *irqd = data; >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd); >> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >> + const struct msm_pingroup *g; >> + unsigned long flags; >> + u32 val; >> + >> + if (!irqd_is_level_type(irqd)) { >> + g = &pctrl->soc->groups[irqd->hwirq]; >> + raw_spin_lock_irqsave(&pctrl->lock, flags); >> + val = BIT(g->intr_status_bit); >> + writel(val, pctrl->regs + g->intr_status_reg); >> + raw_spin_unlock_irqrestore(&pctrl->lock, flags); >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int msm_gpio_pdc_pin_request(struct irq_data *d) >> +{ >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >> + struct platform_device *pdev = to_platform_device(pctrl->dev); >> + const char *pin_name; >> + int irq; >> + int ret; >> + >> + pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq); >> + if (!pin_name) >> + return -ENOMEM; >> + >> + irq = platform_get_irq_byname(pdev, pin_name); >> + if (irq < 0) { >> + kfree(pin_name); >> + return 0; > >Do I understand correctly that this is the case where the pin isn't >routed to the PDC? > Yes, correct. >> + } >> + >> + ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d), >> + pin_name, d); >> + if (ret) { >> + pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq); > >'\n' is missing > ok. >> + kfree(pin_name); >> + return ret; >> + } >> + >> + irq_set_handler_data(d->irq, irq_get_irq_data(irq)); >> + disable_irq(irq); >> + >> + return 0; >> +} >> + >> +static int msm_gpio_pdc_pin_release(struct irq_data *d) >> +{ >> + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); >> + >> + if (pdc_irqd) { >> + irq_set_handler_data(d->irq, NULL); >> + free_irq(pdc_irqd->irq, d); > >You need to free 'pin_name' allocated in msm_gpio_pdc_pin_request(). >IIUC it should be available in irq_desc->action->name. > Yes, I didn't realize that free_irq returns the name when I posted this series (noted in the cover letter). Will fix. -- Lina
Hi, Changes in v2: - Compile and test on 4.18 on SDM845 - Fix IRQ map in patch #3 - Address review comments (I still need to find a way to free memory allocated for PDC IRQ.) - Specify type for IRQ in DT - This series needs V3 of the PDC DT bingings [4] - gic-v3 settings are also needed [5] Changes in v1: - Avoid GPIO-PDC map in .c file - Trigger GPIO by writing to the hardware - Hooked up to suspend/resume callbacks - Dropped PDC DT bindings (see dependencies) This is an attempt at a solution to enable wake up from suspend and deep idle using GPIO as a wakeup source. The 845 uses a new interrupt controller (PDC) that lies in the always-on domain and can sense interrupts that are routed to it, when the GIC is powered off. It would then wakeup the GIC and replay the interrupt which would then be relayed to the AP. The PDC interrupt controller driver is merged upstream [1],[2]. The following set of patches extends the wakeup capability to GPIOs using the PDC. The TLMM pinctrl driver for the SoC available at [3]. The complexity with the solution stems from the fact that only a selected few GPIO lines are routed to the PDC in addition the TLMMs. They are also from different banks on the pinctrl and the TLMM summary line is not routed to the PDC. Hence the PDC cannot be considered as parent of the TLMM irqchip (or can we ?). This is what it looks like - [ PIN ] -----[ TLMM ]---------------> [ GIC ] ---> [ CPU ] | <summary IRQ> ^ | | ----------------------------------> [ PDC ] <wakeup IRQs> I had a brief discussion with Linus on this and the idea implemented is based on his suggestion. When an IRQ (let's call this latent IRQ) for a GPIO is requested, the ->irq_request_resources() is used by the TLMM driver to request a PDC pin. The PDC pin associated with the GPIO is read from a static map available in the pinctrl-sdm845.c. (I think there should be a better location than a static map, more on that later). Knowing the PDC pin from the map, we could look up the DT bindings and request the PDC interrupt with the same trigger mask as the interrupt requested. The ->set_type and ->set_wake are also trapped to set the PDC IRQ's polarity and enable it when the latent IRQ is requested. When the PDC detects the interrupt at suspend, it wakes up the GIC and replays the wakeup IRQ. The GPIO handler function for the latent IRQ is invoked in turn. Please review these patches and your inputs would be greatly appreciated and (kindly) let me know if I have committed any blunders with this approach. There is definitely opportunity to improve the location of the static GPIO-PDC pin map. We could possibly put it as an data argument in the interrupts definition of the PDC or with interrupt names. Also, I am still sorting out some issues with the IRQ handling part of these patches. And I am unsure of how to set the polarity of the PDC pin without locking, since we are not in hierarchy with the PDC interrupt controller. Again, your inputs on these would be greatly helpful. Thanks, Lina [1]. drivers/irqchip/qcom-pdc.c [2]. Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt [3]. drivers/pinctrl/qcom/pinctrl-msm.c [4]. https://lore.kernel.org/patchwork/patch/977589/ [5]. https://lore.kernel.org/patchwork/patch/975425/ Lina Iyer (5): drivers: pinctrl: qcom: add wakeup capability to GPIO dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845 drivers: pinctrl: msm: enable PDC interrupt only during suspend drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend arm64: dts: qcom: add wake up interrupts for GPIOs for SDM845 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 104 ++++++++++- arch/arm64/boot/dts/qcom/sdm845.dtsi | 152 +++++++++++++++- drivers/pinctrl/qcom/pinctrl-msm.c | 164 ++++++++++++++++++ drivers/pinctrl/qcom/pinctrl-msm.h | 3 + drivers/pinctrl/qcom/pinctrl-sdm845.c | 6 + 5 files changed, 425 insertions(+), 4 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project