On Tue, 12 Jan 2010 15:43:50 -0800
Andrew Morton <[email protected]> wrote:

> (cc linux-mmc)
>
> On Fri, 8 Jan 2010 07:40:01 -0800
> Philip Langdale <[email protected]> wrote:
>
> > On Fri, 08 Jan 2010 17:24:44 +0200
> > Maxim Levitsky <[email protected]> wrote:
> >
> > > On Thu, 2009-11-26 at 23:55 -0800, Philip Langdale wrote:
> > > > On Fri, 27 Nov 2009 01:53:04 +0200
> > > > Maxim Levitsky <[email protected]> wrote:
> > > >
> > > > > >From 2d3002e29c329d76ca4e26d9f427814151a9648c Mon Sep 17
> > > > > >00:00:00 2001
> > > > > From: Maxim Levitsky <[email protected]>
> > > > > Date: Fri, 27 Nov 2009 00:53:28 +0200
> > > > > Subject: [PATCH] port ricoh_mmc to be pci quirk
> > > > >
> > > > > This patch solves nasty problem original driver has.
> > > > > Original goal of the ricoh_mmc was to disable this device
> > > > > because then, mmc cards can be read using standard SDHCI
> > > > > controller, thus avoiding writing of yet another driver.
> > > > > However, the act of disablement, makes other pci functions
> > > > > that belong to this controller (xD and memstick) shift up one
> > > > > level, thus pci core has now wrong idea about these devices.
> > > > >
> > > > > To fix this issue, this patch moves the driver into pci quirk
> > > > > section, thus it is executes before the pci is enumerated, and
> > > > > therefore solving that issue, also same sequence of commands
> > > > > is performed on resume for same reasons.
> > > > >
> > > > > Also regardless of the above, this way is cleaner.
> > > > >
> > > > > You still need to set CONFIG_MMC_RICOH_MMC
> > > > > to enable this quirk
> > > >
> > > > I like it. Only comment is that I'd like the printks to identify
> > > > which controller is involved. If Andrew is happy with the CONFIG
> > > > mechanism, then you've got an ack from me.
> > > >
> > > > Thanks for doing this.
> > >
> > > Hi, do you plan to include this patch in the kernel?
> >
> > That's Andrew's call, but to the extent that my opinion matters, I
> > support it being included.
> >
>
> I have a note here that Pierre had issues with the patch. I'm
> uncertain whether those are now resolved?

The last comment from Pierre I have is from the 25th of November and
says:

> I have no objections to this patch.

:-)

--phil

2010-01-13 06:47:14

On Fri, Jan 29, 2010 at 03:37:35PM +0200, Maxim Levitsky wrote:
> Here, updated patch with your comments addressed.
> This is also tested with real MMC card.
>
> This adds some > 80 chars lines, but I hope that is ok.

Some cosmetic issues, other than that:

Acked-by: Wolfram Sang <[email protected]>

>
> ---
>
> >From 9b208770b2421eae50e46d826f08a4084c2bcdb9 Mon Sep 17 00:00:00 2001
> From: Maxim Levitsky <[email protected]>
> Date: Fri, 29 Jan 2010 15:30:46 +0200
> Subject: [PATCH] Port ricoh_mmc to pci quirk
>
> This patch solves nasty problem original driver had.
> Original goal of the ricoh_mmc, was to disable this device because
> then, mmc cards can be read using standard SDHCI controller,
> thus avoiding the need in yet another driver.
> However, the act of disablement, makes other pci functions that belong to
> this controller (xD and memstick) shift up one level, thus pci core has now wrong idea
> about these devices.
>
> To fix this issue, this patch moves the driver into pci quirk section, thus it
> is executed before the pci is enumerated, and therefore solve that issue,
> also the same is preformed on resume for same reasons.
>
> Also regardless of the above, this way is cleaner.
>
> You still need to set CONFIG_MMC_RICOH_MMC
> to enable this quirk
>
> Signed-off-by: Maxim Levitsky <[email protected]>
> ---
> drivers/mmc/host/Kconfig | 10 +-
> drivers/mmc/host/Makefile | 1 -
> drivers/mmc/host/ricoh_mmc.c | 262 ------------------------------------------
> drivers/pci/quirks.c | 82 +++++++++++++
> 4 files changed, 85 insertions(+), 270 deletions(-)
> delete mode 100644 drivers/mmc/host/ricoh_mmc.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index ce1d288..6bc3ecb 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -69,20 +69,16 @@ config MMC_SDHCI_PCI
> If unsure, say N.
>
> config MMC_RICOH_MMC
> - tristate "Ricoh MMC Controller Disabler (EXPERIMENTAL)"
> + bool "Ricoh MMC Controller Disabler (EXPERIMENTAL)"
> depends on MMC_SDHCI_PCI
> help
> - This selects the disabler for the Ricoh MMC Controller. This
> + This adds a pci quirk to disable 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
> + useless. It is safe to select this 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_SDHCI_OF
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 3d253dd..f480397 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -12,7 +12,6 @@ obj-$(CONFIG_MMC_IMX) += imxmmc.o
> obj-$(CONFIG_MMC_MXC) += mxcmmc.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_SDHCI_PLTFM) += sdhci-pltfm.o
> obj-$(CONFIG_MMC_SDHCI_S3C) += sdhci-s3c.o
> obj-$(CONFIG_MMC_WBSD) += wbsd.o
> diff --git a/drivers/mmc/host/ricoh_mmc.c b/drivers/mmc/host/ricoh_mmc.c
> deleted file mode 100644
> index f627905..0000000
> --- a/drivers/mmc/host/ricoh_mmc.c
> +++ /dev/null
> @@ -1,262 +0,0 @@
> -/*
> - * ricoh_mmc.c - Dummy driver to disable the Rioch MMC controller.
> - *
> - * Copyright (C) 2007 Philip Langdale, All Rights Reserved.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or (at
> - * your option) any later version.
> - */
> -
> -/*
> - * This is a conceptually ridiculous driver, but it is required by the way
> - * 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.
> - * At that point, the SDHCI controller will start seeing them. As a bonus,
> - * a detection event occurs immediately, even if the MMC card is already
> - * in the reader.
> - *
> - * 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>
> -
> -#define DRIVER_NAME "ricoh-mmc"
> -
> -static const struct pci_device_id pci_ids[] __devinitdata = {
> - {
> - .vendor = PCI_VENDOR_ID_RICOH,
> - .device = PCI_DEVICE_ID_RICOH_R5C843,
> - .subvendor = PCI_ANY_ID,
> - .subdevice = PCI_ANY_ID,
> - },
> - { /* end: all zeroes */ },
> -};
> -
> -MODULE_DEVICE_TABLE(pci, pci_ids);
> -
> -static int ricoh_mmc_disable(struct pci_dev *fw_dev)
> -{
> - u8 write_enable;
> - u8 write_target;
> - u8 disable;
> -
> - if (fw_dev->device == PCI_DEVICE_ID_RICOH_RL5C476) {
> - /* via RL5C476 */
> -
> - pci_read_config_byte(fw_dev, 0xB7, &disable);
> - if (disable & 0x02) {
> - printk(KERN_INFO DRIVER_NAME
> - ": Controller already disabled. " \
> - "Nothing to do.\n");
> - return -ENODEV;
> - }
> -
> - pci_read_config_byte(fw_dev, 0x8E, &write_enable);
> - pci_write_config_byte(fw_dev, 0x8E, 0xAA);
> - pci_read_config_byte(fw_dev, 0x8D, &write_target);
> - pci_write_config_byte(fw_dev, 0x8D, 0xB7);
> - pci_write_config_byte(fw_dev, 0xB7, disable | 0x02);
> - pci_write_config_byte(fw_dev, 0x8E, write_enable);
> - pci_write_config_byte(fw_dev, 0x8D, write_target);
> - } else {
> - /* via R5C832 */
> -
> - pci_read_config_byte(fw_dev, 0xCB, &disable);
> - if (disable & 0x02) {
> - printk(KERN_INFO DRIVER_NAME
> - ": Controller already disabled. " \
> - "Nothing to do.\n");
> - return -ENODEV;
> - }
> -
> - pci_read_config_byte(fw_dev, 0xCA, &write_enable);
> - pci_write_config_byte(fw_dev, 0xCA, 0x57);
> - pci_write_config_byte(fw_dev, 0xCB, disable | 0x02);
> - pci_write_config_byte(fw_dev, 0xCA, write_enable);
> - }
> -
> - printk(KERN_INFO DRIVER_NAME
> - ": Controller is now disabled.\n");
> -
> - return 0;
> -}
> -
> -static int ricoh_mmc_enable(struct pci_dev *fw_dev)
> -{
> - u8 write_enable;
> - u8 write_target;
> - u8 disable;
> -
> - if (fw_dev->device == PCI_DEVICE_ID_RICOH_RL5C476) {
> - /* via RL5C476 */
> -
> - pci_read_config_byte(fw_dev, 0x8E, &write_enable);
> - pci_write_config_byte(fw_dev, 0x8E, 0xAA);
> - pci_read_config_byte(fw_dev, 0x8D, &write_target);
> - pci_write_config_byte(fw_dev, 0x8D, 0xB7);
> - pci_read_config_byte(fw_dev, 0xB7, &disable);
> - pci_write_config_byte(fw_dev, 0xB7, disable & ~0x02);
> - pci_write_config_byte(fw_dev, 0x8E, write_enable);
> - pci_write_config_byte(fw_dev, 0x8D, write_target);
> - } else {
> - /* via R5C832 */
> -
> - pci_read_config_byte(fw_dev, 0xCA, &write_enable);
> - pci_read_config_byte(fw_dev, 0xCB, &disable);
> - pci_write_config_byte(fw_dev, 0xCA, 0x57);
> - pci_write_config_byte(fw_dev, 0xCB, disable & ~0x02);
> - pci_write_config_byte(fw_dev, 0xCA, write_enable);
> - }
> -
> - printk(KERN_INFO DRIVER_NAME
> - ": Controller is now re-enabled.\n");
> -
> - return 0;
> -}
> -
> -static int __devinit ricoh_mmc_probe(struct pci_dev *pdev,
> - const struct pci_device_id *ent)
> -{
> - u8 rev;
> - u8 ctrlfound = 0;
> -
> - struct pci_dev *fw_dev = NULL;
> -
> - BUG_ON(pdev == NULL);
> - BUG_ON(ent == NULL);
> -
> - pci_read_config_byte(pdev, PCI_CLASS_REVISION, &rev);
> -
> - printk(KERN_INFO DRIVER_NAME
> - ": Ricoh MMC controller found at %s [%04x:%04x] (rev %x)\n",
> - pci_name(pdev), (int)pdev->vendor, (int)pdev->device,
> - (int)rev);
> -
> - while ((fw_dev =
> - 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;
> -
> - pci_set_drvdata(pdev, fw_dev);
> -
> - ++ctrlfound;
> - break;
> - }
> - }
> -
> - fw_dev = NULL;
> -
> - while (!ctrlfound &&
> - (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;
> -
> - pci_set_drvdata(pdev, fw_dev);
> -
> - ++ctrlfound;
> - }
> - }
> -
> - if (!ctrlfound) {
> - printk(KERN_WARNING DRIVER_NAME
> - ": Main Ricoh function not found. Cannot disable controller.\n");
> - return -ENODEV;
> - }
> -
> - return 0;
> -}
> -
> -static void __devexit ricoh_mmc_remove(struct pci_dev *pdev)
> -{
> - struct pci_dev *fw_dev = NULL;
> -
> - fw_dev = pci_get_drvdata(pdev);
> - BUG_ON(fw_dev == NULL);
> -
> - ricoh_mmc_enable(fw_dev);
> -
> - pci_set_drvdata(pdev, NULL);
> -}
> -
> -static int ricoh_mmc_suspend_late(struct pci_dev *pdev, pm_message_t state)
> -{
> - struct pci_dev *fw_dev = NULL;
> -
> - fw_dev = pci_get_drvdata(pdev);
> - BUG_ON(fw_dev == NULL);
> -
> - printk(KERN_INFO DRIVER_NAME ": Suspending.\n");
> -
> - ricoh_mmc_enable(fw_dev);
> -
> - return 0;
> -}
> -
> -static int ricoh_mmc_resume_early(struct pci_dev *pdev)
> -{
> - struct pci_dev *fw_dev = NULL;
> -
> - fw_dev = pci_get_drvdata(pdev);
> - BUG_ON(fw_dev == NULL);
> -
> - printk(KERN_INFO DRIVER_NAME ": Resuming.\n");
> -
> - ricoh_mmc_disable(fw_dev);
> -
> - return 0;
> -}
> -
> -static struct pci_driver ricoh_mmc_driver = {
> - .name = DRIVER_NAME,
> - .id_table = pci_ids,
> - .probe = ricoh_mmc_probe,
> - .remove = __devexit_p(ricoh_mmc_remove),
> - .suspend_late = ricoh_mmc_suspend_late,
> - .resume_early = ricoh_mmc_resume_early,
> -};
> -
> -/*****************************************************************************\
> - * *
> - * Driver init/exit *
> - * *
> -\*****************************************************************************/
> -
> -static int __init ricoh_mmc_drv_init(void)
> -{
> - printk(KERN_INFO DRIVER_NAME
> - ": Ricoh MMC Controller disabling driver\n");
> - printk(KERN_INFO DRIVER_NAME ": Copyright(c) Philip Langdale\n");
> -
> - return pci_register_driver(&ricoh_mmc_driver);
> -}
> -
> -static void __exit ricoh_mmc_drv_exit(void)
> -{
> - pci_unregister_driver(&ricoh_mmc_driver);
> -}
> -
> -module_init(ricoh_mmc_drv_init);
> -module_exit(ricoh_mmc_drv_exit);
> -
> -MODULE_AUTHOR("Philip Langdale <[email protected]>");
> -MODULE_DESCRIPTION("Ricoh MMC Controller disabling driver");
> -MODULE_LICENSE("GPL");
> -
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index c746943..df08a2c 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2520,6 +2520,88 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x150d, quirk_i82576_sriov);
>
> #endif /* CONFIG_PCI_IOV */
>
> +/*
> + * This is a quirk for Ricoh MMC controller found as a part of

for the Ricoh...

> + * multifunction chip.

some multifunction chips.

> +
> + * This is very similar and based on ricoh_mmc driver written by

on the ricoh_mmc...

> + * Philip Langdale. Thank you for these magic sequences.
> + *
> + * 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.
> + * At that point, the SDHCI controller will start seeing them
> + * 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

These paragraphs would benefit a lot from reformatting ('gqip' in vim, other
editors surely have such a function, too ;)).

> +
> + * This has to be done early, because as soon as we disable the MMC controller
> + * other pci functions shift up one level, e.g. function #2 becomes function
> + * #1, and this will confuse the pci core.
> + */
> +
> +#ifdef CONFIG_MMC_RICOH_MMC
> +static void ricoh_mmc_fixup_rl5c476(struct pci_dev *dev)
> +{
> + /* disable via cardbus interface */
> + u8 write_enable;
> + u8 write_target;
> + u8 disable;
> +
> + /* disable must be done via function #0 */
> + if (PCI_FUNC(dev->devfn))
> + return;
> +
> + pci_read_config_byte(dev, 0xB7, &disable);
> + if (disable & 0x02)
> + return;
> +
> + pci_read_config_byte(dev, 0x8E, &write_enable);
> + pci_write_config_byte(dev, 0x8E, 0xAA);
> + pci_read_config_byte(dev, 0x8D, &write_target);
> + pci_write_config_byte(dev, 0x8D, 0xB7);
> + pci_write_config_byte(dev, 0xB7, disable | 0x02);
> + pci_write_config_byte(dev, 0x8E, write_enable);
> + pci_write_config_byte(dev, 0x8D, write_target);
> +
> + dev_notice(&dev->dev, "proprietary Ricoh mmc controller disabled (via cardbus function)\n");
> + dev_notice(&dev->dev, "MMC cards are now supported by standard SDHCI controller\n");

1st line is 'mmc', second is 'MMC'. I'd prefer always MMC.

> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_RICOH, PCI_DEVICE_ID_RICOH_RL5C476, ricoh_mmc_fixup_rl5c476);
> +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_RICOH, PCI_DEVICE_ID_RICOH_RL5C476, ricoh_mmc_fixup_rl5c476);
> +
> +static void ricoh_mmc_fixup_r5c832(struct pci_dev *dev)
> +{
> + /* disable via firewire interface */
> + u8 write_enable;
> + u8 disable;
> +
> + /* disable must be done via function #0 */
> + if (PCI_FUNC(dev->devfn))
> + return;
> +
> + pci_read_config_byte(dev, 0xCB, &disable);
> +
> + if (disable & 0x02)
> + return;
> +
> + pci_read_config_byte(dev, 0xCA, &write_enable);
> + pci_write_config_byte(dev, 0xCA, 0x57);
> + pci_write_config_byte(dev, 0xCB, disable | 0x02);
> + pci_write_config_byte(dev, 0xCA, write_enable);
> +
> + dev_notice(&dev->dev, "proprietary Ricoh mmc controller disabled (via firewire function)\n");
> + dev_notice(&dev->dev, "MMC cards are now supported by standard SDHCI controller\n");

ditto.

> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_RICOH, PCI_DEVICE_ID_RICOH_R5C832, ricoh_mmc_fixup_r5c832);
> +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_RICOH, PCI_DEVICE_ID_RICOH_R5C832, ricoh_mmc_fixup_r5c832);
> +#endif /*CONFIG_MMC_RICOH_MMC*/
> +
> +
> static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
> struct pci_fixup *end)
> {
> --
> 1.6.3.3
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |

Attachments:

(No filename) (15.58 kB)
signature.asc (197.00 B)
Digital signature Download all attachments

2010-01-29 22:10:40

by Maxim Levitsky

[permalink] [raw]

Subject: Re: [PATCH v3] port ricoh_mmc to pci quirk

On Fri, 2010-01-29 at 08:06 -0800, Philip Langdale wrote:
> On Fri, 29 Jan 2010 15:37:35 +0200
> Maxim Levitsky <[email protected]> wrote:
>
> > Here, updated patch with your comments addressed.
> > This is also tested with real MMC card.
>
> Actual code gets my thumbs up.
>
> > This adds some > 80 chars lines, but I hope that is ok.
>
> It's against the coding style and as far as I can see, the only
> lines that are > 80 are the dev_notice and DECLARE_FIXUP lines - both
> of which can be trivially split across multiple lines, so please just
> do that.
Well, I have nothing against this, but, even Linus himself stated that
he dislikes broken printk lines.

I also noticed that some DECLARE_FIXUP in this file are longer that 80,
and since breaking this doesn't add any value, I thought that is ok.
Remember that main purpose of 80 char limit is to make code simpler.

However, I don't really care if you insist.

Best regards,
Maxim Levitsky

2010-01-29 22:13:17

by Maxim Levitsky

[permalink] [raw]

Subject: Re: [PATCH v3] port ricoh_mmc to pci quirk

On Fri, 2010-01-29 at 17:33 +0100, Wolfram Sang wrote:
> On Fri, Jan 29, 2010 at 03:37:35PM +0200, Maxim Levitsky wrote:
> > Here, updated patch with your comments addressed.
> > This is also tested with real MMC card.
> >
> > This adds some > 80 chars lines, but I hope that is ok.
>
> Some cosmetic issues, other than that:

I guess I introduced more spelling & grammar errors in this version that
I fixed... Anyway will try to be more careful now. English isn't my best
subject.

Best regards,
Maxim Levitsky

2010-01-30 02:19:14

by Philip Langdale

[permalink] [raw]

Subject: Re: [PATCH v3] port ricoh_mmc to pci quirk

On Sat, 30 Jan 2010 00:10:33 +0200
Maxim Levitsky <[email protected]> wrote:

> Well, I have nothing against this, but, even Linus himself stated that
> he dislikes broken printk lines.
>
> I also noticed that some DECLARE_FIXUP in this file are longer that
> 80, and since breaking this doesn't add any value, I thought that is
> ok. Remember that main purpose of 80 char limit is to make code
> simpler.
>
> However, I don't really care if you insist.

I don't want to be obnoxious either - if no one else is concerned about
the lines, I won't kick up a fuss.

And thanks for sticking with this patch - I want to see it in soon too;
I'd like to sort out the new PCI-e Ricoh parts which are going to need
a new magic sequence.

--phil

2010-01-30 21:28:51

by Maxim Levitsky

[permalink] [raw]

Subject: [PATCH] Port ricoh_mmc to pci quirk

This patch solves nasty problem original driver had.
Original goal of the ricoh_mmc, was to disable this device because
then, mmc cards can be read using standard SDHCI controller,
thus avoiding the need in yet another driver.
However, the act of disablement, makes other pci functions that belong to
this controller (xD and memstick) shift up one level, thus pci core has now wrong idea
about these devices.

To fix this issue, this patch moves the driver into pci quirk section, thus it
is executed before the pci is enumerated, and therefore solve that issue,
also the same is preformed on resume for same reasons.

Also regardless of the above, this way is cleaner.

You still need to set CONFIG_MMC_RICOH_MMC
to enable this quirk

Signed-off-by: Maxim Levitsky <[email protected]>
---
drivers/mmc/host/Kconfig | 10 +-
drivers/mmc/host/Makefile | 1 -
drivers/mmc/host/ricoh_mmc.c | 262 ------------------------------------------
drivers/pci/quirks.c | 85 ++++++++++++++
4 files changed, 88 insertions(+), 270 deletions(-)
delete mode 100644 drivers/mmc/host/ricoh_mmc.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index ce1d288..6bc3ecb 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -69,20 +69,16 @@ config MMC_SDHCI_PCI
If unsure, say N.

config MMC_RICOH_MMC
- tristate "Ricoh MMC Controller Disabler (EXPERIMENTAL)"
+ bool "Ricoh MMC Controller Disabler (EXPERIMENTAL)"
depends on MMC_SDHCI_PCI
help
- This selects the disabler for the Ricoh MMC Controller. This
+ This adds a pci quirk to disable 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
+ useless. It is safe to select this 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_SDHCI_OF
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 3d253dd..f480397 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -12,7 +12,6 @@ obj-$(CONFIG_MMC_IMX) += imxmmc.o
obj-$(CONFIG_MMC_MXC) += mxcmmc.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_SDHCI_PLTFM) += sdhci-pltfm.o
obj-$(CONFIG_MMC_SDHCI_S3C) += sdhci-s3c.o
obj-$(CONFIG_MMC_WBSD) += wbsd.o
diff --git a/drivers/mmc/host/ricoh_mmc.c b/drivers/mmc/host/ricoh_mmc.c
deleted file mode 100644
index f627905..0000000
--- a/drivers/mmc/host/ricoh_mmc.c
+++ /dev/null
@@ -1,262 +0,0 @@
-/*
- * ricoh_mmc.c - Dummy driver to disable the Rioch MMC controller.
- *
- * Copyright (C) 2007 Philip Langdale, All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or (at
- * your option) any later version.
- */
-
-/*
- * This is a conceptually ridiculous driver, but it is required by the way
- * 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.
- * At that point, the SDHCI controller will start seeing them. As a bonus,
- * a detection event occurs immediately, even if the MMC card is already
- * in the reader.
- *
- * 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>
-
-#define DRIVER_NAME "ricoh-mmc"
-
-static const struct pci_device_id pci_ids[] __devinitdata = {
- {
- .vendor = PCI_VENDOR_ID_RICOH,
- .device = PCI_DEVICE_ID_RICOH_R5C843,
- .subvendor = PCI_ANY_ID,
- .subdevice = PCI_ANY_ID,
- },
- { /* end: all zeroes */ },
-};
-
-MODULE_DEVICE_TABLE(pci, pci_ids);
-
-static int ricoh_mmc_disable(struct pci_dev *fw_dev)
-{
- u8 write_enable;
- u8 write_target;
- u8 disable;
-
- if (fw_dev->device == PCI_DEVICE_ID_RICOH_RL5C476) {
- /* via RL5C476 */
-
- pci_read_config_byte(fw_dev, 0xB7, &disable);
- if (disable & 0x02) {
- printk(KERN_INFO DRIVER_NAME
- ": Controller already disabled. " \
- "Nothing to do.\n");
- return -ENODEV;
- }
-
- pci_read_config_byte(fw_dev, 0x8E, &write_enable);
- pci_write_config_byte(fw_dev, 0x8E, 0xAA);
- pci_read_config_byte(fw_dev, 0x8D, &write_target);
- pci_write_config_byte(fw_dev, 0x8D, 0xB7);
- pci_write_config_byte(fw_dev, 0xB7, disable | 0x02);
- pci_write_config_byte(fw_dev, 0x8E, write_enable);
- pci_write_config_byte(fw_dev, 0x8D, write_target);
- } else {
- /* via R5C832 */
-
- pci_read_config_byte(fw_dev, 0xCB, &disable);
- if (disable & 0x02) {
- printk(KERN_INFO DRIVER_NAME
- ": Controller already disabled. " \
- "Nothing to do.\n");
- return -ENODEV;
- }
-
- pci_read_config_byte(fw_dev, 0xCA, &write_enable);
- pci_write_config_byte(fw_dev, 0xCA, 0x57);
- pci_write_config_byte(fw_dev, 0xCB, disable | 0x02);
- pci_write_config_byte(fw_dev, 0xCA, write_enable);
- }
-
- printk(KERN_INFO DRIVER_NAME
- ": Controller is now disabled.\n");
-
- return 0;
-}
-
-static int ricoh_mmc_enable(struct pci_dev *fw_dev)
-{
- u8 write_enable;
- u8 write_target;
- u8 disable;
-
- if (fw_dev->device == PCI_DEVICE_ID_RICOH_RL5C476) {
- /* via RL5C476 */
-
- pci_read_config_byte(fw_dev, 0x8E, &write_enable);
- pci_write_config_byte(fw_dev, 0x8E, 0xAA);
- pci_read_config_byte(fw_dev, 0x8D, &write_target);
- pci_write_config_byte(fw_dev, 0x8D, 0xB7);
- pci_read_config_byte(fw_dev, 0xB7, &disable);
- pci_write_config_byte(fw_dev, 0xB7, disable & ~0x02);
- pci_write_config_byte(fw_dev, 0x8E, write_enable);
- pci_write_config_byte(fw_dev, 0x8D, write_target);
- } else {
- /* via R5C832 */
-
- pci_read_config_byte(fw_dev, 0xCA, &write_enable);
- pci_read_config_byte(fw_dev, 0xCB, &disable);
- pci_write_config_byte(fw_dev, 0xCA, 0x57);
- pci_write_config_byte(fw_dev, 0xCB, disable & ~0x02);
- pci_write_config_byte(fw_dev, 0xCA, write_enable);
- }
-
- printk(KERN_INFO DRIVER_NAME
- ": Controller is now re-enabled.\n");
-
- return 0;
-}
-
-static int __devinit ricoh_mmc_probe(struct pci_dev *pdev,
- const struct pci_device_id *ent)
-{
- u8 rev;
- u8 ctrlfound = 0;
-
- struct pci_dev *fw_dev = NULL;
-
- BUG_ON(pdev == NULL);
- BUG_ON(ent == NULL);
-
- pci_read_config_byte(pdev, PCI_CLASS_REVISION, &rev);
-
- printk(KERN_INFO DRIVER_NAME
- ": Ricoh MMC controller found at %s [%04x:%04x] (rev %x)\n",
- pci_name(pdev), (int)pdev->vendor, (int)pdev->device,
- (int)rev);
-
- while ((fw_dev =
- 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;
-
- pci_set_drvdata(pdev, fw_dev);
-
- ++ctrlfound;
- break;
- }
- }
-
- fw_dev = NULL;
-
- while (!ctrlfound &&
- (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;
-
- pci_set_drvdata(pdev, fw_dev);
-
- ++ctrlfound;
- }
- }
-
- if (!ctrlfound) {
- printk(KERN_WARNING DRIVER_NAME
- ": Main Ricoh function not found. Cannot disable controller.\n");
- return -ENODEV;
- }
-
- return 0;
-}
-
-static void __devexit ricoh_mmc_remove(struct pci_dev *pdev)
-{
- struct pci_dev *fw_dev = NULL;
-
- fw_dev = pci_get_drvdata(pdev);
- BUG_ON(fw_dev == NULL);
-
- ricoh_mmc_enable(fw_dev);
-
- pci_set_drvdata(pdev, NULL);
-}
-
-static int ricoh_mmc_suspend_late(struct pci_dev *pdev, pm_message_t state)
-{
- struct pci_dev *fw_dev = NULL;
-
- fw_dev = pci_get_drvdata(pdev);
- BUG_ON(fw_dev == NULL);
-
- printk(KERN_INFO DRIVER_NAME ": Suspending.\n");
-
- ricoh_mmc_enable(fw_dev);
-
- return 0;
-}
-
-static int ricoh_mmc_resume_early(struct pci_dev *pdev)
-{
- struct pci_dev *fw_dev = NULL;
-
- fw_dev = pci_get_drvdata(pdev);
- BUG_ON(fw_dev == NULL);
-
- printk(KERN_INFO DRIVER_NAME ": Resuming.\n");
-
- ricoh_mmc_disable(fw_dev);
-
- return 0;
-}
-
-static struct pci_driver ricoh_mmc_driver = {
- .name = DRIVER_NAME,
- .id_table = pci_ids,
- .probe = ricoh_mmc_probe,
- .remove = __devexit_p(ricoh_mmc_remove),
- .suspend_late = ricoh_mmc_suspend_late,
- .resume_early = ricoh_mmc_resume_early,
-};
-
-/*****************************************************************************\
- * *
- * Driver init/exit *
- * *
-\*****************************************************************************/
-
-static int __init ricoh_mmc_drv_init(void)
-{
- printk(KERN_INFO DRIVER_NAME
- ": Ricoh MMC Controller disabling driver\n");
- printk(KERN_INFO DRIVER_NAME ": Copyright(c) Philip Langdale\n");
-
- return pci_register_driver(&ricoh_mmc_driver);
-}
-
-static void __exit ricoh_mmc_drv_exit(void)
-{
- pci_unregister_driver(&ricoh_mmc_driver);
-}
-
-module_init(ricoh_mmc_drv_init);
-module_exit(ricoh_mmc_drv_exit);
-
-MODULE_AUTHOR("Philip Langdale <[email protected]>");
-MODULE_DESCRIPTION("Ricoh MMC Controller disabling driver");
-MODULE_LICENSE("GPL");
-
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index c746943..df921db 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2520,6 +2520,91 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x150d, quirk_i82576_sriov);

#endif /* CONFIG_PCI_IOV */

+/*
+ * This is a quirk for the Ricoh MMC controller found as a part of
+ * some mulifunction chips.
+
+ * This is very similiar and based on the ricoh_mmc driver written by
+ * Philip Langdale. Thank you for these magic sequences.
+ *
+ * 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.
+ * At that point, the SDHCI controller will start seeing them
+ * 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
+ *
+ * This has to be done early, because as soon as we disable the MMC controller
+ * other pci functions shift up one level, e.g. function #2 becomes function
+ * #1, and this will confuse the pci core.
+ */
+
+#ifdef CONFIG_MMC_RICOH_MMC
+static void ricoh_mmc_fixup_rl5c476(struct pci_dev *dev)
+{
+ /* disable via cardbus interface */
+ u8 write_enable;
+ u8 write_target;
+ u8 disable;
+
+ /* disable must be done via function #0 */
+ if (PCI_FUNC(dev->devfn))
+ return;
+
+ pci_read_config_byte(dev, 0xB7, &disable);
+ if (disable & 0x02)
+ return;
+
+ pci_read_config_byte(dev, 0x8E, &write_enable);
+ pci_write_config_byte(dev, 0x8E, 0xAA);
+ pci_read_config_byte(dev, 0x8D, &write_target);
+ pci_write_config_byte(dev, 0x8D, 0xB7);
+ pci_write_config_byte(dev, 0xB7, disable | 0x02);
+ pci_write_config_byte(dev, 0x8E, write_enable);
+ pci_write_config_byte(dev, 0x8D, write_target);
+
+ dev_notice(&dev->dev, "proprietary Ricoh MMC controller disabled (via cardbus function)\n");
+ dev_notice(&dev->dev, "MMC cards are now supported by standard SDHCI controller\n");
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_RICOH, PCI_DEVICE_ID_RICOH_RL5C476, ricoh_mmc_fixup_rl5c476);
+DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_RICOH, PCI_DEVICE_ID_RICOH_RL5C476, ricoh_mmc_fixup_rl5c476);
+
+static void ricoh_mmc_fixup_r5c832(struct pci_dev *dev)
+{
+ /* disable via firewire interface */
+ u8 write_enable;
+ u8 disable;
+
+ /* disable must be done via function #0 */
+ if (PCI_FUNC(dev->devfn))
+ return;
+
+ pci_read_config_byte(dev, 0xCB, &disable);
+
+ if (disable & 0x02)
+ return;
+
+ pci_read_config_byte(dev, 0xCA, &write_enable);
+ pci_write_config_byte(dev, 0xCA, 0x57);
+ pci_write_config_byte(dev, 0xCB, disable | 0x02);
+ pci_write_config_byte(dev, 0xCA, write_enable);
+
+ dev_notice(&dev->dev, "proprietary Ricoh MMC controller disabled (via firewire function)\n");
+ dev_notice(&dev->dev, "MMC cards are now supported by standard SDHCI controller\n");
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_RICOH, PCI_DEVICE_ID_RICOH_R5C832, ricoh_mmc_fixup_r5c832);
+DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_RICOH, PCI_DEVICE_ID_RICOH_R5C832, ricoh_mmc_fixup_r5c832);
+#endif /*CONFIG_MMC_RICOH_MMC*/
+
+
static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
struct pci_fixup *end)
{
--
1.6.3.3

2010-02-01 15:45:55

by Maxim Levitsky

[permalink] [raw]

Subject: Re: [PATCH] Port ricoh_mmc to pci quirk

On Sat, 2010-01-30 at 23:28 +0200, Maxim Levitsky wrote:
> This patch solves nasty problem original driver had.
> Original goal of the ricoh_mmc, was to disable this device because
> then, mmc cards can be read using standard SDHCI controller,
> thus avoiding the need in yet another driver.
> However, the act of disablement, makes other pci functions that belong to
> this controller (xD and memstick) shift up one level, thus pci core has now wrong idea
> about these devices.
>
> To fix this issue, this patch moves the driver into pci quirk section, thus it
> is executed before the pci is enumerated, and therefore solve that issue,
> also the same is preformed on resume for same reasons.
>
> Also regardless of the above, this way is cleaner.
You probably didn't notice that patch, because I used git-send-email,
and forgot to set subject properly.
This meant to be next version of this patch.

Best regards,
Maxim Levitsky