| Message ID | 20180907012243.86603-1-chris.brandt@renesas.com |
|---|---|
| Headers | show |
| Series | Add support for RZ/A2 wdt | expand |
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); >
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
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
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
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
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