Skip to contentSkip to conversation

dw_mmc: add support for pre_req and post_req

PATCH In review Jaehoon Chung 2 participants 5 messages Apr 7, 2011 – Apr 18, 2011

Subsystems (2)
MMC, SD and SDIO Synopsys DesignWare MMC/SD/SDIO driver
lore
Share & tools
lore

Jaehoon Chung

author maintainer
#0 lore raw

PATCH 1/1 dw_mmc: add support for pre_req and post_req

This patch is based on Per Forlin's patches.
([PATCH v2 00/12]mmc: use nonblock mmc requests to minimize latency) After applied Per Forlin's patches, this patch must be apply. In dw_mmc.c, support for pre/post_reqeust().

Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by kyungmin Park <[email protected]>
---
 drivers/mmc/host/dw_mmc.c  |   84 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/mmc/dw_mmc.h |    7 ++++
 2 files changed, 84 insertions(+), 7 deletions(-)
drivers/mmc/host/dw_mmc.c +77 -7
index 87e1f57..67eeb85 100644
@@ -417,6 +417,42 @@ static int dw_mci_idmac_init(struct dw_mci *host)
 	return 0;
 }
 
+static unsigned int dw_mci_pre_dma_transfer(struct dw_mci *host,
+			struct mmc_data *data, struct dw_mci_next *next)
+{
+	unsigned int sg_len;
+
+	BUG_ON(next && data->host_cookie);
+	BUG_ON(!next && data->host_cookie &&
+			data->host_cookie != host->next_data.cookie);
+
+	if (!next && data->host_cookie &&
+			data->host_cookie != host->next_data.cookie) {
+		data->host_cookie = 0;
+	}
+
+	if (next ||
+		(!next && data->host_cookie != host->next_data.cookie)) {
+		sg_len = dma_map_sg(&host->pdev->dev, data->sg,
+				data->sg_len, ((data->flags & MMC_DATA_WRITE)
+			      ? DMA_TO_DEVICE : DMA_FROM_DEVICE));
+	} else {
+		sg_len = host->next_data.sg_len;
+		host->next_data.sg_len = 0;
+	}
+
+	if (sg_len == 0)
+		return -EINVAL;
+
+	if (next) {
+		next->sg_len = sg_len;
+		data->host_cookie = ++next->cookie < 0 ? 1 : next->cookie;
+	} else
+		data->sg_len = sg_len;
+
+	return sg_len;
+}
+
 static struct dw_mci_dma_ops dw_mci_idmac_ops = {
 	.init = dw_mci_idmac_init,
 	.start = dw_mci_idmac_start_dma,

@@ -451,13 +487,9 @@ static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data)
 			return -EINVAL;
 	}
 
-	if (data->flags & MMC_DATA_READ)
-		direction = DMA_FROM_DEVICE;
-	else
-		direction = DMA_TO_DEVICE;
-
-	sg_len = dma_map_sg(&host->pdev->dev, data->sg, data->sg_len,
-			    direction);
+	sg_len = dw_mci_pre_dma_transfer(host, data, NULL);
+	if (sg_len < 0)
+		return sg_len;
 
 	dev_vdbg(&host->pdev->dev,
 		 "sd sg_cpu: %#lx sg_dma: %#lx sg_len: %d\n",

@@ -643,6 +675,42 @@ static void dw_mci_queue_request(struct dw_mci *host, struct dw_mci_slot *slot,
 	spin_unlock_bh(&host->lock);
 }
 
+static void dw_mci_post_request(struct mmc_host *mmc, struct mmc_request *mrq,
+		int err)
+{
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+	struct mmc_data *data = mrq->data;
+
+	if (!data)
+		return;
+
+	if (slot->host->use_dma) {
+		dma_unmap_sg(&slot->host->pdev->dev, data->sg, data->sg_len,
+			     ((data->flags & MMC_DATA_WRITE)
+			      ? DMA_TO_DEVICE : DMA_FROM_DEVICE));
+
+		data->host_cookie = 0;
+	}
+}
+
+static void dw_mci_pre_request(struct mmc_host *mmc, struct mmc_request *mrq,
+		bool is_first_req)
+{
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+	struct mmc_data *data = mrq->data;
+
+	if (!data)
+		return;
+
+	BUG_ON(mrq->data->host_cookie);
+
+	if (slot->host->use_dma) {
+		if (dw_mci_pre_dma_transfer(slot->host, mrq->data,
+					&slot->host->next_data))
+			mrq->data->host_cookie = 0;
+	}
+}
+
 static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 {
 	struct dw_mci_slot *slot = mmc_priv(mmc);

@@ -748,6 +816,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 
 static const struct mmc_host_ops dw_mci_ops = {
 	.request	= dw_mci_request,
+	.pre_req	= dw_mci_pre_request,
+	.post_req	= dw_mci_post_request,
 	.set_ios	= dw_mci_set_ios,
 	.get_ro		= dw_mci_get_ro,
 	.get_cd		= dw_mci_get_cd,
include/linux/mmc/dw_mmc.h +7 -0
index c0207a7..dca82ee 100644
@@ -35,6 +35,11 @@ enum {
 
 struct mmc_data;
 
+struct dw_mci_next {
+	unsigned int	sg_len;
+	s32		cookie;
+};
+
 /**
  * struct dw_mci - MMC controller state shared between all slots
  * @lock: Spinlock protecting the queue and associated data.

@@ -154,6 +159,8 @@ struct dw_mci {
 	u32			quirks;
 
 	struct regulator	*vmmc;	/* Power regulator */
+
+	struct dw_mci_next	next_data;
 };
 
 /* DMA ops for Internal/External DMAC interface */
#1 lore raw

Hi Jaehoon,

On Thu, Apr 07, 2011 at 05:04:52PM +0900, Jaehoon Chung wrote:
[...]

Quoted 4 lines
drivers/mmc/host/dw_mmc.c +4 -0
+static unsigned int dw_mci_pre_dma_transfer(struct dw_mci *host,
+			struct mmc_data *data, struct dw_mci_next *next)
+{
+	unsigned int sg_len;

drivers/mmc/host/dw_mmc.c static int dw_mci_idmac_init(struct dw_mci *host) · line 433

Quoted 10 lines
drivers/mmc/host/dw_mmc.c +10 -0
+
+	BUG_ON(next && data->host_cookie);
+	BUG_ON(!next && data->host_cookie &&
+			data->host_cookie != host->next_data.cookie);
+
+	if (!next && data->host_cookie &&
+			data->host_cookie != host->next_data.cookie) {
+		data->host_cookie = 0;
+	}
+

I'm unsure if the 'if' statement makes any sense here, since the exactly same conditions have been caught by the BUG_ON just above it.

Quoted 12 lines
drivers/mmc/host/dw_mmc.c +12 -0
+	if (next ||
+		(!next && data->host_cookie != host->next_data.cookie)) {
+		sg_len = dma_map_sg(&host->pdev->dev, data->sg,
+				data->sg_len, ((data->flags & MMC_DATA_WRITE)
+			      ? DMA_TO_DEVICE : DMA_FROM_DEVICE));
+	} else {
+		sg_len = host->next_data.sg_len;
+		host->next_data.sg_len = 0;
+	}
+
+	if (sg_len == 0)
+		return -EINVAL;

drivers/mmc/host/dw_mmc.c static int dw_mci_idmac_init(struct dw_mci *host) · line 455

Quoted 10 lines
drivers/mmc/host/dw_mmc.c +10 -0
+
+	if (next) {
+		next->sg_len = sg_len;
+		data->host_cookie = ++next->cookie < 0 ? 1 : next->cookie;
+	} else
+		data->sg_len = sg_len;
+
+	return sg_len;
+}
+

Function dw_mci_pre_dma_transfer() returns non-zero value anyway, either -EINVAL or sg_len ...

[...]

Quoted 5 lines
drivers/mmc/host/dw_mmc.c +5 -0
+static void dw_mci_pre_request(struct mmc_host *mmc, struct mmc_request *mrq,
+		bool is_first_req)
+{
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+	struct mmc_data *data = mrq->data;

drivers/mmc/host/dw_mmc.c static void dw_mci_queue_request(struct dw_mci *host, struct dw_mci_slot *slot, · line 710

Quoted 10 lines
drivers/mmc/host/dw_mmc.c +10 -0
+
+	if (!data)
+		return;
+
+	BUG_ON(mrq->data->host_cookie);
+
+	if (slot->host->use_dma) {
+		if (dw_mci_pre_dma_transfer(slot->host, mrq->data,
+					&slot->host->next_data))
+			mrq->data->host_cookie = 0;

... while it steps back to old blocking way by setting data->host_cookie 0 when dw_mci_pre_dma_transfer returns non-zero.

Per my understanding, it means the non-blocking optimization will always get bypassed anyway, so I doubt the patch can really gain performance improvement. Did you get the chance to measure?

Signature · 2 lines
Regards,
Shawn
author maintainer
#2 lore raw

Hi Shawn..

Quoted 13 lines · Shawn Guo

Hi Jaehoon,

On Thu, Apr 07, 2011 at 05:04:52PM +0900, Jaehoon Chung wrote:
[...]

Quoted 9 lines
drivers/mmc/host/dw_mmc.c +9 -0
+static unsigned int dw_mci_pre_dma_transfer(struct dw_mci *host,
+			struct mmc_data *data, struct dw_mci_next *next)
+{
+	unsigned int sg_len;
+
+	BUG_ON(next && data->host_cookie);
+	BUG_ON(!next && data->host_cookie &&
+			data->host_cookie != host->next_data.cookie);
+
Quoted 9 lines

drivers/mmc/host/dw_mmc.c static int dw_mci_idmac_init(struct dw_mci *host) · line 433

Quoted 5 lines
drivers/mmc/host/dw_mmc.c +5 -0
+	if (!next && data->host_cookie &&
+			data->host_cookie != host->next_data.cookie) {
+		data->host_cookie = 0;
+	}
+

I'm unsure if the 'if' statement makes any sense here, since the exactly same conditions have been caught by the BUG_ON just above it.

You're right..i'll modify this..

Quoted 16 lines
Quoted 16 lines
drivers/mmc/host/dw_mmc.c +16 -0
+	if (next ||
+		(!next && data->host_cookie != host->next_data.cookie)) {
+		sg_len = dma_map_sg(&host->pdev->dev, data->sg,
+				data->sg_len, ((data->flags & MMC_DATA_WRITE)
+			      ? DMA_TO_DEVICE : DMA_FROM_DEVICE));
+	} else {
+		sg_len = host->next_data.sg_len;
+		host->next_data.sg_len = 0;
+	}
+
+	if (sg_len == 0)
+		return -EINVAL;
+
+	if (next) {
+		next->sg_len = sg_len;
+		data->host_cookie = ++next->cookie < 0 ? 1 : next->cookie;
Quoted 9 lines

drivers/mmc/host/dw_mmc.c static int dw_mci_idmac_init(struct dw_mci *host) · line 455

Quoted 6 lines
drivers/mmc/host/dw_mmc.c +6 -0
+	} else
+		data->sg_len = sg_len;
+
+	return sg_len;
+}
+

Function dw_mci_pre_dma_transfer() returns non-zero value anyway, either -EINVAL or sg_len ...

Sorry,, i didn't understand this your comments..

Quoted 19 lines

[...]

Quoted 5 lines
drivers/mmc/host/dw_mmc.c +5 -0
+static void dw_mci_pre_request(struct mmc_host *mmc, struct mmc_request *mrq,
+		bool is_first_req)
+{
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+	struct mmc_data *data = mrq->data;

drivers/mmc/host/dw_mmc.c static void dw_mci_queue_request(struct dw_mci *host, struct dw_mci_slot *slot, · line 710

Quoted 10 lines
drivers/mmc/host/dw_mmc.c +10 -0
+
+	if (!data)
+		return;
+
+	BUG_ON(mrq->data->host_cookie);
+
+	if (slot->host->use_dma) {
+		if (dw_mci_pre_dma_transfer(slot->host, mrq->data,
+					&slot->host->next_data))
+			mrq->data->host_cookie = 0;

... while it steps back to old blocking way by setting data->host_cookie 0 when dw_mci_pre_dma_transfer returns non-zero.

Quoted 4 lines

Per my understanding, it means the non-blocking optimization will always get bypassed anyway, so I doubt the patch can really gain performance improvement. Did you get the chance to measure?

Actually, i didn't get performance improvement, but didn't fully affect by CPU_FREQ. Somebody get performance improvement?

Regards,
Jaehoon Chung

#3 lore raw
Quoted 73 lines · Jaehoon Chung

Hi Shawn..

Quoted 13 lines · Shawn Guo

Hi Jaehoon,

On Thu, Apr 07, 2011 at 05:04:52PM +0900, Jaehoon Chung wrote:
[...]

Quoted 9 lines
drivers/mmc/host/dw_mmc.c +9 -0
+static unsigned int dw_mci_pre_dma_transfer(struct dw_mci *host,
+			struct mmc_data *data, struct dw_mci_next *next)
+{
+	unsigned int sg_len;
+
+	BUG_ON(next && data->host_cookie);
+	BUG_ON(!next && data->host_cookie &&
+			data->host_cookie != host->next_data.cookie);
+
Quoted 9 lines

drivers/mmc/host/dw_mmc.c static int dw_mci_idmac_init(struct dw_mci *host) · line 433

Quoted 5 lines
drivers/mmc/host/dw_mmc.c +5 -0
+	if (!next && data->host_cookie &&
+			data->host_cookie != host->next_data.cookie) {
+		data->host_cookie = 0;
+	}
+

I'm unsure if the 'if' statement makes any sense here, since the exactly same conditions have been caught by the BUG_ON just above it.

You're right..i'll modify this..

Quoted 16 lines
Quoted 16 lines
drivers/mmc/host/dw_mmc.c +16 -0
+	if (next ||
+		(!next && data->host_cookie != host->next_data.cookie)) {
+		sg_len = dma_map_sg(&host->pdev->dev, data->sg,
+				data->sg_len, ((data->flags & MMC_DATA_WRITE)
+			      ? DMA_TO_DEVICE : DMA_FROM_DEVICE));
+	} else {
+		sg_len = host->next_data.sg_len;
+		host->next_data.sg_len = 0;
+	}
+
+	if (sg_len == 0)
+		return -EINVAL;
+
+	if (next) {
+		next->sg_len = sg_len;
+		data->host_cookie = ++next->cookie < 0 ? 1 : next->cookie;
Quoted 9 lines

drivers/mmc/host/dw_mmc.c static int dw_mci_idmac_init(struct dw_mci *host) · line 455

Quoted 6 lines
drivers/mmc/host/dw_mmc.c +6 -0
+	} else
+		data->sg_len = sg_len;
+
+	return sg_len;
+}
+

Function dw_mci_pre_dma_transfer() returns non-zero value anyway, either -EINVAL or sg_len ...

Sorry,, i didn't understand this your comments..

Quoted 19 lines

[...]

Quoted 5 lines
drivers/mmc/host/dw_mmc.c +5 -0
+static void dw_mci_pre_request(struct mmc_host *mmc, struct mmc_request *mrq,
+		bool is_first_req)
+{
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+	struct mmc_data *data = mrq->data;

drivers/mmc/host/dw_mmc.c static void dw_mci_queue_request(struct dw_mci *host, struct dw_mci_slot *slot, · line 710

Quoted 10 lines
drivers/mmc/host/dw_mmc.c +10 -0
+
+	if (!data)
+		return;
+
+	BUG_ON(mrq->data->host_cookie);
+
+	if (slot->host->use_dma) {
+		if (dw_mci_pre_dma_transfer(slot->host, mrq->data,
+					&slot->host->next_data))
+			mrq->data->host_cookie = 0;

... while it steps back to old blocking way by setting data->host_cookie 0 when dw_mci_pre_dma_transfer returns non-zero.

Quoted 7 lines
Quoted 4 lines

Per my understanding, it means the non-blocking optimization will always get bypassed anyway, so I doubt the patch can really gain performance improvement. Did you get the chance to measure?

Actually, i didn't get performance improvement, but didn't fully affect by CPU_FREQ. Somebody get performance improvement?

Though the performance improvement is limited, I did see nearly 4% improvement with best case on mxs-mmc.

Try the following change in dw_mci_pre_request to see if you can see any performance difference.

	if (slot->host->use_dma) {
		if (dw_mci_pre_dma_transfer(slot->host, mrq->data,
					&slot->host->next_data) < 0)
			mrq->data->host_cookie = 0;
Signature · 2 lines
Regards,
Shawn
author maintainer
#4 lore raw

Hi Shawn,

Quoted 6 lines · Shawn Guo
	if (slot->host->use_dma) {
		if (dw_mci_pre_dma_transfer(slot->host, mrq->data,
					&slot->host->next_data) < 0)
			mrq->data->host_cookie = 0;

Ok..I understood yours..:) i'll check and test..
Thanks for your comment.

Regards,
Jaehoon Chung