On Wed, 20 Feb 2019 at 18:13, Ludovic BARRE <ludovic.barre@xxxxxx> wrote:
>
> hi Ulf
>
> I think you can apply this patch.
> The R1B problem (due to busy timeout) can be seen in a future series
> about busy detect.
> Thanks
>
> Tested-by: Ludovic Barre <ludovic.barre@xxxxxx>
Okay, great, I queue this up.
Kind regards
Uffe
>
> Regards
> Ludo
>
> On 2/20/19 11:27 AM, Ludovic BARRE wrote:
> > hi Ulf
> >
> > On 2/4/19 4:39 PM, Ulf Hansson wrote:
> >> On Mon, 4 Feb 2019 at 15:18, Ludovic BARRE <ludovic.barre@xxxxxx> wrote:
> >>>
> >>> hi Ulf
> >>>
> >>> One comment below in mmci_cmd_irq function
> >>>
> >>> when I rebase my next serie about "busy detect" I've a kernel panic
> >>> null pointer, could you give me some time to investigate this issue
> >>> before apply this patch.
> >>
> >> Sure!
> >>
> >> FYI, I have tested this on a ux500 and didn't observe any regressions.
> >
> > great
> >
> >>
> >>>
> >>>
> >>> On 1/29/19 3:35 PM, Ulf Hansson wrote:
> >>>> The current approach with sending a CMD12 (STOP_TRANSMISSION) to
> >>>> complete a
> >>>> data transfer request, either because of using the open-ended
> >>>> transmission
> >>>> type or because of receiving an error during a pre-defined data
> >>>> transfer,
> >>>> isn't sufficient for the STM32 sdmmc variant. More precisely, this
> >>>> variant
> >>>> needs to clear the DPSM ("Data Path State Machine") by sending a
> >>>> CMD12, for
> >>>> all failing ADTC commands.
> >>>>
> >>>> Support this, by adding a struct mmc_command inside the struct
> >>>> mmci_host
> >>>> and initialize it to a CMD12 during ->probe(). Let's also add checks
> >>>> for
> >>>> the new conditions, to enable mmci_data_irq() and mmci_cmd_irq() to
> >>>> postpone the calls to mmci_request_end(), but instead send the CMD12.
> >>>>
> >>>> Cc: Ludovic Barre <ludovic.barre@xxxxxx>
> >>>> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> >>>> ---
> >>>> drivers/mmc/host/mmci.c | 25 +++++++++++++++++++++++--
> >>>> drivers/mmc/host/mmci.h | 1 +
> >>>> 2 files changed, 24 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> >>>> index e352f5ad5801..45b97b41601c 100644
> >>>> --- a/drivers/mmc/host/mmci.c
> >>>> +++ b/drivers/mmc/host/mmci.c
> >>>> @@ -1127,6 +1127,12 @@ mmci_start_command(struct mmci_host *host,
> >>>> struct mmc_command *cmd, u32 c)
> >>>> writel(c, base + MMCICOMMAND);
> >>>> }
> >>>>
> >>>> +static void mmci_stop_command(struct mmci_host *host)
> >>>> +{
> >>>> + host->stop_abort.error = 0;
> >>>> + mmci_start_command(host, &host->stop_abort, 0);
> >>>> +}
> >>>> +
> >>>> static void
> >>>> mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
> >>>> unsigned int status)
> >>>> @@ -1196,10 +1202,16 @@ mmci_data_irq(struct mmci_host *host, struct
> >>>> mmc_data *data,
> >>>> /* The error clause is handled above,
> >>>> success! */
> >>>> data->bytes_xfered = data->blksz * data->blocks;
> >>>>
> >>>> - if (!data->stop || (host->mrq->sbc && !data->error))
> >>>> + if (!data->stop) {
> >>>> + if (host->variant->cmdreg_stop && data->error)
> >>>> + mmci_stop_command(host);
> >>>> + else
> >>>> + mmci_request_end(host, data->mrq);
> >>>> + } else if (host->mrq->sbc && !data->error) {
> >>>> mmci_request_end(host, data->mrq);
> >>>> - else
> >>>> + } else {
> >>>> mmci_start_command(host, data->stop, 0);
> >>>> + }
> >>>> }
> >>>> }
> >>>>
> >>>> @@ -1298,6 +1310,10 @@ mmci_cmd_irq(struct mmci_host *host, struct
> >>>> mmc_command *cmd,
> >>>> mmci_dma_error(host);
> >>>>
> >>>> mmci_stop_data(host);
> >>>> + if (host->variant->cmdreg_stop && cmd->error) {
> >>>> + mmci_stop_command(host);
> >>>> + return;
> >>>> + }
> >>>
> >>>> }
> >>>
> >>> In fact a cmd without host->data could fail with DPSM enabled.
> >>> example:
> >>> sent a cmd12 with MMC_RSP_BUSY flag (R1B command) and so no data.
> >>> In sdmmc variant the MMCIDATATIMER is used to busy timeout.
> >>> The DPSM is used to sample D0 line and is activated.
> >>> the DTIMEOUT flag is set after when no end of busy received before the
> >>> timeout. (DPSM stays in BUSY and wait for Abort)
> >>
> >> Oh, that I didn't know about! That make it quite complicated, I am
> >> afraid.
> >>
> >> To send a new CMD12 to clear the DPSM, because the previous CDM12
> >> failed seems just wrong. Why would the second CMD12 succeed and not
> >> timeout again, if you see what I mean.
> >>
> >> There are also other commands using R1B, that doesn't involve
> >> transferring data. The CMD5 (sleep) for example. If any of these
> >> commands would timeout, then why would a following CMD12 not do so as
> >> well?
> >>
> >> To me, it seems like a reset of the controller is the only really sane
> >> way of treating these scenarios, isn't it? Of course, it means we need
> >> to be able to restore the state of the controller after such a reset
> >> as well. Is this possible?
> >>
> >
> > POV sdmmc hardware block the second cmd12 just allows to clear the DPSM
> > (for stop abort cmd the R1B may not be set).
> > I agree with you, if a command fail due to R1B (busy timeout) the better
> > way is to reset the controller, protocol...
> >
> > Do to that we could call mmc_hw_reset function
> > (drivers/mmc/core/core.c). However this function can't be call
> > in irq context and ongoing request must be completed.
> >
> > proposition:
> > The host driver could set a error value (in cmd->error or data->error)
> > like EDEADLK or ECONNRESET (to be defined)...
> > The framework could check error value (for example in
> > mmc_wait_for_req_done) and call mmc_hw_reset if needed.
> >
> > What do you think?
> >
> >>>
> >>> So The mmci_stop_command may be sent when there are cmd->error and
> >>> status & MCI_DATATIMEOUT
> >>>
> >>>
> >>>> mmci_request_end(host, host->mrq);
> >>>> } else if (sbc) {
> >>>> @@ -1956,6 +1972,11 @@ static int mmci_probe(struct amba_device *dev,
> >>>> mmc->max_busy_timeout = 0;
> >>>> }
> >>>>
> >>>> + /* Prepare a CMD12 - needed to clear the DPSM on some
> >>>> variants. */
> >>>> + host->stop_abort.opcode = MMC_STOP_TRANSMISSION;
> >>>> + host->stop_abort.arg = 0;
> >>>> + host->stop_abort.flags = MMC_RSP_R1B | MMC_CMD_AC;
> >>>> +
> >>>> mmc->ops = &mmci_ops;
> >>>>
> >>>> /* We support these PM capabilities. */
> >>>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> >>>> index 24229097d05c..14df81054438 100644
> >>>> --- a/drivers/mmc/host/mmci.h
> >>>> +++ b/drivers/mmc/host/mmci.h
> >>>> @@ -377,6 +377,7 @@ struct mmci_host {
> >>>> void __iomem *base;
> >>>> struct mmc_request *mrq;
> >>>> struct mmc_command *cmd;
> >>>> + struct mmc_command stop_abort;
> >>>> struct mmc_data *data;
> >>>> struct mmc_host *mmc;
> >>>> struct clk *clk;
> >>>>
> >>
> >> Kind regards
> >> Uffe
> >>
