Thanks for all the helpful reviews. Here is the next revision addressing the
comments.
Changes in RFC v2:
- Address review comments #3, #4, #6, #7, #8, #9, #10
- Rebased on top of linux-next GPIO latest patches [1],[3],[4]
- Increase PDC max irqs in #2 (avoid merge conflicts with downstream)
- Add Reviewed-by #5
Note: This revision does not update writing the config registers that are
written from the PDC. There needs more discussion in that area. #6, #7
---
This series is another attempt on adding wakeup capable GPIOs for QCOM
SoC. This patchset is based on Linus's support for hierarchical GPIOs
merged into linux-next [1]. The essense of the idea remains the same as
the previous submission [2]. GPIO irqchip TLMM is setup in hierarchy wit
the PDC as the wakeup-parent. PDC's interrupt parent is the GIC. GPIOs
in QCOM SoC that are wakeup capable (when TLMM is powered off) are
routed to the PDC as well and can be detected at the always-on interrupt
controller (PDC). The idea is setup the irqchips in hierarchy and if the
interrupt is handled at the PDC, then TLMM relinquishes control and
configuration of the interrupt to the PDC.
There are few new additions in this submission. The first is the
additional SPI configuration that needs to be done to setup the GPIO
type in a register interface between the PDC and the GIC. This is needed
only for GPIOs. This registers in some QCOM SoCs is access restricted
and has to be written from the TZ. The DT bindings are also updated for
this new requirement. The second change is that with the new
hierarchical support in gpiolib, we could remove the .alloc and
.translate functions from the pinctrl driver. But to distinguish the
case where a wakeup interrupt controller needs the TLMM to configure the
GPIO interrupts (in the case of MPM interrupt controller), irqdomain
flags have been added. The third change is ensure the interrupt
controllers' interrupt pending bits are cleared when the GPIO is enabled
as an interrupt.
Please consider reviewing these patches.
Thanks,
Lina
[1]. https://lore.kernel.org/linux-gpio/[email protected]/
[2]. https://lkml.org/lkml/2019/5/7/1173
[3]. https://lore.kernel.org/r/[email protected]
[4]. https://lore.kernel.org/r/[email protected]
Lina Iyer (12):
irqdomain: add bus token DOMAIN_BUS_WAKEUP
drivers: irqchip: qcom-pdc: update max PDC interrupts
drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask
drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs
of: irq: document properties for wakeup interrupt parent
dt-bindings/interrupt-controller: pdc: add SPI config register
drivers: irqchip: pdc: additionally set type in SPI config registers
drivers: pinctrl: msm: setup GPIO chip in hierarchy
drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs
arm64: dts: qcom: add PDC interrupt controller for SDM845
arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845
arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845
Maulik Shah (2):
genirq: Introduce irq_chip_get/set_parent_state calls
drivers: irqchip: pdc: Add irqchip set/get state calls
.../bindings/interrupt-controller/interrupts.txt | 13 ++
.../bindings/interrupt-controller/qcom,pdc.txt | 13 +-
arch/arm64/boot/dts/qcom/sdm845.dtsi | 11 +
arch/arm64/configs/defconfig | 1 +
drivers/irqchip/qcom-pdc.c | 238 +++++++++++++++++++--
drivers/pinctrl/qcom/pinctrl-msm.c | 119 +++++++++++
drivers/pinctrl/qcom/pinctrl-msm.h | 16 ++
drivers/pinctrl/qcom/pinctrl-sdm845.c | 23 +-
include/linux/irq.h | 6 +
include/linux/irqdomain.h | 1 +
include/linux/soc/qcom/irq.h | 32 +++
kernel/irq/chip.c | 44 ++++
12 files changed, 502 insertions(+), 15 deletions(-)
create mode 100644 include/linux/soc/qcom/irq.h
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Newer SoCs have increased the number of interrupts routed to the PDC
interrupt controller. Update the definition of max PDC interrupts.
Signed-off-by: Lina Iyer <[email protected]>
---
drivers/irqchip/qcom-pdc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index faa7d61..b230794 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2017-2019, The Linux Foundation. All rights reserved.
*/
#include <linux/err.h>
@@ -18,7 +18,7 @@
#include <linux/slab.h>
#include <linux/types.h>
-#define PDC_MAX_IRQS 126
+#define PDC_MAX_IRQS 168
#define CLEAR_INTR(reg, intr) (reg & ~(1 << intr))
#define ENABLE_INTR(reg, intr) (reg | (1 << intr))
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Maulik Shah <[email protected]>
On certain QTI chipsets some GPIOs are direct-connect interrupts to the
GIC to be used as regular interrupt lines. When the GPIOs are not used
for interrupt generation the interrupt line is disabled. But disabling
the interrupt at GIC does not prevent the interrupt to be reported as
pending at GIC_ISPEND. Later, when drivers call enable_irq() on the
interrupt, an unwanted interrupt occurs.
Introduce get and set methods for irqchip's parent to clear it's pending
irq state. This then can be invoked by the GPIO interrupt controller on
the parents in it hierarchy to clear the interrupt before enabling the
interrupt.
Signed-off-by: Maulik Shah <[email protected]>
[updated commit text and minor code fixes]
Signed-off-by: Lina Iyer <[email protected]>
---
Changes in RFC v2 -
- Rephrase commit text
- Address code review comments
---
include/linux/irq.h | 6 ++++++
kernel/irq/chip.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+)
diff --git a/include/linux/irq.h b/include/linux/irq.h
index fb301cf..7853eb9 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -610,6 +610,12 @@ extern int irq_chip_pm_put(struct irq_data *data);
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
extern void handle_fasteoi_ack_irq(struct irq_desc *desc);
extern void handle_fasteoi_mask_irq(struct irq_desc *desc);
+extern int irq_chip_set_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool val);
+extern int irq_chip_get_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool *state);
extern void irq_chip_enable_parent(struct irq_data *data);
extern void irq_chip_disable_parent(struct irq_data *data);
extern void irq_chip_ack_parent(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index b76703b..b3fa2d8 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1298,6 +1298,50 @@ EXPORT_SYMBOL_GPL(handle_fasteoi_mask_irq);
#endif /* CONFIG_IRQ_FASTEOI_HIERARCHY_HANDLERS */
/**
+ * irq_chip_set_parent_state - set the state of a parent interrupt.
+ *
+ * @data: Pointer to interrupt specific data
+ * @which: State to be restored (one of IRQCHIP_STATE_*)
+ * @val: Value corresponding to @which
+ *
+ * Conditional success, if the underlying irqchip does not implement it.
+ */
+int irq_chip_set_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool val)
+{
+ data = data->parent_data;
+
+ if (!data || !data->chip->irq_set_irqchip_state)
+ return 0;
+
+ return data->chip->irq_set_irqchip_state(data, which, val);
+}
+EXPORT_SYMBOL_GPL(irq_chip_set_parent_state);
+
+/**
+ * irq_chip_get_parent_state - get the state of a parent interrupt.
+ *
+ * @data: Pointer to interrupt specific data
+ * @which: one of IRQCHIP_STATE_* the caller wants to know
+ * @state: a pointer to a boolean where the state is to be stored
+ *
+ * Conditional success, if the underlying irqchip does not implement it.
+ */
+int irq_chip_get_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool *state)
+{
+ data = data->parent_data;
+
+ if (!data || !data->chip->irq_get_irqchip_state)
+ return 0;
+
+ return data->chip->irq_get_irqchip_state(data, which, state);
+}
+EXPORT_SYMBOL_GPL(irq_chip_get_parent_state);
+
+/**
* irq_chip_enable_parent - Enable the parent interrupt (defaults to unmask if
* NULL)
* @data: Pointer to interrupt specific data
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Maulik Shah <[email protected]>
Add irqchip calls to set/get interrupt state from the parent interrupt
controller. When GPIOs are renabled as interrupt lines, it is desirable
to clear the interrupt state at the GIC. This avoids any unwanted
interrupt as a result of stale pending state recorded when the line was
used as a GPIO.
Signed-off-by: Maulik Shah <[email protected]>
[updated commit text]
Signed-off-by: Lina Iyer <[email protected]>
---
drivers/irqchip/qcom-pdc.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index affb0bfa..2b49e70 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -5,6 +5,7 @@
#include <linux/err.h>
#include <linux/init.h>
+#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqchip.h>
#include <linux/irqdomain.h>
@@ -87,6 +88,24 @@ static void qcom_pdc_gic_disable(struct irq_data *d)
irq_chip_disable_parent(d);
}
+static int qcom_pdc_gic_get_irqchip_state(struct irq_data *d,
+ enum irqchip_irq_state which, bool *state)
+{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return 0;
+
+ return irq_chip_get_parent_state(d, which, state);
+}
+
+static int qcom_pdc_gic_set_irqchip_state(struct irq_data *d,
+ enum irqchip_irq_state which, bool value)
+{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return 0;
+
+ return irq_chip_set_parent_state(d, which, value);
+}
+
static void qcom_pdc_gic_enable(struct irq_data *d)
{
if (d->hwirq == GPIO_NO_WAKE_IRQ)
@@ -248,6 +267,8 @@ static struct irq_chip qcom_pdc_gic_chip = {
.irq_unmask = qcom_pdc_gic_unmask,
.irq_disable = qcom_pdc_gic_disable,
.irq_enable = qcom_pdc_gic_enable,
+ .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state,
+ .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state,
.irq_retrigger = irq_chip_retrigger_hierarchy,
.irq_set_type = qcom_pdc_gic_set_type,
.flags = IRQCHIP_MASK_ON_SUSPEND |
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Some interrupt controllers in a SoC, are always powered on and have a
select interrupts routed to them, so that they can wakeup the SoC from
suspend. Add wakeup-parent DT property to refer to these interrupt
controllers.
Cc: [email protected]
Signed-off-by: Lina Iyer <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../devicetree/bindings/interrupt-controller/interrupts.txt | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
index 8a3c408..c10e310 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
@@ -108,3 +108,16 @@ commonly used:
sensitivity = <7>;
};
};
+
+3) Interrupt wakeup parent
+--------------------------
+
+Some interrupt controllers in a SoC, are always powered on and have a select
+interrupts routed to them, so that they can wakeup the SoC from suspend. These
+interrupt controllers do not fall into the category of a parent interrupt
+controller and can be specified by the "wakeup-parent" property and contain a
+single phandle referring to the wakeup capable interrupt controller.
+
+ Example:
+ wakeup-parent = <&pdc_intc>;
+
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
A single controller can handle normal interrupts and wake-up interrupts
independently, with a different numbering space. It is thus crucial to
allow the driver for such a controller discriminate between the two.
A simple way to do so is to tag the wake-up irqdomain with a "bus token"
that indicates the wake-up domain. This slightly abuses the notion of
bus, but also radically simplifies the design of such a driver. Between
two evils, we choose the least damaging.
Suggested-by: Stephen Boyd <[email protected]>
Signed-off-by: Lina Iyer <[email protected]>
---
include/linux/irqdomain.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 07ec8b3..cc846ab 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -83,6 +83,7 @@ enum irq_domain_bus_token {
DOMAIN_BUS_IPI,
DOMAIN_BUS_FSL_MC_MSI,
DOMAIN_BUS_TI_SCI_INTA_MSI,
+ DOMAIN_BUS_WAKEUP,
};
/**
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
When an interrupt is to be serviced, the convention is to mask the
interrupt at the chip and unmask after servicing the interrupt. Enabling
and disabling the interrupt at the PDC irqchip causes an interrupt storm
due to the way dual edge interrupts are handled in hardware.
Skip configuring the PDC when the IRQ is masked and unmasked, instead
use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE
register at the PDC. The PDC's IRQ_ENABLE register is only used during
the monitoring mode when the system is asleep and is not needed for
active mode detection.
Signed-off-by: Lina Iyer <[email protected]>
---
drivers/irqchip/qcom-pdc.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index b230794..5eef5ea 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -63,15 +63,25 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
raw_spin_unlock(&pdc_lock);
}
-static void qcom_pdc_gic_mask(struct irq_data *d)
+static void qcom_pdc_gic_disable(struct irq_data *d)
{
pdc_enable_intr(d, false);
+ irq_chip_disable_parent(d);
+}
+
+static void qcom_pdc_gic_enable(struct irq_data *d)
+{
+ pdc_enable_intr(d, true);
+ irq_chip_enable_parent(d);
+}
+
+static void qcom_pdc_gic_mask(struct irq_data *d)
+{
irq_chip_mask_parent(d);
}
static void qcom_pdc_gic_unmask(struct irq_data *d)
{
- pdc_enable_intr(d, true);
irq_chip_unmask_parent(d);
}
@@ -148,6 +158,8 @@ static struct irq_chip qcom_pdc_gic_chip = {
.irq_eoi = irq_chip_eoi_parent,
.irq_mask = qcom_pdc_gic_mask,
.irq_unmask = qcom_pdc_gic_unmask,
+ .irq_disable = qcom_pdc_gic_disable,
+ .irq_enable = qcom_pdc_gic_enable,
.irq_retrigger = irq_chip_retrigger_hierarchy,
.irq_set_type = qcom_pdc_gic_set_type,
.flags = IRQCHIP_MASK_ON_SUSPEND |
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Fri, Sep 13, 2019 at 11:59 PM Lina Iyer <[email protected]> wrote:
> Some interrupt controllers in a SoC, are always powered on and have a
> select interrupts routed to them, so that they can wakeup the SoC from
> suspend. Add wakeup-parent DT property to refer to these interrupt
> controllers.
>
> Cc: [email protected]
> Signed-off-by: Lina Iyer <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
Hi,
On Fri, Sep 13, 2019 at 3:00 PM Lina Iyer <[email protected]> wrote:
>
> Some interrupt controllers in a SoC, are always powered on and have a
> select interrupts routed to them, so that they can wakeup the SoC from
> suspend. Add wakeup-parent DT property to refer to these interrupt
> controllers.
>
> Cc: [email protected]
> Signed-off-by: Lina Iyer <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> ---
> .../devicetree/bindings/interrupt-controller/interrupts.txt | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> index 8a3c408..c10e310 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> @@ -108,3 +108,16 @@ commonly used:
> sensitivity = <7>;
> };
> };
> +
> +3) Interrupt wakeup parent
> +--------------------------
> +
> +Some interrupt controllers in a SoC, are always powered on and have a select
> +interrupts routed to them, so that they can wakeup the SoC from suspend. These
> +interrupt controllers do not fall into the category of a parent interrupt
> +controller and can be specified by the "wakeup-parent" property and contain a
> +single phandle referring to the wakeup capable interrupt controller.
> +
> + Example:
> + wakeup-parent = <&pdc_intc>;
> +
nit: git flags the above line as whitespace damage. Please remove
if/when you spin.
Thanks!
-Doug