[U-Boot] [RFC] mmc: Properly determine maximum supported bus width

[U-Boot] [RFC] mmc: Properly determine maximum supported bus width

[Andy Fleming]

At some point, a confusion arose about the use of the bit
definitions in host_caps for bus widths, and the value
in ext_csd. By coincidence, a simple shift could convert
between one and the other:

MMC_MODE_1BIT = 0, EXT_CSD_BUS_WIDTH_1 = 0
MMC_MODE_4BIT = 0x100, EXT_CSD_BUS_WIDTH_4 = 1
MMC_MODE_8BIT = 0x200, EXT_CSD_BUS_WIDTH_8 = 2

However, as host_caps is a bitmask of supported things,
there is not, in fact, a one-to-one correspondence. host_caps
is capable of containing MODE_4BIT | MODE_8BIT, so nonsensical
things were happening where we would try to set the bus width
to 12.

The new code clarifies the very different namespaces:

host_caps/card_caps = bitmask (MMC_MODE_*)
ext CSD fields are just an index (EXT_CSD_BUS_WIDTH_*)
mmc->bus_width integer number of bits (1, 4, 8)

We create arrays to map between the namespaces, like in Linux.

Signed-off-by: Andy Fleming <afleming@freescale.com>
---

This is, I think, the approach we should use to identify the
proper bus width for an mmc device. Please review and/or test.


 drivers/mmc/mmc.c |   47 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 5fbf956..c379a74 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -867,7 +867,7 @@ void mmc_set_bus_width(struct mmc *mmc, uint width)
 
 int mmc_startup(struct mmc *mmc)
 {
-	int err, width;
+	int err;
 	uint mult, freq;
 	u64 cmult, csize, capacity;
 	struct mmc_cmd cmd;
@@ -1086,21 +1086,44 @@ int mmc_startup(struct mmc *mmc)
 		else
 			mmc->tran_speed = 25000000;
 	} else {
-		width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
-			 MMC_MODE_WIDTH_BITS_SHIFT);
-		for (; width >= 0; width--) {
-			/* Set the card to use 4 bit*/
+		int idx;
+
+		/* An array of possible bus widths in order of preference */
+		static unsigned ext_csd_bits[] = {
+			EXT_CSD_BUS_WIDTH_8,
+			EXT_CSD_BUS_WIDTH_4,
+			EXT_CSD_BUS_WIDTH_1,
+		};
+
+		/* An array to map CSD bus widths to host cap bits */
+		static unsigned ext_to_hostcaps[] = {
+			[EXT_CSD_BUS_WIDTH_4] = MMC_MODE_4BIT,
+			[EXT_CSD_BUS_WIDTH_8] = MMC_MODE_8BIT,
+		};
+
+		/* An array to map chosen bus width to an integer */
+		static unsigned widths[] = {
+			8, 4, 1,
+		};
+
+		for (idx=0; idx < ARRAY_SIZE(ext_csd_bits); idx++) {
+			unsigned int extw = ext_csd_bits[idx];
+
+			/*
+			 * Check to make sure the controller supports
+			 * this bus width, if it's more than 1
+			 */
+			if (extw != EXT_CSD_BUS_WIDTH_1 &&
+					!(mmc->host_caps & ext_to_hostcaps[extw]))
+				continue;
+
 			err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
-					EXT_CSD_BUS_WIDTH, width);
+					EXT_CSD_BUS_WIDTH, extw);
 
 			if (err)
 				continue;
 
-			if (!width) {
-				mmc_set_bus_width(mmc, 1);
-				break;
-			} else
-				mmc_set_bus_width(mmc, 4 * width);
+			mmc_set_bus_width(mmc, widths[idx]);
 
 			err = mmc_send_ext_csd(mmc, test_csd);
 			if (!err && ext_csd[EXT_CSD_PARTITIONING_SUPPORT] \
@@ -1114,7 +1137,7 @@ int mmc_startup(struct mmc *mmc)
 				 && memcmp(&ext_csd[EXT_CSD_SEC_CNT], \
 					&test_csd[EXT_CSD_SEC_CNT], 4) == 0) {
 
-				mmc->card_caps |= width;
+				mmc->card_caps |= ext_to_hostcaps[extw];
 				break;
 			}
 		}
-- 
1.7.9.7

[U-Boot] [RFC] mmc: Properly determine maximum supported bus width

[Jaehoon Chung]

Hi Andy,

It's working fine on my environment.

Tested-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

On 11/01/2012 02:02 PM, Andy Fleming wrote:
> At some point, a confusion arose about the use of the bit
> definitions in host_caps for bus widths, and the value
> in ext_csd. By coincidence, a simple shift could convert
> between one and the other:
> 
> MMC_MODE_1BIT = 0, EXT_CSD_BUS_WIDTH_1 = 0
> MMC_MODE_4BIT = 0x100, EXT_CSD_BUS_WIDTH_4 = 1
> MMC_MODE_8BIT = 0x200, EXT_CSD_BUS_WIDTH_8 = 2
> 
> However, as host_caps is a bitmask of supported things,
> there is not, in fact, a one-to-one correspondence. host_caps
> is capable of containing MODE_4BIT | MODE_8BIT, so nonsensical
> things were happening where we would try to set the bus width
> to 12.
> 
> The new code clarifies the very different namespaces:
> 
> host_caps/card_caps = bitmask (MMC_MODE_*)
> ext CSD fields are just an index (EXT_CSD_BUS_WIDTH_*)
> mmc->bus_width integer number of bits (1, 4, 8)
> 
> We create arrays to map between the namespaces, like in Linux.
> 
> Signed-off-by: Andy Fleming <afleming@freescale.com>
> ---
> 
> This is, I think, the approach we should use to identify the
> proper bus width for an mmc device. Please review and/or test.
> 
> 
>  drivers/mmc/mmc.c |   47 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 5fbf956..c379a74 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -867,7 +867,7 @@ void mmc_set_bus_width(struct mmc *mmc, uint width)
>  
>  int mmc_startup(struct mmc *mmc)
>  {
> -	int err, width;
> +	int err;
>  	uint mult, freq;
>  	u64 cmult, csize, capacity;
>  	struct mmc_cmd cmd;
> @@ -1086,21 +1086,44 @@ int mmc_startup(struct mmc *mmc)
>  		else
>  			mmc->tran_speed = 25000000;
>  	} else {
> -		width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
> -			 MMC_MODE_WIDTH_BITS_SHIFT);
> -		for (; width >= 0; width--) {
> -			/* Set the card to use 4 bit*/
> +		int idx;
> +
> +		/* An array of possible bus widths in order of preference */
> +		static unsigned ext_csd_bits[] = {
> +			EXT_CSD_BUS_WIDTH_8,
> +			EXT_CSD_BUS_WIDTH_4,
> +			EXT_CSD_BUS_WIDTH_1,
> +		};
> +
> +		/* An array to map CSD bus widths to host cap bits */
> +		static unsigned ext_to_hostcaps[] = {
> +			[EXT_CSD_BUS_WIDTH_4] = MMC_MODE_4BIT,
> +			[EXT_CSD_BUS_WIDTH_8] = MMC_MODE_8BIT,
> +		};
> +
> +		/* An array to map chosen bus width to an integer */
> +		static unsigned widths[] = {
> +			8, 4, 1,
> +		};
> +
> +		for (idx=0; idx < ARRAY_SIZE(ext_csd_bits); idx++) {
> +			unsigned int extw = ext_csd_bits[idx];
> +
> +			/*
> +			 * Check to make sure the controller supports
> +			 * this bus width, if it's more than 1
> +			 */
> +			if (extw != EXT_CSD_BUS_WIDTH_1 &&
> +					!(mmc->host_caps & ext_to_hostcaps[extw]))
> +				continue;
> +
>  			err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
> -					EXT_CSD_BUS_WIDTH, width);
> +					EXT_CSD_BUS_WIDTH, extw);
>  
>  			if (err)
>  				continue;
>  
> -			if (!width) {
> -				mmc_set_bus_width(mmc, 1);
> -				break;
> -			} else
> -				mmc_set_bus_width(mmc, 4 * width);
> +			mmc_set_bus_width(mmc, widths[idx]);
>  
>  			err = mmc_send_ext_csd(mmc, test_csd);
>  			if (!err && ext_csd[EXT_CSD_PARTITIONING_SUPPORT] \
> @@ -1114,7 +1137,7 @@ int mmc_startup(struct mmc *mmc)
>  				 && memcmp(&ext_csd[EXT_CSD_SEC_CNT], \
>  					&test_csd[EXT_CSD_SEC_CNT], 4) == 0) {
>  
> -				mmc->card_caps |= width;
> +				mmc->card_caps |= ext_to_hostcaps[extw];
>  				break;
>  			}
>  		}
> 

[U-Boot] [RFC] mmc: Properly determine maximum supported bus width

[Stephen Warren]

On 10/31/2012 11:02 PM, Andy Fleming wrote:
> At some point, a confusion arose about the use of the bit
> definitions in host_caps for bus widths, and the value
> in ext_csd. By coincidence, a simple shift could convert
> between one and the other:
> 
> MMC_MODE_1BIT = 0, EXT_CSD_BUS_WIDTH_1 = 0
> MMC_MODE_4BIT = 0x100, EXT_CSD_BUS_WIDTH_4 = 1
> MMC_MODE_8BIT = 0x200, EXT_CSD_BUS_WIDTH_8 = 2
> 
> However, as host_caps is a bitmask of supported things,
> there is not, in fact, a one-to-one correspondence. host_caps
> is capable of containing MODE_4BIT | MODE_8BIT, so nonsensical
> things were happening where we would try to set the bus width
> to 12.
> 
> The new code clarifies the very different namespaces:
> 
> host_caps/card_caps = bitmask (MMC_MODE_*)
> ext CSD fields are just an index (EXT_CSD_BUS_WIDTH_*)
> mmc->bus_width integer number of bits (1, 4, 8)
> 
> We create arrays to map between the namespaces, like in Linux.

(Coupled with a small change to the Tegra driver to correctly set
host_caps for 8-/4-bit, which I'll post shortly),

Tested-by: Stephen Warren <swarren@nvidia.com>

Interestingly, I just yesterday started working on a bug I'd filed
months ago to enable 8-bit eMMC support on Tegra, and found the 12-bit
issue, and came up with a very similar patch. Oh well!

[U-Boot] [RFC] mmc: Properly determine maximum supported bus width

[Stephen Warren]

On 10/31/2012 11:02 PM, Andy Fleming wrote:
> At some point, a confusion arose about the use of the bit
> definitions in host_caps for bus widths, and the value
> in ext_csd. By coincidence, a simple shift could convert
> between one and the other:
> 
> MMC_MODE_1BIT = 0, EXT_CSD_BUS_WIDTH_1 = 0
> MMC_MODE_4BIT = 0x100, EXT_CSD_BUS_WIDTH_4 = 1
> MMC_MODE_8BIT = 0x200, EXT_CSD_BUS_WIDTH_8 = 2
> 
> However, as host_caps is a bitmask of supported things,
> there is not, in fact, a one-to-one correspondence. host_caps
> is capable of containing MODE_4BIT | MODE_8BIT, so nonsensical
> things were happening where we would try to set the bus width
> to 12.
> 
> The new code clarifies the very different namespaces:
> 
> host_caps/card_caps = bitmask (MMC_MODE_*)
> ext CSD fields are just an index (EXT_CSD_BUS_WIDTH_*)
> mmc->bus_width integer number of bits (1, 4, 8)
> 
> We create arrays to map between the namespaces, like in Linux.

Andy, is this patch likely to get merged soon to u-boot.git branch
master? Unfortunately, some Tegra patches that ideally rely on this
patch being present have already been applied and merged into
u-boot-arm.git branch master.

[U-Boot] [RFC] mmc: Properly determine maximum supported bus width

[Fleming Andy-AFLEMING]

Yes, I'm processing my queue today.

On Nov 26, 2012, at 17:27, "Stephen Warren" <swarren@wwwdotorg.org> wrote:

> On 10/31/2012 11:02 PM, Andy Fleming wrote:
>> At some point, a confusion arose about the use of the bit
>> definitions in host_caps for bus widths, and the value
>> in ext_csd. By coincidence, a simple shift could convert
>> between one and the other:
>> 
>> MMC_MODE_1BIT = 0, EXT_CSD_BUS_WIDTH_1 = 0
>> MMC_MODE_4BIT = 0x100, EXT_CSD_BUS_WIDTH_4 = 1
>> MMC_MODE_8BIT = 0x200, EXT_CSD_BUS_WIDTH_8 = 2
>> 
>> However, as host_caps is a bitmask of supported things,
>> there is not, in fact, a one-to-one correspondence. host_caps
>> is capable of containing MODE_4BIT | MODE_8BIT, so nonsensical
>> things were happening where we would try to set the bus width
>> to 12.
>> 
>> The new code clarifies the very different namespaces:
>> 
>> host_caps/card_caps = bitmask (MMC_MODE_*)
>> ext CSD fields are just an index (EXT_CSD_BUS_WIDTH_*)
>> mmc->bus_width integer number of bits (1, 4, 8)
>> 
>> We create arrays to map between the namespaces, like in Linux.
> 
> Andy, is this patch likely to get merged soon to u-boot.git branch
> master? Unfortunately, some Tegra patches that ideally rely on this
> patch being present have already been applied and merged into
> u-boot-arm.git branch master.
> 

[U-Boot] [RFC] mmc: Properly determine maximum supported bus width

[Stephen Warren]

On 11/26/2012 04:33 PM, Fleming Andy-AFLEMING wrote:
> Yes, I'm processing my queue today.

This patch didn't show up in the pull request you just sent.

> On Nov 26, 2012, at 17:27, "Stephen Warren" <swarren@wwwdotorg.org> wrote:
> 
>> On 10/31/2012 11:02 PM, Andy Fleming wrote:
>>> At some point, a confusion arose about the use of the bit
>>> definitions in host_caps for bus widths, and the value
>>> in ext_csd. By coincidence, a simple shift could convert
>>> between one and the other:
>>>
>>> MMC_MODE_1BIT = 0, EXT_CSD_BUS_WIDTH_1 = 0
>>> MMC_MODE_4BIT = 0x100, EXT_CSD_BUS_WIDTH_4 = 1
>>> MMC_MODE_8BIT = 0x200, EXT_CSD_BUS_WIDTH_8 = 2
>>>
>>> However, as host_caps is a bitmask of supported things,
>>> there is not, in fact, a one-to-one correspondence. host_caps
>>> is capable of containing MODE_4BIT | MODE_8BIT, so nonsensical
>>> things were happening where we would try to set the bus width
>>> to 12.
>>>
>>> The new code clarifies the very different namespaces:
>>>
>>> host_caps/card_caps = bitmask (MMC_MODE_*)
>>> ext CSD fields are just an index (EXT_CSD_BUS_WIDTH_*)
>>> mmc->bus_width integer number of bits (1, 4, 8)
>>>
>>> We create arrays to map between the namespaces, like in Linux.
>>
>> Andy, is this patch likely to get merged soon to u-boot.git branch
>> master? Unfortunately, some Tegra patches that ideally rely on this
>> patch being present have already been applied and merged into
>> u-boot-arm.git branch master.
>>
> 

[U-Boot] [RFC] mmc: Properly determine maximum supported bus width

[Andy Fleming]

On Tue, Nov 27, 2012 at 5:40 PM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> On 11/26/2012 04:33 PM, Fleming Andy-AFLEMING wrote:
> > Yes, I'm processing my queue today.
>
> This patch didn't show up in the pull request you just sent.
>


Yeah, sorry, I don't know how that happened. I'm applying it now.

Andy