Re: [PATCH v4 5/6] mips: ls1c300: add watchdog support

On 7/23/23 06:39, Du Huanpeng wrote:

Signed-off-by: Du Huanpeng dhu@hodcarrier.org

A small description is always helpful (mandatory) in the commit text. Please add at least one or two sentences here.

---

drivers/watchdog/Kconfig | 8 +++ drivers/watchdog/Makefile | 1 + drivers/watchdog/lsmips_wdt.c | 128 ++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 drivers/watchdog/lsmips_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 646663528a..f60011e5b3 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -171,6 +171,14 @@ config WDT_GPIO doc/device-tree-bindings/watchdog/gpio-wdt.txt for information on how to describe the watchdog in device tree.

+config WDT_LSMIPS

• bool "Loongson MIPS watchdog timer support"
• depends on WDT
• help

•  Select this to enable watchdog timer for Loongson SoCs.
  

•  The watchdog timer is stopped when initialized.
  

•  It performs full SoC reset.
  

• config WDT_MAX6370 bool "MAX6370 watchdog timer support" depends on WDT

diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index fd5d9c7376..8afd61a4fc 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_WDT_ORION) += orion_wdt.o obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o obj-$(CONFIG_WDT_FTWDT010) += ftwdt010_wdt.o obj-$(CONFIG_WDT_GPIO) += gpio_wdt.o +obj-$(CONFIG_WDT_LSMIPS) += lsmips_wdt.o obj-$(CONFIG_WDT_MAX6370) += max6370_wdt.o obj-$(CONFIG_WDT_MESON_GXBB) += meson_gxbb_wdt.o obj-$(CONFIG_WDT_MPC8xxx) += mpc8xxx_wdt.o diff --git a/drivers/watchdog/lsmips_wdt.c b/drivers/watchdog/lsmips_wdt.c new file mode 100644 index 0000000000..69006d332d --- /dev/null +++ b/drivers/watchdog/lsmips_wdt.c @@ -0,0 +1,128 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Watchdog driver for MediaTek SoCs
• • Copyright (C) 2018 MediaTek Inc.
• • Author: Ryder Lee ryder.lee@mediatek.com
• • based on: drivers/watchdog/mtk_wdt.c
• • Copyright (C) 2020-2023 Du Huanpeng dhu@hodcarrier.org
• */

+#include <common.h> +#include <dm.h> +#include <hang.h> +#include <wdt.h> +#include <asm/io.h> +#include <clk.h>

+struct lsmips_wdt_priv {

• void __iomem *base;

+#define WDT_EN 0 +#define WDT_TIMER 4 +#define WDT_SET 8

• ulong clock;
• unsigned long timeout;

+};

+static int lsmips_wdt_reset(struct udevice *dev) +{

• struct lsmips_wdt_priv *priv = dev_get_priv(dev);
• writel(priv->timeout, priv->base + WDT_TIMER);
• writel(1, priv->base + WDT_SET);
• return 0;

+}

+static int lsmips_wdt_stop(struct udevice *dev) +{

• struct lsmips_wdt_priv *priv = dev_get_priv(dev);
• writel(0, priv->base + WDT_EN);
• return 0;

+}

+static int lsmips_wdt_expire_now(struct udevice *dev, ulong flags) +{

• struct lsmips_wdt_priv *priv = dev_get_priv(dev);
• writel(1, priv->base + WDT_EN);
• writel(1, priv->base + WDT_TIMER);
• writel(1, priv->base + WDT_SET);
• hang();
• return 0;

+}

+static int lsmips_wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) +{

• struct lsmips_wdt_priv *priv = dev_get_priv(dev);
• unsigned int timeout;
• const unsigned int MSEC_PER_SEC = 1000;

Upper case variable names are very unusual. Why not use a macro / define instead?

Nitpicking:

Reverse X-mas tree ordering of the variable declarations is preferred.

• timeout = U32_MAX / (priv->clock / MSEC_PER_SEC);
• if (timeout < timeout_ms)

• timeout = U32_MAX;
  

• else

• timeout = timeout_ms * (priv->clock / MSEC_PER_SEC);
  

• debug("WDT: reload = %08x\n", timeout);
• writel(1, priv->base + WDT_EN);
• writel(timeout, priv->base + WDT_TIMER);
• writel(1, priv->base + WDT_SET);
• priv->timeout = timeout;
• return 0;

+}

+static int lsmips_wdt_probe(struct udevice *dev) +{

• struct lsmips_wdt_priv *priv = dev_get_priv(dev);
• struct clk cl;
• ulong clock;
• priv->base = dev_remap_addr(dev);
• if (!priv->base)

• return -ENOENT;
  

• if (clk_get_by_index(dev, 0, &cl) == 0)

• clock = clk_get_rate(&cl);
  

• debug("WDT: clock = %ld\n", clock);
• if (IS_ERR_VALUE(clock)) {

• dev_err(dev, "failed to get rate\n");
  

• return clock;
  

• }
• priv->clock = clock;
• writel(0, priv->base + WDT_EN);
• return 0;

+}

+static const struct wdt_ops lsmips_wdt_ops = {

• .start = lsmips_wdt_start,
• .reset = lsmips_wdt_reset,
• .stop = lsmips_wdt_stop,
• .expire_now = lsmips_wdt_expire_now,

+};

+static const struct udevice_id lsmips_wdt_ids[] = {

• { .compatible = "loongson,ls1c300-wdt"},

I was not able to find this compatible property in the Linux source tree. Is this WDT not supported there?

Thanks, Stefan