Re: Question: why call clk_prepare in pm_clk_acquire

[Ulf Hansson <ulf.hansson@linaro.org>]

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...

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.

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.

For the imx8 based platforms [1], there are only three consumers
(three mmc controllers) of the scmi power domain ("scmi_devpd"). The
corresponding mmc host driver is the
drivers/mmc/host/sdhci-esdhc-imx.c, which needs to handle the clocks
itself. I assume this is the one Peng is referring to.

>
> > >
> > > 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.

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.

In this particular case, sdhci_esdhc_runtime_resume() needs to make
some preparations before it can prepare/enable the clocks (it calls
clk_set_rate() for example).

>
> > > However the clk has flag CLK_SET_RATE_GATE, clk_set_rate will fail in driver, because
> > > clk is prepared in pm_clk_acquire.
> > >
>
> Where is CLK_SET_RATE_GATE set exactly ?
>
> > > Looking into drivers/base/power/clock_ops.c, I see pm_clk_suspend/pm_clk_resume
> > > will handle clk prepare/unprepared, so why pm_clk_acquire will also prepare the clk?
> >
>
> As asked above do you see the actual clk_prepare getting called as I
> see it isn't if lk_is_enabled_when_prepared(clk) = true.
>
> > I agree, the behaviour is certainly questionable to me too. However,
> > it may be tricky to change by now, due to the deployment that has
> > happened over the years.
> >
>
> Agreed.
>
> > In principle we would need to make the part where pm_clk_acquire
> > prepares the clock to become optional, in some clever way.
> >
>
> I see it is already, let us see what is Peng's observation.

Yes, good point, I didn't notice that! However, as stated above, it
seems like it doesn't really matter.

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?

>
> --
> Regards,
> Sudeep

Kind regards
Uffe