On Thu, 3 Jul 2025 at 17:28, Loic Poulain <loic.poul...@oss.qualcomm.com> wrote:
>
> Hi Dmitry,
>
> On Sun, Jun 29, 2025 at 4:57 PM Dmitry Baryshkov
> <dmitry.barysh...@oss.qualcomm.com> wrote:
> >
> > On Sun, Jun 29, 2025 at 10:50:36AM +0200, Loic Poulain wrote:
> > > To configure and enable the DSI PHY PLL clocks, the MDSS AHB clock must
> > > be active for MMIO operations. Typically, this AHB clock is enabled as
> > > part of the DSI PHY interface enabling (dsi_phy_enable_resource).
> > >
> > > However, since these PLL clocks are registered as clock entities, they
> > > can be enabled independently of the DSI PHY interface, leading to
> > > enabling failures and subsequent warnings:
> > >
> > > ```
> > > msm_dsi_phy 5e94400.phy: [drm:dsi_pll_14nm_vco_prepare] *ERROR* DSI PLL
> > > lock failed
> > > ------------[ cut here ]------------
> > > dsi0pllbyte already disabled
> > > WARNING: CPU: 3 PID: 1 at drivers/clk/clk.c:1194
> > > clk_core_disable+0xa4/0xac
> > > CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Tainted:
> > > Tainted: [W]=WARN
> > > Hardware name: Qualcomm Technologies, Inc. Robotics RB1 (DT)
> > > pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > [...]
> > > ```
> > >
> > > This issue is particularly prevalent at boot time during the disabling of
> > > unused clocks (clk_disable_unused()) which includes enabling the parent
> > > clock(s) when CLK_OPS_PARENT_ENABLE flag is set (this is the case for the
> > > 14nm DSI PHY PLL consumers).
> > >
> > > To resolve this issue, we move the AHB clock as a PM dependency of the DSI
> > > PHY device (via pm_clk). Since the DSI PHY device is the parent of the PLL
> > > clocks, this resolves the PLL/AHB dependency. Now the AHB clock is enabled
> > > prior the PLL clk_prepare callback, as part of the runtime-resume chain.
> > >
> > > We also eliminate dsi_phy_[enable|disable]_resource functions, which are
> > > superseded by runtime PM.
> > >
> > > Signed-off-by: Loic Poulain <loic.poul...@oss.qualcomm.com>
> > > ---
> > > v2: Move AHB clock into a proper PM dep instead of manually toggling it
> > > from the PLL clock driver.
> > >
> > > drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 65 +++++++++++----------------
> > > drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 -
> > > 2 files changed, 25 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > > index 5973d7325699..015cb579c669 100644
> > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > > @@ -5,6 +5,8 @@
> > >
> > > #include <linux/clk-provider.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/pm_clock.h>
> > > +#include <linux/pm_runtime.h>
> > > #include <dt-bindings/phy/phy.h>
> > >
> > > #include "dsi_phy.h"
> > > @@ -511,30 +513,6 @@ int msm_dsi_cphy_timing_calc_v4(struct
> > > msm_dsi_dphy_timing *timing,
> > > return 0;
> > > }
> > >
> > > -static int dsi_phy_enable_resource(struct msm_dsi_phy *phy)
> > > -{
> > > - struct device *dev = &phy->pdev->dev;
> > > - int ret;
> > > -
> > > - ret = pm_runtime_resume_and_get(dev);
> > > - if (ret)
> > > - return ret;
> > > -
> > > - ret = clk_prepare_enable(phy->ahb_clk);
> > > - if (ret) {
> > > - DRM_DEV_ERROR(dev, "%s: can't enable ahb clk, %d\n",
> > > __func__, ret);
> > > - pm_runtime_put_sync(dev);
> > > - }
> > > -
> > > - return ret;
> > > -}
> > > -
> > > -static void dsi_phy_disable_resource(struct msm_dsi_phy *phy)
> > > -{
> > > - clk_disable_unprepare(phy->ahb_clk);
> > > - pm_runtime_put(&phy->pdev->dev);
> > > -}
> > > -
> > > static const struct of_device_id dsi_phy_dt_match[] = {
> > > #ifdef CONFIG_DRM_MSM_DSI_28NM_PHY
> > > { .compatible = "qcom,dsi-phy-28nm-hpm",
> > > @@ -696,24 +674,30 @@ static int dsi_phy_driver_probe(struct
> > > platform_device *pdev)
> > > if (ret)
> > > return ret;
> > >
> > > - phy->ahb_clk = msm_clk_get(pdev, "iface");
> > > - if (IS_ERR(phy->ahb_clk))
> > > - return dev_err_probe(dev, PTR_ERR(phy->ahb_clk),
> > > - "Unable to get ahb clk\n");
> > > + platform_set_drvdata(pdev, phy);
> > >
> > > - ret = devm_pm_runtime_enable(&pdev->dev);
> > > + ret = devm_pm_runtime_enable(dev);
> > > if (ret)
> > > return ret;
> > >
> > > - /* PLL init will call into clk_register which requires
> > > - * register access, so we need to enable power and ahb clock.
> > > - */
> > > - ret = dsi_phy_enable_resource(phy);
> > > + ret = devm_pm_clk_create(dev);
> > > if (ret)
> > > return ret;
> > >
> > > + ret = pm_clk_add(dev, "iface");
> >
> > This will break booting the kernel with some old DTS (before 6.0), where
> > we had iface_clk as a DSI PHY clock. Please document it in the commit
> > message.
>
> Do we want to preserve backward compatibility and introduce some sort
> of msm_pm_clk_add to handle both?
Just state that it breaks compat with kernels before 6.0. Then if
anybody has concerns, they can stand up.
--
With best wishes
Dmitry
