On Wed, 21 Sept 2022 at 16:42, Sudeep Holla <sudeep.holla@xxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx> wrote:
> > > > > > 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...
> > > > >
> > > > > 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
