MMC/SDHCI fixes
v3 cover MMC/SDHCI fixes
Quoted 130 lines · Russell King - ARM Linux
This all started with a report from an iMX6 Hummingboard user that they were seeing regular "mmc0: Got data interrupt
0x00000002even 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.chas 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()) insdhci_finish_data()and its child functionsdhci_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()andsdhci_pre_dma_transfer()as withsdhci_finish_data()andsdhci_adma_table_post().- Fix the mess that is the data segment
host_cookiehandling. 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.
Quoted 8 lines
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(-)
Signature · 3 lines
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
PATCH v3 1/25 mmc: core: shut up "voltage-ranges unspecified" pr_info()
Each time a driver such as sdhci-esdhc-imx is probed, we get a info printk complaining that the DT voltage-ranges property has not been specified.
However, the DT binding specifically says that the voltage-ranges property is optional. That means we should not be complaining that DT hasn't specified this property: by indicating that it's optional, it is valid not to have the property in DT.
Silence the warning if the property is missing.
--- drivers/mmc/core/core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/mmc/core/core.c +6 -2
@@ -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 ; }
Signature · 1 line
2.1.0
PATCH v3 2/25 mmc: core: improve mmc_of_parse_voltage() to return better status
In reply to Russell King - ARM LinuxImprove 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.
--- drivers/mmc/core/core.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/mmc/core/core.c +5 -4
@@ -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);
Signature · 1 line
2.1.0
PATCH v3 3/25 mmc: block: shut up "retrying because a re-tune was needed" message
In reply to Russell King - ARM LinuxRe-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.
--- drivers/mmc/card/block.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/mmc/card/block.c +2 -2
@@ -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 ; }
Signature · 1 line
2.1.0
PATCH v3 4/25 mmc: core: report tuning command execution failure reason
In reply to Russell King - ARM LinuxPrint the error code when the tuning command fails. This allows the reason for the failure to be reported, which aids debugging.
--- drivers/mmc/core/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mmc/core/core.c +1 -1
@@ -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);
Signature · 1 line
2.1.0
PATCH v3 5/25 mmc: sdhci: move initialisation of command error member
In reply to Russell King - ARM LinuxWhen a command is started, logically it has no error. Initialise the command's error member to zero whenever we start a command.
--- drivers/mmc/host/sdhci.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +3 -2
@@ -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 ;
Signature · 1 line
2.1.0
PATCH v3 6/25 mmc: sdhci: clean up command error handling
In reply to Russell King - ARM LinuxAvoid multiple tests while handling a command error; simplify the code.
--- drivers/mmc/host/sdhci.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/mmc/host/sdhci.c +6 -6
@@ -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 ; }
Signature · 1 line
2.1.0
PATCH v3 7/25 mmc: sdhci: command response CRC error handling
In reply to Russell King - ARM LinuxWhen 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.
--- drivers/mmc/host/sdhci.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
drivers/mmc/host/sdhci.c +17 -0
@@ -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 ; }
Signature · 1 line
2.1.0
kbuild test robot
build botRe: [PATCH v3 07/25] mmc: sdhci: command response CRC error handling
reproduce:
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
kbuild test robot
build botRe: [PATCH v3 07/25] mmc: sdhci: command response CRC error handling
In reply to Russell Kingreproduce:
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
PATCH v3 8/25 mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer
In reply to Russell King - ARM LinuxUnnecessarily 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.
--- drivers/mmc/host/sdhci.c | 54 ++++++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 36 deletions(-)
drivers/mmc/host/sdhci.c +18 -36
@@ -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 ;
Signature · 1 line
2.1.0
PATCH v3 9/25 mmc: sdhci: allocate alignment and DMA descriptor buffer together
In reply to Russell King - ARM LinuxAllocate 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.
--- drivers/mmc/host/sdhci.c | 55 +++++++++++++++++------------------------------- 1 file changed, 19 insertions(+), 36 deletions(-)
drivers/mmc/host/sdhci.c +19 -36
@@ -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 ;
Signature · 1 line
2.1.0
Adrian Hunter
maintainerRe: [PATCH v3 09/25] mmc: sdhci: allocate alignment and DMA descriptor buffer together
Quoted 61 lines · 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 <[email protected]>--- drivers/mmc/host/sdhci.c | 55 +++++++++++++++++------------------------------- 1 file changed, 19 insertions(+), 36 deletions(-)drivers/mmc/host/sdhci.c +4
-19@@ -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);
drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 2961
Quoted 10 lines
drivers/mmc/host/sdhci.c +4
-3+ 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)) {Quoted 41 lines
drivers/mmc/host/sdhci.c +11
-14pr_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
Quoted 7 lines · Adrian Hunter
drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 2961
Quoted 2 lines · Russell King
drivers/mmc/host/sdhci.c +1
-1- }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?
Signature · 3 lines
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
PATCH v3 10/25 mmc: sdhci: clean up coding style in sdhci_adma_table_pre()
In reply to Russell King - ARM Linux--- drivers/mmc/host/sdhci.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-)
drivers/mmc/host/sdhci.c +9 -20
@@ -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 ;
Signature · 1 line
2.1.0
PATCH v3 11/25 mmc: sdhci: avoid walking SG list for writes
In reply to Russell King - ARM LinuxIf 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.
--- drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
drivers/mmc/host/sdhci.c +22 -20
@@ -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 ; + } } } }
Signature · 1 line
2.1.0
PATCH v3 12/25 mmc: sdhci: factor out common DMA cleanup in sdhci_finish_data()
In reply to Russell King - ARM Linuxsdhci_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().
--- drivers/mmc/host/sdhci.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)
drivers/mmc/host/sdhci.c +7 -22
@@ -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 ; } }
Signature · 1 line
2.1.0
PATCH v3 13/25 mmc: sdhci: move sdhci_pre_dma_transfer()
In reply to Russell King - ARM LinuxMove sdhci_pre_dma_transfer() to avoid needing to declare this function before use.
--- drivers/mmc/host/sdhci.c | 52 +++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 27 deletions(-)
drivers/mmc/host/sdhci.c +25 -27
@@ -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) {
Signature · 1 line
2.1.0
PATCH v3 14/25 mmc: sdhci: factor out sdhci_pre_dma_transfer() from sdhci_adma_table_pre()
In reply to Russell King - ARM LinuxIn 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.
--- drivers/mmc/host/sdhci.c | 62 ++++++++++++++++++------------------------------ 1 file changed, 23 insertions(+), 39 deletions(-)
drivers/mmc/host/sdhci.c +23 -39
@@ -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 ); } }
Signature · 1 line
2.1.0
PATCH v3 15/25 mmc: sdhci: pass the cookie into sdhci_pre_dma_transfer()
In reply to Russell King - ARM LinuxPass the desired cookie for a successful map. This is in preparation to clean up the MAPPED/GIVEN states.
--- drivers/mmc/host/sdhci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/mmc/host/sdhci.c +4 -4
@@ -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)
Signature · 1 line
2.1.0
PATCH v3 16/25 mmc: sdhci: always unmap a mapped data transfer in sdhci_post_req()
In reply to Russell King - ARM LinuxIf the host cookie indicates that the data buffers of a request are mapped at sdhci_post_req() time, always unmap the data buffers.
--- drivers/mmc/host/sdhci.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +1 -2
@@ -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 );
Signature · 1 line
2.1.0
PATCH v3 17/25 mmc: sdhci: clean up host cookie handling
In reply to Russell King - ARM LinuxCommit 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.
--- drivers/mmc/host/sdhci.c | 12 ++++++------ drivers/mmc/host/sdhci.h | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-)
drivers/mmc/host/sdhci.c +6 -6
@@ -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)
drivers/mmc/host/sdhci.h +2 -2
@@ -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 {
Signature · 1 line
2.1.0
PATCH v3 18/25 mmc: sdhci: plug DMA mapping leak on error
In reply to Russell King - ARM LinuxIf 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.
--- drivers/mmc/host/sdhci.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)
drivers/mmc/host/sdhci.c +19 -11
@@ -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. */
Signature · 1 line
2.1.0
PATCH v3 19/25 mmc: sdhci-pxav3: fix higher speed mode capabilities
In reply to Russell King - ARM LinuxCommit 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.
--- drivers/mmc/host/sdhci-pxav3.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci-pxav3.c +4 -2
@@ -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 ;
Signature · 1 line
2.1.0
PATCH v3 20/25 mmc: sdhci: further fix for DMA unmapping in sdhci_post_req()
In reply to Russell King - ARM Linuxsdhci_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.
--- drivers/mmc/host/sdhci.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/mmc/host/sdhci.c +6 -7
@@ -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,
Signature · 1 line
2.1.0
PATCH v3 21/25 mmc: sdhci: fix data timeout (part 1)
In reply to Russell King - ARM LinuxThe 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.
--- drivers/mmc/host/sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mmc/host/sdhci.c +1 -1
@@ -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; }
Signature · 1 line
2.1.0
PATCH v3 22/25 mmc: sdhci: fix data timeout (part 2)
In reply to Russell King - ARM LinuxThe 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.
--- drivers/mmc/host/sdhci.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +13 -2
@@ -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; + } }/*
Signature · 1 line
2.1.0
PATCH v3 23/25 mmc: sdhci: prepare DMA address/size quirk handling consolidation
In reply to Russell King - ARM LinuxPrepare to consolidate the DMA address/size quirk handling into one single loop.
--- drivers/mmc/host/sdhci.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)
drivers/mmc/host/sdhci.c +15 -13
@@ -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 ;
Signature · 1 line
2.1.0
PATCH v3 24/25 mmc: sdhci: consolidate the DMA/ADMA size/address quicks
In reply to Russell King - ARM LinuxRather 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.
--- drivers/mmc/host/sdhci.c | 48 ++++++++++++++++-------------------------------- 1 file changed, 16 insertions(+), 32 deletions(-)
drivers/mmc/host/sdhci.c +16 -32
@@ -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 ;
Signature · 1 line
2.1.0
PATCH v3 25/25 mmc: sdhci: further code simplication
In reply to Russell King - ARM LinuxFurther 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.
--- drivers/mmc/host/sdhci.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/mmc/host/sdhci.c +10 -11
@@ -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 ) {
Signature · 1 line
2.1.0
Hi ,
Quoted 110 lines · Russell King - ARM Linux
Quoted 109 lines · Russell King - ARM Linux
This all started with a report from an iMX6 Hummingboard user that they were seeing regular "mmc0: Got data interrupt
0x00000002even 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.chas 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()) insdhci_finish_data()and its child functionsdhci_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()andsdhci_pre_dma_transfer()as withsdhci_finish_data()andsdhci_adma_table_post().- Fix the mess that is the data segment
host_cookiehandling. 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)...
Quoted 4 lines
Quoted 4 lines
- 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.
Quoted 5 lines
Quoted 5 lines
- 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.
Quoted 11 lines
Quoted 11 lines
- 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
Quoted 23 lines
Quoted 9 lines
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(-)Signature · 3 lines
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com