[PATCH v4 00/25] MMC/SDHCI fixes

[PATCH v4 00/25] MMC/SDHCI fixes

[Russell King - ARM Linux]

On Mon, Dec 21, 2015 at 11:39:56AM +0000, Russell King - ARM Linux wrote:
> 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.
> 
> - reworking the voltage-ranges code so that callers know whether the
>   optional voltage-ranges property was specified.
> 
> - "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 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.
> 
> - Fixing another DMA API leak with the pre/post request handling, where
>   a current request can change the state of the SDHCI_REQ_USE_DMA, and
>   thus stop a mapped request being unmapped.
> 
> - Fixing the SDHCI timeout calculation code to correctly round timeouts
>   up rather than down, and correctly calculate the time (in microseconds)
>   for a certain number of card clocks.
> 
> - Consolidating the SDHCI address/size alignment handling so we only walk
>   the scatterlist once instead of twice.
> 
> Further patches are necessary as there are still a number of observable
> problems when using this driver on iMX6 with UHS cards.  Patches up to
> and including "mmc: sdhci: further fix for DMA unmapping in
> sdhci_post_req()" should be considered for inclusion in the stable
> kernel trees.
> 
> I've tested these changes on iMX6 at UHS speeds, and Armada 388 SoCs
> at HS speeds so far.  Others should test them.

One comment from Adrian addressed.

 drivers/mmc/card/block.c       |   4 +-
 drivers/mmc/core/core.c        |  17 +-
 drivers/mmc/host/sdhci-pxav3.c |   6 +-
 drivers/mmc/host/sdhci.c       | 444 ++++++++++++++++++-----------------------
 drivers/mmc/host/sdhci.h       |   4 +-
 5 files changed, 213 insertions(+), 262 deletions(-)

-- 
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.

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

[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.

Silence the warning if the property is missing.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/core/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index f95d41ffc766..8e25dcb42bfe 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1214,8 +1214,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;
 	}
 
-- 
2.1.0

[PATCH v4 02/25] mmc: core: improve mmc_of_parse_voltage() to return better status

[Russell King]

Improve mmc_of_parse_voltage()'s return values so that drivers can tell
whether a voltage-range specification was present, and whether it has
been successfully parsed, or there was an error while parsing.

We return a negative errno when parsing fails, zero if no voltage-range
specification is present, or one if a voltage-range specification is
successfully parsed.

No users need modifying as no users check the return value.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/core/core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 8e25dcb42bfe..c4b95a5d7299 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1204,8 +1204,9 @@ EXPORT_SYMBOL(mmc_vddrange_to_ocrmask);
  * @np: The device node need to be parsed.
  * @mask: mask of voltages available for MMC/SD/SDIO
  *
- * 1. Return zero on success.
- * 2. Return negative errno: voltage-range is invalid.
+ * Parse the "voltage-ranges" DT property, returning zero if it is not
+ * found, negative errno if the voltage-range specification is invalid,
+ * or one if the voltage-range is specified and successfully parsed.
  */
 int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 {
@@ -1216,7 +1217,7 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 	num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
 	if (!voltage_ranges) {
 		pr_debug("%s: voltage-ranges unspecified\n", np->full_name);
-		return -EINVAL;
+		return 0;
 	}
 	if (!num_ranges) {
 		pr_err("%s: voltage-ranges empty\n", np->full_name);
@@ -1238,7 +1239,7 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 		*mask |= ocr_mask;
 	}
 
-	return 0;
+	return 1;
 }
 EXPORT_SYMBOL(mmc_of_parse_voltage);
 
-- 
2.1.0

[PATCH v4 03/25] mmc: block: shut up "retrying because a re-tune was needed" message

[Russell King]

Re-tuning is part of standard requirements for the higher speed SD
card protocols, and is not an error when this occurs.  When we retry
a command due to a retune, we should not print a message to the
kernel log.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/card/block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 5914263090fc..52c6346fc8c2 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1363,8 +1363,8 @@ static int mmc_blk_err_check(struct mmc_card *card,
 
 	if (brq->data.error) {
 		if (need_retune && !brq->retune_retry_done) {
-			pr_info("%s: retrying because a re-tune was needed\n",
-				req->rq_disk->disk_name);
+			pr_debug("%s: retrying because a re-tune was needed\n",
+				 req->rq_disk->disk_name);
 			brq->retune_retry_done = 1;
 			return MMC_BLK_RETRY;
 		}
-- 
2.1.0

[PATCH v4 04/25] mmc: core: report tuning command execution failure reason

[Russell King]

Print the error code when the tuning command fails.  This allows the
reason for the failure to be reported, which aids debugging.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/core/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c4b95a5d7299..cfb5281b6cd7 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1079,7 +1079,7 @@ int mmc_execute_tuning(struct mmc_card *card)
 	err = host->ops->execute_tuning(host, opcode);
 
 	if (err)
-		pr_err("%s: tuning execution failed\n", mmc_hostname(host));
+		pr_err("%s: tuning execution failed: %d\n", mmc_hostname(host), err);
 	else
 		mmc_retune_enable(host);
 
-- 
2.1.0

[PATCH v4 05/25] mmc: sdhci: move initialisation of command error member

[Russell King]

When a command is started, logically it has no error.  Initialise the
command's error member to zero whenever we start a command.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d622435d1bcc..eaa217f6e628 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1003,6 +1003,9 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 
 	WARN_ON(host->cmd);
 
+	/* Initially, a command has no error */
+	cmd->error = 0;
+
 	/* Wait max 10 ms */
 	timeout = 10;
 
@@ -1097,8 +1100,6 @@ static void sdhci_finish_command(struct sdhci_host *host)
 		}
 	}
 
-	host->cmd->error = 0;
-
 	/* Finished CMD23, now send actual command. */
 	if (host->cmd == host->mrq->sbc) {
 		host->cmd = NULL;
-- 
2.1.0

[PATCH v4 06/25] mmc: sdhci: clean up command error handling

[Russell King]

Avoid multiple tests while handling a command error; simplify the code.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index eaa217f6e628..ada227da39a6 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2323,13 +2323,13 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask)
 		return;
 	}
 
-	if (intmask & SDHCI_INT_TIMEOUT)
-		host->cmd->error = -ETIMEDOUT;
-	else if (intmask & (SDHCI_INT_CRC | SDHCI_INT_END_BIT |
-			SDHCI_INT_INDEX))
-		host->cmd->error = -EILSEQ;
+	if (intmask & (SDHCI_INT_TIMEOUT | SDHCI_INT_CRC |
+		       SDHCI_INT_END_BIT | SDHCI_INT_INDEX)) {
+		if (intmask & SDHCI_INT_TIMEOUT)
+			host->cmd->error = -ETIMEDOUT;
+		else
+			host->cmd->error = -EILSEQ;
 
-	if (host->cmd->error) {
 		tasklet_schedule(&host->finish_tasklet);
 		return;
 	}
-- 
2.1.0

[PATCH v4 07/25] mmc: sdhci: command response CRC error handling

[Russell King]

When we get a response CRC error on a command, it means that the
response we received back from the card was not correct.  It does not
mean that the card did not receive the command correctly.  If the
command is one which initiates a data transfer, the card can enter the
data transfer state, and start sending data.

Moreover, if the request contained a data phase, we do not clean this
up, and this results in the driver triggering DMA API debug warnings,
and also creates a race condition in the driver, between running the
finish_tasklet and the data transfer interrupts, which can trigger a
"Got data interrupt" state dump.

Fix this by handing a response CRC error slightly differently: record
the failure of the data initiating command, but allow the remainder of
the request to be processed normally.  This is safe as core MMC checks
the status of all commands and data transfer phases of the request.

If the card does not initiate a data transfer, then we should time out
according to the data transfer parameters.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ada227da39a6..5f22b842cf25 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2330,6 +2330,23 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask)
 		else
 			host->cmd->error = -EILSEQ;
 
+		/*
+		 * If this command initiates a data phase and a response
+		 * CRC error is signalled, the card can start transferring
+		 * data - the card may have received the command without
+		 * error.  We must not terminate the mmc_request early.
+		 *
+		 * If the card did not receive the command or returned an
+		 * error which prevented it sending data, the data phase
+		 * will time out.
+		 */
+		if (host->cmd->data &&
+		    (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
+		     SDHCI_INT_CRC) {
+			host->cmd = NULL;
+			return;
+		}
+
 		tasklet_schedule(&host->finish_tasklet);
 		return;
 	}
-- 
2.1.0

[PATCH v4 08/25] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Russell King]

Unnecessarily mapping and unmapping the align buffer for SD cards is
expensive: performance measurements on iMX6 show that this gives a hit
of 10% on hdparm buffered disk reads.

MMC/SD card IO comes from the mm/vfs which gives us page based IO, so
for this case, the align buffer is not going to be used.  However, we
still map and unmap this buffer.

Eliminate this by switching the align buffer to be a DMA coherent
buffer, which needs no DMA maintenance to access the buffer.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 54 ++++++++++++++++--------------------------------
 1 file changed, 18 insertions(+), 36 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 5f22b842cf25..b0655990defd 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -465,8 +465,6 @@ static void sdhci_adma_mark_end(void *desc)
 static int sdhci_adma_table_pre(struct sdhci_host *host,
 	struct mmc_data *data)
 {
-	int direction;
-
 	void *desc;
 	void *align;
 	dma_addr_t addr;
@@ -483,20 +481,9 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 	 * We currently guess that it is LE.
 	 */
 
-	if (data->flags & MMC_DATA_READ)
-		direction = DMA_FROM_DEVICE;
-	else
-		direction = DMA_TO_DEVICE;
-
-	host->align_addr = dma_map_single(mmc_dev(host->mmc),
-		host->align_buffer, host->align_buffer_sz, direction);
-	if (dma_mapping_error(mmc_dev(host->mmc), host->align_addr))
-		goto fail;
-	BUG_ON(host->align_addr & SDHCI_ADMA2_MASK);
-
 	host->sg_count = sdhci_pre_dma_transfer(host, data);
 	if (host->sg_count < 0)
-		goto unmap_align;
+		return -EINVAL;
 
 	desc = host->adma_table;
 	align = host->align_buffer;
@@ -570,22 +557,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 		/* nop, end, valid */
 		sdhci_adma_write_desc(host, desc, 0, 0, ADMA2_NOP_END_VALID);
 	}
-
-	/*
-	 * Resync align buffer as we might have changed it.
-	 */
-	if (data->flags & MMC_DATA_WRITE) {
-		dma_sync_single_for_device(mmc_dev(host->mmc),
-			host->align_addr, host->align_buffer_sz, direction);
-	}
-
 	return 0;
-
-unmap_align:
-	dma_unmap_single(mmc_dev(host->mmc), host->align_addr,
-		host->align_buffer_sz, direction);
-fail:
-	return -EINVAL;
 }
 
 static void sdhci_adma_table_post(struct sdhci_host *host,
@@ -605,9 +577,6 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
 	else
 		direction = DMA_TO_DEVICE;
 
-	dma_unmap_single(mmc_dev(host->mmc), host->align_addr,
-		host->align_buffer_sz, direction);
-
 	/* Do a quick scan of the SG list for any unaligned mappings */
 	has_unaligned = false;
 	for_each_sg(data->sg, sg, host->sg_count, i)
@@ -2983,14 +2952,21 @@ int sdhci_add_host(struct sdhci_host *host)
 						      &host->adma_addr,
 						      GFP_KERNEL);
 		host->align_buffer_sz = SDHCI_MAX_SEGS * SDHCI_ADMA2_ALIGN;
-		host->align_buffer = kmalloc(host->align_buffer_sz, GFP_KERNEL);
+		host->align_buffer = dma_alloc_coherent(mmc_dev(mmc),
+							host->align_buffer_sz,
+							&host->align_addr,
+							GFP_KERNEL);
 		if (!host->adma_table || !host->align_buffer) {
 			if (host->adma_table)
 				dma_free_coherent(mmc_dev(mmc),
 						  host->adma_table_sz,
 						  host->adma_table,
 						  host->adma_addr);
-			kfree(host->align_buffer);
+			if (host->align_buffer)
+				dma_free_coherent(mmc_dev(mmc),
+						  host->align_buffer_sz,
+						  host->align_buffer,
+						  host->align_addr);
 			pr_warn("%s: Unable to allocate ADMA buffers - falling back to standard DMA\n",
 				mmc_hostname(mmc));
 			host->flags &= ~SDHCI_USE_ADMA;
@@ -3002,10 +2978,14 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->flags &= ~SDHCI_USE_ADMA;
 			dma_free_coherent(mmc_dev(mmc), host->adma_table_sz,
 					  host->adma_table, host->adma_addr);
-			kfree(host->align_buffer);
+			dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz,
+					  host->align_buffer, host->align_addr);
 			host->adma_table = NULL;
 			host->align_buffer = NULL;
 		}
+
+		/* dma_alloc_coherent returns page aligned and sized buffers */
+		BUG_ON(host->align_addr & SDHCI_ADMA2_MASK);
 	}
 
 	/*
@@ -3469,7 +3449,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 	if (host->adma_table)
 		dma_free_coherent(mmc_dev(mmc), host->adma_table_sz,
 				  host->adma_table, host->adma_addr);
-	kfree(host->align_buffer);
+	if (host->align_buffer)
+		dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz,
+				  host->align_buffer, host->align_addr);
 
 	host->adma_table = NULL;
 	host->align_buffer = NULL;
-- 
2.1.0

[PATCH v4 09/25] mmc: sdhci: allocate alignment and DMA descriptor buffer together

[Russell King]

Allocate both the alignment and DMA descriptor buffers together.  The
size of the alignment buffer will always be aligned to the hosts
required alignment, which gives appropriate alignment to the DMA
descriptors.

We have a maximum of 128 segments, and a maximum alignment of 64 bits.
This gives a maximum alignment buffer size of 1024 bytes.

The DMA descriptors are a maximum of 12 bytes, and we allocate 128 * 2
+ 1 of these, which gives a maximum DMA descriptor buffer size of 3084
bytes.

This means the allocation for a 4K page sized system will be an order-1
allocation, since the resulting overall size is 4108.  This is more
prone to failure than page-sized allocations, but since this allocation
commonly occurs at startup, the chances of failure are small.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 56 +++++++++++++++++-------------------------------
 1 file changed, 20 insertions(+), 36 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b0655990defd..c63d35c0f84b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2932,6 +2932,9 @@ int sdhci_add_host(struct sdhci_host *host)
 		host->flags &= ~SDHCI_USE_SDMA;
 
 	if (host->flags & SDHCI_USE_ADMA) {
+		dma_addr_t dma;
+		void *buf;
+
 		/*
 		 * The DMA descriptor table size is calculated as the maximum
 		 * number of segments times 2, to allow for an alignment
@@ -2947,45 +2950,28 @@ int sdhci_add_host(struct sdhci_host *host)
 					      SDHCI_ADMA2_32_DESC_SZ;
 			host->desc_sz = SDHCI_ADMA2_32_DESC_SZ;
 		}
-		host->adma_table = dma_alloc_coherent(mmc_dev(mmc),
-						      host->adma_table_sz,
-						      &host->adma_addr,
-						      GFP_KERNEL);
+
 		host->align_buffer_sz = SDHCI_MAX_SEGS * SDHCI_ADMA2_ALIGN;
-		host->align_buffer = dma_alloc_coherent(mmc_dev(mmc),
-							host->align_buffer_sz,
-							&host->align_addr,
-							GFP_KERNEL);
-		if (!host->adma_table || !host->align_buffer) {
-			if (host->adma_table)
-				dma_free_coherent(mmc_dev(mmc),
-						  host->adma_table_sz,
-						  host->adma_table,
-						  host->adma_addr);
-			if (host->align_buffer)
-				dma_free_coherent(mmc_dev(mmc),
-						  host->align_buffer_sz,
-						  host->align_buffer,
-						  host->align_addr);
+		buf = dma_alloc_coherent(mmc_dev(mmc), host->align_buffer_sz +
+					 host->adma_table_sz, &dma, GFP_KERNEL);
+		if (!buf) {
 			pr_warn("%s: Unable to allocate ADMA buffers - falling back to standard DMA\n",
 				mmc_hostname(mmc));
 			host->flags &= ~SDHCI_USE_ADMA;
-			host->adma_table = NULL;
-			host->align_buffer = NULL;
-		} else if (host->adma_addr & (SDHCI_ADMA2_DESC_ALIGN - 1)) {
+		} else if ((dma + host->align_buffer_sz) &
+			   (SDHCI_ADMA2_DESC_ALIGN - 1)) {
 			pr_warn("%s: unable to allocate aligned ADMA descriptor\n",
 				mmc_hostname(mmc));
 			host->flags &= ~SDHCI_USE_ADMA;
-			dma_free_coherent(mmc_dev(mmc), host->adma_table_sz,
-					  host->adma_table, host->adma_addr);
-			dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz,
-					  host->align_buffer, host->align_addr);
-			host->adma_table = NULL;
-			host->align_buffer = NULL;
-		}
+			dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
+					  host->adma_table_sz, buf, dma);
+		} else {
+			host->align_buffer = buf;
+			host->align_addr = dma;
 
-		/* dma_alloc_coherent returns page aligned and sized buffers */
-		BUG_ON(host->align_addr & SDHCI_ADMA2_MASK);
+			host->adma_table = buf + host->align_buffer_sz;
+			host->adma_addr = dma + host->align_buffer_sz;
+		}
 	}
 
 	/*
@@ -3446,12 +3432,10 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 	if (!IS_ERR(mmc->supply.vqmmc))
 		regulator_disable(mmc->supply.vqmmc);
 
-	if (host->adma_table)
-		dma_free_coherent(mmc_dev(mmc), host->adma_table_sz,
-				  host->adma_table, host->adma_addr);
 	if (host->align_buffer)
-		dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz,
-				  host->align_buffer, host->align_addr);
+		dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
+				  host->adma_table_sz, host->align_buffer,
+				  host->align_addr);
 
 	host->adma_table = NULL;
 	host->align_buffer = NULL;
-- 
2.1.0

[PATCH v4 10/25] mmc: sdhci: clean up coding style in sdhci_adma_table_pre()

[Russell King]

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c63d35c0f84b..774b0cf05f9d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -465,16 +465,12 @@ static void sdhci_adma_mark_end(void *desc)
 static int sdhci_adma_table_pre(struct sdhci_host *host,
 	struct mmc_data *data)
 {
-	void *desc;
-	void *align;
-	dma_addr_t addr;
-	dma_addr_t align_addr;
-	int len, offset;
-
 	struct scatterlist *sg;
-	int i;
-	char *buffer;
 	unsigned long flags;
+	dma_addr_t addr, align_addr;
+	void *desc, *align;
+	char *buffer;
+	int len, offset, i;
 
 	/*
 	 * The spec does not specify endianness of descriptor table.
@@ -495,10 +491,9 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 		len = sg_dma_len(sg);
 
 		/*
-		 * The SDHCI specification states that ADMA
-		 * addresses must be 32-bit aligned. If they
-		 * aren't, then we use a bounce buffer for
-		 * the (up to three) bytes that screw up the
+		 * The SDHCI specification states that ADMA addresses must
+		 * be 32-bit aligned. If they aren't, then we use a bounce
+		 * buffer for the (up to three) bytes that screw up the
 		 * alignment.
 		 */
 		offset = (SDHCI_ADMA2_ALIGN - (addr & SDHCI_ADMA2_MASK)) &
@@ -542,19 +537,13 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 	}
 
 	if (host->quirks & SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC) {
-		/*
-		* Mark the last descriptor as the terminating descriptor
-		*/
+		/* Mark the last descriptor as the terminating descriptor */
 		if (desc != host->adma_table) {
 			desc -= host->desc_sz;
 			sdhci_adma_mark_end(desc);
 		}
 	} else {
-		/*
-		* Add a terminating entry.
-		*/
-
-		/* nop, end, valid */
+		/* Add a terminating entry - nop, end, valid */
 		sdhci_adma_write_desc(host, desc, 0, 0, ADMA2_NOP_END_VALID);
 	}
 	return 0;
-- 
2.1.0

[PATCH v4 11/25] mmc: sdhci: avoid walking SG list for writes

[Russell King]

If we are writing data to the card, there is no point in walking the
scatterlist to find out if there are any unaligned entries; this is a
needless waste of CPU cycles.  Avoid this by checking for a non-read
tranfer first.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 774b0cf05f9d..9b56bb5546bf 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -559,37 +559,39 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
 	void *align;
 	char *buffer;
 	unsigned long flags;
-	bool has_unaligned;
 
 	if (data->flags & MMC_DATA_READ)
 		direction = DMA_FROM_DEVICE;
 	else
 		direction = DMA_TO_DEVICE;
 
-	/* Do a quick scan of the SG list for any unaligned mappings */
-	has_unaligned = false;
-	for_each_sg(data->sg, sg, host->sg_count, i)
-		if (sg_dma_address(sg) & SDHCI_ADMA2_MASK) {
-			has_unaligned = true;
-			break;
-		}
+	if (data->flags & MMC_DATA_READ) {
+		bool has_unaligned = false;
 
-	if (has_unaligned && data->flags & MMC_DATA_READ) {
-		dma_sync_sg_for_cpu(mmc_dev(host->mmc), data->sg,
-			data->sg_len, direction);
+		/* Do a quick scan of the SG list for any unaligned mappings */
+		for_each_sg(data->sg, sg, host->sg_count, i)
+			if (sg_dma_address(sg) & SDHCI_ADMA2_MASK) {
+				has_unaligned = true;
+				break;
+			}
 
-		align = host->align_buffer;
+		if (has_unaligned) {
+			dma_sync_sg_for_cpu(mmc_dev(host->mmc), data->sg,
+				data->sg_len, direction);
 
-		for_each_sg(data->sg, sg, host->sg_count, i) {
-			if (sg_dma_address(sg) & SDHCI_ADMA2_MASK) {
-				size = SDHCI_ADMA2_ALIGN -
-				       (sg_dma_address(sg) & SDHCI_ADMA2_MASK);
+			align = host->align_buffer;
 
-				buffer = sdhci_kmap_atomic(sg, &flags);
-				memcpy(buffer, align, size);
-				sdhci_kunmap_atomic(buffer, &flags);
+			for_each_sg(data->sg, sg, host->sg_count, i) {
+				if (sg_dma_address(sg) & SDHCI_ADMA2_MASK) {
+					size = SDHCI_ADMA2_ALIGN -
+					       (sg_dma_address(sg) & SDHCI_ADMA2_MASK);
 
-				align += SDHCI_ADMA2_ALIGN;
+					buffer = sdhci_kmap_atomic(sg, &flags);
+					memcpy(buffer, align, size);
+					sdhci_kunmap_atomic(buffer, &flags);
+
+					align += SDHCI_ADMA2_ALIGN;
+				}
 			}
 		}
 	}
-- 
2.1.0

[PATCH v4 12/25] mmc: sdhci: factor out common DMA cleanup in sdhci_finish_data()

[Russell King]

sdhci_finish_data() has two paths which result in identical DMA cleanup.
One is when SDHCI_USE_ADMA is clear, and the other is just before when
SDHCI_USE_ADMA is set, and is performed within sdhci_adma_table_post().

Simplify the code by removing the 'else' and eliminating the duplicate
inside sdhci_adma_table_post().

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9b56bb5546bf..49692b7b4016 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -552,19 +552,12 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 static void sdhci_adma_table_post(struct sdhci_host *host,
 	struct mmc_data *data)
 {
-	int direction;
-
 	struct scatterlist *sg;
 	int i, size;
 	void *align;
 	char *buffer;
 	unsigned long flags;
 
-	if (data->flags & MMC_DATA_READ)
-		direction = DMA_FROM_DEVICE;
-	else
-		direction = DMA_TO_DEVICE;
-
 	if (data->flags & MMC_DATA_READ) {
 		bool has_unaligned = false;
 
@@ -577,7 +570,7 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
 
 		if (has_unaligned) {
 			dma_sync_sg_for_cpu(mmc_dev(host->mmc), data->sg,
-				data->sg_len, direction);
+					    data->sg_len, DMA_FROM_DEVICE);
 
 			align = host->align_buffer;
 
@@ -595,12 +588,6 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
 			}
 		}
 	}
-
-	if (data->host_cookie == COOKIE_MAPPED) {
-		dma_unmap_sg(mmc_dev(host->mmc), data->sg,
-			data->sg_len, direction);
-		data->host_cookie = COOKIE_UNMAPPED;
-	}
 }
 
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
@@ -909,14 +896,12 @@ static void sdhci_finish_data(struct sdhci_host *host)
 	if (host->flags & SDHCI_REQ_USE_DMA) {
 		if (host->flags & SDHCI_USE_ADMA)
 			sdhci_adma_table_post(host, data);
-		else {
-			if (data->host_cookie == COOKIE_MAPPED) {
-				dma_unmap_sg(mmc_dev(host->mmc),
-					data->sg, data->sg_len,
-					(data->flags & MMC_DATA_READ) ?
-					DMA_FROM_DEVICE : DMA_TO_DEVICE);
-				data->host_cookie = COOKIE_UNMAPPED;
-			}
+
+		if (data->host_cookie == COOKIE_MAPPED) {
+			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+				     (data->flags & MMC_DATA_READ) ?
+				     DMA_FROM_DEVICE : DMA_TO_DEVICE);
+			data->host_cookie = COOKIE_UNMAPPED;
 		}
 	}
 
-- 
2.1.0

[PATCH v4 13/25] mmc: sdhci: move sdhci_pre_dma_transfer()

[Russell King]

Move sdhci_pre_dma_transfer() to avoid needing to declare this function
before use.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 52 +++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 49692b7b4016..260b1caeced8 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -53,8 +53,6 @@ static void sdhci_finish_data(struct sdhci_host *);
 static void sdhci_finish_command(struct sdhci_host *);
 static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
 static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
-static int sdhci_pre_dma_transfer(struct sdhci_host *host,
-					struct mmc_data *data);
 static int sdhci_do_get_cd(struct sdhci_host *host);
 
 #ifdef CONFIG_PM
@@ -428,6 +426,31 @@ static void sdhci_transfer_pio(struct sdhci_host *host)
 	DBG("PIO transfer complete.\n");
 }
 
+static int sdhci_pre_dma_transfer(struct sdhci_host *host,
+				  struct mmc_data *data)
+{
+	int sg_count;
+
+	if (data->host_cookie == COOKIE_MAPPED) {
+		data->host_cookie = COOKIE_GIVEN;
+		return data->sg_count;
+	}
+
+	WARN_ON(data->host_cookie == COOKIE_GIVEN);
+
+	sg_count = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+				data->flags & MMC_DATA_WRITE ?
+				DMA_TO_DEVICE : DMA_FROM_DEVICE);
+
+	if (sg_count == 0)
+		return -ENOSPC;
+
+	data->sg_count = sg_count;
+	data->host_cookie = COOKIE_MAPPED;
+
+	return sg_count;
+}
+
 static char *sdhci_kmap_atomic(struct scatterlist *sg, unsigned long *flags)
 {
 	local_irq_save(*flags);
@@ -2070,31 +2093,6 @@ static void sdhci_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
 	}
 }
 
-static int sdhci_pre_dma_transfer(struct sdhci_host *host,
-				       struct mmc_data *data)
-{
-	int sg_count;
-
-	if (data->host_cookie == COOKIE_MAPPED) {
-		data->host_cookie = COOKIE_GIVEN;
-		return data->sg_count;
-	}
-
-	WARN_ON(data->host_cookie == COOKIE_GIVEN);
-
-	sg_count = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
-				data->flags & MMC_DATA_WRITE ?
-				DMA_TO_DEVICE : DMA_FROM_DEVICE);
-
-	if (sg_count == 0)
-		return -ENOSPC;
-
-	data->sg_count = sg_count;
-	data->host_cookie = COOKIE_MAPPED;
-
-	return sg_count;
-}
-
 static void sdhci_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
 			       bool is_first_req)
 {
-- 
2.1.0

[PATCH v4 14/25] mmc: sdhci: factor out sdhci_pre_dma_transfer() from sdhci_adma_table_pre()

[Russell King]

In sdhci_prepare_data(), when SDHCI_REQ_USE_DMA is set, there are two
paths that prepare the data buffers for transfer.  One is when
SDHCI_USE_ADMA is set, and is located inside sdhci_adma_table_pre().
The other is when SDHCI_USE_ADMA is clear, in the else clause of the
above.

Factor out the call to sdhci_pre_dma_transfer() along with its error
checking.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 62 ++++++++++++++++++------------------------------
 1 file changed, 23 insertions(+), 39 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 260b1caeced8..c07d4296ee19 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -485,8 +485,8 @@ static void sdhci_adma_mark_end(void *desc)
 	dma_desc->cmd |= cpu_to_le16(ADMA2_END);
 }
 
-static int sdhci_adma_table_pre(struct sdhci_host *host,
-	struct mmc_data *data)
+static void sdhci_adma_table_pre(struct sdhci_host *host,
+	struct mmc_data *data, int sg_count)
 {
 	struct scatterlist *sg;
 	unsigned long flags;
@@ -500,9 +500,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 	 * We currently guess that it is LE.
 	 */
 
-	host->sg_count = sdhci_pre_dma_transfer(host, data);
-	if (host->sg_count < 0)
-		return -EINVAL;
+	host->sg_count = sg_count;
 
 	desc = host->adma_table;
 	align = host->align_buffer;
@@ -569,7 +567,6 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 		/* Add a terminating entry - nop, end, valid */
 		sdhci_adma_write_desc(host, desc, 0, 0, ADMA2_NOP_END_VALID);
 	}
-	return 0;
 }
 
 static void sdhci_adma_table_post(struct sdhci_host *host,
@@ -699,7 +696,6 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 ctrl;
 	struct mmc_data *data = cmd->data;
-	int ret;
 
 	WARN_ON(host->data);
 
@@ -784,39 +780,27 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 
 	if (host->flags & SDHCI_REQ_USE_DMA) {
-		if (host->flags & SDHCI_USE_ADMA) {
-			ret = sdhci_adma_table_pre(host, data);
-			if (ret) {
-				/*
-				 * This only happens when someone fed
-				 * us an invalid request.
-				 */
-				WARN_ON(1);
-				host->flags &= ~SDHCI_REQ_USE_DMA;
-			} else {
-				sdhci_writel(host, host->adma_addr,
-					SDHCI_ADMA_ADDRESS);
-				if (host->flags & SDHCI_USE_64_BIT_DMA)
-					sdhci_writel(host,
-						     (u64)host->adma_addr >> 32,
-						     SDHCI_ADMA_ADDRESS_HI);
-			}
-		} else {
-			int sg_cnt;
+		int sg_cnt = sdhci_pre_dma_transfer(host, data);
 
-			sg_cnt = sdhci_pre_dma_transfer(host, data);
-			if (sg_cnt <= 0) {
-				/*
-				 * This only happens when someone fed
-				 * us an invalid request.
-				 */
-				WARN_ON(1);
-				host->flags &= ~SDHCI_REQ_USE_DMA;
-			} else {
-				WARN_ON(sg_cnt != 1);
-				sdhci_writel(host, sg_dma_address(data->sg),
-					SDHCI_DMA_ADDRESS);
-			}
+		if (sg_cnt <= 0) {
+			/*
+			 * This only happens when someone fed
+			 * us an invalid request.
+			 */
+			WARN_ON(1);
+			host->flags &= ~SDHCI_REQ_USE_DMA;
+		} else if (host->flags & SDHCI_USE_ADMA) {
+			sdhci_adma_table_pre(host, data, sg_cnt);
+
+			sdhci_writel(host, host->adma_addr, SDHCI_ADMA_ADDRESS);
+			if (host->flags & SDHCI_USE_64_BIT_DMA)
+				sdhci_writel(host,
+					     (u64)host->adma_addr >> 32,
+					     SDHCI_ADMA_ADDRESS_HI);
+		} else {
+			WARN_ON(sg_cnt != 1);
+			sdhci_writel(host, sg_dma_address(data->sg),
+				SDHCI_DMA_ADDRESS);
 		}
 	}
 
-- 
2.1.0

[PATCH v4 15/25] mmc: sdhci: pass the cookie into sdhci_pre_dma_transfer()

[Russell King]

Pass the desired cookie for a successful map.  This is in preparation to
clean up the MAPPED/GIVEN states.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c07d4296ee19..bd7fffa5b317 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -427,7 +427,7 @@ static void sdhci_transfer_pio(struct sdhci_host *host)
 }
 
 static int sdhci_pre_dma_transfer(struct sdhci_host *host,
-				  struct mmc_data *data)
+				  struct mmc_data *data, int cookie)
 {
 	int sg_count;
 
@@ -446,7 +446,7 @@ static int sdhci_pre_dma_transfer(struct sdhci_host *host,
 		return -ENOSPC;
 
 	data->sg_count = sg_count;
-	data->host_cookie = COOKIE_MAPPED;
+	data->host_cookie = cookie;
 
 	return sg_count;
 }
@@ -780,7 +780,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 
 	if (host->flags & SDHCI_REQ_USE_DMA) {
-		int sg_cnt = sdhci_pre_dma_transfer(host, data);
+		int sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
 
 		if (sg_cnt <= 0) {
 			/*
@@ -2085,7 +2085,7 @@ static void sdhci_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
 	mrq->data->host_cookie = COOKIE_UNMAPPED;
 
 	if (host->flags & SDHCI_REQ_USE_DMA)
-		sdhci_pre_dma_transfer(host, mrq->data);
+		sdhci_pre_dma_transfer(host, mrq->data, COOKIE_MAPPED);
 }
 
 static void sdhci_card_event(struct mmc_host *mmc)
-- 
2.1.0

[PATCH v4 16/25] mmc: sdhci: always unmap a mapped data transfer in sdhci_post_req()

[Russell King]

If the host cookie indicates that the data buffers of a request are
mapped at sdhci_post_req() time, always unmap the data buffers.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index bd7fffa5b317..3da2a0461e4b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2068,8 +2068,7 @@ static void sdhci_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
 	struct mmc_data *data = mrq->data;
 
 	if (host->flags & SDHCI_REQ_USE_DMA) {
-		if (data->host_cookie == COOKIE_GIVEN ||
-				data->host_cookie == COOKIE_MAPPED)
+		if (data->host_cookie != COOKIE_UNMAPPED)
 			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
 					 data->flags & MMC_DATA_WRITE ?
 					 DMA_TO_DEVICE : DMA_FROM_DEVICE);
-- 
2.1.0

[PATCH v4 17/25] mmc: sdhci: clean up host cookie handling

[Russell King]

Commit d31911b9374a ("mmc: sdhci: fix dma memory leak in sdhci_pre_req()")
added a complicated method to manage the DMA map state for the data
transfer, but this complexity is not required.

There are three states:
* Unmapped
* Mapped by sdhci_pre_req()
* Mapped by sdhci_prepare_data()

sdhci_prepare_data() needs to know when the data buffers have been
successfully mapped by sdhci_pre_req(), and if so, there is no need to
map them a second time.

When we come to tear down the mapping, we want to know whether
sdhci_post_req() will be called (which is determined by sdhci_pre_req()
having been previously called) so that we can postpone the unmap
operation.

Hence, it makes sense to simply record when the successful DMA map
happened (via COOKIE_PRE_MAPPED vs COOKIE_MAPPED) rather than having
the complex mechanics involving COOKIE_MAPPED vs COOKIE_GIVEN.

If a mapping is created by sdhci_prepare_data(), we must tear it down
ourselves, without waiting for sdhci_post_req() (hence, the new
COOKIE_MAPPED case).  If the mapping is created by sdhci_pre_req()
then sdhci_post_req() is responsible for tearing the mapping down.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 12 ++++++------
 drivers/mmc/host/sdhci.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 3da2a0461e4b..a714c628394b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -431,12 +431,12 @@ static int sdhci_pre_dma_transfer(struct sdhci_host *host,
 {
 	int sg_count;
 
-	if (data->host_cookie == COOKIE_MAPPED) {
-		data->host_cookie = COOKIE_GIVEN;
+	/*
+	 * If the data buffers are already mapped, return the previous
+	 * dma_map_sg() result.
+	 */
+	if (data->host_cookie == COOKIE_PRE_MAPPED)
 		return data->sg_count;
-	}
-
-	WARN_ON(data->host_cookie == COOKIE_GIVEN);
 
 	sg_count = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
 				data->flags & MMC_DATA_WRITE ?
@@ -2084,7 +2084,7 @@ static void sdhci_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
 	mrq->data->host_cookie = COOKIE_UNMAPPED;
 
 	if (host->flags & SDHCI_REQ_USE_DMA)
-		sdhci_pre_dma_transfer(host, mrq->data, COOKIE_MAPPED);
+		sdhci_pre_dma_transfer(host, mrq->data, COOKIE_PRE_MAPPED);
 }
 
 static void sdhci_card_event(struct mmc_host *mmc)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 7654ae5d2b4e..19e9291e5074 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -316,8 +316,8 @@ struct sdhci_adma2_64_desc {
 
 enum sdhci_cookie {
 	COOKIE_UNMAPPED,
-	COOKIE_MAPPED,
-	COOKIE_GIVEN,
+	COOKIE_PRE_MAPPED,	/* mapped by sdhci_pre_req() */
+	COOKIE_MAPPED,		/* mapped by sdhci_prepare_data() */
 };
 
 struct sdhci_host {
-- 
2.1.0

[PATCH v4 18/25] mmc: sdhci: plug DMA mapping leak on error

[Russell King]

If we terminate a command early, we fail to properly clean up the DMA
mappings for the data part of the request.  Move this clean up to the
tasklet, which is the common path for finishing a request so we always
clean up after ourselves.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a714c628394b..b4808ca8e497 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -900,17 +900,9 @@ static void sdhci_finish_data(struct sdhci_host *host)
 	data = host->data;
 	host->data = NULL;
 
-	if (host->flags & SDHCI_REQ_USE_DMA) {
-		if (host->flags & SDHCI_USE_ADMA)
-			sdhci_adma_table_post(host, data);
-
-		if (data->host_cookie == COOKIE_MAPPED) {
-			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
-				     (data->flags & MMC_DATA_READ) ?
-				     DMA_FROM_DEVICE : DMA_TO_DEVICE);
-			data->host_cookie = COOKIE_UNMAPPED;
-		}
-	}
+	if ((host->flags & (SDHCI_REQ_USE_DMA | SDHCI_USE_ADMA)) ==
+	    (SDHCI_REQ_USE_DMA | SDHCI_USE_ADMA))
+		sdhci_adma_table_post(host, data);
 
 	/*
 	 * The specification states that the block count register must
@@ -2165,6 +2157,22 @@ static void sdhci_tasklet_finish(unsigned long param)
 	mrq = host->mrq;
 
 	/*
+	 * Always unmap the data buffers if they were mapped by
+	 * sdhci_prepare_data() whenever we finish with a request.
+	 * This avoids leaking DMA mappings on error.
+	 */
+	if (host->flags & SDHCI_REQ_USE_DMA) {
+		struct mmc_data *data = mrq->data;
+
+		if (data && data->host_cookie == COOKIE_MAPPED) {
+			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+				     (data->flags & MMC_DATA_READ) ?
+				     DMA_FROM_DEVICE : DMA_TO_DEVICE);
+			data->host_cookie = COOKIE_UNMAPPED;
+		}
+	}
+
+	/*
 	 * The controller needs a reset of internal state machines
 	 * upon error conditions.
 	 */
-- 
2.1.0

[PATCH v4 19/25] mmc: sdhci-pxav3: fix higher speed mode capabilities

[Russell King]

Commit 1140011ee9d9 ("mmc: sdhci-pxav3: Modify clock settings for the
SDR50 and DDR50 modes") broke any chance of the SDR50 or DDR50 modes
being used.

The commit claims that SDR50 and DDR50 require clock adjustments in
the SDIO3 Configuration register, which is located via the "conf-sdio3"
resource.  However, when this resource is given, we fail to read the
host capabilities 1 register, resulting in host->caps1 being zero.
Hence, both SDHCI_SUPPORT_SDR50 and SDHCI_SUPPORT_DDR50 bits remain
zero, disabling the SDR50 and DDR50 modes.

The underlying idea in this function appears to be to read the device
capabilities, modify them, and set SDHCI_QUIRK_MISSING_CAPS to cause
our modified capabilities to be used.  Implement exactly that.

Fixes: 1140011ee9d9 ("mmc: sdhci-pxav3: Modify clock settings for the SDR50 and DDR50 modes")
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci-pxav3.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index f5edf9d3a18a..c7f27fe4805a 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -137,6 +137,10 @@ static int armada_38x_quirks(struct platform_device *pdev,
 
 	host->quirks &= ~SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN;
 	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
+
+	host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
+
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 					   "conf-sdio3");
 	if (res) {
@@ -150,7 +154,6 @@ static int armada_38x_quirks(struct platform_device *pdev,
 		 * Configuration register, if the adjustment is not done,
 		 * remove them from the capabilities.
 		 */
-		host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
 		host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50);
 
 		dev_warn(&pdev->dev, "conf-sdio3 register not found: disabling SDR50 and DDR50 modes.\nConsider updating your dtb\n");
@@ -161,7 +164,6 @@ static int armada_38x_quirks(struct platform_device *pdev,
 	 * controller has different capabilities than the ones shown
 	 * in its registers
 	 */
-	host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
 	if (of_property_read_bool(np, "no-1-8-v")) {
 		host->caps &= ~SDHCI_CAN_VDD_180;
 		host->mmc->caps &= ~MMC_CAP_1_8V_DDR;
-- 
2.1.0

[PATCH v4 20/25] mmc: sdhci: further fix for DMA unmapping in sdhci_post_req()

[Russell King]

sdhci_post_req() exists to unmap a previously mapped but already
finished request, while the next request is in progress.  However, the
state of the SDHCI_REQ_USE_DMA flag depends on the last submitted
request.

This means we can end up clearing the flag due to a quirk, which then
means that sdhci_post_req() fails to unmap the DMA buffer, potentially
leading to data corruption.

We can safely ignore the SDHCI_REQ_USE_DMA here, as testing
data->host_cookie is entirely sufficient.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b4808ca8e497..7fed845797a0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2059,13 +2059,12 @@ static void sdhci_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
 	struct sdhci_host *host = mmc_priv(mmc);
 	struct mmc_data *data = mrq->data;
 
-	if (host->flags & SDHCI_REQ_USE_DMA) {
-		if (data->host_cookie != COOKIE_UNMAPPED)
-			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
-					 data->flags & MMC_DATA_WRITE ?
-					 DMA_TO_DEVICE : DMA_FROM_DEVICE);
-		data->host_cookie = COOKIE_UNMAPPED;
-	}
+	if (data->host_cookie != COOKIE_UNMAPPED)
+		dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+			     data->flags & MMC_DATA_WRITE ?
+			       DMA_TO_DEVICE : DMA_FROM_DEVICE);
+
+	data->host_cookie = COOKIE_UNMAPPED;
 }
 
 static void sdhci_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
-- 
2.1.0

[PATCH v4 21/25] mmc: sdhci: fix data timeout (part 1)

[Russell King]

The data timeout gives the minimum amount of time that should be
waited before timing out if no data is received from the card.
Simply dividing the nanosecond part by 1000 does not give this
required guarantee, since such a division rounds down.  Use
DIV_ROUND_UP() to give the desired timeout.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 7fed845797a0..48bc0c6b74f7 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -633,7 +633,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 	if (!data)
 		target_timeout = cmd->busy_timeout * 1000;
 	else {
-		target_timeout = data->timeout_ns / 1000;
+		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
 		if (host->clock)
 			target_timeout += data->timeout_clks / host->clock;
 	}
-- 
2.1.0

[PATCH v4 22/25] mmc: sdhci: fix data timeout (part 2)

[Russell King]

The calculation for the timeout based on the number of card clocks is
incorrect.  The calculation assumed:

	timeout in microseconds = clock cycles / clock in Hz

which is clearly a several orders of magnitude wrong.  Fix this by
multiplying the clock cycles by 1000000 prior to dividing by the Hz
based clock.  Also, as per part 1, ensure that the division rounds
up.

As this needs 64-bit math via do_div(), avoid it if the clock cycles
is zero.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 48bc0c6b74f7..60ec6b5f5600 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -634,8 +634,19 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		target_timeout = cmd->busy_timeout * 1000;
 	else {
 		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
-		if (host->clock)
-			target_timeout += data->timeout_clks / host->clock;
+		if (host->clock && data->timeout_clks) {
+			unsigned long long val;
+
+			/*
+			 * data->timeout_clks is in units of clock cycles.
+			 * host->clock is in Hz.  target_timeout is in us.
+			 * Hence, us = 1000000 * cycles / Hz.  Round up.
+			 */
+			val = 1000000 * data->timeout_clks;
+			if (do_div(val, host->clock))
+				target_timeout++;
+			target_timeout += val;
+		}
 	}
 
 	/*
-- 
2.1.0

[PATCH v4 23/25] mmc: sdhci: prepare DMA address/size quirk handling consolidation

[Russell King]

Prepare to consolidate the DMA address/size quirk handling into one
single loop.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 60ec6b5f5600..a033155984a4 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -733,23 +733,24 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	 * scatterlist.
 	 */
 	if (host->flags & SDHCI_REQ_USE_DMA) {
-		int broken, i;
 		struct scatterlist *sg;
+		unsigned int length_mask;
+		int i;
 
-		broken = 0;
+		length_mask = 0;
 		if (host->flags & SDHCI_USE_ADMA) {
 			if (host->quirks & SDHCI_QUIRK_32BIT_ADMA_SIZE)
-				broken = 1;
+				length_mask = 3;
 		} else {
 			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_SIZE)
-				broken = 1;
+				length_mask = 3;
 		}
 
-		if (unlikely(broken)) {
+		if (unlikely(length_mask)) {
 			for_each_sg(data->sg, sg, data->sg_len, i) {
-				if (sg->length & 0x3) {
+				if (sg->length & length_mask) {
 					DBG("Reverting to PIO because of transfer size (%d)\n",
-						sg->length);
+					    sg->length);
 					host->flags &= ~SDHCI_REQ_USE_DMA;
 					break;
 				}
@@ -762,10 +763,11 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	 * translation to device address space.
 	 */
 	if (host->flags & SDHCI_REQ_USE_DMA) {
-		int broken, i;
 		struct scatterlist *sg;
+		unsigned int offset_mask;
+		int i;
 
-		broken = 0;
+		offset_mask = 0;
 		if (host->flags & SDHCI_USE_ADMA) {
 			/*
 			 * As we use 3 byte chunks to work around
@@ -773,15 +775,15 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 			 * quirk.
 			 */
 			if (host->quirks & SDHCI_QUIRK_32BIT_ADMA_SIZE)
-				broken = 1;
+				offset_mask = 3;
 		} else {
 			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR)
-				broken = 1;
+				offset_mask = 3;
 		}
 
-		if (unlikely(broken)) {
+		if (unlikely(offset_mask)) {
 			for_each_sg(data->sg, sg, data->sg_len, i) {
-				if (sg->offset & 0x3) {
+				if (sg->offset & offset_mask) {
 					DBG("Reverting to PIO because of bad alignment\n");
 					host->flags &= ~SDHCI_REQ_USE_DMA;
 					break;
-- 
2.1.0

[PATCH v4 24/25] mmc: sdhci: consolidate the DMA/ADMA size/address quicks

[Russell King]

Rather than scanning the scatterlist multiple times for each quirk,
scan it once, checking for each possible quirk.  This should be
cheaper due to the length and offset members commonly sharing the
same cache line than scanning the scatterlist multiple times.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 48 ++++++++++++++++--------------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a033155984a4..cd5e62f273b3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -731,22 +731,35 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	/*
 	 * FIXME: This doesn't account for merging when mapping the
 	 * scatterlist.
+	 *
+	 * The assumption here being that alignment and lengths are
+	 * the same after DMA mapping to device address space.
 	 */
 	if (host->flags & SDHCI_REQ_USE_DMA) {
 		struct scatterlist *sg;
-		unsigned int length_mask;
+		unsigned int length_mask, offset_mask;
 		int i;
 
 		length_mask = 0;
+		offset_mask = 0;
 		if (host->flags & SDHCI_USE_ADMA) {
-			if (host->quirks & SDHCI_QUIRK_32BIT_ADMA_SIZE)
+			if (host->quirks & SDHCI_QUIRK_32BIT_ADMA_SIZE) {
 				length_mask = 3;
+				/*
+				 * As we use up to 3 byte chunks to work
+				 * around alignment problems, we need to
+				 * check the offset as well.
+				 */
+				offset_mask = 3;
+			}
 		} else {
 			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_SIZE)
 				length_mask = 3;
+			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR)
+				offset_mask = 3;
 		}
 
-		if (unlikely(length_mask)) {
+		if (unlikely(length_mask | offset_mask)) {
 			for_each_sg(data->sg, sg, data->sg_len, i) {
 				if (sg->length & length_mask) {
 					DBG("Reverting to PIO because of transfer size (%d)\n",
@@ -754,35 +767,6 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 					host->flags &= ~SDHCI_REQ_USE_DMA;
 					break;
 				}
-			}
-		}
-	}
-
-	/*
-	 * The assumption here being that alignment is the same after
-	 * translation to device address space.
-	 */
-	if (host->flags & SDHCI_REQ_USE_DMA) {
-		struct scatterlist *sg;
-		unsigned int offset_mask;
-		int i;
-
-		offset_mask = 0;
-		if (host->flags & SDHCI_USE_ADMA) {
-			/*
-			 * As we use 3 byte chunks to work around
-			 * alignment problems, we need to check this
-			 * quirk.
-			 */
-			if (host->quirks & SDHCI_QUIRK_32BIT_ADMA_SIZE)
-				offset_mask = 3;
-		} else {
-			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR)
-				offset_mask = 3;
-		}
-
-		if (unlikely(offset_mask)) {
-			for_each_sg(data->sg, sg, data->sg_len, i) {
 				if (sg->offset & offset_mask) {
 					DBG("Reverting to PIO because of bad alignment\n");
 					host->flags &= ~SDHCI_REQ_USE_DMA;
-- 
2.1.0

[PATCH v4 25/25] mmc: sdhci: further code simplication

[Russell King]

Further simplify the code in sdhci_prepare_data() - we don't set
SDHCI_REQ_USE_DMA anywhere else in the driver, so there is no
need to set it, and then immediately test it.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index cd5e62f273b3..0e706eefd7d5 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -725,21 +725,20 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	host->data_early = 0;
 	host->data->bytes_xfered = 0;
 
-	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))
-		host->flags |= SDHCI_REQ_USE_DMA;
-
-	/*
-	 * FIXME: This doesn't account for merging when mapping the
-	 * scatterlist.
-	 *
-	 * The assumption here being that alignment and lengths are
-	 * the same after DMA mapping to device address space.
-	 */
-	if (host->flags & SDHCI_REQ_USE_DMA) {
+	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
 		struct scatterlist *sg;
 		unsigned int length_mask, offset_mask;
 		int i;
 
+		host->flags |= SDHCI_REQ_USE_DMA;
+
+		/*
+		 * FIXME: This doesn't account for merging when mapping the
+		 * scatterlist.
+		 *
+		 * The assumption here being that alignment and lengths are
+		 * the same after DMA mapping to device address space.
+		 */
 		length_mask = 0;
 		offset_mask = 0;
 		if (host->flags & SDHCI_USE_ADMA) {
-- 
2.1.0

Re: [PATCH v4 00/25] MMC/SDHCI fixes

[Ulf Hansson]

+ Adrian

On 29 January 2016 at 10:43, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Dec 21, 2015 at 11:39:56AM +0000, Russell King - ARM Linux wrote:
>> 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.
>>
>> - reworking the voltage-ranges code so that callers know whether the
>>   optional voltage-ranges property was specified.
>>
>> - "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 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.
>>
>> - Fixing another DMA API leak with the pre/post request handling, where
>>   a current request can change the state of the SDHCI_REQ_USE_DMA, and
>>   thus stop a mapped request being unmapped.
>>
>> - Fixing the SDHCI timeout calculation code to correctly round timeouts
>>   up rather than down, and correctly calculate the time (in microseconds)
>>   for a certain number of card clocks.
>>
>> - Consolidating the SDHCI address/size alignment handling so we only walk
>>   the scatterlist once instead of twice.
>>
>> Further patches are necessary as there are still a number of observable
>> problems when using this driver on iMX6 with UHS cards.  Patches up to
>> and including "mmc: sdhci: further fix for DMA unmapping in
>> sdhci_post_req()" should be considered for inclusion in the stable
>> kernel trees.
>>
>> I've tested these changes on iMX6 at UHS speeds, and Armada 388 SoCs
>> at HS speeds so far.  Others should test them.
>
> One comment from Adrian addressed.
>
>  drivers/mmc/card/block.c       |   4 +-
>  drivers/mmc/core/core.c        |  17 +-
>  drivers/mmc/host/sdhci-pxav3.c |   6 +-
>  drivers/mmc/host/sdhci.c       | 444 ++++++++++++++++++-----------------------
>  drivers/mmc/host/sdhci.h       |   4 +-
>  5 files changed, 213 insertions(+), 262 deletions(-)
>
> --
> 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.

I have applied patch 1 to 4, for next!

The rest are sdhci patches, which from now on needs acks from Adrian
for me to pick them up.

Thanks and kind regards!
Uffe

Re: [PATCH v4 00/25] MMC/SDHCI fixes

[Adrian Hunter]

On 04/02/16 12:55, Ulf Hansson wrote:
> + Adrian
> 
> On 29 January 2016 at 10:43, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Mon, Dec 21, 2015 at 11:39:56AM +0000, Russell King - ARM Linux wrote:
>>> 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.
>>>
>>> - reworking the voltage-ranges code so that callers know whether the
>>>   optional voltage-ranges property was specified.
>>>
>>> - "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 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.
>>>
>>> - Fixing another DMA API leak with the pre/post request handling, where
>>>   a current request can change the state of the SDHCI_REQ_USE_DMA, and
>>>   thus stop a mapped request being unmapped.
>>>
>>> - Fixing the SDHCI timeout calculation code to correctly round timeouts
>>>   up rather than down, and correctly calculate the time (in microseconds)
>>>   for a certain number of card clocks.
>>>
>>> - Consolidating the SDHCI address/size alignment handling so we only walk
>>>   the scatterlist once instead of twice.
>>>
>>> Further patches are necessary as there are still a number of observable
>>> problems when using this driver on iMX6 with UHS cards.  Patches up to
>>> and including "mmc: sdhci: further fix for DMA unmapping in
>>> sdhci_post_req()" should be considered for inclusion in the stable
>>> kernel trees.
>>>
>>> I've tested these changes on iMX6 at UHS speeds, and Armada 388 SoCs
>>> at HS speeds so far.  Others should test them.
>>
>> One comment from Adrian addressed.
>>
>>  drivers/mmc/card/block.c       |   4 +-
>>  drivers/mmc/core/core.c        |  17 +-
>>  drivers/mmc/host/sdhci-pxav3.c |   6 +-
>>  drivers/mmc/host/sdhci.c       | 444 ++++++++++++++++++-----------------------
>>  drivers/mmc/host/sdhci.h       |   4 +-
>>  5 files changed, 213 insertions(+), 262 deletions(-)
>>
>> --
>> 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.
> 
> I have applied patch 1 to 4, for next!
> 
> The rest are sdhci patches, which from now on needs acks from Adrian
> for me to pick them up.

I have taken the liberty of re-organizing Russell's patches to put the
bug fixes first with stable cc's.  I also did 2 tweaks which result in
final code having the diff below to the original:


diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f857cb2a3d05..03fbb36580f7 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2270,7 +2270,7 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask)
 		 * will time out.
 		 */
 		if (host->cmd->data &&
-		    intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT) ==
+		    (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
 		     SDHCI_INT_CRC) {
 			host->cmd = NULL;
 			return;
@@ -2920,7 +2920,8 @@ int sdhci_add_host(struct sdhci_host *host)
 			pr_warn("%s: Unable to allocate ADMA buffers - falling back to standard DMA\n",
 				mmc_hostname(mmc));
 			host->flags &= ~SDHCI_USE_ADMA;
-		} else if (dma & SDHCI_ADMA2_MASK) {
+		} else if ((dma + host->align_buffer_sz) &
+			   (SDHCI_ADMA2_DESC_ALIGN - 1)) {
 			pr_warn("%s: unable to allocate aligned ADMA descriptor\n",
 				mmc_hostname(mmc));
 			host->flags &= ~SDHCI_USE_ADMA;


Where I have made changes, they are indicated in the patch commit.

I did some basic testing of eMMC, SD card and SDIO, for the fixes alone
and also the entire patch set.

Russell, please let me know if this is OK, and if so, Ulf please consider
pulling:


The following changes since commit 7d9e20d2be0d836c4eb7afca901eced34274c8dd:

  Merge branch 'fixes' into next (2016-02-08 16:22:56 +0100)

are available in the git repository at:


  git://git.infradead.org/users/ahunter/linux-sdhci.git master

for you to fetch changes up to eb6bf3c3513d2e2ab77d86bc6c2f2755073f0745:

  mmc: sdhci: further code simplication (2016-02-11 16:24:46 +0200)

----------------------------------------------------------------
Russell King (22):
      mmc: sdhci: move initialisation of command error member
      mmc: sdhci: clean up command error handling
      mmc: sdhci: fix command response CRC error handling
      mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer
      mmc: sdhci: plug DMA mapping leak on error
      mmc: sdhci-pxav3: fix higher speed mode capabilities
      mmc: sdhci: further fix for DMA unmapping in sdhci_post_req()
      mmc: sdhci: fix data timeout (part 1)
      mmc: sdhci: fix data timeout (part 2)
      mmc: sdhci: allocate alignment and DMA descriptor buffer together
      mmc: sdhci: clean up coding style in sdhci_adma_table_pre()
      mmc: sdhci: avoid walking SG list for writes
      mmc: sdhci: factor out common DMA cleanup in sdhci_finish_data()
      mmc: sdhci: move sdhci_pre_dma_transfer()
      mmc: sdhci: factor out sdhci_pre_dma_transfer() from sdhci_adma_table_pre()
      mmc: sdhci: pass the cookie into sdhci_pre_dma_transfer()
      mmc: sdhci: always unmap a mapped data transfer in sdhci_post_req()
      mmc: sdhci: clean up host cookie handling
      mmc: sdhci: cleanup DMA un-mapping
      mmc: sdhci: prepare DMA address/size quirk handling consolidation
      mmc: sdhci: consolidate the DMA/ADMA size/address quicks
      mmc: sdhci: further code simplication

 drivers/mmc/host/sdhci-pxav3.c |   6 ++-
 drivers/mmc/host/sdhci.c       | 444 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------------------------------
 drivers/mmc/host/sdhci.h       |   4 +-
 3 files changed, 200 insertions(+), 254 deletions(-)


Regards
Adrian

Re: [PATCH v4 00/25] MMC/SDHCI fixes

[Gregory CLEMENT]

Hi Adrian,
 
 On ven., févr. 12 2016, Adrian Hunter <adrian.hunter@intel.com> wrote:

> On 04/02/16 12:55, Ulf Hansson wrote:
>> + Adrian
>> 
>> On 29 January 2016 at 10:43, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Mon, Dec 21, 2015 at 11:39:56AM +0000, Russell King - ARM Linux wrote:
>>>> 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.
>>>>
>>>> - reworking the voltage-ranges code so that callers know whether the
>>>>   optional voltage-ranges property was specified.
>>>>
>>>> - "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 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.
>>>>
>>>> - Fixing another DMA API leak with the pre/post request handling, where
>>>>   a current request can change the state of the SDHCI_REQ_USE_DMA, and
>>>>   thus stop a mapped request being unmapped.
>>>>
>>>> - Fixing the SDHCI timeout calculation code to correctly round timeouts
>>>>   up rather than down, and correctly calculate the time (in microseconds)
>>>>   for a certain number of card clocks.
>>>>
>>>> - Consolidating the SDHCI address/size alignment handling so we only walk
>>>>   the scatterlist once instead of twice.
>>>>
>>>> Further patches are necessary as there are still a number of observable
>>>> problems when using this driver on iMX6 with UHS cards.  Patches up to
>>>> and including "mmc: sdhci: further fix for DMA unmapping in
>>>> sdhci_post_req()" should be considered for inclusion in the stable
>>>> kernel trees.
>>>>
>>>> I've tested these changes on iMX6 at UHS speeds, and Armada 388 SoCs
>>>> at HS speeds so far.  Others should test them.
>>>
>>> One comment from Adrian addressed.
>>>
>>>  drivers/mmc/card/block.c       |   4 +-
>>>  drivers/mmc/core/core.c        |  17 +-
>>>  drivers/mmc/host/sdhci-pxav3.c |   6 +-
>>>  drivers/mmc/host/sdhci.c       | 444 ++++++++++++++++++-----------------------
>>>  drivers/mmc/host/sdhci.h       |   4 +-
>>>  5 files changed, 213 insertions(+), 262 deletions(-)
>>>
>>> --
>>> 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.
>> 
>> I have applied patch 1 to 4, for next!
>> 
>> The rest are sdhci patches, which from now on needs acks from Adrian
>> for me to pick them up.
>
> I have taken the liberty of re-organizing Russell's patches to put the
> bug fixes first with stable cc's.  I also did 2 tweaks which result in
> final code having the diff below to the original:
>
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f857cb2a3d05..03fbb36580f7 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2270,7 +2270,7 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask)
>  		 * will time out.
>  		 */
>  		if (host->cmd->data &&
> -		    intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT) ==
> +		    (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
>  		     SDHCI_INT_CRC) {
>  			host->cmd = NULL;
>  			return;
> @@ -2920,7 +2920,8 @@ int sdhci_add_host(struct sdhci_host *host)
>  			pr_warn("%s: Unable to allocate ADMA buffers - falling back to standard DMA\n",
>  				mmc_hostname(mmc));
>  			host->flags &= ~SDHCI_USE_ADMA;
> -		} else if (dma & SDHCI_ADMA2_MASK) {
> +		} else if ((dma + host->align_buffer_sz) &
> +			   (SDHCI_ADMA2_DESC_ALIGN - 1)) {
>  			pr_warn("%s: unable to allocate aligned ADMA descriptor\n",
>  				mmc_hostname(mmc));
>  			host->flags &= ~SDHCI_USE_ADMA;
>
>
> Where I have made changes, they are indicated in the patch commit.
>
> I did some basic testing of eMMC, SD card and SDIO, for the fixes alone
> and also the entire patch set.
>
> Russell, please let me know if this is OK, and if so, Ulf please consider
> pulling:
>
>
> The following changes since commit 7d9e20d2be0d836c4eb7afca901eced34274c8dd:
>
>   Merge branch 'fixes' into next (2016-02-08 16:22:56 +0100)
>
> are available in the git repository at:
>
>
>   git://git.infradead.org/users/ahunter/linux-sdhci.git master

Thanks to your branch I was able to test the series on the Armada 38x
boards.

At the beginning my main motivation was to ensure that now thanks to the
patch "mmc: sdhci-pxav3: fix higher speed mode capabilities", we really
manage to support SDR50 and DDR50 modes. However I realized that I don't
have any 1.8V support on the board I have.

So I can't confirm this, but at least I didn't see any regression while
using the SD cards.

Gregory



>
> for you to fetch changes up to eb6bf3c3513d2e2ab77d86bc6c2f2755073f0745:
>
>   mmc: sdhci: further code simplication (2016-02-11 16:24:46 +0200)
>
> ----------------------------------------------------------------
> Russell King (22):
>       mmc: sdhci: move initialisation of command error member
>       mmc: sdhci: clean up command error handling
>       mmc: sdhci: fix command response CRC error handling
>       mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer
>       mmc: sdhci: plug DMA mapping leak on error
>       mmc: sdhci-pxav3: fix higher speed mode capabilities
>       mmc: sdhci: further fix for DMA unmapping in sdhci_post_req()
>       mmc: sdhci: fix data timeout (part 1)
>       mmc: sdhci: fix data timeout (part 2)
>       mmc: sdhci: allocate alignment and DMA descriptor buffer together
>       mmc: sdhci: clean up coding style in sdhci_adma_table_pre()
>       mmc: sdhci: avoid walking SG list for writes
>       mmc: sdhci: factor out common DMA cleanup in sdhci_finish_data()
>       mmc: sdhci: move sdhci_pre_dma_transfer()
>       mmc: sdhci: factor out sdhci_pre_dma_transfer() from sdhci_adma_table_pre()
>       mmc: sdhci: pass the cookie into sdhci_pre_dma_transfer()
>       mmc: sdhci: always unmap a mapped data transfer in sdhci_post_req()
>       mmc: sdhci: clean up host cookie handling
>       mmc: sdhci: cleanup DMA un-mapping
>       mmc: sdhci: prepare DMA address/size quirk handling consolidation
>       mmc: sdhci: consolidate the DMA/ADMA size/address quicks
>       mmc: sdhci: further code simplication
>
>  drivers/mmc/host/sdhci-pxav3.c |   6 ++-
>  drivers/mmc/host/sdhci.c       | 444 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------------------------------
>  drivers/mmc/host/sdhci.h       |   4 +-
>  3 files changed, 200 insertions(+), 254 deletions(-)
>
>
> Regards
> Adrian

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v4 00/25] MMC/SDHCI fixes

[Ulf Hansson]

[...]

>
> I have taken the liberty of re-organizing Russell's patches to put the
> bug fixes first with stable cc's.  I also did 2 tweaks which result in
> final code having the diff below to the original:
>
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f857cb2a3d05..03fbb36580f7 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2270,7 +2270,7 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask)
>                  * will time out.
>                  */
>                 if (host->cmd->data &&
> -                   intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT) ==
> +                   (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
>                      SDHCI_INT_CRC) {
>                         host->cmd = NULL;
>                         return;
> @@ -2920,7 +2920,8 @@ int sdhci_add_host(struct sdhci_host *host)
>                         pr_warn("%s: Unable to allocate ADMA buffers - falling back to standard DMA\n",
>                                 mmc_hostname(mmc));
>                         host->flags &= ~SDHCI_USE_ADMA;
> -               } else if (dma & SDHCI_ADMA2_MASK) {
> +               } else if ((dma + host->align_buffer_sz) &
> +                          (SDHCI_ADMA2_DESC_ALIGN - 1)) {
>                         pr_warn("%s: unable to allocate aligned ADMA descriptor\n",
>                                 mmc_hostname(mmc));
>                         host->flags &= ~SDHCI_USE_ADMA;
>
>
> Where I have made changes, they are indicated in the patch commit.
>
> I did some basic testing of eMMC, SD card and SDIO, for the fixes alone
> and also the entire patch set.
>
> Russell, please let me know if this is OK, and if so, Ulf please consider
> pulling:
>
>
> The following changes since commit 7d9e20d2be0d836c4eb7afca901eced34274c8dd:
>
>   Merge branch 'fixes' into next (2016-02-08 16:22:56 +0100)
>
> are available in the git repository at:
>
>
>   git://git.infradead.org/users/ahunter/linux-sdhci.git master
>
> for you to fetch changes up to eb6bf3c3513d2e2ab77d86bc6c2f2755073f0745:
>
>   mmc: sdhci: further code simplication (2016-02-11 16:24:46 +0200)
>
> ----------------------------------------------------------------
> Russell King (22):
>       mmc: sdhci: move initialisation of command error member
>       mmc: sdhci: clean up command error handling
>       mmc: sdhci: fix command response CRC error handling
>       mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer
>       mmc: sdhci: plug DMA mapping leak on error
>       mmc: sdhci-pxav3: fix higher speed mode capabilities
>       mmc: sdhci: further fix for DMA unmapping in sdhci_post_req()
>       mmc: sdhci: fix data timeout (part 1)
>       mmc: sdhci: fix data timeout (part 2)
>       mmc: sdhci: allocate alignment and DMA descriptor buffer together
>       mmc: sdhci: clean up coding style in sdhci_adma_table_pre()
>       mmc: sdhci: avoid walking SG list for writes
>       mmc: sdhci: factor out common DMA cleanup in sdhci_finish_data()
>       mmc: sdhci: move sdhci_pre_dma_transfer()
>       mmc: sdhci: factor out sdhci_pre_dma_transfer() from sdhci_adma_table_pre()
>       mmc: sdhci: pass the cookie into sdhci_pre_dma_transfer()
>       mmc: sdhci: always unmap a mapped data transfer in sdhci_post_req()
>       mmc: sdhci: clean up host cookie handling
>       mmc: sdhci: cleanup DMA un-mapping
>       mmc: sdhci: prepare DMA address/size quirk handling consolidation
>       mmc: sdhci: consolidate the DMA/ADMA size/address quicks
>       mmc: sdhci: further code simplication
>
>  drivers/mmc/host/sdhci-pxav3.c |   6 ++-
>  drivers/mmc/host/sdhci.c       | 444 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------------------------------
>  drivers/mmc/host/sdhci.h       |   4 +-
>  3 files changed, 200 insertions(+), 254 deletions(-)
>
>
> Regards
> Adrian


Thanks Adrian!

I have pulled in the changes, or actually cherry-picked them as I have
re-based my next branch.

Also, I have added Gregory's tested-by tag.

Kind regards
Uffe