diff mbox series

[04/16] soc: imx: gpcv2: split power up and power down sequence control

Message ID 20210429073050.21039-5-peng.fan@oss.nxp.com (mailing list archive)
State New, archived
Headers show
Series soc: imx: gpcv2: support i.MX8MM | expand

Commit Message

Peng Fan (OSS) April 29, 2021, 7:30 a.m. UTC 
From: Lucas Stach <l.stach@pengutronix.de>

The current mixed function to control both power up and power down
sequences is very hard to follow and already contains some sequence
errors like triggering the ADB400 handshake at the wrong time due to
this. Split the function into two, which results in slightly more
code, but is way easier to get right.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/soc/imx/gpcv2.c | 141 ++++++++++++++++++++++++----------------
 1 file changed, 86 insertions(+), 55 deletions(-)

Comments

Frieder Schrempf May 3, 2021, 2:13 p.m. UTC | #1 
On 29.04.21 09:30, Peng Fan (OSS) wrote:
> From: Lucas Stach <l.stach@pengutronix.de>
> 
> The current mixed function to control both power up and power down
> sequences is very hard to follow and already contains some sequence
> errors like triggering the ADB400 handshake at the wrong time due to
> this. Split the function into two, which results in slightly more
> code, but is way easier to get right.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/soc/imx/gpcv2.c | 141 ++++++++++++++++++++++++----------------
>   1 file changed, 86 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 1d90c7802972..7356e48ebdad 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -125,20 +125,19 @@ struct imx_pgc_domain_data {
>   	const struct regmap_access_table *reg_access_table;
>   };
>   
> -static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
> -				      bool on)
> +static inline struct imx_pgc_domain *
> +to_imx_pgc_domain(struct generic_pm_domain *genpd)
>   {
> -	struct imx_pgc_domain *domain = container_of(genpd,
> -						      struct imx_pgc_domain,
> -						      genpd);
> -	unsigned int offset = on ?
> -		GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ;
> -	const bool enable_power_control = !on;
> -	const bool has_regulator = !IS_ERR(domain->regulator);
> -	int i, ret = 0;
> -	u32 pxx_req;
> -
> -	if (has_regulator && on) {
> +	return container_of(genpd, struct imx_pgc_domain, genpd);
> +}
> +
> +static int imx_pgc_power_up(struct generic_pm_domain *genpd)
> +{
> +	struct imx_pgc_domain *domain = to_imx_pgc_domain(genpd);
> +	u32 reg_val;
> +	int ret;
> +
> +	if (!IS_ERR(domain->regulator)) {
>   		ret = regulator_enable(domain->regulator);
>   		if (ret) {
>   			dev_err(domain->dev, "failed to enable regulator\n");
> @@ -147,72 +146,104 @@ static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
>   	}
>   
>   	/* Enable reset clocks for all devices in the domain */
> -	clk_bulk_prepare_enable(domain->num_clks, domain->clks);
> +	ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);

As mentioned, the change above should be moved to patch 03/16.

>   	if (ret) {
>   		dev_err(domain->dev, "failed to enable reset clocks\n");
> +		goto out_regulator_disable;
> +	}
> +
> +	/* request the domain to power up */
> +	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
> +			   domain->bits.pxx, domain->bits.pxx);
> +	/*
> +	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
> +	 * for PUP_REQ/PDN_REQ bit to be cleared
> +	 */
> +	ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
> +				       reg_val, !(reg_val & domain->bits.pxx),
> +				       0, USEC_PER_MSEC);
> +	if (ret) {
> +		dev_err(domain->dev, "failed to command PGC\n");
> +		goto out_clk_disable;
> +	}
> +
> +	/* disable power control */
> +	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
> +			   GPC_PGC_CTRL_PCR, 0);

As we already touch this here, we could also simplify this a bit using 
regmap_clear_bits() instead.

> +
> +	/* request the ADB400 to power up */
> +	if (domain->bits.hsk)
> +		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
> +				   domain->bits.hsk, domain->bits.hsk);
> +
> +	/* Disable reset clocks for all devices in the domain */
> +	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +
> +	return 0;
> +
> +out_clk_disable:
> +	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +out_regulator_disable:
> +	if (!IS_ERR(domain->regulator))
>   		regulator_disable(domain->regulator);
> +
> +	return ret;
> +}
> +
> +static int imx_pgc_power_down(struct generic_pm_domain *genpd)
> +{
> +	struct imx_pgc_domain *domain = to_imx_pgc_domain(genpd);
> +	u32 reg_val;
> +	int ret;
> +
> +	/* Enable reset clocks for all devices in the domain */
> +	ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
> +	if (ret) {
> +		dev_err(domain->dev, "failed to enable reset clocks\n");
>   		return ret;
>   	}
>   
> -	if (enable_power_control)
> -		regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
> -				   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
> -
> +	/* request the ADB400 to power down */
>   	if (domain->bits.hsk)
>   		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
> -				   domain->bits.hsk, on ? domain->bits.hsk : 0);
> +				   domain->bits.hsk, 0);

See above, regmap_clear_bits()

>   
> -	regmap_update_bits(domain->regmap, offset,
> -			   domain->bits.pxx, domain->bits.pxx);
> +	/* enable power control */
> +	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
> +			   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
>   
> +	/* request the domain to power down */
> +	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
> +			   domain->bits.pxx, domain->bits.pxx);
>   	/*
>   	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
>   	 * for PUP_REQ/PDN_REQ bit to be cleared
>   	 */
> -	ret = regmap_read_poll_timeout(domain->regmap, offset, pxx_req,
> -				       !(pxx_req & domain->bits.pxx),
> +	ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
> +				       reg_val, !(reg_val & domain->bits.pxx),
>   				       0, USEC_PER_MSEC);
>   	if (ret) {
>   		dev_err(domain->dev, "failed to command PGC\n");
> -		/*
> -		 * If we were in a process of enabling a
> -		 * domain and failed we might as well disable
> -		 * the regulator we just enabled. And if it
> -		 * was the opposite situation and we failed to
> -		 * power down -- keep the regulator on
> -		 */
> -		on = !on;
> +		goto out_clk_disable;
>   	}
>   
> -	if (enable_power_control)
> -		regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
> -				   GPC_PGC_CTRL_PCR, 0);
> -
>   	/* Disable reset clocks for all devices in the domain */
>   	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
>   
> -	if (has_regulator && !on) {
> -		int err;
> -
> -		err = regulator_disable(domain->regulator);
> -		if (err)
> -			dev_err(domain->dev,
> -				"failed to disable regulator: %d\n", err);
> -		/* Preserve earlier error code */
> -		ret = ret ?: err;
> +	if (!IS_ERR(domain->regulator)) {
> +		ret = regulator_disable(domain->regulator);
> +		if (ret) {
> +			dev_err(domain->dev, "failed to disable regulator\n");
> +			return ret;
> +		}
>   	}
>   
> -	return ret;
> -}
> +	return 0;
>   
> -static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
> -{
> -	return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
> -}
> +out_clk_disable:
> +	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
>   
> -static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
> -{
> -	return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
> +	return ret;
>   }
>   
>   static const struct imx_pgc_domain imx7_pgc_domains[] = {
> @@ -590,8 +621,8 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
>   
>   		domain = pd_pdev->dev.platform_data;
>   		domain->regmap = regmap;
> -		domain->genpd.power_on  = imx_gpc_pu_pgc_sw_pup_req;
> -		domain->genpd.power_off = imx_gpc_pu_pgc_sw_pdn_req;
> +		domain->genpd.power_on  = imx_pgc_power_up;
> +		domain->genpd.power_off = imx_pgc_power_down;
>   
>   		pd_pdev->dev.parent = dev;
>   		pd_pdev->dev.of_node = np;
>
diff mbox series

Patch

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 1d90c7802972..7356e48ebdad 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -125,20 +125,19 @@  struct imx_pgc_domain_data {
 	const struct regmap_access_table *reg_access_table;
 };
 
-static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
-				      bool on)
+static inline struct imx_pgc_domain *
+to_imx_pgc_domain(struct generic_pm_domain *genpd)
 {
-	struct imx_pgc_domain *domain = container_of(genpd,
-						      struct imx_pgc_domain,
-						      genpd);
-	unsigned int offset = on ?
-		GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ;
-	const bool enable_power_control = !on;
-	const bool has_regulator = !IS_ERR(domain->regulator);
-	int i, ret = 0;
-	u32 pxx_req;
-
-	if (has_regulator && on) {
+	return container_of(genpd, struct imx_pgc_domain, genpd);
+}
+
+static int imx_pgc_power_up(struct generic_pm_domain *genpd)
+{
+	struct imx_pgc_domain *domain = to_imx_pgc_domain(genpd);
+	u32 reg_val;
+	int ret;
+
+	if (!IS_ERR(domain->regulator)) {
 		ret = regulator_enable(domain->regulator);
 		if (ret) {
 			dev_err(domain->dev, "failed to enable regulator\n");
@@ -147,72 +146,104 @@  static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
 	}
 
 	/* Enable reset clocks for all devices in the domain */
-	clk_bulk_prepare_enable(domain->num_clks, domain->clks);
+	ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
 	if (ret) {
 		dev_err(domain->dev, "failed to enable reset clocks\n");
+		goto out_regulator_disable;
+	}
+
+	/* request the domain to power up */
+	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
+			   domain->bits.pxx, domain->bits.pxx);
+	/*
+	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
+	 * for PUP_REQ/PDN_REQ bit to be cleared
+	 */
+	ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
+				       reg_val, !(reg_val & domain->bits.pxx),
+				       0, USEC_PER_MSEC);
+	if (ret) {
+		dev_err(domain->dev, "failed to command PGC\n");
+		goto out_clk_disable;
+	}
+
+	/* disable power control */
+	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
+			   GPC_PGC_CTRL_PCR, 0);
+
+	/* request the ADB400 to power up */
+	if (domain->bits.hsk)
+		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
+				   domain->bits.hsk, domain->bits.hsk);
+
+	/* Disable reset clocks for all devices in the domain */
+	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
+
+	return 0;
+
+out_clk_disable:
+	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
+out_regulator_disable:
+	if (!IS_ERR(domain->regulator))
 		regulator_disable(domain->regulator);
+
+	return ret;
+}
+
+static int imx_pgc_power_down(struct generic_pm_domain *genpd)
+{
+	struct imx_pgc_domain *domain = to_imx_pgc_domain(genpd);
+	u32 reg_val;
+	int ret;
+
+	/* Enable reset clocks for all devices in the domain */
+	ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
+	if (ret) {
+		dev_err(domain->dev, "failed to enable reset clocks\n");
 		return ret;
 	}
 
-	if (enable_power_control)
-		regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
-				   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
-
+	/* request the ADB400 to power down */
 	if (domain->bits.hsk)
 		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
-				   domain->bits.hsk, on ? domain->bits.hsk : 0);
+				   domain->bits.hsk, 0);
 
-	regmap_update_bits(domain->regmap, offset,
-			   domain->bits.pxx, domain->bits.pxx);
+	/* enable power control */
+	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
+			   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
 
+	/* request the domain to power down */
+	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
+			   domain->bits.pxx, domain->bits.pxx);
 	/*
 	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
 	 * for PUP_REQ/PDN_REQ bit to be cleared
 	 */
-	ret = regmap_read_poll_timeout(domain->regmap, offset, pxx_req,
-				       !(pxx_req & domain->bits.pxx),
+	ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
+				       reg_val, !(reg_val & domain->bits.pxx),
 				       0, USEC_PER_MSEC);
 	if (ret) {
 		dev_err(domain->dev, "failed to command PGC\n");
-		/*
-		 * If we were in a process of enabling a
-		 * domain and failed we might as well disable
-		 * the regulator we just enabled. And if it
-		 * was the opposite situation and we failed to
-		 * power down -- keep the regulator on
-		 */
-		on = !on;
+		goto out_clk_disable;
 	}
 
-	if (enable_power_control)
-		regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
-				   GPC_PGC_CTRL_PCR, 0);
-
 	/* Disable reset clocks for all devices in the domain */
 	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
 
-	if (has_regulator && !on) {
-		int err;
-
-		err = regulator_disable(domain->regulator);
-		if (err)
-			dev_err(domain->dev,
-				"failed to disable regulator: %d\n", err);
-		/* Preserve earlier error code */
-		ret = ret ?: err;
+	if (!IS_ERR(domain->regulator)) {
+		ret = regulator_disable(domain->regulator);
+		if (ret) {
+			dev_err(domain->dev, "failed to disable regulator\n");
+			return ret;
+		}
 	}
 
-	return ret;
-}
+	return 0;
 
-static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
-{
-	return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
-}
+out_clk_disable:
+	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
 
-static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
-{
-	return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
+	return ret;
 }
 
 static const struct imx_pgc_domain imx7_pgc_domains[] = {
@@ -590,8 +621,8 @@  static int imx_gpcv2_probe(struct platform_device *pdev)
 
 		domain = pd_pdev->dev.platform_data;
 		domain->regmap = regmap;
-		domain->genpd.power_on  = imx_gpc_pu_pgc_sw_pup_req;
-		domain->genpd.power_off = imx_gpc_pu_pgc_sw_pdn_req;
+		domain->genpd.power_on  = imx_pgc_power_up;
+		domain->genpd.power_off = imx_pgc_power_down;
 
 		pd_pdev->dev.parent = dev;
 		pd_pdev->dev.of_node = np;