[PATCH] powerpc: remove WATCHDOG_RESET call from wait_ticks()

[Rasmus Villemoes]

On 17/03/2020 13.21, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <1b6c7efd-8264-6cb5-0b39-3223bae5f8dc at prevas.dk> you wrote:
>>
>> Or do you not agree that __udelay is supposed to be a raw primitive that
>> does the delay and nothing else?
> 
> I agree, and it does that - it converts the microseconds into ticks,
> and calls wait_ticks(), and does nothing else.
> 
> It's the wait_ticks() implementation which references the macro
> WATCHDOG_RESET.

That's hair-splitting, wait_ticks only has that single caller. And
regardless of whether __udelay is a leaf function or not, the call graph
below it is what determines whether it can be considered a primitive.
Currently, it is not.

>>>> There are not that many __udelay() calls, so I doubt this causes a
>>>> regression for anyone. Callers of udelay() are not affected, since
>>>> udelay() itself does one WATCHDOG_RESET() per __udelay() call.
>>>
>>> Which exact platforms have you tested this on?
>>
>> An MPC8309-derived board which does not utilize the SOCs watchdog but
>> has an external gpio-triggered (always-running) watchdog circuit.
> 
> This is not even close to global coverage.

No, of course not. Shall I list all __udelay() calls in the tree and
exclude the ones that are in board-specific code for non-ppc boards? I'm
pretty sure that leaves a very small handful. I can do that.

> Are you aware that there is the PPC-global implementation of
> wait_ticks() in arch/powerpc/lib/ticks.S , plus another MPC83xx
> specific one in drivers/timer/mpc83xx_timer.c?

Yes, that latter one doesn't even compile in the presence of
CONFIG_WATCHDOG or CONFIG_HW_WATCHDOG.

> I don't even understand why MPC83xx needs special treatment.

It doesn't. I don't understand why powerpc's __udelay needs to be
different from all other architectures'. This is not really about the
quirks of ppc SOCs' watchdogs or not, it's about providing a __udelay
primitive that generic code can rely on has certain properties, and in
particular, that watchdog drivers can use without risking being called
recursively.

> In any case it seems to me a board specific redefinition of the
> WATCHDOG_RESET macro would be less intrusive and risky than changing
> code that has been there since the beginning of time (well, at least
> more than 18 years).

The point is, we're being told that everything is moving to DM and
better convert your board or else..., and nowadays CONFIG_WDT comes with
it's own watchdog_reset() which is a rather more complicated beast than
the board-specific ones that used to be sprinkled throughout (and
out-of) the tree. So yes, for the past 18 years, nothing bad has
probably come from doing a WATCHDOG_RESET even deep down in the guts of
arch-specific primitives.

Rasmus