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

[Sascha Hauer <s.hauer@pengutronix.de>]

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