[PATCH 0/5] mmc: sdhi: Allow waiting for idle

[PATCH 0/5] mmc: sdhi: Allow waiting for idle

[Simon Horman]

Some SDHI controllers require waiting for the SD bus to become
idle before writing to some registers. This series allows
this to occur by:
	* ensuring all relevant accesses are made via sd_ctrl_write16()
	* Adding a hook to sd_ctrl_write16()
	* Supplying a hook that implements waiting for idle

The series also includes some clean-ups of related code.

The first three patches are intended for the mmc tree
	* [PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE
	* [PATCH 2/5] mmc: tmio: Share register access functions
	* [PATCH 3/5] mmc: sdhi: Add write16_hook

The remaining two patches are intended for the rmobile tree
	* [PATCH 4/5] ARM: mach-shmobile: ag5evm: consistently name sdhi info
	* [PATCH 5/5] ARM: mach-shmobile: ag5evm: SDHI requires waiting for

I am not aware of any dependencies on patches outside this series.
Dependencies on patches within the series are noted inline.

[PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE

[Simon Horman]

This reflects at least the current usage of this register
and I think it improves the readability of the code ever so slightly.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

Dependencies: none known
---
 drivers/mmc/host/tmio_mmc_dma.c |    2 +-
 include/linux/mmc/tmio.h        |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c
index 25f1ad6..9c4da66 100644
--- a/drivers/mmc/host/tmio_mmc_dma.c
+++ b/drivers/mmc/host/tmio_mmc_dma.c
@@ -26,7 +26,7 @@ static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
 {
 #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)
 	/* Switch DMA mode on or off - SuperH specific? */
-	writew(enable ? 2 : 0, host->ctl + (0xd8 << host->bus_shift));
+	writew(enable ? 2 : 0, host->ctl + (CTL_DMA_ENABLE << host->bus_shift));
 #endif
 }
 
diff --git a/include/linux/mmc/tmio.h b/include/linux/mmc/tmio.h
index 19490b9..0a6e40d 100644
--- a/include/linux/mmc/tmio.h
+++ b/include/linux/mmc/tmio.h
@@ -30,6 +30,7 @@
 #define CTL_TRANSACTION_CTL 0x34
 #define CTL_SDIO_STATUS 0x36
 #define CTL_SDIO_IRQ_MASK 0x38
+#define CTL_DMA_ENABLE 0xd8
 #define CTL_RESET_SD 0xe0
 #define CTL_SDIO_REGS 0x100
 #define CTL_CLK_AND_WAIT_CTL 0x138
-- 
1.7.4.4

[PATCH 2/5] mmc: tmio: Share register access functions

[Simon Horman]

Move register access functions into a shared header.
Use sd_ctrl_write16 in tmio_mmc_dma.c:tmio_mmc_enable_dma().

Other than avoiding (trivial) open-coding, the motivation for
this is to allow platform-hooks in access functions to
be applied across all applicable accesses.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

Dependencies: "mmc: tmio: name 0xd8 as CTL_DMA_ENABLE"
---
 drivers/mmc/host/tmio_mmc.h     |   35 +++++++++++++++++++++++++++++++++++
 drivers/mmc/host/tmio_mmc_dma.c |    2 +-
 drivers/mmc/host/tmio_mmc_pio.c |   34 ----------------------------------
 3 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 8260bc2..0c22df0 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -134,4 +134,39 @@ int tmio_mmc_host_resume(struct device *dev);
 int tmio_mmc_host_runtime_suspend(struct device *dev);
 int tmio_mmc_host_runtime_resume(struct device *dev);
 
+static inline u16 sd_ctrl_read16(struct tmio_mmc_host *host, int addr)
+{
+	return readw(host->ctl + (addr << host->bus_shift));
+}
+
+static inline void sd_ctrl_read16_rep(struct tmio_mmc_host *host, int addr,
+		u16 *buf, int count)
+{
+	readsw(host->ctl + (addr << host->bus_shift), buf, count);
+}
+
+static inline u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr)
+{
+	return readw(host->ctl + (addr << host->bus_shift)) |
+	       readw(host->ctl + ((addr + 2) << host->bus_shift)) << 16;
+}
+
+static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val)
+{
+	writew(val, host->ctl + (addr << host->bus_shift));
+}
+
+static inline void sd_ctrl_write16_rep(struct tmio_mmc_host *host, int addr,
+		u16 *buf, int count)
+{
+	writesw(host->ctl + (addr << host->bus_shift), buf, count);
+}
+
+static inline void sd_ctrl_write32(struct tmio_mmc_host *host, int addr, u32 val)
+{
+	writew(val, host->ctl + (addr << host->bus_shift));
+	writew(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
+}
+
+
 #endif
diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c
index 9c4da66..f24a029 100644
--- a/drivers/mmc/host/tmio_mmc_dma.c
+++ b/drivers/mmc/host/tmio_mmc_dma.c
@@ -26,7 +26,7 @@ static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
 {
 #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)
 	/* Switch DMA mode on or off - SuperH specific? */
-	writew(enable ? 2 : 0, host->ctl + (CTL_DMA_ENABLE << host->bus_shift));
+	sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE);
 #endif
 }
 
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index ad6347b..105f720 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -46,40 +46,6 @@
 
 #include "tmio_mmc.h"
 
-static u16 sd_ctrl_read16(struct tmio_mmc_host *host, int addr)
-{
-	return readw(host->ctl + (addr << host->bus_shift));
-}
-
-static void sd_ctrl_read16_rep(struct tmio_mmc_host *host, int addr,
-		u16 *buf, int count)
-{
-	readsw(host->ctl + (addr << host->bus_shift), buf, count);
-}
-
-static u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr)
-{
-	return readw(host->ctl + (addr << host->bus_shift)) |
-	       readw(host->ctl + ((addr + 2) << host->bus_shift)) << 16;
-}
-
-static void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val)
-{
-	writew(val, host->ctl + (addr << host->bus_shift));
-}
-
-static void sd_ctrl_write16_rep(struct tmio_mmc_host *host, int addr,
-		u16 *buf, int count)
-{
-	writesw(host->ctl + (addr << host->bus_shift), buf, count);
-}
-
-static void sd_ctrl_write32(struct tmio_mmc_host *host, int addr, u32 val)
-{
-	writew(val, host->ctl + (addr << host->bus_shift));
-	writew(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
-}
-
 void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
 {
 	u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) & ~(i & TMIO_MASK_IRQ);
-- 
1.7.4.4

[PATCH 3/5] mmc: sdhi: Add write16_hook

[Simon Horman]

Some controllers require waiting for the bus to become idle
before writing to some registers. I have implemented this
by adding a hook to sd_ctrl_write16() and implementing
a hook for SDHI which waits for the bus to become idle.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

Dependenvies: "mmc: tmio: Share register access functions"
---
 drivers/mmc/host/sh_mobile_sdhi.c |   34 ++++++++++++++++++++++++++++++++++
 drivers/mmc/host/tmio_mmc.h       |    2 ++
 include/linux/mfd/tmio.h          |    8 ++++++++
 include/linux/mmc/tmio.h          |    1 +
 4 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index b365429..6c56453 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -27,6 +27,8 @@
 #include <linux/mfd/tmio.h>
 #include <linux/sh_dma.h>
 
+#include <asm/delay.h>
+
 #include "tmio_mmc.h"
 
 struct sh_mobile_sdhi {
@@ -55,6 +57,37 @@ static int sh_mobile_sdhi_get_cd(struct platform_device *pdev)
 		return -ENOSYS;
 }
 
+static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
+{
+	int timeout = 1000;
+
+	while (--timeout && !(sd_ctrl_read16(host, CTL_STATUS2) & (1 << 13)))
+		udelay(1);
+
+	if (!timeout)
+		dev_warn(host->pdata->dev, "timeout waiting for SD bus idle\n");
+
+}
+
+static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr)
+{
+	if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
+		return;
+
+	switch (addr)
+	{
+	case CTL_SD_CMD:
+	case CTL_STOP_INTERNAL_ACTION:
+	case CTL_XFER_BLK_COUNT:
+	case CTL_SD_CARD_CLK_CTL:
+	case CTL_SD_XFER_LEN:
+	case CTL_SD_MEM_CARD_OPT:
+	case CTL_TRANSACTION_CTL:
+	case CTL_DMA_ENABLE:
+		sh_mobile_sdhi_wait_idle(host);
+	}
+}
+
 static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 {
 	struct sh_mobile_sdhi *priv;
@@ -86,6 +119,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 	mmc_data->hclk = clk_get_rate(priv->clk);
 	mmc_data->set_pwr = sh_mobile_sdhi_set_pwr;
 	mmc_data->get_cd = sh_mobile_sdhi_get_cd;
+	mmc_data->write16_hook = sh_mobile_sdhi_write16_hook;
 	mmc_data->capabilities = MMC_CAP_MMC_HIGHSPEED;
 	if (p) {
 		mmc_data->flags = p->tmio_flags;
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 0c22df0..5912cf3 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -153,6 +153,8 @@ static inline u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr)
 
 static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val)
 {
+	if (host->pdata->write16_hook)
+		host->pdata->write16_hook(host, addr);
 	writew(val, host->ctl + (addr << host->bus_shift));
 }
 
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index 5a90266..dc5db86 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -68,6 +68,11 @@
  * controller and report the event to the driver.
  */
 #define TMIO_MMC_HAS_COLD_CD		(1 << 3)
+/*
+ * Some controllers require waiting for the SD bus to become
+ * idle before writing to some registers.
+ */
+#define TMIO_MMC_HAS_IDLE_WAIT		(1 << 4)
 
 int tmio_core_mmc_enable(void __iomem *cnf, int shift, unsigned long base);
 int tmio_core_mmc_resume(void __iomem *cnf, int shift, unsigned long base);
@@ -80,6 +85,8 @@ struct tmio_mmc_dma {
 	int alignment_shift;
 };
 
+struct tmio_mmc_host;
+
 /*
  * data for the MMC controller
  */
@@ -94,6 +101,7 @@ struct tmio_mmc_data {
 	void (*set_pwr)(struct platform_device *host, int state);
 	void (*set_clk_div)(struct platform_device *host, int state);
 	int (*get_cd)(struct platform_device *host);
+	void (*write16_hook)(struct tmio_mmc_host *host, int addr);
 };
 
 static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
diff --git a/include/linux/mmc/tmio.h b/include/linux/mmc/tmio.h
index 0a6e40d..f235757 100644
--- a/include/linux/mmc/tmio.h
+++ b/include/linux/mmc/tmio.h
@@ -21,6 +21,7 @@
 #define CTL_XFER_BLK_COUNT 0xa
 #define CTL_RESPONSE 0x0c
 #define CTL_STATUS 0x1c
+#define CTL_STATUS2 0x1e
 #define CTL_IRQ_MASK 0x20
 #define CTL_SD_CARD_CLK_CTL 0x24
 #define CTL_SD_XFER_LEN 0x26
-- 
1.7.4.4

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Paul Mundt]

On Mon, Jun 20, 2011 at 03:06:53PM +0900, Simon Horman wrote:
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index b365429..6c56453 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -27,6 +27,8 @@
>  #include <linux/mfd/tmio.h>
>  #include <linux/sh_dma.h>
>  
> +#include <asm/delay.h>
> +
>  #include "tmio_mmc.h"
>  
linux/delay.h will suffice.

> +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
> +{
> +	int timeout = 1000;
> +
> +	while (--timeout && !(sd_ctrl_read16(host, CTL_STATUS2) & (1 << 13)))
> +		udelay(1);
> +
> +	if (!timeout)
> +		dev_warn(host->pdata->dev, "timeout waiting for SD bus idle\n");
> +
> +}
> +
> +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr)
> +{
> +	if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
> +		return;
> +
> +	switch (addr)
> +	{
> +	case CTL_SD_CMD:
> +	case CTL_STOP_INTERNAL_ACTION:
> +	case CTL_XFER_BLK_COUNT:
> +	case CTL_SD_CARD_CLK_CTL:
> +	case CTL_SD_XFER_LEN:
> +	case CTL_SD_MEM_CARD_OPT:
> +	case CTL_TRANSACTION_CTL:
> +	case CTL_DMA_ENABLE:
> +		sh_mobile_sdhi_wait_idle(host);
> +	}
> +}
> +
The timeout error should really be handed down.

> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 0c22df0..5912cf3 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -153,6 +153,8 @@ static inline u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr)
>  
>  static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val)
>  {
> +	if (host->pdata->write16_hook)
> +		host->pdata->write16_hook(host, addr);
>  	writew(val, host->ctl + (addr << host->bus_shift));
>  }
>  
If it times out you are unlikely to want to proceed with the write. At
best you end up with a lost write cycle, at worst potentially a bus
lockup you can't easily recover from.

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Simon Horman]

On Mon, Jun 20, 2011 at 03:25:22PM +0900, Paul Mundt wrote:
> On Mon, Jun 20, 2011 at 03:06:53PM +0900, Simon Horman wrote:
> > diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> > index b365429..6c56453 100644
> > --- a/drivers/mmc/host/sh_mobile_sdhi.c
> > +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> > @@ -27,6 +27,8 @@
> >  #include <linux/mfd/tmio.h>
> >  #include <linux/sh_dma.h>
> >  
> > +#include <asm/delay.h>
> > +
> >  #include "tmio_mmc.h"
> >  
> linux/delay.h will suffice.

Thanks.

> > +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
> > +{
> > +	int timeout = 1000;
> > +
> > +	while (--timeout && !(sd_ctrl_read16(host, CTL_STATUS2) & (1 << 13)))
> > +		udelay(1);
> > +
> > +	if (!timeout)
> > +		dev_warn(host->pdata->dev, "timeout waiting for SD bus idle\n");
> > +
> > +}
> > +
> > +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr)
> > +{
> > +	if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
> > +		return;
> > +
> > +	switch (addr)
> > +	{
> > +	case CTL_SD_CMD:
> > +	case CTL_STOP_INTERNAL_ACTION:
> > +	case CTL_XFER_BLK_COUNT:
> > +	case CTL_SD_CARD_CLK_CTL:
> > +	case CTL_SD_XFER_LEN:
> > +	case CTL_SD_MEM_CARD_OPT:
> > +	case CTL_TRANSACTION_CTL:
> > +	case CTL_DMA_ENABLE:
> > +		sh_mobile_sdhi_wait_idle(host);
> > +	}
> > +}
> > +
> The timeout error should really be handed down.
> 
> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> > index 0c22df0..5912cf3 100644
> > --- a/drivers/mmc/host/tmio_mmc.h
> > +++ b/drivers/mmc/host/tmio_mmc.h
> > @@ -153,6 +153,8 @@ static inline u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr)
> >  
> >  static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val)
> >  {
> > +	if (host->pdata->write16_hook)
> > +		host->pdata->write16_hook(host, addr);
> >  	writew(val, host->ctl + (addr << host->bus_shift));
> >  }
> >  
> If it times out you are unlikely to want to proceed with the write. At
> best you end up with a lost write cycle, at worst potentially a bus
> lockup you can't easily recover from.

As discussed off-line, I'll change this around so that
write16_hook() may return an error and the write will be
skipped if that occurs.

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Guennadi Liakhovetski]

On Mon, 20 Jun 2011, Simon Horman wrote:

> Some controllers require waiting for the bus to become idle
> before writing to some registers. I have implemented this
> by adding a hook to sd_ctrl_write16() and implementing
> a hook for SDHI which waits for the bus to become idle.
> 
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> ---
> 
> Dependenvies: "mmc: tmio: Share register access functions"
> ---
>  drivers/mmc/host/sh_mobile_sdhi.c |   34 ++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/tmio_mmc.h       |    2 ++
>  include/linux/mfd/tmio.h          |    8 ++++++++
>  include/linux/mmc/tmio.h          |    1 +
>  4 files changed, 45 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index b365429..6c56453 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -27,6 +27,8 @@
>  #include <linux/mfd/tmio.h>
>  #include <linux/sh_dma.h>
>  
> +#include <asm/delay.h>

linux/delay.h

> +
>  #include "tmio_mmc.h"
>  
>  struct sh_mobile_sdhi {
> @@ -55,6 +57,37 @@ static int sh_mobile_sdhi_get_cd(struct platform_device *pdev)
>  		return -ENOSYS;
>  }
>  
> +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
> +{
> +	int timeout = 1000;
> +
> +	while (--timeout && !(sd_ctrl_read16(host, CTL_STATUS2) & (1 << 13)))
> +		udelay(1);
> +
> +	if (!timeout)
> +		dev_warn(host->pdata->dev, "timeout waiting for SD bus idle\n");
> +
> +}
> +
> +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr)
> +{
> +	if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
> +		return;
> +
> +	switch (addr)
> +	{
> +	case CTL_SD_CMD:
> +	case CTL_STOP_INTERNAL_ACTION:
> +	case CTL_XFER_BLK_COUNT:
> +	case CTL_SD_CARD_CLK_CTL:
> +	case CTL_SD_XFER_LEN:
> +	case CTL_SD_MEM_CARD_OPT:
> +	case CTL_TRANSACTION_CTL:
> +	case CTL_DMA_ENABLE:
> +		sh_mobile_sdhi_wait_idle(host);
> +	}
> +}
> +
>  static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
>  {
>  	struct sh_mobile_sdhi *priv;
> @@ -86,6 +119,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
>  	mmc_data->hclk = clk_get_rate(priv->clk);
>  	mmc_data->set_pwr = sh_mobile_sdhi_set_pwr;
>  	mmc_data->get_cd = sh_mobile_sdhi_get_cd;
> +	mmc_data->write16_hook = sh_mobile_sdhi_write16_hook;

Not sure I like the "write16_hook" name. You consider this callback as 
something the host might need to do, when writing to a 16-bit register. In 
this specific case it is actually waiting for the bus to become idle, 
which might also be needed in other locations. So, maybe it is better to 
call this callback "wait_idle" or something similar? Or you think, other 
hosts might need a write16 hook doing something different?...

Thanks
Guennadi

>  	mmc_data->capabilities = MMC_CAP_MMC_HIGHSPEED;
>  	if (p) {
>  		mmc_data->flags = p->tmio_flags;
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 0c22df0..5912cf3 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -153,6 +153,8 @@ static inline u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr)
>  
>  static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val)
>  {
> +	if (host->pdata->write16_hook)
> +		host->pdata->write16_hook(host, addr);
>  	writew(val, host->ctl + (addr << host->bus_shift));
>  }
>  
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index 5a90266..dc5db86 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -68,6 +68,11 @@
>   * controller and report the event to the driver.
>   */
>  #define TMIO_MMC_HAS_COLD_CD		(1 << 3)
> +/*
> + * Some controllers require waiting for the SD bus to become
> + * idle before writing to some registers.
> + */
> +#define TMIO_MMC_HAS_IDLE_WAIT		(1 << 4)
>  
>  int tmio_core_mmc_enable(void __iomem *cnf, int shift, unsigned long base);
>  int tmio_core_mmc_resume(void __iomem *cnf, int shift, unsigned long base);
> @@ -80,6 +85,8 @@ struct tmio_mmc_dma {
>  	int alignment_shift;
>  };
>  
> +struct tmio_mmc_host;
> +
>  /*
>   * data for the MMC controller
>   */
> @@ -94,6 +101,7 @@ struct tmio_mmc_data {
>  	void (*set_pwr)(struct platform_device *host, int state);
>  	void (*set_clk_div)(struct platform_device *host, int state);
>  	int (*get_cd)(struct platform_device *host);
> +	void (*write16_hook)(struct tmio_mmc_host *host, int addr);
>  };
>  
>  static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
> diff --git a/include/linux/mmc/tmio.h b/include/linux/mmc/tmio.h
> index 0a6e40d..f235757 100644
> --- a/include/linux/mmc/tmio.h
> +++ b/include/linux/mmc/tmio.h
> @@ -21,6 +21,7 @@
>  #define CTL_XFER_BLK_COUNT 0xa
>  #define CTL_RESPONSE 0x0c
>  #define CTL_STATUS 0x1c
> +#define CTL_STATUS2 0x1e
>  #define CTL_IRQ_MASK 0x20
>  #define CTL_SD_CARD_CLK_CTL 0x24
>  #define CTL_SD_XFER_LEN 0x26
> -- 
> 1.7.4.4
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Simon Horman]

On Mon, Jun 20, 2011 at 08:29:18AM +0200, Guennadi Liakhovetski wrote:
> On Mon, 20 Jun 2011, Simon Horman wrote:
> 
> > Some controllers require waiting for the bus to become idle
> > before writing to some registers. I have implemented this
> > by adding a hook to sd_ctrl_write16() and implementing
> > a hook for SDHI which waits for the bus to become idle.
> > 
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> > 
> > ---
> > 
> > Dependenvies: "mmc: tmio: Share register access functions"
> > ---
> >  drivers/mmc/host/sh_mobile_sdhi.c |   34 ++++++++++++++++++++++++++++++++++
> >  drivers/mmc/host/tmio_mmc.h       |    2 ++
> >  include/linux/mfd/tmio.h          |    8 ++++++++
> >  include/linux/mmc/tmio.h          |    1 +
> >  4 files changed, 45 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> > index b365429..6c56453 100644
> > --- a/drivers/mmc/host/sh_mobile_sdhi.c
> > +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> > @@ -27,6 +27,8 @@
> >  #include <linux/mfd/tmio.h>
> >  #include <linux/sh_dma.h>
> >  
> > +#include <asm/delay.h>
> 
> linux/delay.h

Thanks.

> > +
> >  #include "tmio_mmc.h"
> >  
> >  struct sh_mobile_sdhi {
> > @@ -55,6 +57,37 @@ static int sh_mobile_sdhi_get_cd(struct platform_device *pdev)
> >  		return -ENOSYS;
> >  }
> >  
> > +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
> > +{
> > +	int timeout = 1000;
> > +
> > +	while (--timeout && !(sd_ctrl_read16(host, CTL_STATUS2) & (1 << 13)))
> > +		udelay(1);
> > +
> > +	if (!timeout)
> > +		dev_warn(host->pdata->dev, "timeout waiting for SD bus idle\n");
> > +
> > +}
> > +
> > +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr)
> > +{
> > +	if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
> > +		return;
> > +
> > +	switch (addr)
> > +	{
> > +	case CTL_SD_CMD:
> > +	case CTL_STOP_INTERNAL_ACTION:
> > +	case CTL_XFER_BLK_COUNT:
> > +	case CTL_SD_CARD_CLK_CTL:
> > +	case CTL_SD_XFER_LEN:
> > +	case CTL_SD_MEM_CARD_OPT:
> > +	case CTL_TRANSACTION_CTL:
> > +	case CTL_DMA_ENABLE:
> > +		sh_mobile_sdhi_wait_idle(host);
> > +	}
> > +}
> > +
> >  static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
> >  {
> >  	struct sh_mobile_sdhi *priv;
> > @@ -86,6 +119,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
> >  	mmc_data->hclk = clk_get_rate(priv->clk);
> >  	mmc_data->set_pwr = sh_mobile_sdhi_set_pwr;
> >  	mmc_data->get_cd = sh_mobile_sdhi_get_cd;
> > +	mmc_data->write16_hook = sh_mobile_sdhi_write16_hook;
> 
> Not sure I like the "write16_hook" name. You consider this callback as 
> something the host might need to do, when writing to a 16-bit register. In 
> this specific case it is actually waiting for the bus to become idle, 
> which might also be needed in other locations. So, maybe it is better to 
> call this callback "wait_idle" or something similar? Or you think, other 
> hosts might need a write16 hook doing something different?...

I'm not strongly attached to the name, and I do see your point.  But as it
stands the hook is currently only needed on 16bit register writes; the hook
is called from sd_ctrl_write16(): and sh_mobile_sdhi_write16_hook() works
by looking at addr, which is specific to register reads and writes.  So
while it isn't great, I think the current name isn't entirely horrible.

I think its a little hard to look into a crystal ball and see
where other TMIO hardware might need wait_idle(). Who knows
what we might need to do for the next batch of hardware? :-)

I had considered other approaches to this problem, such as
calling something_wait_idle() from within tmio_mmc_*.c as needed.
But the approach in the patch I posted turned out to be quite
a lot cleaner and have the advantage of concisely documenting
what is being done - that is, writes to which registers need
wait_idle treatment.

The down side is that the callback is obviously more specific
than perhaps a hook ought to be. But I think that it can evolve
as the need arises.

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Magnus Damm]

On Mon, Jun 20, 2011 at 3:06 PM, Simon Horman <horms@verge.net.au> wrote:
> Some controllers require waiting for the bus to become idle
> before writing to some registers. I have implemented this
> by adding a hook to sd_ctrl_write16() and implementing
> a hook for SDHI which waits for the bus to become idle.
>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> ---

Hi Simon,

Thanks for your work on this!

> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -55,6 +57,37 @@ static int sh_mobile_sdhi_get_cd(struct platform_device *pdev)
>                return -ENOSYS;
>  }
>
> +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
> +{
> +       int timeout = 1000;
> +
> +       while (--timeout && !(sd_ctrl_read16(host, CTL_STATUS2) & (1 << 13)))
> +               udelay(1);
> +
> +       if (!timeout)
> +               dev_warn(host->pdata->dev, "timeout waiting for SD bus idle\n");
> +
> +}
> +
> +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr)
> +{
> +       if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
> +               return;

and

> @@ -86,6 +119,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
>        mmc_data->hclk = clk_get_rate(priv->clk);
>        mmc_data->set_pwr = sh_mobile_sdhi_set_pwr;
>        mmc_data->get_cd = sh_mobile_sdhi_get_cd;
> +       mmc_data->write16_hook = sh_mobile_sdhi_write16_hook;
>        mmc_data->capabilities = MMC_CAP_MMC_HIGHSPEED;
>        if (p) {
>                mmc_data->flags = p->tmio_flags;

Doesn't it make more sense to do:

if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
  mmc_data->write16_hook = sh_mobile_sdhi_write16_hook;

and skip the check inside the sh_mobile_sdhi_write16_hook() function?

No need to do that check on every register access unless really needed.

Thanks,

/ magnus

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Simon Horman]

On Mon, Jun 20, 2011 at 04:04:04PM +0900, Magnus Damm wrote:
> On Mon, Jun 20, 2011 at 3:06 PM, Simon Horman <horms@verge.net.au> wrote:
> > Some controllers require waiting for the bus to become idle
> > before writing to some registers. I have implemented this
> > by adding a hook to sd_ctrl_write16() and implementing
> > a hook for SDHI which waits for the bus to become idle.
> >
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > ---
> 
> Hi Simon,
> 
> Thanks for your work on this!
> 
> > --- a/drivers/mmc/host/sh_mobile_sdhi.c
> > +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> > @@ -55,6 +57,37 @@ static int sh_mobile_sdhi_get_cd(struct platform_device *pdev)
> >                return -ENOSYS;
> >  }
> >
> > +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
> > +{
> > +       int timeout = 1000;
> > +
> > +       while (--timeout && !(sd_ctrl_read16(host, CTL_STATUS2) & (1 << 13)))
> > +               udelay(1);
> > +
> > +       if (!timeout)
> > +               dev_warn(host->pdata->dev, "timeout waiting for SD bus idle\n");
> > +
> > +}
> > +
> > +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr)
> > +{
> > +       if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
> > +               return;
> 
> and
> 
> > @@ -86,6 +119,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
> >        mmc_data->hclk = clk_get_rate(priv->clk);
> >        mmc_data->set_pwr = sh_mobile_sdhi_set_pwr;
> >        mmc_data->get_cd = sh_mobile_sdhi_get_cd;
> > +       mmc_data->write16_hook = sh_mobile_sdhi_write16_hook;
> >        mmc_data->capabilities = MMC_CAP_MMC_HIGHSPEED;
> >        if (p) {
> >                mmc_data->flags = p->tmio_flags;
> 
> Doesn't it make more sense to do:
> 
> if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
>   mmc_data->write16_hook = sh_mobile_sdhi_write16_hook;
> 
> and skip the check inside the sh_mobile_sdhi_write16_hook() function?
> 
> No need to do that check on every register access unless really needed.

Sure, good idea.

[PATCH 4/5] ARM: mach-shmobile: ag5evm: consistently name sdhi info structures

[Simon Horman]

Name the SDHI1 instance sh_sdhi1_info to be consistent with sh_sdhi0_info.

Signed-off-by: Simon Horman <horms@verge.net.au>

---

Dependencies: None known
---
 arch/arm/mach-shmobile/board-ag5evm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c
index 1e2aba2..ce5c251 100644
--- a/arch/arm/mach-shmobile/board-ag5evm.c
+++ b/arch/arm/mach-shmobile/board-ag5evm.c
@@ -381,7 +381,7 @@ void ag5evm_sdhi1_set_pwr(struct platform_device *pdev, int state)
 	gpio_set_value(GPIO_PORT114, state);
 }
 
-static struct sh_mobile_sdhi_info sh_sdhi1_platdata = {
+static struct sh_mobile_sdhi_info sh_sdhi1_info = {
 	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE,
 	.tmio_caps	= MMC_CAP_NONREMOVABLE | MMC_CAP_SDIO_IRQ,
 	.tmio_ocr_mask	= MMC_VDD_32_33 | MMC_VDD_33_34,
@@ -413,7 +413,7 @@ static struct platform_device sdhi1_device = {
 	.name		= "sh_mobile_sdhi",
 	.id		= 1,
 	.dev		= {
-		.platform_data	= &sh_sdhi1_platdata,
+		.platform_data	= &sh_sdhi1_info,
 	},
 	.num_resources	= ARRAY_SIZE(sdhi1_resources),
 	.resource	= sdhi1_resources,
-- 
1.7.4.4

[PATCH 5/5] ARM: mach-shmobile: ag5evm: SDHI requires waiting for idle

[Simon Horman]

The SDHI block on the ag5evm requires waiting for idle
before writing to some registers.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

Dependencies:
	"mmc: sdhi: Add write16_hook"
	"ARM: mach-shmobile: ag5evm: consistently name sdhi info structures"
---
 arch/arm/mach-shmobile/board-ag5evm.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c
index ce5c251..cdfdd62 100644
--- a/arch/arm/mach-shmobile/board-ag5evm.c
+++ b/arch/arm/mach-shmobile/board-ag5evm.c
@@ -341,6 +341,7 @@ static struct platform_device mipidsi0_device = {
 static struct sh_mobile_sdhi_info sdhi0_info = {
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
+	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT,
 	.tmio_caps	= MMC_CAP_SD_HIGHSPEED,
 	.tmio_ocr_mask	= MMC_VDD_27_28 | MMC_VDD_28_29,
 };
@@ -382,7 +383,7 @@ void ag5evm_sdhi1_set_pwr(struct platform_device *pdev, int state)
 }
 
 static struct sh_mobile_sdhi_info sh_sdhi1_info = {
-	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE,
+	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE | TMIO_MMC_HAS_IDLE_WAIT,
 	.tmio_caps	= MMC_CAP_NONREMOVABLE | MMC_CAP_SDIO_IRQ,
 	.tmio_ocr_mask	= MMC_VDD_32_33 | MMC_VDD_33_34,
 	.set_pwr	= ag5evm_sdhi1_set_pwr,
-- 
1.7.4.4

[PATCH 0/5 v2] mmc: sdhi: Allow waiting for idle

[Simon Horman]

Some SDHI controllers require waiting for the SD bus to become
idle before writing to some registers. This series allows
this to occur by:
	* ensuring all relevant accesses are made via sd_ctrl_write16()
	* Adding a hook to sd_ctrl_write16()
	* Supplying a hook that implements waiting for idle

The series also includes some clean-ups of related code.

The first three patches are intended for the mmc tree
	* [PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE
	* [PATCH 2/5] mmc: tmio: Share register access functions
	* [PATCH 3/5] mmc: sdhi: Add write16_hook

The remaining two patches are intended for the rmobile tree
	* [PATCH 4/5] ARM: mach-shmobile: ag5evm: consistently name sdhi info
	* [PATCH 5/5] ARM: mach-shmobile: ag5evm: SDHI requires waiting for

I am not aware of any dependencies on patches outside this series.
Dependencies on patches within the series are noted inline.

This is v2 of this patch series, issues addressed since v1 are
all in [PATCH 3/5] mmc: sdhi: Add write16_hook:
	* Include linux/delay.h instead of asm/delay.h
	* Skip write if sh_mobile_sdhi_wait_idle() times out
	  - The bus will probably be in an inconsistent state and writing
	    may lock up the bus
	* Only set hook if TMIO_MMC_HAS_IDLE_WAIT is set in platform data
	  rather than checking for TMIO_MMC_HAS_IDLE_WAIT each time the
	  hook is called.

[PATCH 3/5] mmc: sdhi: Add write16_hook

[Simon Horman]

Some controllers require waiting for the bus to become idle
before writing to some registers. I have implemented this
by adding a hook to sd_ctrl_write16() and implementing
a hook for SDHI which waits for the bus to become idle.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

Dependencies: "mmc: tmio: Share register access functions"

v2:
* Include linux/delay.h instead of asm/delay.h
* Skip write if sh_mobile_sdhi_wait_idle() times out
  - The bus will probably be in an inconsistent state and writing
    may lock up the bus
* Only set hook if TMIO_MMC_HAS_IDLE_WAIT is set in platform data
  rather than checking for TMIO_MMC_HAS_IDLE_WAIT each time the
  hook is called.
---
 drivers/mmc/host/sh_mobile_sdhi.c |   36 ++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/tmio_mmc.h       |    5 +++++
 include/linux/mfd/tmio.h          |    8 ++++++++
 include/linux/mmc/tmio.h          |    1 +
 4 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index b365429..c8b1f6b 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -26,6 +26,7 @@
 #include <linux/mmc/sh_mobile_sdhi.h>
 #include <linux/mfd/tmio.h>
 #include <linux/sh_dma.h>
+#include <linux/delay.h>
 
 #include "tmio_mmc.h"
 
@@ -55,6 +56,39 @@ static int sh_mobile_sdhi_get_cd(struct platform_device *pdev)
 		return -ENOSYS;
 }
 
+static int sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
+{
+	int timeout = 1000;
+
+	while (--timeout && !(sd_ctrl_read16(host, CTL_STATUS2) & (1 << 13)))
+		udelay(1);
+
+	if (!timeout) {
+		dev_warn(host->pdata->dev, "timeout waiting for SD bus idle\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr)
+{
+	switch (addr)
+	{
+	case CTL_SD_CMD:
+	case CTL_STOP_INTERNAL_ACTION:
+	case CTL_XFER_BLK_COUNT:
+	case CTL_SD_CARD_CLK_CTL:
+	case CTL_SD_XFER_LEN:
+	case CTL_SD_MEM_CARD_OPT:
+	case CTL_TRANSACTION_CTL:
+	case CTL_DMA_ENABLE:
+		return sh_mobile_sdhi_wait_idle(host);
+	}
+
+	return 0;
+}
+
 static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 {
 	struct sh_mobile_sdhi *priv;
@@ -86,6 +120,8 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 	mmc_data->hclk = clk_get_rate(priv->clk);
 	mmc_data->set_pwr = sh_mobile_sdhi_set_pwr;
 	mmc_data->get_cd = sh_mobile_sdhi_get_cd;
+	if (mmc_data->flags & TMIO_MMC_HAS_IDLE_WAIT)
+		mmc_data->write16_hook = sh_mobile_sdhi_write16_hook;
 	mmc_data->capabilities = MMC_CAP_MMC_HIGHSPEED;
 	if (p) {
 		mmc_data->flags = p->tmio_flags;
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 0c22df0..211ef6e 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -153,6 +153,11 @@ static inline u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr)
 
 static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val)
 {
+	/* If there is a hook and it returns non-zero then there
+	 * is an error and the write should be skipped
+	 */
+	if (host->pdata->write16_hook && host->pdata->write16_hook(host, addr))
+		return;
 	writew(val, host->ctl + (addr << host->bus_shift));
 }
 
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index 5a90266..0dc9804 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -68,6 +68,11 @@
  * controller and report the event to the driver.
  */
 #define TMIO_MMC_HAS_COLD_CD		(1 << 3)
+/*
+ * Some controllers require waiting for the SD bus to become
+ * idle before writing to some registers.
+ */
+#define TMIO_MMC_HAS_IDLE_WAIT		(1 << 4)
 
 int tmio_core_mmc_enable(void __iomem *cnf, int shift, unsigned long base);
 int tmio_core_mmc_resume(void __iomem *cnf, int shift, unsigned long base);
@@ -80,6 +85,8 @@ struct tmio_mmc_dma {
 	int alignment_shift;
 };
 
+struct tmio_mmc_host;
+
 /*
  * data for the MMC controller
  */
@@ -94,6 +101,7 @@ struct tmio_mmc_data {
 	void (*set_pwr)(struct platform_device *host, int state);
 	void (*set_clk_div)(struct platform_device *host, int state);
 	int (*get_cd)(struct platform_device *host);
+	int (*write16_hook)(struct tmio_mmc_host *host, int addr);
 };
 
 static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
diff --git a/include/linux/mmc/tmio.h b/include/linux/mmc/tmio.h
index 0a6e40d..f235757 100644
--- a/include/linux/mmc/tmio.h
+++ b/include/linux/mmc/tmio.h
@@ -21,6 +21,7 @@
 #define CTL_XFER_BLK_COUNT 0xa
 #define CTL_RESPONSE 0x0c
 #define CTL_STATUS 0x1c
+#define CTL_STATUS2 0x1e
 #define CTL_IRQ_MASK 0x20
 #define CTL_SD_CARD_CLK_CTL 0x24
 #define CTL_SD_XFER_LEN 0x26
-- 
1.7.4.4

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Magnus Damm]

On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman <horms@verge.net.au> wrote:
> Some controllers require waiting for the bus to become idle
> before writing to some registers. I have implemented this
> by adding a hook to sd_ctrl_write16() and implementing
> a hook for SDHI which waits for the bus to become idle.
>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> ---
>
> Dependencies: "mmc: tmio: Share register access functions"
>
> v2:
> * Include linux/delay.h instead of asm/delay.h
> * Skip write if sh_mobile_sdhi_wait_idle() times out
>  - The bus will probably be in an inconsistent state and writing
>    may lock up the bus
> * Only set hook if TMIO_MMC_HAS_IDLE_WAIT is set in platform data
>  rather than checking for TMIO_MMC_HAS_IDLE_WAIT each time the
>  hook is called.
> ---

Thanks Simon, this version looks much better!

> index 5a90266..0dc9804 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -94,6 +101,7 @@ struct tmio_mmc_data {
>        void (*set_pwr)(struct platform_device *host, int state);
>        void (*set_clk_div)(struct platform_device *host, int state);
>        int (*get_cd)(struct platform_device *host);
> +       int (*write16_hook)(struct tmio_mmc_host *host, int addr);
>  };
>
>  static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)

What's the reason behind passing "struct tmio_mmc_host *"  as an
argument to the new hook? Performance? All other callbacks seem to
take a "struct platform_device *", so being consistent here may be
good unless it comes with too much overhead.

Thanks,

/ magnus

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Simon Horman]

On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote:
> On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman <horms@verge.net.au> wrote:
> > Some controllers require waiting for the bus to become idle
> > before writing to some registers. I have implemented this
> > by adding a hook to sd_ctrl_write16() and implementing
> > a hook for SDHI which waits for the bus to become idle.
> >
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > ---
> >
> > Dependencies: "mmc: tmio: Share register access functions"
> >
> > v2:
> > * Include linux/delay.h instead of asm/delay.h
> > * Skip write if sh_mobile_sdhi_wait_idle() times out
> >  - The bus will probably be in an inconsistent state and writing
> >    may lock up the bus
> > * Only set hook if TMIO_MMC_HAS_IDLE_WAIT is set in platform data
> >  rather than checking for TMIO_MMC_HAS_IDLE_WAIT each time the
> >  hook is called.
> > ---
> 
> Thanks Simon, this version looks much better!
> 
> > index 5a90266..0dc9804 100644
> > --- a/include/linux/mfd/tmio.h
> > +++ b/include/linux/mfd/tmio.h
> > @@ -94,6 +101,7 @@ struct tmio_mmc_data {
> >        void (*set_pwr)(struct platform_device *host, int state);
> >        void (*set_clk_div)(struct platform_device *host, int state);
> >        int (*get_cd)(struct platform_device *host);
> > +       int (*write16_hook)(struct tmio_mmc_host *host, int addr);
> >  };
> >
> >  static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
> 
> What's the reason behind passing "struct tmio_mmc_host *"  as an
> argument to the new hook? Performance? All other callbacks seem to
> take a "struct platform_device *", so being consistent here may be
> good unless it comes with too much overhead.

The reason is that
1) The hook is called from sd_ctrl_write16 which takes 
   struct tmio_mmc_host * as its first argument and;
2) The hook that has been implemented calls sd_ctrl_read16() which takes a
   struct tmio_mmc_host * as its first argument.
So it seemed logical to pass that down.

In the caes of 1) we can get the struct platform_device * using host->pdev.
However, in the case of 2) is it less clear to me how we can get the
struct tmio_mmc_host * from a struct platform_device *.

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Magnus Damm]

On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote:
>> On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman <horms@verge.net.au> wrote:
>> > Some controllers require waiting for the bus to become idle
>> > before writing to some registers. I have implemented this
>> > by adding a hook to sd_ctrl_write16() and implementing
>> > a hook for SDHI which waits for the bus to become idle.
>> >
>> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> > Cc: Magnus Damm <magnus.damm@gmail.com>
>> > Signed-off-by: Simon Horman <horms@verge.net.au>
>> >
>> > ---
>> >
>> > Dependencies: "mmc: tmio: Share register access functions"
>> >
>> > v2:
>> > * Include linux/delay.h instead of asm/delay.h
>> > * Skip write if sh_mobile_sdhi_wait_idle() times out
>> >  - The bus will probably be in an inconsistent state and writing
>> >    may lock up the bus
>> > * Only set hook if TMIO_MMC_HAS_IDLE_WAIT is set in platform data
>> >  rather than checking for TMIO_MMC_HAS_IDLE_WAIT each time the
>> >  hook is called.
>> > ---
>>
>> Thanks Simon, this version looks much better!
>>
>> > index 5a90266..0dc9804 100644
>> > --- a/include/linux/mfd/tmio.h
>> > +++ b/include/linux/mfd/tmio.h
>> > @@ -94,6 +101,7 @@ struct tmio_mmc_data {
>> >        void (*set_pwr)(struct platform_device *host, int state);
>> >        void (*set_clk_div)(struct platform_device *host, int state);
>> >        int (*get_cd)(struct platform_device *host);
>> > +       int (*write16_hook)(struct tmio_mmc_host *host, int addr);
>> >  };
>> >
>> >  static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
>>
>> What's the reason behind passing "struct tmio_mmc_host *"  as an
>> argument to the new hook? Performance? All other callbacks seem to
>> take a "struct platform_device *", so being consistent here may be
>> good unless it comes with too much overhead.
>
> The reason is that
> 1) The hook is called from sd_ctrl_write16 which takes
>   struct tmio_mmc_host * as its first argument and;
> 2) The hook that has been implemented calls sd_ctrl_read16() which takes a
>   struct tmio_mmc_host * as its first argument.
> So it seemed logical to pass that down.
>
> In the caes of 1) we can get the struct platform_device * using host->pdev.
> However, in the case of 2) is it less clear to me how we can get the
> struct tmio_mmc_host * from a struct platform_device *.

Have a look at the code in tmio_mmc_host_suspend() for some code that
does struct device * -> struct tmio_mmc_host *:
int tmio_mmc_host_suspend(struct device *dev)
{
	struct mmc_host *mmc = dev_get_drvdata(dev);
	struct tmio_mmc_host *host = mmc_priv(mmc);

You can easily change the dev_get_drvdata() to platform_get_drvdata(),
see include/linux/platform_device.h

I guess a similar conversion can be done in tmio_mmc_enable_dma() to
move from writew() to sd_ctrl_write16()?

Cheers,

/ magnus

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Simon Horman]

On Tue, Jun 21, 2011 at 09:59:37AM +0900, Magnus Damm wrote:
> On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote:
> >> On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman <horms@verge.net.au> wrote:

[snip]

> >> > index 5a90266..0dc9804 100644
> >> > --- a/include/linux/mfd/tmio.h
> >> > +++ b/include/linux/mfd/tmio.h
> >> > @@ -94,6 +101,7 @@ struct tmio_mmc_data {
> >> >        void (*set_pwr)(struct platform_device *host, int state);
> >> >        void (*set_clk_div)(struct platform_device *host, int state);
> >> >        int (*get_cd)(struct platform_device *host);
> >> > +       int (*write16_hook)(struct tmio_mmc_host *host, int addr);
> >> >  };
> >> >
> >> >  static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
> >>
> >> What's the reason behind passing "struct tmio_mmc_host *"  as an
> >> argument to the new hook? Performance? All other callbacks seem to
> >> take a "struct platform_device *", so being consistent here may be
> >> good unless it comes with too much overhead.
> >
> > The reason is that
> > 1) The hook is called from sd_ctrl_write16 which takes
> >   struct tmio_mmc_host * as its first argument and;
> > 2) The hook that has been implemented calls sd_ctrl_read16() which takes a
> >   struct tmio_mmc_host * as its first argument.
> > So it seemed logical to pass that down.
> >
> > In the caes of 1) we can get the struct platform_device * using host->pdev.
> > However, in the case of 2) is it less clear to me how we can get the
> > struct tmio_mmc_host * from a struct platform_device *.
> 
> Have a look at the code in tmio_mmc_host_suspend() for some code that
> does struct device * -> struct tmio_mmc_host *:
> int tmio_mmc_host_suspend(struct device *dev)
> {
> 	struct mmc_host *mmc = dev_get_drvdata(dev);
> 	struct tmio_mmc_host *host = mmc_priv(mmc);
> 
> You can easily change the dev_get_drvdata() to platform_get_drvdata(),
> see include/linux/platform_device.h

Thanks, I'm happy to make that change if you think it is worth it.
(I will need to re-test on AG5, which I could do this afternoon
 if it is free)

> I guess a similar conversion can be done in tmio_mmc_enable_dma() to
> move from writew() to sd_ctrl_write16()?

Are you proposing changing tmio_mmc_enable_dma() to take
a struct platform_device * as its first argument?

tmio_mmc_enable_dma() is already altered in one of the
patches in this series to use sd_ctrl_write16() without
altering the arguments taht tmio_mmc_enable_dma() takes.

static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
{
#if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)
	/* Switch DMA mode on or off - SuperH specific? */
	sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE);
#endif
}

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Magnus Damm]

Hi Simon,

On Tue, Jun 21, 2011 at 10:13 AM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Jun 21, 2011 at 09:59:37AM +0900, Magnus Damm wrote:
>> On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman <horms@verge.net.au> wrote:
>> > On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote:
>> >> On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman <horms@verge.net.au> wrote:
>
> [snip]
>
>> >> > index 5a90266..0dc9804 100644
>> >> > --- a/include/linux/mfd/tmio.h
>> >> > +++ b/include/linux/mfd/tmio.h
>> >> > @@ -94,6 +101,7 @@ struct tmio_mmc_data {
>> >> >        void (*set_pwr)(struct platform_device *host, int state);
>> >> >        void (*set_clk_div)(struct platform_device *host, int state);
>> >> >        int (*get_cd)(struct platform_device *host);
>> >> > +       int (*write16_hook)(struct tmio_mmc_host *host, int addr);
>> >> >  };
>> >> >
>> >> >  static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
>> >>
>> >> What's the reason behind passing "struct tmio_mmc_host *"  as an
>> >> argument to the new hook? Performance? All other callbacks seem to
>> >> take a "struct platform_device *", so being consistent here may be
>> >> good unless it comes with too much overhead.
>> >
>> > The reason is that
>> > 1) The hook is called from sd_ctrl_write16 which takes
>> >   struct tmio_mmc_host * as its first argument and;
>> > 2) The hook that has been implemented calls sd_ctrl_read16() which takes a
>> >   struct tmio_mmc_host * as its first argument.
>> > So it seemed logical to pass that down.
>> >
>> > In the caes of 1) we can get the struct platform_device * using host->pdev.
>> > However, in the case of 2) is it less clear to me how we can get the
>> > struct tmio_mmc_host * from a struct platform_device *.
>>
>> Have a look at the code in tmio_mmc_host_suspend() for some code that
>> does struct device * -> struct tmio_mmc_host *:
>> int tmio_mmc_host_suspend(struct device *dev)
>> {
>>       struct mmc_host *mmc = dev_get_drvdata(dev);
>>       struct tmio_mmc_host *host = mmc_priv(mmc);
>>
>> You can easily change the dev_get_drvdata() to platform_get_drvdata(),
>> see include/linux/platform_device.h
>
> Thanks, I'm happy to make that change if you think it is worth it.
> (I will need to re-test on AG5, which I could do this afternoon
>  if it is free)

Hm, perhaps it can be done with incremental patches in the future?

I think it's good to be consistent and use the same argument passing
style as other callbacks, but at the same time I'm not 100% sure if
passing a platform data pointer is the best approach. It probably made
sense with the old tmio_mmc driver that only hooked up to MFD, but I'm
not sure if that's the case anymore. I'm sure there is room for plenty
of cleanups - but exactly what to do I don't know. =)

At least passing a struct tmio_mmc_host * requires little conversion
which should add minimal overhead.

>> I guess a similar conversion can be done in tmio_mmc_enable_dma() to
>> move from writew() to sd_ctrl_write16()?
>
> Are you proposing changing tmio_mmc_enable_dma() to take
> a struct platform_device * as its first argument?

No, not at all. I just recall someone pointing out that
tmio_mmc_enable_dma() skipped the tmio_mmc specific I/O routines and
used writew() directly. I suspected the reason behind this was the
difficulty of converting between different pointer types, but that may
not be true.

> tmio_mmc_enable_dma() is already altered in one of the
> patches in this series to use sd_ctrl_write16() without
> altering the arguments taht tmio_mmc_enable_dma() takes.

Ok, that's good.

> static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
> {
> #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)
>        /* Switch DMA mode on or off - SuperH specific? */
>        sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE);
> #endif
> }

Hm, perhaps it's my mail setup that's the issue, but the version of
"[PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE" that I'm looking
at is still using writew().

Cheers,

/ magnus

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Simon Horman]

On Tue, Jun 21, 2011 at 10:36:05AM +0900, Magnus Damm wrote:
> Hi Simon,
> 
> On Tue, Jun 21, 2011 at 10:13 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, Jun 21, 2011 at 09:59:37AM +0900, Magnus Damm wrote:
> >> On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote:
> >> >> On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman <horms@verge.net.au> wrote:
> >
> > [snip]
> >
> >> >> > index 5a90266..0dc9804 100644
> >> >> > --- a/include/linux/mfd/tmio.h
> >> >> > +++ b/include/linux/mfd/tmio.h
> >> >> > @@ -94,6 +101,7 @@ struct tmio_mmc_data {
> >> >> >        void (*set_pwr)(struct platform_device *host, int state);
> >> >> >        void (*set_clk_div)(struct platform_device *host, int state);
> >> >> >        int (*get_cd)(struct platform_device *host);
> >> >> > +       int (*write16_hook)(struct tmio_mmc_host *host, int addr);
> >> >> >  };
> >> >> >
> >> >> >  static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
> >> >>
> >> >> What's the reason behind passing "struct tmio_mmc_host *"  as an
> >> >> argument to the new hook? Performance? All other callbacks seem to
> >> >> take a "struct platform_device *", so being consistent here may be
> >> >> good unless it comes with too much overhead.
> >> >
> >> > The reason is that
> >> > 1) The hook is called from sd_ctrl_write16 which takes
> >> >   struct tmio_mmc_host * as its first argument and;
> >> > 2) The hook that has been implemented calls sd_ctrl_read16() which takes a
> >> >   struct tmio_mmc_host * as its first argument.
> >> > So it seemed logical to pass that down.
> >> >
> >> > In the caes of 1) we can get the struct platform_device * using host->pdev.
> >> > However, in the case of 2) is it less clear to me how we can get the
> >> > struct tmio_mmc_host * from a struct platform_device *.
> >>
> >> Have a look at the code in tmio_mmc_host_suspend() for some code that
> >> does struct device * -> struct tmio_mmc_host *:
> >> int tmio_mmc_host_suspend(struct device *dev)
> >> {
> >>       struct mmc_host *mmc = dev_get_drvdata(dev);
> >>       struct tmio_mmc_host *host = mmc_priv(mmc);
> >>
> >> You can easily change the dev_get_drvdata() to platform_get_drvdata(),
> >> see include/linux/platform_device.h
> >
> > Thanks, I'm happy to make that change if you think it is worth it.
> > (I will need to re-test on AG5, which I could do this afternoon
> >  if it is free)
> 
> Hm, perhaps it can be done with incremental patches in the future?
> 
> I think it's good to be consistent and use the same argument passing
> style as other callbacks, but at the same time I'm not 100% sure if
> passing a platform data pointer is the best approach. It probably made
> sense with the old tmio_mmc driver that only hooked up to MFD, but I'm
> not sure if that's the case anymore. I'm sure there is room for plenty
> of cleanups - but exactly what to do I don't know. =)
> 
> At least passing a struct tmio_mmc_host * requires little conversion
> which should add minimal overhead.

Ok, lets stick with what we have for now.
Its a fairly trivial change to update the arguments later
if that is wanted. Testing is slightly less trivial
due to availability of hardware, but not a major problem.

Can you Acked-by, or Reviewed-by the patches in this series if
you are now happy with them?

> >> I guess a similar conversion can be done in tmio_mmc_enable_dma() to
> >> move from writew() to sd_ctrl_write16()?
> >
> > Are you proposing changing tmio_mmc_enable_dma() to take
> > a struct platform_device * as its first argument?
> 
> No, not at all. I just recall someone pointing out that
> tmio_mmc_enable_dma() skipped the tmio_mmc specific I/O routines and
> used writew() directly. I suspected the reason behind this was the
> difficulty of converting between different pointer types, but that may
> not be true.

I was probably the person who pointed that out to you.
I'm unsure of the reason, but at least in its current form
it appears not to be related to the parameters involved.

> > tmio_mmc_enable_dma() is already altered in one of the
> > patches in this series to use sd_ctrl_write16() without
> > altering the arguments taht tmio_mmc_enable_dma() takes.
> 
> Ok, that's good.
> 
> > static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
> > {
> > #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)
> >        /* Switch DMA mode on or off - SuperH specific? */
> >        sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE);
> > #endif
> > }
> 
> Hm, perhaps it's my mail setup that's the issue, but the version of
> "[PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE" that I'm looking
> at is still using writew().

The writew() -> sd_ctrl_write16() change is made by the following patch
in the series, "[PATCH 2/5] mmc: tmio: Share register access functions".

The last hunk looks like this:

diff --git a/drivers/mmc/host/tmio_mmc_dma.c
b/drivers/mmc/host/tmio_mmc_dma.c
index 9c4da66..f24a029 100644
--- a/drivers/mmc/host/tmio_mmc_dma.c
+++ b/drivers/mmc/host/tmio_mmc_dma.c
@@ -26,7 +26,7 @@ static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
 {
 #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)
 	/* Switch DMA mode on or off - SuperH specific? */
-	writew(enable ? 2 : 0, host->ctl + (CTL_DMA_ENABLE << host->bus_shift));
+	sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE);
 #endif
 }

Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

[Magnus Damm]

Hey Simon,

On Tue, Jun 21, 2011 at 11:09 AM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Jun 21, 2011 at 10:36:05AM +0900, Magnus Damm wrote:
>> On Tue, Jun 21, 2011 at 10:13 AM, Simon Horman <horms@verge.net.au> wrote:
>> > On Tue, Jun 21, 2011 at 09:59:37AM +0900, Magnus Damm wrote:
>> >> On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman <horms@verge.net.au> wrote:
>> >> > On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote:
>> >> >> On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman <horms@verge.net.au> wrote:
>> >
>> > [snip]
>> >
>> >> >> > index 5a90266..0dc9804 100644
>> >> >> > --- a/include/linux/mfd/tmio.h
>> >> >> > +++ b/include/linux/mfd/tmio.h
>> >> >> > @@ -94,6 +101,7 @@ struct tmio_mmc_data {
>> >> >> >        void (*set_pwr)(struct platform_device *host, int state);
>> >> >> >        void (*set_clk_div)(struct platform_device *host, int state);
>> >> >> >        int (*get_cd)(struct platform_device *host);
>> >> >> > +       int (*write16_hook)(struct tmio_mmc_host *host, int addr);
>> >> >> >  };
>> >> >> >
>> >> >> >  static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
>> >> >>
>> >> >> What's the reason behind passing "struct tmio_mmc_host *"  as an
>> >> >> argument to the new hook? Performance? All other callbacks seem to
>> >> >> take a "struct platform_device *", so being consistent here may be
>> >> >> good unless it comes with too much overhead.
>> >> >
>> >> > The reason is that
>> >> > 1) The hook is called from sd_ctrl_write16 which takes
>> >> >   struct tmio_mmc_host * as its first argument and;
>> >> > 2) The hook that has been implemented calls sd_ctrl_read16() which takes a
>> >> >   struct tmio_mmc_host * as its first argument.
>> >> > So it seemed logical to pass that down.
>> >> >
>> >> > In the caes of 1) we can get the struct platform_device * using host->pdev.
>> >> > However, in the case of 2) is it less clear to me how we can get the
>> >> > struct tmio_mmc_host * from a struct platform_device *.
>> >>
>> >> Have a look at the code in tmio_mmc_host_suspend() for some code that
>> >> does struct device * -> struct tmio_mmc_host *:
>> >> int tmio_mmc_host_suspend(struct device *dev)
>> >> {
>> >>       struct mmc_host *mmc = dev_get_drvdata(dev);
>> >>       struct tmio_mmc_host *host = mmc_priv(mmc);
>> >>
>> >> You can easily change the dev_get_drvdata() to platform_get_drvdata(),
>> >> see include/linux/platform_device.h
>> >
>> > Thanks, I'm happy to make that change if you think it is worth it.
>> > (I will need to re-test on AG5, which I could do this afternoon
>> >  if it is free)
>>
>> Hm, perhaps it can be done with incremental patches in the future?
>>
>> I think it's good to be consistent and use the same argument passing
>> style as other callbacks, but at the same time I'm not 100% sure if
>> passing a platform data pointer is the best approach. It probably made
>> sense with the old tmio_mmc driver that only hooked up to MFD, but I'm
>> not sure if that's the case anymore. I'm sure there is room for plenty
>> of cleanups - but exactly what to do I don't know. =)
>>
>> At least passing a struct tmio_mmc_host * requires little conversion
>> which should add minimal overhead.
>
> Ok, lets stick with what we have for now.
> Its a fairly trivial change to update the arguments later
> if that is wanted. Testing is slightly less trivial
> due to availability of hardware, but not a major problem.
>
> Can you Acked-by, or Reviewed-by the patches in this series if
> you are now happy with them?

Sure, for all patches included in this series:
Acked-by: Magnus Damm <damm@opensource.se>

>> >> I guess a similar conversion can be done in tmio_mmc_enable_dma() to
>> >> move from writew() to sd_ctrl_write16()?
>> >
>> > Are you proposing changing tmio_mmc_enable_dma() to take
>> > a struct platform_device * as its first argument?
>>
>> No, not at all. I just recall someone pointing out that
>> tmio_mmc_enable_dma() skipped the tmio_mmc specific I/O routines and
>> used writew() directly. I suspected the reason behind this was the
>> difficulty of converting between different pointer types, but that may
>> not be true.
>
> I was probably the person who pointed that out to you.
> I'm unsure of the reason, but at least in its current form
> it appears not to be related to the parameters involved.

Yeah. Probably me being confused of which patches that modify which
functions. =)

>> > tmio_mmc_enable_dma() is already altered in one of the
>> > patches in this series to use sd_ctrl_write16() without
>> > altering the arguments taht tmio_mmc_enable_dma() takes.
>>
>> Ok, that's good.
>>
>> > static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
>> > {
>> > #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)
>> >        /* Switch DMA mode on or off - SuperH specific? */
>> >        sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE);
>> > #endif
>> > }
>>
>> Hm, perhaps it's my mail setup that's the issue, but the version of
>> "[PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE" that I'm looking
>> at is still using writew().
>
> The writew() -> sd_ctrl_write16() change is made by the following patch
> in the series, "[PATCH 2/5] mmc: tmio: Share register access functions".
>
> The last hunk looks like this:
>
> diff --git a/drivers/mmc/host/tmio_mmc_dma.c
> b/drivers/mmc/host/tmio_mmc_dma.c
> index 9c4da66..f24a029 100644
> --- a/drivers/mmc/host/tmio_mmc_dma.c
> +++ b/drivers/mmc/host/tmio_mmc_dma.c
> @@ -26,7 +26,7 @@ static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
>  {
>  #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)
>        /* Switch DMA mode on or off - SuperH specific? */
> -       writew(enable ? 2 : 0, host->ctl + (CTL_DMA_ENABLE << host->bus_shift));
> +       sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE);
>  #endif
>  }

Gotcha. Looking good, thank you!

/ magnus