[PATCH v4 0/2] simplify setting enhanced area of mmc devices

[PATCH v4 0/2] simplify setting enhanced area of mmc devices

[Uwe Kleine-König]

Hello,

v3 of this series is was sent in October 2018. This v4 addresses the
feedback I got then by Sascha: add a call to free that I missed and use
getopt instead of parsing the options by hand. I also dropped the
mandatory "setmax" subcommand. With option parsing the new command is
flexible enough to be expanded in the future for setting other sizes
than maximal.

The first patch is a small cleanup and unchanged since v3.

Best regards
Uwe

Uwe Kleine-König (2):
  mci: provide wrapper for mci_get_device_by_name ∘ devpath_to_name
  mci: implement command to switch a mmc device to enhanced mode

 commands/Kconfig      |  11 +++
 commands/Makefile     |   1 +
 commands/mmc.c        | 200 ++++++++++++++++++++++++++++++++++++++++++
 commands/mmc_extcsd.c |   6 +-
 include/mci.h         |  13 +++
 5 files changed, 228 insertions(+), 3 deletions(-)
 create mode 100644 commands/mmc.c

-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v4 1/2] mci: provide wrapper for mci_get_device_by_name ∘ devpath_to_name

[Uwe Kleine-König]

Also convert the only user of mci_get_device_by_name to this new
wrapper.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 commands/mmc_extcsd.c | 6 +++---
 include/mci.h         | 6 ++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/commands/mmc_extcsd.c b/commands/mmc_extcsd.c
index 889a6c614a47..55decd618553 100644
--- a/commands/mmc_extcsd.c
+++ b/commands/mmc_extcsd.c
@@ -2366,7 +2366,7 @@ static int do_mmc_extcsd(int argc, char *argv[])
 	u8			*dst;
 	int			retval = 0;
 	int			opt;
-	char			*devname;
+	char			*devpath;
 	int			index = 0;
 	int			value = 0;
 	int			write_operation = 0;
@@ -2404,9 +2404,9 @@ static int do_mmc_extcsd(int argc, char *argv[])
 	if (optind == argc)
 		return COMMAND_ERROR_USAGE;
 
-	devname = argv[optind];
+	devpath = argv[optind];
 
-	mci = mci_get_device_by_name(devpath_to_name(devname));
+	mci = mci_get_device_by_devpath(devpath);
 	if (mci == NULL) {
 		retval = -ENOENT;
 		goto error;
diff --git a/include/mci.h b/include/mci.h
index 072008ef9da7..5d0c2f8f9638 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -28,6 +28,7 @@
 
 #include <linux/list.h>
 #include <block.h>
+#include <fs.h>
 #include <regulator.h>
 
 /* These codes should be sorted numerically in order of newness.  If the last
@@ -490,4 +491,9 @@ static inline int mmc_host_is_spi(struct mci_host *host)
 
 struct mci *mci_get_device_by_name(const char *name);
 
+static inline struct mci *mci_get_device_by_devpath(const char *devpath)
+{
+	return mci_get_device_by_name(devpath_to_name(devpath));
+}
+
 #endif /* _MCI_H_ */
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v4 2/2] mci: implement command to switch a mmc device to enhanced mode

[Uwe Kleine-König]

The command structure allows adding more subcommands and is designed to
match the Linux program mmc from the mmc-utils. So later more commands
can easily be added if need be.

Compared to mmc-utils'

	mmc enh_area set <-y|-n|-c> <start KiB> <length KiB> <device>

the command that is implemented here (

	mmc enh_area [-c] <device>

) is easier to use (because you don't have to check the maximal allowed
size by reading some registers and calculate the available size from
them (which then must be calculated back to register values by the mmc
command)) but less flexible as it doesn't allow all the crazy
possibilities specified in the eMMC standard (yet?) but just creates an
enhanced area with maximal size.

In the future something like

	mmc enh_area -s 30k <device>

could be used to not use the maximal but an explicit size.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 commands/Kconfig  |  11 +++
 commands/Makefile |   1 +
 commands/mmc.c    | 200 ++++++++++++++++++++++++++++++++++++++++++++++
 include/mci.h     |   7 ++
 4 files changed, 219 insertions(+)
 create mode 100644 commands/mmc.c

diff --git a/commands/Kconfig b/commands/Kconfig
index 039fd7d1ac74..2a26b2678e84 100644
--- a/commands/Kconfig
+++ b/commands/Kconfig
@@ -233,6 +233,17 @@ config CMD_VERSION
 
 	  barebox 2014.05.0-00142-gb289373 #177 Mon May 12 20:35:55 CEST 2014
 
+config CMD_MMC
+	tristate
+	prompt "mmc command allowing to set enhanced area"
+	depends on MCI
+	help
+	  Configure mmc cards similar to the userspace mmc utility. Compared to
+	  mmc_extcsd it works on a higher abstraction level.
+
+	  Currently only the enh_area subcommand is implemented to configure
+	  the "Enhanced Area" of an mmc device.
+
 config CMD_MMC_EXTCSD
 	tristate
 	prompt "read/write eMMC ext. CSD register"
diff --git a/commands/Makefile b/commands/Makefile
index e69fb5046f59..87d7636d2188 100644
--- a/commands/Makefile
+++ b/commands/Makefile
@@ -119,6 +119,7 @@ obj-$(CONFIG_CMD_DHCP)		+= dhcp.o
 obj-$(CONFIG_CMD_BOOTCHOOSER)	+= bootchooser.o
 obj-$(CONFIG_CMD_DHRYSTONE)	+= dhrystone.o
 obj-$(CONFIG_CMD_SPD_DECODE)	+= spd_decode.o
+obj-$(CONFIG_CMD_MMC)		+= mmc.o
 obj-$(CONFIG_CMD_MMC_EXTCSD)	+= mmc_extcsd.o
 obj-$(CONFIG_CMD_NAND_BITFLIP)	+= nand-bitflip.o
 obj-$(CONFIG_CMD_SEED)		+= seed.o
diff --git a/commands/mmc.c b/commands/mmc.c
new file mode 100644
index 000000000000..b51522fce2a6
--- /dev/null
+++ b/commands/mmc.c
@@ -0,0 +1,200 @@
+#include <command.h>
+#include <mci.h>
+#include <stdio.h>
+#include <string.h>
+#include <getopt.h>
+
+static int mmc_enh_area_setmax(struct mci *mci, u8 *ext_csd)
+{
+	unsigned i;
+	struct {
+		unsigned index;
+		unsigned value;
+	} regval[] = {
+		{
+			.index = EXT_CSD_ERASE_GROUP_DEF,
+			.value = 1,
+		}, {
+			.index = EXT_CSD_ENH_START_ADDR,
+			.value = 0,
+		}, {
+			.index = EXT_CSD_ENH_START_ADDR + 1,
+			.value = 0,
+		}, {
+			.index = EXT_CSD_ENH_START_ADDR + 2,
+			.value = 0,
+		}, {
+			.index = EXT_CSD_ENH_START_ADDR + 3,
+			.value = 0,
+		}, {
+			.index = EXT_CSD_ENH_SIZE_MULT,
+			.value = ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT],
+		}, {
+			.index = EXT_CSD_ENH_SIZE_MULT + 1,
+			.value = ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 1],
+		}, {
+			.index = EXT_CSD_ENH_SIZE_MULT + 2,
+			.value = ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 2],
+		}, {
+			.index = EXT_CSD_PARTITIONS_ATTRIBUTE,
+			.value = ext_csd[EXT_CSD_PARTITIONS_ATTRIBUTE] | EXT_CSD_ENH_USR_MASK,
+		}
+	};
+
+	for (i = 0; i < ARRAY_SIZE(regval); ++i) {
+		int ret = mci_switch(mci, regval[i].index, regval[i].value);
+		if (ret) {
+			printf("Failure to write to register %u", regval[i].index);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int mmc_partitioning_complete(struct mci *mci)
+{
+	int ret;
+
+	ret = mci_switch(mci, EXT_CSD_PARTITION_SETTING_COMPLETED, 1);
+	if (ret)
+		printf("Failure to write to EXT_CSD_PARTITION_SETTING_COMPLETED\n");
+
+	return ret;
+}
+
+static u8 *mci_get_ext_csd(struct mci *mci)
+{
+	u8 *ext_csd;
+	int ret;
+
+	ext_csd = xmalloc(512);
+
+	ret = mci_send_ext_csd(mci, ext_csd);
+	if (ret) {
+		printf("Failure to read EXT_CSD register\n");
+		free(ext_csd);
+		return ERR_PTR(-EIO);
+	}
+
+	return ext_csd;
+}
+
+/* enh_area -m [-c] /dev/mmcX */
+static int do_mmc_enh_area(int argc, char *argv[])
+{
+	const char *devpath;
+	struct mci *mci;
+	u8 *ext_csd;
+	int set_completed = 0;
+	int opt;
+	int ret;
+
+	while ((opt = getopt(argc, argv, "c")) > 0) {
+		switch (opt) {
+		case 'c':
+			set_completed = 1;
+			break;
+		}
+	}
+
+	/*
+	 * -m is currently mandatory to allow more flexible enhanced area
+	 * specifications in the future.
+	 */
+	if (argc - optind != 1) {
+		printf("Usage: mmc enh_area [-c] /dev/mmcX\n");
+		return COMMAND_ERROR_USAGE;
+	}
+
+	devpath = argv[optind];
+
+	mci = mci_get_device_by_devpath(devpath);
+	if (!mci) {
+		printf("Failure to open %s as mci device\n", devpath);
+		return COMMAND_ERROR;
+	}
+
+	ext_csd = mci_get_ext_csd(mci);
+	if (IS_ERR(ext_csd))
+		return COMMAND_ERROR;
+
+	if (!(ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & EXT_CSD_ENH_ATTRIBUTE_EN_MASK)) {
+		printf("Device doesn't support enhanced area\n");
+		goto error;
+	}
+
+	if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED]) {
+		printf("Partitioning already finalized\n");
+		goto error;
+	}
+
+	ret = mmc_enh_area_setmax(mci, ext_csd);
+	if (ret)
+		goto error;
+
+	free(ext_csd);
+
+	if (set_completed) {
+		ret = mmc_partitioning_complete(mci);
+		if (ret)
+			return COMMAND_ERROR;
+		printf("Now power cycle the device to let it reconfigure itself.\n");
+	}
+
+	return COMMAND_SUCCESS;
+
+error:
+	free(ext_csd);
+	return COMMAND_ERROR;
+}
+
+static struct {
+	const char *cmd;
+	int (*func)(int argc, char *argv[]);
+} mmc_subcmds[] = {
+	{
+		.cmd = "enh_area",
+		.func = do_mmc_enh_area,
+	}
+};
+
+static int do_mmc(int argc, char *argv[])
+{
+	size_t i;
+	int (*func)(int argc, char *argv[]) = NULL;
+
+	if (argc < 2) {
+		printf("mmc: required subcommand missing\n");
+		return 1;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(mmc_subcmds); ++i) {
+		if (strcmp(mmc_subcmds[i].cmd, argv[1]) == 0) {
+			func = mmc_subcmds[i].func;
+			break;
+		}
+	}
+
+	if (func) {
+		return func(argc - 1, argv + 1);
+	} else {
+		printf("mmc: subcommand \"%s\" not found\n", argv[1]);
+		return COMMAND_ERROR_USAGE;
+	}
+}
+
+BAREBOX_CMD_HELP_START(mmc)
+BAREBOX_CMD_HELP_TEXT("Modifies mmc properties.")
+BAREBOX_CMD_HELP_TEXT("")
+BAREBOX_CMD_HELP_TEXT("The subcommand enh_area creates an enhanced area of")
+BAREBOX_CMD_HELP_TEXT("maximal size.")
+BAREBOX_CMD_HELP_TEXT("Note, with -c this is an irreversible action.")
+BAREBOX_CMD_HELP_OPT("-c", "complete partitioning")
+BAREBOX_CMD_HELP_END
+
+BAREBOX_CMD_START(mmc)
+	.cmd = do_mmc,
+	BAREBOX_CMD_OPTS("enh_area [-c] /dev/mmcX")
+	BAREBOX_CMD_HELP(cmd_mmc_help)
+BAREBOX_CMD_END
diff --git a/include/mci.h b/include/mci.h
index 5d0c2f8f9638..c1645fa07d85 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -305,6 +305,13 @@
 #define EXT_CSD_CARD_TYPE_SDR_1_2V	(1<<5)	/* Card can run at 200MHz */
 						/* SDR mode @1.2V I/O */
 
+/* register PARTITIONS_ATTRIBUTE [156] */
+#define EXT_CSD_ENH_USR_MASK		(1 << 0)
+
+/* register PARTITIONING_SUPPORT [160] */
+#define EXT_CSD_ENH_ATTRIBUTE_EN_MASK	(1 << 0)
+
+/* register BUS_WIDTH [183], field Bus Mode Selection [4:0] */
 #define EXT_CSD_BUS_WIDTH_1	0	/* Card is in 1 bit mode */
 #define EXT_CSD_BUS_WIDTH_4	1	/* Card is in 4 bit mode */
 #define EXT_CSD_BUS_WIDTH_8	2	/* Card is in 8 bit mode */
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v4 2/2] mci: implement command to switch a mmc device to enhanced mode

[Sascha Hauer]

On Mon, Sep 09, 2019 at 10:34:51PM +0200, Uwe Kleine-König wrote:
> +	ret = mci_send_ext_csd(mci, ext_csd);
> +	if (ret) {
> +		printf("Failure to read EXT_CSD register\n");
> +		free(ext_csd);
> +		return ERR_PTR(-EIO);
> +	}
> +
> +	return ext_csd;
> +}
> +
> +/* enh_area -m [-c] /dev/mmcX */

-m is not implemented

> +static int do_mmc_enh_area(int argc, char *argv[])
> +{
> +	const char *devpath;
> +	struct mci *mci;
> +	u8 *ext_csd;
> +	int set_completed = 0;
> +	int opt;
> +	int ret;
> +
> +	while ((opt = getopt(argc, argv, "c")) > 0) {
> +		switch (opt) {
> +		case 'c':
> +			set_completed = 1;
> +			break;
> +		}
> +	}
> +
> +	/*
> +	 * -m is currently mandatory to allow more flexible enhanced area
> +	 * specifications in the future.
> +	 */

-m?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v4 2/2] mci: implement command to switch a mmc device to enhanced mode

[Uwe Kleine-König]

Hello,

On Thu, Sep 12, 2019 at 08:16:17AM +0200, Sascha Hauer wrote:
> On Mon, Sep 09, 2019 at 10:34:51PM +0200, Uwe Kleine-König wrote:
> > +	ret = mci_send_ext_csd(mci, ext_csd);
> > +	if (ret) {
> > +		printf("Failure to read EXT_CSD register\n");
> > +		free(ext_csd);
> > +		return ERR_PTR(-EIO);
> > +	}
> > +
> > +	return ext_csd;
> > +}
> > +
> > +/* enh_area -m [-c] /dev/mmcX */
> 
> -m is not implemented
> 
> > +static int do_mmc_enh_area(int argc, char *argv[])
> > +{
> > +	const char *devpath;
> > +	struct mci *mci;
> > +	u8 *ext_csd;
> > +	int set_completed = 0;
> > +	int opt;
> > +	int ret;
> > +
> > +	while ((opt = getopt(argc, argv, "c")) > 0) {
> > +		switch (opt) {
> > +		case 'c':
> > +			set_completed = 1;
> > +			break;
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * -m is currently mandatory to allow more flexible enhanced area
> > +	 * specifications in the future.
> > +	 */
> 
> -m?

-m was an intermediate step. You suggested to use -m instead of
"setmax", but while implementing I noticed that with options there is no
need for a keyword or option.

Can you please squash this into my patch?:

diff --git a/commands/mmc.c b/commands/mmc.c
index b51522fce2a6..c696e7b8817b 100644
--- a/commands/mmc.c
+++ b/commands/mmc.c
@@ -80,7 +80,7 @@ static u8 *mci_get_ext_csd(struct mci *mci)
 	return ext_csd;
 }
 
-/* enh_area -m [-c] /dev/mmcX */
+/* enh_area [-c] /dev/mmcX */
 static int do_mmc_enh_area(int argc, char *argv[])
 {
 	const char *devpath;
@@ -98,10 +98,6 @@ static int do_mmc_enh_area(int argc, char *argv[])
 		}
 	}
 
-	/*
-	 * -m is currently mandatory to allow more flexible enhanced area
-	 * specifications in the future.
-	 */
 	if (argc - optind != 1) {
 		printf("Usage: mmc enh_area [-c] /dev/mmcX\n");
 		return COMMAND_ERROR_USAGE;

or should I resend?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v4 2/2] mci: implement command to switch a mmc device to enhanced mode

[Sascha Hauer]

On Thu, Sep 12, 2019 at 08:42:40AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> -m was an intermediate step. You suggested to use -m instead of
> "setmax", but while implementing I noticed that with options there is no
> need for a keyword or option.
> 
> Can you please squash this into my patch?:
> 
> diff --git a/commands/mmc.c b/commands/mmc.c
> index b51522fce2a6..c696e7b8817b 100644
> --- a/commands/mmc.c
> +++ b/commands/mmc.c
> @@ -80,7 +80,7 @@ static u8 *mci_get_ext_csd(struct mci *mci)
>  	return ext_csd;
>  }
>  
> -/* enh_area -m [-c] /dev/mmcX */
> +/* enh_area [-c] /dev/mmcX */
>  static int do_mmc_enh_area(int argc, char *argv[])
>  {
>  	const char *devpath;
> @@ -98,10 +98,6 @@ static int do_mmc_enh_area(int argc, char *argv[])
>  		}
>  	}
>  
> -	/*
> -	 * -m is currently mandatory to allow more flexible enhanced area
> -	 * specifications in the future.
> -	 */
>  	if (argc - optind != 1) {
>  		printf("Usage: mmc enh_area [-c] /dev/mmcX\n");
>  		return COMMAND_ERROR_USAGE;
> 
> or should I resend?

squashed into the patch, no need to resend. Thanks,

 Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox