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

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

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

>>> +	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