On 08/03/2022 23:46, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2022-03-03 23:58:58)
On Fri, 4 Mar 2022 at 07:31, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
Quoting Dmitry Baryshkov (2022-03-03 20:23:06)
On Fri, 4 Mar 2022 at 01:32, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
Quoting Dmitry Baryshkov (2022-02-16 21:55:27)
The only clock for which we set the rate is the "stream_pixel". Rather
than storing the rate and then setting it by looping over all the
clocks, set the clock rate directly.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
[...]
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 07f6bf7e1acb..8e6361dedd77 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1315,7 +1315,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
DRM_DEBUG_DP("setting rate=%lu on clk=%s\n", rate, name);
if (num)
- cfg->rate = rate;
+ clk_set_rate(cfg->clk, rate);
This looks bad. From what I can tell we set the rate of the pixel clk
after enabling the phy and configuring it. See the order of operations
in dp_ctrl_enable_mainlink_clocks() and note how dp_power_clk_enable()
is the one that eventually sets a rate through dp_power_clk_set_rate()
dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link",
ctrl->link->link_params.rate * 1000);
phy_configure(phy, &dp_io->phy_opts);
phy_power_on(phy);
ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, true);
This code has been changed in the previous patch.
Let's get back a bit.
Currently dp_ctrl_set_clock_rate() doesn't change the clock rate. It
just stores the rate in the config so that later the sequence of
dp_power_clk_enable() -> dp_power_clk_set_rate() ->
[dp_power_clk_set_link_rate() -> dev_pm_opp_set_rate() or
msm_dss_clk_set_rate() -> clk_set_rate()] will use that.
There are only two users of dp_ctrl_set_clock_rate():
- dp_ctrl_enable_mainlink_clocks(), which you have quoted above.
This case is handled in the patch 1 from this series. It makes
Patch 1 form this series says DP is unaffected. Huh?
dp_ctrl_enable_mainlink_clocks() call dev_pm_opp_set_rate() directly
without storing (!) the rate in the config, calling
phy_configure()/phy_power_on() and then setting the opp via the
sequence of calls specified above
Note, this handles the "ctrl_link" clock.
- dp_ctrl_enable_stream_clocks(), which calls dp_power_clk_enable()
immediately afterwards. This call would set the stream_pixel rate
while enabling stream clocks. As far as I can see, the stream_pixel is
the only stream clock. So this patch sets the clock rate without
storing in the interim configuration data.
Could you please clarify, what exactly looks bad to you?
Note, this handles the "stream_pixel" clock.
I'm concerned about the order of operations changing between the
phy being powered on and the pixel clk frequency being set. From what I
recall the pixel clk rate operations depend on the phy frequency being
set (which is done through phy_configure?) so if we call clk_set_rate()
on the pixel clk before the phy is set then the clk frequency will be
calculated badly and probably be incorrect.
But the order of operations is mostly unchanged. The only major change
is that the opp point is now set before calling the
phy_configure()/phy_power_on()
Yes that's my concern. The qmp phy driver has a couple clk_set_rate()
calls in the .configure_dp_phy callback. That is called from
phy_power_on() (see qcom_qmp_phy_power_on() and qcom_qmp_phy_dp_ops).
Looking at qcom_qmp_v3_phy_configure_dp_phy() it does
clk_set_rate(dp_clks->dp_link_hw.clk, dp_opts->link_rate * 100000);
clk_set_rate(dp_clks->dp_pixel_hw.clk, pixel_freq);
and I believe the child of dp_pixel_hw is find_clock("stream_pixel").
Looks like that is DISP_CC_MDSS_DP_PIXEL_CLK which is
disp_cc_mdss_dp_pixel_clk_src for the rate settable part. That has
clk_dp_ops which is clk_rcg2_dp_set_rate() for the set rate part. That
wants the parent clk frequency to be something non-zero to use in
rational_best_approximation(). If the clk_set_rate("stream_pixel") call
is made before phy_power_on() then the parent_rate in
clk_rcg2_dp_set_rate() won't be valid and the pixel clk frequency will
be wrong.
Please excuse me, I didn't have time for this patchset up to now.
I double checked the previous patch (drm/msm/dp: "inline"
dp_ctrl_set_clock_rate("ctrl_link")). Note, that the OPP is set _after_
the PHY is powered on and configured.
Does that cover your concerns?
--
With best wishes
Dmitry
