* [PATCH V2 0/2] mmc: mmci: add stop command @ 2018-11-07 9:30 Ludovic Barre 2018-11-07 9:30 ` [PATCH V2 1/2] mmc: mmci: send stop command if sbc error issue Ludovic Barre 2018-11-07 9:30 ` [PATCH V2 2/2] mmc: mmci: add variant property to send stop cmd if a command fail Ludovic Barre 0 siblings, 2 replies; 9+ messages in thread From: Ludovic Barre @ 2018-11-07 9:30 UTC (permalink / raw) To: Ulf Hansson, Rob Herring Cc: srinivas.kandagatla, Maxime Coquelin, Alexandre Torgue, linux-arm-kernel, linux-kernel, devicetree, linux-mmc, linux-stm32, Ludovic Barre From: Ludovic Barre <ludovic.barre@st.com> This patch series sends a stop command: -If set block count command fail, the host needs to issue cmd12 if any error is detected in the CMD18 and CMD25 operations. -On stm32 sdmmc variant, if a request fail the DPSM must be cleared by a stop command. Ludovic Barre (2): mmc: mmci: send stop command if sbc error issue mmc: mmci: add variant property to send stop cmd if a command fail drivers/mmc/host/mmci.c | 38 +++++++++++++++++++++++++++++++++++--- drivers/mmc/host/mmci.h | 4 ++++ 2 files changed, 39 insertions(+), 3 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 1/2] mmc: mmci: send stop command if sbc error issue 2018-11-07 9:30 [PATCH V2 0/2] mmc: mmci: add stop command Ludovic Barre @ 2018-11-07 9:30 ` Ludovic Barre 2018-11-20 9:42 ` Ulf Hansson 2018-11-07 9:30 ` [PATCH V2 2/2] mmc: mmci: add variant property to send stop cmd if a command fail Ludovic Barre 1 sibling, 1 reply; 9+ messages in thread From: Ludovic Barre @ 2018-11-07 9:30 UTC (permalink / raw) To: Ulf Hansson, Rob Herring Cc: srinivas.kandagatla, Maxime Coquelin, Alexandre Torgue, linux-arm-kernel, linux-kernel, devicetree, linux-mmc, linux-stm32, Ludovic Barre 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); - } } } -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2 1/2] mmc: mmci: send stop command if sbc error issue 2018-11-07 9:30 ` [PATCH V2 1/2] mmc: mmci: send stop command if sbc error issue Ludovic Barre @ 2018-11-20 9:42 ` Ulf Hansson 2018-12-05 14:23 ` Ulf Hansson 0 siblings, 1 reply; 9+ messages in thread From: Ulf Hansson @ 2018-11-20 9:42 UTC (permalink / raw) To: Ludovic Barre Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin, Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML, linux-mmc, linux-stm32 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 1/2] mmc: mmci: send stop command if sbc error issue 2018-11-20 9:42 ` Ulf Hansson @ 2018-12-05 14:23 ` Ulf Hansson 2018-12-05 15:49 ` Ludovic BARRE 2018-12-05 16:00 ` Ludovic BARRE 0 siblings, 2 replies; 9+ messages in thread From: Ulf Hansson @ 2018-12-05 14:23 UTC (permalink / raw) To: Ludovic Barre Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin, Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML, linux-mmc, linux-stm32 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 1/2] mmc: mmci: send stop command if sbc error issue 2018-12-05 14:23 ` Ulf Hansson @ 2018-12-05 15:49 ` Ludovic BARRE 2018-12-05 16:00 ` Ludovic BARRE 1 sibling, 0 replies; 9+ messages in thread From: Ludovic BARRE @ 2018-12-05 15:49 UTC (permalink / raw) To: Ulf Hansson Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin, Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML, linux-mmc, linux-stm32 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 1/2] mmc: mmci: send stop command if sbc error issue 2018-12-05 14:23 ` Ulf Hansson 2018-12-05 15:49 ` Ludovic BARRE @ 2018-12-05 16:00 ` Ludovic BARRE 1 sibling, 0 replies; 9+ messages in thread From: Ludovic BARRE @ 2018-12-05 16:00 UTC (permalink / raw) To: Ulf Hansson Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin, Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML, linux-mmc, linux-stm32 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 2/2] mmc: mmci: add variant property to send stop cmd if a command fail 2018-11-07 9:30 [PATCH V2 0/2] mmc: mmci: add stop command Ludovic Barre 2018-11-07 9:30 ` [PATCH V2 1/2] mmc: mmci: send stop command if sbc error issue Ludovic Barre @ 2018-11-07 9:30 ` Ludovic Barre 2018-11-21 17:56 ` Ulf Hansson 1 sibling, 1 reply; 9+ messages in thread From: Ludovic Barre @ 2018-11-07 9:30 UTC (permalink / raw) To: Ulf Hansson, Rob Herring Cc: srinivas.kandagatla, Maxime Coquelin, Alexandre Torgue, linux-arm-kernel, linux-kernel, devicetree, linux-mmc, linux-stm32, Ludovic Barre From: Ludovic Barre <ludovic.barre@st.com> The mmc framework follows the requirement of SD_Specification: the STOP_TRANSMISSION is sent on multiple write/read commands and the stop command (alone), not needed on other ADTC commands. But, if an error happens on command or data transmission, some variants require a stop command "STOP_TRANSMISION" to clear the DPSM "Data Path State Machine". If it's not done the next data command freezes hardware block. Needed to support the STM32 sdmmc variant. Signed-off-by: Ludovic Barre <ludovic.barre@st.com> --- drivers/mmc/host/mmci.c | 33 +++++++++++++++++++++++++++++++++ drivers/mmc/host/mmci.h | 4 ++++ 2 files changed, 37 insertions(+) diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index 13fa640..47b865d 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -21,6 +21,7 @@ #include <linux/err.h> #include <linux/highmem.h> #include <linux/log2.h> +#include <linux/mmc/mmc.h> #include <linux/mmc/pm.h> #include <linux/mmc/host.h> #include <linux/mmc/card.h> @@ -57,6 +58,8 @@ void sdmmc_variant_init(struct mmci_host *host); #else static inline void sdmmc_variant_init(struct mmci_host *host) {} #endif +static void +mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c); static unsigned int fmax = 515633; @@ -274,6 +277,7 @@ static struct variant_data variant_stm32_sdmmc = { .cmdreg_lrsp_crc = MCI_CPSM_STM32_LRSP_CRC, .cmdreg_srsp_crc = MCI_CPSM_STM32_SRSP_CRC, .cmdreg_srsp = MCI_CPSM_STM32_SRSP, + .cmdreg_stop = MCI_CPSM_STM32_CMDSTOP, .data_cmd_enable = MCI_CPSM_STM32_CMDTRANS, .irq_pio_mask = MCI_IRQ_PIO_STM32_MASK, .datactrl_first = true, @@ -573,6 +577,24 @@ void mmci_dma_error(struct mmci_host *host) static void mmci_request_end(struct mmci_host *host, struct mmc_request *mrq) { + /* + * If an error happens on command or data transmission, some variants + * require a stop command to reinit the DPSM. + * If it's not done the next data command freeze hardware block. + */ + if (host->variant->cmdreg_stop) { + u32 dpsm; + + dpsm = readl_relaxed(host->base + MMCISTATUS); + dpsm &= MCI_STM32_DPSMACTIVE; + + if (dpsm && ((mrq->cmd && mrq->cmd->error) || + (mrq->data && mrq->data->error))) { + mmci_start_command(host, &host->stop_abort, 0); + return; + } + } + writel(0, host->base + MMCICOMMAND); BUG_ON(host->data); @@ -1100,6 +1122,10 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) mmci_reg_delay(host); } + if (host->variant->cmdreg_stop && + cmd->opcode == MMC_STOP_TRANSMISSION) + c |= host->variant->cmdreg_stop; + c |= cmd->opcode | host->variant->cmdreg_cpsm_enable; if (cmd->flags & MMC_RSP_PRESENT) { if (cmd->flags & MMC_RSP_136) @@ -1950,6 +1976,13 @@ static int mmci_probe(struct amba_device *dev, mmc->max_busy_timeout = 0; } + /* prepare the stop command, used to abort and reinitialized the DPSM */ + if (variant->cmdreg_stop) { + 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 550dd39..35372cd 100644 --- a/drivers/mmc/host/mmci.h +++ b/drivers/mmc/host/mmci.h @@ -161,6 +161,7 @@ #define MCI_ST_CEATAEND (1 << 23) #define MCI_ST_CARDBUSY (1 << 24) /* Extended status bits for the STM32 variants */ +#define MCI_STM32_DPSMACTIVE BIT(12) #define MCI_STM32_BUSYD0 BIT(20) #define MMCICLEAR 0x038 @@ -264,6 +265,7 @@ struct mmci_host; * @cmdreg_lrsp_crc: enable value for long response with crc * @cmdreg_srsp_crc: enable value for short response with crc * @cmdreg_srsp: enable value for short response without crc + * @cmdreg_stop: enable value for stop and abort transmission * @datalength_bits: number of bits in the MMCIDATALENGTH register * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY * is asserted (likewise for RX) @@ -316,6 +318,7 @@ struct variant_data { unsigned int cmdreg_lrsp_crc; unsigned int cmdreg_srsp_crc; unsigned int cmdreg_srsp; + unsigned int cmdreg_stop; unsigned int datalength_bits; unsigned int fifosize; unsigned int fifohalfsize; @@ -375,6 +378,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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2 2/2] mmc: mmci: add variant property to send stop cmd if a command fail 2018-11-07 9:30 ` [PATCH V2 2/2] mmc: mmci: add variant property to send stop cmd if a command fail Ludovic Barre @ 2018-11-21 17:56 ` Ulf Hansson 2018-11-22 10:14 ` Ludovic BARRE 0 siblings, 1 reply; 9+ messages in thread From: Ulf Hansson @ 2018-11-21 17:56 UTC (permalink / raw) To: Ludovic Barre Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin, Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML, linux-mmc, linux-stm32 On 7 November 2018 at 10:30, Ludovic Barre <ludovic.Barre@st.com> wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > The mmc framework follows the requirement of SD_Specification: > the STOP_TRANSMISSION is sent on multiple write/read commands > and the stop command (alone), not needed on other ADTC commands. > > But, if an error happens on command or data transmission, some > variants require a stop command "STOP_TRANSMISION" to clear the DPSM > "Data Path State Machine". If it's not done the next data > command freezes hardware block. > Needed to support the STM32 sdmmc variant. May I suggest some re-wording of this changelog, as to make it more clear of why this is needed. Something along the lines of: "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 data transfer, isn't sufficient for the STM32 sdmmc variant. More precisely, for STM32 sdmmc the DPSM ("Data Path State Machine" ) needs to be cleared by sending a CMD12, also for the so called ADTC commands. For this reason, add a new mmci variant property and let the driver send a CMD12 to complete ADTC commands, in case it's set." > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/mmc/host/mmci.c | 33 +++++++++++++++++++++++++++++++++ > drivers/mmc/host/mmci.h | 4 ++++ > 2 files changed, 37 insertions(+) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index 13fa640..47b865d 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -21,6 +21,7 @@ > #include <linux/err.h> > #include <linux/highmem.h> > #include <linux/log2.h> > +#include <linux/mmc/mmc.h> > #include <linux/mmc/pm.h> > #include <linux/mmc/host.h> > #include <linux/mmc/card.h> > @@ -57,6 +58,8 @@ void sdmmc_variant_init(struct mmci_host *host); > #else > static inline void sdmmc_variant_init(struct mmci_host *host) {} > #endif > +static void > +mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c); > > static unsigned int fmax = 515633; > > @@ -274,6 +277,7 @@ static struct variant_data variant_stm32_sdmmc = { > .cmdreg_lrsp_crc = MCI_CPSM_STM32_LRSP_CRC, > .cmdreg_srsp_crc = MCI_CPSM_STM32_SRSP_CRC, > .cmdreg_srsp = MCI_CPSM_STM32_SRSP, > + .cmdreg_stop = MCI_CPSM_STM32_CMDSTOP, > .data_cmd_enable = MCI_CPSM_STM32_CMDTRANS, > .irq_pio_mask = MCI_IRQ_PIO_STM32_MASK, > .datactrl_first = true, > @@ -573,6 +577,24 @@ void mmci_dma_error(struct mmci_host *host) > static void > mmci_request_end(struct mmci_host *host, struct mmc_request *mrq) > { > + /* > + * If an error happens on command or data transmission, some variants > + * require a stop command to reinit the DPSM. > + * If it's not done the next data command freeze hardware block. > + */ > + if (host->variant->cmdreg_stop) { > + u32 dpsm; > + > + dpsm = readl_relaxed(host->base + MMCISTATUS); > + dpsm &= MCI_STM32_DPSMACTIVE; > + > + if (dpsm && ((mrq->cmd && mrq->cmd->error) || > + (mrq->data && mrq->data->error))) { > + mmci_start_command(host, &host->stop_abort, 0); > + return; > + } I would rather move this code to a separate function. Also, I think you need something else (or additional) than polling the MMCISTATUS register, as in principle you could end up sending CMD12 several times for the same request, which isn't correct. To me the best solution would probably be to make use of the host->data pointer, as it becomes set when DPSM has been enabled. However, host->data is also cleared in mmci_stop_data() which is being called prior mmci_request_end(). In other words, we need to figure under what conditions the new code above should be triggered/called and then also change the conditions for when mmci_request_end() shall be called. In principle look at callers of mmci_request_end() and mmci_stop_data() and update those paths. > + } > + > writel(0, host->base + MMCICOMMAND); > > BUG_ON(host->data); > @@ -1100,6 +1122,10 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) > mmci_reg_delay(host); > } > > + if (host->variant->cmdreg_stop && > + cmd->opcode == MMC_STOP_TRANSMISSION) > + c |= host->variant->cmdreg_stop; > + Hmm. It looks like the above changes, together with the introduction of the variant property, belongs in a separate patch, preceding $subject patch in the series. The reason is, that to me, it makes sense to add the special treatment of the ADTC commands in a separate patch. Can you please split it up and then of course update the change log accordingly!? > c |= cmd->opcode | host->variant->cmdreg_cpsm_enable; > if (cmd->flags & MMC_RSP_PRESENT) { > if (cmd->flags & MMC_RSP_136) > @@ -1950,6 +1976,13 @@ static int mmci_probe(struct amba_device *dev, > mmc->max_busy_timeout = 0; > } > > + /* prepare the stop command, used to abort and reinitialized the DPSM */ > + if (variant->cmdreg_stop) { > + 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 550dd39..35372cd 100644 > --- a/drivers/mmc/host/mmci.h > +++ b/drivers/mmc/host/mmci.h > @@ -161,6 +161,7 @@ > #define MCI_ST_CEATAEND (1 << 23) > #define MCI_ST_CARDBUSY (1 << 24) > /* Extended status bits for the STM32 variants */ > +#define MCI_STM32_DPSMACTIVE BIT(12) > #define MCI_STM32_BUSYD0 BIT(20) > > #define MMCICLEAR 0x038 > @@ -264,6 +265,7 @@ struct mmci_host; > * @cmdreg_lrsp_crc: enable value for long response with crc > * @cmdreg_srsp_crc: enable value for short response with crc > * @cmdreg_srsp: enable value for short response without crc > + * @cmdreg_stop: enable value for stop and abort transmission > * @datalength_bits: number of bits in the MMCIDATALENGTH register > * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY > * is asserted (likewise for RX) > @@ -316,6 +318,7 @@ struct variant_data { > unsigned int cmdreg_lrsp_crc; > unsigned int cmdreg_srsp_crc; > unsigned int cmdreg_srsp; > + unsigned int cmdreg_stop; > unsigned int datalength_bits; > unsigned int fifosize; > unsigned int fifohalfsize; > @@ -375,6 +378,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; > -- > 2.7.4 > Kind regards Uffe ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 2/2] mmc: mmci: add variant property to send stop cmd if a command fail 2018-11-21 17:56 ` Ulf Hansson @ 2018-11-22 10:14 ` Ludovic BARRE 0 siblings, 0 replies; 9+ messages in thread From: Ludovic BARRE @ 2018-11-22 10:14 UTC (permalink / raw) To: Ulf Hansson Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin, Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML, linux-mmc, linux-stm32 hi Ulf due to some ST internal works, I will busy the next days. But I will sent the change as soon as possible. On 11/21/18 6:56 PM, Ulf Hansson wrote: > On 7 November 2018 at 10:30, Ludovic Barre <ludovic.Barre@st.com> wrote: >> From: Ludovic Barre <ludovic.barre@st.com> >> >> The mmc framework follows the requirement of SD_Specification: >> the STOP_TRANSMISSION is sent on multiple write/read commands >> and the stop command (alone), not needed on other ADTC commands. >> >> But, if an error happens on command or data transmission, some >> variants require a stop command "STOP_TRANSMISION" to clear the DPSM >> "Data Path State Machine". If it's not done the next data >> command freezes hardware block. >> Needed to support the STM32 sdmmc variant. > > May I suggest some re-wording of this changelog, as to make it more > clear of why this is needed. Something along the lines of: > > "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 data > transfer, isn't sufficient for the STM32 sdmmc variant. > > More precisely, for STM32 sdmmc the DPSM ("Data Path State Machine" ) > needs to be cleared by sending a CMD12, also for the so called ADTC > commands. For this reason, add a new mmci variant property and let the > driver send a CMD12 to complete ADTC commands, in case it's set." I will change, it's more accurate and concise, thanks for re-wording > >> >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >> --- >> drivers/mmc/host/mmci.c | 33 +++++++++++++++++++++++++++++++++ >> drivers/mmc/host/mmci.h | 4 ++++ >> 2 files changed, 37 insertions(+) >> >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index 13fa640..47b865d 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -21,6 +21,7 @@ >> #include <linux/err.h> >> #include <linux/highmem.h> >> #include <linux/log2.h> >> +#include <linux/mmc/mmc.h> >> #include <linux/mmc/pm.h> >> #include <linux/mmc/host.h> >> #include <linux/mmc/card.h> >> @@ -57,6 +58,8 @@ void sdmmc_variant_init(struct mmci_host *host); >> #else >> static inline void sdmmc_variant_init(struct mmci_host *host) {} >> #endif >> +static void >> +mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c); >> >> static unsigned int fmax = 515633; >> >> @@ -274,6 +277,7 @@ static struct variant_data variant_stm32_sdmmc = { >> .cmdreg_lrsp_crc = MCI_CPSM_STM32_LRSP_CRC, >> .cmdreg_srsp_crc = MCI_CPSM_STM32_SRSP_CRC, >> .cmdreg_srsp = MCI_CPSM_STM32_SRSP, >> + .cmdreg_stop = MCI_CPSM_STM32_CMDSTOP, >> .data_cmd_enable = MCI_CPSM_STM32_CMDTRANS, >> .irq_pio_mask = MCI_IRQ_PIO_STM32_MASK, >> .datactrl_first = true, >> @@ -573,6 +577,24 @@ void mmci_dma_error(struct mmci_host *host) >> static void >> mmci_request_end(struct mmci_host *host, struct mmc_request *mrq) >> { >> + /* >> + * If an error happens on command or data transmission, some variants >> + * require a stop command to reinit the DPSM. >> + * If it's not done the next data command freeze hardware block. >> + */ >> + if (host->variant->cmdreg_stop) { >> + u32 dpsm; >> + >> + dpsm = readl_relaxed(host->base + MMCISTATUS); >> + dpsm &= MCI_STM32_DPSMACTIVE; >> + >> + if (dpsm && ((mrq->cmd && mrq->cmd->error) || >> + (mrq->data && mrq->data->error))) { >> + mmci_start_command(host, &host->stop_abort, 0); >> + return; >> + } > > I would rather move this code to a separate function. OK Also, I think > you need something else (or additional) than polling the MMCISTATUS > register, as in principle you could end up sending CMD12 several times > for the same request, which isn't correct. In mmci_request_end, if the DPSM is still enabled, there was no previous "cmd12" sent. So Normally, the regular and special cmd12 can't be sent for the same request. > > To me the best solution would probably be to make use of the > host->data pointer, as it becomes set when DPSM has been enabled. > However, host->data is also cleared in mmci_stop_data() which is being > called prior mmci_request_end(). In other words, we need to figure > under what conditions the new code above should be triggered/called > and then also change the conditions for when mmci_request_end() shall > be called. > > In principle look at callers of mmci_request_end() and > mmci_stop_data() and update those paths. > >> + } >> + >> writel(0, host->base + MMCICOMMAND); >> >> BUG_ON(host->data); >> @@ -1100,6 +1122,10 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) >> mmci_reg_delay(host); >> } >> >> + if (host->variant->cmdreg_stop && >> + cmd->opcode == MMC_STOP_TRANSMISSION) >> + c |= host->variant->cmdreg_stop; >> + > > Hmm. > > It looks like the above changes, together with the introduction of the > variant property, belongs in a separate patch, preceding $subject > patch in the series. > > The reason is, that to me, it makes sense to add the special treatment > of the ADTC commands in a separate patch. Can you please split it up > and then of course update the change log accordingly!? Ok, I will split this part in separate patch. > >> c |= cmd->opcode | host->variant->cmdreg_cpsm_enable; >> if (cmd->flags & MMC_RSP_PRESENT) { >> if (cmd->flags & MMC_RSP_136) >> @@ -1950,6 +1976,13 @@ static int mmci_probe(struct amba_device *dev, >> mmc->max_busy_timeout = 0; >> } >> >> + /* prepare the stop command, used to abort and reinitialized the DPSM */ >> + if (variant->cmdreg_stop) { >> + 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 550dd39..35372cd 100644 >> --- a/drivers/mmc/host/mmci.h >> +++ b/drivers/mmc/host/mmci.h >> @@ -161,6 +161,7 @@ >> #define MCI_ST_CEATAEND (1 << 23) >> #define MCI_ST_CARDBUSY (1 << 24) >> /* Extended status bits for the STM32 variants */ >> +#define MCI_STM32_DPSMACTIVE BIT(12) >> #define MCI_STM32_BUSYD0 BIT(20) >> >> #define MMCICLEAR 0x038 >> @@ -264,6 +265,7 @@ struct mmci_host; >> * @cmdreg_lrsp_crc: enable value for long response with crc >> * @cmdreg_srsp_crc: enable value for short response with crc >> * @cmdreg_srsp: enable value for short response without crc >> + * @cmdreg_stop: enable value for stop and abort transmission >> * @datalength_bits: number of bits in the MMCIDATALENGTH register >> * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY >> * is asserted (likewise for RX) >> @@ -316,6 +318,7 @@ struct variant_data { >> unsigned int cmdreg_lrsp_crc; >> unsigned int cmdreg_srsp_crc; >> unsigned int cmdreg_srsp; >> + unsigned int cmdreg_stop; >> unsigned int datalength_bits; >> unsigned int fifosize; >> unsigned int fifohalfsize; >> @@ -375,6 +378,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; >> -- >> 2.7.4 >> > > Kind regards > Uffe > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-12-05 16:00 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-07 9:30 [PATCH V2 0/2] mmc: mmci: add stop command Ludovic Barre 2018-11-07 9:30 ` [PATCH V2 1/2] mmc: mmci: send stop command if sbc error issue Ludovic Barre 2018-11-20 9:42 ` Ulf Hansson 2018-12-05 14:23 ` Ulf Hansson 2018-12-05 15:49 ` Ludovic BARRE 2018-12-05 16:00 ` Ludovic BARRE 2018-11-07 9:30 ` [PATCH V2 2/2] mmc: mmci: add variant property to send stop cmd if a command fail Ludovic Barre 2018-11-21 17:56 ` Ulf Hansson 2018-11-22 10:14 ` Ludovic BARRE
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).