Hi Wolfgang,
On Sat, Oct 15, 2011 at 4:36 AM, Wolfgang Denk <w...@denx.de> wrote:
> Dear Lei Wen,
>
> In message
> <CALZhoSSygHZ22N=odn4qv44a_1zxgupqlsrwa3pbbpnxbxj...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
