[1/2] mmc: mmci: Drop re-read of MMCISTATUS for busy status
diff mbox series

Message ID 20190715164217.27297-1-ulf.hansson@linaro.org
State New
Headers show
Series
  • [1/2] mmc: mmci: Drop re-read of MMCISTATUS for busy status
Related show

Commit Message

Ulf Hansson July 15, 2019, 4:42 p.m. UTC
The MMCISTATUS register is read from the IRQ handler, mmci_irq(). For
processing, its value is then passed as an in-parameter to mmci_cmd_irq().

When dealing with busy detection, the MMCISTATUS register is being re-read,
as to retrieve an updated value. However, this operation is likely costing
more than it benefits, as the value was read only a few cycles ago. For
this reason, let's simply drop the re-read and use the value from the
in-parameter instead.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmci.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Linus Walleij July 16, 2019, 9:26 p.m. UTC | #1
On Mon, Jul 15, 2019 at 6:42 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:

> The MMCISTATUS register is read from the IRQ handler, mmci_irq(). For
> processing, its value is then passed as an in-parameter to mmci_cmd_irq().
>
> When dealing with busy detection, the MMCISTATUS register is being re-read,
> as to retrieve an updated value. However, this operation is likely costing
> more than it benefits, as the value was read only a few cycles ago. For
> this reason, let's simply drop the re-read and use the value from the
> in-parameter instead.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Jean Nicolas GRAUX July 17, 2019, 10:16 a.m. UTC | #2
Hello Ulf, all,

For testing purpose, I cherry-picked your patch on top of a 4.19.30 basis.
(I apologize as it's a bit old. I miss time to do a rebase on current 
linux-next right now.)

Unfortunately, I got a kernel crash applying it :(

As you may know, ST sta1295/sta1385 SoC embeds the same pl08x variant 
than one in U8500.
So It looks like double-checking again mmci status to make sure busy 
flag is still set
just before proceeding for busy end is required in our case.

Regards.
Jean-Nicolas.

On 7/15/19 6:42 PM, Ulf Hansson wrote:
> The MMCISTATUS register is read from the IRQ handler, mmci_irq(). For
> processing, its value is then passed as an in-parameter to mmci_cmd_irq().
>
> When dealing with busy detection, the MMCISTATUS register is being re-read,
> as to retrieve an updated value. However, this operation is likely costing
> more than it benefits, as the value was read only a few cycles ago. For
> this reason, let's simply drop the re-read and use the value from the
> in-parameter instead.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>   drivers/mmc/host/mmci.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 356833a606d5..5f35afc4dbf9 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1233,14 +1233,12 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>   			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.
>   		 */
>   		if (!host->busy_status &&
>   		    !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
> -		    (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> +		    (status & host->variant->busy_detect_flag)) {
>   
>   			/* Clear the busy start IRQ */
>   			writel(host->variant->busy_detect_mask,
Ulf Hansson July 17, 2019, 12:36 p.m. UTC | #3
On Wed, 17 Jul 2019 at 12:16, Jean Nicolas GRAUX
<jean-nicolas.graux@st.com> wrote:
>
> Hello Ulf, all,
>
> For testing purpose, I cherry-picked your patch on top of a 4.19.30 basis.
> (I apologize as it's a bit old. I miss time to do a rebase on current
> linux-next right now.)

No worries about the old kernel, for this change, I think it should be
suufient good as base.

>
> Unfortunately, I got a kernel crash applying it :(

Huh.

Is it crashing because it fails to mount the rootfs on the SD/MMC card?

>
> As you may know, ST sta1295/sta1385 SoC embeds the same pl08x variant
> than one in U8500.
> So It looks like double-checking again mmci status to make sure busy
> flag is still set
> just before proceeding for busy end is required in our case.

Yeah, actually I have a u8500 on my desk now, so I will also test the
patch to see what goes on. Didn't have the time to do it earlier.

My guess is that, at the point when we received the IRQ for a command
that has been sent, and then reading the MMCISTATUS register in
mmci_irq(), the card has not yet started to signal busy on DAT1 and
hence the busy status bit isn't set yet. This leads to that we will
never enable the busy end mask, but just completing the request
directly.

Anyway, let me check and see if I can confirm it.

>
> Regards.
> Jean-Nicolas.

Thanks a lot for helping out testing and reporting the problem!

[...]

Kind regards
Uffe
Ludovic Barre July 17, 2019, 12:50 p.m. UTC | #4
hi Ulf, JN

IMHO you can't split my initial patch in two,
this piece of code is needed (function: mmci_irq)

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

Regards,
Ludo

On 7/17/19 2:36 PM, Ulf Hansson wrote:
> On Wed, 17 Jul 2019 at 12:16, Jean Nicolas GRAUX
> <jean-nicolas.graux@st.com> wrote:
>>
>> Hello Ulf, all,
>>
>> For testing purpose, I cherry-picked your patch on top of a 4.19.30 basis.
>> (I apologize as it's a bit old. I miss time to do a rebase on current
>> linux-next right now.)
> 
> No worries about the old kernel, for this change, I think it should be
> suufient good as base.
> 
>>
>> Unfortunately, I got a kernel crash applying it :(
> 
> Huh.
> 
> Is it crashing because it fails to mount the rootfs on the SD/MMC card?
> 
>>
>> As you may know, ST sta1295/sta1385 SoC embeds the same pl08x variant
>> than one in U8500.
>> So It looks like double-checking again mmci status to make sure busy
>> flag is still set
>> just before proceeding for busy end is required in our case.
> 
> Yeah, actually I have a u8500 on my desk now, so I will also test the
> patch to see what goes on. Didn't have the time to do it earlier.
> 
> My guess is that, at the point when we received the IRQ for a command
> that has been sent, and then reading the MMCISTATUS register in
> mmci_irq(), the card has not yet started to signal busy on DAT1 and
> hence the busy status bit isn't set yet. This leads to that we will
> never enable the busy end mask, but just completing the request
> directly.
> 
> Anyway, let me check and see if I can confirm it.
> 
>>
>> Regards.
>> Jean-Nicolas.
> 
> Thanks a lot for helping out testing and reporting the problem!
> 
> [...]
> 
> Kind regards
> Uffe
>
Ulf Hansson July 17, 2019, 1:05 p.m. UTC | #5
On Wed, 17 Jul 2019 at 14:51, Ludovic BARRE <ludovic.barre@st.com> wrote:
>
> hi Ulf, JN
>
> IMHO you can't split my initial patch in two,
> this piece of code is needed (function: mmci_irq)
>
> -               status &= readl(host->base + MMCIMASK0);
> +               status &= readl(host->base + MMCIMASK0) |
> +                       host->variant->busy_detect_flag;

I am not buying that (at least yet), because it means that you tag on
the busy bit even if the card hasn't signaled busy, which to me seems
wrong.

I running some tests now, hopefully I will be able to complete them
before end today, else I will continue tomorrow mornings.

Kind regards
Uffe

>
> Regards,
> Ludo
>
> On 7/17/19 2:36 PM, Ulf Hansson wrote:
> > On Wed, 17 Jul 2019 at 12:16, Jean Nicolas GRAUX
> > <jean-nicolas.graux@st.com> wrote:
> >>
> >> Hello Ulf, all,
> >>
> >> For testing purpose, I cherry-picked your patch on top of a 4.19.30 basis.
> >> (I apologize as it's a bit old. I miss time to do a rebase on current
> >> linux-next right now.)
> >
> > No worries about the old kernel, for this change, I think it should be
> > suufient good as base.
> >
> >>
> >> Unfortunately, I got a kernel crash applying it :(
> >
> > Huh.
> >
> > Is it crashing because it fails to mount the rootfs on the SD/MMC card?
> >
> >>
> >> As you may know, ST sta1295/sta1385 SoC embeds the same pl08x variant
> >> than one in U8500.
> >> So It looks like double-checking again mmci status to make sure busy
> >> flag is still set
> >> just before proceeding for busy end is required in our case.
> >
> > Yeah, actually I have a u8500 on my desk now, so I will also test the
> > patch to see what goes on. Didn't have the time to do it earlier.
> >
> > My guess is that, at the point when we received the IRQ for a command
> > that has been sent, and then reading the MMCISTATUS register in
> > mmci_irq(), the card has not yet started to signal busy on DAT1 and
> > hence the busy status bit isn't set yet. This leads to that we will
> > never enable the busy end mask, but just completing the request
> > directly.
> >
> > Anyway, let me check and see if I can confirm it.
> >
> >>
> >> Regards.
> >> Jean-Nicolas.
> >
> > Thanks a lot for helping out testing and reporting the problem!
> >
> > [...]
> >
> > Kind regards
> > Uffe
> >
Ulf Hansson July 17, 2019, 1:19 p.m. UTC | #6
On Wed, 17 Jul 2019 at 14:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 17 Jul 2019 at 12:16, Jean Nicolas GRAUX
> <jean-nicolas.graux@st.com> wrote:
> >
> > Hello Ulf, all,
> >
> > For testing purpose, I cherry-picked your patch on top of a 4.19.30 basis.
> > (I apologize as it's a bit old. I miss time to do a rebase on current
> > linux-next right now.)
>
> No worries about the old kernel, for this change, I think it should be
> suufient good as base.
>
> >
> > Unfortunately, I got a kernel crash applying it :(
>
> Huh.
>
> Is it crashing because it fails to mount the rootfs on the SD/MMC card?
>
> >
> > As you may know, ST sta1295/sta1385 SoC embeds the same pl08x variant
> > than one in U8500.
> > So It looks like double-checking again mmci status to make sure busy
> > flag is still set
> > just before proceeding for busy end is required in our case.
>
> Yeah, actually I have a u8500 on my desk now, so I will also test the
> patch to see what goes on. Didn't have the time to do it earlier.
>
> My guess is that, at the point when we received the IRQ for a command
> that has been sent, and then reading the MMCISTATUS register in
> mmci_irq(), the card has not yet started to signal busy on DAT1 and
> hence the busy status bit isn't set yet. This leads to that we will
> never enable the busy end mask, but just completing the request
> directly.
>
> Anyway, let me check and see if I can confirm it.

Problem confirmed.

Moreover, even before my changes it looks like the similar problem is
there. In other words, even if we re-read the status register a few
cycles later in mmci_cmd_irq() there is still a chance that we end up
to by-pass the busy detection, because the card haven't yet started to
signal busy. Just during boot of ux500, I found this to occur three
times, but luckily the card detection works anyway.

[...]

I am working on fix, let's see what I can come up with. Likely to be
posted tomorrow.

Kind regards
Uffe

Patch
diff mbox series

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 356833a606d5..5f35afc4dbf9 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1233,14 +1233,12 @@  mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 			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.
 		 */
 		if (!host->busy_status &&
 		    !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
-		    (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
+		    (status & host->variant->busy_detect_flag)) {
 
 			/* Clear the busy start IRQ */
 			writel(host->variant->busy_detect_mask,