Re: [PATCH] clk: qcom: smd: Disable unused clocks

[Stephan Gerhold <stephan@gerhold.net>]

On Thu, Aug 20, 2020 at 04:27:35PM -0700, Stephen Boyd wrote:
> Quoting Stephan Gerhold (2020-08-18 01:07:38)
> > Hi Jeffrey,
> > 
> > On Mon, Aug 17, 2020 at 08:52:46AM -0600, Jeffrey Hugo wrote:
> > > So essentially, when the clk framework goes through late init, and
> > > decides to turn off clocks that are not being used, it will also turn
> > > off these clocks?
> > > 
> > > I think this is going to break other targets where other subsystems
> > > happen to rely on these sorts of votes from Linux inorder to run/boot
> > > (not saying it's a good thing, just that is how it is and since we
> > > can't change the FW on those....).
> > > 
> > 
> > After thinking about it some more I realized there are definitely some
> > of the clocks we shouldn't disable even when unused, for example the
> > interconnect clocks. With interconnect drivers disabled the system
> > basically locked up entirely once the clock core disabled the clocks.
> 
> The interconnect clks should be moved out of the RPM clk drivers. It's
> over-engineering to have the RPM interconnect drivers talk to the RPM
> clk drivers to change the frequency of interconnects based on a
> calculation in the interconnect driver. It makes sense from a logical
> perspective to express that these are clks, and they have frequencies,
> etc., but when you look closer at it you see that it's nonsense.
> 
> The RPMh interconnect driver should be able to talk directly to the RPM

Note: I'm talking about the clk-smd-rpm (+ SMD RPM interconnect)
drivers here (not RPMh), but I guess the same applies for that as well.

> and turn knobs as it see fit. Nobody else is going to use those clks
> from the RPM clk driver. Any potential consumer is going to go through
> the interconnect layer to change frequencies. So now we've got two
> frameworks interfacing with the same wire protocol and remote processor,
> when we could have only one. And furthermore to the point, the RPM
> interconnect clks are all parented to nothing, so putting them behind
> the clk APIs provides practically zero benefits, like managing the clk
> tree or determining rates down the tree.
> 

That makes sense to me since it would also prevent these clocks from
getting "disabled" when unused.

> Honestly I'd like to see all the various RPM drivers combined into one
> driver that knows what is going on between regulators, interconnects,
> and clks, etc. Carving it up into these different drivers spread across
> the tree helps us review the code and logically split the device into
> pieces, but the flip side is that nobody sees the big picture that a
> call into a framework here boils down to a couple RPM messages sent over
> the wire to the same device.
> 

I'm not sure, most of the RPM drivers are basically pretty much just
long struct definitions for the available regulators, clks,
interconnects, ... of one SoC. Combining this all into one driver would
likely become rather "messy"...

> > For now I fixed this by marking all of DEFINE_CLK_SMD_RPM() as
> > CLK_IGNORE_UNUSED (essentially restoring the current behavior of the
> > driver). For MSM8916 these are exactly the interconnect clocks, but on
> > other platforms there are further clocks that might not need
> > CLK_IGNORE_UNUSED. This could be still optimized later.
> > 
> > > I think this needs to be validated on every single qcom platform using
> > > this driver.
> > > 
> > 
> > After running into the issue above I kind of agree with you. While
> > problems should be limited by marking the "rate" clocks as
> > CLK_IGNORE_UNUSED, it's also possible that one of the platforms requires
> > one of the branch clocks to stay on to boot successfully.
> > 
> > I know for sure that this works properly on MSM8916, so maybe I should
> > make it opt-in and then we add it for each platform after validating it?
> > 
> 
> No! Instead of putting band-aids on this broken mess, please just move
> the interconnect clks out of the clk driver and into the interconnect
> driver.
> 

Hmm, I'm not sure how to implement this in a backwards compatible way.
In particular, clk-smd-rpm supports SoCs that:

  - Have no interconnect driver at all (MSM8998, SDM660, ...)
  - Had the interconnect device nodes added much later than rpmcc
    (MSM8916, MSM8974, ...)
  - Have some clocks that are not covered by the interconnect drivers yet,
    e.g. RPM_SMD_GFX3D_CLK_SRC (Is this even interconnect? Not sure...)

In all those cases they likely (implicitly) rely on rpmcc to keep all
clocks at maximum rate, even if unused. If we were to move the
interconnect clock management to the interconnect drivers, we would
still need to handle the clocks somewhere in all those cases.

How would this work?

Thanks,
Stephan