[v3,2/2] drivers: watchdog: Add StarFive Watchdog driver

Message ID	20230220081926.267695-3-xingyu.wu@starfivetech.com (mailing list archive)
State	Changes Requested
Headers	show Return-Path: <linux-watchdog-owner@vger.kernel.org> From: Xingyu Wu <xingyu.wu@starfivetech.com> To: <linux-riscv@lists.infradead.org>, <devicetree@vger.kernel.org>, <linux-watchdog@vger.kernel.org>, Wim Van Sebroeck <wim@linux-watchdog.org>, Guenter Roeck <linux@roeck-us.net>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> CC: Rob Herring <robh+dt@kernel.org>, Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, Philipp Zabel <p.zabel@pengutronix.de>, Xingyu Wu <xingyu.wu@starfivetech.com>, Samin Guo <samin.guo@starfivetech.com>, <linux-kernel@vger.kernel.org>, Conor Dooley <conor@kernel.org> Subject: [PATCH v3 2/2] drivers: watchdog: Add StarFive Watchdog driver Date: Mon, 20 Feb 2023 16:19:26 +0800 Message-ID: <20230220081926.267695-3-xingyu.wu@starfivetech.com> In-Reply-To: <20230220081926.267695-1-xingyu.wu@starfivetech.com> References: <20230220081926.267695-1-xingyu.wu@starfivetech.com> MIME-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 8BIT Precedence: bulk
Series	Add watchdog driver for StarFive JH7110 RISC-V SoC \| expand [v3,0/2] Add watchdog driver for StarFive JH7110 RISC-V SoC [v3,1/2] dt-bindings: watchdog: Add watchdog for StarFive JH7110 [v3,2/2] drivers: watchdog: Add StarFive Watchdog driver

Xingyu Wu Feb. 20, 2023, 8:19 a.m. UTC

Add watchdog driver for the StarFive JH7110 SoC.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 MAINTAINERS                     |   7 +
 drivers/watchdog/Kconfig        |   9 +
 drivers/watchdog/Makefile       |   2 +
 drivers/watchdog/starfive-wdt.c | 651 ++++++++++++++++++++++++++++++++
 4 files changed, 669 insertions(+)
 create mode 100644 drivers/watchdog/starfive-wdt.c

Guenter Roeck Feb. 23, 2023, 6:23 p.m. UTC | #1

On Mon, Feb 20, 2023 at 04:19:26PM +0800, Xingyu Wu wrote:
> Add watchdog driver for the StarFive JH7110 SoC.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  MAINTAINERS                     |   7 +
>  drivers/watchdog/Kconfig        |   9 +
>  drivers/watchdog/Makefile       |   2 +
>  drivers/watchdog/starfive-wdt.c | 651 ++++++++++++++++++++++++++++++++
>  4 files changed, 669 insertions(+)
>  create mode 100644 drivers/watchdog/starfive-wdt.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 135d93368d36..6cbcf08fa76a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19933,6 +19933,13 @@ F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>  F:	drivers/reset/reset-starfive-jh7100.c
>  F:	include/dt-bindings/reset/starfive-jh7100.h
>  
> +STARFIVE JH7110 WATCHDOG DRIVER
> +M:	Xingyu Wu <xingyu.wu@starfivetech.com>
> +M:	Samin Guo <samin.guo@starfivetech.com>
> +S:	Supported
> +F:	Documentation/devicetree/bindings/watchdog/starfive*
> +F:	drivers/watchdog/starfive-wdt.c
> +
>  STATIC BRANCH/CALL
>  M:	Peter Zijlstra <peterz@infradead.org>
>  M:	Josh Poimboeuf <jpoimboe@kernel.org>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0bc40b763b06..4608eb5c9501 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2089,6 +2089,15 @@ config UML_WATCHDOG
>  	tristate "UML watchdog"
>  	depends on UML || COMPILE_TEST
>  
> +config STARFIVE_WATCHDOG
> +	tristate "StarFive Watchdog support"
> +	depends on RISCV
> +	select WATCHDOG_CORE
> +	default SOC_STARFIVE
> +	help
> +	  Say Y here to support the watchdog of StarFive JH7110 SoC.
> +	  This driver can also be built as a module if choose M.
> +
>  #
>  # ISA-based Watchdog Cards
>  #
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 9cbf6580f16c..4c0bd377e92a 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -211,6 +211,8 @@ obj-$(CONFIG_WATCHDOG_SUN4V)		+= sun4v_wdt.o
>  # Xen
>  obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>  
> +obj-$(CONFIG_STARFIVE_WATCHDOG) += starfive-wdt.o
> +
>  # Architecture Independent
>  obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
>  obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> new file mode 100644
> index 000000000000..dfbb80406076
> --- /dev/null
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -0,0 +1,651 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Starfive Watchdog driver
> + *
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/watchdog.h>
> +
> +/* JH7110 WatchDog register define */
> +#define STARFIVE_WDT_JH7110_LOAD	0x000	/* RW: Watchdog load register */
> +#define STARFIVE_WDT_JH7110_VALUE	0x004	/* RO: The current value for the watchdog counter */
> +#define STARFIVE_WDT_JH7110_CONTROL	0x008	/*
> +						 * RW:
> +						 * [0]: reset enable;
> +						 * [1]: int enable/wdt enable/reload counter;
> +						 * [31:2]: reserve.

reserved

> +						 */
> +#define STARFIVE_WDT_JH7110_INTCLR	0x00c	/* WO: clear intterupt && reload the counter */
> +#define STARFIVE_WDT_JH7110_RIS		0x010	/* RO: Raw interrupt status from the counter */
> +#define STARFIVE_WDT_JH7110_IMS		0x014	/* RO: Enabled interrupt status from the counter */
> +#define STARFIVE_WDT_JH7110_LOCK	0xc00	/*
> +						 * RO: Enable write access to all other registers
> +						 * by writing 0x1ACCE551.
> +						 */

"RO" contradicts itself against the rest of the comment. The register is
written to, so it can't bew RO.

> +
> +/* WDOGCONTROL */
> +#define STARFIVE_WDT_ENABLE			0x1
> +#define STARFIVE_WDT_JH7110_EN_SHIFT		0
> +#define STARFIVE_WDT_RESET_EN			0x1
> +#define STARFIVE_WDT_JH7110_RESEN_SHIFT		1
> +
> +/* WDOGLOCK */
> +#define STARFIVE_WDT_LOCKED			BIT(0)
> +#define STARFIVE_WDT_JH7110_UNLOCK_KEY		0x1acce551
> +
> +/* WDOGINTCLR */
> +#define STARFIVE_WDT_INTCLR			0x1
> +
> +#define STARFIVE_WDT_MAXCNT			0xffffffff
> +#define STARFIVE_WDT_DEFAULT_TIME		(15)
> +#define STARFIVE_WDT_DELAY_US			0
> +#define STARFIVE_WDT_TIMEOUT_US			10000
> +
> +/* module parameter */
> +#define STARFIVE_WDT_EARLY_ENA			0
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +static int heartbeat;
> +static int early_enable = STARFIVE_WDT_EARLY_ENA;
> +
> +module_param(heartbeat, int, 0);
> +module_param(early_enable, int, 0);
> +module_param(nowayout, bool, 0);
> +
> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeat in seconds. (default="
> +		 __MODULE_STRING(STARFIVE_WDT_DEFAULT_TIME) ")");
> +MODULE_PARM_DESC(early_enable,
> +		 "Watchdog is started at boot time if set to 1, default="
> +		 __MODULE_STRING(STARFIVE_WDT_EARLY_ENA));
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct starfive_wdt_variant {
> +	u32 control;
> +	u32 load;
> +	u32 enable;
> +	u32 value;
> +	u32 int_clr;
> +	u32 unlock;
> +	u32 unlock_key;
> +	u32 irq_is_raise;
> +	u8 enrst_shift;
> +	u8 en_shift;
> +};
> +
> +struct starfive_wdt {
> +	unsigned long freq;
> +	struct device *dev;
> +	struct watchdog_device wdt_device;
> +	struct clk *core_clk;
> +	struct clk *apb_clk;
> +	struct reset_control *rsts;
> +	const struct starfive_wdt_variant *drv_data;
> +	u32 count;	/*count of timeout*/
> +	u32 reload;	/*restore the count*/
> +	void __iomem *base;
> +	spinlock_t lock;	/* spinlock for register handling */
> +};
> +
> +/* Register bias in JH7110 */
> +static const struct starfive_wdt_variant drv_data_jh7110 = {
> +	.control = STARFIVE_WDT_JH7110_CONTROL,
> +	.load = STARFIVE_WDT_JH7110_LOAD,
> +	.enable = STARFIVE_WDT_JH7110_CONTROL,
> +	.value = STARFIVE_WDT_JH7110_VALUE,
> +	.int_clr = STARFIVE_WDT_JH7110_INTCLR,
> +	.unlock = STARFIVE_WDT_JH7110_LOCK,
> +	.unlock_key = STARFIVE_WDT_JH7110_UNLOCK_KEY,
> +	.irq_is_raise = STARFIVE_WDT_JH7110_IMS,
> +	.enrst_shift = STARFIVE_WDT_JH7110_RESEN_SHIFT,
> +	.en_shift = STARFIVE_WDT_JH7110_EN_SHIFT,
> +};
> +
> +static const struct of_device_id starfive_wdt_match[] = {
> +	{ .compatible = "starfive,jh7110-wdt", .data = &drv_data_jh7110 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, starfive_wdt_match);
> +
> +static const struct platform_device_id starfive_wdt_ids[] = {
> +	{
> +		.name = "starfive-jh7110-wdt",
> +		.driver_data = (unsigned long)&drv_data_jh7110,
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(platform, starfive_wdt_ids);
> +
> +static int starfive_wdt_get_clock_rate(struct starfive_wdt *wdt)
> +{
> +	wdt->freq = clk_get_rate(wdt->core_clk);
> +	/* The clock rate should not be 0.*/

This is obvious and not worth a comment.

> +	if (wdt->freq)
> +		return 0;
> +
> +	dev_err(wdt->dev, "get clock rate failed.\n");
> +	return -ENOENT;

Error handling should come first

	if (!wdt->freq) {
		dev_err(wdt->dev, "get clock rate failed.\n");
		return -EINVAL;
	}
	return 0;

This is not "No such file or directory" (yes, I see that other drivers
do that, but that doesn't make it better). Use -EINVAL instead.

> +}
> +
> +static int starfive_wdt_get_clock(struct starfive_wdt *wdt)
> +{
> +	wdt->apb_clk = devm_clk_get(wdt->dev, "apb");
> +	if (IS_ERR(wdt->apb_clk)) {
> +		dev_err(wdt->dev, "failed to get apb clock.\n");
> +		return PTR_ERR(wdt->apb_clk);
> +	}
> +
> +	wdt->core_clk = devm_clk_get(wdt->dev, "core");
> +	if (IS_ERR(wdt->core_clk)) {
> +		dev_err(wdt->dev, "failed to get core clock.\n");
> +		return PTR_ERR(wdt->core_clk);
> +	}
> +
> +	return 0;
> +}
> +
> +static int starfive_wdt_reset_init(struct starfive_wdt *wdt)
> +{
> +	int ret = 0;
> +
> +	wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
> +	if (IS_ERR(wdt->rsts)) {
> +		dev_err(wdt->dev, "failed to get rsts error.\n");

I don't think this message is understandable. "failed to get reset
control", maybe. Also, the call may return -EPROBE_DEFER,
which should not generate an error message. Either drop the message
or use dev_err_probe().

> +		ret = PTR_ERR(wdt->rsts);
> +	} else {
> +		ret = reset_control_deassert(wdt->rsts);
> +		if (ret)
> +			dev_err(wdt->dev, "failed to deassert rsts.\n");
> +	}
> +
> +	return ret;
> +}
> +
> +static u32 starfive_wdt_ticks_to_sec(struct starfive_wdt *wdt, u32 ticks)
> +{
> +	return DIV_ROUND_CLOSEST(ticks, wdt->freq);
> +}
> +
> +/*
> + * Write unlock-key to unlock. Write other value to lock. When lock bit is 1,
> + * external accesses to other watchdog registers are ignored.
> + */
> +static bool starfive_wdt_is_locked(struct starfive_wdt *wdt)
> +{
> +	u32 val;
> +
> +	val = readl(wdt->base + wdt->drv_data->unlock);
> +	return !!(val & STARFIVE_WDT_LOCKED);
> +}
> +
> +static void starfive_wdt_unlock(struct starfive_wdt *wdt)
> +{
> +	if (starfive_wdt_is_locked(wdt))
> +		writel(wdt->drv_data->unlock_key,
> +		       wdt->base + wdt->drv_data->unlock);
> +}
> +
> +static void starfive_wdt_lock(struct starfive_wdt *wdt)
> +{
> +	if (!starfive_wdt_is_locked(wdt))
> +		writel(~wdt->drv_data->unlock_key,
> +		       wdt->base + wdt->drv_data->unlock);
> +}
> +
> +/* enable watchdog interrupt to reset/reboot */
> +static void starfive_wdt_enable_reset(struct starfive_wdt *wdt)
> +{
> +	u32 val;
> +
> +	val = readl(wdt->base + wdt->drv_data->control);
> +	val |= STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift;
> +	writel(val, wdt->base + wdt->drv_data->control);
> +}
> +
> +/* disable watchdog interrupt to reset/reboot */
> +static void starfive_wdt_disable_reset(struct starfive_wdt *wdt)
> +{
> +	u32 val;
> +
> +	val = readl(wdt->base + wdt->drv_data->control);
> +	val &= ~(STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift);
> +	writel(val, wdt->base + wdt->drv_data->control);
> +}
> +
> +/* interrupt status whether has been raised from the counter */
> +static bool starfive_wdt_raise_irq_status(struct starfive_wdt *wdt)
> +{
> +	return !!readl(wdt->base + wdt->drv_data->irq_is_raise);
> +}
> +
> +/* clear interrupt signal before initialization or reload */
> +static void starfive_wdt_int_clr(struct starfive_wdt *wdt)
> +{
> +	writel(STARFIVE_WDT_INTCLR, wdt->base + wdt->drv_data->int_clr);
> +}
> +
> +static inline void starfive_wdt_set_count(struct starfive_wdt *wdt, u32 val)
> +{
> +	writel(val, wdt->base + wdt->drv_data->load);
> +}
> +
> +static inline u32 starfive_wdt_get_count(struct starfive_wdt *wdt)
> +{
> +	return readl(wdt->base + wdt->drv_data->value);
> +}
> +
> +/* enable watchdog */
> +static inline void starfive_wdt_enable(struct starfive_wdt *wdt)
> +{
> +	u32 val;
> +
> +	val = readl(wdt->base + wdt->drv_data->enable);
> +	val |= STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift;
> +	writel(val, wdt->base + wdt->drv_data->enable);
> +}
> +
> +/* disable watchdog */
> +static inline void starfive_wdt_disable(struct starfive_wdt *wdt)
> +{
> +	u32 val;
> +
> +	val = readl(wdt->base + wdt->drv_data->enable);
> +	val &= ~(STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift);
> +	writel(val, wdt->base + wdt->drv_data->enable);
> +}
> +
> +static inline void starfive_wdt_set_reload_count(struct starfive_wdt *wdt, u32 count)
> +{
> +	starfive_wdt_set_count(wdt, count);
> +	/* need enable controller to reload counter */
> +	starfive_wdt_enable(wdt);
> +}
> +
> +static unsigned int starfive_wdt_max_timeout(struct starfive_wdt *wdt)
> +{
> +	return DIV_ROUND_UP(STARFIVE_WDT_MAXCNT, (wdt->freq / 2)) - 1;
> +}
> +
> +static unsigned int starfive_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +	u32 count;
> +
> +	starfive_wdt_unlock(wdt);
> +	/*
> +	 * Because set half count value,
> +	 * timeleft value should add the count value before first timeout.
> +	 */
> +	count = starfive_wdt_get_count(wdt);
> +	if (!starfive_wdt_raise_irq_status(wdt))
> +		count += wdt->count;
> +
> +	starfive_wdt_lock(wdt);
> +
> +	return starfive_wdt_ticks_to_sec(wdt, count);
> +}
> +
> +static int starfive_wdt_keepalive(struct watchdog_device *wdd)
> +{
> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	spin_lock(&wdt->lock);
> +
> +	starfive_wdt_unlock(wdt);
> +	starfive_wdt_int_clr(wdt);
> +	starfive_wdt_set_reload_count(wdt, wdt->count);
> +	starfive_wdt_lock(wdt);
> +
> +	spin_unlock(&wdt->lock);
> +
> +	return 0;
> +}
> +
> +static int starfive_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	spin_lock(&wdt->lock);
> +
> +	starfive_wdt_unlock(wdt);
> +	starfive_wdt_disable_reset(wdt);

Is that necessary ? The watchdog is going to be stopped, after all.

> +	starfive_wdt_int_clr(wdt);
> +	starfive_wdt_disable(wdt);
> +	starfive_wdt_lock(wdt);
> +
> +	spin_unlock(&wdt->lock);
> +
> +	return 0;
> +}
> +
> +static int starfive_wdt_pm_stop(struct watchdog_device *wdd)
> +{
> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	starfive_wdt_stop(wdd);
> +	pm_runtime_put_sync(wdt->dev);
> +
> +	return 0;
> +}
> +
> +static int starfive_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	spin_lock(&wdt->lock);
> +	starfive_wdt_unlock(wdt);
> +	/* disable watchdog, to be safe */
> +	starfive_wdt_disable(wdt);
> +
> +	starfive_wdt_enable_reset(wdt);
> +	starfive_wdt_int_clr(wdt);
> +	starfive_wdt_set_count(wdt, wdt->count);
> +	starfive_wdt_enable(wdt);
> +
> +	starfive_wdt_lock(wdt);
> +	spin_unlock(&wdt->lock);
> +
> +	return 0;
> +}
> +
> +static int starfive_wdt_pm_start(struct watchdog_device *wdd)
> +{
> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	pm_runtime_get_sync(wdt->dev);
> +
> +	return starfive_wdt_start(wdd);
> +}
> +
> +static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
> +				    unsigned int timeout)
> +{
> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +	unsigned long freq = wdt->freq;
> +
> +	spin_lock(&wdt->lock);
> +
> +	/*
> +	 * This watchdog takes twice timeouts to reset.
> +	 * In order to reduce time to reset, should set half count value.
> +	 */
> +	wdt->count = timeout * freq / 2;
> +	wdd->timeout = timeout;
> +
> +	starfive_wdt_unlock(wdt);
> +	starfive_wdt_disable(wdt);
> +	starfive_wdt_set_reload_count(wdt, wdt->count);
> +	starfive_wdt_enable(wdt);
> +	starfive_wdt_lock(wdt);
> +
> +	spin_unlock(&wdt->lock);
> +
> +	return 0;
> +}
> +
> +#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
> +
> +static const struct watchdog_info starfive_wdt_ident = {
> +	.options = OPTIONS,
> +	.identity = "StarFive Watchdog",
> +};
> +
> +static const struct watchdog_ops starfive_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = starfive_wdt_pm_start,
> +	.stop = starfive_wdt_pm_stop,
> +	.ping = starfive_wdt_keepalive,
> +	.set_timeout = starfive_wdt_set_timeout,
> +	.get_timeleft = starfive_wdt_get_timeleft,
> +};
> +
> +static const struct watchdog_device starfive_wdd = {
> +	.info = &starfive_wdt_ident,
> +	.ops = &starfive_wdt_ops,
> +	.timeout = STARFIVE_WDT_DEFAULT_TIME,
> +};
> +
> +static inline const struct starfive_wdt_variant *
> +starfive_wdt_get_drv_data(struct platform_device *pdev)
> +{
> +	const struct starfive_wdt_variant *variant;
> +
> +	variant = of_device_get_match_data(&pdev->dev);
> +	if (!variant) {
> +		/* Device matched by platform_device_id */
> +		variant = (struct starfive_wdt_variant *)
> +			   platform_get_device_id(pdev)->driver_data;
> +	}
> +
> +	return variant;
> +}
> +
> +static int starfive_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct starfive_wdt *wdt;
> +	int ret;
> +
> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	wdt->dev = dev;
> +	spin_lock_init(&wdt->lock);
> +	wdt->wdt_device = starfive_wdd;
> +
> +	wdt->drv_data = starfive_wdt_get_drv_data(pdev);
> +
> +	/* get the memory region for the watchdog timer */
> +	wdt->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(wdt->base)) {
> +		ret = PTR_ERR(wdt->base);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +	pm_runtime_enable(wdt->dev);
> +
> +	ret = starfive_wdt_get_clock(wdt);
> +	if (ret)
> +		return ret;
> +
> +	if (pm_runtime_enabled(wdt->dev)) {
> +		ret = pm_runtime_get_sync(wdt->dev);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		/* runtime PM is disabled but clocks need to be enabled */
> +		ret = clk_prepare_enable(wdt->apb_clk);
> +		if (ret) {
> +			dev_err(wdt->dev, "failed to enable apb_clk.\n");
> +			return ret;
> +		}
> +		ret = clk_prepare_enable(wdt->core_clk);
> +		if (ret) {
> +			dev_err(wdt->dev, "failed to enable core_clk.\n");
> +			goto err_apb_clk_disable;
> +		}
> +	}
> +
> +	ret = starfive_wdt_get_clock_rate(wdt);
> +	if (ret)
> +		goto err_clk_disable;
> +
> +	ret = starfive_wdt_reset_init(wdt);
> +	if (ret)
> +		goto err_clk_disable;
> +
> +	wdt->wdt_device.min_timeout = 1;
> +	wdt->wdt_device.max_timeout = starfive_wdt_max_timeout(wdt);

	wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;

should be set here. Otherwise the warning below would always be seen
if the module parameter is not set.

> +
> +	watchdog_set_drvdata(&wdt->wdt_device, wdt);
> +
> +	/*
> +	 * see if we can actually set the requested heartbeat,
> +	 * and if not, try the default value.
> +	 */
> +	watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
> +	if (wdt->wdt_device.timeout == 0 ||

If wdt->wdt_device.timeout is pre-initialized, it will never be 0 here.

> +	    wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {

That won't happen because watchdog_init_timeout() validates it and does
not update the value if it is out of range.

> +		dev_warn(dev, "heartbeat value out of range, default %d used\n",
> +			 STARFIVE_WDT_DEFAULT_TIME);
> +		wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;

And this is then unnecessary. wdt->wdt_device.timeout will always be
valid if it was pre-initialized.

> +	}
> +	starfive_wdt_set_timeout(&wdt->wdt_device, wdt->wdt_device.timeout);
> +
> +	watchdog_set_nowayout(&wdt->wdt_device, nowayout);
> +	watchdog_stop_on_reboot(&wdt->wdt_device);
> +	watchdog_stop_on_unregister(&wdt->wdt_device);
> +
> +	wdt->wdt_device.parent = dev;
> +
> +	ret = watchdog_register_device(&wdt->wdt_device);
> +	if (ret)
> +		goto err_clk_disable;
> +
> +	if (early_enable) {
> +		starfive_wdt_start(&wdt->wdt_device);
> +		set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);

Is it correct to call pm_runtime_put_sync() below if the watchdog
is running ? Doesn't that stop the clock ?

> +	} else {
> +		starfive_wdt_stop(&wdt->wdt_device);
> +	}

Wrong order. This has to be done _before_ the watchdog device is registered
to make sure that the watchdog core knows about WDOG_HW_RUNNING.

> +
> +	pm_runtime_put_sync(wdt->dev);
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(wdt->core_clk);
> +err_apb_clk_disable:
> +	clk_disable_unprepare(wdt->apb_clk);
> +	pm_runtime_disable(wdt->dev);
> +
> +	return ret;
> +}
> +
> +static int starfive_wdt_remove(struct platform_device *dev)
> +{
> +	struct starfive_wdt *wdt = platform_get_drvdata(dev);
> +
> +	starfive_wdt_stop(&wdt->wdt_device);
> +	watchdog_unregister_device(&wdt->wdt_device);
> +
> +	if (pm_runtime_enabled(wdt->dev)) {
> +		pm_runtime_disable(wdt->dev);
> +	} else {
> +		/* disable clock without PM */
> +		clk_disable_unprepare(wdt->core_clk);
> +		clk_disable_unprepare(wdt->apb_clk);
> +	}
> +
> +	return 0;
> +}
> +
> +static void starfive_wdt_shutdown(struct platform_device *dev)
> +{
> +	struct starfive_wdt *wdt = platform_get_drvdata(dev);
> +
> +	starfive_wdt_pm_stop(&wdt->wdt_device);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int starfive_wdt_suspend(struct device *dev)
> +{
> +	int ret;
> +	struct starfive_wdt *wdt = dev_get_drvdata(dev);
> +
> +	starfive_wdt_unlock(wdt);
> +
> +	/* Save watchdog state, and turn it off. */
> +	wdt->reload = starfive_wdt_get_count(wdt);
> +
> +	/* Note that WTCNT doesn't need to be saved. */
> +	starfive_wdt_stop(&wdt->wdt_device);
> +	pm_runtime_force_suspend(dev);
> +
> +	starfive_wdt_lock(wdt);
> +
> +	return 0;
> +}
> +
> +static int starfive_wdt_resume(struct device *dev)
> +{
> +	int ret;
> +	struct starfive_wdt *wdt = dev_get_drvdata(dev);
> +
> +	starfive_wdt_unlock(wdt);
> +
> +	pm_runtime_force_resume(dev);
> +
> +	/* Restore watchdog state. */
> +	starfive_wdt_set_reload_count(wdt, wdt->reload);
> +
> +	starfive_wdt_start(&wdt->wdt_device);
> +
> +	starfive_wdt_lock(wdt);
> +
> +	return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +#ifdef CONFIG_PM
> +static int starfive_wdt_runtime_suspend(struct device *dev)
> +{
> +	struct starfive_wdt *wdt = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(wdt->apb_clk);
> +	clk_disable_unprepare(wdt->core_clk);
> +
> +	return 0;
> +}
> +
> +static int starfive_wdt_runtime_resume(struct device *dev)
> +{
> +	struct starfive_wdt *wdt = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(wdt->apb_clk);
> +	if (ret) {
> +		dev_err(wdt->dev, "failed to enable apb_clk.\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(wdt->core_clk);
> +	if (ret)
> +		dev_err(wdt->dev, "failed to enable core_clk.\n");
> +
> +	return ret;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops starfive_wdt_pm_ops = {
> +	SET_RUNTIME_PM_OPS(starfive_wdt_runtime_suspend, starfive_wdt_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(starfive_wdt_suspend, starfive_wdt_resume)
> +};
> +
> +static struct platform_driver starfive_wdt_driver = {
> +	.probe		= starfive_wdt_probe,
> +	.remove		= starfive_wdt_remove,
> +	.shutdown	= starfive_wdt_shutdown,
> +	.id_table	= starfive_wdt_ids,
> +	.driver		= {
> +		.name	= "starfive-wdt",
> +		.pm	= &starfive_wdt_pm_ops,
> +		.of_match_table = of_match_ptr(starfive_wdt_match),
> +	},
> +};
> +
> +module_platform_driver(starfive_wdt_driver);
> +
> +MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
> +MODULE_AUTHOR("Samin Guo <samin.guo@starfivetech.com>");
> +MODULE_DESCRIPTION("StarFive Watchdog Device Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.25.1
>

Xingyu Wu Feb. 24, 2023, 7:42 a.m. UTC | #2

On 2023/2/24 2:23, Guenter Roeck wrote:
> On Mon, Feb 20, 2023 at 04:19:26PM +0800, Xingyu Wu wrote:
>> Add watchdog driver for the StarFive JH7110 SoC.
>> 
>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> ---
>>  MAINTAINERS                     |   7 +
>>  drivers/watchdog/Kconfig        |   9 +
>>  drivers/watchdog/Makefile       |   2 +
>>  drivers/watchdog/starfive-wdt.c | 651 ++++++++++++++++++++++++++++++++
>>  4 files changed, 669 insertions(+)
>>  create mode 100644 drivers/watchdog/starfive-wdt.c
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 135d93368d36..6cbcf08fa76a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19933,6 +19933,13 @@ F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>>  F:	drivers/reset/reset-starfive-jh7100.c
>>  F:	include/dt-bindings/reset/starfive-jh7100.h
>>  
>> +STARFIVE JH7110 WATCHDOG DRIVER
>> +M:	Xingyu Wu <xingyu.wu@starfivetech.com>
>> +M:	Samin Guo <samin.guo@starfivetech.com>
>> +S:	Supported
>> +F:	Documentation/devicetree/bindings/watchdog/starfive*
>> +F:	drivers/watchdog/starfive-wdt.c
>> +
>>  STATIC BRANCH/CALL
>>  M:	Peter Zijlstra <peterz@infradead.org>
>>  M:	Josh Poimboeuf <jpoimboe@kernel.org>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 0bc40b763b06..4608eb5c9501 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -2089,6 +2089,15 @@ config UML_WATCHDOG
>>  	tristate "UML watchdog"
>>  	depends on UML || COMPILE_TEST
>>  
>> +config STARFIVE_WATCHDOG
>> +	tristate "StarFive Watchdog support"
>> +	depends on RISCV
>> +	select WATCHDOG_CORE
>> +	default SOC_STARFIVE
>> +	help
>> +	  Say Y here to support the watchdog of StarFive JH7110 SoC.
>> +	  This driver can also be built as a module if choose M.
>> +
>>  #
>>  # ISA-based Watchdog Cards
>>  #
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 9cbf6580f16c..4c0bd377e92a 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -211,6 +211,8 @@ obj-$(CONFIG_WATCHDOG_SUN4V)		+= sun4v_wdt.o
>>  # Xen
>>  obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>>  
>> +obj-$(CONFIG_STARFIVE_WATCHDOG) += starfive-wdt.o
>> +
>>  # Architecture Independent
>>  obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
>>  obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
>> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
>> new file mode 100644
>> index 000000000000..dfbb80406076
>> --- /dev/null
>> +++ b/drivers/watchdog/starfive-wdt.c
>> @@ -0,0 +1,651 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Starfive Watchdog driver
>> + *
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>> +#include <linux/watchdog.h>
>> +
>> +/* JH7110 WatchDog register define */
>> +#define STARFIVE_WDT_JH7110_LOAD	0x000	/* RW: Watchdog load register */
>> +#define STARFIVE_WDT_JH7110_VALUE	0x004	/* RO: The current value for the watchdog counter */
>> +#define STARFIVE_WDT_JH7110_CONTROL	0x008	/*
>> +						 * RW:
>> +						 * [0]: reset enable;
>> +						 * [1]: int enable/wdt enable/reload counter;
>> +						 * [31:2]: reserve.
> 
> reserved

Will fix.

> 
>> +						 */
>> +#define STARFIVE_WDT_JH7110_INTCLR	0x00c	/* WO: clear intterupt && reload the counter */
>> +#define STARFIVE_WDT_JH7110_RIS		0x010	/* RO: Raw interrupt status from the counter */
>> +#define STARFIVE_WDT_JH7110_IMS		0x014	/* RO: Enabled interrupt status from the counter */
>> +#define STARFIVE_WDT_JH7110_LOCK	0xc00	/*
>> +						 * RO: Enable write access to all other registers
>> +						 * by writing 0x1ACCE551.
>> +						 */
> 
> "RO" contradicts itself against the rest of the comment. The register is
> written to, so it can't bew RO.

Change to 'RW'.

> 
>> +
>> +/* WDOGCONTROL */
>> +#define STARFIVE_WDT_ENABLE			0x1
>> +#define STARFIVE_WDT_JH7110_EN_SHIFT		0
>> +#define STARFIVE_WDT_RESET_EN			0x1
>> +#define STARFIVE_WDT_JH7110_RESEN_SHIFT		1
>> +
>> +/* WDOGLOCK */
>> +#define STARFIVE_WDT_LOCKED			BIT(0)
>> +#define STARFIVE_WDT_JH7110_UNLOCK_KEY		0x1acce551
>> +
>> +/* WDOGINTCLR */
>> +#define STARFIVE_WDT_INTCLR			0x1
>> +
>> +#define STARFIVE_WDT_MAXCNT			0xffffffff
>> +#define STARFIVE_WDT_DEFAULT_TIME		(15)
>> +#define STARFIVE_WDT_DELAY_US			0
>> +#define STARFIVE_WDT_TIMEOUT_US			10000
>> +
>> +/* module parameter */
>> +#define STARFIVE_WDT_EARLY_ENA			0
>> +
>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>> +static int heartbeat;
>> +static int early_enable = STARFIVE_WDT_EARLY_ENA;
>> +
>> +module_param(heartbeat, int, 0);
>> +module_param(early_enable, int, 0);
>> +module_param(nowayout, bool, 0);
>> +
>> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeat in seconds. (default="
>> +		 __MODULE_STRING(STARFIVE_WDT_DEFAULT_TIME) ")");
>> +MODULE_PARM_DESC(early_enable,
>> +		 "Watchdog is started at boot time if set to 1, default="
>> +		 __MODULE_STRING(STARFIVE_WDT_EARLY_ENA));
>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>> +
>> +struct starfive_wdt_variant {
>> +	u32 control;
>> +	u32 load;
>> +	u32 enable;
>> +	u32 value;
>> +	u32 int_clr;
>> +	u32 unlock;
>> +	u32 unlock_key;
>> +	u32 irq_is_raise;
>> +	u8 enrst_shift;
>> +	u8 en_shift;
>> +};
>> +
>> +struct starfive_wdt {
>> +	unsigned long freq;
>> +	struct device *dev;
>> +	struct watchdog_device wdt_device;
>> +	struct clk *core_clk;
>> +	struct clk *apb_clk;
>> +	struct reset_control *rsts;
>> +	const struct starfive_wdt_variant *drv_data;
>> +	u32 count;	/*count of timeout*/
>> +	u32 reload;	/*restore the count*/
>> +	void __iomem *base;
>> +	spinlock_t lock;	/* spinlock for register handling */
>> +};
>> +
>> +/* Register bias in JH7110 */
>> +static const struct starfive_wdt_variant drv_data_jh7110 = {
>> +	.control = STARFIVE_WDT_JH7110_CONTROL,
>> +	.load = STARFIVE_WDT_JH7110_LOAD,
>> +	.enable = STARFIVE_WDT_JH7110_CONTROL,
>> +	.value = STARFIVE_WDT_JH7110_VALUE,
>> +	.int_clr = STARFIVE_WDT_JH7110_INTCLR,
>> +	.unlock = STARFIVE_WDT_JH7110_LOCK,
>> +	.unlock_key = STARFIVE_WDT_JH7110_UNLOCK_KEY,
>> +	.irq_is_raise = STARFIVE_WDT_JH7110_IMS,
>> +	.enrst_shift = STARFIVE_WDT_JH7110_RESEN_SHIFT,
>> +	.en_shift = STARFIVE_WDT_JH7110_EN_SHIFT,
>> +};
>> +
>> +static const struct of_device_id starfive_wdt_match[] = {
>> +	{ .compatible = "starfive,jh7110-wdt", .data = &drv_data_jh7110 },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, starfive_wdt_match);
>> +
>> +static const struct platform_device_id starfive_wdt_ids[] = {
>> +	{
>> +		.name = "starfive-jh7110-wdt",
>> +		.driver_data = (unsigned long)&drv_data_jh7110,
>> +	},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(platform, starfive_wdt_ids);
>> +
>> +static int starfive_wdt_get_clock_rate(struct starfive_wdt *wdt)
>> +{
>> +	wdt->freq = clk_get_rate(wdt->core_clk);
>> +	/* The clock rate should not be 0.*/
> 
> This is obvious and not worth a comment.

Will drop.

> 
>> +	if (wdt->freq)
>> +		return 0;
>> +
>> +	dev_err(wdt->dev, "get clock rate failed.\n");
>> +	return -ENOENT;
> 
> Error handling should come first
> 
> 	if (!wdt->freq) {
> 		dev_err(wdt->dev, "get clock rate failed.\n");
> 		return -EINVAL;
> 	}
> 	return 0;
> 
> This is not "No such file or directory" (yes, I see that other drivers
> do that, but that doesn't make it better). Use -EINVAL instead.

Will fix.

> 
>> +}
>> +
>> +static int starfive_wdt_get_clock(struct starfive_wdt *wdt)
>> +{
>> +	wdt->apb_clk = devm_clk_get(wdt->dev, "apb");
>> +	if (IS_ERR(wdt->apb_clk)) {
>> +		dev_err(wdt->dev, "failed to get apb clock.\n");
>> +		return PTR_ERR(wdt->apb_clk);
>> +	}
>> +
>> +	wdt->core_clk = devm_clk_get(wdt->dev, "core");
>> +	if (IS_ERR(wdt->core_clk)) {
>> +		dev_err(wdt->dev, "failed to get core clock.\n");
>> +		return PTR_ERR(wdt->core_clk);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int starfive_wdt_reset_init(struct starfive_wdt *wdt)
>> +{
>> +	int ret = 0;
>> +
>> +	wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
>> +	if (IS_ERR(wdt->rsts)) {
>> +		dev_err(wdt->dev, "failed to get rsts error.\n");
> 
> I don't think this message is understandable. "failed to get reset
> control", maybe. Also, the call may return -EPROBE_DEFER,
> which should not generate an error message. Either drop the message
> or use dev_err_probe().

Thanks you for reminding me. I will drop this.
So is the devm_clk_get() and also drop the error message?

> 
>> +		ret = PTR_ERR(wdt->rsts);
>> +	} else {
>> +		ret = reset_control_deassert(wdt->rsts);
>> +		if (ret)
>> +			dev_err(wdt->dev, "failed to deassert rsts.\n");
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static u32 starfive_wdt_ticks_to_sec(struct starfive_wdt *wdt, u32 ticks)
>> +{
>> +	return DIV_ROUND_CLOSEST(ticks, wdt->freq);
>> +}
>> +
>> +/*
>> + * Write unlock-key to unlock. Write other value to lock. When lock bit is 1,
>> + * external accesses to other watchdog registers are ignored.
>> + */
>> +static bool starfive_wdt_is_locked(struct starfive_wdt *wdt)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(wdt->base + wdt->drv_data->unlock);
>> +	return !!(val & STARFIVE_WDT_LOCKED);
>> +}
>> +
>> +static void starfive_wdt_unlock(struct starfive_wdt *wdt)
>> +{
>> +	if (starfive_wdt_is_locked(wdt))
>> +		writel(wdt->drv_data->unlock_key,
>> +		       wdt->base + wdt->drv_data->unlock);
>> +}
>> +
>> +static void starfive_wdt_lock(struct starfive_wdt *wdt)
>> +{
>> +	if (!starfive_wdt_is_locked(wdt))
>> +		writel(~wdt->drv_data->unlock_key,
>> +		       wdt->base + wdt->drv_data->unlock);
>> +}
>> +
>> +/* enable watchdog interrupt to reset/reboot */
>> +static void starfive_wdt_enable_reset(struct starfive_wdt *wdt)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(wdt->base + wdt->drv_data->control);
>> +	val |= STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift;
>> +	writel(val, wdt->base + wdt->drv_data->control);
>> +}
>> +
>> +/* disable watchdog interrupt to reset/reboot */
>> +static void starfive_wdt_disable_reset(struct starfive_wdt *wdt)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(wdt->base + wdt->drv_data->control);
>> +	val &= ~(STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift);
>> +	writel(val, wdt->base + wdt->drv_data->control);
>> +}
>> +
>> +/* interrupt status whether has been raised from the counter */
>> +static bool starfive_wdt_raise_irq_status(struct starfive_wdt *wdt)
>> +{
>> +	return !!readl(wdt->base + wdt->drv_data->irq_is_raise);
>> +}
>> +
>> +/* clear interrupt signal before initialization or reload */
>> +static void starfive_wdt_int_clr(struct starfive_wdt *wdt)
>> +{
>> +	writel(STARFIVE_WDT_INTCLR, wdt->base + wdt->drv_data->int_clr);
>> +}
>> +
>> +static inline void starfive_wdt_set_count(struct starfive_wdt *wdt, u32 val)
>> +{
>> +	writel(val, wdt->base + wdt->drv_data->load);
>> +}
>> +
>> +static inline u32 starfive_wdt_get_count(struct starfive_wdt *wdt)
>> +{
>> +	return readl(wdt->base + wdt->drv_data->value);
>> +}
>> +
>> +/* enable watchdog */
>> +static inline void starfive_wdt_enable(struct starfive_wdt *wdt)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(wdt->base + wdt->drv_data->enable);
>> +	val |= STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift;
>> +	writel(val, wdt->base + wdt->drv_data->enable);
>> +}
>> +
>> +/* disable watchdog */
>> +static inline void starfive_wdt_disable(struct starfive_wdt *wdt)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(wdt->base + wdt->drv_data->enable);
>> +	val &= ~(STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift);
>> +	writel(val, wdt->base + wdt->drv_data->enable);
>> +}
>> +
>> +static inline void starfive_wdt_set_reload_count(struct starfive_wdt *wdt, u32 count)
>> +{
>> +	starfive_wdt_set_count(wdt, count);
>> +	/* need enable controller to reload counter */
>> +	starfive_wdt_enable(wdt);
>> +}
>> +
>> +static unsigned int starfive_wdt_max_timeout(struct starfive_wdt *wdt)
>> +{
>> +	return DIV_ROUND_UP(STARFIVE_WDT_MAXCNT, (wdt->freq / 2)) - 1;
>> +}
>> +
>> +static unsigned int starfive_wdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +	u32 count;
>> +
>> +	starfive_wdt_unlock(wdt);
>> +	/*
>> +	 * Because set half count value,
>> +	 * timeleft value should add the count value before first timeout.
>> +	 */
>> +	count = starfive_wdt_get_count(wdt);
>> +	if (!starfive_wdt_raise_irq_status(wdt))
>> +		count += wdt->count;
>> +
>> +	starfive_wdt_lock(wdt);
>> +
>> +	return starfive_wdt_ticks_to_sec(wdt, count);
>> +}
>> +
>> +static int starfive_wdt_keepalive(struct watchdog_device *wdd)
>> +{
>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	spin_lock(&wdt->lock);
>> +
>> +	starfive_wdt_unlock(wdt);
>> +	starfive_wdt_int_clr(wdt);
>> +	starfive_wdt_set_reload_count(wdt, wdt->count);
>> +	starfive_wdt_lock(wdt);
>> +
>> +	spin_unlock(&wdt->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static int starfive_wdt_stop(struct watchdog_device *wdd)
>> +{
>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	spin_lock(&wdt->lock);
>> +
>> +	starfive_wdt_unlock(wdt);
>> +	starfive_wdt_disable_reset(wdt);
> 
> Is that necessary ? The watchdog is going to be stopped, after all.

It doesn't seem necessary. Drop this. And drop this entire function
because no one use it. 

> 
>> +	starfive_wdt_int_clr(wdt);
>> +	starfive_wdt_disable(wdt);
>> +	starfive_wdt_lock(wdt);
>> +
>> +	spin_unlock(&wdt->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static int starfive_wdt_pm_stop(struct watchdog_device *wdd)
>> +{
>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	starfive_wdt_stop(wdd);
>> +	pm_runtime_put_sync(wdt->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int starfive_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	spin_lock(&wdt->lock);
>> +	starfive_wdt_unlock(wdt);
>> +	/* disable watchdog, to be safe */
>> +	starfive_wdt_disable(wdt);
>> +
>> +	starfive_wdt_enable_reset(wdt);
>> +	starfive_wdt_int_clr(wdt);
>> +	starfive_wdt_set_count(wdt, wdt->count);
>> +	starfive_wdt_enable(wdt);
>> +
>> +	starfive_wdt_lock(wdt);
>> +	spin_unlock(&wdt->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static int starfive_wdt_pm_start(struct watchdog_device *wdd)
>> +{
>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	pm_runtime_get_sync(wdt->dev);
>> +
>> +	return starfive_wdt_start(wdd);
>> +}
>> +
>> +static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
>> +				    unsigned int timeout)
>> +{
>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +	unsigned long freq = wdt->freq;
>> +
>> +	spin_lock(&wdt->lock);
>> +
>> +	/*
>> +	 * This watchdog takes twice timeouts to reset.
>> +	 * In order to reduce time to reset, should set half count value.
>> +	 */
>> +	wdt->count = timeout * freq / 2;
>> +	wdd->timeout = timeout;
>> +
>> +	starfive_wdt_unlock(wdt);
>> +	starfive_wdt_disable(wdt);
>> +	starfive_wdt_set_reload_count(wdt, wdt->count);
>> +	starfive_wdt_enable(wdt);
>> +	starfive_wdt_lock(wdt);
>> +
>> +	spin_unlock(&wdt->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
>> +
>> +static const struct watchdog_info starfive_wdt_ident = {
>> +	.options = OPTIONS,
>> +	.identity = "StarFive Watchdog",
>> +};
>> +
>> +static const struct watchdog_ops starfive_wdt_ops = {
>> +	.owner = THIS_MODULE,
>> +	.start = starfive_wdt_pm_start,
>> +	.stop = starfive_wdt_pm_stop,
>> +	.ping = starfive_wdt_keepalive,
>> +	.set_timeout = starfive_wdt_set_timeout,
>> +	.get_timeleft = starfive_wdt_get_timeleft,
>> +};
>> +
>> +static const struct watchdog_device starfive_wdd = {
>> +	.info = &starfive_wdt_ident,
>> +	.ops = &starfive_wdt_ops,
>> +	.timeout = STARFIVE_WDT_DEFAULT_TIME,
>> +};
>> +
>> +static inline const struct starfive_wdt_variant *
>> +starfive_wdt_get_drv_data(struct platform_device *pdev)
>> +{
>> +	const struct starfive_wdt_variant *variant;
>> +
>> +	variant = of_device_get_match_data(&pdev->dev);
>> +	if (!variant) {
>> +		/* Device matched by platform_device_id */
>> +		variant = (struct starfive_wdt_variant *)
>> +			   platform_get_device_id(pdev)->driver_data;
>> +	}
>> +
>> +	return variant;
>> +}
>> +
>> +static int starfive_wdt_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct starfive_wdt *wdt;
>> +	int ret;
>> +
>> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>> +	if (!wdt)
>> +		return -ENOMEM;
>> +
>> +	wdt->dev = dev;
>> +	spin_lock_init(&wdt->lock);
>> +	wdt->wdt_device = starfive_wdd;
>> +
>> +	wdt->drv_data = starfive_wdt_get_drv_data(pdev);
>> +
>> +	/* get the memory region for the watchdog timer */
>> +	wdt->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(wdt->base)) {
>> +		ret = PTR_ERR(wdt->base);
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, wdt);
>> +	pm_runtime_enable(wdt->dev);
>> +
>> +	ret = starfive_wdt_get_clock(wdt);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (pm_runtime_enabled(wdt->dev)) {
>> +		ret = pm_runtime_get_sync(wdt->dev);
>> +		if (ret < 0)
>> +			return ret;
>> +	} else {
>> +		/* runtime PM is disabled but clocks need to be enabled */
>> +		ret = clk_prepare_enable(wdt->apb_clk);
>> +		if (ret) {
>> +			dev_err(wdt->dev, "failed to enable apb_clk.\n");
>> +			return ret;
>> +		}
>> +		ret = clk_prepare_enable(wdt->core_clk);
>> +		if (ret) {
>> +			dev_err(wdt->dev, "failed to enable core_clk.\n");
>> +			goto err_apb_clk_disable;
>> +		}
>> +	}
>> +
>> +	ret = starfive_wdt_get_clock_rate(wdt);
>> +	if (ret)
>> +		goto err_clk_disable;
>> +
>> +	ret = starfive_wdt_reset_init(wdt);
>> +	if (ret)
>> +		goto err_clk_disable;
>> +
>> +	wdt->wdt_device.min_timeout = 1;
>> +	wdt->wdt_device.max_timeout = starfive_wdt_max_timeout(wdt);
> 
> 	wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
> 
> should be set here. Otherwise the warning below would always be seen
> if the module parameter is not set.
> 
>> +
>> +	watchdog_set_drvdata(&wdt->wdt_device, wdt);
>> +
>> +	/*
>> +	 * see if we can actually set the requested heartbeat,
>> +	 * and if not, try the default value.
>> +	 */
>> +	watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>> +	if (wdt->wdt_device.timeout == 0 ||
> 
> If wdt->wdt_device.timeout is pre-initialized, it will never be 0 here.
> 
>> +	    wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
> 
> That won't happen because watchdog_init_timeout() validates it and does
> not update the value if it is out of range.
> 
>> +		dev_warn(dev, "heartbeat value out of range, default %d used\n",
>> +			 STARFIVE_WDT_DEFAULT_TIME);
>> +		wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
> 
> And this is then unnecessary. wdt->wdt_device.timeout will always be
> valid if it was pre-initialized.

It is changed to be this at beginning of the driver:

static int heartbeat = STARFIVE_WDT_DEFAULT_TIME;

and it is changed to be this here:

ret = watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
if (ret)
	return ret;

Would that be better?

> 
>> +	}
>> +	starfive_wdt_set_timeout(&wdt->wdt_device, wdt->wdt_device.timeout);
>> +
>> +	watchdog_set_nowayout(&wdt->wdt_device, nowayout);
>> +	watchdog_stop_on_reboot(&wdt->wdt_device);
>> +	watchdog_stop_on_unregister(&wdt->wdt_device);
>> +
>> +	wdt->wdt_device.parent = dev;
>> +
>> +	ret = watchdog_register_device(&wdt->wdt_device);
>> +	if (ret)
>> +		goto err_clk_disable;
>> +
>> +	if (early_enable) {
>> +		starfive_wdt_start(&wdt->wdt_device);
>> +		set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> 
> Is it correct to call pm_runtime_put_sync() below if the watchdog
> is running ? Doesn't that stop the clock ?

Add a check of 'early_enable' to pm_runtime_put_sync() or
move it after starfive_wdt_stop() immediately.

> 
>> +	} else {
>> +		starfive_wdt_stop(&wdt->wdt_device);
>> +	}
> 
> Wrong order. This has to be done _before_ the watchdog device is registered
> to make sure that the watchdog core knows about WDOG_HW_RUNNING.

So move this before watchdog_register_device().

Best regards,
Xingyu Wu

Guenter Roeck Feb. 24, 2023, 3:18 p.m. UTC | #3

On 2/23/23 23:42, Xingyu Wu wrote:
> On 2023/2/24 2:23, Guenter Roeck wrote:
>> On Mon, Feb 20, 2023 at 04:19:26PM +0800, Xingyu Wu wrote:
>>> Add watchdog driver for the StarFive JH7110 SoC.
>>>
>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>> ---
>>>   MAINTAINERS                     |   7 +
>>>   drivers/watchdog/Kconfig        |   9 +
>>>   drivers/watchdog/Makefile       |   2 +
>>>   drivers/watchdog/starfive-wdt.c | 651 ++++++++++++++++++++++++++++++++
>>>   4 files changed, 669 insertions(+)
>>>   create mode 100644 drivers/watchdog/starfive-wdt.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 135d93368d36..6cbcf08fa76a 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -19933,6 +19933,13 @@ F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>>>   F:	drivers/reset/reset-starfive-jh7100.c
>>>   F:	include/dt-bindings/reset/starfive-jh7100.h
>>>   
>>> +STARFIVE JH7110 WATCHDOG DRIVER
>>> +M:	Xingyu Wu <xingyu.wu@starfivetech.com>
>>> +M:	Samin Guo <samin.guo@starfivetech.com>
>>> +S:	Supported
>>> +F:	Documentation/devicetree/bindings/watchdog/starfive*
>>> +F:	drivers/watchdog/starfive-wdt.c
>>> +
>>>   STATIC BRANCH/CALL
>>>   M:	Peter Zijlstra <peterz@infradead.org>
>>>   M:	Josh Poimboeuf <jpoimboe@kernel.org>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index 0bc40b763b06..4608eb5c9501 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -2089,6 +2089,15 @@ config UML_WATCHDOG
>>>   	tristate "UML watchdog"
>>>   	depends on UML || COMPILE_TEST
>>>   
>>> +config STARFIVE_WATCHDOG
>>> +	tristate "StarFive Watchdog support"
>>> +	depends on RISCV
>>> +	select WATCHDOG_CORE
>>> +	default SOC_STARFIVE
>>> +	help
>>> +	  Say Y here to support the watchdog of StarFive JH7110 SoC.
>>> +	  This driver can also be built as a module if choose M.
>>> +
>>>   #
>>>   # ISA-based Watchdog Cards
>>>   #
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index 9cbf6580f16c..4c0bd377e92a 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -211,6 +211,8 @@ obj-$(CONFIG_WATCHDOG_SUN4V)		+= sun4v_wdt.o
>>>   # Xen
>>>   obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>>>   
>>> +obj-$(CONFIG_STARFIVE_WATCHDOG) += starfive-wdt.o
>>> +
>>>   # Architecture Independent
>>>   obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
>>>   obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
>>> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
>>> new file mode 100644
>>> index 000000000000..dfbb80406076
>>> --- /dev/null
>>> +++ b/drivers/watchdog/starfive-wdt.c
>>> @@ -0,0 +1,651 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Starfive Watchdog driver
>>> + *
>>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/iopoll.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/reset.h>
>>> +#include <linux/watchdog.h>
>>> +
>>> +/* JH7110 WatchDog register define */
>>> +#define STARFIVE_WDT_JH7110_LOAD	0x000	/* RW: Watchdog load register */
>>> +#define STARFIVE_WDT_JH7110_VALUE	0x004	/* RO: The current value for the watchdog counter */
>>> +#define STARFIVE_WDT_JH7110_CONTROL	0x008	/*
>>> +						 * RW:
>>> +						 * [0]: reset enable;
>>> +						 * [1]: int enable/wdt enable/reload counter;
>>> +						 * [31:2]: reserve.
>>
>> reserved
> 
> Will fix.
> 
>>
>>> +						 */
>>> +#define STARFIVE_WDT_JH7110_INTCLR	0x00c	/* WO: clear intterupt && reload the counter */
>>> +#define STARFIVE_WDT_JH7110_RIS		0x010	/* RO: Raw interrupt status from the counter */
>>> +#define STARFIVE_WDT_JH7110_IMS		0x014	/* RO: Enabled interrupt status from the counter */
>>> +#define STARFIVE_WDT_JH7110_LOCK	0xc00	/*
>>> +						 * RO: Enable write access to all other registers
>>> +						 * by writing 0x1ACCE551.
>>> +						 */
>>
>> "RO" contradicts itself against the rest of the comment. The register is
>> written to, so it can't bew RO.
> 
> Change to 'RW'.
> 
>>
>>> +
>>> +/* WDOGCONTROL */
>>> +#define STARFIVE_WDT_ENABLE			0x1
>>> +#define STARFIVE_WDT_JH7110_EN_SHIFT		0
>>> +#define STARFIVE_WDT_RESET_EN			0x1
>>> +#define STARFIVE_WDT_JH7110_RESEN_SHIFT		1
>>> +
>>> +/* WDOGLOCK */
>>> +#define STARFIVE_WDT_LOCKED			BIT(0)
>>> +#define STARFIVE_WDT_JH7110_UNLOCK_KEY		0x1acce551
>>> +
>>> +/* WDOGINTCLR */
>>> +#define STARFIVE_WDT_INTCLR			0x1
>>> +
>>> +#define STARFIVE_WDT_MAXCNT			0xffffffff
>>> +#define STARFIVE_WDT_DEFAULT_TIME		(15)
>>> +#define STARFIVE_WDT_DELAY_US			0
>>> +#define STARFIVE_WDT_TIMEOUT_US			10000
>>> +
>>> +/* module parameter */
>>> +#define STARFIVE_WDT_EARLY_ENA			0
>>> +
>>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>>> +static int heartbeat;
>>> +static int early_enable = STARFIVE_WDT_EARLY_ENA;
>>> +
>>> +module_param(heartbeat, int, 0);
>>> +module_param(early_enable, int, 0);
>>> +module_param(nowayout, bool, 0);
>>> +
>>> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeat in seconds. (default="
>>> +		 __MODULE_STRING(STARFIVE_WDT_DEFAULT_TIME) ")");
>>> +MODULE_PARM_DESC(early_enable,
>>> +		 "Watchdog is started at boot time if set to 1, default="
>>> +		 __MODULE_STRING(STARFIVE_WDT_EARLY_ENA));
>>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>>> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>> +
>>> +struct starfive_wdt_variant {
>>> +	u32 control;
>>> +	u32 load;
>>> +	u32 enable;
>>> +	u32 value;
>>> +	u32 int_clr;
>>> +	u32 unlock;
>>> +	u32 unlock_key;
>>> +	u32 irq_is_raise;
>>> +	u8 enrst_shift;
>>> +	u8 en_shift;
>>> +};
>>> +
>>> +struct starfive_wdt {
>>> +	unsigned long freq;
>>> +	struct device *dev;
>>> +	struct watchdog_device wdt_device;
>>> +	struct clk *core_clk;
>>> +	struct clk *apb_clk;
>>> +	struct reset_control *rsts;
>>> +	const struct starfive_wdt_variant *drv_data;
>>> +	u32 count;	/*count of timeout*/
>>> +	u32 reload;	/*restore the count*/
>>> +	void __iomem *base;
>>> +	spinlock_t lock;	/* spinlock for register handling */
>>> +};
>>> +
>>> +/* Register bias in JH7110 */
>>> +static const struct starfive_wdt_variant drv_data_jh7110 = {
>>> +	.control = STARFIVE_WDT_JH7110_CONTROL,
>>> +	.load = STARFIVE_WDT_JH7110_LOAD,
>>> +	.enable = STARFIVE_WDT_JH7110_CONTROL,
>>> +	.value = STARFIVE_WDT_JH7110_VALUE,
>>> +	.int_clr = STARFIVE_WDT_JH7110_INTCLR,
>>> +	.unlock = STARFIVE_WDT_JH7110_LOCK,
>>> +	.unlock_key = STARFIVE_WDT_JH7110_UNLOCK_KEY,
>>> +	.irq_is_raise = STARFIVE_WDT_JH7110_IMS,
>>> +	.enrst_shift = STARFIVE_WDT_JH7110_RESEN_SHIFT,
>>> +	.en_shift = STARFIVE_WDT_JH7110_EN_SHIFT,
>>> +};
>>> +
>>> +static const struct of_device_id starfive_wdt_match[] = {
>>> +	{ .compatible = "starfive,jh7110-wdt", .data = &drv_data_jh7110 },
>>> +	{}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, starfive_wdt_match);
>>> +
>>> +static const struct platform_device_id starfive_wdt_ids[] = {
>>> +	{
>>> +		.name = "starfive-jh7110-wdt",
>>> +		.driver_data = (unsigned long)&drv_data_jh7110,
>>> +	},
>>> +	{}
>>> +};
>>> +MODULE_DEVICE_TABLE(platform, starfive_wdt_ids);
>>> +
>>> +static int starfive_wdt_get_clock_rate(struct starfive_wdt *wdt)
>>> +{
>>> +	wdt->freq = clk_get_rate(wdt->core_clk);
>>> +	/* The clock rate should not be 0.*/
>>
>> This is obvious and not worth a comment.
> 
> Will drop.
> 
>>
>>> +	if (wdt->freq)
>>> +		return 0;
>>> +
>>> +	dev_err(wdt->dev, "get clock rate failed.\n");
>>> +	return -ENOENT;
>>
>> Error handling should come first
>>
>> 	if (!wdt->freq) {
>> 		dev_err(wdt->dev, "get clock rate failed.\n");
>> 		return -EINVAL;
>> 	}
>> 	return 0;
>>
>> This is not "No such file or directory" (yes, I see that other drivers
>> do that, but that doesn't make it better). Use -EINVAL instead.
> 
> Will fix.
> 
>>
>>> +}
>>> +
>>> +static int starfive_wdt_get_clock(struct starfive_wdt *wdt)
>>> +{
>>> +	wdt->apb_clk = devm_clk_get(wdt->dev, "apb");
>>> +	if (IS_ERR(wdt->apb_clk)) {
>>> +		dev_err(wdt->dev, "failed to get apb clock.\n");
>>> +		return PTR_ERR(wdt->apb_clk);
>>> +	}
>>> +
>>> +	wdt->core_clk = devm_clk_get(wdt->dev, "core");
>>> +	if (IS_ERR(wdt->core_clk)) {
>>> +		dev_err(wdt->dev, "failed to get core clock.\n");
>>> +		return PTR_ERR(wdt->core_clk);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int starfive_wdt_reset_init(struct starfive_wdt *wdt)
>>> +{
>>> +	int ret = 0;
>>> +
>>> +	wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
>>> +	if (IS_ERR(wdt->rsts)) {
>>> +		dev_err(wdt->dev, "failed to get rsts error.\n");
>>
>> I don't think this message is understandable. "failed to get reset
>> control", maybe. Also, the call may return -EPROBE_DEFER,
>> which should not generate an error message. Either drop the message
>> or use dev_err_probe().
> 
> Thanks you for reminding me. I will drop this.
> So is the devm_clk_get() and also drop the error message?
> 

Yes, actually. It can indeed also return -EPROBE_DEFER.

>>
>>> +		ret = PTR_ERR(wdt->rsts);
>>> +	} else {
>>> +		ret = reset_control_deassert(wdt->rsts);
>>> +		if (ret)
>>> +			dev_err(wdt->dev, "failed to deassert rsts.\n");
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static u32 starfive_wdt_ticks_to_sec(struct starfive_wdt *wdt, u32 ticks)
>>> +{
>>> +	return DIV_ROUND_CLOSEST(ticks, wdt->freq);
>>> +}
>>> +
>>> +/*
>>> + * Write unlock-key to unlock. Write other value to lock. When lock bit is 1,
>>> + * external accesses to other watchdog registers are ignored.
>>> + */
>>> +static bool starfive_wdt_is_locked(struct starfive_wdt *wdt)
>>> +{
>>> +	u32 val;
>>> +
>>> +	val = readl(wdt->base + wdt->drv_data->unlock);
>>> +	return !!(val & STARFIVE_WDT_LOCKED);
>>> +}
>>> +
>>> +static void starfive_wdt_unlock(struct starfive_wdt *wdt)
>>> +{
>>> +	if (starfive_wdt_is_locked(wdt))
>>> +		writel(wdt->drv_data->unlock_key,
>>> +		       wdt->base + wdt->drv_data->unlock);
>>> +}
>>> +
>>> +static void starfive_wdt_lock(struct starfive_wdt *wdt)
>>> +{
>>> +	if (!starfive_wdt_is_locked(wdt))
>>> +		writel(~wdt->drv_data->unlock_key,
>>> +		       wdt->base + wdt->drv_data->unlock);
>>> +}
>>> +
>>> +/* enable watchdog interrupt to reset/reboot */
>>> +static void starfive_wdt_enable_reset(struct starfive_wdt *wdt)
>>> +{
>>> +	u32 val;
>>> +
>>> +	val = readl(wdt->base + wdt->drv_data->control);
>>> +	val |= STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift;
>>> +	writel(val, wdt->base + wdt->drv_data->control);
>>> +}
>>> +
>>> +/* disable watchdog interrupt to reset/reboot */
>>> +static void starfive_wdt_disable_reset(struct starfive_wdt *wdt)
>>> +{
>>> +	u32 val;
>>> +
>>> +	val = readl(wdt->base + wdt->drv_data->control);
>>> +	val &= ~(STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift);
>>> +	writel(val, wdt->base + wdt->drv_data->control);
>>> +}
>>> +
>>> +/* interrupt status whether has been raised from the counter */
>>> +static bool starfive_wdt_raise_irq_status(struct starfive_wdt *wdt)
>>> +{
>>> +	return !!readl(wdt->base + wdt->drv_data->irq_is_raise);
>>> +}
>>> +
>>> +/* clear interrupt signal before initialization or reload */
>>> +static void starfive_wdt_int_clr(struct starfive_wdt *wdt)
>>> +{
>>> +	writel(STARFIVE_WDT_INTCLR, wdt->base + wdt->drv_data->int_clr);
>>> +}
>>> +
>>> +static inline void starfive_wdt_set_count(struct starfive_wdt *wdt, u32 val)
>>> +{
>>> +	writel(val, wdt->base + wdt->drv_data->load);
>>> +}
>>> +
>>> +static inline u32 starfive_wdt_get_count(struct starfive_wdt *wdt)
>>> +{
>>> +	return readl(wdt->base + wdt->drv_data->value);
>>> +}
>>> +
>>> +/* enable watchdog */
>>> +static inline void starfive_wdt_enable(struct starfive_wdt *wdt)
>>> +{
>>> +	u32 val;
>>> +
>>> +	val = readl(wdt->base + wdt->drv_data->enable);
>>> +	val |= STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift;
>>> +	writel(val, wdt->base + wdt->drv_data->enable);
>>> +}
>>> +
>>> +/* disable watchdog */
>>> +static inline void starfive_wdt_disable(struct starfive_wdt *wdt)
>>> +{
>>> +	u32 val;
>>> +
>>> +	val = readl(wdt->base + wdt->drv_data->enable);
>>> +	val &= ~(STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift);
>>> +	writel(val, wdt->base + wdt->drv_data->enable);
>>> +}
>>> +
>>> +static inline void starfive_wdt_set_reload_count(struct starfive_wdt *wdt, u32 count)
>>> +{
>>> +	starfive_wdt_set_count(wdt, count);
>>> +	/* need enable controller to reload counter */
>>> +	starfive_wdt_enable(wdt);
>>> +}
>>> +
>>> +static unsigned int starfive_wdt_max_timeout(struct starfive_wdt *wdt)
>>> +{
>>> +	return DIV_ROUND_UP(STARFIVE_WDT_MAXCNT, (wdt->freq / 2)) - 1;
>>> +}
>>> +
>>> +static unsigned int starfive_wdt_get_timeleft(struct watchdog_device *wdd)
>>> +{
>>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>> +	u32 count;
>>> +
>>> +	starfive_wdt_unlock(wdt);
>>> +	/*
>>> +	 * Because set half count value,
>>> +	 * timeleft value should add the count value before first timeout.
>>> +	 */
>>> +	count = starfive_wdt_get_count(wdt);
>>> +	if (!starfive_wdt_raise_irq_status(wdt))
>>> +		count += wdt->count;
>>> +
>>> +	starfive_wdt_lock(wdt);
>>> +
>>> +	return starfive_wdt_ticks_to_sec(wdt, count);
>>> +}
>>> +
>>> +static int starfive_wdt_keepalive(struct watchdog_device *wdd)
>>> +{
>>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>> +
>>> +	spin_lock(&wdt->lock);
>>> +
>>> +	starfive_wdt_unlock(wdt);
>>> +	starfive_wdt_int_clr(wdt);
>>> +	starfive_wdt_set_reload_count(wdt, wdt->count);
>>> +	starfive_wdt_lock(wdt);
>>> +
>>> +	spin_unlock(&wdt->lock);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int starfive_wdt_stop(struct watchdog_device *wdd)
>>> +{
>>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>> +
>>> +	spin_lock(&wdt->lock);
>>> +
>>> +	starfive_wdt_unlock(wdt);
>>> +	starfive_wdt_disable_reset(wdt);
>>
>> Is that necessary ? The watchdog is going to be stopped, after all.
> 
> It doesn't seem necessary. Drop this. And drop this entire function
> because no one use it.
> 
>>
>>> +	starfive_wdt_int_clr(wdt);
>>> +	starfive_wdt_disable(wdt);
>>> +	starfive_wdt_lock(wdt);
>>> +
>>> +	spin_unlock(&wdt->lock);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int starfive_wdt_pm_stop(struct watchdog_device *wdd)
>>> +{
>>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>> +
>>> +	starfive_wdt_stop(wdd);
>>> +	pm_runtime_put_sync(wdt->dev);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int starfive_wdt_start(struct watchdog_device *wdd)
>>> +{
>>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>> +
>>> +	spin_lock(&wdt->lock);
>>> +	starfive_wdt_unlock(wdt);
>>> +	/* disable watchdog, to be safe */
>>> +	starfive_wdt_disable(wdt);
>>> +
>>> +	starfive_wdt_enable_reset(wdt);
>>> +	starfive_wdt_int_clr(wdt);
>>> +	starfive_wdt_set_count(wdt, wdt->count);
>>> +	starfive_wdt_enable(wdt);
>>> +
>>> +	starfive_wdt_lock(wdt);
>>> +	spin_unlock(&wdt->lock);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int starfive_wdt_pm_start(struct watchdog_device *wdd)
>>> +{
>>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>> +
>>> +	pm_runtime_get_sync(wdt->dev);
>>> +
>>> +	return starfive_wdt_start(wdd);
>>> +}
>>> +
>>> +static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
>>> +				    unsigned int timeout)
>>> +{
>>> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>> +	unsigned long freq = wdt->freq;
>>> +
>>> +	spin_lock(&wdt->lock);
>>> +
>>> +	/*
>>> +	 * This watchdog takes twice timeouts to reset.
>>> +	 * In order to reduce time to reset, should set half count value.
>>> +	 */
>>> +	wdt->count = timeout * freq / 2;
>>> +	wdd->timeout = timeout;
>>> +
>>> +	starfive_wdt_unlock(wdt);
>>> +	starfive_wdt_disable(wdt);
>>> +	starfive_wdt_set_reload_count(wdt, wdt->count);
>>> +	starfive_wdt_enable(wdt);
>>> +	starfive_wdt_lock(wdt);
>>> +
>>> +	spin_unlock(&wdt->lock);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
>>> +
>>> +static const struct watchdog_info starfive_wdt_ident = {
>>> +	.options = OPTIONS,
>>> +	.identity = "StarFive Watchdog",
>>> +};
>>> +
>>> +static const struct watchdog_ops starfive_wdt_ops = {
>>> +	.owner = THIS_MODULE,
>>> +	.start = starfive_wdt_pm_start,
>>> +	.stop = starfive_wdt_pm_stop,
>>> +	.ping = starfive_wdt_keepalive,
>>> +	.set_timeout = starfive_wdt_set_timeout,
>>> +	.get_timeleft = starfive_wdt_get_timeleft,
>>> +};
>>> +
>>> +static const struct watchdog_device starfive_wdd = {
>>> +	.info = &starfive_wdt_ident,
>>> +	.ops = &starfive_wdt_ops,
>>> +	.timeout = STARFIVE_WDT_DEFAULT_TIME,
>>> +};
>>> +
>>> +static inline const struct starfive_wdt_variant *
>>> +starfive_wdt_get_drv_data(struct platform_device *pdev)
>>> +{
>>> +	const struct starfive_wdt_variant *variant;
>>> +
>>> +	variant = of_device_get_match_data(&pdev->dev);
>>> +	if (!variant) {
>>> +		/* Device matched by platform_device_id */
>>> +		variant = (struct starfive_wdt_variant *)
>>> +			   platform_get_device_id(pdev)->driver_data;
>>> +	}
>>> +
>>> +	return variant;
>>> +}
>>> +
>>> +static int starfive_wdt_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct starfive_wdt *wdt;
>>> +	int ret;
>>> +
>>> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>> +	if (!wdt)
>>> +		return -ENOMEM;
>>> +
>>> +	wdt->dev = dev;
>>> +	spin_lock_init(&wdt->lock);
>>> +	wdt->wdt_device = starfive_wdd;
>>> +
>>> +	wdt->drv_data = starfive_wdt_get_drv_data(pdev);
>>> +
>>> +	/* get the memory region for the watchdog timer */
>>> +	wdt->base = devm_platform_ioremap_resource(pdev, 0);
>>> +	if (IS_ERR(wdt->base)) {
>>> +		ret = PTR_ERR(wdt->base);
>>> +		return ret;
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, wdt);
>>> +	pm_runtime_enable(wdt->dev);
>>> +
>>> +	ret = starfive_wdt_get_clock(wdt);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (pm_runtime_enabled(wdt->dev)) {
>>> +		ret = pm_runtime_get_sync(wdt->dev);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +	} else {
>>> +		/* runtime PM is disabled but clocks need to be enabled */
>>> +		ret = clk_prepare_enable(wdt->apb_clk);
>>> +		if (ret) {
>>> +			dev_err(wdt->dev, "failed to enable apb_clk.\n");
>>> +			return ret;
>>> +		}
>>> +		ret = clk_prepare_enable(wdt->core_clk);
>>> +		if (ret) {
>>> +			dev_err(wdt->dev, "failed to enable core_clk.\n");
>>> +			goto err_apb_clk_disable;
>>> +		}
>>> +	}
>>> +
>>> +	ret = starfive_wdt_get_clock_rate(wdt);
>>> +	if (ret)
>>> +		goto err_clk_disable;
>>> +
>>> +	ret = starfive_wdt_reset_init(wdt);
>>> +	if (ret)
>>> +		goto err_clk_disable;
>>> +
>>> +	wdt->wdt_device.min_timeout = 1;
>>> +	wdt->wdt_device.max_timeout = starfive_wdt_max_timeout(wdt);
>>
>> 	wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>
>> should be set here. Otherwise the warning below would always be seen
>> if the module parameter is not set.
>>
>>> +
>>> +	watchdog_set_drvdata(&wdt->wdt_device, wdt);
>>> +
>>> +	/*
>>> +	 * see if we can actually set the requested heartbeat,
>>> +	 * and if not, try the default value.
>>> +	 */
>>> +	watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>> +	if (wdt->wdt_device.timeout == 0 ||
>>
>> If wdt->wdt_device.timeout is pre-initialized, it will never be 0 here.
>>
>>> +	    wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
>>
>> That won't happen because watchdog_init_timeout() validates it and does
>> not update the value if it is out of range.
>>
>>> +		dev_warn(dev, "heartbeat value out of range, default %d used\n",
>>> +			 STARFIVE_WDT_DEFAULT_TIME);
>>> +		wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>
>> And this is then unnecessary. wdt->wdt_device.timeout will always be
>> valid if it was pre-initialized.
> 
> It is changed to be this at beginning of the driver:
> 
> static int heartbeat = STARFIVE_WDT_DEFAULT_TIME;
> 

No, this is wrong. The static variable should be set to 0 to indicate
"use default".

> and it is changed to be this here:
> 
> ret = watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
> if (ret)
> 	return ret;
> 
> Would that be better?
> 

No, it is worse, because it would not instantiate the watchdog at all
if a bad heartbeat is provided.

>>
>>> +	}
>>> +	starfive_wdt_set_timeout(&wdt->wdt_device, wdt->wdt_device.timeout);
>>> +
>>> +	watchdog_set_nowayout(&wdt->wdt_device, nowayout);
>>> +	watchdog_stop_on_reboot(&wdt->wdt_device);
>>> +	watchdog_stop_on_unregister(&wdt->wdt_device);
>>> +
>>> +	wdt->wdt_device.parent = dev;
>>> +
>>> +	ret = watchdog_register_device(&wdt->wdt_device);
>>> +	if (ret)
>>> +		goto err_clk_disable;
>>> +
>>> +	if (early_enable) {
>>> +		starfive_wdt_start(&wdt->wdt_device);
>>> +		set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
>>
>> Is it correct to call pm_runtime_put_sync() below if the watchdog
>> is running ? Doesn't that stop the clock ?
> 
> Add a check of 'early_enable' to pm_runtime_put_sync() or
> move it after starfive_wdt_stop() immediately.
> 
>>
>>> +	} else {
>>> +		starfive_wdt_stop(&wdt->wdt_device);
>>> +	}
>>
>> Wrong order. This has to be done _before_ the watchdog device is registered
>> to make sure that the watchdog core knows about WDOG_HW_RUNNING.
> 
> So move this before watchdog_register_device().
> 
> Best regards,
> Xingyu Wu

Emil Renner Berthing Feb. 26, 2023, 2:14 p.m. UTC | #4

On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>
> Add watchdog driver for the StarFive JH7110 SoC.
>
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  MAINTAINERS                     |   7 +
>  drivers/watchdog/Kconfig        |   9 +
>  drivers/watchdog/Makefile       |   2 +
>  drivers/watchdog/starfive-wdt.c | 651 ++++++++++++++++++++++++++++++++
>  4 files changed, 669 insertions(+)
>  create mode 100644 drivers/watchdog/starfive-wdt.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 135d93368d36..6cbcf08fa76a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19933,6 +19933,13 @@ F:     Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>  F:     drivers/reset/reset-starfive-jh7100.c
>  F:     include/dt-bindings/reset/starfive-jh7100.h
>
> +STARFIVE JH7110 WATCHDOG DRIVER
> +M:     Xingyu Wu <xingyu.wu@starfivetech.com>
> +M:     Samin Guo <samin.guo@starfivetech.com>
> +S:     Supported
> +F:     Documentation/devicetree/bindings/watchdog/starfive*
> +F:     drivers/watchdog/starfive-wdt.c
> +
>  STATIC BRANCH/CALL
>  M:     Peter Zijlstra <peterz@infradead.org>
>  M:     Josh Poimboeuf <jpoimboe@kernel.org>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0bc40b763b06..4608eb5c9501 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2089,6 +2089,15 @@ config UML_WATCHDOG
>         tristate "UML watchdog"
>         depends on UML || COMPILE_TEST
>
> +config STARFIVE_WATCHDOG
> +       tristate "StarFive Watchdog support"
> +       depends on RISCV

Let's do like the pinctrl and clock drivers and

    depends SOC_STARFIVE || COMPILE_TEST

> +       select WATCHDOG_CORE
> +       default SOC_STARFIVE
> +       help
> +         Say Y here to support the watchdog of StarFive JH7110 SoC.
> +         This driver can also be built as a module if choose M.
> +
>  #
>  # ISA-based Watchdog Cards
>  #
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 9cbf6580f16c..4c0bd377e92a 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -211,6 +211,8 @@ obj-$(CONFIG_WATCHDOG_SUN4V)                += sun4v_wdt.o
>  # Xen
>  obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>
> +obj-$(CONFIG_STARFIVE_WATCHDOG) += starfive-wdt.o
> +
>  # Architecture Independent
>  obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
>  obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> new file mode 100644
> index 000000000000..dfbb80406076
> --- /dev/null
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -0,0 +1,651 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Starfive Watchdog driver
> + *
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/watchdog.h>
> +
> +/* JH7110 WatchDog register define */
> +#define STARFIVE_WDT_JH7110_LOAD       0x000   /* RW: Watchdog load register */
> +#define STARFIVE_WDT_JH7110_VALUE      0x004   /* RO: The current value for the watchdog counter */
> +#define STARFIVE_WDT_JH7110_CONTROL    0x008   /*
> +                                                * RW:
> +                                                * [0]: reset enable;
> +                                                * [1]: int enable/wdt enable/reload counter;
> +                                                * [31:2]: reserve.
> +                                                */
> +#define STARFIVE_WDT_JH7110_INTCLR     0x00c   /* WO: clear intterupt && reload the counter */
> +#define STARFIVE_WDT_JH7110_RIS                0x010   /* RO: Raw interrupt status from the counter */
> +#define STARFIVE_WDT_JH7110_IMS                0x014   /* RO: Enabled interrupt status from the counter */
> +#define STARFIVE_WDT_JH7110_LOCK       0xc00   /*
> +                                                * RO: Enable write access to all other registers
> +                                                * by writing 0x1ACCE551.
> +                                                */

This driver is clearly prepared to support multiple SoCs, so hopefully
you've tested that internally. It would be great if you could leave in
the support for the JH7100 in the next revision. This would also make
all the indirection make a lot more sense.

> +/* WDOGCONTROL */
> +#define STARFIVE_WDT_ENABLE                    0x1
> +#define STARFIVE_WDT_JH7110_EN_SHIFT           0
> +#define STARFIVE_WDT_RESET_EN                  0x1
> +#define STARFIVE_WDT_JH7110_RESEN_SHIFT                1
> +
> +/* WDOGLOCK */
> +#define STARFIVE_WDT_LOCKED                    BIT(0)
> +#define STARFIVE_WDT_JH7110_UNLOCK_KEY         0x1acce551
> +
> +/* WDOGINTCLR */
> +#define STARFIVE_WDT_INTCLR                    0x1
> +
> +#define STARFIVE_WDT_MAXCNT                    0xffffffff
> +#define STARFIVE_WDT_DEFAULT_TIME              (15)
> +#define STARFIVE_WDT_DELAY_US                  0
> +#define STARFIVE_WDT_TIMEOUT_US                        10000
> +
> +/* module parameter */
> +#define STARFIVE_WDT_EARLY_ENA                 0
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +static int heartbeat;
> +static int early_enable = STARFIVE_WDT_EARLY_ENA;
> +
> +module_param(heartbeat, int, 0);
> +module_param(early_enable, int, 0);

This also looks like a bool parameter.

> +module_param(nowayout, bool, 0);
> +
> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeat in seconds. (default="
> +                __MODULE_STRING(STARFIVE_WDT_DEFAULT_TIME) ")");
> +MODULE_PARM_DESC(early_enable,
> +                "Watchdog is started at boot time if set to 1, default="
> +                __MODULE_STRING(STARFIVE_WDT_EARLY_ENA));
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +                __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct starfive_wdt_variant {
> +       u32 control;
> +       u32 load;
> +       u32 enable;
> +       u32 value;
> +       u32 int_clr;
> +       u32 unlock;

These are register offsets and not the values of registers, so could
be just unsigned int.

> +       u32 unlock_key;
> +       u32 irq_is_raise;
> +       u8 enrst_shift;
> +       u8 en_shift;
> +};
> +
> +struct starfive_wdt {
> +       unsigned long freq;
> +       struct device *dev;
> +       struct watchdog_device wdt_device;
> +       struct clk *core_clk;
> +       struct clk *apb_clk;
> +       struct reset_control *rsts;
> +       const struct starfive_wdt_variant *drv_data;
> +       u32 count;      /*count of timeout*/
> +       u32 reload;     /*restore the count*/
> +       void __iomem *base;
> +       spinlock_t lock;        /* spinlock for register handling */
> +};
> +
> +/* Register bias in JH7110 */

bias? Maybe "register layout for the JH7110".

> +static const struct starfive_wdt_variant drv_data_jh7110 = {
> +       .control = STARFIVE_WDT_JH7110_CONTROL,
> +       .load = STARFIVE_WDT_JH7110_LOAD,
> +       .enable = STARFIVE_WDT_JH7110_CONTROL,
> +       .value = STARFIVE_WDT_JH7110_VALUE,
> +       .int_clr = STARFIVE_WDT_JH7110_INTCLR,
> +       .unlock = STARFIVE_WDT_JH7110_LOCK,
> +       .unlock_key = STARFIVE_WDT_JH7110_UNLOCK_KEY,
> +       .irq_is_raise = STARFIVE_WDT_JH7110_IMS,
> +       .enrst_shift = STARFIVE_WDT_JH7110_RESEN_SHIFT,
> +       .en_shift = STARFIVE_WDT_JH7110_EN_SHIFT,
> +};
> +
> +static const struct of_device_id starfive_wdt_match[] = {
> +       { .compatible = "starfive,jh7110-wdt", .data = &drv_data_jh7110 },
> +       {}

I like the idiom of ending with { /* sentinel */ } here. Also it is
common to place this struct just above the platform_driver struct
which is the only user.

> +};
> +MODULE_DEVICE_TABLE(of, starfive_wdt_match);
> +
> +static const struct platform_device_id starfive_wdt_ids[] = {
> +       {
> +               .name = "starfive-jh7110-wdt",
> +               .driver_data = (unsigned long)&drv_data_jh7110,
> +       },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(platform, starfive_wdt_ids);

When is this struct used? So far I only know of device tree enabled users.

> +static int starfive_wdt_get_clock_rate(struct starfive_wdt *wdt)
> +{
> +       wdt->freq = clk_get_rate(wdt->core_clk);
> +       /* The clock rate should not be 0.*/
> +       if (wdt->freq)
> +               return 0;
> +
> +       dev_err(wdt->dev, "get clock rate failed.\n");
> +       return -ENOENT;
> +}
> +
> +static int starfive_wdt_get_clock(struct starfive_wdt *wdt)
> +{
> +       wdt->apb_clk = devm_clk_get(wdt->dev, "apb");
> +       if (IS_ERR(wdt->apb_clk)) {
> +               dev_err(wdt->dev, "failed to get apb clock.\n");
> +               return PTR_ERR(wdt->apb_clk);
> +       }
> +
> +       wdt->core_clk = devm_clk_get(wdt->dev, "core");
> +       if (IS_ERR(wdt->core_clk)) {
> +               dev_err(wdt->dev, "failed to get core clock.\n");
> +               return PTR_ERR(wdt->core_clk);
> +       }

Here and above you can use dev_err_probe which will also not print
errors on -EPROBE_DEFER.

> +       return 0;
> +}
> +
> +static int starfive_wdt_reset_init(struct starfive_wdt *wdt)
> +{
> +       int ret = 0;
> +
> +       wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
> +       if (IS_ERR(wdt->rsts)) {
> +               dev_err(wdt->dev, "failed to get rsts error.\n");
> +               ret = PTR_ERR(wdt->rsts);
> +       } else {
> +               ret = reset_control_deassert(wdt->rsts);
> +               if (ret)
> +                       dev_err(wdt->dev, "failed to deassert rsts.\n");
> +       }
> +       return ret;
> +}

How about something like
int ret;

wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
if (IS_ERR(wdt->rsts))
    return dev_err_probe(wdt->dev, PTR_ERR(wdt->rsts), "failed to get
resets\n");

ret = reset_control_deassert(wdt->rsts);
return dev_err_probe(wdt->dev, ret, "failed to deassert resets\n");

> +
> +static u32 starfive_wdt_ticks_to_sec(struct starfive_wdt *wdt, u32 ticks)
> +{
> +       return DIV_ROUND_CLOSEST(ticks, wdt->freq);
> +}
> +
> +/*
> + * Write unlock-key to unlock. Write other value to lock. When lock bit is 1,
> + * external accesses to other watchdog registers are ignored.
> + */
> +static bool starfive_wdt_is_locked(struct starfive_wdt *wdt)
> +{
> +       u32 val;
> +
> +       val = readl(wdt->base + wdt->drv_data->unlock);
> +       return !!(val & STARFIVE_WDT_LOCKED);
> +}
> +
> +static void starfive_wdt_unlock(struct starfive_wdt *wdt)
> +{
> +       if (starfive_wdt_is_locked(wdt))
> +               writel(wdt->drv_data->unlock_key,
> +                      wdt->base + wdt->drv_data->unlock);
> +}
> +
> +static void starfive_wdt_lock(struct starfive_wdt *wdt)
> +{
> +       if (!starfive_wdt_is_locked(wdt))
> +               writel(~wdt->drv_data->unlock_key,
> +                      wdt->base + wdt->drv_data->unlock);
> +}
> +
> +/* enable watchdog interrupt to reset/reboot */
> +static void starfive_wdt_enable_reset(struct starfive_wdt *wdt)
> +{
> +       u32 val;
> +
> +       val = readl(wdt->base + wdt->drv_data->control);
> +       val |= STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift;
> +       writel(val, wdt->base + wdt->drv_data->control);
> +}
> +
> +/* disable watchdog interrupt to reset/reboot */
> +static void starfive_wdt_disable_reset(struct starfive_wdt *wdt)
> +{
> +       u32 val;
> +
> +       val = readl(wdt->base + wdt->drv_data->control);
> +       val &= ~(STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift);
> +       writel(val, wdt->base + wdt->drv_data->control);
> +}
> +
> +/* interrupt status whether has been raised from the counter */
> +static bool starfive_wdt_raise_irq_status(struct starfive_wdt *wdt)
> +{
> +       return !!readl(wdt->base + wdt->drv_data->irq_is_raise);
> +}
> +
> +/* clear interrupt signal before initialization or reload */
> +static void starfive_wdt_int_clr(struct starfive_wdt *wdt)
> +{
> +       writel(STARFIVE_WDT_INTCLR, wdt->base + wdt->drv_data->int_clr);
> +}
> +
> +static inline void starfive_wdt_set_count(struct starfive_wdt *wdt, u32 val)
> +{
> +       writel(val, wdt->base + wdt->drv_data->load);
> +}
> +
> +static inline u32 starfive_wdt_get_count(struct starfive_wdt *wdt)
> +{
> +       return readl(wdt->base + wdt->drv_data->value);
> +}
> +
> +/* enable watchdog */
> +static inline void starfive_wdt_enable(struct starfive_wdt *wdt)
> +{
> +       u32 val;
> +
> +       val = readl(wdt->base + wdt->drv_data->enable);
> +       val |= STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift;
> +       writel(val, wdt->base + wdt->drv_data->enable);
> +}
> +
> +/* disable watchdog */
> +static inline void starfive_wdt_disable(struct starfive_wdt *wdt)
> +{
> +       u32 val;
> +
> +       val = readl(wdt->base + wdt->drv_data->enable);
> +       val &= ~(STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift);
> +       writel(val, wdt->base + wdt->drv_data->enable);
> +}
> +
> +static inline void starfive_wdt_set_reload_count(struct starfive_wdt *wdt, u32 count)
> +{
> +       starfive_wdt_set_count(wdt, count);
> +       /* need enable controller to reload counter */
> +       starfive_wdt_enable(wdt);
> +}
> +
> +static unsigned int starfive_wdt_max_timeout(struct starfive_wdt *wdt)
> +{
> +       return DIV_ROUND_UP(STARFIVE_WDT_MAXCNT, (wdt->freq / 2)) - 1;
> +}
> +
> +static unsigned int starfive_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +       u32 count;
> +
> +       starfive_wdt_unlock(wdt);
> +       /*
> +        * Because set half count value,
> +        * timeleft value should add the count value before first timeout.
> +        */
> +       count = starfive_wdt_get_count(wdt);
> +       if (!starfive_wdt_raise_irq_status(wdt))
> +               count += wdt->count;
> +
> +       starfive_wdt_lock(wdt);
> +
> +       return starfive_wdt_ticks_to_sec(wdt, count);
> +}
> +
> +static int starfive_wdt_keepalive(struct watchdog_device *wdd)
> +{
> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +       spin_lock(&wdt->lock);
> +
> +       starfive_wdt_unlock(wdt);
> +       starfive_wdt_int_clr(wdt);
> +       starfive_wdt_set_reload_count(wdt, wdt->count);
> +       starfive_wdt_lock(wdt);
> +
> +       spin_unlock(&wdt->lock);
> +
> +       return 0;
> +}
> +
> +static int starfive_wdt_stop(struct watchdog_device *wdd)
> +{
> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +       spin_lock(&wdt->lock);
> +
> +       starfive_wdt_unlock(wdt);
> +       starfive_wdt_disable_reset(wdt);
> +       starfive_wdt_int_clr(wdt);
> +       starfive_wdt_disable(wdt);
> +       starfive_wdt_lock(wdt);
> +
> +       spin_unlock(&wdt->lock);
> +
> +       return 0;
> +}
> +
> +static int starfive_wdt_pm_stop(struct watchdog_device *wdd)
> +{
> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +       starfive_wdt_stop(wdd);
> +       pm_runtime_put_sync(wdt->dev);
> +
> +       return 0;
> +}
> +
> +static int starfive_wdt_start(struct watchdog_device *wdd)
> +{
> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +       spin_lock(&wdt->lock);
> +       starfive_wdt_unlock(wdt);
> +       /* disable watchdog, to be safe */
> +       starfive_wdt_disable(wdt);
> +
> +       starfive_wdt_enable_reset(wdt);
> +       starfive_wdt_int_clr(wdt);
> +       starfive_wdt_set_count(wdt, wdt->count);
> +       starfive_wdt_enable(wdt);
> +
> +       starfive_wdt_lock(wdt);
> +       spin_unlock(&wdt->lock);
> +
> +       return 0;
> +}
> +
> +static int starfive_wdt_pm_start(struct watchdog_device *wdd)
> +{
> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +       pm_runtime_get_sync(wdt->dev);
> +
> +       return starfive_wdt_start(wdd);
> +}
> +
> +static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
> +                                   unsigned int timeout)
> +{
> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +       unsigned long freq = wdt->freq;
> +
> +       spin_lock(&wdt->lock);
> +
> +       /*
> +        * This watchdog takes twice timeouts to reset.
> +        * In order to reduce time to reset, should set half count value.
> +        */
> +       wdt->count = timeout * freq / 2;
> +       wdd->timeout = timeout;
> +
> +       starfive_wdt_unlock(wdt);
> +       starfive_wdt_disable(wdt);
> +       starfive_wdt_set_reload_count(wdt, wdt->count);
> +       starfive_wdt_enable(wdt);
> +       starfive_wdt_lock(wdt);
> +
> +       spin_unlock(&wdt->lock);
> +
> +       return 0;
> +}
> +
> +#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
> +
> +static const struct watchdog_info starfive_wdt_ident = {
> +       .options = OPTIONS,
> +       .identity = "StarFive Watchdog",
> +};
> +
> +static const struct watchdog_ops starfive_wdt_ops = {
> +       .owner = THIS_MODULE,
> +       .start = starfive_wdt_pm_start,
> +       .stop = starfive_wdt_pm_stop,
> +       .ping = starfive_wdt_keepalive,
> +       .set_timeout = starfive_wdt_set_timeout,
> +       .get_timeleft = starfive_wdt_get_timeleft,
> +};
> +
> +static const struct watchdog_device starfive_wdd = {
> +       .info = &starfive_wdt_ident,
> +       .ops = &starfive_wdt_ops,
> +       .timeout = STARFIVE_WDT_DEFAULT_TIME,
> +};
> +
> +static inline const struct starfive_wdt_variant *
> +starfive_wdt_get_drv_data(struct platform_device *pdev)
> +{
> +       const struct starfive_wdt_variant *variant;
> +
> +       variant = of_device_get_match_data(&pdev->dev);
> +       if (!variant) {
> +               /* Device matched by platform_device_id */
> +               variant = (struct starfive_wdt_variant *)
> +                          platform_get_device_id(pdev)->driver_data;
> +       }
> +
> +       return variant;
> +}

If this driver is only used through the device tree this whole
function can just be a call to of_device_get_match_data()

> +static int starfive_wdt_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct starfive_wdt *wdt;
> +       int ret;
> +
> +       wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> +       if (!wdt)
> +               return -ENOMEM;
> +
> +       wdt->dev = dev;
> +       spin_lock_init(&wdt->lock);
> +       wdt->wdt_device = starfive_wdd;
> +
> +       wdt->drv_data = starfive_wdt_get_drv_data(pdev);
> +
> +       /* get the memory region for the watchdog timer */
> +       wdt->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(wdt->base)) {
> +               ret = PTR_ERR(wdt->base);
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, wdt);
> +       pm_runtime_enable(wdt->dev);
> +
> +       ret = starfive_wdt_get_clock(wdt);
> +       if (ret)
> +               return ret;
> +
> +       if (pm_runtime_enabled(wdt->dev)) {
> +               ret = pm_runtime_get_sync(wdt->dev);
> +               if (ret < 0)
> +                       return ret;
> +       } else {
> +               /* runtime PM is disabled but clocks need to be enabled */
> +               ret = clk_prepare_enable(wdt->apb_clk);
> +               if (ret) {
> +                       dev_err(wdt->dev, "failed to enable apb_clk.\n");
> +                       return ret;
> +               }
> +               ret = clk_prepare_enable(wdt->core_clk);
> +               if (ret) {
> +                       dev_err(wdt->dev, "failed to enable core_clk.\n");
> +                       goto err_apb_clk_disable;
> +               }
> +       }

The else part is basically starfive_wdt_runtime_resume(). Maybe just call that.


> +       ret = starfive_wdt_get_clock_rate(wdt);
> +       if (ret)
> +               goto err_clk_disable;
> +
> +       ret = starfive_wdt_reset_init(wdt);
> +       if (ret)
> +               goto err_clk_disable;
> +
> +       wdt->wdt_device.min_timeout = 1;
> +       wdt->wdt_device.max_timeout = starfive_wdt_max_timeout(wdt);
> +
> +       watchdog_set_drvdata(&wdt->wdt_device, wdt);
> +
> +       /*
> +        * see if we can actually set the requested heartbeat,
> +        * and if not, try the default value.
> +        */
> +       watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
> +       if (wdt->wdt_device.timeout == 0 ||
> +           wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
> +               dev_warn(dev, "heartbeat value out of range, default %d used\n",
> +                        STARFIVE_WDT_DEFAULT_TIME);
> +               wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
> +       }
> +       starfive_wdt_set_timeout(&wdt->wdt_device, wdt->wdt_device.timeout);
> +
> +       watchdog_set_nowayout(&wdt->wdt_device, nowayout);
> +       watchdog_stop_on_reboot(&wdt->wdt_device);
> +       watchdog_stop_on_unregister(&wdt->wdt_device);
> +
> +       wdt->wdt_device.parent = dev;
> +
> +       ret = watchdog_register_device(&wdt->wdt_device);
> +       if (ret)
> +               goto err_clk_disable;
> +
> +       if (early_enable) {
> +               starfive_wdt_start(&wdt->wdt_device);
> +               set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> +       } else {
> +               starfive_wdt_stop(&wdt->wdt_device);
> +       }
> +
> +       pm_runtime_put_sync(wdt->dev);
> +
> +       return 0;
> +
> +err_clk_disable:
> +       clk_disable_unprepare(wdt->core_clk);
> +err_apb_clk_disable:
> +       clk_disable_unprepare(wdt->apb_clk);
> +       pm_runtime_disable(wdt->dev);
> +
> +       return ret;
> +}
> +
> +static int starfive_wdt_remove(struct platform_device *dev)
> +{
> +       struct starfive_wdt *wdt = platform_get_drvdata(dev);
> +
> +       starfive_wdt_stop(&wdt->wdt_device);
> +       watchdog_unregister_device(&wdt->wdt_device);
> +
> +       if (pm_runtime_enabled(wdt->dev)) {
> +               pm_runtime_disable(wdt->dev);
> +       } else {
> +               /* disable clock without PM */
> +               clk_disable_unprepare(wdt->core_clk);
> +               clk_disable_unprepare(wdt->apb_clk);
> +       }
> +
> +       return 0;
> +}
> +
> +static void starfive_wdt_shutdown(struct platform_device *dev)
> +{
> +       struct starfive_wdt *wdt = platform_get_drvdata(dev);
> +
> +       starfive_wdt_pm_stop(&wdt->wdt_device);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int starfive_wdt_suspend(struct device *dev)
> +{
> +       int ret;
> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
> +
> +       starfive_wdt_unlock(wdt);
> +
> +       /* Save watchdog state, and turn it off. */
> +       wdt->reload = starfive_wdt_get_count(wdt);
> +
> +       /* Note that WTCNT doesn't need to be saved. */
> +       starfive_wdt_stop(&wdt->wdt_device);
> +       pm_runtime_force_suspend(dev);
> +
> +       starfive_wdt_lock(wdt);
> +
> +       return 0;
> +}
> +
> +static int starfive_wdt_resume(struct device *dev)
> +{
> +       int ret;
> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
> +
> +       starfive_wdt_unlock(wdt);
> +
> +       pm_runtime_force_resume(dev);
> +
> +       /* Restore watchdog state. */
> +       starfive_wdt_set_reload_count(wdt, wdt->reload);
> +
> +       starfive_wdt_start(&wdt->wdt_device);
> +
> +       starfive_wdt_lock(wdt);
> +
> +       return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +#ifdef CONFIG_PM
> +static int starfive_wdt_runtime_suspend(struct device *dev)
> +{
> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
> +
> +       clk_disable_unprepare(wdt->apb_clk);
> +       clk_disable_unprepare(wdt->core_clk);
> +
> +       return 0;
> +}
> +
> +static int starfive_wdt_runtime_resume(struct device *dev)
> +{
> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = clk_prepare_enable(wdt->apb_clk);
> +       if (ret) {
> +               dev_err(wdt->dev, "failed to enable apb_clk.\n");
> +               return ret;
> +       }
> +
> +       ret = clk_prepare_enable(wdt->core_clk);
> +       if (ret)
> +               dev_err(wdt->dev, "failed to enable core_clk.\n");
> +
> +       return ret;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops starfive_wdt_pm_ops = {
> +       SET_RUNTIME_PM_OPS(starfive_wdt_runtime_suspend, starfive_wdt_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(starfive_wdt_suspend, starfive_wdt_resume)
> +};
> +
> +static struct platform_driver starfive_wdt_driver = {
> +       .probe          = starfive_wdt_probe,
> +       .remove         = starfive_wdt_remove,
> +       .shutdown       = starfive_wdt_shutdown,
> +       .id_table       = starfive_wdt_ids,
> +       .driver         = {
> +               .name   = "starfive-wdt",
> +               .pm     = &starfive_wdt_pm_ops,
> +               .of_match_table = of_match_ptr(starfive_wdt_match),
> +       },
> +};
> +
> +module_platform_driver(starfive_wdt_driver);
> +
> +MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
> +MODULE_AUTHOR("Samin Guo <samin.guo@starfivetech.com>");
> +MODULE_DESCRIPTION("StarFive Watchdog Device Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Conor Dooley Feb. 26, 2023, 2:33 p.m. UTC | #5

On 26 February 2023 14:14:25 GMT, Emil Renner Berthing <emil.renner.berthing@canonical.com> wrote:
>On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>>
>> Add watchdog driver for the StarFive JH7110 SoC.
>>
>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> ---
>>  MAINTAINERS                     |   7 +
>>  drivers/watchdog/Kconfig        |   9 +
>>  drivers/watchdog/Makefile       |   2 +
>>  drivers/watchdog/starfive-wdt.c | 651 ++++++++++++++++++++++++++++++++
>>  4 files changed, 669 insertions(+)
>>  create mode 100644 drivers/watchdog/starfive-wdt.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 135d93368d36..6cbcf08fa76a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19933,6 +19933,13 @@ F:     Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>>  F:     drivers/reset/reset-starfive-jh7100.c
>>  F:     include/dt-bindings/reset/starfive-jh7100.h
>>
>> +STARFIVE JH7110 WATCHDOG DRIVER
>> +M:     Xingyu Wu <xingyu.wu@starfivetech.com>
>> +M:     Samin Guo <samin.guo@starfivetech.com>
>> +S:     Supported
>> +F:     Documentation/devicetree/bindings/watchdog/starfive*
>> +F:     drivers/watchdog/starfive-wdt.c
>> +
>>  STATIC BRANCH/CALL
>>  M:     Peter Zijlstra <peterz@infradead.org>
>>  M:     Josh Poimboeuf <jpoimboe@kernel.org>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 0bc40b763b06..4608eb5c9501 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -2089,6 +2089,15 @@ config UML_WATCHDOG
>>         tristate "UML watchdog"
>>         depends on UML || COMPILE_TEST
>>
>> +config STARFIVE_WATCHDOG
>> +       tristate "StarFive Watchdog support"
>> +       depends on RISCV
>
>Let's do like the pinctrl and clock drivers and
>
>    depends SOC_STARFIVE || COMPILE_TEST

Or better yet, rebase on 6.3-rc1, and use ARCH_STARFIVE and save me a conversion!

Thanks,
Conor.

Xingyu Wu Feb. 27, 2023, 1:47 a.m. UTC | #6

On 2023/2/26 22:33, Conor Dooley wrote:
> 
> 
> On 26 February 2023 14:14:25 GMT, Emil Renner Berthing <emil.renner.berthing@canonical.com> wrote:
>>On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>>>
>>> Add watchdog driver for the StarFive JH7110 SoC.
>>>
>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>> ---
>>>  MAINTAINERS                     |   7 +
>>>  drivers/watchdog/Kconfig        |   9 +
>>>  drivers/watchdog/Makefile       |   2 +
>>>  drivers/watchdog/starfive-wdt.c | 651 ++++++++++++++++++++++++++++++++
>>>  4 files changed, 669 insertions(+)
>>>  create mode 100644 drivers/watchdog/starfive-wdt.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 135d93368d36..6cbcf08fa76a 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -19933,6 +19933,13 @@ F:     Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>>>  F:     drivers/reset/reset-starfive-jh7100.c
>>>  F:     include/dt-bindings/reset/starfive-jh7100.h
>>>
>>> +STARFIVE JH7110 WATCHDOG DRIVER
>>> +M:     Xingyu Wu <xingyu.wu@starfivetech.com>
>>> +M:     Samin Guo <samin.guo@starfivetech.com>
>>> +S:     Supported
>>> +F:     Documentation/devicetree/bindings/watchdog/starfive*
>>> +F:     drivers/watchdog/starfive-wdt.c
>>> +
>>>  STATIC BRANCH/CALL
>>>  M:     Peter Zijlstra <peterz@infradead.org>
>>>  M:     Josh Poimboeuf <jpoimboe@kernel.org>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index 0bc40b763b06..4608eb5c9501 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -2089,6 +2089,15 @@ config UML_WATCHDOG
>>>         tristate "UML watchdog"
>>>         depends on UML || COMPILE_TEST
>>>
>>> +config STARFIVE_WATCHDOG
>>> +       tristate "StarFive Watchdog support"
>>> +       depends on RISCV
>>
>>Let's do like the pinctrl and clock drivers and
>>
>>    depends SOC_STARFIVE || COMPILE_TEST
> 
> Or better yet, rebase on 6.3-rc1, and use ARCH_STARFIVE and save me a conversion!
> 

Will modify this.

Best regards,
Xingyu Wu

Xingyu Wu Feb. 27, 2023, 6:26 a.m. UTC | #7

On 2023/2/24 23:18, Guenter Roeck wrote:
> On 2/23/23 23:42, Xingyu Wu wrote:
>> On 2023/2/24 2:23, Guenter Roeck wrote:
>>> On Mon, Feb 20, 2023 at 04:19:26PM +0800, Xingyu Wu wrote:
>>>> [...]
>>>> +
>>>> +    wdt->wdt_device.min_timeout = 1;
>>>> +    wdt->wdt_device.max_timeout = starfive_wdt_max_timeout(wdt);
>>>
>>>     wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>
>>> should be set here. Otherwise the warning below would always be seen
>>> if the module parameter is not set.
>>>
>>>> +
>>>> +    watchdog_set_drvdata(&wdt->wdt_device, wdt);
>>>> +
>>>> +    /*
>>>> +     * see if we can actually set the requested heartbeat,
>>>> +     * and if not, try the default value.
>>>> +     */
>>>> +    watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>>> +    if (wdt->wdt_device.timeout == 0 ||
>>>
>>> If wdt->wdt_device.timeout is pre-initialized, it will never be 0 here.
>>>
>>>> +        wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
>>>
>>> That won't happen because watchdog_init_timeout() validates it and does
>>> not update the value if it is out of range.
>>>
>>>> +        dev_warn(dev, "heartbeat value out of range, default %d used\n",
>>>> +             STARFIVE_WDT_DEFAULT_TIME);
>>>> +        wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>
>>> And this is then unnecessary. wdt->wdt_device.timeout will always be
>>> valid if it was pre-initialized.
>>
>> It is changed to be this at beginning of the driver:
>>
>> static int heartbeat = STARFIVE_WDT_DEFAULT_TIME;
>>
> 
> No, this is wrong. The static variable should be set to 0 to indicate
> "use default".
> 
>> and it is changed to be this here:
>>
>> ret = watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>> if (ret)
>>     return ret;
>>
>> Would that be better?
>>
> 
> No, it is worse, because it would not instantiate the watchdog at all
> if a bad heartbeat is provided.
> 

So instantiate the watchdog with hearbeat first. And if this wrong, use default timeout.
:
if (watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev))
	wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;


Best regards,
Xingyu Wu

Guenter Roeck Feb. 27, 2023, 6:36 a.m. UTC | #8

On 2/26/23 22:26, Xingyu Wu wrote:
> On 2023/2/24 23:18, Guenter Roeck wrote:
>> On 2/23/23 23:42, Xingyu Wu wrote:
>>> On 2023/2/24 2:23, Guenter Roeck wrote:
>>>> On Mon, Feb 20, 2023 at 04:19:26PM +0800, Xingyu Wu wrote:
>>>>> [...]
>>>>> +
>>>>> +    wdt->wdt_device.min_timeout = 1;
>>>>> +    wdt->wdt_device.max_timeout = starfive_wdt_max_timeout(wdt);
>>>>
>>>>      wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>>
>>>> should be set here. Otherwise the warning below would always be seen
>>>> if the module parameter is not set.
>>>>
>>>>> +
>>>>> +    watchdog_set_drvdata(&wdt->wdt_device, wdt);
>>>>> +
>>>>> +    /*
>>>>> +     * see if we can actually set the requested heartbeat,
>>>>> +     * and if not, try the default value.
>>>>> +     */
>>>>> +    watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>>>> +    if (wdt->wdt_device.timeout == 0 ||
>>>>
>>>> If wdt->wdt_device.timeout is pre-initialized, it will never be 0 here.
>>>>
>>>>> +        wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
>>>>
>>>> That won't happen because watchdog_init_timeout() validates it and does
>>>> not update the value if it is out of range.
>>>>
>>>>> +        dev_warn(dev, "heartbeat value out of range, default %d used\n",
>>>>> +             STARFIVE_WDT_DEFAULT_TIME);
>>>>> +        wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>>
>>>> And this is then unnecessary. wdt->wdt_device.timeout will always be
>>>> valid if it was pre-initialized.
>>>
>>> It is changed to be this at beginning of the driver:
>>>
>>> static int heartbeat = STARFIVE_WDT_DEFAULT_TIME;
>>>
>>
>> No, this is wrong. The static variable should be set to 0 to indicate
>> "use default".
>>
>>> and it is changed to be this here:
>>>
>>> ret = watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>> if (ret)
>>>      return ret;
>>>
>>> Would that be better?
>>>
>>
>> No, it is worse, because it would not instantiate the watchdog at all
>> if a bad heartbeat is provided.
>>
> 
> So instantiate the watchdog with hearbeat first. And if this wrong, use default timeout.
> :
> if (watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev))
> 	wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
> 

I am kind of lost why you have to make it that complicated.
Just pre-initialize wdt->wdt_device.timeout like all the other drivers do,
and as I had suggested earlier.

Guenter

Xingyu Wu Feb. 27, 2023, 6:45 a.m. UTC | #9

On 2023/2/27 14:36, Guenter Roeck wrote:
> On 2/26/23 22:26, Xingyu Wu wrote:
>> On 2023/2/24 23:18, Guenter Roeck wrote:
>>> On 2/23/23 23:42, Xingyu Wu wrote:
>>>> On 2023/2/24 2:23, Guenter Roeck wrote:
>>>>> On Mon, Feb 20, 2023 at 04:19:26PM +0800, Xingyu Wu wrote:
>>>>>> [...]
>>>>>> +
>>>>>> +    wdt->wdt_device.min_timeout = 1;
>>>>>> +    wdt->wdt_device.max_timeout = starfive_wdt_max_timeout(wdt);
>>>>>
>>>>>      wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>>>
>>>>> should be set here. Otherwise the warning below would always be seen
>>>>> if the module parameter is not set.
>>>>>
>>>>>> +
>>>>>> +    watchdog_set_drvdata(&wdt->wdt_device, wdt);
>>>>>> +
>>>>>> +    /*
>>>>>> +     * see if we can actually set the requested heartbeat,
>>>>>> +     * and if not, try the default value.
>>>>>> +     */
>>>>>> +    watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>>>>> +    if (wdt->wdt_device.timeout == 0 ||
>>>>>
>>>>> If wdt->wdt_device.timeout is pre-initialized, it will never be 0 here.
>>>>>
>>>>>> +        wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
>>>>>
>>>>> That won't happen because watchdog_init_timeout() validates it and does
>>>>> not update the value if it is out of range.
>>>>>
>>>>>> +        dev_warn(dev, "heartbeat value out of range, default %d used\n",
>>>>>> +             STARFIVE_WDT_DEFAULT_TIME);
>>>>>> +        wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>>>
>>>>> And this is then unnecessary. wdt->wdt_device.timeout will always be
>>>>> valid if it was pre-initialized.
>>>>
>>>> It is changed to be this at beginning of the driver:
>>>>
>>>> static int heartbeat = STARFIVE_WDT_DEFAULT_TIME;
>>>>
>>>
>>> No, this is wrong. The static variable should be set to 0 to indicate
>>> "use default".
>>>
>>>> and it is changed to be this here:
>>>>
>>>> ret = watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>>> if (ret)
>>>>      return ret;
>>>>
>>>> Would that be better?
>>>>
>>>
>>> No, it is worse, because it would not instantiate the watchdog at all
>>> if a bad heartbeat is provided.
>>>
>>
>> So instantiate the watchdog with hearbeat first. And if this wrong, use default timeout.
>> :
>> if (watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev))
>>     wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>
> 
> I am kind of lost why you have to make it that complicated.
> Just pre-initialize wdt->wdt_device.timeout like all the other drivers do,
> and as I had suggested earlier.
> 

So you mean just use :
wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
to initialize watchdog directly?

Best regards,
Xingyu Wu

Guenter Roeck Feb. 27, 2023, 2:50 p.m. UTC | #10

On 2/26/23 22:45, Xingyu Wu wrote:
> On 2023/2/27 14:36, Guenter Roeck wrote:
>> On 2/26/23 22:26, Xingyu Wu wrote:
>>> On 2023/2/24 23:18, Guenter Roeck wrote:
>>>> On 2/23/23 23:42, Xingyu Wu wrote:
>>>>> On 2023/2/24 2:23, Guenter Roeck wrote:
>>>>>> On Mon, Feb 20, 2023 at 04:19:26PM +0800, Xingyu Wu wrote:
>>>>>>> [...]
>>>>>>> +
>>>>>>> +    wdt->wdt_device.min_timeout = 1;
>>>>>>> +    wdt->wdt_device.max_timeout = starfive_wdt_max_timeout(wdt);
>>>>>>
>>>>>>       wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>>>>
>>>>>> should be set here. Otherwise the warning below would always be seen
>>>>>> if the module parameter is not set.
>>>>>>
>>>>>>> +
>>>>>>> +    watchdog_set_drvdata(&wdt->wdt_device, wdt);
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * see if we can actually set the requested heartbeat,
>>>>>>> +     * and if not, try the default value.
>>>>>>> +     */
>>>>>>> +    watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>>>>>> +    if (wdt->wdt_device.timeout == 0 ||
>>>>>>
>>>>>> If wdt->wdt_device.timeout is pre-initialized, it will never be 0 here.
>>>>>>
>>>>>>> +        wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
>>>>>>
>>>>>> That won't happen because watchdog_init_timeout() validates it and does
>>>>>> not update the value if it is out of range.
>>>>>>
>>>>>>> +        dev_warn(dev, "heartbeat value out of range, default %d used\n",
>>>>>>> +             STARFIVE_WDT_DEFAULT_TIME);
>>>>>>> +        wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>>>>
>>>>>> And this is then unnecessary. wdt->wdt_device.timeout will always be
>>>>>> valid if it was pre-initialized.
>>>>>
>>>>> It is changed to be this at beginning of the driver:
>>>>>
>>>>> static int heartbeat = STARFIVE_WDT_DEFAULT_TIME;
>>>>>
>>>>
>>>> No, this is wrong. The static variable should be set to 0 to indicate
>>>> "use default".
>>>>
>>>>> and it is changed to be this here:
>>>>>
>>>>> ret = watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>>>> if (ret)
>>>>>       return ret;
>>>>>
>>>>> Would that be better?
>>>>>
>>>>
>>>> No, it is worse, because it would not instantiate the watchdog at all
>>>> if a bad heartbeat is provided.
>>>>
>>>
>>> So instantiate the watchdog with hearbeat first. And if this wrong, use default timeout.
>>> :
>>> if (watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev))
>>>      wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>
>>
>> I am kind of lost why you have to make it that complicated.
>> Just pre-initialize wdt->wdt_device.timeout like all the other drivers do,
>> and as I had suggested earlier.
>>
> 
> So you mean just use :
> wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
> to initialize watchdog directly?
> 

Yes, as I had suggested before, before calling watchdog_init_timeout().

Guenter

> Best regards,
> Xingyu Wu

Xingyu Wu Feb. 28, 2023, 2:07 a.m. UTC | #11

On 2023/2/27 22:50, Guenter Roeck wrote:
> On 2/26/23 22:45, Xingyu Wu wrote:
>> On 2023/2/27 14:36, Guenter Roeck wrote:
>>> On 2/26/23 22:26, Xingyu Wu wrote:
>>>> On 2023/2/24 23:18, Guenter Roeck wrote:
>>>>> On 2/23/23 23:42, Xingyu Wu wrote:
>>>>>> On 2023/2/24 2:23, Guenter Roeck wrote:
>>>>>>> On Mon, Feb 20, 2023 at 04:19:26PM +0800, Xingyu Wu wrote:
>>>>>>>> [...]
>>>>>>>> +
>>>>>>>> +    wdt->wdt_device.min_timeout = 1;
>>>>>>>> +    wdt->wdt_device.max_timeout = starfive_wdt_max_timeout(wdt);
>>>>>>>
>>>>>>>       wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>>>>>
>>>>>>> should be set here. Otherwise the warning below would always be seen
>>>>>>> if the module parameter is not set.
>>>>>>>
>>>>>>>> +
>>>>>>>> +    watchdog_set_drvdata(&wdt->wdt_device, wdt);
>>>>>>>> +
>>>>>>>> +    /*
>>>>>>>> +     * see if we can actually set the requested heartbeat,
>>>>>>>> +     * and if not, try the default value.
>>>>>>>> +     */
>>>>>>>> +    watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>>>>>>> +    if (wdt->wdt_device.timeout == 0 ||
>>>>>>>
>>>>>>> If wdt->wdt_device.timeout is pre-initialized, it will never be 0 here.
>>>>>>>
>>>>>>>> +        wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
>>>>>>>
>>>>>>> That won't happen because watchdog_init_timeout() validates it and does
>>>>>>> not update the value if it is out of range.
>>>>>>>
>>>>>>>> +        dev_warn(dev, "heartbeat value out of range, default %d used\n",
>>>>>>>> +             STARFIVE_WDT_DEFAULT_TIME);
>>>>>>>> +        wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>>>>>
>>>>>>> And this is then unnecessary. wdt->wdt_device.timeout will always be
>>>>>>> valid if it was pre-initialized.
>>>>>>
>>>>>> It is changed to be this at beginning of the driver:
>>>>>>
>>>>>> static int heartbeat = STARFIVE_WDT_DEFAULT_TIME;
>>>>>>
>>>>>
>>>>> No, this is wrong. The static variable should be set to 0 to indicate
>>>>> "use default".
>>>>>
>>>>>> and it is changed to be this here:
>>>>>>
>>>>>> ret = watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>>>>> if (ret)
>>>>>>       return ret;
>>>>>>
>>>>>> Would that be better?
>>>>>>
>>>>>
>>>>> No, it is worse, because it would not instantiate the watchdog at all
>>>>> if a bad heartbeat is provided.
>>>>>
>>>>
>>>> So instantiate the watchdog with hearbeat first. And if this wrong, use default timeout.
>>>> :
>>>> if (watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev))
>>>>      wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>>
>>>
>>> I am kind of lost why you have to make it that complicated.
>>> Just pre-initialize wdt->wdt_device.timeout like all the other drivers do,
>>> and as I had suggested earlier.
>>>
>>
>> So you mean just use :
>> wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>> to initialize watchdog directly?
>>
> 
> Yes, as I had suggested before, before calling watchdog_init_timeout().
> 

OK, thanks.
Best regard,
Xingyu Wu

Xingyu Wu Feb. 28, 2023, 9:44 a.m. UTC | #12

On 2023/2/26 22:14, Emil Renner Berthing wrote:
> On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>>
>> Add watchdog driver for the StarFive JH7110 SoC.
>>
>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> ---
>>  MAINTAINERS                     |   7 +
>>  drivers/watchdog/Kconfig        |   9 +
>>  drivers/watchdog/Makefile       |   2 +
>>  drivers/watchdog/starfive-wdt.c | 651 ++++++++++++++++++++++++++++++++
>>  4 files changed, 669 insertions(+)
>>  create mode 100644 drivers/watchdog/starfive-wdt.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 135d93368d36..6cbcf08fa76a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19933,6 +19933,13 @@ F:     Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>>  F:     drivers/reset/reset-starfive-jh7100.c
>>  F:     include/dt-bindings/reset/starfive-jh7100.h
>>
>> +STARFIVE JH7110 WATCHDOG DRIVER
>> +M:     Xingyu Wu <xingyu.wu@starfivetech.com>
>> +M:     Samin Guo <samin.guo@starfivetech.com>
>> +S:     Supported
>> +F:     Documentation/devicetree/bindings/watchdog/starfive*
>> +F:     drivers/watchdog/starfive-wdt.c
>> +
>>  STATIC BRANCH/CALL
>>  M:     Peter Zijlstra <peterz@infradead.org>
>>  M:     Josh Poimboeuf <jpoimboe@kernel.org>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 0bc40b763b06..4608eb5c9501 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -2089,6 +2089,15 @@ config UML_WATCHDOG
>>         tristate "UML watchdog"
>>         depends on UML || COMPILE_TEST
>>
>> +config STARFIVE_WATCHDOG
>> +       tristate "StarFive Watchdog support"
>> +       depends on RISCV
> 
> Let's do like the pinctrl and clock drivers and
> 
>     depends SOC_STARFIVE || COMPILE_TEST
> 

Yes, will use 'ARCH_STARFIVE' instead.

>> +       select WATCHDOG_CORE
>> +       default SOC_STARFIVE
>> +       help
>> +         Say Y here to support the watchdog of StarFive JH7110 SoC.
>> +         This driver can also be built as a module if choose M.
>> +
>>  #
>>  # ISA-based Watchdog Cards
>>  #
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 9cbf6580f16c..4c0bd377e92a 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -211,6 +211,8 @@ obj-$(CONFIG_WATCHDOG_SUN4V)                += sun4v_wdt.o
>>  # Xen
>>  obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>>
>> +obj-$(CONFIG_STARFIVE_WATCHDOG) += starfive-wdt.o
>> +
>>  # Architecture Independent
>>  obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
>>  obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
>> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
>> new file mode 100644
>> index 000000000000..dfbb80406076
>> --- /dev/null
>> +++ b/drivers/watchdog/starfive-wdt.c
>> @@ -0,0 +1,651 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Starfive Watchdog driver
>> + *
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>> +#include <linux/watchdog.h>
>> +
>> +/* JH7110 WatchDog register define */
>> +#define STARFIVE_WDT_JH7110_LOAD       0x000   /* RW: Watchdog load register */
>> +#define STARFIVE_WDT_JH7110_VALUE      0x004   /* RO: The current value for the watchdog counter */
>> +#define STARFIVE_WDT_JH7110_CONTROL    0x008   /*
>> +                                                * RW:
>> +                                                * [0]: reset enable;
>> +                                                * [1]: int enable/wdt enable/reload counter;
>> +                                                * [31:2]: reserve.
>> +                                                */
>> +#define STARFIVE_WDT_JH7110_INTCLR     0x00c   /* WO: clear intterupt && reload the counter */
>> +#define STARFIVE_WDT_JH7110_RIS                0x010   /* RO: Raw interrupt status from the counter */
>> +#define STARFIVE_WDT_JH7110_IMS                0x014   /* RO: Enabled interrupt status from the counter */
>> +#define STARFIVE_WDT_JH7110_LOCK       0xc00   /*
>> +                                                * RO: Enable write access to all other registers
>> +                                                * by writing 0x1ACCE551.
>> +                                                */
> 
> This driver is clearly prepared to support multiple SoCs, so hopefully
> you've tested that internally. It would be great if you could leave in
> the support for the JH7100 in the next revision. This would also make
> all the indirection make a lot more sense.

The JH7100 watchdog is little different from the 7110 and this driver only
needs to change and add some control register layout and mask to support the 7100.
And the JH8100 watchdog is the same as the 7110 but the 8100 is still in the plan.
So the dt-bingdings need to rename, and which one could be better,
'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?

> 
>> +/* WDOGCONTROL */
>> +#define STARFIVE_WDT_ENABLE                    0x1
>> +#define STARFIVE_WDT_JH7110_EN_SHIFT           0
>> +#define STARFIVE_WDT_RESET_EN                  0x1
>> +#define STARFIVE_WDT_JH7110_RESEN_SHIFT                1
>> +
>> +/* WDOGLOCK */
>> +#define STARFIVE_WDT_LOCKED                    BIT(0)
>> +#define STARFIVE_WDT_JH7110_UNLOCK_KEY         0x1acce551
>> +
>> +/* WDOGINTCLR */
>> +#define STARFIVE_WDT_INTCLR                    0x1
>> +
>> +#define STARFIVE_WDT_MAXCNT                    0xffffffff
>> +#define STARFIVE_WDT_DEFAULT_TIME              (15)
>> +#define STARFIVE_WDT_DELAY_US                  0
>> +#define STARFIVE_WDT_TIMEOUT_US                        10000
>> +
>> +/* module parameter */
>> +#define STARFIVE_WDT_EARLY_ENA                 0
>> +
>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>> +static int heartbeat;
>> +static int early_enable = STARFIVE_WDT_EARLY_ENA;
>> +
>> +module_param(heartbeat, int, 0);
>> +module_param(early_enable, int, 0);
> 
> This also looks like a bool parameter.

Oh will modify it.

> 
>> +module_param(nowayout, bool, 0);
>> +
>> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeat in seconds. (default="
>> +                __MODULE_STRING(STARFIVE_WDT_DEFAULT_TIME) ")");
>> +MODULE_PARM_DESC(early_enable,
>> +                "Watchdog is started at boot time if set to 1, default="
>> +                __MODULE_STRING(STARFIVE_WDT_EARLY_ENA));
>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>> +                __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>> +
>> +struct starfive_wdt_variant {
>> +       u32 control;
>> +       u32 load;
>> +       u32 enable;
>> +       u32 value;
>> +       u32 int_clr;
>> +       u32 unlock;
> 
> These are register offsets and not the values of registers, so could
> be just unsigned int.

Will modify it.

> 
>> +       u32 unlock_key;
>> +       u32 irq_is_raise;
>> +       u8 enrst_shift;
>> +       u8 en_shift;
>> +};
>> +
>> +struct starfive_wdt {
>> +       unsigned long freq;
>> +       struct device *dev;
>> +       struct watchdog_device wdt_device;
>> +       struct clk *core_clk;
>> +       struct clk *apb_clk;
>> +       struct reset_control *rsts;
>> +       const struct starfive_wdt_variant *drv_data;
>> +       u32 count;      /*count of timeout*/
>> +       u32 reload;     /*restore the count*/
>> +       void __iomem *base;
>> +       spinlock_t lock;        /* spinlock for register handling */
>> +};
>> +
>> +/* Register bias in JH7110 */
> 
> bias? Maybe "register layout for the JH7110".

Will use 'register layout' instead.

> 
>> +static const struct starfive_wdt_variant drv_data_jh7110 = {
>> +       .control = STARFIVE_WDT_JH7110_CONTROL,
>> +       .load = STARFIVE_WDT_JH7110_LOAD,
>> +       .enable = STARFIVE_WDT_JH7110_CONTROL,
>> +       .value = STARFIVE_WDT_JH7110_VALUE,
>> +       .int_clr = STARFIVE_WDT_JH7110_INTCLR,
>> +       .unlock = STARFIVE_WDT_JH7110_LOCK,
>> +       .unlock_key = STARFIVE_WDT_JH7110_UNLOCK_KEY,
>> +       .irq_is_raise = STARFIVE_WDT_JH7110_IMS,
>> +       .enrst_shift = STARFIVE_WDT_JH7110_RESEN_SHIFT,
>> +       .en_shift = STARFIVE_WDT_JH7110_EN_SHIFT,
>> +};
>> +
>> +static const struct of_device_id starfive_wdt_match[] = {
>> +       { .compatible = "starfive,jh7110-wdt", .data = &drv_data_jh7110 },
>> +       {}
> 
> I like the idiom of ending with { /* sentinel */ } here. Also it is
> common to place this struct just above the platform_driver struct
> which is the only user.

Will add it.

> 
>> +};
>> +MODULE_DEVICE_TABLE(of, starfive_wdt_match);
>> +
>> +static const struct platform_device_id starfive_wdt_ids[] = {
>> +       {
>> +               .name = "starfive-jh7110-wdt",
>> +               .driver_data = (unsigned long)&drv_data_jh7110,
>> +       },
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(platform, starfive_wdt_ids);
> 
> When is this struct used? So far I only know of device tree enabled users.

The struct will be used when CONFIG_OF is disabled
but I deleted CONFIG_OF by mistake.
Should I add the CONFIG_OF back or remove this struct?

> 
>> +static int starfive_wdt_get_clock_rate(struct starfive_wdt *wdt)
>> +{
>> +       wdt->freq = clk_get_rate(wdt->core_clk);
>> +       /* The clock rate should not be 0.*/
>> +       if (wdt->freq)
>> +               return 0;
>> +
>> +       dev_err(wdt->dev, "get clock rate failed.\n");
>> +       return -ENOENT;
>> +}
>> +
>> +static int starfive_wdt_get_clock(struct starfive_wdt *wdt)
>> +{
>> +       wdt->apb_clk = devm_clk_get(wdt->dev, "apb");
>> +       if (IS_ERR(wdt->apb_clk)) {
>> +               dev_err(wdt->dev, "failed to get apb clock.\n");
>> +               return PTR_ERR(wdt->apb_clk);
>> +       }
>> +
>> +       wdt->core_clk = devm_clk_get(wdt->dev, "core");
>> +       if (IS_ERR(wdt->core_clk)) {
>> +               dev_err(wdt->dev, "failed to get core clock.\n");
>> +               return PTR_ERR(wdt->core_clk);
>> +       }
> 
> Here and above you can use dev_err_probe which will also not print
> errors on -EPROBE_DEFER.

Will modify it.

> 
>> +       return 0;
>> +}
>> +
>> +static int starfive_wdt_reset_init(struct starfive_wdt *wdt)
>> +{
>> +       int ret = 0;
>> +
>> +       wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
>> +       if (IS_ERR(wdt->rsts)) {
>> +               dev_err(wdt->dev, "failed to get rsts error.\n");
>> +               ret = PTR_ERR(wdt->rsts);
>> +       } else {
>> +               ret = reset_control_deassert(wdt->rsts);
>> +               if (ret)
>> +                       dev_err(wdt->dev, "failed to deassert rsts.\n");
>> +       }
>> +       return ret;
>> +}
> 
> How about something like
> int ret;
> 
> wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
> if (IS_ERR(wdt->rsts))
>     return dev_err_probe(wdt->dev, PTR_ERR(wdt->rsts), "failed to get
> resets\n");
> 
> ret = reset_control_deassert(wdt->rsts);
> return dev_err_probe(wdt->dev, ret, "failed to deassert resets\n");

It looks more concise. Thanks.

> 
>> +
>> +static u32 starfive_wdt_ticks_to_sec(struct starfive_wdt *wdt, u32 ticks)
>> +{
>> +       return DIV_ROUND_CLOSEST(ticks, wdt->freq);
>> +}
>> +
>> +/*
>> + * Write unlock-key to unlock. Write other value to lock. When lock bit is 1,
>> + * external accesses to other watchdog registers are ignored.
>> + */
>> +static bool starfive_wdt_is_locked(struct starfive_wdt *wdt)
>> +{
>> +       u32 val;
>> +
>> +       val = readl(wdt->base + wdt->drv_data->unlock);
>> +       return !!(val & STARFIVE_WDT_LOCKED);
>> +}
>> +
>> +static void starfive_wdt_unlock(struct starfive_wdt *wdt)
>> +{
>> +       if (starfive_wdt_is_locked(wdt))
>> +               writel(wdt->drv_data->unlock_key,
>> +                      wdt->base + wdt->drv_data->unlock);
>> +}
>> +
>> +static void starfive_wdt_lock(struct starfive_wdt *wdt)
>> +{
>> +       if (!starfive_wdt_is_locked(wdt))
>> +               writel(~wdt->drv_data->unlock_key,
>> +                      wdt->base + wdt->drv_data->unlock);
>> +}
>> +
>> +/* enable watchdog interrupt to reset/reboot */
>> +static void starfive_wdt_enable_reset(struct starfive_wdt *wdt)
>> +{
>> +       u32 val;
>> +
>> +       val = readl(wdt->base + wdt->drv_data->control);
>> +       val |= STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift;
>> +       writel(val, wdt->base + wdt->drv_data->control);
>> +}
>> +
>> +/* disable watchdog interrupt to reset/reboot */
>> +static void starfive_wdt_disable_reset(struct starfive_wdt *wdt)
>> +{
>> +       u32 val;
>> +
>> +       val = readl(wdt->base + wdt->drv_data->control);
>> +       val &= ~(STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift);
>> +       writel(val, wdt->base + wdt->drv_data->control);
>> +}
>> +
>> +/* interrupt status whether has been raised from the counter */
>> +static bool starfive_wdt_raise_irq_status(struct starfive_wdt *wdt)
>> +{
>> +       return !!readl(wdt->base + wdt->drv_data->irq_is_raise);
>> +}
>> +
>> +/* clear interrupt signal before initialization or reload */
>> +static void starfive_wdt_int_clr(struct starfive_wdt *wdt)
>> +{
>> +       writel(STARFIVE_WDT_INTCLR, wdt->base + wdt->drv_data->int_clr);
>> +}
>> +
>> +static inline void starfive_wdt_set_count(struct starfive_wdt *wdt, u32 val)
>> +{
>> +       writel(val, wdt->base + wdt->drv_data->load);
>> +}
>> +
>> +static inline u32 starfive_wdt_get_count(struct starfive_wdt *wdt)
>> +{
>> +       return readl(wdt->base + wdt->drv_data->value);
>> +}
>> +
>> +/* enable watchdog */
>> +static inline void starfive_wdt_enable(struct starfive_wdt *wdt)
>> +{
>> +       u32 val;
>> +
>> +       val = readl(wdt->base + wdt->drv_data->enable);
>> +       val |= STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift;
>> +       writel(val, wdt->base + wdt->drv_data->enable);
>> +}
>> +
>> +/* disable watchdog */
>> +static inline void starfive_wdt_disable(struct starfive_wdt *wdt)
>> +{
>> +       u32 val;
>> +
>> +       val = readl(wdt->base + wdt->drv_data->enable);
>> +       val &= ~(STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift);
>> +       writel(val, wdt->base + wdt->drv_data->enable);
>> +}
>> +
>> +static inline void starfive_wdt_set_reload_count(struct starfive_wdt *wdt, u32 count)
>> +{
>> +       starfive_wdt_set_count(wdt, count);
>> +       /* need enable controller to reload counter */
>> +       starfive_wdt_enable(wdt);
>> +}
>> +
>> +static unsigned int starfive_wdt_max_timeout(struct starfive_wdt *wdt)
>> +{
>> +       return DIV_ROUND_UP(STARFIVE_WDT_MAXCNT, (wdt->freq / 2)) - 1;
>> +}
>> +
>> +static unsigned int starfive_wdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +       u32 count;
>> +
>> +       starfive_wdt_unlock(wdt);
>> +       /*
>> +        * Because set half count value,
>> +        * timeleft value should add the count value before first timeout.
>> +        */
>> +       count = starfive_wdt_get_count(wdt);
>> +       if (!starfive_wdt_raise_irq_status(wdt))
>> +               count += wdt->count;
>> +
>> +       starfive_wdt_lock(wdt);
>> +
>> +       return starfive_wdt_ticks_to_sec(wdt, count);
>> +}
>> +
>> +static int starfive_wdt_keepalive(struct watchdog_device *wdd)
>> +{
>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +       spin_lock(&wdt->lock);
>> +
>> +       starfive_wdt_unlock(wdt);
>> +       starfive_wdt_int_clr(wdt);
>> +       starfive_wdt_set_reload_count(wdt, wdt->count);
>> +       starfive_wdt_lock(wdt);
>> +
>> +       spin_unlock(&wdt->lock);
>> +
>> +       return 0;
>> +}
>> +
>> +static int starfive_wdt_stop(struct watchdog_device *wdd)
>> +{
>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +       spin_lock(&wdt->lock);
>> +
>> +       starfive_wdt_unlock(wdt);
>> +       starfive_wdt_disable_reset(wdt);
>> +       starfive_wdt_int_clr(wdt);
>> +       starfive_wdt_disable(wdt);
>> +       starfive_wdt_lock(wdt);
>> +
>> +       spin_unlock(&wdt->lock);
>> +
>> +       return 0;
>> +}
>> +
>> +static int starfive_wdt_pm_stop(struct watchdog_device *wdd)
>> +{
>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +       starfive_wdt_stop(wdd);
>> +       pm_runtime_put_sync(wdt->dev);
>> +
>> +       return 0;
>> +}
>> +
>> +static int starfive_wdt_start(struct watchdog_device *wdd)
>> +{
>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +       spin_lock(&wdt->lock);
>> +       starfive_wdt_unlock(wdt);
>> +       /* disable watchdog, to be safe */
>> +       starfive_wdt_disable(wdt);
>> +
>> +       starfive_wdt_enable_reset(wdt);
>> +       starfive_wdt_int_clr(wdt);
>> +       starfive_wdt_set_count(wdt, wdt->count);
>> +       starfive_wdt_enable(wdt);
>> +
>> +       starfive_wdt_lock(wdt);
>> +       spin_unlock(&wdt->lock);
>> +
>> +       return 0;
>> +}
>> +
>> +static int starfive_wdt_pm_start(struct watchdog_device *wdd)
>> +{
>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +       pm_runtime_get_sync(wdt->dev);
>> +
>> +       return starfive_wdt_start(wdd);
>> +}
>> +
>> +static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
>> +                                   unsigned int timeout)
>> +{
>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +       unsigned long freq = wdt->freq;
>> +
>> +       spin_lock(&wdt->lock);
>> +
>> +       /*
>> +        * This watchdog takes twice timeouts to reset.
>> +        * In order to reduce time to reset, should set half count value.
>> +        */
>> +       wdt->count = timeout * freq / 2;
>> +       wdd->timeout = timeout;
>> +
>> +       starfive_wdt_unlock(wdt);
>> +       starfive_wdt_disable(wdt);
>> +       starfive_wdt_set_reload_count(wdt, wdt->count);
>> +       starfive_wdt_enable(wdt);
>> +       starfive_wdt_lock(wdt);
>> +
>> +       spin_unlock(&wdt->lock);
>> +
>> +       return 0;
>> +}
>> +
>> +#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
>> +
>> +static const struct watchdog_info starfive_wdt_ident = {
>> +       .options = OPTIONS,
>> +       .identity = "StarFive Watchdog",
>> +};
>> +
>> +static const struct watchdog_ops starfive_wdt_ops = {
>> +       .owner = THIS_MODULE,
>> +       .start = starfive_wdt_pm_start,
>> +       .stop = starfive_wdt_pm_stop,
>> +       .ping = starfive_wdt_keepalive,
>> +       .set_timeout = starfive_wdt_set_timeout,
>> +       .get_timeleft = starfive_wdt_get_timeleft,
>> +};
>> +
>> +static const struct watchdog_device starfive_wdd = {
>> +       .info = &starfive_wdt_ident,
>> +       .ops = &starfive_wdt_ops,
>> +       .timeout = STARFIVE_WDT_DEFAULT_TIME,
>> +};
>> +
>> +static inline const struct starfive_wdt_variant *
>> +starfive_wdt_get_drv_data(struct platform_device *pdev)
>> +{
>> +       const struct starfive_wdt_variant *variant;
>> +
>> +       variant = of_device_get_match_data(&pdev->dev);
>> +       if (!variant) {
>> +               /* Device matched by platform_device_id */
>> +               variant = (struct starfive_wdt_variant *)
>> +                          platform_get_device_id(pdev)->driver_data;
>> +       }
>> +
>> +       return variant;
>> +}
> 
> If this driver is only used through the device tree this whole
> function can just be a call to of_device_get_match_data()

Well, if remove the platform_device_id struct, just use of_device_get_match_data().

> 
>> +static int starfive_wdt_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct starfive_wdt *wdt;
>> +       int ret;
>> +
>> +       wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>> +       if (!wdt)
>> +               return -ENOMEM;
>> +
>> +       wdt->dev = dev;
>> +       spin_lock_init(&wdt->lock);
>> +       wdt->wdt_device = starfive_wdd;
>> +
>> +       wdt->drv_data = starfive_wdt_get_drv_data(pdev);
>> +
>> +       /* get the memory region for the watchdog timer */
>> +       wdt->base = devm_platform_ioremap_resource(pdev, 0);
>> +       if (IS_ERR(wdt->base)) {
>> +               ret = PTR_ERR(wdt->base);
>> +               return ret;
>> +       }
>> +
>> +       platform_set_drvdata(pdev, wdt);
>> +       pm_runtime_enable(wdt->dev);
>> +
>> +       ret = starfive_wdt_get_clock(wdt);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (pm_runtime_enabled(wdt->dev)) {
>> +               ret = pm_runtime_get_sync(wdt->dev);
>> +               if (ret < 0)
>> +                       return ret;
>> +       } else {
>> +               /* runtime PM is disabled but clocks need to be enabled */
>> +               ret = clk_prepare_enable(wdt->apb_clk);
>> +               if (ret) {
>> +                       dev_err(wdt->dev, "failed to enable apb_clk.\n");
>> +                       return ret;
>> +               }
>> +               ret = clk_prepare_enable(wdt->core_clk);
>> +               if (ret) {
>> +                       dev_err(wdt->dev, "failed to enable core_clk.\n");
>> +                       goto err_apb_clk_disable;
>> +               }
>> +       }
> 
> The else part is basically starfive_wdt_runtime_resume(). Maybe just call that.

The starfive_wdt_runtime_resume() is defined under CONFIG_PM and it could not
be used directly. I should use a new function that enable/disable clock and
can be used in starfive_wdt_runtime_resume() and here.

> 
> 
>> +       ret = starfive_wdt_get_clock_rate(wdt);
>> +       if (ret)
>> +               goto err_clk_disable;
>> +
>> +       ret = starfive_wdt_reset_init(wdt);
>> +       if (ret)
>> +               goto err_clk_disable;
>> +
>> +       wdt->wdt_device.min_timeout = 1;
>> +       wdt->wdt_device.max_timeout = starfive_wdt_max_timeout(wdt);
>> +
>> +       watchdog_set_drvdata(&wdt->wdt_device, wdt);
>> +
>> +       /*
>> +        * see if we can actually set the requested heartbeat,
>> +        * and if not, try the default value.
>> +        */
>> +       watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>> +       if (wdt->wdt_device.timeout == 0 ||
>> +           wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
>> +               dev_warn(dev, "heartbeat value out of range, default %d used\n",
>> +                        STARFIVE_WDT_DEFAULT_TIME);
>> +               wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>> +       }
>> +       starfive_wdt_set_timeout(&wdt->wdt_device, wdt->wdt_device.timeout);
>> +
>> +       watchdog_set_nowayout(&wdt->wdt_device, nowayout);
>> +       watchdog_stop_on_reboot(&wdt->wdt_device);
>> +       watchdog_stop_on_unregister(&wdt->wdt_device);
>> +
>> +       wdt->wdt_device.parent = dev;
>> +
>> +       ret = watchdog_register_device(&wdt->wdt_device);
>> +       if (ret)
>> +               goto err_clk_disable;
>> +
>> +       if (early_enable) {
>> +               starfive_wdt_start(&wdt->wdt_device);
>> +               set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
>> +       } else {
>> +               starfive_wdt_stop(&wdt->wdt_device);
>> +       }
>> +
>> +       pm_runtime_put_sync(wdt->dev);
>> +
>> +       return 0;
>> +
>> +err_clk_disable:
>> +       clk_disable_unprepare(wdt->core_clk);
>> +err_apb_clk_disable:
>> +       clk_disable_unprepare(wdt->apb_clk);
>> +       pm_runtime_disable(wdt->dev);
>> +
>> +       return ret;
>> +}
>> +
>> +static int starfive_wdt_remove(struct platform_device *dev)
>> +{
>> +       struct starfive_wdt *wdt = platform_get_drvdata(dev);
>> +
>> +       starfive_wdt_stop(&wdt->wdt_device);
>> +       watchdog_unregister_device(&wdt->wdt_device);
>> +
>> +       if (pm_runtime_enabled(wdt->dev)) {
>> +               pm_runtime_disable(wdt->dev);
>> +       } else {
>> +               /* disable clock without PM */
>> +               clk_disable_unprepare(wdt->core_clk);
>> +               clk_disable_unprepare(wdt->apb_clk);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void starfive_wdt_shutdown(struct platform_device *dev)
>> +{
>> +       struct starfive_wdt *wdt = platform_get_drvdata(dev);
>> +
>> +       starfive_wdt_pm_stop(&wdt->wdt_device);
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int starfive_wdt_suspend(struct device *dev)
>> +{
>> +       int ret;
>> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
>> +
>> +       starfive_wdt_unlock(wdt);
>> +
>> +       /* Save watchdog state, and turn it off. */
>> +       wdt->reload = starfive_wdt_get_count(wdt);
>> +
>> +       /* Note that WTCNT doesn't need to be saved. */
>> +       starfive_wdt_stop(&wdt->wdt_device);
>> +       pm_runtime_force_suspend(dev);
>> +
>> +       starfive_wdt_lock(wdt);
>> +
>> +       return 0;
>> +}
>> +
>> +static int starfive_wdt_resume(struct device *dev)
>> +{
>> +       int ret;
>> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
>> +
>> +       starfive_wdt_unlock(wdt);
>> +
>> +       pm_runtime_force_resume(dev);
>> +
>> +       /* Restore watchdog state. */
>> +       starfive_wdt_set_reload_count(wdt, wdt->reload);
>> +
>> +       starfive_wdt_start(&wdt->wdt_device);
>> +
>> +       starfive_wdt_lock(wdt);
>> +
>> +       return 0;
>> +}
>> +#endif /* CONFIG_PM_SLEEP */
>> +
>> +#ifdef CONFIG_PM
>> +static int starfive_wdt_runtime_suspend(struct device *dev)
>> +{
>> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
>> +
>> +       clk_disable_unprepare(wdt->apb_clk);
>> +       clk_disable_unprepare(wdt->core_clk);
>> +
>> +       return 0;
>> +}
>> +
>> +static int starfive_wdt_runtime_resume(struct device *dev)
>> +{
>> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
>> +       int ret;
>> +
>> +       ret = clk_prepare_enable(wdt->apb_clk);
>> +       if (ret) {
>> +               dev_err(wdt->dev, "failed to enable apb_clk.\n");
>> +               return ret;
>> +       }
>> +
>> +       ret = clk_prepare_enable(wdt->core_clk);
>> +       if (ret)
>> +               dev_err(wdt->dev, "failed to enable core_clk.\n");
>> +
>> +       return ret;
>> +}
>> +#endif /* CONFIG_PM */
>> +
>> +static const struct dev_pm_ops starfive_wdt_pm_ops = {
>> +       SET_RUNTIME_PM_OPS(starfive_wdt_runtime_suspend, starfive_wdt_runtime_resume, NULL)
>> +       SET_SYSTEM_SLEEP_PM_OPS(starfive_wdt_suspend, starfive_wdt_resume)
>> +};
>> +
>> +static struct platform_driver starfive_wdt_driver = {
>> +       .probe          = starfive_wdt_probe,
>> +       .remove         = starfive_wdt_remove,
>> +       .shutdown       = starfive_wdt_shutdown,
>> +       .id_table       = starfive_wdt_ids,
>> +       .driver         = {
>> +               .name   = "starfive-wdt",
>> +               .pm     = &starfive_wdt_pm_ops,
>> +               .of_match_table = of_match_ptr(starfive_wdt_match),
>> +       },
>> +};
>> +
>> +module_platform_driver(starfive_wdt_driver);
>> +
>> +MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
>> +MODULE_AUTHOR("Samin Guo <samin.guo@starfivetech.com>");
>> +MODULE_DESCRIPTION("StarFive Watchdog Device Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.25.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Best regards,
Xingyu Wu

Emil Renner Berthing Feb. 28, 2023, 10:36 a.m. UTC | #13

On Tue, 28 Feb 2023 at 10:44, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>
> On 2023/2/26 22:14, Emil Renner Berthing wrote:
> > On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
> >>
> >> Add watchdog driver for the StarFive JH7110 SoC.
> >>
> >> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> >> ---
> >>  MAINTAINERS                     |   7 +
> >>  drivers/watchdog/Kconfig        |   9 +
> >>  drivers/watchdog/Makefile       |   2 +
> >>  drivers/watchdog/starfive-wdt.c | 651 ++++++++++++++++++++++++++++++++
> >>  4 files changed, 669 insertions(+)
> >>  create mode 100644 drivers/watchdog/starfive-wdt.c
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 135d93368d36..6cbcf08fa76a 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -19933,6 +19933,13 @@ F:     Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
> >>  F:     drivers/reset/reset-starfive-jh7100.c
> >>  F:     include/dt-bindings/reset/starfive-jh7100.h
> >>
> >> +STARFIVE JH7110 WATCHDOG DRIVER
> >> +M:     Xingyu Wu <xingyu.wu@starfivetech.com>
> >> +M:     Samin Guo <samin.guo@starfivetech.com>
> >> +S:     Supported
> >> +F:     Documentation/devicetree/bindings/watchdog/starfive*
> >> +F:     drivers/watchdog/starfive-wdt.c
> >> +
> >>  STATIC BRANCH/CALL
> >>  M:     Peter Zijlstra <peterz@infradead.org>
> >>  M:     Josh Poimboeuf <jpoimboe@kernel.org>
> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> >> index 0bc40b763b06..4608eb5c9501 100644
> >> --- a/drivers/watchdog/Kconfig
> >> +++ b/drivers/watchdog/Kconfig
> >> @@ -2089,6 +2089,15 @@ config UML_WATCHDOG
> >>         tristate "UML watchdog"
> >>         depends on UML || COMPILE_TEST
> >>
> >> +config STARFIVE_WATCHDOG
> >> +       tristate "StarFive Watchdog support"
> >> +       depends on RISCV
> >
> > Let's do like the pinctrl and clock drivers and
> >
> >     depends SOC_STARFIVE || COMPILE_TEST
> >
>
> Yes, will use 'ARCH_STARFIVE' instead.
>
> >> +       select WATCHDOG_CORE
> >> +       default SOC_STARFIVE
> >> +       help
> >> +         Say Y here to support the watchdog of StarFive JH7110 SoC.
> >> +         This driver can also be built as a module if choose M.
> >> +
> >>  #
> >>  # ISA-based Watchdog Cards
> >>  #
> >> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> >> index 9cbf6580f16c..4c0bd377e92a 100644
> >> --- a/drivers/watchdog/Makefile
> >> +++ b/drivers/watchdog/Makefile
> >> @@ -211,6 +211,8 @@ obj-$(CONFIG_WATCHDOG_SUN4V)                += sun4v_wdt.o
> >>  # Xen
> >>  obj-$(CONFIG_XEN_WDT) += xen_wdt.o
> >>
> >> +obj-$(CONFIG_STARFIVE_WATCHDOG) += starfive-wdt.o
> >> +
> >>  # Architecture Independent
> >>  obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
> >>  obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
> >> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> >> new file mode 100644
> >> index 000000000000..dfbb80406076
> >> --- /dev/null
> >> +++ b/drivers/watchdog/starfive-wdt.c
> >> @@ -0,0 +1,651 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Starfive Watchdog driver
> >> + *
> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> >> + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/iopoll.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/reset.h>
> >> +#include <linux/watchdog.h>
> >> +
> >> +/* JH7110 WatchDog register define */
> >> +#define STARFIVE_WDT_JH7110_LOAD       0x000   /* RW: Watchdog load register */
> >> +#define STARFIVE_WDT_JH7110_VALUE      0x004   /* RO: The current value for the watchdog counter */
> >> +#define STARFIVE_WDT_JH7110_CONTROL    0x008   /*
> >> +                                                * RW:
> >> +                                                * [0]: reset enable;
> >> +                                                * [1]: int enable/wdt enable/reload counter;
> >> +                                                * [31:2]: reserve.
> >> +                                                */
> >> +#define STARFIVE_WDT_JH7110_INTCLR     0x00c   /* WO: clear intterupt && reload the counter */
> >> +#define STARFIVE_WDT_JH7110_RIS                0x010   /* RO: Raw interrupt status from the counter */
> >> +#define STARFIVE_WDT_JH7110_IMS                0x014   /* RO: Enabled interrupt status from the counter */
> >> +#define STARFIVE_WDT_JH7110_LOCK       0xc00   /*
> >> +                                                * RO: Enable write access to all other registers
> >> +                                                * by writing 0x1ACCE551.
> >> +                                                */
> >
> > This driver is clearly prepared to support multiple SoCs, so hopefully
> > you've tested that internally. It would be great if you could leave in
> > the support for the JH7100 in the next revision. This would also make
> > all the indirection make a lot more sense.
>
> The JH7100 watchdog is little different from the 7110 and this driver only
> needs to change and add some control register layout and mask to support the 7100.
> And the JH8100 watchdog is the same as the 7110 but the 8100 is still in the plan.

Great. Yes, that's why I suggested you leave in support for the
JH7100, since very basic support for that is already merged and this
driver doesn't depend on the non-coherent dma support that is missing.

> So the dt-bingdings need to rename, and which one could be better,
> 'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?

Sure, starfive,jh71x0-wdt.yaml sounds good to me.

> >> +/* WDOGCONTROL */
> >> +#define STARFIVE_WDT_ENABLE                    0x1
> >> +#define STARFIVE_WDT_JH7110_EN_SHIFT           0
> >> +#define STARFIVE_WDT_RESET_EN                  0x1
> >> +#define STARFIVE_WDT_JH7110_RESEN_SHIFT                1
> >> +
> >> +/* WDOGLOCK */
> >> +#define STARFIVE_WDT_LOCKED                    BIT(0)
> >> +#define STARFIVE_WDT_JH7110_UNLOCK_KEY         0x1acce551
> >> +
> >> +/* WDOGINTCLR */
> >> +#define STARFIVE_WDT_INTCLR                    0x1
> >> +
> >> +#define STARFIVE_WDT_MAXCNT                    0xffffffff
> >> +#define STARFIVE_WDT_DEFAULT_TIME              (15)
> >> +#define STARFIVE_WDT_DELAY_US                  0
> >> +#define STARFIVE_WDT_TIMEOUT_US                        10000
> >> +
> >> +/* module parameter */
> >> +#define STARFIVE_WDT_EARLY_ENA                 0
> >> +
> >> +static bool nowayout = WATCHDOG_NOWAYOUT;
> >> +static int heartbeat;
> >> +static int early_enable = STARFIVE_WDT_EARLY_ENA;
> >> +
> >> +module_param(heartbeat, int, 0);
> >> +module_param(early_enable, int, 0);
> >
> > This also looks like a bool parameter.
>
> Oh will modify it.
>
> >
> >> +module_param(nowayout, bool, 0);
> >> +
> >> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeat in seconds. (default="
> >> +                __MODULE_STRING(STARFIVE_WDT_DEFAULT_TIME) ")");
> >> +MODULE_PARM_DESC(early_enable,
> >> +                "Watchdog is started at boot time if set to 1, default="
> >> +                __MODULE_STRING(STARFIVE_WDT_EARLY_ENA));
> >> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> >> +                __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >> +
> >> +struct starfive_wdt_variant {
> >> +       u32 control;
> >> +       u32 load;
> >> +       u32 enable;
> >> +       u32 value;
> >> +       u32 int_clr;
> >> +       u32 unlock;
> >
> > These are register offsets and not the values of registers, so could
> > be just unsigned int.
>
> Will modify it.

Great. Now that I look at it again the shifts below are also not
values written to a register, so could also be unsigned char.

> >
> >> +       u32 unlock_key;
> >> +       u32 irq_is_raise;
> >> +       u8 enrst_shift;
> >> +       u8 en_shift;
> >> +};
> >> +
> >> +struct starfive_wdt {
> >> +       unsigned long freq;
> >> +       struct device *dev;
> >> +       struct watchdog_device wdt_device;
> >> +       struct clk *core_clk;
> >> +       struct clk *apb_clk;
> >> +       struct reset_control *rsts;
> >> +       const struct starfive_wdt_variant *drv_data;
> >> +       u32 count;      /*count of timeout*/
> >> +       u32 reload;     /*restore the count*/
> >> +       void __iomem *base;
> >> +       spinlock_t lock;        /* spinlock for register handling */
> >> +};
> >> +
> >> +/* Register bias in JH7110 */
> >
> > bias? Maybe "register layout for the JH7110".
>
> Will use 'register layout' instead.
>
> >
> >> +static const struct starfive_wdt_variant drv_data_jh7110 = {
> >> +       .control = STARFIVE_WDT_JH7110_CONTROL,
> >> +       .load = STARFIVE_WDT_JH7110_LOAD,
> >> +       .enable = STARFIVE_WDT_JH7110_CONTROL,
> >> +       .value = STARFIVE_WDT_JH7110_VALUE,
> >> +       .int_clr = STARFIVE_WDT_JH7110_INTCLR,
> >> +       .unlock = STARFIVE_WDT_JH7110_LOCK,
> >> +       .unlock_key = STARFIVE_WDT_JH7110_UNLOCK_KEY,
> >> +       .irq_is_raise = STARFIVE_WDT_JH7110_IMS,
> >> +       .enrst_shift = STARFIVE_WDT_JH7110_RESEN_SHIFT,
> >> +       .en_shift = STARFIVE_WDT_JH7110_EN_SHIFT,
> >> +};
> >> +
> >> +static const struct of_device_id starfive_wdt_match[] = {
> >> +       { .compatible = "starfive,jh7110-wdt", .data = &drv_data_jh7110 },
> >> +       {}
> >
> > I like the idiom of ending with { /* sentinel */ } here. Also it is
> > common to place this struct just above the platform_driver struct
> > which is the only user.
>
> Will add it.
>
> >
> >> +};
> >> +MODULE_DEVICE_TABLE(of, starfive_wdt_match);
> >> +
> >> +static const struct platform_device_id starfive_wdt_ids[] = {
> >> +       {
> >> +               .name = "starfive-jh7110-wdt",
> >> +               .driver_data = (unsigned long)&drv_data_jh7110,
> >> +       },
> >> +       {}
> >> +};
> >> +MODULE_DEVICE_TABLE(platform, starfive_wdt_ids);
> >
> > When is this struct used? So far I only know of device tree enabled users.
>
> The struct will be used when CONFIG_OF is disabled
> but I deleted CONFIG_OF by mistake.
> Should I add the CONFIG_OF back or remove this struct?

Yeah, as far as I know neither the JH7110 nor JH7100 boots without
CONFIG_OF, which leaves this struct unused. So please remove it.

> >
> >> +static int starfive_wdt_get_clock_rate(struct starfive_wdt *wdt)
> >> +{
> >> +       wdt->freq = clk_get_rate(wdt->core_clk);
> >> +       /* The clock rate should not be 0.*/
> >> +       if (wdt->freq)
> >> +               return 0;
> >> +
> >> +       dev_err(wdt->dev, "get clock rate failed.\n");
> >> +       return -ENOENT;
> >> +}
> >> +
> >> +static int starfive_wdt_get_clock(struct starfive_wdt *wdt)
> >> +{
> >> +       wdt->apb_clk = devm_clk_get(wdt->dev, "apb");
> >> +       if (IS_ERR(wdt->apb_clk)) {
> >> +               dev_err(wdt->dev, "failed to get apb clock.\n");
> >> +               return PTR_ERR(wdt->apb_clk);
> >> +       }
> >> +
> >> +       wdt->core_clk = devm_clk_get(wdt->dev, "core");
> >> +       if (IS_ERR(wdt->core_clk)) {
> >> +               dev_err(wdt->dev, "failed to get core clock.\n");
> >> +               return PTR_ERR(wdt->core_clk);
> >> +       }
> >
> > Here and above you can use dev_err_probe which will also not print
> > errors on -EPROBE_DEFER.
>
> Will modify it.
>
> >
> >> +       return 0;
> >> +}
> >> +
> >> +static int starfive_wdt_reset_init(struct starfive_wdt *wdt)
> >> +{
> >> +       int ret = 0;
> >> +
> >> +       wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
> >> +       if (IS_ERR(wdt->rsts)) {
> >> +               dev_err(wdt->dev, "failed to get rsts error.\n");
> >> +               ret = PTR_ERR(wdt->rsts);
> >> +       } else {
> >> +               ret = reset_control_deassert(wdt->rsts);
> >> +               if (ret)
> >> +                       dev_err(wdt->dev, "failed to deassert rsts.\n");
> >> +       }
> >> +       return ret;
> >> +}
> >
> > How about something like
> > int ret;
> >
> > wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
> > if (IS_ERR(wdt->rsts))
> >     return dev_err_probe(wdt->dev, PTR_ERR(wdt->rsts), "failed to get
> > resets\n");
> >
> > ret = reset_control_deassert(wdt->rsts);
> > return dev_err_probe(wdt->dev, ret, "failed to deassert resets\n");
>
> It looks more concise. Thanks.
>
> >
> >> +
> >> +static u32 starfive_wdt_ticks_to_sec(struct starfive_wdt *wdt, u32 ticks)
> >> +{
> >> +       return DIV_ROUND_CLOSEST(ticks, wdt->freq);
> >> +}
> >> +
> >> +/*
> >> + * Write unlock-key to unlock. Write other value to lock. When lock bit is 1,
> >> + * external accesses to other watchdog registers are ignored.
> >> + */
> >> +static bool starfive_wdt_is_locked(struct starfive_wdt *wdt)
> >> +{
> >> +       u32 val;
> >> +
> >> +       val = readl(wdt->base + wdt->drv_data->unlock);
> >> +       return !!(val & STARFIVE_WDT_LOCKED);
> >> +}
> >> +
> >> +static void starfive_wdt_unlock(struct starfive_wdt *wdt)
> >> +{
> >> +       if (starfive_wdt_is_locked(wdt))
> >> +               writel(wdt->drv_data->unlock_key,
> >> +                      wdt->base + wdt->drv_data->unlock);
> >> +}
> >> +
> >> +static void starfive_wdt_lock(struct starfive_wdt *wdt)
> >> +{
> >> +       if (!starfive_wdt_is_locked(wdt))
> >> +               writel(~wdt->drv_data->unlock_key,
> >> +                      wdt->base + wdt->drv_data->unlock);
> >> +}
> >> +
> >> +/* enable watchdog interrupt to reset/reboot */
> >> +static void starfive_wdt_enable_reset(struct starfive_wdt *wdt)
> >> +{
> >> +       u32 val;
> >> +
> >> +       val = readl(wdt->base + wdt->drv_data->control);
> >> +       val |= STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift;
> >> +       writel(val, wdt->base + wdt->drv_data->control);
> >> +}
> >> +
> >> +/* disable watchdog interrupt to reset/reboot */
> >> +static void starfive_wdt_disable_reset(struct starfive_wdt *wdt)
> >> +{
> >> +       u32 val;
> >> +
> >> +       val = readl(wdt->base + wdt->drv_data->control);
> >> +       val &= ~(STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift);
> >> +       writel(val, wdt->base + wdt->drv_data->control);
> >> +}
> >> +
> >> +/* interrupt status whether has been raised from the counter */
> >> +static bool starfive_wdt_raise_irq_status(struct starfive_wdt *wdt)
> >> +{
> >> +       return !!readl(wdt->base + wdt->drv_data->irq_is_raise);
> >> +}
> >> +
> >> +/* clear interrupt signal before initialization or reload */
> >> +static void starfive_wdt_int_clr(struct starfive_wdt *wdt)
> >> +{
> >> +       writel(STARFIVE_WDT_INTCLR, wdt->base + wdt->drv_data->int_clr);
> >> +}
> >> +
> >> +static inline void starfive_wdt_set_count(struct starfive_wdt *wdt, u32 val)
> >> +{
> >> +       writel(val, wdt->base + wdt->drv_data->load);
> >> +}
> >> +
> >> +static inline u32 starfive_wdt_get_count(struct starfive_wdt *wdt)
> >> +{
> >> +       return readl(wdt->base + wdt->drv_data->value);
> >> +}
> >> +
> >> +/* enable watchdog */
> >> +static inline void starfive_wdt_enable(struct starfive_wdt *wdt)
> >> +{
> >> +       u32 val;
> >> +
> >> +       val = readl(wdt->base + wdt->drv_data->enable);
> >> +       val |= STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift;
> >> +       writel(val, wdt->base + wdt->drv_data->enable);
> >> +}
> >> +
> >> +/* disable watchdog */
> >> +static inline void starfive_wdt_disable(struct starfive_wdt *wdt)
> >> +{
> >> +       u32 val;
> >> +
> >> +       val = readl(wdt->base + wdt->drv_data->enable);
> >> +       val &= ~(STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift);
> >> +       writel(val, wdt->base + wdt->drv_data->enable);
> >> +}
> >> +
> >> +static inline void starfive_wdt_set_reload_count(struct starfive_wdt *wdt, u32 count)
> >> +{
> >> +       starfive_wdt_set_count(wdt, count);
> >> +       /* need enable controller to reload counter */
> >> +       starfive_wdt_enable(wdt);
> >> +}
> >> +
> >> +static unsigned int starfive_wdt_max_timeout(struct starfive_wdt *wdt)
> >> +{
> >> +       return DIV_ROUND_UP(STARFIVE_WDT_MAXCNT, (wdt->freq / 2)) - 1;
> >> +}
> >> +
> >> +static unsigned int starfive_wdt_get_timeleft(struct watchdog_device *wdd)
> >> +{
> >> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> >> +       u32 count;
> >> +
> >> +       starfive_wdt_unlock(wdt);
> >> +       /*
> >> +        * Because set half count value,
> >> +        * timeleft value should add the count value before first timeout.
> >> +        */
> >> +       count = starfive_wdt_get_count(wdt);
> >> +       if (!starfive_wdt_raise_irq_status(wdt))
> >> +               count += wdt->count;
> >> +
> >> +       starfive_wdt_lock(wdt);
> >> +
> >> +       return starfive_wdt_ticks_to_sec(wdt, count);
> >> +}
> >> +
> >> +static int starfive_wdt_keepalive(struct watchdog_device *wdd)
> >> +{
> >> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> >> +
> >> +       spin_lock(&wdt->lock);
> >> +
> >> +       starfive_wdt_unlock(wdt);
> >> +       starfive_wdt_int_clr(wdt);
> >> +       starfive_wdt_set_reload_count(wdt, wdt->count);
> >> +       starfive_wdt_lock(wdt);
> >> +
> >> +       spin_unlock(&wdt->lock);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int starfive_wdt_stop(struct watchdog_device *wdd)
> >> +{
> >> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> >> +
> >> +       spin_lock(&wdt->lock);
> >> +
> >> +       starfive_wdt_unlock(wdt);
> >> +       starfive_wdt_disable_reset(wdt);
> >> +       starfive_wdt_int_clr(wdt);
> >> +       starfive_wdt_disable(wdt);
> >> +       starfive_wdt_lock(wdt);
> >> +
> >> +       spin_unlock(&wdt->lock);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int starfive_wdt_pm_stop(struct watchdog_device *wdd)
> >> +{
> >> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> >> +
> >> +       starfive_wdt_stop(wdd);
> >> +       pm_runtime_put_sync(wdt->dev);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int starfive_wdt_start(struct watchdog_device *wdd)
> >> +{
> >> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> >> +
> >> +       spin_lock(&wdt->lock);
> >> +       starfive_wdt_unlock(wdt);
> >> +       /* disable watchdog, to be safe */
> >> +       starfive_wdt_disable(wdt);
> >> +
> >> +       starfive_wdt_enable_reset(wdt);
> >> +       starfive_wdt_int_clr(wdt);
> >> +       starfive_wdt_set_count(wdt, wdt->count);
> >> +       starfive_wdt_enable(wdt);
> >> +
> >> +       starfive_wdt_lock(wdt);
> >> +       spin_unlock(&wdt->lock);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int starfive_wdt_pm_start(struct watchdog_device *wdd)
> >> +{
> >> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> >> +
> >> +       pm_runtime_get_sync(wdt->dev);
> >> +
> >> +       return starfive_wdt_start(wdd);
> >> +}
> >> +
> >> +static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
> >> +                                   unsigned int timeout)
> >> +{
> >> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> >> +       unsigned long freq = wdt->freq;
> >> +
> >> +       spin_lock(&wdt->lock);
> >> +
> >> +       /*
> >> +        * This watchdog takes twice timeouts to reset.
> >> +        * In order to reduce time to reset, should set half count value.
> >> +        */
> >> +       wdt->count = timeout * freq / 2;
> >> +       wdd->timeout = timeout;
> >> +
> >> +       starfive_wdt_unlock(wdt);
> >> +       starfive_wdt_disable(wdt);
> >> +       starfive_wdt_set_reload_count(wdt, wdt->count);
> >> +       starfive_wdt_enable(wdt);
> >> +       starfive_wdt_lock(wdt);
> >> +
> >> +       spin_unlock(&wdt->lock);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
> >> +
> >> +static const struct watchdog_info starfive_wdt_ident = {
> >> +       .options = OPTIONS,
> >> +       .identity = "StarFive Watchdog",
> >> +};
> >> +
> >> +static const struct watchdog_ops starfive_wdt_ops = {
> >> +       .owner = THIS_MODULE,
> >> +       .start = starfive_wdt_pm_start,
> >> +       .stop = starfive_wdt_pm_stop,
> >> +       .ping = starfive_wdt_keepalive,
> >> +       .set_timeout = starfive_wdt_set_timeout,
> >> +       .get_timeleft = starfive_wdt_get_timeleft,
> >> +};
> >> +
> >> +static const struct watchdog_device starfive_wdd = {
> >> +       .info = &starfive_wdt_ident,
> >> +       .ops = &starfive_wdt_ops,
> >> +       .timeout = STARFIVE_WDT_DEFAULT_TIME,
> >> +};
> >> +
> >> +static inline const struct starfive_wdt_variant *
> >> +starfive_wdt_get_drv_data(struct platform_device *pdev)
> >> +{
> >> +       const struct starfive_wdt_variant *variant;
> >> +
> >> +       variant = of_device_get_match_data(&pdev->dev);
> >> +       if (!variant) {
> >> +               /* Device matched by platform_device_id */
> >> +               variant = (struct starfive_wdt_variant *)
> >> +                          platform_get_device_id(pdev)->driver_data;
> >> +       }
> >> +
> >> +       return variant;
> >> +}
> >
> > If this driver is only used through the device tree this whole
> > function can just be a call to of_device_get_match_data()
>
> Well, if remove the platform_device_id struct, just use of_device_get_match_data().
>
> >
> >> +static int starfive_wdt_probe(struct platform_device *pdev)
> >> +{
> >> +       struct device *dev = &pdev->dev;
> >> +       struct starfive_wdt *wdt;
> >> +       int ret;
> >> +
> >> +       wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> >> +       if (!wdt)
> >> +               return -ENOMEM;
> >> +
> >> +       wdt->dev = dev;
> >> +       spin_lock_init(&wdt->lock);
> >> +       wdt->wdt_device = starfive_wdd;
> >> +
> >> +       wdt->drv_data = starfive_wdt_get_drv_data(pdev);
> >> +
> >> +       /* get the memory region for the watchdog timer */
> >> +       wdt->base = devm_platform_ioremap_resource(pdev, 0);
> >> +       if (IS_ERR(wdt->base)) {
> >> +               ret = PTR_ERR(wdt->base);
> >> +               return ret;
> >> +       }
> >> +
> >> +       platform_set_drvdata(pdev, wdt);
> >> +       pm_runtime_enable(wdt->dev);
> >> +
> >> +       ret = starfive_wdt_get_clock(wdt);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       if (pm_runtime_enabled(wdt->dev)) {
> >> +               ret = pm_runtime_get_sync(wdt->dev);
> >> +               if (ret < 0)
> >> +                       return ret;
> >> +       } else {
> >> +               /* runtime PM is disabled but clocks need to be enabled */
> >> +               ret = clk_prepare_enable(wdt->apb_clk);
> >> +               if (ret) {
> >> +                       dev_err(wdt->dev, "failed to enable apb_clk.\n");
> >> +                       return ret;
> >> +               }
> >> +               ret = clk_prepare_enable(wdt->core_clk);
> >> +               if (ret) {
> >> +                       dev_err(wdt->dev, "failed to enable core_clk.\n");
> >> +                       goto err_apb_clk_disable;
> >> +               }
> >> +       }
> >
> > The else part is basically starfive_wdt_runtime_resume(). Maybe just call that.
>
> The starfive_wdt_runtime_resume() is defined under CONFIG_PM and it could not
> be used directly. I should use a new function that enable/disable clock and
> can be used in starfive_wdt_runtime_resume() and here.

Yes, either that or just define that function outside #ifdef CONFIG_PM.

> >> +
> >> +       watchdog_set_drvdata(&wdt->wdt_device, wdt);
> >> +
> >> +       /*
> >> +        * see if we can actually set the requested heartbeat,
> >> +        * and if not, try the default value.
> >> +        */
> >> +       watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
> >> +       if (wdt->wdt_device.timeout == 0 ||
> >> +           wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
> >> +               dev_warn(dev, "heartbeat value out of range, default %d used\n",
> >> +                        STARFIVE_WDT_DEFAULT_TIME);
> >> +               wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
> >> +       }
> >> +       starfive_wdt_set_timeout(&wdt->wdt_device, wdt->wdt_device.timeout);
> >> +
> >> +       watchdog_set_nowayout(&wdt->wdt_device, nowayout);
> >> +       watchdog_stop_on_reboot(&wdt->wdt_device);
> >> +       watchdog_stop_on_unregister(&wdt->wdt_device);
> >> +
> >> +       wdt->wdt_device.parent = dev;
> >> +
> >> +       ret = watchdog_register_device(&wdt->wdt_device);
> >> +       if (ret)
> >> +               goto err_clk_disable;
> >> +
> >> +       if (early_enable) {
> >> +               starfive_wdt_start(&wdt->wdt_device);
> >> +               set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> >> +       } else {
> >> +               starfive_wdt_stop(&wdt->wdt_device);
> >> +       }
> >> +
> >> +       pm_runtime_put_sync(wdt->dev);
> >> +
> >> +       return 0;
> >> +
> >> +err_clk_disable:
> >> +       clk_disable_unprepare(wdt->core_clk);
> >> +err_apb_clk_disable:
> >> +       clk_disable_unprepare(wdt->apb_clk);
> >> +       pm_runtime_disable(wdt->dev);
> >> +
> >> +       return ret;
> >> +}
> >> +
> >> +static int starfive_wdt_remove(struct platform_device *dev)
> >> +{
> >> +       struct starfive_wdt *wdt = platform_get_drvdata(dev);
> >> +
> >> +       starfive_wdt_stop(&wdt->wdt_device);
> >> +       watchdog_unregister_device(&wdt->wdt_device);
> >> +
> >> +       if (pm_runtime_enabled(wdt->dev)) {
> >> +               pm_runtime_disable(wdt->dev);
> >> +       } else {
> >> +               /* disable clock without PM */
> >> +               clk_disable_unprepare(wdt->core_clk);
> >> +               clk_disable_unprepare(wdt->apb_clk);
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static void starfive_wdt_shutdown(struct platform_device *dev)
> >> +{
> >> +       struct starfive_wdt *wdt = platform_get_drvdata(dev);
> >> +
> >> +       starfive_wdt_pm_stop(&wdt->wdt_device);
> >> +}
> >> +
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int starfive_wdt_suspend(struct device *dev)
> >> +{
> >> +       int ret;
> >> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
> >> +
> >> +       starfive_wdt_unlock(wdt);
> >> +
> >> +       /* Save watchdog state, and turn it off. */
> >> +       wdt->reload = starfive_wdt_get_count(wdt);
> >> +
> >> +       /* Note that WTCNT doesn't need to be saved. */
> >> +       starfive_wdt_stop(&wdt->wdt_device);
> >> +       pm_runtime_force_suspend(dev);
> >> +
> >> +       starfive_wdt_lock(wdt);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int starfive_wdt_resume(struct device *dev)
> >> +{
> >> +       int ret;
> >> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
> >> +
> >> +       starfive_wdt_unlock(wdt);
> >> +
> >> +       pm_runtime_force_resume(dev);
> >> +
> >> +       /* Restore watchdog state. */
> >> +       starfive_wdt_set_reload_count(wdt, wdt->reload);
> >> +
> >> +       starfive_wdt_start(&wdt->wdt_device);
> >> +
> >> +       starfive_wdt_lock(wdt);
> >> +
> >> +       return 0;
> >> +}
> >> +#endif /* CONFIG_PM_SLEEP */
> >> +
> >> +#ifdef CONFIG_PM
> >> +static int starfive_wdt_runtime_suspend(struct device *dev)
> >> +{
> >> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
> >> +
> >> +       clk_disable_unprepare(wdt->apb_clk);
> >> +       clk_disable_unprepare(wdt->core_clk);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int starfive_wdt_runtime_resume(struct device *dev)
> >> +{
> >> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
> >> +       int ret;
> >> +
> >> +       ret = clk_prepare_enable(wdt->apb_clk);
> >> +       if (ret) {
> >> +               dev_err(wdt->dev, "failed to enable apb_clk.\n");
> >> +               return ret;
> >> +       }
> >> +
> >> +       ret = clk_prepare_enable(wdt->core_clk);
> >> +       if (ret)
> >> +               dev_err(wdt->dev, "failed to enable core_clk.\n");
> >> +
> >> +       return ret;
> >> +}
> >> +#endif /* CONFIG_PM */
> >> +
> >> +static const struct dev_pm_ops starfive_wdt_pm_ops = {
> >> +       SET_RUNTIME_PM_OPS(starfive_wdt_runtime_suspend, starfive_wdt_runtime_resume, NULL)
> >> +       SET_SYSTEM_SLEEP_PM_OPS(starfive_wdt_suspend, starfive_wdt_resume)
> >> +};
> >> +
> >> +static struct platform_driver starfive_wdt_driver = {
> >> +       .probe          = starfive_wdt_probe,
> >> +       .remove         = starfive_wdt_remove,
> >> +       .shutdown       = starfive_wdt_shutdown,
> >> +       .id_table       = starfive_wdt_ids,
> >> +       .driver         = {
> >> +               .name   = "starfive-wdt",
> >> +               .pm     = &starfive_wdt_pm_ops,
> >> +               .of_match_table = of_match_ptr(starfive_wdt_match),
> >> +       },
> >> +};
> >> +
> >> +module_platform_driver(starfive_wdt_driver);
> >> +
> >> +MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
> >> +MODULE_AUTHOR("Samin Guo <samin.guo@starfivetech.com>");
> >> +MODULE_DESCRIPTION("StarFive Watchdog Device Driver");
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.25.1
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> Best regards,
> Xingyu Wu
>

Conor Dooley Feb. 28, 2023, 10:51 a.m. UTC | #14

On Tue, Feb 28, 2023 at 11:36:49AM +0100, Emil Renner Berthing wrote:
> On Tue, 28 Feb 2023 at 10:44, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
> > On 2023/2/26 22:14, Emil Renner Berthing wrote:
> > > On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:

> > So the dt-bingdings need to rename, and which one could be better,
> > 'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?
> 
> Sure, starfive,jh71x0-wdt.yaml sounds good to me.

I feel like a common comment I see from the dt folks is to not put
wildcards in filenames & just pick the first compatible.
I could very well be wrong on that front though...

Krzysztof Kozlowski Feb. 28, 2023, 10:57 a.m. UTC | #15

On 28/02/2023 11:51, Conor Dooley wrote:
> On Tue, Feb 28, 2023 at 11:36:49AM +0100, Emil Renner Berthing wrote:
>> On Tue, 28 Feb 2023 at 10:44, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>>> On 2023/2/26 22:14, Emil Renner Berthing wrote:
>>>> On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
> 
>>> So the dt-bingdings need to rename, and which one could be better,
>>> 'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?
>>
>> Sure, starfive,jh71x0-wdt.yaml sounds good to me.
> 
> I feel like a common comment I see from the dt folks is to not put
> wildcards in filenames & just pick the first compatible.
> I could very well be wrong on that front though...

First compatible is a bit better, unless you are sure this will cover
all such compatibles now and in the future. For many bindings the
family/wildcards were fine in filename.

Best regards,
Krzysztof

Conor Dooley Feb. 28, 2023, 11:11 a.m. UTC | #16

On Tue, Feb 28, 2023 at 11:57:58AM +0100, Krzysztof Kozlowski wrote:
> On 28/02/2023 11:51, Conor Dooley wrote:
> > On Tue, Feb 28, 2023 at 11:36:49AM +0100, Emil Renner Berthing wrote:
> >> On Tue, 28 Feb 2023 at 10:44, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
> >>> On 2023/2/26 22:14, Emil Renner Berthing wrote:
> >>>> On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
> > 
> >>> So the dt-bingdings need to rename, and which one could be better,
> >>> 'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?
> >>
> >> Sure, starfive,jh71x0-wdt.yaml sounds good to me.
> > 
> > I feel like a common comment I see from the dt folks is to not put
> > wildcards in filenames & just pick the first compatible.
> > I could very well be wrong on that front though...
> 
> First compatible is a bit better, unless you are sure this will cover
> all such compatibles now and in the future. For many bindings the
> family/wildcards were fine in filename.

Ahh cool, good to know what the specific policy is - thanks!

Xingyu Wu Feb. 28, 2023, 1:16 p.m. UTC | #17

On 2023/2/28 19:11, Conor Dooley wrote:
> On Tue, Feb 28, 2023 at 11:57:58AM +0100, Krzysztof Kozlowski wrote:
>> On 28/02/2023 11:51, Conor Dooley wrote:
>> > On Tue, Feb 28, 2023 at 11:36:49AM +0100, Emil Renner Berthing wrote:
>> >> On Tue, 28 Feb 2023 at 10:44, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>> >>> On 2023/2/26 22:14, Emil Renner Berthing wrote:
>> >>>> On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>> > 
>> >>> So the dt-bingdings need to rename, and which one could be better,
>> >>> 'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?
>> >>
>> >> Sure, starfive,jh71x0-wdt.yaml sounds good to me.
>> > 
>> > I feel like a common comment I see from the dt folks is to not put
>> > wildcards in filenames & just pick the first compatible.
>> > I could very well be wrong on that front though...
>> 
>> First compatible is a bit better, unless you are sure this will cover
>> all such compatibles now and in the future. For many bindings the
>> family/wildcards were fine in filename.
> 
> Ahh cool, good to know what the specific policy is - thanks!

If this watchdog driver is improved to also support JH7100 in next patch,
it seems more reasonable to rename the dt-bingdings to 'starfive,jh71x0-wdt.yaml'.

Krzysztof Kozlowski Feb. 28, 2023, 2:03 p.m. UTC | #18

On 28/02/2023 14:16, Xingyu Wu wrote:
> On 2023/2/28 19:11, Conor Dooley wrote:
>> On Tue, Feb 28, 2023 at 11:57:58AM +0100, Krzysztof Kozlowski wrote:
>>> On 28/02/2023 11:51, Conor Dooley wrote:
>>>> On Tue, Feb 28, 2023 at 11:36:49AM +0100, Emil Renner Berthing wrote:
>>>>> On Tue, 28 Feb 2023 at 10:44, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>>>>>> On 2023/2/26 22:14, Emil Renner Berthing wrote:
>>>>>>> On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>>>>
>>>>>> So the dt-bingdings need to rename, and which one could be better,
>>>>>> 'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?
>>>>>
>>>>> Sure, starfive,jh71x0-wdt.yaml sounds good to me.
>>>>
>>>> I feel like a common comment I see from the dt folks is to not put
>>>> wildcards in filenames & just pick the first compatible.
>>>> I could very well be wrong on that front though...
>>>
>>> First compatible is a bit better, unless you are sure this will cover
>>> all such compatibles now and in the future. For many bindings the
>>> family/wildcards were fine in filename.
>>
>> Ahh cool, good to know what the specific policy is - thanks!
> 
> If this watchdog driver is improved to also support JH7100 in next patch,
> it seems more reasonable to rename the dt-bingdings to 'starfive,jh71x0-wdt.yaml'.

Bindings are for hardware, not drivers. One version of driver in one OS
should not determine the bindings...

Best regards,
Krzysztof

Guenter Roeck Feb. 28, 2023, 2:59 p.m. UTC | #19

On 2/28/23 02:36, Emil Renner Berthing wrote:
> On Tue, 28 Feb 2023 at 10:44, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>>
>> On 2023/2/26 22:14, Emil Renner Berthing wrote:
>>> On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>>>>
>>>> Add watchdog driver for the StarFive JH7110 SoC.
>>>>
>>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>>> ---
>>>>   MAINTAINERS                     |   7 +
>>>>   drivers/watchdog/Kconfig        |   9 +
>>>>   drivers/watchdog/Makefile       |   2 +
>>>>   drivers/watchdog/starfive-wdt.c | 651 ++++++++++++++++++++++++++++++++
>>>>   4 files changed, 669 insertions(+)
>>>>   create mode 100644 drivers/watchdog/starfive-wdt.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 135d93368d36..6cbcf08fa76a 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -19933,6 +19933,13 @@ F:     Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>>>>   F:     drivers/reset/reset-starfive-jh7100.c
>>>>   F:     include/dt-bindings/reset/starfive-jh7100.h
>>>>
>>>> +STARFIVE JH7110 WATCHDOG DRIVER
>>>> +M:     Xingyu Wu <xingyu.wu@starfivetech.com>
>>>> +M:     Samin Guo <samin.guo@starfivetech.com>
>>>> +S:     Supported
>>>> +F:     Documentation/devicetree/bindings/watchdog/starfive*
>>>> +F:     drivers/watchdog/starfive-wdt.c
>>>> +
>>>>   STATIC BRANCH/CALL
>>>>   M:     Peter Zijlstra <peterz@infradead.org>
>>>>   M:     Josh Poimboeuf <jpoimboe@kernel.org>
>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>> index 0bc40b763b06..4608eb5c9501 100644
>>>> --- a/drivers/watchdog/Kconfig
>>>> +++ b/drivers/watchdog/Kconfig
>>>> @@ -2089,6 +2089,15 @@ config UML_WATCHDOG
>>>>          tristate "UML watchdog"
>>>>          depends on UML || COMPILE_TEST
>>>>
>>>> +config STARFIVE_WATCHDOG
>>>> +       tristate "StarFive Watchdog support"
>>>> +       depends on RISCV
>>>
>>> Let's do like the pinctrl and clock drivers and
>>>
>>>      depends SOC_STARFIVE || COMPILE_TEST
>>>
>>
>> Yes, will use 'ARCH_STARFIVE' instead.
>>
>>>> +       select WATCHDOG_CORE
>>>> +       default SOC_STARFIVE
>>>> +       help
>>>> +         Say Y here to support the watchdog of StarFive JH7110 SoC.
>>>> +         This driver can also be built as a module if choose M.
>>>> +
>>>>   #
>>>>   # ISA-based Watchdog Cards
>>>>   #
>>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>>> index 9cbf6580f16c..4c0bd377e92a 100644
>>>> --- a/drivers/watchdog/Makefile
>>>> +++ b/drivers/watchdog/Makefile
>>>> @@ -211,6 +211,8 @@ obj-$(CONFIG_WATCHDOG_SUN4V)                += sun4v_wdt.o
>>>>   # Xen
>>>>   obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>>>>
>>>> +obj-$(CONFIG_STARFIVE_WATCHDOG) += starfive-wdt.o
>>>> +
>>>>   # Architecture Independent
>>>>   obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
>>>>   obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
>>>> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
>>>> new file mode 100644
>>>> index 000000000000..dfbb80406076
>>>> --- /dev/null
>>>> +++ b/drivers/watchdog/starfive-wdt.c
>>>> @@ -0,0 +1,651 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Starfive Watchdog driver
>>>> + *
>>>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/iopoll.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/reset.h>
>>>> +#include <linux/watchdog.h>
>>>> +
>>>> +/* JH7110 WatchDog register define */
>>>> +#define STARFIVE_WDT_JH7110_LOAD       0x000   /* RW: Watchdog load register */
>>>> +#define STARFIVE_WDT_JH7110_VALUE      0x004   /* RO: The current value for the watchdog counter */
>>>> +#define STARFIVE_WDT_JH7110_CONTROL    0x008   /*
>>>> +                                                * RW:
>>>> +                                                * [0]: reset enable;
>>>> +                                                * [1]: int enable/wdt enable/reload counter;
>>>> +                                                * [31:2]: reserve.
>>>> +                                                */
>>>> +#define STARFIVE_WDT_JH7110_INTCLR     0x00c   /* WO: clear intterupt && reload the counter */
>>>> +#define STARFIVE_WDT_JH7110_RIS                0x010   /* RO: Raw interrupt status from the counter */
>>>> +#define STARFIVE_WDT_JH7110_IMS                0x014   /* RO: Enabled interrupt status from the counter */
>>>> +#define STARFIVE_WDT_JH7110_LOCK       0xc00   /*
>>>> +                                                * RO: Enable write access to all other registers
>>>> +                                                * by writing 0x1ACCE551.
>>>> +                                                */
>>>
>>> This driver is clearly prepared to support multiple SoCs, so hopefully
>>> you've tested that internally. It would be great if you could leave in
>>> the support for the JH7100 in the next revision. This would also make
>>> all the indirection make a lot more sense.
>>
>> The JH7100 watchdog is little different from the 7110 and this driver only
>> needs to change and add some control register layout and mask to support the 7100.
>> And the JH8100 watchdog is the same as the 7110 but the 8100 is still in the plan.
> 
> Great. Yes, that's why I suggested you leave in support for the
> JH7100, since very basic support for that is already merged and this
> driver doesn't depend on the non-coherent dma support that is missing.
> 
>> So the dt-bingdings need to rename, and which one could be better,
>> 'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?
> 
> Sure, starfive,jh71x0-wdt.yaml sounds good to me.
> 

So the bindings are guaranteed to support jh71[0-9]0 in the future ?

Weally, please, just leave the name alone. X in file names is just
confusing. Just name everything after the first chip that is supported.

Guenter

>>>> +/* WDOGCONTROL */
>>>> +#define STARFIVE_WDT_ENABLE                    0x1
>>>> +#define STARFIVE_WDT_JH7110_EN_SHIFT           0
>>>> +#define STARFIVE_WDT_RESET_EN                  0x1
>>>> +#define STARFIVE_WDT_JH7110_RESEN_SHIFT                1
>>>> +
>>>> +/* WDOGLOCK */
>>>> +#define STARFIVE_WDT_LOCKED                    BIT(0)
>>>> +#define STARFIVE_WDT_JH7110_UNLOCK_KEY         0x1acce551
>>>> +
>>>> +/* WDOGINTCLR */
>>>> +#define STARFIVE_WDT_INTCLR                    0x1
>>>> +
>>>> +#define STARFIVE_WDT_MAXCNT                    0xffffffff
>>>> +#define STARFIVE_WDT_DEFAULT_TIME              (15)
>>>> +#define STARFIVE_WDT_DELAY_US                  0
>>>> +#define STARFIVE_WDT_TIMEOUT_US                        10000
>>>> +
>>>> +/* module parameter */
>>>> +#define STARFIVE_WDT_EARLY_ENA                 0
>>>> +
>>>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>>>> +static int heartbeat;
>>>> +static int early_enable = STARFIVE_WDT_EARLY_ENA;
>>>> +
>>>> +module_param(heartbeat, int, 0);
>>>> +module_param(early_enable, int, 0);
>>>
>>> This also looks like a bool parameter.
>>
>> Oh will modify it.
>>
>>>
>>>> +module_param(nowayout, bool, 0);
>>>> +
>>>> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeat in seconds. (default="
>>>> +                __MODULE_STRING(STARFIVE_WDT_DEFAULT_TIME) ")");
>>>> +MODULE_PARM_DESC(early_enable,
>>>> +                "Watchdog is started at boot time if set to 1, default="
>>>> +                __MODULE_STRING(STARFIVE_WDT_EARLY_ENA));
>>>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>>>> +                __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>>> +
>>>> +struct starfive_wdt_variant {
>>>> +       u32 control;
>>>> +       u32 load;
>>>> +       u32 enable;
>>>> +       u32 value;
>>>> +       u32 int_clr;
>>>> +       u32 unlock;
>>>
>>> These are register offsets and not the values of registers, so could
>>> be just unsigned int.
>>
>> Will modify it.
> 
> Great. Now that I look at it again the shifts below are also not
> values written to a register, so could also be unsigned char.
> 
>>>
>>>> +       u32 unlock_key;
>>>> +       u32 irq_is_raise;
>>>> +       u8 enrst_shift;
>>>> +       u8 en_shift;
>>>> +};
>>>> +
>>>> +struct starfive_wdt {
>>>> +       unsigned long freq;
>>>> +       struct device *dev;
>>>> +       struct watchdog_device wdt_device;
>>>> +       struct clk *core_clk;
>>>> +       struct clk *apb_clk;
>>>> +       struct reset_control *rsts;
>>>> +       const struct starfive_wdt_variant *drv_data;
>>>> +       u32 count;      /*count of timeout*/
>>>> +       u32 reload;     /*restore the count*/
>>>> +       void __iomem *base;
>>>> +       spinlock_t lock;        /* spinlock for register handling */
>>>> +};
>>>> +
>>>> +/* Register bias in JH7110 */
>>>
>>> bias? Maybe "register layout for the JH7110".
>>
>> Will use 'register layout' instead.
>>
>>>
>>>> +static const struct starfive_wdt_variant drv_data_jh7110 = {
>>>> +       .control = STARFIVE_WDT_JH7110_CONTROL,
>>>> +       .load = STARFIVE_WDT_JH7110_LOAD,
>>>> +       .enable = STARFIVE_WDT_JH7110_CONTROL,
>>>> +       .value = STARFIVE_WDT_JH7110_VALUE,
>>>> +       .int_clr = STARFIVE_WDT_JH7110_INTCLR,
>>>> +       .unlock = STARFIVE_WDT_JH7110_LOCK,
>>>> +       .unlock_key = STARFIVE_WDT_JH7110_UNLOCK_KEY,
>>>> +       .irq_is_raise = STARFIVE_WDT_JH7110_IMS,
>>>> +       .enrst_shift = STARFIVE_WDT_JH7110_RESEN_SHIFT,
>>>> +       .en_shift = STARFIVE_WDT_JH7110_EN_SHIFT,
>>>> +};
>>>> +
>>>> +static const struct of_device_id starfive_wdt_match[] = {
>>>> +       { .compatible = "starfive,jh7110-wdt", .data = &drv_data_jh7110 },
>>>> +       {}
>>>
>>> I like the idiom of ending with { /* sentinel */ } here. Also it is
>>> common to place this struct just above the platform_driver struct
>>> which is the only user.
>>
>> Will add it.
>>
>>>
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, starfive_wdt_match);
>>>> +
>>>> +static const struct platform_device_id starfive_wdt_ids[] = {
>>>> +       {
>>>> +               .name = "starfive-jh7110-wdt",
>>>> +               .driver_data = (unsigned long)&drv_data_jh7110,
>>>> +       },
>>>> +       {}
>>>> +};
>>>> +MODULE_DEVICE_TABLE(platform, starfive_wdt_ids);
>>>
>>> When is this struct used? So far I only know of device tree enabled users.
>>
>> The struct will be used when CONFIG_OF is disabled
>> but I deleted CONFIG_OF by mistake.
>> Should I add the CONFIG_OF back or remove this struct?
> 
> Yeah, as far as I know neither the JH7110 nor JH7100 boots without
> CONFIG_OF, which leaves this struct unused. So please remove it.
> 
>>>
>>>> +static int starfive_wdt_get_clock_rate(struct starfive_wdt *wdt)
>>>> +{
>>>> +       wdt->freq = clk_get_rate(wdt->core_clk);
>>>> +       /* The clock rate should not be 0.*/
>>>> +       if (wdt->freq)
>>>> +               return 0;
>>>> +
>>>> +       dev_err(wdt->dev, "get clock rate failed.\n");
>>>> +       return -ENOENT;
>>>> +}
>>>> +
>>>> +static int starfive_wdt_get_clock(struct starfive_wdt *wdt)
>>>> +{
>>>> +       wdt->apb_clk = devm_clk_get(wdt->dev, "apb");
>>>> +       if (IS_ERR(wdt->apb_clk)) {
>>>> +               dev_err(wdt->dev, "failed to get apb clock.\n");
>>>> +               return PTR_ERR(wdt->apb_clk);
>>>> +       }
>>>> +
>>>> +       wdt->core_clk = devm_clk_get(wdt->dev, "core");
>>>> +       if (IS_ERR(wdt->core_clk)) {
>>>> +               dev_err(wdt->dev, "failed to get core clock.\n");
>>>> +               return PTR_ERR(wdt->core_clk);
>>>> +       }
>>>
>>> Here and above you can use dev_err_probe which will also not print
>>> errors on -EPROBE_DEFER.
>>
>> Will modify it.
>>
>>>
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int starfive_wdt_reset_init(struct starfive_wdt *wdt)
>>>> +{
>>>> +       int ret = 0;
>>>> +
>>>> +       wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
>>>> +       if (IS_ERR(wdt->rsts)) {
>>>> +               dev_err(wdt->dev, "failed to get rsts error.\n");
>>>> +               ret = PTR_ERR(wdt->rsts);
>>>> +       } else {
>>>> +               ret = reset_control_deassert(wdt->rsts);
>>>> +               if (ret)
>>>> +                       dev_err(wdt->dev, "failed to deassert rsts.\n");
>>>> +       }
>>>> +       return ret;
>>>> +}
>>>
>>> How about something like
>>> int ret;
>>>
>>> wdt->rsts = devm_reset_control_array_get_exclusive(wdt->dev);
>>> if (IS_ERR(wdt->rsts))
>>>      return dev_err_probe(wdt->dev, PTR_ERR(wdt->rsts), "failed to get
>>> resets\n");
>>>
>>> ret = reset_control_deassert(wdt->rsts);
>>> return dev_err_probe(wdt->dev, ret, "failed to deassert resets\n");
>>
>> It looks more concise. Thanks.
>>
>>>
>>>> +
>>>> +static u32 starfive_wdt_ticks_to_sec(struct starfive_wdt *wdt, u32 ticks)
>>>> +{
>>>> +       return DIV_ROUND_CLOSEST(ticks, wdt->freq);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Write unlock-key to unlock. Write other value to lock. When lock bit is 1,
>>>> + * external accesses to other watchdog registers are ignored.
>>>> + */
>>>> +static bool starfive_wdt_is_locked(struct starfive_wdt *wdt)
>>>> +{
>>>> +       u32 val;
>>>> +
>>>> +       val = readl(wdt->base + wdt->drv_data->unlock);
>>>> +       return !!(val & STARFIVE_WDT_LOCKED);
>>>> +}
>>>> +
>>>> +static void starfive_wdt_unlock(struct starfive_wdt *wdt)
>>>> +{
>>>> +       if (starfive_wdt_is_locked(wdt))
>>>> +               writel(wdt->drv_data->unlock_key,
>>>> +                      wdt->base + wdt->drv_data->unlock);
>>>> +}
>>>> +
>>>> +static void starfive_wdt_lock(struct starfive_wdt *wdt)
>>>> +{
>>>> +       if (!starfive_wdt_is_locked(wdt))
>>>> +               writel(~wdt->drv_data->unlock_key,
>>>> +                      wdt->base + wdt->drv_data->unlock);
>>>> +}
>>>> +
>>>> +/* enable watchdog interrupt to reset/reboot */
>>>> +static void starfive_wdt_enable_reset(struct starfive_wdt *wdt)
>>>> +{
>>>> +       u32 val;
>>>> +
>>>> +       val = readl(wdt->base + wdt->drv_data->control);
>>>> +       val |= STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift;
>>>> +       writel(val, wdt->base + wdt->drv_data->control);
>>>> +}
>>>> +
>>>> +/* disable watchdog interrupt to reset/reboot */
>>>> +static void starfive_wdt_disable_reset(struct starfive_wdt *wdt)
>>>> +{
>>>> +       u32 val;
>>>> +
>>>> +       val = readl(wdt->base + wdt->drv_data->control);
>>>> +       val &= ~(STARFIVE_WDT_RESET_EN << wdt->drv_data->enrst_shift);
>>>> +       writel(val, wdt->base + wdt->drv_data->control);
>>>> +}
>>>> +
>>>> +/* interrupt status whether has been raised from the counter */
>>>> +static bool starfive_wdt_raise_irq_status(struct starfive_wdt *wdt)
>>>> +{
>>>> +       return !!readl(wdt->base + wdt->drv_data->irq_is_raise);
>>>> +}
>>>> +
>>>> +/* clear interrupt signal before initialization or reload */
>>>> +static void starfive_wdt_int_clr(struct starfive_wdt *wdt)
>>>> +{
>>>> +       writel(STARFIVE_WDT_INTCLR, wdt->base + wdt->drv_data->int_clr);
>>>> +}
>>>> +
>>>> +static inline void starfive_wdt_set_count(struct starfive_wdt *wdt, u32 val)
>>>> +{
>>>> +       writel(val, wdt->base + wdt->drv_data->load);
>>>> +}
>>>> +
>>>> +static inline u32 starfive_wdt_get_count(struct starfive_wdt *wdt)
>>>> +{
>>>> +       return readl(wdt->base + wdt->drv_data->value);
>>>> +}
>>>> +
>>>> +/* enable watchdog */
>>>> +static inline void starfive_wdt_enable(struct starfive_wdt *wdt)
>>>> +{
>>>> +       u32 val;
>>>> +
>>>> +       val = readl(wdt->base + wdt->drv_data->enable);
>>>> +       val |= STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift;
>>>> +       writel(val, wdt->base + wdt->drv_data->enable);
>>>> +}
>>>> +
>>>> +/* disable watchdog */
>>>> +static inline void starfive_wdt_disable(struct starfive_wdt *wdt)
>>>> +{
>>>> +       u32 val;
>>>> +
>>>> +       val = readl(wdt->base + wdt->drv_data->enable);
>>>> +       val &= ~(STARFIVE_WDT_ENABLE << wdt->drv_data->en_shift);
>>>> +       writel(val, wdt->base + wdt->drv_data->enable);
>>>> +}
>>>> +
>>>> +static inline void starfive_wdt_set_reload_count(struct starfive_wdt *wdt, u32 count)
>>>> +{
>>>> +       starfive_wdt_set_count(wdt, count);
>>>> +       /* need enable controller to reload counter */
>>>> +       starfive_wdt_enable(wdt);
>>>> +}
>>>> +
>>>> +static unsigned int starfive_wdt_max_timeout(struct starfive_wdt *wdt)
>>>> +{
>>>> +       return DIV_ROUND_UP(STARFIVE_WDT_MAXCNT, (wdt->freq / 2)) - 1;
>>>> +}
>>>> +
>>>> +static unsigned int starfive_wdt_get_timeleft(struct watchdog_device *wdd)
>>>> +{
>>>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>>> +       u32 count;
>>>> +
>>>> +       starfive_wdt_unlock(wdt);
>>>> +       /*
>>>> +        * Because set half count value,
>>>> +        * timeleft value should add the count value before first timeout.
>>>> +        */
>>>> +       count = starfive_wdt_get_count(wdt);
>>>> +       if (!starfive_wdt_raise_irq_status(wdt))
>>>> +               count += wdt->count;
>>>> +
>>>> +       starfive_wdt_lock(wdt);
>>>> +
>>>> +       return starfive_wdt_ticks_to_sec(wdt, count);
>>>> +}
>>>> +
>>>> +static int starfive_wdt_keepalive(struct watchdog_device *wdd)
>>>> +{
>>>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>>> +
>>>> +       spin_lock(&wdt->lock);
>>>> +
>>>> +       starfive_wdt_unlock(wdt);
>>>> +       starfive_wdt_int_clr(wdt);
>>>> +       starfive_wdt_set_reload_count(wdt, wdt->count);
>>>> +       starfive_wdt_lock(wdt);
>>>> +
>>>> +       spin_unlock(&wdt->lock);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int starfive_wdt_stop(struct watchdog_device *wdd)
>>>> +{
>>>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>>> +
>>>> +       spin_lock(&wdt->lock);
>>>> +
>>>> +       starfive_wdt_unlock(wdt);
>>>> +       starfive_wdt_disable_reset(wdt);
>>>> +       starfive_wdt_int_clr(wdt);
>>>> +       starfive_wdt_disable(wdt);
>>>> +       starfive_wdt_lock(wdt);
>>>> +
>>>> +       spin_unlock(&wdt->lock);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int starfive_wdt_pm_stop(struct watchdog_device *wdd)
>>>> +{
>>>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>>> +
>>>> +       starfive_wdt_stop(wdd);
>>>> +       pm_runtime_put_sync(wdt->dev);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int starfive_wdt_start(struct watchdog_device *wdd)
>>>> +{
>>>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>>> +
>>>> +       spin_lock(&wdt->lock);
>>>> +       starfive_wdt_unlock(wdt);
>>>> +       /* disable watchdog, to be safe */
>>>> +       starfive_wdt_disable(wdt);
>>>> +
>>>> +       starfive_wdt_enable_reset(wdt);
>>>> +       starfive_wdt_int_clr(wdt);
>>>> +       starfive_wdt_set_count(wdt, wdt->count);
>>>> +       starfive_wdt_enable(wdt);
>>>> +
>>>> +       starfive_wdt_lock(wdt);
>>>> +       spin_unlock(&wdt->lock);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int starfive_wdt_pm_start(struct watchdog_device *wdd)
>>>> +{
>>>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>>> +
>>>> +       pm_runtime_get_sync(wdt->dev);
>>>> +
>>>> +       return starfive_wdt_start(wdd);
>>>> +}
>>>> +
>>>> +static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
>>>> +                                   unsigned int timeout)
>>>> +{
>>>> +       struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>>>> +       unsigned long freq = wdt->freq;
>>>> +
>>>> +       spin_lock(&wdt->lock);
>>>> +
>>>> +       /*
>>>> +        * This watchdog takes twice timeouts to reset.
>>>> +        * In order to reduce time to reset, should set half count value.
>>>> +        */
>>>> +       wdt->count = timeout * freq / 2;
>>>> +       wdd->timeout = timeout;
>>>> +
>>>> +       starfive_wdt_unlock(wdt);
>>>> +       starfive_wdt_disable(wdt);
>>>> +       starfive_wdt_set_reload_count(wdt, wdt->count);
>>>> +       starfive_wdt_enable(wdt);
>>>> +       starfive_wdt_lock(wdt);
>>>> +
>>>> +       spin_unlock(&wdt->lock);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
>>>> +
>>>> +static const struct watchdog_info starfive_wdt_ident = {
>>>> +       .options = OPTIONS,
>>>> +       .identity = "StarFive Watchdog",
>>>> +};
>>>> +
>>>> +static const struct watchdog_ops starfive_wdt_ops = {
>>>> +       .owner = THIS_MODULE,
>>>> +       .start = starfive_wdt_pm_start,
>>>> +       .stop = starfive_wdt_pm_stop,
>>>> +       .ping = starfive_wdt_keepalive,
>>>> +       .set_timeout = starfive_wdt_set_timeout,
>>>> +       .get_timeleft = starfive_wdt_get_timeleft,
>>>> +};
>>>> +
>>>> +static const struct watchdog_device starfive_wdd = {
>>>> +       .info = &starfive_wdt_ident,
>>>> +       .ops = &starfive_wdt_ops,
>>>> +       .timeout = STARFIVE_WDT_DEFAULT_TIME,
>>>> +};
>>>> +
>>>> +static inline const struct starfive_wdt_variant *
>>>> +starfive_wdt_get_drv_data(struct platform_device *pdev)
>>>> +{
>>>> +       const struct starfive_wdt_variant *variant;
>>>> +
>>>> +       variant = of_device_get_match_data(&pdev->dev);
>>>> +       if (!variant) {
>>>> +               /* Device matched by platform_device_id */
>>>> +               variant = (struct starfive_wdt_variant *)
>>>> +                          platform_get_device_id(pdev)->driver_data;
>>>> +       }
>>>> +
>>>> +       return variant;
>>>> +}
>>>
>>> If this driver is only used through the device tree this whole
>>> function can just be a call to of_device_get_match_data()
>>
>> Well, if remove the platform_device_id struct, just use of_device_get_match_data().
>>
>>>
>>>> +static int starfive_wdt_probe(struct platform_device *pdev)
>>>> +{
>>>> +       struct device *dev = &pdev->dev;
>>>> +       struct starfive_wdt *wdt;
>>>> +       int ret;
>>>> +
>>>> +       wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>>> +       if (!wdt)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       wdt->dev = dev;
>>>> +       spin_lock_init(&wdt->lock);
>>>> +       wdt->wdt_device = starfive_wdd;
>>>> +
>>>> +       wdt->drv_data = starfive_wdt_get_drv_data(pdev);
>>>> +
>>>> +       /* get the memory region for the watchdog timer */
>>>> +       wdt->base = devm_platform_ioremap_resource(pdev, 0);
>>>> +       if (IS_ERR(wdt->base)) {
>>>> +               ret = PTR_ERR(wdt->base);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       platform_set_drvdata(pdev, wdt);
>>>> +       pm_runtime_enable(wdt->dev);
>>>> +
>>>> +       ret = starfive_wdt_get_clock(wdt);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +       if (pm_runtime_enabled(wdt->dev)) {
>>>> +               ret = pm_runtime_get_sync(wdt->dev);
>>>> +               if (ret < 0)
>>>> +                       return ret;
>>>> +       } else {
>>>> +               /* runtime PM is disabled but clocks need to be enabled */
>>>> +               ret = clk_prepare_enable(wdt->apb_clk);
>>>> +               if (ret) {
>>>> +                       dev_err(wdt->dev, "failed to enable apb_clk.\n");
>>>> +                       return ret;
>>>> +               }
>>>> +               ret = clk_prepare_enable(wdt->core_clk);
>>>> +               if (ret) {
>>>> +                       dev_err(wdt->dev, "failed to enable core_clk.\n");
>>>> +                       goto err_apb_clk_disable;
>>>> +               }
>>>> +       }
>>>
>>> The else part is basically starfive_wdt_runtime_resume(). Maybe just call that.
>>
>> The starfive_wdt_runtime_resume() is defined under CONFIG_PM and it could not
>> be used directly. I should use a new function that enable/disable clock and
>> can be used in starfive_wdt_runtime_resume() and here.
> 
> Yes, either that or just define that function outside #ifdef CONFIG_PM.
> 
>>>> +
>>>> +       watchdog_set_drvdata(&wdt->wdt_device, wdt);
>>>> +
>>>> +       /*
>>>> +        * see if we can actually set the requested heartbeat,
>>>> +        * and if not, try the default value.
>>>> +        */
>>>> +       watchdog_init_timeout(&wdt->wdt_device, heartbeat, dev);
>>>> +       if (wdt->wdt_device.timeout == 0 ||
>>>> +           wdt->wdt_device.timeout > wdt->wdt_device.max_timeout) {
>>>> +               dev_warn(dev, "heartbeat value out of range, default %d used\n",
>>>> +                        STARFIVE_WDT_DEFAULT_TIME);
>>>> +               wdt->wdt_device.timeout = STARFIVE_WDT_DEFAULT_TIME;
>>>> +       }
>>>> +       starfive_wdt_set_timeout(&wdt->wdt_device, wdt->wdt_device.timeout);
>>>> +
>>>> +       watchdog_set_nowayout(&wdt->wdt_device, nowayout);
>>>> +       watchdog_stop_on_reboot(&wdt->wdt_device);
>>>> +       watchdog_stop_on_unregister(&wdt->wdt_device);
>>>> +
>>>> +       wdt->wdt_device.parent = dev;
>>>> +
>>>> +       ret = watchdog_register_device(&wdt->wdt_device);
>>>> +       if (ret)
>>>> +               goto err_clk_disable;
>>>> +
>>>> +       if (early_enable) {
>>>> +               starfive_wdt_start(&wdt->wdt_device);
>>>> +               set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
>>>> +       } else {
>>>> +               starfive_wdt_stop(&wdt->wdt_device);
>>>> +       }
>>>> +
>>>> +       pm_runtime_put_sync(wdt->dev);
>>>> +
>>>> +       return 0;
>>>> +
>>>> +err_clk_disable:
>>>> +       clk_disable_unprepare(wdt->core_clk);
>>>> +err_apb_clk_disable:
>>>> +       clk_disable_unprepare(wdt->apb_clk);
>>>> +       pm_runtime_disable(wdt->dev);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +static int starfive_wdt_remove(struct platform_device *dev)
>>>> +{
>>>> +       struct starfive_wdt *wdt = platform_get_drvdata(dev);
>>>> +
>>>> +       starfive_wdt_stop(&wdt->wdt_device);
>>>> +       watchdog_unregister_device(&wdt->wdt_device);
>>>> +
>>>> +       if (pm_runtime_enabled(wdt->dev)) {
>>>> +               pm_runtime_disable(wdt->dev);
>>>> +       } else {
>>>> +               /* disable clock without PM */
>>>> +               clk_disable_unprepare(wdt->core_clk);
>>>> +               clk_disable_unprepare(wdt->apb_clk);
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static void starfive_wdt_shutdown(struct platform_device *dev)
>>>> +{
>>>> +       struct starfive_wdt *wdt = platform_get_drvdata(dev);
>>>> +
>>>> +       starfive_wdt_pm_stop(&wdt->wdt_device);
>>>> +}
>>>> +
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int starfive_wdt_suspend(struct device *dev)
>>>> +{
>>>> +       int ret;
>>>> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
>>>> +
>>>> +       starfive_wdt_unlock(wdt);
>>>> +
>>>> +       /* Save watchdog state, and turn it off. */
>>>> +       wdt->reload = starfive_wdt_get_count(wdt);
>>>> +
>>>> +       /* Note that WTCNT doesn't need to be saved. */
>>>> +       starfive_wdt_stop(&wdt->wdt_device);
>>>> +       pm_runtime_force_suspend(dev);
>>>> +
>>>> +       starfive_wdt_lock(wdt);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int starfive_wdt_resume(struct device *dev)
>>>> +{
>>>> +       int ret;
>>>> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
>>>> +
>>>> +       starfive_wdt_unlock(wdt);
>>>> +
>>>> +       pm_runtime_force_resume(dev);
>>>> +
>>>> +       /* Restore watchdog state. */
>>>> +       starfive_wdt_set_reload_count(wdt, wdt->reload);
>>>> +
>>>> +       starfive_wdt_start(&wdt->wdt_device);
>>>> +
>>>> +       starfive_wdt_lock(wdt);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +#endif /* CONFIG_PM_SLEEP */
>>>> +
>>>> +#ifdef CONFIG_PM
>>>> +static int starfive_wdt_runtime_suspend(struct device *dev)
>>>> +{
>>>> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
>>>> +
>>>> +       clk_disable_unprepare(wdt->apb_clk);
>>>> +       clk_disable_unprepare(wdt->core_clk);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int starfive_wdt_runtime_resume(struct device *dev)
>>>> +{
>>>> +       struct starfive_wdt *wdt = dev_get_drvdata(dev);
>>>> +       int ret;
>>>> +
>>>> +       ret = clk_prepare_enable(wdt->apb_clk);
>>>> +       if (ret) {
>>>> +               dev_err(wdt->dev, "failed to enable apb_clk.\n");
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       ret = clk_prepare_enable(wdt->core_clk);
>>>> +       if (ret)
>>>> +               dev_err(wdt->dev, "failed to enable core_clk.\n");
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +#endif /* CONFIG_PM */
>>>> +
>>>> +static const struct dev_pm_ops starfive_wdt_pm_ops = {
>>>> +       SET_RUNTIME_PM_OPS(starfive_wdt_runtime_suspend, starfive_wdt_runtime_resume, NULL)
>>>> +       SET_SYSTEM_SLEEP_PM_OPS(starfive_wdt_suspend, starfive_wdt_resume)
>>>> +};
>>>> +
>>>> +static struct platform_driver starfive_wdt_driver = {
>>>> +       .probe          = starfive_wdt_probe,
>>>> +       .remove         = starfive_wdt_remove,
>>>> +       .shutdown       = starfive_wdt_shutdown,
>>>> +       .id_table       = starfive_wdt_ids,
>>>> +       .driver         = {
>>>> +               .name   = "starfive-wdt",
>>>> +               .pm     = &starfive_wdt_pm_ops,
>>>> +               .of_match_table = of_match_ptr(starfive_wdt_match),
>>>> +       },
>>>> +};
>>>> +
>>>> +module_platform_driver(starfive_wdt_driver);
>>>> +
>>>> +MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
>>>> +MODULE_AUTHOR("Samin Guo <samin.guo@starfivetech.com>");
>>>> +MODULE_DESCRIPTION("StarFive Watchdog Device Driver");
>>>> +MODULE_LICENSE("GPL");
>>>> --
>>>> 2.25.1
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-riscv mailing list
>>>> linux-riscv@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>
>> Best regards,
>> Xingyu Wu
>>

Guenter Roeck Feb. 28, 2023, 3:10 p.m. UTC | #20

On 2/28/23 05:16, Xingyu Wu wrote:
> On 2023/2/28 19:11, Conor Dooley wrote:
>> On Tue, Feb 28, 2023 at 11:57:58AM +0100, Krzysztof Kozlowski wrote:
>>> On 28/02/2023 11:51, Conor Dooley wrote:
>>>> On Tue, Feb 28, 2023 at 11:36:49AM +0100, Emil Renner Berthing wrote:
>>>>> On Tue, 28 Feb 2023 at 10:44, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>>>>>> On 2023/2/26 22:14, Emil Renner Berthing wrote:
>>>>>>> On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>>>>
>>>>>> So the dt-bingdings need to rename, and which one could be better,
>>>>>> 'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?
>>>>>
>>>>> Sure, starfive,jh71x0-wdt.yaml sounds good to me.
>>>>
>>>> I feel like a common comment I see from the dt folks is to not put
>>>> wildcards in filenames & just pick the first compatible.
>>>> I could very well be wrong on that front though...
>>>
>>> First compatible is a bit better, unless you are sure this will cover
>>> all such compatibles now and in the future. For many bindings the
>>> family/wildcards were fine in filename.
>>
>> Ahh cool, good to know what the specific policy is - thanks!
> 
> If this watchdog driver is improved to also support JH7100 in next patch,
> it seems more reasonable to rename the dt-bingdings to 'starfive,jh71x0-wdt.yaml'.


Up to the devicetree maintainers to decide, but I for my part never accept
wildcards in file names. You can not guarantee that all of jh71[0-9]0 will
be supported by this set of bindings. On top of that, when / if you add
support for anything outside that range (say, jh7200 or jh8100 or jh7101
or whatever) you'd have an even worse problem. Are you then going to suggest
renaming the file to jhxxxx-wdt ? Or one digit at a time ?

Guenter

Emil Renner Berthing Feb. 28, 2023, 3:32 p.m. UTC | #21

On Tue, 28 Feb 2023 at 16:10, Guenter Roeck <linux@roeck-us.net> wrote:
> On 2/28/23 05:16, Xingyu Wu wrote:
> > On 2023/2/28 19:11, Conor Dooley wrote:
> >> On Tue, Feb 28, 2023 at 11:57:58AM +0100, Krzysztof Kozlowski wrote:
> >>> On 28/02/2023 11:51, Conor Dooley wrote:
> >>>> On Tue, Feb 28, 2023 at 11:36:49AM +0100, Emil Renner Berthing wrote:
> >>>>> On Tue, 28 Feb 2023 at 10:44, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
> >>>>>> On 2023/2/26 22:14, Emil Renner Berthing wrote:
> >>>>>>> On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
> >>>>
> >>>>>> So the dt-bingdings need to rename, and which one could be better,
> >>>>>> 'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?
> >>>>>
> >>>>> Sure, starfive,jh71x0-wdt.yaml sounds good to me.
> >>>>
> >>>> I feel like a common comment I see from the dt folks is to not put
> >>>> wildcards in filenames & just pick the first compatible.
> >>>> I could very well be wrong on that front though...
> >>>
> >>> First compatible is a bit better, unless you are sure this will cover
> >>> all such compatibles now and in the future. For many bindings the
> >>> family/wildcards were fine in filename.
> >>
> >> Ahh cool, good to know what the specific policy is - thanks!
> >
> > If this watchdog driver is improved to also support JH7100 in next patch,
> > it seems more reasonable to rename the dt-bingdings to 'starfive,jh71x0-wdt.yaml'.
>
>
> Up to the devicetree maintainers to decide, but I for my part never accept
> wildcards in file names. You can not guarantee that all of jh71[0-9]0 will
> be supported by this set of bindings. On top of that, when / if you add
> support for anything outside that range (say, jh7200 or jh8100 or jh7101
> or whatever) you'd have an even worse problem. Are you then going to suggest
> renaming the file to jhxxxx-wdt ? Or one digit at a time ?

Makes sense to me, in which case this should be called
starfive,jh7100-wdt since that's the first SoC to feature this
watchdog and will hopefully be supported in the next version of this
patchset.

/Emil

Xingyu Wu March 1, 2023, 3:40 a.m. UTC | #22

On 2023/2/28 23:32, Emil Renner Berthing wrote:
> On Tue, 28 Feb 2023 at 16:10, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 2/28/23 05:16, Xingyu Wu wrote:
>> > On 2023/2/28 19:11, Conor Dooley wrote:
>> >> On Tue, Feb 28, 2023 at 11:57:58AM +0100, Krzysztof Kozlowski wrote:
>> >>> On 28/02/2023 11:51, Conor Dooley wrote:
>> >>>> On Tue, Feb 28, 2023 at 11:36:49AM +0100, Emil Renner Berthing wrote:
>> >>>>> On Tue, 28 Feb 2023 at 10:44, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>> >>>>>> On 2023/2/26 22:14, Emil Renner Berthing wrote:
>> >>>>>>> On Mon, 20 Feb 2023 at 09:21, Xingyu Wu <xingyu.wu@starfivetech.com> wrote:
>> >>>>
>> >>>>>> So the dt-bingdings need to rename, and which one could be better,
>> >>>>>> 'starfive,jh71x0-wdt.yaml' or 'starfive,jh-wdt.yaml'?
>> >>>>>
>> >>>>> Sure, starfive,jh71x0-wdt.yaml sounds good to me.
>> >>>>
>> >>>> I feel like a common comment I see from the dt folks is to not put
>> >>>> wildcards in filenames & just pick the first compatible.
>> >>>> I could very well be wrong on that front though...
>> >>>
>> >>> First compatible is a bit better, unless you are sure this will cover
>> >>> all such compatibles now and in the future. For many bindings the
>> >>> family/wildcards were fine in filename.
>> >>
>> >> Ahh cool, good to know what the specific policy is - thanks!
>> >
>> > If this watchdog driver is improved to also support JH7100 in next patch,
>> > it seems more reasonable to rename the dt-bingdings to 'starfive,jh71x0-wdt.yaml'.
>>
>>
>> Up to the devicetree maintainers to decide, but I for my part never accept
>> wildcards in file names. You can not guarantee that all of jh71[0-9]0 will
>> be supported by this set of bindings. On top of that, when / if you add
>> support for anything outside that range (say, jh7200 or jh8100 or jh7101
>> or whatever) you'd have an even worse problem. Are you then going to suggest
>> renaming the file to jhxxxx-wdt ? Or one digit at a time ?
> 
> Makes sense to me, in which case this should be called
> starfive,jh7100-wdt since that's the first SoC to feature this
> watchdog and will hopefully be supported in the next version of this
> patchset.
> 

Thanks for your suggestions. I will use starfive,jh7100-wdt and improve this
driver to support 7100 in the next patchset.

Best regards,
Xingyu Wu

[v3,2/2] drivers: watchdog: Add StarFive Watchdog driver

Commit Message

Comments

Patch