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

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

[Rasmus Villemoes]

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@prevas.dk>
---
 arch/powerpc/lib/ticks.S | 1 -
 1 file changed, 1 deletion(-)

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
-- 
2.23.0

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

[Wolfgang Denk]

Dear Rasmus,

In message <20200316212337.30204-1-rasmus.villemoes@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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Diplomacy is the art of saying "nice doggy" until you can find a rock.

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

[Rasmus Villemoes]

On 17/03/2020 11.02, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <20200316212337.30204-1-rasmus.villemoes@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

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

[Wolfgang Denk]

Dear Rasmus,

In message <1b6c7efd-8264-6cb5-0b39-3223bae5f8dc@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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I am not now, nor have I ever been, a member of the demigodic party.
                                                   -- Dennis Ritchie

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

[Rasmus Villemoes]

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

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

[Christophe Leroy]

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

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

[Rasmus Villemoes]

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

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

[Wolfgang Denk]

Dear Rasmus,

In message <01193803-be8f-865d-5fce-2c7cee0fd9af@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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"A dirty mind is a joy forever."                       - Randy Kunkee

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

[Wolfgang Denk]

Dear Rasmus,

In message <56b724ef-8051-8a46-5df8-fc5b00bd8408@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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
No, I'm not going to explain it. If you  can't  figure  it  out,  you
didn't want to know anyway... :-)
                   - Larry Wall in <1991Aug7.180856.2854@netlabs.com>

[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@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

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

[Wolfgang Denk]

Dear Rasmus,

In message <86e1c45a-fc23-6794-7ca9-e96910af4743@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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Defaults are wonderful, just like fire.
                  - Larry Wall in <1996Mar6.004121.27890@netlabs.com>