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

[Maxim Levitsky <maximlevitsky@gmail.com>]

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