[PATCH v2 1/2] mci: drop unused parameter from mci_switch()

[PATCH v2 1/2] mci: drop unused parameter from mci_switch()

[Uwe Kleine-König]

The SWITCH command has two purposes:
 a) switch the command set
 b) Write to the EXT_CSD register

If the access field (bits [25:24]) in the argument are b00, we're in
case a), otherwise in b). As mci_switch() always passes
MMC_SWITCH_MODE_WRITE_BYTE (0b3) in the access field, only case b) is
relevant here. According to the eMMC specification[1] the command set
field is ignored in case b) and so the respective parameter (that is
unused already now) can be dropped.

[1] Embedded Multi-Media Card (e•MMC) Electrical Standard (5.1),
    February 2015; paragraph 6.6.1

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

diff --git a/commands/mmc_extcsd.c b/commands/mmc_extcsd.c
index 7a6d39075da0..acd23a466bcb 100644
--- a/commands/mmc_extcsd.c
+++ b/commands/mmc_extcsd.c
@@ -2357,7 +2357,7 @@ static void write_field(struct mci *mci, u8 *reg, u16 index, u8 value,
 		break;
 	}
 
-	mci_switch(mci, 0, index, value);
+	mci_switch(mci, index, value);
 
 out:
 	return;
diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 07911d43d703..3da96f42aaf9 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -396,8 +396,7 @@ int mci_send_ext_csd(struct mci *mci, char *ext_csd)
  * @param value FIXME
  * @return Transaction status (0 on success)
  */
-int mci_switch(struct mci *mci, unsigned set, unsigned index,
-			unsigned value)
+int mci_switch(struct mci *mci, unsigned index, unsigned value)
 {
 	struct mci_cmd cmd;
 
@@ -471,7 +470,7 @@ static int mmc_change_freq(struct mci *mci)
 
 	cardtype = mci->ext_csd[EXT_CSD_DEVICE_TYPE] & EXT_CSD_CARD_TYPE_MASK;
 
-	err = mci_switch(mci, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);
+	err = mci_switch(mci, EXT_CSD_HS_TIMING, 1);
 
 	if (err) {
 		dev_dbg(&mci->dev, "MMC frequency changing failed: %d\n", err);
@@ -1044,9 +1043,7 @@ static int mci_startup_mmc(struct mci *mci)
 		 * 4bit transfer mode. On success set the corresponding
 		 * bus width on the host.
 		 */
-		err = mci_switch(mci, EXT_CSD_CMD_SET_NORMAL,
-				 EXT_CSD_BUS_WIDTH,
-				 ext_csd_bits[idx]);
+		err = mci_switch(mci, EXT_CSD_BUS_WIDTH, ext_csd_bits[idx]);
 		if (err) {
 			if (idx == 0)
 				dev_warn(&mci->dev, "Changing MMC bus width failed: %d\n", err);
@@ -1253,8 +1250,7 @@ static int mci_blk_part_switch(struct mci_part *part)
 		part_config &= ~EXT_CSD_PART_CONFIG_ACC_MASK;
 		part_config |= part->part_cfg;
 
-		ret = mci_switch(mci, EXT_CSD_CMD_SET_NORMAL,
-				EXT_CSD_PARTITION_CONFIG, part_config);
+		ret = mci_switch(mci, EXT_CSD_PARTITION_CONFIG, part_config);
 		if (ret)
 			return ret;
 
@@ -1568,8 +1564,8 @@ static int mci_set_boot(struct param_d *param, void *priv)
 	mci->ext_csd_part_config &= ~(7 << 3);
 	mci->ext_csd_part_config |= mci->bootpart << 3;
 
-	return mci_switch(mci, EXT_CSD_CMD_SET_NORMAL,
-			EXT_CSD_PARTITION_CONFIG, mci->ext_csd_part_config);
+	return mci_switch(mci,
+			  EXT_CSD_PARTITION_CONFIG, mci->ext_csd_part_config);
 }
 
 static const char *mci_boot_names[] = {
diff --git a/include/mci.h b/include/mci.h
index d3115e8cc68f..827eecd39f8d 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -477,8 +477,7 @@ void mci_of_parse(struct mci_host *host);
 void mci_of_parse_node(struct mci_host *host, struct device_node *np);
 int mci_detect_card(struct mci_host *);
 int mci_send_ext_csd(struct mci *mci, char *ext_csd);
-int mci_switch(struct mci *mci, unsigned set, unsigned index,
-			unsigned value);
+int mci_switch(struct mci *mci, unsigned index, unsigned value);
 
 static inline int mmc_host_is_spi(struct mci_host *host)
 {
-- 
2.11.0


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

[PATCH v2 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 setmax <-y|-n|-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 but just creates an
enhanced area with maximal size.

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

diff --git a/commands/Kconfig b/commands/Kconfig
index ae2dc4b0947b..1b29365a06f2 100644
--- a/commands/Kconfig
+++ b/commands/Kconfig
@@ -238,6 +238,11 @@ 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
+
 config CMD_MMC_EXTCSD
 	tristate
 	prompt "read/write eMMC ext. CSD register"
diff --git a/commands/Makefile b/commands/Makefile
index 37486dceb181..bcd0665cfe88 100644
--- a/commands/Makefile
+++ b/commands/Makefile
@@ -120,6 +120,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..2f5c7ad69a37
--- /dev/null
+++ b/commands/mmc.c
@@ -0,0 +1,171 @@
+#include <command.h>
+#include <mci.h>
+#include <stdio.h>
+#include <string.h>
+
+/* enh_area setmax <-y|-n|-c> /dev/mmcX */
+static int do_mmc_enh_area(int argc, char *argv[])
+{
+	char *devname;
+	struct mci *mci;
+	u8 *ext_csd;
+	int set_completed = 0;
+	int ret;
+
+	if (argc != 4 || strcmp(argv[1], "setmax") ||
+	    argv[2][0] != '-' ||
+	    (argv[2][1] != 'y' && argv[2][1] != 'n' && argv[2][1] != 'c')) {
+		printf("Usage: mmc enh_area setmax <-y|-n|-c> /dev/mmcX\n");
+		return 1;
+	}
+
+	if (argv[2][1] == 'y')
+		set_completed = 1;
+
+	devname = argv[3];
+	if (!strncmp(devname, "/dev/", 5))
+		devname += 5;
+
+	mci = mci_get_device_by_name(devname);
+	if (!mci) {
+		printf("Failure to open %s as mci device\n", devname);
+		return -ENOENT;
+	}
+	/* get extcsd */
+	ext_csd = xmalloc(512);
+
+	ret = mci_send_ext_csd(mci, ext_csd);
+	if (ret) {
+		printf("Failure to read EXT_CSD register\n");
+		goto error;
+	}
+
+	if (!(ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & EXT_CSD_ENH_ATTRIBUTE_EN_MASK)) {
+		printf("Device doesn't support enhanced area\n");
+		ret = -EIO;
+		goto error;
+	}
+
+	if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED]) {
+		printf("Partitioning already finalized\n");
+		ret = -EIO;
+		goto error;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ERASE_GROUP_DEF, 1);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ERASE_GROUP_DEF\n");
+		goto error;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR, 0);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_START_ADDR[0]\n");
+		goto error;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR + 1, 0);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_START_ADDR[1]\n");
+		goto error;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR + 2, 0);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_START_ADDR[2]\n");
+		goto error;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR + 3, 0);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_START_ADDR[3]\n");
+		goto error;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_SIZE_MULT, ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT]);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_SIZE_MULT[0]\n");
+		goto error;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_SIZE_MULT + 1, ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 1]);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_SIZE_MULT[1]\n");
+		goto error;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_SIZE_MULT + 2, ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 2]);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_SIZE_MULT[2]\n");
+		goto error;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_PARTITIONS_ATTRIBUTE, ext_csd[EXT_CSD_PARTITIONS_ATTRIBUTE] | EXT_CSD_ENH_USR_MASK);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_PARTITIONS_ATTRIBUTE\n");
+		goto error;
+	}
+
+	if (set_completed) {
+		ret = mci_switch(mci, EXT_CSD_PARTITION_SETTING_COMPLETED, 1);
+		if (ret) {
+			printf("Failure to write to EXT_CSD_PARTITION_SETTING_COMPLETED\n");
+error:
+			free(ext_csd);
+		} else {
+			printf("Now power cycle the device to let it reconfigure itself.\n");
+		}
+	}
+
+	return ret;
+}
+
+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, subcmdlen;
+	int (*func)(int argc, char *argv[]) = NULL;
+
+	if (argc < 2) {
+		printf("mmc: required subcommand missing\n");
+		return 1;
+	}
+
+	subcmdlen = strlen(argv[1]);
+	for (i = 0; i < ARRAY_SIZE(mmc_subcmds); ++i) {
+		if (strncmp(mmc_subcmds[i].cmd, argv[1], subcmdlen) == 0) {
+			if (subcmdlen == strlen(mmc_subcmds[i].cmd)) {
+				/* exact match */
+				func = mmc_subcmds[i].func;
+				break;
+			} else if (func) {
+				printf("mmc: ambiguously abbreviated subcommand");
+				return 1;
+			} else {
+				func = mmc_subcmds[i].func;
+			}
+		}
+	}
+
+	if (func) {
+		int ret = func(argc - 1, argv + 1);
+		return ret < 0;
+	} else {
+		printf("mmc: subcommand \"%s\" not found\n", argv[1]);
+		return 1;
+	}
+}
+
+BAREBOX_CMD_START(mmc)
+	.cmd = do_mmc,
+	BAREBOX_CMD_OPTS("enh_area setmax <-y|-n|-c> /dev/mmcX")
+BAREBOX_CMD_END
diff --git a/include/mci.h b/include/mci.h
index 827eecd39f8d..779da9c156db 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -304,6 +304,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.11.0


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

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

[Sascha Hauer]

On Fri, Jan 19, 2018 at 02:28:25PM +0100, Uwe Kleine-König wrote:
> 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 setmax <-y|-n|-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 but just creates an
> enhanced area with maximal size.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  commands/Kconfig  |   5 ++
>  commands/Makefile |   1 +
>  commands/mmc.c    | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/mci.h     |   7 +++
>  4 files changed, 184 insertions(+)
>  create mode 100644 commands/mmc.c
> 
> diff --git a/commands/Kconfig b/commands/Kconfig
> index ae2dc4b0947b..1b29365a06f2 100644
> --- a/commands/Kconfig
> +++ b/commands/Kconfig
> @@ -238,6 +238,11 @@ 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

Could you mention that this command is designed to control eMMC cards
like the mmc_extcsd command, but on a higher abstraction level, similar
to the userspace mmc utility. Just something like that, to give the user
a hint what the difference between both commands is?

> +
>  config CMD_MMC_EXTCSD
>  	tristate
>  	prompt "read/write eMMC ext. CSD register"
> diff --git a/commands/Makefile b/commands/Makefile
> index 37486dceb181..bcd0665cfe88 100644
> --- a/commands/Makefile
> +++ b/commands/Makefile
> @@ -120,6 +120,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

Experience shows that we want to use such code independent of commands
some day. Could you put the flesh of the subcommands somewhere else, maybe
to drivers/mci/?

> new file mode 100644
> index 000000000000..2f5c7ad69a37
> --- /dev/null
> +++ b/commands/mmc.c
> @@ -0,0 +1,171 @@
> +#include <command.h>
> +#include <mci.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +/* enh_area setmax <-y|-n|-c> /dev/mmcX */
> +static int do_mmc_enh_area(int argc, char *argv[])
> +{
> +	char *devname;
> +	struct mci *mci;
> +	u8 *ext_csd;
> +	int set_completed = 0;
> +	int ret;
> +
> +	if (argc != 4 || strcmp(argv[1], "setmax") ||
> +	    argv[2][0] != '-' ||
> +	    (argv[2][1] != 'y' && argv[2][1] != 'n' && argv[2][1] != 'c')) {
> +		printf("Usage: mmc enh_area setmax <-y|-n|-c> /dev/mmcX\n");
> +		return 1;
> +	}

I assume 'y' means 'yes', 'n' means 'no', but what does 'c' mean? It
doesn't seem to be implemented and none of these options is documented.

> +
> +	if (argv[2][1] == 'y')
> +		set_completed = 1;
> +
> +	devname = argv[3];
> +	if (!strncmp(devname, "/dev/", 5))
> +		devname += 5;
> +
> +	mci = mci_get_device_by_name(devname);
> +	if (!mci) {
> +		printf("Failure to open %s as mci device\n", devname);
> +		return -ENOENT;
> +	}

This part is probably needed by all other future subcommands aswell.
Should it be an extra function?

> +	/* get extcsd */
> +	ext_csd = xmalloc(512);
> +
> +	ret = mci_send_ext_csd(mci, ext_csd);
> +	if (ret) {
> +		printf("Failure to read EXT_CSD register\n");
> +		goto error;
> +	}

Also for reusability for other subcommands at would be good to allocate and fill
in ext_csd somewhere else.

> +
> +	if (!(ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & EXT_CSD_ENH_ATTRIBUTE_EN_MASK)) {
> +		printf("Device doesn't support enhanced area\n");
> +		ret = -EIO;
> +		goto error;
> +	}
> +
> +	if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED]) {
> +		printf("Partitioning already finalized\n");
> +		ret = -EIO;
> +		goto error;
> +	}
> +
> +	ret = mci_switch(mci, EXT_CSD_ERASE_GROUP_DEF, 1);
> +	if (ret) {
> +		printf("Failure to write to EXT_CSD_ERASE_GROUP_DEF\n");
> +		goto error;

This command is *very* verbose in the error path. Would it be enough to
just write the register number of the failed access instead of the name?

> +	}
> +
> +	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR, 0);
> +	if (ret) {
> +		printf("Failure to write to EXT_CSD_ENH_START_ADDR[0]\n");
> +		goto error;
> +	}
> +
> +	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR + 1, 0);
> +	if (ret) {
> +		printf("Failure to write to EXT_CSD_ENH_START_ADDR[1]\n");
> +		goto error;
> +	}
> +
> +	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR + 2, 0);
> +	if (ret) {
> +		printf("Failure to write to EXT_CSD_ENH_START_ADDR[2]\n");
> +		goto error;
> +	}
> +
> +	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR + 3, 0);
> +	if (ret) {
> +		printf("Failure to write to EXT_CSD_ENH_START_ADDR[3]\n");
> +		goto error;
> +	}
> +
> +	ret = mci_switch(mci, EXT_CSD_ENH_SIZE_MULT, ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT]);
> +	if (ret) {
> +		printf("Failure to write to EXT_CSD_ENH_SIZE_MULT[0]\n");
> +		goto error;
> +	}
> +
> +	ret = mci_switch(mci, EXT_CSD_ENH_SIZE_MULT + 1, ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 1]);
> +	if (ret) {
> +		printf("Failure to write to EXT_CSD_ENH_SIZE_MULT[1]\n");
> +		goto error;
> +	}
> +
> +	ret = mci_switch(mci, EXT_CSD_ENH_SIZE_MULT + 2, ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 2]);
> +	if (ret) {
> +		printf("Failure to write to EXT_CSD_ENH_SIZE_MULT[2]\n");
> +		goto error;
> +	}
> +
> +	ret = mci_switch(mci, EXT_CSD_PARTITIONS_ATTRIBUTE, ext_csd[EXT_CSD_PARTITIONS_ATTRIBUTE] | EXT_CSD_ENH_USR_MASK);
> +	if (ret) {
> +		printf("Failure to write to EXT_CSD_PARTITIONS_ATTRIBUTE\n");
> +		goto error;
> +	}
> +
> +	if (set_completed) {
> +		ret = mci_switch(mci, EXT_CSD_PARTITION_SETTING_COMPLETED, 1);
> +		if (ret) {
> +			printf("Failure to write to EXT_CSD_PARTITION_SETTING_COMPLETED\n");
> +error:
> +			free(ext_csd);
> +		} else {
> +			printf("Now power cycle the device to let it reconfigure itself.\n");
> +		}
> +	}

You lose memory when the 'y' option is not given.

> +
> +	return ret;
> +}
> +
> +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, subcmdlen;
> +	int (*func)(int argc, char *argv[]) = NULL;
> +
> +	if (argc < 2) {
> +		printf("mmc: required subcommand missing\n");
> +		return 1;
> +	}
> +
> +	subcmdlen = strlen(argv[1]);
> +	for (i = 0; i < ARRAY_SIZE(mmc_subcmds); ++i) {
> +		if (strncmp(mmc_subcmds[i].cmd, argv[1], subcmdlen) == 0) {
> +			if (subcmdlen == strlen(mmc_subcmds[i].cmd)) {
> +				/* exact match */
> +				func = mmc_subcmds[i].func;
> +				break;
> +			} else if (func) {
> +				printf("mmc: ambiguously abbreviated subcommand");
> +				return 1;
> +			} else {
> +				func = mmc_subcmds[i].func;
> +			}
> +		}
> +	}

Do we really need abbreviated subcommands here? abbreviated command
shouldn't be used in script and I hope this command does not become a
command that people use that frequently that they miss abbreviated
command support. I would just drop it.

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 v2 2/2] mci: implement command to switch a mmc device to enhanced mode

[Uwe Kleine-König]

Hello,

On Mon, Jan 22, 2018 at 09:12:34AM +0100, Sascha Hauer wrote:
> On Fri, Jan 19, 2018 at 02:28:25PM +0100, Uwe Kleine-König wrote:
> > new file mode 100644
> > index 000000000000..2f5c7ad69a37
> > --- /dev/null
> > +++ b/commands/mmc.c
> > @@ -0,0 +1,171 @@
> > +#include <command.h>
> > +#include <mci.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +
> > +/* enh_area setmax <-y|-n|-c> /dev/mmcX */
> > +static int do_mmc_enh_area(int argc, char *argv[])
> > +{
> > +	char *devname;
> > +	struct mci *mci;
> > +	u8 *ext_csd;
> > +	int set_completed = 0;
> > +	int ret;
> > +
> > +	if (argc != 4 || strcmp(argv[1], "setmax") ||
> > +	    argv[2][0] != '-' ||
> > +	    (argv[2][1] != 'y' && argv[2][1] != 'n' && argv[2][1] != 'c')) {
> > +		printf("Usage: mmc enh_area setmax <-y|-n|-c> /dev/mmcX\n");
> > +		return 1;
> > +	}
> 
> I assume 'y' means 'yes', 'n' means 'no', but what does 'c' mean? It
> doesn't seem to be implemented and none of these options is documented.

I'll rework to

	enh_area setmax [-c] /dev/mmcX

and add some documentation.

> > +	if (argv[2][1] == 'y')
> > +		set_completed = 1;
> > +
> > +	devname = argv[3];
> > +	if (!strncmp(devname, "/dev/", 5))
> > +		devname += 5;
> > +
> > +	mci = mci_get_device_by_name(devname);
> > +	if (!mci) {
> > +		printf("Failure to open %s as mci device\n", devname);
> > +		return -ENOENT;
> > +	}
> 
> This part is probably needed by all other future subcommands aswell.
> Should it be an extra function?

Something like:

struct mci *mci_get_device_by_devname(const char *devname)
{
	if (!strncmp(devname, "/dev/", 5))
		devname += 5;

	return mci_get_device_by_name(devname);
}

?

> > +	if (!(ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & EXT_CSD_ENH_ATTRIBUTE_EN_MASK)) {
> > +		printf("Device doesn't support enhanced area\n");
> > +		ret = -EIO;
> > +		goto error;
> > +	}
> > +
> > +	if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED]) {
> > +		printf("Partitioning already finalized\n");
> > +		ret = -EIO;
> > +		goto error;
> > +	}
> > +
> > +	ret = mci_switch(mci, EXT_CSD_ERASE_GROUP_DEF, 1);
> > +	if (ret) {
> > +		printf("Failure to write to EXT_CSD_ERASE_GROUP_DEF\n");
> > +		goto error;
> 
> This command is *very* verbose in the error path. Would it be enough to
> just write the register number of the failed access instead of the name?

It prints a single line if an error happens. I'd not say this is too
verbose. Do you care about the output or the binary size?

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 v2 2/2] mci: implement command to switch a mmc device to enhanced mode

[Sascha Hauer]

Hi Uwe,

On Tue, Oct 02, 2018 at 10:33:10AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Jan 22, 2018 at 09:12:34AM +0100, Sascha Hauer wrote:
> > On Fri, Jan 19, 2018 at 02:28:25PM +0100, Uwe Kleine-König wrote:
> > > new file mode 100644
> > > index 000000000000..2f5c7ad69a37
> > > --- /dev/null
> > > +++ b/commands/mmc.c
> > > @@ -0,0 +1,171 @@
> > > +#include <command.h>
> > > +#include <mci.h>
> > > +#include <stdio.h>
> > > +#include <string.h>
> > > +
> > > +/* enh_area setmax <-y|-n|-c> /dev/mmcX */
> > > +static int do_mmc_enh_area(int argc, char *argv[])
> > > +{
> > > +	char *devname;
> > > +	struct mci *mci;
> > > +	u8 *ext_csd;
> > > +	int set_completed = 0;
> > > +	int ret;
> > > +
> > > +	if (argc != 4 || strcmp(argv[1], "setmax") ||
> > > +	    argv[2][0] != '-' ||
> > > +	    (argv[2][1] != 'y' && argv[2][1] != 'n' && argv[2][1] != 'c')) {
> > > +		printf("Usage: mmc enh_area setmax <-y|-n|-c> /dev/mmcX\n");
> > > +		return 1;
> > > +	}
> > 
> > I assume 'y' means 'yes', 'n' means 'no', but what does 'c' mean? It
> > doesn't seem to be implemented and none of these options is documented.
> 
> I'll rework to
> 
> 	enh_area setmax [-c] /dev/mmcX
> 
> and add some documentation.
> 
> > > +	if (argv[2][1] == 'y')
> > > +		set_completed = 1;
> > > +
> > > +	devname = argv[3];
> > > +	if (!strncmp(devname, "/dev/", 5))
> > > +		devname += 5;
> > > +
> > > +	mci = mci_get_device_by_name(devname);
> > > +	if (!mci) {
> > > +		printf("Failure to open %s as mci device\n", devname);
> > > +		return -ENOENT;
> > > +	}
> > 
> > This part is probably needed by all other future subcommands aswell.
> > Should it be an extra function?
> 
> Something like:
> 
> struct mci *mci_get_device_by_devname(const char *devname)
> {
> 	if (!strncmp(devname, "/dev/", 5))
> 		devname += 5;

devpath_to_name() instead.

> 
> 	return mci_get_device_by_name(devname);
> }
> 
> ?
> 
> > > +	if (!(ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & EXT_CSD_ENH_ATTRIBUTE_EN_MASK)) {
> > > +		printf("Device doesn't support enhanced area\n");
> > > +		ret = -EIO;
> > > +		goto error;
> > > +	}
> > > +
> > > +	if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED]) {
> > > +		printf("Partitioning already finalized\n");
> > > +		ret = -EIO;
> > > +		goto error;
> > > +	}
> > > +
> > > +	ret = mci_switch(mci, EXT_CSD_ERASE_GROUP_DEF, 1);
> > > +	if (ret) {
> > > +		printf("Failure to write to EXT_CSD_ERASE_GROUP_DEF\n");
> > > +		goto error;
> > 
> > This command is *very* verbose in the error path. Would it be enough to
> > just write the register number of the failed access instead of the name?
> 
> It prints a single line if an error happens. I'd not say this is too
> verbose. Do you care about the output or the binary size?

The latter.

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

[PATCH v3 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.19.0


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

[PATCH v3 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 setmax [-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 but just creates an
enhanced area with maximal size.

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

diff --git a/commands/Kconfig b/commands/Kconfig
index 675bd1ca76a0..221d65a358e0 100644
--- a/commands/Kconfig
+++ b/commands/Kconfig
@@ -238,6 +238,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 eb4796389e6b..27a62240b039 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..b3af2d9c769c
--- /dev/null
+++ b/commands/mmc.c
@@ -0,0 +1,195 @@
+#include <command.h>
+#include <mci.h>
+#include <stdio.h>
+#include <string.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");
+		return ERR_PTR(-EIO);
+	}
+
+	return ext_csd;
+}
+
+/* enh_area setmax [-c] /dev/mmcX */
+static int do_mmc_enh_area(int argc, char *argv[])
+{
+	char *devpath;
+	struct mci *mci;
+	u8 *ext_csd;
+	int set_completed = 0;
+	int ret;
+
+	if (argc < 2 || strcmp(argv[1], "setmax") != 0)
+		goto usage;
+
+	if (argc == 3) {
+		/* enh_area setmax /dev/mmcX */
+		devpath = argv[2];
+		set_completed = false;
+	} else if (argc == 4 || strcmp(argv[2], "-c") == 0) {
+		/* enh_area setmax -c /dev/mmcX */
+		devpath = argv[3];
+		set_completed = true;
+	} else {
+usage:
+		printf("Usage: mmc enh_area setmax [-c] /dev/mmcX\n");
+		return COMMAND_ERROR_USAGE;
+	}
+
+	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 setmax [-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.19.0


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

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

[Sascha Hauer]

On Mon, Oct 08, 2018 at 10:29:21AM +0200, Uwe Kleine-König wrote:
> 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 setmax [-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 but just creates an
> enhanced area with maximal size.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> +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");
> +		return ERR_PTR(-EIO);

Please free malloced memory.

> +	}
> +
> +	return ext_csd;
> +}
> +
> +/* enh_area setmax [-c] /dev/mmcX */
> +static int do_mmc_enh_area(int argc, char *argv[])
> +{
> +	char *devpath;
> +	struct mci *mci;
> +	u8 *ext_csd;
> +	int set_completed = 0;
> +	int ret;
> +
> +	if (argc < 2 || strcmp(argv[1], "setmax") != 0)
> +		goto usage;
> +
> +	if (argc == 3) {
> +		/* enh_area setmax /dev/mmcX */
> +		devpath = argv[2];
> +		set_completed = false;
> +	} else if (argc == 4 || strcmp(argv[2], "-c") == 0) {
> +		/* enh_area setmax -c /dev/mmcX */
> +		devpath = argv[3];
> +		set_completed = true;
> +	} else {

This option parsing already is quite hard to read and will be harder if
it is extended in the future. Can't we use getopt() here?

Usage:
enh_area [options] <mmcdev>
 -m	Use maximum available space
 -c	complete partitioning

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

[PATCH v2] 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 setmax [-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 but just creates an
enhanced area with maximal size.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

this is v2 of this patch. I refactored some stuff (but didn't move it
out to drivers/mci as suggested, I'd delay that until need arises),
changed the options to be easier and added some documentation.

Subcommand parsing is now also simpler and doesn't pick up abbreviated
commands.

Some review comments (e.g. "verbose error reporting") isn't addressed
yet as I'm not sure I understand it correctly (see the other mail in
this thread).

I'm sending out already now as the last round was sent out quite some
time to swap this topic in again (in case it was already removed from
your mailbox).

Best regards
Uwe

 commands/Kconfig  |  11 +++
 commands/Makefile |   1 +
 commands/mmc.c    | 209 ++++++++++++++++++++++++++++++++++++++++++++++
 include/mci.h     |   7 ++
 4 files changed, 228 insertions(+)
 create mode 100644 commands/mmc.c

diff --git a/commands/Kconfig b/commands/Kconfig
index 675bd1ca76a0..221d65a358e0 100644
--- a/commands/Kconfig
+++ b/commands/Kconfig
@@ -238,6 +238,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 eb4796389e6b..27a62240b039 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..ea1c6d6e4f53
--- /dev/null
+++ b/commands/mmc.c
@@ -0,0 +1,209 @@
+#include <command.h>
+#include <mci.h>
+#include <stdio.h>
+#include <string.h>
+
+static int mmc_enh_area_setmax(struct mci *mci, u8 *ext_csd)
+{
+	int ret;
+
+	ret = mci_switch(mci, EXT_CSD_ERASE_GROUP_DEF, 1);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ERASE_GROUP_DEF\n");
+		return ret;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR, 0);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_START_ADDR[0]\n");
+		return ret;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR + 1, 0);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_START_ADDR[1]\n");
+		return ret;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR + 2, 0);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_START_ADDR[2]\n");
+		return ret;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_START_ADDR + 3, 0);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_START_ADDR[3]\n");
+		return ret;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_SIZE_MULT, ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT]);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_SIZE_MULT[0]\n");
+		return ret;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_SIZE_MULT + 1, ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 1]);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_SIZE_MULT[1]\n");
+		return ret;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_ENH_SIZE_MULT + 2, ext_csd[EXT_CSD_MAX_ENH_SIZE_MULT + 2]);
+	if (ret) {
+		printf("Failure to write to EXT_CSD_ENH_SIZE_MULT[2]\n");
+		return ret;
+	}
+
+	ret = mci_switch(mci, EXT_CSD_PARTITIONS_ATTRIBUTE, ext_csd[EXT_CSD_PARTITIONS_ATTRIBUTE] | EXT_CSD_ENH_USR_MASK);
+	if (ret)
+		printf("Failure to write to EXT_CSD_PARTITIONS_ATTRIBUTE\n");
+
+	return ret;
+}
+
+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");
+		return ERR_PTR(-EIO);
+	}
+
+	return ext_csd;
+}
+
+/* enh_area setmax [-c] /dev/mmcX */
+static int do_mmc_enh_area(int argc, char *argv[])
+{
+	char *devname;
+	struct mci *mci;
+	u8 *ext_csd;
+	int set_completed = 0;
+	int ret;
+
+	if (argc < 2 || strcmp(argv[1], "setmax") != 0)
+		goto usage;
+
+	if (argc == 3) {
+		/* enh_area setmax /dev/mmcX */
+		devname = argv[2];
+		set_completed = false;
+	} else if (argc == 4 || strcmp(argv[2], "-c") == 0) {
+		/* enh_area setmax -c /dev/mmcX */
+		devname = argv[3];
+		set_completed = true;
+	} else {
+usage:
+		printf("Usage: mmc enh_area setmax [-c] /dev/mmcX\n");
+		return COMMAND_ERROR_USAGE;
+	}
+
+	if (!strncmp(devname, "/dev/", 5))
+		devname += 5;
+
+	mci = mci_get_device_by_name(devname);
+	if (!mci) {
+		printf("Failure to open %s as mci device\n", devname);
+		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 setmax [-c] /dev/mmcX")
+	BAREBOX_CMD_HELP(cmd_mmc_help)
+BAREBOX_CMD_END
diff --git a/include/mci.h b/include/mci.h
index 072008ef9da7..f70184f3b4ab 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -304,6 +304,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.19.0


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

Re: [PATCH v2 1/2] mci: drop unused parameter from mci_switch()

[Sascha Hauer]

On Fri, Jan 19, 2018 at 02:28:24PM +0100, Uwe Kleine-König wrote:
> The SWITCH command has two purposes:
>  a) switch the command set
>  b) Write to the EXT_CSD register
> 
> If the access field (bits [25:24]) in the argument are b00, we're in
> case a), otherwise in b). As mci_switch() always passes
> MMC_SWITCH_MODE_WRITE_BYTE (0b3) in the access field, only case b) is
> relevant here. According to the eMMC specification[1] the command set
> field is ignored in case b) and so the respective parameter (that is
> unused already now) can be dropped.
> 
> [1] Embedded Multi-Media Card (e•MMC) Electrical Standard (5.1),
>     February 2015; paragraph 6.6.1
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

Applied this one.

Sascha

>  commands/mmc_extcsd.c  |  2 +-
>  drivers/mci/mci-core.c | 16 ++++++----------
>  include/mci.h          |  3 +--
>  3 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/commands/mmc_extcsd.c b/commands/mmc_extcsd.c
> index 7a6d39075da0..acd23a466bcb 100644
> --- a/commands/mmc_extcsd.c
> +++ b/commands/mmc_extcsd.c
> @@ -2357,7 +2357,7 @@ static void write_field(struct mci *mci, u8 *reg, u16 index, u8 value,
>  		break;
>  	}
>  
> -	mci_switch(mci, 0, index, value);
> +	mci_switch(mci, index, value);
>  
>  out:
>  	return;
> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> index 07911d43d703..3da96f42aaf9 100644
> --- a/drivers/mci/mci-core.c
> +++ b/drivers/mci/mci-core.c
> @@ -396,8 +396,7 @@ int mci_send_ext_csd(struct mci *mci, char *ext_csd)
>   * @param value FIXME
>   * @return Transaction status (0 on success)
>   */
> -int mci_switch(struct mci *mci, unsigned set, unsigned index,
> -			unsigned value)
> +int mci_switch(struct mci *mci, unsigned index, unsigned value)
>  {
>  	struct mci_cmd cmd;
>  
> @@ -471,7 +470,7 @@ static int mmc_change_freq(struct mci *mci)
>  
>  	cardtype = mci->ext_csd[EXT_CSD_DEVICE_TYPE] & EXT_CSD_CARD_TYPE_MASK;
>  
> -	err = mci_switch(mci, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);
> +	err = mci_switch(mci, EXT_CSD_HS_TIMING, 1);
>  
>  	if (err) {
>  		dev_dbg(&mci->dev, "MMC frequency changing failed: %d\n", err);
> @@ -1044,9 +1043,7 @@ static int mci_startup_mmc(struct mci *mci)
>  		 * 4bit transfer mode. On success set the corresponding
>  		 * bus width on the host.
>  		 */
> -		err = mci_switch(mci, EXT_CSD_CMD_SET_NORMAL,
> -				 EXT_CSD_BUS_WIDTH,
> -				 ext_csd_bits[idx]);
> +		err = mci_switch(mci, EXT_CSD_BUS_WIDTH, ext_csd_bits[idx]);
>  		if (err) {
>  			if (idx == 0)
>  				dev_warn(&mci->dev, "Changing MMC bus width failed: %d\n", err);
> @@ -1253,8 +1250,7 @@ static int mci_blk_part_switch(struct mci_part *part)
>  		part_config &= ~EXT_CSD_PART_CONFIG_ACC_MASK;
>  		part_config |= part->part_cfg;
>  
> -		ret = mci_switch(mci, EXT_CSD_CMD_SET_NORMAL,
> -				EXT_CSD_PARTITION_CONFIG, part_config);
> +		ret = mci_switch(mci, EXT_CSD_PARTITION_CONFIG, part_config);
>  		if (ret)
>  			return ret;
>  
> @@ -1568,8 +1564,8 @@ static int mci_set_boot(struct param_d *param, void *priv)
>  	mci->ext_csd_part_config &= ~(7 << 3);
>  	mci->ext_csd_part_config |= mci->bootpart << 3;
>  
> -	return mci_switch(mci, EXT_CSD_CMD_SET_NORMAL,
> -			EXT_CSD_PARTITION_CONFIG, mci->ext_csd_part_config);
> +	return mci_switch(mci,
> +			  EXT_CSD_PARTITION_CONFIG, mci->ext_csd_part_config);
>  }
>  
>  static const char *mci_boot_names[] = {
> diff --git a/include/mci.h b/include/mci.h
> index d3115e8cc68f..827eecd39f8d 100644
> --- a/include/mci.h
> +++ b/include/mci.h
> @@ -477,8 +477,7 @@ void mci_of_parse(struct mci_host *host);
>  void mci_of_parse_node(struct mci_host *host, struct device_node *np);
>  int mci_detect_card(struct mci_host *);
>  int mci_send_ext_csd(struct mci *mci, char *ext_csd);
> -int mci_switch(struct mci *mci, unsigned set, unsigned index,
> -			unsigned value);
> +int mci_switch(struct mci *mci, unsigned index, unsigned value);
>  
>  static inline int mmc_host_is_spi(struct mci_host *host)
>  {
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

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