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 <[email protected]>
---
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
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?
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 <[email protected]>
> ---
> 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
> -----Original Message-----
> From: Thomas Gleixner [mailto:[email protected]]
> Sent: 2015??8??26?? 11:00
> To: Wang Shenwei-B38339
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; 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?
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> -----Original Message-----
> From: Sudeep Holla [mailto:[email protected]]
> Sent: 2015??8??26?? 11:13
> To: Wang Shenwei-B38339; [email protected]
> Cc: [email protected]; [email protected]; Sudeep Holla;
> [email protected]; [email protected]; 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 <[email protected]>
> > ---
> > 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 26/08/15 17:44, Shenwei Wang wrote:
>
>
>> -----Original Message-----
>> From: Sudeep Holla [mailto:[email protected]]
>> Sent: 2015年8月26日 11:13
>> To: Wang Shenwei-B38339; [email protected]
>> Cc: [email protected]; [email protected]; Sudeep Holla;
>> [email protected]; [email protected]; 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
Regards,
Sudeep
On Wed, 26 Aug 2015, Shenwei Wang wrote:
> > From: Thomas Gleixner [mailto:[email protected]]
> > > + 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
> -----Original Message-----
> From: Thomas Gleixner [mailto:[email protected]]
> > > > +
> > > > 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