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.

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.

 - 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.

[1]: https://lore.kernel.org/r/20250716134717.4085567-3-mwalle@kernel.org/

Signed-off-by: Stephan Gerhold <stephan.gerhold@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.gerhold@linaro.org>

Hi,

On Thu Aug 14, 2025 at 11:18 AM CEST, Stephan Gerhold wrote:
> 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've come back to this and just noticed that the
assigned-clock-rates do actually work. What doesn't work is the
caching of the clock rate. That bug was then masked by calling
of_clk_set_defaults() again in the driver.

Here is what the driver is doing:
 (1) driver gets handle to the clock with clk_get().
 (2) driver enables clock with clk_enable()
 (3) driver does a clk_get_rate() which returns 0, although there is
     already a hardware default in my case. That got me curious
     again..

Now on the k3 platforms the clocking is handled by a firmware and it
appears that the firmware is reporting a clock rate of 0 unless the
clock is actually enabled. After the clock is enabled it will report
the correct rate. (FWIW, I can modify the hardware/firmware default
rate with the assigned-clock-rates DT property).

I've hacked the clock driver to register all clocks with
CLK_GET_RATE_NO_CACHE and then everything is working as expected.

I'm no expert for the clocking framework, but it seems that
clk_get() will ask the HW for the clk rate and caches it early on.

-michael

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-mwalle@kernel.org/
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@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.gerhold@linaro.org>
> 

-- 
With best wishes
Dmitry

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

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

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. :-)

> > 
> > (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.

> > 
> > 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.

Thanks,
Stephan

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

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

On Tue, Aug 19, 2025 at 12:41:00PM +0200, Stephan Gerhold wrote:
> 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:
> > > > > 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.

I completely agree here.

-- 
With best wishes
Dmitry