diff mbox

[v6,02/11] omap_hsmmc: add support for pre_req and post_req

Message ID 1308518257-9783-3-git-send-email-per.forlin@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Per Forlin June 19, 2011, 9:17 p.m. UTC
pre_req() runs dma_map_sg(), post_req() runs dma_unmap_sg.
If not calling pre_req() before omap_hsmmc_request()
dma_map_sg will be issued before starting the transfer.
It is optional to use pre_req(). If issuing pre_req()
post_req() must be to be called as well.

Signed-off-by: Per Forlin <per.forlin@linaro.org>
---
 drivers/mmc/host/omap_hsmmc.c |   87 +++++++++++++++++++++++++++++++++++++++--
 1 files changed, 83 insertions(+), 4 deletions(-)

Comments

kishore kadiyala June 21, 2011, 5:41 a.m. UTC | #1
<snip>

> +
> +static void omap_hsmmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
> +                              bool is_first_req)

I don't see the usage of "is_first_req" below.
Is it required?

> +{
> +       struct omap_hsmmc_host *host = mmc_priv(mmc);
> +
> +       if (mrq->data->host_cookie) {
> +               mrq->data->host_cookie = 0;
> +               return ;
> +       }
> +
> +       if (host->use_dma)
> +               if (omap_hsmmc_pre_dma_transfer(host, mrq->data,
> +                                               &host->next_data))
> +                       mrq->data->host_cookie = 0;
> +}
> +
>  /*

Regards,
Kishore
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Per Forlin June 21, 2011, 6:51 a.m. UTC | #2
On 21 June 2011 07:41, Kishore Kadiyala <kishorek.kadiyala@gmail.com> wrote:
> <snip>
>
>> +
>> +static void omap_hsmmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
>> +                              bool is_first_req)
>
> I don't see the usage of "is_first_req" below.
> Is it required?
>
It is not required. It is only an indication that this request is the
first in a series of request. The host driver may do various
optimisations based on this information. The first request in a series
of jobs can't be prepared in parallel to the previous job. The host
driver can do the following to minimise latency for the first job.
 * Preparing the cache while the MMC read/write cmd is being
processed. In this case the pre_req could do nothing and the job is
instead run in parallel to the read/write cmd being sent. If the
is_first_req is false pre_req will run in parallel to an active
transfer, in this case it is more efficient to prepare the request in
pre_req.
 * Run PIO mode instead of DMA
 * Maybe there can be power related optimisations based on if it is
one single transfer or multiple ones.

>> +{
>> +       struct omap_hsmmc_host *host = mmc_priv(mmc);
>> +
>> +       if (mrq->data->host_cookie) {
>> +               mrq->data->host_cookie = 0;
>> +               return ;
>> +       }
>> +
>> +       if (host->use_dma)
>> +               if (omap_hsmmc_pre_dma_transfer(host, mrq->data,
>> +                                               &host->next_data))
>> +                       mrq->data->host_cookie = 0;
>> +}
>> +
>>  /*
>

Thanks for your comments,
Per
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
kishore kadiyala June 21, 2011, 1:56 p.m. UTC | #3
Hi Per,

On Tue, Jun 21, 2011 at 12:21 PM, Per Forlin <per.forlin@linaro.org> wrote:
> On 21 June 2011 07:41, Kishore Kadiyala <kishorek.kadiyala@gmail.com> wrote:
>> <snip>
>>
>>> +
>>> +static void omap_hsmmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
>>> +                              bool is_first_req)
>>
>> I don't see the usage of "is_first_req" below.
>> Is it required?
>>
> It is not required. It is only an indication that this request is the
> first in a series of request. The host driver may do various
> optimisations based on this information. The first request in a series
> of jobs can't be prepared in parallel to the previous job. The host
> driver can do the following to minimise latency for the first job.
>  * Preparing the cache while the MMC read/write cmd is being
> processed. In this case the pre_req could do nothing and the job is
> instead run in parallel to the read/write cmd being sent. If the
> is_first_req is false pre_req will run in parallel to an active
> transfer, in this case it is more efficient to prepare the request in
> pre_req.
>  * Run PIO mode instead of DMA
>  * Maybe there can be power related optimisations based on if it is
> one single transfer or multiple ones.

Ok, thanks for making things clear.

<snip>

Regards,
Kishore
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre June 21, 2011, 7:18 p.m. UTC | #4
On Tue, 21 Jun 2011, Per Forlin wrote:

> On 21 June 2011 07:41, Kishore Kadiyala <kishorek.kadiyala@gmail.com> wrote:
> > <snip>
> >
> >> +
> >> +static void omap_hsmmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
> >> +                              bool is_first_req)
> >
> > I don't see the usage of "is_first_req" below.
> > Is it required?
> >
> It is not required. It is only an indication that this request is the
> first in a series of request. The host driver may do various
> optimisations based on this information. The first request in a series
> of jobs can't be prepared in parallel to the previous job. The host
> driver can do the following to minimise latency for the first job.
>  * Preparing the cache while the MMC read/write cmd is being
> processed. In this case the pre_req could do nothing and the job is
> instead run in parallel to the read/write cmd being sent. If the
> is_first_req is false pre_req will run in parallel to an active
> transfer, in this case it is more efficient to prepare the request in
> pre_req.
>  * Run PIO mode instead of DMA

That is never going to be a good tradeoff.  If the CPU is busy doing 
PIO, it won't have a chance to prepare a subsequent request in parallel 
to the first transfer.


Nicolas
Nicolas Pitre June 21, 2011, 8:29 p.m. UTC | #5
On Tue, 21 Jun 2011, Per Forlin wrote:

> On 21 June 2011 21:18, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > On Tue, 21 Jun 2011, Per Forlin wrote:
> >
> >> On 21 June 2011 07:41, Kishore Kadiyala <kishorek.kadiyala@gmail.com> wrote:
> >> > <snip>
> >> >
> >> >> +
> >> >> +static void omap_hsmmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
> >> >> +                              bool is_first_req)
> >> >
> >> > I don't see the usage of "is_first_req" below.
> >> > Is it required?
> >> >
> >> It is not required. It is only an indication that this request is the
> >> first in a series of request. The host driver may do various
> >> optimisations based on this information. The first request in a series
> >> of jobs can't be prepared in parallel to the previous job. The host
> >> driver can do the following to minimise latency for the first job.
> >>  * Preparing the cache while the MMC read/write cmd is being
> >> processed. In this case the pre_req could do nothing and the job is
> >> instead run in parallel to the read/write cmd being sent. If the
> >> is_first_req is false pre_req will run in parallel to an active
> >> transfer, in this case it is more efficient to prepare the request in
> >> pre_req.
> >>  * Run PIO mode instead of DMA
> >
> > That is never going to be a good tradeoff.  If the CPU is busy doing
> > PIO, it won't have a chance to prepare a subsequent request in parallel
> > to the first transfer.
> >
> If you have two CPUs and the MMC interrupts are scheduled on the CPU
> 1, CPU 0 can prepare the next one.

Well, it is true that in theory the PIO operation shouldn't take all the 
CPU anyway, so maybe there are some cycles left in between FIFO 
interrupts.

The danger here is of course to be presented with a trickle of single 
requests.  Doing them all with PIO is going to waste more power or 
prevent other tasks from running with 100% CPU which might impact the 
system latency more than the latency we're trying to avoid here.

In other words this is something that should be evaluated and not 
applied freely.


Nicolas
diff mbox

Patch

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index cd317af..b29ca90 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -141,6 +141,11 @@ 
 #define OMAP_HSMMC_WRITE(base, reg, val) \
 	__raw_writel((val), (base) + OMAP_HSMMC_##reg)
 
+struct omap_hsmmc_next {
+	unsigned int	dma_len;
+	s32		cookie;
+};
+
 struct omap_hsmmc_host {
 	struct	device		*dev;
 	struct	mmc_host	*mmc;
@@ -184,6 +189,7 @@  struct omap_hsmmc_host {
 	int			reqs_blocked;
 	int			use_reg;
 	int			req_in_progress;
+	struct omap_hsmmc_next	next_data;
 
 	struct	omap_mmc_platform_data	*pdata;
 };
@@ -1343,8 +1349,9 @@  static void omap_hsmmc_dma_cb(int lch, u16 ch_status, void *cb_data)
 		return;
 	}
 
-	dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
-		omap_hsmmc_get_dma_dir(host, data));
+	if (!data->host_cookie)
+		dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+			     omap_hsmmc_get_dma_dir(host, data));
 
 	req_in_progress = host->req_in_progress;
 	dma_ch = host->dma_ch;
@@ -1362,6 +1369,45 @@  static void omap_hsmmc_dma_cb(int lch, u16 ch_status, void *cb_data)
 	}
 }
 
+static int omap_hsmmc_pre_dma_transfer(struct omap_hsmmc_host *host,
+				       struct mmc_data *data,
+				       struct omap_hsmmc_next *next)
+{
+	int dma_len;
+
+	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)) {
+		dma_len = dma_map_sg(mmc_dev(host->mmc), data->sg,
+				     data->sg_len,
+				     omap_hsmmc_get_dma_dir(host, data));
+
+	} else {
+		dma_len = host->next_data.dma_len;
+		host->next_data.dma_len = 0;
+	}
+
+
+	if (dma_len == 0)
+		return -EINVAL;
+
+	if (next) {
+		next->dma_len = dma_len;
+		data->host_cookie = ++next->cookie < 0 ? 1 : next->cookie;
+	} else
+		host->dma_len = dma_len;
+
+	return 0;
+}
+
 /*
  * Routine to configure and start DMA for the MMC card
  */
@@ -1395,9 +1441,10 @@  static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host,
 			mmc_hostname(host->mmc), ret);
 		return ret;
 	}
+	ret = omap_hsmmc_pre_dma_transfer(host, data, NULL);
+	if (ret)
+		return ret;
 
-	host->dma_len = dma_map_sg(mmc_dev(host->mmc), data->sg,
-			data->sg_len, omap_hsmmc_get_dma_dir(host, data));
 	host->dma_ch = dma_ch;
 	host->dma_sg_idx = 0;
 
@@ -1477,6 +1524,35 @@  omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req)
 	return 0;
 }
 
+static void omap_hsmmc_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
+				int err)
+{
+	struct omap_hsmmc_host *host = mmc_priv(mmc);
+	struct mmc_data *data = mrq->data;
+
+	if (host->use_dma) {
+		dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+			     omap_hsmmc_get_dma_dir(host, data));
+		data->host_cookie = 0;
+	}
+}
+
+static void omap_hsmmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
+			       bool is_first_req)
+{
+	struct omap_hsmmc_host *host = mmc_priv(mmc);
+
+	if (mrq->data->host_cookie) {
+		mrq->data->host_cookie = 0;
+		return ;
+	}
+
+	if (host->use_dma)
+		if (omap_hsmmc_pre_dma_transfer(host, mrq->data,
+						&host->next_data))
+			mrq->data->host_cookie = 0;
+}
+
 /*
  * Request function. for read/write operation
  */
@@ -1925,6 +2001,8 @@  static int omap_hsmmc_disable_fclk(struct mmc_host *mmc, int lazy)
 static const struct mmc_host_ops omap_hsmmc_ops = {
 	.enable = omap_hsmmc_enable_fclk,
 	.disable = omap_hsmmc_disable_fclk,
+	.post_req = omap_hsmmc_post_req,
+	.pre_req = omap_hsmmc_pre_req,
 	.request = omap_hsmmc_request,
 	.set_ios = omap_hsmmc_set_ios,
 	.get_cd = omap_hsmmc_get_cd,
@@ -2074,6 +2152,7 @@  static int __init omap_hsmmc_probe(struct platform_device *pdev)
 	host->mapbase	= res->start;
 	host->base	= ioremap(host->mapbase, SZ_4K);
 	host->power_mode = MMC_POWER_OFF;
+	host->next_data.cookie = 1;
 
 	platform_set_drvdata(pdev, host);
 	INIT_WORK(&host->mmc_carddetect_work, omap_hsmmc_detect);