Re: [PATCH 1/2] watchdog: sama5d4: Cache MR instead of a partial config

[Alexandre Belloni]

On 30/01/2017 at 09:58:13 -0800, Guenter Roeck wrote:
> On Mon, Jan 30, 2017 at 06:18:47PM +0100, Alexandre Belloni wrote:
> > .config is used to cache a part of WDT_MR at probe time and is not used
> > afterwards. Instead of doing that, actually cache MR and avoid reading it
> > every time it is modified.
> > 
> The semantic change here is that the old code leaves "other" bits in the
> mr register alone. I assume you have verified that clearing those bits
> does not have negative impact.
> 

It doesn't have any impact unless you expect to restart a watchdog
exactly were you stopped it.

> Also, I am not really sure if replacing a read from a register is more
> costly than a read from memory. Is that change really worth it ?
> I mean, sure, the way the 'config' variable is used is a bit odd,
> but I don't really see the improvement in its replacement either.
> 

It is difficult to say performance wise (I doubt the sama5d4_wdt
structure stays long enough in the cache). But it allows to avoid
reading mr in the suspend function in 2/2.

> > @@ -132,19 +126,19 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
> >  {
> >  	const char *tmp;
> >  
> > -	wdt->config = AT91_WDT_WDDIS;
> > +	wdt->mr = AT91_WDT_WDDIS;
> >  
> >  	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> >  	    !strcmp(tmp, "software"))
> > -		wdt->config |= AT91_WDT_WDFIEN;
> > +		wdt->mr |= AT91_WDT_WDFIEN;
> >  	else
> > -		wdt->config |= AT91_WDT_WDRSTEN;
> > +		wdt->mr |= AT91_WDT_WDRSTEN;
> >  
> >  	if (of_property_read_bool(np, "atmel,idle-halt"))
> > -		wdt->config |= AT91_WDT_WDIDLEHLT;
> > +		wdt->mr |= AT91_WDT_WDIDLEHLT;
> >  
> >  	if (of_property_read_bool(np, "atmel,dbg-halt"))
> > -		wdt->config |= AT91_WDT_WDDBGHLT;
> > +		wdt->mr |= AT91_WDT_WDDBGHLT;
> >  
> >  	return 0;
> >  }
> > @@ -163,11 +157,10 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> >  	reg &= ~AT91_WDT_WDDIS;
> >  	wdt_write(wdt, AT91_WDT_MR, reg);
> >  
> There is (at least) one read of the mr register left above this code.
> It is not entirely obvious to me why it would make sense to retain
> this single read instead of just clearing the AT91_WDT_WDDIS bit
> from the cached value and writing the rest. Maybe this is to make
> sure that WDV or WDD isn't changed with the bit set, but ....
> the explanation associated with clearing the bit is a bit odd either
> case. It states that AT91_WDT_WDDIS must not be set (meaning the watchdog
> must be running ? Shouldn't it be the opposite ?) when changing WDV or WDD,
> then violates this rule when updating the timeout (which can happen with
> the watchdog running or not).

The datasheet states:
Note: When setting the WDDIS bit, and while it is set, the fields WDV
and WDD must not be modified.

And indeed, this may be an issue in sama5d4_wdt_set_timeout(). That is
something I needed to clarify and forgot about but I think that can be
solved in a separate patch.

If you prefer I can simply remove the weird wdt->config usage, keeping
the register accesses and then rework 2/2 in consequence.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com