Skip to contentSkip to conversation

ricoh_mmc: Handle newer models of Ricoh controllers

PATCH In review Philip Langdale 6 participants 10 messages Nov 25, 2008 – Nov 26, 2008

Subsystems (2)
PCI MMC, SD and SDIO
lore
Share & tools
lore

Philip Langdale

author
#0 lore raw

PATCH 1/1 ricoh_mmc: Handle newer models of Ricoh controllers

The latest generation of laptops are shipping with a newer model of Ricoh chip where the firewire controller is the primary PCI function but a cardbus controller is also present.

The existing code assumes that if a cardbus controller is, present, then it must be the one to manipulate - but the real rule is that you manipulate PCI function 0. This patch adds an additional constraint that the target must be function 0.

Signed-off-by Philip Langdale <[email protected]>

---

 ricoh_mmc.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/mmc/host/ricoh_mmc.c +11 -6
index a16d760..be9e7b3 100644
@@ -11,9 +11,10 @@
 
 /*
  * This is a conceptually ridiculous driver, but it is required by the way
- * the Ricoh multi-function R5C832 works. This chip implements firewire
- * and four different memory card controllers. Two of those controllers are
- * an SDHCI controller and a proprietary MMC controller. The linux SDHCI
+ * the Ricoh multi-function chips (R5CXXX) work. These chips implement
+ * the four main memory card controllers (SD, MMC, MS, xD) and one or both
+ * of cardbus or firewire. It happens that they implement SD and MMC
+ * support as separate controllers (and PCI functions). The linux SDHCI
  * driver supports MMC cards but the chip detects MMC cards in hardware
  * and directs them to the MMC controller - so the SDHCI driver never sees
  * them. To get around this, we must disable the useless MMC controller.

@@ -21,8 +22,10 @@
  * a detection event occurs immediately, even if the MMC card is already
  * in the reader.
  *
- * The relevant registers live on the firewire function, so this is unavoidably
- * ugly. Such is life.
+ * It seems to be the case that the relevant PCI registers to deactivate the
+ * MMC controller live on PCI function 0, which might be the cardbus controller
+ * or the firewire controller, depending on the particular chip in question. As
+ * such, it makes what this driver has to do unavoidably ugly. Such is life.
  */
 
 #include <linux/pci.h>

@@ -143,6 +146,7 @@ static int __devinit ricoh_mmc_probe(struct pci_dev *pdev,
 		pci_get_device(PCI_VENDOR_ID_RICOH,
 			PCI_DEVICE_ID_RICOH_RL5C476, fw_dev))) {
 		if (PCI_SLOT(pdev->devfn) == PCI_SLOT(fw_dev->devfn) &&
+		    PCI_FUNC(fw_dev->devfn) == 0 &&
 		    pdev->bus == fw_dev->bus) {
 			if (ricoh_mmc_disable(fw_dev) != 0)
 				return -ENODEV;

@@ -160,6 +164,7 @@ static int __devinit ricoh_mmc_probe(struct pci_dev *pdev,
 	    (fw_dev = pci_get_device(PCI_VENDOR_ID_RICOH,
 					PCI_DEVICE_ID_RICOH_R5C832, fw_dev))) {
 		if (PCI_SLOT(pdev->devfn) == PCI_SLOT(fw_dev->devfn) &&
+		    PCI_FUNC(fw_dev->devfn) == 0 &&
 		    pdev->bus == fw_dev->bus) {
 			if (ricoh_mmc_disable(fw_dev) != 0)
 				return -ENODEV;

@@ -172,7 +177,7 @@ static int __devinit ricoh_mmc_probe(struct pci_dev *pdev,
 
 	if (!ctrlfound) {
 		printk(KERN_WARNING DRIVER_NAME
-		       ": Main firewire function not found. Cannot disable controller.\n");
+		       ": Main Ricoh function not found. Cannot disable controller.\n");
 		return -ENODEV;
 	}
#1 lore raw

On Tue, Nov 25, 2008 at 12:42:57AM -0800, Philip Langdale wrote:

Hello

Quoted 3 lines

The latest generation of laptops are shipping with a newer model of Ricoh chip where the firewire controller is the primary PCI function but a cardbus controller is also present.

I'm confirming that my latest generation laptop now works with your patch.

Best regards

Matt

#2 lore raw
In reply to Philip Langdale
Quoted 11 lines · On Tue, 25 Nov 2008 00:42:57 -0800 Philip Langdale <[email protected]> wrote:

The latest generation of laptops are shipping with a newer model of Ricoh chip where the firewire controller is the primary PCI function but a cardbus controller is also present.

The existing code assumes that if a cardbus controller is, present, then it must be the one to manipulate - but the real rule is that you manipulate PCI function 0. This patch adds an additional constraint that the target must be function 0.

Signed-off-by Philip Langdale <[email protected]>

---

Mathias, please try to test this ASAP so that I can push it to Linus for .28 (provided it works).

Frank, if possibly it would be useful if you could also test this on the hardware you wrote your patch for. Just so we don't break anything. :)

Rgds

Signature · 6 lines
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.
#3 lore raw
Quoted 3 lines · Pierre Ossman

Frank, if possibly it would be useful if you could also test this on the hardware you wrote your patch for. Just so we don't break anything. :)

Unfortunately i don't have that or comparable hardware anymore here. So i cannot test it currently, but i put it on my todo-list when i'm able to catch it again.

Sorry,
Frank

#4 lore raw
In reply to Philip Langdale
Quoted 3 lines

The latest generation of laptops are shipping with a newer model of Ricoh chip where the firewire controller is the primary PCI function but a cardbus controller is also present.

Note that the current separate ricoh_mmc disabling module approach has been shown to break during suspend/resume. Matthew Garret proposed a patch for that which (with minor fixups) I tested successfully.

A fixed up version of Matthew's patch (not signed off) rebased against current mainline git is attached below. It does not include the changes proposed here.

Original bug report: http://lkml.org/lkml/2008/9/28/90
Matthew's analysis and patch: http://lkml.org/lkml/2008/9/29/87
Test results with patch: http://lkml.org/lkml/2008/9/29/195

Cheers,
FJP

From: Matthew Garrett <[email protected]>

mmc: Use a quirk to disable the proprietary Ricoh MMC controller

drivers/mmc/host/Kconfig +0 -17
index dfa585f..31c5f63 100644
@@ -48,23 +48,6 @@ config MMC_SDHCI_PCI
 
 	  If unsure, say N.
 
-config MMC_RICOH_MMC
-	tristate "Ricoh MMC Controller Disabler  (EXPERIMENTAL)"
-	depends on MMC_SDHCI_PCI
-	help
-	  This selects the disabler for the Ricoh MMC Controller. This
-	  proprietary controller is unnecessary because the SDHCI driver
-	  supports MMC cards on the SD controller, but if it is not
-	  disabled, it will steal the MMC cards away - rendering them
-	  useless. It is safe to select this driver even if you don't
-	  have a Ricoh based card reader.
-
-
-	  To compile this driver as a module, choose M here:
-	  the module will be called ricoh_mmc.
-
-	  If unsure, say Y.
-
 config MMC_OMAP
 	tristate "TI OMAP Multimedia Card Interface support"
 	depends on ARCH_OMAP
drivers/mmc/host/Makefile +0 -1
index c794cc5..0676890 100644
@@ -11,7 +11,6 @@ obj-$(CONFIG_MMC_PXA)		+= pxamci.o
 obj-$(CONFIG_MMC_IMX)		+= imxmmc.o
 obj-$(CONFIG_MMC_SDHCI)		+= sdhci.o
 obj-$(CONFIG_MMC_SDHCI_PCI)	+= sdhci-pci.o
-obj-$(CONFIG_MMC_RICOH_MMC)	+= ricoh_mmc.o
 obj-$(CONFIG_MMC_WBSD)		+= wbsd.o
 obj-$(CONFIG_MMC_AU1X)		+= au1xmmc.o
 obj-$(CONFIG_MMC_OMAP)		+= omap.o
drivers/mmc/host/ricoh_mmc.c deleted +0 -257

257 lines not on this page · raw message

drivers/pci/quirks.c +68 -0

74 lines not on this page · raw message

author
#5 lore raw
Quoted 5 lines · Frans Pop
Quoted 3 lines

The latest generation of laptops are shipping with a newer model of Ricoh chip where the firewire controller is the primary PCI function but a cardbus controller is also present.

Note that the current separate ricoh_mmc disabling module approach has been shown to break during suspend/resume. Matthew Garret proposed a patch for that which (with minor fixups) I tested successfully.

Hmm. Well, I'm interested as to what Pierre thinks. He explicitly didn't want a quirk when we originally looked at the problem and that's why ricoh_mmc exists - but maybe this is a good enough reason to revisit that decision.

--phil

#6 lore raw
Quoted 5 lines · On Wed, 26 Nov 2008 06:42:20 -0800 Philip Langdale <[email protected]> wrote:
Quoted 4 lines · Frans Pop
Quoted 3 lines

The latest generation of laptops are shipping with a newer model of Ricoh chip where the firewire controller is the primary PCI function but a cardbus controller is also present.

Quoted 9 lines
Quoted 3 lines

Note that the current separate ricoh_mmc disabling module approach has been shown to break during suspend/resume. Matthew Garret proposed a patch for that which (with minor fixups) I tested successfully.

Hmm. Well, I'm interested as to what Pierre thinks. He explicitly didn't want a quirk when we originally looked at the problem and that's why ricoh_mmc exists - but maybe this is a good enough reason to revisit that decision.

Actually, what I didn't want was a _sdhci_ quirk. I'm fine with a PCI one. In fact, that's probably the best approach as that register fiddling hides some PCI devices, so it's best if it is done before enumeration.

(Side note: sdhci has now undergone the architectural change that would allow a quirk like this, but the PCI version is probably still best)

Rgds

Signature · 6 lines
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.
#7 lore raw
In reply to Philip Langdale
Quoted 5 lines · Philip Langdale
Quoted 4 lines · Frans Pop
Quoted 3 lines

The latest generation of laptops are shipping with a newer model of Ricoh chip where the firewire controller is the primary PCI function but a cardbus controller is also present.

Quoted 8 lines
Quoted 3 lines

Note that the current separate ricoh_mmc disabling module approach has been shown to break during suspend/resume. Matthew Garret proposed a patch for that which (with minor fixups) I tested successfully.

Hmm. Well, I'm interested as to what Pierre thinks. He explicitly didn't want a quirk when we originally looked at the problem and that's why ricoh_mmc exists - but maybe this is a good enough reason to revisit that decision.

Thinking about it, it could possibly be handled by changing to suspend_noirq and resume_noirq rather than the normal suspend and resume functions? That ought to get the ordering constraints right. The problem occurs when ricoh_mmc suspends before sdhci and resumes after it.

Signature · 1 line
Matthew Garrett | [email protected]
author
#8 lore raw
Quoted 2 lines · Matthew Garrett

Thinking about it, it could possibly be handled by changing to suspend_noirq and resume_noirq rather than the normal suspend and resume functions? That ought to get the ordering constraints right. The problem occurs when ricoh_mmc suspends before sdhci and resumes after it.

I'll investigate that and see what happens.

--phil

#9 lore raw
Quoted 9 lines · On Wed, 26 Nov 2008 06:53:52 -0800 Philip Langdale <[email protected]> wrote:
Quoted 5 lines · Matthew Garrett

Thinking about it, it could possibly be handled by changing to suspend_noirq and resume_noirq rather than the normal suspend and resume functions? That ought to get the ordering constraints right. The problem occurs when ricoh_mmc suspends before sdhci and resumes after it.

I'll investigate that and see what happens.

Or you integrate it into sdhci_pci. Look at the register fiddling for JMicron chips in there.

Rgds

Signature · 6 lines
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.