[PATCH v2, 1/2] driver: watchdog: reset watchdog in designware_wdt_stop() function

[PATCH v2, 1/2] driver: watchdog: reset watchdog in designware_wdt_stop() function

[Meng.Li at windriver.com]

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 at 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.

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;
 }
 
-- 
2.17.1

[PATCH v2,2/2] driver: watchdog: enable wdt command by default

[Meng.Li at windriver.com]

From: MengLi <meng.li@windriver.com>

In latest u-boot code, watchdog feature is implemented, so enable
wdt command by default.

Signed-off-by: Meng Li <Meng.Li@windriver.com>
---
 configs/socfpga_stratix10_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/socfpga_stratix10_defconfig b/configs/socfpga_stratix10_defconfig
index 02d4ac0dae..0256afe511 100644
--- a/configs/socfpga_stratix10_defconfig
+++ b/configs/socfpga_stratix10_defconfig
@@ -40,6 +40,7 @@ CONFIG_CMD_CACHE=y
 CONFIG_CMD_EXT4=y
 CONFIG_CMD_FAT=y
 CONFIG_CMD_FS_GENERIC=y
+CONFIG_CMD_WDT=y
 CONFIG_MTDIDS_DEFAULT="nor0=ff705000.spi.0"
 CONFIG_ENV_IS_IN_MMC=y
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
-- 
2.17.1

[PATCH v2, 1/2] driver: watchdog: reset watchdog in designware_wdt_stop() function

[Stefan Roese]

On 27.04.21 10:41, Meng.Li at 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 at 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

-- 
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 at denx.de

[PATCH v2, 1/2] driver: watchdog: reset watchdog in designware_wdt_stop() function

[Sean Anderson]

On 4/27/21 10:23 AM, Stefan Roese wrote:
 > On 27.04.21 10:41, Meng.Li at 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 at 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
 >

[PATCH v2, 1/2] driver: watchdog: reset watchdog in designware_wdt_stop() function

[Li, Meng]

> -----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 at lists.denx.de; chin.liang.see at intel.com; dinh.nguyen at intel.com;
> sjg at 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 at 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 at 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
>  >

[PATCH v2, 1/2] driver: watchdog: reset watchdog in designware_wdt_stop() function

[Li, Meng]

> -----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 at lists.denx.de;
> chin.liang.see at intel.com; dinh.nguyen at intel.com; sjg at 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 at 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 at 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 at denx.de

[PATCH v2, 1/2] driver: watchdog: reset watchdog in designware_wdt_stop() function

[Stefan Roese]

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 at lists.denx.de;
>> chin.liang.see at intel.com; dinh.nguyen at intel.com; sjg at 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 at 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 at 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 at denx.de


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 at denx.de

[PATCH v2, 1/2] driver: watchdog: reset watchdog in designware_wdt_stop() function

[Stefan Roese]

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 at lists.denx.de; chin.liang.see at intel.com; dinh.nguyen at intel.com;
>> sjg at 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 at 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 at 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

-- 
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 at denx.de