RE: [PATCH v2] cmd: mmc: Add mmc reg read command for reading card registers

["Jaehoon Chung" <jh80.chung@samsung.com>]

Hi,

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Jaehoon Chung
> Sent: Tuesday, October 31, 2023 3:08 PM
> To: Marek Vasut <marex@denx.de>; u-boot@lists.denx.de
> Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>; Heinrich Schuchardt <xypron.glpk@gmx.de>;
> Ilias Apalodimas <ilias.apalodimas@linaro.org>; Ramon Fried <rfried.dev@gmail.com>; Roger Knecht
> <rknecht@pm.me>; Sean Edmond <seanedmond@microsoft.com>; Simon Glass <sjg@chromium.org>; Tobias
> Waldekranz <tobias@waldekranz.com>
> Subject: Re: [PATCH v2] cmd: mmc: Add mmc reg read command for reading card registers
> 
> Hi Marek,
> 
> On 10/10/23 21:47, Marek Vasut wrote:
> > Add extension to the 'mmc' command to read out the card registers.
> > Currently, only the eMMC OCR/CID/CSD/EXTCSD/RCA/DSR register are
> > supported. A register value can either be displayed or read into
> > an environment variable.
> >
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > ---
> > Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Cc: Jaehoon Chung <jh80.chung@samsung.com>
> > Cc: Ramon Fried <rfried.dev@gmail.com>
> > Cc: Roger Knecht <rknecht@pm.me>
> > Cc: Sean Edmond <seanedmond@microsoft.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Tobias Waldekranz <tobias@waldekranz.com>
> 
> Looks good to me. I have tested with your patch on my target.
> 
> mmc reg read cid all
> CID[0]: 0x15010042
> => mmc reg read extcsd all
> EXT_CSD:
> 000:  00 00 00 00 00 00 00 00 00 00
> 010:  00 00 00 00 00 00 39 00 00 00
> ..[snip]..
> 
> Tested-by: Jaehoon Chung <jh80.chung@samsung.com>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> 
> Best Regards,
> Jaehoon Chung
> 
> > ---
> > V2: - Update documentation
> > ---
> >  cmd/Kconfig           |  8 ++++
> >  cmd/mmc.c             | 96 +++++++++++++++++++++++++++++++++++++++++++
> >  doc/usage/cmd/mmc.rst | 26 ++++++++++++
> >  3 files changed, 130 insertions(+)
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 6470b138d2f..dcd99757a1e 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1307,6 +1307,14 @@ config CMD_BKOPS_ENABLE
> >  	  on a eMMC device. The feature is optionally available on eMMC devices
> >  	  conforming to standard >= 4.41.
> >
> > +config CMD_MMC_REG
> > +	bool "Enable support for reading card registers in the mmc command"
> > +	depends on CMD_MMC
> > +	default n
> > +	help
> > +	  Enable the commands for reading card registers. This is useful
> > +	  mostly for debugging or extracting details from the card.
> > +
> >  config CMD_MMC_RPMB
> >  	bool "Enable support for RPMB in the mmc command"
> >  	depends on SUPPORT_EMMC_RPMB
> > diff --git a/cmd/mmc.c b/cmd/mmc.c
> > index c6bd81cebbc..c29f44b7a18 100644
> > --- a/cmd/mmc.c
> > +++ b/cmd/mmc.c
> > @@ -1110,6 +1110,93 @@ static int do_mmc_boot_wp(struct cmd_tbl *cmdtp, int flag,
> >  	return CMD_RET_SUCCESS;
> >  }
> >
> > +#if CONFIG_IS_ENABLED(CMD_MMC_REG)
> > +static int do_mmc_reg(struct cmd_tbl *cmdtp, int flag,
> > +		      int argc, char *const argv[])
> > +{
> > +	ALLOC_CACHE_ALIGN_BUFFER(u8, ext_csd, MMC_MAX_BLOCK_LEN);
> > +	struct mmc *mmc;
> > +	int i, ret;
> > +	u32 off;
> > +
> > +	if (argc < 3 || argc > 5)
> > +		return CMD_RET_USAGE;
> > +
> > +	mmc = find_mmc_device(curr_device);
> > +	if (!mmc) {
> > +		printf("no mmc device at slot %x\n", curr_device);
> > +		return CMD_RET_FAILURE;
> > +	}
> > +
> > +	if (IS_SD(mmc)) {
> > +		printf("SD registers are not supported\n");
> > +		return CMD_RET_FAILURE;
> > +	}
> > +
> > +	off = simple_strtoul(argv[3], NULL, 10);
> > +	if (!strcmp(argv[2], "cid")) {
> > +		if (off > 3)
> > +			return CMD_RET_USAGE;
> > +		printf("CID[%i]: 0x%08x\n", off, mmc->cid[off]);
> > +		if (argv[4])
> > +			env_set_hex(argv[4], mmc->cid[off]);
> > +		return CMD_RET_SUCCESS;
> > +	}
> > +	if (!strcmp(argv[2], "csd")) {
> > +		if (off > 3)
> > +			return CMD_RET_USAGE;
> > +		printf("CSD[%i]: 0x%08x\n", off, mmc->csd[off]);
> > +		if (argv[4])
> > +			env_set_hex(argv[4], mmc->csd[off]);
> > +		return CMD_RET_SUCCESS;
> > +	}
> > +	if (!strcmp(argv[2], "dsr")) {
> > +		printf("DSR: 0x%08x\n", mmc->dsr);
> > +		if (argv[4])
> > +			env_set_hex(argv[4], mmc->dsr);
> > +		return CMD_RET_SUCCESS;
> > +	}
> > +	if (!strcmp(argv[2], "ocr")) {
> > +		printf("OCR: 0x%08x\n", mmc->ocr);
> > +		if (argv[4])
> > +			env_set_hex(argv[4], mmc->ocr);
> > +		return CMD_RET_SUCCESS;
> > +	}
> > +	if (!strcmp(argv[2], "rca")) {
> > +		printf("RCA: 0x%08x\n", mmc->rca);
> > +		if (argv[4])
> > +			env_set_hex(argv[4], mmc->rca);
> > +		return CMD_RET_SUCCESS;
> > +	}
> > +	if (!strcmp(argv[2], "extcsd") &&
> > +	    mmc->version >= MMC_VERSION_4_41) {
> > +		ret = mmc_send_ext_csd(mmc, ext_csd);
> > +		if (ret)
> > +			return ret;
> > +		if (!strcmp(argv[3], "all")) {
> > +			/* Dump the entire register */
> > +			printf("EXT_CSD:");
> > +			for (i = 0; i < MMC_MAX_BLOCK_LEN; i++) {
> > +				if (!(i % 10))
> > +					printf("\n%03i: ", i);
> > +				printf(" %02x", ext_csd[i]);
> > +			}
> > +			printf("\n");
> > +			return CMD_RET_SUCCESS;
> > +		}
> > +		off = simple_strtoul(argv[3], NULL, 10);
> > +		if (off > 512)
> > +			return CMD_RET_USAGE;
> > +		printf("EXT_CSD[%i]: 0x%02x\n", off, ext_csd[off]);
> > +		if (argv[4])
> > +			env_set_hex(argv[4], ext_csd[off]);
> > +		return CMD_RET_SUCCESS;
> > +	}
> > +
> > +	return CMD_RET_FAILURE;
> > +}
> > +#endif
> > +
> >  static struct cmd_tbl cmd_mmc[] = {
> >  	U_BOOT_CMD_MKENT(info, 1, 0, do_mmcinfo, "", ""),
> >  	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
> > @@ -1142,6 +1229,9 @@ static struct cmd_tbl cmd_mmc[] = {
> >  	U_BOOT_CMD_MKENT(bkops-enable, 2, 0, do_mmc_bkops_enable, "", ""),
> >  	U_BOOT_CMD_MKENT(bkops, 4, 0, do_mmc_bkops, "", ""),
> >  #endif
> > +#if CONFIG_IS_ENABLED(CMD_MMC_REG)
> > +	U_BOOT_CMD_MKENT(reg, 5, 0, do_mmc_reg, "", ""),
> > +#endif
> >  };
> >
> >  static int do_mmcops(struct cmd_tbl *cmdtp, int flag, int argc,
> > @@ -1229,6 +1319,12 @@ U_BOOT_CMD(
> >  	"   WARNING: This is a write-once setting.\n"
> >  	"mmc bkops <dev> [auto|manual] [enable|disable]\n"
> >  	" - configure background operations handshake on device\n"
> > +#endif
> > +#if CONFIG_IS_ENABLED(CMD_MMC_REG)
> > +	"mmc reg read <reg> <offset> [env] - read card register <reg> offset <offset>\n"
> > +	"                                    (optionally into [env] variable)\n"
> > +	" - reg: cid/csd/dsr/ocr/rca/extcsd\n"
> > +	" - offset: for cid/csd [0..3], for extcsd [0..511,all]\n"

Is there any reason to add "all" option for cid/csd?

Best Regards,
Jaehoon Chung

> >  #endif
> >  	);
> >
> > diff --git a/doc/usage/cmd/mmc.rst b/doc/usage/cmd/mmc.rst
> > index 71a0303109c..c0924ba5769 100644
> > --- a/doc/usage/cmd/mmc.rst
> > +++ b/doc/usage/cmd/mmc.rst
> > @@ -21,6 +21,7 @@ Synopsis
> >      mmc bootpart-resize <dev> <dev part size MB> <RPMB part size MB>
> >      mmc partconf <dev> [[varname] | [<boot_ack> <boot_partition> <partition_access>]]
> >      mmc rst-function <dev> <value>
> > +    mmc reg read <reg> <offset> [env]
> >
> >  Description
> >  -----------
> > @@ -183,6 +184,31 @@ The 'mmc rst-function' command changes the RST_n_FUNCTION field.
> >          0x3
> >              Reserved
> >
> > +The 'mmc reg read <reg> <offset> [env]' reads eMMC card register and
> > +either print it to standard output, or store the value in environment
> > +variable.
> > +
> > +<reg> with
> > +optional offset <offset> into the register array, and print it to
> > +standard output or store it into environment variable [env].
> > +
> > +    reg
> > +        cid
> > +            The Device IDentification (CID) register. Uses offset.
> > +        csd
> > +            The Device-Specific Data (CSD) register. Uses offset.
> > +        dsr
> > +            The driver stage register (DSR).
> > +        ocr
> > +            The operation conditions register (OCR).
> > +        rca
> > +            The relative Device address (RCA) register.
> > +        extcsd
> > +            The Extended CSD register. Uses offset.
> > +    offset
> > +        For 'cid'/'csd' 128 bit registers '[0..3]' in 32-bit increments. For 'extcsd' 512 bit
> register '[0..512,all]' in 8-bit increments, or 'all' to read the entire register.
> > +    env
> > +        Optional environment variable into which 32-bit value read from register should be stored.
> >
> >  Examples
> >  --------