strange problem with ricoh-mmc

strange problem with ricoh-mmc

[Maxim Levitsky]

Hi,

Maybe this is my fault, don't know, but I found and now confident that
new and possible old version of ricoh-mmc causes troubles after
suspend/resume.

Namely there are two problems.
One is that sometimes card detection on xD controller stops working.
That is it doesn't respond to insert/remove events, and suspend to ram
just updates that once. Granted I wrote this driver, but it appears to
work without richoh-mmc.

Another problem that is unrelated to xD is that sometimes sdhci
conroller issues an interrupt storm, and does so until its driver is
reloaded. At that point it refuses to load with missing voltage levels.

I never seen these without ricoh-mmc pci quirk.

Both problems are semi-rare.

So just one question, did you see these problems?
Do you have a clue on how to proceed?


Best regards,
Maxim Levitsky

Re: strange problem with ricoh-mmc

[Philip Langdale]

On Wed, 02 Jun 2010 21:56:58 +0300, Maxim Levitsky
<maximlevitsky@gmail.com> wrote:
> Hi,
> 
> Maybe this is my fault, don't know, but I found and now confident that
> new and possible old version of ricoh-mmc causes troubles after
> suspend/resume.
> 
> Namely there are two problems.
> One is that sometimes card detection on xD controller stops working.
> That is it doesn't respond to insert/remove events, and suspend to ram
> just updates that once. Granted I wrote this driver, but it appears to
> work without richoh-mmc.
> 
> Another problem that is unrelated to xD is that sometimes sdhci
> conroller issues an interrupt storm, and does so until its driver is
> reloaded. At that point it refuses to load with missing voltage levels.
> 
> I never seen these without ricoh-mmc pci quirk.
> 
> Both problems are semi-rare.
> 
> So just one question, did you see these problems?
> Do you have a clue on how to proceed?

I have never seen these problems, but I could imagine that perhaps the
PCI register pokes have side-effects or there's actually more that needs
to be done than what we currently have. No one has access to the relevant
datasheets so the magic incantations are second hand. More worryingly, it
may not be a well supported configuration - while the other controllers
might be disabled based on physical slot design, SD and MMC are the same,
so I doubt Ricoh or OEMs have ever seriously considered having only one
on. I've not heard of the sdhci problem before - maybe your hardware is
dodgy? :-)

--phil

Re: strange problem with ricoh-mmc

[Maxim Levitsky]

On Wed, 2010-06-02 at 15:54 -0400, Philip Langdale wrote: 
> On Wed, 02 Jun 2010 21:56:58 +0300, Maxim Levitsky
> <maximlevitsky@gmail.com> wrote:
> > Hi,
> > 
> > Maybe this is my fault, don't know, but I found and now confident that
> > new and possible old version of ricoh-mmc causes troubles after
> > suspend/resume.
> > 
> > Namely there are two problems.
> > One is that sometimes card detection on xD controller stops working.
> > That is it doesn't respond to insert/remove events, and suspend to ram
> > just updates that once. Granted I wrote this driver, but it appears to
> > work without richoh-mmc.
> > 
> > Another problem that is unrelated to xD is that sometimes sdhci
> > conroller issues an interrupt storm, and does so until its driver is
> > reloaded. At that point it refuses to load with missing voltage levels.
> > 
> > I never seen these without ricoh-mmc pci quirk.
> > 
> > Both problems are semi-rare.
> > 
> > So just one question, did you see these problems?
> > Do you have a clue on how to proceed?
> 
> I have never seen these problems, but I could imagine that perhaps the
> PCI register pokes have side-effects or there's actually more that needs
> to be done than what we currently have. No one has access to the relevant
> datasheets so the magic incantations are second hand. More worryingly, it
> may not be a well supported configuration - while the other controllers
> might be disabled based on physical slot design, SD and MMC are the same,
> so I doubt Ricoh or OEMs have ever seriously considered having only one
> on. I've not heard of the sdhci problem before - maybe your hardware is
> dodgy? :-)


Thanks.
I some future I maybe consider reverse engineering  the MMC controller.

Best regards,
Maxim Levitsky

Re: strange problem with ricoh-mmc

[Philip Langdale]

On Wed, 02 Jun 2010 23:19:25 +0300, Maxim Levitsky
<maximlevitsky@gmail.com> wrote:
> 
> Thanks.
> I some future I maybe consider reverse engineering  the MMC controller.

Someone actually did, if you dig back through the SDHCI mailing list. It's
apparently almost identical to SDHCI with the Cap flags not set properly.

http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html

Still, even if we got it working properly, it would be rather less elegant
than what we have today, modulo hardware freaking out as you are
experiencing...

--phil

Re: strange problem with ricoh-mmc

[Maxim Levitsky]

On Wed, 2010-06-02 at 16:42 -0400, Philip Langdale wrote: 
> On Wed, 02 Jun 2010 23:19:25 +0300, Maxim Levitsky
> <maximlevitsky@gmail.com> wrote:
> > 
> > Thanks.
> > I some future I maybe consider reverse engineering  the MMC controller.
> 
> Someone actually did, if you dig back through the SDHCI mailing list. It's
> apparently almost identical to SDHCI with the Cap flags not set properly.
This is just great.

Maybe its even possible to make SDHCI use it.

> 
> http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html
> 
> Still, even if we got it working properly, it would be rather less elegant
> than what we have today, modulo hardware freaking out as you are
> experiencing...
The curreent version is very elegant....
Like once you resume the system, insert card and is out of ideas of what
to do next....

Thank you very much!


Best regards,
Maxim Levitsky

[PATCH] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

The current way of disabling it is not well tested by vendor
and has all kinds of bugs that show up on resume from ram/disk.

Old way of disabling is still supported by
continuing to use CONFIG_MMC_RICOH_MMC.

Based on 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
CC: adq_dvb@lidskialf.net
CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>
---
 drivers/mmc/host/sdhci-pci.c |   34 ++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c     |    5 ++++-
 drivers/mmc/host/sdhci.h     |    2 ++
 3 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 65483fd..3843780 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/device.h>
 
 #include <linux/mmc/host.h>
 
@@ -85,6 +86,26 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
 	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
 		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
 
+	/* To disable hardware races against MMC part */
+	device_disable_async_suspend(&chip->pci_dev->dev);
+	return 0;
+}
+
+static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
+{
+	slot->host->caps =
+		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
+			& SDHCI_TIMEOUT_CLK_MASK) |
+
+		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
+			& SDHCI_CLOCK_BASE_MASK) |
+
+		SDHCI_TIMEOUT_CLK_UNIT |
+		SDHCI_CAN_VDD_330 |
+		SDHCI_CAN_DO_SDMA ;
+
+	/* To disable hardware races against SDHC part */
+	device_disable_async_suspend(&slot->chip->pci_dev->dev);
 	return 0;
 }
 
@@ -95,6 +116,11 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
 			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
 };
 
+static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
+	.probe_slot	= ricoh_mmc_probe_slot,
+	.quirks		= SDHCI_QUIRK_NO_CARD_NO_RESET,
+};
+
 static const struct sdhci_pci_fixes sdhci_ene_712 = {
 	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
 			  SDHCI_QUIRK_BROKEN_DMA,
@@ -374,6 +400,14 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	},
 
 	{
+		.vendor         = PCI_VENDOR_ID_RICOH,
+		.device         = 0x843,
+		.subvendor      = PCI_ANY_ID,
+		.subdevice      = PCI_ANY_ID,
+		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
+	},
+
+	{
 		.vendor		= PCI_VENDOR_ID_ENE,
 		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
 		.subvendor	= PCI_ANY_ID,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6d1bd8..dbd9367 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1687,7 +1687,10 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->version);
 	}
 
-	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	if (host->caps)
+		caps = host->caps;
+	else
+		caps = sdhci_readl(host, SDHCI_CAPABILITIES);
 
 	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
 		host->flags |= SDHCI_USE_SDMA;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c846813..b41581a 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -292,6 +292,8 @@ struct sdhci_host {
 
 	struct timer_list	timer;		/* Timer for timeouts */
 
+	unsigned int		caps;		/* Alternative capabilities */
+
 	unsigned long		private[0] ____cacheline_aligned;
 };
 
-- 
1.7.0.4

Re: strange problem with ricoh-mmc

[Maxim Levitsky]

On Thu, 2010-06-03 at 01:03 +0300, Maxim Levitsky wrote: 
> On Wed, 2010-06-02 at 16:42 -0400, Philip Langdale wrote: 
> > On Wed, 02 Jun 2010 23:19:25 +0300, Maxim Levitsky
> > <maximlevitsky@gmail.com> wrote:
> > > 
> > > Thanks.
> > > I some future I maybe consider reverse engineering  the MMC controller.
> > 
> > Someone actually did, if you dig back through the SDHCI mailing list. It's
> > apparently almost identical to SDHCI with the Cap flags not set properly.
> This is just great.
> 
> Maybe its even possible to make SDHCI use it.
> 
> > 
> > http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html
> > 
> > Still, even if we got it working properly, it would be rather less elegant
> > than what we have today, modulo hardware freaking out as you are
> > experiencing...
> The curreent version is very elegant....
> Like once you resume the system, insert card and is out of ideas of what
> to do next....
> 
> Thank you very much!
The patch I just send works here almost perfect.
Well, the asynchronous tries now to resume both sdhci devices in
parallel, and that makes them very unhappy.
Simple solution is just to exclude these two devices from async suspend,
but it seem not to work. I asked that at linux-pm. I am sure that will
be fixed. Anyway disabling async suspend works around this issue.

I wish I knew that this 'proprietary' mmc controller is just a SDHCI in
disguise before....

Big thanks to 'Andrew de Quincey' for this work.

Best regards,
Maxim Levitsky

Re: [PATCH] mmc: make sdhci work with ricoh mmc controller

[Philip Langdale]

On Thu,  3 Jun 2010 04:16:27 +0300
Maxim Levitsky <maximlevitsky@gmail.com> wrote:

> The current way of disabling it is not well tested by vendor
> and has all kinds of bugs that show up on resume from ram/disk.
> 
> Old way of disabling is still supported by
> continuing to use CONFIG_MMC_RICOH_MMC.
>
> Based on
> 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'

As long as this doesn't limit the performance of MMC cards, I can't
complain.

BTW, the new PCIe native controllers from Ricoh don't require the
MMC controller to be disabled - the SD controller sees the cards by
default; I guess some bit has to be twiddled by the MMC driver.

> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> CC: adq_dvb@lidskialf.net
> CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>
> ---
>  drivers/mmc/host/sdhci-pci.c |   34
> ++++++++++++++++++++++++++++++++++ drivers/mmc/host/sdhci.c     |
> 5 ++++- drivers/mmc/host/sdhci.h     |    2 ++
>  3 files changed, 40 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c
> b/drivers/mmc/host/sdhci-pci.c index 65483fd..3843780 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -17,6 +17,7 @@
>  #include <linux/pci.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/slab.h>
> +#include <linux/device.h>
>  
>  #include <linux/mmc/host.h>
>  
> @@ -85,6 +86,26 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
>  	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
>  		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
>  
> +	/* To disable hardware races against MMC part */
> +	device_disable_async_suspend(&chip->pci_dev->dev);
> +	return 0;
> +}

It would be nice if this could be more selective so it only happens
if it's really needed.

> +
> +static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
> +{
> +	slot->host->caps =
> +		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
> +			& SDHCI_TIMEOUT_CLK_MASK) |
> +
> +		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
> +			& SDHCI_CLOCK_BASE_MASK) |
> +
> +		SDHCI_TIMEOUT_CLK_UNIT |
> +		SDHCI_CAN_VDD_330 |
> +		SDHCI_CAN_DO_SDMA ;

Have you been able to establish if 4bit and high-speed operations
work correctly through the MMC controller? I note that you didn't
set SDHCI_CAN_DO_HISPD.

> +
> +	/* To disable hardware races against SDHC part */
> +	device_disable_async_suspend(&slot->chip->pci_dev->dev);
>  	return 0;
>  }
>  
> @@ -95,6 +116,11 @@ static const struct sdhci_pci_fixes sdhci_ricoh =
> { SDHCI_QUIRK_CLOCK_BEFORE_RESET,
>  };
>  
> +static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
> +	.probe_slot	= ricoh_mmc_probe_slot,
> +	.quirks		= SDHCI_QUIRK_NO_CARD_NO_RESET,
> +};
> +
>  static const struct sdhci_pci_fixes sdhci_ene_712 = {
>  	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
>  			  SDHCI_QUIRK_BROKEN_DMA,
> @@ -374,6 +400,14 @@ static const struct pci_device_id pci_ids[]
> __devinitdata = { },
>  
>  	{
> +		.vendor         = PCI_VENDOR_ID_RICOH,
> +		.device         = 0x843,
> +		.subvendor      = PCI_ANY_ID,
> +		.subdevice      = PCI_ANY_ID,
> +		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
> +	},
> +
> +	{
>  		.vendor		= PCI_VENDOR_ID_ENE,
>  		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
>  		.subvendor	= PCI_ANY_ID,
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c6d1bd8..dbd9367 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1687,7 +1687,10 @@ int sdhci_add_host(struct sdhci_host *host)
>  			host->version);
>  	}
>  
> -	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> +	if (host->caps)
> +		caps = host->caps;
> +	else
> +		caps = sdhci_readl(host, SDHCI_CAPABILITIES);

I'd prefer keying this off an explicit quirk.

>  
>  	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
>  		host->flags |= SDHCI_USE_SDMA;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index c846813..b41581a 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -292,6 +292,8 @@ struct sdhci_host {
>  
>  	struct timer_list	timer;		/* Timer for
> timeouts */ 
> +	unsigned int		caps;		/*
> Alternative capabilities */ +
>  	unsigned long		private[0]
> ____cacheline_aligned; };
>  

--phil

Re: [PATCH] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

On Thu, 2010-06-03 at 09:11 -0700, Philip Langdale wrote: 
> On Thu,  3 Jun 2010 04:16:27 +0300
> Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> 
> > The current way of disabling it is not well tested by vendor
> > and has all kinds of bugs that show up on resume from ram/disk.
> > 
> > Old way of disabling is still supported by
> > continuing to use CONFIG_MMC_RICOH_MMC.
> >
> > Based on
> > 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
> 
> As long as this doesn't limit the performance of MMC cards, I can't
> complain.
> 
> BTW, the new PCIe native controllers from Ricoh don't require the
> MMC controller to be disabled - the SD controller sees the cards by
> default; I guess some bit has to be twiddled by the MMC driver.
Can I distinguish between this and newer controllers.
Could you help me with that?


> 
> > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > CC: adq_dvb@lidskialf.net
> > CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>
> > ---
> >  drivers/mmc/host/sdhci-pci.c |   34
> > ++++++++++++++++++++++++++++++++++ drivers/mmc/host/sdhci.c     |
> > 5 ++++- drivers/mmc/host/sdhci.h     |    2 ++
> >  3 files changed, 40 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-pci.c
> > b/drivers/mmc/host/sdhci-pci.c index 65483fd..3843780 100644
> > --- a/drivers/mmc/host/sdhci-pci.c
> > +++ b/drivers/mmc/host/sdhci-pci.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/slab.h>
> > +#include <linux/device.h>
> >  
> >  #include <linux/mmc/host.h>
> >  
> > @@ -85,6 +86,26 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
> >  	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
> >  		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
> >  
> > +	/* To disable hardware races against MMC part */
> > +	device_disable_async_suspend(&chip->pci_dev->dev);
> > +	return 0;
> > +}
> 
> It would be nice if this could be more selective so it only happens
> if it's really needed.
Same as above

> 
> > +
> > +static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
> > +{
> > +	slot->host->caps =
> > +		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
> > +			& SDHCI_TIMEOUT_CLK_MASK) |
> > +
> > +		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
> > +			& SDHCI_CLOCK_BASE_MASK) |
> > +
> > +		SDHCI_TIMEOUT_CLK_UNIT |
> > +		SDHCI_CAN_VDD_330 |
> > +		SDHCI_CAN_DO_SDMA ;
> 
> Have you been able to establish if 4bit and high-speed operations
> work correctly through the MMC controller? I note that you didn't
> set SDHCI_CAN_DO_HISPD.
Didn't test that yet, will do.
I hope my MMCPlus card can do high-speed.

> 
> > +
> > +	/* To disable hardware races against SDHC part */
> > +	device_disable_async_suspend(&slot->chip->pci_dev->dev);
> >  	return 0;
> >  }
> >  
> > @@ -95,6 +116,11 @@ static const struct sdhci_pci_fixes sdhci_ricoh =
> > { SDHCI_QUIRK_CLOCK_BEFORE_RESET,
> >  };
> >  
> > +static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
> > +	.probe_slot	= ricoh_mmc_probe_slot,
> > +	.quirks		= SDHCI_QUIRK_NO_CARD_NO_RESET,
> > +};
> > +
> >  static const struct sdhci_pci_fixes sdhci_ene_712 = {
> >  	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
> >  			  SDHCI_QUIRK_BROKEN_DMA,
> > @@ -374,6 +400,14 @@ static const struct pci_device_id pci_ids[]
> > __devinitdata = { },
> >  
> >  	{
> > +		.vendor         = PCI_VENDOR_ID_RICOH,
> > +		.device         = 0x843,
> > +		.subvendor      = PCI_ANY_ID,
> > +		.subdevice      = PCI_ANY_ID,
> > +		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
> > +	},
> > +
> > +	{
> >  		.vendor		= PCI_VENDOR_ID_ENE,
> >  		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
> >  		.subvendor	= PCI_ANY_ID,
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index c6d1bd8..dbd9367 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1687,7 +1687,10 @@ int sdhci_add_host(struct sdhci_host *host)
> >  			host->version);
> >  	}
> >  
> > -	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> > +	if (host->caps)
> > +		caps = host->caps;
> > +	else
> > +		caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> 
> I'd prefer keying this off an explicit quirk.
Could you explain?

Do you mean I should add SDHCI_QUIRK_MISSING_CAPS ?
This is fine.

If you mean that I should create a SDHCI_QUIRK_RICOH_MMC_CAPS,
and hardcode caps in sdhci, I kind of disagree, too ugly :-)


Btw, suspend/resume races seem to disappear. Maybe I didn't install the
kernel (will test more) 
> >  
> >  	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
> >  		host->flags |= SDHCI_USE_SDMA;
> > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > index c846813..b41581a 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -292,6 +292,8 @@ struct sdhci_host {
> >  
> >  	struct timer_list	timer;		/* Timer for
> > timeouts */ 
> > +	unsigned int		caps;		/*
> > Alternative capabilities */ +
> >  	unsigned long		private[0]
> > ____cacheline_aligned; };
> >  
> 
> --phil


Best regards,
Maxim Levitsky

Re: [PATCH] mmc: make sdhci work with ricoh mmc controller

[Philip Langdale]

On Thu, 03 Jun 2010 19:31:49 +0300
Maxim Levitsky <maximlevitsky@gmail.com> wrote:

> On Thu, 2010-06-03 at 09:11 -0700, Philip Langdale wrote: 
> > On Thu,  3 Jun 2010 04:16:27 +0300
> > Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > 
> > > The current way of disabling it is not well tested by vendor
> > > and has all kinds of bugs that show up on resume from ram/disk.
> > > 
> > > Old way of disabling is still supported by
> > > continuing to use CONFIG_MMC_RICOH_MMC.
> > >
> > > Based on
> > > 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
> > 
> > As long as this doesn't limit the performance of MMC cards, I can't
> > complain.
> > 
> > BTW, the new PCIe native controllers from Ricoh don't require the
> > MMC controller to be disabled - the SD controller sees the cards by
> > default; I guess some bit has to be twiddled by the MMC driver.
> Can I distinguish between this and newer controllers.
> Could you help me with that?

I'll check in on it tomorrow - I don't have access to a relevant machine
today, but I'm pretty sure they changed the PCI ID so it won't be a
problem.

> 
> > 
> > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > > CC: adq_dvb@lidskialf.net
> > > CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>
> > > ---
> > >  drivers/mmc/host/sdhci-pci.c |   34
> > > ++++++++++++++++++++++++++++++++++ drivers/mmc/host/sdhci.c     |
> > > 5 ++++- drivers/mmc/host/sdhci.h     |    2 ++
> > >  3 files changed, 40 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/sdhci-pci.c
> > > b/drivers/mmc/host/sdhci-pci.c index 65483fd..3843780 100644
> > > --- a/drivers/mmc/host/sdhci-pci.c
> > > +++ b/drivers/mmc/host/sdhci-pci.c
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/pci.h>
> > >  #include <linux/dma-mapping.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/device.h>
> > >  
> > >  #include <linux/mmc/host.h>
> > >  
> > > @@ -85,6 +86,26 @@ static int ricoh_probe(struct sdhci_pci_chip
> > > *chip) chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
> > >  		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
> > >  
> > > +	/* To disable hardware races against MMC part */
> > > +	device_disable_async_suspend(&chip->pci_dev->dev);
> > > +	return 0;
> > > +}
> > 
> > It would be nice if this could be more selective so it only happens
> > if it's really needed.
> Same as above

Not sure how to do it in an elegant way. The probe function could search
the PCI bus for the other device - ugh. But if you can't reproduce the
race, then we don't need to worry.

> > 
> > > +
> > > +static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
> > > +{
> > > +	slot->host->caps =
> > > +		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
> > > +			& SDHCI_TIMEOUT_CLK_MASK) |
> > > +
> > > +		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
> > > +			& SDHCI_CLOCK_BASE_MASK) |
> > > +
> > > +		SDHCI_TIMEOUT_CLK_UNIT |
> > > +		SDHCI_CAN_VDD_330 |
> > > +		SDHCI_CAN_DO_SDMA ;
> > 
> > Have you been able to establish if 4bit and high-speed operations
> > work correctly through the MMC controller? I note that you didn't
> > set SDHCI_CAN_DO_HISPD.
> Didn't test that yet, will do.
> I hope my MMCPlus card can do high-speed.

I should get a chance today to test this as well; I'll let you know
what I see.

> > 
> > > +
> > > +	/* To disable hardware races against SDHC part */
> > > +	device_disable_async_suspend(&slot->chip->pci_dev->dev);
> > >  	return 0;
> > >  }
> > >  
> > > @@ -95,6 +116,11 @@ static const struct sdhci_pci_fixes
> > > sdhci_ricoh = { SDHCI_QUIRK_CLOCK_BEFORE_RESET,
> > >  };
> > >  
> > > +static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
> > > +	.probe_slot	= ricoh_mmc_probe_slot,
> > > +	.quirks		= SDHCI_QUIRK_NO_CARD_NO_RESET,
> > > +};
> > > +
> > >  static const struct sdhci_pci_fixes sdhci_ene_712 = {
> > >  	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
> > >  			  SDHCI_QUIRK_BROKEN_DMA,
> > > @@ -374,6 +400,14 @@ static const struct pci_device_id pci_ids[]
> > > __devinitdata = { },
> > >  
> > >  	{
> > > +		.vendor         = PCI_VENDOR_ID_RICOH,
> > > +		.device         = 0x843,
> > > +		.subvendor      = PCI_ANY_ID,
> > > +		.subdevice      = PCI_ANY_ID,
> > > +		.driver_data    =
> > > (kernel_ulong_t)&sdhci_ricoh_mmc,
> > > +	},
> > > +
> > > +	{
> > >  		.vendor		= PCI_VENDOR_ID_ENE,
> > >  		.device		=
> > > PCI_DEVICE_ID_ENE_CB712_SD, .subvendor	= PCI_ANY_ID,
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > > index c6d1bd8..dbd9367 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -1687,7 +1687,10 @@ int sdhci_add_host(struct sdhci_host *host)
> > >  			host->version);
> > >  	}
> > >  
> > > -	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> > > +	if (host->caps)
> > > +		caps = host->caps;
> > > +	else
> > > +		caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> > 
> > I'd prefer keying this off an explicit quirk.
> Could you explain?
> 
> Do you mean I should add SDHCI_QUIRK_MISSING_CAPS ?
> This is fine.
> 
> If you mean that I should create a SDHCI_QUIRK_RICOH_MMC_CAPS,
> and hardcode caps in sdhci, I kind of disagree, too ugly :-)

I meant SDHCI_QUIRK_MISSING_CAPS. :-)

Use host->caps but only when the QUIRK is set.

> 
> Btw, suspend/resume races seem to disappear. Maybe I didn't install
> the kernel (will test more) 
> > >  
> > >  	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
> > >  		host->flags |= SDHCI_USE_SDMA;
> > > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > > index c846813..b41581a 100644
> > > --- a/drivers/mmc/host/sdhci.h
> > > +++ b/drivers/mmc/host/sdhci.h
> > > @@ -292,6 +292,8 @@ struct sdhci_host {
> > >  
> > >  	struct timer_list	timer;		/* Timer
> > > for timeouts */ 
> > > +	unsigned int		caps;		/*
> > > Alternative capabilities */ +
> > >  	unsigned long		private[0]
> > > ____cacheline_aligned; };
> > >  
> > 
> > --phil
> 
> 
> Best regards,
> Maxim Levitsky
> 
> 

--phil

Re: [PATCH] mmc: make sdhci work with ricoh mmc controller

[Philip Langdale]

On Thu, 3 Jun 2010 09:39:14 -0700
Philip Langdale <philipl@overt.org> wrote:

> > > 
> > > Have you been able to establish if 4bit and high-speed operations
> > > work correctly through the MMC controller? I note that you didn't
> > > set SDHCI_CAN_DO_HISPD.
> > Didn't test that yet, will do.
> > I hope my MMCPlus card can do high-speed.
> 
> I should get a chance today to test this as well; I'll let you know
> what I see.
> 

Ok, I was able to try it out and setting HISPD works and 4bit seems
to work too. My MMCplus cards run at the same speed with either
controller.

--phil

Re: [PATCH] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

On Thu, 2010-06-03 at 10:05 -0700, Philip Langdale wrote: 
> On Thu, 3 Jun 2010 09:39:14 -0700
> Philip Langdale <philipl@overt.org> wrote:
> 
> > > > 
> > > > Have you been able to establish if 4bit and high-speed operations
> > > > work correctly through the MMC controller? I note that you didn't
> > > > set SDHCI_CAN_DO_HISPD.
> > > Didn't test that yet, will do.
> > > I hope my MMCPlus card can do high-speed.
> > 
> > I should get a chance today to test this as well; I'll let you know
> > what I see.
> > 
> 
> Ok, I was able to try it out and setting HISPD works and 4bit seems
> to work too. My MMCplus cards run at the same speed with either
> controller.
I too confirm that.

However that suspend/resume race is tough one.
The problem seems that controller doesn't like both devices to be poked
at same time, and normally they won't, but here on resume both are
tested for a card, and this is done asynchronously by mmc core.

I will get to bottom of this sooner or later (I hope).

Best regards,
Maxim Levitsky

Re: [PATCH] mmc: make sdhci work with ricoh mmc controller

[Philip Langdale]

On Thu, 03 Jun 2010 20:35:31 +0300
Maxim Levitsky <maximlevitsky@gmail.com> wrote:

> On Thu, 2010-06-03 at 10:05 -0700, Philip Langdale wrote: 
> > On Thu, 3 Jun 2010 09:39:14 -0700
> > Philip Langdale <philipl@overt.org> wrote:
> > 
> > > > > 
> > > > > Have you been able to establish if 4bit and high-speed
> > > > > operations work correctly through the MMC controller? I note
> > > > > that you didn't set SDHCI_CAN_DO_HISPD.
> > > > Didn't test that yet, will do.
> > > > I hope my MMCPlus card can do high-speed.
> > > 
> > > I should get a chance today to test this as well; I'll let you
> > > know what I see.
> > > 
> > 
> > Ok, I was able to try it out and setting HISPD works and 4bit seems
> > to work too. My MMCplus cards run at the same speed with either
> > controller.
> I too confirm that.

On this subject:

1) Would it make sense to have the hard-coded caps reflect the full
set of caps you see on the sdhci side?

2) We ought to be able to set the MMC high-speed flag for this
controller; I've tried it out and it works fine. The default sdhci
code will never set this flag. I think it would need to an additional
quirk. Pierre argued against setting it on the basis that SD high speed
has slightly different timings; I haven't seen hardware where this
has been an issue.

> However that suspend/resume race is tough one.
> The problem seems that controller doesn't like both devices to be
> poked at same time, and normally they won't, but here on resume both
> are tested for a card, and this is done asynchronously by mmc core.
> 
> I will get to bottom of this sooner or later (I hope).

Hmm. So, if the issue is the test, then you should be able to serialize
in mmc core instead of forcing sync resume in general. An ugly way would
be a quirk that says to serialize all card tests if the controller is
present in the system. In practice it would be fine as systems won't
have arbitrary other sdhci controllers if they have this ricoh mmc
thing. But yes, it isn't clean. :-P

--phil

Re: [PATCH] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

On Thu, 2010-06-03 at 21:42 -0700, Philip Langdale wrote: 
> On Thu, 03 Jun 2010 20:35:31 +0300
> Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> 
> > On Thu, 2010-06-03 at 10:05 -0700, Philip Langdale wrote: 
> > > On Thu, 3 Jun 2010 09:39:14 -0700
> > > Philip Langdale <philipl@overt.org> wrote:
> > > 
> > > > > > 
> > > > > > Have you been able to establish if 4bit and high-speed
> > > > > > operations work correctly through the MMC controller? I note
> > > > > > that you didn't set SDHCI_CAN_DO_HISPD.
> > > > > Didn't test that yet, will do.
> > > > > I hope my MMCPlus card can do high-speed.
> > > > 
> > > > I should get a chance today to test this as well; I'll let you
> > > > know what I see.
> > > > 
> > > 
> > > Ok, I was able to try it out and setting HISPD works and 4bit seems
> > > to work too. My MMCplus cards run at the same speed with either
> > > controller.
> > I too confirm that.
> 
> On this subject:
> 
> 1) Would it make sense to have the hard-coded caps reflect the full
> set of caps you see on the sdhci side?
This would be ugly cause two driver instances would have to talk one
with another. A global variable will be necessary.


> 
> 2) We ought to be able to set the MMC high-speed flag for this
> controller; I've tried it out and it works fine. The default sdhci
> code will never set this flag. I think it would need to an additional
> quirk. Pierre argued against setting it on the basis that SD high speed
> has slightly different timings; I haven't seen hardware where this
> has been an issue.
> 
> > However that suspend/resume race is tough one.
> > The problem seems that controller doesn't like both devices to be
> > poked at same time, and normally they won't, but here on resume both
> > are tested for a card, and this is done asynchronously by mmc core.
> > 
> > I will get to bottom of this sooner or later (I hope).
> 
> Hmm. So, if the issue is the test, then you should be able to serialize
> in mmc core instead of forcing sync resume in general. An ugly way would
> be a quirk that says to serialize all card tests if the controller is
> present in the system. In practice it would be fine as systems won't
> have arbitrary other sdhci controllers if they have this ricoh mmc
> thing. But yes, it isn't clean. :-P
This seems to be worse that I thought.
The problem is that mmc controller tells that card is removed on resume
(after a while it reappears)

This brings a question though, are MMC and SD cards electrically
different?
If not them its interesting how the controller distinguishes between
them.


This isn't a show stopper though, cause the cards are removed/reinserted
anyway unless CONFIG_MMC_UNSAFE_RESUME is set.
The fact that this triggers system hang is another story, and sooner or
later will be fixed ether by some hack in mmc code or by making
del_gendisk not hang when userspace is frozen.


It not due to interleaving, because I tried binding sdhci-pci to only
mmc interface, and yet same problem happens.

Magically, if async suspend is disabled everything works, and it well
tested. and that despite me disabling async suspend on all 4 functions.
(And I know that this works, and makes pm core suspend them in order
from 0 to 4 and synchronously.
I tried adding large delays to simulate delays caused by waiting for
other devices, but it didn't help.

I''l get to bottom of this.



> 
> --phil

Re: [PATCH] mmc: make sdhci work with ricoh mmc controller

[Philip Langdale]

On Fri, 04 Jun 2010 13:07:28 +0300
Maxim Levitsky <maximlevitsky@gmail.com> wrote:


> > 
> > On this subject:
> > 
> > 1) Would it make sense to have the hard-coded caps reflect the full
> > set of caps you see on the sdhci side?
> This would be ugly cause two driver instances would have to talk one
> with another. A global variable will be necessary.

Instead of doing it at runtime, we could read them offline and then
write them in. Of course, I suspect there's some variation in
capabilities of Ricoh parts - I know there's at least two generations
of MMC controllers with the same PCI IDs. :-/
 
> 
> > 
> > 2) We ought to be able to set the MMC high-speed flag for this
> > controller; I've tried it out and it works fine. The default sdhci
> > code will never set this flag. I think it would need to an
> > additional quirk. Pierre argued against setting it on the basis
> > that SD high speed has slightly different timings; I haven't seen
> > hardware where this has been an issue.
> > 
> > > However that suspend/resume race is tough one.
> > > The problem seems that controller doesn't like both devices to be
> > > poked at same time, and normally they won't, but here on resume
> > > both are tested for a card, and this is done asynchronously by
> > > mmc core.
> > > 
> > > I will get to bottom of this sooner or later (I hope).
> > 
> > Hmm. So, if the issue is the test, then you should be able to
> > serialize in mmc core instead of forcing sync resume in general. An
> > ugly way would be a quirk that says to serialize all card tests if
> > the controller is present in the system. In practice it would be
> > fine as systems won't have arbitrary other sdhci controllers if
> > they have this ricoh mmc thing. But yes, it isn't clean. :-P
> This seems to be worse that I thought.
> The problem is that mmc controller tells that card is removed on
> resume (after a while it reappears)
> 
> This brings a question though, are MMC and SD cards electrically
> different?
> If not them its interesting how the controller distinguishes between
> them.

They are not. I suspect the controller pokes the card using SPI mode
just enough to tell them apart.
 
> 
> This isn't a show stopper though, cause the cards are
> removed/reinserted anyway unless CONFIG_MMC_UNSAFE_RESUME is set.
> The fact that this triggers system hang is another story, and sooner
> or later will be fixed ether by some hack in mmc code or by making
> del_gendisk not hang when userspace is frozen.
> 
> 
> It not due to interleaving, because I tried binding sdhci-pci to only
> mmc interface, and yet same problem happens.
> 
> Magically, if async suspend is disabled everything works, and it well
> tested. and that despite me disabling async suspend on all 4
> functions. (And I know that this works, and makes pm core suspend
> them in order from 0 to 4 and synchronously.
> I tried adding large delays to simulate delays caused by waiting for
> other devices, but it didn't help.
> 
> I''l get to bottom of this.

I'm not familiar with what pm core allows but can you tell it to
serialise the functions but still handle the device asynchronously?
That would still allow the maximum possible asynchrony.

> 
> 
> > 
> > --phil
> 
> 
> 




--phil

Re: [PATCH] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

On Fri, 2010-06-04 at 08:05 -0700, Philip Langdale wrote: 
> On Fri, 04 Jun 2010 13:07:28 +0300
> Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> 
> 
> > > 
> > > On this subject:
> > > 
> > > 1) Would it make sense to have the hard-coded caps reflect the full
> > > set of caps you see on the sdhci side?
> > This would be ugly cause two driver instances would have to talk one
> > with another. A global variable will be necessary.
> 
> Instead of doing it at runtime, we could read them offline and then
> write them in. Of course, I suspect there's some variation in
> capabilities of Ricoh parts - I know there's at least two generations
> of MMC controllers with the same PCI IDs. :-/
Nothing against that, although the caps on main controller are broken
too, since it doesn't advertise DMA support...

> 
> > 
> > > 
> > > 2) We ought to be able to set the MMC high-speed flag for this
> > > controller; I've tried it out and it works fine. The default sdhci
> > > code will never set this flag. I think it would need to an
> > > additional quirk. Pierre argued against setting it on the basis
> > > that SD high speed has slightly different timings; I haven't seen
> > > hardware where this has been an issue.
> > > 
> > > > However that suspend/resume race is tough one.
> > > > The problem seems that controller doesn't like both devices to be
> > > > poked at same time, and normally they won't, but here on resume
> > > > both are tested for a card, and this is done asynchronously by
> > > > mmc core.
> > > > 
> > > > I will get to bottom of this sooner or later (I hope).
> > > 
> > > Hmm. So, if the issue is the test, then you should be able to
> > > serialize in mmc core instead of forcing sync resume in general. An
> > > ugly way would be a quirk that says to serialize all card tests if
> > > the controller is present in the system. In practice it would be
> > > fine as systems won't have arbitrary other sdhci controllers if
> > > they have this ricoh mmc thing. But yes, it isn't clean. :-P
> > This seems to be worse that I thought.
> > The problem is that mmc controller tells that card is removed on
> > resume (after a while it reappears)
> > 
> > This brings a question though, are MMC and SD cards electrically
> > different?
> > If not them its interesting how the controller distinguishes between
> > them.
> 
> They are not. I suspect the controller pokes the card using SPI mode
> just enough to tell them apart.
>  
> > 
> > This isn't a show stopper though, cause the cards are
> > removed/reinserted anyway unless CONFIG_MMC_UNSAFE_RESUME is set.
> > The fact that this triggers system hang is another story, and sooner
> > or later will be fixed ether by some hack in mmc code or by making
> > del_gendisk not hang when userspace is frozen.
> > 
> > 
> > It not due to interleaving, because I tried binding sdhci-pci to only
> > mmc interface, and yet same problem happens.
> > 
> > Magically, if async suspend is disabled everything works, and it well
> > tested. and that despite me disabling async suspend on all 4
> > functions. (And I know that this works, and makes pm core suspend
> > them in order from 0 to 4 and synchronously.
> > I tried adding large delays to simulate delays caused by waiting for
> > other devices, but it didn't help.
> > 
> > I''l get to bottom of this.
> 
> I'm not familiar with what pm core allows but can you tell it to
> serialise the functions but still handle the device asynchronously?
> That would still allow the maximum possible asynchrony.
I did that, and I am quite confident that its timing that makes the
difference.
(Or, and this is the worst thing to happen, there is an unrelated device
that must be enabled before mmc....)

Since controller like I suspected from the beginning pokes at the card,
it might be tricky to keep mmc card alive after suspend.
I leave this problem as is for now, and in future when I finish exams, I
am going to understand exactly what is going on.

Still, despite that feel I have just opened a can of worms, still I
think that its better to stick to the way things are done in windows
because hackish or not, things were tested by vendor well enough.

And just to think that all the trouble is literally due to
Microsoft..... grrrrrr.

Best regards,
Maxim Levitsky

Re: [PATCH] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

On Fri, 2010-06-04 at 18:33 +0300, Maxim Levitsky wrote: 
> On Fri, 2010-06-04 at 08:05 -0700, Philip Langdale wrote: 
> > On Fri, 04 Jun 2010 13:07:28 +0300
> > Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > 
> > 
> > > > 
> > > > On this subject:
> > > > 
> > > > 1) Would it make sense to have the hard-coded caps reflect the full
> > > > set of caps you see on the sdhci side?
> > > This would be ugly cause two driver instances would have to talk one
> > > with another. A global variable will be necessary.
> > 
> > Instead of doing it at runtime, we could read them offline and then
> > write them in. Of course, I suspect there's some variation in
> > capabilities of Ricoh parts - I know there's at least two generations
> > of MMC controllers with the same PCI IDs. :-/
> Nothing against that, although the caps on main controller are broken
> too, since it doesn't advertise DMA support...
> 
> > 
> > > 
> > > > 
> > > > 2) We ought to be able to set the MMC high-speed flag for this
> > > > controller; I've tried it out and it works fine. The default sdhci
> > > > code will never set this flag. I think it would need to an
> > > > additional quirk. Pierre argued against setting it on the basis
> > > > that SD high speed has slightly different timings; I haven't seen
> > > > hardware where this has been an issue.
> > > > 
> > > > > However that suspend/resume race is tough one.
> > > > > The problem seems that controller doesn't like both devices to be
> > > > > poked at same time, and normally they won't, but here on resume
> > > > > both are tested for a card, and this is done asynchronously by
> > > > > mmc core.
> > > > > 
> > > > > I will get to bottom of this sooner or later (I hope).
> > > > 
> > > > Hmm. So, if the issue is the test, then you should be able to
> > > > serialize in mmc core instead of forcing sync resume in general. An
> > > > ugly way would be a quirk that says to serialize all card tests if
> > > > the controller is present in the system. In practice it would be
> > > > fine as systems won't have arbitrary other sdhci controllers if
> > > > they have this ricoh mmc thing. But yes, it isn't clean. :-P
> > > This seems to be worse that I thought.
> > > The problem is that mmc controller tells that card is removed on
> > > resume (after a while it reappears)
> > > 
> > > This brings a question though, are MMC and SD cards electrically
> > > different?
> > > If not them its interesting how the controller distinguishes between
> > > them.
> > 
> > They are not. I suspect the controller pokes the card using SPI mode
> > just enough to tell them apart.
> >  
> > > 
> > > This isn't a show stopper though, cause the cards are
> > > removed/reinserted anyway unless CONFIG_MMC_UNSAFE_RESUME is set.
> > > The fact that this triggers system hang is another story, and sooner
> > > or later will be fixed ether by some hack in mmc code or by making
> > > del_gendisk not hang when userspace is frozen.
> > > 
> > > 
> > > It not due to interleaving, because I tried binding sdhci-pci to only
> > > mmc interface, and yet same problem happens.
> > > 
> > > Magically, if async suspend is disabled everything works, and it well
> > > tested. and that despite me disabling async suspend on all 4
> > > functions. (And I know that this works, and makes pm core suspend
> > > them in order from 0 to 4 and synchronously.
> > > I tried adding large delays to simulate delays caused by waiting for
> > > other devices, but it didn't help.
> > > 
> > > I''l get to bottom of this.
> > 
> > I'm not familiar with what pm core allows but can you tell it to
> > serialise the functions but still handle the device asynchronously?
> > That would still allow the maximum possible asynchrony.
> I did that, and I am quite confident that its timing that makes the
> difference.
> (Or, and this is the worst thing to happen, there is an unrelated device
> that must be enabled before mmc....)
> 
> Since controller like I suspected from the beginning pokes at the card,
> it might be tricky to keep mmc card alive after suspend.
> I leave this problem as is for now, and in future when I finish exams, I
> am going to understand exactly what is going on.
> 
> Still, despite that feel I have just opened a can of worms, still I
> think that its better to stick to the way things are done in windows
> because hackish or not, things were tested by vendor well enough.
> 
> And just to think that all the trouble is literally due to
> Microsoft..... grrrrrr.


Found the case.
The problem was just that we need to wait a bit for mmc device to
appear.
It probably pokes the card to see if it is mmc or not.

Now the reader is prefect.
(Well, there is still hang if card is really removed on resume due to
problem in linux (and it is pure software issue). The fix is more or
less known, so I tackle this after exams.
And of course memstick part that I more or less reverse engineer.
Again later I will write a driver.

Best regards,
Maxim Levitsky

[PATCH 1/2] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

The current way of disabling it is not well tested by vendor
and has all kinds of bugs that show up on resume from ram/disk.

Old way of disabling is still supported by
continuing to use CONFIG_MMC_RICOH_MMC.

Based on 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
Most of the credit for this goes to Andrew de Quincey


Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
CC: Andrew de Quincey <adq_dvb@lidskialf.net>
CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>
---
 drivers/mmc/host/sdhci-pci.c |   31 +++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c     |    3 ++-
 drivers/mmc/host/sdhci.h     |    4 ++++
 3 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 65483fd..c4bcaeb 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/device.h>
 
 #include <linux/mmc/host.h>
 
@@ -84,7 +85,21 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
 	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
 	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
 		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
+	return 0;
+}
+
+static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
+{
+	slot->host->caps =
+		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
+			& SDHCI_TIMEOUT_CLK_MASK) |
 
+		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
+			& SDHCI_CLOCK_BASE_MASK) |
+
+		SDHCI_TIMEOUT_CLK_UNIT |
+		SDHCI_CAN_VDD_330 |
+		SDHCI_CAN_DO_SDMA;
 	return 0;
 }
 
@@ -95,6 +110,14 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
 			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
 };
 
+static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
+	.probe_slot	= ricoh_mmc_probe_slot,
+	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
+			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
+			  SDHCI_QUIRK_NO_CARD_NO_RESET |
+			  SDHCI_QUIRK_MISSING_CAPS
+};
+
 static const struct sdhci_pci_fixes sdhci_ene_712 = {
 	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
 			  SDHCI_QUIRK_BROKEN_DMA,
@@ -374,6 +397,14 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	},
 
 	{
+		.vendor         = PCI_VENDOR_ID_RICOH,
+		.device         = 0x843,
+		.subvendor      = PCI_ANY_ID,
+		.subdevice      = PCI_ANY_ID,
+		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
+	},
+
+	{
 		.vendor		= PCI_VENDOR_ID_ENE,
 		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
 		.subvendor	= PCI_ANY_ID,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6d1bd8..483b78e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->version);
 	}
 
-	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? host->caps :
+		sdhci_readl(host, SDHCI_CAPABILITIES);
 
 	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
 		host->flags |= SDHCI_USE_SDMA;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c846813..b1839a3 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -240,6 +240,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
 /* Controller cannot support End Attribute in NOP ADMA descriptor */
 #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
+/* Controller is missing device caps. Use caps provided by host */
+#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
@@ -292,6 +294,8 @@ struct sdhci_host {
 
 	struct timer_list	timer;		/* Timer for timeouts */
 
+	unsigned int		caps;		/* Alternative capabilities */
+
 	unsigned long		private[0] ____cacheline_aligned;
 };
 
-- 
1.7.0.4

[PATCH 1/2] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

The current way of disabling it is not well tested by vendor
and has all kinds of bugs that show up on resume from ram/disk.

Old way of disabling is still supported by
continuing to use CONFIG_MMC_RICOH_MMC.

Based on 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
Most of the credit for this goes to Andrew de Quincey


Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
CC: Andrew de Quincey <adq_dvb@lidskialf.net>
CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>
---
 drivers/mmc/host/sdhci-pci.c |   31 +++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c     |    3 ++-
 drivers/mmc/host/sdhci.h     |    4 ++++
 3 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 65483fd..c4bcaeb 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/device.h>
 
 #include <linux/mmc/host.h>
 
@@ -84,7 +85,21 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
 	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
 	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
 		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
+	return 0;
+}
+
+static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
+{
+	slot->host->caps =
+		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
+			& SDHCI_TIMEOUT_CLK_MASK) |
 
+		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
+			& SDHCI_CLOCK_BASE_MASK) |
+
+		SDHCI_TIMEOUT_CLK_UNIT |
+		SDHCI_CAN_VDD_330 |
+		SDHCI_CAN_DO_SDMA;
 	return 0;
 }
 
@@ -95,6 +110,14 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
 			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
 };
 
+static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
+	.probe_slot	= ricoh_mmc_probe_slot,
+	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
+			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
+			  SDHCI_QUIRK_NO_CARD_NO_RESET |
+			  SDHCI_QUIRK_MISSING_CAPS
+};
+
 static const struct sdhci_pci_fixes sdhci_ene_712 = {
 	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
 			  SDHCI_QUIRK_BROKEN_DMA,
@@ -374,6 +397,14 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	},
 
 	{
+		.vendor         = PCI_VENDOR_ID_RICOH,
+		.device         = 0x843,
+		.subvendor      = PCI_ANY_ID,
+		.subdevice      = PCI_ANY_ID,
+		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
+	},
+
+	{
 		.vendor		= PCI_VENDOR_ID_ENE,
 		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
 		.subvendor	= PCI_ANY_ID,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6d1bd8..483b78e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->version);
 	}
 
-	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? host->caps :
+		sdhci_readl(host, SDHCI_CAPABILITIES);
 
 	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
 		host->flags |= SDHCI_USE_SDMA;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c846813..b1839a3 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -240,6 +240,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
 /* Controller cannot support End Attribute in NOP ADMA descriptor */
 #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
+/* Controller is missing device caps. Use caps provided by host */
+#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
@@ -292,6 +294,8 @@ struct sdhci_host {
 
 	struct timer_list	timer;		/* Timer for timeouts */
 
+	unsigned int		caps;		/* Alternative capabilities */
+
 	unsigned long		private[0] ____cacheline_aligned;
 };
 
-- 
1.7.0.4

[PATCH 2/2] mmc: fix mmc card disappearence on resume on ricoh mmc controllers

[Maxim Levitsky]

The reason was that it takes time for controller to detect the card.
So wait a bit for the card to appear.
In worst case scenario, when you suspend the system with mmc card,
and actually remove it, the resume will be delayed maximum by 2 seconds

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
---
 drivers/mmc/host/sdhci-pci.c |    3 ++-
 drivers/mmc/host/sdhci.c     |   18 ++++++++++++++++++
 drivers/mmc/host/sdhci.h     |    2 ++
 3 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index c4bcaeb..49d863c 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -115,7 +115,8 @@ static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
 	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
 			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
 			  SDHCI_QUIRK_NO_CARD_NO_RESET |
-			  SDHCI_QUIRK_MISSING_CAPS
+			  SDHCI_QUIRK_MISSING_CAPS |
+			  SDHCI_QUIRK_WAIT_CARD_ON_RESUME
 };
 
 static const struct sdhci_pci_fixes sdhci_ene_712 = {
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 483b78e..6cda505 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1612,6 +1612,24 @@ int sdhci_resume_host(struct sdhci_host *host)
 {
 	int ret;
 
+	/* Some controllers (especialy the ricoh mmc controller) delay
+		card detection on resume (probably since the controller
+		has to poke the card to determine if its MMC or not */
+
+	if (host->mmc->bus_ops && (host->quirks &
+			SDHCI_QUIRK_WAIT_CARD_ON_RESUME)) {
+
+		unsigned long timeout = jiffies + msecs_to_jiffies(2000);
+
+		while (!time_after(jiffies, timeout))
+			if (sdhci_readl(host, SDHCI_PRESENT_STATE)
+							& SDHCI_CARD_PRESENT) {
+				break;
+			}
+			cpu_relax();
+	}
+
+
 	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
 		if (host->ops->enable_dma)
 			host->ops->enable_dma(host);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index b1839a3..c9d099c 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -242,6 +242,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
 /* Controller is missing device caps. Use caps provided by host */
 #define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
+/* Card doesn't appear immediatly on resume from low power state */
+#define SDHCI_QUIRK_WAIT_CARD_ON_RESUME			(1<<28)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.7.0.4

Re: [PATCH 1/2] mmc: make sdhci work with ricoh mmc controller

[Philip Langdale]

On Mon,  7 Jun 2010 00:28:50 +0300, Maxim Levitsky
<maximlevitsky@gmail.com> wrote:
> The current way of disabling it is not well tested by vendor
> and has all kinds of bugs that show up on resume from ram/disk.
> 
> Old way of disabling is still supported by
> continuing to use CONFIG_MMC_RICOH_MMC.
> 
> Based on
> 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
> Most of the credit for this goes to Andrew de Quincey
> 
> 
> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> CC: Andrew de Quincey <adq_dvb@lidskialf.net>
> CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>

ACK with one change to add a #define for the device ID.

> ---
>  drivers/mmc/host/sdhci-pci.c |   31 +++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c     |    3 ++-
>  drivers/mmc/host/sdhci.h     |    4 ++++
>  3 files changed, 37 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index 65483fd..c4bcaeb 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -17,6 +17,7 @@
>  #include <linux/pci.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/slab.h>
> +#include <linux/device.h>
>  
>  #include <linux/mmc/host.h>
>  
> @@ -84,7 +85,21 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
>  	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
>  	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
>  		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
> +	return 0;
> +}
> +
> +static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
> +{
> +	slot->host->caps =
> +		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
> +			& SDHCI_TIMEOUT_CLK_MASK) |
>  
> +		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
> +			& SDHCI_CLOCK_BASE_MASK) |
> +
> +		SDHCI_TIMEOUT_CLK_UNIT |
> +		SDHCI_CAN_VDD_330 |
> +		SDHCI_CAN_DO_SDMA;
>  	return 0;
>  }

As we discussed, highspeed works. Of course, sdhci never sets the MMC
highspeed flag so the cap is irrelevant. We'd need another quirk to
indicate highspeed MMC is supported.

This can be done in a separate patch.

> @@ -95,6 +110,14 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
>  			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
>  };
>  
> +static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
> +	.probe_slot	= ricoh_mmc_probe_slot,
> +	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
> +			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
> +			  SDHCI_QUIRK_NO_CARD_NO_RESET |
> +			  SDHCI_QUIRK_MISSING_CAPS
> +};
> +
>  static const struct sdhci_pci_fixes sdhci_ene_712 = {
>  	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
>  			  SDHCI_QUIRK_BROKEN_DMA,
> @@ -374,6 +397,14 @@ static const struct pci_device_id pci_ids[]
> __devinitdata = {
>  	},
>  
>  	{
> +		.vendor         = PCI_VENDOR_ID_RICOH,
> +		.device         = 0x843,
> +		.subvendor      = PCI_ANY_ID,
> +		.subdevice      = PCI_ANY_ID,
> +		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
> +	},
> +
> +	{

It seems we generally want to add a #define for the device ID.
The ENE device below has one.

>  		.vendor		= PCI_VENDOR_ID_ENE,
>  		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
>  		.subvendor	= PCI_ANY_ID,
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c6d1bd8..483b78e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
>  			host->version);
>  	}
>  
> -	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> +	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? host->caps :
> +		sdhci_readl(host, SDHCI_CAPABILITIES);
>  
>  	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
>  		host->flags |= SDHCI_USE_SDMA;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index c846813..b1839a3 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -240,6 +240,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
>  /* Controller cannot support End Attribute in NOP ADMA descriptor */
>  #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
> +/* Controller is missing device caps. Use caps provided by host */
> +#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
>  
>  	int			irq;		/* Device IRQ */
>  	void __iomem *		ioaddr;		/* Mapped address */
> @@ -292,6 +294,8 @@ struct sdhci_host {
>  
>  	struct timer_list	timer;		/* Timer for timeouts */
>  
> +	unsigned int		caps;		/* Alternative capabilities */
> +
>  	unsigned long		private[0] ____cacheline_aligned;
>  };

--phil

Re: [PATCH 1/2] mmc: make sdhci work with ricoh mmc controller

[Philip Langdale]

On Mon,  7 Jun 2010 00:28:50 +0300, Maxim Levitsky
<maximlevitsky@gmail.com> wrote:
> The current way of disabling it is not well tested by vendor
> and has all kinds of bugs that show up on resume from ram/disk.
> 
> Old way of disabling is still supported by
> continuing to use CONFIG_MMC_RICOH_MMC.
> 
> Based on
> 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
> Most of the credit for this goes to Andrew de Quincey
> 
> 
> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> CC: Andrew de Quincey <adq_dvb@lidskialf.net>
> CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>

ACK with one change to add a #define for the device ID.

> ---
>  drivers/mmc/host/sdhci-pci.c |   31 +++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c     |    3 ++-
>  drivers/mmc/host/sdhci.h     |    4 ++++
>  3 files changed, 37 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index 65483fd..c4bcaeb 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -17,6 +17,7 @@
>  #include <linux/pci.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/slab.h>
> +#include <linux/device.h>
>  
>  #include <linux/mmc/host.h>
>  
> @@ -84,7 +85,21 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
>  	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
>  	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
>  		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
> +	return 0;
> +}
> +
> +static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
> +{
> +	slot->host->caps =
> +		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
> +			& SDHCI_TIMEOUT_CLK_MASK) |
>  
> +		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
> +			& SDHCI_CLOCK_BASE_MASK) |
> +
> +		SDHCI_TIMEOUT_CLK_UNIT |
> +		SDHCI_CAN_VDD_330 |
> +		SDHCI_CAN_DO_SDMA;
>  	return 0;
>  }

As we discussed, highspeed works. Of course, sdhci never sets the MMC
highspeed flag so the cap is irrelevant. We'd need another quirk to
indicate highspeed MMC is supported.

This can be done in a separate patch.

> @@ -95,6 +110,14 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
>  			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
>  };
>  
> +static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
> +	.probe_slot	= ricoh_mmc_probe_slot,
> +	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
> +			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
> +			  SDHCI_QUIRK_NO_CARD_NO_RESET |
> +			  SDHCI_QUIRK_MISSING_CAPS
> +};
> +
>  static const struct sdhci_pci_fixes sdhci_ene_712 = {
>  	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
>  			  SDHCI_QUIRK_BROKEN_DMA,
> @@ -374,6 +397,14 @@ static const struct pci_device_id pci_ids[]
> __devinitdata = {
>  	},
>  
>  	{
> +		.vendor         = PCI_VENDOR_ID_RICOH,
> +		.device         = 0x843,
> +		.subvendor      = PCI_ANY_ID,
> +		.subdevice      = PCI_ANY_ID,
> +		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
> +	},
> +
> +	{

It seems we generally want to add a #define for the device ID.
The ENE device below has one.

>  		.vendor		= PCI_VENDOR_ID_ENE,
>  		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
>  		.subvendor	= PCI_ANY_ID,
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c6d1bd8..483b78e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
>  			host->version);
>  	}
>  
> -	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> +	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? host->caps :
> +		sdhci_readl(host, SDHCI_CAPABILITIES);
>  
>  	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
>  		host->flags |= SDHCI_USE_SDMA;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index c846813..b1839a3 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -240,6 +240,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
>  /* Controller cannot support End Attribute in NOP ADMA descriptor */
>  #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
> +/* Controller is missing device caps. Use caps provided by host */
> +#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
>  
>  	int			irq;		/* Device IRQ */
>  	void __iomem *		ioaddr;		/* Mapped address */
> @@ -292,6 +294,8 @@ struct sdhci_host {
>  
>  	struct timer_list	timer;		/* Timer for timeouts */
>  
> +	unsigned int		caps;		/* Alternative capabilities */
> +
>  	unsigned long		private[0] ____cacheline_aligned;
>  };

--phil

Re: [PATCH 2/2] mmc: fix mmc card disappearence on resume on ricoh mmc controllers

[Philip Langdale]

On Mon,  7 Jun 2010 00:28:51 +0300, Maxim Levitsky
<maximlevitsky@gmail.com> wrote:
> The reason was that it takes time for controller to detect the card.
> So wait a bit for the card to appear.
> In worst case scenario, when you suspend the system with mmc card,
> and actually remove it, the resume will be delayed maximum by 2 seconds

I suspect it's probably sending an SD id command to the card and then that
has to time out on MMC cards.

Acked-by: Philip Langdale <philipl@overt.org>

> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |    3 ++-
>  drivers/mmc/host/sdhci.c     |   18 ++++++++++++++++++
>  drivers/mmc/host/sdhci.h     |    2 ++
>  3 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index c4bcaeb..49d863c 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -115,7 +115,8 @@ static const struct sdhci_pci_fixes sdhci_ricoh_mmc
= {
>  	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
>  			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
>  			  SDHCI_QUIRK_NO_CARD_NO_RESET |
> -			  SDHCI_QUIRK_MISSING_CAPS
> +			  SDHCI_QUIRK_MISSING_CAPS |
> +			  SDHCI_QUIRK_WAIT_CARD_ON_RESUME
>  };
>  
>  static const struct sdhci_pci_fixes sdhci_ene_712 = {
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 483b78e..6cda505 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1612,6 +1612,24 @@ int sdhci_resume_host(struct sdhci_host *host)
>  {
>  	int ret;
>  
> +	/* Some controllers (especialy the ricoh mmc controller) delay
> +		card detection on resume (probably since the controller
> +		has to poke the card to determine if its MMC or not */
> +
> +	if (host->mmc->bus_ops && (host->quirks &
> +			SDHCI_QUIRK_WAIT_CARD_ON_RESUME)) {
> +
> +		unsigned long timeout = jiffies + msecs_to_jiffies(2000);
> +
> +		while (!time_after(jiffies, timeout))
> +			if (sdhci_readl(host, SDHCI_PRESENT_STATE)
> +							& SDHCI_CARD_PRESENT) {
> +				break;
> +			}
> +			cpu_relax();
> +	}
> +
> +
>  	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
>  		if (host->ops->enable_dma)
>  			host->ops->enable_dma(host);
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index b1839a3..c9d099c 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -242,6 +242,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
>  /* Controller is missing device caps. Use caps provided by host */
>  #define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
> +/* Card doesn't appear immediatly on resume from low power state */
> +#define SDHCI_QUIRK_WAIT_CARD_ON_RESUME			(1<<28)
>  
>  	int			irq;		/* Device IRQ */
>  	void __iomem *		ioaddr;		/* Mapped address */

--phil

Re: [PATCH 2/2] mmc: fix mmc card disappearence on resume on ricoh mmc controllers

[Philip Langdale]

On Mon,  7 Jun 2010 00:28:51 +0300, Maxim Levitsky
<maximlevitsky@gmail.com> wrote:
> The reason was that it takes time for controller to detect the card.
> So wait a bit for the card to appear.
> In worst case scenario, when you suspend the system with mmc card,
> and actually remove it, the resume will be delayed maximum by 2 seconds

I suspect it's probably sending an SD id command to the card and then that
has to time out on MMC cards.

Acked-by: Philip Langdale <philipl@overt.org>

> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |    3 ++-
>  drivers/mmc/host/sdhci.c     |   18 ++++++++++++++++++
>  drivers/mmc/host/sdhci.h     |    2 ++
>  3 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index c4bcaeb..49d863c 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -115,7 +115,8 @@ static const struct sdhci_pci_fixes sdhci_ricoh_mmc
= {
>  	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
>  			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
>  			  SDHCI_QUIRK_NO_CARD_NO_RESET |
> -			  SDHCI_QUIRK_MISSING_CAPS
> +			  SDHCI_QUIRK_MISSING_CAPS |
> +			  SDHCI_QUIRK_WAIT_CARD_ON_RESUME
>  };
>  
>  static const struct sdhci_pci_fixes sdhci_ene_712 = {
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 483b78e..6cda505 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1612,6 +1612,24 @@ int sdhci_resume_host(struct sdhci_host *host)
>  {
>  	int ret;
>  
> +	/* Some controllers (especialy the ricoh mmc controller) delay
> +		card detection on resume (probably since the controller
> +		has to poke the card to determine if its MMC or not */
> +
> +	if (host->mmc->bus_ops && (host->quirks &
> +			SDHCI_QUIRK_WAIT_CARD_ON_RESUME)) {
> +
> +		unsigned long timeout = jiffies + msecs_to_jiffies(2000);
> +
> +		while (!time_after(jiffies, timeout))
> +			if (sdhci_readl(host, SDHCI_PRESENT_STATE)
> +							& SDHCI_CARD_PRESENT) {
> +				break;
> +			}
> +			cpu_relax();
> +	}
> +
> +
>  	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
>  		if (host->ops->enable_dma)
>  			host->ops->enable_dma(host);
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index b1839a3..c9d099c 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -242,6 +242,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
>  /* Controller is missing device caps. Use caps provided by host */
>  #define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
> +/* Card doesn't appear immediatly on resume from low power state */
> +#define SDHCI_QUIRK_WAIT_CARD_ON_RESUME			(1<<28)
>  
>  	int			irq;		/* Device IRQ */
>  	void __iomem *		ioaddr;		/* Mapped address */

--phil

Re: [PATCH 2/2] mmc: fix mmc card disappearence on resume on ricoh mmc controllers

[Chris Ball]

Hi,

On Mon, Jun 07, 2010 at 12:28:51AM +0300, Maxim Levitsky wrote:
> +	/* Some controllers (especialy the ricoh mmc controller) delay
> +		card detection on resume (probably since the controller
> +		has to poke the card to determine if its MMC or not */
> +
> +	if (host->mmc->bus_ops && (host->quirks &
> +			SDHCI_QUIRK_WAIT_CARD_ON_RESUME)) {
> +
> +		unsigned long timeout = jiffies + msecs_to_jiffies(2000);
> +
> +		while (!time_after(jiffies, timeout))
> +			if (sdhci_readl(host, SDHCI_PRESENT_STATE)
> +							& SDHCI_CARD_PRESENT) {
> +				break;

It looks like your editor is set to four-space instead of eight-space
tab characters, else you wouldn't be using so many tabs here. 

-- 
Chris Ball   <cjb@laptop.org>
One Laptop Per Child

Re: [PATCH 2/2] mmc: fix mmc card disappearence on resume on ricoh mmc controllers

[Maxim Levitsky]

On Mon, 2010-06-07 at 00:23 +0100, Chris Ball wrote: 
> Hi,
> 
> On Mon, Jun 07, 2010 at 12:28:51AM +0300, Maxim Levitsky wrote:
> > +	/* Some controllers (especialy the ricoh mmc controller) delay
> > +		card detection on resume (probably since the controller
> > +		has to poke the card to determine if its MMC or not */
> > +
> > +	if (host->mmc->bus_ops && (host->quirks &
> > +			SDHCI_QUIRK_WAIT_CARD_ON_RESUME)) {
> > +
> > +		unsigned long timeout = jiffies + msecs_to_jiffies(2000);
> > +
> > +		while (!time_after(jiffies, timeout))
> > +			if (sdhci_readl(host, SDHCI_PRESENT_STATE)
> > +							& SDHCI_CARD_PRESENT) {
> > +				break;
> 
> It looks like your editor is set to four-space instead of eight-space
> tab characters, else you wouldn't be using so many tabs here. 
Nope, I think indention is right here.

the break is inside 'if' condition.

Best regards
Maxim Levitsky

Re: [PATCH 1/2] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

On Sun, 2010-06-06 at 19:11 -0400, Philip Langdale wrote: 
> On Mon,  7 Jun 2010 00:28:50 +0300, Maxim Levitsky
> <maximlevitsky@gmail.com> wrote:
> > The current way of disabling it is not well tested by vendor
> > and has all kinds of bugs that show up on resume from ram/disk.
> > 
> > Old way of disabling is still supported by
> > continuing to use CONFIG_MMC_RICOH_MMC.
> > 
> > Based on
> > 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
> > Most of the credit for this goes to Andrew de Quincey
> > 
> > 
> > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > CC: Andrew de Quincey <adq_dvb@lidskialf.net>
> > CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>
> 
> ACK with one change to add a #define for the device ID.
I am not sure about this.
When I submitted a driver for xD part I was told that unless device is
shared by several pieces of code, its not ok to add it to pci_ids.c.
The device is I was told can be hardcoded in the driver.
Nothing against changing it. 
> 
> > ---
> >  drivers/mmc/host/sdhci-pci.c |   31 +++++++++++++++++++++++++++++++
> >  drivers/mmc/host/sdhci.c     |    3 ++-
> >  drivers/mmc/host/sdhci.h     |    4 ++++
> >  3 files changed, 37 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> > index 65483fd..c4bcaeb 100644
> > --- a/drivers/mmc/host/sdhci-pci.c
> > +++ b/drivers/mmc/host/sdhci-pci.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/slab.h>
> > +#include <linux/device.h>
> >  
> >  #include <linux/mmc/host.h>
> >  
> > @@ -84,7 +85,21 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
> >  	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
> >  	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
> >  		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
> > +	return 0;
> > +}
> > +
> > +static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
> > +{
> > +	slot->host->caps =
> > +		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
> > +			& SDHCI_TIMEOUT_CLK_MASK) |
> >  
> > +		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
> > +			& SDHCI_CLOCK_BASE_MASK) |
> > +
> > +		SDHCI_TIMEOUT_CLK_UNIT |
> > +		SDHCI_CAN_VDD_330 |
> > +		SDHCI_CAN_DO_SDMA;
> >  	return 0;
> >  }
> 
> As we discussed, highspeed works. Of course, sdhci never sets the MMC
> highspeed flag so the cap is irrelevant. We'd need another quirk to
> indicate highspeed MMC is supported.
> 
> This can be done in a separate patch.
Sure, but maybe we can enable this for SD/SDHC too?

> 
> > @@ -95,6 +110,14 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
> >  			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
> >  };
> >  
> > +static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
> > +	.probe_slot	= ricoh_mmc_probe_slot,
> > +	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
> > +			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
> > +			  SDHCI_QUIRK_NO_CARD_NO_RESET |
> > +			  SDHCI_QUIRK_MISSING_CAPS
> > +};
> > +
> >  static const struct sdhci_pci_fixes sdhci_ene_712 = {
> >  	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
> >  			  SDHCI_QUIRK_BROKEN_DMA,
> > @@ -374,6 +397,14 @@ static const struct pci_device_id pci_ids[]
> > __devinitdata = {
> >  	},
> >  
> >  	{
> > +		.vendor         = PCI_VENDOR_ID_RICOH,
> > +		.device         = 0x843,
> > +		.subvendor      = PCI_ANY_ID,
> > +		.subdevice      = PCI_ANY_ID,
> > +		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
> > +	},
> > +
> > +	{
> 
> It seems we generally want to add a #define for the device ID.
> The ENE device below has one.
> 
> >  		.vendor		= PCI_VENDOR_ID_ENE,
> >  		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
> >  		.subvendor	= PCI_ANY_ID,
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index c6d1bd8..483b78e 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
> >  			host->version);
> >  	}
> >  
> > -	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> > +	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? host->caps :
> > +		sdhci_readl(host, SDHCI_CAPABILITIES);
> >  
> >  	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
> >  		host->flags |= SDHCI_USE_SDMA;
> > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > index c846813..b1839a3 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -240,6 +240,8 @@ struct sdhci_host {
> >  #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
> >  /* Controller cannot support End Attribute in NOP ADMA descriptor */
> >  #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
> > +/* Controller is missing device caps. Use caps provided by host */
> > +#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
> >  
> >  	int			irq;		/* Device IRQ */
> >  	void __iomem *		ioaddr;		/* Mapped address */
> > @@ -292,6 +294,8 @@ struct sdhci_host {
> >  
> >  	struct timer_list	timer;		/* Timer for timeouts */
> >  
> > +	unsigned int		caps;		/* Alternative capabilities */
> > +
> >  	unsigned long		private[0] ____cacheline_aligned;
> >  };
> 
> --phil

Best regards,
Maxim Levitsky

Re: [PATCH 1/2] mmc: make sdhci work with ricoh mmc controller

[Philip Langdale]

On Mon, 07 Jun 2010 03:37:32 +0300
Maxim Levitsky <maximlevitsky@gmail.com> wrote:

> On Sun, 2010-06-06 at 19:11 -0400, Philip Langdale wrote: 
> > On Mon,  7 Jun 2010 00:28:50 +0300, Maxim Levitsky
> > <maximlevitsky@gmail.com> wrote:
> > > The current way of disabling it is not well tested by vendor
> > > and has all kinds of bugs that show up on resume from ram/disk.
> > > 
> > > Old way of disabling is still supported by
> > > continuing to use CONFIG_MMC_RICOH_MMC.
> > > 
> > > Based on
> > > 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
> > > Most of the credit for this goes to Andrew de Quincey
> > > 
> > > 
> > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > > CC: Andrew de Quincey <adq_dvb@lidskialf.net>
> > > CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>
> > 
> > ACK with one change to add a #define for the device ID.
> I am not sure about this.
> When I submitted a driver for xD part I was told that unless device is
> shared by several pieces of code, its not ok to add it to pci_ids.c.
> The device is I was told can be hardcoded in the driver.
> Nothing against changing it. 

Ok, then leave it as is:

Acked-by: Philip Langdale <philipl@overt.org>

> > > ---
> > >  drivers/mmc/host/sdhci-pci.c |   31
> > > +++++++++++++++++++++++++++++++ drivers/mmc/host/sdhci.c     |
> > > 3 ++- drivers/mmc/host/sdhci.h     |    4 ++++
> > >  3 files changed, 37 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/sdhci-pci.c
> > > b/drivers/mmc/host/sdhci-pci.c index 65483fd..c4bcaeb 100644
> > > --- a/drivers/mmc/host/sdhci-pci.c
> > > +++ b/drivers/mmc/host/sdhci-pci.c
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/pci.h>
> > >  #include <linux/dma-mapping.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/device.h>
> > >  
> > >  #include <linux/mmc/host.h>
> > >  
> > > @@ -84,7 +85,21 @@ static int ricoh_probe(struct sdhci_pci_chip
> > > *chip) if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG
> > > || chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
> > >  		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
> > > +	return 0;
> > > +}
> > > +
> > > +static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
> > > +{
> > > +	slot->host->caps =
> > > +		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
> > > +			& SDHCI_TIMEOUT_CLK_MASK) |
> > >  
> > > +		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
> > > +			& SDHCI_CLOCK_BASE_MASK) |
> > > +
> > > +		SDHCI_TIMEOUT_CLK_UNIT |
> > > +		SDHCI_CAN_VDD_330 |
> > > +		SDHCI_CAN_DO_SDMA;
> > >  	return 0;
> > >  }
> > 
> > As we discussed, highspeed works. Of course, sdhci never sets the
> > MMC highspeed flag so the cap is irrelevant. We'd need another
> > quirk to indicate highspeed MMC is supported.
> > 
> > This can be done in a separate patch.
> Sure, but maybe we can enable this for SD/SDHC too?

Not sure what you mean. The proper SD interface reports the HISPD
cap already. Do you mean enabling high-speed MMC on a 'proper'
SDHCI controller. That's what Pierre didn't want to do blindly.

> > 
> > > @@ -95,6 +110,14 @@ static const struct sdhci_pci_fixes
> > > sdhci_ricoh = { SDHCI_QUIRK_CLOCK_BEFORE_RESET,
> > >  };
> > >  
> > > +static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
> > > +	.probe_slot	= ricoh_mmc_probe_slot,
> > > +	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
> > > +			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
> > > +			  SDHCI_QUIRK_NO_CARD_NO_RESET |
> > > +			  SDHCI_QUIRK_MISSING_CAPS
> > > +};
> > > +
> > >  static const struct sdhci_pci_fixes sdhci_ene_712 = {
> > >  	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
> > >  			  SDHCI_QUIRK_BROKEN_DMA,
> > > @@ -374,6 +397,14 @@ static const struct pci_device_id pci_ids[]
> > > __devinitdata = {
> > >  	},
> > >  
> > >  	{
> > > +		.vendor         = PCI_VENDOR_ID_RICOH,
> > > +		.device         = 0x843,
> > > +		.subvendor      = PCI_ANY_ID,
> > > +		.subdevice      = PCI_ANY_ID,
> > > +		.driver_data    =
> > > (kernel_ulong_t)&sdhci_ricoh_mmc,
> > > +	},
> > > +
> > > +	{
> > 
> > It seems we generally want to add a #define for the device ID.
> > The ENE device below has one.
> > 
> > >  		.vendor		= PCI_VENDOR_ID_ENE,
> > >  		.device		=
> > > PCI_DEVICE_ID_ENE_CB712_SD, .subvendor	= PCI_ANY_ID,
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > > index c6d1bd8..483b78e 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
> > >  			host->version);
> > >  	}
> > >  
> > > -	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> > > +	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ?
> > > host->caps :
> > > +		sdhci_readl(host, SDHCI_CAPABILITIES);
> > >  
> > >  	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
> > >  		host->flags |= SDHCI_USE_SDMA;
> > > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > > index c846813..b1839a3 100644
> > > --- a/drivers/mmc/host/sdhci.h
> > > +++ b/drivers/mmc/host/sdhci.h
> > > @@ -240,6 +240,8 @@ struct sdhci_host {
> > >  #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
> > >  /* Controller cannot support End Attribute in NOP ADMA
> > > descriptor */ #define
> > > SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26) +/*
> > > Controller is missing device caps. Use caps provided by host */
> > > +#define SDHCI_QUIRK_MISSING_CAPS			(1<<27) 
> > >  	int			irq;		/* Device
> > > IRQ */ void __iomem *		ioaddr;		/*
> > > Mapped address */ @@ -292,6 +294,8 @@ struct sdhci_host {
> > >  
> > >  	struct timer_list	timer;		/* Timer
> > > for timeouts */ 
> > > +	unsigned int		caps;		/*
> > > Alternative capabilities */ +
> > >  	unsigned long		private[0]
> > > ____cacheline_aligned; };
> > 
> > --phil
> 
> Best regards,
> Maxim Levitsky
> 
> 

--phil

Re: [PATCH 2/2] mmc: fix mmc card disappearence on resume on ricoh mmc controllers

[Chris Ball]

Hi Maxim,

   >> It looks like your editor is set to four-space instead of
   >> eight-space tab characters, else you wouldn't be using so
   >> many tabs here.

   > Nope, I think indention is right here.
   >
   > the break is inside 'if' condition.

Please look again, I think you're mistaken.  For example, why do you
use seven tab characters for the "& SDHCI_CARD_PRESENT" after the if
line?  With eight-space tabs, it looks like this (converted to
spaces):

+                       if (sdhci_readl(host, SDHCI_PRESENT_STATE)
+                                                       & SDHCI_CARD_PRESENT) {

See http://lkml.org/lkml/2010/6/6/171 for an eight-space tabs
rendering of the patch.

-- 
Chris Ball   <cjb@laptop.org>
One Laptop Per Child

Re: [PATCH 2/2] mmc: fix mmc card disappearence on resume on ricoh mmc controllers

[Chris Ball]

Hi Maxim,

   >> It looks like your editor is set to four-space instead of
   >> eight-space tab characters, else you wouldn't be using so
   >> many tabs here.

   > Nope, I think indention is right here.
   >
   > the break is inside 'if' condition.

Please look again, I think you're mistaken.  For example, why do you
use seven tab characters for the "& SDHCI_CARD_PRESENT" after the if
line?  With eight-space tabs, it looks like this (converted to
spaces):

+                       if (sdhci_readl(host, SDHCI_PRESENT_STATE)
+                                                       & SDHCI_CARD_PRESENT) {

See http://lkml.org/lkml/2010/6/6/171 for an eight-space tabs
rendering of the patch.

-- 
Chris Ball   <cjb@laptop.org>
One Laptop Per Child

Re: [PATCH 2/2] mmc: fix mmc card disappearence on resume on ricoh mmc controllers

[Maxim Levitsky]

On Mon, 2010-06-07 at 01:47 -0400, Chris Ball wrote:
> Hi Maxim,
> 
>    >> It looks like your editor is set to four-space instead of
>    >> eight-space tab characters, else you wouldn't be using so
>    >> many tabs here.
> 
>    > Nope, I think indention is right here.
>    >
>    > the break is inside 'if' condition.
> 
> Please look again, I think you're mistaken.  For example, why do you
> use seven tab characters for the "& SDHCI_CARD_PRESENT" after the if
> line?  With eight-space tabs, it looks like this (converted to
> spaces):
> 
> +                       if (sdhci_readl(host, SDHCI_PRESENT_STATE)
> +                                                       & SDHCI_CARD_PRESENT) {
Ah, this.

I just break the line to avoid hitting the 80 char limit...

You probably meant I need to write:

+                       if (sdhci_readl(host, SDHCI_PRESENT_STATE)
+                                & SDHCI_CARD_PRESENT) {

Nothing against it,

Best regards,
	Maxim Levitsky

Re: [PATCH 2/2] mmc: fix mmc card disappearence on resume on ricoh mmc controllers

[Maxim Levitsky]

On Sun, 2010-06-06 at 19:22 -0400, Philip Langdale wrote:
> On Mon,  7 Jun 2010 00:28:51 +0300, Maxim Levitsky
> <maximlevitsky@gmail.com> wrote:
> > The reason was that it takes time for controller to detect the card.
> > So wait a bit for the card to appear.
> > In worst case scenario, when you suspend the system with mmc card,
> > and actually remove it, the resume will be delayed maximum by 2 seconds
> 
> I suspect it's probably sending an SD id command to the card and then that
> has to time out on MMC cards.
> 
> Acked-by: Philip Langdale <philipl@overt.org>

Thanks!

Note that I spoke too soon about it being perfect
While mmc card that stays in slot now works fine, a card that is
inserted while system is asleep, is detected, but then controller
becomes confused, so card doesn't work. A reinsert does help though.
Overall I need to work around some unusual delays imposed by controller
due to unusual card detection.
I'll sort that out eventually (and hope not to bring much ugliness to
sdhci{,-pci}.c

I think I update this patch to address both issues.

Best regards,
	Maxim Levitsky

[PATCH v3] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

Ok, now I tested it well, and sdhci + mmc controller finally work well.

I found out that small delay is all needed to make the mmc work fine
after resume with or without card.

Its not that pretty solution as it might increase the resume time (I use
a delay of 1/2sec, while around 300 ms is enough)
With parallel resume it probably be unnoticeable.

It might be not enough for other mmc cards (slower?) but then the
problem isn't that big deal, you just reinsert the card after resume or
don't suspend with it if first place.

I will at future do some reverse engineering.
Maybe I find a bit to that will tell that controller is currently busy
checking if card is mmc or not.

I did quite a lot of testing, and it works. 
commit bd9596e86d2321f3f97a90ec157371c366925357
Author: Maxim Levitsky <maximlevitsky@gmail.com>
Date:   Mon Jun 7 02:04:40 2010 +0300

---

    mmc: make sdhci work with ricoh mmc controller
    
    The current way of disabling it is not well tested by vendor
    and has all kinds of bugs that show up on resume from ram/disk.
    
    Old way of disabling is still supported by
    continuing to use CONFIG_MMC_RICOH_MMC.
    
    Based on 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
    Most of the credit for this goes to Andrew de Quincey
    
    
    Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
    CC: Andrew de Quincey <adq_dvb@lidskialf.net>
    CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 569e94d..85d15de 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1259,8 +1259,6 @@ int mmc_suspend_host(struct mmc_host *host)
 
 	if (host->caps & MMC_CAP_DISABLE)
 		cancel_delayed_work(&host->disable);
-	cancel_delayed_work(&host->detect);
-	mmc_flush_scheduled_work();
 
 	mmc_bus_get(host);
 	if (host->bus_ops && !host->bus_dead) {
diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 65483fd..e021431 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/device.h>
 
 #include <linux/mmc/host.h>
 
@@ -84,7 +85,30 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
 	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
 	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
 		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
+	return 0;
+}
+
+static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
+{
+	slot->host->caps =
+		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
+			& SDHCI_TIMEOUT_CLK_MASK) |
+
+		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
+			& SDHCI_CLOCK_BASE_MASK) |
 
+		SDHCI_TIMEOUT_CLK_UNIT |
+		SDHCI_CAN_VDD_330 |
+		SDHCI_CAN_DO_SDMA;
+	return 0;
+}
+
+static int ricoh_mmc_resume(struct sdhci_pci_chip *chip)
+{
+	/* Apply a delay to allow controller to settle */
+	/* Otherwise it becomes confused if card state changed
+		during suspend */
+	msleep(500);
 	return 0;
 }
 
@@ -95,6 +119,15 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
 			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
 };
 
+static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
+	.probe_slot	= ricoh_mmc_probe_slot,
+	.resume		= ricoh_mmc_resume,
+	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
+			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
+			  SDHCI_QUIRK_NO_CARD_NO_RESET |
+			  SDHCI_QUIRK_MISSING_CAPS
+};
+
 static const struct sdhci_pci_fixes sdhci_ene_712 = {
 	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
 			  SDHCI_QUIRK_BROKEN_DMA,
@@ -374,6 +407,14 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	},
 
 	{
+		.vendor         = PCI_VENDOR_ID_RICOH,
+		.device         = 0x843,
+		.subvendor      = PCI_ANY_ID,
+		.subdevice      = PCI_ANY_ID,
+		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
+	},
+
+	{
 		.vendor		= PCI_VENDOR_ID_ENE,
 		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
 		.subvendor	= PCI_ANY_ID,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6d1bd8..483b78e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->version);
 	}
 
-	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? host->caps :
+		sdhci_readl(host, SDHCI_CAPABILITIES);
 
 	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
 		host->flags |= SDHCI_USE_SDMA;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c846813..b1839a3 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -240,6 +240,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
 /* Controller cannot support End Attribute in NOP ADMA descriptor */
 #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
+/* Controller is missing device caps. Use caps provided by host */
+#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
@@ -292,6 +294,8 @@ struct sdhci_host {
 
 	struct timer_list	timer;		/* Timer for timeouts */
 
+	unsigned int		caps;		/* Alternative capabilities */
+
 	unsigned long		private[0] ____cacheline_aligned;
 };
 

[PATCH v4] mmc: make sdhci work with ricoh mmc controller

[maximlevitsky]

From: Maxim Levitsky <maximlevitsky@gmail.com>

The current way of disabling it is not well tested by vendor
and has all kinds of bugs that show up on resume from ram/disk.

Old way of disabling is still supported by
continuing to use CONFIG_MMC_RICOH_MMC.

Based on 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
Most of the credit for this goes to Andrew de Quincey


Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
CC: Andrew de Quincey <adq_dvb@lidskialf.net>
CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>
---
 drivers/mmc/host/sdhci-pci.c |   41 +++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c     |    3 ++-
 drivers/mmc/host/sdhci.h     |    4 ++++
 3 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 65483fd..e021431 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/device.h>
 
 #include <linux/mmc/host.h>
 
@@ -84,7 +85,30 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
 	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
 	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
 		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
+	return 0;
+}
+
+static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
+{
+	slot->host->caps =
+		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
+			& SDHCI_TIMEOUT_CLK_MASK) |
+
+		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
+			& SDHCI_CLOCK_BASE_MASK) |
 
+		SDHCI_TIMEOUT_CLK_UNIT |
+		SDHCI_CAN_VDD_330 |
+		SDHCI_CAN_DO_SDMA;
+	return 0;
+}
+
+static int ricoh_mmc_resume(struct sdhci_pci_chip *chip)
+{
+	/* Apply a delay to allow controller to settle */
+	/* Otherwise it becomes confused if card state changed
+		during suspend */
+	msleep(500);
 	return 0;
 }
 
@@ -95,6 +119,15 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
 			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
 };
 
+static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
+	.probe_slot	= ricoh_mmc_probe_slot,
+	.resume		= ricoh_mmc_resume,
+	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
+			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
+			  SDHCI_QUIRK_NO_CARD_NO_RESET |
+			  SDHCI_QUIRK_MISSING_CAPS
+};
+
 static const struct sdhci_pci_fixes sdhci_ene_712 = {
 	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
 			  SDHCI_QUIRK_BROKEN_DMA,
@@ -374,6 +407,14 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	},
 
 	{
+		.vendor         = PCI_VENDOR_ID_RICOH,
+		.device         = 0x843,
+		.subvendor      = PCI_ANY_ID,
+		.subdevice      = PCI_ANY_ID,
+		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
+	},
+
+	{
 		.vendor		= PCI_VENDOR_ID_ENE,
 		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
 		.subvendor	= PCI_ANY_ID,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6d1bd8..483b78e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->version);
 	}
 
-	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? host->caps :
+		sdhci_readl(host, SDHCI_CAPABILITIES);
 
 	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
 		host->flags |= SDHCI_USE_SDMA;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c846813..b1839a3 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -240,6 +240,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
 /* Controller cannot support End Attribute in NOP ADMA descriptor */
 #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
+/* Controller is missing device caps. Use caps provided by host */
+#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
@@ -292,6 +294,8 @@ struct sdhci_host {
 
 	struct timer_list	timer;		/* Timer for timeouts */
 
+	unsigned int		caps;		/* Alternative capabilities */
+
 	unsigned long		private[0] ____cacheline_aligned;
 };
 
-- 
1.7.0.4

Re: [PATCH v4] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

On Fri, 2010-06-11 at 22:15 +0300, maximlevitsky@gmail.com wrote: 
> From: Maxim Levitsky <maximlevitsky@gmail.com>
> 
> The current way of disabling it is not well tested by vendor
> and has all kinds of bugs that show up on resume from ram/disk.
> 
> Old way of disabling is still supported by
> continuing to use CONFIG_MMC_RICOH_MMC.
> 
> Based on 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
> Most of the credit for this goes to Andrew de Quincey
> 
> 
> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> CC: Andrew de Quincey <adq_dvb@lidskialf.net>
> CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>

Also, I did a lot of testing and no problems (even minor) were observed.
I think this is ready for merge.


Best regards,
Maxim Levitsky

Re: [PATCH v4] mmc: make sdhci work with ricoh mmc controller

[Philip Langdale]

On Fri, 11 Jun 2010 22:15:02 +0300
maximlevitsky@gmail.com wrote:

> From: Maxim Levitsky <maximlevitsky@gmail.com>
> 
> The current way of disabling it is not well tested by vendor
> and has all kinds of bugs that show up on resume from ram/disk.
> 
> Old way of disabling is still supported by
> continuing to use CONFIG_MMC_RICOH_MMC.
> 
> Based on
> 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
> Most of the credit for this goes to Andrew de Quincey
> 
> 
> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> CC: Andrew de Quincey <adq_dvb@lidskialf.net>
> CC: linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>

Acked-by: Philip Langdale <philipl@overt.org>

> ---
>  drivers/mmc/host/sdhci-pci.c |   41
> +++++++++++++++++++++++++++++++++++++++++
> drivers/mmc/host/sdhci.c     |    3 ++- drivers/mmc/host/sdhci.h
> |    4 ++++ 3 files changed, 47 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c
> b/drivers/mmc/host/sdhci-pci.c index 65483fd..e021431 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -17,6 +17,7 @@
>  #include <linux/pci.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/slab.h>
> +#include <linux/device.h>
>  
>  #include <linux/mmc/host.h>
>  
> @@ -84,7 +85,30 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
>  	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
>  	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
>  		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
> +	return 0;
> +}
> +
> +static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
> +{
> +	slot->host->caps =
> +		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
> +			& SDHCI_TIMEOUT_CLK_MASK) |
> +
> +		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
> +			& SDHCI_CLOCK_BASE_MASK) |
>  
> +		SDHCI_TIMEOUT_CLK_UNIT |
> +		SDHCI_CAN_VDD_330 |
> +		SDHCI_CAN_DO_SDMA;
> +	return 0;
> +}
> +
> +static int ricoh_mmc_resume(struct sdhci_pci_chip *chip)
> +{
> +	/* Apply a delay to allow controller to settle */
> +	/* Otherwise it becomes confused if card state changed
> +		during suspend */
> +	msleep(500);
>  	return 0;
>  }
>  
> @@ -95,6 +119,15 @@ static const struct sdhci_pci_fixes sdhci_ricoh =
> { SDHCI_QUIRK_CLOCK_BEFORE_RESET,
>  };
>  
> +static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
> +	.probe_slot	= ricoh_mmc_probe_slot,
> +	.resume		= ricoh_mmc_resume,
> +	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
> +			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
> +			  SDHCI_QUIRK_NO_CARD_NO_RESET |
> +			  SDHCI_QUIRK_MISSING_CAPS
> +};
> +
>  static const struct sdhci_pci_fixes sdhci_ene_712 = {
>  	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
>  			  SDHCI_QUIRK_BROKEN_DMA,
> @@ -374,6 +407,14 @@ static const struct pci_device_id pci_ids[]
> __devinitdata = { },
>  
>  	{
> +		.vendor         = PCI_VENDOR_ID_RICOH,
> +		.device         = 0x843,
> +		.subvendor      = PCI_ANY_ID,
> +		.subdevice      = PCI_ANY_ID,
> +		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
> +	},
> +
> +	{
>  		.vendor		= PCI_VENDOR_ID_ENE,
>  		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
>  		.subvendor	= PCI_ANY_ID,
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c6d1bd8..483b78e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
>  			host->version);
>  	}
>  
> -	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> +	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ?
> host->caps :
> +		sdhci_readl(host, SDHCI_CAPABILITIES);
>  
>  	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
>  		host->flags |= SDHCI_USE_SDMA;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index c846813..b1839a3 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -240,6 +240,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
>  /* Controller cannot support End Attribute in NOP ADMA descriptor */
>  #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
> +/* Controller is missing device caps. Use caps provided by host */
> +#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
>  
>  	int			irq;		/* Device IRQ
> */ void __iomem *		ioaddr;		/* Mapped
> address */ @@ -292,6 +294,8 @@ struct sdhci_host {
>  
>  	struct timer_list	timer;		/* Timer for
> timeouts */ 
> +	unsigned int		caps;		/*
> Alternative capabilities */ +
>  	unsigned long		private[0]
> ____cacheline_aligned; };
>  

--phil

[PATCH 1/2] MMC: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

The current way of disabling it is not well tested by vendor
and has all kinds of bugs that show up on resume from ram/disk.
A very good example is a dead SDHCI controller.

Old way of disabling is still supported by
continuing to use CONFIG_MMC_RICOH_MMC.

Based on 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
Therefore most of the credit for this goes to Andrew de Quincey

CC: Andrew de Quincey <adq_dvb@lidskialf.net>

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
Acked-by: Philip Langdale <philipl@overt.org>
---
 drivers/mmc/host/sdhci-pci.c |   41 +++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c     |    3 ++-
 drivers/mmc/host/sdhci.h     |    4 ++++
 3 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 65483fd..e021431 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/device.h>
 
 #include <linux/mmc/host.h>
 
@@ -84,7 +85,30 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
 	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
 	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
 		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
+	return 0;
+}
+
+static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
+{
+	slot->host->caps =
+		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
+			& SDHCI_TIMEOUT_CLK_MASK) |
+
+		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
+			& SDHCI_CLOCK_BASE_MASK) |
 
+		SDHCI_TIMEOUT_CLK_UNIT |
+		SDHCI_CAN_VDD_330 |
+		SDHCI_CAN_DO_SDMA;
+	return 0;
+}
+
+static int ricoh_mmc_resume(struct sdhci_pci_chip *chip)
+{
+	/* Apply a delay to allow controller to settle */
+	/* Otherwise it becomes confused if card state changed
+		during suspend */
+	msleep(500);
 	return 0;
 }
 
@@ -95,6 +119,15 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
 			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
 };
 
+static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
+	.probe_slot	= ricoh_mmc_probe_slot,
+	.resume		= ricoh_mmc_resume,
+	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
+			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
+			  SDHCI_QUIRK_NO_CARD_NO_RESET |
+			  SDHCI_QUIRK_MISSING_CAPS
+};
+
 static const struct sdhci_pci_fixes sdhci_ene_712 = {
 	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
 			  SDHCI_QUIRK_BROKEN_DMA,
@@ -374,6 +407,14 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	},
 
 	{
+		.vendor         = PCI_VENDOR_ID_RICOH,
+		.device         = 0x843,
+		.subvendor      = PCI_ANY_ID,
+		.subdevice      = PCI_ANY_ID,
+		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
+	},
+
+	{
 		.vendor		= PCI_VENDOR_ID_ENE,
 		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
 		.subvendor	= PCI_ANY_ID,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6d1bd8..483b78e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->version);
 	}
 
-	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? host->caps :
+		sdhci_readl(host, SDHCI_CAPABILITIES);
 
 	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
 		host->flags |= SDHCI_USE_SDMA;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c846813..b1839a3 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -240,6 +240,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
 /* Controller cannot support End Attribute in NOP ADMA descriptor */
 #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
+/* Controller is missing device caps. Use caps provided by host */
+#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
@@ -292,6 +294,8 @@ struct sdhci_host {
 
 	struct timer_list	timer;		/* Timer for timeouts */
 
+	unsigned int		caps;		/* Alternative capabilities */
+
 	unsigned long		private[0] ____cacheline_aligned;
 };
 
-- 
1.7.0.4

[PATCH 1/2] MMC: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

The current way of disabling it is not well tested by vendor
and has all kinds of bugs that show up on resume from ram/disk.
A very good example is a dead SDHCI controller.

Old way of disabling is still supported by
continuing to use CONFIG_MMC_RICOH_MMC.

Based on 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
Therefore most of the credit for this goes to Andrew de Quincey

CC: Andrew de Quincey <adq_dvb@lidskialf.net>

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
Acked-by: Philip Langdale <philipl@overt.org>
---
 drivers/mmc/host/sdhci-pci.c |   41 +++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c     |    3 ++-
 drivers/mmc/host/sdhci.h     |    4 ++++
 3 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 65483fd..e021431 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/device.h>
 
 #include <linux/mmc/host.h>
 
@@ -84,7 +85,30 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
 	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
 	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
 		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
+	return 0;
+}
+
+static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
+{
+	slot->host->caps =
+		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
+			& SDHCI_TIMEOUT_CLK_MASK) |
+
+		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
+			& SDHCI_CLOCK_BASE_MASK) |
 
+		SDHCI_TIMEOUT_CLK_UNIT |
+		SDHCI_CAN_VDD_330 |
+		SDHCI_CAN_DO_SDMA;
+	return 0;
+}
+
+static int ricoh_mmc_resume(struct sdhci_pci_chip *chip)
+{
+	/* Apply a delay to allow controller to settle */
+	/* Otherwise it becomes confused if card state changed
+		during suspend */
+	msleep(500);
 	return 0;
 }
 
@@ -95,6 +119,15 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
 			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
 };
 
+static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
+	.probe_slot	= ricoh_mmc_probe_slot,
+	.resume		= ricoh_mmc_resume,
+	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
+			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
+			  SDHCI_QUIRK_NO_CARD_NO_RESET |
+			  SDHCI_QUIRK_MISSING_CAPS
+};
+
 static const struct sdhci_pci_fixes sdhci_ene_712 = {
 	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
 			  SDHCI_QUIRK_BROKEN_DMA,
@@ -374,6 +407,14 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	},
 
 	{
+		.vendor         = PCI_VENDOR_ID_RICOH,
+		.device         = 0x843,
+		.subvendor      = PCI_ANY_ID,
+		.subdevice      = PCI_ANY_ID,
+		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
+	},
+
+	{
 		.vendor		= PCI_VENDOR_ID_ENE,
 		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
 		.subvendor	= PCI_ANY_ID,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6d1bd8..483b78e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->version);
 	}
 
-	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? host->caps :
+		sdhci_readl(host, SDHCI_CAPABILITIES);
 
 	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
 		host->flags |= SDHCI_USE_SDMA;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c846813..b1839a3 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -240,6 +240,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
 /* Controller cannot support End Attribute in NOP ADMA descriptor */
 #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
+/* Controller is missing device caps. Use caps provided by host */
+#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
@@ -292,6 +294,8 @@ struct sdhci_host {
 
 	struct timer_list	timer;		/* Timer for timeouts */
 
+	unsigned int		caps;		/* Alternative capabilities */
+
 	unsigned long		private[0] ____cacheline_aligned;
 };
 
-- 
1.7.0.4