The Tegra210 AGIC interrupt controller is a 2nd level interrupt controller
located in a separate power domain to the main GIC interrupt controller.
It can route interrupts to the main CPU cluster or an Audio DSP slave.
Ideally we would like to re-use the existing ARM GIC driver because the
AGIC is a GIC-400. However, in order to do so this requires a few
significant changes to the exisiting GIC driver for power management
reasons.
The purpose of this RFC-V2 is to get some feedback on the approach and see
if we can support the AGIC in this way or if a separate driver is
warranted for this device.
Please note that this RFC does not address the issue of interrupt routing.
Ideally I was thinking that having a mechanism/API to migrate an interrupt
from the CPU cluster to the DSP would make sense, however, I don't believe
this is supported today in the kernel. Would such an approach be acceptable
or if not, is there a better way to handle this?
Changes from V1:
- Prevent IRQ mapping from setting the IRQ type and only program the
type when allocating the IRQ.
- Resolved some __init section conflicts found with V1 series.
Jon Hunter (8):
irqdomain: Ensure type settings match for an existing mapping
irqdomain: Don't set type when mapping an IRQ
genirq: Add runtime power management support for IRQ chips
irqchip/gic: Don't initialise chip if mapping IO space fails
irqchip/gic: Return an error if GIC initialisation fails
irqchip/gic: Assign irqchip dynamically
irqchip/gic: Prepare for adding platform driver
irqchip/gic: Add support for tegra AGIC interrupt controller
drivers/irqchip/irq-gic-common.c | 4 +-
drivers/irqchip/irq-gic.c | 473 ++++++++++++++++++++++++++++-----------
include/linux/irq.h | 4 +
kernel/irq/internals.h | 24 ++
kernel/irq/irqdomain.c | 76 +++++--
kernel/irq/manage.c | 14 ++
6 files changed, 447 insertions(+), 148 deletions(-)
--
2.1.4
When mapping an IRQ, if a mapping already exists, then we simply return
the virual IRQ number. However, we do not check that the type settings for
the existing mapping match those for the mapping that is about to be
created. It may be unlikely that the type settings would not match, but
check for this and don't return a valid IRQ mapping if the type settings
do not match.
Signed-off-by: Jon Hunter <[email protected]>
---
kernel/irq/irqdomain.c | 58 +++++++++++++++++++++++++++++++++++++-------------
1 file changed, 43 insertions(+), 15 deletions(-)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 22aa9612ef7c..eae31e978ab2 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -568,8 +568,10 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
{
+ struct device_node *of_node;
struct irq_domain *domain;
irq_hw_number_t hwirq;
+ unsigned int cur_type = IRQ_TYPE_NONE;
unsigned int type = IRQ_TYPE_NONE;
int virq;
@@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
if (irq_domain_translate(domain, fwspec, &hwirq, &type))
return 0;
- if (irq_domain_is_hierarchy(domain)) {
- /*
- * If we've already configured this interrupt,
- * don't do it again, or hell will break loose.
- */
- virq = irq_find_mapping(domain, hwirq);
- if (virq)
- return virq;
+ of_node = irq_domain_get_of_node(domain);
- virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
- if (virq <= 0)
- return 0;
+ /*
+ * If we've already configured this interrupt,
+ * don't do it again, or hell will break loose.
+ */
+ virq = irq_find_mapping(domain, hwirq);
+ if (!virq) {
+ if (irq_domain_is_hierarchy(domain)) {
+ virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE,
+ fwspec);
+ if (virq <= 0)
+ return 0;
+ } else {
+ virq = irq_domain_alloc_descs(-1, 1, hwirq,
+ of_node_to_nid(of_node));
+ if (virq <= 0)
+ return 0;
+
+ if (irq_domain_associate(domain, virq, hwirq)) {
+ irq_free_desc(virq);
+ return 0;
+ }
+ }
} else {
- /* Create mapping */
- virq = irq_create_mapping(domain, hwirq);
- if (!virq)
- return virq;
+ cur_type = irq_get_trigger_type(virq);
+ }
+
+ /*
+ * If the trigger type is not specified or matches the current
+ * trigger type then we are done so return the interrupt number.
+ */
+ if (type == IRQ_TYPE_NONE || type == cur_type)
+ return virq;
+
+ /*
+ * If the trigger type is already set and does
+ * not match this interrupt, then return 0.
+ */
+ if (cur_type != IRQ_TYPE_NONE) {
+ pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
+ hwirq, of_node_full_name(of_node));
+ return 0;
}
/* Set type if specified and different than the current one */
--
2.1.4
Some IRQ chips, such as GPIO controllers or secondary level interrupt
controllers, may require require additional runtime power management
control to ensure they are accessible. For such IRQ chips, it makes sense
to enable the IRQ chip when interrupts are requested and disabled them
again once all interrupts have been freed.
When mapping an IRQ, the IRQ type settings are read and then programmed.
The mapping of the IRQ happens before the IRQ is requested and so the
programming of the type settings occurs before the IRQ is requested. This
is a problem for IRQ chips that require additional power management
control because they may not be accessible yet. Therefore, when mapping
the IRQ, don't program the type settings, just save them and then program
these saved settings when the IRQ is requested (so long as if they are not
overridden via the call to request the IRQ).
Signed-off-by: Jon Hunter <[email protected]>
---
kernel/irq/irqdomain.c | 18 ++++++++++++++----
kernel/irq/manage.c | 7 +++++++
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index eae31e978ab2..4322d6fd0b8f 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -570,6 +570,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
{
struct device_node *of_node;
struct irq_domain *domain;
+ struct irq_data *irq_data;
irq_hw_number_t hwirq;
unsigned int cur_type = IRQ_TYPE_NONE;
unsigned int type = IRQ_TYPE_NONE;
@@ -634,10 +635,19 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
return 0;
}
- /* Set type if specified and different than the current one */
- if (type != IRQ_TYPE_NONE &&
- type != irq_get_trigger_type(virq))
- irq_set_irq_type(virq, type);
+ irq_data = irq_get_irq_data(virq);
+ if (!irq_data) {
+ if (irq_domain_is_hierarchy(domain))
+ irq_domain_free_irqs(virq, 1);
+ else
+ irq_dispose_mapping(virq);
+
+ return 0;
+ }
+
+ /* Store trigger type */
+ irqd_set_trigger_type(irq_data, type);
+
return virq;
}
EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 0eebaeef317b..2a429b061171 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1119,6 +1119,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
new->irq = irq;
/*
+ * If the trigger type is not specified by the caller,
+ * then use the default for this interrupt.
+ */
+ if (!(new->flags & IRQF_TRIGGER_MASK))
+ new->flags |= irqd_get_trigger_type(&desc->irq_data);
+
+ /*
* Check whether the interrupt nests into another interrupt
* thread.
*/
--
2.1.4
Some IRQ chips may be located in a power domain outside of the CPU
subsystem and hence will require device specific runtime power management.
In order to support such IRQ chips, add a pointer for a device structure
to the irq_chip structure, and if this pointer is populated by the IRQ
chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
APIs for this chip will be called when an IRQ is requested/freed,
respectively.
Signed-off-by: Jon Hunter <[email protected]>
---
include/linux/irq.h | 4 ++++
kernel/irq/internals.h | 24 ++++++++++++++++++++++++
kernel/irq/manage.c | 7 +++++++
3 files changed, 35 insertions(+)
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 3c1c96786248..7a61a7f76177 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
/**
* struct irq_chip - hardware interrupt chip descriptor
*
+ * @dev: pointer to associated device
* @name: name for /proc/interrupts
* @irq_startup: start up the interrupt (defaults to ->enable if NULL)
* @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL)
@@ -344,6 +345,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
* @flags: chip specific flags
*/
struct irq_chip {
+ struct device *dev;
const char *name;
unsigned int (*irq_startup)(struct irq_data *data);
void (*irq_shutdown)(struct irq_data *data);
@@ -399,6 +401,7 @@ struct irq_chip {
* IRQCHIP_SKIP_SET_WAKE: Skip chip.irq_set_wake(), for this irq chip
* IRQCHIP_ONESHOT_SAFE: One shot does not require mask/unmask
* IRQCHIP_EOI_THREADED: Chip requires eoi() on unmask in threaded mode
+ * IRQCHIP_HAS_PM: Chip requires runtime power management
*/
enum {
IRQCHIP_SET_TYPE_MASKED = (1 << 0),
@@ -408,6 +411,7 @@ enum {
IRQCHIP_SKIP_SET_WAKE = (1 << 4),
IRQCHIP_ONESHOT_SAFE = (1 << 5),
IRQCHIP_EOI_THREADED = (1 << 6),
+ IRQCHIP_HAS_RPM = (1 << 7),
};
#include <linux/irqdesc.h>
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index fcab63c66905..30a2add7cae6 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -7,6 +7,7 @@
*/
#include <linux/irqdesc.h>
#include <linux/kernel_stat.h>
+#include <linux/pm_runtime.h>
#ifdef CONFIG_SPARSE_IRQ
# define IRQ_BITMAP_BITS (NR_IRQS + 8196)
@@ -125,6 +126,29 @@ static inline void chip_bus_sync_unlock(struct irq_desc *desc)
desc->irq_data.chip->irq_bus_sync_unlock(&desc->irq_data);
}
+/* Inline functions for support of irq chips that require runtime pm */
+static inline int chip_pm_get(struct irq_desc *desc)
+{
+ int retval = 0;
+
+ if (desc->irq_data.chip->dev &&
+ desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
+ retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
+
+ return (retval < 0) ? retval : 0;
+}
+
+static inline int chip_pm_put(struct irq_desc *desc)
+{
+ int retval = 0;
+
+ if (desc->irq_data.chip->dev &&
+ desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
+ retval = pm_runtime_put(desc->irq_data.chip->dev);
+
+ return (retval < 0) ? retval : 0;
+}
+
#define _IRQ_DESC_CHECK (1 << 0)
#define _IRQ_DESC_PERCPU (1 << 1)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 2a429b061171..8a96e4f1e985 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
if (!try_module_get(desc->owner))
return -ENODEV;
+ ret = chip_pm_get(desc);
+ if (ret < 0)
+ return ret;
+
new->irq = irq;
/*
@@ -1400,6 +1404,7 @@ out_thread:
put_task_struct(t);
}
out_mput:
+ chip_pm_put(desc);
module_put(desc->owner);
return ret;
}
@@ -1513,6 +1518,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
}
}
+ chip_pm_put(desc);
module_put(desc->owner);
kfree(action->secondary);
return action;
@@ -1799,6 +1805,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
unregister_handler_proc(irq, action);
+ chip_pm_put(desc);
module_put(desc->owner);
return action;
--
2.1.4
If we fail to map the address space for the GIC distributor or CPU
interface, then don't attempt to initialise the chip, just WARN and
return.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/irqchip/irq-gic.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index abf2ffaed392..561a5cb5b8bc 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1208,10 +1208,14 @@ gic_of_init(struct device_node *node, struct device_node *parent)
return -ENODEV;
dist_base = of_iomap(node, 0);
- WARN(!dist_base, "unable to map gic dist registers\n");
+ if (WARN(!dist_base, "unable to map gic dist registers\n"))
+ return -ENOMEM;
cpu_base = of_iomap(node, 1);
- WARN(!cpu_base, "unable to map gic cpu registers\n");
+ if (WARN(!cpu_base, "unable to map gic cpu registers\n")) {
+ iounmap(dist_base);
+ return -ENOMEM;
+ }
/*
* Disable split EOI/Deactivate if either HYP is not available
--
2.1.4
If the GIC initialisation fails, then currently we do not return an error
or clean-up afterwards. Although for root controllers, this failure may be
fatal anyway, for secondary controllers, it may not be fatal and so return
an error on failure and clean-up.
Also for non-banked GIC controllers, make sure that we free any memory
allocated if we fail to initialise the IRQ domain.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/irqchip/irq-gic.c | 56 +++++++++++++++++++++++++++++++++--------------
1 file changed, 40 insertions(+), 16 deletions(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 561a5cb5b8bc..5d1f1d4396c2 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1032,30 +1032,30 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
.unmap = gic_irq_domain_unmap,
};
-static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
+static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
void __iomem *dist_base, void __iomem *cpu_base,
u32 percpu_offset, struct fwnode_handle *handle)
{
irq_hw_number_t hwirq_base;
struct gic_chip_data *gic;
- int gic_irqs, irq_base, i;
+ int gic_irqs, irq_base, i, ret;
BUG_ON(gic_nr >= MAX_GIC_NR);
gic_check_cpu_features();
gic = &gic_data[gic_nr];
-#ifdef CONFIG_GIC_NON_BANKED
- if (percpu_offset) { /* Frankein-GIC without banked registers... */
+
+ if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
+ /* Frankein-GIC without banked registers... */
unsigned int cpu;
gic->dist_base.percpu_base = alloc_percpu(void __iomem *);
gic->cpu_base.percpu_base = alloc_percpu(void __iomem *);
if (WARN_ON(!gic->dist_base.percpu_base ||
!gic->cpu_base.percpu_base)) {
- free_percpu(gic->dist_base.percpu_base);
- free_percpu(gic->cpu_base.percpu_base);
- return;
+ ret = -ENOMEM;
+ goto err;
}
for_each_possible_cpu(cpu) {
@@ -1067,9 +1067,8 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
}
gic_set_base_accessor(gic, gic_get_percpu_base);
- } else
-#endif
- { /* Normal, sane GIC... */
+ } else {
+ /* Normal, sane GIC... */
WARN(percpu_offset,
"GIC_NON_BANKED not enabled, ignoring %08x offset!",
percpu_offset);
@@ -1119,8 +1118,10 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
hwirq_base, &gic_irq_domain_ops, gic);
}
- if (WARN_ON(!gic->domain))
- return;
+ if (WARN_ON(!gic->domain)) {
+ ret = -ENODEV;
+ goto err;
+ }
if (gic_nr == 0) {
/*
@@ -1142,6 +1143,16 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
gic_dist_init(gic);
gic_cpu_init(gic);
gic_pm_init(gic);
+
+ return 0;
+
+err:
+ if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
+ free_percpu(gic->dist_base.percpu_base);
+ free_percpu(gic->cpu_base.percpu_base);
+ }
+
+ return ret;
}
void __init gic_init(unsigned int gic_nr, int irq_start,
@@ -1202,7 +1213,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
void __iomem *cpu_base;
void __iomem *dist_base;
u32 percpu_offset;
- int irq;
+ int irq, ret;
if (WARN_ON(!node))
return -ENODEV;
@@ -1227,8 +1238,14 @@ gic_of_init(struct device_node *node, struct device_node *parent)
if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
percpu_offset = 0;
- __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
+ ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
&node->fwnode);
+ if (ret) {
+ iounmap(dist_base);
+ iounmap(cpu_base);
+ return ret;
+ }
+
if (!gic_cnt)
gic_init_physaddr(node);
@@ -1317,7 +1334,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
struct acpi_madt_generic_distributor *dist;
void __iomem *cpu_base, *dist_base;
struct fwnode_handle *domain_handle;
- int count;
+ int count, ret;
/* Collect CPU base addresses */
count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
@@ -1360,7 +1377,14 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
return -ENOMEM;
}
- __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle);
+ ret = __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle);
+ if (ret) {
+ pr_err("Failed to initialise GIC\n");
+ irq_domain_free_fwnode(domain_handle);
+ iounmap(cpu_base);
+ iounmap(dist_base);
+ return ret;
+ }
acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle);
return 0;
--
2.1.4
Dynamically assign the irqchip structure for each GIC controller
instance. This is necessary in order to populate the "dev" member
of the irqchip structure for GIC instances that require runtime
power management support. This also allows us to populate a unique
name for each GIC controller.
This is based upon a patch by Linus Walleij <[email protected]>.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/irqchip/irq-gic.c | 41 +++++++++++++++--------------------------
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 5d1f1d4396c2..ebfbb2379320 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -69,6 +69,7 @@ union gic_base {
};
struct gic_chip_data {
+ struct irq_chip chip;
union gic_base dist_base;
union gic_base cpu_base;
#ifdef CONFIG_CPU_PM
@@ -383,7 +384,6 @@ static void gic_handle_cascade_irq(struct irq_desc *desc)
}
static struct irq_chip gic_chip = {
- .name = "GIC",
.irq_mask = gic_mask_irq,
.irq_unmask = gic_unmask_irq,
.irq_eoi = gic_eoi_irq,
@@ -398,23 +398,6 @@ static struct irq_chip gic_chip = {
IRQCHIP_MASK_ON_SUSPEND,
};
-static struct irq_chip gic_eoimode1_chip = {
- .name = "GICv2",
- .irq_mask = gic_eoimode1_mask_irq,
- .irq_unmask = gic_unmask_irq,
- .irq_eoi = gic_eoimode1_eoi_irq,
- .irq_set_type = gic_set_type,
-#ifdef CONFIG_SMP
- .irq_set_affinity = gic_set_affinity,
-#endif
- .irq_get_irqchip_state = gic_irq_get_irqchip_state,
- .irq_set_irqchip_state = gic_irq_set_irqchip_state,
- .irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity,
- .flags = IRQCHIP_SET_TYPE_MASKED |
- IRQCHIP_SKIP_SET_WAKE |
- IRQCHIP_MASK_ON_SUSPEND,
-};
-
void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
{
if (gic_nr >= MAX_GIC_NR)
@@ -925,20 +908,15 @@ void __init gic_init_physaddr(struct device_node *node)
static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
- struct irq_chip *chip = &gic_chip;
-
- if (static_key_true(&supports_deactivate)) {
- if (d->host_data == (void *)&gic_data[0])
- chip = &gic_eoimode1_chip;
- }
+ struct gic_chip_data *gic = d->host_data;
if (hw < 32) {
irq_set_percpu_devid(irq);
- irq_domain_set_info(d, irq, hw, chip, d->host_data,
+ irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
handle_percpu_devid_irq, NULL, NULL);
irq_set_status_flags(irq, IRQ_NOAUTOEN);
} else {
- irq_domain_set_info(d, irq, hw, chip, d->host_data,
+ irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
handle_fasteoi_irq, NULL, NULL);
irq_set_probe(irq);
}
@@ -1046,6 +1024,15 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
gic = &gic_data[gic_nr];
+ gic->chip = gic_chip;
+ gic->chip.name = kasprintf(GFP_KERNEL, "GIC%d", gic_nr);
+
+ if (gic_nr == 0 && static_key_true(&supports_deactivate)) {
+ gic->chip.irq_mask = gic_eoimode1_mask_irq;
+ gic->chip.irq_eoi = gic_eoimode1_eoi_irq;
+ gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity;
+ }
+
if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
/* Frankein-GIC without banked registers... */
unsigned int cpu;
@@ -1147,6 +1134,8 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
return 0;
err:
+ kfree(gic->chip.name);
+
if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
free_percpu(gic->dist_base.percpu_base);
free_percpu(gic->cpu_base.percpu_base);
--
2.1.4
To support GIC chips located in power-domains outside of the CPU subsystem
it is necessary to add a platform driver for these chips, so that the
probing of the chip can be deferred if the power-domain has not yet been
registered with the generic power-domain infrastructure. Before adding a
platform driver for these chips, it is first necessary to move the function
__gic_init_bases() from the __init section so that it is always present
and not removed so it can be used by the platform driver.
The platform driver will only support non-root interrupt controllers
because the root interrupt controller needs to be initialised early. To
avoid section mis-matches it is also necessary to move function calls for
set_smp_cross_call() and set_handle_irq() from __gic_init_bases() (which
are only required for root controllers) because these are also located in
the __init section. Therefore, add the function __gic_init_root() which
will handle root controller specific initialisation and internally call
__gic_init_bases().
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/irqchip/irq-gic-common.c | 4 +-
drivers/irqchip/irq-gic.c | 94 +++++++++++++++++++++++++---------------
2 files changed, 62 insertions(+), 36 deletions(-)
diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index f174ce0ca361..f8ccb4beaeb5 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -65,8 +65,8 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
return ret;
}
-void __init gic_dist_config(void __iomem *base, int gic_irqs,
- void (*sync_access)(void))
+void gic_dist_config(void __iomem *base, int gic_irqs,
+ void (*sync_access)(void))
{
unsigned int i;
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index ebfbb2379320..db3a46e40142 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -717,7 +717,7 @@ static struct notifier_block gic_notifier_block = {
.notifier_call = gic_notifier,
};
-static void __init gic_pm_init(struct gic_chip_data *gic)
+static void gic_pm_init(struct gic_chip_data *gic)
{
gic->saved_ppi_enable = __alloc_percpu(DIV_ROUND_UP(32, 32) * 4,
sizeof(u32));
@@ -735,7 +735,7 @@ static void __init gic_pm_init(struct gic_chip_data *gic)
cpu_pm_register_notifier(&gic_notifier_block);
}
#else
-static void __init gic_pm_init(struct gic_chip_data *gic)
+static void gic_pm_init(struct gic_chip_data *gic)
{
}
#endif
@@ -1010,13 +1010,13 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
.unmap = gic_irq_domain_unmap,
};
-static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
- void __iomem *dist_base, void __iomem *cpu_base,
- u32 percpu_offset, struct fwnode_handle *handle)
+static int __gic_init_bases(unsigned int gic_nr, int irq_start,
+ void __iomem *dist_base, void __iomem *cpu_base,
+ u32 percpu_offset, struct fwnode_handle *handle)
{
irq_hw_number_t hwirq_base;
struct gic_chip_data *gic;
- int gic_irqs, irq_base, i, ret;
+ int gic_irqs, irq_base, ret;
BUG_ON(gic_nr >= MAX_GIC_NR);
@@ -1110,23 +1110,6 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
goto err;
}
- if (gic_nr == 0) {
- /*
- * Initialize the CPU interface map to all CPUs.
- * It will be refined as each CPU probes its ID.
- * This is only necessary for the primary GIC.
- */
- for (i = 0; i < NR_GIC_CPU_IF; i++)
- gic_cpu_map[i] = 0xff;
-#ifdef CONFIG_SMP
- set_smp_cross_call(gic_raise_softirq);
- register_cpu_notifier(&gic_cpu_notifier);
-#endif
- set_handle_irq(gic_handle_irq);
- if (static_key_true(&supports_deactivate))
- pr_info("GIC: Using split EOI/Deactivate mode\n");
- }
-
gic_dist_init(gic);
gic_cpu_init(gic);
gic_pm_init(gic);
@@ -1144,6 +1127,38 @@ err:
return ret;
}
+static int __init __gic_init_root(int irq_start, void __iomem *dist_base,
+ void __iomem *cpu_base, u32 percpu_offset,
+ struct fwnode_handle *handle)
+{
+ int i, ret;
+
+ /*
+ * Initialize the CPU interface map to all CPUs.
+ * It will be refined as each CPU probes its ID.
+ * This is only necessary for the primary GIC.
+ */
+ for (i = 0; i < NR_GIC_CPU_IF; i++)
+ gic_cpu_map[i] = 0xff;
+
+ ret = __gic_init_bases(0, irq_start, dist_base, cpu_base,
+ percpu_offset, handle);
+ if (ret)
+ return ret;
+
+ if (IS_ENABLED(CONFIG_SMP)) {
+ set_smp_cross_call(gic_raise_softirq);
+ register_cpu_notifier(&gic_cpu_notifier);
+ }
+
+ set_handle_irq(gic_handle_irq);
+
+ if (static_key_true(&supports_deactivate))
+ pr_info("GIC: Using split EOI/Deactivate mode\n");
+
+ return 0;
+}
+
void __init gic_init(unsigned int gic_nr, int irq_start,
void __iomem *dist_base, void __iomem *cpu_base)
{
@@ -1152,7 +1167,12 @@ void __init gic_init(unsigned int gic_nr, int irq_start,
* bother with these...
*/
static_key_slow_dec(&supports_deactivate);
- __gic_init_bases(gic_nr, irq_start, dist_base, cpu_base, 0, NULL);
+
+ if (!gic_nr)
+ __gic_init_root(irq_start, dist_base, cpu_base, 0, NULL);
+ else
+ __gic_init_bases(gic_nr, irq_start, dist_base, cpu_base, 0,
+ NULL);
}
#ifdef CONFIG_OF
@@ -1217,18 +1237,24 @@ gic_of_init(struct device_node *node, struct device_node *parent)
return -ENOMEM;
}
- /*
- * Disable split EOI/Deactivate if either HYP is not available
- * or the CPU interface is too small.
- */
- if (gic_cnt == 0 && !gic_check_eoimode(node, &cpu_base))
- static_key_slow_dec(&supports_deactivate);
-
if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
percpu_offset = 0;
- ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
- &node->fwnode);
+ if (!gic_cnt) {
+ /*
+ * Disable split EOI/Deactivate if either HYP is not available
+ * or the CPU interface is too small.
+ */
+ if (!gic_check_eoimode(node, &cpu_base))
+ static_key_slow_dec(&supports_deactivate);
+
+ ret = __gic_init_root(-1, dist_base, cpu_base, percpu_offset,
+ &node->fwnode);
+ } else {
+ ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base,
+ percpu_offset, &node->fwnode);
+ }
+
if (ret) {
iounmap(dist_base);
iounmap(cpu_base);
@@ -1366,7 +1392,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
return -ENOMEM;
}
- ret = __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle);
+ ret = __gic_init_root(-1, dist_base, cpu_base, 0, domain_handle);
if (ret) {
pr_err("Failed to initialise GIC\n");
irq_domain_free_fwnode(domain_handle);
--
2.1.4
Add a driver for the Tegra-AGIC interrupt controller which is compatible
with the ARM GIC-400 interrupt controller.
The Tegra AGIC (Audio GIC) is part of the Audio Processing Engine (APE) on
Tegra210 and can route interrupts to either the GIC for the CPU subsystem
or the Audio DSP (ADSP) within the APE. The AGIC uses CPU interface 0 to
route interrupts to the CPU GIC and CPU interface 1 to route interrupts to
the ADSP.
The APE is located within its own power domain on the chip and so the
AGIC needs to manage both the power domain and its clocks. Commit
afbbd2338176 ("irqchip/gic: Document optional Clock and Power Domain
properties") adding clock and power-domain properties to the GIC binding
and so the aim would be to make use of these to handle power management
(however, this is very much dependent upon adding support for generic
PM domains for Tegra which is still a work-in-progress).
With the AGIC being located in a different power domain to the main CPU
cluster this means that:
1. The interrupt controller cannot be registered via IRQCHIP_DECLARE()
because it needs to be registered as a platform device so that the
generic PM domain core will ensure that the power domain is available
before probing.
2. The interrupt controller cannot be suspended/restored based upon
changes in the CPU power state and needs to use runtime-pm instead.
This is very much a work-in-progress and there are still a few items that
need to be resolved. These items are:
1. Currently the GIC platform driver only supports non-root GICs. The
platform driver performs a save and restore of PPI interrupts for
non-root GICs, which is probably not necessary and so could be changed.
At a minimum we need to re-enable the CPU interface during the device
resume but we could skip the restoration of the PPIs. In general we
could update the driver to only save and restore PPIs for the root
controller, if that makes sense.
2. Currently routing of interrupts to the ADSP for Tegra210 is not
supported by this driver. Although the ADSP on Tegra210 could also setup
the AGIC distributor having two independent subsystems configure the
distributor does not seem like a good idea. Given that the ADSP is a
slave and would be under the control of the kernel via its own driver,
it would seem best that only the kernel configures the distributors
routing of the interrupts. This could be achieved by adding a new genirq
API to migrate the interrupt. The GIC driver already has an API to
migrate all interrupts from one CPU interface to another (which I
understand is for a different reason), but having an generic API to
migrate an interrupt to another device could be useful (unless something
already exists that I have overlooked).
Please let me know if you have any thoughts/opinions on the above.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/irqchip/irq-gic.c | 330 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 253 insertions(+), 77 deletions(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index db3a46e40142..978edb74e7ad 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -26,6 +26,7 @@
#include <linux/module.h>
#include <linux/list.h>
#include <linux/smp.h>
+#include <linux/clk.h>
#include <linux/cpu.h>
#include <linux/cpu_pm.h>
#include <linux/cpumask.h>
@@ -37,6 +38,8 @@
#include <linux/irqdomain.h>
#include <linux/interrupt.h>
#include <linux/percpu.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include <linux/irqchip.h>
#include <linux/irqchip/chained_irq.h>
@@ -70,9 +73,9 @@ union gic_base {
struct gic_chip_data {
struct irq_chip chip;
+ struct clk *clk;
union gic_base dist_base;
union gic_base cpu_base;
-#ifdef CONFIG_CPU_PM
u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
u32 saved_spi_active[DIV_ROUND_UP(1020, 32)];
u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
@@ -80,7 +83,6 @@ struct gic_chip_data {
u32 __percpu *saved_ppi_enable;
u32 __percpu *saved_ppi_active;
u32 __percpu *saved_ppi_conf;
-#endif
struct irq_domain *domain;
unsigned int gic_irqs;
#ifdef CONFIG_GIC_NON_BANKED
@@ -444,7 +446,7 @@ static void gic_cpu_if_up(struct gic_chip_data *gic)
}
-static void __init gic_dist_init(struct gic_chip_data *gic)
+static void gic_dist_init(struct gic_chip_data *gic)
{
unsigned int i;
u32 cpumask;
@@ -518,42 +520,41 @@ int gic_cpu_if_down(unsigned int gic_nr)
return 0;
}
-#ifdef CONFIG_CPU_PM
/*
* Saves the GIC distributor registers during suspend or idle. Must be called
* with interrupts disabled but before powering down the GIC. After calling
* this function, no interrupts will be delivered by the GIC, and another
* platform-specific wakeup source must be enabled.
*/
-static void gic_dist_save(unsigned int gic_nr)
+static void gic_dist_save(struct gic_chip_data *gic)
{
unsigned int gic_irqs;
void __iomem *dist_base;
int i;
- if (gic_nr >= MAX_GIC_NR)
- BUG();
+ if (WARN_ON(!gic))
+ return;
- gic_irqs = gic_data[gic_nr].gic_irqs;
- dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+ gic_irqs = gic->gic_irqs;
+ dist_base = gic_data_dist_base(gic);
if (!dist_base)
return;
for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++)
- gic_data[gic_nr].saved_spi_conf[i] =
+ gic->saved_spi_conf[i] =
readl_relaxed(dist_base + GIC_DIST_CONFIG + i * 4);
for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
- gic_data[gic_nr].saved_spi_target[i] =
+ gic->saved_spi_target[i] =
readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
- gic_data[gic_nr].saved_spi_enable[i] =
+ gic->saved_spi_enable[i] =
readl_relaxed(dist_base + GIC_DIST_ENABLE_SET + i * 4);
for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
- gic_data[gic_nr].saved_spi_active[i] =
+ gic->saved_spi_active[i] =
readl_relaxed(dist_base + GIC_DIST_ACTIVE_SET + i * 4);
}
@@ -564,17 +565,17 @@ static void gic_dist_save(unsigned int gic_nr)
* handled normally, but any edge interrupts that occured will not be seen by
* the GIC and need to be handled by the platform-specific wakeup source.
*/
-static void gic_dist_restore(unsigned int gic_nr)
+static void gic_dist_restore(struct gic_chip_data *gic)
{
unsigned int gic_irqs;
unsigned int i;
void __iomem *dist_base;
- if (gic_nr >= MAX_GIC_NR)
- BUG();
+ if (WARN_ON(!gic))
+ return;
- gic_irqs = gic_data[gic_nr].gic_irqs;
- dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+ gic_irqs = gic->gic_irqs;
+ dist_base = gic_data_dist_base(gic);
if (!dist_base)
return;
@@ -582,7 +583,7 @@ static void gic_dist_restore(unsigned int gic_nr)
writel_relaxed(GICD_DISABLE, dist_base + GIC_DIST_CTRL);
for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++)
- writel_relaxed(gic_data[gic_nr].saved_spi_conf[i],
+ writel_relaxed(gic->saved_spi_conf[i],
dist_base + GIC_DIST_CONFIG + i * 4);
for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
@@ -590,87 +591,87 @@ static void gic_dist_restore(unsigned int gic_nr)
dist_base + GIC_DIST_PRI + i * 4);
for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
- writel_relaxed(gic_data[gic_nr].saved_spi_target[i],
+ writel_relaxed(gic->saved_spi_target[i],
dist_base + GIC_DIST_TARGET + i * 4);
for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++) {
writel_relaxed(GICD_INT_EN_CLR_X32,
dist_base + GIC_DIST_ENABLE_CLEAR + i * 4);
- writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
+ writel_relaxed(gic->saved_spi_enable[i],
dist_base + GIC_DIST_ENABLE_SET + i * 4);
}
for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++) {
writel_relaxed(GICD_INT_EN_CLR_X32,
dist_base + GIC_DIST_ACTIVE_CLEAR + i * 4);
- writel_relaxed(gic_data[gic_nr].saved_spi_active[i],
+ writel_relaxed(gic->saved_spi_active[i],
dist_base + GIC_DIST_ACTIVE_SET + i * 4);
}
writel_relaxed(GICD_ENABLE, dist_base + GIC_DIST_CTRL);
}
-static void gic_cpu_save(unsigned int gic_nr)
+static void gic_cpu_save(struct gic_chip_data *gic)
{
int i;
u32 *ptr;
void __iomem *dist_base;
void __iomem *cpu_base;
- if (gic_nr >= MAX_GIC_NR)
- BUG();
+ if (WARN_ON(!gic))
+ return;
- dist_base = gic_data_dist_base(&gic_data[gic_nr]);
- cpu_base = gic_data_cpu_base(&gic_data[gic_nr]);
+ dist_base = gic_data_dist_base(gic);
+ cpu_base = gic_data_cpu_base(gic);
if (!dist_base || !cpu_base)
return;
- ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_enable);
+ ptr = raw_cpu_ptr(gic->saved_ppi_enable);
for (i = 0; i < DIV_ROUND_UP(32, 32); i++)
ptr[i] = readl_relaxed(dist_base + GIC_DIST_ENABLE_SET + i * 4);
- ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_active);
+ ptr = raw_cpu_ptr(gic->saved_ppi_active);
for (i = 0; i < DIV_ROUND_UP(32, 32); i++)
ptr[i] = readl_relaxed(dist_base + GIC_DIST_ACTIVE_SET + i * 4);
- ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_conf);
+ ptr = raw_cpu_ptr(gic->saved_ppi_conf);
for (i = 0; i < DIV_ROUND_UP(32, 16); i++)
ptr[i] = readl_relaxed(dist_base + GIC_DIST_CONFIG + i * 4);
}
-static void gic_cpu_restore(unsigned int gic_nr)
+static void gic_cpu_restore(struct gic_chip_data *gic)
{
int i;
u32 *ptr;
void __iomem *dist_base;
void __iomem *cpu_base;
- if (gic_nr >= MAX_GIC_NR)
- BUG();
+ if (WARN_ON(!gic))
+ return;
- dist_base = gic_data_dist_base(&gic_data[gic_nr]);
- cpu_base = gic_data_cpu_base(&gic_data[gic_nr]);
+ dist_base = gic_data_dist_base(gic);
+ cpu_base = gic_data_cpu_base(gic);
if (!dist_base || !cpu_base)
return;
- ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_enable);
+ ptr = raw_cpu_ptr(gic->saved_ppi_enable);
for (i = 0; i < DIV_ROUND_UP(32, 32); i++) {
writel_relaxed(GICD_INT_EN_CLR_X32,
dist_base + GIC_DIST_ENABLE_CLEAR + i * 4);
writel_relaxed(ptr[i], dist_base + GIC_DIST_ENABLE_SET + i * 4);
}
- ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_active);
+ ptr = raw_cpu_ptr(gic->saved_ppi_active);
for (i = 0; i < DIV_ROUND_UP(32, 32); i++) {
writel_relaxed(GICD_INT_EN_CLR_X32,
dist_base + GIC_DIST_ACTIVE_CLEAR + i * 4);
writel_relaxed(ptr[i], dist_base + GIC_DIST_ACTIVE_SET + i * 4);
}
- ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_conf);
+ ptr = raw_cpu_ptr(gic->saved_ppi_conf);
for (i = 0; i < DIV_ROUND_UP(32, 16); i++)
writel_relaxed(ptr[i], dist_base + GIC_DIST_CONFIG + i * 4);
@@ -679,7 +680,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
dist_base + GIC_DIST_PRI + i * 4);
writel_relaxed(GICC_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
- gic_cpu_if_up(&gic_data[gic_nr]);
+ gic_cpu_if_up(gic);
}
static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v)
@@ -694,18 +695,18 @@ static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v)
#endif
switch (cmd) {
case CPU_PM_ENTER:
- gic_cpu_save(i);
+ gic_cpu_save(&gic_data[i]);
break;
case CPU_PM_ENTER_FAILED:
case CPU_PM_EXIT:
- gic_cpu_restore(i);
+ gic_cpu_restore(&gic_data[i]);
break;
case CPU_CLUSTER_PM_ENTER:
- gic_dist_save(i);
+ gic_dist_save(&gic_data[i]);
break;
case CPU_CLUSTER_PM_ENTER_FAILED:
case CPU_CLUSTER_PM_EXIT:
- gic_dist_restore(i);
+ gic_dist_restore(&gic_data[i]);
break;
}
}
@@ -734,11 +735,6 @@ static void gic_pm_init(struct gic_chip_data *gic)
if (gic == &gic_data[0])
cpu_pm_register_notifier(&gic_notifier_block);
}
-#else
-static void gic_pm_init(struct gic_chip_data *gic)
-{
-}
-#endif
#ifdef CONFIG_SMP
static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
@@ -1010,24 +1006,23 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
.unmap = gic_irq_domain_unmap,
};
-static int __gic_init_bases(unsigned int gic_nr, int irq_start,
+static int __gic_init_bases(struct gic_chip_data *gic, int irq_start,
void __iomem *dist_base, void __iomem *cpu_base,
- u32 percpu_offset, struct fwnode_handle *handle)
+ u32 percpu_offset, struct fwnode_handle *handle,
+ const char *name)
{
irq_hw_number_t hwirq_base;
- struct gic_chip_data *gic;
int gic_irqs, irq_base, ret;
- BUG_ON(gic_nr >= MAX_GIC_NR);
+ if (WARN_ON(!gic || gic->domain))
+ return -EINVAL;
gic_check_cpu_features();
- gic = &gic_data[gic_nr];
-
gic->chip = gic_chip;
- gic->chip.name = kasprintf(GFP_KERNEL, "GIC%d", gic_nr);
+ gic->chip.name = name;
- if (gic_nr == 0 && static_key_true(&supports_deactivate)) {
+ if (gic == &gic_data[0] && static_key_true(&supports_deactivate)) {
gic->chip.irq_mask = gic_eoimode1_mask_irq;
gic->chip.irq_eoi = gic_eoimode1_eoi_irq;
gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity;
@@ -1083,7 +1078,7 @@ static int __gic_init_bases(unsigned int gic_nr, int irq_start,
* For primary GICs, skip over SGIs.
* For secondary GICs, skip over PPIs, too.
*/
- if (gic_nr == 0 && (irq_start & 31) > 0) {
+ if (gic == &gic_data[0] && (irq_start & 31) > 0) {
hwirq_base = 16;
if (irq_start != -1)
irq_start = (irq_start & ~31) + 16;
@@ -1132,6 +1127,9 @@ static int __init __gic_init_root(int irq_start, void __iomem *dist_base,
struct fwnode_handle *handle)
{
int i, ret;
+ char *name;
+
+ name = kasprintf(GFP_KERNEL, "GIC0");
/*
* Initialize the CPU interface map to all CPUs.
@@ -1141,10 +1139,12 @@ static int __init __gic_init_root(int irq_start, void __iomem *dist_base,
for (i = 0; i < NR_GIC_CPU_IF; i++)
gic_cpu_map[i] = 0xff;
- ret = __gic_init_bases(0, irq_start, dist_base, cpu_base,
- percpu_offset, handle);
- if (ret)
+ ret = __gic_init_bases(&gic_data[0], irq_start, dist_base, cpu_base,
+ percpu_offset, handle, name);
+ if (ret) {
+ kfree(name);
return ret;
+ }
if (IS_ENABLED(CONFIG_SMP)) {
set_smp_cross_call(gic_raise_softirq);
@@ -1162,17 +1162,26 @@ static int __init __gic_init_root(int irq_start, void __iomem *dist_base,
void __init gic_init(unsigned int gic_nr, int irq_start,
void __iomem *dist_base, void __iomem *cpu_base)
{
+ char *name;
+
+ if (WARN_ON(gic_nr >= MAX_GIC_NR))
+ return;
+
/*
* Non-DT/ACPI systems won't run a hypervisor, so let's not
* bother with these...
*/
static_key_slow_dec(&supports_deactivate);
- if (!gic_nr)
+ if (!gic_nr) {
__gic_init_root(irq_start, dist_base, cpu_base, 0, NULL);
- else
- __gic_init_bases(gic_nr, irq_start, dist_base, cpu_base, 0,
- NULL);
+ } else {
+ name = kasprintf(GFP_KERNEL, "GIC%d", gic_nr);
+
+ if (__gic_init_bases(&gic_data[gic_nr], irq_start, dist_base,
+ cpu_base, 0, NULL, name))
+ kfree(name);
+ }
}
#ifdef CONFIG_OF
@@ -1216,6 +1225,28 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
return true;
}
+static int gic_of_setup(struct device_node *node, void __iomem **dist_base,
+ void __iomem **cpu_base, u32 *percpu_offset)
+{
+ if (!node)
+ return -EINVAL;
+
+ *dist_base = of_iomap(node, 0);
+ if (WARN(!*dist_base, "unable to map gic dist registers\n"))
+ return -ENOMEM;
+
+ *cpu_base = of_iomap(node, 1);
+ if (WARN(!*cpu_base, "unable to map gic cpu registers\n")) {
+ iounmap(*dist_base);
+ return -ENOMEM;
+ }
+
+ if (of_property_read_u32(node, "cpu-offset", percpu_offset))
+ *percpu_offset = 0;
+
+ return 0;
+}
+
static int __init
gic_of_init(struct device_node *node, struct device_node *parent)
{
@@ -1227,18 +1258,12 @@ gic_of_init(struct device_node *node, struct device_node *parent)
if (WARN_ON(!node))
return -ENODEV;
- dist_base = of_iomap(node, 0);
- if (WARN(!dist_base, "unable to map gic dist registers\n"))
- return -ENOMEM;
-
- cpu_base = of_iomap(node, 1);
- if (WARN(!cpu_base, "unable to map gic cpu registers\n")) {
- iounmap(dist_base);
- return -ENOMEM;
- }
+ if (WARN_ON(gic_cnt >= MAX_GIC_NR))
+ return -EINVAL;
- if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
- percpu_offset = 0;
+ ret = gic_of_setup(node, &dist_base, &cpu_base, &percpu_offset);
+ if (ret)
+ return ret;
if (!gic_cnt) {
/*
@@ -1251,8 +1276,9 @@ gic_of_init(struct device_node *node, struct device_node *parent)
ret = __gic_init_root(-1, dist_base, cpu_base, percpu_offset,
&node->fwnode);
} else {
- ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base,
- percpu_offset, &node->fwnode);
+ ret = __gic_init_bases(&gic_data[gic_cnt], -1, dist_base,
+ cpu_base, percpu_offset, &node->fwnode,
+ node->name);
}
if (ret) {
@@ -1285,6 +1311,156 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init);
IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
+static int gic_runtime_resume(struct device *dev)
+{
+ struct gic_chip_data *gic = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(gic->clk);
+ if (ret)
+ return ret;
+
+ gic_dist_restore(gic);
+ gic_cpu_restore(gic);
+
+ return 0;
+}
+
+static int gic_runtime_suspend(struct device *dev)
+{
+ struct gic_chip_data *gic = dev_get_drvdata(dev);
+
+ gic_dist_save(gic);
+ gic_cpu_save(gic);
+
+ clk_disable_unprepare(gic->clk);
+
+ return 0;
+}
+
+static int gic_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct gic_chip_data *gic;
+ void __iomem *dist_base;
+ void __iomem *cpu_base;
+ u32 percpu_offset;
+ int ret, irq;
+
+ if (dev->of_node == NULL)
+ return -EINVAL;
+
+ gic = devm_kzalloc(dev, sizeof(*gic), GFP_KERNEL);
+ if (!gic)
+ return -ENOMEM;
+
+ gic->clk = of_clk_get(dev->of_node, 0);
+ if (IS_ERR(gic->clk)) {
+ dev_err(dev, "clock not found\n");
+ return PTR_ERR(gic->clk);
+ }
+
+ platform_set_drvdata(pdev, gic);
+
+ pm_runtime_enable(dev);
+ if (pm_runtime_enabled(dev))
+ ret = pm_runtime_get_sync(dev);
+ else
+ ret = gic_runtime_resume(dev);
+
+ if (ret < 0) {
+ pm_runtime_disable(dev);
+ goto err_rpm;
+ }
+
+ irq = irq_of_parse_and_map(dev->of_node, 0);
+ if (!irq) {
+ ret = -EINVAL;
+ goto err_irq;
+ }
+
+ ret = gic_of_setup(dev->of_node, &dist_base, &cpu_base, &percpu_offset);
+ if (ret)
+ goto err_map;
+
+ ret = __gic_init_bases(gic, -1, dist_base, cpu_base,
+ percpu_offset, &dev->of_node->fwnode,
+ dev->of_node->name);
+ if (ret)
+ goto err_gic;
+
+ gic->chip.dev = dev;
+ gic->chip.flags |= IRQCHIP_HAS_RPM;
+
+ irq_set_chained_handler_and_data(irq, gic_handle_cascade_irq, gic);
+
+ pm_runtime_put(dev);
+
+ dev_info(dev, "GIC IRQ controller registered\n");
+
+ return 0;
+
+err_gic:
+ iounmap(dist_base);
+ iounmap(cpu_base);
+err_map:
+ irq_dispose_mapping(irq);
+err_irq:
+ pm_runtime_disable(dev);
+ if (!pm_runtime_status_suspended(dev))
+ gic_runtime_suspend(dev);
+err_rpm:
+ clk_put(gic->clk);
+
+ return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int gic_resume(struct device *dev)
+{
+ int ret;
+
+ ret = gic_runtime_resume(dev);
+ if (ret < 0)
+ return ret;
+
+ pm_runtime_enable(dev);
+
+ return 0;
+}
+
+static int gic_suspend(struct device *dev)
+{
+ pm_runtime_disable(dev);
+ if (!pm_runtime_status_suspended(dev))
+ return gic_runtime_suspend(dev);
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops gic_pm_ops = {
+ SET_RUNTIME_PM_OPS(gic_runtime_suspend,
+ gic_runtime_resume, NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
+};
+
+static const struct of_device_id gic_match[] = {
+ { .compatible = "nvidia,tegra210-agic", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, gic_match);
+
+static struct platform_driver gic_driver = {
+ .probe = gic_probe,
+ .driver = {
+ .name = "gic",
+ .of_match_table = gic_match,
+ .pm = &gic_pm_ops,
+ }
+};
+
+builtin_platform_driver(gic_driver);
#endif
#ifdef CONFIG_ACPI
--
2.1.4
On 17/12/15 10:48, Jon Hunter wrote:
> Add a driver for the Tegra-AGIC interrupt controller which is compatible
> with the ARM GIC-400 interrupt controller.
>
> The Tegra AGIC (Audio GIC) is part of the Audio Processing Engine (APE) on
> Tegra210 and can route interrupts to either the GIC for the CPU subsystem
> or the Audio DSP (ADSP) within the APE. The AGIC uses CPU interface 0 to
> route interrupts to the CPU GIC and CPU interface 1 to route interrupts to
> the ADSP.
>
> The APE is located within its own power domain on the chip and so the
> AGIC needs to manage both the power domain and its clocks. Commit
> afbbd2338176 ("irqchip/gic: Document optional Clock and Power Domain
> properties") adding clock and power-domain properties to the GIC binding
> and so the aim would be to make use of these to handle power management
> (however, this is very much dependent upon adding support for generic
> PM domains for Tegra which is still a work-in-progress).
>
> With the AGIC being located in a different power domain to the main CPU
> cluster this means that:
> 1. The interrupt controller cannot be registered via IRQCHIP_DECLARE()
> because it needs to be registered as a platform device so that the
> generic PM domain core will ensure that the power domain is available
> before probing.
> 2. The interrupt controller cannot be suspended/restored based upon
> changes in the CPU power state and needs to use runtime-pm instead.
>
> This is very much a work-in-progress and there are still a few items that
> need to be resolved. These items are:
> 1. Currently the GIC platform driver only supports non-root GICs. The
> platform driver performs a save and restore of PPI interrupts for
> non-root GICs, which is probably not necessary and so could be changed.
> At a minimum we need to re-enable the CPU interface during the device
> resume but we could skip the restoration of the PPIs. In general we
> could update the driver to only save and restore PPIs for the root
> controller, if that makes sense.
> 2. Currently routing of interrupts to the ADSP for Tegra210 is not
> supported by this driver. Although the ADSP on Tegra210 could also setup
> the AGIC distributor having two independent subsystems configure the
> distributor does not seem like a good idea. Given that the ADSP is a
> slave and would be under the control of the kernel via its own driver,
> it would seem best that only the kernel configures the distributors
> routing of the interrupts. This could be achieved by adding a new genirq
> API to migrate the interrupt. The GIC driver already has an API to
> migrate all interrupts from one CPU interface to another (which I
> understand is for a different reason), but having an generic API to
> migrate an interrupt to another device could be useful (unless something
> already exists that I have overlooked).
>
> Please let me know if you have any thoughts/opinions on the above.
>
> Signed-off-by: Jon Hunter <[email protected]>
> ---
> drivers/irqchip/irq-gic.c | 330 +++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 253 insertions(+), 77 deletions(-)
[snip]
> +#ifdef CONFIG_PM_SLEEP
> +static int gic_resume(struct device *dev)
> +{
> + int ret;
> +
> + ret = gic_runtime_resume(dev);
> + if (ret < 0)
> + return ret;
> +
> + pm_runtime_enable(dev);
> +
> + return 0;
> +}
> +
> +static int gic_suspend(struct device *dev)
> +{
> + pm_runtime_disable(dev);
> + if (!pm_runtime_status_suspended(dev))
> + return gic_runtime_suspend(dev);
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops gic_pm_ops = {
> + SET_RUNTIME_PM_OPS(gic_runtime_suspend,
> + gic_runtime_resume, NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
> +};
I believe these need to be the noirq variants. Will fix.
Jon
Hi Jon,
On 17/12/15 10:48, Jon Hunter wrote:
> Dynamically assign the irqchip structure for each GIC controller
> instance. This is necessary in order to populate the "dev" member
> of the irqchip structure for GIC instances that require runtime
> power management support. This also allows us to populate a unique
> name for each GIC controller.
>
> This is based upon a patch by Linus Walleij <[email protected]>.
>
> Signed-off-by: Jon Hunter <[email protected]>
This is going to clash horribly with the version I've pulled into my tree:
https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/gic-4.5&id=58b8964990dc6b59198b25337624b8518cb1dd87
so you may want to have a look at this. I don't think it will affect the
work you are doing in a major may though.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Adding linux-gpio, so quoting in full.
On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
> Some IRQ chips, such as GPIO controllers or secondary level interrupt
> controllers, may require require additional runtime power management
> control to ensure they are accessible. For such IRQ chips, it makes sense
> to enable the IRQ chip when interrupts are requested and disabled them
> again once all interrupts have been freed.
>
> When mapping an IRQ, the IRQ type settings are read and then programmed.
> The mapping of the IRQ happens before the IRQ is requested and so the
> programming of the type settings occurs before the IRQ is requested. This
> is a problem for IRQ chips that require additional power management
> control because they may not be accessible yet. Therefore, when mapping
> the IRQ, don't program the type settings, just save them and then program
> these saved settings when the IRQ is requested (so long as if they are not
> overridden via the call to request the IRQ).
>
> Signed-off-by: Jon Hunter <[email protected]>
> ---
> kernel/irq/irqdomain.c | 18 ++++++++++++++----
> kernel/irq/manage.c | 7 +++++++
> 2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index eae31e978ab2..4322d6fd0b8f 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -570,6 +570,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> {
> struct device_node *of_node;
> struct irq_domain *domain;
> + struct irq_data *irq_data;
> irq_hw_number_t hwirq;
> unsigned int cur_type = IRQ_TYPE_NONE;
> unsigned int type = IRQ_TYPE_NONE;
> @@ -634,10 +635,19 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> return 0;
> }
>
> - /* Set type if specified and different than the current one */
> - if (type != IRQ_TYPE_NONE &&
> - type != irq_get_trigger_type(virq))
> - irq_set_irq_type(virq, type);
> + irq_data = irq_get_irq_data(virq);
> + if (!irq_data) {
> + if (irq_domain_is_hierarchy(domain))
> + irq_domain_free_irqs(virq, 1);
> + else
> + irq_dispose_mapping(virq);
> +
> + return 0;
> + }
> +
> + /* Store trigger type */
> + irqd_set_trigger_type(irq_data, type);
> +
> return virq;
> }
> EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 0eebaeef317b..2a429b061171 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1119,6 +1119,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> new->irq = irq;
>
> /*
> + * If the trigger type is not specified by the caller,
> + * then use the default for this interrupt.
> + */
> + if (!(new->flags & IRQF_TRIGGER_MASK))
> + new->flags |= irqd_get_trigger_type(&desc->irq_data);
> +
> + /*
> * Check whether the interrupt nests into another interrupt
> * thread.
> */
This patch looks correct to me. It will have semantic effects though
so we need to be on the lookout for drivers that start to behave strange
as an effect of this, i.e. if they were counting on their .set_type()
to be called during domain setup.
If anyone is aware of such drivers amonst the GPIO chips, please
point them out to me.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
> When mapping an IRQ, if a mapping already exists, then we simply return
> the virual IRQ number. However, we do not check that the type settings for
^virtual
Just that it isn't virtual, it's a Linux IRQ number, we actually use
hwirq for the non-virtual IRQ number/offse in this function.
But I know I may be fighting weathermills here.
> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> {
> + struct device_node *of_node;
> struct irq_domain *domain;
> irq_hw_number_t hwirq;
> + unsigned int cur_type = IRQ_TYPE_NONE;
> unsigned int type = IRQ_TYPE_NONE;
> int virq;
>
> @@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> if (irq_domain_translate(domain, fwspec, &hwirq, &type))
> return 0;
>
> - if (irq_domain_is_hierarchy(domain)) {
> - /*
> - * If we've already configured this interrupt,
> - * don't do it again, or hell will break loose.
> - */
> - virq = irq_find_mapping(domain, hwirq);
> - if (virq)
> - return virq;
> + of_node = irq_domain_get_of_node(domain);
Marc's patches went to great lengths to do this fwspec-neutral,
i.e. it doesn't matter if it's done by DT or ACPI (or whatever).
This just drives a truck through all of that by making
the whole function OF-specific again.
>
> - virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> - if (virq <= 0)
> - return 0;
> + /*
> + * If we've already configured this interrupt,
> + * don't do it again, or hell will break loose.
> + */
> + virq = irq_find_mapping(domain, hwirq);
> + if (!virq) {
> + if (irq_domain_is_hierarchy(domain)) {
> + virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE,
> + fwspec);
> + if (virq <= 0)
> + return 0;
> + } else {
> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
> + of_node_to_nid(of_node));
What is this all of a sudden? Not even mentioned in the
commit. Plus I bet ACPI need something else than OF nid
passed here.
Yours,
Linus Walleij
On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
(Adding Rafael and linux-pm to To: list)
> Some IRQ chips may be located in a power domain outside of the CPU
> subsystem and hence will require device specific runtime power management.
> In order to support such IRQ chips, add a pointer for a device structure
> to the irq_chip structure, and if this pointer is populated by the IRQ
> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
> APIs for this chip will be called when an IRQ is requested/freed,
> respectively.
>
> Signed-off-by: Jon Hunter <[email protected]>
Overall I like what you're trying to do. This will enable e.g. I2C
GPIO supplying expanders to power down if none of its lines are
used for IRQs. (Read below on the suspend() case for even
better stuff we can do!)
(...)
> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> /**
> * struct irq_chip - hardware interrupt chip descriptor
> *
> + * @dev: pointer to associated device
(...)
> struct irq_chip {
> + struct device *dev;
In struct gpio_chip I just this merge window have to merge a gigantic
patch renaming this from "dev" to "parent" because we need to add
a *real* struct device dev; to gpio_chip.
So for the advent that we may in the future need a real struct device
inside irq_chip, name this .parent already today, please.
> +/* Inline functions for support of irq chips that require runtime pm */
> +static inline int chip_pm_get(struct irq_desc *desc)
> +{
> + int retval = 0;
> +
> + if (desc->irq_data.chip->dev &&
> + desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
> + retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
> +
> + return (retval < 0) ? retval : 0;
> +}
That is boiling all PM upward into the platform_device or whatever
it is containing this. But we're not just in it for runtime_pm_suspend()
and runtime_pm_resume(). We also have regular suspend() and
resume(). And ideally that should be handled by the same
callbacks.
First: what if the device contain any wakeup-flagged IRQs?
I think there is something missing here. The suspend() usecase
is not handled by this patch, but we need to think about that
here as well. I think irqchips on GPIO expanders (for example)
should be powered down on suspend() *unless* one or more of
its IRQs is flagged as wakeup, and in that case it should
*not* be powered down, instead it should just mask all
non-wakeup IRQs and restore them on resume().
Second: it's soo easy to get something wrong here. It'd be good
if the kernel was helpful. What about something like:
if (desc->irq_data.chip->dev) {
if (desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
retval = pm_runtime_get_sync(desc->irq_data.chip->dev)
else if (pm_runtime_enabled(desc->irq_data.chip->dev))
dev_warn_once(desc->irq_data.chip->dev, "irqchip not
flagged for RPM but has runtime PM enabled! weird.\n");
}
As I see it, a device that supplies an irqchip, has runtime PM but
is *NOT* setting IRQCHIP_HAS_RPM just *have* to be looked
at in detail, and deserve to have this littering its dmesg so we can
fix it. It just makes no real sense. It more sounds like a recepie for
missing interrupts otherwise.
Yours,
Linus Walleij
On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
> If we fail to map the address space for the GIC distributor or CPU
> interface, then don't attempt to initialise the chip, just WARN and
> return.
>
> Signed-off-by: Jon Hunter <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
> If the GIC initialisation fails, then currently we do not return an error
> or clean-up afterwards. Although for root controllers, this failure may be
> fatal anyway, for secondary controllers, it may not be fatal and so return
> an error on failure and clean-up.
>
> Also for non-banked GIC controllers, make sure that we free any memory
> allocated if we fail to initialise the IRQ domain.
>
> Signed-off-by: Jon Hunter <[email protected]>
(...)
Almost perfect but...
> +err:
> + if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
> + free_percpu(gic->dist_base.percpu_base);
> + free_percpu(gic->cpu_base.percpu_base);
What if the first map worked but not the second?
Should it be:
if (gic->dist_base.percpu_base)
free_percpu(gic->dist_base.percpu_base);
if (gic->cpu_base.percpu_base)
free_percpu(gic->cpu_base.percpu_base);
?
Yours,
Linus Walleij
On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
> Add a driver for the Tegra-AGIC interrupt controller which is compatible
> with the ARM GIC-400 interrupt controller.
(...)
> +static const struct dev_pm_ops gic_pm_ops = {
> + SET_RUNTIME_PM_OPS(gic_runtime_suspend,
> + gic_runtime_resume, NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
> +};
Now you do what I commented on in the earlier patch: assign
the runtime PM functions to normal suspend/resume.
This will have the effect of inhibiting any IRQs marked for
wakeup on the GIC, even if you just want to go to sleep until
something happens, will it not?
You should turn on the alarm clock before going to bed, not
turn it off, as figure of speak ...
Yours,
Linus Walleij
On 17/12/15 13:16, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
>
>> When mapping an IRQ, if a mapping already exists, then we simply return
>> the virual IRQ number. However, we do not check that the type settings for
>
> ^virtual
>
> Just that it isn't virtual, it's a Linux IRQ number, we actually use
> hwirq for the non-virtual IRQ number/offse in this function.
>
> But I know I may be fighting weathermills here.
Ok, will re-word this.
>> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>> {
>> + struct device_node *of_node;
>> struct irq_domain *domain;
>> irq_hw_number_t hwirq;
>> + unsigned int cur_type = IRQ_TYPE_NONE;
>> unsigned int type = IRQ_TYPE_NONE;
>> int virq;
>>
>> @@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>> if (irq_domain_translate(domain, fwspec, &hwirq, &type))
>> return 0;
>>
>> - if (irq_domain_is_hierarchy(domain)) {
>> - /*
>> - * If we've already configured this interrupt,
>> - * don't do it again, or hell will break loose.
>> - */
>> - virq = irq_find_mapping(domain, hwirq);
>> - if (virq)
>> - return virq;
>> + of_node = irq_domain_get_of_node(domain);
>
> Marc's patches went to great lengths to do this fwspec-neutral,
> i.e. it doesn't matter if it's done by DT or ACPI (or whatever).
>
> This just drives a truck through all of that by making
> the whole function OF-specific again.
Yes, was not sure if this would be popular. I was on the fence, but I
saw the following ...
if (!domain) {
pr_warn("no irq domain found for %s !\n",
of_node_full_name(to_of_node(fwspec->fwnode)));
return 0;
}
... and thought we are not completely agnostic. However, if you prefer I
park my mini else where, I can definitely drop this, no big deal ;-)
>>
>> - virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
>> - if (virq <= 0)
>> - return 0;
>> + /*
>> + * If we've already configured this interrupt,
>> + * don't do it again, or hell will break loose.
>> + */
>> + virq = irq_find_mapping(domain, hwirq);
>> + if (!virq) {
>> + if (irq_domain_is_hierarchy(domain)) {
>> + virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE,
>> + fwspec);
>> + if (virq <= 0)
>> + return 0;
>> + } else {
>> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
>> + of_node_to_nid(of_node));
>
> What is this all of a sudden? Not even mentioned in the
> commit. Plus I bet ACPI need something else than OF nid
> passed here.
Do you mean the else part of all of the above?
So in the current code, the else part calls irq_create_mapping() and
this function internally, calls irq_find_mapping(). Given that I am now
calling irq_find_mapping() before the if, I don't really need to call
irq_create_mapping() again, I just need to call the functions in
irq_create_mapping() that allocate and setup the IRQ number. Sorry, I
did not really explain this. However, if it is simpler, I can call
irq_create_mapping() instead and may be this makes the change easier to
read.
Cheers
Jon
On 17/12/15 13:19, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
>
> (Adding Rafael and linux-pm to To: list)
>
>> Some IRQ chips may be located in a power domain outside of the CPU
>> subsystem and hence will require device specific runtime power management.
>> In order to support such IRQ chips, add a pointer for a device structure
>> to the irq_chip structure, and if this pointer is populated by the IRQ
>> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
>> APIs for this chip will be called when an IRQ is requested/freed,
>> respectively.
>>
>> Signed-off-by: Jon Hunter <[email protected]>
>
> Overall I like what you're trying to do. This will enable e.g. I2C
> GPIO supplying expanders to power down if none of its lines are
> used for IRQs. (Read below on the suspend() case for even
> better stuff we can do!)
>
> (...)
>> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>> /**
>> * struct irq_chip - hardware interrupt chip descriptor
>> *
>> + * @dev: pointer to associated device
> (...)
>> struct irq_chip {
>> + struct device *dev;
>
> In struct gpio_chip I just this merge window have to merge a gigantic
> patch renaming this from "dev" to "parent" because we need to add
> a *real* struct device dev; to gpio_chip.
>
> So for the advent that we may in the future need a real struct device
> inside irq_chip, name this .parent already today, please.
Ok, fine with me.
>> +/* Inline functions for support of irq chips that require runtime pm */
>> +static inline int chip_pm_get(struct irq_desc *desc)
>> +{
>> + int retval = 0;
>> +
>> + if (desc->irq_data.chip->dev &&
>> + desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>> + retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
>> +
>> + return (retval < 0) ? retval : 0;
>> +}
>
> That is boiling all PM upward into the platform_device or whatever
> it is containing this. But we're not just in it for runtime_pm_suspend()
> and runtime_pm_resume(). We also have regular suspend() and
> resume(). And ideally that should be handled by the same
> callbacks.
Yes, I have purposely not tried to handle suspend here as I have left it
to be handle by suspend_device_irqs() called during system suspend.
I don't follow why we need to handle regular suspend here? Or is this
for chips that do not support runtime-pm?
> First: what if the device contain any wakeup-flagged IRQs?
So today with this patch, the IRQ chip would only be runtime suspended
if all IRQs are freed. So it should not impact wakeups. Unless I am
missing something?
> I think there is something missing here. The suspend() usecase
> is not handled by this patch, but we need to think about that
> here as well. I think irqchips on GPIO expanders (for example)
> should be powered down on suspend() *unless* one or more of
> its IRQs is flagged as wakeup, and in that case it should
> *not* be powered down, instead it should just mask all
> non-wakeup IRQs and restore them on resume().
>
> Second: it's soo easy to get something wrong here. It'd be good
> if the kernel was helpful. What about something like:
>
> if (desc->irq_data.chip->dev) {
> if (desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
> retval = pm_runtime_get_sync(desc->irq_data.chip->dev)
> else if (pm_runtime_enabled(desc->irq_data.chip->dev))
> dev_warn_once(desc->irq_data.chip->dev, "irqchip not
> flagged for RPM but has runtime PM enabled! weird.\n");
> }
>
> As I see it, a device that supplies an irqchip, has runtime PM but
> is *NOT* setting IRQCHIP_HAS_RPM just *have* to be looked
> at in detail, and deserve to have this littering its dmesg so we can
> fix it. It just makes no real sense. It more sounds like a recepie for
> missing interrupts otherwise.
Yes, I agree, additional checking such as the above would be helpful.
Thanks.
Cheers
Jon
On 17/12/15 13:26, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
>
>> If the GIC initialisation fails, then currently we do not return an error
>> or clean-up afterwards. Although for root controllers, this failure may be
>> fatal anyway, for secondary controllers, it may not be fatal and so return
>> an error on failure and clean-up.
>>
>> Also for non-banked GIC controllers, make sure that we free any memory
>> allocated if we fail to initialise the IRQ domain.
>>
>> Signed-off-by: Jon Hunter <[email protected]>
> (...)
>
> Almost perfect but...
>
>> +err:
>> + if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
>> + free_percpu(gic->dist_base.percpu_base);
>> + free_percpu(gic->cpu_base.percpu_base);
>
> What if the first map worked but not the second?
>
> Should it be:
>
> if (gic->dist_base.percpu_base)
> free_percpu(gic->dist_base.percpu_base);
> if (gic->cpu_base.percpu_base)
> free_percpu(gic->cpu_base.percpu_base);
>
> ?
Yes this is a bit lazy, but the first thing free_percpu() checks if the
pointer is NULL and simply returns.
If you look at the current code in __gic_init_bases(), if one of the two
fail, we still try to free both. I did not like this, but when I looked
at it, I could see that is does work. Happy to change it though.
Cheers
Jon
On 17/12/15 11:00, Marc Zyngier wrote:
> Hi Jon,
>
> On 17/12/15 10:48, Jon Hunter wrote:
>> Dynamically assign the irqchip structure for each GIC controller
>> instance. This is necessary in order to populate the "dev" member
>> of the irqchip structure for GIC instances that require runtime
>> power management support. This also allows us to populate a unique
>> name for each GIC controller.
>>
>> This is based upon a patch by Linus Walleij <[email protected]>.
>>
>> Signed-off-by: Jon Hunter <[email protected]>
>
> This is going to clash horribly with the version I've pulled into my tree:
>
> https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/gic-4.5&id=58b8964990dc6b59198b25337624b8518cb1dd87
>
> so you may want to have a look at this. I don't think it will affect the
> work you are doing in a major may though.
Ok, great. I was wondering if this change has been queued somewhere. It
is not too different from mine, however, AFAICT you could get rid of the
gic_eoimode1_chip structure, as I have done, as it is mostly the same as
gic_chip.
Cheers
Jon
On 17/12/15 13:32, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
>
>> Add a driver for the Tegra-AGIC interrupt controller which is compatible
>> with the ARM GIC-400 interrupt controller.
> (...)
>> +static const struct dev_pm_ops gic_pm_ops = {
>> + SET_RUNTIME_PM_OPS(gic_runtime_suspend,
>> + gic_runtime_resume, NULL)
>> + SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
>> +};
>
> Now you do what I commented on in the earlier patch: assign
> the runtime PM functions to normal suspend/resume.
>
> This will have the effect of inhibiting any IRQs marked for
> wakeup on the GIC, even if you just want to go to sleep until
> something happens, will it not?
>
> You should turn on the alarm clock before going to bed, not
> turn it off, as figure of speak ...
Yes I am alway having problems with my alarm, may be this is why ;-)
I see what you are saying, so if there are any wake-ups enabled then we
should not suspend the chip. Right?
Cheers
Jon
On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <[email protected]> wrote:
> On 17/12/15 13:16, Linus Walleij wrote:
>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
>>> + } else {
>>> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>> + of_node_to_nid(of_node));
>>
>> What is this all of a sudden? Not even mentioned in the
>> commit. Plus I bet ACPI need something else than OF nid
>> passed here.
>
> Do you mean the else part of all of the above?
Yes
> So in the current code, the else part calls irq_create_mapping() (...)
No, not that... The fact that you all of a sudden have started
calling irq_domain_alloc_descs() which the function didn't do
before, totally changing the calling semantics for everyone in
the kernel, leading to the problem I then describe with this
potentially being called before the irqdomain for the irqchip
is initialized and descs getting "random" numbers.
Yours,
Linus Walleij
On Tue, Dec 22, 2015 at 10:58 AM, Linus Walleij
<[email protected]> wrote:
> On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <[email protected]> wrote:
>> On 17/12/15 13:16, Linus Walleij wrote:
>>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
>>>> + } else {
>>>> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>>> + of_node_to_nid(of_node));
>>>
>>> What is this all of a sudden? Not even mentioned in the
>>> commit. Plus I bet ACPI need something else than OF nid
>>> passed here.
>>
>> Do you mean the else part of all of the above?
>
> Yes
>
>> So in the current code, the else part calls irq_create_mapping() (...)
>
> No, not that... The fact that you all of a sudden have started
> calling irq_domain_alloc_descs() which the function didn't do
> before, totally changing the calling semantics for everyone in
> the kernel, leading to the problem I then describe with this
> potentially being called before the irqdomain for the irqchip
> is initialized and descs getting "random" numbers.
I see I didn't go into those details in my first answer. Hm, I
guess I got uncertain and deleted it because I remember
writing it...
I am simply worries that starting to call irq_domain_alloc_descs()
has unintended side effects, especially on platforms using
legacy or simple irqdomains.
Yours,
Linus Walleij
On Fri, Dec 18, 2015 at 11:44 AM, Jon Hunter <[email protected]> wrote:
> On 17/12/15 13:32, Linus Walleij wrote:
>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
>>
>>> Add a driver for the Tegra-AGIC interrupt controller which is compatible
>>> with the ARM GIC-400 interrupt controller.
>> (...)
>>> +static const struct dev_pm_ops gic_pm_ops = {
>>> + SET_RUNTIME_PM_OPS(gic_runtime_suspend,
>>> + gic_runtime_resume, NULL)
>>> + SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
>>> +};
>>
>> Now you do what I commented on in the earlier patch: assign
>> the runtime PM functions to normal suspend/resume.
>>
>> This will have the effect of inhibiting any IRQs marked for
>> wakeup on the GIC, even if you just want to go to sleep until
>> something happens, will it not?
>>
>> You should turn on the alarm clock before going to bed, not
>> turn it off, as figure of speak ...
>
> Yes I am alway having problems with my alarm, may be this is why ;-)
>
> I see what you are saying, so if there are any wake-ups enabled then we
> should not suspend the chip. Right?
Yep, so I think you should just wrap the normal suspend functions
into some function that check if we have some wakeup active
and then just return 0, else if everything is clear, call the same
runtime PM hooks, and it will work nicely I think. IIUC the wakeup
flag is only for the suspend/resume/sleep case so this should
be enough.
Yours,
Linus Walleij
On 12/17/2015 12:48 PM, Jon Hunter wrote:
> Some IRQ chips, such as GPIO controllers or secondary level interrupt
> controllers, may require require additional runtime power management
> control to ensure they are accessible. For such IRQ chips, it makes sense
> to enable the IRQ chip when interrupts are requested and disabled them
> again once all interrupts have been freed.
>
> When mapping an IRQ, the IRQ type settings are read and then programmed.
> The mapping of the IRQ happens before the IRQ is requested and so the
> programming of the type settings occurs before the IRQ is requested. This
> is a problem for IRQ chips that require additional power management
> control because they may not be accessible yet. Therefore, when mapping
> the IRQ, don't program the type settings, just save them and then program
> these saved settings when the IRQ is requested (so long as if they are not
> overridden via the call to request the IRQ).
>
> Signed-off-by: Jon Hunter <[email protected]>
> ---
> kernel/irq/irqdomain.c | 18 ++++++++++++++----
> kernel/irq/manage.c | 7 +++++++
> 2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index eae31e978ab2..4322d6fd0b8f 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -570,6 +570,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> {
> struct device_node *of_node;
> struct irq_domain *domain;
> + struct irq_data *irq_data;
> irq_hw_number_t hwirq;
> unsigned int cur_type = IRQ_TYPE_NONE;
> unsigned int type = IRQ_TYPE_NONE;
> @@ -634,10 +635,19 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> return 0;
> }
>
> - /* Set type if specified and different than the current one */
> - if (type != IRQ_TYPE_NONE &&
> - type != irq_get_trigger_type(virq))
> - irq_set_irq_type(virq, type);
^^^ note: Return code hes never ever been checked here ;)
This patch has side effect - some boards which have
ARM TWD timer will produce below backtrace on boot:
genirq: Setting trigger mode 4 for irq 17 failed (gic_set_type+0x0/0x58)
twd: can't register interrupt 17 (-22)
------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at arch/arm/kernel/smp_twd.c:405 twd_local_timer_of_register+0x68/0x7c()
twd_local_timer_of_register failed (-22)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.13-01909-g59e42df #53
Hardware name: Generic AM43 (Flattened Device Tree)
Backtrace:
[<c00130dc>] (dump_backtrace) from [<c00132fc>] (show_stack+0x18/0x1c)
r7:c0797cec r6:00000000 r5:c08fa7bc r4:00000000
[<c00132e4>] (show_stack) from [<c0639334>] (dump_stack+0x98/0xe0)
[<c063929c>] (dump_stack) from [<c0044928>] (warn_slowpath_common+0x7c/0xb8)
r7:c0797cec r6:00000195 r5:00000009 r4:c08b3f30
[<c00448ac>] (warn_slowpath_common) from [<c004499c>] (warn_slowpath_fmt+0x38/0x40)
r8:c0932000 r7:c08b48c0 r6:ffffffff r5:ee5c9fb4 r4:c0797cc0
[<c0044968>] (warn_slowpath_fmt) from [<c085c440>] (twd_local_timer_of_register+0x68/0x7c)
r3:ffffffea r2:c0797cc0
r4:c09322c4
[<c085c3d8>] (twd_local_timer_of_register) from [<c0889d74>] (clocksource_of_init+0x50/0x90)
r5:00000001 r4:ee5c9fb4
[<c0889d24>] (clocksource_of_init) from [<c0864224>] (omap4_local_timer_init+0x34/0x68)
r5:c0932000 r4:00000000
[<c08641f0>] (omap4_local_timer_init) from [<c085b7f4>] (time_init+0x24/0x38)
[<c085b7d0>] (time_init) from [<c0857b84>] (start_kernel+0x248/0x3e4)
[<c085793c>] (start_kernel) from [<8000807c>] (0x8000807c)
r10:00000000 r9:412fc09a r8:80004059 r7:c08b8884 r6:c089934c r5:c08b4970
r4:c0932214
---[ end trace cb88537fdc8fa200 ]---
This happens, most probably, because TWD IRQs definitions in DT
do not corresponds HW and gic_configure_irq() will return -EINVAL
example (am4372.dtsi):
local_timer: timer@48240600 {
compatible = "arm,cortex-a9-twd-timer";
reg = <0x48240600 0x100>;
interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
interrupt-parent = <&gic>;
clocks = <&mpu_periphclk>;
status = "disabled";
};
So, some additional fixes might be required. Potentially problematic files are:
arch/arm/boot/dts/bcm5301x.dtsi
arch/arm/boot/dts/bcm63138.dtsi
arch/arm/boot/dts/berlin2cd.dtsi
arch/arm/boot/dts/berlin2.dtsi
arch/arm/boot/dts/berlin2q.dtsi
arch/arm/boot/dts/hip01.dtsi
arch/arm/boot/dts/omap4.dtsi
arch/arm/boot/dts/r8a7779.dts
arch/arm/boot/dts/rk3xxx.dtsi
arch/arm/boot/dts/sh73a0.dtsi
arch/arm/boot/dts/socfpga_arria10.dtsi
arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/spear13xx.dtsi
arch/arm/boot/dts/ste-dbx5x0.dtsi
arch/arm/boot/dts/tegra20.dtsi
arch/arm/boot/dts/tegra30.dtsi
arch/arm/boot/dts/uniphier-ph1-ld4.dtsi
arch/arm/boot/dts/uniphier-ph1-XXXX.dtsi
arch/arm/boot/dts/uniphier-proxstream2.dtsi
arch/arm/boot/dts/vexpress-v2p-ca5s.dts
arch/arm/boot/dts/vexpress-v2p-ca9.dts
Any way, It seems working for me, but I've back-ported and tested
it on kernel 4.1 and will try to do more tests this week.
Thanks a lot.
--
regards,
-grygorii
On 22/12/15 10:00, Linus Walleij wrote:
> On Tue, Dec 22, 2015 at 10:58 AM, Linus Walleij
> <[email protected]> wrote:
>> On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <[email protected]> wrote:
>>> On 17/12/15 13:16, Linus Walleij wrote:
>>>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
>>>>> + } else {
>>>>> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>>>> + of_node_to_nid(of_node));
>>>>
>>>> What is this all of a sudden? Not even mentioned in the
>>>> commit. Plus I bet ACPI need something else than OF nid
>>>> passed here.
>>>
>>> Do you mean the else part of all of the above?
>>
>> Yes
>>
>>> So in the current code, the else part calls irq_create_mapping() (...)
>>
>> No, not that... The fact that you all of a sudden have started
>> calling irq_domain_alloc_descs() which the function didn't do
>> before, totally changing the calling semantics for everyone in
>> the kernel, leading to the problem I then describe with this
>> potentially being called before the irqdomain for the irqchip
>> is initialized and descs getting "random" numbers.
>
> I see I didn't go into those details in my first answer. Hm, I
> guess I got uncertain and deleted it because I remember
> writing it...
>
> I am simply worries that starting to call irq_domain_alloc_descs()
> has unintended side effects, especially on platforms using
> legacy or simple irqdomains.
Ok, no problem I will keep the existing irq_create_mapping() instead then.
Cheers
Jon
On 12/22/2015 12:00 PM, Linus Walleij wrote:
> On Tue, Dec 22, 2015 at 10:58 AM, Linus Walleij
> <[email protected]> wrote:
>> On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <[email protected]> wrote:
>>> On 17/12/15 13:16, Linus Walleij wrote:
>>>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <[email protected]> wrote:
>>>>> + } else {
>>>>> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>>>> + of_node_to_nid(of_node));
>>>>
>>>> What is this all of a sudden? Not even mentioned in the
>>>> commit. Plus I bet ACPI need something else than OF nid
>>>> passed here.
>>>
>>> Do you mean the else part of all of the above?
>>
>> Yes
>>
>>> So in the current code, the else part calls irq_create_mapping() (...)
>>
>> No, not that... The fact that you all of a sudden have started
>> calling irq_domain_alloc_descs() which the function didn't do
>> before, totally changing the calling semantics for everyone in
>> the kernel, leading to the problem I then describe with this
>> potentially being called before the irqdomain for the irqchip
>> is initialized and descs getting "random" numbers.
>
> I see I didn't go into those details in my first answer. Hm, I
> guess I got uncertain and deleted it because I remember
> writing it...
>
> I am simply worries that starting to call irq_domain_alloc_descs()
> has unintended side effects, especially on platforms using
> legacy or simple irqdomains.
>
I think, some misunderstanding here introduced by replacing
irq_create_mapping() call on subset of direct calls to
irq_domain_alloc_descs() and irq_domain_associate().
--
regards,
-grygorii
On 22/12/15 11:18, Grygorii Strashko wrote:
> On 12/17/2015 12:48 PM, Jon Hunter wrote:
>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>> controllers, may require require additional runtime power management
>> control to ensure they are accessible. For such IRQ chips, it makes sense
>> to enable the IRQ chip when interrupts are requested and disabled them
>> again once all interrupts have been freed.
>>
>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>> The mapping of the IRQ happens before the IRQ is requested and so the
>> programming of the type settings occurs before the IRQ is requested. This
>> is a problem for IRQ chips that require additional power management
>> control because they may not be accessible yet. Therefore, when mapping
>> the IRQ, don't program the type settings, just save them and then program
>> these saved settings when the IRQ is requested (so long as if they are not
>> overridden via the call to request the IRQ).
>>
>> Signed-off-by: Jon Hunter <[email protected]>
>> ---
>> kernel/irq/irqdomain.c | 18 ++++++++++++++----
>> kernel/irq/manage.c | 7 +++++++
>> 2 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index eae31e978ab2..4322d6fd0b8f 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -570,6 +570,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>> {
>> struct device_node *of_node;
>> struct irq_domain *domain;
>> + struct irq_data *irq_data;
>> irq_hw_number_t hwirq;
>> unsigned int cur_type = IRQ_TYPE_NONE;
>> unsigned int type = IRQ_TYPE_NONE;
>> @@ -634,10 +635,19 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>> return 0;
>> }
>>
>> - /* Set type if specified and different than the current one */
>> - if (type != IRQ_TYPE_NONE &&
>> - type != irq_get_trigger_type(virq))
>> - irq_set_irq_type(virq, type);
>
> ^^^ note: Return code hes never ever been checked here ;)
>
> This patch has side effect - some boards which have
> ARM TWD timer will produce below backtrace on boot:
>
> genirq: Setting trigger mode 4 for irq 17 failed (gic_set_type+0x0/0x58)
> twd: can't register interrupt 17 (-22)
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at arch/arm/kernel/smp_twd.c:405 twd_local_timer_of_register+0x68/0x7c()
> twd_local_timer_of_register failed (-22)
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.13-01909-g59e42df #53
> Hardware name: Generic AM43 (Flattened Device Tree)
> Backtrace:
> [<c00130dc>] (dump_backtrace) from [<c00132fc>] (show_stack+0x18/0x1c)
> r7:c0797cec r6:00000000 r5:c08fa7bc r4:00000000
> [<c00132e4>] (show_stack) from [<c0639334>] (dump_stack+0x98/0xe0)
> [<c063929c>] (dump_stack) from [<c0044928>] (warn_slowpath_common+0x7c/0xb8)
> r7:c0797cec r6:00000195 r5:00000009 r4:c08b3f30
> [<c00448ac>] (warn_slowpath_common) from [<c004499c>] (warn_slowpath_fmt+0x38/0x40)
> r8:c0932000 r7:c08b48c0 r6:ffffffff r5:ee5c9fb4 r4:c0797cc0
> [<c0044968>] (warn_slowpath_fmt) from [<c085c440>] (twd_local_timer_of_register+0x68/0x7c)
> r3:ffffffea r2:c0797cc0
> r4:c09322c4
> [<c085c3d8>] (twd_local_timer_of_register) from [<c0889d74>] (clocksource_of_init+0x50/0x90)
> r5:00000001 r4:ee5c9fb4
> [<c0889d24>] (clocksource_of_init) from [<c0864224>] (omap4_local_timer_init+0x34/0x68)
> r5:c0932000 r4:00000000
> [<c08641f0>] (omap4_local_timer_init) from [<c085b7f4>] (time_init+0x24/0x38)
> [<c085b7d0>] (time_init) from [<c0857b84>] (start_kernel+0x248/0x3e4)
> [<c085793c>] (start_kernel) from [<8000807c>] (0x8000807c)
> r10:00000000 r9:412fc09a r8:80004059 r7:c08b8884 r6:c089934c r5:c08b4970
> r4:c0932214
> ---[ end trace cb88537fdc8fa200 ]---
>
> This happens, most probably, because TWD IRQs definitions in DT
> do not corresponds HW and gic_configure_irq() will return -EINVAL
> example (am4372.dtsi):
> local_timer: timer@48240600 {
> compatible = "arm,cortex-a9-twd-timer";
> reg = <0x48240600 0x100>;
> interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
> interrupt-parent = <&gic>;
> clocks = <&mpu_periphclk>;
> status = "disabled";
> };
Hmm ... that's interesting. Looking at the GIC documentation, it does
say that it is "implementation defined" whether you can program the type
bit for a PPI. Therefore, I am wondering if we should convert the error
into a WARN instead? It would be hard to know which of the below would
be impacted from just looking at the DT files.
> So, some additional fixes might be required. Potentially problematic files are:
> arch/arm/boot/dts/bcm5301x.dtsi
> arch/arm/boot/dts/bcm63138.dtsi
> arch/arm/boot/dts/berlin2cd.dtsi
> arch/arm/boot/dts/berlin2.dtsi
> arch/arm/boot/dts/berlin2q.dtsi
> arch/arm/boot/dts/hip01.dtsi
> arch/arm/boot/dts/omap4.dtsi
> arch/arm/boot/dts/r8a7779.dts
> arch/arm/boot/dts/rk3xxx.dtsi
> arch/arm/boot/dts/sh73a0.dtsi
> arch/arm/boot/dts/socfpga_arria10.dtsi
> arch/arm/boot/dts/socfpga.dtsi
> arch/arm/boot/dts/spear13xx.dtsi
> arch/arm/boot/dts/ste-dbx5x0.dtsi
> arch/arm/boot/dts/tegra20.dtsi
> arch/arm/boot/dts/tegra30.dtsi
So far I have not seen any issues on tegra.
> arch/arm/boot/dts/uniphier-ph1-ld4.dtsi
> arch/arm/boot/dts/uniphier-ph1-XXXX.dtsi
> arch/arm/boot/dts/uniphier-proxstream2.dtsi
> arch/arm/boot/dts/vexpress-v2p-ca5s.dts
> arch/arm/boot/dts/vexpress-v2p-ca9.dts
>
>
> Any way, It seems working for me, but I've back-ported and tested
> it on kernel 4.1 and will try to do more tests this week.
>
> Thanks a lot.
Great. No problem.
Jon
On Tue, Dec 22, 2015 at 12:56 PM, Jon Hunter <[email protected]> wrote:
> On 22/12/15 11:18, Grygorii Strashko wrote:
>> On 12/17/2015 12:48 PM, Jon Hunter wrote:
>>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>>> controllers, may require require additional runtime power management
>>> control to ensure they are accessible. For such IRQ chips, it makes sense
>>> to enable the IRQ chip when interrupts are requested and disabled them
>>> again once all interrupts have been freed.
>>>
>>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>>> The mapping of the IRQ happens before the IRQ is requested and so the
>>> programming of the type settings occurs before the IRQ is requested. This
>>> is a problem for IRQ chips that require additional power management
>>> control because they may not be accessible yet. Therefore, when mapping
>>> the IRQ, don't program the type settings, just save them and then program
>>> these saved settings when the IRQ is requested (so long as if they are not
>>> overridden via the call to request the IRQ).
>>>
>>> Signed-off-by: Jon Hunter <[email protected]>
>> This patch has side effect - some boards which have
>> ARM TWD timer will produce below backtrace on boot:
>>
>> genirq: Setting trigger mode 4 for irq 17 failed (gic_set_type+0x0/0x58)
>> twd: can't register interrupt 17 (-22)
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 0 at arch/arm/kernel/smp_twd.c:405 twd_local_timer_of_register+0x68/0x7c()
>> This happens, most probably, because TWD IRQs definitions in DT
>> do not corresponds HW and gic_configure_irq() will return -EINVAL
>> example (am4372.dtsi):
>> local_timer: timer@48240600 {
>> compatible = "arm,cortex-a9-twd-timer";
>> reg = <0x48240600 0x100>;
>> interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
>> interrupt-parent = <&gic>;
>> clocks = <&mpu_periphclk>;
>> status = "disabled";
>> };
>
>
> Hmm ... that's interesting. Looking at the GIC documentation, it does
> say that it is "implementation defined" whether you can program the type
> bit for a PPI. Therefore, I am wondering if we should convert the error
> into a WARN instead? It would be hard to know which of the below would
> be impacted from just looking at the DT files.
>
>> So, some additional fixes might be required. Potentially problematic files are:
>> arch/arm/boot/dts/r8a7779.dts
>> arch/arm/boot/dts/sh73a0.dtsi
The above two are affected. Sending patches...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 02/05/2016 04:37 PM, Linus Walleij wrote:
> On Thu, Jan 21, 2016 at 8:51 PM, Thomas Gleixner <[email protected]> wrote:
>
>> So as long as an interrupt handler is installed, there is no sane way that we
>> can decide to power down the irq chip, unless that chip has the magic ability
>> to relay incoming interrupts while powered down :)
>
> Actually isn't that exactly what almost every SoC that supports
> deepsleep does?
>
> They power off the primary interrupt controller and arm the padring
> of the SoC with an asynchronous edge detector to wake up as soon
> as something happens on a few select lines, like a keypad button
> or whatnot.
>
> The asynchronous edge detector is handled by the ROM or some
> power-management microcontroller, which wakes up the system
> and restores power to the CPU and primary interrupt controller.
> That is the magic ability right there.
>
> Of course as the wakeup signal may be deasserted at the
> point the system actually comes back up, so the magic ROM
> power management unit then needs to latch
> any latent IRQs from some shadow register to the primary interrupt
> controller, which as far as I've seen is done by out-of-tree hacks
> similar to the irq_[get/set]_irqchip_state() implemented
> by Marc Zyngier, albeit for virtualization.
>
> I've not seen it on any non-primary interrupt controller though.
>
OMAP gpio can lose context(Power) even if there
GPIO IRQs configured as wakeup sources (wakeup happen through the SoC specific HW
responsible for wakeup in this case) - pcs.
It also injects IRQs if their triggering type is not fully compatible with PM HW.
--
regards,
-grygorii