[PATCH v4 2/2] drm/msm/dp: rewrite dss_module_power to use bulk clock functions
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Thu Feb 17 00:56:41 UTC 2022
On 16/02/2022 05:35, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-01-31 13:05:13)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
>> index 094b39bfed8c..f16072f33cdb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
>> @@ -10,7 +10,6 @@
>> #include <linux/phy/phy.h>
>> #include <linux/phy/phy-dp.h>
>>
>> -#include "dp_clk_util.h"
>> #include "msm_drv.h"
>>
>> #define DP_LABEL "MDSS DP DISPLAY"
>> @@ -106,6 +105,22 @@ struct dp_regulator_cfg {
>> struct dp_reg_entry regs[DP_DEV_REGULATOR_MAX];
>> };
>>
>> +enum dss_clk_type {
>> + DSS_CLK_AHB, /* no set rate. rate controlled through rpm */
>> + DSS_CLK_PCLK,
>> +};
>> +
>> +struct dss_clk {
>> + enum dss_clk_type type;
>> + unsigned long rate;
>> +};
>> +
>> +struct dss_module_power {
>> + unsigned int num_clk;
>> + struct clk_bulk_data *clocks;
>> + struct dss_clk *clk_config;
>> +};
>> +
>> /**
>> * struct dp_parser - DP parser's data exposed to clients
>> *
>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
>> index b48b45e92bfa..318e1f8629cb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>> @@ -105,59 +105,40 @@ static int dp_power_clk_init(struct dp_power_private *power)
>> ctrl = &power->parser->mp[DP_CTRL_PM];
>> stream = &power->parser->mp[DP_STREAM_PM];
>>
>> - rc = msm_dss_get_clk(dev, core->clk_config, core->num_clk);
>> + rc = devm_clk_bulk_get(dev, core->num_clk, core->clocks);
>
> Could we go further and devm_clk_bulk_get_all() and then either have
> some new clk API that goes through the bulk list and finds the one named
> something and hands over a pointer to it, think "clk_get() on top of
> clk_bulk_data", or just get the clk again that you want to set a rate on
> and have two pointers but otherwise it's a don't care. Then we wouldn't
> need to do much at all here to store the rate settable clk and find it
> in a loop.
The clocks are stored in three different structures, because dp_power
enables and disables them separately. See dp_power_clk_enable() and
dp_ctrl.c. This devm_clk_bulk_get_all is out of question.
On the other hand, we can call set_rate on disabled clocks. Let me see
if we can get this into manageble state.
>
>> if (rc) {
>> DRM_ERROR("failed to get %s clk. err=%d\n",
>> dp_parser_pm_name(DP_CORE_PM), rc);
>> return rc;
>> }
>>
>> - rc = msm_dss_get_clk(dev, ctrl->clk_config, ctrl->num_clk);
>> + rc = devm_clk_bulk_get(dev, ctrl->num_clk, ctrl->clocks);
>> if (rc) {
>> DRM_ERROR("failed to get %s clk. err=%d\n",
>> dp_parser_pm_name(DP_CTRL_PM), rc);
>> - msm_dss_put_clk(core->clk_config, core->num_clk);
>> return -ENODEV;
>> }
>>
>> - rc = msm_dss_get_clk(dev, stream->clk_config, stream->num_clk);
>> + rc = devm_clk_bulk_get(dev, stream->num_clk, stream->clocks);
>> if (rc) {
>> DRM_ERROR("failed to get %s clk. err=%d\n",
>> dp_parser_pm_name(DP_CTRL_PM), rc);
>> - msm_dss_put_clk(core->clk_config, core->num_clk);
>> return -ENODEV;
>> }
>>
>> return 0;
>> }
>>
>> -static int dp_power_clk_deinit(struct dp_power_private *power)
>> -{
>> - struct dss_module_power *core, *ctrl, *stream;
>> -
>> - core = &power->parser->mp[DP_CORE_PM];
>> - ctrl = &power->parser->mp[DP_CTRL_PM];
>> - stream = &power->parser->mp[DP_STREAM_PM];
>> -
>> - if (!core || !ctrl || !stream) {
>> - DRM_ERROR("invalid power_data\n");
>> - return -EINVAL;
>> - }
>> -
>> - msm_dss_put_clk(ctrl->clk_config, ctrl->num_clk);
>> - msm_dss_put_clk(core->clk_config, core->num_clk);
>> - msm_dss_put_clk(stream->clk_config, stream->num_clk);
>> - 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)
>> + struct dss_module_power *mp, int enable)
>> {
>> + struct dss_clk *clk_arry = mp->clk_config;
>> + int num_clk = mp->num_clk;
>> u32 rate;
>> int i, rc = 0;
>>
>> for (i = 0; i < num_clk; i++) {
>> - if (clk_arry[i].clk) {
>> + if (mp->clocks[i].clk) {
>> if (clk_arry[i].type == DSS_CLK_PCLK) {
>> if (enable)
>> rate = clk_arry[i].rate;
>> @@ -168,9 +149,49 @@ static int dp_power_clk_set_link_rate(struct dp_power_private *power,
>> if (rc)
>> break;
>> }
>> + } else {
>> + DRM_ERROR("%pS->%s: '%s' is not available\n",
>> + __builtin_return_address(0), __func__,
>> + mp->clocks[i].id);
>> + rc = -EPERM;
>> + break;
>> + }
>> + }
>> + return rc;
>> +}
>> +
>> +static int dp_clk_set_rate(struct dss_module_power *mp)
>> +{
>> + struct dss_clk *clk_arry = mp->clk_config;
>> + int num_clk = mp->num_clk;
>> + int i, rc = 0;
>>
>> + for (i = 0; i < num_clk; i++) {
>> + if (mp->clocks[i].clk) {
>> + if (clk_arry[i].type != DSS_CLK_AHB) {
>
> This loops is gross.
Yep. Tried to keep the origin code here.
>
>> + DRM_DEBUG("%pS->%s: '%s' rate %ld\n",
>> + __builtin_return_address(0), __func__,
>> + mp->clocks[i].id,
>> + clk_arry[i].rate);
>> + rc = clk_set_rate(mp->clocks[i].clk,
>> + clk_arry[i].rate);
>> + if (rc) {
>> + DRM_ERROR("%pS->%s: %s failed. rc=%d\n",
>> + __builtin_return_address(0),
>> + __func__,
>> + mp->clocks[i].id, rc);
>> + break;
>> + }
>> + }
>> + } else {
>> + DRM_ERROR("%pS->%s: '%s' is not available\n",
>> + __builtin_return_address(0), __func__,
>> + mp->clocks[i].id);
>> + rc = -EPERM;
>> + break;
>> }
>> }
>> +
>> return rc;
>> }
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list