watchdog: renesas_wdt: Fix interrupt enable for timer
diff mbox series

Message ID 1558603778-20848-1-git-send-email-na-hoan@jinso.co.jp
State Under Review
Delegated to: Geert Uytterhoeven
Headers show
Series
  • watchdog: renesas_wdt: Fix interrupt enable for timer
Related show

Commit Message

グェン・アン・ホァン May 23, 2019, 9:29 a.m. UTC
From: Hoan Nguyen An <na-hoan@jinso.co.jp>

Fix setting for bit WOVFE of RWTCSRA. Keep it enable follow hardware document.

Signed-off-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
---
 drivers/watchdog/renesas_wdt.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Wolfram Sang May 23, 2019, 11:04 a.m. UTC | #1
Hi,

On Thu, May 23, 2019 at 06:29:37PM +0900, Nguyen An Hoan wrote:
> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
> 
> Fix setting for bit WOVFE of RWTCSRA. Keep it enable follow hardware document.

Hmm, I can't find it in the docs. Which version of the documentation do
you use?


> -	rwdt_write(priv, priv->cks, RWTCSRA);
> +	val |= priv->cks;
> +	rwdt_write(priv, val, RWTCSRA);

Have you tested this successfully? According to the docs, CKS bits are
all 1 by default. So, your |= operation should be a NOP and we can't
select a CKS value anymore if I am not mistaken.

>  	rwdt_write(priv, 0, RWTCSRB);
>  
>  	while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
>  		cpu_relax();
> -
> -	rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA);
> +	/* Enable interrupt and timer */
> +	rwdt_write(priv, val | RWTCSRA_WOVFE | RWTCSRA_TME, RWTCSRA);

What is the use of enabling an interrupt without having an interrupt
handler? (And I never understood why there is an interrupt for an
overflowing watchdog. We won't have time to serve it, or am I
overlooking something obvious?)

Kind regards,

   Wolfram
Geert Uytterhoeven May 23, 2019, 11:26 a.m. UTC | #2
Hi Wolfram,

On Thu, May 23, 2019 at 1:04 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, May 23, 2019 at 06:29:37PM +0900, Nguyen An Hoan wrote:
> > From: Hoan Nguyen An <na-hoan@jinso.co.jp>
> >
> > Fix setting for bit WOVFE of RWTCSRA. Keep it enable follow hardware document.

> >       rwdt_write(priv, 0, RWTCSRB);
> >
> >       while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
> >               cpu_relax();
> > -
> > -     rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA);
> > +     /* Enable interrupt and timer */
> > +     rwdt_write(priv, val | RWTCSRA_WOVFE | RWTCSRA_TME, RWTCSRA);
>
> What is the use of enabling an interrupt without having an interrupt
> handler?

Exactly.

> (And I never understood why there is an interrupt for an
> overflowing watchdog. We won't have time to serve it, or am I
> overlooking something obvious?)

I guess it (the hardware, not the Linux watchdog driver) might be used
as a generic timer? Or the interrupt may signal the RT core that the
application cores have been restarted?

But in the context of (the current) Linux watchdog driver, this doesn't
make much sense.

Gr{oetje,eeting}s,

                        Geert
グェン・アン・ホァン May 24, 2019, 1:13 a.m. UTC | #3
Dear Wolfram-san
Dear Geert- san

Thank you very much
Wolfram Sang wrote:
> Hi,
> 
> On Thu, May 23, 2019 at 06:29:37PM +0900, Nguyen An Hoan wrote:
> > From: Hoan Nguyen An <na-hoan@jinso.co.jp>
> > 
> > Fix setting for bit WOVFE of RWTCSRA. Keep it enable follow hardware document.
> 
> Hmm, I can't find it in the docs. Which version of the documentation do
> you use?
> 
> 
> > -	rwdt_write(priv, priv->cks, RWTCSRA);
> > +	val |= priv->cks;
> > +	rwdt_write(priv, val, RWTCSRA);
> 
> Have you tested this successfully? According to the docs, CKS bits are
> all 1 by default. So, your |= operation should be a NOP and we can't
> select a CKS value anymore if I am not mistaken.
> 
I tested and can confirm WOVFE was be disable by command 
rwdt_write(priv, priv->cks, RWTCSRA);
I don't understand why this bit is turned off but the watchdog can still reset, but 
according to the document it will be 1.

> >  	rwdt_write(priv, 0, RWTCSRB);
> >  
> >  	while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
> >  		cpu_relax();
> > -
> > -	rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA);
> > +	/* Enable interrupt and timer */
> > +	rwdt_write(priv, val | RWTCSRA_WOVFE | RWTCSRA_TME, RWTCSRA);
> 
> What is the use of enabling an interrupt without having an interrupt
> handler? (And I never understood why there is an interrupt for an
> overflowing watchdog. We won't have time to serve it, or am I
> overlooking something obvious?)

I have added the interrupt node to dtsi and created the interrupt handler to successfully handle the Secure watchdog Gen2, but this is not documented.  With Gen 3, I am also thinking whether it is necessary or not.  Thank you!!!
With Gen3, after reset by WDT, then restart will have an interrupt when probe timer(), but we can do this no reset, after this,  timer operate normally. 
Problaly this patch should RFC

Thank you for your helps!!!


> 
> Kind regards,
> 
>    Wolfram
>
Yoshihiro Shimoda May 24, 2019, 2:01 a.m. UTC | #4
Dear Hoan-san,

> From: "グェン・アン・ホァン", Sent: Friday, May 24, 2019 10:14 AM
> 
> Dear Wolfram-san
> Dear Geert- san
> 
> Thank you very much
> Wolfram Sang wrote:
> > Hi,
> >
> > On Thu, May 23, 2019 at 06:29:37PM +0900, Nguyen An Hoan wrote:
> > > From: Hoan Nguyen An <na-hoan@jinso.co.jp>
> > >
> > > Fix setting for bit WOVFE of RWTCSRA. Keep it enable follow hardware document.
> >
> > Hmm, I can't find it in the docs. Which version of the documentation do
> > you use?

Do you have any comment about this question?

> > > -	rwdt_write(priv, priv->cks, RWTCSRA);
> > > +	val |= priv->cks;
> > > +	rwdt_write(priv, val, RWTCSRA);
> >
> > Have you tested this successfully? According to the docs, CKS bits are
> > all 1 by default. So, your |= operation should be a NOP and we can't
> > select a CKS value anymore if I am not mistaken.
> >
> I tested and can confirm WOVFE was be disable by command
> rwdt_write(priv, priv->cks, RWTCSRA);
> I don't understand why this bit is turned off but the watchdog can still reset, but
> according to the document it will be 1.

Your answer doesn't have CKS bits things. As Wolfram-san mentioned, the default CKS bits
value is b'111 and then the code "val |= priv->cks" can be not set to the priv->cks value.
In other words, if the priv->cks is set to 0, the driver should set the CKS bits to 0,
but your code will be set to b'111.

> > >  	rwdt_write(priv, 0, RWTCSRB);
> > >
> > >  	while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
> > >  		cpu_relax();
> > > -
> > > -	rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA);
> > > +	/* Enable interrupt and timer */
> > > +	rwdt_write(priv, val | RWTCSRA_WOVFE | RWTCSRA_TME, RWTCSRA);
> >
> > What is the use of enabling an interrupt without having an interrupt
> > handler? (And I never understood why there is an interrupt for an
> > overflowing watchdog. We won't have time to serve it, or am I
> > overlooking something obvious?)
> 
> I have added the interrupt node to dtsi and created the interrupt handler to successfully handle the Secure watchdog Gen2,
> but this is not documented.  With Gen 3, I am also thinking whether it is necessary or not.  Thank you!!!
> With Gen3, after reset by WDT, then restart will have an interrupt when probe timer(), but we can do this no reset, after
> this,  timer operate normally.
> Problaly this patch should RFC

I'm afraid I could not understand these comments well, but if you have the interrupt handler for this renesas_wdt driver,
you should contain it on this patch :)  To be honest, I have no idea how to use the interrupt though.

Best regards,
Yoshihiro Shimoda

> Thank you for your helps!!!
> 
> 
> >
> > Kind regards,
> >
> >    Wolfram
> >

Patch
diff mbox series

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 565dbc1..a6aca0e 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -19,6 +19,7 @@ 
 
 #define RWTCNT		0
 #define RWTCSRA		4
+#define RWTCSRA_WOVFE	BIT(3)
 #define RWTCSRA_WOVF	BIT(4)
 #define RWTCSRA_WRFLG	BIT(5)
 #define RWTCSRA_TME	BIT(7)
@@ -82,13 +83,14 @@  static int rwdt_start(struct watchdog_device *wdev)
 	rwdt_write(priv, val, RWTCSRA);
 
 	rwdt_init_timeout(wdev);
-	rwdt_write(priv, priv->cks, RWTCSRA);
+	val |= priv->cks;
+	rwdt_write(priv, val, RWTCSRA);
 	rwdt_write(priv, 0, RWTCSRB);
 
 	while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
 		cpu_relax();
-
-	rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA);
+	/* Enable interrupt and timer */
+	rwdt_write(priv, val | RWTCSRA_WOVFE | RWTCSRA_TME, RWTCSRA);
 
 	return 0;
 }