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.me...@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.me...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
