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

[Sergey Semin]

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, Sergey.Semin@xxxxxxxxxxxxxxxxxxxx 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