RE: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts

[Chris Brandt <Chris.Brandt@renesas.com>]

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