From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755217Ab1DURJj (ORCPT ); Thu, 21 Apr 2011 13:09:39 -0400 Received: from cantor.suse.de ([195.135.220.2]:57268 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755180Ab1DURJh (ORCPT ); Thu, 21 Apr 2011 13:09:37 -0400 Date: Thu, 21 Apr 2011 19:09:36 +0200 Message-ID: From: Takashi Iwai To: Chris Ball Cc: Aries Lee , linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mmc: Fix read-only detection with JMicron 388 chip In-Reply-To: References: User-Agent: Wanderlust/2.15.6 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.7 Emacs/23.2 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org At Thu, 21 Apr 2011 12:40:26 -0400, Chris Ball wrote: > > Hi Takashi, > > On Thu, Apr 21 2011, Takashi Iwai wrote: > > On HP laptops with JMicron 388 chip, the write-locked SD card isn't > > detected correctly as read-only in many cases. This is because the > > PRESENT_STATE register becomes unsable just after plugging, and it > > returns the WRITE_PROTECT bit wrongly at the first read. > > > > This patch fixes the read-only detection by adding the own get_ro op > > for checking the register more intensively with a relatively long > > delay. > > > > The patch is tested with 2.6.39-rc4 kernel. > > > > Cc: Aries Lee > > Signed-off-by: Takashi Iwai > > --- > > drivers/mmc/host/sdhci-pci.c | 34 ++++++++++++++++++++++++++++++++++ > > 1 files changed, 34 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c > > index a136be7..9037b2a 100644 > > --- a/drivers/mmc/host/sdhci-pci.c > > +++ b/drivers/mmc/host/sdhci-pci.c > > @@ -346,6 +346,35 @@ static void jmicron_enable_mmc(struct sdhci_host *host, int on) > > writeb(scratch, host->ioaddr + 0xC0); > > } > > > > +/* As PRESENT_STATE register becomes unstable just after plugging, > > + * need to check the RO-cap multiple times with a relatively long delay. > > + */ > > + > > +#define SAMPLE_COUNT 5 > > + > > +static unsigned int jmicron_get_ro(struct sdhci_host *host) > > +{ > > + int i, ro_count; > > + > > + ro_count = 0; > > + for (i = 0; i < SAMPLE_COUNT; i++) { > > + int present = sdhci_readl(host, SDHCI_PRESENT_STATE); > > + if (!(present & SDHCI_WRITE_PROTECT)) { > > + if (++ro_count > SAMPLE_COUNT / 2) > > + return 1; > > + } > > + msleep(30); > > + } > > + return 0; > > +} > > + > > +static int sdhci_pci_enable_dma(struct sdhci_host *host); > > + > > +static struct sdhci_ops jmicron_pci_ops = { > > + .enable_dma = sdhci_pci_enable_dma, > > + .get_ro = jmicron_get_ro, > > +}; > > + > > static int jmicron_probe_slot(struct sdhci_pci_slot *slot) > > { > > if (slot->chip->pdev->revision == 0) { > > @@ -383,6 +412,11 @@ static int jmicron_probe_slot(struct sdhci_pci_slot *slot) > > > > slot->host->mmc->caps |= MMC_CAP_BUS_WIDTH_TEST; > > > > + /* Replace the ops for unstable get_ro detection */ > > + if (slot->chip->pdev->device == PCI_DEVICE_ID_JMICRON_JMB388_SD || > > + slot->chip->pdev->device == PCI_DEVICE_ID_JMICRON_JMB388_ESD) > > + slot->host->ops = &jmicron_pci_ops; > > + > > return 0; > > } > > I don't like overwriting ops here -- it's too magical, and now we have > to maintain the ops table in two places. A quirk seems justified here, > even though we're trying to reduce them in general. Well, I also used quirk bit in my very first version I worked for 2.6.32 kernel. But when I looked at 2.6.39, quirks are almost full -- only the last one bit is left for bit 31. So I didn't want to finish it :) > Can anyone find a better solution? One way would be to copy the ops table itself in struct sdhci_host instead of keeping the ops table pointer. Then you can overwrite only the specific op in each probe_slot callback. thanks, Takashi