[PATCH 1/2] mmc: add helper to query max enhanced part size

[PATCH 1/2] mmc: add helper to query max enhanced part size

[Matthias Schiffer]

From: Markus Niebel <Markus.Niebel@ew.tq-group.com>

This helper will be used later on in an extension of the mmc
command.

Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/mmc/mmc.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/mmc.h     |  1 +
 2 files changed, 39 insertions(+)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index d3babbfeb1c..c1b1ef7eb0b 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1039,6 +1039,44 @@ int mmc_switch_part(struct mmc *mmc, unsigned int part_num)
 }
 
 #if CONFIG_IS_ENABLED(MMC_HW_PARTITIONING)
+int mmc_max_enhanced_size_sectors(struct mmc *mmc, u64 *size)
+{
+	u64 sz;
+	int err;
+
+	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
+
+	if (IS_SD(mmc) || mmc->version < MMC_VERSION_4_41) {
+		pr_err("eMMC >= 4.4 required for enhanced user data area\n");
+		return -EMEDIUMTYPE;
+	}
+
+	if (!(mmc->part_support & PART_SUPPORT)) {
+		pr_err("Card does not support partitioning\n");
+		return -EMEDIUMTYPE;
+	}
+
+	if (!mmc->hc_wp_grp_size) {
+		pr_err("Card does not define HC WP group size\n");
+		return -EMEDIUMTYPE;
+	}
+
+	err = mmc_send_ext_csd(mmc, ext_csd);
+	if (err)
+		return err;
+
+	sz =
+		(ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 2] << 16) +
+		(ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 1] << 8) +
+		ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT];
+	sz *= ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE];
+	sz *= ext_csd[EXT_CSD_HC_WP_GRP_SIZE];
+	sz *= SZ_1K;
+	*size = sz;
+
+	return 0;
+}
+
 int mmc_hwpart_config(struct mmc *mmc,
 		      const struct mmc_hwpart_conf *conf,
 		      enum mmc_hwpart_conf_mode mode)
diff --git a/include/mmc.h b/include/mmc.h
index b92e2553402..3e1fc82d9b4 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -846,6 +846,7 @@ void print_mmc_devices(char separator);
  */
 int get_mmc_num(void);
 int mmc_switch_part(struct mmc *mmc, unsigned int part_num);
+int mmc_max_enhanced_size_sectors(struct mmc *mmc, u64 *size);
 int mmc_hwpart_config(struct mmc *mmc, const struct mmc_hwpart_conf *conf,
 		      enum mmc_hwpart_conf_mode mode);
 
-- 
2.17.1

[PATCH 2/2] cmd/mmc: add subcommand to query max enhanced partition size

[Matthias Schiffer]

From: Markus Niebel <Markus.Niebel@ew.tq-group.com>

The new command prints the sector count and size in a human-readable
format and sets an environment variable for scripted handling. The
variable value is set in decimal to match what the 'mmc hwpartition'
command expects.

The environment variable can be used for automated partitioning scripts,
for example the following would convert a whole eMMC to pSLC mode:

    mmc maxhwpartsectors
    mmc hwpartition user enh 0 ${maxhwpartsectors} wrrel on complete

Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

The human-readable output of the command could also be added to `mmc info`,
but it would still be great to have a separate command that sets an
environment variable for scripting, like this patch adds.


 cmd/mmc.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/cmd/mmc.c b/cmd/mmc.c
index f1e30d0cf64..d0b33cc0494 100644
--- a/cmd/mmc.c
+++ b/cmd/mmc.c
@@ -593,6 +593,33 @@ static int do_mmc_list(struct cmd_tbl *cmdtp, int flag,
 }
 
 #if CONFIG_IS_ENABLED(MMC_HW_PARTITIONING)
+static int do_mmc_maxhwpartsectors(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[])
+{
+	struct mmc *mmc;
+	u64 sectors;
+
+	mmc = init_mmc_device(curr_device, false);
+	if (!mmc)
+		return CMD_RET_FAILURE;
+
+	if (mmc_max_enhanced_size_sectors(mmc, &sectors))
+		return CMD_RET_FAILURE;
+
+	/* Ensure that the value fits in mmc_hwpart_conf::user.enh_size */
+	if (sectors > UINT_MAX) {
+		puts("ERROR: sector count larger than UINT_MAX\n");
+		return CMD_RET_FAILURE;
+	}
+
+	env_set_ulong("maxhwpartsectors", sectors);
+
+	printf("Maximum size of hardware partition: %u sectors (",
+	       (uint)sectors);
+	print_size(sectors * 512, ")\n");
+
+	return 0;
+}
+
 static int parse_hwpart_user(struct mmc_hwpart_conf *pconf,
 			     int argc, char *const argv[])
 {
@@ -1021,6 +1048,7 @@ static struct cmd_tbl cmd_mmc[] = {
 	U_BOOT_CMD_MKENT(dev, 4, 0, do_mmc_dev, "", ""),
 	U_BOOT_CMD_MKENT(list, 1, 1, do_mmc_list, "", ""),
 #if CONFIG_IS_ENABLED(MMC_HW_PARTITIONING)
+	U_BOOT_CMD_MKENT(maxhwpartsectors, 1, 0, do_mmc_maxhwpartsectors, "", ""),
 	U_BOOT_CMD_MKENT(hwpartition, 28, 0, do_mmc_hwpartition, "", ""),
 #endif
 #ifdef CONFIG_SUPPORT_EMMC_BOOT
@@ -1084,6 +1112,8 @@ U_BOOT_CMD(
 	"mmc list - lists available devices\n"
 	"mmc wp - power on write protect boot partitions\n"
 #if CONFIG_IS_ENABLED(MMC_HW_PARTITIONING)
+	"mmc maxhwpartsectors - shows the maximum number of 512-byte blocks usable for hardware partitioning\n"
+	"  Sets env var maxhwpartsectors on success.\n"
 	"mmc hwpartition <USER> <GP> <MODE> - does hardware partitioning\n"
 	"  arguments (sizes in 512-byte blocks):\n"
 	"   USER - <user> <enh> <start> <cnt> <wrrel> <{on|off}>\n"
-- 
2.17.1

Re: [PATCH 2/2] cmd/mmc: add subcommand to query max enhanced partition size

[Jaehoon Chung]

On 9/22/21 9:30 PM, Matthias Schiffer wrote:
> From: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> 
> The new command prints the sector count and size in a human-readable
> format and sets an environment variable for scripted handling. The
> variable value is set in decimal to match what the 'mmc hwpartition'
> command expects.
> 
> The environment variable can be used for automated partitioning scripts,
> for example the following would convert a whole eMMC to pSLC mode:
> 
>     mmc maxhwpartsectors
>     mmc hwpartition user enh 0 ${maxhwpartsectors} wrrel on complete


I don't have any objection about this patch.
If we can use more simpler variable name than maxhwpartsectors, I think that it's more useful.
But it's not important. :)

And Could you update doc/usage/mmc.rst about this command?

Best Regards,
Jaehoon Chung

> 
> Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
> 
> The human-readable output of the command could also be added to `mmc info`,
> but it would still be great to have a separate command that sets an
> environment variable for scripting, like this patch adds.
> 
> 
>  cmd/mmc.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/cmd/mmc.c b/cmd/mmc.c
> index f1e30d0cf64..d0b33cc0494 100644
> --- a/cmd/mmc.c
> +++ b/cmd/mmc.c
> @@ -593,6 +593,33 @@ static int do_mmc_list(struct cmd_tbl *cmdtp, int flag,
>  }
>  
>  #if CONFIG_IS_ENABLED(MMC_HW_PARTITIONING)
> +static int do_mmc_maxhwpartsectors(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	struct mmc *mmc;
> +	u64 sectors;
> +
> +	mmc = init_mmc_device(curr_device, false);
> +	if (!mmc)
> +		return CMD_RET_FAILURE;
> +
> +	if (mmc_max_enhanced_size_sectors(mmc, &sectors))
> +		return CMD_RET_FAILURE;
> +
> +	/* Ensure that the value fits in mmc_hwpart_conf::user.enh_size */
> +	if (sectors > UINT_MAX) {
> +		puts("ERROR: sector count larger than UINT_MAX\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	env_set_ulong("maxhwpartsectors", sectors);
> +
> +	printf("Maximum size of hardware partition: %u sectors (",
> +	       (uint)sectors);
> +	print_size(sectors * 512, ")\n");
> +
> +	return 0;
> +}
> +
>  static int parse_hwpart_user(struct mmc_hwpart_conf *pconf,
>  			     int argc, char *const argv[])
>  {
> @@ -1021,6 +1048,7 @@ static struct cmd_tbl cmd_mmc[] = {
>  	U_BOOT_CMD_MKENT(dev, 4, 0, do_mmc_dev, "", ""),
>  	U_BOOT_CMD_MKENT(list, 1, 1, do_mmc_list, "", ""),
>  #if CONFIG_IS_ENABLED(MMC_HW_PARTITIONING)
> +	U_BOOT_CMD_MKENT(maxhwpartsectors, 1, 0, do_mmc_maxhwpartsectors, "", ""),
>  	U_BOOT_CMD_MKENT(hwpartition, 28, 0, do_mmc_hwpartition, "", ""),
>  #endif
>  #ifdef CONFIG_SUPPORT_EMMC_BOOT
> @@ -1084,6 +1112,8 @@ U_BOOT_CMD(
>  	"mmc list - lists available devices\n"
>  	"mmc wp - power on write protect boot partitions\n"
>  #if CONFIG_IS_ENABLED(MMC_HW_PARTITIONING)
> +	"mmc maxhwpartsectors - shows the maximum number of 512-byte blocks usable for hardware partitioning\n"
> +	"  Sets env var maxhwpartsectors on success.\n"
>  	"mmc hwpartition <USER> <GP> <MODE> - does hardware partitioning\n"
>  	"  arguments (sizes in 512-byte blocks):\n"
>  	"   USER - <user> <enh> <start> <cnt> <wrrel> <{on|off}>\n"
> 

Re: [PATCH 1/2] mmc: add helper to query max enhanced part size

[Jaehoon Chung]

Hi,

On 9/22/21 9:30 PM, Matthias Schiffer wrote:
> From: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> 
> This helper will be used later on in an extension of the mmc
> command.
> 
> Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/mmc/mmc.c | 38 ++++++++++++++++++++++++++++++++++++++
>  include/mmc.h     |  1 +
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index d3babbfeb1c..c1b1ef7eb0b 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -1039,6 +1039,44 @@ int mmc_switch_part(struct mmc *mmc, unsigned int part_num)
>  }
>  
>  #if CONFIG_IS_ENABLED(MMC_HW_PARTITIONING)
> +int mmc_max_enhanced_size_sectors(struct mmc *mmc, u64 *size)
> +{
> +	u64 sz;
> +	int err;
> +
> +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
> +
> +	if (IS_SD(mmc) || mmc->version < MMC_VERSION_4_41) {
> +		pr_err("eMMC >= 4.4 required for enhanced user data area\n");

Error log is considering about only eMMC. It can be SD-card.

> +		return -EMEDIUMTYPE;
> +	}
> +
> +	if (!(mmc->part_support & PART_SUPPORT)) {
> +		pr_err("Card does not support partitioning\n");
> +		return -EMEDIUMTYPE;
> +	}
> +
> +	if (!mmc->hc_wp_grp_size) {
> +		pr_err("Card does not define HC WP group size\n");
> +		return -EMEDIUMTYPE;
> +	}
> +
> +	err = mmc_send_ext_csd(mmc, ext_csd);
> +	if (err)
> +		return err;
> +
> +	sz =
> +		(ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 2] << 16) +
> +		(ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 1] << 8) +
> +		ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT];
> +	sz *= ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE];
> +	sz *= ext_csd[EXT_CSD_HC_WP_GRP_SIZE];
> +	sz *= SZ_1K;

To use the num of sector, i think better that is adding Comment.
And using not "sz" as variable. It seems to describe real size, not sector.
According to spec, it's 512kByte. It can be confused.

Best Regards,
Jaehoon Chung

> +	*size = sz;
> +
> +	return 0;
> +}
> +
>  int mmc_hwpart_config(struct mmc *mmc,
>  		      const struct mmc_hwpart_conf *conf,
>  		      enum mmc_hwpart_conf_mode mode)
> diff --git a/include/mmc.h b/include/mmc.h
> index b92e2553402..3e1fc82d9b4 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -846,6 +846,7 @@ void print_mmc_devices(char separator);
>   */
>  int get_mmc_num(void);
>  int mmc_switch_part(struct mmc *mmc, unsigned int part_num);
> +int mmc_max_enhanced_size_sectors(struct mmc *mmc, u64 *size);
>  int mmc_hwpart_config(struct mmc *mmc, const struct mmc_hwpart_conf *conf,
>  		      enum mmc_hwpart_conf_mode mode);
>  
> 

Re: [PATCH 1/2] mmc: add helper to query max enhanced part size

[Matthias Schiffer]

On Thu, 2021-09-23 at 20:58 +0900, Jaehoon Chung wrote:
> Hi,
> 
> On 9/22/21 9:30 PM, Matthias Schiffer wrote:
> > From: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> > 
> > This helper will be used later on in an extension of the mmc
> > command.
> > 
> > Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  drivers/mmc/mmc.c | 38 ++++++++++++++++++++++++++++++++++++++
> >  include/mmc.h     |  1 +
> >  2 files changed, 39 insertions(+)
> > 
> > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> > index d3babbfeb1c..c1b1ef7eb0b 100644
> > --- a/drivers/mmc/mmc.c
> > +++ b/drivers/mmc/mmc.c
> > @@ -1039,6 +1039,44 @@ int mmc_switch_part(struct mmc *mmc, unsigned int part_num)
> >  }
> >  
> >  #if CONFIG_IS_ENABLED(MMC_HW_PARTITIONING)
> > +int mmc_max_enhanced_size_sectors(struct mmc *mmc, u64 *size)
> > +{
> > +	u64 sz;
> > +	int err;
> > +
> > +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
> > +
> > +	if (IS_SD(mmc) || mmc->version < MMC_VERSION_4_41) {
> > +		pr_err("eMMC >= 4.4 required for enhanced user data area\n");
> 
> Error log is considering about only eMMC. It can be SD-card.

This check and message were taken from mmc_hwpart_config(). I think it
is okay (after all it tells you "eMMC [...] required [...]" if you try
the command on an SD card), but I can extend the message if you want.

I also noticed another slight difference between the check and the
message: The check is for eMMC 4.41, while the message talks about eMMC
4.4. I'd like to make both match, but I don't know whether 4.4 or 4.41
is the correct requirement.


> 
> > +		return -EMEDIUMTYPE;
> > +	}
> > +
> > +	if (!(mmc->part_support & PART_SUPPORT)) {
> > +		pr_err("Card does not support partitioning\n");
> > +		return -EMEDIUMTYPE;
> > +	}
> > +
> > +	if (!mmc->hc_wp_grp_size) {
> > +		pr_err("Card does not define HC WP group size\n");
> > +		return -EMEDIUMTYPE;
> > +	}
> > +
> > +	err = mmc_send_ext_csd(mmc, ext_csd);
> > +	if (err)
> > +		return err;
> > +
> > +	sz =
> > +		(ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 2] << 16) +
> > +		(ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 1] << 8) +
> > +		ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT];
> > +	sz *= ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE];
> > +	sz *= ext_csd[EXT_CSD_HC_WP_GRP_SIZE];
> > +	sz *= SZ_1K;
> 
> To use the num of sector, i think better that is adding Comment.
> And using not "sz" as variable. It seems to describe real size, not sector.
> According to spec, it's 512kByte. It can be confused.

Makes sense, I'll change the variable name.

> 
> Best Regards,
> Jaehoon Chung
> 
> > +	*size = sz;
> > +
> > +	return 0;
> > +}
> > +
> >  int mmc_hwpart_config(struct mmc *mmc,
> >  		      const struct mmc_hwpart_conf *conf,
> >  		      enum mmc_hwpart_conf_mode mode)
> > diff --git a/include/mmc.h b/include/mmc.h
> > index b92e2553402..3e1fc82d9b4 100644
> > --- a/include/mmc.h
> > +++ b/include/mmc.h
> > @@ -846,6 +846,7 @@ void print_mmc_devices(char separator);
> >   */
> >  int get_mmc_num(void);
> >  int mmc_switch_part(struct mmc *mmc, unsigned int part_num);
> > +int mmc_max_enhanced_size_sectors(struct mmc *mmc, u64 *size);
> >  int mmc_hwpart_config(struct mmc *mmc, const struct mmc_hwpart_conf *conf,
> >  		      enum mmc_hwpart_conf_mode mode);
> >  
> > 
> 
>