[PATCH V1 1/1] watchdog: f71808e_wdt: fix F81866 bit operation

[PATCH V1 1/1] watchdog: f71808e_wdt: fix F81866 bit operation

[Ji-Ze Hong (Peter Hong)]

Fix error bit operation in watchdog_start()

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/watchdog/f71808e_wdt.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
index 9a1c761258ce..9129485732c7 100644
--- a/drivers/watchdog/f71808e_wdt.c
+++ b/drivers/watchdog/f71808e_wdt.c
@@ -387,18 +387,17 @@ static int watchdog_start(void)
 
 	case f81866:
 		/* Set pin 70 to WDTRST# */
-		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
-				  BIT(3) | BIT(0));
-		superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
-				BIT(2));
+		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 3);
+		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 0);
+		superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 2);
+
 		/*
 		 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
 		 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
 		 *     BIT5: 0 -> WDTRST#
 		 *           1 -> GPIO15
 		 */
-		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_GPIO1,
-				  BIT(5));
+		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_GPIO1, 5);
 		break;
 
 	default:
-- 
2.7.4

Re: [PATCH V1 1/1] watchdog: f71808e_wdt: fix F81866 bit operation

[Greg KH]

On Fri, Mar 22, 2019 at 11:36:56AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Fix error bit operation in watchdog_start()
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/watchdog/f71808e_wdt.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH V1 1/1] watchdog: f71808e_wdt: fix F81866 bit operation

[Guenter Roeck]

On 3/21/19 8:36 PM, Ji-Ze Hong (Peter Hong) wrote:
> Fix error bit operation in watchdog_start()
> 

Hmm ... does that mean it never worked ? Did you test it this time ?

A secondary concern is that the watchdog doesn't _have_ to trigger WDTRST,
but may also trigger PWOK. But that may be a separate issue.

Please add:

Fixes: 14b24a88a3660 ("watchdog: f71808e_wdt: Add F81866 support")

> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
>   drivers/watchdog/f71808e_wdt.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
> index 9a1c761258ce..9129485732c7 100644
> --- a/drivers/watchdog/f71808e_wdt.c
> +++ b/drivers/watchdog/f71808e_wdt.c
> @@ -387,18 +387,17 @@ static int watchdog_start(void)
>   
>   	case f81866:
>   		/* Set pin 70 to WDTRST# */
> -		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
> -				  BIT(3) | BIT(0));
> -		superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
> -				BIT(2));
> +		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 3);
> +		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 0);
> +		superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 2);

Better use superio_inb()/superio_outb(). The above is (much) more expensive,
and  we have no real idea what the impact of changing one bit at a time may be.

Thanks,
Guenter

> +
>   		/*
>   		 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
>   		 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
>   		 *     BIT5: 0 -> WDTRST#
>   		 *           1 -> GPIO15
>   		 */
> -		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_GPIO1,
> -				  BIT(5));
> +		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_GPIO1, 5);
>   		break;
>   
>   	default:
> 

Re: [PATCH V1 1/1] watchdog: f71808e_wdt: fix F81866 bit operation

[Ji-Ze Hong (Peter Hong)]

Guenter Roeck 於 2019/3/22 下午 09:06 寫道:
> On 3/21/19 8:36 PM, Ji-Ze Hong (Peter Hong) wrote:
>> Fix error bit operation in watchdog_start()
>>
> 
> Hmm ... does that mean it never worked ? Did you test it this time ?

Sorry for lacking test procedure. I had only test the functional (reset)
, not to test the register value.

The F81866 PIN70 (WDTRST#/GPIO15) is default set to WDTRST# function and
the old code only change register 27h bit(4) - PORT_4E_EN.

If the mainboard entry port is 4Eh, the old code is equal to nothing
done, but when the mainboard entry port is 2Eh, this code will make the
change from entry port 2Eh to 4Eh.

https://html.alldatasheet.com/html-pdf/459086/FINTEK/F81866AD/26531/119/F81866AD.html

> A secondary concern is that the watchdog doesn't _have_ to trigger WDTRST,
> but may also trigger PWOK. But that may be a separate issue.

Out watchdog is only support WDTRST#.

> Please add:
> 
> Fixes: 14b24a88a3660 ("watchdog: f71808e_wdt: Add F81866 support")

OK, I'll add to v2

>> diff --git a/drivers/watchdog/f71808e_wdt.c 
>> b/drivers/watchdog/f71808e_wdt.c
>> index 9a1c761258ce..9129485732c7 100644
>> --- a/drivers/watchdog/f71808e_wdt.c
>> +++ b/drivers/watchdog/f71808e_wdt.c
>> @@ -387,18 +387,17 @@ static int watchdog_start(void)
>>       case f81866:
>>           /* Set pin 70 to WDTRST# */
>> -        superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
>> -                  BIT(3) | BIT(0));
>> -        superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
>> -                BIT(2));
>> +        superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 3);
>> +        superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 0);
>> +        superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 2);
> 
> Better use superio_inb()/superio_outb(). The above is (much) more 
> expensive,
> and  we have no real idea what the impact of changing one bit at a time 
> may be.

Could I add a superio_mask_write(reg, mask, data) with v2 patch like
following fintek driver ?
https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/8250/8250_fintek.c#L113

Thanks
-- 
With Best Regards,
Peter Hong

Re: [PATCH V1 1/1] watchdog: f71808e_wdt: fix F81866 bit operation

[Guenter Roeck]

On 3/24/19 11:10 PM, Ji-Ze Hong (Peter Hong) wrote:
> Guenter Roeck 於 2019/3/22 下午 09:06 寫道:
>> On 3/21/19 8:36 PM, Ji-Ze Hong (Peter Hong) wrote:
>>> Fix error bit operation in watchdog_start()
>>>
>>
>> Hmm ... does that mean it never worked ? Did you test it this time ?
> 
> Sorry for lacking test procedure. I had only test the functional (reset)
> , not to test the register value.
> 
> The F81866 PIN70 (WDTRST#/GPIO15) is default set to WDTRST# function and
> the old code only change register 27h bit(4) - PORT_4E_EN.
> 
> If the mainboard entry port is 4Eh, the old code is equal to nothing
> done, but when the mainboard entry port is 2Eh, this code will make the
> change from entry port 2Eh to 4Eh.
> 
> https://html.alldatasheet.com/html-pdf/459086/FINTEK/F81866AD/26531/119/F81866AD.html
> 
>> A secondary concern is that the watchdog doesn't _have_ to trigger WDTRST,
>> but may also trigger PWOK. But that may be a separate issue.
> 
> Out watchdog is only support WDTRST#.
> 
>> Please add:
>>
>> Fixes: 14b24a88a3660 ("watchdog: f71808e_wdt: Add F81866 support")
> 
> OK, I'll add to v2
> 
>>> diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
>>> index 9a1c761258ce..9129485732c7 100644
>>> --- a/drivers/watchdog/f71808e_wdt.c
>>> +++ b/drivers/watchdog/f71808e_wdt.c
>>> @@ -387,18 +387,17 @@ static int watchdog_start(void)
>>>       case f81866:
>>>           /* Set pin 70 to WDTRST# */
>>> -        superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
>>> -                  BIT(3) | BIT(0));
>>> -        superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
>>> -                BIT(2));
>>> +        superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 3);
>>> +        superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 0);
>>> +        superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 2);
>>
>> Better use superio_inb()/superio_outb(). The above is (much) more expensive,
>> and  we have no real idea what the impact of changing one bit at a time may be.
> 
> Could I add a superio_mask_write(reg, mask, data) with v2 patch like
> following fintek driver ?
> https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/8250/8250_fintek.c#L113
> 

If you wish. but not necessary. For the fix it would be better to have only the fix.
You could introduce the function in a second patch and use it wherever read/modify/write
of a mask is used in the driver (I think it was used at least in one other place).

Guenter