Re: [PATCH v2] watchdog: f71808e_wdt: Add F81865 support

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

On 04/22/2016 07:47 AM, Knud Poulsen wrote:
>
>
> On 2016-04-22 16:19, Knud Poulsen wrote:
>>
>>
>> On 2016-04-22 15:59, Guenter Roeck wrote:
>>> On 04/22/2016 06:51 AM, Guenter Roeck wrote:
>>>> On 04/22/2016 04:48 AM, Knud Poulsen wrote:
>>>>> Adds watchdog support for Fintek F81865 Super-IO chip to
>>>>> Fintek wdt driver (f71808e_wdt)
>>>>>
>>>>> Tested and verified on Lanner LEC-3030 Industrial PC
>>>>>
>>>>> Datasheet references:
>>>>> http://www.hardwaresecrets.com/datasheets/F81865_V028P.pdf
>>>>> http://www.alldatasheet.com/datasheet-pdf/pdf/406317/FINTEK/F81865.html
>>>>>
>>>>> Signed-off-by: Knud Poulsen <knpo@ieee.org>
>>>>> ---
>>>>>    drivers/watchdog/f71808e_wdt.c | 31 +++++++++++++++++++++++++++----
>>>>>    1 file changed, 27 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
>>>>> index 016bd93..4133cd3 100644
>>>>> --- a/drivers/watchdog/f71808e_wdt.c
>>>>> +++ b/drivers/watchdog/f71808e_wdt.c
>>>>> @@ -59,6 +59,7 @@
>>>>>    #define SIO_F71869A_ID        0x1007    /* Chipset ID */
>>>>>    #define SIO_F71882_ID        0x0541    /* Chipset ID */
>>>>>    #define SIO_F71889_ID        0x0723    /* Chipset ID */
>>>>> +#define SIO_F81865_ID        0x0704    /* Chipset ID */
>>>>>
>>>>>    #define F71808FG_REG_WDO_CONF        0xf0
>>>>>    #define F71808FG_REG_WDT_CONF        0xf5
>>>>> @@ -71,6 +72,10 @@
>>>>>    #define F71808FG_FLAG_WD_PULSE        4
>>>>>    #define F71808FG_FLAG_WD_UNIT        3
>>>>>
>>>>> +#define F81865_REG_WDO_CONF        0xfa
>>>>> +#define F81865_FLAG_WDOUT_EN        0
>>>>> +#define F81865_FLAG_WDTMOUT_STS        6
>>>>> +
>>> [ ... ]
>>>
>>>>>
>>>>>        wdt_conf = superio_inb(sioaddr, F71808FG_REG_WDT_CONF);
>>>>> -    watchdog.caused_reboot = wdt_conf & F71808FG_FLAG_WDTMOUT_STS;
>>>>> +
>>>>> +    if (watchdog.type == f81865)
>>>>> +        watchdog.caused_reboot = wdt_conf & F81865_FLAG_WDTMOUT_STS;
>>>>> +    else
>>>>> +        watchdog.caused_reboot = wdt_conf & F71808FG_FLAG_WDTMOUT_STS;
>>>>>
>>>> Are you sure this one is correct ? The bit configuration is exactly the same
>>>> as with other chips supported by this driver. Actually, I don't see an indication
>>>> in any of the datasheets I have which would suggest that reading bit 5 means
>>>> that the last reboot occurred due to a watchdog timeout. As far as I can see,
>>>> it just means that the watchdog is enabled.
>>>>
>>>> Am I missing something here ?
>>>>
>>> After another look, I am even more confused. F81865_FLAG_WDTMOUT_STS and
>>> F71808FG_FLAG_WDTMOUT_STS are bit maps, not bits.
>>>
>>> 5 = 0b0101, 6 = 0b0110. Bit 0/1 select the pulse width, bit 2 selects
>>> the output polarity of RSTOUT. What does this have to do with the reboot cause ?
>>
>> You're right, great catch!!, I didn't grok the bit-map, not bit-index part here
>> and got thrown by the 6 vs 5 in the datasheet vs the existing code, *Doh*
>>
>> ps3 en route.
>
> Hi Guenter,
>
> The coin drops... ok, I now share your confusion 110%... let me trace and test
> this *a lot* more on Monday when I have access to the hardware...
>
> ps3 by ~mid next week...
>
No problem. Yes, testing this will be important.

Thanks,
Guenter