[PATCH 1/2] watchdog: dw: RMW the control register

[PATCH 1/2] watchdog: dw: RMW the control register

[Brian Norris]

RK3399 has rst_pulse_length in CONTROL_REG[4:2], determining the length
of pulse to issue for system reset. We shouldn't clobber this value,
because that might make the system reset ineffective. On RK3399, we're
seeing that a value of 000b (meaning 2 cycles) yields an unreliable
(partial?) reset, and so we only fully reset after the watchdog fires a
second time. If we retain the system default (010b, or 8 clock cycles),
then the watchdog reset is much more reliable.

Read-modify-write retains the system value and improves reset
reliability.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/watchdog/dw_wdt.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index c2f4ff516230..6925d3e6c6b3 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -34,6 +34,7 @@
 
 #define WDOG_CONTROL_REG_OFFSET		    0x00
 #define WDOG_CONTROL_REG_WDT_EN_MASK	    0x01
+#define WDOG_CONTROL_REG_RESP_MODE_MASK	    0x02
 #define WDOG_TIMEOUT_RANGE_REG_OFFSET	    0x04
 #define WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT    4
 #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
@@ -124,11 +125,16 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
 static int dw_wdt_start(struct watchdog_device *wdd)
 {
 	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
+	u32 val;
 
 	dw_wdt_set_timeout(wdd, wdd->timeout);
 
-	writel(WDOG_CONTROL_REG_WDT_EN_MASK,
-	       dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
+	val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
+	/* Disable interrupt mode; always perform system reset. */
+	val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
+	/* Enable watchdog. */
+	val |= WDOG_CONTROL_REG_WDT_EN_MASK;
+	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
 
 	return 0;
 }
-- 
2.16.2.660.g709887971b-goog

[PATCH 2/2] watchdog: dw: save/restore control and timeout across suspend/resume

[Brian Norris]

Some platforms lose this state in suspend. It should be safe to do this
unconditionally.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/watchdog/dw_wdt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 6925d3e6c6b3..094986047194 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -57,6 +57,9 @@ struct dw_wdt {
 	unsigned long		rate;
 	struct watchdog_device	wdd;
 	struct reset_control	*rst;
+	/* Save/restore */
+	u32			control;
+	u32			timeout;
 };
 
 #define to_dw_wdt(wdd)	container_of(wdd, struct dw_wdt, wdd)
@@ -204,6 +207,9 @@ static int dw_wdt_suspend(struct device *dev)
 {
 	struct dw_wdt *dw_wdt = dev_get_drvdata(dev);
 
+	dw_wdt->control = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
+	dw_wdt->timeout = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+
 	clk_disable_unprepare(dw_wdt->clk);
 
 	return 0;
@@ -217,6 +223,9 @@ static int dw_wdt_resume(struct device *dev)
 	if (err)
 		return err;
 
+	writel(dw_wdt->timeout, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+	writel(dw_wdt->control, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
+
 	dw_wdt_ping(&dw_wdt->wdd);
 
 	return 0;
-- 
2.16.2.660.g709887971b-goog

Re: [PATCH 1/2] watchdog: dw: RMW the control register

[Brian Norris]

+ linux-rockchip (probably should have included in the first place)

On Fri, Mar 9, 2018 at 6:44 PM, Brian Norris <briannorris@chromium.org> wrote:
> RK3399 has rst_pulse_length in CONTROL_REG[4:2], determining the length
> of pulse to issue for system reset. We shouldn't clobber this value,
> because that might make the system reset ineffective. On RK3399, we're
> seeing that a value of 000b (meaning 2 cycles) yields an unreliable
> (partial?) reset, and so we only fully reset after the watchdog fires a
> second time. If we retain the system default (010b, or 8 clock cycles),
> then the watchdog reset is much more reliable.
>
> Read-modify-write retains the system value and improves reset
> reliability.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  drivers/watchdog/dw_wdt.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index c2f4ff516230..6925d3e6c6b3 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -34,6 +34,7 @@
>
>  #define WDOG_CONTROL_REG_OFFSET                    0x00
>  #define WDOG_CONTROL_REG_WDT_EN_MASK       0x01
> +#define WDOG_CONTROL_REG_RESP_MODE_MASK            0x02
>  #define WDOG_TIMEOUT_RANGE_REG_OFFSET      0x04
>  #define WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT    4
>  #define WDOG_CURRENT_COUNT_REG_OFFSET      0x08
> @@ -124,11 +125,16 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>  static int dw_wdt_start(struct watchdog_device *wdd)
>  {
>         struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> +       u32 val;
>
>         dw_wdt_set_timeout(wdd, wdd->timeout);
>
> -       writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> -              dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> +       val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> +       /* Disable interrupt mode; always perform system reset. */
> +       val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
> +       /* Enable watchdog. */
> +       val |= WDOG_CONTROL_REG_WDT_EN_MASK;
> +       writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>
>         return 0;
>  }
> --
> 2.16.2.660.g709887971b-goog
>

Re: [PATCH 1/2] watchdog: dw: RMW the control register

[Guenter Roeck]

Hi Brian,

On 03/09/2018 06:44 PM, Brian Norris wrote:
> RK3399 has rst_pulse_length in CONTROL_REG[4:2], determining the length
> of pulse to issue for system reset. We shouldn't clobber this value,
> because that might make the system reset ineffective. On RK3399, we're
> seeing that a value of 000b (meaning 2 cycles) yields an unreliable
> (partial?) reset, and so we only fully reset after the watchdog fires a
> second time. If we retain the system default (010b, or 8 clock cycles),
> then the watchdog reset is much more reliable.
> 
> Read-modify-write retains the system value and improves reset
> reliability.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>   drivers/watchdog/dw_wdt.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index c2f4ff516230..6925d3e6c6b3 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -34,6 +34,7 @@
>   
>   #define WDOG_CONTROL_REG_OFFSET		    0x00
>   #define WDOG_CONTROL_REG_WDT_EN_MASK	    0x01
> +#define WDOG_CONTROL_REG_RESP_MODE_MASK	    0x02
>   #define WDOG_TIMEOUT_RANGE_REG_OFFSET	    0x04
>   #define WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT    4
>   #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
> @@ -124,11 +125,16 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>   static int dw_wdt_start(struct watchdog_device *wdd)
>   {
>   	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> +	u32 val;
>   
>   	dw_wdt_set_timeout(wdd, wdd->timeout);
>   
> -	writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> -	       dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> +	val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> +	/* Disable interrupt mode; always perform system reset. */
> +	val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;

You don't talk about this change in the description.

> +	/* Enable watchdog. */
> +	val |= WDOG_CONTROL_REG_WDT_EN_MASK;
> +	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>   
>   	return 0;
>   }
> 

Similar code is in dw_wdt_restart(), where it may be equally or even
more important. Granted, only if the watchdog isn't running, but still...

Guenter

Re: [PATCH 2/2] watchdog: dw: save/restore control and timeout across suspend/resume

[Guenter Roeck]

On 03/09/2018 06:44 PM, Brian Norris wrote:
> Some platforms lose this state in suspend. It should be safe to do this
> unconditionally.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

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

> ---
>   drivers/watchdog/dw_wdt.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 6925d3e6c6b3..094986047194 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -57,6 +57,9 @@ struct dw_wdt {
>   	unsigned long		rate;
>   	struct watchdog_device	wdd;
>   	struct reset_control	*rst;
> +	/* Save/restore */
> +	u32			control;
> +	u32			timeout;
>   };
>   
>   #define to_dw_wdt(wdd)	container_of(wdd, struct dw_wdt, wdd)
> @@ -204,6 +207,9 @@ static int dw_wdt_suspend(struct device *dev)
>   {
>   	struct dw_wdt *dw_wdt = dev_get_drvdata(dev);
>   
> +	dw_wdt->control = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> +	dw_wdt->timeout = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> +
>   	clk_disable_unprepare(dw_wdt->clk);
>   
>   	return 0;
> @@ -217,6 +223,9 @@ static int dw_wdt_resume(struct device *dev)
>   	if (err)
>   		return err;
>   
> +	writel(dw_wdt->timeout, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> +	writel(dw_wdt->control, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> +
>   	dw_wdt_ping(&dw_wdt->wdd);
>   
>   	return 0;
> 

Re: [PATCH 1/2] watchdog: dw: RMW the control register

[Brian Norris]

Hi,

On Fri, Mar 09, 2018 at 07:20:38PM -0800, Guenter Roeck wrote:
> On 03/09/2018 06:44 PM, Brian Norris wrote:
> > RK3399 has rst_pulse_length in CONTROL_REG[4:2], determining the length
> > of pulse to issue for system reset. We shouldn't clobber this value,
> > because that might make the system reset ineffective. On RK3399, we're
> > seeing that a value of 000b (meaning 2 cycles) yields an unreliable
> > (partial?) reset, and so we only fully reset after the watchdog fires a
> > second time. If we retain the system default (010b, or 8 clock cycles),
> > then the watchdog reset is much more reliable.
> > 
> > Read-modify-write retains the system value and improves reset
> > reliability.
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> >   drivers/watchdog/dw_wdt.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index c2f4ff516230..6925d3e6c6b3 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -34,6 +34,7 @@
> >   #define WDOG_CONTROL_REG_OFFSET		    0x00
> >   #define WDOG_CONTROL_REG_WDT_EN_MASK	    0x01
> > +#define WDOG_CONTROL_REG_RESP_MODE_MASK	    0x02
> >   #define WDOG_TIMEOUT_RANGE_REG_OFFSET	    0x04
> >   #define WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT    4
> >   #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
> > @@ -124,11 +125,16 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >   static int dw_wdt_start(struct watchdog_device *wdd)
> >   {
> >   	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> > +	u32 val;
> >   	dw_wdt_set_timeout(wdd, wdd->timeout);
> > -	writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> > -	       dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> > +	val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> > +	/* Disable interrupt mode; always perform system reset. */
> > +	val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
> 
> You don't talk about this change in the description.

I guess I could mention it. I was assuming that was an intended behavior
of the existing driver: that we set resp_mode=0 (via clobber), so we
always get a system reset (we don't try to handle any interrupt in this
driver).

I'll include something along those lines in the commit message.

> > +	/* Enable watchdog. */
> > +	val |= WDOG_CONTROL_REG_WDT_EN_MASK;
> > +	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> >   	return 0;
> >   }
> > 
> 
> Similar code is in dw_wdt_restart(), where it may be equally or even
> more important. Granted, only if the watchdog isn't running, but still...

Oh, I misread that code. It looked like an read/modify/write already,
but it was actually just a read/check/write. I should fix that, since
otherwise the restart will clobber the very thing I'm trying to fix
here, which might actually make the intended machine restart quite
ineffective.

Thanks,
Brian

Re: [PATCH 1/2] watchdog: dw: RMW the control register

[Guenter Roeck]

Hi Brian,

On 03/09/2018 07:28 PM, Brian Norris wrote:
> Hi,
> 
> On Fri, Mar 09, 2018 at 07:20:38PM -0800, Guenter Roeck wrote:
>> On 03/09/2018 06:44 PM, Brian Norris wrote:
>>> RK3399 has rst_pulse_length in CONTROL_REG[4:2], determining the length
>>> of pulse to issue for system reset. We shouldn't clobber this value,
>>> because that might make the system reset ineffective. On RK3399, we're
>>> seeing that a value of 000b (meaning 2 cycles) yields an unreliable
>>> (partial?) reset, and so we only fully reset after the watchdog fires a
>>> second time. If we retain the system default (010b, or 8 clock cycles),
>>> then the watchdog reset is much more reliable.
>>>
>>> Read-modify-write retains the system value and improves reset
>>> reliability.
>>>
>>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>>> ---
>>>    drivers/watchdog/dw_wdt.c | 10 ++++++++--
>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
>>> index c2f4ff516230..6925d3e6c6b3 100644
>>> --- a/drivers/watchdog/dw_wdt.c
>>> +++ b/drivers/watchdog/dw_wdt.c
>>> @@ -34,6 +34,7 @@
>>>    #define WDOG_CONTROL_REG_OFFSET		    0x00
>>>    #define WDOG_CONTROL_REG_WDT_EN_MASK	    0x01
>>> +#define WDOG_CONTROL_REG_RESP_MODE_MASK	    0x02
>>>    #define WDOG_TIMEOUT_RANGE_REG_OFFSET	    0x04
>>>    #define WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT    4
>>>    #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
>>> @@ -124,11 +125,16 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>>>    static int dw_wdt_start(struct watchdog_device *wdd)
>>>    {
>>>    	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
>>> +	u32 val;
>>>    	dw_wdt_set_timeout(wdd, wdd->timeout);
>>> -	writel(WDOG_CONTROL_REG_WDT_EN_MASK,
>>> -	       dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>>> +	val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>>> +	/* Disable interrupt mode; always perform system reset. */
>>> +	val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
>>
>> You don't talk about this change in the description.
> 
> I guess I could mention it. I was assuming that was an intended behavior
> of the existing driver: that we set resp_mode=0 (via clobber), so we
> always get a system reset (we don't try to handle any interrupt in this
> driver).
>
I don't think it was intended behavior. We don't even know for sure (or at least
I don't know) if all implementations of this IP have the same configuration bit
layout. All we can do is hope for the best.

Still, clobbering just 1 bit is better than clobbering 30 bit.

Thanks,
Guenter

> I'll include something along those lines in the commit message.
> 
>>> +	/* Enable watchdog. */
>>> +	val |= WDOG_CONTROL_REG_WDT_EN_MASK;
>>> +	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>>>    	return 0;
>>>    }
>>>
>>
>> Similar code is in dw_wdt_restart(), where it may be equally or even
>> more important. Granted, only if the watchdog isn't running, but still...
> 
> Oh, I misread that code. It looked like an read/modify/write already,
> but it was actually just a read/check/write. I should fix that, since
> otherwise the restart will clobber the very thing I'm trying to fix
> here, which might actually make the intended machine restart quite
> ineffective.
> 
> Thanks,
> Brian
> 

Re: [PATCH 1/2] watchdog: dw: RMW the control register

[Brian Norris]

On Fri, Mar 9, 2018 at 8:02 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 03/09/2018 07:28 PM, Brian Norris wrote:
>> I guess I could mention it. I was assuming that was an intended behavior
>> of the existing driver: that we set resp_mode=0 (via clobber), so we
>> always get a system reset (we don't try to handle any interrupt in this
>> driver).
>>
> I don't think it was intended behavior. We don't even know for sure (or at
> least
> I don't know) if all implementations of this IP have the same configuration
> bit
> layout. All we can do is hope for the best.

Huh, OK. I did try to look for any sort of generic DesignWare register
documentation, and I couldn't find one easily (even with a proper
Synopsys account -- maybe I wasn't looking in the right place). But
besides the Rockchip TRMs, I did find some openly accessible Altera
SoCFPGA docs [1] which also use this, and they have a few things to
add:
(1) they have the same 'reset pulse length' field, except it's labeled RO
(2) they have the same 'response mode' field
(3) the docs for the entire register say:

"The value of a reserved bit must be maintained in software. When you
modify registers containing reserved bit fields, you must use a
read-modify-write operation to preserve state and prevent
indeterminate system behavior."

So, that pretty well corroborates my patch. Nice.

> Still, clobbering just 1 bit is better than clobbering 30 bit.

Yeah, that's the idea. Well, as long as it's only the 1 bit I want to clobber ;)

I guess if we really find that any of this becomes more problematic
(and varies enough from IP to IP), then we'll need chip-specific
compatible properties.

Brian

[1] e.g. https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/hb/arria-10/a10_5v4.pdf