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

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

[Lina Iyer]

Changes in v2:
- Drviers will specify PDC pin as their interrupt and PDC as interrupt parent
- Updated driver to pick up PDC pin mappings from DT
- Cleanup driver per Marc's suggestions
- Addressed DT bindings comments from Rob H
- Addressed FTRACE comments from Steven R

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.

RFC v1: https://patchwork.kernel.org/patch/10180857/

Lina Iyer (3):
  drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
  dt-bindings/interrupt-controller: pdc: descibe PDC device binding
  drivers: irqchip: pdc: log PDC info in FTRACE

 .../bindings/interrupt-controller/qcom,pdc.txt     |  78 +++++
 drivers/irqchip/Kconfig                            |   9 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/qcom-pdc.c                         | 333 +++++++++++++++++++++
 include/trace/events/pdc.h                         |  55 ++++
 5 files changed, 476 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
 create mode 100644 drivers/irqchip/qcom-pdc.c
 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 v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Lina Iyer]

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

The Power Domain Controller (PDC) on QTI SoCs like SDM845 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. Only a subset of the processor's interrupts are
routed through the PDC to the GIC. The PDC powers on the processors'
domain, when in low power mode and replays 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>
---
 drivers/irqchip/Kconfig    |   9 ++
 drivers/irqchip/Makefile   |   1 +
 drivers/irqchip/qcom-pdc.c | 326 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 336 insertions(+)
 create mode 100644 drivers/irqchip/qcom-pdc.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index c70476b34a53..506c6aa7f0b4 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 (QTI) mobile chips.
+
 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..a392380eada6
--- /dev/null
+++ b/drivers/irqchip/qcom-pdc.c
@@ -0,0 +1,326 @@
+/* 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>
+
+#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
+
+struct pdc_pin_data {
+	int pin;
+	int hwirq;
+};
+
+static DEFINE_SPINLOCK(pdc_lock);
+static void __iomem *pdc_base;
+
+static inline void pdc_reg_write(int reg, u32 i, u32 val)
+{
+	writel_relaxed(val, pdc_base + reg + i * sizeof(u32));
+}
+
+static inline u32 pdc_reg_read(int reg, u32 i)
+{
+	return readl_relaxed(pdc_base + reg + i * sizeof(u32));
+}
+
+static inline void pdc_enable_intr(struct irq_data *d, bool on)
+{
+	int pin_out = d->hwirq;
+	u32 index, mask;
+	u32 enable;
+	unsigned long flags;
+
+	index = pin_out / 32;
+	mask = pin_out % 32;
+
+	spin_lock_irqsave(&pdc_lock, flags);
+	enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
+	enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
+	pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
+	spin_unlock_irqrestore(&pdc_lock, flags);
+}
+
+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);
+}
+
+/*
+ * 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 ) POLARITY
+ * 3'b0 00  Level sensitive active low    LOW
+ * 3'b0 01  Rising edge sensitive         NOT USED
+ * 3'b0 10  Falling edge sensitive        LOW
+ * 3'b0 11  Dual Edge sensitive           NOT USED
+ * 3'b1 00  Level senstive active High    HIGH
+ * 3'b1 01  Falling Edge sensitive        NOT USED
+ * 3'b1 10  Rising edge sensitive         HIGH
+ * 3'b1 11  Dual Edge sensitive           HIGH
+ */
+enum pdc_irq_config_bits {
+	PDC_POLARITY_LOW	= 0, // 0 00
+	PDC_FALLING_EDGE	= 2, // 0 10
+	PDC_POLARITY_HIGH	= 4, // 1 00
+	PDC_RISING_EDGE		= 6, // 1 10
+	PDC_DUAL_EDGE		= 7, // 1 11
+};
+
+/**
+ * qcom_pdc_gic_set_type: Configure PDC for the interrupt
+ *
+ * @d: the interrupt data
+ * @type: the interrupt type
+ *
+ * If @type is edge triggered, forward that as Rising edge as PDC
+ * takes care of converting falling edge to rising edge signal
+ * If @type is level, then forward that as level high as PDC
+ * takes care of converting falling edge to rising edge signal
+ */
+static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
+{
+	int pin_out = d->hwirq;
+	enum pdc_irq_config_bits pdc_type;
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		pdc_type = PDC_RISING_EDGE;
+		type = IRQ_TYPE_EDGE_RISING;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		pdc_type = PDC_FALLING_EDGE;
+		type = IRQ_TYPE_EDGE_RISING;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		pdc_type = PDC_DUAL_EDGE;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		pdc_type = PDC_POLARITY_HIGH;
+		type = IRQ_TYPE_LEVEL_HIGH;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		pdc_type = PDC_POLARITY_LOW;
+		type = IRQ_TYPE_LEVEL_HIGH;
+		break;
+	default:
+		pdc_type = PDC_POLARITY_HIGH;
+		break;
+	}
+
+	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
+
+	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_unmask		= qcom_pdc_gic_unmask,
+	.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 irq_hw_number_t get_irq_for_pin(int pin, struct pdc_pin_data *pdc_data)
+{
+	int i;
+
+	for (i = 0; pdc_data[i].pin >= 0; i++)
+		if (pdc_data[i].pin == pin)
+			return pdc_data[i].hwirq;
+
+	return pin;
+}
+
+static int qcom_pdc_translate(struct irq_domain *d,
+	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count < 3)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[1];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+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, parent_hwirq;
+	unsigned int type;
+	int ret;
+
+	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return -EINVAL;
+
+	parent_hwirq = get_irq_for_pin(hwirq, domain->host_data);
+	ret  = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+			&qcom_pdc_gic_chip, (void *)parent_hwirq);
+	if (ret)
+		return ret;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		type = IRQ_TYPE_EDGE_RISING;
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		type = IRQ_TYPE_LEVEL_HIGH;
+
+	parent_fwspec = *fwspec;
+	parent_fwspec.param[1] = parent_hwirq;
+	parent_fwspec.param[2] &= ~IRQ_TYPE_SENSE_MASK;
+	parent_fwspec.param[2] |= type;
+	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 int pdc_setup_pin_mapping(struct device_node *np,
+				struct pdc_pin_data **data)
+{
+	int ret;
+	int n, i, j, k, pins = 0;
+	int *val;
+	struct pdc_pin_data *map;
+
+	n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
+	if (!n || n % 3)
+		return -EINVAL;
+
+	val = kcalloc(n, sizeof(u32), GFP_KERNEL);
+	if (!val)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(np, "qcom,pdc-ranges", val, n);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < n; i += 3)
+		pins += val[i + 2];
+
+	if (pins > PDC_MAX_IRQS)
+		return -EFAULT;
+
+	map = kcalloc(pins + 1, sizeof(*map), GFP_KERNEL);
+	if (!map) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	for (i = 0, k = 0; i < n; i += 3) {
+		for (j = 0; j < val[i + 2]; j++, k++) {
+			map[k].pin = val[i] + j;
+			map[k].hwirq = val[i + 1] + j;
+		}
+	}
+
+	map[k].pin = -1;
+	*data = map;
+fail:
+	kfree(val);
+	return ret;
+}
+
+int qcom_pdc_init(struct device_node *node, struct device_node *parent)
+{
+	struct irq_domain *parent_domain, *pdc_domain;
+	struct pdc_pin_data *pdc_data = NULL;
+	int ret;
+
+	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;
+	}
+
+	ret = pdc_setup_pin_mapping(node, &pdc_data);
+	if (ret) {
+		pr_err("failed to setup PDC pin mapping\n");
+		goto failure;
+	}
+
+	pdc_domain = irq_domain_create_hierarchy(parent_domain, 0, PDC_MAX_IRQS,
+					of_fwnode_handle(node), &qcom_pdc_ops,
+					pdc_data);
+	if (!pdc_domain) {
+		pr_err("GIC domain add failed\n");
+		ret = -ENOMEM;
+		goto failure;
+	}
+
+	return 0;
+
+failure:
+	kfree(pdc_data);
+	iounmap(pdc_base);
+	return ret;
+}
+
+IRQCHIP_DECLARE(pdc_sdm845, "qcom,sdm845-pdc", qcom_pdc_init);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC v2 2/3] 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     | 78 ++++++++++++++++++++++
 1 file changed, 78 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..7bf40cb6a4f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
@@ -0,0 +1,78 @@
+PDC interrupt controller
+
+Qualcomm Technologies Inc. SoCs based on the RPM Hardened architecture 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. Drivers requiring
+wakeup capabilities of their device interrupts routed through the PDC, must
+specify PDC as their interrupt controller and request the PDC port associated
+with the GIC interrupt. See example below.
+
+Properties:
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: Should contain "qcom,<soc>-pdc"
+		    - "qcom,sdm845-pdc": For SDM845
+
+- 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.
+		    The value must match that of the parent interrupt
+		    controller defined in the DT.
+		    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.
+
+- qcom,pdc-range:
+	Usage: required
+	Value type: <u32 array>
+	Definition: Specifies the PDC pin offset and the number of PDC ports.
+		    The tuples indicates the valid mapping of valid PDC ports
+		    and their hwirq mapping.
+		    The first element of the tuple is the staring PDC port num.
+		    The second element is the hwirq number for the PDC port.
+		    The third element is the number of elements in sequence.
+
+Example:
+
+	pdc: interrupt-controller@b220000 {
+		compatible = "qcom,sdm845-pdc";
+		reg = <0xb220000 0x30000>;
+		qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
+		#interrupt-cells = <3>;
+		interrupt-parent = <&intc>;
+		interrupt-controller;
+	};
+
+The DT binding of a device that wants to use the GIC SPI 514 as a wakeup
+interrupt, would look like this -
+
+	wake-device {
+		[...]
+		interrupt-parent = <&pdc>;
+		interrupt = <0 2 0>;
+	};
+
+[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

[PATCH RFC v2 3/3] 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 |  7 ++++++
 include/trace/events/pdc.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 include/trace/events/pdc.h

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index a392380eada6..7f177ad88713 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"
 
 #define PDC_MAX_IRQS		126
 
@@ -68,6 +70,8 @@ static inline void pdc_enable_intr(struct irq_data *d, bool on)
 	enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
 	pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
 	spin_unlock_irqrestore(&pdc_lock, flags);
+
+	trace_irq_pin_config(PDC_ENTRY, pin_out, (u64)d->chip_data, 0, on);
 }
 
 static void qcom_pdc_gic_mask(struct irq_data *d)
@@ -149,6 +153,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 
 	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
 
+	trace_irq_pin_config(PDC_TYPE_CONFIG, pin_out, (u64)d->chip_data,
+				pdc_type, 0);
+
 	return irq_chip_set_type_parent(d, type);
 }
 
diff --git a/include/trace/events/pdc.h b/include/trace/events/pdc.h
new file mode 100644
index 000000000000..0e894bbc9e85
--- /dev/null
+++ b/include/trace/events/pdc.h
@@ -0,0 +1,55 @@
+/* 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>
+
+#define PDC_ENTRY		1
+#define PDC_TYPE_CONFIG		2
+
+TRACE_EVENT(irq_pin_config,
+
+	TP_PROTO(u32 func, int pin, u64 hwirq, u32 type, u32 enable),
+
+	TP_ARGS(func, pin, hwirq, type, enable),
+
+	TP_STRUCT__entry(
+		__field(u32, func)
+		__field(int, pin)
+		__field(u64, hwirq)
+		__field(u32, type)
+		__field(u32, enable)
+	),
+
+	TP_fast_assign(
+		__entry->func = func;
+		__entry->pin = pin;
+		__entry->hwirq = hwirq;
+		__entry->type = type;
+		__entry->enable = enable;
+	),
+
+	TP_printk("%s hwirq:%lu pin:%d type:%u enable:%u",
+		print_symbolic(__entry->func,
+				{ PDC_ENTRY, "pdc_entry" },
+				{ PDC_TYPE_CONFIG, "pdc_type_config" }),
+		__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 v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Marc Zyngier]

Hi Lina,

On 02/02/18 14:21, Lina Iyer wrote:
> From : Archana Sathyakumar <asathyak@codeaurora.org>
> 
> The Power Domain Controller (PDC) on QTI SoCs like SDM845 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. Only a subset of the processor's interrupts are
> routed through the PDC to the GIC. The PDC powers on the processors'
> domain, when in low power mode and replays 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>
> ---
>  drivers/irqchip/Kconfig    |   9 ++
>  drivers/irqchip/Makefile   |   1 +
>  drivers/irqchip/qcom-pdc.c | 326 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 336 insertions(+)
>  create mode 100644 drivers/irqchip/qcom-pdc.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index c70476b34a53..506c6aa7f0b4 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 (QTI) mobile chips.
> +
>  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..a392380eada6
> --- /dev/null
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -0,0 +1,326 @@
> +/* 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>
> +
> +#define PDC_MAX_IRQS		126

Is that an absolute, architectural maximum? Or should it come from the
DT (being the sum of all ranges that are provided by this PDC)?

> +
> +#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
> +
> +struct pdc_pin_data {
> +	int pin;
> +	int hwirq;
> +};

I seriously doubt you need this structure, see below.

> +
> +static DEFINE_SPINLOCK(pdc_lock);
> +static void __iomem *pdc_base;

You only have one of those per SoC?

> +
> +static inline void pdc_reg_write(int reg, u32 i, u32 val)
> +{
> +	writel_relaxed(val, pdc_base + reg + i * sizeof(u32));
> +}
> +
> +static inline u32 pdc_reg_read(int reg, u32 i)
> +{
> +	return readl_relaxed(pdc_base + reg + i * sizeof(u32));
> +}
> +
> +static inline void pdc_enable_intr(struct irq_data *d, bool on)
> +{
> +	int pin_out = d->hwirq;
> +	u32 index, mask;
> +	u32 enable;
> +	unsigned long flags;
> +
> +	index = pin_out / 32;
> +	mask = pin_out % 32;
> +
> +	spin_lock_irqsave(&pdc_lock, flags);
> +	enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
> +	enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
> +	pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
> +	spin_unlock_irqrestore(&pdc_lock, flags);
> +}
> +
> +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);
> +}
> +
> +/*
> + * 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 ) POLARITY
> + * 3'b0 00  Level sensitive active low    LOW
> + * 3'b0 01  Rising edge sensitive         NOT USED
> + * 3'b0 10  Falling edge sensitive        LOW
> + * 3'b0 11  Dual Edge sensitive           NOT USED
> + * 3'b1 00  Level senstive active High    HIGH
> + * 3'b1 01  Falling Edge sensitive        NOT USED
> + * 3'b1 10  Rising edge sensitive         HIGH
> + * 3'b1 11  Dual Edge sensitive           HIGH
> + */
> +enum pdc_irq_config_bits {
> +	PDC_POLARITY_LOW	= 0, // 0 00
> +	PDC_FALLING_EDGE	= 2, // 0 10
> +	PDC_POLARITY_HIGH	= 4, // 1 00
> +	PDC_RISING_EDGE		= 6, // 1 10
> +	PDC_DUAL_EDGE		= 7, // 1 11
> +};
> +
> +/**
> + * qcom_pdc_gic_set_type: Configure PDC for the interrupt
> + *
> + * @d: the interrupt data
> + * @type: the interrupt type
> + *
> + * If @type is edge triggered, forward that as Rising edge as PDC
> + * takes care of converting falling edge to rising edge signal
> + * If @type is level, then forward that as level high as PDC
> + * takes care of converting falling edge to rising edge signal
> + */
> +static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
> +{
> +	int pin_out = d->hwirq;
> +	enum pdc_irq_config_bits pdc_type;
> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		pdc_type = PDC_RISING_EDGE;
> +		type = IRQ_TYPE_EDGE_RISING;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		pdc_type = PDC_FALLING_EDGE;
> +		type = IRQ_TYPE_EDGE_RISING;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		pdc_type = PDC_DUAL_EDGE;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		pdc_type = PDC_POLARITY_HIGH;
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		pdc_type = PDC_POLARITY_LOW;
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +		break;
> +	default:
> +		pdc_type = PDC_POLARITY_HIGH;
> +		break;

If this default clause triggers, something is pretty wrong. You may want
to warn and bail out instead.

> +	}
> +
> +	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
> +
> +	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_unmask		= qcom_pdc_gic_unmask,
> +	.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 irq_hw_number_t get_irq_for_pin(int pin, struct pdc_pin_data *pdc_data)
> +{
> +	int i;
> +
> +	for (i = 0; pdc_data[i].pin >= 0; i++)
> +		if (pdc_data[i].pin == pin)
> +			return pdc_data[i].hwirq;
> +
> +	return pin;
> +}

This is the function that irks me. The DT already gives you a nice set
of ranges with all the information you need. And yet, you expand the
whole thing into at most 127 structures, wasting memory and making the
search time a function of the number of interrupts instead of being that
of the number of regions. Not very nice. How about something like this
(untested):

struct pin_region {
	u32 pin_base;
	u32 parent_base;
	u32 cnt;
};

struct pin_region *pin_regions;
int pin_region_count;

static int pdc_pin_to_parent_hwirq(int pin)
{
	int i;

	for (i = 0; i < pin_region_count; i++) {
		if (pin >= pin_regions[i].pin_base &&
		    pin < (pin_regions[i].pin_base + pin_regions[i].cnt)
			return (pin_regions[i].parent_base + pin -
				pin_regions[i].pin_base);
	}

	WARN();
	return -1;
}

Less memory, less comparisons.

> +
> +static int qcom_pdc_translate(struct irq_domain *d,
> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count < 3)
> +			return -EINVAL;

Why 3? Reading the DT binding, this is indeed set to 3 without any
reason. I'd suggest this becomes 2, encoding the pin number and the
trigger information, as the leading 0 is quite useless. Yes, I know
other examples in the kernel are using this 0, and that was a
consequence of retrofitting the omitted interrupt controllers (back in
the days of the stupid gic_arch_extn...). Don't do that.

> +
> +		*hwirq = fwspec->param[1];
> +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;

... and fix this accordingly.

> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +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, parent_hwirq;
> +	unsigned int type;
> +	int ret;
> +
> +	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return -EINVAL;
> +
> +	parent_hwirq = get_irq_for_pin(hwirq, domain->host_data);
> +	ret  = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +			&qcom_pdc_gic_chip, (void *)parent_hwirq);
> +	if (ret)
> +		return ret;
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +
> +	parent_fwspec = *fwspec;
> +	parent_fwspec.param[1] = parent_hwirq;
> +	parent_fwspec.param[2] &= ~IRQ_TYPE_SENSE_MASK;
> +	parent_fwspec.param[2] |= type;
> +	parent_fwspec.fwnode = domain->parent->fwnode;

This becomes:

	parent_fwspec.fwnode = domain->parent->fwnode;
	parent_fwspec.param_count = 3; // That's what the GIC wants
	parent_fwspec.param[0] = 0; //GIC_SPI
	parent_fwspec.param[1] = parent_hwirq;
	parent_fwspec.param[2] = type;

> +
> +	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 int pdc_setup_pin_mapping(struct device_node *np,
> +				struct pdc_pin_data **data)
> +{
> +	int ret;
> +	int n, i, j, k, pins = 0;
> +	int *val;
> +	struct pdc_pin_data *map;
> +
> +	n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
> +	if (!n || n % 3)
> +		return -EINVAL;
> +
> +	val = kcalloc(n, sizeof(u32), GFP_KERNEL);
> +	if (!val)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(np, "qcom,pdc-ranges", val, n);
> +	if (ret)
> +		return ret;

Memory leak.

> +
> +	for (i = 0; i < n; i += 3)
> +		pins += val[i + 2];
> +
> +	if (pins > PDC_MAX_IRQS)
> +		return -EFAULT;

EFAULT is for an actual page fault. If you get one here, you have
bigger problems. EINVAL is better. is You're also leaking memory.

> +
> +	map = kcalloc(pins + 1, sizeof(*map), GFP_KERNEL);
> +	if (!map) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	for (i = 0, k = 0; i < n; i += 3) {
> +		for (j = 0; j < val[i + 2]; j++, k++) {
> +			map[k].pin = val[i] + j;
> +			map[k].hwirq = val[i + 1] + j;
> +		}
> +	}

That's the thing that needs to go. Just store the ranges as exposed by
the DT, maybe with some checking that there is no overlap in the
ranges (not that it matters much...).

> +
> +	map[k].pin = -1;
> +	*data = map;
> +fail:
> +	kfree(val);
> +	return ret;
> +}
> +
> +int qcom_pdc_init(struct device_node *node, struct device_node *parent)
> +{
> +	struct irq_domain *parent_domain, *pdc_domain;
> +	struct pdc_pin_data *pdc_data = NULL;
> +	int ret;
> +
> +	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;
> +	}
> +
> +	ret = pdc_setup_pin_mapping(node, &pdc_data);
> +	if (ret) {
> +		pr_err("failed to setup PDC pin mapping\n");
> +		goto failure;
> +	}
> +
> +	pdc_domain = irq_domain_create_hierarchy(parent_domain, 0, PDC_MAX_IRQS,
> +					of_fwnode_handle(node), &qcom_pdc_ops,
> +					pdc_data);
> +	if (!pdc_domain) {
> +		pr_err("GIC domain add failed\n");
> +		ret = -ENOMEM;
> +		goto failure;
> +	}
> +
> +	return 0;
> +
> +failure:
> +	kfree(pdc_data);
> +	iounmap(pdc_base);
> +	return ret;
> +}
> +
> +IRQCHIP_DECLARE(pdc_sdm845, "qcom,sdm845-pdc", qcom_pdc_init);
> 

Thanks,

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

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Thomas Gleixner]

On Fri, 2 Feb 2018, Lina Iyer wrote:
> +static inline void pdc_enable_intr(struct irq_data *d, bool on)
> +{
> +	int pin_out = d->hwirq;
> +	u32 index, mask;
> +	u32 enable;
> +	unsigned long flags;
> +
> +	index = pin_out / 32;
> +	mask = pin_out % 32;
> +
> +	spin_lock_irqsave(&pdc_lock, flags);

Please make this a raw spinlock. Aside of that the _irqsave() is pointless
as the chip callbacks are already called with interrupts disabled.

> +	enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
> +	enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);

You really should cache the enable register content to avoid the read back

> +	pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
> +	spin_unlock_irqrestore(&pdc_lock, flags);
> +}
> +
> +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);
> +}
> +
> +/*
> + * 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 ) POLARITY
> + * 3'b0 00  Level sensitive active low    LOW
> + * 3'b0 01  Rising edge sensitive         NOT USED
> + * 3'b0 10  Falling edge sensitive        LOW
> + * 3'b0 11  Dual Edge sensitive           NOT USED
> + * 3'b1 00  Level senstive active High    HIGH
> + * 3'b1 01  Falling Edge sensitive        NOT USED
> + * 3'b1 10  Rising edge sensitive         HIGH
> + * 3'b1 11  Dual Edge sensitive           HIGH
> + */
> +enum pdc_irq_config_bits {
> +	PDC_POLARITY_LOW	= 0, // 0 00

What's wrong with

       PDC_POLARITY_LOW		= 000b,
       PDC_FALLING_EDGE		= 010b,

instead of decimal and these weird comments ?

> +static irq_hw_number_t get_irq_for_pin(int pin, struct pdc_pin_data *pdc_data)
> +{
> +	int i;
> +
> +	for (i = 0; pdc_data[i].pin >= 0; i++)
> +		if (pdc_data[i].pin == pin)
> +			return pdc_data[i].hwirq;

Please let the for() loop have braces. See:

       https://marc.info/?l=linux-kernel&m=148467980905537&w=2

> +
> +	return pin;
> +}
> +
> +static int qcom_pdc_translate(struct irq_domain *d,
> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)

Please align the arguments right of the opening brace:

static int qcom_pdc_translate(struct irq_domain *d,
			      struct irq_fwspec *fwspec, unsigned long *hwirq,
			      unsigned int *type)


> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count < 3)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[1];
> +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int qcom_pdc_alloc(struct irq_domain *domain,
> +			unsigned int virq, unsigned int nr_irqs, void *data)

Ditto

> +static int pdc_setup_pin_mapping(struct device_node *np,
> +				struct pdc_pin_data **data)
> +{
> +	int ret;
> +	int n, i, j, k, pins = 0;
> +	int *val;

I have no idea what's the rationale behind these 3 lines of int declarations.

> +	struct pdc_pin_data *map;
> +
> +	n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
> +	if (!n || n % 3)
> +		return -EINVAL;
> +
> +	val = kcalloc(n, sizeof(u32), GFP_KERNEL);
> +	if (!val)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(np, "qcom,pdc-ranges", val, n);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < n; i += 3)
> +		pins += val[i + 2];
> +
> +	if (pins > PDC_MAX_IRQS)
> +		return -EFAULT;
> +
> +	map = kcalloc(pins + 1, sizeof(*map), GFP_KERNEL);
> +	if (!map) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	for (i = 0, k = 0; i < n; i += 3) {
> +		for (j = 0; j < val[i + 2]; j++, k++) {
> +			map[k].pin = val[i] + j;
> +			map[k].hwirq = val[i + 1] + j;
> +		}
> +	}

This all is really horrible to read. First of all the val[] array. That can
be represented in a structure, right? Without looking at the DT patch the
code lets me assume:

   struct pdcrange {
   	u32	pin;
	u32	hwirq;
	u32	numpins;
	u32	unused;
   };

So the above becomes:

	nelm = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
	if (!nelm || nelm % 3)
		return -EINVAL;

	nranges = nelm / 4;	
	ranges = kcalloc(nranges, sizeof(*prng), GFP_KERNEL);
	if (!ranges)
		return -ENOMEM;

which makes the pin counting readable:

	for (i = 0; i < nranges; i++)
		pins += ranges[i].numpins;

And then allows to write the map initialization with:

	*data = map;
	for (i = 0; i < nranges; i++) {
		for (j = 0; j < ranges[i].numpins; j++, map++) {
			map->pin = ranges[i].pin + j;
			map->hwirq = ranges[i].hwirq + j;
		}
	}
	map->pin = -1;

Hmm?

> +int qcom_pdc_init(struct device_node *node, struct device_node *parent)
> +{
> +	struct irq_domain *parent_domain, *pdc_domain;
> +	struct pdc_pin_data *pdc_data = NULL;
> +	int ret;
> +
> +	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;
> +	}
> +
> +	ret = pdc_setup_pin_mapping(node, &pdc_data);

You can let pdc_setup_pin_mapping() return a pointer to pdc_data or NULL
and check the pointer for ERR or NULL instead of having that ret
indirection.

> +	if (ret) {
> +		pr_err("failed to setup PDC pin mapping\n");
> +		goto failure;
> +	}
> +
> +	pdc_domain = irq_domain_create_hierarchy(parent_domain, 0, PDC_MAX_IRQS,
> +					of_fwnode_handle(node), &qcom_pdc_ops,
> +					pdc_data);

See comment about argument alignement above. Hint: shortening the *_domain
variable names helps.

Thanks,

	tglx

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

[Thomas Gleixner]

On Fri, 2 Feb 2018, Lina Iyer wrote:
> +++ b/include/trace/events/pdc.h
> @@ -0,0 +1,55 @@
> +/* 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.

Can you please use the proper SPDX identifiers instead of the boiler plate?
Same for the driver source file.

> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM pdc
> +
> +#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_PDC_H_
> +
> +#include <linux/tracepoint.h>
> +
> +#define PDC_ENTRY		1
> +#define PDC_TYPE_CONFIG		2
> +
> +TRACE_EVENT(irq_pin_config,

This is really a too generic name for a PDC specific breakpoint.

Aside of that the question is whether this really justifies a trace
point. Wouldn't it be sufficient to use the GENERIC_IRQ_DEBUGFS
infrastructure to make this accessible via debugfs?

Thanks,

	tglx

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

[Marc Zyngier]

On 02/02/18 14:21, 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     | 78 ++++++++++++++++++++++
>  1 file changed, 78 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..7bf40cb6a4f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> @@ -0,0 +1,78 @@
> +PDC interrupt controller
> +
> +Qualcomm Technologies Inc. SoCs based on the RPM Hardened architecture 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. Drivers requiring
> +wakeup capabilities of their device interrupts routed through the PDC, must
> +specify PDC as their interrupt controller and request the PDC port associated
> +with the GIC interrupt. See example below.
> +
> +Properties:
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: Should contain "qcom,<soc>-pdc"
> +		    - "qcom,sdm845-pdc": For SDM845
> +
> +- 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.
> +		    The value must match that of the parent interrupt
> +		    controller defined in the DT.
> +		    The encoding of these cells are same as described in [1].

There shouldn't be such a requirement. These are two independent pieces
of HW, and the first parameter doesn't mean anything for the PDC.

> +
> +- 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.
> +
> +- qcom,pdc-range:
> +	Usage: required
> +	Value type: <u32 array>
> +	Definition: Specifies the PDC pin offset and the number of PDC ports.
> +		    The tuples indicates the valid mapping of valid PDC ports
> +		    and their hwirq mapping.
> +		    The first element of the tuple is the staring PDC port num.
> +		    The second element is the hwirq number for the PDC port.
> +		    The third element is the number of elements in sequence.
> +
> +Example:
> +
> +	pdc: interrupt-controller@b220000 {
> +		compatible = "qcom,sdm845-pdc";
> +		reg = <0xb220000 0x30000>;
> +		qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
> +		#interrupt-cells = <3>;
> +		interrupt-parent = <&intc>;
> +		interrupt-controller;
> +	};
> +
> +The DT binding of a device that wants to use the GIC SPI 514 as a wakeup
> +interrupt, would look like this -
> +
> +	wake-device {
> +		[...]
> +		interrupt-parent = <&pdc>;
> +		interrupt = <0 2 0>;

Again: 0 is not a valid trigger value.

> +	};
> +
> +[1]. Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> 

Thanks,

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

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

[Steven Rostedt]

On Fri,  2 Feb 2018 07:22:00 -0700
Lina Iyer <ilina@codeaurora.org> wrote:

Hi Lina,

This looks really good. I have one nit below.


> 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 |  7 ++++++
>  include/trace/events/pdc.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 include/trace/events/pdc.h
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index a392380eada6..7f177ad88713 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"
>  
>  #define PDC_MAX_IRQS		126
>  
> @@ -68,6 +70,8 @@ static inline void pdc_enable_intr(struct irq_data *d, bool on)
>  	enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
>  	pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
>  	spin_unlock_irqrestore(&pdc_lock, flags);
> +
> +	trace_irq_pin_config(PDC_ENTRY, pin_out, (u64)d->chip_data, 0, on);
>  }
>  
>  static void qcom_pdc_gic_mask(struct irq_data *d)
> @@ -149,6 +153,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>  
>  	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
>  
> +	trace_irq_pin_config(PDC_TYPE_CONFIG, pin_out, (u64)d->chip_data,
> +				pdc_type, 0);

I wonder if it makes more sense to just pass 'd' into the trace events,
and then do the dereference there. The reason is to try to get as much
code out of the calling path as possible. Even though trace events use
jump labels and have no conditional branches, the code to call the
function is still within the code using the trace events. By passing in
'd' and doing the redirect in the trace event code, we remove the
setting up of the redirect from the caller, and save some cache lines
in the process.

-- Steve



> +
>  	return irq_chip_set_type_parent(d, type);
>  }
>  
> diff --git a/include/trace/events/pdc.h b/include/trace/events/pdc.h
> new file mode 100644
> index 000000000000..0e894bbc9e85
> --- /dev/null
> +++ b/include/trace/events/pdc.h
> @@ -0,0 +1,55 @@
> +/* 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>
> +
> +#define PDC_ENTRY		1
> +#define PDC_TYPE_CONFIG		2
> +
> +TRACE_EVENT(irq_pin_config,
> +
> +	TP_PROTO(u32 func, int pin, u64 hwirq, u32 type, u32 enable),
> +
> +	TP_ARGS(func, pin, hwirq, type, enable),
> +
> +	TP_STRUCT__entry(
> +		__field(u32, func)
> +		__field(int, pin)
> +		__field(u64, hwirq)
> +		__field(u32, type)
> +		__field(u32, enable)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->func = func;
> +		__entry->pin = pin;
> +		__entry->hwirq = hwirq;
> +		__entry->type = type;
> +		__entry->enable = enable;
> +	),
> +
> +	TP_printk("%s hwirq:%lu pin:%d type:%u enable:%u",
> +		print_symbolic(__entry->func,
> +				{ PDC_ENTRY, "pdc_entry" },
> +				{ PDC_TYPE_CONFIG, "pdc_type_config" }),
> +		__entry->pin, __entry->hwirq, __entry->type, __entry->enable)
> +);
> +
> +#endif
> +#define TRACE_INCLUDE_FILE pdc
> +#include <trace/define_trace.h>

Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Lina Iyer]

On Fri, Feb 02 2018 at 16:21 +0000, Marc Zyngier wrote:
>Hi Lina,
>
>On 02/02/18 14:21, Lina Iyer wrote:
>> From : Archana Sathyakumar <asathyak@codeaurora.org>
>>
>> The Power Domain Controller (PDC) on QTI SoCs like SDM845 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. Only a subset of the processor's interrupts are
>> routed through the PDC to the GIC. The PDC powers on the processors'
>> domain, when in low power mode and replays 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>
>> ---
>> +struct pdc_pin_data {
>> +	int pin;
>> +	int hwirq;
>> +};
>
>I seriously doubt you need this structure, see below.
>
>> +
>> +static DEFINE_SPINLOCK(pdc_lock);
>> +static void __iomem *pdc_base;
>
>You only have one of those per SoC?
>
There is only one that Linux can control.

>> +static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>> +{
>> +	int pin_out = d->hwirq;
>> +	enum pdc_irq_config_bits pdc_type;
>> +
>> +	switch (type) {
>> +	case IRQ_TYPE_EDGE_RISING:
>> +		pdc_type = PDC_RISING_EDGE;
>> +		type = IRQ_TYPE_EDGE_RISING;
>> +		break;
>> +	case IRQ_TYPE_EDGE_FALLING:
>> +		pdc_type = PDC_FALLING_EDGE;
>> +		type = IRQ_TYPE_EDGE_RISING;
>> +		break;
>> +	case IRQ_TYPE_EDGE_BOTH:
>> +		pdc_type = PDC_DUAL_EDGE;
>> +		break;
>> +	case IRQ_TYPE_LEVEL_HIGH:
>> +		pdc_type = PDC_POLARITY_HIGH;
>> +		type = IRQ_TYPE_LEVEL_HIGH;
>> +		break;
>> +	case IRQ_TYPE_LEVEL_LOW:
>> +		pdc_type = PDC_POLARITY_LOW;
>> +		type = IRQ_TYPE_LEVEL_HIGH;
>> +		break;
>> +	default:
>> +		pdc_type = PDC_POLARITY_HIGH;
>> +		break;
>
>If this default clause triggers, something is pretty wrong. You may want
>to warn and bail out instead.
>
The hardware defaults to this. I can bail out as well.

>> +	}
>> +
>> +	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
>> +
>> +	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_unmask		= qcom_pdc_gic_unmask,
>> +	.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 irq_hw_number_t get_irq_for_pin(int pin, struct pdc_pin_data *pdc_data)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; pdc_data[i].pin >= 0; i++)
>> +		if (pdc_data[i].pin == pin)
>> +			return pdc_data[i].hwirq;
>> +
>> +	return pin;
>> +}
>
>This is the function that irks me. The DT already gives you a nice set
>of ranges with all the information you need. And yet, you expand the
>whole thing into at most 127 structures, wasting memory and making the
>search time a function of the number of interrupts instead of being that
>of the number of regions. Not very nice. How about something like this
>(untested):
>
Duh! Why didn't I think of this.. Thanks.

>struct pin_region {
>	u32 pin_base;
>	u32 parent_base;
>	u32 cnt;
>};
>
>struct pin_region *pin_regions;
>int pin_region_count;
>
>static int pdc_pin_to_parent_hwirq(int pin)
>{
>	int i;
>
>	for (i = 0; i < pin_region_count; i++) {
>		if (pin >= pin_regions[i].pin_base &&
>		    pin < (pin_regions[i].pin_base + pin_regions[i].cnt)
>			return (pin_regions[i].parent_base + pin -
>				pin_regions[i].pin_base);
>	}
>
>	WARN();
>	return -1;
>}
>
>Less memory, less comparisons.
>
Agreed.

>> +
>> +static int qcom_pdc_translate(struct irq_domain *d,
>> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
>> +{
>> +	if (is_of_node(fwspec->fwnode)) {
>> +		if (fwspec->param_count < 3)
>> +			return -EINVAL;
>
>Why 3? Reading the DT binding, this is indeed set to 3 without any
>reason. I'd suggest this becomes 2, encoding the pin number and the
>trigger information, as the leading 0 is quite useless. Yes, I know
>other examples in the kernel are using this 0, and that was a
>consequence of retrofitting the omitted interrupt controllers (back in
>the days of the stupid gic_arch_extn...). Don't do that.
>
>> +
>> +		*hwirq = fwspec->param[1];
>> +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
>
>... and fix this accordingly.
>
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +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, parent_hwirq;
>> +	unsigned int type;
>> +	int ret;
>> +
>> +	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
>> +	if (ret)
>> +		return -EINVAL;
>> +
>> +	parent_hwirq = get_irq_for_pin(hwirq, domain->host_data);
>> +	ret  = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
>> +			&qcom_pdc_gic_chip, (void *)parent_hwirq);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>> +		type = IRQ_TYPE_EDGE_RISING;
>> +
>> +	if (type & IRQ_TYPE_LEVEL_MASK)
>> +		type = IRQ_TYPE_LEVEL_HIGH;
>> +
>> +	parent_fwspec = *fwspec;
>> +	parent_fwspec.param[1] = parent_hwirq;
>> +	parent_fwspec.param[2] &= ~IRQ_TYPE_SENSE_MASK;
>> +	parent_fwspec.param[2] |= type;
>> +	parent_fwspec.fwnode = domain->parent->fwnode;
>
>This becomes:
>
>	parent_fwspec.fwnode = domain->parent->fwnode;
>	parent_fwspec.param_count = 3; // That's what the GIC wants
>	parent_fwspec.param[0] = 0; //GIC_SPI
>	parent_fwspec.param[1] = parent_hwirq;
>	parent_fwspec.param[2] = type;
>
Sure.

>> +
>> +	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 int pdc_setup_pin_mapping(struct device_node *np,
>> +				struct pdc_pin_data **data)
>> +{
>> +	int ret;
>> +	int n, i, j, k, pins = 0;
>> +	int *val;
>> +	struct pdc_pin_data *map;
>> +
>> +	n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
>> +	if (!n || n % 3)
>> +		return -EINVAL;
>> +
>> +	val = kcalloc(n, sizeof(u32), GFP_KERNEL);
>> +	if (!val)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_u32_array(np, "qcom,pdc-ranges", val, n);
>> +	if (ret)
>> +		return ret;
>
>Memory leak.
>
Ok.

>> +
>> +	for (i = 0; i < n; i += 3)
>> +		pins += val[i + 2];
>> +
>> +	if (pins > PDC_MAX_IRQS)
>> +		return -EFAULT;
>
>EFAULT is for an actual page fault. If you get one here, you have
>bigger problems. EINVAL is better. is You're also leaking memory.
>
Ok.
>> +
>> +	map = kcalloc(pins + 1, sizeof(*map), GFP_KERNEL);
>> +	if (!map) {
>> +		ret = -ENOMEM;
>> +		goto fail;
>> +	}
>> +
>> +	for (i = 0, k = 0; i < n; i += 3) {
>> +		for (j = 0; j < val[i + 2]; j++, k++) {
>> +			map[k].pin = val[i] + j;
>> +			map[k].hwirq = val[i + 1] + j;
>> +		}
>> +	}
>
>That's the thing that needs to go. Just store the ranges as exposed by
>the DT, maybe with some checking that there is no overlap in the
>ranges (not that it matters much...).
>
Makes sense.

Thanks,
Lina

Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Lina Iyer]

All valid comments. Will fix them all in the next rev.

Thanks Thomas.

-- Lina

On Fri, Feb 02 2018 at 15:37 +0000, Thomas Gleixner wrote:
>On Fri, 2 Feb 2018, Lina Iyer wrote:
>> +static inline void pdc_enable_intr(struct irq_data *d, bool on)
>> +{
>> +	int pin_out = d->hwirq;
>> +	u32 index, mask;
>> +	u32 enable;
>> +	unsigned long flags;
>> +
>> +	index = pin_out / 32;
>> +	mask = pin_out % 32;
>> +
>> +	spin_lock_irqsave(&pdc_lock, flags);
>
>Please make this a raw spinlock. Aside of that the _irqsave() is pointless
>as the chip callbacks are already called with interrupts disabled.
>
>> +	enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
>> +	enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
>
>You really should cache the enable register content to avoid the read back
>
>> +	pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
>> +	spin_unlock_irqrestore(&pdc_lock, flags);
>> +}
>> +
>> +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);
>> +}
>> +
>> +/*
>> + * 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 ) POLARITY
>> + * 3'b0 00  Level sensitive active low    LOW
>> + * 3'b0 01  Rising edge sensitive         NOT USED
>> + * 3'b0 10  Falling edge sensitive        LOW
>> + * 3'b0 11  Dual Edge sensitive           NOT USED
>> + * 3'b1 00  Level senstive active High    HIGH
>> + * 3'b1 01  Falling Edge sensitive        NOT USED
>> + * 3'b1 10  Rising edge sensitive         HIGH
>> + * 3'b1 11  Dual Edge sensitive           HIGH
>> + */
>> +enum pdc_irq_config_bits {
>> +	PDC_POLARITY_LOW	= 0, // 0 00
>
>What's wrong with
>
>       PDC_POLARITY_LOW		= 000b,
>       PDC_FALLING_EDGE		= 010b,
>
>instead of decimal and these weird comments ?
>
>> +static irq_hw_number_t get_irq_for_pin(int pin, struct pdc_pin_data *pdc_data)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; pdc_data[i].pin >= 0; i++)
>> +		if (pdc_data[i].pin == pin)
>> +			return pdc_data[i].hwirq;
>
>Please let the for() loop have braces. See:
>
>       https://marc.info/?l=linux-kernel&m=148467980905537&w=2
>
>> +
>> +	return pin;
>> +}
>> +
>> +static int qcom_pdc_translate(struct irq_domain *d,
>> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
>
>Please align the arguments right of the opening brace:
>
>static int qcom_pdc_translate(struct irq_domain *d,
>			      struct irq_fwspec *fwspec, unsigned long *hwirq,
>			      unsigned int *type)
>
>
>> +{
>> +	if (is_of_node(fwspec->fwnode)) {
>> +		if (fwspec->param_count < 3)
>> +			return -EINVAL;
>> +
>> +		*hwirq = fwspec->param[1];
>> +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int qcom_pdc_alloc(struct irq_domain *domain,
>> +			unsigned int virq, unsigned int nr_irqs, void *data)
>
>Ditto
>
>> +static int pdc_setup_pin_mapping(struct device_node *np,
>> +				struct pdc_pin_data **data)
>> +{
>> +	int ret;
>> +	int n, i, j, k, pins = 0;
>> +	int *val;
>
>I have no idea what's the rationale behind these 3 lines of int declarations.
>
>> +	struct pdc_pin_data *map;
>> +
>> +	n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
>> +	if (!n || n % 3)
>> +		return -EINVAL;
>> +
>> +	val = kcalloc(n, sizeof(u32), GFP_KERNEL);
>> +	if (!val)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_u32_array(np, "qcom,pdc-ranges", val, n);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < n; i += 3)
>> +		pins += val[i + 2];
>> +
>> +	if (pins > PDC_MAX_IRQS)
>> +		return -EFAULT;
>> +
>> +	map = kcalloc(pins + 1, sizeof(*map), GFP_KERNEL);
>> +	if (!map) {
>> +		ret = -ENOMEM;
>> +		goto fail;
>> +	}
>> +
>> +	for (i = 0, k = 0; i < n; i += 3) {
>> +		for (j = 0; j < val[i + 2]; j++, k++) {
>> +			map[k].pin = val[i] + j;
>> +			map[k].hwirq = val[i + 1] + j;
>> +		}
>> +	}
>
>This all is really horrible to read. First of all the val[] array. That can
>be represented in a structure, right? Without looking at the DT patch the
>code lets me assume:
>
>   struct pdcrange {
>   	u32	pin;
>	u32	hwirq;
>	u32	numpins;
>	u32	unused;
>   };
>
>So the above becomes:
>
>	nelm = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
>	if (!nelm || nelm % 3)
>		return -EINVAL;
>
>	nranges = nelm / 4;
>	ranges = kcalloc(nranges, sizeof(*prng), GFP_KERNEL);
>	if (!ranges)
>		return -ENOMEM;
>
>which makes the pin counting readable:
>
>	for (i = 0; i < nranges; i++)
>		pins += ranges[i].numpins;
>
>And then allows to write the map initialization with:
>
>	*data = map;
>	for (i = 0; i < nranges; i++) {
>		for (j = 0; j < ranges[i].numpins; j++, map++) {
>			map->pin = ranges[i].pin + j;
>			map->hwirq = ranges[i].hwirq + j;
>		}
>	}
>	map->pin = -1;
>
>Hmm?
>
>> +int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>> +{
>> +	struct irq_domain *parent_domain, *pdc_domain;
>> +	struct pdc_pin_data *pdc_data = NULL;
>> +	int ret;
>> +
>> +	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;
>> +	}
>> +
>> +	ret = pdc_setup_pin_mapping(node, &pdc_data);
>
>You can let pdc_setup_pin_mapping() return a pointer to pdc_data or NULL
>and check the pointer for ERR or NULL instead of having that ret
>indirection.
>
>> +	if (ret) {
>> +		pr_err("failed to setup PDC pin mapping\n");
>> +		goto failure;
>> +	}
>> +
>> +	pdc_domain = irq_domain_create_hierarchy(parent_domain, 0, PDC_MAX_IRQS,
>> +					of_fwnode_handle(node), &qcom_pdc_ops,
>> +					pdc_data);
>
>See comment about argument alignement above. Hint: shortening the *_domain
>variable names helps.
>
>Thanks,
>
>	tglx

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

[Lina Iyer]

On Fri, Feb 02 2018 at 16:28 +0000, Marc Zyngier wrote:
>On 02/02/18 14:21, 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     | 78 ++++++++++++++++++++++
>>  1 file changed, 78 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..7bf40cb6a4f8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>> @@ -0,0 +1,78 @@
>> +PDC interrupt controller
>> +
>> +Qualcomm Technologies Inc. SoCs based on the RPM Hardened architecture 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. Drivers requiring
>> +wakeup capabilities of their device interrupts routed through the PDC, must
>> +specify PDC as their interrupt controller and request the PDC port associated
>> +with the GIC interrupt. See example below.
>> +
>> +Properties:
>> +
>> +- compatible:
>> +	Usage: required
>> +	Value type: <string>
>> +	Definition: Should contain "qcom,<soc>-pdc"
>> +		    - "qcom,sdm845-pdc": For SDM845
>> +
>> +- 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.
>> +		    The value must match that of the parent interrupt
>> +		    controller defined in the DT.
>> +		    The encoding of these cells are same as described in [1].
>
>There shouldn't be such a requirement. These are two independent pieces
>of HW, and the first parameter doesn't mean anything for the PDC.
>
Wouldn't that be confusing - that we have different definitions for
interrupts in the same DT? I agree that they are different interrupt
controllers, but it just feels odd.

I will change this to just take 2 cells.

>> +
>> +- 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.
>> +
>> +- qcom,pdc-range:
>> +	Usage: required
>> +	Value type: <u32 array>
>> +	Definition: Specifies the PDC pin offset and the number of PDC ports.
>> +		    The tuples indicates the valid mapping of valid PDC ports
>> +		    and their hwirq mapping.
>> +		    The first element of the tuple is the staring PDC port num.
>> +		    The second element is the hwirq number for the PDC port.
>> +		    The third element is the number of elements in sequence.
>> +
>> +Example:
>> +
>> +	pdc: interrupt-controller@b220000 {
>> +		compatible = "qcom,sdm845-pdc";
>> +		reg = <0xb220000 0x30000>;
>> +		qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
>> +		#interrupt-cells = <3>;
>> +		interrupt-parent = <&intc>;
>> +		interrupt-controller;
>> +	};
>> +
>> +The DT binding of a device that wants to use the GIC SPI 514 as a wakeup
>> +interrupt, would look like this -
>> +
>> +	wake-device {
>> +		[...]
>> +		interrupt-parent = <&pdc>;
>> +		interrupt = <0 2 0>;
>
>Again: 0 is not a valid trigger value.
>
Ok.

Thanks,
Lina

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

[Marc Zyngier]

On 02/02/18 16:46, Lina Iyer wrote:
> On Fri, Feb 02 2018 at 16:28 +0000, Marc Zyngier wrote:
>> On 02/02/18 14:21, 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     | 78 ++++++++++++++++++++++
>>>  1 file changed, 78 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..7bf40cb6a4f8
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>>> @@ -0,0 +1,78 @@
>>> +PDC interrupt controller
>>> +
>>> +Qualcomm Technologies Inc. SoCs based on the RPM Hardened architecture 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. Drivers requiring
>>> +wakeup capabilities of their device interrupts routed through the PDC, must
>>> +specify PDC as their interrupt controller and request the PDC port associated
>>> +with the GIC interrupt. See example below.
>>> +
>>> +Properties:
>>> +
>>> +- compatible:
>>> +	Usage: required
>>> +	Value type: <string>
>>> +	Definition: Should contain "qcom,<soc>-pdc"
>>> +		    - "qcom,sdm845-pdc": For SDM845
>>> +
>>> +- 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.
>>> +		    The value must match that of the parent interrupt
>>> +		    controller defined in the DT.
>>> +		    The encoding of these cells are same as described in [1].
>>
>> There shouldn't be such a requirement. These are two independent pieces
>> of HW, and the first parameter doesn't mean anything for the PDC.
>>
> Wouldn't that be confusing - that we have different definitions for
> interrupts in the same DT? I agree that they are different interrupt
> controllers, but it just feels odd.

I think it feels more bizarre to have pointless fields in the interrupt
specifier. And most DTs have some sort of secondary interrupt controller
that only take two (or even one) parameters. I don't think we should
treat this any differently.

> I will change this to just take 2 cells.

Thanks,

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

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

[Lina Iyer]

On Fri, Feb 02 2018 at 16:32 +0000, Steven Rostedt wrote:
>On Fri,  2 Feb 2018 07:22:00 -0700
>Lina Iyer <ilina@codeaurora.org> wrote:
>
>Hi Lina,
>
>This looks really good. I have one nit below.
>
>
>> 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 |  7 ++++++
>>  include/trace/events/pdc.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 62 insertions(+)
>>  create mode 100644 include/trace/events/pdc.h
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index a392380eada6..7f177ad88713 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"
>>
>>  #define PDC_MAX_IRQS		126
>>
>> @@ -68,6 +70,8 @@ static inline void pdc_enable_intr(struct irq_data *d, bool on)
>>  	enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
>>  	pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
>>  	spin_unlock_irqrestore(&pdc_lock, flags);
>> +
>> +	trace_irq_pin_config(PDC_ENTRY, pin_out, (u64)d->chip_data, 0, on);
>>  }
>>
>>  static void qcom_pdc_gic_mask(struct irq_data *d)
>> @@ -149,6 +153,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>>
>>  	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
>>
>> +	trace_irq_pin_config(PDC_TYPE_CONFIG, pin_out, (u64)d->chip_data,
>> +				pdc_type, 0);
>
>I wonder if it makes more sense to just pass 'd' into the trace events,
>and then do the dereference there. The reason is to try to get as much
>code out of the calling path as possible. Even though trace events use
>jump labels and have no conditional branches, the code to call the
>function is still within the code using the trace events. By passing in
>'d' and doing the redirect in the trace event code, we remove the
>setting up of the redirect from the caller, and save some cache lines
>in the process.
Makes sense. Will fix it.

Thanks Steve.

-- Lina

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

[Lina Iyer]

On Fri, Feb 02 2018 at 15:57 +0000, Thomas Gleixner wrote:
>On Fri, 2 Feb 2018, Lina Iyer wrote:
>> +++ b/include/trace/events/pdc.h
>> @@ -0,0 +1,55 @@
>> +/* 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.
>
>Can you please use the proper SPDX identifiers instead of the boiler plate?
>Same for the driver source file.
>
Sure.

>> + */
>> +
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM pdc
>> +
>> +#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _TRACE_PDC_H_
>> +
>> +#include <linux/tracepoint.h>
>> +
>> +#define PDC_ENTRY		1
>> +#define PDC_TYPE_CONFIG		2
>> +
>> +TRACE_EVENT(irq_pin_config,
>
>This is really a too generic name for a PDC specific breakpoint.
>
Hmm.. right.

>Aside of that the question is whether this really justifies a trace
>point. Wouldn't it be sufficient to use the GENERIC_IRQ_DEBUGFS
>infrastructure to make this accessible via debugfs?
>
>
Some product configurations disable debugfs. I will ask around if this
can be dropped.

-- Lina

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

[Lina Iyer]

On Fri, Feb 02 2018 at 23:02 +0000, Lina Iyer wrote:
>On Fri, Feb 02 2018 at 15:57 +0000, Thomas Gleixner wrote:
>>On Fri, 2 Feb 2018, Lina Iyer wrote:
>>>+++ b/include/trace/events/pdc.h
>>>@@ -0,0 +1,55 @@
>>>+/* 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.
>>
>>Can you please use the proper SPDX identifiers instead of the boiler plate?
>>Same for the driver source file.
>>
>Sure.
>
>>>+ */
>>>+
>>>+#undef TRACE_SYSTEM
>>>+#define TRACE_SYSTEM pdc
>>>+
>>>+#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
>>>+#define _TRACE_PDC_H_
>>>+
>>>+#include <linux/tracepoint.h>
>>>+
>>>+#define PDC_ENTRY		1
>>>+#define PDC_TYPE_CONFIG		2
>>>+
>>>+TRACE_EVENT(irq_pin_config,
>>
>>This is really a too generic name for a PDC specific breakpoint.
>>
>Hmm.. right.
>
>>Aside of that the question is whether this really justifies a trace
>>point. Wouldn't it be sufficient to use the GENERIC_IRQ_DEBUGFS
>>infrastructure to make this accessible via debugfs?
>>
>>
>Some product configurations disable debugfs. I will ask around if this
>can be dropped.
>
Memory dumps after a crash have support for FTRACE and it helps greatly
on production issues. Hence the preference for FTRACE support.

-- Lina

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

[Lina Iyer]

Hi Steve,

On Fri, Feb 02 2018 at 14:22 +0000, Lina Iyer wrote:
>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 |  7 ++++++
> include/trace/events/pdc.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 62 insertions(+)
> create mode 100644 include/trace/events/pdc.h
>
>diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>index a392380eada6..7f177ad88713 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"
>
In addition to this PDC patch, there are a few drivers with FTRACE
support coming for up QCOM SoCs. Would it make sense to open up a new
sub-folder for SoC specific FTRACE like say,
trace/events/soc/
trace/events/soc/qcom/

What would be your recommendation?

At the very least, I am thinking of renaming this file to
trace/events/qcom-pdc.h.

Thanks,
Lina

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

[Steven Rostedt]

On Mon, 5 Feb 2018 15:18:46 +0000
Lina Iyer <ilina@codeaurora.org> wrote:

> >Some product configurations disable debugfs. I will ask around if this
> >can be dropped.
> >  
> Memory dumps after a crash have support for FTRACE and it helps greatly
> on production issues. Hence the preference for FTRACE support.

Note, ftrace no longer depends on debugfs. It has its own tracefs file
system:

 # mount -t tracefs nodev /sys/kernel/tracing

-- Steve

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

[Steven Rostedt]

On Mon, 5 Feb 2018 15:50:48 +0000
Lina Iyer <ilina@codeaurora.org> wrote:


> >diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> >index a392380eada6..7f177ad88713 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"
> >  
> In addition to this PDC patch, there are a few drivers with FTRACE
> support coming for up QCOM SoCs. Would it make sense to open up a new
> sub-folder for SoC specific FTRACE like say,
> trace/events/soc/
> trace/events/soc/qcom/
> 
> What would be your recommendation?
> 
> At the very least, I am thinking of renaming this file to
> trace/events/qcom-pdc.h.

I'm thinking it makes more sense to create the trace.h file inside the
drivers/irqchip/ directory. xfs does this, as well as other locations.

The include/trace/events/ was more of a generic method, but when events
start getting more specific, they should be in their own directories.

See samples/trace_events/* on how to have the file locally (or look at
xfs, search for trace in fs/xfs/Makefile).

-- Steve

Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Rasmus Villemoes]

On 2018-02-02 15:58, Marc Zyngier wrote:
> Hi Lina,
> 
> On 02/02/18 14:21, Lina Iyer wrote:
>> From : Archana Sathyakumar <asathyak@codeaurora.org>
>>
>> +
>> +static int qcom_pdc_translate(struct irq_domain *d,
>> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
>> +{
>> +	if (is_of_node(fwspec->fwnode)) {
>> +		if (fwspec->param_count < 3)
>> +			return -EINVAL;
> 
> Why 3? Reading the DT binding, this is indeed set to 3 without any
> reason. I'd suggest this becomes 2, encoding the pin number and the
> trigger information, as the leading 0 is quite useless. Yes, I know
> other examples in the kernel are using this 0, and that was a
> consequence of retrofitting the omitted interrupt controllers (back in
> the days of the stupid gic_arch_extn...). Don't do that.
> 

Hi Marc

I'm about to send out a new revision of the ls-extirq patchset, and
thanks to you pointing me to these patches, I've read the comments on
the various revisions of this series and tried to take those into
account. But the above confused me, because in response to my first RFC
(https://patchwork.kernel.org/patch/10102643/) we have

On 2017-12-08 17:09, Marc Zyngier wrote:
> On 08/12/17 15:11, Alexander Stein wrote:
>> Hi Rasmus,
>>
>>> +
>>> +Required properties:
>>> +- compatible: should be "fsl,ls1021a-extirq"
>>> +- interrupt-controller: Identifies the node as an interrupt controller
>>> +- #interrupt-cells: Use the same format as specified by GIC in
arm,gic.txt.
>>
>> Do you really need 3 interrupt-cells here? As you've written below
you don't
>> support PPI anyway the 1st flag might be dropped then. So support
just 2 cells:
>> * IRQ number (IRQ0 - IRQ5)
>> * IRQ flags
>
> The convention for irqchip stacked on top of a GIC is to keep the
> interrupt specifier the same. It makes the maintenance if the DT much
> easier, and doesn't hurt at all.

Personally, I'd actually prefer the simpler interrupt specifiers without
a redundant 0. Maybe I'm just missing some difference between this case
and the ls-extirq one?

Thanks,
Rasmus

Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Marc Zyngier]

Hi Rasmus,

On 23/02/18 12:16, Rasmus Villemoes wrote:
> On 2018-02-02 15:58, Marc Zyngier wrote:
>> Hi Lina,
>>
>> On 02/02/18 14:21, Lina Iyer wrote:
>>> From : Archana Sathyakumar <asathyak@codeaurora.org>
>>>
>>> +
>>> +static int qcom_pdc_translate(struct irq_domain *d,
>>> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
>>> +{
>>> +	if (is_of_node(fwspec->fwnode)) {
>>> +		if (fwspec->param_count < 3)
>>> +			return -EINVAL;
>>
>> Why 3? Reading the DT binding, this is indeed set to 3 without any
>> reason. I'd suggest this becomes 2, encoding the pin number and the
>> trigger information, as the leading 0 is quite useless. Yes, I know
>> other examples in the kernel are using this 0, and that was a
>> consequence of retrofitting the omitted interrupt controllers (back in
>> the days of the stupid gic_arch_extn...). Don't do that.
>>
> 
> Hi Marc
> 
> I'm about to send out a new revision of the ls-extirq patchset, and
> thanks to you pointing me to these patches, I've read the comments on
> the various revisions of this series and tried to take those into
> account. But the above confused me, because in response to my first RFC
> (https://patchwork.kernel.org/patch/10102643/) we have
> 
> On 2017-12-08 17:09, Marc Zyngier wrote:
>> On 08/12/17 15:11, Alexander Stein wrote:
>>> Hi Rasmus,
>>>
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be "fsl,ls1021a-extirq"
>>>> +- interrupt-controller: Identifies the node as an interrupt controller
>>>> +- #interrupt-cells: Use the same format as specified by GIC in
> arm,gic.txt.
>>>
>>> Do you really need 3 interrupt-cells here? As you've written below
> you don't
>>> support PPI anyway the 1st flag might be dropped then. So support
> just 2 cells:
>>> * IRQ number (IRQ0 - IRQ5)
>>> * IRQ flags
>>
>> The convention for irqchip stacked on top of a GIC is to keep the
>> interrupt specifier the same. It makes the maintenance if the DT much
>> easier, and doesn't hurt at all.
> 
> Personally, I'd actually prefer the simpler interrupt specifiers without
> a redundant 0. Maybe I'm just missing some difference between this case
> and the ls-extirq one?
The difference is that you're adding a new irqchip to an existing DT,
and you get some possible breakage. Maybe you'd be happy with the
breakage, that's your call (and the maintainer's). In the QC case, it is
old brand new, so no harm in doing the right thing from day one.

It is in the end an implementation decision, and you could go either way.

Hope this helps,

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

Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Rasmus Villemoes]

On 2018-02-23 14:37, Marc Zyngier wrote:
> Hi Rasmus,
> 
> On 23/02/18 12:16, Rasmus Villemoes wrote:
>> On 2018-02-02 15:58, Marc Zyngier wrote:

>>> Why 3? Reading the DT binding, this is indeed set to 3 without any
>>> reason. I'd suggest this becomes 2, encoding the pin number and the
>>> trigger information, as the leading 0 is quite useless. Yes, I know
>>> other examples in the kernel are using this 0, and that was a
>>> consequence of retrofitting the omitted interrupt controllers (back in
>>> the days of the stupid gic_arch_extn...). Don't do that.
>>>
>>
>> Hi Marc
>>
>> I'm about to send out a new revision of the ls-extirq patchset, and
>> thanks to you pointing me to these patches, I've read the comments on
>> the various revisions of this series and tried to take those into
>> account. But the above confused me, because in response to my first RFC
>> (https://patchwork.kernel.org/patch/10102643/) we have
>>
>> On 2017-12-08 17:09, Marc Zyngier wrote:
>>> On 08/12/17 15:11, Alexander Stein wrote:
>>>> Hi Rasmus,
>>>>
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: should be "fsl,ls1021a-extirq"
>>>>> +- interrupt-controller: Identifies the node as an interrupt controller
>>>>> +- #interrupt-cells: Use the same format as specified by GIC in
>> arm,gic.txt.
>>>>
>>>> Do you really need 3 interrupt-cells here? As you've written below
>> you don't
>>>> support PPI anyway the 1st flag might be dropped then. So support
>> just 2 cells:
>>>> * IRQ number (IRQ0 - IRQ5)
>>>> * IRQ flags
>>>
>>> The convention for irqchip stacked on top of a GIC is to keep the
>>> interrupt specifier the same. It makes the maintenance if the DT much
>>> easier, and doesn't hurt at all.
>>
>> Personally, I'd actually prefer the simpler interrupt specifiers without
>> a redundant 0. Maybe I'm just missing some difference between this case
>> and the ls-extirq one?
> The difference is that you're adding a new irqchip to an existing DT,
> and you get some possible breakage. Maybe you'd be happy with the
> breakage, that's your call (and the maintainer's).

OK. In the ls1021a case, I actually think "breaking" any existing users
if and when they move to the new driver/irqchip is a good thing: the
power-on-reset value is such that the lines have the polarity inverted.
So there could be some board with a device with either a EDGE_FALLING or
LEVEL_LOW interrupt connected to one of the external interrupt lines,
which is described in DT by "lying" and using the opposite flag.
Changing #interrupt-cells prevents such (ab)users from just changing
interrupt-parent and calling it a day.

 In the QC case, it is
> old brand new, so no harm in doing the right thing from day one>
> It is in the end an implementation decision, and you could go either way.

Doing the right thing sounds nice, so I'll go with that :)

Thanks,
Rasmus