HI Aswath,
> -----Original Message-----
> From: Aswath Govindraju <a-govindr...@ti.com>
> Sent: Friday, July 30, 2021 10:41 AM
> To: Ashok Reddy Soma <ashok...@xilinx.com>
> Cc: Lokesh Vutla <lokeshvu...@ti.com>; Vignesh Raghavendra
> <vigne...@ti.com>; Kishon Vijay Abraham I <kis...@ti.com>; Peng Fan
> <peng....@nxp.com>; Jaehoon Chung <jh80.ch...@samsung.com>; Simon
> Glass <s...@chromium.org>; Heinrich Schuchardt <xypron.g...@gmx.de>; Bin
> Meng <bmeng...@gmail.com>; Patrick Delaunay
> <patrick.delau...@foss.st.com>; Pragnesh Patel <pragnesh.pa...@sifive.com>;
> Frédéric Danis <frederic.da...@collabora.com>; Reuben Dowle
> <reubendow...@gmail.com>; Yangbo Lu <yangbo...@nxp.com>; Stefan Bosch
> <stefa...@posteo.net>; Nicolas Saenz Julienne <nsaenzjulie...@suse.de>; Pali
> Rohár <p...@kernel.org>; u-boot@lists.denx.de
> Subject: Re: [PATCH] mmc: Add support for enumerating MMC card in a given
> mode using mmc command
>
> Hi Ashok,
>
> On 30/07/21 10:17 am, Ashok Reddy Soma wrote:
> > Hi Aswath,
> >
> >> -----Original Message-----
> >> From: Aswath Govindraju <a-govindr...@ti.com>
> >> Sent: Thursday, July 29, 2021 8:25 PM
> >> Cc: Lokesh Vutla <lokeshvu...@ti.com>; Vignesh Raghavendra
> >> <vigne...@ti.com>; Kishon Vijay Abraham I <kis...@ti.com>; Aswath
> >> Govindraju <a-govindr...@ti.com>; Peng Fan <peng....@nxp.com>;
> >> Jaehoon Chung <jh80.ch...@samsung.com>; Simon Glass
> >> <s...@chromium.org>; Heinrich Schuchardt <xypron.g...@gmx.de>; Bin
> >> Meng <bmeng...@gmail.com>; Patrick Delaunay
> >> <patrick.delau...@foss.st.com>; Pragnesh Patel
> >> <pragnesh.pa...@sifive.com>; Frédéric Danis
> >> <frederic.da...@collabora.com>; Reuben Dowle
> >> <reubendow...@gmail.com>; Yangbo Lu <yangbo...@nxp.com>; Stefan
> Bosch
> >> <stefa...@posteo.net>; Ashok Reddy Soma <ashok...@xilinx.com>;
> >> Nicolas Saenz Julienne <nsaenzjulie...@suse.de>; Pali Rohár
> >> <p...@kernel.org>; u-boot@lists.denx.de
> >> Subject: [PATCH] mmc: Add support for enumerating MMC card in a given
> >> mode using mmc command
> >>
> >> Add support for enumerating MMC card in a given mode using mmc rescan
> >> and mmc dev commands. The speed mode is provided as the last argument
> >> in these commands and is indicated using the index from enum bus_mode
> >> in include/mmc.h. A speed mode can be set only if it has already been
> >> enabled in the device tree.
> >>
> >> Signed-off-by: Aswath Govindraju <a-govindr...@ti.com>
> >> ---
> >> cmd/Kconfig | 10 ++++++
> >> cmd/mmc.c | 71 ++++++++++++++++++++++++++++------------
> >> drivers/mmc/mmc-uclass.c | 3 ++
> >> drivers/mmc/mmc.c | 18 ++++++++++
> >> include/mmc.h | 4 +++
> >> 5 files changed, 85 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/cmd/Kconfig b/cmd/Kconfig index
> >> ffef3cc76ca4..3a857b3f6e2e 100644
> >> --- a/cmd/Kconfig
> >> +++ b/cmd/Kconfig
> >> @@ -2389,4 +2389,14 @@ config CMD_UBIFS
> >> help
> >> UBIFS is a file system for flash devices which works on top of UBI.
> >>
> >> +config MMC_SPEED_MODE_SET
> >> + bool "set speed mode using mmc command"
> >> + depends on CMD_MMC
> >> + default n
> >> + help
> >> + Enable setting speed mode using mmc rescan and mmc dev
> >> commands.
> >> + The speed mode is provided as the last argument in these commands
> >> + and is indicated using the index from enum bus_mode in
> >> + include/mmc.h. A speed mode can be set only if it has already
> >> + been enabled in the device tree.
> >> endmenu
> >> diff --git a/cmd/mmc.c b/cmd/mmc.c
> >> index b942576b58ac..073e87d69634 100644
> >> --- a/cmd/mmc.c
> >> +++ b/cmd/mmc.c
> >> @@ -120,7 +120,9 @@ static void print_mmcinfo(struct mmc *mmc)
> >> }
> >> }
> >> }
> >> -static struct mmc *init_mmc_device(int dev, bool force_init)
> >> +
> >> +static struct mmc *init_mmc_device(int dev, bool force_init,
> >> + enum bus_mode speed_mode)
> >> {
> >> struct mmc *mmc;
> >> mmc = find_mmc_device(dev);
> >> @@ -134,6 +136,11 @@ static struct mmc *init_mmc_device(int dev, bool
> >> force_init)
> >>
> >> if (force_init)
> >> mmc->has_init = 0;
> >> +
> >> +#if CONFIG_IS_ENABLED(MMC_SPEED_MODE_SET)
> >> + mmc->user_speed_mode = speed_mode;
> >> +#endif
> >> +
> > Can you please use if (IS_ENABLED(CONFIG_MMC_SPEED_MODE_SET)
> >
> >> if (mmc_init(mmc))
> >> return NULL;
> >>
> >> @@ -159,7 +166,7 @@ static int do_mmcinfo(struct cmd_tbl *cmdtp, int
> >> flag, int argc,
> >> }
> >> }
> >>
> >> - mmc = init_mmc_device(curr_device, false);
> >> + mmc = init_mmc_device(curr_device, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -293,7 +300,7 @@ static int do_mmcrpmb(struct cmd_tbl *cmdtp, int
> flag,
> >> if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp))
> >> return CMD_RET_SUCCESS;
> >>
> >> - mmc = init_mmc_device(curr_device, false);
> >> + mmc = init_mmc_device(curr_device, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -338,7 +345,7 @@ static int do_mmc_read(struct cmd_tbl *cmdtp, int
> >> flag,
> >> blk = simple_strtoul(argv[2], NULL, 16);
> >> cnt = simple_strtoul(argv[3], NULL, 16);
> >>
> >> - mmc = init_mmc_device(curr_device, false);
> >> + mmc = init_mmc_device(curr_device, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -387,7 +394,7 @@ static int do_mmc_sparse_write(struct cmd_tbl
> >> *cmdtp, int flag,
> >> return CMD_RET_FAILURE;
> >> }
> >>
> >> - mmc = init_mmc_device(curr_device, false);
> >> + mmc = init_mmc_device(curr_device, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -431,7 +438,7 @@ static int do_mmc_write(struct cmd_tbl *cmdtp,
> >> int flag,
> >> blk = simple_strtoul(argv[2], NULL, 16);
> >> cnt = simple_strtoul(argv[3], NULL, 16);
> >>
> >> - mmc = init_mmc_device(curr_device, false);
> >> + mmc = init_mmc_device(curr_device, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -460,7 +467,7 @@ static int do_mmc_erase(struct cmd_tbl *cmdtp,
> >> int flag,
> >> blk = simple_strtoul(argv[1], NULL, 16);
> >> cnt = simple_strtoul(argv[2], NULL, 16);
> >>
> >> - mmc = init_mmc_device(curr_device, false);
> >> + mmc = init_mmc_device(curr_device, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -482,8 +489,19 @@ static int do_mmc_rescan(struct cmd_tbl *cmdtp,
> >> int flag,
> >> int argc, char *const argv[])
> >> {
> >> struct mmc *mmc;
> >> + enum bus_mode speed_mode = MMC_MODES_END;
> >> +
> >> + if (argc == 1) {
> >> + mmc = init_mmc_device(curr_device, true,
> >> MMC_MODES_END);
> >> + } else if (argc == 2) {
> >> + /* parse the mode */
> >> + speed_mode = simple_strtoul(argv[1], NULL, 10);
> >> + /* Need to pass the the speed mode required too */
> >> + mmc = init_mmc_device(curr_device, true, speed_mode);
> >> + } else {
> >> + return CMD_RET_USAGE;
> >> + }
> >>
> >> - mmc = init_mmc_device(curr_device, true);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -496,7 +514,7 @@ static int do_mmc_part(struct cmd_tbl *cmdtp, int
> flag,
> >> struct blk_desc *mmc_dev;
> >> struct mmc *mmc;
> >>
> >> - mmc = init_mmc_device(curr_device, false);
> >> + mmc = init_mmc_device(curr_device, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -515,11 +533,14 @@ static int do_mmc_dev(struct cmd_tbl *cmdtp,
> >> int flag, {
> >> int dev, part = 0, ret;
> >> struct mmc *mmc;
> >> + enum bus_mode speed_mode = MMC_MODES_END;
> >>
> >> if (argc == 1) {
> >> dev = curr_device;
> >> + mmc = init_mmc_device(dev, true, MMC_MODES_END);
> >> } else if (argc == 2) {
> >> dev = simple_strtoul(argv[1], NULL, 10);
> >> + mmc = init_mmc_device(dev, true, MMC_MODES_END);
> >> } else if (argc == 3) {
> >> dev = (int)simple_strtoul(argv[1], NULL, 10);
> >> part = (int)simple_strtoul(argv[2], NULL, 10); @@ -528,11
> >> +549,16 @@ static int do_mmc_dev(struct cmd_tbl *cmdtp, int flag,
> >> PART_ACCESS_MASK);
> >> return CMD_RET_FAILURE;
> >> }
> >> + mmc = init_mmc_device(dev, true, MMC_MODES_END);
> >> + } else if (argc == 4) {
> >> + dev = (int)simple_strtoul(argv[1], NULL, 10);
> >> + part = (int)simple_strtoul(argv[2], NULL, 10);
> >> + speed_mode = simple_strtoul(argv[3], NULL, 10);
> >> + mmc = init_mmc_device(curr_device, true, speed_mode);
> >> } else {
> >> return CMD_RET_USAGE;
> >> }
> >>
> >> - mmc = init_mmc_device(dev, true);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -636,7 +662,7 @@ static int do_mmc_hwpartition(struct cmd_tbl
> >> *cmdtp, int flag,
> >> enum mmc_hwpart_conf_mode mode =
> >> MMC_HWPART_CONF_CHECK;
> >> int i, r, pidx;
> >>
> >> - mmc = init_mmc_device(curr_device, false);
> >> + mmc = init_mmc_device(curr_device, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -726,7 +752,7 @@ static int do_mmc_bootbus(struct cmd_tbl *cmdtp,
> >> int flag,
> >> reset = simple_strtoul(argv[3], NULL, 10);
> >> mode = simple_strtoul(argv[4], NULL, 10);
> >>
> >> - mmc = init_mmc_device(dev, false);
> >> + mmc = init_mmc_device(dev, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -789,7 +815,7 @@ static int do_mmc_boot_resize(struct cmd_tbl
> >> *cmdtp, int flag,
> >> bootsize = simple_strtoul(argv[2], NULL, 10);
> >> rpmbsize = simple_strtoul(argv[3], NULL, 10);
> >>
> >> - mmc = init_mmc_device(dev, false);
> >> + mmc = init_mmc_device(dev, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -844,7 +870,7 @@ static int do_mmc_partconf(struct cmd_tbl *cmdtp,
> >> int flag,
> >>
> >> dev = simple_strtoul(argv[1], NULL, 10);
> >>
> >> - mmc = init_mmc_device(dev, false);
> >> + mmc = init_mmc_device(dev, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -887,7 +913,7 @@ static int do_mmc_rst_func(struct cmd_tbl *cmdtp,
> >> int flag,
> >> return CMD_RET_USAGE;
> >> }
> >>
> >> - mmc = init_mmc_device(dev, false);
> >> + mmc = init_mmc_device(dev, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -939,7 +965,7 @@ static int do_mmc_bkops_enable(struct cmd_tbl
> >> *cmdtp, int flag,
> >>
> >> dev = simple_strtoul(argv[1], NULL, 10);
> >>
> >> - mmc = init_mmc_device(dev, false);
> >> + mmc = init_mmc_device(dev, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >>
> >> @@ -958,7 +984,7 @@ static int do_mmc_boot_wp(struct cmd_tbl *cmdtp,
> >> int flag,
> >> int err;
> >> struct mmc *mmc;
> >>
> >> - mmc = init_mmc_device(curr_device, false);
> >> + mmc = init_mmc_device(curr_device, false, MMC_MODES_END);
> >> if (!mmc)
> >> return CMD_RET_FAILURE;
> >> if (IS_SD(mmc)) {
> >> @@ -983,9 +1009,9 @@ static struct cmd_tbl cmd_mmc[] = { #if
> >> CONFIG_IS_ENABLED(CMD_MMC_SWRITE)
> >> U_BOOT_CMD_MKENT(swrite, 3, 0, do_mmc_sparse_write, "", ""),
> #endif
> >> - U_BOOT_CMD_MKENT(rescan, 1, 1, do_mmc_rescan, "", ""),
> >> + U_BOOT_CMD_MKENT(rescan, 2, 1, do_mmc_rescan, "", ""),
> >> U_BOOT_CMD_MKENT(part, 1, 1, do_mmc_part, "", ""),
> >> - U_BOOT_CMD_MKENT(dev, 3, 0, do_mmc_dev, "", ""),
> >> + 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(hwpartition, 28, 0, do_mmc_hwpartition, "",
> ""),
> >> @@ -1042,9 +1068,12 @@ U_BOOT_CMD(
> >> "mmc swrite addr blk#\n"
> >> #endif
> >> "mmc erase blk# cnt\n"
> >> - "mmc rescan\n"
> >> + "mmc rescan [mode]\n"
> >> "mmc part - lists available partition on current mmc device\n"
> >> - "mmc dev [dev] [part] - show or set current mmc device [partition]\n"
> >> + "mmc dev [dev] [part] [mode] - show or set current mmc device
> >> [partition] and set mode\n"
> >> + " - the required speed mode is passed as the index from the
> >> +following
> >> list\n"
> >> + " [MMC_LEGACY, MMC_HS, SD_HS, MMC_HS_52, MMC_DDR_52,
> >> UHS_SDR12, UHS_SDR25,\n"
> >> + " UHS_SDR50, UHS_DDR50, UHS_SDR104, MMC_HS_200,
> >> MMC_HS_400, MMC_HS_400_ES]\n"
> >> "mmc list - lists available devices\n"
> >> "mmc wp - power on write protect boot partitions\n"
> >> #if CONFIG_IS_ENABLED(MMC_HW_PARTITIONING)
> >> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
> >> index 0e13238c7e7a..66fe8bed984b 100644
> >> --- a/drivers/mmc/mmc-uclass.c
> >> +++ b/drivers/mmc/mmc-uclass.c
> >> @@ -415,6 +415,9 @@ int mmc_bind(struct udevice *dev, struct mmc
> >> *mmc, const struct mmc_config *cfg)
> >> bdesc->part_type = cfg->part_type;
> >> mmc->dev = dev;
> >>
> >> +#if CONFIG_IS_ENABLED(MMC_SPEED_MODE_SET)
> >> + mmc->user_speed_mode = MMC_MODES_END; #endif
> >
> > same here
> >
> >> return 0;
> >> }
> >>
> >> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index
> >> 8078a89f18cb..b24a45d458d4 100644
> >> --- a/drivers/mmc/mmc.c
> >> +++ b/drivers/mmc/mmc.c
> >> @@ -2863,6 +2863,24 @@ int mmc_start_init(struct mmc *mmc)
> >> */
> >> mmc->host_caps = mmc->cfg->host_caps | MMC_CAP(MMC_LEGACY)
> >> |
> >> MMC_CAP(MMC_LEGACY) | MMC_MODE_1BIT;
> >> +
> >> +#if CONFIG_IS_ENABLED(MMC_SPEED_MODE_SET)
> >
> > here as well
> >
>
> I can't use if (IS_ENABLED(CONFIG_MMC_SPEED_MODE_SET) because of [1],
> as it would lead to build errors.
>
> If it is ok, to remove #ifdef around [1] and add user_speed_mode field
> irrespective of config, then I will be able to use it.
I see no problem in removing [1]. Please go ahead.
Thanks,
Ashok
>
> Thanks,
> Aswath
>
> > Thanks,
> > Ashok
> >
> >> + if (mmc->user_speed_mode != MMC_MODES_END) {
> >> + int i;
> >> + /* set host caps */
> >> + if (mmc->host_caps & MMC_CAP(mmc->user_speed_mode)) {
> >> + /* Remove all existing speed capabilities */
> >> + for (i = MMC_LEGACY; i < MMC_MODES_END; i++)
> >> + mmc->host_caps &= ~MMC_CAP(i);
> >> + mmc->host_caps |= (MMC_CAP(mmc-
> >>> user_speed_mode)
> >> + | MMC_CAP(MMC_LEGACY) |
> >> + MMC_CAP(MMC_LEGACY) |
> >> MMC_MODE_1BIT);
> >> + } else {
> >> + pr_err("bus_mode requested is not supported\n");
> >> + return -EINVAL;
> >> + }
> >> + }
> >> +#endif
> >> #if CONFIG_IS_ENABLED(DM_MMC)
> >> mmc_deferred_probe(mmc);
> >> #endif
> >> diff --git a/include/mmc.h b/include/mmc.h index
> >> 0bf19de20e52..e64ef7611dd7 100644
> >> --- a/include/mmc.h
> >> +++ b/include/mmc.h
> >> @@ -726,6 +726,10 @@ struct mmc {
> >> */
> >> u32 quirks;
> >> u8 hs400_tuning;
> >> +
> >> +#if CONFIG_IS_ENABLED(MMC_SPEED_MODE_SET)
> >> + enum bus_mode user_speed_mode;
> >> +#endif
>
> [1]
>
> >> };
> >>
> >> #if CONFIG_IS_ENABLED(DM_MMC)
> >> --
> >> 2.17.1
> >
