[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Lei Wen]

Hi Andy,

On Thu, Apr 28, 2011 at 1:00 AM, Andy Fleming <afleming at gmail.com> wrote:
> On Fri, Apr 15, 2011 at 9:17 PM, Lei Wen <leiwen at marvell.com> wrote:
>> From now on, follow the general rule "mmc dev [dev]" to change the
>> mmc command applied device, like ide and usb...
>>
>> Signed-off-by: Lei Wen <leiwen at marvell.com>
>> ---
>> Changelog:
>> V2: use the "mmc_cur_dev" to replace explicitly specify the dev num
>>    in mmc command.
>
>
> I'd really prefer if there were still the option to specify the device
> in the read/write commands. I appreciate the convenience of not having
> to specify it every time, but I feel that by doing things this way, we
> create an artificial separation between a transaction, and the target
> of the transaction. It tends to encourage a notion that a transaction
> can only be done to the global-current device, which is fine for
> command-line interfaces, but can result in broken programming
> interfaces
>
>
>
>>  int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>  {
>> -       int rc = 0;
>> +       if (argc < 2)
>> +               return cmd_usage(cmdtp);
>>
>> -       switch (argc) {
>> -       case 3:
>> -               if (strcmp(argv[1], "rescan") == 0) {
>> -                       int dev = simple_strtoul(argv[2], NULL, 10);
>> -                       struct mmc *mmc = find_mmc_device(dev);
>> +       if (strcmp(argv[1], "rescan") == 0) {
>> +               struct mmc *mmc = find_mmc_device(mmc_cur_dev);
>>
>> -                       if (!mmc)
>> -                               return 1;
>> +               if (!mmc)
>> +                       return 1;
>> +
>> +               mmc_init(mmc);
>>
>> -                       mmc_init(mmc);
>> +               return 0;
>> +       } else if (strncmp(argv[1], "part", 4) == 0) {
>> +               block_dev_desc_t *mmc_dev;
>> +               struct mmc *mmc = find_mmc_device(mmc_cur_dev);
>>
>> +               if (!mmc) {
>> +                       puts("no mmc devices available\n");
>
> Technically, this just means that no device is selected. I'm sure that
> would only happen if no devices were available, but that's an
> assumption carried outside of the vicinity of this code. Someone might
> change that assumption (for instance, if something could cause an mmc
> device to be removed)

If that device be removed, I think the following mmc_init could check it out,
wouldn't it? For this check, my understanding is to see whether that slot has
been register or not, then following init would double check device whether
function.

>
>> +                       return 1;
>> +               }
>> +               mmc_init(mmc);
>> +               mmc_dev = mmc_get_dev(mmc_cur_dev);
>> +               if (mmc_dev != NULL &&
>> +                               mmc_dev->type != DEV_TYPE_UNKNOWN) {
>> +                       print_part(mmc_dev);
>
> [...]
>
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index 6805b33..377f13a 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -34,7 +34,7 @@
>>  #include <div64.h>
>>
>>  static struct list_head mmc_devices;
>> -static int cur_dev_num = -1;
>
>
> Please don't remove this, or conflate this value with the global
> "current" pointer. This is used to assign the block dev num.
> mmc_cur_dev is a runtime value that will change.  If, for some reason,
> someone needs to interact with an mmc device before all of them have
> come up, we will utterly break.  This is the sort of reason that I'm
> not big on this sort of interaction paradigm.
>
>
>> +int mmc_cur_dev = -1;
>
>
> You should make this a pointer to the mmc device, not an integer.
> Also, you should keep it in the cmd_mmc file.  The *only* place this
> should be used is to declare what the current device for user
> interaction is. Board code that wants to interact with MMC devices
> should feel free to do so, irrespective of the user (ie, the user's
> desired card should not affect which card the board code accesses, and
> the board code should not cause the user's selection to change).

I agree with your concern.
The main reason I mmc_cur_dev global is for cmd_mmc cannot get the info
of how many slots has been enabled mmc.c

I think make a function in mmc.c like mmc_get_num() which would also fulfill
my need.

>
>
>>
>>  int __board_mmc_getcd(u8 *cd, struct mmc *mmc) {
>>        return -1;
>> @@ -849,7 +849,7 @@ int mmc_register(struct mmc *mmc)
>>  {
>>        /* Setup the universal parts of the block interface just once */
>>        mmc->block_dev.if_type = IF_TYPE_MMC;
>> -       mmc->block_dev.dev = cur_dev_num++;
>> +       mmc->block_dev.dev = mmc_cur_dev++;
>
> Make sure to switch this back
>
>>        mmc->block_dev.removable = 1;
>>        mmc->block_dev.block_read = mmc_bread;
>>        mmc->block_dev.block_write = mmc_bwrite;
>> @@ -936,12 +936,20 @@ void print_mmc_devices(char separator)
>>
>>  int mmc_initialize(bd_t *bis)
>>  {
>> +       int ret = -1;
>>        INIT_LIST_HEAD (&mmc_devices);
>> -       cur_dev_num = 0;
>> +       mmc_cur_dev = 0;
>>
>>        if (board_mmc_init(bis) < 0)
>> -               cpu_mmc_init(bis);
>> +               ret = cpu_mmc_init(bis);
>> +       else
>> +               ret = 0;
>>
>> +       /* Put the device 0 as the default */
>> +       if (ret < 0)
>> +               mmc_cur_dev = -1;
>> +       else
>> +               mmc_cur_dev = 0;
>
> These seems needlessly awkward. I suggest that we can initialize it
> either to zero, or to the last register device, inside mmc_register().
> You don't need "ret" at all, then. Because this should be in cmd_mmc,
> we would add a function to cmd_mmc, like: mmc_set_current_dev(struct
> mmc *)

I agree. mmc_set_current_dev may not need, as cmd_mmc still pass the dev
num when it call the read/write function to the mmc.c.

>
>
>>        print_mmc_devices(',');
>>
>>        return 0;
>> diff --git a/include/mmc.h b/include/mmc.h
>> index fcd0fd1..dd9bf15 100644
>> --- a/include/mmc.h
>> +++ b/include/mmc.h
>> @@ -295,4 +295,7 @@ int atmel_mci_init(void *regs);
>>  int mmc_legacy_init(int verbose);
>>  #endif
>>
>> +#ifdef CONFIG_GENERIC_MMC
>> +extern int mmc_cur_dev;
>> +#endif
>
> And then this isn't necessary, but you can declare mmc_set_current_dev(), here
>
Best regards,
Lei