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?
>
> (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.
>
> 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.
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).
--
With best wishes
Dmitry
