[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 at seco.com>
>> Sent: Tuesday, April 27, 2021 10:50 PM
>> To: Stefan Roese <sr at denx.de>; Li, Meng <Meng.Li at 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 at 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 at denx.de>  >  > Thanks,  > Stefan  >  >>
>> Signed-off-by: Meng Li <Meng.Li at 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