Re: [PATCH] Revert "clk: qcom: dispcc-sm8250: Enable parents for pixel clocks"

On Wed, Jul 22, 2026 at 12:28:18PM +0300, Vladimir Zapolskiy wrote:
> On 7/22/26 02:30, Dmitry Baryshkov wrote:
> > On Tue, Jul 21, 2026 at 01:39:23AM +0300, Vladimir Zapolskiy wrote:
> > > On 7/20/26 23:54, Dmitry Baryshkov wrote:
> > > > On Mon, Jul 20, 2026 at 09:21:03PM +0300, Vladimir Zapolskiy wrote:
> > > > > On 7/20/26 18:00, Dmitry Baryshkov wrote:
> > > > > > On Wed, Jul 01, 2026 at 03:11:47AM +0300, Vladimir Zapolskiy wrote:
> > > > > > > This reverts commit acf7a91d0b0e9e3ef374944021de62062125b7e4.
> > > > > > > 
> > > > > > > The commit causes quite severe problems in runtime due to the introduced
> > > > > > > cyclic clock dependency:
> > > > > > > 
> > > > > > > * dsi@ae9400 platform device and its probe depends on dispcc clocks,
> > > > > > > * the reverted commit links dispcc clocks enablement to mdss_dsiX_phy clocks,
> > > > > > > * at this stage DSI PHY PLL cannot be locked to complete driver initialization,
> > > > > > >      the parent clocks for DISPCC provided mdss_pclkX clocks can not be enabled.
> > > > > > > 
> > > > > > > On RB5 board with no connected display panel on boot I get this error report:
> > > > > > > 
> > > > > > >      DSI PLL(0) lock failed, status=0x00000000
> > > > > > >      PLL(0) lock failed
> > > > > > 
> > > > > > Why can't it be locked at this stage? Is it underpowered? Or is the code
> > > > > > trying to lock it on some random, unsupported value?
> > > > > 
> > > > > Since there is no display panel attached, I believe DSI PLL clocks should
> > > > > not be enabled at all, however the reverted commit requires it to happen.
> > > > 
> > > > If there is no display panel, you can keep DSI hosts and DSI PHYs
> > > > disabled. If they are enabled, they can be configured to some
> > > > intermediate values (even if for the sake of turning off the GDSC).
> > > > 
> > > > > 
> > > > > I don't have a panel for RB5 to test, if the PLL clocks can be locked or
> > > > > not, when there is an attached panel.
> > > > > 
> > > > > In general I believe the problem is plainly related to the established
> > > > > cyclic dependency as it's stated above, and in any case the reverted
> > > > > commit is a clear regression for RB5 board with no panel.
> > > > 
> > > > RB5 ships with the lt9611uxc bridge.
> > > > 
> > > > There is no cyclic dependency, AFAIR. It's DSI PHY PLL <- dispcc <- MDSS
> > > > pclk clocks.
> > > 
> > > What does '<-' above mean?
> > > 
> > >  From sm8250.dtsi (using labels only):
> > > 
> > > * dispcc is supplied by mdss_dsi[01]_phy,
> > > * mdss_dsi[01]_phy are supplied by dispcc,
> > > * mdss_dsi[01]_phy are children of mdss,
> > > * mdss is supplied by dispcc.
> > > 
> > > After the applied commit, which I advocate to revert for now, the ball
> > > of clocks is no longer left untangled and linear.
> > 
> > The devices have always had a circular dependency. The commit in
> > question adds nothing to that.
> 
> Unfortunately the devices were in the circular dependency,
> the clock controller device driver/registration provided by
> the dispcc and mdss_dsiX_phy devices were luckily not cycled.
> 
> The reverted commit requests (unprovided) "dsiX_phy_pll_out_dsiclk"
> clock to be enabled whilst setting "disp_cc_mdss_pclk0" clock parent
> from "bi_tcxo". That's where it breaks, I suppose.

If it is unprovided, why do we see PLL lock attempts? After a quick
check at this point we are programming the PLL registers, but they are
not reaching the hardware (and read back as 0 at this point) or the
hardware is powered down.

I've performed several attempts to understand, what is missing here, but
I didn't get any positive results. Neither of the following works:
- Adding cpu-cfg interconnect
- changing DSI PHY code to power on regulators at runtime pm resume
- Adding refgen supply to DSI PHY
- Adding MMCX domain vote to DSI PHY
- several other small hacks


I will try inquiring internally.

One thing I'm pretty sure at this point. The original patch was correct.
We should be turning on parents, otherwise the RCGs can lock up on
reparent.

> 
> > > 
> > > If there is a clear regression caused by some commit, it would make
> > > sense to revert it first, then reapply it after an alternative fix
> > > is ready.
> > 
> > Unless the commit is correct and just uncovers an issue elsewhere.
> 
> So, what is it be to fixed here technically? An advice is welcome.
> 
> OTOH If the issue was there before, likely it hasn't been ever
> reported before, and now its rate of reproducibility jumped to 100%,
> this is still a regression.

-- 
With best wishes
Dmitry