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