[U-Boot] [PATCH] mmc: add bkops-enable command

[U-Boot] [PATCH] mmc: add bkops-enable command

[Tomas Melin]

Add new command for enabling the background operations
handshake functionality (BKOPS_EN, EXT_CSD byte [163])
on eMMC devices.

This is an optional feature of eMMCs, the setting is write-once.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 cmd/mmc.c         | 26 ++++++++++++++++++++++++++
 drivers/mmc/mmc.c | 30 ++++++++++++++++++++++++++++++
 include/mmc.h     |  4 ++++
 3 files changed, 60 insertions(+)

diff --git a/cmd/mmc.c b/cmd/mmc.c
index b2761e9..3ae9682 100644
--- a/cmd/mmc.c
+++ b/cmd/mmc.c
@@ -729,6 +729,29 @@ static int do_mmc_setdsr(cmd_tbl_t *cmdtp, int flag,
 	return ret;
 }
 
+static int do_mmc_bkops_enable(cmd_tbl_t *cmdtp, int flag,
+				   int argc, char * const argv[])
+{
+	int dev;
+	struct mmc *mmc;
+
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	dev = simple_strtoul(argv[1], NULL, 10);
+
+	mmc = init_mmc_device(dev, false);
+	if (!mmc)
+		return CMD_RET_FAILURE;
+
+	if (IS_SD(mmc)) {
+		puts("BKOPS_EN only exists on eMMC\n");
+		return CMD_RET_FAILURE;
+	}
+
+	return mmc_set_bkops_enable(mmc);
+}
+
 static cmd_tbl_t cmd_mmc[] = {
 	U_BOOT_CMD_MKENT(info, 1, 0, do_mmcinfo, "", ""),
 	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
@@ -749,6 +772,7 @@ static cmd_tbl_t cmd_mmc[] = {
 	U_BOOT_CMD_MKENT(rpmb, CONFIG_SYS_MAXARGS, 1, do_mmcrpmb, "", ""),
 #endif
 	U_BOOT_CMD_MKENT(setdsr, 2, 0, do_mmc_setdsr, "", ""),
+	U_BOOT_CMD_MKENT(bkops-enable, 2, 0, do_mmc_bkops_enable, "", ""),
 };
 
 static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
@@ -813,6 +837,8 @@ U_BOOT_CMD(
 	"mmc rpmb counter - read the value of the write counter\n"
 #endif
 	"mmc setdsr <value> - set DSR register value\n"
+	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
+	"   WARNING: This is a write-once setting.\n"
 	);
 
 /* Old command kept for compatibility. Same as 'mmc info' */
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 0cec02c..d6f40cc 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1810,3 +1810,33 @@ int mmc_initialize(bd_t *bis)
 	mmc_do_preinit();
 	return 0;
 }
+
+int mmc_set_bkops_enable(struct mmc *mmc)
+{
+	int err;
+	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
+
+	err = mmc_send_ext_csd(mmc, ext_csd);
+	if (err) {
+		puts("Could not get ext_csd register values\n");
+		return err;
+	}
+
+	if (!(ext_csd[EXT_CSD_BKOPS_SUPPORT] & 0x1)) {
+		puts("Background operations not supported on device\n");
+		return -EMEDIUMTYPE;
+	}
+
+	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
+		puts("Background operations already enabled\n");
+		return 0;
+	}
+
+	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
+	if (err) {
+		puts("Failed to enable background operations\n");
+		return err;
+	}
+
+	return 0;
+}
diff --git a/include/mmc.h b/include/mmc.h
index 5ef37d3..0772d53 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -175,6 +175,7 @@
 #define EXT_CSD_MAX_ENH_SIZE_MULT	157	/* R */
 #define EXT_CSD_PARTITIONING_SUPPORT	160	/* RO */
 #define EXT_CSD_RST_N_FUNCTION		162	/* R/W */
+#define EXT_CSD_BKOPS_EN		163	/* R/W */
 #define EXT_CSD_WR_REL_PARAM		166	/* R */
 #define EXT_CSD_WR_REL_SET		167	/* R/W */
 #define EXT_CSD_RPMB_MULT		168	/* RO */
@@ -189,6 +190,7 @@
 #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_BKOPS_SUPPORT		502	/* RO */
 
 /*
  * EXT_CSD field definitions
@@ -541,6 +543,8 @@ int mmc_rpmb_read(struct mmc *mmc, void *addr, unsigned short blk,
 		  unsigned short cnt, unsigned char *key);
 int mmc_rpmb_write(struct mmc *mmc, void *addr, unsigned short blk,
 		   unsigned short cnt, unsigned char *key);
+int mmc_set_bkops_enable(struct mmc *mmc);
+
 /**
  * Start device initialization and return immediately; it does not block on
  * polling OCR (operation condition register) status.  Then you should call
-- 
2.1.4

[U-Boot] mmc: add bkops-enable command

[Jaehoon Chung]

Hi,

On 11/16/2016 06:59 PM, tomas.melin at vaisala.com wrote:
> Add new command for enabling the background operations
> handshake functionality (BKOPS_EN, EXT_CSD byte [163])
> on eMMC devices.
> 
> This is an optional feature of eMMCs, the setting is write-once.

Why needs to set bkops on bootloader? Is there special reason?
And Linux kernel has already discussed about this.

I don't want to provide this command on u-boot side.
Don't handle Onetime programmable register on u-boot. So NACK.

Best Regards,
Jaehoon Chung


> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  cmd/mmc.c         | 26 ++++++++++++++++++++++++++
>  drivers/mmc/mmc.c | 30 ++++++++++++++++++++++++++++++
>  include/mmc.h     |  4 ++++
>  3 files changed, 60 insertions(+)
> 
> diff --git a/cmd/mmc.c b/cmd/mmc.c
> index b2761e9..3ae9682 100644
> --- a/cmd/mmc.c
> +++ b/cmd/mmc.c
> @@ -729,6 +729,29 @@ static int do_mmc_setdsr(cmd_tbl_t *cmdtp, int flag,
>  	return ret;
>  }
>  
> +static int do_mmc_bkops_enable(cmd_tbl_t *cmdtp, int flag,
> +				   int argc, char * const argv[])
> +{
> +	int dev;
> +	struct mmc *mmc;
> +
> +	if (argc != 2)
> +		return CMD_RET_USAGE;
> +
> +	dev = simple_strtoul(argv[1], NULL, 10);
> +
> +	mmc = init_mmc_device(dev, false);
> +	if (!mmc)
> +		return CMD_RET_FAILURE;
> +
> +	if (IS_SD(mmc)) {
> +		puts("BKOPS_EN only exists on eMMC\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	return mmc_set_bkops_enable(mmc);
> +}
> +
>  static cmd_tbl_t cmd_mmc[] = {
>  	U_BOOT_CMD_MKENT(info, 1, 0, do_mmcinfo, "", ""),
>  	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
> @@ -749,6 +772,7 @@ static cmd_tbl_t cmd_mmc[] = {
>  	U_BOOT_CMD_MKENT(rpmb, CONFIG_SYS_MAXARGS, 1, do_mmcrpmb, "", ""),
>  #endif
>  	U_BOOT_CMD_MKENT(setdsr, 2, 0, do_mmc_setdsr, "", ""),
> +	U_BOOT_CMD_MKENT(bkops-enable, 2, 0, do_mmc_bkops_enable, "", ""),
>  };
>  
>  static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> @@ -813,6 +837,8 @@ U_BOOT_CMD(
>  	"mmc rpmb counter - read the value of the write counter\n"
>  #endif
>  	"mmc setdsr <value> - set DSR register value\n"
> +	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
> +	"   WARNING: This is a write-once setting.\n"
>  	);
>  
>  /* Old command kept for compatibility. Same as 'mmc info' */
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 0cec02c..d6f40cc 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -1810,3 +1810,33 @@ int mmc_initialize(bd_t *bis)
>  	mmc_do_preinit();
>  	return 0;
>  }
> +
> +int mmc_set_bkops_enable(struct mmc *mmc)
> +{
> +	int err;
> +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
> +
> +	err = mmc_send_ext_csd(mmc, ext_csd);
> +	if (err) {
> +		puts("Could not get ext_csd register values\n");
> +		return err;
> +	}
> +
> +	if (!(ext_csd[EXT_CSD_BKOPS_SUPPORT] & 0x1)) {
> +		puts("Background operations not supported on device\n");
> +		return -EMEDIUMTYPE;
> +	}
> +
> +	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
> +		puts("Background operations already enabled\n");
> +		return 0;
> +	}
> +
> +	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
> +	if (err) {
> +		puts("Failed to enable background operations\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> diff --git a/include/mmc.h b/include/mmc.h
> index 5ef37d3..0772d53 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -175,6 +175,7 @@
>  #define EXT_CSD_MAX_ENH_SIZE_MULT	157	/* R */
>  #define EXT_CSD_PARTITIONING_SUPPORT	160	/* RO */
>  #define EXT_CSD_RST_N_FUNCTION		162	/* R/W */
> +#define EXT_CSD_BKOPS_EN		163	/* R/W */
>  #define EXT_CSD_WR_REL_PARAM		166	/* R */
>  #define EXT_CSD_WR_REL_SET		167	/* R/W */
>  #define EXT_CSD_RPMB_MULT		168	/* RO */
> @@ -189,6 +190,7 @@
>  #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_BKOPS_SUPPORT		502	/* RO */
>  
>  /*
>   * EXT_CSD field definitions
> @@ -541,6 +543,8 @@ int mmc_rpmb_read(struct mmc *mmc, void *addr, unsigned short blk,
>  		  unsigned short cnt, unsigned char *key);
>  int mmc_rpmb_write(struct mmc *mmc, void *addr, unsigned short blk,
>  		   unsigned short cnt, unsigned char *key);
> +int mmc_set_bkops_enable(struct mmc *mmc);
> +
>  /**
>   * Start device initialization and return immediately; it does not block on
>   * polling OCR (operation condition register) status.  Then you should call
> 
> 

[U-Boot] mmc: add bkops-enable command

[Tomas Melin]

Hi,

On 11/16/2016 02:05 PM, Jaehoon Chung wrote:
> 
> Why needs to set bkops on bootloader? Is there special reason?
> And Linux kernel has already discussed about this.
> 
It is beneficial to be able to do all required eMMC settings without being dependent on Linux booting.
It saves time in production to do initial eMMC setup directly from bootloader.

> I don't want to provide this command on u-boot side.
> Don't handle Onetime programmable register on u-boot. So NACK.

U-Boot already provides One-time programmable commands such as hwpartition and rst-function.
This adds to that same palette of commands that are needed when properly configuring an eMMC device. 

One possible option going forward could be putting all eMMC-configure related commands behind a common CONFIG_ option.

BR,
Tomas


> 
>>
>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>> ---
>>  cmd/mmc.c         | 26 ++++++++++++++++++++++++++
>>  drivers/mmc/mmc.c | 30 ++++++++++++++++++++++++++++++
>>  include/mmc.h     |  4 ++++
>>  3 files changed, 60 insertions(+)
>>
>> diff --git a/cmd/mmc.c b/cmd/mmc.c
>> index b2761e9..3ae9682 100644
>> --- a/cmd/mmc.c
>> +++ b/cmd/mmc.c
>> @@ -729,6 +729,29 @@ static int do_mmc_setdsr(cmd_tbl_t *cmdtp, int flag,
>>  	return ret;
>>  }
>>  
>> +static int do_mmc_bkops_enable(cmd_tbl_t *cmdtp, int flag,
>> +				   int argc, char * const argv[])
>> +{
>> +	int dev;
>> +	struct mmc *mmc;
>> +
>> +	if (argc != 2)
>> +		return CMD_RET_USAGE;
>> +
>> +	dev = simple_strtoul(argv[1], NULL, 10);
>> +
>> +	mmc = init_mmc_device(dev, false);
>> +	if (!mmc)
>> +		return CMD_RET_FAILURE;
>> +
>> +	if (IS_SD(mmc)) {
>> +		puts("BKOPS_EN only exists on eMMC\n");
>> +		return CMD_RET_FAILURE;
>> +	}
>> +
>> +	return mmc_set_bkops_enable(mmc);
>> +}
>> +
>>  static cmd_tbl_t cmd_mmc[] = {
>>  	U_BOOT_CMD_MKENT(info, 1, 0, do_mmcinfo, "", ""),
>>  	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
>> @@ -749,6 +772,7 @@ static cmd_tbl_t cmd_mmc[] = {
>>  	U_BOOT_CMD_MKENT(rpmb, CONFIG_SYS_MAXARGS, 1, do_mmcrpmb, "", ""),
>>  #endif
>>  	U_BOOT_CMD_MKENT(setdsr, 2, 0, do_mmc_setdsr, "", ""),
>> +	U_BOOT_CMD_MKENT(bkops-enable, 2, 0, do_mmc_bkops_enable, "", ""),
>>  };
>>  
>>  static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> @@ -813,6 +837,8 @@ U_BOOT_CMD(
>>  	"mmc rpmb counter - read the value of the write counter\n"
>>  #endif
>>  	"mmc setdsr <value> - set DSR register value\n"
>> +	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
>> +	"   WARNING: This is a write-once setting.\n"
>>  	);
>>  
>>  /* Old command kept for compatibility. Same as 'mmc info' */
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index 0cec02c..d6f40cc 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -1810,3 +1810,33 @@ int mmc_initialize(bd_t *bis)
>>  	mmc_do_preinit();
>>  	return 0;
>>  }
>> +
>> +int mmc_set_bkops_enable(struct mmc *mmc)
>> +{
>> +	int err;
>> +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
>> +
>> +	err = mmc_send_ext_csd(mmc, ext_csd);
>> +	if (err) {
>> +		puts("Could not get ext_csd register values\n");
>> +		return err;
>> +	}
>> +
>> +	if (!(ext_csd[EXT_CSD_BKOPS_SUPPORT] & 0x1)) {
>> +		puts("Background operations not supported on device\n");
>> +		return -EMEDIUMTYPE;
>> +	}
>> +
>> +	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
>> +		puts("Background operations already enabled\n");
>> +		return 0;
>> +	}
>> +
>> +	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
>> +	if (err) {
>> +		puts("Failed to enable background operations\n");
>> +		return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/include/mmc.h b/include/mmc.h
>> index 5ef37d3..0772d53 100644
>> --- a/include/mmc.h
>> +++ b/include/mmc.h
>> @@ -175,6 +175,7 @@
>>  #define EXT_CSD_MAX_ENH_SIZE_MULT	157	/* R */
>>  #define EXT_CSD_PARTITIONING_SUPPORT	160	/* RO */
>>  #define EXT_CSD_RST_N_FUNCTION		162	/* R/W */
>> +#define EXT_CSD_BKOPS_EN		163	/* R/W */
>>  #define EXT_CSD_WR_REL_PARAM		166	/* R */
>>  #define EXT_CSD_WR_REL_SET		167	/* R/W */
>>  #define EXT_CSD_RPMB_MULT		168	/* RO */
>> @@ -189,6 +190,7 @@
>>  #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_BKOPS_SUPPORT		502	/* RO */
>>  
>>  /*
>>   * EXT_CSD field definitions
>> @@ -541,6 +543,8 @@ int mmc_rpmb_read(struct mmc *mmc, void *addr, unsigned short blk,
>>  		  unsigned short cnt, unsigned char *key);
>>  int mmc_rpmb_write(struct mmc *mmc, void *addr, unsigned short blk,
>>  		   unsigned short cnt, unsigned char *key);
>> +int mmc_set_bkops_enable(struct mmc *mmc);
>> +
>>  /**
>>   * Start device initialization and return immediately; it does not block on
>>   * polling OCR (operation condition register) status.  Then you should call
>>
>>

[U-Boot] mmc: add bkops-enable command

[Jaehoon Chung]

On 11/16/2016 10:12 PM, Tomas Melin wrote:
> Hi,
> 
> On 11/16/2016 02:05 PM, Jaehoon Chung wrote:
>>
>> Why needs to set bkops on bootloader? Is there special reason?
>> And Linux kernel has already discussed about this.
>>
> It is beneficial to be able to do all required eMMC settings without being dependent on Linux booting.
> It saves time in production to do initial eMMC setup directly from bootloader.
> 
>> I don't want to provide this command on u-boot side.
>> Don't handle Onetime programmable register on u-boot. So NACK.
> 
> U-Boot already provides One-time programmable commands such as hwpartition and rst-function.
> This adds to that same palette of commands that are needed when properly configuring an eMMC device. 

hwpartition and rst-function didn't affect the performance. And it's not critical thing.
But BKOPS needs to consider too many things. 
Just even set to enable bit..it's not working.

Did you check the periodic bkops and auto bkops? And is it working correct?
How to control suspend and Idle state? LEVEL2/3?

When i had tested the bkops, it has affected IO performance...Of course, it should be maintained average.
Because it should be doing GC with bkops. That is not reason that has to enable on bootloader.

I will not apply on u-boot-mmc for bkops. There is no benefit.

If you set to enable bkops, then you did to pass the other controlling responsibility to Kernel. 

Best Regards,
Jaehoon Chung

> 
> One possible option going forward could be putting all eMMC-configure related commands behind a common CONFIG_ option.
> 
> BR,
> Tomas
> 
> 
>>
>>>
>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>> ---
>>>  cmd/mmc.c         | 26 ++++++++++++++++++++++++++
>>>  drivers/mmc/mmc.c | 30 ++++++++++++++++++++++++++++++
>>>  include/mmc.h     |  4 ++++
>>>  3 files changed, 60 insertions(+)
>>>
>>> diff --git a/cmd/mmc.c b/cmd/mmc.c
>>> index b2761e9..3ae9682 100644
>>> --- a/cmd/mmc.c
>>> +++ b/cmd/mmc.c
>>> @@ -729,6 +729,29 @@ static int do_mmc_setdsr(cmd_tbl_t *cmdtp, int flag,
>>>  	return ret;
>>>  }
>>>  
>>> +static int do_mmc_bkops_enable(cmd_tbl_t *cmdtp, int flag,
>>> +				   int argc, char * const argv[])
>>> +{
>>> +	int dev;
>>> +	struct mmc *mmc;
>>> +
>>> +	if (argc != 2)
>>> +		return CMD_RET_USAGE;
>>> +
>>> +	dev = simple_strtoul(argv[1], NULL, 10);
>>> +
>>> +	mmc = init_mmc_device(dev, false);
>>> +	if (!mmc)
>>> +		return CMD_RET_FAILURE;
>>> +
>>> +	if (IS_SD(mmc)) {
>>> +		puts("BKOPS_EN only exists on eMMC\n");
>>> +		return CMD_RET_FAILURE;
>>> +	}
>>> +
>>> +	return mmc_set_bkops_enable(mmc);
>>> +}
>>> +
>>>  static cmd_tbl_t cmd_mmc[] = {
>>>  	U_BOOT_CMD_MKENT(info, 1, 0, do_mmcinfo, "", ""),
>>>  	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
>>> @@ -749,6 +772,7 @@ static cmd_tbl_t cmd_mmc[] = {
>>>  	U_BOOT_CMD_MKENT(rpmb, CONFIG_SYS_MAXARGS, 1, do_mmcrpmb, "", ""),
>>>  #endif
>>>  	U_BOOT_CMD_MKENT(setdsr, 2, 0, do_mmc_setdsr, "", ""),
>>> +	U_BOOT_CMD_MKENT(bkops-enable, 2, 0, do_mmc_bkops_enable, "", ""),
>>>  };
>>>  
>>>  static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>> @@ -813,6 +837,8 @@ U_BOOT_CMD(
>>>  	"mmc rpmb counter - read the value of the write counter\n"
>>>  #endif
>>>  	"mmc setdsr <value> - set DSR register value\n"
>>> +	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
>>> +	"   WARNING: This is a write-once setting.\n"
>>>  	);
>>>  
>>>  /* Old command kept for compatibility. Same as 'mmc info' */
>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>> index 0cec02c..d6f40cc 100644
>>> --- a/drivers/mmc/mmc.c
>>> +++ b/drivers/mmc/mmc.c
>>> @@ -1810,3 +1810,33 @@ int mmc_initialize(bd_t *bis)
>>>  	mmc_do_preinit();
>>>  	return 0;
>>>  }
>>> +
>>> +int mmc_set_bkops_enable(struct mmc *mmc)
>>> +{
>>> +	int err;
>>> +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
>>> +
>>> +	err = mmc_send_ext_csd(mmc, ext_csd);
>>> +	if (err) {
>>> +		puts("Could not get ext_csd register values\n");
>>> +		return err;
>>> +	}
>>> +
>>> +	if (!(ext_csd[EXT_CSD_BKOPS_SUPPORT] & 0x1)) {
>>> +		puts("Background operations not supported on device\n");
>>> +		return -EMEDIUMTYPE;
>>> +	}
>>> +
>>> +	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
>>> +		puts("Background operations already enabled\n");
>>> +		return 0;
>>> +	}
>>> +
>>> +	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
>>> +	if (err) {
>>> +		puts("Failed to enable background operations\n");
>>> +		return err;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> diff --git a/include/mmc.h b/include/mmc.h
>>> index 5ef37d3..0772d53 100644
>>> --- a/include/mmc.h
>>> +++ b/include/mmc.h
>>> @@ -175,6 +175,7 @@
>>>  #define EXT_CSD_MAX_ENH_SIZE_MULT	157	/* R */
>>>  #define EXT_CSD_PARTITIONING_SUPPORT	160	/* RO */
>>>  #define EXT_CSD_RST_N_FUNCTION		162	/* R/W */
>>> +#define EXT_CSD_BKOPS_EN		163	/* R/W */
>>>  #define EXT_CSD_WR_REL_PARAM		166	/* R */
>>>  #define EXT_CSD_WR_REL_SET		167	/* R/W */
>>>  #define EXT_CSD_RPMB_MULT		168	/* RO */
>>> @@ -189,6 +190,7 @@
>>>  #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_BKOPS_SUPPORT		502	/* RO */
>>>  
>>>  /*
>>>   * EXT_CSD field definitions
>>> @@ -541,6 +543,8 @@ int mmc_rpmb_read(struct mmc *mmc, void *addr, unsigned short blk,
>>>  		  unsigned short cnt, unsigned char *key);
>>>  int mmc_rpmb_write(struct mmc *mmc, void *addr, unsigned short blk,
>>>  		   unsigned short cnt, unsigned char *key);
>>> +int mmc_set_bkops_enable(struct mmc *mmc);
>>> +
>>>  /**
>>>   * Start device initialization and return immediately; it does not block on
>>>   * polling OCR (operation condition register) status.  Then you should call
>>>
>>>

[U-Boot] mmc: add bkops-enable command

[Tomas Melin]

Hi,

Thank you for your valuable comments and for taking the time to respond.
Please see below for further comments.

On 11/16/2016 03:39 PM, Jaehoon Chung wrote:
>  
> On 11/16/2016 10:12 PM, Tomas Melin wrote:
>> Hi,
>>
>> On 11/16/2016 02:05 PM, Jaehoon Chung wrote:
>>>
>>> Why needs to set bkops on bootloader? Is there special reason?
>>> And Linux kernel has already discussed about this.
>>>
>> It is beneficial to be able to do all required eMMC settings without being dependent on Linux booting.
>> It saves time in production to do initial eMMC setup directly from bootloader.
>>
>>> I don't want to provide this command on u-boot side.
>>> Don't handle Onetime programmable register on u-boot. So NACK.
>>
>> U-Boot already provides One-time programmable commands such as hwpartition and rst-function.
>> This adds to that same palette of commands that are needed when properly configuring an eMMC device. 
> 
> hwpartition and rst-function didn't affect the performance. And it's not critical thing.
> But BKOPS needs to consider too many things. 
> Just even set to enable bit..it's not working.

Sorry, but I don't see how this patch affects performance? This only adds a command (mmc bkops enable) that gives the user 
a _possibility_ to enable bkops in the eMMC. The choise whether or not to enable bkops is still left to the user.

> 
> Did you check the periodic bkops and auto bkops? And is it working correct?
> How to control suspend and Idle state? LEVEL2/3?
> 
> When i had tested the bkops, it has affected IO performance...Of course, it should be maintained average.
> Because it should be doing GC with bkops. That is not reason that has to enable on bootloader.

The reason to have this command in U-Boot is to be able to do _all_ configuration settings for the eMMC 
during manufacturing, prior to flashing the device. Thus, all settings are done only once from U-Boot.
In normal operation, the OS driver will be responsible for bkops handling.

BR,
Tomas

> 
> I will not apply on u-boot-mmc for bkops. There is no benefit.
> 
> If you set to enable bkops, then you did to pass the other controlling responsibility to Kernel. 
> 
> Best Regards,
> Jaehoon Chung
> 
>>
>> One possible option going forward could be putting all eMMC-configure related commands behind a common CONFIG_ option.
>>
>> BR,
>> Tomas
>>
>>
>>>
>>>>
>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>> ---
>>>>  cmd/mmc.c         | 26 ++++++++++++++++++++++++++
>>>>  drivers/mmc/mmc.c | 30 ++++++++++++++++++++++++++++++
>>>>  include/mmc.h     |  4 ++++
>>>>  3 files changed, 60 insertions(+)
>>>>
>>>> diff --git a/cmd/mmc.c b/cmd/mmc.c
>>>> index b2761e9..3ae9682 100644
>>>> --- a/cmd/mmc.c
>>>> +++ b/cmd/mmc.c
>>>> @@ -729,6 +729,29 @@ static int do_mmc_setdsr(cmd_tbl_t *cmdtp, int flag,
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +static int do_mmc_bkops_enable(cmd_tbl_t *cmdtp, int flag,
>>>> +				   int argc, char * const argv[])
>>>> +{
>>>> +	int dev;
>>>> +	struct mmc *mmc;
>>>> +
>>>> +	if (argc != 2)
>>>> +		return CMD_RET_USAGE;
>>>> +
>>>> +	dev = simple_strtoul(argv[1], NULL, 10);
>>>> +
>>>> +	mmc = init_mmc_device(dev, false);
>>>> +	if (!mmc)
>>>> +		return CMD_RET_FAILURE;
>>>> +
>>>> +	if (IS_SD(mmc)) {
>>>> +		puts("BKOPS_EN only exists on eMMC\n");
>>>> +		return CMD_RET_FAILURE;
>>>> +	}
>>>> +
>>>> +	return mmc_set_bkops_enable(mmc);
>>>> +}
>>>> +
>>>>  static cmd_tbl_t cmd_mmc[] = {
>>>>  	U_BOOT_CMD_MKENT(info, 1, 0, do_mmcinfo, "", ""),
>>>>  	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
>>>> @@ -749,6 +772,7 @@ static cmd_tbl_t cmd_mmc[] = {
>>>>  	U_BOOT_CMD_MKENT(rpmb, CONFIG_SYS_MAXARGS, 1, do_mmcrpmb, "", ""),
>>>>  #endif
>>>>  	U_BOOT_CMD_MKENT(setdsr, 2, 0, do_mmc_setdsr, "", ""),
>>>> +	U_BOOT_CMD_MKENT(bkops-enable, 2, 0, do_mmc_bkops_enable, "", ""),
>>>>  };
>>>>  
>>>>  static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>> @@ -813,6 +837,8 @@ U_BOOT_CMD(
>>>>  	"mmc rpmb counter - read the value of the write counter\n"
>>>>  #endif
>>>>  	"mmc setdsr <value> - set DSR register value\n"
>>>> +	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
>>>> +	"   WARNING: This is a write-once setting.\n"
>>>>  	);
>>>>  
>>>>  /* Old command kept for compatibility. Same as 'mmc info' */
>>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>>> index 0cec02c..d6f40cc 100644
>>>> --- a/drivers/mmc/mmc.c
>>>> +++ b/drivers/mmc/mmc.c
>>>> @@ -1810,3 +1810,33 @@ int mmc_initialize(bd_t *bis)
>>>>  	mmc_do_preinit();
>>>>  	return 0;
>>>>  }
>>>> +
>>>> +int mmc_set_bkops_enable(struct mmc *mmc)
>>>> +{
>>>> +	int err;
>>>> +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
>>>> +
>>>> +	err = mmc_send_ext_csd(mmc, ext_csd);
>>>> +	if (err) {
>>>> +		puts("Could not get ext_csd register values\n");
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	if (!(ext_csd[EXT_CSD_BKOPS_SUPPORT] & 0x1)) {
>>>> +		puts("Background operations not supported on device\n");
>>>> +		return -EMEDIUMTYPE;
>>>> +	}
>>>> +
>>>> +	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
>>>> +		puts("Background operations already enabled\n");
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
>>>> +	if (err) {
>>>> +		puts("Failed to enable background operations\n");
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> diff --git a/include/mmc.h b/include/mmc.h
>>>> index 5ef37d3..0772d53 100644
>>>> --- a/include/mmc.h
>>>> +++ b/include/mmc.h
>>>> @@ -175,6 +175,7 @@
>>>>  #define EXT_CSD_MAX_ENH_SIZE_MULT	157	/* R */
>>>>  #define EXT_CSD_PARTITIONING_SUPPORT	160	/* RO */
>>>>  #define EXT_CSD_RST_N_FUNCTION		162	/* R/W */
>>>> +#define EXT_CSD_BKOPS_EN		163	/* R/W */
>>>>  #define EXT_CSD_WR_REL_PARAM		166	/* R */
>>>>  #define EXT_CSD_WR_REL_SET		167	/* R/W */
>>>>  #define EXT_CSD_RPMB_MULT		168	/* RO */
>>>> @@ -189,6 +190,7 @@
>>>>  #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_BKOPS_SUPPORT		502	/* RO */
>>>>  
>>>>  /*
>>>>   * EXT_CSD field definitions
>>>> @@ -541,6 +543,8 @@ int mmc_rpmb_read(struct mmc *mmc, void *addr, unsigned short blk,
>>>>  		  unsigned short cnt, unsigned char *key);
>>>>  int mmc_rpmb_write(struct mmc *mmc, void *addr, unsigned short blk,
>>>>  		   unsigned short cnt, unsigned char *key);
>>>> +int mmc_set_bkops_enable(struct mmc *mmc);
>>>> +
>>>>  /**
>>>>   * Start device initialization and return immediately; it does not block on
>>>>   * polling OCR (operation condition register) status.  Then you should call
>>>>
>>>>

[U-Boot] mmc: add bkops-enable command

[Jaehoon Chung]

Hi Tomas,

On 11/17/2016 08:05 PM, Tomas Melin wrote:
> Hi,
> 
> Thank you for your valuable comments and for taking the time to respond.
> Please see below for further comments.

No problem, It's useful to discuss about something. :)

> 
> On 11/16/2016 03:39 PM, Jaehoon Chung wrote:
>>  
>> On 11/16/2016 10:12 PM, Tomas Melin wrote:
>>> Hi,
>>>
>>> On 11/16/2016 02:05 PM, Jaehoon Chung wrote:
>>>>
>>>> Why needs to set bkops on bootloader? Is there special reason?
>>>> And Linux kernel has already discussed about this.
>>>>
>>> It is beneficial to be able to do all required eMMC settings without being dependent on Linux booting.
>>> It saves time in production to do initial eMMC setup directly from bootloader.
>>>
>>>> I don't want to provide this command on u-boot side.
>>>> Don't handle Onetime programmable register on u-boot. So NACK.
>>>
>>> U-Boot already provides One-time programmable commands such as hwpartition and rst-function.
>>> This adds to that same palette of commands that are needed when properly configuring an eMMC device. 
>>
>> hwpartition and rst-function didn't affect the performance. And it's not critical thing.
>> But BKOPS needs to consider too many things. 
>> Just even set to enable bit..it's not working.
> 
> Sorry, but I don't see how this patch affects performance? This only adds a command (mmc bkops enable) that gives the user 
> a _possibility_ to enable bkops in the eMMC. The choise whether or not to enable bkops is still left to the user.

You're right. This patch doesn't affect performance, because this patch just provides to enable bkops, right?
My means is "i don't want to provide the command for enabling BKOPS.".

There is no information on u-boot whether kernel can fully support the BKOPS functionality or not.
Do you know how to run bkops on kernel side when bkops is enabled from u-boot?

As i know, there are discussions about enabling BKOPS at Kernel mmc mailing.
Even when i had implemented the BKOPS feature, i put the some flag for enabling BKOPS. (likes MMC_CAP2_ENABLE_BKOPS)
It can be also chosen from USER with dt property. but Finally we removed this capability.

1) When BKOPS is enabled, needs to consider IO performance.
2) There is no periodic BKOPS functionality.
3) eMMC5.1 has introduced new automatic bkops feature..BIT[1] ATUO_EN.
(Also not apply on linux-kernel.)
4) mmc-util supports all configuration on kernel side. 

If it has to enable this, i think better that leaves a matter in Kernel.

> 
>>
>> Did you check the periodic bkops and auto bkops? And is it working correct?
>> How to control suspend and Idle state? LEVEL2/3?
>>
>> When i had tested the bkops, it has affected IO performance...Of course, it should be maintained average.
>> Because it should be doing GC with bkops. That is not reason that has to enable on bootloader.
> 
> The reason to have this command in U-Boot is to be able to do _all_ configuration settings for the eMMC 
> during manufacturing, prior to flashing the device. Thus, all settings are done only once from U-Boot.
> In normal operation, the OS driver will be responsible for bkops handling.

_all_ configuration? i don't agree..We don't need to put _all_ configuration command on u-boot side.

Best Regards,
Jaehoon Chung

> 
> BR,
> Tomas
> 
>>
>> I will not apply on u-boot-mmc for bkops. There is no benefit.
>>
>> If you set to enable bkops, then you did to pass the other controlling responsibility to Kernel. 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>> One possible option going forward could be putting all eMMC-configure related commands behind a common CONFIG_ option.
>>>
>>> BR,
>>> Tomas
>>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>> ---
>>>>>  cmd/mmc.c         | 26 ++++++++++++++++++++++++++
>>>>>  drivers/mmc/mmc.c | 30 ++++++++++++++++++++++++++++++
>>>>>  include/mmc.h     |  4 ++++
>>>>>  3 files changed, 60 insertions(+)
>>>>>
>>>>> diff --git a/cmd/mmc.c b/cmd/mmc.c
>>>>> index b2761e9..3ae9682 100644
>>>>> --- a/cmd/mmc.c
>>>>> +++ b/cmd/mmc.c
>>>>> @@ -729,6 +729,29 @@ static int do_mmc_setdsr(cmd_tbl_t *cmdtp, int flag,
>>>>>  	return ret;
>>>>>  }
>>>>>  
>>>>> +static int do_mmc_bkops_enable(cmd_tbl_t *cmdtp, int flag,
>>>>> +				   int argc, char * const argv[])
>>>>> +{
>>>>> +	int dev;
>>>>> +	struct mmc *mmc;
>>>>> +
>>>>> +	if (argc != 2)
>>>>> +		return CMD_RET_USAGE;
>>>>> +
>>>>> +	dev = simple_strtoul(argv[1], NULL, 10);
>>>>> +
>>>>> +	mmc = init_mmc_device(dev, false);
>>>>> +	if (!mmc)
>>>>> +		return CMD_RET_FAILURE;
>>>>> +
>>>>> +	if (IS_SD(mmc)) {
>>>>> +		puts("BKOPS_EN only exists on eMMC\n");
>>>>> +		return CMD_RET_FAILURE;
>>>>> +	}
>>>>> +
>>>>> +	return mmc_set_bkops_enable(mmc);
>>>>> +}
>>>>> +
>>>>>  static cmd_tbl_t cmd_mmc[] = {
>>>>>  	U_BOOT_CMD_MKENT(info, 1, 0, do_mmcinfo, "", ""),
>>>>>  	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
>>>>> @@ -749,6 +772,7 @@ static cmd_tbl_t cmd_mmc[] = {
>>>>>  	U_BOOT_CMD_MKENT(rpmb, CONFIG_SYS_MAXARGS, 1, do_mmcrpmb, "", ""),
>>>>>  #endif
>>>>>  	U_BOOT_CMD_MKENT(setdsr, 2, 0, do_mmc_setdsr, "", ""),
>>>>> +	U_BOOT_CMD_MKENT(bkops-enable, 2, 0, do_mmc_bkops_enable, "", ""),
>>>>>  };
>>>>>  
>>>>>  static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>> @@ -813,6 +837,8 @@ U_BOOT_CMD(
>>>>>  	"mmc rpmb counter - read the value of the write counter\n"
>>>>>  #endif
>>>>>  	"mmc setdsr <value> - set DSR register value\n"
>>>>> +	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
>>>>> +	"   WARNING: This is a write-once setting.\n"
>>>>>  	);
>>>>>  
>>>>>  /* Old command kept for compatibility. Same as 'mmc info' */
>>>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>>>> index 0cec02c..d6f40cc 100644
>>>>> --- a/drivers/mmc/mmc.c
>>>>> +++ b/drivers/mmc/mmc.c
>>>>> @@ -1810,3 +1810,33 @@ int mmc_initialize(bd_t *bis)
>>>>>  	mmc_do_preinit();
>>>>>  	return 0;
>>>>>  }
>>>>> +
>>>>> +int mmc_set_bkops_enable(struct mmc *mmc)
>>>>> +{
>>>>> +	int err;
>>>>> +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
>>>>> +
>>>>> +	err = mmc_send_ext_csd(mmc, ext_csd);
>>>>> +	if (err) {
>>>>> +		puts("Could not get ext_csd register values\n");
>>>>> +		return err;
>>>>> +	}
>>>>> +
>>>>> +	if (!(ext_csd[EXT_CSD_BKOPS_SUPPORT] & 0x1)) {
>>>>> +		puts("Background operations not supported on device\n");
>>>>> +		return -EMEDIUMTYPE;
>>>>> +	}
>>>>> +
>>>>> +	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
>>>>> +		puts("Background operations already enabled\n");
>>>>> +		return 0;
>>>>> +	}
>>>>> +
>>>>> +	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
>>>>> +	if (err) {
>>>>> +		puts("Failed to enable background operations\n");
>>>>> +		return err;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> diff --git a/include/mmc.h b/include/mmc.h
>>>>> index 5ef37d3..0772d53 100644
>>>>> --- a/include/mmc.h
>>>>> +++ b/include/mmc.h
>>>>> @@ -175,6 +175,7 @@
>>>>>  #define EXT_CSD_MAX_ENH_SIZE_MULT	157	/* R */
>>>>>  #define EXT_CSD_PARTITIONING_SUPPORT	160	/* RO */
>>>>>  #define EXT_CSD_RST_N_FUNCTION		162	/* R/W */
>>>>> +#define EXT_CSD_BKOPS_EN		163	/* R/W */
>>>>>  #define EXT_CSD_WR_REL_PARAM		166	/* R */
>>>>>  #define EXT_CSD_WR_REL_SET		167	/* R/W */
>>>>>  #define EXT_CSD_RPMB_MULT		168	/* RO */
>>>>> @@ -189,6 +190,7 @@
>>>>>  #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_BKOPS_SUPPORT		502	/* RO */
>>>>>  
>>>>>  /*
>>>>>   * EXT_CSD field definitions
>>>>> @@ -541,6 +543,8 @@ int mmc_rpmb_read(struct mmc *mmc, void *addr, unsigned short blk,
>>>>>  		  unsigned short cnt, unsigned char *key);
>>>>>  int mmc_rpmb_write(struct mmc *mmc, void *addr, unsigned short blk,
>>>>>  		   unsigned short cnt, unsigned char *key);
>>>>> +int mmc_set_bkops_enable(struct mmc *mmc);
>>>>> +
>>>>>  /**
>>>>>   * Start device initialization and return immediately; it does not block on
>>>>>   * polling OCR (operation condition register) status.  Then you should call
>>>>>
>>>>>
> 
> 
> 

[U-Boot] mmc: add bkops-enable command

[Tomas Melin]

Hi Jaehoon,

On 11/18/2016 07:07 AM, Jaehoon Chung wrote:
>> On 11/16/2016 03:39 PM, Jaehoon Chung wrote:
>>>  
>>> On 11/16/2016 10:12 PM, Tomas Melin wrote:
>>>> Hi,
>>>>
>>>> On 11/16/2016 02:05 PM, Jaehoon Chung wrote:
>>>>>
>>>>> Why needs to set bkops on bootloader? Is there special reason?
>>>>> And Linux kernel has already discussed about this.
>>>>>
>>>> It is beneficial to be able to do all required eMMC settings without being dependent on Linux booting.
>>>> It saves time in production to do initial eMMC setup directly from bootloader.
>>>>
>>>>> I don't want to provide this command on u-boot side.
>>>>> Don't handle Onetime programmable register on u-boot. So NACK.
>>>>
>>>> U-Boot already provides One-time programmable commands such as hwpartition and rst-function.
>>>> This adds to that same palette of commands that are needed when properly configuring an eMMC device. 
>>>
>>> hwpartition and rst-function didn't affect the performance. And it's not critical thing.
>>> But BKOPS needs to consider too many things. 
>>> Just even set to enable bit..it's not working.
>>
>> Sorry, but I don't see how this patch affects performance? This only adds a command (mmc bkops enable) that gives the user 
>> a _possibility_ to enable bkops in the eMMC. The choise whether or not to enable bkops is still left to the user.
> 
> You're right. This patch doesn't affect performance, because this patch just provides to enable bkops, right?

Yes. 

> My means is "i don't want to provide the command for enabling BKOPS.".

Could you please explain why you want to make that decision for all users, and not leave it up to the end user whether or not to take bkops into use? Wouldnt it make sense to provide it as a separate CONFIG_ option?

> 
> There is no information on u-boot whether kernel can fully support the BKOPS functionality or not.
> Do you know how to run bkops on kernel side when bkops is enabled from u-boot?
> 
> As i know, there are discussions about enabling BKOPS at Kernel mmc mailing.
> Even when i had implemented the BKOPS feature, i put the some flag for enabling BKOPS. (likes MMC_CAP2_ENABLE_BKOPS)
> It can be also chosen from USER with dt property. but Finally we removed this capability.
> 
> 1) When BKOPS is enabled, needs to consider IO performance.
> 2) There is no periodic BKOPS functionality.
> 3) eMMC5.1 has introduced new automatic bkops feature..BIT[1] ATUO_EN.
> (Also not apply on linux-kernel.)
> 4) mmc-util supports all configuration on kernel side. 

Is your meaning by this that you think that the kernel driver implementation is insufficient, and that you therefore do not recommmend using bkops functionality for eMMC at all (ever)?

> 
> If it has to enable this, i think better that leaves a matter in Kernel.
>>>
>>> Did you check the periodic bkops and auto bkops? And is it working correct?
>>> How to control suspend and Idle state? LEVEL2/3?
>>>
>>> When i had tested the bkops, it has affected IO performance...Of course, it should be maintained average.
>>> Because it should be doing GC with bkops. That is not reason that has to enable on bootloader.
>>
>> The reason to have this command in U-Boot is to be able to do _all_ configuration settings for the eMMC 
>> during manufacturing, prior to flashing the device. Thus, all settings are done only once from U-Boot.
>> In normal operation, the OS driver will be responsible for bkops handling.
> 
> _all_ configuration? i don't agree..We don't need to put _all_ configuration command on u-boot side.

As stated previously, booting into Linux just to do a single eMMC setting and then rebooting takes a lot more time than doing it from the 
bootloader.

BR,
Tomas

> 
> Best Regards,
> Jaehoon Chung
> 
>>
>> BR,
>> Tomas
>>
>>>
>>> I will not apply on u-boot-mmc for bkops. There is no benefit.
>>>
>>> If you set to enable bkops, then you did to pass the other controlling responsibility to Kernel. 
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>
>>>> One possible option going forward could be putting all eMMC-configure related commands behind a common CONFIG_ option.
>>>>
>>>> BR,
>>>> Tomas
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>>> ---
>>>>>>  cmd/mmc.c         | 26 ++++++++++++++++++++++++++
>>>>>>  drivers/mmc/mmc.c | 30 ++++++++++++++++++++++++++++++
>>>>>>  include/mmc.h     |  4 ++++
>>>>>>  3 files changed, 60 insertions(+)
>>>>>>
>>>>>> diff --git a/cmd/mmc.c b/cmd/mmc.c
>>>>>> index b2761e9..3ae9682 100644
>>>>>> --- a/cmd/mmc.c
>>>>>> +++ b/cmd/mmc.c
>>>>>> @@ -729,6 +729,29 @@ static int do_mmc_setdsr(cmd_tbl_t *cmdtp, int flag,
>>>>>>  	return ret;
>>>>>>  }
>>>>>>  
>>>>>> +static int do_mmc_bkops_enable(cmd_tbl_t *cmdtp, int flag,
>>>>>> +				   int argc, char * const argv[])
>>>>>> +{
>>>>>> +	int dev;
>>>>>> +	struct mmc *mmc;
>>>>>> +
>>>>>> +	if (argc != 2)
>>>>>> +		return CMD_RET_USAGE;
>>>>>> +
>>>>>> +	dev = simple_strtoul(argv[1], NULL, 10);
>>>>>> +
>>>>>> +	mmc = init_mmc_device(dev, false);
>>>>>> +	if (!mmc)
>>>>>> +		return CMD_RET_FAILURE;
>>>>>> +
>>>>>> +	if (IS_SD(mmc)) {
>>>>>> +		puts("BKOPS_EN only exists on eMMC\n");
>>>>>> +		return CMD_RET_FAILURE;
>>>>>> +	}
>>>>>> +
>>>>>> +	return mmc_set_bkops_enable(mmc);
>>>>>> +}
>>>>>> +
>>>>>>  static cmd_tbl_t cmd_mmc[] = {
>>>>>>  	U_BOOT_CMD_MKENT(info, 1, 0, do_mmcinfo, "", ""),
>>>>>>  	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
>>>>>> @@ -749,6 +772,7 @@ static cmd_tbl_t cmd_mmc[] = {
>>>>>>  	U_BOOT_CMD_MKENT(rpmb, CONFIG_SYS_MAXARGS, 1, do_mmcrpmb, "", ""),
>>>>>>  #endif
>>>>>>  	U_BOOT_CMD_MKENT(setdsr, 2, 0, do_mmc_setdsr, "", ""),
>>>>>> +	U_BOOT_CMD_MKENT(bkops-enable, 2, 0, do_mmc_bkops_enable, "", ""),
>>>>>>  };
>>>>>>  
>>>>>>  static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>>> @@ -813,6 +837,8 @@ U_BOOT_CMD(
>>>>>>  	"mmc rpmb counter - read the value of the write counter\n"
>>>>>>  #endif
>>>>>>  	"mmc setdsr <value> - set DSR register value\n"
>>>>>> +	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
>>>>>> +	"   WARNING: This is a write-once setting.\n"
>>>>>>  	);
>>>>>>  
>>>>>>  /* Old command kept for compatibility. Same as 'mmc info' */
>>>>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>>>>> index 0cec02c..d6f40cc 100644
>>>>>> --- a/drivers/mmc/mmc.c
>>>>>> +++ b/drivers/mmc/mmc.c
>>>>>> @@ -1810,3 +1810,33 @@ int mmc_initialize(bd_t *bis)
>>>>>>  	mmc_do_preinit();
>>>>>>  	return 0;
>>>>>>  }
>>>>>> +
>>>>>> +int mmc_set_bkops_enable(struct mmc *mmc)
>>>>>> +{
>>>>>> +	int err;
>>>>>> +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
>>>>>> +
>>>>>> +	err = mmc_send_ext_csd(mmc, ext_csd);
>>>>>> +	if (err) {
>>>>>> +		puts("Could not get ext_csd register values\n");
>>>>>> +		return err;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (!(ext_csd[EXT_CSD_BKOPS_SUPPORT] & 0x1)) {
>>>>>> +		puts("Background operations not supported on device\n");
>>>>>> +		return -EMEDIUMTYPE;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
>>>>>> +		puts("Background operations already enabled\n");
>>>>>> +		return 0;
>>>>>> +	}
>>>>>> +
>>>>>> +	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
>>>>>> +	if (err) {
>>>>>> +		puts("Failed to enable background operations\n");
>>>>>> +		return err;
>>>>>> +	}
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> diff --git a/include/mmc.h b/include/mmc.h
>>>>>> index 5ef37d3..0772d53 100644
>>>>>> --- a/include/mmc.h
>>>>>> +++ b/include/mmc.h
>>>>>> @@ -175,6 +175,7 @@
>>>>>>  #define EXT_CSD_MAX_ENH_SIZE_MULT	157	/* R */
>>>>>>  #define EXT_CSD_PARTITIONING_SUPPORT	160	/* RO */
>>>>>>  #define EXT_CSD_RST_N_FUNCTION		162	/* R/W */
>>>>>> +#define EXT_CSD_BKOPS_EN		163	/* R/W */
>>>>>>  #define EXT_CSD_WR_REL_PARAM		166	/* R */
>>>>>>  #define EXT_CSD_WR_REL_SET		167	/* R/W */
>>>>>>  #define EXT_CSD_RPMB_MULT		168	/* RO */
>>>>>> @@ -189,6 +190,7 @@
>>>>>>  #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_BKOPS_SUPPORT		502	/* RO */
>>>>>>  
>>>>>>  /*
>>>>>>   * EXT_CSD field definitions
>>>>>> @@ -541,6 +543,8 @@ int mmc_rpmb_read(struct mmc *mmc, void *addr, unsigned short blk,
>>>>>>  		  unsigned short cnt, unsigned char *key);
>>>>>>  int mmc_rpmb_write(struct mmc *mmc, void *addr, unsigned short blk,
>>>>>>  		   unsigned short cnt, unsigned char *key);
>>>>>> +int mmc_set_bkops_enable(struct mmc *mmc);
>>>>>> +
>>>>>>  /**
>>>>>>   * Start device initialization and return immediately; it does not block on
>>>>>>   * polling OCR (operation condition register) status.  Then you should call
>>>>>>
>>>>>>
>>
>>
>>
> 

[U-Boot] mmc: add bkops-enable command

[Jaehoon Chung]

Hi Tomas,

On 11/21/2016 04:52 PM, Tomas Melin wrote:
> Hi Jaehoon,
> 
> On 11/18/2016 07:07 AM, Jaehoon Chung wrote:
>>> On 11/16/2016 03:39 PM, Jaehoon Chung wrote:
>>>>  
>>>> On 11/16/2016 10:12 PM, Tomas Melin wrote:
>>>>> Hi,
>>>>>
>>>>> On 11/16/2016 02:05 PM, Jaehoon Chung wrote:
>>>>>>
>>>>>> Why needs to set bkops on bootloader? Is there special reason?
>>>>>> And Linux kernel has already discussed about this.
>>>>>>
>>>>> It is beneficial to be able to do all required eMMC settings without being dependent on Linux booting.
>>>>> It saves time in production to do initial eMMC setup directly from bootloader.
>>>>>
>>>>>> I don't want to provide this command on u-boot side.
>>>>>> Don't handle Onetime programmable register on u-boot. So NACK.
>>>>>
>>>>> U-Boot already provides One-time programmable commands such as hwpartition and rst-function.
>>>>> This adds to that same palette of commands that are needed when properly configuring an eMMC device. 
>>>>
>>>> hwpartition and rst-function didn't affect the performance. And it's not critical thing.
>>>> But BKOPS needs to consider too many things. 
>>>> Just even set to enable bit..it's not working.
>>>
>>> Sorry, but I don't see how this patch affects performance? This only adds a command (mmc bkops enable) that gives the user 
>>> a _possibility_ to enable bkops in the eMMC. The choise whether or not to enable bkops is still left to the user.
>>
>> You're right. This patch doesn't affect performance, because this patch just provides to enable bkops, right?
> 
> Yes. 
> 
>> My means is "i don't want to provide the command for enabling BKOPS.".
> 
> Could you please explain why you want to make that decision for all users, and not leave it up to the end user whether or not to take bkops into use? Wouldnt it make sense to provide it as a separate CONFIG_ option?

1) kernel didn't support the BKOPS feature fully yet.
2) if MUST enable this bit, it's right that leaves a matter in Kernel.
3) about other configurations,
- Even though RST_N_FUNCTION is enabled on u-boot, but kernel can control to use or not with MMC_CAP_HW_RESET flag and host->ops->hw_reset callback.
- HWpartition command is not doing anything in kernel side..So it can be used in u-boot.
4) If it needs to run bkops in u-boot, i agreed that provides this command.
5) If provides the bkops enabling,,the doing kernel is more easier then u-boot..

My preference for u-boot-mmc is 
- some feature relevant to kernel side..leaves a matter in kernel.
- e.g) u-boot can enable the BKOPS..but it's black-box what bkops doing and how to control in kernel.
  So i don't want to put command for this feature.

> 
>>
>> There is no information on u-boot whether kernel can fully support the BKOPS functionality or not.
>> Do you know how to run bkops on kernel side when bkops is enabled from u-boot?
>>
>> As i know, there are discussions about enabling BKOPS at Kernel mmc mailing.
>> Even when i had implemented the BKOPS feature, i put the some flag for enabling BKOPS. (likes MMC_CAP2_ENABLE_BKOPS)
>> It can be also chosen from USER with dt property. but Finally we removed this capability.
>>
>> 1) When BKOPS is enabled, needs to consider IO performance.
>> 2) There is no periodic BKOPS functionality.
>> 3) eMMC5.1 has introduced new automatic bkops feature..BIT[1] ATUO_EN.
>> (Also not apply on linux-kernel.)
>> 4) mmc-util supports all configuration on kernel side. 
> 
> Is your meaning by this that you think that the kernel driver implementation is insufficient, and that you therefore do not recommmend using bkops functionality for eMMC at all (ever)?

No...If user really want to use this, they can use the mmc-utils..

> 
>>
>> If it has to enable this, i think better that leaves a matter in Kernel.
>>>>
>>>> Did you check the periodic bkops and auto bkops? And is it working correct?
>>>> How to control suspend and Idle state? LEVEL2/3?
>>>>
>>>> When i had tested the bkops, it has affected IO performance...Of course, it should be maintained average.
>>>> Because it should be doing GC with bkops. That is not reason that has to enable on bootloader.
>>>
>>> The reason to have this command in U-Boot is to be able to do _all_ configuration settings for the eMMC 
>>> during manufacturing, prior to flashing the device. Thus, all settings are done only once from U-Boot.
>>> In normal operation, the OS driver will be responsible for bkops handling.
>>
>> _all_ configuration? i don't agree..We don't need to put _all_ configuration command on u-boot side.
> 
> As stated previously, booting into Linux just to do a single eMMC setting and then rebooting takes a lot more time than doing it from the 
> bootloader.

I don't think so..If it needs to set some register in Linux, then it's possible to enable at booting time.
Of course, it has to implement something with property in devicetree.

Best Regards,
Jaehoon Chung

> 
> BR,
> Tomas
> 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>> BR,
>>> Tomas
>>>
>>>>
>>>> I will not apply on u-boot-mmc for bkops. There is no benefit.
>>>>
>>>> If you set to enable bkops, then you did to pass the other controlling responsibility to Kernel. 
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>>
>>>>> One possible option going forward could be putting all eMMC-configure related commands behind a common CONFIG_ option.
>>>>>
>>>>> BR,
>>>>> Tomas
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>>>> ---
>>>>>>>  cmd/mmc.c         | 26 ++++++++++++++++++++++++++
>>>>>>>  drivers/mmc/mmc.c | 30 ++++++++++++++++++++++++++++++
>>>>>>>  include/mmc.h     |  4 ++++
>>>>>>>  3 files changed, 60 insertions(+)
>>>>>>>
>>>>>>> diff --git a/cmd/mmc.c b/cmd/mmc.c
>>>>>>> index b2761e9..3ae9682 100644
>>>>>>> --- a/cmd/mmc.c
>>>>>>> +++ b/cmd/mmc.c
>>>>>>> @@ -729,6 +729,29 @@ static int do_mmc_setdsr(cmd_tbl_t *cmdtp, int flag,
>>>>>>>  	return ret;
>>>>>>>  }
>>>>>>>  
>>>>>>> +static int do_mmc_bkops_enable(cmd_tbl_t *cmdtp, int flag,
>>>>>>> +				   int argc, char * const argv[])
>>>>>>> +{
>>>>>>> +	int dev;
>>>>>>> +	struct mmc *mmc;
>>>>>>> +
>>>>>>> +	if (argc != 2)
>>>>>>> +		return CMD_RET_USAGE;
>>>>>>> +
>>>>>>> +	dev = simple_strtoul(argv[1], NULL, 10);
>>>>>>> +
>>>>>>> +	mmc = init_mmc_device(dev, false);
>>>>>>> +	if (!mmc)
>>>>>>> +		return CMD_RET_FAILURE;
>>>>>>> +
>>>>>>> +	if (IS_SD(mmc)) {
>>>>>>> +		puts("BKOPS_EN only exists on eMMC\n");
>>>>>>> +		return CMD_RET_FAILURE;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return mmc_set_bkops_enable(mmc);
>>>>>>> +}
>>>>>>> +
>>>>>>>  static cmd_tbl_t cmd_mmc[] = {
>>>>>>>  	U_BOOT_CMD_MKENT(info, 1, 0, do_mmcinfo, "", ""),
>>>>>>>  	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
>>>>>>> @@ -749,6 +772,7 @@ static cmd_tbl_t cmd_mmc[] = {
>>>>>>>  	U_BOOT_CMD_MKENT(rpmb, CONFIG_SYS_MAXARGS, 1, do_mmcrpmb, "", ""),
>>>>>>>  #endif
>>>>>>>  	U_BOOT_CMD_MKENT(setdsr, 2, 0, do_mmc_setdsr, "", ""),
>>>>>>> +	U_BOOT_CMD_MKENT(bkops-enable, 2, 0, do_mmc_bkops_enable, "", ""),
>>>>>>>  };
>>>>>>>  
>>>>>>>  static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>>>> @@ -813,6 +837,8 @@ U_BOOT_CMD(
>>>>>>>  	"mmc rpmb counter - read the value of the write counter\n"
>>>>>>>  #endif
>>>>>>>  	"mmc setdsr <value> - set DSR register value\n"
>>>>>>> +	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
>>>>>>> +	"   WARNING: This is a write-once setting.\n"
>>>>>>>  	);
>>>>>>>  
>>>>>>>  /* Old command kept for compatibility. Same as 'mmc info' */
>>>>>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>>>>>> index 0cec02c..d6f40cc 100644
>>>>>>> --- a/drivers/mmc/mmc.c
>>>>>>> +++ b/drivers/mmc/mmc.c
>>>>>>> @@ -1810,3 +1810,33 @@ int mmc_initialize(bd_t *bis)
>>>>>>>  	mmc_do_preinit();
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>> +
>>>>>>> +int mmc_set_bkops_enable(struct mmc *mmc)
>>>>>>> +{
>>>>>>> +	int err;
>>>>>>> +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
>>>>>>> +
>>>>>>> +	err = mmc_send_ext_csd(mmc, ext_csd);
>>>>>>> +	if (err) {
>>>>>>> +		puts("Could not get ext_csd register values\n");
>>>>>>> +		return err;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (!(ext_csd[EXT_CSD_BKOPS_SUPPORT] & 0x1)) {
>>>>>>> +		puts("Background operations not supported on device\n");
>>>>>>> +		return -EMEDIUMTYPE;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
>>>>>>> +		puts("Background operations already enabled\n");
>>>>>>> +		return 0;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
>>>>>>> +	if (err) {
>>>>>>> +		puts("Failed to enable background operations\n");
>>>>>>> +		return err;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> diff --git a/include/mmc.h b/include/mmc.h
>>>>>>> index 5ef37d3..0772d53 100644
>>>>>>> --- a/include/mmc.h
>>>>>>> +++ b/include/mmc.h
>>>>>>> @@ -175,6 +175,7 @@
>>>>>>>  #define EXT_CSD_MAX_ENH_SIZE_MULT	157	/* R */
>>>>>>>  #define EXT_CSD_PARTITIONING_SUPPORT	160	/* RO */
>>>>>>>  #define EXT_CSD_RST_N_FUNCTION		162	/* R/W */
>>>>>>> +#define EXT_CSD_BKOPS_EN		163	/* R/W */
>>>>>>>  #define EXT_CSD_WR_REL_PARAM		166	/* R */
>>>>>>>  #define EXT_CSD_WR_REL_SET		167	/* R/W */
>>>>>>>  #define EXT_CSD_RPMB_MULT		168	/* RO */
>>>>>>> @@ -189,6 +190,7 @@
>>>>>>>  #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_BKOPS_SUPPORT		502	/* RO */
>>>>>>>  
>>>>>>>  /*
>>>>>>>   * EXT_CSD field definitions
>>>>>>> @@ -541,6 +543,8 @@ int mmc_rpmb_read(struct mmc *mmc, void *addr, unsigned short blk,
>>>>>>>  		  unsigned short cnt, unsigned char *key);
>>>>>>>  int mmc_rpmb_write(struct mmc *mmc, void *addr, unsigned short blk,
>>>>>>>  		   unsigned short cnt, unsigned char *key);
>>>>>>> +int mmc_set_bkops_enable(struct mmc *mmc);
>>>>>>> +
>>>>>>>  /**
>>>>>>>   * Start device initialization and return immediately; it does not block on
>>>>>>>   * polling OCR (operation condition register) status.  Then you should call
>>>>>>>
>>>>>>>
>>>
>>>
>>>
>>
> 
> 
> 

[U-Boot] mmc: add bkops-enable command

[Tomas Melin]

Hi Jaehoon,

On 11/23/2016 11:53 AM, Jaehoon Chung wrote:
> On 11/21/2016 04:52 PM, Tomas Melin wrote:
>> Is your meaning by this that you think that the kernel driver implementation is insufficient, and that you therefore do not recommmend using bkops functionality for eMMC at all (ever)?
> 
> No...If user really want to use this, they can use the mmc-utils..

Actually, I have also been using mmc-utils for this purpose, and even provided patches upstream for the tool. It seems that all settings are not really working properly. E.g. setting both hwpartition and write reliability failes in some cases with our devices, the write reliability bit setting is for some reason lost after the power cycle. The same problems have not be noticed doing the same configurations from U-Boot.
So doing the emmc setting seems more reliable in U-Boot.

Below v2 of the original patch that instead puts the command into a separate CONFIG_CMD_BKOPS_ENABLE that must be enabled explicitly by
the user if he or she wishes to use the command.
I understand your standpoint and arguments for not providing the command in U-Boot with default setting, but please still consider
accepting it as a configurable command that can be pulled in by those users who need it. Either way, we will
keep it as a out-of-tree patch. But frankly, I think this command might be of use to many others as well.

BR, 
Tomas

[U-Boot] mmc: add bkops-enable command

[Jaehoon Chung]

Hi Tomas,

On 11/23/2016 09:50 PM, Tomas Melin wrote:
> Hi Jaehoon,
> 
> On 11/23/2016 11:53 AM, Jaehoon Chung wrote:
>> On 11/21/2016 04:52 PM, Tomas Melin wrote:
>>> Is your meaning by this that you think that the kernel driver implementation is insufficient, and that you therefore do not recommmend using bkops functionality for eMMC at all (ever)?
>>
>> No...If user really want to use this, they can use the mmc-utils..
> 
> Actually, I have also been using mmc-utils for this purpose, and even provided patches upstream for the tool. It seems that all settings are not really working properly. E.g. setting both hwpartition and write reliability failes in some cases with our devices, the write reliability bit setting is for some reason lost after the power cycle. The same problems have not be noticed doing the same configurations from U-Boot.
> So doing the emmc setting seems more reliable in U-Boot.
> 
> Below v2 of the original patch that instead puts the command into a separate CONFIG_CMD_BKOPS_ENABLE that must be enabled explicitly by
> the user if he or she wishes to use the command.
> I understand your standpoint and arguments for not providing the command in U-Boot with default setting, but please still consider
> accepting it as a configurable command that can be pulled in by those users who need it. Either way, we will
> keep it as a out-of-tree patch. But frankly, I think this command might be of use to many others as well.

I understood what your purpose..Could you send the patch again?
After sending the patch V2 again, i will check again for satisfying both you and me. :)

> 
> BR, 
> Tomas
> 
> 
>>From 7387943be48e7ccc2bf6aa1d30d35ef279c98f2d Mon Sep 17 00:00:00 2001
> From: Tomas Melin <tomas.melin@vaisala.com>
> Date: Wed, 16 Nov 2016 09:10:02 +0200
> Subject: [PATCH] mmc: add bkops-enable command
> 
> Add new command that provides possibility to enable the
> background operations handshake functionality
> (BKOPS_EN, EXT_CSD byte [163]) on eMMC devices.
> 
> This is an optional feature of eMMCs, the setting is write-once.
> The command must be explicitly taken into use with
> CONFIG_CMD_BKOPS_ENABLE.
> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  cmd/Kconfig       |  7 +++++++
>  cmd/mmc.c         | 32 ++++++++++++++++++++++++++++++++
>  drivers/mmc/mmc.c | 32 ++++++++++++++++++++++++++++++++
>  include/mmc.h     |  6 ++++++
>  4 files changed, 77 insertions(+)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index e339d86..8286d15 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -550,6 +550,13 @@ config SYS_AMBAPP_PRINT_ON_STARTUP
>  	help
>  	  Show AMBA Plug-n-Play information on startup.
>  
> +config CMD_BKOPS_ENABLE
> +	bool "mmc bkops enable"
> +	depends on CMD_MMC
> +	default n
> +	help
> +	  Enable background operations handshake on device.
> +
>  config CMD_BLOCK_CACHE
>  	bool "blkcache - control and stats for block cache"
>  	depends on BLOCK_CACHE
> diff --git a/cmd/mmc.c b/cmd/mmc.c
> index b2761e9..b8dcc26 100644
> --- a/cmd/mmc.c
> +++ b/cmd/mmc.c
> @@ -729,6 +729,31 @@ static int do_mmc_setdsr(cmd_tbl_t *cmdtp, int flag,
>  	return ret;
>  }
>  
> +#ifdef CONFIG_CMD_BKOPS_ENABLE
> +static int do_mmc_bkops_enable(cmd_tbl_t *cmdtp, int flag,
> +				   int argc, char * const argv[])
> +{
> +	int dev;
> +	struct mmc *mmc;
> +
> +	if (argc != 2)
> +		return CMD_RET_USAGE;
> +
> +	dev = simple_strtoul(argv[1], NULL, 10);
> +
> +	mmc = init_mmc_device(dev, false);
> +	if (!mmc)
> +		return CMD_RET_FAILURE;
> +
> +	if (IS_SD(mmc)) {
> +		puts("BKOPS_EN only exists on eMMC\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	return mmc_set_bkops_enable(mmc);
> +}
> +#endif
> +
>  static cmd_tbl_t cmd_mmc[] = {
>  	U_BOOT_CMD_MKENT(info, 1, 0, do_mmcinfo, "", ""),
>  	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
> @@ -749,6 +774,9 @@ static cmd_tbl_t cmd_mmc[] = {
>  	U_BOOT_CMD_MKENT(rpmb, CONFIG_SYS_MAXARGS, 1, do_mmcrpmb, "", ""),
>  #endif
>  	U_BOOT_CMD_MKENT(setdsr, 2, 0, do_mmc_setdsr, "", ""),
> +#ifdef CONFIG_CMD_BKOPS_ENABLE
> +	U_BOOT_CMD_MKENT(bkops-enable, 2, 0, do_mmc_bkops_enable, "", ""),
> +#endif
>  };
>  
>  static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> @@ -813,6 +841,10 @@ U_BOOT_CMD(
>  	"mmc rpmb counter - read the value of the write counter\n"
>  #endif
>  	"mmc setdsr <value> - set DSR register value\n"
> +#ifdef CONFIG_CMD_BKOPS_ENABLE
> +	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
> +	"   WARNING: This is a write-once setting.\n"

"Write-once setting" -> "Onetime programmable"

> +#endif
>  	);
>  
>  /* Old command kept for compatibility. Same as 'mmc info' */
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 0cec02c..d40d13f 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -1810,3 +1810,35 @@ int mmc_initialize(bd_t *bis)
>  	mmc_do_preinit();
>  	return 0;
>  }
> +
> +#ifdef CONFIG_CMD_BKOPS_ENABLE
> +int mmc_set_bkops_enable(struct mmc *mmc)
> +{
> +	int err;
> +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
> +
> +	err = mmc_send_ext_csd(mmc, ext_csd);
> +	if (err) {
> +		puts("Could not get ext_csd register values\n");
> +		return err;
> +	}

Does it need to check Card version?

> +
> +	if (!(ext_csd[EXT_CSD_BKOPS_SUPPORT] & 0x1)) {
> +		puts("Background operations not supported on device\n");
> +		return -EMEDIUMTYPE;
> +	}
> +
> +	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
> +		puts("Background operations already enabled\n");
> +		return 0;
> +	}
> +
> +	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
> +	if (err) {
> +		puts("Failed to enable background operations\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +#endif
> diff --git a/include/mmc.h b/include/mmc.h
> index 5ef37d3..2c677f7 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -175,6 +175,7 @@
>  #define EXT_CSD_MAX_ENH_SIZE_MULT	157	/* R */
>  #define EXT_CSD_PARTITIONING_SUPPORT	160	/* RO */
>  #define EXT_CSD_RST_N_FUNCTION		162	/* R/W */
> +#define EXT_CSD_BKOPS_EN		163	/* R/W */

Maybe not R/W.

Best Regards,
Jaehoon Chung

>  #define EXT_CSD_WR_REL_PARAM		166	/* R */
>  #define EXT_CSD_WR_REL_SET		167	/* R/W */
>  #define EXT_CSD_RPMB_MULT		168	/* RO */
> @@ -189,6 +190,7 @@
>  #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_BKOPS_SUPPORT		502	/* RO */
>  
>  /*
>   * EXT_CSD field definitions
> @@ -541,6 +543,10 @@ int mmc_rpmb_read(struct mmc *mmc, void *addr, unsigned short blk,
>  		  unsigned short cnt, unsigned char *key);
>  int mmc_rpmb_write(struct mmc *mmc, void *addr, unsigned short blk,
>  		   unsigned short cnt, unsigned char *key);
> +#ifdef CONFIG_CMD_BKOPS_ENABLE
> +int mmc_set_bkops_enable(struct mmc *mmc);
> +#endif
> +
>  /**
>   * Start device initialization and return immediately; it does not block on
>   * polling OCR (operation condition register) status.  Then you should call
>