| Message ID | 20180905173743.47938-1-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | watchdog: rza_wdt: Extend clock sources for RZ/A2 | expand |
Hi Chris, On Wed, Sep 5, 2018 at 7:40 PM Chris Brandt <chris.brandt@renesas.com> wrote: > The RZ/A2 watchdog timer extends the clock source options in order to give > you longer timeout options. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Thanks for your patch! > --- a/drivers/watchdog/rza_wdt.c > +++ b/drivers/watchdog/rza_wdt.c > @@ -38,8 +38,37 @@ struct rza_wdt { > struct watchdog_device wdev; > void __iomem *base; > struct clk *clk; > + bool ext_cks; > + 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); > + u16 counter; > + > + if (priv->ext_cks) { > + /* Add the 1 to deal with the inevitable fraction */ > + counter = ((timeout * rate) / 4194304) + 1; "timeout * rate" may overflow DIV_ROUND_UP()? > + if (counter > 255) > + counter = 0; > + > + priv->cks = 0xF; > + priv->count = 256 - counter; > + } else { > + /* Start timer with longest timeout */ > + priv->cks = 7; > + priv->count = 0; > + } > + > + priv->timeout = timeout; > + > + pr_debug("%s: timeout set to %d (WTCNT=%d)\n", __func__, %u for unsigned. > + timeout, priv->count); > +} > @@ -150,20 +184,29 @@ static int rza_wdt_probe(struct platform_device *pdev) > return -ENOENT; > } > > - /* Assume slowest clock rate possible (CKS=7) */ > - rate /= 16384; > + if (of_device_is_compatible(np, "renesas,r7s9210-wdt")) > + priv->ext_cks = true; That's not the proper way to support multiple devices. Please add an entry for "renesas,r7s9210-wdt" to rza_wdt_of_match[]. "renesas,r7s9210-wdt" must be documented in the DT bindings. I suggest storing cks in rza_wdt_of_match[].data, and retrieving it with of_device_get_match_data() in your probe function, so you can use that to differentiate, and get rid of the ext_cks flag. BTW, is the RZ/A2 WDT compatible with the RZ/A1 WDT, i.e. does it work with the unmodified driver? If not, "renesas,rza-wdt" must not be used as a fallback. Gr{oetje,eeting}s, Geert
Hi Geert, On Thursday, September 06, 2018, Geert Uytterhoeven wrote: > Thanks for your patch! Thank you for your review. > "timeout * rate" may overflow > DIV_ROUND_UP()? OK. > > + pr_debug("%s: timeout set to %d (WTCNT=%d)\n", __func__, > > %u for unsigned. OK. > > + if (of_device_is_compatible(np, "renesas,r7s9210-wdt")) > > + priv->ext_cks = true; > > That's not the proper way to support multiple devices. > Please add an entry for "renesas,r7s9210-wdt" to rza_wdt_of_match[]. > "renesas,r7s9210-wdt" must be documented in the DT bindings. > > I suggest storing cks in rza_wdt_of_match[].data, and retrieving it with > of_device_get_match_data() in your probe function, so you can use that to > differentiate, OK, I will change to that. > and get rid of the ext_cks flag. I still need to keep track if it's a RZ/A1 or RZ/A2 (ext_cks) for functions outside of probe (rza_wdt_calc_timeout). > BTW, is the RZ/A2 WDT compatible with the RZ/A1 WDT, i.e. does it work > with the unmodified driver? If not, "renesas,rza-wdt" must not be used as > a > fallback. I would say it is not, because the dividers are different (meaning the calculated timeouts would not be correct). Thanks, Chris
Hi Chris, On Thu, Sep 6, 2018 at 2:44 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Thursday, September 06, 2018, Geert Uytterhoeven wrote: > > > + if (of_device_is_compatible(np, "renesas,r7s9210-wdt")) > > > + priv->ext_cks = true; > > > > That's not the proper way to support multiple devices. > > Please add an entry for "renesas,r7s9210-wdt" to rza_wdt_of_match[]. > > "renesas,r7s9210-wdt" must be documented in the DT bindings. > > > > I suggest storing cks in rza_wdt_of_match[].data, and retrieving it with > > of_device_get_match_data() in your probe function, so you can use that to > > differentiate, > > OK, I will change to that. > > > and get rid of the ext_cks flag. > > I still need to keep track if it's a RZ/A1 or RZ/A2 (ext_cks) for > functions outside of probe (rza_wdt_calc_timeout). You can check if priv->cks > 7? Gr{oetje,eeting}s, Geert
Hi Geert, On Thursday, September 06, 2018, Geert Uytterhoeven wrote: > > > and get rid of the ext_cks flag. > > > > I still need to keep track if it's a RZ/A1 or RZ/A2 (ext_cks) for > > functions outside of probe (rza_wdt_calc_timeout). > > You can check if priv->cks > 7? That's a good point! I'll do that. Thanks, Chris
Hi Geert, Correction: On Thursday, September 06, 2018, Chris Brandt wrote: > > BTW, is the RZ/A2 WDT compatible with the RZ/A1 WDT, i.e. does it work > > with the unmodified driver? If not, "renesas,rza-wdt" must not be used > as > > a > > fallback. > > I would say it is not, because the dividers are different (meaning the > calculated timeouts would not be correct). Actually, the clock dividers are the same for the first three bits between RZ/A1 and RZ/A2. So you could use "renesas,rza-wdt" with an RZ/A2 and it would work exactly the same as RZ/A1. Chris
diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c index e618218d2374..78e0d61519ec 100644 --- a/drivers/watchdog/rza_wdt.c +++ b/drivers/watchdog/rza_wdt.c @@ -38,8 +38,37 @@ struct rza_wdt { struct watchdog_device wdev; void __iomem *base; struct clk *clk; + bool ext_cks; + 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); + u16 counter; + + if (priv->ext_cks) { + /* Add the 1 to deal with the inevitable fraction */ + counter = ((timeout * rate) / 4194304) + 1; + if (counter > 255) + counter = 0; + + priv->cks = 0xF; + priv->count = 256 - counter; + } else { + /* Start timer with longest timeout */ + priv->cks = 7; + priv->count = 0; + } + + priv->timeout = timeout; + + pr_debug("%s: timeout set to %d (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 = %d\n", __func__, wdev->timeout); return 0; } @@ -130,6 +163,7 @@ static int rza_wdt_probe(struct platform_device *pdev) struct resource *res; unsigned long rate; int ret; + struct device_node *np = pdev->dev.of_node; priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); if (!priv) @@ -150,20 +184,29 @@ static int rza_wdt_probe(struct platform_device *pdev) return -ENOENT; } - /* Assume slowest clock rate possible (CKS=7) */ - rate /= 16384; + if (of_device_is_compatible(np, "renesas,r7s9210-wdt")) + priv->ext_cks = true; priv->wdev.info = &rza_wdt_ident, priv->wdev.ops = &rza_wdt_ops, priv->wdev.parent = &pdev->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(&pdev->dev, "max hw timeout of %dms\n", - priv->wdev.max_hw_heartbeat_ms); + if (priv->ext_cks) { + /* Assume slowest clock rate possible (CKS=0xF) */ + priv->wdev.max_timeout = (4194304 * U8_MAX) / rate; + } else { + /* 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(&pdev->dev, "max hw timeout of %dms\n", + priv->wdev.max_hw_heartbeat_ms); + } priv->wdev.min_timeout = 1; priv->wdev.timeout = DEFAULT_TIMEOUT;
The RZ/A2 watchdog timer extends the clock source options in order to give you longer timeout options. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- drivers/watchdog/rza_wdt.c | 75 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 59 insertions(+), 16 deletions(-)