lkml.org 
[lkml]   [2017]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] watchdog: sama5d4: Cache MR instead of a partial config
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

\
 
 \ /
  Last update: 2017-02-01 17:57    [W:0.065 / U:7.904 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site