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

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

On 08/31/2018 05:29 PM, Marek Behun wrote:
> 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.

Excellent. Thanks for checking!

Guenter

> 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
> 
>