[PATCH V9 0/3] irqchip: qcom: Add IRQ combiner driver

Add support for IRQ combiners in the Top-level Control and Status
Registers (TCSR) hardware block in Qualcomm Technologies chips.

The first patch prevents the ACPI core from attempting to map IRQ resources
with a valid ResourceSource as GSIs.

The first patch adds support for ResourceSource/IRQ domain mapping and
fixes IRQ probe deferral by allowing platform_device IRQ resources to be
re-initialized from the corresponding ACPI IRQ resource.

Both changes described above are conditional on the ACPI_GENERIC_GSI config.

The third patch takes advantage of the new capabilities to implement
the driver for the IRQ combiners.

Tested on top of v4.9.

Changes V8 -> V9:
* Do not attempt the mapping for non-GSI IRQs during bus scan.
* Make some public APIs private to drivers/acpi/irq.c since they are no
  longer used on other modules.

Changes V7 -> V8:
* Reorder patches to allow all new code to be under drivers/acpi/irq.c.
* Change acpi_irq_get implementation to be more similar to of_irq_get
  to improve maintainability.

Agustin Vega-Frias (3):
  ACPI: Generic GSI: Do not attempt to map non-GSI IRQs during bus scan
  ACPI: Add support for ResourceSource/IRQ domain mapping
  irqchip: qcom: Add IRQ combiner driver

 drivers/acpi/Makefile               |   2 +-
 drivers/acpi/gsi.c                  |  98 -----------
 drivers/acpi/irq.c                  | 280 +++++++++++++++++++++++++++++++
 drivers/acpi/resource.c             |  17 +-
 drivers/base/platform.c             |   9 +-
 drivers/irqchip/Kconfig             |   9 +
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/qcom-irq-combiner.c | 322 ++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h                |  10 ++
 9 files changed, 647 insertions(+), 101 deletions(-)
 delete mode 100644 drivers/acpi/gsi.c
 create mode 100644 drivers/acpi/irq.c
 create mode 100644 drivers/irqchip/qcom-irq-combiner.c

--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

ACPI extended IRQ resources may contain a Resource Source field to specify
an alternate interrupt controller, attempting to map them as GSIs is
incorrect, so just disable the platform resource.

Since this field is currently ignored, we make this change conditional
on CONFIG_ACPI_GENERIC_GSI to keep the current behavior on x86 platforms,
in case some existing ACPI tables are using this incorrectly.

Signed-off-by: Agustin Vega-Frias <redacted>
---
 drivers/acpi/resource.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 56241eb..76ca4e9 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c

@@ -43,6 +43,18 @@ static inline bool acpi_iospace_resource_valid(struct resource *res)
 acpi_iospace_resource_valid(struct resource *res) { return true; }
 #endif
 
+#ifdef CONFIG_ACPI_GENERIC_GSI
+static inline bool is_gsi(struct acpi_resource_extended_irq *ext_irq)
+{
+	return ext_irq->resource_source.string_length == 0;
+}
+#else
+static inline bool is_gsi(struct acpi_resource_extended_irq *ext_irq)
+{
+	return true;
+}
+#endif
+
 static bool acpi_dev_resource_len_valid(u64 start, u64 end, u64 len, bool io)
 {
 	u64 reslen = end - start + 1;

@@ -470,9 +482,12 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 			acpi_dev_irqresource_disabled(res, 0);
 			return false;
 		}
-		acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
+		if (is_gsi(ext_irq))
+			acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
 					 ext_irq->triggering, ext_irq->polarity,
 					 ext_irq->sharable, false);
+		else
+			acpi_dev_irqresource_disabled(res, 0);
 		break;
 	default:
 		res->flags = 0;

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

ACPI extended IRQ resources may contain a ResourceSource to specify
an alternate interrupt controller. Introduce acpi_irq_get and use it
to implement ResourceSource/IRQ domain mapping.

The new API is similar to of_irq_get and allows re-initialization
of a platform resource from the ACPI extended IRQ resource, and
provides proper behavior for probe deferral when the domain is not
yet present when called.

Signed-off-by: Agustin Vega-Frias <redacted>
---
 drivers/acpi/Makefile         |   2 +-
 drivers/acpi/{gsi.c => irq.c} | 182 ++++++++++++++++++++++++++++++++++++++++++
 drivers/base/platform.c       |   9 ++-
 include/linux/acpi.h          |  10 +++
 4 files changed, 201 insertions(+), 2 deletions(-)
 rename drivers/acpi/{gsi.c => irq.c} (32%)

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 9ed0878..a391bbc 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile

@@ -55,7 +55,7 @@ acpi-$(CONFIG_DEBUG_FS)		+= debugfs.o
 acpi-$(CONFIG_ACPI_NUMA)	+= numa.o
 acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
 acpi-y				+= acpi_lpat.o
-acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
+acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
 acpi-$(CONFIG_ACPI_WATCHDOG)	+= acpi_watchdog.o
 
 # These are (potentially) separate modules

diff --git a/drivers/acpi/gsi.c b/drivers/acpi/irq.c
similarity index 32%
rename from drivers/acpi/gsi.c
rename to drivers/acpi/irq.c
index ee9e0f2..133d3f8 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/irq.c

@@ -85,6 +85,188 @@ void acpi_unregister_gsi(u32 gsi)
 EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
 
 /**
+ * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
+ *                                  acpi_resource_source which is used
+ *                                  as an IRQ domain id
+ * @source: acpi_resource_source to use for the lookup
+ *
+ * Returns: The appropriate IRQ fwhandle domain id
+ *          NULL on failure
+ */
+static struct fwnode_handle *
+acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
+{
+	struct fwnode_handle *result;
+	struct acpi_device *device;
+	acpi_handle handle;
+	acpi_status status;
+
+	if (!source->string_length)
+		return acpi_gsi_domain_id;
+
+	status = acpi_get_handle(NULL, source->string_ptr, &handle);
+	if (ACPI_FAILURE(status)) {
+		pr_warn("Could not find handle for %s\n", source->string_ptr);
+		return NULL;
+	}
+
+	device = acpi_bus_get_acpi_device(handle);
+	if (!device) {
+		pr_warn("Could not get device for %s\n", source->string_ptr);
+		return NULL;
+	}
+
+	result = &device->fwnode;
+	acpi_bus_put_acpi_device(device);
+
+	return result;
+}
+
+/**
+ * Context for the resource walk used to lookup IRQ resources.
+ */
+struct acpi_irq_parse_one_ctx {
+	int rc;
+	unsigned int index;
+	unsigned long *res_flags;
+	struct irq_fwspec *fwspec;
+};
+
+/**
+ * acpi_irq_parse_one_match - Handle a matching IRQ resource
+ */
+static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode,
+					    u32 hwirq, u8 triggering,
+					    u8 polarity, u8 shareable,
+					    struct acpi_irq_parse_one_ctx *ctx)
+{
+	ctx->rc = 0;
+	*ctx->res_flags = acpi_dev_irq_flags(triggering, polarity, shareable);
+	ctx->fwspec->fwnode = fwnode;
+	ctx->fwspec->param[0] = hwirq;
+	ctx->fwspec->param[1] = acpi_dev_get_irq_type(triggering, polarity);
+	ctx->fwspec->param_count = 2;
+}
+
+/**
+ * acpi_irq_parse_one_cb - Handle the given resource
+ * @ares: resource to handle
+ * @context: context for the walk, contains the lookup index and references
+ *           to the flags and fwspec where the result is returned
+ *
+ * This is called by acpi_walk_resources passing each resource returned by
+ * the _CRS method. We only inspect IRQ resources. Since IRQ resources
+ * might contain multiple interrupts we check if the index is within this
+ * one's interrupt array, otherwise we subtract the current resource IRQ
+ * count from the lookup index to prepare for the next resource.
+ * Once a match is found we call acpi_irq_parse_one_match to populate
+ * the result and end the walk by returning AE_CTRL_TERMINATE.
+ *
+ * Return AE_OK if the walk should continue, AE_CTRL_TERMINATE if a matching
+ * IRQ resource was found.
+ */
+static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares,
+					 void *context)
+{
+	struct acpi_irq_parse_one_ctx *ctx = context;
+	struct acpi_resource_irq *irq;
+	struct acpi_resource_extended_irq *eirq;
+	struct fwnode_handle *fwnode;
+
+	switch (ares->type) {
+	case ACPI_RESOURCE_TYPE_IRQ:
+		irq = &ares->data.irq;
+		if (ctx->index >= irq->interrupt_count) {
+			ctx->index -= irq->interrupt_count;
+			return AE_OK;
+		}
+		fwnode = acpi_gsi_domain_id;
+		acpi_irq_parse_one_match(fwnode, irq->interrupts[ctx->index],
+					 irq->triggering, irq->polarity,
+					 irq->sharable, ctx);
+		return AE_CTRL_TERMINATE;
+	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+		eirq = &ares->data.extended_irq;
+		if (ctx->index >= eirq->interrupt_count) {
+			ctx->index -= eirq->interrupt_count;
+			return AE_OK;
+		}
+		fwnode = acpi_get_irq_source_fwhandle(&eirq->resource_source);
+		acpi_irq_parse_one_match(fwnode, eirq->interrupts[ctx->index],
+					 eirq->triggering, eirq->polarity,
+					 eirq->sharable, ctx);
+		return AE_CTRL_TERMINATE;
+	}
+
+	return AE_OK;
+}
+
+/**
+ * acpi_irq_parse_one - Resolve an interrupt for a device
+ * @handle: the device whose interrupt is to be resolved
+ * @index: index of the interrupt to resolve
+ * @fwspec: structure irq_fwspec filled by this function
+ * @flags: resource flags filled by this function
+ *
+ * This function resolves an interrupt for a device by walking its CRS resources
+ * to find the appropriate ACPI IRQ resource and populating the given structure
+ * which can be used to retrieve a Linux IRQ number.
+ *
+ * Returns the result stored in ctx.rc by the callback, or -EINVAL if the given
+ * index is out of range.
+ */
+static int acpi_irq_parse_one(acpi_handle handle, unsigned int index,
+			      struct irq_fwspec *fwspec, unsigned long *flags)
+{
+	struct acpi_irq_parse_one_ctx ctx = { -EINVAL, index, flags, fwspec };
+	acpi_status status;
+
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+				     acpi_irq_parse_one_cb, &ctx);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+	return ctx.rc;
+}
+
+/**
+ * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
+ *                use it to initialize the given Linux IRQ resource.
+ * @handle ACPI device handle
+ * @index  ACPI IRQ resource index to lookup
+ * @res    Linux IRQ resource to initialize
+ *
+ * Return: 0 on success
+ *         -EINVAL if an error occurs
+ *         -EPROBE_DEFER if the IRQ lookup/conversion failed
+ */
+int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
+{
+	int rc;
+	struct irq_fwspec fwspec;
+	struct irq_domain *domain;
+	unsigned long flags;
+
+	rc = acpi_irq_parse_one(handle, index, &fwspec, &flags);
+	if (rc)
+		return rc;
+
+	domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
+	if (!domain)
+		return -EPROBE_DEFER;
+
+	rc = irq_create_fwspec_mapping(&fwspec);
+	if (rc <= 0)
+		return -EINVAL;
+
+	res->start = rc;
+	res->end = rc;
+	res->flags = flags;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_irq_get);
+
+/**
  * acpi_set_irq_model - Setup the GSI irqdomain information
  * @model: the value assigned to acpi_irq_model
  * @fwnode: the irq_domain identifier for mapping and looking up

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index c4af003..61423d2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c

@@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 	}
 
 	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
+	if (r && r->flags & IORESOURCE_DISABLED && ACPI_COMPANION(&dev->dev)) {
+		int ret;
+
+		ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * The resources may pass trigger flags to the irqs that need
 	 * to be set up. It so happens that the trigger flags for

@@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
 		memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head));
 	}
 }
-

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 61a3d90..2284fc6 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h

@@ -1123,4 +1123,14 @@ static inline void acpi_table_upgrade(void) { }
 static inline int parse_spcr(bool earlycon) { return 0; }
 #endif
 
+#ifdef CONFIG_ACPI_GENERIC_GSI
+int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res);
+#else
+static inline int acpi_irq_get(acpi_handle handle, unsigned int index,
+			       struct resource *res)
+{
+	return -EINVAL;
+}
+#endif
+
 #endif	/*_LINUX_ACPI_H*/

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Driver for interrupt combiners in the Top-level Control and Status
Registers (TCSR) hardware block in Qualcomm Technologies chips.

An interrupt combiner in this block combines a set of interrupts by
OR'ing the individual interrupt signals into a summary interrupt
signal routed to a parent interrupt controller, and provides read-
only, 32-bit registers to query the status of individual interrupts.
The status bit for IRQ n is bit (n % 32) within register (n / 32)
of the given combiner. Thus, each combiner can be described as a set
of register offsets and the number of IRQs managed.

Signed-off-by: Agustin Vega-Frias <redacted>
---
 drivers/irqchip/Kconfig             |   9 +
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/qcom-irq-combiner.c | 322 ++++++++++++++++++++++++++++++++++++
 3 files changed, 332 insertions(+)
 create mode 100644 drivers/irqchip/qcom-irq-combiner.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bc0af33..3e3430c 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig

@@ -279,3 +279,12 @@ config EZNPS_GIC
 config STM32_EXTI
 	bool
 	select IRQ_DOMAIN
+
+config QCOM_IRQ_COMBINER
+	bool "QCOM IRQ combiner support"
+	depends on ARCH_QCOM && ACPI
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Say yes here to add support for the IRQ combiner devices embedded
+	  in Qualcomm Technologies chips.

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e4dbfc8..1818a0b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile

@@ -74,3 +74,4 @@ obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
+obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o

diff --git a/drivers/irqchip/qcom-irq-combiner.c b/drivers/irqchip/qcom-irq-combiner.c
new file mode 100644
index 0000000..0055e08
--- /dev/null
+++ b/drivers/irqchip/qcom-irq-combiner.c

@@ -0,0 +1,322 @@
+/* Copyright (c) 2015-2016, 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.
+ */
+
+/*
+ * Driver for interrupt combiners in the Top-level Control and Status
+ * Registers (TCSR) hardware block in Qualcomm Technologies chips.
+ * An interrupt combiner in this block combines a set of interrupts by
+ * OR'ing the individual interrupt signals into a summary interrupt
+ * signal routed to a parent interrupt controller, and provides read-
+ * only, 32-bit registers to query the status of individual interrupts.
+ * The status bit for IRQ n is bit (n % 32) within register (n / 32)
+ * of the given combiner. Thus, each combiner can be described as a set
+ * of register offsets and the number of IRQs managed.
+ */
+
+#include <linux/acpi.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/platform_device.h>
+
+#define REG_SIZE 32
+
+struct combiner_reg {
+	void __iomem *addr;
+	unsigned long mask;
+};
+
+struct combiner {
+	struct irq_domain   *domain;
+	int                 parent_irq;
+	u32                 nirqs;
+	u32                 nregs;
+	struct combiner_reg regs[0];
+};
+
+static inline u32 irq_register(int irq)
+{
+	return irq / REG_SIZE;
+}
+
+static inline u32 irq_bit(int irq)
+{
+	return irq % REG_SIZE;
+
+}
+
+static inline int irq_nr(u32 reg, u32 bit)
+{
+	return reg * REG_SIZE + bit;
+}
+
+/*
+ * Handler for the cascaded IRQ.
+ */
+static void combiner_handle_irq(struct irq_desc *desc)
+{
+	struct combiner *combiner = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 reg;
+
+	chained_irq_enter(chip, desc);
+
+	for (reg = 0; reg < combiner->nregs; reg++) {
+		int virq;
+		int hwirq;
+		u32 bit;
+		u32 status;
+
+		if (combiner->regs[reg].mask == 0)
+			continue;
+
+		status = readl_relaxed(combiner->regs[reg].addr);
+		status &= combiner->regs[reg].mask;
+
+		while (status) {
+			bit = __ffs(status);
+			status &= ~(1 << bit);
+			hwirq = irq_nr(reg, bit);
+			virq = irq_find_mapping(combiner->domain, hwirq);
+			if (virq >= 0)
+				generic_handle_irq(virq);
+
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+/*
+ * irqchip callbacks
+ */
+
+static void combiner_irq_chip_mask_irq(struct irq_data *data)
+{
+	struct combiner *combiner = irq_data_get_irq_chip_data(data);
+	struct combiner_reg *reg = combiner->regs + irq_register(data->hwirq);
+
+	clear_bit(irq_bit(data->hwirq), &reg->mask);
+}
+
+static void combiner_irq_chip_unmask_irq(struct irq_data *data)
+{
+	struct combiner *combiner = irq_data_get_irq_chip_data(data);
+	struct combiner_reg *reg = combiner->regs + irq_register(data->hwirq);
+
+	set_bit(irq_bit(data->hwirq), &reg->mask);
+}
+
+static struct irq_chip irq_chip = {
+	.irq_mask = combiner_irq_chip_mask_irq,
+	.irq_unmask = combiner_irq_chip_unmask_irq,
+	.name = "qcom-irq-combiner"
+};
+
+/*
+ * irq_domain_ops callbacks
+ */
+
+static int combiner_irq_map(struct irq_domain *domain, unsigned int irq,
+				   irq_hw_number_t hwirq)
+{
+	struct combiner *combiner = domain->host_data;
+
+	if (hwirq >= combiner->nirqs)
+		return -EINVAL;
+
+	irq_set_chip_and_handler(irq, &irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, combiner);
+	irq_set_noprobe(irq);
+	return 0;
+}
+
+static void combiner_irq_unmap(struct irq_domain *domain, unsigned int irq)
+{
+	struct irq_data *data = irq_get_irq_data(irq);
+
+	if (WARN_ON(!data))
+		return;
+	irq_domain_reset_irq_data(data);
+}
+
+static int combiner_irq_translate(struct irq_domain *d, struct irq_fwspec *fws,
+				  unsigned long *hwirq, unsigned int *type)
+{
+	if (is_acpi_node(fws->fwnode)) {
+		if (WARN_ON((fws->param_count != 2) ||
+			    (fws->param[1] & IORESOURCE_IRQ_LOWEDGE) ||
+			    (fws->param[1] & IORESOURCE_IRQ_HIGHEDGE)))
+			return -EINVAL;
+
+		*hwirq = fws->param[0];
+		*type = fws->param[1];
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static const struct irq_domain_ops domain_ops = {
+	.map = combiner_irq_map,
+	.unmap = combiner_irq_unmap,
+	.translate = combiner_irq_translate
+};
+
+/*
+ * Device probing
+ */
+
+static acpi_status count_registers_cb(struct acpi_resource *ares, void *context)
+{
+	int *count = context;
+
+	if (ares->type == ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
+		++(*count);
+	return AE_OK;
+}
+
+static int count_registers(struct platform_device *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status status;
+	int count = 0;
+
+	if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
+		return -EINVAL;
+
+	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
+				     count_registers_cb, &count);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+	return count;
+}
+
+struct get_registers_context {
+	struct device *dev;
+	struct combiner *combiner;
+	int err;
+};
+
+static acpi_status get_registers_cb(struct acpi_resource *ares, void *context)
+{
+	struct get_registers_context *ctx = context;
+	struct acpi_resource_generic_register *reg;
+	phys_addr_t paddr;
+	void __iomem *vaddr;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
+		return AE_OK;
+
+	reg = &ares->data.generic_reg;
+	paddr = reg->address;
+	if ((reg->space_id != ACPI_SPACE_MEM) ||
+	    (reg->bit_offset != 0) ||
+	    (reg->bit_width > REG_SIZE)) {
+		dev_err(ctx->dev, "Bad register resource @%pa\n", &paddr);
+		ctx->err = -EINVAL;
+		return AE_ERROR;
+	}
+
+	vaddr = devm_ioremap(ctx->dev, reg->address, REG_SIZE);
+	if (IS_ERR(vaddr)) {
+		dev_err(ctx->dev, "Can't map register @%pa\n", &paddr);
+		ctx->err = PTR_ERR(vaddr);
+		return AE_ERROR;
+	}
+
+	ctx->combiner->regs[ctx->combiner->nregs].addr = vaddr;
+	ctx->combiner->nirqs += reg->bit_width;
+	ctx->combiner->nregs++;
+	return AE_OK;
+}
+
+static int get_registers(struct platform_device *pdev, struct combiner *comb)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status status;
+	struct get_registers_context ctx;
+
+	if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
+		return -EINVAL;
+
+	ctx.dev = &pdev->dev;
+	ctx.combiner = comb;
+	ctx.err = 0;
+
+	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
+				     get_registers_cb, &ctx);
+	if (ACPI_FAILURE(status))
+		return ctx.err;
+	return 0;
+}
+
+static int __init combiner_probe(struct platform_device *pdev)
+{
+	struct combiner *combiner;
+	size_t alloc_sz;
+	u32 nregs;
+	int err;
+
+	nregs = count_registers(pdev);
+	if (nregs <= 0) {
+		dev_err(&pdev->dev, "Error reading register resources\n");
+		return -EINVAL;
+	}
+
+	alloc_sz = sizeof(*combiner) + sizeof(struct combiner_reg) * nregs;
+	combiner = devm_kzalloc(&pdev->dev, alloc_sz, GFP_KERNEL);
+	if (!combiner)
+		return -ENOMEM;
+
+	err = get_registers(pdev, combiner);
+	if (err < 0)
+		return err;
+
+	combiner->parent_irq = platform_get_irq(pdev, 0);
+	if (combiner->parent_irq <= 0) {
+		dev_err(&pdev->dev, "Error getting IRQ resource\n");
+		return -EPROBE_DEFER;
+	}
+
+	combiner->domain = irq_domain_create_linear(pdev->dev.fwnode, combiner->nirqs,
+						    &domain_ops, combiner);
+	if (!combiner->domain)
+		/* Errors printed by irq_domain_create_linear */
+		return -ENODEV;
+
+	irq_set_chained_handler_and_data(combiner->parent_irq,
+					 combiner_handle_irq, combiner);
+
+	dev_info(&pdev->dev, "Initialized with [p=%d,n=%d,r=%p]\n",
+		 combiner->parent_irq, combiner->nirqs, combiner->regs[0].addr);
+	return 0;
+}
+
+static const struct acpi_device_id qcom_irq_combiner_acpi_match[] = {
+	{ "QCOM80B1", },
+	{ }
+};
+
+static struct platform_driver qcom_irq_combiner_probe = {
+	.driver = {
+		.name = "qcom-irq-combiner",
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(qcom_irq_combiner_acpi_match),
+	},
+	.probe = combiner_probe,
+};
+
+static int __init register_qcom_irq_combiner(void)
+{
+	return platform_driver_register(&qcom_irq_combiner_probe);
+}
+device_initcall(register_qcom_irq_combiner);

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

On Wed, Dec 14, 2016 at 05:10:36PM -0500, Agustin Vega-Frias wrote:

Well, patch is simple there is not much to say other that on the
firmware side I honestly do not see many options, either we remove the
ifdeffery above and make the check unconditional (ie we do check on
x86/ia64 too instead of always returning true) and see if things hold up
on x86 (at least we try) or we will never know and will never be able to
use this on x86 if there will ever be need.

It would be certainly weird to find out that a descriptor has a
resource_source pointer put there by mistake (because that's what we are
talking about, things work on x86/ia64 by ignoring the resource_source
pointer so any string there is just an unfortunate mistake AFAICS).

I am quite tempted to remove the ifdef and make the is_gsi() check effective on
x86/ia64 too.

Lorenzo

Hi Lorenzo,

On 2016-12-16 11:24, Lorenzo Pieralisi wrote:

On Wed, Dec 14, 2016 at 05:10:36PM -0500, Agustin Vega-Frias wrote:

quoted

ACPI extended IRQ resources may contain a Resource Source field to 
specify
an alternate interrupt controller, attempting to map them as GSIs is
incorrect, so just disable the platform resource.

Since this field is currently ignored, we make this change conditional
on CONFIG_ACPI_GENERIC_GSI to keep the current behavior on x86 
platforms,
in case some existing ACPI tables are using this incorrectly.

Signed-off-by: Agustin Vega-Frias <redacted>
---
 drivers/acpi/resource.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 56241eb..76ca4e9 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c

@@ -43,6 +43,18 @@ static inline bool 

acpi_iospace_resource_valid(struct resource *res)
 acpi_iospace_resource_valid(struct resource *res) { return true; }
 #endif

+#ifdef CONFIG_ACPI_GENERIC_GSI
+static inline bool is_gsi(struct acpi_resource_extended_irq *ext_irq)
+{
+	return ext_irq->resource_source.string_length == 0;
+}
+#else
+static inline bool is_gsi(struct acpi_resource_extended_irq *ext_irq)
+{
+	return true;
+}
+#endif

Well, patch is simple there is not much to say other that on the
firmware side I honestly do not see many options, either we remove the
ifdeffery above and make the check unconditional (ie we do check on
x86/ia64 too instead of always returning true) and see if things hold 
up
on x86 (at least we try) or we will never know and will never be able 
to
use this on x86 if there will ever be need.

It would be certainly weird to find out that a descriptor has a
resource_source pointer put there by mistake (because that's what we 
are
talking about, things work on x86/ia64 by ignoring the resource_source
pointer so any string there is just an unfortunate mistake AFAICS).

I am quite tempted to remove the ifdef and make the is_gsi() check 
effective on
x86/ia64 too.

I wouldn't be opposed to that. I added the #ifdef out of abundance of 
caution,
but if the x86 folks agree and we can get sufficient testing to ensure 
this
doesn't break anything I'd like to remove it too.

Thanks,
Agustin

Lorenzo

quoted

+
 static bool acpi_dev_resource_len_valid(u64 start, u64 end, u64 len, 
bool io)
 {
 	u64 reslen = end - start + 1;

@@ -470,9 +482,12 @@ bool acpi_dev_resource_interrupt(struct 

acpi_resource *ares, int index,
 			acpi_dev_irqresource_disabled(res, 0);
 			return false;
 		}
-		acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
+		if (is_gsi(ext_irq))
+			acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
 					 ext_irq->triggering, ext_irq->polarity,
 					 ext_irq->sharable, false);
+		else
+			acpi_dev_irqresource_disabled(res, 0);
 		break;
 	default:
 		res->flags = 0;
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Hi,

Is there any more feedback on this beyond Lorenzo's suggestion to drop
the conditional check on the first patch?
How can we move forward on this series?

Thanks.

On 2016-12-14 17:10, Agustin Vega-Frias wrote:

Add support for IRQ combiners in the Top-level Control and Status
Registers (TCSR) hardware block in Qualcomm Technologies chips.

The first patch prevents the ACPI core from attempting to map IRQ 
resources
with a valid ResourceSource as GSIs.

The second patch adds support for ResourceSource/IRQ domain mapping and
fixes IRQ probe deferral by allowing platform_device IRQ resources to 
be
re-initialized from the corresponding ACPI IRQ resource.

Both changes described above are conditional on the ACPI_GENERIC_GSI 
config.

The third patch takes advantage of the new capabilities to implement
the driver for the IRQ combiners.

Tested on top of v4.9.

Changes V8 -> V9:
* Do not attempt the mapping for non-GSI IRQs during bus scan.
* Make some public APIs private to drivers/acpi/irq.c since they are no
  longer used on other modules.

Changes V7 -> V8:
* Reorder patches to allow all new code to be under drivers/acpi/irq.c.
* Change acpi_irq_get implementation to be more similar to of_irq_get
  to improve maintainability.

Agustin Vega-Frias (3):
  ACPI: Generic GSI: Do not attempt to map non-GSI IRQs during bus scan
  ACPI: Add support for ResourceSource/IRQ domain mapping
  irqchip: qcom: Add IRQ combiner driver

 drivers/acpi/Makefile               |   2 +-
 drivers/acpi/gsi.c                  |  98 -----------
 drivers/acpi/irq.c                  | 280 
+++++++++++++++++++++++++++++++
 drivers/acpi/resource.c             |  17 +-
 drivers/base/platform.c             |   9 +-
 drivers/irqchip/Kconfig             |   9 +
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/qcom-irq-combiner.c | 322 
++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h                |  10 ++
 9 files changed, 647 insertions(+), 101 deletions(-)
 delete mode 100644 drivers/acpi/gsi.c
 create mode 100644 drivers/acpi/irq.c
 create mode 100644 drivers/irqchip/qcom-irq-combiner.c

--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

On Tue, Jan 3, 2017 at 4:19 PM, Agustin Vega-Frias
[off-list ref] wrote:

Hi,

Is there any more feedback on this beyond Lorenzo's suggestion to drop
the conditional check on the first patch?
How can we move forward on this series?

Essentially, I need to convince myself that patches [1-2/3] are fine
which hasn't happened yet.

Thanks,
Rafael

On 2017-01-03 16:56, Rafael J. Wysocki wrote:

On Tue, Jan 3, 2017 at 4:19 PM, Agustin Vega-Frias
[off-list ref] wrote:

quoted

Hi,

Is there any more feedback on this beyond Lorenzo's suggestion to drop
the conditional check on the first patch?
How can we move forward on this series?

Essentially, I need to convince myself that patches [1-2/3] are fine
which hasn't happened yet.

Thanks Rafael, I'll hold on for your feedback.

Thanks,
Rafael

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Hi Agustin,

On 14/12/16 22:10, Agustin Vega-Frias wrote:

I'm a bit worried by this. If I understand it well, this is a pure
software construct (controlled from combiner_irq_chip_{un,}mask_irq) and
there is nothing that actually masks the interrupt at the HW level.

So if a device asserts its interrupt line, what mechanism do we have to
make sure that we don't end-up with the CPU pegged in interrupt context?

Small bug: virq == 0 shouldn't happen, since this would be an indication
that the hwirq doesn't have a mapping.

Given that this should have come from the translate function, can we
move the check there instead, so that we validate everything early?

Can this happen?

Other than the questions I have above, this now looks in a much better
shape. Hopefully Rafael will soon come back will his conclusions on the
first two patches, as I'd like to see this code to get some -next for a
while.

Thanks,

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

Hey Marc,

On 2017-01-05 11:48, Marc Zyngier wrote:

Hi Agustin,

On 14/12/16 22:10, Agustin Vega-Frias wrote:

quoted

Driver for interrupt combiners in the Top-level Control and Status
Registers (TCSR) hardware block in Qualcomm Technologies chips.

An interrupt combiner in this block combines a set of interrupts by
OR'ing the individual interrupt signals into a summary interrupt
signal routed to a parent interrupt controller, and provides read-
only, 32-bit registers to query the status of individual interrupts.
The status bit for IRQ n is bit (n % 32) within register (n / 32)
of the given combiner. Thus, each combiner can be described as a set
of register offsets and the number of IRQs managed.

Signed-off-by: Agustin Vega-Frias <redacted>
---
 drivers/irqchip/Kconfig             |   9 +
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/qcom-irq-combiner.c | 322 
++++++++++++++++++++++++++++++++++++
 3 files changed, 332 insertions(+)
 create mode 100644 drivers/irqchip/qcom-irq-combiner.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bc0af33..3e3430c 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig

@@ -279,3 +279,12 @@ config EZNPS_GIC
 config STM32_EXTI
 	bool
 	select IRQ_DOMAIN
+
+config QCOM_IRQ_COMBINER
+	bool "QCOM IRQ combiner support"
+	depends on ARCH_QCOM && ACPI
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Say yes here to add support for the IRQ combiner devices embedded
+	  in Qualcomm Technologies chips.

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e4dbfc8..1818a0b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile

@@ -74,3 +74,4 @@ obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
+obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o

diff --git a/drivers/irqchip/qcom-irq-combiner.c 

b/drivers/irqchip/qcom-irq-combiner.c
new file mode 100644
index 0000000..0055e08

--- /dev/null
+++ b/drivers/irqchip/qcom-irq-combiner.c

@@ -0,0 +1,322 @@
+/* Copyright (c) 2015-2016, 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.
+ */
+
+/*
+ * Driver for interrupt combiners in the Top-level Control and Status
+ * Registers (TCSR) hardware block in Qualcomm Technologies chips.
+ * An interrupt combiner in this block combines a set of interrupts 
by
+ * OR'ing the individual interrupt signals into a summary interrupt
+ * signal routed to a parent interrupt controller, and provides read-
+ * only, 32-bit registers to query the status of individual 
interrupts.
+ * The status bit for IRQ n is bit (n % 32) within register (n / 32)
+ * of the given combiner. Thus, each combiner can be described as a 
set
+ * of register offsets and the number of IRQs managed.
+ */
+
+#include <linux/acpi.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/platform_device.h>
+
+#define REG_SIZE 32
+
+struct combiner_reg {
+	void __iomem *addr;
+	unsigned long mask;
+};
+
+struct combiner {
+	struct irq_domain   *domain;
+	int                 parent_irq;
+	u32                 nirqs;
+	u32                 nregs;
+	struct combiner_reg regs[0];
+};
+
+static inline u32 irq_register(int irq)
+{
+	return irq / REG_SIZE;
+}
+
+static inline u32 irq_bit(int irq)
+{
+	return irq % REG_SIZE;
+
+}
+
+static inline int irq_nr(u32 reg, u32 bit)
+{
+	return reg * REG_SIZE + bit;
+}
+
+/*
+ * Handler for the cascaded IRQ.
+ */
+static void combiner_handle_irq(struct irq_desc *desc)
+{
+	struct combiner *combiner = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 reg;
+
+	chained_irq_enter(chip, desc);
+
+	for (reg = 0; reg < combiner->nregs; reg++) {
+		int virq;
+		int hwirq;
+		u32 bit;
+		u32 status;
+
+		if (combiner->regs[reg].mask == 0)
+			continue;

I'm a bit worried by this. If I understand it well, this is a pure
software construct (controlled from combiner_irq_chip_{un,}mask_irq) 
and
there is nothing that actually masks the interrupt at the HW level.

So if a device asserts its interrupt line, what mechanism do we have to
make sure that we don't end-up with the CPU pegged in interrupt 
context?

Yes, unfortunately this is a dumb hardware combiner; however, the real 
use
of mask here is to optimize IRQ handling if the combiner instance has 
its
IRQ statuses across more than one register. Currently all active 
instances
only use one register, but we are getting close to 32 in one case, so I
wanted to accommodate a general case where an instance can combine more
than 32 IRQs.

Having said that, what I'm inclined to do is to remove the use of mask
on the status read form the register on the next two lines, and then let
irq_find_mapping fail if we are getting an unexpected IRQ, then call
handle_bad_irq(desc).

Do you have any other suggestions to handle the scenario? E.g., can we
safely disable the parent IRQ from this context if we see too many
spurious interrupts?

quoted

+
+		status = readl_relaxed(combiner->regs[reg].addr);
+		status &= combiner->regs[reg].mask;
+
+		while (status) {
+			bit = __ffs(status);
+			status &= ~(1 << bit);
+			hwirq = irq_nr(reg, bit);
+			virq = irq_find_mapping(combiner->domain, hwirq);
+			if (virq >= 0)

Small bug: virq == 0 shouldn't happen, since this would be an 
indication
that the hwirq doesn't have a mapping.

Will fix this on V10.

quoted

+				generic_handle_irq(virq);
+
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+/*
+ * irqchip callbacks
+ */
+
+static void combiner_irq_chip_mask_irq(struct irq_data *data)
+{
+	struct combiner *combiner = irq_data_get_irq_chip_data(data);
+	struct combiner_reg *reg = combiner->regs + 
irq_register(data->hwirq);
+
+	clear_bit(irq_bit(data->hwirq), &reg->mask);
+}
+
+static void combiner_irq_chip_unmask_irq(struct irq_data *data)
+{
+	struct combiner *combiner = irq_data_get_irq_chip_data(data);
+	struct combiner_reg *reg = combiner->regs + 
irq_register(data->hwirq);
+
+	set_bit(irq_bit(data->hwirq), &reg->mask);
+}
+
+static struct irq_chip irq_chip = {
+	.irq_mask = combiner_irq_chip_mask_irq,
+	.irq_unmask = combiner_irq_chip_unmask_irq,
+	.name = "qcom-irq-combiner"
+};
+
+/*
+ * irq_domain_ops callbacks
+ */
+
+static int combiner_irq_map(struct irq_domain *domain, unsigned int 
irq,
+				   irq_hw_number_t hwirq)
+{
+	struct combiner *combiner = domain->host_data;
+
+	if (hwirq >= combiner->nirqs)
+		return -EINVAL;

Given that this should have come from the translate function, can we
move the check there instead, so that we validate everything early?

Will fix this on V10.

quoted

+
+	irq_set_chip_and_handler(irq, &irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, combiner);
+	irq_set_noprobe(irq);
+	return 0;
+}
+
+static void combiner_irq_unmap(struct irq_domain *domain, unsigned 
int irq)
+{
+	struct irq_data *data = irq_get_irq_data(irq);
+
+	if (WARN_ON(!data))
+		return;

Can this happen?

I will remove this check on V10 given that irq_domain_disassociate 
already
has a check for a NULL irq_data.

quoted

+	irq_domain_reset_irq_data(data);
+}
+
+static int combiner_irq_translate(struct irq_domain *d, struct 
irq_fwspec *fws,
+				  unsigned long *hwirq, unsigned int *type)
+{
+	if (is_acpi_node(fws->fwnode)) {
+		if (WARN_ON((fws->param_count != 2) ||
+			    (fws->param[1] & IORESOURCE_IRQ_LOWEDGE) ||
+			    (fws->param[1] & IORESOURCE_IRQ_HIGHEDGE)))
+			return -EINVAL;
+
+		*hwirq = fws->param[0];
+		*type = fws->param[1];
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static const struct irq_domain_ops domain_ops = {
+	.map = combiner_irq_map,
+	.unmap = combiner_irq_unmap,
+	.translate = combiner_irq_translate
+};
+
+/*
+ * Device probing
+ */
+
+static acpi_status count_registers_cb(struct acpi_resource *ares, 
void *context)
+{
+	int *count = context;
+
+	if (ares->type == ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
+		++(*count);
+	return AE_OK;
+}
+
+static int count_registers(struct platform_device *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status status;
+	int count = 0;
+
+	if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
+		return -EINVAL;
+
+	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
+				     count_registers_cb, &count);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+	return count;
+}
+
+struct get_registers_context {
+	struct device *dev;
+	struct combiner *combiner;
+	int err;
+};
+
+static acpi_status get_registers_cb(struct acpi_resource *ares, void 
*context)
+{
+	struct get_registers_context *ctx = context;
+	struct acpi_resource_generic_register *reg;
+	phys_addr_t paddr;
+	void __iomem *vaddr;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
+		return AE_OK;
+
+	reg = &ares->data.generic_reg;
+	paddr = reg->address;
+	if ((reg->space_id != ACPI_SPACE_MEM) ||
+	    (reg->bit_offset != 0) ||
+	    (reg->bit_width > REG_SIZE)) {
+		dev_err(ctx->dev, "Bad register resource @%pa\n", &paddr);
+		ctx->err = -EINVAL;
+		return AE_ERROR;
+	}
+
+	vaddr = devm_ioremap(ctx->dev, reg->address, REG_SIZE);
+	if (IS_ERR(vaddr)) {
+		dev_err(ctx->dev, "Can't map register @%pa\n", &paddr);
+		ctx->err = PTR_ERR(vaddr);
+		return AE_ERROR;
+	}
+
+	ctx->combiner->regs[ctx->combiner->nregs].addr = vaddr;
+	ctx->combiner->nirqs += reg->bit_width;
+	ctx->combiner->nregs++;
+	return AE_OK;
+}
+
+static int get_registers(struct platform_device *pdev, struct 
combiner *comb)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status status;
+	struct get_registers_context ctx;
+
+	if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
+		return -EINVAL;
+
+	ctx.dev = &pdev->dev;
+	ctx.combiner = comb;
+	ctx.err = 0;
+
+	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
+				     get_registers_cb, &ctx);
+	if (ACPI_FAILURE(status))
+		return ctx.err;
+	return 0;
+}
+
+static int __init combiner_probe(struct platform_device *pdev)
+{
+	struct combiner *combiner;
+	size_t alloc_sz;
+	u32 nregs;
+	int err;
+
+	nregs = count_registers(pdev);
+	if (nregs <= 0) {
+		dev_err(&pdev->dev, "Error reading register resources\n");
+		return -EINVAL;
+	}
+
+	alloc_sz = sizeof(*combiner) + sizeof(struct combiner_reg) * nregs;
+	combiner = devm_kzalloc(&pdev->dev, alloc_sz, GFP_KERNEL);
+	if (!combiner)
+		return -ENOMEM;
+
+	err = get_registers(pdev, combiner);
+	if (err < 0)
+		return err;
+
+	combiner->parent_irq = platform_get_irq(pdev, 0);
+	if (combiner->parent_irq <= 0) {
+		dev_err(&pdev->dev, "Error getting IRQ resource\n");
+		return -EPROBE_DEFER;
+	}
+
+	combiner->domain = irq_domain_create_linear(pdev->dev.fwnode, 
combiner->nirqs,
+						    &domain_ops, combiner);
+	if (!combiner->domain)
+		/* Errors printed by irq_domain_create_linear */
+		return -ENODEV;
+
+	irq_set_chained_handler_and_data(combiner->parent_irq,
+					 combiner_handle_irq, combiner);
+
+	dev_info(&pdev->dev, "Initialized with [p=%d,n=%d,r=%p]\n",
+		 combiner->parent_irq, combiner->nirqs, combiner->regs[0].addr);
+	return 0;
+}
+
+static const struct acpi_device_id qcom_irq_combiner_acpi_match[] = {
+	{ "QCOM80B1", },
+	{ }
+};
+
+static struct platform_driver qcom_irq_combiner_probe = {
+	.driver = {
+		.name = "qcom-irq-combiner",
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(qcom_irq_combiner_acpi_match),
+	},
+	.probe = combiner_probe,
+};
+
+static int __init register_qcom_irq_combiner(void)
+{
+	return platform_driver_register(&qcom_irq_combiner_probe);
+}
+device_initcall(register_qcom_irq_combiner);

Other than the questions I have above, this now looks in a much better
shape. Hopefully Rafael will soon come back will his conclusions on the
first two patches, as I'd like to see this code to get some -next for a
while.

Yes, hopefully we can get to that in the following weeks.

Thanks,
Agustin

Thanks,

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

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

On 06/01/17 13:17, Agustin Vega-Frias wrote:

Hey Marc,

On 2017-01-05 11:48, Marc Zyngier wrote:

quoted

Hi Agustin,

On 14/12/16 22:10, Agustin Vega-Frias wrote:

quoted

Driver for interrupt combiners in the Top-level Control and Status
Registers (TCSR) hardware block in Qualcomm Technologies chips.

An interrupt combiner in this block combines a set of interrupts by
OR'ing the individual interrupt signals into a summary interrupt
signal routed to a parent interrupt controller, and provides read-
only, 32-bit registers to query the status of individual interrupts.
The status bit for IRQ n is bit (n % 32) within register (n / 32)
of the given combiner. Thus, each combiner can be described as a set
of register offsets and the number of IRQs managed.

Signed-off-by: Agustin Vega-Frias <redacted>
---
 drivers/irqchip/Kconfig             |   9 +
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/qcom-irq-combiner.c | 322 
++++++++++++++++++++++++++++++++++++
 3 files changed, 332 insertions(+)
 create mode 100644 drivers/irqchip/qcom-irq-combiner.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bc0af33..3e3430c 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig

@@ -279,3 +279,12 @@ config EZNPS_GIC
 config STM32_EXTI
 	bool
 	select IRQ_DOMAIN
+
+config QCOM_IRQ_COMBINER
+	bool "QCOM IRQ combiner support"
+	depends on ARCH_QCOM && ACPI
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Say yes here to add support for the IRQ combiner devices embedded
+	  in Qualcomm Technologies chips.

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e4dbfc8..1818a0b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile

@@ -74,3 +74,4 @@ obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
+obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o

diff --git a/drivers/irqchip/qcom-irq-combiner.c 

b/drivers/irqchip/qcom-irq-combiner.c
new file mode 100644
index 0000000..0055e08

--- /dev/null
+++ b/drivers/irqchip/qcom-irq-combiner.c

@@ -0,0 +1,322 @@
+/* Copyright (c) 2015-2016, 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.
+ */
+
+/*
+ * Driver for interrupt combiners in the Top-level Control and Status
+ * Registers (TCSR) hardware block in Qualcomm Technologies chips.
+ * An interrupt combiner in this block combines a set of interrupts 
by
+ * OR'ing the individual interrupt signals into a summary interrupt
+ * signal routed to a parent interrupt controller, and provides read-
+ * only, 32-bit registers to query the status of individual 
interrupts.
+ * The status bit for IRQ n is bit (n % 32) within register (n / 32)
+ * of the given combiner. Thus, each combiner can be described as a 
set
+ * of register offsets and the number of IRQs managed.
+ */
+
+#include <linux/acpi.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/platform_device.h>
+
+#define REG_SIZE 32
+
+struct combiner_reg {
+	void __iomem *addr;
+	unsigned long mask;
+};
+
+struct combiner {
+	struct irq_domain   *domain;
+	int                 parent_irq;
+	u32                 nirqs;
+	u32                 nregs;
+	struct combiner_reg regs[0];
+};
+
+static inline u32 irq_register(int irq)
+{
+	return irq / REG_SIZE;
+}
+
+static inline u32 irq_bit(int irq)
+{
+	return irq % REG_SIZE;
+
+}
+
+static inline int irq_nr(u32 reg, u32 bit)
+{
+	return reg * REG_SIZE + bit;
+}
+
+/*
+ * Handler for the cascaded IRQ.
+ */
+static void combiner_handle_irq(struct irq_desc *desc)
+{
+	struct combiner *combiner = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 reg;
+
+	chained_irq_enter(chip, desc);
+
+	for (reg = 0; reg < combiner->nregs; reg++) {
+		int virq;
+		int hwirq;
+		u32 bit;
+		u32 status;
+
+		if (combiner->regs[reg].mask == 0)
+			continue;

I'm a bit worried by this. If I understand it well, this is a pure
software construct (controlled from combiner_irq_chip_{un,}mask_irq) 
and
there is nothing that actually masks the interrupt at the HW level.

So if a device asserts its interrupt line, what mechanism do we have to
make sure that we don't end-up with the CPU pegged in interrupt 
context?

Yes, unfortunately this is a dumb hardware combiner; however, the
real use of mask here is to optimize IRQ handling if the combiner
instance has its IRQ statuses across more than one register.
Currently all active instances only use one register, but we are
getting close to 32 in one case, so I wanted to accommodate a general
case where an instance can combine more than 32 IRQs.

Having said that, what I'm inclined to do is to remove the use of mask
on the status read form the register on the next two lines, and then let
irq_find_mapping fail if we are getting an unexpected IRQ, then call
handle_bad_irq(desc).

Unfortunately, having a mapping in place is not an indication that
someone can handle the interrupt. Also, you still need to handle
mask/unmask, and I don't see how you manage that, given that this
interrupt combiner seems to be a glorified OR gate.

Do you have any other suggestions to handle the scenario? E.g., can we
safely disable the parent IRQ from this context if we see too many
spurious interrupts?

I don't think so. There is two issues here:
- If a device is stuck with an active interrupt, you won't be able to
reliably detect that this is an invalid condition (the mapping trick
above is not reliable, and generic_handle_irq doesn't return anything
useful)
- Even if you could detect it, what do you do? Killing the cascade
interrupt would also kill all the other devices. Is that something
acceptable in your setup? Can you implement mask/unmask the same way?

Thanks,

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

Hi Rafael,

On 2017-01-03 16:56, Rafael J. Wysocki wrote:

On Tue, Jan 3, 2017 at 4:19 PM, Agustin Vega-Frias
[off-list ref] wrote:

quoted

Hi,

Is there any more feedback on this beyond Lorenzo's suggestion to drop
the conditional check on the first patch?
How can we move forward on this series?

Essentially, I need to convince myself that patches [1-2/3] are fine
which hasn't happened yet.

Pinging again. Do you have any questions that might help with your
review? I have some minor changes I have to make to the driver itself
(patch 3) and I'd like to submit any changes you might want along with
those.

Thanks,
Agustin

Thanks,
Rafael

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

On 16/01/17 14:07, Agustin Vega-Frias wrote:

Hi Rafael,

On 2017-01-03 16:56, Rafael J. Wysocki wrote:

quoted

On Tue, Jan 3, 2017 at 4:19 PM, Agustin Vega-Frias
[off-list ref] wrote:

quoted

Hi,

Is there any more feedback on this beyond Lorenzo's suggestion to drop
the conditional check on the first patch?
How can we move forward on this series?

Essentially, I need to convince myself that patches [1-2/3] are fine
which hasn't happened yet.

Pinging again. Do you have any questions that might help with your
review? I have some minor changes I have to make to the driver itself
(patch 3) and I'd like to submit any changes you might want along with
those.

I'd like to add that these two initial patches are now a prerequisite
for Hanjun's series, so it'd be good to have an idea of where we're
going on that front.

Thanks,

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

On 2017/1/16 22:14, Marc Zyngier wrote: