watchdog: f71808e_wdt: fix F81866 bit operation
Subsystems (2)
PATCH 1/1 watchdog: f71808e_wdt: fix F81866 bit operation
Fix error bit operation in watchdog_start()
--- drivers/watchdog/f71808e_wdt.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
drivers/watchdog/f71808e_wdt.c +5 -6
@@ -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 :
Signature · 1 line
2.7.4
Greg KH
maintainerQuoted 6 lines · Ji-Ze Hong (Peter Hong)
Fix error bit operation in
watchdog_start()Signed-off-by Ji-Ze Hong (Peter Hong) <[email protected]>--- 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>
Guenter Roeck
maintainerQuoted 2 lines · Ji-Ze Hong (Peter Hong)
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:
Quoted 10 lines
Signed-off-by Ji-Ze Hong (Peter Hong) <[email protected]>--- drivers/watchdog/f71808e_wdt.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)drivers/watchdog/f71808e_wdt.c +0
-0
drivers/watchdog/f71808e_wdt.c static int watchdog_start(void) · line 392
Quoted 10 lines
drivers/watchdog/f71808e_wdt.c +3
-4@@ -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
Quoted 14 lines
drivers/watchdog/f71808e_wdt.c +2
-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 :
Guenter Roeck 於 2019/3/22 下午 09:06 寫道:
Quoted 5 lines · Ji-Ze Hong (Peter Hong)
Quoted 2 lines
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
Quoted 2 lines
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#.
Quoted 3 lines
Please add:
Fixes 14b24a88a3660 ("watchdog: f71808e_wdt: Add F81866 support")
OK, I'll add to v2
Quoted 9 lines
Quoted 9 lines
drivers/watchdog/f71808e_wdt.c +0
-0@@ -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));
Quoted 8 lines
Quoted 5 lines
-
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
Signature · 2 lines
With Best Regards, Peter Hong
Guenter Roeck
maintainerQuoted 48 lines · Ji-Ze Hong (Peter Hong)
Guenter Roeck 於 2019/3/22 下午 09:06 寫道:
Quoted 5 lines · Ji-Ze Hong (Peter Hong)
Quoted 2 lines
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
Quoted 2 lines
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#.
Quoted 3 lines
Please add:
Fixes 14b24a88a3660 ("watchdog: f71808e_wdt: Add F81866 support")OK, I'll add to v2
Quoted 9 lines
Quoted 9 lines
drivers/watchdog/f71808e_wdt.c +0
-0@@ -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));Quoted 8 lines
Quoted 5 lines
-
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.
Quoted 4 lines
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