| Message ID | 1553225816-24375-1-git-send-email-hpeter+linux_kernel@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [V1,1/1] watchdog: f71808e_wdt: fix F81866 bit operation | expand |
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>
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: >
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
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
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:
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(-)