Re: [PATCH 2/2] clk: qcom: rcg2, msm/dsi: Fix hangs caused by register writes while clocks are off

[Dmitry Baryshkov]

On Sat, Dec 13, 2025 at 12:08:17AM +0100, David Heidelberg via B4 Relay wrote:
> From: Petr Hodina <petr.hod...@protonmail.com>
> 
> This patch fixes system hangs that occur when RCG2 and DSI code paths
> perform register accesses while the associated clocks or power domains
> are disabled.

In general this should not be happening. Do you have a description of
the corresponding code path?

> 
> For the Qualcomm RCG2 clock driver, updating M/N/D registers while the
> clock is gated can cause the hardware to lock up. Avoid toggling the
> update bit when the clock is disabled and instead write the configuration
> directly.
> 
> Signed-off-by: Petr Hodina <petr.hod...@protonmail.com>
> Signed-off-by: David Heidelberg <da...@ixit.cz>
> ---
>  drivers/clk/qcom/clk-rcg2.c        | 18 ++++++++++++++++++
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 13 +++++++++++++
>  2 files changed, 31 insertions(+)

This needs to be split into two patches.

> 
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index e18cb8807d735..a18d2b9319670 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -1182,6 +1182,24 @@ static int clk_pixel_set_rate(struct clk_hw *hw, 
> unsigned long rate,
>               f.m = frac->num;
>               f.n = frac->den;
>  
> +             /*
> +              * If clock is disabled, update the M, N and D registers and
> +              * don't hit the update bit.
> +              */
> +             if (!clk_hw_is_enabled(hw)) {
> +                     int ret;
> +
> +                     ret = regmap_read(rcg->clkr.regmap, 
> RCG_CFG_OFFSET(rcg), &cfg);
> +                     if (ret)
> +                             return ret;
> +
> +                     ret = __clk_rcg2_configure(rcg, &f, &cfg);
> +                     if (ret)
> +                             return ret;
> +
> +                     return regmap_write(rcg->clkr.regmap, 
> RCG_CFG_OFFSET(rcg), cfg);
> +             }
> +
>               return clk_rcg2_configure(rcg, &f);
>       }
>       return -EINVAL;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index e0de545d40775..374ed966e960b 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -762,6 +762,12 @@ dsi_get_cmd_fmt(const enum mipi_dsi_pixel_format 
> mipi_fmt)
>  
>  static void dsi_ctrl_disable(struct msm_dsi_host *msm_host)
>  {
> +     /* Check if we're already powered off before writing registers */
> +     if (!msm_host->power_on) {
> +             pr_info("DSI CTRL: Skipping register write - host already 
> powered off\n");

It definitely should be dev_something. Probably dev_warn().

> +             return;
> +     }
> +
>       dsi_write(msm_host, REG_DSI_CTRL, 0);
>  }
>  
> @@ -2489,6 +2495,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
>  {
>       struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>       const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
> +     int ret;
> +

Extra empty line

>  
>       mutex_lock(&msm_host->dev_mutex);
>       if (!msm_host->power_on) {
> @@ -2496,6 +2504,11 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
>               goto unlock_ret;
>       }
>  
> +     /* Ensure clocks are enabled before register access */

And this looks like yet another fix, prompting for a separate commmit.

> +     ret = pm_runtime_get_sync(&msm_host->pdev->dev);
> +     if (ret < 0)
> +             pm_runtime_put_noidle(&msm_host->pdev->dev);

pm_runtime_resume_and_get()

Also, where is a corresponding put() ? We are leaking the runtime PM
counter otherwise.

> +
>       dsi_ctrl_disable(msm_host);
>  
>       pinctrl_pm_select_sleep_state(&msm_host->pdev->dev);
> 
> -- 
> 2.51.0
> 
> 

-- 
With best wishes
Dmitry