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

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

[Agustin Vega-Frias]

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.10-rc4.

Changes V10 -> V11:
* Add probe table and use it to implement driver presence detection.
* Fix kernel doc formatting in drivers/acpi/irq.c and some minor style issues.
* Minor bug fixes in the driver.

Changes V9 -> V10:
* Add checks for the producer_consumer field to not use produced IRQ
  resources as consumed.
* Minor bug fixes in the driver.

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.

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 => irq.c}       | 221 +++++++++++++++++++++++++
 drivers/acpi/resource.c             |  18 +-
 drivers/base/platform.c             |   8 +
 drivers/irqchip/Kconfig             |   9 +
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/qcom-irq-combiner.c | 320 ++++++++++++++++++++++++++++++++++++
 include/asm-generic/vmlinux.lds.h   |   1 +
 include/linux/acpi.h                |  15 ++
 9 files changed, 593 insertions(+), 2 deletions(-)
 rename drivers/acpi/{gsi.c => irq.c} (28%)
 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.

[PATCH V11 1/3] ACPI: Generic GSI: Do not attempt to map non-GSI IRQs during bus scan

[Agustin Vega-Frias]

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 <agustinv@codeaurora.org>
---
 drivers/acpi/resource.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index cb57962..69cd430 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -43,6 +43,19 @@ 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 &&
+	       ext_irq->producer_consumer == ACPI_CONSUMER;
+}
+#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 +483,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.

[PATCH V11 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Agustin Vega-Frias]

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 <agustinv@codeaurora.org>
---
 drivers/acpi/Makefile             |   2 +-
 drivers/acpi/{gsi.c => irq.c}     | 221 ++++++++++++++++++++++++++++++++++++++
 drivers/base/platform.c           |   8 ++
 include/asm-generic/vmlinux.lds.h |   1 +
 include/linux/acpi.h              |  15 +++
 5 files changed, 246 insertions(+), 1 deletion(-)
 rename drivers/acpi/{gsi.c => irq.c} (28%)

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 28%
rename from drivers/acpi/gsi.c
rename to drivers/acpi/irq.c
index ee9e0f2..db4ee4c 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/irq.c
@@ -85,6 +85,227 @@ void acpi_unregister_gsi(u32 gsi)
 EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
 
 /**
+ * acpi_get_irq_source_fwhandle() - Retrieve fwhandle from IRQ resource source.
+ * @source: acpi_resource_source to use for the lookup.
+ *
+ * Description:
+ * Retrieve the fwhandle of the device referenced by the given IRQ resource
+ * source. In addition to it being a valid ACPI device, its _HID must be
+ * listed on the probe table of ACPI DSDT irqchips. The check is used to
+ * determine if a driver for the referenced device is available and decide
+ * if we should try deferred probing if the device has not been probed yet.
+ * Drivers must use the __dsdt_irqchip attribute when declaring the table
+ * of acpi_device_ids for the device, e.g.:
+ *
+ * static const struct acpi_device_id my_device_ids[] __dsdt_irqchip = {
+ *         { "ABCD0123", },
+ *         { }
+ * };
+ *
+ * Return:
+ * The referenced device fwhandle or NULL on failure
+ */
+static struct fwnode_handle *
+acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
+{
+	struct fwnode_handle *result = NULL;
+	struct acpi_device *device;
+	struct acpi_hardware_id *hwid;
+	struct acpi_device_id *devid;
+	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 (WARN_ON(ACPI_FAILURE(status)))
+		return NULL;
+
+	device = acpi_bus_get_acpi_device(handle);
+	if (WARN_ON(!device))
+		return NULL;
+
+	list_for_each_entry(hwid, &device->pnp.ids, list) {
+		for (devid = &__dsdt_irqchip_acpi_probe_table;
+		     devid < &__dsdt_irqchip_acpi_probe_table_end; devid++) {
+			if (devid->id && !strcmp(devid->id, hwid->id)) {
+				result = &device->fwnode;
+				break;
+			}
+		}
+	}
+
+	acpi_bus_put_acpi_device(device);
+
+	return result;
+}
+
+/*
+ * Context for the resource walk used to lookup IRQ resources.
+ * Contains a return code, the lookup index, and references to the flags
+ * and fwspec where the result is returned.
+ */
+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.
+ * @fwnode: matching fwnode
+ * @hwirq: hardware IRQ number
+ * @triggering: triggering attributes of hwirq
+ * @polarity: polarity attributes of hwirq
+ * @polarity: polarity attributes of hwirq
+ * @shareable: shareable attributes of hwirq
+ * @ctx: acpi_irq_parse_one_ctx updated by this function
+ *
+ * Description:
+ * Handle a matching IRQ resource by populating the given ctx with
+ * the information passed.
+ */
+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)
+{
+	if (!fwnode)
+		return;
+	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
+ *
+ * Description:
+ * 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 (eirq->producer_consumer == ACPI_PRODUCER)
+			return AE_OK;
+		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
+ *
+ * Description:
+ * Resolves an interrupt for a device by walking its CRS resources to find
+ * the appropriate ACPI IRQ resource and populating the given struct irq_fwspec
+ * and flags.
+ *
+ * Return:
+ * The result stored in ctx.rc by the callback, or the default -EINVAL value
+ * if an error occurs.
+ */
+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_walk_resources(handle, METHOD_NAME__CRS, acpi_irq_parse_one_cb, &ctx);
+	return ctx.rc;
+}
+
+/**
+ * acpi_irq_get - Lookup an ACPI IRQ resource and use it to initialize resource.
+ * @handle: ACPI device handle
+ * @index:  ACPI IRQ resource index to lookup
+ * @res:    Linux IRQ resource to initialize
+ *
+ * Description:
+ * Look for the ACPI IRQ resource with the given index and use it to initialize
+ * the given Linux IRQ resource.
+ *
+ * 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)
+{
+	struct irq_fwspec fwspec;
+	struct irq_domain *domain;
+	unsigned long flags;
+	int rc;
+
+	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..040d474 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 && has_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
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 0968d13..0763c8a 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -567,6 +567,7 @@
 	ACPI_PROBE_TABLE(irqchip)					\
 	ACPI_PROBE_TABLE(clksrc)					\
 	ACPI_PROBE_TABLE(iort)						\
+	ACPI_PROBE_TABLE(dsdt_irqchip)					\
 	EARLYCON_TABLE()
 
 #define INIT_TEXT							\
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 5b36974..15fac1a 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1056,6 +1056,11 @@ struct acpi_probe_entry {
 					  (&ACPI_PROBE_TABLE_END(t) -	\
 					   &ACPI_PROBE_TABLE(t)));	\
 	})
+
+#define __dsdt_irqchip __section(__dsdt_irqchip_acpi_probe_table)
+extern struct acpi_device_id __dsdt_irqchip_acpi_probe_table;
+extern struct acpi_device_id __dsdt_irqchip_acpi_probe_table_end;
+
 #else
 static inline int acpi_dev_get_property(struct acpi_device *adev,
 					const char *name, acpi_object_type type,
@@ -1153,4 +1158,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.

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

[Agustin Vega-Frias]

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 <agustinv@codeaurora.org>
---
 drivers/irqchip/Kconfig             |   9 +
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/qcom-irq-combiner.c | 320 ++++++++++++++++++++++++++++++++++++
 3 files changed, 330 insertions(+)
 create mode 100644 drivers/irqchip/qcom-irq-combiner.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index ae96731..125528f 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -283,3 +283,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 0e55d94..5a531863 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -75,3 +75,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..c558b0a
--- /dev/null
+++ b/drivers/irqchip/qcom-irq-combiner.c
@@ -0,0 +1,320 @@
+/* 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.
+ */
+
+#define pr_fmt(fmt) "QCOM80B1:" fmt
+
+#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 enabled;
+};
+
+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;
+
+		bit = readl_relaxed(combiner->regs[reg].addr);
+		status = bit & combiner->regs[reg].enabled;
+		if (!status)
+			pr_warn_ratelimited("Unexpected IRQ on CPU%d: (%08x %08lx %p)\n",
+					    smp_processor_id(), bit,
+					    combiner->regs[reg].enabled,
+					    combiner->regs[reg].addr);
+
+		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->enabled);
+}
+
+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->enabled);
+}
+
+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)
+{
+	irq_set_chip_and_handler(irq, &irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, domain->host_data);
+	irq_set_noprobe(irq);
+	return 0;
+}
+
+static void combiner_irq_unmap(struct irq_domain *domain, unsigned int irq)
+{
+	irq_domain_reset_irq_data(irq_get_irq_data(irq));
+}
+
+static int combiner_irq_translate(struct irq_domain *d, struct irq_fwspec *fws,
+				  unsigned long *hwirq, unsigned int *type)
+{
+	struct combiner *combiner = d->host_data;
+
+	if (is_acpi_node(fws->fwnode)) {
+		if (WARN_ON((fws->param_count != 2) ||
+			    (fws->param[0] >= combiner->nirqs) ||
+			    (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_ids[] __dsdt_irqchip = {
+	{ "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_ids),
+	},
+	.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.

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

[Rafael J. Wysocki]

On Fri, Jan 20, 2017 at 3:34 AM, Agustin Vega-Frias
<agustinv@codeaurora.org> 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.10-rc4.
>
> Changes V10 -> V11:
> * Add probe table and use it to implement driver presence detection.
> * Fix kernel doc formatting in drivers/acpi/irq.c and some minor style issues.
> * Minor bug fixes in the driver.

I'm travelling now
(http://marc.info/?l=linux-pm&m=148410629024194&w=2), but I'll get
to this when I'm back.

Thanks,
Rafael

Re: [PATCH V11 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Lorenzo Pieralisi]

On Thu, Jan 19, 2017 at 09:34:19PM -0500, Agustin Vega-Frias wrote:
> 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 <agustinv@codeaurora.org>
> ---
>  drivers/acpi/Makefile             |   2 +-
>  drivers/acpi/{gsi.c => irq.c}     | 221 ++++++++++++++++++++++++++++++++++++++
>  drivers/base/platform.c           |   8 ++
>  include/asm-generic/vmlinux.lds.h |   1 +
>  include/linux/acpi.h              |  15 +++
>  5 files changed, 246 insertions(+), 1 deletion(-)
>  rename drivers/acpi/{gsi.c => irq.c} (28%)
> 
> 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 28%
> rename from drivers/acpi/gsi.c
> rename to drivers/acpi/irq.c
> index ee9e0f2..db4ee4c 100644
> --- a/drivers/acpi/gsi.c
> +++ b/drivers/acpi/irq.c
> @@ -85,6 +85,227 @@ void acpi_unregister_gsi(u32 gsi)
>  EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
>  
>  /**
> + * acpi_get_irq_source_fwhandle() - Retrieve fwhandle from IRQ resource source.
> + * @source: acpi_resource_source to use for the lookup.
> + *
> + * Description:
> + * Retrieve the fwhandle of the device referenced by the given IRQ resource
> + * source. In addition to it being a valid ACPI device, its _HID must be
> + * listed on the probe table of ACPI DSDT irqchips. The check is used to
> + * determine if a driver for the referenced device is available and decide
> + * if we should try deferred probing if the device has not been probed yet.
> + * Drivers must use the __dsdt_irqchip attribute when declaring the table
> + * of acpi_device_ids for the device, e.g.:
> + *
> + * static const struct acpi_device_id my_device_ids[] __dsdt_irqchip = {
> + *         { "ABCD0123", },
> + *         { }
> + * };
> + *

Ok, thanks for doing that. In my opinion it should be done by reusing
the ACPI_DECLARE_PROBE_ENTRY mechanism by simply adding a flag to detect
whether the entry refers to a static table or AML and a corresponding
acpi_device_id for the matching.

This driver matching mechanism though is just an optimization, the IRQ
deferral mechanism you are putting in place is much more important, so
for now let's focus on getting code in v10 (plus all cosmetic "fixes"
introduced in this patch minus the linker section) agreed upon and
upstream, adding this linker section later is not a big deal IMO, it
must not slow you down.

So, no need for reposting anything let's focus on the IRQ probe deferral
core functionality and get it merged.

Thanks,
Lorenzo

> + * Return:
> + * The referenced device fwhandle or NULL on failure
> + */
> +static struct fwnode_handle *
> +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
> +{
> +	struct fwnode_handle *result = NULL;
> +	struct acpi_device *device;
> +	struct acpi_hardware_id *hwid;
> +	struct acpi_device_id *devid;
> +	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 (WARN_ON(ACPI_FAILURE(status)))
> +		return NULL;
> +
> +	device = acpi_bus_get_acpi_device(handle);
> +	if (WARN_ON(!device))
> +		return NULL;
> +
> +	list_for_each_entry(hwid, &device->pnp.ids, list) {
> +		for (devid = &__dsdt_irqchip_acpi_probe_table;
> +		     devid < &__dsdt_irqchip_acpi_probe_table_end; devid++) {
> +			if (devid->id && !strcmp(devid->id, hwid->id)) {
> +				result = &device->fwnode;
> +				break;
> +			}
> +		}
> +	}
> +
> +	acpi_bus_put_acpi_device(device);
> +
> +	return result;
> +}
> +
> +/*
> + * Context for the resource walk used to lookup IRQ resources.
> + * Contains a return code, the lookup index, and references to the flags
> + * and fwspec where the result is returned.
> + */
> +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.
> + * @fwnode: matching fwnode
> + * @hwirq: hardware IRQ number
> + * @triggering: triggering attributes of hwirq
> + * @polarity: polarity attributes of hwirq
> + * @polarity: polarity attributes of hwirq
> + * @shareable: shareable attributes of hwirq
> + * @ctx: acpi_irq_parse_one_ctx updated by this function
> + *
> + * Description:
> + * Handle a matching IRQ resource by populating the given ctx with
> + * the information passed.
> + */
> +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)
> +{
> +	if (!fwnode)
> +		return;
> +	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
> + *
> + * Description:
> + * 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 (eirq->producer_consumer == ACPI_PRODUCER)
> +			return AE_OK;
> +		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
> + *
> + * Description:
> + * Resolves an interrupt for a device by walking its CRS resources to find
> + * the appropriate ACPI IRQ resource and populating the given struct irq_fwspec
> + * and flags.
> + *
> + * Return:
> + * The result stored in ctx.rc by the callback, or the default -EINVAL value
> + * if an error occurs.
> + */
> +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_walk_resources(handle, METHOD_NAME__CRS, acpi_irq_parse_one_cb, &ctx);
> +	return ctx.rc;
> +}
> +
> +/**
> + * acpi_irq_get - Lookup an ACPI IRQ resource and use it to initialize resource.
> + * @handle: ACPI device handle
> + * @index:  ACPI IRQ resource index to lookup
> + * @res:    Linux IRQ resource to initialize
> + *
> + * Description:
> + * Look for the ACPI IRQ resource with the given index and use it to initialize
> + * the given Linux IRQ resource.
> + *
> + * 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)
> +{
> +	struct irq_fwspec fwspec;
> +	struct irq_domain *domain;
> +	unsigned long flags;
> +	int rc;
> +
> +	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..040d474 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 && has_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
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 0968d13..0763c8a 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -567,6 +567,7 @@
>  	ACPI_PROBE_TABLE(irqchip)					\
>  	ACPI_PROBE_TABLE(clksrc)					\
>  	ACPI_PROBE_TABLE(iort)						\
> +	ACPI_PROBE_TABLE(dsdt_irqchip)					\
>  	EARLYCON_TABLE()
>  
>  #define INIT_TEXT							\
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 5b36974..15fac1a 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1056,6 +1056,11 @@ struct acpi_probe_entry {
>  					  (&ACPI_PROBE_TABLE_END(t) -	\
>  					   &ACPI_PROBE_TABLE(t)));	\
>  	})
> +
> +#define __dsdt_irqchip __section(__dsdt_irqchip_acpi_probe_table)
> +extern struct acpi_device_id __dsdt_irqchip_acpi_probe_table;
> +extern struct acpi_device_id __dsdt_irqchip_acpi_probe_table_end;
> +
>  #else
>  static inline int acpi_dev_get_property(struct acpi_device *adev,
>  					const char *name, acpi_object_type type,
> @@ -1153,4 +1158,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.
> 

Re: [PATCH V11 3/3] irqchip: qcom: Add IRQ combiner driver

[Andy Shevchenko]

On Fri, Jan 20, 2017 at 4:34 AM, Agustin Vega-Frias
<agustinv@codeaurora.org> wrote:
> 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.

> +static inline u32 irq_register(int irq)
> +{
> +       return irq / REG_SIZE;
> +}
> +
> +static inline u32 irq_bit(int irq)
> +{
> +       return irq % REG_SIZE;
> +
> +}

Besides extra line I do not see a benefit of those helpers. On first
glance they even increase characters to type.

> +static inline int irq_nr(u32 reg, u32 bit)
> +{
> +       return reg * REG_SIZE + bit;
> +}

This one might make sense.

> +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;
> +
> +               bit = readl_relaxed(combiner->regs[reg].addr);
> +               status = bit & combiner->regs[reg].enabled;
> +               if (!status)
> +                       pr_warn_ratelimited("Unexpected IRQ on CPU%d: (%08x %08lx %p)\n",
> +                                           smp_processor_id(), bit,
> +                                           combiner->regs[reg].enabled,
> +                                           combiner->regs[reg].addr);
> +

> +               while (status) {
> +                       bit = __ffs(status);
> +                       status &= ~(1 << bit);

Interesting way of for_each_set_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
> + */

Useless.

> +/*
> + * irq_domain_ops callbacks
> + */

Ditto.

> +/*
> + * Device probing
> + */

Ditto.

> +static acpi_status count_registers_cb(struct acpi_resource *ares, void *context)
> +{
> +       int *count = context;

I would consider to define a struct. It would be easy to extend if needed and...

> +
> +       if (ares->type == ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
> +               ++(*count);

...allows not to use such of constructions. (I think above is
equivalent to ++*count).

> +       return AE_OK;

> +}
> +
> +static int count_registers(struct platform_device *pdev)
> +{
> +       struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);

You don't use adev, so, ACPI_HANDLE() ?

> +       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;
> +}

Oh, since you are using this just as a helper to get count first, why
not to combine this in one callback?
What's the benefit of separation?

> +
> +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;
> +       }

This all sounds to me like an OperationalRegion. But I'm not sure it's
suitable here.
Do you have ACPI table carved in stone?

> +
> +       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_ids[] __dsdt_irqchip = {
> +       { "QCOM80B1", },
> +       { }
> +};
> +
> +static struct platform_driver qcom_irq_combiner_probe = {
> +       .driver = {
> +               .name = "qcom-irq-combiner",

> +               .owner = THIS_MODULE,

Do you still need this?

> +               .acpi_match_table = ACPI_PTR(qcom_irq_combiner_ids),
> +       },

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V11 1/3] ACPI: Generic GSI: Do not attempt to map non-GSI IRQs during bus scan

[Andy Shevchenko]

On Fri, Jan 20, 2017 at 4:34 AM, Agustin Vega-Frias
<agustinv@codeaurora.org> wrote:
> 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.

> @@ -43,6 +43,19 @@ 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

#if IS_ENABLED() ?

> +static inline bool is_gsi(struct acpi_resource_extended_irq *ext_irq)
> +{
> +       return ext_irq->resource_source.string_length == 0 &&
> +              ext_irq->producer_consumer == ACPI_CONSUMER;
> +}
> +#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 +483,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);

And why not to create a helper called acpi_dev_get_gsi_irqresource()
instead of that one?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V11 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Andy Shevchenko]

On Fri, Jan 20, 2017 at 4:34 AM, Agustin Vega-Frias
<agustinv@codeaurora.org> wrote:
> 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.

> +static struct fwnode_handle *
> +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
> +{
> +       struct fwnode_handle *result = NULL;
> +       struct acpi_device *device;
> +       struct acpi_hardware_id *hwid;
> +       struct acpi_device_id *devid;
> +       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 (WARN_ON(ACPI_FAILURE(status)))
> +               return NULL;
> +
> +       device = acpi_bus_get_acpi_device(handle);
> +       if (WARN_ON(!device))
> +               return NULL;
> +

> +       list_for_each_entry(hwid, &device->pnp.ids, list) {

> +               for (devid = &__dsdt_irqchip_acpi_probe_table;
> +                    devid < &__dsdt_irqchip_acpi_probe_table_end; devid++) {
> +                       if (devid->id && !strcmp(devid->id, hwid->id)) {
> +                               result = &device->fwnode;
> +                               break;
> +                       }
> +               }

Looks like a candidate for linker table API. (I recommend to Cc Luis
for this part)

> +       }

> +/**
> + * acpi_irq_parse_one_match - Handle a matching IRQ resource.
> + * @fwnode: matching fwnode
> + * @hwirq: hardware IRQ number
> + * @triggering: triggering attributes of hwirq
> + * @polarity: polarity attributes of hwirq
> + * @polarity: polarity attributes of hwirq
> + * @shareable: shareable attributes of hwirq
> + * @ctx: acpi_irq_parse_one_ctx updated by this function
> + *
> + * Description:
> + * Handle a matching IRQ resource by populating the given ctx with
> + * the information passed.
> + */
> +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)
> +{

> +       if (!fwnode)
> +               return;

> +       ctx->rc = 0;

Perhaps ctx->rc = fwnode ? 0 : -EINVAL; ?

> +       *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;
> +}

> +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
> +{
> +       struct irq_fwspec fwspec;
> +       struct irq_domain *domain;
> +       unsigned long flags;
> +       int rc;
> +
> +       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)

Is rc = 0 an error?

> +               return -EINVAL;
> +
> +       res->start = rc;
> +       res->end = rc;
> +       res->flags = flags;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_irq_get);

> --- 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 && has_acpi_companion(&dev->dev)) {
> +               int ret;
> +
> +               ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> +               if (ret)
> +                       return ret;
> +       }

> +#ifdef CONFIG_ACPI_GENERIC_GSI

#if IS_ENABLED()

> +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)

Perhaps

static inline
int ...

> +{
> +       return -EINVAL;
> +}
> +#endif

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V11 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Lorenzo Pieralisi]

On Thu, Jan 26, 2017 at 03:01:18AM +0200, Andy Shevchenko wrote:
> On Fri, Jan 20, 2017 at 4:34 AM, Agustin Vega-Frias
> <agustinv@codeaurora.org> wrote:
> > 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.
> 
> > +static struct fwnode_handle *
> > +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
> > +{
> > +       struct fwnode_handle *result = NULL;
> > +       struct acpi_device *device;
> > +       struct acpi_hardware_id *hwid;
> > +       struct acpi_device_id *devid;
> > +       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 (WARN_ON(ACPI_FAILURE(status)))
> > +               return NULL;
> > +
> > +       device = acpi_bus_get_acpi_device(handle);
> > +       if (WARN_ON(!device))
> > +               return NULL;
> > +
> 
> > +       list_for_each_entry(hwid, &device->pnp.ids, list) {
> 
> > +               for (devid = &__dsdt_irqchip_acpi_probe_table;
> > +                    devid < &__dsdt_irqchip_acpi_probe_table_end; devid++) {
> > +                       if (devid->id && !strcmp(devid->id, hwid->id)) {
> > +                               result = &device->fwnode;
> > +                               break;
> > +                       }
> > +               }
> 
> Looks like a candidate for linker table API. (I recommend to Cc Luis
> for this part)

This linker table entry scheme is just an optimization and should
not gate the series.

> > +       }
> 
> > +/**
> > + * acpi_irq_parse_one_match - Handle a matching IRQ resource.
> > + * @fwnode: matching fwnode
> > + * @hwirq: hardware IRQ number
> > + * @triggering: triggering attributes of hwirq
> > + * @polarity: polarity attributes of hwirq
> > + * @polarity: polarity attributes of hwirq
> > + * @shareable: shareable attributes of hwirq
> > + * @ctx: acpi_irq_parse_one_ctx updated by this function
> > + *
> > + * Description:
> > + * Handle a matching IRQ resource by populating the given ctx with
> > + * the information passed.
> > + */
> > +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)
> > +{
> 
> > +       if (!fwnode)
> > +               return;
> 
> > +       ctx->rc = 0;
> 
> Perhaps ctx->rc = fwnode ? 0 : -EINVAL; ?
> 
> > +       *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;
> > +}
> 
> > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
> > +{
> > +       struct irq_fwspec fwspec;
> > +       struct irq_domain *domain;
> > +       unsigned long flags;
> > +       int rc;
> > +
> > +       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)
> 
> Is rc = 0 an error?

Yes.

> > +               return -EINVAL;
> > +
> > +       res->start = rc;
> > +       res->end = rc;
> > +       res->flags = flags;
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_irq_get);
> 
> > --- 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 && has_acpi_companion(&dev->dev)) {
> > +               int ret;
> > +
> > +               ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> > +               if (ret)
> > +                       return ret;
> > +       }
> 
> > +#ifdef CONFIG_ACPI_GENERIC_GSI
> 
> #if IS_ENABLED()
> 
> > +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)
> 
> Perhaps
> 
> static inline
> int ...

It is late -rc5 and notwithstanding cosmetics changes, can we make
progress with this patch series or not please ?

Thanks,
Lorenzo

Re: [PATCH V11 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Andy Shevchenko]

On Thu, Jan 26, 2017 at 12:15 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Thu, Jan 26, 2017 at 03:01:18AM +0200, Andy Shevchenko wrote:
>> On Fri, Jan 20, 2017 at 4:34 AM, Agustin Vega-Frias
>> <agustinv@codeaurora.org> wrote:
>> > 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.

>> > +       list_for_each_entry(hwid, &device->pnp.ids, list) {
>>
>> > +               for (devid = &__dsdt_irqchip_acpi_probe_table;
>> > +                    devid < &__dsdt_irqchip_acpi_probe_table_end; devid++) {
>> > +                       if (devid->id && !strcmp(devid->id, hwid->id)) {
>> > +                               result = &device->fwnode;
>> > +                               break;
>> > +                       }
>> > +               }
>>
>> Looks like a candidate for linker table API. (I recommend to Cc Luis
>> for this part)
>
> This linker table entry scheme is just an optimization and should
> not gate the series.

That's correct. Though Luis might give some advice regarding to this
piece of code.

>> > +#ifdef CONFIG_ACPI_GENERIC_GSI
>>
>> #if IS_ENABLED()

>> > +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)
>>
>> Perhaps
>>
>> static inline
>> int ...
>
> It is late -rc5 and notwithstanding cosmetics changes, can we make
> progress with this patch series or not please ?

I agree for the second case from what's left above, first one might be
useful to amend.
At the end it's a Rafael's word, I'm not insisting on cosmetic
changes. So, what about the rest?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V11 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Rafael J. Wysocki]

On Thursday, January 26, 2017 10:15:21 AM Lorenzo Pieralisi wrote:
> On Thu, Jan 26, 2017 at 03:01:18AM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 20, 2017 at 4:34 AM, Agustin Vega-Frias
> > <agustinv@codeaurora.org> wrote:
> > > 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.
> > 
> > > +static struct fwnode_handle *
> > > +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
> > > +{
> > > +       struct fwnode_handle *result = NULL;
> > > +       struct acpi_device *device;
> > > +       struct acpi_hardware_id *hwid;
> > > +       struct acpi_device_id *devid;
> > > +       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 (WARN_ON(ACPI_FAILURE(status)))
> > > +               return NULL;
> > > +
> > > +       device = acpi_bus_get_acpi_device(handle);
> > > +       if (WARN_ON(!device))
> > > +               return NULL;
> > > +
> > 
> > > +       list_for_each_entry(hwid, &device->pnp.ids, list) {
> > 
> > > +               for (devid = &__dsdt_irqchip_acpi_probe_table;
> > > +                    devid < &__dsdt_irqchip_acpi_probe_table_end; devid++) {
> > > +                       if (devid->id && !strcmp(devid->id, hwid->id)) {
> > > +                               result = &device->fwnode;
> > > +                               break;
> > > +                       }
> > > +               }
> > 
> > Looks like a candidate for linker table API. (I recommend to Cc Luis
> > for this part)
> 
> This linker table entry scheme is just an optimization and should
> not gate the series.
> 
> > > +       }
> > 
> > > +/**
> > > + * acpi_irq_parse_one_match - Handle a matching IRQ resource.
> > > + * @fwnode: matching fwnode
> > > + * @hwirq: hardware IRQ number
> > > + * @triggering: triggering attributes of hwirq
> > > + * @polarity: polarity attributes of hwirq
> > > + * @polarity: polarity attributes of hwirq
> > > + * @shareable: shareable attributes of hwirq
> > > + * @ctx: acpi_irq_parse_one_ctx updated by this function
> > > + *
> > > + * Description:
> > > + * Handle a matching IRQ resource by populating the given ctx with
> > > + * the information passed.
> > > + */
> > > +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)
> > > +{
> > 
> > > +       if (!fwnode)
> > > +               return;
> > 
> > > +       ctx->rc = 0;
> > 
> > Perhaps ctx->rc = fwnode ? 0 : -EINVAL; ?
> > 
> > > +       *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;
> > > +}
> > 
> > > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
> > > +{
> > > +       struct irq_fwspec fwspec;
> > > +       struct irq_domain *domain;
> > > +       unsigned long flags;
> > > +       int rc;
> > > +
> > > +       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)
> > 
> > Is rc = 0 an error?
> 
> Yes.
> 
> > > +               return -EINVAL;
> > > +
> > > +       res->start = rc;
> > > +       res->end = rc;
> > > +       res->flags = flags;
> > > +
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(acpi_irq_get);
> > 
> > > --- 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 && has_acpi_companion(&dev->dev)) {
> > > +               int ret;
> > > +
> > > +               ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > 
> > > +#ifdef CONFIG_ACPI_GENERIC_GSI
> > 
> > #if IS_ENABLED()
> > 
> > > +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)
> > 
> > Perhaps
> > 
> > static inline
> > int ...
> 
> It is late -rc5 and notwithstanding cosmetics changes, can we make
> progress with this patch series or not please ?

Yes, we can.

I've just returned from travels and have not had the time to look at things in
detail yet.

I sent a notice about my travel in advance specifically so people didn't expect
me to respond while I was traveling, but obviously everybody simply ignored it
and went ahead with their lives.  And then I get messages like this which
doesn't make me happy at all.

I will start processing things shortly, but there is a backlog I need to work
through and this one may not be the first item in it.

Thanks,
Rafael

Re: [PATCH V11 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Lorenzo Pieralisi]

On Thu, Jan 26, 2017 at 11:43:43AM +0100, Rafael J. Wysocki wrote:
> On Thursday, January 26, 2017 10:15:21 AM Lorenzo Pieralisi wrote:
> > On Thu, Jan 26, 2017 at 03:01:18AM +0200, Andy Shevchenko wrote:
> > > On Fri, Jan 20, 2017 at 4:34 AM, Agustin Vega-Frias
> > > <agustinv@codeaurora.org> wrote:
> > > > 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.
> > > 
> > > > +static struct fwnode_handle *
> > > > +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
> > > > +{
> > > > +       struct fwnode_handle *result = NULL;
> > > > +       struct acpi_device *device;
> > > > +       struct acpi_hardware_id *hwid;
> > > > +       struct acpi_device_id *devid;
> > > > +       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 (WARN_ON(ACPI_FAILURE(status)))
> > > > +               return NULL;
> > > > +
> > > > +       device = acpi_bus_get_acpi_device(handle);
> > > > +       if (WARN_ON(!device))
> > > > +               return NULL;
> > > > +
> > > 
> > > > +       list_for_each_entry(hwid, &device->pnp.ids, list) {
> > > 
> > > > +               for (devid = &__dsdt_irqchip_acpi_probe_table;
> > > > +                    devid < &__dsdt_irqchip_acpi_probe_table_end; devid++) {
> > > > +                       if (devid->id && !strcmp(devid->id, hwid->id)) {
> > > > +                               result = &device->fwnode;
> > > > +                               break;
> > > > +                       }
> > > > +               }
> > > 
> > > Looks like a candidate for linker table API. (I recommend to Cc Luis
> > > for this part)
> > 
> > This linker table entry scheme is just an optimization and should
> > not gate the series.
> > 
> > > > +       }
> > > 
> > > > +/**
> > > > + * acpi_irq_parse_one_match - Handle a matching IRQ resource.
> > > > + * @fwnode: matching fwnode
> > > > + * @hwirq: hardware IRQ number
> > > > + * @triggering: triggering attributes of hwirq
> > > > + * @polarity: polarity attributes of hwirq
> > > > + * @polarity: polarity attributes of hwirq
> > > > + * @shareable: shareable attributes of hwirq
> > > > + * @ctx: acpi_irq_parse_one_ctx updated by this function
> > > > + *
> > > > + * Description:
> > > > + * Handle a matching IRQ resource by populating the given ctx with
> > > > + * the information passed.
> > > > + */
> > > > +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)
> > > > +{
> > > 
> > > > +       if (!fwnode)
> > > > +               return;
> > > 
> > > > +       ctx->rc = 0;
> > > 
> > > Perhaps ctx->rc = fwnode ? 0 : -EINVAL; ?
> > > 
> > > > +       *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;
> > > > +}
> > > 
> > > > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
> > > > +{
> > > > +       struct irq_fwspec fwspec;
> > > > +       struct irq_domain *domain;
> > > > +       unsigned long flags;
> > > > +       int rc;
> > > > +
> > > > +       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)
> > > 
> > > Is rc = 0 an error?
> > 
> > Yes.
> > 
> > > > +               return -EINVAL;
> > > > +
> > > > +       res->start = rc;
> > > > +       res->end = rc;
> > > > +       res->flags = flags;
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(acpi_irq_get);
> > > 
> > > > --- 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 && has_acpi_companion(&dev->dev)) {
> > > > +               int ret;
> > > > +
> > > > +               ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> > > > +               if (ret)
> > > > +                       return ret;
> > > > +       }
> > > 
> > > > +#ifdef CONFIG_ACPI_GENERIC_GSI
> > > 
> > > #if IS_ENABLED()
> > > 
> > > > +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)
> > > 
> > > Perhaps
> > > 
> > > static inline
> > > int ...
> > 
> > It is late -rc5 and notwithstanding cosmetics changes, can we make
> > progress with this patch series or not please ?
> 
> Yes, we can.
> 
> I've just returned from travels and have not had the time to look at things in
> detail yet.
> 
> I sent a notice about my travel in advance specifically so people didn't expect
> me to respond while I was traveling, but obviously everybody simply ignored it
> and went ahead with their lives.  And then I get messages like this which
> doesn't make me happy at all.

I had to ask given that there are other series that depend on it and
that this code will eventually have to go via a tree that is not ACPI,
so that will require some coordination and it is getting late in the
cycle, that's all my message was meant for, apologies.

> I will start processing things shortly, but there is a backlog I need to work
> through and this one may not be the first item in it.

Ok no problems then, thank you !

Lorenzo

Re: [PATCH V11 1/3] ACPI: Generic GSI: Do not attempt to map non-GSI IRQs during bus scan

[Rafael J. Wysocki]

On Fri, Jan 20, 2017 at 3:34 AM, Agustin Vega-Frias
<agustinv@codeaurora.org> wrote:
> 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 <agustinv@codeaurora.org>

This one is fine by me, so feel free to add my ACK to it.

Or if you want me to apply it, please let me know.

> ---
>  drivers/acpi/resource.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index cb57962..69cd430 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -43,6 +43,19 @@ 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 &&
> +              ext_irq->producer_consumer == ACPI_CONSUMER;
> +}
> +#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 +483,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;
> --

Thanks,
Rafael

Re: [PATCH V11 3/3] irqchip: qcom: Add IRQ combiner driver

[Agustin Vega-Frias]

Hi Andy,

On 2017-01-25 19:44, Andy Shevchenko wrote:
> On Fri, Jan 20, 2017 at 4:34 AM, Agustin Vega-Frias
> <agustinv@codeaurora.org> wrote:
>> 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.
> 
>> +static inline u32 irq_register(int irq)
>> +{
>> +       return irq / REG_SIZE;
>> +}
>> +
>> +static inline u32 irq_bit(int irq)
>> +{
>> +       return irq % REG_SIZE;
>> +
>> +}
> 
> Besides extra line I do not see a benefit of those helpers. On first
> glance they even increase characters to type.
> 

Will remove these.

>> +static inline int irq_nr(u32 reg, u32 bit)
>> +{
>> +       return reg * REG_SIZE + bit;
>> +}
> 
> This one might make sense.
> 
>> +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;
>> +
>> +               bit = readl_relaxed(combiner->regs[reg].addr);
>> +               status = bit & combiner->regs[reg].enabled;
>> +               if (!status)
>> +                       pr_warn_ratelimited("Unexpected IRQ on CPU%d: 
>> (%08x %08lx %p)\n",
>> +                                           smp_processor_id(), bit,
>> +                                           
>> combiner->regs[reg].enabled,
>> +                                           combiner->regs[reg].addr);
>> +
> 
>> +               while (status) {
>> +                       bit = __ffs(status);
>> +                       status &= ~(1 << bit);
> 
> Interesting way of for_each_set_bit() ?
> 

I'm leaving this as-is since in arm64 using __ffs can be optimized
better by using the bic instruction.

>> +                       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
>> + */
> 
> Useless.
> 

Will remove

>> +/*
>> + * irq_domain_ops callbacks
>> + */
> 
> Ditto.
> 

Will remove

>> +/*
>> + * Device probing
>> + */
> 
> Ditto.
> 

Will remove

>> +static acpi_status count_registers_cb(struct acpi_resource *ares, 
>> void *context)
>> +{
>> +       int *count = context;
> 
> I would consider to define a struct. It would be easy to extend if 
> needed and...
> 

The intent here is always to get the count so I don't see value
in adding the struct.

>> +
>> +       if (ares->type == ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
>> +               ++(*count);
> 
> ...allows not to use such of constructions. (I think above is
> equivalent to ++*count).
> 

IMHO having the parentheses makes the code clearer.

>> +       return AE_OK;
> 
>> +}
>> +
>> +static int count_registers(struct platform_device *pdev)
>> +{
>> +       struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> 
> You don't use adev, so, ACPI_HANDLE() ?
> 

Will change

>> +       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;
>> +}
> 
> Oh, since you are using this just as a helper to get count first, why
> not to combine this in one callback?
> What's the benefit of separation?
> 

This is because we do an allocation based on the count first.

>> +
>> +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;
>> +       }
> 
> This all sounds to me like an OperationalRegion. But I'm not sure it's
> suitable here.
> Do you have ACPI table carved in stone?
> 

I decided to go with registers because in some cases these might be non-
contiguous.

>> +
>> +       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_ids[] 
>> __dsdt_irqchip = {
>> +       { "QCOM80B1", },
>> +       { }
>> +};
>> +
>> +static struct platform_driver qcom_irq_combiner_probe = {
>> +       .driver = {
>> +               .name = "qcom-irq-combiner",
> 
>> +               .owner = THIS_MODULE,
> 
> Do you still need this?
> 

Will remove

Thanks,
Agustin

>> +               .acpi_match_table = ACPI_PTR(qcom_irq_combiner_ids),
>> +       },
> 
> --
> With Best Regards,
> Andy Shevchenko

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