[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] [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

[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>

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

[Alex Dubov]

Have you looked at the last version (0.8)? It fixed all outstanding issues (as far as I know).


--- Sergey Yanovich <ynvich@gmail.com> wrote:


---------------------------------
  Hi,

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

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

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

This patch only provides sources.
[PATCH1/2] [mmc] alternative TI FM MMC/SD driver for 2.6.21-rc7
Kernel configuration in this message.
[PATCH2/2] [mmc] alternative TIFM driver config for 2.6.21-rc7

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

The driver seems to be practically stable, but it definitely must betested by more people. Please
also report any issues with this driverto linux bug#8352 so that valuable info is not lost.

Best regards,
Sergey Yanovich



__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

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

[Brad Campbell]

Alex Dubov wrote:
> Have you looked at the last version (0.8)? It fixed all outstanding issues (as far as I know).
> 

Seconded. I've been running Alex's latest driver since its release. I routinely suspend/resume 
60-100 times between boots to S3 and disk, I've suspended with cards in the socket and I've tested 
it using SD and MMC .. I've not been able to fault it now. In fact, checking my mail archives, my 
last reported bug was on the 17th of Feb and it has been rock solid since. I just don't think about 
it anymore, it simply works...

Brad
-- 
"Human beings, who are almost unique in having the ability
to learn from the experience of others, are also remarkable
for their apparent disinclination to do so." -- Douglas Adams

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

[Fabio Comolli]

+1

On 4/20/07, Brad Campbell <brad@wasp.net.au> wrote:
> Alex Dubov wrote:
> > Have you looked at the last version (0.8)? It fixed all outstanding issues (as far as I know).
> >
>
> Seconded. I've been running Alex's latest driver since its release. I routinely suspend/resume
> 60-100 times between boots to S3 and disk, I've suspended with cards in the socket and I've tested
> it using SD and MMC .. I've not been able to fault it now. In fact, checking my mail archives, my
> last reported bug was on the 17th of Feb and it has been rock solid since. I just don't think about
> it anymore, it simply works...
>
> Brad
> --
> "Human beings, who are almost unique in having the ability
> to learn from the experience of others, are also remarkable
> for their apparent disinclination to do so." -- Douglas Adams
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

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

[Sergey Yanovich]

Hi,

>> > Have you looked at the last version (0.8)? It fixed all outstanding 
>> issues (as far as I know).
>> >
>>
>> Seconded. I've been running Alex's latest driver since its release. I 
>> routinely suspend/resume
>> 60-100 times between boots to S3 and disk, I've suspended with cards 
>> in the socket and I've tested
>> it using SD and MMC .. I've not been able to fault it now. In fact, 
>> checking my mail archives, my
>> last reported bug was on the 17th of Feb and it has been rock solid 
>> since. I just don't think about
>> it anymore, it simply works...
>>

Well, I am tracking Alex's svn. It is for off-the-tree builds. I 
reorganized Alex's code in Oct 2006 and added
a patch to my debian/patches. Until debian shipped linux-2.6.20, there 
were no [tifm_*] in their tree, so no conflicts. The 2.6.20 came with 
tifm_sd-v0.6 which is not so stable. I dug lkml and bugzilla.kernel.org 
for fresh patches, but found 
http://bugzilla.kernel.org/show_bug.cgi?id=8052. Which made me think, 
svn is still unstable. I merged in-kernel updates to my old (v2.6.18-*) 
version. It compiles and runs OK, so I posted it. No offense to anyone.

Best regards,
Sergey

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

[Alex Dubov]

> Finally, tifm_sd module needs to be manually inserted.

By the way, the driver emits custom uevent when the new card is detected. I was going to look at
this some day in the future, but if you want to mess a little with hotplug scripts the issue can
be easily solved.

As I already said before, many of the complications exist because this is  an universal adapter,
and memorystick support is quite near in the queue. A good hotplug script will, therefore, look at
the "TIFM_CARD_TYPE" event var and load the appropriate media driver.


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

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

[Sergey Yanovich]

Hi,

> > Finally, tifm_sd module needs to be manually inserted.
>
> By the way, the driver emits custom uevent when the new card is detected. I was going to look at
> this some day in the future, but if you want to mess a little with hotplug scripts the issue can
> be easily solved.

It would be nice if you to provide a sample script. Unfortunately,
hotplug scripts are distribution specific. And internal knowledge of
your modules is required to write one.

For a typical, non-linux-geek user there are just two states of the device -
available and not available. Ububtu is famous for its end-user support.
They ship your driver since linux-2.6.17. But they pack it in one module.
And that is _much_ easier, then a hotplug script.

And maybe, you would care to create a branch in you repository for
in-the-tree compiles.

> As I already said before, many of the complications exist because this is  an universal adapter,
> and memorystick support is quite near in the queue.

The scope of your project is exceptionally wide. You plan to develop a whole
new layer for memorystick as a part.

At the same time, the [tifm_sd] code is device specific, and [tifm_7xx1]
code is also device specific. And both modules depend on the same device
(of family of devices). That makes me think that a bus/controller/slot
construction is not going to make thing any easier, but adds complexity.

In case of MMC/SD all abstraction has already been separated by the mmc
layer, in case of MS it is up to you, how to implement.

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

[Matthew Garrett]

On Sun, Apr 22, 2007 at 03:15:22PM +0300, Sergey Yanovich wrote:

> For a typical, non-linux-geek user there are just two states of the device -
> available and not available. Ububtu is famous for its end-user support.
> They ship your driver since linux-2.6.17. But they pack it in one module.
> And that is _much_ easier, then a hotplug script.

No, we ship a udev script.

> At the same time, the [tifm_sd] code is device specific, and [tifm_7xx1]
> code is also device specific. And both modules depend on the same device
> (of family of devices). That makes me think that a bus/controller/slot
> construction is not going to make thing any easier, but adds complexity.

At one point it looked like it might be possible to drive the tifm_7xx0 
devices in a similar way. I'm not sure if this is actually the case, but 
right now the driver design seems to accurately reflect the reality of 
the hardware design. I don't see any especially strong argument for 
breaking that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Sergey Yanovich]

> > For a typical, non-linux-geek user there are just two states of the device -
> > available and not available. Ububtu is famous for its end-user support.
> > They ship your driver since linux-2.6.17. But they pack it in one module.
> > And that is _much_ easier, then a hotplug script.
>
> No, we ship a udev script.

OK, me bad for using the present tense. But you had a single module
in Oct 2006, didn't you? And maybe, you would like to post the script.

--
Sergey Yanovich

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

[Alex Dubov]

--- Sergey Yanovich <ynvich@gmail.com> wrote:

> > > For a typical, non-linux-geek user there are just two states of the device -
> > > available and not available. Ububtu is famous for its end-user support.
> > > They ship your driver since linux-2.6.17. But they pack it in one module.
> > > And that is _much_ easier, then a hotplug script.
> >
> > No, we ship a udev script.
> 
> OK, me bad for using the present tense. But you had a single module
> in Oct 2006, didn't you? And maybe, you would like to post the script.

Here's a simple udev rule that will do what you want:

SUBSYSTEM=="tifm", ACTION=="add", TIFM_CARD_TYPE=="SD", RUN+="/sbin/modprobe tifm_sd"

(just stick it somewhere in the udev rules).

You shall take into consideration that mmc currently lacks uevent support, so mmc_block must be
loaded manually (this is a long pending todo thingy; supposedly it waits for the first sdio
driver). There's no direct dependency of tifm_sd on mmc_block (only on mmc_core).

May be I'll add a modalias entry later so the explicit rule will not be needed.

As a side note: I have some very good reasons for the current driver architecture. You may want to
look them up in the mail archive (I outlined them during the initial driver submission).


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

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

[Sergey Yanovich]

> SUBSYSTEM=="tifm", ACTION=="add", TIFM_CARD_TYPE=="SD", RUN+="/sbin/modprobe tifm_sd"

Thanks.

> As a side note: I have some very good reasons for the current driver architecture. You may want to
> look them up in the mail archive (I outlined them during the initial driver submission).

I am not in any way argue that your driver architecture is wrong or that you
should change anything. My point was simple. [tifm_sd] can only work with
[tifm_7xx1]. If you add support for let's say [tifm_8xx2] in the future, which
would have port offsets different that [tifm_7xx1], you would also need a
completely new modules for slots (sd, ms, etc).

--
Sergey Yanovich

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

[Alex Dubov]

> 
> I am not in any way argue that your driver architecture is wrong or that you
> should change anything. My point was simple. [tifm_sd] can only work with
> [tifm_7xx1]. If you add support for let's say [tifm_8xx2] in the future, which
> would have port offsets different that [tifm_7xx1], you would also need a
> completely new modules for slots (sd, ms, etc).
> 

Does not this constitutes an unbounded speculation? And then, what would you propose to do with
adapters that have SD support disabled? There are quite a few of those in the wild, as of right
now (SD support is provided by bundled SDHCI on such systems, if at all). Similar argument goes
for other media types as well - many controllers have xD support disabled too (I think you have
one of those - Sony really values its customers). After all, it is not healthy to have dead code
in the kernel.

On the other hand, if TI puts out a controller which is functionally identical, but has different
register map, it wouldn't be hard to refactor the code. 


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

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

[Sergey Yanovich]

Hi,
>> If you add support for let's say [tifm_8xx2] in the future, which
>> would have port offsets different that [tifm_7xx1], you would also need a
>> completely new modules for slots (sd, ms, etc).
>>
>
> Does not this constitutes an unbounded speculation?
Only time will tell :)
> And then, what would you propose to do with
> adapters that have SD support disabled? There are quite a few of those in the wild, as of right
> now (SD support is provided by bundled SDHCI on such systems, if at all). Similar argument goes
> for other media types as well - many controllers have xD support disabled too (I think you have
> one of those - Sony really values its customers). After all, it is not healthy to have dead code
> in the kernel.
>
A typical kernel config is an allmconfig, which has tones of dead
code: just see a 'General setup' part of your distro '.config'.
There are item like 'SMP' selected by default for 686+ CPUs. And
this is far more overhead that a single check of card type on
insert.

To allow customization, boolean module options that disable certain
card type may suffice.

And again, you are doing a great work with the driver.

--
Sergey Yanovich

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

[Pierre Ossman]

Sergey Yanovich wrote:
> 
> I have found it easier to rewrite the driver, than to fix.

Before you get your hopes up, this development model is not one that will get
your code merged upstream. You should really try to work with Alex, not side
step him. Drivers are rarely complex enough to warrant, or even have room for, a
rewrite. And judging from your code it looks more like reorganising the code
that's already there.

In short, this "rewrite" won't fly. Report the issues you have with the current
driver and work with the existing maintainer to get them fixed.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

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

[Alex Dubov]

--- Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:

> Sergey Yanovich wrote:
> > 
> > I have found it easier to rewrite the driver, than to fix.
> 
> Before you get your hopes up, this development model is not one that will get
> your code merged upstream. You should really try to work with Alex, not side
> step him. Drivers are rarely complex enough to warrant, or even have room for, a
> rewrite. And judging from your code it looks more like reorganising the code
> that's already there.

It is a sad truth. Instead of raising real issues that may remain in the driver, I was presented
with "non-proof" that bus-adapter-device architecture I'm using is somehow bad and the driver
should be turned into a monolithic blob, using config variables to disable unneeded functionality.
Considering, that udev handles automatic loading of the drivers just fine (so it's not an end user
issue at any rate), I don't see any justification for the change.


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

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

[Sergey Yanovich]

Alex Dubov wrote:
>> Before you get your hopes up, this development model is not one that will get
>> your code merged upstream. You should really try to work with Alex, not side
>> step him. Drivers are rarely complex enough to warrant, or even have room for, a
>> rewrite. And judging from your code it looks more like reorganising the code
>> that's already there.
>
> It is a sad truth. Instead of raising real issues that may remain in the driver, I was presented
> with "non-proof" that bus-adapter-device architecture I'm using is somehow bad and the driver
> should be turned into a monolithic blob, using config variables to disable unneeded functionality.
> Considering, that udev handles automatic loading of the drivers just fine (so it's not an end user
> issue at any rate), I don't see any justification for the change.

I may be doing something the wrong way. I absolutely don't intend to
start flames in this list.

Alex, your driver is great. You have done enormous amount of work,
reverse engineering proprietary drivers. Since the territory you work on
is unchartered, you can choose any design model, you see appropriate.

However, since we are working in an open community, everyone can give
his opinion on this issue. The commenter must definitely back his words
with real arguments. The patch at the top of this thread is such an
argument. You may or may not care about it, at will.

I have submitted my version only after I have failed to find a stable
version of your driver for the current kernel. If there is one, just
post link to the patch. If not, let's make one.

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

[Alex Dubov]

> 
> I have submitted my version only after I have failed to find a stable
> version of your driver for the current kernel. If there is one, just
> post link to the patch. If not, let's make one.
> 
As I already said, I'm not aware of any issues with version 0.8 of the driver. If you have any
problems with it, I'll be glad to fix them.

The version in question was recently committed into the -mm tree. Otherwise, it's available from
the project's website for a couple of months now:

http://developer.berlios.de/project/showfiles.php?group_id=5510


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

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

[Sergey Yanovich]

> As I already said, I'm not aware of any issues with version 0.8 of the driver. If you have any
> problems with it, I'll be glad to fix them.
>
> The version in question was recently committed into the -mm tree. Otherwise, it's available from
> the project's website for a couple of months now:

It seems a bit out-of-date. Here is the output:

build-i386-sony-tx770p$ make
scripts/kconfig/conf -s arch/i386/Kconfig
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  CHK     include/linux/compile.h
  CC [M]  drivers/misc/tifm_core.o
  CC [M]  drivers/misc/tifm_7xx1.o
  CC [M]  drivers/mmc/tifm_sd.o
drivers/mmc/tifm_sd.c: In function ‘tifm_sd_probe’:
drivers/mmc/tifm_sd.c:987: error: ‘struct mmc_host’ has no member named 
‘max_sectors’
make[2]: *** [drivers/mmc/tifm_sd.o] Error 1
make[1]: *** [drivers/mmc] Error 2
make: *** [drivers] Error 2

This is with v2.6.21 and your tarball unpacked like this:
linux/tifm.h -> include/linux/tifm.h
tifm_sd.c -> drivers/mmc/tifm_sd.c
tifm_7xx1.c -> drivers/misc/tifm_7xx1.c
tifm_core.c -> drivers/misc/tifm_core.c

I will look in mm and write back.

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

[Alex Dubov]

> It seems a bit out-of-date. Here is the output:
> 
It clearly says there that the driver is for 2.6.20. The changes needed for 2.6.21 are actually
very small (couple of fields in the mmc layer were renamed).

In general, it is impossible to maintain out-of-tree driver in sync with kernel tree - too many
changes are committed into it. The -mm version obviously fits its tree.


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

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

[Sergey Yanovich]

Alex Dubov wrote:
> In general, it is impossible to maintain out-of-tree driver in sync with kernel tree - too many
> changes are committed into it. The -mm version obviously fits its tree.
>
I have compiled v2.6.21 with git-mmc.patch of v2.6.21-rc7.mm2.
After [tifm_sd] is loaded. My smoke test script at

http://bugzilla.kernel.org/attachment.cgi?id=11240&action=view

reliably hangs suspend.

I didn't even see a failure for 'modprove -r tifm', which
doesn't exist in this build.

I will try to bisect git-mmc.patch to locate the source of
trouble. And I will compile my version to check it against
-mm tree.

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

[Alex Dubov]

> After [tifm_sd] is loaded. My smoke test script at
> 
> http://bugzilla.kernel.org/attachment.cgi?id=11240&action=view
> 
> reliably hangs suspend.
> 
What is "modprobe tifm"? What modules have you loaded when it hangs?


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

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

[Sergey Yanovich]

> What is "modprobe tifm"?

tifm is the name of the "monolithic blob" that I test :).

> What modules have you loaded when it hangs?
>

I believe is the relevant part:
~$ lsmod | grep "mmc\|tifm"
tifm_sd                11272  0
mmc_core               25812  1 tifm_sd
tifm_7xx1               6848  0
tifm_core               9876  2 tifm_sd,tifm_7xx1

Since there are [tifm_*] in lsmod; modprobe tifm is no run.

Can you successfully run this test with -mm tree? I that case
the fault may be hardware related.

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

[Sergey Yanovich]

> Can you successfully run this test with -mm tree? I that case
> the fault may be hardware related.

I have uploaded an updated version to bugzilla:
http://bugzilla.kernel.org/attachment.cgi?id=11308&action=view

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

[Pierre Ossman]

Sergey Yanovich wrote:
> I have compiled v2.6.21 with git-mmc.patch of v2.6.21-rc7.mm2.
> After [tifm_sd] is loaded. My smoke test script at
> 
> http://bugzilla.kernel.org/attachment.cgi?id=11240&action=view
> 
> reliably hangs suspend.

I really wish you would stop removing me from cc...

Suspend is broken in -mm for all controllers. There is a bit of a race between
the resume and remove functions that causes things to deadlock. I have a fix,
but I'm working on pushing a lot of stuff to Linus right now so it might take a
few days before things hit the repos.

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

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

[Alex Dubov]

--- Pierre Ossman <drzeus-list@drzeus.cx> wrote:

> Sergey Yanovich wrote:
> > I have compiled v2.6.21 with git-mmc.patch of v2.6.21-rc7.mm2.
> > After [tifm_sd] is loaded. My smoke test script at
> > 
> > http://bugzilla.kernel.org/attachment.cgi?id=11240&action=view
> > 
> > reliably hangs suspend.
> 
> I really wish you would stop removing me from cc...
> 
> Suspend is broken in -mm for all controllers. There is a bit of a race between
> the resume and remove functions that causes things to deadlock. I have a fix,
> but I'm working on pushing a lot of stuff to Linus right now so it might take a
> few days before things hit the repos.
> 

I prepared a tarball with 2.6.21 compatible driver (tifm-0.8e on berlios). I don't see any
problems on stock 2.6.21.1. You may want to test it on your machine.


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

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

[Sergey Yanovich]

Alex Dubov wrote:
> I prepared a tarball with 2.6.21 compatible driver (tifm-0.8e on berlios).
This is "good enough" to withdraw my patch. I have filed it to
http://bugzilla.kernel.org/attachment.cgi?id=11312&action=view

in bug 8052
http://bugzilla.kernel.org/show_bug.cgi?id=8052

which seems to be resolved by the patch.

 > I don't see any problems on stock 2.6.21.1.


Well, this version seem to ignore cards which are present at
load.

I have this output:
~$ check once
Loading module tifm_sd ...
Checking for a card at /dev/mmcblk0p1 ...
failed.

for this script:
http://bugzilla.kernel.org/attachment.cgi?id=11317&action=view

and you can can see this bug for more details:
http://bugzilla.kernel.org/show_bug.cgi?id=8393