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 at 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 at 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
>