[PATCH] irqchip/gic: restore global interrupts group settings in distributor

[PATCH] irqchip/gic: restore global interrupts group settings in distributor

[Anson Huang]

In GIC's distributor initializtion, all global interrupts
are set to group 1, however, after suspend/resume with
ARM/GIC power off/on, distributor does NOT restore
these global interrupts group setting, it will cause
system fail to resume.

This patch adds global interrupts group setting restore
for distributor.

Signed-off-by: Anson Huang <b20788@freescale.com>
---
 drivers/irqchip/irq-gic.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a530d9a..c8fa6ee 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -532,6 +532,16 @@ static void gic_dist_restore(unsigned int gic_nr)
 		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
 			dist_base + GIC_DIST_ENABLE_SET + i * 4);
 
+	writel_relaxed(GICD_ENABLE_GRP1, dist_base + GIC_DIST_CTRL);
+
+	/*
+	 * Optionally set all global interrupts to be group 1.
+	 */
+	if (readl_relaxed(dist_base + GIC_DIST_CTRL) & GICD_ENABLE_GRP1) {
+		for (i = 32; i < gic_irqs; i += 32)
+			writel_relaxed(0xffffffff, dist_base + GIC_DIST_IGROUP + i * 4 / 32);
+	}
+
 	writel_relaxed(GICD_ENABLE | GICD_ENABLE_GRP1, dist_base + GIC_DIST_CTRL);
 }
 
-- 
1.9.1

Re: [PATCH] irqchip/gic: restore global interrupts group settings in distributor

[Marc Zyngier]

Hi Anson,

On 05/08/15 17:39, Anson Huang wrote:
> In GIC's distributor initializtion, all global interrupts
> are set to group 1, however, after suspend/resume with
> ARM/GIC power off/on, distributor does NOT restore
> these global interrupts group setting, it will cause
> system fail to resume.
> 
> This patch adds global interrupts group setting restore
> for distributor.
> 
> Signed-off-by: Anson Huang <b20788@freescale.com>
> ---
>  drivers/irqchip/irq-gic.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index a530d9a..c8fa6ee 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -532,6 +532,16 @@ static void gic_dist_restore(unsigned int gic_nr)
>  		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
>  			dist_base + GIC_DIST_ENABLE_SET + i * 4);
>  
> +	writel_relaxed(GICD_ENABLE_GRP1, dist_base + GIC_DIST_CTRL);
> +
> +	/*
> +	 * Optionally set all global interrupts to be group 1.
> +	 */
> +	if (readl_relaxed(dist_base + GIC_DIST_CTRL) & GICD_ENABLE_GRP1) {
> +		for (i = 32; i < gic_irqs; i += 32)
> +			writel_relaxed(0xffffffff, dist_base + GIC_DIST_IGROUP + i * 4 / 32);
> +	}
> +
>  	writel_relaxed(GICD_ENABLE | GICD_ENABLE_GRP1, dist_base + GIC_DIST_CTRL);
>  }
>  
> 

I'm afraid you'll have to explain a few more things here.

For GICv1/v2, we exclusively use Group0 interrupts when booted in secure
mode (i.e. we don't use FIQ yet, but RMK and Daniel Thompson have
patches for that). When booted in non-secure, the group configuration is
not accessible (it is secure only).

So the first case is not applicable yet, and the second one is not
possible. Which side are you on?

Thanks,

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

Re: [PATCH] irqchip/gic: restore global interrupts group settings in distributor

[Anson Huang]

On Wed, Aug 05, 2015 at 10:12:35AM +0100, Marc Zyngier wrote:
> Hi Anson,
> 
> On 05/08/15 17:39, Anson Huang wrote:
> > In GIC's distributor initializtion, all global interrupts
> > are set to group 1, however, after suspend/resume with
> > ARM/GIC power off/on, distributor does NOT restore
> > these global interrupts group setting, it will cause
> > system fail to resume.
> > 
> > This patch adds global interrupts group setting restore
> > for distributor.
> > 
> > Signed-off-by: Anson Huang <b20788@freescale.com>
> > ---
> >  drivers/irqchip/irq-gic.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index a530d9a..c8fa6ee 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -532,6 +532,16 @@ static void gic_dist_restore(unsigned int gic_nr)
> >  		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
> >  			dist_base + GIC_DIST_ENABLE_SET + i * 4);
> >  
> > +	writel_relaxed(GICD_ENABLE_GRP1, dist_base + GIC_DIST_CTRL);
> > +
> > +	/*
> > +	 * Optionally set all global interrupts to be group 1.
> > +	 */
> > +	if (readl_relaxed(dist_base + GIC_DIST_CTRL) & GICD_ENABLE_GRP1) {
> > +		for (i = 32; i < gic_irqs; i += 32)
> > +			writel_relaxed(0xffffffff, dist_base + GIC_DIST_IGROUP + i * 4 / 32);
> > +	}
> > +
> >  	writel_relaxed(GICD_ENABLE | GICD_ENABLE_GRP1, dist_base + GIC_DIST_CTRL);
> >  }
> >  
> > 
> 
> I'm afraid you'll have to explain a few more things here.
> 
> For GICv1/v2, we exclusively use Group0 interrupts when booted in secure
> mode (i.e. we don't use FIQ yet, but RMK and Daniel Thompson have
> patches for that). When booted in non-secure, the group configuration is
> not accessible (it is secure only).
> 
> So the first case is not applicable yet, and the second one is not
> possible. Which side are you on?
> 
> Thanks,
> 
> 	M.

Hi, Marc
	I may NOT know the history of enabling secure/non-secure of GIC very well, will spend
some time look into it later, during the upstream of our i.MX6UL's suepnd/resume,
I found patch(19bcd001 ARM: cobble together FIQ backtracing) break kernel resume function. After
looking into the gic driver, I saw the dist_inti set all global interrupts to GROUP 1(non-secure), but in
dist_restore, it does NOT restore those settings, after restoring them, suspend/resume works.

	Don't we need to make this GIC distributor settings after resume aligned with before suspend?
Before suspend, global interrupts are set to non-secure, but after resume, it is by default set to
secure mode, while cpu controller signal secure interrupts to FIQn, so that is incorrect, not sure
if my understanding is correct, please correct me if I am wrong.

	Thanks.
Anson

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

Re: [PATCH] irqchip/gic: restore global interrupts group settings in distributor

[Marc Zyngier]

On 05/08/15 10:17, Anson Huang wrote:
> On Wed, Aug 05, 2015 at 10:12:35AM +0100, Marc Zyngier wrote:
>> Hi Anson,
>>
>> On 05/08/15 17:39, Anson Huang wrote:
>>> In GIC's distributor initializtion, all global interrupts
>>> are set to group 1, however, after suspend/resume with
>>> ARM/GIC power off/on, distributor does NOT restore
>>> these global interrupts group setting, it will cause
>>> system fail to resume.
>>>
>>> This patch adds global interrupts group setting restore
>>> for distributor.
>>>
>>> Signed-off-by: Anson Huang <b20788@freescale.com>
>>> ---
>>>  drivers/irqchip/irq-gic.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>> index a530d9a..c8fa6ee 100644
>>> --- a/drivers/irqchip/irq-gic.c
>>> +++ b/drivers/irqchip/irq-gic.c
>>> @@ -532,6 +532,16 @@ static void gic_dist_restore(unsigned int gic_nr)
>>>  		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
>>>  			dist_base + GIC_DIST_ENABLE_SET + i * 4);
>>>  
>>> +	writel_relaxed(GICD_ENABLE_GRP1, dist_base + GIC_DIST_CTRL);
>>> +
>>> +	/*
>>> +	 * Optionally set all global interrupts to be group 1.
>>> +	 */
>>> +	if (readl_relaxed(dist_base + GIC_DIST_CTRL) & GICD_ENABLE_GRP1) {
>>> +		for (i = 32; i < gic_irqs; i += 32)
>>> +			writel_relaxed(0xffffffff, dist_base + GIC_DIST_IGROUP + i * 4 / 32);
>>> +	}
>>> +
>>>  	writel_relaxed(GICD_ENABLE | GICD_ENABLE_GRP1, dist_base + GIC_DIST_CTRL);
>>>  }
>>>  
>>>
>>
>> I'm afraid you'll have to explain a few more things here.
>>
>> For GICv1/v2, we exclusively use Group0 interrupts when booted in secure
>> mode (i.e. we don't use FIQ yet, but RMK and Daniel Thompson have
>> patches for that). When booted in non-secure, the group configuration is
>> not accessible (it is secure only).
>>
>> So the first case is not applicable yet, and the second one is not
>> possible. Which side are you on?
>>
>> Thanks,
>>
>> 	M.
> 
> Hi, Marc
> 	I may NOT know the history of enabling secure/non-secure of GIC very well, will spend
> some time look into it later, during the upstream of our i.MX6UL's suepnd/resume,
> I found patch(19bcd001 ARM: cobble together FIQ backtracing) break kernel resume function. After
> looking into the gic driver, I saw the dist_inti set all global interrupts to GROUP 1(non-secure), but in
> dist_restore, it does NOT restore those settings, after restoring them, suspend/resume works.

Right, this is the code from Russell and Daniel I was talking about. As
it says in the commit message:

  Cobble the FIQ backtracing together, some of this patch is based on
  some of Daniel Thompson's work.  Experimental, and not for submission
  yet.

So this patch doesn't apply to mainline.

> 	Don't we need to make this GIC distributor settings after resume aligned with before suspend?
> Before suspend, global interrupts are set to non-secure, but after resume, it is by default set to
> secure mode, while cpu controller signal secure interrupts to FIQn, so that is incorrect, not sure
> if my understanding is correct, please correct me if I am wrong.

You're correct that something has to be done. But this should be done as
part of Russell and Daniel FIQ series.

Russell, are you willing to carry that patch (or something similar) as
part of your series?

Thanks,

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

Re: [PATCH] irqchip/gic: restore global interrupts group settings in distributor

[Russell King - ARM Linux]

On Wed, Aug 05, 2015 at 10:59:10AM +0100, Marc Zyngier wrote:
> On 05/08/15 10:17, Anson Huang wrote:
> > On Wed, Aug 05, 2015 at 10:12:35AM +0100, Marc Zyngier wrote:
> > 	I may NOT know the history of enabling secure/non-secure of GIC very well, will spend
> > some time look into it later, during the upstream of our i.MX6UL's suepnd/resume,
> > I found patch(19bcd001 ARM: cobble together FIQ backtracing) break kernel resume function. After
> > looking into the gic driver, I saw the dist_inti set all global interrupts to GROUP 1(non-secure), but in
> > dist_restore, it does NOT restore those settings, after restoring them, suspend/resume works.
> 
> Right, this is the code from Russell and Daniel I was talking about. As
> it says in the commit message:
> 
>   Cobble the FIQ backtracing together, some of this patch is based on
>   some of Daniel Thompson's work.  Experimental, and not for submission
>   yet.
> 
> So this patch doesn't apply to mainline.

It's also not intended to be mainline material.  As it says "Experimental"
and that means "it will probably break some things".  One of the biggest
issues that this patch has is that if the FIQ IPI gets sent, the rest of
the system effectively stops until reboot, because we _never_ ack the FIQ
IPI.  Anyone using this patch in a production system is taking a _very_
big risk.  No, enabling the other kernel reboot-on-whatever options won't
save you.

> > 	Don't we need to make this GIC distributor settings after resume aligned with before suspend?
> > Before suspend, global interrupts are set to non-secure, but after resume, it is by default set to
> > secure mode, while cpu controller signal secure interrupts to FIQn, so that is incorrect, not sure
> > if my understanding is correct, please correct me if I am wrong.
> 
> You're correct that something has to be done. But this should be done as
> part of Russell and Daniel FIQ series.
> 
> Russell, are you willing to carry that patch (or something similar) as
> part of your series?

I don't see the worth of doing so.

My patch is really only meant as a method to stitch the FIQ and NMI
backtrace code together for my own purposes.  My patch is derived in
part from my early work and Daniel's patch set, and is definitely
incomplete.  It's not going to end up in mainline, and it certainly
has problems, s/r being the least of them.

Even if I merge it into my patch set or patch (I'd prefer it to go
into the patch itself as I'm carrying too many private commits
against mainline already) I don't see the value as I'm unlikely to
re-post that patch, especially as the generic NMI backtrace code is
queued for merging for 4.3-rc1.

This leaves just the FIQ <-> NMI backtrace hookup outstanding, and
that's something which Daniel and myself need to finally settle on -
and that's probably going to be Daniel's patches rather than mine
provided they pass testing here.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] irqchip/gic: restore global interrupts group settings in distributor

[Marc Zyngier]

On 05/08/15 11:21, Russell King - ARM Linux wrote:
> On Wed, Aug 05, 2015 at 10:59:10AM +0100, Marc Zyngier wrote:
>> On 05/08/15 10:17, Anson Huang wrote:
>>> On Wed, Aug 05, 2015 at 10:12:35AM +0100, Marc Zyngier wrote:
>>> 	I may NOT know the history of enabling secure/non-secure of GIC very well, will spend
>>> some time look into it later, during the upstream of our i.MX6UL's suepnd/resume,
>>> I found patch(19bcd001 ARM: cobble together FIQ backtracing) break kernel resume function. After
>>> looking into the gic driver, I saw the dist_inti set all global interrupts to GROUP 1(non-secure), but in
>>> dist_restore, it does NOT restore those settings, after restoring them, suspend/resume works.
>>
>> Right, this is the code from Russell and Daniel I was talking about. As
>> it says in the commit message:
>>
>>   Cobble the FIQ backtracing together, some of this patch is based on
>>   some of Daniel Thompson's work.  Experimental, and not for submission
>>   yet.
>>
>> So this patch doesn't apply to mainline.
> 
> It's also not intended to be mainline material.  As it says "Experimental"
> and that means "it will probably break some things".  One of the biggest
> issues that this patch has is that if the FIQ IPI gets sent, the rest of
> the system effectively stops until reboot, because we _never_ ack the FIQ
> IPI.  Anyone using this patch in a production system is taking a _very_
> big risk.  No, enabling the other kernel reboot-on-whatever options won't
> save you.
> 
>>> 	Don't we need to make this GIC distributor settings after resume aligned with before suspend?
>>> Before suspend, global interrupts are set to non-secure, but after resume, it is by default set to
>>> secure mode, while cpu controller signal secure interrupts to FIQn, so that is incorrect, not sure
>>> if my understanding is correct, please correct me if I am wrong.
>>
>> You're correct that something has to be done. But this should be done as
>> part of Russell and Daniel FIQ series.
>>
>> Russell, are you willing to carry that patch (or something similar) as
>> part of your series?
> 
> I don't see the worth of doing so.
> 
> My patch is really only meant as a method to stitch the FIQ and NMI
> backtrace code together for my own purposes.  My patch is derived in
> part from my early work and Daniel's patch set, and is definitely
> incomplete.  It's not going to end up in mainline, and it certainly
> has problems, s/r being the least of them.

Understood. At the moment, this series seems to be in -next, which
people interpret as "this is on its way to mainline". Anson definitely
interpreted it this way, and Jon Hunter did the same last week, basing
his own patches on top of this series. That's certainly the way I see
-next too.

Maybe it is worth pulling it out of -next so that it doesn't give people
the wrong impression?

Thanks,

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

Re: [PATCH] irqchip/gic: restore global interrupts group settings in distributor

[Russell King - ARM Linux]

On Wed, Aug 05, 2015 at 11:42:44AM +0100, Marc Zyngier wrote:
> Understood. At the moment, this series seems to be in -next, which

Damn it, it's not supposed to be!  Thanks for pointing that out.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.