On 17/03/2020 15.31, Wolfgang Denk wrote: > Dear Rasmus, > > In message <[email protected]> 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
