[PATCH RFC 00/14] qcom: support wakeup capable GPIOs

[PATCH RFC 00/14] qcom: support wakeup capable GPIOs

[Lina Iyer]

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 with
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

Lina Iyer (12):
  irqdomain: add bus token DOMAIN_BUS_WAKEUP
  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: fix use of deprecated gpiolib APIs
  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

 .../interrupt-controller/interrupts.txt       |  13 +
 .../interrupt-controller/qcom,pdc.txt         |   9 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  11 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/irqchip/qcom-pdc.c                    | 234 +++++++++++++++++-
 drivers/pinctrl/qcom/pinctrl-msm.c            | 142 +++++++++--
 drivers/pinctrl/qcom/pinctrl-msm.h            |  16 ++
 drivers/pinctrl/qcom/pinctrl-sdm845.c         |  83 ++++++-
 include/linux/irq.h                           |   6 +
 include/linux/irqdomain.h                     |   1 +
 include/linux/soc/qcom/irq.h                  |  34 +++
 kernel/irq/chip.c                             |  44 ++++
 12 files changed, 566 insertions(+), 28 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

[PATCH RFC 01/14] irqdomain: add bus token DOMAIN_BUS_WAKEUP

[Lina Iyer]

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 <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 include/linux/irqdomain.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 07ec8b390161..cc846abeff28 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

[PATCH RFC 02/14] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask

[Lina Iyer]

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 <ilina@codeaurora.org>
---
 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 faa7d61b9d6c..338fae604af5 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

[PATCH RFC 03/14] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Lina Iyer]

Introduce a new domain for wakeup capable GPIOs. The domain can be
requested using the bus token DOMAIN_BUS_WAKEUP. In the following
patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
irqchip. Requesting a wakeup GPIO will setup the GPIO and the
corresponding PDC interrupt as its parent.

Co-developed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/irqchip/qcom-pdc.c   | 104 ++++++++++++++++++++++++++++++++---
 include/linux/soc/qcom/irq.h |  34 ++++++++++++
 2 files changed, 129 insertions(+), 9 deletions(-)
 create mode 100644 include/linux/soc/qcom/irq.h

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 338fae604af5..ad1faf634bcf 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -13,12 +13,13 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/soc/qcom/irq.h>
 #include <linux/spinlock.h>
-#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 
 #define PDC_MAX_IRQS		126
+#define PDC_MAX_GPIO_IRQS	256
 
 #define CLEAR_INTR(reg, intr)	(reg & ~(1 << intr))
 #define ENABLE_INTR(reg, intr)	(reg | (1 << intr))
@@ -26,6 +27,8 @@
 #define IRQ_ENABLE_BANK		0x10
 #define IRQ_i_CFG		0x110
 
+#define PDC_NO_PARENT_IRQ	~0UL
+
 struct pdc_pin_region {
 	u32 pin_base;
 	u32 parent_base;
@@ -65,23 +68,35 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
 
 static void qcom_pdc_gic_disable(struct irq_data *d)
 {
+	if (d->hwirq == GPIO_NO_WAKE_IRQ)
+		return;
+
 	pdc_enable_intr(d, false);
 	irq_chip_disable_parent(d);
 }
 
 static void qcom_pdc_gic_enable(struct irq_data *d)
 {
+	if (d->hwirq == GPIO_NO_WAKE_IRQ)
+		return;
+
 	pdc_enable_intr(d, true);
 	irq_chip_enable_parent(d);
 }
 
 static void qcom_pdc_gic_mask(struct irq_data *d)
 {
+	if (d->hwirq == GPIO_NO_WAKE_IRQ)
+		return;
+
 	irq_chip_mask_parent(d);
 }
 
 static void qcom_pdc_gic_unmask(struct irq_data *d)
 {
+	if (d->hwirq == GPIO_NO_WAKE_IRQ)
+		return;
+
 	irq_chip_unmask_parent(d);
 }
 
@@ -124,6 +139,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 	int pin_out = d->hwirq;
 	enum pdc_irq_config_bits pdc_type;
 
+	if (pin_out == GPIO_NO_WAKE_IRQ)
+		return 0;
+
 	switch (type) {
 	case IRQ_TYPE_EDGE_RISING:
 		pdc_type = PDC_EDGE_RISING;
@@ -181,8 +199,7 @@ static irq_hw_number_t get_parent_hwirq(int pin)
 			return (region->parent_base + pin - region->pin_base);
 	}
 
-	WARN_ON(1);
-	return ~0UL;
+	return PDC_NO_PARENT_IRQ;
 }
 
 static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
@@ -211,17 +228,17 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq,
 
 	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
 	if (ret)
-		return -EINVAL;
-
-	parent_hwirq = get_parent_hwirq(hwirq);
-	if (parent_hwirq == ~0UL)
-		return -EINVAL;
+		return ret;
 
 	ret  = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
 					     &qcom_pdc_gic_chip, NULL);
 	if (ret)
 		return ret;
 
+	parent_hwirq = get_parent_hwirq(hwirq);
+	if (parent_hwirq == PDC_NO_PARENT_IRQ)
+		return 0;
+
 	if (type & IRQ_TYPE_EDGE_BOTH)
 		type = IRQ_TYPE_EDGE_RISING;
 
@@ -244,6 +261,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
 	.free		= irq_domain_free_irqs_common,
 };
 
+static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
+			       unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec *fwspec = data;
+	struct irq_fwspec parent_fwspec;
+	irq_hw_number_t hwirq, parent_hwirq;
+	unsigned int type;
+	int ret;
+
+	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &qcom_pdc_gic_chip, NULL);
+	if (ret)
+		return ret;
+
+	if (hwirq == GPIO_NO_WAKE_IRQ)
+		return 0;
+
+	parent_hwirq = get_parent_hwirq(hwirq);
+	if (parent_hwirq == PDC_NO_PARENT_IRQ)
+		return 0;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		type = IRQ_TYPE_EDGE_RISING;
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		type = IRQ_TYPE_LEVEL_HIGH;
+
+	parent_fwspec.fwnode      = domain->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0]    = 0;
+	parent_fwspec.param[1]    = parent_hwirq;
+	parent_fwspec.param[2]    = type;
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+					    &parent_fwspec);
+}
+
+static int qcom_pdc_gpio_domain_select(struct irq_domain *d,
+				       struct irq_fwspec *fwspec,
+				       enum irq_domain_bus_token bus_token)
+{
+	return (bus_token == DOMAIN_BUS_WAKEUP);
+}
+
+static const struct irq_domain_ops qcom_pdc_gpio_ops = {
+	.select		= qcom_pdc_gpio_domain_select,
+	.alloc		= qcom_pdc_gpio_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
 static int pdc_setup_pin_mapping(struct device_node *np)
 {
 	int ret, n;
@@ -282,7 +353,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)
 
 static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 {
-	struct irq_domain *parent_domain, *pdc_domain;
+	struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
 	int ret;
 
 	pdc_base = of_iomap(node, 0);
@@ -313,8 +384,23 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 		goto fail;
 	}
 
+	pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain,
+						      IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP,
+						      PDC_MAX_GPIO_IRQS,
+						      of_fwnode_handle(node),
+						      &qcom_pdc_gpio_ops, NULL);
+	if (!pdc_gpio_domain) {
+		pr_err("%pOF: GIC domain add failed for GPIO domain\n", node);
+		ret = -ENOMEM;
+		goto remove;
+	}
+
+	irq_domain_update_bus_token(pdc_gpio_domain, DOMAIN_BUS_WAKEUP);
+
 	return 0;
 
+remove:
+	irq_domain_remove(pdc_domain);
 fail:
 	kfree(pdc_region);
 	iounmap(pdc_base);
diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
new file mode 100644
index 000000000000..73239917dc38
--- /dev/null
+++ b/include/linux/soc/qcom/irq.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __QCOM_IRQ_H
+#define __QCOM_IRQ_H
+
+#include <linux/irqdomain.h>
+
+#define GPIO_NO_WAKE_IRQ	~0U
+
+/**
+ * QCOM specific IRQ domain flags that distinguishes the handling of wakeup
+ * capable interrupts by different interrupt controllers.
+ *
+ * IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP: Line must be masked at TLMM and the
+ *                                  interrupt configuration is done at PDC
+ * IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP: Interrupt configuration is handled at TLMM
+ */
+#define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP		(1 << 17)
+#define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP		(1 << 18)
+
+/**
+ * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt
+ *                                configuration
+ * @parent: irq domain
+ *
+ * This QCOM specific irq domain call returns if the interrupt controller
+ * requires the interrupt be masked at the child interrupt controller.
+ */
+static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *parent)
+{
+	return (parent->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP);
+}
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC 04/14] of: irq: document properties for wakeup interrupt parent

[Lina Iyer]

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: devicetree@vger.kernel.org
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 .../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 8a3c40829899..c10e31050dd2 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

[PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register

[Lina Iyer]

In addition to configuring the PDC, additional registers that interface
the GIC have to be configured to match the GPIO type. The registers on
some QCOM SoCs are access restricted, while on other SoCs are not. They
SoCs with access restriction to these SPI registers need to be written
from the firmware using the SCM interface. Add a flag to indicate if the
register is to be written using SCM interface.

Cc: devicetree@vger.kernel.org
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 .../bindings/interrupt-controller/qcom,pdc.txt           | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
index 8e0797cb1487..852fcba98ea6 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
@@ -50,15 +50,22 @@ Properties:
 		    The second element is the GIC hwirq number for the PDC port.
 		    The third element is the number of interrupts in sequence.
 
+- qcom,scm-spi-cfg:
+	Usage: optional
+	Value type: <bool>
+	Definition: Specifies if the SPI configuration registers have to be
+		    written from the firmware.
+
 Example:
 
 	pdc: interrupt-controller@b220000 {
 		compatible = "qcom,sdm845-pdc";
-		reg = <0xb220000 0x30000>;
+		reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
 		qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
 		#interrupt-cells = <2>;
 		interrupt-parent = <&intc>;
 		interrupt-controller;
+		qcom,scm-spi-cfg;
 	};
 
 DT binding of a device that wants to use the GIC SPI 514 as a wakeup
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC 06/14] drivers: irqchip: pdc: additionally set type in SPI config registers

[Lina Iyer]

GPIOs that can be configured as wakeup are routed to the PDC wakeup
interrupt controller and from there to the GIC interrupt controller. On
some QCOM SoCs, the interface to the GIC for wakeup capable GPIOs have
additional hardware registers that need to be configured as well to
match the trigger type of the GPIO. This register interfaces the PDC to
the GIC and therefore updated from the PDC driver.

Typically, the firmware intializes the interface registers for the
wakeup capable GPIOs with commonly used GPIO trigger type, but it is
possible that a platform may want to use the GPIO differently. So, in
addition to configuring the PDC, configure the interface registers as
well.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/irqchip/qcom-pdc.c | 93 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index ad1faf634bcf..bf5f98bb4d2b 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -18,6 +18,8 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 
+#include <linux/qcom_scm.h>
+
 #define PDC_MAX_IRQS		126
 #define PDC_MAX_GPIO_IRQS	256
 
@@ -35,10 +37,20 @@ struct pdc_pin_region {
 	u32 cnt;
 };
 
+struct spi_cfg_regs {
+	union {
+		u64 start;
+		void __iomem *base;
+	};
+	resource_size_t size;
+	bool scm_io;
+};
+
 static DEFINE_RAW_SPINLOCK(pdc_lock);
 static void __iomem *pdc_base;
 static struct pdc_pin_region *pdc_region;
 static int pdc_region_cnt;
+static struct spi_cfg_regs *spi_cfg;
 
 static void pdc_reg_write(int reg, u32 i, u32 val)
 {
@@ -100,6 +112,57 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
 	irq_chip_unmask_parent(d);
 }
 
+static u32 __spi_pin_read(unsigned int pin)
+{
+	void __iomem *cfg_reg = spi_cfg->base + pin * 4;
+	u64 scm_cfg_reg = spi_cfg->start + pin * 4;
+
+	if (spi_cfg->scm_io) {
+		unsigned int val;
+
+		qcom_scm_io_readl(scm_cfg_reg, &val);
+		return val;
+	} else {
+		return readl(cfg_reg);
+	}
+}
+
+static void __spi_pin_write(unsigned int pin, unsigned int val)
+{
+	void __iomem *cfg_reg = spi_cfg->base + pin * 4;
+	u64 scm_cfg_reg = spi_cfg->start + pin * 4;
+
+	if (spi_cfg->scm_io)
+		qcom_scm_io_writel(scm_cfg_reg, val);
+	else
+		writel(val, cfg_reg);
+}
+
+static int spi_configure_type(irq_hw_number_t hwirq, unsigned int type)
+{
+	int spi = hwirq - 32;
+	u32 pin = spi / 32;
+	u32 mask = BIT(spi % 32);
+	u32 val;
+	unsigned long flags;
+
+	if (!spi_cfg)
+		return 0;
+
+	if (pin * 4 > spi_cfg->size)
+		return -EFAULT;
+
+	raw_spin_lock_irqsave(&pdc_lock, flags);
+	val = __spi_pin_read(pin);
+	val &= ~mask;
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		val |= mask;
+	__spi_pin_write(pin, val);
+	raw_spin_unlock_irqrestore(&pdc_lock, flags);
+
+	return 0;
+}
+
 /*
  * GIC does not handle falling edge or active low. To allow falling edge and
  * active low interrupts to be handled at GIC, PDC has an inverter that inverts
@@ -137,7 +200,9 @@ enum pdc_irq_config_bits {
 static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 {
 	int pin_out = d->hwirq;
+	int parent_hwirq = d->parent_data->hwirq;
 	enum pdc_irq_config_bits pdc_type;
+	int ret;
 
 	if (pin_out == GPIO_NO_WAKE_IRQ)
 		return 0;
@@ -168,6 +233,11 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 
 	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
 
+	/* Additionally, configure (only) the GPIO in the f/w */
+	ret = spi_configure_type(parent_hwirq, type);
+	if (ret)
+		return ret;
+
 	return irq_chip_set_type_parent(d, type);
 }
 
@@ -354,6 +424,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)
 static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 {
 	struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
+	struct resource res;
 	int ret;
 
 	pdc_base = of_iomap(node, 0);
@@ -384,6 +455,27 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 		goto fail;
 	}
 
+	ret = of_address_to_resource(node, 1, &res);
+	if (!ret) {
+		spi_cfg = kcalloc(1, sizeof(*spi_cfg), GFP_KERNEL);
+		if (!spi_cfg) {
+			ret = -ENOMEM;
+			goto remove;
+		}
+		spi_cfg->scm_io = of_find_property(node,
+						   "qcom,scm-spi-cfg", NULL);
+		spi_cfg->size = resource_size(&res);
+		if (spi_cfg->scm_io) {
+			spi_cfg->start = res.start;
+		} else {
+			spi_cfg->base = ioremap(res.start, spi_cfg->size);
+			if (!spi_cfg->base) {
+				ret = -ENOMEM;
+				goto remove;
+			}
+		}
+	}
+
 	pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain,
 						      IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP,
 						      PDC_MAX_GPIO_IRQS,
@@ -401,6 +493,7 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 
 remove:
 	irq_domain_remove(pdc_domain);
+	kfree(spi_cfg);
 fail:
 	kfree(pdc_region);
 	iounmap(pdc_base);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC 07/14] genirq: Introduce irq_chip_get/set_parent_state calls

[Lina Iyer]

From: Maulik Shah <mkshah@codeaurora.org>

On certain QTI chipsets some GPIOs are direct-connect interrupts
to the GIC.

Even when GPIOs are not used for interrupt generation and interrupt
line is disabled, it does not prevent interrupt to get pending at
GIC_ISPEND. When drivers call enable_irq unwanted interrupt occures.

Introduce irq_chip_get/set_parent_state calls to clear pending irq
which can get called within irq_enable of child irq chip to clear
any pending irq before enabling.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 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 fb301cf29148..7853eb9301f2 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 b76703b2c0af..6bb5b22bb0a7 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1297,6 +1297,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
+ *
+ */
+int irq_chip_set_parent_state(struct irq_data *data,
+			      enum irqchip_irq_state which,
+			      bool val)
+{
+	data = data->parent_data;
+	if (!data)
+		return 0;
+
+	if (data->chip->irq_set_irqchip_state)
+		return data->chip->irq_set_irqchip_state(data, which, val);
+
+	return 0;
+}
+EXPORT_SYMBOL(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
+ *
+ */
+int irq_chip_get_parent_state(struct irq_data *data,
+			      enum irqchip_irq_state which,
+			      bool *state)
+{
+	data = data->parent_data;
+	if (!data)
+		return 0;
+
+	if (data->chip->irq_get_irqchip_state)
+		return data->chip->irq_get_irqchip_state(data, which, state);
+
+	return 0;
+}
+EXPORT_SYMBOL(irq_chip_get_parent_state);
+
 /**
  * irq_chip_enable_parent - Enable the parent interrupt (defaults to unmask if
  * NULL)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC 08/14] drivers: irqchip: pdc: Add irqchip set/get state calls

[Lina Iyer]

From: Maulik Shah <mkshah@codeaurora.org>

Add irqchip calls to set/get interrupt status from the parent interrupt
controller.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 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 bf5f98bb4d2b..ffd5f83d1023 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

[PATCH RFC 09/14] drivers: pinctrl: msm: fix use of deprecated gpiolib APIs

[Lina Iyer]

Replace gpiochip_irqchip_add() and gpiochip_set_chained_irqchip() calls
by populating the gpio_irq_chip data structures instead.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 7f35c196bb3e..76e8528e4d0a 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1027,7 +1027,19 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
 	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
-	ret = gpiochip_add_data(&pctrl->chip, pctrl);
+	chip->irq.chip = &pctrl->irq_chip;
+	chip->irq.default_type = IRQ_TYPE_NONE;
+	chip->irq.handler = handle_bad_irq;
+	chip->irq.fwnode = pctrl->dev->fwnode;
+	chip->irq.parent_handler = msm_gpio_irq_handler;
+	chip->irq.num_parents = 1;
+	chip->irq.parents = devm_kcalloc(pctrl->dev, 1,
+					 sizeof(*chip->irq.parents),
+					 GFP_KERNEL);
+	if (!chip->irq.parents)
+		return -ENOMEM;
+
+	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed register gpiochip\n");
 		return ret;
@@ -1053,20 +1065,6 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 		}
 	}
 
-	ret = gpiochip_irqchip_add(chip,
-				   &pctrl->irq_chip,
-				   0,
-				   handle_edge_irq,
-				   IRQ_TYPE_NONE);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n");
-		gpiochip_remove(&pctrl->chip);
-		return -ENOSYS;
-	}
-
-	gpiochip_set_chained_irqchip(chip, &pctrl->irq_chip, pctrl->irq,
-				     msm_gpio_irq_handler);
-
 	return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC 10/14] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Lina Iyer]

Some GPIOs are marked as wakeup capable and are routed to another
interrupt controller that is an always-domain and can detect interrupts
even most of the SoC is powered off. The wakeup interrupt controller
wakes up the GIC and replays the interrupt at the GIC.

Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller
and ensure the wakeup GPIOs are handled correctly.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 114 +++++++++++++++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.h |  16 ++++
 2 files changed, 130 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 76e8528e4d0a..d626264fe678 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -23,6 +23,8 @@
 #include <linux/pm.h>
 #include <linux/log2.h>
 
+#include <linux/soc/qcom/irq.h>
+
 #include "../core.h"
 #include "../pinconf.h"
 #include "pinctrl-msm.h"
@@ -44,6 +46,7 @@
  * @enabled_irqs:   Bitmap of currently enabled irqs.
  * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
  *                  detection.
+ * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt contrroller
  * @soc;            Reference to soc_data of platform specific data.
  * @regs:           Base addresses for the TLMM tiles.
  */
@@ -61,6 +64,7 @@ struct msm_pinctrl {
 
 	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
 	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+	DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO);
 
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs[MAX_NR_TILES];
@@ -708,6 +712,12 @@ static void msm_gpio_irq_mask(struct irq_data *d)
 	unsigned long flags;
 	u32 val;
 
+	if (d->parent_data)
+		irq_chip_mask_parent(d);
+
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -752,6 +762,12 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
 	unsigned long flags;
 	u32 val;
 
+	if (d->parent_data)
+		irq_chip_unmask_parent(d);
+
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -779,10 +795,43 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
 
 static void msm_gpio_irq_enable(struct irq_data *d)
 {
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+
+	/*
+	 * Clear the interrupt that may be pending before we enable
+	 * the line.
+	 * This is especially a problem with the GPIOs routed to the
+	 * PDC. These GPIOs are direct-connect interrupts to the GIC.
+	 * Disabling the interrupt line at the PDC does not prevent
+	 * the interrupt from being latched at the GIC. The state at
+	 * GIC needs to be cleared before enabling.
+	 */
+	if (d->parent_data) {
+		irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
+		irq_chip_enable_parent(d);
+	}
+
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return;
 
 	msm_gpio_irq_clear_unmask(d, true);
 }
 
+static void msm_gpio_irq_disable(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+
+	if (d->parent_data)
+		irq_chip_disable_parent(d);
+
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return;
+
+	msm_gpio_irq_mask(d);
+}
+
 static void msm_gpio_irq_unmask(struct irq_data *d)
 {
 	msm_gpio_irq_clear_unmask(d, false);
@@ -796,6 +845,9 @@ static void msm_gpio_irq_ack(struct irq_data *d)
 	unsigned long flags;
 	u32 val;
 
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -821,6 +873,12 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	unsigned long flags;
 	u32 val;
 
+	if (d->parent_data)
+		irq_chip_set_type_parent(d, type);
+
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return 0;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -913,6 +971,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned long flags;
 
+	if (d->parent_data)
+		irq_chip_set_wake_parent(d, on);
+
+	/*
+	 * While they may not wake up when the TLMM is powered off,
+	 * some GPIOs would like to wakeup the system from suspend
+	 * when TLMM is powered on. To allow that, enable the GPIO
+	 * summary line to be wakeup capable at GIC.
+	 */
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	irq_set_irq_wake(pctrl->irq, on);
@@ -991,6 +1058,30 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static int msm_gpio_wakeirq(struct gpio_chip *gc,
+			    unsigned int child,
+			    unsigned int child_type,
+			    unsigned int *parent,
+			    unsigned int *parent_type)
+{
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct msm_gpio_wakeirq_map *map;
+	int i;
+
+	*parent = GPIO_NO_WAKE_IRQ;
+	*parent_type = IRQ_TYPE_EDGE_RISING;
+
+	for (i = 0; i < pctrl->soc->nwakeirq_map; i++) {
+		map = &pctrl->soc->wakeirq_map[i];
+		if (map->gpio == child) {
+			*parent = map->wakeirq;
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 {
 	if (pctrl->soc->reserved_gpios)
@@ -1004,6 +1095,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	struct gpio_chip *chip;
 	int ret;
 	unsigned ngpio = pctrl->soc->ngpios;
+	struct device_node *dn;
 
 	if (WARN_ON(ngpio > MAX_NR_GPIO))
 		return -EINVAL;
@@ -1019,14 +1111,36 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 
 	pctrl->irq_chip.name = "msmgpio";
 	pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
+	pctrl->irq_chip.irq_disable = msm_gpio_irq_disable;
 	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
 	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
+	pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
 	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
 	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
+	dn = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
+	if (dn) {
+		int i;
+		bool skip;
+		unsigned int gpio;
+
+		chip->irq.parent_domain = irq_find_matching_host(dn,
+						 DOMAIN_BUS_WAKEUP);
+		of_node_put(dn);
+		if (!chip->irq.parent_domain)
+			return -EPROBE_DEFER;
+		chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq;
+
+		skip = irq_domain_qcom_handle_wakeup(chip->irq.parent_domain);
+		for (i = 0; skip && i < pctrl->soc->nwakeirq_map; i++) {
+			gpio = pctrl->soc->wakeirq_map[i].gpio;
+			set_bit(gpio, pctrl->skip_wake_irqs);
+		}
+	}
+
 	chip->irq.chip = &pctrl->irq_chip;
 	chip->irq.default_type = IRQ_TYPE_NONE;
 	chip->irq.handler = handle_bad_irq;
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 48569cda8471..15470203b446 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -5,6 +5,8 @@
 #ifndef __PINCTRL_MSM_H__
 #define __PINCTRL_MSM_H__
 
+#include <linux/gpio/driver.h>
+
 struct pinctrl_pin_desc;
 
 /**
@@ -91,6 +93,16 @@ struct msm_pingroup {
 	unsigned intr_detection_width:5;
 };
 
+/**
+ * struct msm_gpio_wakeirq_map - Map of GPIOs and their wakeup pins
+ * @gpio:          The GPIOs that are wakeup capable
+ * @wakeirq:       The interrupt at the always-on interrupt controller
+ */
+struct msm_gpio_wakeirq_map {
+	unsigned int gpio;
+	unsigned int wakeirq;
+};
+
 /**
  * struct msm_pinctrl_soc_data - Qualcomm pin controller driver configuration
  * @pins:	    An array describing all pins the pin controller affects.
@@ -101,6 +113,8 @@ struct msm_pingroup {
  * @ngroups:	    The numbmer of entries in @groups.
  * @ngpio:	    The number of pingroups the driver should expose as GPIOs.
  * @pull_no_keeper: The SoC does not support keeper bias.
+ * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
+ * @nwakeirq_map:   The number of entries in @hierarchy_map
  */
 struct msm_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
@@ -114,6 +128,8 @@ struct msm_pinctrl_soc_data {
 	const char *const *tiles;
 	unsigned int ntiles;
 	const int *reserved_gpios;
+	const struct msm_gpio_wakeirq_map *wakeirq_map;
+	unsigned int nwakeirq_map;
 };
 
 extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC 11/14] drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs

[Lina Iyer]

Add interrupt parents for wakeup capable GPIOs for Qualcomm SDM845 SoC.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-sdm845.c | 83 ++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
index 39f498c09906..5f43dabcd8eb 100644
--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2016-2019, The Linux Foundation. All rights reserved.
  */
 
 #include <linux/acpi.h>
@@ -1282,6 +1282,84 @@ static const int sdm845_acpi_reserved_gpios[] = {
 	0, 1, 2, 3, 81, 82, 83, 84, -1
 };
 
+static const struct msm_gpio_wakeirq_map sdm845_pdc_map[] = {
+	{1, 30},
+	{3, 31},
+	{5, 32},
+	{10, 33},
+	{11, 34},
+	{20, 35},
+	{22, 36},
+	{24, 37},
+	{26, 38},
+	{30, 39},
+	{31, 117},
+	{32, 41},
+	{34, 42},
+	{36, 43},
+	{37, 44},
+	{38, 45},
+	{39, 46},
+	{40, 47},
+	{41, 115},
+	{43, 49},
+	{44, 50},
+	{46, 51},
+	{48, 52},
+	{49, 118},
+	{52, 54},
+	{53, 55},
+	{54, 56},
+	{56, 57},
+	{57, 58},
+	{58, 59},
+	{59, 60},
+	{60, 61},
+	{61, 62},
+	{62, 63},
+	{63, 64},
+	{64, 65},
+	{66, 66},
+	{68, 67},
+	{71, 68},
+	{73, 69},
+	{77, 70},
+	{78, 71},
+	{79, 72},
+	{80, 73},
+	{84, 74},
+	{85, 75},
+	{86, 76},
+	{88, 77},
+	{89, 116},
+	{91, 79},
+	{92, 80},
+	{95, 81},
+	{96, 82},
+	{97, 83},
+	{101, 84},
+	{103, 85},
+	{104, 86},
+	{115, 90},
+	{116, 91},
+	{117, 92},
+	{118, 93},
+	{119, 94},
+	{120, 95},
+	{121, 96},
+	{122, 97},
+	{123, 98},
+	{124, 99},
+	{125, 100},
+	{127, 102},
+	{128, 103},
+	{129, 104},
+	{130, 105},
+	{132, 106},
+	{133, 107},
+	{145, 108},
+};
+
 static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
 	.pins = sdm845_pins,
 	.npins = ARRAY_SIZE(sdm845_pins),
@@ -1290,6 +1368,9 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
 	.groups = sdm845_groups,
 	.ngroups = ARRAY_SIZE(sdm845_groups),
 	.ngpios = 151,
+	.wakeirq_map = sdm845_pdc_map,
+	.nwakeirq_map = ARRAY_SIZE(sdm845_pdc_map),
+
 };
 
 static const struct msm_pinctrl_soc_data sdm845_acpi_pinctrl = {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC 12/14] arm64: dts: qcom: add PDC interrupt controller for SDM845

[Lina Iyer]

Add PDC interrupt controller device bindings for SDM845.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index be0022e09465..ffe28b3e41d8 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2375,6 +2375,16 @@
 			#power-domain-cells = <1>;
 		};
 
+		pdc_intc: interrupt-controller@b220000 {
+			compatible = "qcom,sdm845-pdc";
+			reg = <0 0x0b220000 0 0x30000>, <0x179900f0 0x60>;
+			qcom,pdc-ranges = <0 480 94>, <94 609 15>, <115 630 7>;
+			#interrupt-cells = <2>;
+			interrupt-parent = <&intc>;
+			interrupt-controller;
+			qcom,scm-spi-cfg;
+		};
+
 		pdc_reset: reset-controller@b2e0000 {
 			compatible = "qcom,sdm845-pdc-global";
 			reg = <0 0x0b2e0000 0 0x20000>;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC 13/14] arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845

[Lina Iyer]

PDC always-on interrupt controller can detect certain GPIOs even when
the TLMM interrupt controller is powered off. Link the PDC as TLMM's
wakeup parent.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index ffe28b3e41d8..3002793ee688 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1358,6 +1358,7 @@
 			interrupt-controller;
 			#interrupt-cells = <2>;
 			gpio-ranges = <&tlmm 0 0 150>;
+			wakeup-parent = <&pdc_intc>;
 
 			qspi_clk: qspi-clk {
 				pinmux {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC 14/14] arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845

[Lina Iyer]

Enable PDC interrupt controller for SDM845 devices. The interrupt
controller can detect wakeup capable interrupts when the SoC is in a low
power state.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 0e58ef02880c..310b6048054a 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -729,6 +729,7 @@ CONFIG_ARCH_R8A77970=y
 CONFIG_ARCH_R8A77980=y
 CONFIG_ARCH_R8A77990=y
 CONFIG_ARCH_R8A77995=y
+CONFIG_QCOM_PDC=y
 CONFIG_ROCKCHIP_PM_DOMAINS=y
 CONFIG_ARCH_TEGRA_132_SOC=y
 CONFIG_ARCH_TEGRA_210_SOC=y
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH RFC 03/14] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Marc Zyngier]

[Please use my kernel.org address in the future. The days of this
arm.com address are numbered...]

On 29/08/2019 19:11, Lina Iyer wrote:
> Introduce a new domain for wakeup capable GPIOs. The domain can be
> requested using the bus token DOMAIN_BUS_WAKEUP. In the following
> patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
> irqchip. Requesting a wakeup GPIO will setup the GPIO and the
> corresponding PDC interrupt as its parent.
> 
> Co-developed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  drivers/irqchip/qcom-pdc.c   | 104 ++++++++++++++++++++++++++++++++---
>  include/linux/soc/qcom/irq.h |  34 ++++++++++++
>  2 files changed, 129 insertions(+), 9 deletions(-)
>  create mode 100644 include/linux/soc/qcom/irq.h
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 338fae604af5..ad1faf634bcf 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -13,12 +13,13 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
> +#include <linux/soc/qcom/irq.h>
>  #include <linux/spinlock.h>
> -#include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  
>  #define PDC_MAX_IRQS		126
> +#define PDC_MAX_GPIO_IRQS	256
>  
>  #define CLEAR_INTR(reg, intr)	(reg & ~(1 << intr))
>  #define ENABLE_INTR(reg, intr)	(reg | (1 << intr))
> @@ -26,6 +27,8 @@
>  #define IRQ_ENABLE_BANK		0x10
>  #define IRQ_i_CFG		0x110
>  
> +#define PDC_NO_PARENT_IRQ	~0UL
> +
>  struct pdc_pin_region {
>  	u32 pin_base;
>  	u32 parent_base;
> @@ -65,23 +68,35 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
>  
>  static void qcom_pdc_gic_disable(struct irq_data *d)
>  {
> +	if (d->hwirq == GPIO_NO_WAKE_IRQ)
> +		return;
> +
>  	pdc_enable_intr(d, false);
>  	irq_chip_disable_parent(d);
>  }
>  
>  static void qcom_pdc_gic_enable(struct irq_data *d)
>  {
> +	if (d->hwirq == GPIO_NO_WAKE_IRQ)
> +		return;
> +
>  	pdc_enable_intr(d, true);
>  	irq_chip_enable_parent(d);
>  }
>  
>  static void qcom_pdc_gic_mask(struct irq_data *d)
>  {
> +	if (d->hwirq == GPIO_NO_WAKE_IRQ)
> +		return;
> +
>  	irq_chip_mask_parent(d);
>  }
>  
>  static void qcom_pdc_gic_unmask(struct irq_data *d)
>  {
> +	if (d->hwirq == GPIO_NO_WAKE_IRQ)
> +		return;
> +
>  	irq_chip_unmask_parent(d);
>  }
>  
> @@ -124,6 +139,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>  	int pin_out = d->hwirq;
>  	enum pdc_irq_config_bits pdc_type;
>  
> +	if (pin_out == GPIO_NO_WAKE_IRQ)
> +		return 0;
> +
>  	switch (type) {
>  	case IRQ_TYPE_EDGE_RISING:
>  		pdc_type = PDC_EDGE_RISING;
> @@ -181,8 +199,7 @@ static irq_hw_number_t get_parent_hwirq(int pin)
>  			return (region->parent_base + pin - region->pin_base);
>  	}
>  
> -	WARN_ON(1);
> -	return ~0UL;
> +	return PDC_NO_PARENT_IRQ;
>  }
>  
>  static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
> @@ -211,17 +228,17 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq,
>  
>  	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
>  	if (ret)
> -		return -EINVAL;
> -
> -	parent_hwirq = get_parent_hwirq(hwirq);
> -	if (parent_hwirq == ~0UL)
> -		return -EINVAL;
> +		return ret;
>  
>  	ret  = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
>  					     &qcom_pdc_gic_chip, NULL);
>  	if (ret)
>  		return ret;
>  
> +	parent_hwirq = get_parent_hwirq(hwirq);
> +	if (parent_hwirq == PDC_NO_PARENT_IRQ)
> +		return 0;
> +
>  	if (type & IRQ_TYPE_EDGE_BOTH)
>  		type = IRQ_TYPE_EDGE_RISING;
>  
> @@ -244,6 +261,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
>  	.free		= irq_domain_free_irqs_common,
>  };
>  
> +static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
> +			       unsigned int nr_irqs, void *data)
> +{
> +	struct irq_fwspec *fwspec = data;
> +	struct irq_fwspec parent_fwspec;
> +	irq_hw_number_t hwirq, parent_hwirq;
> +	unsigned int type;
> +	int ret;
> +
> +	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +					    &qcom_pdc_gic_chip, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (hwirq == GPIO_NO_WAKE_IRQ)
> +		return 0;
> +
> +	parent_hwirq = get_parent_hwirq(hwirq);
> +	if (parent_hwirq == PDC_NO_PARENT_IRQ)
> +		return 0;
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +
> +	parent_fwspec.fwnode      = domain->parent->fwnode;
> +	parent_fwspec.param_count = 3;
> +	parent_fwspec.param[0]    = 0;
> +	parent_fwspec.param[1]    = parent_hwirq;
> +	parent_fwspec.param[2]    = type;
> +
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> +					    &parent_fwspec);
> +}
> +
> +static int qcom_pdc_gpio_domain_select(struct irq_domain *d,
> +				       struct irq_fwspec *fwspec,
> +				       enum irq_domain_bus_token bus_token)
> +{
> +	return (bus_token == DOMAIN_BUS_WAKEUP);
> +}
> +
> +static const struct irq_domain_ops qcom_pdc_gpio_ops = {
> +	.select		= qcom_pdc_gpio_domain_select,
> +	.alloc		= qcom_pdc_gpio_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +};
> +
>  static int pdc_setup_pin_mapping(struct device_node *np)
>  {
>  	int ret, n;
> @@ -282,7 +353,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)
>  
>  static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>  {
> -	struct irq_domain *parent_domain, *pdc_domain;
> +	struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
>  	int ret;
>  
>  	pdc_base = of_iomap(node, 0);
> @@ -313,8 +384,23 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>  		goto fail;
>  	}
>  
> +	pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain,
> +						      IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP,
> +						      PDC_MAX_GPIO_IRQS,
> +						      of_fwnode_handle(node),
> +						      &qcom_pdc_gpio_ops, NULL);
> +	if (!pdc_gpio_domain) {
> +		pr_err("%pOF: GIC domain add failed for GPIO domain\n", node);
> +		ret = -ENOMEM;
> +		goto remove;
> +	}
> +
> +	irq_domain_update_bus_token(pdc_gpio_domain, DOMAIN_BUS_WAKEUP);
> +
>  	return 0;
>  
> +remove:
> +	irq_domain_remove(pdc_domain);
>  fail:
>  	kfree(pdc_region);
>  	iounmap(pdc_base);
> diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
> new file mode 100644
> index 000000000000..73239917dc38
> --- /dev/null
> +++ b/include/linux/soc/qcom/irq.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __QCOM_IRQ_H
> +#define __QCOM_IRQ_H
> +
> +#include <linux/irqdomain.h>
> +
> +#define GPIO_NO_WAKE_IRQ	~0U
> +
> +/**
> + * QCOM specific IRQ domain flags that distinguishes the handling of wakeup
> + * capable interrupts by different interrupt controllers.
> + *
> + * IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP: Line must be masked at TLMM and the
> + *                                  interrupt configuration is done at PDC
> + * IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP: Interrupt configuration is handled at TLMM
> + */
> +#define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP		(1 << 17)
> +#define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP		(1 << 18)

Any reason why you're starting at bit 17? The available range in from
bit 16... But overall, it would be better if you expressed it as:

#define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP	(IRQ_DOMAIN_FLAG_NONCORE << 0)
#define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 1)

> +
> +/**
> + * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt
> + *                                configuration
> + * @parent: irq domain
> + *
> + * This QCOM specific irq domain call returns if the interrupt controller
> + * requires the interrupt be masked at the child interrupt controller.
> + */
> +static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *parent)
> +{
> +	return (parent->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP);
> +}
> +
> +#endif
> 

But most of this file isn't used by this patch, so maybe it should be
moved somewhere else...

Thanks,

	M.
-- 
Jazz is not dead, it just smells funny...

Re: [PATCH RFC 03/14] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Lina Iyer]

On Fri, Aug 30 2019 at 08:50 -0600, Marc Zyngier wrote:
>[Please use my kernel.org address in the future. The days of this
>arm.com address are numbered...]
>
Sure, will update and repost.

>On 29/08/2019 19:11, Lina Iyer wrote:
>> Introduce a new domain for wakeup capable GPIOs. The domain can be
>> requested using the bus token DOMAIN_BUS_WAKEUP. In the following
>> patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
>> irqchip. Requesting a wakeup GPIO will setup the GPIO and the
>> corresponding PDC interrupt as its parent.
>>
>> Co-developed-by: Stephen Boyd <swboyd@chromium.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>  drivers/irqchip/qcom-pdc.c   | 104 ++++++++++++++++++++++++++++++++---
>>  include/linux/soc/qcom/irq.h |  34 ++++++++++++
>>  2 files changed, 129 insertions(+), 9 deletions(-)
>>  create mode 100644 include/linux/soc/qcom/irq.h
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 338fae604af5..ad1faf634bcf 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -13,12 +13,13 @@
>>  #include <linux/of.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_device.h>
>> +#include <linux/soc/qcom/irq.h>
>>  #include <linux/spinlock.h>
>> -#include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  #include <linux/types.h>
>>
>>  #define PDC_MAX_IRQS		126
>> +#define PDC_MAX_GPIO_IRQS	256
>>
>>  #define CLEAR_INTR(reg, intr)	(reg & ~(1 << intr))
>>  #define ENABLE_INTR(reg, intr)	(reg | (1 << intr))
>> @@ -26,6 +27,8 @@
>>  #define IRQ_ENABLE_BANK		0x10
>>  #define IRQ_i_CFG		0x110
>>
>> +#define PDC_NO_PARENT_IRQ	~0UL
>> +
>>  struct pdc_pin_region {
>>  	u32 pin_base;
>>  	u32 parent_base;
>> @@ -65,23 +68,35 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
>>
>>  static void qcom_pdc_gic_disable(struct irq_data *d)
>>  {
>> +	if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> +		return;
>> +
>>  	pdc_enable_intr(d, false);
>>  	irq_chip_disable_parent(d);
>>  }
>>
>>  static void qcom_pdc_gic_enable(struct irq_data *d)
>>  {
>> +	if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> +		return;
>> +
>>  	pdc_enable_intr(d, true);
>>  	irq_chip_enable_parent(d);
>>  }
>>
>>  static void qcom_pdc_gic_mask(struct irq_data *d)
>>  {
>> +	if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> +		return;
>> +
>>  	irq_chip_mask_parent(d);
>>  }
>>
>>  static void qcom_pdc_gic_unmask(struct irq_data *d)
>>  {
>> +	if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> +		return;
>> +
>>  	irq_chip_unmask_parent(d);
>>  }
>>
>> @@ -124,6 +139,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>>  	int pin_out = d->hwirq;
>>  	enum pdc_irq_config_bits pdc_type;
>>
>> +	if (pin_out == GPIO_NO_WAKE_IRQ)
>> +		return 0;
>> +
>>  	switch (type) {
>>  	case IRQ_TYPE_EDGE_RISING:
>>  		pdc_type = PDC_EDGE_RISING;
>> @@ -181,8 +199,7 @@ static irq_hw_number_t get_parent_hwirq(int pin)
>>  			return (region->parent_base + pin - region->pin_base);
>>  	}
>>
>> -	WARN_ON(1);
>> -	return ~0UL;
>> +	return PDC_NO_PARENT_IRQ;
>>  }
>>
>>  static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
>> @@ -211,17 +228,17 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq,
>>
>>  	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
>>  	if (ret)
>> -		return -EINVAL;
>> -
>> -	parent_hwirq = get_parent_hwirq(hwirq);
>> -	if (parent_hwirq == ~0UL)
>> -		return -EINVAL;
>> +		return ret;
>>
>>  	ret  = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
>>  					     &qcom_pdc_gic_chip, NULL);
>>  	if (ret)
>>  		return ret;
>>
>> +	parent_hwirq = get_parent_hwirq(hwirq);
>> +	if (parent_hwirq == PDC_NO_PARENT_IRQ)
>> +		return 0;
>> +
>>  	if (type & IRQ_TYPE_EDGE_BOTH)
>>  		type = IRQ_TYPE_EDGE_RISING;
>>
>> @@ -244,6 +261,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
>>  	.free		= irq_domain_free_irqs_common,
>>  };
>>
>> +static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
>> +			       unsigned int nr_irqs, void *data)
>> +{
>> +	struct irq_fwspec *fwspec = data;
>> +	struct irq_fwspec parent_fwspec;
>> +	irq_hw_number_t hwirq, parent_hwirq;
>> +	unsigned int type;
>> +	int ret;
>> +
>> +	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
>> +					    &qcom_pdc_gic_chip, NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (hwirq == GPIO_NO_WAKE_IRQ)
>> +		return 0;
>> +
>> +	parent_hwirq = get_parent_hwirq(hwirq);
>> +	if (parent_hwirq == PDC_NO_PARENT_IRQ)
>> +		return 0;
>> +
>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>> +		type = IRQ_TYPE_EDGE_RISING;
>> +
>> +	if (type & IRQ_TYPE_LEVEL_MASK)
>> +		type = IRQ_TYPE_LEVEL_HIGH;
>> +
>> +	parent_fwspec.fwnode      = domain->parent->fwnode;
>> +	parent_fwspec.param_count = 3;
>> +	parent_fwspec.param[0]    = 0;
>> +	parent_fwspec.param[1]    = parent_hwirq;
>> +	parent_fwspec.param[2]    = type;
>> +
>> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
>> +					    &parent_fwspec);
>> +}
>> +
>> +static int qcom_pdc_gpio_domain_select(struct irq_domain *d,
>> +				       struct irq_fwspec *fwspec,
>> +				       enum irq_domain_bus_token bus_token)
>> +{
>> +	return (bus_token == DOMAIN_BUS_WAKEUP);
>> +}
>> +
>> +static const struct irq_domain_ops qcom_pdc_gpio_ops = {
>> +	.select		= qcom_pdc_gpio_domain_select,
>> +	.alloc		= qcom_pdc_gpio_alloc,
>> +	.free		= irq_domain_free_irqs_common,
>> +};
>> +
>>  static int pdc_setup_pin_mapping(struct device_node *np)
>>  {
>>  	int ret, n;
>> @@ -282,7 +353,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)
>>
>>  static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>>  {
>> -	struct irq_domain *parent_domain, *pdc_domain;
>> +	struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
>>  	int ret;
>>
>>  	pdc_base = of_iomap(node, 0);
>> @@ -313,8 +384,23 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>>  		goto fail;
>>  	}
>>
>> +	pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain,
>> +						      IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP,
>> +						      PDC_MAX_GPIO_IRQS,
>> +						      of_fwnode_handle(node),
>> +						      &qcom_pdc_gpio_ops, NULL);
>> +	if (!pdc_gpio_domain) {
>> +		pr_err("%pOF: GIC domain add failed for GPIO domain\n", node);
>> +		ret = -ENOMEM;
>> +		goto remove;
>> +	}
>> +
>> +	irq_domain_update_bus_token(pdc_gpio_domain, DOMAIN_BUS_WAKEUP);
>> +
>>  	return 0;
>>
>> +remove:
>> +	irq_domain_remove(pdc_domain);
>>  fail:
>>  	kfree(pdc_region);
>>  	iounmap(pdc_base);
>> diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
>> new file mode 100644
>> index 000000000000..73239917dc38
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/irq.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef __QCOM_IRQ_H
>> +#define __QCOM_IRQ_H
>> +
>> +#include <linux/irqdomain.h>
>> +
>> +#define GPIO_NO_WAKE_IRQ	~0U
>> +
>> +/**
>> + * QCOM specific IRQ domain flags that distinguishes the handling of wakeup
>> + * capable interrupts by different interrupt controllers.
>> + *
>> + * IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP: Line must be masked at TLMM and the
>> + *                                  interrupt configuration is done at PDC
>> + * IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP: Interrupt configuration is handled at TLMM
>> + */
>> +#define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP		(1 << 17)
>> +#define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP		(1 << 18)
>
>Any reason why you're starting at bit 17? The available range in from
>bit 16... But overall, it would be better if you expressed it as:
>
>#define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP	(IRQ_DOMAIN_FLAG_NONCORE << 0)
>#define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 1)
>
Okay.

>> +
>> +/**
>> + * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt
>> + *                                configuration
>> + * @parent: irq domain
>> + *
>> + * This QCOM specific irq domain call returns if the interrupt controller
>> + * requires the interrupt be masked at the child interrupt controller.
>> + */
>> +static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *parent)
>> +{
>> +	return (parent->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP);
>> +}
>> +
>> +#endif
>>
>
>But most of this file isn't used by this patch, so maybe it should be
>moved somewhere else...
>
Apart from creating the domain, this is not used here, but a separate
patch seemed excessive. Let me know if you have any suggestions.

Thanks,
Lina

Re: [PATCH RFC 03/14] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Marc Zyngier]

On 30/08/2019 16:58, Lina Iyer wrote:
> On Fri, Aug 30 2019 at 08:50 -0600, Marc Zyngier wrote:
>> [Please use my kernel.org address in the future. The days of this
>> arm.com address are numbered...]
>>
> Sure, will update and repost.
> 
>> On 29/08/2019 19:11, Lina Iyer wrote:
>>> Introduce a new domain for wakeup capable GPIOs. The domain can be
>>> requested using the bus token DOMAIN_BUS_WAKEUP. In the following
>>> patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
>>> irqchip. Requesting a wakeup GPIO will setup the GPIO and the
>>> corresponding PDC interrupt as its parent.
>>>
>>> Co-developed-by: Stephen Boyd <swboyd@chromium.org>
>>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>> ---
>>>  drivers/irqchip/qcom-pdc.c   | 104 ++++++++++++++++++++++++++++++++---
>>>  include/linux/soc/qcom/irq.h |  34 ++++++++++++
>>>  2 files changed, 129 insertions(+), 9 deletions(-)
>>>  create mode 100644 include/linux/soc/qcom/irq.h
>>>

[...]

>>> diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
>>> new file mode 100644
>>> index 000000000000..73239917dc38
>>> --- /dev/null
>>> +++ b/include/linux/soc/qcom/irq.h
>>> @@ -0,0 +1,34 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#ifndef __QCOM_IRQ_H
>>> +#define __QCOM_IRQ_H
>>> +
>>> +#include <linux/irqdomain.h>
>>> +
>>> +#define GPIO_NO_WAKE_IRQ	~0U
>>> +
>>> +/**
>>> + * QCOM specific IRQ domain flags that distinguishes the handling of wakeup
>>> + * capable interrupts by different interrupt controllers.
>>> + *
>>> + * IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP: Line must be masked at TLMM and the
>>> + *                                  interrupt configuration is done at PDC
>>> + * IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP: Interrupt configuration is handled at TLMM
>>> + */
>>> +#define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP		(1 << 17)
>>> +#define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP		(1 << 18)
>>
>> Any reason why you're starting at bit 17? The available range in from
>> bit 16... But overall, it would be better if you expressed it as:
>>
>> #define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP	(IRQ_DOMAIN_FLAG_NONCORE << 0)
>> #define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 1)
>>
> Okay.
> 
>>> +
>>> +/**
>>> + * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt
>>> + *                                configuration
>>> + * @parent: irq domain
>>> + *
>>> + * This QCOM specific irq domain call returns if the interrupt controller
>>> + * requires the interrupt be masked at the child interrupt controller.
>>> + */
>>> +static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *parent)
>>> +{
>>> +	return (parent->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP);
>>> +}
>>> +
>>> +#endif
>>>
>>
>> But most of this file isn't used by this patch, so maybe it should be
>> moved somewhere else...
>>
> Apart from creating the domain, this is not used here, but a separate
> patch seemed excessive. Let me know if you have any suggestions.

My personal preference would be to move it into the patch that actually
makes use of this.

	M.
-- 
Jazz is not dead, it just smells funny...

Re: [PATCH RFC 04/14] of: irq: document properties for wakeup interrupt parent

[Rob Herring]

On Thu, 29 Aug 2019 12:11:53 -0600, Lina Iyer 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: devicetree@vger.kernel.org
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  .../bindings/interrupt-controller/interrupts.txt    | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register

[Rob Herring]

On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
> In addition to configuring the PDC, additional registers that interface
> the GIC have to be configured to match the GPIO type. The registers on
> some QCOM SoCs are access restricted, while on other SoCs are not. They
> SoCs with access restriction to these SPI registers need to be written

Took me a minute to figure out this is GIC SPI interrupts, not SPI bus.

> from the firmware using the SCM interface. Add a flag to indicate if the
> register is to be written using SCM interface.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  .../bindings/interrupt-controller/qcom,pdc.txt           | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> index 8e0797cb1487..852fcba98ea6 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> @@ -50,15 +50,22 @@ Properties:
>  		    The second element is the GIC hwirq number for the PDC port.
>  		    The third element is the number of interrupts in sequence.
>  
> +- qcom,scm-spi-cfg:
> +	Usage: optional
> +	Value type: <bool>
> +	Definition: Specifies if the SPI configuration registers have to be
> +		    written from the firmware.
> +
>  Example:
>  
>  	pdc: interrupt-controller@b220000 {
>  		compatible = "qcom,sdm845-pdc";
> -		reg = <0xb220000 0x30000>;
> +		reg = <0xb220000 0x30000>, <0x179900f0 0x60>;

There needs to be a description for reg updated. These aren't GIC 
registers are they? Because those go in the GIC node.

>  		qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
>  		#interrupt-cells = <2>;
>  		interrupt-parent = <&intc>;
>  		interrupt-controller;
> +		qcom,scm-spi-cfg;
>  	};
>  
>  DT binding of a device that wants to use the GIC SPI 514 as a wakeup
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register

[Marc Zyngier]

On 02/09/2019 14:38, Rob Herring wrote:
> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
>> In addition to configuring the PDC, additional registers that interface
>> the GIC have to be configured to match the GPIO type. The registers on
>> some QCOM SoCs are access restricted, while on other SoCs are not. They
>> SoCs with access restriction to these SPI registers need to be written
> 
> Took me a minute to figure out this is GIC SPI interrupts, not SPI bus.
> 
>> from the firmware using the SCM interface. Add a flag to indicate if the
>> register is to be written using SCM interface.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>  .../bindings/interrupt-controller/qcom,pdc.txt           | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>> index 8e0797cb1487..852fcba98ea6 100644
>> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>> @@ -50,15 +50,22 @@ Properties:
>>  		    The second element is the GIC hwirq number for the PDC port.
>>  		    The third element is the number of interrupts in sequence.
>>  
>> +- qcom,scm-spi-cfg:
>> +	Usage: optional
>> +	Value type: <bool>
>> +	Definition: Specifies if the SPI configuration registers have to be
>> +		    written from the firmware.
>> +
>>  Example:
>>  
>>  	pdc: interrupt-controller@b220000 {
>>  		compatible = "qcom,sdm845-pdc";
>> -		reg = <0xb220000 0x30000>;
>> +		reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
> 
> There needs to be a description for reg updated. These aren't GIC 
> registers are they? Because those go in the GIC node.

This is completely insane. Why are the GIC registers configured as
secure the first place, if they are expected to be in control of the
non-secure?

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register

[Lina Iyer]

On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
>On 02/09/2019 14:38, Rob Herring wrote:
>> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
>>> In addition to configuring the PDC, additional registers that interface
>>> the GIC have to be configured to match the GPIO type. The registers on
>>> some QCOM SoCs are access restricted, while on other SoCs are not. They
>>> SoCs with access restriction to these SPI registers need to be written
>>
>> Took me a minute to figure out this is GIC SPI interrupts, not SPI bus.
>>
>>> from the firmware using the SCM interface. Add a flag to indicate if the
>>> register is to be written using SCM interface.
>>>
>>> Cc: devicetree@vger.kernel.org
>>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>> ---
>>>  .../bindings/interrupt-controller/qcom,pdc.txt           | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>>> index 8e0797cb1487..852fcba98ea6 100644
>>> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>>> @@ -50,15 +50,22 @@ Properties:
>>>  		    The second element is the GIC hwirq number for the PDC port.
>>>  		    The third element is the number of interrupts in sequence.
>>>
>>> +- qcom,scm-spi-cfg:
>>> +	Usage: optional
>>> +	Value type: <bool>
>>> +	Definition: Specifies if the SPI configuration registers have to be
>>> +		    written from the firmware.
>>> +
>>>  Example:
>>>
>>>  	pdc: interrupt-controller@b220000 {
>>>  		compatible = "qcom,sdm845-pdc";
>>> -		reg = <0xb220000 0x30000>;
>>> +		reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
>>
>> There needs to be a description for reg updated. These aren't GIC
>> registers are they? Because those go in the GIC node.
>
They are not GIC registers. I will update this documentation.

>This is completely insane. Why are the GIC registers configured as
>secure the first place, if they are expected to be in control of the
>non-secure?
These are not GIC registers but located on the PDC interface to the GIC.
They may or may not be secure access controlled, depending on the SoC.

Thanks,
Lina

Re: [PATCH RFC 03/14] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Stephen Boyd]

Quoting Lina Iyer (2019-08-29 11:11:52)
> Introduce a new domain for wakeup capable GPIOs. The domain can be
> requested using the bus token DOMAIN_BUS_WAKEUP. In the following
> patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
> irqchip. Requesting a wakeup GPIO will setup the GPIO and the
> corresponding PDC interrupt as its parent.
> 
> Co-developed-by: Stephen Boyd <swboyd@chromium.org>

Per the Documentation about Co-developed-by this should have my 

Signed-off-by: Stephen Boyd <swboyd@chromium.org>

here. Please add it for the next version.

> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  drivers/irqchip/qcom-pdc.c   | 104 ++++++++++++++++++++++++++++++++---
>  include/linux/soc/qcom/irq.h |  34 ++++++++++++
>  2 files changed, 129 insertions(+), 9 deletions(-)
>  create mode 100644 include/linux/soc/qcom/irq.h
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 338fae604af5..ad1faf634bcf 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -244,6 +261,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
>         .free           = irq_domain_free_irqs_common,
>  };
>  
> +static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
> +                              unsigned int nr_irqs, void *data)
> +{
> +       struct irq_fwspec *fwspec = data;
> +       struct irq_fwspec parent_fwspec;
> +       irq_hw_number_t hwirq, parent_hwirq;
> +       unsigned int type;
> +       int ret;
> +
> +       ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
> +       if (ret)
> +               return ret;
> +
> +       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +                                           &qcom_pdc_gic_chip, NULL);
> +       if (ret)
> +               return ret;
> +
> +       if (hwirq == GPIO_NO_WAKE_IRQ)
> +               return 0;
> +
> +       parent_hwirq = get_parent_hwirq(hwirq);
> +       if (parent_hwirq == PDC_NO_PARENT_IRQ)
> +               return 0;
> +
> +       if (type & IRQ_TYPE_EDGE_BOTH)
> +               type = IRQ_TYPE_EDGE_RISING;
> +
> +       if (type & IRQ_TYPE_LEVEL_MASK)
> +               type = IRQ_TYPE_LEVEL_HIGH;
> +
> +       parent_fwspec.fwnode      = domain->parent->fwnode;
> +       parent_fwspec.param_count = 3;
> +       parent_fwspec.param[0]    = 0;
> +       parent_fwspec.param[1]    = parent_hwirq;
> +       parent_fwspec.param[2]    = type;
> +
> +       return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> +                                           &parent_fwspec);
> +}
> +
> +static int qcom_pdc_gpio_domain_select(struct irq_domain *d,
> +                                      struct irq_fwspec *fwspec,
> +                                      enum irq_domain_bus_token bus_token)
> +{
> +       return (bus_token == DOMAIN_BUS_WAKEUP);

Drop the parenthesis please.

> +}
> +
> +static const struct irq_domain_ops qcom_pdc_gpio_ops = {
> +       .select         = qcom_pdc_gpio_domain_select,
> +       .alloc          = qcom_pdc_gpio_alloc,
> +       .free           = irq_domain_free_irqs_common,
> +};
> +
>  static int pdc_setup_pin_mapping(struct device_node *np)
>  {
>         int ret, n;
> @@ -282,7 +353,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)
>  
>  static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>  {
> -       struct irq_domain *parent_domain, *pdc_domain;
> +       struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
>         int ret;
>  
>         pdc_base = of_iomap(node, 0);
> @@ -313,8 +384,23 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>                 goto fail;
>         }
>  
> +       pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain,
> +                                                     IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP,
> +                                                     PDC_MAX_GPIO_IRQS,
> +                                                     of_fwnode_handle(node),
> +                                                     &qcom_pdc_gpio_ops, NULL);
> +       if (!pdc_gpio_domain) {
> +               pr_err("%pOF: GIC domain add failed for GPIO domain\n", node);

s/GIC/PDC/?

> +               ret = -ENOMEM;
> +               goto remove;
> +       }
> +
> +       irq_domain_update_bus_token(pdc_gpio_domain, DOMAIN_BUS_WAKEUP);
> +
>         return 0;
>  
> +remove:
> +       irq_domain_remove(pdc_domain);
>  fail:
>         kfree(pdc_region);
>         iounmap(pdc_base);
> diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
> new file mode 100644
> index 000000000000..73239917dc38
> --- /dev/null
> +++ b/include/linux/soc/qcom/irq.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __QCOM_IRQ_H
> +#define __QCOM_IRQ_H
> +
> +#include <linux/irqdomain.h>

Just forward declare struct irq_domain instead?

> +
> +#define GPIO_NO_WAKE_IRQ       ~0U
> +
> +/**
> + * QCOM specific IRQ domain flags that distinguishes the handling of wakeup
> + * capable interrupts by different interrupt controllers.
> + *
> + * IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP: Line must be masked at TLMM and the
> + *                                  interrupt configuration is done at PDC
> + * IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP: Interrupt configuration is handled at TLMM
> + */
> +#define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP                (1 << 17)
> +#define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP                (1 << 18)

Why do these numbers start at 17?

> +
> +/**
> + * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt
> + *                                configuration
> + * @parent: irq domain
> + *
> + * This QCOM specific irq domain call returns if the interrupt controller
> + * requires the interrupt be masked at the child interrupt controller.
> + */
> +static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *parent)
> +{
> +       return (parent->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP);
> +}
> +
> +#endif
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register

[Stephen Boyd]

Quoting Lina Iyer (2019-09-03 10:07:22)
> On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
> >On 02/09/2019 14:38, Rob Herring wrote:
> >> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
> >>> In addition to configuring the PDC, additional registers that interface
> >>> the GIC have to be configured to match the GPIO type. The registers on
> >>> some QCOM SoCs are access restricted, while on other SoCs are not. They
> >>> SoCs with access restriction to these SPI registers need to be written
> >>
> >> Took me a minute to figure out this is GIC SPI interrupts, not SPI bus.
> >>
> >>> from the firmware using the SCM interface. Add a flag to indicate if the
> >>> register is to be written using SCM interface.
> >>>
> >>> Cc: devicetree@vger.kernel.org
> >>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> >>> ---
> >>>  .../bindings/interrupt-controller/qcom,pdc.txt           | 9 ++++++++-
> >>>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> >>> index 8e0797cb1487..852fcba98ea6 100644
> >>> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> >>> @@ -50,15 +50,22 @@ Properties:
> >>>                 The second element is the GIC hwirq number for the PDC port.
> >>>                 The third element is the number of interrupts in sequence.
> >>>
> >>> +- qcom,scm-spi-cfg:
> >>> +   Usage: optional
> >>> +   Value type: <bool>
> >>> +   Definition: Specifies if the SPI configuration registers have to be
> >>> +               written from the firmware.
> >>> +
> >>>  Example:
> >>>
> >>>     pdc: interrupt-controller@b220000 {
> >>>             compatible = "qcom,sdm845-pdc";
> >>> -           reg = <0xb220000 0x30000>;
> >>> +           reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
> >>
> >> There needs to be a description for reg updated. These aren't GIC
> >> registers are they? Because those go in the GIC node.
> >
> They are not GIC registers. I will update this documentation.
> 
> >This is completely insane. Why are the GIC registers configured as
> >secure the first place, if they are expected to be in control of the
> >non-secure?
> These are not GIC registers but located on the PDC interface to the GIC.
> They may or may not be secure access controlled, depending on the SoC.
> 

It looks like it falls under this "mailbox" device which is really the
catch all bucket for bits with no home besides they're related to the
apps CPUs/subsystem.

	apss_shared: mailbox@17990000 {
		compatible = "qcom,sdm845-apss-shared";
		reg = <0 0x17990000 0 0x1000>;
		#mbox-cells = <1>;
	};

Can you point to this node with a phandle and then parse the reg
property out of it to use in the scm readl/writel APIs? Maybe it can be
a two cell property with <&apps_shared 0xf0> to indicate the offset to
the registers to read/write? In non-secure mode presumably we need to
also write these registers? Good news is that there's a regmap for this
driver already, so maybe that can be acquired from the pdc driver.

Re: [PATCH RFC 08/14] drivers: irqchip: pdc: Add irqchip set/get state calls

[Stephen Boyd]

Quoting Lina Iyer (2019-08-29 11:11:57)
> From: Maulik Shah <mkshah@codeaurora.org>
> 
> Add irqchip calls to set/get interrupt status from the parent interrupt

s/status/state?

> controller.

Can you add some comment on why you want to do this? I'm looking for
something like, "Add this support so we can replay edge triggered
interrupts detected in suspend by the PDC to its interrupt parent
(typically the GIC)".

> 
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>

Re: [PATCH RFC 09/14] drivers: pinctrl: msm: fix use of deprecated gpiolib APIs

[Stephen Boyd]

The subject is misleading. It's not fixing anything, just moving away
from deprecated to "supported" APIs.

Quoting Lina Iyer (2019-08-29 11:11:58)
> Replace gpiochip_irqchip_add() and gpiochip_set_chained_irqchip() calls
> by populating the gpio_irq_chip data structures instead.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>

Re: [PATCH RFC 06/14] drivers: irqchip: pdc: additionally set type in SPI config registers

[Stephen Boyd]

Quoting Lina Iyer (2019-08-29 11:11:55)
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index ad1faf634bcf..bf5f98bb4d2b 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -100,6 +112,57 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
>         irq_chip_unmask_parent(d);
>  }
>  
> +static u32 __spi_pin_read(unsigned int pin)
> +{
> +       void __iomem *cfg_reg = spi_cfg->base + pin * 4;
> +       u64 scm_cfg_reg = spi_cfg->start + pin * 4;
> +
> +       if (spi_cfg->scm_io) {
> +               unsigned int val;
> +
> +               qcom_scm_io_readl(scm_cfg_reg, &val);
> +               return val;
> +       } else {
> +               return readl(cfg_reg);
> +       }

Please remove the else and just return readl() result instead.

> +}
> +
> +static void __spi_pin_write(unsigned int pin, unsigned int val)
> +{
> +       void __iomem *cfg_reg = spi_cfg->base + pin * 4;
> +       u64 scm_cfg_reg = spi_cfg->start + pin * 4;
> +
> +       if (spi_cfg->scm_io)
> +               qcom_scm_io_writel(scm_cfg_reg, val);
> +       else
> +               writel(val, cfg_reg);
> +}
> +
> +static int spi_configure_type(irq_hw_number_t hwirq, unsigned int type)
> +{
> +       int spi = hwirq - 32;
> +       u32 pin = spi / 32;
> +       u32 mask = BIT(spi % 32);
> +       u32 val;
> +       unsigned long flags;
> +
> +       if (!spi_cfg)
> +               return 0;
> +
> +       if (pin * 4 > spi_cfg->size)
> +               return -EFAULT;
> +
> +       raw_spin_lock_irqsave(&pdc_lock, flags);

Ah I don't think the regmap would use a raw spinlock, so that's another
hurdle to get over here.

> +       val = __spi_pin_read(pin);
> +       val &= ~mask;
> +       if (type & IRQ_TYPE_LEVEL_MASK)
> +               val |= mask;
> +       __spi_pin_write(pin, val);

Does monitoring level triggered interrupts matter? I'm asking if the
whole thing can be configured to monitor for edges regardless of trigger
type and then let the level handling be done by the GIC after the wakeup
or when the device is active.

> +       raw_spin_unlock_irqrestore(&pdc_lock, flags);
> +
> +       return 0;
> +}
> +
>  /*
>   * GIC does not handle falling edge or active low. To allow falling edge and
>   * active low interrupts to be handled at GIC, PDC has an inverter that inverts

Re: [PATCH RFC 11/14] drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs

[Stephen Boyd]

Quoting Lina Iyer (2019-08-29 11:12:00)
> diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> index 39f498c09906..5f43dabcd8eb 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> @@ -1282,6 +1282,84 @@ static const int sdm845_acpi_reserved_gpios[] = {
>         0, 1, 2, 3, 81, 82, 83, 84, -1
>  };
>  
> +static const struct msm_gpio_wakeirq_map sdm845_pdc_map[] = {
> +       {1, 30},

Please add spaces around the braces. Maybe you can have 5 per row? Would
make it a little more compact and still easy to confirm.

> +       {3, 31},
> +       {5, 32},
> +       {10, 33},

Re: [PATCH RFC 07/14] genirq: Introduce irq_chip_get/set_parent_state calls

[Stephen Boyd]

Quoting Lina Iyer (2019-08-29 11:11:56)
> From: Maulik Shah <mkshah@codeaurora.org>
> 
> On certain QTI chipsets some GPIOs are direct-connect interrupts
> to the GIC.
> 
> Even when GPIOs are not used for interrupt generation and interrupt
> line is disabled, it does not prevent interrupt to get pending at
> GIC_ISPEND. When drivers call enable_irq unwanted interrupt occures.

Inidicate functions with parenthesis like enable_irq().

> 
> Introduce irq_chip_get/set_parent_state calls to clear pending irq
> which can get called within irq_enable of child irq chip to clear
> any pending irq before enabling.

This sentence is hard to read.

> 
> index b76703b2c0af..6bb5b22bb0a7 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -1297,6 +1297,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
> + *
> + */
> +int irq_chip_set_parent_state(struct irq_data *data,
> +                             enum irqchip_irq_state which,
> +                             bool val)
> +{
> +       data = data->parent_data;
> +       if (!data)
> +               return 0;
> +
> +       if (data->chip->irq_set_irqchip_state)
> +               return data->chip->irq_set_irqchip_state(data, which, val);
> +
> +       return 0;

How about 

	if (!data || !data->chip->irq_set_irqchip_state)
		return 0;
	
	return data->chip->irq_set_irqchip_state(...)

> +}
> +EXPORT_SYMBOL(irq_chip_set_parent_state);
> +
> +/**
> + *     irq_chip_get_parent_state - get the state of a parent interrupt.

Why is this indented so much?

> + *     @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
> + *

Document return value?

> + */
> +int irq_chip_get_parent_state(struct irq_data *data,
> +                             enum irqchip_irq_state which,
> +                             bool *state)
> +{
> +       data = data->parent_data;
> +       if (!data)
> +               return 0;
> +
> +       if (data->chip->irq_get_irqchip_state)
> +               return data->chip->irq_get_irqchip_state(data, which, state);
> +

Same comment here about collapsing logic.

> +       return 0;
> +}
> +EXPORT_SYMBOL(irq_chip_get_parent_state);

Please make these symbols _GPL.

> +
>  /**
>   * irq_chip_enable_parent - Enable the parent interrupt (defaults to unmask if
>   * NULL)

Re: [PATCH RFC 02/14] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask

[Stephen Boyd]

Quoting Lina Iyer (2019-08-29 11:11:51)
> 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.

I think this is saying that we want to always let the line be sent
through the PDC to the parent irqchip, in this case GIC, so that we
don't get an interrupt storm for dual edge interrupts? Why does dual
edge interrupts cause a problem?

> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>

Re: [PATCH RFC 12/14] arm64: dts: qcom: add PDC interrupt controller for SDM845

[Maulik Shah]

On 8/29/2019 11:42 PM, Lina Iyer wrote:
> Add PDC interrupt controller device bindings for SDM845.
>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>   arch/arm64/boot/dts/qcom/sdm845.dtsi | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index be0022e09465..ffe28b3e41d8 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2375,6 +2375,16 @@
>   			#power-domain-cells = <1>;
>   		};
>   
> +		pdc_intc: interrupt-controller@b220000 {
> +			compatible = "qcom,sdm845-pdc";
> +			reg = <0 0x0b220000 0 0x30000>, <0x179900f0 0x60>;

second register also needs to be in below format

<0 0x179900f0 0 0x60>

> +			qcom,pdc-ranges = <0 480 94>, <94 609 15>, <115 630 7>;
> +			#interrupt-cells = <2>;
> +			interrupt-parent = <&intc>;
> +			interrupt-controller;
> +			qcom,scm-spi-cfg;
> +		};
> +
>   		pdc_reset: reset-controller@b2e0000 {
>   			compatible = "qcom,sdm845-pdc-global";
>   			reg = <0 0x0b2e0000 0 0x20000>;

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC 09/14] drivers: pinctrl: msm: fix use of deprecated gpiolib APIs

[Linus Walleij]

On Thu, Aug 29, 2019 at 7:35 PM Lina Iyer <ilina@codeaurora.org> wrote:

> Replace gpiochip_irqchip_add() and gpiochip_set_chained_irqchip() calls
> by populating the gpio_irq_chip data structures instead.
>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>

This is mostly fixed upstream I think, but:

> +       chip->irq.fwnode = pctrl->dev->fwnode;

This fwnode assignment is missing though.

Sorry for the constant churn and required rebasing...

Yours,
Linus Walleij

Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register

[Lina Iyer]

On Wed, Sep 11 2019 at 04:05 -0600, Linus Walleij wrote:
>On Thu, Aug 29, 2019 at 8:47 PM Lina Iyer <ilina@codeaurora.org> wrote:
>
>> +- qcom,scm-spi-cfg:
>> +       Usage: optional
>> +       Value type: <bool>
>> +       Definition: Specifies if the SPI configuration registers have to be
>> +                   written from the firmware.
>> +
>>  Example:
>>
>>         pdc: interrupt-controller@b220000 {
>>                 compatible = "qcom,sdm845-pdc";
>> -               reg = <0xb220000 0x30000>;
>> +               reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
>>                 qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
>>                 #interrupt-cells = <2>;
>>                 interrupt-parent = <&intc>;
>>                 interrupt-controller;
>> +               qcom,scm-spi-cfg;
>
>You can probably drop this bool if you just give names to the registers.
>
>Like
>reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
>reg-names = "gic", "pdc";
>
>Then jus check explicitly for a "pdc" register and in that case
>initialize the PDC.
>
Well the address remains the same. The bool defines how to access that
register address - from linux or from the firmware using SCM calls. But
I get your point, I could have different register namess - pdc-linux or
pdc-scm and request by name. I can then use that to determine the mode
for accessing the register.

Thanks,
Lina

Re: [PATCH RFC 02/14] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask

[Lina Iyer]

On Thu, Sep 05 2019 at 18:39 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-08-29 11:11:51)
>> 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.
>
>I think this is saying that we want to always let the line be sent
>through the PDC to the parent irqchip, in this case GIC, so that we
>don't get an interrupt storm for dual edge interrupts? Why does dual
>edge interrupts cause a problem?
>
I am not sure about the hardware details, but the PDC designers did not
expect enable and disable to be called whenever the interrupt is
handled. This specially becomes a problem for dual edge interrupts which
seems to generate a interrupt storm when enabled/disabled while handling
the interrupt.

--Lina

Re: [PATCH RFC 09/14] drivers: pinctrl: msm: fix use of deprecated gpiolib APIs

[Lina Iyer]

On Wed, Sep 11 2019 at 04:19 -0600, Linus Walleij wrote:
>On Thu, Aug 29, 2019 at 7:35 PM Lina Iyer <ilina@codeaurora.org> wrote:
>
>> Replace gpiochip_irqchip_add() and gpiochip_set_chained_irqchip() calls
>> by populating the gpio_irq_chip data structures instead.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>
>This is mostly fixed upstream I think, but:
>
>> +       chip->irq.fwnode = pctrl->dev->fwnode;
>
>This fwnode assignment is missing though.
>
>Sorry for the constant churn and required rebasing...
>
Not a problem. Will rebase.

Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register

[Lina Iyer]

Sorry, I couldn't get to this earlier.

On Thu, Sep 05 2019 at 18:03 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-09-03 10:07:22)
>> On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
>> >On 02/09/2019 14:38, Rob Herring wrote:
>> >> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
>> These are not GIC registers but located on the PDC interface to the GIC.
>> They may or may not be secure access controlled, depending on the SoC.
>>
>
>It looks like it falls under this "mailbox" device which is really the
>catch all bucket for bits with no home besides they're related to the
>apps CPUs/subsystem.
>
Thanks for pointing to this.
>	apss_shared: mailbox@17990000 {
>		compatible = "qcom,sdm845-apss-shared";
>		reg = <0 0x17990000 0 0x1000>;
But this doesn't seem correct. The registers in this page are all not
mailbox door bell registers. We should restrict the space allocated to
the mbox to 0xC or something, definitely, not the whole page. They all
cannot be treated as a mailbox registers.
>		#mbox-cells = <1>;
>	};
>
>Can you point to this node with a phandle and then parse the reg
>property out of it to use in the scm readl/writel APIs? Maybe it can be
>a two cell property with <&apps_shared 0xf0> to indicate the offset to
>the registers to read/write? In non-secure mode presumably we need to
>also write these registers? Good news is that there's a regmap for this
>driver already, so maybe that can be acquired from the pdc driver.
>
The register space collection seems to be mix of different types of
application processor registers that should probably not be grouped up
under one subsystem. A single regmap doesn't seem correct either.

-- Lina

Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register

[Lina Iyer]

Adding Sibi

On Fri, Sep 13 2019 at 13:53 -0600, Lina Iyer wrote:
>Sorry, I couldn't get to this earlier.
>
>On Thu, Sep 05 2019 at 18:03 -0600, Stephen Boyd wrote:
>>Quoting Lina Iyer (2019-09-03 10:07:22)
>>>On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
>>>>On 02/09/2019 14:38, Rob Herring wrote:
>>>>> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
>>>These are not GIC registers but located on the PDC interface to the GIC.
>>>They may or may not be secure access controlled, depending on the SoC.
>>>
>>
>>It looks like it falls under this "mailbox" device which is really the
>>catch all bucket for bits with no home besides they're related to the
>>apps CPUs/subsystem.
>>
>Thanks for pointing to this.
>>	apss_shared: mailbox@17990000 {
>>		compatible = "qcom,sdm845-apss-shared";
>>		reg = <0 0x17990000 0 0x1000>;
>But this doesn't seem correct. The registers in this page are all not
>mailbox door bell registers. We should restrict the space allocated to
>the mbox to 0xC or something, definitely, not the whole page. They all
>cannot be treated as a mailbox registers.
>>		#mbox-cells = <1>;
>>	};
>>
>>Can you point to this node with a phandle and then parse the reg
>>property out of it to use in the scm readl/writel APIs? Maybe it can be
>>a two cell property with <&apps_shared 0xf0> to indicate the offset to
>>the registers to read/write? In non-secure mode presumably we need to
>>also write these registers? Good news is that there's a regmap for this
>>driver already, so maybe that can be acquired from the pdc driver.
>>
>The register space collection seems to be mix of different types of
>application processor registers that should probably not be grouped up
>under one subsystem. A single regmap doesn't seem correct either.
>
>-- Lina

Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register

[Stephen Boyd]

Quoting Lina Iyer (2019-09-17 14:50:20)
> On Fri, Sep 13 2019 at 13:53 -0600, Lina Iyer wrote:
> >On Thu, Sep 05 2019 at 18:03 -0600, Stephen Boyd wrote:
> >>Quoting Lina Iyer (2019-09-03 10:07:22)
> >>>On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
> >>>>On 02/09/2019 14:38, Rob Herring wrote:
> >>>>> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
> >>>These are not GIC registers but located on the PDC interface to the GIC.
> >>>They may or may not be secure access controlled, depending on the SoC.
> >>>
> >>
> >>It looks like it falls under this "mailbox" device which is really the
> >>catch all bucket for bits with no home besides they're related to the
> >>apps CPUs/subsystem.
> >>
> >Thanks for pointing to this.
> >>      apss_shared: mailbox@17990000 {
> >>              compatible = "qcom,sdm845-apss-shared";
> >>              reg = <0 0x17990000 0 0x1000>;
> >But this doesn't seem correct. The registers in this page are all not
> >mailbox door bell registers. We should restrict the space allocated to
> >the mbox to 0xC or something, definitely, not the whole page. They all
> >cannot be treated as a mailbox registers.

Well the binding is already done and this is the compatible string for
this node and register region. Sounds like this node is a mailbox plus
some more stuff in the same page.

> >>              #mbox-cells = <1>;
> >>      };
> >>
> >>Can you point to this node with a phandle and then parse the reg
> >>property out of it to use in the scm readl/writel APIs? Maybe it can be
> >>a two cell property with <&apps_shared 0xf0> to indicate the offset to
> >>the registers to read/write? In non-secure mode presumably we need to
> >>also write these registers? Good news is that there's a regmap for this
> >>driver already, so maybe that can be acquired from the pdc driver.
> >>
> >The register space collection seems to be mix of different types of
> >application processor registers that should probably not be grouped up
> >under one subsystem. A single regmap doesn't seem correct either.

Why isn't a single regmap correct? The PDC driver should be able to use
it to read/write into this register space. The lock on the regmap will
need to be changed to a raw lock though for RT. Otherwise it looks OK to
me.

Re: [PATCH RFC 02/14] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask

[Stephen Boyd]

Quoting Lina Iyer (2019-09-11 09:15:57)
> On Thu, Sep 05 2019 at 18:39 -0600, Stephen Boyd wrote:
> >Quoting Lina Iyer (2019-08-29 11:11:51)
> >> 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.
> >
> >I think this is saying that we want to always let the line be sent
> >through the PDC to the parent irqchip, in this case GIC, so that we
> >don't get an interrupt storm for dual edge interrupts? Why does dual
> >edge interrupts cause a problem?
> >
> I am not sure about the hardware details, but the PDC designers did not
> expect enable and disable to be called whenever the interrupt is
> handled. This specially becomes a problem for dual edge interrupts which
> seems to generate a interrupt storm when enabled/disabled while handling
> the interrupt.
> 

Ok. I just wanted to confirm that masking "doesn't matter" to the PDC
because it assumes the irqchip closer to the CPU will be able to mask it
anyway. Is that right?

Re: [PATCH RFC 02/14] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask

[Lina Iyer]

On Fri, Sep 20 2019 at 16:22 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-09-11 09:15:57)
>> On Thu, Sep 05 2019 at 18:39 -0600, Stephen Boyd wrote:
>> >Quoting Lina Iyer (2019-08-29 11:11:51)
>> >> 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.
>> >
>> >I think this is saying that we want to always let the line be sent
>> >through the PDC to the parent irqchip, in this case GIC, so that we
>> >don't get an interrupt storm for dual edge interrupts? Why does dual
>> >edge interrupts cause a problem?
>> >
>> I am not sure about the hardware details, but the PDC designers did not
>> expect enable and disable to be called whenever the interrupt is
>> handled. This specially becomes a problem for dual edge interrupts which
>> seems to generate a interrupt storm when enabled/disabled while handling
>> the interrupt.
>>
>
>Ok. I just wanted to confirm that masking "doesn't matter" to the PDC
>because it assumes the irqchip closer to the CPU will be able to mask it
>anyway. Is that right?
>
That is correct.

Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register

[Sibi Sankar]

On 2019-09-21 03:50, Stephen Boyd wrote:
> Quoting Lina Iyer (2019-09-17 14:50:20)
>> On Fri, Sep 13 2019 at 13:53 -0600, Lina Iyer wrote:
>> >On Thu, Sep 05 2019 at 18:03 -0600, Stephen Boyd wrote:
>> >>Quoting Lina Iyer (2019-09-03 10:07:22)
>> >>>On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
>> >>>>On 02/09/2019 14:38, Rob Herring wrote:
>> >>>>> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
>> >>>These are not GIC registers but located on the PDC interface to the GIC.
>> >>>They may or may not be secure access controlled, depending on the SoC.
>> >>>
>> >>
>> >>It looks like it falls under this "mailbox" device which is really the
>> >>catch all bucket for bits with no home besides they're related to the
>> >>apps CPUs/subsystem.
>> >>
>> >Thanks for pointing to this.
>> >>      apss_shared: mailbox@17990000 {
>> >>              compatible = "qcom,sdm845-apss-shared";
>> >>              reg = <0 0x17990000 0 0x1000>;
>> >But this doesn't seem correct. The registers in this page are all not
>> >mailbox door bell registers. We should restrict the space allocated to
>> >the mbox to 0xC or something, definitely, not the whole page. They all
>> >cannot be treated as a mailbox registers.
> 
> Well the binding is already done and this is the compatible string for
> this node and register region. Sounds like this node is a mailbox plus
> some more stuff in the same page.
> 

Bjorn already noticed ^^ during the
original review. Hence the compatible
was correctly named "apss-shared"
instead of following the older bindings.

>> >>              #mbox-cells = <1>;
>> >>      };
>> >>
>> >>Can you point to this node with a phandle and then parse the reg
>> >>property out of it to use in the scm readl/writel APIs? Maybe it can be
>> >>a two cell property with <&apps_shared 0xf0> to indicate the offset to
>> >>the registers to read/write? In non-secure mode presumably we need to
>> >>also write these registers? Good news is that there's a regmap for this
>> >>driver already, so maybe that can be acquired from the pdc driver.
>> >>
>> >The register space collection seems to be mix of different types of
>> >application processor registers that should probably not be grouped up
>> >under one subsystem. A single regmap doesn't seem correct either.
> 
> Why isn't a single regmap correct? The PDC driver should be able to use
> it to read/write into this register space. The lock on the regmap will
> need to be changed to a raw lock though for RT. Otherwise it looks OK 
> to
> me.

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.