[v4,0/2] ARM: r9a06g032: add support for the watchdogs

Hi all,

This series adds support for the watchdog timers of the RZ/N1.
The watchdog driver (rzn1-wdt.c) is derived from the driver available at
https://github.com/renesas-rz/rzn1_linux.git with a few modifications

In order to be able to reset the board when a watchdog timer expires,
the RSTEN register must be configured. it is the responsability of the
bootloader to set those bits (or not, depending on the chosen policy).

If the watchdog reset source is not enabled, an interrupt is triggered
when the watchdog expires. The interrupt handler will trigger an
emergency restart.


Changes v3 -> v4:
 * dts: removed the patches that modify the device tree (already taken in
   the renesas dt tree)
 * driver: Call emergency_restart() in the interrupt handler.
 
Changes v2 -> v3:
* dts: changed compatible strings to include "renesas,r9a06g032-wdt" and
  "renesas,rzn1-wdt".
* driver: removed the SOC-specific "renesas,r9a06g032-wdt".
* removed all the changes in the clock driver: the watchdog reset source
  are not disabled anymore when the machine is halted.
* fixed the clock rate type in the computations.
* removed unnecessary printout and call to clk_disable_unprepare() in the
  driver probe().
    
Changes v1 -> v2:
* Modified the clock driver to not enable the watchdog reset sources.
  On other renesas platforms, those bits are by the bootloader. The
  watchdog reset sources are still disabled when the platform is halted
  to prevent a watchdog reset.
* Added a SOC-specific compatible "renesas,r9a06g032-wdt"
* reordered the dts/i entries
* default timeout is 60 seconds
* reworked the probe function of the wdt driver to better error cases
* removed the set_timeout() and use a fixed period computed in probe().
  This removes the confusion and makes it clear that the period defined
  by the user space in indeed handled by the watchdog core


Jean-Jacques Hiblot (1):
  dt-bindings: watchdog: renesas,wdt: Add support for RZ/N1

Phil Edworthy (1):
  watchdog: Add Renesas RZ/N1 Watchdog driver

 .../bindings/watchdog/renesas,wdt.yaml        |   6 +
 drivers/watchdog/Kconfig                      |   8 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/rzn1_wdt.c                   | 207 ++++++++++++++++++
 4 files changed, 222 insertions(+)
 create mode 100644 drivers/watchdog/rzn1_wdt.c

On Wed, Mar 30, 2022 at 12:08:29PM +0200, Jean-Jacques Hiblot wrote:
> diff --git a/drivers/watchdog/rzn1_wdt.c b/drivers/watchdog/rzn1_wdt.c
[...]
> +/*
> + * Renesas RZ/N1 Watchdog timer.
> + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even
> + * cope with 2 seconds.
> + *
> + * Copyright 2018 Renesas Electronics Europe Ltd.

s/2018/2022/ ?

> +#define RZN1_WDT_RETRIGGER			0x0
> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL		0
> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK	0xfff
> +#define RZN1_WDT_RETRIGGER_PRESCALE		BIT(12)
> +#define RZN1_WDT_RETRIGGER_ENABLE		BIT(13)
> +#define RZN1_WDT_RETRIGGER_WDSI			(0x2 << 14)

Do RZN1_WDT_RETRIGGER_RELOAD_VAL and RZN1_WDT_RETRIGGER_WDSI get 1 more tab
indent intentionally?

> +static const struct watchdog_device rzn1_wdt = {
> +	.info = &rzn1_wdt_info,
> +	.ops = &rzn1_wdt_ops,
> +	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
> +};
[...]
> +static int rzn1_wdt_probe(struct platform_device *pdev)
> +{
[...]
> +	wdt->wdt = rzn1_wdt;

Does it really need to copy the memory?  For example,

1. Use the memory in `wdt` directly and fill the `wdd`.

struct watchdog_device *wdd = &wdt->wdt;
wdd->info = &rzn1_wdt_info;
wdd->ops = &rzn1_wdt_ops;
...

2. Use drvdata instead of container_of().

Use watchdog_set_drvdata() in _probe and watchdog_get_drvdata() in the
watchdog ops to get struct rzn1_watchdog.

> +static const struct of_device_id rzn1_wdt_match[] = {
> +	{ .compatible = "renesas,rzn1-wdt" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rzn1_wdt_match);

Doesn't it need to guard by CONFIG_OF?

> +static struct platform_driver rzn1_wdt_driver = {
> +	.probe		= rzn1_wdt_probe,
> +	.driver		= {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= rzn1_wdt_match,

Does it makes more sense to use of_match_ptr()?

> +	},
> +};
> +
> +module_platform_driver(rzn1_wdt_driver);

To make it look like a whole thing, I prefer to remove the extra blank line
in between struct platform_driver and module_platform_driver().

On 3/30/22 23:08, Tzung-Bi Shih wrote:
> On Wed, Mar 30, 2022 at 12:08:29PM +0200, Jean-Jacques Hiblot wrote:
>> diff --git a/drivers/watchdog/rzn1_wdt.c b/drivers/watchdog/rzn1_wdt.c
> [...]
>> +/*
>> + * Renesas RZ/N1 Watchdog timer.
>> + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even
>> + * cope with 2 seconds.
>> + *
>> + * Copyright 2018 Renesas Electronics Europe Ltd.
> 
> s/2018/2022/ ?
> 
>> +#define RZN1_WDT_RETRIGGER			0x0
>> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL		0
>> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK	0xfff
>> +#define RZN1_WDT_RETRIGGER_PRESCALE		BIT(12)
>> +#define RZN1_WDT_RETRIGGER_ENABLE		BIT(13)
>> +#define RZN1_WDT_RETRIGGER_WDSI			(0x2 << 14)
> 
> Do RZN1_WDT_RETRIGGER_RELOAD_VAL and RZN1_WDT_RETRIGGER_WDSI get 1 more tab
> indent intentionally?
> 

That only looks like it due to the "+" at the beginning of the line.
If you look at the actual code the alignment is ok.

>> +static const struct watchdog_device rzn1_wdt = {
>> +	.info = &rzn1_wdt_info,
>> +	.ops = &rzn1_wdt_ops,
>> +	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
>> +};
> [...]
>> +static int rzn1_wdt_probe(struct platform_device *pdev)
>> +{
> [...]
>> +	wdt->wdt = rzn1_wdt;
> 
> Does it really need to copy the memory?  For example,
> 
> 1. Use the memory in `wdt` directly and fill the `wdd`.
> 
> struct watchdog_device *wdd = &wdt->wdt;
> wdd->info = &rzn1_wdt_info;
> wdd->ops = &rzn1_wdt_ops;
> ...
> 
> 2. Use drvdata instead of container_of().
> 
> Use watchdog_set_drvdata() in _probe and watchdog_get_drvdata() in the
> watchdog ops to get struct rzn1_watchdog.
> 
That would indeed be preferred. The static data structure isn't really useful.

>> +static const struct of_device_id rzn1_wdt_match[] = {
>> +	{ .compatible = "renesas,rzn1-wdt" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, rzn1_wdt_match);
> 
> Doesn't it need to guard by CONFIG_OF?
> 
Only if of_match_ptr() is used below, and then I'd prefer __maybe_unused

>> +static struct platform_driver rzn1_wdt_driver = {
>> +	.probe		= rzn1_wdt_probe,
>> +	.driver		= {
>> +		.name		= KBUILD_MODNAME,
>> +		.of_match_table	= rzn1_wdt_match,
> 
> Does it makes more sense to use of_match_ptr()?
> 

Usually we leave that up to driver authors.

>> +	},
>> +};
>> +
>> +module_platform_driver(rzn1_wdt_driver);
> 
> To make it look like a whole thing, I prefer to remove the extra blank line
> in between struct platform_driver and module_platform_driver().

We usually leave that up to driver authors. Many watchdog driver leave
an empty line, so it is ok (as long as there are no two empty lines).

Thanks,
Guenter

On 31/03/2022 08:08, Tzung-Bi Shih wrote:
> On Wed, Mar 30, 2022 at 12:08:29PM +0200, Jean-Jacques Hiblot wrote:
>> diff --git a/drivers/watchdog/rzn1_wdt.c b/drivers/watchdog/rzn1_wdt.c
> [...]
>> +/*
>> + * Renesas RZ/N1 Watchdog timer.
>> + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even
>> + * cope with 2 seconds.
>> + *
>> + * Copyright 2018 Renesas Electronics Europe Ltd.
> s/2018/2022/ ?
This driver wasn't written by me originally. So I'd rather not change 
this line.
>
>> +#define RZN1_WDT_RETRIGGER			0x0
>> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL		0
>> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK	0xfff
>> +#define RZN1_WDT_RETRIGGER_PRESCALE		BIT(12)
>> +#define RZN1_WDT_RETRIGGER_ENABLE		BIT(13)
>> +#define RZN1_WDT_RETRIGGER_WDSI			(0x2 << 14)
> Do RZN1_WDT_RETRIGGER_RELOAD_VAL and RZN1_WDT_RETRIGGER_WDSI get 1 more tab
> indent intentionally?
>
>> +static const struct watchdog_device rzn1_wdt = {
>> +	.info = &rzn1_wdt_info,
>> +	.ops = &rzn1_wdt_ops,
>> +	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
>> +};
> [...]
>> +static int rzn1_wdt_probe(struct platform_device *pdev)
>> +{
> [...]
>> +	wdt->wdt = rzn1_wdt;
> Does it really need to copy the memory?  For example,
>
> 1. Use the memory in `wdt` directly and fill the `wdd`.
>
> struct watchdog_device *wdd = &wdt->wdt;
> wdd->info = &rzn1_wdt_info;
> wdd->ops = &rzn1_wdt_ops;
> ...
>
> 2. Use drvdata instead of container_of().
>
> Use watchdog_set_drvdata() in _probe and watchdog_get_drvdata() in the
> watchdog ops to get struct rzn1_watchdog.
I'll rework this. Thanks for the review
>> +static const struct of_device_id rzn1_wdt_match[] = {
>> +	{ .compatible = "renesas,rzn1-wdt" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, rzn1_wdt_match);
> Doesn't it need to guard by CONFIG_OF?

I don't think it has to.

>
>> +static struct platform_driver rzn1_wdt_driver = {
>> +	.probe		= rzn1_wdt_probe,
>> +	.driver		= {
>> +		.name		= KBUILD_MODNAME,
>> +		.of_match_table	= rzn1_wdt_match,
> Does it makes more sense to use of_match_ptr()?
>
>> +	},
>> +};
>> +
>> +module_platform_driver(rzn1_wdt_driver);
> To make it look like a whole thing, I prefer to remove the extra blank line
> in between struct platform_driver and module_platform_driver().