[U-Boot] [PATCH V2 1/2] mmc: change magic number to macro define

[Lei Wen]

Hi Wolfgang,

On Sat, Oct 15, 2011 at 4:36 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Lei Wen,
>
> In message <CALZhoSSygHZ22N=odN4qv44a_1ZxguPQLSRwA3pBbPnXbXjC8g at mail.gmail.com> you wrote:
>>
>> > And both the "index" and "value" arguments are never used in I/O
>> > context, i. e. they are actual plain integer parameters.  So just keep
>> > it as
>> >
>> >        err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);
>>
>> Should I also keep the EXT_CSD_HS_TIMING like macro for previous
>> ext_csd parsing?
>> Or just add another ext_csd structure definition to the parse the
>> ext_csd, while keep macros
>> to be called by mmc_switch?
>
> Let's try to understand what we are trying to acchieve. The purpose
> of using C structs instead of a notation of "base address + register
> offset" is that this way the struct entries that represent the
> registers now have types, and the compiler can perform proper type
> checking when using these data in I/O accessors.
>
> So we use structs to describe the "memory map" of the hardware, the
> mapping of device registers to addresses, and the data types give
> information about the width of the register (8, 16, 32, ... bit).
>
> Note that all the time we are talking about device registers and I/O
> operations - that is situations where I/O accessors will be used.
>
>
> On the other hand, when it comes to definitions of bit fields, like
> here:
>
> /*
>  * EXT_CSD field definitions
>  */
>
> #define EXT_CSD_CMD_SET_NORMAL          (1 << 0)
> #define EXT_CSD_CMD_SET_SECURE          (1 << 1)
> #define EXT_CSD_CMD_SET_CPSECURE        (1 << 2)
> ...
>
> or of indexes, like here:
>
> /*
>  * EXT_CSD fields
>  */
>
> #define EXT_CSD_PART_CONF       179     /* R/W */
> #define EXT_CSD_BUS_WIDTH       183     /* R/W */
> #define EXT_CSD_HS_TIMING       185     /* R/W */
> ...
>
> ..then a struct makes no sense - we have plain integer constants
> here.
>
>
> To verify, please have a look at the code of mmc_switch():
>
> int mmc_switch(struct mmc *mmc, u8 set, u8 index, u8 value)
> {
>        struct mmc_cmd cmd;
>        int timeout = 1000;
>        int ret;
>
>        cmd.cmdidx = MMC_CMD_SWITCH;
>        cmd.resp_type = MMC_RSP_R1b;
>        cmd.cmdarg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
>                                 (index << 16) |
>                                 (value << 8);
> ...
>        ret = mmc_send_cmd(mmc, &cmd, NULL);
>
>
> As you can see, the arguments are ORed together to form an argument
> to mmc_send_cmd() - they are not used in a I/O accessor or any
> similar function.
>
>
> In short: neither EXT_CSD_CMD_SET_NORMAL nor EXT_CSD_HS_TIMING
> describe a device register that is used in any form of I/O
> operations, so it is OK to leave these as simple #define's; the use
> of a struct would not make sense here.
>

Thanks for your kindly explaination on the c structure usage in UBOOT.
So should I keep the V2 version of this patch, or anything else need
to be modified?

Thanks,
Lei