Re: [PATCH v7] drm/msm/dsi/phy: Fix reading zero as PLL rates when unprepared

[Neil Armstrong <neil.armstrong@linaro.org>]

On 10/24/25 16:34, Krzysztof Kozlowski wrote:
> On 24/10/2025 14:43, Neil Armstrong wrote:
>> Hi,
>>
>> On 9/8/25 11:49, Krzysztof Kozlowski wrote:
>>> Hardware Programming Guide for DSI PHY says that PLL_SHUTDOWNB and
>>> DIGTOP_PWRDN_B have to be asserted for any PLL register access.
>>> Whenever dsi_pll_7nm_vco_recalc_rate() or dsi_pll_7nm_vco_set_rate()
>>> were called on unprepared PLL, driver read values of zero leading to all
>>> sort of further troubles, like failing to set pixel and byte clock
>>> rates.
>>>
>>> Asserting the PLL shutdown bit is done by dsi_pll_enable_pll_bias() (and
>>> corresponding dsi_pll_disable_pll_bias()) which are called through the
>>> code, including from PLL .prepare() and .unprepare() callbacks.
>>>
>>> The .set_rate() and .recalc_rate() can be called almost anytime from
>>> external users including times when PLL is or is not prepared, thus
>>> driver should not interfere with the prepare status.
>>>
>>> Implement simple reference counting for the PLL bias, so
>>> set_rate/recalc_rate will not change the status of prepared PLL.
>>>
>>> Issue of reading 0 in .recalc_rate() did not show up on existing
>>> devices, but only after re-ordering the code for SM8750.
>>
>> It happens this breaks the bonded DSI use-case, mainly because both PHYs
>> uses the same PLL, and trying to enable the DSI0 PHY PLL from the DSI1
>> PHY fails because the DSI0 PHY enable_count == 0.
> 
> 
> If it is ==0, the check you removed would not be hit and enable would
> work. I don't quite get the analysis.
> 
>>
>> Reverting part the the patch makes the bonded work again:
>> ===================><===============================
>> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
>> index 32f06edd21a9..24811c52d34c 100644
>> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
>> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
>> @@ -426,11 +426,8 @@ static void dsi_pll_enable_pll_bias(struct dsi_pll_7nm *pll)
>>    	u32 data;
>>
>>    	spin_lock_irqsave(&pll->pll_enable_lock, flags);
>> -	if (pll->pll_enable_cnt++) {
>> -		spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
>> -		WARN_ON(pll->pll_enable_cnt == INT_MAX);
>> -		return;
>> -	}
>> +	pll->pll_enable_cnt++;
>> +	WARN_ON(pll->pll_enable_cnt == INT_MAX);
>>
>>    	data = readl(pll->phy->base + REG_DSI_7nm_PHY_CMN_CTRL_0);
>>    	data |= DSI_7nm_PHY_CMN_CTRL_0_PLL_SHUTDOWNB;
>> @@ -965,10 +962,8 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
>>    	u32 const delay_us = 5;
>>    	u32 const timeout_us = 1000;
>>    	struct msm_dsi_dphy_timing *timing = &phy->timing;
>> -	struct dsi_pll_7nm *pll = phy->pll_data;
>>    	void __iomem *base = phy->base;
>>    	bool less_than_1500_mhz;
>> -	unsigned long flags;
>>    	u32 vreg_ctrl_0, vreg_ctrl_1, lane_ctrl0;
>>    	u32 glbl_pemph_ctrl_0;
>>    	u32 glbl_str_swi_cal_sel_ctrl, glbl_hstx_str_ctrl_0;
>> @@ -1090,13 +1085,10 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
>>    		glbl_rescode_bot_ctrl = 0x3c;
>>    	}
>>
>> -	spin_lock_irqsave(&pll->pll_enable_lock, flags);
> 
> This should not be removed.
> 
>> -	pll->pll_enable_cnt = 1;
> 
> So you basically remoevd pll_enable_cnt everywhere and reverted entirely
> my commit. How is this patch different than revert?

No I did not, I kept the dsi_pll_disable_pll_bias() refcounting and call from
all the clock ops, which is basically needed here to never access PLL without
PLL_SHUTDOWNB and DIGTOP_PWRDN_B being asserted.

I only removed the pll_enable_cnt from dsi_7nm_phy_enable/disable because the
PHY code is designed to allow setting the PLL rate while the PHY is disabled.
And the bonded DSI hits this use case by setting the DSI0 PHY PLL rate while
configuring the PLL1 PHY.

So I wonder why it was added in the beginning because since you call dsi_pll_disable_pll_bias()
in each clk op, the Hardware Programming Guide for DSI PHY is satisfied ?

The commit message doesn't say anything related to dsi_7nm_phy_enable/disable.

Neil

> 
> Best regards,
> Krzysztof