Re: [PATCH v2] phy: phy-rockchip-samsung-hdptx: Fix PHY PLL output 50.25MHz error

From: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
To: Diederik de Haas <didi.debian@cknow.org>,
	Algea Cao <algea.cao@rock-chips.com>,
	vkoul@kernel.org, kishon@kernel.org, heiko@sntech.de,
	andy.yan@rock-chips.com
Cc: linux-phy@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] phy: phy-rockchip-samsung-hdptx: Fix PHY PLL output 50.25MHz error
Date: Fri, 16 May 2025 19:02:51 +0300	[thread overview]
Message-ID: <9e29ef0e-c373-4f48-8f37-7111987d2dd0@collabora.com> (raw)
In-Reply-To: <D9XP2BH2CU02.1R9E1GSXQD9JB@cknow.org>

Hi Diederik,

On 5/16/25 6:36 PM, Diederik de Haas wrote:
> Hi,
> 
> On Sun Apr 27, 2025 at 11:51 AM CEST, Algea Cao wrote:
>> When using HDMI PLL frequency division coefficient at 50.25MHz
>> that is calculated by rk_hdptx_phy_clk_pll_calc(), it fails to
>> get PHY LANE lock. Although the calculated values are within the
>> allowable range of PHY PLL configuration.
>>
>> In order to fix the PHY LANE lock error and provide the expected
>> 50.25MHz output, manually compute the required PHY PLL frequency
>> division coefficient and add it to ropll_tmds_cfg configuration
>> table.
>>
>> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
>> Reviewed-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> index fe7c05748356..77236f012a1f 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> @@ -476,6 +476,8 @@ static const struct ropll_config ropll_tmds_cfg[] = {
>>  	  1, 1, 0, 0x20, 0x0c, 1, 0x0e, 0, 0, },
>>  	{ 650000, 162, 162, 1, 1, 11, 1, 1, 1, 1, 1, 1, 1, 54, 0, 16, 4, 1,
>>  	  1, 1, 0, 0x20, 0x0c, 1, 0x0e, 0, 0, },
>> +	{ 502500, 84, 84, 1, 1, 7, 1, 1, 1, 1, 1, 1, 1, 11, 1, 4, 5,
>> +	  4, 11, 1, 0, 0x20, 0x0c, 1, 0x0e, 0, 0, },
>>  	{ 337500, 0x70, 0x70, 1, 1, 0xf, 1, 1, 1, 1, 1, 1, 1, 0x2, 0, 0x01, 5,
>>  	  1, 1, 1, 0, 0x20, 0x0c, 1, 0x0e, 0, 0, },
>>  	{ 400000, 100, 100, 1, 1, 11, 1, 1, 0, 1, 0, 1, 1, 0x9, 0, 0x05, 0,
> 
> I found this patch in the 'fixes' branch in linux-phy:
> https://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git/commit/?h=fixes&id=f9475055b11c0c70979bd1667a76b2ebae638eb7
> 
> In the 'next' branch in linux-phy, I found this commit:
> 0edf9d2bb9b4 ("phy: rockchip: samsung-hdptx: Avoid Hz<->hHz unit conversion overhead")
> https://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git/commit/?h=next&id=0edf9d2bb9b4ba7566dfdc7605883e04575129d9
> 
> Where the values in ropll_tmds_cfg are converted from hHz to Hz and the
> data type changes from u32 to unsigned long long.
> But AFAICT it does NOT convert this '502500' value, which IIUC means
> most values are in Hz, while this one is in hHz.
> 
> Am I missing something or should this new value also be converted?

Yeah, the conversion is necessary. FWIW, this has been already fixed by
Stephen Rothwell in linux-next, while Vinod will handle it before
sending the PR:

https://lore.kernel.org/all/aCXEOGUDcnaoGKWW@vaman/

Regards,
Cristian

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip