[PATCH V3 0/2] mmc: mmci: add stop command

[PATCH V3 0/2] mmc: mmci: add stop command

[Ludovic Barre]

From: Ludovic Barre <ludovic.barre@st.com>

This patch series adds variant property to:
-Set cmdstop bit on STOP_TRANSMISSION command.
-On command or data error, send a stop command.
to clear DPSM if it's yet activated.

change v3:
-Move the cmdstop bit in separate patch.
-Use Ulf re-wording for patch 2/2.
-Move specific part in a separate function.

Ludovic Barre (2):
  mmc: mmci: add variant property to set command stop bit
  mmc: mmci: send stop command to clear the dpsm

 drivers/mmc/host/mmci.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/mmci.h |  4 ++++
 2 files changed, 47 insertions(+)

-- 
2.7.4

[PATCH V3 1/2] mmc: mmci: add variant property to set command stop bit

[Ludovic Barre]

From: Ludovic Barre <ludovic.barre@st.com>

On cmd12 (STOP_TRANSMISSION), STM32 sdmmc variant needs to set
cmdstop bit in command register. The CPSM ("Command Path State Machine")
treats the command as a Stop Transmission command and signals
abort to the DPSM ("Data Path State Machine").

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
 drivers/mmc/host/mmci.c | 6 ++++++
 drivers/mmc/host/mmci.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 13fa640..e352f5a 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>
@@ -274,6 +275,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,
@@ -1100,6 +1102,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)
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 550dd39..2422909 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -264,6 +264,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 +317,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;
-- 
2.7.4

[PATCH V3 2/2] mmc: mmci: send stop command to clear the dpsm

[Ludovic Barre]

From: Ludovic Barre <ludovic.barre@st.com>

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, let the driver send a CMD12 to complete
ADTC commands, in case it's set (depend of cmdreg_stop variant property).

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
 drivers/mmc/host/mmci.c | 37 +++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/mmci.h |  2 ++
 2 files changed, 39 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index e352f5a..4e5643d 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -58,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;
 
@@ -572,9 +574,37 @@ void mmci_dma_error(struct mmci_host *host)
 		host->ops->dma_error(host);
 }
 
+static int mmci_stop_command(struct mmci_host *host, struct mmc_request *mrq)
+{
+	u32 dpsm;
+
+	/*
+	 * If an error happens on command or data transmission
+	 * the DPSM stay enabled. The CPSM required a stop command
+	 * to reinitialize the 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 -EBUSY;
+	}
+
+	return 0;
+}
+
 static void
 mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
 {
+	/*
+	 * For variant with cmdstop bit, a stop command could be needed
+	 * to finish the request.
+	 */
+	if (host->variant->cmdreg_stop && mmci_stop_command(host, mrq))
+		return;
+
 	writel(0, host->base + MMCICOMMAND);
 
 	BUG_ON(host->data);
@@ -1956,6 +1986,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 2422909..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
@@ -377,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

Re: [PATCH V3 1/2] mmc: mmci: add variant property to set command stop bit

[Ulf Hansson]

On Thu, 6 Dec 2018 at 16:13, Ludovic Barre <ludovic.Barre@st.com> wrote:
>
> From: Ludovic Barre <ludovic.barre@st.com>
>
> On cmd12 (STOP_TRANSMISSION), STM32 sdmmc variant needs to set
> cmdstop bit in command register. The CPSM ("Command Path State Machine")
> treats the command as a Stop Transmission command and signals
> abort to the DPSM ("Data Path State Machine").
>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>

Applied for next, thanks!

Withholding patch2 for a while, as I need some more time to review it.

Kind regards
Uffe

> ---
>  drivers/mmc/host/mmci.c | 6 ++++++
>  drivers/mmc/host/mmci.h | 2 ++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 13fa640..e352f5a 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>
> @@ -274,6 +275,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,
> @@ -1100,6 +1102,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)
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index 550dd39..2422909 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -264,6 +264,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 +317,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;
> --
> 2.7.4
>

Re: [PATCH V3 1/2] mmc: mmci: add variant property to set command stop bit

[Ludovic BARRE]

On 12/11/18 10:47 AM, Ulf Hansson wrote:
> On Thu, 6 Dec 2018 at 16:13, Ludovic Barre <ludovic.Barre@st.com> wrote:
>>
>> From: Ludovic Barre <ludovic.barre@st.com>
>>
>> On cmd12 (STOP_TRANSMISSION), STM32 sdmmc variant needs to set
>> cmdstop bit in command register. The CPSM ("Command Path State Machine")
>> treats the command as a Stop Transmission command and signals
>> abort to the DPSM ("Data Path State Machine").
>>
>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> 
> Applied for next, thanks!

thanks

> 
> Withholding patch2 for a while, as I need some more time to review it.

No problem,

Regards
Ludo

> 
> Kind regards
> Uffe
> 
>> ---
>>   drivers/mmc/host/mmci.c | 6 ++++++
>>   drivers/mmc/host/mmci.h | 2 ++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 13fa640..e352f5a 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>
>> @@ -274,6 +275,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,
>> @@ -1100,6 +1102,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)
>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
>> index 550dd39..2422909 100644
>> --- a/drivers/mmc/host/mmci.h
>> +++ b/drivers/mmc/host/mmci.h
>> @@ -264,6 +264,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 +317,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;
>> --
>> 2.7.4
>>

Re: [PATCH V3 1/2] mmc: mmci: add variant property to set command stop bit

[Ludovic BARRE]

hi Ulf

happy new years.

Just a gentleman ping about patch2 of this series
"mmc: mmci: send stop command to clear the dpsm."

Regards
Ludo

On 12/11/18 10:53 AM, Ludovic BARRE wrote:
> 
> 
> On 12/11/18 10:47 AM, Ulf Hansson wrote:
>> On Thu, 6 Dec 2018 at 16:13, Ludovic Barre <ludovic.Barre@st.com> wrote:
>>>
>>> From: Ludovic Barre <ludovic.barre@st.com>
>>>
>>> On cmd12 (STOP_TRANSMISSION), STM32 sdmmc variant needs to set
>>> cmdstop bit in command register. The CPSM ("Command Path State Machine")
>>> treats the command as a Stop Transmission command and signals
>>> abort to the DPSM ("Data Path State Machine").
>>>
>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>>
>> Applied for next, thanks!
> 
> thanks
> 
>>
>> Withholding patch2 for a while, as I need some more time to review it.
> 
> No problem,
> 
> Regards
> Ludo
> 
>>
>> Kind regards
>> Uffe
>>
>>> ---
>>>   drivers/mmc/host/mmci.c | 6 ++++++
>>>   drivers/mmc/host/mmci.h | 2 ++
>>>   2 files changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>> index 13fa640..e352f5a 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>
>>> @@ -274,6 +275,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,
>>> @@ -1100,6 +1102,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)
>>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
>>> index 550dd39..2422909 100644
>>> --- a/drivers/mmc/host/mmci.h
>>> +++ b/drivers/mmc/host/mmci.h
>>> @@ -264,6 +264,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 +317,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;
>>> -- 
>>> 2.7.4
>>>

Re: [Linux-stm32] [PATCH V3 1/2] mmc: mmci: add variant property to set command stop bit

[Ludovic BARRE]

hi Ulf

I don't think you've seen my previous mail :-(
what is your feeling about "mmc: mmci: send stop command to clear the dpsm"

Regards
Ludo

On 1/3/19 11:35 AM, Ludovic BARRE wrote:
> hi Ulf
> 
> happy new years.
> 
> Just a gentleman ping about patch2 of this series
> "mmc: mmci: send stop command to clear the dpsm."
> 
> Regards
> Ludo
> 
> On 12/11/18 10:53 AM, Ludovic BARRE wrote:
>>
>>
>> On 12/11/18 10:47 AM, Ulf Hansson wrote:
>>> On Thu, 6 Dec 2018 at 16:13, Ludovic Barre <ludovic.Barre@st.com> wrote:
>>>>
>>>> From: Ludovic Barre <ludovic.barre@st.com>
>>>>
>>>> On cmd12 (STOP_TRANSMISSION), STM32 sdmmc variant needs to set
>>>> cmdstop bit in command register. The CPSM ("Command Path State 
>>>> Machine")
>>>> treats the command as a Stop Transmission command and signals
>>>> abort to the DPSM ("Data Path State Machine").
>>>>
>>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>>>
>>> Applied for next, thanks!
>>
>> thanks
>>
>>>
>>> Withholding patch2 for a while, as I need some more time to review it.
>>
>> No problem,
>>
>> Regards
>> Ludo
>>
>>>
>>> Kind regards
>>> Uffe
>>>
>>>> ---
>>>>   drivers/mmc/host/mmci.c | 6 ++++++
>>>>   drivers/mmc/host/mmci.h | 2 ++
>>>>   2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>>> index 13fa640..e352f5a 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>
>>>> @@ -274,6 +275,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,
>>>> @@ -1100,6 +1102,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)
>>>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
>>>> index 550dd39..2422909 100644
>>>> --- a/drivers/mmc/host/mmci.h
>>>> +++ b/drivers/mmc/host/mmci.h
>>>> @@ -264,6 +264,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 +317,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;
>>>> -- 
>>>> 2.7.4
>>>>
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32

Re: [Linux-stm32] [PATCH V3 1/2] mmc: mmci: add variant property to set command stop bit

[Ulf Hansson]

On Thu, 24 Jan 2019 at 16:03, Ludovic BARRE <ludovic.barre@st.com> wrote:
>
> hi Ulf
>
> I don't think you've seen my previous mail :-(
> what is your feeling about "mmc: mmci: send stop command to clear the dpsm"

Apologize for the delay. I wanted to check this in detail so I applied
your patch locally and started to play/test it.

However, a couple of other regressions was reported for v5.0 rcs, so I
got sidetracked.

Back on track by now, so I will have look asap. Thanks for pinging me!

Kind regards
Uffe

>
> Regards
> Ludo
>
> On 1/3/19 11:35 AM, Ludovic BARRE wrote:
> > hi Ulf
> >
> > happy new years.
> >
> > Just a gentleman ping about patch2 of this series
> > "mmc: mmci: send stop command to clear the dpsm."
> >
> > Regards
> > Ludo
> >
> > On 12/11/18 10:53 AM, Ludovic BARRE wrote:
> >>
> >>
> >> On 12/11/18 10:47 AM, Ulf Hansson wrote:
> >>> On Thu, 6 Dec 2018 at 16:13, Ludovic Barre <ludovic.Barre@st.com> wrote:
> >>>>
> >>>> From: Ludovic Barre <ludovic.barre@st.com>
> >>>>
> >>>> On cmd12 (STOP_TRANSMISSION), STM32 sdmmc variant needs to set
> >>>> cmdstop bit in command register. The CPSM ("Command Path State
> >>>> Machine")
> >>>> treats the command as a Stop Transmission command and signals
> >>>> abort to the DPSM ("Data Path State Machine").
> >>>>
> >>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> >>>
> >>> Applied for next, thanks!
> >>
> >> thanks
> >>
> >>>
> >>> Withholding patch2 for a while, as I need some more time to review it.
> >>
> >> No problem,
> >>
> >> Regards
> >> Ludo
> >>
> >>>
> >>> Kind regards
> >>> Uffe
> >>>
> >>>> ---
> >>>>   drivers/mmc/host/mmci.c | 6 ++++++
> >>>>   drivers/mmc/host/mmci.h | 2 ++
> >>>>   2 files changed, 8 insertions(+)
> >>>>
> >>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> >>>> index 13fa640..e352f5a 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>
> >>>> @@ -274,6 +275,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,
> >>>> @@ -1100,6 +1102,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)
> >>>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> >>>> index 550dd39..2422909 100644
> >>>> --- a/drivers/mmc/host/mmci.h
> >>>> +++ b/drivers/mmc/host/mmci.h
> >>>> @@ -264,6 +264,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 +317,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;
> >>>> --
> >>>> 2.7.4
> >>>>
> > _______________________________________________
> > Linux-stm32 mailing list
> > Linux-stm32@st-md-mailman.stormreply.com
> > https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32

Re: [PATCH V3 2/2] mmc: mmci: send stop command to clear the dpsm

[Ulf Hansson]

On Thu, 6 Dec 2018 at 16:13, Ludovic Barre <ludovic.Barre@st.com> wrote:
>
> From: Ludovic Barre <ludovic.barre@st.com>
>
> 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, let the driver send a CMD12 to complete
> ADTC commands, in case it's set (depend of cmdreg_stop variant property).
>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> ---
>  drivers/mmc/host/mmci.c | 37 +++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/mmci.h |  2 ++
>  2 files changed, 39 insertions(+)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index e352f5a..4e5643d 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -58,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;
>
> @@ -572,9 +574,37 @@ void mmci_dma_error(struct mmci_host *host)
>                 host->ops->dma_error(host);
>  }
>
> +static int mmci_stop_command(struct mmci_host *host, struct mmc_request *mrq)
> +{
> +       u32 dpsm;
> +
> +       /*
> +        * If an error happens on command or data transmission
> +        * the DPSM stay enabled. The CPSM required a stop command
> +        * to reinitialize the 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 -EBUSY;
> +       }

Unless I have got it wrong, I think there are several problems with
the above code and how you call it.

1) We may be reading the MMCISTATUS register when we don't need to
(because there are no errors).

2) Nothing prevents keep sending the CMD12 over and over again, for
the same request. This could happen, as long as the
MCI_STM32_DPSMACTIVE remains set. I guess it simply shouldn't happen,
but I rather prevent this being possible altogether, as to avoid a
potential hang. It's better to propagate errors.

3) There is a scenario when the DPSM has been enabled, while we fail
with the "sbc" command, this isn't covered, but I think should, right?

4) host->stop_abort.error needs to be reset to zero, in-between
sending the internal CMD12 command. Otherwise, we may end up re-using
an error code from a failed CMD12 command, over and over again.

> +
> +       return 0;
> +}
> +
>  static void
>  mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
>  {
> +       /*
> +        * For variant with cmdstop bit, a stop command could be needed
> +        * to finish the request.
> +        */
> +       if (host->variant->cmdreg_stop && mmci_stop_command(host, mrq))
> +               return;
> +
>         writel(0, host->base + MMCICOMMAND);
>
>         BUG_ON(host->data);
> @@ -1956,6 +1986,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 2422909..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
> @@ -377,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
>

To fix the issues I pointed out above, I decided to try out something
myself. So, I will post a patch in a few minutes, can you please give
it try at your end?

Kind regards
Uffe