Re: [PATCH] clk: rk3288: add stub for ops->enable

[Marius Dinu <m95d+git@psihoexpert.ro>]

On Mon, 2026-03-16 11.10.38 ++0100, Jonas Karlman wrote:
> Hi Marius,
> 
> On 3/16/2026 10:57 AM, Marius Dinu wrote:
> > This fixes a bug where the watchdog can't enable the clock.
> > AFAIK, this clock is always enabled.
> > 
> > Tested on Asus TinkerBoard. Test results:
> > wdt dev         => works
> > wdt start 200   => works, but timeout is 43s
> > wdt start 10000 => works, but timeout is 1m27s
> > wdt reset       => works
> > wdt stop        => doesn't work
> 
> I have a different patch incoming that should fix this issue at watchdog
> driver level, something like following should make this work and
> properly handle platforms not implementing clk_enable ops.
> 
> diff --git a/drivers/watchdog/designware_wdt.c b/drivers/watchdog/designware_wdt.c
> index bd9d7105366..91228de5e8e 100644
> --- a/drivers/watchdog/designware_wdt.c
> +++ b/drivers/watchdog/designware_wdt.c
> @@ -122,7 +122,7 @@ static int designware_wdt_probe(struct udevice *dev)
>                 return ret;
> 
>  	ret = clk_enable(&clk);
> -	if (ret)
> +	if (ret && ret != -ENOSYS)
>  		return ret;
> 
>  	priv->clk_khz = clk_get_rate(&clk) / 1000;
> 

OK. I'm moving on from this arch and I just wanted to send all remaining
fixes that I use.

About your patch: aren't there commands or drivers other than wdt that may
try to enable the clock and fail?

> > 
> > Signed-off-by: Marius Dinu <m95d+git@psihoexpert.ro>
> > ---
> >  drivers/clk/rockchip/clk_rk3288.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/clk/rockchip/clk_rk3288.c b/drivers/clk/rockchip/clk_rk3288.c
> > index a4ff1c41abb..9cc883662ff 100644
> > --- a/drivers/clk/rockchip/clk_rk3288.c
> > +++ b/drivers/clk/rockchip/clk_rk3288.c
> > @@ -745,6 +745,10 @@ static ulong rockchip_saradc_set_clk(struct rockchip_cru *cru, uint hz)
> >  	return rockchip_saradc_get_clk(cru);
> >  }
> >  
> > +static int rk3288_clk_enable(struct clk *clk){
> > +	return 0;
> 
> This should properly be implemented for the clocks you need, and return
> -ENOSYS for the clocks not implemented.
> 
> > +}
> > +
> >  static ulong rk3288_clk_get_rate(struct clk *clk)
> >  {
> >  	struct rk3288_clk_priv *priv = dev_get_priv(clk->dev);
> > @@ -947,6 +951,7 @@ static int __maybe_unused rk3288_clk_set_parent(struct clk *clk, struct clk *par
> >  }
> >  
> >  static struct clk_ops rk3288_clk_ops = {
> > +	.enable		= rk3288_clk_enable,
> 
> Also include proper disable ops for the clocks you add enable ops for.
> 
> Regards,
> Jonas
> 
> >  	.get_rate	= rk3288_clk_get_rate,
> >  	.set_rate	= rk3288_clk_set_rate,
> >  #if CONFIG_IS_ENABLED(OF_REAL)
> 

Right! That's probably why "wdt stop" doesn't work.

Marius

PS: There is a duplicate of this email sent from the wrong address. Sorry.