Re: [PATCH v4 03/12] phy: rockchip: samsung-hdptx: Fix clock ratio setup

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

On 3/6/25 3:35 PM, Maxime Ripard wrote:
> On Tue, Mar 04, 2025 at 02:04:18PM +0200, Cristian Ciocaltea wrote:
>> Hi Maxime,
>>
>> On 3/4/25 10:13 AM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Tue, Mar 04, 2025 at 03:44:02AM +0200, Cristian Ciocaltea wrote:
>>>> The switch from 1/10 to 1/40 clock ratio must happen when exceeding the
>>>> 340 MHz rate limit of HDMI 1.4, i.e. when entering the HDMI 2.0 domain,
>>>> and not before.
>>>>
>>>> While at it, introduce a define for this rate limit constant.
>>>>
>>>> Fixes: 553be2830c5f ("phy: rockchip: Add Samsung HDMI/eDP Combo PHY driver")
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>>>> index f88369864c50e4563834ccbb26f1f9f440e99271..cf2c3a46604cb9d8c26fe5ec8346904e0b62848f 100644
>>>> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>>>> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>>>> @@ -320,6 +320,7 @@
>>>>  #define LN3_TX_SER_RATE_SEL_HBR2_MASK	BIT(3)
>>>>  #define LN3_TX_SER_RATE_SEL_HBR3_MASK	BIT(2)
>>>>  
>>>> +#define HDMI14_MAX_RATE			340000000
>>>>  #define HDMI20_MAX_RATE			600000000
>>>>  
>>>>  enum dp_link_rate {
>>>> @@ -1072,7 +1073,7 @@ static int rk_hdptx_ropll_tmds_mode_config(struct rk_hdptx_phy *hdptx,
>>>>  
>>>>  	regmap_write(hdptx->regmap, LNTOP_REG(0200), 0x06);
>>>>  
>>>> -	if (rate >= 3400000) {
>>>> +	if (rate > HDMI14_MAX_RATE / 100) {
>>>
>>> The rate seems to come from rk_hdptx_phy_power_on and eventually from
>>> tmds_char_rate in the PHY config options, so its rate is in Hertz.
>>
>> The rate coming from rk_hdptx_phy_power_on() is in hHz, since it passed
>> via dw_hdmi_qp_rockchip_encoder_enable() as
>>
>>   phy_set_bus_width(hdmi->phy, div_u64(rate, 100));
>>
>>> HDMI14_MAX_RATE and HDMI20_MAX_RATE are both defined in Hertz as well.
>>> It seems super odd to mee that you then convert HDMI14_MAX_RATE to hHz?
>>
>> This stems from the ropll_tmds_cfg table containing the configuration
>> params for the supported rates which requires the search keys to be
>> provided in hHz rather than Hz.
>>
>> I agree this is nothing but confusing, that's why I fixed this up in
>> "phy: rockchip: samsung-hdptx: Avoid Hz-hHz unit conversion overhead"
> 
> Yeah, sorry, I noticed it after sending the review. Still, I'd advise to
> put that patch first, it's a bit weird to add more patch we're going to
> rework later on.

No worries and yes, I've already planned to move the patch before all
the others adding more stuff.  I'd still keep this patch as is, to make
it easier for backporting, as well as the next three patches dropping
stuff, before continuing with the unit conversion.