imx8 regression: cyclic_register for watchdog@30280000 failed

imx8 regression: cyclic_register for watchdog@30280000 failed

[Tim Harvey]

Greetings,

I've noticed a regression since the merge of the cyclic framework use
for watchdog on my imx8m boards:

cyclic_register for watchdog@30280000 failed
WDT:   Failed to start watchdog@30280000

A bisect lead me to the following 3 sequential patches:
29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
^^^ bad
881d4108257a cyclic: Introduce schedule() function
^^^ unbuildable
c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
^^^ unbootable

Before I did in much deeper has anyone else run into and/or resolved
this? I'm wondering if something is missing from defconfig?

Best Regards,

Tim

Re: imx8 regression: cyclic_register for watchdog@30280000 failed

[Stefan Roese]

Hi Tim,

On 25.10.22 18:32, Tim Harvey wrote:
> Greetings,
> 
> I've noticed a regression since the merge of the cyclic framework use
> for watchdog on my imx8m boards:
> 
> cyclic_register for watchdog@30280000 failed
> WDT:   Failed to start watchdog@30280000

Could you please post the complete bootlog? Which board(s) are these
(defconfig)?

> A bisect lead me to the following 3 sequential patches:
> 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
> ^^^ bad
> 881d4108257a cyclic: Introduce schedule() function
> ^^^ unbuildable
> c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
> ^^^ unbootable
> 
> Before I did in much deeper has anyone else run into and/or resolved
> this? I'm wondering if something is missing from defconfig?

I'll look into this today.

Thanks,
Stefan

Re: imx8 regression: cyclic_register for watchdog@30280000 failed

[Rasmus Villemoes]

On 25/10/2022 18.32, Tim Harvey wrote:
> Greetings,
> 
> I've noticed a regression since the merge of the cyclic framework use
> for watchdog on my imx8m boards:
> 
> cyclic_register for watchdog@30280000 failed
> WDT:   Failed to start watchdog@30280000
> 
> A bisect lead me to the following 3 sequential patches:
> 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
> ^^^ bad
> 881d4108257a cyclic: Introduce schedule() function
> ^^^ unbuildable
> c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
> ^^^ unbootable

Can you provide some more details on "unbootable" and "unbuildable"?
I.e., what build error do you get for the middle patch, and what do you
see on the console with the "unbootable" image?

Also, the .configs used in each case might be interesting, certainly all
lines containing CYCLIC, WATCHDOG or WDT.

One thing I notice is that I think wdt_stop should unregister the cyclic
function; there's really no point having the cyclic framework keep
calling wdt_cyclic() only to bail out at "if (!priv->running)" -
moreover, it's almost certainly a bug if the device is started again via
another wdt_start(), because then we have two cyclic instances
registered for the same device.

I also think the cyclic API is somewhat misdesigned. The storage for the
cyclic metadata should be provided by the caller (e.g. in the watchdog
case just embedded as part of struct wdt_priv), so that
cyclic_register() can never fail. Otherwise we have this awkward
situation where ops->start() have already been called and succeeded, but
then perhaps we fail to register the cyclic instance; should we then do
wdt_stop(), and what if _that_ then fails?

This also allows the callback to be a little more type-safe; let the
callback take a "struct cyclic_info *" as argument, and then the
callback can use e.g. container_of to get the containing wdt_priv.

Rasmus

Re: imx8 regression: cyclic_register for watchdog@30280000 failed

[Tim Harvey]

On Wed, Oct 26, 2022 at 5:33 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 25/10/2022 18.32, Tim Harvey wrote:
> > Greetings,
> >
> > I've noticed a regression since the merge of the cyclic framework use
> > for watchdog on my imx8m boards:
> >
> > cyclic_register for watchdog@30280000 failed
> > WDT:   Failed to start watchdog@30280000
> >
> > A bisect lead me to the following 3 sequential patches:
> > 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
> > ^^^ bad
> > 881d4108257a cyclic: Introduce schedule() function
> > ^^^ unbuildable
> > c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
> > ^^^ unbootable
>
> Can you provide some more details on "unbootable" and "unbuildable"?
> I.e., what build error do you get for the middle patch, and what do you
> see on the console with the "unbootable" image?

Rasmus,

Sure. I'm building and testing for imx8mm_venice.

Here are the patches in question listed in commit order:
d219fc06b30d configs: Resync with savedefconfig
^^^ no issues found

c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
^^^ unbootable for imx8mm_venice - hangs after SPL banner:
U-Boot SPL 2022.10-rc3-00241-gc2fd0ca1a822 (Oct 26 2022 - 10:08:54 -0700)
^^^ this occurs because 'uclass_get_device_by_driver(UCLASS_MISC,
DM_DRIVER_GET(gsc), &dev))' in gateworks/venice/spl.c board_init_f
hangs and I'm not clear why this patch affects that

881d4108257a cyclic: Introduce schedule() function
^^^ unbuildable for imx8mm_venice_defconfig
 CC      arch/arm/lib/sections.o
In file included from include/asm-generic/global_data.h:23,
                 from ./arch/arm/include/asm/global_data.h:102,
                 from include/dm/of.h:11,
                 from include/dm/ofnode.h:12,
                 from include/dm/device.h:13,
                 from include/linux/mtd/mtd.h:26,
                 from include/nand.h:17,
                 from cmd/bootm.c:17:
include/cyclic.h:116:19: error: macro "schedule" passed 1 arguments, but takes j
ust 0
 void schedule(void);

29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
^^^ build issue resolved, boot issue on imx8mm-venice resolved, but
this is where we now encounter watchdog failures in both the SPL and
U-Boot:

SPL:
cyclic_register for watchdog@30280000 failed
WDT:   Failed to start watchdog@30280000

The failure here is 'Cyclic IF not ready yet' (which should probably
be an error print not a debug print). In this case the watchdog gets
started but never reset via cyclic. This is due to cyclic_init never
being called in the SPL - where is that supposed to occur?

U-Boot:
cyclic function watchdog@30280000 took too long: 2976us vs 1000us max, disabling

Here also the watchdog gets started but never reset via cyclic so the
board will reset itself after 60s sitting in U-Boot for example.

The issue here is that for some reason the first call to wdt_cyclic()
takes about 3ms vs subsequent calls taking about 185us on this
platform which exceeds the 1ms default max of
CONFIG_CYCLIC_MAX_CPU_TIME_US and thus the watchdog reset is disabled
and the board resets in 60s. Setting
CONFIG_CYCLIC_MAX_CPU_TIME_US=5000 resolves that issue but this feels
like a workaround that perhaps shouldn't be required. I assume the
extra time in the first call is the probing of the device. So I think
the fix for this U-Boot regression is to move the init part of
wdt_cyclic to wdt_start() and have wdt_cyclic() only call wdt_reset()

Fabio, I assume you see this on other imx8m boards?

> Also, the .configs used in each case might be interesting, certainly all
> lines containing CYCLIC, WATCHDOG or WDT.
>

$ grep CYCLIC .config
CONFIG_CYCLIC=y
CONFIG_CYCLIC_MAX_CPU_TIME_US=1000
# CONFIG_CMD_MX_CYCLIC is not set
CONFIG_CMD_CYCLIC=y
$ grep WATCHDOG .config
CONFIG_SPL_WATCHDOG=y
CONFIG_SYSRESET_WATCHDOG=y
# CONFIG_SYSRESET_WATCHDOG_AUTO is not set
CONFIG_WATCHDOG=y
CONFIG_WATCHDOG_AUTOSTART=y
CONFIG_WATCHDOG_TIMEOUT_MSECS=60000
CONFIG_IMX_WATCHDOG=y
# CONFIG_WATCHDOG_RESET_DISABLE is not set
# CONFIG_ULP_WATCHDOG is not set
# CONFIG_DESIGNWARE_WATCHDOG is not set
# CONFIG_XILINX_TB_WATCHDOG is not set
$ grep WDT .config
# CONFIG_CMD_WDT is not set
CONFIG_WDT=y
# CONFIG_WDT_APPLE is not set
# CONFIG_WDT_ASPEED is not set
# CONFIG_WDT_AST2600 is not set
# CONFIG_WDT_AT91 is not set
# CONFIG_WDT_CDNS is not set
# CONFIG_WDT_CORTINA is not set
# CONFIG_WDT_GPIO is not set
# CONFIG_WDT_MAX6370 is not set
# CONFIG_WDT_MESON_GXBB is not set
# CONFIG_WDT_ORION is not set
# CONFIG_WDT_SBSA is not set
# CONFIG_WDT_SP805 is not set
# CONFIG_WDT_STM32MP is not set
CONFIG_SPL_WDT=y

> One thing I notice is that I think wdt_stop should unregister the cyclic
> function; there's really no point having the cyclic framework keep
> calling wdt_cyclic() only to bail out at "if (!priv->running)" -
> moreover, it's almost certainly a bug if the device is started again via
> another wdt_start(), because then we have two cyclic instances
> registered for the same device.
>
> I also think the cyclic API is somewhat misdesigned. The storage for the
> cyclic metadata should be provided by the caller (e.g. in the watchdog
> case just embedded as part of struct wdt_priv), so that
> cyclic_register() can never fail. Otherwise we have this awkward
> situation where ops->start() have already been called and succeeded, but
> then perhaps we fail to register the cyclic instance; should we then do
> wdt_stop(), and what if _that_ then fails?
>
> This also allows the callback to be a little more type-safe; let the
> callback take a "struct cyclic_info *" as argument, and then the
> callback can use e.g. container_of to get the containing wdt_priv.
>

I really didn't follow the various submissions for the cyclic
framework so I don't have any input on that end.

Best Regards,

Tim

Re: imx8 regression: cyclic_register for watchdog@30280000 failed

[Stefan Roese]

[-- Attachment #1: Type: text/plain, Size: 6980 bytes --]

Tim,

On 26.10.22 21:06, Tim Harvey wrote:
> On Wed, Oct 26, 2022 at 5:33 AM Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> On 25/10/2022 18.32, Tim Harvey wrote:
>>> Greetings,
>>>
>>> I've noticed a regression since the merge of the cyclic framework use
>>> for watchdog on my imx8m boards:
>>>
>>> cyclic_register for watchdog@30280000 failed
>>> WDT:   Failed to start watchdog@30280000
>>>
>>> A bisect lead me to the following 3 sequential patches:
>>> 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
>>> ^^^ bad
>>> 881d4108257a cyclic: Introduce schedule() function
>>> ^^^ unbuildable
>>> c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
>>> ^^^ unbootable
>>
>> Can you provide some more details on "unbootable" and "unbuildable"?
>> I.e., what build error do you get for the middle patch, and what do you
>> see on the console with the "unbootable" image?
> 
> Rasmus,
> 
> Sure. I'm building and testing for imx8mm_venice.
> 
> Here are the patches in question listed in commit order:
> d219fc06b30d configs: Resync with savedefconfig
> ^^^ no issues found
> 
> c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
> ^^^ unbootable for imx8mm_venice - hangs after SPL banner:
> U-Boot SPL 2022.10-rc3-00241-gc2fd0ca1a822 (Oct 26 2022 - 10:08:54 -0700)
> ^^^ this occurs because 'uclass_get_device_by_driver(UCLASS_MISC,
> DM_DRIVER_GET(gsc), &dev))' in gateworks/venice/spl.c board_init_f
> hangs and I'm not clear why this patch affects that
> 
> 881d4108257a cyclic: Introduce schedule() function
> ^^^ unbuildable for imx8mm_venice_defconfig
>   CC      arch/arm/lib/sections.o
> In file included from include/asm-generic/global_data.h:23,
>                   from ./arch/arm/include/asm/global_data.h:102,
>                   from include/dm/of.h:11,
>                   from include/dm/ofnode.h:12,
>                   from include/dm/device.h:13,
>                   from include/linux/mtd/mtd.h:26,
>                   from include/nand.h:17,
>                   from cmd/bootm.c:17:
> include/cyclic.h:116:19: error: macro "schedule" passed 1 arguments, but takes j
> ust 0
>   void schedule(void);
> 
> 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
> ^^^ build issue resolved, boot issue on imx8mm-venice resolved, but
> this is where we now encounter watchdog failures in both the SPL and
> U-Boot:
> 
> SPL:
> cyclic_register for watchdog@30280000 failed
> WDT:   Failed to start watchdog@30280000
> 
> The failure here is 'Cyclic IF not ready yet' (which should probably
> be an error print not a debug print).

Ye, makes sense. I'll change this.

> In this case the watchdog gets
> started but never reset via cyclic. This is due to cyclic_init never
> being called in the SPL - where is that supposed to occur?

I did not have many targets with WDT in SPL to test with. Seems that
I missed to call into cyclic_init() here.

> U-Boot:
> cyclic function watchdog@30280000 took too long: 2976us vs 1000us max, disabling
> 
> Here also the watchdog gets started but never reset via cyclic so the
> board will reset itself after 60s sitting in U-Boot for example.

This is already changed in master since yesterday. Now only a warning
will be shown upon long execution time but the function will not
be disabled. So please re-check with master and report if this
works better.

> The issue here is that for some reason the first call to wdt_cyclic()
> takes about 3ms vs subsequent calls taking about 185us on this
> platform which exceeds the 1ms default max of
> CONFIG_CYCLIC_MAX_CPU_TIME_US and thus the watchdog reset is disabled
> and the board resets in 60s. Setting
> CONFIG_CYCLIC_MAX_CPU_TIME_US=5000 resolves that issue but this feels
> like a workaround that perhaps shouldn't be required.

I agree.

> I assume the
> extra time in the first call is the probing of the device. So I think
> the fix for this U-Boot regression is to move the init part of
> wdt_cyclic to wdt_start() and have wdt_cyclic() only call wdt_reset()
> 
> Fabio, I assume you see this on other imx8m boards?
> 
>> Also, the .configs used in each case might be interesting, certainly all
>> lines containing CYCLIC, WATCHDOG or WDT.
>>
> 
> $ grep CYCLIC .config
> CONFIG_CYCLIC=y
> CONFIG_CYCLIC_MAX_CPU_TIME_US=1000
> # CONFIG_CMD_MX_CYCLIC is not set
> CONFIG_CMD_CYCLIC=y
> $ grep WATCHDOG .config
> CONFIG_SPL_WATCHDOG=y
> CONFIG_SYSRESET_WATCHDOG=y
> # CONFIG_SYSRESET_WATCHDOG_AUTO is not set
> CONFIG_WATCHDOG=y
> CONFIG_WATCHDOG_AUTOSTART=y
> CONFIG_WATCHDOG_TIMEOUT_MSECS=60000
> CONFIG_IMX_WATCHDOG=y
> # CONFIG_WATCHDOG_RESET_DISABLE is not set
> # CONFIG_ULP_WATCHDOG is not set
> # CONFIG_DESIGNWARE_WATCHDOG is not set
> # CONFIG_XILINX_TB_WATCHDOG is not set
> $ grep WDT .config
> # CONFIG_CMD_WDT is not set
> CONFIG_WDT=y
> # CONFIG_WDT_APPLE is not set
> # CONFIG_WDT_ASPEED is not set
> # CONFIG_WDT_AST2600 is not set
> # CONFIG_WDT_AT91 is not set
> # CONFIG_WDT_CDNS is not set
> # CONFIG_WDT_CORTINA is not set
> # CONFIG_WDT_GPIO is not set
> # CONFIG_WDT_MAX6370 is not set
> # CONFIG_WDT_MESON_GXBB is not set
> # CONFIG_WDT_ORION is not set
> # CONFIG_WDT_SBSA is not set
> # CONFIG_WDT_SP805 is not set
> # CONFIG_WDT_STM32MP is not set
> CONFIG_SPL_WDT=y
> 
>> One thing I notice is that I think wdt_stop should unregister the cyclic
>> function; there's really no point having the cyclic framework keep
>> calling wdt_cyclic() only to bail out at "if (!priv->running)" -
>> moreover, it's almost certainly a bug if the device is started again via
>> another wdt_start(), because then we have two cyclic instances
>> registered for the same device.
>>
>> I also think the cyclic API is somewhat misdesigned. The storage for the
>> cyclic metadata should be provided by the caller (e.g. in the watchdog
>> case just embedded as part of struct wdt_priv), so that
>> cyclic_register() can never fail. Otherwise we have this awkward
>> situation where ops->start() have already been called and succeeded, but
>> then perhaps we fail to register the cyclic instance; should we then do
>> wdt_stop(), and what if _that_ then fails?
>>
>> This also allows the callback to be a little more type-safe; let the
>> callback take a "struct cyclic_info *" as argument, and then the
>> callback can use e.g. container_of to get the containing wdt_priv.
>>
> 
> I really didn't follow the various submissions for the cyclic
> framework so I don't have any input on that end.

We were aware that this cyclic IF and especially the WDT move to
cyclic might produce some breakages. But it was impossible, at least
for me, to foresee all potential issue. So it was merged very early
in this release cycle, so that we have time to fix all problems.

I've a small patch that might solve the SPL problem you are seeing.
The U-Boot proper issue should be fixed in master already. Please
give this attached patch a try and let me know, if it works.

Thanks,
Stefan

[-- Attachment #2: 0001-spl-cylic-Call-cyclic_init-in-board_init_r.patch --]
[-- Type: text/x-patch, Size: 1025 bytes --]

From 00804bb118db35d19a35431561a69c812106efc2 Mon Sep 17 00:00:00 2001
From: Stefan Roese <sr@denx.de>
Date: Thu, 27 Oct 2022 08:23:59 +0200
Subject: [PATCH] spl: cylic: Call cyclic_init() in board_init_r()

It's necessary to init the cyclic IF so that functions can be
registered. Especially the SPL WDT is a user here. This patch
adds the missing call to cyclic_init() in the SPL version of
board_init_r().

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 common/spl/spl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index 752b5b247cb8..fab369c7144f 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -798,6 +798,9 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 	spl_board_init();
 #endif
 
+	/* Init the cyclic infrastructure after malloc and timer is ready */
+	cyclic_init();
+
 #if defined(CONFIG_SPL_WATCHDOG) && CONFIG_IS_ENABLED(WDT)
 	initr_watchdog();
 #endif
-- 
2.38.1

Re: imx8 regression: cyclic_register for watchdog@30280000 failed

[Rasmus Villemoes]

On 27/10/2022 08.32, Stefan Roese wrote:

>> In this case the watchdog gets
>> started but never reset via cyclic. This is due to cyclic_init never
>> being called in the SPL - where is that supposed to occur?
> 
> I did not have many targets with WDT in SPL to test with. Seems that
> I missed to call into cyclic_init() here.

I have an idea for getting rid of cyclic_init() (and thus the need for
ensuring it gets called) completely, give me a day or two to spin a
small series.

Rasmus

Re: imx8 regression: cyclic_register for watchdog@30280000 failed

[Stefan Roese]

On 27.10.22 12:06, Rasmus Villemoes wrote:
> On 27/10/2022 08.32, Stefan Roese wrote:
> 
>>> In this case the watchdog gets
>>> started but never reset via cyclic. This is due to cyclic_init never
>>> being called in the SPL - where is that supposed to occur?
>>
>> I did not have many targets with WDT in SPL to test with. Seems that
>> I missed to call into cyclic_init() here.
> 
> I have an idea for getting rid of cyclic_init() (and thus the need for
> ensuring it gets called) completely, give me a day or two to spin a
> small series.

Perfect Rasmus. Thanks for looking into this. I've read your other
mail as well but haven't found the time yet to answer.

Still the test from Tim would be interesting, if quickly possible -
which I assume should be possible.

Thanks,
Stefan

Re: imx8 regression: cyclic_register for watchdog@30280000 failed

[Tim Harvey]

On Wed, Oct 26, 2022 at 11:32 PM Stefan Roese <sr@denx.de> wrote:
>
> Tim,
>
> On 26.10.22 21:06, Tim Harvey wrote:
> > On Wed, Oct 26, 2022 at 5:33 AM Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> On 25/10/2022 18.32, Tim Harvey wrote:
> >>> Greetings,
> >>>
> >>> I've noticed a regression since the merge of the cyclic framework use
> >>> for watchdog on my imx8m boards:
> >>>
> >>> cyclic_register for watchdog@30280000 failed
> >>> WDT:   Failed to start watchdog@30280000
> >>>
> >>> A bisect lead me to the following 3 sequential patches:
> >>> 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
> >>> ^^^ bad
> >>> 881d4108257a cyclic: Introduce schedule() function
> >>> ^^^ unbuildable
> >>> c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
> >>> ^^^ unbootable
> >>
> >> Can you provide some more details on "unbootable" and "unbuildable"?
> >> I.e., what build error do you get for the middle patch, and what do you
> >> see on the console with the "unbootable" image?
> >
> > Rasmus,
> >
> > Sure. I'm building and testing for imx8mm_venice.
> >
> > Here are the patches in question listed in commit order:
> > d219fc06b30d configs: Resync with savedefconfig
> > ^^^ no issues found
> >
> > c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
> > ^^^ unbootable for imx8mm_venice - hangs after SPL banner:
> > U-Boot SPL 2022.10-rc3-00241-gc2fd0ca1a822 (Oct 26 2022 - 10:08:54 -0700)
> > ^^^ this occurs because 'uclass_get_device_by_driver(UCLASS_MISC,
> > DM_DRIVER_GET(gsc), &dev))' in gateworks/venice/spl.c board_init_f
> > hangs and I'm not clear why this patch affects that
> >
> > 881d4108257a cyclic: Introduce schedule() function
> > ^^^ unbuildable for imx8mm_venice_defconfig
> >   CC      arch/arm/lib/sections.o
> > In file included from include/asm-generic/global_data.h:23,
> >                   from ./arch/arm/include/asm/global_data.h:102,
> >                   from include/dm/of.h:11,
> >                   from include/dm/ofnode.h:12,
> >                   from include/dm/device.h:13,
> >                   from include/linux/mtd/mtd.h:26,
> >                   from include/nand.h:17,
> >                   from cmd/bootm.c:17:
> > include/cyclic.h:116:19: error: macro "schedule" passed 1 arguments, but takes j
> > ust 0
> >   void schedule(void);
> >
> > 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
> > ^^^ build issue resolved, boot issue on imx8mm-venice resolved, but
> > this is where we now encounter watchdog failures in both the SPL and
> > U-Boot:
> >
> > SPL:
> > cyclic_register for watchdog@30280000 failed
> > WDT:   Failed to start watchdog@30280000
> >
> > The failure here is 'Cyclic IF not ready yet' (which should probably
> > be an error print not a debug print).
>
> Ye, makes sense. I'll change this.
>
> > In this case the watchdog gets
> > started but never reset via cyclic. This is due to cyclic_init never
> > being called in the SPL - where is that supposed to occur?
>
> I did not have many targets with WDT in SPL to test with. Seems that
> I missed to call into cyclic_init() here.
>
> > U-Boot:
> > cyclic function watchdog@30280000 took too long: 2976us vs 1000us max, disabling
> >
> > Here also the watchdog gets started but never reset via cyclic so the
> > board will reset itself after 60s sitting in U-Boot for example.
>
> This is already changed in master since yesterday. Now only a warning
> will be shown upon long execution time but the function will not
> be disabled. So please re-check with master and report if this
> works better.

confirmed - the cyclic call no longer gets cancelled and its now just a warning

>
> > The issue here is that for some reason the first call to wdt_cyclic()
> > takes about 3ms vs subsequent calls taking about 185us on this
> > platform which exceeds the 1ms default max of
> > CONFIG_CYCLIC_MAX_CPU_TIME_US and thus the watchdog reset is disabled
> > and the board resets in 60s. Setting
> > CONFIG_CYCLIC_MAX_CPU_TIME_US=5000 resolves that issue but this feels
> > like a workaround that perhaps shouldn't be required.
>
> I agree.

Sounds like Rasmus is going to spin a patch for this but at least now
it's just a warning message.

>
> > I assume the
> > extra time in the first call is the probing of the device. So I think
> > the fix for this U-Boot regression is to move the init part of
> > wdt_cyclic to wdt_start() and have wdt_cyclic() only call wdt_reset()
> >
> > Fabio, I assume you see this on other imx8m boards?
> >
> >> Also, the .configs used in each case might be interesting, certainly all
> >> lines containing CYCLIC, WATCHDOG or WDT.
> >>
> >
> > $ grep CYCLIC .config
> > CONFIG_CYCLIC=y
> > CONFIG_CYCLIC_MAX_CPU_TIME_US=1000
> > # CONFIG_CMD_MX_CYCLIC is not set
> > CONFIG_CMD_CYCLIC=y
> > $ grep WATCHDOG .config
> > CONFIG_SPL_WATCHDOG=y
> > CONFIG_SYSRESET_WATCHDOG=y
> > # CONFIG_SYSRESET_WATCHDOG_AUTO is not set
> > CONFIG_WATCHDOG=y
> > CONFIG_WATCHDOG_AUTOSTART=y
> > CONFIG_WATCHDOG_TIMEOUT_MSECS=60000
> > CONFIG_IMX_WATCHDOG=y
> > # CONFIG_WATCHDOG_RESET_DISABLE is not set
> > # CONFIG_ULP_WATCHDOG is not set
> > # CONFIG_DESIGNWARE_WATCHDOG is not set
> > # CONFIG_XILINX_TB_WATCHDOG is not set
> > $ grep WDT .config
> > # CONFIG_CMD_WDT is not set
> > CONFIG_WDT=y
> > # CONFIG_WDT_APPLE is not set
> > # CONFIG_WDT_ASPEED is not set
> > # CONFIG_WDT_AST2600 is not set
> > # CONFIG_WDT_AT91 is not set
> > # CONFIG_WDT_CDNS is not set
> > # CONFIG_WDT_CORTINA is not set
> > # CONFIG_WDT_GPIO is not set
> > # CONFIG_WDT_MAX6370 is not set
> > # CONFIG_WDT_MESON_GXBB is not set
> > # CONFIG_WDT_ORION is not set
> > # CONFIG_WDT_SBSA is not set
> > # CONFIG_WDT_SP805 is not set
> > # CONFIG_WDT_STM32MP is not set
> > CONFIG_SPL_WDT=y
> >
> >> One thing I notice is that I think wdt_stop should unregister the cyclic
> >> function; there's really no point having the cyclic framework keep
> >> calling wdt_cyclic() only to bail out at "if (!priv->running)" -
> >> moreover, it's almost certainly a bug if the device is started again via
> >> another wdt_start(), because then we have two cyclic instances
> >> registered for the same device.
> >>
> >> I also think the cyclic API is somewhat misdesigned. The storage for the
> >> cyclic metadata should be provided by the caller (e.g. in the watchdog
> >> case just embedded as part of struct wdt_priv), so that
> >> cyclic_register() can never fail. Otherwise we have this awkward
> >> situation where ops->start() have already been called and succeeded, but
> >> then perhaps we fail to register the cyclic instance; should we then do
> >> wdt_stop(), and what if _that_ then fails?
> >>
> >> This also allows the callback to be a little more type-safe; let the
> >> callback take a "struct cyclic_info *" as argument, and then the
> >> callback can use e.g. container_of to get the containing wdt_priv.
> >>
> >
> > I really didn't follow the various submissions for the cyclic
> > framework so I don't have any input on that end.
>
> We were aware that this cyclic IF and especially the WDT move to
> cyclic might produce some breakages. But it was impossible, at least
> for me, to foresee all potential issue. So it was merged very early
> in this release cycle, so that we have time to fix all problems.

understood! I haven't done a lot of testing until now because the imx
changes get left until the end of the merge window. I would love to
see that change in the next release.

>
> I've a small patch that might solve the SPL problem you are seeing.
> The U-Boot proper issue should be fixed in master already. Please
> give this attached patch a try and let me know, if it works.
>

Yes, this does resolve the SPL issue.

Tim

Re: imx8 regression: cyclic_register for watchdog@30280000 failed

[Stefan Roese]

Tim,

On 27.10.22 17:34, Tim Harvey wrote:
> On Wed, Oct 26, 2022 at 11:32 PM Stefan Roese <sr@denx.de> wrote:
>>
>> Tim,
>>
>> On 26.10.22 21:06, Tim Harvey wrote:
>>> On Wed, Oct 26, 2022 at 5:33 AM Rasmus Villemoes
>>> <rasmus.villemoes@prevas.dk> wrote:
>>>>
>>>> On 25/10/2022 18.32, Tim Harvey wrote:
>>>>> Greetings,
>>>>>
>>>>> I've noticed a regression since the merge of the cyclic framework use
>>>>> for watchdog on my imx8m boards:
>>>>>
>>>>> cyclic_register for watchdog@30280000 failed
>>>>> WDT:   Failed to start watchdog@30280000
>>>>>
>>>>> A bisect lead me to the following 3 sequential patches:
>>>>> 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
>>>>> ^^^ bad
>>>>> 881d4108257a cyclic: Introduce schedule() function
>>>>> ^^^ unbuildable
>>>>> c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
>>>>> ^^^ unbootable
>>>>
>>>> Can you provide some more details on "unbootable" and "unbuildable"?
>>>> I.e., what build error do you get for the middle patch, and what do you
>>>> see on the console with the "unbootable" image?
>>>
>>> Rasmus,
>>>
>>> Sure. I'm building and testing for imx8mm_venice.
>>>
>>> Here are the patches in question listed in commit order:
>>> d219fc06b30d configs: Resync with savedefconfig
>>> ^^^ no issues found
>>>
>>> c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
>>> ^^^ unbootable for imx8mm_venice - hangs after SPL banner:
>>> U-Boot SPL 2022.10-rc3-00241-gc2fd0ca1a822 (Oct 26 2022 - 10:08:54 -0700)
>>> ^^^ this occurs because 'uclass_get_device_by_driver(UCLASS_MISC,
>>> DM_DRIVER_GET(gsc), &dev))' in gateworks/venice/spl.c board_init_f
>>> hangs and I'm not clear why this patch affects that
>>>
>>> 881d4108257a cyclic: Introduce schedule() function
>>> ^^^ unbuildable for imx8mm_venice_defconfig
>>>    CC      arch/arm/lib/sections.o
>>> In file included from include/asm-generic/global_data.h:23,
>>>                    from ./arch/arm/include/asm/global_data.h:102,
>>>                    from include/dm/of.h:11,
>>>                    from include/dm/ofnode.h:12,
>>>                    from include/dm/device.h:13,
>>>                    from include/linux/mtd/mtd.h:26,
>>>                    from include/nand.h:17,
>>>                    from cmd/bootm.c:17:
>>> include/cyclic.h:116:19: error: macro "schedule" passed 1 arguments, but takes j
>>> ust 0
>>>    void schedule(void);
>>>
>>> 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
>>> ^^^ build issue resolved, boot issue on imx8mm-venice resolved, but
>>> this is where we now encounter watchdog failures in both the SPL and
>>> U-Boot:
>>>
>>> SPL:
>>> cyclic_register for watchdog@30280000 failed
>>> WDT:   Failed to start watchdog@30280000
>>>
>>> The failure here is 'Cyclic IF not ready yet' (which should probably
>>> be an error print not a debug print).
>>
>> Ye, makes sense. I'll change this.
>>
>>> In this case the watchdog gets
>>> started but never reset via cyclic. This is due to cyclic_init never
>>> being called in the SPL - where is that supposed to occur?
>>
>> I did not have many targets with WDT in SPL to test with. Seems that
>> I missed to call into cyclic_init() here.
>>
>>> U-Boot:
>>> cyclic function watchdog@30280000 took too long: 2976us vs 1000us max, disabling
>>>
>>> Here also the watchdog gets started but never reset via cyclic so the
>>> board will reset itself after 60s sitting in U-Boot for example.
>>
>> This is already changed in master since yesterday. Now only a warning
>> will be shown upon long execution time but the function will not
>> be disabled. So please re-check with master and report if this
>> works better.
> 
> confirmed - the cyclic call no longer gets cancelled and its now just a warning

Good. We should improve this, that this warning will not be displayed,
at least in the WDT event, at some point.

>>> The issue here is that for some reason the first call to wdt_cyclic()
>>> takes about 3ms vs subsequent calls taking about 185us on this
>>> platform which exceeds the 1ms default max of
>>> CONFIG_CYCLIC_MAX_CPU_TIME_US and thus the watchdog reset is disabled
>>> and the board resets in 60s. Setting
>>> CONFIG_CYCLIC_MAX_CPU_TIME_US=5000 resolves that issue but this feels
>>> like a workaround that perhaps shouldn't be required.
>>
>> I agree.
> 
> Sounds like Rasmus is going to spin a patch for this but at least now
> it's just a warning message.

Yes. Let's wait for Rasmus'es work on this.

>>> I assume the
>>> extra time in the first call is the probing of the device. So I think
>>> the fix for this U-Boot regression is to move the init part of
>>> wdt_cyclic to wdt_start() and have wdt_cyclic() only call wdt_reset()
>>>
>>> Fabio, I assume you see this on other imx8m boards?
>>>
>>>> Also, the .configs used in each case might be interesting, certainly all
>>>> lines containing CYCLIC, WATCHDOG or WDT.
>>>>
>>>
>>> $ grep CYCLIC .config
>>> CONFIG_CYCLIC=y
>>> CONFIG_CYCLIC_MAX_CPU_TIME_US=1000
>>> # CONFIG_CMD_MX_CYCLIC is not set
>>> CONFIG_CMD_CYCLIC=y
>>> $ grep WATCHDOG .config
>>> CONFIG_SPL_WATCHDOG=y
>>> CONFIG_SYSRESET_WATCHDOG=y
>>> # CONFIG_SYSRESET_WATCHDOG_AUTO is not set
>>> CONFIG_WATCHDOG=y
>>> CONFIG_WATCHDOG_AUTOSTART=y
>>> CONFIG_WATCHDOG_TIMEOUT_MSECS=60000
>>> CONFIG_IMX_WATCHDOG=y
>>> # CONFIG_WATCHDOG_RESET_DISABLE is not set
>>> # CONFIG_ULP_WATCHDOG is not set
>>> # CONFIG_DESIGNWARE_WATCHDOG is not set
>>> # CONFIG_XILINX_TB_WATCHDOG is not set
>>> $ grep WDT .config
>>> # CONFIG_CMD_WDT is not set
>>> CONFIG_WDT=y
>>> # CONFIG_WDT_APPLE is not set
>>> # CONFIG_WDT_ASPEED is not set
>>> # CONFIG_WDT_AST2600 is not set
>>> # CONFIG_WDT_AT91 is not set
>>> # CONFIG_WDT_CDNS is not set
>>> # CONFIG_WDT_CORTINA is not set
>>> # CONFIG_WDT_GPIO is not set
>>> # CONFIG_WDT_MAX6370 is not set
>>> # CONFIG_WDT_MESON_GXBB is not set
>>> # CONFIG_WDT_ORION is not set
>>> # CONFIG_WDT_SBSA is not set
>>> # CONFIG_WDT_SP805 is not set
>>> # CONFIG_WDT_STM32MP is not set
>>> CONFIG_SPL_WDT=y
>>>
>>>> One thing I notice is that I think wdt_stop should unregister the cyclic
>>>> function; there's really no point having the cyclic framework keep
>>>> calling wdt_cyclic() only to bail out at "if (!priv->running)" -
>>>> moreover, it's almost certainly a bug if the device is started again via
>>>> another wdt_start(), because then we have two cyclic instances
>>>> registered for the same device.
>>>>
>>>> I also think the cyclic API is somewhat misdesigned. The storage for the
>>>> cyclic metadata should be provided by the caller (e.g. in the watchdog
>>>> case just embedded as part of struct wdt_priv), so that
>>>> cyclic_register() can never fail. Otherwise we have this awkward
>>>> situation where ops->start() have already been called and succeeded, but
>>>> then perhaps we fail to register the cyclic instance; should we then do
>>>> wdt_stop(), and what if _that_ then fails?
>>>>
>>>> This also allows the callback to be a little more type-safe; let the
>>>> callback take a "struct cyclic_info *" as argument, and then the
>>>> callback can use e.g. container_of to get the containing wdt_priv.
>>>>
>>>
>>> I really didn't follow the various submissions for the cyclic
>>> framework so I don't have any input on that end.
>>
>> We were aware that this cyclic IF and especially the WDT move to
>> cyclic might produce some breakages. But it was impossible, at least
>> for me, to foresee all potential issue. So it was merged very early
>> in this release cycle, so that we have time to fix all problems.
> 
> understood! I haven't done a lot of testing until now because the imx
> changes get left until the end of the merge window. I would love to
> see that change in the next release.

We will definitely fix this cyclic WDT integration in this release
cycle.

>> I've a small patch that might solve the SPL problem you are seeing.
>> The U-Boot proper issue should be fixed in master already. Please
>> give this attached patch a try and let me know, if it works.
>>
> 
> Yes, this does resolve the SPL issue.

Good. Thanks for testing.

Thanks,
Stefan

Re: imx8 regression: cyclic_register for watchdog@30280000 failed

[Rasmus Villemoes]

On 26/10/2022 21.06, Tim Harvey wrote:

> 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
> ^^^ build issue resolved, boot issue on imx8mm-venice resolved, but
> this is where we now encounter watchdog failures in both the SPL and
> U-Boot:
> 
> SPL:
> cyclic_register for watchdog@30280000 failed
> WDT:   Failed to start watchdog@30280000
> 
> The failure here is 'Cyclic IF not ready yet' (which should probably
> be an error print not a debug print). In this case the watchdog gets
> started but never reset via cyclic. This is due to cyclic_init never
> being called in the SPL - where is that supposed to occur?

Actually, the code both in current master and at 29caf9305b6f does

        if (!gd->cyclic->cyclic_ready) {
                pr_debug("Cyclic IF not ready yet\n");
                return NULL;
        }

and when cyclic_init() hasn't been called, that gd->cyclic is NULL.
Unfortunately, on the imx8m platforms, 0 is a perfectly valid address
(that's where the bootrom code lives), so the fact that this even
triggers is mostly random, AFAICT; there might as well have been a
non-zero byte at that offset from address 0. But then things would
probably have exploded in interesting ways when the new cyclic_info
would be added to the garbage gd->cyclic->cyclic_list (maybe the write
to that memory would just result in a reset or hang).

Anyway, the series I just sent removes this check entirely and should
work in SPL as well without the cyclic_init(), but please do test.

Rasmus

Re: imx8 regression: cyclic_register for watchdog@30280000 failed

[Stefan Roese]

Hi Rasmus,

On 28.10.22 14:14, Rasmus Villemoes wrote:
> On 26/10/2022 21.06, Tim Harvey wrote:
> 
>> 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
>> ^^^ build issue resolved, boot issue on imx8mm-venice resolved, but
>> this is where we now encounter watchdog failures in both the SPL and
>> U-Boot:
>>
>> SPL:
>> cyclic_register for watchdog@30280000 failed
>> WDT:   Failed to start watchdog@30280000
>>
>> The failure here is 'Cyclic IF not ready yet' (which should probably
>> be an error print not a debug print). In this case the watchdog gets
>> started but never reset via cyclic. This is due to cyclic_init never
>> being called in the SPL - where is that supposed to occur?
> 
> Actually, the code both in current master and at 29caf9305b6f does
> 
>          if (!gd->cyclic->cyclic_ready) {
>                  pr_debug("Cyclic IF not ready yet\n");
>                  return NULL;
>          }
> 
> and when cyclic_init() hasn't been called, that gd->cyclic is NULL.

Ugh!

> Unfortunately, on the imx8m platforms, 0 is a perfectly valid address
> (that's where the bootrom code lives), so the fact that this even
> triggers is mostly random, AFAICT; there might as well have been a
> non-zero byte at that offset from address 0. But then things would
> probably have exploded in interesting ways when the new cyclic_info
> would be added to the garbage gd->cyclic->cyclic_list (maybe the write
> to that memory would just result in a reset or hang).
> 
> Anyway, the series I just sent removes this check entirely and should
> work in SPL as well without the cyclic_init(), but please do test.

Thanks again for working on this. I'll try to look into this very soon
(weekend or beginning of next week). Looks very promising. :)

Thanks,
Stefan