Message-ID: <aKRUvCVpz8y47TPs@linaro.org>
Date: Tue, 19 Aug 2025 12:41:00 +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 Tue, Aug 19, 2025 at 04:19:26AM +0300, Dmitry Baryshkov wrote:
> On Mon, Aug 18, 2025 at 11:41:16AM +0200, Stephan Gerhold wrote:
> > On Sat, Aug 16, 2025 at 04:55:00PM +0300, Dmitry Baryshkov wrote:
> > > On Thu, Aug 14, 2025 at 02:38:45PM +0200, Stephan Gerhold wrote:
> > > > 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).
> > > 
> > > 
> > > How would the CCF react if we return -ENA from the enable() method of
> > > the PHY clock if it's not available yet?
> > > 
> > 
> > With the current setup it wouldn't change anything, because the failing
> > operation is just the clk_set_parent() that happens from the driver core
> > before the clock will be enabled. It wouldn't reach the enable() method.
> > 
> > With CLK_OPS_PARENT_ENABLE, I would expect clk_set_parent() to fail,
> > which also doesn't get us any further. :-)
> 
> Ack
> 
> > 
> > > > 
> > > > (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.
> > > 
> > > For RCG2 we already have a lot of tricks like that.
> > > 
> > 
> > That is true, although e.g. the clk_rcg2_shared_ops apply the tricks
> > (the caching of clock ops) only while the clock is off. When the clock
> > is off, it doesn't matter what we return about the freq/parents from the
> > clk ops. The problematic case I mentioned above would occur if the clock
> > is (for whatever reason) already running sourced from XO during boot.
> > 
> > In other words, I could imagine that implementing something like the
> > clk_rcg2_shared_ops for the DP clocks could fix the error I'm trying to
> > solve in this patch series. However, it would only work if the clock is
> > really off during boot and not already running sourced from XO.
> 
> link_clk_src clocks are clk_byte2_ops, so they don't have separate
> enable/disable ops. You might implement something close to
> clk_regmap_phy_mux_ops: turn XO parent into "disabled" state.
> 

Thanks for the suggestion, I'll keep that in mind.

> > 
> > > > 
> > > > 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.
> > > 
> > > I know. But what happens when we power down the PHY? The clock is
> > > assumed to have the PHY clock as a parent, but it's supposedly not
> > > clocking.
> > > 
> > 
> > I don't think this is a big problem in practice, given that these clocks
> > are only consumed by a single driver that manages both PHY and clocks
> > anyway. The clock should always get disabled before the PHY is powered
> > down.
> > 
> > > Another option would be to introduce a safe config for the PHYs and make
> > > sure that the PHY is brought up every time we need it to be up (e.g. via
> > > pm_runtime).
> > 
> > I considered that as well, but what exactly would I use as "safe"
> > configuration? There are lots of PHY configuration registers that are
> > set based on the rate or other parameters of the panel/display
> > connected.
> > 
> > Implementing something like clk_rcg2_shared_ops could presumably work,
> > with the limitation that it will only work if the clock is really off
> > during boot and not already running from XO. Otherwise, I think the
> > simple approach of delaying the clk_set_parent() implemented in this
> > series is still the most straightforward way to solve this issue.
> 
> I know that it works, but it feels a bit clumsy to me.
> 

I realize that adding a field to the platform_driver struct feels a bit
weird, but I think in general requiring more control about when exactly
assigned-clock-parents/rates are applied is a valid use case. The reason
we haven't seen more of these issues is likely mainly because people
just avoid using assigned-clock-parents/rates in these use cases, even
if it would be the right way to describe the hardware.

I'm happy to try implementing the workaround in the Qualcomm clock
drivers, but hearing more opinions about the more general approach of
this patch series would also be good.

Thanks,
Stephan

Powered by blists - more mailing lists