[PATCH v3 0/3] Add hardware unbusy interrupt support for dw_mmc

[PATCH v3 0/3] Add hardware unbusy interrupt support for dw_mmc

[Shawn Lin]

DesignWare MMC controler lacks proper hardware wait
busy support, so the software must check the busy
state if the following tranfser need use data line.
And dw_mci_wait_while_busy() now use busy check mostly
with host's lock hold, and we can't relinguish CPU
at this moment, which makes it rather painful when used
in low-end platform. To solve this, Rockchip invent
a new hardware unbusy interrupt module, which could
generate interrupt if the state machine runs from
busy state into idle state.


Changes in v3:
- remove unused irqflags and check hw_unbusy_int in
  dw_mci_wait_hw_unbusy() as well.

Changes in v2:
- add tag from Ziyuan Xu
- remove lock for checking event since it's already atomic operation

Shawn Lin (3):
  mmc: dw_mmc: Check busy state in dw_mci_request()
  mmc: dw_mmc: Add hardware unbusy interrupt support
  mmc: dw_mmc-rockchip: Enable hardware unbusy interrupt support

 drivers/mmc/host/dw_mmc-rockchip.c | 30 ++++++++++++++++++++
 drivers/mmc/host/dw_mmc.c          | 56 +++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/dw_mmc.h          |  6 ++++
 3 files changed, 91 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH v3 1/3] mmc: dw_mmc: Check busy state in dw_mci_request()

[Shawn Lin]

Move it from dw_mci_start_command() to dw_mci_request().
Then dw_mci_wait_while_busy() isn't called with host's
lock hold.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Tested-by: Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v3: None
Changes in v2: None

 drivers/mmc/host/dw_mmc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 80dc2fd..703dedf 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -426,7 +426,6 @@ static void dw_mci_start_command(struct dw_mci *host,
 
 	mci_writel(host, CMDARG, cmd->arg);
 	wmb(); /* drain writebuffer */
-	dw_mci_wait_while_busy(host, cmd_flags);
 
 	mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START);
 
@@ -1419,6 +1418,10 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		return;
 	}
 
+	if ((mrq->cmd->opcode != MMC_SEND_STATUS && mrq->cmd->data) &&
+	    !(mrq->cmd->opcode == SD_SWITCH_VOLTAGE))
+		dw_mci_wait_while_busy(host, SDMMC_CMD_PRV_DAT_WAIT);
+
 	spin_lock_bh(&host->lock);
 
 	dw_mci_queue_request(host, slot, mrq);
-- 
1.9.1

[PATCH v3 2/3] mmc: dw_mmc: Add hardware unbusy interrupt support

[Shawn Lin]

So that we don't need to busy checking the busy state,
but relinguish the CPU and rely on the hareware interrupt
if available.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Tested-by: Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v3:
- remove unused irqflags and check hw_unbusy_int in
  dw_mci_wait_hw_unbusy() as well.

Changes in v2:
- add tag from Ziyuan Xu
- remove lock for checking event since it's already atomic operation

 drivers/mmc/host/dw_mmc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/dw_mmc.h |  6 ++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 703dedf..bb6b243 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -107,6 +107,8 @@ struct idmac_desc {
 /* Each descriptor can transfer up to 4KB of data in chained mode */
 #define DW_MCI_DESC_DATA_LENGTH	0x1000
 
+DECLARE_WAIT_QUEUE_HEAD(unbusy_waiter);
+
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)
 {
@@ -231,9 +233,34 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 	return true;
 }
 
+static inline int dw_mci_wait_hw_unbusy(struct dw_mci *host,
+					unsigned long timeout)
+{
+	int err;
+
+	set_bit(EVENT_UNBUSY_COMPLETE, &host->pending_events);
+	err = host->drv_data->prepare_hw_unbusy(host, true);
+	if (err)
+		return err;
+
+	wait_event_interruptible_timeout(unbusy_waiter,
+					 !test_bit(EVENT_UNBUSY_COMPLETE,
+						   &host->pending_events),
+					 timeout);
+
+	if (test_and_clear_bit(EVENT_UNBUSY_COMPLETE,
+			       &host->pending_events)) {
+		dev_err(host->dev, "Busy; trying anyway\n");
+		host->drv_data->prepare_hw_unbusy(host, false);
+	}
+
+	return 0;
+}
+
 static void dw_mci_wait_while_busy(struct dw_mci *host, u32 cmd_flags)
 {
 	u32 status;
+	int err;
 
 	/*
 	 * Databook says that before issuing a new data transfer command
@@ -245,6 +272,15 @@ static void dw_mci_wait_while_busy(struct dw_mci *host, u32 cmd_flags)
 	 */
 	if ((cmd_flags & SDMMC_CMD_PRV_DAT_WAIT) &&
 	    !(cmd_flags & SDMMC_CMD_VOLT_SWITCH)) {
+		/* Resort to hw unbusy interrupt first */
+		if (host->drv_data->prepare_hw_unbusy &&
+		    host->hw_unbusy_int != -EINVAL) {
+			err = dw_mci_wait_hw_unbusy(host,
+					jiffies + msecs_to_jiffies(500));
+			if (!err)
+				return;
+		}
+
 		if (readl_poll_timeout_atomic(host->regs + SDMMC_STATUS,
 					      status,
 					      !(status & SDMMC_STATUS_BUSY),
@@ -2734,6 +2770,14 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
 			dw_mci_handle_cd(host);
 		}
 
+		/* Check hardware unbusy interrupt */
+		if (host->hw_unbusy_int != -EINVAL &&
+		    pending & BIT(host->hw_unbusy_int)) {
+			mci_writel(host, RINTSTS, BIT(host->hw_unbusy_int));
+			clear_bit(EVENT_UNBUSY_COMPLETE, &host->pending_events);
+			wake_up_interruptible(&unbusy_waiter);
+		}
+
 		if (pending & SDMMC_INT_SDIO(slot->sdio_id)) {
 			mci_writel(host, RINTSTS,
 				   SDMMC_INT_SDIO(slot->sdio_id));
@@ -3249,6 +3293,8 @@ int dw_mci_probe(struct dw_mci *host)
 		reset_control_deassert(host->pdata->rstc);
 	}
 
+	host->hw_unbusy_int = -EINVAL;
+
 	if (drv_data && drv_data->init) {
 		ret = drv_data->init(host);
 		if (ret) {
@@ -3360,6 +3406,11 @@ int dw_mci_probe(struct dw_mci *host)
 	mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
 		   SDMMC_INT_TXDR | SDMMC_INT_RXDR |
 		   DW_MCI_ERROR_FLAGS);
+
+	if (host->hw_unbusy_int != -EINVAL)
+		mci_writel(host, INTMASK,
+			   mci_readl(host, INTMASK) | BIT(host->hw_unbusy_int));
+
 	/* Enable mci interrupt */
 	mci_writel(host, CTRL, SDMMC_CTRL_INT_ENABLE);
 
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 46e9f8e..fb4c173 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -33,6 +33,7 @@ enum dw_mci_state {
 
 enum {
 	EVENT_CMD_COMPLETE = 0,
+	EVENT_UNBUSY_COMPLETE,
 	EVENT_XFER_COMPLETE,
 	EVENT_DATA_COMPLETE,
 	EVENT_DATA_ERROR,
@@ -124,6 +125,7 @@ struct dw_mci_dma_slave {
  * @irq_flags: The flags to be passed to request_irq.
  * @irq: The irq value to be passed to request_irq.
  * @sdio_id0: Number of slot0 in the SDIO interrupt registers.
+ * @hw_unbusy_int: Number of unbusy interrupt in the interrupt registers.
  * @cmd11_timer: Timer for SD3.0 voltage switch over scheme.
  * @cto_timer: Timer for broken command transfer over scheme.
  * @dto_timer: Timer for broken data transfer over scheme.
@@ -230,6 +232,7 @@ struct dw_mci {
 	int			irq;
 
 	int			sdio_id0;
+	int                     hw_unbusy_int;
 
 	struct timer_list       cmd11_timer;
 	struct timer_list       cto_timer;
@@ -551,6 +554,8 @@ struct dw_mci_slot {
  * @set_ios: handle bus specific extensions.
  * @parse_dt: parse implementation specific device tree properties.
  * @execute_tuning: implementation specific tuning procedure.
+ * @prepare_hw_unbusy: implementation specific procedure for
+ *                    controlling hw unbusy interrupt.
  *
  * Provide controller implementation specific extensions. The usage of this
  * data structure is fully optional and usage of each member in this structure
@@ -565,6 +570,7 @@ struct dw_mci_drv_data {
 	int		(*execute_tuning)(struct dw_mci_slot *slot, u32 opcode);
 	int		(*prepare_hs400_tuning)(struct dw_mci *host,
 						struct mmc_ios *ios);
+	int             (*prepare_hw_unbusy)(struct dw_mci *host, bool enable);
 	int		(*switch_voltage)(struct mmc_host *mmc,
 					  struct mmc_ios *ios);
 };
-- 
1.9.1

[PATCH v3 3/3] mmc: dw_mmc-rockchip: Enable hardware unbusy interrupt support

[Shawn Lin]

The new register for controlling hardware unbusy interrupt
is in 0x120. It looks like:

|------------------------------------------------------------|
|Bit    |  Attribute |  Reset Value  | Description           |
|------------------------------------------------------------|
|31:25  |    RO      |      0x0      |  reserved             |
|------------------------------------------------------------|
|24     |    RO      |      0x0      |  rdyint_cnt_finish    |
|       |            |               |When high, it indicates|
|	|            |               |that the rdyint counter|
|       |            |               |is finished.           |
|------------------------------------------------------------|
|23:16  |    RO      |      0x0      |  rdyint_cnt_status    |
|       |            |               |Couner status, reflect |
|       |            |               |internal counter value.|
|------------------------------------------------------------|
|15:9   |    RO      |      0x0      |  reserved             |
|------------------------------------------------------------|
|8      |    RW      |      0x0      |  rdyint_gen_working   |
|       |            |               |When set, IP starts to |
|       |            |               |count and generate one |
|       |            |               |rdyint trigger. After  |
|       |            |               |the rdyint trigger is  |
|       |            |               |generated, it will be  |
|       |            |               |cleaned automatically. |
|       |            |               |Software should set it |
|       |            |               |again next time.       |
|------------------------------------------------------------|
|7:0    |    RW      |      0xff     |   rdyint_gen_maxval   |
|       |            |               |Max counter value for  |
|       |            |               |the IP to count when   |
|       |            |               |rdyint_gen_working is  |
|       |            |               |set. This counter is   |
|       |            |               |based on biu_clk.      |
|------------------------------------------------------------|

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Tested-by: Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v3: None
Changes in v2: None

 drivers/mmc/host/dw_mmc-rockchip.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 8c86a80..85b1e42 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -20,6 +20,9 @@
 #include "dw_mmc-pltfm.h"
 
 #define RK3288_CLKGEN_DIV       2
+#define RKMMC_RDYINT_GEN	0x120
+#define RKMMC_RDYINT_GEN_WORKING BIT(8)
+#define RKMMC_RDYINT_GEN_MAXVAL GENMASK(7, 0)
 
 struct dw_mci_rockchip_priv_data {
 	struct clk		*drv_clk;
@@ -28,6 +31,23 @@ struct dw_mci_rockchip_priv_data {
 	int			num_phases;
 };
 
+static int dw_mci_rockchip_prepare_hw_unbusy(struct dw_mci *host,
+					     bool enable)
+{
+	u32 reg = readl(host->regs + RKMMC_RDYINT_GEN);
+
+	if (enable)
+		/* Self-clean when generating unbusy int */
+		reg |= RKMMC_RDYINT_GEN_WORKING;
+	else
+		/* Otherwise do it manually to avoid racing condition */
+		reg &= ~RKMMC_RDYINT_GEN_WORKING;
+
+	writel(reg, host->regs + RKMMC_RDYINT_GEN);
+
+	return 0;
+}
+
 static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 {
 	struct dw_mci_rockchip_priv_data *priv = host->priv;
@@ -301,6 +321,15 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
 				    "rockchip,rk3288-dw-mshc"))
 		host->bus_hz /= RK3288_CLKGEN_DIV;
 
+	/* Some Rockchip SoCs use hw unbusy int */
+	if (of_device_is_compatible(host->dev->of_node,
+				    "rockchip,rk1808-dw-mshc")) {
+		host->hw_unbusy_int = 16;
+		writel(~(RKMMC_RDYINT_GEN_WORKING |
+			 RKMMC_RDYINT_GEN_MAXVAL),
+		       host->regs + RKMMC_RDYINT_GEN);
+	}
+
 	return 0;
 }
 
@@ -322,6 +351,7 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
 	.set_ios		= dw_mci_rk3288_set_ios,
 	.execute_tuning		= dw_mci_rk3288_execute_tuning,
 	.parse_dt		= dw_mci_rk3288_parse_dt,
+	.prepare_hw_unbusy      = dw_mci_rockchip_prepare_hw_unbusy,
 	.init			= dw_mci_rockchip_init,
 };
 
-- 
1.9.1

Re: [PATCH v3 1/3] mmc: dw_mmc: Check busy state in dw_mci_request()

[Ulf Hansson]

On Tue, 19 Mar 2019 at 10:14, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>
> Move it from dw_mci_start_command() to dw_mci_request().
> Then dw_mci_wait_while_busy() isn't called with host's
> lock hold.

So, I decided to have a closer look at this.

>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Tested-by: Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/mmc/host/dw_mmc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 80dc2fd..703dedf 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -426,7 +426,6 @@ static void dw_mci_start_command(struct dw_mci *host,
>
>         mci_writel(host, CMDARG, cmd->arg);
>         wmb(); /* drain writebuffer */
> -       dw_mci_wait_while_busy(host, cmd_flags);
>
>         mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START);
>
> @@ -1419,6 +1418,10 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>                 return;
>         }
>
> +       if ((mrq->cmd->opcode != MMC_SEND_STATUS && mrq->cmd->data) &&
> +           !(mrq->cmd->opcode == SD_SWITCH_VOLTAGE))
> +               dw_mci_wait_while_busy(host, SDMMC_CMD_PRV_DAT_WAIT);
> +

This looks weird, because according to the change-log it sounds like
you are moving things around to just avoid having the "lock" held.

To me, there seems to be more changes because of the new checks for
the "opcode" above, no?

 Moreover, dw_mci_wait_while_busy() is also called from
mci_send_cmd(), which means it may still be called with the "lock" is
held. That is really confusing and needs more explanation.

>         spin_lock_bh(&host->lock);
>
>         dw_mci_queue_request(host, slot, mrq);
> --
> 1.9.1
>
>
>

Finally, I am not sure I understand why dw_mci_wait_while_busy() is
called before starting a command, at all. That seems wrong to me.
Instead, shouldn't we look at cmd->flags & MMC_RSP_BUSY, for the
request in question and don't call mmc_request_done() for it, before
the card have stop signaled busy. At least that the behavior the mmc
core expects by the driver.

Kind regards
Uffe

Re: [PATCH v3 1/3] mmc: dw_mmc: Check busy state in dw_mci_request()【请注意，邮件由linux-mmc-owner@vger.kernel.org代发】

[Shawn Lin]

On 2019/3/19 22:41, Ulf Hansson wrote:
> On Tue, 19 Mar 2019 at 10:14, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>>
>> Move it from dw_mci_start_command() to dw_mci_request().
>> Then dw_mci_wait_while_busy() isn't called with host's
>> lock hold.
> 
> So, I decided to have a closer look at this.
> 
>>
>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> Tested-by: Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/mmc/host/dw_mmc.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 80dc2fd..703dedf 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -426,7 +426,6 @@ static void dw_mci_start_command(struct dw_mci *host,
>>
>>          mci_writel(host, CMDARG, cmd->arg);
>>          wmb(); /* drain writebuffer */
>> -       dw_mci_wait_while_busy(host, cmd_flags);
>>
>>          mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START);
>>
>> @@ -1419,6 +1418,10 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>                  return;
>>          }
>>
>> +       if ((mrq->cmd->opcode != MMC_SEND_STATUS && mrq->cmd->data) &&
>> +           !(mrq->cmd->opcode == SD_SWITCH_VOLTAGE))
>> +               dw_mci_wait_while_busy(host, SDMMC_CMD_PRV_DAT_WAIT);
>> +
> 
> This looks weird, because according to the change-log it sounds like
> you are moving things around to just avoid having the "lock" held.
> 
> To me, there seems to be more changes because of the new checks for
> the "opcode" above, no?

yes, that's what original dw_mci_wait_while_busy() did internally.

> 
>   Moreover, dw_mci_wait_while_busy() is also called from
> mci_send_cmd(), which means it may still be called with the "lock" is
> held. That is really confusing and needs more explanation.

dwmmc need to send a *NULL* cmd with SDMMC_CMD_UPD_CLK set in its CMD
register, in order to update bus clock, which is quite differnet from
others hosts. So mci_send_cmd()->dw_mci_wait_while_busy() is used for 
updating bus clock but not in the hot path. So the main concern is to
move dw_mci_wait_while_busy() out from normal data requst.


> 
>>          spin_lock_bh(&host->lock);
>>
>>          dw_mci_queue_request(host, slot, mrq);
>> --
>> 1.9.1
>>
>>
>>
> 
> Finally, I am not sure I understand why dw_mci_wait_while_busy() is
> called before starting a command, at all. That seems wrong to me.
> Instead, shouldn't we look at cmd->flags & MMC_RSP_BUSY, for the
> request in question and don't call mmc_request_done() for it, before
> the card have stop signaled busy. At least that the behavior the mmc
> core expects by the driver.
> 

yes, that was from commit 0bdbd0e88cf6b603("mmc: dw_mmc: Don't start 
commands while busy"), and it says "We'll leverage the existing dw_mmc
knowledge about whether it should wait for the previous command to
finish to know whether we should check for busy before sending the
command". I think the word, "leverage", explains the whole point, seems
a little tricky though.

> Kind regards
> Uffe
> 
>