[PATCH v3 2/6] phy: rockchip: samsung-hdptx: Handle uncommitted PHY config changes

[Cristian Ciocaltea]

Hi,

TL;DR: This is similar to the review we got on v2, i.e. no real concerns, the
series can be safely applied.

Regards,
Cristian

On 6/11/26 3:52 PM, sashiko-bot at kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider: - [High] Ignoring PLL configuration errors in
> `rk_hdptx_phy_consumer_get()` leads to silent hardware failure and violates
> the CCF API contract. --
> 
> commit 6b14268a38889c73eedd6c085d780316eb0e7c3c Author: Cristian Ciocaltea
> <cristian.ciocaltea at collabora.com>
> 
> phy: rockchip: samsung-hdptx: Handle uncommitted PHY config changes
> 
> This commit introduces a pll_config_dirty flag to keep track of uncommitted
> PHY configuration changes. It ensures that changes to the PHY link rate or
> color depth are delayed and properly programmed into the hardware when the PHY
> usage count is incremented.
> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c index
>> 710603afff866..5295d5f6f287a 100644 ---
>> a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c +++
>> b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> [ ... ]
>> @@ -1347,25 +1354,22 @@ static int rk_hdptx_phy_consumer_get(struct
>> rk_hdptx_phy *hdptx) return 0;
>>  
>>  	ret = regmap_read(hdptx->grf, GRF_HDPTX_STATUS, &status); -	if (ret)
>>  	-		goto dec_usage; - -	if (status &
>>  	HDPTX_O_PLL_LOCK_DONE) -		dev_warn(hdptx->dev, "PLL locked
>>  	by unknown consumer!\n"); +	if (ret) { +
>>  	atomic_dec(&hdptx->usage_count); +		return ret; +	}
>>  
>>  	if (mode == PHY_MODE_DP) { rk_hdptx_dp_reset(hdptx); } else { -
>>  	ret = rk_hdptx_pll_cmn_config(hdptx); -		if (ret) -
>>  	goto dec_usage; +		/* +		 * Ignore PLL config
>>  	errors at this point as pll_config_dirty +		 * was not reset
>>  	and, therefore, operation will be retried. +		 */ +
>>  	rk_hdptx_pll_cmn_config(hdptx); }
>>  
>>  	return 0;
> 
> [Severity: High] Is it safe to ignore the return value of
> rk_hdptx_pll_cmn_config() here?

Yes, this is done on purpose since the error is not critical at this point and
we don't want to bail out too early. In practice, this shouldn't happen, unless
the hardware is faulty, in which case the kernel is likely to crash before
reaching this.

> 
> If rk_hdptx_pll_cmn_config() fails, we return 0 to the caller. This causes
> clk_prepare() to return success to the Common Clock Framework even though
> the hardware failed to initialize. Does this violate the API contract?
> 
> Also, hdptx->usage_count is left incremented despite the hardware failure.

That's not a valid usecase, the hardware will be eventually programmed by
rk_hdptx_phy_clk_set_rate() or rk_hdptx_phy_power_on(), whichever comes first.
The clock provider is dependent on the PHY configuration, which is expected to
follow clk_prepare_enable().

> 
> [ ... ]
>> @@ -1700,13 +1704,18 @@ static int rk_hdptx_phy_power_on(struct phy *phy)
>>  		if (ret)
>>  			rk_hdptx_phy_consumer_put(hdptx, true);
>>  	} else {
>> -		regmap_write(hdptx->grf, GRF_HDPTX_CON0,
>> -			     HDPTX_MODE_SEL << 16 | FIELD_PREP(HDPTX_MODE_SEL, 0x0));
>> +		if (hdptx->pll_config_dirty)
>> +			ret = rk_hdptx_pll_cmn_config(hdptx);
> 
> [Severity: High]
> Will this retry block actually execute if pll_config_dirty was already false?
> 
> If the initial rk_hdptx_pll_cmn_config() failure happened in 
> rk_hdptx_phy_consumer_get() when pll_config_dirty was false, it seems
> pll_config_dirty remains false. When rk_hdptx_phy_power_on() is later called,
> it checks if pll_config_dirty is true and might skip the retry entirely,
> leaving the hardware in a partially initialized state with an unlocked PLL.

The hardware is not really useful without rk_hdptx_phy_configure() being called,
which sets pll_config_dirty to true.

> 
>> +		if (!ret) {
>> +			regmap_write(hdptx->grf, GRF_HDPTX_CON0,
>> +				     HDPTX_MODE_SEL << 16 | FIELD_PREP(HDPTX_MODE_SEL, 0x0));
>> +
>> +			if (hdptx->hdmi_cfg.mode == PHY_HDMI_MODE_FRL)
>> +				ret = rk_hdptx_frl_lcpll_mode_config(hdptx);
>> +			else
>> +				ret = rk_hdptx_tmds_ropll_mode_config(hdptx);
>> +		}
>>  
>>  		if (ret)
>>  			rk_hdptx_phy_consumer_put(hdptx, true);
>