Re: [PATCH v4 4/8] phy: rockchip: samsung-hdptx: Handle uncommitted PHY config changes

[Cristian Ciocaltea]

On 7/20/26 5:17 PM, Dmitry Baryshkov wrote:
> On Fri, Jun 12, 2026 at 02:45:23AM +0300, Cristian Ciocaltea wrote:
>> Any changes to the PHY link rate and/or color depth done via the HDMI
>> PHY configuration API are not immediately programmed into the hardware,
>> but are delayed until the PHY usage count gets incremented from 0 to 1,
>> that is when it is powered on or when the PLL clock exposed through
>> the CCF API is prepared, whichever comes first.
> 
> Why do you need either-of here? Can it be just one path?

The thing is that PLL clock usage is dynamic, depending on the rate, but it can
also be disabled permanently. When it's in use, it triggers before power on,
hence the hardware must be programmed at that time.

>>
>> Since the clock might remain in prepared state after subsequent PHY
>> config changes, the programming can also be triggered via
>> clk_ops.set_rate().  However, from the clock consumer perspective (i.e.
>> VOP2 display controller), the (pixel) clock rate doesn't vary with bpc,
>> as that is handled internally by the PHY and reflected in the TDMS
>> character rate only.
>>
>> As a consequence, changing the bpc while preserving the modeline may
>> lead to out-of-sync issues between CCF and HDMI PHY config state,
>> because the .set_rate() callback is not invoked when clock rate remains
>> constant.  This may also happen when the PHY PLL has been pre-programmed
>> by an external entity, e.g. the bootloader, which is actually a
>> regression introduced by the recent FRL patches.
>>
>> Introduce a pll_config_dirty flag to keep track of uncommitted PHY
>> config changes and use it in clk_ops.determine_rate() to invalidate the
>> current clock rate (as known by CCF) and, consequently, ensure those
>> changes are programmed into hardware via clk_ops.set_rate().
>>
>> Moreover, proceed with a similar fix in phy_ops.power_on() callback, to
>> handle the scenario where the CCF API is not used due to operating in
>> FRL mode, while the clock is still in a prepared state and thus
>> preventing rk_hdptx_phy_consumer_get() to apply the updated PHY
>> configuration.
>>
>> Fixes: de5dba833118 ("phy: rockchip: samsung-hdptx: Add HDMI 2.1 FRL support")
>> Fixes: 9d0ec51d7c22 ("phy: rockchip: samsung-hdptx: Add high color depth management")
>> Tested-by: Thomas Niederprüm <dubito@xxxxxxxxx>
>> Tested-by: Simon Wright <simon@xxxxxxxxx>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxxxxxx>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 84 +++++++++++++----------
>>  1 file changed, 48 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> index b210c1a88b25..25bd821cd039 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> @@ -413,6 +413,7 @@ struct rk_hdptx_phy {
>>  
>>  	/* clk provider */
>>  	struct clk_hw hw;
>> +	bool pll_config_dirty;
> 
> Which lock protects it?

This is not protected by a driver-local lock, as the impacted calls are
orchestrated by the DRM core, which should guaranty the expected sequence of
operations:

1. encoder-check -> phy_configure -> no HW programming, pll_config_dirty=true
2. VOP2 crtc-enable -> set_rate -> HW commit, pll_config_dirty=false
3. bridge-enable -> phy_power_on, no HW programming (pll_config_dirty already
   false)

> 
>>  	bool restrict_rate_change;
>>  
>>  	atomic_t usage_count;
>