[U-Boot] mmc: add bkops-enable command
Tomas Melin
tomas.melin at vaisala.com
Mon Nov 21 08:52:05 CET 2016
Hi Jaehoon,
On 11/18/2016 07:07 AM, Jaehoon Chung wrote:
>> 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.
>
> You're right. This patch doesn't affect performance, because this patch just provides to enable bkops, right?
Yes.
> My means is "i don't want to provide the command for enabling BKOPS.".
Could you please explain why you want to make that decision for all users, and not leave it up to the end user whether or not to take bkops into use? Wouldnt it make sense to provide it as a separate CONFIG_ option?
>
> There is no information on u-boot whether kernel can fully support the BKOPS functionality or not.
> Do you know how to run bkops on kernel side when bkops is enabled from u-boot?
>
> As i know, there are discussions about enabling BKOPS at Kernel mmc mailing.
> Even when i had implemented the BKOPS feature, i put the some flag for enabling BKOPS. (likes MMC_CAP2_ENABLE_BKOPS)
> It can be also chosen from USER with dt property. but Finally we removed this capability.
>
> 1) When BKOPS is enabled, needs to consider IO performance.
> 2) There is no periodic BKOPS functionality.
> 3) eMMC5.1 has introduced new automatic bkops feature..BIT[1] ATUO_EN.
> (Also not apply on linux-kernel.)
> 4) mmc-util supports all configuration on kernel side.
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)?
>
> If it has to enable this, i think better that leaves a matter in Kernel.
>>>
>>> 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.
>
> _all_ configuration? i don't agree..We don't need to put _all_ configuration command on u-boot side.
As stated previously, booting into Linux just to do a single eMMC setting and then rebooting takes a lot more time than doing it from the
bootloader.
BR,
Tomas
>
> Best Regards,
> Jaehoon Chung
>
>>
>> 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.melin at 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
>>>>>>
>>>>>>
>>
>>
>>
>
More information about the U-Boot
mailing list