RE: [PATCH V2] irqchip: gpcv2: remove unnecessary functions

From: Anson Huang
Date: Mon May 28 2018 - 04:34:21 EST


Please ignore this patch for now, I found something incorrect, since the gpc make/unmask and set wake uses same registers, there are some sync issue about it. So I think keep the old way is OK, although the save/restore looks a little confuse. Sorry for the noise.

Anson Huang
Best Regards!


> -----Original Message-----
> From: Anson Huang
> Sent: Monday, May 28, 2018 2:13 PM
> To: tglx@xxxxxxxxxxxxx; jason@xxxxxxxxxxxxxx; marc.zyngier@xxxxxxx
> Cc: dl-linux-imx <linux-imx@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [PATCH V2] irqchip: gpcv2: remove unnecessary functions
>
> GPC is in always-on domain, it never lost its content during suspend/resume, so
> no need to do save/restore for it during suspend/resume.
>
> Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> ---
> changes since V1:
> Add missing wakeup source write into GPC IMR register;
> remove all necessary arrays;
> suspend/resume tested passed with u-boot I develop for suspend/resume
> support.
> drivers/irqchip/irq-imx-gpcv2.c | 50 +++--------------------------------------
> 1 file changed, 3 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> index 4760307..6bb6ba0 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -21,53 +21,11 @@
> 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;
> };
>
> static struct gpcv2_irqchip_data *imx_gpcv2_instance;
>
> -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; @@ -79,9 +37,9 @@
> static int imx_gpcv2_irq_set_wake(struct irq_data *d, unsigned int on)
> 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);
> + val = readl_relaxed(reg);
> + val = on ? (val & ~mask) : (val | mask);
> + writel_relaxed(val, reg);
> raw_spin_unlock_irqrestore(&cd->rlock, flags);
>
> /*
> @@ -238,7 +196,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 */ @@ -252,7 +209,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);
>
> /*
> * Clear the OF_POPULATED flag set in of_irq_init so that
> --
> 2.7.4