[PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Magnus Damm]

tmio_mmc: Minor fixes and cnf/irq changes

These patches fix two trivial issues and updates the tmio_mmc
driver to support hardware configurations that lack the cnf io
area (only a single ctl area) but may use multiple interrupts.

[PATCH 01/05] tmio_mmc: Fix one off, use resource_size() in probe()
[PATCH 02/05] tmio_mmc: Fix use after free in remove()
[PATCH 03/05] tmio_mmc: Break out cnf area operations
[PATCH 04/05] tmio_mmc: Make cnf area optional
[PATCH 05/05] tmio_mmc: Support multiple interrupts

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/mmc/host/tmio_mmc.c |  156 ++++++++++++++++++++++++++++++-------------
 drivers/mmc/host/tmio_mmc.h |    1 
 2 files changed, 109 insertions(+), 48 deletions(-)

[PATCH 01/05] tmio_mmc: Fix one off, use resource_size() in probe()

[Magnus Damm]

From:  Magnus Damm <damm@opensource.se>

Update the tmio_mmc code to use resource_size(). With this
patch applied the correct resource size is passed to ioremap().

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/mmc/host/tmio_mmc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- 0001/drivers/mmc/host/tmio_mmc.c
+++ work/drivers/mmc/host/tmio_mmc.c	2009-01-29 16:08:04.000000000 +0900
@@ -568,11 +568,11 @@ static int __devinit tmio_mmc_probe(stru
 	host->mmc = mmc;
 	platform_set_drvdata(dev, mmc);
 
-	host->ctl = ioremap(res_ctl->start, res_ctl->end - res_ctl->start);
+	host->ctl = ioremap(res_ctl->start, resource_size(res_ctl));
 	if (!host->ctl)
 		goto host_free;
 
-	host->cnf = ioremap(res_cnf->start, res_cnf->end - res_cnf->start);
+	host->cnf = ioremap(res_cnf->start, resource_size(res_cnf));
 	if (!host->cnf)
 		goto unmap_ctl;
 

[PATCH 02/05] tmio_mmc: Fix use after free in remove()

[Magnus Damm]

From:  Magnus Damm <damm@opensource.se>

Update the tmio_mmc code to call mmc_free_host() when
done using the private data. Without this fix the driver
frees memory and then keeps on using it as private data.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/mmc/host/tmio_mmc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 0010/drivers/mmc/host/tmio_mmc.c
+++ work/drivers/mmc/host/tmio_mmc.c	2009-03-11 19:24:03.000000000 +0900
@@ -650,10 +650,10 @@ static int __devexit tmio_mmc_remove(str
 	if (mmc) {
 		struct tmio_mmc_host *host = mmc_priv(mmc);
 		mmc_remove_host(mmc);
-		mmc_free_host(mmc);
 		free_irq(host->irq, host);
 		iounmap(host->ctl);
 		iounmap(host->cnf);
+		mmc_free_host(mmc);
 	}
 
 	return 0;

[PATCH 03/05] tmio_mmc: Break out cnf area operations

[Magnus Damm]

From:  Magnus Damm <damm@opensource.se>

Break out tmio_mmc io operations for the cnf area.
This moves similar register operations into one place.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/mmc/host/tmio_mmc.c |   59 ++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 20 deletions(-)

--- 0011/drivers/mmc/host/tmio_mmc.c
+++ work/drivers/mmc/host/tmio_mmc.c	2009-03-11 19:31:58.000000000 +0900
@@ -35,6 +35,36 @@
 
 #include "tmio_mmc.h"
 
+static void tmio_mmc_cnf_power(struct tmio_mmc_host *host, int on)
+{
+	tmio_iowrite8(on ? 0x02 : 0x00, host->cnf + CNF_PWR_CTL_2);
+}
+
+static void tmio_mmc_cnf_setup_regs(struct tmio_mmc_host *host,
+				    struct platform_device *dev)
+{
+	/* Enable the MMC/SD Control registers */
+	tmio_iowrite16(SDCREN, host->cnf + CNF_CMD);
+	tmio_iowrite32(dev->resource[0].start & 0xfffe,
+		       host->cnf + CNF_CTL_BASE);
+}
+
+static void tmio_mmc_cnf_setup_clock(struct tmio_mmc_host *host,
+				     int divide_by_one)
+{
+	tmio_iowrite8(divide_by_one ? 0 : 1,
+		      host->cnf + CNF_SD_CLK_MODE);
+}
+
+static void tmio_mmc_cnf_setup_late(struct tmio_mmc_host *host)
+{
+	/* Disable SD power during suspend */
+	tmio_iowrite8(0x01, host->cnf + CNF_PWR_CTL_3);
+
+	/* The below is required but why? FIXME */
+	tmio_iowrite8(0x1f, host->cnf + CNF_STOP_CLK_CTL);
+}
+
 /*
  * Fixme - documentation conflicts on what the clock values are for the
  * various dividers.
@@ -46,7 +76,6 @@
 
 static void tmio_mmc_set_clock(struct tmio_mmc_host *host, int new_clock)
 {
-	void __iomem *cnf = host->cnf;
 	void __iomem *ctl = host->ctl;
 	u32 clk = 0, clock;
 
@@ -59,7 +88,8 @@ static void tmio_mmc_set_clock(struct tm
 			clk = 0x20000;
 
 		clk >>= 2;
-		tmio_iowrite8((clk & 0x8000) ? 0 : 1, cnf + CNF_SD_CLK_MODE);
+
+		tmio_mmc_cnf_setup_clock(host, clk & 0x8000);
 		clk |= 0x100;
 	}
 
@@ -449,7 +479,6 @@ fail:
 static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
-	void __iomem *cnf = host->cnf;
 	void __iomem *ctl = host->ctl;
 
 	if (ios->clock)
@@ -458,12 +487,11 @@ static void tmio_mmc_set_ios(struct mmc_
 	/* Power sequence - OFF -> ON -> UP */
 	switch (ios->power_mode) {
 	case MMC_POWER_OFF: /* power down SD bus */
-		tmio_iowrite8(0x00, cnf + CNF_PWR_CTL_2);
+		tmio_mmc_cnf_power(host, 0);
 		tmio_mmc_clk_stop(host);
 		break;
 	case MMC_POWER_ON: /* power up SD bus */
-
-		tmio_iowrite8(0x02, cnf + CNF_PWR_CTL_2);
+		tmio_mmc_cnf_power(host, 1);
 		break;
 	case MMC_POWER_UP: /* start bus clock */
 		tmio_mmc_clk_start(host);
@@ -518,12 +546,10 @@ static int tmio_mmc_resume(struct platfo
 	struct mfd_cell	*cell = (struct mfd_cell *)dev->dev.platform_data;
 	struct mmc_host *mmc = platform_get_drvdata(dev);
 	struct tmio_mmc_host *host = mmc_priv(mmc);
-	void __iomem *cnf = host->cnf;
 	int ret = 0;
 
 	/* Enable the MMC/SD Control registers */
-	tmio_iowrite16(SDCREN, cnf + CNF_CMD);
-	tmio_iowrite32(dev->resource[0].start & 0xfffe, cnf + CNF_CTL_BASE);
+	tmio_mmc_cnf_setup_regs(host, dev);
 
 	/* Tell the MFD core we are ready to be enabled */
 	if (cell->enable) {
@@ -583,9 +609,7 @@ static int __devinit tmio_mmc_probe(stru
 	mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
 
 	/* Enable the MMC/SD Control registers */
-	tmio_iowrite16(SDCREN, host->cnf + CNF_CMD);
-	tmio_iowrite32(dev->resource[0].start & 0xfffe,
-		host->cnf + CNF_CTL_BASE);
+	tmio_mmc_cnf_setup_regs(host, dev);
 
 	/* Tell the MFD core we are ready to be enabled */
 	if (cell->enable) {
@@ -594,15 +618,10 @@ static int __devinit tmio_mmc_probe(stru
 			goto unmap_cnf;
 	}
 
-	/* Disable SD power during suspend */
-	tmio_iowrite8(0x01, host->cnf + CNF_PWR_CTL_3);
-
-	/* The below is required but why? FIXME */
-	tmio_iowrite8(0x1f, host->cnf + CNF_STOP_CLK_CTL);
-
-	/* Power down SD bus*/
-	tmio_iowrite8(0x0,  host->cnf + CNF_PWR_CTL_2);
+	tmio_mmc_cnf_setup_late(host);
 
+	/* Power down SD bus */
+	tmio_mmc_cnf_power(host, 0);
 	tmio_mmc_clk_stop(host);
 	reset(host);
 

[PATCH 04/05] tmio_mmc: Make cnf area optional

[Magnus Damm]

From:  Magnus Damm <damm@opensource.se>

Add support for tmio_mmc hardware configurations that
lack the cnf io area. With this patch such hardware
can pass a single ctl io area with the platform data.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/mmc/host/tmio_mmc.c |   33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

--- 0012/drivers/mmc/host/tmio_mmc.c
+++ work/drivers/mmc/host/tmio_mmc.c	2009-03-11 19:35:46.000000000 +0900
@@ -37,12 +37,18 @@
 
 static void tmio_mmc_cnf_power(struct tmio_mmc_host *host, int on)
 {
+	if (!host->cnf)
+		return;
+
 	tmio_iowrite8(on ? 0x02 : 0x00, host->cnf + CNF_PWR_CTL_2);
 }
 
 static void tmio_mmc_cnf_setup_regs(struct tmio_mmc_host *host,
 				    struct platform_device *dev)
 {
+	if (!host->cnf)
+		return;
+
 	/* Enable the MMC/SD Control registers */
 	tmio_iowrite16(SDCREN, host->cnf + CNF_CMD);
 	tmio_iowrite32(dev->resource[0].start & 0xfffe,
@@ -52,12 +58,18 @@ static void tmio_mmc_cnf_setup_regs(stru
 static void tmio_mmc_cnf_setup_clock(struct tmio_mmc_host *host,
 				     int divide_by_one)
 {
+	if (!host->cnf)
+		return;
+
 	tmio_iowrite8(divide_by_one ? 0 : 1,
 		      host->cnf + CNF_SD_CLK_MODE);
 }
 
 static void tmio_mmc_cnf_setup_late(struct tmio_mmc_host *host)
 {
+	if (!host->cnf)
+		return;
+
 	/* Disable SD power during suspend */
 	tmio_iowrite8(0x01, host->cnf + CNF_PWR_CTL_3);
 
@@ -576,12 +588,8 @@ static int __devinit tmio_mmc_probe(stru
 	struct mmc_host *mmc;
 	int ret = -ENOMEM;
 
-	if (dev->num_resources != 3)
-		goto out;
-
 	res_ctl = platform_get_resource(dev, IORESOURCE_MEM, 0);
-	res_cnf = platform_get_resource(dev, IORESOURCE_MEM, 1);
-	if (!res_ctl || !res_cnf) {
+	if (!res_ctl) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -598,9 +606,12 @@ static int __devinit tmio_mmc_probe(stru
 	if (!host->ctl)
 		goto host_free;
 
-	host->cnf = ioremap(res_cnf->start, resource_size(res_cnf));
-	if (!host->cnf)
-		goto unmap_ctl;
+	res_cnf = platform_get_resource(dev, IORESOURCE_MEM, 1);
+	if (res_cnf) {
+		host->cnf = ioremap(res_cnf->start, resource_size(res_cnf));
+		if (!host->cnf)
+			goto unmap_ctl;
+	}
 
 	mmc->ops = &tmio_mmc_ops;
 	mmc->caps = MMC_CAP_4_BIT_DATA;
@@ -651,7 +662,8 @@ static int __devinit tmio_mmc_probe(stru
 	return 0;
 
 unmap_cnf:
-	iounmap(host->cnf);
+	if (host->cnf)
+		iounmap(host->cnf);
 unmap_ctl:
 	iounmap(host->ctl);
 host_free:
@@ -670,8 +682,9 @@ static int __devexit tmio_mmc_remove(str
 		struct tmio_mmc_host *host = mmc_priv(mmc);
 		mmc_remove_host(mmc);
 		free_irq(host->irq, host);
+		if (host->cnf)
+			iounmap(host->cnf);
 		iounmap(host->ctl);
-		iounmap(host->cnf);
 		mmc_free_host(mmc);
 	}
 

[PATCH 05/05] tmio_mmc: Support multiple interrupts

[Magnus Damm]

From:  Magnus Damm <damm@opensource.se>

Add support for tmio_mmc hardware configurations with
multiple interrupts. With this patch applied all interrupts
passed as platform data will be used by the driver.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/mmc/host/tmio_mmc.c |   58 ++++++++++++++++++++++++++++++++-----------
 drivers/mmc/host/tmio_mmc.h |    1 
 2 files changed, 44 insertions(+), 15 deletions(-)

--- 0013/drivers/mmc/host/tmio_mmc.c
+++ work/drivers/mmc/host/tmio_mmc.c	2009-03-11 19:38:59.000000000 +0900
@@ -537,6 +537,45 @@ static struct mmc_host_ops tmio_mmc_ops 
 	.get_ro         = tmio_mmc_get_ro,
 };
 
+static int tmio_mmc_hook_irqs(struct platform_device *dev, int hook)
+{
+	struct mmc_host *mmc = platform_get_drvdata(dev);
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+	struct resource *res;
+	int ret = -ENXIO;
+	int q, m;
+	int k = 0;
+	int n = 0;
+
+	while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
+		for (n = res->start; hook && n <= res->end; n++) {
+			if (request_irq(n, tmio_mmc_irq, IRQF_DISABLED,
+					dev_name(&dev->dev), host))
+				goto rollback;
+
+			set_irq_type(n, IRQ_TYPE_EDGE_FALLING);
+		}
+		k++;
+	}
+
+	if (hook)
+		return k > 0 ? 0 : -ENOENT;
+
+	k--;
+	ret = 0;
+
+ rollback:
+	for (q = k; k >= 0; k--) {
+		for (m = n; m >= res->start; m--)
+			free_irq(m, host);
+
+		res = platform_get_resource(dev, IORESOURCE_IRQ, k - 1);
+		m = res->end;
+	}
+
+	return ret;
+}
+
 #ifdef CONFIG_PM
 static int tmio_mmc_suspend(struct platform_device *dev, pm_message_t state)
 {
@@ -636,25 +675,16 @@ static int __devinit tmio_mmc_probe(stru
 	tmio_mmc_clk_stop(host);
 	reset(host);
 
-	ret = platform_get_irq(dev, 0);
-	if (ret >= 0)
-		host->irq = ret;
-	else
-		goto unmap_cnf;
-
-	disable_mmc_irqs(host->ctl, TMIO_MASK_ALL);
-
-	ret = request_irq(host->irq, tmio_mmc_irq, IRQF_DISABLED, "tmio-mmc",
-		host);
+	ret = tmio_mmc_hook_irqs(dev, 1);
 	if (ret)
 		goto unmap_cnf;
 
-	set_irq_type(host->irq, IRQ_TYPE_EDGE_FALLING);
+	disable_mmc_irqs(host->ctl, TMIO_MASK_ALL);
 
 	mmc_add_host(mmc);
 
-	printk(KERN_INFO "%s at 0x%08lx irq %d\n", mmc_hostname(host->mmc),
-	       (unsigned long)host->ctl, host->irq);
+	printk(KERN_INFO "%s at 0x%08lx\n", mmc_hostname(host->mmc),
+	       (unsigned long)host->ctl);
 
 	/* Unmask the IRQs we want to know about */
 	enable_mmc_irqs(host->ctl,  TMIO_MASK_IRQ);
@@ -681,7 +711,7 @@ static int __devexit tmio_mmc_remove(str
 	if (mmc) {
 		struct tmio_mmc_host *host = mmc_priv(mmc);
 		mmc_remove_host(mmc);
-		free_irq(host->irq, host);
+		tmio_mmc_hook_irqs(dev, 0);
 		if (host->cnf)
 			iounmap(host->cnf);
 		iounmap(host->ctl);
--- 0001/drivers/mmc/host/tmio_mmc.h
+++ work/drivers/mmc/host/tmio_mmc.h	2009-03-11 19:38:03.000000000 +0900
@@ -112,7 +112,6 @@ struct tmio_mmc_host {
 	struct mmc_request      *mrq;
 	struct mmc_data         *data;
 	struct mmc_host         *mmc;
-	int                     irq;
 
 	/* pio related stuff */
 	struct scatterlist      *sg_ptr;

Re: [PATCH 05/05] tmio_mmc: Support multiple interrupts

[Ian Molton]

Magnus Damm wrote:
> From:  Magnus Damm <damm@opensource.se>
> 
> Add support for tmio_mmc hardware configurations with
> multiple interrupts. With this patch applied all interrupts
> passed as platform data will be used by the driver.

What is this patch intended to support?

Re: [PATCH 01/05] tmio_mmc: Fix one off, use resource_size() in probe()

[Ian Molton]

Magnus Damm wrote:
> From:  Magnus Damm <damm@opensource.se>
> 
> Update the tmio_mmc code to use resource_size(). With this
> patch applied the correct resource size is passed to ioremap().

Acked.

Re: [PATCH 02/05] tmio_mmc: Fix use after free in remove()

[Ian Molton]

Magnus Damm wrote:
> From:  Magnus Damm <damm@opensource.se>
> 
> Update the tmio_mmc code to call mmc_free_host() when
> done using the private data. Without this fix the driver
> frees memory and then keeps on using it as private data.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>

Acked-by: Ian Molton <ian@mnementh.co.uk>

> ---
> 
>  drivers/mmc/host/tmio_mmc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- 0010/drivers/mmc/host/tmio_mmc.c
> +++ work/drivers/mmc/host/tmio_mmc.c	2009-03-11 19:24:03.000000000 +0900
> @@ -650,10 +650,10 @@ static int __devexit tmio_mmc_remove(str
>  	if (mmc) {
>  		struct tmio_mmc_host *host = mmc_priv(mmc);
>  		mmc_remove_host(mmc);
> -		mmc_free_host(mmc);
>  		free_irq(host->irq, host);
>  		iounmap(host->ctl);
>  		iounmap(host->cnf);
> +		mmc_free_host(mmc);
>  	}
>  
>  	return 0;
> 

Re: [PATCH 03/05] tmio_mmc: Break out cnf area operations

[Ian Molton]

Magnus Damm wrote:
> From:  Magnus Damm <damm@opensource.se>
> 
> Break out tmio_mmc io operations for the cnf area.
> This moves similar register operations into one place.

Not sure about this one yet. AFAICT this totally disables the HBA power 
control.

I'm wondering if this is a common problem.

It seems to me  that its likely that clock and power control are often 
external to the host controller chip. (not just on TMIO MMC).

I think it'd be a good idea if we came up with an infrastructre that 
allowed a set of clock / power control callbacks into the platform code.

Whats your use case for this? samee applies to the related cnf patch too.

Re: [PATCH 05/05] tmio_mmc: Support multiple interrupts

[Magnus Damm]

On Wed, Mar 11, 2009 at 11:21 PM, Ian Molton <ian@mnementh.co.uk> wrote:
> Magnus Damm wrote:
>>
>> From:  Magnus Damm <damm@opensource.se>
>>
>> Add support for tmio_mmc hardware configurations with
>> multiple interrupts. With this patch applied all interrupts
>> passed as platform data will be used by the driver.
>
> What is this patch intended to support?

I'll make sure you get CC:ed on the platform data patch later on. =)

The hardware has up to four interrupts assigned to the mmc block.
Since I have no documentation at all I'm not sure which interrupt line
is used for what, but after a bit more testing this morning it seems
like one of 4 is enough to get this driver working. I guess the other
interrupts are used for hotplug.

So please ignore this patch for now. Thanks.

/ magnus

Re: [PATCH 03/05] tmio_mmc: Break out cnf area operations

[Magnus Damm]

On Wed, Mar 11, 2009 at 11:39 PM, Ian Molton <ian@mnementh.co.uk> wrote:
> Magnus Damm wrote:
>>
>> From:  Magnus Damm <damm@opensource.se>
>>
>> Break out tmio_mmc io operations for the cnf area.
>> This moves similar register operations into one place.
>
> Not sure about this one yet. AFAICT this totally disables the HBA power
> control.

Hm, my plan was not to introduce any changes to the register accesses.
With this patch code is only broken out, and the patch after this
makes it possible to use this driver without the cnf block. For
hardware with the cnf block included there should be no change. Unless
I made some mistake that is. =)

> I'm wondering if this is a common problem.
>
> It seems to me  that its likely that clock and power control are often
> external to the host controller chip. (not just on TMIO MMC).

I agree. These patches show just that.

> I think it'd be a good idea if we came up with an infrastructre that allowed
> a set of clock / power control callbacks into the platform code.
>
> Whats your use case for this? samee applies to the related cnf patch too.

Please wait for platform data patches. They will be sent out late
before -rc1 if we can agree on some way to make the cnf area optional.
Without that (this patch and the next) we can't really use this driver
so platform data patches are pointless. =)

This is lucky guesswork. I just happened to figure out that I can use
the tmio_mmc driver to drive the ctl block of this hardware. As for
memory window setup, voltage control, and hotplug - I have no idea. No
docs apart from the Toshiba docs that I can find online. But breaking
out the power control and hotplug management sounds like a good plan.

As for clock control, maybe breaking out that as well is a good idea.
At least part of it. The first thought that pops into my mind is to
tie in the clock framework. So we can use clk_enable() and
clk_disable() for run time power management and call clk_get_rate() to
figure out the parent clock rate and use that to calculate the divider
for CTL_SD_CARD_CLK_CTL. Not sure if the clock framework is supported
on your target platforms though.

Do you have any recommended hardware platform to test this driver?
I'll try to find such hardware platform (or similar) so I have
something to compare with. Unfortunately I don't have that much time
to spend on this. But an hour here and there is probably fine.

How would you like to proceed? I'd prefer to merge this first and
break out after, but I'm not sure if that fits well with your plan.

Thanks for your help!

Cheers,

/ magnus

Re: [PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Wed, 11 Mar 2009 21:58:45 +0900
Magnus Damm <magnus.damm@gmail.com> wrote:

> tmio_mmc: Minor fixes and cnf/irq changes
> 
> These patches fix two trivial issues and updates the tmio_mmc
> driver to support hardware configurations that lack the cnf io
> area (only a single ctl area) but may use multiple interrupts.
> 
> [PATCH 01/05] tmio_mmc: Fix one off, use resource_size() in probe()
> [PATCH 02/05] tmio_mmc: Fix use after free in remove()
> [PATCH 03/05] tmio_mmc: Break out cnf area operations
> [PATCH 04/05] tmio_mmc: Make cnf area optional
> [PATCH 05/05] tmio_mmc: Support multiple interrupts
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---

I've queued up patch one and two for now. Thanks.

Ian, I noticed that there isn't a MAINTAINERS entry for this driver.
Care to send me a patch to remedy that?

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Magnus Damm]

On 3/16/09, Pierre Ossman <drzeus@drzeus.cx> wrote:
> On Wed, 11 Mar 2009 21:58:45 +0900
>  Magnus Damm <magnus.damm@gmail.com> wrote:
>
>  > tmio_mmc: Minor fixes and cnf/irq changes

>  > [PATCH 01/05] tmio_mmc: Fix one off, use resource_size() in probe()
>  > [PATCH 02/05] tmio_mmc: Fix use after free in remove()
>  > [PATCH 03/05] tmio_mmc: Break out cnf area operations
>  > [PATCH 04/05] tmio_mmc: Make cnf area optional
>  > [PATCH 05/05] tmio_mmc: Support multiple interrupts
>  >
>  > Signed-off-by: Magnus Damm <damm@opensource.se>

> I've queued up patch one and two for now. Thanks.

Thanks for your help. I was hoping on getting patch 3 and 4 included
in 2.6.30 as well if possible. I suspect that i have to rework them a
bit first though. That's no problem. Just let me know how you guys
prefer things and i'll resend.

/ magnus

Re: [PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Ian Molton]

Pierre Ossman wrote:

> Ian, I noticed that there isn't a MAINTAINERS entry for this driver.
> Care to send me a patch to remedy that?

Something like this?

diff --git a/MAINTAINERS b/MAINTAINERS
index 5d460c9..6dd8805 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4298,6 +4298,11 @@ L:	tlinux-users@tce.toshiba-dme.co.jp
  W:	http://www.buzzard.org.uk/toshiba/
  S:	Maintained

+TMIO MMC DRIVER
+P:	Ian Molton
+M:	ian@mnementh.co.uk
+S:	Maintained
+
  TPM DEVICE DRIVER
  P:	Debora Velarde
  M:	debora@linux.vnet.ibm.com

Re: [PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Ian Molton]

Magnus Damm wrote:

> Thanks for your help. I was hoping on getting patch 3 and 4 included
> in 2.6.30 as well if possible. I suspect that i have to rework them a
> bit first though. That's no problem. Just let me know how you guys
> prefer things and i'll resend.

Can you clarify how your platform actually controls the MMC clock? does 
it at all?

what is your platform?

Ideally, the MMC code would use the clock API for some of this, but I 
seem to remember there were issues with this (cant recall what right now 
and its 2:05 AM).

Re: [PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

On Tue, 24 Mar 2009 02:00:04 +0000
Ian Molton <ian@mnementh.co.uk> wrote:

> Pierre Ossman wrote:
> 
> > Ian, I noticed that there isn't a MAINTAINERS entry for this driver.
> > Care to send me a patch to remedy that?
> 
> Something like this?
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5d460c9..6dd8805 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4298,6 +4298,11 @@ L:	tlinux-users@tce.toshiba-dme.co.jp
>   W:	http://www.buzzard.org.uk/toshiba/
>   S:	Maintained
> 
> +TMIO MMC DRIVER
> +P:	Ian Molton
> +M:	ian@mnementh.co.uk
> +S:	Maintained
> +
>   TPM DEVICE DRIVER
>   P:	Debora Velarde
>   M:	debora@linux.vnet.ibm.com

Yes, thank you. I've queued up a commit with this.

-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Magnus Damm]

On Tue, Mar 24, 2009 at 11:07 AM, Ian Molton <ian@mnementh.co.uk> wrote:
> Magnus Damm wrote:
>
>> Thanks for your help. I was hoping on getting patch 3 and 4 included
>> in 2.6.30 as well if possible. I suspect that i have to rework them a
>> bit first though. That's no problem. Just let me know how you guys
>> prefer things and i'll resend.
>
> Can you clarify how your platform actually controls the MMC clock? does it
> at all?

I don't know. The block is embedded in an SoC and I have no
documentation. I assume there is some kind of clock hooked up to the
hardware block, and making use of that should be quite simple since we
already have clock framework support in place for the processor.

> what is your platform?

With tmio_mmc and the posted patches in this series I can use some
platform data to improve MMC performance. So far MMC support looks
like this:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=70e5c4f0843c3021a2e95b46edd8925a885d2e31

I think the MMC performance can be improved about 100x with tmio_mmc,
so it would be great to get this working. =)

> Ideally, the MMC code would use the clock API for some of this, but I seem
> to remember there were issues with this (cant recall what right now and its
> 2:05 AM).

No worries. For me the clock framework would be the best fit too. Not
sure about the exact details right now, but I _do_ know that we right
now we have nothing, so being able to use tmio_mmc as is (with posted
patches) is a great step forward. Working out the clock framework
details after that should be no biggie. Step by step.

Or if you want me to rewrite things that's fine as well. Just let me
know what to do. =)

Thanks for your help!

/ magnus

Re: [PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Magnus Damm]

On Wed, Mar 25, 2009 at 5:56 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Tue, Mar 24, 2009 at 11:07 AM, Ian Molton <ian@mnementh.co.uk> wrote:
>> Ideally, the MMC code would use the clock API for some of this, but I seem
>> to remember there were issues with this (cant recall what right now and its
>> 2:05 AM).
>
> No worries. For me the clock framework would be the best fit too. Not
> sure about the exact details right now, but I _do_ know that we right
> now we have nothing, so being able to use tmio_mmc as is (with posted
> patches) is a great step forward. Working out the clock framework
> details after that should be no biggie. Step by step.
>
> Or if you want me to rewrite things that's fine as well. Just let me
> know what to do. =)

Ping? Please let me know how you want me to rework the patches. Unless
they are ok as-is. Any feedback on how to rewrite them would be
greatly appreciated.

These patches should leave register accesses unchanged for the common
case. The only change is to allow using the driver with only a single
io memory area. I split out this change in two separate patches to
make review easy.

I realize that we're running late for 2.6.30-rc1, but since these
patches are isolated driver changes a later merge may be possible, I'm
not sure.

Anyway, regardless of merge timing it would be great to move forward somehow.

Thank you.

/ magnus

Re: [PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Ian Molton]

Magnus Damm wrote:

> Ping? Please let me know how you want me to rework the patches. Unless
> they are ok as-is. Any feedback on how to rewrite them would be
> greatly appreciated.

I replied to this earlier. Basically, investigate using the clk API. You 
should also try to work out how your board controls  clock / power to 
the socckets (if it can at all).

Like I said though, IIRC the clk API had shortcommings last time I 
looked which made it impossible to use on MFD devices (its tied to the 
CPU architecture, wheras the MFDs are platform independant. Dmitry did 
some work on this, but I dont recall how far he got.

Let me know if you come up with answers / solutions to these probelms. 
Until then, NAK - lets do it the right way, one time only. Not hack and 
bodge it repeatedly.

Sorry if that seems harsh, but I dont have time to review a hack thats 
going to end up replaced anyway when its done properly.

Best starting point would be to look up Dmitrys work on making the clk 
api CPU agnostic (if that hasnt already been merged). Then tmio-mmc can 
be modified to reqest a clock from its parent device (be that an MFD 
core or a platform device or whatever).

-Ian

-Ian

Re: [PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Magnus Damm]

On 3/31/09, Ian Molton <ian@mnementh.co.uk> wrote:
> Magnus Damm wrote:
>
>
> > Ping? Please let me know how you want me to rework the patches. Unless
> > they are ok as-is. Any feedback on how to rewrite them would be
> > greatly appreciated.
> >
>
>  I replied to this earlier. Basically, investigate using the clk API. You
> should also try to work out how your board controls  clock / power to the
> socckets (if it can at all).

The SoC is directly connected to the SD connector. I've verified this
by looking at board schematics. There is no power control hardware on
the boards that I've seen so far, but I'm currently working with
hardware designers to make sure they will add such capabilities to
future boards. The power will then be controlled by board specific
code, most likely using GPIO pins. The hardware block that the
tmio_mmc driver is handling does not have any power control
functionality.

As for the clock API, adding such a feature to the tmio_mmc driver is
not very complicated, especially for the SoC case where we already
have control over all system clocks.

>  Like I said though, IIRC the clk API had shortcommings last time I looked
> which made it impossible to use on MFD devices (its tied to the CPU
> architecture, wheras the MFDs are platform independant. Dmitry did some work
> on this, but I dont recall how far he got.

Some architectures may have clock framework support, some may not. I
guess wrapping the clock functions in #ifdefs is one (ugly) way to
support both cases. And if we consider MFD it certainly becomes more
complicated.

>  Let me know if you come up with answers / solutions to these probelms.
> Until then, NAK - lets do it the right way, one time only. Not hack and
> bodge it repeatedly.

So the current tmio_mmc driver does not use the clock API. With my
patches the clock API us still unused. I agree that working on adding
clock API support is needed, but I don't see how this is related to
single iomem window support.

Regardless of clock API, I still need a way to use the driver with a
single iomem window. Please propose how to use single iomem window
harware with tmio_mmc.

>  Sorry if that seems harsh, but I dont have time to review a hack thats
> going to end up replaced anyway when its done properly.

So exactly what is the "proper" solution for single iomem window support?

And why does single iomem window support have to block on clock API support?

>  Best starting point would be to look up Dmitrys work on making the clk api
> CPU agnostic (if that hasnt already been merged). Then tmio-mmc can be
> modified to reqest a clock from its parent device (be that an MFD core or a
> platform device or whatever).

Yes, that sounds like a good starting point for clock API support.

But... How do I use tmio_mmc with hardware that only has a single
iomem window? I need that regardless of clock API, and that's what the
code in this patch series is all about.

Thanks,

/ magnus

Re: [PATCH 00/05] tmio_mmc: Minor fixes and cnf/irq changes

[Ian Molton]

Magnus Damm wrote:

> The SoC is directly connected to the SD connector. I've verified this
> by looking at board schematics. There is no power control hardware on
> the boards that I've seen so far,

Which SoC? (I think you said before, but I forgot).

> but I'm currently working with
> hardware designers to make sure they will add such capabilities to
> future boards. The power will then be controlled by board specific
> code, most likely using GPIO pins. The hardware block that the
> tmio_mmc driver is handling does not have any power control
> functionality.

Great stuff.

> As for the clock API, adding such a feature to the tmio_mmc driver is
> not very complicated, especially for the SoC case where we already
> have control over all system clocks.

The problem (as I pointed out, and you noted below) is that we cant only 
consider the SoC case because there ARE other current users of the 
device in-tree and they use MFD. They arent going away.

> Some architectures may have clock framework support, some may not. I
> guess wrapping the clock functions in #ifdefs is one (ugly) way to
> support both cases. And if we consider MFD it certainly becomes more
> complicated.

I dont mind tmio-mmc requiring the clk api. its only used on embedded 
platforms and these usually have clk support.

> So the current tmio_mmc driver does not use the clock API. With my
> patches the clock API us still unused. I agree that working on adding
> clock API support is needed, but I don't see how this is related to
> single iomem window support.

  Because that second iomem window is _purely_ in control of clock and 
power management.

Rather than design a bunch of special callbacks for clock control which 
will later be got rid of I think it is better that we create the proper 
infrastructure in the first place. Its on my to-do but IIRC Dmitry has 
done some work on it already and you're more than welcome to finish that 
work off.

> Regardless of clock API, I still need a way to use the driver with a
> single iomem window. Please propose how to use single iomem window
> harware with tmio_mmc.

If you dont want to extend the clk api to cover it, you can use a patch 
in your local tree?

> So exactly what is the "proper" solution for single iomem window support?

Extand the clk API to be arch agnostic.

> And why does single iomem window support have to block on clock API support?

Because with clk API support it is not necessary.

> But... How do I use tmio_mmc with hardware that only has a single
> iomem window?

if you get the clk API support generalised, I personally promise I will 
rip out the second IO window myself and move it to the TMIO/ASIC3 MFD 
core (where it belongs). Then we both get what we want.

> I need that regardless of clock API, and that's what the
> code in this patch series is all about.

Not regardless. single IO window in the tmio-mmc driver is  _dependant_ 
on clk API support, IMO.

-Ian