Question: why call clk_prepare in pm_clk_acquire

Question: why call clk_prepare in pm_clk_acquire

[Peng Fan]

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") ,

we use scmi power domain, but not use scmi clk, but with upper commit, the clk is prepared 
when pm_clk_acquire.

However the clk has flag CLK_SET_RATE_GATE, clk_set_rate will fail in driver, because
clk is prepared in pm_clk_acquire.

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?


Thanks,
Peng.

Re: Question: why call clk_prepare in pm_clk_acquire

[Ulf Hansson]

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?

>
> we use scmi power domain, but not use scmi clk, but with upper commit, the clk is prepared
> when pm_clk_acquire.
>
> However the clk has flag CLK_SET_RATE_GATE, clk_set_rate will fail in driver, because
> clk is prepared in pm_clk_acquire.
>
> 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?

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.

In principle we would need to make the part where pm_clk_acquire
prepares the clock to become optional, in some clever way.

>
>
> Thanks,
> Peng.

Kind regards
Uffe

Re: Question: why call clk_prepare in pm_clk_acquire

[Sudeep Holla]

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.

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

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

-- 
Regards,
Sudeep

Re: Question: why call clk_prepare in pm_clk_acquire

[Ulf Hansson]

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

Re: Question: why call clk_prepare in pm_clk_acquire

[Sudeep Holla]

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

RE: Question: why call clk_prepare in pm_clk_acquire

[Peng Fan]

> Subject: Re: Question: why call clk_prepare in pm_clk_acquire
> 
> 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.
> 
> > >
> > > 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 ?

We are using 5.15 kernel, and clk_is_enabled_when_prepared(clk) is false.

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

The is flag in clk driver, drivers/clk/imx/clk-imx8ulp.c,
imx8ulp_clk_hw_composite will use CLK_SET_RATE_GATE.

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

Yes, clk_is_enabled_when_prepared(clk) is not always true in my case,
This function is just checking ops:
clk && !(clk->core->ops->enable && clk->core->ops->disable)

Regards,
Peng.

> 
> > 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.
> 
> --
> Regards,
> Sudeep

RE: Question: why call clk_prepare in pm_clk_acquire

[Peng Fan]

> Subject: Re: Question: why call clk_prepare in pm_clk_acquire
> 
> 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.

Yes, sdhc is just one of the devices, we have some downstream driver
also has issue with using scmi power domain.

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

Yes, because the clk prepared, in runtime resume, the clk would not able
to support reparent and set rate, because CLK_SET_PARENT_GATE,
CLK_SET_RATE_GATE are in when registering the clk, unless we do
clk_unprepare first.

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

That would be welcomed, and I could help test.

Thanks,
Peng
> 
> >
> > --
> > Regards,
> > Sudeep
> 
> Kind regards
> Uffe

RE: Question: why call clk_prepare in pm_clk_acquire

[Peng Fan]

> Subject: Re: Question: why call clk_prepare in pm_clk_acquire
> 
> 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 ?

Just my understanding:

        ret = genpd_start_dev(genpd, dev);  --> will invoke pm_clk_resume [1]
        if (ret)
                goto err_poweroff;

        ret = __genpd_runtime_resume(dev);  --> invoke drivres runtime_resume[2]
        if (ret)
                goto err_stop;

[1] will prepare the clks in my case
[2] will not able to set parent, set rate for the clks because the clks are registered
with CLK_SET_RATE_GATE, CLK_SET_RATE_PARENT.

BTW: i.MX8ULP only use SCMI POWER DOMAIN, we NOT use SCMI CLK, the clk
ip still handled by linux itself.

Regards,
Peng.

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

Re: Question: why call clk_prepare in pm_clk_acquire

[Sudeep Holla]

On Sun, Sep 11, 2022 at 01:31:15AM +0000, Peng Fan wrote:
> > Subject: Re: Question: why call clk_prepare in pm_clk_acquire
> > 
> > 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.
> > 
> > > >
> > > > 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 ?
> 
> We are using 5.15 kernel, and clk_is_enabled_when_prepared(clk) is false.
>

Sorry I was just looking at scmi clk driver.

> > 
> > > > 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 ?
> 
> The is flag in clk driver, drivers/clk/imx/clk-imx8ulp.c,
> imx8ulp_clk_hw_composite will use CLK_SET_RATE_GATE.
>

Understood.

> > 
> > > > 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.
> 
> Yes, clk_is_enabled_when_prepared(clk) is not always true in my case,
> This function is just checking ops:
> clk && !(clk->core->ops->enable && clk->core->ops->disable)
> 

Got it, I was assuming you are using scmi clk driver which is not the case.

-- 
Regards,
Sudeep

Re: Question: why call clk_prepare in pm_clk_acquire

[Geert Uytterhoeven]

Hi Sudeep,

On Fri, Sep 9, 2022 at 4:51 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> 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:
> > > > > 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.

I'm not aware of Renesas platforms using SCMI...

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

Juno and imx8 are not Renesas...

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

Hence no objection from me ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Question: why call clk_prepare in pm_clk_acquire

[Geert Uytterhoeven]

Hi Sudeep,

CC Dien Pham

On Mon, Sep 12, 2022 at 6:49 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Sep 9, 2022 at 4:51 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > 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:
> > > > > > 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.
>
> I'm not aware of Renesas platforms using SCMI...

Upon closer look, Diep Pham did report a build issue in the SCMI code, so
perhaps Diep knows more...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Question: why call clk_prepare in pm_clk_acquire

[Sudeep Holla]

On Mon, Sep 12, 2022 at 06:58:49PM +0100, Geert Uytterhoeven wrote:
> Hi Sudeep,
> 
> CC Dien Pham
> 
> On Mon, Sep 12, 2022 at 6:49 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Sep 9, 2022 at 4:51 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > 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:
> > > > > > > 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.
> >
> > I'm not aware of Renesas platforms using SCMI...
> 
> Upon closer look, Diep Pham did report a build issue in the SCMI code, so
> perhaps Diep knows more...
> 

Yes indeed, Diep Pham tested the original patch IIRC and also has reported
few bugs in SCMI clock code which are fixed. Hence I know it is used by
Renesas.

Hi Peng,

Absence of DTS changes indicate nothing. I am aware of couple of vendors
who use SCMI on several platforms and do report issues regularly and help
in review of the code. So DTS is not a good indicator of SCMI usage
unfortunately. On reason could be that since it is a firmware, bootloaders
can detect and update DTS, just my thought and may differ from the reality.

-- 
Regards,
Sudeep

Re: Question: why call clk_prepare in pm_clk_acquire

[Nicolas Pitre]

On Wed, 14 Sep 2022, Sudeep Holla wrote:

> On Mon, Sep 12, 2022 at 06:58:49PM +0100, Geert Uytterhoeven wrote:
> > Hi Sudeep,
> > 
> > CC Dien Pham
> > 
> > On Mon, Sep 12, 2022 at 6:49 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Sep 9, 2022 at 4:51 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > 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:
> > > > > > > > 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.
> > >
> > > I'm not aware of Renesas platforms using SCMI...
> > 
> > Upon closer look, Diep Pham did report a build issue in the SCMI code, so
> > perhaps Diep knows more...
> > 
> 
> Yes indeed, Diep Pham tested the original patch IIRC and also has reported
> few bugs in SCMI clock code which are fixed. Hence I know it is used by
> Renesas.
> 
> Hi Peng,
> 
> Absence of DTS changes indicate nothing. I am aware of couple of vendors
> who use SCMI on several platforms and do report issues regularly and help
> in review of the code. So DTS is not a good indicator of SCMI usage
> unfortunately. On reason could be that since it is a firmware, bootloaders
> can detect and update DTS, just my thought and may differ from the reality.

Sorry for the delay.

This patch was indeed requested by Renesas for one of their platform 
that uses SCMI clocks. I didn't have access to the platform myself at 
the time but the patch was positively validated and tested by Renesas.

This works in conjunction with commit 0bfa0820c274 that made generic 
clock pm code usable with the SCMI layer.

I didn't touch any clock stuff since then and I forgot about the finer 
details unfortunately.


Nicolas

RE: Question: why call clk_prepare in pm_clk_acquire

[Peng Fan]

> Subject: Re: Question: why call clk_prepare in pm_clk_acquire
> 
> On Mon, Sep 12, 2022 at 06:58:49PM +0100, Geert Uytterhoeven wrote:
> > Hi Sudeep,
> >
> > CC Dien Pham
> >
> > On Mon, Sep 12, 2022 at 6:49 PM Geert Uytterhoeven <geert@linux-
> m68k.org> wrote:
> > > On Fri, Sep 9, 2022 at 4:51 PM Sudeep Holla <sudeep.holla@arm.com>
> wrote:
> > > > 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:
> > > > > > > > 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.
> > >
> > > I'm not aware of Renesas platforms using SCMI...
> >
> > Upon closer look, Diep Pham did report a build issue in the SCMI code,
> > so perhaps Diep knows more...
> >
> 
> Yes indeed, Diep Pham tested the original patch IIRC and also has reported
> few bugs in SCMI clock code which are fixed. Hence I know it is used by
> Renesas.
> 
> Hi Peng,
> 
> Absence of DTS changes indicate nothing. I am aware of couple of vendors
> who use SCMI on several platforms and do report issues regularly and help
> in review of the code. So DTS is not a good indicator of SCMI usage
> unfortunately. On reason could be that since it is a firmware, bootloaders
> can detect and update DTS, just my thought and may differ from the reality.

Could we make the GENPD_FLAG_PM_CLK as a optional flag as Ulf suggested?
Such as non scmi clk platforms not require this flag, or any other suggestion?

Regards,
Peng.

> 
> --
> Regards,
> Sudeep

Re: Question: why call clk_prepare in pm_clk_acquire

[Sudeep Holla]

Hi Peng,

On Thu, Sep 15, 2022 at 12:59:32AM +0000, Peng Fan wrote:
> > Subject: Re: Question: why call clk_prepare in pm_clk_acquire
> > 
> > On Mon, Sep 12, 2022 at 06:58:49PM +0100, Geert Uytterhoeven wrote:
> > > Hi Sudeep,
> > >
> > > CC Dien Pham
> > >
> > > On Mon, Sep 12, 2022 at 6:49 PM Geert Uytterhoeven <geert@linux-
> > m68k.org> wrote:
> > > > On Fri, Sep 9, 2022 at 4:51 PM Sudeep Holla <sudeep.holla@arm.com>
> > wrote:
> > > > > 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:
> > > > > > > > > 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.
> > > >
> > > > I'm not aware of Renesas platforms using SCMI...
> > >
> > > Upon closer look, Diep Pham did report a build issue in the SCMI code,
> > > so perhaps Diep knows more...
> > >
> > 
> > Yes indeed, Diep Pham tested the original patch IIRC and also has reported
> > few bugs in SCMI clock code which are fixed. Hence I know it is used by
> > Renesas.
> > 
> > Hi Peng,
> > 
> > Absence of DTS changes indicate nothing. I am aware of couple of vendors
> > who use SCMI on several platforms and do report issues regularly and help
> > in review of the code. So DTS is not a good indicator of SCMI usage
> > unfortunately. On reason could be that since it is a firmware, bootloaders
> > can detect and update DTS, just my thought and may differ from the reality.
> 
> Could we make the GENPD_FLAG_PM_CLK as a optional flag as Ulf suggested?
> Such as non scmi clk platforms not require this flag, or any other suggestion?
> 

I agreed to make it conditional, but what that "condition" must be is
something I don't know as I don't understand it. What I want to avoid it
to make it platform dependent(using some platform specific compatible) in
the generic SCMI PM domain driver if possible.

Hi Dien,

If you could confirm that there are Renesas platforms dependent on this,
that would be great. We could simply revert the original commit if there
is no more use for this instead of finding other ways to fix the issue
Peng has reported.

-- 
Regards,
Sudeep

Re: Question: why call clk_prepare in pm_clk_acquire

[Ulf Hansson]

On Wed, 14 Sept 2022 at 19:05, Nicolas Pitre <nico@fluxnic.net> wrote:
>
> On Wed, 14 Sep 2022, Sudeep Holla wrote:
>
> > On Mon, Sep 12, 2022 at 06:58:49PM +0100, Geert Uytterhoeven wrote:
> > > Hi Sudeep,
> > >
> > > CC Dien Pham
> > >
> > > On Mon, Sep 12, 2022 at 6:49 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Fri, Sep 9, 2022 at 4:51 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > 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:
> > > > > > > > > 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.
> > > >
> > > > I'm not aware of Renesas platforms using SCMI...
> > >
> > > Upon closer look, Diep Pham did report a build issue in the SCMI code, so
> > > perhaps Diep knows more...
> > >
> >
> > Yes indeed, Diep Pham tested the original patch IIRC and also has reported
> > few bugs in SCMI clock code which are fixed. Hence I know it is used by
> > Renesas.
> >
> > Hi Peng,
> >
> > Absence of DTS changes indicate nothing. I am aware of couple of vendors
> > who use SCMI on several platforms and do report issues regularly and help
> > in review of the code. So DTS is not a good indicator of SCMI usage
> > unfortunately. On reason could be that since it is a firmware, bootloaders
> > can detect and update DTS, just my thought and may differ from the reality.
>
> Sorry for the delay.
>
> This patch was indeed requested by Renesas for one of their platform
> that uses SCMI clocks. I didn't have access to the platform myself at
> the time but the patch was positively validated and tested by Renesas.
>
> This works in conjunction with commit 0bfa0820c274 that made generic
> clock pm code usable with the SCMI layer.
>
> I didn't touch any clock stuff since then and I forgot about the finer
> details unfortunately.

Thanks Nico for coming back with this information. To me, it looks
like the patch may be applicable to some Renesas' downstream kernel
then.

In my opinion I think we should rather try to revert, to avoid any
further problems. So I am going to send that patch and see what people
think about it.

Another option, which Sudeep doesn't seem very happy about too, is to
make the GENPD_FLAG_PM_CLK conditional, based on a platform
compatible.

Kind regards
Uffe

Re: Question: why call clk_prepare in pm_clk_acquire

[Sudeep Holla]

On Mon, Sep 19, 2022 at 11:53:18AM +0200, Ulf Hansson wrote:
> On Wed, 14 Sept 2022 at 19:05, Nicolas Pitre <nico@fluxnic.net> wrote:
> >
> > On Wed, 14 Sep 2022, Sudeep Holla wrote:
> >
> > > On Mon, Sep 12, 2022 at 06:58:49PM +0100, Geert Uytterhoeven wrote:
> > > > Hi Sudeep,
> > > >
> > > > CC Dien Pham
> > > >
> > > > On Mon, Sep 12, 2022 at 6:49 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Fri, Sep 9, 2022 at 4:51 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > > 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:
> > > > > > > > > > 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.
> > > > >
> > > > > I'm not aware of Renesas platforms using SCMI...
> > > >
> > > > Upon closer look, Diep Pham did report a build issue in the SCMI code, so
> > > > perhaps Diep knows more...
> > > >
> > >
> > > Yes indeed, Diep Pham tested the original patch IIRC and also has reported
> > > few bugs in SCMI clock code which are fixed. Hence I know it is used by
> > > Renesas.
> > >
> > > Hi Peng,
> > >
> > > Absence of DTS changes indicate nothing. I am aware of couple of vendors
> > > who use SCMI on several platforms and do report issues regularly and help
> > > in review of the code. So DTS is not a good indicator of SCMI usage
> > > unfortunately. On reason could be that since it is a firmware, bootloaders
> > > can detect and update DTS, just my thought and may differ from the reality.
> >
> > Sorry for the delay.
> >
> > This patch was indeed requested by Renesas for one of their platform
> > that uses SCMI clocks. I didn't have access to the platform myself at
> > the time but the patch was positively validated and tested by Renesas.
> >
> > This works in conjunction with commit 0bfa0820c274 that made generic
> > clock pm code usable with the SCMI layer.
> >
> > I didn't touch any clock stuff since then and I forgot about the finer
> > details unfortunately.
> 
> Thanks Nico for coming back with this information. To me, it looks
> like the patch may be applicable to some Renesas' downstream kernel
> then.
>

Though I agree in most of the case, I am not sure in this particular
case as they may not need any downstream kernel changes for SCMI. All
they need is DT nodes.

> In my opinion I think we should rather try to revert, to avoid any
> further problems. So I am going to send that patch and see what people
> think about it.
>

Since I see absolute silence from Renesas, I am happy to revert if no
one has any objections.

> Another option, which Sudeep doesn't seem very happy about too, is to
> make the GENPD_FLAG_PM_CLK conditional, based on a platform
> compatible.

Correct, I would rather make it generic based on clock flags like in this
case it is CLK_SET_PARENT_GATE or CLK_SET_RATE_GATE or something right ?

-- 
Regards,
Sudeep

Re: Question: why call clk_prepare in pm_clk_acquire

[Ulf Hansson]

On Wed, 21 Sept 2022 at 16:42, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Mon, Sep 19, 2022 at 11:53:18AM +0200, Ulf Hansson wrote:
> > On Wed, 14 Sept 2022 at 19:05, Nicolas Pitre <nico@fluxnic.net> wrote:
> > >
> > > On Wed, 14 Sep 2022, Sudeep Holla wrote:
> > >
> > > > On Mon, Sep 12, 2022 at 06:58:49PM +0100, Geert Uytterhoeven wrote:
> > > > > Hi Sudeep,
> > > > >
> > > > > CC Dien Pham
> > > > >
> > > > > On Mon, Sep 12, 2022 at 6:49 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > On Fri, Sep 9, 2022 at 4:51 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > > > 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:
> > > > > > > > > > > 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.
> > > > > >
> > > > > > I'm not aware of Renesas platforms using SCMI...
> > > > >
> > > > > Upon closer look, Diep Pham did report a build issue in the SCMI code, so
> > > > > perhaps Diep knows more...
> > > > >
> > > >
> > > > Yes indeed, Diep Pham tested the original patch IIRC and also has reported
> > > > few bugs in SCMI clock code which are fixed. Hence I know it is used by
> > > > Renesas.
> > > >
> > > > Hi Peng,
> > > >
> > > > Absence of DTS changes indicate nothing. I am aware of couple of vendors
> > > > who use SCMI on several platforms and do report issues regularly and help
> > > > in review of the code. So DTS is not a good indicator of SCMI usage
> > > > unfortunately. On reason could be that since it is a firmware, bootloaders
> > > > can detect and update DTS, just my thought and may differ from the reality.
> > >
> > > Sorry for the delay.
> > >
> > > This patch was indeed requested by Renesas for one of their platform
> > > that uses SCMI clocks. I didn't have access to the platform myself at
> > > the time but the patch was positively validated and tested by Renesas.
> > >
> > > This works in conjunction with commit 0bfa0820c274 that made generic
> > > clock pm code usable with the SCMI layer.
> > >
> > > I didn't touch any clock stuff since then and I forgot about the finer
> > > details unfortunately.
> >
> > Thanks Nico for coming back with this information. To me, it looks
> > like the patch may be applicable to some Renesas' downstream kernel
> > then.
> >
>
> Though I agree in most of the case, I am not sure in this particular
> case as they may not need any downstream kernel changes for SCMI. All
> they need is DT nodes.

Right, good point!

>
> > In my opinion I think we should rather try to revert, to avoid any
> > further problems. So I am going to send that patch and see what people
> > think about it.
> >
>
> Since I see absolute silence from Renesas, I am happy to revert if no
> one has any objections.

Okay, good!

>
> > Another option, which Sudeep doesn't seem very happy about too, is to
> > make the GENPD_FLAG_PM_CLK conditional, based on a platform
> > compatible.
>
> Correct, I would rather make it generic based on clock flags like in this
> case it is CLK_SET_PARENT_GATE or CLK_SET_RATE_GATE or something right ?

That needs some more thinking, but potentially it could work - at
least for this particular case with clk_set_rate().

Although, as it's likely that the subsystem/driver is already handling
the clock(s), the whole thing with using GENPD_FLAG_PM_CLK is in most
cases superfluous. That means we end up running unnecessary code-paths
during runtime suspend/resume (to ungate/gate clock(s) twice) - and
that involves acquiring/releasing locks too, when that isn't really
needed.

Kind regards
Uffe