Re: [U-Boot] [PATCH 16/18] mmc: extend the mmc hardware partitioning API with write reliability

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.SantaCruz@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