On 17/03/2020 13.21, Wolfgang Denk wrote: > Dear Rasmus, > > In message <1b6c7efd-8264-6cb5-0b39-3223bae5f...@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