[PATCH v1 0/1] ahci: imx: setup power saving methods

[PATCH v1 0/1] ahci: imx: setup power saving methods

[Richard Zhu]

In order to save power consumption amap.
* Disable sata phy internal pll reference clock when
sysetem enter into suspend mode, enable it after resume.
* Enter into test power down mode when there is no sata disk
detected on the port and 'AHCI_IMX_PHY_POWER_DOWN_MODE' is
enabled.

[v1] ahci: imx: setup power saving methods

drivers/ata/Kconfig    |    8 +++++
drivers/ata/ahci_imx.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 83 insertions(+), 3 deletions(-)

[v1] ahci: imx: setup power saving methods

[Richard Zhu]

From: Richard Zhu <r65037@freescale.com>

In order to save power consumption amap.
* Disable sata phy internal pll reference clock when
sysetem enter into suspend mode, enable it after resume.
* Enter into test power down mode when there is no sata disk
detected on the port and 'AHCI_IMX_PHY_POWER_DOWN_MODE' is
enabled.

Signed-off-by: Richard Zhu <r65037@freescale.com>
---
 drivers/ata/Kconfig    |    8 +++++
 drivers/ata/ahci_imx.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 4e73772..84b09f0 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -106,6 +106,14 @@ config AHCI_IMX
 
 	  If unsure, say N.
 
+config AHCI_IMX_PHY_POWER_DOWN_MODE
+	bool "Power saving mode when there is no SATA DEV detected on the port"
+	depends on AHCI_IMX
+	help
+	  This option enable the power down mode of the imx ahci when there is
+	  no sata device detected on the port, the sata port wouldn't be
+	  functional anymore except one system power down, and power up again.
+
 config SATA_FSL
 	tristate "Freescale 3.0Gbps SATA support"
 	depends on FSL_SOC
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 58debb0..c15dade 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -1,6 +1,6 @@
 /*
+ * copyright (c) 2013 Freescale Semiconductor, Inc.
  * Freescale IMX AHCI SATA platform driver
- * Copyright 2013 Freescale Semiconductor, Inc.
  *
  * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
  *
@@ -28,6 +28,10 @@
 #include "ahci.h"
 
 enum {
+	/* Port0 PHY Control */
+	PORT_PHY_CTL = 0x178,
+	/* PORT_PHY_CTL bits */
+	PORT_PHY_CTL_PDDQ_LOC = 0x100000,
 	HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
 };
 
@@ -36,12 +40,13 @@ struct imx_ahci_priv {
 	struct clk *sata_ref_clk;
 	struct clk *ahb_clk;
 	struct regmap *gpr;
+	bool no_device;
 };
 
 static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
 {
-	int ret = 0;
-	unsigned int reg_val;
+	int ret = 0, iterations = 200;
+	u32 reg_val, sstatus;
 	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
 
 	imxpriv->gpr =
@@ -105,6 +110,36 @@ static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
 	reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
 	writel(reg_val, mmio + HOST_TIMER1MS);
 
+	if (IS_ENABLED(CONFIG_AHCI_IMX_PHY_POWER_DOWN_MODE)) {
+		/*
+		 * In order to save power consumption, enter PDDQ mode
+		 * when there is no device detected on the port
+		 */
+		do {
+			sstatus = readl(mmio + 0x100 + PORT_SCR_STAT);
+			if ((sstatus & 0xF) == 0)
+				usleep_range(1000, 2000);
+			else
+				break;
+
+			if (iterations == 0) {
+				pr_info("No sata disk.\n");
+				reg_val = readl(mmio + PORT_PHY_CTL);
+				writel(reg_val | PORT_PHY_CTL_PDDQ_LOC,
+						mmio + PORT_PHY_CTL);
+				regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
+						IMX6Q_GPR13_SATA_MPLL_CLK_EN,
+						!IMX6Q_GPR13_SATA_MPLL_CLK_EN);
+				clk_disable_unprepare(imxpriv->sata_ref_clk);
+				imxpriv->no_device = 1;
+
+				return 0;
+			}
+		} while (iterations-- > 0);
+	} else {
+		imxpriv->no_device = 0;
+	}
+
 	return 0;
 }
 
@@ -117,9 +152,46 @@ static void imx6q_sata_exit(struct device *dev)
 	clk_disable_unprepare(imxpriv->sata_ref_clk);
 }
 
+static int imx_ahci_suspend(struct device *dev)
+{
+	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
+
+	if (!(imxpriv->no_device)) {
+		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
+				IMX6Q_GPR13_SATA_MPLL_CLK_EN,
+				!IMX6Q_GPR13_SATA_MPLL_CLK_EN);
+		clk_disable_unprepare(imxpriv->sata_ref_clk);
+	}
+
+	return 0;
+}
+
+static int imx_ahci_resume(struct device *dev)
+{
+	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
+	int ret;
+
+	if (!(imxpriv->no_device)) {
+		ret = clk_prepare_enable(imxpriv->sata_ref_clk);
+		if (ret < 0) {
+			dev_err(dev, "pre-enable sata_ref clock err:%d\n", ret);
+			return ret;
+		}
+
+		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
+				IMX6Q_GPR13_SATA_MPLL_CLK_EN,
+				IMX6Q_GPR13_SATA_MPLL_CLK_EN);
+		usleep_range(1000, 2000);
+	}
+
+	return 0;
+}
+
 static struct ahci_platform_data imx6q_sata_pdata = {
 	.init = imx6q_sata_init,
 	.exit = imx6q_sata_exit,
+	.suspend = imx_ahci_suspend,
+	.resume = imx_ahci_resume,
 };
 
 static const struct of_device_id imx_ahci_of_match[] = {
-- 
1.7.5.4

Re: [v1] ahci: imx: setup power saving methods

[Tejun Heo]

Hello,

On Fri, Sep 27, 2013 at 04:07:45PM +0800, Richard Zhu wrote:
> +config AHCI_IMX_PHY_POWER_DOWN_MODE
> +	bool "Power saving mode when there is no SATA DEV detected on the port"
> +	depends on AHCI_IMX
> +	help
> +	  This option enable the power down mode of the imx ahci when there is
> +	  no sata device detected on the port, the sata port wouldn't be
> +	  functional anymore except one system power down, and power up again.

The last part of the sentence is a bit difficult to understand.  Also,
I'm not sure whether making this a build option is a good idea.
Please read on.

> @@ -105,6 +110,36 @@ static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
>  	reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
>  	writel(reg_val, mmio + HOST_TIMER1MS);
>  
> +	if (IS_ENABLED(CONFIG_AHCI_IMX_PHY_POWER_DOWN_MODE)) {
> +		/*
> +		 * In order to save power consumption, enter PDDQ mode
> +		 * when there is no device detected on the port
> +		 */
> +		do {
> +			sstatus = readl(mmio + 0x100 + PORT_SCR_STAT);
> +			if ((sstatus & 0xF) == 0)
> +				usleep_range(1000, 2000);
> +			else
> +				break;
> +
> +			if (iterations == 0) {
> +				pr_info("No sata disk.\n");
> +				reg_val = readl(mmio + PORT_PHY_CTL);
> +				writel(reg_val | PORT_PHY_CTL_PDDQ_LOC,
> +						mmio + PORT_PHY_CTL);
> +				regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
> +						IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> +						!IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> +				clk_disable_unprepare(imxpriv->sata_ref_clk);
> +				imxpriv->no_device = 1;
> +
> +				return 0;
> +			}
> +		} while (iterations-- > 0);
> +	} else {
> +		imxpriv->no_device = 0;
> +	}

This looks like it really should be part of lpm framework.  Allow the
ahci_platform driver to override set_lpm() and turning off PHY if
MIN_POWER && !dev should work, right?

Thanks.

-- 
tejun

Re: [v1] ahci: imx: setup power saving methods

[Shawn Guo]

On Fri, Sep 27, 2013 at 09:14:41AM -0400, Tejun Heo wrote:
> Hello,
> 
> On Fri, Sep 27, 2013 at 04:07:45PM +0800, Richard Zhu wrote:
> > +config AHCI_IMX_PHY_POWER_DOWN_MODE
> > +	bool "Power saving mode when there is no SATA DEV detected on the port"
> > +	depends on AHCI_IMX
> > +	help
> > +	  This option enable the power down mode of the imx ahci when there is
> > +	  no sata device detected on the port, the sata port wouldn't be
> > +	  functional anymore except one system power down, and power up again.
> 
> The last part of the sentence is a bit difficult to understand.  Also,
> I'm not sure whether making this a build option is a good idea.

Agreed.  Why do not we support this unconditionally?

Shawn

Re: 回复：Re: [v1] ahci: imx: setup power saving methods

[Shawn Guo]

On Sun, Sep 29, 2013 at 07:40:57AM +0000, Zhu Richard-R65037 wrote:
> Hi Shawn:
> Thanks.
> The hot-plug can’t be supported, if we support this method unconditionally.

Ah, okay.  Then you should make this clear in the help text of the
Kconfig option.

Shawn

Re: 回复：Re: [v1] ahci: imx: setup power saving methods

[Tejun Heo]

On Sun, Sep 29, 2013 at 03:48:18PM +0800, Shawn Guo wrote:
> On Sun, Sep 29, 2013 at 07:40:57AM +0000, Zhu Richard-R65037 wrote:
> > Hi Shawn:
> > Thanks.
> > The hot-plug can’t be supported, if we support this method unconditionally.
> 
> Ah, okay.  Then you should make this clear in the help text of the
> Kconfig option.

Yeah but it's still weird to require kernel build to flip this
feature, isn't it?  Shouldn't it be a module param or something?

Thanks.

-- 
tejun

RE: 回复：Re: [v1] ahci: imx: setup power saving methods

[Zhu Richard-R65037]

Hi Tejun:
I'm back from holiday, Thanks for your review and suggestions.
The kernel build option would be replaced by the usage of the module parameters.

Best Regards
Richard Zhu


-----Original Message-----
From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo
Sent: Sunday, September 29, 2013 8:51 PM
To: Shawn Guo
Cc: Zhu Richard-R65037; Richard Zhu; linux-arm-kernel@lists.infradead.org; jgarzik@pobox.com; linux-ide@vger.kernel.org
Subject: Re: 回复：Re: [v1] ahci: imx: setup power saving methods

On Sun, Sep 29, 2013 at 03:48:18PM +0800, Shawn Guo wrote:
> On Sun, Sep 29, 2013 at 07:40:57AM +0000, Zhu Richard-R65037 wrote:
> > Hi Shawn:
> > Thanks.
> > The hot-plug can’t be supported, if we support this method unconditionally.
> 
> Ah, okay.  Then you should make this clear in the help text of the 
> Kconfig option.

Yeah but it's still weird to require kernel build to flip this feature, isn't it?  Shouldn't it be a module param or something?

Thanks.

--
tejun

Re: [v1] ahci: imx: setup power saving methods

[Shawn Guo]

On Fri, Sep 27, 2013 at 04:07:45PM +0800, Richard Zhu wrote:
> From: Richard Zhu <r65037@freescale.com>
> 
> In order to save power consumption amap.
> * Disable sata phy internal pll reference clock when
> sysetem enter into suspend mode, enable it after resume.
> * Enter into test power down mode when there is no sata disk
> detected on the port and 'AHCI_IMX_PHY_POWER_DOWN_MODE' is
> enabled.
> 
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  drivers/ata/Kconfig    |    8 +++++
>  drivers/ata/ahci_imx.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 83 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 4e73772..84b09f0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -106,6 +106,14 @@ config AHCI_IMX
>  
>  	  If unsure, say N.
>  
> +config AHCI_IMX_PHY_POWER_DOWN_MODE
> +	bool "Power saving mode when there is no SATA DEV detected on the port"
> +	depends on AHCI_IMX
> +	help
> +	  This option enable the power down mode of the imx ahci when there is
> +	  no sata device detected on the port, the sata port wouldn't be
> +	  functional anymore except one system power down, and power up again.
> +
>  config SATA_FSL
>  	tristate "Freescale 3.0Gbps SATA support"
>  	depends on FSL_SOC
> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
> index 58debb0..c15dade 100644
> --- a/drivers/ata/ahci_imx.c
> +++ b/drivers/ata/ahci_imx.c
> @@ -1,6 +1,6 @@
>  /*
> + * copyright (c) 2013 Freescale Semiconductor, Inc.
>   * Freescale IMX AHCI SATA platform driver
> - * Copyright 2013 Freescale Semiconductor, Inc.
>   *
>   * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
>   *
> @@ -28,6 +28,10 @@
>  #include "ahci.h"
>  
>  enum {
> +	/* Port0 PHY Control */
> +	PORT_PHY_CTL = 0x178,
> +	/* PORT_PHY_CTL bits */
> +	PORT_PHY_CTL_PDDQ_LOC = 0x100000,
>  	HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
>  };
>  
> @@ -36,12 +40,13 @@ struct imx_ahci_priv {
>  	struct clk *sata_ref_clk;
>  	struct clk *ahb_clk;
>  	struct regmap *gpr;
> +	bool no_device;
>  };
>  
>  static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
>  {
> -	int ret = 0;
> -	unsigned int reg_val;
> +	int ret = 0, iterations = 200;
> +	u32 reg_val, sstatus;

I think the following is less diff stat and easier to understand.

	int ret = 0;
+	int iterations = 200;
	unsigned int reg_val;
+	u32 sstatus;

>  	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
>  
>  	imxpriv->gpr =
> @@ -105,6 +110,36 @@ static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
>  	reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
>  	writel(reg_val, mmio + HOST_TIMER1MS);
>  
> +	if (IS_ENABLED(CONFIG_AHCI_IMX_PHY_POWER_DOWN_MODE)) {
> +		/*
> +		 * In order to save power consumption, enter PDDQ mode
> +		 * when there is no device detected on the port
> +		 */
> +		do {
> +			sstatus = readl(mmio + 0x100 + PORT_SCR_STAT);
> +			if ((sstatus & 0xF) == 0)
> +				usleep_range(1000, 2000);
> +			else
> +				break;
> +
> +			if (iterations == 0) {
> +				pr_info("No sata disk.\n");

dev_info

> +				reg_val = readl(mmio + PORT_PHY_CTL);
> +				writel(reg_val | PORT_PHY_CTL_PDDQ_LOC,
> +						mmio + PORT_PHY_CTL);
> +				regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
> +						IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> +						!IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> +				clk_disable_unprepare(imxpriv->sata_ref_clk);
> +				imxpriv->no_device = 1;
> +
> +				return 0;
> +			}
> +		} while (iterations-- > 0);
> +	} else {
> +		imxpriv->no_device = 0;
> +	}

This else block can just be saved, since imxpriv is allocated by
devm_kzalloc() which already clears the structure?

> +
>  	return 0;
>  }
>  
> @@ -117,9 +152,46 @@ static void imx6q_sata_exit(struct device *dev)
>  	clk_disable_unprepare(imxpriv->sata_ref_clk);
>  }
>  
> +static int imx_ahci_suspend(struct device *dev)
> +{
> +	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
> +
> +	if (!(imxpriv->no_device)) {

'if (!imxpriv->no_device)' is good enough.

> +		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
> +				IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> +				!IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> +		clk_disable_unprepare(imxpriv->sata_ref_clk);
> +	}
> +
> +	return 0;
> +}
> +
> +static int imx_ahci_resume(struct device *dev)
> +{
> +	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
> +	int ret;
> +
> +	if (!(imxpriv->no_device)) {

Ditto

> +		ret = clk_prepare_enable(imxpriv->sata_ref_clk);
> +		if (ret < 0) {
> +			dev_err(dev, "pre-enable sata_ref clock err:%d\n", ret);

Word 'pre-enable' is confusing, and 'enable' is fine.

Shawn

> +			return ret;
> +		}
> +
> +		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
> +				IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> +				IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> +		usleep_range(1000, 2000);
> +	}
> +
> +	return 0;
> +}
> +
>  static struct ahci_platform_data imx6q_sata_pdata = {
>  	.init = imx6q_sata_init,
>  	.exit = imx6q_sata_exit,
> +	.suspend = imx_ahci_suspend,
> +	.resume = imx_ahci_resume,
>  };
>  
>  static const struct of_device_id imx_ahci_of_match[] = {
> -- 
> 1.7.5.4
>