Hi Tomas,
On 11/17/2016 08:05 PM, Tomas Melin wrote:
> Hi,
>
> Thank you for your valuable comments and for taking the time to respond.
> Please see below for further comments.
No problem, It's useful to discuss about something. :)
>
> 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?
My means is "i don't want to provide the command for enabling BKOPS.".
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.
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.
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.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
