| Message ID | 1440604166-2624-1-git-send-email-shenwei.wang@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <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
> -----Original Message----- > From: Thomas Gleixner [mailto:tglx@linutronix.de] > Sent: 2015?8?26? 11:00 > To: Wang Shenwei-B38339 > Cc: shawn.guo@linaro.org; jason@lakedaemon.net; sudeep.holla@arm.com; > linux-arm-kernel@lists.infradead.org; linux-kernel@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?
> -----Original Message----- > From: Sudeep Holla [mailto:sudeep.holla@arm.com] > Sent: 2015?8?26? 11:13 > To: Wang Shenwei-B38339; jason@lakedaemon.net > Cc: shawn.guo@linaro.org; tglx@linutronix.de; Sudeep Holla; > linux-arm-kernel@lists.infradead.org; linux-kernel@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
On 26/08/15 17:44, Shenwei Wang wrote: > > >> -----Original Message----- >> From: Sudeep Holla [mailto:sudeep.holla@arm.com] >> Sent: 2015?8?26? 11:13 >> To: Wang Shenwei-B38339; jason@lakedaemon.net >> Cc: shawn.guo@linaro.org; tglx@linutronix.de; Sudeep Holla; >> linux-arm-kernel@lists.infradead.org; linux-kernel@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
On Wed, 26 Aug 2015, Shenwei Wang wrote: > > From: Thomas Gleixner [mailto:tglx@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
> -----Original Message----- > From: Thomas Gleixner [mailto:tglx@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
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; }
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(-)