| Message ID | 1351746158-19009-1-git-send-email-afleming@freescale.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Andy Fleming |
| Headers | show |
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; > } > } >
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!
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.
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. >
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. >> >
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
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; } }
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(-)