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? Best, Diego -- Diego Santa Cruz, PhD Technology Architect spinetix.com > -----Original Message----- > From: Pantelis Antoniou [mailto:pa...@antoniou-consulting.com] > Sent: Friday, November 28, 2014 11:09 AM > To: Diego Santa Cruz > Cc: u-boot@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.santac...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot