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

[Anson Huang]

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