[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

[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

[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
> 

[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: [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: [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 at 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 at lists.infradead.org; jgarzik at pobox.com; linux-ide at 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