Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Aug 30, 2018 at 08:50:04PM +0200, Marek Behun wrote:
> > > > +static u64 get_counter_value(struct armada_37xx_watchdog *dev)
> > > > +{
> > > > +	u64 val;
> > > > +
> > > > +	val = readl(dev->reg + CNTR_COUNT_HIGH);
> > > > +	val = (val << 32) | readl(dev->reg + CNTR_COUNT_LOW);    
> > > 
> > > Is this guaranteed to be consistent ? What happens if there is a
> > > 32-bit wrap between those two operations ?  
> > 
> > hmmm. The address is not divisible by 8, so I can't use readq :( what
> > do you propose?
> 
> What do you think of this solution?
> 
> u64 val;
> u32 low1, low2;
> 
> low1 = readl(dev->reg + CNTR_COUNT_LOW);
> val = readl(dev->reg + CNTR_COUNT_HIGH);
> low2 = readl(dev->reg + CNTR_COUNT_LOW);
> 
> /*
>  * If low jumped in this short time more than 2^31, a wrap probably
>  * occured. Read high again.
>  */
> if (low2 - low1 > 0x80000000)
> 	val = readl(dev->reg + CNTR_COUNT_HIGH);
> val = (val << 32) | low2;

Yes, that is one option. The other would be to read high again
all the time and repeat reading low if high changed on the second
read of high.

	high1 = readl(dev->reg + CNTR_COUNT_HIGH);
	low = readl(dev->reg + CNTR_COUNT_LOW);
	high2 = readl(dev->reg + CNTR_COUNT_HIGH);
	if (high2 != high1)
		low = readl(dev->reg + CNTR_COUNT_LOW);
	val = (high2 << 32) | low;

There is no ambiguity in this case: We _know_ that
a wrap occurred if high1 and high2 are different.

Guenter