* [PATCH v1 0/1] ahci: imx: setup power saving methods
@ 2013-09-27 8:07 Richard Zhu
2013-09-27 8:07 ` [v1] " Richard Zhu
0 siblings, 1 reply; 8+ messages in thread
From: Richard Zhu @ 2013-09-27 8:07 UTC (permalink / raw)
To: shawn.guo; +Cc: tj, linux-ide, jgarzik, linux-arm-kernel
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(-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* [v1] ahci: imx: setup power saving methods
2013-09-27 8:07 [PATCH v1 0/1] ahci: imx: setup power saving methods Richard Zhu
@ 2013-09-27 8:07 ` Richard Zhu
2013-09-27 13:14 ` Tejun Heo
2013-09-29 7:33 ` Shawn Guo
0 siblings, 2 replies; 8+ messages in thread
From: Richard Zhu @ 2013-09-27 8:07 UTC (permalink / raw)
To: shawn.guo; +Cc: linux-arm-kernel, jgarzik, tj, linux-ide, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [v1] ahci: imx: setup power saving methods
2013-09-27 8:07 ` [v1] " Richard Zhu
@ 2013-09-27 13:14 ` Tejun Heo
2013-09-29 7:35 ` Shawn Guo
2013-09-29 7:33 ` Shawn Guo
1 sibling, 1 reply; 8+ messages in thread
From: Tejun Heo @ 2013-09-27 13:14 UTC (permalink / raw)
To: Richard Zhu; +Cc: shawn.guo, linux-arm-kernel, jgarzik, linux-ide, Richard Zhu
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [v1] ahci: imx: setup power saving methods
2013-09-27 8:07 ` [v1] " Richard Zhu
2013-09-27 13:14 ` Tejun Heo
@ 2013-09-29 7:33 ` Shawn Guo
1 sibling, 0 replies; 8+ messages in thread
From: Shawn Guo @ 2013-09-29 7:33 UTC (permalink / raw)
To: Richard Zhu; +Cc: linux-arm-kernel, jgarzik, tj, linux-ide, Richard Zhu
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [v1] ahci: imx: setup power saving methods
2013-09-27 13:14 ` Tejun Heo
@ 2013-09-29 7:35 ` Shawn Guo
[not found] ` <1vugbd691bjfibh5crdihlrh.1380440262216@email.android.com>
0 siblings, 1 reply; 8+ messages in thread
From: Shawn Guo @ 2013-09-29 7:35 UTC (permalink / raw)
To: Tejun Heo; +Cc: Richard Zhu, linux-arm-kernel, jgarzik, linux-ide, Richard Zhu
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: 回复：Re: [v1] ahci: imx: setup power saving methods
[not found] ` <1vugbd691bjfibh5crdihlrh.1380440262216@email.android.com>
@ 2013-09-29 7:48 ` Shawn Guo
2013-09-29 12:51 ` Tejun Heo
0 siblings, 1 reply; 8+ messages in thread
From: Shawn Guo @ 2013-09-29 7:48 UTC (permalink / raw)
To: Zhu Richard-R65037
Cc: Tejun Heo, Richard Zhu, linux-arm-kernel@lists.infradead.org,
jgarzik@pobox.com, linux-ide@vger.kernel.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: 回复：Re: [v1] ahci: imx: setup power saving methods
2013-09-29 7:48 ` 回复：Re: " Shawn Guo
@ 2013-09-29 12:51 ` Tejun Heo
2013-10-08 7:02 ` Zhu Richard-R65037
0 siblings, 1 reply; 8+ messages in thread
From: Tejun Heo @ 2013-09-29 12:51 UTC (permalink / raw)
To: Shawn Guo
Cc: Zhu Richard-R65037, Richard Zhu,
linux-arm-kernel@lists.infradead.org, jgarzik@pobox.com,
linux-ide@vger.kernel.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: 回复：Re: [v1] ahci: imx: setup power saving methods
2013-09-29 12:51 ` Tejun Heo
@ 2013-10-08 7:02 ` Zhu Richard-R65037
0 siblings, 0 replies; 8+ messages in thread
From: Zhu Richard-R65037 @ 2013-10-08 7:02 UTC (permalink / raw)
To: Tejun Heo, Shawn Guo
Cc: Richard Zhu, linux-arm-kernel@lists.infradead.org,
jgarzik@pobox.com, linux-ide@vger.kernel.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-10-08 7:02 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-27 8:07 [PATCH v1 0/1] ahci: imx: setup power saving methods Richard Zhu
2013-09-27 8:07 ` [v1] " Richard Zhu
2013-09-27 13:14 ` Tejun Heo
2013-09-29 7:35 ` Shawn Guo
[not found] ` <1vugbd691bjfibh5crdihlrh.1380440262216@email.android.com>
2013-09-29 7:48 ` 回复：Re: " Shawn Guo
2013-09-29 12:51 ` Tejun Heo
2013-10-08 7:02 ` Zhu Richard-R65037
2013-09-29 7:33 ` Shawn Guo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).