Message-ID: <aJ3Y1XhvTPB7J6az@linaro.org>
Date: Thu, 14 Aug 2025 14:38:45 +0200
From: Stephan Gerhold <stephan.gerhold@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>, Stephen Boyd <sboyd@...nel.org>,
	Michael Turquette <mturquette@...libre.com>,
	Dmitry Baryshkov <lumag@...nel.org>,
	Rob Clark <robin.clark@....qualcomm.com>,
	Abhinav Kumar <abhinav.kumar@...ux.dev>,
	Jessica Zhang <jessica.zhang@....qualcomm.com>,
	Sean Paul <sean@...rly.run>,
	Marijn Suijten <marijn.suijten@...ainline.org>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
	Abel Vesa <abel.vesa@...aro.org>, Michael Walle <mwalle@...nel.org>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	Neil Armstrong <neil.armstrong@...aro.org>
Subject: Re: [PATCH 0/2] driver core: platform: / drm/msm: dp: Delay applying
 clock defaults

On Thu, Aug 14, 2025 at 02:55:44PM +0300, Dmitry Baryshkov wrote:
> On Thu, Aug 14, 2025 at 11:18:05AM +0200, Stephan Gerhold wrote:
> > Currently, the platform driver core always calls of_clk_set_defaults()
> > before calling the driver probe() function. This will apply any
> > "assigned-clock-parents" and "assigned-clock-rates" specified in the device
> > tree. However, in some situations, these defaults cannot be safely applied
> > before the driver has performed some early initialization. Otherwise, the
> > clock operations might fail or the device could malfunction.
> > 
> > This is the case for the DP/DSI controller on some Qualcomm platforms. We
> > use assigned-clock-parents there to bind the DP/DSI link clocks to the PHY,
> > but this fails if the PHY is not already powered on. We often bypass this
> > problem because the boot firmware already sets up the correct clock parent,
> > but this is not always the case.
> 
> So, the issue is that our abstraction is loose and we register a clock
> before it becomes usable. Would it be better to delay registering a
> clock until it's actually useable? (and then maybe to unregister on the
> link shutdown)
> 
> > 
> > Michael had a somewhat related problem in the PVR driver recently [1],
> > where of_clk_set_defaults() needs to be called a second time from the PVR
> > driver (after the GPU has been powered on) to make the assigned-clock-rates
> > work correctly.
> > 
> > I propose adding a simple flag to the platform_driver struct that skips the
> > call to of_clk_set_defaults(). The platform driver can then call it later
> > after the necessary initialization was performed (in my case: after the PHY
> > was fully enabled for the first time).
> > 
> > There are also alternative solutions that I considered, but so far
> > I discarded them in favor of this simple one:
> > 
> >  - Avoid use of assigned-clock-parents: We could move the clocks from
> >    "assigned-clock-parents" to "clocks" and call clk_set_parent() manually
> >    from the driver. This is what we did for DSI on SM8750 (see commit
> >    80dd5911cbfd ("drm/msm/dsi: Add support for SM8750")).
> > 
> >    This is the most realistic alternative, but it has a few disadvantages:
> > 
> >     - We need additional boilerplate in the driver to assign all the clock
> >       parents, that would be normally hidden by of_clk_set_defaults().
> > 
> >     - We need to change the existing DT bindings for a number of platforms
> >       just to workaround this limitation in the Linux driver stack. The DT
> >       does not specify when to apply the assigned-clock-parents, so there
> >       is nothing wrong with the current hardware description.
> > 
> >  - Use clock subsystem CLK_OPS_PARENT_ENABLE flag: In theory, this would
> >    enable the new parent before we try to reparent to it. It does not work
> >    in this situation, because the clock subsystem does not have enough
> >    information to power on the PHY. Only the DP/DSI driver has.
> > 
> Another possible option would be to introduce the 'not useable' state /
> flag to the CCF, pointing out that the clock is registered, but should
> not be considered for parenting operations.
> 
> >  - Cache the new parent in the clock driver: We could try to workaround
> >    this problem in the clock driver, by delaying application of the new
> >    clock parent until the parent actually gets enabled. From the
> >    perspective of the clock subsystem, the clock would be already
> >    reparented. This would create an inconsistent state: What if the clock
> >    is already running off some other parent and we get a clk_set_rate()
> >    before the parent clock gets enabled? It would operate on the new
> >    parent, but the actual rate is still being derived from the old parent.
> > 
> 
> But... Generally it feels that we should be able to bring up the clocks
> in some 'safe' configuration, so that the set_parent / set_rate calls
> can succeed. E.g. DISP_CC_MDSS_DPTX0_LINK_CLK_SRC can be clocked from XO
> until we actually need to switch it to a proper rate. I see that
> e.g. dispcc-sm8550.c sets 'CLK_SET_RATE_PARENT' on some of DP clock
> sources for no reason (PHY clock rates can not be set through CCF, they
> are controlled through PHY ops).
> 

I don't think there is any problem with the 'safe' configuration you
mention. I have not tried, but we should be able to use that. However,
my understanding is that reparenting does not fail because the clock
itself is in an "unusable" state, but because the new parent is in an
"unusable" state. We can run the clock from XO, but that wouldn't solve
the problem of reparenting to the PHY (until the PHY is fully
configured).

(It would help a lot if you can find someone from the hardware team at
 Qualcomm to confirm that. Everything I write is just based on
 experiments I have done.)

So, assume that DISP_CC_MDSS_DPTX0_LINK_CLK_SRC is already running from
XO, but the PHY is powered off. Now of_clk_set_defaults() gets called
and we get the call to clk_set_parent() while the PHY is off. How do we
deal with that? Returning 0 without actually changing the parent would
result in inconsistent state, as I described above. clk_get_parent()
would return the new parent, but actually it's still running from XO.

With my changes in this series the clock state is always consistent with
the state returned by the clk APIs. We just delay the call to
clk_set_parent() until we know that it can succeed.

Thanks,
Stephan

Powered by blists - more mailing lists