| Message ID | 51B97CDB.7010207@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hello Shimoda-san Thank you for your patch. On Thu, 13 Jun 2013, Shimoda, Yoshihiro wrote: > This patch adds SET_BLOCK_COUNT(CMD23) support to sh_mmcif driver. > If we add MMC_CAP_CMD23 to ".caps" of sh_mmcif_plat_data, the mmc > core driver will use CMD23. Then, the sh_mmcif driver can use > Reliable Write feature. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > This patch is based on the latest mmc-next branch of mmc.git. > > drivers/mmc/host/sh_mmcif.c | 83 +++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 81 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c > index 8ef5efa..14d4c81 100644 > --- a/drivers/mmc/host/sh_mmcif.c > +++ b/drivers/mmc/host/sh_mmcif.c > @@ -244,6 +244,7 @@ struct sh_mmcif_host { > bool power; > bool card_present; > struct mutex thread_lock; > + struct completion sbc_complete; > > /* DMA support */ > struct dma_chan *chan_rx; > @@ -802,7 +803,11 @@ static u32 sh_mmcif_set_cmd(struct sh_mmcif_host *host, > tmp |= CMD_SET_DWEN; > /* CMLTE/CMD12EN */ > if (opc == MMC_READ_MULTIPLE_BLOCK || opc == MMC_WRITE_MULTIPLE_BLOCK) { > - tmp |= CMD_SET_CMLTE | CMD_SET_CMD12EN; > + /* If SBC, we don't use CMD12(STOP) */ > + if (mrq->sbc) > + tmp |= CMD_SET_CMLTE; > + else > + tmp |= CMD_SET_CMLTE | CMD_SET_CMD12EN; > sh_mmcif_bitset(host, MMCIF_CE_BLOCK_SET, > data->blocks << 16); > } > @@ -903,6 +908,43 @@ static void sh_mmcif_stop_cmd(struct sh_mmcif_host *host, > host->wait_for = MMCIF_WAIT_FOR_STOP; > } > > +static bool sh_mmcif_send_sbc(struct sh_mmcif_host *host, > + struct mmc_request *mrq) > +{ > + struct mmc_request req_orig = *mrq; > + long time; > + > + /* Switch the commands around */ > + mrq->cmd = mrq->sbc; > + mrq->sbc = NULL; > + mrq->data = NULL; > + mrq->stop = NULL; > + > + /* Send SBC Cmd */ > + sh_mmcif_start_cmd(host, mrq); > + > + /* Normal completion time is less than 1us */ > + time = wait_for_completion_interruptible_timeout(&host->sbc_complete, > + host->timeout); I'm afraid this doesn't look like a correct approach to me. In commit f985da1 "mmc: sh_mmcif: process requests asynchronously" I converted the driver to not wait inside its .request() method. This your patch makes a part of the .request() processing synchronous again by adding a wait to it. Besides you're very much special casing the processing of the SBC command. I think, it would be better to process it asynchronously too, implementing it as a sequence of two requests, similar to how sdhci.c does it (see sdhci_request() nearer the end the "if (mrq->sbc...) handling and sdhci_finish_command() below the "Finished CMD23, now send actual command" comment). Would that be possible to convert this patch to execute similarly and to avoid special-casing as much as possible? Just check for an SBC in .request(), if there is one send it instead of the proper request. Then in completion check, whether it's the SBC that has just completed, and if so, now send the actual request. Thanks Guennadi > + > + /* Restore */ > + mrq->cmd = req_orig.cmd; > + mrq->sbc = req_orig.sbc; > + mrq->data = req_orig.data; > + mrq->stop = req_orig.stop; > + > + if (mrq->sbc->error || host->sd_error || time <= 0) { > + dev_dbg(&host->pd->dev, "%s(): failed!\n", __func__); > + host->state = STATE_IDLE; > + if (!time) > + mrq->sbc->error = -ETIMEDOUT; > + mmc_request_done(host->mmc, mrq); > + return true; > + } > + > + return false; > +} > + > static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq) > { > struct sh_mmcif_host *host = mmc_priv(mmc); > @@ -938,6 +980,11 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq) > > host->mrq = mrq; > > + if (mrq->sbc) { > + if (sh_mmcif_send_sbc(host, mrq)) > + return; > + } > + > sh_mmcif_start_cmd(host, mrq); > } > > @@ -1074,6 +1121,9 @@ static bool sh_mmcif_end_cmd(struct sh_mmcif_host *host) > > sh_mmcif_get_response(host, cmd); > > + if (cmd->opcode == MMC_SET_BLOCK_COUNT) > + complete(&host->sbc_complete); > + > if (!data) > return false; > > @@ -1212,13 +1262,35 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id) > return IRQ_HANDLED; > } > > + if (mrq->sbc && (mrq->cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK) && > + (host->wait_for != MMCIF_WAIT_FOR_WRITE_END)) { > + /* Wait for end of data phase */ > + host->wait_for = MMCIF_WAIT_FOR_WRITE_END; > + sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MDTRANE); > + schedule_delayed_work(&host->timeout_work, host->timeout); > + mutex_unlock(&host->thread_lock); > + return IRQ_HANDLED; > + } > + > + if (mrq->sbc && (mrq->cmd->opcode == MMC_READ_MULTIPLE_BLOCK) && > + (host->wait_for != MMCIF_WAIT_FOR_READ_END)) { > + /* Wait for end of data phase */ > + host->wait_for = MMCIF_WAIT_FOR_READ_END; > + sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFRE); > + schedule_delayed_work(&host->timeout_work, host->timeout); > + mutex_unlock(&host->thread_lock); > + return IRQ_HANDLED; > + } > + > if (host->wait_for != MMCIF_WAIT_FOR_STOP) { > struct mmc_data *data = mrq->data; > if (!mrq->cmd->error && data && !data->error) > data->bytes_xfered = > data->blocks * data->blksz; > > - if (mrq->stop && !mrq->cmd->error && (!data || !data->error)) { > + /* If SBC, we don't use CMD12(STOP) */ > + if (mrq->stop && !mrq->cmd->error && (!data || !data->error) && > + !mrq->sbc) { > sh_mmcif_stop_cmd(host, mrq); > if (!mrq->stop->error) { > schedule_delayed_work(&host->timeout_work, host->timeout); > @@ -1228,6 +1300,12 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id) > } > } > > + if ((mrq->cmd->opcode == MMC_SET_BLOCK_COUNT) && !mrq->cmd->error) { > + schedule_delayed_work(&host->timeout_work, host->timeout); > + mutex_unlock(&host->thread_lock); > + return IRQ_HANDLED; > + } > + > host->wait_for = MMCIF_WAIT_FOR_REQUEST; > host->state = STATE_IDLE; > host->mrq = NULL; > @@ -1379,6 +1457,7 @@ static int sh_mmcif_probe(struct platform_device *pdev) > host->pd = pdev; > > spin_lock_init(&host->lock); > + init_completion(&host->sbc_complete); > > mmc->ops = &sh_mmcif_ops; > sh_mmcif_init_ocr(host); > -- > 1.7.1 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Guennadi-san, (2013/06/13 17:33), Guennadi Liakhovetski wrote: < snip > >> +static bool sh_mmcif_send_sbc(struct sh_mmcif_host *host, >> + struct mmc_request *mrq) >> +{ >> + struct mmc_request req_orig = *mrq; >> + long time; >> + >> + /* Switch the commands around */ >> + mrq->cmd = mrq->sbc; >> + mrq->sbc = NULL; >> + mrq->data = NULL; >> + mrq->stop = NULL; >> + >> + /* Send SBC Cmd */ >> + sh_mmcif_start_cmd(host, mrq); >> + >> + /* Normal completion time is less than 1us */ >> + time = wait_for_completion_interruptible_timeout(&host->sbc_complete, >> + host->timeout); > > I'm afraid this doesn't look like a correct approach to me. In commit > f985da1 "mmc: sh_mmcif: process requests asynchronously" I converted the > driver to not wait inside its .request() method. This your patch makes a > part of the .request() processing synchronous again by adding a wait to > it. Besides you're very much special casing the processing of the SBC > command. I think, it would be better to process it asynchronously too, > implementing it as a sequence of two requests, similar to how sdhci.c does > it (see sdhci_request() nearer the end the "if (mrq->sbc...) handling and > sdhci_finish_command() below the "Finished CMD23, now send actual > command" comment). Would that be possible to convert this patch to execute > similarly and to avoid special-casing as much as possible? Just check for > an SBC in .request(), if there is one send it instead of the proper > request. Then in completion check, whether it's the SBC that has just > completed, and if so, now send the actual request. Thank you for your comment. I should have checked your patch... I will modify this SBC patch to remove the wait_for_completion...() in the .request(). Best regards, Yoshihiro Shimoda > Thanks > Guennadi > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c index 8ef5efa..14d4c81 100644 --- a/drivers/mmc/host/sh_mmcif.c +++ b/drivers/mmc/host/sh_mmcif.c @@ -244,6 +244,7 @@ struct sh_mmcif_host { bool power; bool card_present; struct mutex thread_lock; + struct completion sbc_complete; /* DMA support */ struct dma_chan *chan_rx; @@ -802,7 +803,11 @@ static u32 sh_mmcif_set_cmd(struct sh_mmcif_host *host, tmp |= CMD_SET_DWEN; /* CMLTE/CMD12EN */ if (opc == MMC_READ_MULTIPLE_BLOCK || opc == MMC_WRITE_MULTIPLE_BLOCK) { - tmp |= CMD_SET_CMLTE | CMD_SET_CMD12EN; + /* If SBC, we don't use CMD12(STOP) */ + if (mrq->sbc) + tmp |= CMD_SET_CMLTE; + else + tmp |= CMD_SET_CMLTE | CMD_SET_CMD12EN; sh_mmcif_bitset(host, MMCIF_CE_BLOCK_SET, data->blocks << 16); } @@ -903,6 +908,43 @@ static void sh_mmcif_stop_cmd(struct sh_mmcif_host *host, host->wait_for = MMCIF_WAIT_FOR_STOP; } +static bool sh_mmcif_send_sbc(struct sh_mmcif_host *host, + struct mmc_request *mrq) +{ + struct mmc_request req_orig = *mrq; + long time; + + /* Switch the commands around */ + mrq->cmd = mrq->sbc; + mrq->sbc = NULL; + mrq->data = NULL; + mrq->stop = NULL; + + /* Send SBC Cmd */ + sh_mmcif_start_cmd(host, mrq); + + /* Normal completion time is less than 1us */ + time = wait_for_completion_interruptible_timeout(&host->sbc_complete, + host->timeout); + + /* Restore */ + mrq->cmd = req_orig.cmd; + mrq->sbc = req_orig.sbc; + mrq->data = req_orig.data; + mrq->stop = req_orig.stop; + + if (mrq->sbc->error || host->sd_error || time <= 0) { + dev_dbg(&host->pd->dev, "%s(): failed!\n", __func__); + host->state = STATE_IDLE; + if (!time) + mrq->sbc->error = -ETIMEDOUT; + mmc_request_done(host->mmc, mrq); + return true; + } + + return false; +} + static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq) { struct sh_mmcif_host *host = mmc_priv(mmc); @@ -938,6 +980,11 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq) host->mrq = mrq; + if (mrq->sbc) { + if (sh_mmcif_send_sbc(host, mrq)) + return; + } + sh_mmcif_start_cmd(host, mrq); } @@ -1074,6 +1121,9 @@ static bool sh_mmcif_end_cmd(struct sh_mmcif_host *host) sh_mmcif_get_response(host, cmd); + if (cmd->opcode == MMC_SET_BLOCK_COUNT) + complete(&host->sbc_complete); + if (!data) return false; @@ -1212,13 +1262,35 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id) return IRQ_HANDLED; } + if (mrq->sbc && (mrq->cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK) && + (host->wait_for != MMCIF_WAIT_FOR_WRITE_END)) { + /* Wait for end of data phase */ + host->wait_for = MMCIF_WAIT_FOR_WRITE_END; + sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MDTRANE); + schedule_delayed_work(&host->timeout_work, host->timeout); + mutex_unlock(&host->thread_lock); + return IRQ_HANDLED; + } + + if (mrq->sbc && (mrq->cmd->opcode == MMC_READ_MULTIPLE_BLOCK) && + (host->wait_for != MMCIF_WAIT_FOR_READ_END)) { + /* Wait for end of data phase */ + host->wait_for = MMCIF_WAIT_FOR_READ_END; + sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFRE); + schedule_delayed_work(&host->timeout_work, host->timeout); + mutex_unlock(&host->thread_lock); + return IRQ_HANDLED; + } + if (host->wait_for != MMCIF_WAIT_FOR_STOP) { struct mmc_data *data = mrq->data; if (!mrq->cmd->error && data && !data->error) data->bytes_xfered = data->blocks * data->blksz; - if (mrq->stop && !mrq->cmd->error && (!data || !data->error)) { + /* If SBC, we don't use CMD12(STOP) */ + if (mrq->stop && !mrq->cmd->error && (!data || !data->error) && + !mrq->sbc) { sh_mmcif_stop_cmd(host, mrq); if (!mrq->stop->error) { schedule_delayed_work(&host->timeout_work, host->timeout); @@ -1228,6 +1300,12 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id) } } + if ((mrq->cmd->opcode == MMC_SET_BLOCK_COUNT) && !mrq->cmd->error) { + schedule_delayed_work(&host->timeout_work, host->timeout); + mutex_unlock(&host->thread_lock); + return IRQ_HANDLED; + } + host->wait_for = MMCIF_WAIT_FOR_REQUEST; host->state = STATE_IDLE; host->mrq = NULL; @@ -1379,6 +1457,7 @@ static int sh_mmcif_probe(struct platform_device *pdev) host->pd = pdev; spin_lock_init(&host->lock); + init_completion(&host->sbc_complete); mmc->ops = &sh_mmcif_ops; sh_mmcif_init_ocr(host);
This patch adds SET_BLOCK_COUNT(CMD23) support to sh_mmcif driver. If we add MMC_CAP_CMD23 to ".caps" of sh_mmcif_plat_data, the mmc core driver will use CMD23. Then, the sh_mmcif driver can use Reliable Write feature. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- This patch is based on the latest mmc-next branch of mmc.git. drivers/mmc/host/sh_mmcif.c | 83 +++++++++++++++++++++++++++++++++++++++++- 1 files changed, 81 insertions(+), 2 deletions(-)