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(-)
diff --git a/drivers/mmc/host/ricoh_mmc.c b/drivers/mmc/host/ricoh_mmc.c
index a16d760..be9e7b3 100644
--- a/drivers/mmc/host/ricoh_mmc.c
+++ b/drivers/mmc/host/ricoh_mmc.c
@@ -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;
}
On Tue, Nov 25, 2008 at 12:42:57AM -0800, Philip Langdale wrote:
Hello
> 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
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
--
-- 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.
> 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
Frans Pop 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.
>
> 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
On Wed, 26 Nov 2008 06:42:20 -0800
Philip Langdale <[email protected]> wrote:
> Frans Pop 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.
> >
> > 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
--
-- 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.
On Wed, Nov 26, 2008 at 06:42:20AM -0800, Philip Langdale wrote:
> Frans Pop 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.
> >
> >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.
--
Matthew Garrett | [email protected]
Matthew Garrett wrote:
>
> 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
On Wed, 26 Nov 2008 06:53:52 -0800
Philip Langdale <[email protected]> wrote:
> Matthew Garrett wrote:
> >
> > 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
--
-- 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.
Pierre Ossman wrote:
> 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