mmc: test mmc bus width on startup

For we don't know mmc bus width from reading registers, the only way to check is to test.

Change-Id Ia50df480bd14349e36ac8217d8d290cf732db089

Signed-off-by Lei Wen <[email protected]>

---

Quoted 6 lines · Lei Wen

For we don't know mmc bus width from reading registers, the only way to check is to test.

Change-Id Ia50df480bd14349e36ac8217d8d290cf732db089

Signed-off-by Lei Wen <[email protected]>

---

[...]

Quoted 10 lines

+ ? ? ? ? ? ? ? ? ? ? if (!err && ext_csd[160] == test_csd[160]
+ ? ? ? ? ? ? ? ? ? ? ? ? && ext_csd[175] == test_csd[175]
+ ? ? ? ? ? ? ? ? ? ? ? ? && ext_csd[192] == test_csd[192]
+ ? ? ? ? ? ? ? ? ? ? ? ? && ext_csd[224] == test_csd[224]
+ ? ? ? ? ? ? ? ? ? ? ? ? && memcmp(&ext_csd[212], &test_csd[212], 4) == 0) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc->card_caps |= width;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? }
? ? ? ? ? ? ? }

Quoted 1 line

? ? ? ? ? ? ? if (mmc->card_caps & MMC_MODE_HS) {

Hi Lei,

I see you're accessing the ext_csd via some magic constants. Can you possibly convert it to structure-based access ? Like struct ext_csd csd; csd->member ...

Previous magic number is hard to parse its meaning, change it to respective macro definition

Signed-off-by Lei Wen <[email protected]>

include/mmc.h +2 -7

@@ -145,13 +145,15 @@
 /*
  * 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 */
-#define EXT_CSD_CARD_TYPE	196	/* RO */
-#define EXT_CSD_REV		192	/* RO */
-#define EXT_CSD_SEC_CNT		212	/* RO, 4 bytes */
+#define EXT_CSD_PARTITIONING_SUPPORT	160	/* RO */
+#define EXT_CSD_ERASE_GROUP_DEF		175	/* R/W */

Quoted 4 lines · Lei Wen

Previous magic number is hard to parse its meaning, change it to respective macro definition

Signed-off-by Lei Wen <[email protected]>

[..]

Quoted 23 lines

include/mmc.h +0 -0

@@ -145,13 +145,15 @@

?/*
? * EXT_CSD fields
? */

include/mmc.h +9 -7

-
-#define EXT_CSD_PART_CONF ? ?179 ? ? /* R/W */
-#define EXT_CSD_BUS_WIDTH ? ?183 ? ? /* R/W */
-#define EXT_CSD_HS_TIMING ? ?185 ? ? /* R/W */
-#define EXT_CSD_CARD_TYPE ? ?196 ? ? /* RO */
-#define EXT_CSD_REV ? ? ? ? ?192 ? ? /* RO */
-#define EXT_CSD_SEC_CNT ? ? ? ? ? ? ?212 ? ? /* RO, 4 bytes */
+#define EXT_CSD_PARTITIONING_SUPPORT 160 ? ? /* RO */
+#define EXT_CSD_ERASE_GROUP_DEF ? ? ? ? ? ? ?175 ? ? /* R/W */
+#define EXT_CSD_PART_CONF ? ? ? ? ? ?179 ? ? /* R/W */
+#define EXT_CSD_BUS_WIDTH ? ? ? ? ? ?183 ? ? /* R/W */
+#define EXT_CSD_HS_TIMING ? ? ? ? ? ?185 ? ? /* R/W */
+#define EXT_CSD_REV ? ? ? ? ? ? ? ? ?192 ? ? /* RO */
+#define EXT_CSD_CARD_TYPE ? ? ? ? ? ?196 ? ? /* RO */
+#define EXT_CSD_SEC_CNT ? ? ? ? ? ? ? ? ? ? ?212 ? ? /* RO, 4 bytes */
+#define EXT_CSD_HC_ERASE_GRP_SIZE ? ?224 ? ? /* RO */

Quoted 2 lines

?/*
? * EXT_CSD field definitions

Hi Lei,
this is better, but what about structure-based access ?

struct somrthing {
?u8 a1;
?u8 a2;
...
};

Like this.

Also, CC Andy.

Hi Marek,

Quoted 43 lines · Marek Vasut

Quoted 4 lines · Lei Wen

Previous magic number is hard to parse its meaning, change it to respective macro definition

Signed-off-by Lei Wen <[email protected]>

[..]

Quoted 15 lines

include/mmc.h +2 -7

@@ -145,13 +145,15 @@
 /*
  * 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 */
-#define EXT_CSD_CARD_TYPE    196     /* RO */
-#define EXT_CSD_REV          192     /* RO */
-#define EXT_CSD_SEC_CNT              212     /* RO, 4 bytes */
+#define EXT_CSD_PARTITIONING_SUPPORT 160     /* RO */
+#define EXT_CSD_ERASE_GROUP_DEF              175     /* R/W */

include/mmc.h line 159

Quoted 10 lines

include/mmc.h +7 -0

+#define EXT_CSD_PART_CONF            179     /* R/W */
+#define EXT_CSD_BUS_WIDTH            183     /* R/W */
+#define EXT_CSD_HS_TIMING            185     /* R/W */
+#define EXT_CSD_REV                  192     /* RO */
+#define EXT_CSD_CARD_TYPE            196     /* RO */
+#define EXT_CSD_SEC_CNT                      212     /* RO, 4 bytes */
+#define EXT_CSD_HC_ERASE_GRP_SIZE    224     /* RO */
 
 /*
  * EXT_CSD field definitions

Hi Lei,
this is better, but what about structure-based access ?

struct somrthing {
u8 a1;
u8 a2;
...
};

Quoted 3 lines

Like this.

Also, CC Andy.

The ext_csd current usage in mmc.c is not too much, here I mean only few of the fields of the ext_csd is used, also fully definition of ext_csd member would seems so huge a structure at its appearence...

So macro may looks more concise and could parse from its meaning easily enough.

Anyway, more comments on this welcomes. :)

Best regards,
Lei

The ext_csd current usage in mmc.c is not too much, here I mean only few of the fields of the ext_csd is used, also fully definition of ext_csd member would seems so huge a structure at its appearence...

So macro may looks more concise and could parse from its meaning easily eno= ugh.

Dear Lei Wen,

Quoted 5 lines · In message <CALZhoSRhbf2vMU5oLP3Hwh4yQ4xFip19aJD24Gn4sy-RM6B+Eg@mail.gmail.com>…

The ext_csd current usage in mmc.c is not too much, here I mean only few of the fields of the ext_csd is used, also fully definition of ext_csd member would seems so huge a structure at its appearence...

Quoted 2 lines

So macro may looks more concise and could parse from its meaning easily eno= ugh.

We do not accept (typeless) register offset definitions. Please use a struct, so the compiler has a chance to perform type checking.

Quoted 6 lines

Quoted 3 lines

So macro may looks more concise and could parse from its meaning easily eno= ugh.

We do not accept (typeless) register offset definitions. Please use a struct, so the compiler has a chance to perform type checking.

I check the code again, and find there is a reason for previous defined macro to use.
That is those register offset defined as macro may need later passing as a function
parameter like:

        err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);

So if the ext_csd change to structure, maybe the function call here don't looks like so
concise as before... What do you think for this?

Dear Lei Wen,

Quoted 7 lines · In message <CALZhoSQbvKj0MtqryeHX-4LkvQJR2=B9u_m4yJjfM1mjv2MSEw@mail.gmail.com>…

Quoted 6 lines

Quoted 3 lines

So macro may looks more concise and could parse from its meaning easily eno= ugh.

We do not accept (typeless) register offset definitions. Please use a struct, so the compiler has a chance to perform type checking.

Quoted 6 lines

I check the code again, and find there is a reason for previous defined macro to use.
That is those register offset defined as macro may need later passing as a function
parameter like:
? ? ? ? err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);

I don;t see that any register (which now would be an address in a struct insteadd of being an offset to be added to a base address) would be passed as parameter to mmc_switch().

So this is not an issue at all.

Quoted 3 lines

So if the ext_csd change to structure, maybe the function call here don't looks like so
concise as before... What do you think for this?

The EXT_CSD field definitions are completely independent of the way how you access the registers.

Hi Wolfgang,

Quoted 21 lines · Wolfgang Denk

Dear Lei Wen,

Quoted 7 lines · In message <CALZhoSQbvKj0MtqryeHX-4LkvQJR2=B9u_m4yJjfM1mjv2MSEw@mail.gmail.com>…

Quoted 6 lines

Quoted 3 lines

So macro may looks more concise and could parse from its meaning easily eno= ugh.

We do not accept (typeless) register offset definitions. Please use a struct, so the compiler has a chance to perform type checking.

Quoted 6 lines

I check the code again, and find there is a reason for previous defined macro to use.
That is those register offset defined as macro may need later passing as a function
parameter like:

        err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);

I don;t see that any register (which now would be an address in a struct insteadd of being an offset to be added to a base address) would be passed as parameter to mmc_switch().

Quoted 9 lines

So this is not an issue at all.

Quoted 3 lines

So if the ext_csd change to structure, maybe the function call here don't looks like so
concise as before... What do you think for this?

The EXT_CSD field definitions are completely independent of the way how you access the registers.

So do you means I should keep the EXT_CSD_HS_TIMING? Or I change like below?

from
err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1); to:
err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, &ext_csd.hs_timing - &ext_csd, 1);

Hi Lei Wen,

Quoted 34 lines · Lei Wen

Hi Wolfgang,

Quoted 21 lines · Wolfgang Denk

Dear Lei Wen,

Quoted 7 lines · In message <CALZhoSQbvKj0MtqryeHX-4LkvQJR2=B9u_m4yJjfM1mjv2MSEw@mail.gmail.com>…

Quoted 6 lines

Quoted 3 lines

So macro may looks more concise and could parse from its meaning easily eno= ugh.

We do not accept (typeless) register offset definitions. Please use a struct, so the compiler has a chance to perform type checking.

Quoted 6 lines

I check the code again, and find there is a reason for previous defined macro to use.
That is those register offset defined as macro may need later passing as a function
parameter like:
? ? ? ? err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);

I don;t see that any register (which now would be an address in a struct insteadd of being an offset to be added to a base address) would be passed as parameter to mmc_switch().

Quoted 9 lines

So this is not an issue at all.

Quoted 3 lines

So if the ext_csd change to structure, maybe the function call here don't looks like so
concise as before... What do you think for this?

The EXT_CSD field definitions are completely independent of the way how you access the registers.

Quoted 6 lines

So do you means I should keep the EXT_CSD_HS_TIMING? Or I change like below?

from
err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1); to:
err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, &ext_csd.hs_timing - &ext_csd, 1);

I imagine the compiler will complain that the types for &ext_csd.hs_timing and &ext_csd are different.

Try:
? ? ? ?struct ext_csd *ext_csd_ptr = 0;

? ? ? ?err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, &ext_csd_ptr->hs_timing, 1);

Quoted 7 lines

So do you means I should keep the EXT_CSD_HS_TIMING? Or I change like below?

from
err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1); to:
err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, &ext_csd.hs_timing - &ext_csd, 1);

I imagine the compiler will complain that the types for &ext_csd.hs_timing and &ext_csd are different.

Try:

	struct ext_csd *ext_csd_ptr = 0;

	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, &ext_csd_ptr->hs_timing, 1);

Dear Lei Wen,

Quoted 8 lines · In message <CALButC+bZZwYhE0VT99Yjh_=p0LVJqNMM1yVZSEX3inuTn7PcQ@mail.gmail.com>…

Quoted 7 lines

So do you means I should keep the EXT_CSD_HS_TIMING? Or I change like below?

from
err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1); to:
err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, &ext_csd.hs_timing - &ext_csd, 1);

Quoted 7 lines

I imagine the compiler will complain that the types for &ext_csd.hs_timing and &ext_csd are different.

Try:
? ? ? struct ext_csd *ext_csd_ptr = 0;

? ? ? err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, &ext_csd_ptr->hs_timing, 1);

Umm... no.

The signature of mmc_switch() [as used here - there is also a two argument version; what a mess!] is:

? ? ? ?int mmc_switch(struct mmc *mmc, u8 set, u8 index, u8 value)

The "set" argument is bous, as the function never uses it anywhere.

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

Quoted 5 lines

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

Quoted 1 line

       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?

Dear Lei Wen,

Quoted 5 lines · In message <CALZhoSSygHZ22N=odN4qv44a_1ZxguPQLSRwA3pBbPnXbXjC8g@mail.gmail.com>…

Quoted 5 lines

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

Quoted 7 lines

Quoted 1 line

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

-----Original Message-----
From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Lei Wen
Sent: Thursday, October 06, 2011 8:41 PM
To: Marek Vasut
Cc: Lei Wen; u-boot at lists.denx.de; Andy Fleming
Subject: Re: [U-Boot] [PATCH V2 1/2] mmc: change magic number to macro define

Hi Marek,

Quoted 21 lines · Marek Vasut

Quoted 4 lines · Lei Wen

Previous magic number is hard to parse its meaning, change it to respective macro definition

Signed-off-by Lei Wen <[email protected]>

[..]

Quoted 13 lines

include/mmc.h +0 -0

@@ -145,13 +145,15 @@

?/*
? * EXT_CSD fields
? */

include/mmc.h +0 -7

-
-#define EXT_CSD_PART_CONF ? ?179 ? ? /* R/W */
-#define EXT_CSD_BUS_WIDTH ? ?183 ? ? /* R/W */
-#define EXT_CSD_HS_TIMING ? ?185 ? ? /* R/W */
-#define EXT_CSD_CARD_TYPE ? ?196 ? ? /* RO */
-#define EXT_CSD_REV ? ? ? ? ?192 ? ? /* RO */
-#define EXT_CSD_SEC_CNT ? ? ? ? ? ? ?212 ? ? /* RO, 4 bytes */

Quoted 8 lines

include/mmc.h

Quoted 8 lines

include/mmc.h +8 -0

+#define EXT_CSD_PARTITIONING_SUPPORT 160 ? ? /* RO */
+#define EXT_CSD_ERASE_GROUP_DEF ? ? ? ? ? ? ?175 ? ? /* R/W */
+#define EXT_CSD_PART_CONF ? ? ? ? ? ?179 ? ? /* R/W */
+#define EXT_CSD_BUS_WIDTH ? ? ? ? ? ?183 ? ? /* R/W */
+#define EXT_CSD_HS_TIMING ? ? ? ? ? ?185 ? ? /* R/W */
+#define EXT_CSD_REV ? ? ? ? ? ? ? ? ?192 ? ? /* RO */
+#define EXT_CSD_CARD_TYPE ? ? ? ? ? ?196 ? ? /* RO */
+#define EXT_CSD_SEC_CNT ? ? ? ? ? ? ? ? ? ? ?212 ? ? /* RO, 4 bytes

*/

Quoted 14 lines

Quoted 4 lines

+#define EXT_CSD_HC_ERASE_GRP_SIZE ? ?224 ? ? /* RO */

?/*
? * EXT_CSD field definitions

Hi Lei,
this is better, but what about structure-based access ?

struct somrthing {
?u8 a1;
?u8 a2;
...
};

Quoted 4 lines

Like this.

Also, CC Andy.

The ext_csd current usage in mmc.c is not too much, here I mean only few of
the fields of the ext_csd is used, also fully definition of ext_csd member would seems so huge a structure at its appearence...

So macro may looks more concise and could parse from its meaning easily enough.

Anyway, more comments on this welcomes. :)

Quoted 72 lines

-----Original Message-----
From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Lei Wen
Sent: Thursday, October 06, 2011 8:41 PM
To: Marek Vasut
Cc: Lei Wen; u-boot at lists.denx.de; Andy Fleming
Subject: Re: [U-Boot] [PATCH V2 1/2] mmc: change magic number to macro define

Hi Marek,

Quoted 21 lines · Marek Vasut

Quoted 4 lines · Lei Wen

Previous magic number is hard to parse its meaning, change it to respective macro definition

Signed-off-by Lei Wen <[email protected]>

[..]

Quoted 13 lines

include/mmc.h +0 -0

@@ -145,13 +145,15 @@

?/*
? * EXT_CSD fields
? */

include/mmc.h +0 -7

-
-#define EXT_CSD_PART_CONF ? ?179 ? ? /* R/W */
-#define EXT_CSD_BUS_WIDTH ? ?183 ? ? /* R/W */
-#define EXT_CSD_HS_TIMING ? ?185 ? ? /* R/W */
-#define EXT_CSD_CARD_TYPE ? ?196 ? ? /* RO */
-#define EXT_CSD_REV ? ? ? ? ?192 ? ? /* RO */
-#define EXT_CSD_SEC_CNT ? ? ? ? ? ? ?212 ? ? /* RO, 4 bytes */

Quoted 8 lines

include/mmc.h

Quoted 8 lines

include/mmc.h +8 -0

+#define EXT_CSD_PARTITIONING_SUPPORT 160 ? ? /* RO */
+#define EXT_CSD_ERASE_GROUP_DEF ? ? ? ? ? ? ?175 ? ? /* R/W */
+#define EXT_CSD_PART_CONF ? ? ? ? ? ?179 ? ? /* R/W */
+#define EXT_CSD_BUS_WIDTH ? ? ? ? ? ?183 ? ? /* R/W */
+#define EXT_CSD_HS_TIMING ? ? ? ? ? ?185 ? ? /* R/W */
+#define EXT_CSD_REV ? ? ? ? ? ? ? ? ?192 ? ? /* RO */
+#define EXT_CSD_CARD_TYPE ? ? ? ? ? ?196 ? ? /* RO */
+#define EXT_CSD_SEC_CNT ? ? ? ? ? ? ? ? ? ? ?212 ? ? /* RO, 4 bytes

*/

Quoted 14 lines

Quoted 4 lines

+#define EXT_CSD_HC_ERASE_GRP_SIZE ? ?224 ? ? /* RO */

?/*
? * EXT_CSD field definitions

Hi Lei,
this is better, but what about structure-based access ?

struct somrthing {
?u8 a1;
?u8 a2;
...
};

Quoted 4 lines

Like this.

Also, CC Andy.

The ext_csd current usage in mmc.c is not too much, here I mean only few of
the fields of the ext_csd is used, also fully definition of ext_csd member would seems so huge a structure at its appearence...

So macro may looks more concise and could parse from its meaning easily enough.

Quoted 1 line

Anyway, more comments on this welcomes. :)

Dear Lei
Using c-struct is our strategy, may be full definition is huge, you may skip them inserting padding in the c-struct.

Regards..
Prafulla . .

Quoted 36 lines

-----Original Message-----
From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Lei Wen
Sent: Thursday, October 06, 2011 8:41 PM
To: Marek Vasut
Cc: Lei Wen; u-boot at lists.denx.de; Andy Fleming
Subject: Re: [U-Boot] [PATCH V2 1/2] mmc: change magic number to macro define

Hi Marek,

Quoted 10 lines · Marek Vasut

Hi Lei,
this is better, but what about structure-based access ?

struct somrthing {
u8 a1;
u8 a2;
...
};

Quoted 4 lines

Like this.

Also, CC Andy.

The ext_csd current usage in mmc.c is not too much, here I mean only few of
the fields of the ext_csd is used, also fully definition of ext_csd member would seems so huge a structure at its appearence...

Quoted 4 lines

So macro may looks more concise and could parse from its meaning easily enough.

Anyway, more comments on this welcomes. :)

Dear Lei
Using c-struct is our strategy, may be full definition is huge, you may skip them inserting padding in the c-struct.

Hi Prafulla,

Quoted 23 lines

Quoted 23 lines

Quoted 10 lines

Hi Lei,
this is better, but what about structure-based access ?

struct somrthing {
?u8 a1;
?u8 a2;
...
};

Quoted 4 lines

Like this.

Also, CC Andy.

The ext_csd current usage in mmc.c is not too much, here I mean only few of
the fields of the ext_csd is used, also fully definition of ext_csd member would seems so huge a structure at its appearence...

So macro may looks more concise and could parse from its meaning easily enough.

Quoted 4 lines

Quoted 1 line

Anyway, more comments on this welcomes. :)

Dear Lei
Using c-struct is our strategy, may be full definition is huge, you may skip them inserting padding in the c-struct.

I personally prefer the complete struct to be defined from day one. This means you don't have to patch it when you end up needing to use another member later on and risk fouling up the offsets