[U-Boot] [PATCH 16/18] mmc: extend the mmc hardware partitioning API with write reliability
Pantelis Antoniou
panto at antoniou-consulting.com
Fri Nov 28 15:20:47 CET 2014
Hi Diego,
> On Nov 28, 2014, at 12:57 , Diego Santa Cruz <Diego.SantaCruz at spinetix.com> wrote:
>
> Hi Pantelis,
>
>> Hi Diego,
>>
>> At first glance it's OK, I'm not the turning of user_enh_start, _size to a
>> structure
>> has a lot of benefits. It makes the diff longer.
>>
>
> I did that so that all the user data area members can be easily reset to zero when parsing the mmc hwpartition sub-command arguments, in particular if any more members get added in the future, as it is easy to forget to add a reset to zero when adding a new member to the main structure. Keeping all of them in a sub-structure allows to just memset the sub-structure to zero and not worry about members that may get added in the future.
>
>>> struct mmc_hwpart_conf {
>>> - uint user_enh_start; /* in 512-byte sectors */
>>> - uint user_enh_size; /* in 512-byte sectors, if 0 no enhanced user data
>> */
>>> + struct {
>>> + uint enh_start; /* in 512-byte sectors */
>>> + uint enh_size; /* in 512-byte sectors, if 0 no enhanced user
>> data */
>>> + unsigned wr_rel_change : 1;
>>> + unsigned wr_rel_set : 1;
>>
>> How about user_wr_rel_change : 1; & wr_rel_set : 1;
>>
>> It will cut down on the diff size.
>>
>
> I am not sure I fully understand your comment. You mean not using a struct and just put them in the main struct with the user_ prefix?
>
I think the source of the confusion is that you’ve introduced user_enh_start, user_enh_size in one patch and then modify them
again in another. That causes diff changes to show up due to the introduction of the structure. How about declaring the structure
in the first patch and the new members in another?
> Best,
>
> Diego
>
Regards
— Pantelis
> --
> Diego Santa Cruz, PhD
> Technology Architect
> spinetix.com
>
>
>> -----Original Message-----
>> From: Pantelis Antoniou [mailto:panto at antoniou-consulting.com]
>> Sent: Friday, November 28, 2014 11:09 AM
>> To: Diego Santa Cruz
>> Cc: u-boot at lists.denx.de
>> Subject: Re: [PATCH 16/18] mmc: extend the mmc hardware partitioning API with
>> write reliability
>>
>>> On Nov 28, 2014, at 11:10 , Diego Santa Cruz <Diego.SantaCruz at spinetix.com>
>> wrote:
>>>
>>> The eMMC partition write reliability settings are to be set while
>>> partitioning a device, as per the eMMC spec, so changes to these
>>> attributes needs to be done in the hardware partitioning API.
>>> This commit adds such support.
>>> ---
>>> common/cmd_mmc.c | 21 +++++++++++----------
>>> drivers/mmc/mmc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++------
>>> include/mmc.h | 19 ++++++++++++++++---
>>> 3 files changed, 72 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
>>> index 3943fb7..c0a4a3e 100644
>>> --- a/common/cmd_mmc.c
>>> +++ b/common/cmd_mmc.c
>>> @@ -501,9 +501,10 @@ static int do_mmc_hwpartition(cmd_tbl_t *cmdtp, int
>> flag,
>>> if (!strcmp(argv[i], "userenh")) {
>>> if (i + 2 >= argc)
>>> return CMD_RET_USAGE;
>>> - pconf.user_enh_start =
>>> + memset(&pconf.user, 0, sizeof(pconf.user));
>>> + pconf.user.enh_start =
>>> simple_strtoul(argv[i+1], NULL, 10);
>>> - pconf.user_enh_size =
>>> + pconf.user.enh_size =
>>> simple_strtoul(argv[i+2], NULL, 10);
>>> i += 3;
>>> } else if (!strncmp(argv[i], "gp", 2) &&
>>> @@ -512,14 +513,14 @@ static int do_mmc_hwpartition(cmd_tbl_t *cmdtp, int
>> flag,
>>> if (i + 1 >= argc)
>>> return CMD_RET_USAGE;
>>> pidx = argv[i][2] - '1';
>>> + memset(&pconf.gp_part[pidx], 0,
>>> + sizeof(pconf.gp_part[pidx]));
>>> pconf.gp_part[pidx].size =
>>> simple_strtoul(argv[i+1], NULL, 10);
>>> - if (i + 2 < argc && !strcmp(argv[i+2], "enh")) {
>>> + i += 2;
>>> + if (i < argc && !strcmp(argv[i], "enh")) {
>>> pconf.gp_part[pidx].enhanced = 1;
>>> - i += 3;
>>> - } else {
>>> - pconf.gp_part[pidx].enhanced = 0;
>>> - i += 2;
>>> + i++;
>>> }
>>> } else if (!strcmp(argv[i], "check")) {
>>> mode = MMC_HWPART_CONF_CHECK;
>>> @@ -536,11 +537,11 @@ static int do_mmc_hwpartition(cmd_tbl_t *cmdtp, int
>> flag,
>>> }
>>>
>>> puts("Partition configuration:\n");
>>> - if (pconf.user_enh_size) {
>>> + if (pconf.user.enh_size) {
>>> puts("\tUser Enhanced Start: ");
>>> - print_size(((u64)pconf.user_enh_start) << 9, "\n");
>>> + print_size(((u64)pconf.user.enh_start) << 9, "\n");
>>> puts("\tUser Enhanced Size: ");
>>> - print_size(((u64)pconf.user_enh_size) << 9, "\n");
>>> + print_size(((u64)pconf.user.enh_size) << 9, "\n");
>>> } else {
>>> puts("\tNo enhanced user data area\n");
>>> }
>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>> index 311097f..7cd21f2 100644
>>> --- a/drivers/mmc/mmc.c
>>> +++ b/drivers/mmc/mmc.c
>>> @@ -615,6 +615,7 @@ int mmc_hwpart_config(struct mmc *mmc,
>>> u32 gp_size_mult[4];
>>> u32 max_enh_size_mult;
>>> u32 tot_enh_size_mult = 0;
>>> + u8 wr_rel_set;
>>> int i, pidx, err;
>>> ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
>>>
>>> @@ -637,19 +638,19 @@ int mmc_hwpart_config(struct mmc *mmc,
>>> }
>>>
>>> /* check partition alignment and total enhanced size */
>>> - if (conf->user_enh_size) {
>>> - if (conf->user_enh_size % mmc->hc_wp_grp_size ||
>>> - conf->user_enh_start % mmc->hc_wp_grp_size) {
>>> + if (conf->user.enh_size) {
>>> + if (conf->user.enh_size % mmc->hc_wp_grp_size ||
>>> + conf->user.enh_start % mmc->hc_wp_grp_size) {
>>> printf("User data enhanced area not HC WP group "
>>> "size aligned\n");
>>> return -EINVAL;
>>> }
>>> part_attrs |= EXT_CSD_ENH_USR;
>>> - enh_size_mult = conf->user_enh_size / mmc->hc_wp_grp_size;
>>> + enh_size_mult = conf->user.enh_size / mmc->hc_wp_grp_size;
>>> if (mmc->high_capacity) {
>>> - enh_start_addr = conf->user_enh_start;
>>> + enh_start_addr = conf->user.enh_start;
>>> } else {
>>> - enh_start_addr = (conf->user_enh_start << 9);
>>> + enh_start_addr = (conf->user.enh_start << 9);
>>> }
>>> } else {
>>> enh_size_mult = 0;
>>> @@ -689,6 +690,33 @@ int mmc_hwpart_config(struct mmc *mmc,
>>> return -EMEDIUMTYPE;
>>> }
>>>
>>> + /* The default value of EXT_CSD_WR_REL_SET is device
>>> + * dependent, the values can only be changed if the
>>> + * EXT_CSD_HS_CTRL_REL bit is set. The values can be
>>> + * changed only once and before partitioning is completed. */
>>> + wr_rel_set = ext_csd[EXT_CSD_WR_REL_SET];
>>> + if (conf->user.wr_rel_change) {
>>> + if (conf->user.wr_rel_set)
>>> + wr_rel_set |= EXT_CSD_WR_DATA_REL_USR;
>>> + else
>>> + wr_rel_set &= ~EXT_CSD_WR_DATA_REL_USR;
>>> + }
>>> + for (pidx = 0; pidx < 4; pidx++) {
>>> + if (conf->gp_part[pidx].wr_rel_change) {
>>> + if (conf->gp_part[pidx].wr_rel_set)
>>> + wr_rel_set |= EXT_CSD_WR_DATA_REL_GP(pidx);
>>> + else
>>> + wr_rel_set &= ~EXT_CSD_WR_DATA_REL_GP(pidx);
>>> + }
>>> + }
>>> +
>>> + if (wr_rel_set != ext_csd[EXT_CSD_WR_REL_SET] &&
>>> + !(ext_csd[EXT_CSD_WR_REL_PARAM] & EXT_CSD_HS_CTRL_REL)) {
>>> + puts("Card does not support host controlled partition write "
>>> + "reliability settings\n");
>>> + return -EMEDIUMTYPE;
>>> + }
>>> +
>>> if (ext_csd[EXT_CSD_PART_SETTING_COMPLETED] & PART_SETTING_COMPLETED) {
>>> printf("Card already partitioned\n");
>>> return -EPERM;
>>> @@ -745,6 +773,17 @@ int mmc_hwpart_config(struct mmc *mmc,
>>> if (mode == MMC_HWPART_CONF_SET)
>>> return 0;
>>>
>>> + /* The WR_REL_SET is a write-once register but shall be
>>> + * written before setting PART_SETTING_COMPLETED. As it is
>>> + * write-once we can only write it when completing the
>>> + * partitioning. */
>>> + if (wr_rel_set != ext_csd[EXT_CSD_WR_REL_SET]) {
>>> + err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
>>> + EXT_CSD_WR_REL_SET, wr_rel_set);
>>> + if (err)
>>> + return err;
>>> + }
>>> +
>>> /* Setting PART_SETTING_COMPLETED confirms the partition
>>> * configuration but it only becomes effective after power
>>> * cycle, so we do not adjust the partition related settings
>>> diff --git a/include/mmc.h b/include/mmc.h
>>> index 7892880..33595f0 100644
>>> --- a/include/mmc.h
>>> +++ b/include/mmc.h
>>> @@ -154,6 +154,8 @@
>>> #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_WR_REL_PARAM 166 /* R */
>>> +#define EXT_CSD_WR_REL_SET 167 /* R/W */
>>> #define EXT_CSD_RPMB_MULT 168 /* RO */
>>> #define EXT_CSD_ERASE_GROUP_DEF 175 /* R/W */
>>> #define EXT_CSD_BOOT_BUS_WIDTH 177
>>> @@ -204,6 +206,11 @@
>>> #define EXT_CSD_ENH_USR (1 << 0) /* user data area is enhanced */
>>> #define EXT_CSD_ENH_GP(x) (1 << ((x)+1)) /* GP part (x+1) is
>> enhanced */
>>>
>>> +#define EXT_CSD_HS_CTRL_REL (1 << 0) /* host controlled WR_REL_SET */
>>> +
>>> +#define EXT_CSD_WR_DATA_REL_USR (1 << 0) /* user data area
>> WR_REL */
>>> +#define EXT_CSD_WR_DATA_REL_GP(x) (1 << ((x)+1)) /* GP part (x+1)
>> WR_REL */
>>> +
>>> #define R1_ILLEGAL_COMMAND (1 << 22)
>>> #define R1_APP_CMD (1 << 5)
>>>
>>> @@ -331,11 +338,17 @@ struct mmc {
>>> };
>>>
>>> + } user;
>>> struct {
>>> uint size; /* in 512-byte sectors */
>>> - int enhanced;
>>> + unsigned enhanced : 1;
>>> + unsigned wr_rel_change : 1;
>>> + unsigned wr_rel_set : 1;
>>> } gp_part[4];
>>> };
>>>
>>> --
>>> 1.7.1
>>>
>>
>> Regards
>>
>> - Pantelis
>
More information about the U-Boot
mailing list