[U-Boot] [PATCH v1 00/11] Improvements on the MMC switch
This series brings some improvements related to the switch command: - use timeouts specified in the EXT_CSD instead of fixed values - retry switching partitions a few time before calling it a failure - make switching mode more reliable by checking the status of operation afterward. - make switching faster by using the DAT0 line as a ready/busy signal - Allow HS200/H400 modes with boot partitions. That didn't work well before because the switch was not reliable enough.
This has been tested on DRA76-evm and beaglebone black. Travis build: https://travis-ci.org/jjhiblot/u-boot/builds/550855605
Jean-Jacques Hiblot (11): mmc: omap_hsmmc: reset FSM for DAT and CMD lines if needed before a new command mmc: omap_hsmmc: don't fill the send_init_stream callback Revert "mmc: Add a new callback function to perform the 74 clocks cycle sequence" mmc: omap_hsmmc: provide wait_dat0 even if UHS modes are not supported mmc: add mmc_poll_for_busy() and change the purpose of mmc_send_status() mmc: if possible, poll the busy state using DAT0 mmc: use the generic timeout for cmd6 (SWITCH) provided in the ext_csd mmc: When switching partition, use the timeout specified in the ext_csd mmc: During a switch, poll on dat0 if available and check the final status mmc: do not change mode when accessing a boot partition mmc: retry a few times if a partition switch failed
drivers/mmc/mmc-uclass.c | 15 ---- drivers/mmc/mmc.c | 169 +++++++++++++++++++++----------------- drivers/mmc/mmc_private.h | 9 +- drivers/mmc/mmc_write.c | 4 +- drivers/mmc/omap_hsmmc.c | 26 ++---- include/mmc.h | 19 ++--- 6 files changed, 115 insertions(+), 127 deletions(-)
It sometimes happen that the PSTATE register does not indicate that the bus is ready when it really is. This usually happens after a mode switch. In that case it makes sense to reset the FSM handling the CMD and DATA
Also reset the FSMs if the STATE register cannot be cleared. This also sometimes happens after a mode switch.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/mmc/omap_hsmmc.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c index 133cdc1352..5446ca8b8d 100644 --- a/drivers/mmc/omap_hsmmc.c +++ b/drivers/mmc/omap_hsmmc.c @@ -1065,18 +1065,17 @@ static int omap_hsmmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, if (get_timer(0) - start > MAX_RETRY_MS) { printf("%s: timedout waiting on cmd inhibit to clear\n", __func__); + mmc_reset_controller_fsm(mmc_base, SYSCTL_SRD); + mmc_reset_controller_fsm(mmc_base, SYSCTL_SRC); return -ETIMEDOUT; } } writel(0xFFFFFFFF, &mmc_base->stat); - start = get_timer(0); - while (readl(&mmc_base->stat)) { - if (get_timer(0) - start > MAX_RETRY_MS) { - printf("%s: timedout waiting for STAT (%x) to clear\n", - __func__, readl(&mmc_base->stat)); - return -ETIMEDOUT; - } + if (readl(&mmc_base->stat)) { + mmc_reset_controller_fsm(mmc_base, SYSCTL_SRD); + mmc_reset_controller_fsm(mmc_base, SYSCTL_SRC); } + /* * CMDREG * CMDIDX[13:8] : Command index
This is not required. The MMC core sends CMD0 right after the initialization and it serves the same purpose.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/mmc/omap_hsmmc.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c index 5446ca8b8d..d8f36cd1db 100644 --- a/drivers/mmc/omap_hsmmc.c +++ b/drivers/mmc/omap_hsmmc.c @@ -775,14 +775,6 @@ tuning_error: return ret; } #endif - -static void omap_hsmmc_send_init_stream(struct udevice *dev) -{ - struct omap_hsmmc_data *priv = dev_get_priv(dev); - struct hsmmc *mmc_base = priv->base_addr; - - mmc_init_stream(mmc_base); -} #endif
static void mmc_enable_irq(struct mmc *mmc, struct mmc_cmd *cmd) @@ -1521,7 +1513,6 @@ static const struct dm_mmc_ops omap_hsmmc_ops = { #ifdef MMC_SUPPORTS_TUNING .execute_tuning = omap_hsmmc_execute_tuning, #endif - .send_init_stream = omap_hsmmc_send_init_stream, #if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) .wait_dat0 = omap_hsmmc_wait_dat0, #endif
This reverts commit 318a7a576bc49aa8b4207e694d3fbd48c663d6ac.
The last and only user of this callback had been the omap_hsmmc driver. It is not used anymore. Removing the callback.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/mmc/mmc-uclass.c | 13 ------------- drivers/mmc/mmc.c | 5 ----- include/mmc.h | 10 ---------- 3 files changed, 28 deletions(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index a9c8f335c1..6742d99e3d 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -47,19 +47,6 @@ int mmc_set_ios(struct mmc *mmc) return dm_mmc_set_ios(mmc->dev); }
-void dm_mmc_send_init_stream(struct udevice *dev) -{ - struct dm_mmc_ops *ops = mmc_get_ops(dev); - - if (ops->send_init_stream) - ops->send_init_stream(dev); -} - -void mmc_send_init_stream(struct mmc *mmc) -{ - dm_mmc_send_init_stream(mmc->dev); -} - #if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) int dm_mmc_wait_dat0(struct udevice *dev, int state, int timeout) { diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 456c1b4cc9..0ccbe10c5e 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -1504,10 +1504,6 @@ static int mmc_execute_tuning(struct mmc *mmc, uint opcode) } #endif
-static void mmc_send_init_stream(struct mmc *mmc) -{ -} - static int mmc_set_ios(struct mmc *mmc) { int ret = 0; @@ -2664,7 +2660,6 @@ int mmc_get_op_cond(struct mmc *mmc)
retry: mmc_set_initial_state(mmc); - mmc_send_init_stream(mmc);
/* Reset the Card */ err = mmc_go_idle(mmc); diff --git a/include/mmc.h b/include/mmc.h index 1f30f71d25..920f4cc53c 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -414,14 +414,6 @@ struct dm_mmc_ops { */ int (*set_ios)(struct udevice *dev);
- /** - * send_init_stream() - send the initialization stream: 74 clock cycles - * This is used after power up before sending the first command - * - * @dev: Device to update - */ - void (*send_init_stream)(struct udevice *dev); - /** * get_cd() - See whether a card is present * @@ -468,7 +460,6 @@ struct dm_mmc_ops { int dm_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, struct mmc_data *data); int dm_mmc_set_ios(struct udevice *dev); -void dm_mmc_send_init_stream(struct udevice *dev); int dm_mmc_get_cd(struct udevice *dev); int dm_mmc_get_wp(struct udevice *dev); int dm_mmc_execute_tuning(struct udevice *dev, uint opcode); @@ -476,7 +467,6 @@ int dm_mmc_wait_dat0(struct udevice *dev, int state, int timeout);
/* Transition functions for compatibility */ int mmc_set_ios(struct mmc *mmc); -void mmc_send_init_stream(struct mmc *mmc); int mmc_getcd(struct mmc *mmc); int mmc_getwp(struct mmc *mmc); int mmc_execute_tuning(struct mmc *mmc, uint opcode);
This function can also be used for eMMC devices.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/mmc/omap_hsmmc.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c index d8f36cd1db..3ea7f4e173 100644 --- a/drivers/mmc/omap_hsmmc.c +++ b/drivers/mmc/omap_hsmmc.c @@ -430,7 +430,6 @@ static void omap_hsmmc_conf_bus_power(struct mmc *mmc, uint signal_voltage) writel(ac12, &mmc_base->ac12); }
-#if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) static int omap_hsmmc_wait_dat0(struct udevice *dev, int state, int timeout) { int ret = -ETIMEDOUT; @@ -456,7 +455,6 @@ static int omap_hsmmc_wait_dat0(struct udevice *dev, int state, int timeout)
return ret; } -#endif
#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE) #if CONFIG_IS_ENABLED(DM_REGULATOR) @@ -1513,9 +1511,7 @@ static const struct dm_mmc_ops omap_hsmmc_ops = { #ifdef MMC_SUPPORTS_TUNING .execute_tuning = omap_hsmmc_execute_tuning, #endif -#if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) .wait_dat0 = omap_hsmmc_wait_dat0, -#endif }; #else static const struct mmc_ops omap_hsmmc_ops = {
mmc_send_status() is currently used to poll the card until it is ready, not actually returning the status of the card. Make it return the status and add another function to poll the card.
Also remove the 'extern' declaration in the mmc-private.h header to comply with the coding standard.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/mmc/mmc.c | 45 ++++++++++++++++++++++++++------------- drivers/mmc/mmc_private.h | 9 ++++---- drivers/mmc/mmc_write.c | 4 ++-- 3 files changed, 37 insertions(+), 21 deletions(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 0ccbe10c5e..d4eed62926 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -206,7 +206,7 @@ int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) } #endif
-int mmc_send_status(struct mmc *mmc, int timeout) +int mmc_send_status(struct mmc *mmc, unsigned int *status) { struct mmc_cmd cmd; int err, retries = 5; @@ -216,23 +216,39 @@ int mmc_send_status(struct mmc *mmc, int timeout) if (!mmc_host_is_spi(mmc)) cmd.cmdarg = mmc->rca << 16;
- while (1) { + while (retries--) { err = mmc_send_cmd(mmc, &cmd, NULL); if (!err) { - if ((cmd.response[0] & MMC_STATUS_RDY_FOR_DATA) && - (cmd.response[0] & MMC_STATUS_CURR_STATE) != - MMC_STATE_PRG) - break; + mmc_trace_state(mmc, &cmd); + *status = cmd.response[0]; + return 0; + } + } + mmc_trace_state(mmc, &cmd); + return -ECOMM; +} + +int mmc_poll_for_busy(struct mmc *mmc, int timeout) +{ + unsigned int status; + int err;
- if (cmd.response[0] & MMC_STATUS_MASK) { + while (1) { + err = mmc_send_status(mmc, &status); + if (err) + return err; + + if ((status & MMC_STATUS_RDY_FOR_DATA) && + (status & MMC_STATUS_CURR_STATE) != + MMC_STATE_PRG) + break; + + if (status & MMC_STATUS_MASK) { #if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT) - pr_err("Status Error: 0x%08x\n", - cmd.response[0]); + pr_err("Status Error: 0x%08x\n", status); #endif - return -ECOMM; - } - } else if (--retries < 0) - return err; + return -ECOMM; + }
if (timeout-- <= 0) break; @@ -240,7 +256,6 @@ int mmc_send_status(struct mmc *mmc, int timeout) udelay(1000); }
- mmc_trace_state(mmc, &cmd); if (timeout <= 0) { #if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT) pr_err("Timeout waiting card ready\n"); @@ -752,7 +767,7 @@ static int __mmc_switch(struct mmc *mmc, u8 set, u8 index, u8 value, }
/* Waiting for the ready status */ - return mmc_send_status(mmc, timeout); + return mmc_poll_for_busy(mmc, timeout); }
return ret; diff --git a/drivers/mmc/mmc_private.h b/drivers/mmc/mmc_private.h index f49b6eb573..35170d03ab 100644 --- a/drivers/mmc/mmc_private.h +++ b/drivers/mmc/mmc_private.h @@ -11,10 +11,11 @@
#include <mmc.h>
-extern int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, - struct mmc_data *data); -extern int mmc_send_status(struct mmc *mmc, int timeout); -extern int mmc_set_blocklen(struct mmc *mmc, int len); +int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data); +int mmc_send_status(struct mmc *mmc, unsigned int *status); +int mmc_poll_for_busy(struct mmc *mmc, int timeout); + +int mmc_set_blocklen(struct mmc *mmc, int len); #ifdef CONFIG_FSL_ESDHC_ADAPTER_IDENT void mmc_adapter_card_type_ident(void); #endif diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c index c8c83c9188..02648b0f50 100644 --- a/drivers/mmc/mmc_write.c +++ b/drivers/mmc/mmc_write.c @@ -119,7 +119,7 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t start, lbaint_t blkcnt) blk += blk_r;
/* Waiting for the ready status */ - if (mmc_send_status(mmc, timeout)) + if (mmc_poll_for_busy(mmc, timeout)) return 0; }
@@ -177,7 +177,7 @@ static ulong mmc_write_blocks(struct mmc *mmc, lbaint_t start, }
/* Waiting for the ready status */ - if (mmc_send_status(mmc, timeout)) + if (mmc_poll_for_busy(mmc, timeout)) return 0;
return blkcnt;
Using the DAT0 line as a rdy/busy line is an alternative to reading the status register of the card. It especially useful in situation where the bus is not in a good shape, like when modes are switched. This is also how the linux driver behaves.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/mmc/mmc-uclass.c | 2 -- drivers/mmc/mmc.c | 6 ++++-- include/mmc.h | 2 -- 3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 6742d99e3d..65d9da48f6 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -47,7 +47,6 @@ int mmc_set_ios(struct mmc *mmc) return dm_mmc_set_ios(mmc->dev); }
-#if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) int dm_mmc_wait_dat0(struct udevice *dev, int state, int timeout) { struct dm_mmc_ops *ops = mmc_get_ops(dev); @@ -61,7 +60,6 @@ int mmc_wait_dat0(struct mmc *mmc, int state, int timeout) { return dm_mmc_wait_dat0(mmc->dev, state, timeout); } -#endif
int dm_mmc_get_wp(struct udevice *dev) { diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index d4eed62926..89085868c6 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -29,12 +29,10 @@ static int mmc_select_mode_and_width(struct mmc *mmc, uint card_caps);
#if !CONFIG_IS_ENABLED(DM_MMC)
-#if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) static int mmc_wait_dat0(struct mmc *mmc, int state, int timeout) { return -ENOSYS; } -#endif
__weak int board_mmc_getwp(struct mmc *mmc) { @@ -233,6 +231,10 @@ int mmc_poll_for_busy(struct mmc *mmc, int timeout) unsigned int status; int err;
+ err = mmc_wait_dat0(mmc, 1, timeout); + if (err != -ENOSYS) + return err; + while (1) { err = mmc_send_status(mmc, &status); if (err) diff --git a/include/mmc.h b/include/mmc.h index 920f4cc53c..854778deed 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -441,7 +441,6 @@ struct dm_mmc_ops { int (*execute_tuning)(struct udevice *dev, uint opcode); #endif
-#if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) /** * wait_dat0() - wait until dat0 is in the target state * (CLK must be running during the wait) @@ -452,7 +451,6 @@ struct dm_mmc_ops { * @return 0 if dat0 is in the target state, -ve on error */ int (*wait_dat0)(struct udevice *dev, int state, int timeout); -#endif };
#define mmc_get_ops(dev) ((struct dm_mmc_ops *)(dev)->driver->ops)
On 6/27/19 12:31 PM, Jean-Jacques Hiblot wrote:
Using the DAT0 line as a rdy/busy line is an alternative to reading the status register of the card. It especially useful in situation where the bus is not in a good shape, like when modes are switched. This is also how the linux driver behaves.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com
Is this documented somewhere that this is an OK thing to do ? If the bus is not in a good shape, what guarantee do we have that DAT0 (part of the bus ;-) ) would be in good shape ?
On 28/06/2019 02:07, Marek Vasut wrote:
On 6/27/19 12:31 PM, Jean-Jacques Hiblot wrote:
Using the DAT0 line as a rdy/busy line is an alternative to reading the status register of the card. It especially useful in situation where the bus is not in a good shape, like when modes are switched. This is also how the linux driver behaves.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com
Is this documented somewhere that this is an OK thing to do ? If the bus is not in a good shape, what guarantee do we have that DAT0 (part of the bus ;-) ) would be in good shape ?
The spec states in "6.6.1 Command sets and extended settings": [...] The SWITCH command response is of type R1b, therefore, the host should read the Device status, using SEND_STATUS command, after the busy signal is de-asserted, to check the result of the SWITCH operation.[...]
Also this is how it is done in linux, except that instead of wait_dat0(), the host driver provides a card_busy() function.
I think that "not in good shape" probably means transients on the bus, maybe due to changes in the voltage levels. Looking at DAT0 should be fine because it is grounded while the card is busy.
The only trouble with using DAT0, is that the clock must run while waiting otherwise DAT0 may not be de-asserted, hence the encapsulation in the wait_dat0() so that the HW can disable automatic clock-gating before monitoring DAT0. Here is the relevant text from the spec: 6.7 "clock control" [...] The host is allowed to shut down the clock of a “busy” Device. The Device will complete the programming operation regardless of the host clock. However, the host must provide a clock edge for the Device to turn off its busy signal. Without a clock edge the Device (unless previously disconnected by a deselect command (CMD7)) will force the DAT0 line down, forever. [...]
On 6/28/19 10:28 AM, Jean-Jacques Hiblot wrote:
On 28/06/2019 02:07, Marek Vasut wrote:
On 6/27/19 12:31 PM, Jean-Jacques Hiblot wrote:
Using the DAT0 line as a rdy/busy line is an alternative to reading the status register of the card. It especially useful in situation where the bus is not in a good shape, like when modes are switched. This is also how the linux driver behaves.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com
Is this documented somewhere that this is an OK thing to do ? If the bus is not in a good shape, what guarantee do we have that DAT0 (part of the bus ;-) ) would be in good shape ?
The spec states in "6.6.1 Command sets and extended settings": [...] The SWITCH command response is of type R1b, therefore, the host should read the Device status, using SEND_STATUS command, after the busy signal is de-asserted, to check the result of the SWITCH operation.[...]
Also this is how it is done in linux, except that instead of wait_dat0(), the host driver provides a card_busy() function.
I think that "not in good shape" probably means transients on the bus, maybe due to changes in the voltage levels. Looking at DAT0 should be fine because it is grounded while the card is busy.
The only trouble with using DAT0, is that the clock must run while waiting otherwise DAT0 may not be de-asserted, hence the encapsulation in the wait_dat0() so that the HW can disable automatic clock-gating before monitoring DAT0. Here is the relevant text from the spec: 6.7 "clock control" [...] The host is allowed to shut down the clock of a “busy” Device. The Device will complete the programming operation regardless of the host clock. However, the host must provide a clock edge for the Device to turn off its busy signal. Without a clock edge the Device (unless previously disconnected by a deselect command (CMD7)) will force the DAT0 line down, forever. [...]
Ah, good, maybe this should be in the commit message?
Also, which version of the spec is that ?
On 28/06/2019 15:38, Marek Vasut wrote:
On 6/28/19 10:28 AM, Jean-Jacques Hiblot wrote:
On 28/06/2019 02:07, Marek Vasut wrote:
On 6/27/19 12:31 PM, Jean-Jacques Hiblot wrote:
Using the DAT0 line as a rdy/busy line is an alternative to reading the status register of the card. It especially useful in situation where the bus is not in a good shape, like when modes are switched. This is also how the linux driver behaves.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com
Is this documented somewhere that this is an OK thing to do ? If the bus is not in a good shape, what guarantee do we have that DAT0 (part of the bus ;-) ) would be in good shape ?
The spec states in "6.6.1 Command sets and extended settings": [...] The SWITCH command response is of type R1b, therefore, the host should read the Device status, using SEND_STATUS command, after the busy signal is de-asserted, to check the result of the SWITCH operation.[...]
Also this is how it is done in linux, except that instead of wait_dat0(), the host driver provides a card_busy() function.
I think that "not in good shape" probably means transients on the bus, maybe due to changes in the voltage levels. Looking at DAT0 should be fine because it is grounded while the card is busy.
The only trouble with using DAT0, is that the clock must run while waiting otherwise DAT0 may not be de-asserted, hence the encapsulation in the wait_dat0() so that the HW can disable automatic clock-gating before monitoring DAT0. Here is the relevant text from the spec: 6.7 "clock control" [...] The host is allowed to shut down the clock of a “busy” Device. The Device will complete the programming operation regardless of the host clock. However, the host must provide a clock edge for the Device to turn off its busy signal. Without a clock edge the Device (unless previously disconnected by a deselect command (CMD7)) will force the DAT0 line down, forever. [...]
Ah, good, maybe this should be in the commit message?
which part ? the clock stuff ?
Also, which version of the spec is that ?
I'm quoting from JESD84-B51A (rev 5.1A) but IFAIK rdy/busy on DAT0 has been there from the start.
On 6/28/19 4:26 PM, Jean-Jacques Hiblot wrote:
On 28/06/2019 15:38, Marek Vasut wrote:
On 6/28/19 10:28 AM, Jean-Jacques Hiblot wrote:
On 28/06/2019 02:07, Marek Vasut wrote:
On 6/27/19 12:31 PM, Jean-Jacques Hiblot wrote:
Using the DAT0 line as a rdy/busy line is an alternative to reading the status register of the card. It especially useful in situation where the bus is not in a good shape, like when modes are switched. This is also how the linux driver behaves.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com
Is this documented somewhere that this is an OK thing to do ? If the bus is not in a good shape, what guarantee do we have that DAT0 (part of the bus ;-) ) would be in good shape ?
The spec states in "6.6.1 Command sets and extended settings": [...] The SWITCH command response is of type R1b, therefore, the host should read the Device status, using SEND_STATUS command, after the busy signal is de-asserted, to check the result of the SWITCH operation.[...]
Also this is how it is done in linux, except that instead of wait_dat0(), the host driver provides a card_busy() function.
I think that "not in good shape" probably means transients on the bus, maybe due to changes in the voltage levels. Looking at DAT0 should be fine because it is grounded while the card is busy.
The only trouble with using DAT0, is that the clock must run while waiting otherwise DAT0 may not be de-asserted, hence the encapsulation in the wait_dat0() so that the HW can disable automatic clock-gating before monitoring DAT0. Here is the relevant text from the spec: 6.7 "clock control" [...] The host is allowed to shut down the clock of a “busy” Device. The Device will complete the programming operation regardless of the host clock. However, the host must provide a clock edge for the Device to turn off its busy signal. Without a clock edge the Device (unless previously disconnected by a deselect command (CMD7)) will force the DAT0 line down, forever. [...]
Ah, good, maybe this should be in the commit message?
which part ? the clock stuff ?
Also, which version of the spec is that ?
I'm quoting from JESD84-B51A (rev 5.1A) but IFAIK rdy/busy on DAT0 has been there from the start.
^ yes, that part should likely be in the commit message, this is really useful info which should be retained in the history IMO.
Starting with rev 4.5, the eMMC can define a generic timeout for the SWITCH command.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/mmc/mmc.c | 10 +++++++++- include/mmc.h | 2 ++ 2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 89085868c6..74590a5657 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -21,6 +21,8 @@ #include <div64.h> #include "mmc_private.h"
+#define DEFAULT_CMD6_TIMEOUT_MS 500 + static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage); static int mmc_power_cycle(struct mmc *mmc); #if !CONFIG_IS_ENABLED(MMC_TINY) @@ -745,10 +747,13 @@ static int __mmc_switch(struct mmc *mmc, u8 set, u8 index, u8 value, bool send_status) { struct mmc_cmd cmd; - int timeout = 1000; + int timeout = DEFAULT_CMD6_TIMEOUT_MS; int retries = 3; int ret;
+ if (mmc->gen_cmd6_time) + timeout = mmc->gen_cmd6_time * 10; + cmd.cmdidx = MMC_CMD_SWITCH; cmd.resp_type = MMC_RSP_R1b; cmd.cmdarg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) | @@ -2135,6 +2140,9 @@ static int mmc_startup_v4(struct mmc *mmc) mmc->capacity_user = capacity; }
+ if (mmc->version >= MMC_VERSION_4_5) + mmc->gen_cmd6_time = ext_csd[EXT_CSD_GENERIC_CMD6_TIME]; + /* The partition data may be non-zero but it is only * effective if PARTITION_SETTING_COMPLETED is set in * EXT_CSD, so ignore any data if this bit is not set, diff --git a/include/mmc.h b/include/mmc.h index 854778deed..8159ef1448 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -226,6 +226,7 @@ static inline bool mmc_is_tuning_cmd(uint cmdidx) #define EXT_CSD_HC_WP_GRP_SIZE 221 /* RO */ #define EXT_CSD_HC_ERASE_GRP_SIZE 224 /* RO */ #define EXT_CSD_BOOT_MULT 226 /* RO */ +#define EXT_CSD_GENERIC_CMD6_TIME 248 /* RO */ #define EXT_CSD_BKOPS_SUPPORT 502 /* RO */
/* @@ -581,6 +582,7 @@ struct mmc { u8 part_attr; u8 wr_rel_set; u8 part_config; + u8 gen_cmd6_time; uint tran_speed; uint legacy_speed; /* speed for the legacy mode provided by the card */ uint read_bl_len;
The e-MMC spec allows the e-MMC to specify a timeout for the partition switch command. It can take up to 2550 ms. There is no lower limit to this value in the spec, but do as the the linux driver does and force it to be at least 300ms.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/mmc/mmc.c | 10 ++++++++++ include/mmc.h | 5 +++++ 2 files changed, 15 insertions(+)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 74590a5657..0f77348681 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -748,12 +748,17 @@ static int __mmc_switch(struct mmc *mmc, u8 set, u8 index, u8 value, { struct mmc_cmd cmd; int timeout = DEFAULT_CMD6_TIMEOUT_MS; + bool is_part_switch = (set == EXT_CSD_CMD_SET_NORMAL) && + (index == EXT_CSD_PART_CONF); int retries = 3; int ret;
if (mmc->gen_cmd6_time) timeout = mmc->gen_cmd6_time * 10;
+ if (is_part_switch && mmc->part_switch_time) + timeout = mmc->part_switch_time * 10; + cmd.cmdidx = MMC_CMD_SWITCH; cmd.resp_type = MMC_RSP_R1b; cmd.cmdarg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) | @@ -2152,6 +2157,11 @@ static int mmc_startup_v4(struct mmc *mmc) part_completed = !!(ext_csd[EXT_CSD_PARTITION_SETTING] & EXT_CSD_PARTITION_SETTING_COMPLETED);
+ mmc->part_switch_time = ext_csd[EXT_CSD_PART_SWITCH_TIME]; + /* Some eMMC set the value too low so set a minimum */ + if (mmc->part_switch_time < MMC_MIN_PART_SWITCH_TIME && mmc->part_switch_time) + mmc->part_switch_time = MMC_MIN_PART_SWITCH_TIME; + /* store the partition info of emmc */ mmc->part_support = ext_csd[EXT_CSD_PARTITIONING_SUPPORT]; if ((ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & PART_SUPPORT) || diff --git a/include/mmc.h b/include/mmc.h index 8159ef1448..c199d1a0e8 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -222,6 +222,7 @@ static inline bool mmc_is_tuning_cmd(uint cmdidx) #define EXT_CSD_HS_TIMING 185 /* R/W */ #define EXT_CSD_REV 192 /* RO */ #define EXT_CSD_CARD_TYPE 196 /* RO */ +#define EXT_CSD_PART_SWITCH_TIME 199 /* RO */ #define EXT_CSD_SEC_CNT 212 /* RO, 4 bytes */ #define EXT_CSD_HC_WP_GRP_SIZE 221 /* RO */ #define EXT_CSD_HC_ERASE_GRP_SIZE 224 /* RO */ @@ -583,6 +584,7 @@ struct mmc { u8 wr_rel_set; u8 part_config; u8 gen_cmd6_time; + u8 part_switch_time; uint tran_speed; uint legacy_speed; /* speed for the legacy mode provided by the card */ uint read_bl_len; @@ -829,6 +831,9 @@ extern uint mmc_get_env_part(struct mmc *mmc); # endif int mmc_get_env_dev(void);
+/* Minimum partition switch timeout in units of 10-milliseconds */ +#define MMC_MIN_PART_SWITCH_TIME 30 /* 300 ms */ + /* Set block count limit because of 16 bit register limit on some hardware*/ #ifndef CONFIG_SYS_MMC_MAX_BLK_COUNT #define CONFIG_SYS_MMC_MAX_BLK_COUNT 65535
The switch operation can sometimes make the bus unreliable, in that case the send_status parameter should be false to indicate not to poll using CMD13. If polling on dat0 is possible, we should use it to detect the end of the operation. At the end of the operation it is safe to use CMD13 to get the status of the card. It is important to do so because the operation may have failed.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/mmc/mmc.c | 49 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 13 deletions(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 0f77348681..b0684596f9 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -746,6 +746,7 @@ static int mmc_send_ext_csd(struct mmc *mmc, u8 *ext_csd) static int __mmc_switch(struct mmc *mmc, u8 set, u8 index, u8 value, bool send_status) { + unsigned int status, start; struct mmc_cmd cmd; int timeout = DEFAULT_CMD6_TIMEOUT_MS; bool is_part_switch = (set == EXT_CSD_CMD_SET_NORMAL) && @@ -765,25 +766,47 @@ static int __mmc_switch(struct mmc *mmc, u8 set, u8 index, u8 value, (index << 16) | (value << 8);
- while (retries > 0) { + do { ret = mmc_send_cmd(mmc, &cmd, NULL); + } while (ret && retries-- > 0);
- if (ret) { - retries--; - continue; - } + if (ret) + return ret;
- if (!send_status) { - mdelay(50); - return 0; - } + start = get_timer(0);
- /* Waiting for the ready status */ - return mmc_poll_for_busy(mmc, timeout); - } + /* poll dat0 for rdy/buys status */ + ret = mmc_wait_dat0(mmc, 1, timeout); + if (ret && ret != -ENOSYS) + return ret;
- return ret; + /* + * In cases when not allowed to poll by using CMD13 or because we aren't + * capable of polling by using mmc_wait_dat0, then rely on waiting the + * stated timeout to be sufficient. + */ + if (ret == -ENOSYS && !send_status) + mdelay(timeout); + + /* Finally wait until the card is ready or indicates a failure + * to switch. It doesn't hurt to use CMD13 here even if send_status + * is false, because by now (after 'timeout' ms) the bus should be + * reliable. + */ + do { + ret = mmc_send_status(mmc, &status); + + if (!ret && (status & MMC_STATUS_SWITCH_ERROR)) { + pr_debug("switch failed %d/%d/0x%x !\n", set, index, + value); + return -EIO; + } + if (!ret && (status & MMC_STATUS_RDY_FOR_DATA)) + return 0; + udelay(100); + } while (get_timer(start) < timeout);
+ return -ETIMEDOUT; }
int mmc_switch(struct mmc *mmc, u8 set, u8 index, u8 value)
Accessing the boot partition had been error prone with HS200 and HS400 and was disabled. The driver first switched to a lesser mode and then switched the partition access. It was mostly due to a bad handling of the switch and has been fixed, so let's remove this 'feature'
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/mmc/mmc.c | 36 ------------------------------------ 1 file changed, 36 deletions(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index b0684596f9..709733747a 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -955,46 +955,10 @@ static int mmc_set_capacity(struct mmc *mmc, int part_num) return 0; }
-#if CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) -static int mmc_boot_part_access_chk(struct mmc *mmc, unsigned int part_num) -{ - int forbidden = 0; - bool change = false; - - if (part_num & PART_ACCESS_MASK) - forbidden = MMC_CAP(MMC_HS_200); - - if (MMC_CAP(mmc->selected_mode) & forbidden) { - pr_debug("selected mode (%s) is forbidden for part %d\n", - mmc_mode_name(mmc->selected_mode), part_num); - change = true; - } else if (mmc->selected_mode != mmc->best_mode) { - pr_debug("selected mode is not optimal\n"); - change = true; - } - - if (change) - return mmc_select_mode_and_width(mmc, - mmc->card_caps & ~forbidden); - - return 0; -} -#else -static inline int mmc_boot_part_access_chk(struct mmc *mmc, - unsigned int part_num) -{ - return 0; -} -#endif - int mmc_switch_part(struct mmc *mmc, unsigned int part_num) { int ret;
- ret = mmc_boot_part_access_chk(mmc, part_num); - if (ret) - return ret; - ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, (mmc->part_config & ~PART_ACCESS_MASK) | (part_num & PART_ACCESS_MASK));
This operation may fail. Retry it a few times before giving up and report a failure.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com
---
drivers/mmc/mmc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 709733747a..cec39a9acf 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -958,10 +958,14 @@ static int mmc_set_capacity(struct mmc *mmc, int part_num) int mmc_switch_part(struct mmc *mmc, unsigned int part_num) { int ret; + int retry = 3;
- ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, - (mmc->part_config & ~PART_ACCESS_MASK) - | (part_num & PART_ACCESS_MASK)); + do { + ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, + EXT_CSD_PART_CONF, + (mmc->part_config & ~PART_ACCESS_MASK) + | (part_num & PART_ACCESS_MASK)); + } while (ret && retry--);
/* * Set the capacity if the switch succeeded or was intended
Subject: [PATCH v1 11/11] mmc: retry a few times if a partition switch failed
This operation may fail. Retry it a few times before giving up and report a failure.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com
drivers/mmc/mmc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 709733747a..cec39a9acf 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -958,10 +958,14 @@ static int mmc_set_capacity(struct mmc *mmc, int part_num) int mmc_switch_part(struct mmc *mmc, unsigned int part_num) { int ret;
- int retry = 3;
- ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_PART_CONF,
(mmc->part_config & ~PART_ACCESS_MASK)| (part_num & PART_ACCESS_MASK));
do {
ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,EXT_CSD_PART_CONF,(mmc->part_config & ~PART_ACCESS_MASK)| (part_num & PART_ACCESS_MASK));} while (ret && retry--);
/*
- Set the capacity if the switch succeeded or was intended
--
Patchset applied to mmc/master. Please raise, if any objections.
Thanks, Peng.
2.17.1
participants (3)
-
Jean-Jacques Hiblot -
Marek Vasut -
Peng Fan