On 17/03/2020 15.31, Wolfgang Denk wrote:
Dear Rasmus,
In message 01193803-be8f-865d-5fce-2c7cee0fd9af@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