[PATCH] cmd: mmc: Expand bkops handling

[PATCH] cmd: mmc: Expand bkops handling

[Marek Vasut]

Add more capable "bkops" command which allows enabling and disabling both
manual and automatic bkops. The existing 'mmc bkops-enable' subcommand is
poorly named to cover all the possibilities, hence the new-ish subcommand.
Note that both commands are wrappers around the same common code.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Peng Fan <peng.fan@nxp.com>
---
 cmd/mmc.c         | 49 +++++++++++++++++++++++++++++++++++++++--------
 drivers/mmc/mmc.c | 14 +++++++++-----
 include/mmc.h     |  2 +-
 3 files changed, 51 insertions(+), 14 deletions(-)

diff --git a/cmd/mmc.c b/cmd/mmc.c
index c79d9407986..94deb9a1686 100644
--- a/cmd/mmc.c
+++ b/cmd/mmc.c
@@ -1020,16 +1020,12 @@ static int do_mmc_setdsr(struct cmd_tbl *cmdtp, int flag,
 }
 
 #ifdef CONFIG_CMD_BKOPS_ENABLE
-static int do_mmc_bkops_enable(struct cmd_tbl *cmdtp, int flag,
-			       int argc, char *const argv[])
+static int mmc_bkops_common(char *device, bool autobkops, bool enable)
 {
-	int dev;
 	struct mmc *mmc;
+	int dev;
 
-	if (argc != 2)
-		return CMD_RET_USAGE;
-
-	dev = dectoul(argv[1], NULL);
+	dev = dectoul(device, NULL);
 
 	mmc = init_mmc_device(dev, false);
 	if (!mmc)
@@ -1040,7 +1036,41 @@ static int do_mmc_bkops_enable(struct cmd_tbl *cmdtp, int flag,
 		return CMD_RET_FAILURE;
 	}
 
-	return mmc_set_bkops_enable(mmc);
+	return mmc_set_bkops_enable(mmc, autobkops, enable);
+}
+
+static int do_mmc_bkops(struct cmd_tbl *cmdtp, int flag,
+			int argc, char * const argv[])
+{
+	bool autobkops, enable;
+
+	if (argc != 4)
+		return CMD_RET_USAGE;
+
+	if (!strcmp(argv[2], "manual"))
+		autobkops = false;
+	else if (!strcmp(argv[2], "auto"))
+		autobkops = true;
+	else
+		return CMD_RET_FAILURE;
+
+	if (!strcmp(argv[3], "disable"))
+		enable = false;
+	else if (!strcmp(argv[3], "enable"))
+		enable = true;
+	else
+		return CMD_RET_FAILURE;
+
+	return mmc_bkops_common(argv[1], autobkops, enable);
+}
+
+static int do_mmc_bkops_enable(struct cmd_tbl *cmdtp, int flag,
+			       int argc, char * const argv[])
+{
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	return mmc_bkops_common(argv[1], false, true);
 }
 #endif
 
@@ -1102,6 +1132,7 @@ static struct cmd_tbl cmd_mmc[] = {
 	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, "", ""),
+	U_BOOT_CMD_MKENT(bkops, 4, 0, do_mmc_bkops, "", ""),
 #endif
 };
 
@@ -1188,6 +1219,8 @@ U_BOOT_CMD(
 #ifdef CONFIG_CMD_BKOPS_ENABLE
 	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
 	"   WARNING: This is a write-once setting.\n"
+	"mmc bkops <dev> [auto|manual] [enable|disable]\n"
+	" - configure background operations handshake on device\n"
 #endif
 	);
 
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 210703ea46b..afbc497b12c 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -3127,9 +3127,10 @@ int mmc_init_device(int num)
 #endif
 
 #ifdef CONFIG_CMD_BKOPS_ENABLE
-int mmc_set_bkops_enable(struct mmc *mmc)
+int mmc_set_bkops_enable(struct mmc *mmc, bool autobkops, bool enable)
 {
 	int err;
+	u32 bit = autobkops ? BIT(1) : BIT(0);
 	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
 
 	err = mmc_send_ext_csd(mmc, ext_csd);
@@ -3143,18 +3144,21 @@ int mmc_set_bkops_enable(struct mmc *mmc)
 		return -EMEDIUMTYPE;
 	}
 
-	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
+	if (enable && (ext_csd[EXT_CSD_BKOPS_EN] & bit)) {
 		puts("Background operations already enabled\n");
 		return 0;
 	}
 
-	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
+	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN,
+			 enable ? bit : 0);
 	if (err) {
-		puts("Failed to enable manual background operations\n");
+		printf("Failed to %sable manual background operations\n",
+		       enable ? "en" : "dis");
 		return err;
 	}
 
-	puts("Enabled manual background operations\n");
+	printf("%sabled %s background operations\n",
+	       enable ? "En" : "Dis", autobkops ? "auto" : "manual");
 
 	return 0;
 }
diff --git a/include/mmc.h b/include/mmc.h
index 571fa625d02..e116e78f343 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -893,7 +893,7 @@ int mmc_rpmb_route_frames(struct mmc *mmc, void *req, unsigned long reqlen,
 			  void *rsp, unsigned long rsplen);
 
 #ifdef CONFIG_CMD_BKOPS_ENABLE
-int mmc_set_bkops_enable(struct mmc *mmc);
+int mmc_set_bkops_enable(struct mmc *mmc, bool autobkops, bool enable);
 #endif
 
 /**
-- 
2.35.1

Re: [PATCH] cmd: mmc: Expand bkops handling

[Simon Glass]

Hi Marek,

On Wed, 21 Dec 2022 at 22:11, Marek Vasut <marex@denx.de> wrote:
>
> Add more capable "bkops" command which allows enabling and disabling both
> manual and automatic bkops. The existing 'mmc bkops-enable' subcommand is
> poorly named to cover all the possibilities, hence the new-ish subcommand.
> Note that both commands are wrappers around the same common code.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> ---
>  cmd/mmc.c         | 49 +++++++++++++++++++++++++++++++++++++++--------
>  drivers/mmc/mmc.c | 14 +++++++++-----
>  include/mmc.h     |  2 +-
>  3 files changed, 51 insertions(+), 14 deletions(-)

Make a start on docs for this?

>
> diff --git a/cmd/mmc.c b/cmd/mmc.c
> index c79d9407986..94deb9a1686 100644
> --- a/cmd/mmc.c
> +++ b/cmd/mmc.c
> @@ -1020,16 +1020,12 @@ static int do_mmc_setdsr(struct cmd_tbl *cmdtp, int flag,
>  }
>
>  #ifdef CONFIG_CMD_BKOPS_ENABLE
> -static int do_mmc_bkops_enable(struct cmd_tbl *cmdtp, int flag,
> -                              int argc, char *const argv[])
> +static int mmc_bkops_common(char *device, bool autobkops, bool enable)
>  {
> -       int dev;
>         struct mmc *mmc;
> +       int dev;
>
> -       if (argc != 2)
> -               return CMD_RET_USAGE;
> -
> -       dev = dectoul(argv[1], NULL);
> +       dev = dectoul(device, NULL);
>
>         mmc = init_mmc_device(dev, false);
>         if (!mmc)
> @@ -1040,7 +1036,41 @@ static int do_mmc_bkops_enable(struct cmd_tbl *cmdtp, int flag,
>                 return CMD_RET_FAILURE;
>         }
>
> -       return mmc_set_bkops_enable(mmc);
> +       return mmc_set_bkops_enable(mmc, autobkops, enable);
> +}
> +
> +static int do_mmc_bkops(struct cmd_tbl *cmdtp, int flag,
> +                       int argc, char * const argv[])
> +{
> +       bool autobkops, enable;
> +
> +       if (argc != 4)
> +               return CMD_RET_USAGE;
> +
> +       if (!strcmp(argv[2], "manual"))
> +               autobkops = false;
> +       else if (!strcmp(argv[2], "auto"))
> +               autobkops = true;
> +       else
> +               return CMD_RET_FAILURE;
> +
> +       if (!strcmp(argv[3], "disable"))
> +               enable = false;
> +       else if (!strcmp(argv[3], "enable"))
> +               enable = true;
> +       else
> +               return CMD_RET_FAILURE;

You could just check the first letter, perhaps, to save code space?

> +
> +       return mmc_bkops_common(argv[1], autobkops, enable);
> +}
> +
> +static int do_mmc_bkops_enable(struct cmd_tbl *cmdtp, int flag,
> +                              int argc, char * const argv[])
> +{
> +       if (argc != 2)
> +               return CMD_RET_USAGE;
> +
> +       return mmc_bkops_common(argv[1], false, true);
>  }
>  #endif
>
> @@ -1102,6 +1132,7 @@ static struct cmd_tbl cmd_mmc[] = {
>         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, "", ""),
> +       U_BOOT_CMD_MKENT(bkops, 4, 0, do_mmc_bkops, "", ""),
>  #endif
>  };
>
> @@ -1188,6 +1219,8 @@ U_BOOT_CMD(
>  #ifdef CONFIG_CMD_BKOPS_ENABLE
>         "mmc bkops-enable <dev> - enable background operations handshake on device\n"
>         "   WARNING: This is a write-once setting.\n"
> +       "mmc bkops <dev> [auto|manual] [enable|disable]\n"
> +       " - configure background operations handshake on device\n"
>  #endif
>         );
>
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 210703ea46b..afbc497b12c 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -3127,9 +3127,10 @@ int mmc_init_device(int num)
>  #endif
>
>  #ifdef CONFIG_CMD_BKOPS_ENABLE

We shouldn't really need this #ifdef, since if it is not called it
won't be included in the binary.

> -int mmc_set_bkops_enable(struct mmc *mmc)
> +int mmc_set_bkops_enable(struct mmc *mmc, bool autobkops, bool enable)
>  {
>         int err;
> +       u32 bit = autobkops ? BIT(1) : BIT(0);
>         ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
>
>         err = mmc_send_ext_csd(mmc, ext_csd);
> @@ -3143,18 +3144,21 @@ int mmc_set_bkops_enable(struct mmc *mmc)
>                 return -EMEDIUMTYPE;
>         }
>
> -       if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
> +       if (enable && (ext_csd[EXT_CSD_BKOPS_EN] & bit)) {
>                 puts("Background operations already enabled\n");
>                 return 0;
>         }
>
> -       err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
> +       err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN,
> +                        enable ? bit : 0);
>         if (err) {
> -               puts("Failed to enable manual background operations\n");
> +               printf("Failed to %sable manual background operations\n",
> +                      enable ? "en" : "dis");
>                 return err;
>         }
>
> -       puts("Enabled manual background operations\n");
> +       printf("%sabled %s background operations\n",
> +              enable ? "En" : "Dis", autobkops ? "auto" : "manual");
>
>         return 0;
>  }
> diff --git a/include/mmc.h b/include/mmc.h
> index 571fa625d02..e116e78f343 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -893,7 +893,7 @@ int mmc_rpmb_route_frames(struct mmc *mmc, void *req, unsigned long reqlen,
>                           void *rsp, unsigned long rsplen);
>
>  #ifdef CONFIG_CMD_BKOPS_ENABLE

Same here

> -int mmc_set_bkops_enable(struct mmc *mmc);
> +int mmc_set_bkops_enable(struct mmc *mmc, bool autobkops, bool enable);

Please add comments for these two

>  #endif
>
>  /**
> --
> 2.35.1
>

Regards,
Simon

RE: [PATCH] cmd: mmc: Expand bkops handling

[Jaehoon Chung]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Thursday, December 22, 2022 2:11 PM
> To: u-boot@lists.denx.de
> Cc: Marek Vasut <marex@denx.de>; Jaehoon Chung <jh80.chung@samsung.com>; Peng Fan <peng.fan@nxp.com>
> Subject: [PATCH] cmd: mmc: Expand bkops handling
> 
> Add more capable "bkops" command which allows enabling and disabling both
> manual and automatic bkops. The existing 'mmc bkops-enable' subcommand is
> poorly named to cover all the possibilities, hence the new-ish subcommand.
> Note that both commands are wrappers around the same common code.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> ---
>  cmd/mmc.c         | 49 +++++++++++++++++++++++++++++++++++++++--------
>  drivers/mmc/mmc.c | 14 +++++++++-----
>  include/mmc.h     |  2 +-
>  3 files changed, 51 insertions(+), 14 deletions(-)
> 
> diff --git a/cmd/mmc.c b/cmd/mmc.c
> index c79d9407986..94deb9a1686 100644
> --- a/cmd/mmc.c
> +++ b/cmd/mmc.c
> @@ -1020,16 +1020,12 @@ static int do_mmc_setdsr(struct cmd_tbl *cmdtp, int flag,
>  }
> 
>  #ifdef CONFIG_CMD_BKOPS_ENABLE
> -static int do_mmc_bkops_enable(struct cmd_tbl *cmdtp, int flag,
> -			       int argc, char *const argv[])
> +static int mmc_bkops_common(char *device, bool autobkops, bool enable)
>  {
> -	int dev;
>  	struct mmc *mmc;
> +	int dev;
> 
> -	if (argc != 2)
> -		return CMD_RET_USAGE;
> -
> -	dev = dectoul(argv[1], NULL);
> +	dev = dectoul(device, NULL);
> 
>  	mmc = init_mmc_device(dev, false);
>  	if (!mmc)
> @@ -1040,7 +1036,41 @@ static int do_mmc_bkops_enable(struct cmd_tbl *cmdtp, int flag,
>  		return CMD_RET_FAILURE;
>  	}
> 
> -	return mmc_set_bkops_enable(mmc);
> +	return mmc_set_bkops_enable(mmc, autobkops, enable);
> +}
> +
> +static int do_mmc_bkops(struct cmd_tbl *cmdtp, int flag,
> +			int argc, char * const argv[])
> +{
> +	bool autobkops, enable;
> +
> +	if (argc != 4)
> +		return CMD_RET_USAGE;
> +
> +	if (!strcmp(argv[2], "manual"))
> +		autobkops = false;
> +	else if (!strcmp(argv[2], "auto"))
> +		autobkops = true;
> +	else
> +		return CMD_RET_FAILURE;
> +
> +	if (!strcmp(argv[3], "disable"))
> +		enable = false;

AFAIK, "manual" enable is one time programmable. It can't disable again after enabled, isn't it?

Best Regards,
Jaehoon Chung

> +	else if (!strcmp(argv[3], "enable"))
> +		enable = true;
> +	else
> +		return CMD_RET_FAILURE;
> +
> +	return mmc_bkops_common(argv[1], autobkops, enable);
> +}
> +
> +static int do_mmc_bkops_enable(struct cmd_tbl *cmdtp, int flag,
> +			       int argc, char * const argv[])
> +{
> +	if (argc != 2)
> +		return CMD_RET_USAGE;
> +
> +	return mmc_bkops_common(argv[1], false, true);
>  }
>  #endif
> 
> @@ -1102,6 +1132,7 @@ static struct cmd_tbl cmd_mmc[] = {
>  	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, "", ""),
> +	U_BOOT_CMD_MKENT(bkops, 4, 0, do_mmc_bkops, "", ""),
>  #endif
>  };
> 
> @@ -1188,6 +1219,8 @@ U_BOOT_CMD(
>  #ifdef CONFIG_CMD_BKOPS_ENABLE
>  	"mmc bkops-enable <dev> - enable background operations handshake on device\n"
>  	"   WARNING: This is a write-once setting.\n"
> +	"mmc bkops <dev> [auto|manual] [enable|disable]\n"
> +	" - configure background operations handshake on device\n"
>  #endif
>  	);
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 210703ea46b..afbc497b12c 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -3127,9 +3127,10 @@ int mmc_init_device(int num)
>  #endif
> 
>  #ifdef CONFIG_CMD_BKOPS_ENABLE
> -int mmc_set_bkops_enable(struct mmc *mmc)
> +int mmc_set_bkops_enable(struct mmc *mmc, bool autobkops, bool enable)
>  {
>  	int err;
> +	u32 bit = autobkops ? BIT(1) : BIT(0);
>  	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
> 
>  	err = mmc_send_ext_csd(mmc, ext_csd);
> @@ -3143,18 +3144,21 @@ int mmc_set_bkops_enable(struct mmc *mmc)
>  		return -EMEDIUMTYPE;
>  	}
> 
> -	if (ext_csd[EXT_CSD_BKOPS_EN] & 0x1) {
> +	if (enable && (ext_csd[EXT_CSD_BKOPS_EN] & bit)) {
>  		puts("Background operations already enabled\n");
>  		return 0;
>  	}
> 
> -	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN, 1);
> +	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BKOPS_EN,
> +			 enable ? bit : 0);
>  	if (err) {
> -		puts("Failed to enable manual background operations\n");
> +		printf("Failed to %sable manual background operations\n",
> +		       enable ? "en" : "dis");
>  		return err;
>  	}
> 
> -	puts("Enabled manual background operations\n");
> +	printf("%sabled %s background operations\n",
> +	       enable ? "En" : "Dis", autobkops ? "auto" : "manual");
> 
>  	return 0;
>  }
> diff --git a/include/mmc.h b/include/mmc.h
> index 571fa625d02..e116e78f343 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -893,7 +893,7 @@ int mmc_rpmb_route_frames(struct mmc *mmc, void *req, unsigned long reqlen,
>  			  void *rsp, unsigned long rsplen);
> 
>  #ifdef CONFIG_CMD_BKOPS_ENABLE
> -int mmc_set_bkops_enable(struct mmc *mmc);
> +int mmc_set_bkops_enable(struct mmc *mmc, bool autobkops, bool enable);
>  #endif
> 
>  /**
> --
> 2.35.1

Re: [PATCH] cmd: mmc: Expand bkops handling

[Marek Vasut]

On 12/27/22 00:55, Jaehoon Chung wrote:
> Hi,

Hi,

[...]

>> @@ -1040,7 +1036,41 @@ static int do_mmc_bkops_enable(struct cmd_tbl *cmdtp, int flag,
>>   		return CMD_RET_FAILURE;
>>   	}
>>
>> -	return mmc_set_bkops_enable(mmc);
>> +	return mmc_set_bkops_enable(mmc, autobkops, enable);
>> +}
>> +
>> +static int do_mmc_bkops(struct cmd_tbl *cmdtp, int flag,
>> +			int argc, char * const argv[])
>> +{
>> +	bool autobkops, enable;
>> +
>> +	if (argc != 4)
>> +		return CMD_RET_USAGE;
>> +
>> +	if (!strcmp(argv[2], "manual"))
>> +		autobkops = false;
>> +	else if (!strcmp(argv[2], "auto"))
>> +		autobkops = true;
>> +	else
>> +		return CMD_RET_FAILURE;
>> +
>> +	if (!strcmp(argv[3], "disable"))
>> +		enable = false;
> 
> AFAIK, "manual" enable is one time programmable. It can't disable again after enabled, isn't it?

Indeed the bkops are all OTP.

Re: [PATCH] cmd: mmc: Expand bkops handling

[Marek Vasut]

On 12/22/22 18:47, Simon Glass wrote:

[...]

>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index 210703ea46b..afbc497b12c 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -3127,9 +3127,10 @@ int mmc_init_device(int num)
>>   #endif
>>
>>   #ifdef CONFIG_CMD_BKOPS_ENABLE
> 
> We shouldn't really need this #ifdef, since if it is not called it
> won't be included in the binary.

You would get "function defined but not used" maybe-unused warning.

>> -int mmc_set_bkops_enable(struct mmc *mmc)
>> +int mmc_set_bkops_enable(struct mmc *mmc, bool autobkops, bool enable)
>>   {
>>          int err;
>> +       u32 bit = autobkops ? BIT(1) : BIT(0);
>>          ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
>>
>>          err = mmc_send_ext_csd(mmc, ext_csd);

[...]