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

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

[Jon Hunter]

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

[PATCH V2 02/14] irqchip/gic: WARN if setting the interrupt type for a PPI fails

[Jon Hunter]

Setting the interrupt type for private peripheral interrupts (PPIs) may
not be supported by a given GIC because it is IMPLEMENTATION DEFINED
whether this is allowed. There is no way to know if setting the type is
supported for a given GIC and so the value written is read back to
verify it matches the desired configuration. If it does not match then
an error is return.

There are cases where the interrupt configuration read from firmware
(such as a device-tree blob), has been incorrect and hence
gic_configure_irq() has returned an error. This error has gone
undetected because the error code returned was ignored but the interrupt
still worked fine because the configuration for the interrupt could not
be overwritten.

Given that this has done undetected and that failing to set the
configuration for a PPI may not be a catastrophic, don't return an error
but WARN if we fail to configure a PPI. This will allows us to fix up
any places in the kernel where we should be checking the return status
and maintain backward compatibility with firmware images that may have
incorrect PPI configurations.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic-common.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index ffff5a45f1e3..9fa92a17225c 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -56,12 +56,15 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
 
 	/*
 	 * Write back the new configuration, and possibly re-enable
-	 * the interrupt. If we fail to write a new configuration,
-	 * return an error.
+	 * the interrupt. WARN if we fail to write a new configuration
+	 * and return an error if we failed to write the configuration
+	 * for an SPI. If we fail to write the configuration for a PPI
+	 * this is most likely because the GIC does not allow us to set
+	 * the configuration and so it is not a catastrophic failure.
 	 */
 	writel_relaxed(val, base + GIC_DIST_CONFIG + confoff);
-	if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val)
-		ret = -EINVAL;
+	if (WARN_ON(readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val))
+		ret = irq < 32 ? 0 : -EINVAL;
 
 	if (sync_access)
 		sync_access();
-- 
2.1.4

Re: [PATCH V2 02/14] irqchip/gic: WARN if setting the interrupt type for a PPI fails

[Marc Zyngier]

On 20/04/16 12:03, Jon Hunter wrote:
> Setting the interrupt type for private peripheral interrupts (PPIs) may
> not be supported by a given GIC because it is IMPLEMENTATION DEFINED
> whether this is allowed. There is no way to know if setting the type is
> supported for a given GIC and so the value written is read back to
> verify it matches the desired configuration. If it does not match then
> an error is return.
> 
> There are cases where the interrupt configuration read from firmware
> (such as a device-tree blob), has been incorrect and hence
> gic_configure_irq() has returned an error. This error has gone
> undetected because the error code returned was ignored but the interrupt
> still worked fine because the configuration for the interrupt could not
> be overwritten.
> 
> Given that this has done undetected and that failing to set the
> configuration for a PPI may not be a catastrophic, don't return an error
> but WARN if we fail to configure a PPI. This will allows us to fix up
> any places in the kernel where we should be checking the return status
> and maintain backward compatibility with firmware images that may have
> incorrect PPI configurations.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Acked-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>

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

[PATCH V2 03/14] irqchip: Mask the non-type/sense bits when translating an IRQ

[Jon Hunter]

The firmware parameter that contains the IRQ sense bits may also contain
other data. When return the IRQ type, bits outside of these sense bits
should be masked. If these bits are not masked and
irq_create_fwspec_mapping() is called to map an IRQ, then the comparison
of the type returned from irq_domain_translate() will never match
that returned by irq_get_trigger_type() (because this function masks the
none sense bits) and so we will always call irq_set_irq_type() to program
the type even if it was not really necessary.

Currently, the downside to this is unnecessarily re-programmming the type
but nevertheless this should be avoided.

The Tegra LIC and TI Crossbar irqchips all have client instances (from
reviewing the device-tree sources) where bits outside the IRQ sense bits
are set, but do not mask these bits. Therefore, ensure these bits are
masked for these irqchips.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-crossbar.c | 2 +-
 drivers/irqchip/irq-tegra.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 75573fa431ba..1eef56a89b1f 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -183,7 +183,7 @@ static int crossbar_domain_translate(struct irq_domain *d,
 			return -EINVAL;
 
 		*hwirq = fwspec->param[1];
-		*type = fwspec->param[2];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
 		return 0;
 	}
 
diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
index 50be9639e27e..e902f081e16c 100644
--- a/drivers/irqchip/irq-tegra.c
+++ b/drivers/irqchip/irq-tegra.c
@@ -235,7 +235,7 @@ static int tegra_ictlr_domain_translate(struct irq_domain *d,
 			return -EINVAL;
 
 		*hwirq = fwspec->param[1];
-		*type = fwspec->param[2];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
 		return 0;
 	}
 
-- 
2.1.4

Re: [PATCH V2 03/14] irqchip: Mask the non-type/sense bits when translating an IRQ

[Marc Zyngier]

On 20/04/16 12:03, Jon Hunter wrote:
> The firmware parameter that contains the IRQ sense bits may also contain
> other data. When return the IRQ type, bits outside of these sense bits
> should be masked. If these bits are not masked and
> irq_create_fwspec_mapping() is called to map an IRQ, then the comparison
> of the type returned from irq_domain_translate() will never match
> that returned by irq_get_trigger_type() (because this function masks the
> none sense bits) and so we will always call irq_set_irq_type() to program
> the type even if it was not really necessary.
> 
> Currently, the downside to this is unnecessarily re-programmming the type
> but nevertheless this should be avoided.
> 
> The Tegra LIC and TI Crossbar irqchips all have client instances (from
> reviewing the device-tree sources) where bits outside the IRQ sense bits
> are set, but do not mask these bits. Therefore, ensure these bits are
> masked for these irqchips.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Acked-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>

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

[PATCH V2 01/14] irqchip/gic: Don't unnecessarily write the IRQ configuration

[Jon Hunter]

If the interrupt configuration matches the current configuration, then
don't bother writing the configuration again.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
---
 drivers/irqchip/irq-gic-common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index f174ce0ca361..ffff5a45f1e3 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -50,13 +50,17 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
 	else if (type & IRQ_TYPE_EDGE_BOTH)
 		val |= confmask;
 
+	/* If the current configuration is the same, then we are done */
+	if (val == oldval)
+		return 0;
+
 	/*
 	 * Write back the new configuration, and possibly re-enable
-	 * the interrupt. If we tried to write a new configuration and failed,
+	 * the interrupt. If we fail to write a new configuration,
 	 * return an error.
 	 */
 	writel_relaxed(val, base + GIC_DIST_CONFIG + confoff);
-	if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val && val != oldval)
+	if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val)
 		ret = -EINVAL;
 
 	if (sync_access)
-- 
2.1.4

[PATCH V2 04/14] irqdomain: Fix handling of type settings for existing mappings

[Jon Hunter]

When mapping an IRQ, it is possible that a mapping for the IRQ already
exists. If mapping does exist then there are the following issues with
regard to the handling of the IRQ type settings ...
1. If the domain is part of a hierarchy, then:
   a. We do not check that the type settings for the existing mapping
      match those of the new mapping.
   b. We do not check to see if the type settings have been programmed
      yet (and they might not have been) and so we may never set the
      type.
2. If the domain is NOT part of a hierarchy, we will overwrite the
   current type settings programmed if they are different from the
   previous mapping. Please note that irq_create_mapping()
   calls irq_find_mapping() to check if a mapping already exists.

Although, it may be unlikely that the type settings for a shared
interrupt would not match, nonetheless we should check for this.
Therefore, to fix this check if a mapping exists (regardless of whether
the domain is part of a hierarchy or not) and if it does then:
1. Return the IRQ number if the type settings match or are not
   specified.
2. Program the type settings and return the IRQ number if the type
   settings have not been programmed yet.
3. Otherwise if the type setting do not match, then print a warning
   and don't return the IRQ number.

Furthermore, add a warning if the type return by irq_domain_translate()
has bits outside the sense mask set and then clear these bits. If these
bits are not cleared then this will cause the comparision of the type
settings for an existing mapping to fail with that of the new mapping
even if the sense bit themselves match. The reason being is that the
existing type settings are read by calling irq_get_trigger_type() which
will clear any bits outside the sense mask. This will allow us to detect
irqchips that are not correctly clearing these bits and fix them.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 kernel/irq/irqdomain.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 245a485ffb61..88e9328b7aab 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -592,15 +592,43 @@ 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)) {
+	/*
+	 * WARN if the irqchip returns a type with bits
+	 * outside the sense mask set and clear these bits.
+	 */
+	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
+		type &= IRQ_TYPE_SENSE_MASK;
+
+	/*
+	 * 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 we've already configured this interrupt,
-		 * don't do it again, or hell will break loose.
+		 * If the trigger type is not specified or matches the
+		 * current trigger type then we are done so return the
+		 * interrupt number. Otherwise, if the trigger type does
+		 * not match return 0.
 		 */
-		virq = irq_find_mapping(domain, hwirq);
-		if (virq)
+		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
 			return virq;
 
+		/*
+		 * If the trigger type has not been set yet, then set
+		 * it now and return the interrupt number.
+		 */
+		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
+			irq_set_irq_type(virq, type);
+			return virq;
+		}
+
+		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
+			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
+		return 0;
+	}
+
+	if (irq_domain_is_hierarchy(domain)) {
 		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
 		if (virq <= 0)
 			return 0;
-- 
2.1.4

Re: [PATCH V2 04/14] irqdomain: Fix handling of type settings for existing mappings

[Jon Hunter]

On 20/04/16 12:03, Jon Hunter wrote:
> When mapping an IRQ, it is possible that a mapping for the IRQ already
> exists. If mapping does exist then there are the following issues with
> regard to the handling of the IRQ type settings ...
> 1. If the domain is part of a hierarchy, then:
>    a. We do not check that the type settings for the existing mapping
>       match those of the new mapping.
>    b. We do not check to see if the type settings have been programmed
>       yet (and they might not have been) and so we may never set the
>       type.
> 2. If the domain is NOT part of a hierarchy, we will overwrite the
>    current type settings programmed if they are different from the
>    previous mapping. Please note that irq_create_mapping()
>    calls irq_find_mapping() to check if a mapping already exists.
> 
> Although, it may be unlikely that the type settings for a shared
> interrupt would not match, nonetheless we should check for this.
> Therefore, to fix this check if a mapping exists (regardless of whether
> the domain is part of a hierarchy or not) and if it does then:
> 1. Return the IRQ number if the type settings match or are not
>    specified.
> 2. Program the type settings and return the IRQ number if the type
>    settings have not been programmed yet.
> 3. Otherwise if the type setting do not match, then print a warning
>    and don't return the IRQ number.
> 
> Furthermore, add a warning if the type return by irq_domain_translate()
> has bits outside the sense mask set and then clear these bits. If these
> bits are not cleared then this will cause the comparision of the type
> settings for an existing mapping to fail with that of the new mapping
> even if the sense bit themselves match. The reason being is that the
> existing type settings are read by calling irq_get_trigger_type() which
> will clear any bits outside the sense mask. This will allow us to detect
> irqchips that are not correctly clearing these bits and fix them.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  kernel/irq/irqdomain.c | 38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 245a485ffb61..88e9328b7aab 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -592,15 +592,43 @@ 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)) {
> +	/*
> +	 * WARN if the irqchip returns a type with bits
> +	 * outside the sense mask set and clear these bits.
> +	 */
> +	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
> +		type &= IRQ_TYPE_SENSE_MASK;
> +
> +	/*
> +	 * 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 we've already configured this interrupt,
> -		 * don't do it again, or hell will break loose.
> +		 * If the trigger type is not specified or matches the
> +		 * current trigger type then we are done so return the
> +		 * interrupt number. Otherwise, if the trigger type does
> +		 * not match return 0.

I think I should have dropped this 'otherwise' sentence from here.

Jon

Re: [PATCH V2 04/14] irqdomain: Fix handling of type settings for existing mappings

[Marc Zyngier]

On 20/04/16 12:03, Jon Hunter wrote:
> When mapping an IRQ, it is possible that a mapping for the IRQ already
> exists. If mapping does exist then there are the following issues with
> regard to the handling of the IRQ type settings ...
> 1. If the domain is part of a hierarchy, then:
>    a. We do not check that the type settings for the existing mapping
>       match those of the new mapping.
>    b. We do not check to see if the type settings have been programmed
>       yet (and they might not have been) and so we may never set the
>       type.
> 2. If the domain is NOT part of a hierarchy, we will overwrite the
>    current type settings programmed if they are different from the
>    previous mapping. Please note that irq_create_mapping()
>    calls irq_find_mapping() to check if a mapping already exists.
> 
> Although, it may be unlikely that the type settings for a shared
> interrupt would not match, nonetheless we should check for this.
> Therefore, to fix this check if a mapping exists (regardless of whether
> the domain is part of a hierarchy or not) and if it does then:
> 1. Return the IRQ number if the type settings match or are not
>    specified.
> 2. Program the type settings and return the IRQ number if the type
>    settings have not been programmed yet.
> 3. Otherwise if the type setting do not match, then print a warning
>    and don't return the IRQ number.
> 
> Furthermore, add a warning if the type return by irq_domain_translate()
> has bits outside the sense mask set and then clear these bits. If these
> bits are not cleared then this will cause the comparision of the type
> settings for an existing mapping to fail with that of the new mapping
> even if the sense bit themselves match. The reason being is that the
> existing type settings are read by calling irq_get_trigger_type() which
> will clear any bits outside the sense mask. This will allow us to detect
> irqchips that are not correctly clearing these bits and fix them.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Reviewed-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>

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

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

[Jon Hunter]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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

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

[Jon Hunter]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  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);

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

[Marc Zyngier]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  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...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jon Hunter]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  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

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

[Marc Zyngier]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>  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...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 05/14] genirq: Look-up trigger type if not specified by caller

[Jon Hunter]

For some devices the IRQ trigger type for a device is read from
firmware, such as device-tree. The IRQ trigger type is typically read
when the mapping for IRQ is created, which is before the IRQ is
requested. Hence, the IRQ trigger type is programmed when mapping the
IRQ and not when requesting the IRQ.

Although this works for most cases, in order to support IRQ chips which
require runtime power management, which may not be accessible prior
to requesting the IRQ, it is desirable to look-up the IRQ trigger type
when it is requested. Therefore, if the IRQ trigger type is not
specified when __setup_irq() is called, look-up the saved IRQ trigger
type. This will allow us to defer the programming of the trigger type
from when the IRQ is mapped to when it is actually requested.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 kernel/irq/manage.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index cc1cc641d653..b2a93a37f772 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1117,6 +1117,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

[PATCH V2 07/14] genirq: Add runtime power management support for IRQ chips

[Jon Hunter]

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 CONFIG_PM is selected in the kernel
configuration, 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 <jonathanh@nvidia.com>
---
 include/linux/irq.h    |  4 ++++
 kernel/irq/chip.c      | 35 +++++++++++++++++++++++++++++++++++
 kernel/irq/internals.h |  1 +
 kernel/irq/manage.c    | 33 +++++++++++++++++++++++++++++++--
 4 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index c4de62348ff2..a2140d9fa188 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 /**
  * struct irq_chip - hardware interrupt chip descriptor
  *
+ * @parent_device:	pointer to parent device for irqchip
  * @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)
@@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  * @flags:		chip specific flags
  */
 struct irq_chip {
+	struct device	*parent_device;
 	const char	*name;
 	unsigned int	(*irq_startup)(struct irq_data *data);
 	void		(*irq_shutdown)(struct irq_data *data);
@@ -488,6 +490,8 @@ extern void handle_bad_irq(struct irq_desc *desc);
 extern void handle_nested_irq(unsigned int irq);
 
 extern int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg);
+extern int irq_chip_pm_get(struct irq_data *data);
+extern int irq_chip_pm_put(struct irq_data *data);
 #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
 extern void irq_chip_enable_parent(struct irq_data *data);
 extern void irq_chip_disable_parent(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 2f9f2b0e79f2..b09226e895c7 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1093,3 +1093,38 @@ int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 
 	return 0;
 }
+
+/**
+ * irq_chip_pm_get - Enable power for an IRQ chip
+ * @data:	Pointer to interrupt specific data
+ *
+ * Enable the power to the IRQ chip referenced by the interrupt data
+ * structure.
+ */
+int irq_chip_pm_get(struct irq_data *data)
+{
+	int retval = 0;
+
+	if (IS_ENABLED(CONFIG_PM) && data->chip->parent_device)
+		retval = pm_runtime_get_sync(data->chip->parent_device);
+
+	return (retval < 0) ? retval : 0;
+}
+
+/**
+ * irq_chip_pm_put - Disable power for an IRQ chip
+ * @data:	Pointer to interrupt specific data
+ *
+ * Disable the power to the IRQ chip referenced by the interrupt data
+ * structure, belongs. Note that power will only be disabled, once this
+ * function has been called for all IRQs that have called irq_chip_pm_get().
+ */
+int irq_chip_pm_put(struct irq_data *data)
+{
+	int retval = 0;
+
+	if (IS_ENABLED(CONFIG_PM) && data->chip->parent_device)
+		retval = pm_runtime_put(data->chip->parent_device);
+
+	return (retval < 0) ? retval : 0;
+}
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 09be2c903c6d..d5edcdc9382a 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)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index b2a93a37f772..a015a84ee39c 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1414,12 +1414,20 @@ int setup_irq(unsigned int irq, struct irqaction *act)
 	int retval;
 	struct irq_desc *desc = irq_to_desc(irq);
 
-	if (WARN_ON(irq_settings_is_per_cpu_devid(desc)))
+	if (WARN_ON(!desc || irq_settings_is_per_cpu_devid(desc)))
 		return -EINVAL;
+
+	retval = irq_chip_pm_get(&desc->irq_data);
+	if (retval < 0)
+		return retval;
+
 	chip_bus_lock(desc);
 	retval = __setup_irq(irq, desc, act);
 	chip_bus_sync_unlock(desc);
 
+	if (retval)
+		irq_chip_pm_put(&desc->irq_data);
+
 	return retval;
 }
 EXPORT_SYMBOL_GPL(setup_irq);
@@ -1513,6 +1521,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 		}
 	}
 
+	irq_chip_pm_put(&desc->irq_data);
 	module_put(desc->owner);
 	kfree(action->secondary);
 	return action;
@@ -1655,11 +1664,16 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	action->name = devname;
 	action->dev_id = dev_id;
 
+	retval = irq_chip_pm_get(&desc->irq_data);
+	if (retval < 0)
+		return retval;
+
 	chip_bus_lock(desc);
 	retval = __setup_irq(irq, desc, action);
 	chip_bus_sync_unlock(desc);
 
 	if (retval) {
+		irq_chip_pm_put(&desc->irq_data);
 		kfree(action->secondary);
 		kfree(action);
 	}
@@ -1829,6 +1843,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
 
 	unregister_handler_proc(irq, action);
 
+	irq_chip_pm_put(&desc->irq_data);
 	module_put(desc->owner);
 	return action;
 
@@ -1891,10 +1906,18 @@ int setup_percpu_irq(unsigned int irq, struct irqaction *act)
 
 	if (!desc || !irq_settings_is_per_cpu_devid(desc))
 		return -EINVAL;
+
+	retval = irq_chip_pm_get(&desc->irq_data);
+	if (retval < 0)
+		return retval;
+
 	chip_bus_lock(desc);
 	retval = __setup_irq(irq, desc, act);
 	chip_bus_sync_unlock(desc);
 
+	if (retval)
+		irq_chip_pm_get(&desc->irq_data);
+
 	return retval;
 }
 
@@ -1938,12 +1961,18 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
 	action->name = devname;
 	action->percpu_dev_id = dev_id;
 
+	retval = irq_chip_pm_get(&desc->irq_data);
+	if (retval < 0)
+		return retval;
+
 	chip_bus_lock(desc);
 	retval = __setup_irq(irq, desc, action);
 	chip_bus_sync_unlock(desc);
 
-	if (retval)
+	if (retval) {
+		irq_chip_pm_put(&desc->irq_data);
 		kfree(action);
+	}
 
 	return retval;
 }
-- 
2.1.4

Re: [PATCH V2 07/14] genirq: Add runtime power management support for IRQ chips

[Kevin Hilman]

Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> writes:

> 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 CONFIG_PM is selected in the kernel
> configuration, 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 <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[...]

> @@ -1891,10 +1906,18 @@ int setup_percpu_irq(unsigned int irq, struct irqaction *act)
>  
>  	if (!desc || !irq_settings_is_per_cpu_devid(desc))
>  		return -EINVAL;
> +
> +	retval = irq_chip_pm_get(&desc->irq_data);
> +	if (retval < 0)
> +		return retval;
> +
>  	chip_bus_lock(desc);
>  	retval = __setup_irq(irq, desc, act);
>  	chip_bus_sync_unlock(desc);
>  
> +	if (retval)
> +		irq_chip_pm_get(&desc->irq_data);
> +

Shouldn't this one be a _put() ?

Otherwise, LGTM

Reviewed-by: Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

Kevin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 07/14] genirq: Add runtime power management support for IRQ chips

[Jon Hunter]

On 20/04/16 18:11, Kevin Hilman wrote:
> Jon Hunter <jonathanh@nvidia.com> writes:
> 
>> 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 CONFIG_PM is selected in the kernel
>> configuration, 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 <jonathanh@nvidia.com>
> 
> [...]
> 
>> @@ -1891,10 +1906,18 @@ int setup_percpu_irq(unsigned int irq, struct irqaction *act)
>>  
>>  	if (!desc || !irq_settings_is_per_cpu_devid(desc))
>>  		return -EINVAL;
>> +
>> +	retval = irq_chip_pm_get(&desc->irq_data);
>> +	if (retval < 0)
>> +		return retval;
>> +
>>  	chip_bus_lock(desc);
>>  	retval = __setup_irq(irq, desc, act);
>>  	chip_bus_sync_unlock(desc);
>>  
>> +	if (retval)
>> +		irq_chip_pm_get(&desc->irq_data);
>> +
> 
> Shouldn't this one be a _put() ?

Good grief! Yes it should. Sorry, will fix :-(

> Otherwise, LGTM
> 
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Thanks!
Jon

[PATCH V2 08/14] irqchip/gic: Don't initialise chip if mapping IO space fails

[Jon Hunter]

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 <jonathanh@nvidia.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 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 282344b95ec2..48ed3cd215df 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1201,10 +1201,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

[PATCH V2 09/14] irqchip/gic: Remove static irq_chip definition for eoimode1

[Jon Hunter]

There are only 3 differences (not including the name) in the definitions
of the gic_chip and gic_eoimode1_chip structures. Instead of statically
defining the gic_eoimode1_chip structure, remove it and populate the
eoimode1 functions dynamically for the appropriate GIC irqchips.

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

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 48ed3cd215df..f1b51317267f 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -391,20 +391,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,
-	.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)
 {
 	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
@@ -1026,10 +1012,14 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 	gic = &gic_data[gic_nr];
 
 	/* Initialize irq_chip */
+	gic->chip = gic_chip;
+
 	if (static_key_true(&supports_deactivate) && gic_nr == 0) {
-		gic->chip = gic_eoimode1_chip;
+		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 = "GICv2";
 	} else {
-		gic->chip = gic_chip;
 		gic->chip.name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr);
 	}
 
-- 
2.1.4

[PATCH V2 10/14] irqchip/gic: Return an error if GIC initialisation fails

[Jon Hunter]

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.

For non-banked GIC controllers, make sure that we free any memory
allocated if we fail to initialise the IRQ domain. Please note that
free_percpu() only frees memory if the pointer passed to it is not NULL
and so it is unnecessary to check if both pointers are valid or not.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic.c | 59 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index f1b51317267f..d327cb5cbc65 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -997,13 +997,13 @@ 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 >= CONFIG_ARM_GIC_MAX_NR);
 
@@ -1018,7 +1018,7 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		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 = "GICv2";
+		gic->chip.name = kasprintf(GFP_KERNEL, "GICv2");
 	} else {
 		gic->chip.name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr);
 	}
@@ -1028,17 +1028,16 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		gic->chip.irq_set_affinity = gic_set_affinity;
 #endif
 
-#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 error;
 		}
 
 		for_each_possible_cpu(cpu) {
@@ -1050,9 +1049,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);
@@ -1102,8 +1100,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 error;
+	}
 
 	if (gic_nr == 0) {
 		/*
@@ -1125,6 +1125,18 @@ 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;
+
+error:
+	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
+		free_percpu(gic->dist_base.percpu_base);
+		free_percpu(gic->cpu_base.percpu_base);
+	}
+
+	kfree(gic->chip.name);
+
+	return ret;
 }
 
 void __init gic_init(unsigned int gic_nr, int irq_start,
@@ -1185,7 +1197,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;
@@ -1210,8 +1222,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);
 
@@ -1300,7 +1318,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,
@@ -1343,7 +1361,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);
 
-- 
2.1.4

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

[Jon Hunter]

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

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

[Jon Hunter]

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

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

[Jon Hunter]

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

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

[Marc Zyngier]

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

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

[Mark Rutland]

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
> 

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

[Jon Hunter]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>
>> 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

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

[Mark Rutland]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>
> >> 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.

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

[Jon Hunter]

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

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

[Jon Hunter]

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



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Rutland]

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.

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

[Geert Uytterhoeven]

On Wed, Apr 27, 2016 at 7:38 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jon Hunter]

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Geert Uytterhoeven]

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

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

[Mark Rutland]

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.

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

[Jon Hunter]

Hi Mark,

On 28/04/16 10:55, Mark Rutland wrote:

[...]

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

On further testing, by bailing out in the presence of clocks and/or
power-domains, the problem I now see is that although the primary gic-400
has been registered, we still try to probe it again later as it matches
the platform driver. One way to avoid this would be ...

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index e7bfc175b8e1..631da7ad0dbf 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches)
                         * its children can get processed in a subsequent pass.
                         */
                        list_add_tail(&desc->list, &intc_parent_list);
+
+                       of_node_set_flag(desc->dev, OF_POPULATED);
                }

If this is not appropriate then I guess I will just need to use
"tegra210-agic" for the compatibility flag.

Cheers
Jon

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

[Geert Uytterhoeven]

Hi Jon,

On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> 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.
>
> On further testing, by bailing out in the presence of clocks and/or
> power-domains, the problem I now see is that although the primary gic-400
> has been registered, we still try to probe it again later as it matches
> the platform driver. One way to avoid this would be ...
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index e7bfc175b8e1..631da7ad0dbf 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches)
>                          * its children can get processed in a subsequent pass.
>                          */
>                         list_add_tail(&desc->list, &intc_parent_list);
> +
> +                       of_node_set_flag(desc->dev, OF_POPULATED);
>                 }

That sounds like the right thing to do to me...

> If this is not appropriate then I guess I will just need to use
> "tegra210-agic" for the compatibility flag.

As I want this for plain gic-400, I'd be unhappy ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jon Hunter]

Hi Geert,

On 07/05/16 15:10, Geert Uytterhoeven wrote:
> Hi Jon,
> 
> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>> 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.
>>
>> On further testing, by bailing out in the presence of clocks and/or
>> power-domains, the problem I now see is that although the primary gic-400
>> has been registered, we still try to probe it again later as it matches
>> the platform driver. One way to avoid this would be ...
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index e7bfc175b8e1..631da7ad0dbf 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches)
>>                          * its children can get processed in a subsequent pass.
>>                          */
>>                         list_add_tail(&desc->list, &intc_parent_list);
>> +
>> +                       of_node_set_flag(desc->dev, OF_POPULATED);
>>                 }
> 
> That sounds like the right thing to do to me...

OK. The more I think about this, it does seem silly to create a device
and pdata for a device that has already been instantiated.

>> If this is not appropriate then I guess I will just need to use
>> "tegra210-agic" for the compatibility flag.
> 
> As I want this for plain gic-400, I'd be unhappy ;-)

No problem. However, there is more work that would be needed to get this
to work for root controllers which I think that you want.

Cheers
Jon

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

[Marc Zyngier]

On 08/05/16 13:25, Jon Hunter wrote:
> Hi Geert,
> 
> On 07/05/16 15:10, Geert Uytterhoeven wrote:
>> Hi Jon,
>>
>> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>> 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.
>>>
>>> On further testing, by bailing out in the presence of clocks and/or
>>> power-domains, the problem I now see is that although the primary gic-400
>>> has been registered, we still try to probe it again later as it matches
>>> the platform driver. One way to avoid this would be ...
>>>
>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>>> index e7bfc175b8e1..631da7ad0dbf 100644
>>> --- a/drivers/of/irq.c
>>> +++ b/drivers/of/irq.c
>>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches)
>>>                          * its children can get processed in a subsequent pass.
>>>                          */
>>>                         list_add_tail(&desc->list, &intc_parent_list);
>>> +
>>> +                       of_node_set_flag(desc->dev, OF_POPULATED);
>>>                 }
>>
>> That sounds like the right thing to do to me...
> 
> OK. The more I think about this, it does seem silly to create a device
> and pdata for a device that has already been instantiated.
> 
>>> If this is not appropriate then I guess I will just need to use
>>> "tegra210-agic" for the compatibility flag.
>>
>> As I want this for plain gic-400, I'd be unhappy ;-)
> 
> No problem. However, there is more work that would be needed to get this
> to work for root controllers which I think that you want.

All this brings the discussion back to the root of the problem: irqchips
(and timers) are not first class devices, because we need them too early
for that.

I'd really like to solve this, but the kernel init is incredibly
complicated, and the subsystem dependencies completely undocumented. It
looks like we need the timer early because the we fork a thread for
PID-1, and the scheduler is going to need some form of tick.

So ideally, we'd be able to move the irq/timer stuff *after* the device
framework (which itself requires devtmpfs to be up and running, hence
dragging the whole VM and VFS), but before the scheduler is initialized.
I'm sure there is plenty of other dependencies I haven't worked out yet.

If anyone has some spare time and willing to help, please speak now! ;-)

Thanks,

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

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

[Rob Herring]

On Sat, May 7, 2016 at 9:10 AM, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> Hi Jon,
>
> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>> 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.
>>
>> On further testing, by bailing out in the presence of clocks and/or
>> power-domains, the problem I now see is that although the primary gic-400
>> has been registered, we still try to probe it again later as it matches
>> the platform driver. One way to avoid this would be ...
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index e7bfc175b8e1..631da7ad0dbf 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches)
>>                          * its children can get processed in a subsequent pass.
>>                          */
>>                         list_add_tail(&desc->list, &intc_parent_list);
>> +
>> +                       of_node_set_flag(desc->dev, OF_POPULATED);
>>                 }
>
> That sounds like the right thing to do to me...

Seems fine to me, but it would be a problem since this is a global
decision if you wanted to have some hand-off from an "early driver" to
a platform driver. I guess setting the flag could move to drivers that
need it although I don't think drivers should be touching the flags.

>> If this is not appropriate then I guess I will just need to use
>> "tegra210-agic" for the compatibility flag.
>
> As I want this for plain gic-400, I'd be unhappy ;-)

IMO, the plain gic-400 should not have these dependencies and you
should use SoC specific compatible strings should you need to deal
with this problem.

Rob

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

[Jon Hunter]

Hi Rob,

On 11/05/16 16:51, Rob Herring wrote:
> On Sat, May 7, 2016 at 9:10 AM, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> Hi Jon,
>>
>> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>> 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.
>>>
>>> On further testing, by bailing out in the presence of clocks and/or
>>> power-domains, the problem I now see is that although the primary gic-400
>>> has been registered, we still try to probe it again later as it matches
>>> the platform driver. One way to avoid this would be ...
>>>
>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>>> index e7bfc175b8e1..631da7ad0dbf 100644
>>> --- a/drivers/of/irq.c
>>> +++ b/drivers/of/irq.c
>>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches)
>>>                          * its children can get processed in a subsequent pass.
>>>                          */
>>>                         list_add_tail(&desc->list, &intc_parent_list);
>>> +
>>> +                       of_node_set_flag(desc->dev, OF_POPULATED);
>>>                 }
>>
>> That sounds like the right thing to do to me...
> 
> Seems fine to me, but it would be a problem since this is a global
> decision if you wanted to have some hand-off from an "early driver" to
> a platform driver. I guess setting the flag could move to drivers that
> need it although I don't think drivers should be touching the flags.

Isn't this the other way around? Setting this flag means that I have
been populated and so don't bother creating a platform device for this
device as it isn't needed. A by-product if this, is that if we did
happen to have a platform driver for the irqchip that also has an early
driver, then the hand-off would never happen if the early init was
successful.

The driver would still have to decide whether to hand-off and to do that
it would need to return an error from the early driver [0].

>>> If this is not appropriate then I guess I will just need to use
>>> "tegra210-agic" for the compatibility flag.
>>
>> As I want this for plain gic-400, I'd be unhappy ;-)
> 
> IMO, the plain gic-400 should not have these dependencies and you
> should use SoC specific compatible strings should you need to deal
> with this problem.

It is fine for my case, but it does mean I cannot say ...

	compatible = "tegra210-agic", "gic-400";

... because this will always match the early driver (unless we do
something like I have suggested above). So I would have ...

	compatible = "tegra210-agic";

Cheers
Jon

[0] http://marc.info/?l=devicetree&m=146237938725709&w=2

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

[Jon Hunter]

On 11/05/16 17:08, Jon Hunter wrote:
> Hi Rob,
> 
> On 11/05/16 16:51, Rob Herring wrote:
>> On Sat, May 7, 2016 at 9:10 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> Hi Jon,
>>>
>>> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>> 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.
>>>>
>>>> On further testing, by bailing out in the presence of clocks and/or
>>>> power-domains, the problem I now see is that although the primary gic-400
>>>> has been registered, we still try to probe it again later as it matches
>>>> the platform driver. One way to avoid this would be ...
>>>>
>>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>>>> index e7bfc175b8e1..631da7ad0dbf 100644
>>>> --- a/drivers/of/irq.c
>>>> +++ b/drivers/of/irq.c
>>>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches)
>>>>                          * its children can get processed in a subsequent pass.
>>>>                          */
>>>>                         list_add_tail(&desc->list, &intc_parent_list);
>>>> +
>>>> +                       of_node_set_flag(desc->dev, OF_POPULATED);
>>>>                 }
>>>
>>> That sounds like the right thing to do to me...
>>
>> Seems fine to me, but it would be a problem since this is a global
>> decision if you wanted to have some hand-off from an "early driver" to
>> a platform driver. I guess setting the flag could move to drivers that
>> need it although I don't think drivers should be touching the flags.
> 
> Isn't this the other way around? Setting this flag means that I have
> been populated and so don't bother creating a platform device for this
> device as it isn't needed. A by-product if this, is that if we did
> happen to have a platform driver for the irqchip that also has an early
> driver, then the hand-off would never happen if the early init was
> successful.
> 
> The driver would still have to decide whether to hand-off and to do that
> it would need to return an error from the early driver [0].

I mean, the "early driver would still have to decide whether to hand-off
..."

Jon

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

[Jon Hunter]

On 11/05/16 17:08, Jon Hunter wrote:
> Hi Rob,
> 
> On 11/05/16 16:51, Rob Herring wrote:
>> On Sat, May 7, 2016 at 9:10 AM, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>>> Hi Jon,
>>>
>>> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> 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.
>>>>
>>>> On further testing, by bailing out in the presence of clocks and/or
>>>> power-domains, the problem I now see is that although the primary gic-400
>>>> has been registered, we still try to probe it again later as it matches
>>>> the platform driver. One way to avoid this would be ...
>>>>
>>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>>>> index e7bfc175b8e1..631da7ad0dbf 100644
>>>> --- a/drivers/of/irq.c
>>>> +++ b/drivers/of/irq.c
>>>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches)
>>>>                          * its children can get processed in a subsequent pass.
>>>>                          */
>>>>                         list_add_tail(&desc->list, &intc_parent_list);
>>>> +
>>>> +                       of_node_set_flag(desc->dev, OF_POPULATED);
>>>>                 }
>>>
>>> That sounds like the right thing to do to me...
>>
>> Seems fine to me, but it would be a problem since this is a global
>> decision if you wanted to have some hand-off from an "early driver" to
>> a platform driver. I guess setting the flag could move to drivers that
>> need it although I don't think drivers should be touching the flags.
> 
> Isn't this the other way around? Setting this flag means that I have
> been populated and so don't bother creating a platform device for this
> device as it isn't needed. A by-product if this, is that if we did
> happen to have a platform driver for the irqchip that also has an early
> driver, then the hand-off would never happen if the early init was
> successful.
> 
> The driver would still have to decide whether to hand-off and to do that
> it would need to return an error from the early driver [0].
> 
>>>> If this is not appropriate then I guess I will just need to use
>>>> "tegra210-agic" for the compatibility flag.
>>>
>>> As I want this for plain gic-400, I'd be unhappy ;-)
>>
>> IMO, the plain gic-400 should not have these dependencies and you
>> should use SoC specific compatible strings should you need to deal
>> with this problem.
> 
> It is fine for my case, but it does mean I cannot say ...
> 
> 	compatible = "tegra210-agic", "gic-400";
> 
> ... because this will always match the early driver (unless we do
> something like I have suggested above). So I would have ...

Sorry this is wrong. The above will always match the early driver.

The problem with the above compatibility string is that, if the platform
driver matches "gic-400" then it will try to probe all gic-400s even if
they have been initialised early and this is definitely not what we
want. This could be solved by setting the OF_POPULATED flag.

Jon

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

[Rob Herring]

On Wed, May 11, 2016 at 11:16 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
> On 11/05/16 17:08, Jon Hunter wrote:
>> Hi Rob,
>>
>> On 11/05/16 16:51, Rob Herring wrote:
>>> On Sat, May 7, 2016 at 9:10 AM, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>>>> Hi Jon,
>>>>
>>>> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>> 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.
>>>>>
>>>>> On further testing, by bailing out in the presence of clocks and/or
>>>>> power-domains, the problem I now see is that although the primary gic-400
>>>>> has been registered, we still try to probe it again later as it matches
>>>>> the platform driver. One way to avoid this would be ...
>>>>>
>>>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>>>>> index e7bfc175b8e1..631da7ad0dbf 100644
>>>>> --- a/drivers/of/irq.c
>>>>> +++ b/drivers/of/irq.c
>>>>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches)
>>>>>                          * its children can get processed in a subsequent pass.
>>>>>                          */
>>>>>                         list_add_tail(&desc->list, &intc_parent_list);
>>>>> +
>>>>> +                       of_node_set_flag(desc->dev, OF_POPULATED);
>>>>>                 }
>>>>
>>>> That sounds like the right thing to do to me...
>>>
>>> Seems fine to me, but it would be a problem since this is a global
>>> decision if you wanted to have some hand-off from an "early driver" to
>>> a platform driver. I guess setting the flag could move to drivers that
>>> need it although I don't think drivers should be touching the flags.
>>
>> Isn't this the other way around? Setting this flag means that I have
>> been populated and so don't bother creating a platform device for this
>> device as it isn't needed. A by-product if this, is that if we did
>> happen to have a platform driver for the irqchip that also has an early
>> driver, then the hand-off would never happen if the early init was
>> successful.
>>
>> The driver would still have to decide whether to hand-off and to do that
>> it would need to return an error from the early driver [0].
>>
>>>>> If this is not appropriate then I guess I will just need to use
>>>>> "tegra210-agic" for the compatibility flag.
>>>>
>>>> As I want this for plain gic-400, I'd be unhappy ;-)
>>>
>>> IMO, the plain gic-400 should not have these dependencies and you
>>> should use SoC specific compatible strings should you need to deal
>>> with this problem.
>>
>> It is fine for my case, but it does mean I cannot say ...
>>
>>       compatible = "tegra210-agic", "gic-400";
>>
>> ... because this will always match the early driver (unless we do
>> something like I have suggested above). So I would have ...
>
> Sorry this is wrong. The above will always match the early driver.
>
> The problem with the above compatibility string is that, if the platform
> driver matches "gic-400" then it will try to probe all gic-400s even if
> they have been initialised early and this is definitely not what we
> want. This could be solved by setting the OF_POPULATED flag.

A platform driver for just gic-400 is wrong IMO until we have platform
drivers for all interrupt controllers.

Another reason to set OF_POPULATED flag is we are needlessly creating
platform devices for irq controllers that will never have platform
drivers. So I'd go with that approach.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jon Hunter]

Hi Rob, Mark,

On 11/05/16 17:30, Rob Herring wrote:
> A platform driver for just gic-400 is wrong IMO until we have platform
> drivers for all interrupt controllers.

Yes, that is fine with me, but can we decide on whether the platform
driver should match "tegra210-agic" or the early driver should bail-out
if clocks/power-domains are present?

I am fine with either, but I think that Rob prefers the tegra210-agic
compat string and Mark prefers to bail-out of the early driver if
clocks/power-domains are present.

> Another reason to set OF_POPULATED flag is we are needlessly creating
> platform devices for irq controllers that will never have platform
> drivers. So I'd go with that approach.

Yes exactly, that was the point I was trying to make ;-)

Jon

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

[Mark Rutland]

On Wed, May 11, 2016 at 05:53:43PM +0100, Jon Hunter wrote:
> Hi Rob, Mark,
> 
> On 11/05/16 17:30, Rob Herring wrote:
> > A platform driver for just gic-400 is wrong IMO until we have platform
> > drivers for all interrupt controllers.
> 
> Yes, that is fine with me, but can we decide on whether the platform
> driver should match "tegra210-agic" or the early driver should bail-out
> if clocks/power-domains are present?
> 
> I am fine with either, but I think that Rob prefers the tegra210-agic
> compat string and Mark prefers to bail-out of the early driver if
> clocks/power-domains are present.

If anything, I wasn't too keen on bailing out becuase of those
properties, as I mentioned for the case of a root interrupt controller.

I am happy to match the "tegra210-agic" string specifically.

Thanks,
Mark.

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

[Jon Hunter]

On 11/05/16 18:28, Mark Rutland wrote:
> On Wed, May 11, 2016 at 05:53:43PM +0100, Jon Hunter wrote:
>> Hi Rob, Mark,
>>
>> On 11/05/16 17:30, Rob Herring wrote:
>>> A platform driver for just gic-400 is wrong IMO until we have platform
>>> drivers for all interrupt controllers.
>>
>> Yes, that is fine with me, but can we decide on whether the platform
>> driver should match "tegra210-agic" or the early driver should bail-out
>> if clocks/power-domains are present?
>>
>> I am fine with either, but I think that Rob prefers the tegra210-agic
>> compat string and Mark prefers to bail-out of the early driver if
>> clocks/power-domains are present.
> 
> If anything, I wasn't too keen on bailing out becuase of those
> properties, as I mentioned for the case of a root interrupt controller.
> 
> I am happy to match the "tegra210-agic" string specifically.

Thanks guys. I will stick with tegra210-agic for now.

Cheers
Jon

[PATCH V2 14/14] irqchip/gic: Add support for tegra AGIC interrupt controller

[Jon Hunter]

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") has already added clock and power-domain properties to the
GIC binding and so we can make use of these for the AGIC.

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.

The GIC platform driver has been implemented by making the following
changes to the core GIC driver:
1. Remove the dependency on CONFIG_CPU_PM from PM specific variables and
   functions so that they can be used by the platform driver even when
   CONFIG_CPU_PM is not selected.
2. Move the code that maps the GIC registers and parses the device-tree
   blob into a new function called gic_of_setup() that can be used by
   both the platform driver as well as the existing driver.
3. Add and register platform driver for the GIC. The platform driver
   uses the PM_CLK framework for managing the clocks used by the GIC
   and so select CONFIG_PM_CLK.

Finally, a couple other notes on the implementation are:
1. Currently the GIC platform driver only supports non-root GICs and
   assumes that the GIC has a parent interrupt. It is assumed that
   root interrupt controllers need to be initialised early.
2. There is no specific suspend handling for GICs registered as platform
   devices. Non-wakeup interrupts will be disabled by the kernel during
   late suspend, however, this alone will not power down the GIC if
   interrupts have been requested and not freed. Therefore, requestors
   of non-wakeup interrupts will need to free them on entering suspend
   in order to power-down the GIC.

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

Please note that so far I have not added all the clock names for the
various GIC versions (as described by the DT documentation). I thought
we could add these as they are needed.

 drivers/irqchip/Kconfig   |   1 +
 drivers/irqchip/irq-gic.c | 225 +++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 203 insertions(+), 23 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 3e124793e224..d3b05426eb38 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -7,6 +7,7 @@ config ARM_GIC
 	select IRQ_DOMAIN
 	select IRQ_DOMAIN_HIERARCHY
 	select MULTI_IRQ_HANDLER
+	select PM_CLK
 
 config ARM_GIC_MAX_NR
 	int
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 056c420d0960..1876097613c9 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -26,17 +26,22 @@
 #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>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/acpi.h>
 #include <linux/irqdomain.h>
 #include <linux/interrupt.h>
 #include <linux/percpu.h>
+#include <linux/platform_device.h>
+#include <linux/pm_clock.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/irqchip.h>
 #include <linux/irqchip/chained_irq.h>
@@ -68,11 +73,15 @@ union gic_base {
 	void __percpu * __iomem *percpu_base;
 };
 
+struct gic_clk_data {
+	unsigned int num_clocks;
+	const char *const *clocks;
+};
+
 struct gic_chip_data {
 	struct irq_chip chip;
 	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 +89,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
@@ -510,7 +518,6 @@ 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
@@ -726,11 +733,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)
@@ -1225,27 +1227,42 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 	return true;
 }
 
-int __init
-gic_of_init(struct device_node *node, struct device_node *parent)
+static int gic_of_setup(struct device_node *node, void __iomem **dist_base,
+			void __iomem **cpu_base, u32 *percpu_offset)
 {
-	void __iomem *cpu_base;
-	void __iomem *dist_base;
-	u32 percpu_offset;
-	int irq, ret;
-
 	if (WARN_ON(!node))
 		return -ENODEV;
 
-	dist_base = of_iomap(node, 0);
-	if (WARN(!dist_base, "unable to map gic dist registers\n"))
+	*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);
+	*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;
+}
+
+int __init gic_of_init(struct device_node *node, struct device_node *parent)
+{
+	void __iomem *cpu_base;
+	void __iomem *dist_base;
+	u32 percpu_offset;
+	int irq, ret;
+
+	if (WARN_ON(gic_cnt >= CONFIG_ARM_GIC_MAX_NR))
+		return -EINVAL;
+
+	ret = gic_of_setup(node, &dist_base, &cpu_base, &percpu_offset);
+	if (ret)
+		return ret;
+
 	/*
 	 * Disable split EOI/Deactivate if either HYP is not available
 	 * or the CPU interface is too small.
@@ -1253,9 +1270,6 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	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 (ret) {
@@ -1288,6 +1302,171 @@ 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 = pm_clk_resume(dev);
+	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);
+
+	return pm_clk_suspend(dev);
+}
+
+static int gic_get_clocks(struct device *dev, const struct gic_clk_data *data)
+{
+	struct clk *clk;
+	unsigned int i;
+	int ret;
+
+	if (!dev || !data)
+		return -EINVAL;
+
+	ret = pm_clk_create(dev);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < data->num_clocks; i++) {
+		clk = of_clk_get_by_name(dev->of_node, data->clocks[i]);
+		if (IS_ERR(clk)) {
+			dev_err(dev, "failed to get clock %s\n",
+				data->clocks[i]);
+			ret = PTR_ERR(clk);
+			goto error;
+		}
+
+		ret = pm_clk_add_clk(dev, clk);
+		if (ret) {
+			dev_err(dev, "failed to add clock at index %d\n", i);
+			clk_put(clk);
+			goto error;
+		}
+	}
+
+	return 0;
+
+error:
+	pm_clk_destroy(dev);
+
+	return ret;
+}
+
+static int gic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct gic_clk_data *data;
+	struct gic_chip_data *gic;
+	void __iomem *dist_base;
+	void __iomem *cpu_base;
+	u32 percpu_offset;
+	int ret, irq;
+
+	data = of_device_get_match_data(&pdev->dev);
+	if (!data) {
+		dev_err(&pdev->dev, "no device match found\n");
+		return -ENODEV;
+	}
+
+	gic = devm_kzalloc(dev, sizeof(*gic), GFP_KERNEL);
+	if (!gic)
+		return -ENOMEM;
+
+	ret = gic_get_clocks(dev, data);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, gic);
+
+	pm_runtime_enable(dev);
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto rpm_disable;
+
+	irq = irq_of_parse_and_map(dev->of_node, 0);
+	if (!irq) {
+		ret = -EINVAL;
+		goto rpm_put;
+	}
+
+	ret = gic_of_setup(dev->of_node, &dist_base, &cpu_base, &percpu_offset);
+	if (ret)
+		goto irq_dispose;
+
+	ret = gic_init_bases(gic, -1, dist_base, cpu_base,
+			     percpu_offset, &dev->of_node->fwnode,
+			     dev->of_node->name);
+	if (ret)
+		goto gic_unmap;
+
+	gic->chip.parent_device = dev;
+
+	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;
+
+gic_unmap:
+	iounmap(dist_base);
+	iounmap(cpu_base);
+irq_dispose:
+	irq_dispose_mapping(irq);
+rpm_put:
+	pm_runtime_put_sync(dev);
+rpm_disable:
+	pm_runtime_disable(dev);
+	pm_clk_destroy(dev);
+
+	return ret;
+}
+
+static const struct dev_pm_ops gic_pm_ops = {
+	SET_RUNTIME_PM_OPS(gic_runtime_suspend,
+			   gic_runtime_resume, NULL)
+};
+
+static const char * const tegra210_agic_clocks[] = {
+	"ape", "apb2ape",
+};
+
+static const struct gic_clk_data tegra210_agic_data = {
+	.num_clocks = ARRAY_SIZE(tegra210_agic_clocks),
+	.clocks = tegra210_agic_clocks,
+};
+
+static const struct of_device_id gic_match[] = {
+	{ .compatible = "nvidia,tegra210-agic", .data = &tegra210_agic_data },
+	{},
+};
+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

Re: [PATCH V2 14/14] irqchip/gic: Add support for tegra AGIC interrupt controller

[Marc Zyngier]

On 20/04/16 12:03, 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") has already added clock and power-domain properties to the
> GIC binding and so we can make use of these for the AGIC.
> 
> 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.
> 
> The GIC platform driver has been implemented by making the following
> changes to the core GIC driver:
> 1. Remove the dependency on CONFIG_CPU_PM from PM specific variables and
>    functions so that they can be used by the platform driver even when
>    CONFIG_CPU_PM is not selected.
> 2. Move the code that maps the GIC registers and parses the device-tree
>    blob into a new function called gic_of_setup() that can be used by
>    both the platform driver as well as the existing driver.
> 3. Add and register platform driver for the GIC. The platform driver
>    uses the PM_CLK framework for managing the clocks used by the GIC
>    and so select CONFIG_PM_CLK.
> 
> Finally, a couple other notes on the implementation are:
> 1. Currently the GIC platform driver only supports non-root GICs and
>    assumes that the GIC has a parent interrupt. It is assumed that
>    root interrupt controllers need to be initialised early.
> 2. There is no specific suspend handling for GICs registered as platform
>    devices. Non-wakeup interrupts will be disabled by the kernel during
>    late suspend, however, this alone will not power down the GIC if
>    interrupts have been requested and not freed. Therefore, requestors
>    of non-wakeup interrupts will need to free them on entering suspend
>    in order to power-down the GIC.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 
> Please note that so far I have not added all the clock names for the
> various GIC versions (as described by the DT documentation). I thought
> we could add these as they are needed.

So while the code looks OK, I'd like to stop adding more code to this
poor GIC driver, because it is starting to look like a huge mess.

What is preventing us to move this to a separate irq-gic-pm.c file?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 14/14] irqchip/gic: Add support for tegra AGIC interrupt controller

[Jon Hunter]

On 22/04/16 10:57, Marc Zyngier wrote:
> On 20/04/16 12:03, 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") has already added clock and power-domain properties to the
>> GIC binding and so we can make use of these for the AGIC.
>>
>> 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.
>>
>> The GIC platform driver has been implemented by making the following
>> changes to the core GIC driver:
>> 1. Remove the dependency on CONFIG_CPU_PM from PM specific variables and
>>    functions so that they can be used by the platform driver even when
>>    CONFIG_CPU_PM is not selected.
>> 2. Move the code that maps the GIC registers and parses the device-tree
>>    blob into a new function called gic_of_setup() that can be used by
>>    both the platform driver as well as the existing driver.
>> 3. Add and register platform driver for the GIC. The platform driver
>>    uses the PM_CLK framework for managing the clocks used by the GIC
>>    and so select CONFIG_PM_CLK.
>>
>> Finally, a couple other notes on the implementation are:
>> 1. Currently the GIC platform driver only supports non-root GICs and
>>    assumes that the GIC has a parent interrupt. It is assumed that
>>    root interrupt controllers need to be initialised early.
>> 2. There is no specific suspend handling for GICs registered as platform
>>    devices. Non-wakeup interrupts will be disabled by the kernel during
>>    late suspend, however, this alone will not power down the GIC if
>>    interrupts have been requested and not freed. Therefore, requestors
>>    of non-wakeup interrupts will need to free them on entering suspend
>>    in order to power-down the GIC.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>
>> Please note that so far I have not added all the clock names for the
>> various GIC versions (as described by the DT documentation). I thought
>> we could add these as they are needed.
> 
> So while the code looks OK, I'd like to stop adding more code to this
> poor GIC driver, because it is starting to look like a huge mess.

Agree.

> What is preventing us to move this to a separate irq-gic-pm.c file?

Absolutely nothing. Once we sort out the clocking, I will do that.

Cheers
Jon