MMC/SDHCI fixes

PATCH Superseded Russell King - ARM Linux 3 participants 28 messages Dec 19, 2015 – Dec 21, 2015

Subsystems (2)

MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Russell King - ARM Linux

author

2015-12-19 20:29 UTC #0 lore raw

cover MMC/SDHCI fixes

This all started with a report from an iMX6 Hummingboard user that they were seeing regular "mmc0: Got data interrupt 0x00000002 even though no data operation was in progress." messages with 4.4-rc and additional patches to enable UHS support in DT for the platform.

When I tested 4.4-rc on an iMX6 Hummingboard with UHS support enabled, I found several other issues. This patch series addresses some of the issues.

However, while testing on Armada 388, an issue was found there, and a fix is also included in this patch set, however it does not depend on the rest of the patch series.

We all know that the SDHCI driver is in poor state, and Ulf's desire to see it turned into a library (which actually comes from myself.) However, with the driver in such a poor state, it makes sense to fix a few of the issues first, especially if they result in the code shrinking - which they do.

However, given what I've found below, I have to wonder how many of the quirks that we have in this driver are down to the poor code rather than real hardware problems: how many of them are due to poorly investigated bugs.

This series starts off by shutting up some kernel messages that should not be verbose:
- the voltage-ranges DT property is optional, so we should not print a message saying that it's missing from DT. That suggests it isn't optional. As no ill effect comes from not specifying it, I've dropped this message to debug level.

- "retrying because a re-tune was needed" - a re-tune is something that is part of the higher speed SD specs, and it's required to happen periodically. So it's expected and normal behaviour. There's no need to print this to the kernel log.

- When tuning fails, rather than a bland "mmc0: tuning execution failed" message, print the error code so that we can see what the reason for the failure is. MMC fails on this point in many places.

- Move the initialisation of the command ->error member. It looks to me as if this is not required; mmc_start_request() does this already, as it seems the rest of the MMC core does too. Maybe this should be removed altogether?

- Testing error bits, then setting the error, then testing the error value, and completing the command is a very lengthy way to deal with errors here. Test the error bits, and then decide what error occurred before completing. (This code is actually buggy, see the next change).

- If a command _response_ CRC error occurs, we can't be certain whether the card received the command successfully, and has started processing it. If it has, and it results in data being read from the card, the card will enter data transfer state, and start sending data to the host. Therefore, it is completely wrong to blindly terminate the command, especially when such termination fails to clean _anything_ up - eg, it leaves DMA in place so the host can continue DMAing to the memory, and it leaves buffers DMA mapped - which eventually annoys the DMA API debugging. Fix this by assuming that a CRC error is a receive side error. If the card did not correctly receive the command, it will not initiate a data transfer, and we should time out after the card specified timeout.

With the unmodified mainline code, even if we reset the state machines when finishing a request in error, the _card_ itself may well still be in data transfer mode when this happens. How can we be sure what state the data side is actually in? Could this be why (eg) sdhci-esdhc-imx.c has this:

        /* FIXME: delay a bit for card to be ready for next tuning due to errors */
        mdelay(1);

And how many more such _hacks_ are working around similar problems?

- The unaligned buffer handling is a total waste of resources in its current form. We DMA map, sync, and unmap it on every transfer, whether the buffer is used or not. For MMC/SD transfers, these will be coming from the VFS/MM layers, which operate through the page cache - hence the vast majority of buffers will be page aligned. Performance measurements on iMX6 show that this additional DMA maintanence is responsible for a 10% reduction in read transfer performance. Switch this to use a DMA coherent mapping instead, which needs no DMA maintanence.

- sdhci_adma_table_post() is badly coded: we walk the scatterlist looking for any buffers which might not be aligned (which can be a rare event, see above) and then we only do something if we find an unaligned buffer _and_ it's a read transfer. So for a write transfer, this scatterlist walking is a total waste of CPU cycles. Testing the transfer direction is much cheaper than walking the scatterlist. Re-organise the code to do the cheap test first.

- There are two paths to identical DMA unmapping code (the code around dma_unmap_sg()) in sdhci_finish_data() and its child function sdhci_adma_table_post(). Trivially simplify this duplication.

- Move sdhci_pre_dma_transfer() so we avoid needing to declare it.

- Do the same thing with sdhci_prepare_data() and sdhci_pre_dma_transfer() as with sdhci_finish_data() and sdhci_adma_table_post().

- Fix the mess that is the data segment host_cookie handling. The existing code seems like a candidate for the obfuscated coding prize! Four patches address this, turning it into something much easier to understand, which then allows (finally)...

- The DMA API leak which occurs when we have mapped the DMA buffers in sdhci_prepare_data() and an error occurs early on in the processing of a request.

- A final change (mentioned above) which fixes a prior commit for Armada 38x using sdhci-pxav3.c, found while trying to get the higher speed modes working on these SoCs. Clearly the existing code is broken, this at least resolves some of the breakage by allowing the correct high-order SDHCI capabilities through.

All these patches are resulting from _only_ today; I'm sure more will be coming in the forthcoming weeks, as all I seem to have to do is give code a little shake and _lots_ of bugs appear. Despite this being a series of 17 patches, it does not yet solve _all_ the problems I've found so far on iMX6 with SDHCI.

I've tested these changes on iMX6 at UHS speeds, and Armada 388 SoCs at HS speeds so far. Others should test them.

 drivers/mmc/card/block.c       |   4 +-
 drivers/mmc/core/core.c        |  10 +-
 drivers/mmc/host/sdhci-pxav3.c |   6 +-
 drivers/mmc/host/sdhci.c       | 318 ++++++++++++++++++-----------------------
 drivers/mmc/host/sdhci.h       |   4 +-
 5 files changed, 157 insertions(+), 185 deletions(-)

Signature · 3 lines

RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Russell King

2015-12-19 20:29 UTC #1 lore raw

PATCH 1/17 mmc: core: shut up "voltage-ranges unspecified" pr_info()

Each time a driver such as sdhci-esdhc-imx is probed, we get a info printk complaining that the DT voltage-ranges property has not been specified.

However, the DT binding specifically says that the voltage-ranges property is optional. That means we should not be complaining that DT hasn't specified this property: by indicating that it's optional, it is valid not to have the property in DT.

Silence the warning if the property is missing.

Signed-off-by Russell King <[email protected]>

---
 drivers/mmc/core/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/mmc/core/core.c +6 -2

index 5ae89e48fd85..b5e663b67cb5 100644
@@ -1220,8 +1220,12 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 
 	voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
 	num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
-	if (!voltage_ranges || !num_ranges) {
-		pr_info("%s: voltage-ranges unspecified\n", np->full_name);
+	if (!voltage_ranges) {
+		pr_debug("%s: voltage-ranges unspecified\n", np->full_name);
+		return -EINVAL;
+	}
+	if (!num_ranges) {
+		pr_err("%s: voltage-ranges empty\n", np->full_name);
 		return -EINVAL;
 	}

Signature · 1 line

2.1.0

Ulf Hansson

2015-12-21 10:18 UTC #2 lore raw

Re: [PATCH 01/17] mmc: core: shut up "voltage-ranges unspecified" pr_info()

Quoted 8 lines · Russell King

Each time a driver such as sdhci-esdhc-imx is probed, we get a info printk complaining that the DT voltage-ranges property has not been specified.
However, the DT binding specifically says that the voltage-ranges property is optional. That means we should not be complaining that DT hasn't specified this property: by indicating that it's optional, it is valid not to have the property in DT.

Agree!

Quoted 13 lines

Silence the warning if the property is missing.
Signed-off-by Russell King <[email protected]>
---
 drivers/mmc/core/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/mmc/core/core.c +0 -0
index 5ae89e48fd85..b5e663b67cb5 100644

drivers/mmc/core/core.c int mmc_of_parse_voltage(struct device_node *np, u32 *mask) · line 1225

Quoted 8 lines

drivers/mmc/core/core.c +3 -2

@@ -1220,8 +1220,12 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 
        voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
        num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
-       if (!voltage_ranges || !num_ranges) {
-               pr_info("%s: voltage-ranges unspecified\n", np->full_name);
+       if (!voltage_ranges) {
+               pr_debug("%s: voltage-ranges unspecified\n", np->full_name);
+               return -EINVAL;

Because it's optional, I don't think we should return an error code here.

drivers/mmc/core/core.c int mmc_of_parse_voltage(struct device_node *np, u32 *mask) · line 1229

Quoted 4 lines

drivers/mmc/core/core.c +3 -0

+       }
+       if (!num_ranges) {
+               pr_err("%s: voltage-ranges empty\n", np->full_name);
                return -EINVAL;

Ditto.

Quoted 5 lines

drivers/mmc/core/core.c +0 -1
        }
 
--
2.1.0

Kind regards
Uffe

Russell King - ARM Linux

author

2015-12-21 10:24 UTC #3 lore raw

Re: [PATCH 01/17] mmc: core: shut up "voltage-ranges unspecified" pr_info()

Quoted 26 lines · Ulf Hansson

Quoted 8 lines · Russell King
Each time a driver such as sdhci-esdhc-imx is probed, we get a info printk complaining that the DT voltage-ranges property has not been specified.
However, the DT binding specifically says that the voltage-ranges property is optional. That means we should not be complaining that DT hasn't specified this property: by indicating that it's optional, it is valid not to have the property in DT.
Agree!
Quoted 14 lines
Silence the warning if the property is missing.
Signed-off-by Russell King <[email protected]>
---
 drivers/mmc/core/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/mmc/core/core.c +0 -0
index 5ae89e48fd85..b5e663b67cb5 100644
@@ -1220,8 +1220,12 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 

Quoted 9 lines

drivers/mmc/core/core.c int mmc_of_parse_voltage(struct device_node *np, u32 *mask) · line 1225

Quoted 7 lines

drivers/mmc/core/core.c +3 -2

        voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
        num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
-       if (!voltage_ranges || !num_ranges) {
-               pr_info("%s: voltage-ranges unspecified\n", np->full_name);
+       if (!voltage_ranges) {
+               pr_debug("%s: voltage-ranges unspecified\n", np->full_name);
+               return -EINVAL;

Because it's optional, I don't think we should return an error code here.

Maybe, but that's changing the behaviour beyond what I said in the commit message, hence should be a separate patch so that if it causes problems, then it can be reverted independently of this fix.

Quoted 6 lines

drivers/mmc/core/core.c int mmc_of_parse_voltage(struct device_node *np, u32 *mask) · line 1229
Quoted 4 lines
drivers/mmc/core/core.c +3 -0
+       }
+       if (!num_ranges) {
+               pr_err("%s: voltage-ranges empty\n", np->full_name);
                return -EINVAL;
Ditto.

For this one, it's entirely reasonable - it's a mal-formed voltage-ranges property.

Signature · 3 lines

RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Russell King

2015-12-19 20:30 UTC #4 lore raw

PATCH 1/17 mmc: core: shut up "voltage-ranges unspecified" pr_info()