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.

> 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 {
> };
> 
> 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.

> +     } 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