Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1383283 > unrolled thread

[PATCH V2 00/14] Add support for Tegra210 AGIC

Back to article view | Back to linux.kernel

Contents

  [PATCH V2 00/14] Add support for Tegra210 AGIC Jon Hunter <jonathanh@nvidia.com> - 2016-04-20 13:10 +0200
    [PATCH V2 12/14] irqchip/gic: Prepare for adding platform driver Jon Hunter <jonathanh@nvidia.com> - 2016-04-20 13:10 +0200
    [PATCH V2 11/14] irqchip/gic: Pass GIC pointer to save/restore functions Jon Hunter <jonathanh@nvidia.com> - 2016-04-20 13:10 +0200
    [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC Jon Hunter <jonathanh@nvidia.com> - 2016-04-20 13:10 +0200
      Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Marc Zyngier <marc.zyngier@arm.com> - 2016-04-22 11:50 +0200
      Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Mark Rutland <mark.rutland@arm.com> - 2016-04-22 12:10 +0200
        Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Jon Hunter <jonathanh@nvidia.com> - 2016-04-22 13:20 +0200
          Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Mark Rutland <mark.rutland@arm.com> - 2016-04-22 13:30 +0200
            Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Jon Hunter <jonathanh@nvidia.com> - 2016-04-22 17:00 +0200
            Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Jon Hunter <jonathanh@nvidia.com> - 2016-04-27 17:50 +0200
              Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Mark Rutland <mark.rutland@arm.com> - 2016-04-27 19:40 +0200
                Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Geert Uytterhoeven <geert@linux-m68k.org> - 2016-04-27 20:10 +0200
                Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Jon Hunter <jonathanh@nvidia.com> - 2016-04-28 10:20 +0200
                  Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Geert Uytterhoeven <geert@linux-m68k.org> - 2016-04-28 10:40 +0200
                  Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for  Tegra210 AGIC Mark Rutland <mark.rutland@arm.com> - 2016-04-28 12:00 +0200
    [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-04-20 13:10 +0200
      Re: [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-04-21 17:50 +0200
        Re: [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ Marc Zyngier <marc.zyngier@arm.com> - 2016-04-22 10:30 +0200
          Re: [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-04-22 10:50 +0200
            Re: [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ Marc Zyngier <marc.zyngier@arm.com> - 2016-04-22 11:40 +0200
    [PATCH V2 08/14] irqchip/gic: Don't initialise chip if mapping IO space fails Jon Hunter <jonathanh@nvidia.com> - 2016-04-20 13:10 +0200
    [PATCH V2 09/14] irqchip/gic: Remove static irq_chip definition for eoimode1 Jon Hunter <jonathanh@nvidia.com> - 2016-04-20 13:10 +0200
    [PATCH V2 01/14] irqchip/gic: Don't unnecessarily write the IRQ configuration Jon Hunter <jonathanh@nvidia.com> - 2016-04-20 13:20 +0200
    [PATCH V2 04/14] irqdomain: Fix handling of type settings for existing mappings Jon Hunter <jonathanh@nvidia.com> - 2016-04-20 13:20 +0200
      Re: [PATCH V2 04/14] irqdomain: Fix handling of type settings for  existing mappings Jon Hunter <jonathanh@nvidia.com> - 2016-04-21 13:40 +0200
      Re: [PATCH V2 04/14] irqdomain: Fix handling of type settings for  existing mappings Marc Zyngier <marc.zyngier@arm.com> - 2016-04-22 10:20 +0200
    [PATCH V2 02/14] irqchip/gic: WARN if setting the interrupt type for a PPI fails Jon Hunter <jonathanh@nvidia.com> - 2016-04-20 13:20 +0200
      Re: [PATCH V2 02/14] irqchip/gic: WARN if setting the interrupt type  for a PPI fails Marc Zyngier <marc.zyngier@arm.com> - 2016-04-22 11:00 +0200

Page 1 of 2  [1] 2  Next page →

#1383283 — [PATCH V2 00/14] Add support for Tegra210 AGIC

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.
This series only support routing interrupts to the main CPU cluster.

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 adding
runtime power management support for irqchips and several significant
changes to the exisiting GIC driver for power management reasons.

Changes since V1:
- Updated GIC to only WARN and not return an error if configuring a PPI
  fails but will still return an error if an SPI fails (per discussion
  with Marc).
- Dropped change to mask sense bits for GIC-v3 (as this is not
  necessary)
- Split patch to avoid setting interrupt type when mapping the IRQ into
  two patches per TGLX's feedback.
- Changed name of irqchip device structure to "parent_device"
- Moved call to irq_chip_pm_get() outside of chip_bus_lock().
- Dropped patch to remove clock names from GIC DT documentation and
  added AGIC clock names.
- Update GIC platform driver to look-up clocks names from static list.

Jon Hunter (14):
  irqchip/gic: Don't unnecessarily write the IRQ configuration
  irqchip/gic: WARN if setting the interrupt type for a PPI fails
  irqchip: Mask the non-type/sense bits when translating an IRQ
  irqdomain: Fix handling of type settings for existing mappings
  genirq: Look-up trigger type if not specified by caller
  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: Remove static irq_chip definition for eoimode1
  irqchip/gic: Return an error if GIC initialisation fails
  irqchip/gic: Pass GIC pointer to save/restore functions
  irqchip/gic: Prepare for adding platform driver
  dt-bindings: arm-gic: Add documentation for Tegra210 AGIC
  irqchip/gic: Add support for tegra AGIC interrupt controller

 .../bindings/interrupt-controller/arm,gic.txt      |   2 +
 drivers/irqchip/Kconfig                            |   1 +
 drivers/irqchip/irq-crossbar.c                     |   2 +-
 drivers/irqchip/irq-gic-common.c                   |  19 +-
 drivers/irqchip/irq-gic.c                          | 440 ++++++++++++++++-----
 drivers/irqchip/irq-tegra.c                        |   2 +-
 include/linux/irq.h                                |   4 +
 include/linux/irqdomain.h                          |   3 +
 kernel/irq/chip.c                                  |  35 ++
 kernel/irq/internals.h                             |   1 +
 kernel/irq/irqdomain.c                             |  55 ++-
 kernel/irq/manage.c                                |  40 +-
 12 files changed, 481 insertions(+), 123 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1383285 — [PATCH V2 12/14] irqchip/gic: Prepare for adding platform driver

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 resources, such as a power-domain,
is not yet available.

To re-use the code that initialises the GIC (found in __gic_init_bases()),
from within the platform driver, it is necessary to move the code from the
__init section so that it is always present and not removed. Unfortunately,
it is not possible to simply drop the __init from the function declaration
for __gic_init_bases() because it contains calls to set_smp_cross_call()
and set_handle_irq() which are both located in the __init section.
Fortunately, these calls are only required for the root controller and
because the platform driver will only support non-root controllers that
can be initialised later in the boot process, we can move these calls to
another function. Move the bulk of the code from __gic_init_bases() to a
new function called gic_init_bases() which is not located in the __init
section and can be used by the platform driver. Update __gic_init_bases()
to call gic_init_bases() and if necessary, set_smp_cross_call() and
set_handle_irq().

The function, gic_init_bases(), references the GIC via a pointer to the
GIC chip data structure instead of an index so that it can be used by the
platform driver and statically declared GICs. This means that the name
must be passed to gic_init_bases() as well, because the name will not be
passed upon an index for platform devices.

Drop the __init section from the gic_dist_config(), gic_dist_init() and
gic_pm_init() so these can be re-used by the platform driver as well.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic-common.c |  4 +-
 drivers/irqchip/irq-gic.c        | 80 +++++++++++++++++++++++++++-------------
 2 files changed, 57 insertions(+), 27 deletions(-)

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index 9fa92a17225c..083c30390aa3 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -72,8 +72,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 8fe1e9cd9a36..056c420d0960 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -436,7 +436,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;
@@ -709,7 +709,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));
@@ -727,7 +727,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
@@ -1001,34 +1001,31 @@ 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(struct gic_chip_data *gic, int irq_start,
+			  void __iomem *dist_base, void __iomem *cpu_base,
+			  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, i, ret;
 
-	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
+	if (WARN_ON(!gic || gic->domain))
+		return -EINVAL;
 
 	gic_check_cpu_features();
 
-	gic = &gic_data[gic_nr];
-
 	/* Initialize irq_chip */
 	gic->chip = gic_chip;
+	gic->chip.name = name;
 
-	if (static_key_true(&supports_deactivate) && gic_nr == 0) {
+	if (static_key_true(&supports_deactivate) && gic == &gic_data[0]) {
 		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;
-		gic->chip.name = kasprintf(GFP_KERNEL, "GICv2");
-	} else {
-		gic->chip.name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr);
 	}
 
 #ifdef CONFIG_SMP
-	if (gic_nr == 0)
+	if (gic == &gic_data[0])
 		gic->chip.irq_set_affinity = gic_set_affinity;
 #endif
 
@@ -1082,7 +1079,7 @@ static int __init __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;
@@ -1109,7 +1106,7 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		goto error;
 	}
 
-	if (gic_nr == 0) {
+	if (gic == &gic_data[0]) {
 		/*
 		 * Initialize the CPU interface map to all CPUs.
 		 * It will be refined as each CPU probes its ID.
@@ -1117,13 +1114,6 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		 */
 		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);
@@ -1138,7 +1128,47 @@ error:
 		free_percpu(gic->cpu_base.percpu_base);
 	}
 
-	kfree(gic->chip.name);
+	return ret;
+}
+
+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)
+{
+	struct gic_chip_data *gic;
+	char *name;
+	int ret;
+
+	if (WARN_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR))
+		return -EINVAL;
+
+	gic = &gic_data[gic_nr];
+
+	if (static_key_true(&supports_deactivate) && gic_nr == 0)
+		name = kasprintf(GFP_KERNEL, "GICv2");
+	else
+		name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr);
+
+	ret = gic_init_bases(gic, irq_start, dist_base, cpu_base,
+			     percpu_offset, handle, name);
+	if (ret) {
+		kfree(gic->chip.name);
+		return ret;
+	}
+
+	if (gic_nr == 0) {
+#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");
+	}
 
 	return ret;
 }
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1383290 — [PATCH V2 11/14] irqchip/gic: Pass GIC pointer to save/restore functions

Instead of passing the GIC index to the save/restore functions pass a
pointer to the GIC chip data. This will allow these save/restore
functions to be re-used by a platform driver where the GIC chip data
structure is allocated dynamically and so there is no applicable index
for identifying the GIC.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 74 +++++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 35 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index d327cb5cbc65..8fe1e9cd9a36 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -517,34 +517,35 @@ int gic_cpu_if_down(unsigned int gic_nr)
  * 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;
 
-	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
+	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);
 }
 
@@ -555,16 +556,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;
 
-	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
+	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;
@@ -572,7 +574,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++)
@@ -580,85 +582,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;
 
-	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
+	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;
 
-	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
+	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);
 
@@ -667,7 +671,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)
@@ -682,18 +686,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;
 		}
 	}
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1383291 — [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

The Tegra AGIC interrupt controller is compatible with the ARM GIC-400
interrupt controller. The Tegra AGIC requires two clocks, namely the
"ape" (functional) and "apb2ape" (interface) clocks, to operate. Add
the compatible string and clock information for the AGIC to the GIC
device-tree binding documentation.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---

I am not sure if it will be popular to add Tegra specific clock names
to the GIC DT docs. However, in that case, then possibly the only
alternative is to move the Tegra AGIC driver into its own file and
expose the GIC APIs for it to use. Then we could add our own DT doc
for the Tegra AGIC as well (based upon the ARM GIC).

 Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
index 793c20ff8fcc..6f34267f1438 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
@@ -21,6 +21,7 @@ Main node required properties:
 	"arm,pl390"
 	"arm,tc11mp-gic"
 	"brcm,brahma-b15-gic"
+	"nvidia,tegra210-agic"
 	"qcom,msm-8660-qgic"
 	"qcom,msm-qgic2"
 - interrupt-controller : Identifies the node as an interrupt controller
@@ -70,6 +71,7 @@ Optional
 	"PERIPHCLK", "PERIPHCLKEN" (for "arm,cortex-a9-gic")
 	"clk" (for "arm,gic-400")
 	"gclk" (for "arm,pl390")
+	"ape" and "apb2ape" (for "nvidia,tegra,agic")
 
 - power-domains : A phandle and PM domain specifier as defined by bindings of
 		  the power controller specified by phandle, used when the GIC
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1384829 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

On 20/04/16 12:03, Jon Hunter wrote:
> The Tegra AGIC interrupt controller is compatible with the ARM GIC-400
> interrupt controller. The Tegra AGIC requires two clocks, namely the
> "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add
> the compatible string and clock information for the AGIC to the GIC
> device-tree binding documentation.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> 
> I am not sure if it will be popular to add Tegra specific clock names
> to the GIC DT docs. However, in that case, then possibly the only
> alternative is to move the Tegra AGIC driver into its own file and
> expose the GIC APIs for it to use. Then we could add our own DT doc
> for the Tegra AGIC as well (based upon the ARM GIC).

Mark, Rob: any input on this? That would work for me, but I'd like an
ack from one of you.

Thanks,

	M.

> 
>  Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
> index 793c20ff8fcc..6f34267f1438 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
> @@ -21,6 +21,7 @@ Main node required properties:
>  	"arm,pl390"
>  	"arm,tc11mp-gic"
>  	"brcm,brahma-b15-gic"
> +	"nvidia,tegra210-agic"
>  	"qcom,msm-8660-qgic"
>  	"qcom,msm-qgic2"
>  - interrupt-controller : Identifies the node as an interrupt controller
> @@ -70,6 +71,7 @@ Optional
>  	"PERIPHCLK", "PERIPHCLKEN" (for "arm,cortex-a9-gic")
>  	"clk" (for "arm,gic-400")
>  	"gclk" (for "arm,pl390")
> +	"ape" and "apb2ape" (for "nvidia,tegra,agic")
>  
>  - power-domains : A phandle and PM domain specifier as defined by bindings of
>  		  the power controller specified by phandle, used when the GIC
> 


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

[toc] | [prev] | [next] | [standalone]

#1384910 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

On Wed, Apr 20, 2016 at 12:03:56PM +0100, Jon Hunter wrote:
> The Tegra AGIC interrupt controller is compatible with the ARM GIC-400
> interrupt controller.

The cover letter says it _is_ a GIC-400, just used in a slightly unusual
manner (i.e. not directly connected to CPUs).

> The Tegra AGIC requires two clocks, namely the
> "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add
> the compatible string and clock information for the AGIC to the GIC
> device-tree binding documentation.

The GIC-400 spec only describes "CLK" (which is what I imagine "ape" is.
There isn't an APB clock described, and the manual seems to show GIC-400
directly connected to AXI rather than APB, so that doesn't seem to even
be the usual "apb_pclk".

Is there some wrapper logic around a GIC-400 to giove it an APB
interface? Or am I misudnerstanding the spec?

> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> 
> I am not sure if it will be popular to add Tegra specific clock names
> to the GIC DT docs. However, in that case, then possibly the only
> alternative is to move the Tegra AGIC driver into its own file and
> expose the GIC APIs for it to use. Then we could add our own DT doc
> for the Tegra AGIC as well (based upon the ARM GIC).

The clock-names don't seem right to me, as they sound like provide names
or global clock line names rather than consumer-side names ("clk" and
"apb_pclk").

I'm also not certain about the compatible string; if this really is a
GIC-400 then I would at least expect "arm,gic-400" as a fallback.

Thanks,
Mark.

>  Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
> index 793c20ff8fcc..6f34267f1438 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
> @@ -21,6 +21,7 @@ Main node required properties:
>  	"arm,pl390"
>  	"arm,tc11mp-gic"
>  	"brcm,brahma-b15-gic"
> +	"nvidia,tegra210-agic"
>  	"qcom,msm-8660-qgic"
>  	"qcom,msm-qgic2"
>  - interrupt-controller : Identifies the node as an interrupt controller
> @@ -70,6 +71,7 @@ Optional
>  	"PERIPHCLK", "PERIPHCLKEN" (for "arm,cortex-a9-gic")
>  	"clk" (for "arm,gic-400")
>  	"gclk" (for "arm,pl390")
> +	"ape" and "apb2ape" (for "nvidia,tegra,agic")
>  
>  - power-domains : A phandle and PM domain specifier as defined by bindings of
>  		  the power controller specified by phandle, used when the GIC
> -- 
> 2.1.4
> 

[toc] | [prev] | [next] | [standalone]

#1385012 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

On 22/04/16 11:00, Mark Rutland wrote:
> On Wed, Apr 20, 2016 at 12:03:56PM +0100, Jon Hunter wrote:
>> The Tegra AGIC interrupt controller is compatible with the ARM GIC-400
>> interrupt controller.
> 
> The cover letter says it _is_ a GIC-400, just used in a slightly unusual
> manner (i.e. not directly connected to CPUs).

Correct.

>> The Tegra AGIC requires two clocks, namely the
>> "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add
>> the compatible string and clock information for the AGIC to the GIC
>> device-tree binding documentation.
> 
> The GIC-400 spec only describes "CLK" (which is what I imagine "ape" is.
> There isn't an APB clock described, and the manual seems to show GIC-400
> directly connected to AXI rather than APB, so that doesn't seem to even
> be the usual "apb_pclk".
> 
> Is there some wrapper logic around a GIC-400 to giove it an APB
> interface? Or am I misudnerstanding the spec?

Looking at the Tegra documentation what we have is ...

APB --> AXI switch --> AGIC (GIC400)

I am not sure how such a switch would typically be modeled in DT but we
need the apb clock to interface to the GIC registers. I am not sure if
something like simple-pm-bus is appropriate here.

>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>
>> I am not sure if it will be popular to add Tegra specific clock names
>> to the GIC DT docs. However, in that case, then possibly the only
>> alternative is to move the Tegra AGIC driver into its own file and
>> expose the GIC APIs for it to use. Then we could add our own DT doc
>> for the Tegra AGIC as well (based upon the ARM GIC).
> 
> The clock-names don't seem right to me, as they sound like provide names
> or global clock line names rather than consumer-side names ("clk" and
> "apb_pclk").

Yes that would be fine with me.

> I'm also not certain about the compatible string; if this really is a
> GIC-400 then I would at least expect "arm,gic-400" as a fallback.

OK.

Cheers
Jon

[toc] | [prev] | [next] | [standalone]

#1385041 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

On Fri, Apr 22, 2016 at 12:12:57PM +0100, Jon Hunter wrote:
> 
> On 22/04/16 11:00, Mark Rutland wrote:
> > On Wed, Apr 20, 2016 at 12:03:56PM +0100, Jon Hunter wrote:
> >> The Tegra AGIC interrupt controller is compatible with the ARM GIC-400
> >> interrupt controller.
> > 
> > The cover letter says it _is_ a GIC-400, just used in a slightly unusual
> > manner (i.e. not directly connected to CPUs).
> 
> Correct.
> 
> >> The Tegra AGIC requires two clocks, namely the
> >> "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add
> >> the compatible string and clock information for the AGIC to the GIC
> >> device-tree binding documentation.
> > 
> > The GIC-400 spec only describes "CLK" (which is what I imagine "ape" is.
> > There isn't an APB clock described, and the manual seems to show GIC-400
> > directly connected to AXI rather than APB, so that doesn't seem to even
> > be the usual "apb_pclk".
> > 
> > Is there some wrapper logic around a GIC-400 to giove it an APB
> > interface? Or am I misudnerstanding the spec?
> 
> Looking at the Tegra documentation what we have is ...
> 
> APB --> AXI switch --> AGIC (GIC400)
> 
> I am not sure how such a switch would typically be modeled in DT but we
> need the apb clock to interface to the GIC registers. I am not sure if
> something like simple-pm-bus is appropriate here.

I think we need some representation of that AXI switch in the DT;
whether simple-pm-bus is appropriate is another question. We probably
need a specific compatible string / binding regardless.

Thanks,
Mark.

> 
> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> >> ---
> >>
> >> I am not sure if it will be popular to add Tegra specific clock names
> >> to the GIC DT docs. However, in that case, then possibly the only
> >> alternative is to move the Tegra AGIC driver into its own file and
> >> expose the GIC APIs for it to use. Then we could add our own DT doc
> >> for the Tegra AGIC as well (based upon the ARM GIC).
> > 
> > The clock-names don't seem right to me, as they sound like provide names
> > or global clock line names rather than consumer-side names ("clk" and
> > "apb_pclk").
> 
> Yes that would be fine with me.

Ok; if we model the apb_pclk as owned by the AXI switch (which it is),
then there's no change for the GIC binding, short of the additional
compatible string as an extension of "arm,gic-400", as we already model
that clock in the GIC-400 binding.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1385254 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

On 22/04/16 12:22, Mark Rutland wrote:
> On Fri, Apr 22, 2016 at 12:12:57PM +0100, Jon Hunter wrote:
>>
>> On 22/04/16 11:00, Mark Rutland wrote:
>>> On Wed, Apr 20, 2016 at 12:03:56PM +0100, Jon Hunter wrote:
>>>> The Tegra AGIC interrupt controller is compatible with the ARM GIC-400
>>>> interrupt controller.
>>>
>>> The cover letter says it _is_ a GIC-400, just used in a slightly unusual
>>> manner (i.e. not directly connected to CPUs).
>>
>> Correct.
>>
>>>> The Tegra AGIC requires two clocks, namely the
>>>> "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add
>>>> the compatible string and clock information for the AGIC to the GIC
>>>> device-tree binding documentation.
>>>
>>> The GIC-400 spec only describes "CLK" (which is what I imagine "ape" is.
>>> There isn't an APB clock described, and the manual seems to show GIC-400
>>> directly connected to AXI rather than APB, so that doesn't seem to even
>>> be the usual "apb_pclk".
>>>
>>> Is there some wrapper logic around a GIC-400 to giove it an APB
>>> interface? Or am I misudnerstanding the spec?
>>
>> Looking at the Tegra documentation what we have is ...
>>
>> APB --> AXI switch --> AGIC (GIC400)
>>
>> I am not sure how such a switch would typically be modeled in DT but we
>> need the apb clock to interface to the GIC registers. I am not sure if
>> something like simple-pm-bus is appropriate here.
> 
> I think we need some representation of that AXI switch in the DT;
> whether simple-pm-bus is appropriate is another question. We probably
> need a specific compatible string / binding regardless.

OK, I will have a look at that.

>>> The clock-names don't seem right to me, as they sound like provide names
>>> or global clock line names rather than consumer-side names ("clk" and
>>> "apb_pclk").
>>
>> Yes that would be fine with me.
> 
> Ok; if we model the apb_pclk as owned by the AXI switch (which it is),
> then there's no change for the GIC binding, short of the additional
> compatible string as an extension of "arm,gic-400", as we already model
> that clock in the GIC-400 binding.

Yes that makes sense.

Thanks
Jon

[toc] | [prev] | [next] | [standalone]

#1389280 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

On 22/04/16 12:22, Mark Rutland wrote:

[snip]

>>>> I am not sure if it will be popular to add Tegra specific clock names
>>>> to the GIC DT docs. However, in that case, then possibly the only
>>>> alternative is to move the Tegra AGIC driver into its own file and
>>>> expose the GIC APIs for it to use. Then we could add our own DT doc
>>>> for the Tegra AGIC as well (based upon the ARM GIC).
>>>
>>> The clock-names don't seem right to me, as they sound like provide names
>>> or global clock line names rather than consumer-side names ("clk" and
>>> "apb_pclk").
>>
>> Yes that would be fine with me.
> 
> Ok; if we model the apb_pclk as owned by the AXI switch (which it is),
> then there's no change for the GIC binding, short of the additional
> compatible string as an extension of "arm,gic-400", as we already model
> that clock in the GIC-400 binding.

I have been re-working this based upon the feedback received. In the GIC
driver we have the following definitions ...

 IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
 IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init);
 IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init);
 IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
 IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
 IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init);
 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);


If I have something like the following in my dts ...

	agic: interrupt-controller@702f9000 {
		compatible = "nvidia,tegra210-agic", "arm,gic-400";
		...
	};

The problem with this is that it tries to register the interrupt controller
early during of_irq_init() before the platform driver has chance to
initialise it. To avoid this I got rid of the "nvidia,tegra210-agic" string
and added the following for the platform driver ...

static const struct of_device_id gic_match[] = {
       { .compatible = "arm,arm11mp-gic-pm",    .data = &arm11mp_gic_data   },
       { .compatible = "arm,cortex-a15-gic-pm", .data = &cortexa15_gic_data },
       { .compatible = "arm,cortex-a9-gic-pm",  .data = &cortexa9_gic_data  },
       { .compatible = "arm,gic400-pm",         .data = &gic400_data        },
       { .compatible = "arm,pl390-pm",          .data = &pl390_data         },
       {},
};

It is not ideal as now we have a *-pm variant of each compatible string :-(

Another option would be to add some code in gic_of_init() to check for the
presence of a "clocks" node in the DT binding and bail out of the early 
initialisation if found but may be that is a bit of a hack.

Mark, what are your thoughts on this?

Cheers
Jon

[toc] | [prev] | [next] | [standalone]

#1389407 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

On Wed, Apr 27, 2016 at 04:34:53PM +0100, Jon Hunter wrote:
> 
> On 22/04/16 12:22, Mark Rutland wrote:
> 
> [snip]
> 
> >>>> I am not sure if it will be popular to add Tegra specific clock names
> >>>> to the GIC DT docs. However, in that case, then possibly the only
> >>>> alternative is to move the Tegra AGIC driver into its own file and
> >>>> expose the GIC APIs for it to use. Then we could add our own DT doc
> >>>> for the Tegra AGIC as well (based upon the ARM GIC).
> >>>
> >>> The clock-names don't seem right to me, as they sound like provide names
> >>> or global clock line names rather than consumer-side names ("clk" and
> >>> "apb_pclk").
> >>
> >> Yes that would be fine with me.
> > 
> > Ok; if we model the apb_pclk as owned by the AXI switch (which it is),
> > then there's no change for the GIC binding, short of the additional
> > compatible string as an extension of "arm,gic-400", as we already model
> > that clock in the GIC-400 binding.
> 
> I have been re-working this based upon the feedback received. In the GIC
> driver we have the following definitions ...
> 
>  IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
>  IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init);
>  IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init);
>  IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
>  IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
>  IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init);
>  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);
> 
> 
> If I have something like the following in my dts ...
> 
> 	agic: interrupt-controller@702f9000 {
> 		compatible = "nvidia,tegra210-agic", "arm,gic-400";
> 		...
> 	};
> 
> The problem with this is that it tries to register the interrupt controller
> early during of_irq_init() before the platform driver has chance to
> initialise it.

Probe order strikes again...

> To avoid this I got rid of the "nvidia,tegra210-agic" string and added
> the following for the platform driver ...
> 
> static const struct of_device_id gic_match[] = {
>        { .compatible = "arm,arm11mp-gic-pm",    .data = &arm11mp_gic_data   },
>        { .compatible = "arm,cortex-a15-gic-pm", .data = &cortexa15_gic_data },
>        { .compatible = "arm,cortex-a9-gic-pm",  .data = &cortexa9_gic_data  },
>        { .compatible = "arm,gic400-pm",         .data = &gic400_data        },
>        { .compatible = "arm,pl390-pm",          .data = &pl390_data         },
>        {},
> };
> 
> It is not ideal as now we have a *-pm variant of each compatible string :-(

Yeah, that's a non-starter. :(

> Another option would be to add some code in gic_of_init() to check for the
> presence of a "clocks" node in the DT binding and bail out of the early 
> initialisation if found but may be that is a bit of a hack.

I fear that someone may validly have a clocks property in their root GIC
node, at which point things would fall apart. I was under the impression
this was the case for some Renesas boards (though I didn't find an
example in tree).

So I suspect that using the clocks property in that way isn't going to
work out well.

> Mark, what are your thoughts on this?

Collectively: "aargh", "oh no".

We could instead explicitly match "nvidia,tegra210-agic", bailing out if
we see that. Otherwise, if we can't handle it like a GIC-400, then we
can just drop the GIC-400 compatible string from the fallback list.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1389424 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

On Wed, Apr 27, 2016 at 7:38 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Apr 27, 2016 at 04:34:53PM +0100, Jon Hunter wrote:
>> On 22/04/16 12:22, Mark Rutland wrote:
>> [snip]
>>
>> >>>> I am not sure if it will be popular to add Tegra specific clock names
>> >>>> to the GIC DT docs. However, in that case, then possibly the only
>> >>>> alternative is to move the Tegra AGIC driver into its own file and
>> >>>> expose the GIC APIs for it to use. Then we could add our own DT doc
>> >>>> for the Tegra AGIC as well (based upon the ARM GIC).
>> >>>
>> >>> The clock-names don't seem right to me, as they sound like provide names
>> >>> or global clock line names rather than consumer-side names ("clk" and
>> >>> "apb_pclk").
>> >>
>> >> Yes that would be fine with me.
>> >
>> > Ok; if we model the apb_pclk as owned by the AXI switch (which it is),
>> > then there's no change for the GIC binding, short of the additional
>> > compatible string as an extension of "arm,gic-400", as we already model
>> > that clock in the GIC-400 binding.
>>
>> I have been re-working this based upon the feedback received. In the GIC
>> driver we have the following definitions ...
>>
>>  IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
>>  IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init);
>>  IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init);
>>  IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
>>  IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
>>  IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init);
>>  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);
>>
>>
>> If I have something like the following in my dts ...
>>
>>       agic: interrupt-controller@702f9000 {
>>               compatible = "nvidia,tegra210-agic", "arm,gic-400";
>>               ...
>>       };
>>
>> The problem with this is that it tries to register the interrupt controller
>> early during of_irq_init() before the platform driver has chance to
>> initialise it.
>
> Probe order strikes again...
>
>> To avoid this I got rid of the "nvidia,tegra210-agic" string and added
>> the following for the platform driver ...
>>
>> static const struct of_device_id gic_match[] = {
>>        { .compatible = "arm,arm11mp-gic-pm",    .data = &arm11mp_gic_data   },
>>        { .compatible = "arm,cortex-a15-gic-pm", .data = &cortexa15_gic_data },
>>        { .compatible = "arm,cortex-a9-gic-pm",  .data = &cortexa9_gic_data  },
>>        { .compatible = "arm,gic400-pm",         .data = &gic400_data        },
>>        { .compatible = "arm,pl390-pm",          .data = &pl390_data         },
>>        {},
>> };
>>
>> It is not ideal as now we have a *-pm variant of each compatible string :-(
>
> Yeah, that's a non-starter. :(
>
>> Another option would be to add some code in gic_of_init() to check for the
>> presence of a "clocks" node in the DT binding and bail out of the early
>> initialisation if found but may be that is a bit of a hack.

Or the presence of a power-domains property...

> I fear that someone may validly have a clocks property in their root GIC
> node, at which point things would fall apart. I was under the impression
> this was the case for some Renesas boards (though I didn't find an
> example in tree).

We don't have the GIC clocks in the GIC nodes yet, as there's no suitable
mechanism (e.g. CLK_ENABLE_HAND_OFF) in upstream yet to prevent them
from being disabled ("unused" clocks are disabled).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

[toc] | [prev] | [next] | [standalone]

#1389856 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

On 27/04/16 18:38, Mark Rutland wrote:
> On Wed, Apr 27, 2016 at 04:34:53PM +0100, Jon Hunter wrote:
>>
>> On 22/04/16 12:22, Mark Rutland wrote:
>>
>> [snip]
>>
>>>>>> I am not sure if it will be popular to add Tegra specific clock names
>>>>>> to the GIC DT docs. However, in that case, then possibly the only
>>>>>> alternative is to move the Tegra AGIC driver into its own file and
>>>>>> expose the GIC APIs for it to use. Then we could add our own DT doc
>>>>>> for the Tegra AGIC as well (based upon the ARM GIC).
>>>>>
>>>>> The clock-names don't seem right to me, as they sound like provide names
>>>>> or global clock line names rather than consumer-side names ("clk" and
>>>>> "apb_pclk").
>>>>
>>>> Yes that would be fine with me.
>>>
>>> Ok; if we model the apb_pclk as owned by the AXI switch (which it is),
>>> then there's no change for the GIC binding, short of the additional
>>> compatible string as an extension of "arm,gic-400", as we already model
>>> that clock in the GIC-400 binding.
>>
>> I have been re-working this based upon the feedback received. In the GIC
>> driver we have the following definitions ...
>>
>>  IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
>>  IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init);
>>  IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init);
>>  IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
>>  IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
>>  IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init);
>>  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);
>>
>>
>> If I have something like the following in my dts ...
>>
>> 	agic: interrupt-controller@702f9000 {
>> 		compatible = "nvidia,tegra210-agic", "arm,gic-400";
>> 		...
>> 	};
>>
>> The problem with this is that it tries to register the interrupt controller
>> early during of_irq_init() before the platform driver has chance to
>> initialise it.
> 
> Probe order strikes again...
> 
>> To avoid this I got rid of the "nvidia,tegra210-agic" string and added
>> the following for the platform driver ...
>>
>> static const struct of_device_id gic_match[] = {
>>        { .compatible = "arm,arm11mp-gic-pm",    .data = &arm11mp_gic_data   },
>>        { .compatible = "arm,cortex-a15-gic-pm", .data = &cortexa15_gic_data },
>>        { .compatible = "arm,cortex-a9-gic-pm",  .data = &cortexa9_gic_data  },
>>        { .compatible = "arm,gic400-pm",         .data = &gic400_data        },
>>        { .compatible = "arm,pl390-pm",          .data = &pl390_data         },
>>        {},
>> };
>>
>> It is not ideal as now we have a *-pm variant of each compatible string :-(
> 
> Yeah, that's a non-starter. :(

That is what I feared. Understood.

>> Another option would be to add some code in gic_of_init() to check for the
>> presence of a "clocks" node in the DT binding and bail out of the early 
>> initialisation if found but may be that is a bit of a hack.
> 
> I fear that someone may validly have a clocks property in their root GIC
> node, at which point things would fall apart. I was under the impression
> this was the case for some Renesas boards (though I didn't find an
> example in tree).
> 
> So I suspect that using the clocks property in that way isn't going to
> work out well.
> 
>> Mark, what are your thoughts on this?
> 
> Collectively: "aargh", "oh no".

Yes, exactly :-(

> We could instead explicitly match "nvidia,tegra210-agic", bailing out if
> we see that. Otherwise, if we can't handle it like a GIC-400, then we
> can just drop the GIC-400 compatible string from the fallback list.

Would it also be a none-starter to have "arm,gic-pm" instead of
"nvidia,tegra210-agic"? At this point it is not really specific to tegra
any more and so I was hoping to get rid of that. For example, ...

	compatible = "arm,gic-pm", "arm,gic-400";

Cheers
Jon

[toc] | [prev] | [next] | [standalone]

#1389889 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

Hi Jon,

On Thu, Apr 28, 2016 at 10:11 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> On 27/04/16 18:38, Mark Rutland wrote:
>> On Wed, Apr 27, 2016 at 04:34:53PM +0100, Jon Hunter wrote:
>>> On 22/04/16 12:22, Mark Rutland wrote:
>>> [snip]
>>>>>>> I am not sure if it will be popular to add Tegra specific clock names
>>>>>>> to the GIC DT docs. However, in that case, then possibly the only
>>>>>>> alternative is to move the Tegra AGIC driver into its own file and
>>>>>>> expose the GIC APIs for it to use. Then we could add our own DT doc
>>>>>>> for the Tegra AGIC as well (based upon the ARM GIC).
>>>>>>
>>>>>> The clock-names don't seem right to me, as they sound like provide names
>>>>>> or global clock line names rather than consumer-side names ("clk" and
>>>>>> "apb_pclk").
>>>>>
>>>>> Yes that would be fine with me.
>>>>
>>>> Ok; if we model the apb_pclk as owned by the AXI switch (which it is),
>>>> then there's no change for the GIC binding, short of the additional
>>>> compatible string as an extension of "arm,gic-400", as we already model
>>>> that clock in the GIC-400 binding.
>>>
>>> I have been re-working this based upon the feedback received. In the GIC
>>> driver we have the following definitions ...
>>>
>>>  IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
>>>  IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init);
>>>  IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init);
>>>  IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
>>>  IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
>>>  IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init);
>>>  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);
>>>
>>>
>>> If I have something like the following in my dts ...
>>>
>>>      agic: interrupt-controller@702f9000 {
>>>              compatible = "nvidia,tegra210-agic", "arm,gic-400";
>>>              ...
>>>      };
>>>
>>> The problem with this is that it tries to register the interrupt controller
>>> early during of_irq_init() before the platform driver has chance to
>>> initialise it.
>>
>> Probe order strikes again...
>>
>>> To avoid this I got rid of the "nvidia,tegra210-agic" string and added
>>> the following for the platform driver ...
>>>
>>> static const struct of_device_id gic_match[] = {
>>>        { .compatible = "arm,arm11mp-gic-pm",    .data = &arm11mp_gic_data   },
>>>        { .compatible = "arm,cortex-a15-gic-pm", .data = &cortexa15_gic_data },
>>>        { .compatible = "arm,cortex-a9-gic-pm",  .data = &cortexa9_gic_data  },
>>>        { .compatible = "arm,gic400-pm",         .data = &gic400_data        },
>>>        { .compatible = "arm,pl390-pm",          .data = &pl390_data         },
>>>        {},
>>> };
>>>
>>> It is not ideal as now we have a *-pm variant of each compatible string :-(
>>
>> Yeah, that's a non-starter. :(
>
> That is what I feared. Understood.
>
>>> Another option would be to add some code in gic_of_init() to check for the
>>> presence of a "clocks" node in the DT binding and bail out of the early
>>> initialisation if found but may be that is a bit of a hack.
>>
>> I fear that someone may validly have a clocks property in their root GIC
>> node, at which point things would fall apart. I was under the impression
>> this was the case for some Renesas boards (though I didn't find an
>> example in tree).
>>
>> So I suspect that using the clocks property in that way isn't going to
>> work out well.
>>
>>> Mark, what are your thoughts on this?
>>
>> Collectively: "aargh", "oh no".
>
> Yes, exactly :-(
>
>> We could instead explicitly match "nvidia,tegra210-agic", bailing out if
>> we see that. Otherwise, if we can't handle it like a GIC-400, then we
>> can just drop the GIC-400 compatible string from the fallback list.
>
> Would it also be a none-starter to have "arm,gic-pm" instead of
> "nvidia,tegra210-agic"? At this point it is not really specific to tegra
> any more and so I was hoping to get rid of that. For example, ...
>
>         compatible = "arm,gic-pm", "arm,gic-400";

The "-pm" is not a property of the GIC, but of the SoC. So IMHO the compatible
value should be plain "arm,gic-400".

If a device node has "clocks", "interrupts", "power-domains"[*], ...
properties, and the corresponding providers are not yet available, a driver
typically returns -EPROBE_DEFER, and will be retried later by the driver core.

[*] For "power-domains" this is handled by the device core. I.e. .probe()
     won't even be called before the dependency has been fulfilled.

With IRQCHIP_DECLARE(), you don't have the retrying, and probe order (w.r.t.
to other subsystems) is fixed.

But as you said, gic_of_init() could just bail out if it "clocks" and/or
"power-domains" properties are present, but their providers aren't.
Later, the remaining GICs can be initialized from the platform driver.
You just have to make sure no GIC is initialized twice (I believe that's what
was plaguing me last time I tried your series).

That's probably the closest you can get to normal platform_driver
behavior, without converting the whole GIC driver to a normal platform_driver,
which may cause problems on platforms that are currently working fine.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

[toc] | [prev] | [next] | [standalone]

#1389952 — Re: [PATCH V2 13/14] dt-bindings: arm-gic: Add documentation for Tegra210 AGIC

On Thu, Apr 28, 2016 at 09:11:03AM +0100, Jon Hunter wrote:
> 
> On 27/04/16 18:38, Mark Rutland wrote:
> > On Wed, Apr 27, 2016 at 04:34:53PM +0100, Jon Hunter wrote:
> >>
> >> On 22/04/16 12:22, Mark Rutland wrote:
> >>
> >> [snip]
> >>
> >>>>>> I am not sure if it will be popular to add Tegra specific clock names
> >>>>>> to the GIC DT docs. However, in that case, then possibly the only
> >>>>>> alternative is to move the Tegra AGIC driver into its own file and
> >>>>>> expose the GIC APIs for it to use. Then we could add our own DT doc
> >>>>>> for the Tegra AGIC as well (based upon the ARM GIC).
> >>>>>
> >>>>> The clock-names don't seem right to me, as they sound like provide names
> >>>>> or global clock line names rather than consumer-side names ("clk" and
> >>>>> "apb_pclk").
> >>>>
> >>>> Yes that would be fine with me.
> >>>
> >>> Ok; if we model the apb_pclk as owned by the AXI switch (which it is),
> >>> then there's no change for the GIC binding, short of the additional
> >>> compatible string as an extension of "arm,gic-400", as we already model
> >>> that clock in the GIC-400 binding.
> >>
> >> I have been re-working this based upon the feedback received. In the GIC
> >> driver we have the following definitions ...
> >>
> >>  IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
> >>  IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init);
> >>  IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init);
> >>  IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
> >>  IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
> >>  IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init);
> >>  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);
> >>
> >>
> >> If I have something like the following in my dts ...
> >>
> >> 	agic: interrupt-controller@702f9000 {
> >> 		compatible = "nvidia,tegra210-agic", "arm,gic-400";
> >> 		...
> >> 	};
> >>
> >> The problem with this is that it tries to register the interrupt controller
> >> early during of_irq_init() before the platform driver has chance to
> >> initialise it.

[...]

> > We could instead explicitly match "nvidia,tegra210-agic", bailing out if
> > we see that. Otherwise, if we can't handle it like a GIC-400, then we
> > can just drop the GIC-400 compatible string from the fallback list.
> 
> Would it also be a none-starter to have "arm,gic-pm" instead of
> "nvidia,tegra210-agic"? At this point it is not really specific to tegra
> any more and so I was hoping to get rid of that. For example, ...
> 
> 	compatible = "arm,gic-pm", "arm,gic-400";

I'm not keen on the "*-pm" approach, as such compatible strings aren't
reall describing HW, but rather the SW policy to apply, and really would
only be there to bodge around a structural issue we have in Linux today
w.r.t. the device model split and probe ordering.

The "nvidia,tegra210-agic" string can be taken as describing any
Tegra-210 specific integration quirks, though I agree that's also not
fantastic for extending PM support beyond Tegra 210 and variants
thereof.

So maybe the best approach is bailing out in the presence of clocks
and/or power domains after all, on the assumption that nothing today has
those properties, though I fear we may have problems with that later
down the line if/when people describe those for the root GIC to describe
those must be hogged, even if not explicitly managed.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1383293 — [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ

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

Add a stub function for irq_domain_free_irqs() to avoid any compilation
errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 include/linux/irqdomain.h |  3 +++
 kernel/irq/irqdomain.c    | 17 +++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 2aed04396210..fc66876d1965 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
 	return -1;
 }
 
+static inline void irq_domain_free_irqs(unsigned int virq,
+					unsigned int nr_irqs) { }
+
 static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
 {
 	return false;
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 88e9328b7aab..78a022f560f3 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
 unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 {
 	struct irq_domain *domain;
+	struct irq_data *irq_data;
 	irq_hw_number_t hwirq;
 	unsigned int type = IRQ_TYPE_NONE;
 	int virq;
@@ -639,10 +640,18 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 			return virq;
 	}
 
-	/* 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);
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1384382 — Re: [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ

On 20/04/16 12:03, 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).
> 
> Add a stub function for irq_domain_free_irqs() to avoid any compilation
> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  include/linux/irqdomain.h |  3 +++
>  kernel/irq/irqdomain.c    | 17 +++++++++++++----
>  2 files changed, 16 insertions(+), 4 deletions(-)

[snip]
 
> -	/* 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);
> +

I appear to have missed another place for saving the irq type
which I had change in this version. Next time I will triple
check! Should have been ...


diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 2aed04396210..fc66876d1965 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
        return -1;
 }
 
+static inline void irq_domain_free_irqs(unsigned int virq,
+                                       unsigned int nr_irqs) { }
+
 static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
 {
        return false;
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 3d6ef5527b71..46ecf5d468b2 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
 unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 {
        struct irq_domain *domain;
+       struct irq_data *irq_data;
        irq_hw_number_t hwirq;
        unsigned int type = IRQ_TYPE_NONE;
        int virq;
@@ -619,7 +620,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
                 * it now and return the interrupt number.
                 */
                if (IRQ_TYPE_NONE == irq_get_trigger_type(virq)) {
-                       irq_set_irq_type(virq, type);
+                       irq_data = irq_get_irq_data(virq);
+                       if (!irq_data)
+                               return 0;
+
+                       irqd_set_trigger_type(irq_data, type);
                        return virq;
                }
 
@@ -639,10 +644,18 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
                        return virq;
        }
 
-       /* 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);

[toc] | [prev] | [next] | [standalone]

#1384781 — Re: [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ

Hi Jon,

On 21/04/16 16:45, Jon Hunter wrote:
> 
> On 20/04/16 12:03, 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).
>>
>> Add a stub function for irq_domain_free_irqs() to avoid any compilation
>> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  include/linux/irqdomain.h |  3 +++
>>  kernel/irq/irqdomain.c    | 17 +++++++++++++----
>>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> [snip]
>  
>> -	/* 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);
>> +
> 
> I appear to have missed another place for saving the irq type
> which I had change in this version. Next time I will triple
> check! Should have been ...
> 
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 2aed04396210..fc66876d1965 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
>         return -1;
>  }
>  
> +static inline void irq_domain_free_irqs(unsigned int virq,
> +                                       unsigned int nr_irqs) { }
> +
>  static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
>  {
>         return false;
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 3d6ef5527b71..46ecf5d468b2 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
>  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  {
>         struct irq_domain *domain;
> +       struct irq_data *irq_data;
>         irq_hw_number_t hwirq;
>         unsigned int type = IRQ_TYPE_NONE;
>         int virq;
> @@ -619,7 +620,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>                  * it now and return the interrupt number.
>                  */
>                 if (IRQ_TYPE_NONE == irq_get_trigger_type(virq)) {
> -                       irq_set_irq_type(virq, type);
> +                       irq_data = irq_get_irq_data(virq);
> +                       if (!irq_data)
> +                               return 0;

Can you point out in which circumstances irq_data can be NULL? If we can
lookup the interrupt in the domain, it'd better exist somewhere. Or am I
missing something obvious?

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1384796 — Re: [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ

On 22/04/16 09:22, Marc Zyngier wrote:
> Hi Jon,
> 
> On 21/04/16 16:45, Jon Hunter wrote:
>>
>> On 20/04/16 12:03, 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).
>>>
>>> Add a stub function for irq_domain_free_irqs() to avoid any compilation
>>> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  include/linux/irqdomain.h |  3 +++
>>>  kernel/irq/irqdomain.c    | 17 +++++++++++++----
>>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> [snip]
>>  
>>> -	/* 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);
>>> +
>>
>> I appear to have missed another place for saving the irq type
>> which I had change in this version. Next time I will triple
>> check! Should have been ...
>>
>>
>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>> index 2aed04396210..fc66876d1965 100644
>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
>>         return -1;
>>  }
>>  
>> +static inline void irq_domain_free_irqs(unsigned int virq,
>> +                                       unsigned int nr_irqs) { }
>> +
>>  static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
>>  {
>>         return false;
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index 3d6ef5527b71..46ecf5d468b2 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
>>  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>  {
>>         struct irq_domain *domain;
>> +       struct irq_data *irq_data;
>>         irq_hw_number_t hwirq;
>>         unsigned int type = IRQ_TYPE_NONE;
>>         int virq;
>> @@ -619,7 +620,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>                  * it now and return the interrupt number.
>>                  */
>>                 if (IRQ_TYPE_NONE == irq_get_trigger_type(virq)) {
>> -                       irq_set_irq_type(virq, type);
>> +                       irq_data = irq_get_irq_data(virq);
>> +                       if (!irq_data)
>> +                               return 0;
> 
> Can you point out in which circumstances irq_data can be NULL? If we can
> lookup the interrupt in the domain, it'd better exist somewhere. Or am I
> missing something obvious?

To be honest, I was not 100% sure if this could be possible if we find
an existing mapping or if there were any paths that could race. So if we
can guarantee that it is not NULL here, I can drop this and simplify the
change.

By the way, is there any sort of reference count for mapping an IRQ so
that if irq_create_fwspec_mapping() is called more than once for an IRQ,
it is not freed on the first call to irq_dispose_mapping()? I assume
there is but could not see where this is handled.

Cheers
Jon

[toc] | [prev] | [next] | [standalone]

#1384826 — Re: [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ

On 22/04/16 09:48, Jon Hunter wrote:
> 
> On 22/04/16 09:22, Marc Zyngier wrote:
>> Hi Jon,
>>
>> On 21/04/16 16:45, Jon Hunter wrote:
>>>
>>> On 20/04/16 12:03, 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).
>>>>
>>>> Add a stub function for irq_domain_free_irqs() to avoid any compilation
>>>> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>> ---
>>>>  include/linux/irqdomain.h |  3 +++
>>>>  kernel/irq/irqdomain.c    | 17 +++++++++++++----
>>>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> [snip]
>>>  
>>>> -	/* 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);
>>>> +
>>>
>>> I appear to have missed another place for saving the irq type
>>> which I had change in this version. Next time I will triple
>>> check! Should have been ...
>>>
>>>
>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>>> index 2aed04396210..fc66876d1965 100644
>>> --- a/include/linux/irqdomain.h
>>> +++ b/include/linux/irqdomain.h
>>> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
>>>         return -1;
>>>  }
>>>  
>>> +static inline void irq_domain_free_irqs(unsigned int virq,
>>> +                                       unsigned int nr_irqs) { }
>>> +
>>>  static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
>>>  {
>>>         return false;
>>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>>> index 3d6ef5527b71..46ecf5d468b2 100644
>>> --- a/kernel/irq/irqdomain.c
>>> +++ b/kernel/irq/irqdomain.c
>>> @@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
>>>  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>>  {
>>>         struct irq_domain *domain;
>>> +       struct irq_data *irq_data;
>>>         irq_hw_number_t hwirq;
>>>         unsigned int type = IRQ_TYPE_NONE;
>>>         int virq;
>>> @@ -619,7 +620,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>>                  * it now and return the interrupt number.
>>>                  */
>>>                 if (IRQ_TYPE_NONE == irq_get_trigger_type(virq)) {
>>> -                       irq_set_irq_type(virq, type);
>>> +                       irq_data = irq_get_irq_data(virq);
>>> +                       if (!irq_data)
>>> +                               return 0;
>>
>> Can you point out in which circumstances irq_data can be NULL? If we can
>> lookup the interrupt in the domain, it'd better exist somewhere. Or am I
>> missing something obvious?
> 
> To be honest, I was not 100% sure if this could be possible if we find
> an existing mapping or if there were any paths that could race. So if we
> can guarantee that it is not NULL here, I can drop this and simplify the
> change.

No, you're right. It could very well race against irq_dispose_mapping
(and the disassociate method doesn't provide enough guarantee).
Unfortunately, most of the irqdomain code still relies on this race not
happening. How to fix this is still a bit unclear to me.

> By the way, is there any sort of reference count for mapping an IRQ so
> that if irq_create_fwspec_mapping() is called more than once for an IRQ,
> it is not freed on the first call to irq_dispose_mapping()? I assume
> there is but could not see where this is handled.

There is none, unfortunately.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web