| Message ID | s5hoc3zd27i.wl%tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <arieslee@jmicron.com> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > 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. Can anyone find a better solution? Thanks, - Chris.
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 <arieslee@jmicron.com> > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Takashi, On Thu, Apr 21 2011, Takashi Iwai wrote: >> 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. I think I'd rather not touch ops at all and keep the code simple -- if you don't mind, please post a version with quirks, and I'll work on freeing up a few bits. Thanks, - Chris.
At Thu, 21 Apr 2011 13:36:03 -0400, Chris Ball wrote: > > Hi Takashi, > > On Thu, Apr 21 2011, Takashi Iwai wrote: > >> 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. > > I think I'd rather not touch ops at all and keep the code simple -- > if you don't mind, please post a version with quirks, and I'll work > on freeing up a few bits. OK, I'll send shortly. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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 <arieslee@jmicron.com> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/mmc/host/sdhci-pci.c | 34 ++++++++++++++++++++++++++++++++++ 1 files changed, 34 insertions(+), 0 deletions(-)