Hi,
Resending as v1. The series was sent incorrectly as v2.
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)
Dependencies:
https://lkml.org/lkml/2018/8/17/137
https://lkml.org/lkml/2018/8/15/289
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
*** BLURB HERE ***
Lina Iyer (4):
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
Marc Zyngier (1):
irqchip/gic-v3: Allow interrupt to be configured as wake-up sources
.../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 58 +++++++++++++++++-
arch/arm64/boot/dts/qcom/sdm845.dtsi | 57 +++++++++++++++++-
drivers/irqchip/irq-gic-v3.c | 8 ++-
drivers/pinctrl/qcom/pinctrl-msm.c | 60 ++++++++++++++++++-
drivers/pinctrl/qcom/pinctrl-msm.h | 3 +
drivers/pinctrl/qcom/pinctrl-sdm845.c | 6 ++
6 files changed, 185 insertions(+), 7 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
GPIOs that are wakeup capable have interrupt lines that are routed to
the always-on interrupt controller (PDC) in parallel to the pinctrl. The
interrupts listed here are the wake up lines corresponding to GPIOs.
Signed-off-by: Lina Iyer <[email protected]>
---
Changes in v1:
- Use interrupt-extended for all TLMM interrupts
- Define GPIO-PDC map using interrupt-names
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 57 +++++++++++++++++++++++++++-
1 file changed, 56 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 87ffc32dc597..2379684373d3 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -712,11 +712,66 @@
tlmm: pinctrl@3400000 {
compatible = "qcom,sdm845-pinctrl";
reg = <0x03400000 0xc00000>;
- interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
+ interrupts-extended = <&intc GIC_SPI 208 0>,
+ <&pdc 510 0>, <&pdc 511 0>, <&pdc 512 0>,
+ <&pdc 513 0>, <&pdc 514 0>, <&pdc 515 0>,
+ <&pdc 516 0>, <&pdc 517 0>, <&pdc 518 0>,
+ <&pdc 519 0>, <&pdc 632 0>, <&pdc 639 0>,
+ <&pdc 521 0>, <&pdc 522 0>, <&pdc 523 0>,
+ <&pdc 524 0>, <&pdc 525 0>, <&pdc 526 0>,
+ <&pdc 527 0>, <&pdc 630 0>, <&pdc 637 0>,
+ <&pdc 529 0>, <&pdc 530 0>, <&pdc 531 0>,
+ <&pdc 532 0>, <&pdc 633 0>, <&pdc 640 0>,
+ <&pdc 534 0>, <&pdc 535 0>, <&pdc 536 0>,
+ <&pdc 537 0>, <&pdc 538 0>, <&pdc 539 0>,
+ <&pdc 540 0>, <&pdc 541 0>, <&pdc 542 0>,
+ <&pdc 543 0>, <&pdc 544 0>, <&pdc 545 0>,
+ <&pdc 546 0>, <&pdc 547 0>, <&pdc 548 0>,
+ <&pdc 549 0>, <&pdc 550 0>, <&pdc 551 0>,
+ <&pdc 552 0>, <&pdc 553 0>, <&pdc 554 0>,
+ <&pdc 555 0>, <&pdc 556 0>, <&pdc 557 0>,
+ <&pdc 631 0>, <&pdc 638 0>, <&pdc 559 0>,
+ <&pdc 560 0>, <&pdc 561 0>, <&pdc 562 0>,
+ <&pdc 563 0>, <&pdc 564 0>, <&pdc 565 0>,
+ <&pdc 566 0>, <&pdc 570 0>, <&pdc 571 0>,
+ <&pdc 572 0>, <&pdc 573 0>, <&pdc 609 0>,
+ <&pdc 610 0>, <&pdc 611 0>, <&pdc 612 0>,
+ <&pdc 613 0>, <&pdc 614 0>, <&pdc 615 0>,
+ <&pdc 617 0>, <&pdc 618 0>, <&pdc 619 0>,
+ <&pdc 620 0>, <&pdc 621 0>, <&pdc 622 0>,
+ <&pdc 623 0>;
+ interrupt-names = "summary-irq",
+ "gpio1", "gpio3", "gpio5",
+ "gpio10", "gpio11", "gpio20",
+ "gpio22", "gpio24", "gpio26",
+ "gpio30", "gpio31", "gpio31",
+ "gpio32", "gpio34", "gpio36",
+ "gpio37", "gpio38", "gpio39",
+ "gpio40", "gpio41", "gpio41",
+ "gpio43", "gpio44", "gpio46",
+ "gpio48", "gpio49", "gpio49",
+ "gpio52", "gpio53", "gpio54",
+ "gpio56", "gpio57", "gpio58",
+ "gpio59", "gpio60", "gpio61",
+ "gpio62", "gpio63", "gpio64",
+ "gpio66", "gpio68", "gpio71",
+ "gpio73", "gpio77", "gpio78",
+ "gpio79", "gpio80", "gpio84",
+ "gpio85", "gpio86", "gpio88",
+ "gpio89", "gpio89", "gpio91",
+ "gpio92", "gpio95", "gpio96",
+ "gpio97", "gpio101", "gpio103",
+ "gpio104", "gpio115", "gpio116",
+ "gpio117", "gpio118", "gpio119",
+ "gpio120", "gpio121", "gpio122",
+ "gpio123", "gpio124", "gpio125",
+ "gpio127", "gpio128", "gpio129",
+ "gpio130", "gpio132", "gpio133",
+ "gpio145";
qup_i2c0_default: qup-i2c0-default {
pinmux {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Marc Zyngier <[email protected]>
Although GICv3 doesn't directly offers support for wake-up interrupts
and relies on external HW for this, it shouldn't prevent the driver
for such HW from doing it work.
Let's set the required flags on the irq_chip structures.
Reported-by: Lina Iyer <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 76ea56d779a1..2d71c79bc698 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -861,7 +861,9 @@ static struct irq_chip gic_chip = {
.irq_set_affinity = gic_set_affinity,
.irq_get_irqchip_state = gic_irq_get_irqchip_state,
.irq_set_irqchip_state = gic_irq_set_irqchip_state,
- .flags = IRQCHIP_SET_TYPE_MASKED,
+ .flags = IRQCHIP_SET_TYPE_MASKED |
+ IRQCHIP_SKIP_SET_WAKE |
+ IRQCHIP_MASK_ON_SUSPEND,
};
static struct irq_chip gic_eoimode1_chip = {
@@ -874,7 +876,9 @@ static struct irq_chip gic_eoimode1_chip = {
.irq_get_irqchip_state = gic_irq_get_irqchip_state,
.irq_set_irqchip_state = gic_irq_set_irqchip_state,
.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity,
- .flags = IRQCHIP_SET_TYPE_MASKED,
+ .flags = IRQCHIP_SET_TYPE_MASKED |
+ IRQCHIP_SKIP_SET_WAKE |
+ IRQCHIP_MASK_ON_SUSPEND,
};
#define GIC_ID_NR (1U << gic_data.rdists.id_bits)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
---
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)
+};
+
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,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 60 +++++++++++++++++++++++++++++-
drivers/pinctrl/qcom/pinctrl-msm.h | 3 ++
2 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 03ef1d29d078..17e541f8f09d 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_IRQ 1024
#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_irqs: 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,14 @@ struct msm_pinctrl {
DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+ DECLARE_BITMAP(pdc_irqs, MAX_PDC_IRQ);
const struct msm_pinctrl_soc_data *soc;
void __iomem *regs;
};
+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 +792,11 @@ 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);
+ on ? set_bit(pdc_irqd->irq, pctrl->pdc_irqs) :
+ clear_bit(pdc_irqd->irq, pctrl->pdc_irqs);
+ }
irq_set_irq_wake(pctrl->irq, on);
@@ -920,6 +928,8 @@ 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);
disable_irq(irq);
return 0;
@@ -1070,6 +1080,54 @@ 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;
+ int i;
+
+ in_suspend = true;
+ for_each_set_bit(i, pctrl->pdc_irqs, MAX_PDC_IRQ) {
+ irqd = irq_get_handler_data(i);
+ /*
+ * 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)) {
+ irq_set_irq_wake(irqd->irq, false);
+ disable_irq(irqd->irq);
+ enable_irq(i);
+ }
+ }
+
+ return 0;
+}
+
+int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
+{
+ struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+ struct irq_data *irqd;
+ int i;
+
+ for_each_set_bit(i, pctrl->pdc_irqs, MAX_PDC_IRQ) {
+ irqd = irq_get_handler_data(i);
+ /*
+ * The TLMM will be operational now, so disable
+ * the PDC IRQ.
+ */
+ if (irqd_is_wakeup_set(irq_get_irq_data(i))) {
+ disable_irq_nosync(i);
+ irq_set_irq_wake(irqd->irq, true);
+ enable_irq(irqd->irq);
+ }
+ }
+ in_suspend = false;
+
+ return 0;
+}
+
int msm_pinctrl_probe(struct platform_device *pdev,
const struct msm_pinctrl_soc_data *soc_data)
{
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 9b9feea540ff..21b56fb5dae9 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -123,4 +123,7 @@ int msm_pinctrl_probe(struct platform_device *pdev,
const struct msm_pinctrl_soc_data *soc_data);
int msm_pinctrl_remove(struct platform_device *pdev);
+int msm_pinctrl_suspend_late(struct device *dev);
+int msm_pinctrl_resume_late(struct device *dev);
+
#endif
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Update the documentation to use interrupts-extended format for
specifying the TLMM summary IRQ line that is requested from GIC and the
PDC interrupts corresponding to the wakeup capable GPIOs.
Update the example to show PDC interrupts for the wakeup capable GPIOs
for SDM845.
Cc: [email protected]
Signed-off-by: Lina Iyer <[email protected]>
---
.../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 58 ++++++++++++++++++-
1 file changed, 55 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
index 665aadb5ea28..d7408cc74e01 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
@@ -13,10 +13,21 @@ SDM845 platform.
Value type: <prop-encoded-array>
Definition: the base address and size of the TLMM register space.
-- interrupts:
+- interrupts-extended:
Usage: required
Value type: <prop-encoded-array>
- Definition: should specify the TLMM summary IRQ.
+ Definition: should specify the TLMM summary IRQ as the first
+ interrupt. Optionally, wake up capable GPIOs may list
+ their corresponding PDC interrupts here.
+
+- interrupt-names:
+ Usage: required
+ Value type: <string>
+ Definition: the names matching the interrupt definition in the
+ interrupts-extended property. The first interrupt name
+ must be "summary-irq" for the TLMM summary IRQ. PDC
+ interrupts must be described by "gpioN", where N is the
+ GPIO line corresponding to the PDC IRQ.
- interrupt-controller:
Usage: required
@@ -155,11 +166,52 @@ Example:
tlmm: pinctrl@3400000 {
compatible = "qcom,sdm845-pinctrl";
reg = <0x03400000 0xc00000>;
- interrupts = <GIC_SPI 208 0>;
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
+ interrupts-extended = <&intc GIC_SPI 208 0>,
+ <&pdc 510 0>, <&pdc 511 0>, <&pdc 512 0>, <&pdc 513 0>,
+ <&pdc 514 0>, <&pdc 515 0>, <&pdc 516 0>, <&pdc 517 0>,
+ <&pdc 518 0>, <&pdc 519 0>, <&pdc 632 0>, <&pdc 639 0>,
+ <&pdc 521 0>, <&pdc 522 0>, <&pdc 523 0>, <&pdc 524 0>,
+ <&pdc 525 0>, <&pdc 526 0>, <&pdc 527 0>, <&pdc 630 0>,
+ <&pdc 637 0>, <&pdc 529 0>, <&pdc 530 0>, <&pdc 531 0>,
+ <&pdc 532 0>, <&pdc 633 0>, <&pdc 640 0>, <&pdc 534 0>,
+ <&pdc 535 0>, <&pdc 536 0>, <&pdc 537 0>, <&pdc 538 0>,
+ <&pdc 539 0>, <&pdc 540 0>, <&pdc 541 0>, <&pdc 542 0>,
+ <&pdc 543 0>, <&pdc 544 0>, <&pdc 545 0>, <&pdc 546 0>,
+ <&pdc 547 0>, <&pdc 548 0>, <&pdc 549 0>, <&pdc 550 0>,
+ <&pdc 551 0>, <&pdc 552 0>, <&pdc 553 0>, <&pdc 554 0>,
+ <&pdc 555 0>, <&pdc 556 0>, <&pdc 557 0>, <&pdc 631 0>,
+ <&pdc 638 0>, <&pdc 559 0>, <&pdc 560 0>, <&pdc 561 0>,
+ <&pdc 562 0>, <&pdc 563 0>, <&pdc 564 0>, <&pdc 565 0>,
+ <&pdc 566 0>, <&pdc 570 0>, <&pdc 571 0>, <&pdc 572 0>,
+ <&pdc 573 0>, <&pdc 609 0>, <&pdc 610 0>, <&pdc 611 0>,
+ <&pdc 612 0>, <&pdc 613 0>, <&pdc 614 0>, <&pdc 615 0>,
+ <&pdc 617 0>, <&pdc 618 0>, <&pdc 619 0>, <&pdc 620 0>,
+ <&pdc 621 0>, <&pdc 622 0>, <&pdc 623 0>;
+ interrupt-names = "summary-irq",
+ "gpio1", "gpio3", "gpio5", "gpio10",
+ "gpio11", "gpio20", "gpio22", "gpio24",
+ "gpio26", "gpio30", "gpio31", "gpio31",
+ "gpio32", "gpio34", "gpio36", "gpio37",
+ "gpio38", "gpio39", "gpio40", "gpio41",
+ "gpio41", "gpio43", "gpio44", "gpio46",
+ "gpio48", "gpio49", "gpio49", "gpio52",
+ "gpio53", "gpio54", "gpio56", "gpio57",
+ "gpio58", "gpio59", "gpio60", "gpio61",
+ "gpio62", "gpio63", "gpio64", "gpio66",
+ "gpio68", "gpio71", "gpio73", "gpio77",
+ "gpio78", "gpio79", "gpio80", "gpio84",
+ "gpio85", "gpio86", "gpio88", "gpio89",
+ "gpio89", "gpio91", "gpio92", "gpio95",
+ "gpio96", "gpio97", "gpio101", "gpio103",
+ "gpio104", "gpio115", "gpio116", "gpio117",
+ "gpio118", "gpio119", "gpio120", "gpio121",
+ "gpio122", "gpio123", "gpio124", "gpio125",
+ "gpio127", "gpio128", "gpio129", "gpio130",
+ "gpio132", "gpio133", "gpio145";
qup9_active: qup9-active {
mux {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Hi Lina,
On Fri, 17 Aug 2018 20:10:23 +0100,
Lina Iyer <[email protected]> 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 <[email protected]>
> ---
> drivers/pinctrl/qcom/pinctrl-msm.c | 60 +++++++++++++++++++++++++++++-
> drivers/pinctrl/qcom/pinctrl-msm.h | 3 ++
> 2 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 03ef1d29d078..17e541f8f09d 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_IRQ 1024
Where is this value coming from? Is it guaranteed to be an
architectural maximum? Or something that is likely to vary in future
implementations?
> #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_irqs: 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,14 @@ struct msm_pinctrl {
>
> DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
> DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
> + DECLARE_BITMAP(pdc_irqs, MAX_PDC_IRQ);
>
> const struct msm_pinctrl_soc_data *soc;
> void __iomem *regs;
> };
>
> +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 +792,11 @@ 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);
> + on ? set_bit(pdc_irqd->irq, pctrl->pdc_irqs) :
> + clear_bit(pdc_irqd->irq, pctrl->pdc_irqs);
I think we'll all survive the two extra lines if you write this as an
'if' statement (unless you're competing for the next IOCCC, and then
you need to up your game a bit).
Also, are you indexing the bitmap using a Linux irq number? If so,
that's an absolute NACK. Out of the box, a Linux IRQ can be up to
NR_IRQS+8196 on arm64, and there are plans to push that to be a much
larger space.
> + }
>
> irq_set_irq_wake(pctrl->irq, on);
I'm a bit worried by the way you call into the irq subsystem with this
spinlock held. Have you run that code with lockdep enabled?
>
> @@ -920,6 +928,8 @@ 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);
Could you explain what this is trying to do? I'm trying to understand
this code, but this function isn't in 4.18...
> disable_irq(irq);
>
> return 0;
> @@ -1070,6 +1080,54 @@ 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;
> + int i;
> +
> + in_suspend = true;
> + for_each_set_bit(i, pctrl->pdc_irqs, MAX_PDC_IRQ) {
> + irqd = irq_get_handler_data(i);
So this is what I though. You're using the Linux IRQ, and not the pin
number (or whatever HW-dependent index that would otherwise make
sense). Please fix it.
> + /*
> + * 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)) {
> + irq_set_irq_wake(irqd->irq, false);
> + disable_irq(irqd->irq);
> + enable_irq(i);
> + }
> + }
> +
> + return 0;
> +}
> +
> +int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
> +{
> + struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
> + struct irq_data *irqd;
> + int i;
> +
> + for_each_set_bit(i, pctrl->pdc_irqs, MAX_PDC_IRQ) {
> + irqd = irq_get_handler_data(i);
> + /*
> + * The TLMM will be operational now, so disable
> + * the PDC IRQ.
> + */
> + if (irqd_is_wakeup_set(irq_get_irq_data(i))) {
> + disable_irq_nosync(i);
> + irq_set_irq_wake(irqd->irq, true);
> + enable_irq(irqd->irq);
> + }
> + }
> + in_suspend = false;
> +
> + return 0;
> +}
> +
> int msm_pinctrl_probe(struct platform_device *pdev,
> const struct msm_pinctrl_soc_data *soc_data)
> {
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 9b9feea540ff..21b56fb5dae9 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -123,4 +123,7 @@ int msm_pinctrl_probe(struct platform_device *pdev,
> const struct msm_pinctrl_soc_data *soc_data);
> int msm_pinctrl_remove(struct platform_device *pdev);
>
> +int msm_pinctrl_suspend_late(struct device *dev);
> +int msm_pinctrl_resume_late(struct device *dev);
> +
> #endif
I can't really review this code any further, as it seems that I'm
missing some crucial dependencies. But there is a number of things
that feel quite wrong in this code, and that need to be addressed
anyway.
Thanks,
M.
--
Jazz is not dead, it just smell funny.
On 20/08/18 16:26, Lina Iyer wrote:
> On Sat, Aug 18 2018 at 07:13 -0600, Marc Zyngier wrote:
>> Hi Lina,
>>
>> On Fri, 17 Aug 2018 20:10:23 +0100,
>> Lina Iyer <[email protected]> wrote:
[...]
>>> @@ -920,6 +928,8 @@ 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);
>>
>> Could you explain what this is trying to do? I'm trying to understand
>> this code, but this function isn't in 4.18...
>>
> Oh, I have been able to test only on 4.14 so far. The flag does seem to
> exist at least, I didn't get a compiler error.
>
> I read this in kernel/irq/chip.c -
>
> If the interrupt chip does not implement the irq_disable callback,
> a driver can disable the lazy approach for a particular irq line by
> calling 'irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)'. This can
> be used for devices which cannot disable the interrupt at the
> device level under certain circumstances and have to use
> disable_irq[_nosync] instead.
>
> And interpreted this as something that this would prevent 'relaxed'
> disable. I am enabling and disabling the IRQ in suspend path, that I
> thought this would help avoid issues caused by late disable. Am I
> mistaken?
Sorry, I wasn't clear enough. I'm talking about what you're trying to do
in this particular function (msm_gpio_pdc_pin_request), which doesn't
exist in 4.18. Short of having a bit of context, I can hardly review this.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Sat, Aug 18 2018 at 07:13 -0600, Marc Zyngier wrote:
>Hi Lina,
>
>On Fri, 17 Aug 2018 20:10:23 +0100,
>Lina Iyer <[email protected]> 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 <[email protected]>
>> ---
>> drivers/pinctrl/qcom/pinctrl-msm.c | 60 +++++++++++++++++++++++++++++-
>> drivers/pinctrl/qcom/pinctrl-msm.h | 3 ++
>> 2 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 03ef1d29d078..17e541f8f09d 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_IRQ 1024
>
>Where is this value coming from? Is it guaranteed to be an
>architectural maximum? Or something that is likely to vary in future
>implementations?
>
>> #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_irqs: 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,14 @@ struct msm_pinctrl {
>>
>> DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
>> DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
>> + DECLARE_BITMAP(pdc_irqs, MAX_PDC_IRQ);
>>
>> const struct msm_pinctrl_soc_data *soc;
>> void __iomem *regs;
>> };
>>
>> +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 +792,11 @@ 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);
>> + on ? set_bit(pdc_irqd->irq, pctrl->pdc_irqs) :
>> + clear_bit(pdc_irqd->irq, pctrl->pdc_irqs);
>
>I think we'll all survive the two extra lines if you write this as an
>'if' statement (unless you're competing for the next IOCCC, and then
>you need to up your game a bit).
>
>Also, are you indexing the bitmap using a Linux irq number? If so,
>that's an absolute NACK. Out of the box, a Linux IRQ can be up to
>NR_IRQS+8196 on arm64, and there are plans to push that to be a much
>larger space.
>
I didn't realize this. I have been using linux IRQ number on this
bitmask and I will need to fix this.
>> + }
>>
>> irq_set_irq_wake(pctrl->irq, on);
>
>I'm a bit worried by the way you call into the irq subsystem with this
>spinlock held. Have you run that code with lockdep enabled?
>
I have not tried lockdep. Will try it.
This specific line is already part of the driver. I added a similar line
irq_set_irq_wake(pdc_irqd->irq) just above following the same pattern.
>>
>> @@ -920,6 +928,8 @@ 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);
>
>Could you explain what this is trying to do? I'm trying to understand
>this code, but this function isn't in 4.18...
>
Oh, I have been able to test only on 4.14 so far. The flag does seem to
exist at least, I didn't get a compiler error.
I read this in kernel/irq/chip.c -
If the interrupt chip does not implement the irq_disable callback,
a driver can disable the lazy approach for a particular irq line by
calling 'irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)'. This can
be used for devices which cannot disable the interrupt at the
device level under certain circumstances and have to use
disable_irq[_nosync] instead.
And interpreted this as something that this would prevent 'relaxed'
disable. I am enabling and disabling the IRQ in suspend path, that I
thought this would help avoid issues caused by late disable. Am I
mistaken?
>> disable_irq(irq);
>>
>> return 0;
>> @@ -1070,6 +1080,54 @@ 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;
>> + int i;
>> +
>> + in_suspend = true;
>> + for_each_set_bit(i, pctrl->pdc_irqs, MAX_PDC_IRQ) {
>> + irqd = irq_get_handler_data(i);
>
>So this is what I though. You're using the Linux IRQ, and not the pin
>number (or whatever HW-dependent index that would otherwise make
>sense). Please fix it.
>
Noted.
>> + /*
>> + * 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)) {
>> + irq_set_irq_wake(irqd->irq, false);
>> + disable_irq(irqd->irq);
>> + enable_irq(i);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
>> +{
>> + struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
>> + struct irq_data *irqd;
>> + int i;
>> +
>> + for_each_set_bit(i, pctrl->pdc_irqs, MAX_PDC_IRQ) {
>> + irqd = irq_get_handler_data(i);
>> + /*
>> + * The TLMM will be operational now, so disable
>> + * the PDC IRQ.
>> + */
>> + if (irqd_is_wakeup_set(irq_get_irq_data(i))) {
>> + disable_irq_nosync(i);
>> + irq_set_irq_wake(irqd->irq, true);
>> + enable_irq(irqd->irq);
>> + }
>> + }
>> + in_suspend = false;
>> +
>> + return 0;
>> +}
>> +
>> int msm_pinctrl_probe(struct platform_device *pdev,
>> const struct msm_pinctrl_soc_data *soc_data)
>> {
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
>> index 9b9feea540ff..21b56fb5dae9 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
>> @@ -123,4 +123,7 @@ int msm_pinctrl_probe(struct platform_device *pdev,
>> const struct msm_pinctrl_soc_data *soc_data);
>> int msm_pinctrl_remove(struct platform_device *pdev);
>>
>> +int msm_pinctrl_suspend_late(struct device *dev);
>> +int msm_pinctrl_resume_late(struct device *dev);
>> +
>> #endif
>
>I can't really review this code any further, as it seems that I'm
>missing some crucial dependencies. But there is a number of things
>that feel quite wrong in this code, and that need to be addressed
>anyway.
>
Thanks for reviewing Marc.
-- Lina
On Mon, Aug 20 2018 at 09:34 -0600, Marc Zyngier wrote:
>On 20/08/18 16:26, Lina Iyer wrote:
>> On Sat, Aug 18 2018 at 07:13 -0600, Marc Zyngier wrote:
>>> Hi Lina,
>>>
>>> On Fri, 17 Aug 2018 20:10:23 +0100,
>>> Lina Iyer <[email protected]> wrote:
>
>[...]
>
>>>> @@ -920,6 +928,8 @@ 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);
>>>
>>> Could you explain what this is trying to do? I'm trying to understand
>>> this code, but this function isn't in 4.18...
>>>
>> Oh, I have been able to test only on 4.14 so far. The flag does seem to
>> exist at least, I didn't get a compiler error.
>>
>> I read this in kernel/irq/chip.c -
>>
>> If the interrupt chip does not implement the irq_disable callback,
>> a driver can disable the lazy approach for a particular irq line by
>> calling 'irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)'. This can
>> be used for devices which cannot disable the interrupt at the
>> device level under certain circumstances and have to use
>> disable_irq[_nosync] instead.
>>
>> And interpreted this as something that this would prevent 'relaxed'
>> disable. I am enabling and disabling the IRQ in suspend path, that I
>> thought this would help avoid issues caused by late disable. Am I
>> mistaken?
>
>Sorry, I wasn't clear enough. I'm talking about what you're trying to do
>in this particular function (msm_gpio_pdc_pin_request), which doesn't
>exist in 4.18. Short of having a bit of context, I can hardly review this.
>
Sorry, my patch generation during the resend is messed up. Seems like I
didn't send that patch out during the resend.
-- Lina
On Fri, 17 Aug 2018 13:10:22 -0600, Lina Iyer wrote:
> Update the documentation to use interrupts-extended format for
> specifying the TLMM summary IRQ line that is requested from GIC and the
> PDC interrupts corresponding to the wakeup capable GPIOs.
>
> Update the example to show PDC interrupts for the wakeup capable GPIOs
> for SDM845.
>
> Cc: [email protected]
> Signed-off-by: Lina Iyer <[email protected]>
> ---
> .../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 58 ++++++++++++++++++-
> 1 file changed, 55 insertions(+), 3 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>
On Fri 17 Aug 12:10 PDT 2018, Lina Iyer wrote:
> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
[..]
> @@ -155,11 +166,52 @@ Example:
> tlmm: pinctrl@3400000 {
> compatible = "qcom,sdm845-pinctrl";
> reg = <0x03400000 0xc00000>;
> - interrupts = <GIC_SPI 208 0>;
> gpio-controller;
> #gpio-cells = <2>;
> interrupt-controller;
> #interrupt-cells = <2>;
> + interrupts-extended = <&intc GIC_SPI 208 0>,
> + <&pdc 510 0>, <&pdc 511 0>, <&pdc 512 0>, <&pdc 513 0>,
> + <&pdc 514 0>, <&pdc 515 0>, <&pdc 516 0>, <&pdc 517 0>,
> + <&pdc 518 0>, <&pdc 519 0>, <&pdc 632 0>, <&pdc 639 0>,
> + <&pdc 521 0>, <&pdc 522 0>, <&pdc 523 0>, <&pdc 524 0>,
> + <&pdc 525 0>, <&pdc 526 0>, <&pdc 527 0>, <&pdc 630 0>,
> + <&pdc 637 0>, <&pdc 529 0>, <&pdc 530 0>, <&pdc 531 0>,
> + <&pdc 532 0>, <&pdc 633 0>, <&pdc 640 0>, <&pdc 534 0>,
> + <&pdc 535 0>, <&pdc 536 0>, <&pdc 537 0>, <&pdc 538 0>,
> + <&pdc 539 0>, <&pdc 540 0>, <&pdc 541 0>, <&pdc 542 0>,
> + <&pdc 543 0>, <&pdc 544 0>, <&pdc 545 0>, <&pdc 546 0>,
> + <&pdc 547 0>, <&pdc 548 0>, <&pdc 549 0>, <&pdc 550 0>,
> + <&pdc 551 0>, <&pdc 552 0>, <&pdc 553 0>, <&pdc 554 0>,
> + <&pdc 555 0>, <&pdc 556 0>, <&pdc 557 0>, <&pdc 631 0>,
> + <&pdc 638 0>, <&pdc 559 0>, <&pdc 560 0>, <&pdc 561 0>,
> + <&pdc 562 0>, <&pdc 563 0>, <&pdc 564 0>, <&pdc 565 0>,
> + <&pdc 566 0>, <&pdc 570 0>, <&pdc 571 0>, <&pdc 572 0>,
> + <&pdc 573 0>, <&pdc 609 0>, <&pdc 610 0>, <&pdc 611 0>,
> + <&pdc 612 0>, <&pdc 613 0>, <&pdc 614 0>, <&pdc 615 0>,
> + <&pdc 617 0>, <&pdc 618 0>, <&pdc 619 0>, <&pdc 620 0>,
> + <&pdc 621 0>, <&pdc 622 0>, <&pdc 623 0>;
I would expect that there are about 80 WARN_ON() hit in the irq code
when you boot mainline with this. Have you tried that?
Looks reasonable otherwise.
Regards,
Bjorn
On Mon, 20 Aug 2018 09:49:59 -0600
Lina Iyer <[email protected]> wrote:
> On Mon, Aug 20 2018 at 09:34 -0600, Marc Zyngier wrote:
> >On 20/08/18 16:26, Lina Iyer wrote:
> >> On Sat, Aug 18 2018 at 07:13 -0600, Marc Zyngier wrote:
> >>> Hi Lina,
> >>>
> >>> On Fri, 17 Aug 2018 20:10:23 +0100,
> >>> Lina Iyer <[email protected]> wrote:
> >
> >[...]
> >
> >>>> @@ -920,6 +928,8 @@ 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);
> >>>
> >>> Could you explain what this is trying to do? I'm trying to understand
> >>> this code, but this function isn't in 4.18...
> >>>
> >> Oh, I have been able to test only on 4.14 so far. The flag does seem to
> >> exist at least, I didn't get a compiler error.
> >>
> >> I read this in kernel/irq/chip.c -
> >>
> >> If the interrupt chip does not implement the irq_disable callback,
> >> a driver can disable the lazy approach for a particular irq line by
> >> calling 'irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)'. This can
> >> be used for devices which cannot disable the interrupt at the
> >> device level under certain circumstances and have to use
> >> disable_irq[_nosync] instead.
> >>
> >> And interpreted this as something that this would prevent 'relaxed'
> >> disable. I am enabling and disabling the IRQ in suspend path, that I
> >> thought this would help avoid issues caused by late disable. Am I
> >> mistaken?
> >
> >Sorry, I wasn't clear enough. I'm talking about what you're trying to do
> >in this particular function (msm_gpio_pdc_pin_request), which doesn't
> >exist in 4.18. Short of having a bit of context, I can hardly review this.
> >
> Sorry, my patch generation during the resend is messed up. Seems like I
> didn't send that patch out during the resend.
Please make sure you test with mainline. Basing your developments on
something as old as 4.14 makes no sense for something that targets
mainline. You should write your code on mainline, test it there, and
eventually backport it to whatever version you want to use.
Otherwise, we are guaranteed to merge something that will not work.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On 21/08/18 00:19, Bjorn Andersson wrote:
> On Fri 17 Aug 12:10 PDT 2018, Lina Iyer wrote:
>> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> [..]
>> @@ -155,11 +166,52 @@ Example:
>> tlmm: pinctrl@3400000 {
>> compatible = "qcom,sdm845-pinctrl";
>> reg = <0x03400000 0xc00000>;
>> - interrupts = <GIC_SPI 208 0>;
>> gpio-controller;
>> #gpio-cells = <2>;
>> interrupt-controller;
>> #interrupt-cells = <2>;
>> + interrupts-extended = <&intc GIC_SPI 208 0>,
>> + <&pdc 510 0>, <&pdc 511 0>, <&pdc 512 0>, <&pdc 513 0>,
>> + <&pdc 514 0>, <&pdc 515 0>, <&pdc 516 0>, <&pdc 517 0>,
>> + <&pdc 518 0>, <&pdc 519 0>, <&pdc 632 0>, <&pdc 639 0>,
>> + <&pdc 521 0>, <&pdc 522 0>, <&pdc 523 0>, <&pdc 524 0>,
>> + <&pdc 525 0>, <&pdc 526 0>, <&pdc 527 0>, <&pdc 630 0>,
>> + <&pdc 637 0>, <&pdc 529 0>, <&pdc 530 0>, <&pdc 531 0>,
>> + <&pdc 532 0>, <&pdc 633 0>, <&pdc 640 0>, <&pdc 534 0>,
>> + <&pdc 535 0>, <&pdc 536 0>, <&pdc 537 0>, <&pdc 538 0>,
>> + <&pdc 539 0>, <&pdc 540 0>, <&pdc 541 0>, <&pdc 542 0>,
>> + <&pdc 543 0>, <&pdc 544 0>, <&pdc 545 0>, <&pdc 546 0>,
>> + <&pdc 547 0>, <&pdc 548 0>, <&pdc 549 0>, <&pdc 550 0>,
>> + <&pdc 551 0>, <&pdc 552 0>, <&pdc 553 0>, <&pdc 554 0>,
>> + <&pdc 555 0>, <&pdc 556 0>, <&pdc 557 0>, <&pdc 631 0>,
>> + <&pdc 638 0>, <&pdc 559 0>, <&pdc 560 0>, <&pdc 561 0>,
>> + <&pdc 562 0>, <&pdc 563 0>, <&pdc 564 0>, <&pdc 565 0>,
>> + <&pdc 566 0>, <&pdc 570 0>, <&pdc 571 0>, <&pdc 572 0>,
>> + <&pdc 573 0>, <&pdc 609 0>, <&pdc 610 0>, <&pdc 611 0>,
>> + <&pdc 612 0>, <&pdc 613 0>, <&pdc 614 0>, <&pdc 615 0>,
>> + <&pdc 617 0>, <&pdc 618 0>, <&pdc 619 0>, <&pdc 620 0>,
>> + <&pdc 621 0>, <&pdc 622 0>, <&pdc 623 0>;
>
> I would expect that there are about 80 WARN_ON() hit in the irq code
> when you boot mainline with this. Have you tried that?
Dunno about the ones pointing to the PDC, but the one pointing to the
GIC is definitely a howler.
Another reason for this code to be developed and tested with mainline.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Quoting Lina Iyer (2018-08-17 12:10:23)
> 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.
What about idle paths? Don't we want to disable the TLMM interrupt and
enable the PDC interrupt when the whole cluster goes idle so we get
wakeup interrupts? It's really unfortunate that the hardware can't
replay the interrupt from PDC to TLMM when it knows TLMM didn't get the
interrupt (because the whole chip was off) or the GIC didn't get the
summary irq (because the GIC was powered off). A little more hardware
effort would make this completely transparent to software and make TLMM
work across all low power modes.
Because of this complicated dance, it may make sense to always get the
interrupt at the PDC and then replay it into the TLMM chip "manually"
with the irq_set_irqchip_state() APIs. This way the duplicate interrupt
can't happen. The only way for the interrupt handler to run would be by
PDC poking the TLMM hardware to inject the irq into the status register.
I think with the TLMM that's possible if we configure the pin to have
the raw status bit disabled (so that edges on the physical line don't
latch into the GPIO interrupt status register) and the normal status bit
enabled (so that if the status register changes we'll interrupt the
CPU). It needs some testing to make sure that actually works though. If
it does work, then we have a way to inject interrupts on TLMM without
worry that the TLMM hardware will also see the interrupt.
Is there a good way to test an interrupt to see if it's edge or level
type configured? And is it really a problem to make PDC the hierarchical
parent of TLMM here so that PDC can intercept the type and wake state of
the GPIO irq? Plus there's the part where a GIC SPI interrupt runs for
some GPIO irq, and that needs to be decoded to figure out which GPIO it
is for and if it should be replayed or not. Maybe all types of GPIO irqs
can be replayed and if it's a level type interrupt we waste some time
handling the PDC interrupt just to do nothing besides forward what would
presumably already work without PDC intervention.
On Fri, Aug 24 2018 at 02:22 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-08-17 12:10:23)
>> 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.
>
>What about idle paths? Don't we want to disable the TLMM interrupt and
>enable the PDC interrupt when the whole cluster goes idle so we get
>wakeup interrupts?
We would need to do this from the idle paths. When we have that support
(a patch for cluster power down is in the works), we would need to hook
up to TLMM and do the same.
>It's really unfortunate that the hardware can't
>replay the interrupt from PDC to TLMM when it knows TLMM didn't get the
>interrupt (because the whole chip was off) or the GIC didn't get the
>summary irq (because the GIC was powered off). A little more hardware
>effort would make this completely transparent to software and make TLMM
>work across all low power modes.
>
I wouln't pretend to understand what it entails in the hardware. But, I
believe the complication stems from the fact that PDC is sensing the raw
GPIO just as TLMM when active and sensing itself. To know when to replay
only the interrupt lines for the TLMM (knowing the TLMM was powered off)
might be a lot of hardware.
>Because of this complicated dance, it may make sense to always get the
>interrupt at the PDC and then replay it into the TLMM chip "manually"
>with the irq_set_irqchip_state() APIs. This way the duplicate interrupt
>can't happen. The only way for the interrupt handler to run would be by
>PDC poking the TLMM hardware to inject the irq into the status register.
If the PDC interrupt was always enabled and the interrupt at TLMM was
always disabled, all we would need to set the action handler of the PDC
interrupt to that of the TLMM. I couldn't find a way to retrieve that
nicely.
>I think with the TLMM that's possible if we configure the pin to have
>the raw status bit disabled (so that edges on the physical line don't
>latch into the GPIO interrupt status register) and the normal status bit
>enabled (so that if the status register changes we'll interrupt the
>CPU). It needs some testing to make sure that actually works though. If
>it does work, then we have a way to inject interrupts on TLMM without
>worry that the TLMM hardware will also see the interrupt.
>
>Is there a good way to test an interrupt to see if it's edge or level
>type configured? And is it really a problem to make PDC the hierarchical
>parent of TLMM here so that PDC can intercept the type and wake state of
>the GPIO irq?
Alternately, could we just return the PDC interrupt in gpio_to_irq() and
let the driver manipulate only the PDC interrupt ? Ofcourse, drivers
that request the GPIO as interrupt in DT, would now have to request the
PDC interrupt directly. That could avoid the dance during every
idle/suspend. I am not sure how nice it is do this, would like to know
your thoughts.
>Plus there's the part where a GIC SPI interrupt runs for
>some GPIO irq, and that needs to be decoded to figure out which GPIO it
>is for and if it should be replayed or not. Maybe all types of GPIO irqs
>can be replayed and if it's a level type interrupt we waste some time
>handling the PDC interrupt just to do nothing besides forward what would
>presumably already work without PDC intervention.
>
Quoting Lina Iyer (2018-08-24 10:14:32)
> On Fri, Aug 24 2018 at 02:22 -0600, Stephen Boyd wrote:
> >Quoting Lina Iyer (2018-08-17 12:10:23)
> >> 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.
> >
> >What about idle paths? Don't we want to disable the TLMM interrupt and
> >enable the PDC interrupt when the whole cluster goes idle so we get
> >wakeup interrupts?
> We would need to do this from the idle paths. When we have that support
> (a patch for cluster power down is in the works), we would need to hook
> up to TLMM and do the same.
Ok so then this approach doesn't really seem to work for the CPU idle
paths.
> >Because of this complicated dance, it may make sense to always get the
> >interrupt at the PDC and then replay it into the TLMM chip "manually"
> >with the irq_set_irqchip_state() APIs. This way the duplicate interrupt
> >can't happen. The only way for the interrupt handler to run would be by
> >PDC poking the TLMM hardware to inject the irq into the status register.
> If the PDC interrupt was always enabled and the interrupt at TLMM was
> always disabled, all we would need to set the action handler of the PDC
> interrupt to that of the TLMM. I couldn't find a way to retrieve that
> nicely.
Can't we just configure a different chained IRQ handler with
irq_set_chained_handler_and_data() for each of the GPIO IRQs that are
handled by PDC to be the interrupts provide by the PDC irq controller
that match the GPIOs? And then set their parent irq with
irq_set_parent() for completeness? And also move those GPIOs from the
existing msm_gpio irqchip to a different PDC gpio irqchip that does
nothing besides push irqchip calls up to the PDC irqchip? Then we don't
even have to think about resending anything and we can rely on PDC to do
all the interrupt sensing all the time but still provide the irqs from
the GPIO controller.
>
> >I think with the TLMM that's possible if we configure the pin to have
> >the raw status bit disabled (so that edges on the physical line don't
> >latch into the GPIO interrupt status register) and the normal status bit
> >enabled (so that if the status register changes we'll interrupt the
> >CPU). It needs some testing to make sure that actually works though. If
> >it does work, then we have a way to inject interrupts on TLMM without
> >worry that the TLMM hardware will also see the interrupt.
>
> >
> >Is there a good way to test an interrupt to see if it's edge or level
> >type configured? And is it really a problem to make PDC the hierarchical
> >parent of TLMM here so that PDC can intercept the type and wake state of
> >the GPIO irq?
> Alternately, could we just return the PDC interrupt in gpio_to_irq() and
> let the driver manipulate only the PDC interrupt ? Ofcourse, drivers
> that request the GPIO as interrupt in DT, would now have to request the
> PDC interrupt directly. That could avoid the dance during every
> idle/suspend. I am not sure how nice it is do this, would like to know
> your thoughts.
>
I hope it doesn't come to that.
On Mon, Aug 27 2018 at 14:01 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-08-24 10:14:32)
>> On Fri, Aug 24 2018 at 02:22 -0600, Stephen Boyd wrote:
>> >Quoting Lina Iyer (2018-08-17 12:10:23)
>> >> 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.
>> >
>> >What about idle paths? Don't we want to disable the TLMM interrupt and
>> >enable the PDC interrupt when the whole cluster goes idle so we get
>> >wakeup interrupts?
>> We would need to do this from the idle paths. When we have that support
>> (a patch for cluster power down is in the works), we would need to hook
>> up to TLMM and do the same.
>
>Ok so then this approach doesn't really seem to work for the CPU idle
>paths.
>
>> >Because of this complicated dance, it may make sense to always get the
>> >interrupt at the PDC and then replay it into the TLMM chip "manually"
>> >with the irq_set_irqchip_state() APIs. This way the duplicate interrupt
>> >can't happen. The only way for the interrupt handler to run would be by
>> >PDC poking the TLMM hardware to inject the irq into the status register.
>> If the PDC interrupt was always enabled and the interrupt at TLMM was
>> always disabled, all we would need to set the action handler of the PDC
>> interrupt to that of the TLMM. I couldn't find a way to retrieve that
>> nicely.
>
>Can't we just configure a different chained IRQ handler with
>irq_set_chained_handler_and_data() for each of the GPIO IRQs that are
>handled by PDC to be the interrupts provide by the PDC irq controller
>that match the GPIOs? And then set their parent irq with
>irq_set_parent() for completeness? And also move those GPIOs from the
>existing msm_gpio irqchip to a different PDC gpio irqchip that does
>nothing besides push irqchip calls up to the PDC irqchip? Then we don't
>even have to think about resending anything and we can rely on PDC to do
>all the interrupt sensing all the time but still provide the irqs from
>the GPIO controller.
>
Seems like the irqchips need to be in hierarchy for this to work, which
is not the case with TLMM and the PDC, currently.
-- Lina
Quoting Lina Iyer (2018-09-04 14:09:34)
> On Mon, Aug 27 2018 at 14:01 -0600, Stephen Boyd wrote:
> >
> >Can't we just configure a different chained IRQ handler with
> >irq_set_chained_handler_and_data() for each of the GPIO IRQs that are
> >handled by PDC to be the interrupts provide by the PDC irq controller
> >that match the GPIOs? And then set their parent irq with
> >irq_set_parent() for completeness? And also move those GPIOs from the
> >existing msm_gpio irqchip to a different PDC gpio irqchip that does
> >nothing besides push irqchip calls up to the PDC irqchip? Then we don't
> >even have to think about resending anything and we can rely on PDC to do
> >all the interrupt sensing all the time but still provide the irqs from
> >the GPIO controller.
> >
> Seems like the irqchips need to be in hierarchy for this to work, which
> is not the case with TLMM and the PDC, currently.
>
Why? Does something mandate that the chained irq is also the
hierarchical parent irqchip?
On Tue, Sep 04 2018 at 16:00 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-09-04 14:09:34)
>> On Mon, Aug 27 2018 at 14:01 -0600, Stephen Boyd wrote:
>> >
>> >Can't we just configure a different chained IRQ handler with
>> >irq_set_chained_handler_and_data() for each of the GPIO IRQs that are
>> >handled by PDC to be the interrupts provide by the PDC irq controller
>> >that match the GPIOs? And then set their parent irq with
>> >irq_set_parent() for completeness? And also move those GPIOs from the
>> >existing msm_gpio irqchip to a different PDC gpio irqchip that does
>> >nothing besides push irqchip calls up to the PDC irqchip? Then we don't
>> >even have to think about resending anything and we can rely on PDC to do
>> >all the interrupt sensing all the time but still provide the irqs from
>> >the GPIO controller.
>> >
>> Seems like the irqchips need to be in hierarchy for this to work, which
>> is not the case with TLMM and the PDC, currently.
>>
>
>Why? Does something mandate that the chained irq is also the
>hierarchical parent irqchip?
>
All the _parent() functions like irq_set_wake_parent() etc need
parent_data to be set, which is only set during hierarchy.
-- Lina