[PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Lina Iyer]

On newer Qualcomm Techonologies Inc's SoCs like the SDM845, the GIC is in a
power domain that can be powered off when not needed. Interrupts that need to
be sensed even when the GIC is powered off, are routed through an interrupt
controller in an always-on domain called the Power Domain Controller a.k.a PDC.
This series adds support for the PDC's interrupt controller.

Please consider reviewing these patches.

Lina Iyer (4):
  drivers: irqchip: pdc: add support for PDC interrupt controller
  dt-bindings/interrupt-controller: pdc: descibe PDC device binding
  drivers: irqchip: pdc: log PDC info in FTRACE
  drivers: irqchip: qcom: add pin information for SDM845

 .../bindings/interrupt-controller/qcom,pdc.txt     |  55 ++++
 drivers/irqchip/Kconfig                            |   9 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/qcom-pdc-sdm845.c                  | 130 +++++++++
 drivers/irqchip/qcom-pdc.c                         | 291 +++++++++++++++++++++
 drivers/irqchip/qcom-pdc.h                         |  31 +++
 include/trace/events/pdc.h                         |  50 ++++
 7 files changed, 567 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
 create mode 100644 drivers/irqchip/qcom-pdc-sdm845.c
 create mode 100644 drivers/irqchip/qcom-pdc.c
 create mode 100644 drivers/irqchip/qcom-pdc.h
 create mode 100644 include/trace/events/pdc.h

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

[PATCH RFC 1/4] drivers: irqchip: pdc: add support for PDC interrupt controller

[Lina Iyer]

>From : Archana Sathyakumar <asathyak@codeaurora.org>

The Power Domain Controller (PDC) hardware block on Qualcomm SoCs houses
an interrupt controller along with other domain control functions to
handle interrupt related functions like handle falling edge or active
low which are not detected at the GIC and handle wakeup interrupts.

The interrupt controller is on an always-on domain for the purpose of
waking up the processor, but only a subset of the processor's interrupts
are routed through the PDC to the GIC. The PDC powers on the processor's
domain, bringing the domain out of low power mode and replays the
pending interrupts so the GIC may wake up the processor.

Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
[Lina: Split out DT bindings target data and initialization changes]
---
 drivers/irqchip/Kconfig    |   9 ++
 drivers/irqchip/Makefile   |   1 +
 drivers/irqchip/qcom-pdc.c | 282 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/irqchip/qcom-pdc.h |  30 +++++
 4 files changed, 322 insertions(+)
 create mode 100644 drivers/irqchip/qcom-pdc.c
 create mode 100644 drivers/irqchip/qcom-pdc.h

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index c70476b34a53..9d54151157d5 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -343,4 +343,13 @@ config MESON_IRQ_GPIO
        help
          Support Meson SoC Family GPIO Interrupt Multiplexer
 
+config QCOM_PDC
+	bool "QCOM PDC"
+	depends on ARCH_QCOM
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Power Domain Controller driver to manage and configure wakeup
+	  IRQs for Qualcomm Technologies Inc.'s (QTI) mobile SoCs.
+
 endmenu
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index d2df34a54d38..280723d83916 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -84,3 +84,4 @@ obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
 obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o
 obj-$(CONFIG_ARCH_SYNQUACER)		+= irq-sni-exiu.o
 obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
+obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
new file mode 100644
index 000000000000..57f1348bd81c
--- /dev/null
+++ b/drivers/irqchip/qcom-pdc.c
@@ -0,0 +1,282 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/spinlock.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "qcom-pdc.h"
+
+#define PDC_MAX_IRQS			126
+
+#define CLEAR_INTR(reg, intr)		(reg & ~(1 << intr))
+#define ENABLE_INTR(reg, intr)		(reg | (1 << intr))
+
+#define IRQ_ENABLE_BANK			0x10
+#define IRQ_i_CFG			0x110
+
+static DEFINE_SPINLOCK(pdc_lock);
+static void __iomem *pdc_base;
+
+static int get_pdc_pin(irq_hw_number_t hwirq, void *data)
+{
+	int i;
+	struct pdc_pin *pdc_data = (struct pdc_pin *) data;
+
+	for (i = 0; pdc_data[i].pin >= 0; i++) {
+		if (pdc_data[i].hwirq == hwirq)
+			return pdc_data[i].pin;
+	}
+
+	return -EINVAL;
+}
+
+static inline void pdc_enable_intr(struct irq_data *d, bool on)
+{
+	int pin_out = get_pdc_pin(d->hwirq, d->chip_data);
+	unsigned int index, mask;
+	u32 enable, r_enable;
+	unsigned long flags;
+
+	if (pin_out < 0)
+		return;
+
+	index = pin_out / 32;
+	mask = pin_out % 32;
+	spin_lock_irqsave(&pdc_lock, flags);
+	enable = readl_relaxed(pdc_base + IRQ_ENABLE_BANK + (index *
+					sizeof(uint32_t)));
+	enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
+	writel_relaxed(enable, pdc_base + IRQ_ENABLE_BANK + (index *
+						sizeof(uint32_t)));
+	do {
+		r_enable = readl_relaxed(pdc_base + IRQ_ENABLE_BANK +
+					(index * sizeof(uint32_t)));
+		if (r_enable == enable)
+			break;
+		udelay(5);
+	} while (1);
+	spin_unlock_irqrestore(&pdc_lock, flags);
+	WARN_ON(r_enable != enable);
+}
+
+static void qcom_pdc_gic_mask(struct irq_data *d)
+{
+	pdc_enable_intr(d, false);
+	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);
+}
+
+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_disable(struct irq_data *d)
+{
+	pdc_enable_intr(d, false);
+	irq_chip_disable_parent(d);
+}
+
+/*
+ * 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
+ * falling edge into a rising edge and active low into an active high.
+ * For the inverter to work, the polarity bit in the IRQ_CONFIG register has to
+ * set as per the table below.
+ * (polarity, falling edge, rising edge )  ORIG          POL CONV     POLARITY
+ * 3'b0 00  Level sensitive active low    (~~~|_____)   (___|~~~~~)   LOW
+ * 3'b0 10  Falling edge sensitive        (~~~|__|~~)   (___|~~|__)   LOW
+ * 3'b1 00  Level senstive active High    (___|~~~~~)   (___|~~~~~)   HIGH
+ * 3'b1 10  Rising edge sensitive         (___|~~|__)   (___|~~|__)   HIGH
+ */
+enum pdc_irq_config_bits {
+	POLARITY_LOW	= 0, // b000
+	FALLING_EDGE	= 2, // b010
+	POLARITY_HIGH	= 4, // b100
+	RISING_EDGE	= 6, // b110
+};
+
+static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
+{
+	int pin_out = get_pdc_pin(d->hwirq, d->chip_data);
+	u32 pdc_type = 0, config;
+
+	if (pin_out < 0)
+		goto fwd_to_parent;
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		pdc_type = RISING_EDGE;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		pdc_type = FALLING_EDGE;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		pdc_type = POLARITY_HIGH;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		pdc_type = POLARITY_LOW;
+		break;
+	default:
+		pdc_type = POLARITY_HIGH;
+		break;
+	}
+	writel_relaxed(pdc_type, pdc_base + IRQ_i_CFG +
+			(pin_out * sizeof(uint32_t)));
+
+	do {
+		config = readl_relaxed(pdc_base + IRQ_i_CFG +
+				(pin_out * sizeof(uint32_t)));
+		if (config == pdc_type)
+			break;
+		udelay(5);
+	} while (1);
+
+	/*
+	 * If type is edge triggered, forward that as Rising edge as PDC
+	 * takes care of converting falling edge to rising edge signal
+	 */
+	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
+		type = IRQ_TYPE_EDGE_RISING;
+
+	/*
+	 * If type is level, then forward that as level high as PDC
+	 * takes care of converting falling edge to rising edge signal
+	 */
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
+		type = IRQ_TYPE_LEVEL_HIGH;
+
+fwd_to_parent:
+	return irq_chip_set_type_parent(d, type);
+}
+
+static struct irq_chip qcom_pdc_gic_chip = {
+	.name			= "pdc-gic",
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_mask		= qcom_pdc_gic_mask,
+	.irq_enable		= qcom_pdc_gic_enable,
+	.irq_unmask		= qcom_pdc_gic_unmask,
+	.irq_disable		= qcom_pdc_gic_disable,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_set_type		= qcom_pdc_gic_set_type,
+	.flags			= IRQCHIP_MASK_ON_SUSPEND |
+				  IRQCHIP_SET_TYPE_MASKED |
+				  IRQCHIP_SKIP_SET_WAKE,
+	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,
+#ifdef CONFIG_SMP
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+#endif
+};
+
+static int qcom_pdc_translate(struct irq_domain *d,
+	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
+{
+	return d->parent->ops->translate(d->parent, fwspec, hwirq, type);
+}
+
+static int qcom_pdc_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;
+	int i;
+	unsigned int type;
+	int ret;
+
+	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return -EINVAL;
+
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+				&qcom_pdc_gic_chip, domain->host_data);
+
+	parent_fwspec = *fwspec;
+	parent_fwspec.fwnode = domain->parent->fwnode;
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+						&parent_fwspec);
+}
+
+static const struct irq_domain_ops qcom_pdc_ops = {
+	.translate	= qcom_pdc_translate,
+	.alloc		= qcom_pdc_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
+static const struct of_device_id pdc_table[] = {
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pdc_table);
+
+int qcom_pdc_init(struct device_node *node, struct device_node *parent)
+{
+	struct irq_domain *parent_domain,  *pdc_domain;
+	const struct pdc_pin *data;
+	const struct of_device_id *id;
+	int ret;
+
+	id = of_match_node(pdc_table, node);
+	if (id)
+		data = id->data;
+	else
+		return -ENODEV;
+
+	pdc_base = of_iomap(node, 0);
+	if (!pdc_base) {
+		pr_err("%s(): unable to map PDC registers\n", node->full_name);
+		return -ENXIO;
+	}
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		pr_err("unable to obtain PDC parent domain\n");
+		ret = -ENXIO;
+		goto failure;
+	}
+
+	pdc_domain = irq_domain_add_hierarchy(parent_domain, 0, PDC_MAX_IRQS,
+					node, &qcom_pdc_ops, (void *)data);
+	if (!pdc_domain) {
+		pr_err("GIC domain add failed\n");
+		ret = -ENOMEM;
+		goto failure;
+	}
+
+	pdc_domain->name = "qcom,pdc";
+	return 0;
+
+failure:
+	iounmap(pdc_base);
+	return ret;
+}
diff --git a/drivers/irqchip/qcom-pdc.h b/drivers/irqchip/qcom-pdc.h
new file mode 100644
index 000000000000..b5b64390175e
--- /dev/null
+++ b/drivers/irqchip/qcom-pdc.h
@@ -0,0 +1,30 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __QCOM_PDC_H__
+#define __QCOM_PDC_H__
+
+#include <linux/irq.h>
+
+/**
+ * pdc_pin: Mapping of PDC port to hwirq
+ *
+ * @pin: PDC port for the IRQ
+ * @hwirq: hw IRQ number
+ */
+struct pdc_pin {
+	int pin;
+	irq_hw_number_t hwirq;
+};
+
+#endif /* __QCOM_PDC_H__ */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH RFC 1/4] drivers: irqchip: pdc: add support for PDC interrupt controller

[Marc Zyngier]

Hi Lina, Archana,

On 23/01/18 17:56, Lina Iyer wrote:
> From : Archana Sathyakumar <asathyak@codeaurora.org>
> 
> The Power Domain Controller (PDC) hardware block on Qualcomm SoCs houses
> an interrupt controller along with other domain control functions to
> handle interrupt related functions like handle falling edge or active
> low which are not detected at the GIC and handle wakeup interrupts.
> 
> The interrupt controller is on an always-on domain for the purpose of
> waking up the processor, but only a subset of the processor's interrupts
> are routed through the PDC to the GIC. The PDC powers on the processor's
> domain, bringing the domain out of low power mode and replays the
> pending interrupts so the GIC may wake up the processor.
> 
> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> [Lina: Split out DT bindings target data and initialization changes]
> ---
>  drivers/irqchip/Kconfig    |   9 ++
>  drivers/irqchip/Makefile   |   1 +
>  drivers/irqchip/qcom-pdc.c | 282 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/irqchip/qcom-pdc.h |  30 +++++
>  4 files changed, 322 insertions(+)
>  create mode 100644 drivers/irqchip/qcom-pdc.c
>  create mode 100644 drivers/irqchip/qcom-pdc.h
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index c70476b34a53..9d54151157d5 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -343,4 +343,13 @@ config MESON_IRQ_GPIO
>         help
>           Support Meson SoC Family GPIO Interrupt Multiplexer
>  
> +config QCOM_PDC
> +	bool "QCOM PDC"
> +	depends on ARCH_QCOM
> +	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  Power Domain Controller driver to manage and configure wakeup
> +	  IRQs for Qualcomm Technologies Inc.'s (QTI) mobile SoCs.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index d2df34a54d38..280723d83916 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -84,3 +84,4 @@ obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>  obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o
>  obj-$(CONFIG_ARCH_SYNQUACER)		+= irq-sni-exiu.o
>  obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
> +obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> new file mode 100644
> index 000000000000..57f1348bd81c
> --- /dev/null
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -0,0 +1,282 @@
> +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include "qcom-pdc.h"
> +
> +#define PDC_MAX_IRQS			126
> +
> +#define CLEAR_INTR(reg, intr)		(reg & ~(1 << intr))
> +#define ENABLE_INTR(reg, intr)		(reg | (1 << intr))
> +
> +#define IRQ_ENABLE_BANK			0x10
> +#define IRQ_i_CFG			0x110
> +
> +static DEFINE_SPINLOCK(pdc_lock);
> +static void __iomem *pdc_base;
> +
> +static int get_pdc_pin(irq_hw_number_t hwirq, void *data)
> +{
> +	int i;
> +	struct pdc_pin *pdc_data = (struct pdc_pin *) data;
> +
> +	for (i = 0; pdc_data[i].pin >= 0; i++) {
> +		if (pdc_data[i].hwirq == hwirq)
> +			return pdc_data[i].pin;
> +	}
> +
> +	return -EINVAL;
> +}

That's a total NAK. See my reply to patch #4.

> +
> +static inline void pdc_enable_intr(struct irq_data *d, bool on)
> +{
> +	int pin_out = get_pdc_pin(d->hwirq, d->chip_data);
> +	unsigned int index, mask;
> +	u32 enable, r_enable;
> +	unsigned long flags;
> +
> +	if (pin_out < 0)
> +		return;
> +
> +	index = pin_out / 32;
> +	mask = pin_out % 32;
> +	spin_lock_irqsave(&pdc_lock, flags);
> +	enable = readl_relaxed(pdc_base + IRQ_ENABLE_BANK + (index *
> +					sizeof(uint32_t)));
> +	enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
> +	writel_relaxed(enable, pdc_base + IRQ_ENABLE_BANK + (index *
> +						sizeof(uint32_t)));
> +	do {
> +		r_enable = readl_relaxed(pdc_base + IRQ_ENABLE_BANK +
> +					(index * sizeof(uint32_t)));

Consider either using an intermediate variable or an accessor for this
"pdc_base + IRQ... + ...".

> +		if (r_enable == enable)
> +			break;
> +		udelay(5);
> +	} while (1);
> +	spin_unlock_irqrestore(&pdc_lock, flags);

Ouch. this feels very heavy handed. How about placing the polling side
outside of the spinlock section, only testing the affected bit instead?
And maybe a timeout just in case the HW is getting wedged.

> +	WARN_ON(r_enable != enable);
> +}
> +
> +static void qcom_pdc_gic_mask(struct irq_data *d)
> +{
> +	pdc_enable_intr(d, false);
> +	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);
> +}
> +
> +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_disable(struct irq_data *d)
> +{
> +	pdc_enable_intr(d, false);
> +	irq_chip_disable_parent(d);
> +}

Why do you have to provide both {en,dis}able and {un,}mask? The GIC only
has {un,}mask, and given that this is pretty tied to it, you should
probably stick with what it implements.

> +
> +/*
> + * 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
> + * falling edge into a rising edge and active low into an active high.
> + * For the inverter to work, the polarity bit in the IRQ_CONFIG register has to
> + * set as per the table below.
> + * (polarity, falling edge, rising edge )  ORIG          POL CONV     POLARITY
> + * 3'b0 00  Level sensitive active low    (~~~|_____)   (___|~~~~~)   LOW
> + * 3'b0 10  Falling edge sensitive        (~~~|__|~~)   (___|~~|__)   LOW
> + * 3'b1 00  Level senstive active High    (___|~~~~~)   (___|~~~~~)   HIGH
> + * 3'b1 10  Rising edge sensitive         (___|~~|__)   (___|~~|__)   HIGH

I'm not sure about the ASCII art. I'd rather see a table that outlines
the use of each bit individually (and potentially document bit 0 which
seems to be unused).

> + */
> +enum pdc_irq_config_bits {
> +	POLARITY_LOW	= 0, // b000
> +	FALLING_EDGE	= 2, // b010
> +	POLARITY_HIGH	= 4, // b100
> +	RISING_EDGE	= 6, // b110

Consider prefixing those with PDC_

> +};
> +
> +static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
> +{
> +	int pin_out = get_pdc_pin(d->hwirq, d->chip_data);
> +	u32 pdc_type = 0, config;
> +
> +	if (pin_out < 0)
> +		goto fwd_to_parent;

How can that even happen?

> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		pdc_type = RISING_EDGE;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		pdc_type = FALLING_EDGE;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		pdc_type = POLARITY_HIGH;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		pdc_type = POLARITY_LOW;
> +		break;
> +	default:
> +		pdc_type = POLARITY_HIGH;
> +		break;

If I say "IRQ_TYPE_EDGE_BOTH", I get level high? Probably not what you want.

> +	}
> +	writel_relaxed(pdc_type, pdc_base + IRQ_i_CFG +
> +			(pin_out * sizeof(uint32_t)));
> +
> +	do {
> +		config = readl_relaxed(pdc_base + IRQ_i_CFG +
> +				(pin_out * sizeof(uint32_t)));
> +		if (config == pdc_type)
> +			break;
> +		udelay(5);
> +	} while (1);

Same remark about the timeout.

> +
> +	/*
> +	 * If type is edge triggered, forward that as Rising edge as PDC
> +	 * takes care of converting falling edge to rising edge signal
> +	 */
> +	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	/*
> +	 * If type is level, then forward that as level high as PDC
> +	 * takes care of converting falling edge to rising edge signal
> +	 */
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> +		type = IRQ_TYPE_LEVEL_HIGH;

You can fold that into the switch statement above.

> +
> +fwd_to_parent:
> +	return irq_chip_set_type_parent(d, type);
> +}
> +
> +static struct irq_chip qcom_pdc_gic_chip = {
> +	.name			= "pdc-gic",
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_mask		= qcom_pdc_gic_mask,
> +	.irq_enable		= qcom_pdc_gic_enable,
> +	.irq_unmask		= qcom_pdc_gic_unmask,
> +	.irq_disable		= qcom_pdc_gic_disable,
> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> +	.irq_set_type		= qcom_pdc_gic_set_type,
> +	.flags			= IRQCHIP_MASK_ON_SUSPEND |
> +				  IRQCHIP_SET_TYPE_MASKED |
> +				  IRQCHIP_SKIP_SET_WAKE,
> +	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,
> +#ifdef CONFIG_SMP
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +#endif
> +};
> +
> +static int qcom_pdc_translate(struct irq_domain *d,
> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
> +{
> +	return d->parent->ops->translate(d->parent, fwspec, hwirq, type);

No way. The translate operation is local to your domain. You don't go
and fish in another domain's private stuff. Please implement your own.
The reason you're getting away with it is because you're in the DT by
providing the GIC SPI instead of the pin into the PDC.

Don't do that. Expose the pin in the DT, use the alloc method to map the
PDC pin into the GIC pin.

> +}
> +
> +static int qcom_pdc_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;
> +	int i;
> +	unsigned int type;
> +	int ret;
> +
> +	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return -EINVAL;
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +				&qcom_pdc_gic_chip, domain->host_data);

You don't need this loop. It is only when you deal with Multi-MSI that
you can end-up allocating more than a single interrupt per allocation.
In the case of wired interrupts, you are guaranteed that nr_irqs == 1.

> +
> +	parent_fwspec = *fwspec;
> +	parent_fwspec.fwnode = domain->parent->fwnode;
> +
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> +						&parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops qcom_pdc_ops = {
> +	.translate	= qcom_pdc_translate,
> +	.alloc		= qcom_pdc_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +};
> +
> +static const struct of_device_id pdc_table[] = {
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, pdc_table);
> +
> +int qcom_pdc_init(struct device_node *node, struct device_node *parent)
> +{
> +	struct irq_domain *parent_domain,  *pdc_domain;
> +	const struct pdc_pin *data;
> +	const struct of_device_id *id;
> +	int ret;
> +
> +	id = of_match_node(pdc_table, node);
> +	if (id)
> +		data = id->data;
> +	else
> +		return -ENODEV;
> +
> +	pdc_base = of_iomap(node, 0);
> +	if (!pdc_base) {
> +		pr_err("%s(): unable to map PDC registers\n", node->full_name);
> +		return -ENXIO;
> +	}
> +
> +	parent_domain = irq_find_host(parent);
> +	if (!parent_domain) {
> +		pr_err("unable to obtain PDC parent domain\n");
> +		ret = -ENXIO;
> +		goto failure;
> +	}
> +
> +	pdc_domain = irq_domain_add_hierarchy(parent_domain, 0, PDC_MAX_IRQS,> +					node, &qcom_pdc_ops, (void *)data);

Consider using irq_domain_create_hierarchy instead, as it uses fwnode,
and thus consistent with the use of irq_fwspec elsewhere in the code.

> +	if (!pdc_domain) {
> +		pr_err("GIC domain add failed\n");
> +		ret = -ENOMEM;
> +		goto failure;
> +	}
> +
> +	pdc_domain->name = "qcom,pdc";

Why do you need this? The irqdomain layer should already assign a name
already.

> +	return 0;
> +
> +failure:
> +	iounmap(pdc_base);
> +	return ret;
> +}
> diff --git a/drivers/irqchip/qcom-pdc.h b/drivers/irqchip/qcom-pdc.h
> new file mode 100644
> index 000000000000..b5b64390175e
> --- /dev/null
> +++ b/drivers/irqchip/qcom-pdc.h
> @@ -0,0 +1,30 @@
> +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __QCOM_PDC_H__
> +#define __QCOM_PDC_H__
> +
> +#include <linux/irq.h>
> +
> +/**
> + * pdc_pin: Mapping of PDC port to hwirq
> + *
> + * @pin: PDC port for the IRQ
> + * @hwirq: hw IRQ number
> + */
> +struct pdc_pin {
> +	int pin;
> +	irq_hw_number_t hwirq;
> +};
> +
> +#endif /* __QCOM_PDC_H__ */
> 

What's the point of this include file?

There is one thing that worries me in this driver. You say that the PDC
"replays the pending interrupts so the GIC may wake up the processor".
How is that done without any PM hook allowing for a switch from GIC to
PDC? How do you ensure that you transition from one to the other without
loosing interrupts (edge interrupts, in particular)? Or can you get
spurious interrupts instead?

Thanks,

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

Re: [PATCH RFC 1/4] drivers: irqchip: pdc: add support for PDC interrupt controller

[Lina Iyer]

Hi Marc,
On Wed, Jan 24 2018 at 14:20 +0000, Marc Zyngier wrote:
>Hi Lina, Archana,
>
>On 23/01/18 17:56, Lina Iyer wrote:
>> From : Archana Sathyakumar <asathyak@codeaurora.org>
>>
>> The Power Domain Controller (PDC) hardware block on Qualcomm SoCs houses
>> an interrupt controller along with other domain control functions to
>> handle interrupt related functions like handle falling edge or active
>> low which are not detected at the GIC and handle wakeup interrupts.
>>
>> The interrupt controller is on an always-on domain for the purpose of
>> waking up the processor, but only a subset of the processor's interrupts
>> are routed through the PDC to the GIC. The PDC powers on the processor's
>> domain, bringing the domain out of low power mode and replays the
>> pending interrupts so the GIC may wake up the processor.
>>
>> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> [Lina: Split out DT bindings target data and initialization changes]
>> ---

>There is one thing that worries me in this driver. You say that the PDC
>"replays the pending interrupts so the GIC may wake up the processor".
>How is that done without any PM hook allowing for a switch from GIC to
>PDC? How do you ensure that you transition from one to the other without
>loosing interrupts (edge interrupts, in particular)? Or can you get
>spurious interrupts instead?
>
The hand-off between PDC and GIC happens in hardware and is transparent
to software. S/W just enables the PDC pins that would wake up the
processor and when any of those interrupts fire, the hardware powers up
the domain which inturn makes the GIC operational. The PDC then replays
the interrupts so the GIC may see them and wake up the CPUs.

-- Lina

Re: [PATCH RFC 1/4] drivers: irqchip: pdc: add support for PDC interrupt controller

[Lina Iyer]

Hi Mark,

On Wed, Jan 24 2018 at 14:20 +0000, Marc Zyngier wrote:
>Hi Lina, Archana,
>
>On 23/01/18 17:56, Lina Iyer wrote:
>> From : Archana Sathyakumar <asathyak@codeaurora.org>
>>
>> The Power Domain Controller (PDC) hardware block on Qualcomm SoCs houses
>> an interrupt controller along with other domain control functions to
>> handle interrupt related functions like handle falling edge or active
>> low which are not detected at the GIC and handle wakeup interrupts.
>>
>> The interrupt controller is on an always-on domain for the purpose of
>> waking up the processor, but only a subset of the processor's interrupts
>> are routed through the PDC to the GIC. The PDC powers on the processor's
>> domain, bringing the domain out of low power mode and replays the
>> pending interrupts so the GIC may wake up the processor.
>>
>> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> [Lina: Split out DT bindings target data and initialization changes]
>> ---

[...]

>> +
>> +static int qcom_pdc_translate(struct irq_domain *d,
>> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
>> +{
>> +	return d->parent->ops->translate(d->parent, fwspec, hwirq, type);
>
>No way. The translate operation is local to your domain. You don't go
>and fish in another domain's private stuff. Please implement your own.
>The reason you're getting away with it is because you're in the DT by
>providing the GIC SPI instead of the pin into the PDC.
>
I am looking into this approach. Was hoping to get some clarifications
from you.

The PDC sits between the device and the the GIC. Platform drivers
receive their interrupts from GIC. They are not aware of the fact that
the GIC may lose power and hand over its job to PDC. The platform
drivers may configure an interrupt as a wakeup interrupt, in which case,
we would wake up the CPU even if we are in system sleep or suspend mode.
Platform driver don't know about the PDC pin or its configuration
information. It makes it easier to keep that information contained
within the PDC driver. Instead of getting the pin-hwirq map from the
table as in patch #4, I can get that information cleanly from DT.

>Don't do that. Expose the pin in the DT, use the alloc method to map the
>PDC pin into the GIC pin.
>
I would like to understand how you mean by this. I am thinking something
like this in the dts -

/ {
	interrupt-controller = <&pdc>;
	soc {
		intc: interrupt-controller@176000 {
			[...]
			interrupt-controller;
			interrupt-parent = <&intc>;
		};

		pdc: interrupt-controller@210000 {
			[...]
			interrupt-controller;
			interrupt-parent = <&intc>;
			qcm,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
		};

		foo-device {
			interrupts = <GIC_SPI 481 IRQ_TYPE_NONE>;
		};
	};
 };

Where the qcom,pdc-ranges is defined as -
	<pdc-pin-start interrupt-vector size-of-sequence>.

For this example, the PDC map is established for pin0-pin93 using 94
interrupts in sequence starting from 512 and so on. This allows for
holes in the map per the hardware interrupt topology.

I am not sure if you were asking to specify the pin in the 'interrupts'
property in each device. I would like to avoid that as this may be an
information that the device author may care less about. Would you agree?

If I completely missed your point, would you please care to elaborate?

Thanks,
Lina

Re: [PATCH RFC 1/4] drivers: irqchip: pdc: add support for PDC interrupt controller

[Marc Zyngier]

On 30/01/18 17:56, Lina Iyer wrote:
> Hi Mark,
> 
> On Wed, Jan 24 2018 at 14:20 +0000, Marc Zyngier wrote:
>> Hi Lina, Archana,
>>
>> On 23/01/18 17:56, Lina Iyer wrote:
>>> From : Archana Sathyakumar <asathyak@codeaurora.org>
>>>
>>> The Power Domain Controller (PDC) hardware block on Qualcomm SoCs houses
>>> an interrupt controller along with other domain control functions to
>>> handle interrupt related functions like handle falling edge or active
>>> low which are not detected at the GIC and handle wakeup interrupts.
>>>
>>> The interrupt controller is on an always-on domain for the purpose of
>>> waking up the processor, but only a subset of the processor's interrupts
>>> are routed through the PDC to the GIC. The PDC powers on the processor's
>>> domain, bringing the domain out of low power mode and replays the
>>> pending interrupts so the GIC may wake up the processor.
>>>
>>> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
>>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>> [Lina: Split out DT bindings target data and initialization changes]
>>> ---
> 
> [...]
> 
>>> +
>>> +static int qcom_pdc_translate(struct irq_domain *d,
>>> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
>>> +{
>>> +	return d->parent->ops->translate(d->parent, fwspec, hwirq, type);
>>
>> No way. The translate operation is local to your domain. You don't go
>> and fish in another domain's private stuff. Please implement your own.
>> The reason you're getting away with it is because you're in the DT by
>> providing the GIC SPI instead of the pin into the PDC.
>>
> I am looking into this approach. Was hoping to get some clarifications
> from you.
> 
> The PDC sits between the device and the the GIC. Platform drivers
> receive their interrupts from GIC. They are not aware of the fact that
> the GIC may lose power and hand over its job to PDC. The platform

I disagree here. If the PDC is between the device and the GIC, then the
device interrupt line is routed to the PDC, and nothing else. The PDC
itself is tied to the GIC, but that's none of the device's business.

In general, the device shouldn't care about what interrupt controller it
is connected to. So please just describe the HW. There is about 10
similar configurations in the tree at the moment for the exact same
thing. They are simpler because their PDC equivalent has been designed
to exactly align with the GIC pins, but that's absolutely not a requirement.

> drivers may configure an interrupt as a wakeup interrupt, in which case,
> we would wake up the CPU even if we are in system sleep or suspend mode.
> Platform driver don't know about the PDC pin or its configuration
> information. It makes it easier to keep that information contained
> within the PDC driver. Instead of getting the pin-hwirq map from the
> table as in patch #4, I can get that information cleanly from DT.

That part is fine.

> 
>> Don't do that. Expose the pin in the DT, use the alloc method to map the
>> PDC pin into the GIC pin.
>>
> I would like to understand how you mean by this. I am thinking something
> like this in the dts -
> 
> / {
> 	interrupt-controller = <&pdc>;
> 	soc {
> 		intc: interrupt-controller@176000 {
> 			[...]
> 			interrupt-controller;
> 			interrupt-parent = <&intc>;
> 		};
> 
> 		pdc: interrupt-controller@210000 {
> 			[...]
> 			interrupt-controller;
> 			interrupt-parent = <&intc>;
> 			qcm,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
> 		};
> 
> 		foo-device {
> 			interrupts = <GIC_SPI 481 IRQ_TYPE_NONE>;

Drop the GIC_SPI (you should only use it in the PDC driver itself when
requesting the GIC version of the interrupt), make sure 481 is a PDC pin
number (it looks like a GIC SPI to me), and *never* encode IRQ_TYPE_NONE
in DT (put the actual trigger type).

> 		};
> 	};
>  };
> 
> Where the qcom,pdc-ranges is defined as -
> 	<pdc-pin-start interrupt-vector size-of-sequence>.
> 
> For this example, the PDC map is established for pin0-pin93 using 94
> interrupts in sequence starting from 512 and so on. This allows for
> holes in the map per the hardware interrupt topology.
> 
> I am not sure if you were asking to specify the pin in the 'interrupts'
> property in each device. I would like to avoid that as this may be an
> information that the device author may care less about. Would you agree?

The way we represent these stacked interrupt controllers is by tying the
device to the closest interrupt controller. Look at

> 
> If I completely missed your point, would you please care to elaborate?

Hope the above helps,

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

Re: [PATCH RFC 1/4] drivers: irqchip: pdc: add support for PDC interrupt controller

[Lina Iyer]

On Tue, Jan 30 2018 at 18:11 +0000, Marc Zyngier wrote:
>On 30/01/18 17:56, Lina Iyer wrote:
>> Hi Mark,
>>
>> On Wed, Jan 24 2018 at 14:20 +0000, Marc Zyngier wrote:
>>> Hi Lina, Archana,
>>>
>>> On 23/01/18 17:56, Lina Iyer wrote:
>>>> From : Archana Sathyakumar <asathyak@codeaurora.org>
>>>>
>>>> The Power Domain Controller (PDC) hardware block on Qualcomm SoCs houses
>>>> an interrupt controller along with other domain control functions to
>>>> handle interrupt related functions like handle falling edge or active
>>>> low which are not detected at the GIC and handle wakeup interrupts.
>>>>
>>>> The interrupt controller is on an always-on domain for the purpose of
>>>> waking up the processor, but only a subset of the processor's interrupts
>>>> are routed through the PDC to the GIC. The PDC powers on the processor's
>>>> domain, bringing the domain out of low power mode and replays the
>>>> pending interrupts so the GIC may wake up the processor.
>>>>
>>>> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
>>>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>>> [Lina: Split out DT bindings target data and initialization changes]
>>>> ---
>>
>> [...]
>>
>>>> +
>>>> +static int qcom_pdc_translate(struct irq_domain *d,
>>>> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
>>>> +{
>>>> +	return d->parent->ops->translate(d->parent, fwspec, hwirq, type);
>>>
>>> No way. The translate operation is local to your domain. You don't go
>>> and fish in another domain's private stuff. Please implement your own.
>>> The reason you're getting away with it is because you're in the DT by
>>> providing the GIC SPI instead of the pin into the PDC.
>>>
>> I am looking into this approach. Was hoping to get some clarifications
>> from you.
>>
>> The PDC sits between the device and the the GIC. Platform drivers
>> receive their interrupts from GIC. They are not aware of the fact that
>> the GIC may lose power and hand over its job to PDC. The platform
>
>I disagree here. If the PDC is between the device and the GIC, then the
>device interrupt line is routed to the PDC, and nothing else. The PDC
>itself is tied to the GIC, but that's none of the device's business.
>
>In general, the device shouldn't care about what interrupt controller it
>is connected to. So please just describe the HW. There is about 10
>similar configurations in the tree at the moment for the exact same
>thing. They are simpler because their PDC equivalent has been designed
>to exactly align with the GIC pins, but that's absolutely not a requirement.
>
This is exactly the case. The PDC is not aligned with GIC for all
interrupts. To be a bit more clear than what I seem to conveyed, some
interrupts are routed to the GIC and to the PDC and some are not.

Interrupts like USB that need to be detected for a falling edge or level
low (i.e., not detected at the GIC) are routed through the PDC as well.
The PDC detects these and re-triggers the GIC interrupts with the
correct polarity so the GIC may sense it.

In another case, when the GIC is powered off, the PDC will continue to
montior the interrupts routed through the PDC interrupt controller and
if any of the enabled ones trigger, the PDC will wake up the GIC and
replay the interrupts at the GIC.

To the device the presence of PDC is transparent, nor it is aware if the
GIC was powered off while the interrupt was enabled.

>> drivers may configure an interrupt as a wakeup interrupt, in which case,
>> we would wake up the CPU even if we are in system sleep or suspend mode.
>> Platform driver don't know about the PDC pin or its configuration
>> information. It makes it easier to keep that information contained
>> within the PDC driver. Instead of getting the pin-hwirq map from the
>> table as in patch #4, I can get that information cleanly from DT.
>
>That part is fine.
>
>>
>>> Don't do that. Expose the pin in the DT, use the alloc method to map the
>>> PDC pin into the GIC pin.
>>>
>> I would like to understand how you mean by this. I am thinking something
>> like this in the dts -
>>
>> / {
>> 	interrupt-controller = <&pdc>;
>> 	soc {
>> 		intc: interrupt-controller@176000 {
>> 			[...]
>> 			interrupt-controller;
>> 			interrupt-parent = <&intc>;
>> 		};
>>
>> 		pdc: interrupt-controller@210000 {
>> 			[...]
>> 			interrupt-controller;
>> 			interrupt-parent = <&intc>;
>> 			qcm,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
>> 		};
>>
>> 		foo-device {
>> 			interrupts = <GIC_SPI 481 IRQ_TYPE_NONE>;
>
>Drop the GIC_SPI (you should only use it in the PDC driver itself when
>requesting the GIC version of the interrupt), make sure 481 is a PDC pin
>number (it looks like a GIC SPI to me), and *never* encode IRQ_TYPE_NONE
>in DT (put the actual trigger type).
>
Sure, I just picked it up as an example.Yes, this is a GIC SPI and not a
PDC port. This is what the platform drivers desire. They have an
interrupt number assigned for their device and it would be preferrable
for them to use this interrupt vector at the GIC in DT. They dont't want
to know the PDC pin (even though the interrupt controller for the SoC
may be the PDC in DT). This is because the devices that do not have their
interrupt routed through the PDC, would anyways have only the GIC
interrupt vector to provide. They have no PDC port available for them.
The foo-device above is an example of one such device that does not have
it's interrupt routed through the PDC (it doesn't fall in the
pdc-ranges).

The PDC behaves more like arch extn to the GIC than a stacked interrupt
controller in the hierarchy.

>> 		};
>> 	};
>>  };
>>
>> Where the qcom,pdc-ranges is defined as -
>> 	<pdc-pin-start interrupt-vector size-of-sequence>.
>>
>> For this example, the PDC map is established for pin0-pin93 using 94
>> interrupts in sequence starting from 512 and so on. This allows for
>> holes in the map per the hardware interrupt topology.
>>
>> I am not sure if you were asking to specify the pin in the 'interrupts'
>> property in each device. I would like to avoid that as this may be an
>> information that the device author may care less about. Would you agree?
>
>The way we represent these stacked interrupt controllers is by tying the
>device to the closest interrupt controller. Look at
>
Did you miss a reference here?

We looked at tegra's implementation and it appears similar, thought they
don't seemt to have the same complexity.

Thanks,
Lina

Re: [PATCH RFC 1/4] drivers: irqchip: pdc: add support for PDC interrupt controller

[Marc Zyngier]

On 31/01/18 16:24, Lina Iyer wrote:
> On Tue, Jan 30 2018 at 18:11 +0000, Marc Zyngier wrote:
>> On 30/01/18 17:56, Lina Iyer wrote:
>>> Hi Mark,
>>>
>>> On Wed, Jan 24 2018 at 14:20 +0000, Marc Zyngier wrote:
>>>> Hi Lina, Archana,
>>>>
>>>> On 23/01/18 17:56, Lina Iyer wrote:
>>>>> From : Archana Sathyakumar <asathyak@codeaurora.org>
>>>>>
>>>>> The Power Domain Controller (PDC) hardware block on Qualcomm SoCs houses
>>>>> an interrupt controller along with other domain control functions to
>>>>> handle interrupt related functions like handle falling edge or active
>>>>> low which are not detected at the GIC and handle wakeup interrupts.
>>>>>
>>>>> The interrupt controller is on an always-on domain for the purpose of
>>>>> waking up the processor, but only a subset of the processor's interrupts
>>>>> are routed through the PDC to the GIC. The PDC powers on the processor's
>>>>> domain, bringing the domain out of low power mode and replays the
>>>>> pending interrupts so the GIC may wake up the processor.
>>>>>
>>>>> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
>>>>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>>>> [Lina: Split out DT bindings target data and initialization changes]
>>>>> ---
>>>
>>> [...]
>>>
>>>>> +
>>>>> +static int qcom_pdc_translate(struct irq_domain *d,
>>>>> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
>>>>> +{
>>>>> +	return d->parent->ops->translate(d->parent, fwspec, hwirq, type);
>>>>
>>>> No way. The translate operation is local to your domain. You don't go
>>>> and fish in another domain's private stuff. Please implement your own.
>>>> The reason you're getting away with it is because you're in the DT by
>>>> providing the GIC SPI instead of the pin into the PDC.
>>>>
>>> I am looking into this approach. Was hoping to get some clarifications
>>> from you.
>>>
>>> The PDC sits between the device and the the GIC. Platform drivers
>>> receive their interrupts from GIC. They are not aware of the fact that
>>> the GIC may lose power and hand over its job to PDC. The platform
>>
>> I disagree here. If the PDC is between the device and the GIC, then the
>> device interrupt line is routed to the PDC, and nothing else. The PDC
>> itself is tied to the GIC, but that's none of the device's business.
>>
>> In general, the device shouldn't care about what interrupt controller it
>> is connected to. So please just describe the HW. There is about 10
>> similar configurations in the tree at the moment for the exact same
>> thing. They are simpler because their PDC equivalent has been designed
>> to exactly align with the GIC pins, but that's absolutely not a requirement.
>>
> This is exactly the case. The PDC is not aligned with GIC for all
> interrupts. To be a bit more clear than what I seem to conveyed, some
> interrupts are routed to the GIC and to the PDC and some are not.

And? That's not a problem. A single device can have some interrupts
routed to an interrupt controller, and some to another. See the OMAP
stuff and the use of interrupts-extended.

> Interrupts like USB that need to be detected for a falling edge or level
> low (i.e., not detected at the GIC) are routed through the PDC as well.
> The PDC detects these and re-triggers the GIC interrupts with the
> correct polarity so the GIC may sense it.
> 
> In another case, when the GIC is powered off, the PDC will continue to
> montior the interrupts routed through the PDC interrupt controller and
> if any of the enabled ones trigger, the PDC will wake up the GIC and
> replay the interrupts at the GIC.
> 
> To the device the presence of PDC is transparent, nor it is aware if the
> GIC was powered off while the interrupt was enabled.

The device never cares about the controller it's on, and none of that
affects the way it works. It is connected to a pin that has certain
properties, and that's about it.

> 
>>> drivers may configure an interrupt as a wakeup interrupt, in which case,
>>> we would wake up the CPU even if we are in system sleep or suspend mode.
>>> Platform driver don't know about the PDC pin or its configuration
>>> information. It makes it easier to keep that information contained
>>> within the PDC driver. Instead of getting the pin-hwirq map from the
>>> table as in patch #4, I can get that information cleanly from DT.
>>
>> That part is fine.
>>
>>>
>>>> Don't do that. Expose the pin in the DT, use the alloc method to map the
>>>> PDC pin into the GIC pin.
>>>>
>>> I would like to understand how you mean by this. I am thinking something
>>> like this in the dts -
>>>
>>> / {
>>> 	interrupt-controller = <&pdc>;
>>> 	soc {
>>> 		intc: interrupt-controller@176000 {
>>> 			[...]
>>> 			interrupt-controller;
>>> 			interrupt-parent = <&intc>;
>>> 		};
>>>
>>> 		pdc: interrupt-controller@210000 {
>>> 			[...]
>>> 			interrupt-controller;
>>> 			interrupt-parent = <&intc>;
>>> 			qcm,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
>>> 		};
>>>
>>> 		foo-device {
>>> 			interrupts = <GIC_SPI 481 IRQ_TYPE_NONE>;
>>
>> Drop the GIC_SPI (you should only use it in the PDC driver itself when
>> requesting the GIC version of the interrupt), make sure 481 is a PDC pin
>> number (it looks like a GIC SPI to me), and *never* encode IRQ_TYPE_NONE
>> in DT (put the actual trigger type).
>>
> Sure, I just picked it up as an example.Yes, this is a GIC SPI and not a
> PDC port. This is what the platform drivers desire. They have an
> interrupt number assigned for their device and it would be preferrable
> for them to use this interrupt vector at the GIC in DT. They dont't want
> to know the PDC pin (even though the interrupt controller for the SoC
> may be the PDC in DT). This is because the devices that do not have their
> interrupt routed through the PDC, would anyways have only the GIC
> interrupt vector to provide. They have no PDC port available for them.
> The foo-device above is an example of one such device that does not have
> it's interrupt routed through the PDC (it doesn't fall in the
> pdc-ranges).
> 
> The PDC behaves more like arch extn to the GIC than a stacked interrupt
> controller in the hierarchy.

You're misguided here. Your PDC is not special, and it fits in the
existing framework just fine, just like all the other GPC, wakeupgen,
ictlr, and the rest.

> 
>>> 		};
>>> 	};
>>>  };
>>>
>>> Where the qcom,pdc-ranges is defined as -
>>> 	<pdc-pin-start interrupt-vector size-of-sequence>.
>>>
>>> For this example, the PDC map is established for pin0-pin93 using 94
>>> interrupts in sequence starting from 512 and so on. This allows for
>>> holes in the map per the hardware interrupt topology.
>>>
>>> I am not sure if you were asking to specify the pin in the 'interrupts'
>>> property in each device. I would like to avoid that as this may be an
>>> information that the device author may care less about. Would you agree?
>>
>> The way we represent these stacked interrupt controllers is by tying the
>> device to the closest interrupt controller. Look at
>>
> Did you miss a reference here?

Probably. I got tired.

> We looked at tegra's implementation and it appears similar, thought they
> don't seemt to have the same complexity.

Exactly. They are pretty similar. Hence my suggestion to adopt the same
approach, which you don't seem to agree with.

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

Re: [PATCH RFC 1/4] drivers: irqchip: pdc: add support for PDC interrupt controller

[Lina Iyer]

On Wed, Jan 31 2018 at 16:46 +0000, Marc Zyngier wrote:
>On 31/01/18 16:24, Lina Iyer wrote:
>> On Tue, Jan 30 2018 at 18:11 +0000, Marc Zyngier wrote:
>>> On 30/01/18 17:56, Lina Iyer wrote:
>>>> On Wed, Jan 24 2018 at 14:20 +0000, Marc Zyngier wrote:
>>>>> On 23/01/18 17:56, Lina Iyer wrote:
>>>>>> From : Archana Sathyakumar <asathyak@codeaurora.org>
>>>>>>
>>>>>> The Power Domain Controller (PDC) hardware block on Qualcomm SoCs houses
>>>>>> an interrupt controller along with other domain control functions to
>>>>>> handle interrupt related functions like handle falling edge or active
>>>>>> low which are not detected at the GIC and handle wakeup interrupts.
>>>>>>
>>>>>> The interrupt controller is on an always-on domain for the purpose of
>>>>>> waking up the processor, but only a subset of the processor's interrupts
>>>>>> are routed through the PDC to the GIC. The PDC powers on the processor's
>>>>>> domain, bringing the domain out of low power mode and replays the
>>>>>> pending interrupts so the GIC may wake up the processor.
>>>>>>
>>>>>> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
>>>>>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>>>>> [Lina: Split out DT bindings target data and initialization changes]
>>>>>> ---
>>>>
>> We looked at tegra's implementation and it appears similar, thought they
>> don't seemt to have the same complexity.
>
>Exactly. They are pretty similar. Hence my suggestion to adopt the same
>approach, which you don't seem to agree with.
>
I am reworking the driver based on what you have suggested in this
thread. Will submit a patchset for review in the next couple of days.

Once again, thank you for your time Marc.

-- Lina

[PATCH RFC 2/4] dt-bindings/interrupt-controller: pdc: descibe PDC device binding

[Lina Iyer]

From: Archana Sathyakumar <asathyak@codeaurora.org>

Add device binding documentation for the PDC Interrupt controller on
QCOM SoC's like the SDM845. The interrupt-controller can be used to
sense edge low interrupts and wakeup interrupts when the GIC is
non-operational.

Cc: devicetree@vger.kernel.org
Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 .../bindings/interrupt-controller/qcom,pdc.txt     | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
new file mode 100644
index 000000000000..c4592bbf678d
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
@@ -0,0 +1,55 @@
+PDC interrupt controller
+
+Qualcomm Technologies Inc. SoCs based on the RPM Hardened archicture have a
+Power Domain Controller (PDC) that is on always-on domain. In addition to
+providing power control for the power domains, the hardware also has an
+interrupt controller that can be used to help detect edge low interrupts as
+well detect interrupts when the GIC is non-operational.
+
+GIC is parent interrupt controller at the highest level. Platform interrupt
+controller PDC is next in hierarchy, followed by others. This driver only
+configures the interrupts, does not handle them.
+
+Properties:
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: Should contain "qcom,pdc" and "qcom,pdc-<target>"
+		    - "qcom,pdc-sdm845": For sdm845 pin data
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: Specifies the base physical address for PDC hardware.
+
+- interrupt-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: Specifies the number of cells needed to encode an interrupt
+		    source.
+		    Value must be 3.
+		    The encoding of these cells are same as described in [1].
+
+- interrupt-parent:
+	Usage: required
+	Value type: <phandle>
+	Definition: Specifies the interrupt parent necessary for hierarchical
+		    domain to operate.
+
+- interrupt-controller:
+	Usage: required
+	Value type: <bool>
+	Definition: Identifies the node as an interrupt controller.
+
+Example:
+
+	pdc: interrupt-controller@b220000 {
+		compatible = "qcom,pdc", "qcom,pdc-sdm845";
+		reg = <0xb220000 0x30000>;
+		#interrupt-cells = <3>;
+		interrupt-parent = <&intc>;
+		interrupt-controller;
+	};
+
+[1]. Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH RFC 2/4] dt-bindings/interrupt-controller: pdc: descibe PDC device binding

[Sudeep Holla]

On 23/01/18 17:56, Lina Iyer wrote:
> From: Archana Sathyakumar <asathyak@codeaurora.org>
> 
> Add device binding documentation for the PDC Interrupt controller on
> QCOM SoC's like the SDM845. The interrupt-controller can be used to
> sense edge low interrupts and wakeup interrupts when the GIC is
> non-operational.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  .../bindings/interrupt-controller/qcom,pdc.txt     | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> new file mode 100644
> index 000000000000..c4592bbf678d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> @@ -0,0 +1,55 @@
> +PDC interrupt controller
> +
> +Qualcomm Technologies Inc. SoCs based on the RPM Hardened archicture have a
> +Power Domain Controller (PDC) that is on always-on domain. In addition to
> +providing power control for the power domains, the hardware also has an
> +interrupt controller that can be used to help detect edge low interrupts as
> +well detect interrupts when the GIC is non-operational.
> +
> +GIC is parent interrupt controller at the highest level. Platform interrupt
> +controller PDC is next in hierarchy, followed by others.

> This driver only configures the interrupts, does not handle them.

Not sure if the above statement belongs to the binding.

-- 
Regards,
Sudeep

Re: [PATCH RFC 2/4] dt-bindings/interrupt-controller: pdc: descibe PDC device binding

[Lina Iyer]

On Tue, Jan 23 2018 at 18:10 +0000, Sudeep Holla wrote:
>
>
>On 23/01/18 17:56, Lina Iyer wrote:
>> From: Archana Sathyakumar <asathyak@codeaurora.org>
>>
>> Add device binding documentation for the PDC Interrupt controller on
>> QCOM SoC's like the SDM845. The interrupt-controller can be used to
>> sense edge low interrupts and wakeup interrupts when the GIC is
>> non-operational.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>  .../bindings/interrupt-controller/qcom,pdc.txt     | 55 ++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>> new file mode 100644
>> index 000000000000..c4592bbf678d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>> @@ -0,0 +1,55 @@
>> +PDC interrupt controller
>> +
>> +Qualcomm Technologies Inc. SoCs based on the RPM Hardened archicture have a
>> +Power Domain Controller (PDC) that is on always-on domain. In addition to
>> +providing power control for the power domains, the hardware also has an
>> +interrupt controller that can be used to help detect edge low interrupts as
>> +well detect interrupts when the GIC is non-operational.
>> +
>> +GIC is parent interrupt controller at the highest level. Platform interrupt
>> +controller PDC is next in hierarchy, followed by others.
>
>> This driver only configures the interrupts, does not handle them.
>
>Not sure if the above statement belongs to the binding.
>
Will fix.

Thanks,
Lina

Re: [PATCH RFC 2/4] dt-bindings/interrupt-controller: pdc: descibe PDC device binding

[Marc Zyngier]

On 23/01/18 17:56, Lina Iyer wrote:
> From: Archana Sathyakumar <asathyak@codeaurora.org>
> 
> Add device binding documentation for the PDC Interrupt controller on
> QCOM SoC's like the SDM845. The interrupt-controller can be used to
> sense edge low interrupts and wakeup interrupts when the GIC is
> non-operational.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  .../bindings/interrupt-controller/qcom,pdc.txt     | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> new file mode 100644
> index 000000000000..c4592bbf678d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> @@ -0,0 +1,55 @@
> +PDC interrupt controller
> +
> +Qualcomm Technologies Inc. SoCs based on the RPM Hardened archicture have a
nit: architecture

> +Power Domain Controller (PDC) that is on always-on domain. In addition to
> +providing power control for the power domains, the hardware also has an
> +interrupt controller that can be used to help detect edge low interrupts as
> +well detect interrupts when the GIC is non-operational.
> +
> +GIC is parent interrupt controller at the highest level. Platform interrupt
> +controller PDC is next in hierarchy, followed by others. This driver only
> +configures the interrupts, does not handle them.
> +
> +Properties:
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: Should contain "qcom,pdc" and "qcom,pdc-<target>"
> +		    - "qcom,pdc-sdm845": For sdm845 pin data
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: Specifies the base physical address for PDC hardware.
> +
> +- interrupt-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: Specifies the number of cells needed to encode an interrupt
> +		    source.
> +		    Value must be 3.
> +		    The encoding of these cells are same as described in [1].

The GICv3 binding allows for more cells (at least 4), so you'll have to
adapt both in the binding and in the driver.

> +
> +- interrupt-parent:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: Specifies the interrupt parent necessary for hierarchical
> +		    domain to operate.
> +
> +- interrupt-controller:
> +	Usage: required
> +	Value type: <bool>
> +	Definition: Identifies the node as an interrupt controller.
> +
> +Example:
> +
> +	pdc: interrupt-controller@b220000 {
> +		compatible = "qcom,pdc", "qcom,pdc-sdm845";
> +		reg = <0xb220000 0x30000>;
> +		#interrupt-cells = <3>;
> +		interrupt-parent = <&intc>;
> +		interrupt-controller;
> +	};
> +
> +[1]. Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> 

You'll also have to specify the ranges for the pin to SPI mapping.

Thanks,

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

Re: [PATCH RFC 2/4] dt-bindings/interrupt-controller: pdc: descibe PDC device binding

[Rob Herring]

On Tue, Jan 23, 2018 at 10:56:54AM -0700, Lina Iyer wrote:
> From: Archana Sathyakumar <asathyak@codeaurora.org>
> 
> Add device binding documentation for the PDC Interrupt controller on
> QCOM SoC's like the SDM845. The interrupt-controller can be used to
> sense edge low interrupts and wakeup interrupts when the GIC is
> non-operational.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  .../bindings/interrupt-controller/qcom,pdc.txt     | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> new file mode 100644
> index 000000000000..c4592bbf678d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> @@ -0,0 +1,55 @@
> +PDC interrupt controller
> +
> +Qualcomm Technologies Inc. SoCs based on the RPM Hardened archicture have a
> +Power Domain Controller (PDC) that is on always-on domain. In addition to
> +providing power control for the power domains, the hardware also has an
> +interrupt controller that can be used to help detect edge low interrupts as
> +well detect interrupts when the GIC is non-operational.
> +
> +GIC is parent interrupt controller at the highest level. Platform interrupt
> +controller PDC is next in hierarchy, followed by others. This driver only
> +configures the interrupts, does not handle them.
> +
> +Properties:
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: Should contain "qcom,pdc" and "qcom,pdc-<target>"
> +		    - "qcom,pdc-sdm845": For sdm845 pin data

pin data?

Convention is <soc>-<ip block>

Need to make the order of compatibles clear. Generally that is done with 
'followed by "qcom,pdc"' after the list of compatibles. But then, do you 
really need a fallback. This seems like a block that would change 
frequently. If there's not more than 2 or 3 chips with the same block, 
don't do a fallback.

> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: Specifies the base physical address for PDC hardware.
> +
> +- interrupt-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: Specifies the number of cells needed to encode an interrupt
> +		    source.
> +		    Value must be 3.
> +		    The encoding of these cells are same as described in [1].
> +
> +- interrupt-parent:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: Specifies the interrupt parent necessary for hierarchical
> +		    domain to operate.
> +
> +- interrupt-controller:
> +	Usage: required
> +	Value type: <bool>
> +	Definition: Identifies the node as an interrupt controller.
> +
> +Example:
> +
> +	pdc: interrupt-controller@b220000 {
> +		compatible = "qcom,pdc", "qcom,pdc-sdm845";

This is backwards.

> +		reg = <0xb220000 0x30000>;
> +		#interrupt-cells = <3>;
> +		interrupt-parent = <&intc>;
> +		interrupt-controller;
> +	};
> +
> +[1]. Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 3/4] drivers: irqchip: pdc: log PDC info in FTRACE

[Lina Iyer]

From: Archana Sathyakumar <asathyak@codeaurora.org>

Log key PDC pin configuration in FTRACE.

Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/irqchip/qcom-pdc.c |  6 ++++++
 include/trace/events/pdc.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 include/trace/events/pdc.h

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 57f1348bd81c..9b626e9f3a29 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -26,6 +26,8 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#define CREATE_TRACE_POINTS
+#include "trace/events/pdc.h"
 
 #include "qcom-pdc.h"
 
@@ -78,6 +80,7 @@ static inline void pdc_enable_intr(struct irq_data *d, bool on)
 			break;
 		udelay(5);
 	} while (1);
+	trace_irq_pin_config("enable", (u32)pin_out, (u32)d->hwirq, 0, on);
 	spin_unlock_irqrestore(&pdc_lock, flags);
 	WARN_ON(r_enable != enable);
 }
@@ -161,6 +164,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 		udelay(5);
 	} while (1);
 
+	trace_irq_pin_config("type_config", (u32)pin_out, (u32)d->hwirq,
+				pdc_type, 0);
+
 	/*
 	 * If type is edge triggered, forward that as Rising edge as PDC
 	 * takes care of converting falling edge to rising edge signal
diff --git a/include/trace/events/pdc.h b/include/trace/events/pdc.h
new file mode 100644
index 000000000000..bec54c414880
--- /dev/null
+++ b/include/trace/events/pdc.h
@@ -0,0 +1,50 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM pdc
+
+#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PDC_H_
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(irq_pin_config,
+
+	TP_PROTO(char *func, u32 pin, u32 hwirq, u32 type, u32 enable),
+
+	TP_ARGS(func, pin, hwirq, type, enable),
+
+	TP_STRUCT__entry(
+		__field(char *, func)
+		__field(u32, pin)
+		__field(u32, hwirq)
+		__field(u32, type)
+		__field(u32, enable)
+	),
+
+	TP_fast_assign(
+		__entry->pin = pin;
+		__entry->func = func;
+		__entry->hwirq = hwirq;
+		__entry->type = type;
+		__entry->enable = enable;
+	),
+
+	TP_printk("%s hwirq:%u pin:%u type:%u enable:%u",
+		__entry->func, __entry->pin, __entry->hwirq, __entry->type,
+		__entry->enable)
+);
+
+#endif
+#define TRACE_INCLUDE_FILE pdc
+#include <trace/define_trace.h>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH RFC 3/4] drivers: irqchip: pdc: log PDC info in FTRACE

[Steven Rostedt]

On Tue, 23 Jan 2018 10:56:55 -0700
Lina Iyer <ilina@codeaurora.org> wrote:

> From: Archana Sathyakumar <asathyak@codeaurora.org>

Hi Lina and Archana,

> 
> Log key PDC pin configuration in FTRACE.
> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  drivers/irqchip/qcom-pdc.c |  6 ++++++
>  include/trace/events/pdc.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>  create mode 100644 include/trace/events/pdc.h
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 57f1348bd81c..9b626e9f3a29 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -26,6 +26,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> +#define CREATE_TRACE_POINTS
> +#include "trace/events/pdc.h"
>  
>  #include "qcom-pdc.h"
>  
> @@ -78,6 +80,7 @@ static inline void pdc_enable_intr(struct irq_data *d, bool on)
>  			break;
>  		udelay(5);
>  	} while (1);
> +	trace_irq_pin_config("enable", (u32)pin_out, (u32)d->hwirq, 0, on);

I'm concerned with that string you are passing in.

>  	spin_unlock_irqrestore(&pdc_lock, flags);
>  	WARN_ON(r_enable != enable);
>  }
> @@ -161,6 +164,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>  		udelay(5);
>  	} while (1);
>  
> +	trace_irq_pin_config("type_config", (u32)pin_out, (u32)d->hwirq,

This one too.

> +				pdc_type, 0);
> +
>  	/*
>  	 * If type is edge triggered, forward that as Rising edge as PDC
>  	 * takes care of converting falling edge to rising edge signal
> diff --git a/include/trace/events/pdc.h b/include/trace/events/pdc.h
> new file mode 100644
> index 000000000000..bec54c414880
> --- /dev/null
> +++ b/include/trace/events/pdc.h
> @@ -0,0 +1,50 @@
> +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM pdc
> +
> +#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_PDC_H_
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(irq_pin_config,
> +
> +	TP_PROTO(char *func, u32 pin, u32 hwirq, u32 type, u32 enable),
> +
> +	TP_ARGS(func, pin, hwirq, type, enable),
> +
> +	TP_STRUCT__entry(
> +		__field(char *, func)
> +		__field(u32, pin)
> +		__field(u32, hwirq)
> +		__field(u32, type)
> +		__field(u32, enable)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->pin = pin;
> +		__entry->func = func;
> +		__entry->hwirq = hwirq;
> +		__entry->type = type;
> +		__entry->enable = enable;
> +	),
> +
> +	TP_printk("%s hwirq:%u pin:%u type:%u enable:%u",

%s is dereferencing a pointer, which works fine when reading the ASCII
translated trace files (/sys/kernel/debug/tracing/trace and friends).
But it breaks when we use trace-cmd or perf. Because they record the
binary data and they do not have access to memory inside the kernel.

What I would recommend is in pdc.h have:

#define PDC_ENTRY		1
#define PDC_TYPE_CONFIG		2

Then have the tracepoints do instead of:

	trace_irq_pin_config("enable", ...);
	trace_irq_pin_config("type_config", ...);

Have them do:

	trace_irq_pin_config(PDC_ENTRY, ...);
	trace_irq_pin_config(PDC_TYPE_CONFIG, ...);

And then have:

	TP_PROTO(u32 func, ...)

	TP_STRUCT__entry(
		__field(u32, func)
		[...]

	TP_fast_assign(
		__entry->func = func;
		[...]

	TP_printk("%s hwirq...",
		print_symbolic(__entry->func,
				{ PDC_ENTRY, "entry" },
				{ PDC_TYPE_CONFIG, "type_config" }),
		[...]

And you will get the same effect, with trace-cmd and perf still working
as expected.

-- Steve


> +		__entry->func, __entry->pin, __entry->hwirq, __entry->type,
> +		__entry->enable)
> +);
> +
> +#endif
> +#define TRACE_INCLUDE_FILE pdc
> +#include <trace/define_trace.h>

Re: [PATCH RFC 3/4] drivers: irqchip: pdc: log PDC info in FTRACE

[Lina Iyer]

On Tue, Jan 23 2018 at 18:13 +0000, Steven Rostedt wrote:
>On Tue, 23 Jan 2018 10:56:55 -0700
>Lina Iyer <ilina@codeaurora.org> wrote:
>
>> From: Archana Sathyakumar <asathyak@codeaurora.org>
>
>Hi Lina and Archana,
>
>>
>> Log key PDC pin configuration in FTRACE.
>>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>  drivers/irqchip/qcom-pdc.c |  6 ++++++
>>  include/trace/events/pdc.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 56 insertions(+)
>>  create mode 100644 include/trace/events/pdc.h
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 57f1348bd81c..9b626e9f3a29 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -26,6 +26,8 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  #include <linux/types.h>
>> +#define CREATE_TRACE_POINTS
>> +#include "trace/events/pdc.h"
>>
>>  #include "qcom-pdc.h"
>>
>> @@ -78,6 +80,7 @@ static inline void pdc_enable_intr(struct irq_data *d, bool on)
>>  			break;
>>  		udelay(5);
>>  	} while (1);
>> +	trace_irq_pin_config("enable", (u32)pin_out, (u32)d->hwirq, 0, on);
>
>I'm concerned with that string you are passing in.
>
>>  	spin_unlock_irqrestore(&pdc_lock, flags);
>>  	WARN_ON(r_enable != enable);
>>  }
>> @@ -161,6 +164,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>>  		udelay(5);
>>  	} while (1);
>>
>> +	trace_irq_pin_config("type_config", (u32)pin_out, (u32)d->hwirq,
>
>This one too.
>
>> +				pdc_type, 0);
>> +
>>  	/*
>>  	 * If type is edge triggered, forward that as Rising edge as PDC
>>  	 * takes care of converting falling edge to rising edge signal
>> diff --git a/include/trace/events/pdc.h b/include/trace/events/pdc.h
>> new file mode 100644
>> index 000000000000..bec54c414880
>> --- /dev/null
>> +++ b/include/trace/events/pdc.h
>> @@ -0,0 +1,50 @@
>> +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM pdc
>> +
>> +#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _TRACE_PDC_H_
>> +
>> +#include <linux/tracepoint.h>
>> +
>> +TRACE_EVENT(irq_pin_config,
>> +
>> +	TP_PROTO(char *func, u32 pin, u32 hwirq, u32 type, u32 enable),
>> +
>> +	TP_ARGS(func, pin, hwirq, type, enable),
>> +
>> +	TP_STRUCT__entry(
>> +		__field(char *, func)
>> +		__field(u32, pin)
>> +		__field(u32, hwirq)
>> +		__field(u32, type)
>> +		__field(u32, enable)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__entry->pin = pin;
>> +		__entry->func = func;
>> +		__entry->hwirq = hwirq;
>> +		__entry->type = type;
>> +		__entry->enable = enable;
>> +	),
>> +
>> +	TP_printk("%s hwirq:%u pin:%u type:%u enable:%u",
>
>%s is dereferencing a pointer, which works fine when reading the ASCII
>translated trace files (/sys/kernel/debug/tracing/trace and friends).
>But it breaks when we use trace-cmd or perf. Because they record the
>binary data and they do not have access to memory inside the kernel.
>
>What I would recommend is in pdc.h have:
>
>#define PDC_ENTRY		1
>#define PDC_TYPE_CONFIG		2
>
>Then have the tracepoints do instead of:
>
>	trace_irq_pin_config("enable", ...);
>	trace_irq_pin_config("type_config", ...);
>
>Have them do:
>
>	trace_irq_pin_config(PDC_ENTRY, ...);
>	trace_irq_pin_config(PDC_TYPE_CONFIG, ...);
>
>And then have:
>
>	TP_PROTO(u32 func, ...)
>
>	TP_STRUCT__entry(
>		__field(u32, func)
>		[...]
>
>	TP_fast_assign(
>		__entry->func = func;
>		[...]
>
>	TP_printk("%s hwirq...",
>		print_symbolic(__entry->func,
>				{ PDC_ENTRY, "entry" },
>				{ PDC_TYPE_CONFIG, "type_config" }),
>		[...]
>
>And you will get the same effect, with trace-cmd and perf still working
>as expected.
>
Thanks Steve. I will take care of it in the next spin.

-- Lina

>-- Steve
>
>
>> +		__entry->func, __entry->pin, __entry->hwirq, __entry->type,
>> +		__entry->enable)
>> +);
>> +
>> +#endif
>> +#define TRACE_INCLUDE_FILE pdc
>> +#include <trace/define_trace.h>
>

[PATCH RFC 4/4] drivers: irqchip: qcom: add pin information for SDM845

[Lina Iyer]

From: Archana Sathyakumar <asathyak@codeaurora.org>

Add PDC pin information for SDM845. Interrupts listed are wake up
sources for the processor, when the processor and GIC are powered down.

Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/irqchip/Makefile          |   2 +-
 drivers/irqchip/qcom-pdc-sdm845.c | 130 ++++++++++++++++++++++++++++++++++++++
 drivers/irqchip/qcom-pdc.c        |   3 +
 drivers/irqchip/qcom-pdc.h        |   1 +
 4 files changed, 135 insertions(+), 1 deletion(-)
 create mode 100644 drivers/irqchip/qcom-pdc-sdm845.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 280723d83916..0cf8f3feb47f 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -84,4 +84,4 @@ obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
 obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o
 obj-$(CONFIG_ARCH_SYNQUACER)		+= irq-sni-exiu.o
 obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
-obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
+obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o qcom-pdc-sdm845.o
diff --git a/drivers/irqchip/qcom-pdc-sdm845.c b/drivers/irqchip/qcom-pdc-sdm845.c
new file mode 100644
index 000000000000..57d0fb51d5ac
--- /dev/null
+++ b/drivers/irqchip/qcom-pdc-sdm845.c
@@ -0,0 +1,130 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include "qcom-pdc.h"
+
+const struct pdc_pin sdm845_data[] = {
+	{0, 512}, /* rpmh_wake */
+	{1, 513}, /* ee0_apps_hlos_spmi_periph_irq */
+	{2, 514}, /* ee1_apps_trustzone_spmi_periph_irq */
+	{3, 515}, /* secure_wdog_expired */
+	{4, 516}, /* secure_wdog_bark_irq */
+	{5, 517}, /* aop_wdog_expired_irq */
+	{6, 518}, /* qmp_usb3_lfps_rxterm_irq */
+	{7, 519}, /* qmp_usb3_lfps_rxterm_irq */
+	{8, 520}, /* eud_p0_dmse_int_mx */
+	{9, 521}, /* eud_p0_dpse_int_mx */
+	{10, 522}, /* eud_p1_dmse_int_mx */
+	{11, 523}, /* eud_p1_dpse_int_mx */
+	{12, 524}, /* eud_int_mx[1] */
+	{13, 525}, /* ssc_xpu_irq_summary */
+	{14, 526}, /* wd_bite_apps */
+	{15, 527}, /* ssc_vmidmt_irq_summary */
+	{16, 528}, /* q6ss_irq_out_apps_ipc[4] */
+	{17, 529}, /* not-connected */
+	{18, 530}, /* aoss_pmic_arb_mpu_xpu_summary_irq */
+	{19, 531}, /* apps_pdc_irq_in_19 */
+	{20, 532}, /* apps_pdc_irq_in_20 */
+	{21, 533}, /* apps_pdc_irq_in_21 */
+	{22, 534}, /* pdc_apps_epcb_timeout_summary_irq */
+	{23, 535}, /* spmi_protocol_irq */
+	{24, 536}, /* tsense0_tsense_max_min_int */
+	{25, 537}, /* tsense1_tsense_max_min_int */
+	{26, 538}, /* tsense0_upper_lower_intr */
+	{27, 539}, /* tsense1_upper_lower_intr */
+	{28, 540}, /* tsense0_critical_intr */
+	{29, 541}, /* tsense1_critical_intr */
+	{30, 542}, /* core_bi_px_gpio_1 */
+	{31, 543}, /* core_bi_px_gpio_3 */
+	{32, 544}, /* core_bi_px_gpio_5 */
+	{33, 545}, /* core_bi_px_gpio_10 */
+	{34, 546}, /* core_bi_px_gpio_11 */
+	{35, 547}, /* core_bi_px_gpio_20 */
+	{36, 548}, /* core_bi_px_gpio_22 */
+	{37, 549}, /* core_bi_px_gpio_24 */
+	{38, 550}, /* core_bi_px_gpio_26 */
+	{39, 551}, /* core_bi_px_gpio_30 */
+	{41, 553}, /* core_bi_px_gpio_32 */
+	{42, 554}, /* core_bi_px_gpio_34 */
+	{43, 555}, /* core_bi_px_gpio_36 */
+	{44, 556}, /* core_bi_px_gpio_37 */
+	{45, 557}, /* core_bi_px_gpio_38 */
+	{46, 558}, /* core_bi_px_gpio_39 */
+	{47, 559}, /* core_bi_px_gpio_40 */
+	{49, 561}, /* core_bi_px_gpio_43 */
+	{50, 562}, /* core_bi_px_gpio_44 */
+	{51, 563}, /* core_bi_px_gpio_46 */
+	{52, 564}, /* core_bi_px_gpio_48 */
+	{54, 566}, /* core_bi_px_gpio_52 */
+	{55, 567}, /* core_bi_px_gpio_53 */
+	{56, 568}, /* core_bi_px_gpio_54 */
+	{57, 569}, /* core_bi_px_gpio_56 */
+	{58, 570}, /* core_bi_px_gpio_57 */
+	{59, 571}, /* core_bi_px_gpio_58 */
+	{60, 572}, /* core_bi_px_gpio_59 */
+	{61, 573}, /* core_bi_px_gpio_60 */
+	{62, 574}, /* core_bi_px_gpio_61 */
+	{63, 575}, /* core_bi_px_gpio_62 */
+	{64, 576}, /* core_bi_px_gpio_63 */
+	{65, 577}, /* core_bi_px_gpio_64 */
+	{66, 578}, /* core_bi_px_gpio_66 */
+	{67, 579}, /* core_bi_px_gpio_68 */
+	{68, 580}, /* core_bi_px_gpio_71 */
+	{69, 581}, /* core_bi_px_gpio_73 */
+	{70, 582}, /* core_bi_px_gpio_77 */
+	{71, 583}, /* core_bi_px_gpio_78 */
+	{72, 584}, /* core_bi_px_gpio_79 */
+	{73, 585}, /* core_bi_px_gpio_80 */
+	{74, 586}, /* core_bi_px_gpio_84 */
+	{75, 587}, /* core_bi_px_gpio_85 */
+	{76, 588}, /* core_bi_px_gpio_86 */
+	{77, 589}, /* core_bi_px_gpio_88 */
+	{79, 591}, /* core_bi_px_gpio_91 */
+	{80, 592}, /* core_bi_px_gpio_92 */
+	{81, 593}, /* core_bi_px_gpio_95 */
+	{82, 594}, /* core_bi_px_gpio_96 */
+	{83, 595}, /* core_bi_px_gpio_97 */
+	{84, 596}, /* core_bi_px_gpio_101 */
+	{85, 597}, /* core_bi_px_gpio_103 */
+	{86, 598}, /* core_bi_px_gpio_104 */
+	{87, 599}, /* core_bi_px_to_mpm[6] */
+	{88, 600}, /* core_bi_px_to_mpm[0] */
+	{89, 601}, /* core_bi_px_to_mpm[1] */
+	{90, 602}, /* core_bi_px_gpio_115 */
+	{91, 603}, /* core_bi_px_gpio_116 */
+	{92, 604}, /* core_bi_px_gpio_117 */
+	{93, 605}, /* core_bi_px_gpio_118 */
+	{94, 641}, /* core_bi_px_gpio_119 */
+	{95, 642}, /* core_bi_px_gpio_120 */
+	{96, 643}, /* core_bi_px_gpio_121 */
+	{97, 644}, /* core_bi_px_gpio_122 */
+	{98, 645}, /* core_bi_px_gpio_123 */
+	{99, 646}, /* core_bi_px_gpio_124 */
+	{100, 647}, /* core_bi_px_gpio_125 */
+	{101, 648}, /* core_bi_px_to_mpm[5] */
+	{102, 649}, /* core_bi_px_gpio_127 */
+	{103, 650}, /* core_bi_px_gpio_128 */
+	{104, 651}, /* core_bi_px_gpio_129 */
+	{105, 652}, /* core_bi_px_gpio_130 */
+	{106, 653}, /* core_bi_px_gpio_132 */
+	{107, 654}, /* core_bi_px_gpio_133 */
+	{108, 655}, /* core_bi_px_gpio_145 */
+	{119, 666}, /* core_bi_px_to_mpm[2] */
+	{120, 667}, /* core_bi_px_to_mpm[3] */
+	{121, 668}, /* core_bi_px_to_mpm[4] */
+	{115, 662}, /* core_bi_px_gpio_41 */
+	{116, 663}, /* core_bi_px_gpio_89 */
+	{117, 664}, /* core_bi_px_gpio_31 */
+	{118, 665}, /* core_bi_px_gpio_49 */
+	{-1}
+};
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 9b626e9f3a29..0d54db4e915b 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -241,6 +241,7 @@ static const struct irq_domain_ops qcom_pdc_ops = {
 };
 
 static const struct of_device_id pdc_table[] = {
+	{ .compatible = "qcom,pdc-sdm845", .data = sdm845_data, },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pdc_table);
@@ -286,3 +287,5 @@ int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 	iounmap(pdc_base);
 	return ret;
 }
+
+IRQCHIP_DECLARE(pdc_sdm845, "qcom,pdc-sdm845", qcom_pdc_init);
diff --git a/drivers/irqchip/qcom-pdc.h b/drivers/irqchip/qcom-pdc.h
index b5b64390175e..6a9d9f9eb4f1 100644
--- a/drivers/irqchip/qcom-pdc.h
+++ b/drivers/irqchip/qcom-pdc.h
@@ -27,4 +27,5 @@ struct pdc_pin {
 	irq_hw_number_t hwirq;
 };
 
+extern const struct pdc_pin sdm845_data[];
 #endif /* __QCOM_PDC_H__ */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH RFC 4/4] drivers: irqchip: qcom: add pin information for SDM845

[Marc Zyngier]

On 23/01/18 17:56, Lina Iyer wrote:
> From: Archana Sathyakumar <asathyak@codeaurora.org>
> 
> Add PDC pin information for SDM845. Interrupts listed are wake up
> sources for the processor, when the processor and GIC are powered down.
> 
> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  drivers/irqchip/Makefile          |   2 +-
>  drivers/irqchip/qcom-pdc-sdm845.c | 130 ++++++++++++++++++++++++++++++++++++++
>  drivers/irqchip/qcom-pdc.c        |   3 +
>  drivers/irqchip/qcom-pdc.h        |   1 +
>  4 files changed, 135 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/irqchip/qcom-pdc-sdm845.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 280723d83916..0cf8f3feb47f 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -84,4 +84,4 @@ obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>  obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o
>  obj-$(CONFIG_ARCH_SYNQUACER)		+= irq-sni-exiu.o
>  obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
> -obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> +obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o qcom-pdc-sdm845.o
> diff --git a/drivers/irqchip/qcom-pdc-sdm845.c b/drivers/irqchip/qcom-pdc-sdm845.c
> new file mode 100644
> index 000000000000..57d0fb51d5ac
> --- /dev/null
> +++ b/drivers/irqchip/qcom-pdc-sdm845.c
> @@ -0,0 +1,130 @@
> +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include "qcom-pdc.h"
> +
> +const struct pdc_pin sdm845_data[] = {
> +	{0, 512}, /* rpmh_wake */
> +	{1, 513}, /* ee0_apps_hlos_spmi_periph_irq */
> +	{2, 514}, /* ee1_apps_trustzone_spmi_periph_irq */
> +	{3, 515}, /* secure_wdog_expired */
> +	{4, 516}, /* secure_wdog_bark_irq */
> +	{5, 517}, /* aop_wdog_expired_irq */
> +	{6, 518}, /* qmp_usb3_lfps_rxterm_irq */

[...]

Nice try, but no. This is DT material. Please use the pin number in your
DT, which the driver uses as a hwirq. When it comes to mapping it to the
corresponding GIC SPI, use a per-soc helper that comes from DT too (you
seem to only have two ranges here, so that's pretty easy to do).

> +};
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 9b626e9f3a29..0d54db4e915b 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -241,6 +241,7 @@ static const struct irq_domain_ops qcom_pdc_ops = {
>  };
>  
>  static const struct of_device_id pdc_table[] = {
> +	{ .compatible = "qcom,pdc-sdm845", .data = sdm845_data, },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, pdc_table);
> @@ -286,3 +287,5 @@ int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>  	iounmap(pdc_base);
>  	return ret;
>  }
> +
> +IRQCHIP_DECLARE(pdc_sdm845, "qcom,pdc-sdm845", qcom_pdc_init);
> diff --git a/drivers/irqchip/qcom-pdc.h b/drivers/irqchip/qcom-pdc.h
> index b5b64390175e..6a9d9f9eb4f1 100644
> --- a/drivers/irqchip/qcom-pdc.h
> +++ b/drivers/irqchip/qcom-pdc.h
> @@ -27,4 +27,5 @@ struct pdc_pin {
>  	irq_hw_number_t hwirq;
>  };
>  
> +extern const struct pdc_pin sdm845_data[];
>  #endif /* __QCOM_PDC_H__ */
> 

Thanks,

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

Re: [PATCH RFC 4/4] drivers: irqchip: qcom: add pin information for SDM845

[Lina Iyer]

On Wed, Jan 24 2018 at 14:20 +0000, Marc Zyngier wrote:
>On 23/01/18 17:56, Lina Iyer wrote:
>> From: Archana Sathyakumar <asathyak@codeaurora.org>
>>
>> +#include "qcom-pdc.h"
>> +
>> +const struct pdc_pin sdm845_data[] = {
>> +	{0, 512}, /* rpmh_wake */
>> +	{1, 513}, /* ee0_apps_hlos_spmi_periph_irq */
>> +	{2, 514}, /* ee1_apps_trustzone_spmi_periph_irq */
>> +	{3, 515}, /* secure_wdog_expired */
>> +	{4, 516}, /* secure_wdog_bark_irq */
>> +	{5, 517}, /* aop_wdog_expired_irq */
>> +	{6, 518}, /* qmp_usb3_lfps_rxterm_irq */
>
>[...]
>
>Nice try, but no. This is DT material. Please use the pin number in your
>DT, which the driver uses as a hwirq. When it comes to mapping it to the
>corresponding GIC SPI, use a per-soc helper that comes from DT too (you
>seem to only have two ranges here, so that's pretty easy to do).
>
Thanks Marc. I will look into this approach.

-- Lina

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Sudeep Holla]

Hi Lina,

On Tue, Jan 23, 2018 at 5:56 PM, Lina Iyer <ilina@codeaurora.org> wrote:
> On newer Qualcomm Techonologies Inc's SoCs like the SDM845, the GIC is in a
> power domain that can be powered off when not needed. Interrupts that need to
> be sensed even when the GIC is powered off, are routed through an interrupt
> controller in an always-on domain called the Power Domain Controller a.k.a PDC.
> This series adds support for the PDC's interrupt controller.
>

Sorry for the basic questions:

1. Will the GIC be powered off in any other state other than System suspend ?

2. Why this needs to be done in Linux, why can't it be transparent and hidden
    in the firmware doing the actual GIC power down ? I assume Linux is not
    powering down the GIC.

3. I see some bits that enable secure interrupts in one of the patch.
Is that even
    safe to allow Linux to enable some secure interrupts in PDC ?

--
Regards,
Sudeep

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Lina Iyer]

On Tue, Jan 23 2018 at 18:15 +0000, Sudeep Holla wrote:
>Hi Lina,
>
>On Tue, Jan 23, 2018 at 5:56 PM, Lina Iyer <ilina@codeaurora.org> wrote:
>> On newer Qualcomm Techonologies Inc's SoCs like the SDM845, the GIC is in a
>> power domain that can be powered off when not needed. Interrupts that need to
>> be sensed even when the GIC is powered off, are routed through an interrupt
>> controller in an always-on domain called the Power Domain Controller a.k.a PDC.
>> This series adds support for the PDC's interrupt controller.
>>
>
>Sorry for the basic questions:
>
>1. Will the GIC be powered off in any other state other than System suspend ?
>
Yes. When all the CPUs are in idle, there is an opportunity to power off
the CPU's power domain and the GIC. QCOM SoCs have been doing that for
many generations now.

>2. Why this needs to be done in Linux, why can't it be transparent and hidden
>    in the firmware doing the actual GIC power down ? I assume Linux is not
>    powering down the GIC.
No. You are right, Linux is not powering off the GIC directly. A
dedicated processor for power management in the SoC does that. Platform
drivers in Linux, know and configure the wakeup interrupts (depending on
the usecase). This is runtime specific and this is the way to tell the
SoC to wake up the processor even if the GIC and the CPU domain were
powered off.

>
>3. I see some bits that enable secure interrupts in one of the patch.
>Is that even
>    safe to allow Linux to enable some secure interrupts in PDC ?
>
Linux should not and would not configure secure interrupts. We would not
have permissions for secure interrupts. The interrupt names might be a
misnomer, but the interrupts listed in patch #4 are all non-secure
interrupts.

Thanks,
Lina

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Sudeep Holla]

On 23/01/18 18:44, Lina Iyer wrote:
> On Tue, Jan 23 2018 at 18:15 +0000, Sudeep Holla wrote:
>> Hi Lina,
>>
>> On Tue, Jan 23, 2018 at 5:56 PM, Lina Iyer <ilina@codeaurora.org> wrote:
>>> On newer Qualcomm Techonologies Inc's SoCs like the SDM845, the GIC
>>> is in a
>>> power domain that can be powered off when not needed. Interrupts that
>>> need to
>>> be sensed even when the GIC is powered off, are routed through an
>>> interrupt
>>> controller in an always-on domain called the Power Domain Controller
>>> a.k.a PDC.
>>> This series adds support for the PDC's interrupt controller.
>>>
>>
>> Sorry for the basic questions:
>>
>> 1. Will the GIC be powered off in any other state other than System
>> suspend ?
>>
> Yes. When all the CPUs are in idle, there is an opportunity to power off
> the CPU's power domain and the GIC. QCOM SoCs have been doing that for
> many generations now.
> 

OK interesting, in that case so either GIC state is saved/restored with
some out of tree patches or the firmware takes care of it and it's
transparent to Linux ?

Also when will this PDC wakeup interrupts get configured ?

>> 2. Why this needs to be done in Linux, why can't it be transparent and
>> hidden
>>    in the firmware doing the actual GIC power down ? I assume Linux is
>> not
>>    powering down the GIC.
> No. You are right, Linux is not powering off the GIC directly. A
> dedicated processor for power management in the SoC does that. Platform
> drivers in Linux, know and configure the wakeup interrupts (depending on
> the usecase). This is runtime specific and this is the way to tell the
> SoC to wake up the processor even if the GIC and the CPU domain were
> powered off.
> 

OK, understood. By transparent, I mean firmware can copy the interrupts
enabled in the GIC to the PDC. It need not be kernel driven.

>>
>> 3. I see some bits that enable secure interrupts in one of the patch.
>> Is that even
>>    safe to allow Linux to enable some secure interrupts in PDC ?
>>
> Linux should not and would not configure secure interrupts. We would not
> have permissions for secure interrupts. The interrupt names might be a
> misnomer, but the interrupts listed in patch #4 are all non-secure
> interrupts.
> 

OK. So I can assume PDC is partitioned in secure and non-secure. If not
it's safe not have any access for PDC in the kernel. Couple of designs
of similar PDC I have seen is system wide and does handle even secure
part of the system. That was the main reason for checking.

-- 
Regards,
Sudeep

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Lina Iyer]

On Wed, Jan 24 2018 at 10:10 +0000, Sudeep Holla wrote:
>
>
>On 23/01/18 18:44, Lina Iyer wrote:
>> On Tue, Jan 23 2018 at 18:15 +0000, Sudeep Holla wrote:
>>> Hi Lina,
>>>
>>> On Tue, Jan 23, 2018 at 5:56 PM, Lina Iyer <ilina@codeaurora.org> wrote:
>>>> On newer Qualcomm Techonologies Inc's SoCs like the SDM845, the GIC
>>>> is in a
>>>> power domain that can be powered off when not needed. Interrupts that
>>>> need to
>>>> be sensed even when the GIC is powered off, are routed through an
>>>> interrupt
>>>> controller in an always-on domain called the Power Domain Controller
>>>> a.k.a PDC.
>>>> This series adds support for the PDC's interrupt controller.
>>>>
>>>
>>> Sorry for the basic questions:
>>>
>>> 1. Will the GIC be powered off in any other state other than System
>>> suspend ?
>>>
>> Yes. When all the CPUs are in idle, there is an opportunity to power off
>> the CPU's power domain and the GIC. QCOM SoCs have been doing that for
>> many generations now.
>>
>
>OK interesting, in that case so either GIC state is saved/restored with
>some out of tree patches or the firmware takes care of it and it's
>transparent to Linux ?
>
Yes. It is handled by a remote processor, which is aware that the application
processor subsystem has been powered off.

>Also when will this PDC wakeup interrupts get configured ?
>
The platform drivers configure the IRQ as a wake source and if the IRQ
is one of those listed as routed to the PDC, the PDC is configured to
sense the interrupt and when the application processor domain is powered
on and the GIC can sense the interrupts, it is replayed to the GIC,
which then wakes up the processor.

>>> 2. Why this needs to be done in Linux, why can't it be transparent and
>>> hidden
>>>    in the firmware doing the actual GIC power down ? I assume Linux is
>>> not
>>>    powering down the GIC.
>> No. You are right, Linux is not powering off the GIC directly. A
>> dedicated processor for power management in the SoC does that. Platform
>> drivers in Linux, know and configure the wakeup interrupts (depending on
>> the usecase). This is runtime specific and this is the way to tell the
>> SoC to wake up the processor even if the GIC and the CPU domain were
>> powered off.
>>
>
>OK, understood. By transparent, I mean firmware can copy the interrupts
>enabled in the GIC to the PDC. It need not be kernel driven.
>
Yes, through the hierarchy.

>>>
>>> 3. I see some bits that enable secure interrupts in one of the patch.
>>> Is that even
>>>    safe to allow Linux to enable some secure interrupts in PDC ?
>>>
>> Linux should not and would not configure secure interrupts. We would not
>> have permissions for secure interrupts. The interrupt names might be a
>> misnomer, but the interrupts listed in patch #4 are all non-secure
>> interrupts.
>>
>
>OK. So I can assume PDC is partitioned in secure and non-secure. If not
>it's safe not have any access for PDC in the kernel. Couple of designs
>of similar PDC I have seen is system wide and does handle even secure
>part of the system. That was the main reason for checking.
>
Yes. There is a partition and protected. So only permitted ELs can write
to the registers. This is done by the firmware at boot.

Thanks,
Lina

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Sudeep Holla]

On 24/01/18 17:43, Lina Iyer wrote:
> On Wed, Jan 24 2018 at 10:10 +0000, Sudeep Holla wrote:
>>
>>
>> On 23/01/18 18:44, Lina Iyer wrote:
>>> On Tue, Jan 23 2018 at 18:15 +0000, Sudeep Holla wrote:
>>>> Hi Lina,
>>>>
>>>> On Tue, Jan 23, 2018 at 5:56 PM, Lina Iyer <ilina@codeaurora.org>
>>>> wrote:
>>>>> On newer Qualcomm Techonologies Inc's SoCs like the SDM845, the GIC
>>>>> is in a
>>>>> power domain that can be powered off when not needed. Interrupts that
>>>>> need to
>>>>> be sensed even when the GIC is powered off, are routed through an
>>>>> interrupt
>>>>> controller in an always-on domain called the Power Domain Controller
>>>>> a.k.a PDC.
>>>>> This series adds support for the PDC's interrupt controller.
>>>>>
>>>>
>>>> Sorry for the basic questions:
>>>>
>>>> 1. Will the GIC be powered off in any other state other than System
>>>> suspend ?
>>>>
>>> Yes. When all the CPUs are in idle, there is an opportunity to power off
>>> the CPU's power domain and the GIC. QCOM SoCs have been doing that for
>>> many generations now.
>>>
>>
>> OK interesting, in that case so either GIC state is saved/restored with
>> some out of tree patches or the firmware takes care of it and it's
>> transparent to Linux ?
>>
> Yes. It is handled by a remote processor, which is aware that the
> application processor subsystem has been powered off.
> 

OK, better.

>> Also when will this PDC wakeup interrupts get configured ?
>>
> The platform drivers configure the IRQ as a wake source and if the IRQ
> is one of those listed as routed to the PDC, the PDC is configured to
> sense the interrupt and when the application processor domain is powered
> on and the GIC can sense the interrupts, it is replayed to the GIC,
> which then wakes up the processor.
> 

Now why can't this be done in the firmware entirely, if it can
save/restore GIC state.

>>>> 2. Why this needs to be done in Linux, why can't it be transparent and
>>>> hidden
>>>>    in the firmware doing the actual GIC power down ? I assume Linux is
>>>> not
>>>>    powering down the GIC.
>>> No. You are right, Linux is not powering off the GIC directly. A
>>> dedicated processor for power management in the SoC does that. Platform
>>> drivers in Linux, know and configure the wakeup interrupts (depending on
>>> the usecase). This is runtime specific and this is the way to tell the
>>> SoC to wake up the processor even if the GIC and the CPU domain were
>>> powered off.
>>>
>>
>> OK, understood. By transparent, I mean firmware can copy the interrupts
>> enabled in the GIC to the PDC. It need not be kernel driven.
>>
> Yes, through the hierarchy.
> 

/me confused. Are you saying it's possible for f/w to copy wakeup
sources from GIC to PDC or not ?

>>>>
>>>> 3. I see some bits that enable secure interrupts in one of the patch.
>>>> Is that even
>>>>    safe to allow Linux to enable some secure interrupts in PDC ?
>>>>
>>> Linux should not and would not configure secure interrupts. We would not
>>> have permissions for secure interrupts. The interrupt names might be a
>>> misnomer, but the interrupts listed in patch #4 are all non-secure
>>> interrupts.
>>>
>>
>> OK. So I can assume PDC is partitioned in secure and non-secure. If not
>> it's safe not have any access for PDC in the kernel. Couple of designs
>> of similar PDC I have seen is system wide and does handle even secure
>> part of the system. That was the main reason for checking.
>>
> Yes. There is a partition and protected. So only permitted ELs can write
> to the registers. This is done by the firmware at boot.
> 

Just for myself to understand better, so you have multiple partitions in
PDC and one of them is given to EL1 or it just has one partition and
that can be configured so that only permitted ELx is allowed to access
it(in your case it's EL1)

-- 
Regards,
Sudeep

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Lina Iyer]

On Wed, Jan 24 2018 at 17:54 +0000, Sudeep Holla wrote:
>
>
>On 24/01/18 17:43, Lina Iyer wrote:
>> On Wed, Jan 24 2018 at 10:10 +0000, Sudeep Holla wrote:
>>>
>>>
>>> On 23/01/18 18:44, Lina Iyer wrote:
>>>> On Tue, Jan 23 2018 at 18:15 +0000, Sudeep Holla wrote:

>>> Also when will this PDC wakeup interrupts get configured ?
>>>
>> The platform drivers configure the IRQ as a wake source and if the IRQ
>> is one of those listed as routed to the PDC, the PDC is configured to
>> sense the interrupt and when the application processor domain is powered
>> on and the GIC can sense the interrupts, it is replayed to the GIC,
>> which then wakes up the processor.
>>
>
>Now why can't this be done in the firmware entirely, if it can
>save/restore GIC state.
>
That is because platform drivers make the choice of which interrupts are
wake up capable at runtime, based on the usecase. They have to have a
way to tell the firmware to do that. Earlier we used IPC to tell the
remote processor to handle that. Now we do that by writing to the
registers locally from Linux.

>>> OK, understood. By transparent, I mean firmware can copy the interrupts
>>> enabled in the GIC to the PDC. It need not be kernel driven.
>>>
>> Yes, through the hierarchy.
>>
>
>/me confused. Are you saying it's possible for f/w to copy wakeup
>sources from GIC to PDC or not ?
>
Platform drivers leave their interrupts enabled at the GIC. Only
interrupts that are wakeup capable are of interest when the processor is
powered down. The remote processor (or f/w) will need to know the set of
wakeup capable interrupts and then configure the PDC by copying from
GIC. As mentioned earlier, it is simplified by letting Linux write to
the PDC reqisters directly.

>> Yes. There is a partition and protected. So only permitted ELs can write
>> to the registers. This is done by the firmware at boot.
>>
>
>Just for myself to understand better, so you have multiple partitions in
>PDC and one of them is given to EL1 or it just has one partition and
>that can be configured so that only permitted ELx is allowed to access
>it(in your case it's EL1)
>
Yes.

Thanks,
Lina

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Sudeep Holla]

On 25/01/18 15:54, Lina Iyer wrote:
> On Wed, Jan 24 2018 at 17:54 +0000, Sudeep Holla wrote:
>>
>>
>> On 24/01/18 17:43, Lina Iyer wrote:
>>> On Wed, Jan 24 2018 at 10:10 +0000, Sudeep Holla wrote:
>>>>
>>>>
>>>> On 23/01/18 18:44, Lina Iyer wrote:
>>>>> On Tue, Jan 23 2018 at 18:15 +0000, Sudeep Holla wrote:
> 
>>>> Also when will this PDC wakeup interrupts get configured ?
>>>>
>>> The platform drivers configure the IRQ as a wake source and if the IRQ
>>> is one of those listed as routed to the PDC, the PDC is configured to
>>> sense the interrupt and when the application processor domain is powered
>>> on and the GIC can sense the interrupts, it is replayed to the GIC,
>>> which then wakes up the processor.
>>>
>>
>> Now why can't this be done in the firmware entirely, if it can
>> save/restore GIC state.
>>
> That is because platform drivers make the choice of which interrupts are
> wake up capable at runtime, based on the usecase. They have to have a
> way to tell the firmware to do that. Earlier we used IPC to tell the
> remote processor to handle that. Now we do that by writing to the
> registers locally from Linux.
> 
>>>> OK, understood. By transparent, I mean firmware can copy the interrupts
>>>> enabled in the GIC to the PDC. It need not be kernel driven.
>>>>
>>> Yes, through the hierarchy.
>>>
>>
>> /me confused. Are you saying it's possible for f/w to copy wakeup
>> sources from GIC to PDC or not ?
>>
> Platform drivers leave their interrupts enabled at the GIC. Only
> interrupts that are wakeup capable are of interest when the processor is
> powered down. The remote processor (or f/w) will need to know the set of
> wakeup capable interrupts and then configure the PDC by copying from
> GIC. As mentioned earlier, it is simplified by letting Linux write to
> the PDC reqisters directly.
> 

OK looks like I have not properly conveyed what I wanted to :(
So lets take a peripheral A which has GIC interrupt X and a
corresponding PDC interrupt Y.

IIUC you want to configure Y from Linux while X is still enabled.

1. GICv3 masks all the interrupts(~X) that are not wakeup sources.
   So when you say "platform drivers leave their interrupts enabled at
   the GIC", it's not completely correct.

2. GIC CPU interface is disabled in firmware, so it's better to copy the
   wakeup source to PDC just before that in the firmware.

3. Remote f/w must just know the mapping to PDC(X) for all the enabled
   interrupts(Y) at the GIC and enable them accordingly at PDC. Is that
   not what you have in the array in patch 4 ?

I find above approach simpler instead of getting those wakeup
interrupts defined per peripheral in DT. Further if there are any secure
wakeup interrupts the firmware can also deal with that.

>>> Yes. There is a partition and protected. So only permitted ELs can write
>>> to the registers. This is done by the firmware at boot.
>>>
>>
>> Just for myself to understand better, so you have multiple partitions in
>> PDC and one of them is given to EL1 or it just has one partition and
>> that can be configured so that only permitted ELx is allowed to access
>> it(in your case it's EL1)
>>
> Yes.

Yes for the former or the latter :) ?

-- 
Regards,
Sudeep

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Lina Iyer]

On Thu, Jan 25 2018 at 16:39 +0000, Sudeep Holla wrote:
>
>
>On 25/01/18 15:54, Lina Iyer wrote:
>> On Wed, Jan 24 2018 at 17:54 +0000, Sudeep Holla wrote:
>>>
>>>
>>> On 24/01/18 17:43, Lina Iyer wrote:
>>>> On Wed, Jan 24 2018 at 10:10 +0000, Sudeep Holla wrote:
>>>>>
>>>>>
>>>>> On 23/01/18 18:44, Lina Iyer wrote:
>>>>>> On Tue, Jan 23 2018 at 18:15 +0000, Sudeep Holla wrote:

>> Platform drivers leave their interrupts enabled at the GIC. Only
>> interrupts that are wakeup capable are of interest when the processor is
>> powered down. The remote processor (or f/w) will need to know the set of
>> wakeup capable interrupts and then configure the PDC by copying from
>> GIC. As mentioned earlier, it is simplified by letting Linux write to
>> the PDC reqisters directly.
>>
>
>OK looks like I have not properly conveyed what I wanted to :(
>So lets take a peripheral A which has GIC interrupt X and a
>corresponding PDC interrupt Y.
>
>IIUC you want to configure Y from Linux while X is still enabled.
>
>1. GICv3 masks all the interrupts(~X) that are not wakeup sources.
>   So when you say "platform drivers leave their interrupts enabled at
>   the GIC", it's not completely correct.
>
During idle path, the interrupts remain enabled. The GIC configuration
is retained, but the controller cannot sense interrupts because the
GIC logic is powered off. The PDC takes over for GIC at this time.

>2. GIC CPU interface is disabled in firmware, so it's better to copy the
>   wakeup source to PDC just before that in the firmware.
>
>3. Remote f/w must just know the mapping to PDC(X) for all the enabled
>   interrupts(Y) at the GIC and enable them accordingly at PDC. Is that
>   not what you have in the array in patch 4 ?
>
>I find above approach simpler instead of getting those wakeup
>interrupts defined per peripheral in DT. Further if there are any secure
>wakeup interrupts the firmware can also deal with that.
>
You assume that the remote processor is capable of doing all that. It is
better to de-centralize all this and have individual processors do the
work of configuring their wake up sources. We used to do that in earlier
SoCs but with SDM845, we moved to de-centralized model to reduce latency
and take the load off from time-critical idle path at the remote
processor. Dumping all this work into the firmware/PSCI is not desirable
either.

>>>> Yes. There is a partition and protected. So only permitted ELs can write
>>>> to the registers. This is done by the firmware at boot.
>>>>
>>>
>>> Just for myself to understand better, so you have multiple partitions in
>>> PDC and one of them is given to EL1 or it just has one partition and
>>> that can be configured so that only permitted ELx is allowed to access
>>> it(in your case it's EL1)
>>>
>> Yes.
>
>Yes for the former or the latter :) ?
>
The latter.

Thanks,
Lina

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Sudeep Holla]

On 25/01/18 18:13, Lina Iyer wrote:
> On Thu, Jan 25 2018 at 16:39 +0000, Sudeep Holla wrote:
>>
>>
>> On 25/01/18 15:54, Lina Iyer wrote:
>>> On Wed, Jan 24 2018 at 17:54 +0000, Sudeep Holla wrote:
>>>>
>>>>
>>>> On 24/01/18 17:43, Lina Iyer wrote:
>>>>> On Wed, Jan 24 2018 at 10:10 +0000, Sudeep Holla wrote:
>>>>>>
>>>>>>
>>>>>> On 23/01/18 18:44, Lina Iyer wrote:
>>>>>>> On Tue, Jan 23 2018 at 18:15 +0000, Sudeep Holla wrote:
> 
>>> Platform drivers leave their interrupts enabled at the GIC. Only
>>> interrupts that are wakeup capable are of interest when the processor is
>>> powered down. The remote processor (or f/w) will need to know the set of
>>> wakeup capable interrupts and then configure the PDC by copying from
>>> GIC. As mentioned earlier, it is simplified by letting Linux write to
>>> the PDC reqisters directly.
>>>
>>
>> OK looks like I have not properly conveyed what I wanted to :(
>> So lets take a peripheral A which has GIC interrupt X and a
>> corresponding PDC interrupt Y.
>>
>> IIUC you want to configure Y from Linux while X is still enabled.
>>
>> 1. GICv3 masks all the interrupts(~X) that are not wakeup sources.
>>   So when you say "platform drivers leave their interrupts enabled at
>>   the GIC", it's not completely correct.
>>
> During idle path, the interrupts remain enabled. The GIC configuration
> is retained, but the controller cannot sense interrupts because the
> GIC logic is powered off. The PDC takes over for GIC at this time.
> 

Ah OK, so PDC interrupts needs to be enabled all the time then.
I was missing that.

>> 2. GIC CPU interface is disabled in firmware, so it's better to copy the
>>   wakeup source to PDC just before that in the firmware.
>>
>> 3. Remote f/w must just know the mapping to PDC(X) for all the enabled
>>   interrupts(Y) at the GIC and enable them accordingly at PDC. Is that
>>   not what you have in the array in patch 4 ?
>>
>> I find above approach simpler instead of getting those wakeup
>> interrupts defined per peripheral in DT. Further if there are any secure
>> wakeup interrupts the firmware can also deal with that.
>>
> You assume that the remote processor is capable of doing all that. It is
> better to de-centralize all this and have individual processors do the
> work of configuring their wake up sources. We used to do that in earlier
> SoCs but with SDM845, we moved to de-centralized model to reduce latency
> and take the load off from time-critical idle path at the remote
> processor. Dumping all this work into the firmware/PSCI is not desirable
> either.
> 

It may have some advantages to decentralize but will that not cause
issues in complex systems ? I assume even modem and other processors can
access and configure these wakeup interrupts. What happens if 2 such
processors try to access it at the same time ?

Thanks for you patience and taking time to help me understand the design.

-- 
Regards,
Sudeep

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Lina Iyer]

On Thu, Jan 25 2018 at 18:43 +0000, Sudeep Holla wrote:
>
>
>On 25/01/18 18:13, Lina Iyer wrote:
>> On Thu, Jan 25 2018 at 16:39 +0000, Sudeep Holla wrote:
>>>
>>>
>>> On 25/01/18 15:54, Lina Iyer wrote:
>>>> On Wed, Jan 24 2018 at 17:54 +0000, Sudeep Holla wrote:
>>>>>
>>>>>
>>>>> On 24/01/18 17:43, Lina Iyer wrote:
>>>>>> On Wed, Jan 24 2018 at 10:10 +0000, Sudeep Holla wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 23/01/18 18:44, Lina Iyer wrote:
>>>>>>>> On Tue, Jan 23 2018 at 18:15 +0000, Sudeep Holla wrote:
>>
>Ah OK, so PDC interrupts needs to be enabled all the time then.
>I was missing that.
>
>>> 2. GIC CPU interface is disabled in firmware, so it's better to copy the
>>>   wakeup source to PDC just before that in the firmware.
>>>
>>> 3. Remote f/w must just know the mapping to PDC(X) for all the enabled
>>>   interrupts(Y) at the GIC and enable them accordingly at PDC. Is that
>>>   not what you have in the array in patch 4 ?
>>>
>>> I find above approach simpler instead of getting those wakeup
>>> interrupts defined per peripheral in DT. Further if there are any secure
>>> wakeup interrupts the firmware can also deal with that.
>>>
>> You assume that the remote processor is capable of doing all that. It is
>> better to de-centralize all this and have individual processors do the
>> work of configuring their wake up sources. We used to do that in earlier
>> SoCs but with SDM845, we moved to de-centralized model to reduce latency
>> and take the load off from time-critical idle path at the remote
>> processor. Dumping all this work into the firmware/PSCI is not desirable
>> either.
>>
>
>It may have some advantages to decentralize but will that not cause
>issues in complex systems ? I assume even modem and other processors can
>access and configure these wakeup interrupts. What happens if 2 such
>processors try to access it at the same time ?
>
Every processor in the SoC has its own PDC and does exactly the same
thing in SW. The hardware blocks are replicated for each of the
'subsystem' and they behave similarly.

>Thanks for you patience and taking time to help me understand the design.
>
Sure.

Thanks,
Lina

Re: [PATCH RFC 0/4] irqchip: qcom: add support for PDC interrupt controller

[Sudeep Holla]

On 25/01/18 20:05, Lina Iyer wrote:
> On Thu, Jan 25 2018 at 18:43 +0000, Sudeep Holla wrote:
>>

[...]

>> It may have some advantages to decentralize but will that not cause
>> issues in complex systems ? I assume even modem and other processors can
>> access and configure these wakeup interrupts. What happens if 2 such
>> processors try to access it at the same time ?
>>
> Every processor in the SoC has its own PDC and does exactly the same
> thing in SW. The hardware blocks are replicated for each of the
> 'subsystem' and they behave similarly.
> 

Ah that explains a lot.

-- 
Regards,
Sudeep