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 <[email protected]>
> ---
> 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.
> + 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?
> + 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.
> __func__, ret);
> @@ -808,7 +793,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
> phy_en_fail:
> regulator_bulk_disable(phy->cfg->num_regulators, phy->supplies);
> reg_en_fail:
> - dsi_phy_disable_resource(phy);
> + pm_runtime_put(dev);
> res_en_fail:
> return ret;
> }
> @@ -821,7 +806,7 @@ void msm_dsi_phy_disable(struct msm_dsi_phy *phy)
> phy->cfg->ops.disable(phy);
>
> regulator_bulk_disable(phy->cfg->num_regulators, phy->supplies);
> - dsi_phy_disable_resource(phy);
> + pm_runtime_put(&phy->pdev->dev);
> }
>
> void msm_dsi_phy_set_usecase(struct msm_dsi_phy *phy,
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> index 7ea608f620fe..0c65821a3db2 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> @@ -103,7 +103,6 @@ struct msm_dsi_phy {
> phys_addr_t lane_size;
> int id;
>
> - struct clk *ahb_clk;
> struct regulator_bulk_data *supplies;
>
> struct msm_dsi_dphy_timing timing;
> --
> 2.34.1
>
--
With best wishes
Dmitry
