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

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

[Algea Cao]

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>

---

Changes in v2:
- Modify some descriptions in commit message.
---
 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,
-- 
2.43.0

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

[Heiko Stübner]

Am Sonntag, 27. April 2025, 11:51:24 Mitteleuropäische Sommerzeit schrieb Algea Cao:
> 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>

Acked-by: Heiko Stuebner <heiko@sntech.de>

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

[Vinod Koul]

On Sun, 27 Apr 2025 17:51:24 +0800, 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.
> 
> [...]

Applied, thanks!

[1/1] phy: phy-rockchip-samsung-hdptx: Fix PHY PLL output 50.25MHz error
      commit: f9475055b11c0c70979bd1667a76b2ebae638eb7

Best regards,
-- 
~Vinod

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

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 2394 bytes --]

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?

Cheers,
  Diederik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Cristian Ciocaltea]

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

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

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 2889 bytes --]

Hi Cristian, 

On Fri May 16, 2025 at 6:02 PM CEST, Cristian Ciocaltea wrote:
> On 5/16/25 6:36 PM, Diederik de Haas wrote:
>> 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/

Excellent. Thanks for the explanation :-)

Cheers,
  Diederik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]