From: Aries Lee <[email protected]>
Some old MMC devices fail with the 4/8 bits the driver tries to use
exclusively. This patch adds a test for the given bus setup and falls
back to the lower bit mode (until 1-bit mode) when the test fails.
[Major rework and refactoring by tiwai]
[Quirk addition and many fixes by prakity]
v1->v2:
- Rebased to the code with DDR support, set DDR bit properly
- Return always error when bus-switching fallback failed
- Define MMC_BUS_TEST_{R|W} in linux/mmc/mmc.h
- Add quirk MMC_CAP_BUS_WIDTH_TEST -- default not used for compatibility
- Ignore errors on BUS_TEST_W -- improves chances test will work
Signed-off-by: Aries Lee <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
Signed-off-by: Philip Rakity <[email protected]>
Tested-by: Philip Rakity <[email protected]>
---
Chris, this is a revised version of the patch. Philip merged his patch
into this version and tested. Please apply.
drivers/mmc/core/mmc.c | 76 ++++++++++++++++++++------------
drivers/mmc/core/mmc_ops.c | 102 ++++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/core/mmc_ops.h | 1 +
drivers/mmc/host/sdhci.c | 7 +++-
drivers/mmc/host/sdhci.h | 1 +
include/linux/mmc/host.h | 1 +
include/linux/mmc/mmc.h | 2 +
7 files changed, 160 insertions(+), 30 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 77f93c3..3d51949 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -534,39 +534,57 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
*/
if ((card->csd.mmca_vsn >= CSD_SPEC_VER_4) &&
(host->caps & (MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA))) {
- unsigned ext_csd_bit, bus_width;
-
- if (host->caps & MMC_CAP_8_BIT_DATA) {
- if (ddr)
- ext_csd_bit = EXT_CSD_DDR_BUS_WIDTH_8;
- else
- ext_csd_bit = EXT_CSD_BUS_WIDTH_8;
- bus_width = MMC_BUS_WIDTH_8;
- } else {
- if (ddr)
- ext_csd_bit = EXT_CSD_DDR_BUS_WIDTH_4;
- else
- ext_csd_bit = EXT_CSD_BUS_WIDTH_4;
- bus_width = MMC_BUS_WIDTH_4;
+ static unsigned ext_csd_bits[][2] = {
+ { EXT_CSD_BUS_WIDTH_8, EXT_CSD_DDR_BUS_WIDTH_8 },
+ { EXT_CSD_BUS_WIDTH_4, EXT_CSD_DDR_BUS_WIDTH_4 },
+ { EXT_CSD_BUS_WIDTH_1, EXT_CSD_BUS_WIDTH_1 },
+ };
+ static unsigned bus_widths[] = {
+ MMC_BUS_WIDTH_8,
+ MMC_BUS_WIDTH_4,
+ MMC_BUS_WIDTH_1
+ };
+ unsigned idx, bus_width;
+
+ if (host->caps & MMC_CAP_8_BIT_DATA)
+ idx = 0;
+ else
+ idx = 1;
+ for (; idx < ARRAY_SIZE(bus_widths); idx++) {
+ bus_width = bus_widths[idx];
+ if (bus_width == MMC_BUS_WIDTH_1)
+ ddr = 0; /* no DDR for 1-bit width */
+ err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+ EXT_CSD_BUS_WIDTH,
+ ext_csd_bits[idx][0]);
+ if (!err) {
+ /*
+ * if controller can't handle bus width test
+ * use the highest bus width to
+ * maintain compatibility with previous linux
+ */
+ if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
+ break;
+ mmc_set_bus_width_ddr(card->host,
+ bus_width, MMC_SDR_MODE);
+ err = mmc_bus_test(card, bus_width);
+ if (!err)
+ break;
+ }
}
- err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
- EXT_CSD_BUS_WIDTH, ext_csd_bit);
-
- if (err && err != -EBADMSG)
- goto free_card;
-
+ if (!err && ddr) {
+ err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+ EXT_CSD_BUS_WIDTH,
+ ext_csd_bits[idx][1]);
+ }
if (err) {
printk(KERN_WARNING "%s: switch to bus width %d ddr %d "
- "failed\n", mmc_hostname(card->host),
- 1 << bus_width, ddr);
- err = 0;
- } else {
- if (ddr)
- mmc_card_set_ddr_mode(card);
- else
- ddr = MMC_SDR_MODE;
-
+ "failed\n", mmc_hostname(card->host),
+ 1 << bus_width, ddr);
+ goto free_card;
+ } else if (ddr) {
+ mmc_card_set_ddr_mode(card);
mmc_set_bus_width_ddr(card->host, bus_width, ddr);
}
}
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 326447c..f1bc7d8 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -462,3 +462,105 @@ int mmc_send_status(struct mmc_card *card, u32 *status)
return 0;
}
+static int
+mmc_send_bus_test(struct mmc_card *card, struct mmc_host *host, u8 opcode,
+ u8 len)
+{
+ struct mmc_request mrq;
+ struct mmc_command cmd;
+ struct mmc_data data;
+ struct scatterlist sg;
+ u8 *data_buf;
+ u8 *test_buf;
+ int i, err;
+ static u8 testdata_8bit[8] = { 0x55, 0xaa, 0, 0, 0, 0, 0, 0 };
+ static u8 testdata_4bit[4] = { 0x5a, 0, 0, 0 };
+
+ /* dma onto stack is unsafe/nonportable, but callers to this
+ * routine normally provide temporary on-stack buffers ...
+ */
+ data_buf = kmalloc(len, GFP_KERNEL);
+ if (!data_buf)
+ return -ENOMEM;
+
+ if (len == 8)
+ test_buf = testdata_8bit;
+ else if (len == 4)
+ test_buf = testdata_4bit;
+ else {
+ printk(KERN_ERR "%s: Invaild bus_width %d\n",
+ mmc_hostname(host), len);
+ kfree(data_buf);
+ return -EINVAL;
+ }
+
+ if (opcode == MMC_BUS_TEST_W)
+ memcpy(data_buf, test_buf, len);
+
+ memset(&mrq, 0, sizeof(struct mmc_request));
+ memset(&cmd, 0, sizeof(struct mmc_command));
+ memset(&data, 0, sizeof(struct mmc_data));
+
+ mrq.cmd = &cmd;
+ mrq.data = &data;
+ cmd.opcode = opcode;
+ cmd.arg = 0;
+
+ /* NOTE HACK: the MMC_RSP_SPI_R1 is always correct here, but we
+ * rely on callers to never use this with "native" calls for reading
+ * CSD or CID. Native versions of those commands use the R2 type,
+ * not R1 plus a data block.
+ */
+ cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+
+ data.blksz = len;
+ data.blocks = 1;
+ if (opcode == MMC_BUS_TEST_R)
+ data.flags = MMC_DATA_READ;
+ else
+ data.flags = MMC_DATA_WRITE;
+
+ data.sg = &sg;
+ data.sg_len = 1;
+ sg_init_one(&sg, data_buf, len);
+ mmc_wait_for_req(host, &mrq);
+ err = 0;
+ if (opcode == MMC_BUS_TEST_R) {
+ for (i = 0; i < len / 4; i++)
+ if ((test_buf[i] ^ data_buf[i]) != 0xff) {
+ err = -EIO;
+ break;
+ }
+ }
+ kfree(data_buf);
+
+ if (cmd.error)
+ return cmd.error;
+ if (data.error)
+ return data.error;
+
+ return err;
+}
+
+int mmc_bus_test(struct mmc_card *card, u8 bus_width)
+{
+ int err, width;
+
+ if (bus_width == MMC_BUS_WIDTH_8)
+ width = 8;
+ else if (bus_width == MMC_BUS_WIDTH_4)
+ width = 4;
+ else if (bus_width == MMC_BUS_WIDTH_1)
+ return 0; /* no need for test */
+ else
+ return -EINVAL;
+
+ /*
+ * ignore errors from BUS_TEST_W. BUS_TEST_R will fail
+ * if there is a problem. Improves chances test will work
+ */
+ mmc_send_bus_test(card, card->host, MMC_BUS_TEST_W, width);
+ err = mmc_send_bus_test(card, card->host, MMC_BUS_TEST_R, width);
+ return err;
+}
+
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 653eb8e..e6d44b8 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -26,6 +26,7 @@ int mmc_send_cid(struct mmc_host *host, u32 *cid);
int mmc_spi_read_ocr(struct mmc_host *host, int highcap, u32 *ocrp);
int mmc_spi_set_crc(struct mmc_host *host, int use_crc);
int mmc_card_sleepawake(struct mmc_host *host, int sleep);
+int mmc_bus_test(struct mmc_card *card, u8 bus_width);
#endif
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a25db42..d2673b7 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -23,6 +23,7 @@
#include <linux/leds.h>
+#include <linux/mmc/mmc.h>
#include <linux/mmc/host.h>
#include "sdhci.h"
@@ -1518,7 +1519,11 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
if (intmask & SDHCI_INT_DATA_TIMEOUT)
host->data->error = -ETIMEDOUT;
- else if (intmask & (SDHCI_INT_DATA_CRC | SDHCI_INT_DATA_END_BIT))
+ else if (intmask & SDHCI_INT_DATA_END_BIT)
+ host->data->error = -EILSEQ;
+ else if ((intmask & SDHCI_INT_DATA_CRC) &&
+ SDHCI_GET_CMD(sdhci_readw(host, SDHCI_COMMAND))
+ != MMC_BUS_TEST_R)
host->data->error = -EILSEQ;
else if (intmask & SDHCI_INT_ADMA_ERROR) {
printk(KERN_ERR "%s: ADMA error\n", mmc_hostname(host->mmc));
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index e42d7f0..5be9060 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -52,6 +52,7 @@
#define SDHCI_CMD_RESP_SHORT_BUSY 0x03
#define SDHCI_MAKE_CMD(c, f) (((c & 0xff) << 8) | (f & 0xff))
+#define SDHCI_GET_CMD(c) ((c>>8) & 0x3f)
#define SDHCI_RESPONSE 0x10
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 30f6fad..0fc7d46 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -169,6 +169,7 @@ struct mmc_host {
#define MMC_CAP_1_2V_DDR (1 << 12) /* can support */
/* DDR mode at 1.2V */
#define MMC_CAP_POWER_OFF_CARD (1 << 13) /* Can power off after boot */
+#define MMC_CAP_BUS_WIDTH_TEST (1 << 14) /* CMD14/CMD19 bus width ok */
mmc_pm_flag_t pm_caps; /* supported pm features */
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 956fbd87..612301f 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -40,7 +40,9 @@
#define MMC_READ_DAT_UNTIL_STOP 11 /* adtc [31:0] dadr R1 */
#define MMC_STOP_TRANSMISSION 12 /* ac R1b */
#define MMC_SEND_STATUS 13 /* ac [31:16] RCA R1 */
+#define MMC_BUS_TEST_R 14 /* adtc R1 */
#define MMC_GO_INACTIVE_STATE 15 /* ac [31:16] RCA */
+#define MMC_BUS_TEST_W 19 /* adtc R1 */
#define MMC_SPI_READ_OCR 58 /* spi spi_R3 */
#define MMC_SPI_CRC_ON_OFF 59 /* spi [0:0] flag spi_R1 */
--
1.7.3.1
Hi Takashi,
On Wed, Dec 15, 2010 at 08:14:24AM +0100, Takashi Iwai wrote:
> From: Aries Lee <[email protected]>
>
> Some old MMC devices fail with the 4/8 bits the driver tries to use
> exclusively. This patch adds a test for the given bus setup and falls
> back to the lower bit mode (until 1-bit mode) when the test fails.
>
> [Major rework and refactoring by tiwai]
> [Quirk addition and many fixes by prakity]
>
> v1->v2:
> - Rebased to the code with DDR support, set DDR bit properly
> - Return always error when bus-switching fallback failed
> - Define MMC_BUS_TEST_{R|W} in linux/mmc/mmc.h
> - Add quirk MMC_CAP_BUS_WIDTH_TEST -- default not used for compatibility
> - Ignore errors on BUS_TEST_W -- improves chances test will work
>
> Signed-off-by: Aries Lee <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> Signed-off-by: Philip Rakity <[email protected]>
> Tested-by: Philip Rakity <[email protected]>
This looks good, but adds a warning:
drivers/mmc/core/mmc.c: In function ‘mmc_init_card’:
drivers/mmc/core/mmc.c:547: warning: ‘bus_width’ may be used uninitialized in this function
Thanks,
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
Chris,
It is not possible for bus_width to be not initialized. This would imply ARRAY_SIZE(bus_widths) is 1. Certainly not true.
We could just initialize by changing
+ unsigned idx, bus_width;
to
+ unsigned idx, bus_width = 0;
I wonder what compiler are you using so we can avoid this issue in future.
Philip
+ static unsigned ext_csd_bits[][2] = {
+ { EXT_CSD_BUS_WIDTH_8, EXT_CSD_DDR_BUS_WIDTH_8 },
+ { EXT_CSD_BUS_WIDTH_4, EXT_CSD_DDR_BUS_WIDTH_4 },
+ { EXT_CSD_BUS_WIDTH_1, EXT_CSD_BUS_WIDTH_1 },
+ };
+ static unsigned bus_widths[] = {
+ MMC_BUS_WIDTH_8,
+ MMC_BUS_WIDTH_4,
+ MMC_BUS_WIDTH_1
+ };
+ unsigned idx, bus_width;
+
+ if (host->caps & MMC_CAP_8_BIT_DATA)
+ idx = 0;
+ else
+ idx = 1;
+ for (; idx < ARRAY_SIZE(bus_widths); idx++) {
+ bus_width = bus_widths[idx];
+ if (bus_width == MMC_BUS_WIDTH_1)
+ ddr = 0; /* no DDR for 1-bit width */
On Dec 16, 2010, at 3:40 PM, Chris Ball wrote:
> Hi Takashi,
>
> On Wed, Dec 15, 2010 at 08:14:24AM +0100, Takashi Iwai wrote:
>> From: Aries Lee <[email protected]>
>>
>> Some old MMC devices fail with the 4/8 bits the driver tries to use
>> exclusively. This patch adds a test for the given bus setup and falls
>> back to the lower bit mode (until 1-bit mode) when the test fails.
>>
>> [Major rework and refactoring by tiwai]
>> [Quirk addition and many fixes by prakity]
>>
>> v1->v2:
>> - Rebased to the code with DDR support, set DDR bit properly
>> - Return always error when bus-switching fallback failed
>> - Define MMC_BUS_TEST_{R|W} in linux/mmc/mmc.h
>> - Add quirk MMC_CAP_BUS_WIDTH_TEST -- default not used for compatibility
>> - Ignore errors on BUS_TEST_W -- improves chances test will work
>>
>> Signed-off-by: Aries Lee <[email protected]>
>> Signed-off-by: Takashi Iwai <[email protected]>
>> Signed-off-by: Philip Rakity <[email protected]>
>> Tested-by: Philip Rakity <[email protected]>
>
> This looks good, but adds a warning:
>
> drivers/mmc/core/mmc.c: In function ?mmc_init_card?:
> drivers/mmc/core/mmc.c:547: warning: ?bus_width? may be used uninitialized in this function
>
> Thanks,
>
> --
> Chris Ball <[email protected]> <http://printf.net/>
> One Laptop Per Child
Hi Philip,
On Thu, Dec 16, 2010 at 06:33:49PM -0800, Philip Rakity wrote:
> It is not possible for bus_width to be not initialized. This would imply ARRAY_SIZE(bus_widths) is 1. Certainly not true.
Right, I agree. We should fix the warning anyway.
> We could just initialize by changing
> + unsigned idx, bus_width;
> to
> + unsigned idx, bus_width = 0;
Okay, I've pushed to mmc-next with that change.
> I wonder what compiler are you using so we can avoid this issue in future.
Ah, good point -- I was building with a gcc 4.1.2 ARM cross-compiler,
and using a gcc 4.5.1 cross-build instead avoids the warning.
Thanks,
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
At Fri, 17 Dec 2010 03:43:42 +0000,
Chris Ball wrote:
>
> Hi Philip,
>
> On Thu, Dec 16, 2010 at 06:33:49PM -0800, Philip Rakity wrote:
> > It is not possible for bus_width to be not initialized. This would imply ARRAY_SIZE(bus_widths) is 1. Certainly not true.
>
> Right, I agree. We should fix the warning anyway.
Well, this is always a gray-zone question. One prefers fixing it
either via uninitialized_var() or by setting a bogus value. But, this
would cover any possible real bug in future. Thus another prefers
ignoring it, because it's just a compiler bug (mostly of old gcc).
After all, it's up to maintainer, so take as you like :)
thanks,
Takashi
> > We could just initialize by changing
> > + unsigned idx, bus_width;
> > to
> > + unsigned idx, bus_width = 0;
>
> Okay, I've pushed to mmc-next with that change.
>
> > I wonder what compiler are you using so we can avoid this issue in future.
>
> Ah, good point -- I was building with a gcc 4.1.2 ARM cross-compiler,
> and using a gcc 4.5.1 cross-build instead avoids the warning.
>
> Thanks,
>
> --
> Chris Ball <[email protected]> <http://printf.net/>
> One Laptop Per Child
>
On Fri, Dec 17, 2010 at 3:11 AM, Takashi Iwai <[email protected]> wrote:
> At Fri, 17 Dec 2010 03:43:42 +0000,
> Chris Ball wrote:
>>
>> Hi Philip,
>>
>> On Thu, Dec 16, 2010 at 06:33:49PM -0800, Philip Rakity wrote:
>> > It is not possible for bus_width to be not initialized. ?This would imply ARRAY_SIZE(bus_widths) is 1. ?Certainly not true.
>>
>> Right, I agree. ?We should fix the warning anyway.
>
> Well, this is always a gray-zone question. ?One prefers fixing it
> either via uninitialized_var() or by setting a bogus value. ?But, this
> would cover any possible real bug in future. ?Thus another prefers
> ignoring it, because it's just a compiler bug (mostly of old gcc).
>
> After all, it's up to maintainer, so take as you like :)
>
>
> thanks,
>
> Takashi
>
>
>> > We could just initialize by changing
>> > + ? ? ? ? ? unsigned idx, bus_width;
>> > to
>> > + ? ? ? ? ? unsigned idx, bus_width = 0;
>>
>> Okay, I've pushed to mmc-next with that change.
>>
>> > I wonder what compiler are you using so we can avoid this issue in future.
>>
>> Ah, good point -- I was building with a gcc 4.1.2 ARM cross-compiler,
>> and using a gcc 4.5.1 cross-build instead avoids the warning.
>>
>> Thanks,
>>
>> --
>> Chris Ball ? <[email protected]> ? <http://printf.net/>
>> One Laptop Per Child
>>
>
Could you help adding this modification?
We found error happen since bus_width is not set at these condition:
1. ddr=0
2. not set MMC_CAP_BUS_WIDTH_TEST
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 1d8409f..86cac0d 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -586,7 +586,11 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
} else if (ddr) {
mmc_card_set_ddr_mode(card);
mmc_set_bus_width_ddr(card->host, bus_width, ddr);
- }
+
+ } else
+ mmc_set_bus_width(card->host,
+ bus_widths[idx]);
+
}
if (!oldcard)
Can you please try this diff and see if it works for you.
Will do formal patch after your testing. What card is failing ?
Please let me know the manufacturing information so can add card to my test suite.
Philip
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 1d8409f..77072c8 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -558,6 +558,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
EXT_CSD_BUS_WIDTH,
ext_csd_bits[idx][0]);
if (!err) {
+ mmc_set_bus_width_ddr(card->host,
+ bus_width, MMC_SDR_MODE);
/*
* If controller can't handle bus width test,
* use the highest bus width to maintain
@@ -565,8 +567,6 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
*/
if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
break;
- mmc_set_bus_width_ddr(card->host,
- bus_width, MMC_SDR_MODE);
err = mmc_bus_test(card, bus_width);
if (!err)
break;
@@ -586,7 +586,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
} else if (ddr) {
mmc_card_set_ddr_mode(card);
mmc_set_bus_width_ddr(card->host, bus_width, ddr);
- }
+ } else
+ mmc_set_bus_width (card->host, bus_width);
}
if (!oldcard)
Philip
On Dec 21, 2010, at 2:59 AM, zhangfei gao wrote:
> On Fri, Dec 17, 2010 at 3:11 AM, Takashi Iwai <[email protected]> wrote:
>> At Fri, 17 Dec 2010 03:43:42 +0000,
>> Chris Ball wrote:
>>>
>>> Hi Philip,
>>>
>>> On Thu, Dec 16, 2010 at 06:33:49PM -0800, Philip Rakity wrote:
>>>> It is not possible for bus_width to be not initialized. This would imply ARRAY_SIZE(bus_widths) is 1. Certainly not true.
>>>
>>> Right, I agree. We should fix the warning anyway.
>>
>> Well, this is always a gray-zone question. One prefers fixing it
>> either via uninitialized_var() or by setting a bogus value. But, this
>> would cover any possible real bug in future. Thus another prefers
>> ignoring it, because it's just a compiler bug (mostly of old gcc).
>>
>> After all, it's up to maintainer, so take as you like :)
>>
>>
>> thanks,
>>
>> Takashi
>>
>>
>>>> We could just initialize by changing
>>>> + unsigned idx, bus_width;
>>>> to
>>>> + unsigned idx, bus_width = 0;
>>>
>>> Okay, I've pushed to mmc-next with that change.
>>>
>>>> I wonder what compiler are you using so we can avoid this issue in future.
>>>
>>> Ah, good point -- I was building with a gcc 4.1.2 ARM cross-compiler,
>>> and using a gcc 4.5.1 cross-build instead avoids the warning.
>>>
>>> Thanks,
>>>
>>> --
>>> Chris Ball <[email protected]> <http://printf.net/>
>>> One Laptop Per Child
>>>
>>
>
> Could you help adding this modification?
> We found error happen since bus_width is not set at these condition:
> 1. ddr=0
> 2. not set MMC_CAP_BUS_WIDTH_TEST
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 1d8409f..86cac0d 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -586,7 +586,11 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> } else if (ddr) {
> mmc_card_set_ddr_mode(card);
> mmc_set_bus_width_ddr(card->host, bus_width, ddr);
> - }
> +
> + } else
> + mmc_set_bus_width(card->host,
> + bus_widths[idx]);
> +
> }
>
> if (!oldcard)
Zhangfei,
Reproduced the problem on mmp2. Fix enclosed. Slight change from Zhangfei
original suggestion.
Please confirm by adding tested-by.
Philip
>From 8b35e007d67dd593ff5e8a07b564c438e8303aae Mon Sep 17 00:00:00 2001
From: Philip Rakity <[email protected]>
Date: Tue, 21 Dec 2010 11:29:33 -0800
Subject: [PATCH] mmc: fix regression testing bus width when CAP not defined
First reported by Zhangfei Gao.
set mmc bus width before checking for CAP for mmc bus width testing.
set bus width when card is not ddr
Signed-off-by: Philip Rakity <[email protected]>
Tested-by: Philip Rakity
---
drivers/mmc/core/mmc.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 1d8409f..3a2905f 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -558,6 +558,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
EXT_CSD_BUS_WIDTH,
ext_csd_bits[idx][0]);
if (!err) {
+ mmc_set_bus_width_ddr(card->host,
+ bus_width, MMC_SDR_MODE);
/*
* If controller can't handle bus width test,
* use the highest bus width to maintain
@@ -565,8 +567,6 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
*/
if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
break;
- mmc_set_bus_width_ddr(card->host,
- bus_width, MMC_SDR_MODE);
err = mmc_bus_test(card, bus_width);
if (!err)
break;
@@ -586,7 +586,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
} else if (ddr) {
mmc_card_set_ddr_mode(card);
mmc_set_bus_width_ddr(card->host, bus_width, ddr);
- }
+ } else
+ mmc_set_bus_width (card->host, bus_width);
}
if (!oldcard)
--
1.6.0.4
On Tue, Dec 21, 2010 at 11:36 AM, Philip Rakity <[email protected]> wrote:
>
>
> Can you please try this diff and see if it works for you.
>
> Will do formal patch after your testing. ?What card is failing ?
>
> Please let me know the manufacturing information so can add card to my test suite.
>
> Philip
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 1d8409f..77072c8 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -558,6 +558,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? EXT_CSD_BUS_WIDTH,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ext_csd_bits[idx][0]);
> ? ? ? ? ? ? ? ? ? ? ? ?if (!err) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_set_bus_width_ddr(card->host,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bus_width, MMC_SDR_MODE);
Test OK,
Curious why move here, then mmc_set_bus_width_ddr is called twice in
fact when ddr=0 && (!(host->caps & MMC_CAP_BUS_WIDTH_TEST)), though
not impact function.
mmc_set_bus_width is mmc_set_bus_width_ddr(host, width, MMC_SDR_MODE).
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? * If controller can't handle bus width test,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? * use the highest bus width to maintain
> @@ -565,8 +567,6 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? */
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_set_bus_width_ddr(card->host,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bus_width, MMC_SDR_MODE);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?err = mmc_bus_test(card, bus_width);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?if (!err)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
> @@ -586,7 +586,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> ? ? ? ? ? ? ? ?} else if (ddr) {
> ? ? ? ? ? ? ? ? ? ? ? ?mmc_card_set_ddr_mode(card);
> ? ? ? ? ? ? ? ? ? ? ? ?mmc_set_bus_width_ddr(card->host, bus_width, ddr);
> - ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? } else
> + ? ? ? ? ? ? ? ? ? ? ? mmc_set_bus_width (card->host, bus_width);
> ? ? ? ?}
>
> ? ? ? ?if (!oldcard)
>
>
> Philip
>
> On Dec 21, 2010, at 2:59 AM, zhangfei gao wrote:
>
>> On Fri, Dec 17, 2010 at 3:11 AM, Takashi Iwai <[email protected]> wrote:
>>> At Fri, 17 Dec 2010 03:43:42 +0000,
>>> Chris Ball wrote:
>>>>
>>>> Hi Philip,
>>>>
>>>> On Thu, Dec 16, 2010 at 06:33:49PM -0800, Philip Rakity wrote:
>>>>> It is not possible for bus_width to be not initialized. ?This would imply ARRAY_SIZE(bus_widths) is 1. ?Certainly not true.
>>>>
>>>> Right, I agree. ?We should fix the warning anyway.
>>>
>>> Well, this is always a gray-zone question. ?One prefers fixing it
>>> either via uninitialized_var() or by setting a bogus value. ?But, this
>>> would cover any possible real bug in future. ?Thus another prefers
>>> ignoring it, because it's just a compiler bug (mostly of old gcc).
>>>
>>> After all, it's up to maintainer, so take as you like :)
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>>
>>>>> We could just initialize by changing
>>>>> + ? ? ? ? ? unsigned idx, bus_width;
>>>>> to
>>>>> + ? ? ? ? ? unsigned idx, bus_width = 0;
>>>>
>>>> Okay, I've pushed to mmc-next with that change.
>>>>
>>>>> I wonder what compiler are you using so we can avoid this issue in future.
>>>>
>>>> Ah, good point -- I was building with a gcc 4.1.2 ARM cross-compiler,
>>>> and using a gcc 4.5.1 cross-build instead avoids the warning.
>>>>
>>>> Thanks,
>>>>
>>>> --
>>>> Chris Ball ? <[email protected]> ? <http://printf.net/>
>>>> One Laptop Per Child
>>>>
>>>
>>
>> Could you help adding this modification?
>> We found error happen since bus_width is not set at these condition:
>> 1. ddr=0
>> 2. not set MMC_CAP_BUS_WIDTH_TEST
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 1d8409f..86cac0d 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -586,7 +586,11 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>> ? ? ? ? ? ? ? } else if (ddr) {
>> ? ? ? ? ? ? ? ? ? ? ? mmc_card_set_ddr_mode(card);
>> ? ? ? ? ? ? ? ? ? ? ? mmc_set_bus_width_ddr(card->host, bus_width, ddr);
>> - ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? } else
>> + ? ? ? ? ? ? ? ? ? ? mmc_set_bus_width(card->host,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bus_widths[idx]);
>> +
>> ? ? ? }
>>
>> ? ? ? if (!oldcard)
>
>
At Tue, 21 Dec 2010 22:56:32 -0500,
zhangfei gao wrote:
>
> On Tue, Dec 21, 2010 at 11:36 AM, Philip Rakity <[email protected]> wrote:
> >
> >
> > Can you please try this diff and see if it works for you.
> >
> > Will do formal patch after your testing. What card is failing ?
> >
> > Please let me know the manufacturing information so can add card to my test suite.
> >
> > Philip
> >
> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > index 1d8409f..77072c8 100644
> > --- a/drivers/mmc/core/mmc.c
> > +++ b/drivers/mmc/core/mmc.c
> > @@ -558,6 +558,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> > EXT_CSD_BUS_WIDTH,
> > ext_csd_bits[idx][0]);
> > if (!err) {
> > + mmc_set_bus_width_ddr(card->host,
> > + bus_width, MMC_SDR_MODE);
> Test OK,
> Curious why move here, then mmc_set_bus_width_ddr is called twice in
> fact when ddr=0 && (!(host->caps & MMC_CAP_BUS_WIDTH_TEST)), though
> not impact function.
> mmc_set_bus_width is mmc_set_bus_width_ddr(host, width, MMC_SDR_MODE).
Right. How about the patch below? This one just moves the call
before caps test, so it's simpler (and avoiding double calls).
thanks,
Takashi
===
>From b66b9704f8d2fefa402741fb17949224b2766b4f Mon Sep 17 00:00:00 2001
From: Takashi Iwai <[email protected]>
Date: Wed, 22 Dec 2010 09:54:33 +0100
Subject: [PATCH] mmc: fix mmc_set_bus_width_ddr() call without bus-width-test cap
With the bus-width test patch, mmc_set_bus_width*() isn't called properly
when the driver doesn't set MMC_CAP_BUS_WIDTH and no DDR mode.
This patch fixes the regression by moving the call up before the cap test.
Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/mmc/core/mmc.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 1d8409f..c86dd73 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -558,6 +558,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
EXT_CSD_BUS_WIDTH,
ext_csd_bits[idx][0]);
if (!err) {
+ mmc_set_bus_width_ddr(card->host,
+ bus_width, MMC_SDR_MODE);
/*
* If controller can't handle bus width test,
* use the highest bus width to maintain
@@ -565,8 +567,6 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
*/
if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
break;
- mmc_set_bus_width_ddr(card->host,
- bus_width, MMC_SDR_MODE);
err = mmc_bus_test(card, bus_width);
if (!err)
break;
--
1.7.3.4
On Wed, Dec 22, 2010 at 3:59 AM, Takashi Iwai <[email protected]> wrote:
> At Tue, 21 Dec 2010 22:56:32 -0500,
> zhangfei gao wrote:
>>
>> On Tue, Dec 21, 2010 at 11:36 AM, Philip Rakity <[email protected]> wrote:
>> >
>> >
>> > Can you please try this diff and see if it works for you.
>> >
>> > Will do formal patch after your testing. ?What card is failing ?
>> >
>> > Please let me know the manufacturing information so can add card to my test suite.
>> >
>> > Philip
>> >
>> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> > index 1d8409f..77072c8 100644
>> > --- a/drivers/mmc/core/mmc.c
>> > +++ b/drivers/mmc/core/mmc.c
>> > @@ -558,6 +558,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? EXT_CSD_BUS_WIDTH,
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ext_csd_bits[idx][0]);
>> > ? ? ? ? ? ? ? ? ? ? ? ?if (!err) {
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_set_bus_width_ddr(card->host,
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bus_width, MMC_SDR_MODE);
>> Test OK,
>> Curious why move here, then mmc_set_bus_width_ddr is called twice in
>> fact when ddr=0 && ?(!(host->caps & MMC_CAP_BUS_WIDTH_TEST)), though
>> not impact function.
>> mmc_set_bus_width is mmc_set_bus_width_ddr(host, width, MMC_SDR_MODE).
>
> Right. ?How about the patch below? ?This one just moves the call
> before caps test, so it's simpler (and avoiding double calls).
>
>
> thanks,
>
> Takashi
>
> ===
> From b66b9704f8d2fefa402741fb17949224b2766b4f Mon Sep 17 00:00:00 2001
> From: Takashi Iwai <[email protected]>
> Date: Wed, 22 Dec 2010 09:54:33 +0100
> Subject: [PATCH] mmc: fix mmc_set_bus_width_ddr() call without bus-width-test cap
>
> With the bus-width test patch, mmc_set_bus_width*() isn't called properly
> when the driver doesn't set MMC_CAP_BUS_WIDTH and no DDR mode.
> This patch fixes the regression by moving the call up before the cap test.
>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> ?drivers/mmc/core/mmc.c | ? ?4 ++--
> ?1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 1d8409f..c86dd73 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -558,6 +558,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? EXT_CSD_BUS_WIDTH,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ext_csd_bits[idx][0]);
> ? ? ? ? ? ? ? ? ? ? ? ?if (!err) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_set_bus_width_ddr(card->host,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bus_width, MMC_SDR_MODE);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? * If controller can't handle bus width test,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? * use the highest bus width to maintain
> @@ -565,8 +567,6 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? */
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_set_bus_width_ddr(card->host,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bus_width, MMC_SDR_MODE);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?err = mmc_bus_test(card, bus_width);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?if (!err)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
> --
> 1.7.3.4
It's also works, solve (ddr=0 & (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))) issue.
Is it possible to merge to original patch.
Thanks
>
>
>