| Message ID | 1302556424-21951-2-git-send-email-andreiw@motorola.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: linux-mmc-owner@vger.kernel.org > [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Andrei Warkentin > Sent: Tuesday, April 12, 2011 5:14 AM > To: linux-mmc@vger.kernel.org > Cc: arnd@arndb.de; cjb@laptop.org; Andrei Warkentin > Subject: [patchv3 2/4] MMC: SDHCI R1B command handling + MMC_CAP_ERASE. > > ERASE command needs R1B response, so fix R1B-type command > handling for SDHCI controller. For non-DAT commands using a busy > reponse, the cmd->cmd_timeout_ms (in ms) field is used for timeout > calculations. > > Based on patch by Chuanxiao Dong <chuanxiao.dong@intel.com> > Signed-off-by: Andrei Warkentin <andreiw@motorola.com> > --- > drivers/mmc/host/sdhci.c | 43 +++++++++++++++++++++++++++---------------- > 1 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 9e15f41..173e980 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -40,7 +40,6 @@ > > static unsigned int debug_quirks = 0; > > -static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *); > static void sdhci_finish_data(struct sdhci_host *); > > static void sdhci_send_command(struct sdhci_host *, struct mmc_command *); > @@ -591,9 +590,10 @@ static void sdhci_adma_table_post(struct sdhci_host > *host, > data->sg_len, direction); > } > > -static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data) > +static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command > *cmd) > { > u8 count; > + struct mmc_data *data = cmd->data; > unsigned target_timeout, current_timeout; > > /* > @@ -605,12 +605,16 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, > struct mmc_data *data) > if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL) > return 0xE; > > - /* timeout in us */ > - target_timeout = data->timeout_ns / 1000 + > - data->timeout_clks / host->clock; > + /* Unspecified timeout, assume max */ > + if (!data && !cmd->cmd_timeout_ms) > + return 0xE; Just a inline question here. I noticed cmd_timeout_ms only be set for the ERASE command and SWITCH command, for other types of commands, this value is left not initialized. Cmd_timeout_ms may not be zero and also not be initialized. And next, driver will use this value to calculate the timeout. So I think using an uninitialized value here doesn't make sense. If we want to use it, we have to make sure at this point, this value is already initialized. > > - if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) > - host->timeout_clk = host->clock / 1000; > + /* timeout in us */ > + if (!data) > + target_timeout = cmd->cmd_timeout_ms * 1000; That is where I concerned about the uninitialized cmd_timeout_ms. > + else > + target_timeout = data->timeout_ns / 1000 + > + data->timeout_clks / host->clock; > > /* > * Figure out needed cycles. > @@ -632,8 +636,9 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, > struct mmc_data *data) > } > > if (count >= 0xF) { > - printk(KERN_WARNING "%s: Too large timeout requested!\n", > - mmc_hostname(host->mmc)); > + printk(KERN_WARNING "%s: Too large timeout requested for > CMD%d!\n", > + mmc_hostname(host->mmc), > + cmd->opcode); > count = 0xE; > } > > @@ -651,15 +656,21 @@ static void sdhci_set_transfer_irqs(struct sdhci_host > *host) > sdhci_clear_set_irqs(host, dma_irqs, pio_irqs); > } > > -static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data) > +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command > *cmd) > { > u8 count; > u8 ctrl; > + struct mmc_data *data = cmd->data; > int ret; > > WARN_ON(host->data); > > - if (data == NULL) > + if (data || (cmd->flags & MMC_RSP_BUSY)) { > + count = sdhci_calc_timeout(host, cmd); > + sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL); > + } > + > + if (!data) > return; > > /* Sanity checks */ > @@ -670,9 +681,6 @@ static void sdhci_prepare_data(struct sdhci_host *host, > struct mmc_data *data) > host->data = data; > host->data_early = 0; > > - count = sdhci_calc_timeout(host, data); > - sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL); > - > if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) > host->flags |= SDHCI_REQ_USE_DMA; > > @@ -920,7 +928,7 @@ static void sdhci_send_command(struct sdhci_host *host, > struct mmc_command *cmd) > > host->cmd = cmd; > > - sdhci_prepare_data(host, cmd->data); > + sdhci_prepare_data(host, cmd); > > sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT); > > @@ -1867,6 +1875,9 @@ int sdhci_add_host(struct sdhci_host *host) > if (caps & SDHCI_TIMEOUT_CLK_UNIT) > host->timeout_clk *= 1000; > > + if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) > + host->timeout_clk = host->clock / 1000; > + > /* > * Set host parameters. > */ > @@ -1879,7 +1890,7 @@ int sdhci_add_host(struct sdhci_host *host) > mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200; > > mmc->f_max = host->max_clk; > - mmc->caps |= MMC_CAP_SDIO_IRQ; > + mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE; > > /* > * A controller may support 8-bit width, but the board itself > -- > 1.7.0.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 12, 2011 at 4:06 AM, Dong, Chuanxiao <chuanxiao.dong@intel.com> wrote: > > >> -----Original Message----- >> From: linux-mmc-owner@vger.kernel.org >> [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Andrei Warkentin >> Sent: Tuesday, April 12, 2011 5:14 AM >> To: linux-mmc@vger.kernel.org >> Cc: arnd@arndb.de; cjb@laptop.org; Andrei Warkentin >> Subject: [patchv3 2/4] MMC: SDHCI R1B command handling + MMC_CAP_ERASE. >> >> ERASE command needs R1B response, so fix R1B-type command >> handling for SDHCI controller. For non-DAT commands using a busy >> reponse, the cmd->cmd_timeout_ms (in ms) field is used for timeout >> calculations. >> >> Based on patch by Chuanxiao Dong <chuanxiao.dong@intel.com> >> Signed-off-by: Andrei Warkentin <andreiw@motorola.com> >> --- >> drivers/mmc/host/sdhci.c | 43 +++++++++++++++++++++++++++---------------- >> 1 files changed, 27 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index 9e15f41..173e980 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -40,7 +40,6 @@ >> >> static unsigned int debug_quirks = 0; >> >> -static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *); >> static void sdhci_finish_data(struct sdhci_host *); >> >> static void sdhci_send_command(struct sdhci_host *, struct mmc_command *); >> @@ -591,9 +590,10 @@ static void sdhci_adma_table_post(struct sdhci_host >> *host, >> data->sg_len, direction); >> } >> >> -static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data) >> +static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command >> *cmd) >> { >> u8 count; >> + struct mmc_data *data = cmd->data; >> unsigned target_timeout, current_timeout; >> >> /* >> @@ -605,12 +605,16 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, >> struct mmc_data *data) >> if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL) >> return 0xE; >> >> - /* timeout in us */ >> - target_timeout = data->timeout_ns / 1000 + >> - data->timeout_clks / host->clock; >> + /* Unspecified timeout, assume max */ >> + if (!data && !cmd->cmd_timeout_ms) >> + return 0xE; > Just a inline question here. I noticed cmd_timeout_ms only be set for the ERASE command and SWITCH command, for other types of commands, this value is left not initialized. Cmd_timeout_ms may not be zero and also not be initialized. And next, driver will use this value to calculate the timeout. So I think using an uninitialized value here doesn't make sense. If we want to use it, we have to make sure at this point, this value is already initialized. First, cmd_timeout only has meaning for non-DAT-transfer commands using DAT as a busy indicator. This is exactly why the cmd_timeout_ms affects the "data timeout" on the host. Hence - cmd_timeout_ms only makes sense for R1b commands. (R5b too, but that's SDIO land and I don't want to push support for something I can't verify) Second - if it's not initialized, it is zero (look in block.c, the entire brq is cleared to 0, look in mmc_ops, sd_ops, sdio_ops). Remember, if !data and !cmd_timeout, then this must be a busy-wait command with no timeout specified, we pick the safe maximum of 0xE. Also again, this timeout has any meaning (and is only calculated) only if the command actually leverages the DAT line. What commands are you interested in? There are not a lot of R1b commands. This patch addressed missing DAT timeout for R1b commands like erase, switch, etc. > >> >> - if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) >> - host->timeout_clk = host->clock / 1000; >> + /* timeout in us */ >> + if (!data) >> + target_timeout = cmd->cmd_timeout_ms * 1000; > That is where I concerned about the uninitialized cmd_timeout_ms. You claim it's uninitialized, but it is zero because all consumers of mmc_command memset the structure to 0. > >> + else >> + target_timeout = data->timeout_ns / 1000 + >> + data->timeout_clks / host->clock; >> >> /* >> * Figure out needed cycles. >> @@ -632,8 +636,9 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, >> struct mmc_data *data) >> } >> >> if (count >= 0xF) { >> - printk(KERN_WARNING "%s: Too large timeout requested!\n", >> - mmc_hostname(host->mmc)); >> + printk(KERN_WARNING "%s: Too large timeout requested for >> CMD%d!\n", >> + mmc_hostname(host->mmc), >> + cmd->opcode); >> count = 0xE; >> } >> >> @@ -651,15 +656,21 @@ static void sdhci_set_transfer_irqs(struct sdhci_host >> *host) >> sdhci_clear_set_irqs(host, dma_irqs, pio_irqs); >> } >> >> -static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data) >> +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command >> *cmd) >> { >> u8 count; >> u8 ctrl; >> + struct mmc_data *data = cmd->data; >> int ret; >> >> WARN_ON(host->data); >> >> - if (data == NULL) >> + if (data || (cmd->flags & MMC_RSP_BUSY)) { >> + count = sdhci_calc_timeout(host, cmd); >> + sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL); >> + } >> + >> + if (!data) >> return; >> >> /* Sanity checks */ >> @@ -670,9 +681,6 @@ static void sdhci_prepare_data(struct sdhci_host *host, >> struct mmc_data *data) >> host->data = data; >> host->data_early = 0; >> >> - count = sdhci_calc_timeout(host, data); >> - sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL); >> - >> if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) >> host->flags |= SDHCI_REQ_USE_DMA; >> >> @@ -920,7 +928,7 @@ static void sdhci_send_command(struct sdhci_host *host, >> struct mmc_command *cmd) >> >> host->cmd = cmd; >> >> - sdhci_prepare_data(host, cmd->data); >> + sdhci_prepare_data(host, cmd); >> >> sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT); >> >> @@ -1867,6 +1875,9 @@ int sdhci_add_host(struct sdhci_host *host) >> if (caps & SDHCI_TIMEOUT_CLK_UNIT) >> host->timeout_clk *= 1000; >> >> + if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) >> + host->timeout_clk = host->clock / 1000; >> + >> /* >> * Set host parameters. >> */ >> @@ -1879,7 +1890,7 @@ int sdhci_add_host(struct sdhci_host *host) >> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200; >> >> mmc->f_max = host->max_clk; >> - mmc->caps |= MMC_CAP_SDIO_IRQ; >> + mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE; >> >> /* >> * A controller may support 8-bit width, but the board itself >> -- >> 1.7.0.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Andrei Warkentin [mailto:andreiw@motorola.com] > Sent: Wednesday, April 13, 2011 2:05 AM > To: Dong, Chuanxiao > Cc: linux-mmc@vger.kernel.org; arnd@arndb.de; cjb@laptop.org > Subject: Re: [patchv3 2/4] MMC: SDHCI R1B command handling + > MMC_CAP_ERASE. > > On Tue, Apr 12, 2011 at 4:06 AM, Dong, Chuanxiao > <chuanxiao.dong@intel.com> wrote: > > > > > >> -----Original Message----- > >> From: linux-mmc-owner@vger.kernel.org > >> [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Andrei Warkentin > >> Sent: Tuesday, April 12, 2011 5:14 AM > >> To: linux-mmc@vger.kernel.org > >> Cc: arnd@arndb.de; cjb@laptop.org; Andrei Warkentin > >> Subject: [patchv3 2/4] MMC: SDHCI R1B command handling + > MMC_CAP_ERASE. > >> > >> ERASE command needs R1B response, so fix R1B-type command > >> handling for SDHCI controller. For non-DAT commands using a busy > >> reponse, the cmd->cmd_timeout_ms (in ms) field is used for timeout > >> calculations. > >> > >> Based on patch by Chuanxiao Dong <chuanxiao.dong@intel.com> > >> Signed-off-by: Andrei Warkentin <andreiw@motorola.com> > >> --- > >> drivers/mmc/host/sdhci.c | 43 > +++++++++++++++++++++++++++---------------- > >> 1 files changed, 27 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > >> index 9e15f41..173e980 100644 > >> --- a/drivers/mmc/host/sdhci.c > >> +++ b/drivers/mmc/host/sdhci.c > >> @@ -40,7 +40,6 @@ > >> > >> static unsigned int debug_quirks = 0; > >> > >> -static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *); > >> static void sdhci_finish_data(struct sdhci_host *); > >> > >> static void sdhci_send_command(struct sdhci_host *, struct mmc_command > *); > >> @@ -591,9 +590,10 @@ static void sdhci_adma_table_post(struct sdhci_host > >> *host, > >> data->sg_len, direction); > >> } > >> > >> -static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data) > >> +static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command > >> *cmd) > >> { > >> u8 count; > >> + struct mmc_data *data = cmd->data; > >> unsigned target_timeout, current_timeout; > >> > >> /* > >> @@ -605,12 +605,16 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, > >> struct mmc_data *data) > >> if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL) > >> return 0xE; > >> > >> - /* timeout in us */ > >> - target_timeout = data->timeout_ns / 1000 + > >> - data->timeout_clks / host->clock; > >> + /* Unspecified timeout, assume max */ > >> + if (!data && !cmd->cmd_timeout_ms) > >> + return 0xE; > > Just a inline question here. I noticed cmd_timeout_ms only be set for the ERASE > command and SWITCH command, for other types of commands, this value is left > not initialized. Cmd_timeout_ms may not be zero and also not be initialized. And > next, driver will use this value to calculate the timeout. So I think using an > uninitialized value here doesn't make sense. If we want to use it, we have to make > sure at this point, this value is already initialized. > > First, cmd_timeout only has meaning for non-DAT-transfer commands > using DAT as a busy indicator. This is exactly why the cmd_timeout_ms > affects the "data timeout" on the host. Hence - cmd_timeout_ms only > makes sense for R1b commands. (R5b too, but that's SDIO land and I > don't want to push support for something I can't verify) Second - if > it's not initialized, it is zero (look in block.c, the entire brq is > cleared to 0, look in mmc_ops, sd_ops, sdio_ops). > > Remember, if !data and !cmd_timeout, then this must be a busy-wait > command with no timeout specified, we pick the safe maximum of 0xE. > Also again, this timeout has any meaning (and is only calculated) only > if the command actually leverages the DAT line. > > What commands are you interested in? There are not a lot of R1b > commands. This patch addressed missing DAT timeout for R1b commands > like erase, switch, etc. > > > > >> > >> - if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) > >> - host->timeout_clk = host->clock / 1000; > >> + /* timeout in us */ > >> + if (!data) > >> + target_timeout = cmd->cmd_timeout_ms * 1000; > > That is where I concerned about the uninitialized cmd_timeout_ms. > > You claim it's uninitialized, but it is zero because all consumers of > mmc_command memset the structure to 0. Yes, if all the consumers of mmc_command memset the structure to 0, there will be no problem. But just reviewed the code, and found mmc_app_cmd() (in sd_ops.c) didn't memset the command structure. So I think if we can make sure all the command structures will be initialized before using it, that would be better. > > > > >> + else > >> + target_timeout = data->timeout_ns / 1000 + > >> + data->timeout_clks / host->clock; > >> > >> /* > >> * Figure out needed cycles. > >> @@ -632,8 +636,9 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, > >> struct mmc_data *data) > >> } > >> > >> if (count >= 0xF) { > >> - printk(KERN_WARNING "%s: Too large timeout requested!\n", > >> - mmc_hostname(host->mmc)); > >> + printk(KERN_WARNING "%s: Too large timeout requested for > >> CMD%d!\n", > >> + mmc_hostname(host->mmc), > >> + cmd->opcode); > >> count = 0xE; > >> } > >> > >> @@ -651,15 +656,21 @@ static void sdhci_set_transfer_irqs(struct sdhci_host > >> *host) > >> sdhci_clear_set_irqs(host, dma_irqs, pio_irqs); > >> } > >> > >> -static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data > *data) > >> +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command > >> *cmd) > >> { > >> u8 count; > >> u8 ctrl; > >> + struct mmc_data *data = cmd->data; > >> int ret; > >> > >> WARN_ON(host->data); > >> > >> - if (data == NULL) > >> + if (data || (cmd->flags & MMC_RSP_BUSY)) { > >> + count = sdhci_calc_timeout(host, cmd); > >> + sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL); > >> + } > >> + > >> + if (!data) > >> return; > >> > >> /* Sanity checks */ > >> @@ -670,9 +681,6 @@ static void sdhci_prepare_data(struct sdhci_host *host, > >> struct mmc_data *data) > >> host->data = data; > >> host->data_early = 0; > >> > >> - count = sdhci_calc_timeout(host, data); > >> - sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL); > >> - > >> if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) > >> host->flags |= SDHCI_REQ_USE_DMA; > >> > >> @@ -920,7 +928,7 @@ static void sdhci_send_command(struct sdhci_host > *host, > >> struct mmc_command *cmd) > >> > >> host->cmd = cmd; > >> > >> - sdhci_prepare_data(host, cmd->data); > >> + sdhci_prepare_data(host, cmd); > >> > >> sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT); > >> > >> @@ -1867,6 +1875,9 @@ int sdhci_add_host(struct sdhci_host *host) > >> if (caps & SDHCI_TIMEOUT_CLK_UNIT) > >> host->timeout_clk *= 1000; > >> > >> + if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) > >> + host->timeout_clk = host->clock / 1000; > >> + > >> /* > >> * Set host parameters. > >> */ > >> @@ -1879,7 +1890,7 @@ int sdhci_add_host(struct sdhci_host *host) > >> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200; > >> > >> mmc->f_max = host->max_clk; > >> - mmc->caps |= MMC_CAP_SDIO_IRQ; > >> + mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE; > >> > >> /* > >> * A controller may support 8-bit width, but the board itself > >> -- > >> 1.7.0.4 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 12, 2011 at 8:59 PM, Dong, Chuanxiao
<chuanxiao.dong@intel.com> wrote:
> Yes, if all the consumers of mmc_command memset the structure to 0, there will be no problem. But just reviewed the code, and found mmc_app_cmd() (in sd_ops.c) didn't memset the command structure. So I think if we can make sure all the command structures will be initialized before using it, that would be better.
I would say that CMD55 is not a command that utilizes the DAT line,
again, so not setting cmd_timeout itself is not a problem.
However, using uninitialized data is nasty, and can lead to problems
if cmd->data ever happens to be not NULL.
I'll send a patch for this tomorrow. Thanks for catching this. I'm
curious if this is the reason behind Cyril's problem.
Cyril can you try modifying mmc_app_cmd like this? Does that do
anything for you?
static int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
{
int err;
struct mmc_command cmd;
BUG_ON(!host);
BUG_ON(card && (card->host != host));
+ memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_APP_CMD;
A
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi! > > Yes, if all the consumers of mmc_command memset the structure to 0, there > > will be no problem. But just reviewed the code, and found mmc_app_cmd() (in > > sd_ops.c) didn't memset the command structure. So I think if we can make > > sure all the command structures will be initialized before using it, that > > would be better. > > I would say that CMD55 is not a command that utilizes the DAT line, > again, so not setting cmd_timeout itself is not a problem. > > However, using uninitialized data is nasty, and can lead to problems > if cmd->data ever happens to be not NULL. > I'll send a patch for this tomorrow. Thanks for catching this. I'm > curious if this is the reason behind Cyril's problem. > > Cyril can you try modifying mmc_app_cmd like this? Does that do > anything for you? > > static int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card) > { > int err; > struct mmc_command cmd; > > BUG_ON(!host); > BUG_ON(card && (card->host != host)); > > + memset(&cmd, 0, sizeof(struct mmc_command)); > cmd.opcode = MMC_APP_CMD; > Not really, it behaves same regardless the memset().
Hi Andrei, On Mon, Apr 11 2011, Andrei Warkentin wrote: > ERASE command needs R1B response, so fix R1B-type command > handling for SDHCI controller. For non-DAT commands using a busy > reponse, the cmd->cmd_timeout_ms (in ms) field is used for timeout > calculations. > > Based on patch by Chuanxiao Dong <chuanxiao.dong@intel.com> > Signed-off-by: Andrei Warkentin <andreiw@motorola.com> Looks like this patch has been implicated in causing IO errors on ThinkPads with a Ricoh device -- please could you take a look at https://bugzilla.kernel.org/show_bug.cgi?id=38922 ? Thanks, - Chris.
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 9e15f41..173e980 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -40,7 +40,6 @@ static unsigned int debug_quirks = 0; -static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *); static void sdhci_finish_data(struct sdhci_host *); static void sdhci_send_command(struct sdhci_host *, struct mmc_command *); @@ -591,9 +590,10 @@ static void sdhci_adma_table_post(struct sdhci_host *host, data->sg_len, direction); } -static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data) +static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) { u8 count; + struct mmc_data *data = cmd->data; unsigned target_timeout, current_timeout; /* @@ -605,12 +605,16 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data) if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL) return 0xE; - /* timeout in us */ - target_timeout = data->timeout_ns / 1000 + - data->timeout_clks / host->clock; + /* Unspecified timeout, assume max */ + if (!data && !cmd->cmd_timeout_ms) + return 0xE; - if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) - host->timeout_clk = host->clock / 1000; + /* timeout in us */ + if (!data) + target_timeout = cmd->cmd_timeout_ms * 1000; + else + target_timeout = data->timeout_ns / 1000 + + data->timeout_clks / host->clock; /* * Figure out needed cycles. @@ -632,8 +636,9 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data) } if (count >= 0xF) { - printk(KERN_WARNING "%s: Too large timeout requested!\n", - mmc_hostname(host->mmc)); + printk(KERN_WARNING "%s: Too large timeout requested for CMD%d!\n", + mmc_hostname(host->mmc), + cmd->opcode); count = 0xE; } @@ -651,15 +656,21 @@ static void sdhci_set_transfer_irqs(struct sdhci_host *host) sdhci_clear_set_irqs(host, dma_irqs, pio_irqs); } -static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data) +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) { u8 count; u8 ctrl; + struct mmc_data *data = cmd->data; int ret; WARN_ON(host->data); - if (data == NULL) + if (data || (cmd->flags & MMC_RSP_BUSY)) { + count = sdhci_calc_timeout(host, cmd); + sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL); + } + + if (!data) return; /* Sanity checks */ @@ -670,9 +681,6 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data) host->data = data; host->data_early = 0; - count = sdhci_calc_timeout(host, data); - sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL); - if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) host->flags |= SDHCI_REQ_USE_DMA; @@ -920,7 +928,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) host->cmd = cmd; - sdhci_prepare_data(host, cmd->data); + sdhci_prepare_data(host, cmd); sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT); @@ -1867,6 +1875,9 @@ int sdhci_add_host(struct sdhci_host *host) if (caps & SDHCI_TIMEOUT_CLK_UNIT) host->timeout_clk *= 1000; + if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) + host->timeout_clk = host->clock / 1000; + /* * Set host parameters. */ @@ -1879,7 +1890,7 @@ int sdhci_add_host(struct sdhci_host *host) mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200; mmc->f_max = host->max_clk; - mmc->caps |= MMC_CAP_SDIO_IRQ; + mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE; /* * A controller may support 8-bit width, but the board itself
ERASE command needs R1B response, so fix R1B-type command handling for SDHCI controller. For non-DAT commands using a busy reponse, the cmd->cmd_timeout_ms (in ms) field is used for timeout calculations. Based on patch by Chuanxiao Dong <chuanxiao.dong@intel.com> Signed-off-by: Andrei Warkentin <andreiw@motorola.com> --- drivers/mmc/host/sdhci.c | 43 +++++++++++++++++++++++++++---------------- 1 files changed, 27 insertions(+), 16 deletions(-)