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).
> [1]: https://lore.kernel.org/r/20250716134717.4085567-3-mwa...@kernel.org/
>
> Signed-off-by: Stephan Gerhold <stephan.gerh...@linaro.org>
> ---
> Stephan Gerhold (2):
> driver core: platform: Add option to skip/delay applying clock defaults
> drm/msm: dp: Delay applying clock defaults until PHY is fully enabled
>
> drivers/base/platform.c | 8 +++++---
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 10 ++++++++++
> drivers/gpu/drm/msm/dp/dp_display.c | 2 ++
> include/linux/platform_device.h | 6 ++++++
> 4 files changed, 23 insertions(+), 3 deletions(-)
> ---
> base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
> change-id: 20250812-platform-delay-clk-defaults-44002859f5c5
>
> Best regards,
> --
> Stephan Gerhold <stephan.gerh...@linaro.org>
>
--
With best wishes
Dmitry
