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

Reply via email to