[PATCH 0/8] mmc: mmci: Improved PM and SDIO support

[PATCH 0/8] mmc: mmci: Improved PM and SDIO support

[Ulf Hansson]

This patch series replaces previous version of the series below:
	- Improved PM support, cleanup and bugfixes
	- Improvements and bugfixes for SDIO

Patches that were accepted has been removed from the series and the
ones left has been rebased.

Patch 1 to 5:
	- PM patches.
Patch 6 - 8:
	- SDIO patches.

Per Forlin (1):
  mmc: mmci: Add constraints on alignment for SDIO

Ulf Hansson (7):
  mmc: mmci: Cache MMCICLOCK and MMCIPOWER register
  mmc: mmci: Decrease current consumption in suspend
  mmc: mmci: Implement PM runtime callbacks to save power
  mmc: mmci: Use ios_handler to save power
  mmc: mmci: Support MMC_PM_KEEP_POWER
  mmc: mmci: Fix incorrect handling of HW flow control for SDIO
  mmc: mmci: Support any block sizes for ux500v2 variant

 drivers/mmc/host/mmci.c |  251 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/mmci.h |   10 ++-
 2 files changed, 222 insertions(+), 39 deletions(-)

-- 
1.7.5.4

[PATCH 1/8] mmc: mmci: Cache MMCICLOCK and MMCIPOWER register

[Ulf Hansson]

Instead of reading a register value everytime we need to
apply a new value for it, maintain a cached copy for it.
This also means we are able to skip writes that are not
needed.

Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |   41 +++++++++++++++++++++++++++++------------
 drivers/mmc/host/mmci.h |    3 ++-
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 9e544cf..0de2f3d 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -121,6 +121,28 @@ static struct variant_data variant_ux500v2 = {
 /*
  * This must be called with host->lock held
  */
+static void mmci_write_clkreg(struct mmci_host *host, u32 clk)
+{
+	if (host->clk_reg != clk) {
+		host->clk_reg = clk;
+		writel(clk, host->base + MMCICLOCK);
+	}
+}
+
+/*
+ * This must be called with host->lock held
+ */
+static void mmci_write_pwrreg(struct mmci_host *host, u32 pwr)
+{
+	if (host->pwr_reg != pwr) {
+		host->pwr_reg = pwr;
+		writel(pwr, host->base + MMCIPOWER);
+	}
+}
+
+/*
+ * This must be called with host->lock held
+ */
 static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
 {
 	struct variant_data *variant = host->variant;
@@ -165,7 +187,7 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
 	if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_8)
 		clk |= MCI_ST_8BIT_BUS;
 
-	writel(clk, host->base + MMCICLOCK);
+	mmci_write_clkreg(host, clk);
 }
 
 static void
@@ -843,14 +865,13 @@ static int mmci_pio_write(struct mmci_host *host, char *buffer, unsigned int rem
 		 */
 		if (variant->sdio &&
 		    mmc_card_sdio(host->mmc->card)) {
+			u32 clk;
 			if (count < 8)
-				writel(readl(host->base + MMCICLOCK) &
-					~variant->clkreg_enable,
-					host->base + MMCICLOCK);
+				clk = host->clk_reg & ~variant->clkreg_enable;
 			else
-				writel(readl(host->base + MMCICLOCK) |
-					variant->clkreg_enable,
-					host->base + MMCICLOCK);
+				clk = host->clk_reg | variant->clkreg_enable;
+
+			mmci_write_clkreg(host, clk);
 		}
 
 		/*
@@ -1111,11 +1132,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	spin_lock_irqsave(&host->lock, flags);
 
 	mmci_set_clkreg(host, ios->clock);
-
-	if (host->pwr != pwr) {
-		host->pwr = pwr;
-		writel(pwr, host->base + MMCIPOWER);
-	}
+	mmci_write_pwrreg(host, pwr);
 
 	spin_unlock_irqrestore(&host->lock, flags);
 
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 89eb2e3..d437ccf 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -179,7 +179,8 @@ struct mmci_host {
 
 	unsigned int		mclk;
 	unsigned int		cclk;
-	u32			pwr;
+	u32			pwr_reg;
+	u32			clk_reg;
 	struct mmci_platform_data *plat;
 	struct variant_data	*variant;
 
-- 
1.7.5.4

[PATCH 2/8] mmc: mmci: Decrease current consumption in suspend

[Ulf Hansson]

To decrease current consumption in suspend state the
VCORE regulator, the MCLK and PCLK for the ARM PL18x
are now disabled.

When resuming the resourses are re-enabled and
register values for MMCICLOCK, MMCIPOWER and MMCIMASK0
are restored.

Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |   64 ++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 0de2f3d..cd7e144 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1497,6 +1497,60 @@ static int __devexit mmci_remove(struct amba_device *dev)
 }
 
 #ifdef CONFIG_SUSPEND
+static int mmci_save(struct amba_device *dev)
+{
+	struct mmc_host *mmc = amba_get_drvdata(dev);
+	unsigned long flags;
+
+	if (mmc) {
+		struct mmci_host *host = mmc_priv(mmc);
+
+		spin_lock_irqsave(&host->lock, flags);
+
+		/*
+		 * Make sure we do not get any interrupts when we disabled the
+		 * clock and the regulator and as well make sure to clear the
+		 * registers for clock and power.
+		 */
+		writel(0, host->base + MMCIMASK0);
+		writel(0, host->base + MMCIPOWER);
+		writel(0, host->base + MMCICLOCK);
+
+		spin_unlock_irqrestore(&host->lock, flags);
+
+		clk_unprepare(host->clk);
+		clk_disable(host->clk);
+		amba_vcore_disable(dev);
+	}
+
+	return 0;
+}
+
+static int mmci_restore(struct amba_device *dev)
+{
+	struct mmc_host *mmc = amba_get_drvdata(dev);
+	unsigned long flags;
+
+	if (mmc) {
+		struct mmci_host *host = mmc_priv(mmc);
+
+		amba_vcore_enable(dev);
+		clk_prepare(host->clk);
+		clk_enable(host->clk);
+
+		spin_lock_irqsave(&host->lock, flags);
+
+		/* Restore registers and re-enable interrupts. */
+		writel(host->clk_reg, host->base + MMCICLOCK);
+		writel(host->pwr_reg, host->base + MMCIPOWER);
+		writel(MCI_IRQENABLE, host->base + MMCIMASK0);
+
+		spin_unlock_irqrestore(&host->lock, flags);
+	}
+
+	return 0;
+}
+
 static int mmci_suspend(struct device *dev)
 {
 	struct amba_device *adev = to_amba_device(dev);
@@ -1504,12 +1558,11 @@ static int mmci_suspend(struct device *dev)
 	int ret = 0;
 
 	if (mmc) {
-		struct mmci_host *host = mmc_priv(mmc);
-
 		ret = mmc_suspend_host(mmc);
 		if (ret == 0) {
 			pm_runtime_get_sync(dev);
-			writel(0, host->base + MMCIMASK0);
+			mmci_save(adev);
+			amba_pclk_disable(adev);
 		}
 	}
 
@@ -1523,9 +1576,8 @@ static int mmci_resume(struct device *dev)
 	int ret = 0;
 
 	if (mmc) {
-		struct mmci_host *host = mmc_priv(mmc);
-
-		writel(MCI_IRQENABLE, host->base + MMCIMASK0);
+		amba_pclk_enable(adev);
+		mmci_restore(adev);
 		pm_runtime_put(dev);
 
 		ret = mmc_resume_host(mmc);
-- 
1.7.5.4

[PATCH 3/8] mmc: mmci: Implement PM runtime callbacks to save power

[Ulf Hansson]

In the runtime_suspend callback we make use of mmci_save
to disable the VCORE regulator and the MCLK to decrease
current consumption. At runtime resume, we use mmci_restore
to re-enable the resourses again.

>From now on this will mean that especially the mmci_restore
function must be fast to execute since otherwise request
latency will be introduced.

For the ARM Primcell PL180, the MMCIPOWER register is used
to control an external power supply to the card. Thus we
need to prevent the runtime callbacks from doing save and
restore, otherwise the power to card will be cut. This is
done by adding a new flag to the variant data.

Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 42 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index cd7e144..a7c8f8f 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -55,6 +55,7 @@ static unsigned int fmax = 515633;
  * @blksz_datactrl16: true if Block size is at b16..b30 position in datactrl register
  * @pwrreg_powerup: power up value for MMCIPOWER register
  * @signal_direction: input/out direction of bus signals can be indicated
+ * @pwrreg_ctrl_power: bits in MMCIPOWER register controls ext. power supply
  */
 struct variant_data {
 	unsigned int		clkreg;
@@ -67,6 +68,7 @@ struct variant_data {
 	bool			blksz_datactrl16;
 	u32			pwrreg_powerup;
 	bool			signal_direction;
+	bool			pwrreg_ctrl_power;
 };
 
 static struct variant_data variant_arm = {
@@ -74,6 +76,7 @@ static struct variant_data variant_arm = {
 	.fifohalfsize		= 8 * 4,
 	.datalength_bits	= 16,
 	.pwrreg_powerup		= MCI_PWR_UP,
+	.pwrreg_ctrl_power	= true,
 };
 
 static struct variant_data variant_arm_extended_fifo = {
@@ -81,6 +84,7 @@ static struct variant_data variant_arm_extended_fifo = {
 	.fifohalfsize		= 64 * 4,
 	.datalength_bits	= 16,
 	.pwrreg_powerup		= MCI_PWR_UP,
+	.pwrreg_ctrl_power	= true,
 };
 
 static struct variant_data variant_u300 = {
@@ -1496,7 +1500,7 @@ static int __devexit mmci_remove(struct amba_device *dev)
 	return 0;
 }
 
-#ifdef CONFIG_SUSPEND
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_PM_RUNTIME)
 static int mmci_save(struct amba_device *dev)
 {
 	struct mmc_host *mmc = amba_get_drvdata(dev);
@@ -1550,7 +1554,9 @@ static int mmci_restore(struct amba_device *dev)
 
 	return 0;
 }
+#endif
 
+#ifdef CONFIG_SUSPEND
 static int mmci_suspend(struct device *dev)
 {
 	struct amba_device *adev = to_amba_device(dev);
@@ -1587,8 +1593,43 @@ static int mmci_resume(struct device *dev)
 }
 #endif
 
+#ifdef CONFIG_PM_RUNTIME
+static int mmci_runtime_suspend(struct device *dev)
+{
+	struct amba_device *adev = to_amba_device(dev);
+	struct mmc_host *mmc = amba_get_drvdata(adev);
+	int ret = 0;
+
+	if (mmc) {
+		struct mmci_host *host = mmc_priv(mmc);
+		struct variant_data *variant = host->variant;
+		if (!variant->pwrreg_ctrl_power)
+			ret = mmci_save(adev);
+	}
+
+	return ret;
+}
+
+static int mmci_runtime_resume(struct device *dev)
+{
+	struct amba_device *adev = to_amba_device(dev);
+	struct mmc_host *mmc = amba_get_drvdata(adev);
+	int ret = 0;
+
+	if (mmc) {
+		struct mmci_host *host = mmc_priv(mmc);
+		struct variant_data *variant = host->variant;
+		if (!variant->pwrreg_ctrl_power)
+			ret = mmci_restore(adev);
+	}
+
+	return ret;
+}
+#endif
+
 static const struct dev_pm_ops mmci_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(mmci_suspend, mmci_resume)
+	SET_RUNTIME_PM_OPS(mmci_runtime_suspend, mmci_runtime_resume, NULL)
 };
 
 static struct amba_id mmci_ids[] = {
-- 
1.7.5.4

[PATCH 4/8] mmc: mmci: Use ios_handler to save power

[Ulf Hansson]

To disable a levelshifter when we are in an idle state will
decrease current consumption. We make use of the ios_handler
at runtime suspend and at regular suspend to accomplish this.

Of course depending on the used levelshifter the decrease of
current differs. For ST6G3244ME the value is up to ~200 uA.

Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index a7c8f8f..76ce2cd 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1505,10 +1505,22 @@ static int mmci_save(struct amba_device *dev)
 {
 	struct mmc_host *mmc = amba_get_drvdata(dev);
 	unsigned long flags;
+	struct mmc_ios ios;
+	int ret = 0;
 
 	if (mmc) {
 		struct mmci_host *host = mmc_priv(mmc);
 
+		/* Let the ios_handler act on a POWER_OFF to save power. */
+		if (host->plat->ios_handler) {
+			memcpy(&ios, &mmc->ios, sizeof(struct mmc_ios));
+			ios.power_mode = MMC_POWER_OFF;
+			ret = host->plat->ios_handler(mmc_dev(mmc),
+						      &ios);
+			if (ret)
+				return ret;
+		}
+
 		spin_lock_irqsave(&host->lock, flags);
 
 		/*
@@ -1527,7 +1539,7 @@ static int mmci_save(struct amba_device *dev)
 		amba_vcore_disable(dev);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int mmci_restore(struct amba_device *dev)
@@ -1550,6 +1562,11 @@ static int mmci_restore(struct amba_device *dev)
 		writel(MCI_IRQENABLE, host->base + MMCIMASK0);
 
 		spin_unlock_irqrestore(&host->lock, flags);
+
+		/* Restore settings done by the ios_handler. */
+		if (host->plat->ios_handler)
+			host->plat->ios_handler(mmc_dev(mmc),
+						&mmc->ios);
 	}
 
 	return 0;
-- 
1.7.5.4

[PATCH 4/8] mmc: mmci: Use ios_handler to save power

[Vitaly Wool]

Hi Ulf,

On Tue, Jan 17, 2012 at 3:34 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> To disable a levelshifter when we are in an idle state will
> decrease current consumption. We make use of the ios_handler
> at runtime suspend and at regular suspend to accomplish this.
>
> Of course depending on the used levelshifter the decrease of
> current differs. For ST6G3244ME the value is up to ~200 uA.
>
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ?drivers/mmc/host/mmci.c | ? 19 ++++++++++++++++++-
> ?1 files changed, 18 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index a7c8f8f..76ce2cd 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1505,10 +1505,22 @@ static int mmci_save(struct amba_device *dev)
> ?{
> ? ? ? ?struct mmc_host *mmc = amba_get_drvdata(dev);
> ? ? ? ?unsigned long flags;
> + ? ? ? struct mmc_ios ios;
> + ? ? ? int ret = 0;
>
> ? ? ? ?if (mmc) {
> ? ? ? ? ? ? ? ?struct mmci_host *host = mmc_priv(mmc);
>
> + ? ? ? ? ? ? ? /* Let the ios_handler act on a POWER_OFF to save power. */
> + ? ? ? ? ? ? ? if (host->plat->ios_handler) {
> + ? ? ? ? ? ? ? ? ? ? ? memcpy(&ios, &mmc->ios, sizeof(struct mmc_ios));
> + ? ? ? ? ? ? ? ? ? ? ? ios.power_mode = MMC_POWER_OFF;
> + ? ? ? ? ? ? ? ? ? ? ? ret = host->plat->ios_handler(mmc_dev(mmc),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &ios);
> + ? ? ? ? ? ? ? ? ? ? ? if (ret)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return ret;
> + ? ? ? ? ? ? ? }
> +
> ? ? ? ? ? ? ? ?spin_lock_irqsave(&host->lock, flags);
>
> ? ? ? ? ? ? ? ?/*
> @@ -1527,7 +1539,7 @@ static int mmci_save(struct amba_device *dev)
> ? ? ? ? ? ? ? ?amba_vcore_disable(dev);
> ? ? ? ?}
>
> - ? ? ? return 0;
> + ? ? ? return ret;
> ?}
>
> ?static int mmci_restore(struct amba_device *dev)
> @@ -1550,6 +1562,11 @@ static int mmci_restore(struct amba_device *dev)
> ? ? ? ? ? ? ? ?writel(MCI_IRQENABLE, host->base + MMCIMASK0);
>
> ? ? ? ? ? ? ? ?spin_unlock_irqrestore(&host->lock, flags);
> +
> + ? ? ? ? ? ? ? /* Restore settings done by the ios_handler. */
> + ? ? ? ? ? ? ? if (host->plat->ios_handler)
> + ? ? ? ? ? ? ? ? ? ? ? host->plat->ios_handler(mmc_dev(mmc),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &mmc->ios);
> ? ? ? ?}
>
> ? ? ? ?return 0;

this patch is a disaster because it cuts off the chip's power on
suspend regardless of whether MMC_PM_KEEP_POWER flag has been set or
not. Simply put: it breaks everything This patch therefore gets a
strongest *NACK* from me.

Thanks,
   Vitaly

[PATCH 4/8] mmc: mmci: Use ios_handler to save power

[Russell King - ARM Linux]

On Wed, Apr 18, 2012 at 01:57:27PM +0200, Vitaly Wool wrote:
> Hi Ulf,
> 
> On Tue, Jan 17, 2012 at 3:34 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> > To disable a levelshifter when we are in an idle state will
> > decrease current consumption. We make use of the ios_handler
> > at runtime suspend and at regular suspend to accomplish this.
> >
> > Of course depending on the used levelshifter the decrease of
> > current differs. For ST6G3244ME the value is up to ~200 uA.
> >
> > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > ?drivers/mmc/host/mmci.c | ? 19 ++++++++++++++++++-
> > ?1 files changed, 18 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> > index a7c8f8f..76ce2cd 100644
> > --- a/drivers/mmc/host/mmci.c
> > +++ b/drivers/mmc/host/mmci.c
> > @@ -1505,10 +1505,22 @@ static int mmci_save(struct amba_device *dev)
> > ?{
> > ? ? ? ?struct mmc_host *mmc = amba_get_drvdata(dev);
> > ? ? ? ?unsigned long flags;
> > + ? ? ? struct mmc_ios ios;
> > + ? ? ? int ret = 0;
> >
> > ? ? ? ?if (mmc) {
> > ? ? ? ? ? ? ? ?struct mmci_host *host = mmc_priv(mmc);
> >
> > + ? ? ? ? ? ? ? /* Let the ios_handler act on a POWER_OFF to save power. */
> > + ? ? ? ? ? ? ? if (host->plat->ios_handler) {
> > + ? ? ? ? ? ? ? ? ? ? ? memcpy(&ios, &mmc->ios, sizeof(struct mmc_ios));
> > + ? ? ? ? ? ? ? ? ? ? ? ios.power_mode = MMC_POWER_OFF;
> > + ? ? ? ? ? ? ? ? ? ? ? ret = host->plat->ios_handler(mmc_dev(mmc),
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &ios);
> > + ? ? ? ? ? ? ? ? ? ? ? if (ret)
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return ret;
> > + ? ? ? ? ? ? ? }
> > +
> > ? ? ? ? ? ? ? ?spin_lock_irqsave(&host->lock, flags);
> >
> > ? ? ? ? ? ? ? ?/*
> > @@ -1527,7 +1539,7 @@ static int mmci_save(struct amba_device *dev)
> > ? ? ? ? ? ? ? ?amba_vcore_disable(dev);
> > ? ? ? ?}
> >
> > - ? ? ? return 0;
> > + ? ? ? return ret;
> > ?}
> >
> > ?static int mmci_restore(struct amba_device *dev)
> > @@ -1550,6 +1562,11 @@ static int mmci_restore(struct amba_device *dev)
> > ? ? ? ? ? ? ? ?writel(MCI_IRQENABLE, host->base + MMCIMASK0);
> >
> > ? ? ? ? ? ? ? ?spin_unlock_irqrestore(&host->lock, flags);
> > +
> > + ? ? ? ? ? ? ? /* Restore settings done by the ios_handler. */
> > + ? ? ? ? ? ? ? if (host->plat->ios_handler)
> > + ? ? ? ? ? ? ? ? ? ? ? host->plat->ios_handler(mmc_dev(mmc),
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &mmc->ios);
> > ? ? ? ?}
> >
> > ? ? ? ?return 0;
> 
> this patch is a disaster because it cuts off the chip's power on
> suspend regardless of whether MMC_PM_KEEP_POWER flag has been set or
> not. Simply put: it breaks everything This patch therefore gets a
> strongest *NACK* from me.

Thank you for backing up what I've already said about this patch (which
is why I stopped applying Ulf's MMC patches at this patch.)

I've also been concerned with the mere saving and restoring of the
clock and power registers in this patch - something which the ARM version
of the primecell does not actually support.

I've said that before...

[PATCH 4/8] mmc: mmci: Use ios_handler to save power

[Ulf Hansson]

Hi Russell,

Sorry for a late reply.

On 04/18/2012 03:45 PM, Russell King - ARM Linux wrote:
> On Wed, Apr 18, 2012 at 01:57:27PM +0200, Vitaly Wool wrote:
>> Hi Ulf,
>>
>> On Tue, Jan 17, 2012 at 3:34 PM, Ulf Hansson<ulf.hansson@stericsson.com>  wrote:
>>> To disable a levelshifter when we are in an idle state will
>>> decrease current consumption. We make use of the ios_handler
>>> at runtime suspend and at regular suspend to accomplish this.
>>>
>>> Of course depending on the used levelshifter the decrease of
>>> current differs. For ST6G3244ME the value is up to ~200 uA.
>>>
>>> Tested-by: Linus Walleij<linus.walleij@linaro.org>
>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>> Signed-off-by: Linus Walleij<linus.walleij@linaro.org>
>>> ---
>>>   drivers/mmc/host/mmci.c |   19 ++++++++++++++++++-
>>>   1 files changed, 18 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>> index a7c8f8f..76ce2cd 100644
>>> --- a/drivers/mmc/host/mmci.c
>>> +++ b/drivers/mmc/host/mmci.c
>>> @@ -1505,10 +1505,22 @@ static int mmci_save(struct amba_device *dev)
>>>   {
>>>         struct mmc_host *mmc = amba_get_drvdata(dev);
>>>         unsigned long flags;
>>> +       struct mmc_ios ios;
>>> +       int ret = 0;
>>>
>>>         if (mmc) {
>>>                 struct mmci_host *host = mmc_priv(mmc);
>>>
>>> +               /* Let the ios_handler act on a POWER_OFF to save power. */
>>> +               if (host->plat->ios_handler) {
>>> +                       memcpy(&ios,&mmc->ios, sizeof(struct mmc_ios));
>>> +                       ios.power_mode = MMC_POWER_OFF;
>>> +                       ret = host->plat->ios_handler(mmc_dev(mmc),
>>> +&ios);
>>> +                       if (ret)
>>> +                               return ret;
>>> +               }
>>> +
>>>                 spin_lock_irqsave(&host->lock, flags);
>>>
>>>                 /*
>>> @@ -1527,7 +1539,7 @@ static int mmci_save(struct amba_device *dev)
>>>                 amba_vcore_disable(dev);
>>>         }
>>>
>>> -       return 0;
>>> +       return ret;
>>>   }
>>>
>>>   static int mmci_restore(struct amba_device *dev)
>>> @@ -1550,6 +1562,11 @@ static int mmci_restore(struct amba_device *dev)
>>>                 writel(MCI_IRQENABLE, host->base + MMCIMASK0);
>>>
>>>                 spin_unlock_irqrestore(&host->lock, flags);
>>> +
>>> +               /* Restore settings done by the ios_handler. */
>>> +               if (host->plat->ios_handler)
>>> +                       host->plat->ios_handler(mmc_dev(mmc),
>>> +&mmc->ios);
>>>         }
>>>
>>>         return 0;
>>
>> this patch is a disaster because it cuts off the chip's power on
>> suspend regardless of whether MMC_PM_KEEP_POWER flag has been set or
>> not. Simply put: it breaks everything This patch therefore gets a
>> strongest *NACK* from me.

Agree, the ios_handler should not be called like this. In our internal 
code base the ios_handler were only handling the levelshifter, which is 
safe to disable in the runtim_suspend sequence. But of course, and 
ios_handler may control power to the card as well and thus shall not be 
called like this patch does.

>
> Thank you for backing up what I've already said about this patch (which
> is why I stopped applying Ulf's MMC patches at this patch.)
>
> I've also been concerned with the mere saving and restoring of the
> clock and power registers in this patch - something which the ARM version
> of the primecell does not actually support.

The ios_handler is not used for the ARM version so it should not be 
causing any issue I think. But, still, according to input from Vitaly 
above, this patch is not OK.

I will rework this patch. Thanks for your input.

>
> I've said that before...
>

Kind regards
Ulf Hansson

[PATCH 5/8] mmc: mmci: Support MMC_PM_KEEP_POWER

[Ulf Hansson]

Add MMC_PM_KEEP_POWER to pm_caps so SDIO clients are able
to use this option to prevent power off in suspend.

Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 76ce2cd..3defe69 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1319,6 +1319,9 @@ static int __devinit mmci_probe(struct amba_device *dev,
 		mmc->ocr_avail = plat->ocr_mask;
 	mmc->caps = plat->capabilities;
 
+	/* We support these PM capabilities. */
+	mmc->pm_caps = MMC_PM_KEEP_POWER;
+
 	/*
 	 * We can do SGIO
 	 */
-- 
1.7.5.4

[PATCH 6/8] mmc: mmci: Fix incorrect handling of HW flow control for SDIO

[Ulf Hansson]

For data writes smaller than 8 bytes (only SDIO case), HW flow
control was disabled but never re-enabled again. This meant that
a following large read request could randomly give buffer overrun
errors.

This patch is based upon a patch from Stefan Nilsson.

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
Signed-off-by: Fredrik Soderstedt <fredrik.soderstedt@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |   38 ++++++++++++++++++++------------------
 1 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 3defe69..e7b4500 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -638,10 +638,28 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 	if (data->flags & MMC_DATA_READ)
 		datactrl |= MCI_DPSM_DIRECTION;
 
-	/* The ST Micro variants has a special bit to enable SDIO */
 	if (variant->sdio && host->mmc->card)
-		if (mmc_card_sdio(host->mmc->card))
+		if (mmc_card_sdio(host->mmc->card)) {
+
+			/*
+			 * The ST Micro variant for SDIO write transfer sizes
+			 * less then 8 bytes must have clock H/W flow control
+			 * disabled.
+			 */
+			u32 clk;
+			if ((host->size < 8) && (data->flags & MMC_DATA_WRITE))
+				clk = host->clk_reg & ~variant->clkreg_enable;
+			else
+				clk = host->clk_reg | variant->clkreg_enable;
+
+			mmci_write_clkreg(host, clk);
+
+			/*
+			 * The ST Micro variants has a special bit
+			 * to enable SDIO.
+			 */
 			datactrl |= MCI_ST_DPSM_SDIOEN;
+		}
 
 	/*
 	 * Attempt to use DMA operation mode, if this
@@ -863,22 +881,6 @@ static int mmci_pio_write(struct mmci_host *host, char *buffer, unsigned int rem
 		count = min(remain, maxcnt);
 
 		/*
-		 * The ST Micro variant for SDIO transfer sizes
-		 * less then 8 bytes should have clock H/W flow
-		 * control disabled.
-		 */
-		if (variant->sdio &&
-		    mmc_card_sdio(host->mmc->card)) {
-			u32 clk;
-			if (count < 8)
-				clk = host->clk_reg & ~variant->clkreg_enable;
-			else
-				clk = host->clk_reg | variant->clkreg_enable;
-
-			mmci_write_clkreg(host, clk);
-		}
-
-		/*
 		 * SDIO especially may want to send something that is
 		 * not divisible by 4 (as opposed to card sectors
 		 * etc), and the FIFO only accept full 32-bit writes.
-- 
1.7.5.4

[PATCH 7/8] mmc: mmci: Add constraints on alignment for SDIO

[Ulf Hansson]

From: Per Forlin <per.forlin@stericsson.com>

Buffers must be 4-byte aligned due to restrictions that the
PL18x FIFO accesses must be done in a 4-byte aligned manner.

Moreover DPSM_DMAREQCTL must be enabled for SDIO to support
writes for non 32-byte aligned sg element lengths. In PIO
mode any buffer length can be handled as long as the buffer
address is 4-byte aligned.

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Per Forlin <per.forlin@stericsson.com>
Signed-off-by: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
 drivers/mmc/host/mmci.h |    7 ++++++
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index e7b4500..94c04c3 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -45,6 +45,7 @@ static unsigned int fmax = 515633;
  * struct variant_data - MMCI variant-specific quirks
  * @clkreg: default value for MCICLOCK register
  * @clkreg_enable: enable value for MMCICLOCK register
+ * @dma_sdio_req_ctrl: enable value for DMAREQCTL register for SDIO write
  * @datalength_bits: number of bits in the MMCIDATALENGTH register
  * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY
  *	      is asserted (likewise for RX)
@@ -60,6 +61,7 @@ static unsigned int fmax = 515633;
 struct variant_data {
 	unsigned int		clkreg;
 	unsigned int		clkreg_enable;
+	unsigned int		dma_sdio_req_ctrl;
 	unsigned int		datalength_bits;
 	unsigned int		fifosize;
 	unsigned int		fifohalfsize;
@@ -102,6 +104,7 @@ static struct variant_data variant_ux500 = {
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
+	.dma_sdio_req_ctrl	= MCI_ST_DPSM_DMAREQCTL,
 	.datalength_bits	= 24,
 	.sdio			= true,
 	.st_clkdiv		= true,
@@ -114,6 +117,7 @@ static struct variant_data variant_ux500v2 = {
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
+	.dma_sdio_req_ctrl	= MCI_ST_DPSM_DMAREQCTL,
 	.datalength_bits	= 24,
 	.sdio			= true,
 	.st_clkdiv		= true,
@@ -123,6 +127,30 @@ static struct variant_data variant_ux500v2 = {
 };
 
 /*
+ * Validate mmc prerequisites
+ */
+static int mmci_validate_data(struct mmci_host *host,
+			      struct mmc_data *data)
+{
+	if (!data)
+		return 0;
+
+	if (!is_power_of_2(data->blksz)) {
+		dev_err(mmc_dev(host->mmc),
+			"unsupported block size (%d bytes)\n", data->blksz);
+		return -EINVAL;
+	}
+
+	if (data->sg->offset & 3) {
+		dev_err(mmc_dev(host->mmc),
+			"unsupported alignment (0x%x)\n", data->sg->offset);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/*
  * This must be called with host->lock held
  */
 static void mmci_write_clkreg(struct mmci_host *host, u32 clk)
@@ -432,8 +460,12 @@ static int mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data,
 	if (!chan)
 		return -EINVAL;
 
-	/* If less than or equal to the fifo size, don't bother with DMA */
-	if (data->blksz * data->blocks <= variant->fifosize)
+	/*
+	 * If less than or equal to the fifo size, don't bother with DMA.
+	 * SDIO transfers may not be 4-byte aligned, fall back to PIO.
+	 */
+	if (data->blksz * data->blocks <= variant->fifosize ||
+	    (data->blksz * data->blocks) & 3)
 		return -EINVAL;
 
 	device = chan->device;
@@ -468,6 +500,7 @@ static int mmci_dma_start_data(struct mmci_host *host, unsigned int datactrl)
 {
 	int ret;
 	struct mmc_data *data = host->data;
+	struct variant_data *variant = host->variant;
 
 	ret = mmci_dma_prep_data(host, host->data, NULL);
 	if (ret)
@@ -482,6 +515,11 @@ static int mmci_dma_start_data(struct mmci_host *host, unsigned int datactrl)
 
 	datactrl |= MCI_DPSM_DMAENABLE;
 
+	/* Some hardware versions need special flags for SDIO DMA write. */
+	if (variant->sdio && host->mmc->card && mmc_card_sdio(host->mmc->card)
+	    && (data->flags & MMC_DATA_WRITE))
+		datactrl |= variant->dma_sdio_req_ctrl;
+
 	/* Trigger the DMA transfer */
 	writel(datactrl, host->base + MMCIDATACTRL);
 
@@ -526,6 +564,9 @@ static void mmci_pre_request(struct mmc_host *mmc, struct mmc_request *mrq,
 	if (!data)
 		return;
 
+	if (mmci_validate_data(host, mrq->data))
+		return;
+
 	if (data->host_cookie) {
 		data->host_cookie = 0;
 		return;
@@ -1036,10 +1077,8 @@ static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	WARN_ON(host->mrq != NULL);
 
-	if (mrq->data && !is_power_of_2(mrq->data->blksz)) {
-		dev_err(mmc_dev(mmc), "unsupported block size (%d bytes)\n",
-			mrq->data->blksz);
-		mrq->cmd->error = -EINVAL;
+	mrq->cmd->error = mmci_validate_data(host, mrq->data);
+	if (mrq->cmd->error) {
 		mmc_request_done(mmc, mrq);
 		return;
 	}
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index d437ccf..095c01c 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -60,6 +60,13 @@
 #define MCI_ST_DPSM_RWMOD	(1 << 10)
 #define MCI_ST_DPSM_SDIOEN	(1 << 11)
 /* Control register extensions in the ST Micro Ux500 versions */
+/*
+ * DMA request control is required for write
+ * if transfer size is not 32-byte aligned.
+ * DMA request control is also needed if the total
+ * transfer size is 32-byte aligned but any of the
+ * sg element lengths are not 32-byte aligned.
+ */
 #define MCI_ST_DPSM_DMAREQCTL	(1 << 12)
 #define MCI_ST_DPSM_DBOOTMODEEN	(1 << 13)
 #define MCI_ST_DPSM_BUSYMODE	(1 << 14)
-- 
1.7.5.4

[PATCH 8/8] mmc: mmci: Support any block sizes for ux500v2 variant

[Ulf Hansson]

For the ux500v2 variant of the PL18x block, any block sizes are
supported. This will make it possible to decrease data overhead
for SDIO transfers.

This patch is based upon a patch from Stefan Nilsson.

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 94c04c3..1c6c874 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -57,6 +57,7 @@ static unsigned int fmax = 515633;
  * @pwrreg_powerup: power up value for MMCIPOWER register
  * @signal_direction: input/out direction of bus signals can be indicated
  * @pwrreg_ctrl_power: bits in MMCIPOWER register controls ext. power supply
+ * @any_blksize: true if block any sizes are supported
  */
 struct variant_data {
 	unsigned int		clkreg;
@@ -71,6 +72,7 @@ struct variant_data {
 	u32			pwrreg_powerup;
 	bool			signal_direction;
 	bool			pwrreg_ctrl_power;
+	bool			any_blksize;
 };
 
 static struct variant_data variant_arm = {
@@ -124,6 +126,7 @@ static struct variant_data variant_ux500v2 = {
 	.blksz_datactrl16	= true,
 	.pwrreg_powerup		= MCI_PWR_ON,
 	.signal_direction	= true,
+	.any_blksize		= true,
 };
 
 /*
@@ -132,10 +135,12 @@ static struct variant_data variant_ux500v2 = {
 static int mmci_validate_data(struct mmci_host *host,
 			      struct mmc_data *data)
 {
+	struct variant_data *variant = host->variant;
+
 	if (!data)
 		return 0;
 
-	if (!is_power_of_2(data->blksz)) {
+	if (!is_power_of_2(data->blksz) && !variant->any_blksize) {
 		dev_err(mmc_dev(host->mmc),
 			"unsupported block size (%d bytes)\n", data->blksz);
 		return -EINVAL;
@@ -669,7 +674,6 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 	writel(host->size, base + MMCIDATALENGTH);
 
 	blksz_bits = ffs(data->blksz) - 1;
-	BUG_ON(1 << blksz_bits != data->blksz);
 
 	if (variant->blksz_datactrl16)
 		datactrl = MCI_DPSM_ENABLE | (data->blksz << 16);
-- 
1.7.5.4