[mmc] alternative TI FM MMC/SD driver for 2.6.21-rc7

[mmc] alternative TI FM MMC/SD driver for 2.6.21-rc7

[Sergey Yanovich]

Hi,

The device is present in many notebooks. Notebooks depend heavily on 
suspend/resume functionality. tifm_core/7xx1/sd family is an ambitous, 
but uncompleted project. It used to crash on resuming, or hang up on 
suspending. A less common failure used to be trigerred by a fast card 
insert/removal sequence. Finally, tifm_sd module needs to be manually 
inserted.

I have found it easier to rewrite the driver, than to fix. This driver 
is kind of mutant. The bones are taken from sdhci and omap, the meat - 
from tifm_*. It contains all features (and bugs except named above) of 
tifm_* as it was in kernel 2.6.21-rc7.

I have been testing this version since linux-2.6.18 (daily reading 
photos from cards, daily suspending/resuming) without a single glitch.

This patch only provides sources.
http://bugzilla.kernel.org/attachment.cgi?id=11238&action=view
 
Kernel configuration in this message.
http://bugzilla.kernel.org/attachment.cgi?id=11239&action=view

Alex Dubov has done exceptionally great lots of work to teach linux 
speak to TIFM. This is just a reorganization of his project.

The driver seems to be practically stable, but it definitely must be 
tested by more people. Please also report any issues with this driver to 
http://bugzilla.kernel.org/show_bug.cgi?id=8352 so that valuable info is 
not lost.

Best regards,
Sergey Yanovich

Re: [mmc] alternative TI FM MMC/SD driver for 2.6.21-rc7

[Arnd Bergmann]

On Thursday 19 April 2007, Sergey Yanovich wrote:
> The device is present in many notebooks. Notebooks depend heavily on 
> suspend/resume functionality. tifm_core/7xx1/sd family is an ambitous, 
> but uncompleted project. It used to crash on resuming, or hang up on 
> suspending. A less common failure used to be trigerred by a fast card 
> insert/removal sequence. Finally, tifm_sd module needs to be manually 
> inserted.

As very general comments, you should have the maintainer of the subsystem
(Pierre in this case) on Cc when posting a driver, and you should include
the patch inline in your mail, see Documentation/SubmittingPatches.

More specific to your patch:

You should include the Makefile and Kconfig changes in the same patch/mail,
no point splitting these out.

Don't define your own DBG macro, instead use the predefined dev_dbg()
that has a similar definition.

Your mmc_tifm_irq_chip() function does a _very_ long delay of 100
miliseconds. This is normally not acceptable, since it is a noticeable
time in which the system is completely unresponsive. Maybe you can convert
the tasklet to a workqueue, which lets you call msleep instead of mdelay.

Your use of pci_map_sg() looks wrong, you simply can't assume that the
return value is '1' in general. I've stumbled over that same problem
in the sdhci driver, so it may be inherent to the mmc layer and not
be driver specific.

Other than that, your driver looks pretty good to me.

	Arnd <><

Re: [mmc] alternative TI FM MMC/SD driver for 2.6.21-rc7

[Sergey Yanovich]

Hi,

Arnd Bergmann wrote:
> As very general comments, you should have the maintainer of the subsystem
> (Pierre in this case) on Cc when posting a driver, and you should include
> the patch inline in your mail, see Documentation/SubmittingPatches.
>   
I have cc'ed both Pierre and Alex, but my first message was blocked
by the list as it contained html.

For inlinning, this is my first kernel patch. I have just followed
http://www.tux.org/lkml/#s2-2.
> You should include the Makefile and Kconfig changes in the same patch/mail,
> no point splitting these out.
>   
Once again it was an advise from http://www.tux.org/lkml/#s1-10. 
<http://www.tux.org/lkml/#s1-10>
> Don't define your own DBG macro, instead use the predefined dev_dbg()
> that has a similar definition.
>   
Somewhere in 0.5-0.6 version this driver has issues with timeouts , 
which were
revealing in a non-debug kernel builds only . So this was a nessecity. I 
will purge
them now.
> Your mmc_tifm_irq_chip() function does a _very_ long delay of 100
> miliseconds. This is normally not acceptable, since it is a noticeable
> time in which the system is completely unresponsive. Maybe you can convert
> the tasklet to a workqueue, which lets you call msleep instead of mdelay.
>   
This is done intentionally to prevent a race condition when a card is 
removed
and immediately reinserted. There may be a more complicated way to solve 
this
issue, but didn't think about them. This only happens when an MMC/SD 
card is
inserted/removed. And it takes at least as long to process the event in 
other
parts of subsystem.
> Your use of pci_map_sg() looks wrong, you simply can't assume that the
> return value is '1' in general. I've stumbled over that same problem
> in the sdhci driver, so it may be inherent to the mmc layer and not
> be driver specific.
>   
This is taken as is from [tifm_sd]. I suppose this relates to a hardware
limitation:

+	mmc->max_hw_segs = 1;
+	mmc->max_phys_segs = 1;


Best regards,
Sergey

[PATCH] [mmc] Removes custom debug macro

[Sergey Yanovich]

Signed-off-by: Sergey Yanovich <ynvich@gmail.com>
---
 drivers/mmc/tifm.c |   22 ++++++----------------
 1 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/tifm.c b/drivers/mmc/tifm.c
index 5410e66..30cab30 100644
--- a/drivers/mmc/tifm.c
+++ b/drivers/mmc/tifm.c
@@ -28,15 +28,6 @@
 #define DRIVER_NAME "tifm"
 #define DRIVER_VERSION "0.7"
 
-/*#define CONFIG_MMC_DEBUG*/
-#ifdef CONFIG_MMC_DEBUG
-#define DBG(f,arg...) \
-	printk(KERN_DEBUG DRIVER_NAME ": " f,##arg)
-#else /* CONFIG_MMC_DEBUG */
-#define DBG(f,arg...) \
-	do { } while (0)
-#endif /* CONFIG_MMC_DEBUG */
-
 static int no_dma = 0;
 
 struct tifm_chip;
@@ -594,7 +585,6 @@ mmc_tifm_irq(int irq, void *dev_id)
 		if(card_status && chip->socket[cnt])
 			mmc_tifm_irq_card(chip->socket[cnt],card_status);
 	}
-	/*DBG("got irq status 0x%08x\n", irq_status);*/
 	spin_unlock(&chip->lock);
 	return IRQ_HANDLED;
 }
@@ -611,7 +601,7 @@ mmc_tifm_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	struct mmc_data *data = mrq->cmd->data;
 
 	spin_lock_irqsave(&socket->lock, flags);
-	DBG("executing opcode %u\n",mrq->cmd->opcode);
+	dev_dbg(socket->chip->pdev,"executing opcode %u\n",mrq->cmd->opcode);
 	if(!(socket->flags & POWER_ON)){
 		spin_unlock_irqrestore(&socket->lock, flags);
 		goto err_out;
@@ -665,7 +655,7 @@ mmc_tifm_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	int cnt;
 
 	spin_lock_irqsave(&socket->lock, flags);
-	DBG("setting io for socket %u\n",socket->index);
+	dev_dbg(socket->chip->pdev,"setting io for socket %u\n",socket->index);
 
 	if(!(socket->flags & MEDIA_MMC)){
 		writel(0,socket->addr + SOCK_MMCSD_INT_ENABLE);
@@ -808,7 +798,7 @@ mmc_tifm_suspend(struct pci_dev *pdev, pm_message_t state)
 		spin_unlock_irqrestore(chip->socket[cnt]->lock,flags);
 		mmc=container_of((void*)chip->socket[cnt],
 				struct mmc_host,private);
-		DBG(" ... socket %u\n",cnt);
+		dev_dbg(chip->pdev," ... socket %u\n",cnt);
 		mmc_suspend_host(mmc,state);
 		spin_lock_irqsave(chip->socket[cnt]->lock,flags);
 		chip->socket[cnt]->flags = 0;
@@ -889,14 +879,14 @@ mmc_tifm_probe_socket(struct tifm_chip *chip, int num)
 	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
 	mmc->max_seg_size = mmc->max_req_size;
 
-	DBG("initing tasklets for socket %u\n",num);
+	dev_dbg(chip->pdev,"initing tasklets for socket %u\n",num);
 	tasklet_init(&socket->irq_chip,mmc_tifm_irq_chip,(unsigned long)socket);
 	tasklet_init(&socket->cmd_end,mmc_tifm_cmd_end,(unsigned long)socket);
 	setup_timer(&socket->timer,mmc_tifm_timer,(unsigned long)socket);
 	chip->socket[num] = socket;
 	tifm_socket_event(chip,num,1);
 
-	DBG("adding mmc host for socket %u\n",num);
+	dev_dbg(chip->pdev,"adding mmc host for socket %u\n",num);
 	mmc_add_host(mmc);
 	
 	return rc;
@@ -983,7 +973,7 @@ mmc_tifm_probe(struct pci_dev *pdev, const struct pci_device_id *dev_id)
 		else
 			break;
 	}
-	DBG("created %u socket(s)\n",sockets-cnt-1);
+	dev_dbg(chip->pdev,"created %u socket(s)\n",sockets-cnt-1);
 	if(rc)
 		goto err_out_unmap;
 	return 0;
-- 
1.5.1.2

[PATCH] [mmc] Removes custom debug macro

[Sergey Yanovich]

Signed-off-by: Sergey Yanovich <ynvich@gmail.com>
---
 drivers/mmc/tifm.c |   22 ++++++----------------
 1 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/tifm.c b/drivers/mmc/tifm.c
index 5410e66..30cab30 100644
--- a/drivers/mmc/tifm.c
+++ b/drivers/mmc/tifm.c
@@ -28,15 +28,6 @@
 #define DRIVER_NAME "tifm"
 #define DRIVER_VERSION "0.7"
 
-/*#define CONFIG_MMC_DEBUG*/
-#ifdef CONFIG_MMC_DEBUG
-#define DBG(f,arg...) \
-	printk(KERN_DEBUG DRIVER_NAME ": " f,##arg)
-#else /* CONFIG_MMC_DEBUG */
-#define DBG(f,arg...) \
-	do { } while (0)
-#endif /* CONFIG_MMC_DEBUG */
-
 static int no_dma = 0;
 
 struct tifm_chip;
@@ -594,7 +585,6 @@ mmc_tifm_irq(int irq, void *dev_id)
 		if(card_status && chip->socket[cnt])
 			mmc_tifm_irq_card(chip->socket[cnt],card_status);
 	}
-	/*DBG("got irq status 0x%08x\n", irq_status);*/
 	spin_unlock(&chip->lock);
 	return IRQ_HANDLED;
 }
@@ -611,7 +601,7 @@ mmc_tifm_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	struct mmc_data *data = mrq->cmd->data;
 
 	spin_lock_irqsave(&socket->lock, flags);
-	DBG("executing opcode %u\n",mrq->cmd->opcode);
+	dev_dbg(socket->chip->pdev,"executing opcode %u\n",mrq->cmd->opcode);
 	if(!(socket->flags & POWER_ON)){
 		spin_unlock_irqrestore(&socket->lock, flags);
 		goto err_out;
@@ -665,7 +655,7 @@ mmc_tifm_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	int cnt;
 
 	spin_lock_irqsave(&socket->lock, flags);
-	DBG("setting io for socket %u\n",socket->index);
+	dev_dbg(socket->chip->pdev,"setting io for socket %u\n",socket->index);
 
 	if(!(socket->flags & MEDIA_MMC)){
 		writel(0,socket->addr + SOCK_MMCSD_INT_ENABLE);
@@ -808,7 +798,7 @@ mmc_tifm_suspend(struct pci_dev *pdev, pm_message_t state)
 		spin_unlock_irqrestore(chip->socket[cnt]->lock,flags);
 		mmc=container_of((void*)chip->socket[cnt],
 				struct mmc_host,private);
-		DBG(" ... socket %u\n",cnt);
+		dev_dbg(chip->pdev," ... socket %u\n",cnt);
 		mmc_suspend_host(mmc,state);
 		spin_lock_irqsave(chip->socket[cnt]->lock,flags);
 		chip->socket[cnt]->flags = 0;
@@ -889,14 +879,14 @@ mmc_tifm_probe_socket(struct tifm_chip *chip, int num)
 	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
 	mmc->max_seg_size = mmc->max_req_size;
 
-	DBG("initing tasklets for socket %u\n",num);
+	dev_dbg(chip->pdev,"initing tasklets for socket %u\n",num);
 	tasklet_init(&socket->irq_chip,mmc_tifm_irq_chip,(unsigned long)socket);
 	tasklet_init(&socket->cmd_end,mmc_tifm_cmd_end,(unsigned long)socket);
 	setup_timer(&socket->timer,mmc_tifm_timer,(unsigned long)socket);
 	chip->socket[num] = socket;
 	tifm_socket_event(chip,num,1);
 
-	DBG("adding mmc host for socket %u\n",num);
+	dev_dbg(chip->pdev,"adding mmc host for socket %u\n",num);
 	mmc_add_host(mmc);
 	
 	return rc;
@@ -983,7 +973,7 @@ mmc_tifm_probe(struct pci_dev *pdev, const struct pci_device_id *dev_id)
 		else
 			break;
 	}
-	DBG("created %u socket(s)\n",sockets-cnt-1);
+	dev_dbg(chip->pdev,"created %u socket(s)\n",sockets-cnt-1);
 	if(rc)
 		goto err_out_unmap;
 	return 0;
-- 
1.5.1.2

Re: [PATCH] [mmc] Removes custom debug macro

[Arnd Bergmann]

On Wednesday 25 April 2007, Sergey Yanovich wrote:
> Signed-off-by: Sergey Yanovich <ynvich@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH] [mmc] [tifm] Reduces delay in card insert/removal

[Sergey Yanovich]

First, I tried to replace 'mdelay' with 'msleep' and put it after
'unlock'. It gives a certain oops.

With the delay of 50 msec driver remains stable. It has something
to do with hardware initialization, so this value should not be
affected by CPU speed. I hope so.

Signed-off-by: Sergey Yanovich <ynvich@gmail.com>
---
 drivers/mmc/tifm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/tifm.c b/drivers/mmc/tifm.c
index 30cab30..310f07c 100644
--- a/drivers/mmc/tifm.c
+++ b/drivers/mmc/tifm.c
@@ -361,7 +361,7 @@ mmc_tifm_irq_chip(unsigned long param)
 		socket->state = READY;
 		tasklet_schedule(&socket->cmd_end);
 	}
-	mdelay(100);
+	mdelay(50);
 	spin_unlock_irqrestore(&socket->lock, flags);
 	mmc_detect_change(mmc, 0);
 }
-- 
1.5.1.2

[PATCH] [mmc] [tifm] Reduces delay in card insert/removal

[Sergey Yanovich]

First, I tried to replace 'mdelay' with 'msleep' and put it after
'unlock'. It gives a certain oops.

With the delay of 50 msec driver remains stable. It has something
to do with hardware initialization, so this value should not be
affected by CPU speed. I hope so.

Signed-off-by: Sergey Yanovich <ynvich@gmail.com>
---
 drivers/mmc/tifm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/tifm.c b/drivers/mmc/tifm.c
index 30cab30..310f07c 100644
--- a/drivers/mmc/tifm.c
+++ b/drivers/mmc/tifm.c
@@ -361,7 +361,7 @@ mmc_tifm_irq_chip(unsigned long param)
 		socket->state = READY;
 		tasklet_schedule(&socket->cmd_end);
 	}
-	mdelay(100);
+	mdelay(50);
 	spin_unlock_irqrestore(&socket->lock, flags);
 	mmc_detect_change(mmc, 0);
 }
-- 
1.5.1.2

Re: [PATCH] [mmc] [tifm] Reduces delay in card insert/removal

[Arnd Bergmann]

On Wednesday 25 April 2007, Sergey Yanovich wrote:

> First, I tried to replace 'mdelay' with 'msleep' and put it after
> 'unlock'. It gives a certain oops.
> 
> With the delay of 50 msec driver remains stable. It has something
> to do with hardware initialization, so this value should not be
> affected by CPU speed. I hope so.
> 
> Signed-off-by: Sergey Yanovich <ynvich@gmail.com>

No, 50ms is still too much, I'd consider every instance of 'mdelay'
to be a bug in general, except maybe with exceptionally broken
hardware, which I don't think this is.

The reason that it gave you an oops is that you call it from
a tasklet. You have to convert the tasklet to a workqueue at the
same time if you want to sleep inside it. In fact, it might
be a good idea to convert all the tasklets in that driver
to use schedule_work instead.

	Arnd <><

[PATCH] [mmc] [tifm] Reduces delay in card insert/removal

[Sergey Yanovich]

First, I tried to replace 'mdelay' with 'msleep' and put it after
'unlock'. It gives a certain oops.

With the delay of 50 msec driver remains stable. It has something
to do with hardware initialization, so this value should not be
affected by CPU speed. I hope so.

Signed-off-by: Sergey Yanovich <ynvich@gmail.com>
---
 drivers/mmc/tifm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/tifm.c b/drivers/mmc/tifm.c
index 30cab30..310f07c 100644
--- a/drivers/mmc/tifm.c
+++ b/drivers/mmc/tifm.c
@@ -361,7 +361,7 @@ mmc_tifm_irq_chip(unsigned long param)
 		socket->state = READY;
 		tasklet_schedule(&socket->cmd_end);
 	}
-	mdelay(100);
+	mdelay(50);
 	spin_unlock_irqrestore(&socket->lock, flags);
 	mmc_detect_change(mmc, 0);
 }
-- 
1.5.1.2