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

[Marek Behun <marek.behun@nic.cz>]

Hello Guenter,
I just read in the specification that software does not need to
do debouncing, because when low is read, high is latched into 32 bit
flip flop so that it can be read correctly.
So the correct way is to read low first, then high, and that's it.
I shall write a comment describing this into new code.
Marek

On Thu, 30 Aug 2018 12:12:26 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> 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