* [PATCH V10 1/3] ACPI: Generic GSI: Do not attempt to map non-GSI IRQs during bus scan
2017-01-18 16:46 [PATCH V10 0/3] irqchip: qcom: Add IRQ combiner driver Agustin Vega-Frias
@ 2017-01-18 16:46 ` Agustin Vega-Frias
2017-01-18 16:46 ` [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping Agustin Vega-Frias
2017-01-18 16:46 ` [PATCH V10 3/3] irqchip: qcom: Add IRQ combiner driver Agustin Vega-Frias
2 siblings, 0 replies; 14+ messages in thread
From: Agustin Vega-Frias @ 2017-01-18 16:46 UTC (permalink / raw)
To: linux-kernel, linux-acpi, linux-arm-kernel, rjw, lenb, tglx,
jason, marc.zyngier
Cc: lorenzo.pieralisi, timur, cov, agross, harba, jcm, msalter,
mlangsdo, ahs3, astone, graeme.gregory, guohanjun,
charles.garcia-tobin, 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.
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
2017-01-18 16:46 [PATCH V10 0/3] irqchip: qcom: Add IRQ combiner driver Agustin Vega-Frias
2017-01-18 16:46 ` [PATCH V10 1/3] ACPI: Generic GSI: Do not attempt to map non-GSI IRQs during bus scan Agustin Vega-Frias
@ 2017-01-18 16:46 ` Agustin Vega-Frias
2017-01-18 18:00 ` Andy Shevchenko
2017-01-31 22:10 ` Rafael J. Wysocki
2017-01-18 16:46 ` [PATCH V10 3/3] irqchip: qcom: Add IRQ combiner driver Agustin Vega-Frias
2 siblings, 2 replies; 14+ messages in thread
From: Agustin Vega-Frias @ 2017-01-18 16:46 UTC (permalink / raw)
To: linux-kernel, linux-acpi, linux-arm-kernel, rjw, lenb, tglx,
jason, marc.zyngier
Cc: lorenzo.pieralisi, timur, cov, agross, harba, jcm, msalter,
mlangsdo, ahs3, astone, graeme.gregory, guohanjun,
charles.garcia-tobin, 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 | 98 -----------------
drivers/acpi/irq.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/base/platform.c | 9 +-
include/linux/acpi.h | 10 ++
5 files changed, 301 insertions(+), 100 deletions(-)
delete mode 100644 drivers/acpi/gsi.c
create mode 100644 drivers/acpi/irq.c
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/gsi.c
deleted file mode 100644
index ee9e0f2..0000000
--- a/drivers/acpi/gsi.c
+++ /dev/null
@@ -1,98 +0,0 @@
-/*
- * ACPI GSI IRQ layer
- *
- * Copyright (C) 2015 ARM Ltd.
- * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-#include <linux/acpi.h>
-#include <linux/irq.h>
-#include <linux/irqdomain.h>
-#include <linux/of.h>
-
-enum acpi_irq_model_id acpi_irq_model;
-
-static struct fwnode_handle *acpi_gsi_domain_id;
-
-/**
- * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
- * @gsi: GSI IRQ number to map
- * @irq: pointer where linux IRQ number is stored
- *
- * irq location updated with irq value [>0 on success, 0 on failure]
- *
- * Returns: linux IRQ number on success (>0)
- * -EINVAL on failure
- */
-int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
-{
- struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
- DOMAIN_BUS_ANY);
-
- *irq = irq_find_mapping(d, gsi);
- /*
- * *irq == 0 means no mapping, that should
- * be reported as a failure
- */
- return (*irq > 0) ? *irq : -EINVAL;
-}
-EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
-
-/**
- * acpi_register_gsi() - Map a GSI to a linux IRQ number
- * @dev: device for which IRQ has to be mapped
- * @gsi: GSI IRQ number
- * @trigger: trigger type of the GSI number to be mapped
- * @polarity: polarity of the GSI to be mapped
- *
- * Returns: a valid linux IRQ number on success
- * -EINVAL on failure
- */
-int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
- int polarity)
-{
- struct irq_fwspec fwspec;
-
- if (WARN_ON(!acpi_gsi_domain_id)) {
- pr_warn("GSI: No registered irqchip, giving up\n");
- return -EINVAL;
- }
-
- fwspec.fwnode = acpi_gsi_domain_id;
- fwspec.param[0] = gsi;
- fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
- fwspec.param_count = 2;
-
- return irq_create_fwspec_mapping(&fwspec);
-}
-EXPORT_SYMBOL_GPL(acpi_register_gsi);
-
-/**
- * acpi_unregister_gsi() - Free a GSI<->linux IRQ number mapping
- * @gsi: GSI IRQ number
- */
-void acpi_unregister_gsi(u32 gsi)
-{
- struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
- DOMAIN_BUS_ANY);
- int irq = irq_find_mapping(d, gsi);
-
- irq_dispose_mapping(irq);
-}
-EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
-
-/**
- * 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
- * GSI interrupts
- */
-void __init acpi_set_irq_model(enum acpi_irq_model_id model,
- struct fwnode_handle *fwnode)
-{
- acpi_irq_model = model;
- acpi_gsi_domain_id = fwnode;
-}
diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
new file mode 100644
index 0000000..cadc4cdd
--- /dev/null
+++ b/drivers/acpi/irq.c
@@ -0,0 +1,282 @@
+/*
+ * ACPI GSI IRQ layer
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/acpi.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+
+enum acpi_irq_model_id acpi_irq_model;
+
+static struct fwnode_handle *acpi_gsi_domain_id;
+
+/**
+ * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
+ * @gsi: GSI IRQ number to map
+ * @irq: pointer where linux IRQ number is stored
+ *
+ * irq location updated with irq value [>0 on success, 0 on failure]
+ *
+ * Returns: linux IRQ number on success (>0)
+ * -EINVAL on failure
+ */
+int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
+{
+ struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
+ DOMAIN_BUS_ANY);
+
+ *irq = irq_find_mapping(d, gsi);
+ /*
+ * *irq == 0 means no mapping, that should
+ * be reported as a failure
+ */
+ return (*irq > 0) ? *irq : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
+
+/**
+ * acpi_register_gsi() - Map a GSI to a linux IRQ number
+ * @dev: device for which IRQ has to be mapped
+ * @gsi: GSI IRQ number
+ * @trigger: trigger type of the GSI number to be mapped
+ * @polarity: polarity of the GSI to be mapped
+ *
+ * Returns: a valid linux IRQ number on success
+ * -EINVAL on failure
+ */
+int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
+ int polarity)
+{
+ struct irq_fwspec fwspec;
+
+ if (WARN_ON(!acpi_gsi_domain_id)) {
+ pr_warn("GSI: No registered irqchip, giving up\n");
+ return -EINVAL;
+ }
+
+ fwspec.fwnode = acpi_gsi_domain_id;
+ fwspec.param[0] = gsi;
+ fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
+ fwspec.param_count = 2;
+
+ return irq_create_fwspec_mapping(&fwspec);
+}
+EXPORT_SYMBOL_GPL(acpi_register_gsi);
+
+/**
+ * acpi_unregister_gsi() - Free a GSI<->linux IRQ number mapping
+ * @gsi: GSI IRQ number
+ */
+void acpi_unregister_gsi(u32 gsi)
+{
+ struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
+ DOMAIN_BUS_ANY);
+ int irq = irq_find_mapping(d, gsi);
+
+ irq_dispose_mapping(irq);
+}
+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 (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
+ *
+ * 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
+ * GSI interrupts
+ */
+void __init acpi_set_irq_model(enum acpi_irq_model_id model,
+ struct fwnode_handle *fwnode)
+{
+ acpi_irq_model = model;
+ acpi_gsi_domain_id = fwnode;
+}
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 5b36974..03a94cd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1153,4 +1153,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.
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
2017-01-18 16:46 ` [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping Agustin Vega-Frias
@ 2017-01-18 18:00 ` Andy Shevchenko
2017-01-18 19:02 ` Agustin Vega-Frias
2017-01-31 22:10 ` Rafael J. Wysocki
1 sibling, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2017-01-18 18:00 UTC (permalink / raw)
To: Agustin Vega-Frias
Cc: linux-kernel, linux-acpi, linux-arm Mailing List,
Rafael J. Wysocki, Len Brown, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Lorenzo Pieralisi, Timur Tabi,
Christopher Covington, Andy Gross, harba, Jon Masters, msalter,
mlangsdo, Al Stone, astone, Graeme Gregory, guohanjun,
charles.garcia-tobin
On Wed, Jan 18, 2017 at 6:46 PM, 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.
>
Care to use -M -C when prepare this patch?
> drivers/acpi/gsi.c | 98 -----------------
> drivers/acpi/irq.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++++
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
2017-01-18 18:00 ` Andy Shevchenko
@ 2017-01-18 19:02 ` Agustin Vega-Frias
0 siblings, 0 replies; 14+ messages in thread
From: Agustin Vega-Frias @ 2017-01-18 19:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-kernel, linux-acpi, linux-arm Mailing List,
Rafael J. Wysocki, Len Brown, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Lorenzo Pieralisi, Timur Tabi,
Christopher Covington, Andy Gross, harba, Jon Masters, msalter,
mlangsdo, Al Stone, astone, Graeme Gregory, guohanjun,
charles.garcia-tobin
Hi Andy,
On 2017-01-18 13:00, Andy Shevchenko wrote:
> On Wed, Jan 18, 2017 at 6:46 PM, 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.
>>
>
> Care to use -M -C when prepare this patch?
>
I did use it, but the similarity index was below the default threshold.
I'll send in a new version of this patch with this fixed.
Thanks,
Agustin
>> drivers/acpi/gsi.c | 98 -----------------
>> drivers/acpi/irq.c | 282
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>
> --
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
2017-01-18 16:46 ` [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping Agustin Vega-Frias
2017-01-18 18:00 ` Andy Shevchenko
@ 2017-01-31 22:10 ` Rafael J. Wysocki
2017-02-02 22:38 ` Agustin Vega-Frias
1 sibling, 1 reply; 14+ messages in thread
From: Rafael J. Wysocki @ 2017-01-31 22:10 UTC (permalink / raw)
To: Agustin Vega-Frias
Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
linux-arm-kernel, Rafael J. Wysocki, Len Brown, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Lorenzo Pieralisi, Timur Tabi,
Christopher Covington, Andy Gross, harba, Jon Masters,
Mark Salter, Mark Langsdorf, Al Stone, astone, Graeme Gregory,
Hanjun Guo, Charles Garcia Tobin
On Wed, Jan 18, 2017 at 5:46 PM, 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.
>
> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
> ---
> drivers/acpi/Makefile | 2 +-
> drivers/acpi/gsi.c | 98 -----------------
> drivers/acpi/irq.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/base/platform.c | 9 +-
> include/linux/acpi.h | 10 ++
> 5 files changed, 301 insertions(+), 100 deletions(-)
> delete mode 100644 drivers/acpi/gsi.c
> create mode 100644 drivers/acpi/irq.c
>
> 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/gsi.c
> deleted file mode 100644
> index ee9e0f2..0000000
> --- a/drivers/acpi/gsi.c
> +++ /dev/null
> @@ -1,98 +0,0 @@
> -/*
> - * ACPI GSI IRQ layer
> - *
> - * Copyright (C) 2015 ARM Ltd.
> - * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */
> -#include <linux/acpi.h>
> -#include <linux/irq.h>
> -#include <linux/irqdomain.h>
> -#include <linux/of.h>
> -
> -enum acpi_irq_model_id acpi_irq_model;
> -
> -static struct fwnode_handle *acpi_gsi_domain_id;
> -
> -/**
> - * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
> - * @gsi: GSI IRQ number to map
> - * @irq: pointer where linux IRQ number is stored
> - *
> - * irq location updated with irq value [>0 on success, 0 on failure]
> - *
> - * Returns: linux IRQ number on success (>0)
> - * -EINVAL on failure
> - */
> -int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> -{
> - struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
> - DOMAIN_BUS_ANY);
> -
> - *irq = irq_find_mapping(d, gsi);
> - /*
> - * *irq == 0 means no mapping, that should
> - * be reported as a failure
> - */
> - return (*irq > 0) ? *irq : -EINVAL;
> -}
> -EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
> -
> -/**
> - * acpi_register_gsi() - Map a GSI to a linux IRQ number
> - * @dev: device for which IRQ has to be mapped
> - * @gsi: GSI IRQ number
> - * @trigger: trigger type of the GSI number to be mapped
> - * @polarity: polarity of the GSI to be mapped
> - *
> - * Returns: a valid linux IRQ number on success
> - * -EINVAL on failure
> - */
> -int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
> - int polarity)
> -{
> - struct irq_fwspec fwspec;
> -
> - if (WARN_ON(!acpi_gsi_domain_id)) {
> - pr_warn("GSI: No registered irqchip, giving up\n");
> - return -EINVAL;
> - }
> -
> - fwspec.fwnode = acpi_gsi_domain_id;
> - fwspec.param[0] = gsi;
> - fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
> - fwspec.param_count = 2;
> -
> - return irq_create_fwspec_mapping(&fwspec);
> -}
> -EXPORT_SYMBOL_GPL(acpi_register_gsi);
> -
> -/**
> - * acpi_unregister_gsi() - Free a GSI<->linux IRQ number mapping
> - * @gsi: GSI IRQ number
> - */
> -void acpi_unregister_gsi(u32 gsi)
> -{
> - struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
> - DOMAIN_BUS_ANY);
> - int irq = irq_find_mapping(d, gsi);
> -
> - irq_dispose_mapping(irq);
> -}
> -EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
> -
> -/**
> - * 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
> - * GSI interrupts
> - */
> -void __init acpi_set_irq_model(enum acpi_irq_model_id model,
> - struct fwnode_handle *fwnode)
> -{
> - acpi_irq_model = model;
> - acpi_gsi_domain_id = fwnode;
> -}
> diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
> new file mode 100644
> index 0000000..cadc4cdd
> --- /dev/null
> +++ b/drivers/acpi/irq.c
> @@ -0,0 +1,282 @@
> +/*
> + * ACPI GSI IRQ layer
> + *
> + * Copyright (C) 2015 ARM Ltd.
> + * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/acpi.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +
> +enum acpi_irq_model_id acpi_irq_model;
> +
> +static struct fwnode_handle *acpi_gsi_domain_id;
> +
> +/**
> + * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
> + * @gsi: GSI IRQ number to map
> + * @irq: pointer where linux IRQ number is stored
> + *
> + * irq location updated with irq value [>0 on success, 0 on failure]
> + *
> + * Returns: linux IRQ number on success (>0)
> + * -EINVAL on failure
> + */
> +int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> +{
> + struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
> + DOMAIN_BUS_ANY);
> +
> + *irq = irq_find_mapping(d, gsi);
> + /*
> + * *irq == 0 means no mapping, that should
> + * be reported as a failure
> + */
> + return (*irq > 0) ? *irq : -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
> +
> +/**
> + * acpi_register_gsi() - Map a GSI to a linux IRQ number
> + * @dev: device for which IRQ has to be mapped
> + * @gsi: GSI IRQ number
> + * @trigger: trigger type of the GSI number to be mapped
> + * @polarity: polarity of the GSI to be mapped
> + *
> + * Returns: a valid linux IRQ number on success
> + * -EINVAL on failure
> + */
> +int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
> + int polarity)
> +{
> + struct irq_fwspec fwspec;
> +
> + if (WARN_ON(!acpi_gsi_domain_id)) {
> + pr_warn("GSI: No registered irqchip, giving up\n");
> + return -EINVAL;
> + }
> +
> + fwspec.fwnode = acpi_gsi_domain_id;
> + fwspec.param[0] = gsi;
> + fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
> + fwspec.param_count = 2;
> +
> + return irq_create_fwspec_mapping(&fwspec);
> +}
> +EXPORT_SYMBOL_GPL(acpi_register_gsi);
> +
> +/**
> + * acpi_unregister_gsi() - Free a GSI<->linux IRQ number mapping
> + * @gsi: GSI IRQ number
> + */
> +void acpi_unregister_gsi(u32 gsi)
> +{
> + struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
> + DOMAIN_BUS_ANY);
> + int irq = irq_find_mapping(d, gsi);
> +
> + irq_dispose_mapping(irq);
> +}
> +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 (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
> + *
> + * 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
> + * GSI interrupts
> + */
> +void __init acpi_set_irq_model(enum acpi_irq_model_id model,
> + struct fwnode_handle *fwnode)
> +{
> + acpi_irq_model = model;
> + acpi_gsi_domain_id = fwnode;
> +}
> 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 code above is a bit convoluted. It would be better to write it as
something like
if (r && r->flags & IORESOURCE_DISABLED) {
struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
if (adev) {
int ret = acpi_irq_get(adev->handle, 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 5b36974..03a94cd 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1153,4 +1153,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*/
The rest looks reasonable to me.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
2017-01-31 22:10 ` Rafael J. Wysocki
@ 2017-02-02 22:38 ` Agustin Vega-Frias
0 siblings, 0 replies; 14+ messages in thread
From: Agustin Vega-Frias @ 2017-02-02 22:38 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
linux-arm-kernel, Rafael J. Wysocki, Len Brown, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Lorenzo Pieralisi, Timur Tabi,
Christopher Covington, Andy Gross, harba, Jon Masters,
Mark Salter, Mark Langsdorf, Al Stone, astone, Graeme Gregory,
Hanjun Guo, Charles Garcia Tobin, rjwysocki
Hi Rafael,
On 2017-01-31 17:10, Rafael J. Wysocki wrote:
> On Wed, Jan 18, 2017 at 5:46 PM, 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.
>>
>> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
>> ---
> [...]
>> 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 code above is a bit convoluted. It would be better to write it as
> something like
>
> if (r && r->flags & IORESOURCE_DISABLED) {
> struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
>
> if (adev) {
> int ret = acpi_irq_get(adev->handle, num, r);
>
> if (ret)
> return ret;
> }
> }
>
I changed this to:
r = platform_get_resource(dev, IORESOURCE_IRQ, num);
if (has_acpi_companion(&dev->dev)) {
if (r && r->flags & IORESOURCE_DISABLED) {
int ret;
ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
if (ret)
return ret;
}
}
To avoid errors when CONFIG_ACPI is disabled.
Thanks for the ACKs. V12 coming soon.
Agustin
>> +
>> /*
>> * 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 5b36974..03a94cd 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1153,4 +1153,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*/
>
> The rest looks reasonable to me.
>
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V10 3/3] irqchip: qcom: Add IRQ combiner driver
2017-01-18 16:46 [PATCH V10 0/3] irqchip: qcom: Add IRQ combiner driver Agustin Vega-Frias
2017-01-18 16:46 ` [PATCH V10 1/3] ACPI: Generic GSI: Do not attempt to map non-GSI IRQs during bus scan Agustin Vega-Frias
2017-01-18 16:46 ` [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping Agustin Vega-Frias
@ 2017-01-18 16:46 ` Agustin Vega-Frias
2017-01-19 10:02 ` Marc Zyngier
2 siblings, 1 reply; 14+ messages in thread
From: Agustin Vega-Frias @ 2017-01-18 16:46 UTC (permalink / raw)
To: linux-kernel, linux-acpi, linux-arm-kernel, rjw, lenb, tglx,
jason, marc.zyngier
Cc: lorenzo.pieralisi, timur, cov, agross, harba, jcm, msalter,
mlangsdo, ahs3, astone, graeme.gregory, guohanjun,
charles.garcia-tobin, 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 | 319 ++++++++++++++++++++++++++++++++++++
3 files changed, 329 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..b7f0631
--- /dev/null
+++ b/drivers/irqchip/qcom-irq-combiner.c
@@ -0,0 +1,319 @@
+/* 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) {
+ WARN_ON(readl_relaxed(combiner->regs[reg].addr) != 0);
+ continue;
+ }
+
+ status = readl_relaxed(combiner->regs[reg].addr);
+ WARN_ON((status & ~combiner->regs[reg].mask) != 0);
+ 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), ®->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), ®->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)
+{
+ 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_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.
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH V10 3/3] irqchip: qcom: Add IRQ combiner driver
2017-01-18 16:46 ` [PATCH V10 3/3] irqchip: qcom: Add IRQ combiner driver Agustin Vega-Frias
@ 2017-01-19 10:02 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2017-01-19 10:02 UTC (permalink / raw)
To: Agustin Vega-Frias, linux-kernel, linux-acpi, linux-arm-kernel,
rjw, lenb, tglx, jason
Cc: lorenzo.pieralisi, timur, cov, agross, harba, jcm, msalter,
mlangsdo, ahs3, astone, graeme.gregory, guohanjun,
charles.garcia-tobin
Hi Agustin,
On 18/01/17 16:46, Agustin Vega-Frias 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.
>
> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
> ---
> drivers/irqchip/Kconfig | 9 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/qcom-irq-combiner.c | 319 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 329 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..b7f0631
> --- /dev/null
> +++ b/drivers/irqchip/qcom-irq-combiner.c
> @@ -0,0 +1,319 @@
> +/* 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) {
> + WARN_ON(readl_relaxed(combiner->regs[reg].addr) != 0);
Since we've now established that you couldn't really mask interrupts at
all, screaming like this is just going to make the matter worse. Please
drop this, or at the very least ratelimit it. Also, this is really
tortuous since your 'mask' is actually an 'enable' (you only accept an
interrupt if the bit is set).
But let's look at what you're testing here: If no interrupt is enabled,
scream if something is pending. I really wonder what you get by doing
so -- pending and masked interrupts are extremely common, and because
you cannot handle them doesn't mean you should kill the machine (which
your WARN_ON is guaranteed to do). Also, this should be consolidated
with the code below...
> + continue;
> + }
> +
> + status = readl_relaxed(combiner->regs[reg].addr);
> + WARN_ON((status & ~combiner->regs[reg].mask) != 0);
> + status &= combiner->regs[reg].mask;
Same thing: protesting against masked+pending interrupts is futile. If
they happen, your CPU is wedged anyway. I'd suggest that you drop the 0
test case (which doesn't save anything anyway):
#define pr_fmt(fmt) "QCOM80B1:" fmt
bit = readl_relaxed(combiner->regs[reg].addr);
status = bit & combiner->regs[reg].mask;
if (!status)
pr_warn_ratelimited("screaming interrupt (%08x %08x), CPU%d wedged\n",
bit, combiner->regs[reg].mask, smp_processor_id());
and that's it, as the rest of the code already takes care of the status==0
case. Optionally, you could disable the parent interrupt and kill all the
other devices connected to the same combiner, since that's your only
alternative to loosing a CPU.
> +
> + 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), ®->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), ®->mask);
> +}
I'd still advocate for 'mask' to be renamed 'enabled', or to have the
bits flipped the other way around. Your call.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
2017-01-18 19:04 [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping Agustin Vega-Frias
@ 2017-01-18 19:45 ` Andy Shevchenko
2017-01-19 12:36 ` Lorenzo Pieralisi
2017-01-19 15:40 ` Agustin Vega-Frias
0 siblings, 2 replies; 14+ messages in thread
From: Andy Shevchenko @ 2017-01-18 19:45 UTC (permalink / raw)
To: Agustin Vega-Frias
Cc: linux-kernel, linux-acpi, linux-arm Mailing List,
Rafael J. Wysocki, Len Brown, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Lorenzo Pieralisi, Timur Tabi,
Christopher Covington, Andy Gross, harba, Jon Masters, msalter,
mlangsdo, Al Stone, astone, Graeme Gregory, guohanjun,
charles.garcia-tobin
On Wed, Jan 18, 2017 at 9:04 PM, 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.
Thanks, looks better now.
> /**
> + * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
> + * acpi_resource_source which is used
> + * as an IRQ domain id
Please, make short description here and put long one below
> + * @source: acpi_resource_source to use for the lookup
> + *
* Description:
* ...
*
> + * Returns: The appropriate IRQ fwhandle domain id
> + * NULL on failure
* Return:
* blablabla
> + */
> +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);
I'm not sure both messages have a value.
> + return NULL;
> + }
> +/**
You mark as kernel doc, but in fact it's just a comment.
> + * 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
This looks better, but lacks of parameter descriptions.
> + */
> +/**
> + * 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
Make it shorter, ideally into one line
> + *
* 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
* Return:
* blablabla
> + * IRQ resource was found.
> + */
> +/**
> + * 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.
Ditto for Description and Return.
> + */
> +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;
Shouldn't you have the same in rc? Would be redundant.
> + 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
Ah, you missed colons after field names:
* @field1:
> + *
> + * Return:
Next line: 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;
Put this last in the definition block.
> + 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;
Hmm... Could it be other issues here?
> +
> + rc = irq_create_fwspec_mapping(&fwspec);
> + if (rc <= 0)
> + return -EINVAL;
> +
> + res->start = rc;
> + res->end = rc;
> + res->flags = flags;
Perhaps struct resource *r should be a parameter to acpi_irq_parse_one().
> +
> + 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)) {
has_acpi_companion() ?
> + int ret;
> +
> + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> + if (ret)
> + return ret;
> + }
> @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
> memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head));
> }
> }
> -
It doesn't belong here.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
2017-01-18 19:45 ` Andy Shevchenko
@ 2017-01-19 12:36 ` Lorenzo Pieralisi
2017-01-19 16:14 ` Agustin Vega-Frias
2017-01-19 15:40 ` Agustin Vega-Frias
1 sibling, 1 reply; 14+ messages in thread
From: Lorenzo Pieralisi @ 2017-01-19 12:36 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Agustin Vega-Frias, linux-kernel, linux-acpi,
linux-arm Mailing List, Rafael J. Wysocki, Len Brown,
Thomas Gleixner, Jason Cooper, Marc Zyngier, Timur Tabi,
Christopher Covington, Andy Gross, harba, Jon Masters, msalter,
mlangsdo, Al Stone, astone, Graeme Gregory, guohanjun,
charles.garcia-tobin
On Wed, Jan 18, 2017 at 09:45:56PM +0200, Andy Shevchenko wrote:
[...]
> > +/**
> > + * 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
>
> Ah, you missed colons after field names:
> * @field1:
>
> > + *
> > + * Return:
>
> Next line: 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;
>
> Put this last in the definition block.
>
> > + 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;
>
> Hmm... Could it be other issues here?
That's a good point. Here probing should be deferred only if we know a
driver capable of handling fwspec.fwnode will have a chance to be
probed/initialized eventually right (which basically means it is
compiled in the kernel and we hope it will probed successfully) ? I am
not sure there is an easy way to detect that in ACPI at the moment, I
suspect it is time we added a linker section (or augment the existing
one used for irqchip early MADT parsing - IRQCHIP_ACPI_DECLARE) for this
purpose I do not see any other option (apart from leaving devices in the
deferred probe list if a driver for the irqchip represented by
fwspec.fwnode is not present in the kernel, which is a bit sloppy, or
resorting to checking Kconfig entries to detect compile time enabled
irqchip drivers).
> > +
> > + rc = irq_create_fwspec_mapping(&fwspec);
> > + if (rc <= 0)
> > + return -EINVAL;
> > +
>
> > + res->start = rc;
> > + res->end = rc;
> > + res->flags = flags;
>
> Perhaps struct resource *r should be a parameter to acpi_irq_parse_one().
Yeah but then you would end up with flags initialized in
acpi_irq_parse_one() and the other resource params (ie start, end) here,
I do not think it is nicer.
Thanks,
Lorenzo
>
> > +
> > + 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)) {
>
> has_acpi_companion() ?
>
> > + int ret;
> > +
> > + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> > + if (ret)
> > + return ret;
> > + }
>
> > @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
> > memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head));
> > }
> > }
> > -
>
> It doesn't belong here.
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
2017-01-19 12:36 ` Lorenzo Pieralisi
@ 2017-01-19 16:14 ` Agustin Vega-Frias
0 siblings, 0 replies; 14+ messages in thread
From: Agustin Vega-Frias @ 2017-01-19 16:14 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Andy Shevchenko, linux-kernel, linux-acpi,
linux-arm Mailing List, Rafael J. Wysocki, Len Brown,
Thomas Gleixner, Jason Cooper, Marc Zyngier, Timur Tabi,
Christopher Covington, Andy Gross, harba, Jon Masters, msalter,
mlangsdo, Al Stone, astone, Graeme Gregory, guohanjun,
charles.garcia-tobin
On 2017-01-19 07:36, Lorenzo Pieralisi wrote:
> On Wed, Jan 18, 2017 at 09:45:56PM +0200, Andy Shevchenko wrote:
>
> [...]
>
>> > +/**
>> > + * 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
>>
>> Ah, you missed colons after field names:
>> * @field1:
>>
>> > + *
>> > + * Return:
>>
>> Next line: 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;
>>
>> Put this last in the definition block.
>>
>> > + 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;
>>
>> Hmm... Could it be other issues here?
>
> That's a good point. Here probing should be deferred only if we know a
> driver capable of handling fwspec.fwnode will have a chance to be
> probed/initialized eventually right (which basically means it is
> compiled in the kernel and we hope it will probed successfully) ? I am
> not sure there is an easy way to detect that in ACPI at the moment, I
> suspect it is time we added a linker section (or augment the existing
> one used for irqchip early MADT parsing - IRQCHIP_ACPI_DECLARE) for
> this
> purpose I do not see any other option (apart from leaving devices in
> the
> deferred probe list if a driver for the irqchip represented by
> fwspec.fwnode is not present in the kernel, which is a bit sloppy, or
> resorting to checking Kconfig entries to detect compile time enabled
> irqchip drivers).
I'll add the linker section suggested by Lorenzo for the extra check.
We can do that check in acpi_get_irq_source_fwhandle and make the lookup
fail if the device is not listed in the table.
That way we can keep this check as-is.
Thoughts?
>
>> > +
>> > + rc = irq_create_fwspec_mapping(&fwspec);
>> > + if (rc <= 0)
>> > + return -EINVAL;
>> > +
>>
>> > + res->start = rc;
>> > + res->end = rc;
>> > + res->flags = flags;
>>
>> Perhaps struct resource *r should be a parameter to
>> acpi_irq_parse_one().
>
> Yeah but then you would end up with flags initialized in
> acpi_irq_parse_one() and the other resource params (ie start, end)
> here,
> I do not think it is nicer.
The reason this took this form is that we are trying to get some
symmetry
with what of_irq_get does.
Thanks,
Agustin
>
> Thanks,
> Lorenzo
>
> [...]
>
>>
>> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
2017-01-18 19:45 ` Andy Shevchenko
2017-01-19 12:36 ` Lorenzo Pieralisi
@ 2017-01-19 15:40 ` Agustin Vega-Frias
1 sibling, 0 replies; 14+ messages in thread
From: Agustin Vega-Frias @ 2017-01-19 15:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-kernel, linux-acpi, linux-arm Mailing List,
Rafael J. Wysocki, Len Brown, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Lorenzo Pieralisi, Timur Tabi,
Christopher Covington, Andy Gross, harba, Jon Masters, msalter,
mlangsdo, Al Stone, astone, Graeme Gregory, guohanjun,
charles.garcia-tobin
Hi Andy,
On 2017-01-18 14:45, Andy Shevchenko wrote:
> On Wed, Jan 18, 2017 at 9:04 PM, Agustin Vega-Frias
> <agustinv@codeaurora.org> wrote:
>
> [...]
>
>
>> + */
>> +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);
>
> I'm not sure both messages have a value.
I'll probably just drop the explicit pr_warns and just use WARN_ON on
the if condition. Is that a reasonable alternative in your view?
>
>> + return NULL;
>> + }
>
>
> [...]
>
>> + */
>> +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;
>
> Shouldn't you have the same in rc? Would be redundant.
>
Good point, since we always return -EINVAL on failure we can skip
the check since rc will only be updated on success.
>> + return ctx.rc;
>> +}
>> +
>
> [...]
>
>> + domain = irq_find_matching_fwnode(fwspec.fwnode,
>> DOMAIN_BUS_ANY);
>> + if (!domain)
>> + return -EPROBE_DEFER;
>
> Hmm... Could it be other issues here?
I'll comment on this on Lorenzo's reply.
>
>> +
>> + rc = irq_create_fwspec_mapping(&fwspec);
>> + if (rc <= 0)
>> + return -EINVAL;
>> +
>
>> + res->start = rc;
>> + res->end = rc;
>> + res->flags = flags;
>
> Perhaps struct resource *r should be a parameter to
> acpi_irq_parse_one().
I'll comment on this on Lorenzo's reply.
>
>> +
>> + 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
I'll address the other issues on V11.
Thanks for your thorough review,
Agustin
>
> --
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread