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

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

[Hsin-Yi Wang]

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(-)

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] soc: mtk-pm-domains: do not register smi node as syscon

[Hsin-Yi Wang]

Mediatek requires mmsys clocks to be unprepared during suspend,
otherwise system has chances to hang.

syscon_regmap_lookup_by_phandle_optional() will attach and prepare the
first clock in smi node, leading to additional prepare to the clock
which is not balanced with the prepare/unprepare pair in resume/suspend
callbacks.

If a power domain node requests an smi node and the smi node's first
clock is an mmsys clock, it will results in an unstabke suspend resume.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/soc/mediatek/mtk-pm-domains.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
index 536d8c64b2b4..a9ba71eee4bb 100644
--- a/drivers/soc/mediatek/mtk-pm-domains.c
+++ b/drivers/soc/mediatek/mtk-pm-domains.c
@@ -296,7 +296,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 {
 	const struct scpsys_domain_data *domain_data;
 	struct scpsys_domain *pd;
-	struct device_node *root_node = scpsys->dev->of_node;
+	struct device_node *root_node = scpsys->dev->of_node, *smi_node;
 	struct property *prop;
 	const char *clk_name;
 	int i, ret, num_clks;
@@ -352,9 +352,13 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 	if (IS_ERR(pd->infracfg))
 		return ERR_CAST(pd->infracfg);
 
-	pd->smi = syscon_regmap_lookup_by_phandle_optional(node, "mediatek,smi");
-	if (IS_ERR(pd->smi))
-		return ERR_CAST(pd->smi);
+	smi_node = of_parse_phandle(node, "mediatek,smi", 0);
+	if (smi_node) {
+		pd->smi = device_node_to_regmap(smi_node);
+		of_node_put(smi_node);
+		if (IS_ERR(pd->smi))
+			return ERR_CAST(pd->smi);
+	}
 
 	num_clks = of_clk_get_parent_count(node);
 	if (num_clks > 0) {
-- 
2.32.0.rc0.204.g9fa02ecfa5-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] arm64: dts: mt8183: remove syscon from smi_common node

[Hsin-Yi Wang]

We don't need to register smi_common as syscon. Also add required
property power-domains for this node.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 arch/arm64/boot/dts/mediatek/mt8183.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index c5e822b6b77a..e074c0d402ff 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -1263,13 +1263,14 @@ larb0: larb@14017000 {
 		};
 
 		smi_common: smi@14019000 {
-			compatible = "mediatek,mt8183-smi-common", "syscon";
+			compatible = "mediatek,mt8183-smi-common";
 			reg = <0 0x14019000 0 0x1000>;
 			clocks = <&mmsys CLK_MM_SMI_COMMON>,
 				 <&mmsys CLK_MM_SMI_COMMON>,
 				 <&mmsys CLK_MM_GALS_COMM0>,
 				 <&mmsys CLK_MM_GALS_COMM1>;
 			clock-names = "apb", "smi", "gals0", "gals1";
+			power-domains = <&spm MT8183_POWER_DOMAIN_DISP>;
 		};
 
 		imgsys: syscon@15020000 {
-- 
2.32.0.rc0.204.g9fa02ecfa5-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Chun-Jie Chen]

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);
>  }
>  
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] soc: mtk-pm-domains: do not register smi node as syscon

[Chun-Jie Chen]

On Mon, 2021-05-31 at 12:35 +0800, Hsin-Yi Wang wrote:
> Mediatek requires mmsys clocks to be unprepared during suspend,
> otherwise system has chances to hang.
> 
> syscon_regmap_lookup_by_phandle_optional() will attach and prepare
> the
> first clock in smi node, leading to additional prepare to the clock
> which is not balanced with the prepare/unprepare pair in
> resume/suspend
> callbacks.
> 
> If a power domain node requests an smi node and the smi node's first
> clock is an mmsys clock, it will results in an unstabke suspend
> resume.
> 
> 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 | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c
> b/drivers/soc/mediatek/mtk-pm-domains.c
> index 536d8c64b2b4..a9ba71eee4bb 100644
> --- a/drivers/soc/mediatek/mtk-pm-domains.c
> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> @@ -296,7 +296,7 @@ generic_pm_domain *scpsys_add_one_domain(struct
> scpsys *scpsys, struct device_no
>  {
>  	const struct scpsys_domain_data *domain_data;
>  	struct scpsys_domain *pd;
> -	struct device_node *root_node = scpsys->dev->of_node;
> +	struct device_node *root_node = scpsys->dev->of_node,
> *smi_node;
>  	struct property *prop;
>  	const char *clk_name;
>  	int i, ret, num_clks;
> @@ -352,9 +352,13 @@ generic_pm_domain *scpsys_add_one_domain(struct
> scpsys *scpsys, struct device_no
>  	if (IS_ERR(pd->infracfg))
>  		return ERR_CAST(pd->infracfg);
>  
> -	pd->smi = syscon_regmap_lookup_by_phandle_optional(node,
> "mediatek,smi");
> -	if (IS_ERR(pd->smi))
> -		return ERR_CAST(pd->smi);
> +	smi_node = of_parse_phandle(node, "mediatek,smi", 0);
> +	if (smi_node) {
> +		pd->smi = device_node_to_regmap(smi_node);
> +		of_node_put(smi_node);
> +		if (IS_ERR(pd->smi))
> +			return ERR_CAST(pd->smi);
> +	}
>  
>  	num_clks = of_clk_get_parent_count(node);
>  	if (num_clks > 0) {
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] soc: mtk-pm-domains: do not register smi node as syscon

[Enric Balletbo Serra]

Hi Hsin-Yi,

(again without html, sorry for the noise)

Thank you for the patch.

Missatge de Hsin-Yi Wang <hsinyi@chromium.org> del dia dl., 31 de maig
2021 a les 6:35:
>
> Mediatek requires mmsys clocks to be unprepared during suspend,
> otherwise system has chances to hang.
>
> syscon_regmap_lookup_by_phandle_optional() will attach and prepare the
> first clock in smi node, leading to additional prepare to the clock
> which is not balanced with the prepare/unprepare pair in resume/suspend
> callbacks.
>
> If a power domain node requests an smi node and the smi node's first
> clock is an mmsys clock, it will results in an unstabke suspend resume.

Typo s/unstabke/unstable/

I think it would be nice to have a Fixes tag for that patch. So can be
picked for the stable kernel more easily.

>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>

I have a nit below but the patch looks good to me. So

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

> ---
>  drivers/soc/mediatek/mtk-pm-domains.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
> index 536d8c64b2b4..a9ba71eee4bb 100644
> --- a/drivers/soc/mediatek/mtk-pm-domains.c
> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> @@ -296,7 +296,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
>  {
>         const struct scpsys_domain_data *domain_data;
>         struct scpsys_domain *pd;
> -       struct device_node *root_node = scpsys->dev->of_node;
> +       struct device_node *root_node = scpsys->dev->of_node, *smi_node;

nit: Personal preference, but I'd prefer to add the smi_node in a new
line, so it's really clear that the only thing you are doing here is
adding a new variable.

>         struct property *prop;
>         const char *clk_name;
>         int i, ret, num_clks;
> @@ -352,9 +352,13 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
>         if (IS_ERR(pd->infracfg))
>                 return ERR_CAST(pd->infracfg);
>
> -       pd->smi = syscon_regmap_lookup_by_phandle_optional(node, "mediatek,smi");
> -       if (IS_ERR(pd->smi))
> -               return ERR_CAST(pd->smi);
> +       smi_node = of_parse_phandle(node, "mediatek,smi", 0);
> +       if (smi_node) {
> +               pd->smi = device_node_to_regmap(smi_node);
> +               of_node_put(smi_node);
> +               if (IS_ERR(pd->smi))
> +                       return ERR_CAST(pd->smi);
> +       }
>
>         num_clks = of_clk_get_parent_count(node);
>         if (num_clks > 0) {
> --
> 2.32.0.rc0.204.g9fa02ecfa5-goog
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Enric Balletbo Serra]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] arm64: dts: mt8183: remove syscon from smi_common node

[Enric Balletbo Serra]

Hi Hsin-Yi,

(again without html, sorry for the noise)

Thank you for the patch.

Missatge de Hsin-Yi Wang <hsinyi@chromium.org> del dia dl., 31 de maig
2021 a les 6:36:
>
> We don't need to register smi_common as syscon. Also add required
> property power-domains for this node.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>

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

> ---
>  arch/arm64/boot/dts/mediatek/mt8183.dtsi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> index c5e822b6b77a..e074c0d402ff 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> @@ -1263,13 +1263,14 @@ larb0: larb@14017000 {
>                 };
>
>                 smi_common: smi@14019000 {
> -                       compatible = "mediatek,mt8183-smi-common", "syscon";
> +                       compatible = "mediatek,mt8183-smi-common";
>                         reg = <0 0x14019000 0 0x1000>;
>                         clocks = <&mmsys CLK_MM_SMI_COMMON>,
>                                  <&mmsys CLK_MM_SMI_COMMON>,
>                                  <&mmsys CLK_MM_GALS_COMM0>,
>                                  <&mmsys CLK_MM_GALS_COMM1>;
>                         clock-names = "apb", "smi", "gals0", "gals1";
> +                       power-domains = <&spm MT8183_POWER_DOMAIN_DISP>;
>                 };
>
>                 imgsys: syscon@15020000 {
> --
> 2.32.0.rc0.204.g9fa02ecfa5-goog
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel