> -----Original Message----- > From: Andrei Warkentin [mailto:andreiw@xxxxxxxxxxxx] > Sent: Wednesday, April 13, 2011 2:05 AM > To: Dong, Chuanxiao > Cc: linux-mmc@xxxxxxxxxxxxxxx; arnd@xxxxxxxx; cjb@xxxxxxxxxx > 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@xxxxxxxxx> wrote: > > > > > >> -----Original Message----- > >> From: linux-mmc-owner@xxxxxxxxxxxxxxx > >> [mailto:linux-mmc-owner@xxxxxxxxxxxxxxx] On Behalf Of Andrei Warkentin > >> Sent: Tuesday, April 12, 2011 5:14 AM > >> To: linux-mmc@xxxxxxxxxxxxxxx > >> Cc: arnd@xxxxxxxx; cjb@xxxxxxxxxx; 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@xxxxxxxxx> > >> Signed-off-by: Andrei Warkentin <andreiw@xxxxxxxxxxxx> > >> --- > >> 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@xxxxxxxxxxxxxxx > >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
