[U-Boot] [PATCH] mmc: test mmc bus width on startup

[U-Boot] [PATCH] mmc: test mmc bus width on startup

[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 <leiwen@marvell.com>
---
 drivers/mmc/mmc.c |   39 ++++++++++++++++++++-------------------
 1 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 391bc2b..e384399 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -631,8 +631,6 @@ int mmc_change_freq(struct mmc *mmc)
 	if (mmc->version < MMC_VERSION_4)
 		return 0;
 
-	mmc->card_caps |= MMC_MODE_4BIT;
-
 	err = mmc_send_ext_csd(mmc, ext_csd);
 
 	if (err)
@@ -856,11 +854,11 @@ void mmc_set_bus_width(struct mmc *mmc, uint width)
 
 int mmc_startup(struct mmc *mmc)
 {
-	int err;
+	int err, width;
 	uint mult, freq;
 	u64 cmult, csize, capacity;
 	struct mmc_cmd cmd;
-	char ext_csd[512];
+	char ext_csd[512], test_csd[512];
 	int timeout = 1000;
 
 #ifdef CONFIG_MMC_SPI_CRC_ON
@@ -1077,26 +1075,29 @@ int mmc_startup(struct mmc *mmc)
 		else
 			mmc_set_clock(mmc, 25000000);
 	} else {
-		if (mmc->card_caps & MMC_MODE_4BIT) {
+		for (width = EXT_CSD_BUS_WIDTH_8; width >= 0; width--) {
 			/* Set the card to use 4 bit*/
 			err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
-					EXT_CSD_BUS_WIDTH,
-					EXT_CSD_BUS_WIDTH_4);
-
-			if (err)
-				return err;
-
-			mmc_set_bus_width(mmc, 4);
-		} else if (mmc->card_caps & MMC_MODE_8BIT) {
-			/* Set the card to use 8 bit*/
-			err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
-					EXT_CSD_BUS_WIDTH,
-					EXT_CSD_BUS_WIDTH_8);
+					EXT_CSD_BUS_WIDTH, width);
 
 			if (err)
-				return err;
+				continue;
 
-			mmc_set_bus_width(mmc, 8);
+			if (!width) {
+				mmc_set_bus_width(mmc, 1);
+				break;
+			} else
+				mmc_set_bus_width(mmc, 4 * width);
+
+			err = mmc_send_ext_csd(mmc, test_csd);
+			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;
+			}
 		}
 
 		if (mmc->card_caps & MMC_MODE_HS) {
-- 
1.7.0.4

[U-Boot] [PATCH] mmc: test mmc bus width on startup

[Marek Vasut]

On Monday, October 03, 2011 11:03:56 AM Lei Wen wrote:
> 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 <leiwen@marvell.com>
> ---

[...]

> +			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;
> +			}
>  		}
> 
>  		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 ... 
?

Cheers

[U-Boot] [PATCH] mmc: test mmc bus width on startup

[Lei Wen]

Hi Marek,

On Mon, Oct 3, 2011 at 6:41 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Monday, October 03, 2011 11:03:56 AM Lei Wen wrote:
>> 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 <leiwen@marvell.com>
>> ---
>
> [...]
>
>> + ? ? ? ? ? ? ? ? ? ? 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;
>> + ? ? ? ? ? ? ? ? ? ? }
>> ? ? ? ? ? ? ? }
>>
>> ? ? ? ? ? ? ? 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 ...

This suggestion is reasonable, I would do a magic number clean up in
the mmc.c together
with this patch.

Thanks,
Lei

[U-Boot] [PATCH V2 0/2] test mmc bus on startup

[Lei Wen]

Changelog:
V2: Clean up mmc.c to change all magic number for ext_csd to the macro
    definition

Lei Wen (2):
  mmc: change magic number to macro define
  mmc: test mmc bus width on startup

 drivers/mmc/mmc.c |   66 ++++++++++++++++++++++++++++++----------------------
 include/mmc.h     |   16 +++++++-----
 2 files changed, 47 insertions(+), 35 deletions(-)

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

[Lei Wen]

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

Signed-off-by: Lei Wen <leiwen@marvell.com>
---
Changelog:
V2: change all magic number for ext_csd to macro definition

 drivers/mmc/mmc.c |   21 ++++++++++++---------
 include/mmc.h     |   16 +++++++++-------
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 391bc2b..1d75940 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -638,7 +638,7 @@ int mmc_change_freq(struct mmc *mmc)
 	if (err)
 		return err;
 
-	cardtype = ext_csd[196] & 0xf;
+	cardtype = ext_csd[EXT_CSD_CARD_TYPE] & 0xf;
 
 	err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);
 
@@ -652,7 +652,7 @@ int mmc_change_freq(struct mmc *mmc)
 		return err;
 
 	/* No high-speed support */
-	if (!ext_csd[185])
+	if (!ext_csd[EXT_CSD_HS_TIMING])
 		return 0;
 
 	/* High Speed is set, there are two types: 52MHz and 26MHz */
@@ -1006,14 +1006,16 @@ int mmc_startup(struct mmc *mmc)
 	if (!IS_SD(mmc) && (mmc->version >= MMC_VERSION_4)) {
 		/* check  ext_csd version and capacity */
 		err = mmc_send_ext_csd(mmc, ext_csd);
-		if (!err & (ext_csd[192] >= 2)) {
+		if (!err & (ext_csd[EXT_CSD_REV] >= 2)) {
 			/*
 			 * According to the JEDEC Standard, the value of
 			 * ext_csd's capacity is valid if the value is more
 			 * than 2GB
 			 */
-			capacity = ext_csd[212] << 0 | ext_csd[213] << 8 |
-				   ext_csd[214] << 16 | ext_csd[215] << 24;
+			capacity = ext_csd[EXT_CSD_SEC_CNT] << 0
+					| ext_csd[EXT_CSD_SEC_CNT + 1] << 8
+					| ext_csd[EXT_CSD_SEC_CNT + 2] << 16
+					| ext_csd[EXT_CSD_SEC_CNT + 3] << 24;
 			capacity *= 512;
 			if ((capacity >> 20) > 2 * 1024)
 				mmc->capacity = capacity;
@@ -1024,8 +1026,9 @@ int mmc_startup(struct mmc *mmc)
 		 * group size from ext_csd directly, or calculate
 		 * the group size from the csd value.
 		 */
-		if (ext_csd[175])
-			mmc->erase_grp_size = ext_csd[224] * 512 * 1024;
+		if (ext_csd[EXT_CSD_ERASE_GROUP_DEF])
+			mmc->erase_grp_size =
+			      ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] * 512 * 1024;
 		else {
 			int erase_gsz, erase_gmul;
 			erase_gsz = (mmc->csd[2] & 0x00007c00) >> 10;
@@ -1035,8 +1038,8 @@ int mmc_startup(struct mmc *mmc)
 		}
 
 		/* store the partition info of emmc */
-		if (ext_csd[160] & PART_SUPPORT)
-			mmc->part_config = ext_csd[179];
+		if (ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & PART_SUPPORT)
+			mmc->part_config = ext_csd[EXT_CSD_PART_CONF];
 	}
 
 	if (IS_SD(mmc))
diff --git a/include/mmc.h b/include/mmc.h
index 53aff9b..015a7f3 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -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 */
+#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
-- 
1.7.0.4

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

[Marek Vasut]

On Tuesday, October 04, 2011 08:35:10 AM Lei Wen wrote:
> Previous magic number is hard to parse its meaning, change it to
> respective macro definition
> 
> Signed-off-by: Lei Wen <leiwen@marvell.com>

[..]

> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -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 */
> +#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;
...
};

Like this.

Also, CC Andy.

Cheers

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

[Lei Wen]

Hi Marek,

On Tue, Oct 4, 2011 at 8:07 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Tuesday, October 04, 2011 08:35:10 AM Lei Wen wrote:
>> Previous magic number is hard to parse its meaning, change it to
>> respective macro definition
>>
>> Signed-off-by: Lei Wen <leiwen@marvell.com>
>
> [..]
>
>> --- a/include/mmc.h
>> +++ b/include/mmc.h
>> @@ -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 */
>> +#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;
> ...
> };
>
> 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

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

[Marek Vasut]

On Thursday, October 06, 2011 05:10:32 PM Lei Wen wrote:
> Hi Marek,
> 
> On Tue, Oct 4, 2011 at 8:07 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > On Tuesday, October 04, 2011 08:35:10 AM Lei Wen wrote:
> >> Previous magic number is hard to parse its meaning, change it to
> >> respective macro definition
> >> 
> >> Signed-off-by: Lei Wen <leiwen@marvell.com>
> > 
> > [..]
> > 
> >> --- a/include/mmc.h
> >> +++ b/include/mmc.h
> >> @@ -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 */
> >> +#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;
> > ...
> > };
> > 
> > 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

Hi Lei,

let's see what Andy thinks of this approach.

Cheers

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

[Wolfgang Denk]

Dear Lei Wen,

In message <CALZhoSRhbf2vMU5oLP3Hwh4yQ4xFip19aJD24Gn4sy-RM6B+Eg@mail.gmail.com> you wrote:
>
> 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.

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"An open mind has but one disadvantage: it collects dirt."
                                                    - a saying at RPI

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

[Lei Wen]

Hi Wolfgang,

On Fri, Oct 7, 2011 at 1:39 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <CALZhoSRhbf2vMU5oLP3Hwh4yQ4xFip19aJD24Gn4sy-RM6B+Eg@mail.gmail.com> you wrote:
>>
>> 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.
>
> 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?

Thanks,
Lei

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

[Wolfgang Denk]

Dear Lei Wen,

In message <CALZhoSQbvKj0MtqryeHX-4LkvQJR2=B9u_m4yJjfM1mjv2MSEw@mail.gmail.com> you wrote:
> 
> >> 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);

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.

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
######## This message was made from 100% recycled electrons. ########

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

[Lei Wen]

Hi Wolfgang,

On Tue, Oct 11, 2011 at 1:33 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <CALZhoSQbvKj0MtqryeHX-4LkvQJR2=B9u_m4yJjfM1mjv2MSEw@mail.gmail.com> you wrote:
>>
>> >> 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);
>
> 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.
>
>> 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);

Best regards,
Lei

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

[Graeme Russ]

Hi Lei Wen,

On Tue, Oct 11, 2011 at 11:48 AM, Lei Wen <adrian.wenl@gmail.com> wrote:
> Hi Wolfgang,
>
> On Tue, Oct 11, 2011 at 1:33 AM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Lei Wen,
>>
>> In message <CALZhoSQbvKj0MtqryeHX-4LkvQJR2=B9u_m4yJjfM1mjv2MSEw@mail.gmail.com> you wrote:
>>>
>>> >> 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);
>>
>> 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.
>>
>>> 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);

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

Regards,

Graeme

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

[Lei Wen]

Hi Graeme,

On Tue, Oct 11, 2011 at 9:09 AM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Lei Wen,
>
> On Tue, Oct 11, 2011 at 11:48 AM, Lei Wen <adrian.wenl@gmail.com> wrote:
>> Hi Wolfgang,
>>
>> On Tue, Oct 11, 2011 at 1:33 AM, Wolfgang Denk <wd@denx.de> wrote:
>>> Dear Lei Wen,
>>>
>>> In message <CALZhoSQbvKj0MtqryeHX-4LkvQJR2=B9u_m4yJjfM1mjv2MSEw@mail.gmail.com> you wrote:
>>>>
>>>> >> 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);
>>>
>>> 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.
>>>
>>>> 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);
>
> 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);

Yes, it looks more clear than mine. If have no other suggestion, I
would do like this to format my patches.

Thanks,
Lei

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

[Wolfgang Denk]

Dear Lei Wen,

In message <CALButC+bZZwYhE0VT99Yjh_=p0LVJqNMM1yVZSEX3inuTn7PcQ@mail.gmail.com> Graeme Russ wrote:
> 
> > 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);

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


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
That Microsoft, the Trabant of the operating  system  world,  may  be
glancing  over the Berlin Wall at the Audis and BMWs and Mercedes. In
their own universe Trabants and Ladas were mainstream too...
                                                   -- Evan Leibovitch

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

[Lei Wen]

Hi Wolfgang,

On Fri, Oct 14, 2011 at 4:09 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <CALButC+bZZwYhE0VT99Yjh_=p0LVJqNMM1yVZSEX3inuTn7PcQ@mail.gmail.com> Graeme Russ wrote:
>>
>> > 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);
>
> 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);
>
>

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?

Thanks,
Lei

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

[Wolfgang Denk]

Dear Lei Wen,

In message <CALZhoSSygHZ22N=odN4qv44a_1ZxguPQLSRwA3pBbPnXbXjC8g@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.



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Life would be so much easier if everyone read the manual.

[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@denx.de> wrote:
> Dear Lei Wen,
>
> In message <CALZhoSSygHZ22N=odN4qv44a_1ZxguPQLSRwA3pBbPnXbXjC8g@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] [PATCH V2 1/2] mmc: change magic number to macro define

[Wolfgang Denk]

Dear Lei Wen,

In message <CALZhoSTNDr9u9AjQKu8F8mx_9jdpBBRHJLOCYRPzUdCB63dQJA@mail.gmail.com> you wrote:
> 
> 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?

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

[Prafulla Wadaskar]

> -----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,
> 
> On Tue, Oct 4, 2011 at 8:07 PM, Marek Vasut <marek.vasut@gmail.com>
> wrote:
> > On Tuesday, October 04, 2011 08:35:10 AM Lei Wen wrote:
> >> Previous magic number is hard to parse its meaning, change it to
> >> respective macro definition
> >>
> >> Signed-off-by: Lei Wen <leiwen@marvell.com>
> >
> > [..]
> >
> >> --- a/include/mmc.h
> >> +++ b/include/mmc.h
> >> @@ -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 */
> >> +#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;
> > ...
> > };
> >
> > 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. :)

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

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

[Lei Wen]

On Fri, Oct 7, 2011 at 4:36 PM, Prafulla Wadaskar <prafulla@marvell.com> wrote:
>
>
>> -----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,
>>
>> On Tue, Oct 4, 2011 at 8:07 PM, Marek Vasut <marek.vasut@gmail.com>
>> wrote:
>> > On Tuesday, October 04, 2011 08:35:10 AM Lei Wen wrote:
>> >> Previous magic number is hard to parse its meaning, change it to
>> >> respective macro definition
>> >>
>> >> Signed-off-by: Lei Wen <leiwen@marvell.com>
>> >
>> > [..]
>> >
>> >> --- a/include/mmc.h
>> >> +++ b/include/mmc.h
>> >> @@ -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 */
>> >> +#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;
>> > ...
>> > };
>> >
>> > 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. :)
>
> 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 . .
>

Ok... Got that.
I would send patch soon using the c style.

Thanks,
Lei

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

[Graeme Russ]

Hi Prafulla,

On Fri, Oct 7, 2011 at 7:36 PM, Prafulla Wadaskar <prafulla@marvell.com> wrote:
>
>
>> -----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,
>>
>> On Tue, Oct 4, 2011 at 8:07 PM, Marek Vasut <marek.vasut@gmail.com>
>> wrote:

>> >
>> > Hi Lei,
>> > this is better, but what about structure-based access ?
>> >
>> > struct somrthing {
>> >  u8 a1;
>> >  u8 a2;
>> > ...
>> > };
>> >
>> > 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. :)
>
> 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

Regards,

Graeme

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

[Andy Fleming]

On Mon, Oct 10, 2011 at 8:12 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Prafulla,
>
>>> >
>>> > Hi Lei,
>>> > this is better, but what about structure-based access ?
>>> >
>>> > struct somrthing {
>>> > ?u8 a1;
>>> > ?u8 a2;
>>> > ...
>>> > };
>>> >
>>> > 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. :)
>>
>> 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

One possible issue with defining a struct for the ext CSD is that the
512-byte structure changes depending on the version. This will require
great care to make sure the unions are all set up properly to cover
the various specs.

Linux uses a character array with index offsets, but parses the ext
csd into a software structure in the "card" object.

I don't object to using a struct, but we may find it's a bit
heavyweight. I do think it's ugly to pass in pointer math to
mmc_switch.  Let's hide it with a macro:

#define mmc_ext_csd_offset(x) (((struct ext_csd *)0)->x)

Which would remake that:

err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, mmc_ext_csd_offset(hs_timing), 1);

[U-Boot] [PATCH V2 2/2] mmc: test mmc bus width on startup

[Lei Wen]

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

Current compare offset is:
EXT_CSD_PARTITIONING_SUPPORT
EXT_CSD_ERASE_GROUP_DEF
EXT_CSD_REV
EXT_CSD_HC_ERASE_GRP_SIZE
EXT_CSD_SEC_CNT

Signed-off-by: Lei Wen <leiwen@marvell.com>
---
Changelog:
V2: change all magic number for ext_csd to macro definition

 drivers/mmc/mmc.c |   45 ++++++++++++++++++++++++++-------------------
 1 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 1d75940..ea19d27 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -631,8 +631,6 @@ int mmc_change_freq(struct mmc *mmc)
 	if (mmc->version < MMC_VERSION_4)
 		return 0;
 
-	mmc->card_caps |= MMC_MODE_4BIT;
-
 	err = mmc_send_ext_csd(mmc, ext_csd);
 
 	if (err)
@@ -856,11 +854,11 @@ void mmc_set_bus_width(struct mmc *mmc, uint width)
 
 int mmc_startup(struct mmc *mmc)
 {
-	int err;
+	int err, width;
 	uint mult, freq;
 	u64 cmult, csize, capacity;
 	struct mmc_cmd cmd;
-	char ext_csd[512];
+	char ext_csd[512], test_csd[512];
 	int timeout = 1000;
 
 #ifdef CONFIG_MMC_SPI_CRC_ON
@@ -1080,26 +1078,35 @@ int mmc_startup(struct mmc *mmc)
 		else
 			mmc_set_clock(mmc, 25000000);
 	} else {
-		if (mmc->card_caps & MMC_MODE_4BIT) {
+		for (width = EXT_CSD_BUS_WIDTH_8; width >= 0; width--) {
 			/* Set the card to use 4 bit*/
 			err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
-					EXT_CSD_BUS_WIDTH,
-					EXT_CSD_BUS_WIDTH_4);
-
-			if (err)
-				return err;
-
-			mmc_set_bus_width(mmc, 4);
-		} else if (mmc->card_caps & MMC_MODE_8BIT) {
-			/* Set the card to use 8 bit*/
-			err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
-					EXT_CSD_BUS_WIDTH,
-					EXT_CSD_BUS_WIDTH_8);
+					EXT_CSD_BUS_WIDTH, width);
 
 			if (err)
-				return err;
+				continue;
 
-			mmc_set_bus_width(mmc, 8);
+			if (!width) {
+				mmc_set_bus_width(mmc, 1);
+				break;
+			} else
+				mmc_set_bus_width(mmc, 4 * width);
+
+			err = mmc_send_ext_csd(mmc, test_csd);
+			if (!err && ext_csd[EXT_CSD_PARTITIONING_SUPPORT] \
+				    == test_csd[EXT_CSD_PARTITIONING_SUPPORT]
+				 && ext_csd[EXT_CSD_ERASE_GROUP_DEF] \
+				    == test_csd[EXT_CSD_ERASE_GROUP_DEF] \
+				 && ext_csd[EXT_CSD_REV] \
+				    == test_csd[EXT_CSD_REV]
+				 && ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] \
+				    == test_csd[EXT_CSD_HC_ERASE_GRP_SIZE]
+				 && memcmp(&ext_csd[EXT_CSD_SEC_CNT], \
+					&test_csd[EXT_CSD_SEC_CNT], 4) == 0) {
+
+				mmc->card_caps |= width;
+				break;
+			}
 		}
 
 		if (mmc->card_caps & MMC_MODE_HS) {
-- 
1.7.0.4