| Message ID | 20190826180222.15960-1-philipl@overt.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: rtsx: Don't attempt to use 'Low-Voltage' VDD for SD cards | expand |
On Mon, 26 Aug 2019 at 20:02, Philip Langdale <philipl@overt.org> wrote: > > There have been multiple reports of failures for newer SD cards > with Realtek readers using the rtsx_pci_sdmmc driver in recent > months, including myself. > > One common theme was that Sandisk A2 cards all failed while older > cards worked fine. The first reports in bugzilla showed up in late > 2018 corresponding with the availabilty of these cards. > > After some false starts, what I realised was that these new cards > have bit 7 set in OCR which means they claim to support the 'Low > Voltage Range' for VDD. This is an incompletely defined concept > in the SD spec - in fact, there is no precise voltage associated > with the 'Low Voltage Range'. We have to go to the MMC spec to > see that it is 1.65-1.95V. > > So, I believe there is currently no legitimate way for an SD > card to operate in the low voltage range, and a card that claims > support by setting bit 7 is simply wrong, although technically > not out of spec. Because the spec doesn't cover how to handle a > card that sets this bit, we really need to act as if the bit is > not set and do normal VDD initialisation. > > And, in fact, this is exactly what happens in the sdhci driver. > By default, it elides MMC_VDD_165_195 for SD cards, and there is > core logic that allows different values of ocr_avail for SD and > MMC (and SDIO, even). > > As such, an equivalent change should be made in the two rtsx > drivers (pci and usb) to explicitly set ocr_avail_sd, leaving > out the low voltage range. > > There's a valid question about whether this elidation should be > moved into the core logic, so that we never try and bring up an > SD card in the low voltage range, for all host controllers. > > For now, at least, I'm going to punt on that question and use > the existing infrastructure to fix the driver I can test. > > I considered setting ocr_avail_sdio as well, but I noted that > the sdhci driver does not do this implicit elidation in that > case, and so didn't want to make the change blindly; I am not > in a position to test it. > > Needless to say, after this change, the Sandisk A2 card works > correctly. Thanks for all the details in the changelog, much appreciated! To put it simple. mmc->ocr_avail_sd|sdio|mmc should never have been introduced in the first place. It's a workaround to something that should have been taken care of better by the core layer, which you also suggests. > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=201447 > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=202473 Ah, I forgot to add these links in my just posted patch. Will do when I apply, if the tests works as expected, of course. Kind regards Uffe > Signed-off-by: Philip Langdale <philipl@overt.org> > --- > drivers/mmc/host/rtsx_pci_sdmmc.c | 4 ++++ > drivers/mmc/host/rtsx_usb_sdmmc.c | 4 ++++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c > index bd50935dc37d..ff78b2abfe8b 100644 > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > @@ -1342,6 +1342,10 @@ static void realtek_init_host(struct realtek_pci_sdmmc *host) > mmc->f_min = 250000; > mmc->f_max = 208000000; > mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195; > + /* MMC_VDD_165_195 is not really defined for SD cards. Ensure we > + * never attempt to initialise a card with the bit set in OCR. > + */ > + mmc->ocr_avail_sd = MMC_VDD_32_33 | MMC_VDD_33_34; > mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED | > MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST | > MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 | MMC_CAP_ERASE; > diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c > index 81d0dfe553a8..590c7c4c189b 100644 > --- a/drivers/mmc/host/rtsx_usb_sdmmc.c > +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c > @@ -1311,6 +1311,10 @@ static void rtsx_usb_init_host(struct rtsx_usb_sdmmc *host) > mmc->f_min = 250000; > mmc->f_max = 208000000; > mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195; > + /* MMC_VDD_165_195 is not really defined for SD cards. Ensure we > + * never attempt to initialise a card with the bit set in OCR. > + */ > + mmc->ocr_avail_sd = MMC_VDD_32_33 | MMC_VDD_33_34; > mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED | > MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST | > MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 | MMC_CAP_UHS_SDR50 | > -- > 2.20.1 >
diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c index bd50935dc37d..ff78b2abfe8b 100644 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c @@ -1342,6 +1342,10 @@ static void realtek_init_host(struct realtek_pci_sdmmc *host) mmc->f_min = 250000; mmc->f_max = 208000000; mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195; + /* MMC_VDD_165_195 is not really defined for SD cards. Ensure we + * never attempt to initialise a card with the bit set in OCR. + */ + mmc->ocr_avail_sd = MMC_VDD_32_33 | MMC_VDD_33_34; mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST | MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 | MMC_CAP_ERASE; diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c index 81d0dfe553a8..590c7c4c189b 100644 --- a/drivers/mmc/host/rtsx_usb_sdmmc.c +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c @@ -1311,6 +1311,10 @@ static void rtsx_usb_init_host(struct rtsx_usb_sdmmc *host) mmc->f_min = 250000; mmc->f_max = 208000000; mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195; + /* MMC_VDD_165_195 is not really defined for SD cards. Ensure we + * never attempt to initialise a card with the bit set in OCR. + */ + mmc->ocr_avail_sd = MMC_VDD_32_33 | MMC_VDD_33_34; mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST | MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 | MMC_CAP_UHS_SDR50 |
There have been multiple reports of failures for newer SD cards with Realtek readers using the rtsx_pci_sdmmc driver in recent months, including myself. One common theme was that Sandisk A2 cards all failed while older cards worked fine. The first reports in bugzilla showed up in late 2018 corresponding with the availabilty of these cards. After some false starts, what I realised was that these new cards have bit 7 set in OCR which means they claim to support the 'Low Voltage Range' for VDD. This is an incompletely defined concept in the SD spec - in fact, there is no precise voltage associated with the 'Low Voltage Range'. We have to go to the MMC spec to see that it is 1.65-1.95V. So, I believe there is currently no legitimate way for an SD card to operate in the low voltage range, and a card that claims support by setting bit 7 is simply wrong, although technically not out of spec. Because the spec doesn't cover how to handle a card that sets this bit, we really need to act as if the bit is not set and do normal VDD initialisation. And, in fact, this is exactly what happens in the sdhci driver. By default, it elides MMC_VDD_165_195 for SD cards, and there is core logic that allows different values of ocr_avail for SD and MMC (and SDIO, even). As such, an equivalent change should be made in the two rtsx drivers (pci and usb) to explicitly set ocr_avail_sd, leaving out the low voltage range. There's a valid question about whether this elidation should be moved into the core logic, so that we never try and bring up an SD card in the low voltage range, for all host controllers. For now, at least, I'm going to punt on that question and use the existing infrastructure to fix the driver I can test. I considered setting ocr_avail_sdio as well, but I noted that the sdhci driver does not do this implicit elidation in that case, and so didn't want to make the change blindly; I am not in a position to test it. Needless to say, after this change, the Sandisk A2 card works correctly. BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=201447 BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=202473 Signed-off-by: Philip Langdale <philipl@overt.org> --- drivers/mmc/host/rtsx_pci_sdmmc.c | 4 ++++ drivers/mmc/host/rtsx_usb_sdmmc.c | 4 ++++ 2 files changed, 8 insertions(+)