| Message ID | 20190811021917.29736-1-philipl@overt.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: rtsx_pci: Do not set MMC_CAP2_FULL_PWR_CYCLE | expand |
On Sun, 11 Aug 2019 at 04:25, 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 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. > > I did some experiments and saw that removing the > MMC_CAP2_FULL_PWR_CYCLE cap from the driver made the problem > go away. That then causes us to ask why it helps. This cap was > introduced to provide a way for a host controller to indicate it > can fully power cycle a card. Then in ce69d37, we introduced > power cycling a card as part of changing the operation voltage > of the card. First, let's align on the terminology. "vmmc" is the power to the card. Normal range for SD cards is 2.7V to 3.6V. "vqmmc" is the I/O voltage level. Legacy SD cards supports only 3.3V, while UHS-I cards supports both 3.3V and 1.8V. > > Now, this is the part where I'm under-informed, but reading the > SD spec, it says that the card will reset to 3.3V operations on > a power cycle, which seems to make sense, as it always has to > be negotiated. To clarify, the vqmmc (I/O voltage level) internally expected by an SD card after a reset (even after a soft reset with CMD0) is always 3.3V. For vmmc, it's okay to stick to a pre-negotiated voltage level after a reset. That's actually already what the mmc core does, during a system resume. > > Then, we observe that A2 cards are _required_ to support low > voltage signalling (1.8V) and this is, I think, the only host > visible difference in A2 vs older cards. And in my testing > (with a deficient set of cards, I admit - I'm away from my > main collection), I see that the A2 card does indeed get > switched to 1.8V while older (pre-UHS) cards always run at > 3.3V. I unfortunately lack any UHS pre-A2 cards to test > whether 1.8V capable pre-A2 cards fail or succeed, but I > would expect them to fail (and maybe no one was making 1.8V > cards routinely until A2). > > This leaves me slightly undercertain as to the root of the > problem. Perhaps the issue is that the driver should not have > ever set this flag, because the hardware doesn't actually support > the functionality that the flag corresponds to. Or, perhaps the > problem is that power cycling on setting the voltage is never > correct for an SD card, or only correct for eMMC, etc. > > There are a handful of embedded controllers that the cap is set > for in device tree and the rtsx controllers (pci & usb) are the > only 'traditional' drivers that set the cap. As such, I think > the most likely explaination is that the cap was incorrectly > set in the first place, based on a misunderstanding of what > it actually means. I admit, it's a bit unclear of what MMC_CAP2_FULL_PWR_CYCLE means for SD cards. For eMMC it's rather straight forward; it means the host supports to power cycle *both* vmmc and vqmmc. This is needed to reset the internal state of the eMMC (useful when another HW-reset option isn't supported). For SD cards, being able to power cycle vmmc is sufficient, as the internal logic of the SD card should be reset by cutting vmmc. In other words, if your host can power cycle vmmc, it seems correct to use MMC_CAP2_FULL_PWR_CYCLE. > > So, let's stop setting the cap, and observe that A2 cards > now work 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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c > index bd50935dc37d..1d7c942fc7f3 100644 > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > @@ -1345,7 +1345,7 @@ static void realtek_init_host(struct realtek_pci_sdmmc *host) > 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; > - mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP | MMC_CAP2_FULL_PWR_CYCLE; > + mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP; > mmc->max_current_330 = 400; > mmc->max_current_180 = 800; > mmc->ops = &realtek_pci_sdmmc_ops; > -- > 2.20.1 > I am not convinced that the patch is correct, but I may be wrong. A few things that I wonder about. 1. To support switch UHS-I SD cards into 1.8V I/O, the host should implement the ->card_busy() host ops. This isn't the case for rtsx_pci_sdmmc.c (you should see a warning printed to the log "cannot verify signal voltage switch" during the card initialization), but instead it internally tries to deal with the 1.8V I/O voltage switch inside its ->start_signal_voltage_switch() ops. It looks fragile to me. No matter what, I think it's a good idea to convert into using the ->card_busy() ops. 2. The mmc core expects hosts that supports 3.3V and 1.8V I/O, to be able to move from 1.8V back to 3.3V. Perhaps that path isn't so well tested for rtsx_pci_sdmmc.c and maybe it's a good idea to make sure this works as expected. Kind regards Uffe
On 2019-08-21 07:55, Ulf Hansson wrote: >> >> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c >> b/drivers/mmc/host/rtsx_pci_sdmmc.c >> index bd50935dc37d..1d7c942fc7f3 100644 >> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c >> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c >> @@ -1345,7 +1345,7 @@ static void realtek_init_host(struct >> realtek_pci_sdmmc *host) >> 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; >> - mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP | >> MMC_CAP2_FULL_PWR_CYCLE; >> + mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP; >> mmc->max_current_330 = 400; >> mmc->max_current_180 = 800; >> mmc->ops = &realtek_pci_sdmmc_ops; >> -- >> 2.20.1 >> > > I am not convinced that the patch is correct, but I may be wrong. A > few things that I wonder about. > > 1. > To support switch UHS-I SD cards into 1.8V I/O, the host should > implement the ->card_busy() host ops. This isn't the case for > rtsx_pci_sdmmc.c (you should see a warning printed to the log > "cannot verify signal voltage switch" during the card initialization), > but instead it internally tries to deal with the 1.8V I/O voltage > switch inside its ->start_signal_voltage_switch() ops. > > It looks fragile to me. No matter what, I think it's a good idea to > convert into using the ->card_busy() ops. Interestingly, the rtsx_usb_sdmmc driver implements card_busy, so this was a conscious effort from the original author(s). However, it's not relevant to the bug here, as things fall apart before we get to UHS-I switching. > 2. > The mmc core expects hosts that supports 3.3V and 1.8V I/O, to be able > to move from 1.8V back to 3.3V. Perhaps that path isn't so well tested > for rtsx_pci_sdmmc.c and maybe it's a good idea to make sure this > works as expected. So, I did more debugging and I've narrowed down the problem. You are correct, that this is not directly caused by FULL_PWR_CYCLE. What is actually happening is that these Sandisk A2 cards (Is anyone else selling an A2 card? I haven't seen one) claim to support the low voltage range in OCR (bit 7) for vmmc. At least for me, that's unusual. I've never seen an SD card support the low voltage range - It was introduced for MMC(plus) and I'd only ever seen it on those cards. This seems to be the key difference from the older UHS-1 cards (which I've now been able to test and they work fine with the driver - so it really is specific to these new A2 cards). Anyway. So what is happening is that the mmc_select_voltage logic tries to switch to the lowest supported voltage and then issues the pwr_cycle sequence if the controller supports it. Then, the card comes up in some kind of broken state, and everything fails past that point. With that in mind, I removed the host flag that indicates support for the low voltage range, and now the card works fine with FULL_PWR_CYCLE retained. Fun times. So, what's actually wrong? Well, either the controller doesn't really support the low voltage range, or the card doesn't. As my machine here only has a microsd slot, I can't test with an MMCplus card to see if low voltage MMC would work. Without that I can't say whether we should remove the host flag or somehow blacklist low voltage range on these cards (or any SD card?) Would love to get your thoughts. --phil
On Thu, 22 Aug 2019 16:05:22 -0700 Philip Langdale <philipl@overt.org> wrote: > So, what's actually wrong? Well, either the controller doesn't really > support the > low voltage range, or the card doesn't. As my machine here only has a > microsd slot, > I can't test with an MMCplus card to see if low voltage MMC would > work. Without that > I can't say whether we should remove the host flag or somehow > blacklist low voltage > range on these cards (or any SD card?) > > Would love to get your thoughts. I did more reading and thinking. In the MMC spec, a low-voltage (1.8V) supporting card can run at 1.8V directly from start up (and this is what the existing core logic does). But for SD cards, i think this is not true. I read the Low-Voltage-Signaling announcement which says there is a new negotiation mechanism but I can't find it documented in the SD 6.0 spec (despite the announcement saying it's part of the SD 6.0 spec). That implies that direct bring up at low voltage doesn't work like it does for an MMC card. Also, again without seeing an actual spec, I'd guess this means that having bit 7 set in OCR cannot be interpreted the same way for an SD card as for an MMC card. Perhaps it should be read to mean that the LVS negotiation process should be executed, but even that might be silly, because the whole point of LVS is to support scenarios where only 1.8V is usable - that means negotiation must be done with no prior knowledge. So you can't read OCR until you already know that 1.8V is supported. So maybe the punchline here is that shared logic that will try and bring up a card at 1.8V should be MMC only and for SD cards, the LVS negotiation should be implemented instead. Finally, this Sandisk A2 card doesn't have the LV logo on it, so I don't know whether it would support 1.8V operation if the LVS negotiation was done directly or not - and I don't know whether setting bit 7 in OCR is correct for an LV compliant card. Lots of open questions. --phil
On Sun, 25 Aug 2019 at 00:25, Philip Langdale <philipl@overt.org> wrote: > > On Thu, 22 Aug 2019 16:05:22 -0700 > Philip Langdale <philipl@overt.org> wrote: > > > So, what's actually wrong? Well, either the controller doesn't really > > support the > > low voltage range, or the card doesn't. As my machine here only has a > > microsd slot, > > I can't test with an MMCplus card to see if low voltage MMC would > > work. Without that > > I can't say whether we should remove the host flag or somehow > > blacklist low voltage > > range on these cards (or any SD card?) > > > > Would love to get your thoughts. > > I did more reading and thinking. In the MMC spec, a low-voltage (1.8V) > supporting card can run at 1.8V directly from start up (and this is > what the existing core logic does). But for SD cards, i think this is > not true. I read the Low-Voltage-Signaling announcement which says > there is a new negotiation mechanism but I can't find it documented in > the SD 6.0 spec (despite the announcement saying it's part of the SD > 6.0 spec). That implies that direct bring up at low voltage doesn't > work like it does for an MMC card. Thanks a lot for analyzing the problem more deeply! As far as I can understand, the the OCR register for SD card represents the voltage range for VDD (and VDD1 for UHS-II cards), which is represented by the vmmc regulator in the mmc core. The OCR is isn't about the I/O signaling voltage, at least to my understanding. When in comes to "dual voltage" SD cards, I think we can simply forget about those, at least until someone come and says we need to support then. Moreover, reading the spec doesn't really mention much about "dual voltage" SD cards. > > Also, again without seeing an actual spec, I'd guess this means that > having bit 7 set in OCR cannot be interpreted the same way for an SD > card as for an MMC card. Perhaps it should be read to mean that the LVS > negotiation process should be executed, but even that might be silly, > because the whole point of LVS is to support scenarios where only 1.8V > is usable - that means negotiation must be done with no prior > knowledge. So you can't read OCR until you already know that 1.8V is > supported. Again, OCR is probably not about LVS. Becuase LVS is about supporting to initialize the SD card in 1.8V signal voltage. > > So maybe the punchline here is that shared logic that will try and > bring up a card at 1.8V should be MMC only and for SD cards, the LVS > negotiation should be implemented instead. > > Finally, this Sandisk A2 card doesn't have the LV logo on it, so I > don't know whether it would support 1.8V operation if the LVS > negotiation was done directly or not - and I don't know whether setting > bit 7 in OCR is correct for an LV compliant card. Yeah. I think we can agree on that having bit7 set in OCR is not really useful for SD card. In principle I think we should just just ignore it for all SD cards. Do you want to send a patch, or if you prefer, I can do it!? Kind regards Uffe
On 2019-08-26 08:39, Ulf Hansson wrote: >> >> Finally, this Sandisk A2 card doesn't have the LV logo on it, so I >> don't know whether it would support 1.8V operation if the LVS >> negotiation was done directly or not - and I don't know whether >> setting >> bit 7 in OCR is correct for an LV compliant card. > > Yeah. > > I think we can agree on that having bit7 set in OCR is not really > useful for SD card. In principle I think we should just just ignore it > for all SD cards. > > Do you want to send a patch, or if you prefer, I can do it!? I started looking at how we might make a general change to ignore for all cards, and in the process, I saw that we already have a way to differentiate ocr_avail for different card types, and the sdhci driver uses this to elide the low voltage range when dealing with SD cards. So I've made the small change to have the rtsx drivers set ocr_avail_sd and skip the low voltage range support. This makes the cards work, as you'd expect. There's still a fair claim that the elidation logic should move into the core, so that all host controllers automatically benefit, but I think I should leave that to you. It's a core change and I don't pretend to understand all the considerations for all the various supported controllers. I've sent a new diff with the rtsx-specific fix. Thanks, --phil
On Mon, 26 Aug 2019 11:06:26 -0700 Philip Langdale <philipl@overt.org> wrote: > > I started looking at how we might make a general change to ignore for > all > cards, and in the process, I saw that we already have a way to > differentiate > ocr_avail for different card types, and the sdhci driver uses this to > elide the low voltage range when dealing with SD cards. So I've made > the small change to have the rtsx drivers set ocr_avail_sd and skip > the low voltage > range support. This makes the cards work, as you'd expect. > > There's still a fair claim that the elidation logic should move into > the core, > so that all host controllers automatically benefit, but I think I > should leave > that to you. It's a core change and I don't pretend to understand all > the > considerations for all the various supported controllers. > > I've sent a new diff with the rtsx-specific fix. > More reading == more thoughts. So, I went through the simplified 6.0 spec more carefully and this is what I've extracted: * Low Voltage Signalling: The 1.8V I/O voltage we already support * Low Voltage Interface: A new 6.0 feature for setting VDD to 1.8V The LVI is documented in the "Low Voltage Interface Addendum" that is not included in the simplified spec. Why? Who knows. Do you have access to this by any chance? There are specific references in the simplified spec when discussing the OCR that make it pretty clear that bit 7 in the OCR indicates the card supports LVI and can be initialised in 1.8V mode if the LVI negotiation is followed. There is also a clear statement that an A2 card must support LVI. That means that the Sandisk card I'm looking at is 'correctly' setting bit 7 in OCR to indicate LVI support. And there's no "LV" logo because A2 implies LVI. It's deeply annoying that Low Voltage initialisation is done differently for SD vs MMC despite using the same mechanism to indicate support. Overall, this implies that until we can implement LVI negotiation, it would be semantically correct to ignore bit 7 explicitly for SD cards, and hopefully we'd eventually be able to implement the negotiation and support low voltage VDD when the controller and card support it. --phil
On Tue, 27 Aug 2019 at 05:31, Philip Langdale <philipl@overt.org> wrote: > > On Mon, 26 Aug 2019 11:06:26 -0700 > Philip Langdale <philipl@overt.org> wrote: > > > > > I started looking at how we might make a general change to ignore for > > all > > cards, and in the process, I saw that we already have a way to > > differentiate > > ocr_avail for different card types, and the sdhci driver uses this to > > elide the low voltage range when dealing with SD cards. So I've made > > the small change to have the rtsx drivers set ocr_avail_sd and skip > > the low voltage > > range support. This makes the cards work, as you'd expect. > > > > There's still a fair claim that the elidation logic should move into > > the core, > > so that all host controllers automatically benefit, but I think I > > should leave > > that to you. It's a core change and I don't pretend to understand all > > the > > considerations for all the various supported controllers. > > > > I've sent a new diff with the rtsx-specific fix. > > > > More reading == more thoughts. > > So, I went through the simplified 6.0 spec more carefully and this is > what I've extracted: > > * Low Voltage Signalling: The 1.8V I/O voltage we already support > * Low Voltage Interface: A new 6.0 feature for setting VDD to 1.8V I have the full specs available, as Linaro is a member of the SD Association. I can't find anything that confirms that "Low Voltage Interface" has anything to do with the supply voltage (VDD). It's all about signaling voltage, unless I am mistaken somehow. All I can find for VDD (or VDD1) is that it always needs to stay between 2.7V - 3.6V. > > The LVI is documented in the "Low Voltage Interface Addendum" that is > not included in the simplified spec. Why? Who knows. Do you have access > to this by any chance? I have the spec. The below is stated in the initial part from the LVI addendum. "There has been a demand of low voltage signaling interface memory card for especially mobile host devices. To meet the demand, this addendum defines low voltage signaling interface in SD mode (called "LVS interface") that starts in 1.8V signaling UHS-I mode without going through 3.3V signaling, that is host can start card communication in SDR12 mode from the start of initialization and skips Voltage Switch Sequence (CMD11). "LVS" denotes Low Voltage Signaling for communication lines but 3.3V power supply is still used." > > There are specific references in the simplified spec when discussing > the OCR that make it pretty clear that bit 7 in the OCR indicates the > card supports LVI and can be initialised in 1.8V mode if the LVI > negotiation is followed. Actually no, I don't think so. Bit7 in OCR is a very old thing and it's about "dual voltage cards" and not LVI. I did some more research to really understand. By searching for "dual voltage" in the spec(s) and bit7 in OCR. I found it being introduced already in version 2.00. Then I realized that the current text in 7.1, is a leftover that should have been removed altogether and not only parts as was done in version 3.01. My guess, is that it was probably introduced while the "eSD" spec was being developed, as an alternative to eMMC. Additionally, reading the LVI addendum and especially the LVI initialization sequence for a LV compatible host+card, I found nothing that mentions bit7 in the OCR register. > > There is also a clear statement that an A2 card must support LVI. > > That means that the Sandisk card I'm looking at is 'correctly' setting > bit 7 in OCR to indicate LVI support. And there's no "LV" logo because > A2 implies LVI. > > It's deeply annoying that Low Voltage initialisation is done > differently for SD vs MMC despite using the same mechanism to indicate > support. > > Overall, this implies that until we can implement LVI negotiation, it > would be semantically correct to ignore bit 7 explicitly for SD cards, > and hopefully we'd eventually be able to implement the negotiation and > support low voltage VDD when the controller and card support it. According to the above, I don't think we ever need to care about bit 7 in the OCR. But yes, supporting LVI may be complicated, but that's another story. :-) Anyway, I decided to post a patch, please test it at your side and see if it fixes the problem for you. Kind regards Uffe
On Tue, 27 Aug 2019 10:12:15 +0200 Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > According to the above, I don't think we ever need to care about bit 7 > in the OCR. But yes, supporting LVI may be complicated, but that's > another story. :-) Yes. Sorry, this has been very confusing for me. With the context you provided, it is clear the LVI is still referring to signal voltage. So the bit 7 being set for these cards looks like a mistake and out-of-spec. Surprising from Sandisk. > Anyway, I decided to post a patch, please test it at your side and see > if it fixes the problem for you. Took a look and responded. Did the trick, as you'd expect. Much thanks! --phil
diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c index bd50935dc37d..1d7c942fc7f3 100644 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c @@ -1345,7 +1345,7 @@ static void realtek_init_host(struct realtek_pci_sdmmc *host) 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; - mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP | MMC_CAP2_FULL_PWR_CYCLE; + mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP; mmc->max_current_330 = 400; mmc->max_current_180 = 800; mmc->ops = &realtek_pci_sdmmc_ops;
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 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. I did some experiments and saw that removing the MMC_CAP2_FULL_PWR_CYCLE cap from the driver made the problem go away. That then causes us to ask why it helps. This cap was introduced to provide a way for a host controller to indicate it can fully power cycle a card. Then in ce69d37, we introduced power cycling a card as part of changing the operation voltage of the card. Now, this is the part where I'm under-informed, but reading the SD spec, it says that the card will reset to 3.3V operations on a power cycle, which seems to make sense, as it always has to be negotiated. Then, we observe that A2 cards are _required_ to support low voltage signalling (1.8V) and this is, I think, the only host visible difference in A2 vs older cards. And in my testing (with a deficient set of cards, I admit - I'm away from my main collection), I see that the A2 card does indeed get switched to 1.8V while older (pre-UHS) cards always run at 3.3V. I unfortunately lack any UHS pre-A2 cards to test whether 1.8V capable pre-A2 cards fail or succeed, but I would expect them to fail (and maybe no one was making 1.8V cards routinely until A2). This leaves me slightly undercertain as to the root of the problem. Perhaps the issue is that the driver should not have ever set this flag, because the hardware doesn't actually support the functionality that the flag corresponds to. Or, perhaps the problem is that power cycling on setting the voltage is never correct for an SD card, or only correct for eMMC, etc. There are a handful of embedded controllers that the cap is set for in device tree and the rtsx controllers (pci & usb) are the only 'traditional' drivers that set the cap. As such, I think the most likely explaination is that the cap was incorrectly set in the first place, based on a misunderstanding of what it actually means. So, let's stop setting the cap, and observe that A2 cards now work 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)