diff mbox series

[v5,11/12] phy: rockchip: samsung-hdptx: Optimize internal rate handling

Message ID 20250308-phy-sam-hdptx-bpc-v5-11-35087287f9d1@collabora.com (mailing list archive)
State New
Headers show
Series phy: rockchip: samsung-hdptx: Support high color depth management | expand

Commit Message

Cristian Ciocaltea March 8, 2025, 12:21 p.m. UTC 
Drop the rate parameter from a bunch of internal helpers and, instead,
make better use of the newly introduced ->hdmi_cfg.tmds_char_rate driver
data.

Additionally, rename the rather ambiguous ->rate member of struct
rk_hdptx_phy to ->hw_rate and ensure rk_hdptx_ropll_tmds_cmn_config()
updates it only after all the other operations have been successful.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 59 ++++++++++++-----------
 1 file changed, 30 insertions(+), 29 deletions(-)

Comments

Dmitry Baryshkov March 9, 2025, 10:17 a.m. UTC | #1 
On Sat, 8 Mar 2025 at 14:21, Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:
>
> Drop the rate parameter from a bunch of internal helpers and, instead,
> make better use of the newly introduced ->hdmi_cfg.tmds_char_rate driver
> data.
>
> Additionally, rename the rather ambiguous ->rate member of struct
> rk_hdptx_phy to ->hw_rate and ensure rk_hdptx_ropll_tmds_cmn_config()

Would it be better to name it tmds_char_rate?

> updates it only after all the other operations have been successful.

Please don't mix refactoring / renames and the actual code changes.

>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 59 ++++++++++++-----------
>  1 file changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> index a62c634f5254e742088515c3b21c05f95acbaded..98f831c3704f5cd65dcf29a4f0a12862a4aa0482 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> @@ -401,7 +401,7 @@ struct rk_hdptx_phy {
>
>         /* clk provider */
>         struct clk_hw hw;
> -       unsigned long rate;
> +       unsigned long hw_rate;
>         bool restrict_rate_change;
>
>         atomic_t usage_count;
> @@ -968,33 +968,34 @@ static bool rk_hdptx_phy_clk_pll_calc(unsigned long rate,
>         return true;
>  }
>
> -static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx,
> -                                         unsigned long rate)
> +static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx)
>  {
>         const struct ropll_config *cfg = NULL;
>         struct ropll_config rc = {0};
> -       int i;
> +       int ret, i;
>
> -       hdptx->rate = rate;
> +       if (!hdptx->hdmi_cfg.tmds_char_rate)
> +               return 0;
>
>         for (i = 0; i < ARRAY_SIZE(ropll_tmds_cfg); i++)
> -               if (rate == ropll_tmds_cfg[i].rate) {
> +               if (hdptx->hdmi_cfg.tmds_char_rate == ropll_tmds_cfg[i].rate) {
>                         cfg = &ropll_tmds_cfg[i];
>                         break;
>                 }
>
>         if (!cfg) {
> -               if (rk_hdptx_phy_clk_pll_calc(rate, &rc)) {
> -                       cfg = &rc;
> -               } else {
> -                       dev_err(hdptx->dev, "%s cannot find pll cfg\n", __func__);
> +               if (!rk_hdptx_phy_clk_pll_calc(hdptx->hdmi_cfg.tmds_char_rate, &rc)) {
> +                       dev_err(hdptx->dev, "%s cannot find pll cfg for rate=%llu\n",
> +                               __func__, hdptx->hdmi_cfg.tmds_char_rate);
>                         return -EINVAL;
>                 }
> +
> +               cfg = &rc;
>         }
>
> -       dev_dbg(hdptx->dev, "%s rate=%lu mdiv=%u sdiv=%u sdm_en=%u k_sign=%u k=%u lc=%u\n",
> -               __func__, rate, cfg->pms_mdiv, cfg->pms_sdiv + 1, cfg->sdm_en,
> -               cfg->sdm_num_sign, cfg->sdm_num, cfg->sdm_deno);
> +       dev_dbg(hdptx->dev, "%s rate=%llu mdiv=%u sdiv=%u sdm_en=%u k_sign=%u k=%u lc=%u\n",
> +               __func__, hdptx->hdmi_cfg.tmds_char_rate, cfg->pms_mdiv, cfg->pms_sdiv + 1,
> +               cfg->sdm_en, cfg->sdm_num_sign, cfg->sdm_num, cfg->sdm_deno);
>
>         rk_hdptx_pre_power_up(hdptx);
>
> @@ -1030,17 +1031,20 @@ static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx,
>         regmap_update_bits(hdptx->regmap, CMN_REG(0086), PLL_PCG_CLK_EN_MASK,
>                            FIELD_PREP(PLL_PCG_CLK_EN_MASK, 0x1));
>
> -       return rk_hdptx_post_enable_pll(hdptx);
> +       ret = rk_hdptx_post_enable_pll(hdptx);
> +       if (!ret)
> +               hdptx->hw_rate = hdptx->hdmi_cfg.tmds_char_rate;
> +
> +       return ret;
>  }
>
> -static int rk_hdptx_ropll_tmds_mode_config(struct rk_hdptx_phy *hdptx,
> -                                          unsigned long rate)
> +static int rk_hdptx_ropll_tmds_mode_config(struct rk_hdptx_phy *hdptx)
>  {
>         rk_hdptx_multi_reg_write(hdptx, rk_hdtpx_common_sb_init_seq);
>
>         regmap_write(hdptx->regmap, LNTOP_REG(0200), 0x06);
>
> -       if (rate > HDMI14_MAX_RATE) {
> +       if (hdptx->hdmi_cfg.tmds_char_rate > HDMI14_MAX_RATE) {
>                 /* For 1/40 bitrate clk */
>                 rk_hdptx_multi_reg_write(hdptx, rk_hdtpx_tmds_lntop_highbr_seq);
>         } else {
> @@ -1092,8 +1096,7 @@ static void rk_hdptx_dp_reset(struct rk_hdptx_phy *hdptx)
>                      HDPTX_I_BGR_EN << 16 | FIELD_PREP(HDPTX_I_BGR_EN, 0x0));
>  }
>
> -static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx,
> -                                    unsigned long rate)
> +static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx)
>  {
>         enum phy_mode mode = phy_get_mode(hdptx->phy);
>         u32 status;
> @@ -1112,11 +1115,9 @@ static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx,
>         if (mode == PHY_MODE_DP) {
>                 rk_hdptx_dp_reset(hdptx);
>         } else {
> -               if (rate) {
> -                       ret = rk_hdptx_ropll_tmds_cmn_config(hdptx, rate);
> -                       if (ret)
> -                               goto dec_usage;
> -               }
> +               ret = rk_hdptx_ropll_tmds_cmn_config(hdptx);
> +               if (ret)
> +                       goto dec_usage;
>         }
>
>         return 0;
> @@ -1429,7 +1430,7 @@ static int rk_hdptx_phy_power_on(struct phy *phy)
>                 dev_dbg(hdptx->dev, "%s rate=%llu\n", __func__, hdptx->hdmi_cfg.tmds_char_rate);
>         }
>
> -       ret = rk_hdptx_phy_consumer_get(hdptx, hdptx->hdmi_cfg.tmds_char_rate);
> +       ret = rk_hdptx_phy_consumer_get(hdptx);
>         if (ret)
>                 return ret;
>
> @@ -1460,7 +1461,7 @@ static int rk_hdptx_phy_power_on(struct phy *phy)
>                 regmap_write(hdptx->grf, GRF_HDPTX_CON0,
>                              HDPTX_MODE_SEL << 16 | FIELD_PREP(HDPTX_MODE_SEL, 0x0));
>
> -               ret = rk_hdptx_ropll_tmds_mode_config(hdptx, hdptx->hdmi_cfg.tmds_char_rate);
> +               ret = rk_hdptx_ropll_tmds_mode_config(hdptx);
>                 if (ret)
>                         rk_hdptx_phy_consumer_put(hdptx, true);
>         }
> @@ -1828,7 +1829,7 @@ static int rk_hdptx_phy_clk_prepare(struct clk_hw *hw)
>  {
>         struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
>
> -       return rk_hdptx_phy_consumer_get(hdptx, hdptx->rate);
> +       return rk_hdptx_phy_consumer_get(hdptx);
>  }
>
>  static void rk_hdptx_phy_clk_unprepare(struct clk_hw *hw)
> @@ -1843,7 +1844,7 @@ static unsigned long rk_hdptx_phy_clk_recalc_rate(struct clk_hw *hw,
>  {
>         struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
>
> -       return hdptx->rate;
> +       return hdptx->hw_rate;
>  }
>
>  static long rk_hdptx_phy_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> @@ -1895,7 +1896,7 @@ static int rk_hdptx_phy_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>          * while the latter being executed only once, i.e. when clock remains
>          * in the prepared state during rate changes.
>          */
> -       return rk_hdptx_ropll_tmds_cmn_config(hdptx, rate);
> +       return rk_hdptx_ropll_tmds_cmn_config(hdptx);
>  }
>
>  static const struct clk_ops hdptx_phy_clk_ops = {
>
> --
> 2.48.1
>
Cristian Ciocaltea March 9, 2025, 10:32 a.m. UTC | #2 
On 3/9/25 12:17 PM, Dmitry Baryshkov wrote:
> On Sat, 8 Mar 2025 at 14:21, Cristian Ciocaltea
> <cristian.ciocaltea@collabora.com> wrote:
>>
>> Drop the rate parameter from a bunch of internal helpers and, instead,
>> make better use of the newly introduced ->hdmi_cfg.tmds_char_rate driver
>> data.
>>
>> Additionally, rename the rather ambiguous ->rate member of struct
>> rk_hdptx_phy to ->hw_rate and ensure rk_hdptx_ropll_tmds_cmn_config()
> 
> Would it be better to name it tmds_char_rate?

This is only used in rk_hdptx_phy_clk_recalc_rate() to provide the
actual rate programmed in hardware, hence the "hw_rate" naming.
Renaming it to tmds_char_rate might add some confusion with the similar
one in ->hdmi_cfg, but I don't really have any strong preference here.

>> updates it only after all the other operations have been successful.
> 
> Please don't mix refactoring / renames and the actual code changes.

Right, I should have moved that to a dedicated patch.  Will handle in
the next revision.

Thanks,
Cristian
Dmitry Baryshkov March 9, 2025, 10:33 a.m. UTC | #3 
On Sun, 9 Mar 2025 at 12:32, Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:
>
> On 3/9/25 12:17 PM, Dmitry Baryshkov wrote:
> > On Sat, 8 Mar 2025 at 14:21, Cristian Ciocaltea
> > <cristian.ciocaltea@collabora.com> wrote:
> >>
> >> Drop the rate parameter from a bunch of internal helpers and, instead,
> >> make better use of the newly introduced ->hdmi_cfg.tmds_char_rate driver
> >> data.
> >>
> >> Additionally, rename the rather ambiguous ->rate member of struct
> >> rk_hdptx_phy to ->hw_rate and ensure rk_hdptx_ropll_tmds_cmn_config()
> >
> > Would it be better to name it tmds_char_rate?
>
> This is only used in rk_hdptx_phy_clk_recalc_rate() to provide the
> actual rate programmed in hardware, hence the "hw_rate" naming.

Ack

> Renaming it to tmds_char_rate might add some confusion with the similar
> one in ->hdmi_cfg, but I don't really have any strong preference here.
>
> >> updates it only after all the other operations have been successful.
> >
> > Please don't mix refactoring / renames and the actual code changes.
>
> Right, I should have moved that to a dedicated patch.  Will handle in
> the next revision.
>
> Thanks,
> Cristian
diff mbox series

Patch

diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
index a62c634f5254e742088515c3b21c05f95acbaded..98f831c3704f5cd65dcf29a4f0a12862a4aa0482 100644
--- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
+++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
@@ -401,7 +401,7 @@  struct rk_hdptx_phy {
 
 	/* clk provider */
 	struct clk_hw hw;
-	unsigned long rate;
+	unsigned long hw_rate;
 	bool restrict_rate_change;
 
 	atomic_t usage_count;
@@ -968,33 +968,34 @@  static bool rk_hdptx_phy_clk_pll_calc(unsigned long rate,
 	return true;
 }
 
-static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx,
-					  unsigned long rate)
+static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx)
 {
 	const struct ropll_config *cfg = NULL;
 	struct ropll_config rc = {0};
-	int i;
+	int ret, i;
 
-	hdptx->rate = rate;
+	if (!hdptx->hdmi_cfg.tmds_char_rate)
+		return 0;
 
 	for (i = 0; i < ARRAY_SIZE(ropll_tmds_cfg); i++)
-		if (rate == ropll_tmds_cfg[i].rate) {
+		if (hdptx->hdmi_cfg.tmds_char_rate == ropll_tmds_cfg[i].rate) {
 			cfg = &ropll_tmds_cfg[i];
 			break;
 		}
 
 	if (!cfg) {
-		if (rk_hdptx_phy_clk_pll_calc(rate, &rc)) {
-			cfg = &rc;
-		} else {
-			dev_err(hdptx->dev, "%s cannot find pll cfg\n", __func__);
+		if (!rk_hdptx_phy_clk_pll_calc(hdptx->hdmi_cfg.tmds_char_rate, &rc)) {
+			dev_err(hdptx->dev, "%s cannot find pll cfg for rate=%llu\n",
+				__func__, hdptx->hdmi_cfg.tmds_char_rate);
 			return -EINVAL;
 		}
+
+		cfg = &rc;
 	}
 
-	dev_dbg(hdptx->dev, "%s rate=%lu mdiv=%u sdiv=%u sdm_en=%u k_sign=%u k=%u lc=%u\n",
-		__func__, rate, cfg->pms_mdiv, cfg->pms_sdiv + 1, cfg->sdm_en,
-		cfg->sdm_num_sign, cfg->sdm_num, cfg->sdm_deno);
+	dev_dbg(hdptx->dev, "%s rate=%llu mdiv=%u sdiv=%u sdm_en=%u k_sign=%u k=%u lc=%u\n",
+		__func__, hdptx->hdmi_cfg.tmds_char_rate, cfg->pms_mdiv, cfg->pms_sdiv + 1,
+		cfg->sdm_en, cfg->sdm_num_sign, cfg->sdm_num, cfg->sdm_deno);
 
 	rk_hdptx_pre_power_up(hdptx);
 
@@ -1030,17 +1031,20 @@  static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx,
 	regmap_update_bits(hdptx->regmap, CMN_REG(0086), PLL_PCG_CLK_EN_MASK,
 			   FIELD_PREP(PLL_PCG_CLK_EN_MASK, 0x1));
 
-	return rk_hdptx_post_enable_pll(hdptx);
+	ret = rk_hdptx_post_enable_pll(hdptx);
+	if (!ret)
+		hdptx->hw_rate = hdptx->hdmi_cfg.tmds_char_rate;
+
+	return ret;
 }
 
-static int rk_hdptx_ropll_tmds_mode_config(struct rk_hdptx_phy *hdptx,
-					   unsigned long rate)
+static int rk_hdptx_ropll_tmds_mode_config(struct rk_hdptx_phy *hdptx)
 {
 	rk_hdptx_multi_reg_write(hdptx, rk_hdtpx_common_sb_init_seq);
 
 	regmap_write(hdptx->regmap, LNTOP_REG(0200), 0x06);
 
-	if (rate > HDMI14_MAX_RATE) {
+	if (hdptx->hdmi_cfg.tmds_char_rate > HDMI14_MAX_RATE) {
 		/* For 1/40 bitrate clk */
 		rk_hdptx_multi_reg_write(hdptx, rk_hdtpx_tmds_lntop_highbr_seq);
 	} else {
@@ -1092,8 +1096,7 @@  static void rk_hdptx_dp_reset(struct rk_hdptx_phy *hdptx)
 		     HDPTX_I_BGR_EN << 16 | FIELD_PREP(HDPTX_I_BGR_EN, 0x0));
 }
 
-static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx,
-				     unsigned long rate)
+static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx)
 {
 	enum phy_mode mode = phy_get_mode(hdptx->phy);
 	u32 status;
@@ -1112,11 +1115,9 @@  static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx,
 	if (mode == PHY_MODE_DP) {
 		rk_hdptx_dp_reset(hdptx);
 	} else {
-		if (rate) {
-			ret = rk_hdptx_ropll_tmds_cmn_config(hdptx, rate);
-			if (ret)
-				goto dec_usage;
-		}
+		ret = rk_hdptx_ropll_tmds_cmn_config(hdptx);
+		if (ret)
+			goto dec_usage;
 	}
 
 	return 0;
@@ -1429,7 +1430,7 @@  static int rk_hdptx_phy_power_on(struct phy *phy)
 		dev_dbg(hdptx->dev, "%s rate=%llu\n", __func__, hdptx->hdmi_cfg.tmds_char_rate);
 	}
 
-	ret = rk_hdptx_phy_consumer_get(hdptx, hdptx->hdmi_cfg.tmds_char_rate);
+	ret = rk_hdptx_phy_consumer_get(hdptx);
 	if (ret)
 		return ret;
 
@@ -1460,7 +1461,7 @@  static int rk_hdptx_phy_power_on(struct phy *phy)
 		regmap_write(hdptx->grf, GRF_HDPTX_CON0,
 			     HDPTX_MODE_SEL << 16 | FIELD_PREP(HDPTX_MODE_SEL, 0x0));
 
-		ret = rk_hdptx_ropll_tmds_mode_config(hdptx, hdptx->hdmi_cfg.tmds_char_rate);
+		ret = rk_hdptx_ropll_tmds_mode_config(hdptx);
 		if (ret)
 			rk_hdptx_phy_consumer_put(hdptx, true);
 	}
@@ -1828,7 +1829,7 @@  static int rk_hdptx_phy_clk_prepare(struct clk_hw *hw)
 {
 	struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
 
-	return rk_hdptx_phy_consumer_get(hdptx, hdptx->rate);
+	return rk_hdptx_phy_consumer_get(hdptx);
 }
 
 static void rk_hdptx_phy_clk_unprepare(struct clk_hw *hw)
@@ -1843,7 +1844,7 @@  static unsigned long rk_hdptx_phy_clk_recalc_rate(struct clk_hw *hw,
 {
 	struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
 
-	return hdptx->rate;
+	return hdptx->hw_rate;
 }
 
 static long rk_hdptx_phy_clk_round_rate(struct clk_hw *hw, unsigned long rate,
@@ -1895,7 +1896,7 @@  static int rk_hdptx_phy_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 	 * while the latter being executed only once, i.e. when clock remains
 	 * in the prepared state during rate changes.
 	 */
-	return rk_hdptx_ropll_tmds_cmn_config(hdptx, rate);
+	return rk_hdptx_ropll_tmds_cmn_config(hdptx);
 }
 
 static const struct clk_ops hdptx_phy_clk_ops = {