From: Rajendra Nayak <rnayak@codeaurora.org> To: Stephen Boyd <swboyd@chromium.org>, Kuogee Hsieh <khsieh@codeaurora.org>, robdclark@gmail.com, sean@poorly.run Cc: tanmay@codeaurora.org, abhinavk@codeaurora.org, aravindh@codeaurora.org, airlied@linux.ie, daniel@ffwll.ch, linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/msm/dp: add voltage corners voting support base on dp link rate Date: Wed, 30 Sep 2020 21:54:57 +0530 [thread overview] Message-ID: <01467014-1190-6e1b-8120-472719861a5e@codeaurora.org> (raw) In-Reply-To: <160145429763.310579.786737478429183087@swboyd.mtv.corp.google.com> On 9/30/2020 1:54 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2020-09-29 10:10:26) >> Set link rate by using OPP set rate api so that CX level will be set >> accordingly base on the link rate. > > s/base/based/ > >> >> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org> >> --- >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> index 2e3e1917351f..e1595d829e04 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> @@ -1849,6 +1853,21 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link, >> return ERR_PTR(-ENOMEM); >> } >> >> + ctrl->opp_table = dev_pm_opp_set_clkname(dev, "ctrl_link"); I see that downstream has multiple DP clocks which end up voting on CX, we don't have a way of associating multiple OPP tables with a device upstream, so whats usually done is (assuming all the clocks get scaled in lock step, which I assume is the case here) we pick the clock with the 'highest' CX requirement and associate that with the OPP table. I haven't looked but I am hoping thats how we have decided to associate "ctrl_link" clock here? >> + >> + if (IS_ERR(ctrl->opp_table)) { >> + dev_err(dev, "invalid DP OPP table in device tree\n"); >> + ctrl->opp_table = NULL; >> + } else { >> + /* OPP table is optional */ >> + ret = dev_pm_opp_of_add_table(dev); >> + if (ret && ret != -ENODEV) { >> + dev_err(dev, "add DP OPP table\n"); > > This is debug noise right? > >> + dev_pm_opp_put_clkname(ctrl->opp_table); >> + ctrl->opp_table = NULL; >> + } >> + } >> + >> init_completion(&ctrl->idle_comp); >> init_completion(&ctrl->video_comp); >> >> @@ -1864,6 +1883,18 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link, >> return &ctrl->dp_ctrl; >> } >> >> -void dp_ctrl_put(struct dp_ctrl *dp_ctrl) >> +void dp_ctrl_put(struct device *dev, struct dp_ctrl *dp_ctrl) >> { >> + struct dp_ctrl_private *ctrl; >> + >> + if (!dp_ctrl) > > Can this happen? > >> + return; >> + >> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl); >> + >> + if (ctrl->opp_table != NULL) { > > This is usually written as > > if (ctrl->opp_table) > >> + dev_pm_opp_of_remove_table(dev); >> + dev_pm_opp_put_clkname(ctrl->opp_table); >> + ctrl->opp_table = NULL; >> + } >> } >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h >> index f60ba93c8678..19b412a93e02 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h >> @@ -31,6 +31,6 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link, >> struct dp_panel *panel, struct drm_dp_aux *aux, >> struct dp_power *power, struct dp_catalog *catalog, >> struct dp_parser *parser); >> -void dp_ctrl_put(struct dp_ctrl *dp_ctrl); >> +void dp_ctrl_put(struct device *dev, struct dp_ctrl *dp_ctrl); > > Is 'dev' not inside 'dp_ctrl'? > >> >> #endif /* _DP_CTRL_H_ */ >> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c >> index 17c1fc6a2d44..3d75bf09e38f 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_power.c >> +++ b/drivers/gpu/drm/msm/dp/dp_power.c >> @@ -8,12 +8,14 @@ >> #include <linux/clk.h> >> #include <linux/clk-provider.h> >> #include <linux/regulator/consumer.h> >> +#include <linux/pm_opp.h> >> #include "dp_power.h" >> #include "msm_drv.h" >> >> struct dp_power_private { >> struct dp_parser *parser; >> struct platform_device *pdev; >> + struct device *dev; >> struct clk *link_clk_src; >> struct clk *pixel_provider; >> struct clk *link_provider; >> @@ -148,18 +150,49 @@ static int dp_power_clk_deinit(struct dp_power_private *power) >> return 0; >> } >> >> +static int dp_power_clk_set_link_rate(struct dp_power_private *power, >> + struct dss_clk *clk_arry, int num_clk, int enable) >> +{ >> + u32 rate; >> + int i, rc = 0; >> + >> + for (i = 0; i < num_clk; i++) { >> + if (clk_arry[i].clk) { >> + if (clk_arry[i].type == DSS_CLK_PCLK) { >> + if (enable) >> + rate = clk_arry[i].rate; >> + else >> + rate = 0; >> + >> + rc = dev_pm_opp_set_rate(power->dev, rate); > > Why do we keep going if rc is non-zero? > >> + } >> + >> + } >> + } >> + return rc; >> +} >> + >> static int dp_power_clk_set_rate(struct dp_power_private *power, >> enum dp_pm_type module, bool enable) >> { >> int rc = 0; >> struct dss_module_power *mp = &power->parser->mp[module]; >> >> - if (enable) { >> - rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk); >> + if (module == DP_CTRL_PM) { >> + rc = dp_power_clk_set_link_rate(power, mp->clk_config, mp->num_clk, enable); >> if (rc) { >> - DRM_ERROR("failed to set clks rate.\n"); >> + DRM_ERROR("failed to set link clks rate.\n"); >> return rc; >> } >> + } else { >> + >> + if (enable) { >> + rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk); >> + if (rc) { >> + DRM_ERROR("failed to set clks rate.\n"); > > Not sure we need the period on these error messages. > >> + return rc; >> + } >> + } >> } >> >> rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, enable); >> >> base-commit: 3c0f462da069af12211901ddf26f7e16e6951d9b >> prerequisite-patch-id: a109eaf08147f50149ad661a58122b6745a52445 > > Can you rebase this on Rob's msm-next tree > (https://gitlab.freedesktop.org/drm/msm.git) and test? It doesn't apply > for me because I have the dp phy patch from there. > -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
WARNING: multiple messages have this Message-ID (diff)
From: Rajendra Nayak <rnayak@codeaurora.org> To: Stephen Boyd <swboyd@chromium.org>, Kuogee Hsieh <khsieh@codeaurora.org>, robdclark@gmail.com, sean@poorly.run Cc: airlied@linux.ie, linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, abhinavk@codeaurora.org, tanmay@codeaurora.org, aravindh@codeaurora.org, freedreno@lists.freedesktop.org Subject: Re: [PATCH] drm/msm/dp: add voltage corners voting support base on dp link rate Date: Wed, 30 Sep 2020 21:54:57 +0530 [thread overview] Message-ID: <01467014-1190-6e1b-8120-472719861a5e@codeaurora.org> (raw) In-Reply-To: <160145429763.310579.786737478429183087@swboyd.mtv.corp.google.com> On 9/30/2020 1:54 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2020-09-29 10:10:26) >> Set link rate by using OPP set rate api so that CX level will be set >> accordingly base on the link rate. > > s/base/based/ > >> >> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org> >> --- >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> index 2e3e1917351f..e1595d829e04 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> @@ -1849,6 +1853,21 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link, >> return ERR_PTR(-ENOMEM); >> } >> >> + ctrl->opp_table = dev_pm_opp_set_clkname(dev, "ctrl_link"); I see that downstream has multiple DP clocks which end up voting on CX, we don't have a way of associating multiple OPP tables with a device upstream, so whats usually done is (assuming all the clocks get scaled in lock step, which I assume is the case here) we pick the clock with the 'highest' CX requirement and associate that with the OPP table. I haven't looked but I am hoping thats how we have decided to associate "ctrl_link" clock here? >> + >> + if (IS_ERR(ctrl->opp_table)) { >> + dev_err(dev, "invalid DP OPP table in device tree\n"); >> + ctrl->opp_table = NULL; >> + } else { >> + /* OPP table is optional */ >> + ret = dev_pm_opp_of_add_table(dev); >> + if (ret && ret != -ENODEV) { >> + dev_err(dev, "add DP OPP table\n"); > > This is debug noise right? > >> + dev_pm_opp_put_clkname(ctrl->opp_table); >> + ctrl->opp_table = NULL; >> + } >> + } >> + >> init_completion(&ctrl->idle_comp); >> init_completion(&ctrl->video_comp); >> >> @@ -1864,6 +1883,18 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link, >> return &ctrl->dp_ctrl; >> } >> >> -void dp_ctrl_put(struct dp_ctrl *dp_ctrl) >> +void dp_ctrl_put(struct device *dev, struct dp_ctrl *dp_ctrl) >> { >> + struct dp_ctrl_private *ctrl; >> + >> + if (!dp_ctrl) > > Can this happen? > >> + return; >> + >> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl); >> + >> + if (ctrl->opp_table != NULL) { > > This is usually written as > > if (ctrl->opp_table) > >> + dev_pm_opp_of_remove_table(dev); >> + dev_pm_opp_put_clkname(ctrl->opp_table); >> + ctrl->opp_table = NULL; >> + } >> } >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h >> index f60ba93c8678..19b412a93e02 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h >> @@ -31,6 +31,6 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link, >> struct dp_panel *panel, struct drm_dp_aux *aux, >> struct dp_power *power, struct dp_catalog *catalog, >> struct dp_parser *parser); >> -void dp_ctrl_put(struct dp_ctrl *dp_ctrl); >> +void dp_ctrl_put(struct device *dev, struct dp_ctrl *dp_ctrl); > > Is 'dev' not inside 'dp_ctrl'? > >> >> #endif /* _DP_CTRL_H_ */ >> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c >> index 17c1fc6a2d44..3d75bf09e38f 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_power.c >> +++ b/drivers/gpu/drm/msm/dp/dp_power.c >> @@ -8,12 +8,14 @@ >> #include <linux/clk.h> >> #include <linux/clk-provider.h> >> #include <linux/regulator/consumer.h> >> +#include <linux/pm_opp.h> >> #include "dp_power.h" >> #include "msm_drv.h" >> >> struct dp_power_private { >> struct dp_parser *parser; >> struct platform_device *pdev; >> + struct device *dev; >> struct clk *link_clk_src; >> struct clk *pixel_provider; >> struct clk *link_provider; >> @@ -148,18 +150,49 @@ static int dp_power_clk_deinit(struct dp_power_private *power) >> return 0; >> } >> >> +static int dp_power_clk_set_link_rate(struct dp_power_private *power, >> + struct dss_clk *clk_arry, int num_clk, int enable) >> +{ >> + u32 rate; >> + int i, rc = 0; >> + >> + for (i = 0; i < num_clk; i++) { >> + if (clk_arry[i].clk) { >> + if (clk_arry[i].type == DSS_CLK_PCLK) { >> + if (enable) >> + rate = clk_arry[i].rate; >> + else >> + rate = 0; >> + >> + rc = dev_pm_opp_set_rate(power->dev, rate); > > Why do we keep going if rc is non-zero? > >> + } >> + >> + } >> + } >> + return rc; >> +} >> + >> static int dp_power_clk_set_rate(struct dp_power_private *power, >> enum dp_pm_type module, bool enable) >> { >> int rc = 0; >> struct dss_module_power *mp = &power->parser->mp[module]; >> >> - if (enable) { >> - rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk); >> + if (module == DP_CTRL_PM) { >> + rc = dp_power_clk_set_link_rate(power, mp->clk_config, mp->num_clk, enable); >> if (rc) { >> - DRM_ERROR("failed to set clks rate.\n"); >> + DRM_ERROR("failed to set link clks rate.\n"); >> return rc; >> } >> + } else { >> + >> + if (enable) { >> + rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk); >> + if (rc) { >> + DRM_ERROR("failed to set clks rate.\n"); > > Not sure we need the period on these error messages. > >> + return rc; >> + } >> + } >> } >> >> rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, enable); >> >> base-commit: 3c0f462da069af12211901ddf26f7e16e6951d9b >> prerequisite-patch-id: a109eaf08147f50149ad661a58122b6745a52445 > > Can you rebase this on Rob's msm-next tree > (https://gitlab.freedesktop.org/drm/msm.git) and test? It doesn't apply > for me because I have the dp phy patch from there. > -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-09-30 16:25 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-29 17:10 [PATCH] drm/msm/dp: add voltage corners voting support base on dp link rate Kuogee Hsieh 2020-09-29 17:10 ` Kuogee Hsieh 2020-09-30 8:24 ` Stephen Boyd 2020-09-30 8:24 ` Stephen Boyd 2020-09-30 16:24 ` Rajendra Nayak [this message] 2020-09-30 16:24 ` Rajendra Nayak 2020-10-03 18:21 ` khsieh 2020-10-03 18:21 ` khsieh
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=01467014-1190-6e1b-8120-472719861a5e@codeaurora.org \ --to=rnayak@codeaurora.org \ --cc=abhinavk@codeaurora.org \ --cc=airlied@linux.ie \ --cc=aravindh@codeaurora.org \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=khsieh@codeaurora.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=robdclark@gmail.com \ --cc=sean@poorly.run \ --cc=swboyd@chromium.org \ --cc=tanmay@codeaurora.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.