[PATCH v3 00/25] MMC/SDHCI fixes

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

Repost against 4.5-rc1, with one additional patch.

 drivers/mmc/card/block.c       |   4 +-
 drivers/mmc/core/core.c        |  17 +-
 drivers/mmc/host/sdhci-pxav3.c |   6 +-
 drivers/mmc/host/sdhci.c       | 443 ++++++++++++++++++-----------------------
 drivers/mmc/host/sdhci.h       |   4 +-
 5 files changed, 212 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 v3 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 v3 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 v3 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 v3 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 v3 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 v3 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 v3 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..cbc4145daaf8 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 v3 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 cbc4145daaf8..c70a2d2eb6d9 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 v3 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 | 55 +++++++++++++++++-------------------------------
 1 file changed, 19 insertions(+), 36 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c70a2d2eb6d9..f23fdb485393 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,27 @@ 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 & SDHCI_ADMA2_MASK) {
 			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 +3431,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 v3 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 f23fdb485393..d1f802507ab1 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 v3 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 d1f802507ab1..fea74ebb4615 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 v3 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 fea74ebb4615..3a7b0ef0196a 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 v3 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 3a7b0ef0196a..f260804d3d65 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 v3 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 f260804d3d65..27f37f562ea7 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 v3 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 27f37f562ea7..51f6678879d3 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 v3 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 51f6678879d3..5143003f2c67 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 v3 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 5143003f2c67..44e85916a8da 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 v3 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 44e85916a8da..27a6e54a6f63 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 v3 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 v3 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 27a6e54a6f63..5e249360a108 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 v3 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 5e249360a108..7fa9b2e8cfb2 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 v3 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 7fa9b2e8cfb2..30a790ee74f5 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 v3 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 30a790ee74f5..12d53009a6f8 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 v3 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 12d53009a6f8..92ceceb47b74 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 v3 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 92ceceb47b74..f9a5be9fa19c 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 v3 07/25] mmc: sdhci: command response CRC error handling

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3376 bytes --]

Hi Russell,

[auto build test WARNING on ulf.hansson-mmc/next]
[also build test WARNING on v4.5-rc1 next-20160125]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Russell-King/mmc-core-shut-up-voltage-ranges-unspecified-pr_info/20160126-214738
base:   https://git.linaro.org/people/ulf.hansson/mmc next
config: x86_64-randconfig-x018-01260845 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/linkage.h:4:0,
                    from include/linux/kernel.h:6,
                    from include/linux/delay.h:10,
                    from drivers/mmc/host/sdhci.c:16:
   drivers/mmc/host/sdhci.c: In function 'sdhci_cmd_irq':
   drivers/mmc/host/sdhci.c:2344:15: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
          intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT) ==
                  ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                               ^
>> drivers/mmc/host/sdhci.c:2343:3: note: in expansion of macro 'if'
      if (host->cmd->data &&
      ^
   drivers/mmc/host/sdhci.c:2344:15: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
          intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT) ==
                  ^
   include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                                           ^
>> drivers/mmc/host/sdhci.c:2343:3: note: in expansion of macro 'if'
      if (host->cmd->data &&
      ^
   drivers/mmc/host/sdhci.c:2344:15: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
          intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT) ==
                  ^
   include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
>> drivers/mmc/host/sdhci.c:2343:3: note: in expansion of macro 'if'
      if (host->cmd->data &&
      ^

vim +/if +2343 drivers/mmc/host/sdhci.c

  2327			       SDHCI_INT_END_BIT | SDHCI_INT_INDEX)) {
  2328			if (intmask & SDHCI_INT_TIMEOUT)
  2329				host->cmd->error = -ETIMEDOUT;
  2330			else
  2331				host->cmd->error = -EILSEQ;
  2332	
  2333			/*
  2334			 * If this command initiates a data phase and a response
  2335			 * CRC error is signalled, the card can start transferring
  2336			 * data - the card may have received the command without
  2337			 * error.  We must not terminate the mmc_request early.
  2338			 *
  2339			 * If the card did not receive the command or returned an
  2340			 * error which prevented it sending data, the data phase
  2341			 * will time out.
  2342			 */
> 2343			if (host->cmd->data &&
  2344			    intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT) ==
  2345			     SDHCI_INT_CRC) {
  2346				host->cmd = NULL;
  2347				return;
  2348			}
  2349	
  2350			tasklet_schedule(&host->finish_tasklet);
  2351			return;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 25668 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1928 bytes --]

Hi Russell,

[auto build test WARNING on ulf.hansson-mmc/next]
[also build test WARNING on v4.5-rc1 next-20160125]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Russell-King/mmc-core-shut-up-voltage-ranges-unspecified-pr_info/20160126-214738
base:   https://git.linaro.org/people/ulf.hansson/mmc next
config: x86_64-randconfig-x010-01260845 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/mmc/host/sdhci.c: In function 'sdhci_cmd_irq':
>> drivers/mmc/host/sdhci.c:2344:15: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
          intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT) ==
                  ^

vim +2344 drivers/mmc/host/sdhci.c

  2328			if (intmask & SDHCI_INT_TIMEOUT)
  2329				host->cmd->error = -ETIMEDOUT;
  2330			else
  2331				host->cmd->error = -EILSEQ;
  2332	
  2333			/*
  2334			 * If this command initiates a data phase and a response
  2335			 * CRC error is signalled, the card can start transferring
  2336			 * data - the card may have received the command without
  2337			 * error.  We must not terminate the mmc_request early.
  2338			 *
  2339			 * If the card did not receive the command or returned an
  2340			 * error which prevented it sending data, the data phase
  2341			 * will time out.
  2342			 */
  2343			if (host->cmd->data &&
> 2344			    intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT) ==
  2345			     SDHCI_INT_CRC) {
  2346				host->cmd = NULL;
  2347				return;
  2348			}
  2349	
  2350			tasklet_schedule(&host->finish_tasklet);
  2351			return;
  2352		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23799 bytes --]

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

[Gregory CLEMENT]

Hi ,
 
 On mar., janv. 26 2016, 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.
>>

For all the upper change, I made a review of them. I am not an SDHCI
export but for my point of view everything seems very sensible.

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

This one is my fault I ported this patch with the assumption it was
working. I didn't had a SDCard recent enough to test it at this time. I
hope having an UHS grade card soon and being able to send a tested-by.

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

As for the first patches, I reviewed them, I didn't find any flaw, but
again I am not an expert.

Gregory

>> 
>> 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.
>
> Repost against 4.5-rc1, with one additional patch.
>
>  drivers/mmc/card/block.c       |   4 +-
>  drivers/mmc/core/core.c        |  17 +-
>  drivers/mmc/host/sdhci-pxav3.c |   6 +-
>  drivers/mmc/host/sdhci.c       | 443 ++++++++++++++++++-----------------------
>  drivers/mmc/host/sdhci.h       |   4 +-
>  5 files changed, 212 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.

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

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

[Adrian Hunter]

On 26/01/16 15:39, Russell King wrote:
> 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 | 55 +++++++++++++++++-------------------------------
>  1 file changed, 19 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c70a2d2eb6d9..f23fdb485393 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,27 @@ 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 & SDHCI_ADMA2_MASK) {

The descriptor table has a bigger alignment requirement, so that should remain the check i.e.

		} 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 +3431,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;
> 

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

[Russell King - ARM Linux]

On Wed, Jan 27, 2016 at 11:14:41AM +0200, Adrian Hunter wrote:
> On 26/01/16 15:39, Russell King wrote:
> > -		} else if (host->adma_addr & (SDHCI_ADMA2_DESC_ALIGN - 1)) {
> > +		} else if (dma & SDHCI_ADMA2_MASK) {
> 
> The descriptor table has a bigger alignment requirement, so that should remain the check i.e.
> 
> 		} else if ((dma + host->align_buffer_sz) & (SDHCI_ADMA2_DESC_ALIGN - 1)) {

No other comments?

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