[5/7] watchdog: dw_wdt: Support devices with asynch clocks

Commit Message

Serge Semin March 6, 2020, 1:27 p.m. UTC

From: Serge Semin <[e-mail]>

DW Watchdog IP core can be synthesised with asynchronous timer/APB
clocks support (WDT_ASYNC_CLK_MODE_ENABLE == 1). In this case
separate clock signals are supposed to be used to feed watchdog timer
and APB interface of the device. Currently the driver supports
the synchronous mode only. Since there is no way to determine which
mode was actually activated for device from its registers, we have to
rely on the platform device configuration data. If optional "pclk"
clock source is supplied, we consider the device working in asynchronous
mode, otherwise the driver falls back to the synchronous configuration.

Signed-off-by: Serge Semin <[e-mail]>
Signed-off-by: Alexey Malahov <[e-mail]>
Cc: Thomas Bogendoerfer <[e-mail]>
Cc: Paul Burton <[e-mail]>
Cc: Ralf Baechle <[e-mail]>
---
 drivers/watchdog/dw_wdt.c | 48 +++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 5 deletions(-)

Comments

Guenter Roeck March 15, 2020, 2:22 p.m. UTC | #1

On Fri, Mar 06, 2020 at 04:27:45PM +0300, [e-mail] wrote:
> From: Serge Semin <[e-mail]>
> 
> DW Watchdog IP core can be synthesised with asynchronous timer/APB
> clocks support (WDT_ASYNC_CLK_MODE_ENABLE == 1). In this case
> separate clock signals are supposed to be used to feed watchdog timer
> and APB interface of the device. Currently the driver supports
> the synchronous mode only. Since there is no way to determine which
> mode was actually activated for device from its registers, we have to
> rely on the platform device configuration data. If optional "pclk"
> clock source is supplied, we consider the device working in asynchronous
> mode, otherwise the driver falls back to the synchronous configuration.
> 
> Signed-off-by: Serge Semin <[e-mail]>
> Signed-off-by: Alexey Malahov <[e-mail]>
> Cc: Thomas Bogendoerfer <[e-mail]>
> Cc: Paul Burton <[e-mail]>
> Cc: Ralf Baechle <[e-mail]>
> ---
>  drivers/watchdog/dw_wdt.c | 48 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 4a57b7d777dc..eb909c63a1b5 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -61,6 +61,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>  struct dw_wdt {
>  	void __iomem		*regs;
>  	struct clk		*clk;
> +	struct clk		*pclk;
>  	unsigned long		rate;
>  	unsigned int		max_top;
>  	unsigned int		timeouts[DW_WDT_NUM_TOPS];
> @@ -270,6 +271,7 @@ static int dw_wdt_suspend(struct device *dev)
>  	dw_wdt->control = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>  	dw_wdt->timeout = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>  
> +	clk_disable_unprepare(dw_wdt->pclk);
>  	clk_disable_unprepare(dw_wdt->clk);
>  
>  	return 0;
> @@ -283,6 +285,12 @@ static int dw_wdt_resume(struct device *dev)
>  	if (err)
>  		return err;
>  
> +	err = clk_prepare_enable(dw_wdt->pclk);
> +	if (err) {
> +		clk_disable_unprepare(dw_wdt->clk);
> +		return err;
> +	}
> +
>  	writel(dw_wdt->timeout, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>  	writel(dw_wdt->control, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>  
> @@ -344,9 +352,18 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  	if (IS_ERR(dw_wdt->regs))
>  		return PTR_ERR(dw_wdt->regs);
>  
> -	dw_wdt->clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(dw_wdt->clk))
> -		return PTR_ERR(dw_wdt->clk);
> +	/*
> +	 * Try to request the watchdog dedicated timer clock source. It must
> +	 * be supplied if asynchronous mode is enabled. Otherwise fallback
> +	 * to the common timer/bus clocks configuration, in which the very first
> +	 * found clocks supply both timer and APB signals.
> +	 */
> +	dw_wdt->clk = devm_clk_get(dev, "tclk");
> +	if (IS_ERR(dw_wdt->clk)) {
> +		dw_wdt->clk = devm_clk_get(dev, NULL);
> +		if (IS_ERR(dw_wdt->clk))
> +			return PTR_ERR(dw_wdt->clk);
> +	}
>  
>  	ret = clk_prepare_enable(dw_wdt->clk);
>  	if (ret)
> @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  		goto out_disable_clk;
>  	}
>  
> +	/*
> +	 * Request APB clocks if device is configured with async clocks mode.
> +	 * In this case both tclk and pclk clocks are supposed to be specified.
> +	 * Alas we can't know for sure whether async mode was really activated,
> +	 * so the pclk reference is left optional. If it it's failed to be
> +	 * found we consider the device configured in synchronous clocks mode.
> +	 */
> +	dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
> +	if (IS_ERR(dw_wdt->pclk)) {
> +		ret = PTR_ERR(dw_wdt->pclk);
> +		goto out_disable_clk;
> +	}
> +
> +	ret = clk_prepare_enable(dw_wdt->pclk);

Not every implementation of clk_enable() checks for a NULL parameter.
Some return an error. This can not be trusted to work on all platforms /
architectures.

> +	if (ret)
> +		goto out_disable_clk;
> +
>  	dw_wdt->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
>  	if (IS_ERR(dw_wdt->rst)) {
>  		ret = PTR_ERR(dw_wdt->rst);
> -		goto out_disable_clk;
> +		goto out_disable_pclk;
>  	}
>  
>  	reset_control_deassert(dw_wdt->rst);
> @@ -399,10 +433,13 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  
>  	ret = watchdog_register_device(wdd);
>  	if (ret)
> -		goto out_disable_clk;
> +		goto out_disable_pclk;
>  
>  	return 0;
>  
> +out_disable_pclk:
> +	clk_disable_unprepare(dw_wdt->pclk);
> +
>  out_disable_clk:
>  	clk_disable_unprepare(dw_wdt->clk);
>  	return ret;
> @@ -414,6 +451,7 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
>  
>  	watchdog_unregister_device(&dw_wdt->wdd);
>  	reset_control_assert(dw_wdt->rst);
> +	clk_disable_unprepare(dw_wdt->pclk);
>  	clk_disable_unprepare(dw_wdt->clk);
>  
>  	return 0;

Serge Semin April 10, 2020, 6:59 p.m. UTC | #2

Michael, Stephen, could you take a look at the issue we've got here?

Guenter, my comment is below.

On Sun, Mar 15, 2020 at 07:22:07AM -0700, Guenter Roeck wrote:
> On Fri, Mar 06, 2020 at 04:27:45PM +0300, [e-mail] wrote:
> > From: Serge Semin <[e-mail]>
> > 
> > DW Watchdog IP core can be synthesised with asynchronous timer/APB
> > clocks support (WDT_ASYNC_CLK_MODE_ENABLE == 1). In this case
> > separate clock signals are supposed to be used to feed watchdog timer
> > and APB interface of the device. Currently the driver supports
> > the synchronous mode only. Since there is no way to determine which
> > mode was actually activated for device from its registers, we have to
> > rely on the platform device configuration data. If optional "pclk"
> > clock source is supplied, we consider the device working in asynchronous
> > mode, otherwise the driver falls back to the synchronous configuration.
> > 
> > Signed-off-by: Serge Semin <[e-mail]>
> > Signed-off-by: Alexey Malahov <[e-mail]>
> > Cc: Thomas Bogendoerfer <[e-mail]>
> > Cc: Paul Burton <[e-mail]>
> > Cc: Ralf Baechle <[e-mail]>
> > ---
> >  drivers/watchdog/dw_wdt.c | 48 +++++++++++++++++++++++++++++++++++----
> >  1 file changed, 43 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index 4a57b7d777dc..eb909c63a1b5 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -61,6 +61,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> >  struct dw_wdt {
> >  	void __iomem		*regs;
> >  	struct clk		*clk;
> > +	struct clk		*pclk;
> >  	unsigned long		rate;
> >  	unsigned int		max_top;
> >  	unsigned int		timeouts[DW_WDT_NUM_TOPS];
> > @@ -270,6 +271,7 @@ static int dw_wdt_suspend(struct device *dev)
> >  	dw_wdt->control = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> >  	dw_wdt->timeout = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >  
> > +	clk_disable_unprepare(dw_wdt->pclk);
> >  	clk_disable_unprepare(dw_wdt->clk);
> >  
> >  	return 0;
> > @@ -283,6 +285,12 @@ static int dw_wdt_resume(struct device *dev)
> >  	if (err)
> >  		return err;
> >  
> > +	err = clk_prepare_enable(dw_wdt->pclk);
> > +	if (err) {
> > +		clk_disable_unprepare(dw_wdt->clk);
> > +		return err;
> > +	}
> > +
> >  	writel(dw_wdt->timeout, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >  	writel(dw_wdt->control, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> >  
> > @@ -344,9 +352,18 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  	if (IS_ERR(dw_wdt->regs))
> >  		return PTR_ERR(dw_wdt->regs);
> >  
> > -	dw_wdt->clk = devm_clk_get(dev, NULL);
> > -	if (IS_ERR(dw_wdt->clk))
> > -		return PTR_ERR(dw_wdt->clk);
> > +	/*
> > +	 * Try to request the watchdog dedicated timer clock source. It must
> > +	 * be supplied if asynchronous mode is enabled. Otherwise fallback
> > +	 * to the common timer/bus clocks configuration, in which the very first
> > +	 * found clocks supply both timer and APB signals.
> > +	 */
> > +	dw_wdt->clk = devm_clk_get(dev, "tclk");
> > +	if (IS_ERR(dw_wdt->clk)) {
> > +		dw_wdt->clk = devm_clk_get(dev, NULL);
> > +		if (IS_ERR(dw_wdt->clk))
> > +			return PTR_ERR(dw_wdt->clk);
> > +	}
> >  
> >  	ret = clk_prepare_enable(dw_wdt->clk);
> >  	if (ret)
> > @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  		goto out_disable_clk;
> >  	}
> >  
> > +	/*
> > +	 * Request APB clocks if device is configured with async clocks mode.
> > +	 * In this case both tclk and pclk clocks are supposed to be specified.
> > +	 * Alas we can't know for sure whether async mode was really activated,
> > +	 * so the pclk reference is left optional. If it it's failed to be
> > +	 * found we consider the device configured in synchronous clocks mode.
> > +	 */
> > +	dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
> > +	if (IS_ERR(dw_wdt->pclk)) {
> > +		ret = PTR_ERR(dw_wdt->pclk);
> > +		goto out_disable_clk;
> > +	}
> > +
> > +	ret = clk_prepare_enable(dw_wdt->pclk);
> 
> Not every implementation of clk_enable() checks for a NULL parameter.
> Some return an error. This can not be trusted to work on all platforms /
> architectures.

Hm, this was unexpected twist. I've submitted not a single patch with optional
clock API usage. It was first time I've got a comment like this, that the
API isn't cross-platform. As I see it this isn't the patch problem, but the
platforms/common clock bug. The platforms code must have been submitted before
the optional clock API was introduced or the API hasn't been properly
implemented or we don't understand something.

Stephen, Michael could you clarify the situation with the
cross-platformness of the optional clock API.

-Sergey

> 
> > +	if (ret)
> > +		goto out_disable_clk;
> > +
> >  	dw_wdt->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
> >  	if (IS_ERR(dw_wdt->rst)) {
> >  		ret = PTR_ERR(dw_wdt->rst);
> > -		goto out_disable_clk;
> > +		goto out_disable_pclk;
> >  	}
> >  
> >  	reset_control_deassert(dw_wdt->rst);
> > @@ -399,10 +433,13 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  
> >  	ret = watchdog_register_device(wdd);
> >  	if (ret)
> > -		goto out_disable_clk;
> > +		goto out_disable_pclk;
> >  
> >  	return 0;
> >  
> > +out_disable_pclk:
> > +	clk_disable_unprepare(dw_wdt->pclk);
> > +
> >  out_disable_clk:
> >  	clk_disable_unprepare(dw_wdt->clk);
> >  	return ret;
> > @@ -414,6 +451,7 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
> >  
> >  	watchdog_unregister_device(&dw_wdt->wdd);
> >  	reset_control_assert(dw_wdt->rst);
> > +	clk_disable_unprepare(dw_wdt->pclk);
> >  	clk_disable_unprepare(dw_wdt->clk);
> >  
> >  	return 0;

Stephen Boyd April 13, 2020, 8:52 p.m. UTC | #3

Quoting Sergey Semin (2020-04-10 11:59:34)
> Michael, Stephen, could you take a look at the issue we've got here?
> 
> Guenter, my comment is below.
> 
> On Sun, Mar 15, 2020 at 07:22:07AM -0700, Guenter Roeck wrote:
> > On Fri, Mar 06, 2020 at 04:27:45PM +0300, [e-mail] wrote:
> > > @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> > >             goto out_disable_clk;
> > >     }
> > >  
> > > +   /*
> > > +    * Request APB clocks if device is configured with async clocks mode.
> > > +    * In this case both tclk and pclk clocks are supposed to be specified.
> > > +    * Alas we can't know for sure whether async mode was really activated,
> > > +    * so the pclk reference is left optional. If it it's failed to be
> > > +    * found we consider the device configured in synchronous clocks mode.
> > > +    */
> > > +   dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
> > > +   if (IS_ERR(dw_wdt->pclk)) {
> > > +           ret = PTR_ERR(dw_wdt->pclk);
> > > +           goto out_disable_clk;
> > > +   }
> > > +
> > > +   ret = clk_prepare_enable(dw_wdt->pclk);
> > 
> > Not every implementation of clk_enable() checks for a NULL parameter.
> > Some return an error. This can not be trusted to work on all platforms /
> > architectures.
> 
> Hm, this was unexpected twist. I've submitted not a single patch with optional
> clock API usage. It was first time I've got a comment like this, that the
> API isn't cross-platform. As I see it this isn't the patch problem, but the
> platforms/common clock bug. The platforms code must have been submitted before
> the optional clock API was introduced or the API hasn't been properly
> implemented or we don't understand something.
> 
> Stephen, Michael could you clarify the situation with the
> cross-platformness of the optional clock API.
> 

NULL is a valid clk to return from clk_get(). And the documentation of
clk_enable() says that "If the clock can not be enabled/disabled, this
should return success". Given that a NULL pointer can't do much of
anything I think any platform that returns an error in this situation is
deviating from the documentation of the clk API.

Does any platform that uses this driver use one of these non-common clk
framework implementations? All of this may not matter if they all use
the CCF.

Guenter Roeck April 14, 2020, 2:55 a.m. UTC | #4

On 4/13/20 1:52 PM, Stephen Boyd wrote:
> Quoting Sergey Semin (2020-04-10 11:59:34)
>> Michael, Stephen, could you take a look at the issue we've got here?
>>
>> Guenter, my comment is below.
>>
>> On Sun, Mar 15, 2020 at 07:22:07AM -0700, Guenter Roeck wrote:
>>> On Fri, Mar 06, 2020 at 04:27:45PM +0300, [e-mail] wrote:
>>>> @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>>>>             goto out_disable_clk;
>>>>     }
>>>>  
>>>> +   /*
>>>> +    * Request APB clocks if device is configured with async clocks mode.
>>>> +    * In this case both tclk and pclk clocks are supposed to be specified.
>>>> +    * Alas we can't know for sure whether async mode was really activated,
>>>> +    * so the pclk reference is left optional. If it it's failed to be
>>>> +    * found we consider the device configured in synchronous clocks mode.
>>>> +    */
>>>> +   dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
>>>> +   if (IS_ERR(dw_wdt->pclk)) {
>>>> +           ret = PTR_ERR(dw_wdt->pclk);
>>>> +           goto out_disable_clk;
>>>> +   }
>>>> +
>>>> +   ret = clk_prepare_enable(dw_wdt->pclk);
>>>
>>> Not every implementation of clk_enable() checks for a NULL parameter.
>>> Some return an error. This can not be trusted to work on all platforms /
>>> architectures.
>>
>> Hm, this was unexpected twist. I've submitted not a single patch with optional
>> clock API usage. It was first time I've got a comment like this, that the
>> API isn't cross-platform. As I see it this isn't the patch problem, but the
>> platforms/common clock bug. The platforms code must have been submitted before
>> the optional clock API was introduced or the API hasn't been properly
>> implemented or we don't understand something.
>>
>> Stephen, Michael could you clarify the situation with the
>> cross-platformness of the optional clock API.
>>
> 
> NULL is a valid clk to return from clk_get(). And the documentation of
> clk_enable() says that "If the clock can not be enabled/disabled, this
> should return success". Given that a NULL pointer can't do much of
> anything I think any platform that returns an error in this situation is
> deviating from the documentation of the clk API.
> 

This is not about returning an error; some platforms don't check for NULL.

> Does any platform that uses this driver use one of these non-common clk
> framework implementations? All of this may not matter if they all use
> the CCF.
> 

Currently the driver is only used on arm and arm64. Maybe those are safe ?

Also, it looks like clk_enable() exists in the non-standard implementations,
but clk_prepare (and thus clk_prepare_enable) only exist in the standard
implementation. With that, maybe it is always safe to call
clk_prepare_enable() with a NULL parameter ?

Thanks,
Guenter

Serge Semin April 14, 2020, 10:01 a.m. UTC | #5

On Mon, Apr 13, 2020 at 07:55:42PM -0700, Guenter Roeck wrote:
> On 4/13/20 1:52 PM, Stephen Boyd wrote:
> > Quoting Sergey Semin (2020-04-10 11:59:34)
> >> Michael, Stephen, could you take a look at the issue we've got here?
> >>
> >> Guenter, my comment is below.
> >>
> >> On Sun, Mar 15, 2020 at 07:22:07AM -0700, Guenter Roeck wrote:
> >>> On Fri, Mar 06, 2020 at 04:27:45PM +0300, [e-mail] wrote:
> >>>> @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >>>>             goto out_disable_clk;
> >>>>     }
> >>>>  
> >>>> +   /*
> >>>> +    * Request APB clocks if device is configured with async clocks mode.
> >>>> +    * In this case both tclk and pclk clocks are supposed to be specified.
> >>>> +    * Alas we can't know for sure whether async mode was really activated,
> >>>> +    * so the pclk reference is left optional. If it it's failed to be
> >>>> +    * found we consider the device configured in synchronous clocks mode.
> >>>> +    */
> >>>> +   dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
> >>>> +   if (IS_ERR(dw_wdt->pclk)) {
> >>>> +           ret = PTR_ERR(dw_wdt->pclk);
> >>>> +           goto out_disable_clk;
> >>>> +   }
> >>>> +
> >>>> +   ret = clk_prepare_enable(dw_wdt->pclk);
> >>>
> >>> Not every implementation of clk_enable() checks for a NULL parameter.
> >>> Some return an error. This can not be trusted to work on all platforms /
> >>> architectures.
> >>
> >> Hm, this was unexpected twist. I've submitted not a single patch with optional
> >> clock API usage. It was first time I've got a comment like this, that the
> >> API isn't cross-platform. As I see it this isn't the patch problem, but the
> >> platforms/common clock bug. The platforms code must have been submitted before
> >> the optional clock API was introduced or the API hasn't been properly
> >> implemented or we don't understand something.
> >>
> >> Stephen, Michael could you clarify the situation with the
> >> cross-platformness of the optional clock API.
> >>
> > 
> > NULL is a valid clk to return from clk_get(). And the documentation of
> > clk_enable() says that "If the clock can not be enabled/disabled, this
> > should return success". Given that a NULL pointer can't do much of
> > anything I think any platform that returns an error in this situation is
> > deviating from the documentation of the clk API.
> > 

Stephen, thanks for clarification. AFAICS regarding ARM the next three
platforms don't follow that rule:
- arch/arm/mach-ep93xx: No CCF support. clk_enable() returns -EINVAL if NULL
  clk parameter specified.
- arch/arm/mach-mmp: If no CCF enabled, then clk_enable() doesn't check
  the parameter for NULLness.
- arch/arm/mach-omap1: No CCF support. clk_enable() returns -EINVAL if
  NULL clk parameter specified.

Though these platforms statically instantiate the platform devices
except mmp, which may use dtb on some systems. Who knows how the drivers
are using the clocks after the instantiation. Maybe they don't.

> 
> This is not about returning an error; some platforms don't check for NULL.

Please, see the comment above. For ARM it's just three platforms. Though
as Stephen said returning error is wrong in this case too.

> 
> > Does any platform that uses this driver use one of these non-common clk
> > framework implementations? All of this may not matter if they all use
> > the CCF.
> > 
> 
> Currently the driver is only used on arm and arm64. Maybe those are safe ?

AFAICS these are only platforms using snps,dw-wdt at the moment:
	ARM64: Synaptics Berkin 4CT
	ARM64: Intel/Altera SOCFPGAs 
	ARM64: Rockchip PX30/RK33xx
	ARM: Altera SOCFPGA Arria10
	ARM: Marvell Berlin SoCs
	ARM: Rockhip RK3xxx
	ARM: Realview RV1108

All of them rely on CCF. In addition to them there is going to be
Baikal-T1 SoC based on MIPS P5600 CPU core, which clocks also require
CCF. So yes, we are safe to use the optional clocks API at the moment.

> 
> Also, it looks like clk_enable() exists in the non-standard implementations,
> but clk_prepare (and thus clk_prepare_enable) only exist in the standard
> implementation. With that, maybe it is always safe to call
> clk_prepare_enable() with a NULL parameter ?

No. clk_prepare_enable() calls both clk_prepare() and clk_enable()
sequentially. So if some of them don't expect NULL pointer passed, then
the system will blow up. Though this isn't problem for snps,dw-wdt
driver, since all the platforms using it rely on CCF, which has optional
clocks support.

If in future we have a platform, which don't use CCF and don't support
NULL-value of the clk parameter, then this will be the platform problem,
since it doesn't implement the clock API correctly.

Regards,
-Sergey

> 
> Thanks,
> Guenter

Patch

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 4a57b7d777dc..eb909c63a1b5 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -61,6 +61,7 @@  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 struct dw_wdt {
 	void __iomem		*regs;
 	struct clk		*clk;
+	struct clk		*pclk;
 	unsigned long		rate;
 	unsigned int		max_top;
 	unsigned int		timeouts[DW_WDT_NUM_TOPS];
@@ -270,6 +271,7 @@  static int dw_wdt_suspend(struct device *dev)
 	dw_wdt->control = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
 	dw_wdt->timeout = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
 
+	clk_disable_unprepare(dw_wdt->pclk);
 	clk_disable_unprepare(dw_wdt->clk);
 
 	return 0;
@@ -283,6 +285,12 @@  static int dw_wdt_resume(struct device *dev)
 	if (err)
 		return err;
 
+	err = clk_prepare_enable(dw_wdt->pclk);
+	if (err) {
+		clk_disable_unprepare(dw_wdt->clk);
+		return err;
+	}
+
 	writel(dw_wdt->timeout, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
 	writel(dw_wdt->control, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
 
@@ -344,9 +352,18 @@  static int dw_wdt_drv_probe(struct platform_device *pdev)
 	if (IS_ERR(dw_wdt->regs))
 		return PTR_ERR(dw_wdt->regs);
 
-	dw_wdt->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(dw_wdt->clk))
-		return PTR_ERR(dw_wdt->clk);
+	/*
+	 * Try to request the watchdog dedicated timer clock source. It must
+	 * be supplied if asynchronous mode is enabled. Otherwise fallback
+	 * to the common timer/bus clocks configuration, in which the very first
+	 * found clocks supply both timer and APB signals.
+	 */
+	dw_wdt->clk = devm_clk_get(dev, "tclk");
+	if (IS_ERR(dw_wdt->clk)) {
+		dw_wdt->clk = devm_clk_get(dev, NULL);
+		if (IS_ERR(dw_wdt->clk))
+			return PTR_ERR(dw_wdt->clk);
+	}
 
 	ret = clk_prepare_enable(dw_wdt->clk);
 	if (ret)
@@ -358,10 +375,27 @@  static int dw_wdt_drv_probe(struct platform_device *pdev)
 		goto out_disable_clk;
 	}
 
+	/*
+	 * Request APB clocks if device is configured with async clocks mode.
+	 * In this case both tclk and pclk clocks are supposed to be specified.
+	 * Alas we can't know for sure whether async mode was really activated,
+	 * so the pclk reference is left optional. If it it's failed to be
+	 * found we consider the device configured in synchronous clocks mode.
+	 */
+	dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
+	if (IS_ERR(dw_wdt->pclk)) {
+		ret = PTR_ERR(dw_wdt->pclk);
+		goto out_disable_clk;
+	}
+
+	ret = clk_prepare_enable(dw_wdt->pclk);
+	if (ret)
+		goto out_disable_clk;
+
 	dw_wdt->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
 	if (IS_ERR(dw_wdt->rst)) {
 		ret = PTR_ERR(dw_wdt->rst);
-		goto out_disable_clk;
+		goto out_disable_pclk;
 	}
 
 	reset_control_deassert(dw_wdt->rst);
@@ -399,10 +433,13 @@  static int dw_wdt_drv_probe(struct platform_device *pdev)
 
 	ret = watchdog_register_device(wdd);
 	if (ret)
-		goto out_disable_clk;
+		goto out_disable_pclk;
 
 	return 0;
 
+out_disable_pclk:
+	clk_disable_unprepare(dw_wdt->pclk);
+
 out_disable_clk:
 	clk_disable_unprepare(dw_wdt->clk);
 	return ret;
@@ -414,6 +451,7 @@  static int dw_wdt_drv_remove(struct platform_device *pdev)
 
 	watchdog_unregister_device(&dw_wdt->wdd);
 	reset_control_assert(dw_wdt->rst);
+	clk_disable_unprepare(dw_wdt->pclk);
 	clk_disable_unprepare(dw_wdt->clk);
 
 	return 0;