[PATCH V3 00/17] Add support for Tegra210 AGIC

[PATCH V3 00/17] Add support for Tegra210 AGIC

[Jon Hunter]

The Tegra210 has a 2nd level interrupt controller located in a separate
power domain to the main GIC interrupt controller and hence requires
runtime-pm support.

Add a platform driver for the GICs that require runtime-pm and make the
necessary changes to the genirq and irqdomain core to support IRQ chips
that require runtime-pm.

Please note that although as the subject states this adds support for
the Tegra210 AGIC, in this current version there is really nothing
specific in this series for Tegra. However, rather than changing the
subject I opted to keep it the same so people recognise this is a
continuation of that work.

Changes since V2:
- Corrected RPM support for irqchips in genirq core as pointed out by
  Kevin Hilman.
- Corrected patch to save the irq type when mapping the interrupt.
- Dropped changes to DT binding documentation and added patch to prevent
  early init of GICs if the 'clocks' and/or 'power-domains' DT
  properties are present (as we have discussed).
- Moved platform driver code into it's own source file.
- Separate changes for preparing for the platform driver into 3 patches
  in an attempt to make it more readable!
- Added fix for checking an interrupt descriptor is valid during IRQ
  setup.

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 (17):
  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: Ensure IRQ descriptor is valid when setting-up the 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: Don't allow early initialisation if GIC requires RPM
  irqchip/gic: Add helper function for configuring a GIC via device-tree
  irqchip/gic: Split GIC init in preparation for platform driver
  irqchip/gic: Prepare for adding platform driver
  irqchip/gic: Add platform driver for non-root GICs that require RPM

 drivers/irqchip/Kconfig          |   6 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-crossbar.c   |   2 +-
 drivers/irqchip/irq-gic-common.c |  19 ++-
 drivers/irqchip/irq-gic-pm.c     | 241 ++++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-gic.c        | 277 +++++++++++++++++++++++----------------
 drivers/irqchip/irq-gic.h        |  63 +++++++++
 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           |  58 ++++++--
 kernel/irq/manage.c              |  40 +++++-
 14 files changed, 617 insertions(+), 135 deletions(-)
 create mode 100644 drivers/irqchip/irq-gic-pm.c
 create mode 100644 drivers/irqchip/irq-gic.h

-- 
2.1.4

--
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 V3 01/17] 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@nvidia.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 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 V3 02/17] 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>
Acked-by: Marc Zyngier <marc.zyngier@arm.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 V3 02/17] irqchip/gic: WARN if setting the interrupt type for a PPI fails

[Marc Zyngier]

Hi Jon,

On 04/05/16 17:25, 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>
> ---
>  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();
> 

I'm going to slightly backpedal on that one:

When running in non-secure mode, you can reconfigure secure interrupts
(for obvious reasons). But you don't know which mode you're running in
either. A typical example is the arch timer, which requests both secure
and non-secure interrupts, because we cannot know which side of the CPU
we're running on. In the non-secure case, we end-up with a splat that
is rather undeserved.

So I'm tempted to tone down the splat in the PPI case like this:

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index 083c303..1605e42 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -63,8 +63,17 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
 	 * the configuration and so it is not a catastrophic failure.
 	 */
 	writel_relaxed(val, base + GIC_DIST_CONFIG + confoff);
-	if (WARN_ON(readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val))
-		ret = irq < 32 ? 0 : -EINVAL;
+	oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
+	if (oldval != val) {
+		if (irq < 32) {
+			pr_warn("GIC: PPI%d is either secure or misconfigured\n",
+				irq - 16);
+			ret = 0;
+		} else {
+			WARN_ON(1);
+			ret = -EINVAL;
+		}
+	}
 
 	if (sync_access)
 		sync_access();

Thoughts?

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

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

[Jon Hunter]

Hi Marc,

On 05/05/16 13:06, Marc Zyngier wrote:
> Hi Jon,
> 
> On 04/05/16 17:25, 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@nvidia.com>
>> Acked-by: Marc Zyngier <marc.zyngier@arm.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();
>>
> 
> I'm going to slightly backpedal on that one:
> 
> When running in non-secure mode, you can reconfigure secure interrupts

Do you mean 'cannot'?

> (for obvious reasons). But you don't know which mode you're running in
> either. A typical example is the arch timer, which requests both secure
> and non-secure interrupts, because we cannot know which side of the CPU
> we're running on. In the non-secure case, we end-up with a splat that
> is rather undeserved.

Yes seems sensible.

> So I'm tempted to tone down the splat in the PPI case like this:
> 
> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
> index 083c303..1605e42 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -63,8 +63,17 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
>  	 * the configuration and so it is not a catastrophic failure.
>  	 */
>  	writel_relaxed(val, base + GIC_DIST_CONFIG + confoff);
> -	if (WARN_ON(readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val))
> -		ret = irq < 32 ? 0 : -EINVAL;
> +	oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
> +	if (oldval != val) {
> +		if (irq < 32) {
> +			pr_warn("GIC: PPI%d is either secure or misconfigured\n",
> +				irq - 16);
> +			ret = 0;
> +		} else {
> +			WARN_ON(1);
> +			ret = -EINVAL;
> +		}
> +	}
>  
>  	if (sync_access)
>  		sync_access();
> 
> Thoughts?

That is fine with me. Do you want me to re-spin or do you want to apply
your change on top? However, before I re-spin would like to get your
thoughts on patches 13-17.

Cheers
Jon

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

[Marc Zyngier]

On Thu, 5 May 2016 14:22:06 +0100
Jon Hunter <jonathanh@nvidia.com> wrote:

> Hi Marc,
> 
> On 05/05/16 13:06, Marc Zyngier wrote:
> > Hi Jon,
> > 
> > On 04/05/16 17:25, 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@nvidia.com>
> >> Acked-by: Marc Zyngier <marc.zyngier@arm.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();
> >>
> > 
> > I'm going to slightly backpedal on that one:
> > 
> > When running in non-secure mode, you can reconfigure secure interrupts
> 
> Do you mean 'cannot'?

Yes, sorry.

> > (for obvious reasons). But you don't know which mode you're running in
> > either. A typical example is the arch timer, which requests both secure
> > and non-secure interrupts, because we cannot know which side of the CPU
> > we're running on. In the non-secure case, we end-up with a splat that
> > is rather undeserved.
> 
> Yes seems sensible.
> 
> > So I'm tempted to tone down the splat in the PPI case like this:
> > 
> > diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
> > index 083c303..1605e42 100644
> > --- a/drivers/irqchip/irq-gic-common.c
> > +++ b/drivers/irqchip/irq-gic-common.c
> > @@ -63,8 +63,17 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
> >  	 * the configuration and so it is not a catastrophic failure.
> >  	 */
> >  	writel_relaxed(val, base + GIC_DIST_CONFIG + confoff);
> > -	if (WARN_ON(readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val))
> > -		ret = irq < 32 ? 0 : -EINVAL;
> > +	oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
> > +	if (oldval != val) {
> > +		if (irq < 32) {
> > +			pr_warn("GIC: PPI%d is either secure or misconfigured\n",
> > +				irq - 16);
> > +			ret = 0;
> > +		} else {
> > +			WARN_ON(1);
> > +			ret = -EINVAL;
> > +		}
> > +	}
> >  
> >  	if (sync_access)
> >  		sync_access();
> > 
> > Thoughts?
> 
> That is fine with me. Do you want me to re-spin or do you want to apply
> your change on top? However, before I re-spin would like to get your
> thoughts on patches 13-17.

I can squash this into your own patch if you're OK with it. I'll reply
to your other patches shortly, as I have a number of comments there.

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

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

[Jon Hunter]

On 05/05/16 14:40, Marc Zyngier wrote:
> On Thu, 5 May 2016 14:22:06 +0100
> Jon Hunter <jonathanh@nvidia.com> wrote:
> 
>> Hi Marc,
>>
>> On 05/05/16 13:06, Marc Zyngier wrote:
>>> Hi Jon,
>>>
>>> On 04/05/16 17:25, 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@nvidia.com>
>>>> Acked-by: Marc Zyngier <marc.zyngier@arm.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();
>>>>
>>>
>>> I'm going to slightly backpedal on that one:
>>>
>>> When running in non-secure mode, you can reconfigure secure interrupts
>>
>> Do you mean 'cannot'?
> 
> Yes, sorry.
> 
>>> (for obvious reasons). But you don't know which mode you're running in
>>> either. A typical example is the arch timer, which requests both secure
>>> and non-secure interrupts, because we cannot know which side of the CPU
>>> we're running on. In the non-secure case, we end-up with a splat that
>>> is rather undeserved.
>>
>> Yes seems sensible.
>>
>>> So I'm tempted to tone down the splat in the PPI case like this:
>>>
>>> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
>>> index 083c303..1605e42 100644
>>> --- a/drivers/irqchip/irq-gic-common.c
>>> +++ b/drivers/irqchip/irq-gic-common.c
>>> @@ -63,8 +63,17 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
>>>  	 * the configuration and so it is not a catastrophic failure.
>>>  	 */
>>>  	writel_relaxed(val, base + GIC_DIST_CONFIG + confoff);
>>> -	if (WARN_ON(readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val))
>>> -		ret = irq < 32 ? 0 : -EINVAL;
>>> +	oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
>>> +	if (oldval != val) {
>>> +		if (irq < 32) {
>>> +			pr_warn("GIC: PPI%d is either secure or misconfigured\n",
>>> +				irq - 16);
>>> +			ret = 0;
>>> +		} else {
>>> +			WARN_ON(1);
>>> +			ret = -EINVAL;
>>> +		}
>>> +	}
>>>  
>>>  	if (sync_access)
>>>  		sync_access();
>>>
>>> Thoughts?
>>
>> That is fine with me. Do you want me to re-spin or do you want to apply
>> your change on top? However, before I re-spin would like to get your
>> thoughts on patches 13-17.
> 
> I can squash this into your own patch if you're OK with it. I'll reply
> to your other patches shortly, as I have a number of comments there.

Yes that is fine with me.

Cheers
Jon

[PATCH V3 04/17] 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@nvidia.com>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
---
 kernel/irq/irqdomain.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index e564e16f2530..d68371213fc9 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -587,15 +587,42 @@ 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.
 		 */
-		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

[PATCH V3 05/17] 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 V3 06/17] 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@nvidia.com>
---
 include/linux/irqdomain.h |  3 +++
 kernel/irq/irqdomain.c    | 23 ++++++++++++++++++-----
 2 files changed, 21 insertions(+), 5 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 d68371213fc9..bbf5b9b8ac3d 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -564,6 +564,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;
@@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 		 * it now and return the interrupt number.
 		 */
 		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
-			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;
 		}
 
@@ -633,10 +638,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 V3 06/17] irqdomain: Don't set type when mapping an IRQ

[Marc Zyngier]

On 04/05/16 17:25, Jon Hunter wrote:
> Some IRQ chips, such as GPIO controllers or secondary level interrupt
> controllers, may require require additional runtime power management
> control to ensure they are accessible. For such IRQ chips, it makes sense
> to enable the IRQ chip when interrupts are requested and disabled them
> again once all interrupts have been freed.
> 
> When mapping an IRQ, the IRQ type settings are read and then programmed.
> The mapping of the IRQ happens before the IRQ is requested and so the
> programming of the type settings occurs before the IRQ is requested. This
> is a problem for IRQ chips that require additional power management
> control because they may not be accessible yet. Therefore, when mapping
> the IRQ, don't program the type settings, just save them and then program
> these saved settings when the IRQ is requested (so long as if they are not
> overridden via the call to request the IRQ).
> 
> Add a stub function for irq_domain_free_irqs() to avoid any compilation
> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  include/linux/irqdomain.h |  3 +++
>  kernel/irq/irqdomain.c    | 23 ++++++++++++++++++-----
>  2 files changed, 21 insertions(+), 5 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 d68371213fc9..bbf5b9b8ac3d 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -564,6 +564,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;
> @@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  		 * it now and return the interrupt number.
>  		 */
>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> -			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;
>  		}
>  
> @@ -633,10 +638,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);
> 

This patch have the effect of making misconfigured PPIs absolutely
obvious. I still need to wrap my head around the root cause, but here's
the findings I have so far:

- kvmtool generates a DT with the wrong trigger information (edge
instead of level) for the timer.
- with this patch applied, "cyclictest -S" reliably locks up when run in
a guest (missing a timer interrupt, goodbye CPU).
- Either fixing kvmtool or reverting that patch makes it work reliably
again.

My gut feeling is that until that patch, the failing irq_set_irq_type()
wasn't affecting the kernel's view of the trigger (it was still treated
as level). With this patch, the kernel now trusts whatever is coming
from the firmware, and the misconfiguration becomes obvious. And just
grepping through the DT files for arm and arm64 sends makes me thing
"Holly effin' crap!".

I'm not saying that we shouldn't perform this change though. But it is
quite obvious that it is going to break an awful lot of existing code
and platforms. I'm also cooking a small patch for the arch timer (which
seems to be described in DT with a fairly high level of brokenness), so
that we can mop-up most of the brain damage.

Thanks,

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

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

[Jon Hunter]

On 09/05/16 13:23, Marc Zyngier wrote:
> On 04/05/16 17:25, 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    | 23 ++++++++++++++++++-----
>>  2 files changed, 21 insertions(+), 5 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 d68371213fc9..bbf5b9b8ac3d 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -564,6 +564,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;
>> @@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>  		 * it now and return the interrupt number.
>>  		 */
>>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>> -			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;
>>  		}
>>  
>> @@ -633,10 +638,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);
>>
> 
> This patch have the effect of making misconfigured PPIs absolutely
> obvious. I still need to wrap my head around the root cause, but here's
> the findings I have so far:
>
> - kvmtool generates a DT with the wrong trigger information (edge
> instead of level) for the timer.
> - with this patch applied, "cyclictest -S" reliably locks up when run in
> a guest (missing a timer interrupt, goodbye CPU).
> - Either fixing kvmtool or reverting that patch makes it work reliably
> again.
> 
> My gut feeling is that until that patch, the failing irq_set_irq_type()
> wasn't affecting the kernel's view of the trigger (it was still treated
> as level). With this patch, the kernel now trusts whatever is coming
> from the firmware, and the misconfiguration becomes obvious. And just
> grepping through the DT files for arm and arm64 sends makes me thing
> "Holly effin' crap!".
> 
> I'm not saying that we shouldn't perform this change though. But it is
> quite obvious that it is going to break an awful lot of existing code
> and platforms. I'm also cooking a small patch for the arch timer (which
> seems to be described in DT with a fairly high level of brokenness), so
> that we can mop-up most of the brain damage.

Hmmm ... yes I see. I wonder if we should make the setting of the type
here dependent upon PM being enabled for an irqchip? We could check to
see if the .parent_device is populated and if so only then save the type
and otherwise just set it as we do today.

We could add a WARN to the existing irq_set_irq_type() or may be just a
pr_warn() if a WARN is too verbose so people can fix up any issues.

I am also wondering if patch 4/17 "iqdomain: Fix handling of type
settings for existing mappings" could generate a lot of reports
interrupts failing due to bad firmware? I wonder if I should tone this
patch down to a warning message as well as opposed to a complete failure.

Cheers
Jon

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

[Marc Zyngier]

On 09/05/16 14:13, Jon Hunter wrote:
> 
> On 09/05/16 13:23, Marc Zyngier wrote:
>> On 04/05/16 17:25, 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    | 23 ++++++++++++++++++-----
>>>  2 files changed, 21 insertions(+), 5 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 d68371213fc9..bbf5b9b8ac3d 100644
>>> --- a/kernel/irq/irqdomain.c
>>> +++ b/kernel/irq/irqdomain.c
>>> @@ -564,6 +564,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;
>>> @@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>>  		 * it now and return the interrupt number.
>>>  		 */
>>>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>> -			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;
>>>  		}
>>>  
>>> @@ -633,10 +638,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);
>>>
>>
>> This patch have the effect of making misconfigured PPIs absolutely
>> obvious. I still need to wrap my head around the root cause, but here's
>> the findings I have so far:
>>
>> - kvmtool generates a DT with the wrong trigger information (edge
>> instead of level) for the timer.
>> - with this patch applied, "cyclictest -S" reliably locks up when run in
>> a guest (missing a timer interrupt, goodbye CPU).
>> - Either fixing kvmtool or reverting that patch makes it work reliably
>> again.
>>
>> My gut feeling is that until that patch, the failing irq_set_irq_type()
>> wasn't affecting the kernel's view of the trigger (it was still treated
>> as level). With this patch, the kernel now trusts whatever is coming
>> from the firmware, and the misconfiguration becomes obvious. And just
>> grepping through the DT files for arm and arm64 sends makes me thing
>> "Holly effin' crap!".
>>
>> I'm not saying that we shouldn't perform this change though. But it is
>> quite obvious that it is going to break an awful lot of existing code
>> and platforms. I'm also cooking a small patch for the arch timer (which
>> seems to be described in DT with a fairly high level of brokenness), so
>> that we can mop-up most of the brain damage.
> 
> Hmmm ... yes I see. I wonder if we should make the setting of the type
> here dependent upon PM being enabled for an irqchip? We could check to
> see if the .parent_device is populated and if so only then save the type
> and otherwise just set it as we do today.

I don't really like the idea of having multiple code paths for the same thing.
This is very error prone, and likely to bitrot pretty quickly.

> We could add a WARN to the existing irq_set_irq_type() or may be just a
> pr_warn() if a WARN is too verbose so people can fix up any issues.
> 
> I am also wondering if patch 4/17 "iqdomain: Fix handling of type
> settings for existing mappings" could generate a lot of reports
> interrupts failing due to bad firmware? I wonder if I should tone this
> patch down to a warning message as well as opposed to a complete failure.

We'll see. We can always tone it down a notch, should it prove to be too noisy...
So far, I haven't seen it firing. On the other hand, I get the following stuff
on my APM board:

[    0.000000] GIC: PPI0 is either secure or misconfigured
[    0.000000] GIC: PPI13 is either secure or misconfigured
[    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ1, assuming level low
[    0.000000] arm_arch_timer: WARNING: Please fix your firmware
[    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2, assuming level low
[    0.000000] arm_arch_timer: WARNING: Please fix your firmware

Pretty awesome...

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

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

[Jon Hunter]

On 09/05/16 16:10, Marc Zyngier wrote:
> On 09/05/16 14:13, Jon Hunter wrote:
>> On 09/05/16 13:23, Marc Zyngier wrote:

[snip]

>>> This patch have the effect of making misconfigured PPIs absolutely
>>> obvious. I still need to wrap my head around the root cause, but here's
>>> the findings I have so far:
>>>
>>> - kvmtool generates a DT with the wrong trigger information (edge
>>> instead of level) for the timer.
>>> - with this patch applied, "cyclictest -S" reliably locks up when run in
>>> a guest (missing a timer interrupt, goodbye CPU).
>>> - Either fixing kvmtool or reverting that patch makes it work reliably
>>> again.
>>>
>>> My gut feeling is that until that patch, the failing irq_set_irq_type()
>>> wasn't affecting the kernel's view of the trigger (it was still treated
>>> as level). With this patch, the kernel now trusts whatever is coming
>>> from the firmware, and the misconfiguration becomes obvious. And just
>>> grepping through the DT files for arm and arm64 sends makes me thing
>>> "Holly effin' crap!".
>>>
>>> I'm not saying that we shouldn't perform this change though. But it is
>>> quite obvious that it is going to break an awful lot of existing code
>>> and platforms. I'm also cooking a small patch for the arch timer (which
>>> seems to be described in DT with a fairly high level of brokenness), so
>>> that we can mop-up most of the brain damage.
>>
>> Hmmm ... yes I see. I wonder if we should make the setting of the type
>> here dependent upon PM being enabled for an irqchip? We could check to
>> see if the .parent_device is populated and if so only then save the type
>> and otherwise just set it as we do today.
> 
> I don't really like the idea of having multiple code paths for the same thing.
> This is very error prone, and likely to bitrot pretty quickly.

True. However, we really need this change for irqchips and runtime-pm.
So to confirm what are you suggesting we do? We could add a WARN around
irq_set_irq_type() in irq_create_fwspec_mapping() for v4.7 and see how
many complaints we get :-)

>> We could add a WARN to the existing irq_set_irq_type() or may be just a
>> pr_warn() if a WARN is too verbose so people can fix up any issues.
>>
>> I am also wondering if patch 4/17 "iqdomain: Fix handling of type
>> settings for existing mappings" could generate a lot of reports
>> interrupts failing due to bad firmware? I wonder if I should tone this
>> patch down to a warning message as well as opposed to a complete failure.
> 
> We'll see. We can always tone it down a notch, should it prove to be too noisy...
> So far, I haven't seen it firing. On the other hand, I get the following stuff
> on my APM board:
> 
> [    0.000000] GIC: PPI0 is either secure or misconfigured
> [    0.000000] GIC: PPI13 is either secure or misconfigured
> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ1, assuming level low
> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2, assuming level low
> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
> 
> Pretty awesome...

Indeed.

Jon

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

[Marc Zyngier]

On 09/05/16 16:44, Jon Hunter wrote:
> 
> On 09/05/16 16:10, Marc Zyngier wrote:
>> On 09/05/16 14:13, Jon Hunter wrote:
>>> On 09/05/16 13:23, Marc Zyngier wrote:
> 
> [snip]
> 
>>>> This patch have the effect of making misconfigured PPIs absolutely
>>>> obvious. I still need to wrap my head around the root cause, but here's
>>>> the findings I have so far:
>>>>
>>>> - kvmtool generates a DT with the wrong trigger information (edge
>>>> instead of level) for the timer.
>>>> - with this patch applied, "cyclictest -S" reliably locks up when run in
>>>> a guest (missing a timer interrupt, goodbye CPU).
>>>> - Either fixing kvmtool or reverting that patch makes it work reliably
>>>> again.
>>>>
>>>> My gut feeling is that until that patch, the failing irq_set_irq_type()
>>>> wasn't affecting the kernel's view of the trigger (it was still treated
>>>> as level). With this patch, the kernel now trusts whatever is coming
>>>> from the firmware, and the misconfiguration becomes obvious. And just
>>>> grepping through the DT files for arm and arm64 sends makes me thing
>>>> "Holly effin' crap!".
>>>>
>>>> I'm not saying that we shouldn't perform this change though. But it is
>>>> quite obvious that it is going to break an awful lot of existing code
>>>> and platforms. I'm also cooking a small patch for the arch timer (which
>>>> seems to be described in DT with a fairly high level of brokenness), so
>>>> that we can mop-up most of the brain damage.
>>>
>>> Hmmm ... yes I see. I wonder if we should make the setting of the type
>>> here dependent upon PM being enabled for an irqchip? We could check to
>>> see if the .parent_device is populated and if so only then save the type
>>> and otherwise just set it as we do today.
>>
>> I don't really like the idea of having multiple code paths for the same thing.
>> This is very error prone, and likely to bitrot pretty quickly.
> 
> True. However, we really need this change for irqchips and runtime-pm.
> So to confirm what are you suggesting we do? We could add a WARN around
> irq_set_irq_type() in irq_create_fwspec_mapping() for v4.7 and see how
> many complaints we get :-)

Let's add a pr_warn(), and see how noisy this becomes. We may have to
remove it if it becomes too loud though.

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 V3 07/17] genirq: Ensure IRQ descriptor is valid when setting-up the IRQ

[Jon Hunter]

In the function, setup_irq(), we don't check that the descriptor
returned from irq_to_desc() is valid before we start using it. For
example chip_bus_lock() called from setup_irq(), assumes that the
descriptor pointer is valid and doesn't check before dereferencing it.

In many other functions including setup/free_percpu_irq() we do check
that the descriptor returned is not NULL and therefore add the same test
to setup_irq() to ensure the descriptor returned is valid.

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

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index b2a93a37f772..eaedeb74b49d 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1414,7 +1414,7 @@ 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 (!desc || WARN_ON(irq_settings_is_per_cpu_devid(desc)))
 		return -EINVAL;
 	chip_bus_lock(desc);
 	retval = __setup_irq(irq, desc, act);
-- 
2.1.4

[PATCH V3 08/17] 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>
Reviewed-by: Kevin Hilman <khilman@baylibre.com>
---
 include/linux/irq.h    |  4 ++++
 kernel/irq/chip.c      | 35 +++++++++++++++++++++++++++++++++++
 kernel/irq/internals.h |  1 +
 kernel/irq/manage.c    | 31 ++++++++++++++++++++++++++++++-
 4 files changed, 70 insertions(+), 1 deletion(-)

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 eaedeb74b49d..f8fd1fbc02ea 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1416,10 +1416,18 @@ int setup_irq(unsigned int irq, struct irqaction *act)
 
 	if (!desc || WARN_ON(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_put(&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

[PATCH V3 09/17] 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 095bb5b5c3f2..16f632dcc2ad 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1200,10 +1200,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 V3 10/17] 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 16f632dcc2ad..13a0d77e8ae9 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);
@@ -1025,10 +1011,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 V3 11/17] 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 13a0d77e8ae9..82931aa68868 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -998,13 +998,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);
 
@@ -1017,7 +1017,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);
 	}
@@ -1027,17 +1027,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) {
@@ -1049,9 +1048,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);
@@ -1101,8 +1099,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) {
 		/*
@@ -1124,6 +1124,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,
@@ -1184,7 +1196,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;
@@ -1209,8 +1221,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);
 
@@ -1299,7 +1317,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,
@@ -1342,7 +1360,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 V3 12/17] 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 82931aa68868..30666f349649 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -518,34 +518,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);
 }
 
@@ -556,16 +557,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;
@@ -573,7 +575,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++)
@@ -581,85 +583,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);
 
@@ -668,7 +672,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)
@@ -683,18 +687,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 V3 13/17] irqchip/gic: Don't allow early initialisation if GIC requires RPM

[Jon Hunter]

Commit afbbd2338176 ("irqchip/gic: Document optional Clock and Power
Domain properties") updated the device-tree binding documentation for
the GIC to add optional clock and power domain information. Currently,
the GIC driver does support these optional properties and there do not
appear to be any GIC instances that define these.

To support GICs that require runtime power management and hence, define
the clock and/or power-domain properties, a platform driver for GICs
using power management will be added. However, this presents a problem
because by adding a platform driver in addition to the current GIC
driver, we will have two places where we can match the GIC compatibility
string to initialise the GIC and these are:
 1. By the IRQCHIP_DECLARE macro for early initialisation of GICs.
 2. By the platform driver's device-tree match table.

To prevent a GIC which requires power management from being initialised
early (by matching the compatibility string specified by
IRQCHIP_DECLARE), during early inialisation, if we detect the GIC has
either the 'clocks' or 'power-domains' property present bail out of the
early initialisation and allow the platform driver to initialise the
device.

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

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 30666f349649..a10274926690 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1205,6 +1205,15 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	if (WARN_ON(!node))
 		return -ENODEV;
 
+	/*
+	 * If the GIC device has either a 'clocks' node or a 'power-domains'
+	 * node populated, then bail out now because this driver is currently
+	 * unable to support devices which require power management.
+	 */
+	if (of_property_read_bool(node, "clocks") ||
+	    of_property_read_bool(node, "power-domains"))
+		return 0;
+
 	dist_base = of_iomap(node, 0);
 	if (WARN(!dist_base, "unable to map gic dist registers\n"))
 		return -ENOMEM;
-- 
2.1.4

[PATCH V3 14/17] irqchip/gic: Add helper function for configuring a GIC via device-tree

[Jon Hunter]

Move the code that configures a GIC via device-tree into it's own
function. This will allow us to re-use this function when adding a
platform driver to support GICs that require power-management.

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

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a10274926690..2fc5018d6eb9 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1194,6 +1194,28 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 	return true;
 }
 
+static int gic_of_setup(struct device_node *node, void __iomem **dist_base,
+			void __iomem **cpu_base, u32 *percpu_offset)
+{
+	if (!node || !dist_base || !cpu_base || !percpu_offset)
+		return -EINVAL;
+
+	*dist_base = of_iomap(node, 0);
+	if (WARN(!*dist_base, "unable to map gic dist registers\n"))
+		return -ENOMEM;
+
+	*cpu_base = of_iomap(node, 1);
+	if (WARN(!*cpu_base, "unable to map gic cpu registers\n")) {
+		iounmap(*dist_base);
+		return -ENOMEM;
+	}
+
+	if (of_property_read_u32(node, "cpu-offset", percpu_offset))
+		*percpu_offset = 0;
+
+	return 0;
+}
+
 int __init
 gic_of_init(struct device_node *node, struct device_node *parent)
 {
@@ -1214,15 +1236,9 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	    of_property_read_bool(node, "power-domains"))
 		return 0;
 
-	dist_base = of_iomap(node, 0);
-	if (WARN(!dist_base, "unable to map gic dist registers\n"))
-		return -ENOMEM;
-
-	cpu_base = of_iomap(node, 1);
-	if (WARN(!cpu_base, "unable to map gic cpu registers\n")) {
-		iounmap(dist_base);
-		return -ENOMEM;
-	}
+	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
@@ -1231,9 +1247,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) {
-- 
2.1.4

[PATCH V3 15/17] irqchip/gic: Split GIC init in preparation for platform driver

[Jon Hunter]

To re-use the code that initialises the GIC (found in
__gic_init_bases()), from within a 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 initial 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().

To allow the platform driver to dynamically allocate the GIC chip data
structure, rather than passing an index to reference the GIC to
gic_init_bases() pass a pointer to the chip data instead. 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 a platform device.

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

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 2fc5018d6eb9..15e8a12813cc 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1002,32 +1002,29 @@ 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);
+	int gic_irqs, irq_base, ret;
 
-	gic = &gic_data[gic_nr];
+	if (WARN_ON(!gic || gic->domain))
+		return -EINVAL;
 
 	/* 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
 
@@ -1081,7 +1078,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;
@@ -1108,6 +1105,42 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		goto error;
 	}
 
+	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);
+	}
+
+	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 i, 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(name);
+		return ret;
+	}
+
 	if (gic_nr == 0) {
 		/*
 		 * Initialize the CPU interface map to all CPUs.
@@ -1130,16 +1163,6 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 	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,
-- 
2.1.4

[PATCH V3 03/17] 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
---
 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

--
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 V3 16/17] irqchip/gic: Prepare for adding platform driver

[Jon Hunter]

To support GICs that require runtime-pm, it is necessary to add a
platform driver, so that the probing of the chip can be deferred if
resources, such as a power-domain, is not yet available.

To prepare for adding a platform driver:
 1. 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.
 2. Move the definitions for gic_base and gic_chip_data structures to a
    local header files along with prototypes for functions required by
    the platform driver.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/irqchip/irq-gic-common.c |  4 +--
 drivers/irqchip/irq-gic.c        | 57 +++++++++++-------------------------
 drivers/irqchip/irq-gic.h        | 63 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 42 deletions(-)
 create mode 100644 drivers/irqchip/irq-gic.h

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 15e8a12813cc..bf9a256a1269 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -48,6 +48,7 @@
 #include <asm/smp_plat.h>
 #include <asm/virt.h>
 
+#include "irq-gic.h"
 #include "irq-gic-common.h"
 
 #ifdef CONFIG_ARM64
@@ -63,31 +64,6 @@ static void gic_check_cpu_features(void)
 #define gic_check_cpu_features()	do { } while(0)
 #endif
 
-union gic_base {
-	void __iomem *common_base;
-	void __percpu * __iomem *percpu_base;
-};
-
-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)];
-	u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
-	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
-	void __iomem *(*get_base)(union gic_base *);
-#endif
-};
-
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);
 
 /*
@@ -352,7 +328,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
 	} while (1);
 }
 
-static void gic_handle_cascade_irq(struct irq_desc *desc)
+void gic_handle_cascade_irq(struct irq_desc *desc)
 {
 	struct gic_chip_data *chip_data = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
@@ -436,7 +412,7 @@ static void gic_cpu_if_up(struct gic_chip_data *gic)
 }
 
 
-static void __init gic_dist_init(struct gic_chip_data *gic)
+void gic_dist_init(struct gic_chip_data *gic)
 {
 	unsigned int i;
 	u32 cpumask;
@@ -459,7 +435,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
 	writel_relaxed(GICD_ENABLE, base + GIC_DIST_CTRL);
 }
 
-static void gic_cpu_init(struct gic_chip_data *gic)
+void gic_cpu_init(struct gic_chip_data *gic)
 {
 	void __iomem *dist_base = gic_data_dist_base(gic);
 	void __iomem *base = gic_data_cpu_base(gic);
@@ -518,7 +494,7 @@ 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(struct gic_chip_data *gic)
+void gic_dist_save(struct gic_chip_data *gic)
 {
 	unsigned int gic_irqs;
 	void __iomem *dist_base;
@@ -557,7 +533,7 @@ static void gic_dist_save(struct gic_chip_data *gic)
  * 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(struct gic_chip_data *gic)
+void gic_dist_restore(struct gic_chip_data *gic)
 {
 	unsigned int gic_irqs;
 	unsigned int i;
@@ -603,7 +579,7 @@ static void gic_dist_restore(struct gic_chip_data *gic)
 	writel_relaxed(GICD_ENABLE, dist_base + GIC_DIST_CTRL);
 }
 
-static void gic_cpu_save(struct gic_chip_data *gic)
+void gic_cpu_save(struct gic_chip_data *gic)
 {
 	int i;
 	u32 *ptr;
@@ -633,7 +609,7 @@ static void gic_cpu_save(struct gic_chip_data *gic)
 
 }
 
-static void gic_cpu_restore(struct gic_chip_data *gic)
+void gic_cpu_restore(struct gic_chip_data *gic)
 {
 	int i;
 	u32 *ptr;
@@ -710,7 +686,7 @@ static struct notifier_block gic_notifier_block = {
 	.notifier_call = gic_notifier,
 };
 
-static void __init gic_pm_init(struct gic_chip_data *gic)
+void gic_pm_init(struct gic_chip_data *gic)
 {
 	gic->saved_ppi_enable = __alloc_percpu(DIV_ROUND_UP(32, 32) * 4,
 		sizeof(u32));
@@ -728,7 +704,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)
+void gic_pm_init(struct gic_chip_data *gic)
 {
 }
 #endif
@@ -1002,10 +978,10 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 	.unmap = gic_irq_domain_unmap,
 };
 
-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)
+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;
 	int gic_irqs, irq_base, ret;
@@ -1153,6 +1129,7 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		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");
@@ -1217,8 +1194,8 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 	return true;
 }
 
-static int gic_of_setup(struct device_node *node, void __iomem **dist_base,
-			void __iomem **cpu_base, u32 *percpu_offset)
+int gic_of_setup(struct device_node *node, void __iomem **dist_base,
+		 void __iomem **cpu_base, u32 *percpu_offset)
 {
 	if (!node || !dist_base || !cpu_base || !percpu_offset)
 		return -EINVAL;
diff --git a/drivers/irqchip/irq-gic.h b/drivers/irqchip/irq-gic.h
new file mode 100644
index 000000000000..59198d5e7175
--- /dev/null
+++ b/drivers/irqchip/irq-gic.h
@@ -0,0 +1,63 @@
+/*
+ * Copyright (C) 2016 NVIDIA CORPORATION, All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _IRQ_GIC_H
+#define _IRQ_GIC_H
+
+union gic_base {
+	void __iomem *common_base;
+	void __percpu * __iomem *percpu_base;
+};
+
+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)];
+	u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
+	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
+	void __iomem *(*get_base)(union gic_base *);
+#endif
+};
+
+void gic_cpu_init(struct gic_chip_data *gic);
+void gic_cpu_save(struct gic_chip_data *gic);
+void gic_cpu_restore(struct gic_chip_data *gic);
+void gic_dist_init(struct gic_chip_data *gic);
+void gic_dist_save(struct gic_chip_data *gic);
+void gic_dist_restore(struct gic_chip_data *gic);
+void gic_pm_init(struct gic_chip_data *gic);
+
+int gic_of_setup(struct device_node *node, void __iomem **dist_base,
+		 void __iomem **cpu_base, u32 *percpu_offset);
+
+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);
+
+void gic_handle_cascade_irq(struct irq_desc *desc);
+
+#endif /* _IRQ_GIC_H */
-- 
2.1.4

Re: [PATCH V3 16/17] irqchip/gic: Prepare for adding platform driver

[Marc Zyngier]

On 04/05/16 17:25, Jon Hunter wrote:
> To support GICs that require runtime-pm, it is necessary to add a
> platform driver, so that the probing of the chip can be deferred if
> resources, such as a power-domain, is not yet available.
> 
> To prepare for adding a platform driver:
>  1. 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.
>  2. Move the definitions for gic_base and gic_chip_data structures to a
>     local header files along with prototypes for functions required by
>     the platform driver.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/irqchip/irq-gic-common.c |  4 +--
>  drivers/irqchip/irq-gic.c        | 57 +++++++++++-------------------------
>  drivers/irqchip/irq-gic.h        | 63 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 42 deletions(-)
>  create mode 100644 drivers/irqchip/irq-gic.h
> 
> 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 15e8a12813cc..bf9a256a1269 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -48,6 +48,7 @@
>  #include <asm/smp_plat.h>
>  #include <asm/virt.h>
>  
> +#include "irq-gic.h"
>  #include "irq-gic-common.h"
>  
>  #ifdef CONFIG_ARM64
> @@ -63,31 +64,6 @@ static void gic_check_cpu_features(void)
>  #define gic_check_cpu_features()	do { } while(0)
>  #endif
>  
> -union gic_base {
> -	void __iomem *common_base;
> -	void __percpu * __iomem *percpu_base;
> -};
> -
> -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)];
> -	u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
> -	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
> -	void __iomem *(*get_base)(union gic_base *);
> -#endif
> -};
> -
>  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>  
>  /*
> @@ -352,7 +328,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>  	} while (1);
>  }
>  
> -static void gic_handle_cascade_irq(struct irq_desc *desc)
> +void gic_handle_cascade_irq(struct irq_desc *desc)
>  {
>  	struct gic_chip_data *chip_data = irq_desc_get_handler_data(desc);
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> @@ -436,7 +412,7 @@ static void gic_cpu_if_up(struct gic_chip_data *gic)
>  }
>  
>  
> -static void __init gic_dist_init(struct gic_chip_data *gic)
> +void gic_dist_init(struct gic_chip_data *gic)
>  {
>  	unsigned int i;
>  	u32 cpumask;
> @@ -459,7 +435,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
>  	writel_relaxed(GICD_ENABLE, base + GIC_DIST_CTRL);
>  }
>  
> -static void gic_cpu_init(struct gic_chip_data *gic)
> +void gic_cpu_init(struct gic_chip_data *gic)
>  {
>  	void __iomem *dist_base = gic_data_dist_base(gic);
>  	void __iomem *base = gic_data_cpu_base(gic);
> @@ -518,7 +494,7 @@ 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(struct gic_chip_data *gic)
> +void gic_dist_save(struct gic_chip_data *gic)
>  {
>  	unsigned int gic_irqs;
>  	void __iomem *dist_base;
> @@ -557,7 +533,7 @@ static void gic_dist_save(struct gic_chip_data *gic)
>   * 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(struct gic_chip_data *gic)
> +void gic_dist_restore(struct gic_chip_data *gic)
>  {
>  	unsigned int gic_irqs;
>  	unsigned int i;
> @@ -603,7 +579,7 @@ static void gic_dist_restore(struct gic_chip_data *gic)
>  	writel_relaxed(GICD_ENABLE, dist_base + GIC_DIST_CTRL);
>  }
>  
> -static void gic_cpu_save(struct gic_chip_data *gic)
> +void gic_cpu_save(struct gic_chip_data *gic)
>  {
>  	int i;
>  	u32 *ptr;
> @@ -633,7 +609,7 @@ static void gic_cpu_save(struct gic_chip_data *gic)
>  
>  }
>  
> -static void gic_cpu_restore(struct gic_chip_data *gic)
> +void gic_cpu_restore(struct gic_chip_data *gic)
>  {
>  	int i;
>  	u32 *ptr;
> @@ -710,7 +686,7 @@ static struct notifier_block gic_notifier_block = {
>  	.notifier_call = gic_notifier,
>  };
>  
> -static void __init gic_pm_init(struct gic_chip_data *gic)
> +void gic_pm_init(struct gic_chip_data *gic)
>  {
>  	gic->saved_ppi_enable = __alloc_percpu(DIV_ROUND_UP(32, 32) * 4,
>  		sizeof(u32));
> @@ -728,7 +704,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)
> +void gic_pm_init(struct gic_chip_data *gic)
>  {
>  }
>  #endif
> @@ -1002,10 +978,10 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
>  	.unmap = gic_irq_domain_unmap,
>  };
>  
> -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)
> +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;
>  	int gic_irqs, irq_base, ret;
> @@ -1153,6 +1129,7 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
>  		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");
> @@ -1217,8 +1194,8 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>  	return true;
>  }
>  
> -static int gic_of_setup(struct device_node *node, void __iomem **dist_base,
> -			void __iomem **cpu_base, u32 *percpu_offset)
> +int gic_of_setup(struct device_node *node, void __iomem **dist_base,
> +		 void __iomem **cpu_base, u32 *percpu_offset)
>  {
>  	if (!node || !dist_base || !cpu_base || !percpu_offset)
>  		return -EINVAL;
> diff --git a/drivers/irqchip/irq-gic.h b/drivers/irqchip/irq-gic.h
> new file mode 100644
> index 000000000000..59198d5e7175
> --- /dev/null
> +++ b/drivers/irqchip/irq-gic.h
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (C) 2016 NVIDIA CORPORATION, All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef _IRQ_GIC_H
> +#define _IRQ_GIC_H
> +
> +union gic_base {
> +	void __iomem *common_base;
> +	void __percpu * __iomem *percpu_base;
> +};
> +
> +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)];
> +	u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
> +	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
> +	void __iomem *(*get_base)(union gic_base *);
> +#endif
> +};

Gahhh. No. Please. Last time we did that, it took 6 months to untangle
the mess people made by adding their own hacks in this structure, 
so I definitely want to keep it completely private, forever. Same goes
for the gic_{dist,cpu.pm}_init() functions.

I've had a go at this, and came up with the following patch. I've only
briefly tested it on a host and a VM, so it is likely to break some stuff
somewhere, but you'll get the idea: The gic_chip_data struct is entirely
opaque, allocated by the GIC driver itself, with a few new fields in
it so that it becomes self-contained. This applies on top of your series.

It should also make it easy to switch to a model where we allocate
the structure dynamically instead of the old static crap.

Thoughts?

	M.

diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
index 0a86da6..c4d0621 100644
--- a/drivers/irqchip/irq-gic-pm.c
+++ b/drivers/irqchip/irq-gic-pm.c
@@ -97,9 +97,6 @@ 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);
@@ -108,16 +105,10 @@ static int gic_probe(struct platform_device *pdev)
 		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);
@@ -131,21 +122,16 @@ static int gic_probe(struct platform_device *pdev)
 		goto rpm_put;
 	}
 
-	ret = gic_of_setup(dev->of_node, &dist_base, &cpu_base, &percpu_offset);
+	ret = gic_of_setup(dev->of_node, dev, &gic);
 	if (ret)
 		goto irq_dispose;
 
-	ret = gic_init_bases(gic, -1, dist_base, cpu_base,
-			     percpu_offset, &dev->of_node->fwnode,
+	ret = gic_init_bases(gic, -1, gic, &dev->of_node->fwnode,
 			     dev->of_node->name);
 	if (ret)
 		goto gic_unmap;
 
-	gic_dist_init(gic);
-	gic_cpu_init(gic);
-	gic_pm_init(gic);
-
-	gic->chip.parent_device = dev;
+	platform_set_drvdata(pdev, gic);
 
 	irq_set_chained_handler_and_data(irq, gic_handle_cascade_irq, gic);
 
@@ -156,8 +142,7 @@ static int gic_probe(struct platform_device *pdev)
 	return 0;
 
 gic_unmap:
-	iounmap(dist_base);
-	iounmap(cpu_base);
+	gic_of_teardown(gic);
 irq_dispose:
 	irq_dispose_mapping(irq);
 rpm_put:
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 5108a85..e779c5d 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -51,6 +51,34 @@
 #include "irq-gic.h"
 #include "irq-gic-common.h"
 
+union gic_base {
+	void __iomem *common_base;
+	void __percpu * __iomem *percpu_base;
+};
+
+struct gic_chip_data {
+	struct irq_chip chip;
+	union gic_base dist_base;
+	union gic_base cpu_base;
+	void __iomem *raw_dist_base;
+	void __iomem *raw_cpu_base;
+	u32 percpu_offset;
+#if defined(CONFIG_CPU_PM) || defined(ARM_GIC_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)];
+	u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
+	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
+	void __iomem *(*get_base)(union gic_base *);
+#endif
+};
+
 #ifdef CONFIG_ARM64
 #include <asm/cpufeature.h>
 
@@ -420,7 +448,7 @@ static void gic_cpu_if_up(struct gic_chip_data *gic)
 }
 
 
-void gic_dist_init(struct gic_chip_data *gic)
+static void gic_dist_init(struct gic_chip_data *gic)
 {
 	unsigned int i;
 	u32 cpumask;
@@ -443,7 +471,7 @@ void gic_dist_init(struct gic_chip_data *gic)
 	writel_relaxed(GICD_ENABLE, base + GIC_DIST_CTRL);
 }
 
-void gic_cpu_init(struct gic_chip_data *gic)
+static void gic_cpu_init(struct gic_chip_data *gic)
 {
 	void __iomem *dist_base = gic_data_dist_base(gic);
 	void __iomem *base = gic_data_cpu_base(gic);
@@ -693,7 +721,7 @@ static struct notifier_block gic_notifier_block = {
 	.notifier_call = gic_notifier,
 };
 
-void 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));
@@ -711,7 +739,7 @@ void gic_pm_init(struct gic_chip_data *gic)
 		cpu_pm_register_notifier(&gic_notifier_block);
 }
 #else
-void gic_pm_init(struct gic_chip_data *gic)
+static void gic_pm_init(struct gic_chip_data *gic)
 {
 }
 #endif
@@ -986,9 +1014,7 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 };
 
 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)
+		   struct fwnode_handle *handle, const char *name)
 {
 	irq_hw_number_t hwirq_base;
 	int gic_irqs, irq_base, ret;
@@ -1013,7 +1039,7 @@ int gic_init_bases(struct gic_chip_data *gic, int irq_start,
 		gic->chip.irq_set_affinity = gic_set_affinity;
 #endif
 
-	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
+	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && gic->percpu_offset) {
 		/* Frankein-GIC without banked registers... */
 		unsigned int cpu;
 
@@ -1028,19 +1054,19 @@ int gic_init_bases(struct gic_chip_data *gic, int irq_start,
 		for_each_possible_cpu(cpu) {
 			u32 mpidr = cpu_logical_map(cpu);
 			u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
-			unsigned long offset = percpu_offset * core_id;
-			*per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
-			*per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
+			unsigned long offset = gic->percpu_offset * core_id;
+			*per_cpu_ptr(gic->dist_base.percpu_base, cpu) = gic->raw_dist_base + offset;
+			*per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = gic->raw_cpu_base + offset;
 		}
 
 		gic_set_base_accessor(gic, gic_get_percpu_base);
 	} else {
 		/* Normal, sane GIC... */
-		WARN(percpu_offset,
+		WARN(gic->percpu_offset,
 		     "GIC_NON_BANKED not enabled, ignoring %08x offset!",
-		     percpu_offset);
-		gic->dist_base.common_base = dist_base;
-		gic->cpu_base.common_base = cpu_base;
+		     gic->percpu_offset);
+		gic->dist_base.common_base = gic->raw_dist_base;
+		gic->cpu_base.common_base = gic->raw_cpu_base;
 		gic_set_base_accessor(gic, gic_get_common_base);
 	}
 
@@ -1090,10 +1116,14 @@ int gic_init_bases(struct gic_chip_data *gic, int irq_start,
 		goto error;
 	}
 
+	gic_dist_init(gic);
+	gic_cpu_init(gic);
+	gic_pm_init(gic);
+
 	return 0;
 
 error:
-	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
+	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && gic->percpu_offset) {
 		free_percpu(gic->dist_base.percpu_base);
 		free_percpu(gic->cpu_base.percpu_base);
 	}
@@ -1101,37 +1131,24 @@ error:
 	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)
+static int __init __gic_init_bases(struct gic_chip_data *gic, int irq_start,
+				   struct fwnode_handle *handle)
 {
-	struct gic_chip_data *gic;
 	char *name;
-	int i, ret;
-
-	if (WARN_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR))
-		return -EINVAL;
-
-	gic = &gic_data[gic_nr];
+	int ret;
 
-	if (static_key_true(&supports_deactivate) && gic_nr == 0)
+	if (static_key_true(&supports_deactivate) && gic == &gic_data[0])
 		name = kasprintf(GFP_KERNEL, "GICv2");
 	else
-		name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr);
+		name = kasprintf(GFP_KERNEL, "GIC-%d", (int)(gic - &gic_data[0]));
 
-	ret = gic_init_bases(gic, irq_start, dist_base, cpu_base, percpu_offset,
-			     handle, name);
-	if (ret) {
-		kfree(name);
-		return ret;
-	}
-
-	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.
 		 * This is only necessary for the primary GIC.
 		 */
+		int i;
 		for (i = 0; i < NR_GIC_CPU_IF; i++)
 			gic_cpu_map[i] = 0xff;
 #ifdef CONFIG_SMP
@@ -1144,22 +1161,26 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 			pr_info("GIC: Using split EOI/Deactivate mode\n");
 	}
 
-	gic_dist_init(gic);
-	gic_cpu_init(gic);
-	gic_pm_init(gic);
+	ret = gic_init_bases(gic, irq_start, handle, name);
+	if (ret)
+		kfree(name);
 
-	return 0;
+	return ret;
 }
 
 void __init gic_init(unsigned int gic_nr, int irq_start,
 		     void __iomem *dist_base, void __iomem *cpu_base)
 {
+	struct gic_chip_data *gic = &gic_data[gic_nr];
+
 	/*
 	 * Non-DT/ACPI systems won't run a hypervisor, so let's not
 	 * bother with these...
 	 */
 	static_key_slow_dec(&supports_deactivate);
-	__gic_init_bases(gic_nr, irq_start, dist_base, cpu_base, 0, NULL);
+	gic->raw_dist_base = dist_base;
+	gic->raw_cpu_base = cpu_base;
+	__gic_init_bases(gic, irq_start, NULL);
 }
 
 #ifdef CONFIG_OF
@@ -1203,34 +1224,52 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 	return true;
 }
 
-int gic_of_setup(struct device_node *node, void __iomem **dist_base,
-		 void __iomem **cpu_base, u32 *percpu_offset)
+void gic_of_teardown(struct gic_chip_data *gic)
 {
-	if (!node || !dist_base || !cpu_base || !percpu_offset)
-		return -EINVAL;
+	if (gic->raw_dist_base)
+		iounmap(gic->raw_dist_base);
+	if (gic->raw_cpu_base)
+		iounmap(gic->raw_cpu_base);
+}
 
-	*dist_base = of_iomap(node, 0);
-	if (WARN(!*dist_base, "unable to map gic dist registers\n"))
-		return -ENOMEM;
+int gic_of_setup(struct device_node *node, struct device *dev,
+		 struct gic_chip_data **gicp)
+{
+	struct gic_chip_data *gic;
 
-	*cpu_base = of_iomap(node, 1);
-	if (WARN(!*cpu_base, "unable to map gic cpu registers\n")) {
-		iounmap(*dist_base);
-		return -ENOMEM;
+	if (!node || !gicp)
+		return -EINVAL;
+
+	if (dev) {
+		*gicp = devm_kzalloc(dev, sizeof(*gic), GFP_KERNEL);
+		if (!*gicp)
+			return -ENOMEM;
 	}
 
-	if (of_property_read_u32(node, "cpu-offset", percpu_offset))
-		*percpu_offset = 0;
+	gic = *gicp;
+
+	gic->raw_dist_base = of_iomap(node, 0);
+	if (WARN(!gic->raw_dist_base, "unable to map gic dist registers\n"))
+		goto err;
+
+	gic->raw_cpu_base = of_iomap(node, 1);
+	if (WARN(!gic->raw_cpu_base, "unable to map gic cpu registers\n"))
+		goto err;
+
+	if (of_property_read_u32(node, "cpu-offset", &gic->percpu_offset))
+		gic->percpu_offset = 0;
 
+	gic->chip.parent_device = dev;
 	return 0;
+err:
+	gic_of_teardown(gic);
+	return -ENOMEM;
 }
 
 int __init
 gic_of_init(struct device_node *node, struct device_node *parent)
 {
-	void __iomem *cpu_base;
-	void __iomem *dist_base;
-	u32 percpu_offset;
+	struct gic_chip_data *gic;
 	int irq, ret;
 
 	if (WARN_ON(!node))
@@ -1245,7 +1284,8 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	    of_property_read_bool(node, "power-domains"))
 		return 0;
 
-	ret = gic_of_setup(node, &dist_base, &cpu_base, &percpu_offset);
+	gic = &gic_data[gic_cnt];
+	ret = gic_of_setup(node, NULL, &gic);
 	if (ret)
 		return ret;
 
@@ -1253,14 +1293,13 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	 * Disable split EOI/Deactivate if either HYP is not available
 	 * or the CPU interface is too small.
 	 */
-	if (gic_cnt == 0 && !gic_check_eoimode(node, &cpu_base))
+	if (gic_cnt == 0 && !gic_check_eoimode(node, &gic->raw_cpu_base))
 		static_key_slow_dec(&supports_deactivate);
 
-	ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
-			 &node->fwnode);
+	ret = __gic_init_bases(gic, -1, &node->fwnode);
 	if (ret) {
-		iounmap(dist_base);
-		iounmap(cpu_base);
+		iounmap(gic->raw_dist_base);
+		iounmap(gic->raw_cpu_base);
 		return ret;
 	}
 
@@ -1395,7 +1434,9 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
 		return -ENOMEM;
 	}
 
-	ret = __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle);
+	gic_data[0].raw_dist_base = dist_base;
+	gic_data[0].raw_cpu_base = cpu_base;
+	ret = __gic_init_bases(&gic_data[0], -1, domain_handle);
 	if (ret) {
 		pr_err("Failed to initialise GIC\n");
 		irq_domain_free_fwnode(domain_handle);
diff --git a/drivers/irqchip/irq-gic.h b/drivers/irqchip/irq-gic.h
index 31e7733..77d4001 100644
--- a/drivers/irqchip/irq-gic.h
+++ b/drivers/irqchip/irq-gic.h
@@ -17,46 +17,18 @@
 #ifndef _IRQ_GIC_H
 #define _IRQ_GIC_H
 
-union gic_base {
-	void __iomem *common_base;
-	void __percpu * __iomem *percpu_base;
-};
+struct gic_chip_data;
 
-struct gic_chip_data {
-	struct irq_chip chip;
-	union gic_base dist_base;
-	union gic_base cpu_base;
-#if defined(CONFIG_CPU_PM) || defined(ARM_GIC_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)];
-	u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
-	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
-	void __iomem *(*get_base)(union gic_base *);
-#endif
-};
-
-void gic_cpu_init(struct gic_chip_data *gic);
 void gic_cpu_save(struct gic_chip_data *gic);
 void gic_cpu_restore(struct gic_chip_data *gic);
-void gic_dist_init(struct gic_chip_data *gic);
 void gic_dist_save(struct gic_chip_data *gic);
 void gic_dist_restore(struct gic_chip_data *gic);
-void gic_pm_init(struct gic_chip_data *gic);
-
-int gic_of_setup(struct device_node *node, void __iomem **dist_base,
-		 void __iomem **cpu_base, u32 *percpu_offset);
 
+int gic_of_setup(struct device_node *node, struct device *dev,
+		 struct gic_chip_data **gic);
+void gic_of_teardown(struct gic_chip_data *gic);
 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);
+		   struct fwnode_handle *handle, const char *name);
 
 void gic_handle_cascade_irq(struct irq_desc *desc);
 

-- 
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 V3 16/17] irqchip/gic: Prepare for adding platform driver

[Jon Hunter]

Hi Marc,

On 05/05/16 15:13, Marc Zyngier wrote:

[...]

> Gahhh. No. Please. Last time we did that, it took 6 months to untangle
> the mess people made by adding their own hacks in this structure, 
> so I definitely want to keep it completely private, forever. Same goes
> for the gic_{dist,cpu.pm}_init() functions.

OK.

> I've had a go at this, and came up with the following patch. I've only
> briefly tested it on a host and a VM, so it is likely to break some stuff
> somewhere, but you'll get the idea: The gic_chip_data struct is entirely
> opaque, allocated by the GIC driver itself, with a few new fields in
> it so that it becomes self-contained. This applies on top of your series.
> 
> It should also make it easy to switch to a model where we allocate
> the structure dynamically instead of the old static crap.
> 
> Thoughts?

Yes I have been doing some testing and with a couple tweaks we can make
something like this work. One thing that caught me out was ...

> +int gic_of_setup(struct device_node *node, struct device *dev,
> +		 struct gic_chip_data **gicp)
> +{
> +	struct gic_chip_data *gic;
>  
> -	*cpu_base = of_iomap(node, 1);
> -	if (WARN(!*cpu_base, "unable to map gic cpu registers\n")) {
> -		iounmap(*dist_base);
> -		return -ENOMEM;
> +	if (!node || !gicp)
> +		return -EINVAL;
> +
> +	if (dev) {
> +		*gicp = devm_kzalloc(dev, sizeof(*gic), GFP_KERNEL);
> +		if (!*gicp)
> +			return -ENOMEM;
>  	}
>  
> -	if (of_property_read_u32(node, "cpu-offset", percpu_offset))
> -		*percpu_offset = 0;
> +	gic = *gicp;
> +
> +	gic->raw_dist_base = of_iomap(node, 0);
> +	if (WARN(!gic->raw_dist_base, "unable to map gic dist registers\n"))
> +		goto err;
> +
> +	gic->raw_cpu_base = of_iomap(node, 1);
> +	if (WARN(!gic->raw_cpu_base, "unable to map gic cpu registers\n"))
> +		goto err;
> +
> +	if (of_property_read_u32(node, "cpu-offset", &gic->percpu_offset))
> +		gic->percpu_offset = 0;
>  
> +	gic->chip.parent_device = dev;

We can't initialise the device here as it gets overwritten in the
gic_init_bases. So I have had to re-organise things a bit. Good news is
that I have eliminated the call from the platform driver to
gic_init_bases so we only have a single call to initialise the GIC.

Cheers
Jon

Re: [PATCH V3 16/17] irqchip/gic: Prepare for adding platform driver

[Marc Zyngier]

On Fri, 6 May 2016 15:09:51 +0100
Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:

Hi Jon,

[...]

> > +int gic_of_setup(struct device_node *node, struct device *dev,
> > +		 struct gic_chip_data **gicp)
> > +{
> > +	struct gic_chip_data *gic;
> >  
> > -	*cpu_base = of_iomap(node, 1);
> > -	if (WARN(!*cpu_base, "unable to map gic cpu registers\n")) {
> > -		iounmap(*dist_base);
> > -		return -ENOMEM;
> > +	if (!node || !gicp)
> > +		return -EINVAL;
> > +
> > +	if (dev) {
> > +		*gicp = devm_kzalloc(dev, sizeof(*gic), GFP_KERNEL);
> > +		if (!*gicp)
> > +			return -ENOMEM;
> >  	}
> >  
> > -	if (of_property_read_u32(node, "cpu-offset", percpu_offset))
> > -		*percpu_offset = 0;
> > +	gic = *gicp;
> > +
> > +	gic->raw_dist_base = of_iomap(node, 0);
> > +	if (WARN(!gic->raw_dist_base, "unable to map gic dist registers\n"))
> > +		goto err;
> > +
> > +	gic->raw_cpu_base = of_iomap(node, 1);
> > +	if (WARN(!gic->raw_cpu_base, "unable to map gic cpu registers\n"))
> > +		goto err;
> > +
> > +	if (of_property_read_u32(node, "cpu-offset", &gic->percpu_offset))
> > +		gic->percpu_offset = 0;
> >  
> > +	gic->chip.parent_device = dev;
> 
> We can't initialise the device here as it gets overwritten in the
> gic_init_bases. So I have had to re-organise things a bit. Good news is
> that I have eliminated the call from the platform driver to
> gic_init_bases so we only have a single call to initialise the GIC.

Ah, good. That sounds a lot better. Looking forward to v4.

Thanks,

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

[PATCH V3 17/17] irqchip/gic: Add platform driver for non-root GICs that require RPM

[Jon Hunter]

Add a platform driver to support non-root GICs that require runtime
power-management. Currently, only non-root GICs are supported because
the functions, smp_cross_call() and set_handle_irq(), that need to
be called for a root controller are located in the __init section and
so cannot be called by the platform driver.

The GIC platform driver re-uses many functions from the existing GIC
driver including some functions to save and restore the GIC context
during power transitions. The functions for saving and restoring the
GIC context are currently only defined if CONFIG_CPU_PM is enabled and
to ensure that these functions are always defined when the platform
driver is enabled, a dependency on CONFIG_ARM_GIC_PM (which selects the
platform driver) has been added.

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>
---
 drivers/irqchip/Kconfig      |   6 ++
 drivers/irqchip/Makefile     |   1 +
 drivers/irqchip/irq-gic-pm.c | 241 +++++++++++++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-gic.c    |   2 +-
 drivers/irqchip/irq-gic.h    |   2 +-
 5 files changed, 250 insertions(+), 2 deletions(-)
 create mode 100644 drivers/irqchip/irq-gic-pm.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 1ab632a94db3..58d2cd197fff 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -8,6 +8,12 @@ config ARM_GIC
 	select IRQ_DOMAIN_HIERARCHY
 	select MULTI_IRQ_HANDLER
 
+config ARM_GIC_PM
+	bool
+	depends on PM
+	select ARM_GIC
+	select PM_CLK
+
 config ARM_GIC_MAX_NR
 	int
 	default 2 if ARCH_REALVIEW
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 9d54d53fe223..3803fdc9366d 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun4i.o
 obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi-nmi.o
 obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
 obj-$(CONFIG_ARM_GIC)			+= irq-gic.o irq-gic-common.o
+obj-$(CONFIG_ARM_GIC_PM)		+= irq-gic-pm.o
 obj-$(CONFIG_REALVIEW_DT)		+= irq-gic-realview.o
 obj-$(CONFIG_ARM_GIC_V2M)		+= irq-gic-v2m.o
 obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-common.o
diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
new file mode 100644
index 000000000000..0a86da6c5afd
--- /dev/null
+++ b/drivers/irqchip/irq-gic-pm.c
@@ -0,0 +1,241 @@
+/*
+ * Copyright (C) 2016 NVIDIA CORPORATION, All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/pm_clock.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#include "irq-gic.h"
+
+struct gic_clk_data {
+	unsigned int num_clocks;
+	const char *const *clocks;
+};
+
+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) {
+		dev_err(dev, "no parent interrupt found!\n");
+		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_dist_init(gic);
+	gic_cpu_init(gic);
+	gic_pm_init(gic);
+
+	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 arm11mp_gic_clocks[] = {
+	"ic_clk",
+};
+
+static const struct gic_clk_data arm11mp_gic_data = {
+	.num_clocks = ARRAY_SIZE(arm11mp_gic_clocks),
+	.clocks = arm11mp_gic_clocks,
+};
+
+static const char * const cortexa15_gic_clocks[] = {
+	"PERIPHCLKEN",
+};
+
+static const struct gic_clk_data cortexa15_gic_data = {
+	.num_clocks = ARRAY_SIZE(cortexa15_gic_clocks),
+	.clocks = cortexa15_gic_clocks,
+};
+
+static const char * const cortexa9_gic_clocks[] = {
+	"PERIPHCLK", "PERIPHCLKEN",
+};
+
+static const struct gic_clk_data cortexa9_gic_data = {
+	.num_clocks = ARRAY_SIZE(cortexa9_gic_clocks),
+	.clocks = cortexa15_gic_clocks,
+};
+
+static const char * const gic400_clocks[] = {
+	"clk",
+};
+
+static const struct gic_clk_data gic400_data = {
+	.num_clocks = ARRAY_SIZE(gic400_clocks),
+	.clocks = gic400_clocks,
+};
+
+static const char * const pl390_clocks[] = {
+	"gclk",
+};
+
+static const struct gic_clk_data pl390_data = {
+	.num_clocks = ARRAY_SIZE(pl390_clocks),
+	.clocks = pl390_clocks,
+};
+
+static const struct of_device_id gic_match[] = {
+	{ .compatible = "arm,arm11mp-gic",	.data = &arm11mp_gic_data   },
+	{ .compatible = "arm,cortex-a15-gic",	.data = &cortexa15_gic_data },
+	{ .compatible = "arm,cortex-a9-gic",	.data = &cortexa9_gic_data  },
+	{ .compatible = "arm,gic-400",		.data = &gic400_data	    },
+	{ .compatible = "arm,pl390",		.data = &pl390_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);
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index bf9a256a1269..6059daf4f4c8 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -487,7 +487,7 @@ int gic_cpu_if_down(unsigned int gic_nr)
 	return 0;
 }
 
-#ifdef CONFIG_CPU_PM
+#if defined(CONFIG_CPU_PM) || defined(ARM_GIC_PM)
 /*
  * Saves the GIC distributor registers during suspend or idle.  Must be called
  * with interrupts disabled but before powering down the GIC.  After calling
diff --git a/drivers/irqchip/irq-gic.h b/drivers/irqchip/irq-gic.h
index 59198d5e7175..31e77338a798 100644
--- a/drivers/irqchip/irq-gic.h
+++ b/drivers/irqchip/irq-gic.h
@@ -26,7 +26,7 @@ struct gic_chip_data {
 	struct irq_chip chip;
 	union gic_base dist_base;
 	union gic_base cpu_base;
-#ifdef CONFIG_CPU_PM
+#if defined(CONFIG_CPU_PM) || defined(ARM_GIC_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)];
-- 
2.1.4