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

[Rasmus Villemoes]

On 17/03/2020 15.31, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <01193803-be8f-865d-5fce-2c7cee0fd9af at prevas.dk> you wrote:
>>
>>> 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.
> 
> Maybe you focus on cleaning up this first?

I don't have much luck getting bugs in the various mpc8xxx/mpc83xx
drivers fixed. As long as the gpio and spi drivers that I do use and
tried to be a good citizen and sent patches for upstream are still
buggy, I don't really feel like fixing a driver that I don't use.

>> 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.
> 
> If any such recursive calls happen, then this is a bug in their
> implementation, isn't it?  And it should be trivial to detect and to
> fix.

Huh? Suppose hypothetically somebody implements a driver for an external
watchdog circuit connected to a gpio. The data sheet says the reset
sequence consists of setting the gpio high, waiting at least two
microseconds, then setting the gpio low. That's probably implemented as

set_the_gpio(1);
__udelay(2);
set_the_gpio(0);

Now, that works perfectly on ARM, mips, whatnot. Then some day somebody
is handed a ppc-based board with such a watchdog. Obviously that driver
doesn't work for them. Is that a bug in the original implementation?

>> 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.
> 
> You certainly have a point here.  But when you try to improve any
> code, the first thing you must do is to guarantee it continues to
> work on all affected systems.  I don't dare to give even a prognosis
> before testing this on a number of different hardware configurations.
> 
> Testing on a single platform (which apparently has aother problems,
> or you would not need any such change) is not convincing.

I don't, actually, need this change, but suggested it as a way towards
making the primitives available a little more consistent across
architectures since I stumbled on this implementation detail while
single-stepping my board to figure out why it would reset in the SPL. As
I'm unable to convince you that the benefits of that outweigh the risk
of introducing a regression, feel free to drop it.

I do, however, need the other ppc patch I sent yesterday:
https://patchwork.ozlabs.org/patch/1255844/ . That one should satisfy
the requirement that it cannot possibly break anything for existing boards.

Rasmus