| Message ID | 20200817140908.185976-1-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: qcom: smd: Disable unused clocks | expand |
On Mon, Aug 17, 2020 at 8:10 AM Stephan Gerhold <stephan@gerhold.net> wrote: > > 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 > by anything. > > 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 > > where: > - 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 <georgi.djakov@linaro.org> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > --- > Originally I reported this here: > https://lore.kernel.org/linux-arm-msm/20200523120810.GA166540@gerhold.net/ > > 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... 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....). I think this needs to be validated on every single qcom platform using this driver. Also, out of curiosity, how are you validating that BB_CLK2 is actually off after this change? > --- > drivers/clk/qcom/clk-smd-rpm.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c > index 0e1dfa89489e..fa960cb794a8 100644 > --- a/drivers/clk/qcom/clk-smd-rpm.c > +++ b/drivers/clk/qcom/clk-smd-rpm.c > @@ -171,6 +171,9 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r) > if (ret) > return ret; > > + /* During handoff we force all clocks on */ > + r->enabled = true; > + > return 0; > } > > @@ -300,6 +303,13 @@ static void clk_smd_rpm_unprepare(struct clk_hw *hw) > mutex_unlock(&rpm_smd_clk_lock); > } > > +static int clk_smd_rpm_is_prepared(struct clk_hw *hw) > +{ > + struct clk_smd_rpm *r = to_clk_smd_rpm(hw); > + > + return r->enabled; > +} > + > static int clk_smd_rpm_set_rate(struct clk_hw *hw, unsigned long rate, > unsigned long parent_rate) > { > @@ -396,6 +406,7 @@ static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm) > static const struct clk_ops clk_smd_rpm_ops = { > .prepare = clk_smd_rpm_prepare, > .unprepare = clk_smd_rpm_unprepare, > + .is_prepared = clk_smd_rpm_is_prepared, > .set_rate = clk_smd_rpm_set_rate, > .round_rate = clk_smd_rpm_round_rate, > .recalc_rate = clk_smd_rpm_recalc_rate, > @@ -404,6 +415,7 @@ static const struct clk_ops clk_smd_rpm_ops = { > static const struct clk_ops clk_smd_rpm_branch_ops = { > .prepare = clk_smd_rpm_prepare, > .unprepare = clk_smd_rpm_unprepare, > + .is_prepared = clk_smd_rpm_is_prepared, > }; > > /* msm8916 */ > -- > 2.28.0 >
On Mon, Aug 17, 2020 at 08:52:46AM -0600, Jeffrey Hugo wrote: > > 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... > > 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? > With this patch: yes. > 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. > I think this needs to be validated on every single qcom platform using > this driver. > > Also, out of curiosity, how are you validating that BB_CLK2 is > actually off after this change? > Since BB_CLK1/2 and RF_CLK1/2 are part of the PMIC (at least on MSM8916) I used the regmap debugfs interface to read the clock registers through SPMI from Linux. From the "PM8916 Hardware Register Description" [1] I got the registers mentioned in the table, e.g. for BB_CLK2: 0x5208: BB_CLK2_STATUS1 BIT(7): CLK_OK (Indicates Hardware or Software enable and includes warmup delay) 0x0: BBCLK_OFF 0x1: BBCLK_ON I read the registers from /sys/kernel/debug/regmap/0-00/registers: Without this patch: 5108: 80 5208: 80 5408: 80 5508: 80 With this patch (and with clk-smd-rpm entirely disabled): 5108: 80 5208: 00 5408: 00 5508: 00 Stephan [1]: https://developer.qualcomm.com/download/sd410/pm8916-hardware-register-description.pdf
On Mon, Aug 17, 2020 at 9:29 AM Stephan Gerhold <stephan@gerhold.net> wrote: > > On Mon, Aug 17, 2020 at 08:52:46AM -0600, Jeffrey Hugo wrote: > > > 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... > > > > 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? > > > > With this patch: yes. > > > 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. Based on my experience with 8998, I disagree. I would need to dig up the history for specifics. > > 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. > > > I think this needs to be validated on every single qcom platform using > > this driver. > > > > Also, out of curiosity, how are you validating that BB_CLK2 is > > actually off after this change? > > > > Since BB_CLK1/2 and RF_CLK1/2 are part of the PMIC (at least on MSM8916) > I used the regmap debugfs interface to read the clock registers > through SPMI from Linux. > > From the "PM8916 Hardware Register Description" [1] I got the registers > mentioned in the table, e.g. for BB_CLK2: > > 0x5208: BB_CLK2_STATUS1 > BIT(7): CLK_OK (Indicates Hardware or Software enable and > includes warmup delay) > 0x0: BBCLK_OFF > 0x1: BBCLK_ON > > I read the registers from /sys/kernel/debug/regmap/0-00/registers: > > Without this patch: > 5108: 80 > 5208: 80 > 5408: 80 > 5508: 80 > > With this patch (and with clk-smd-rpm entirely disabled): > 5108: 80 > 5208: 00 > 5408: 00 > 5508: 00 > > Stephan > > [1]: https://developer.qualcomm.com/download/sd410/pm8916-hardware-register-description.pdf Hmm, 8916 is probably old enough where you can actually do that. For the modern SoCs, you'll have to go through jtag to get an accurate view of the clocks.
On Mon, Aug 17, 2020 at 09:46:08AM -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? > > > > > > > With this patch: yes. > > > > > 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. > > Based on my experience with 8998, I disagree. I would need to dig up > the history for specifics. > I don't know anything about 8998, so it's possible. My statement was based on a quick look at the downstream code: For some reason there is an entirely separate MSM clock framework downstream: 1. During msm_clock_register() [1] it calls __handoff_clk() for all the clocks. 2. __handoff_clk() [2] calls clk->ops->handoff(clk) and if that returns success (HANDOFF_ENABLED_CLK) it adds the clock to a "handoff_list". -> rpm_clk_handoff() [3] forces the clock on similar to mainline. 3. In a late init call (clock_late_init()) [4] it iterates over "handoff_list" and reduces the prepare_count again and eventually disables the clock. In this patch I implement something equivalent to (3). [1]: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/clk/msm/clock.c?h=LA.UM.7.2.r2-06200-8x98.0#n985 [2]: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/clk/msm/clock.c?h=LA.UM.7.2.r2-06200-8x98.0#n873 [3]: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/clk/msm/clock-rpm.c?h=LA.UM.7.2.r2-06200-8x98.0#n263 [4]: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/clk/msm/clock.c?h=LA.UM.7.2.r2-06200-8x98.0#n1351 > > > > > Also, out of curiosity, how are you validating that BB_CLK2 is > > > actually off after this change? > > > > > > > Since BB_CLK1/2 and RF_CLK1/2 are part of the PMIC (at least on MSM8916) > > I used the regmap debugfs interface to read the clock registers > > through SPMI from Linux. > > > > From the "PM8916 Hardware Register Description" [1] I got the registers > > mentioned in the table, e.g. for BB_CLK2: > > > > 0x5208: BB_CLK2_STATUS1 > > BIT(7): CLK_OK (Indicates Hardware or Software enable and > > includes warmup delay) > > 0x0: BBCLK_OFF > > 0x1: BBCLK_ON > > > > I read the registers from /sys/kernel/debug/regmap/0-00/registers: > > > > Without this patch: > > 5108: 80 > > 5208: 80 > > 5408: 80 > > 5508: 80 > > > > With this patch (and with clk-smd-rpm entirely disabled): > > 5108: 80 > > 5208: 00 > > 5408: 00 > > 5508: 00 > > > > Stephan > > > > [1]: https://developer.qualcomm.com/download/sd410/pm8916-hardware-register-description.pdf > > Hmm, 8916 is probably old enough where you can actually do that. For > the modern SoCs, you'll have to go through jtag to get an accurate > view of the clocks. I guess I was lucky then :) Stephan
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. 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? Stephan
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 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. > > > 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. Sorry for the rant and thanks for sending a patch to fix problems you're seeing.
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
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c index 0e1dfa89489e..fa960cb794a8 100644 --- a/drivers/clk/qcom/clk-smd-rpm.c +++ b/drivers/clk/qcom/clk-smd-rpm.c @@ -171,6 +171,9 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r) if (ret) return ret; + /* During handoff we force all clocks on */ + r->enabled = true; + return 0; } @@ -300,6 +303,13 @@ static void clk_smd_rpm_unprepare(struct clk_hw *hw) mutex_unlock(&rpm_smd_clk_lock); } +static int clk_smd_rpm_is_prepared(struct clk_hw *hw) +{ + struct clk_smd_rpm *r = to_clk_smd_rpm(hw); + + return r->enabled; +} + static int clk_smd_rpm_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate) { @@ -396,6 +406,7 @@ static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm) static const struct clk_ops clk_smd_rpm_ops = { .prepare = clk_smd_rpm_prepare, .unprepare = clk_smd_rpm_unprepare, + .is_prepared = clk_smd_rpm_is_prepared, .set_rate = clk_smd_rpm_set_rate, .round_rate = clk_smd_rpm_round_rate, .recalc_rate = clk_smd_rpm_recalc_rate, @@ -404,6 +415,7 @@ static const struct clk_ops clk_smd_rpm_ops = { static const struct clk_ops clk_smd_rpm_branch_ops = { .prepare = clk_smd_rpm_prepare, .unprepare = clk_smd_rpm_unprepare, + .is_prepared = clk_smd_rpm_is_prepared, }; /* msm8916 */
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 by anything. 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 where: - 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 <georgi.djakov@linaro.org> Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- Originally I reported this here: https://lore.kernel.org/linux-arm-msm/20200523120810.GA166540@gerhold.net/ 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... --- drivers/clk/qcom/clk-smd-rpm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)