| Message ID | 1551802205-32188-2-git-send-email-ludovic.Barre@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: mmci: add busy detect for stm32 sdmmc variant | expand |
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 >
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 >>
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 > >>
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 >>>>
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
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
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 >
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
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 >
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
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;