Add support for IRQ combiners in the Top-level Control and Status
Registers (TCSR) hardware block in Qualcomm Technologies chips.
The first patch fixes IRQ probe deferral by allowing platform_device
IRQ resources to be re-initialized if the ACPI core failed to find
the IRQ domain during ACPI bus scan.
The second patch adds support for ResourceSource/IRQ domain mapping
when using Extended IRQ Resources with a specific ResourceSource.
These changes are conditional on the ACPI_GENERIC_GSI config.
The third patch takes advantage of the new capabilities to implement
the driver for the IRQ combiners.
Tested on top of v4.9-rc4.
Changes V6 -> V7:
* Consolidate changes for ResourceSource/IRQ domain mapping to the same
source file implementing the generic GSI support, making it conditional
on CONFIG_ACPI_GENERIC_GSI.
* Eliminate some code duplication by implementing acpi_register_gsi in
terms of the new acpi_register_irq API.
Changes V5 -> V6:
* Drop probe table and on-demand probing based on resource_source
* Add patch to fix probe deferral
* Change back combiner driver to use the platform_device/platform_driver
APIs
Agustin Vega-Frias (3):
ACPI: Retry IRQ conversion if it failed previously
ACPI: Add support for ResourceSource/IRQ domain mapping
irqchip: qcom: Add IRQ combiner driver
drivers/acpi/Makefile | 2 +-
drivers/acpi/{gsi.c => irq.c} | 98 +++++++++--
drivers/acpi/resource.c | 88 ++++++++--
drivers/base/platform.c | 9 +-
drivers/irqchip/Kconfig | 8 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/qcom-irq-combiner.c | 337 ++++++++++++++++++++++++++++++++++++
include/linux/acpi.h | 26 +++
8 files changed, 541 insertions(+), 28 deletions(-)
rename drivers/acpi/{gsi.c => irq.c} (53%)
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.
This allows probe deferral to work properly when a dependent device
fails to get a valid IRQ because the IRQ domain was not registered
at the time the resources were added to the platform_device.
Signed-off-by: Agustin Vega-Frias <[email protected]>
---
drivers/acpi/resource.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
drivers/base/platform.c | 9 +++++++-
include/linux/acpi.h | 7 ++++++
3 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 56241eb..4beda15 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
return (type & types) ? 0 : 1;
}
EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type);
+
+struct acpi_irq_get_ctx {
+ unsigned int index;
+ struct resource *res;
+};
+
+static acpi_status acpi_irq_get_cb(struct acpi_resource *ares, void *context)
+{
+ struct acpi_irq_get_ctx *ctx = context;
+ struct acpi_resource_irq *irq;
+ struct acpi_resource_extended_irq *ext_irq;
+
+ switch (ares->type) {
+ case ACPI_RESOURCE_TYPE_IRQ:
+ irq = &ares->data.irq;
+ if (ctx->index < irq->interrupt_count) {
+ acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
+ return AE_CTRL_TERMINATE;
+ }
+ ctx->index -= irq->interrupt_count;
+ break;
+ case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+ ext_irq = &ares->data.extended_irq;
+ if (ctx->index < ext_irq->interrupt_count) {
+ acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
+ return AE_CTRL_TERMINATE;
+ }
+ ctx->index -= ext_irq->interrupt_count;
+ break;
+ }
+
+ return AE_OK;
+}
+
+/**
+ * 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)
+{
+ struct acpi_irq_get_ctx ctx = { index, res };
+ acpi_status status;
+
+ status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+ acpi_irq_get_cb, &ctx);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+ if (res->flags & IORESOURCE_DISABLED)
+ return -EPROBE_DEFER;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_irq_get);
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 689a8b9..325bdb9 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -406,6 +406,7 @@ bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
unsigned int acpi_dev_get_irq_type(int triggering, int polarity);
bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
struct resource *res);
+int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res);
void acpi_dev_free_resource_list(struct list_head *list);
int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
@@ -763,6 +764,12 @@ static inline int acpi_reconfig_notifier_unregister(struct notifier_block *nb)
return -EINVAL;
}
+static inline int acpi_irq_get(acpi_handle handle, unsigned int index,
+ struct resource *res)
+{
+ return -EINVAL;
+}
+
#endif /* !CONFIG_ACPI */
#ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Driver for interrupt combiners in the Top-level Control and Status
Registers (TCSR) hardware block in Qualcomm Technologies chips.
An interrupt combiner in this block combines a set of interrupts by
OR'ing the individual interrupt signals into a summary interrupt
signal routed to a parent interrupt controller, and provides read-
only, 32-bit registers to query the status of individual interrupts.
The status bit for IRQ n is bit (n % 32) within register (n / 32)
of the given combiner. Thus, each combiner can be described as a set
of register offsets and the number of IRQs managed.
Signed-off-by: Agustin Vega-Frias <[email protected]>
---
drivers/irqchip/Kconfig | 8 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/qcom-irq-combiner.c | 337 ++++++++++++++++++++++++++++++++++++
3 files changed, 346 insertions(+)
create mode 100644 drivers/irqchip/qcom-irq-combiner.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bc0af33..610f902 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -279,3 +279,11 @@ config EZNPS_GIC
config STM32_EXTI
bool
select IRQ_DOMAIN
+
+config QCOM_IRQ_COMBINER
+ bool "QCOM IRQ combiner support"
+ depends on ARCH_QCOM
+ select IRQ_DOMAIN
+ help
+ Say yes here to add support for the IRQ combiner devices embedded
+ in Qualcomm Technologies chips.
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e4dbfc8..1818a0b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -74,3 +74,4 @@ obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o
obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o
obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
+obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
diff --git a/drivers/irqchip/qcom-irq-combiner.c b/drivers/irqchip/qcom-irq-combiner.c
new file mode 100644
index 0000000..fc25251
--- /dev/null
+++ b/drivers/irqchip/qcom-irq-combiner.c
@@ -0,0 +1,337 @@
+/* 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_chip irq_chip;
+ struct irq_domain *domain;
+ int parent_irq;
+ u32 nirqs;
+ u32 nregs;
+ struct combiner_reg regs[0];
+};
+
+static inline u32 irq_register(int irq)
+{
+ return irq / REG_SIZE;
+}
+
+static inline u32 irq_bit(int irq)
+{
+ return irq % REG_SIZE;
+
+}
+
+static inline int irq_nr(u32 reg, u32 bit)
+{
+ return reg * REG_SIZE + bit;
+}
+
+/*
+ * Handler for the cascaded IRQ.
+ */
+static void combiner_handle_irq(struct irq_desc *desc)
+{
+ struct combiner *combiner = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ u32 reg;
+
+ chained_irq_enter(chip, desc);
+
+ for (reg = 0; reg < combiner->nregs; reg++) {
+ int virq;
+ int hwirq;
+ u32 bit;
+ u32 status;
+
+ if (combiner->regs[reg].mask == 0)
+ continue;
+
+ status = readl_relaxed(combiner->regs[reg].addr);
+ status &= combiner->regs[reg].mask;
+
+ while (status) {
+ bit = __ffs(status);
+ status &= ~(1 << bit);
+ hwirq = irq_nr(reg, bit);
+ virq = irq_find_mapping(combiner->domain, hwirq);
+ if (virq >= 0)
+ generic_handle_irq(virq);
+
+ }
+ }
+
+ chained_irq_exit(chip, desc);
+}
+
+/*
+ * irqchip callbacks
+ */
+
+static void combiner_irq_chip_mask_irq(struct irq_data *data)
+{
+ struct combiner *combiner = irq_data_get_irq_chip_data(data);
+ struct combiner_reg *reg = combiner->regs + irq_register(data->hwirq);
+
+ clear_bit(irq_bit(data->hwirq), ®->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);
+}
+
+/*
+ * irq_domain_ops callbacks
+ */
+
+static int combiner_irq_map(struct irq_domain *domain, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ struct combiner *combiner = domain->host_data;
+
+ if (hwirq >= combiner->nirqs)
+ return -EINVAL;
+
+ irq_set_chip_and_handler(irq, &combiner->irq_chip, handle_level_irq);
+ irq_set_chip_data(irq, combiner);
+ irq_set_parent(irq, combiner->parent_irq);
+ irq_set_noprobe(irq);
+ return 0;
+}
+
+static void combiner_irq_unmap(struct irq_domain *domain, unsigned int irq)
+{
+ irq_set_chip_and_handler(irq, NULL, NULL);
+ irq_set_chip_data(irq, NULL);
+ irq_set_parent(irq, -1);
+}
+
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+static int combiner_irq_translate(struct irq_domain *d, struct irq_fwspec *fws,
+ unsigned long *hwirq, unsigned int *type)
+{
+ if (is_acpi_node(fws->fwnode)) {
+ if (fws->param_count != 2)
+ return -EINVAL;
+
+ *hwirq = fws->param[0];
+ *type = fws->param[1];
+ return 0;
+ }
+
+ return -EINVAL;
+}
+#endif
+
+static const struct irq_domain_ops domain_ops = {
+ .map = combiner_irq_map,
+ .unmap = combiner_irq_unmap,
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+ .translate = combiner_irq_translate
+#endif
+};
+
+/*
+ * Device probing
+ */
+
+#ifdef CONFIG_ACPI
+
+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;
+}
+
+#else /* !CONFIG_ACPI */
+
+static int count_registers(struct platform_device *pdev)
+{
+ return -EINVAL;
+}
+
+static int get_registers(struct platform_device *pdev, struct combiner *comb)
+{
+ return -EINVAL;
+}
+
+#endif
+
+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 -EINVAL;
+ }
+
+ 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);
+ combiner->irq_chip.irq_mask = combiner_irq_chip_mask_irq;
+ combiner->irq_chip.irq_unmask = combiner_irq_chip_unmask_irq;
+ combiner->irq_chip.name = pdev->name;
+
+ 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.
When an Extended IRQ Resource contains a valid ResourceSource
use it to map the IRQ on the domain associated with the ACPI
device referenced.
With this in place an irqchip driver can create its domain using
irq_domain_create_linear and pass the device fwnode to create
the domain mapping. When dependent devices are probed these
changes allow the ACPI core find the domain and map the IRQ.
Signed-off-by: Agustin Vega-Frias <[email protected]>
---
drivers/acpi/Makefile | 2 +-
drivers/acpi/{gsi.c => irq.c} | 98 +++++++++++++++++++++++++++++++++++++------
drivers/acpi/resource.c | 29 +++++++------
include/linux/acpi.h | 19 +++++++++
4 files changed, 121 insertions(+), 27 deletions(-)
rename drivers/acpi/{gsi.c => irq.c} (53%)
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 53%
rename from drivers/acpi/gsi.c
rename to drivers/acpi/irq.c
index ee9e0f2..c6ecaab 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/irq.c
@@ -18,6 +18,45 @@
static struct fwnode_handle *acpi_gsi_domain_id;
/**
+ * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
+ * acpi_resource_source which is used
+ * to be 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
+ */
+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;
+}
+EXPORT_SYMBOL_GPL(acpi_get_irq_source_fwhandle);
+
+/**
* 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
@@ -42,6 +81,50 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
/**
+ * acpi_register_irq() - Map a hardware to a linux IRQ number
+ * @source: IRQ source
+ * @hwirq: Hardware IRQ number
+ * @trigger: trigger type of the IRQ number to be mapped
+ * @polarity: polarity of the IRQ to be mapped
+ *
+ * Returns: a valid linux IRQ number on success
+ * -EINVAL on failure
+ */
+int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger,
+ int polarity)
+{
+ struct irq_fwspec fwspec;
+
+ if (!source)
+ return -EINVAL;
+
+ if (irq_find_matching_fwnode(source, DOMAIN_BUS_ANY) == NULL)
+ return -EPROBE_DEFER;
+
+ fwspec.fwnode = source;
+ fwspec.param[0] = hwirq;
+ 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_irq);
+
+/**
+ * acpi_unregister_irq() - Free a Hardware IRQ<->linux IRQ number mapping
+ * @hwirq: Hardware IRQ number
+ */
+void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq)
+{
+ struct irq_domain *d = irq_find_matching_fwnode(source,
+ DOMAIN_BUS_ANY);
+ int irq = irq_find_mapping(d, hwirq);
+
+ irq_dispose_mapping(irq);
+}
+EXPORT_SYMBOL_GPL(acpi_unregister_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
@@ -54,19 +137,12 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
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);
+ return acpi_register_irq(acpi_gsi_domain_id, gsi, trigger, polarity);
}
EXPORT_SYMBOL_GPL(acpi_register_gsi);
@@ -76,11 +152,7 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
*/
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);
+ acpi_unregister_irq(acpi_gsi_domain_id, gsi);
}
EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 4beda15..83cff00 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -374,21 +374,22 @@ unsigned int acpi_dev_get_irq_type(int triggering, int polarity)
}
EXPORT_SYMBOL_GPL(acpi_dev_get_irq_type);
-static void acpi_dev_irqresource_disabled(struct resource *res, u32 gsi)
+static void acpi_dev_irqresource_disabled(struct resource *res, u32 hwirq)
{
- res->start = gsi;
- res->end = gsi;
+ res->start = hwirq;
+ res->end = hwirq;
res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
}
-static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
+static void acpi_dev_get_irqresource(struct resource *res, u32 hwirq,
+ struct fwnode_handle *source,
u8 triggering, u8 polarity, u8 shareable,
bool legacy)
{
int irq, p, t;
- if (!valid_IRQ(gsi)) {
- acpi_dev_irqresource_disabled(res, gsi);
+ if (!source && !valid_IRQ(hwirq)) {
+ acpi_dev_irqresource_disabled(res, hwirq);
return;
}
@@ -402,25 +403,25 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
* using extended IRQ descriptors we take the IRQ configuration
* from _CRS directly.
*/
- if (legacy && !acpi_get_override_irq(gsi, &t, &p)) {
+ if (legacy && !acpi_get_override_irq(hwirq, &t, &p)) {
u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
if (triggering != trig || polarity != pol) {
- pr_warning("ACPI: IRQ %d override to %s, %s\n", gsi,
- t ? "level" : "edge", p ? "low" : "high");
+ pr_warn("ACPI: IRQ %d override to %s, %s\n", hwirq,
+ t ? "level" : "edge", p ? "low" : "high");
triggering = trig;
polarity = pol;
}
}
res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
- irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
+ irq = acpi_register_irq(source, hwirq, triggering, polarity);
if (irq >= 0) {
res->start = irq;
res->end = irq;
} else {
- acpi_dev_irqresource_disabled(res, gsi);
+ acpi_dev_irqresource_disabled(res, hwirq);
}
}
@@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
{
struct acpi_resource_irq *irq;
struct acpi_resource_extended_irq *ext_irq;
+ struct fwnode_handle *src;
switch (ares->type) {
case ACPI_RESOURCE_TYPE_IRQ:
@@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
acpi_dev_irqresource_disabled(res, 0);
return false;
}
- acpi_dev_get_irqresource(res, irq->interrupts[index],
+ acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
irq->triggering, irq->polarity,
irq->sharable, true);
break;
@@ -470,7 +472,8 @@ 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],
+ src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
+ acpi_dev_get_irqresource(res, ext_irq->interrupts[index], src,
ext_irq->triggering, ext_irq->polarity,
ext_irq->sharable, false);
break;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 325bdb9..1099b51 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -321,6 +321,25 @@ void acpi_set_irq_model(enum acpi_irq_model_id model,
*/
void acpi_unregister_gsi (u32 gsi);
+#ifdef CONFIG_ACPI_GENERIC_GSI
+struct fwnode_handle *
+acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source);
+int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger,
+ int polarity);
+void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq);
+#else
+#define acpi_get_irq_source_fwhandle(source) (NULL)
+static inline int acpi_register_irq(struct fwnode_handle *source, u32 hwirq,
+ int trigger, int polarity)
+{
+ return acpi_register_gsi(NULL, hwirq, trigger, polarity);
+}
+static inline void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq)
+{
+ acpi_unregister_gsi(hwirq);
+}
+#endif
+
struct pci_dev;
int acpi_pci_irq_enable (struct pci_dev *dev);
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On Sun, Nov 13, 2016 at 04:59:33PM -0500, Agustin Vega-Frias wrote:
> This allows probe deferral to work properly when a dependent device
> fails to get a valid IRQ because the IRQ domain was not registered
> at the time the resources were added to the platform_device.
>
> Signed-off-by: Agustin Vega-Frias <[email protected]>
> ---
> drivers/acpi/resource.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/base/platform.c | 9 +++++++-
> include/linux/acpi.h | 7 ++++++
> 3 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 56241eb..4beda15 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> return (type & types) ? 0 : 1;
> }
> EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type);
> +
> +struct acpi_irq_get_ctx {
> + unsigned int index;
> + struct resource *res;
> +};
> +
> +static acpi_status acpi_irq_get_cb(struct acpi_resource *ares, void *context)
> +{
> + struct acpi_irq_get_ctx *ctx = context;
> + struct acpi_resource_irq *irq;
> + struct acpi_resource_extended_irq *ext_irq;
> +
> + switch (ares->type) {
> + case ACPI_RESOURCE_TYPE_IRQ:
> + irq = &ares->data.irq;
> + if (ctx->index < irq->interrupt_count) {
> + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
> + return AE_CTRL_TERMINATE;
> + }
> + ctx->index -= irq->interrupt_count;
I do not understand this code, mind explaining what it is meant to do ?
In particular I do not understand the logic behind the index decrement,
I think I am missing something here.
> + break;
> + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> + ext_irq = &ares->data.extended_irq;
> + if (ctx->index < ext_irq->interrupt_count) {
> + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
> + return AE_CTRL_TERMINATE;
> + }
> + ctx->index -= ext_irq->interrupt_count;
Ditto.
Thanks,
Lorenzo
> + break;
> + }
> +
> + return AE_OK;
> +}
> +
> +/**
> + * 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)
> +{
> + struct acpi_irq_get_ctx ctx = { index, res };
> + acpi_status status;
> +
> + status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> + acpi_irq_get_cb, &ctx);
> + if (ACPI_FAILURE(status))
> + return -EINVAL;
> + if (res->flags & IORESOURCE_DISABLED)
> + return -EPROBE_DEFER;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_irq_get);
> 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 689a8b9..325bdb9 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -406,6 +406,7 @@ bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
> unsigned int acpi_dev_get_irq_type(int triggering, int polarity);
> bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> struct resource *res);
> +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res);
>
> void acpi_dev_free_resource_list(struct list_head *list);
> int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
> @@ -763,6 +764,12 @@ static inline int acpi_reconfig_notifier_unregister(struct notifier_block *nb)
> return -EINVAL;
> }
>
> +static inline int acpi_irq_get(acpi_handle handle, unsigned int index,
> + struct resource *res)
> +{
> + return -EINVAL;
> +}
> +
> #endif /* !CONFIG_ACPI */
>
> #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
> --
> 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.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lorenzo,
On 2016-11-15 10:48, Lorenzo Pieralisi wrote:
> On Sun, Nov 13, 2016 at 04:59:33PM -0500, Agustin Vega-Frias wrote:
>> This allows probe deferral to work properly when a dependent device
>> fails to get a valid IRQ because the IRQ domain was not registered
>> at the time the resources were added to the platform_device.
>>
>> Signed-off-by: Agustin Vega-Frias <[email protected]>
>> ---
>> drivers/acpi/resource.c | 59
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>> drivers/base/platform.c | 9 +++++++-
>> include/linux/acpi.h | 7 ++++++
>> 3 files changed, 74 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> index 56241eb..4beda15 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct
>> acpi_resource *ares,
>> return (type & types) ? 0 : 1;
>> }
>> EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type);
>> +
>> +struct acpi_irq_get_ctx {
>> + unsigned int index;
>> + struct resource *res;
>> +};
>> +
>> +static acpi_status acpi_irq_get_cb(struct acpi_resource *ares, void
>> *context)
>> +{
>> + struct acpi_irq_get_ctx *ctx = context;
>> + struct acpi_resource_irq *irq;
>> + struct acpi_resource_extended_irq *ext_irq;
>> +
>> + switch (ares->type) {
>> + case ACPI_RESOURCE_TYPE_IRQ:
>> + irq = &ares->data.irq;
>> + if (ctx->index < irq->interrupt_count) {
>> + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
>> + return AE_CTRL_TERMINATE;
>> + }
>> + ctx->index -= irq->interrupt_count;
>
> I do not understand this code, mind explaining what it is meant to do ?
>
> In particular I do not understand the logic behind the index decrement,
> I think I am missing something here.
>
ACPI IRQ resources can be encoded into two types of structures:
struct acpi_resource_irq,
struct acpi_resource_extended_irq.
In theory only the extended version can contain multiple IRQs, but the
Linux
ACPI core accommodates non-compliant DSDT tables that have regular IRQ
resources
contain multiple IRQs.
To better explain, suppose you have a device that handles two GSIs and
one
other IRQ form a separate device:
Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00, )
{ 130, 131 }
Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00,
"\\_SB.TCS0.QIC0", )
{ 4 }
These are encoded into two separate structures with their own interrupts
array:
res0.interrupts[] = { 130, 131 }
res1.interrupts[] = { 4 }
However, from the perspective of a client driver these are indexed into
a flat space:
[0] -> 130
[1] -> 131
[2] -> 4
Now say mapping of IRQ 4 failed during bus scan. When acpi_irq_get
retries to map
it, the client code will pass index 2. acpi_walk_resources will call
acpi_irq_get_cb
with the first IRQ resource. If the index is less than the number of
IRQs, we know
this IRQ resource contains the IRQ we want so we call
acpi_dev_resource_interrupt
to do the actual mapping and return AE_CTRL_TERMINATE so
acpi_walk_resources does
not continue walking the resource buffer. On the other hand if the index
is equal
or larger it means we need to skip this IRQ resource and look at the
next one,
but we need to adjust the lookup index to that of the next IRQ resource.
Makes sense?
>> + break;
>> + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>> + ext_irq = &ares->data.extended_irq;
>> + if (ctx->index < ext_irq->interrupt_count) {
>> + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
>> + return AE_CTRL_TERMINATE;
>> + }
>> + ctx->index -= ext_irq->interrupt_count;
>
> Ditto.
The same logic is used for both types of resources because they are
handled in
the same way by the ACPI core when it comes to indexing.
Thanks,
Agustin
>
> Thanks,
> Lorenzo
>
>> + break;
>> + }
>> +
>> + return AE_OK;
>> +}
>> +
>> +/**
>> + * 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)
>> +{
>> + struct acpi_irq_get_ctx ctx = { index, res };
>> + acpi_status status;
>> +
>> + status = acpi_walk_resources(handle, METHOD_NAME__CRS,
>> + acpi_irq_get_cb, &ctx);
>> + if (ACPI_FAILURE(status))
>> + return -EINVAL;
>> + if (res->flags & IORESOURCE_DISABLED)
>> + return -EPROBE_DEFER;
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_irq_get);
>> 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 689a8b9..325bdb9 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -406,6 +406,7 @@ bool acpi_dev_resource_ext_address_space(struct
>> acpi_resource *ares,
>> unsigned int acpi_dev_get_irq_type(int triggering, int polarity);
>> bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int
>> index,
>> struct resource *res);
>> +int acpi_irq_get(acpi_handle handle, unsigned int index, struct
>> resource *res);
>>
>> void acpi_dev_free_resource_list(struct list_head *list);
>> int acpi_dev_get_resources(struct acpi_device *adev, struct list_head
>> *list,
>> @@ -763,6 +764,12 @@ static inline int
>> acpi_reconfig_notifier_unregister(struct notifier_block *nb)
>> return -EINVAL;
>> }
>>
>> +static inline int acpi_irq_get(acpi_handle handle, unsigned int
>> index,
>> + struct resource *res)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> #endif /* !CONFIG_ACPI */
>>
>> #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
>> --
>> 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.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
>> in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
On Tue, Nov 15, 2016 at 12:43:38PM -0500, Agustin Vega-Frias wrote:
> Hi Lorenzo,
>
> On 2016-11-15 10:48, Lorenzo Pieralisi wrote:
> >On Sun, Nov 13, 2016 at 04:59:33PM -0500, Agustin Vega-Frias wrote:
> >>This allows probe deferral to work properly when a dependent device
> >>fails to get a valid IRQ because the IRQ domain was not registered
> >>at the time the resources were added to the platform_device.
> >>
> >>Signed-off-by: Agustin Vega-Frias <[email protected]>
> >>---
> >> drivers/acpi/resource.c | 59
> >>+++++++++++++++++++++++++++++++++++++++++++++++++
> >> drivers/base/platform.c | 9 +++++++-
> >> include/linux/acpi.h | 7 ++++++
> >> 3 files changed, 74 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> >>index 56241eb..4beda15 100644
> >>--- a/drivers/acpi/resource.c
> >>+++ b/drivers/acpi/resource.c
> >>@@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct
> >>acpi_resource *ares,
> >> return (type & types) ? 0 : 1;
> >> }
> >> EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type);
> >>+
> >>+struct acpi_irq_get_ctx {
> >>+ unsigned int index;
> >>+ struct resource *res;
> >>+};
> >>+
> >>+static acpi_status acpi_irq_get_cb(struct acpi_resource *ares,
> >>void *context)
> >>+{
> >>+ struct acpi_irq_get_ctx *ctx = context;
> >>+ struct acpi_resource_irq *irq;
> >>+ struct acpi_resource_extended_irq *ext_irq;
> >>+
> >>+ switch (ares->type) {
> >>+ case ACPI_RESOURCE_TYPE_IRQ:
> >>+ irq = &ares->data.irq;
> >>+ if (ctx->index < irq->interrupt_count) {
> >>+ acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
> >>+ return AE_CTRL_TERMINATE;
> >>+ }
> >>+ ctx->index -= irq->interrupt_count;
> >
> >I do not understand this code, mind explaining what it is meant to do ?
> >
> >In particular I do not understand the logic behind the index decrement,
> >I think I am missing something here.
> >
>
> ACPI IRQ resources can be encoded into two types of structures:
>
> struct acpi_resource_irq,
> struct acpi_resource_extended_irq.
>
> In theory only the extended version can contain multiple IRQs, but
> the Linux
> ACPI core accommodates non-compliant DSDT tables that have regular
> IRQ resources
> contain multiple IRQs.
>
> To better explain, suppose you have a device that handles two GSIs
> and one
> other IRQ form a separate device:
>
> Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00, )
> { 130, 131 }
>
> Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00,
> "\\_SB.TCS0.QIC0", )
> { 4 }
>
> These are encoded into two separate structures with their own
> interrupts array:
>
> res0.interrupts[] = { 130, 131 }
> res1.interrupts[] = { 4 }
>
> However, from the perspective of a client driver these are indexed
> into a flat space:
>
> [0] -> 130
> [1] -> 131
> [2] -> 4
>
> Now say mapping of IRQ 4 failed during bus scan. When acpi_irq_get
> retries to map it, the client code will pass index 2.
> acpi_walk_resources will call acpi_irq_get_cb with the first IRQ
> resource. If the index is less than the number of IRQs, we know this
> IRQ resource contains the IRQ we want so we call
> acpi_dev_resource_interrupt to do the actual mapping and return
> AE_CTRL_TERMINATE so acpi_walk_resources does not continue walking the
> resource buffer. On the other hand if the index is equal or larger it
> means we need to skip this IRQ resource and look at the next one, but
> we need to adjust the lookup index to that of the next IRQ resource.
>
> Makes sense?
Yes, basically it is to create a linear index out of multiple resources,
the DT case is simpler since you get the interrupt out of a single
property that we can easily index (ie we have to know which firmware
entry corresponds to the resource that we are retrying). That deserves
a comment.
Thanks for explaining.
Lorenzo
> >>+ break;
> >>+ case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> >>+ ext_irq = &ares->data.extended_irq;
> >>+ if (ctx->index < ext_irq->interrupt_count) {
> >>+ acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
> >>+ return AE_CTRL_TERMINATE;
> >>+ }
> >>+ ctx->index -= ext_irq->interrupt_count;
> >
> >Ditto.
>
> The same logic is used for both types of resources because they are
> handled in
> the same way by the ACPI core when it comes to indexing.
>
> Thanks,
> Agustin
>
> >
> >Thanks,
> >Lorenzo
> >
> >>+ break;
> >>+ }
> >>+
> >>+ return AE_OK;
> >>+}
> >>+
> >>+/**
> >>+ * 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)
> >>+{
> >>+ struct acpi_irq_get_ctx ctx = { index, res };
> >>+ acpi_status status;
> >>+
> >>+ status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> >>+ acpi_irq_get_cb, &ctx);
> >>+ if (ACPI_FAILURE(status))
> >>+ return -EINVAL;
> >>+ if (res->flags & IORESOURCE_DISABLED)
> >>+ return -EPROBE_DEFER;
> >>+ return 0;
> >>+}
> >>+EXPORT_SYMBOL_GPL(acpi_irq_get);
> >>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 689a8b9..325bdb9 100644
> >>--- a/include/linux/acpi.h
> >>+++ b/include/linux/acpi.h
> >>@@ -406,6 +406,7 @@ bool
> >>acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
> >> unsigned int acpi_dev_get_irq_type(int triggering, int polarity);
> >> bool acpi_dev_resource_interrupt(struct acpi_resource *ares,
> >>int index,
> >> struct resource *res);
> >>+int acpi_irq_get(acpi_handle handle, unsigned int index, struct
> >>resource *res);
> >>
> >> void acpi_dev_free_resource_list(struct list_head *list);
> >> int acpi_dev_get_resources(struct acpi_device *adev, struct
> >>list_head *list,
> >>@@ -763,6 +764,12 @@ static inline int
> >>acpi_reconfig_notifier_unregister(struct notifier_block *nb)
> >> return -EINVAL;
> >> }
> >>
> >>+static inline int acpi_irq_get(acpi_handle handle, unsigned int
> >>index,
> >>+ struct resource *res)
> >>+{
> >>+ return -EINVAL;
> >>+}
> >>+
> >> #endif /* !CONFIG_ACPI */
> >>
> >> #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
> >>--
> >>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.
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe
> >>linux-acpi" in
> >>the body of a message to [email protected]
> >>More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
> Linux Foundation Collaborative Project.
Hi Lorenzo,
On 2016-11-16 12:18, Lorenzo Pieralisi wrote:
> On Tue, Nov 15, 2016 at 12:43:38PM -0500, Agustin Vega-Frias wrote:
>> Hi Lorenzo,
>>
>> On 2016-11-15 10:48, Lorenzo Pieralisi wrote:
>> >On Sun, Nov 13, 2016 at 04:59:33PM -0500, Agustin Vega-Frias wrote:
>> >>This allows probe deferral to work properly when a dependent device
>> >>fails to get a valid IRQ because the IRQ domain was not registered
>> >>at the time the resources were added to the platform_device.
>> >>
>> >>Signed-off-by: Agustin Vega-Frias <[email protected]>
>> >>---
>> >> drivers/acpi/resource.c | 59
>> >>+++++++++++++++++++++++++++++++++++++++++++++++++
>> >> drivers/base/platform.c | 9 +++++++-
>> >> include/linux/acpi.h | 7 ++++++
>> >> 3 files changed, 74 insertions(+), 1 deletion(-)
>> >>
>> >>diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> >>index 56241eb..4beda15 100644
>> >>--- a/drivers/acpi/resource.c
>> >>+++ b/drivers/acpi/resource.c
>> >>@@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct
>> >>acpi_resource *ares,
>> >> return (type & types) ? 0 : 1;
>> >> }
>> >> EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type);
>> >>+
>> >>+struct acpi_irq_get_ctx {
>> >>+ unsigned int index;
>> >>+ struct resource *res;
>> >>+};
>> >>+
>> >>+static acpi_status acpi_irq_get_cb(struct acpi_resource *ares,
>> >>void *context)
>> >>+{
>> >>+ struct acpi_irq_get_ctx *ctx = context;
>> >>+ struct acpi_resource_irq *irq;
>> >>+ struct acpi_resource_extended_irq *ext_irq;
>> >>+
>> >>+ switch (ares->type) {
>> >>+ case ACPI_RESOURCE_TYPE_IRQ:
>> >>+ irq = &ares->data.irq;
>> >>+ if (ctx->index < irq->interrupt_count) {
>> >>+ acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
>> >>+ return AE_CTRL_TERMINATE;
>> >>+ }
>> >>+ ctx->index -= irq->interrupt_count;
>> >
>> >I do not understand this code, mind explaining what it is meant to do ?
>> >
>> >In particular I do not understand the logic behind the index decrement,
>> >I think I am missing something here.
>> >
>>
>> ACPI IRQ resources can be encoded into two types of structures:
>>
>> struct acpi_resource_irq,
>> struct acpi_resource_extended_irq.
>>
>> In theory only the extended version can contain multiple IRQs, but
>> the Linux
>> ACPI core accommodates non-compliant DSDT tables that have regular
>> IRQ resources
>> contain multiple IRQs.
>>
>> To better explain, suppose you have a device that handles two GSIs
>> and one
>> other IRQ form a separate device:
>>
>> Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00, )
>> { 130, 131 }
>>
>> Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00,
>> "\\_SB.TCS0.QIC0", )
>> { 4 }
>>
>> These are encoded into two separate structures with their own
>> interrupts array:
>>
>> res0.interrupts[] = { 130, 131 }
>> res1.interrupts[] = { 4 }
>>
>> However, from the perspective of a client driver these are indexed
>> into a flat space:
>>
>> [0] -> 130
>> [1] -> 131
>> [2] -> 4
>>
>> Now say mapping of IRQ 4 failed during bus scan. When acpi_irq_get
>> retries to map it, the client code will pass index 2.
>> acpi_walk_resources will call acpi_irq_get_cb with the first IRQ
>> resource. If the index is less than the number of IRQs, we know this
>> IRQ resource contains the IRQ we want so we call
>> acpi_dev_resource_interrupt to do the actual mapping and return
>> AE_CTRL_TERMINATE so acpi_walk_resources does not continue walking the
>> resource buffer. On the other hand if the index is equal or larger it
>> means we need to skip this IRQ resource and look at the next one, but
>> we need to adjust the lookup index to that of the next IRQ resource.
>>
>> Makes sense?
>
> Yes, basically it is to create a linear index out of multiple
> resources,
> the DT case is simpler since you get the interrupt out of a single
> property that we can easily index (ie we have to know which firmware
> entry corresponds to the resource that we are retrying). That deserves
> a comment.
>
Good point. I will add the comment on the next version of the series,
however, I will hold a few days for more feedback before I submit that.
Thanks,
Agustin
> Thanks for explaining.
>
> Lorenzo
>
>> >>+ break;
>> >>+ case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>> >>+ ext_irq = &ares->data.extended_irq;
>> >>+ if (ctx->index < ext_irq->interrupt_count) {
>> >>+ acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
>> >>+ return AE_CTRL_TERMINATE;
>> >>+ }
>> >>+ ctx->index -= ext_irq->interrupt_count;
>> >
>> >Ditto.
>>
>> The same logic is used for both types of resources because they are
>> handled in
>> the same way by the ACPI core when it comes to indexing.
>>
>> Thanks,
>> Agustin
>>
>> >
>> >Thanks,
>> >Lorenzo
>> >
>> >>+ break;
>> >>+ }
>> >>+
>> >>+ return AE_OK;
>> >>+}
>> >>+
>> >>+/**
>> >>+ * 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)
>> >>+{
>> >>+ struct acpi_irq_get_ctx ctx = { index, res };
>> >>+ acpi_status status;
>> >>+
>> >>+ status = acpi_walk_resources(handle, METHOD_NAME__CRS,
>> >>+ acpi_irq_get_cb, &ctx);
>> >>+ if (ACPI_FAILURE(status))
>> >>+ return -EINVAL;
>> >>+ if (res->flags & IORESOURCE_DISABLED)
>> >>+ return -EPROBE_DEFER;
>> >>+ return 0;
>> >>+}
>> >>+EXPORT_SYMBOL_GPL(acpi_irq_get);
>> >>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 689a8b9..325bdb9 100644
>> >>--- a/include/linux/acpi.h
>> >>+++ b/include/linux/acpi.h
>> >>@@ -406,6 +406,7 @@ bool
>> >>acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
>> >> unsigned int acpi_dev_get_irq_type(int triggering, int polarity);
>> >> bool acpi_dev_resource_interrupt(struct acpi_resource *ares,
>> >>int index,
>> >> struct resource *res);
>> >>+int acpi_irq_get(acpi_handle handle, unsigned int index, struct
>> >>resource *res);
>> >>
>> >> void acpi_dev_free_resource_list(struct list_head *list);
>> >> int acpi_dev_get_resources(struct acpi_device *adev, struct
>> >>list_head *list,
>> >>@@ -763,6 +764,12 @@ static inline int
>> >>acpi_reconfig_notifier_unregister(struct notifier_block *nb)
>> >> return -EINVAL;
>> >> }
>> >>
>> >>+static inline int acpi_irq_get(acpi_handle handle, unsigned int
>> >>index,
>> >>+ struct resource *res)
>> >>+{
>> >>+ return -EINVAL;
>> >>+}
>> >>+
>> >> #endif /* !CONFIG_ACPI */
>> >>
>> >> #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
>> >>--
>> >>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.
>> >>
>> >>--
>> >>To unsubscribe from this list: send the line "unsubscribe
>> >>linux-acpi" in
>> >>the body of a message to [email protected]
>> >>More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> --
>> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
>> Technologies, Inc.
>> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
>> Linux Foundation Collaborative Project.
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
Hi Agustin,
On Sun, Nov 13, 2016 at 04:59:34PM -0500, Agustin Vega-Frias wrote:
> When an Extended IRQ Resource contains a valid ResourceSource
> use it to map the IRQ on the domain associated with the ACPI
> device referenced.
>
> With this in place an irqchip driver can create its domain using
> irq_domain_create_linear and pass the device fwnode to create
> the domain mapping. When dependent devices are probed these
> changes allow the ACPI core find the domain and map the IRQ.
>
> Signed-off-by: Agustin Vega-Frias <[email protected]>
> ---
> drivers/acpi/Makefile | 2 +-
> drivers/acpi/{gsi.c => irq.c} | 98 +++++++++++++++++++++++++++++++++++++------
> drivers/acpi/resource.c | 29 +++++++------
> include/linux/acpi.h | 19 +++++++++
> 4 files changed, 121 insertions(+), 27 deletions(-)
> rename drivers/acpi/{gsi.c => irq.c} (53%)
It looks to me the direction is the right one but I have a question
for you and others below.
> 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 53%
> rename from drivers/acpi/gsi.c
> rename to drivers/acpi/irq.c
> index ee9e0f2..c6ecaab 100644
> --- a/drivers/acpi/gsi.c
> +++ b/drivers/acpi/irq.c
> @@ -18,6 +18,45 @@
> static struct fwnode_handle *acpi_gsi_domain_id;
>
> /**
> + * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
> + * acpi_resource_source which is used
> + * to be 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
> + */
> +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;
> +}
> +EXPORT_SYMBOL_GPL(acpi_get_irq_source_fwhandle);
> +
> +/**
> * 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
> @@ -42,6 +81,50 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
>
> /**
> + * acpi_register_irq() - Map a hardware to a linux IRQ number
> + * @source: IRQ source
> + * @hwirq: Hardware IRQ number
> + * @trigger: trigger type of the IRQ number to be mapped
> + * @polarity: polarity of the IRQ to be mapped
> + *
> + * Returns: a valid linux IRQ number on success
> + * -EINVAL on failure
Nit: You need to update the return values list.
> + */
> +int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger,
> + int polarity)
> +{
> + struct irq_fwspec fwspec;
> +
> + if (!source)
> + return -EINVAL;
> +
> + if (irq_find_matching_fwnode(source, DOMAIN_BUS_ANY) == NULL)
> + return -EPROBE_DEFER;
> +
> + fwspec.fwnode = source;
> + fwspec.param[0] = hwirq;
> + 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_irq);
> +
> +/**
> + * acpi_unregister_irq() - Free a Hardware IRQ<->linux IRQ number mapping
> + * @hwirq: Hardware IRQ number
> + */
> +void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq)
> +{
> + struct irq_domain *d = irq_find_matching_fwnode(source,
> + DOMAIN_BUS_ANY);
> + int irq = irq_find_mapping(d, hwirq);
> +
> + irq_dispose_mapping(irq);
> +}
> +EXPORT_SYMBOL_GPL(acpi_unregister_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
> @@ -54,19 +137,12 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> 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);
> + return acpi_register_irq(acpi_gsi_domain_id, gsi, trigger, polarity);
> }
> EXPORT_SYMBOL_GPL(acpi_register_gsi);
>
> @@ -76,11 +152,7 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
> */
> 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);
> + acpi_unregister_irq(acpi_gsi_domain_id, gsi);
> }
> EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 4beda15..83cff00 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -374,21 +374,22 @@ unsigned int acpi_dev_get_irq_type(int triggering, int polarity)
> }
> EXPORT_SYMBOL_GPL(acpi_dev_get_irq_type);
>
> -static void acpi_dev_irqresource_disabled(struct resource *res, u32 gsi)
> +static void acpi_dev_irqresource_disabled(struct resource *res, u32 hwirq)
> {
> - res->start = gsi;
> - res->end = gsi;
> + res->start = hwirq;
> + res->end = hwirq;
> res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
> }
>
> -static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
> +static void acpi_dev_get_irqresource(struct resource *res, u32 hwirq,
> + struct fwnode_handle *source,
> u8 triggering, u8 polarity, u8 shareable,
> bool legacy)
> {
> int irq, p, t;
>
> - if (!valid_IRQ(gsi)) {
> - acpi_dev_irqresource_disabled(res, gsi);
> + if (!source && !valid_IRQ(hwirq)) {
> + acpi_dev_irqresource_disabled(res, hwirq);
> return;
> }
>
> @@ -402,25 +403,25 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
> * using extended IRQ descriptors we take the IRQ configuration
> * from _CRS directly.
> */
> - if (legacy && !acpi_get_override_irq(gsi, &t, &p)) {
> + if (legacy && !acpi_get_override_irq(hwirq, &t, &p)) {
> u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
> u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
>
> if (triggering != trig || polarity != pol) {
> - pr_warning("ACPI: IRQ %d override to %s, %s\n", gsi,
> - t ? "level" : "edge", p ? "low" : "high");
> + pr_warn("ACPI: IRQ %d override to %s, %s\n", hwirq,
> + t ? "level" : "edge", p ? "low" : "high");
> triggering = trig;
> polarity = pol;
> }
> }
>
> res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
> - irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
> + irq = acpi_register_irq(source, hwirq, triggering, polarity);
> if (irq >= 0) {
> res->start = irq;
> res->end = irq;
> } else {
> - acpi_dev_irqresource_disabled(res, gsi);
> + acpi_dev_irqresource_disabled(res, hwirq);
> }
> }
>
> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> {
> struct acpi_resource_irq *irq;
> struct acpi_resource_extended_irq *ext_irq;
> + struct fwnode_handle *src;
>
> switch (ares->type) {
> case ACPI_RESOURCE_TYPE_IRQ:
> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> acpi_dev_irqresource_disabled(res, 0);
> return false;
> }
> - acpi_dev_get_irqresource(res, irq->interrupts[index],
> + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
> irq->triggering, irq->polarity,
> irq->sharable, true);
> break;
> @@ -470,7 +472,8 @@ 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],
> + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
Is there a reason why we need to do the domain look-up here ?
I would like to understand if, by reshuffling the code (and by returning
the resource_source to the calling code - somehow), it would be possible
to just mirror what the OF code does in of_irq_get(), namely:
(1) parse the irq entry -> of_irq_parse_one()
(2) look the domain up -> irq_find_host()
(3) create the mapping -> irq_create_of_mapping()
You wrote the code already, I think it is just a matter of shuffling
it around (well, minus returning the resource_source to the caller
which is phandle equivalent in DT).
You abstracted away (2) and (3) behind acpi_register_irq(), that
on anything than does not use ACPI_GENERIC_GSI is just glue code
to acpi_register_gsi().
Also, it is not a question on this patch but I ask it here because it
is related. On ACPI you are doing the reverse of what is done in
DT in platform_get_irq():
- get the resources already parsed -> platform_get_resource()
- if they are disabled -> acpi_irq_get()
and I think the ordering is tied to my question above because
you carry out the domain look up in acpi_dev_resource_interrupt()
so that if for any reason it fails the corresponding resource
is disabled so that we try to get it again through acpi_irq_get().
I suspect you did it this way to make sure:
a) keep the current ACPI IRQ parsing interface changes to a mininum
b) avoid changing the behaviour on x86/ia64; in particular, calling
acpi_register_gsi() for the _same_ mapping (an IRQ that was already
registered at device creation resource parsing) multiple times can
trigger issues on x86/ia64
I think that's a reasonable approach but I wanted to get these
clarifications, I do not think you are far from getting this
done but since it is a significant change I think it is worth
discussing the points I raised above because I think the DT code
sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ
layer perspective (instead of having the domain look-up buried
inside the ACPI IRQ resource parsing API).
Thanks !
Lorenzo
> + acpi_dev_get_irqresource(res, ext_irq->interrupts[index], src,
> ext_irq->triggering, ext_irq->polarity,
> ext_irq->sharable, false);
> break;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 325bdb9..1099b51 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -321,6 +321,25 @@ void acpi_set_irq_model(enum acpi_irq_model_id model,
> */
> void acpi_unregister_gsi (u32 gsi);
>
> +#ifdef CONFIG_ACPI_GENERIC_GSI
> +struct fwnode_handle *
> +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source);
> +int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger,
> + int polarity);
> +void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq);
> +#else
> +#define acpi_get_irq_source_fwhandle(source) (NULL)
> +static inline int acpi_register_irq(struct fwnode_handle *source, u32 hwirq,
> + int trigger, int polarity)
> +{
> + return acpi_register_gsi(NULL, hwirq, trigger, polarity);
> +}
> +static inline void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq)
> +{
> + acpi_unregister_gsi(hwirq);
> +}
> +#endif
> +
> struct pci_dev;
>
> int acpi_pci_irq_enable (struct pci_dev *dev);
> --
> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
>
Hi Agustin,
On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote:
[...]
> > @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> > {
> > struct acpi_resource_irq *irq;
> > struct acpi_resource_extended_irq *ext_irq;
> > + struct fwnode_handle *src;
> >
> > switch (ares->type) {
> > case ACPI_RESOURCE_TYPE_IRQ:
> > @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> > acpi_dev_irqresource_disabled(res, 0);
> > return false;
> > }
> > - acpi_dev_get_irqresource(res, irq->interrupts[index],
> > + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
> > irq->triggering, irq->polarity,
> > irq->sharable, true);
> > break;
> > @@ -470,7 +472,8 @@ 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],
> > + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
>
> Is there a reason why we need to do the domain look-up here ?
>
> I would like to understand if, by reshuffling the code (and by returning
> the resource_source to the calling code - somehow), it would be possible
> to just mirror what the OF code does in of_irq_get(), namely:
>
> (1) parse the irq entry -> of_irq_parse_one()
> (2) look the domain up -> irq_find_host()
> (3) create the mapping -> irq_create_of_mapping()
>
> You wrote the code already, I think it is just a matter of shuffling
> it around (well, minus returning the resource_source to the caller
> which is phandle equivalent in DT).
>
> You abstracted away (2) and (3) behind acpi_register_irq(), that
> on anything than does not use ACPI_GENERIC_GSI is just glue code
> to acpi_register_gsi().
>
> Also, it is not a question on this patch but I ask it here because it
> is related. On ACPI you are doing the reverse of what is done in
> DT in platform_get_irq():
>
> - get the resources already parsed -> platform_get_resource()
> - if they are disabled -> acpi_irq_get()
>
> and I think the ordering is tied to my question above because
> you carry out the domain look up in acpi_dev_resource_interrupt()
> so that if for any reason it fails the corresponding resource
> is disabled so that we try to get it again through acpi_irq_get().
>
> I suspect you did it this way to make sure:
>
> a) keep the current ACPI IRQ parsing interface changes to a mininum
> b) avoid changing the behaviour on x86/ia64; in particular, calling
> acpi_register_gsi() for the _same_ mapping (an IRQ that was already
> registered at device creation resource parsing) multiple times can
> trigger issues on x86/ia64
>
> I think that's a reasonable approach but I wanted to get these
> clarifications, I do not think you are far from getting this
> done but since it is a significant change I think it is worth
> discussing the points I raised above because I think the DT code
> sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ
> layer perspective (instead of having the domain look-up buried
> inside the ACPI IRQ resource parsing API).
I had another look and to achieve the above one way of doing that is to
implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for
!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI
we would fall back to current solution for ACPI). Within acpi_irq_get()
you can easily carry out the same steps (1->2->3) above in ACPI you have
the code already there I think it is easy to change the
acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and
the interface would become identical to of_irq_get() that is an
advantage to maintain it from an IRQ maintainership perspective I think,
that's my opinion.
There is still a nagging snag though. When platform devices are
created, core ACPI code parse the resources through:
acpi_dev_get_resources()
and we _have_ to have way to avoid initializing IRQ resources that
have a dependency (ie there is a resource_source pointer that is valid
in their descriptors) that's easy to do if we think that's the right
thing to do and can hardly break current code (which ignores the
resource_source altogether).
It is an important difference with DT probing, where the IRQ
resources are only created if the domain reference (ie interrupt
controller phandle) is satisfied at of_device_alloc() time
(see of_device_alloc()).
Thoughts ? Please let me know, the code to implement what I say
is already in these patches, it is just a matter of reshuffling it.
Thanks !
Lorenzo
This patch works fine on my D05 board.
Tested-by: Majun <[email protected]>
?? 2016/11/14 5:59, Agustin Vega-Frias д??:
> This allows probe deferral to work properly when a dependent device
> fails to get a valid IRQ because the IRQ domain was not registered
> at the time the resources were added to the platform_device.
>
> Signed-off-by: Agustin Vega-Frias <[email protected]>
> ---
> drivers/acpi/resource.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/base/platform.c | 9 +++++++-
> include/linux/acpi.h | 7 ++++++
> 3 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 56241eb..4beda15 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
> return (type & types) ? 0 : 1;
> }
> EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type);
> +
> +struct acpi_irq_get_ctx {
> + unsigned int index;
> + struct resource *res;
> +};
> +
> +static acpi_status acpi_irq_get_cb(struct acpi_resource *ares, void *context)
> +{
> + struct acpi_irq_get_ctx *ctx = context;
> + struct acpi_resource_irq *irq;
> + struct acpi_resource_extended_irq *ext_irq;
> +
> + switch (ares->type) {
> + case ACPI_RESOURCE_TYPE_IRQ:
> + irq = &ares->data.irq;
> + if (ctx->index < irq->interrupt_count) {
> + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
> + return AE_CTRL_TERMINATE;
> + }
> + ctx->index -= irq->interrupt_count;
> + break;
> + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> + ext_irq = &ares->data.extended_irq;
> + if (ctx->index < ext_irq->interrupt_count) {
> + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res);
> + return AE_CTRL_TERMINATE;
> + }
> + ctx->index -= ext_irq->interrupt_count;
> + break;
> + }
> +
> + return AE_OK;
> +}
> +
> +/**
> + * 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)
> +{
> + struct acpi_irq_get_ctx ctx = { index, res };
> + acpi_status status;
> +
> + status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> + acpi_irq_get_cb, &ctx);
> + if (ACPI_FAILURE(status))
> + return -EINVAL;
> + if (res->flags & IORESOURCE_DISABLED)
> + return -EPROBE_DEFER;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_irq_get);
> 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 689a8b9..325bdb9 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -406,6 +406,7 @@ bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
> unsigned int acpi_dev_get_irq_type(int triggering, int polarity);
> bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> struct resource *res);
> +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res);
>
> void acpi_dev_free_resource_list(struct list_head *list);
> int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
> @@ -763,6 +764,12 @@ static inline int acpi_reconfig_notifier_unregister(struct notifier_block *nb)
> return -EINVAL;
> }
>
> +static inline int acpi_irq_get(acpi_handle handle, unsigned int index,
> + struct resource *res)
> +{
> + return -EINVAL;
> +}
> +
> #endif /* !CONFIG_ACPI */
>
> #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
>
Hi Rafael,
Can you chime in on Lorenzo's feedback and the discussion below?
It would be great if you can comment on the reason ACPI does things
in a certain way.
Hi Lorenzo,
On 2016-11-25 06:40, Lorenzo Pieralisi wrote:
> Hi Agustin,
>
> On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote:
>
> [...]
>
>> > @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>> > {
>> > struct acpi_resource_irq *irq;
>> > struct acpi_resource_extended_irq *ext_irq;
>> > + struct fwnode_handle *src;
>> >
>> > switch (ares->type) {
>> > case ACPI_RESOURCE_TYPE_IRQ:
>> > @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>> > acpi_dev_irqresource_disabled(res, 0);
>> > return false;
>> > }
>> > - acpi_dev_get_irqresource(res, irq->interrupts[index],
>> > + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
>> > irq->triggering, irq->polarity,
>> > irq->sharable, true);
>> > break;
>> > @@ -470,7 +472,8 @@ 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],
>> > + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
>>
>> Is there a reason why we need to do the domain look-up here ?
Because we need to pass the resource down to acpi_dev_get_irqresource
which does the mapping through acpi_register_irq/acpi_register_gsi.
>>
>> I would like to understand if, by reshuffling the code (and by
>> returning
>> the resource_source to the calling code - somehow), it would be
>> possible
>> to just mirror what the OF code does in of_irq_get(), namely:
>>
>> (1) parse the irq entry -> of_irq_parse_one()
>> (2) look the domain up -> irq_find_host()
>> (3) create the mapping -> irq_create_of_mapping()
>>
>> You wrote the code already, I think it is just a matter of shuffling
>> it around (well, minus returning the resource_source to the caller
>> which is phandle equivalent in DT).
This is one area in which DT and ACPI are fundamentally different. In DT
once the flattened blob is expanded the data is fixed. In ACPI the data
returned by a method can change. In reality most methods like CRS return
constants, but given that per-spec they are methods the interpreter has
to be involved, which makes it an expensive operation. I believe that is
the reason the resource parsing code in ACPI attempts all mappings
during
the bus scan. Rafael can you comment on this?
One way to do what you suggest would be to defer IRQ mapping by, e.g.,
populating res->start with the HW IRQ number and res->end with the
fwnode.
That way we can avoid having to walk the resource buffer when a mapping
is needed. I don't think that approach would deviate much more from
the spec from what the current ahead-of-time mapping does, but it would
require more changes in the core code. An alternative would be to do
that only for resources that fail to map.
>>
>> You abstracted away (2) and (3) behind acpi_register_irq(), that
>> on anything than does not use ACPI_GENERIC_GSI is just glue code
>> to acpi_register_gsi().
>>
>> Also, it is not a question on this patch but I ask it here because it
>> is related. On ACPI you are doing the reverse of what is done in
>> DT in platform_get_irq():
>>
>> - get the resources already parsed -> platform_get_resource()
>> - if they are disabled -> acpi_irq_get()
>>
>> and I think the ordering is tied to my question above because
>> you carry out the domain look up in acpi_dev_resource_interrupt()
>> so that if for any reason it fails the corresponding resource
>> is disabled so that we try to get it again through acpi_irq_get().
>>
>> I suspect you did it this way to make sure:
>>
>> a) keep the current ACPI IRQ parsing interface changes to a mininum
>> b) avoid changing the behaviour on x86/ia64; in particular, calling
>> acpi_register_gsi() for the _same_ mapping (an IRQ that was already
>> registered at device creation resource parsing) multiple times can
>> trigger issues on x86/ia64
You are correct about my reasons. I wanted to keep ACPI core code
changes
to a minimum, and I also needed to work within the current
implementation
which uses the pre-converted IRQ resources.
>>
>> I think that's a reasonable approach but I wanted to get these
>> clarifications, I do not think you are far from getting this
>> done but since it is a significant change I think it is worth
>> discussing the points I raised above because I think the DT code
>> sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ
>> layer perspective (instead of having the domain look-up buried
>> inside the ACPI IRQ resource parsing API).
>
> I had another look and to achieve the above one way of doing that is to
> implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for
> !ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI
> we would fall back to current solution for ACPI). Within acpi_irq_get()
> you can easily carry out the same steps (1->2->3) above in ACPI you
> have
> the code already there I think it is easy to change the
> acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and
> the interface would become identical to of_irq_get() that is an
> advantage to maintain it from an IRQ maintainership perspective I
> think,
> that's my opinion.
I think I get what you mean. I'll take a stab at implementing
acpi_irq_get()
in the way you suggest.
>
> There is still a nagging snag though. When platform devices are
> created, core ACPI code parse the resources through:
>
> acpi_dev_get_resources()
>
> and we _have_ to have way to avoid initializing IRQ resources that
> have a dependency (ie there is a resource_source pointer that is valid
> in their descriptors) that's easy to do if we think that's the right
> thing to do and can hardly break current code (which ignores the
> resource_source altogether).
I'd rather keep the core code as-is with regard to the ahead-of-time
conversion. Whether a resource source is available at the time of the
bus
scan should be transparent to the code in drivers/acpi/resource.c, and
we need the initialization as a disabled resource to signal the need
to retry anyway.
Rafael, do you have any other suggestions/feedback on how to go about
doing this?
Thanks,
Agustin
>
> It is an important difference with DT probing, where the IRQ
> resources are only created if the domain reference (ie interrupt
> controller phandle) is satisfied at of_device_alloc() time
> (see of_device_alloc()).
>
> Thoughts ? Please let me know, the code to implement what I say
> is already in these patches, it is just a matter of reshuffling it.
>
> Thanks !
> Lorenzo
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
Hi Agustin,
On 2016/11/14 5:59, Agustin Vega-Frias wrote:
> Add support for IRQ combiners in the Top-level Control and Status
> Registers (TCSR) hardware block in Qualcomm Technologies chips.
>
> The first patch fixes IRQ probe deferral by allowing platform_device
> IRQ resources to be re-initialized if the ACPI core failed to find
> the IRQ domain during ACPI bus scan.
>
> The second patch adds support for ResourceSource/IRQ domain mapping
> when using Extended IRQ Resources with a specific ResourceSource.
> These changes are conditional on the ACPI_GENERIC_GSI config.
>
> The third patch takes advantage of the new capabilities to implement
> the driver for the IRQ combiners.
>
> Tested on top of v4.9-rc4.
>
> Changes V6 -> V7:
> * Consolidate changes for ResourceSource/IRQ domain mapping to the same
> source file implementing the generic GSI support, making it conditional
> on CONFIG_ACPI_GENERIC_GSI.
> * Eliminate some code duplication by implementing acpi_register_gsi in
> terms of the new acpi_register_irq API.
I had a test on Hisilicon D03 which needs patch 1 and 2 in this patch
set to enable the mbigen irqchip, and it works pretty good.
Patch 1/3 can remove the device dependency for the irqchip and platform
devices, and I removed my patch (ACPI: irq: introduce interrupt
producer) then add your patch 2/3, devices such as SAS and native
network works fine on my D03.
I will comment on the patches later.
Thanks
Hanjun
Hi Agustin,
On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote:
> Hi Rafael,
>
> Can you chime in on Lorenzo's feedback and the discussion below?
> It would be great if you can comment on the reason ACPI does things
> in a certain way.
>
> Hi Lorenzo,
>
> On 2016-11-25 06:40, Lorenzo Pieralisi wrote:
> >Hi Agustin,
> >
> >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote:
> >
> >[...]
> >
> >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> >>> {
> >>> struct acpi_resource_irq *irq;
> >>> struct acpi_resource_extended_irq *ext_irq;
> >>> + struct fwnode_handle *src;
> >>>
> >>> switch (ares->type) {
> >>> case ACPI_RESOURCE_TYPE_IRQ:
> >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> >>> acpi_dev_irqresource_disabled(res, 0);
> >>> return false;
> >>> }
> >>> - acpi_dev_get_irqresource(res, irq->interrupts[index],
> >>> + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
> >>> irq->triggering, irq->polarity,
> >>> irq->sharable, true);
> >>> break;
> >>> @@ -470,7 +472,8 @@ 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],
> >>> + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
> >>
> >>Is there a reason why we need to do the domain look-up here ?
>
> Because we need to pass the resource down to acpi_dev_get_irqresource
> which does the mapping through acpi_register_irq/acpi_register_gsi.
>
> >>
> >>I would like to understand if, by reshuffling the code (and by
> >>returning
> >>the resource_source to the calling code - somehow), it would be
> >>possible
> >>to just mirror what the OF code does in of_irq_get(), namely:
> >>
> >>(1) parse the irq entry -> of_irq_parse_one()
> >>(2) look the domain up -> irq_find_host()
> >>(3) create the mapping -> irq_create_of_mapping()
> >>
> >>You wrote the code already, I think it is just a matter of shuffling
> >>it around (well, minus returning the resource_source to the caller
> >>which is phandle equivalent in DT).
>
> This is one area in which DT and ACPI are fundamentally different. In DT
> once the flattened blob is expanded the data is fixed. In ACPI the data
> returned by a method can change. In reality most methods like CRS return
> constants, but given that per-spec they are methods the interpreter has
> to be involved, which makes it an expensive operation. I believe that is
> the reason the resource parsing code in ACPI attempts all mappings
> during
> the bus scan. Rafael can you comment on this?
>
> One way to do what you suggest would be to defer IRQ mapping by, e.g.,
> populating res->start with the HW IRQ number and res->end with the
> fwnode.
> That way we can avoid having to walk the resource buffer when a mapping
> is needed. I don't think that approach would deviate much more from
> the spec from what the current ahead-of-time mapping does, but it would
> require more changes in the core code. An alternative would be to do
> that only for resources that fail to map.
>
> >>
> >>You abstracted away (2) and (3) behind acpi_register_irq(), that
> >>on anything than does not use ACPI_GENERIC_GSI is just glue code
> >>to acpi_register_gsi().
> >>
> >>Also, it is not a question on this patch but I ask it here because it
> >>is related. On ACPI you are doing the reverse of what is done in
> >>DT in platform_get_irq():
> >>
> >>- get the resources already parsed -> platform_get_resource()
> >>- if they are disabled -> acpi_irq_get()
> >>
> >>and I think the ordering is tied to my question above because
> >>you carry out the domain look up in acpi_dev_resource_interrupt()
> >>so that if for any reason it fails the corresponding resource
> >>is disabled so that we try to get it again through acpi_irq_get().
> >>
> >>I suspect you did it this way to make sure:
> >>
> >>a) keep the current ACPI IRQ parsing interface changes to a mininum
> >>b) avoid changing the behaviour on x86/ia64; in particular, calling
> >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already
> >> registered at device creation resource parsing) multiple times can
> >> trigger issues on x86/ia64
>
> You are correct about my reasons. I wanted to keep ACPI core code
> changes
> to a minimum, and I also needed to work within the current
> implementation
> which uses the pre-converted IRQ resources.
>
> >>
> >>I think that's a reasonable approach but I wanted to get these
> >>clarifications, I do not think you are far from getting this
> >>done but since it is a significant change I think it is worth
> >>discussing the points I raised above because I think the DT code
> >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ
> >>layer perspective (instead of having the domain look-up buried
> >>inside the ACPI IRQ resource parsing API).
> >
> >I had another look and to achieve the above one way of doing that is to
> >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for
> >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI
> >we would fall back to current solution for ACPI). Within acpi_irq_get()
> >you can easily carry out the same steps (1->2->3) above in ACPI
> >you have
> >the code already there I think it is easy to change the
> >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and
> >the interface would become identical to of_irq_get() that is an
> >advantage to maintain it from an IRQ maintainership perspective I
> >think,
> >that's my opinion.
>
> I think I get what you mean. I'll take a stab at implementing
> acpi_irq_get()
> in the way you suggest.
>
> >
> >There is still a nagging snag though. When platform devices are
> >created, core ACPI code parse the resources through:
> >
> >acpi_dev_get_resources()
> >
> >and we _have_ to have way to avoid initializing IRQ resources that
> >have a dependency (ie there is a resource_source pointer that is valid
> >in their descriptors) that's easy to do if we think that's the right
> >thing to do and can hardly break current code (which ignores the
> >resource_source altogether).
>
> I'd rather keep the core code as-is with regard to the ahead-of-time
> conversion. Whether a resource source is available at the time of
> the bus
> scan should be transparent to the code in drivers/acpi/resource.c, and
> we need the initialization as a disabled resource to signal the need
> to retry anyway.
Yes, exactly that's the nub. Your current code works, I am trying to
make it more modular and similar to the DT/irqdomain IRQ look-up path,
which has its advantages.
There are two options IMHO:
- always disable the resource if it has a resource_source dependency and defer
its parsing to acpi_irq_get() (where you can easily implement steps 1-2-3 above).
What I wanted to say is that, by disabling the resource if it has a
resource_source dependency you can't break x86/ia64 (it is ignored at
present - hopefully there is nothing that we are not aware of behind
that choice). On x86/ia64 acpi_irq_get() would be an empty stub.
This way you would keep the irqdomain look-up out of the ACPI resource
parsing API, correct ?
- keep code as-is
Your point on _CRS being _current_ resource setting is perfectly valid
so platform_get_resource() in platform_get_irq() must always take
precedence over acpi_irq_get() (which should just apply to disabled
resources), I am not sure that doing it the other way around is safe.
> Rafael, do you have any other suggestions/feedback on how to go about
> doing this?
Yes, comments very appreciated, these changes are not trivial and need
agreement.
Thanks,
Lorenzo
>
> Thanks,
> Agustin
>
> >
> >It is an important difference with DT probing, where the IRQ
> >resources are only created if the domain reference (ie interrupt
> >controller phandle) is satisfied at of_device_alloc() time
> >(see of_device_alloc()).
> >
> >Thoughts ? Please let me know, the code to implement what I say
> >is already in these patches, it is just a matter of reshuffling it.
> >
> >Thanks !
> >Lorenzo
>
> --
> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
> Linux Foundation Collaborative Project.
On Tue, Nov 29, 2016 at 1:11 PM, Lorenzo Pieralisi
<[email protected]> wrote:
> Hi Agustin,
>
> On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote:
>> Hi Rafael,
>>
>> Can you chime in on Lorenzo's feedback and the discussion below?
>> It would be great if you can comment on the reason ACPI does things
>> in a certain way.
>>
>> Hi Lorenzo,
>>
>> On 2016-11-25 06:40, Lorenzo Pieralisi wrote:
>> >Hi Agustin,
>> >
>> >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote:
>> >
>> >[...]
>> >
>> >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>> >>> {
>> >>> struct acpi_resource_irq *irq;
>> >>> struct acpi_resource_extended_irq *ext_irq;
>> >>> + struct fwnode_handle *src;
>> >>>
>> >>> switch (ares->type) {
>> >>> case ACPI_RESOURCE_TYPE_IRQ:
>> >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>> >>> acpi_dev_irqresource_disabled(res, 0);
>> >>> return false;
>> >>> }
>> >>> - acpi_dev_get_irqresource(res, irq->interrupts[index],
>> >>> + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
>> >>> irq->triggering, irq->polarity,
>> >>> irq->sharable, true);
>> >>> break;
>> >>> @@ -470,7 +472,8 @@ 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],
>> >>> + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
>> >>
>> >>Is there a reason why we need to do the domain look-up here ?
>>
>> Because we need to pass the resource down to acpi_dev_get_irqresource
>> which does the mapping through acpi_register_irq/acpi_register_gsi.
>>
>> >>
>> >>I would like to understand if, by reshuffling the code (and by
>> >>returning
>> >>the resource_source to the calling code - somehow), it would be
>> >>possible
>> >>to just mirror what the OF code does in of_irq_get(), namely:
>> >>
>> >>(1) parse the irq entry -> of_irq_parse_one()
>> >>(2) look the domain up -> irq_find_host()
>> >>(3) create the mapping -> irq_create_of_mapping()
>> >>
>> >>You wrote the code already, I think it is just a matter of shuffling
>> >>it around (well, minus returning the resource_source to the caller
>> >>which is phandle equivalent in DT).
>>
>> This is one area in which DT and ACPI are fundamentally different. In DT
>> once the flattened blob is expanded the data is fixed. In ACPI the data
>> returned by a method can change. In reality most methods like CRS return
>> constants, but given that per-spec they are methods the interpreter has
>> to be involved, which makes it an expensive operation. I believe that is
>> the reason the resource parsing code in ACPI attempts all mappings
>> during
>> the bus scan. Rafael can you comment on this?
>>
>> One way to do what you suggest would be to defer IRQ mapping by, e.g.,
>> populating res->start with the HW IRQ number and res->end with the
>> fwnode.
>> That way we can avoid having to walk the resource buffer when a mapping
>> is needed. I don't think that approach would deviate much more from
>> the spec from what the current ahead-of-time mapping does, but it would
>> require more changes in the core code. An alternative would be to do
>> that only for resources that fail to map.
>>
>> >>
>> >>You abstracted away (2) and (3) behind acpi_register_irq(), that
>> >>on anything than does not use ACPI_GENERIC_GSI is just glue code
>> >>to acpi_register_gsi().
>> >>
>> >>Also, it is not a question on this patch but I ask it here because it
>> >>is related. On ACPI you are doing the reverse of what is done in
>> >>DT in platform_get_irq():
>> >>
>> >>- get the resources already parsed -> platform_get_resource()
>> >>- if they are disabled -> acpi_irq_get()
>> >>
>> >>and I think the ordering is tied to my question above because
>> >>you carry out the domain look up in acpi_dev_resource_interrupt()
>> >>so that if for any reason it fails the corresponding resource
>> >>is disabled so that we try to get it again through acpi_irq_get().
>> >>
>> >>I suspect you did it this way to make sure:
>> >>
>> >>a) keep the current ACPI IRQ parsing interface changes to a mininum
>> >>b) avoid changing the behaviour on x86/ia64; in particular, calling
>> >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already
>> >> registered at device creation resource parsing) multiple times can
>> >> trigger issues on x86/ia64
>>
>> You are correct about my reasons. I wanted to keep ACPI core code
>> changes
>> to a minimum, and I also needed to work within the current
>> implementation
>> which uses the pre-converted IRQ resources.
>>
>> >>
>> >>I think that's a reasonable approach but I wanted to get these
>> >>clarifications, I do not think you are far from getting this
>> >>done but since it is a significant change I think it is worth
>> >>discussing the points I raised above because I think the DT code
>> >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ
>> >>layer perspective (instead of having the domain look-up buried
>> >>inside the ACPI IRQ resource parsing API).
>> >
>> >I had another look and to achieve the above one way of doing that is to
>> >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for
>> >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI
>> >we would fall back to current solution for ACPI). Within acpi_irq_get()
>> >you can easily carry out the same steps (1->2->3) above in ACPI
>> >you have
>> >the code already there I think it is easy to change the
>> >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and
>> >the interface would become identical to of_irq_get() that is an
>> >advantage to maintain it from an IRQ maintainership perspective I
>> >think,
>> >that's my opinion.
>>
>> I think I get what you mean. I'll take a stab at implementing
>> acpi_irq_get()
>> in the way you suggest.
>>
>> >
>> >There is still a nagging snag though. When platform devices are
>> >created, core ACPI code parse the resources through:
>> >
>> >acpi_dev_get_resources()
>> >
>> >and we _have_ to have way to avoid initializing IRQ resources that
>> >have a dependency (ie there is a resource_source pointer that is valid
>> >in their descriptors) that's easy to do if we think that's the right
>> >thing to do and can hardly break current code (which ignores the
>> >resource_source altogether).
>>
>> I'd rather keep the core code as-is with regard to the ahead-of-time
>> conversion. Whether a resource source is available at the time of
>> the bus
>> scan should be transparent to the code in drivers/acpi/resource.c, and
>> we need the initialization as a disabled resource to signal the need
>> to retry anyway.
>
> Yes, exactly that's the nub. Your current code works, I am trying to
> make it more modular and similar to the DT/irqdomain IRQ look-up path,
> which has its advantages.
>
> There are two options IMHO:
>
> - always disable the resource if it has a resource_source dependency and defer
> its parsing to acpi_irq_get() (where you can easily implement steps 1-2-3 above).
> What I wanted to say is that, by disabling the resource if it has a
> resource_source dependency you can't break x86/ia64 (it is ignored at
> present - hopefully there is nothing that we are not aware of behind
> that choice). On x86/ia64 acpi_irq_get() would be an empty stub.
> This way you would keep the irqdomain look-up out of the ACPI resource
> parsing API, correct ?
> - keep code as-is
>
> Your point on _CRS being _current_ resource setting is perfectly valid
> so platform_get_resource() in platform_get_irq() must always take
> precedence over acpi_irq_get() (which should just apply to disabled
> resources), I am not sure that doing it the other way around is safe.
>
>> Rafael, do you have any other suggestions/feedback on how to go about
>> doing this?
>
> Yes, comments very appreciated, these changes are not trivial and need
> agreement.
So I need more time.
But basically, _CRS can't really change on the fly AFAICS. I'm not
even sure it is valid for it to change at all after the first
evaluation if _SRS/_PRS are not present.
Thanks,
Rafael
Hi Hanjun,
On 2016-11-29 06:31, Hanjun Guo wrote:
> Hi Agustin,
>
> On 2016/11/14 5:59, Agustin Vega-Frias wrote:
>> Add support for IRQ combiners in the Top-level Control and Status
>> Registers (TCSR) hardware block in Qualcomm Technologies chips.
>>
>> The first patch fixes IRQ probe deferral by allowing platform_device
>> IRQ resources to be re-initialized if the ACPI core failed to find
>> the IRQ domain during ACPI bus scan.
>>
>> The second patch adds support for ResourceSource/IRQ domain mapping
>> when using Extended IRQ Resources with a specific ResourceSource.
>> These changes are conditional on the ACPI_GENERIC_GSI config.
>>
>> The third patch takes advantage of the new capabilities to implement
>> the driver for the IRQ combiners.
>>
>> Tested on top of v4.9-rc4.
>>
>> Changes V6 -> V7:
>> * Consolidate changes for ResourceSource/IRQ domain mapping to the
>> same
>> source file implementing the generic GSI support, making it
>> conditional
>> on CONFIG_ACPI_GENERIC_GSI.
>> * Eliminate some code duplication by implementing acpi_register_gsi in
>> terms of the new acpi_register_irq API.
>
> I had a test on Hisilicon D03 which needs patch 1 and 2 in this patch
> set to enable the mbigen irqchip, and it works pretty good.
>
> Patch 1/3 can remove the device dependency for the irqchip and platform
> devices, and I removed my patch (ACPI: irq: introduce interrupt
> producer) then add your patch 2/3, devices such as SAS and native
> network works fine on my D03.
Thanks for testing. I will submit V8 with some of the changes suggested
by Lorenzo hopefully later today.
Agustin
>
> I will comment on the patches later.
>
> Thanks
> Hanjun
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
Hi Lorenzo,
On 2016-11-29 07:11, Lorenzo Pieralisi wrote:
> Hi Agustin,
>
> On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote:
>> Hi Rafael,
>>
>> Can you chime in on Lorenzo's feedback and the discussion below?
>> It would be great if you can comment on the reason ACPI does things
>> in a certain way.
>>
>> Hi Lorenzo,
>>
>> On 2016-11-25 06:40, Lorenzo Pieralisi wrote:
>> >Hi Agustin,
>> >
>> >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote:
>> >
>> >[...]
>> >
>> >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>> >>> {
>> >>> struct acpi_resource_irq *irq;
>> >>> struct acpi_resource_extended_irq *ext_irq;
>> >>> + struct fwnode_handle *src;
>> >>>
>> >>> switch (ares->type) {
>> >>> case ACPI_RESOURCE_TYPE_IRQ:
>> >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>> >>> acpi_dev_irqresource_disabled(res, 0);
>> >>> return false;
>> >>> }
>> >>> - acpi_dev_get_irqresource(res, irq->interrupts[index],
>> >>> + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
>> >>> irq->triggering, irq->polarity,
>> >>> irq->sharable, true);
>> >>> break;
>> >>> @@ -470,7 +472,8 @@ 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],
>> >>> + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
>> >>
>> >>Is there a reason why we need to do the domain look-up here ?
>>
>> Because we need to pass the resource down to acpi_dev_get_irqresource
>> which does the mapping through acpi_register_irq/acpi_register_gsi.
>>
>> >>
>> >>I would like to understand if, by reshuffling the code (and by
>> >>returning
>> >>the resource_source to the calling code - somehow), it would be
>> >>possible
>> >>to just mirror what the OF code does in of_irq_get(), namely:
>> >>
>> >>(1) parse the irq entry -> of_irq_parse_one()
>> >>(2) look the domain up -> irq_find_host()
>> >>(3) create the mapping -> irq_create_of_mapping()
>> >>
>> >>You wrote the code already, I think it is just a matter of shuffling
>> >>it around (well, minus returning the resource_source to the caller
>> >>which is phandle equivalent in DT).
>>
>> This is one area in which DT and ACPI are fundamentally different. In
>> DT
>> once the flattened blob is expanded the data is fixed. In ACPI the
>> data
>> returned by a method can change. In reality most methods like CRS
>> return
>> constants, but given that per-spec they are methods the interpreter
>> has
>> to be involved, which makes it an expensive operation. I believe that
>> is
>> the reason the resource parsing code in ACPI attempts all mappings
>> during
>> the bus scan. Rafael can you comment on this?
>>
>> One way to do what you suggest would be to defer IRQ mapping by, e.g.,
>> populating res->start with the HW IRQ number and res->end with the
>> fwnode.
>> That way we can avoid having to walk the resource buffer when a
>> mapping
>> is needed. I don't think that approach would deviate much more from
>> the spec from what the current ahead-of-time mapping does, but it
>> would
>> require more changes in the core code. An alternative would be to do
>> that only for resources that fail to map.
>>
>> >>
>> >>You abstracted away (2) and (3) behind acpi_register_irq(), that
>> >>on anything than does not use ACPI_GENERIC_GSI is just glue code
>> >>to acpi_register_gsi().
>> >>
>> >>Also, it is not a question on this patch but I ask it here because it
>> >>is related. On ACPI you are doing the reverse of what is done in
>> >>DT in platform_get_irq():
>> >>
>> >>- get the resources already parsed -> platform_get_resource()
>> >>- if they are disabled -> acpi_irq_get()
>> >>
>> >>and I think the ordering is tied to my question above because
>> >>you carry out the domain look up in acpi_dev_resource_interrupt()
>> >>so that if for any reason it fails the corresponding resource
>> >>is disabled so that we try to get it again through acpi_irq_get().
>> >>
>> >>I suspect you did it this way to make sure:
>> >>
>> >>a) keep the current ACPI IRQ parsing interface changes to a mininum
>> >>b) avoid changing the behaviour on x86/ia64; in particular, calling
>> >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already
>> >> registered at device creation resource parsing) multiple times can
>> >> trigger issues on x86/ia64
>>
>> You are correct about my reasons. I wanted to keep ACPI core code
>> changes
>> to a minimum, and I also needed to work within the current
>> implementation
>> which uses the pre-converted IRQ resources.
>>
>> >>
>> >>I think that's a reasonable approach but I wanted to get these
>> >>clarifications, I do not think you are far from getting this
>> >>done but since it is a significant change I think it is worth
>> >>discussing the points I raised above because I think the DT code
>> >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ
>> >>layer perspective (instead of having the domain look-up buried
>> >>inside the ACPI IRQ resource parsing API).
>> >
>> >I had another look and to achieve the above one way of doing that is to
>> >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for
>> >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI
>> >we would fall back to current solution for ACPI). Within acpi_irq_get()
>> >you can easily carry out the same steps (1->2->3) above in ACPI
>> >you have
>> >the code already there I think it is easy to change the
>> >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and
>> >the interface would become identical to of_irq_get() that is an
>> >advantage to maintain it from an IRQ maintainership perspective I
>> >think,
>> >that's my opinion.
>>
>> I think I get what you mean. I'll take a stab at implementing
>> acpi_irq_get()
>> in the way you suggest.
>>
>> >
>> >There is still a nagging snag though. When platform devices are
>> >created, core ACPI code parse the resources through:
>> >
>> >acpi_dev_get_resources()
>> >
>> >and we _have_ to have way to avoid initializing IRQ resources that
>> >have a dependency (ie there is a resource_source pointer that is valid
>> >in their descriptors) that's easy to do if we think that's the right
>> >thing to do and can hardly break current code (which ignores the
>> >resource_source altogether).
>>
>> I'd rather keep the core code as-is with regard to the ahead-of-time
>> conversion. Whether a resource source is available at the time of
>> the bus
>> scan should be transparent to the code in drivers/acpi/resource.c, and
>> we need the initialization as a disabled resource to signal the need
>> to retry anyway.
>
> Yes, exactly that's the nub. Your current code works, I am trying to
> make it more modular and similar to the DT/irqdomain IRQ look-up path,
> which has its advantages.
>
> There are two options IMHO:
>
> - always disable the resource if it has a resource_source dependency
> and defer
> its parsing to acpi_irq_get() (where you can easily implement steps
> 1-2-3 above).
> What I wanted to say is that, by disabling the resource if it has a
> resource_source dependency you can't break x86/ia64 (it is ignored at
> present - hopefully there is nothing that we are not aware of behind
> that choice). On x86/ia64 acpi_irq_get() would be an empty stub.
> This way you would keep the irqdomain look-up out of the ACPI
> resource
> parsing API, correct ?
> - keep code as-is
I plan to work on V8 today to address the point you raised w.r.t. making
it look more like DT for maintainability. I will leave other things
as-is
with an understanding that we might revisit based on Rafael's feedback
Thanks,
Agustin
>
> Your point on _CRS being _current_ resource setting is perfectly valid
> so platform_get_resource() in platform_get_irq() must always take
> precedence over acpi_irq_get() (which should just apply to disabled
> resources), I am not sure that doing it the other way around is safe.
>
>> Rafael, do you have any other suggestions/feedback on how to go about
>> doing this?
>
> Yes, comments very appreciated, these changes are not trivial and need
> agreement.
>
> Thanks,
> Lorenzo
>
>>
>> Thanks,
>> Agustin
>>
>> >
>> >It is an important difference with DT probing, where the IRQ
>> >resources are only created if the domain reference (ie interrupt
>> >controller phandle) is satisfied at of_device_alloc() time
>> >(see of_device_alloc()).
>> >
>> >Thoughts ? Please let me know, the code to implement what I say
>> >is already in these patches, it is just a matter of reshuffling it.
>> >
>> >Thanks !
>> >Lorenzo
>>
>> --
>> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
>> Technologies, Inc.
>> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
>> Linux Foundation Collaborative Project.
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
Hi Rafael,
On 2016-11-29 07:43, Rafael J. Wysocki wrote:
> On Tue, Nov 29, 2016 at 1:11 PM, Lorenzo Pieralisi
> <[email protected]> wrote:
>> Hi Agustin,
>>
>> On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote:
>>> Hi Rafael,
>>>
>>> Can you chime in on Lorenzo's feedback and the discussion below?
>>> It would be great if you can comment on the reason ACPI does things
>>> in a certain way.
>>>
>>> Hi Lorenzo,
>>>
>>> On 2016-11-25 06:40, Lorenzo Pieralisi wrote:
>>> >Hi Agustin,
>>> >
>>> >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote:
>>> >
>>> >[...]
>>> >
>>> >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>>> >>> {
>>> >>> struct acpi_resource_irq *irq;
>>> >>> struct acpi_resource_extended_irq *ext_irq;
>>> >>> + struct fwnode_handle *src;
>>> >>>
>>> >>> switch (ares->type) {
>>> >>> case ACPI_RESOURCE_TYPE_IRQ:
>>> >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>>> >>> acpi_dev_irqresource_disabled(res, 0);
>>> >>> return false;
>>> >>> }
>>> >>> - acpi_dev_get_irqresource(res, irq->interrupts[index],
>>> >>> + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
>>> >>> irq->triggering, irq->polarity,
>>> >>> irq->sharable, true);
>>> >>> break;
>>> >>> @@ -470,7 +472,8 @@ 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],
>>> >>> + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
>>> >>
>>> >>Is there a reason why we need to do the domain look-up here ?
>>>
>>> Because we need to pass the resource down to acpi_dev_get_irqresource
>>> which does the mapping through acpi_register_irq/acpi_register_gsi.
>>>
>>> >>
>>> >>I would like to understand if, by reshuffling the code (and by
>>> >>returning
>>> >>the resource_source to the calling code - somehow), it would be
>>> >>possible
>>> >>to just mirror what the OF code does in of_irq_get(), namely:
>>> >>
>>> >>(1) parse the irq entry -> of_irq_parse_one()
>>> >>(2) look the domain up -> irq_find_host()
>>> >>(3) create the mapping -> irq_create_of_mapping()
>>> >>
>>> >>You wrote the code already, I think it is just a matter of shuffling
>>> >>it around (well, minus returning the resource_source to the caller
>>> >>which is phandle equivalent in DT).
>>>
>>> This is one area in which DT and ACPI are fundamentally different. In
>>> DT
>>> once the flattened blob is expanded the data is fixed. In ACPI the
>>> data
>>> returned by a method can change. In reality most methods like CRS
>>> return
>>> constants, but given that per-spec they are methods the interpreter
>>> has
>>> to be involved, which makes it an expensive operation. I believe that
>>> is
>>> the reason the resource parsing code in ACPI attempts all mappings
>>> during
>>> the bus scan. Rafael can you comment on this?
>>>
>>> One way to do what you suggest would be to defer IRQ mapping by,
>>> e.g.,
>>> populating res->start with the HW IRQ number and res->end with the
>>> fwnode.
>>> That way we can avoid having to walk the resource buffer when a
>>> mapping
>>> is needed. I don't think that approach would deviate much more from
>>> the spec from what the current ahead-of-time mapping does, but it
>>> would
>>> require more changes in the core code. An alternative would be to do
>>> that only for resources that fail to map.
>>>
>>> >>
>>> >>You abstracted away (2) and (3) behind acpi_register_irq(), that
>>> >>on anything than does not use ACPI_GENERIC_GSI is just glue code
>>> >>to acpi_register_gsi().
>>> >>
>>> >>Also, it is not a question on this patch but I ask it here because it
>>> >>is related. On ACPI you are doing the reverse of what is done in
>>> >>DT in platform_get_irq():
>>> >>
>>> >>- get the resources already parsed -> platform_get_resource()
>>> >>- if they are disabled -> acpi_irq_get()
>>> >>
>>> >>and I think the ordering is tied to my question above because
>>> >>you carry out the domain look up in acpi_dev_resource_interrupt()
>>> >>so that if for any reason it fails the corresponding resource
>>> >>is disabled so that we try to get it again through acpi_irq_get().
>>> >>
>>> >>I suspect you did it this way to make sure:
>>> >>
>>> >>a) keep the current ACPI IRQ parsing interface changes to a mininum
>>> >>b) avoid changing the behaviour on x86/ia64; in particular, calling
>>> >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already
>>> >> registered at device creation resource parsing) multiple times can
>>> >> trigger issues on x86/ia64
>>>
>>> You are correct about my reasons. I wanted to keep ACPI core code
>>> changes
>>> to a minimum, and I also needed to work within the current
>>> implementation
>>> which uses the pre-converted IRQ resources.
>>>
>>> >>
>>> >>I think that's a reasonable approach but I wanted to get these
>>> >>clarifications, I do not think you are far from getting this
>>> >>done but since it is a significant change I think it is worth
>>> >>discussing the points I raised above because I think the DT code
>>> >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ
>>> >>layer perspective (instead of having the domain look-up buried
>>> >>inside the ACPI IRQ resource parsing API).
>>> >
>>> >I had another look and to achieve the above one way of doing that is to
>>> >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for
>>> >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI
>>> >we would fall back to current solution for ACPI). Within acpi_irq_get()
>>> >you can easily carry out the same steps (1->2->3) above in ACPI
>>> >you have
>>> >the code already there I think it is easy to change the
>>> >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and
>>> >the interface would become identical to of_irq_get() that is an
>>> >advantage to maintain it from an IRQ maintainership perspective I
>>> >think,
>>> >that's my opinion.
>>>
>>> I think I get what you mean. I'll take a stab at implementing
>>> acpi_irq_get()
>>> in the way you suggest.
>>>
>>> >
>>> >There is still a nagging snag though. When platform devices are
>>> >created, core ACPI code parse the resources through:
>>> >
>>> >acpi_dev_get_resources()
>>> >
>>> >and we _have_ to have way to avoid initializing IRQ resources that
>>> >have a dependency (ie there is a resource_source pointer that is valid
>>> >in their descriptors) that's easy to do if we think that's the right
>>> >thing to do and can hardly break current code (which ignores the
>>> >resource_source altogether).
>>>
>>> I'd rather keep the core code as-is with regard to the ahead-of-time
>>> conversion. Whether a resource source is available at the time of
>>> the bus
>>> scan should be transparent to the code in drivers/acpi/resource.c,
>>> and
>>> we need the initialization as a disabled resource to signal the need
>>> to retry anyway.
>>
>> Yes, exactly that's the nub. Your current code works, I am trying to
>> make it more modular and similar to the DT/irqdomain IRQ look-up path,
>> which has its advantages.
>>
>> There are two options IMHO:
>>
>> - always disable the resource if it has a resource_source dependency
>> and defer
>> its parsing to acpi_irq_get() (where you can easily implement steps
>> 1-2-3 above).
>> What I wanted to say is that, by disabling the resource if it has a
>> resource_source dependency you can't break x86/ia64 (it is ignored
>> at
>> present - hopefully there is nothing that we are not aware of behind
>> that choice). On x86/ia64 acpi_irq_get() would be an empty stub.
>> This way you would keep the irqdomain look-up out of the ACPI
>> resource
>> parsing API, correct ?
>> - keep code as-is
>>
>> Your point on _CRS being _current_ resource setting is perfectly valid
>> so platform_get_resource() in platform_get_irq() must always take
>> precedence over acpi_irq_get() (which should just apply to disabled
>> resources), I am not sure that doing it the other way around is safe.
>>
>>> Rafael, do you have any other suggestions/feedback on how to go about
>>> doing this?
>>
>> Yes, comments very appreciated, these changes are not trivial and need
>> agreement.
>
> So I need more time.
Please wait for V8 which will address some issues raised by Lorenzo.
>
> But basically, _CRS can't really change on the fly AFAICS. I'm not
> even sure it is valid for it to change at all after the first
> evaluation if _SRS/_PRS are not present.
That's good to know and it opens more possibilities.
Thanks,
Agustin
>
> Thanks,
> Rafael
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
On Tue, Nov 29, 2016 at 4:20 PM, Agustin Vega-Frias
<[email protected]> wrote:
> Hi Rafael,
>
>
> On 2016-11-29 07:43, Rafael J. Wysocki wrote:
>>
>> On Tue, Nov 29, 2016 at 1:11 PM, Lorenzo Pieralisi
>> <[email protected]> wrote:
>>>
>>> Hi Agustin,
>>>
>>> On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> Can you chime in on Lorenzo's feedback and the discussion below?
>>>> It would be great if you can comment on the reason ACPI does things
>>>> in a certain way.
>>>>
>>>> Hi Lorenzo,
>>>>
>>>> On 2016-11-25 06:40, Lorenzo Pieralisi wrote:
>>>> >Hi Agustin,
>>>> >
>>>> >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote:
>>>> >
>>>> >[...]
>>>> >
>>>> >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct
>>>> >>> acpi_resource *ares, int index,
>>>> >>> {
>>>> >>> struct acpi_resource_irq *irq;
>>>> >>> struct acpi_resource_extended_irq *ext_irq;
>>>> >>> + struct fwnode_handle *src;
>>>> >>>
>>>> >>> switch (ares->type) {
>>>> >>> case ACPI_RESOURCE_TYPE_IRQ:
>>>> >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct
>>>> >>> acpi_resource *ares, int index,
>>>> >>> acpi_dev_irqresource_disabled(res, 0);
>>>> >>> return false;
>>>> >>> }
>>>> >>> - acpi_dev_get_irqresource(res, irq->interrupts[index],
>>>> >>> + acpi_dev_get_irqresource(res, irq->interrupts[index],
>>>> >>> NULL,
>>>> >>> irq->triggering, irq->polarity,
>>>> >>> irq->sharable, true);
>>>> >>> break;
>>>> >>> @@ -470,7 +472,8 @@ 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],
>>>> >>> + src =
>>>> >>> acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
>>>> >>
>>>> >>Is there a reason why we need to do the domain look-up here ?
>>>>
>>>> Because we need to pass the resource down to acpi_dev_get_irqresource
>>>> which does the mapping through acpi_register_irq/acpi_register_gsi.
>>>>
>>>> >>
>>>> >>I would like to understand if, by reshuffling the code (and by
>>>> >>returning
>>>> >>the resource_source to the calling code - somehow), it would be
>>>> >>possible
>>>> >>to just mirror what the OF code does in of_irq_get(), namely:
>>>> >>
>>>> >>(1) parse the irq entry -> of_irq_parse_one()
>>>> >>(2) look the domain up -> irq_find_host()
>>>> >>(3) create the mapping -> irq_create_of_mapping()
>>>> >>
>>>> >>You wrote the code already, I think it is just a matter of shuffling
>>>> >>it around (well, minus returning the resource_source to the caller
>>>> >>which is phandle equivalent in DT).
>>>>
>>>> This is one area in which DT and ACPI are fundamentally different. In DT
>>>> once the flattened blob is expanded the data is fixed. In ACPI the data
>>>> returned by a method can change. In reality most methods like CRS return
>>>> constants, but given that per-spec they are methods the interpreter has
>>>> to be involved, which makes it an expensive operation. I believe that is
>>>> the reason the resource parsing code in ACPI attempts all mappings
>>>> during
>>>> the bus scan. Rafael can you comment on this?
>>>>
>>>> One way to do what you suggest would be to defer IRQ mapping by, e.g.,
>>>> populating res->start with the HW IRQ number and res->end with the
>>>> fwnode.
>>>> That way we can avoid having to walk the resource buffer when a mapping
>>>> is needed. I don't think that approach would deviate much more from
>>>> the spec from what the current ahead-of-time mapping does, but it would
>>>> require more changes in the core code. An alternative would be to do
>>>> that only for resources that fail to map.
>>>>
>>>> >>
>>>> >>You abstracted away (2) and (3) behind acpi_register_irq(), that
>>>> >>on anything than does not use ACPI_GENERIC_GSI is just glue code
>>>> >>to acpi_register_gsi().
>>>> >>
>>>> >>Also, it is not a question on this patch but I ask it here because it
>>>> >>is related. On ACPI you are doing the reverse of what is done in
>>>> >>DT in platform_get_irq():
>>>> >>
>>>> >>- get the resources already parsed -> platform_get_resource()
>>>> >>- if they are disabled -> acpi_irq_get()
>>>> >>
>>>> >>and I think the ordering is tied to my question above because
>>>> >>you carry out the domain look up in acpi_dev_resource_interrupt()
>>>> >>so that if for any reason it fails the corresponding resource
>>>> >>is disabled so that we try to get it again through acpi_irq_get().
>>>> >>
>>>> >>I suspect you did it this way to make sure:
>>>> >>
>>>> >>a) keep the current ACPI IRQ parsing interface changes to a mininum
>>>> >>b) avoid changing the behaviour on x86/ia64; in particular, calling
>>>> >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already
>>>> >> registered at device creation resource parsing) multiple times can
>>>> >> trigger issues on x86/ia64
>>>>
>>>> You are correct about my reasons. I wanted to keep ACPI core code
>>>> changes
>>>> to a minimum, and I also needed to work within the current
>>>> implementation
>>>> which uses the pre-converted IRQ resources.
>>>>
>>>> >>
>>>> >>I think that's a reasonable approach but I wanted to get these
>>>> >>clarifications, I do not think you are far from getting this
>>>> >>done but since it is a significant change I think it is worth
>>>> >>discussing the points I raised above because I think the DT code
>>>> >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ
>>>> >>layer perspective (instead of having the domain look-up buried
>>>> >>inside the ACPI IRQ resource parsing API).
>>>> >
>>>> >I had another look and to achieve the above one way of doing that is to
>>>> >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for
>>>> >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI
>>>> >we would fall back to current solution for ACPI). Within acpi_irq_get()
>>>> >you can easily carry out the same steps (1->2->3) above in ACPI
>>>> >you have
>>>> >the code already there I think it is easy to change the
>>>> >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and
>>>> >the interface would become identical to of_irq_get() that is an
>>>> >advantage to maintain it from an IRQ maintainership perspective I
>>>> >think,
>>>> >that's my opinion.
>>>>
>>>> I think I get what you mean. I'll take a stab at implementing
>>>> acpi_irq_get()
>>>> in the way you suggest.
>>>>
>>>> >
>>>> >There is still a nagging snag though. When platform devices are
>>>> >created, core ACPI code parse the resources through:
>>>> >
>>>> >acpi_dev_get_resources()
>>>> >
>>>> >and we _have_ to have way to avoid initializing IRQ resources that
>>>> >have a dependency (ie there is a resource_source pointer that is valid
>>>> >in their descriptors) that's easy to do if we think that's the right
>>>> >thing to do and can hardly break current code (which ignores the
>>>> >resource_source altogether).
>>>>
>>>> I'd rather keep the core code as-is with regard to the ahead-of-time
>>>> conversion. Whether a resource source is available at the time of
>>>> the bus
>>>> scan should be transparent to the code in drivers/acpi/resource.c, and
>>>> we need the initialization as a disabled resource to signal the need
>>>> to retry anyway.
>>>
>>>
>>> Yes, exactly that's the nub. Your current code works, I am trying to
>>> make it more modular and similar to the DT/irqdomain IRQ look-up path,
>>> which has its advantages.
>>>
>>> There are two options IMHO:
>>>
>>> - always disable the resource if it has a resource_source dependency and
>>> defer
>>> its parsing to acpi_irq_get() (where you can easily implement steps
>>> 1-2-3 above).
>>> What I wanted to say is that, by disabling the resource if it has a
>>> resource_source dependency you can't break x86/ia64 (it is ignored at
>>> present - hopefully there is nothing that we are not aware of behind
>>> that choice). On x86/ia64 acpi_irq_get() would be an empty stub.
>>> This way you would keep the irqdomain look-up out of the ACPI resource
>>> parsing API, correct ?
>>> - keep code as-is
>>>
>>> Your point on _CRS being _current_ resource setting is perfectly valid
>>> so platform_get_resource() in platform_get_irq() must always take
>>> precedence over acpi_irq_get() (which should just apply to disabled
>>> resources), I am not sure that doing it the other way around is safe.
>>>
>>>> Rafael, do you have any other suggestions/feedback on how to go about
>>>> doing this?
>>>
>>>
>>> Yes, comments very appreciated, these changes are not trivial and need
>>> agreement.
>>
>>
>> So I need more time.
>
>
> Please wait for V8 which will address some issues raised by Lorenzo.
>
>>
>> But basically, _CRS can't really change on the fly AFAICS. I'm not
>> even sure it is valid for it to change at all after the first
>> evaluation if _SRS/_PRS are not present.
>
>
> That's good to know and it opens more possibilities.
Actually, to me it follows from the very purpose of _CRS that, as long
as the device is enabled, it should be expected to return the same
data every time it is evaluated, unless _SRS is invoked in the
meantime. Otherwise, it would be possible for the device's resources
to change unexpectedly under a driver using it.
Thanks,
Rafael
On 2016/11/30 0:26, Rafael J. Wysocki wrote:
> On Tue, Nov 29, 2016 at 4:20 PM, Agustin Vega-Frias
> <[email protected]> wrote:
>> Hi Rafael,
>>
>>
>> On 2016-11-29 07:43, Rafael J. Wysocki wrote:
>>> On Tue, Nov 29, 2016 at 1:11 PM, Lorenzo Pieralisi
>>> <[email protected]> wrote:
>>>> Hi Agustin,
>>>>
>>>> On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote:
>>>>> Hi Rafael,
>>>>>
>>>>> Can you chime in on Lorenzo's feedback and the discussion below?
>>>>> It would be great if you can comment on the reason ACPI does things
>>>>> in a certain way.
>>>>>
>>>>> Hi Lorenzo,
>>>>>
>>>>> On 2016-11-25 06:40, Lorenzo Pieralisi wrote:
>>>>>> Hi Agustin,
>>>>>>
>>>>>> On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct
>>>>>>>> acpi_resource *ares, int index,
>>>>>>>> {
>>>>>>>> struct acpi_resource_irq *irq;
>>>>>>>> struct acpi_resource_extended_irq *ext_irq;
>>>>>>>> + struct fwnode_handle *src;
>>>>>>>>
>>>>>>>> switch (ares->type) {
>>>>>>>> case ACPI_RESOURCE_TYPE_IRQ:
>>>>>>>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct
>>>>>>>> acpi_resource *ares, int index,
>>>>>>>> acpi_dev_irqresource_disabled(res, 0);
>>>>>>>> return false;
>>>>>>>> }
>>>>>>>> - acpi_dev_get_irqresource(res, irq->interrupts[index],
>>>>>>>> + acpi_dev_get_irqresource(res, irq->interrupts[index],
>>>>>>>> NULL,
>>>>>>>> irq->triggering, irq->polarity,
>>>>>>>> irq->sharable, true);
>>>>>>>> break;
>>>>>>>> @@ -470,7 +472,8 @@ 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],
>>>>>>>> + src =
>>>>>>>> acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
>>>>>>> Is there a reason why we need to do the domain look-up here ?
>>>>> Because we need to pass the resource down to acpi_dev_get_irqresource
>>>>> which does the mapping through acpi_register_irq/acpi_register_gsi.
>>>>>
>>>>>>> I would like to understand if, by reshuffling the code (and by
>>>>>>> returning
>>>>>>> the resource_source to the calling code - somehow), it would be
>>>>>>> possible
>>>>>>> to just mirror what the OF code does in of_irq_get(), namely:
>>>>>>>
>>>>>>> (1) parse the irq entry -> of_irq_parse_one()
>>>>>>> (2) look the domain up -> irq_find_host()
>>>>>>> (3) create the mapping -> irq_create_of_mapping()
>>>>>>>
>>>>>>> You wrote the code already, I think it is just a matter of shuffling
>>>>>>> it around (well, minus returning the resource_source to the caller
>>>>>>> which is phandle equivalent in DT).
>>>>> This is one area in which DT and ACPI are fundamentally different. In DT
>>>>> once the flattened blob is expanded the data is fixed. In ACPI the data
>>>>> returned by a method can change. In reality most methods like CRS return
>>>>> constants, but given that per-spec they are methods the interpreter has
>>>>> to be involved, which makes it an expensive operation. I believe that is
>>>>> the reason the resource parsing code in ACPI attempts all mappings
>>>>> during
>>>>> the bus scan. Rafael can you comment on this?
>>>>>
>>>>> One way to do what you suggest would be to defer IRQ mapping by, e.g.,
>>>>> populating res->start with the HW IRQ number and res->end with the
>>>>> fwnode.
>>>>> That way we can avoid having to walk the resource buffer when a mapping
>>>>> is needed. I don't think that approach would deviate much more from
>>>>> the spec from what the current ahead-of-time mapping does, but it would
>>>>> require more changes in the core code. An alternative would be to do
>>>>> that only for resources that fail to map.
>>>>>
>>>>>>> You abstracted away (2) and (3) behind acpi_register_irq(), that
>>>>>>> on anything than does not use ACPI_GENERIC_GSI is just glue code
>>>>>>> to acpi_register_gsi().
>>>>>>>
>>>>>>> Also, it is not a question on this patch but I ask it here because it
>>>>>>> is related. On ACPI you are doing the reverse of what is done in
>>>>>>> DT in platform_get_irq():
>>>>>>>
>>>>>>> - get the resources already parsed -> platform_get_resource()
>>>>>>> - if they are disabled -> acpi_irq_get()
>>>>>>>
>>>>>>> and I think the ordering is tied to my question above because
>>>>>>> you carry out the domain look up in acpi_dev_resource_interrupt()
>>>>>>> so that if for any reason it fails the corresponding resource
>>>>>>> is disabled so that we try to get it again through acpi_irq_get().
>>>>>>>
>>>>>>> I suspect you did it this way to make sure:
>>>>>>>
>>>>>>> a) keep the current ACPI IRQ parsing interface changes to a mininum
>>>>>>> b) avoid changing the behaviour on x86/ia64; in particular, calling
>>>>>>> acpi_register_gsi() for the _same_ mapping (an IRQ that was already
>>>>>>> registered at device creation resource parsing) multiple times can
>>>>>>> trigger issues on x86/ia64
>>>>> You are correct about my reasons. I wanted to keep ACPI core code
>>>>> changes
>>>>> to a minimum, and I also needed to work within the current
>>>>> implementation
>>>>> which uses the pre-converted IRQ resources.
>>>>>
>>>>>>> I think that's a reasonable approach but I wanted to get these
>>>>>>> clarifications, I do not think you are far from getting this
>>>>>>> done but since it is a significant change I think it is worth
>>>>>>> discussing the points I raised above because I think the DT code
>>>>>>> sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ
>>>>>>> layer perspective (instead of having the domain look-up buried
>>>>>>> inside the ACPI IRQ resource parsing API).
>>>>>> I had another look and to achieve the above one way of doing that is to
>>>>>> implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for
>>>>>> !ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI
>>>>>> we would fall back to current solution for ACPI). Within acpi_irq_get()
>>>>>> you can easily carry out the same steps (1->2->3) above in ACPI
>>>>>> you have
>>>>>> the code already there I think it is easy to change the
>>>>>> acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and
>>>>>> the interface would become identical to of_irq_get() that is an
>>>>>> advantage to maintain it from an IRQ maintainership perspective I
>>>>>> think,
>>>>>> that's my opinion.
>>>>> I think I get what you mean. I'll take a stab at implementing
>>>>> acpi_irq_get()
>>>>> in the way you suggest.
>>>>>
>>>>>> There is still a nagging snag though. When platform devices are
>>>>>> created, core ACPI code parse the resources through:
>>>>>>
>>>>>> acpi_dev_get_resources()
>>>>>>
>>>>>> and we _have_ to have way to avoid initializing IRQ resources that
>>>>>> have a dependency (ie there is a resource_source pointer that is valid
>>>>>> in their descriptors) that's easy to do if we think that's the right
>>>>>> thing to do and can hardly break current code (which ignores the
>>>>>> resource_source altogether).
>>>>> I'd rather keep the core code as-is with regard to the ahead-of-time
>>>>> conversion. Whether a resource source is available at the time of
>>>>> the bus
>>>>> scan should be transparent to the code in drivers/acpi/resource.c, and
>>>>> we need the initialization as a disabled resource to signal the need
>>>>> to retry anyway.
>>>>
>>>> Yes, exactly that's the nub. Your current code works, I am trying to
>>>> make it more modular and similar to the DT/irqdomain IRQ look-up path,
>>>> which has its advantages.
>>>>
>>>> There are two options IMHO:
>>>>
>>>> - always disable the resource if it has a resource_source dependency and
>>>> defer
>>>> its parsing to acpi_irq_get() (where you can easily implement steps
>>>> 1-2-3 above).
>>>> What I wanted to say is that, by disabling the resource if it has a
>>>> resource_source dependency you can't break x86/ia64 (it is ignored at
>>>> present - hopefully there is nothing that we are not aware of behind
>>>> that choice). On x86/ia64 acpi_irq_get() would be an empty stub.
>>>> This way you would keep the irqdomain look-up out of the ACPI resource
>>>> parsing API, correct ?
>>>> - keep code as-is
>>>>
>>>> Your point on _CRS being _current_ resource setting is perfectly valid
>>>> so platform_get_resource() in platform_get_irq() must always take
>>>> precedence over acpi_irq_get() (which should just apply to disabled
>>>> resources), I am not sure that doing it the other way around is safe.
>>>>
>>>>> Rafael, do you have any other suggestions/feedback on how to go about
>>>>> doing this?
>>>>
>>>> Yes, comments very appreciated, these changes are not trivial and need
>>>> agreement.
>>>
>>> So I need more time.
>>
>> Please wait for V8 which will address some issues raised by Lorenzo.
>>
>>> But basically, _CRS can't really change on the fly AFAICS. I'm not
>>> even sure it is valid for it to change at all after the first
>>> evaluation if _SRS/_PRS are not present.
>>
>> That's good to know and it opens more possibilities.
> Actually, to me it follows from the very purpose of _CRS that, as long
> as the device is enabled, it should be expected to return the same
> data every time it is evaluated, unless _SRS is invoked in the
> meantime. Otherwise, it would be possible for the device's resources
> to change unexpectedly under a driver using it.
Agreed, here is the hotplug case:
For hotplug cases, _CRS is updated before the device is enabled. for example
for a memory hot add, BIOS will update the _CRS to collect memory address
ranges before notify OS, but once the BIOS notified OS to add the memory in
(BIOS will enable the device to set the _STA to functional), _CRS can't be updated.
Thanks
Hanjun