Re: [PATCH v1 2/2] watchdog: npcm: Add reset status support

[Guenter Roeck]

On 2/10/26 05:38, Tomer Maimon wrote:
> Add reset status detection for NPCM watchdog driver on both NPCM7XX and
> NPCM8XX platforms. Implement GCR register integration via syscon for
> reset status detection and configurable reset type mapping via device
> tree properties.
>
> Signed-off-by: Tomer Maimon <tmaimon77@xxxxxxxxx>
> ---
>   drivers/watchdog/npcm_wdt.c | 110 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 110 insertions(+)
>
> diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c
> index e62ea054bc61..ebece5d6240a 100644
> --- a/drivers/watchdog/npcm_wdt.c
> +++ b/drivers/watchdog/npcm_wdt.c
> @@ -12,9 +12,25 @@
>   #include <linux/platform_device.h>
>   #include <linux/slab.h>
>   #include <linux/watchdog.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define NPCM7XX_RESSR_OFFSET	0x6C
> +#define NPCM7XX_INTCR2_OFFSET	0x60
>   
>   #define NPCM_WTCR	0x1C
>   
> +#define NPCM7XX_PORST	BIT(31)
> +#define NPCM7XX_CORST	BIT(30)
> +#define NPCM7XX_WD0RST	BIT(29)
> +#define NPCM7XX_WD1RST	BIT(24)
> +#define NPCM7XX_WD2RST	BIT(23)
> +#define NPCM7XX_SWR1RST	BIT(28)
> +#define NPCM7XX_SWR2RST	BIT(27)
> +#define NPCM7XX_SWR3RST	BIT(26)
> +#define NPCM7XX_SWR4RST	BIT(25)
> +#define NPCM8XX_RST	(GENMASK(31, 23) | GENMASK(15, 12))
> +
>   #define NPCM_WTCLK	(BIT(10) | BIT(11))	/* Clock divider */
>   #define NPCM_WTE	BIT(7)			/* Enable */
>   #define NPCM_WTIE	BIT(6)			/* Enable irq */
> @@ -45,6 +61,9 @@ struct npcm_wdt {
>   	struct watchdog_device  wdd;
>   	void __iomem		*reg;
>   	struct clk		*clk;
> +	u32			card_reset;
> +	u32			ext1_reset;
> +	u32			ext2_reset;
>   };
>   
>   static inline struct npcm_wdt *to_npcm_wdt(struct watchdog_device *wdd)
> @@ -185,6 +204,95 @@ static const struct watchdog_ops npcm_wdt_ops = {
>   	.restart = npcm_wdt_restart,
>   };
>   
> +static u32 npcm_wdt_reset_type(const char *reset_type)
> +{
> +	if (!strcmp(reset_type, "porst"))
> +		return NPCM7XX_PORST;
> +	else if (!strcmp(reset_type, "corst"))
> +		return NPCM7XX_CORST;
> +	else if (!strcmp(reset_type, "wd0"))
> +		return NPCM7XX_WD0RST;
> +	else if (!strcmp(reset_type, "wd1"))
> +		return NPCM7XX_WD1RST;
> +	else if (!strcmp(reset_type, "wd2"))
> +		return NPCM7XX_WD2RST;
> +	else if (!strcmp(reset_type, "sw1"))
> +		return NPCM7XX_SWR1RST;
> +	else if (!strcmp(reset_type, "sw2"))
> +		return NPCM7XX_SWR2RST;
> +	else if (!strcmp(reset_type, "sw3"))
> +		return NPCM7XX_SWR3RST;
> +	else if (!strcmp(reset_type, "sw4"))
> +		return NPCM7XX_SWR4RST;
> +
> +	return 0;
> +}
> +
> +static void npcm_get_reset_status(struct npcm_wdt *wdt, struct device *dev)
> +{
> +	const char *card_reset_type;
> +	const char *ext1_reset_type;
> +	const char *ext2_reset_type;
> +	struct regmap *gcr_regmap;
> +	u32 rstval, ressrval;
> +	int ret;
> +
> +	gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> +	if (IS_ERR(gcr_regmap)) {
> +		dev_warn(dev, "Failed to find gcr syscon, WD reset status not supported\n");

A warning is quite strong here, given that this is new code and the
syscon reference may not exist in existing devicetree files. notice
should be good enough.

> +		return;
> +	}
> +
> +	ret = of_property_read_string(dev->of_node,
> +				      "nuvoton,card-reset-type",
> +				      &card_reset_type);
> +	if (ret)
> +		wdt->card_reset = NPCM7XX_PORST;
> +	else
> +		wdt->card_reset = npcm_wdt_reset_type(card_reset_type);
> +
> +	ret = of_property_read_string(dev->of_node,
> +				      "nuvoton,ext1-reset-type",
> +				      &ext1_reset_type);
> +	if (ret)
> +		wdt->ext1_reset = 0;

wdt is zero-allocated, so setting those variables to 0 is not necessary.

> +	else
> +		wdt->ext1_reset = npcm_wdt_reset_type(ext1_reset_type);
> +
> +	ret = of_property_read_string(dev->of_node,
> +				      "nuvoton,ext2-reset-type",
> +				      &ext2_reset_type);
> +	if (ret)
> +		wdt->ext2_reset = 0;
> +	else
> +		wdt->ext2_reset = npcm_wdt_reset_type(ext2_reset_type);
> +
> +	regmap_read(gcr_regmap, NPCM7XX_INTCR2_OFFSET, &rstval);

This warrants an explanation/comment: Why is it not necessary to check
the return value of the regmap operations ?

> +	/* prefer the most specific SoC first */
> +	if (of_device_is_compatible(dev->of_node, "nuvoton,npcm845-wdt")) {
> +		regmap_write(gcr_regmap, NPCM7XX_INTCR2_OFFSET,
> +			     rstval & ~NPCM8XX_RST);
> +	} else if (of_device_is_compatible(dev->of_node, "nuvoton,npcm750-wdt")) {
> +		if ((rstval & NPCM7XX_PORST) == 0) {
> +			rstval = NPCM7XX_PORST;
> +			regmap_write(gcr_regmap, NPCM7XX_INTCR2_OFFSET,
> +				     rstval | NPCM7XX_PORST);

That "| NPCM7XX_PORST" is pretty pointless here since rstval was
just set to that value.

> +		} else {
> +			rstval = 0;
> +		}

Another comment needed: This negates NPCM7XX_PORST and otherwise clear
rstval. The reason is not immediately (or, rather, at all) obvious.

> +		regmap_read(gcr_regmap, NPCM7XX_RESSR_OFFSET, &ressrval);
> +		rstval |= ressrval;
> +		regmap_write(gcr_regmap, NPCM7XX_RESSR_OFFSET, ressrval);
> +	}

If the device is not compatible to either chip, retval is just passed
on and nothing is written to the chip. That warrants another comment.

[ Yes, I see that the driver does not currently support another chip.

> +
> +	if (rstval & wdt->card_reset)
> +		wdt->wdd.bootstatus |= WDIOF_CARDRESET;
> +	if (rstval & wdt->ext1_reset)
> +		wdt->wdd.bootstatus |= WDIOF_EXTERN1;
> +	if (rstval & wdt->ext2_reset)
> +		wdt->wdd.bootstatus |= WDIOF_EXTERN2;
> +}
> +
>   static int npcm_wdt_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -208,6 +316,8 @@ static int npcm_wdt_probe(struct platform_device *pdev)
>   	if (irq < 0)
>   		return irq;
>   
> +	npcm_get_reset_status(wdt, dev);
> +
>   	wdt->wdd.info = &npcm_wdt_info;
>   	wdt->wdd.ops = &npcm_wdt_ops;
>   	wdt->wdd.min_timeout = 1;