From: Ludovic Barre <[email protected]>
This patch series adds busy detect for stm32 sdmmc variant.
Some adaptations are required:
-On sdmmc the data timer is started on data transfert
and busy state, so we must add hardware busy timeout support.
-Add busy_complete callback at mmci_host_ops to allow to define
a specific busy completion by variant.
-Add sdmmc busy_complete callback.
V6:
-mmci_start_command: set datatimer only on rsp_busy flag
(remove host->mrq->data).
-move max_busy_timeout in set_ios callback.
-typo fix: err_msk, clks on one lines.
V5:
-Replaces !cmd->data to !host->mrq->data to avoid overwrite
of datatimer register by the first command (cmd23, without data) of
SBC request.
V4:
-Re-work with busy_complete callback
-In series, move "mmc: mmci: add hardware busy timeout feature" in
first to simplify busy_complete prototype with err_msk parameter.
V3:
-rebase on latest mmc next
-replace re-read by status parameter.
V2:
-mmci_cmd_irq cleanup in separate patch.
-simplify the busy_detect_flag exclude
-replace sdmmc specific comment in
"mmc: mmci: avoid fake busy polling in mmci_irq"
to focus on common behavior
Ludovic Barre (3):
mmc: mmci: add hardware busy timeout feature
mmc: mmci: add busy_complete callback
mmc: mmci: sdmmc: add busy_complete callback
drivers/mmc/host/mmci.c | 183 +++++++++++++++++-----------
drivers/mmc/host/mmci.h | 7 +-
drivers/mmc/host/mmci_stm32_sdmmc.c | 38 ++++++
3 files changed, 156 insertions(+), 72 deletions(-)
--
2.17.1
From: Ludovic Barre <[email protected]>
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).
-Add MCI_DATATIMEOUT error management in mmci_cmd_irq.
Signed-off-by: Ludovic Barre <[email protected]>
---
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;
} 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
From: Ludovic Barre <[email protected]>
This patch adds busy_completion callback at mmci_host_ops
to allow to define a specific busy completion by variant.
The legacy code corresponding to busy completion used
by ux500 variants is moved to ux500_busy_complete function.
The busy_detect boolean property is replaced by
busy_complete callback definition.
Signed-off-by: Ludovic Barre <[email protected]>
---
drivers/mmc/host/mmci.c | 142 +++++++++++++++++++++-------------------
drivers/mmc/host/mmci.h | 3 +-
2 files changed, 76 insertions(+), 69 deletions(-)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index c30319255dc2..e20164f4354d 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -44,6 +44,7 @@
#define DRIVER_NAME "mmci-pl18x"
static void mmci_variant_init(struct mmci_host *host);
+static void ux500_variant_init(struct mmci_host *host);
static void ux500v2_variant_init(struct mmci_host *host);
static unsigned int fmax = 515633;
@@ -175,7 +176,6 @@ static struct variant_data variant_ux500 = {
.f_max = 100000000,
.signal_direction = true,
.pwrreg_clkgate = true,
- .busy_detect = true,
.busy_dpsm_flag = MCI_DPSM_ST_BUSYMODE,
.busy_detect_flag = MCI_ST_CARDBUSY,
.busy_detect_mask = MCI_ST_BUSYENDMASK,
@@ -184,7 +184,7 @@ static struct variant_data variant_ux500 = {
.irq_pio_mask = MCI_IRQ_PIO_MASK,
.start_err = MCI_STARTBITERR,
.opendrain = MCI_OD,
- .init = mmci_variant_init,
+ .init = ux500_variant_init,
};
static struct variant_data variant_ux500v2 = {
@@ -208,7 +208,6 @@ static struct variant_data variant_ux500v2 = {
.f_max = 100000000,
.signal_direction = true,
.pwrreg_clkgate = true,
- .busy_detect = true,
.busy_dpsm_flag = MCI_DPSM_ST_BUSYMODE,
.busy_detect_flag = MCI_ST_CARDBUSY,
.busy_detect_mask = MCI_ST_BUSYENDMASK,
@@ -610,6 +609,67 @@ static u32 ux500v2_get_dctrl_cfg(struct mmci_host *host)
return MCI_DPSM_ENABLE | (host->data->blksz << 16);
}
+static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
+{
+ void __iomem *base = host->base;
+
+ /*
+ * Before unmasking for the busy end IRQ, confirm that the
+ * command was sent successfully. To keep track of having a
+ * command in-progress, waiting for busy signaling to end,
+ * store the status in host->busy_status.
+ *
+ * Note that, the card may need a couple of clock cycles before
+ * it starts signaling busy on DAT0, hence re-read the
+ * MMCISTATUS register here, to allow the busy bit to be set.
+ * Potentially we may even need to poll the register for a
+ * while, to allow it to be set, but tests indicates that it
+ * isn't needed.
+ */
+ if (!host->busy_status && !(status & err_msk) &&
+ (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
+ writel(readl(base + MMCIMASK0) |
+ host->variant->busy_detect_mask,
+ base + MMCIMASK0);
+
+ host->busy_status = status & (MCI_CMDSENT | MCI_CMDRESPEND);
+ return false;
+ }
+
+ /*
+ * If there is a command in-progress that has been successfully
+ * sent, then bail out if busy status is set and wait for the
+ * busy end IRQ.
+ *
+ * Note that, the HW triggers an IRQ on both edges while
+ * monitoring DAT0 for busy completion, but there is only one
+ * status bit in MMCISTATUS for the busy state. Therefore
+ * both the start and the end interrupts needs to be cleared,
+ * one after the other. So, clear the busy start IRQ here.
+ */
+ if (host->busy_status &&
+ (status & host->variant->busy_detect_flag)) {
+ writel(host->variant->busy_detect_mask, base + MMCICLEAR);
+ return false;
+ }
+
+ /*
+ * If there is a command in-progress that has been successfully
+ * sent and the busy bit isn't set, it means we have received
+ * the busy end IRQ. Clear and mask the IRQ, then continue to
+ * process the command.
+ */
+ if (host->busy_status) {
+ writel(host->variant->busy_detect_mask, base + MMCICLEAR);
+
+ writel(readl(base + MMCIMASK0) &
+ ~host->variant->busy_detect_mask, base + MMCIMASK0);
+ host->busy_status = 0;
+ }
+
+ return true;
+}
+
/*
* All the DMA operation mode stuff goes inside this ifdef.
* This assumes that you have a generic DMA device interface,
@@ -953,9 +1013,16 @@ void mmci_variant_init(struct mmci_host *host)
host->ops = &mmci_variant_ops;
}
+void ux500_variant_init(struct mmci_host *host)
+{
+ host->ops = &mmci_variant_ops;
+ host->ops->busy_complete = ux500_busy_complete;
+}
+
void ux500v2_variant_init(struct mmci_host *host)
{
host->ops = &mmci_variant_ops;
+ host->ops->busy_complete = ux500_busy_complete;
host->ops->get_datactrl_cfg = ux500v2_get_dctrl_cfg;
}
@@ -1235,68 +1302,9 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
return;
/* Handle busy detection on DAT0 if the variant supports it. */
- if (busy_resp && host->variant->busy_detect) {
-
- /*
- * Before unmasking for the busy end IRQ, confirm that the
- * command was sent successfully. To keep track of having a
- * command in-progress, waiting for busy signaling to end,
- * store the status in host->busy_status.
- *
- * Note that, the card may need a couple of clock cycles before
- * it starts signaling busy on DAT0, hence re-read the
- * MMCISTATUS register here, to allow the busy bit to be set.
- * Potentially we may even need to poll the register for a
- * while, to allow it to be set, but tests indicates that it
- * isn't needed.
- */
- if (!host->busy_status && !(status & err_msk) &&
- (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
-
- writel(readl(base + MMCIMASK0) |
- host->variant->busy_detect_mask,
- base + MMCIMASK0);
-
- host->busy_status =
- status & (MCI_CMDSENT|MCI_CMDRESPEND);
- return;
- }
-
- /*
- * If there is a command in-progress that has been successfully
- * sent, then bail out if busy status is set and wait for the
- * busy end IRQ.
- *
- * Note that, the HW triggers an IRQ on both edges while
- * monitoring DAT0 for busy completion, but there is only one
- * status bit in MMCISTATUS for the busy state. Therefore
- * both the start and the end interrupts needs to be cleared,
- * one after the other. So, clear the busy start IRQ here.
- */
- if (host->busy_status &&
- (status & host->variant->busy_detect_flag)) {
- writel(host->variant->busy_detect_mask,
- host->base + MMCICLEAR);
+ if (busy_resp && host->ops->busy_complete)
+ if (!host->ops->busy_complete(host, status, err_msk))
return;
- }
-
- /*
- * If there is a command in-progress that has been successfully
- * sent and the busy bit isn't set, it means we have received
- * the busy end IRQ. Clear and mask the IRQ, then continue to
- * process the command.
- */
- if (host->busy_status) {
-
- writel(host->variant->busy_detect_mask,
- host->base + MMCICLEAR);
-
- writel(readl(base + MMCIMASK0) &
- ~host->variant->busy_detect_mask,
- base + MMCIMASK0);
- host->busy_status = 0;
- }
- }
host->cmd = NULL;
@@ -1537,7 +1545,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
* clear the corresponding IRQ.
*/
status &= readl(host->base + MMCIMASK0);
- if (host->variant->busy_detect)
+ if (host->ops->busy_complete)
writel(status & ~host->variant->busy_detect_mask,
host->base + MMCICLEAR);
else
@@ -1605,7 +1613,7 @@ 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)
+ if (!host->ops->busy_complete)
return;
if (host->variant->busy_timeout && mmc->actual_clock)
@@ -1980,7 +1988,7 @@ static int mmci_probe(struct amba_device *dev,
/*
* Enable busy detection.
*/
- if (variant->busy_detect) {
+ if (host->ops->busy_complete) {
mmci_ops.card_busy = mmci_card_busy;
/*
* Not all variants have a flag to enable busy detection
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index d8b7f6774e8f..733f9a035b06 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -286,7 +286,6 @@ struct mmci_host;
* @f_max: maximum clk frequency supported by the controller.
* @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
@@ -334,7 +333,6 @@ struct variant_data {
u32 f_max;
u8 signal_direction:1;
u8 pwrreg_clkgate:1;
- u8 busy_detect:1;
u8 busy_timeout:1;
u32 busy_dpsm_flag;
u32 busy_detect_flag;
@@ -369,6 +367,7 @@ struct mmci_host_ops {
void (*dma_error)(struct mmci_host *host);
void (*set_clkreg)(struct mmci_host *host, unsigned int desired);
void (*set_pwrreg)(struct mmci_host *host, unsigned int pwr);
+ bool (*busy_complete)(struct mmci_host *host, u32 status, u32 err_msk);
};
struct mmci_host {
--
2.17.1
hi Ulf
Just a "gentleman ping" about this series and
https://lkml.org/lkml/2019/9/4/747
Regards
Ludo
Le 9/5/19 à 2:21 PM, Ludovic Barre a écrit :
> From: Ludovic Barre <[email protected]>
>
> This patch series adds busy detect for stm32 sdmmc variant.
> Some adaptations are required:
> -On sdmmc the data timer is started on data transfert
> and busy state, so we must add hardware busy timeout support.
> -Add busy_complete callback at mmci_host_ops to allow to define
> a specific busy completion by variant.
> -Add sdmmc busy_complete callback.
>
> V6:
> -mmci_start_command: set datatimer only on rsp_busy flag
> (remove host->mrq->data).
> -move max_busy_timeout in set_ios callback.
> -typo fix: err_msk, clks on one lines.
>
> V5:
> -Replaces !cmd->data to !host->mrq->data to avoid overwrite
> of datatimer register by the first command (cmd23, without data) of
> SBC request.
>
> V4:
> -Re-work with busy_complete callback
> -In series, move "mmc: mmci: add hardware busy timeout feature" in
> first to simplify busy_complete prototype with err_msk parameter.
>
> V3:
> -rebase on latest mmc next
> -replace re-read by status parameter.
>
> V2:
> -mmci_cmd_irq cleanup in separate patch.
> -simplify the busy_detect_flag exclude
> -replace sdmmc specific comment in
> "mmc: mmci: avoid fake busy polling in mmci_irq"
> to focus on common behavior
>
> Ludovic Barre (3):
> mmc: mmci: add hardware busy timeout feature
> mmc: mmci: add busy_complete callback
> mmc: mmci: sdmmc: add busy_complete callback
>
> drivers/mmc/host/mmci.c | 183 +++++++++++++++++-----------
> drivers/mmc/host/mmci.h | 7 +-
> drivers/mmc/host/mmci_stm32_sdmmc.c | 38 ++++++
> 3 files changed, 156 insertions(+), 72 deletions(-)
>
On Wed, 18 Sep 2019 at 11:33, Ludovic BARRE <[email protected]> wrote:
>
> hi Ulf
>
> Just a "gentleman ping" about this series and
> https://lkml.org/lkml/2019/9/4/747
Thanks for pinging, I will come to this as soon as I can. September
has been a busy month, being on the road most of the time.
Apologize for the delays!
[...]
Kind regards
Uffe
On Thu, 5 Sep 2019 at 14:21, Ludovic Barre <[email protected]> wrote:
>
> From: Ludovic Barre <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]> wrote:
>
> On Thu, 5 Sep 2019 at 14:21, Ludovic Barre <[email protected]> wrote:
> >
> > From: Ludovic Barre <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
On Thu, 5 Sep 2019 at 14:22, Ludovic Barre <[email protected]> wrote:
>
> From: Ludovic Barre <[email protected]>
>
> This patch adds busy_completion callback at mmci_host_ops
> to allow to define a specific busy completion by variant.
>
> The legacy code corresponding to busy completion used
> by ux500 variants is moved to ux500_busy_complete function.
>
> The busy_detect boolean property is replaced by
> busy_complete callback definition.
At this point I prefer to keep th busy_detect boolean property. It
makes the code a bit more consistent.
Although, I think in case busy_detect is set for the variant, the
variant also needs to assign the new ->busy_completion() callback. In
other words, we don't need to check for a valid callback in code if
busy_detect is set.
Otherwise, this looks good to me!
Kind regards
Uffe
>
> Signed-off-by: Ludovic Barre <[email protected]>
> ---
> drivers/mmc/host/mmci.c | 142 +++++++++++++++++++++-------------------
> drivers/mmc/host/mmci.h | 3 +-
> 2 files changed, 76 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index c30319255dc2..e20164f4354d 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -44,6 +44,7 @@
> #define DRIVER_NAME "mmci-pl18x"
>
> static void mmci_variant_init(struct mmci_host *host);
> +static void ux500_variant_init(struct mmci_host *host);
> static void ux500v2_variant_init(struct mmci_host *host);
>
> static unsigned int fmax = 515633;
> @@ -175,7 +176,6 @@ static struct variant_data variant_ux500 = {
> .f_max = 100000000,
> .signal_direction = true,
> .pwrreg_clkgate = true,
> - .busy_detect = true,
> .busy_dpsm_flag = MCI_DPSM_ST_BUSYMODE,
> .busy_detect_flag = MCI_ST_CARDBUSY,
> .busy_detect_mask = MCI_ST_BUSYENDMASK,
> @@ -184,7 +184,7 @@ static struct variant_data variant_ux500 = {
> .irq_pio_mask = MCI_IRQ_PIO_MASK,
> .start_err = MCI_STARTBITERR,
> .opendrain = MCI_OD,
> - .init = mmci_variant_init,
> + .init = ux500_variant_init,
> };
>
> static struct variant_data variant_ux500v2 = {
> @@ -208,7 +208,6 @@ static struct variant_data variant_ux500v2 = {
> .f_max = 100000000,
> .signal_direction = true,
> .pwrreg_clkgate = true,
> - .busy_detect = true,
> .busy_dpsm_flag = MCI_DPSM_ST_BUSYMODE,
> .busy_detect_flag = MCI_ST_CARDBUSY,
> .busy_detect_mask = MCI_ST_BUSYENDMASK,
> @@ -610,6 +609,67 @@ static u32 ux500v2_get_dctrl_cfg(struct mmci_host *host)
> return MCI_DPSM_ENABLE | (host->data->blksz << 16);
> }
>
> +static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> +{
> + void __iomem *base = host->base;
> +
> + /*
> + * Before unmasking for the busy end IRQ, confirm that the
> + * command was sent successfully. To keep track of having a
> + * command in-progress, waiting for busy signaling to end,
> + * store the status in host->busy_status.
> + *
> + * Note that, the card may need a couple of clock cycles before
> + * it starts signaling busy on DAT0, hence re-read the
> + * MMCISTATUS register here, to allow the busy bit to be set.
> + * Potentially we may even need to poll the register for a
> + * while, to allow it to be set, but tests indicates that it
> + * isn't needed.
> + */
> + if (!host->busy_status && !(status & err_msk) &&
> + (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> + writel(readl(base + MMCIMASK0) |
> + host->variant->busy_detect_mask,
> + base + MMCIMASK0);
> +
> + host->busy_status = status & (MCI_CMDSENT | MCI_CMDRESPEND);
> + return false;
> + }
> +
> + /*
> + * If there is a command in-progress that has been successfully
> + * sent, then bail out if busy status is set and wait for the
> + * busy end IRQ.
> + *
> + * Note that, the HW triggers an IRQ on both edges while
> + * monitoring DAT0 for busy completion, but there is only one
> + * status bit in MMCISTATUS for the busy state. Therefore
> + * both the start and the end interrupts needs to be cleared,
> + * one after the other. So, clear the busy start IRQ here.
> + */
> + if (host->busy_status &&
> + (status & host->variant->busy_detect_flag)) {
> + writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> + return false;
> + }
> +
> + /*
> + * If there is a command in-progress that has been successfully
> + * sent and the busy bit isn't set, it means we have received
> + * the busy end IRQ. Clear and mask the IRQ, then continue to
> + * process the command.
> + */
> + if (host->busy_status) {
> + writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> +
> + writel(readl(base + MMCIMASK0) &
> + ~host->variant->busy_detect_mask, base + MMCIMASK0);
> + host->busy_status = 0;
> + }
> +
> + return true;
> +}
> +
> /*
> * All the DMA operation mode stuff goes inside this ifdef.
> * This assumes that you have a generic DMA device interface,
> @@ -953,9 +1013,16 @@ void mmci_variant_init(struct mmci_host *host)
> host->ops = &mmci_variant_ops;
> }
>
> +void ux500_variant_init(struct mmci_host *host)
> +{
> + host->ops = &mmci_variant_ops;
> + host->ops->busy_complete = ux500_busy_complete;
> +}
> +
> void ux500v2_variant_init(struct mmci_host *host)
> {
> host->ops = &mmci_variant_ops;
> + host->ops->busy_complete = ux500_busy_complete;
> host->ops->get_datactrl_cfg = ux500v2_get_dctrl_cfg;
> }
>
> @@ -1235,68 +1302,9 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> return;
>
> /* Handle busy detection on DAT0 if the variant supports it. */
> - if (busy_resp && host->variant->busy_detect) {
> -
> - /*
> - * Before unmasking for the busy end IRQ, confirm that the
> - * command was sent successfully. To keep track of having a
> - * command in-progress, waiting for busy signaling to end,
> - * store the status in host->busy_status.
> - *
> - * Note that, the card may need a couple of clock cycles before
> - * it starts signaling busy on DAT0, hence re-read the
> - * MMCISTATUS register here, to allow the busy bit to be set.
> - * Potentially we may even need to poll the register for a
> - * while, to allow it to be set, but tests indicates that it
> - * isn't needed.
> - */
> - if (!host->busy_status && !(status & err_msk) &&
> - (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> -
> - writel(readl(base + MMCIMASK0) |
> - host->variant->busy_detect_mask,
> - base + MMCIMASK0);
> -
> - host->busy_status =
> - status & (MCI_CMDSENT|MCI_CMDRESPEND);
> - return;
> - }
> -
> - /*
> - * If there is a command in-progress that has been successfully
> - * sent, then bail out if busy status is set and wait for the
> - * busy end IRQ.
> - *
> - * Note that, the HW triggers an IRQ on both edges while
> - * monitoring DAT0 for busy completion, but there is only one
> - * status bit in MMCISTATUS for the busy state. Therefore
> - * both the start and the end interrupts needs to be cleared,
> - * one after the other. So, clear the busy start IRQ here.
> - */
> - if (host->busy_status &&
> - (status & host->variant->busy_detect_flag)) {
> - writel(host->variant->busy_detect_mask,
> - host->base + MMCICLEAR);
> + if (busy_resp && host->ops->busy_complete)
> + if (!host->ops->busy_complete(host, status, err_msk))
> return;
> - }
> -
> - /*
> - * If there is a command in-progress that has been successfully
> - * sent and the busy bit isn't set, it means we have received
> - * the busy end IRQ. Clear and mask the IRQ, then continue to
> - * process the command.
> - */
> - if (host->busy_status) {
> -
> - writel(host->variant->busy_detect_mask,
> - host->base + MMCICLEAR);
> -
> - writel(readl(base + MMCIMASK0) &
> - ~host->variant->busy_detect_mask,
> - base + MMCIMASK0);
> - host->busy_status = 0;
> - }
> - }
>
> host->cmd = NULL;
>
> @@ -1537,7 +1545,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> * clear the corresponding IRQ.
> */
> status &= readl(host->base + MMCIMASK0);
> - if (host->variant->busy_detect)
> + if (host->ops->busy_complete)
> writel(status & ~host->variant->busy_detect_mask,
> host->base + MMCICLEAR);
> else
> @@ -1605,7 +1613,7 @@ 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)
> + if (!host->ops->busy_complete)
> return;
>
> if (host->variant->busy_timeout && mmc->actual_clock)
> @@ -1980,7 +1988,7 @@ static int mmci_probe(struct amba_device *dev,
> /*
> * Enable busy detection.
> */
> - if (variant->busy_detect) {
> + if (host->ops->busy_complete) {
> mmci_ops.card_busy = mmci_card_busy;
> /*
> * Not all variants have a flag to enable busy detection
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index d8b7f6774e8f..733f9a035b06 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -286,7 +286,6 @@ struct mmci_host;
> * @f_max: maximum clk frequency supported by the controller.
> * @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
> @@ -334,7 +333,6 @@ struct variant_data {
> u32 f_max;
> u8 signal_direction:1;
> u8 pwrreg_clkgate:1;
> - u8 busy_detect:1;
> u8 busy_timeout:1;
> u32 busy_dpsm_flag;
> u32 busy_detect_flag;
> @@ -369,6 +367,7 @@ struct mmci_host_ops {
> void (*dma_error)(struct mmci_host *host);
> void (*set_clkreg)(struct mmci_host *host, unsigned int desired);
> void (*set_pwrreg)(struct mmci_host *host, unsigned int pwr);
> + bool (*busy_complete)(struct mmci_host *host, u32 status, u32 err_msk);
> };
>
> struct mmci_host {
> --
> 2.17.1
>
hi Ulf
Le 10/4/19 à 8:20 AM, Ulf Hansson a écrit :
> On Fri, 4 Oct 2019 at 08:12, Ulf Hansson <[email protected]> wrote:
>>
>> On Thu, 5 Sep 2019 at 14:21, Ludovic Barre <[email protected]> wrote:
>>>
>>> From: Ludovic Barre <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]> wrote:
>
> hi Ulf
>
> Le 10/4/19 à 8:20 AM, Ulf Hansson a écrit :
> > On Fri, 4 Oct 2019 at 08:12, Ulf Hansson <[email protected]> wrote:
> >>
> >> On Thu, 5 Sep 2019 at 14:21, Ludovic Barre <[email protected]> wrote:
> >>>
> >>> From: Ludovic Barre <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> ---
> >>> 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