Date:	Wed, 8 Jun 2016 14:46:33 +0000
From:	Alex Lemberg <Alex.Lemberg@...disk.com>
To:	Shawn Lin <shawn.lin@...k-chips.com>,
	Ulf Hansson <ulf.hansson@...aro.org>
CC:	Adrian Hunter <adrian.hunter@...el.com>,
	Jaehoon Chung <jh80.chung@...sung.com>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Doug Anderson <dianders@...omium.org>,
	"linux-rockchip@...ts.infradead.org" 
	<linux-rockchip@...ts.infradead.org>
Subject: Re: [PATCH] mmc: core: add auto bkops support

Hi Shawn,

Is the intention in this patch to stop using MANUAL_BKOPS for all eMMC5.1+ devices, and only use AUTO_BKOPS?
Please see several questions inline.


On 6/6/16, 6:07 AM, "linux-mmc-owner@...r.kernel.org on behalf of Shawn Lin" <linux-mmc-owner@...r.kernel.org on behalf of shawn.lin@...k-chips.com> wrote:

>JEDEC eMMC v5.1 introduce an autonomously initiated method
>for background operations.
>
>Host that wants to enable the device to perform background
>operations during device idle time, should signal the device
>by setting AUTO_EN in BKOPS_EN field EXT_CSD[163] to 1b. When
>this bit is set, the device may start or stop background operations
>whenever it sees fit, without any notification to the host.
>
>When AUTO_EN bit is set, the host should keep the device power
>active. The host may set or clear this bit at any time based on
>its power constraints or other considerations.
>
>Currently the manual bkops is only be used under the async req
>circumstances and it's a bit complicated to be controlled as the
>perfect method is that we should do some idle monitor just as rpm
>and send HPI each time if receiving rd/wr req. But it will impact
>performance significantly, especially for random iops since the
>weight of executing HPI against r/w small piece of LBAs is
>nonnegligible.
>
>So we now prefer to select the auto one unconditionally if supported
>which makes it as simple as possible. It should really good enough
>for devices to manage its internal policy for bkops rather than the
>host, which makes us believe that we could achieve the best
>performance for all the devices implementing auto bkops and the only
>thing we should do is to disable it when cutting off the power.
>
>Signed-off-by: Shawn Lin <shawn.lin@...k-chips.com>
>---
>
> drivers/mmc/core/core.c  | 127 +++++++++++++++++++++++++++++++++++++----------
> drivers/mmc/core/mmc.c   |  30 ++++++++++-
> include/linux/mmc/card.h |   1 +
> include/linux/mmc/core.h |   4 +-
> include/linux/mmc/mmc.h  |   1 +
> 5 files changed, 133 insertions(+), 30 deletions(-)
>
>diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>index e864187..c2e5a66 100644
>--- a/drivers/mmc/core/core.c
>+++ b/drivers/mmc/core/core.c
>@@ -297,24 +297,13 @@ static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> 	return 0;
> }
> 
>-/**
>- *	mmc_start_bkops - start BKOPS for supported cards
>- *	@card: MMC card to start BKOPS
>- *	@form_exception: A flag to indicate if this function was
>- *			 called due to an exception raised by the card
>- *
>- *	Start background operations whenever requested.
>- *	When the urgent BKOPS bit is set in a R1 command response
>- *	then background operations should be started immediately.
>-*/
>-void mmc_start_bkops(struct mmc_card *card, bool from_exception)
>+static void  mmc_start_man_bkops(struct mmc_card *card,
>+				 bool from_exception)
> {
> 	int err;
> 	int timeout;
> 	bool use_busy_signal;
> 
>-	BUG_ON(!card);
>-
> 	if (!card->ext_csd.man_bkops_en || mmc_card_doing_bkops(card))
> 		return;
> 
>@@ -347,7 +336,7 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
> 			EXT_CSD_BKOPS_START, 1, timeout,
> 			use_busy_signal, true, false);
> 	if (err) {
>-		pr_warn("%s: Error %d starting bkops\n",
>+		pr_warn("%s: Error %d starting manual bkops\n",
> 			mmc_hostname(card->host), err);
> 		mmc_retune_release(card->host);
> 		goto out;
>@@ -365,6 +354,55 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
> out:
> 	mmc_release_host(card->host);
> }
>+
>+static void  mmc_start_auto_bkops(struct mmc_card *card)
>+{
>+	int err;
>+
>+	/* If it's already enable auto_bkops, nothing to do */
>+	if (card->ext_csd.auto_bkops_en)
>+		return;
>+
>+	mmc_claim_host(card->host);
>+
>+	mmc_retune_hold(card->host);
>+
>+	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>+			EXT_CSD_BKOPS_EN, 2,
>+			card->ext_csd.generic_cmd6_time);
>+	if (err)
>+		pr_warn("%s: Error %d starting auto bkops\n",
>+			mmc_hostname(card->host), err);
>+
>+	card->ext_csd.auto_bkops_en = true;
>+	mmc_card_set_doing_bkops(card);
>+	mmc_retune_release(card->host);
>+	mmc_release_host(card->host);
>+}
>+
>+
>+/**
>+ *	mmc_start_bkops - start BKOPS for supported cards
>+ *	@card: MMC card to start BKOPS
>+ *	@form_exception: A flag to indicate if this function was
>+ *			 called due to an exception raised by the card
>+ *          @is_auto: A flag to indicate if we should use auto bkops
>+ *
>+ *	Start background operations whenever requested.
>+ *	When the urgent BKOPS bit is set in a R1 command response
>+ *	then background operations should be started immediately, which is
>+ *	only needed for man_bkops.
>+*/
>+void mmc_start_bkops(struct mmc_card *card, bool from_exception,
>+		     bool is_auto)
>+{
>+	BUG_ON(!card);
>+
>+	if (is_auto)

As far as I understand, the AUTO_BKOPS is enabled during “mmc_init_card()”.
If its correct, why the “mmc_start_auto_bkops()” function is needed?


>+		return mmc_start_auto_bkops(card);
>+	else
>+		return mmc_start_man_bkops(card, from_exception);
>+}
> EXPORT_SYMBOL(mmc_start_bkops);
> 
> /*
>@@ -610,7 +648,9 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
> 			if (areq)
> 				mmc_post_req(host, areq->mrq, -EINVAL);
> 
>-			mmc_start_bkops(host->card, true);
>+			/* Prefer to use auto bkops for eMMC 5.1 or later */

If the decision is to use AUTO_BKOPS for all eMMC5.1+ devices, why you need to enable AUTO_BKOPS bit
on every R1_EXCEPTION_EVENT?
AUTO_BKOPS should be enabled only once...


>+			mmc_start_bkops(host->card, true,
>+					host->card->ext_csd.rev >= 8);
> 
> 			/* prepare the request again */
> 			if (areq)
>@@ -751,20 +791,10 @@ int mmc_wait_for_cmd(struct mmc_host *host, struct mmc_command *cmd, int retries
> 
> EXPORT_SYMBOL(mmc_wait_for_cmd);
> 
>-/**
>- *	mmc_stop_bkops - stop ongoing BKOPS
>- *	@card: MMC card to check BKOPS
>- *
>- *	Send HPI command to stop ongoing background operations to
>- *	allow rapid servicing of foreground operations, e.g. read/
>- *	writes. Wait until the card comes out of the programming state
>- *	to avoid errors in servicing read/write requests.
>- */
>-int mmc_stop_bkops(struct mmc_card *card)
>+static int mmc_stop_man_bkops(struct mmc_card *card)
> {
> 	int err = 0;
> 
>-	BUG_ON(!card);
> 	err = mmc_interrupt_hpi(card);
> 
> 	/*
>@@ -779,6 +809,51 @@ int mmc_stop_bkops(struct mmc_card *card)
> 
> 	return err;
> }
>+
>+static int mmc_stop_auto_bkops(struct mmc_card *card)
>+{
>+	int err = 0;
>+
>+	if (!card->ext_csd.auto_bkops_en)
>+		return 0;
>+

Shouldn’t the BKOPS_STATUS be checked prior to disabling the BKOPS activity of the device?


>+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>+			EXT_CSD_BKOPS_EN, 0, MMC_BKOPS_MAX_TIMEOUT,
>+			true, true, false);
>+	if (err)
>+		pr_warn("%s: Error %d stoping auto bkops\n",
>+			mmc_hostname(card->host), err);
>+
>+	card->ext_csd.auto_bkops_en = false;
>+	mmc_card_clr_doing_bkops(card);
>+	mmc_retune_release(card->host);
>+
>+	return err;
>+}
>+
>+/**
>+ *	mmc_stop_bkops - stop ongoing BKOPS
>+ *	@card: MMC card to check BKOPS
>+ *	@is_auto: A flag to indicate if we should use auto bkops
>+ *
>+ *	Send HPI command to stop ongoing background operations to
>+ *	allow rapid servicing of foreground operations, e.g. read/
>+ *	writes. Wait until the card comes out of the programming state
>+ *	to avoid errors in servicing read/write requests.
>+ *
>+ *	But for auto bkops, we could only disable AUTO_EN instead of
>+ *	sending HPI. And the firmware could do it automatically.
>+ *
>+ */
>+int mmc_stop_bkops(struct mmc_card *card, bool is_auto)
>+{
>+	BUG_ON(!card);
>+
>+	if (is_auto)
>+		return mmc_stop_auto_bkops(card);
>+	else
>+		return mmc_stop_man_bkops(card);
>+}
> EXPORT_SYMBOL(mmc_stop_bkops);
> 
> int mmc_read_bkops_status(struct mmc_card *card)
>diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>index 61d6b34..c65bea7 100644
>--- a/drivers/mmc/core/mmc.c
>+++ b/drivers/mmc/core/mmc.c
>@@ -508,6 +508,13 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
> 		/* check whether the eMMC card supports BKOPS */
> 		if (ext_csd[EXT_CSD_BKOPS_SUPPORT] & 0x1) {
> 			card->ext_csd.bkops = 1;
>+
>+			/* for eMMC v5.1 or later, we could use auto_bkops */
>+			if (card->ext_csd.rev >= 8)
>+				card->ext_csd.auto_bkops_en =
>+					(ext_csd[EXT_CSD_BKOPS_EN] &
>+						EXT_CSD_AUTO_BKOPS_MASK);
>+
> 			card->ext_csd.man_bkops_en =
> 					(ext_csd[EXT_CSD_BKOPS_EN] &
> 						EXT_CSD_MANUAL_BKOPS_MASK);
>@@ -516,6 +523,9 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
> 			if (!card->ext_csd.man_bkops_en)
> 				pr_debug("%s: MAN_BKOPS_EN bit is not set\n",
> 					mmc_hostname(card->host));
>+			if (!card->ext_csd.auto_bkops_en)
>+				pr_debug("%s: AUTO_BKOPS_EN bit is not set\n",
>+					mmc_hostname(card->host));
> 		}
> 
> 		/* check whether the eMMC card supports HPI */
>@@ -1241,7 +1251,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
> 	}
> 
> 	err = mmc_select_bus_width(card);
>-	if (IS_ERR_VALUE(err))
>+	if (err < 0)
> 		goto out_err;
> 
> 	/* Switch card to HS mode */
>@@ -1676,6 +1686,21 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> 	mmc_select_powerclass(card);
> 
> 	/*
>+	 * Enable auto bkops feature which is mandatory for
>+	 * eMMC v5.1 or later
>+	 */
>+	if (card->ext_csd.rev >= 8) {
>+		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>+				 EXT_CSD_BKOPS_EN, 2,
>+				 card->ext_csd.generic_cmd6_time);
>+		if (err)
>+			pr_warn("%s: Error %d starting auto bkops\n",
>+				mmc_hostname(card->host), err);
>+
>+		card->ext_csd.auto_bkops_en = true;
>+	}
>+
>+	/*
> 	 * Enable HPI feature (if supported)
> 	 */
> 	if (card->ext_csd.hpi) {
>@@ -1898,7 +1923,8 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
> 		goto out;
> 
> 	if (mmc_card_doing_bkops(host->card)) {
>-		err = mmc_stop_bkops(host->card);
>+		err = mmc_stop_bkops(host->card,
>+				     host->card->ext_csd.rev >= 8);
> 		if (err)
> 			goto out;
> 	}
>diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>index 22defc2..f82f9be 100644
>--- a/include/linux/mmc/card.h
>+++ b/include/linux/mmc/card.h
>@@ -84,6 +84,7 @@ struct mmc_ext_csd {
> 	unsigned int		hpi_cmd;		/* cmd used as HPI */
> 	bool			bkops;		/* background support bit */
> 	bool			man_bkops_en;	/* manual bkops enable bit */
>+	bool			auto_bkops_en;	/* auto bkops enbale bit */
> 	unsigned int            data_sector_size;       /* 512 bytes or 4KB */
> 	unsigned int            data_tag_unit_size;     /* DATA TAG UNIT size */
> 	unsigned int		boot_ro_lock;		/* ro lock support */
>diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>index b01e77d..45d53ef 100644
>--- a/include/linux/mmc/core.h
>+++ b/include/linux/mmc/core.h
>@@ -140,7 +140,7 @@ struct mmc_request {
> struct mmc_card;
> struct mmc_async_req;
> 
>-extern int mmc_stop_bkops(struct mmc_card *);
>+extern int mmc_stop_bkops(struct mmc_card *, bool);
> extern int mmc_read_bkops_status(struct mmc_card *);
> extern struct mmc_async_req *mmc_start_req(struct mmc_host *,
> 					   struct mmc_async_req *, int *);
>@@ -150,7 +150,7 @@ extern int mmc_wait_for_cmd(struct mmc_host *, struct mmc_command *, int);
> extern int mmc_app_cmd(struct mmc_host *, struct mmc_card *);
> extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card *,
> 	struct mmc_command *, int);
>-extern void mmc_start_bkops(struct mmc_card *card, bool from_exception);
>+extern void mmc_start_bkops(struct mmc_card *, bool, bool);
> extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
> extern int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
> extern int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
>diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>index c376209..a1cced9 100644
>--- a/include/linux/mmc/mmc.h
>+++ b/include/linux/mmc/mmc.h
>@@ -436,6 +436,7 @@ struct _mmc_csd {
>  * BKOPS modes
>  */
> #define EXT_CSD_MANUAL_BKOPS_MASK	0x01
>+#define EXT_CSD_AUTO_BKOPS_MASK		0x02
> 
> /*
>  * MMC_SWITCH access modes
>-- 
>2.3.7
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>the body of a message to majordomo@...r.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists