[RESEND PATCH v2] drm/msm/dsi: Fix 14nm DSI PHY PLL Lock issue
Loic Poulain
loic.poulain at oss.qualcomm.com
Thu Jul 3 14:28:21 UTC 2025
Hi Dmitry,
On Sun, Jun 29, 2025 at 4:57 PM Dmitry Baryshkov
<dmitry.baryshkov at 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.poulain at 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?
>
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret, "Unable to get iface clk\n");
> > +
> > if (phy->cfg->ops.pll_init) {
> > + /* PLL init will call into clk_register which requires
> > + * register access, so we need to enable power and ahb clock.
> > + */
>
> I think with pm_clk this is no longer reuquired. Could you please verify
> it and drop extra pm_runtime calls from probe?
Will try.
>
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret)
> > + return ret;
> > +
> > ret = phy->cfg->ops.pll_init(phy);
> > + pm_runtime_put(&pdev->dev);
> > if (ret)
> > return dev_err_probe(dev, ret,
> > "PLL init failed; need separate clk driver\n");
> > @@ -725,18 +709,19 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
> > return dev_err_probe(dev, ret,
> > "Failed to register clk provider\n");
> >
> > - dsi_phy_disable_resource(phy);
> > -
> > - platform_set_drvdata(pdev, phy);
> > -
> > return 0;
> > }
> >
> > +static const struct dev_pm_ops dsi_phy_pm_ops = {
> > + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
> > +};
> > +
> > static struct platform_driver dsi_phy_platform_driver = {
> > .probe = dsi_phy_driver_probe,
> > .driver = {
> > .name = "msm_dsi_phy",
> > .of_match_table = dsi_phy_dt_match,
> > + .pm = &dsi_phy_pm_ops,
> > },
> > };
> >
> > @@ -762,7 +747,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
> >
> > dev = &phy->pdev->dev;
> >
> > - ret = dsi_phy_enable_resource(phy);
> > + ret = pm_runtime_resume_and_get(dev);
> > if (ret) {
> > DRM_DEV_ERROR(dev, "%s: resource enable failed, %d\n",
>
> It would be nice to make error prints to follow the code changes.
Sure.
Regards,
Loic
More information about the dri-devel
mailing list