[1/3] soc: mtk-pm-domains: Fix the clock prepared issue

Message ID	20210531043502.2702645-1-hsinyi@chromium.org (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=FJ4T=K2=lists.infradead.org=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@kernel.org> DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6B4C66120E From: Hsin-Yi Wang <hsinyi@chromium.org> To: Matthias Brugger <matthias.bgg@gmail.com> Cc: linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, Enric Balletbo i Serra <enric.balletbo@collabora.com>, chun-jie.chen@mediatek.corp-partner.google.com, Yong Wu <yong.wu@mediatek.com> Subject: [PATCH 1/3] soc: mtk-pm-domains: Fix the clock prepared issue Date: Mon, 31 May 2021 12:35:00 +0800 Message-Id: <20210531043502.2702645-1-hsinyi@chromium.org> MIME-Version: 1.0 Precedence: list Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" <linux-mediatek-bounces@lists.infradead.org> Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org
Series	[1/3] soc: mtk-pm-domains: Fix the clock prepared issue \| expand [1/3] soc: mtk-pm-domains: Fix the clock prepared issue [2/3] soc: mtk-pm-domains: do not register smi node as syscon [3/3] arm64: dts: mt8183: remove syscon from smi_common node

Message ID

20210531043502.2702645-1-hsinyi@chromium.org (mailing list archive)

State

New, archived

Headers

DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6B4C66120E
From: Hsin-Yi Wang <hsinyi@chromium.org>
To: Matthias Brugger <matthias.bgg@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org,
 linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
 Enric Balletbo i Serra <enric.balletbo@collabora.com>,
 chun-jie.chen@mediatek.corp-partner.google.com,
 Yong Wu <yong.wu@mediatek.com>
Subject: [PATCH 1/3] soc: mtk-pm-domains: Fix the clock prepared issue
Date: Mon, 31 May 2021 12:35:00 +0800
Message-Id: <20210531043502.2702645-1-hsinyi@chromium.org>
MIME-Version: 1.0
Precedence: list
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Sender: "Linux-mediatek" <linux-mediatek-bounces@lists.infradead.org>
Errors-To: 
 linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org

Series

[1/3] soc: mtk-pm-domains: Fix the clock prepared issue | expand

Commit Message

Hsin-Yi Wang May 31, 2021, 4:35 a.m. UTC

From: Weiyi Lu <weiyi.lu@mediatek.com>

In this new power domain driver, when adding one power domain
it will prepare the depenedent clocks at the same.
So we only do clk_bulk_enable/disable control during power ON/OFF.
When system suspend, the pm runtime framework will forcely power off
power domains. However, the dependent clocks are disabled but kept
preapred.

In MediaTek clock drivers, PLL would be turned ON when we do
clk_bulk_prepare control.

Clock hierarchy:
PLL -->
       DIV_CK -->
                 CLK_MUX
                 (may be dependent clocks)
                         -->
                             SUBSYS_CG
                             (may be dependent clocks)

It will lead some unexpected clock states during system suspend.
This patch will fix by doing prepare_enable/disable_unprepare on
dependent clocks at the same time while we are going to power on/off
any power domain.

Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/soc/mediatek/mtk-pm-domains.c | 31 +++++++--------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

Comments

Chun-Jie Chen May 31, 2021, 10:42 a.m. UTC | #1

On Mon, 2021-05-31 at 12:35 +0800, Hsin-Yi Wang wrote:
> From: Weiyi Lu <weiyi.lu@mediatek.com>
> 
> In this new power domain driver, when adding one power domain
> it will prepare the depenedent clocks at the same.
> So we only do clk_bulk_enable/disable control during power ON/OFF.
> When system suspend, the pm runtime framework will forcely power off
> power domains. However, the dependent clocks are disabled but kept
> preapred.
> 
> In MediaTek clock drivers, PLL would be turned ON when we do
> clk_bulk_prepare control.
> 
> Clock hierarchy:
> PLL -->
>        DIV_CK -->
>                  CLK_MUX
>                  (may be dependent clocks)
>                          -->
>                              SUBSYS_CG
>                              (may be dependent clocks)
> 
> It will lead some unexpected clock states during system suspend.
> This patch will fix by doing prepare_enable/disable_unprepare on
> dependent clocks at the same time while we are going to power on/off
> any power domain.
> 
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>

Reviewed-by: chun-jie.chen <chun-jie.chen@mediatek.com>

> ---
>  drivers/soc/mediatek/mtk-pm-domains.c | 31 +++++++----------------
> ----
>  1 file changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c
> b/drivers/soc/mediatek/mtk-pm-domains.c
> index 0af00efa0ef8..536d8c64b2b4 100644
> --- a/drivers/soc/mediatek/mtk-pm-domains.c
> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> @@ -211,7 +211,7 @@ static int scpsys_power_on(struct
> generic_pm_domain *genpd)
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_bulk_enable(pd->num_clks, pd->clks);
> +	ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
>  	if (ret)
>  		goto err_reg;
>  
> @@ -229,7 +229,7 @@ static int scpsys_power_on(struct
> generic_pm_domain *genpd)
>  	regmap_clear_bits(scpsys->base, pd->data->ctl_offs,
> PWR_ISO_BIT);
>  	regmap_set_bits(scpsys->base, pd->data->ctl_offs,
> PWR_RST_B_BIT);
>  
> -	ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
> +	ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd-
> >subsys_clks);
>  	if (ret)
>  		goto err_pwr_ack;
>  
> @@ -246,9 +246,9 @@ static int scpsys_power_on(struct
> generic_pm_domain *genpd)
>  err_disable_sram:
>  	scpsys_sram_disable(pd);
>  err_disable_subsys_clks:
> -	clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> +	clk_bulk_disable_unprepare(pd->num_subsys_clks, pd-
> >subsys_clks);
>  err_pwr_ack:
> -	clk_bulk_disable(pd->num_clks, pd->clks);
> +	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>  err_reg:
>  	scpsys_regulator_disable(pd->supply);
>  	return ret;
> @@ -269,7 +269,7 @@ static int scpsys_power_off(struct
> generic_pm_domain *genpd)
>  	if (ret < 0)
>  		return ret;
>  
> -	clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> +	clk_bulk_disable_unprepare(pd->num_subsys_clks, pd-
> >subsys_clks);
>  
>  	/* subsys power off */
>  	regmap_clear_bits(scpsys->base, pd->data->ctl_offs,
> PWR_RST_B_BIT);
> @@ -284,7 +284,7 @@ static int scpsys_power_off(struct
> generic_pm_domain *genpd)
>  	if (ret < 0)
>  		return ret;
>  
> -	clk_bulk_disable(pd->num_clks, pd->clks);
> +	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>  
>  	scpsys_regulator_disable(pd->supply);
>  
> @@ -405,14 +405,6 @@ generic_pm_domain *scpsys_add_one_domain(struct
> scpsys *scpsys, struct device_no
>  		pd->subsys_clks[i].clk = clk;
>  	}
>  
> -	ret = clk_bulk_prepare(pd->num_clks, pd->clks);
> -	if (ret)
> -		goto err_put_subsys_clocks;
> -
> -	ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
> -	if (ret)
> -		goto err_unprepare_clocks;
> -
>  	/*
>  	 * Initially turn on all domains to make the domains usable
>  	 * with !CONFIG_PM and to get the hardware in sync with the
> @@ -427,7 +419,7 @@ generic_pm_domain *scpsys_add_one_domain(struct
> scpsys *scpsys, struct device_no
>  		ret = scpsys_power_on(&pd->genpd);
>  		if (ret < 0) {
>  			dev_err(scpsys->dev, "%pOF: failed to power on
> domain: %d\n", node, ret);
> -			goto err_unprepare_clocks;
> +			goto err_put_subsys_clocks;
>  		}
>  	}
>  
> @@ -435,7 +427,7 @@ generic_pm_domain *scpsys_add_one_domain(struct
> scpsys *scpsys, struct device_no
>  		ret = -EINVAL;
>  		dev_err(scpsys->dev,
>  			"power domain with id %d already exists, check
> your device-tree\n", id);
> -		goto err_unprepare_subsys_clocks;
> +		goto err_put_subsys_clocks;
>  	}
>  
>  	if (!pd->data->name)
> @@ -455,10 +447,6 @@ generic_pm_domain *scpsys_add_one_domain(struct
> scpsys *scpsys, struct device_no
>  
>  	return scpsys->pd_data.domains[id];
>  
> -err_unprepare_subsys_clocks:
> -	clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> -err_unprepare_clocks:
> -	clk_bulk_unprepare(pd->num_clks, pd->clks);
>  err_put_subsys_clocks:
>  	clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
>  err_put_clocks:
> @@ -537,10 +525,7 @@ static void scpsys_remove_one_domain(struct
> scpsys_domain *pd)
>  			"failed to remove domain '%s' : %d - state may
> be inconsistent\n",
>  			pd->genpd.name, ret);
>  
> -	clk_bulk_unprepare(pd->num_clks, pd->clks);
>  	clk_bulk_put(pd->num_clks, pd->clks);
> -
> -	clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
>  	clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
>  }
>

Enric Balletbo Serra May 31, 2021, 9:38 p.m. UTC | #2

Hi Hsin-Yi,

Thank you for the patch.

Missatge de Hsin-Yi Wang <hsinyi@chromium.org> del dia dl., 31 de maig
2021 a les 6:35:
>
> From: Weiyi Lu <weiyi.lu@mediatek.com>
>
> In this new power domain driver, when adding one power domain
> it will prepare the depenedent clocks at the same.

Typo: s/depenedent/dependent/

> So we only do clk_bulk_enable/disable control during power ON/OFF.
> When system suspend, the pm runtime framework will forcely power off
> power domains. However, the dependent clocks are disabled but kept
> preapred.

Typo: s/preapred/prepared

>
> In MediaTek clock drivers, PLL would be turned ON when we do
> clk_bulk_prepare control.
>
> Clock hierarchy:
> PLL -->
>        DIV_CK -->
>                  CLK_MUX
>                  (may be dependent clocks)
>                          -->
>                              SUBSYS_CG
>                              (may be dependent clocks)
>
> It will lead some unexpected clock states during system suspend.
> This patch will fix by doing prepare_enable/disable_unprepare on
> dependent clocks at the same time while we are going to power on/off
> any power domain.
>

I think it would be nice to have a Fixes tag here, so this can be
backported more easily.

> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>

Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

> ---
>  drivers/soc/mediatek/mtk-pm-domains.c | 31 +++++++--------------------
>  1 file changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
> index 0af00efa0ef8..536d8c64b2b4 100644
> --- a/drivers/soc/mediatek/mtk-pm-domains.c
> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> @@ -211,7 +211,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>         if (ret)
>                 return ret;
>
> -       ret = clk_bulk_enable(pd->num_clks, pd->clks);
> +       ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
>         if (ret)
>                 goto err_reg;
>
> @@ -229,7 +229,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>         regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_ISO_BIT);
>         regmap_set_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
>
> -       ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
> +       ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
>         if (ret)
>                 goto err_pwr_ack;
>
> @@ -246,9 +246,9 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>  err_disable_sram:
>         scpsys_sram_disable(pd);
>  err_disable_subsys_clks:
> -       clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> +       clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
>  err_pwr_ack:
> -       clk_bulk_disable(pd->num_clks, pd->clks);
> +       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>  err_reg:
>         scpsys_regulator_disable(pd->supply);
>         return ret;
> @@ -269,7 +269,7 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>         if (ret < 0)
>                 return ret;
>
> -       clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> +       clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
>
>         /* subsys power off */
>         regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
> @@ -284,7 +284,7 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>         if (ret < 0)
>                 return ret;
>
> -       clk_bulk_disable(pd->num_clks, pd->clks);
> +       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>
>         scpsys_regulator_disable(pd->supply);
>
> @@ -405,14 +405,6 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
>                 pd->subsys_clks[i].clk = clk;
>         }
>
> -       ret = clk_bulk_prepare(pd->num_clks, pd->clks);
> -       if (ret)
> -               goto err_put_subsys_clocks;
> -
> -       ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
> -       if (ret)
> -               goto err_unprepare_clocks;
> -
>         /*
>          * Initially turn on all domains to make the domains usable
>          * with !CONFIG_PM and to get the hardware in sync with the
> @@ -427,7 +419,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
>                 ret = scpsys_power_on(&pd->genpd);
>                 if (ret < 0) {
>                         dev_err(scpsys->dev, "%pOF: failed to power on domain: %d\n", node, ret);
> -                       goto err_unprepare_clocks;
> +                       goto err_put_subsys_clocks;
>                 }
>         }
>
> @@ -435,7 +427,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
>                 ret = -EINVAL;
>                 dev_err(scpsys->dev,
>                         "power domain with id %d already exists, check your device-tree\n", id);
> -               goto err_unprepare_subsys_clocks;
> +               goto err_put_subsys_clocks;
>         }
>
>         if (!pd->data->name)
> @@ -455,10 +447,6 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
>
>         return scpsys->pd_data.domains[id];
>
> -err_unprepare_subsys_clocks:
> -       clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> -err_unprepare_clocks:
> -       clk_bulk_unprepare(pd->num_clks, pd->clks);
>  err_put_subsys_clocks:
>         clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
>  err_put_clocks:
> @@ -537,10 +525,7 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
>                         "failed to remove domain '%s' : %d - state may be inconsistent\n",
>                         pd->genpd.name, ret);
>
> -       clk_bulk_unprepare(pd->num_clks, pd->clks);
>         clk_bulk_put(pd->num_clks, pd->clks);
> -
> -       clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
>         clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
>  }
>
> --
> 2.32.0.rc0.204.g9fa02ecfa5-goog
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
index 0af00efa0ef8..536d8c64b2b4 100644
--- a/drivers/soc/mediatek/mtk-pm-domains.c
+++ b/drivers/soc/mediatek/mtk-pm-domains.c
@@ -211,7 +211,7 @@  static int scpsys_power_on(struct generic_pm_domain *genpd)
 	if (ret)
 		return ret;
 
-	ret = clk_bulk_enable(pd->num_clks, pd->clks);
+	ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
 	if (ret)
 		goto err_reg;
 
@@ -229,7 +229,7 @@  static int scpsys_power_on(struct generic_pm_domain *genpd)
 	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_ISO_BIT);
 	regmap_set_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
 
-	ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
+	ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
 	if (ret)
 		goto err_pwr_ack;
 
@@ -246,9 +246,9 @@  static int scpsys_power_on(struct generic_pm_domain *genpd)
 err_disable_sram:
 	scpsys_sram_disable(pd);
 err_disable_subsys_clks:
-	clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
+	clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
 err_pwr_ack:
-	clk_bulk_disable(pd->num_clks, pd->clks);
+	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
 err_reg:
 	scpsys_regulator_disable(pd->supply);
 	return ret;
@@ -269,7 +269,7 @@  static int scpsys_power_off(struct generic_pm_domain *genpd)
 	if (ret < 0)
 		return ret;
 
-	clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
+	clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
 
 	/* subsys power off */
 	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
@@ -284,7 +284,7 @@  static int scpsys_power_off(struct generic_pm_domain *genpd)
 	if (ret < 0)
 		return ret;
 
-	clk_bulk_disable(pd->num_clks, pd->clks);
+	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
 
 	scpsys_regulator_disable(pd->supply);
 
@@ -405,14 +405,6 @@  generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 		pd->subsys_clks[i].clk = clk;
 	}
 
-	ret = clk_bulk_prepare(pd->num_clks, pd->clks);
-	if (ret)
-		goto err_put_subsys_clocks;
-
-	ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
-	if (ret)
-		goto err_unprepare_clocks;
-
 	/*
 	 * Initially turn on all domains to make the domains usable
 	 * with !CONFIG_PM and to get the hardware in sync with the
@@ -427,7 +419,7 @@  generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 		ret = scpsys_power_on(&pd->genpd);
 		if (ret < 0) {
 			dev_err(scpsys->dev, "%pOF: failed to power on domain: %d\n", node, ret);
-			goto err_unprepare_clocks;
+			goto err_put_subsys_clocks;
 		}
 	}
 
@@ -435,7 +427,7 @@  generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 		ret = -EINVAL;
 		dev_err(scpsys->dev,
 			"power domain with id %d already exists, check your device-tree\n", id);
-		goto err_unprepare_subsys_clocks;
+		goto err_put_subsys_clocks;
 	}
 
 	if (!pd->data->name)
@@ -455,10 +447,6 @@  generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 
 	return scpsys->pd_data.domains[id];
 
-err_unprepare_subsys_clocks:
-	clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
-err_unprepare_clocks:
-	clk_bulk_unprepare(pd->num_clks, pd->clks);
 err_put_subsys_clocks:
 	clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
 err_put_clocks:
@@ -537,10 +525,7 @@  static void scpsys_remove_one_domain(struct scpsys_domain *pd)
 			"failed to remove domain '%s' : %d - state may be inconsistent\n",
 			pd->genpd.name, ret);
 
-	clk_bulk_unprepare(pd->num_clks, pd->clks);
 	clk_bulk_put(pd->num_clks, pd->clks);
-
-	clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
 	clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
 }

[1/3] soc: mtk-pm-domains: Fix the clock prepared issue

Commit Message

Comments

Patch