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