Re: [PATCH] clocksource/drivers/sun4i: Wait for pending reload before CTRL write

On 7/25/26 13:33, Ondřej Jirman wrote:
> From: Ondrej Jirman <[email protected]>
> 
> The A13 manual says of TMR0_RELOAD: "After the bit is set, it can not be
> written again before it's cleared automatically."  Both time_stop() and
> time_start() read-modify-write the control register, so a reload still in
> flight gets rewritten and the write can be dropped, leaving the timer
> unarmed.  In oneshot mode this happens on every tick, and the tick
> eventually stops for good.
> 
> Wait for the reload to clear first.  All control register updates go
> through sun4i_clkevt_time_stop().
> 
> Without this my Allwinner A13 based Pocketbook Touch Lux 3 stops
> scheduling processes during boot or within a few seconds of executing
> userspace. Only sysrq+t over serial port works at this stage, which is
> how I discovered the root cause.
> 
> Fixes: 7e14183469d8 ("clocksource: sun4i: Fix bug when switching from periodic to oneshot modes")
> Signed-off-by: Ondrej Jirman <[email protected]>
> ---
> BTW, similar issue will likely be also in timer-sun5i.c but I don't have any
> device that would exercise it. And it's possible that this triggers more easily
> with CONFIG_HZ=1000 which I use.
> 
>   drivers/clocksource/timer-sun4i.c | 26 +++++++++++++++++++++++++-
>   1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/timer-sun4i.c b/drivers/clocksource/timer-sun4i.c
> index 7bdcc60ad43c..2e7457f671c4 100644
> --- a/drivers/clocksource/timer-sun4i.c
> +++ b/drivers/clocksource/timer-sun4i.c
> @@ -39,6 +39,9 @@
>   
>   #define TIMER_SYNC_TICKS	3
>   
> +/* The reload bit clears a couple of source clock cycles after it is set. */
> +#define TIMER_RELOAD_MAX_POLL	100
> +
>   /*
>    * 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
> @@ -53,9 +56,30 @@ static void sun4i_clkevt_sync(void __iomem *base)
>   		cpu_relax();
>   }
>   
> +/*
> + * The control register must not be written while a reload is still in
> + * flight, or the write can be dropped.
> + */
> +static int sun4i_clkevt_wait_reload(void __iomem *base, u8 timer)
> +{
> +	int i;
> +
> +	for (i = 0; i < TIMER_RELOAD_MAX_POLL; i++) {
> +		if (!(readl(base + TIMER_CTL_REG(timer)) & TIMER_CTL_RELOAD))
> +			return 0;
> +		cpu_relax();
> +	}

The loop looks correct but I'm wondering why not use the helper:

ret = readl_poll_timeout_atomic(base + TIMER_CTL_REG(timer), val,
				!(val & TIMER_CTL_RELOAD), 0, timeout);


> +	return -ETIME;
> +}
> +
>   static void sun4i_clkevt_time_stop(void __iomem *base, u8 timer)
>   {
> -	u32 val = readl(base + TIMER_CTL_REG(timer));
> +	u32 val;
> +
> +	sun4i_clkevt_wait_reload(base, timer);

If it fails ?

> +
> +	val = readl(base + TIMER_CTL_REG(timer));
>   	writel(val & ~TIMER_CTL_ENABLE, base + TIMER_CTL_REG(timer));
>   	sun4i_clkevt_sync(base);
>   }