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 <[email protected]>
Best regards,
Krzysztof
