From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A2B5BECAAD2 for ; Mon, 29 Aug 2022 08:09:38 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id AC1DB8458A; Mon, 29 Aug 2022 10:09:36 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1661760576; bh=q0y3CrKXUTULrncpN8pxl+98sltMWn5X+S4w7NnlxEc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=oxfrLEvdC/A52D0UaNqzbFwKDG2z1c5Hc1VRRsSEH7hCBj6yt1psWSbFuD/33acag Stn0ddQlZXtbtG42JlaNVMcd9m+V+fGcqO+8MjyahX57tf5PLA34NuEFnSxXsQDjjo bD4WwEyKknxL1QJEmrX2rZR4rNREG5iNM2N9AJX7RkaImjBHQD7g+4Nh+G9YA+klNe II7zklh8z9bZ8NSPczW29ZyZeSpXWsBQ9m9uuAvmlrKqL0yeXnfDXLL2c15nk9bnT6 aAyha7PT7T3bDiDAZ9IItHoHEZ2SSArv4buEZzHwnm66lcTQ0SSX8wlyS4kkcAJigt kBitlzA4xD9Pg== Received: by phobos.denx.de (Postfix, from userid 109) id 4ED7E84609; Mon, 29 Aug 2022 10:09:35 +0200 (CEST) Received: from mout-u-107.mailbox.org (mout-u-107.mailbox.org [IPv6:2001:67c:2050:101:465::107]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 08A4583F69 for ; Mon, 29 Aug 2022 10:09:33 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=fail smtp.mailfrom=sr@denx.de Received: from smtp1.mailbox.org (smtp1.mailbox.org [IPv6:2001:67c:2050:b231:465::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-u-107.mailbox.org (Postfix) with ESMTPS id 4MGNQ313l3z9sN2; Mon, 29 Aug 2022 10:09:31 +0200 (CEST) Message-ID: <6be9ef5e-90e8-7eef-0c25-5272777f20e9@denx.de> Date: Mon, 29 Aug 2022 10:09:30 +0200 MIME-Version: 1.0 Subject: Re: [RFC PATCH 2/8] watchdog: Integrate watchdog triggering into the cyclic framework Content-Language: en-US To: Rasmus Villemoes , u-boot@lists.denx.de Cc: trini@konsulko.com, sjg@chromium.org References: <20220829062313.32654-1-sr@denx.de> <20220829062313.32654-3-sr@denx.de> <5bcfe2be-8f1b-8e8c-e122-bc0dad517a11@prevas.dk> From: Stefan Roese In-Reply-To: <5bcfe2be-8f1b-8e8c-e122-bc0dad517a11@prevas.dk> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 4MGNQ313l3z9sN2 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On 29.08.22 09:38, Rasmus Villemoes wrote: > On 29/08/2022 08.23, Stefan Roese wrote: >> This patch integrates the watchdog triggering into the recently added >> cyclic infrastructure. Each watchdog device that shall be triggered >> registers it's own cyclic function. This way, multiple watchdog devices >> are still supported, each via a cyclic function with separate trigger >> intervals. >> >> Signed-off-by: Stefan Roese >> --- >> drivers/watchdog/Kconfig | 2 + >> drivers/watchdog/wdt-uclass.c | 80 +++++++++++++++++++++-------------- >> 2 files changed, 50 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> index 50e6a1efba51..e55deaf906b5 100644 >> --- a/drivers/watchdog/Kconfig >> +++ b/drivers/watchdog/Kconfig >> @@ -3,6 +3,7 @@ menu "Watchdog Timer Support" >> config WATCHDOG >> bool "Enable U-Boot watchdog reset" >> depends on !HW_WATCHDOG >> + select CYCLIC >> help >> This option enables U-Boot watchdog support where U-Boot is using >> watchdog_reset function to service watchdog device in U-Boot. Enable >> @@ -74,6 +75,7 @@ config WDT >> bool "Enable driver model for watchdog timer drivers" >> depends on DM >> imply WATCHDOG >> + select CYCLIC >> help >> Enable driver model for watchdog timer. At the moment the API >> is very simple and only supports four operations: >> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c >> index dbf556467d3c..1bf1298d0ecf 100644 >> --- a/drivers/watchdog/wdt-uclass.c >> +++ b/drivers/watchdog/wdt-uclass.c >> @@ -6,6 +6,7 @@ >> #define LOG_CATEGORY UCLASS_WDT >> >> #include >> +#include >> #include >> #include >> #include >> @@ -38,8 +39,33 @@ struct wdt_priv { >> bool running; >> /* No autostart */ >> bool noautostart; >> + >> + struct cyclic_info *cyclic; >> }; >> >> +static void wdt_cyclic(void *ctx) >> +{ >> + struct udevice *dev = ctx; >> + struct wdt_priv *priv; >> + struct uclass *uc; >> + >> + /* Exit if GD is not ready or watchdog is not initialized yet */ >> + if (!gd || !(gd->flags & GD_FLG_WDT_READY)) >> + return; > > Hm, by the time this callback can get called, which is certainly not > before it has been registered, aren't we sure that these conditions are > met? They were clearly needed in the "old" watchdog_reset because that > could end up being invoked more or less from _everywhere_. But here I > think it's redundant. Agreed. I was unsure while moving this code and then forgot about it. I'll drop this these checks in the next version. >> int initr_watchdog(void) >> @@ -105,8 +128,28 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) >> ret = ops->start(dev, timeout_ms, flags); >> if (ret == 0) { >> struct wdt_priv *priv = dev_get_uclass_priv(dev); >> + char str[16]; >> >> priv->running = true; >> + >> + memset(str, 0, 16); >> + if (IS_ENABLED(CONFIG_WATCHDOG)) { >> + /* Register the watchdog driver as a cyclic function */ >> + priv->cyclic = cyclic_register(wdt_cyclic, >> + priv->reset_period * 1000, >> + dev->name, dev); >> + if (!priv->cyclic) { >> + printf("cyclic_register for %s failed\n", >> + dev->name); >> + } else { >> + snprintf(str, 16, "all %ldms", >> + priv->reset_period); >> + } >> + } >> + >> + printf("WDT: Started %s with%s servicing %s (%ds timeout)\n", > > "servicing all 50ms" doesn't sound right to me. "servicing every 50ms" > perhaps? Sound better, thanks. > Also, even if !priv->cyclic we still seem to end here, which > doesn't seem right. Good catch. I'll fix this accordingly. Thanks, Stefan