mmc: make sdhci work with ricoh mmc controller

Prev: [PATCH 04/11] vfs: Allow handle based open on symlinks
Next: [PATCH -V13 06/11] ext4: Copy fs UUID to superblock

From: Philip Langdale on 3 Jun 2010 12:20

---

On Thu, 3 Jun 2010 04:16:27 +0300
Maxim Levitsky <maximlevitsky(a)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(a)gmail.com>
> CC: adq_dvb(a)lidskialf.net
> CC: linux-mmc(a)vger.kernel.org <linux-mmc(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Maxim Levitsky on 3 Jun 2010 12:40

---

On Thu, 2010-06-03 at 09:11 -0700, Philip Langdale wrote:
> On Thu, 3 Jun 2010 04:16:27 +0300
> Maxim Levitsky <maximlevitsky(a)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(a)gmail.com>
> > CC: adq_dvb(a)lidskialf.net
> > CC: linux-mmc(a)vger.kernel.org <linux-mmc(a)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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Philip Langdale on 3 Jun 2010 12:40

---

On Thu, 03 Jun 2010 19:31:49 +0300
Maxim Levitsky <maximlevitsky(a)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(a)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(a)gmail.com>
> > > CC: adq_dvb(a)lidskialf.net
> > > CC: linux-mmc(a)vger.kernel.org <linux-mmc(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Philip Langdale on 3 Jun 2010 13:10

---

On Thu, 3 Jun 2010 09:39:14 -0700
Philip Langdale <philipl(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Maxim Levitsky on 3 Jun 2010 13:40

---

On Thu, 2010-06-03 at 10:05 -0700, Philip Langdale wrote:
> On Thu, 3 Jun 2010 09:39:14 -0700
> Philip Langdale <philipl(a)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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

 |  Next  |  Last
Pages: 1 2 3
Prev: [PATCH 04/11] vfs: Allow handle based open on symlinks
Next: [PATCH -V13 06/11] ext4: Copy fs UUID to superblock