On Mon, 26 Aug 2019 at 20:02, Philip Langdale <philipl@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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 >
