| Message ID | 20200316212337.30204-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | New |
| Headers | show |
| Series | powerpc: remove WATCHDOG_RESET call from wait_ticks() | expand |
Dear Rasmus, In message <20200316212337.30204-1-rasmus.villemoes at prevas.dk> you wrote: > wait_ticks() is only used by the ppc-specific __udelay() > function. Having the powerpc version of __udelay implicitly call > WATCHDOG_RESET() is inconsistent with other architectures' (and the > generic __udelay() in lib/time.c) implementations. It also means a > driver cannot use __udelay() as the raw primitive it is supposed to be > - e.g. a watchdog driver that needs to do a short delay between two > operations needed to perform a ping sequence. Many PPC processors implement the watchdog differently than other sysytems - for example, on many systems the watchdog is automatically enabled after power on / reset. > 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? Best regards, Wolfgang Denk
On 17/03/2020 11.02, Wolfgang Denk wrote: > Dear Rasmus, > > In message <20200316212337.30204-1-rasmus.villemoes at prevas.dk> you wrote: >> wait_ticks() is only used by the ppc-specific __udelay() >> function. Having the powerpc version of __udelay implicitly call >> WATCHDOG_RESET() is inconsistent with other architectures' (and the >> generic __udelay() in lib/time.c) implementations. It also means a >> driver cannot use __udelay() as the raw primitive it is supposed to be >> - e.g. a watchdog driver that needs to do a short delay between two >> operations needed to perform a ping sequence. > > Many PPC processors implement the watchdog differently than other > sysytems - for example, on many systems the watchdog is > automatically enabled after power on / reset. Yes, and? I don't see why the __udelay() implementation on ppc should have a watchdog_reset call when the generic udelay() function already has, and no other __udelay has - that makes it very hard to reason about what kind of code might get called if one chooses udelay() vs __udelay(). While the SOC's watchdog typically only requires poking a few mmapped registers, there are other cases of a watchdog_reset implementation (e.g. the generic one one gets from CONFIG_WDT) that is quite a lot more involved. Or do you not agree that __udelay is supposed to be a raw primitive that does the delay and nothing else? >> 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. Rasmus
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. > >> 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. 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? I don't even understand why MPC83xx needs special treatment. 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). Best regards, Wolfgang Denk
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
Le 17/03/2020 ? 14:07, Rasmus Villemoes a ?crit?: > >> 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. > CONFIG_WDT comes with a watchdog_reset() when you defined CONFIG_WATCHDOG. For boards like powerpc boards with embedded SOC watchdog that needs to be pinged from the very begining, we have CONFIG_HW_WATCHDOG and that's compatible with DM. See https://gitlab.denx.de/u-boot/u-boot/-/commit/a68256074f4239008c6d5c936bc0f8857f3d1b8a Christophe
On 17/03/2020 14.21, Christophe Leroy wrote: > > > Le 17/03/2020 ? 14:07, Rasmus Villemoes a ?crit?: >> >>> 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. >> > > CONFIG_WDT comes with a watchdog_reset() when you defined CONFIG_WATCHDOG. > > For boards like powerpc boards with embedded SOC watchdog that needs to > be pinged from the very begining, we have CONFIG_HW_WATCHDOG and that's > compatible with DM. See > https://gitlab.denx.de/u-boot/u-boot/-/commit/a68256074f4239008c6d5c936bc0f8857f3d1b8a Yes, but I'm not talking about or using the SOC watchdog. Also, this is not really about the particular watchdog device at all. It is about what generic code can expect from the __udelay primitive, which is not currently that on ppc. FTR, my watchdog is an external gpio-triggered one. And sure, it's ten lines of code to make a board-specific watchdog_reset that pokes the appropriate SOC registers to flip that gpio. But that doesn't really work when other things, including stuff that goes via the proper DM channels, uses gpios in the same bank - especially not when the state of output gpios cannot be read back but must be stored in a shadow register. Rasmus
Dear Rasmus, In message <01193803-be8f-865d-5fce-2c7cee0fd9af at prevas.dk> you wrote: > > >> 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. No, I mean there is a pretty large numbe rof different processor families out there which were not covered by any of your tests. I think I still know a bit of the Power Architecture, but I would not dare to guarantee that your suggested change does not break any of these. Testing on a single processor / family is definitely not good enoug to give any confidence. > > 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? > 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 You misunderstand. The Power Architecture has been the first ever running U-Boot. In the beginning, two decades back, the project was not called U-Boot but PPCBoot - guess why. So you should rephrase your question into: why did all other architectures deviate from the reference implementation? > 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. > 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. Best regards, Wolfgang Denk
Dear Rasmus, In message <56b724ef-8051-8a46-5df8-fc5b00bd8408 at prevas.dk> you wrote: > > Yes, but I'm not talking about or using the SOC watchdog. Also, this is > not really about the particular watchdog device at all. It is about what > generic code can expect from the __udelay primitive, which is not > currently that on ppc. Maybe what you expect from __udelay() is not what other people expect(ed) from it? I don't think there is any formal description of that __udelay() is supposed to do in any existing U-Boot documentation... Please just face facts: for nearly two decades this code has been doing what it does, and I guess there were good reasons at that time (like memory footprint and resources to fix a specific problem). I agree that from a design point of view this is not nice, but that's not the point here. Best regards, Wolfgang Denk
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
Dear Rasmus, In message <86e1c45a-fc23-6794-7ca9-e96910af4743 at prevas.dk> you wrote: > > > 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. OK. > 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. This other patch lacks documentation - just from reading the code nobody will be able to understand what this piece of code is intended for, so there is a risk that it might be removed by some later "cleanup". Please add some comment in the code to explain the intentions. Best regards, Wolfgang Denk
diff --git a/arch/powerpc/lib/ticks.S b/arch/powerpc/lib/ticks.S index c487f938fa..f4410bbc3a 100644 --- a/arch/powerpc/lib/ticks.S +++ b/arch/powerpc/lib/ticks.S @@ -42,7 +42,6 @@ wait_ticks: addc r14, r4, r14 /* Compute end time lower */ addze r15, r3 /* and end time upper */ - WATCHDOG_RESET /* Trigger watchdog, if needed */ 1: bl get_ticks /* Get current time */ subfc r4, r4, r14 /* Subtract current time from end time */ subfe. r3, r3, r15
wait_ticks() is only used by the ppc-specific __udelay() function. Having the powerpc version of __udelay implicitly call WATCHDOG_RESET() is inconsistent with other architectures' (and the generic __udelay() in lib/time.c) implementations. It also means a driver cannot use __udelay() as the raw primitive it is supposed to be - e.g. a watchdog driver that needs to do a short delay between two operations needed to perform a ping sequence. 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. Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk> --- arch/powerpc/lib/ticks.S | 1 - 1 file changed, 1 deletion(-)