[PATCH 0/4] mmc: mmci: add busy detect for stm32 sdmmc variant

[PATCH 0/4] mmc: mmci: add busy detect for stm32 sdmmc variant

[Ludovic Barre]

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

This patch series adds busy detect for stm32 sdmmc variant.
Some adaptations are required:
-Avoid to check and poll busy status when is not expected.
-Clear busy status bit if busy_detect_flag and busy_detect_mask are
 different.
-Add hardware busy timeout with MMCIDATATIMER register.

Ludovic Barre (4):
  mmc: mmci: avoid fake busy polling
  mmc: mmci: fix clear of busy detect status
  mmc: mmci: add hardware busy timeout feature
  mmc: mmci: add busy detect for stm32 sdmmc variant

 drivers/mmc/host/mmci.c | 67 +++++++++++++++++++++++++++++++++++++++----------
 drivers/mmc/host/mmci.h |  3 +++
 2 files changed, 57 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH 1/4] mmc: mmci: avoid fake busy polling

[Ludovic Barre]

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

The busy status bit could occurred even if no busy response is
expected (example cmd11). On sdmmc variant, the busy_detect_flag
reflects inverted value of d0 state, it's sampled at the end of a
CMD response and a second time 2 clk cycles after the CMD response.
To avoid a fake busy, the busy status could be checked and polled
only if the command has RSP_BUSY flag.

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
 drivers/mmc/host/mmci.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 387ff14..4901b73 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 	     unsigned int status)
 {
 	void __iomem *base = host->base;
-	bool sbc;
+	bool sbc, busy_resp;
 
 	if (!cmd)
 		return;
 
 	sbc = (cmd == host->mrq->sbc);
+	busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
 
 	/*
 	 * We need to be one of these interrupts to be considered worth
@@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 	/*
 	 * ST Micro variant: handle busy detection.
 	 */
-	if (host->variant->busy_detect) {
-		bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
+	if (busy_resp && host->variant->busy_detect) {
 
 		/* We are busy with a command, return */
 		if (host->busy_status &&
@@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 		 * that the special busy status bit is still set before
 		 * proceeding.
 		 */
-		if (!host->busy_status && busy_resp &&
+		if (!host->busy_status &&
 		    !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
 		    (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
 
@@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 {
 	struct mmci_host *host = dev_id;
 	u32 status;
+	bool busy_resp;
 	int ret = 0;
 
 	spin_lock(&host->lock);
@@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 		}
 
 		/*
-		 * Don't poll for busy completion in irq context.
+		 * Don't poll for:
+		 * -busy completion in irq context.
+		 * -no busy response expected.
 		 */
-		if (host->variant->busy_detect && host->busy_status)
+		busy_resp = host->cmd ?
+			!!(host->cmd->flags & MMC_RSP_BUSY) : false;
+
+		if (host->variant->busy_detect &&
+		    (!busy_resp || host->busy_status))
 			status &= ~host->variant->busy_detect_flag;
 
 		ret = 1;
-- 
2.7.4

[PATCH 2/4] mmc: mmci: fix clear of busy detect status

[Ludovic Barre]

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

The "busy_detect_flag" is used to read/clear busy value of
mmci status. The "busy_detect_mask" is used to manage busy irq of
mmci mask.
For sdmmc variant, the 2 properties have not the same offset.
To clear the busyd0 status bit, we must add busy detect flag,
the mmci mask is not enough.

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
 drivers/mmc/host/mmci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 4901b73..3fa4386 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1533,7 +1533,8 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 		 * to make sure that both start and end interrupts are always
 		 * cleared one after the other.
 		 */
-		status &= readl(host->base + MMCIMASK0);
+		status &= readl(host->base + MMCIMASK0) |
+			host->variant->busy_detect_flag;
 		if (host->variant->busy_detect)
 			writel(status & ~host->variant->busy_detect_mask,
 			       host->base + MMCICLEAR);
-- 
2.7.4

[PATCH 3/4] mmc: mmci: add hardware busy timeout feature

[Ludovic Barre]

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

In some variants, the data timer is enabled when the DPSM is in
busy state (while data transfert or MMC_RSP_BUSY), and could
generate a data timeout error if the counter reach 0.

-Define max_busy_timeout (in ms) according to clock.
-Set data timer register if the command has rsp_busy flag.
 If busy_timeout is not defined by framework, the busy
 length after Data Burst is defined as 1 second
 (refer: 4.6.2.2 Write of sd specification part1 v6-0).
-Add MCI_DATATIMEOUT error management in mmci_cmd_irq.

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

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 3fa4386..d1b7563 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1093,6 +1093,7 @@ static void
 mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
 {
 	void __iomem *base = host->base;
+	unsigned long long clks = 0;
 
 	dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n",
 	    cmd->opcode, cmd->arg, cmd->flags);
@@ -1115,6 +1116,19 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
 		else
 			c |= host->variant->cmdreg_srsp;
 	}
+
+	if (host->variant->busy_timeout && !cmd->data) {
+		if (cmd->flags & MMC_RSP_BUSY) {
+			if (!cmd->busy_timeout)
+				cmd->busy_timeout = 1000;
+
+			clks = (unsigned long long)cmd->busy_timeout;
+			clks *=	host->cclk;
+			do_div(clks, MSEC_PER_SEC);
+		}
+		writel_relaxed(clks, host->base + MMCIDATATIMER);
+	}
+
 	if (/*interrupt*/0)
 		c |= MCI_CPSM_INTERRUPT;
 
@@ -1221,6 +1235,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 {
 	void __iomem *base = host->base;
 	bool sbc, busy_resp;
+	u32 err_msk;
 
 	if (!cmd)
 		return;
@@ -1233,8 +1248,12 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 	 * handling. Note that we tag on any latent IRQs postponed
 	 * due to waiting for busy status.
 	 */
-	if (!((status|host->busy_status) &
-	      (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND)))
+	err_msk = MCI_CMDCRCFAIL | MCI_CMDTIMEOUT;
+	if (host->variant->busy_timeout && busy_resp)
+		err_msk |= MCI_DATATIMEOUT;
+
+	if (!((status | host->busy_status) &
+	      (err_msk | MCI_CMDSENT | MCI_CMDRESPEND)))
 		return;
 
 	/*
@@ -1243,7 +1262,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 	if (busy_resp && host->variant->busy_detect) {
 
 		/* We are busy with a command, return */
-		if (host->busy_status &&
+		if (host->busy_status && !(status & (err_msk)) &&
 		    (status & host->variant->busy_detect_flag))
 			return;
 
@@ -1253,9 +1272,9 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 		 * that the special busy status bit is still set before
 		 * proceeding.
 		 */
-		if (!host->busy_status &&
-		    !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
-		    (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
+		if (!host->busy_status && !(status & (err_msk)) &&
+		    (readl(base + MMCISTATUS) &
+		     host->variant->busy_detect_flag)) {
 
 			/* Clear the busy start IRQ */
 			writel(host->variant->busy_detect_mask,
@@ -1297,6 +1316,9 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 		cmd->error = -ETIMEDOUT;
 	} else if (status & MCI_CMDCRCFAIL && cmd->flags & MMC_RSP_CRC) {
 		cmd->error = -EILSEQ;
+	} else if (host->variant->busy_timeout && busy_resp &&
+		   status & MCI_DATATIMEOUT) {
+		cmd->error = -ETIMEDOUT;
 	} else {
 		cmd->resp[0] = readl(base + MMCIRESPONSE0);
 		cmd->resp[1] = readl(base + MMCIRESPONSE1);
@@ -1564,6 +1586,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 			status &= ~host->variant->busy_detect_flag;
 
 		ret = 1;
+
 	} while (status);
 
 	spin_unlock(&host->lock);
@@ -1968,6 +1991,8 @@ static int mmci_probe(struct amba_device *dev,
 	 * Enable busy detection.
 	 */
 	if (variant->busy_detect) {
+		u32 max_busy_timeout = 0;
+
 		mmci_ops.card_busy = mmci_card_busy;
 		/*
 		 * Not all variants have a flag to enable busy detection
@@ -1977,7 +2002,11 @@ static int mmci_probe(struct amba_device *dev,
 			mmci_write_datactrlreg(host,
 					       host->variant->busy_dpsm_flag);
 		mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
-		mmc->max_busy_timeout = 0;
+
+		if (variant->busy_timeout)
+			max_busy_timeout = ~0UL / (mmc->f_max / MSEC_PER_SEC);
+
+		mmc->max_busy_timeout = max_busy_timeout;
 	}
 
 	/* Prepare a CMD12 - needed to clear the DPSM on some variants. */
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 14df810..7d9eb92 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -289,6 +289,7 @@ struct mmci_host;
  * @signal_direction: input/out direction of bus signals can be indicated
  * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock
  * @busy_detect: true if the variant supports busy detection on DAT0.
+ * @busy_timeout: true if the variant supports hardware busy timeout on R1B.
  * @busy_dpsm_flag: bitmask enabling busy detection in the DPSM
  * @busy_detect_flag: bitmask identifying the bit in the MMCISTATUS register
  *		      indicating that the card is busy
@@ -338,6 +339,7 @@ struct variant_data {
 	u8			signal_direction:1;
 	u8			pwrreg_clkgate:1;
 	u8			busy_detect:1;
+	u8			busy_timeout:1;
 	u32			busy_dpsm_flag;
 	u32			busy_detect_flag;
 	u32			busy_detect_mask;
-- 
2.7.4

[PATCH 4/4] mmc: mmci: add busy detect for stm32 sdmmc variant

[Ludovic Barre]

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

This patch enables busy detection for stm32 sdmmc which requires
to set data timer to define the busy timeout.
sdmmc has 2 flags:
-busyd0: inverted value of d0 line.
-busyd0end which indicates only end of busy following a cmd response.
Only one interrupt on busyd0end.

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

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index d1b7563..1011c99 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -283,6 +283,10 @@ static struct variant_data variant_stm32_sdmmc = {
 	.datalength_bits	= 25,
 	.datactrl_blocksz	= 14,
 	.stm32_idmabsize_mask	= GENMASK(12, 5),
+	.busy_detect		= true,
+	.busy_timeout		= true,
+	.busy_detect_flag	= MCI_STM32_BUSYD0,
+	.busy_detect_mask	= MCI_STM32_BUSYD0ENDMASK,
 	.init			= sdmmc_variant_init,
 };
 
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 7d9eb92..9179a6c 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -162,6 +162,7 @@
 #define MCI_ST_CARDBUSY		(1 << 24)
 /* Extended status bits for the STM32 variants */
 #define MCI_STM32_BUSYD0	BIT(20)
+#define MCI_STM32_BUSYD0END	BIT(21)
 
 #define MMCICLEAR		0x038
 #define MCI_CMDCRCFAILCLR	(1 << 0)
-- 
2.7.4

Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling

[Ulf Hansson]

On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote:
>
> From: Ludovic Barre <ludovic.barre@st.com>
>
> The busy status bit could occurred even if no busy response is
> expected (example cmd11). On sdmmc variant, the busy_detect_flag
> reflects inverted value of d0 state, it's sampled at the end of a
> CMD response and a second time 2 clk cycles after the CMD response.
> To avoid a fake busy, the busy status could be checked and polled
> only if the command has RSP_BUSY flag.
>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>

Before I review this, can you tell what HW you have tested this on?

Kind regards
Uffe

> ---
>  drivers/mmc/host/mmci.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 387ff14..4901b73 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>              unsigned int status)
>  {
>         void __iomem *base = host->base;
> -       bool sbc;
> +       bool sbc, busy_resp;
>
>         if (!cmd)
>                 return;
>
>         sbc = (cmd == host->mrq->sbc);
> +       busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
>
>         /*
>          * We need to be one of these interrupts to be considered worth
> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>         /*
>          * ST Micro variant: handle busy detection.
>          */
> -       if (host->variant->busy_detect) {
> -               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
> +       if (busy_resp && host->variant->busy_detect) {
>
>                 /* We are busy with a command, return */
>                 if (host->busy_status &&
> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>                  * that the special busy status bit is still set before
>                  * proceeding.
>                  */
> -               if (!host->busy_status && busy_resp &&
> +               if (!host->busy_status &&
>                     !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
>                     (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
>
> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>  {
>         struct mmci_host *host = dev_id;
>         u32 status;
> +       bool busy_resp;
>         int ret = 0;
>
>         spin_lock(&host->lock);
> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>                 }
>
>                 /*
> -                * Don't poll for busy completion in irq context.
> +                * Don't poll for:
> +                * -busy completion in irq context.
> +                * -no busy response expected.
>                  */
> -               if (host->variant->busy_detect && host->busy_status)
> +               busy_resp = host->cmd ?
> +                       !!(host->cmd->flags & MMC_RSP_BUSY) : false;
> +
> +               if (host->variant->busy_detect &&
> +                   (!busy_resp || host->busy_status))
>                         status &= ~host->variant->busy_detect_flag;
>
>                 ret = 1;
> --
> 2.7.4
>

Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling

[Ludovic BARRE]

Hi Ulf

On 3/6/19 10:00 AM, Ulf Hansson wrote:
> On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote:
>>
>> From: Ludovic Barre <ludovic.barre@st.com>
>>
>> The busy status bit could occurred even if no busy response is
>> expected (example cmd11). On sdmmc variant, the busy_detect_flag
>> reflects inverted value of d0 state, it's sampled at the end of a
>> CMD response and a second time 2 clk cycles after the CMD response.
>> To avoid a fake busy, the busy status could be checked and polled
>> only if the command has RSP_BUSY flag.
>>
>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> 
> Before I review this, can you tell what HW you have tested this on?

I tested on stm32mp157c (stm32_sdmmc variant)

> 
> Kind regards
> Uffe
> 
>> ---
>>   drivers/mmc/host/mmci.c | 19 +++++++++++++------
>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 387ff14..4901b73 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>               unsigned int status)
>>   {
>>          void __iomem *base = host->base;
>> -       bool sbc;
>> +       bool sbc, busy_resp;
>>
>>          if (!cmd)
>>                  return;
>>
>>          sbc = (cmd == host->mrq->sbc);
>> +       busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
>>
>>          /*
>>           * We need to be one of these interrupts to be considered worth
>> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>          /*
>>           * ST Micro variant: handle busy detection.
>>           */
>> -       if (host->variant->busy_detect) {
>> -               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
>> +       if (busy_resp && host->variant->busy_detect) {
>>
>>                  /* We are busy with a command, return */
>>                  if (host->busy_status &&
>> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>                   * that the special busy status bit is still set before
>>                   * proceeding.
>>                   */
>> -               if (!host->busy_status && busy_resp &&
>> +               if (!host->busy_status &&
>>                      !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
>>                      (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
>>
>> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>   {
>>          struct mmci_host *host = dev_id;
>>          u32 status;
>> +       bool busy_resp;
>>          int ret = 0;
>>
>>          spin_lock(&host->lock);
>> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>                  }
>>
>>                  /*
>> -                * Don't poll for busy completion in irq context.
>> +                * Don't poll for:
>> +                * -busy completion in irq context.
>> +                * -no busy response expected.
>>                   */
>> -               if (host->variant->busy_detect && host->busy_status)
>> +               busy_resp = host->cmd ?
>> +                       !!(host->cmd->flags & MMC_RSP_BUSY) : false;
>> +
>> +               if (host->variant->busy_detect &&
>> +                   (!busy_resp || host->busy_status))
>>                          status &= ~host->variant->busy_detect_flag;
>>
>>                  ret = 1;
>> --
>> 2.7.4
>>

Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling

[Ulf Hansson]

On Wed, 6 Mar 2019 at 10:04, Ludovic BARRE <ludovic.barre@st.com> wrote:
>
> Hi Ulf
>
> On 3/6/19 10:00 AM, Ulf Hansson wrote:
> > On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote:
> >>
> >> From: Ludovic Barre <ludovic.barre@st.com>
> >>
> >> The busy status bit could occurred even if no busy response is
> >> expected (example cmd11). On sdmmc variant, the busy_detect_flag
> >> reflects inverted value of d0 state, it's sampled at the end of a
> >> CMD response and a second time 2 clk cycles after the CMD response.
> >> To avoid a fake busy, the busy status could be checked and polled
> >> only if the command has RSP_BUSY flag.
> >>
> >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> >
> > Before I review this, can you tell what HW you have tested this on?
>
> I tested on stm32mp157c (stm32_sdmmc variant)

Okay, I see. So we need to get this tested for the ux500v2 variant as
well. I try to get some time to do that, soon.

However it seems like you could benefit from having one of those
boards yourself. It would speed up the process, as you wouldn't have
to rely on me doing the test. :-) Is there a chance of you could dig
up some of these old boards from somewhere?

Kind regards
Uffe

>
> >
> > Kind regards
> > Uffe
> >
> >> ---
> >>   drivers/mmc/host/mmci.c | 19 +++++++++++++------
> >>   1 file changed, 13 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> >> index 387ff14..4901b73 100644
> >> --- a/drivers/mmc/host/mmci.c
> >> +++ b/drivers/mmc/host/mmci.c
> >> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> >>               unsigned int status)
> >>   {
> >>          void __iomem *base = host->base;
> >> -       bool sbc;
> >> +       bool sbc, busy_resp;
> >>
> >>          if (!cmd)
> >>                  return;
> >>
> >>          sbc = (cmd == host->mrq->sbc);
> >> +       busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
> >>
> >>          /*
> >>           * We need to be one of these interrupts to be considered worth
> >> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> >>          /*
> >>           * ST Micro variant: handle busy detection.
> >>           */
> >> -       if (host->variant->busy_detect) {
> >> -               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
> >> +       if (busy_resp && host->variant->busy_detect) {
> >>
> >>                  /* We are busy with a command, return */
> >>                  if (host->busy_status &&
> >> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> >>                   * that the special busy status bit is still set before
> >>                   * proceeding.
> >>                   */
> >> -               if (!host->busy_status && busy_resp &&
> >> +               if (!host->busy_status &&
> >>                      !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
> >>                      (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> >>
> >> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> >>   {
> >>          struct mmci_host *host = dev_id;
> >>          u32 status;
> >> +       bool busy_resp;
> >>          int ret = 0;
> >>
> >>          spin_lock(&host->lock);
> >> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> >>                  }
> >>
> >>                  /*
> >> -                * Don't poll for busy completion in irq context.
> >> +                * Don't poll for:
> >> +                * -busy completion in irq context.
> >> +                * -no busy response expected.
> >>                   */
> >> -               if (host->variant->busy_detect && host->busy_status)
> >> +               busy_resp = host->cmd ?
> >> +                       !!(host->cmd->flags & MMC_RSP_BUSY) : false;
> >> +
> >> +               if (host->variant->busy_detect &&
> >> +                   (!busy_resp || host->busy_status))
> >>                          status &= ~host->variant->busy_detect_flag;
> >>
> >>                  ret = 1;
> >> --
> >> 2.7.4
> >>

Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling

[Ludovic BARRE]

Hi Ulf

I could have a snowball board, I don't remember if it a ux500v1 or v2?
This board has been removed of u-boot mainline, do you have a specific
repository for snowball or what branch do you advise?
do you have a snowball wiki (up-to-date) for flashing...

BR
Ludo
On 3/6/19 10:49 AM, Ulf Hansson wrote:
> On Wed, 6 Mar 2019 at 10:04, Ludovic BARRE <ludovic.barre@st.com> wrote:
>>
>> Hi Ulf
>>
>> On 3/6/19 10:00 AM, Ulf Hansson wrote:
>>> On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote:
>>>>
>>>> From: Ludovic Barre <ludovic.barre@st.com>
>>>>
>>>> The busy status bit could occurred even if no busy response is
>>>> expected (example cmd11). On sdmmc variant, the busy_detect_flag
>>>> reflects inverted value of d0 state, it's sampled at the end of a
>>>> CMD response and a second time 2 clk cycles after the CMD response.
>>>> To avoid a fake busy, the busy status could be checked and polled
>>>> only if the command has RSP_BUSY flag.
>>>>
>>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>>>
>>> Before I review this, can you tell what HW you have tested this on?
>>
>> I tested on stm32mp157c (stm32_sdmmc variant)
> 
> Okay, I see. So we need to get this tested for the ux500v2 variant as
> well. I try to get some time to do that, soon.
> 
> However it seems like you could benefit from having one of those
> boards yourself. It would speed up the process, as you wouldn't have
> to rely on me doing the test. :-) Is there a chance of you could dig
> up some of these old boards from somewhere?
> 
> Kind regards
> Uffe
> 
>>
>>>
>>> Kind regards
>>> Uffe
>>>
>>>> ---
>>>>    drivers/mmc/host/mmci.c | 19 +++++++++++++------
>>>>    1 file changed, 13 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>>> index 387ff14..4901b73 100644
>>>> --- a/drivers/mmc/host/mmci.c
>>>> +++ b/drivers/mmc/host/mmci.c
>>>> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>>>                unsigned int status)
>>>>    {
>>>>           void __iomem *base = host->base;
>>>> -       bool sbc;
>>>> +       bool sbc, busy_resp;
>>>>
>>>>           if (!cmd)
>>>>                   return;
>>>>
>>>>           sbc = (cmd == host->mrq->sbc);
>>>> +       busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
>>>>
>>>>           /*
>>>>            * We need to be one of these interrupts to be considered worth
>>>> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>>>           /*
>>>>            * ST Micro variant: handle busy detection.
>>>>            */
>>>> -       if (host->variant->busy_detect) {
>>>> -               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
>>>> +       if (busy_resp && host->variant->busy_detect) {
>>>>
>>>>                   /* We are busy with a command, return */
>>>>                   if (host->busy_status &&
>>>> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>>>                    * that the special busy status bit is still set before
>>>>                    * proceeding.
>>>>                    */
>>>> -               if (!host->busy_status && busy_resp &&
>>>> +               if (!host->busy_status &&
>>>>                       !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
>>>>                       (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
>>>>
>>>> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>>>    {
>>>>           struct mmci_host *host = dev_id;
>>>>           u32 status;
>>>> +       bool busy_resp;
>>>>           int ret = 0;
>>>>
>>>>           spin_lock(&host->lock);
>>>> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>>>                   }
>>>>
>>>>                   /*
>>>> -                * Don't poll for busy completion in irq context.
>>>> +                * Don't poll for:
>>>> +                * -busy completion in irq context.
>>>> +                * -no busy response expected.
>>>>                    */
>>>> -               if (host->variant->busy_detect && host->busy_status)
>>>> +               busy_resp = host->cmd ?
>>>> +                       !!(host->cmd->flags & MMC_RSP_BUSY) : false;
>>>> +
>>>> +               if (host->variant->busy_detect &&
>>>> +                   (!busy_resp || host->busy_status))
>>>>                           status &= ~host->variant->busy_detect_flag;
>>>>
>>>>                   ret = 1;
>>>> --
>>>> 2.7.4
>>>>

Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling

[Linus Walleij]

On Wed, Mar 6, 2019 at 11:09 AM Ludovic BARRE <ludovic.barre@st.com> wrote:

> I could have a snowball board, I don't remember if it a ux500v1 or v2?

I think they are all v2.

> This board has been removed of u-boot mainline, do you have a specific
> repository for snowball or what branch do you advise?
> do you have a snowball wiki (up-to-date) for flashing...

I have some information here:
https://dflund.se/~triad/krad/ux500/

I'd be surprised if you don't have U-boot or fastboot on the board
already.

Yours,
Linus Walleij

Re: [PATCH 0/4] mmc: mmci: add busy detect for stm32 sdmmc variant

[Ludovic BARRE]

Hi Ulf

Just a gentleman ping about this series.
I sent this series at same time of dt_mode
(no dependence between both).

BR
Ludo

On 3/5/19 5:10 PM, Ludovic Barre wrote:
> From: Ludovic Barre <ludovic.barre@st.com>
> 
> This patch series adds busy detect for stm32 sdmmc variant.
> Some adaptations are required:
> -Avoid to check and poll busy status when is not expected.
> -Clear busy status bit if busy_detect_flag and busy_detect_mask are
>   different.
> -Add hardware busy timeout with MMCIDATATIMER register.
> 
> Ludovic Barre (4):
>    mmc: mmci: avoid fake busy polling
>    mmc: mmci: fix clear of busy detect status
>    mmc: mmci: add hardware busy timeout feature
>    mmc: mmci: add busy detect for stm32 sdmmc variant
> 
>   drivers/mmc/host/mmci.c | 67 +++++++++++++++++++++++++++++++++++++++----------
>   drivers/mmc/host/mmci.h |  3 +++
>   2 files changed, 57 insertions(+), 13 deletions(-)
> 

Re: [PATCH 0/4] mmc: mmci: add busy detect for stm32 sdmmc variant

[Ulf Hansson]

On Thu, 11 Apr 2019 at 14:37, Ludovic BARRE <ludovic.barre@st.com> wrote:
>
> Hi Ulf
>
> Just a gentleman ping about this series.
> I sent this series at same time of dt_mode
> (no dependence between both).

Thanks for pinging.

It's been a busy period, with travels etc. I am just catching up on
everything and I will soon come back to this.

I haven't tried to apply this on top of the other recent queued
changes for mmci, but perhaps you can check if a rebase is needed?
Especially since I plan to run this on my ux500 board.

Kind regards
Uffe

>
> BR
> Ludo
>
> On 3/5/19 5:10 PM, Ludovic Barre wrote:
> > From: Ludovic Barre <ludovic.barre@st.com>
> >
> > This patch series adds busy detect for stm32 sdmmc variant.
> > Some adaptations are required:
> > -Avoid to check and poll busy status when is not expected.
> > -Clear busy status bit if busy_detect_flag and busy_detect_mask are
> >   different.
> > -Add hardware busy timeout with MMCIDATATIMER register.
> >
> > Ludovic Barre (4):
> >    mmc: mmci: avoid fake busy polling
> >    mmc: mmci: fix clear of busy detect status
> >    mmc: mmci: add hardware busy timeout feature
> >    mmc: mmci: add busy detect for stm32 sdmmc variant
> >
> >   drivers/mmc/host/mmci.c | 67 +++++++++++++++++++++++++++++++++++++++----------
> >   drivers/mmc/host/mmci.h |  3 +++
> >   2 files changed, 57 insertions(+), 13 deletions(-)
> >

Re: [PATCH 0/4] mmc: mmci: add busy detect for stm32 sdmmc variant

[Ludovic BARRE]

hi Ulf

On 4/11/19 3:29 PM, Ulf Hansson wrote:
> On Thu, 11 Apr 2019 at 14:37, Ludovic BARRE <ludovic.barre@st.com> wrote:
>>
>> Hi Ulf
>>
>> Just a gentleman ping about this series.
>> I sent this series at same time of dt_mode
>> (no dependence between both).
> 
> Thanks for pinging.
> 
> It's been a busy period, with travels etc. I am just catching up on
> everything and I will soon come back to this.

No problem, I saw lot of power group presentations at linaro connect :-)

> 
> I haven't tried to apply this on top of the other recent queued
> changes for mmci, but perhaps you can check if a rebase is needed?

I try on the last mmc next, rebase, build and stm32mp157 test OK

> Especially since I plan to run this on my ux500 board.
> 
> Kind regards
> Uffe
> 
>>
>> BR
>> Ludo
>>
>> On 3/5/19 5:10 PM, Ludovic Barre wrote:
>>> From: Ludovic Barre <ludovic.barre@st.com>
>>>
>>> This patch series adds busy detect for stm32 sdmmc variant.
>>> Some adaptations are required:
>>> -Avoid to check and poll busy status when is not expected.
>>> -Clear busy status bit if busy_detect_flag and busy_detect_mask are
>>>    different.
>>> -Add hardware busy timeout with MMCIDATATIMER register.
>>>
>>> Ludovic Barre (4):
>>>     mmc: mmci: avoid fake busy polling
>>>     mmc: mmci: fix clear of busy detect status
>>>     mmc: mmci: add hardware busy timeout feature
>>>     mmc: mmci: add busy detect for stm32 sdmmc variant
>>>
>>>    drivers/mmc/host/mmci.c | 67 +++++++++++++++++++++++++++++++++++++++----------
>>>    drivers/mmc/host/mmci.h |  3 +++
>>>    2 files changed, 57 insertions(+), 13 deletions(-)
>>>

Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling

[Ulf Hansson]

On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote:
>
> From: Ludovic Barre <ludovic.barre@st.com>
>
> The busy status bit could occurred even if no busy response is
> expected (example cmd11). On sdmmc variant, the busy_detect_flag
> reflects inverted value of d0 state, it's sampled at the end of a
> CMD response and a second time 2 clk cycles after the CMD response.
> To avoid a fake busy, the busy status could be checked and polled
> only if the command has RSP_BUSY flag.

I would appreciate a better explanation of what this patch really changes.

The above is giving some background to the behavior of sdmmc variant,
but at this point that variant doesn't even have the
->variant->busy_detect flag set.

>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> ---
>  drivers/mmc/host/mmci.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 387ff14..4901b73 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>              unsigned int status)
>  {
>         void __iomem *base = host->base;
> -       bool sbc;
> +       bool sbc, busy_resp;
>
>         if (!cmd)
>                 return;
>
>         sbc = (cmd == host->mrq->sbc);
> +       busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
>
>         /*
>          * We need to be one of these interrupts to be considered worth
> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>         /*
>          * ST Micro variant: handle busy detection.
>          */
> -       if (host->variant->busy_detect) {
> -               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
> +       if (busy_resp && host->variant->busy_detect) {
>
>                 /* We are busy with a command, return */
>                 if (host->busy_status &&
> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>                  * that the special busy status bit is still set before
>                  * proceeding.
>                  */
> -               if (!host->busy_status && busy_resp &&
> +               if (!host->busy_status &&
>                     !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
>                     (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {

All the changes above makes perfect sense to me, but looks more like a
cleanup of the code, rather than actually changing the behavior.

>
> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>  {
>         struct mmci_host *host = dev_id;
>         u32 status;
> +       bool busy_resp;
>         int ret = 0;
>
>         spin_lock(&host->lock);
> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>                 }
>
>                 /*
> -                * Don't poll for busy completion in irq context.
> +                * Don't poll for:
> +                * -busy completion in irq context.
> +                * -no busy response expected.
>                  */
> -               if (host->variant->busy_detect && host->busy_status)
> +               busy_resp = host->cmd ?
> +                       !!(host->cmd->flags & MMC_RSP_BUSY) : false;

This doesn't make sense to me, but I may be missing something.

host->busy_status is being updated by mmci_cmd_irq() and only when
MMC_RSP_BUSY is set for the command in flight. In other words,
checking for MMC_RSP_BUSY here as well is redundant. No?

> +
> +               if (host->variant->busy_detect &&
> +                   (!busy_resp || host->busy_status))
>                         status &= ~host->variant->busy_detect_flag;
>
>                 ret = 1;
> --
> 2.7.4
>

Kind regards
Uffe

Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling

[Ludovic BARRE]

hi Ulf

On 4/23/19 3:39 PM, Ulf Hansson wrote:
> On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote:
>>
>> From: Ludovic Barre <ludovic.barre@st.com>
>>
>> The busy status bit could occurred even if no busy response is
>> expected (example cmd11). On sdmmc variant, the busy_detect_flag
>> reflects inverted value of d0 state, it's sampled at the end of a
>> CMD response and a second time 2 clk cycles after the CMD response.
>> To avoid a fake busy, the busy status could be checked and polled
>> only if the command has RSP_BUSY flag.
> 
> I would appreciate a better explanation of what this patch really changes.
> 
> The above is giving some background to the behavior of sdmmc variant,
> but at this point that variant doesn't even have the
> ->variant->busy_detect flag set.
> 

Yes, I will try to explain more and focus on common behavior.

>>
>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>> ---
>>   drivers/mmc/host/mmci.c | 19 +++++++++++++------
>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 387ff14..4901b73 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>               unsigned int status)
>>   {
>>          void __iomem *base = host->base;
>> -       bool sbc;
>> +       bool sbc, busy_resp;
>>
>>          if (!cmd)
>>                  return;
>>
>>          sbc = (cmd == host->mrq->sbc);
>> +       busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
>>
>>          /*
>>           * We need to be one of these interrupts to be considered worth
>> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>          /*
>>           * ST Micro variant: handle busy detection.
>>           */
>> -       if (host->variant->busy_detect) {
>> -               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
>> +       if (busy_resp && host->variant->busy_detect) {
>>
>>                  /* We are busy with a command, return */
>>                  if (host->busy_status &&
>> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>                   * that the special busy status bit is still set before
>>                   * proceeding.
>>                   */
>> -               if (!host->busy_status && busy_resp &&
>> +               if (!host->busy_status &&
>>                      !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
>>                      (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> 
> All the changes above makes perfect sense to me, but looks more like a
> cleanup of the code, rather than actually changing the behavior.

yes, few changing (this just avoid to enter in
"if (host->variant->busy_detect)") at each time.
I could move this part in cleanup patch (before this patch)

> 
>>
>> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>   {
>>          struct mmci_host *host = dev_id;
>>          u32 status;
>> +       bool busy_resp;
>>          int ret = 0;
>>
>>          spin_lock(&host->lock);
>> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>                  }
>>
>>                  /*
>> -                * Don't poll for busy completion in irq context.
>> +                * Don't poll for:
>> +                * -busy completion in irq context.
>> +                * -no busy response expected.
>>                   */
>> -               if (host->variant->busy_detect && host->busy_status)
>> +               busy_resp = host->cmd ?
>> +                       !!(host->cmd->flags & MMC_RSP_BUSY) : false;
> 
> This doesn't make sense to me, but I may be missing something.
> 
> host->busy_status is being updated by mmci_cmd_irq() and only when
> MMC_RSP_BUSY is set for the command in flight. In other words,
> checking for MMC_RSP_BUSY here as well is redundant. No?

In mmci_irq the "do while" loops until the status is totally cleared.

Today (for variant with busy_detect option), the status busy_detect_flag
is excluded only while busy_status period (command with MMC_RSP_BUSY and 
while busy line is low => "busy_status=1")

On SDMMC variant I noticed that busy_detect_flag status could be enabled
even if the command is not MMC_RSP_BUSY, for example sdmmc variant stay
in loop while cmd11 voltage switch.

So I wish extend host->variant->busy_detect_flag exclusion for all
commands which is not a MMC_RSP_BUSY. I suppose that other variants
could have the same behavior, and else there is no impact, normally.

> 
>> +
>> +               if (host->variant->busy_detect &&
>> +                   (!busy_resp || host->busy_status))
>>                          status &= ~host->variant->busy_detect_flag;
>>
>>                  ret = 1;
>> --
>> 2.7.4
>>
> 
> Kind regards
> Uffe
> 

Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling

[Ulf Hansson]

On Thu, 25 Apr 2019 at 11:22, Ludovic BARRE <ludovic.barre@st.com> wrote:
>
> hi Ulf
>
> On 4/23/19 3:39 PM, Ulf Hansson wrote:
> > On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote:
> >>
> >> From: Ludovic Barre <ludovic.barre@st.com>
> >>
> >> The busy status bit could occurred even if no busy response is
> >> expected (example cmd11). On sdmmc variant, the busy_detect_flag
> >> reflects inverted value of d0 state, it's sampled at the end of a
> >> CMD response and a second time 2 clk cycles after the CMD response.
> >> To avoid a fake busy, the busy status could be checked and polled
> >> only if the command has RSP_BUSY flag.
> >
> > I would appreciate a better explanation of what this patch really changes.
> >
> > The above is giving some background to the behavior of sdmmc variant,
> > but at this point that variant doesn't even have the
> > ->variant->busy_detect flag set.
> >
>
> Yes, I will try to explain more and focus on common behavior.
>
> >>
> >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> >> ---
> >>   drivers/mmc/host/mmci.c | 19 +++++++++++++------
> >>   1 file changed, 13 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> >> index 387ff14..4901b73 100644
> >> --- a/drivers/mmc/host/mmci.c
> >> +++ b/drivers/mmc/host/mmci.c
> >> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> >>               unsigned int status)
> >>   {
> >>          void __iomem *base = host->base;
> >> -       bool sbc;
> >> +       bool sbc, busy_resp;
> >>
> >>          if (!cmd)
> >>                  return;
> >>
> >>          sbc = (cmd == host->mrq->sbc);
> >> +       busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
> >>
> >>          /*
> >>           * We need to be one of these interrupts to be considered worth
> >> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> >>          /*
> >>           * ST Micro variant: handle busy detection.
> >>           */
> >> -       if (host->variant->busy_detect) {
> >> -               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
> >> +       if (busy_resp && host->variant->busy_detect) {
> >>
> >>                  /* We are busy with a command, return */
> >>                  if (host->busy_status &&
> >> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> >>                   * that the special busy status bit is still set before
> >>                   * proceeding.
> >>                   */
> >> -               if (!host->busy_status && busy_resp &&
> >> +               if (!host->busy_status &&
> >>                      !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
> >>                      (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> >
> > All the changes above makes perfect sense to me, but looks more like a
> > cleanup of the code, rather than actually changing the behavior.
>
> yes, few changing (this just avoid to enter in
> "if (host->variant->busy_detect)") at each time.
> I could move this part in cleanup patch (before this patch)

Sounds good to me!

>
> >
> >>
> >> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> >>   {
> >>          struct mmci_host *host = dev_id;
> >>          u32 status;
> >> +       bool busy_resp;
> >>          int ret = 0;
> >>
> >>          spin_lock(&host->lock);
> >> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> >>                  }
> >>
> >>                  /*
> >> -                * Don't poll for busy completion in irq context.
> >> +                * Don't poll for:
> >> +                * -busy completion in irq context.
> >> +                * -no busy response expected.
> >>                   */
> >> -               if (host->variant->busy_detect && host->busy_status)
> >> +               busy_resp = host->cmd ?
> >> +                       !!(host->cmd->flags & MMC_RSP_BUSY) : false;
> >
> > This doesn't make sense to me, but I may be missing something.
> >
> > host->busy_status is being updated by mmci_cmd_irq() and only when
> > MMC_RSP_BUSY is set for the command in flight. In other words,
> > checking for MMC_RSP_BUSY here as well is redundant. No?
>
> In mmci_irq the "do while" loops until the status is totally cleared.
>
> Today (for variant with busy_detect option), the status busy_detect_flag
> is excluded only while busy_status period (command with MMC_RSP_BUSY and
> while busy line is low => "busy_status=1")
>
> On SDMMC variant I noticed that busy_detect_flag status could be enabled
> even if the command is not MMC_RSP_BUSY, for example sdmmc variant stay
> in loop while cmd11 voltage switch.

Right, I see.

>
> So I wish extend host->variant->busy_detect_flag exclusion for all
> commands which is not a MMC_RSP_BUSY. I suppose that other variants
> could have the same behavior, and else there is no impact, normally.

I am guessing this is because the variant->busy_dpsm_flag has been set
in the datactrl register, which is needed for mmci_card_busy().

That said, I am kind of wondering if we ever should need repeat the
while loop if 'status' contains the bit for
host->variant->busy_detect_flag. I mean we have already called
mmci_cmd_irq() to handle it.

So, couldn't we just always do:

if (host->variant->busy_detect_flag)
    status &= ~host->variant->busy_detect_flag;

No?

>
> >
> >> +
> >> +               if (host->variant->busy_detect &&
> >> +                   (!busy_resp || host->busy_status))
> >>                          status &= ~host->variant->busy_detect_flag;
> >>
> >>                  ret = 1;
> >> --
> >> 2.7.4
> >>
> >
> > Kind regards
> > Uffe
> >

Kind regards
Uffe

Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling

[Ludovic BARRE]

On 4/25/19 12:08 PM, Ulf Hansson wrote:
> On Thu, 25 Apr 2019 at 11:22, Ludovic BARRE <ludovic.barre@st.com> wrote:
>>
>> hi Ulf
>>
>> On 4/23/19 3:39 PM, Ulf Hansson wrote:
>>> On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote:
>>>>
>>>> From: Ludovic Barre <ludovic.barre@st.com>
>>>>
>>>> The busy status bit could occurred even if no busy response is
>>>> expected (example cmd11). On sdmmc variant, the busy_detect_flag
>>>> reflects inverted value of d0 state, it's sampled at the end of a
>>>> CMD response and a second time 2 clk cycles after the CMD response.
>>>> To avoid a fake busy, the busy status could be checked and polled
>>>> only if the command has RSP_BUSY flag.
>>>
>>> I would appreciate a better explanation of what this patch really changes.
>>>
>>> The above is giving some background to the behavior of sdmmc variant,
>>> but at this point that variant doesn't even have the
>>> ->variant->busy_detect flag set.
>>>
>>
>> Yes, I will try to explain more and focus on common behavior.
>>
>>>>
>>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>>>> ---
>>>>    drivers/mmc/host/mmci.c | 19 +++++++++++++------
>>>>    1 file changed, 13 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>>> index 387ff14..4901b73 100644
>>>> --- a/drivers/mmc/host/mmci.c
>>>> +++ b/drivers/mmc/host/mmci.c
>>>> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>>>                unsigned int status)
>>>>    {
>>>>           void __iomem *base = host->base;
>>>> -       bool sbc;
>>>> +       bool sbc, busy_resp;
>>>>
>>>>           if (!cmd)
>>>>                   return;
>>>>
>>>>           sbc = (cmd == host->mrq->sbc);
>>>> +       busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
>>>>
>>>>           /*
>>>>            * We need to be one of these interrupts to be considered worth
>>>> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>>>           /*
>>>>            * ST Micro variant: handle busy detection.
>>>>            */
>>>> -       if (host->variant->busy_detect) {
>>>> -               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
>>>> +       if (busy_resp && host->variant->busy_detect) {
>>>>
>>>>                   /* We are busy with a command, return */
>>>>                   if (host->busy_status &&
>>>> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>>>                    * that the special busy status bit is still set before
>>>>                    * proceeding.
>>>>                    */
>>>> -               if (!host->busy_status && busy_resp &&
>>>> +               if (!host->busy_status &&
>>>>                       !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
>>>>                       (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
>>>
>>> All the changes above makes perfect sense to me, but looks more like a
>>> cleanup of the code, rather than actually changing the behavior.
>>
>> yes, few changing (this just avoid to enter in
>> "if (host->variant->busy_detect)") at each time.
>> I could move this part in cleanup patch (before this patch)
> 
> Sounds good to me!
> 
>>
>>>
>>>>
>>>> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>>>    {
>>>>           struct mmci_host *host = dev_id;
>>>>           u32 status;
>>>> +       bool busy_resp;
>>>>           int ret = 0;
>>>>
>>>>           spin_lock(&host->lock);
>>>> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>>>                   }
>>>>
>>>>                   /*
>>>> -                * Don't poll for busy completion in irq context.
>>>> +                * Don't poll for:
>>>> +                * -busy completion in irq context.
>>>> +                * -no busy response expected.
>>>>                    */
>>>> -               if (host->variant->busy_detect && host->busy_status)
>>>> +               busy_resp = host->cmd ?
>>>> +                       !!(host->cmd->flags & MMC_RSP_BUSY) : false;
>>>
>>> This doesn't make sense to me, but I may be missing something.
>>>
>>> host->busy_status is being updated by mmci_cmd_irq() and only when
>>> MMC_RSP_BUSY is set for the command in flight. In other words,
>>> checking for MMC_RSP_BUSY here as well is redundant. No?
>>
>> In mmci_irq the "do while" loops until the status is totally cleared.
>>
>> Today (for variant with busy_detect option), the status busy_detect_flag
>> is excluded only while busy_status period (command with MMC_RSP_BUSY and
>> while busy line is low => "busy_status=1")
>>
>> On SDMMC variant I noticed that busy_detect_flag status could be enabled
>> even if the command is not MMC_RSP_BUSY, for example sdmmc variant stay
>> in loop while cmd11 voltage switch.
> 
> Right, I see.
> 
>>
>> So I wish extend host->variant->busy_detect_flag exclusion for all
>> commands which is not a MMC_RSP_BUSY. I suppose that other variants
>> could have the same behavior, and else there is no impact, normally.
> 
> I am guessing this is because the variant->busy_dpsm_flag has been set
> in the datactrl register, which is needed for mmci_card_busy().
> 
> That said, I am kind of wondering if we ever should need repeat the
> while loop if 'status' contains the bit for
> host->variant->busy_detect_flag. I mean we have already called
> mmci_cmd_irq() to handle it.
> 
> So, couldn't we just always do:
> 
> if (host->variant->busy_detect_flag)
>      status &= ~host->variant->busy_detect_flag;
> 
> No?

yes that make sense, I launched tests on sdmmc and it's ok.
I think, that we could take on this solution.

If it's ok for you, I resend a series with all modifications.

Regards
Ludo

> 
>>
>>>
>>>> +
>>>> +               if (host->variant->busy_detect &&
>>>> +                   (!busy_resp || host->busy_status))
>>>>                           status &= ~host->variant->busy_detect_flag;
>>>>
>>>>                   ret = 1;
>>>> --
>>>> 2.7.4
>>>>
>>>
>>> Kind regards
>>> Uffe
>>>
> 
> Kind regards
> Uffe
> 

Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling

[Ulf Hansson]

On Thu, 25 Apr 2019 at 16:09, Ludovic BARRE <ludovic.barre@st.com> wrote:
>
>
> On 4/25/19 12:08 PM, Ulf Hansson wrote:
> > On Thu, 25 Apr 2019 at 11:22, Ludovic BARRE <ludovic.barre@st.com> wrote:
> >>
> >> hi Ulf
> >>
> >> On 4/23/19 3:39 PM, Ulf Hansson wrote:
> >>> On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote:
> >>>>
> >>>> From: Ludovic Barre <ludovic.barre@st.com>
> >>>>
> >>>> The busy status bit could occurred even if no busy response is
> >>>> expected (example cmd11). On sdmmc variant, the busy_detect_flag
> >>>> reflects inverted value of d0 state, it's sampled at the end of a
> >>>> CMD response and a second time 2 clk cycles after the CMD response.
> >>>> To avoid a fake busy, the busy status could be checked and polled
> >>>> only if the command has RSP_BUSY flag.
> >>>
> >>> I would appreciate a better explanation of what this patch really changes.
> >>>
> >>> The above is giving some background to the behavior of sdmmc variant,
> >>> but at this point that variant doesn't even have the
> >>> ->variant->busy_detect flag set.
> >>>
> >>
> >> Yes, I will try to explain more and focus on common behavior.
> >>
> >>>>
> >>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> >>>> ---
> >>>>    drivers/mmc/host/mmci.c | 19 +++++++++++++------
> >>>>    1 file changed, 13 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> >>>> index 387ff14..4901b73 100644
> >>>> --- a/drivers/mmc/host/mmci.c
> >>>> +++ b/drivers/mmc/host/mmci.c
> >>>> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> >>>>                unsigned int status)
> >>>>    {
> >>>>           void __iomem *base = host->base;
> >>>> -       bool sbc;
> >>>> +       bool sbc, busy_resp;
> >>>>
> >>>>           if (!cmd)
> >>>>                   return;
> >>>>
> >>>>           sbc = (cmd == host->mrq->sbc);
> >>>> +       busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
> >>>>
> >>>>           /*
> >>>>            * We need to be one of these interrupts to be considered worth
> >>>> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> >>>>           /*
> >>>>            * ST Micro variant: handle busy detection.
> >>>>            */
> >>>> -       if (host->variant->busy_detect) {
> >>>> -               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
> >>>> +       if (busy_resp && host->variant->busy_detect) {
> >>>>
> >>>>                   /* We are busy with a command, return */
> >>>>                   if (host->busy_status &&
> >>>> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> >>>>                    * that the special busy status bit is still set before
> >>>>                    * proceeding.
> >>>>                    */
> >>>> -               if (!host->busy_status && busy_resp &&
> >>>> +               if (!host->busy_status &&
> >>>>                       !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
> >>>>                       (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> >>>
> >>> All the changes above makes perfect sense to me, but looks more like a
> >>> cleanup of the code, rather than actually changing the behavior.
> >>
> >> yes, few changing (this just avoid to enter in
> >> "if (host->variant->busy_detect)") at each time.
> >> I could move this part in cleanup patch (before this patch)
> >
> > Sounds good to me!
> >
> >>
> >>>
> >>>>
> >>>> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> >>>>    {
> >>>>           struct mmci_host *host = dev_id;
> >>>>           u32 status;
> >>>> +       bool busy_resp;
> >>>>           int ret = 0;
> >>>>
> >>>>           spin_lock(&host->lock);
> >>>> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> >>>>                   }
> >>>>
> >>>>                   /*
> >>>> -                * Don't poll for busy completion in irq context.
> >>>> +                * Don't poll for:
> >>>> +                * -busy completion in irq context.
> >>>> +                * -no busy response expected.
> >>>>                    */
> >>>> -               if (host->variant->busy_detect && host->busy_status)
> >>>> +               busy_resp = host->cmd ?
> >>>> +                       !!(host->cmd->flags & MMC_RSP_BUSY) : false;
> >>>
> >>> This doesn't make sense to me, but I may be missing something.
> >>>
> >>> host->busy_status is being updated by mmci_cmd_irq() and only when
> >>> MMC_RSP_BUSY is set for the command in flight. In other words,
> >>> checking for MMC_RSP_BUSY here as well is redundant. No?
> >>
> >> In mmci_irq the "do while" loops until the status is totally cleared.
> >>
> >> Today (for variant with busy_detect option), the status busy_detect_flag
> >> is excluded only while busy_status period (command with MMC_RSP_BUSY and
> >> while busy line is low => "busy_status=1")
> >>
> >> On SDMMC variant I noticed that busy_detect_flag status could be enabled
> >> even if the command is not MMC_RSP_BUSY, for example sdmmc variant stay
> >> in loop while cmd11 voltage switch.
> >
> > Right, I see.
> >
> >>
> >> So I wish extend host->variant->busy_detect_flag exclusion for all
> >> commands which is not a MMC_RSP_BUSY. I suppose that other variants
> >> could have the same behavior, and else there is no impact, normally.
> >
> > I am guessing this is because the variant->busy_dpsm_flag has been set
> > in the datactrl register, which is needed for mmci_card_busy().
> >
> > That said, I am kind of wondering if we ever should need repeat the
> > while loop if 'status' contains the bit for
> > host->variant->busy_detect_flag. I mean we have already called
> > mmci_cmd_irq() to handle it.
> >
> > So, couldn't we just always do:
> >
> > if (host->variant->busy_detect_flag)
> >      status &= ~host->variant->busy_detect_flag;
> >
> > No?
>
> yes that make sense, I launched tests on sdmmc and it's ok.
> I think, that we could take on this solution.

Great!

>
> If it's ok for you, I resend a series with all modifications.

Yes, please do. I haven't reviewed the rest of the series yet, but it
may be better to do that when the next version is ready.

In either case, I should have some time to run some tests of the next
version, if you manage to send it within a couple of days or so.

[...]

Kind regards
Uffe