Re: [PATCH 1/3] mmc: core: activate pre-erased multiple write support for sd card

[Jan Glauber <jan.glauber@caviumnetworks.com>]

Hi Shawn,

I'm trying your patches on Cavium's ARM SOC. How do I get debug
messages from the MMC layer nowadays? Enabling CONFIG_MMC_DEBUG
does not show anything (4.14-rc2) but worked in the past.

thanks,
Jan

On Wed, Sep 27, 2017 at 11:40:36AM +0800, Shawn Lin wrote:
> Per SD specification version 4.10, section 4.3.4, it says "pre-erased
> (ACMD23) will make a following Multiple Block Write operation faster
> compared to the same operation without preceding ACMD23". ACMD23 is
> mandatory for all sd cards and the spec also says "Command STOP_TRAN
> (CMD12) shall be used to stop the transmission in Write Multiple Block
> whether or not the preerase (ACMD23) feature is used."
> 
> However current code only check to see if CMD23 is available. If not, it
> would fallback to open-ending mode. So this patch is trying to activate
> ACMD23 support for SD cards when doing multiple write.
> 
> The policy is:
> <1> If the sd card claims to support CMD23, the behaviour isn't
> changed. So we still use CMD23 for both of multiple block read and
> write as before.
> <2> If the sd card *doesn't* claims to support CMD23, we force all
> the multiple block write to use preceding ACMD23. The multiple read
> behaviour is changed since ACMD23 is for write only, per spec.
> <3> If the sd card *falsely* claims to support CMD23, and we add the
> quirk, MMC_QUIRK_BLK_NO_CMD23. The behaviour is the same as <2>. And
> one of the reports and test could be found at the bottom link.
> 
> Tested with the following cards:
> Apacer High Speed SDHC 8GB
> ADATA High Speed SDHC 4GB
> ADATA Premier UHS-I SDHC 16GB
> ez Share wifi-combo-SDHC 32GB
> Kingston UHS-I micro SDHC 32GB
> Toshiba High Speed micro SDHC 4GB
> Toshiba High Speed micro SDHC 8GB
> Toshiba Exceria UHS-I micro SDHC 32GB
> PNY High Speed micro SDHC 32GB
> Sandisk Ultra UHS-I micro SDHC 16GB
> Sandisk Extreme Pro UHS-I micro SDXC 512GB
> Samsung UHS-I micro SDHC 32GB
> Sony SFUX UHS-I SDHC 32GB
> Silicon Power High Speed SDHC 16GB
> Transcend UHS-I SDXC 64GB
> Topmore High Speed SDHC 8GB
> Longsys UHS-I micro SDHC 32GB
> Lexar Professional UHS-I micro SDHC 32GB
> .....
> 
> More cards have been tested but not need to list here. Some of them
> support CMD23 but some don't. All the test shows this patch work fine.
> 
> Link: https://patchwork.kernel.org/patch/9946125/
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/mmc/core/block.c | 50 ++++++++++++++++++++++++++++++++++++++++++------
>  drivers/mmc/core/core.c  |  6 ++++++
>  include/linux/mmc/core.h |  2 ++
>  3 files changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index ab9c780..b253d6a 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -101,8 +101,9 @@ struct mmc_blk_data {
>  	struct list_head rpmbs;
>  
>  	unsigned int	flags;
> -#define MMC_BLK_CMD23	(1 << 0)	/* Can do SET_BLOCK_COUNT for multiblock */
> -#define MMC_BLK_REL_WR	(1 << 1)	/* MMC Reliable write support */
> +#define MMC_BLK_CMD23	BIT(0)	/* Can do SET_BLOCK_COUNT for multiblock */
> +#define MMC_BLK_REL_WR	BIT(1)	/* MMC Reliable write support */
> +#define MMC_BLK_ACMD23	BIT(2)	/* Can do pre-erased before multiblock write */
>  
>  	unsigned int	usage;
>  	unsigned int	read_only;
> @@ -905,7 +906,8 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks)
>  }
>  
>  static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms,
> -		bool hw_busy_detect, struct request *req, bool *gen_err)
> +		bool hw_busy_detect, struct request *req, bool *gen_err,
> +		u32 *card_status)
>  {
>  	unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
>  	int err = 0;
> @@ -949,6 +951,9 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms,
>  	} while (!(status & R1_READY_FOR_DATA) ||
>  		 (R1_CURRENT_STATE(status) == R1_STATE_PRG));
>  
> +	if (card_status)
> +		*card_status = status;
> +
>  	return err;
>  }
>  
> @@ -994,7 +999,8 @@ static int send_stop(struct mmc_card *card, unsigned int timeout_ms,
>  		*gen_err = true;
>  	}
>  
> -	return card_busy_detect(card, timeout_ms, use_r1b_resp, req, gen_err);
> +	return card_busy_detect(card, timeout_ms, use_r1b_resp, req,
> +				gen_err, NULL);
>  }
>  
>  #define ERR_NOMEDIUM	3
> @@ -1520,6 +1526,7 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
>  	 */
>  	if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {
>  		int err;
> +		u32 stop_status;
>  
>  		/* Check stop command response */
>  		if (brq->stop.resp[0] & R1_ERROR) {
> @@ -1530,9 +1537,23 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
>  		}
>  
>  		err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, false, req,
> -					&gen_err);
> +					&gen_err, &stop_status);
>  		if (err)
>  			return MMC_BLK_CMD_ERR;
> +
> +		/*
> +		 * Some sd cards still leave in recv state after sending CMD12
> +		 * when preceding ACMD23 is used. But try again will fix that.
> +		 */
> +		if (brq->sbc.flags & MMC_CMD_SD_APP &&
> +		    R1_CURRENT_STATE(stop_status) == R1_STATE_RCV) {
> +			err = send_stop(card,
> +					DIV_ROUND_UP(brq->data.timeout_ns,
> +						     1000000),
> +					req, &gen_err, &stop_status);
> +			if (err)
> +				return MMC_BLK_CMD_ERR;
> +		}
>  	}
>  
>  	/* if general error occurs, retry the write operation. */
> @@ -1687,6 +1708,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>  	struct request *req = mmc_queue_req_to_req(mqrq);
>  	struct mmc_blk_data *md = mq->blkdata;
>  	bool do_rel_wr, do_data_tag;
> +	bool need_acmd23;
>  
>  	mmc_blk_data_prep(mq, mqrq, disable_multi, &do_rel_wr, &do_data_tag);
>  
> @@ -1727,11 +1749,22 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>  	 * that for hosts that don't expose MMC_CAP_CMD23, no
>  	 * change of behavior will be observed.
>  	 *
> +	 * Per sd spec, section 4.3.4, it suggests to use ACMD23
> +	 * do achieve better write performance for those who can't
> +	 * support CMD23. But also note that ACMD23 is for write only,
> +	 * so we still need use CMD23 to do multiple block data transfer
> +	 * if the card claims to support CMD23.
> +	 *
>  	 * N.B: Some MMC cards experience perf degradation.
>  	 * We'll avoid using CMD23-bounded multiblock writes for
>  	 * these, while retaining features like reliable writes.
>  	 */
> -	if ((md->flags & MMC_BLK_CMD23) && mmc_op_multi(brq->cmd.opcode) &&
> +	need_acmd23 = !(md->flags & MMC_BLK_CMD23) &&
> +		      brq->cmd.opcode == MMC_WRITE_MULTIPLE_BLOCK &&
> +		      md->flags & MMC_BLK_ACMD23;
> +
> +	if (((md->flags & MMC_BLK_CMD23) || need_acmd23) &&
> +	    mmc_op_multi(brq->cmd.opcode) &&
>  	    (do_rel_wr || !(card->quirks & MMC_QUIRK_BLK_NO_CMD23) ||
>  	     do_data_tag)) {
>  		brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
> @@ -1739,6 +1772,8 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>  			(do_rel_wr ? (1 << 31) : 0) |
>  			(do_data_tag ? (1 << 29) : 0);
>  		brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +		if (need_acmd23)
> +			brq->sbc.flags |= MMC_CMD_SD_APP;
>  		brq->mrq.sbc = &brq->sbc;
>  	}
>  
> @@ -2158,6 +2193,9 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>  		    (mmc_card_sd(card) &&
>  		     card->scr.cmds & SD_SCR_CMD23_SUPPORT))
>  			md->flags |= MMC_BLK_CMD23;
> +
> +		if (mmc_card_sd(card))
> +			md->flags |= MMC_BLK_ACMD23;
>  	}
>  
>  	if (mmc_card_mmc(card) &&
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 66c9cf4..bc96e6d 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -403,6 +403,12 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq)
>  
>  	mmc_wait_ongoing_tfr_cmd(host);
>  
> +	if (mrq && mrq->sbc && mrq->sbc->flags & MMC_CMD_SD_APP) {
> +		err = mmc_app_cmd(host, host->card);
> +		if (err)
> +			return err;
> +	}
> +
>  	mrq->done = mmc_wait_data_done;
>  	mrq->host = host;
>  
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 9275193..dcb5783 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -46,6 +46,8 @@ struct mmc_command {
>  #define MMC_CMD_BC	(2 << 5)
>  #define MMC_CMD_BCR	(3 << 5)
>  
> +#define MMC_CMD_SD_APP	(1 << 6)		/* app cmd for SD cards */
> +
>  #define MMC_RSP_SPI_S1	(1 << 7)		/* one status byte */
>  #define MMC_RSP_SPI_S2	(1 << 8)		/* second byte */
>  #define MMC_RSP_SPI_B4	(1 << 9)		/* four data bytes */
> -- 
> 1.9.1
>