Re: [PATCH v1 1/3] dt-bindings: watchdog: Add watchdog for StarFive

Re: [PATCH v1 1/3] dt-bindings: watchdog: Add watchdog for StarFive

[Krzysztof Kozlowski]

On 02/12/2022 10:39, xingu.wu wrote:
> From: Xingyu Wu <xingyu.wu@starfivetech.com>
> 
> Add bindings to describe the watchdog for the StarFive SoCs.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  .../bindings/watchdog/starfive,wdt.yaml       | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml b/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
> new file mode 100644
> index 000000000000..ece3e80153a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml

Filename should be based on compatible. You do not allow here any other
models... If you intent and you are 100% sure you will grow with new
models, make it maybe a family-based name.


> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/starfive,wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive Watchdog
> +
> +allOf:
> +  - $ref: "watchdog.yaml#"

Drop quotes.

> +
> +maintainers:
> +  - Samin Guo <samin.guo@starfivetech.com>
> +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-wdt
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Core clock
> +      - description: APB clock
> +
> +  clock-names:
> +    items:
> +      - const: core_clk

Drop _clk

> +      - const: apb_clk

Drop _clk

> +
> +  resets:
> +    items:
> +      - description: APB reset
> +      - description: Core reset
> +
> +  reset-names:
> +    items:
> +      - const: rst_apb

Drop rst_

> +      - const: rst_core

Ditto

> +
> +  timeout-sec: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - timeout-sec
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/starfive-jh7110.h>
> +    #include <dt-bindings/reset/starfive-jh7110.h>
> +
> +    watchdog@13070000 {
> +            compatible = "starfive,jh7110-wdt";

Use 4 spaces for example indentation.

> +            reg = <0x13070000 0x10000>;
> +            interrupts = <68>;
> +            clocks = <&syscrg_clk JH7110_SYSCLK_WDT_CORE>,
> +                     <&syscrg_clk JH7110_SYSCLK_WDT_APB>;
> +            clock-names = "core_clk", "apb_clk";
> +            resets = <&syscrg_rst JH7110_SYSRST_WDT_APB>,
> +                     <&syscrg_rst JH7110_SYSRST_WDT_CORE>;
> +            reset-names = "rst_apb", "rst_core";
> +            timeout-sec = <15>;
> +    };
> +

Drop trailing line.

Best regards,
Krzysztof

Re: [PATCH v1 3/3] riscv: dts: starfive: jh7110: Add watchdog node

[Krzysztof Kozlowski]

On 02/12/2022 10:39, xingu.wu wrote:
> From: Xingyu Wu <xingyu.wu@starfivetech.com>
> 
> This adds the watchdog node for the Starfive JH7110 SoC.

Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index c22e8f1d2640..22f5a37d691e 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -433,5 +433,19 @@ uart5: serial@12020000 {
>  			reg-shift = <2>;
>  			status = "disabled";
>  		};
> +
> +		wdog: watchdog@13070000 {
> +			compatible = "starfive,jh7110-wdt";
> +			reg = <0x0 0x13070000 0x0 0x10000>;
> +			interrupts = <68>;
> +			clocks = <&syscrg JH7110_SYSCLK_WDT_CORE>,
> +				 <&syscrg JH7110_SYSCLK_WDT_APB>;
> +			clock-names = "core_clk", "apb_clk";
> +			resets = <&syscrg JH7110_SYSRST_WDT_APB>,
> +				 <&syscrg JH7110_SYSRST_WDT_CORE>;
> +			reset-names = "rst_apb", "rst_core";
> +			timeout-sec = <15>;
> +			status = "okay";

Why? okay is by default

Best regards,
Krzysztof

Re: [PATCH v1 0/3] Add watchdog driver for StarFive RISC-V SoCs

[Conor Dooley]

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

Hello!

+CC Hal Feng

On Fri, Dec 02, 2022 at 05:39:40PM +0800, xingu.wu wrote:
> This patch serises are to add watchdog driver for the StarFive RISC-V SoCs
> such as JH7110. The first patch adds docunmentation to describe device
> tree bindings. The subsequent patch adds watchdog driver and support
> JH7110 SoC. The last patch adds device node about watchdog to JH7110
> dts.
> 
> The watchdog driver has been tested on the VisionFive 2 boards which
> equip with JH7110 SoC and works normally.
> 
> This patchset should be applied after the patchset [1], [2], [3]:
> [1] https://lore.kernel.org/all/20221118010627.70576-1-hal.feng@starfivetech.com/
> [2] https://lore.kernel.org/all/20221118011108.70715-1-hal.feng@starfivetech.com/
> [3] https://lore.kernel.org/all/20221118011714.70877-1-hal.feng@starfivetech.com/

You say "patchset" but that's not really the case. Realistically, only
one patch here has a real dependency on the above AFAICT.
How about for v2, whenever you send that, in your dt-bindings you drop
the uses of dt-bindings/*/starfive-jh7110.h & instead put the numbers?
Same goes, IMO, for ~all the jh7110 dt-bindings, see here:
https://lore.kernel.org/all/624cba7f-1be9-7100-91d7-f9232c855d9f@linaro.org/

Then the only person that has to care about prerequisite patches is then
me for the dt patch and you will not have the driver itself delayed
until the clock bindings are merged?

Perhaps Guenter likes having the defines, but my opinion would be get as
much as possible that does not have real dependencies on the clock &
reset bindings capable of being merged in isolation.

Thanks,
Conor.

> 
> Xingyu Wu (3):
>   dt-bindings: watchdog: Add watchdog for StarFive
>   drivers: watchdog: Add StarFive Watchdog driver
>   riscv: dts: starfive: jh7110: Add watchdog node
> 
>  .../bindings/watchdog/starfive,wdt.yaml       |  77 ++
>  MAINTAINERS                                   |   7 +
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      |  14 +
>  drivers/watchdog/Kconfig                      |  11 +
>  drivers/watchdog/Makefile                     |   3 +
>  drivers/watchdog/starfive-wdt.c               | 854 ++++++++++++++++++
>  6 files changed, 966 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>  create mode 100644 drivers/watchdog/starfive-wdt.c
> 
> 
> base-commit: 094226ad94f471a9f19e8f8e7140a09c2625abaa
> prerequisite-patch-id: 6b1b43a55b9773bec61ab6c1bbaa54dccbac0837
> prerequisite-patch-id: 09c98554df52d17ba5fd604125f8cdd62cbe80d1
> prerequisite-patch-id: 29fe0b0c19b6f0cd31114ee9fe17fe9732047f33
> prerequisite-patch-id: c59d9908de90e09ba2b9a81aadbf9fb9f00c8f04
> prerequisite-patch-id: 94ac03d518993921bcfc9cc9f58d7da0c3528b51
> prerequisite-patch-id: 694f7400375f5b85581fc1821e427334507826f2
> prerequisite-patch-id: 699d49c4439dadb4b7cf900857f027d050cd6093
> prerequisite-patch-id: 40d773f5a19912f731ee5fd4739ed2e3c2157b07
> prerequisite-patch-id: 2bc3fd6df5dda116efe882045863d6c88aa81b3a
> prerequisite-patch-id: 735e62255c75801bdc4c0b4107850bce821ff7f5
> prerequisite-patch-id: b2a923b922e661fa6085185f33c1f1e733db9110
> prerequisite-patch-id: b2bbc28354075432f059344eba5a127a653475cf
> prerequisite-patch-id: 70eab7b7eee728afcd90e40f6743d1356f6d81ab
> prerequisite-patch-id: 6276b2a23818c65ff2ad3d65b562615690cffee9
> prerequisite-patch-id: d834ece14ffb525b8c3e661e78736692f33fca9b
> prerequisite-patch-id: 4c17a3ce4dae9b788795d915bf775630f5c43c53
> prerequisite-patch-id: dabb913fd478e97593e45c23fee4be9fd807f851
> prerequisite-patch-id: 22fa141f7f0f80a5d619e9f3f4cf161ad06f108e
> prerequisite-patch-id: f306819c257ea73aff8e06b17b5731053cdddfc8
> prerequisite-patch-id: 536114f0732646095ef5302a165672b3290d4c75
> prerequisite-patch-id: 258ea5f9b8bf41b6981345dcc81795f25865d38f
> prerequisite-patch-id: 8b6f2c9660c0ac0ee4e73e4c21aca8e6b75e81b9
> prerequisite-patch-id: e09e995700a814a763aa304ad3881a7222acf556
> prerequisite-patch-id: 05803238293fcc90c8e83018a1103c41133a816c
> prerequisite-patch-id: d074c7ffa2917a9f754d5801e3f67bc980f9de4c
> prerequisite-patch-id: ef23fdf3466b3c713b3826e8545c8dd2bc6cc9d7
> prerequisite-patch-id: d5da3475c6a3588e11a1678feb565bdd459b548e
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 2/3] drivers: watchdog: Add StarFive Watchdog driver

[Guenter Roeck]

On 12/2/22 01:39, xingu.wu wrote:
> From: Xingyu Wu <xingyu.wu@starfivetech.com>
> 
> Add watchdog driver for the StarFive JH7110 SoC.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>   MAINTAINERS                     |   7 +
>   drivers/watchdog/Kconfig        |  11 +
>   drivers/watchdog/Makefile       |   3 +
>   drivers/watchdog/starfive-wdt.c | 854 ++++++++++++++++++++++++++++++++
>   4 files changed, 875 insertions(+)
>   create mode 100644 drivers/watchdog/starfive-wdt.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a70c1d0f303e..133214dd2f60 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19623,6 +19623,13 @@ F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>   F:	drivers/reset/starfive/
>   F:	include/dt-bindings/reset/starfive*
>   
> +STARFIVE 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 b64bc49c7f30..658fc9fef4fa 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2082,6 +2082,17 @@ config UML_WATCHDOG
>   	tristate "UML watchdog"
>   	depends on UML || COMPILE_TEST
>   
> +# RISCV Architecture
> +
> +config STARFIVE_WATCHDOG
> +	tristate "StarFive Watchdog support"
> +	depends on RISCV
> +	select WATCHDOG_CORE
> +	default SOC_STARFIVE
> +	help
> +	  Say Y here to support the StarFive watchdog. 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 d41e5f830ae7..9c22f1a43d5c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -210,6 +210,9 @@ obj-$(CONFIG_WATCHDOG_SUN4V)		+= sun4v_wdt.o
>   # Xen
>   obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>   
> +# RISCV Architecture
> +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..dcb6e693ba6c
> --- /dev/null
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -0,0 +1,854 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Starfive Watchdog driver
> + *
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/reset-controller.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.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]: res.
> +						 */
> +#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.
> +						 */
> +#define STARFIVE_WDT_JH7110_ITCR	0xf00	/*
> +						 * RW: When set HIGH,
> +						 * places the Watchdog into integraeion test mode.\\intetration

> +						 */
> +#define STARFIVE_WDT_JH7110_ITOP	0xf04	/*
> +						 * WO: [0] WDOGRES value Integration Test Mode.
> +						 * Value output on WDOGRES when in
> +						 * Integration Test Mode.
> +						 * [1] Integration Test WDOGINT value.
> +						 * Value output on WDOGINT when in
> +						 * Integration Test Mode.
> +						 */
> +
> +#define STARFIVE_WDT_JH7110_UNLOCK_KEY		0x1acce551
> +#define STARFIVE_WDT_JH7110_RESEN_SHIFT		1
> +#define STARFIVE_WDT_JH7110_EN_SHIFT		0
> +#define STARFIVE_WDT_JH7110_INT_EN_SHIFT	STARFIVE_WDT_JH7110_EN_SHIFT
> +
> +/* WDOGCONTROL */
> +#define STARFIVE_WDT_INT_EN	0x0
> +#define STARFIVE_WDT_INT_DIS	BIT(0)
> +#define STARFIVE_WDT_RESET_EN	0x1
> +
> +/* WDOGLOCK */
> +#define STARFIVE_WDT_LOCKED	BIT(0)
> +
> +#define STARFIVE_WDT_INTCLR	0x1
> +#define STARFIVE_WDT_ENABLE	0x1
> +#define STARFIVE_WDT_ATBOOT	0x0
> +#define STARFIVE_WDT_MAXCNT	0xffffffff
> +
> +#define STARFIVE_WDT_DEFAULT_TIME	(15)
> +#define STARFIVE_WDT_DELAY_US		0
> +#define STARFIVE_WDT_TIMEOUT_US		10000
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +static int tmr_margin;
> +static int tmr_atboot = STARFIVE_WDT_ATBOOT;
> +static int soft_noboot;
> +
> +module_param(tmr_margin, int, 0);
> +module_param(tmr_atboot, int, 0);
> +module_param(nowayout, bool, 0);
> +module_param(soft_noboot, int, 0);
> +
> +MODULE_PARM_DESC(tmr_margin, "Watchdog tmr_margin in seconds. (default="
> +		 __MODULE_STRING(STARFIVE_WDT_DEFAULT_TIME) ")");
> +MODULE_PARM_DESC(tmr_atboot,
> +		 "Watchdog is started at boot time if set to 1, default="
> +		 __MODULE_STRING(STARFIVE_WDT_ATBOOT));
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +MODULE_PARM_DESC(soft_noboot,
> +		 "Watchdog action, set to 1 to ignore reboots, 0 to reboot (default 0)");
> +
> +struct starfive_wdt_variant_t {
> +	u32 unlock_key;
> +	u8 enrst_shift;
> +	u8 en_shift;
> +	u8 intclr_check;
> +	u8 intclr_ava_shift;
> +};
> +
> +struct starfive_wdt_variant {
> +	u32 control;
> +	u32 load;
> +	u32 enable;
> +	u32 reload;
> +	u32 value;
> +	u32 int_clr;
> +	u32 int_mask;
> +	u32 unlock;
> +	u32 enter_irq_status;
> +	struct starfive_wdt_variant_t *variant;
> +};
> +
> +struct starfive_wdt {
> +	u64 freq;

clk_get_rate() returns unsigned long. Why would u64 be needed here ?

> +	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 */
> +
> +	int irq;
> +};
> +
> +#ifdef CONFIG_OF
> +static struct starfive_wdt_variant_t jh7110_variant = {
> +	.unlock_key = STARFIVE_WDT_JH7110_UNLOCK_KEY,
> +	.enrst_shift = STARFIVE_WDT_JH7110_RESEN_SHIFT,
> +	.en_shift = STARFIVE_WDT_JH7110_EN_SHIFT,
> +};
> +
> +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,
> +	.enter_irq_status = STARFIVE_WDT_JH7110_IMS,
> +	.variant = &jh7110_variant,
> +};
> +

If there is no specific reason for all that complexity, drop it and introduce
it if/when necessary.

> +static const struct of_device_id starfive_wdt_match[] = {
> +	{ .compatible = "starfive,jh7110-wdt", .data = &drv_data_jh7110 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, starfive_wdt_match);
> +#endif
> +
> +static const struct platform_device_id starfive_wdt_ids[] = {
> +	{
> +		.name = "starfive-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)
> +{
> +	int ret;
> +	u32 freq;
> +
> +	if (!IS_ERR(wdt->core_clk)) {
> +		wdt->freq = clk_get_rate(wdt->core_clk);
> +		return 0;
> +	}
> +	dev_dbg(wdt->dev, "get rate failed, need clock-frequency define in dts.\n");
> +
> +	/* Next we try to get clock-frequency from dts.*/
> +	ret = of_property_read_u32(wdt->dev->of_node, "clock-frequency", &freq);
> +	if (!ret) {
> +		wdt->freq = (u64)freq;

Unnecessary typecast.

> +		return 0;
> +	}
> +	dev_err(wdt->dev, "get clock-frequency failed\n");
> +
> +	return -ENOENT;
> +}

I have never seen this kind of code before. Why would it be needed ?

> +
> +static int starfive_wdt_enable_clock(struct starfive_wdt *wdt)
> +{
> +	int ret = 0;
> +
> +	wdt->apb_clk = devm_clk_get(wdt->dev, "apb_clk");
> +	if (!IS_ERR(wdt->apb_clk)) {
> +		ret = clk_prepare_enable(wdt->apb_clk);
> +		if (ret) {
> +			dev_err(wdt->dev, "failed to enable apb_clk.\n");
> +			goto err;
> +		}
> +	} else {
> +		dev_err(wdt->dev, "failed to get apb_clk.\n");
> +		ret = PTR_ERR(wdt->apb_clk);
> +		goto err;
> +	}

Why not use devm_clk_get_enabled() ?

> +
> +	wdt->core_clk = devm_clk_get(wdt->dev, "core_clk");
> +	if (!IS_ERR(wdt->core_clk)) {
> +		ret = clk_prepare_enable(wdt->core_clk);
> +		if (ret) {
> +			dev_err(wdt->dev, "failed to enable core_clk.\n");
> +			goto err;
> +		}
> +	} else {
> +		dev_err(wdt->dev, "failed to get core_clk.\n");
> +		ret = PTR_ERR(wdt->core_clk);
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	return ret;

Unnecessary. Return directly.

> +}
> +
> +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)) {
> +		ret = reset_control_deassert(wdt->rsts);
> +		if (ret) {
> +			dev_err(wdt->dev, "failed to deassert rsts.\n");
> +			goto err;
> +		}
> +	} else {
> +		dev_err(wdt->dev, "failed to get rsts error.\n");

failed ... error is redundant. Drop "errror".

> +		ret = PTR_ERR(wdt->rsts);
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	return ret;

Unnecessary. Return directly.

> +}
> +
> +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 int starfive_wdt_is_locked(struct starfive_wdt *wdt)

bool ?

> +{
> +	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->variant->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->variant->unlock_key,
> +		       wdt->base + wdt->drv_data->unlock);
> +}
> +
> +/* enable watchdog interrupt */
> +static inline void starfive_wdt_int_enable(struct starfive_wdt *wdt)
> +{
> +	u32 val;
> +
> +	if (wdt->drv_data->int_mask) {
> +		val = readl(wdt->base + wdt->drv_data->int_mask);
> +		val &= ~STARFIVE_WDT_INT_DIS;
> +		writel(val, wdt->base + wdt->drv_data->int_mask);
> +	}
> +}
> +
> +/* disable watchdog interrupt */
> +static inline void starfive_wdt_int_disable(struct starfive_wdt *wdt)
> +{
> +	u32 val;
> +
> +	if (wdt->drv_data->int_mask) {
> +		val = readl(wdt->base + wdt->drv_data->int_mask);
> +		val |= STARFIVE_WDT_INT_DIS;
> +		writel(val, wdt->base + wdt->drv_data->int_mask);
> +	}
> +}
> +
> +/* enable watchdog interrupt to reset */
> +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->variant->enrst_shift;
> +	writel(val, wdt->base + wdt->drv_data->control);
> +}
> +
> +/* disable watchdog interrupt to reset */
> +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->variant->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->enter_irq_status);
> +}
> +
> +static void starfive_wdt_int_clr(struct starfive_wdt *wdt)
> +{
> +	void __iomem *addr;
> +	u8 clr_check;
> +	u8 clr_ava_shift;
> +	u32 value;
> +	int ret = 0;
> +
> +	addr = wdt->base + wdt->drv_data->int_clr;
> +	clr_ava_shift = wdt->drv_data->variant->intclr_ava_shift;
> +	clr_check = wdt->drv_data->variant->intclr_check;
> +	if (clr_check) {
> +		/* waiting interrupt can be to clearing */
> +		value = readl(addr);
> +		ret = readl_poll_timeout_atomic(addr, value,
> +						!(value & BIT(clr_ava_shift)),
> +						STARFIVE_WDT_DELAY_US, STARFIVE_WDT_TIMEOUT_US);
> +	}
> +
> +	if (!ret)
> +		writel(STARFIVE_WDT_INTCLR, addr);
> +
> +	if (starfive_wdt_raise_irq_status(wdt))
> +		enable_irq(wdt->irq);
> +}
> +
> +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);
> +}
> +
> +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->variant->en_shift;
> +	writel(val, wdt->base + wdt->drv_data->enable);
> +}
> +
> +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->variant->en_shift);
> +	writel(val, wdt->base + wdt->drv_data->enable);
> +}
> +
> +static inline void starfive_wdt_set_relod_count(struct starfive_wdt *wdt, u32 count)
> +{
> +	writel(count, wdt->base + wdt->drv_data->load);
> +	if (wdt->drv_data->reload)
> +		writel(0x1, wdt->base + wdt->drv_data->reload);
> +	else
> +		/* jh7110 need enable controller to reload counter */
> +		starfive_wdt_enable(wdt);
> +}
> +
> +static void starfive_wdt_mask_and_disable_reset(struct starfive_wdt *wdt, bool mask)
> +{
> +	starfive_wdt_unlock(wdt);
> +
> +	if (mask)
> +		starfive_wdt_disable_reset(wdt);
> +	else
> +		starfive_wdt_enable_reset(wdt);
> +
> +	starfive_wdt_lock(wdt);
> +}
> +
> +static unsigned int starfive_wdt_max_timeout(struct starfive_wdt *wdt)
> +{
> +	return DIV_ROUND_UP(STARFIVE_WDT_MAXCNT, wdt->freq) - 1;
> +}
> +
> +static unsigned int starfive_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +	u32 count;
> +	u32 irq_status;
> +
> +	starfive_wdt_unlock(wdt);
> +	/*
> +	 * Because set half count value,
> +	 * timeleft value should add the count value before first timeout.
> +	 */
> +	irq_status = starfive_wdt_raise_irq_status(wdt) ? 1 : 0;
> +	count = starfive_wdt_get_count(wdt) + (1 - irq_status) * 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_relod_count(wdt, wdt->count);
> +	starfive_wdt_lock(wdt);
> +
> +	spin_unlock(&wdt->lock);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t starfive_wdt_interrupt_handler(int irq, void *data)
> +{
> +	/*
> +	 * We don't clear the IRQ status. It's supposed to be done by the
> +	 * following ping operations.
> +	 */
> +	struct platform_device *pdev = data;
> +	struct starfive_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	/* Disable the IRQ and avoid re-entry interrupt. */
> +	disable_irq_nosync(wdt->irq);
> +
> +	return IRQ_HANDLED;
> +}

What is the purpose of this interrupt ? It doesn't add any value
as implemented.

> +
> +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_int_disable(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(wdt->dev);
> +
> +	return 0;
> +}
> +
> +static int starfive_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	pm_runtime_get_sync(wdt->dev);
> +
> +	spin_lock(&wdt->lock);
> +
> +	starfive_wdt_unlock(wdt);
> +
> +	if (soft_noboot)
> +		starfive_wdt_disable_reset(wdt);
> +	else
> +		starfive_wdt_enable_reset(wdt);
> +
> +	starfive_wdt_int_clr(wdt);
> +	starfive_wdt_set_count(wdt, wdt->count);
> +	starfive_wdt_int_enable(wdt);
> +	starfive_wdt_enable(wdt);
> +
> +	starfive_wdt_lock(wdt);
> +
> +	spin_unlock(&wdt->lock);
> +
> +	return 0;
> +}
> +
> +static int starfive_wdt_restart(struct watchdog_device *wdd, unsigned long action,
> +				void *data)
> +{
> +	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);
> +
> +	if (soft_noboot)
> +		starfive_wdt_disable_reset(wdt);
> +	else
> +		starfive_wdt_enable_reset(wdt);
> +
> +	/* put initial values into count and data */
> +	starfive_wdt_set_count(wdt, wdt->count);

This is supposed to reset the system. Why write wdt->count
instead of 0 or 1 ?

> +
> +	/* set the watchdog to go and reset... */
> +	starfive_wdt_int_clr(wdt);
> +	starfive_wdt_int_enable(wdt);
> +	starfive_wdt_enable(wdt);
> +
> +	starfive_wdt_lock(wdt);
> +	spin_unlock(&wdt->lock);
> +
> +	return 0;
> +}
> +
> +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;
> +	unsigned int count;
> +
> +	spin_lock(&wdt->lock);
> +
> +	if (timeout < 1)
> +		return -EINVAL;

Can not happen if configured properly, ie if min_timeout is set *which it is).

> +
> +	/*
> +	 * This watchdog takes twice timeouts to reset.
> +	 * In order to reduce time to reset, should set half count value.
> +	 */
> +	count = timeout * freq / 2;
> +
> +	if (count > STARFIVE_WDT_MAXCNT) {
> +		dev_warn(wdt->dev, "timeout %d too big,use the MAX-timeout set.\n",
> +			 timeout);
> +		timeout = starfive_wdt_max_timeout(wdt);
> +		count = timeout * freq;
> +	}
> +
> +	dev_info(wdt->dev, "Heartbeat: timeout=%d, count/2=%d (%08x)\n",
> +		 timeout, count, count);
> +
> +	starfive_wdt_unlock(wdt);
> +	starfive_wdt_disable(wdt);
> +	starfive_wdt_set_relod_count(wdt, count);
> +	starfive_wdt_enable(wdt);
> +	starfive_wdt_lock(wdt);
> +
> +	wdt->count = count;
> +	wdd->timeout = timeout;
> +
> +	spin_unlock(&wdt->lock);
> +
> +	return 0;
> +}
> +
> +#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
> +
> +static const struct watchdog_info starfive_wdt_ident = {
> +	.options = OPTIONS,
> +	.firmware_version = 0,
> +	.identity = "StarFive Watchdog",
> +};
> +
> +static const struct watchdog_ops starfive_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = starfive_wdt_start,
> +	.stop = starfive_wdt_pm_stop,
> +	.ping = starfive_wdt_keepalive,
> +	.set_timeout = starfive_wdt_set_timeout,
> +	.restart = starfive_wdt_restart,
> +	.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 started = 0;
> +	int ret;
> +
> +	pm_runtime_enable(dev);
> +
> +	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);
> +
> +	wdt->irq = platform_get_irq(pdev, 0);
> +	if (wdt->irq < 0) {
> +		dev_err(dev, "can not find irq.\n");
> +		return wdt->irq;
> +	}
> +
> +	/* 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);
> +		goto err;
> +	}
> +
> +	ret = starfive_wdt_enable_clock(wdt);
> +	if (ret)
> +		goto err;
> +
> +	starfive_wdt_get_clock_rate(wdt);
> +

This can fail, leaving the clock rate uninitialized.

> +	ret = starfive_wdt_reset_init(wdt);
> +	if (ret)
> +		goto err;
> +
> +	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 timer margin,
> +	 * and if not, try the default value.
> +	 */
> +	watchdog_init_timeout(&wdt->wdt_device, tmr_margin, dev);
> +
> +	ret = starfive_wdt_set_timeout(&wdt->wdt_device,
> +				       wdt->wdt_device.timeout);
> +	if (ret) {
> +		dev_err(dev, "tmr_margin value out of range, default %d used\n",
> +			STARFIVE_WDT_DEFAULT_TIME);
> +		starfive_wdt_set_timeout(&wdt->wdt_device,
> +					 STARFIVE_WDT_DEFAULT_TIME);
> +	}
> +
> +	ret = devm_request_irq(dev, wdt->irq, starfive_wdt_interrupt_handler, 0,
> +			       pdev->name, pdev);
> +	if (ret != 0) {
> +		dev_err(dev, "failed to install irq (%d)\n", ret);
> +		goto err;
> +	}
> +

That interrupt seems pointless.

> +	watchdog_set_nowayout(&wdt->wdt_device, nowayout);
> +	watchdog_set_restart_priority(&wdt->wdt_device, 128);
> +
> +	wdt->wdt_device.parent = dev;
> +
> +	ret = watchdog_register_device(&wdt->wdt_device);
> +	if (ret)
> +		goto err;
> +
> +	starfive_wdt_mask_and_disable_reset(wdt, false);
> +
> +	if (tmr_atboot && started == 0) {
> +		starfive_wdt_start(&wdt->wdt_device);
> +	} else if (!tmr_atboot) {
> +		/*
> +		 *if we're not enabling the watchdog, then ensure it is
> +		 * disabled if it has been left running from the bootloader
> +		 * or other source.
> +		 */
> +		starfive_wdt_stop(&wdt->wdt_device);
> +	}

This should be done before registering the watdhdog, and the watchdog core
should be informed if the watchdog is running.

> +
> +#ifdef CONFIG_PM
> +	clk_disable_unprepare(wdt->core_clk);
> +	clk_disable_unprepare(wdt->apb_clk);
> +#endif
> +
> +	platform_set_drvdata(pdev, wdt);
> +
> +	return 0;
> +
> +err:
> +	return ret;

This is never acceptable. Return directly.

> +}
> +
> +static int starfive_wdt_remove(struct platform_device *dev)
> +{
> +	struct starfive_wdt *wdt = platform_get_drvdata(dev);
> +
> +	starfive_wdt_mask_and_disable_reset(wdt, true);
> +	watchdog_unregister_device(&wdt->wdt_device);
> +
> +	clk_disable_unprepare(wdt->core_clk);
> +	clk_disable_unprepare(wdt->apb_clk);
> +	pm_runtime_disable(wdt->dev);

This is potentially unbalanced. I would suggest to use
watchdog_stop_on_unregister().

> +
> +	return 0;
> +}
> +
> +static void starfive_wdt_shutdown(struct platform_device *dev)
> +{
> +	struct starfive_wdt *wdt = platform_get_drvdata(dev);
> +
> +	starfive_wdt_mask_and_disable_reset(wdt, true);
> +
> +	starfive_wdt_pm_stop(&wdt->wdt_device);

Use watchdog_stop_on_reboot().

> +}
> +
> +#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);
> +
> +	starfive_wdt_mask_and_disable_reset(wdt, true);
> +
> +	/* 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_relod_count(wdt, wdt->reload);
> +
> +	starfive_wdt_restart(&wdt->wdt_device, 0, NULL);
> +
> +	starfive_wdt_mask_and_disable_reset(wdt, false);
> +
> +	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");
> +		goto err;
> +	}
> +
> +	ret = clk_prepare_enable(wdt->core_clk);
> +	if (ret) {
> +		dev_err(wdt->dev, "failed to enable core_clk.\n");
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	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 v2");

Re: [PATCH v1 1/3] dt-bindings: watchdog: Add watchdog for StarFive

[Xingyu Wu]

On 2022/12/2 18:46, Krzysztof Kozlowski wrote:
> On 02/12/2022 10:39, xingu.wu wrote:
>> From: Xingyu Wu <xingyu.wu@starfivetech.com>
>> 
>> Add bindings to describe the watchdog for the StarFive SoCs.
>> 
>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> ---
>>  .../bindings/watchdog/starfive,wdt.yaml       | 77 +++++++++++++++++++
>>  1 file changed, 77 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml b/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>> new file mode 100644
>> index 000000000000..ece3e80153a0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
> 
> Filename should be based on compatible. You do not allow here any other
> models... If you intent and you are 100% sure you will grow with new
> models, make it maybe a family-based name.

First, thank you for your reply. We have some other SoCs like JH7100 would use
this watchdog driver, but we now use JH7110 as our main release chip.
As you say, should we use "starfive,jh71xx-wdt.yaml" as filename?


>> @@ -0,0 +1,77 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/watchdog/starfive,wdt.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive Watchdog
>> +
>> +allOf:
>> +  - $ref: "watchdog.yaml#"
> 
> Drop quotes.

Will fix.
>> +
>> +maintainers:
>> +  - Samin Guo <samin.guo@starfivetech.com>
>> +  - Xingyu Wu <xingyu.wu@starfivetech.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: starfive,jh7110-wdt
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: Core clock
>> +      - description: APB clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: core_clk
> 
> Drop _clk

Will fix.

> 
>> +      - const: apb_clk
> 
> Drop _clk

Will fix.

> 
>> +
>> +  resets:
>> +    items:
>> +      - description: APB reset
>> +      - description: Core reset
>> +
>> +  reset-names:
>> +    items:
>> +      - const: rst_apb
> 
> Drop rst_

Will fix.

> 
>> +      - const: rst_core
> 
> Ditto

Will fix.

> 
>> +
>> +  timeout-sec: true
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - resets
>> +  - reset-names
>> +  - timeout-sec
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/starfive-jh7110.h>
>> +    #include <dt-bindings/reset/starfive-jh7110.h>
>> +
>> +    watchdog@13070000 {
>> +            compatible = "starfive,jh7110-wdt";
> 
> Use 4 spaces for example indentation.

Will fix.

> 
>> +            reg = <0x13070000 0x10000>;
>> +            interrupts = <68>;
>> +            clocks = <&syscrg_clk JH7110_SYSCLK_WDT_CORE>,
>> +                     <&syscrg_clk JH7110_SYSCLK_WDT_APB>;
>> +            clock-names = "core_clk", "apb_clk";
>> +            resets = <&syscrg_rst JH7110_SYSRST_WDT_APB>,
>> +                     <&syscrg_rst JH7110_SYSRST_WDT_CORE>;
>> +            reset-names = "rst_apb", "rst_core";
>> +            timeout-sec = <15>;
>> +    };
>> +
> 
> Drop trailing line.

Will fix.

Best regards,
Xingyu Wu

Re: [PATCH v1 1/3] dt-bindings: watchdog: Add watchdog for StarFive

[Krzysztof Kozlowski]

On 05/12/2022 04:49, Xingyu Wu wrote:
> On 2022/12/2 18:46, Krzysztof Kozlowski wrote:
>> On 02/12/2022 10:39, xingu.wu wrote:
>>> From: Xingyu Wu <xingyu.wu@starfivetech.com>
>>>
>>> Add bindings to describe the watchdog for the StarFive SoCs.
>>>
>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>> ---
>>>  .../bindings/watchdog/starfive,wdt.yaml       | 77 +++++++++++++++++++
>>>  1 file changed, 77 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml b/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>>> new file mode 100644
>>> index 000000000000..ece3e80153a0
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>>
>> Filename should be based on compatible. You do not allow here any other
>> models... If you intent and you are 100% sure you will grow with new
>> models, make it maybe a family-based name.
> 
> First, thank you for your reply. We have some other SoCs like JH7100 would use
> this watchdog driver, but we now use JH7110 as our main release chip.
> As you say, should we use "starfive,jh71xx-wdt.yaml" as filename?

starfive,jh7110-wdt.yaml
I would say because you do not expect any other models (const for
compatible, not enum)

Best regards,
Krzysztof

Re: [PATCH v1 2/3] drivers: watchdog: Add StarFive Watchdog driver

[Xingyu Wu]

On 2022/12/2 23:08, Guenter Roeck wrote:
> On 12/2/22 01:39, xingu.wu wrote:
>> From: Xingyu Wu <xingyu.wu@starfivetech.com>
>>
>> Add watchdog driver for the StarFive JH7110 SoC.
>>
>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> ---
>>   MAINTAINERS                     |   7 +
>>   drivers/watchdog/Kconfig        |  11 +
>>   drivers/watchdog/Makefile       |   3 +
>>   drivers/watchdog/starfive-wdt.c | 854 ++++++++++++++++++++++++++++++++
>>   4 files changed, 875 insertions(+)
>>   create mode 100644 drivers/watchdog/starfive-wdt.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a70c1d0f303e..133214dd2f60 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19623,6 +19623,13 @@ F:    Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>>   F:    drivers/reset/starfive/
>>   F:    include/dt-bindings/reset/starfive*
>>   +STARFIVE 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 b64bc49c7f30..658fc9fef4fa 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -2082,6 +2082,17 @@ config UML_WATCHDOG
>>       tristate "UML watchdog"
>>       depends on UML || COMPILE_TEST
>>   +# RISCV Architecture
>> +
>> +config STARFIVE_WATCHDOG
>> +    tristate "StarFive Watchdog support"
>> +    depends on RISCV
>> +    select WATCHDOG_CORE
>> +    default SOC_STARFIVE
>> +    help
>> +      Say Y here to support the StarFive watchdog. 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 d41e5f830ae7..9c22f1a43d5c 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -210,6 +210,9 @@ obj-$(CONFIG_WATCHDOG_SUN4V)        += sun4v_wdt.o
>>   # Xen
>>   obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>>   +# RISCV Architecture
>> +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..dcb6e693ba6c
>> --- /dev/null
>> +++ b/drivers/watchdog/starfive-wdt.c
>> @@ -0,0 +1,854 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Starfive Watchdog driver
>> + *
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/slab.h>
>> +#include <linux/timer.h>
>> +#include <linux/types.h>
>> +#include <linux/uaccess.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]: res.
>> +                         */
>> +#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.
>> +                         */
>> +#define STARFIVE_WDT_JH7110_ITCR    0xf00    /*
>> +                         * RW: When set HIGH,
>> +                         * places the Watchdog into integraeion test mode.\\intetration
> 
>> +                         */
>> +#define STARFIVE_WDT_JH7110_ITOP    0xf04    /*
>> +                         * WO: [0] WDOGRES value Integration Test Mode.
>> +                         * Value output on WDOGRES when in
>> +                         * Integration Test Mode.
>> +                         * [1] Integration Test WDOGINT value.
>> +                         * Value output on WDOGINT when in
>> +                         * Integration Test Mode.
>> +                         */
>> +
>> +#define STARFIVE_WDT_JH7110_UNLOCK_KEY        0x1acce551
>> +#define STARFIVE_WDT_JH7110_RESEN_SHIFT        1
>> +#define STARFIVE_WDT_JH7110_EN_SHIFT        0
>> +#define STARFIVE_WDT_JH7110_INT_EN_SHIFT    STARFIVE_WDT_JH7110_EN_SHIFT
>> +
>> +/* WDOGCONTROL */
>> +#define STARFIVE_WDT_INT_EN    0x0
>> +#define STARFIVE_WDT_INT_DIS    BIT(0)
>> +#define STARFIVE_WDT_RESET_EN    0x1
>> +
>> +/* WDOGLOCK */
>> +#define STARFIVE_WDT_LOCKED    BIT(0)
>> +
>> +#define STARFIVE_WDT_INTCLR    0x1
>> +#define STARFIVE_WDT_ENABLE    0x1
>> +#define STARFIVE_WDT_ATBOOT    0x0
>> +#define STARFIVE_WDT_MAXCNT    0xffffffff
>> +
>> +#define STARFIVE_WDT_DEFAULT_TIME    (15)
>> +#define STARFIVE_WDT_DELAY_US        0
>> +#define STARFIVE_WDT_TIMEOUT_US        10000
>> +
>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>> +static int tmr_margin;
>> +static int tmr_atboot = STARFIVE_WDT_ATBOOT;
>> +static int soft_noboot;
>> +
>> +module_param(tmr_margin, int, 0);
>> +module_param(tmr_atboot, int, 0);
>> +module_param(nowayout, bool, 0);
>> +module_param(soft_noboot, int, 0);
>> +
>> +MODULE_PARM_DESC(tmr_margin, "Watchdog tmr_margin in seconds. (default="
>> +         __MODULE_STRING(STARFIVE_WDT_DEFAULT_TIME) ")");
>> +MODULE_PARM_DESC(tmr_atboot,
>> +         "Watchdog is started at boot time if set to 1, default="
>> +         __MODULE_STRING(STARFIVE_WDT_ATBOOT));
>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>> +         __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>> +MODULE_PARM_DESC(soft_noboot,
>> +         "Watchdog action, set to 1 to ignore reboots, 0 to reboot (default 0)");
>> +
>> +struct starfive_wdt_variant_t {
>> +    u32 unlock_key;
>> +    u8 enrst_shift;
>> +    u8 en_shift;
>> +    u8 intclr_check;
>> +    u8 intclr_ava_shift;
>> +};
>> +
>> +struct starfive_wdt_variant {
>> +    u32 control;
>> +    u32 load;
>> +    u32 enable;
>> +    u32 reload;
>> +    u32 value;
>> +    u32 int_clr;
>> +    u32 int_mask;
>> +    u32 unlock;
>> +    u32 enter_irq_status;
>> +    struct starfive_wdt_variant_t *variant;
>> +};
>> +
>> +struct starfive_wdt {
>> +    u64 freq;
> 
> clk_get_rate() returns unsigned long. Why would u64 be needed here ?

Will fix and use unsigned long instead of u64.

> 
>> +    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 */
>> +
>> +    int irq;
>> +};
>> +
>> +#ifdef CONFIG_OF
>> +static struct starfive_wdt_variant_t jh7110_variant = {
>> +    .unlock_key = STARFIVE_WDT_JH7110_UNLOCK_KEY,
>> +    .enrst_shift = STARFIVE_WDT_JH7110_RESEN_SHIFT,
>> +    .en_shift = STARFIVE_WDT_JH7110_EN_SHIFT,
>> +};
>> +
>> +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,
>> +    .enter_irq_status = STARFIVE_WDT_JH7110_IMS,
>> +    .variant = &jh7110_variant,
>> +};
>> +
> 
> If there is no specific reason for all that complexity, drop it and introduce
> it if/when necessary.

We have some other SoCs like JH7100 would use this watchdog driver, and use this
struct to distinguish the register addresses of different SoCs.

> 
>> +static const struct of_device_id starfive_wdt_match[] = {
>> +    { .compatible = "starfive,jh7110-wdt", .data = &drv_data_jh7110 },
>> +    {}
>> +};
>> +MODULE_DEVICE_TABLE(of, starfive_wdt_match);
>> +#endif
>> +
>> +static const struct platform_device_id starfive_wdt_ids[] = {
>> +    {
>> +        .name = "starfive-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)
>> +{
>> +    int ret;
>> +    u32 freq;
>> +
>> +    if (!IS_ERR(wdt->core_clk)) {
>> +        wdt->freq = clk_get_rate(wdt->core_clk);
>> +        return 0;
>> +    }
>> +    dev_dbg(wdt->dev, "get rate failed, need clock-frequency define in dts.\n");
>> +
>> +    /* Next we try to get clock-frequency from dts.*/
>> +    ret = of_property_read_u32(wdt->dev->of_node, "clock-frequency", &freq);
>> +    if (!ret) {
>> +        wdt->freq = (u64)freq;
> 
> Unnecessary typecast.

Will fix.

> 
>> +        return 0;
>> +    }
>> +    dev_err(wdt->dev, "get clock-frequency failed\n");
>> +
>> +    return -ENOENT;
>> +}
> 
> I have never seen this kind of code before. Why would it be needed ?

I will drop it and only get clock rate by clk_get_rate().

> 
>> +
>> +static int starfive_wdt_enable_clock(struct starfive_wdt *wdt)
>> +{
>> +    int ret = 0;
>> +
>> +    wdt->apb_clk = devm_clk_get(wdt->dev, "apb_clk");
>> +    if (!IS_ERR(wdt->apb_clk)) {
>> +        ret = clk_prepare_enable(wdt->apb_clk);
>> +        if (ret) {
>> +            dev_err(wdt->dev, "failed to enable apb_clk.\n");
>> +            goto err;
>> +        }
>> +    } else {
>> +        dev_err(wdt->dev, "failed to get apb_clk.\n");
>> +        ret = PTR_ERR(wdt->apb_clk);
>> +        goto err;
>> +    }
> 
> Why not use devm_clk_get_enabled() ?

Thanks for your advice. This driver was used in earlier Linux version before
and I haven't seen this devm_clk_get_enabled(). Now I will try to use it.

> 
>> +
>> +    wdt->core_clk = devm_clk_get(wdt->dev, "core_clk");
>> +    if (!IS_ERR(wdt->core_clk)) {
>> +        ret = clk_prepare_enable(wdt->core_clk);
>> +        if (ret) {
>> +            dev_err(wdt->dev, "failed to enable core_clk.\n");
>> +            goto err;
>> +        }
>> +    } else {
>> +        dev_err(wdt->dev, "failed to get core_clk.\n");
>> +        ret = PTR_ERR(wdt->core_clk);
>> +        goto err;
>> +    }
>> +
>> +    return 0;
>> +
>> +err:
>> +    return ret;
> 
> Unnecessary. Return directly.

Will fix.

> 
>> +}
>> +
>> +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)) {
>> +        ret = reset_control_deassert(wdt->rsts);
>> +        if (ret) {
>> +            dev_err(wdt->dev, "failed to deassert rsts.\n");
>> +            goto err;
>> +        }
>> +    } else {
>> +        dev_err(wdt->dev, "failed to get rsts error.\n");
> 
> failed ... error is redundant. Drop "errror".

Will fix.

> 
>> +        ret = PTR_ERR(wdt->rsts);
>> +        goto err;
>> +    }
>> +
>> +    return 0;
>> +
>> +err:
>> +    return ret;
> 
> Unnecessary. Return directly.

Will fix.

> 
>> +}
>> +
>> +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 int starfive_wdt_is_locked(struct starfive_wdt *wdt)
> 
> bool ?

Will use bool better.

> 
>> +{
>> +    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->variant->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->variant->unlock_key,
>> +               wdt->base + wdt->drv_data->unlock);
>> +}
>> +
>> +/* enable watchdog interrupt */
>> +static inline void starfive_wdt_int_enable(struct starfive_wdt *wdt)
>> +{
>> +    u32 val;
>> +
>> +    if (wdt->drv_data->int_mask) {
>> +        val = readl(wdt->base + wdt->drv_data->int_mask);
>> +        val &= ~STARFIVE_WDT_INT_DIS;
>> +        writel(val, wdt->base + wdt->drv_data->int_mask);
>> +    }
>> +}
>> +
>> +/* disable watchdog interrupt */
>> +static inline void starfive_wdt_int_disable(struct starfive_wdt *wdt)
>> +{
>> +    u32 val;
>> +
>> +    if (wdt->drv_data->int_mask) {
>> +        val = readl(wdt->base + wdt->drv_data->int_mask);
>> +        val |= STARFIVE_WDT_INT_DIS;
>> +        writel(val, wdt->base + wdt->drv_data->int_mask);
>> +    }
>> +}
>> +
>> +/* enable watchdog interrupt to reset */
>> +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->variant->enrst_shift;
>> +    writel(val, wdt->base + wdt->drv_data->control);
>> +}
>> +
>> +/* disable watchdog interrupt to reset */
>> +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->variant->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->enter_irq_status);
>> +}
>> +
>> +static void starfive_wdt_int_clr(struct starfive_wdt *wdt)
>> +{
>> +    void __iomem *addr;
>> +    u8 clr_check;
>> +    u8 clr_ava_shift;
>> +    u32 value;
>> +    int ret = 0;
>> +
>> +    addr = wdt->base + wdt->drv_data->int_clr;
>> +    clr_ava_shift = wdt->drv_data->variant->intclr_ava_shift;
>> +    clr_check = wdt->drv_data->variant->intclr_check;
>> +    if (clr_check) {
>> +        /* waiting interrupt can be to clearing */
>> +        value = readl(addr);
>> +        ret = readl_poll_timeout_atomic(addr, value,
>> +                        !(value & BIT(clr_ava_shift)),
>> +                        STARFIVE_WDT_DELAY_US, STARFIVE_WDT_TIMEOUT_US);
>> +    }
>> +
>> +    if (!ret)
>> +        writel(STARFIVE_WDT_INTCLR, addr);
>> +
>> +    if (starfive_wdt_raise_irq_status(wdt))
>> +        enable_irq(wdt->irq);
>> +}
>> +
>> +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);
>> +}
>> +
>> +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->variant->en_shift;
>> +    writel(val, wdt->base + wdt->drv_data->enable);
>> +}
>> +
>> +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->variant->en_shift);
>> +    writel(val, wdt->base + wdt->drv_data->enable);
>> +}
>> +
>> +static inline void starfive_wdt_set_relod_count(struct starfive_wdt *wdt, u32 count)
>> +{
>> +    writel(count, wdt->base + wdt->drv_data->load);
>> +    if (wdt->drv_data->reload)
>> +        writel(0x1, wdt->base + wdt->drv_data->reload);
>> +    else
>> +        /* jh7110 need enable controller to reload counter */
>> +        starfive_wdt_enable(wdt);
>> +}
>> +
>> +static void starfive_wdt_mask_and_disable_reset(struct starfive_wdt *wdt, bool mask)
>> +{
>> +    starfive_wdt_unlock(wdt);
>> +
>> +    if (mask)
>> +        starfive_wdt_disable_reset(wdt);
>> +    else
>> +        starfive_wdt_enable_reset(wdt);
>> +
>> +    starfive_wdt_lock(wdt);
>> +}
>> +
>> +static unsigned int starfive_wdt_max_timeout(struct starfive_wdt *wdt)
>> +{
>> +    return DIV_ROUND_UP(STARFIVE_WDT_MAXCNT, wdt->freq) - 1;
>> +}
>> +
>> +static unsigned int starfive_wdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +    struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +    u32 count;
>> +    u32 irq_status;
>> +
>> +    starfive_wdt_unlock(wdt);
>> +    /*
>> +     * Because set half count value,
>> +     * timeleft value should add the count value before first timeout.
>> +     */
>> +    irq_status = starfive_wdt_raise_irq_status(wdt) ? 1 : 0;
>> +    count = starfive_wdt_get_count(wdt) + (1 - irq_status) * 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_relod_count(wdt, wdt->count);
>> +    starfive_wdt_lock(wdt);
>> +
>> +    spin_unlock(&wdt->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static irqreturn_t starfive_wdt_interrupt_handler(int irq, void *data)
>> +{
>> +    /*
>> +     * We don't clear the IRQ status. It's supposed to be done by the
>> +     * following ping operations.
>> +     */
>> +    struct platform_device *pdev = data;
>> +    struct starfive_wdt *wdt = platform_get_drvdata(pdev);
>> +
>> +    /* Disable the IRQ and avoid re-entry interrupt. */
>> +    disable_irq_nosync(wdt->irq);
>> +
>> +    return IRQ_HANDLED;
>> +}
> 
> What is the purpose of this interrupt ? It doesn't add any value
> as implemented.

After your reminder, I think this is no longer necessary now and will
drop it.

> 
>> +
>> +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_int_disable(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(wdt->dev);
>> +
>> +    return 0;
>> +}
>> +
>> +static int starfive_wdt_start(struct watchdog_device *wdd)
>> +{
>> +    struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +    pm_runtime_get_sync(wdt->dev);
>> +
>> +    spin_lock(&wdt->lock);
>> +
>> +    starfive_wdt_unlock(wdt);
>> +
>> +    if (soft_noboot)
>> +        starfive_wdt_disable_reset(wdt);
>> +    else
>> +        starfive_wdt_enable_reset(wdt);
>> +
>> +    starfive_wdt_int_clr(wdt);
>> +    starfive_wdt_set_count(wdt, wdt->count);
>> +    starfive_wdt_int_enable(wdt);
>> +    starfive_wdt_enable(wdt);
>> +
>> +    starfive_wdt_lock(wdt);
>> +
>> +    spin_unlock(&wdt->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int starfive_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>> +                void *data)
>> +{
>> +    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);
>> +
>> +    if (soft_noboot)
>> +        starfive_wdt_disable_reset(wdt);
>> +    else
>> +        starfive_wdt_enable_reset(wdt);
>> +
>> +    /* put initial values into count and data */
>> +    starfive_wdt_set_count(wdt, wdt->count);
> 
> This is supposed to reset the system. Why write wdt->count
> instead of 0 or 1 ?

This is in order to make the watchdog work again and make sure the
counter start from wdt->count which had been set before restart.

> 
>> +
>> +    /* set the watchdog to go and reset... */
>> +    starfive_wdt_int_clr(wdt);
>> +    starfive_wdt_int_enable(wdt);
>> +    starfive_wdt_enable(wdt);
>> +
>> +    starfive_wdt_lock(wdt);
>> +    spin_unlock(&wdt->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +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;
>> +    unsigned int count;
>> +
>> +    spin_lock(&wdt->lock);
>> +
>> +    if (timeout < 1)
>> +        return -EINVAL;
> 
> Can not happen if configured properly, ie if min_timeout is set *which it is).

Will use min_timeout instead of 1.

> 
>> +
>> +    /*
>> +     * This watchdog takes twice timeouts to reset.
>> +     * In order to reduce time to reset, should set half count value.
>> +     */
>> +    count = timeout * freq / 2;
>> +
>> +    if (count > STARFIVE_WDT_MAXCNT) {
>> +        dev_warn(wdt->dev, "timeout %d too big,use the MAX-timeout set.\n",
>> +             timeout);
>> +        timeout = starfive_wdt_max_timeout(wdt);
>> +        count = timeout * freq;
>> +    }
>> +
>> +    dev_info(wdt->dev, "Heartbeat: timeout=%d, count/2=%d (%08x)\n",
>> +         timeout, count, count);
>> +
>> +    starfive_wdt_unlock(wdt);
>> +    starfive_wdt_disable(wdt);
>> +    starfive_wdt_set_relod_count(wdt, count);
>> +    starfive_wdt_enable(wdt);
>> +    starfive_wdt_lock(wdt);
>> +
>> +    wdt->count = count;
>> +    wdd->timeout = timeout;
>> +
>> +    spin_unlock(&wdt->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
>> +
>> +static const struct watchdog_info starfive_wdt_ident = {
>> +    .options = OPTIONS,
>> +    .firmware_version = 0,
>> +    .identity = "StarFive Watchdog",
>> +};
>> +
>> +static const struct watchdog_ops starfive_wdt_ops = {
>> +    .owner = THIS_MODULE,
>> +    .start = starfive_wdt_start,
>> +    .stop = starfive_wdt_pm_stop,
>> +    .ping = starfive_wdt_keepalive,
>> +    .set_timeout = starfive_wdt_set_timeout,
>> +    .restart = starfive_wdt_restart,
>> +    .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 started = 0;
>> +    int ret;
>> +
>> +    pm_runtime_enable(dev);
>> +
>> +    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);
>> +
>> +    wdt->irq = platform_get_irq(pdev, 0);
>> +    if (wdt->irq < 0) {
>> +        dev_err(dev, "can not find irq.\n");
>> +        return wdt->irq;
>> +    }
>> +
>> +    /* 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);
>> +        goto err;
>> +    }
>> +
>> +    ret = starfive_wdt_enable_clock(wdt);
>> +    if (ret)
>> +        goto err;
>> +
>> +    starfive_wdt_get_clock_rate(wdt);
>> +
> 
> This can fail, leaving the clock rate uninitialized.

Will fix.

> 
>> +    ret = starfive_wdt_reset_init(wdt);
>> +    if (ret)
>> +        goto err;
>> +
>> +    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 timer margin,
>> +     * and if not, try the default value.
>> +     */
>> +    watchdog_init_timeout(&wdt->wdt_device, tmr_margin, dev);
>> +
>> +    ret = starfive_wdt_set_timeout(&wdt->wdt_device,
>> +                       wdt->wdt_device.timeout);
>> +    if (ret) {
>> +        dev_err(dev, "tmr_margin value out of range, default %d used\n",
>> +            STARFIVE_WDT_DEFAULT_TIME);
>> +        starfive_wdt_set_timeout(&wdt->wdt_device,
>> +                     STARFIVE_WDT_DEFAULT_TIME);
>> +    }
>> +
>> +    ret = devm_request_irq(dev, wdt->irq, starfive_wdt_interrupt_handler, 0,
>> +                   pdev->name, pdev);
>> +    if (ret != 0) {
>> +        dev_err(dev, "failed to install irq (%d)\n", ret);
>> +        goto err;
>> +    }
>> +
> 
> That interrupt seems pointless.

Will drop it.

> 
>> +    watchdog_set_nowayout(&wdt->wdt_device, nowayout);
>> +    watchdog_set_restart_priority(&wdt->wdt_device, 128);
>> +
>> +    wdt->wdt_device.parent = dev;
>> +
>> +    ret = watchdog_register_device(&wdt->wdt_device);
>> +    if (ret)
>> +        goto err;
>> +
>> +    starfive_wdt_mask_and_disable_reset(wdt, false);
>> +
>> +    if (tmr_atboot && started == 0) {
>> +        starfive_wdt_start(&wdt->wdt_device);
>> +    } else if (!tmr_atboot) {
>> +        /*
>> +         *if we're not enabling the watchdog, then ensure it is
>> +         * disabled if it has been left running from the bootloader
>> +         * or other source.
>> +         */
>> +        starfive_wdt_stop(&wdt->wdt_device);
>> +    }
> 
> This should be done before registering the watdhdog, and the watchdog core
> should be informed if the watchdog is running.

This is supposed to decide whether to start the watchdog at the end of registration.
It also can be stared by user after registering the watchdog.

> 
>> +
>> +#ifdef CONFIG_PM
>> +    clk_disable_unprepare(wdt->core_clk);
>> +    clk_disable_unprepare(wdt->apb_clk);
>> +#endif
>> +
>> +    platform_set_drvdata(pdev, wdt);
>> +
>> +    return 0;
>> +
>> +err:
>> +    return ret;
> 
> This is never acceptable. Return directly.

Will fix.

> 
>> +}
>> +
>> +static int starfive_wdt_remove(struct platform_device *dev)
>> +{
>> +    struct starfive_wdt *wdt = platform_get_drvdata(dev);
>> +
>> +    starfive_wdt_mask_and_disable_reset(wdt, true);
>> +    watchdog_unregister_device(&wdt->wdt_device);
>> +
>> +    clk_disable_unprepare(wdt->core_clk);
>> +    clk_disable_unprepare(wdt->apb_clk);
>> +    pm_runtime_disable(wdt->dev);
> 
> This is potentially unbalanced. I would suggest to use
> watchdog_stop_on_unregister().

Thank for your advice. I will add watchdog_stop_on_unregister() when
registering the watchdog.

> 
>> +
>> +    return 0;
>> +}
>> +
>> +static void starfive_wdt_shutdown(struct platform_device *dev)
>> +{
>> +    struct starfive_wdt *wdt = platform_get_drvdata(dev);
>> +
>> +    starfive_wdt_mask_and_disable_reset(wdt, true);
>> +
>> +    starfive_wdt_pm_stop(&wdt->wdt_device);
> 
> Use watchdog_stop_on_reboot().

I will add watchdog_stop_on_reboot() when registering the watchdog.

> 
>> +}
>> +
>> +#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);
>> +
>> +    starfive_wdt_mask_and_disable_reset(wdt, true);
>> +
>> +    /* 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_relod_count(wdt, wdt->reload);
>> +
>> +    starfive_wdt_restart(&wdt->wdt_device, 0, NULL);
>> +
>> +    starfive_wdt_mask_and_disable_reset(wdt, false);
>> +
>> +    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");
>> +        goto err;
>> +    }
>> +
>> +    ret = clk_prepare_enable(wdt->core_clk);
>> +    if (ret) {
>> +        dev_err(wdt->dev, "failed to enable core_clk.\n");
>> +        goto err;
>> +    }
>> +
>> +    return 0;
>> +
>> +err:
>> +    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 v2");
> 

Best regards,
Xingyu Wu

Re: [PATCH v1 1/3] dt-bindings: watchdog: Add watchdog for StarFive

[Xingyu Wu]

On 2022/12/5 15:30, Krzysztof Kozlowski wrote:
> On 05/12/2022 04:49, Xingyu Wu wrote:
>> On 2022/12/2 18:46, Krzysztof Kozlowski wrote:
>>> On 02/12/2022 10:39, xingu.wu wrote:
>>>> From: Xingyu Wu <xingyu.wu@starfivetech.com>
>>>>
>>>> Add bindings to describe the watchdog for the StarFive SoCs.
>>>>
>>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>>> ---
>>>>  .../bindings/watchdog/starfive,wdt.yaml       | 77 +++++++++++++++++++
>>>>  1 file changed, 77 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml b/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>>>> new file mode 100644
>>>> index 000000000000..ece3e80153a0
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>>>
>>> Filename should be based on compatible. You do not allow here any other
>>> models... If you intent and you are 100% sure you will grow with new
>>> models, make it maybe a family-based name.
>> 
>> First, thank you for your reply. We have some other SoCs like JH7100 would use
>> this watchdog driver, but we now use JH7110 as our main release chip.
>> As you say, should we use "starfive,jh71xx-wdt.yaml" as filename?
> 
> starfive,jh7110-wdt.yaml
> I would say because you do not expect any other models (const for
> compatible, not enum)
> 

Thanks, I will rename the file in the patches for next version.

Best regards,
Xingyu Wu

Re: [PATCH v1 3/3] riscv: dts: starfive: jh7110: Add watchdog node

[Xingyu Wu]

On 2022/12/2 18:48, Krzysztof Kozlowski wrote:
> On 02/12/2022 10:39, xingu.wu wrote:
>> From: Xingyu Wu <xingyu.wu@starfivetech.com>
>> 
>> This adds the watchdog node for the Starfive JH7110 SoC.
> 
> Do not use "This commit/patch".
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

Will drop 'This'.

> 
>> 
>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> ---
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>> 
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> index c22e8f1d2640..22f5a37d691e 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -433,5 +433,19 @@ uart5: serial@12020000 {
>>  			reg-shift = <2>;
>>  			status = "disabled";
>>  		};
>> +
>> +		wdog: watchdog@13070000 {
>> +			compatible = "starfive,jh7110-wdt";
>> +			reg = <0x0 0x13070000 0x0 0x10000>;
>> +			interrupts = <68>;
>> +			clocks = <&syscrg JH7110_SYSCLK_WDT_CORE>,
>> +				 <&syscrg JH7110_SYSCLK_WDT_APB>;
>> +			clock-names = "core_clk", "apb_clk";
>> +			resets = <&syscrg JH7110_SYSRST_WDT_APB>,
>> +				 <&syscrg JH7110_SYSRST_WDT_CORE>;
>> +			reset-names = "rst_apb", "rst_core";
>> +			timeout-sec = <15>;
>> +			status = "okay";
> 
> Why? okay is by default
> 

Will drop it.

Best regards,
Xingyu Wu