mmc: mxs-mmc: add support for pre_req and post_req

Message ID 1303058010-30256-1-git-send-email-shawn.guo@linaro.org
State New
Headers show

Commit Message

Shawn Guo April 17, 2011, 4:33 p.m.
pre_req() runs dma_map_sg() post_req() runs dma_unmap_sg.
If not calling pre_req() before mxs_mmc_request(), request()
will prepare the cache just like it did it before.
It is optional to use pre_req() and post_req().

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/mmc/host/mxs-mmc.c |   75 ++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 72 insertions(+), 3 deletions(-)

Comments

Per Forlin April 20, 2011, 7:58 a.m. | #1
On 17 April 2011 18:33, Shawn Guo <shawn.guo@linaro.org> wrote:
> pre_req() runs dma_map_sg() post_req() runs dma_unmap_sg.
> If not calling pre_req() before mxs_mmc_request(), request()
> will prepare the cache just like it did it before.
> It is optional to use pre_req() and post_req().
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/mmc/host/mxs-mmc.c |   75 ++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 72 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> index 99d39a6..63c2ae2 100644
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
> @@ -137,6 +137,10 @@
>
>  #define SSP_PIO_NUM    3
>
> +struct mxs_mmc_next {
> +       s32 cookie;
> +};
> +
>  struct mxs_mmc_host {
>        struct mmc_host                 *mmc;
>        struct mmc_request              *mrq;
> @@ -154,6 +158,7 @@ struct mxs_mmc_host {
>        struct mxs_dma_data             dma_data;
>        unsigned int                    dma_dir;
>        u32                             ssp_pio_words[SSP_PIO_NUM];
> +       struct mxs_mmc_next             next_data;
>
>        unsigned int                    version;
>        unsigned char                   bus_width;
> @@ -302,6 +307,31 @@ static irqreturn_t mxs_mmc_irq_handler(int irq, void *dev_id)
>        return IRQ_HANDLED;
>  }
>
> +static int mxs_mmc_prep_dma_data(struct mxs_mmc_host *host,
> +                               struct mmc_data *data,
> +                               struct mxs_mmc_next *next)
> +{
> +       if (!next && data->host_cookie &&
> +           data->host_cookie != host->next_data.cookie) {
> +               printk(KERN_WARNING "[%s] invalid cookie: data->host_cookie %d"
> +                      " host->next_data.cookie %d\n",
> +                      __func__, data->host_cookie, host->next_data.cookie);
> +               data->host_cookie = 0;
> +       }
> +
> +       /* Check if next job is already prepared */
> +       if (next || (!next && data->host_cookie != host->next_data.cookie))
> +               if (dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
> +                              (data->flags & MMC_DATA_WRITE) ?
> +                              DMA_TO_DEVICE : DMA_FROM_DEVICE) == 0)
> +                       return -EINVAL;
> +
> +       if (next)
> +               data->host_cookie = ++next->cookie < 0 ? 1 : next->cookie;
> +
> +       return 0;
> +}
> +
>  static struct dma_async_tx_descriptor *mxs_mmc_prep_dma(
>        struct mxs_mmc_host *host, unsigned int append)
>  {
> @@ -312,8 +342,8 @@ static struct dma_async_tx_descriptor *mxs_mmc_prep_dma(
>
>        if (data) {
>                /* data */
> -               dma_map_sg(mmc_dev(host->mmc), data->sg,
> -                          data->sg_len, host->dma_dir);
> +               if (mxs_mmc_prep_dma_data(host, data, NULL))
> +                       return NULL;
>                sgl = data->sg;
>                sg_len = data->sg_len;
>        } else {
> @@ -328,9 +358,11 @@ static struct dma_async_tx_descriptor *mxs_mmc_prep_dma(
>                desc->callback = mxs_mmc_dma_irq_callback;
>                desc->callback_param = host;
>        } else {
> -               if (data)
> +               if (data) {
>                        dma_unmap_sg(mmc_dev(host->mmc), data->sg,
>                                     data->sg_len, host->dma_dir);
> +                       data->host_cookie = 0;
> +               }
When is dma_unmap_sg called? If host_cookie is set dma_unmap() should
only be called from post_req.
My guess is
+ if (data && !data->host_cookie) {
It looks like only dma_map is run in parallel with transfer but not
dma_unmap. This may explain the numbers.
Shawn Guo April 20, 2011, 8:17 a.m. | #2
On Wed, Apr 20, 2011 at 09:58:48AM +0200, Per Forlin wrote:
> >  static struct dma_async_tx_descriptor *mxs_mmc_prep_dma(
> >        struct mxs_mmc_host *host, unsigned int append)
> >  {
> > @@ -312,8 +342,8 @@ static struct dma_async_tx_descriptor *mxs_mmc_prep_dma(
> >
> >        if (data) {
> >                /* data */
> > -               dma_map_sg(mmc_dev(host->mmc), data->sg,
> > -                          data->sg_len, host->dma_dir);
> > +               if (mxs_mmc_prep_dma_data(host, data, NULL))
> > +                       return NULL;
> >                sgl = data->sg;
> >                sg_len = data->sg_len;
> >        } else {
> > @@ -328,9 +358,11 @@ static struct dma_async_tx_descriptor *mxs_mmc_prep_dma(
> >                desc->callback = mxs_mmc_dma_irq_callback;
> >                desc->callback_param = host;
> >        } else {
> > -               if (data)
> > +               if (data) {
> >                        dma_unmap_sg(mmc_dev(host->mmc), data->sg,
> >                                     data->sg_len, host->dma_dir);
> > +                       data->host_cookie = 0;
> > +               }
> When is dma_unmap_sg called? If host_cookie is set dma_unmap() should
> only be called from post_req.
> My guess is
> + if (data && !data->host_cookie) {
> It looks like only dma_map is run in parallel with transfer but not
> dma_unmap. This may explain the numbers.

Good catch.  I forgot patching mxs_mmc_request_done where dma_unmap_sg
is called.  Will correct and retest ...
Russell King - ARM Linux April 28, 2011, 10:10 a.m. | #3
On Thu, Apr 28, 2011 at 09:52:17AM +0200, Per Forlin wrote:
> I wanted to test the performance without cache penalty but removing
> dma_map_sg may not work since it produces the physical mapped sg list.
> This is not as simple as I first thought. Make a copy for dma_map_sg
> (call it dma_map_sg_no_cache) and modify it to not clean/inc the
> cache. Replace dma_map_sg with dma_map_sg_no_cache in the mxs-mmc
> driver.
> Removing dma_unmap should be ok for this test case.
> Do you still get very low numbers?

We can live in the hope that this is gathering evidence to illustrate
why having DMA incoherent caches are bad news for performance, and that
one day ARM Ltd will one day give us proper DMA cache coherency for all
devices.

Patch

diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
index 99d39a6..63c2ae2 100644
--- a/drivers/mmc/host/mxs-mmc.c
+++ b/drivers/mmc/host/mxs-mmc.c
@@ -137,6 +137,10 @@ 
 
 #define SSP_PIO_NUM	3
 
+struct mxs_mmc_next {
+	s32 cookie;
+};
+
 struct mxs_mmc_host {
 	struct mmc_host			*mmc;
 	struct mmc_request		*mrq;
@@ -154,6 +158,7 @@  struct mxs_mmc_host {
 	struct mxs_dma_data		dma_data;
 	unsigned int			dma_dir;
 	u32				ssp_pio_words[SSP_PIO_NUM];
+	struct mxs_mmc_next		next_data;
 
 	unsigned int			version;
 	unsigned char			bus_width;
@@ -302,6 +307,31 @@  static irqreturn_t mxs_mmc_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int mxs_mmc_prep_dma_data(struct mxs_mmc_host *host,
+				struct mmc_data *data,
+				struct mxs_mmc_next *next)
+{
+	if (!next && data->host_cookie &&
+	    data->host_cookie != host->next_data.cookie) {
+		printk(KERN_WARNING "[%s] invalid cookie: data->host_cookie %d"
+		       " host->next_data.cookie %d\n",
+		       __func__, data->host_cookie, host->next_data.cookie);
+		data->host_cookie = 0;
+	}
+
+	/* Check if next job is already prepared */
+	if (next || (!next && data->host_cookie != host->next_data.cookie))
+		if (dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+			       (data->flags & MMC_DATA_WRITE) ?
+			       DMA_TO_DEVICE : DMA_FROM_DEVICE) == 0)
+			return -EINVAL;
+
+	if (next)
+		data->host_cookie = ++next->cookie < 0 ? 1 : next->cookie;
+
+	return 0;
+}
+
 static struct dma_async_tx_descriptor *mxs_mmc_prep_dma(
 	struct mxs_mmc_host *host, unsigned int append)
 {
@@ -312,8 +342,8 @@  static struct dma_async_tx_descriptor *mxs_mmc_prep_dma(
 
 	if (data) {
 		/* data */
-		dma_map_sg(mmc_dev(host->mmc), data->sg,
-			   data->sg_len, host->dma_dir);
+		if (mxs_mmc_prep_dma_data(host, data, NULL))
+			return NULL;
 		sgl = data->sg;
 		sg_len = data->sg_len;
 	} else {
@@ -328,9 +358,11 @@  static struct dma_async_tx_descriptor *mxs_mmc_prep_dma(
 		desc->callback = mxs_mmc_dma_irq_callback;
 		desc->callback_param = host;
 	} else {
-		if (data)
+		if (data) {
 			dma_unmap_sg(mmc_dev(host->mmc), data->sg,
 				     data->sg_len, host->dma_dir);
+			data->host_cookie = 0;
+		}
 	}
 
 	return desc;
@@ -553,6 +585,40 @@  static void mxs_mmc_start_cmd(struct mxs_mmc_host *host,
 	}
 }
 
+static void mxs_mmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
+			    bool is_first_req)
+{
+	struct mxs_mmc_host *host = mmc_priv(mmc);
+	struct mmc_data *data = mrq->data;
+
+	if (!data)
+		return;
+
+	if (data->host_cookie) {
+		data->host_cookie = 0;
+		return;
+	}
+
+	if (mxs_mmc_prep_dma_data(host, data, &host->next_data))
+		data->host_cookie = 0;
+}
+
+static void mxs_mmc_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
+			     int err)
+{
+	struct mxs_mmc_host *host = mmc_priv(mmc);
+	struct mmc_data *data = mrq->data;
+
+	if (!data)
+		return;
+
+	if (data->host_cookie) {
+		dma_unmap_sg(mmc_dev(host->mmc), data->sg,
+			     data->sg_len, host->dma_dir);
+		data->host_cookie = 0;
+	}
+}
+
 static void mxs_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
 {
 	struct mxs_mmc_host *host = mmc_priv(mmc);
@@ -644,6 +710,8 @@  static void mxs_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
 }
 
 static const struct mmc_host_ops mxs_mmc_ops = {
+	.pre_req = mxs_mmc_pre_req,
+	.post_req = mxs_mmc_post_req,
 	.request = mxs_mmc_request,
 	.get_ro = mxs_mmc_get_ro,
 	.get_cd = mxs_mmc_get_cd,
@@ -708,6 +776,7 @@  static int mxs_mmc_probe(struct platform_device *pdev)
 	host->dma_res = dmares;
 	host->irq = irq_err;
 	host->sdio_irq_en = 0;
+	host->next_data.cookie = 1;
 
 	host->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(host->clk)) {