[PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

[PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

[Shenwei Wang]

Based on Sudeep Holla's review comments, the implementation can
be simplified by using the two flags: IRQCHIP_SKIP_SET_WAKE and
IRQCHIP_MASK_ON_SUSPEND. This patch enables the flags in the
struct irq_chip and removes the unnecessory syscore_ops callbacks.

Signed-off-by: Shenwei Wang <shenwei.wang@freescale.com>
---
 drivers/irqchip/irq-imx-gpcv2.c | 83 +++++++----------------------------------
 1 file changed, 13 insertions(+), 70 deletions(-)

diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
index 4a97afa..e25df78 100644
--- a/drivers/irqchip/irq-imx-gpcv2.c
+++ b/drivers/irqchip/irq-imx-gpcv2.c
@@ -22,7 +22,6 @@ struct gpcv2_irqchip_data {
 	struct raw_spinlock    rlock;
 	void __iomem       *gpc_base;
 	u32 wakeup_sources[IMR_NUM];
-	u32 saved_irq_mask[IMR_NUM];
 	u32 cpu2wakeup;
 };
 
@@ -30,79 +29,25 @@ static struct gpcv2_irqchip_data *imx_gpcv2_instance;
 
 u32 imx_gpcv2_get_wakeup_source(u32 **sources)
 {
-	if (!imx_gpcv2_instance)
+	struct gpcv2_irqchip_data *cd;
+	void __iomem *reg;
+	int i;
+
+	cd = imx_gpcv2_instance;
+	if (!cd)
 		return 0;
 
+	for (i = 0; i < IMR_NUM; i++) {
+		reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
+		cd->wakeup_sources[i] = readl_relaxed(reg);
+	}
+
 	if (sources)
-		*sources = imx_gpcv2_instance->wakeup_sources;
+		*sources = cd->wakeup_sources;
 
 	return IMR_NUM;
 }
 
-static int gpcv2_wakeup_source_save(void)
-{
-	struct gpcv2_irqchip_data *cd;
-	void __iomem *reg;
-	int i;
-
-	cd = imx_gpcv2_instance;
-	if (!cd)
-		return 0;
-
-	for (i = 0; i < IMR_NUM; i++) {
-		reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
-		cd->saved_irq_mask[i] = readl_relaxed(reg);
-		writel_relaxed(cd->wakeup_sources[i], reg);
-	}
-
-	return 0;
-}
-
-static void gpcv2_wakeup_source_restore(void)
-{
-	struct gpcv2_irqchip_data *cd;
-	void __iomem *reg;
-	int i;
-
-	cd = imx_gpcv2_instance;
-	if (!cd)
-		return;
-
-	for (i = 0; i < IMR_NUM; i++) {
-		reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
-		writel_relaxed(cd->saved_irq_mask[i], reg);
-	}
-}
-
-static struct syscore_ops imx_gpcv2_syscore_ops = {
-	.suspend	= gpcv2_wakeup_source_save,
-	.resume		= gpcv2_wakeup_source_restore,
-};
-
-static int imx_gpcv2_irq_set_wake(struct irq_data *d, unsigned int on)
-{
-	struct gpcv2_irqchip_data *cd = d->chip_data;
-	unsigned int idx = d->hwirq / 32;
-	unsigned long flags;
-	void __iomem *reg;
-	u32 mask, val;
-
-	raw_spin_lock_irqsave(&cd->rlock, flags);
-	reg = cd->gpc_base + cd->cpu2wakeup + idx * 4;
-	mask = 1 << d->hwirq % 32;
-	val = cd->wakeup_sources[idx];
-
-	cd->wakeup_sources[idx] = on ? (val & ~mask) : (val | mask);
-	raw_spin_unlock_irqrestore(&cd->rlock, flags);
-
-	/*
-	 * Do *not* call into the parent, as the GIC doesn't have any
-	 * wake-up facility...
-	 */
-
-	return 0;
-}
-
 static void imx_gpcv2_irq_unmask(struct irq_data *d)
 {
 	struct gpcv2_irqchip_data *cd = d->chip_data;
@@ -140,11 +85,11 @@ static struct irq_chip gpcv2_irqchip_data_chip = {
 	.irq_eoi		= irq_chip_eoi_parent,
 	.irq_mask		= imx_gpcv2_irq_mask,
 	.irq_unmask		= imx_gpcv2_irq_unmask,
-	.irq_set_wake		= imx_gpcv2_irq_set_wake,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 #ifdef CONFIG_SMP
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
 #endif
+	.flags			= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
 };
 
 static int imx_gpcv2_domain_xlate(struct irq_domain *domain,
@@ -253,7 +198,6 @@ static int __init imx_gpcv2_irqchip_init(struct device_node *node,
 	for (i = 0; i < IMR_NUM; i++) {
 		writel_relaxed(~0, cd->gpc_base + GPC_IMR1_CORE0 + i * 4);
 		writel_relaxed(~0, cd->gpc_base + GPC_IMR1_CORE1 + i * 4);
-		cd->wakeup_sources[i] = ~0;
 	}
 
 	/* Let CORE0 as the default CPU to wake up by GPC */
@@ -267,7 +211,6 @@ static int __init imx_gpcv2_irqchip_init(struct device_node *node,
 	writel_relaxed(~0x1, cd->gpc_base + cd->cpu2wakeup);
 
 	imx_gpcv2_instance = cd;
-	register_syscore_ops(&imx_gpcv2_syscore_ops);
 
 	return 0;
 }
-- 
2.5.0.rc2

[PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

[Thomas Gleixner]

On Wed, 26 Aug 2015, Shenwei Wang wrote:
>  u32 imx_gpcv2_get_wakeup_source(u32 **sources)
>  {
> -	if (!imx_gpcv2_instance)
> +	struct gpcv2_irqchip_data *cd;
> +	void __iomem *reg;
> +	int i;
> +
> +	cd = imx_gpcv2_instance;
> +	if (!cd)
>  		return 0;
>  
> +	for (i = 0; i < IMR_NUM; i++) {
> +		reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> +		cd->wakeup_sources[i] = readl_relaxed(reg);
> +	}
> +
>  	if (sources)
> -		*sources = imx_gpcv2_instance->wakeup_sources;
> +		*sources = cd->wakeup_sources;
>  
>  	return IMR_NUM;

You do not need the intermediate storage at all.

u32 imx_gpcv2_get_wakeup_source(u32 *sources)
{
	if (sources) {
		for (i = 0; i < IMR_NUM; i++) {
			reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
			sources[i] = readl_relaxed(reg);
		}
	}
	....

Hmm?

[PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

[Sudeep Holla]

typo in $subject

On 26/08/15 16:49, Shenwei Wang wrote:
> Based on Sudeep Holla's review comments, the implementation can
> be simplified by using the two flags: IRQCHIP_SKIP_SET_WAKE and
> IRQCHIP_MASK_ON_SUSPEND. This patch enables the flags in the
> struct irq_chip and removes the unnecessory syscore_ops callbacks.
>
> Signed-off-by: Shenwei Wang <shenwei.wang@freescale.com>
> ---
>   drivers/irqchip/irq-imx-gpcv2.c | 83 +++++++----------------------------------
>   1 file changed, 13 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> index 4a97afa..e25df78 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -22,7 +22,6 @@ struct gpcv2_irqchip_data {
>   	struct raw_spinlock    rlock;
>   	void __iomem       *gpc_base;
>   	u32 wakeup_sources[IMR_NUM];
> -	u32 saved_irq_mask[IMR_NUM];
>   	u32 cpu2wakeup;
>   };
>
> @@ -30,79 +29,25 @@ static struct gpcv2_irqchip_data *imx_gpcv2_instance;
>
>   u32 imx_gpcv2_get_wakeup_source(u32 **sources)

I assume this patch is against -next and I don't see any users of
imx_gpcv2_get_wakeup_source in -next.

If possible I would avoid exposing this function by implementing
suspend_ops just as before(just saving raw h/w reg values and restoring
then back on resume w/o tagging them as wakeup mask though they might be
indeed wakeup mask).

In that way, this driver is self-contained and whatever imx code calls
this function will not have dependency on this driver, no ? Do you need
access to imx_gpcv2_get_wakeup_source too early in resume much before
suspend_ops resume ? I would like to see the user of that function to
comment on that any further.

Regards,
Sudeep

[PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

[Shenwei Wang]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx at linutronix.de]
> Sent: 2015?8?26? 11:00
> To: Wang Shenwei-B38339
> Cc: shawn.guo at linaro.org; jason at lakedaemon.net; sudeep.holla at arm.com;
> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Huang
> Yongcai-B20788
> Subject: Re: [PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation
> 
> On Wed, 26 Aug 2015, Shenwei Wang wrote:
> >  u32 imx_gpcv2_get_wakeup_source(u32 **sources)  {
> > -	if (!imx_gpcv2_instance)
> > +	struct gpcv2_irqchip_data *cd;
> > +	void __iomem *reg;
> > +	int i;
> > +
> > +	cd = imx_gpcv2_instance;
> > +	if (!cd)
> >  		return 0;
> >
> > +	for (i = 0; i < IMR_NUM; i++) {
> > +		reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> > +		cd->wakeup_sources[i] = readl_relaxed(reg);
> > +	}
> > +
> >  	if (sources)
> > -		*sources = imx_gpcv2_instance->wakeup_sources;
> > +		*sources = cd->wakeup_sources;
> >
> >  	return IMR_NUM;
> 
> You do not need the intermediate storage at all.
> 
> u32 imx_gpcv2_get_wakeup_source(u32 *sources) {
> 	if (sources) {
> 		for (i = 0; i < IMR_NUM; i++) {
> 			reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> 			sources[i] = readl_relaxed(reg);
> 		}
> 	}
> 	....

Using the intermediate storage here can make the caller a little easier,
because the caller does not need to malloc the memory before the call.
Especially the caller does not even know how many memory to allocate
In the beginning.

Thanks,
Shenwei

> Hmm?

[PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

[Shenwei Wang]

> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla at arm.com]
> Sent: 2015?8?26? 11:13
> To: Wang Shenwei-B38339; jason at lakedaemon.net
> Cc: shawn.guo at linaro.org; tglx at linutronix.de; Sudeep Holla;
> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Huang
> Yongcai-B20788
> Subject: Re: [PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation
> 
> typo in $subject

Just noticed. Will change it.

> On 26/08/15 16:49, Shenwei Wang wrote:
> > Based on Sudeep Holla's review comments, the implementation can be
> > simplified by using the two flags: IRQCHIP_SKIP_SET_WAKE and
> > IRQCHIP_MASK_ON_SUSPEND. This patch enables the flags in the struct
> > irq_chip and removes the unnecessory syscore_ops callbacks.
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang@freescale.com>
> > ---
> >   drivers/irqchip/irq-imx-gpcv2.c | 83 +++++++----------------------------------
> >   1 file changed, 13 insertions(+), 70 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-imx-gpcv2.c
> > b/drivers/irqchip/irq-imx-gpcv2.c index 4a97afa..e25df78 100644
> > --- a/drivers/irqchip/irq-imx-gpcv2.c
> > +++ b/drivers/irqchip/irq-imx-gpcv2.c
> > @@ -22,7 +22,6 @@ struct gpcv2_irqchip_data {
> >   	struct raw_spinlock    rlock;
> >   	void __iomem       *gpc_base;
> >   	u32 wakeup_sources[IMR_NUM];
> > -	u32 saved_irq_mask[IMR_NUM];
> >   	u32 cpu2wakeup;
> >   };
> >
> > @@ -30,79 +29,25 @@ static struct gpcv2_irqchip_data
> > *imx_gpcv2_instance;
> >
> >   u32 imx_gpcv2_get_wakeup_source(u32 **sources)
> 
> I assume this patch is against -next and I don't see any users of
> imx_gpcv2_get_wakeup_source in -next.
> 
> If possible I would avoid exposing this function by implementing suspend_ops just
> as before(just saving raw h/w reg values and restoring then back on resume w/o
> tagging them as wakeup mask though they might be indeed wakeup mask).
> 
> In that way, this driver is self-contained and whatever imx code calls this function
> will not have dependency on this driver, no ? Do you need access to
> imx_gpcv2_get_wakeup_source too early in resume much before suspend_ops
> resume ? I would like to see the user of that function to comment on that any
> further.

It is linux-next. The user is in the following patch which is under review.
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/361388.html

The access to this function will only happen in suspend_ops so far.

Regards,
Shenwei


> Regards,
> Sudeep

[PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

[Sudeep Holla]

On 26/08/15 17:44, Shenwei Wang wrote:
>
>
>> -----Original Message-----
>> From: Sudeep Holla [mailto:sudeep.holla at arm.com]
>> Sent: 2015?8?26? 11:13
>> To: Wang Shenwei-B38339; jason at lakedaemon.net
>> Cc: shawn.guo at linaro.org; tglx at linutronix.de; Sudeep Holla;
>> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Huang
>> Yongcai-B20788
>> Subject: Re: [PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation
>>
>> typo in $subject
>
> Just noticed. Will change it.
>
>> On 26/08/15 16:49, Shenwei Wang wrote:
>>> Based on Sudeep Holla's review comments, the implementation can be
>>> simplified by using the two flags: IRQCHIP_SKIP_SET_WAKE and
>>> IRQCHIP_MASK_ON_SUSPEND. This patch enables the flags in the struct
>>> irq_chip and removes the unnecessory syscore_ops callbacks.
>>>
>>> Signed-off-by: Shenwei Wang <shenwei.wang@freescale.com>
>>> ---
>>>    drivers/irqchip/irq-imx-gpcv2.c | 83 +++++++----------------------------------
>>>    1 file changed, 13 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-imx-gpcv2.c
>>> b/drivers/irqchip/irq-imx-gpcv2.c index 4a97afa..e25df78 100644
>>> --- a/drivers/irqchip/irq-imx-gpcv2.c
>>> +++ b/drivers/irqchip/irq-imx-gpcv2.c
>>> @@ -22,7 +22,6 @@ struct gpcv2_irqchip_data {
>>>    	struct raw_spinlock    rlock;
>>>    	void __iomem       *gpc_base;
>>>    	u32 wakeup_sources[IMR_NUM];
>>> -	u32 saved_irq_mask[IMR_NUM];
>>>    	u32 cpu2wakeup;
>>>    };
>>>
>>> @@ -30,79 +29,25 @@ static struct gpcv2_irqchip_data
>>> *imx_gpcv2_instance;
>>>
>>>    u32 imx_gpcv2_get_wakeup_source(u32 **sources)
>>
>> I assume this patch is against -next and I don't see any users of
>> imx_gpcv2_get_wakeup_source in -next.
>>
>> If possible I would avoid exposing this function by implementing suspend_ops just
>> as before(just saving raw h/w reg values and restoring then back on resume w/o
>> tagging them as wakeup mask though they might be indeed wakeup mask).
>>
>> In that way, this driver is self-contained and whatever imx code calls this function
>> will not have dependency on this driver, no ? Do you need access to
>> imx_gpcv2_get_wakeup_source too early in resume much before suspend_ops
>> resume ? I would like to see the user of that function to comment on that any
>> further.
>
> It is linux-next. The user is in the following patch which is under review.
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/361388.html
>

I got lost trying to follow through that 1000 odd line of code with lots
of register accesses. I couldn't understand much, so I gave up.

Such details are abstracted well and hidden in firmware with PSCI(good
that it's enforced in ARM64, hopefully ARM32 also sees more adoption).

So, for the parts adding those 2 flags and removing the unnecessary
code, you can add:
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Regards,
Sudeep

[PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

[Thomas Gleixner]

On Wed, 26 Aug 2015, Shenwei Wang wrote:
> > From: Thomas Gleixner [mailto:tglx at linutronix.de]
> > > +	cd = imx_gpcv2_instance;
> > > +	if (!cd)
> > >  		return 0;
> > >
> > > +	for (i = 0; i < IMR_NUM; i++) {
> > > +		reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> > > +		cd->wakeup_sources[i] = readl_relaxed(reg);
> > > +	}
> > > +
> > >  	if (sources)
> > > -		*sources = imx_gpcv2_instance->wakeup_sources;
> > > +		*sources = cd->wakeup_sources;
> > >
> > >  	return IMR_NUM;
> > 
> > You do not need the intermediate storage at all.
> > 
> > u32 imx_gpcv2_get_wakeup_source(u32 *sources) {
> > 	if (sources) {
> > 		for (i = 0; i < IMR_NUM; i++) {
> > 			reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> > 			sources[i] = readl_relaxed(reg);
> > 		}
> > 	}
> > 	....
> 
> Using the intermediate storage here can make the caller a little easier,
> because the caller does not need to malloc the memory before the call.
> Especially the caller does not even know how many memory to allocate
> In the beginning.

Fair enough, but why do you need that case where sources can be NULL
just to return IMR_NUM?

Thanks,

	tglx

[PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

[Shenwei Wang]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx at linutronix.de]
> > > > +
> > > >  	if (sources)
> > > > -		*sources = imx_gpcv2_instance->wakeup_sources;
> > > > +		*sources = cd->wakeup_sources;
> > > >
> > > >  	return IMR_NUM;
> > >
> > > You do not need the intermediate storage at all.
> > >
> > > u32 imx_gpcv2_get_wakeup_source(u32 *sources) {
> > > 	if (sources) {
> > > 		for (i = 0; i < IMR_NUM; i++) {
> > > 			reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> > > 			sources[i] = readl_relaxed(reg);
> > > 		}
> > > 	}
> > > 	....
> >
> > Using the intermediate storage here can make the caller a little
> > easier, because the caller does not need to malloc the memory before the call.
> > Especially the caller does not even know how many memory to allocate
> > In the beginning.
> 
> Fair enough, but why do you need that case where sources can be NULL just to
> return IMR_NUM?

The intention is to get the IMR_NUM only. But so far no user uses this feature.

Thanks,
Shenwei

> Thanks,
> 
> 	tglx