If runtime PM is used for the clock providers and they're part of a
power domain, then the power domain supply will be cut off when runtime
suspended. That means all BLK CTL registers belonging to that power
domain will be reset. Hence, the clock settings will revert to default
values messing up the consumer clock settings.

Also, fix the suspend/resume routines as well, as the clock was left ON
when going to suspend.

Fixes: 5224b189462f ("clk: imx: add i.MX95 BLK CTL clk driver")
Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
 drivers/clk/imx/clk-imx95-blk-ctl.c | 55 ++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/imx/clk-imx95-blk-ctl.c b/drivers/clk/imx/clk-imx95-blk-ctl.c
index 7e88877a62451..7f9bbca517284 100644
--- a/drivers/clk/imx/clk-imx95-blk-ctl.c
+++ b/drivers/clk/imx/clk-imx95-blk-ctl.c
@@ -448,12 +448,36 @@ static int imx95_bc_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void __maybe_unused imx95_bc_save_reg(struct imx95_blk_ctl *bc)
+{
+	const struct imx95_blk_ctl_dev_data *bc_data;
+
+	bc_data = of_device_get_match_data(bc->dev);
+	if (!bc_data)
+		return;
+
+	bc->clk_reg_restore = readl(bc->base + bc_data->clk_reg_offset);
+}
+
+static void __maybe_unused imx95_bc_restore_reg(struct imx95_blk_ctl *bc)
+{
+	const struct imx95_blk_ctl_dev_data *bc_data;
+
+	bc_data = of_device_get_match_data(bc->dev);
+	if (!bc_data)
+		return;
+
+	writel(bc->clk_reg_restore, bc->base + bc_data->clk_reg_offset);
+}
+
 #ifdef CONFIG_PM
 static int imx95_bc_runtime_suspend(struct device *dev)
 {
 	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
 
+	imx95_bc_save_reg(bc);
 	clk_disable_unprepare(bc->clk_apb);
+
 	return 0;
 }
 
@@ -461,7 +485,10 @@ static int imx95_bc_runtime_resume(struct device *dev)
 {
 	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
 
-	return clk_prepare_enable(bc->clk_apb);
+	clk_prepare_enable(bc->clk_apb);
+	imx95_bc_restore_reg(bc);
+
+	return 0;
 }
 #endif
 
@@ -469,22 +496,12 @@ static int imx95_bc_runtime_resume(struct device *dev)
 static int imx95_bc_suspend(struct device *dev)
 {
 	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
-	const struct imx95_blk_ctl_dev_data *bc_data;
-	int ret;
 
-	bc_data = of_device_get_match_data(dev);
-	if (!bc_data)
+	if (pm_runtime_suspended(dev))
 		return 0;
 
-	if (bc_data->rpm_enabled) {
-		ret = pm_runtime_get_sync(bc->dev);
-		if (ret < 0) {
-			pm_runtime_put_noidle(bc->dev);
-			return ret;
-		}
-	}
-
-	bc->clk_reg_restore = readl(bc->base + bc_data->clk_reg_offset);
+	imx95_bc_save_reg(bc);
+	clk_disable_unprepare(bc->clk_apb);
 
 	return 0;
 }
@@ -492,16 +509,12 @@ static int imx95_bc_suspend(struct device *dev)
 static int imx95_bc_resume(struct device *dev)
 {
 	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
-	const struct imx95_blk_ctl_dev_data *bc_data;
 
-	bc_data = of_device_get_match_data(dev);
-	if (!bc_data)
+	if (pm_runtime_suspended(dev))
 		return 0;
 
-	writel(bc->clk_reg_restore, bc->base + bc_data->clk_reg_offset);
-
-	if (bc_data->rpm_enabled)
-		pm_runtime_put(bc->dev);
+	clk_prepare_enable(bc->clk_apb);
+	imx95_bc_restore_reg(bc);
 
 	return 0;
 }
-- 
2.46.1

Subject:
save and store registers at suspend()/resume() function

On Wed, Jul 09, 2025 at 03:23:20PM +0300, Laurentiu Palcu wrote:
> If runtime PM is used for the clock providers and they're part of a
> power domain, then the power domain supply will be cut off when runtime
> suspended. That means all BLK CTL registers belonging to that power
> domain will be reset. Hence, the clock settings will revert to default
> values messing up the consumer clock settings.

Needn't "hence ..."

Save/restore register value at suspend/resume functions to fix this problem.

>
> Also, fix the suspend/resume routines as well, as the clock was left ON
> when going to suspend.

Do you means fix the problem clock left ON after suspend?

Pomain cut off, why clock can left on?

>
> Fixes: 5224b189462f ("clk: imx: add i.MX95 BLK CTL clk driver")
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  drivers/clk/imx/clk-imx95-blk-ctl.c | 55 ++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx95-blk-ctl.c b/drivers/clk/imx/clk-imx95-blk-ctl.c
> index 7e88877a62451..7f9bbca517284 100644
> --- a/drivers/clk/imx/clk-imx95-blk-ctl.c
> +++ b/drivers/clk/imx/clk-imx95-blk-ctl.c
> @@ -448,12 +448,36 @@ static int imx95_bc_probe(struct platform_device *pdev)
>  	return ret;
>  }
>
> +static void __maybe_unused imx95_bc_save_reg(struct imx95_blk_ctl *bc)
> +{
> +	const struct imx95_blk_ctl_dev_data *bc_data;
> +
> +	bc_data = of_device_get_match_data(bc->dev);
> +	if (!bc_data)
> +		return;
> +
> +	bc->clk_reg_restore = readl(bc->base + bc_data->clk_reg_offset);
> +}
> +
> +static void __maybe_unused imx95_bc_restore_reg(struct imx95_blk_ctl *bc)
> +{
> +	const struct imx95_blk_ctl_dev_data *bc_data;
> +
> +	bc_data = of_device_get_match_data(bc->dev);

Generally, bc_data should in imx95_blk_ctl_dev_data and set once at probe.

So imx95_bc_save_reg() and imx95_bc_restore_reg() will be simpfied.

> +	if (!bc_data)
> +		return;
> +
> +	writel(bc->clk_reg_restore, bc->base + bc_data->clk_reg_offset);
> +}
> +
>  #ifdef CONFIG_PM
>  static int imx95_bc_runtime_suspend(struct device *dev)
>  {
>  	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
>
> +	imx95_bc_save_reg(bc);

this help function just one line. direct use

writel(bc->clk_reg_restore, bc->base + bc->bc_data->clk_reg_offset);

>  	clk_disable_unprepare(bc->clk_apb);
> +
>  	return 0;
>  }
>
> @@ -461,7 +485,10 @@ static int imx95_bc_runtime_resume(struct device *dev)
>  {
>  	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
>
> -	return clk_prepare_enable(bc->clk_apb);
> +	clk_prepare_enable(bc->clk_apb);

Need check ret value;

> +	imx95_bc_restore_reg(bc);
> +
> +	return 0;
>  }
>  #endif
>
> @@ -469,22 +496,12 @@ static int imx95_bc_runtime_resume(struct device *dev)
>  static int imx95_bc_suspend(struct device *dev)
>  {
>  	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
> -	const struct imx95_blk_ctl_dev_data *bc_data;
> -	int ret;
>
> -	bc_data = of_device_get_match_data(dev);
> -	if (!bc_data)
> +	if (pm_runtime_suspended(dev))
>  		return 0;
>
> -	if (bc_data->rpm_enabled) {
> -		ret = pm_runtime_get_sync(bc->dev);
> -		if (ret < 0) {
> -			pm_runtime_put_noidle(bc->dev);
> -			return ret;
> -		}
> -	}
> -
> -	bc->clk_reg_restore = readl(bc->base + bc_data->clk_reg_offset);
> +	imx95_bc_save_reg(bc);
> +	clk_disable_unprepare(bc->clk_apb);
>
>  	return 0;
>  }
> @@ -492,16 +509,12 @@ static int imx95_bc_suspend(struct device *dev)
>  static int imx95_bc_resume(struct device *dev)
>  {
>  	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
> -	const struct imx95_blk_ctl_dev_data *bc_data;
>
> -	bc_data = of_device_get_match_data(dev);
> -	if (!bc_data)
> +	if (pm_runtime_suspended(dev))
>  		return 0;
>
> -	writel(bc->clk_reg_restore, bc->base + bc_data->clk_reg_offset);
> -
> -	if (bc_data->rpm_enabled)
> -		pm_runtime_put(bc->dev);
> +	clk_prepare_enable(bc->clk_apb);
> +	imx95_bc_restore_reg(bc);
>
>  	return 0;
>  }

look like imx95_bc_suspend(resume) is simple enough

Can you use DEFINE_RUNTIME_DEV_PM_OPS?

Frank

> --
> 2.46.1
>

Hi Frank,

On Fri, Jul 11, 2025 at 12:03:22AM -0400, Frank Li wrote:
> Subject:
> save and store registers at suspend()/resume() function
> 
> On Wed, Jul 09, 2025 at 03:23:20PM +0300, Laurentiu Palcu wrote:
> > If runtime PM is used for the clock providers and they're part of a
> > power domain, then the power domain supply will be cut off when runtime
> > suspended. That means all BLK CTL registers belonging to that power
> > domain will be reset. Hence, the clock settings will revert to default
> > values messing up the consumer clock settings.
> 
> Needn't "hence ..."
> 
> Save/restore register value at suspend/resume functions to fix this problem.
> 
> >
> > Also, fix the suspend/resume routines as well, as the clock was left ON
> > when going to suspend.
> 
> Do you means fix the problem clock left ON after suspend?
> 
> Pomain cut off, why clock can left on?

I'm referring to the BLK_CTL clock supplier here. We need to call
clk_disable_unprepare() in suspend() too.

> 
> >
> > Fixes: 5224b189462f ("clk: imx: add i.MX95 BLK CTL clk driver")
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > ---
> >  drivers/clk/imx/clk-imx95-blk-ctl.c | 55 ++++++++++++++++++-----------
> >  1 file changed, 34 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx95-blk-ctl.c b/drivers/clk/imx/clk-imx95-blk-ctl.c
> > index 7e88877a62451..7f9bbca517284 100644
> > --- a/drivers/clk/imx/clk-imx95-blk-ctl.c
> > +++ b/drivers/clk/imx/clk-imx95-blk-ctl.c
> > @@ -448,12 +448,36 @@ static int imx95_bc_probe(struct platform_device *pdev)
> >  	return ret;
> >  }
> >
> > +static void __maybe_unused imx95_bc_save_reg(struct imx95_blk_ctl *bc)
> > +{
> > +	const struct imx95_blk_ctl_dev_data *bc_data;
> > +
> > +	bc_data = of_device_get_match_data(bc->dev);
> > +	if (!bc_data)
> > +		return;
> > +
> > +	bc->clk_reg_restore = readl(bc->base + bc_data->clk_reg_offset);
> > +}
> > +
> > +static void __maybe_unused imx95_bc_restore_reg(struct imx95_blk_ctl *bc)
> > +{
> > +	const struct imx95_blk_ctl_dev_data *bc_data;
> > +
> > +	bc_data = of_device_get_match_data(bc->dev);
> 
> Generally, bc_data should in imx95_blk_ctl_dev_data and set once at probe.
> 
> So imx95_bc_save_reg() and imx95_bc_restore_reg() will be simpfied.

Agree, I'll do this in a separate patch though for easier review.

> 
> > +	if (!bc_data)
> > +		return;
> > +
> > +	writel(bc->clk_reg_restore, bc->base + bc_data->clk_reg_offset);
> > +}
> > +
> >  #ifdef CONFIG_PM
> >  static int imx95_bc_runtime_suspend(struct device *dev)
> >  {
> >  	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
> >
> > +	imx95_bc_save_reg(bc);
> 
> this help function just one line. direct use
> 
> writel(bc->clk_reg_restore, bc->base + bc->bc_data->clk_reg_offset);

ok.

> 
> >  	clk_disable_unprepare(bc->clk_apb);
> > +
> >  	return 0;
> >  }
> >
> > @@ -461,7 +485,10 @@ static int imx95_bc_runtime_resume(struct device *dev)
> >  {
> >  	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
> >
> > -	return clk_prepare_enable(bc->clk_apb);
> > +	clk_prepare_enable(bc->clk_apb);
> 
> Need check ret value;
> 
> > +	imx95_bc_restore_reg(bc);
> > +
> > +	return 0;
> >  }
> >  #endif
> >
> > @@ -469,22 +496,12 @@ static int imx95_bc_runtime_resume(struct device *dev)
> >  static int imx95_bc_suspend(struct device *dev)
> >  {
> >  	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
> > -	const struct imx95_blk_ctl_dev_data *bc_data;
> > -	int ret;
> >
> > -	bc_data = of_device_get_match_data(dev);
> > -	if (!bc_data)
> > +	if (pm_runtime_suspended(dev))
> >  		return 0;
> >
> > -	if (bc_data->rpm_enabled) {
> > -		ret = pm_runtime_get_sync(bc->dev);
> > -		if (ret < 0) {
> > -			pm_runtime_put_noidle(bc->dev);
> > -			return ret;
> > -		}
> > -	}
> > -
> > -	bc->clk_reg_restore = readl(bc->base + bc_data->clk_reg_offset);
> > +	imx95_bc_save_reg(bc);
> > +	clk_disable_unprepare(bc->clk_apb);
> >
> >  	return 0;
> >  }
> > @@ -492,16 +509,12 @@ static int imx95_bc_suspend(struct device *dev)
> >  static int imx95_bc_resume(struct device *dev)
> >  {
> >  	struct imx95_blk_ctl *bc = dev_get_drvdata(dev);
> > -	const struct imx95_blk_ctl_dev_data *bc_data;
> >
> > -	bc_data = of_device_get_match_data(dev);
> > -	if (!bc_data)
> > +	if (pm_runtime_suspended(dev))
> >  		return 0;
> >
> > -	writel(bc->clk_reg_restore, bc->base + bc_data->clk_reg_offset);
> > -
> > -	if (bc_data->rpm_enabled)
> > -		pm_runtime_put(bc->dev);
> > +	clk_prepare_enable(bc->clk_apb);
> > +	imx95_bc_restore_reg(bc);
> >
> >  	return 0;
> >  }
> 
> look like imx95_bc_suspend(resume) is simple enough
> 
> Can you use DEFINE_RUNTIME_DEV_PM_OPS?

Not really. Some clocks provided by BLK_CTL use RPM, others don't. If we
use DEFINE_RUNTIME_DEV_PM_OPS, then suspend()/resume() routines will
never be called when rpm_enable is false.

Thanks,
Laurentiu

> 
> Frank
> 
> > --
> > 2.46.1
> >