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.
>
> > >
> > > 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.
>
> Thanks,
> Stephan
--
With best wishes
Dmitry
