[PATCH v3 1/6] phy: rockchip: samsung-hdptx: Fix rate recalculation for high bpc

[Cristian Ciocaltea]

On 6/11/26 5:43 PM, sashiko-bot at kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Potential divide-by-zero when computing SDM fraction if bootloader leaves specific PLL registers uninitialized or misconfigured.
> - [High] FRL_8G4L_RATE (3.2GHz) cascade mode is not handled correctly in clock rate recalculation, causing it to return 0.

It looks like Sashiko is improving, as it didn’t flag these issues during the previous reviews.

I addressed them in v4:

https://lore.kernel.org/all/20260612-hdptx-clk-fixes-v4-0-ce5e1d456cda@collabora.com/

Regards,
Cristian

> --
> 
> commit 7c989d5f5cf68e3aea19b9dd58be7c8b907619aa
> Author: Cristian Ciocaltea <cristian.ciocaltea at collabora.com>
> 
> phy: rockchip: samsung-hdptx: Fix rate recalculation for high bpc
> 
> This patch updates the rate recalculation logic to read the bpc value directly
> from hardware registers instead of relying on the uninitialized HDMI PHY
> configuration. This resolves incorrect rate calculations when bpc is higher
> than 8.
> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> index 2d973bc37f076..710603afff866 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> @@ -2168,7 +2168,7 @@ static u64 rk_hdptx_phy_clk_calc_rate_from_pll_cfg(struct rk_hdptx_phy *hdptx)
>>  	struct lcpll_config lcpll_hw;
>>  	struct ropll_config ropll_hw;
>>  	u64 fout, sdm;
>> -	u32 mode, val;
>> +	u32 mode, bpc, val;
>>  	int ret, i;
>>  
>>  	ret = regmap_read(hdptx->regmap, CMN_REG(0008), &mode);
> 
> [Severity: High]
> This is a pre-existing issue, but does 
> rk_hdptx_phy_clk_calc_rate_from_pll_cfg() fail to handle cascade mode for the
> 3.2GHz FRL_8G4L_RATE?
> 
> When the hardware is configured in cascade mode using both LCPLL and ROPLL,
> CMN_REG(0008) has LCPLL_LCVCO_MODE_EN_MASK set.
> 
> The function sees this bit and assumes the rate must be determined solely
> from the rk_hdptx_frl_lcpll_cfg array:
> 
> drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c:rk_hdptx_phy_clk_calc_rate_from_pll_cfg() {
>     ...
>     if (mode & LCPLL_LCVCO_MODE_EN_MASK) {
>         ...
>         for (i = 0; i < ARRAY_SIZE(rk_hdptx_frl_lcpll_cfg); i++) {
>             ...
>                 return cfg->rate;
>         }
> 
>         dev_dbg(hdptx->dev, "%s no FRL match found\n", __func__);
>         return 0;
>     }
>     ...
> }
> 
> For the 3.2GHz config, it searches the array for an entry with pms_mdiv of
> 107, finds no match, and returns 0 instead of falling through to the ROPLL
> calculation.
> 
> Could this bypass the ROPLL calculation and cause the clock framework to
> register a 0 Hz rate for the 3.2GHz HDMI mode?
> 
> [ ... ]
> 
>> @@ -2266,6 +2266,7 @@ static u64 rk_hdptx_phy_clk_calc_rate_from_pll_cfg(struct rk_hdptx_phy *hdptx)
>>  	if (ret)
>>  		return 0;
>>  	ropll_hw.pms_sdiv = ((val & PLL_PCG_POSTDIV_SEL_MASK) >> 4) + 1;
>> +	bpc = (FIELD_GET(PLL_PCG_CLK_SEL_MASK, val) << 1) + 8;
>>  
>>  	fout = PLL_REF_CLK * ropll_hw.pms_mdiv;
>>  	if (ropll_hw.sdm_en) {
> 
> [Severity: High]
> This is a pre-existing issue, but could this division trigger a
> divide-by-zero panic if the bootloader leaves the PLL registers
> uninitialized or misconfigured?
> 
> The calculation uses div_u64() with a denominator read directly from hardware:
> 
> drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c:rk_hdptx_phy_clk_calc_rate_from_pll_cfg() {
>     ...
>     if (ropll_hw.sdm_en) {
>         sdm = div_u64(PLL_REF_CLK * ropll_hw.sdc_deno *
>                       ropll_hw.pms_mdiv * ropll_hw.sdm_num,
>                       16 * ropll_hw.sdm_deno *
>                       (ropll_hw.sdc_deno * ropll_hw.sdc_n - ropll_hw.sdc_num));
>     ...
> }
> 
> The values ropll_hw.sdm_deno, ropll_hw.sdc_deno, ropll_hw.sdc_n, and
> ropll_hw.sdc_num are populated from CMN_REG(0060), CMN_REG(0070), etc.,
> without any zero-checks.
> 
> If the PLL is enabled but these registers evaluate to 0, it appears this
> would cause a panic during driver probe or when the clock framework queries
> the rate.
> 
> Is it safe to directly trust the hardware register state for this denominator
> without sanitization?
> 
>> @@ -2280,7 +2281,7 @@ static u64 rk_hdptx_phy_clk_calc_rate_from_pll_cfg(struct rk_hdptx_phy *hdptx)
>>  			fout = fout + sdm;
>>  	}
>>  
>> -	return div_u64(fout * 2, ropll_hw.pms_sdiv * 10);
>> +	return DIV_ROUND_CLOSEST_ULL(fout * 2 * 8, ropll_hw.pms_sdiv * 10 * bpc);
>>  }
>