| 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 | expand |
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
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,
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
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 >
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 > >
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
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,
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(-) -- 2.17.1