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 X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 567BFC2B9F2 for ; Sat, 22 May 2021 07:18:20 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id C9276611AD for ; Sat, 22 May 2021 07:18:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C9276611AD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 1A8D080C92; Sat, 22 May 2021 09:18:14 +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=1621667894; bh=cPorC0meA7dx8Aj+FTruV4G+p1KqMwQ9BLE/8CE0B0Q=; h=Subject:From:To:Cc:References:Date:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=coqdU21Eq103Zl719yQ9DE8acC4CZiWfRjAU2b9agAB4o4hdJUt50aez4eGrVMrRA hE0dMy8X4DJXeBUL4O07whBO1agvy4PS5Y3UnzchtktSyscaAAtybQmHrVBgV/kfSJ j5Ur0biGeDB0IzJf8WhhOh9Vf2NoarQCzW2m6vcDpyDVzuI8m4bd2k9bvllRAntnX/ 2X1oP3zA4pn2vHhBCOJigAN6iFF7f+cqDrRt/A8VqBmo6d/8k+UyDXwA2FQc9Yhs33 WeY+I3XlQpDNpM3WpOYWgeisnt68W7ez9j8e9TaVqX4oJJDom/TbTn5bNBH/Ps6J6t TSHPj+cqIBo8w== Received: by phobos.denx.de (Postfix, from userid 109) id 20F2581895; Sat, 22 May 2021 09:18:11 +0200 (CEST) Received: from mout-u-107.mailbox.org (mout-u-107.mailbox.org [91.198.250.252]) (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 2E68B8092A for ; Sat, 22 May 2021 09:18:07 +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 smtp2.mailbox.org (smtp2.mailbox.org [IPv6:2001:67c:2050:105:465:1:2:0]) (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 4FnFDv0llFzQk1P; Sat, 22 May 2021 09:18:07 +0200 (CEST) Received: from smtp2.mailbox.org ([80.241.60.241]) by hefe.heinlein-support.de (hefe.heinlein-support.de [91.198.250.172]) (amavisd-new, port 10030) with ESMTP id Dil7umDm4icy; Sat, 22 May 2021 09:18:03 +0200 (CEST) Subject: Re: [PATCH 1/2] watchdog: add gpio watchdog driver From: Stefan Roese To: Rasmus Villemoes , u-boot@lists.denx.de Cc: Simon Glass , Tom Rini References: <20210510154738.3385393-1-rasmus.villemoes@prevas.dk> <6538a9d2-0747-f320-9844-c79238743d63@denx.de> <69be9b42-042f-8461-c581-62f9da1157da@prevas.dk> <07d6797b-5d5d-51ee-0546-3e21cf7ce6fc@denx.de> Message-ID: <5232e901-ad98-941c-b182-159f62998325@denx.de> Date: Sat, 22 May 2021 09:18:02 +0200 MIME-Version: 1.0 In-Reply-To: <07d6797b-5d5d-51ee-0546-3e21cf7ce6fc@denx.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: de-DE Content-Transfer-Encoding: 8bit X-MBO-SPAM-Probability: X-Rspamd-Score: -5.86 / 15.00 / 15.00 X-Rspamd-Queue-Id: C033D3B X-Rspamd-UID: 6675ee X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 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.102.4 at phobos.denx.de X-Virus-Status: Clean On 11.05.21 09:20, Stefan Roese wrote: > On 11.05.21 08:40, Rasmus Villemoes wrote: >> On 11/05/2021 07.55, Stefan Roese wrote: >>> On 10.05.21 17:47, Rasmus Villemoes wrote: >>>> A rather common kind of external watchdog circuit is one that is kept >>>> alive by toggling a gpio. Add a driver for handling such a watchdog. >>>> >>>> The compatible string is probably a little odd as it has nothing to do >>>> with linux per se - however, I chose that to make .dts snippets >>>> reusable between device trees used with U-Boot and linux, and this is >>>> the (only) compatible string that linux' corresponding driver and DT >>>> binding accepts. I have asked whether one should/could add "wdt-gpio" >>>> to that binding, but the answer was no: >>>> >>>> https://lore.kernel.org/lkml/CAL_JsqKEGaFpiFV_oAtE+S_bnHkg4qry+bhx2EDs=NSbVf_giA@mail.gmail.com/ >>>> >>>> >>>> >>>> If someone feels strongly about this, I can certainly remove the >>>> "linux," part from the string - it probably wouldn't the only place >>>> where >>>> one can't reuse a DT snippet as-is between linux and U-Boot. >>>> >>>> Signed-off-by: Rasmus Villemoes >>>> --- >>>>    .../watchdog/gpio-wdt.txt                     | 15 ++++++ >>>>    drivers/watchdog/Kconfig                      |  7 +++ >>>>    drivers/watchdog/Makefile                     |  1 + >>>>    drivers/watchdog/gpio_wdt.c                   | 51 >>>> +++++++++++++++++++ >>>>    4 files changed, 74 insertions(+) >>>>    create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt >>>>    create mode 100644 drivers/watchdog/gpio_wdt.c >>>> >>>> diff --git a/doc/device-tree-bindings/watchdog/gpio-wdt.txt >>>> b/doc/device-tree-bindings/watchdog/gpio-wdt.txt >>>> new file mode 100644 >>>> index 0000000000..2283b7ba6e >>>> --- /dev/null >>>> +++ b/doc/device-tree-bindings/watchdog/gpio-wdt.txt >>>> @@ -0,0 +1,15 @@ >>>> +GPIO watchdog timer >>>> + >>>> +Describes a simple watchdog timer which is reset by toggling a gpio. >>>> + >>>> +Required properties: >>>> + >>>> +- compatible: must be "linux,wdt-gpio" >>>> +- gpios: gpio to toggle when wdt driver reset method is called >>>> + >>>> +Example: >>>> + >>>> +    gpio-wdt { >>>> +        gpios = <&gpio0 1 0>; >>>> +        compatible = "linux,wdt-gpio"; >>>> +    }; >>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>>> index f0ff2612a6..2cf378db29 100644 >>>> --- a/drivers/watchdog/Kconfig >>>> +++ b/drivers/watchdog/Kconfig >>>> @@ -147,6 +147,13 @@ config WDT_CORTINA >>>>          This driver support all CPU ISAs supported by Cortina >>>>          Access CAxxxx SoCs. >>>>    +config WDT_GPIO >>>> +    bool "External gpio watchdog support" >>>> +    depends on WDT >>>> +    depends on DM_GPIO >>>> +    help >>>> +       Support for external watchdog fed by toggling a gpio. >>>> + >>>>    config WDT_MPC8xx >>>>        bool "MPC8xx watchdog timer support" >>>>        depends on WDT && MPC8xx >>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >>>> index 5c7ef593fe..f14415bb8e 100644 >>>> --- a/drivers/watchdog/Makefile >>>> +++ b/drivers/watchdog/Makefile >>>> @@ -25,6 +25,7 @@ obj-$(CONFIG_WDT_BOOKE) += booke_wdt.o >>>>    obj-$(CONFIG_WDT_CORTINA) += cortina_wdt.o >>>>    obj-$(CONFIG_WDT_ORION) += orion_wdt.o >>>>    obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o >>>> +obj-$(CONFIG_WDT_GPIO) += gpio_wdt.o >>>>    obj-$(CONFIG_WDT_MPC8xx) += mpc8xx_wdt.o >>>>    obj-$(CONFIG_WDT_MT7620) += mt7620_wdt.o >>>>    obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o >>>> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c >>>> new file mode 100644 >>>> index 0000000000..9dba9c254e >>>> --- /dev/null >>>> +++ b/drivers/watchdog/gpio_wdt.c >>>> @@ -0,0 +1,51 @@ >>>> +// SPDX-License-Identifier: GPL-2.0+ >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +struct gpio_wdt_priv { >>>> +    struct gpio_desc gpio; >>>> +    int state; >>>> +}; >>>> + >>>> +static int gpio_wdt_reset(struct udevice *dev) >>>> +{ >>>> +    struct gpio_wdt_priv *priv = dev_get_priv(dev); >>>> + >>>> +    priv->state = !priv->state; >>>> +    return dm_gpio_set_value(&priv->gpio, priv->state); >>>> +} >>>> + >>>> +static int dm_probe(struct udevice *dev) >>>> +{ >>>> +    struct gpio_wdt_priv *priv = dev_get_priv(dev); >>>> +    int ret; >>>> + >>>> +    ret = gpio_request_by_name(dev, "gpios", 0, &priv->gpio, >>>> GPIOD_IS_OUT); >>>> +    if (ret < 0) { >>>> +        dev_err(dev, "Request for wdt gpio failed: %d\n", ret); >>>> +        return ret; >>>> +    } >>>> +    return gpio_wdt_reset(dev); >>>> +} >>>> + >>>> +static const struct wdt_ops gpio_wdt_ops = { >>>> +    .reset = gpio_wdt_reset, >>>> +}; >>> >>> I'm noticing that you don't have a "start" function. How is this GPIO >>> watchdog enabled? Is it "always on"? Or does it start with the first >>> triggering? >>> >> >> I have yet to encounter one of these that isn't always-running - it's >> more or less the only reason one would add an external watchdog circuit >> instead of relying on whatever the SOC has. > > Okay. > >>> Perhaps it makes sense to add a note about this into the driver to make >>> this clear. >> >> Will do. But I now see a much bigger problem due to some refactoring in >> wdt-uclass that basically requires a ->start method or GD_FLG_WDT_READY >> won't be set. Sigh. I'm looking through the pending watchdog patches right now. Do you plan to send an updated version of this patchset soon (mostly updates to documentation)? Thanks, Stefan