On 8/30/21 1:18 PM, Aswath Govindraju wrote:
> 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! It's my mis-reading. :)
It's working fine.

Reviewed-by: Jaehoon Chung <jh80.ch...@samsung.com>
Tested-by: Jaehoon Chung <jh80.ch...@samsung.com>


Best Regards,
Jaehoon Chung

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