Re: Question: why call clk_prepare in pm_clk_acquire

[Sudeep Holla <sudeep.holla@arm.com>]

On Fri, Sep 09, 2022 at 01:12:03PM +0200, Ulf Hansson wrote:
> On Thu, 8 Sept 2022 at 19:38, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Sep 08, 2022 at 04:37:13PM +0200, Ulf Hansson wrote:
> > > On Thu, 8 Sept 2022 at 09:33, Peng Fan <peng.fan@nxp.com> wrote:
> > > >
> > > > Hi All,
> > > >
> > > > We are facing an issue clk_set_rate fail with commit a3b884cef873 ("firmware:
> > > > arm_scmi: Add clock management to the SCMI power domain") ,
> > >
> > > Hmm, I wonder about the main reason behind that commit. Can we revert
> > > it or is there some platform/driver that is really relying on it?
> > >
> >
> > IIUC, at the time of the commit, it was needed on some Renesas platform.
> > Not sure if it is still used or not.
> 
> Okay! Maybe Nico remembers more, as he authored the patch...
>

May be, or even check with Renesas team who tested his patch.

> Normally it's best decided on a platform basis, whether it really
> makes sense to use the GENPD_FLAG_PM_CLK. As the scmi power domain is
> a cross platform power domain, it worries me that we lose some needed
> flexibility, which is likely to make it more difficult to use it for
> some platforms. Also note, the main point behind GENPD_FLAG_PM_CLK,
> was just to consolidate code.
>

I agree and share similar concern.

> That said, I decided to do some research, by looking at the DTS files
> in the kernel. So far, there is only Juno and the imx8 based
> platform(s) that are using the scmi power domain.
>

Yes but there are few without any DTS upstream that I know.

> >
> > > >
> > > > we use scmi power domain, but not use scmi clk, but with upper commit, the clk is prepared
> > > > when pm_clk_acquire.
> > > >
> >
> > Is this based on latest SCMI clocks that support atomic or older one
> > which doesn't. If latter, I see pm_clk_acquire doesn't actually call
> > prepare as if clk_is_enabled_when_prepared(clk) = true. Do you see have
> > issue ?
>
> It doesn't really matter if we would be using an atomic clock or not.
>

No what I meant is pm_clk_acquire doesn't call prepare as clk_is_enabled_when_prepared
is true for scmi clocks(non atomic).

> The problem is that when using GENPD_FLAG_PM_CLK, during runtime
> resume (genpd_runtime_resume) we end up calling pm_clk_resume(), but
> prior invoking the consumer driver's ->runtime_resume() callback. In
> other words, the clock(s) will already be prepared and enabled when
> the driver's ->runtime_resume() callback gets invoked. That certainly
> isn't going to work for all cases.
>

Any specific reasons ? Sorry I am missing to understand why that would
be an issue ?


[...]

> In my opinion we should really try to move away from using
> GENPD_FLAG_PM_CLK for the scmi power domain. I can prepare a patch, if
> you think it makes sense?
> 

As along as Renesas is fine with that, it should be OK, but doesn't removing
that flag means we can drop {attach,detach}_dev callbacks too as they are just
adding clocks and without the flag it is useless. Sounds like we must revert
the patch completely IIUC.

-- 
Regards,
Sudeep