| Message ID | 20210427084113.9051-1-Meng.Li@windriver.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | [v2,1/2] driver: watchdog: reset watchdog in designware_wdt_stop() function | expand |
On 27.04.21 10:41, Meng.Li@windriver.com wrote: > From: MengLi <meng.li@windriver.com> > > In uboot command line environment, watchdog is not able to be > stopped with below commands: > SOCFPGA_STRATIX10 # wdt dev watchdog@ffd00200 > SOCFPGA_STRATIX10 # wdt stop > Refer to watchdog driver in linux kernel, it is also need to reset > watchdog after disable it so that the disable action takes effect. > > v2: > Change "#if CONFIG_IS_ENABLED(DM_RESET)" into > "if (CONFIG_IS_ENABLED(DM_RESET)) {", and define the variable > into if condition sentence. A few comments: This version changelog belongs below the "---" line. Please Cc interested people upon new versions, e.g. myself as I reviewed this patch. Other that this: Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > Signed-off-by: Meng Li <Meng.Li@windriver.com> > --- > drivers/watchdog/designware_wdt.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/watchdog/designware_wdt.c b/drivers/watchdog/designware_wdt.c > index 12f09a7a39..57cad1effc 100644 > --- a/drivers/watchdog/designware_wdt.c > +++ b/drivers/watchdog/designware_wdt.c > @@ -96,6 +96,23 @@ static int designware_wdt_stop(struct udevice *dev) > designware_wdt_reset(dev); > writel(0, priv->base + DW_WDT_CR); > > + if (CONFIG_IS_ENABLED(DM_RESET)) { > + struct reset_ctl_bulk resets; > + int ret; > + > + ret = reset_get_bulk(dev, &resets); > + if (ret) > + return ret; > + > + ret = reset_assert_bulk(&resets); > + if (ret) > + return ret; > + > + ret = reset_deassert_bulk(&resets); > + if (ret) > + return ret; > + } > + > return 0; > } > > Viele Grüße, Stefan
On 4/27/21 10:23 AM, Stefan Roese wrote: > On 27.04.21 10:41, Meng.Li@windriver.com wrote: >> From: MengLi <meng.li@windriver.com> >> >> In uboot command line environment, watchdog is not able to be >> stopped with below commands: >> SOCFPGA_STRATIX10 # wdt dev watchdog@ffd00200 >> SOCFPGA_STRATIX10 # wdt stop >> Refer to watchdog driver in linux kernel, it is also need to reset >> watchdog after disable it so that the disable action takes effect. >> >> v2: >> Change "#if CONFIG_IS_ENABLED(DM_RESET)" into >> "if (CONFIG_IS_ENABLED(DM_RESET)) {", and define the variable >> into if condition sentence. > > A few comments: > > This version changelog belongs below the "---" line. > > Please Cc interested people upon new versions, e.g. myself as I reviewed > this patch. > > Other that this: > > Reviewed-by: Stefan Roese <sr@denx.de> > > Thanks, > Stefan > >> Signed-off-by: Meng Li <Meng.Li@windriver.com> >> --- >> drivers/watchdog/designware_wdt.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/drivers/watchdog/designware_wdt.c b/drivers/watchdog/designware_wdt.c >> index 12f09a7a39..57cad1effc 100644 >> --- a/drivers/watchdog/designware_wdt.c >> +++ b/drivers/watchdog/designware_wdt.c >> @@ -96,6 +96,23 @@ static int designware_wdt_stop(struct udevice *dev) >> designware_wdt_reset(dev); >> writel(0, priv->base + DW_WDT_CR); >> + if (CONFIG_IS_ENABLED(DM_RESET)) { >> + struct reset_ctl_bulk resets; >> + int ret; >> + >> + ret = reset_get_bulk(dev, &resets); Have you considered adding the resets to designware_wdt_priv and saving them when we request them in probe()? --Sean >> + if (ret) >> + return ret; >> + >> + ret = reset_assert_bulk(&resets); >> + if (ret) >> + return ret; >> + >> + ret = reset_deassert_bulk(&resets); >> + if (ret) >> + return ret; >> + } >> + >> return 0; >> } >> > > > Viele Grüße, > Stefan >
> -----Original Message----- > From: Sean Anderson <sean.anderson@seco.com> > Sent: Tuesday, April 27, 2021 10:50 PM > To: Stefan Roese <sr@denx.de>; Li, Meng <Meng.Li@windriver.com>; u- > boot@lists.denx.de; chin.liang.see@intel.com; dinh.nguyen@intel.com; > sjg@chromium.org > Subject: Re: [PATCH v2, 1/2] driver: watchdog: reset watchdog in > designware_wdt_stop() function > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On 4/27/21 10:23 AM, Stefan Roese wrote: > > On 27.04.21 10:41, Meng.Li@windriver.com wrote: > >> From: MengLi <meng.li@windriver.com> >> >> In uboot command line > environment, watchdog is not able to be >> stopped with below commands: > >> SOCFPGA_STRATIX10 # wdt dev watchdog@ffd00200 >> > SOCFPGA_STRATIX10 # wdt stop >> Refer to watchdog driver in linux kernel, > it is also need to reset >> watchdog after disable it so that the disable action > takes effect. > >> > >> v2: > >> Change "#if CONFIG_IS_ENABLED(DM_RESET)" into >> "if > (CONFIG_IS_ENABLED(DM_RESET)) {", and define the variable >> into if > condition sentence. > > > > A few comments: > > > > This version changelog belongs below the "---" line. > > > > Please Cc interested people upon new versions, e.g. myself as I reviewed > > this patch. > > > > Other that this: > > > > Reviewed-by: Stefan Roese <sr@denx.de> > > Thanks, > Stefan > >> > Signed-off-by: Meng Li <Meng.Li@windriver.com> >> --- > >> drivers/watchdog/designware_wdt.c | 17 +++++++++++++++++ > >> 1 file changed, 17 insertions(+) > >> > >> diff --git a/drivers/watchdog/designware_wdt.c > b/drivers/watchdog/designware_wdt.c > >> index 12f09a7a39..57cad1effc 100644 > >> --- a/drivers/watchdog/designware_wdt.c > >> +++ b/drivers/watchdog/designware_wdt.c > >> @@ -96,6 +96,23 @@ static int designware_wdt_stop(struct udevice > *dev) > >> designware_wdt_reset(dev); > >> writel(0, priv->base + DW_WDT_CR); > >> + if (CONFIG_IS_ENABLED(DM_RESET)) { > >> + struct reset_ctl_bulk resets; > >> + int ret; > >> + > >> + ret = reset_get_bulk(dev, &resets); > > Have you considered adding the resets to designware_wdt_priv and saving > them when we request them in probe()? > Yes! thanks for reminding me. But I want to fix this issue by modifying the minimum range code. Because I and not the original person of creating this driver. I don't want to other part of code if it is not essential. Thanks, Limeng > --Sean > > >> + if (ret) > >> + return ret; > >> + > >> + ret = reset_assert_bulk(&resets); > >> + if (ret) > >> + return ret; > >> + > >> + ret = reset_deassert_bulk(&resets); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> return 0; > >> } > >> > > > > > > Viele Grüße, > > Stefan > >
> -----Original Message----- > From: Stefan Roese <sr@denx.de> > Sent: Tuesday, April 27, 2021 10:23 PM > To: Li, Meng <Meng.Li@windriver.com>; u-boot@lists.denx.de; > chin.liang.see@intel.com; dinh.nguyen@intel.com; sjg@chromium.org > Subject: Re: [PATCH v2, 1/2] driver: watchdog: reset watchdog in > designware_wdt_stop() function > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On 27.04.21 10:41, Meng.Li@windriver.com wrote: > > From: MengLi <meng.li@windriver.com> > > > > In uboot command line environment, watchdog is not able to be stopped > > with below commands: > > SOCFPGA_STRATIX10 # wdt dev watchdog@ffd00200 > > SOCFPGA_STRATIX10 # wdt stop > > Refer to watchdog driver in linux kernel, it is also need to reset > > watchdog after disable it so that the disable action takes effect. > > > > v2: > > Change "#if CONFIG_IS_ENABLED(DM_RESET)" into "if > > (CONFIG_IS_ENABLED(DM_RESET)) {", and define the variable into if > > condition sentence. > > A few comments: > > This version changelog belongs below the "---" line. > > Please Cc interested people upon new versions, e.g. myself as I reviewed > this patch. > > Other that this: > > Reviewed-by: Stefan Roese <sr@denx.de> > Would you like me to add "Cc:" into comment log, too? Or only CC to you in mail list? Thanks, Limeng > Thanks, > Stefan > > > Signed-off-by: Meng Li <Meng.Li@windriver.com> > > --- > > drivers/watchdog/designware_wdt.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/drivers/watchdog/designware_wdt.c > > b/drivers/watchdog/designware_wdt.c > > index 12f09a7a39..57cad1effc 100644 > > --- a/drivers/watchdog/designware_wdt.c > > +++ b/drivers/watchdog/designware_wdt.c > > @@ -96,6 +96,23 @@ static int designware_wdt_stop(struct udevice *dev) > > designware_wdt_reset(dev); > > writel(0, priv->base + DW_WDT_CR); > > > > + if (CONFIG_IS_ENABLED(DM_RESET)) { > > + struct reset_ctl_bulk resets; > > + int ret; > > + > > + ret = reset_get_bulk(dev, &resets); > > + if (ret) > > + return ret; > > + > > + ret = reset_assert_bulk(&resets); > > + if (ret) > > + return ret; > > + > > + ret = reset_deassert_bulk(&resets); > > + if (ret) > > + return ret; > > + } > > + > > return 0; > > } > > > > > > > Viele Grüße, > Stefan > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
On 28.04.21 04:15, Li, Meng wrote: > > >> -----Original Message----- >> From: Stefan Roese <sr@denx.de> >> Sent: Tuesday, April 27, 2021 10:23 PM >> To: Li, Meng <Meng.Li@windriver.com>; u-boot@lists.denx.de; >> chin.liang.see@intel.com; dinh.nguyen@intel.com; sjg@chromium.org >> Subject: Re: [PATCH v2, 1/2] driver: watchdog: reset watchdog in >> designware_wdt_stop() function >> >> [Please note: This e-mail is from an EXTERNAL e-mail address] >> >> On 27.04.21 10:41, Meng.Li@windriver.com wrote: >>> From: MengLi <meng.li@windriver.com> >>> >>> In uboot command line environment, watchdog is not able to be stopped >>> with below commands: >>> SOCFPGA_STRATIX10 # wdt dev watchdog@ffd00200 >>> SOCFPGA_STRATIX10 # wdt stop >>> Refer to watchdog driver in linux kernel, it is also need to reset >>> watchdog after disable it so that the disable action takes effect. >>> >>> v2: >>> Change "#if CONFIG_IS_ENABLED(DM_RESET)" into "if >>> (CONFIG_IS_ENABLED(DM_RESET)) {", and define the variable into if >>> condition sentence. >> >> A few comments: >> >> This version changelog belongs below the "---" line. >> >> Please Cc interested people upon new versions, e.g. myself as I reviewed >> this patch. >> >> Other that this: >> >> Reviewed-by: Stefan Roese <sr@denx.de> >> > > Would you like me to add "Cc:" into comment log, too? Or only CC to > you in mail list? That's up to you. But when you put the Cc: in the commit text (below the Reviewed-by tag), then "git send-email" will automatically Cc all listed addresses. Thanks, Stefan > Thanks, > Limeng > >> Thanks, >> Stefan >> >>> Signed-off-by: Meng Li <Meng.Li@windriver.com> >>> --- >>> drivers/watchdog/designware_wdt.c | 17 +++++++++++++++++ >>> 1 file changed, 17 insertions(+) >>> >>> diff --git a/drivers/watchdog/designware_wdt.c >>> b/drivers/watchdog/designware_wdt.c >>> index 12f09a7a39..57cad1effc 100644 >>> --- a/drivers/watchdog/designware_wdt.c >>> +++ b/drivers/watchdog/designware_wdt.c >>> @@ -96,6 +96,23 @@ static int designware_wdt_stop(struct udevice *dev) >>> designware_wdt_reset(dev); >>> writel(0, priv->base + DW_WDT_CR); >>> >>> + if (CONFIG_IS_ENABLED(DM_RESET)) { >>> + struct reset_ctl_bulk resets; >>> + int ret; >>> + >>> + ret = reset_get_bulk(dev, &resets); >>> + if (ret) >>> + return ret; >>> + >>> + ret = reset_assert_bulk(&resets); >>> + if (ret) >>> + return ret; >>> + >>> + ret = reset_deassert_bulk(&resets); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> return 0; >>> } >>> >>> >> >> >> Viele Grüße, >> Stefan >> >> -- >> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de Viele Grüße, Stefan
On 28.04.21 04:12, Li, Meng wrote: > > >> -----Original Message----- >> From: Sean Anderson <sean.anderson@seco.com> >> Sent: Tuesday, April 27, 2021 10:50 PM >> To: Stefan Roese <sr@denx.de>; Li, Meng <Meng.Li@windriver.com>; u- >> boot@lists.denx.de; chin.liang.see@intel.com; dinh.nguyen@intel.com; >> sjg@chromium.org >> Subject: Re: [PATCH v2, 1/2] driver: watchdog: reset watchdog in >> designware_wdt_stop() function >> >> [Please note: This e-mail is from an EXTERNAL e-mail address] >> >> On 4/27/21 10:23 AM, Stefan Roese wrote: >> > On 27.04.21 10:41, Meng.Li@windriver.com wrote: >> >> From: MengLi <meng.li@windriver.com> >> >> In uboot command line >> environment, watchdog is not able to be >> stopped with below commands: >> >> SOCFPGA_STRATIX10 # wdt dev watchdog@ffd00200 >> >> SOCFPGA_STRATIX10 # wdt stop >> Refer to watchdog driver in linux kernel, >> it is also need to reset >> watchdog after disable it so that the disable action >> takes effect. >> >> >> >> v2: >> >> Change "#if CONFIG_IS_ENABLED(DM_RESET)" into >> "if >> (CONFIG_IS_ENABLED(DM_RESET)) {", and define the variable >> into if >> condition sentence. >> > >> > A few comments: >> > >> > This version changelog belongs below the "---" line. >> > >> > Please Cc interested people upon new versions, e.g. myself as I reviewed > >> this patch. >> > >> > Other that this: >> > >> > Reviewed-by: Stefan Roese <sr@denx.de> > > Thanks, > Stefan > >> >> Signed-off-by: Meng Li <Meng.Li@windriver.com> >> --- >> >> drivers/watchdog/designware_wdt.c | 17 +++++++++++++++++ >> >> 1 file changed, 17 insertions(+) >> >> >> >> diff --git a/drivers/watchdog/designware_wdt.c >> b/drivers/watchdog/designware_wdt.c >> >> index 12f09a7a39..57cad1effc 100644 >> >> --- a/drivers/watchdog/designware_wdt.c >> >> +++ b/drivers/watchdog/designware_wdt.c >> >> @@ -96,6 +96,23 @@ static int designware_wdt_stop(struct udevice >> *dev) >> >> designware_wdt_reset(dev); >> >> writel(0, priv->base + DW_WDT_CR); >> >> + if (CONFIG_IS_ENABLED(DM_RESET)) { >> >> + struct reset_ctl_bulk resets; >> >> + int ret; >> >> + >> >> + ret = reset_get_bulk(dev, &resets); >> >> Have you considered adding the resets to designware_wdt_priv and saving >> them when we request them in probe()? >> > > Yes! thanks for reminding me. Yes. Since the reset gets referenced multiple times (with your addition now), this absolutely makes sense. > But I want to fix this issue by modifying the minimum range code. > Because I and not the original person of creating this driver. > I don't want to other part of code if it is not essential. That's not the way this open source development works. Please make the suggested changes and submit the patch for review. The review process with potential tests should catch potential issues. Thanks, Stefan > Thanks, > Limeng > >> --Sean >> >> >> + if (ret) >> >> + return ret; >> >> + >> >> + ret = reset_assert_bulk(&resets); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + ret = reset_deassert_bulk(&resets); >> >> + if (ret) >> >> + return ret; >> >> + } >> >> + >> >> return 0; >> >> } >> >> >> > >> > >> > Viele Grüße, >> > Stefan >> > Viele Grüße, Stefan
diff --git a/drivers/watchdog/designware_wdt.c b/drivers/watchdog/designware_wdt.c index 12f09a7a39..57cad1effc 100644 --- a/drivers/watchdog/designware_wdt.c +++ b/drivers/watchdog/designware_wdt.c @@ -96,6 +96,23 @@ static int designware_wdt_stop(struct udevice *dev) designware_wdt_reset(dev); writel(0, priv->base + DW_WDT_CR); + if (CONFIG_IS_ENABLED(DM_RESET)) { + struct reset_ctl_bulk resets; + int ret; + + ret = reset_get_bulk(dev, &resets); + if (ret) + return ret; + + ret = reset_assert_bulk(&resets); + if (ret) + return ret; + + ret = reset_deassert_bulk(&resets); + if (ret) + return ret; + } + return 0; }