Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jernej =?utf-8?B?xaBrcmFiZWM=?= <jernej.skrabec@gmail.com>
To:     daniel.lezcano@linaro.org, tglx@linutronix.de, wens@csie.org,
        samuel@sholland.org, maxime@cerno.tech,
        Victor Hassan <victor@allwinnertech.com>
Cc:     linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-sunxi@lists.linux.dev
Subject: Re: [RESEND] clocksource: sun4i: Fix the bug that tick_resume stucks
Date:   Wed, 12 Oct 2022 23:46:41 +0200
Message-ID: <14741195.tv2OnDr8pf@kista>
In-Reply-To: <20221009032507.39145-1-victor@allwinnertech.com>
References: <20221009032507.39145-1-victor@allwinnertech.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

Hi Victor,

Dne nedelja, 09. oktober 2022 ob 05:25:07 CEST je Victor Hassan napisal(a):
> Currently syscore_resume() will stuck on tick_resume().
> Fix this by changing  `.tick_resume` from
> sun4i_clkevt_shutdown() to a new function sun4i_tick_resume().

This is pretty involved change. Can you split it in more manageable chunks? 
Also you didn't state why it stucks and what does new approach differently so 
it doesn't stuck.

Best regards,
Jernej

> 
> Signed-off-by: Victor Hassan <victor@allwinnertech.com>
> ---
>  drivers/clocksource/timer-sun4i.c | 131 ++++++++++++++++++++++--------
>  1 file changed, 96 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-sun4i.c
> b/drivers/clocksource/timer-sun4i.c index 94dc6e42e983..574398c35a22 100644
> --- a/drivers/clocksource/timer-sun4i.c
> +++ b/drivers/clocksource/timer-sun4i.c
> @@ -38,6 +38,19 @@
> 
>  #define TIMER_SYNC_TICKS	3
> 
> +/* Registers which needs to be saved and restored before and after sleeping
> */ +static u32 sun4i_timer_regs_offset[] = {
> +	TIMER_IRQ_EN_REG,
> +	TIMER_CTL_REG(0),
> +	TIMER_INTVAL_REG(0),
> +	TIMER_CNTVAL_REG(0),
> +	TIMER_CTL_REG(1),
> +	TIMER_INTVAL_REG(1),
> +	TIMER_CNTVAL_REG(1),
> +};
> +
> +static void __iomem *sun4i_timer_sched_base __read_mostly;
> +
>  /*
>   * When we disable a timer, we need to wait at least for 2 cycles of
>   * the timer source clock. We will use for that the clocksource timer
> @@ -79,10 +92,41 @@ static void sun4i_clkevt_time_start(void __iomem *base,
> u8 timer, base + TIMER_CTL_REG(timer));
>  }
> 
> +static inline void sun4i_timer_save_regs(struct timer_of *to)
> +{
> +	void __iomem *base = timer_of_base(to);
> +	int i;
> +	u32 *regs_backup = (u32 *)to->private_data;
> +
> +	for (i = 0; i < ARRAY_SIZE(sun4i_timer_regs_offset); i++)
> +		regs_backup[i] = readl(base + 
sun4i_timer_regs_offset[i]);
> +}
> +
> +static inline void sun4i_timer_restore_regs(struct timer_of *to)
> +{
> +	void __iomem *base = timer_of_base(to);
> +	int i;
> +	u32 *regs_backup = (u32 *)to->private_data;
> +
> +	for (i = 0; i < ARRAY_SIZE(sun4i_timer_regs_offset); i++)
> +		writel(regs_backup[i], base + 
sun4i_timer_regs_offset[i]);
> +}
> +
>  static int sun4i_clkevt_shutdown(struct clock_event_device *evt)
>  {
>  	struct timer_of *to = to_timer_of(evt);
> 
> +	sun4i_timer_save_regs(to);
> +	sun4i_clkevt_time_stop(timer_of_base(to), 0);
> +
> +	return 0;
> +}
> +
> +static int sun4i_tick_resume(struct clock_event_device *evt)
> +{
> +	struct timer_of *to = to_timer_of(evt);
> +
> +	sun4i_timer_restore_regs(to);
>  	sun4i_clkevt_time_stop(timer_of_base(to), 0);
> 
>  	return 0;
> @@ -137,45 +181,54 @@ static irqreturn_t sun4i_timer_interrupt(int irq, void
> *dev_id) return IRQ_HANDLED;
>  }
> 
> -static struct timer_of to = {
> -	.flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE,
> -
> -	.clkevt = {
> -		.name = "sun4i_tick",
> -		.rating = 350,
> -		.features = CLOCK_EVT_FEAT_PERIODIC | 
CLOCK_EVT_FEAT_ONESHOT,
> -		.set_state_shutdown = sun4i_clkevt_shutdown,
> -		.set_state_periodic = sun4i_clkevt_set_periodic,
> -		.set_state_oneshot = sun4i_clkevt_set_oneshot,
> -		.tick_resume = sun4i_clkevt_shutdown,
> -		.set_next_event = sun4i_clkevt_next_event,
> -		.cpumask = cpu_possible_mask,
> -	},
> -
> -	.of_irq = {
> -		.handler = sun4i_timer_interrupt,
> -		.flags = IRQF_TIMER | IRQF_IRQPOLL,
> -	},
> -};
> +static void __init sun4i_clockevent_init(struct timer_of *to)
> +{
> +	to->clkevt.name = "sun4i_tick";
> +	to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC | 
CLOCK_EVT_FEAT_ONESHOT;
> +	to->clkevt.set_state_shutdown = sun4i_clkevt_shutdown;
> +	to->clkevt.set_state_periodic = sun4i_clkevt_set_periodic;
> +	to->clkevt.set_state_oneshot = sun4i_clkevt_set_oneshot;
> +	to->clkevt.tick_resume = sun4i_tick_resume;
> +	to->clkevt.set_next_event = sun4i_clkevt_next_event;
> +	to->clkevt.cpumask = cpu_possible_mask;
> +	to->of_irq.flags = IRQF_TIMER | IRQF_IRQPOLL;
> +
> +	sun4i_timer_sched_base = timer_of_base(to) + TIMER_CNTVAL_REG(1);
> +}
> 
>  static u64 notrace sun4i_timer_sched_read(void)
>  {
> -	return ~readl(timer_of_base(&to) + TIMER_CNTVAL_REG(1));
> +	return (u64)~readl(sun4i_timer_sched_base);
>  }
> 
>  static int __init sun4i_timer_init(struct device_node *node)
>  {
> +	struct timer_of *to;
>  	int ret;
>  	u32 val;
> 
> -	ret = timer_of_init(node, &to);
> +	to = kzalloc(sizeof(*to), GFP_KERNEL);
> +	if (!to)
> +		return -ENOMEM;
> +
> +	to->flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE;
> +	to->of_irq.handler = sun4i_timer_interrupt;
> +	ret = timer_of_init(node, to);
>  	if (ret)
> -		return ret;
> +		goto err;
> 
> -	writel(~0, timer_of_base(&to) + TIMER_INTVAL_REG(1));
> +	sun4i_clockevent_init(to);
> +
> +	to->private_data = kcalloc(ARRAY_SIZE(sun4i_timer_regs_offset),
> sizeof(u32), GFP_KERNEL); +	if (!to->private_data) {
> +		ret = -ENOMEM;
> +		goto err1;
> +	}
> +
> +	writel(~0, timer_of_base(to) + TIMER_INTVAL_REG(1));
>  	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD |
>  	       TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M),
> -	       timer_of_base(&to) + TIMER_CTL_REG(1));
> +	       timer_of_base(to) + TIMER_CTL_REG(1));
> 
>  	/*
>  	 * sched_clock_register does not have priorities, and on sun6i and
> @@ -186,32 +239,40 @@ static int __init sun4i_timer_init(struct device_node
> *node) of_machine_is_compatible("allwinner,sun5i-a10s") ||
>  	    of_machine_is_compatible("allwinner,suniv-f1c100s"))
>  		sched_clock_register(sun4i_timer_sched_read, 32,
> -				     timer_of_rate(&to));
> +				     timer_of_rate(to));
> 
> -	ret = clocksource_mmio_init(timer_of_base(&to) + 
TIMER_CNTVAL_REG(1),
> -				    node->name, 
timer_of_rate(&to), 350, 32,
> +	ret = clocksource_mmio_init(timer_of_base(to) + 
TIMER_CNTVAL_REG(1),
> +				    node->name, 
timer_of_rate(to), 350, 32,
>  				    clocksource_mmio_readl_down);
>  	if (ret) {
>  		pr_err("Failed to register clocksource\n");
> -		return ret;
> +		goto err2;
>  	}
> 
>  	writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M),
> -	       timer_of_base(&to) + TIMER_CTL_REG(0));
> +	       timer_of_base(to) + TIMER_CTL_REG(0));
> 
>  	/* Make sure timer is stopped before playing with interrupts */
> -	sun4i_clkevt_time_stop(timer_of_base(&to), 0);
> +	sun4i_clkevt_time_stop(timer_of_base(to), 0);
> 
>  	/* clear timer0 interrupt */
> -	sun4i_timer_clear_interrupt(timer_of_base(&to));
> +	sun4i_timer_clear_interrupt(timer_of_base(to));
> 
> -	clockevents_config_and_register(&to.clkevt, timer_of_rate(&to),
> +	clockevents_config_and_register(&to->clkevt, timer_of_rate(to),
>  					TIMER_SYNC_TICKS, 
0xffffffff);
> 
>  	/* Enable timer0 interrupt */
> -	val = readl(timer_of_base(&to) + TIMER_IRQ_EN_REG);
> -	writel(val | TIMER_IRQ_EN(0), timer_of_base(&to) + 
TIMER_IRQ_EN_REG);
> +	val = readl(timer_of_base(to) + TIMER_IRQ_EN_REG);
> +	writel(val | TIMER_IRQ_EN(0), timer_of_base(to) + 
TIMER_IRQ_EN_REG);
> +
> +	return ret;
> 
> +err2:
> +	kfree(to->private_data);
> +err1:
> +	timer_of_cleanup(to);
> +err:
> +	kfree(to);
>  	return ret;
>  }
>  TIMER_OF_DECLARE(sun4i, "allwinner,sun4i-a10-timer",
> --
> 2.29.0