Re: [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume

[Guenter Roeck]

On 12/7/18 1:45 PM, Wolfram Sang wrote:

Hi Guenter, all,


After discussing this mail thread [1] again, we concluded that giving
userspace enough time to prepare is our favourite option. So, do not
keep the time value when suspended but reset it when resuming.

[1] https://patchwork.kernel.org/patch/10252209/

Signed-off-by: Wolfram Sang 


Above exchange says it all, no need to repeat.

Reviewed-by: Guenter Roeck 


Thanks.

I can relate to the policy argument, though. Regardless of this patch, I
wonder if we can make it configurable from userspace. A draft:

#define WDIOF_RESUME_OPTS   0x0800

#define WDIOS_RESUME_KEEP   0x0008
#define WDIOS_RESUME_RESET  0x0010

and then in watchdog_ioctl() under WDIOC_SETOPTIONS:

if (!(wdd->info->options & WDIOF_RESUME_OPTS))
err = -EOPNOTSUPP;
goto break;

if (val & WDIOS_RESUME_KEEP)
wdd->status |= WDOG_KEEP_TIMER_WHEN_RESUME;

if (val & WDIOS_RESUME_RESET)
wdd->status ~= ~WDOG_KEEP_TIMER_WHEN_RESUME;

So, drivers with WDIOF_RESUME_OPTS could act on the
WDOG_KEEP_TIMER_WHEN_RESUME flag.

Opinions?



Not entirely sure I understand the use case.

Having said that, if we were to add this option, I think only a single
flag would be needed - WDIOF_RESUME_KEEP. All we need to do is declare
that a ping on resume shall be the default. Anything else would result
in undefined per-driver default behavior.

Guenter

Re: [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume

[Guenter Roeck]

On 12/4/18 4:01 AM, Wolfram Sang wrote:

After discussing this mail thread [1] again, we concluded that giving
userspace enough time to prepare is our favourite option. So, do not
keep the time value when suspended but reset it when resuming.

[1] https://patchwork.kernel.org/patch/10252209/

Signed-off-by: Wolfram Sang 


Above exchange says it all, no need to repeat.

Reviewed-by: Guenter Roeck 


---

Fabrizio: can you agree to that? The R-Car BSP team and we (the R-Car upstream
team) would prefer it this way (knowing it is also not perfect).

  drivers/watchdog/renesas_wdt.c | 11 ---
  1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index b570962e84f3..9f2307bf727b 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -48,7 +48,6 @@ struct rwdt_priv {
void __iomem *base;
struct watchdog_device wdev;
unsigned long clk_rate;
-   u16 time_left;
u8 cks;
  };
  
@@ -263,10 +262,9 @@ static int __maybe_unused rwdt_suspend(struct device *dev)

  {
struct rwdt_priv *priv = dev_get_drvdata(dev);
  
-	if (watchdog_active(>wdev)) {

-   priv->time_left = readw(priv->base + RWTCNT);
+   if (watchdog_active(>wdev))
rwdt_stop(>wdev);
-   }
+
return 0;
  }
  
@@ -274,10 +272,9 @@ static int __maybe_unused rwdt_resume(struct device *dev)

  {
struct rwdt_priv *priv = dev_get_drvdata(dev);
  
-	if (watchdog_active(>wdev)) {

+   if (watchdog_active(>wdev))
rwdt_start(>wdev);
-   rwdt_write(priv, priv->time_left, RWTCNT);
-   }
+
return 0;
  }
  

Re: [PATCH v2] watchdog: renesas_wdt: don't set divider while watchdog is running

[Guenter Roeck]

On Wed, Nov 07, 2018 at 08:46:02PM +0100, Wolfram Sang wrote:
> The datasheet says we must stop the timer before changing the clock
> divider. This can happen when the restart handler is called while the
> watchdog is running.
> 
> Signed-off-by: Wolfram Sang 

Reviewed-by: Guenter Roeck 

> ---
> 
> Change since V1: reworded commit message.
> 
> I sent V1 back then when it was more a recommendation of the datasheet to do 
> it
> like this. But meanwhile the code changed, so we actually need to do it.
> 
>  drivers/watchdog/renesas_wdt.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 0d74c3e48979..55c9eb6c6e51 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -74,12 +74,17 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
>  static int rwdt_start(struct watchdog_device *wdev)
>  {
>   struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> + u8 val;
>  
>   pm_runtime_get_sync(wdev->parent);
>  
> - rwdt_write(priv, 0, RWTCSRB);
> - rwdt_write(priv, priv->cks, RWTCSRA);
> + /* Stop the timer before we modify any register */
> + val = readb_relaxed(priv->base + RWTCSRA) & ~RWTCSRA_TME;
> + rwdt_write(priv, val, RWTCSRA);
> +
>   rwdt_init_timeout(wdev);
> + rwdt_write(priv, priv->cks, RWTCSRA);
> + rwdt_write(priv, 0, RWTCSRB);
>  
>   while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
>   cpu_relax();
> -- 
> 2.11.0
> 

Re: [PATCH] watchdog: renesas_wdt: Fix typos

[Guenter Roeck]

On Fri, Nov 02, 2018 at 07:21:11PM +, Fabrizio Castro wrote:

Do not use "," but ";" to separate instructions.

> Signed-off-by: Fabrizio Castro 

Reviewed-by: Guenter Roeck 

> ---
>  drivers/watchdog/renesas_wdt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 0d74c3e..b570962 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -220,8 +220,8 @@ static int rwdt_probe(struct platform_device *pdev)
>   goto out_pm_disable;
>   }
>  
> - priv->wdev.info = _ident,
> - priv->wdev.ops = _ops,
> + priv->wdev.info = _ident;
> + priv->wdev.ops = _ops;
>   priv->wdev.parent = >dev;
>   priv->wdev.min_timeout = 1;
>   priv->wdev.max_timeout = DIV_BY_CLKS_PER_SEC(priv, 65536);
> -- 
> 2.7.4
> 

Re: [PATCH v2] thermal_hwmon: Fix the 'No sensors found' error after replacing the parameter NULL by the actual device

[Guenter Roeck]

On Thu, Oct 04, 2018 at 10:29:19AM +0900, Cao Van Dong wrote:
> Dear Marc-san,
> 
> Thanks for your comment!
> 
> >>Formerly, when registering the hwmon device, we pass NULL as the device.
> >>It's not a problem.
> >>Recently, the developer has replaced the parameter NULL as the device by
> >>the actual device.
> >>This causes the "No sensors found" error. Therefore, instead of using
> >>the device we will use pass
> >
> >What does report this error? Is it a userspace application?
> 
> Below are the comments of Guente-san:
>  " The "No sensors found" message is from the "sensors" application.
>    The device type associated with the passed device is unknown to
>    libsensors (it is parsed from the parent device subsystem name -
>    what is that, anyway ?). This could be addressed in libsensors,
>    or the thermal subsystem could attach itself to a known subsystem,
>    or the thermal subsystem must pass a pointer the parent device. "
> 
> I think the problem is the thermal subsystem must pass a pointer the parent
> device.
> 
Thinking about this more ...

We only know that passing the parent device works for your use case.
We do not know if parent device types are well defined and recognized
as supported by libsensors. As such, passing the parent device as
parameter may not always work as intended.

Unfortunately, the only means to test or evaluate this would be to
run "sensors" on all affected systems, and/or to inspect the thermal
device source code to determine all parent device types.

A better solution would have been to ensure that the device pointer
passed to hwmon is recognized by libsensors, but unfortunately it
is a bit late for that.

Guenter

> >>the parent of that device as parameter. This will sync with the
> >>processor on the hwmon core.
> >>This patch is to fix this error.
> >>
> >>This patch is based on the v4.19-rc3 tag.
> >>
> >>---
> >
> >This patch has no SoB.
> 
> I will add SoB later.
> 
> >>  drivers/thermal/thermal_hwmon.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/thermal/thermal_hwmon.c
> >>b/drivers/thermal/thermal_hwmon.c
> >>index 40c69a5..a918ba9 100644
> >>--- a/drivers/thermal/thermal_hwmon.c
> >>+++ b/drivers/thermal/thermal_hwmon.c
> >>@@ -143,7 +143,7 @@ int thermal_add_hwmon_sysfs(struct
> >>thermal_zone_device *tz)
> >>  INIT_LIST_HEAD(>tz_list);
> >>  strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
> >>  strreplace(hwmon->type, '-', '_');
> >>-    hwmon->device = hwmon_device_register_with_info(>device,
> >>hwmon->type,
> >>+    hwmon->device = hwmon_device_register_with_info(tz->device.parent,
> >>hwmon->type,
> >>  hwmon, NULL, NULL);
> >>  if (IS_ERR(hwmon->device)) {
> >>  result = PTR_ERR(hwmon->device);
> >>
> >
> >It is not clear to me that this is any better. What is the parent device
> >in this case? Can you give an example of what breaks in the hierarchy?
> >
> >Given how close we are to to 4.19, I'd rather we revert f6b6b52ef7a54160
> >if there are userspace visible regressions.
> >
> 
> Initially, I considered your patch to be correct and went to look at the
> processor in the hwmon core. Then I created the v1 patch.
> After patch v1, I noticed that it is necessary to check for cases where the
> device has parent and if there is no parent.
> However, when looking at function hwmon_device_register_with_info() and its
> header, I see that the device parameter passed in
> must be the parent device ("* @dev: the parent device"). Therefore, I think
> it's best to pass a parent device parameter to this function.
> So, I decided to create this v2 patch.
> 
> 
> Regards,
> Dong/Jinso
> 
> 

Re: dt-bindings: watchdog: renesas-wdt: Document r8a7744 support

[Guenter Roeck]

On Tue, Sep 25, 2018 at 06:07:21PM +0100, Biju Das wrote:
> RZ/G1N (R8A7744) watchdog implementation is compatible with R-Car
> Gen2, therefore add relevant documentation.
> 
> Signed-off-by: Biju Das 
> Reviewed-by: Chris Paterson 
> Reviewed-by: Simon Horman 
> Reviewed-by: Geert Uytterhoeven 

Reviewed-by: Guenter Roeck 

> ---
>  Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
> b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> index 9407212..d72d118 100644
> --- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> @@ -6,6 +6,7 @@ Required properties:
>   version.
>  Examples with soctypes are:
>- "renesas,r8a7743-wdt" (RZ/G1M)
> +  - "renesas,r8a7744-wdt" (RZ/G1N)
>- "renesas,r8a7745-wdt" (RZ/G1E)
>- "renesas,r8a774a1-wdt" (RZ/G2M)
>- "renesas,r8a7790-wdt" (R-Car H2)

Re: [v4,2/2] dt-bindings: watchdog: renesas-wdt: Add support for R7S9210

[Guenter Roeck]

On Mon, Sep 10, 2018 at 02:52:33PM -0500, Chris Brandt wrote:
> Document support for RZ/A2
> 
> Signed-off-by: Chris Brandt 

Reviewed-by: Guenter Roeck 

> ---
>  Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
> b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> index 5d47a262474c..45a709dd0345 100644
> --- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> @@ -19,6 +19,7 @@ Required properties:
>- "renesas,r8a77990-wdt" (R-Car E3)
>- "renesas,r8a77995-wdt" (R-Car D3)
>- "renesas,r7s72100-wdt" (RZ/A1)
> +  - "renesas,r7s9210-wdt"  (RZ/A2)
>   The generic compatible string must be:
>- "renesas,rza-wdt" for RZ/A
>- "renesas,rcar-gen2-wdt" for R-Car Gen2 and RZ/G

Re: [v4,1/2] watchdog: rza_wdt: Support longer timeouts

[Guenter Roeck]

On Mon, Sep 10, 2018 at 02:52:32PM -0500, Chris Brandt wrote:
> The RZ/A2 watchdog timer extends the clock source options in order to
> allow for longer timeouts.
> 
> Signed-off-by: Chris Brandt 

Reviewed-by: Guenter Roeck 

> ---
> v4:
>  * Documented CKS_3BIT/CKS_4BIT better
>  * Changed 16384 and 4194304 into #define
>  * Removed rza_wdt.timeout
>  * Removed extra ( ) from DIV_ROUND_UP
>  * Removed check for counter value > 256
>  * Added set_timeout function
>  * Removed checking for new timeout value in ping function
>  * Removed unneeded 'else' case when checking .data in probe
> v3:
>  * Removed + 1 from DIV_ROUND_UP line
>  * resetting to 0 if time to big did not make as much sense are resetting
>to 256
> v2:
> * use DIV_ROUND_UP
> * use %u for pr_debug
> * use of_match data to determine the size of CKS register
> ---
>  drivers/watchdog/rza_wdt.c | 88 
> --
>  1 file changed, 70 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
> index e618218d2374..aeca6b13c797 100644
> --- a/drivers/watchdog/rza_wdt.c
> +++ b/drivers/watchdog/rza_wdt.c
> @@ -14,6 +14,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -34,12 +35,45 @@
>  #define WRCSR_RSTE   BIT(6)
>  #define WRCSR_CLEAR_WOVF 0xA500  /* special value */
>  
> +/* The maximum CKS register setting value to get the longest timeout */
> +#define CKS_3BIT 0x7
> +#define CKS_4BIT 0xF
> +
> +#define DIVIDER_3BIT 16384   /* Clock divider when CKS = 0x7 */
> +#define DIVIDER_4BIT 4194304 /* Clock divider when CKS = 0xF */
> +
>  struct rza_wdt {
>   struct watchdog_device wdev;
>   void __iomem *base;
>   struct clk *clk;
> + u8 count;
> + u8 cks;
>  };
>  
> +static void rza_wdt_calc_timeout(struct rza_wdt *priv, int timeout)
> +{
> + unsigned long rate = clk_get_rate(priv->clk);
> + unsigned int ticks;
> +
> + if (priv->cks == CKS_4BIT) {
> + ticks = DIV_ROUND_UP(timeout * rate, DIVIDER_4BIT);
> +
> + /*
> +  * Since max_timeout was set in probe, we know that the timeout
> +  * value passed will never calculate to a tick value greater
> +  * than 256.
> +  */
> + priv->count = 256 - ticks;
> +
> + } else {
> + /* Start timer with longest timeout */
> + priv->count = 0;
> + }
> +
> + pr_debug("%s: timeout set to %u (WTCNT=%d)\n", __func__,
> +  timeout, priv->count);
> +}
> +
>  static int rza_wdt_start(struct watchdog_device *wdev)
>  {
>   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> @@ -51,13 +85,12 @@ static int rza_wdt_start(struct watchdog_device *wdev)
>   readb(priv->base + WRCSR);
>   writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
>  
> - /*
> -  * Start timer with slowest clock source and reset option enabled.
> -  */
> + rza_wdt_calc_timeout(priv, wdev->timeout);
> +
>   writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
> - writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> - writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
> -priv->base + WTCSR);
> + writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
> + writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME |
> +WTSCR_CKS(priv->cks), priv->base + WTCSR);
>  
>   return 0;
>  }
> @@ -75,8 +108,17 @@ static int rza_wdt_ping(struct watchdog_device *wdev)
>  {
>   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
>  
> - writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> + writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
>  
> + pr_debug("%s: timeout = %u\n", __func__, wdev->timeout);
> +
> + return 0;
> +}
> +
> +static int rza_set_timeout(struct watchdog_device *wdev, unsigned int 
> timeout)
> +{
> + wdev->timeout = timeout;
> + rza_wdt_start(wdev);
>   return 0;
>  }
>  
> @@ -121,6 +163,7 @@ static const struct watchdog_ops rza_wdt_ops = {
>   .start = rza_wdt_start,
>   .stop = rza_wdt_stop,
>   .ping = rza_wdt_ping,
> + .set_timeout = rza_set_timeout,
>   .restart = rza_wdt_restart,
>  };
>  
> @@ -150,20 +193,28 @@ static int rza_wdt_probe(struct platform_device *pdev)
>   return -ENOENT;
>   }
>  
> - /* Assume slowest clock rate possible (CKS=7) */
> - rate /= 16384;
> -

Re: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts

[Guenter Roeck]

On Mon, Sep 10, 2018 at 05:36:39PM +, Chris Brandt wrote:
> On Monday, September 10, 2018 1, Guenter Roeck wrote:
> > > #2. If the CKS is only 4-bits, the max HW timeout is 32 seconds. (so
> > > 'timeout' can never be more that a u8).
> > >
> > That is not the point. The point is that there is no need to keep two
> > 'timeout' variables.
> 
> But there was a reason.
> 
> The reason was that the upper layer would call the .ping() function 
> without calling .start() again.
> 
> Meaning it would change 'timeout' in the structure, but not explicitly 
> tell the driver.
> 

It does that because there is no set_timeout function.

> That why I had to keep track of my own timeout (what the HW was actually
> set to).
> 
> Every time the upper layer calls .ping(), I have to see if the timeout 
> field still matches.
> 
> 
> > > The number "4194304" is exactly how it is documented in the hardware
> > > manual, that is why I'm using it that way. Yes, 0x40 makes more
> > > sense, but I like matching the device documenting as much as possible to
> > > help the next person that comes along and has to debug this code.
> > >
> > Use at least a define.
> 
> OK.
> 
> 
> > > Because when I was doing all my testing, I found cases where '.ping' was
> > > called from the upper layer without '.start' being called first. So, I
> > > changed the code as you see it now. This guaranteed I would also get the
> > > timeout the user was requesting.
> > >
> > That would only happen if the watchdog is considered to be running.
> > Also, we are talking about the set_timeout function which is the one which
> > should set the timeout and update the HW if needed, ie if the watchdog is
> > already running.
> 
> This driver doesn't have .set_timeout
> 
> static const struct watchdog_ops rza_wdt_ops = {
>   .owner = THIS_MODULE,
>   .start = rza_wdt_start,
>   .stop = rza_wdt_stop,
>   .ping = rza_wdt_ping,
>   .restart = rza_wdt_restart,
> };
> 

It wasn't needed before, but that doesn't mean it is not needed now.

> 
> If you really don't like checking in .ping, I can add a set_timeout 
> function and remove the local priv->timeout.
> 

No, I do not like checking and setting the timeout in the ping function
at all. That is what the set_timeout function is for, and I don't see
a point in bypassing the infrastructure.

Guenter

Re: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts

[Guenter Roeck]

On Mon, Sep 10, 2018 at 01:53:28PM +, Chris Brandt wrote:
> On Saturday, September 08, 2018 1, Guenter Roeck wrote:
> > > +#define CKS_3BIT 0x7
> > > +#define CKS_4BIT 0xF
> >
> > Any special reason for the value of those defines ? They are just used as
> > flags,
> > or am I missing something ? Why not just use 0 / 1 or an enum ?
> 
> Geert's suggestion was:
> 
>   >> I suggest storing cks in rza_wdt_of_match[].data, and
>   >> retrieving it with of_device_get_match_data() in your
>   >> probe function...
> 
> So now I just literally read in the value I want to write into CKS 
> register in the probe function.
> 
> priv->cks = (unsigned int)of_device_get_match_data(>dev);
> 
> And since I want to slowest clock source (CKS) possible, that's '0x7' if
> CKS is only 3 bits, and '0xF' if CKS is 4 bits.
> I can add a comment above the #define to explain my reasoning.
> 
Yes, that would help.

> 
> > >   struct rza_wdt {
> > >   struct watchdog_device wdev;
> > >   void __iomem *base;
> > >   struct clk *clk;
> > > + u8 count;
> > > + u8 cks;
> > > + u8 timeout;
> > 
> > Hmm ... this limits the effective timeout to 255 seconds. That seems odd.
> > Maybe it is true in practice, if the clock is always guaranteed to be
> > above 4194304 Hz, but it is an odd assumption that isn't really reflected
> > in the code.
> 
> I can change that to something else like u16.
> 
Sorry, I see no point ion 1) keeping this a separate variable and not using
the one in the watchdog data structure.

> In reality, there are 2 variations of HW:
> 
> #1. If the CKS is only 3-bits, the max HW timeout is 200ms, so I'm 
> setting 'max_hw_heartbeat_ms' and then the use can set a timeout as long as 
> they want (but it's not really a true HW watchdog).
> 
> #2. If the CKS is only 4-bits, the max HW timeout is 32 seconds. (so 
> 'timeout' can never be more that a u8).
> 
That is not the point. The point is that there is no need to keep two
'timeout' variables.

> 
> > > + if (priv->cks == CKS_4BIT) {
> > > + ticks = DIV_ROUND_UP((timeout * rate), 4194304);
> > 
> > The ( ) around timeout * rate is unnecessary.
> 
> Yes, you're right.
> 
> 
> > Also, it would be nice
> > to have a define and explanation for 4194304 (and 0x40 would probably
> > be a better value to use).
> 
> The number "4194304" is exactly how it is documented in the hardware 
> manual, that is why I'm using it that way. Yes, 0x40 makes more 
> sense, but I like matching the device documenting as much as possible to 
> help the next person that comes along and has to debug this code.
> 
Use at least a define.

> 
> > > + if (ticks > 256)
> > > + ticks = 256;
> > 
> > If you keep this, you should as well recalculate timeout since it won't
> > match the expected value.
> > 
> > if (ticks > 256) {
> > ticks = 256;
> > timeout = ticks * 4194304 / rate;
> > }
> 
> That's a good point!
> 
> 
> > Not that it can ever happen, since max_timeout limits the value.
> > Personally I would rather see this dropped with a comment stating that
> > ticks <= 256 is guaranteed by max_timeout; I am not a friend of dead code
> > in the kernel.
> 
> I agree. I will drop this code and put a comment.
> 
> 
> > > @@ -75,7 +103,12 @@ static int rza_wdt_ping(struct watchdog_device
> > *wdev)
> > >   {
> > >   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> > >
> > > - writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> > > + if (priv->timeout != wdev->timeout)
> > > + rza_wdt_calc_timeout(priv, wdev->timeout);
> > > +
> > FWIW, odd way of updating the timeout. Why not do it in the set_timeout()
> > function where it belongs. Which makes me wonder why priv->timeout is
> > needed
> > in the first place (and why it is u8 - as mentioned above).
> 
> Because when I was doing all my testing, I found cases where '.ping' was
> called from the upper layer without '.start' being called first. So, I 
> changed the code as you see it now. This guaranteed I would also get the
> timeout the user was requesting.
> 
That would only happen if the watchdog is considered to be running. 
Also, we are talking about the set_timeout function which is the one which
should set the timeout and update the HW if needed, ie if the watchdog is
already running.

> 
> > > + writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
> > > +
> > > + pr_debug("%s: timeout = %u\n", __func__, wdev->timeout);
> > >
> > 
> > Do you really want this displayed with each ping, even as debug message ?
> > Just wondering.
> 
> This is how you can see that sometimes '.ping' is called without '.start'
> being called first.
> 
If that happens and the watchdog was not already started, it would be
a bug that would affect all watchdog drivers. If that is the case,
working around it in a driver is most definitely the wrong solution.

Guenter

Re: watchdog: renesas_wdt: convert to SPDX identifiers

[Guenter Roeck]

On Fri, Sep 07, 2018 at 02:10:58AM +, Kuninori Morimoto wrote:
> From: Kuninori Morimoto 
> 
> This patch updates license to use SPDX-License-Identifier
> instead of verbose license text.
> 
> Signed-off-by: Kuninori Morimoto 
> Reviewed-by: Guenter Roeck 

Hmmm .. any chance for you folks to synchronize with each other ?
I already have "watchdog: use SPDX identifier for Renesas drivers",
submitted by Wolfram Sang a couple of weeks ago.

Guenter

> ---
>  drivers/watchdog/renesas_wdt.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 88d81fe..84bb9d3 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -1,12 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Watchdog driver for Renesas WDT watchdog
>   *
>   * Copyright (C) 2015-17 Wolfram Sang, Sang Engineering 
> 
>   * Copyright (C) 2015-17 Renesas Electronics Corporation
> - *
> - * This program is free software; you can redistribute it and/or modify it
> - * under the terms of the GNU General Public License version 2 as published 
> by
> - * the Free Software Foundation.
>   */
>  #include 
>  #include 

Re: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts

[Guenter Roeck]

On 09/06/2018 06:22 PM, Chris Brandt wrote:

The RZ/A2 watchdog timer extends the clock source options in order to
allow for longer timeouts.

Signed-off-by: Chris Brandt 
---
v3:
  * Removed + 1 from DIV_ROUND_UP line
  * resetting to 0 if time to big did not make as much sense are resetting
to 256
v2:
* use DIV_ROUND_UP
* use %u for pr_debug
* use of_match data to determine the size of CKS register
---
  drivers/watchdog/rza_wdt.c | 81 +++---
  1 file changed, 63 insertions(+), 18 deletions(-)

diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
index e618218d2374..64a733492b96 100644
--- a/drivers/watchdog/rza_wdt.c
+++ b/drivers/watchdog/rza_wdt.c
@@ -14,6 +14,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -34,12 +35,40 @@

  #define WRCSR_RSTEBIT(6)
  #define WRCSR_CLEAR_WOVF  0xA500  /* special value */
  
+#define CKS_3BIT		0x7

+#define CKS_4BIT   0xF
+


Any special reason for the value of those defines ? They are just used as flags,
or am I missing something ? Why not just use 0 / 1 or an enum ?


  struct rza_wdt {
struct watchdog_device wdev;
void __iomem *base;
struct clk *clk;
+   u8 count;
+   u8 cks;
+   u8 timeout;


Hmm ... this limits the effective timeout to 255 seconds. That seems odd.
Maybe it is true in practice, if the clock is always guaranteed to be
above 4194304 Hz, but it is an odd assumption that isn't really reflected
in the code.


  };
  
+static void rza_wdt_calc_timeout(struct rza_wdt *priv, int timeout)

+{
+   unsigned long rate = clk_get_rate(priv->clk);
+   unsigned int ticks;
+
+   if (priv->cks == CKS_4BIT) {
+   ticks = DIV_ROUND_UP((timeout * rate), 4194304);


The ( ) around timeout * rate is unnecessary. Also, it would be nice
to have a define and explanation for 4194304 (and 0x40 would probably
be a better value to use).


+   if (ticks > 256)
+   ticks = 256;


If you keep this, you should as well recalculate timeout since it won't
match the expected value.

if (ticks > 256) {
ticks = 256;
timeout = ticks * 4194304 / rate;
}

Not that it can ever happen, since max_timeout limits the value.
Personally I would rather see this dropped with a comment stating that
ticks <= 256 is guaranteed by max_timeout; I am not a friend of dead code
in the kernel.


+
+   priv->count = 256 - ticks;
+   } else {
+   /* Start timer with longest timeout */
+   priv->count = 0;
+   }
+
+   priv->timeout = timeout;
+
+   pr_debug("%s: timeout set to %u (WTCNT=%d)\n", __func__,
+timeout, priv->count);
+}
+
  static int rza_wdt_start(struct watchdog_device *wdev)
  {
struct rza_wdt *priv = watchdog_get_drvdata(wdev);
@@ -51,13 +80,12 @@ static int rza_wdt_start(struct watchdog_device *wdev)
readb(priv->base + WRCSR);
writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
  
-	/*

-* Start timer with slowest clock source and reset option enabled.
-*/
+   rza_wdt_calc_timeout(priv, wdev->timeout);
+
writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
-   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
-   writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
-  priv->base + WTCSR);
+   writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
+   writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME |
+  WTSCR_CKS(priv->cks), priv->base + WTCSR);
  
  	return 0;

  }
@@ -75,7 +103,12 @@ static int rza_wdt_ping(struct watchdog_device *wdev)
  {
struct rza_wdt *priv = watchdog_get_drvdata(wdev);
  
-	writew(WTCNT_MAGIC | 0, priv->base + WTCNT);

+   if (priv->timeout != wdev->timeout)
+   rza_wdt_calc_timeout(priv, wdev->timeout);
+

FWIW, odd way of updating the timeout. Why not do it in the set_timeout()
function where it belongs. Which makes me wonder why priv->timeout is needed
in the first place (and why it is u8 - as mentioned above).


+   writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
+
+   pr_debug("%s: timeout = %u\n", __func__, wdev->timeout);
  


Do you really want this displayed with each ping, even as debug message ?
Just wondering.


return 0;
  }
@@ -150,20 +183,31 @@ static int rza_wdt_probe(struct platform_device *pdev)
return -ENOENT;
}
  
-	/* Assume slowest clock rate possible (CKS=7) */

-   rate /= 16384;
-
priv->wdev.info = _wdt_ident,
priv->wdev.ops = _wdt_ops,
priv->wdev.parent = >dev;
  
-	/*

-* Since the max possible timeout of our 8-bit count register is less
-* than a second, we must use max_hw_heartbeat_ms.
-*/
-   priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX) / rate;
-   dev_dbg(>dev, "max 

Re: [PATCH] watchdog: rza_wdt: convert to SPDX identifiers

[Guenter Roeck]

On Fri, Sep 07, 2018 at 02:11:17AM +, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto 
> 
> This patch updates license to use SPDX-License-Identifier
> instead of verbose license text.
> 
> Signed-off-by: Kuninori Morimoto 

Reviewed-by: Guenter Roeck 

> ---
>  drivers/watchdog/rza_wdt.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
> index e618218..c63ef03 100644
> --- a/drivers/watchdog/rza_wdt.c
> +++ b/drivers/watchdog/rza_wdt.c
> @@ -1,12 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Renesas RZ/A Series WDT Driver
>   *
>   * Copyright (C) 2017 Renesas Electronics America, Inc.
>   * Copyright (C) 2017 Chris Brandt
> - *
> - * This file is subject to the terms and conditions of the GNU General Public
> - * License.  See the file "COPYING" in the main directory of this archive
> - * for more details.
>   */
>  
>  #include 
> -- 
> 2.7.4
> 

Re: [PATCH] watchdog: renesas_wdt: convert to SPDX identifiers

[Guenter Roeck]

On Fri, Sep 07, 2018 at 02:10:58AM +, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto 
> 
> This patch updates license to use SPDX-License-Identifier
> instead of verbose license text.
> 
> Signed-off-by: Kuninori Morimoto 

Reviewed-by: Guenter Roeck 

> ---
>  drivers/watchdog/renesas_wdt.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 88d81fe..84bb9d3 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -1,12 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Watchdog driver for Renesas WDT watchdog
>   *
>   * Copyright (C) 2015-17 Wolfram Sang, Sang Engineering 
> 
>   * Copyright (C) 2015-17 Renesas Electronics Corporation
> - *
> - * This program is free software; you can redistribute it and/or modify it
> - * under the terms of the GNU General Public License version 2 as published 
> by
> - * the Free Software Foundation.
>   */
>  #include 
>  #include 
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/2] watchdog: rza_wdt: Support longer timeouts

[Guenter Roeck]

On Thu, Sep 06, 2018 at 11:52:47AM -0700, Guenter Roeck wrote:
> On Thu, Sep 06, 2018 at 01:37:37PM -0500, Chris Brandt wrote:
> > The RZ/A2 watchdog timer extends the clock source options in order to
> > allow for longer timeouts.
> > 
> > Signed-off-by: Chris Brandt 
> > ---
> > v2:
> > * use DIV_ROUND_UP
> > * use %u for pr_debug
> > * use of_match data to determine the size of CKS register
> > ---
> >  drivers/watchdog/rza_wdt.c | 81 
> > +++---
> >  1 file changed, 63 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
> > index e618218d2374..d55eb947d08c 100644
> > --- a/drivers/watchdog/rza_wdt.c
> > +++ b/drivers/watchdog/rza_wdt.c
> > @@ -14,6 +14,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  
> > @@ -34,12 +35,40 @@
> >  #define WRCSR_RSTE BIT(6)
> >  #define WRCSR_CLEAR_WOVF   0xA500  /* special value */
> >  
> > +#define CKS_3BIT   0x7
> > +#define CKS_4BIT   0xF
> > +
> >  struct rza_wdt {
> > struct watchdog_device wdev;
> > void __iomem *base;
> > struct clk *clk;
> > +   u8 count;
> > +   u8 cks;
> > +   u8 timeout;
> >  };
> >  
> > +static void rza_wdt_calc_timeout(struct rza_wdt *priv, int timeout)
> > +{
> > +   int rate = clk_get_rate(priv->clk);
> 
> clk_get_rate() returns unsigned long.
> 
> > +   u16 counter;
> > +
> > +   if (priv->cks == CKS_4BIT) {
> > +   counter = DIV_ROUND_UP((timeout * rate),  4194304) + 1;
> 
> two spaces ?
> 
> Also, I am not sure how this prevents overflows. Was't the concern
> that timeout * rate might overflow an int ?
> 
> Also, why still "+ 1" ? Wasn't DIV_ROUND_UP() supposed to take care
> of that ?
> 
> > +   if (counter > 255)
> > +   counter = 0;
> 
> This is difficult to understand. 
> 
> > +
> > +   priv->count = 256 - counter;
> 
> It sets priv->count to 256, meaning that 0x5B00 instead of
> 0x5Axx will be written into the counter register. That really
> asks for some explanation.
> 

No, wait, priv->count is an 8-bit variable, so this really sets
priv->counter to 0. I am getting more and more confused. Why
not just set "counter = 256" above to make it obvious if that
is what is supposed to happen ?

Guenter

> > +   } else {
> > +   /* Start timer with longest timeout */
> > +   priv->count = 0;
> > +   }
> > +
> > +   priv->timeout = timeout;
> > +
> > +   pr_debug("%s: timeout set to %u (WTCNT=%d)\n", __func__,
> > +timeout, priv->count);
> > +}
> > +
> >  static int rza_wdt_start(struct watchdog_device *wdev)
> >  {
> > struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> > @@ -51,13 +80,12 @@ static int rza_wdt_start(struct watchdog_device *wdev)
> > readb(priv->base + WRCSR);
> > writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
> >  
> > -   /*
> > -* Start timer with slowest clock source and reset option enabled.
> > -*/
> > +   rza_wdt_calc_timeout(priv, wdev->timeout);
> > +
> > writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
> > -   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> > -   writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
> > -  priv->base + WTCSR);
> > +   writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
> > +   writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME |
> > +  WTSCR_CKS(priv->cks), priv->base + WTCSR);
> >  
> > return 0;
> >  }
> > @@ -75,7 +103,12 @@ static int rza_wdt_ping(struct watchdog_device *wdev)
> >  {
> > struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> >  
> > -   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> > +   if (priv->timeout != wdev->timeout)
> > +   rza_wdt_calc_timeout(priv, wdev->timeout);
> > +
> > +   writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
> > +
> > +   pr_debug("%s: timeout = %u\n", __func__, wdev->timeout);
> >  
> > return 0;
> >  }
> > @@ -150,20 +183,31 @@ static int rza_wdt_probe(struct platform_device *pdev)
> > return -ENOENT;
> > }
> >  
> > -   /* Assume slowest clock rate possible (CKS=7) */
> > -   rate /= 1638

Re: [PATCH v2 1/2] watchdog: rza_wdt: Support longer timeouts

[Guenter Roeck]

On Thu, Sep 06, 2018 at 01:37:37PM -0500, Chris Brandt wrote:
> The RZ/A2 watchdog timer extends the clock source options in order to
> allow for longer timeouts.
> 
> Signed-off-by: Chris Brandt 
> ---
> v2:
> * use DIV_ROUND_UP
> * use %u for pr_debug
> * use of_match data to determine the size of CKS register
> ---
>  drivers/watchdog/rza_wdt.c | 81 
> +++---
>  1 file changed, 63 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
> index e618218d2374..d55eb947d08c 100644
> --- a/drivers/watchdog/rza_wdt.c
> +++ b/drivers/watchdog/rza_wdt.c
> @@ -14,6 +14,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -34,12 +35,40 @@
>  #define WRCSR_RSTE   BIT(6)
>  #define WRCSR_CLEAR_WOVF 0xA500  /* special value */
>  
> +#define CKS_3BIT 0x7
> +#define CKS_4BIT 0xF
> +
>  struct rza_wdt {
>   struct watchdog_device wdev;
>   void __iomem *base;
>   struct clk *clk;
> + u8 count;
> + u8 cks;
> + u8 timeout;
>  };
>  
> +static void rza_wdt_calc_timeout(struct rza_wdt *priv, int timeout)
> +{
> + int rate = clk_get_rate(priv->clk);

clk_get_rate() returns unsigned long.

> + u16 counter;
> +
> + if (priv->cks == CKS_4BIT) {
> + counter = DIV_ROUND_UP((timeout * rate),  4194304) + 1;

two spaces ?

Also, I am not sure how this prevents overflows. Was't the concern
that timeout * rate might overflow an int ?

Also, why still "+ 1" ? Wasn't DIV_ROUND_UP() supposed to take care
of that ?

> + if (counter > 255)
> + counter = 0;

This is difficult to understand. 

> +
> + priv->count = 256 - counter;

It sets priv->count to 256, meaning that 0x5B00 instead of
0x5Axx will be written into the counter register. That really
asks for some explanation.

> + } else {
> + /* Start timer with longest timeout */
> + priv->count = 0;
> + }
> +
> + priv->timeout = timeout;
> +
> + pr_debug("%s: timeout set to %u (WTCNT=%d)\n", __func__,
> +  timeout, priv->count);
> +}
> +
>  static int rza_wdt_start(struct watchdog_device *wdev)
>  {
>   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> @@ -51,13 +80,12 @@ static int rza_wdt_start(struct watchdog_device *wdev)
>   readb(priv->base + WRCSR);
>   writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
>  
> - /*
> -  * Start timer with slowest clock source and reset option enabled.
> -  */
> + rza_wdt_calc_timeout(priv, wdev->timeout);
> +
>   writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
> - writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> - writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
> -priv->base + WTCSR);
> + writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
> + writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME |
> +WTSCR_CKS(priv->cks), priv->base + WTCSR);
>  
>   return 0;
>  }
> @@ -75,7 +103,12 @@ static int rza_wdt_ping(struct watchdog_device *wdev)
>  {
>   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
>  
> - writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> + if (priv->timeout != wdev->timeout)
> + rza_wdt_calc_timeout(priv, wdev->timeout);
> +
> + writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
> +
> + pr_debug("%s: timeout = %u\n", __func__, wdev->timeout);
>  
>   return 0;
>  }
> @@ -150,20 +183,31 @@ static int rza_wdt_probe(struct platform_device *pdev)
>   return -ENOENT;
>   }
>  
> - /* Assume slowest clock rate possible (CKS=7) */
> - rate /= 16384;
> -
>   priv->wdev.info = _wdt_ident,
>   priv->wdev.ops = _wdt_ops,
>   priv->wdev.parent = >dev;
>  
> - /*
> -  * Since the max possible timeout of our 8-bit count register is less
> -  * than a second, we must use max_hw_heartbeat_ms.
> -  */
> - priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX) / rate;
> - dev_dbg(>dev, "max hw timeout of %dms\n",
> -  priv->wdev.max_hw_heartbeat_ms);
> + priv->cks = (unsigned int)of_device_get_match_data(>dev);
> + if (priv->cks == CKS_4BIT) {
> + /* Assume slowest clock rate possible (CKS=0xF) */
> + priv->wdev.max_timeout = (4194304 * U8_MAX) / rate;
> +
> + } else if (priv->cks == CKS_3BIT) {
> + /* Assume slowest clock rate possible (CKS=7) */
> + rate /= 16384;
> +
> + /*
> +  * Since the max possible timeout of our 8-bit count
> +  * register is less than a second, we must use
> +  * max_hw_heartbeat_ms.
> +  */
> + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX) / rate;
> + dev_dbg(>dev, "max hw timeout of %dms\n",
> +  priv->wdev.max_hw_heartbeat_ms);
> + } else {
> + 

Re: [RFC PATCH v3 1/4] watchdog: core: add mechanism to prevent removing if NOWAYOUT

[Guenter Roeck]

On Wed, Aug 29, 2018 at 09:42:38AM +0200, Wolfram Sang wrote:
> To prevent removing if NOWAYOUT, we invalidate the .remove function and
> suppress the bind/unbind attributes in sysfs. These are driver
> capabilities, so we need to set it up at runtime during init. To avoid
> boilerplate, introduce module_watchdog_driver() similar to
> module_driver(). On top of that, we then build
> module_watchdog_platform_driver(). Others may follow, if needed.
> 
> Signed-off-by: Wolfram Sang 
> ---
>  include/linux/watchdog.h | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 44985c4a1e86..c8ecbc53c807 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -216,4 +216,21 @@ extern void watchdog_unregister_device(struct 
> watchdog_device *);
>  /* devres register variant */
>  int devm_watchdog_register_device(struct device *dev, struct watchdog_device 
> *);
>  
> +#define module_watchdog_driver(__driver, __register, __unregister, 
> __nowayout, ...) \

Another option might be to add another argument to this macro. Something like:

#define module_watchdog_driver(__wdriver, __driver, __register, __unregister, 
__nowayout, ...) \
static int __init __wdriver##_init(void) \
{ \
__driver->suppress_bind_attrs = !!(__nowayout); \
return __register(&(__wdriver)  ##__VA_ARGS__); \
} \
module_init(__wdriver##_init); \
static void __exit __wdriver##_exit(void) \
{ \
__unregister(&(__wdriver), ##__VA_ARGS__); \
} \
module_exit(__wdriver##_exit)

#define module_watchdog_platform_driver(__platform_driver, __nowayout) \
module_watchdog_driver(__platform_driver, &__platform_driver->driver, \
platform_driver_register, \
platform_driver_unregister, __nowayout)

This would avoid the dependency of having a ".driver" structure in each
supported bus driver structure.

Would that make sense ?

Guenter

Re: [RFC PATCH v2 1/4] watchdog: core: add mechanism to prevent removing if NOWAYOUT

[Guenter Roeck]

On Tue, Aug 28, 2018 at 10:07:40PM +0200, Wolfram Sang wrote:
> Hi Guenter,
> 
> > > + __driver.remove = NULL; \
> > 
> > Does that really do any good ? If I understand correctly, the only
> > impact is that the platform driver remove function will believe that
> > nothing needs to be done on removal. See platform_drv_remove().
> 
> This might be biased for my use case. Not calling remove will also leave
> the clock for that module enabled. But true, the platform device will be
> removed and without proper cleanup, there could be dangling pointers.
> 

For many drivers the watchdog device would not be removed. That would
indeed be fatal.

> In general, what do you think of this approach if we'd left out the
> above line?
> 

LGTM without that line.

Thanks,
Guenter

Re: [RFC PATCH v2 1/4] watchdog: core: add mechanism to prevent removing if NOWAYOUT

[Guenter Roeck]

On Tue, Aug 28, 2018 at 09:14:13PM +0200, Wolfram Sang wrote:
> To prevent removing if NOWAYOUT, we invalidate the .remove function and
> suppress the bind/unbind attributes in sysfs. These are driver
> capabilities, so we need to set it up at runtime during init. To avoid
> boilerplate, introduce module_watchdog_driver() similar to
> module_driver(). On top of that, we then build
> module_watchdog_platform_driver(). Others may follow, if needed.
> 
> Signed-off-by: Wolfram Sang 
> ---
>  include/linux/watchdog.h | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 44985c4a1e86..5768fb6b5cde 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -216,4 +216,24 @@ extern void watchdog_unregister_device(struct 
> watchdog_device *);
>  /* devres register variant */
>  int devm_watchdog_register_device(struct device *dev, struct watchdog_device 
> *);
>  
> +#define module_watchdog_driver(__driver, __register, __unregister, 
> __nowayout, ...) \
> +static int __init __driver##_init(void) \
> +{ \
> + if (__nowayout) { \
> + __driver.driver.suppress_bind_attrs = true; \
> + __driver.remove = NULL; \

Does that really do any good ? If I understand correctly, the only
impact is that the platform driver remove function will believe that
nothing needs to be done on removal. See platform_drv_remove().

> + } \
> + return __register(&(__driver)  ##__VA_ARGS__); \
> +} \
> +module_init(__driver##_init); \
> +static void __exit __driver##_exit(void) \
> +{ \
> + __unregister(&(__driver), ##__VA_ARGS__); \
> +} \
> +module_exit(__driver##_exit)
> +
> +#define module_watchdog_platform_driver(__platform_driver, __nowayout) \
> + module_watchdog_driver(__platform_driver, platform_driver_register, \
> + platform_driver_unregister, __nowayout)
> +
>  #endif  /* ifndef _LINUX_WATCHDOG_H */
> -- 
> 2.11.0
> 

Re: [RFC PATCH] watchdog: HACK: disable bind attributes with NOWAYOUT

[Guenter Roeck]

On 08/28/2018 03:29 AM, Wolfram Sang wrote:

With NOWAYOUT, prevent bind/unbind possibilities in SYSFS.
Proof-of-concept, not for upstream yet.

Signed-off-by: Wolfram Sang 
---

So, this is really an RFC to check if something like this is considered useful
or not. If so, we probably need to do it differently because modifying the
parent's driver is likely a layering violation. We could add the driver to
modify as an optional parameter to watchdog_set_nowayout(). I wouldn't favor
another seperate function to configure this, but am open for discussion.

Thanks,

Wolfram

  include/linux/watchdog.h | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 44985c4a1e86..241de0fa0010 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -143,8 +143,10 @@ static inline bool watchdog_hw_running(struct 
watchdog_device *wdd)
  /* Use the following function to set the nowayout feature */
  static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool 
nowayout)
  {
-   if (nowayout)
+   if (nowayout) {
set_bit(WDOG_NO_WAY_OUT, >status);
+   wdd->parent->driver->suppress_bind_attrs = true;


That makes sense to me. We can not assume that wdd->parent is set, so it won't 
work as-is.
Not sure what a "correct" solution might be. Passing "parent" as argument 
doesn't really
solve any layering argument - stating that it violates layering if parent is 
pulled from
wdd but not if it is passed as argument seems to be a bit ridiculous.

Did you ensure that the attributes are not already created by the time
suppress_bind_attrs is set ?

Thanks,
Guenter

Re: [PATCH 2/2] watchdog: renesas_wdt: stop when unregistering

[Guenter Roeck]

On 08/28/2018 03:13 AM, Wolfram Sang wrote:

We want to go into a sane state when unregistering. Currently, it
happens that the watchdog stops when unbinding because of RuntimePM
stopping the core clock. When rebinding, the core clock gets reactivated
and the watchdog fires even though it hasn't been opened by userspace
yet. Strange scenario, yes, but sane state is much preferred anyhow.

Signed-off-by: Wolfram Sang 


Reviewed-by: Guenter Roeck 


---
  drivers/watchdog/renesas_wdt.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 88d81feba4e6..f45cb183fa75 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -234,6 +234,7 @@ static int rwdt_probe(struct platform_device *pdev)
watchdog_set_drvdata(>wdev, priv);
watchdog_set_nowayout(>wdev, nowayout);
watchdog_set_restart_priority(>wdev, 0);
+   watchdog_stop_on_unregister(>wdev);
  
  	/* This overrides the default timeout only if DT configuration was found */

ret = watchdog_init_timeout(>wdev, 0, >dev);

Re: [PATCH 1/2] watchdog: core: fix null pointer dereference when releasing cdev

[Guenter Roeck]

On 08/28/2018 03:13 AM, Wolfram Sang wrote:

watchdog_stop() calls watchdog_update_worker() which needs a valid
wdd->wd_data pointer. So, when unregistering the cdev, clear the
pointers after we call watchdog_stop(), not before.

Fixes: bb292ac1c602 ("watchdog: Introduce watchdog_stop_on_unregister helper")
Signed-off-by: Wolfram Sang 


Reviewed-by: Guenter Roeck 


---
  drivers/watchdog/watchdog_dev.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ffbdc4642ea5..f6c24b22b37c 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -1019,16 +1019,16 @@ static void watchdog_cdev_unregister(struct 
watchdog_device *wdd)
old_wd_data = NULL;
}
  
-	mutex_lock(_data->lock);

-   wd_data->wdd = NULL;
-   wdd->wd_data = NULL;
-   mutex_unlock(_data->lock);
-
if (watchdog_active(wdd) &&
test_bit(WDOG_STOP_ON_UNREGISTER, >status)) {
watchdog_stop(wdd);
}
  
+	mutex_lock(_data->lock);

+   wd_data->wdd = NULL;
+   wdd->wd_data = NULL;
+   mutex_unlock(_data->lock);
+
hrtimer_cancel(_data->timer);
kthread_cancel_work_sync(_data->work);
  

Re: [PATCH v2] dt-bindings: watchdog: renesas-wdt: Add support for the R8A77990 wdt

[Guenter Roeck]

Hi Geert,

On 07/06/2018 01:55 AM, Geert Uytterhoeven wrote:

From: Masaharu Hayakawa 

Document support for the Watchdog Timer (WDT) Controller in the Renesas
R-Car E3 (R8A77990) SoC.

No driver update is needed.

Signed-off-by: Masaharu Hayakawa 
Signed-off-by: Geert Uytterhoeven 
Reviewed-by: Guenter Roeck 
Acked-by: Rob Herring 


The patch is queued in my tree, so there should be no need to resend it anymore.

Guenter


---
v2:
   - Add Reviewed-by, Acked-by.
---
  Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 1 +
  1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
index f24d802b8056f6c6..5d47a262474cfe0e 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
@@ -16,6 +16,7 @@ Required properties:
 - "renesas,r8a7796-wdt" (R-Car M3-W)
 - "renesas,r8a77965-wdt" (R-Car M3-N)
 - "renesas,r8a77970-wdt" (R-Car V3M)
+- "renesas,r8a77990-wdt" (R-Car E3)
 - "renesas,r8a77995-wdt" (R-Car D3)
 - "renesas,r7s72100-wdt" (RZ/A1)
The generic compatible string must be:

Re: dt-bindings: watchdog: renesas-wdt: Add support for the R8A77990 wdt

[Guenter Roeck]

On Tue, Jun 05, 2018 at 07:18:33PM +0200, Geert Uytterhoeven wrote:
> From: Masaharu Hayakawa 
> 
> Document support for the Watchdog Timer (WDT) Controller in the Renesas
> R-Car E3 (R8A77990) SoC.
> 
> No driver update is needed.
> 
> Signed-off-by: Masaharu Hayakawa 
> Signed-off-by: Geert Uytterhoeven 

This refuses to apply for me. What tree is it based on ?

Guenter

> ---
>  Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
> b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> index f24d802b8056f6c6..5d47a262474cfe0e 100644
> --- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> @@ -16,6 +16,7 @@ Required properties:
>- "renesas,r8a7796-wdt" (R-Car M3-W)
>- "renesas,r8a77965-wdt" (R-Car M3-N)
>- "renesas,r8a77970-wdt" (R-Car V3M)
> +  - "renesas,r8a77990-wdt" (R-Car E3)
>- "renesas,r8a77995-wdt" (R-Car D3)
>- "renesas,r7s72100-wdt" (RZ/A1)
>   The generic compatible string must be:

Re: dt-bindings: watchdog: renesas-wdt: Add support for the R8A77990 wdt

[Guenter Roeck]

On Tue, Jun 05, 2018 at 07:18:33PM +0200, Geert Uytterhoeven wrote:
> From: Masaharu Hayakawa 
> 
> Document support for the Watchdog Timer (WDT) Controller in the Renesas
> R-Car E3 (R8A77990) SoC.
> 
> No driver update is needed.
> 
> Signed-off-by: Masaharu Hayakawa 
> Signed-off-by: Geert Uytterhoeven 

Reviewed-by: Guenter Roeck 

> ---
>  Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
> b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> index f24d802b8056f6c6..5d47a262474cfe0e 100644
> --- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> @@ -16,6 +16,7 @@ Required properties:
>- "renesas,r8a7796-wdt" (R-Car M3-W)
>- "renesas,r8a77965-wdt" (R-Car M3-N)
>- "renesas,r8a77970-wdt" (R-Car V3M)
> +  - "renesas,r8a77990-wdt" (R-Car E3)
>- "renesas,r8a77995-wdt" (R-Car D3)
>- "renesas,r7s72100-wdt" (RZ/A1)
>   The generic compatible string must be:

Re: [PATCH 4/9] hwmon: fschmd: fix typo 'can by' to 'can be'

[Guenter Roeck]

On 05/06/2018 04:23 AM, Wolfram Sang wrote:

Signed-off-by: Wolfram Sang 


Applied to hwmon-next.

Thanks,
Guenter


---
  drivers/hwmon/fschmd.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/fschmd.c b/drivers/hwmon/fschmd.c
index 5e78229ade049f..22d3a84f13ef58 100644
--- a/drivers/hwmon/fschmd.c
+++ b/drivers/hwmon/fschmd.c
@@ -105,7 +105,7 @@ static const u8 FSCHMD_REG_VOLT[7][6] = {
  static const int FSCHMD_NO_VOLT_SENSORS[7] = { 3, 3, 3, 3, 3, 3, 6 };
  
  /*

- * minimum pwm at which the fan is driven (pwm can by increased depending on
+ * minimum pwm at which the fan is driven (pwm can be increased depending on
   * the temp. Notice that for the scy some fans share there minimum speed.
   * Also notice that with the scy the sensor order is different than with the
   * other chips, this order was in the 2.4 driver and kept for consistency.

Re: [PATCH v3] watchdog: renesas-wdt: Add support for the R8A77965 WDT

[Guenter Roeck]

On 05/07/2018 05:29 AM, Wolfram Sang wrote:

Document support for the Watchdog Timer (WDT) Controller in the Renesas
R-Car M3-N (R8A77965) SoC. No driver update is needed.

Signed-off-by: Takeshi Kihara <takeshi.kihara...@renesas.com>
[wsa: rebased to v4.17-rc3]
Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>


This one applies.

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

Thanks!
Guenter


---

Change since V2: rebase to 4.17-rc3

  Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 1 +
  1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
index 74b2f03c1515..fa56697a1ba6 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
@@ -7,6 +7,7 @@ Required properties:
 - "renesas,r7s72100-wdt" (RZ/A1)
 - "renesas,r8a7795-wdt" (R-Car H3)
 - "renesas,r8a7796-wdt" (R-Car M3-W)
+- "renesas,r8a77965-wdt" (R-Car M3-N)
 - "renesas,r8a77970-wdt" (R-Car V3M)
 - "renesas,r8a77995-wdt" (R-Car D3)
  

Re: [v2] watchdog: renesas-wdt: Add support for the R8A77965 WDT

[Guenter Roeck]

On Sat, Apr 14, 2018 at 12:18:58PM +0200, Wolfram Sang wrote:
> Document support for the Watchdog Timer (WDT) Controller in the Renesas
> R-Car M3-N (R8A77965) SoC. No driver update is needed.
> 
> Signed-off-by: Takeshi Kihara <takeshi.kihara...@renesas.com>
> [wsa: rebased to top-of-tree]
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>
> Reviewed-by: Guenter Roeck <li...@roeck-us.net>
> Reviewed-by: Simon Horman <horms+rene...@verge.net.au>
> Reviewed-by: Vaishali Thakkar <vthak...@vaishalithakkar.in>

Unfortunately this patch isn't based on anything recognized by git,
and even though it is simple I can't get it to apply. Can someone please
rebase it to mainline and resend ?

Thanks,
Guenter

> ---
> 
> Change since v1: s/M3N/M3-N/ (Thanks, Geert!)
> 
>  Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
> b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> index 123bb1b2654b..613d860f2353 100644
> --- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> @@ -15,6 +15,7 @@ Required properties:
>- "renesas,r8a7794-wdt" (R-Car E2)
>- "renesas,r8a7795-wdt" (R-Car H3)
>- "renesas,r8a7796-wdt" (R-Car M3-W)
> +  - "renesas,r8a77965-wdt" (R-Car M3-N)
>- "renesas,r8a77970-wdt" (R-Car V3M)
>- "renesas,r8a77995-wdt" (R-Car D3)
>  The generic compatible string must be:

Re: [PATCH] watchdog: renesas-wdt: Remove R-Car M2-W ES2.x from blacklist

[Guenter Roeck]

On 04/20/2018 05:20 AM, Geert Uytterhoeven wrote:

System restart triggered by watchdog time-out works fine on a Koelsch
board with R-Car M2-W ES2.0.

Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>


Reviewed-by: Guenter Roeck <li...@roeck-us.net>


---
Thanks to Magnus and Shimoda-san for providing board access!
---
  drivers/watchdog/renesas_wdt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 514db5cc15951125..88d81feba4e60087 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -146,7 +146,7 @@ static const struct soc_device_attribute 
rwdt_quirks_match[] = {
.data = (void *)1,  /* needs single CPU */
}, {
.soc_id = "r8a7791",
-   .revision = "ES[12].*",
+   .revision = "ES1.*",
.data = (void *)1,  /* needs single CPU */
}, {
.soc_id = "r8a7792",

Re: [PATCH 59/61] watchdog: simplify getting .drvdata

[Guenter Roeck]

On Thu, Apr 19, 2018 at 04:06:29PM +0200, Wolfram Sang wrote:
> We should get drvdata from struct device directly. Going via
> platform_device is an unneeded step back and forth.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
> 
> Build tested only. buildbot is happy. Please apply individually.
> 
>  drivers/watchdog/cadence_wdt.c   | 6 ++
>  drivers/watchdog/of_xilinx_wdt.c | 6 ++
>  drivers/watchdog/wdat_wdt.c  | 6 ++
>  3 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
> index 3ec1f418837d..c3924356d173 100644
> --- a/drivers/watchdog/cadence_wdt.c
> +++ b/drivers/watchdog/cadence_wdt.c
> @@ -418,8 +418,7 @@ static void cdns_wdt_shutdown(struct platform_device 
> *pdev)
>   */
>  static int __maybe_unused cdns_wdt_suspend(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct cdns_wdt *wdt = platform_get_drvdata(pdev);
> + struct cdns_wdt *wdt = dev_get_drvdata(dev);
>  
>   if (watchdog_active(>cdns_wdt_device)) {
>   cdns_wdt_stop(>cdns_wdt_device);
> @@ -438,8 +437,7 @@ static int __maybe_unused cdns_wdt_suspend(struct device 
> *dev)
>  static int __maybe_unused cdns_wdt_resume(struct device *dev)
>  {
>   int ret;
> - struct platform_device *pdev = to_platform_device(dev);
> - struct cdns_wdt *wdt = platform_get_drvdata(pdev);
> + struct cdns_wdt *wdt = dev_get_drvdata(dev);
>  
>   if (watchdog_active(>cdns_wdt_device)) {
>   ret = clk_prepare_enable(wdt->clk);
> diff --git a/drivers/watchdog/of_xilinx_wdt.c 
> b/drivers/watchdog/of_xilinx_wdt.c
> index 4acbe05e27bb..d3f7eb046678 100644
> --- a/drivers/watchdog/of_xilinx_wdt.c
> +++ b/drivers/watchdog/of_xilinx_wdt.c
> @@ -268,8 +268,7 @@ static int xwdt_remove(struct platform_device *pdev)
>   */
>  static int __maybe_unused xwdt_suspend(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct xwdt_device *xdev = platform_get_drvdata(pdev);
> + struct xwdt_device *xdev = dev_get_drvdata(dev);
>  
>   if (watchdog_active(>xilinx_wdt_wdd))
>   xilinx_wdt_stop(>xilinx_wdt_wdd);
> @@ -285,8 +284,7 @@ static int __maybe_unused xwdt_suspend(struct device *dev)
>   */
>  static int __maybe_unused xwdt_resume(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct xwdt_device *xdev = platform_get_drvdata(pdev);
> + struct xwdt_device *xdev = dev_get_drvdata(dev);
>   int ret = 0;
>  
>   if (watchdog_active(>xilinx_wdt_wdd))
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index 0da9943d405f..56ad19608a9b 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -447,8 +447,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>  #ifdef CONFIG_PM_SLEEP
>  static int wdat_wdt_suspend_noirq(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct wdat_wdt *wdat = platform_get_drvdata(pdev);
> + struct wdat_wdt *wdat = dev_get_drvdata(dev);
>   int ret;
>  
>   if (!watchdog_active(>wdd))
> @@ -475,8 +474,7 @@ static int wdat_wdt_suspend_noirq(struct device *dev)
>  
>  static int wdat_wdt_resume_noirq(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct wdat_wdt *wdat = platform_get_drvdata(pdev);
> + struct wdat_wdt *wdat = dev_get_drvdata(dev);
>   int ret;
>  
>   if (!watchdog_active(>wdd))
> -- 
> 2.11.0
> 

Re: [PATCH v2] watchdog: renesas-wdt: Add support for the R8A77965 WDT

[Guenter Roeck]

On 04/14/2018 03:18 AM, Wolfram Sang wrote:

Document support for the Watchdog Timer (WDT) Controller in the Renesas
R-Car M3-N (R8A77965) SoC. No driver update is needed.

Signed-off-by: Takeshi Kihara <takeshi.kihara...@renesas.com>
[wsa: rebased to top-of-tree]
Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>


Reviewed-by: Guenter Roeck <li...@roeck-us.net>


---

Change since v1: s/M3N/M3-N/ (Thanks, Geert!)

  Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 1 +
  1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
index 123bb1b2654b..613d860f2353 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
@@ -15,6 +15,7 @@ Required properties:
 - "renesas,r8a7794-wdt" (R-Car E2)
 - "renesas,r8a7795-wdt" (R-Car H3)
 - "renesas,r8a7796-wdt" (R-Car M3-W)
+- "renesas,r8a77965-wdt" (R-Car M3-N)
 - "renesas,r8a77970-wdt" (R-Car V3M)
 - "renesas,r8a77995-wdt" (R-Car D3)
   The generic compatible string must be:

Re: [PATCH] watchdog: renesas-wdt: Add support for WDIOF_CARDRESET

[Guenter Roeck]

On Tue, Mar 20, 2018 at 10:36:26PM +0100, Wolfram Sang wrote:
> From: Veeraiyan Chidambaram 
> 
> This patch adds the WDIOF_CARDRESET support for the Renessas platform
> watchdog, to know if the board reboot is due to a watchdog reset.
> 
> This is done via the WOVF bit (bit 4) of the RWTCSRA register, which
> indicates if RWTCNT overflowed, triggering the reset in last boot.
> 
> Signed-off-by: Veeraiyan Chidambaram 
> [takeshi.kihara.df: changed to read the RWTCSRA register while clock is
>  enabled]
> Signed-off-by: Takeshi Kihara 
> Signed-off-by: Wolfram Sang 

Reviewed-by: Guenetr Roeck 

> ---
>  drivers/watchdog/renesas_wdt.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 6b8c6ddfe30b31..514db5cc159511 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -121,7 +121,8 @@ static int rwdt_restart(struct watchdog_device *wdev, 
> unsigned long action,
>  }
>  
>  static const struct watchdog_info rwdt_ident = {
> - .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
> + WDIOF_CARDRESET,
>   .identity = "Renesas WDT Watchdog",
>  };
>  
> @@ -197,9 +198,10 @@ static int rwdt_probe(struct platform_device *pdev)
>   return PTR_ERR(clk);
>  
>   pm_runtime_enable(>dev);
> -
>   pm_runtime_get_sync(>dev);
>   priv->clk_rate = clk_get_rate(clk);
> + priv->wdev.bootstatus = (readb_relaxed(priv->base + RWTCSRA) &
> + RWTCSRA_WOVF) ? WDIOF_CARDRESET : 0;
>   pm_runtime_put(>dev);
>  
>   if (!priv->clk_rate) {
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 2/3] watchdog: renesas_wdt: Add R-Car Gen2 support

[Guenter Roeck]

On Mon, Mar 05, 2018 at 03:30:25PM +, Fabrizio Castro wrote:
> Due to commits:
> * "ARM: shmobile: Add watchdog support",
> * "ARM: shmobile: rcar-gen2: Add watchdog support", and
> * "soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2",
> we now have everything we needed for the watchdog to work on Gen2 and
> RZ/G1.
> 
> However, on early revisions of some R-Car Gen2 SoCs, and depending on SMP
> configuration, the system may fail to restart on watchdog time-out, and
> lock up instead.
> 
> Specifically:
>   - On R-Car H2 ES1.0 and M2-W ES1.0, watchdog restart fails unless
> only the first CPU core is in use (using e.g. the "maxcpus=1" kernel
> commandline option).
>   - On R-Car V2H ES1.1, watchdog restart fails unless SMP is disabled
> completely (using CONFIG_SMP=n during build configuration, or using
> the "nosmp" or "maxcpus=0" kernel commandline options).
> 
> This commit adds "renesas,rcar-gen2-wdt" as compatible string for R-Car
> Gen2 and RZ/G1, but also prevents the system from using the watchdog
> driver in cases where the system would fail to restart by blacklisting
> the affected SoCs, using the minimum known working revisions (ES2.0 on R-Car
> H2, and ES3.0 on M2-W), and taking the actual SMP software configuration
> into account.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.cas...@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram 
> <ramesh.shanmugasunda...@bp.renesas.com>
> [Geert: blacklisting logic]
> Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
> ---
> v7->v8:
> * folded patch "watchdog: renesas_wdt: Blacklist early R-Car Gen2 SoCs" from
>   Geert into this patch.
> 
> Since Simon only reviewed Geert's patch, and Wolfram and Guenter only reviewed
> earlier versions of this patch, I am not including their "Reviewed-by" as they
> may disagree with the final result.
> 

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> Thanks,
> Fab
> 
>  drivers/watchdog/renesas_wdt.c | 49 
> +-
>  1 file changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 024d54e..0dede5b 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -16,6 +16,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  
>  #define RWTCNT   0
> @@ -121,6 +123,44 @@ static const struct watchdog_ops rwdt_ops = {
>   .get_timeleft = rwdt_get_timeleft,
>  };
>  
> +#if defined(CONFIG_ARCH_RCAR_GEN2) && defined(CONFIG_SMP)
> +/*
> + * Watchdog-reset integration is broken on early revisions of R-Car Gen2 SoCs
> + */
> +static const struct soc_device_attribute rwdt_quirks_match[] = {
> + {
> + .soc_id = "r8a7790",
> + .revision = "ES1.*",
> + .data = (void *)1,  /* needs single CPU */
> + }, {
> + .soc_id = "r8a7791",
> + .revision = "ES[12].*",
> + .data = (void *)1,  /* needs single CPU */
> + }, {
> + .soc_id = "r8a7792",
> + .revision = "*",
> + .data = (void *)0,  /* needs SMP disabled */
> + },
> + { /* sentinel */ }
> +};
> +
> +static bool rwdt_blacklisted(struct device *dev)
> +{
> + const struct soc_device_attribute *attr;
> +
> + attr = soc_device_match(rwdt_quirks_match);
> + if (attr && setup_max_cpus > (uintptr_t)attr->data) {
> + dev_info(dev, "Watchdog blacklisted on %s %s\n", attr->soc_id,
> +  attr->revision);
> + return true;
> + }
> +
> + return false;
> +}
> +#else /* !CONFIG_ARCH_RCAR_GEN2 || !CONFIG_SMP */
> +static inline bool rwdt_blacklisted(struct device *dev) { return false; }
> +#endif /* !CONFIG_ARCH_RCAR_GEN2 || !CONFIG_SMP */
> +
>  static int rwdt_probe(struct platform_device *pdev)
>  {
>   struct rwdt_priv *priv;
> @@ -129,6 +169,9 @@ static int rwdt_probe(struct platform_device *pdev)
>   unsigned long clks_per_sec;
>   int ret, i;
>  
> + if (rwdt_blacklisted(>dev))
> + return -ENODEV;
> +
>   priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
>   if (!priv)
>   return -ENOMEM;
> @@ -228,12 +271,8 @@ static int __maybe_unused rwdt_resume(struct device *dev)
>  
>  static SIMPLE_DEV_PM_OPS(rwdt_pm_ops, rwdt_suspend, rwdt_resume);
> 

Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Guenter Roeck]

On 03/02/2018 02:45 AM, Fabrizio Castro wrote:

Dear All,

perhaps someone from this email thread could explain to me what's the actual
(general) expectation from a system perspective (at resume) from the watchdog,
because I can see pitfalls whether 1) we simply start the watchdog at resume or
2) we pick up from where we left.

If we have a system that goes to sleep quite a bit, option 1) may cause the 
watchdog
to never fire, even though user space is not explicitly pinging the watchdog. 
As Geert
has pointed out, going to sleep and waking up adds a delay, therefore with 
option 2)
you may miss the opportunity to ping the watchdog and therefore the system may
restart even when it shouldn't. However, with option 2) user space can make
arrangements to compensate for the delay, and when user space compensates for
that it means the system is probably sane. With option 1) instead we are 
basically
pinging the watchdog without explicitly doing so from user space, which I don't 
think
is what we want here, but I may be wrong.

Could someone please shed some light here?


If the system goes to sleep so often that the watchdog never triggers just 
because of
that, it must either be in pretty good shape, in which case the watchdog 
doesn't need
to fire, or it is in bad shape, and the repeated stopping/restarting of the 
watchdog
would ultimately cause the system to die with the watchdog stopped anyway.

Overall, just the fact that the watchdog has to be stopped during suspend is a 
weak spot.
Bad luck if the system hangs after the watchdog was stopped. Since suspend is a 
critical
operation )in the sense that if anything goes wrong, that is the time for it), 
that is
a _real_ weak spot. If anything, we should be concerned about that, not about 
the exact
timing of watchdog pings.

Sure, you can leave it to user space to adjust for the resume time. Let's hope 
that the
watchdog daemon does that, and that it gets to run fast enough to actually do 
it.
I do wonder though how it would know. Are processes informed about a resume 
event ?

Personally I rather play it safe, meaning I rather give the watchdog a bit of 
additional
slack during resume. Having said that, as mentioned before, I am willing to 
accept
the patch as is, in the assumption that the authors know what they are doing.

Guenter


Thanks,
Fab



Subject: Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

Hi Fabrizio,

On Thu, Mar 1, 2018 at 7:17 PM, Fabrizio Castro
 wrote:

On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
on R-Car Gen3 we power the IP down during suspend.

This commit adds suspend/resume support, so that the watchdog counting
"pauses" during suspend on all of the SoCs compatible with this driver
and on those we are now adding support for (R-Car Gen2 and RZ/G1).

Signed-off-by: Fabrizio Castro 
Signed-off-by: Ramesh Shanmugasundaram 
---
v6->v7:
* backup and restore register RWTCNT instead of using rwdt_get_timeleft and
   rwdt_set_timeleft


Thanks for the update (v6 and v7)!



  drivers/watchdog/renesas_wdt.c | 26 ++
  1 file changed, 26 insertions(+)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 831ef83..024d54e 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -49,6 +49,7 @@ struct rwdt_priv {
 void __iomem *base;
 struct watchdog_device wdev;
 unsigned long clk_rate;
+   u16 time_left;
 u8 cks;
  };

@@ -203,6 +204,30 @@ static int rwdt_remove(struct platform_device *pdev)
 return 0;
  }

+static int __maybe_unused rwdt_suspend(struct device *dev)
+{
+   struct rwdt_priv *priv = dev_get_drvdata(dev);
+
+   if (watchdog_active(>wdev)) {
+   priv->time_left = readw(priv->base + RWTCNT);
+   rwdt_stop(>wdev);
+   }
+   return 0;
+}
+
+static int __maybe_unused rwdt_resume(struct device *dev)
+{
+   struct rwdt_priv *priv = dev_get_drvdata(dev);
+
+   if (watchdog_active(>wdev)) {
+   rwdt_start(>wdev);
+   rwdt_write(priv, priv->time_left, RWTCNT);


Upon given it more thought, I'm a bit worried about restoring the
original time left.
In my experiments, it may take a few seconds before userspace fully resumes.
If time_left was a small value, the system may reboot before userspace has
a chance to send its next ping.
This was with NFS root, so heavily impacted by the delays introduced by the
PHY link getting up again.

So just using rwdt_stop()/rwdt_start() may be the safest option.

Gr{oetje,eeting}s,

 Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 

Re: [PATCH v7 2/3] watchdog: renesas_wdt: Add R-Car Gen2 support

[Guenter Roeck]

On Thu, Mar 01, 2018 at 06:17:22PM +, Fabrizio Castro wrote:
> Due to commits:
> * "ARM: shmobile: Add watchdog support",
> * "ARM: shmobile: rcar-gen2: Add watchdog support", and
> * "soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2",
> we now have everything we needed for the watchdog to work on Gen2 and
> RZ/G1.
> 
> This commit adds "renesas,rcar-gen2-wdt" as compatible string for R-Car
> Gen2 and RZ/G1.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.cas...@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram 
> <ramesh.shanmugasunda...@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>
> Reviewed-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
> v6->v7:
> * no change
> 
>  drivers/watchdog/renesas_wdt.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 024d54e..9fc4c78 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -228,12 +228,8 @@ static int __maybe_unused rwdt_resume(struct device *dev)
>  
>  static SIMPLE_DEV_PM_OPS(rwdt_pm_ops, rwdt_suspend, rwdt_resume);
>  
> -/*
> - * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for 
> SMP
> - * to work there, one also needs a RESET (RST) driver which does not exist 
> yet
> - * due to HW issues. This needs to be solved before adding compatibles here.
> - */
>  static const struct of_device_id rwdt_ids[] = {
> + { .compatible = "renesas,rcar-gen2-wdt", },
>   { .compatible = "renesas,rcar-gen3-wdt", },
>   { /* sentinel */ }
>  };
> -- 
> 2.7.4
> 

Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Guenter Roeck]

On Thu, Mar 01, 2018 at 06:17:21PM +, Fabrizio Castro wrote:
> On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
> on R-Car Gen3 we power the IP down during suspend.
> 
> This commit adds suspend/resume support, so that the watchdog counting
> "pauses" during suspend on all of the SoCs compatible with this driver
> and on those we are now adding support for (R-Car Gen2 and RZ/G1).
> 
> Signed-off-by: Fabrizio Castro <fabrizio.cas...@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram 
> <ramesh.shanmugasunda...@bp.renesas.com>

Usually, on resume, we just restart the watchdog, with the expectation in mind
that there may be some delay in userspace before it gets to send the next ping.
Presumably that is not a concern here, so

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
> v6->v7:
> * backup and restore register RWTCNT instead of using rwdt_get_timeleft and
>   rwdt_set_timeleft
> 
>  drivers/watchdog/renesas_wdt.c | 26 ++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 831ef83..024d54e 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -49,6 +49,7 @@ struct rwdt_priv {
>   void __iomem *base;
>   struct watchdog_device wdev;
>   unsigned long clk_rate;
> + u16 time_left;
>   u8 cks;
>  };
>  
> @@ -203,6 +204,30 @@ static int rwdt_remove(struct platform_device *pdev)
>   return 0;
>  }
>  
> +static int __maybe_unused rwdt_suspend(struct device *dev)
> +{
> + struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> + if (watchdog_active(>wdev)) {
> + priv->time_left = readw(priv->base + RWTCNT);
> + rwdt_stop(>wdev);
> + }
> + return 0;
> +}
> +
> +static int __maybe_unused rwdt_resume(struct device *dev)
> +{
> + struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> + if (watchdog_active(>wdev)) {
> + rwdt_start(>wdev);
> + rwdt_write(priv, priv->time_left, RWTCNT);
> + }
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rwdt_pm_ops, rwdt_suspend, rwdt_resume);
> +
>  /*
>   * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for 
> SMP
>   * to work there, one also needs a RESET (RST) driver which does not exist 
> yet
> @@ -218,6 +243,7 @@ static struct platform_driver rwdt_driver = {
>   .driver = {
>   .name = "renesas_wdt",
>   .of_match_table = rwdt_ids,
> + .pm = _pm_ops,
>   },
>   .probe = rwdt_probe,
>   .remove = rwdt_remove,
> -- 
> 2.7.4
> 

Re: [PATCH] watchdog: renesas_wdt: adapt timer setup to recommended procedure

[Guenter Roeck]

On Tue, Feb 27, 2018 at 05:22:56PM +0100, Wolfram Sang wrote:
> 
> > the core gets it wrong. Just add a note to the probe function where cks is
> > initialized that RWTCSRA_TME must not be set, and that RWTCSRA_TME must be
> > cleared before changing the divider.
> 
> But we would still need to disable TME sperately if the caclulated cks
> is not the same value as the current register value (be it power-on
> default or something the firmware did set). Because we shall not disable
> TME and modify cks at the same time.
> 
AFAICS this only applies if the watchdog is already running at boot,
which is not currently supported to start with.

Guenter

Re: [PATCH] watchdog: renesas_wdt: adapt timer setup to recommended procedure

[Guenter Roeck]

On 02/27/2018 05:00 AM, Wolfram Sang wrote:

-   rwdt_write(priv, 0, RWTCSRB);
-   rwdt_write(priv, priv->cks, RWTCSRA);


Isn't this done implicitly with the above already ?
After all, priv->cks won't have RWTCSRA_TME set.


Yes. The request came from a group doing some (safety?) audit who didn't
like the implicit handling which might change if the code in probe()
might change somewhen. And the datasheet explicitly says to disable the
timer first before doing anything with the clock dividers. Given that, I
can agree to this change, too.



Defensive code is fine, bit that is a bit too defensive. We might as well
start checking value ranges in the drivers' set_timeout functions just in case
the core gets it wrong. Just add a note to the probe function where cks is
initialized that RWTCSRA_TME must not be set, and that RWTCSRA_TME must be
cleared before changing the divider.

Guenter

Re: [PATCH] watchdog: renesas_wdt: adapt timer setup to recommended procedure

[Guenter Roeck]

On Mon, Feb 26, 2018 at 10:49:05PM +0100, Wolfram Sang wrote:
> The datasheet says we should stop the timer before changing the clock
> divider. So, let's meet that recommendation.
> 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/watchdog/renesas_wdt.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 831ef83f6de15b..c4a17d72d02506 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -74,12 +74,17 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
>  static int rwdt_start(struct watchdog_device *wdev)
>  {
>   struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> + u8 val;
>  
>   pm_runtime_get_sync(wdev->parent);
>  
> - rwdt_write(priv, 0, RWTCSRB);
> - rwdt_write(priv, priv->cks, RWTCSRA);

Isn't this done implicitly with the above already ?
After all, priv->cks won't have RWTCSRA_TME set.

The only exception I can think of is if the watchdog is 
already running during boot, but that situation isn't handled
anyway.

Thanks,
Guenter

> + /* Stop the timer before we modify any register */
> + val = readb_relaxed(priv->base + RWTCSRA) & ~RWTCSRA_TME;
> + rwdt_write(priv, val, RWTCSRA);
> +
>   rwdt_init_timeout(wdev);
> + rwdt_write(priv, priv->cks, RWTCSRA);
> + rwdt_write(priv, 0, RWTCSRB);
>  
>   while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
>   cpu_relax();
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 12/26] watchdog: renesas_wdt: Add R-Car Gen2 support

[Guenter Roeck]

On Mon, Feb 12, 2018 at 05:44:21PM +, Fabrizio Castro wrote:
> Due to commits:
> * "ARM: shmobile: Add watchdog support",
> * "ARM: shmobile: rcar-gen2: Add watchdog support", and
> * "soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2",
> we now have everything we needed for the watchdog to work on Gen2 and
> RZ/G1.
> 
> This commit adds "renesas,rcar-gen2-wdt" as compatible string for R-Car
> Gen2 and RZ/G1, and since on those platforms the rwdt clock needs to be
> always ON, when suspending to RAM we need to explicitly disable the
> counting by clearing TME from RWTCSRA.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.cas...@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram 
> <ramesh.shanmugasunda...@bp.renesas.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
> v4->v5:
> * various improvements suggested by Wolfram
> 
>  drivers/watchdog/renesas_wdt.c | 27 ++-
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 831ef83..e972b7b 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -203,13 +203,29 @@ static int rwdt_remove(struct platform_device *pdev)
>   return 0;
>  }
>  
> -/*
> - * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for 
> SMP
> - * to work there, one also needs a RESET (RST) driver which does not exist 
> yet
> - * due to HW issues. This needs to be solved before adding compatibles here.
> - */
> +static int __maybe_unused rwdt_suspend(struct device *dev)
> +{
> + struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> + if (watchdog_active(>wdev))
> + rwdt_write(priv, priv->cks, RWTCSRA);
> + return 0;
> +}
> +
> +static int __maybe_unused rwdt_resume(struct device *dev)
> +{
> + struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> + if (watchdog_active(>wdev))
> + rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA);
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rwdt_pm_ops, rwdt_suspend, rwdt_resume);
> +
>  static const struct of_device_id rwdt_ids[] = {
>   { .compatible = "renesas,rcar-gen3-wdt", },
> + { .compatible = "renesas,rcar-gen2-wdt", },
>   { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, rwdt_ids);
> @@ -218,6 +234,7 @@ static struct platform_driver rwdt_driver = {
>   .driver = {
>   .name = "renesas_wdt",
>   .of_match_table = rwdt_ids,
> + .pm = _pm_ops,
>   },
>   .probe = rwdt_probe,
>   .remove = rwdt_remove,
> -- 
> 2.7.4
> 

Re: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler

[Guenter Roeck]

On 01/31/2018 10:24 AM, Fabrizio Castro wrote:

On iWave's boards iwg20d and iwg22d the only way to reboot the system is
by means of the watchdog.
This patch adds a restart handler to rwdt_ops, and also makes sure we
keep its priority to a medium level, in order to not override other more
effective handlers.

Signed-off-by: Fabrizio Castro <fabrizio.cas...@bp.renesas.com>
Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasunda...@bp.renesas.com>


Reviewed-by: Guenter Roeck <li...@roeck-us.net>


---
v3->v4:
* New patch spawn out from patch 12/16. The restart handler on Gen3 is
   controversial, hopefully this patch will help finalizing the discussion.

  drivers/watchdog/renesas_wdt.c | 20 
  1 file changed, 20 insertions(+)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 0a1a402..6d1c4b9 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -107,6 +107,24 @@ static unsigned int rwdt_get_timeleft(struct 
watchdog_device *wdev)
return DIV_BY_CLKS_PER_SEC(priv, 65536 - val);
  }
  
+static int rwdt_restart(struct watchdog_device *wdev, unsigned long action,

+   void *data)
+{
+   struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
+
+   pm_runtime_get_sync(wdev->parent);
+
+   rwdt_write(priv, 0x00, RWTCSRB);
+   rwdt_write(priv, 0x00, RWTCSRA);
+   rwdt_write(priv, 0x, RWTCNT);
+
+   while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
+   cpu_relax();
+
+   rwdt_write(priv, 0x80, RWTCSRA);
+   return 0;
+}
+
  static const struct watchdog_info rwdt_ident = {
.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
.identity = "Renesas WDT Watchdog",
@@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = {
.stop = rwdt_stop,
.ping = rwdt_init_timeout,
.get_timeleft = rwdt_get_timeleft,
+   .restart = rwdt_restart,
  };
  
  static int rwdt_probe(struct platform_device *pdev)

@@ -176,6 +195,7 @@ static int rwdt_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, priv);
watchdog_set_drvdata(>wdev, priv);
watchdog_set_nowayout(>wdev, nowayout);
+   watchdog_set_restart_priority(>wdev, 128);
  
  	/* This overrides the default timeout only if DT configuration was found */

ret = watchdog_init_timeout(>wdev, 0, >dev);

Re: [RFC v4 12/26] watchdog: renesas_wdt: Add R-Car Gen2 support

[Guenter Roeck]

On 01/31/2018 10:24 AM, Fabrizio Castro wrote:

Due to commits:
* "ARM: shmobile: Add watchdog support",
* "ARM: shmobile: rcar-gen2: Add watchdog support", and
* "soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2",
we now have everything we needed for the watchdog to work on Gen2 and
RZ/G1.

This commit adds "renesas,rcar-gen2-wdt" as compatible string for R-Car
Gen2 and RZ/G1, and since on those platforms the rwdt clock needs to be
always ON, when suspending to RAM we need to explicitly disable the
counting by clearing TME from RWTCSRA.

Signed-off-by: Fabrizio Castro 
Signed-off-by: Ramesh Shanmugasundaram 
---
v3->4:
* in this new version the changes to the driver have been splitted into
   two commits, this patch takes care of the basic Gen2 support, patch 13/26
   takes care of the restart handler.

  drivers/watchdog/renesas_wdt.c | 42 +-
  1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 831ef83..0a1a402 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -203,13 +203,42 @@ static int rwdt_remove(struct platform_device *pdev)
return 0;
  }
  
-/*

- * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for SMP
- * to work there, one also needs a RESET (RST) driver which does not exist yet
- * due to HW issues. This needs to be solved before adding compatibles here.
- */
+#ifdef CONFIG_PM
+static int rwdt_suspend(struct device *dev)
+{
+   struct platform_device *pdev;
+   struct rwdt_priv *priv;
+
+   pdev = to_platform_device(dev);
+   priv = platform_get_drvdata(pdev);
+   if (watchdog_active(>wdev)) {
+   rwdt_write(priv, priv->cks, RWTCSRA);
+   }


Unnecessary { }


+   return 0;
+}
+
+static int rwdt_resume(struct device *dev)
+{
+   struct platform_device *pdev;
+   struct rwdt_priv *priv;
+
+   pdev = to_platform_device(dev);
+   priv = platform_get_drvdata(pdev);
+   if (watchdog_active(>wdev)) {
+   rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA);
+   }


Same here. FWIW, checkpatch does complain about that.

Guenter

Re: [RFC v3 11/25] watchdog: renesas_wdt: Add R-Car Gen2 support

[Guenter Roeck]

On 01/31/2018 04:13 AM, Geert Uytterhoeven wrote:

Hi Fabrizio,

On Wed, Jan 31, 2018 at 11:47 AM, Fabrizio Castro
 wrote:

Subject: Re: [RFC v3 11/25] watchdog: renesas_wdt: Add R-Car Gen2 support
On Tue, Jan 30, 2018 at 08:22:44PM +, Fabrizio Castro wrote:

On R-Car Gen2 and RZ/G1 the rwdt clock needs to be always ON, therefore
when suspending to RAM we need to explicitly disable the counting by
clearing TME from RWTCSRA.
Also, on some systems RWDT is the only piece of HW that allows the SoC
to be restarted, therefore this patch implements the restart callback.

Signed-off-by: Fabrizio Castro 
Signed-off-by: Ramesh Shanmugasundaram 
---
Wolfram, was the restart callback implementation missing for a reason?
Is its implementation going to break any Gen3 platform?


The changes clearly are way more than claimed in the subject. Adding
restart handler and PM support may be prerequisites for Gen2, but the
changes apply to Gen3 as well. What happened to "one patch per logical
change" ?


The PM implementation should be ok for Gen3 too, without this patch series the 
kernel
would disable the rwdt clock when suspending to RAM, this would "pause" the 
counting.
This patch series declares the rwdt clock as critical for Gen2 and RZ/G1, which 
means we
need to explicitly disable the counting to keep the same behaviour in place.


Note that if the kernel crashes after rwdt_suspend(), the watchdog won't
restart the system. But that's an issue common to many watchdog driver, right?


If so, that would be buggy.


With respect to the restart callback implementation, that may well have 
consequences on
Gen3, hopefully Wolfram will give us a feedback on this.
In particular I would like to know if we should be installing the restart 
callback only when
we use "renesas,rcar-gen2-wdt" as compatible string, or it's ok to make it 
available for
Gen3 as well.


IIRC, the reason we don't have the restart callback on R-Car Gen3 is the
arm64 maintainers insisting on using PSCI, and vetoing other means,
to restart the system.



You could just give it lower priority than PSCI.


Which leaves us with a few boards where we have to use the watchdog, and
wait until the timeout triggers...



Which means the veto is counter-productive and thus meaningless.

Guenter


The only way to reboot iWave's boards iwg20d and iwg22d is to use the internal 
watchdog,
that's why the restart implementation was merged into this patch.
I can split this patch in two, one for PM support and one for restart support 
for the next
version, what do you think about this?


Splitting in individual logical changes sounds like a good idea to me.

Gr{oetje,eeting}s,

 Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 -- Linus Torvalds

Re: [RFC v3 11/25] watchdog: renesas_wdt: Add R-Car Gen2 support

[Guenter Roeck]

On Tue, Jan 30, 2018 at 08:22:44PM +, Fabrizio Castro wrote:
> On R-Car Gen2 and RZ/G1 the rwdt clock needs to be always ON, therefore
> when suspending to RAM we need to explicitly disable the counting by
> clearing TME from RWTCSRA.
> Also, on some systems RWDT is the only piece of HW that allows the SoC
> to be restarted, therefore this patch implements the restart callback.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> ---
> Wolfram, was the restart callback implementation missing for a reason?
> Is its implementation going to break any Gen3 platform?
> 

The changes clearly are way more than claimed in the subject. Adding
restart handler and PM support may be prerequisites for Gen2, but the
changes apply to Gen3 as well. What happened to "one patch per logical
change" ?

> v1->v3:
> * unified Gen2 and Gen3 drivers.
> 
>  drivers/watchdog/renesas_wdt.c | 61 
> ++
>  1 file changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 831ef83..eedb016 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -107,6 +107,24 @@ static unsigned int rwdt_get_timeleft(struct 
> watchdog_device *wdev)
>   return DIV_BY_CLKS_PER_SEC(priv, 65536 - val);
>  }
>  
> +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action,
> + void *data)
> +{
> + struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> + pm_runtime_get_sync(wdev->parent);
> +
> + rwdt_write(priv, 0x00, RWTCSRB);
> + rwdt_write(priv, 0x00, RWTCSRA);
> + rwdt_write(priv, 0x, RWTCNT);
> +
> + while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
> + cpu_relax();
> +
> + rwdt_write(priv, 0x80, RWTCSRA);
> + return 0;
> +}
> +
>  static const struct watchdog_info rwdt_ident = {
>   .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
>   .identity = "Renesas WDT Watchdog",
> @@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = {
>   .stop = rwdt_stop,
>   .ping = rwdt_init_timeout,
>   .get_timeleft = rwdt_get_timeleft,
> + .restart = rwdt_restart,
>  };
>  
>  static int rwdt_probe(struct platform_device *pdev)
> @@ -203,13 +222,42 @@ static int rwdt_remove(struct platform_device *pdev)
>   return 0;
>  }
>  
> -/*
> - * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for 
> SMP
> - * to work there, one also needs a RESET (RST) driver which does not exist 
> yet
> - * due to HW issues. This needs to be solved before adding compatibles here.
> - */

Have those issues been resolved ?

> +#ifdef CONFIG_PM
> +static int rwdt_suspend(struct device *dev)
> +{
> + struct platform_device *pdev;
> + struct rwdt_priv *priv;
> +
> + pdev = to_platform_device(dev);
> + priv = platform_get_drvdata(pdev);
> + if (watchdog_active(>wdev)) {
> + rwdt_write(priv, priv->cks, RWTCSRA);
> + }
> + return 0;
> +}
> +
> +static int rwdt_resume(struct device *dev)
> +{
> + struct platform_device *pdev;
> + struct rwdt_priv *priv;
> +
> + pdev = to_platform_device(dev);
> + priv = platform_get_drvdata(pdev);
> + if (watchdog_active(>wdev)) {
> + rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA);
> + }
> + return 0;
> +}
> +
> +static const struct dev_pm_ops rwdt_pm = {
> + .suspend = rwdt_suspend,
> + .resume = rwdt_resume,
> +};
> +#endif
> +
>  static const struct of_device_id rwdt_ids[] = {
>   { .compatible = "renesas,rcar-gen3-wdt", },
> + { .compatible = "renesas,rcar-gen2-wdt", },
>   { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, rwdt_ids);
> @@ -218,6 +266,9 @@ static struct platform_driver rwdt_driver = {
>   .driver = {
>   .name = "renesas_wdt",
>   .of_match_table = rwdt_ids,
> +#ifdef CONFIG_PM
> + .pm = _pm,
> +#endif
>   },
>   .probe = rwdt_probe,
>   .remove = rwdt_remove,
> -- 
> 2.7.4
> 

Re: [RFC 24/37] watchdog: renesas_wdt_gen2: Add Gen2 specific driver

[Guenter Roeck]

On Thu, Jan 25, 2018 at 06:02:58PM +, Fabrizio Castro wrote:
> R-Car Gen2 (and RZ/G1) platforms need some tweaking for SMP and watchdog
> to coexist nicely.
> This new driver is based on top of Wolfram Sang's driver
> (drivers/watchdog/renesas_wdt.c), and it contains the quirks necessary
> for R-Car Gen2 and RZ/G1 to work properly and in harmony with the rest of
> the system. In particular, the driver:
> * expects the device clock to be ON all the time,
> * "pauses" the watchdog operation during suspend, and
> * "reassures" the SMP bringup function about the availability of the
>   watchdog registers.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> ---
>  drivers/watchdog/Kconfig|  15 +-
>  drivers/watchdog/Makefile   |   1 +
>  drivers/watchdog/renesas_wdt_gen2.c | 270 
> 
>  drivers/watchdog/renesas_wdt_gen2.h |  22 +++
>  4 files changed, 306 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/watchdog/renesas_wdt_gen2.c
>  create mode 100644 drivers/watchdog/renesas_wdt_gen2.h
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index ca200d1..e580c72 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -725,12 +725,23 @@ config ATLAS7_WATCHDOG
> module will be called atlas7_wdt.
>  
>  config RENESAS_WDT
> - tristate "Renesas WDT Watchdog"
> + tristate "Renesas R-Car Gen3 WDT Watchdog"
>   depends on ARCH_RENESAS || COMPILE_TEST
>   select WATCHDOG_CORE
>   help
> This driver adds watchdog support for the integrated watchdogs in the
> -   Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).
> +   Renesas R-Car Gen3 devices.
> +
> +config RENESAS_WDT_GEN2
> + tristate "Renesas R-Car Gen2 and RZ/G1 WDT Watchdog"
> + depends on ARCH_RENESAS || COMPILE_TEST
> + select WATCHDOG_CORE
> + help
> +   This driver adds watchdog support for the integrated watchdogs in the
> +   Renesas R-Car Gen2 and RZ/G1 devices.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called renesas_wdt_gen2.
>  
>  config RENESAS_RZAWDT
>   tristate "Renesas RZ/A WDT Watchdog"
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 715a210..57ab810 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
>  obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
>  obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
>  obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
> +obj-$(CONFIG_RENESAS_WDT_GEN2) += renesas_wdt_gen2.o
>  obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
>  obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>  obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
> diff --git a/drivers/watchdog/renesas_wdt_gen2.c 
> b/drivers/watchdog/renesas_wdt_gen2.c
> new file mode 100644
> index 000..c841636
> --- /dev/null
> +++ b/drivers/watchdog/renesas_wdt_gen2.c
> @@ -0,0 +1,270 @@
> +/*
> + * Watchdog driver for Renesas WDT watchdog
> + *
> + * Copyright (C) 2015-17 Wolfram Sang, Sang Engineering 
> 
> + * Copyright (C) 2018Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published 
> by
> + * the Free Software Foundation.

Please use the SPDX identifier.

> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "renesas_wdt_gen2.h"
> +
> +#define RWTCNT   0
> +#define RWTCSRA  4
> +#define RWTCSRA_WOVF BIT(4)
> +#define RWTCSRA_WRFLGBIT(5)
> +#define RWTCSRA_TME  BIT(7)
> +#define RWTCSRB  8
> +
> +#define RWDT_DEFAULT_TIMEOUT 60U
> +
> +/*
> + * In probe, clk_rate is checked to be not more than 16 bit * biggest clock
> + * divider (12 bits). d is only a factor to fully utilize the WDT counter and
> + * will not exceed its 16 bits. Thus, no overflow, we stay below 32 bits.
> + */
> +#define MUL_BY_CLKS_PER_SEC(p, d) \
> + DIV_ROUND_UP((d) * (p)->clk_rate, clk_divs[(p)->cks])
> +
> +/* d is 16 bit, clk_divs 12 bit -> no 32 bit overflow */
> +#define DIV_BY_CLKS_PER_SEC(p, d) ((d) * clk_divs[(p)->cks] / (p)->clk_rate)
> +
> +static const unsigned int clk_divs[] = { 1, 4, 16, 32, 64, 128, 1024, 4096 };
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started 
> (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct rwdt_priv {
> + void __iomem *base;
> + struct watchdog_device wdev;
> + unsigned long clk_rate;
> + bool 

Re: [RFC 22/37] watchdog: renesas_wdt: Add restart support

[Guenter Roeck]

On Thu, Jan 25, 2018 at 06:02:56PM +, Fabrizio Castro wrote:
> This commit extends the driver to add restart support by implementing
> the restart callback to trigger the watchdog as quickly as possible.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> ---
>  drivers/watchdog/renesas_wdt.c | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 831ef83..120ddac 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -107,6 +107,24 @@ static unsigned int rwdt_get_timeleft(struct 
> watchdog_device *wdev)
>   return DIV_BY_CLKS_PER_SEC(priv, 65536 - val);
>  }
>  
> +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action,
> + void *data)
> +{
> + struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> + pm_runtime_get_sync(wdev->parent);
> +
> + rwdt_write(priv, 0x00, RWTCSRB);
> + rwdt_write(priv, 0x00, RWTCSRA);
> + rwdt_write(priv, 0x, RWTCNT);
> +
> + while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
> + cpu_relax();

Can this get stuck forever or should there be a timeout ?

Guenter

> +
> + rwdt_write(priv, 0x80, RWTCSRA);
> + return 0;
> +}
> +
>  static const struct watchdog_info rwdt_ident = {
>   .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
>   .identity = "Renesas WDT Watchdog",
> @@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = {
>   .stop = rwdt_stop,
>   .ping = rwdt_init_timeout,
>   .get_timeleft = rwdt_get_timeleft,
> + .restart = rwdt_restart,
>  };
>  
>  static int rwdt_probe(struct platform_device *pdev)
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dt-bindings: watchdog: renesas-wdt: Add support for the r8a77970 wdt

[Guenter Roeck]

On Mon, Oct 30, 2017 at 04:43:19PM +0100, Geert Uytterhoeven wrote:
> Document support for the Watchdog Timer (WDT) Controller in the Renesas
> R-Car V3M (r8a77970) SoC.  Restore sort order while at it.
> 
> No driver update is needed.
> 
> Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
> b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> index bf6d1ca58af7d198..74b2f03c151553f5 100644
> --- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> @@ -4,10 +4,11 @@ Required properties:
>  - compatible : Should be "renesas,-wdt", and
>  "renesas,rcar-gen3-wdt" or "renesas,rza-wdt" as fallback.
>  Examples with soctypes are:
> +  - "renesas,r7s72100-wdt" (RZ/A1)
>- "renesas,r8a7795-wdt" (R-Car H3)
>- "renesas,r8a7796-wdt" (R-Car M3-W)
> +  - "renesas,r8a77970-wdt" (R-Car V3M)
>- "renesas,r8a77995-wdt" (R-Car D3)
> -  - "renesas,r7s72100-wdt" (RZ/A1)
>  
>When compatible with the generic version, nodes must list the SoC-specific
>version corresponding to the platform first, followed by the generic
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: dt-bindings: watchdog: renesas-wdt: Add support for the r8a77995 wdt

[Guenter Roeck]

On Thu, Aug 17, 2017 at 01:14:25PM +0200, Geert Uytterhoeven wrote:
> Document support for the Watchdog Timer (WDT) Controller in the Renesas
> R-Car D3 (r8a77995) SoC.
> 
> No driver update is needed.
> 
> Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
> Acked-by: Simon Horman <horms+rene...@verge.net.au>
> Acked-by: Rob Herring <r...@kernel.org>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
> b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> index 9e306afbbd49e91b..bf6d1ca58af7d198 100644
> --- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
> @@ -6,6 +6,7 @@ Required properties:
>  Examples with soctypes are:
>- "renesas,r8a7795-wdt" (R-Car H3)
>- "renesas,r8a7796-wdt" (R-Car M3-W)
> +  - "renesas,r8a77995-wdt" (R-Car D3)
>- "renesas,r7s72100-wdt" (RZ/A1)
>  
>When compatible with the generic version, nodes must list the SoC-specific

Re: [RFC, 2/3] watchdog: renesas_wdt: make 'clk' a variable local to probe()

[Guenter Roeck]

On Wed, Jul 26, 2017 at 11:54:38PM +0200, Wolfram Sang wrote:
> It is not needed outside probe() anymore.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  drivers/watchdog/renesas_wdt.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index a03997b418ba9c..2927b5086e156e 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -48,7 +48,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once 
> started (default="
>  struct rwdt_priv {
>   void __iomem *base;
>   struct watchdog_device wdev;
> - struct clk *clk;
>   unsigned long clk_rate;
>   u8 cks;
>  };
> @@ -125,6 +124,7 @@ static int rwdt_probe(struct platform_device *pdev)
>  {
>   struct rwdt_priv *priv;
>   struct resource *res;
> + struct clk *clk;
>   unsigned long clks_per_sec;
>   int ret, i;
>  
> @@ -137,14 +137,14 @@ static int rwdt_probe(struct platform_device *pdev)
>   if (IS_ERR(priv->base))
>   return PTR_ERR(priv->base);
>  
> - priv->clk = devm_clk_get(>dev, NULL);
> - if (IS_ERR(priv->clk))
> - return PTR_ERR(priv->clk);
> + clk = devm_clk_get(>dev, NULL);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
>  
>   pm_runtime_enable(>dev);
>  
>   pm_runtime_get_sync(>dev);
> - priv->clk_rate = clk_get_rate(priv->clk);
> + priv->clk_rate = clk_get_rate(clk);
>   pm_runtime_put(>dev);
>  
>   if (!priv->clk_rate) {

Re: [RFC,3/3] watchdog: renesas_wdt: update copyright dates

[Guenter Roeck]

On Wed, Jul 26, 2017 at 11:54:39PM +0200, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  drivers/watchdog/renesas_wdt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 2927b5086e156e..831ef83f6de15b 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -1,8 +1,8 @@
>  /*
>   * Watchdog driver for Renesas WDT watchdog
>   *
> - * Copyright (C) 2015-16 Wolfram Sang, Sang Engineering 
> <w...@sang-engineering.com>
> - * Copyright (C) 2015-16 Renesas Electronics Corporation
> + * Copyright (C) 2015-17 Wolfram Sang, Sang Engineering 
> <w...@sang-engineering.com>
> + * Copyright (C) 2015-17 Renesas Electronics Corporation
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU General Public License version 2 as published 
> by

Re: [RFC, 1/3] watchdog: renesas_wdt: consistently use RuntimePM for clock management

[Guenter Roeck]

On Wed, Jul 26, 2017 at 11:54:37PM +0200, Wolfram Sang wrote:
> On Renesas R-Car archs, RuntimePM does all the clock handling. So, use
> it consistently to enable/disable the clocks. Also make sure that clocks
> are really enabled around clk_get_rate(). clk_summary looks proper now:
> 
>   clock   enable_cnt  prepare_cnt rate ...
> Before this commit:
> 
> At boot:  rwdt1   1   32768 0 0
> WDT running:  rwdt2   2   32768 0 0
> 
> After this commit:
> 
> At boot:  rwdt0   1   32768 0 0
> WDT running   rwdt1   1   32768 0 0
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  drivers/watchdog/renesas_wdt.c | 33 +++--
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index e3f204bb8802aa..a03997b418ba9c 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -76,7 +76,7 @@ static int rwdt_start(struct watchdog_device *wdev)
>  {
>   struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
>  
> - clk_prepare_enable(priv->clk);
> + pm_runtime_get_sync(wdev->parent);
>  
>   rwdt_write(priv, 0, RWTCSRB);
>   rwdt_write(priv, priv->cks, RWTCSRA);
> @@ -95,7 +95,7 @@ static int rwdt_stop(struct watchdog_device *wdev)
>   struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
>  
>   rwdt_write(priv, priv->cks, RWTCSRA);
> - clk_disable_unprepare(priv->clk);
> + pm_runtime_put(wdev->parent);
>  
>   return 0;
>  }
> @@ -141,9 +141,16 @@ static int rwdt_probe(struct platform_device *pdev)
>   if (IS_ERR(priv->clk))
>   return PTR_ERR(priv->clk);
>  
> + pm_runtime_enable(>dev);
> +
> + pm_runtime_get_sync(>dev);
>   priv->clk_rate = clk_get_rate(priv->clk);
> - if (!priv->clk_rate)
> - return -ENOENT;
> + pm_runtime_put(>dev);
> +
> + if (!priv->clk_rate) {
> + ret = -ENOENT;
> + goto out_pm_disable;
> + }
>  
>   for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
>   clks_per_sec = priv->clk_rate / clk_divs[i];
> @@ -155,12 +162,10 @@ static int rwdt_probe(struct platform_device *pdev)
>  
>   if (i < 0) {
>   dev_err(>dev, "Can't find suitable clock divider\n");
> - return -ERANGE;
> + ret = -ERANGE;
> + goto out_pm_disable;
>   }
>  
> - pm_runtime_enable(>dev);
> - pm_runtime_get_sync(>dev);
> -
>   priv->wdev.info = _ident,
>   priv->wdev.ops = _ops,
>   priv->wdev.parent = >dev;
> @@ -178,13 +183,14 @@ static int rwdt_probe(struct platform_device *pdev)
>   dev_warn(>dev, "Specified timeout value invalid, using 
> default\n");
>  
>   ret = watchdog_register_device(>wdev);
> - if (ret < 0) {
> - pm_runtime_put(>dev);
> - pm_runtime_disable(>dev);
> - return ret;
> - }
> + if (ret < 0)
> + goto out_pm_disable;
>  
>   return 0;
> +
> + out_pm_disable:
> + pm_runtime_disable(>dev);
> + return ret;
>  }
>  
>  static int rwdt_remove(struct platform_device *pdev)
> @@ -192,7 +198,6 @@ static int rwdt_remove(struct platform_device *pdev)
>   struct rwdt_priv *priv = platform_get_drvdata(pdev);
>  
>   watchdog_unregister_device(>wdev);
> - pm_runtime_put(>dev);
>   pm_runtime_disable(>dev);
>  
>   return 0;

Re: [RFC PATCH 0/3] watchdog: renesas_wdt: use standard clock handling

[Guenter Roeck]

On Mon, Aug 14, 2017 at 05:47:28PM +0200, Wolfram Sang wrote:
> On Mon, Aug 14, 2017 at 08:32:30AM -0700, Guenter Roeck wrote:
> > On Wed, Jul 26, 2017 at 11:54:36PM +0200, Wolfram Sang wrote:
> > > As mentioned in a response to a previous patch, clock handling for Renesas
> > > R-Car is done via RuntimePM. Patch 1 implements that consequently. Patch 2
> > > is a cleanup then possible and patch 3 is a trivial copyright update.
> > > 
> > > This is RFC because I'd like Geert's comments on these patches first which
> > > might take a few days...
> > > 
> > Was there any feedback from Geert ? I don't recall seeing any.
> 
> He just got back from holidays and is bravely working through his mail
> queue :)
> 

Good to hear that I am not the only one with that problem :-).

Guenter

Re: [RFC PATCH 0/3] watchdog: renesas_wdt: use standard clock handling

[Guenter Roeck]

On Wed, Jul 26, 2017 at 11:54:36PM +0200, Wolfram Sang wrote:
> As mentioned in a response to a previous patch, clock handling for Renesas
> R-Car is done via RuntimePM. Patch 1 implements that consequently. Patch 2
> is a cleanup then possible and patch 3 is a trivial copyright update.
> 
> This is RFC because I'd like Geert's comments on these patches first which
> might take a few days...
> 
Was there any feedback from Geert ? I don't recall seeing any.

Guenter

> Looking forward to other comments, too, of course :)
> 
> These patches are on top of the already reviewed 'better-precision' series for
> this watchdog. A branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
> renesas/watchdog-fixes
> 
> These patches have been tested on a Renesas Salvator-X / r8a7796 (R-Car M3-W)
> 
> Thanks and kind regards,
> 
>Wolfram
> 
> 
> Wolfram Sang (3):
>   watchdog: renesas_wdt: consistently use RuntimePM for clock management
>   watchdog: renesas_wdt: make 'clk' a variable local to probe()
>   watchdog: renesas_wdt: update copyright dates
> 
>  drivers/watchdog/renesas_wdt.c | 47 
> +++---
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] watchdog: asm9260_wdt: don't round closest with get_timeleft

[Guenter Roeck]

On 07/17/2017 10:12 AM, Wolfram Sang wrote:

We should never return more time left than there actually is. So, switch
to a plain divider instead of DIV_ROUND_CLOSEST.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>


Reviewed-by: Guenter Roeck <li...@roeck-us.net>


---

This is similar to a patch for the renesas-rwdt[1]. I think it makes sense to
remove this pattern from the whole subsystem, if we can agree on the behaviour
stated in the patch description. So, this is the other driver using
DIV_ROUND_CLOSEST.

Note: This driver and the lpc18xx_wdt.c extremly likely drive the same IP core.
They would ideally be merged.

[1] https://patchwork.kernel.org/patch/9845459/

  drivers/watchdog/asm9260_wdt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/asm9260_wdt.c b/drivers/watchdog/asm9260_wdt.c
index 53da001f0838ee..f40dd71c904bef 100644
--- a/drivers/watchdog/asm9260_wdt.c
+++ b/drivers/watchdog/asm9260_wdt.c
@@ -82,7 +82,7 @@ static unsigned int asm9260_wdt_gettimeleft(struct 
watchdog_device *wdd)
  
  	counter = ioread32(priv->iobase + HW_WDTV);
  
-	return DIV_ROUND_CLOSEST(counter, priv->wdt_freq);

+   return counter / priv->wdt_freq;
  }
  
  static int asm9260_wdt_updatetimeout(struct watchdog_device *wdd)

Re: [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow

[Guenter Roeck]

On 07/18/2017 12:21 AM, Wolfram Sang wrote:

-   unsigned long rate;
-   unsigned int clks_per_sec;
+   unsigned long rate, clks_per_sec;


If you make this change, you should also update rwdt_priv.clks_per_sec
(yes I know it's removed in a later patch in this series).


Right. I will change but also wait a bit for more comments.



I for my part don't have any. Feel free to add

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

to the series when you resend.

Thanks,
Guenter

Re: [PATCH v6 3/3] ARM: dts: r7s72100: Add watchdog timer

[Guenter Roeck]

On 03/04/2017 02:37 PM, Chris Brandt wrote:

Add watchdog timer support for RZ/A1.
For the RZ/A1, the only way to do a reset is to overflow the WDT, so this
is useful even if you don't need the watchdog functionality.

Signed-off-by: Chris Brandt <chris.bra...@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>


Acked-by: Guenter Roeck <li...@roeck-us.net>


---
v4:
* changed from timer@ to watchdog@
v3:
* added Reviewed-by
v2:
* changed "renesas,r7s72100-reset" to "renesas,r7s72100-wdt"
* changed "renesas,wdt-reset" to "renesas,rza-wdt"
* added interupt property (even though it is not used)
* added clocks property
---
 arch/arm/boot/dts/r7s72100.dtsi | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index ed62e19..22f96b7 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -382,6 +382,13 @@
cache-level = <2>;
};

+   wdt: watchdog@fcfe {
+   compatible = "renesas,r7s72100-wdt", "renesas,rza-wdt";
+   reg = <0xfcfe 0x6>;
+   interrupts = ;
+   clocks = <_clk>;
+   };
+
i2c0: i2c@fcfee000 {
#address-cells = <1>;
#size-cells = <0>;

Re: [PATCH v6 2/3] watchdog: renesas-wdt: add support for rza

[Guenter Roeck]

On 03/04/2017 02:37 PM, Chris Brandt wrote:

Describe the WDT hardware in the RZ/A series.

Signed-off-by: Chris Brandt <chris.bra...@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>
Acked-by: Rob Herring <r...@kernel.org>


Reviewed-by: Guenter Roeck <li...@roeck-us.net>


---
v3:
* Add Acked-by, Reviewed-by.
v2:
* added to renesas-wdt.txt instead of creating a new file
* changed commit title
* added "renesas,rza-wdt" as a fallback
* added interrupts property
---
 Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
index da24e31..9e306af 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
@@ -2,10 +2,11 @@ Renesas Watchdog Timer (WDT) Controller

 Required properties:
 - compatible : Should be "renesas,-wdt", and
-  "renesas,rcar-gen3-wdt" as fallback.
+  "renesas,rcar-gen3-wdt" or "renesas,rza-wdt" as fallback.
   Examples with soctypes are:
 - "renesas,r8a7795-wdt" (R-Car H3)
 - "renesas,r8a7796-wdt" (R-Car M3-W)
+- "renesas,r7s72100-wdt" (RZ/A1)

   When compatible with the generic version, nodes must list the SoC-specific
   version corresponding to the platform first, followed by the generic
@@ -17,6 +18,7 @@ Required properties:
 Optional properties:
 - timeout-sec : Contains the watchdog timeout in seconds
 - power-domains : the power domain the WDT belongs to
+- interrupts: Some WDTs have an interrupt when used in interval timer mode

 Examples:

Re: [PATCH v6 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On 03/04/2017 02:37 PM, Chris Brandt wrote:

Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset
handler is also included since a WDT overflow is the only method for
restarting an RZ/A SoC.

Signed-off-by: Chris Brandt <chris.bra...@renesas.com>


Reviewed-by: Guenter Roeck <li...@roeck-us.net>


---
v3:
* added #include 
* alphabetized #include list
* removed call to platform_set_drvdata
v2:
* removed extra lines from file header comments
* changed (1<<#) to BIT(#)
* changed #define WTSCR_CKS(i) i to (i)
* changed format to #define SOMETHINGvalue (and align values)
* now check if clock rate < 16384
* added space before and after '/' for "(1000 * U8_MAX) / rate"
* changed dev_info to dev_dbg for printing max_hw_heartbeat_ms
* added watchdog_init_timeout() for user to set timeout in DT
* switched to using devm_watchdog_register_device()
* added error message if register fails
* simplified rza_wdt_probe() return
* removed function rza_wdt_remove()
---
 drivers/watchdog/Kconfig   |   8 ++
 drivers/watchdog/Makefile  |   1 +
 drivers/watchdog/rza_wdt.c | 199 +
 3 files changed, 208 insertions(+)
 create mode 100644 drivers/watchdog/rza_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index acb00b5..123c516 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -701,6 +701,14 @@ config RENESAS_WDT
  This driver adds watchdog support for the integrated watchdogs in the
  Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).

+config RENESAS_RZAWDT
+   tristate "Renesas RZ/A WDT Watchdog"
+   depends on ARCH_RENESAS || COMPILE_TEST
+   select WATCHDOG_CORE
+   help
+ This driver adds watchdog support for the integrated watchdogs in the
+ Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
+
 config ASPEED_WATCHDOG
tristate "Aspeed 2400 watchdog support"
depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c3d35e..84b897c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
 obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
 obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
 obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
+obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
 obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o

 # AVR32 Architecture
diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
new file mode 100644
index 000..e618218
--- /dev/null
+++ b/drivers/watchdog/rza_wdt.c
@@ -0,0 +1,199 @@
+/*
+ * Renesas RZ/A Series WDT Driver
+ *
+ * Copyright (C) 2017 Renesas Electronics America, Inc.
+ * Copyright (C) 2017 Chris Brandt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DEFAULT_TIMEOUT30
+
+/* Watchdog Timer Registers */
+#define WTCSR  0
+#define WTCSR_MAGIC0xA500
+#define WTSCR_WT   BIT(6)
+#define WTSCR_TME  BIT(5)
+#define WTSCR_CKS(i)   (i)
+
+#define WTCNT  2
+#define WTCNT_MAGIC0x5A00
+
+#define WRCSR  4
+#define WRCSR_MAGIC0x5A00
+#define WRCSR_RSTE BIT(6)
+#define WRCSR_CLEAR_WOVF   0xA500  /* special value */
+
+struct rza_wdt {
+   struct watchdog_device wdev;
+   void __iomem *base;
+   struct clk *clk;
+};
+
+static int rza_wdt_start(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   /* Stop timer */
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   /* Must dummy read WRCSR:WOVF at least once before clearing */
+   readb(priv->base + WRCSR);
+   writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
+
+   /*
+* Start timer with slowest clock source and reset option enabled.
+*/
+   writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
+   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+   writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
+  priv->base + WTCSR);
+
+   return 0;
+}
+
+static int rza_wdt_stop(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   return 0;
+}
+
+static int rza_wdt_ping(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+
+   return 0;
+}
+
+static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action,
+   void *data)

Re: [PATCH v5 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On 03/03/2017 09:31 AM, Chris Brandt wrote:

Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset
handler is also included since a WDT overflow is the only method for
restarting an RZ/A SoC.

Signed-off-by: Chris Brandt 
---
v2:
* removed extra lines from file header comments
* changed (1<<#) to BIT(#)
* changed #define WTSCR_CKS(i) i to (i)
* changed format to #define SOMETHINGvalue (and align values)
* now check if clock rate < 16384
* added space before and after '/' for "(1000 * U8_MAX) / rate"
* changed dev_info to dev_dbg for printing max_hw_heartbeat_ms
* added watchdog_init_timeout() for user to set timeout in DT
* switched to using devm_watchdog_register_device()
* added error message if register fails
* simplified rza_wdt_probe() return
* removed function rza_wdt_remove()
---
 drivers/watchdog/Kconfig   |   8 ++
 drivers/watchdog/Makefile  |   1 +
 drivers/watchdog/rza_wdt.c | 199 +
 3 files changed, 208 insertions(+)
 create mode 100644 drivers/watchdog/rza_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index acb00b5..123c516 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -701,6 +701,14 @@ config RENESAS_WDT
  This driver adds watchdog support for the integrated watchdogs in the
  Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).

+config RENESAS_RZAWDT
+   tristate "Renesas RZ/A WDT Watchdog"
+   depends on ARCH_RENESAS || COMPILE_TEST
+   select WATCHDOG_CORE
+   help
+ This driver adds watchdog support for the integrated watchdogs in the
+ Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
+
 config ASPEED_WATCHDOG
tristate "Aspeed 2400 watchdog support"
depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c3d35e..84b897c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
 obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
 obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
 obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
+obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
 obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o

 # AVR32 Architecture
diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
new file mode 100644
index 000..76a9c1f
--- /dev/null
+++ b/drivers/watchdog/rza_wdt.c
@@ -0,0 +1,199 @@
+/*
+ * Renesas RZ/A Series WDT Driver
+ *
+ * Copyright (C) 2017 Renesas Electronics America, Inc.
+ * Copyright (C) 2017 Chris Brandt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+


Also needs to include . While at it, please order include
files alphabetically.


+#define DEFAULT_TIMEOUT30
+
+/* Watchdog Timer Registers */
+#define WTCSR  0
+#define WTCSR_MAGIC0xA500
+#define WTSCR_WT   BIT(6)
+#define WTSCR_TME  BIT(5)
+#define WTSCR_CKS(i)   (i)
+
+#define WTCNT  2
+#define WTCNT_MAGIC0x5A00
+
+#define WRCSR  4
+#define WRCSR_MAGIC0x5A00
+#define WRCSR_RSTE BIT(6)
+#define WRCSR_CLEAR_WOVF   0xA500  /* special value */
+
+struct rza_wdt {
+   struct watchdog_device wdev;
+   void __iomem *base;
+   struct clk *clk;
+};
+
+static int rza_wdt_start(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   /* Stop timer */
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   /* Must dummy read WRCSR:WOVF at least once before clearing */
+   readb(priv->base + WRCSR);
+   writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
+
+   /*
+* Start timer with slowest clock source and reset option enabled.
+*/
+   writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
+   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+   writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
+  priv->base + WTCSR);
+
+   return 0;
+}
+
+static int rza_wdt_stop(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   return 0;
+}
+
+static int rza_wdt_ping(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+
+   return 0;
+}
+
+static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action,
+   void *data)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   /* Stop timer */
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   /* 

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On Thu, Mar 02, 2017 at 06:22:17PM +, Chris Brandt wrote:
> On Thursday, March 02, 2017, Guenter Roeck wrote:
> > > > > > The rate check should probably be here to avoid situations where
> > > > > > rate < 16384.
> > > > >
> > > > > Do I need that if it's technically not possible to have a 'rate'
> > > > > less
> > > > than 25MHz?
> > > > >
> > > > > These watchdogs HW are always feed directly from the peripheral
> > > > > clock and there is no such thing as a 16kHz peripheral block an
> > > > > any Renesas
> > > > SoC.
> > > > >
> > > > Following that line of argument, can clk_get_rate() ever return 0 ?
> > >
> > > In the DT binding, it says that a clock source is required to be present.
> > >
> > > If the user leaves out the "clocks =", then devm_clk_get will fail.
> > >
> > > If the user puts in some crazy value for "clocks = ", then maybe you
> > > could get
> > > 0 (assuming there is a valid clock node they made by themselves
> > > somewhere that runs at 0Hz).
> > > But in that extreme case, I think they deserve to have it crash and
> > > burn because who knows what they are doing.
> > >
> > 
> > But then there could also be a clock source with a rate of less than 16
> > kHz, as wrong as it may be ?
> > 
> > Anyway, I disagree about the crash and burn. It isn't as if this would be
> > really fatal except for the watchdog driver. Bad data in devicetree should
> > not result in a system crash.
> 
> OK. I will put the check in. Something like:
> 
>   rate = clk_get_rate(priv->clk); 
>   if (rate < 16384) {
>   dev_err(>dev, "invalid clock specified\n");
>   return -ENOENT;
>   }
> 
> 
Sounds good. Maybe display the wrong rate as well ?
Not a strong opinion though.

Thanks,
Guenter

> > > > That is the point of devm_ functions. It also means that you won't
> > > > need a remove function if you also use devm_watchdog_register_device().
> > >
> > > OK.
> > > I see that only 1 driver is using devm_watchdog_register_device
> > > (wdat_wdt.c), so maybe that is a new method.
> > >
> > Yes, it is quite new. Still, you are a bit behind. I count 19 users in the
> > mainline kernel.
> 
> OK, I see now.
> 
> 
> Thank you,
> Chris

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On Thu, Mar 02, 2017 at 03:38:07PM +, Chris Brandt wrote:
> Hello Guenter,
> 
> Thank you for your review!
> 
> 
> On Thursday, March 02, 2017, Guenter Roeck wrote:
> > > +/*
> > > + * Renesas RZ/A Series WDT Driver
> > > + *
> > > + * Copyright (C) 2017 Renesas Electronics America, Inc.
> > > + * Copyright (C) 2017 Chris Brandt
> > > + *
> > > + * This file is subject to the terms and conditions of the GNU
> > > +General Public
> > > + * License.  See the file "COPYING" in the main directory of this
> > > +archive
> > > + * for more details.
> > > + *
> > > + *
> > 
> > The above two lines are unnecessary.
> 
> OK.
> 
> #I'll assume you mean take out just the last sentence (2 lines), not both
> sentences (all 3 lines).
> 
The two empty lines.

>  
> > > +/* Watchdog Timer Registers */
> > > +#define WTCSR 0
> > > +#define WTCSR_MAGIC 0xA500
> > > +#define WTSCR_WT (1<<6)
> > > +#define WTSCR_TME (1<<5)
> > 
> > BIT()
> 
> OK.
> 
> > > +#define WTSCR_CKS(i) i
> > 
> > (i)
> 
> OK.
> 
> 
> > > +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */
> > 
> > Please use
> > #define SOMETHINGvalue
> > throughout and make sure value is aligned.
> 
> OK.
> 
> > > + rate = clk_get_rate(priv->clk);
> > > + if (!rate)
> > > + return -ENOENT;
> > > +
> > > + /* Assume slowest clock rate possible (CKS=7) */
> > > + rate /= 16384;
> > > +
> > 
> > The rate check should probably be here to avoid situations where rate <
> > 16384.
> 
> Do I need that if it's technically not possible to have a 'rate' less than 
> 25MHz?
> 
> These watchdogs HW are always feed directly from the peripheral clock and 
> there
> is no such thing as a 16kHz peripheral block an any Renesas SoC.
> 
Following that line of argument, can clk_get_rate() ever return 0 ?

> 
> > > + priv->wdev.info = _wdt_ident,
> > > + priv->wdev.ops = _wdt_ops,
> > > + priv->wdev.parent = >dev;
> > > +
> > > + /*
> > > +  * Since the max possible timeout of our 8-bit count register is
> > less
> > > +  * than a second, we must use max_hw_heartbeat_ms.
> > > +  */
> > > + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
> > 
> > space before and after /
> 
> OK.
> #Funny because checkpatch.pl said it didn't like a space on one side but
>  not the other, so I choose no spaces and it was happy. I'm way below 80
>  characters for that line so it doesn't matter to me.
> 

That would be a bug in checkpatch. coding style, chapter 3.1, still applies.
Or at least I hope so.

> 
> > > + dev_info(>dev, "max hw timeout of %dms\n",
> > > +  priv->wdev.max_hw_heartbeat_ms);
> > 
> > dev_dbg ?
> 
> OK.
> #I guess we don't need to see that info on every boot.
> 
>  
> > > +
> > > + priv->wdev.min_timeout = 1;
> > > + priv->wdev.timeout = DEFAULT_TIMEOUT;
> > > +
> > 
> > Add watchdog_init_timeout(>wdev, 0, >dev); to optionally get
> > the timeout from dt ?
> 
> OK. Good idea.
> 
> > > + platform_set_drvdata(pdev, priv);
> > > + watchdog_set_drvdata(>wdev, priv);
> > > +
> > > + ret = watchdog_register_device(>wdev);
> > 
> > devm_watchdog_register_device()
> 
> OK.
> 
>  
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return 0;

Also just
return ret;

> > > +}
> > > +
> > > +static int rza_wdt_remove(struct platform_device *pdev) {
> > > + struct rza_wdt *priv = platform_get_drvdata(pdev);
> > > +
> > > + watchdog_unregister_device(>wdev);
> > > + iounmap(priv->base);
> > 
> > iounmap is unnecessary (and wrong).
> 
> Anything mapped with devm_ioremap_resource() automatically gets unmapped
> when the drive gets unloaded?

That is the point of devm_ functions. It also means that you won't need
a remove function if you also use devm_watchdog_register_device().

Guenter

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On Thu, Mar 02, 2017 at 05:31:18PM +, Chris Brandt wrote:
> On Thursday, March 02, 2017, Guenter Roeck worte:
> > > > The above two lines are unnecessary.
> > >
> > > OK.
> > >
> > > #I'll assume you mean take out just the last sentence (2 lines), not
> > > both sentences (all 3 lines).
> > >
> > The two empty lines.
> 
> Ooops! That makes more sense.
> 
> 
> > > > > + rate = clk_get_rate(priv->clk);
> > > > > + if (!rate)
> > > > > + return -ENOENT;
> > > > > +
> > > > > + /* Assume slowest clock rate possible (CKS=7) */
> > > > > + rate /= 16384;
> > > > > +
> > > >
> > > > The rate check should probably be here to avoid situations where
> > > > rate < 16384.
> > >
> > > Do I need that if it's technically not possible to have a 'rate' less
> > than 25MHz?
> > >
> > > These watchdogs HW are always feed directly from the peripheral clock
> > > and there is no such thing as a 16kHz peripheral block an any Renesas
> > SoC.
> > >
> > Following that line of argument, can clk_get_rate() ever return 0 ?
> 
> In the DT binding, it says that a clock source is required to be present.
> 
> If the user leaves out the "clocks =", then devm_clk_get will fail.
> 
> If the user puts in some crazy value for "clocks = ", then maybe you could get
> 0 (assuming there is a valid clock node they made by themselves somewhere that
> runs at 0Hz).
> But in that extreme case, I think they deserve to have it crash and burn 
> because
> who knows what they are doing.
> 

But then there could also be a clock source with a rate of less than 16 kHz, as
wrong as it may be ?

Anyway, I disagree about the crash and burn. It isn't as if this would be really
fatal except for the watchdog driver. Bad data in devicetree should not result
in a system crash.

> 
> > > > > + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
> > > >
> > > > space before and after /
> > >
> > > OK.
> > > #Funny because checkpatch.pl said it didn't like a space on one side
> > > but  not the other, so I choose no spaces and it was happy. I'm way
> > > below 80  characters for that line so it doesn't matter to me.
> > >
> > 
> > That would be a bug in checkpatch. coding style, chapter 3.1, still
> > applies.
> > Or at least I hope so.
> 
> OK. Thank you for the clarification.
> 
> 
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > > + return 0;
> > 
> > Also just
> > return ret;
> 
> OK.
> 
> 
> > > > > +static int rza_wdt_remove(struct platform_device *pdev) {
> > > > > + struct rza_wdt *priv = platform_get_drvdata(pdev);
> > > > > +
> > > > > + watchdog_unregister_device(>wdev);
> > > > > + iounmap(priv->base);
> > > >
> > > > iounmap is unnecessary (and wrong).
> > >
> > > Anything mapped with devm_ioremap_resource() automatically gets
> > > unmapped when the drive gets unloaded?
> > 
> > That is the point of devm_ functions. It also means that you won't need a
> > remove function if you also use devm_watchdog_register_device().
> 
> OK.
> I see that only 1 driver is using devm_watchdog_register_device (wdat_wdt.c), 
> so
> maybe that is a new method.
> 
Yes, it is quite new. Still, you are a bit behind. I count 19 users
in the mainline kernel.

Guenter

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On 03/02/2017 05:57 AM, Chris Brandt wrote:

Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset
handler is also included since a WDT overflow is the only method for
restarting an RZ/A SoC.

Signed-off-by: Chris Brandt 
---
 drivers/watchdog/Kconfig   |   8 ++
 drivers/watchdog/Makefile  |   1 +
 drivers/watchdog/rza_wdt.c | 208 +
 3 files changed, 217 insertions(+)
 create mode 100644 drivers/watchdog/rza_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index acb00b5..123c516 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -701,6 +701,14 @@ config RENESAS_WDT
  This driver adds watchdog support for the integrated watchdogs in the
  Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).

+config RENESAS_RZAWDT
+   tristate "Renesas RZ/A WDT Watchdog"
+   depends on ARCH_RENESAS || COMPILE_TEST
+   select WATCHDOG_CORE
+   help
+ This driver adds watchdog support for the integrated watchdogs in the
+ Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
+
 config ASPEED_WATCHDOG
tristate "Aspeed 2400 watchdog support"
depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c3d35e..84b897c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
 obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
 obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
 obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
+obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
 obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o

 # AVR32 Architecture
diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
new file mode 100644
index 000..17442c5
--- /dev/null
+++ b/drivers/watchdog/rza_wdt.c
@@ -0,0 +1,208 @@
+/*
+ * Renesas RZ/A Series WDT Driver
+ *
+ * Copyright (C) 2017 Renesas Electronics America, Inc.
+ * Copyright (C) 2017 Chris Brandt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ *


The above two lines are unnecessary.


+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DEFAULT_TIMEOUT 30
+
+/* Watchdog Timer Registers */
+#define WTCSR 0
+#define WTCSR_MAGIC 0xA500
+#define WTSCR_WT (1<<6)
+#define WTSCR_TME (1<<5)


BIT()


+#define WTSCR_CKS(i) i


(i)


+
+#define WTCNT 2
+#define WTCNT_MAGIC 0x5A00
+
+#define WRCSR 4
+#define WRCSR_MAGIC 0x5A00
+#define WRCSR_RSTE (1<<6)


BIT()


+#define WRCSR_CLEAR_WOVF 0xA500 /* special value */


Please use
#define SOMETHINGvalue
throughout and make sure value is aligned.


+
+struct rza_wdt {
+   struct watchdog_device wdev;
+   void __iomem *base;
+   struct clk *clk;
+};
+
+static int rza_wdt_start(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   /* Stop timer */
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   /* Must dummy read WRCSR:WOVF at least once before clearing */
+   readb(priv->base + WRCSR);
+   writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
+
+   /*
+* Start timer with slowest clock source and reset option enabled.
+*/
+   writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
+   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+   writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
+  priv->base + WTCSR);
+
+   return 0;
+}
+
+static int rza_wdt_stop(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   return 0;
+}
+
+static int rza_wdt_ping(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+
+   return 0;
+}
+
+static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action,
+   void *data)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   /* Stop timer */
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   /* Must dummy read WRCSR:WOVF at least once before clearing */
+   readb(priv->base + WRCSR);
+   writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
+
+   /*
+* Start timer with fastest clock source and only 1 clock left before
+* overflow with reset option enabled.
+*/
+   writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
+   writew(WTCNT_MAGIC | 255, priv->base + WTCNT);
+   writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, priv->base + WTCSR);
+
+   /*
+* Actually make sure the above sequence hits hardware before sleeping.
+*/
+   wmb();
+
+   /* Wait for WDT overflow (reset) */

Re: [PATCH v2 1/3] power: reset: Add Renesas reset driver

[Guenter Roeck]

On Wed, Feb 22, 2017 at 01:35:12PM +, Chris Brandt wrote:
> Hello Geert and Guenter,
> 
> On Thursday, February 16, 2017, Guenter Roeck wrote:
> > FWIW, the watchdog subsystem should support that easily, even with 125 ms
> > hardware timeout. We added that capability for that very purpose. That
> > would only fail if the system is stuck with interrupts disabled for more
> > than 125 ms, which seems unlikely. I think the gpio watchdog on some
> > systems has a similar low hardware timeout.
> 
> 
> While I'm going to try that for the RZ/A1, I have a question for you guys
> (or anyone else that has an opinion about end applications)
> 
> 
> If I were going to make a request to the chip designers to make the timeout 
> longer
> for the next RZ/A chip, what would be a good max timeout for common Linux 
> applications?
> 
> Looking through the drivers in the watchdog directly, I see default timeouts 
> of 20,
> 30, 60, and 120 seconds.
> 
30 and 60 are pretty common for default timeouts, though I personally think
they are a bit long. If you want direct HW support, a maximum of 120 seconds
should be sufficient though not really necessary anymore since the core
supports virtual timeouts. A maximum of at least 30 seconds would be needed
if the watchdog is supposed to run at boot time (ie if it is enabled by
ROMMON/BIOS and kept running by the kernel).

Hope this helps,
Guenter

Re: [PATCH v2 1/3] power: reset: Add Renesas reset driver

[Guenter Roeck]

On 02/17/2017 12:09 AM, Geert Uytterhoeven wrote:

Hi Günter,

On Fri, Feb 17, 2017 at 5:45 AM, Guenter Roeck <li...@roeck-us.net> wrote:

On 02/16/2017 06:00 PM, Chris Brandt wrote:

On Thursday, February 16, 2017, Guenter Roeck wrote:
If this WDT had a timeout longer than 125ms, I would make a real watchdog
driver
out of it. But at the moment, it just serves as the only way to reset the
chip.
Historically, this was the only way to reset a SH4 device...and we just
lived
with that fact. When Renesas moved from SH4 to ARM, a lot of the design
teams
just kept the same philosophy (and copied the HW blocks they knew already
worked)


FWIW, the watchdog subsystem should support that easily, even with 125 ms
hardware
timeout. We added that capability for that very purpose. That would only
fail if
the system is stuck with interrupts disabled for more than 125 ms, which
seems
unlikely. I think the gpio watchdog on some systems has a similar low
hardware
timeout.


I also thought 125ms was a bit short, but I'm happy to be proven wrong!
Let's make a real watchdog driver instead ;-)


v2: * changed DT bindings from 'wdt-reset' to 'rza-wdt' * changed
hard coded register values to defines * added msleep to while(1)
loop


Sure you can sleep here ?


As per Geert's review:

On Wednesday, February 15, 2017, Geert Uytterhoeven wrote:


+ +   /* Wait for WDT overflow */ +   while (1)


+   ;


This burns CPU, and thus power, albeit for a very short time.
Perhaps add an msleep() to the loop body?


Note that you only have 7.7us before the restart occurs, so probably
not much sleeping will be going on.


Let me rephrase my question. Is it guaranteed that you _can_ sleep in
this
function, or in other words that it is guaranteed that interrupts are
enabled ?


Hmm, I'm not sure if will actually 'sleep' or not. Regardless, interrupts
or not,
in 7.7us, the internal reset circuit is going to get triggered and the
whole system
is going to reboot not matter what is going on.



That isn't the point, really. You might get a WARN blob if msleep() is
called
with interrupts disabled, but of course you won't really see that because it
can
not be displayed in 7.7 us.


If it's not 100% guaranteed that we cannot sleep, we should not use msleep(),
and stick to busy waiting.
On ARM, we could also use wfi().


I know Geert's suggestion was in reference to saving power...but in
reality it's
probably negligible when we are talking about 7.7us. The reboot is going
to
automatically shut off all the peripherals clocks as well.


Maybe udelay(10); would have been more acceptable (and appropriate).


Inside the while (1) loop? That's the same as a plain "while (1) ;" ;-)
Or just udelay(10) and return, with the latter never happening if everything
goes well? Then the next restart handler will be tried, if available.



That is what I meant. Or use udelay(20) to be on the safe side.

Guenter


Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] power: reset: Add Renesas reset driver

[Guenter Roeck]

On 02/16/2017 06:00 PM, Chris Brandt wrote:

On Thursday, February 16, 2017, Guenter Roeck wrote:

Hmm, ok. Guess I don't have to understand that you can not use the
watchdog driver because of the above, but implementing exactly the same
functionality in a separate driver is ok.

[ I am sure I am missing something here, so just ignore what I am saying ]


Honestly, I don't have a handle on all the latest 'policies and procedures'
for all the various subsystems. All I know is that I want to figure out how
to execute my 5 lines of code to reset the system when the user types "reboot"
on the command line.

If this WDT had a timeout longer than 125ms, I would make a real watchdog driver
out of it. But at the moment, it just serves as the only way to reset the chip.
Historically, this was the only way to reset a SH4 device...and we just lived
with that fact. When Renesas moved from SH4 to ARM, a lot of the design teams
just kept the same philosophy (and copied the HW blocks they knew already 
worked)



FWIW, the watchdog subsystem should support that easily, even with 125 ms 
hardware
timeout. We added that capability for that very purpose. That would only fail if
the system is stuck with interrupts disabled for more than 125 ms, which seems
unlikely. I think the gpio watchdog on some systems has a similar low hardware
timeout.




v2: * changed DT bindings from 'wdt-reset' to 'rza-wdt' * changed
hard coded register values to defines * added msleep to while(1)
loop


Sure you can sleep here ?


As per Geert's review:

On Wednesday, February 15, 2017, Geert Uytterhoeven wrote:

+ +   /* Wait for WDT overflow */ +   while (1)

+   ;


This burns CPU, and thus power, albeit for a very short time.
Perhaps add an msleep() to the loop body?


Note that you only have 7.7us before the restart occurs, so probably
not much sleeping will be going on.


Let me rephrase my question. Is it guaranteed that you _can_ sleep in this
function, or in other words that it is guaranteed that interrupts are
enabled ?


Hmm, I'm not sure if will actually 'sleep' or not. Regardless, interrupts or 
not,
in 7.7us, the internal reset circuit is going to get triggered and the whole 
system
is going to reboot not matter what is going on.



That isn't the point, really. You might get a WARN blob if msleep() is called
with interrupts disabled, but of course you won't really see that because it can
not be displayed in 7.7 us.


I know Geert's suggestion was in reference to saving power...but in reality it's
probably negligible when we are talking about 7.7us. The reboot is going to
automatically shut off all the peripherals clocks as well.



Maybe udelay(10); would have been more acceptable (and appropriate).

Guenter

Re: [PATCH v2 1/3] power: reset: Add Renesas reset driver

[Guenter Roeck]

On Thu, Feb 16, 2017 at 06:40:05PM +, Chris Brandt wrote:
> On Thursday, February 16, 2017, Guenter Roeck wrote:
> > On Thu, Feb 16, 2017 at 12:23:18PM -0500, Chris Brandt wrote:
> > > Some Renesas SoCs do not have a reset register and the only way to do a SW
> > > controlled reset is to use the watchdog timer. Additionally, since all the
> > > WDT timeout options are so quick, a system reset is about the only thing
> > > it's good for.
> > >
> > > Signed-off-by: Chris Brandt <chris.bra...@renesas.com> Reviewed-by: Geert
> > > Uytterhoeven <geert+rene...@glider.be>
> > 
> > Wondering - why not use the watchdog driver and the infrastructure provided
> > by the watchdog core (ie the restart callback) instead ?
> 
> That was Geert's first suggestion, but then he said:
> 
> On Friday, February 10, 2017, Geert Uytterhoeven wrote:
> > On Fri, Feb 10, 2017 at 3:59 PM, Chris Brandt <chris.bra...@renesas.com>
> > wrote:
> > > On Friday, February 10, 2017, Geert Uytterhoeven wrote:
> > >> >  static const char *const r7s72100_boards_compat_dt[] __initconst = {
> > >> >  "renesas,r7s72100", NULL, @@ -29,4 +58,5 @@
> > >> >  DT_MACHINE_START(R7S72100_DT, "Generic R7S72100
> > >> (Flattened Device Tree)")
> > >> > .init_early = shmobile_init_delay, .init_late  =
> > >> > shmobile_init_late, .dt_compat  =
> > >> > r7s72100_boards_compat_dt, +   .restart=
> > >> > r7s72100_restart, MACHINE_END
> > >>
> > >> Perhaps unsurprisingly, I'd recommend writing a watchdog driver instead.
> > >> drivers/watchdog/renesas_wdt.c (currently supporting R-Car Gen3 only) may
> > >> serve as an example.
> > >
> > > Why do you guys always make things more difficult for me... ;)
> > >
> > > To be clear, you are recommending I make a WDT timer driver (and not use
> > > .restart) and that will reset the system?
> > >
> > > Or, make a WDT driver just so I can steal the base address?
> > 
> > I meant a WDT timer driver. If the watchdog driver provides a .restart
> > callback, it will be used for system reset (hmm, renesas_wdt.c no longer has
> > the .restart callback, as per arm64 "system reset must be implemented using
> > PSCI"-policy).
> 

Hmm, ok. Guess I don't have to understand that you can not use the watchdog
driver because of the above, but implementing exactly the same functionality
in a separate driver is ok.

[ I am sure I am missing something here, so just ignore what I am saying ]

> 
> 
> > > v2: * changed DT bindings from 'wdt-reset' to 'rza-wdt' * changed hard
> > > coded register values to defines * added msleep to while(1) loop
> > 
> > Sure you can sleep here ?
> 
> As per Geert's review:
> 
> On Wednesday, February 15, 2017, Geert Uytterhoeven wrote:
> > > + +   /* Wait for WDT overflow */ +   while (1) +   ;
> > 
> > This burns CPU, and thus power, albeit for a very short time.  Perhaps add
> > an msleep() to the loop body?
> 
> Note that you only have 7.7us before the restart occurs, so probably not much
> sleeping will be going on.
> 
Let me rephrase my question. Is it guaranteed that you _can_ sleep in this
function, or in other words that it is guaranteed that interrupts are enabled ?

Thanks,
Guenter

Re: [PATCH v2 1/3] power: reset: Add Renesas reset driver

[Guenter Roeck]

On Thu, Feb 16, 2017 at 12:23:18PM -0500, Chris Brandt wrote:
> Some Renesas SoCs do not have a reset register and the only way to do a SW
> controlled reset is to use the watchdog timer. Additionally, since all the
> WDT timeout options are so quick, a system reset is about the only thing
> it's good for.
> 
> Signed-off-by: Chris Brandt 
> Reviewed-by: Geert Uytterhoeven 

Wondering - why not use the watchdog driver and the infrastructure
provided by the watchdog core (ie the restart callback) instead ?

> ---
> v2:
> * changed DT bindings from 'wdt-reset' to 'rza-wdt'
> * changed hard coded register values to defines
> * added msleep to while(1) loop

Sure you can sleep here ?

Guenter

> * removed unnecessary #include files
> * added Reviewed-by: Geert Uytterhoeven
> ---
>  drivers/power/reset/Kconfig |   9 +++
>  drivers/power/reset/Makefile|   1 +
>  drivers/power/reset/renesas-reset.c | 112 
> 
>  3 files changed, 122 insertions(+)
>  create mode 100644 drivers/power/reset/renesas-reset.c
> 
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index b8caccc..e3100c9 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -130,6 +130,15 @@ config POWER_RESET_QNAP
>  
> Say Y if you have a QNAP NAS.
>  
> +config POWER_RESET_RENESAS
> + tristate "Renesas WDT reset driver"
> + depends on ARCH_RENESAS || COMPILE_TEST
> + depends on HAS_IOMEM
> + help
> +   Reboot support for Renesas SoCs with WDT reset.
> +   Some Renesas SoCs do not have a reset register and the only way
> +   to do a SW controlled reset is to use the watchdog timer.
> +
>  config POWER_RESET_RESTART
>   bool "Restart power-off driver"
>   help
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index 11dae3b..a78a56c 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
>  obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o
>  obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
>  obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
> +obj-$(CONFIG_POWER_RESET_RENESAS) += renesas-reset.o
>  obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
>  obj-$(CONFIG_POWER_RESET_ST) += st-poweroff.o
>  obj-$(CONFIG_POWER_RESET_VERSATILE) += arm-versatile-reboot.o
> diff --git a/drivers/power/reset/renesas-reset.c 
> b/drivers/power/reset/renesas-reset.c
> new file mode 100644
> index 000..812cee0
> --- /dev/null
> +++ b/drivers/power/reset/renesas-reset.c
> @@ -0,0 +1,112 @@
> +/*
> + * Renesas WDT Reset Driver
> + *
> + * Copyright (C) 2017 Renesas Electronics America, Inc.
> + * Copyright (C) 2017 Chris Brandt
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * based on rmobile-reset.c
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* Watchdog Timer Registers */
> +#define WTCSR 0
> +#define WTCSR_MAGIC 0xA500
> +#define WTSCR_WT (1<<6)
> +#define WTSCR_TME (1<<5)
> +
> +#define WTCNT 2
> +#define WTCNT_MAGIC 0x5A00
> +
> +#define WRCSR 4
> +#define WRCSR_MAGIC 0x5A00
> +#define WRCSR_RSTE (1<<6)
> +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */
> +
> +static void __iomem *base;
> +
> +static int wdt_reset_handler(struct notifier_block *this,
> +  unsigned long mode, void *cmd)
> +{
> + pr_debug("%s %lu\n", __func__, mode);
> +
> + /* Dummy read (must read WRCSR:WOVF at least once before clearing) */
> + readb(base + WRCSR);
> +
> + writew(WRCSR_CLEAR_WOVF, base + WRCSR); /* Clear WOVF */
> + writew(WRCSR_MAGIC | WRCSR_RSTE, base + WRCSR); /* Reset Enable */
> + writew(WTCNT_MAGIC, base + WTCNT);  /* Counter to 00 */
> +
> + /* Start timer */
> + writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, base + WTCSR);
> +
> + /* Wait for WDT overflow (reset) */
> + while (1)
> + msleep(1);
> +
> + return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block wdt_reset_nb = {
> + .notifier_call = wdt_reset_handler,
> + .priority = 192,
> +};
> +
> +static int wdt_reset_probe(struct platform_device *pdev)
> +{
> + int error;
> +
> + base = of_iomap(pdev->dev.of_node, 0);
> + if (!base)
> + return -ENODEV;
> +
> + error = register_restart_handler(_reset_nb);
> + if (error) {
> + dev_err(>dev,
> + "cannot register restart handler (err=%d)\n", error);
> + goto fail_unmap;
> + }
> +
> + return 0;
> +
> +fail_unmap:
> + iounmap(base);
> + return error;
> +}
> +
> +static int wdt_reset_remove(struct platform_device *pdev)
> +{
> + 

Re: [PATCH v2] watchdog: softdog: make pretimeout support a compile option

[Guenter Roeck]

On Tue, Feb 07, 2017 at 03:03:29PM +0100, Wolfram Sang wrote:
> It occurred to me that the panic pretimeout governor will stall the
> softdog, because it is purely software which simply breaks when the
> kernel panics. Testing governors with the softdog on the other hand is
> really useful, so make this feature a compile time option which nees to
> be enabled explicitly. This also removes the overhead if pretimeout
> support is not used because it will now be compiled away (saving ~10% on
> ARM32).
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
> 
> Change since V1:
> 
> * use IS_DEFINED as if-conditions, removing the need for #ifdef
> 
> I have to admit that this result is better readable and what the compiler
> compiles away is sufficient.
> 
>  drivers/watchdog/Kconfig   |  8 
>  drivers/watchdog/softdog.c | 21 +
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index acb00b53a5207b..70726ce3d166e8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -71,6 +71,14 @@ config SOFT_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called softdog.
>  
> +config SOFT_WATCHDOG_PRETIMEOUT
> + bool "Software watchdog pretimeout governor support"
> + depends on SOFT_WATCHDOG && WATCHDOG_PRETIMEOUT_GOV
> + help
> +   Enable this if you want to use pretimeout governors with the software
> +   watchdog. Be aware that governors might affect the watchdog because it
> +   is purely software, e.g. the panic governor will stall it!
> +
>  config DA9052_WATCHDOG
>   tristate "Dialog DA9052 Watchdog"
>   depends on PMIC_DA9052
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index c7bdc986dca1c2..7983029852ab0d 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -87,11 +87,13 @@ static int softdog_ping(struct watchdog_device *w)
>   if (!mod_timer(_ticktock, jiffies + (w->timeout * HZ)))
>   __module_get(THIS_MODULE);
>  
> - if (w->pretimeout)
> - mod_timer(_preticktock, jiffies +
> -   (w->timeout - w->pretimeout) * HZ);
> - else
> - del_timer(_preticktock);
> + if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT)) {
> + if (w->pretimeout)
> + mod_timer(_preticktock, jiffies +
> +   (w->timeout - w->pretimeout) * HZ);
> + else
> + del_timer(_preticktock);
> + }
>  
>   return 0;
>  }
> @@ -101,15 +103,15 @@ static int softdog_stop(struct watchdog_device *w)
>   if (del_timer(_ticktock))
>   module_put(THIS_MODULE);
>  
> - del_timer(_preticktock);
> + if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT))
> + del_timer(_preticktock);
>  
>   return 0;
>  }
>  
>  static struct watchdog_info softdog_info = {
>   .identity = "Software Watchdog",
> - .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE |
> -WDIOF_PRETIMEOUT,
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
>  };
>  
>  static const struct watchdog_ops softdog_ops = {
> @@ -134,6 +136,9 @@ static int __init softdog_init(void)
>   watchdog_set_nowayout(_dev, nowayout);
>   watchdog_stop_on_reboot(_dev);
>  
> + if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT))
> + softdog_info.options |= WDIOF_PRETIMEOUT;
> +
>   ret = watchdog_register_device(_dev);
>   if (ret)
>   return ret;
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] watchdog: softdog: make pretimeout support a compile option

[Guenter Roeck]

On Fri, Jan 06, 2017 at 09:41:44AM +0100, Wolfram Sang wrote:
> It occured to me that the panic pretimeout governor will stall the
> softdog, because it is purely software which simply halts on panic.
> Testing governors with the softdog on the other hand is really useful.
> So, make this feature a compile time option which needs to be enabled
> explicitly. This also removes the overhead if pretimeout support is not
> used because it will now be compiled away (saving ~10% on ARM32).
> 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/watchdog/Kconfig   |  8 
>  drivers/watchdog/softdog.c | 27 +++
>  2 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index acb00b53a5207b..70726ce3d166e8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -71,6 +71,14 @@ config SOFT_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called softdog.
>  
> +config SOFT_WATCHDOG_PRETIMEOUT
> + bool "Software watchdog pretimeout governor support"
> + depends on SOFT_WATCHDOG && WATCHDOG_PRETIMEOUT_GOV
> + help
> +   Enable this if you want to use pretimeout governors with the software
> +   watchdog. Be aware that governors might affect the watchdog because it
> +   is purely software, e.g. the panic governor will stall it!
> +
>  config DA9052_WATCHDOG
>   tristate "Dialog DA9052 Watchdog"
>   depends on PMIC_DA9052
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index c7bdc986dca1c2..bc8b7da61d8aa7 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -74,6 +74,7 @@ static struct timer_list softdog_ticktock =
>  
>  static struct watchdog_device softdog_dev;
>  
> +#if IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT)
>  static void softdog_pretimeout(unsigned long data)

I would prefer __maybe_unused here ..

>  {
>   watchdog_notify_pretimeout(_dev);
> @@ -82,16 +83,23 @@ static void softdog_pretimeout(unsigned long data)
>  static struct timer_list softdog_preticktock =
>   TIMER_INITIALIZER(softdog_pretimeout, 0, 0);
>  
> +static struct timer_list *softdog_preticktock_ptr = _preticktock;
> +#else
> +static void *softdog_preticktock_ptr = NULL;
> +#endif /* CONFIG_SOFT_WATCHDOG_PRETIMEOUT */
> +
>  static int softdog_ping(struct watchdog_device *w)
>  {
>   if (!mod_timer(_ticktock, jiffies + (w->timeout * HZ)))
>   __module_get(THIS_MODULE);
>  
> - if (w->pretimeout)
> - mod_timer(_preticktock, jiffies +
> -   (w->timeout - w->pretimeout) * HZ);
> - else
> - del_timer(_preticktock);
> + if (softdog_preticktock_ptr) {

and "if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT))" here.

> + if (w->pretimeout)
> + mod_timer(softdog_preticktock_ptr, jiffies +
> +   (w->timeout - w->pretimeout) * HZ);
> + else
> + del_timer(softdog_preticktock_ptr);
> + }
>  
>   return 0;
>  }
> @@ -101,15 +109,15 @@ static int softdog_stop(struct watchdog_device *w)
>   if (del_timer(_ticktock))
>   module_put(THIS_MODULE);
>  
> - del_timer(_preticktock);
> + if (softdog_preticktock_ptr)

Is this conditional needed (assuming we get rid of softdog_preticktock_ptr) ?
Ok though if you want to use it to drop the code if not needed.

> + del_timer(softdog_preticktock_ptr);
>  
>   return 0;
>  }
>  
>  static struct watchdog_info softdog_info = {
>   .identity = "Software Watchdog",
> - .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE |
> -WDIOF_PRETIMEOUT,
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
>  };
>  
>  static const struct watchdog_ops softdog_ops = {
> @@ -134,6 +142,9 @@ static int __init softdog_init(void)
>   watchdog_set_nowayout(_dev, nowayout);
>   watchdog_stop_on_reboot(_dev);
>  
> + if (softdog_preticktock_ptr)
> + softdog_info.options |= WDIOF_PRETIMEOUT;
> +
>   ret = watchdog_register_device(_dev);
>   if (ret)
>   return ret;
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] linux/kernel.h: Fix DIV_ROUND_CLOSEST to support negative divisors

[Guenter Roeck]

On 12/22/2016 02:22 AM, Niklas Söderlund wrote:

Add support to DIV_ROUND_CLOSEST for negative divisors if both dividend
and divisor variable types are signed. This should not alter current
behavior for users of the macro as previously negative divisors where
not supported.

Before:

DIV_ROUND_CLOSEST(  59,  4) =  15
DIV_ROUND_CLOSEST(  59, -4) = -14
DIV_ROUND_CLOSEST( -59,  4) = -15
DIV_ROUND_CLOSEST( -59, -4) =  14

After:

DIV_ROUND_CLOSEST(  59,  4) =  15
DIV_ROUND_CLOSEST(  59, -4) = -15
DIV_ROUND_CLOSEST( -59,  4) = -15
DIV_ROUND_CLOSEST( -59, -4) =  15

Signed-off-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
---

Hi,

While working on a thermal driver I encounter a scenario where the
divisor could be negative, instead of adding local code to handle this I
though I first try to add support for this in DIV_ROUND_CLOSEST.

Maybe there is a reason why this is not already supported? If so please
let me know and I can do something locally in the driver and sorry for
the noise.



Only reason is that I could not figure out a way to make it work.
LGTM except for nitpick.

Reviewed-by: Guenter Roeck <li...@roeck-us.net>


 include/linux/kernel.h | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index bc6ed52a39b9..f1b2f7e1b2f0 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -95,16 +95,18 @@
 )

 /*
- * Divide positive or negative dividend by positive divisor and round
- * to closest integer. Result is undefined for negative divisors and
- * for negative dividends if the divisor variable type is unsigned.
+ * Divide positive or negative dividend by positive or negative divisor
+ * and round to closest integer. Result is undefined for negative
+ * divisors if dividends variable type is unsigned and for negative


s/dividends/the dividend/


+ * dividends if the divisor variable type is unsigned.
  */
 #define DIV_ROUND_CLOSEST(x, divisor)( \
 {  \
typeof(x) __x = x;  \
typeof(divisor) __d = divisor;  \
(((typeof(x))-1) > 0 ||  \
-((typeof(divisor))-1) > 0 || (__x) > 0) ?\
+((typeof(divisor))-1) > 0 ||\
+(((__x) > 0) == ((__d) > 0))) ?  \
(((__x) + ((__d) / 2)) / (__d)) :   \
(((__x) - ((__d) / 2)) / (__d));\
 }  \

Re: [PATCH 1/7] watchdog: add watchdog pretimeout framework

[Guenter Roeck]

On Thu, May 26, 2016 at 06:41:36PM +0200, Wolfram Sang wrote:
[ ... ]
> 
> No doubt about that. I had some ideas and thought it is easier to talk
> over code. If you want to rebase it, too, I'd be happy to check what you
> came up with to solve the problems. I might still argue that I prefer
> the less-code approach, but it will be Guenter's / Wim's decision, of
> course.
> 
I have a large back-log of patches to review. Simple patches with less code
will get preferential treating. The more complex, the higher the likelyhood
that the patches get pushed to the end of the queue.

Giving a quick glance, I liked Wolfram's patches because they seemed to
be simple and straightforward. I hope the next version won't add too much
additional complexity.

Guenter

Re: [PATCH 3/7] watchdog: softdog: consistently use softdog_ prefix

[Guenter Roeck]

On Wed, May 25, 2016 at 08:37:45AM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> And move module_init/exit to the proper place while here.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  drivers/watchdog/softdog.c | 21 ++---
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index a9ad27dd46502b..0a29f5a0833787 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -54,12 +54,12 @@ module_param(soft_panic, int, 0);
>  MODULE_PARM_DESC(soft_panic,
>   "Softdog action, set to 1 to panic, 0 to reboot (default=0)");
>  
> -static void watchdog_fire(unsigned long);
> +static void softdog_fire(unsigned long);
>  
> -static struct timer_list watchdog_ticktock =
> - TIMER_INITIALIZER(watchdog_fire, 0, 0);
> +static struct timer_list softdog_ticktock =
> + TIMER_INITIALIZER(softdog_fire, 0, 0);
>  
> -static void watchdog_fire(unsigned long data)
> +static void softdog_fire(unsigned long data)
>  {
>   module_put(THIS_MODULE);
>   if (soft_noboot)
> @@ -76,14 +76,14 @@ static void watchdog_fire(unsigned long data)
>  
>  static int softdog_ping(struct watchdog_device *w)
>  {
> - if (!mod_timer(_ticktock, jiffies+(w->timeout*HZ)))
> + if (!mod_timer(_ticktock, jiffies+(w->timeout*HZ)))
>   __module_get(THIS_MODULE);
>   return 0;
>  }
>  
>  static int softdog_stop(struct watchdog_device *w)
>  {
> - if (del_timer(_ticktock))
> + if (del_timer(_ticktock))
>   module_put(THIS_MODULE);
>  
>   return 0;
> @@ -115,7 +115,7 @@ static struct watchdog_device softdog_dev = {
>   .timeout = TIMER_MARGIN,
>  };
>  
> -static int __init watchdog_init(void)
> +static int __init softdog_init(void)
>  {
>   int ret;
>  
> @@ -132,14 +132,13 @@ static int __init watchdog_init(void)
>  
>   return 0;
>  }
> +module_init(softdog_init);
>  
> -static void __exit watchdog_exit(void)
> +static void __exit softdog_exit(void)
>  {
>   watchdog_unregister_device(_dev);
>  }
> -
> -module_init(watchdog_init);
> -module_exit(watchdog_exit);
> +module_exit(softdog_exit);
>  
>  MODULE_AUTHOR("Alan Cox");
>  MODULE_DESCRIPTION("Software Watchdog Device Driver");
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] watchdog: softdog: remove forward declaration

[Guenter Roeck]

On Wed, May 25, 2016 at 08:37:46AM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  drivers/watchdog/softdog.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index 0a29f5a0833787..42faa3d424d5ca 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -54,11 +54,6 @@ module_param(soft_panic, int, 0);
>  MODULE_PARM_DESC(soft_panic,
>   "Softdog action, set to 1 to panic, 0 to reboot (default=0)");
>  
> -static void softdog_fire(unsigned long);
> -
> -static struct timer_list softdog_ticktock =
> - TIMER_INITIALIZER(softdog_fire, 0, 0);
> -
>  static void softdog_fire(unsigned long data)
>  {
>   module_put(THIS_MODULE);
> @@ -74,6 +69,9 @@ static void softdog_fire(unsigned long data)
>   }
>  }
>  
> +static struct timer_list softdog_ticktock =
> + TIMER_INITIALIZER(softdog_fire, 0, 0);
> +
>  static int softdog_ping(struct watchdog_device *w)
>  {
>   if (!mod_timer(_ticktock, jiffies+(w->timeout*HZ)))
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] watchdog: softdog: sort includes to avoid duplicates

[Guenter Roeck]

On Wed, May 25, 2016 at 08:37:47AM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  drivers/watchdog/softdog.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index 42faa3d424d5ca..ab0e02fc81a276 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -21,15 +21,15 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include 
> +#include 
> +#include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
> +#include 
>  #include 
> -#include 
> -#include 
> -#include 
> -#include 
>  
>  #define TIMER_MARGIN 60  /* Default is 60 seconds */
>  static unsigned int soft_margin = TIMER_MARGIN;  /* in seconds */
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] watchdog: softdog: drop superfluous set_timeout callback

[Guenter Roeck]

On Wed, May 25, 2016 at 08:37:48AM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> If we leave set_timeout empty, the core will do exactly what is
> implemented here anyway.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  drivers/watchdog/softdog.c | 7 ---
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index ab0e02fc81a276..5e3a30b99d4415 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -87,12 +87,6 @@ static int softdog_stop(struct watchdog_device *w)
>   return 0;
>  }
>  
> -static int softdog_set_timeout(struct watchdog_device *w, unsigned int t)
> -{
> - w->timeout = t;
> - return 0;
> -}
> -
>  static struct watchdog_info softdog_info = {
>   .identity = "Software Watchdog",
>   .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> @@ -102,7 +96,6 @@ static struct watchdog_ops softdog_ops = {
>   .owner = THIS_MODULE,
>   .start = softdog_ping,
>   .stop = softdog_stop,
> - .set_timeout = softdog_set_timeout,
>  };
>  
>  static struct watchdog_device softdog_dev = {
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] watchdog: softdog: improve coding style

[Guenter Roeck]

On Wed, May 25, 2016 at 08:37:49AM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  drivers/watchdog/softdog.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index 5e3a30b99d4415..b067edf246dff2 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -57,9 +57,9 @@ MODULE_PARM_DESC(soft_panic,
>  static void softdog_fire(unsigned long data)
>  {
>   module_put(THIS_MODULE);
> - if (soft_noboot)
> + if (soft_noboot) {
>   pr_crit("Triggered - Reboot ignored\n");
> - else if (soft_panic) {
> + } else if (soft_panic) {
>   pr_crit("Initiating panic\n");
>   panic("Software Watchdog Timer expired");
>   } else {
> @@ -74,7 +74,7 @@ static struct timer_list softdog_ticktock =
>  
>  static int softdog_ping(struct watchdog_device *w)
>  {
> - if (!mod_timer(_ticktock, jiffies+(w->timeout*HZ)))
> + if (!mod_timer(_ticktock, jiffies + (w->timeout * HZ)))
>   __module_get(THIS_MODULE);
>   return 0;
>  }
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/7] watchdog: softdog: use watchdog core to init timeout value

[Guenter Roeck]

On Wed, May 25, 2016 at 08:37:44AM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> Error string and comment say we fall back to a default, but in reality
> we bailed out. Refactor the code to use the core helper which then
> matches the described behaviour. While updating the init message anyhow,
> shorten it while we are here; no need for versioning there as well and
> the name is already given via pr_fmt.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  drivers/watchdog/softdog.c | 17 +
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index 8bc0b164afc94b..a9ad27dd46502b 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -111,22 +111,15 @@ static struct watchdog_device softdog_dev = {
>   .info = _info,
>   .ops = _ops,
>   .min_timeout = 1,
> - .max_timeout = 0x
> + .max_timeout = 65535,
> + .timeout = TIMER_MARGIN,
>  };
>  
>  static int __init watchdog_init(void)
>  {
>   int ret;
>  
> - /* Check that the soft_margin value is within it's range;
> -if not reset to the default */
> - if (soft_margin < 1 || soft_margin > 65535) {
> - pr_info("soft_margin must be 0 < soft_margin < 65536, using 
> %d\n",
> - TIMER_MARGIN);
> - return -EINVAL;
> - }
> - softdog_dev.timeout = soft_margin;
> -
> + watchdog_init_timeout(_dev, soft_margin, NULL);
>   watchdog_set_nowayout(_dev, nowayout);
>   watchdog_stop_on_reboot(_dev);
>  
> @@ -134,8 +127,8 @@ static int __init watchdog_init(void)
>   if (ret)
>   return ret;
>  
> - pr_info("Software Watchdog Timer: 0.08 initialized. soft_noboot=%d 
> soft_margin=%d sec soft_panic=%d (nowayout=%d)\n",
> - soft_noboot, soft_margin, soft_panic, nowayout);
> + pr_info("initialized. soft_noboot=%d soft_margin=%d sec soft_panic=%d 
> (nowayout=%d)\n",
> + soft_noboot, softdog_dev.timeout, soft_panic, nowayout);
>  
>   return 0;
>  }
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/4] watchdog: add driver for Renesas Gen3 WDT watchdogs

[Guenter Roeck]

On Mon, Apr 04, 2016 at 08:14:02PM +0200, Geert Uytterhoeven wrote:
> Hi Günter,
> 
> On Mon, Apr 4, 2016 at 7:52 PM, Guenter Roeck <li...@roeck-us.net> wrote:
> > On Mon, Apr 04, 2016 at 07:02:45PM +0200, Geert Uytterhoeven wrote:
> >> On Mon, Apr 4, 2016 at 5:25 PM, Guenter Roeck <li...@roeck-us.net> wrote:
> >> > On Mon, Apr 04, 2016 at 05:21:44PM +0200, Geert Uytterhoeven wrote:
> >> >> On Mon, Apr 4, 2016 at 4:59 PM, Wolfram Sang <w...@the-dreams.de> wrote:
> >> >> >> My Salvator-X reboots after timeout from "cat > /dev/watchdog0", but
> >> >> >> it doesn't reboot through "reboot" or "reboot -f"?
> >> >> >
> >> >> > That sadly doesn't work on Gen3. From the RFC v5 cover letter:
> >> >>
> >> >> > ===
> >> >> >
> >> >> > * drop restart_handler since ARM64 uses PSCI firmware resets which do
> >> >> >   not call restart handlers
> >> >> >
> >> >> > The last point was quite a bummer to me because plain reboot was the
> >> >> > reason I wrote this driver ;) Well, so is life...
> >> >>
> >> >> That's indeed silly. Can't we have it as a low-priority restart 
> >> >> handler, to
> >> >
> >> > Yes, it is. It defeats the purpose of restart handlers. PSCI reset 
> >> > should have
> >> > been implemented as a high priority restart handler.
> >>
> >> Unfortunately that won't work: psci_sys_reset() doesn't return on failure.
> >>
> > You mean it just hangs ? That is bad. If that is the case, it is not 
> > reliable
> > and thus should be a low priority (or at best medium priority) restart 
> > handler
> > (which can be replaced with a working higher priority one).
> 
> On my Salvator-X board, it hangs. The call into PSCI doesn't return.
> As this is firmware, it may depend on the version of the firmware.
> 
> >> We can still clear arm_pm_restart in platform code, though ;-)
> >>
> > I had originally planned to replace arm_pm_restart() completely with restart
> > handlers. Maybe I should revive the effort ?
> 
> Perhaps. As I remember, it was not such a pretty experience?
> 
That was the attempt to introduce the same mechanism for power-off handling.
Yes, that bad experience got me to abandon the attempt to finish the restart
handler cleanup as well, but it may still worth at least a final attempt.

Guenter

Re: [PATCH v2 0/4] watchdog: add driver for Renesas Gen3 WDT watchdogs

[Guenter Roeck]

On Mon, Apr 04, 2016 at 07:02:45PM +0200, Geert Uytterhoeven wrote:
> Hi Günter,
> 
> On Mon, Apr 4, 2016 at 5:25 PM, Guenter Roeck <li...@roeck-us.net> wrote:
> > On Mon, Apr 04, 2016 at 05:21:44PM +0200, Geert Uytterhoeven wrote:
> >> On Mon, Apr 4, 2016 at 4:59 PM, Wolfram Sang <w...@the-dreams.de> wrote:
> >> >> My Salvator-X reboots after timeout from "cat > /dev/watchdog0", but
> >> >> it doesn't reboot through "reboot" or "reboot -f"?
> >> >
> >> > That sadly doesn't work on Gen3. From the RFC v5 cover letter:
> >>
> >> > ===
> >> >
> >> > * drop restart_handler since ARM64 uses PSCI firmware resets which do
> >> >   not call restart handlers
> >> >
> >> > The last point was quite a bummer to me because plain reboot was the
> >> > reason I wrote this driver ;) Well, so is life...
> >>
> >> That's indeed silly. Can't we have it as a low-priority restart handler, to
> >
> > Yes, it is. It defeats the purpose of restart handlers. PSCI reset should 
> > have
> > been implemented as a high priority restart handler.
> 
> Unfortunately that won't work: psci_sys_reset() doesn't return on failure.
> 
You mean it just hangs ? That is bad. If that is the case, it is not reliable
and thus should be a low priority (or at best medium priority) restart handler
(which can be replaced with a working higher priority one).

> We can still clear arm_pm_restart in platform code, though ;-)
> 
I had originally planned to replace arm_pm_restart() completely with restart
handlers. Maybe I should revive the effort ?

Guenter

Re: [PATCH v2 4/4] arm64: defconfig: enable Renesas Watchdog Timer

[Guenter Roeck]

On Fri, Apr 01, 2016 at 01:56:26PM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

> ---
>  arch/arm64/configs/defconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index f78360c08c1eca..795d40e810ff95 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -168,6 +168,8 @@ CONFIG_POWER_RESET_SYSCON=y
>  CONFIG_THERMAL=y
>  CONFIG_THERMAL_EMULATION=y
>  CONFIG_EXYNOS_THERMAL=y
> +CONFIG_WATCHDOG=y
> +CONFIG_RENESAS_WDT=y
>  CONFIG_MFD_SPMI_PMIC=y
>  CONFIG_MFD_SEC_CORE=y
>  CONFIG_REGULATOR=y
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] arm64: dts: r8a7795: Add RWDT node

[Guenter Roeck]

On Fri, Apr 01, 2016 at 01:56:24PM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> This patch adds the RWDT device node for r8a7795.
> 
> Signed-off-by: Takeshi Kihara <takeshi.kihara...@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Acked-by: Guenter Roeck <li...@roeck-us.net>

I assume the dts patches will go through the an arm64 tree.
Is this correct ?

Thanks,
Guenter

> ---
>  arch/arm64/boot/dts/renesas/r8a7795.dtsi | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi 
> b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> index bccd52e0462b92..ab0d20b7cb6f7b 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> @@ -167,6 +167,14 @@
>   (GIC_CPU_MASK_SIMPLE(4) | 
> IRQ_TYPE_LEVEL_HIGH)>;
>   };
>  
> + wdt0: watchdog@e602 {
> + compatible = "renesas,r8a7795-wdt", 
> "renesas,rcar-gen3-wdt";
> + reg = <0 0xe602 0 0x0c>;
> + clocks = < CPG_MOD 402>;
> + power-domains = <>;
> + status = "disabled";
> + };
> +
>   gpio0: gpio@e605 {
>   compatible = "renesas,gpio-r8a7795",
>"renesas,gpio-rcar";
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] watchdog: renesas-wdt: add driver

[Guenter Roeck]

Hi Wolfram,

On 04/01/2016 04:36 AM, Wolfram Sang wrote:

Hi Guenter,


+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, S_IRUGO);


Sure you want this parameter readable ? No problem with me, but it is unusual,
so I figure it is worth asking.


No reason, will stick to the usual case.


+static void rwdt_write(struct rwdt_priv *priv, u32 val, unsigned reg)


Please use 'unsigned int' throughout.


Can do. May I ask why?



There are people changing unsigned -> unsigned int with coccinelle scripts.
Besides that, checkpatch warns about it.

Sure, I know, checkpatch is just a script, I know that story, but I don't
want to have to deal with the inevitable follow-up patches.


+   rwdt_write(priv, 65536 - wdev->timeout * priv->clks_per_sec, RWTCNT);
+


Just wondering, does reading RWTCNT return the remaining timeout ?
If so, you could easily implement WDIOC_GETTIMEOUT.


I assume you mean GETTIMELEFT. Tried that, seems to work.


Yes, sorry.

Thanks,
Guenter


function. In other words, you can just drop rwdt_set_timeout() entirely.


Cool news! Works fine.

Thanks for the prompt review! Will send V2 in a minute.

Wolfram

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] watchdog: renesas-wdt: add driver

[Guenter Roeck]

Hi Wolfram,

On Wed, Mar 30, 2016 at 05:28:42PM +0200, Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Add support for watchdogs (RWDT and SWDT) found on RCar Gen3 based SoCs
> from Renesas.
> 
> Signed-off-by: Wolfram Sang 
> ---
>  
[ ... ]

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published 
> by
> + * the Free Software Foundation.
> + */

Please also include linux/bitops.h.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
[ ... ]

> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, S_IRUGO);

Sure you want this parameter readable ? No problem with me, but it is unusual,
so I figure it is worth asking.

> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started 
> (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct rwdt_priv {
> + void __iomem *base;
> + struct watchdog_device wdev;
> + struct clk *clk;
> + unsigned clks_per_sec;
> + u8 cks;
> +};
> +
> +static void rwdt_write(struct rwdt_priv *priv, u32 val, unsigned reg)

Please use 'unsigned int' throughout.

> +{
> + if (reg == RWTCNT)
> + val |= 0x5a5a;
> + else
> + val |= 0xa5a5a500;
> +
> + writel_relaxed(val, priv->base + reg);
> +}
> +
> +static int rwdt_init_timeout(struct watchdog_device *wdev)
> +{
> + struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> + rwdt_write(priv, 65536 - wdev->timeout * priv->clks_per_sec, RWTCNT);
> +

Just wondering, does reading RWTCNT return the remaining timeout ?
If so, you could easily implement WDIOC_GETTIMEOUT.

> + return 0;
> +}
> +
> +static int rwdt_set_timeout(struct watchdog_device *wdev, unsigned 
> new_timeout)
> +{
> + wdev->timeout = new_timeout;
> + rwdt_init_timeout(wdev);
> +
The watchdog core calls the ping function after updating the timeout,
so the call here is unnecessary. On top of that, the watchdog core now also
updates wdev->timeout if WDIOF_SETTIMEOUT is set and there is no set_timeout
function. In other words, you can just drop rwdt_set_timeout() entirely.

Thanks,
Guenter