| Message ID | 1541583041-17461-2-git-send-email-ludovic.Barre@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: mmci: add stop command | expand |
On 7 November 2018 at 10:30, Ludovic Barre <ludovic.Barre@st.com> wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > Refer to "4.15 set block count command" of sd specification: > Host needs to issue CMD12 if any error is detected in > the CMD18 and CMD25 operations. > > In sbc case, the data->stop is fill by framework. > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/mmc/host/mmci.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index 82bab35..13fa640 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -1190,11 +1190,10 @@ 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) { > + if (!data->stop || (host->mrq->sbc && !data->error)) > mmci_request_end(host, data->mrq); > - } else { > + else > mmci_start_command(host, data->stop, 0); This looks correct to me! Although, just wanted to double check that you tested this for a case where we have host->mrq->sbc set and got an error in data->error? I guess it can be tricky, so I was thinking of manually trying to instruct the code, to set an error in data->error, at some point to trigger this code. That would at least give us some confidence that it works as expected. Thoughts? > - } > } > } > > -- > 2.7.4 > Kind regards Uffe
On Tue, 20 Nov 2018 at 10:42, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On 7 November 2018 at 10:30, Ludovic Barre <ludovic.Barre@st.com> wrote: > > From: Ludovic Barre <ludovic.barre@st.com> > > > > Refer to "4.15 set block count command" of sd specification: > > Host needs to issue CMD12 if any error is detected in > > the CMD18 and CMD25 operations. > > > > In sbc case, the data->stop is fill by framework. > > > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > > --- > > drivers/mmc/host/mmci.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > > index 82bab35..13fa640 100644 > > --- a/drivers/mmc/host/mmci.c > > +++ b/drivers/mmc/host/mmci.c > > @@ -1190,11 +1190,10 @@ 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) { > > + if (!data->stop || (host->mrq->sbc && !data->error)) > > mmci_request_end(host, data->mrq); > > - } else { > > + else > > mmci_start_command(host, data->stop, 0); > > This looks correct to me! > > Although, just wanted to double check that you tested this for a case > where we have host->mrq->sbc set and got an error in data->error? I > guess it can be tricky, so I was thinking of manually trying to > instruct the code, to set an error in data->error, at some point to > trigger this code. That would at least give us some confidence that it > works as expected. I did some manual tests to trigger the error path. As far as I can tell, it works as expected and I observes that the core is able to recover and re-send the request. [...] So, I have added my tested-by tag and applied this for next. Thanks! In regards to patch2/2 I am awaiting your update. Kind regards Uffe
On 12/5/18 3:23 PM, Ulf Hansson wrote: > On Tue, 20 Nov 2018 at 10:42, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> On 7 November 2018 at 10:30, Ludovic Barre <ludovic.Barre@st.com> wrote: >>> From: Ludovic Barre <ludovic.barre@st.com> >>> >>> Refer to "4.15 set block count command" of sd specification: >>> Host needs to issue CMD12 if any error is detected in >>> the CMD18 and CMD25 operations. >>> >>> In sbc case, the data->stop is fill by framework. >>> >>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >>> --- >>> drivers/mmc/host/mmci.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >>> index 82bab35..13fa640 100644 >>> --- a/drivers/mmc/host/mmci.c >>> +++ b/drivers/mmc/host/mmci.c >>> @@ -1190,11 +1190,10 @@ 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) { >>> + if (!data->stop || (host->mrq->sbc && !data->error)) >>> mmci_request_end(host, data->mrq); >>> - } else { >>> + else >>> mmci_start_command(host, data->stop, 0); >> >> This looks correct to me! >> >> Although, just wanted to double check that you tested this for a case >> where we have host->mrq->sbc set and got an error in data->error? I >> guess it can be tricky, so I was thinking of manually trying to >> instruct the code, to set an error in data->error, at some point to >> trigger this code. That would at least give us some confidence that it >> works as expected. > > I did some manual tests to trigger the error path. As far as I can > tell, it works as expected and I observes that the core is able to > recover and re-send the request. > > [...] > > So, I have added my tested-by tag and applied this for next. Thanks! > > In regards to patch2/2 I am awaiting your update. > > Kind regards > Uffe >
On 12/5/18 3:23 PM, Ulf Hansson wrote: > On Tue, 20 Nov 2018 at 10:42, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> On 7 November 2018 at 10:30, Ludovic Barre <ludovic.Barre@st.com> wrote: >>> From: Ludovic Barre <ludovic.barre@st.com> >>> >>> Refer to "4.15 set block count command" of sd specification: >>> Host needs to issue CMD12 if any error is detected in >>> the CMD18 and CMD25 operations. >>> >>> In sbc case, the data->stop is fill by framework. >>> >>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >>> --- >>> drivers/mmc/host/mmci.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >>> index 82bab35..13fa640 100644 >>> --- a/drivers/mmc/host/mmci.c >>> +++ b/drivers/mmc/host/mmci.c >>> @@ -1190,11 +1190,10 @@ 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) { >>> + if (!data->stop || (host->mrq->sbc && !data->error)) >>> mmci_request_end(host, data->mrq); >>> - } else { >>> + else >>> mmci_start_command(host, data->stop, 0); >> >> This looks correct to me! >> >> Although, just wanted to double check that you tested this for a case >> where we have host->mrq->sbc set and got an error in data->error? I >> guess it can be tricky, so I was thinking of manually trying to >> instruct the code, to set an error in data->error, at some point to >> trigger this code. That would at least give us some confidence that it >> works as expected. > > I did some manual tests to trigger the error path. As far as I can > tell, it works as expected and I observes that the core is able to > recover and re-send the request. Ulf, very thanks for the tests, and sorry for my busy status. I will send as soon as possible the 2/2 with your recommendation (I will more spare time for upstream) > > [...] > > So, I have added my tested-by tag and applied this for next. Thanks! > > In regards to patch2/2 I am awaiting your update. > > Kind regards > Uffe >
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index 82bab35..13fa640 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -1190,11 +1190,10 @@ 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) { + if (!data->stop || (host->mrq->sbc && !data->error)) mmci_request_end(host, data->mrq); - } else { + else mmci_start_command(host, data->stop, 0); - } } }