alternative TI FM MMC/SD driver for 2.6.21-rc7
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
Quoted 6 lines · Sergey Yanovich
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_sdmodule 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 <><
Hi,
Quoted 4 lines · Arnd Bergmann
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.
Quoted 3 lines
You should include the
MakefileandKconfigchanges 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>
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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 1/1 Removes custom debug macro
In reply to Arnd Bergmann--- drivers/mmc/tifm.c | 22 ++++++---------------- 1 files changed, 6 insertions(+), 16 deletions(-)
drivers/mmc/tifm.c +6 -16
@@ -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 ;
Signature · 1 line
1.5.1.2
PATCH 1/1 Reduces delay in card insert/removal
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.
--- drivers/mmc/tifm.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/mmc/tifm.c +1 -1
@@ -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 ); }
Signature · 1 line
1.5.1.2
PATCH 1/1 Reduces delay in card insert/removal
In reply to Arnd BergmannFirst, 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.
--- drivers/mmc/tifm.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/mmc/tifm.c +1 -1
@@ -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 ); }
Signature · 1 line
1.5.1.2
Re: [PATCH] [mmc] Removes custom debug macro
In reply to Sergey YanovichQuoted 1 line · Sergey Yanovich
Signed-off-by Sergey Yanovich <[email protected]>
PATCH 1/1 Reduces delay in card insert/removal
In reply to Sergey YanovichFirst, 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.
--- drivers/mmc/tifm.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/mmc/tifm.c +1 -1
@@ -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 ); }
Signature · 1 line
1.5.1.2
PATCH 1/1 Removes custom debug macro
In reply to Arnd Bergmann--- drivers/mmc/tifm.c | 22 ++++++---------------- 1 files changed, 6 insertions(+), 16 deletions(-)
drivers/mmc/tifm.c +6 -16
@@ -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 ;
Signature · 1 line
1.5.1.2
Re: [PATCH] [mmc] [tifm] Reduces delay in card insert/removal
In reply to Sergey YanovichQuoted 8 lines · 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 <[email protected]>
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 <><