At the moment, clk-smd-rpm forces all clocks on at probe time
(for "handoff"). However, it does not make the clk core aware of that.
This means that the clocks stay enabled forever if they are not used
We can easily disable them again after bootup has been completed,
by making the clk core aware of the state. This is implemented by
returning the current state of the clock in is_prepared().
Checking the SPMI clock registers reveals that this allows the RPM
to disable unused BB/RF clocks. For example, on MSM8916 with all
remote processors (except RPM) disabled, we get:
| | BOOTLOADER | HANDOFF | BEFORE | AFTER |
| BB_CLK1_STATUS1 (0x5108) | ON* | ON | ON | ON* |
| BB_CLK2_STATUS1 (0x5208) | OFF | ON | ON | OFF |
| RF_CLK1_STATUS1 (0x5408) | OFF | ON | ON | OFF |
| RF_CLK2_STATUS1 (0x5508) | OFF | ON | ON | OFF |
* BB_CLK1 seems to be always-on in RPM on MSM8916
- BOOTLOADER = clk-smd-rpm disabled entirely in device tree
- HANDOFF = temporarily after clk-smd-rpm was probed
- BEFORE/AFTER = after boot without/with the changes in this commit
With this commit BB_CLK2/RF_CLK1/RF_CLK2 are disabled again when unused.
Cc: Georgi Djakov <email@example.com>
Signed-off-by: Stephan Gerhold <firstname.lastname@example.org>
Originally I reported this here:
Overall I'm not entirely sure why we need to force all these clocks
on at all... But the downstream driver also seems to do it and the RPM
interface is barely documented, so I didn't feel comfortable changing it...
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....).
As far as I can tell the behavior implemented in this patch (= force
clocks on during boot but disable them when unused) is the same on that
is used on the downstream kernel. Most FW is probably written with the
downstream kernel in mind, so I don't think this is going to cause trouble.
The only situation this patch could break something is if we forgot to
manage the clocks for one of the devices in mainline
(and implicitly relied on clk-smd-rpm to keep them always-on).
For example, one situation I checked is for WCNSS on MSM8916.
It seems to require RF_CLK2 to boot. However, this is already handled in
qcom_wcnss_iris.c where the clock is forced on until WCNSS is ready.
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
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.
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.
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.
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?
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?
Sorry for the rant and thanks for sending a patch to fix problems you're
Also, out of curiosity, how are you validating that BB_CLK2 is
actually off after this change?