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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 24/10/2025 16:48, Neil Armstrong wrote:
> 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

Indeed, I see now.

> 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.

OK, I understand now.

> 
> 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 ?

Don't remember anymore.

> 
> The commit message doesn't say anything related to dsi_7nm_phy_enable/disable.
> 
With your patch it works fine and does not produce clock warnings, so
seems fine. If you send proper fix, then:


Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof