Hi Jaehoon,

On 28/08/21 10:09 am, Jaehoon Chung wrote:
> Hi Aswath,
> 
> On 8/14/21 2:34 AM, Aswath Govindraju wrote:
>> 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.
> 
> Sorry for replying too late. I have tested with your patch.
> 
> It's confused the usage.
> 
>>
>> Signed-off-by: Aswath Govindraju <a-govindr...@ti.com>
>> ---
>>  cmd/Kconfig              | 10 ++++++++
>>  cmd/mmc.c                | 52 +++++++++++++++++++++++++++++++++-------
>>  drivers/mmc/mmc-uclass.c |  5 +++-
>>  drivers/mmc/mmc.c        | 22 ++++++++++++++++-
>>  include/mmc.h            |  2 ++
>>  5 files changed, 81 insertions(+), 10 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 c67ad7624227..f1e30d0cf64b 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,10 @@ static struct mmc *init_mmc_device(int dev, bool 
>> force_init)
>>  
>>      if (force_init)
>>              mmc->has_init = 0;
>> +
>> +    if (IS_ENABLED(CONFIG_MMC_SPEED_MODE_SET))
>> +            mmc->user_speed_mode = speed_mode;
>> +
>>      if (mmc_init(mmc))
>>              return NULL;
>>  
>> @@ -145,6 +151,11 @@ static struct mmc *init_mmc_device(int dev, bool 
>> force_init)
>>      return mmc;
>>  }
>>  
>> +static struct mmc *init_mmc_device(int dev, bool force_init)
>> +{
>> +    return __init_mmc_device(dev, force_init, MMC_MODES_END);
>> +}
>> +
>>  static int do_mmcinfo(struct cmd_tbl *cmdtp, int flag, int argc,
>>                    char *const argv[])
>>  {
>> @@ -482,8 +493,17 @@ 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);
>> +    } else if (argc == 2) {
>> +            speed_mode = (int)dectoul(argv[1], NULL);
>> +            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;
>>  
>> @@ -515,11 +535,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);
>>      } else if (argc == 2) {
>> -            dev = dectoul(argv[1], NULL);
>> +            dev = (int)dectoul(argv[1], NULL);
>> +            mmc = init_mmc_device(dev, true);
>>      } else if (argc == 3) {
>>              dev = (int)dectoul(argv[1], NULL);
>>              part = (int)dectoul(argv[2], NULL);
>> @@ -528,11 +551,21 @@ static int do_mmc_dev(struct cmd_tbl *cmdtp, int flag,
>>                             PART_ACCESS_MASK);
>>                      return CMD_RET_FAILURE;
>>              }
>> +            mmc = init_mmc_device(dev, true);
>> +    } else if (argc == 4) {
>> +            dev = (int)dectoul(argv[1], NULL);
>> +            part = (int)dectoul(argv[2], NULL);
>> +            if (part > PART_ACCESS_MASK) {
>> +                    printf("#part_num shouldn't be larger than %d\n",
>> +                           PART_ACCESS_MASK);
>> +                    return CMD_RET_FAILURE;
>> +            }
>> +            speed_mode = (int)dectoul(argv[3], NULL);
>> +            mmc = __init_mmc_device(dev, true, speed_mode);
>>      } else {
>>              return CMD_RET_USAGE;
>>      }
>>  
>> -    mmc = init_mmc_device(dev, true);
>>      if (!mmc)
>>              return CMD_RET_FAILURE;
>>  
>> @@ -983,9 +1016,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 +1075,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"
[1]

>> +    "    [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"
> 
> If developer doesn't read a doc file, it's difficult to know the number of 
> mode.
> Because this usage seems that [mode] can be passed by string.
> 
> When i have tested with your patch, I have run the below command.
> # mmc rescan MMC_HS_52
> - but it's always set to legacy mode. So I thought this was not working fine.
> But I have changed to input 3 instead of MMC_HS_52. It's working
> # mmc rescan 3
> 
> Is my test right?
> 

Yes, that is correct. The index of the speed mode has be provided[1].

Thanks,
Aswath

> Best Regards,
> Jaehoon Chung
> 
>>      "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..3ee92d03ca23 100644
>> --- a/drivers/mmc/mmc-uclass.c
>> +++ b/drivers/mmc/mmc-uclass.c
>> @@ -342,6 +342,9 @@ void mmc_do_preinit(void)
>>  
>>              if (!m)
>>                      continue;
>> +
>> +            m->user_speed_mode = MMC_MODES_END;  /* Initialising user set 
>> speed mode */
>> +
>>              if (m->preinit)
>>                      mmc_start_init(m);
>>      }
>> @@ -414,7 +417,7 @@ int mmc_bind(struct udevice *dev, struct mmc *mmc, const 
>> struct mmc_config *cfg)
>>      /* setup initial part type */
>>      bdesc->part_type = cfg->part_type;
>>      mmc->dev = dev;
>> -
>> +    mmc->user_speed_mode = MMC_MODES_END;
>>      return 0;
>>  }
>>  
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index 8078a89f18cb..bd348974ae7d 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -2862,7 +2862,25 @@ int mmc_start_init(struct mmc *mmc)
>>       * timings.
>>       */
>>      mmc->host_caps = mmc->cfg->host_caps | MMC_CAP(MMC_LEGACY) |
>> -                     MMC_CAP(MMC_LEGACY) | MMC_MODE_1BIT;
>> +                     MMC_MODE_1BIT;
>> +
>> +    if (IS_ENABLED(CONFIG_MMC_SPEED_MODE_SET)) {
>> +            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_MODE_1BIT);
>> +                    } else {
>> +                            pr_err("bus_mode requested is not supported\n");
>> +                            return -EINVAL;
>> +                    }
>> +            }
>> +    }
>>  #if CONFIG_IS_ENABLED(DM_MMC)
>>      mmc_deferred_probe(mmc);
>>  #endif
>> @@ -3060,6 +3078,8 @@ int mmc_init_device(int num)
>>      }
>>  
>>      m = mmc_get_mmc_dev(dev);
>> +    m->user_speed_mode = MMC_MODES_END; /* Initialising user set speed mode 
>> */
>> +
>>      if (!m)
>>              return 0;
>>      if (m->preinit)
>> diff --git a/include/mmc.h b/include/mmc.h
>> index 0bf19de20e52..b92e25534021 100644
>> --- a/include/mmc.h
>> +++ b/include/mmc.h
>> @@ -726,6 +726,8 @@ struct mmc {
>>                                */
>>      u32 quirks;
>>      u8 hs400_tuning;
>> +
>> +    enum bus_mode user_speed_mode; /* input speed mode from user */
>>  };
>>  
>>  #if CONFIG_IS_ENABLED(DM_MMC)
>>
> 

Reply via email to