Re: [PATCH v3 5/8] phy: rockchip: samsung-hdptx: Setup TMDS char rate via phy_configure_opts_hdmi

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

On 2/24/25 1:38 AM, Dmitry Baryshkov wrote:
> On Sun, 23 Feb 2025 at 13:02, Cristian Ciocaltea
> <cristian.ciocaltea@collabora.com> wrote:
>>
>> The current workaround to setup the TMDS character rate relies on the
>> unconventional usage of phy_set_bus_width().
>>
>> Make use of the recently introduced HDMI PHY configuration API for this
>> purpose.  The workaround will be dropped as soon as the switch has been
>> completed on both ends.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 37 +++++++++++++++++------
>>  1 file changed, 27 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> index f9b5c96d6c789e435657e224032d35b5a6950945..dd91a7272e246b2133112effdb080a847fd15abe 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> @@ -402,6 +402,9 @@ struct rk_hdptx_phy {
>>         int nr_clks;
>>         struct reset_control_bulk_data rsts[RST_MAX];
>>
>> +       /* PHY config opts */
>> +       unsigned long tmds_char_rate;
> 
> It's easier to embed struct phy_configure_opts_hdmi here, in the end
> you  add bpc here in one of the next patches.

Indeed, handled in [1], part of v4.
 
>> +
>>         /* clk provider */
>>         struct clk_hw hw;
>>         unsigned long rate;

[...]

>> @@ -1830,6 +1837,16 @@ static int rk_hdptx_phy_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>>  {
>>         struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
>>
>> +       /*
>> +        * The TMDS char rate set via phy_configure(), if any, has
>> +        * precedence over the rate provided via clk_set_rate().
> 
> I think this is not so nice. It makes CCF desync from the actual rate
> programmed into the hardware. Maybe you can make the clock r/o?

Oh, you're right.  I should have added just a warning here, though I
haven't really noticed this in practice so far.

I ended up providing a new patch [2] to properly restrict altering 
the TMDS char rate.  As explained in the commit message, making the 
clock read-only is currently not feasible, but I'll revisit this as
soon as the switch to relying exclusively on the HDMI PHY config API 
for the rate setup gets completed on both ends.

Thanks for the review!

Regards,
Cristian
 
>> +        */
>> +       if (hdptx->tmds_char_rate && hdptx->tmds_char_rate != rate) {
>> +               dev_dbg(hdptx->dev, "Replaced clk_set_rate=%lu with tmds_char_rate=%lu\n",
>> +                       rate, hdptx->tmds_char_rate);
>> +               rate = hdptx->tmds_char_rate;
>> +       }
>> +
>>         return rk_hdptx_ropll_tmds_cmn_config(hdptx, rate / 100);
>>  }
>>
>>
>> --
>> 2.48.1

[1] https://lore.kernel.org/lkml/20250304-phy-sam-hdptx-bpc-v4-7-8657847c13f7@collabora.com/
[2] https://lore.kernel.org/lkml/20250304-phy-sam-hdptx-bpc-v4-9-8657847c13f7@collabora.com/