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?
Best regards,
Krzysztof
