| Message ID | 20190905122112.29672-2-ludovic.Barre@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: mmci: add busy detect for stm32 sdmmc variant | expand |
On Thu, 5 Sep 2019 at 14:21, Ludovic Barre <ludovic.Barre@st.com> wrote: > > From: Ludovic Barre <ludovic.barre@st.com> > > In some variants, the data timer starts and decrements > when the DPSM enters in Wait_R or Busy state > (while data transfer or MMC_RSP_BUSY), and generates a > data timeout error if the counter reach 0. > > -Define max_busy_timeout (in ms) according to clock. > -Set data timer register if the command has rsp_busy flag. > If busy_timeout is not defined by framework, the busy > length after Data Burst is defined as 1 second > (refer: 4.6.2.2 Write of sd specification part1 v6-0). How about re-phrasing this as below: ----- In the stm32_sdmmc variant, the datatimer is active not only during data transfers with the DPSM, but also while waiting for the busyend IRQs from commands having the MMC_RSP_BUSY flag set. This leads to an incorrect IRQ being raised to signal MCI_DATATIMEOUT error, which simply breaks the behaviour. Address this by updating the datatimer value before sending a command having the MMC_RSP_BUSY flag set. To inform the mmc core about the maximum supported busy timeout, which also depends on the current clock rate, set ->max_busy_timeout (in ms). ----- Regarding the busy_timeout, the core should really assign it a value for all commands having the RSP_BUSY flag set. However, I realize the core needs to be improved to cover all these cases - and I am looking at that, but not there yet. I would also suggest to use a greater value than 1s, as that seems a bit low for the "undefined" case. Perhaps use the max_busy_timeout, which would be nice a simple or 10s, which I think is used by some other drivers. > -Add MCI_DATATIMEOUT error management in mmci_cmd_irq. > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/mmc/host/mmci.c | 42 ++++++++++++++++++++++++++++++++++++----- > drivers/mmc/host/mmci.h | 3 +++ > 2 files changed, 40 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index c37e70dbe250..c30319255dc2 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -1075,6 +1075,7 @@ static void > mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) > { > void __iomem *base = host->base; > + unsigned long long clks; > > dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n", > cmd->opcode, cmd->arg, cmd->flags); > @@ -1097,6 +1098,16 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) > else > c |= host->variant->cmdreg_srsp; > } > + > + if (host->variant->busy_timeout && cmd->flags & MMC_RSP_BUSY) { > + if (!cmd->busy_timeout) > + cmd->busy_timeout = 1000; > + > + clks = (unsigned long long)cmd->busy_timeout * host->cclk; > + do_div(clks, MSEC_PER_SEC); > + writel_relaxed(clks, host->base + MMCIDATATIMER); > + } > + > if (/*interrupt*/0) > c |= MCI_CPSM_INTERRUPT; > > @@ -1201,6 +1212,7 @@ static void > mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > unsigned int status) > { > + u32 err_msk = MCI_CMDCRCFAIL | MCI_CMDTIMEOUT; > void __iomem *base = host->base; > bool sbc, busy_resp; > > @@ -1215,8 +1227,11 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > * handling. Note that we tag on any latent IRQs postponed > * due to waiting for busy status. > */ > - if (!((status|host->busy_status) & > - (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND))) > + if (host->variant->busy_timeout && busy_resp) > + err_msk |= MCI_DATATIMEOUT; > + > + if (!((status | host->busy_status) & > + (err_msk | MCI_CMDSENT | MCI_CMDRESPEND))) > return; > > /* Handle busy detection on DAT0 if the variant supports it. */ > @@ -1235,8 +1250,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > * while, to allow it to be set, but tests indicates that it > * isn't needed. > */ > - if (!host->busy_status && > - !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) && > + if (!host->busy_status && !(status & err_msk) && > (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) { > > writel(readl(base + MMCIMASK0) | > @@ -1290,6 +1304,9 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > cmd->error = -ETIMEDOUT; > } else if (status & MCI_CMDCRCFAIL && cmd->flags & MMC_RSP_CRC) { > cmd->error = -EILSEQ; > + } else if (host->variant->busy_timeout && busy_resp && > + status & MCI_DATATIMEOUT) { > + cmd->error = -ETIMEDOUT; It's not really clear to me what happens with the busy detection status bit (variant->busy_detect_flag), in case a MCI_DATATIMEOUT IRQ is raised, while also having host->busy_status set (waiting for busyend). By looking at the code a few lines above this, we may do a "return;" while waiting for the busyend IRQ even if MCI_DATATIMEOUT also is raised, potentially losing that from being caught. Is that really correct? > } else { > cmd->resp[0] = readl(base + MMCIRESPONSE0); > cmd->resp[1] = readl(base + MMCIRESPONSE1); > @@ -1583,6 +1600,20 @@ static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq) > spin_unlock_irqrestore(&host->lock, flags); > } > > +static void mmci_set_max_busy_timeout(struct mmc_host *mmc) > +{ > + struct mmci_host *host = mmc_priv(mmc); > + u32 max_busy_timeout = 0; > + > + if (!host->variant->busy_detect) > + return; > + > + if (host->variant->busy_timeout && mmc->actual_clock) > + max_busy_timeout = ~0UL / (mmc->actual_clock / MSEC_PER_SEC); > + > + mmc->max_busy_timeout = max_busy_timeout; > +} > + > static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > { > struct mmci_host *host = mmc_priv(mmc); > @@ -1687,6 +1718,8 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > else > mmci_set_clkreg(host, ios->clock); > > + mmci_set_max_busy_timeout(mmc); > + > if (host->ops && host->ops->set_pwrreg) > host->ops->set_pwrreg(host, pwr); > else > @@ -1957,7 +1990,6 @@ static int mmci_probe(struct amba_device *dev, > mmci_write_datactrlreg(host, > host->variant->busy_dpsm_flag); > mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; > - mmc->max_busy_timeout = 0; > } > > /* Prepare a CMD12 - needed to clear the DPSM on some variants. */ > diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h > index 833236ecb31e..d8b7f6774e8f 100644 > --- a/drivers/mmc/host/mmci.h > +++ b/drivers/mmc/host/mmci.h > @@ -287,6 +287,8 @@ struct mmci_host; > * @signal_direction: input/out direction of bus signals can be indicated > * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock > * @busy_detect: true if the variant supports busy detection on DAT0. > + * @busy_timeout: true if the variant starts data timer when the DPSM > + * enter in Wait_R or Busy state. > * @busy_dpsm_flag: bitmask enabling busy detection in the DPSM > * @busy_detect_flag: bitmask identifying the bit in the MMCISTATUS register > * indicating that the card is busy > @@ -333,6 +335,7 @@ struct variant_data { > u8 signal_direction:1; > u8 pwrreg_clkgate:1; > u8 busy_detect:1; > + u8 busy_timeout:1; > u32 busy_dpsm_flag; > u32 busy_detect_flag; > u32 busy_detect_mask; > -- > 2.17.1 > Kind regards Uffe
On Fri, 4 Oct 2019 at 08:12, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 5 Sep 2019 at 14:21, Ludovic Barre <ludovic.Barre@st.com> wrote: > > > > From: Ludovic Barre <ludovic.barre@st.com> > > > > In some variants, the data timer starts and decrements > > when the DPSM enters in Wait_R or Busy state > > (while data transfer or MMC_RSP_BUSY), and generates a > > data timeout error if the counter reach 0. > > > > > > -Define max_busy_timeout (in ms) according to clock. > > -Set data timer register if the command has rsp_busy flag. > > If busy_timeout is not defined by framework, the busy > > length after Data Burst is defined as 1 second > > (refer: 4.6.2.2 Write of sd specification part1 v6-0). > > How about re-phrasing this as below: > > ----- > In the stm32_sdmmc variant, the datatimer is active not only during > data transfers with the DPSM, but also while waiting for the busyend > IRQs from commands having the MMC_RSP_BUSY flag set. This leads to an > incorrect IRQ being raised to signal MCI_DATATIMEOUT error, which > simply breaks the behaviour. > > Address this by updating the datatimer value before sending a command > having the MMC_RSP_BUSY flag set. To inform the mmc core about the > maximum supported busy timeout, which also depends on the current > clock rate, set ->max_busy_timeout (in ms). > ----- > > Regarding the busy_timeout, the core should really assign it a value > for all commands having the RSP_BUSY flag set. However, I realize the > core needs to be improved to cover all these cases - and I am looking > at that, but not there yet. > > I would also suggest to use a greater value than 1s, as that seems a > bit low for the "undefined" case. Perhaps use the max_busy_timeout, > which would be nice a simple or 10s, which I think is used by some > other drivers. > > > -Add MCI_DATATIMEOUT error management in mmci_cmd_irq. > > > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > > --- > > drivers/mmc/host/mmci.c | 42 ++++++++++++++++++++++++++++++++++++----- > > drivers/mmc/host/mmci.h | 3 +++ > > 2 files changed, 40 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > > index c37e70dbe250..c30319255dc2 100644 > > --- a/drivers/mmc/host/mmci.c > > +++ b/drivers/mmc/host/mmci.c > > @@ -1075,6 +1075,7 @@ static void > > mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) > > { > > void __iomem *base = host->base; > > + unsigned long long clks; > > > > dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n", > > cmd->opcode, cmd->arg, cmd->flags); > > @@ -1097,6 +1098,16 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) > > else > > c |= host->variant->cmdreg_srsp; > > } > > + > > + if (host->variant->busy_timeout && cmd->flags & MMC_RSP_BUSY) { > > + if (!cmd->busy_timeout) > > + cmd->busy_timeout = 1000; > > + > > + clks = (unsigned long long)cmd->busy_timeout * host->cclk; > > + do_div(clks, MSEC_PER_SEC); > > + writel_relaxed(clks, host->base + MMCIDATATIMER); > > + } > > + > > if (/*interrupt*/0) > > c |= MCI_CPSM_INTERRUPT; > > > > @@ -1201,6 +1212,7 @@ static void > > mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > > unsigned int status) > > { > > + u32 err_msk = MCI_CMDCRCFAIL | MCI_CMDTIMEOUT; > > void __iomem *base = host->base; > > bool sbc, busy_resp; > > > > @@ -1215,8 +1227,11 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > > * handling. Note that we tag on any latent IRQs postponed > > * due to waiting for busy status. > > */ > > - if (!((status|host->busy_status) & > > - (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND))) > > + if (host->variant->busy_timeout && busy_resp) > > + err_msk |= MCI_DATATIMEOUT; > > + > > + if (!((status | host->busy_status) & > > + (err_msk | MCI_CMDSENT | MCI_CMDRESPEND))) > > return; > > > > /* Handle busy detection on DAT0 if the variant supports it. */ > > @@ -1235,8 +1250,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > > * while, to allow it to be set, but tests indicates that it > > * isn't needed. > > */ > > - if (!host->busy_status && > > - !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) && > > + if (!host->busy_status && !(status & err_msk) && > > (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) { > > > > writel(readl(base + MMCIMASK0) | > > @@ -1290,6 +1304,9 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > > cmd->error = -ETIMEDOUT; > > } else if (status & MCI_CMDCRCFAIL && cmd->flags & MMC_RSP_CRC) { > > cmd->error = -EILSEQ; > > + } else if (host->variant->busy_timeout && busy_resp && > > + status & MCI_DATATIMEOUT) { > > + cmd->error = -ETIMEDOUT; > > It's not really clear to me what happens with the busy detection > status bit (variant->busy_detect_flag), in case a MCI_DATATIMEOUT IRQ > is raised, while also having host->busy_status set (waiting for > busyend). > > By looking at the code a few lines above this, we may do a "return;" > while waiting for the busyend IRQ even if MCI_DATATIMEOUT also is > raised, potentially losing that from being caught. Is that really > correct? A second thought. That "return;" is to manage the busyend IRQ being raised of the first edge due to broken HW. So I guess, this isn't an issue for stm32_sdmmc variant after all? I have a look at the next patches in the series.. [...] Kind regards Uffe
hi Ulf Le 10/4/19 à 8:20 AM, Ulf Hansson a écrit : > On Fri, 4 Oct 2019 at 08:12, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> On Thu, 5 Sep 2019 at 14:21, Ludovic Barre <ludovic.Barre@st.com> wrote: >>> >>> From: Ludovic Barre <ludovic.barre@st.com> >>> >>> In some variants, the data timer starts and decrements >>> when the DPSM enters in Wait_R or Busy state >>> (while data transfer or MMC_RSP_BUSY), and generates a >>> data timeout error if the counter reach 0. >> >> >>> >>> -Define max_busy_timeout (in ms) according to clock. >>> -Set data timer register if the command has rsp_busy flag. >>> If busy_timeout is not defined by framework, the busy >>> length after Data Burst is defined as 1 second >>> (refer: 4.6.2.2 Write of sd specification part1 v6-0). >> >> How about re-phrasing this as below: >> >> ----- >> In the stm32_sdmmc variant, the datatimer is active not only during >> data transfers with the DPSM, but also while waiting for the busyend >> IRQs from commands having the MMC_RSP_BUSY flag set. This leads to an >> incorrect IRQ being raised to signal MCI_DATATIMEOUT error, which >> simply breaks the behaviour. >> >> Address this by updating the datatimer value before sending a command >> having the MMC_RSP_BUSY flag set. To inform the mmc core about the >> maximum supported busy timeout, which also depends on the current >> clock rate, set ->max_busy_timeout (in ms). Thanks for the re-phrasing. >> ----- >> >> Regarding the busy_timeout, the core should really assign it a value >> for all commands having the RSP_BUSY flag set. However, I realize the >> core needs to be improved to cover all these cases - and I am looking >> at that, but not there yet. >> >> I would also suggest to use a greater value than 1s, as that seems a >> bit low for the "undefined" case. Perhaps use the max_busy_timeout, >> which would be nice a simple or 10s, which I think is used by some >> other drivers. OK, I will set 10s, the max_busy_timeout could be very long for small frequencies (example, 25Mhz => 171s). >> >>> -Add MCI_DATATIMEOUT error management in mmci_cmd_irq. >>> >>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >>> --- >>> drivers/mmc/host/mmci.c | 42 ++++++++++++++++++++++++++++++++++++----- >>> drivers/mmc/host/mmci.h | 3 +++ >>> 2 files changed, 40 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >>> index c37e70dbe250..c30319255dc2 100644 >>> --- a/drivers/mmc/host/mmci.c >>> +++ b/drivers/mmc/host/mmci.c >>> @@ -1075,6 +1075,7 @@ static void >>> mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) >>> { >>> void __iomem *base = host->base; >>> + unsigned long long clks; >>> >>> dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n", >>> cmd->opcode, cmd->arg, cmd->flags); >>> @@ -1097,6 +1098,16 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) >>> else >>> c |= host->variant->cmdreg_srsp; >>> } >>> + >>> + if (host->variant->busy_timeout && cmd->flags & MMC_RSP_BUSY) { >>> + if (!cmd->busy_timeout) >>> + cmd->busy_timeout = 1000; >>> + >>> + clks = (unsigned long long)cmd->busy_timeout * host->cclk; >>> + do_div(clks, MSEC_PER_SEC); >>> + writel_relaxed(clks, host->base + MMCIDATATIMER); >>> + } >>> + >>> if (/*interrupt*/0) >>> c |= MCI_CPSM_INTERRUPT; >>> >>> @@ -1201,6 +1212,7 @@ static void >>> mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >>> unsigned int status) >>> { >>> + u32 err_msk = MCI_CMDCRCFAIL | MCI_CMDTIMEOUT; >>> void __iomem *base = host->base; >>> bool sbc, busy_resp; >>> >>> @@ -1215,8 +1227,11 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >>> * handling. Note that we tag on any latent IRQs postponed >>> * due to waiting for busy status. >>> */ >>> - if (!((status|host->busy_status) & >>> - (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND))) >>> + if (host->variant->busy_timeout && busy_resp) >>> + err_msk |= MCI_DATATIMEOUT; >>> + >>> + if (!((status | host->busy_status) & >>> + (err_msk | MCI_CMDSENT | MCI_CMDRESPEND))) >>> return; >>> >>> /* Handle busy detection on DAT0 if the variant supports it. */ >>> @@ -1235,8 +1250,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >>> * while, to allow it to be set, but tests indicates that it >>> * isn't needed. >>> */ >>> - if (!host->busy_status && >>> - !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) && >>> + if (!host->busy_status && !(status & err_msk) && >>> (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) { >>> >>> writel(readl(base + MMCIMASK0) | >>> @@ -1290,6 +1304,9 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >>> cmd->error = -ETIMEDOUT; >>> } else if (status & MCI_CMDCRCFAIL && cmd->flags & MMC_RSP_CRC) { >>> cmd->error = -EILSEQ; >>> + } else if (host->variant->busy_timeout && busy_resp && >>> + status & MCI_DATATIMEOUT) { >>> + cmd->error = -ETIMEDOUT; >> >> It's not really clear to me what happens with the busy detection >> status bit (variant->busy_detect_flag), in case a MCI_DATATIMEOUT IRQ >> is raised, while also having host->busy_status set (waiting for >> busyend). >> >> By looking at the code a few lines above this, we may do a "return;" >> while waiting for the busyend IRQ even if MCI_DATATIMEOUT also is >> raised, potentially losing that from being caught. Is that really >> correct? > > A second thought. That "return;" is to manage the busyend IRQ being > raised of the first edge due to broken HW. So I guess, this isn't an > issue for stm32_sdmmc variant after all? > > I have a look at the next patches in the series.. you're referring to "return" of ? if (host->busy_status && (status & host->variant->busy_detect_flag)) { writel(host->variant->busy_detect_mask, host->base + MMCICLEAR); return; } For stm32 variant (in patch 3/3): the "busy completion" is released immediately if there is an error or busyd0end, and cleans: irq, busyd0end mask, busy_status variable. I could add similar action in patch 2/3 function: "ux500_busy_complete" static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk) { void __iomem *base = host->base; if (status & err_msk) goto complete; ... complete: /* specific action to clean busy detection, irq, mask, busy_status */ } what do you think about it? > > [...] > > Kind regards > Uffe >
On Fri, 4 Oct 2019 at 14:59, Ludovic BARRE <ludovic.barre@st.com> wrote: > > hi Ulf > > Le 10/4/19 à 8:20 AM, Ulf Hansson a écrit : > > On Fri, 4 Oct 2019 at 08:12, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> > >> On Thu, 5 Sep 2019 at 14:21, Ludovic Barre <ludovic.Barre@st.com> wrote: > >>> > >>> From: Ludovic Barre <ludovic.barre@st.com> > >>> > >>> In some variants, the data timer starts and decrements > >>> when the DPSM enters in Wait_R or Busy state > >>> (while data transfer or MMC_RSP_BUSY), and generates a > >>> data timeout error if the counter reach 0. > >> > >> > >>> > >>> -Define max_busy_timeout (in ms) according to clock. > >>> -Set data timer register if the command has rsp_busy flag. > >>> If busy_timeout is not defined by framework, the busy > >>> length after Data Burst is defined as 1 second > >>> (refer: 4.6.2.2 Write of sd specification part1 v6-0). > >> > >> How about re-phrasing this as below: > >> > >> ----- > >> In the stm32_sdmmc variant, the datatimer is active not only during > >> data transfers with the DPSM, but also while waiting for the busyend > >> IRQs from commands having the MMC_RSP_BUSY flag set. This leads to an > >> incorrect IRQ being raised to signal MCI_DATATIMEOUT error, which > >> simply breaks the behaviour. > >> > >> Address this by updating the datatimer value before sending a command > >> having the MMC_RSP_BUSY flag set. To inform the mmc core about the > >> maximum supported busy timeout, which also depends on the current > >> clock rate, set ->max_busy_timeout (in ms). > > Thanks for the re-phrasing. > > >> ----- > >> > >> Regarding the busy_timeout, the core should really assign it a value > >> for all commands having the RSP_BUSY flag set. However, I realize the > >> core needs to be improved to cover all these cases - and I am looking > >> at that, but not there yet. > >> > >> I would also suggest to use a greater value than 1s, as that seems a > >> bit low for the "undefined" case. Perhaps use the max_busy_timeout, > >> which would be nice a simple or 10s, which I think is used by some > >> other drivers. > > OK, I will set 10s, the max_busy_timeout could be very long for small > frequencies (example, 25Mhz => 171s). > > >> > >>> -Add MCI_DATATIMEOUT error management in mmci_cmd_irq. > >>> > >>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > >>> --- > >>> drivers/mmc/host/mmci.c | 42 ++++++++++++++++++++++++++++++++++++----- > >>> drivers/mmc/host/mmci.h | 3 +++ > >>> 2 files changed, 40 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > >>> index c37e70dbe250..c30319255dc2 100644 > >>> --- a/drivers/mmc/host/mmci.c > >>> +++ b/drivers/mmc/host/mmci.c > >>> @@ -1075,6 +1075,7 @@ static void > >>> mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) > >>> { > >>> void __iomem *base = host->base; > >>> + unsigned long long clks; > >>> > >>> dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n", > >>> cmd->opcode, cmd->arg, cmd->flags); > >>> @@ -1097,6 +1098,16 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) > >>> else > >>> c |= host->variant->cmdreg_srsp; > >>> } > >>> + > >>> + if (host->variant->busy_timeout && cmd->flags & MMC_RSP_BUSY) { > >>> + if (!cmd->busy_timeout) > >>> + cmd->busy_timeout = 1000; > >>> + > >>> + clks = (unsigned long long)cmd->busy_timeout * host->cclk; > >>> + do_div(clks, MSEC_PER_SEC); > >>> + writel_relaxed(clks, host->base + MMCIDATATIMER); > >>> + } > >>> + > >>> if (/*interrupt*/0) > >>> c |= MCI_CPSM_INTERRUPT; > >>> > >>> @@ -1201,6 +1212,7 @@ static void > >>> mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > >>> unsigned int status) > >>> { > >>> + u32 err_msk = MCI_CMDCRCFAIL | MCI_CMDTIMEOUT; > >>> void __iomem *base = host->base; > >>> bool sbc, busy_resp; > >>> > >>> @@ -1215,8 +1227,11 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > >>> * handling. Note that we tag on any latent IRQs postponed > >>> * due to waiting for busy status. > >>> */ > >>> - if (!((status|host->busy_status) & > >>> - (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND))) > >>> + if (host->variant->busy_timeout && busy_resp) > >>> + err_msk |= MCI_DATATIMEOUT; > >>> + > >>> + if (!((status | host->busy_status) & > >>> + (err_msk | MCI_CMDSENT | MCI_CMDRESPEND))) > >>> return; > >>> > >>> /* Handle busy detection on DAT0 if the variant supports it. */ > >>> @@ -1235,8 +1250,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > >>> * while, to allow it to be set, but tests indicates that it > >>> * isn't needed. > >>> */ > >>> - if (!host->busy_status && > >>> - !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) && > >>> + if (!host->busy_status && !(status & err_msk) && > >>> (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) { > >>> > >>> writel(readl(base + MMCIMASK0) | > >>> @@ -1290,6 +1304,9 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > >>> cmd->error = -ETIMEDOUT; > >>> } else if (status & MCI_CMDCRCFAIL && cmd->flags & MMC_RSP_CRC) { > >>> cmd->error = -EILSEQ; > >>> + } else if (host->variant->busy_timeout && busy_resp && > >>> + status & MCI_DATATIMEOUT) { > >>> + cmd->error = -ETIMEDOUT; > >> > >> It's not really clear to me what happens with the busy detection > >> status bit (variant->busy_detect_flag), in case a MCI_DATATIMEOUT IRQ > >> is raised, while also having host->busy_status set (waiting for > >> busyend). > >> > >> By looking at the code a few lines above this, we may do a "return;" > >> while waiting for the busyend IRQ even if MCI_DATATIMEOUT also is > >> raised, potentially losing that from being caught. Is that really > >> correct? > > > > A second thought. That "return;" is to manage the busyend IRQ being > > raised of the first edge due to broken HW. So I guess, this isn't an > > issue for stm32_sdmmc variant after all? > > > > I have a look at the next patches in the series.. > > you're referring to "return" of ? > if (host->busy_status && > (status & host->variant->busy_detect_flag)) { > writel(host->variant->busy_detect_mask, > host->base + MMCICLEAR); > return; > } > > For stm32 variant (in patch 3/3): the "busy completion" is > released immediately if there is an error or busyd0end, > and cleans: irq, busyd0end mask, busy_status variable. Right, thanks for clarifying! > > I could add similar action in patch 2/3 function: "ux500_busy_complete" > > static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 > err_msk) > { > void __iomem *base = host->base; > > if (status & err_msk) > goto complete; > ... > complete: > /* specific action to clean busy detection, irq, mask, busy_status */ > } > > what do you think about it? For the legacy variant, the MCI_DATATIMEOUT isn't an issue as it can't be raised while waiting for busyend. So, I think this is fine as is. Kind regards Uffe
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index c37e70dbe250..c30319255dc2 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -1075,6 +1075,7 @@ static void mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) { void __iomem *base = host->base; + unsigned long long clks; dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n", cmd->opcode, cmd->arg, cmd->flags); @@ -1097,6 +1098,16 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) else c |= host->variant->cmdreg_srsp; } + + if (host->variant->busy_timeout && cmd->flags & MMC_RSP_BUSY) { + if (!cmd->busy_timeout) + cmd->busy_timeout = 1000; + + clks = (unsigned long long)cmd->busy_timeout * host->cclk; + do_div(clks, MSEC_PER_SEC); + writel_relaxed(clks, host->base + MMCIDATATIMER); + } + if (/*interrupt*/0) c |= MCI_CPSM_INTERRUPT; @@ -1201,6 +1212,7 @@ static void mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, unsigned int status) { + u32 err_msk = MCI_CMDCRCFAIL | MCI_CMDTIMEOUT; void __iomem *base = host->base; bool sbc, busy_resp; @@ -1215,8 +1227,11 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, * handling. Note that we tag on any latent IRQs postponed * due to waiting for busy status. */ - if (!((status|host->busy_status) & - (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND))) + if (host->variant->busy_timeout && busy_resp) + err_msk |= MCI_DATATIMEOUT; + + if (!((status | host->busy_status) & + (err_msk | MCI_CMDSENT | MCI_CMDRESPEND))) return; /* Handle busy detection on DAT0 if the variant supports it. */ @@ -1235,8 +1250,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, * while, to allow it to be set, but tests indicates that it * isn't needed. */ - if (!host->busy_status && - !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) && + if (!host->busy_status && !(status & err_msk) && (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) { writel(readl(base + MMCIMASK0) | @@ -1290,6 +1304,9 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, cmd->error = -ETIMEDOUT; } else if (status & MCI_CMDCRCFAIL && cmd->flags & MMC_RSP_CRC) { cmd->error = -EILSEQ; + } else if (host->variant->busy_timeout && busy_resp && + status & MCI_DATATIMEOUT) { + cmd->error = -ETIMEDOUT; } else { cmd->resp[0] = readl(base + MMCIRESPONSE0); cmd->resp[1] = readl(base + MMCIRESPONSE1); @@ -1583,6 +1600,20 @@ static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq) spin_unlock_irqrestore(&host->lock, flags); } +static void mmci_set_max_busy_timeout(struct mmc_host *mmc) +{ + struct mmci_host *host = mmc_priv(mmc); + u32 max_busy_timeout = 0; + + if (!host->variant->busy_detect) + return; + + if (host->variant->busy_timeout && mmc->actual_clock) + max_busy_timeout = ~0UL / (mmc->actual_clock / MSEC_PER_SEC); + + mmc->max_busy_timeout = max_busy_timeout; +} + static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) { struct mmci_host *host = mmc_priv(mmc); @@ -1687,6 +1718,8 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) else mmci_set_clkreg(host, ios->clock); + mmci_set_max_busy_timeout(mmc); + if (host->ops && host->ops->set_pwrreg) host->ops->set_pwrreg(host, pwr); else @@ -1957,7 +1990,6 @@ static int mmci_probe(struct amba_device *dev, mmci_write_datactrlreg(host, host->variant->busy_dpsm_flag); mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; - mmc->max_busy_timeout = 0; } /* Prepare a CMD12 - needed to clear the DPSM on some variants. */ diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h index 833236ecb31e..d8b7f6774e8f 100644 --- a/drivers/mmc/host/mmci.h +++ b/drivers/mmc/host/mmci.h @@ -287,6 +287,8 @@ struct mmci_host; * @signal_direction: input/out direction of bus signals can be indicated * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock * @busy_detect: true if the variant supports busy detection on DAT0. + * @busy_timeout: true if the variant starts data timer when the DPSM + * enter in Wait_R or Busy state. * @busy_dpsm_flag: bitmask enabling busy detection in the DPSM * @busy_detect_flag: bitmask identifying the bit in the MMCISTATUS register * indicating that the card is busy @@ -333,6 +335,7 @@ struct variant_data { u8 signal_direction:1; u8 pwrreg_clkgate:1; u8 busy_detect:1; + u8 busy_timeout:1; u32 busy_dpsm_flag; u32 busy_detect_flag; u32 busy_detect_mask;