| Message ID | 20250308-phy-sam-hdptx-bpc-v5-11-35087287f9d1@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | phy: rockchip: samsung-hdptx: Support high color depth management | expand |
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 >
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
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 --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 = {
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(-)