diff mbox series

[V2] mmc: mmci: Clarify comments and some code for busy detection

Message ID 20190722111317.20177-1-ulf.hansson@linaro.org
State New
Headers show
Series [V2] mmc: mmci: Clarify comments and some code for busy detection | expand

Commit Message

Ulf Hansson July 22, 2019, 11:13 a.m. UTC 
The code dealing with busy detection is somewhat complicated. In a way to
make it a bit clearer, let's try to clarify the comments in the code about
it.

Additionally, move the part for clearing the so called busy start IRQ, to
the place where the IRQ is actually delivered. Ideally, this should make
the code a bit more robust, but also a bit easier to understand.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---

Changes in v2:
	- Squashed patch 1 and patch 2.
	- Keep the re-read on the status register, but explain why it's needed.
	- Move the clearing of the busy start IRQ at the point it's delivered.

---
 drivers/mmc/host/mmci.c | 63 ++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

-- 
2.17.1

Comments

Jean Nicolas GRAUX July 23, 2019, 8:12 a.m. UTC | #1 
Hello Ulf, all,

I tried this new patch and did not face issues so far.
That is said, some comments below.

Regards, Jean-Nicolas.

On 7/22/19 1:13 PM, Ulf Hansson wrote:
> The code dealing with busy detection is somewhat complicated. In a way to

> make it a bit clearer, let's try to clarify the comments in the code about

> it.

>

> Additionally, move the part for clearing the so called busy start IRQ, to

> the place where the IRQ is actually delivered. Ideally, this should make

> the code a bit more robust, but also a bit easier to understand.

>

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---

>

> Changes in v2:

> 	- Squashed patch 1 and patch 2.

> 	- Keep the re-read on the status register, but explain why it's needed.

> 	- Move the clearing of the busy start IRQ at the point it's delivered.

>

> ---

>   drivers/mmc/host/mmci.c | 63 ++++++++++++++++++++++-------------------

>   1 file changed, 34 insertions(+), 29 deletions(-)

>

> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c

> index 356833a606d5..d3f876c8c292 100644

> --- a/drivers/mmc/host/mmci.c

> +++ b/drivers/mmc/host/mmci.c

> @@ -1222,47 +1222,58 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,

>   	      (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND)))

>   		return;

>   

> -	/*

> -	 * ST Micro variant: handle busy detection.

> -	 */

> +	/* Handle busy detection on DAT0 if the variant supports it. */

>   	if (busy_resp && host->variant->busy_detect) {

>   

> -		/* We are busy with a command, return */

> +		/*

> +		 * If there is a command in-progress that has been successfully

> +		 * sent, then bail out if busy status is set and wait for the

> +		 * busy end IRQ.

> +		 *

> +		 * Note that, the HW triggers an IRQ on both edges while

> +		 * monitoring DAT0 for busy completion, but there is only one

> +		 * status bit in MMCISTATUS for the busy state. Therefore

> +		 * both the start and the end interrupts needs to be cleared,

> +		 * one after the other. So, clear the busy start IRQ here.

> +		 */

>   		if (host->busy_status &&

> -		    (status & host->variant->busy_detect_flag))


To my mind, purpose of that if condition is to wait for busy end event
while the one just below is to clear start event and unmask busy end irq.
So I think maybe it's a bit late to clear busy start event ? What do you 
think ?

> +		    (status & host->variant->busy_detect_flag)) {

> +			writel(host->variant->busy_detect_mask,

> +			       host->base + MMCICLEAR);

>   			return;

> +		}

>   

>   		/*

> -		 * We were not busy, but we now got a busy response on

> -		 * something that was not an error, and we double-check

> -		 * that the special busy status bit is still set before

> -		 * proceeding.

> +		 * Before unmasking for the busy end IRQ, confirm that the

> +		 * command was sent successfully. To keep track of having a

> +		 * command in-progress, waiting for busy signaling to end,

> +		 * store the status in host->busy_status.

> +		 *

> +		 * Note that, the card may need a couple of clock cycles before

> +		 * it starts signaling busy on DAT0, hence re-read the

> +		 * MMCISTATUS register here, to allow the busy bit to be set.

> +		 * Potentially we may even need to poll the register for a

> +		 * while, to allow it to be set, but tests indicates that it

> +		 * isn't needed.

>   		 */

>   		if (!host->busy_status &&

>   		    !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&

>   		    (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {

>   

> -			/* Clear the busy start IRQ */

> -			writel(host->variant->busy_detect_mask,

> -			       host->base + MMCICLEAR);


Why not clearing busy start event as soon as possible ? Maybe I am wrong 
but as far as I understand,
we shall (always) enter that if condition before the one just above ?

> -

> -			/* Unmask the busy end IRQ */

>   			writel(readl(base + MMCIMASK0) |

>   			       host->variant->busy_detect_mask,

>   			       base + MMCIMASK0);

> -			/*

> -			 * Now cache the last response status code (until

> -			 * the busy bit goes low), and return.

> -			 */

> +

>   			host->busy_status =

>   				status & (MCI_CMDSENT|MCI_CMDRESPEND);

>   			return;

>   		}

>   

>   		/*

> -		 * At this point we are not busy with a command, we have

> -		 * not received a new busy request, clear and mask the busy

> -		 * end IRQ and fall through to process the IRQ.

> +		 * If there is a command in-progress that has been successfully

> +		 * sent and the busy bit isn't set, it means we have received

> +		 * the busy end IRQ. Clear and mask the IRQ, then continue to

> +		 * process the command.

>   		 */

>   		if (host->busy_status) {

>   

> @@ -1508,14 +1519,8 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)

>   		}

>   

>   		/*

> -		 * We intentionally clear the MCI_ST_CARDBUSY IRQ (if it's

> -		 * enabled) in mmci_cmd_irq() function where ST Micro busy

> -		 * detection variant is handled. Considering the HW seems to be

> -		 * triggering the IRQ on both edges while monitoring DAT0 for

> -		 * busy completion and that same status bit is used to monitor

> -		 * start and end of busy detection, special care must be taken

> -		 * to make sure that both start and end interrupts are always

> -		 * cleared one after the other.

> +		 * Busy detection is managed by mmci_cmd_irq(), including to

> +		 * clear the corresponding IRQ.

>   		 */

>   		status &= readl(host->base + MMCIMASK0);

>   		if (host->variant->busy_detect)
Ulf Hansson July 23, 2019, 8:34 a.m. UTC | #2 
On Tue, 23 Jul 2019 at 10:12, Jean Nicolas GRAUX
<jean-nicolas.graux@st.com> wrote:
>

> Hello Ulf, all,

>

> I tried this new patch and did not face issues so far.


Great, thanks!

> That is said, some comments below.

>

> Regards, Jean-Nicolas.

>

> On 7/22/19 1:13 PM, Ulf Hansson wrote:

> > The code dealing with busy detection is somewhat complicated. In a way to

> > make it a bit clearer, let's try to clarify the comments in the code about

> > it.

> >

> > Additionally, move the part for clearing the so called busy start IRQ, to

> > the place where the IRQ is actually delivered. Ideally, this should make

> > the code a bit more robust, but also a bit easier to understand.

> >

> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> > ---

> >

> > Changes in v2:

> >       - Squashed patch 1 and patch 2.

> >       - Keep the re-read on the status register, but explain why it's needed.

> >       - Move the clearing of the busy start IRQ at the point it's delivered.

> >

> > ---

> >   drivers/mmc/host/mmci.c | 63 ++++++++++++++++++++++-------------------

> >   1 file changed, 34 insertions(+), 29 deletions(-)

> >

> > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c

> > index 356833a606d5..d3f876c8c292 100644

> > --- a/drivers/mmc/host/mmci.c

> > +++ b/drivers/mmc/host/mmci.c

> > @@ -1222,47 +1222,58 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,

> >             (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND)))

> >               return;

> >

> > -     /*

> > -      * ST Micro variant: handle busy detection.

> > -      */

> > +     /* Handle busy detection on DAT0 if the variant supports it. */

> >       if (busy_resp && host->variant->busy_detect) {

> >

> > -             /* We are busy with a command, return */

> > +             /*

> > +              * If there is a command in-progress that has been successfully

> > +              * sent, then bail out if busy status is set and wait for the

> > +              * busy end IRQ.

> > +              *

> > +              * Note that, the HW triggers an IRQ on both edges while

> > +              * monitoring DAT0 for busy completion, but there is only one

> > +              * status bit in MMCISTATUS for the busy state. Therefore

> > +              * both the start and the end interrupts needs to be cleared,

> > +              * one after the other. So, clear the busy start IRQ here.

> > +              */

> >               if (host->busy_status &&

> > -                 (status & host->variant->busy_detect_flag))

>

> To my mind, purpose of that if condition is to wait for busy end event

> while the one just below is to clear start event and unmask busy end irq.

> So I think maybe it's a bit late to clear busy start event ? What do you

> think ?


Before my change, we are always reaching this if clause, only once and
without having any other status bit set but the busy bit. It sounds to
me that this is actually the busy start IRQ that we receives, see more
why I think so below.

That said, by clearing the busy start IRQ here, we still only reach
this if clause, only once.

>

> > +                 (status & host->variant->busy_detect_flag)) {

> > +                     writel(host->variant->busy_detect_mask,

> > +                            host->base + MMCICLEAR);

> >                       return;

> > +             }

> >

> >               /*

> > -              * We were not busy, but we now got a busy response on

> > -              * something that was not an error, and we double-check

> > -              * that the special busy status bit is still set before

> > -              * proceeding.

> > +              * Before unmasking for the busy end IRQ, confirm that the

> > +              * command was sent successfully. To keep track of having a

> > +              * command in-progress, waiting for busy signaling to end,

> > +              * store the status in host->busy_status.

> > +              *

> > +              * Note that, the card may need a couple of clock cycles before

> > +              * it starts signaling busy on DAT0, hence re-read the

> > +              * MMCISTATUS register here, to allow the busy bit to be set.

> > +              * Potentially we may even need to poll the register for a

> > +              * while, to allow it to be set, but tests indicates that it

> > +              * isn't needed.

> >                */

> >               if (!host->busy_status &&

> >                   !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&

> >                   (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {

> >

> > -                     /* Clear the busy start IRQ */

> > -                     writel(host->variant->busy_detect_mask,

> > -                            host->base + MMCICLEAR);

>

> Why not clearing busy start event as soon as possible ? Maybe I am wrong

> but as far as I understand,

> we shall (always) enter that if condition before the one just above ?


Two things feel wrong about by clearing the IRQ here.

1) We have not yet unmasked the busy end IRQ and we don't have a bit
in the IRQ mask for the busy *start* IRQ (they are the same). Then we
are clearing an IRQ that we have not yet unmasked to receive, which
seems odd/wrong to me.
2) Even if we clear it here, we are still receiving the busy start
IRQ, as described in my comment above.

>

> > -

> > -                     /* Unmask the busy end IRQ */

> >                       writel(readl(base + MMCIMASK0) |

> >                              host->variant->busy_detect_mask,

> >                              base + MMCIMASK0);

> > -                     /*

> > -                      * Now cache the last response status code (until

> > -                      * the busy bit goes low), and return.

> > -                      */

> > +

> >                       host->busy_status =

> >                               status & (MCI_CMDSENT|MCI_CMDRESPEND);

> >                       return;

> >               }

> >

> >               /*

> > -              * At this point we are not busy with a command, we have

> > -              * not received a new busy request, clear and mask the busy

> > -              * end IRQ and fall through to process the IRQ.

> > +              * If there is a command in-progress that has been successfully

> > +              * sent and the busy bit isn't set, it means we have received

> > +              * the busy end IRQ. Clear and mask the IRQ, then continue to

> > +              * process the command.

> >                */

> >               if (host->busy_status) {

> >

> > @@ -1508,14 +1519,8 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)

> >               }

> >

> >               /*

> > -              * We intentionally clear the MCI_ST_CARDBUSY IRQ (if it's

> > -              * enabled) in mmci_cmd_irq() function where ST Micro busy

> > -              * detection variant is handled. Considering the HW seems to be

> > -              * triggering the IRQ on both edges while monitoring DAT0 for

> > -              * busy completion and that same status bit is used to monitor

> > -              * start and end of busy detection, special care must be taken

> > -              * to make sure that both start and end interrupts are always

> > -              * cleared one after the other.

> > +              * Busy detection is managed by mmci_cmd_irq(), including to

> > +              * clear the corresponding IRQ.

> >                */

> >               status &= readl(host->base + MMCIMASK0);

> >               if (host->variant->busy_detect)

>


Kind regards
Uffe
Jean Nicolas GRAUX July 23, 2019, 9:01 a.m. UTC | #3 
On 7/23/19 10:34 AM, Ulf Hansson wrote:
> On Tue, 23 Jul 2019 at 10:12, Jean Nicolas GRAUX

> <jean-nicolas.graux@st.com> wrote:

>> Hello Ulf, all,

>>

>> I tried this new patch and did not face issues so far.

> Great, thanks!

>

>> That is said, some comments below.

>>

>> Regards, Jean-Nicolas.

>>

>> On 7/22/19 1:13 PM, Ulf Hansson wrote:

>>> The code dealing with busy detection is somewhat complicated. In a way to

>>> make it a bit clearer, let's try to clarify the comments in the code about

>>> it.

>>>

>>> Additionally, move the part for clearing the so called busy start IRQ, to

>>> the place where the IRQ is actually delivered. Ideally, this should make

>>> the code a bit more robust, but also a bit easier to understand.

>>>

>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

>>> ---

>>>

>>> Changes in v2:

>>>        - Squashed patch 1 and patch 2.

>>>        - Keep the re-read on the status register, but explain why it's needed.

>>>        - Move the clearing of the busy start IRQ at the point it's delivered.

>>>

>>> ---

>>>    drivers/mmc/host/mmci.c | 63 ++++++++++++++++++++++-------------------

>>>    1 file changed, 34 insertions(+), 29 deletions(-)

>>>

>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c

>>> index 356833a606d5..d3f876c8c292 100644

>>> --- a/drivers/mmc/host/mmci.c

>>> +++ b/drivers/mmc/host/mmci.c

>>> @@ -1222,47 +1222,58 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,

>>>              (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND)))

>>>                return;

>>>

>>> -     /*

>>> -      * ST Micro variant: handle busy detection.

>>> -      */

>>> +     /* Handle busy detection on DAT0 if the variant supports it. */

>>>        if (busy_resp && host->variant->busy_detect) {

>>>

>>> -             /* We are busy with a command, return */

>>> +             /*

>>> +              * If there is a command in-progress that has been successfully

>>> +              * sent, then bail out if busy status is set and wait for the

>>> +              * busy end IRQ.

>>> +              *

>>> +              * Note that, the HW triggers an IRQ on both edges while

>>> +              * monitoring DAT0 for busy completion, but there is only one

>>> +              * status bit in MMCISTATUS for the busy state. Therefore

>>> +              * both the start and the end interrupts needs to be cleared,

>>> +              * one after the other. So, clear the busy start IRQ here.

>>> +              */

>>>                if (host->busy_status &&

>>> -                 (status & host->variant->busy_detect_flag))

>> To my mind, purpose of that if condition is to wait for busy end event

>> while the one just below is to clear start event and unmask busy end irq.

>> So I think maybe it's a bit late to clear busy start event ? What do you

>> think ?

> Before my change, we are always reaching this if clause, only once and

> without having any other status bit set but the busy bit. It sounds to

> me that this is actually the busy start IRQ that we receives, see more

> why I think so below.

>

> That said, by clearing the busy start IRQ here, we still only reach

> this if clause, only once.

>

>>> +                 (status & host->variant->busy_detect_flag)) {

>>> +                     writel(host->variant->busy_detect_mask,

>>> +                            host->base + MMCICLEAR);

>>>                        return;

>>> +             }

>>>

>>>                /*

>>> -              * We were not busy, but we now got a busy response on

>>> -              * something that was not an error, and we double-check

>>> -              * that the special busy status bit is still set before

>>> -              * proceeding.

>>> +              * Before unmasking for the busy end IRQ, confirm that the

>>> +              * command was sent successfully. To keep track of having a

>>> +              * command in-progress, waiting for busy signaling to end,

>>> +              * store the status in host->busy_status.

>>> +              *

>>> +              * Note that, the card may need a couple of clock cycles before

>>> +              * it starts signaling busy on DAT0, hence re-read the

>>> +              * MMCISTATUS register here, to allow the busy bit to be set.

>>> +              * Potentially we may even need to poll the register for a

>>> +              * while, to allow it to be set, but tests indicates that it

>>> +              * isn't needed.

>>>                 */

>>>                if (!host->busy_status &&

>>>                    !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&

>>>                    (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {

>>>

>>> -                     /* Clear the busy start IRQ */

>>> -                     writel(host->variant->busy_detect_mask,

>>> -                            host->base + MMCICLEAR);

>> Why not clearing busy start event as soon as possible ? Maybe I am wrong

>> but as far as I understand,

>> we shall (always) enter that if condition before the one just above ?

> Two things feel wrong about by clearing the IRQ here.

>

> 1) We have not yet unmasked the busy end IRQ and we don't have a bit

> in the IRQ mask for the busy *start* IRQ (they are the same). Then we

> are clearing an IRQ that we have not yet unmasked to receive, which

> seems odd/wrong to me.

> 2) Even if we clear it here, we are still receiving the busy start

> IRQ, as described in my comment above.


Ok, that makes sense ;)

I guess what can be a bit confusing is that we have to unmask busy end 
irq before clearing busy start event.
To better understand the sequence, purely for cosmetic, I wonder whether 
we shall not move that if condition before the first one where we clear 
the busy start event.

>

>>> -

>>> -                     /* Unmask the busy end IRQ */

>>>                        writel(readl(base + MMCIMASK0) |

>>>                               host->variant->busy_detect_mask,

>>>                               base + MMCIMASK0);

>>> -                     /*

>>> -                      * Now cache the last response status code (until

>>> -                      * the busy bit goes low), and return.

>>> -                      */

>>> +

>>>                        host->busy_status =

>>>                                status & (MCI_CMDSENT|MCI_CMDRESPEND);

>>>                        return;

>>>                }

>>>

>>>                /*

>>> -              * At this point we are not busy with a command, we have

>>> -              * not received a new busy request, clear and mask the busy

>>> -              * end IRQ and fall through to process the IRQ.

>>> +              * If there is a command in-progress that has been successfully

>>> +              * sent and the busy bit isn't set, it means we have received

>>> +              * the busy end IRQ. Clear and mask the IRQ, then continue to

>>> +              * process the command.

>>>                 */

>>>                if (host->busy_status) {

>>>

>>> @@ -1508,14 +1519,8 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)

>>>                }

>>>

>>>                /*

>>> -              * We intentionally clear the MCI_ST_CARDBUSY IRQ (if it's

>>> -              * enabled) in mmci_cmd_irq() function where ST Micro busy

>>> -              * detection variant is handled. Considering the HW seems to be

>>> -              * triggering the IRQ on both edges while monitoring DAT0 for

>>> -              * busy completion and that same status bit is used to monitor

>>> -              * start and end of busy detection, special care must be taken

>>> -              * to make sure that both start and end interrupts are always

>>> -              * cleared one after the other.

>>> +              * Busy detection is managed by mmci_cmd_irq(), including to

>>> +              * clear the corresponding IRQ.

>>>                 */

>>>                status &= readl(host->base + MMCIMASK0);

>>>                if (host->variant->busy_detect)

> Kind regards

> Uffe
Ulf Hansson July 23, 2019, 11:56 a.m. UTC | #4 
[...]

> >>>                /*

> >>> -              * We were not busy, but we now got a busy response on

> >>> -              * something that was not an error, and we double-check

> >>> -              * that the special busy status bit is still set before

> >>> -              * proceeding.

> >>> +              * Before unmasking for the busy end IRQ, confirm that the

> >>> +              * command was sent successfully. To keep track of having a

> >>> +              * command in-progress, waiting for busy signaling to end,

> >>> +              * store the status in host->busy_status.

> >>> +              *

> >>> +              * Note that, the card may need a couple of clock cycles before

> >>> +              * it starts signaling busy on DAT0, hence re-read the

> >>> +              * MMCISTATUS register here, to allow the busy bit to be set.

> >>> +              * Potentially we may even need to poll the register for a

> >>> +              * while, to allow it to be set, but tests indicates that it

> >>> +              * isn't needed.

> >>>                 */

> >>>                if (!host->busy_status &&

> >>>                    !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&

> >>>                    (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {

> >>>

> >>> -                     /* Clear the busy start IRQ */

> >>> -                     writel(host->variant->busy_detect_mask,

> >>> -                            host->base + MMCICLEAR);

> >> Why not clearing busy start event as soon as possible ? Maybe I am wrong

> >> but as far as I understand,

> >> we shall (always) enter that if condition before the one just above ?

> > Two things feel wrong about by clearing the IRQ here.

> >

> > 1) We have not yet unmasked the busy end IRQ and we don't have a bit

> > in the IRQ mask for the busy *start* IRQ (they are the same). Then we

> > are clearing an IRQ that we have not yet unmasked to receive, which

> > seems odd/wrong to me.

> > 2) Even if we clear it here, we are still receiving the busy start

> > IRQ, as described in my comment above.

>

> Ok, that makes sense ;)

>

> I guess what can be a bit confusing is that we have to unmask busy end

> irq before clearing busy start event.

> To better understand the sequence, purely for cosmetic, I wonder whether

> we shall not move that if condition before the first one where we clear

> the busy start event.


That is definitely a good idea, it should improve the understand of
the code/sequence. Let me re-spin a v3.

[...]

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 356833a606d5..d3f876c8c292 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1222,47 +1222,58 @@  mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 	      (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND)))
 		return;
 
-	/*
-	 * ST Micro variant: handle busy detection.
-	 */
+	/* Handle busy detection on DAT0 if the variant supports it. */
 	if (busy_resp && host->variant->busy_detect) {
 
-		/* We are busy with a command, return */
+		/*
+		 * If there is a command in-progress that has been successfully
+		 * sent, then bail out if busy status is set and wait for the
+		 * busy end IRQ.
+		 *
+		 * Note that, the HW triggers an IRQ on both edges while
+		 * monitoring DAT0 for busy completion, but there is only one
+		 * status bit in MMCISTATUS for the busy state. Therefore
+		 * both the start and the end interrupts needs to be cleared,
+		 * one after the other. So, clear the busy start IRQ here.
+		 */
 		if (host->busy_status &&
-		    (status & host->variant->busy_detect_flag))
+		    (status & host->variant->busy_detect_flag)) {
+			writel(host->variant->busy_detect_mask,
+			       host->base + MMCICLEAR);
 			return;
+		}
 
 		/*
-		 * We were not busy, but we now got a busy response on
-		 * something that was not an error, and we double-check
-		 * that the special busy status bit is still set before
-		 * proceeding.
+		 * Before unmasking for the busy end IRQ, confirm that the
+		 * command was sent successfully. To keep track of having a
+		 * command in-progress, waiting for busy signaling to end,
+		 * store the status in host->busy_status.
+		 *
+		 * Note that, the card may need a couple of clock cycles before
+		 * it starts signaling busy on DAT0, hence re-read the
+		 * MMCISTATUS register here, to allow the busy bit to be set.
+		 * Potentially we may even need to poll the register for a
+		 * while, to allow it to be set, but tests indicates that it
+		 * isn't needed.
 		 */
 		if (!host->busy_status &&
 		    !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
 		    (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
 
-			/* Clear the busy start IRQ */
-			writel(host->variant->busy_detect_mask,
-			       host->base + MMCICLEAR);
-
-			/* Unmask the busy end IRQ */
 			writel(readl(base + MMCIMASK0) |
 			       host->variant->busy_detect_mask,
 			       base + MMCIMASK0);
-			/*
-			 * Now cache the last response status code (until
-			 * the busy bit goes low), and return.
-			 */
+
 			host->busy_status =
 				status & (MCI_CMDSENT|MCI_CMDRESPEND);
 			return;
 		}
 
 		/*
-		 * At this point we are not busy with a command, we have
-		 * not received a new busy request, clear and mask the busy
-		 * end IRQ and fall through to process the IRQ.
+		 * If there is a command in-progress that has been successfully
+		 * sent and the busy bit isn't set, it means we have received
+		 * the busy end IRQ. Clear and mask the IRQ, then continue to
+		 * process the command.
 		 */
 		if (host->busy_status) {
 
@@ -1508,14 +1519,8 @@  static irqreturn_t mmci_irq(int irq, void *dev_id)
 		}
 
 		/*
-		 * We intentionally clear the MCI_ST_CARDBUSY IRQ (if it's
-		 * enabled) in mmci_cmd_irq() function where ST Micro busy
-		 * detection variant is handled. Considering the HW seems to be
-		 * triggering the IRQ on both edges while monitoring DAT0 for
-		 * busy completion and that same status bit is used to monitor
-		 * start and end of busy detection, special care must be taken
-		 * to make sure that both start and end interrupts are always
-		 * cleared one after the other.
+		 * Busy detection is managed by mmci_cmd_irq(), including to
+		 * clear the corresponding IRQ.
 		 */
 		status &= readl(host->base + MMCIMASK0);
 		if (host->variant->busy_detect)