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

On Wed, Aug 12, 2026 at 01:49:18PM +0200, Daniel Lezcano wrote:
> On 8/12/26 13:09, Ondřej Jirman wrote:
> > Hello Daniel,
> > 
> > On Wed, Aug 12, 2026 at 11:56:03AM +0200, Daniel Lezcano wrote:
> > > 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);
> > > 
> > 
> > I can. The only difference would be that this would loop up to 1000 * timeout
> > times, instead of 100 times in case reload bit never returns to 0. But that
> > never happens anyway.
> 
> Sorry I don't get the 1000 * timeout

If you look at implementation of readl_poll_timeout_atomic() the timeout
parameter means that readl_poll_timeout_atomic() will do at most timeout * 1000
thight loops, so minimum timeout is 1000 loops.

> Does the documentation says anything about the duration before the register
> is cleared ?

Not directly, but for start after stop, manual says to wait at least 2 Tcycles
(input clock cycles to the timer).

Regards,
	o.

> > > > +	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 ?
> > 
> > There's not much to do, since sun4i_clkevt_time_stop() does not return anything
> > anyway and it's used across the file quite extensively. Well, the file is small,
> > but sun4i_clkevt_time_stop is called in every function exported via timer_of
> > struct... :)
> > 
> > I can rewrite all these functions to report all errors to timer core, but in
> > practice reload always resets very quickly, and a condition where the loop even
> > loops happens once every few seconds or so. I doubt anything but return 0
> > would ever happen anyway. I'd certainly notice, because it would again cause
> > instant lockup of my e-reader. Maybe I can just return void from
> > sun4i_clkevt_wait_reload() and not pretend there are errors to report,
> > instead?
> IMO, returning void is fine along with a message, may be a WARN as it is
> really, AFAIU, not supposed to happen. It will be easy for anyone to find
> out where the issue is coming from
>