* Re: [PATCH] cmd: mmc: Expand bkops handling
2022-12-22 5:10 ` [PATCH] cmd: mmc: Expand bkops handling Marek Vasut
@ 2022-12-22 17:47 ` Simon Glass
2023-01-05 14:20 ` Marek Vasut
2022-12-26 23:55 ` Jaehoon Chung
1 sibling, 1 reply; 5+ messages in thread
From: Simon Glass @ 2022-12-22 17:47 UTC (permalink / raw)
To: Marek Vasut; +Cc: u-boot, Jaehoon Chung, Peng Fan
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
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] cmd: mmc: Expand bkops handling
2022-12-22 5:10 ` [PATCH] cmd: mmc: Expand bkops handling Marek Vasut
2022-12-22 17:47 ` Simon Glass
@ 2022-12-26 23:55 ` Jaehoon Chung
2023-01-05 14:14 ` Marek Vasut
1 sibling, 1 reply; 5+ messages in thread
From: Jaehoon Chung @ 2022-12-26 23:55 UTC (permalink / raw)
To: 'Marek Vasut', u-boot; +Cc: 'Peng Fan'
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
^ permalink raw reply [flat|nested] 5+ messages in thread