From: Ludovic Barre <[email protected]>
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
From: Ludovic Barre <[email protected]>
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 <[email protected]>
---
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
From: Ludovic Barre <[email protected]>
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 <[email protected]>
---
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
On Thu, 6 Dec 2018 at 16:13, Ludovic Barre <[email protected]> wrote:
>
> From: Ludovic Barre <[email protected]>
>
> 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 <[email protected]>
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
>
On 12/11/18 10:47 AM, Ulf Hansson wrote:
> On Thu, 6 Dec 2018 at 16:13, Ludovic Barre <[email protected]> wrote:
>>
>> From: Ludovic Barre <[email protected]>
>>
>> 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 <[email protected]>
>
> 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
>>
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 <[email protected]> wrote:
>>>
>>> From: Ludovic Barre <[email protected]>
>>>
>>> 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 <[email protected]>
>>
>> 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
>>>
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 <[email protected]> wrote:
>>>>
>>>> From: Ludovic Barre <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>
>>> 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
> [email protected]
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
On Thu, 24 Jan 2019 at 16:03, Ludovic BARRE <[email protected]> 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 <[email protected]> wrote:
> >>>>
> >>>> From: Ludovic Barre <[email protected]>
> >>>>
> >>>> 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 <[email protected]>
> >>>
> >>> 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
> > [email protected]
> > https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
On Thu, 6 Dec 2018 at 16:13, Ludovic Barre <[email protected]> wrote:
>
> From: Ludovic Barre <[email protected]>
>
> 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 <[email protected]>
> ---
> 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