* [PATCH v3 0/2] Add support for RZ/A2 wdt @ 2018-09-07 1:22 Chris Brandt 2018-09-07 1:22 ` [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts Chris Brandt 2018-09-07 1:22 ` [PATCH v3 2/2] dt-bindings: watchdog: renesas-wdt: Add support for R7S9210 Chris Brandt 0 siblings, 2 replies; 13+ messages in thread From: Chris Brandt @ 2018-09-07 1:22 UTC (permalink / raw) To: Wim Van Sebroeck, Guenter Roeck, Rob Herring, Mark Rutland, Geert Uytterhoeven Cc: linux-watchdog, devicetree, linux-renesas-soc, Simon Horman, Chris Brandt Slightly modify the rza_wdt.c driver and update the binding docs. Chris Brandt (2): watchdog: rza_wdt: Support longer timeouts dt-bindings: watchdog: renesas-wdt: Add support for R7S9210 .../devicetree/bindings/watchdog/renesas-wdt.txt | 1 + drivers/watchdog/rza_wdt.c | 81 +++++++++++++++++----- 2 files changed, 64 insertions(+), 18 deletions(-) -- 2.16.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts 2018-09-07 1:22 [PATCH v3 0/2] Add support for RZ/A2 wdt Chris Brandt @ 2018-09-07 1:22 ` Chris Brandt 2018-09-08 16:10 ` Guenter Roeck 2018-09-07 1:22 ` [PATCH v3 2/2] dt-bindings: watchdog: renesas-wdt: Add support for R7S9210 Chris Brandt 1 sibling, 1 reply; 13+ messages in thread From: Chris Brandt @ 2018-09-07 1:22 UTC (permalink / raw) To: Wim Van Sebroeck, Guenter Roeck, Rob Herring, Mark Rutland, Geert Uytterhoeven Cc: linux-watchdog, devicetree, linux-renesas-soc, Simon Horman, Chris Brandt The RZ/A2 watchdog timer extends the clock source options in order to allow for longer timeouts. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- 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 <linux/delay.h> #include <linux/module.h> #include <linux/of_address.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/watchdog.h> @@ -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) +{ + unsigned long rate = clk_get_rate(priv->clk); + unsigned int ticks; + + if (priv->cks == CKS_4BIT) { + ticks = DIV_ROUND_UP((timeout * rate), 4194304); + if (ticks > 256) + ticks = 256; + + 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); + + 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 = &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); + priv->cks = (unsigned int)of_device_get_match_data(&pdev->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(&pdev->dev, "max hw timeout of %dms\n", + priv->wdev.max_hw_heartbeat_ms); + } else { + dev_err(&pdev->dev, "invalid CKS value (%u)\n", priv->cks); + return -EINVAL; + } priv->wdev.min_timeout = 1; priv->wdev.timeout = DEFAULT_TIMEOUT; @@ -179,7 +223,8 @@ static int rza_wdt_probe(struct platform_device *pdev) } static const struct of_device_id rza_wdt_of_match[] = { - { .compatible = "renesas,rza-wdt", }, + { .compatible = "renesas,r7s9210-wdt", .data = (void *)CKS_4BIT, }, + { .compatible = "renesas,rza-wdt", .data = (void *)CKS_3BIT, }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, rza_wdt_of_match); -- 2.16.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts 2018-09-07 1:22 ` [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts Chris Brandt @ 2018-09-08 16:10 ` Guenter Roeck 2018-09-10 13:53 ` Chris Brandt 0 siblings, 1 reply; 13+ messages in thread From: Guenter Roeck @ 2018-09-08 16:10 UTC (permalink / raw) To: Chris Brandt, Wim Van Sebroeck, Rob Herring, Mark Rutland, Geert Uytterhoeven Cc: linux-watchdog, devicetree, linux-renesas-soc, Simon Horman 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 <chris.brandt@renesas.com> > --- > 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 <linux/delay.h> > #include <linux/module.h> > #include <linux/of_address.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/watchdog.h> > > @@ -34,12 +35,40 @@ > #define WRCSR_RSTE BIT(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 0x400000 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 = &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); > + priv->cks = (unsigned int)of_device_get_match_data(&pdev->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(&pdev->dev, "max hw timeout of %dms\n", > + priv->wdev.max_hw_heartbeat_ms); > + } else { > + dev_err(&pdev->dev, "invalid CKS value (%u)\n", priv->cks); > + return -EINVAL; > + } I don't really see the point of this else statement. It is pretty much dead code, and the message is quite useless for the user. > > priv->wdev.min_timeout = 1; > priv->wdev.timeout = DEFAULT_TIMEOUT; > @@ -179,7 +223,8 @@ static int rza_wdt_probe(struct platform_device *pdev) > } > > static const struct of_device_id rza_wdt_of_match[] = { > - { .compatible = "renesas,rza-wdt", }, > + { .compatible = "renesas,r7s9210-wdt", .data = (void *)CKS_4BIT, }, > + { .compatible = "renesas,rza-wdt", .data = (void *)CKS_3BIT, }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, rza_wdt_of_match); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts 2018-09-08 16:10 ` Guenter Roeck @ 2018-09-10 13:53 ` Chris Brandt 2018-09-10 13:53 ` Chris Brandt 2018-09-10 16:13 ` Guenter Roeck 0 siblings, 2 replies; 13+ messages in thread From: Chris Brandt @ 2018-09-10 13:53 UTC (permalink / raw) To: Guenter Roeck, Wim Van Sebroeck, Rob Herring, Mark Rutland, Geert Uytterhoeven Cc: linux-watchdog, devicetree, linux-renesas-soc, Simon Horman 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(&pdev->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. > > 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. 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). > > + 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 0x400000 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, 0x400000 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. > > + 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. > > + 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 (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(&pdev->dev, "max hw timeout of %dms\n", > > + priv->wdev.max_hw_heartbeat_ms); > > + } else { > > + dev_err(&pdev->dev, "invalid CKS value (%u)\n", priv->cks); > > + return -EINVAL; > > + } > > I don't really see the point of this else statement. It is pretty much > dead code, > and the message is quite useless for the user. Good point. The only time you would hit this is if you were modifying the driver. And in that case, you are not a "user". I will delete the else. Thank you for your review. Chris ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts 2018-09-10 13:53 ` Chris Brandt @ 2018-09-10 13:53 ` Chris Brandt 2018-09-10 16:13 ` Guenter Roeck 1 sibling, 0 replies; 13+ messages in thread From: Chris Brandt @ 2018-09-10 13:53 UTC (permalink / raw) To: Guenter Roeck, Wim Van Sebroeck, Rob Herring, Mark Rutland, Geert Uytterhoeven Cc: linux-watchdog, devicetree, linux-renesas-soc, Simon Horman 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(&pdev->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. > > 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. 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). > > + 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 0x400000 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, 0x400000 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. > > + 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. > > + 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 (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(&pdev->dev, "max hw timeout of %dms\n", > > + priv->wdev.max_hw_heartbeat_ms); > > + } else { > > + dev_err(&pdev->dev, "invalid CKS value (%u)\n", priv->cks); > > + return -EINVAL; > > + } > > I don't really see the point of this else statement. It is pretty much > dead code, > and the message is quite useless for the user. Good point. The only time you would hit this is if you were modifying the driver. And in that case, you are not a "user". I will delete the else. Thank you for your review. Chris ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts 2018-09-10 13:53 ` Chris Brandt 2018-09-10 13:53 ` Chris Brandt @ 2018-09-10 16:13 ` Guenter Roeck 2018-09-10 17:36 ` Chris Brandt 1 sibling, 1 reply; 13+ messages in thread From: Guenter Roeck @ 2018-09-10 16:13 UTC (permalink / raw) To: Chris Brandt Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Geert Uytterhoeven, linux-watchdog, devicetree, linux-renesas-soc, Simon Horman On Mon, Sep 10, 2018 at 01:53:28PM +0000, 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(&pdev->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 0x400000 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, 0x400000 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts 2018-09-10 16:13 ` Guenter Roeck @ 2018-09-10 17:36 ` Chris Brandt 2018-09-10 17:36 ` Chris Brandt 2018-09-10 17:59 ` Guenter Roeck 0 siblings, 2 replies; 13+ messages in thread From: Chris Brandt @ 2018-09-10 17:36 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Geert Uytterhoeven, linux-watchdog, devicetree, linux-renesas-soc, Simon Horman 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. 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, 0x400000 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, }; If you really don't like checking in .ping, I can add a set_timeout function and remove the local priv->timeout. Chris ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts 2018-09-10 17:36 ` Chris Brandt @ 2018-09-10 17:36 ` Chris Brandt 2018-09-10 17:59 ` Guenter Roeck 1 sibling, 0 replies; 13+ messages in thread From: Chris Brandt @ 2018-09-10 17:36 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Geert Uytterhoeven, linux-watchdog, devicetree, linux-renesas-soc, Simon Horman 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. 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, 0x400000 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, }; If you really don't like checking in .ping, I can add a set_timeout function and remove the local priv->timeout. Chris ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts 2018-09-10 17:36 ` Chris Brandt 2018-09-10 17:36 ` Chris Brandt @ 2018-09-10 17:59 ` Guenter Roeck 2018-09-10 18:15 ` Chris Brandt 1 sibling, 1 reply; 13+ messages in thread From: Guenter Roeck @ 2018-09-10 17:59 UTC (permalink / raw) To: Chris Brandt Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Geert Uytterhoeven, linux-watchdog, devicetree, linux-renesas-soc, Simon Horman On Mon, Sep 10, 2018 at 05:36:39PM +0000, 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, 0x400000 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts 2018-09-10 17:59 ` Guenter Roeck @ 2018-09-10 18:15 ` Chris Brandt 2018-09-10 18:15 ` Chris Brandt 0 siblings, 1 reply; 13+ messages in thread From: Chris Brandt @ 2018-09-10 18:15 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Geert Uytterhoeven, linux-watchdog, devicetree, linux-renesas-soc, Simon Horman On Monday, September 10, 2018, Guenter Roeck wrote: > > 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. OK. I will add a set_timeout and resubmit. Chris ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts 2018-09-10 18:15 ` Chris Brandt @ 2018-09-10 18:15 ` Chris Brandt 0 siblings, 0 replies; 13+ messages in thread From: Chris Brandt @ 2018-09-10 18:15 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Geert Uytterhoeven, linux-watchdog, devicetree, linux-renesas-soc, Simon Horman On Monday, September 10, 2018, Guenter Roeck wrote: > > 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. OK. I will add a set_timeout and resubmit. Chris ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 2/2] dt-bindings: watchdog: renesas-wdt: Add support for R7S9210 2018-09-07 1:22 [PATCH v3 0/2] Add support for RZ/A2 wdt Chris Brandt 2018-09-07 1:22 ` [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts Chris Brandt @ 2018-09-07 1:22 ` Chris Brandt 2018-09-10 20:49 ` Rob Herring 1 sibling, 1 reply; 13+ messages in thread From: Chris Brandt @ 2018-09-07 1:22 UTC (permalink / raw) To: Wim Van Sebroeck, Guenter Roeck, Rob Herring, Mark Rutland, Geert Uytterhoeven Cc: linux-watchdog, devicetree, linux-renesas-soc, Simon Horman, Chris Brandt Document support for RZ/A2 Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- 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 -- 2.16.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: watchdog: renesas-wdt: Add support for R7S9210 2018-09-07 1:22 ` [PATCH v3 2/2] dt-bindings: watchdog: renesas-wdt: Add support for R7S9210 Chris Brandt @ 2018-09-10 20:49 ` Rob Herring 0 siblings, 0 replies; 13+ messages in thread From: Rob Herring @ 2018-09-10 20:49 UTC (permalink / raw) To: Chris Brandt Cc: Wim Van Sebroeck, Guenter Roeck, Mark Rutland, Geert Uytterhoeven, linux-watchdog, devicetree, linux-renesas-soc, Simon Horman, Chris Brandt On Thu, 6 Sep 2018 20:22:43 -0500, Chris Brandt wrote: > Document support for RZ/A2 > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 1 + > 1 file changed, 1 insertion(+) > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-09-10 23:11 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-07 1:22 [PATCH v3 0/2] Add support for RZ/A2 wdt Chris Brandt 2018-09-07 1:22 ` [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts Chris Brandt 2018-09-08 16:10 ` Guenter Roeck 2018-09-10 13:53 ` Chris Brandt 2018-09-10 13:53 ` Chris Brandt 2018-09-10 16:13 ` Guenter Roeck 2018-09-10 17:36 ` Chris Brandt 2018-09-10 17:36 ` Chris Brandt 2018-09-10 17:59 ` Guenter Roeck 2018-09-10 18:15 ` Chris Brandt 2018-09-10 18:15 ` Chris Brandt 2018-09-07 1:22 ` [PATCH v3 2/2] dt-bindings: watchdog: renesas-wdt: Add support for R7S9210 Chris Brandt 2018-09-10 20:49 ` Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).