Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755088Ab0FGBlp (ORCPT ); Sun, 6 Jun 2010 21:41:45 -0400 Received: from usul.saidi.cx ([204.11.33.34]:54043 "EHLO usul.overt.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754084Ab0FGBln (ORCPT ); Sun, 6 Jun 2010 21:41:43 -0400 Date: Sun, 6 Jun 2010 18:41:39 -0700 From: Philip Langdale To: Maxim Levitsky Cc: linux-mmc@vger.kernel.org, linux-kernel , Andrew de Quincey Subject: Re: [PATCH 1/2] mmc: make sdhci work with ricoh mmc controller Message-ID: <20100606184139.5505d2a5@fido5> In-Reply-To: <1275871052.24091.7.camel@maxim-laptop> References: <1275859475.3151.4.camel@maxim-laptop> <1275859731-2898-1-git-send-email-maximlevitsky@gmail.com> <1275871052.24091.7.camel@maxim-laptop> X-Mailer: Claws Mail 3.7.6 (GTK+ 2.20.1; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-SA-Do-Not-RunX1: Yes Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6026 Lines: 170 On Mon, 07 Jun 2010 03:37:32 +0300 Maxim Levitsky wrote: > On Sun, 2010-06-06 at 19:11 -0400, Philip Langdale wrote: > > On Mon, 7 Jun 2010 00:28:50 +0300, Maxim Levitsky > > 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' > > > Most of the credit for this goes to Andrew de Quincey > > > > > > > > > Signed-off-by: Maxim Levitsky > > > CC: Andrew de Quincey > > > CC: linux-mmc@vger.kernel.org > > > > ACK with one change to add a #define for the device ID. > I am not sure about this. > When I submitted a driver for xD part I was told that unless device is > shared by several pieces of code, its not ok to add it to pci_ids.c. > The device is I was told can be hardcoded in the driver. > Nothing against changing it. Ok, then leave it as is: Acked-by: Philip Langdale > > > --- > > > drivers/mmc/host/sdhci-pci.c | 31 > > > +++++++++++++++++++++++++++++++ drivers/mmc/host/sdhci.c | > > > 3 ++- drivers/mmc/host/sdhci.h | 4 ++++ > > > 3 files changed, 37 insertions(+), 1 deletions(-) > > > > > > diff --git a/drivers/mmc/host/sdhci-pci.c > > > b/drivers/mmc/host/sdhci-pci.c index 65483fd..c4bcaeb 100644 > > > --- a/drivers/mmc/host/sdhci-pci.c > > > +++ b/drivers/mmc/host/sdhci-pci.c > > > @@ -17,6 +17,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > #include > > > > > > @@ -84,7 +85,21 @@ static int ricoh_probe(struct sdhci_pci_chip > > > *chip) if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG > > > || chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY) > > > chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET; > > > + return 0; > > > +} > > > + > > > +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; > > > return 0; > > > } > > > > As we discussed, highspeed works. Of course, sdhci never sets the > > MMC highspeed flag so the cap is irrelevant. We'd need another > > quirk to indicate highspeed MMC is supported. > > > > This can be done in a separate patch. > Sure, but maybe we can enable this for SD/SDHC too? Not sure what you mean. The proper SD interface reports the HISPD cap already. Do you mean enabling high-speed MMC on a 'proper' SDHCI controller. That's what Pierre didn't want to do blindly. > > > > > @@ -95,6 +110,14 @@ 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_32BIT_DMA_ADDR | > > > + SDHCI_QUIRK_CLOCK_BEFORE_RESET | > > > + SDHCI_QUIRK_NO_CARD_NO_RESET | > > > + SDHCI_QUIRK_MISSING_CAPS > > > +}; > > > + > > > static const struct sdhci_pci_fixes sdhci_ene_712 = { > > > .quirks = SDHCI_QUIRK_SINGLE_POWER_WRITE | > > > SDHCI_QUIRK_BROKEN_DMA, > > > @@ -374,6 +397,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, > > > + }, > > > + > > > + { > > > > It seems we generally want to add a #define for the device ID. > > The ENE device below has one. > > > > > .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..483b78e 100644 > > > --- a/drivers/mmc/host/sdhci.c > > > +++ b/drivers/mmc/host/sdhci.c > > > @@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host) > > > host->version); > > > } > > > > > > - caps = sdhci_readl(host, SDHCI_CAPABILITIES); > > > + caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? > > > host->caps : > > > + sdhci_readl(host, SDHCI_CAPABILITIES); > > > > > > 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..b1839a3 100644 > > > --- a/drivers/mmc/host/sdhci.h > > > +++ b/drivers/mmc/host/sdhci.h > > > @@ -240,6 +240,8 @@ struct sdhci_host { > > > #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN (1<<25) > > > /* Controller cannot support End Attribute in NOP ADMA > > > descriptor */ #define > > > SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC (1<<26) +/* > > > Controller is missing device caps. Use caps provided by host */ > > > +#define SDHCI_QUIRK_MISSING_CAPS (1<<27) > > > int irq; /* Device > > > IRQ */ void __iomem * ioaddr; /* > > > Mapped address */ @@ -292,6 +294,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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/