[PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Venkatraman S]

>From 1c63dd42fc6c563c931168779ce73401332faa52 Mon Sep 17 00:00:00 2001
From: Venkatraman S <svenkatr@ti.com>
Date: Thu, 29 Apr 2010 22:43:31 +0530
Subject: [PATCH 2/2] omap hsmmc: adaptation of sdma descriptor
autoloading feature

Start to use the sDMA descriptor autoloading feature.
For large datablocks, the MMC driver has to repeatedly setup,
program and teardown the dma channel for each element of the
sglist received in omap_hsmmc_request.

By using descriptor autoloading, transfers from / to each element of
the sglist is pre programmed into a linked list. The sDMA driver
completes the entire transaction and provides a single interrupt.

Due to this, number of dma interrupts for a typical 100MB transfer on the MMC is
reduced from 25000 to about 400 (approximate). Transfer speeds are
improved by ~5%.
(Though it varies on the size of read / write & improves on huge transfers)

Descriptor autoloading is available only in 3630 and 4430 (as of now).
Hence normal DMA mode is also retained.

Signed-off-by: Venkatraman S <svenkatr@ti.com>
CC: Adrian Hunter <adrian.hunter@nokia.com>
CC: Madhusudhan C <madhu.cr@ti.com>
CC: Shilimkar Santosh <santosh.shilimkar@ti.com>
CC: Tony Lindgren <tony@atomide.com>
---
 Changes since previous version
  * Rebased to Adrian Hunter's interrupt syncronisation patch
             https://patchwork.kernel.org/patch/94670/
  * Rename ICR name definition
 drivers/mmc/host/omap_hsmmc.c |  148 ++++++++++++++++++++++++++++++++++------
 1 files changed, 125 insertions(+), 23 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 65093d4..095fd94 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -102,6 +102,8 @@
 #define SRD			(1 << 26)
 #define SOFTRESET		(1 << 1)
 #define RESETDONE		(1 << 0)
+/* End of superblock indicator for sglist transfers */
+#define DMA_ICR_SGLIST_END	0x4D00

 /*
  * FIXME: Most likely all the data using these _DEVID defines should come
@@ -118,6 +120,12 @@
 #define OMAP_MMC_MASTER_CLOCK	96000000
 #define DRIVER_NAME		"mmci-omap-hs"

+#define DMA_TYPE_NODMA	0
+#define DMA_TYPE_SDMA	1
+#define DMA_TYPE_SDMA_DLOAD 2
+
+#define DMA_CTRL_BUF_SIZE	(PAGE_SIZE * 3)
+
 /* Timeouts for entering power saving states on inactivity, msec */
 #define OMAP_MMC_DISABLED_TIMEOUT	100
 #define OMAP_MMC_SLEEP_TIMEOUT		1000
@@ -170,7 +178,11 @@ struct omap_hsmmc_host {
 	u32			bytesleft;
 	int			suspended;
 	int			irq;
-	int			use_dma, dma_ch;
+	int			dma_caps;
+	int			dma_in_use;
+	int			dma_ch;
+	void			*dma_ctrl_buf;
+	dma_addr_t		dma_ctrl_buf_phy;
 	int			dma_line_tx, dma_line_rx;
 	int			slot_id;
 	int			got_dbclk;
@@ -527,7 +539,7 @@ static void omap_hsmmc_enable_irq(struct
omap_hsmmc_host *host)
 {
 	unsigned int irq_mask;

-	if (host->use_dma)
+	if (host->dma_in_use)
 		irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
 	else
 		irq_mask = INT_EN_MASK;
@@ -813,7 +825,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host
*host, struct mmc_command *cmd,
 			cmdreg &= ~(DDIR);
 	}

-	if (host->use_dma)
+	if (host->dma_in_use)
 		cmdreg |= DMA_EN;

 	host->req_in_progress = 1;
@@ -842,7 +854,7 @@ static void omap_hsmmc_request_done(struct
omap_hsmmc_host *host, struct mmc_req

 	omap_hsmmc_disable_irq(host);
 	/* Do not complete the request if DMA is still in progress */
-	if (mrq->data && host->use_dma && dma_ch != -1)
+	if (mrq->data && host->dma_in_use && dma_ch != -1)
 		return;
 	host->mrq = NULL;
 	mmc_request_done(host->mmc, mrq);
@@ -920,7 +932,7 @@ static void omap_hsmmc_dma_cleanup(struct
omap_hsmmc_host *host, int errno)
 	host->dma_ch = -1;
 	spin_unlock(&host->irq_lock);

-	if (host->use_dma && dma_ch != -1) {
+	if (host->dma_in_use && dma_ch != -1) {
 		dma_unmap_sg(mmc_dev(host->mmc), host->data->sg, host->dma_len,
 			omap_hsmmc_get_dma_dir(host, host->data));
 		omap_free_dma(dma_ch);
@@ -1266,7 +1278,6 @@ static void omap_hsmmc_config_dma_params(struct
omap_hsmmc_host *host,
 			omap_hsmmc_get_dma_sync_dev(host, data),
 			!(data->flags & MMC_DATA_WRITE));

-	omap_start_dma(dma_ch);
 }

 /*
@@ -1279,21 +1290,23 @@ static void omap_hsmmc_dma_cb(int lch, u16
ch_status, void *cb_data)
 	int dma_ch, req_in_progress;

 	if (ch_status & OMAP2_DMA_MISALIGNED_ERR_IRQ)
-		dev_dbg(mmc_dev(host->mmc), "MISALIGNED_ADRS_ERR\n");
+		dev_dbg(mmc_dev(host->mmc), "Misaligned address error\n");

 	spin_lock(&host->irq_lock);
 	if (host->dma_ch < 0) {
 		spin_unlock(&host->irq_lock);
 		return;
 	}
-
-	host->dma_sg_idx++;
-	if (host->dma_sg_idx < host->dma_len) {
-		/* Fire up the next transfer. */
-		omap_hsmmc_config_dma_params(host, data,
+	if (host->dma_in_use == DMA_TYPE_SDMA) {
+		host->dma_sg_idx++;
+		if (host->dma_sg_idx < host->dma_len) {
+			/* Fire up the next transfer. */
+			omap_hsmmc_config_dma_params(host, data,
 					   data->sg + host->dma_sg_idx);
-		spin_unlock(&host->irq_lock);
-		return;
+			omap_start_dma(host->dma_ch);
+			spin_unlock(&host->irq_lock);
+			return;
+		}
 	}

 	dma_unmap_sg(mmc_dev(host->mmc), data->sg, host->dma_len,
@@ -1315,10 +1328,19 @@ static void omap_hsmmc_dma_cb(int lch, u16
ch_status, void *cb_data)
 	}
 }

+static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host)
+{
+	if (host->dma_in_use == DMA_TYPE_SDMA)
+		omap_start_dma(host->dma_ch);
+	else if (host->dma_in_use == DMA_TYPE_SDMA_DLOAD)
+		return omap_start_dma_sglist_transfers(host->dma_ch, -1);
+
+	return 0;
+}
 /*
- * Routine to configure and start DMA for the MMC card
+ * Routine to configure DMA for the MMC card
  */
-static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host,
+static int omap_hsmmc_configure_sdma(struct omap_hsmmc_host *host,
 					struct mmc_request *req)
 {
 	int dma_ch = 0, ret = 0, i;
@@ -1359,6 +1381,56 @@ static int omap_hsmmc_start_dma_transfer(struct
omap_hsmmc_host *host,
 	return 0;
 }

+static int omap_hsmmc_configure_sdma_sglist(struct omap_hsmmc_host *host,
+		struct mmc_request *req)
+{
+	int i;
+	struct omap_dma_sglist_node *sglist, *snode;
+	struct mmc_data *data = req->data;
+	int blksz;
+	int dmadir = omap_hsmmc_get_dma_dir(host, data);
+	struct omap_dma_sglist_type2a_params *t2p;
+
+	sglist = (struct omap_dma_sglist_node *) host->dma_ctrl_buf;
+	snode = sglist;
+	blksz = host->data->blksz;
+
+	if ((host->dma_len * sizeof(*snode)) > DMA_CTRL_BUF_SIZE) {
+		dev_err(mmc_dev(host->mmc), "not enough sglist memory %d\n",
+			host->dma_len);
+		return -ENOMEM;
+	}
+	for (i = 0; i < host->dma_len; snode++, i++) {
+		snode->desc_type = OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a;
+		snode->num_of_elem = blksz / 4;
+		t2p = &snode->sg_node.t2a;
+
+		if (dmadir == DMA_FROM_DEVICE) {
+			t2p->src_addr = host->mapbase + OMAP_HSMMC_DATA;
+			t2p->dst_addr = sg_dma_address(data->sg + i);
+		} else {
+			t2p->dst_addr = host->mapbase + OMAP_HSMMC_DATA;
+			t2p->src_addr = sg_dma_address(data->sg + i);
+		}
+		snode->flags =
+			OMAP_DMA_LIST_DST_VALID | OMAP_DMA_LIST_SRC_VALID;
+
+		t2p->cfn_fn = sg_dma_len(data->sg + i) / host->data->blksz;
+		t2p->cicr = DMA_ICR_SGLIST_END;
+
+		t2p->dst_frame_idx_or_pkt_size = 0;
+		t2p->src_frame_idx_or_pkt_size = 0;
+		t2p->dst_elem_idx = 0;
+		t2p->src_elem_idx = 0;
+	}
+	dev_dbg(mmc_dev(host->mmc), "new sglist %x len =%d\n",
+			host->dma_ctrl_buf_phy, i);
+	omap_set_dma_sglist_mode(host->dma_ch, sglist,
+			host->dma_ctrl_buf_phy, i, NULL);
+	omap_dma_set_sglist_fastmode(host->dma_ch, 1);
+	return 0;
+}
+
 static void set_data_timeout(struct omap_hsmmc_host *host,
 			     unsigned int timeout_ns,
 			     unsigned int timeout_clks)
@@ -1420,14 +1492,23 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host
*host, struct mmc_request *req)
 					| (req->data->blocks << 16));
 	set_data_timeout(host, req->data->timeout_ns, req->data->timeout_clks);

-	if (host->use_dma) {
-		ret = omap_hsmmc_start_dma_transfer(host, req);
-		if (ret != 0) {
-			dev_dbg(mmc_dev(host->mmc), "MMC start dma failure\n");
+	if (host->dma_caps & DMA_TYPE_SDMA) {
+		ret = omap_hsmmc_configure_sdma(host, req);
+		if (ret)
 			return ret;
-		}
+		host->dma_in_use = DMA_TYPE_SDMA;
 	}
-	return 0;
+	if ((host->dma_caps & DMA_TYPE_SDMA_DLOAD) &&
+		host->data->sg_len > 4) {
+		ret = omap_hsmmc_configure_sdma_sglist(host, req);
+		if (ret)
+			return ret;
+		host->dma_in_use = DMA_TYPE_SDMA_DLOAD;
+
+	}
+	ret = omap_hsmmc_start_dma_transfer(host);
+	return ret;
+
 }

 /*
@@ -2007,7 +2088,9 @@ static int __init omap_hsmmc_probe(struct
platform_device *pdev)
 	host->mmc	= mmc;
 	host->pdata	= pdata;
 	host->dev	= &pdev->dev;
-	host->use_dma	= 1;
+	host->dma_caps	= DMA_TYPE_SDMA;
+	host->dma_in_use	= DMA_TYPE_NODMA;
+	host->dma_ctrl_buf = NULL;
 	host->dev->dma_mask = &pdata->dma_mask;
 	host->dma_ch	= -1;
 	host->irq	= irq;
@@ -2088,6 +2171,15 @@ static int __init omap_hsmmc_probe(struct
platform_device *pdev)
 							" clk failed\n");
 	}

+	if (cpu_is_omap44xx() || cpu_is_omap3630()) {
+		host->dma_ctrl_buf = dma_alloc_coherent(NULL,
+					DMA_CTRL_BUF_SIZE,
+					&host->dma_ctrl_buf_phy,
+					0);
+		if (host->dma_ctrl_buf != NULL)
+			host->dma_caps |= DMA_TYPE_SDMA_DLOAD;
+	}
+
 	/* Since we do only SG emulation, we can have as many segs
 	 * as we want. */
 	mmc->max_phys_segs = 1024;
@@ -2213,6 +2305,10 @@ err_reg:
 err_irq_cd_init:
 	free_irq(host->irq, host);
 err_irq:
+	if (host->dma_ctrl_buf)
+		dma_free_coherent(NULL, DMA_CTRL_BUF_SIZE,
+				host->dma_ctrl_buf,
+				host->dma_ctrl_buf_phy);
 	mmc_host_disable(host->mmc);
 	clk_disable(host->iclk);
 	clk_put(host->fclk);
@@ -2240,6 +2336,12 @@ static int omap_hsmmc_remove(struct
platform_device *pdev)
 	if (host) {
 		mmc_host_enable(host->mmc);
 		mmc_remove_host(host->mmc);
+
+		if (host->dma_ctrl_buf != NULL) {
+			dma_free_coherent(NULL, DMA_CTRL_BUF_SIZE,
+				host->dma_ctrl_buf,
+				host->dma_ctrl_buf_phy);
+		}
 		if (host->use_reg)
 			omap_hsmmc_reg_put(host);
 		if (host->pdata->cleanup)
-- 
1.6.3.3

RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Shilimkar, Santosh]

> -----Original Message-----
> From: svenkatr@gmail.com [mailto:svenkatr@gmail.com] On Behalf Of Venkatraman S
> Sent: Thursday, April 29, 2010 11:05 PM
> To: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-kernel@lists.arm.linux.org.uk
> Cc: Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Shilimkar, Santosh; Tony
> Lindgren
> Subject: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
> 
> From 1c63dd42fc6c563c931168779ce73401332faa52 Mon Sep 17 00:00:00 2001
> From: Venkatraman S <svenkatr@ti.com>
> Date: Thu, 29 Apr 2010 22:43:31 +0530
> Subject: [PATCH 2/2] omap hsmmc: adaptation of sdma descriptor
> autoloading feature
> 
> Start to use the sDMA descriptor autoloading feature.
> For large datablocks, the MMC driver has to repeatedly setup,
> program and teardown the dma channel for each element of the
> sglist received in omap_hsmmc_request.
> 
> By using descriptor autoloading, transfers from / to each element of
> the sglist is pre programmed into a linked list. The sDMA driver
> completes the entire transaction and provides a single interrupt.
> 
> Due to this, number of dma interrupts for a typical 100MB transfer on the MMC is
> reduced from 25000 to about 400 (approximate). Transfer speeds are
> improved by ~5%.
> (Though it varies on the size of read / write & improves on huge transfers)
> 
> Descriptor autoloading is available only in 3630 and 4430 (as of now).
> Hence normal DMA mode is also retained.
> 
> Signed-off-by: Venkatraman S <svenkatr@ti.com>
> CC: Adrian Hunter <adrian.hunter@nokia.com>
> CC: Madhusudhan C <madhu.cr@ti.com>
> CC: Shilimkar Santosh <santosh.shilimkar@ti.com>
> CC: Tony Lindgren <tony@atomide.com>
> ---
>  Changes since previous version
>   * Rebased to Adrian Hunter's interrupt syncronisation patch
>              https://patchwork.kernel.org/patch/94670/
>   * Rename ICR name definition
>  drivers/mmc/host/omap_hsmmc.c |  148 ++++++++++++++++++++++++++++++++++------
>  1 files changed, 125 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 65093d4..095fd94 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -102,6 +102,8 @@
>  #define SRD			(1 << 26)
>  #define SOFTRESET		(1 << 1)
>  #define RESETDONE		(1 << 0)
> +/* End of superblock indicator for sglist transfers */
> +#define DMA_ICR_SGLIST_END	0x4D00
> 
>  /*
>   * FIXME: Most likely all the data using these _DEVID defines should come
> @@ -118,6 +120,12 @@
>  #define OMAP_MMC_MASTER_CLOCK	96000000
>  #define DRIVER_NAME		"mmci-omap-hs"
> 
> +#define DMA_TYPE_NODMA	0
" DMA_TYPE_NODMA" doesn't make sense
> +#define DMA_TYPE_SDMA	1
> +#define DMA_TYPE_SDMA_DLOAD 2

How about ??
#define TRANSFER_NODMA			0
#define TRANSFER_SDMA_NORMAL		1
#define TRANSFER_SDMA_DESCRIPTOR	2
I think ADMA is also in pipeline, so it can become then
#define TRANSFER_ADMA_DESCRIPTOR	3
> +
> +#define DMA_CTRL_BUF_SIZE	(PAGE_SIZE * 3)
> +
>  /* Timeouts for entering power saving states on inactivity, msec */
>  #define OMAP_MMC_DISABLED_TIMEOUT	100
>  #define OMAP_MMC_SLEEP_TIMEOUT		1000
> @@ -170,7 +178,11 @@ struct omap_hsmmc_host {
>  	u32			bytesleft;
>  	int			suspended;
>  	int			irq;
> -	int			use_dma, dma_ch;
> +	int			dma_caps;
> +	int			dma_in_use;
This flag isn't necessary. What you are doing is
just renaming it. Can't you avoid that ??
Inudertand the intent behind "dma_in_use " instead
of "use_dma", but you can surely avoid this change.

> +	int			dma_ch;
> +	void			*dma_ctrl_buf;
> +	dma_addr_t		dma_ctrl_buf_phy;
>  	int			dma_line_tx, dma_line_rx;
>  	int			slot_id;
>  	int			got_dbclk;
> @@ -527,7 +539,7 @@ static void omap_hsmmc_enable_irq(struct
> omap_hsmmc_host *host)
>  {
>  	unsigned int irq_mask;
> 
> -	if (host->use_dma)
> +	if (host->dma_in_use)
This will go with no flag change.
>  		irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
>  	else
>  		irq_mask = INT_EN_MASK;
> @@ -813,7 +825,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host
> *host, struct mmc_command *cmd,
>  			cmdreg &= ~(DDIR);
>  	}
> 
> -	if (host->use_dma)
> +	if (host->dma_in_use)
ditto
>  		cmdreg |= DMA_EN;
> 
>  	host->req_in_progress = 1;
> @@ -842,7 +854,7 @@ static void omap_hsmmc_request_done(struct
> omap_hsmmc_host *host, struct mmc_req
> 
>  	omap_hsmmc_disable_irq(host);
>  	/* Do not complete the request if DMA is still in progress */
> -	if (mrq->data && host->use_dma && dma_ch != -1)
> +	if (mrq->data && host->dma_in_use && dma_ch != -1)
ditto
>  		return;
>  	host->mrq = NULL;
>  	mmc_request_done(host->mmc, mrq);
> @@ -920,7 +932,7 @@ static void omap_hsmmc_dma_cleanup(struct
> omap_hsmmc_host *host, int errno)
>  	host->dma_ch = -1;
>  	spin_unlock(&host->irq_lock);
> 
> -	if (host->use_dma && dma_ch != -1) {
> +	if (host->dma_in_use && dma_ch != -1) {
ditto
>  		dma_unmap_sg(mmc_dev(host->mmc), host->data->sg, host->dma_len,
>  			omap_hsmmc_get_dma_dir(host, host->data));
>  		omap_free_dma(dma_ch);
> @@ -1266,7 +1278,6 @@ static void omap_hsmmc_config_dma_params(struct
> omap_hsmmc_host *host,
>  			omap_hsmmc_get_dma_sync_dev(host, data),
>  			!(data->flags & MMC_DATA_WRITE));
> 
> -	omap_start_dma(dma_ch);
>  }
> 
>  /*
> @@ -1279,21 +1290,23 @@ static void omap_hsmmc_dma_cb(int lch, u16
> ch_status, void *cb_data)
>  	int dma_ch, req_in_progress;
> 
>  	if (ch_status & OMAP2_DMA_MISALIGNED_ERR_IRQ)
> -		dev_dbg(mmc_dev(host->mmc), "MISALIGNED_ADRS_ERR\n");
> +		dev_dbg(mmc_dev(host->mmc), "Misaligned address error\n");
> 
>  	spin_lock(&host->irq_lock);
>  	if (host->dma_ch < 0) {
>  		spin_unlock(&host->irq_lock);
>  		return;
>  	}
> -
> -	host->dma_sg_idx++;
> -	if (host->dma_sg_idx < host->dma_len) {
> -		/* Fire up the next transfer. */
> -		omap_hsmmc_config_dma_params(host, data,
> +	if (host->dma_in_use == DMA_TYPE_SDMA) {
> +		host->dma_sg_idx++;
> +		if (host->dma_sg_idx < host->dma_len) {
> +			/* Fire up the next transfer. */
> +			omap_hsmmc_config_dma_params(host, data,
>  					   data->sg + host->dma_sg_idx);
> -		spin_unlock(&host->irq_lock);
> -		return;
> +			omap_start_dma(host->dma_ch);
> +			spin_unlock(&host->irq_lock);
> +			return;
> +		}
>  	}
> 
>  	dma_unmap_sg(mmc_dev(host->mmc), data->sg, host->dma_len,
> @@ -1315,10 +1328,19 @@ static void omap_hsmmc_dma_cb(int lch, u16
> ch_status, void *cb_data)
>  	}
>  }
> 
> +static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host)
> +{
> +	if (host->dma_in_use == DMA_TYPE_SDMA)
> +		omap_start_dma(host->dma_ch);
> +	else if (host->dma_in_use == DMA_TYPE_SDMA_DLOAD)
> +		return omap_start_dma_sglist_transfers(host->dma_ch, -1);
Instead of "-1" , some macro for "NO_PAUSE" would have been better. 
> +
> +	return 0;
> +}
>  /*
> - * Routine to configure and start DMA for the MMC card
> + * Routine to configure DMA for the MMC card
>   */
> -static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host,
> +static int omap_hsmmc_configure_sdma(struct omap_hsmmc_host *host,
>  					struct mmc_request *req)
>  {
>  	int dma_ch = 0, ret = 0, i;
> @@ -1359,6 +1381,56 @@ static int omap_hsmmc_start_dma_transfer(struct
> omap_hsmmc_host *host,
>  	return 0;
>  }
> 
> +static int omap_hsmmc_configure_sdma_sglist(struct omap_hsmmc_host *host,
> +		struct mmc_request *req)
> +{
> +	int i;
> +	struct omap_dma_sglist_node *sglist, *snode;
> +	struct mmc_data *data = req->data;
> +	int blksz;
> +	int dmadir = omap_hsmmc_get_dma_dir(host, data);
> +	struct omap_dma_sglist_type2a_params *t2p;
> +
> +	sglist = (struct omap_dma_sglist_node *) host->dma_ctrl_buf;
> +	snode = sglist;
> +	blksz = host->data->blksz;
> +
> +	if ((host->dma_len * sizeof(*snode)) > DMA_CTRL_BUF_SIZE) {
> +		dev_err(mmc_dev(host->mmc), "not enough sglist memory %d\n",
> +			host->dma_len);
> +		return -ENOMEM;
> +	}
> +	for (i = 0; i < host->dma_len; snode++, i++) {
> +		snode->desc_type = OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a;
> +		snode->num_of_elem = blksz / 4;
> +		t2p = &snode->sg_node.t2a;
> +
> +		if (dmadir == DMA_FROM_DEVICE) {
> +			t2p->src_addr = host->mapbase + OMAP_HSMMC_DATA;
> +			t2p->dst_addr = sg_dma_address(data->sg + i);
> +		} else {
> +			t2p->dst_addr = host->mapbase + OMAP_HSMMC_DATA;
> +			t2p->src_addr = sg_dma_address(data->sg + i);
> +		}
> +		snode->flags =
> +			OMAP_DMA_LIST_DST_VALID | OMAP_DMA_LIST_SRC_VALID;
> +
> +		t2p->cfn_fn = sg_dma_len(data->sg + i) / host->data->blksz;
> +		t2p->cicr = DMA_ICR_SGLIST_END;
> +
> +		t2p->dst_frame_idx_or_pkt_size = 0;
> +		t2p->src_frame_idx_or_pkt_size = 0;
> +		t2p->dst_elem_idx = 0;
> +		t2p->src_elem_idx = 0;
> +	}
> +	dev_dbg(mmc_dev(host->mmc), "new sglist %x len =%d\n",
> +			host->dma_ctrl_buf_phy, i);
> +	omap_set_dma_sglist_mode(host->dma_ch, sglist,
> +			host->dma_ctrl_buf_phy, i, NULL);
> +	omap_dma_set_sglist_fastmode(host->dma_ch, 1);

You should check for return value of above two. If any one of above fails
the transfer will fail BADLY
> +	return 0;
> +}
> +
>  static void set_data_timeout(struct omap_hsmmc_host *host,
>  			     unsigned int timeout_ns,
>  			     unsigned int timeout_clks)
> @@ -1420,14 +1492,23 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host
> *host, struct mmc_request *req)
>  					| (req->data->blocks << 16));
>  	set_data_timeout(host, req->data->timeout_ns, req->data->timeout_clks);
> 
> -	if (host->use_dma) {
> -		ret = omap_hsmmc_start_dma_transfer(host, req);
> -		if (ret != 0) {
> -			dev_dbg(mmc_dev(host->mmc), "MMC start dma failure\n");
> +	if (host->dma_caps & DMA_TYPE_SDMA) {
> +		ret = omap_hsmmc_configure_sdma(host, req);
> +		if (ret)
>  			return ret;
> -		}
> +		host->dma_in_use = DMA_TYPE_SDMA;
>  	}
> -	return 0;
> +	if ((host->dma_caps & DMA_TYPE_SDMA_DLOAD) &&
> +		host->data->sg_len > 4) {
> +		ret = omap_hsmmc_configure_sdma_sglist(host, req);
> +		if (ret)
> +			return ret;
> +		host->dma_in_use = DMA_TYPE_SDMA_DLOAD;
> +
> +	}
> +	ret = omap_hsmmc_start_dma_transfer(host);
> +	return ret;
> +
>  }
> 
>  /*
> @@ -2007,7 +2088,9 @@ static int __init omap_hsmmc_probe(struct
> platform_device *pdev)
>  	host->mmc	= mmc;
>  	host->pdata	= pdata;
>  	host->dev	= &pdev->dev;
> -	host->use_dma	= 1;
> +	host->dma_caps	= DMA_TYPE_SDMA;
> +	host->dma_in_use	= DMA_TYPE_NODMA;
> +	host->dma_ctrl_buf = NULL;
>  	host->dev->dma_mask = &pdata->dma_mask;
>  	host->dma_ch	= -1;
>  	host->irq	= irq;
> @@ -2088,6 +2171,15 @@ static int __init omap_hsmmc_probe(struct
> platform_device *pdev)
>  							" clk failed\n");
>  	}
> 
> +	if (cpu_is_omap44xx() || cpu_is_omap3630()) {
Can we avoid above by passing this part of platform data??
devices.c
> +		host->dma_ctrl_buf = dma_alloc_coherent(NULL,
> +					DMA_CTRL_BUF_SIZE,
> +					&host->dma_ctrl_buf_phy,
> +					0);
> +		if (host->dma_ctrl_buf != NULL)
> +			host->dma_caps |= DMA_TYPE_SDMA_DLOAD;
> +	}
> +
>  	/* Since we do only SG emulation, we can have as many segs
>  	 * as we want. */
Not your patch but above commenting style isn't right
>  	mmc->max_phys_segs = 1024;
> @@ -2213,6 +2305,10 @@ err_reg:
>  err_irq_cd_init:
>  	free_irq(host->irq, host);
>  err_irq:
> +	if (host->dma_ctrl_buf)
> +		dma_free_coherent(NULL, DMA_CTRL_BUF_SIZE,
> +				host->dma_ctrl_buf,
> +				host->dma_ctrl_buf_phy);
>  	mmc_host_disable(host->mmc);
>  	clk_disable(host->iclk);
>  	clk_put(host->fclk);
> @@ -2240,6 +2336,12 @@ static int omap_hsmmc_remove(struct
> platform_device *pdev)
>  	if (host) {
>  		mmc_host_enable(host->mmc);
>  		mmc_remove_host(host->mmc);
> +
> +		if (host->dma_ctrl_buf != NULL) {
> +			dma_free_coherent(NULL, DMA_CTRL_BUF_SIZE,
> +				host->dma_ctrl_buf,
> +				host->dma_ctrl_buf_phy);
> +		}
>  		if (host->use_reg)
>  			omap_hsmmc_reg_put(host);
>  		if (host->pdata->cleanup)
> --
> 1.6.3.3

Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Venkatraman S]

[Long sections have been trimmed to the context of discussion]
Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote:
>
>> -----Original Message-----
>> From: svenkatr@gmail.com [mailto:svenkatr@gmail.com] On Behalf Of Venkatraman S
>> Sent: Thursday, April 29, 2010 11:05 PM
>> To: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-kernel@lists.arm.linux.org.uk
>> Cc: Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Shilimkar, Santosh; Tony
>> Lindgren
>> Subject: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
>>
>> From 1c63dd42fc6c563c931168779ce73401332faa52 Mon Sep 17 00:00:00 2001
>> From: Venkatraman S <svenkatr@ti.com>
>> Date: Thu, 29 Apr 2010 22:43:31 +0530
>> Subject: [PATCH 2/2] omap hsmmc: adaptation of sdma descriptor
>> autoloading feature
>>
>> Start to use the sDMA descriptor autoloading feature.
>> For large datablocks, the MMC driver has to repeatedly setup,
>> program and teardown the dma channel for each element of the
>> sglist received in omap_hsmmc_request.
>>
>> By using descriptor autoloading, transfers from / to each element of
>> the sglist is pre programmed into a linked list. The sDMA driver
>> completes the entire transaction and provides a single interrupt.
>>
>> Due to this, number of dma interrupts for a typical 100MB transfer on the MMC is
>> reduced from 25000 to about 400 (approximate). Transfer speeds are
>> improved by ~5%.
>> (Though it varies on the size of read / write & improves on huge transfers)
>>
>> Descriptor autoloading is available only in 3630 and 4430 (as of now).
>> Hence normal DMA mode is also retained.
>>
>> Signed-off-by: Venkatraman S <svenkatr@ti.com>
>> CC: Adrian Hunter <adrian.hunter@nokia.com>
>> CC: Madhusudhan C <madhu.cr@ti.com>
>> CC: Shilimkar Santosh <santosh.shilimkar@ti.com>
>> CC: Tony Lindgren <tony@atomide.com>
>> ---
>>  Changes since previous version
>>   * Rebased to Adrian Hunter's interrupt syncronisation patch
>>              https://patchwork.kernel.org/patch/94670/
>>   * Rename ICR name definition
>>  drivers/mmc/host/omap_hsmmc.c |  148 ++++++++++++++++++++++++++++++++++------
>>  1 files changed, 125 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index 65093d4..095fd94 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -102,6 +102,8 @@
>>  #define SRD                  (1 << 26)
>>  #define SOFTRESET            (1 << 1)
>>  #define RESETDONE            (1 << 0)
>> +/* End of superblock indicator for sglist transfers */
>> +#define DMA_ICR_SGLIST_END   0x4D00
>>
>>  /*
>>   * FIXME: Most likely all the data using these _DEVID defines should come
>> @@ -118,6 +120,12 @@
>>  #define OMAP_MMC_MASTER_CLOCK        96000000
>>  #define DRIVER_NAME          "mmci-omap-hs"
>>
>> +#define DMA_TYPE_NODMA       0
> " DMA_TYPE_NODMA" doesn't make sense
>> +#define DMA_TYPE_SDMA        1
>> +#define DMA_TYPE_SDMA_DLOAD 2
>
> How about ??
> #define TRANSFER_NODMA                  0
> #define TRANSFER_SDMA_NORMAL            1
> #define TRANSFER_SDMA_DESCRIPTOR        2
> I think ADMA is also in pipeline, so it can become then
> #define TRANSFER_ADMA_DESCRIPTOR        3

Yes  I was planning to use 3 for ADMA, but the names don't
make a big difference.

>> +
>> +#define DMA_CTRL_BUF_SIZE    (PAGE_SIZE * 3)
>> +
>>  /* Timeouts for entering power saving states on inactivity, msec */
>>  #define OMAP_MMC_DISABLED_TIMEOUT    100
>>  #define OMAP_MMC_SLEEP_TIMEOUT               1000
>> @@ -170,7 +178,11 @@ struct omap_hsmmc_host {
>>       u32                     bytesleft;
>>       int                     suspended;
>>       int                     irq;
>> -     int                     use_dma, dma_ch;
>> +     int                     dma_caps;
>> +     int                     dma_in_use;
> This flag isn't necessary. What you are doing is
> just renaming it. Can't you avoid that ??
> Inudertand the intent behind "dma_in_use " instead
> of "use_dma", but you can surely avoid this change.

Actually there is a shift in the meaning of these variables
so I believe the change is necessary.
With my changes, the type of DMA to use (SDMA, SDMA_DESC_LOAD)
[and in the future NODMA, ADMA] can vary on a per transfer basis.
Previously use_dma was a static capability variable of whether DMA
was available. That has been move to dma_caps. The dma_in_use,
as the name more intuitively suggests, keeps track of the type of transfer
used in the current transaction.


>>  {
>>       unsigned int irq_mask;
>>
>> -     if (host->use_dma)
>> +     if (host->dma_in_use)
> This will go with no flag change.

Explanation as above

>>               irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
>>       else
>>               irq_mask = INT_EN_MASK;
>> @@ -813,7 +825,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host
>> *host, struct mmc_command *cmd,
>>                       cmdreg &= ~(DDIR);
>>       }
>>
>> -     if (host->use_dma)
>> +     if (host->dma_in_use)
> ditto
>>               cmdreg |= DMA_EN;
>>
>>       host->req_in_progress = 1;
>> @@ -842,7 +854,7 @@ static void omap_hsmmc_request_done(struct
>> omap_hsmmc_host *host, struct mmc_req
>>
>>       omap_hsmmc_disable_irq(host);
>>       /* Do not complete the request if DMA is still in progress */
>> -     if (mrq->data && host->use_dma && dma_ch != -1)
>> +     if (mrq->data && host->dma_in_use && dma_ch != -1)
> ditto
>>               return;
>>       host->mrq = NULL;
>>       mmc_request_done(host->mmc, mrq);
>> @@ -920,7 +932,7 @@ static void omap_hsmmc_dma_cleanup(struct
>> omap_hsmmc_host *host, int errno)
>>       host->dma_ch = -1;
>>       spin_unlock(&host->irq_lock);
>>
>> -     if (host->use_dma && dma_ch != -1) {
>> +     if (host->dma_in_use && dma_ch != -1) {
> ditto
>>               dma_unmap_sg(mmc_dev(host->mmc), host->data->sg, host->dma_len,
>>                       omap_hsmmc_get_dma_dir(host, host->data));
>>               omap_free_dma(dma_ch);
>> @@ -1266,7 +1278,6 @@ static void omap_hsmmc_config_dma_params(struct
>> omap_hsmmc_host *host,
>>                       omap_hsmmc_get_dma_sync_dev(host, data),
>>                       !(data->flags & MMC_DATA_WRITE));
>>
>> -     omap_start_dma(dma_ch);
>>  }
>>
>>  /*
>> @@ -1279,21 +1290,23 @@ static void omap_hsmmc_dma_cb(int lch, u16
>> ch_status, void *cb_data)
>>       int dma_ch, req_in_progress;
>>
>>       if (ch_status & OMAP2_DMA_MISALIGNED_ERR_IRQ)
>> -             dev_dbg(mmc_dev(host->mmc), "MISALIGNED_ADRS_ERR\n");
>> +             dev_dbg(mmc_dev(host->mmc), "Misaligned address error\n");
>>
>>       spin_lock(&host->irq_lock);
>>       if (host->dma_ch < 0) {
>>               spin_unlock(&host->irq_lock);
>>               return;
>>       }
>> -
>> -     host->dma_sg_idx++;
>> -     if (host->dma_sg_idx < host->dma_len) {
>> -             /* Fire up the next transfer. */
>> -             omap_hsmmc_config_dma_params(host, data,
>> +     if (host->dma_in_use == DMA_TYPE_SDMA) {
>> +             host->dma_sg_idx++;
>> +             if (host->dma_sg_idx < host->dma_len) {
>> +                     /* Fire up the next transfer. */
>> +                     omap_hsmmc_config_dma_params(host, data,
>>                                          data->sg + host->dma_sg_idx);
>> -             spin_unlock(&host->irq_lock);
>> -             return;
>> +                     omap_start_dma(host->dma_ch);
>> +                     spin_unlock(&host->irq_lock);
>> +                     return;
>> +             }
>>       }
>>
>>       dma_unmap_sg(mmc_dev(host->mmc), data->sg, host->dma_len,
>> @@ -1315,10 +1328,19 @@ static void omap_hsmmc_dma_cb(int lch, u16
>> ch_status, void *cb_data)
>>       }
>>  }
>>
>> +static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host)
>> +{
>> +     if (host->dma_in_use == DMA_TYPE_SDMA)
>> +             omap_start_dma(host->dma_ch);
>> +     else if (host->dma_in_use == DMA_TYPE_SDMA_DLOAD)
>> +             return omap_start_dma_sglist_transfers(host->dma_ch, -1);
> Instead of "-1" , some macro for "NO_PAUSE" would have been better.
OK

>> +
>> +     return 0;
>> +}
>>  /*
>> - * Routine to configure and start DMA for the MMC card
>> + * Routine to configure DMA for the MMC card
>>   */
>> -static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host,
>> +static int omap_hsmmc_configure_sdma(struct omap_hsmmc_host *host,
>>                                       struct mmc_request *req)
>>  {
>>       int dma_ch = 0, ret = 0, i;
>> @@ -1359,6 +1381,56 @@ static int omap_hsmmc_start_dma_transfer(struct
>> omap_hsmmc_host *host,
>>       return 0;
>>  }
>>
>> +static int omap_hsmmc_configure_sdma_sglist(struct omap_hsmmc_host *host,
>> +             struct mmc_request *req)
>> +{
>> +     int i;
>> +     struct omap_dma_sglist_node *sglist, *snode;
>> +     struct mmc_data *data = req->data;
>> +     int blksz;
>> +     int dmadir = omap_hsmmc_get_dma_dir(host, data);
>> +     struct omap_dma_sglist_type2a_params *t2p;
>> +
>> +     sglist = (struct omap_dma_sglist_node *) host->dma_ctrl_buf;
>> +     snode = sglist;
>> +     blksz = host->data->blksz;
>> +
>> +     if ((host->dma_len * sizeof(*snode)) > DMA_CTRL_BUF_SIZE) {
>> +             dev_err(mmc_dev(host->mmc), "not enough sglist memory %d\n",
>> +                     host->dma_len);
>> +             return -ENOMEM;
>> +     }
>> +     for (i = 0; i < host->dma_len; snode++, i++) {
>> +             snode->desc_type = OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a;
>> +             snode->num_of_elem = blksz / 4;
>> +             t2p = &snode->sg_node.t2a;
>> +
>> +             if (dmadir == DMA_FROM_DEVICE) {
>> +                     t2p->src_addr = host->mapbase + OMAP_HSMMC_DATA;
>> +                     t2p->dst_addr = sg_dma_address(data->sg + i);
>> +             } else {
>> +                     t2p->dst_addr = host->mapbase + OMAP_HSMMC_DATA;
>> +                     t2p->src_addr = sg_dma_address(data->sg + i);
>> +             }
>> +             snode->flags =
>> +                     OMAP_DMA_LIST_DST_VALID | OMAP_DMA_LIST_SRC_VALID;
>> +
>> +             t2p->cfn_fn = sg_dma_len(data->sg + i) / host->data->blksz;
>> +             t2p->cicr = DMA_ICR_SGLIST_END;
>> +
>> +             t2p->dst_frame_idx_or_pkt_size = 0;
>> +             t2p->src_frame_idx_or_pkt_size = 0;
>> +             t2p->dst_elem_idx = 0;
>> +             t2p->src_elem_idx = 0;
>> +     }
>> +     dev_dbg(mmc_dev(host->mmc), "new sglist %x len =%d\n",
>> +                     host->dma_ctrl_buf_phy, i);
>> +     omap_set_dma_sglist_mode(host->dma_ch, sglist,
>> +                     host->dma_ctrl_buf_phy, i, NULL);
>> +     omap_dma_set_sglist_fastmode(host->dma_ch, 1);
>
> You should check for return value of above two. If any one of above fails
> the transfer will fail BADLY

These functions have no return code (Similar to omap_start_dma, which doesn't
have return code either)

>> +     return 0;
>> +}
>> +
>>  static void set_data_timeout(struct omap_hsmmc_host *host,
>>                            unsigned int timeout_ns,
>>                            unsigned int timeout_clks)
>> @@ -1420,14 +1492,23 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host
>> *host, struct mmc_request *req)
>>                                       | (req->data->blocks << 16));
>>       set_data_timeout(host, req->data->timeout_ns, req->data->timeout_clks);
>>
>> -     if (host->use_dma) {
>> -             ret = omap_hsmmc_start_dma_transfer(host, req);
>> -             if (ret != 0) {
>> -                     dev_dbg(mmc_dev(host->mmc), "MMC start dma failure\n");
>> +     if (host->dma_caps & DMA_TYPE_SDMA) {
>> +             ret = omap_hsmmc_configure_sdma(host, req);
>> +             if (ret)
>>                       return ret;
>> -             }
>> +             host->dma_in_use = DMA_TYPE_SDMA;
>>       }
>> -     return 0;
>> +     if ((host->dma_caps & DMA_TYPE_SDMA_DLOAD) &&
>> +             host->data->sg_len > 4) {
>> +             ret = omap_hsmmc_configure_sdma_sglist(host, req);
>> +             if (ret)
>> +                     return ret;
>> +             host->dma_in_use = DMA_TYPE_SDMA_DLOAD;
>> +
>> +     }
>> +     ret = omap_hsmmc_start_dma_transfer(host);
>> +     return ret;
>> +
>>  }
>>
>>  /*
>> @@ -2007,7 +2088,9 @@ static int __init omap_hsmmc_probe(struct
>> platform_device *pdev)
>>       host->mmc       = mmc;
>>       host->pdata     = pdata;
>>       host->dev       = &pdev->dev;
>> -     host->use_dma   = 1;
>> +     host->dma_caps  = DMA_TYPE_SDMA;
>> +     host->dma_in_use        = DMA_TYPE_NODMA;
>> +     host->dma_ctrl_buf = NULL;
>>       host->dev->dma_mask = &pdata->dma_mask;
>>       host->dma_ch    = -1;
>>       host->irq       = irq;
>> @@ -2088,6 +2171,15 @@ static int __init omap_hsmmc_probe(struct
>> platform_device *pdev)
>>                                                       " clk failed\n");
>>       }
>>
>> +     if (cpu_is_omap44xx() || cpu_is_omap3630()) {
> Can we avoid above by passing this part of platform data??
> devices.c

I am not clear about the method. The board files export the
omap_mmc_platform_data.
Does it imply that all board files have to change and export
the capability so that it can be queried ?

>> +             host->dma_ctrl_buf = dma_alloc_coherent(NULL,
>> +                                     DMA_CTRL_BUF_SIZE,
>> +                                     &host->dma_ctrl_buf_phy,
>> +                                     0);
>> +             if (host->dma_ctrl_buf != NULL)
>> +                     host->dma_caps |= DMA_TYPE_SDMA_DLOAD;
>> +     }
>> +
>>       /* Since we do only SG emulation, we can have as many segs
>>        * as we want. */
> Not your patch but above commenting style isn't right
OK
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Madhusudhan]

<snip>

> >> +     if (cpu_is_omap44xx() || cpu_is_omap3630()) {
> > Can we avoid above by passing this part of platform data??
> > devices.c
> 
> I am not clear about the method. The board files export the
> omap_mmc_platform_data.
> Does it imply that all board files have to change and export
> the capability so that it can be queried ?
> 

I don’t see why this capability needs to be passed from the platform data.
As this feature is dependant on omap it is okay to have a cpu check in the
driver as the patch is doing it.

> >> +             host->dma_ctrl_buf = dma_alloc_coherent(NULL,
> >> +                                     DMA_CTRL_BUF_SIZE,
> >> +                                     &host->dma_ctrl_buf_phy,
> >> +                                     0);
> >> +             if (host->dma_ctrl_buf != NULL)
> >> +                     host->dma_caps |= DMA_TYPE_SDMA_DLOAD;
> >> +     }
> >> +
> >>       /* Since we do only SG emulation, we can have as many segs
> >>        * as we want. */
> > Not your patch but above commenting style isn't right
> OK

RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Shilimkar, Santosh]

> -----Original Message-----
> From: Chikkature Rajashekar, Madhusudhan
> Sent: Wednesday, May 05, 2010 10:51 PM
> To: S, Venkatraman; Shilimkar, Santosh
> Cc: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-kernel@lists.arm.linux.org.uk;
> 'Adrian Hunter'; Kadiyala, Kishore; 'Tony Lindgren'
> Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
> 
> <snip>
> 
> > >> +     if (cpu_is_omap44xx() || cpu_is_omap3630()) {
> > > Can we avoid above by passing this part of platform data??
> > > devices.c
> >
> > I am not clear about the method. The board files export the
> > omap_mmc_platform_data.
> > Does it imply that all board files have to change and export
> > the capability so that it can be queried ?
> >
> 
> I don’t see why this capability needs to be passed from the platform data.
> As this feature is dependant on omap it is okay to have a cpu check in the
> driver as the patch is doing it.
> 
Because Descriptor load feature is not MMC IP capability but SDMA capability.
To make MMC driver portable we should avoid cluttering with CPU specific checks.

This is recommended way unless and until you think otherwise.

Regards,
Santosh

RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Shilimkar, Santosh]

> -----Original Message-----
> From: svenkatr@gmail.com [mailto:svenkatr@gmail.com] On Behalf Of Venkatraman S
> Sent: Wednesday, May 05, 2010 9:49 PM
> To: Shilimkar, Santosh
> Cc: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-kernel@lists.arm.linux.org.uk;
> Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
> 
> [Long sections have been trimmed to the context of discussion]
> Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote:
> >
> >> -----Original Message-----
> >> From: svenkatr@gmail.com [mailto:svenkatr@gmail.com] On Behalf Of Venkatraman S
> >> Sent: Thursday, April 29, 2010 11:05 PM
> >> To: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-kernel@lists.arm.linux.org.uk
> >> Cc: Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Shilimkar, Santosh; Tony
> >> Lindgren
> >> Subject: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
> >>
> >> From 1c63dd42fc6c563c931168779ce73401332faa52 Mon Sep 17 00:00:00 2001
> >> From: Venkatraman S <svenkatr@ti.com>
> >> Date: Thu, 29 Apr 2010 22:43:31 +0530
> >> Subject: [PATCH 2/2] omap hsmmc: adaptation of sdma descriptor
> >> autoloading feature
> >>
> >> Start to use the sDMA descriptor autoloading feature.
> >> For large datablocks, the MMC driver has to repeatedly setup,
> >> program and teardown the dma channel for each element of the
> >> sglist received in omap_hsmmc_request.
> >>
> >> By using descriptor autoloading, transfers from / to each element of
> >> the sglist is pre programmed into a linked list. The sDMA driver
> >> completes the entire transaction and provides a single interrupt.
> >>
> >> Due to this, number of dma interrupts for a typical 100MB transfer on the MMC is
> >> reduced from 25000 to about 400 (approximate). Transfer speeds are
> >> improved by ~5%.
> >> (Though it varies on the size of read / write & improves on huge transfers)
> >>
> >> Descriptor autoloading is available only in 3630 and 4430 (as of now).
> >> Hence normal DMA mode is also retained.
> >>
> >> Signed-off-by: Venkatraman S <svenkatr@ti.com>
> >> CC: Adrian Hunter <adrian.hunter@nokia.com>
> >> CC: Madhusudhan C <madhu.cr@ti.com>
> >> CC: Shilimkar Santosh <santosh.shilimkar@ti.com>
> >> CC: Tony Lindgren <tony@atomide.com>
> >> ---
> >>  Changes since previous version
> >>   * Rebased to Adrian Hunter's interrupt syncronisation patch
> >>              https://patchwork.kernel.org/patch/94670/
> >>   * Rename ICR name definition
> >>  drivers/mmc/host/omap_hsmmc.c |  148 ++++++++++++++++++++++++++++++++++------
> >>  1 files changed, 125 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> >> index 65093d4..095fd94 100644
> >> --- a/drivers/mmc/host/omap_hsmmc.c
> >> +++ b/drivers/mmc/host/omap_hsmmc.c
> >> @@ -102,6 +102,8 @@
> >>  #define SRD                  (1 << 26)
> >>  #define SOFTRESET            (1 << 1)
> >>  #define RESETDONE            (1 << 0)
> >> +/* End of superblock indicator for sglist transfers */
> >> +#define DMA_ICR_SGLIST_END   0x4D00
> >>
> >>  /*
> >>   * FIXME: Most likely all the data using these _DEVID defines should come
> >> @@ -118,6 +120,12 @@
> >>  #define OMAP_MMC_MASTER_CLOCK        96000000
> >>  #define DRIVER_NAME          "mmci-omap-hs"
> >>
> >> +#define DMA_TYPE_NODMA       0
> > " DMA_TYPE_NODMA" doesn't make sense
> >> +#define DMA_TYPE_SDMA        1
> >> +#define DMA_TYPE_SDMA_DLOAD 2
> >
> > How about ??
> > #define TRANSFER_NODMA                  0
> > #define TRANSFER_SDMA_NORMAL            1
> > #define TRANSFER_SDMA_DESCRIPTOR        2
> > I think ADMA is also in pipeline, so it can become then
> > #define TRANSFER_ADMA_DESCRIPTOR        3
> 
> Yes  I was planning to use 3 for ADMA, but the names don't
> make a big difference.
> 
Good. Name does makes the difference when somebody reads the code.
" DMA_TYPE_NODMA" what you make out from this if somebody has no
background of what patch is doing.

> >> +
> >> +#define DMA_CTRL_BUF_SIZE    (PAGE_SIZE * 3)
> >> +
> >>  /* Timeouts for entering power saving states on inactivity, msec */
> >>  #define OMAP_MMC_DISABLED_TIMEOUT    100
> >>  #define OMAP_MMC_SLEEP_TIMEOUT               1000
> >> @@ -170,7 +178,11 @@ struct omap_hsmmc_host {
> >>       u32                     bytesleft;
> >>       int                     suspended;
> >>       int                     irq;
> >> -     int                     use_dma, dma_ch;
> >> +     int                     dma_caps;
> >> +     int                     dma_in_use;
> > This flag isn't necessary. What you are doing is
> > just renaming it. Can't you avoid that ??
> > Inudertand the intent behind "dma_in_use " instead
> > of "use_dma", but you can surely avoid this change.
> 
> Actually there is a shift in the meaning of these variables
> so I believe the change is necessary.
> With my changes, the type of DMA to use (SDMA, SDMA_DESC_LOAD)
> [and in the future NODMA, ADMA] can vary on a per transfer basis.
> Previously use_dma was a static capability variable of whether DMA
> was available. That has been move to dma_caps. The dma_in_use,
> as the name more intuitively suggests, keeps track of the type of transfer
> used in the current transaction.
> 
I still feel it's no necessary considering the no. of lines gets changed because
of this. 
You take a call. I am fine with it.
> 
> >>  {
> >>       unsigned int irq_mask;
> >>
> >> -     if (host->use_dma)
> >> +     if (host->dma_in_use)
> > This will go with no flag change.
> 
> Explanation as above
> 
> >>               irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
> >>       else
> >>               irq_mask = INT_EN_MASK;
> >> @@ -813,7 +825,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host
> >> *host, struct mmc_command *cmd,
> >>                       cmdreg &= ~(DDIR);
> >>       }
> >>
> >> -     if (host->use_dma)
> >> +     if (host->dma_in_use)
> > ditto
> >>               cmdreg |= DMA_EN;
> >>
> >>       host->req_in_progress = 1;
<snip ..>


> >> +static int omap_hsmmc_configure_sdma_sglist(struct omap_hsmmc_host *host,
> >> +             struct mmc_request *req)
> >> +{
> >> +     int i;
> >> +     struct omap_dma_sglist_node *sglist, *snode;
> >> +     struct mmc_data *data = req->data;
> >> +     int blksz;
> >> +     int dmadir = omap_hsmmc_get_dma_dir(host, data);
> >> +     struct omap_dma_sglist_type2a_params *t2p;
> >> +
> >> +     sglist = (struct omap_dma_sglist_node *) host->dma_ctrl_buf;
> >> +     snode = sglist;
> >> +     blksz = host->data->blksz;
> >> +
> >> +     if ((host->dma_len * sizeof(*snode)) > DMA_CTRL_BUF_SIZE) {
> >> +             dev_err(mmc_dev(host->mmc), "not enough sglist memory %d\n",
> >> +                     host->dma_len);
> >> +             return -ENOMEM;
> >> +     }
> >> +     for (i = 0; i < host->dma_len; snode++, i++) {
> >> +             snode->desc_type = OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a;
> >> +             snode->num_of_elem = blksz / 4;
> >> +             t2p = &snode->sg_node.t2a;
> >> +
> >> +             if (dmadir == DMA_FROM_DEVICE) {
> >> +                     t2p->src_addr = host->mapbase + OMAP_HSMMC_DATA;
> >> +                     t2p->dst_addr = sg_dma_address(data->sg + i);
> >> +             } else {
> >> +                     t2p->dst_addr = host->mapbase + OMAP_HSMMC_DATA;
> >> +                     t2p->src_addr = sg_dma_address(data->sg + i);
> >> +             }
> >> +             snode->flags =
> >> +                     OMAP_DMA_LIST_DST_VALID | OMAP_DMA_LIST_SRC_VALID;
> >> +
> >> +             t2p->cfn_fn = sg_dma_len(data->sg + i) / host->data->blksz;
> >> +             t2p->cicr = DMA_ICR_SGLIST_END;
> >> +
> >> +             t2p->dst_frame_idx_or_pkt_size = 0;
> >> +             t2p->src_frame_idx_or_pkt_size = 0;
> >> +             t2p->dst_elem_idx = 0;
> >> +             t2p->src_elem_idx = 0;
> >> +     }
> >> +     dev_dbg(mmc_dev(host->mmc), "new sglist %x len =%d\n",
> >> +                     host->dma_ctrl_buf_phy, i);
> >> +     omap_set_dma_sglist_mode(host->dma_ch, sglist,
> >> +                     host->dma_ctrl_buf_phy, i, NULL);
> >> +     omap_dma_set_sglist_fastmode(host->dma_ch, 1);
> >
> > You should check for return value of above two. If any one of above fails
> > the transfer will fail BADLY
> 
> These functions have no return code (Similar to omap_start_dma, which doesn't
> have return code either)

Are you sure ? Here is the function body.

+int omap_set_dma_sglist_mode(int lch, struct omap_dma_sglist_node *sgparams,
+	dma_addr_t padd, int nelem, struct omap_dma_channel_params *chparams)
+{
+	struct omap_dma_list_config_params *lcfg;
+	int l = DMA_LIST_CDP_LISTMODE; /* Enable Linked list mode in CDP */
+
+	if ((dma_caps0_status & DMA_CAPS_SGLIST_SUPPORT) == 0) {
+		printk(KERN_ERR "omap DMA: sglist feature not supported\n");
+		return -EPERM;
+	}
+	if (dma_chan[lch].flags & OMAP_DMA_ACTIVE) {
+		printk(KERN_ERR "omap DMA: configuring active DMA channel\n");
+		return -EPERM;
+	}
+
+	if (padd == 0) {
+		printk(KERN_ERR "omap DMA: sglist invalid dma_addr\n");
+		return -EINVAL;
+	}
+	lcfg = &dma_chan[lch].list_config;
+
+	lcfg->sghead = sgparams;
+	lcfg->num_elem = nelem;
+	lcfg->sgheadphy = padd;
+	lcfg->pausenode = -1;
+
+
+	if (NULL == chparams)
+		l |= DMA_LIST_CDP_FASTMODE;
+	else
+		omap_set_dma_params(lch, chparams);
+
+	dma_write(l, CDP(lch));
+	dma_write(0, CCDN(lch)); /* Reset List index numbering */
+	/* Initialize frame and element counters to invalid values */
+	dma_write(OMAP_DMA_INVALID_FRAME_COUNT, CCFN(lch));
+	dma_write(OMAP_DMA_INVALID_ELEM_COUNT, CCEN(lch));
+
+	return dma_sglist_set_phy_params(sgparams, lcfg->sgheadphy,
+			nelem, dma_read(CICR(lch)));
+
+}


> 
> >> +     return 0;
> >> +}
> >> +
> >>  static void set_data_timeout(struct omap_hsmmc_host *host,
> >>                            unsigned int timeout_ns,
> >>                            unsigned int timeout_clks)
> >> @@ -1420,14 +1492,23 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host
> >> *host, struct mmc_request *req)
> >>                                       | (req->data->blocks << 16));
> >>       set_data_timeout(host, req->data->timeout_ns, req->data->timeout_clks);
> >>
> >> -     if (host->use_dma) {
> >> -             ret = omap_hsmmc_start_dma_transfer(host, req);
> >> -             if (ret != 0) {
> >> -                     dev_dbg(mmc_dev(host->mmc), "MMC start dma failure\n");
> >> +     if (host->dma_caps & DMA_TYPE_SDMA) {
> >> +             ret = omap_hsmmc_configure_sdma(host, req);
> >> +             if (ret)
> >>                       return ret;
> >> -             }
> >> +             host->dma_in_use = DMA_TYPE_SDMA;
> >>       }
> >> -     return 0;
> >> +     if ((host->dma_caps & DMA_TYPE_SDMA_DLOAD) &&
> >> +             host->data->sg_len > 4) {
> >> +             ret = omap_hsmmc_configure_sdma_sglist(host, req);
> >> +             if (ret)
> >> +                     return ret;
> >> +             host->dma_in_use = DMA_TYPE_SDMA_DLOAD;
> >> +
> >> +     }
> >> +     ret = omap_hsmmc_start_dma_transfer(host);
> >> +     return ret;
> >> +
> >>  }
> >>
> >>  /*
> >> @@ -2007,7 +2088,9 @@ static int __init omap_hsmmc_probe(struct
> >> platform_device *pdev)
> >>       host->mmc       = mmc;
> >>       host->pdata     = pdata;
> >>       host->dev       = &pdev->dev;
> >> -     host->use_dma   = 1;
> >> +     host->dma_caps  = DMA_TYPE_SDMA;
> >> +     host->dma_in_use        = DMA_TYPE_NODMA;
> >> +     host->dma_ctrl_buf = NULL;
> >>       host->dev->dma_mask = &pdata->dma_mask;
> >>       host->dma_ch    = -1;
> >>       host->irq       = irq;
> >> @@ -2088,6 +2171,15 @@ static int __init omap_hsmmc_probe(struct
> >> platform_device *pdev)
> >>                                                       " clk failed\n");
> >>       }
> >>
> >> +     if (cpu_is_omap44xx() || cpu_is_omap3630()) {
> > Can we avoid above by passing this part of platform data??
> > devices.c
> 
> I am not clear about the method. The board files export the
> omap_mmc_platform_data.
> Does it imply that all board files have to change and export
> the capability so that it can be queried ?
No. You don't have to modify the board files. This would need
change in devices.c which common for all omap boards.

I don't have a strong opinion on this point but just put forth an
alternate way to avoid such SOC specific check in drivers.
You can take call on this
> 
> >> +             host->dma_ctrl_buf = dma_alloc_coherent(NULL,
> >> +                                     DMA_CTRL_BUF_SIZE,
> >> +                                     &host->dma_ctrl_buf_phy,
> >> +                                     0);
> >> +             if (host->dma_ctrl_buf != NULL)
> >> +                     host->dma_caps |= DMA_TYPE_SDMA_DLOAD;
> >> +     }
> >> +
> >>       /* Since we do only SG emulation, we can have as many segs
> >>        * as we want. */
> > Not your patch but above commenting style isn't right
> OK
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Venkatraman S]

Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote:
>> -----Original Message-----
>> From: svenkatr@gmail.com [mailto:svenkatr@gmail.com] On Behalf Of Venkatraman S
>> Sent: Wednesday, May 05, 2010 9:49 PM
>> To: Shilimkar, Santosh
>> Cc: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-kernel@lists.arm.linux.org.uk;
>> Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
>> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
>>
>> [Long sections have been trimmed to the context of discussion]
>> Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote:
>> >
>> >> -----Original Message-----
>> >> From: svenkatr@gmail.com [mailto:svenkatr@gmail.com] On Behalf Of Venkatraman S
>> >> Sent: Thursday, April 29, 2010 11:05 PM
>> >> To: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-kernel@lists.arm.linux.org.uk
>> >> Cc: Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Shilimkar, Santosh; Tony
>> >> Lindgren
>> >> Subject: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
>> >>
>> >> From 1c63dd42fc6c563c931168779ce73401332faa52 Mon Sep 17 00:00:00 2001
>> >> From: Venkatraman S <svenkatr@ti.com>
>> >> Date: Thu, 29 Apr 2010 22:43:31 +0530
>> >> Subject: [PATCH 2/2] omap hsmmc: adaptation of sdma descriptor
>> >> autoloading feature
>> >>
>> >> Start to use the sDMA descriptor autoloading feature.
>> >> For large datablocks, the MMC driver has to repeatedly setup,
>> >> program and teardown the dma channel for each element of the
>> >> sglist received in omap_hsmmc_request.
>> >>
>> >> By using descriptor autoloading, transfers from / to each element of
>> >> the sglist is pre programmed into a linked list. The sDMA driver
>> >> completes the entire transaction and provides a single interrupt.
>> >>
>> >> Due to this, number of dma interrupts for a typical 100MB transfer on the MMC is
>> >> reduced from 25000 to about 400 (approximate). Transfer speeds are
>> >> improved by ~5%.
>> >> (Though it varies on the size of read / write & improves on huge transfers)
>> >>
>> >> Descriptor autoloading is available only in 3630 and 4430 (as of now).
>> >> Hence normal DMA mode is also retained.
>> >>
>> >> Signed-off-by: Venkatraman S <svenkatr@ti.com>
>> >> CC: Adrian Hunter <adrian.hunter@nokia.com>
>> >> CC: Madhusudhan C <madhu.cr@ti.com>
>> >> CC: Shilimkar Santosh <santosh.shilimkar@ti.com>
>> >> CC: Tony Lindgren <tony@atomide.com>
>> >> ---
>> >>  Changes since previous version
>> >>   * Rebased to Adrian Hunter's interrupt syncronisation patch
>> >>              https://patchwork.kernel.org/patch/94670/
>> >>   * Rename ICR name definition
>> >>  drivers/mmc/host/omap_hsmmc.c |  148 ++++++++++++++++++++++++++++++++++------
>> >>  1 files changed, 125 insertions(+), 23 deletions(-)
>> >>
>> >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> >> index 65093d4..095fd94 100644
>> >> --- a/drivers/mmc/host/omap_hsmmc.c
>> >> +++ b/drivers/mmc/host/omap_hsmmc.c
>> >> @@ -102,6 +102,8 @@
>> >>  #define SRD                  (1 << 26)
>> >>  #define SOFTRESET            (1 << 1)
>> >>  #define RESETDONE            (1 << 0)
>> >> +/* End of superblock indicator for sglist transfers */
>> >> +#define DMA_ICR_SGLIST_END   0x4D00
>> >>
>> >>  /*
>> >>   * FIXME: Most likely all the data using these _DEVID defines should come
>> >> @@ -118,6 +120,12 @@
>> >>  #define OMAP_MMC_MASTER_CLOCK        96000000
>> >>  #define DRIVER_NAME          "mmci-omap-hs"
>> >>
>> >> +#define DMA_TYPE_NODMA       0
>> > " DMA_TYPE_NODMA" doesn't make sense
>> >> +#define DMA_TYPE_SDMA        1
>> >> +#define DMA_TYPE_SDMA_DLOAD 2
>> >
>> > How about ??
>> > #define TRANSFER_NODMA                  0
>> > #define TRANSFER_SDMA_NORMAL            1
>> > #define TRANSFER_SDMA_DESCRIPTOR        2
>> > I think ADMA is also in pipeline, so it can become then
>> > #define TRANSFER_ADMA_DESCRIPTOR        3
>>
>> Yes  I was planning to use 3 for ADMA, but the names don't
>> make a big difference.
>>
> Good. Name does makes the difference when somebody reads the code.
> " DMA_TYPE_NODMA" what you make out from this if somebody has no
> background of what patch is doing.

How about DMA_TYPE_NONE ?

>
>> >> +
>> >> +#define DMA_CTRL_BUF_SIZE    (PAGE_SIZE * 3)
>> >> +
>> >>  /* Timeouts for entering power saving states on inactivity, msec */
>> >>  #define OMAP_MMC_DISABLED_TIMEOUT    100
>> >>  #define OMAP_MMC_SLEEP_TIMEOUT               1000
>> >> @@ -170,7 +178,11 @@ struct omap_hsmmc_host {
>> >>       u32                     bytesleft;
>> >>       int                     suspended;
>> >>       int                     irq;
>> >> -     int                     use_dma, dma_ch;
>> >> +     int                     dma_caps;
>> >> +     int                     dma_in_use;
>> > This flag isn't necessary. What you are doing is
>> > just renaming it. Can't you avoid that ??
>> > Inudertand the intent behind "dma_in_use " instead
>> > of "use_dma", but you can surely avoid this change.
>>
>> Actually there is a shift in the meaning of these variables
>> so I believe the change is necessary.
>> With my changes, the type of DMA to use (SDMA, SDMA_DESC_LOAD)
>> [and in the future NODMA, ADMA] can vary on a per transfer basis.
>> Previously use_dma was a static capability variable of whether DMA
>> was available. That has been move to dma_caps. The dma_in_use,
>> as the name more intuitively suggests, keeps track of the type of transfer
>> used in the current transaction.
>>
> I still feel it's no necessary considering the no. of lines gets changed because
> of this.
> You take a call. I am fine with it.
>>
>> >>  {
>> >>       unsigned int irq_mask;
>> >>
>> >> -     if (host->use_dma)
>> >> +     if (host->dma_in_use)
>> > This will go with no flag change.
>>
>> Explanation as above
>>
>> >>               irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
>> >>       else
>> >>               irq_mask = INT_EN_MASK;
>> >> @@ -813,7 +825,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host
>> >> *host, struct mmc_command *cmd,
>> >>                       cmdreg &= ~(DDIR);
>> >>       }
>> >>
>> >> -     if (host->use_dma)
>> >> +     if (host->dma_in_use)
>> > ditto
>> >>               cmdreg |= DMA_EN;
>> >>
>> >>       host->req_in_progress = 1;
> <snip ..>
>
>
>> >> +static int omap_hsmmc_configure_sdma_sglist(struct omap_hsmmc_host *host,
>> >> +             struct mmc_request *req)
>> >> +{
>> >> +     int i;
>> >> +     struct omap_dma_sglist_node *sglist, *snode;
>> >> +     struct mmc_data *data = req->data;
>> >> +     int blksz;
>> >> +     int dmadir = omap_hsmmc_get_dma_dir(host, data);
>> >> +     struct omap_dma_sglist_type2a_params *t2p;
>> >> +
>> >> +     sglist = (struct omap_dma_sglist_node *) host->dma_ctrl_buf;
>> >> +     snode = sglist;
>> >> +     blksz = host->data->blksz;
>> >> +
>> >> +     if ((host->dma_len * sizeof(*snode)) > DMA_CTRL_BUF_SIZE) {
>> >> +             dev_err(mmc_dev(host->mmc), "not enough sglist memory %d\n",
>> >> +                     host->dma_len);
>> >> +             return -ENOMEM;
>> >> +     }
>> >> +     for (i = 0; i < host->dma_len; snode++, i++) {
>> >> +             snode->desc_type = OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a;
>> >> +             snode->num_of_elem = blksz / 4;
>> >> +             t2p = &snode->sg_node.t2a;
>> >> +
>> >> +             if (dmadir == DMA_FROM_DEVICE) {
>> >> +                     t2p->src_addr = host->mapbase + OMAP_HSMMC_DATA;
>> >> +                     t2p->dst_addr = sg_dma_address(data->sg + i);
>> >> +             } else {
>> >> +                     t2p->dst_addr = host->mapbase + OMAP_HSMMC_DATA;
>> >> +                     t2p->src_addr = sg_dma_address(data->sg + i);
>> >> +             }
>> >> +             snode->flags =
>> >> +                     OMAP_DMA_LIST_DST_VALID | OMAP_DMA_LIST_SRC_VALID;
>> >> +
>> >> +             t2p->cfn_fn = sg_dma_len(data->sg + i) / host->data->blksz;
>> >> +             t2p->cicr = DMA_ICR_SGLIST_END;
>> >> +
>> >> +             t2p->dst_frame_idx_or_pkt_size = 0;
>> >> +             t2p->src_frame_idx_or_pkt_size = 0;
>> >> +             t2p->dst_elem_idx = 0;
>> >> +             t2p->src_elem_idx = 0;
>> >> +     }
>> >> +     dev_dbg(mmc_dev(host->mmc), "new sglist %x len =%d\n",
>> >> +                     host->dma_ctrl_buf_phy, i);
>> >> +     omap_set_dma_sglist_mode(host->dma_ch, sglist,
>> >> +                     host->dma_ctrl_buf_phy, i, NULL);
>> >> +     omap_dma_set_sglist_fastmode(host->dma_ch, 1);
>> >
>> > You should check for return value of above two. If any one of above fails
>> > the transfer will fail BADLY
>>
>> These functions have no return code (Similar to omap_start_dma, which doesn't
>> have return code either)
>
> Are you sure ? Here is the function body.
>
> +int omap_set_dma_sglist_mode(int lch, struct omap_dma_sglist_node *sgparams,
> +       dma_addr_t padd, int nelem, struct omap_dma_channel_params *chparams)
> +{
> +       struct omap_dma_list_config_params *lcfg;
> +       int l = DMA_LIST_CDP_LISTMODE; /* Enable Linked list mode in CDP */
> +
> +       if ((dma_caps0_status & DMA_CAPS_SGLIST_SUPPORT) == 0) {
> +               printk(KERN_ERR "omap DMA: sglist feature not supported\n");
> +               return -EPERM;
> +       }
> +       if (dma_chan[lch].flags & OMAP_DMA_ACTIVE) {
> +               printk(KERN_ERR "omap DMA: configuring active DMA channel\n");
> +               return -EPERM;
> +       }
> +
> +       if (padd == 0) {
> +               printk(KERN_ERR "omap DMA: sglist invalid dma_addr\n");
> +               return -EINVAL;
> +       }
> +       lcfg = &dma_chan[lch].list_config;
> +
> +       lcfg->sghead = sgparams;
> +       lcfg->num_elem = nelem;
> +       lcfg->sgheadphy = padd;
> +       lcfg->pausenode = -1;
> +
> +
> +       if (NULL == chparams)
> +               l |= DMA_LIST_CDP_FASTMODE;
> +       else
> +               omap_set_dma_params(lch, chparams);
> +
> +       dma_write(l, CDP(lch));
> +       dma_write(0, CCDN(lch)); /* Reset List index numbering */
> +       /* Initialize frame and element counters to invalid values */
> +       dma_write(OMAP_DMA_INVALID_FRAME_COUNT, CCFN(lch));
> +       dma_write(OMAP_DMA_INVALID_ELEM_COUNT, CCEN(lch));
> +
> +       return dma_sglist_set_phy_params(sgparams, lcfg->sgheadphy,
> +                       nelem, dma_read(CICR(lch)));
> +
> +}
>
 OK. I missed that.
>
>>
>> >> +     return 0;
>> >> +}
>> >> +
>> >>  static void set_data_timeout(struct omap_hsmmc_host *host,
>> >>                            unsigned int timeout_ns,
>> >>                            unsigned int timeout_clks)
>> >> @@ -1420,14 +1492,23 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host
>> >> *host, struct mmc_request *req)
>> >>                                       | (req->data->blocks << 16));
>> >>       set_data_timeout(host, req->data->timeout_ns, req->data->timeout_clks);
>> >>
>> >> -     if (host->use_dma) {
>> >> -             ret = omap_hsmmc_start_dma_transfer(host, req);
>> >> -             if (ret != 0) {
>> >> -                     dev_dbg(mmc_dev(host->mmc), "MMC start dma failure\n");
>> >> +     if (host->dma_caps & DMA_TYPE_SDMA) {
>> >> +             ret = omap_hsmmc_configure_sdma(host, req);
>> >> +             if (ret)
>> >>                       return ret;
>> >> -             }
>> >> +             host->dma_in_use = DMA_TYPE_SDMA;
>> >>       }
>> >> -     return 0;
>> >> +     if ((host->dma_caps & DMA_TYPE_SDMA_DLOAD) &&
>> >> +             host->data->sg_len > 4) {
>> >> +             ret = omap_hsmmc_configure_sdma_sglist(host, req);
>> >> +             if (ret)
>> >> +                     return ret;
>> >> +             host->dma_in_use = DMA_TYPE_SDMA_DLOAD;
>> >> +
>> >> +     }
>> >> +     ret = omap_hsmmc_start_dma_transfer(host);
>> >> +     return ret;
>> >> +
>> >>  }
>> >>
>> >>  /*
>> >> @@ -2007,7 +2088,9 @@ static int __init omap_hsmmc_probe(struct
>> >> platform_device *pdev)
>> >>       host->mmc       = mmc;
>> >>       host->pdata     = pdata;
>> >>       host->dev       = &pdev->dev;
>> >> -     host->use_dma   = 1;
>> >> +     host->dma_caps  = DMA_TYPE_SDMA;
>> >> +     host->dma_in_use        = DMA_TYPE_NODMA;
>> >> +     host->dma_ctrl_buf = NULL;
>> >>       host->dev->dma_mask = &pdata->dma_mask;
>> >>       host->dma_ch    = -1;
>> >>       host->irq       = irq;
>> >> @@ -2088,6 +2171,15 @@ static int __init omap_hsmmc_probe(struct
>> >> platform_device *pdev)
>> >>                                                       " clk failed\n");
>> >>       }
>> >>
>> >> +     if (cpu_is_omap44xx() || cpu_is_omap3630()) {
>> > Can we avoid above by passing this part of platform data??
>> > devices.c
>>
>> I am not clear about the method. The board files export the
>> omap_mmc_platform_data.
>> Does it imply that all board files have to change and export
>> the capability so that it can be queried ?
> No. You don't have to modify the board files. This would need
> change in devices.c which common for all omap boards.
>
> I don't have a strong opinion on this point but just put forth an
> alternate way to avoid such SOC specific check in drivers.
> You can take call on this

Thanks. I will go with Madhu's vote now. With ADMA and other features
coming in, I'll think of a generic way to export the capabilities
from the platform data.

/Venkat.

[PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Venkatraman S]

Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote:
>> -----Original Message-----
>> From: svenkatr at gmail.com [mailto:svenkatr at gmail.com] On Behalf Of Venkatraman S
>> Sent: Wednesday, May 05, 2010 9:49 PM
>> To: Shilimkar, Santosh
>> Cc: linux-omap at vger.kernel.org; linux-mmc at vger.kernel.org; linux-arm-kernel at lists.arm.linux.org.uk;
>> Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
>> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
>>
>> [Long sections have been trimmed to the context of discussion]
>> Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote:
>> >
>> >> -----Original Message-----
>> >> From: svenkatr at gmail.com [mailto:svenkatr at gmail.com] On Behalf Of Venkatraman S
>> >> Sent: Thursday, April 29, 2010 11:05 PM
>> >> To: linux-omap at vger.kernel.org; linux-mmc at vger.kernel.org; linux-arm-kernel at lists.arm.linux.org.uk
>> >> Cc: Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Shilimkar, Santosh; Tony
>> >> Lindgren
>> >> Subject: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
>> >>
>> >> From 1c63dd42fc6c563c931168779ce73401332faa52 Mon Sep 17 00:00:00 2001
>> >> From: Venkatraman S <svenkatr@ti.com>
>> >> Date: Thu, 29 Apr 2010 22:43:31 +0530
>> >> Subject: [PATCH 2/2] omap hsmmc: adaptation of sdma descriptor
>> >> autoloading feature
>> >>
>> >> Start to use the sDMA descriptor autoloading feature.
>> >> For large datablocks, the MMC driver has to repeatedly setup,
>> >> program and teardown the dma channel for each element of the
>> >> sglist received in omap_hsmmc_request.
>> >>
>> >> By using descriptor autoloading, transfers from / to each element of
>> >> the sglist is pre programmed into a linked list. The sDMA driver
>> >> completes the entire transaction and provides a single interrupt.
>> >>
>> >> Due to this, number of dma interrupts for a typical 100MB transfer on the MMC is
>> >> reduced from 25000 to about 400 (approximate). Transfer speeds are
>> >> improved by ~5%.
>> >> (Though it varies on the size of read / write & improves on huge transfers)
>> >>
>> >> Descriptor autoloading is available only in 3630 and 4430 (as of now).
>> >> Hence normal DMA mode is also retained.
>> >>
>> >> Signed-off-by: Venkatraman S <svenkatr@ti.com>
>> >> CC: Adrian Hunter <adrian.hunter@nokia.com>
>> >> CC: Madhusudhan C <madhu.cr@ti.com>
>> >> CC: Shilimkar Santosh <santosh.shilimkar@ti.com>
>> >> CC: Tony Lindgren <tony@atomide.com>
>> >> ---
>> >> ?Changes since previous version
>> >> ? * Rebased to Adrian Hunter's interrupt syncronisation patch
>> >> ? ? ? ? ? ? ?https://patchwork.kernel.org/patch/94670/
>> >> ? * Rename ICR name definition
>> >> ?drivers/mmc/host/omap_hsmmc.c | ?148 ++++++++++++++++++++++++++++++++++------
>> >> ?1 files changed, 125 insertions(+), 23 deletions(-)
>> >>
>> >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> >> index 65093d4..095fd94 100644
>> >> --- a/drivers/mmc/host/omap_hsmmc.c
>> >> +++ b/drivers/mmc/host/omap_hsmmc.c
>> >> @@ -102,6 +102,8 @@
>> >> ?#define SRD ? ? ? ? ? ? ? ? ?(1 << 26)
>> >> ?#define SOFTRESET ? ? ? ? ? ?(1 << 1)
>> >> ?#define RESETDONE ? ? ? ? ? ?(1 << 0)
>> >> +/* End of superblock indicator for sglist transfers */
>> >> +#define DMA_ICR_SGLIST_END ? 0x4D00
>> >>
>> >> ?/*
>> >> ? * FIXME: Most likely all the data using these _DEVID defines should come
>> >> @@ -118,6 +120,12 @@
>> >> ?#define OMAP_MMC_MASTER_CLOCK ? ? ? ?96000000
>> >> ?#define DRIVER_NAME ? ? ? ? ?"mmci-omap-hs"
>> >>
>> >> +#define DMA_TYPE_NODMA ? ? ? 0
>> > " DMA_TYPE_NODMA" doesn't make sense
>> >> +#define DMA_TYPE_SDMA ? ? ? ?1
>> >> +#define DMA_TYPE_SDMA_DLOAD 2
>> >
>> > How about ??
>> > #define TRANSFER_NODMA ? ? ? ? ? ? ? ? ?0
>> > #define TRANSFER_SDMA_NORMAL ? ? ? ? ? ?1
>> > #define TRANSFER_SDMA_DESCRIPTOR ? ? ? ?2
>> > I think ADMA is also in pipeline, so it can become then
>> > #define TRANSFER_ADMA_DESCRIPTOR ? ? ? ?3
>>
>> Yes ?I was planning to use 3 for ADMA, but the names don't
>> make a big difference.
>>
> Good. Name does makes the difference when somebody reads the code.
> " DMA_TYPE_NODMA" what you make out from this if somebody has no
> background of what patch is doing.

How about DMA_TYPE_NONE ?

>
>> >> +
>> >> +#define DMA_CTRL_BUF_SIZE ? ?(PAGE_SIZE * 3)
>> >> +
>> >> ?/* Timeouts for entering power saving states on inactivity, msec */
>> >> ?#define OMAP_MMC_DISABLED_TIMEOUT ? ?100
>> >> ?#define OMAP_MMC_SLEEP_TIMEOUT ? ? ? ? ? ? ? 1000
>> >> @@ -170,7 +178,11 @@ struct omap_hsmmc_host {
>> >> ? ? ? u32 ? ? ? ? ? ? ? ? ? ? bytesleft;
>> >> ? ? ? int ? ? ? ? ? ? ? ? ? ? suspended;
>> >> ? ? ? int ? ? ? ? ? ? ? ? ? ? irq;
>> >> - ? ? int ? ? ? ? ? ? ? ? ? ? use_dma, dma_ch;
>> >> + ? ? int ? ? ? ? ? ? ? ? ? ? dma_caps;
>> >> + ? ? int ? ? ? ? ? ? ? ? ? ? dma_in_use;
>> > This flag isn't necessary. What you are doing is
>> > just renaming it. Can't you avoid that ??
>> > Inudertand the intent behind "dma_in_use " instead
>> > of "use_dma", but you can surely avoid this change.
>>
>> Actually there is a shift in the meaning of these variables
>> so I believe the change is necessary.
>> With my changes, the type of DMA to use (SDMA, SDMA_DESC_LOAD)
>> [and in the future NODMA, ADMA] can vary on a per transfer basis.
>> Previously use_dma was a static capability variable of whether DMA
>> was available. That has been move to dma_caps. The dma_in_use,
>> as the name more intuitively suggests, keeps track of the type of transfer
>> used in the current transaction.
>>
> I still feel it's no necessary considering the no. of lines gets changed because
> of this.
> You take a call. I am fine with it.
>>
>> >> ?{
>> >> ? ? ? unsigned int irq_mask;
>> >>
>> >> - ? ? if (host->use_dma)
>> >> + ? ? if (host->dma_in_use)
>> > This will go with no flag change.
>>
>> Explanation as above
>>
>> >> ? ? ? ? ? ? ? irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
>> >> ? ? ? else
>> >> ? ? ? ? ? ? ? irq_mask = INT_EN_MASK;
>> >> @@ -813,7 +825,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host
>> >> *host, struct mmc_command *cmd,
>> >> ? ? ? ? ? ? ? ? ? ? ? cmdreg &= ~(DDIR);
>> >> ? ? ? }
>> >>
>> >> - ? ? if (host->use_dma)
>> >> + ? ? if (host->dma_in_use)
>> > ditto
>> >> ? ? ? ? ? ? ? cmdreg |= DMA_EN;
>> >>
>> >> ? ? ? host->req_in_progress = 1;
> <snip ..>
>
>
>> >> +static int omap_hsmmc_configure_sdma_sglist(struct omap_hsmmc_host *host,
>> >> + ? ? ? ? ? ? struct mmc_request *req)
>> >> +{
>> >> + ? ? int i;
>> >> + ? ? struct omap_dma_sglist_node *sglist, *snode;
>> >> + ? ? struct mmc_data *data = req->data;
>> >> + ? ? int blksz;
>> >> + ? ? int dmadir = omap_hsmmc_get_dma_dir(host, data);
>> >> + ? ? struct omap_dma_sglist_type2a_params *t2p;
>> >> +
>> >> + ? ? sglist = (struct omap_dma_sglist_node *) host->dma_ctrl_buf;
>> >> + ? ? snode = sglist;
>> >> + ? ? blksz = host->data->blksz;
>> >> +
>> >> + ? ? if ((host->dma_len * sizeof(*snode)) > DMA_CTRL_BUF_SIZE) {
>> >> + ? ? ? ? ? ? dev_err(mmc_dev(host->mmc), "not enough sglist memory %d\n",
>> >> + ? ? ? ? ? ? ? ? ? ? host->dma_len);
>> >> + ? ? ? ? ? ? return -ENOMEM;
>> >> + ? ? }
>> >> + ? ? for (i = 0; i < host->dma_len; snode++, i++) {
>> >> + ? ? ? ? ? ? snode->desc_type = OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a;
>> >> + ? ? ? ? ? ? snode->num_of_elem = blksz / 4;
>> >> + ? ? ? ? ? ? t2p = &snode->sg_node.t2a;
>> >> +
>> >> + ? ? ? ? ? ? if (dmadir == DMA_FROM_DEVICE) {
>> >> + ? ? ? ? ? ? ? ? ? ? t2p->src_addr = host->mapbase + OMAP_HSMMC_DATA;
>> >> + ? ? ? ? ? ? ? ? ? ? t2p->dst_addr = sg_dma_address(data->sg + i);
>> >> + ? ? ? ? ? ? } else {
>> >> + ? ? ? ? ? ? ? ? ? ? t2p->dst_addr = host->mapbase + OMAP_HSMMC_DATA;
>> >> + ? ? ? ? ? ? ? ? ? ? t2p->src_addr = sg_dma_address(data->sg + i);
>> >> + ? ? ? ? ? ? }
>> >> + ? ? ? ? ? ? snode->flags =
>> >> + ? ? ? ? ? ? ? ? ? ? OMAP_DMA_LIST_DST_VALID | OMAP_DMA_LIST_SRC_VALID;
>> >> +
>> >> + ? ? ? ? ? ? t2p->cfn_fn = sg_dma_len(data->sg + i) / host->data->blksz;
>> >> + ? ? ? ? ? ? t2p->cicr = DMA_ICR_SGLIST_END;
>> >> +
>> >> + ? ? ? ? ? ? t2p->dst_frame_idx_or_pkt_size = 0;
>> >> + ? ? ? ? ? ? t2p->src_frame_idx_or_pkt_size = 0;
>> >> + ? ? ? ? ? ? t2p->dst_elem_idx = 0;
>> >> + ? ? ? ? ? ? t2p->src_elem_idx = 0;
>> >> + ? ? }
>> >> + ? ? dev_dbg(mmc_dev(host->mmc), "new sglist %x len =%d\n",
>> >> + ? ? ? ? ? ? ? ? ? ? host->dma_ctrl_buf_phy, i);
>> >> + ? ? omap_set_dma_sglist_mode(host->dma_ch, sglist,
>> >> + ? ? ? ? ? ? ? ? ? ? host->dma_ctrl_buf_phy, i, NULL);
>> >> + ? ? omap_dma_set_sglist_fastmode(host->dma_ch, 1);
>> >
>> > You should check for return value of above two. If any one of above fails
>> > the transfer will fail BADLY
>>
>> These functions have no return code (Similar to omap_start_dma, which doesn't
>> have return code either)
>
> Are you sure ? Here is the function body.
>
> +int omap_set_dma_sglist_mode(int lch, struct omap_dma_sglist_node *sgparams,
> + ? ? ? dma_addr_t padd, int nelem, struct omap_dma_channel_params *chparams)
> +{
> + ? ? ? struct omap_dma_list_config_params *lcfg;
> + ? ? ? int l = DMA_LIST_CDP_LISTMODE; /* Enable Linked list mode in CDP */
> +
> + ? ? ? if ((dma_caps0_status & DMA_CAPS_SGLIST_SUPPORT) == 0) {
> + ? ? ? ? ? ? ? printk(KERN_ERR "omap DMA: sglist feature not supported\n");
> + ? ? ? ? ? ? ? return -EPERM;
> + ? ? ? }
> + ? ? ? if (dma_chan[lch].flags & OMAP_DMA_ACTIVE) {
> + ? ? ? ? ? ? ? printk(KERN_ERR "omap DMA: configuring active DMA channel\n");
> + ? ? ? ? ? ? ? return -EPERM;
> + ? ? ? }
> +
> + ? ? ? if (padd == 0) {
> + ? ? ? ? ? ? ? printk(KERN_ERR "omap DMA: sglist invalid dma_addr\n");
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> + ? ? ? lcfg = &dma_chan[lch].list_config;
> +
> + ? ? ? lcfg->sghead = sgparams;
> + ? ? ? lcfg->num_elem = nelem;
> + ? ? ? lcfg->sgheadphy = padd;
> + ? ? ? lcfg->pausenode = -1;
> +
> +
> + ? ? ? if (NULL == chparams)
> + ? ? ? ? ? ? ? l |= DMA_LIST_CDP_FASTMODE;
> + ? ? ? else
> + ? ? ? ? ? ? ? omap_set_dma_params(lch, chparams);
> +
> + ? ? ? dma_write(l, CDP(lch));
> + ? ? ? dma_write(0, CCDN(lch)); /* Reset List index numbering */
> + ? ? ? /* Initialize frame and element counters to invalid values */
> + ? ? ? dma_write(OMAP_DMA_INVALID_FRAME_COUNT, CCFN(lch));
> + ? ? ? dma_write(OMAP_DMA_INVALID_ELEM_COUNT, CCEN(lch));
> +
> + ? ? ? return dma_sglist_set_phy_params(sgparams, lcfg->sgheadphy,
> + ? ? ? ? ? ? ? ? ? ? ? nelem, dma_read(CICR(lch)));
> +
> +}
>
 OK. I missed that.
>
>>
>> >> + ? ? return 0;
>> >> +}
>> >> +
>> >> ?static void set_data_timeout(struct omap_hsmmc_host *host,
>> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned int timeout_ns,
>> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned int timeout_clks)
>> >> @@ -1420,14 +1492,23 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host
>> >> *host, struct mmc_request *req)
>> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | (req->data->blocks << 16));
>> >> ? ? ? set_data_timeout(host, req->data->timeout_ns, req->data->timeout_clks);
>> >>
>> >> - ? ? if (host->use_dma) {
>> >> - ? ? ? ? ? ? ret = omap_hsmmc_start_dma_transfer(host, req);
>> >> - ? ? ? ? ? ? if (ret != 0) {
>> >> - ? ? ? ? ? ? ? ? ? ? dev_dbg(mmc_dev(host->mmc), "MMC start dma failure\n");
>> >> + ? ? if (host->dma_caps & DMA_TYPE_SDMA) {
>> >> + ? ? ? ? ? ? ret = omap_hsmmc_configure_sdma(host, req);
>> >> + ? ? ? ? ? ? if (ret)
>> >> ? ? ? ? ? ? ? ? ? ? ? return ret;
>> >> - ? ? ? ? ? ? }
>> >> + ? ? ? ? ? ? host->dma_in_use = DMA_TYPE_SDMA;
>> >> ? ? ? }
>> >> - ? ? return 0;
>> >> + ? ? if ((host->dma_caps & DMA_TYPE_SDMA_DLOAD) &&
>> >> + ? ? ? ? ? ? host->data->sg_len > 4) {
>> >> + ? ? ? ? ? ? ret = omap_hsmmc_configure_sdma_sglist(host, req);
>> >> + ? ? ? ? ? ? if (ret)
>> >> + ? ? ? ? ? ? ? ? ? ? return ret;
>> >> + ? ? ? ? ? ? host->dma_in_use = DMA_TYPE_SDMA_DLOAD;
>> >> +
>> >> + ? ? }
>> >> + ? ? ret = omap_hsmmc_start_dma_transfer(host);
>> >> + ? ? return ret;
>> >> +
>> >> ?}
>> >>
>> >> ?/*
>> >> @@ -2007,7 +2088,9 @@ static int __init omap_hsmmc_probe(struct
>> >> platform_device *pdev)
>> >> ? ? ? host->mmc ? ? ? = mmc;
>> >> ? ? ? host->pdata ? ? = pdata;
>> >> ? ? ? host->dev ? ? ? = &pdev->dev;
>> >> - ? ? host->use_dma ? = 1;
>> >> + ? ? host->dma_caps ?= DMA_TYPE_SDMA;
>> >> + ? ? host->dma_in_use ? ? ? ?= DMA_TYPE_NODMA;
>> >> + ? ? host->dma_ctrl_buf = NULL;
>> >> ? ? ? host->dev->dma_mask = &pdata->dma_mask;
>> >> ? ? ? host->dma_ch ? ?= -1;
>> >> ? ? ? host->irq ? ? ? = irq;
>> >> @@ -2088,6 +2171,15 @@ static int __init omap_hsmmc_probe(struct
>> >> platform_device *pdev)
>> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? " clk failed\n");
>> >> ? ? ? }
>> >>
>> >> + ? ? if (cpu_is_omap44xx() || cpu_is_omap3630()) {
>> > Can we avoid above by passing this part of platform data??
>> > devices.c
>>
>> I am not clear about the method. The board files export the
>> omap_mmc_platform_data.
>> Does it imply that all board files have to change and export
>> the capability so that it can be queried ?
> No. You don't have to modify the board files. This would need
> change in devices.c which common for all omap boards.
>
> I don't have a strong opinion on this point but just put forth an
> alternate way to avoid such SOC specific check in drivers.
> You can take call on this

Thanks. I will go with Madhu's vote now. With ADMA and other features
coming in, I'll think of a generic way to export the capabilities
from the platform data.

/Venkat.

Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[kishore kadiyala]

<<snip>>

>> I am not clear about the method. The board files export the
>> omap_mmc_platform_data.
>> Does it imply that all board files have to change and export
>> the capability so that it can be queried ?
> No. You don't have to modify the board files. This would need
> change in devices.c which common for all omap boards.
>
> I don't have a strong opinion on this point but just put forth an
> alternate way to avoid such SOC specific check in drivers.
> You can take call on this

Agree. How about adding a flag in hsmmc.h & omap_mmc_platform_data,
that would take care of SDMA & SDMA_DLAOD in the driver instead  going
with SOC check .

RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Shilimkar, Santosh]

> -----Original Message-----
> From: svenkatr@gmail.com [mailto:svenkatr@gmail.com] On Behalf Of S, Venkatraman
> Sent: Thursday, May 06, 2010 2:27 PM
> To: Shilimkar, Santosh
> Cc: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; Chikkature Rajashekar, Madhusudhan; Adrian
> Hunter; Kadiyala, Kishore; Tony Lindgren; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
>
> Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote:
> >> -----Original Message-----
> >> From: svenkatr@gmail.com [mailto:svenkatr@gmail.com] On Behalf Of Venkatraman S
> >> Sent: Wednesday, May 05, 2010 9:49 PM
> >> To: Shilimkar, Santosh
> >> Cc: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-
> kernel@lists.arm.linux.org.uk;
> >> Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
> >> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
> >>
> >> [Long sections have been trimmed to the context of discussion]
> >> Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote:
> >> >
> >> >> -----Original Message-----
> >> >> From: svenkatr@gmail.com [mailto:svenkatr@gmail.com] On Behalf Of Venkatraman S
> >> >> Sent: Thursday, April 29, 2010 11:05 PM
> >> >> To: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-
> kernel@lists.arm.linux.org.uk
> >> >> Cc: Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Shilimkar, Santosh;
> Tony
> >> >> Lindgren
> >> >> Subject: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
> >> >>
> >> >> From 1c63dd42fc6c563c931168779ce73401332faa52 Mon Sep 17 00:00:00 2001
> >> >> From: Venkatraman S <svenkatr@ti.com>
> >> >> Date: Thu, 29 Apr 2010 22:43:31 +0530
> >> >> Subject: [PATCH 2/2] omap hsmmc: adaptation of sdma descriptor
> >> >> autoloading feature
> >> >>
> >> >> Start to use the sDMA descriptor autoloading feature.
> >> >> For large datablocks, the MMC driver has to repeatedly setup,
> >> >> program and teardown the dma channel for each element of the
> >> >> sglist received in omap_hsmmc_request.
> >> >>
> >> >> By using descriptor autoloading, transfers from / to each element of
> >> >> the sglist is pre programmed into a linked list. The sDMA driver
> >> >> completes the entire transaction and provides a single interrupt.
> >> >>
> >> >> Due to this, number of dma interrupts for a typical 100MB transfer on the MMC is
> >> >> reduced from 25000 to about 400 (approximate). Transfer speeds are
> >> >> improved by ~5%.
> >> >> (Though it varies on the size of read / write & improves on huge transfers)
> >> >>
> >> >> Descriptor autoloading is available only in 3630 and 4430 (as of now).
> >> >> Hence normal DMA mode is also retained.
> >> >>
> >> >> Signed-off-by: Venkatraman S <svenkatr@ti.com>
> >> >> CC: Adrian Hunter <adrian.hunter@nokia.com>
> >> >> CC: Madhusudhan C <madhu.cr@ti.com>
> >> >> CC: Shilimkar Santosh <santosh.shilimkar@ti.com>
> >> >> CC: Tony Lindgren <tony@atomide.com>
> >> >> ---
> >> >>  Changes since previous version
> >> >>   * Rebased to Adrian Hunter's interrupt syncronisation patch
> >> >>              https://patchwork.kernel.org/patch/94670/
> >> >>   * Rename ICR name definition
> >> >>  drivers/mmc/host/omap_hsmmc.c |  148 ++++++++++++++++++++++++++++++++++------
> >> >>  1 files changed, 125 insertions(+), 23 deletions(-)
> >> >>
> >> >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> >> >> index 65093d4..095fd94 100644
> >> >> --- a/drivers/mmc/host/omap_hsmmc.c
> >> >> +++ b/drivers/mmc/host/omap_hsmmc.c
> >> >> @@ -102,6 +102,8 @@
> >> >>  #define SRD                  (1 << 26)
> >> >>  #define SOFTRESET            (1 << 1)
> >> >>  #define RESETDONE            (1 << 0)
> >> >> +/* End of superblock indicator for sglist transfers */
> >> >> +#define DMA_ICR_SGLIST_END   0x4D00
> >> >>
> >> >>  /*
> >> >>   * FIXME: Most likely all the data using these _DEVID defines should come
> >> >> @@ -118,6 +120,12 @@
> >> >>  #define OMAP_MMC_MASTER_CLOCK        96000000
> >> >>  #define DRIVER_NAME          "mmci-omap-hs"
> >> >>
> >> >> +#define DMA_TYPE_NODMA       0
> >> > " DMA_TYPE_NODMA" doesn't make sense
> >> >> +#define DMA_TYPE_SDMA        1
> >> >> +#define DMA_TYPE_SDMA_DLOAD 2
> >> >
> >> > How about ??
> >> > #define TRANSFER_NODMA                  0
> >> > #define TRANSFER_SDMA_NORMAL            1
> >> > #define TRANSFER_SDMA_DESCRIPTOR        2
> >> > I think ADMA is also in pipeline, so it can become then
> >> > #define TRANSFER_ADMA_DESCRIPTOR        3
> >>
> >> Yes  I was planning to use 3 for ADMA, but the names don't
> >> make a big difference.
> >>
> > Good. Name does makes the difference when somebody reads the code.
> > " DMA_TYPE_NODMA" what you make out from this if somebody has no
> > background of what patch is doing.
>
> How about DMA_TYPE_NONE ?
>
Better
> >
> >> >> +
> >> >> +#define DMA_CTRL_BUF_SIZE    (PAGE_SIZE * 3)
> >> >> +
> >> >>  /* Timeouts for entering power saving states on inactivity, msec */
> >> >>  #define OMAP_MMC_DISABLED_TIMEOUT    100
> >> >>  #define OMAP_MMC_SLEEP_TIMEOUT               1000
> >> >> @@ -170,7 +178,11 @@ struct omap_hsmmc_host {
> >> >>       u32                     bytesleft;
> >> >>       int                     suspended;
> >> >>       int                     irq;
> >> >> -     int                     use_dma, dma_ch;
> >> >> +     int                     dma_caps;
> >> >> +     int                     dma_in_use;
> >> > This flag isn't necessary. What you are doing is
> >> > just renaming it. Can't you avoid that ??
> >> > Inudertand the intent behind "dma_in_use " instead
> >> > of "use_dma", but you can surely avoid this change.
> >>
> >> Actually there is a shift in the meaning of these variables
> >> so I believe the change is necessary.
> >> With my changes, the type of DMA to use (SDMA, SDMA_DESC_LOAD)
> >> [and in the future NODMA, ADMA] can vary on a per transfer basis.
> >> Previously use_dma was a static capability variable of whether DMA
> >> was available. That has been move to dma_caps. The dma_in_use,
> >> as the name more intuitively suggests, keeps track of the type of transfer
> >> used in the current transaction.
> >>
> > I still feel it's no necessary considering the no. of lines gets changed because
> > of this.
> > You take a call. I am fine with it.
> >>
> >> >>  {
> >> >>       unsigned int irq_mask;
> >> >>
> >> >> -     if (host->use_dma)
> >> >> +     if (host->dma_in_use)
> >> > This will go with no flag change.
> >>
> >> Explanation as above
> >>
> >> >>               irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
> >> >>       else
> >> >>               irq_mask = INT_EN_MASK;
> >> >> @@ -813,7 +825,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host
> >> >> *host, struct mmc_command *cmd,
> >> >>                       cmdreg &= ~(DDIR);
> >> >>       }
> >> >>
> >> >> -     if (host->use_dma)
> >> >> +     if (host->dma_in_use)
> >> > ditto
> >> >>               cmdreg |= DMA_EN;
> >> >>
> >> >>       host->req_in_progress = 1;
> > <snip ..>
> >
> >
> >> >> +static int omap_hsmmc_configure_sdma_sglist(struct omap_hsmmc_host *host,
> >> >> +             struct mmc_request *req)
> >> >> +{
> >> >> +     int i;
> >> >> +     struct omap_dma_sglist_node *sglist, *snode;
> >> >> +     struct mmc_data *data = req->data;
> >> >> +     int blksz;
> >> >> +     int dmadir = omap_hsmmc_get_dma_dir(host, data);
> >> >> +     struct omap_dma_sglist_type2a_params *t2p;
> >> >> +
> >> >> +     sglist = (struct omap_dma_sglist_node *) host->dma_ctrl_buf;
> >> >> +     snode = sglist;
> >> >> +     blksz = host->data->blksz;
> >> >> +
> >> >> +     if ((host->dma_len * sizeof(*snode)) > DMA_CTRL_BUF_SIZE) {
> >> >> +             dev_err(mmc_dev(host->mmc), "not enough sglist memory %d\n",
> >> >> +                     host->dma_len);
> >> >> +             return -ENOMEM;
> >> >> +     }
> >> >> +     for (i = 0; i < host->dma_len; snode++, i++) {
> >> >> +             snode->desc_type = OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a;
> >> >> +             snode->num_of_elem = blksz / 4;
> >> >> +             t2p = &snode->sg_node.t2a;
> >> >> +
> >> >> +             if (dmadir == DMA_FROM_DEVICE) {
> >> >> +                     t2p->src_addr = host->mapbase + OMAP_HSMMC_DATA;
> >> >> +                     t2p->dst_addr = sg_dma_address(data->sg + i);
> >> >> +             } else {
> >> >> +                     t2p->dst_addr = host->mapbase + OMAP_HSMMC_DATA;
> >> >> +                     t2p->src_addr = sg_dma_address(data->sg + i);
> >> >> +             }
> >> >> +             snode->flags =
> >> >> +                     OMAP_DMA_LIST_DST_VALID | OMAP_DMA_LIST_SRC_VALID;
> >> >> +
> >> >> +             t2p->cfn_fn = sg_dma_len(data->sg + i) / host->data->blksz;
> >> >> +             t2p->cicr = DMA_ICR_SGLIST_END;
> >> >> +
> >> >> +             t2p->dst_frame_idx_or_pkt_size = 0;
> >> >> +             t2p->src_frame_idx_or_pkt_size = 0;
> >> >> +             t2p->dst_elem_idx = 0;
> >> >> +             t2p->src_elem_idx = 0;
> >> >> +     }
> >> >> +     dev_dbg(mmc_dev(host->mmc), "new sglist %x len =%d\n",
> >> >> +                     host->dma_ctrl_buf_phy, i);
> >> >> +     omap_set_dma_sglist_mode(host->dma_ch, sglist,
> >> >> +                     host->dma_ctrl_buf_phy, i, NULL);
> >> >> +     omap_dma_set_sglist_fastmode(host->dma_ch, 1);
> >> >
> >> > You should check for return value of above two. If any one of above fails
> >> > the transfer will fail BADLY
> >>
> >> These functions have no return code (Similar to omap_start_dma, which doesn't
> >> have return code either)
> >
> > Are you sure ? Here is the function body.
> >
> > +int omap_set_dma_sglist_mode(int lch, struct omap_dma_sglist_node *sgparams,
> > +       dma_addr_t padd, int nelem, struct omap_dma_channel_params *chparams)
> > +{
> > +       struct omap_dma_list_config_params *lcfg;
> > +       int l = DMA_LIST_CDP_LISTMODE; /* Enable Linked list mode in CDP */
> > +
> > +       if ((dma_caps0_status & DMA_CAPS_SGLIST_SUPPORT) == 0) {
> > +               printk(KERN_ERR "omap DMA: sglist feature not supported\n");
> > +               return -EPERM;
> > +       }
> > +       if (dma_chan[lch].flags & OMAP_DMA_ACTIVE) {
> > +               printk(KERN_ERR "omap DMA: configuring active DMA channel\n");
> > +               return -EPERM;
> > +       }
> > +
> > +       if (padd == 0) {
> > +               printk(KERN_ERR "omap DMA: sglist invalid dma_addr\n");
> > +               return -EINVAL;
> > +       }
> > +       lcfg = &dma_chan[lch].list_config;
> > +
> > +       lcfg->sghead = sgparams;
> > +       lcfg->num_elem = nelem;
> > +       lcfg->sgheadphy = padd;
> > +       lcfg->pausenode = -1;
> > +
> > +
> > +       if (NULL == chparams)
> > +               l |= DMA_LIST_CDP_FASTMODE;
> > +       else
> > +               omap_set_dma_params(lch, chparams);
> > +
> > +       dma_write(l, CDP(lch));
> > +       dma_write(0, CCDN(lch)); /* Reset List index numbering */
> > +       /* Initialize frame and element counters to invalid values */
> > +       dma_write(OMAP_DMA_INVALID_FRAME_COUNT, CCFN(lch));
> > +       dma_write(OMAP_DMA_INVALID_ELEM_COUNT, CCEN(lch));
> > +
> > +       return dma_sglist_set_phy_params(sgparams, lcfg->sgheadphy,
> > +                       nelem, dma_read(CICR(lch)));
> > +
> > +}
> >
>  OK. I missed that.
> >
> >>
> >> >> +     return 0;
> >> >> +}
> >> >> +
> >> >>  static void set_data_timeout(struct omap_hsmmc_host *host,
> >> >>                            unsigned int timeout_ns,
> >> >>                            unsigned int timeout_clks)
> >> >> @@ -1420,14 +1492,23 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host
> >> >> *host, struct mmc_request *req)
> >> >>                                       | (req->data->blocks << 16));
> >> >>       set_data_timeout(host, req->data->timeout_ns, req->data->timeout_clks);
> >> >>
> >> >> -     if (host->use_dma) {
> >> >> -             ret = omap_hsmmc_start_dma_transfer(host, req);
> >> >> -             if (ret != 0) {
> >> >> -                     dev_dbg(mmc_dev(host->mmc), "MMC start dma failure\n");
> >> >> +     if (host->dma_caps & DMA_TYPE_SDMA) {
> >> >> +             ret = omap_hsmmc_configure_sdma(host, req);
> >> >> +             if (ret)
> >> >>                       return ret;
> >> >> -             }
> >> >> +             host->dma_in_use = DMA_TYPE_SDMA;
> >> >>       }
> >> >> -     return 0;
> >> >> +     if ((host->dma_caps & DMA_TYPE_SDMA_DLOAD) &&
> >> >> +             host->data->sg_len > 4) {
> >> >> +             ret = omap_hsmmc_configure_sdma_sglist(host, req);
> >> >> +             if (ret)
> >> >> +                     return ret;
> >> >> +             host->dma_in_use = DMA_TYPE_SDMA_DLOAD;
> >> >> +
> >> >> +     }
> >> >> +     ret = omap_hsmmc_start_dma_transfer(host);
> >> >> +     return ret;
> >> >> +
> >> >>  }
> >> >>
> >> >>  /*
> >> >> @@ -2007,7 +2088,9 @@ static int __init omap_hsmmc_probe(struct
> >> >> platform_device *pdev)
> >> >>       host->mmc       = mmc;
> >> >>       host->pdata     = pdata;
> >> >>       host->dev       = &pdev->dev;
> >> >> -     host->use_dma   = 1;
> >> >> +     host->dma_caps  = DMA_TYPE_SDMA;
> >> >> +     host->dma_in_use        = DMA_TYPE_NODMA;
> >> >> +     host->dma_ctrl_buf = NULL;
> >> >>       host->dev->dma_mask = &pdata->dma_mask;
> >> >>       host->dma_ch    = -1;
> >> >>       host->irq       = irq;
> >> >> @@ -2088,6 +2171,15 @@ static int __init omap_hsmmc_probe(struct
> >> >> platform_device *pdev)
> >> >>                                                       " clk failed\n");
> >> >>       }
> >> >>
> >> >> +     if (cpu_is_omap44xx() || cpu_is_omap3630()) {
> >> > Can we avoid above by passing this part of platform data??
> >> > devices.c
> >>
> >> I am not clear about the method. The board files export the
> >> omap_mmc_platform_data.
> >> Does it imply that all board files have to change and export
> >> the capability so that it can be queried ?
> > No. You don't have to modify the board files. This would need
> > change in devices.c which common for all omap boards.
> >
> > I don't have a strong opinion on this point but just put forth an
> > alternate way to avoid such SOC specific check in drivers.
> > You can take call on this
>
> Thanks. I will go with Madhu's vote now. With ADMA and other features
> coming in, I'll think of a generic way to export the capabilities
> from the platform data.
>
One of the problem with such approaches is ones the code get merged it's not
revisited from improvements.
If Tony is ok with this I am fine too.

Regards,
Santosh

.

[PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Shilimkar, Santosh]

> -----Original Message-----
> From: svenkatr at gmail.com [mailto:svenkatr at gmail.com] On Behalf Of S, Venkatraman
> Sent: Thursday, May 06, 2010 2:27 PM
> To: Shilimkar, Santosh
> Cc: linux-omap at vger.kernel.org; linux-mmc at vger.kernel.org; Chikkature Rajashekar, Madhusudhan; Adrian
> Hunter; Kadiyala, Kishore; Tony Lindgren; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
>
> Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote:
> >> -----Original Message-----
> >> From: svenkatr at gmail.com [mailto:svenkatr at gmail.com] On Behalf Of Venkatraman S
> >> Sent: Wednesday, May 05, 2010 9:49 PM
> >> To: Shilimkar, Santosh
> >> Cc: linux-omap at vger.kernel.org; linux-mmc at vger.kernel.org; linux-arm-
> kernel at lists.arm.linux.org.uk;
> >> Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
> >> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
> >>
> >> [Long sections have been trimmed to the context of discussion]
> >> Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote:
> >> >
> >> >> -----Original Message-----
> >> >> From: svenkatr at gmail.com [mailto:svenkatr at gmail.com] On Behalf Of Venkatraman S
> >> >> Sent: Thursday, April 29, 2010 11:05 PM
> >> >> To: linux-omap at vger.kernel.org; linux-mmc at vger.kernel.org; linux-arm-
> kernel at lists.arm.linux.org.uk
> >> >> Cc: Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Shilimkar, Santosh;
> Tony
> >> >> Lindgren
> >> >> Subject: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
> >> >>
> >> >> From 1c63dd42fc6c563c931168779ce73401332faa52 Mon Sep 17 00:00:00 2001
> >> >> From: Venkatraman S <svenkatr@ti.com>
> >> >> Date: Thu, 29 Apr 2010 22:43:31 +0530
> >> >> Subject: [PATCH 2/2] omap hsmmc: adaptation of sdma descriptor
> >> >> autoloading feature
> >> >>
> >> >> Start to use the sDMA descriptor autoloading feature.
> >> >> For large datablocks, the MMC driver has to repeatedly setup,
> >> >> program and teardown the dma channel for each element of the
> >> >> sglist received in omap_hsmmc_request.
> >> >>
> >> >> By using descriptor autoloading, transfers from / to each element of
> >> >> the sglist is pre programmed into a linked list. The sDMA driver
> >> >> completes the entire transaction and provides a single interrupt.
> >> >>
> >> >> Due to this, number of dma interrupts for a typical 100MB transfer on the MMC is
> >> >> reduced from 25000 to about 400 (approximate). Transfer speeds are
> >> >> improved by ~5%.
> >> >> (Though it varies on the size of read / write & improves on huge transfers)
> >> >>
> >> >> Descriptor autoloading is available only in 3630 and 4430 (as of now).
> >> >> Hence normal DMA mode is also retained.
> >> >>
> >> >> Signed-off-by: Venkatraman S <svenkatr@ti.com>
> >> >> CC: Adrian Hunter <adrian.hunter@nokia.com>
> >> >> CC: Madhusudhan C <madhu.cr@ti.com>
> >> >> CC: Shilimkar Santosh <santosh.shilimkar@ti.com>
> >> >> CC: Tony Lindgren <tony@atomide.com>
> >> >> ---
> >> >>  Changes since previous version
> >> >>   * Rebased to Adrian Hunter's interrupt syncronisation patch
> >> >>              https://patchwork.kernel.org/patch/94670/
> >> >>   * Rename ICR name definition
> >> >>  drivers/mmc/host/omap_hsmmc.c |  148 ++++++++++++++++++++++++++++++++++------
> >> >>  1 files changed, 125 insertions(+), 23 deletions(-)
> >> >>
> >> >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> >> >> index 65093d4..095fd94 100644
> >> >> --- a/drivers/mmc/host/omap_hsmmc.c
> >> >> +++ b/drivers/mmc/host/omap_hsmmc.c
> >> >> @@ -102,6 +102,8 @@
> >> >>  #define SRD                  (1 << 26)
> >> >>  #define SOFTRESET            (1 << 1)
> >> >>  #define RESETDONE            (1 << 0)
> >> >> +/* End of superblock indicator for sglist transfers */
> >> >> +#define DMA_ICR_SGLIST_END   0x4D00
> >> >>
> >> >>  /*
> >> >>   * FIXME: Most likely all the data using these _DEVID defines should come
> >> >> @@ -118,6 +120,12 @@
> >> >>  #define OMAP_MMC_MASTER_CLOCK        96000000
> >> >>  #define DRIVER_NAME          "mmci-omap-hs"
> >> >>
> >> >> +#define DMA_TYPE_NODMA       0
> >> > " DMA_TYPE_NODMA" doesn't make sense
> >> >> +#define DMA_TYPE_SDMA        1
> >> >> +#define DMA_TYPE_SDMA_DLOAD 2
> >> >
> >> > How about ??
> >> > #define TRANSFER_NODMA                  0
> >> > #define TRANSFER_SDMA_NORMAL            1
> >> > #define TRANSFER_SDMA_DESCRIPTOR        2
> >> > I think ADMA is also in pipeline, so it can become then
> >> > #define TRANSFER_ADMA_DESCRIPTOR        3
> >>
> >> Yes  I was planning to use 3 for ADMA, but the names don't
> >> make a big difference.
> >>
> > Good. Name does makes the difference when somebody reads the code.
> > " DMA_TYPE_NODMA" what you make out from this if somebody has no
> > background of what patch is doing.
>
> How about DMA_TYPE_NONE ?
>
Better
> >
> >> >> +
> >> >> +#define DMA_CTRL_BUF_SIZE    (PAGE_SIZE * 3)
> >> >> +
> >> >>  /* Timeouts for entering power saving states on inactivity, msec */
> >> >>  #define OMAP_MMC_DISABLED_TIMEOUT    100
> >> >>  #define OMAP_MMC_SLEEP_TIMEOUT               1000
> >> >> @@ -170,7 +178,11 @@ struct omap_hsmmc_host {
> >> >>       u32                     bytesleft;
> >> >>       int                     suspended;
> >> >>       int                     irq;
> >> >> -     int                     use_dma, dma_ch;
> >> >> +     int                     dma_caps;
> >> >> +     int                     dma_in_use;
> >> > This flag isn't necessary. What you are doing is
> >> > just renaming it. Can't you avoid that ??
> >> > Inudertand the intent behind "dma_in_use " instead
> >> > of "use_dma", but you can surely avoid this change.
> >>
> >> Actually there is a shift in the meaning of these variables
> >> so I believe the change is necessary.
> >> With my changes, the type of DMA to use (SDMA, SDMA_DESC_LOAD)
> >> [and in the future NODMA, ADMA] can vary on a per transfer basis.
> >> Previously use_dma was a static capability variable of whether DMA
> >> was available. That has been move to dma_caps. The dma_in_use,
> >> as the name more intuitively suggests, keeps track of the type of transfer
> >> used in the current transaction.
> >>
> > I still feel it's no necessary considering the no. of lines gets changed because
> > of this.
> > You take a call. I am fine with it.
> >>
> >> >>  {
> >> >>       unsigned int irq_mask;
> >> >>
> >> >> -     if (host->use_dma)
> >> >> +     if (host->dma_in_use)
> >> > This will go with no flag change.
> >>
> >> Explanation as above
> >>
> >> >>               irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
> >> >>       else
> >> >>               irq_mask = INT_EN_MASK;
> >> >> @@ -813,7 +825,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host
> >> >> *host, struct mmc_command *cmd,
> >> >>                       cmdreg &= ~(DDIR);
> >> >>       }
> >> >>
> >> >> -     if (host->use_dma)
> >> >> +     if (host->dma_in_use)
> >> > ditto
> >> >>               cmdreg |= DMA_EN;
> >> >>
> >> >>       host->req_in_progress = 1;
> > <snip ..>
> >
> >
> >> >> +static int omap_hsmmc_configure_sdma_sglist(struct omap_hsmmc_host *host,
> >> >> +             struct mmc_request *req)
> >> >> +{
> >> >> +     int i;
> >> >> +     struct omap_dma_sglist_node *sglist, *snode;
> >> >> +     struct mmc_data *data = req->data;
> >> >> +     int blksz;
> >> >> +     int dmadir = omap_hsmmc_get_dma_dir(host, data);
> >> >> +     struct omap_dma_sglist_type2a_params *t2p;
> >> >> +
> >> >> +     sglist = (struct omap_dma_sglist_node *) host->dma_ctrl_buf;
> >> >> +     snode = sglist;
> >> >> +     blksz = host->data->blksz;
> >> >> +
> >> >> +     if ((host->dma_len * sizeof(*snode)) > DMA_CTRL_BUF_SIZE) {
> >> >> +             dev_err(mmc_dev(host->mmc), "not enough sglist memory %d\n",
> >> >> +                     host->dma_len);
> >> >> +             return -ENOMEM;
> >> >> +     }
> >> >> +     for (i = 0; i < host->dma_len; snode++, i++) {
> >> >> +             snode->desc_type = OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a;
> >> >> +             snode->num_of_elem = blksz / 4;
> >> >> +             t2p = &snode->sg_node.t2a;
> >> >> +
> >> >> +             if (dmadir == DMA_FROM_DEVICE) {
> >> >> +                     t2p->src_addr = host->mapbase + OMAP_HSMMC_DATA;
> >> >> +                     t2p->dst_addr = sg_dma_address(data->sg + i);
> >> >> +             } else {
> >> >> +                     t2p->dst_addr = host->mapbase + OMAP_HSMMC_DATA;
> >> >> +                     t2p->src_addr = sg_dma_address(data->sg + i);
> >> >> +             }
> >> >> +             snode->flags =
> >> >> +                     OMAP_DMA_LIST_DST_VALID | OMAP_DMA_LIST_SRC_VALID;
> >> >> +
> >> >> +             t2p->cfn_fn = sg_dma_len(data->sg + i) / host->data->blksz;
> >> >> +             t2p->cicr = DMA_ICR_SGLIST_END;
> >> >> +
> >> >> +             t2p->dst_frame_idx_or_pkt_size = 0;
> >> >> +             t2p->src_frame_idx_or_pkt_size = 0;
> >> >> +             t2p->dst_elem_idx = 0;
> >> >> +             t2p->src_elem_idx = 0;
> >> >> +     }
> >> >> +     dev_dbg(mmc_dev(host->mmc), "new sglist %x len =%d\n",
> >> >> +                     host->dma_ctrl_buf_phy, i);
> >> >> +     omap_set_dma_sglist_mode(host->dma_ch, sglist,
> >> >> +                     host->dma_ctrl_buf_phy, i, NULL);
> >> >> +     omap_dma_set_sglist_fastmode(host->dma_ch, 1);
> >> >
> >> > You should check for return value of above two. If any one of above fails
> >> > the transfer will fail BADLY
> >>
> >> These functions have no return code (Similar to omap_start_dma, which doesn't
> >> have return code either)
> >
> > Are you sure ? Here is the function body.
> >
> > +int omap_set_dma_sglist_mode(int lch, struct omap_dma_sglist_node *sgparams,
> > +       dma_addr_t padd, int nelem, struct omap_dma_channel_params *chparams)
> > +{
> > +       struct omap_dma_list_config_params *lcfg;
> > +       int l = DMA_LIST_CDP_LISTMODE; /* Enable Linked list mode in CDP */
> > +
> > +       if ((dma_caps0_status & DMA_CAPS_SGLIST_SUPPORT) == 0) {
> > +               printk(KERN_ERR "omap DMA: sglist feature not supported\n");
> > +               return -EPERM;
> > +       }
> > +       if (dma_chan[lch].flags & OMAP_DMA_ACTIVE) {
> > +               printk(KERN_ERR "omap DMA: configuring active DMA channel\n");
> > +               return -EPERM;
> > +       }
> > +
> > +       if (padd == 0) {
> > +               printk(KERN_ERR "omap DMA: sglist invalid dma_addr\n");
> > +               return -EINVAL;
> > +       }
> > +       lcfg = &dma_chan[lch].list_config;
> > +
> > +       lcfg->sghead = sgparams;
> > +       lcfg->num_elem = nelem;
> > +       lcfg->sgheadphy = padd;
> > +       lcfg->pausenode = -1;
> > +
> > +
> > +       if (NULL == chparams)
> > +               l |= DMA_LIST_CDP_FASTMODE;
> > +       else
> > +               omap_set_dma_params(lch, chparams);
> > +
> > +       dma_write(l, CDP(lch));
> > +       dma_write(0, CCDN(lch)); /* Reset List index numbering */
> > +       /* Initialize frame and element counters to invalid values */
> > +       dma_write(OMAP_DMA_INVALID_FRAME_COUNT, CCFN(lch));
> > +       dma_write(OMAP_DMA_INVALID_ELEM_COUNT, CCEN(lch));
> > +
> > +       return dma_sglist_set_phy_params(sgparams, lcfg->sgheadphy,
> > +                       nelem, dma_read(CICR(lch)));
> > +
> > +}
> >
>  OK. I missed that.
> >
> >>
> >> >> +     return 0;
> >> >> +}
> >> >> +
> >> >>  static void set_data_timeout(struct omap_hsmmc_host *host,
> >> >>                            unsigned int timeout_ns,
> >> >>                            unsigned int timeout_clks)
> >> >> @@ -1420,14 +1492,23 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host
> >> >> *host, struct mmc_request *req)
> >> >>                                       | (req->data->blocks << 16));
> >> >>       set_data_timeout(host, req->data->timeout_ns, req->data->timeout_clks);
> >> >>
> >> >> -     if (host->use_dma) {
> >> >> -             ret = omap_hsmmc_start_dma_transfer(host, req);
> >> >> -             if (ret != 0) {
> >> >> -                     dev_dbg(mmc_dev(host->mmc), "MMC start dma failure\n");
> >> >> +     if (host->dma_caps & DMA_TYPE_SDMA) {
> >> >> +             ret = omap_hsmmc_configure_sdma(host, req);
> >> >> +             if (ret)
> >> >>                       return ret;
> >> >> -             }
> >> >> +             host->dma_in_use = DMA_TYPE_SDMA;
> >> >>       }
> >> >> -     return 0;
> >> >> +     if ((host->dma_caps & DMA_TYPE_SDMA_DLOAD) &&
> >> >> +             host->data->sg_len > 4) {
> >> >> +             ret = omap_hsmmc_configure_sdma_sglist(host, req);
> >> >> +             if (ret)
> >> >> +                     return ret;
> >> >> +             host->dma_in_use = DMA_TYPE_SDMA_DLOAD;
> >> >> +
> >> >> +     }
> >> >> +     ret = omap_hsmmc_start_dma_transfer(host);
> >> >> +     return ret;
> >> >> +
> >> >>  }
> >> >>
> >> >>  /*
> >> >> @@ -2007,7 +2088,9 @@ static int __init omap_hsmmc_probe(struct
> >> >> platform_device *pdev)
> >> >>       host->mmc       = mmc;
> >> >>       host->pdata     = pdata;
> >> >>       host->dev       = &pdev->dev;
> >> >> -     host->use_dma   = 1;
> >> >> +     host->dma_caps  = DMA_TYPE_SDMA;
> >> >> +     host->dma_in_use        = DMA_TYPE_NODMA;
> >> >> +     host->dma_ctrl_buf = NULL;
> >> >>       host->dev->dma_mask = &pdata->dma_mask;
> >> >>       host->dma_ch    = -1;
> >> >>       host->irq       = irq;
> >> >> @@ -2088,6 +2171,15 @@ static int __init omap_hsmmc_probe(struct
> >> >> platform_device *pdev)
> >> >>                                                       " clk failed\n");
> >> >>       }
> >> >>
> >> >> +     if (cpu_is_omap44xx() || cpu_is_omap3630()) {
> >> > Can we avoid above by passing this part of platform data??
> >> > devices.c
> >>
> >> I am not clear about the method. The board files export the
> >> omap_mmc_platform_data.
> >> Does it imply that all board files have to change and export
> >> the capability so that it can be queried ?
> > No. You don't have to modify the board files. This would need
> > change in devices.c which common for all omap boards.
> >
> > I don't have a strong opinion on this point but just put forth an
> > alternate way to avoid such SOC specific check in drivers.
> > You can take call on this
>
> Thanks. I will go with Madhu's vote now. With ADMA and other features
> coming in, I'll think of a generic way to export the capabilities
> from the platform data.
>
One of the problem with such approaches is ones the code get merged it's not
revisited from improvements.
If Tony is ok with this I am fine too.

Regards,
Santosh

.

RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Shilimkar, Santosh]

> -----Original Message-----
> From: kishore kadiyala [mailto:kishorek.kadiyala@gmail.com]
> Sent: Thursday, May 06, 2010 2:32 PM
> To: Shilimkar, Santosh
> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-
> kernel@lists.arm.linux.org.uk; Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Kadiyala, Kishore;
> Tony Lindgren
> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
> 
> <<snip>>
> 
> >> I am not clear about the method. The board files export the
> >> omap_mmc_platform_data.
> >> Does it imply that all board files have to change and export
> >> the capability so that it can be queried ?
> > No. You don't have to modify the board files. This would need
> > change in devices.c which common for all omap boards.
> >
> > I don't have a strong opinion on this point but just put forth an
> > alternate way to avoid such SOC specific check in drivers.
> > You can take call on this
> 
> Agree. How about adding a flag in hsmmc.h & omap_mmc_platform_data,
> that would take care of SDMA & SDMA_DLAOD in the driver instead  going
> with SOC check .

Good idea Kishore. 
Venkat,
Can you do what kishore is suggesting.

Regards,
Santosh

RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Madhusudhan]

> -----Original Message-----
> From: Shilimkar, Santosh [mailto:santosh.shilimkar@ti.com]
> Sent: Thursday, May 06, 2010 4:39 AM
> To: kishore kadiyala
> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org;
> linux-arm-kernel@lists.arm.linux.org.uk; Chikkature Rajashekar,
> Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
> Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
> autoloading feature
> 
> > -----Original Message-----
> > From: kishore kadiyala [mailto:kishorek.kadiyala@gmail.com]
> > Sent: Thursday, May 06, 2010 2:32 PM
> > To: Shilimkar, Santosh
> > Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
> mmc@vger.kernel.org; linux-arm-
> > kernel@lists.arm.linux.org.uk; Chikkature Rajashekar, Madhusudhan;
> Adrian Hunter; Kadiyala, Kishore;
> > Tony Lindgren
> > Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
> autoloading feature
> >
> > <<snip>>
> >
> > >> I am not clear about the method. The board files export the
> > >> omap_mmc_platform_data.
> > >> Does it imply that all board files have to change and export
> > >> the capability so that it can be queried ?
> > > No. You don't have to modify the board files. This would need
> > > change in devices.c which common for all omap boards.
> > >
> > > I don't have a strong opinion on this point but just put forth an
> > > alternate way to avoid such SOC specific check in drivers.
> > > You can take call on this
> >
> > Agree. How about adding a flag in hsmmc.h & omap_mmc_platform_data,
> > that would take care of SDMA & SDMA_DLAOD in the driver instead  going
> > with SOC check .
> 
> Good idea Kishore.
> Venkat,
> Can you do what kishore is suggesting.
> 
omap_mmc_platform_data is MMC specific platform data. Why add a SDMA
specific feature capability into it? Even though you add it there, you will
still need to have a cpu check before that can be set in a common code.

> Regards,
> Santosh

RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Shilimkar, Santosh]

> -----Original Message-----
> From: Chikkature Rajashekar, Madhusudhan
> Sent: Thursday, May 06, 2010 9:50 PM
> To: Shilimkar, Santosh; 'kishore kadiyala'
> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; linux-arm-
> kernel@lists.arm.linux.org.uk; 'Adrian Hunter'; Kadiyala, Kishore; 'Tony Lindgren'
> Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature
> 
> 
> 
> > -----Original Message-----
> > From: Shilimkar, Santosh [mailto:santosh.shilimkar@ti.com]
> > Sent: Thursday, May 06, 2010 4:39 AM
> > To: kishore kadiyala
> > Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org;
> > linux-arm-kernel@lists.arm.linux.org.uk; Chikkature Rajashekar,
> > Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
> > Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
> > autoloading feature
> >
> > > -----Original Message-----
> > > From: kishore kadiyala [mailto:kishorek.kadiyala@gmail.com]
> > > Sent: Thursday, May 06, 2010 2:32 PM
> > > To: Shilimkar, Santosh
> > > Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
> > mmc@vger.kernel.org; linux-arm-
> > > kernel@lists.arm.linux.org.uk; Chikkature Rajashekar, Madhusudhan;
> > Adrian Hunter; Kadiyala, Kishore;
> > > Tony Lindgren
> > > Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
> > autoloading feature
> > >
> > > <<snip>>
> > >
> > > >> I am not clear about the method. The board files export the
> > > >> omap_mmc_platform_data.
> > > >> Does it imply that all board files have to change and export
> > > >> the capability so that it can be queried ?
> > > > No. You don't have to modify the board files. This would need
> > > > change in devices.c which common for all omap boards.
> > > >
> > > > I don't have a strong opinion on this point but just put forth an
> > > > alternate way to avoid such SOC specific check in drivers.
> > > > You can take call on this
> > >
> > > Agree. How about adding a flag in hsmmc.h & omap_mmc_platform_data,
> > > that would take care of SDMA & SDMA_DLAOD in the driver instead  going
> > > with SOC check .
> >
> > Good idea Kishore.
> > Venkat,
> > Can you do what kishore is suggesting.
> >
> omap_mmc_platform_data is MMC specific platform data. Why add a SDMA
> specific feature capability into it? Even though you add it there, you will
> still need to have a cpu check before that can be set in a common code.
> 

CPU checks are allowed to be in the platform files. That is where such 
machine/SOC specific differentiation should be done and not in the device
drivers.
That way device drivers remains clean and portable.

I want to stop this thread here since neither the patch author nor the file
maintainer thinks that cpu checks in the device drivers is bad idea.

Please decide within yourself and move on.

Regards,
Santosh

RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Madhusudhan]

> -----Original Message-----
> From: Shilimkar, Santosh [mailto:santosh.shilimkar@ti.com]
> Sent: Thursday, May 06, 2010 11:52 PM
> To: Chikkature Rajashekar, Madhusudhan; 'kishore kadiyala'
> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org;
> linux-arm-kernel@lists.arm.linux.org.uk; 'Adrian Hunter'; Kadiyala,
> Kishore; 'Tony Lindgren'
> Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
> autoloading feature
> 
> > -----Original Message-----
> > From: Chikkature Rajashekar, Madhusudhan
> > Sent: Thursday, May 06, 2010 9:50 PM
> > To: Shilimkar, Santosh; 'kishore kadiyala'
> > Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
> mmc@vger.kernel.org; linux-arm-
> > kernel@lists.arm.linux.org.uk; 'Adrian Hunter'; Kadiyala, Kishore; 'Tony
> Lindgren'
> > Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
> autoloading feature
> >
> >
> >
> > > -----Original Message-----
> > > From: Shilimkar, Santosh [mailto:santosh.shilimkar@ti.com]
> > > Sent: Thursday, May 06, 2010 4:39 AM
> > > To: kishore kadiyala
> > > Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
> mmc@vger.kernel.org;
> > > linux-arm-kernel@lists.arm.linux.org.uk; Chikkature Rajashekar,
> > > Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
> > > Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
> > > autoloading feature
> > >
> > > > -----Original Message-----
> > > > From: kishore kadiyala [mailto:kishorek.kadiyala@gmail.com]
> > > > Sent: Thursday, May 06, 2010 2:32 PM
> > > > To: Shilimkar, Santosh
> > > > Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
> > > mmc@vger.kernel.org; linux-arm-
> > > > kernel@lists.arm.linux.org.uk; Chikkature Rajashekar, Madhusudhan;
> > > Adrian Hunter; Kadiyala, Kishore;
> > > > Tony Lindgren
> > > > Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma
> descriptor
> > > autoloading feature
> > > >
> > > > <<snip>>
> > > >
> > > > >> I am not clear about the method. The board files export the
> > > > >> omap_mmc_platform_data.
> > > > >> Does it imply that all board files have to change and export
> > > > >> the capability so that it can be queried ?
> > > > > No. You don't have to modify the board files. This would need
> > > > > change in devices.c which common for all omap boards.
> > > > >
> > > > > I don't have a strong opinion on this point but just put forth an
> > > > > alternate way to avoid such SOC specific check in drivers.
> > > > > You can take call on this
> > > >
> > > > Agree. How about adding a flag in hsmmc.h & omap_mmc_platform_data,
> > > > that would take care of SDMA & SDMA_DLAOD in the driver instead
> going
> > > > with SOC check .
> > >
> > > Good idea Kishore.
> > > Venkat,
> > > Can you do what kishore is suggesting.
> > >
> > omap_mmc_platform_data is MMC specific platform data. Why add a SDMA
> > specific feature capability into it? Even though you add it there, you
> will
> > still need to have a cpu check before that can be set in a common code.
> >
> 
> CPU checks are allowed to be in the platform files. That is where such
> machine/SOC specific differentiation should be done and not in the device
> drivers.
> That way device drivers remains clean and portable.
> 
> I want to stop this thread here since neither the patch author nor the
> file
> maintainer thinks that cpu checks in the device drivers is bad idea.
> 
> Please decide within yourself and move on.
> 

I am not saying that it is wrong. My point here is that adding this
particular flag into MMC platform data to differentiate a SDMA specific
feature which got introduced post certain SOC may not be needed. But you can
always post your comments on the list which will be looked at by a wider
audience and finally the right patch will go in. 

> Regards,
> Santosh

Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Nishanth Menon]

Chikkature Rajashekar, Madhusudhan had written, on 05/07/2010 11:59 AM, 
the following:
> 
>>
>>> -----Original Message-----
>>> From: Chikkature Rajashekar, Madhusudhan
>>> Sent: Thursday, May 06, 2010 9:50 PM
>>> To: Shilimkar, Santosh; 'kishore kadiyala'
>>> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
>> mmc@vger.kernel.org; linux-arm-
>>> kernel@lists.arm.linux.org.uk; 'Adrian Hunter'; Kadiyala, Kishore; 'Tony
>> Lindgren'
>>> Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
>> autoloading feature
>>>
>>>
>>>> -----Original Message-----
>>>> From: Shilimkar, Santosh [mailto:santosh.shilimkar@ti.com]
>>>> Sent: Thursday, May 06, 2010 4:39 AM
>>>> To: kishore kadiyala
>>>> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
>> mmc@vger.kernel.org;
>>>> linux-arm-kernel@lists.arm.linux.org.uk; Chikkature Rajashekar,
>>>> Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
>>>> Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
>>>> autoloading feature
>>>>
>>>>> -----Original Message-----
>>>>> From: kishore kadiyala [mailto:kishorek.kadiyala@gmail.com]
>>>>> Sent: Thursday, May 06, 2010 2:32 PM
>>>>> To: Shilimkar, Santosh
>>>>> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
>>>> mmc@vger.kernel.org; linux-arm-
>>>>> kernel@lists.arm.linux.org.uk; Chikkature Rajashekar, Madhusudhan;
>>>> Adrian Hunter; Kadiyala, Kishore;
>>>>> Tony Lindgren
>>>>> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma
>> descriptor
>>>> autoloading feature
>>>>> <<snip>>
>>>>>
>>>>>>> I am not clear about the method. The board files export the
>>>>>>> omap_mmc_platform_data.
>>>>>>> Does it imply that all board files have to change and export
>>>>>>> the capability so that it can be queried ?
>>>>>> No. You don't have to modify the board files. This would need
>>>>>> change in devices.c which common for all omap boards.
>>>>>>
>>>>>> I don't have a strong opinion on this point but just put forth an
>>>>>> alternate way to avoid such SOC specific check in drivers.
>>>>>> You can take call on this
>>>>> Agree. How about adding a flag in hsmmc.h & omap_mmc_platform_data,
>>>>> that would take care of SDMA & SDMA_DLAOD in the driver instead
>> going
>>>>> with SOC check .
>>>> Good idea Kishore.
>>>> Venkat,
>>>> Can you do what kishore is suggesting.
>>>>
>>> omap_mmc_platform_data is MMC specific platform data. Why add a SDMA
>>> specific feature capability into it? Even though you add it there, you
>> will
>>> still need to have a cpu check before that can be set in a common code.
>>>
>> CPU checks are allowed to be in the platform files. That is where such
>> machine/SOC specific differentiation should be done and not in the device
>> drivers.
>> That way device drivers remains clean and portable.
>>
>> I want to stop this thread here since neither the patch author nor the
>> file
>> maintainer thinks that cpu checks in the device drivers is bad idea.
>>
>> Please decide within yourself and move on.
>>
> 
> I am not saying that it is wrong. My point here is that adding this
> particular flag into MMC platform data to differentiate a SDMA specific
> feature which got introduced post certain SOC may not be needed. But you can
> always post your comments on the list which will be looked at by a wider
> audience and finally the right patch will go in. 
Please see [1] for SOC specific feature handling. any reasons we can't 
handle it by adding a new feature?

[1] 
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/plat-omap/include/plat/cpu.h#l439

-- 
Regards,
Nishanth Menon

RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Madhusudhan]

<snip>

> >>>>
> >>> omap_mmc_platform_data is MMC specific platform data. Why add a SDMA
> >>> specific feature capability into it? Even though you add it there, you
> >> will
> >>> still need to have a cpu check before that can be set in a common
> code.
> >>>
> >> CPU checks are allowed to be in the platform files. That is where such
> >> machine/SOC specific differentiation should be done and not in the
> device
> >> drivers.
> >> That way device drivers remains clean and portable.
> >>
> >> I want to stop this thread here since neither the patch author nor the
> >> file
> >> maintainer thinks that cpu checks in the device drivers is bad idea.
> >>
> >> Please decide within yourself and move on.
> >>
> >
> > I am not saying that it is wrong. My point here is that adding this
> > particular flag into MMC platform data to differentiate a SDMA specific
> > feature which got introduced post certain SOC may not be needed. But you
> can
> > always post your comments on the list which will be looked at by a wider
> > audience and finally the right patch will go in.
> Please see [1] for SOC specific feature handling. any reasons we can't
> handle it by adding a new feature?
> 
> [1]
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-
> 2.6.git;a=blob;f=arch/arm/plat-omap/include/plat/cpu.h#l439
> 

Yes that makes correct sense. Note that these features will exist on omap4
also so handle in a common way. Surely this will be better than adding a
flag which does not belong to MMC platform data.

Regards,
Madhu

> --
> Regards,
> Nishanth Menon

Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Venkatraman S]

Nishanth Menon <nm@ti.com> wrote:
> Chikkature Rajashekar, Madhusudhan had written, on 05/07/2010 11:59 AM, the
> following:
>>
>>>
>>>> -----Original Message-----
>>>> From: Chikkature Rajashekar, Madhusudhan
>>>> Sent: Thursday, May 06, 2010 9:50 PM
>>>> To: Shilimkar, Santosh; 'kishore kadiyala'
>>>> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
>>>
>>> mmc@vger.kernel.org; linux-arm-
>>>>
>>>> kernel@lists.arm.linux.org.uk; 'Adrian Hunter'; Kadiyala, Kishore; 'Tony
>>>
>>> Lindgren'
>>>>
>>>> Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
>>>
>>> autoloading feature
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Shilimkar, Santosh [mailto:santosh.shilimkar@ti.com]
>>>>> Sent: Thursday, May 06, 2010 4:39 AM
>>>>> To: kishore kadiyala
>>>>> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
>>>
>>> mmc@vger.kernel.org;
>>>>>
>>>>> linux-arm-kernel@lists.arm.linux.org.uk; Chikkature Rajashekar,
>>>>> Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
>>>>> Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
>>>>> autoloading feature
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: kishore kadiyala [mailto:kishorek.kadiyala@gmail.com]
>>>>>> Sent: Thursday, May 06, 2010 2:32 PM
>>>>>> To: Shilimkar, Santosh
>>>>>> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
>>>>>
>>>>> mmc@vger.kernel.org; linux-arm-
>>>>>>
>>>>>> kernel@lists.arm.linux.org.uk; Chikkature Rajashekar, Madhusudhan;
>>>>>
>>>>> Adrian Hunter; Kadiyala, Kishore;
>>>>>>
>>>>>> Tony Lindgren
>>>>>> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma
>>>
>>> descriptor
>>>>>
>>>>> autoloading feature
>>>>>>
>>>>>> <<snip>>
>>>>>>
>>>>>>>> I am not clear about the method. The board files export the
>>>>>>>> omap_mmc_platform_data.
>>>>>>>> Does it imply that all board files have to change and export
>>>>>>>> the capability so that it can be queried ?
>>>>>>>
>>>>>>> No. You don't have to modify the board files. This would need
>>>>>>> change in devices.c which common for all omap boards.
>>>>>>>
>>>>>>> I don't have a strong opinion on this point but just put forth an
>>>>>>> alternate way to avoid such SOC specific check in drivers.
>>>>>>> You can take call on this
>>>>>>
>>>>>> Agree. How about adding a flag in hsmmc.h & omap_mmc_platform_data,
>>>>>> that would take care of SDMA & SDMA_DLAOD in the driver instead
>>>
>>> going
>>>>>>
>>>>>> with SOC check .
>>>>>
>>>>> Good idea Kishore.
>>>>> Venkat,
>>>>> Can you do what kishore is suggesting.
>>>>>
>>>> omap_mmc_platform_data is MMC specific platform data. Why add a SDMA
>>>> specific feature capability into it? Even though you add it there, you
>>>
>>> will
>>>>
>>>> still need to have a cpu check before that can be set in a common code.
>>>>
>>> CPU checks are allowed to be in the platform files. That is where such
>>> machine/SOC specific differentiation should be done and not in the device
>>> drivers.
>>> That way device drivers remains clean and portable.
>>>
>>> I want to stop this thread here since neither the patch author nor the
>>> file
>>> maintainer thinks that cpu checks in the device drivers is bad idea.
>>>
>>> Please decide within yourself and move on.
>>>
>>
>> I am not saying that it is wrong. My point here is that adding this
>> particular flag into MMC platform data to differentiate a SDMA specific
>> feature which got introduced post certain SOC may not be needed. But you
>> can
>> always post your comments on the list which will be looked at by a wider
>> audience and finally the right patch will go in.
>
> Please see [1] for SOC specific feature handling. any reasons we can't
> handle it by adding a new feature?
>
> [1]
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/plat-omap/include/plat/cpu.h#l439
>

Thanks. I can add a new feature here, but I see that the API is tied
to OMAP3, whereas the DMA feature is common
to 3630, OMAP4 and mostly everything after that. I can work on an
upgrade, but do you see that
as a dependency and done on the context of this patch ?
Regards,
Venkat.

Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Nishanth Menon]

On 05/09/2010 05:51 AM, Venkatraman S wrote:
> Nishanth Menon<nm@ti.com>  wrote:
>> Chikkature Rajashekar, Madhusudhan had written, on 05/07/2010 11:59 AM, the
>> following:
>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Chikkature Rajashekar, Madhusudhan
>>>>> Sent: Thursday, May 06, 2010 9:50 PM
>>>>> To: Shilimkar, Santosh; 'kishore kadiyala'
>>>>> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
>>>>
>>>> mmc@vger.kernel.org; linux-arm-
>>>>>
>>>>> kernel@lists.arm.linux.org.uk; 'Adrian Hunter'; Kadiyala, Kishore; 'Tony
>>>>
>>>> Lindgren'
>>>>>
>>>>> Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
>>>>
>>>> autoloading feature
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Shilimkar, Santosh [mailto:santosh.shilimkar@ti.com]
>>>>>> Sent: Thursday, May 06, 2010 4:39 AM
>>>>>> To: kishore kadiyala
>>>>>> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
>>>>
>>>> mmc@vger.kernel.org;
>>>>>>
>>>>>> linux-arm-kernel@lists.arm.linux.org.uk; Chikkature Rajashekar,
>>>>>> Madhusudhan; Adrian Hunter; Kadiyala, Kishore; Tony Lindgren
>>>>>> Subject: RE: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor
>>>>>> autoloading feature
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: kishore kadiyala [mailto:kishorek.kadiyala@gmail.com]
>>>>>>> Sent: Thursday, May 06, 2010 2:32 PM
>>>>>>> To: Shilimkar, Santosh
>>>>>>> Cc: S, Venkatraman; linux-omap@vger.kernel.org; linux-
>>>>>>
>>>>>> mmc@vger.kernel.org; linux-arm-
>>>>>>>
>>>>>>> kernel@lists.arm.linux.org.uk; Chikkature Rajashekar, Madhusudhan;
>>>>>>
>>>>>> Adrian Hunter; Kadiyala, Kishore;
>>>>>>>
>>>>>>> Tony Lindgren
>>>>>>> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma
>>>>
>>>> descriptor
>>>>>>
>>>>>> autoloading feature
>>>>>>>
>>>>>>> <<snip>>
>>>>>>>
>>>>>>>>> I am not clear about the method. The board files export the
>>>>>>>>> omap_mmc_platform_data.
>>>>>>>>> Does it imply that all board files have to change and export
>>>>>>>>> the capability so that it can be queried ?
>>>>>>>>
>>>>>>>> No. You don't have to modify the board files. This would need
>>>>>>>> change in devices.c which common for all omap boards.
>>>>>>>>
>>>>>>>> I don't have a strong opinion on this point but just put forth an
>>>>>>>> alternate way to avoid such SOC specific check in drivers.
>>>>>>>> You can take call on this
>>>>>>>
>>>>>>> Agree. How about adding a flag in hsmmc.h&  omap_mmc_platform_data,
>>>>>>> that would take care of SDMA&  SDMA_DLAOD in the driver instead
>>>>
>>>> going
>>>>>>>
>>>>>>> with SOC check .
>>>>>>
>>>>>> Good idea Kishore.
>>>>>> Venkat,
>>>>>> Can you do what kishore is suggesting.
>>>>>>
>>>>> omap_mmc_platform_data is MMC specific platform data. Why add a SDMA
>>>>> specific feature capability into it? Even though you add it there, you
>>>>
>>>> will
>>>>>
>>>>> still need to have a cpu check before that can be set in a common code.
>>>>>
>>>> CPU checks are allowed to be in the platform files. That is where such
>>>> machine/SOC specific differentiation should be done and not in the device
>>>> drivers.
>>>> That way device drivers remains clean and portable.
>>>>
>>>> I want to stop this thread here since neither the patch author nor the
>>>> file
>>>> maintainer thinks that cpu checks in the device drivers is bad idea.
>>>>
>>>> Please decide within yourself and move on.
>>>>
>>>
>>> I am not saying that it is wrong. My point here is that adding this
>>> particular flag into MMC platform data to differentiate a SDMA specific
>>> feature which got introduced post certain SOC may not be needed. But you
>>> can
>>> always post your comments on the list which will be looked at by a wider
>>> audience and finally the right patch will go in.
>>
>> Please see [1] for SOC specific feature handling. any reasons we can't
>> handle it by adding a new feature?
>>
>> [1]
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/plat-omap/include/plat/cpu.h#l439
>>
>
> Thanks. I can add a new feature here, but I see that the API is tied
> to OMAP3, whereas the DMA feature is common
> to 3630, OMAP4 and mostly everything after that. I can work on an
> upgrade, but do you see that
> as a dependency and done on the context of this patch ?
> Regards,
> Venkat.

Yes, I am aware that the current APIs are tied to OMAP3, no reason that 
we cant introduce a OMAP version independent feature.. Yes, IMHO, this 
is an SOC specific feature that has no place in a platform data.. lets 
not misuse that.
Regards,
NM

Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Venkatraman S]

Nishanth Menon <menon.nishanth@gmail.com> wrote:
> On 05/09/2010 05:51 AM, Venkatraman S wrote:
>>
>> Nishanth Menon<nm@ti.com>  wrote:
>>>
>>> Chikkature Rajashekar, Madhusudhan had written, on 05/07/2010 11:59 AM,
>>> the
>>> following:
>>>>
>>>>>
>>>>>>>> Subject: Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma
>>>>>
>>>>> descriptor
>>>>>>>
>>>>>>> autoloading feature
>>>>>>>>
>>>>>>>> <<snip>>
>>>>>>>>
>>>>>>>>>> I am not clear about the method. The board files export the
>>>>>>>>>> omap_mmc_platform_data.
>>>>>>>>>> Does it imply that all board files have to change and export
>>>>>>>>>> the capability so that it can be queried ?
>>>>>>>>>
>>>>>>>>> No. You don't have to modify the board files. This would need
>>>>>>>>> change in devices.c which common for all omap boards.
>>>>>>>>>
>>>>>>>>> I don't have a strong opinion on this point but just put forth an
>>>>>>>>> alternate way to avoid such SOC specific check in drivers.
>>>>>>>>> You can take call on this
>>>>>>>>
>>>>>>>> Agree. How about adding a flag in hsmmc.h&  omap_mmc_platform_data,
>>>>>>>> that would take care of SDMA&  SDMA_DLAOD in the driver instead
>>>>>
>>>>> going
>>>>>>>>
>>>>>>>> with SOC check .
>>>>>>>
>>>>>>> Good idea Kishore.
>>>>>>> Venkat,
>>>>>>> Can you do what kishore is suggesting.
>>>>>>>
>>>>>> omap_mmc_platform_data is MMC specific platform data. Why add a SDMA
>>>>>> specific feature capability into it? Even though you add it there, you
>>>>>
>>>>> will
>>>>>>
>>>>>> still need to have a cpu check before that can be set in a common
>>>>>> code.
>>>>>>
>>>>> CPU checks are allowed to be in the platform files. That is where such
>>>>> machine/SOC specific differentiation should be done and not in the
>>>>> device
>>>>> drivers.
>>>>> That way device drivers remains clean and portable.
>>>>>
>>>>> I want to stop this thread here since neither the patch author nor the
>>>>> file
>>>>> maintainer thinks that cpu checks in the device drivers is bad idea.
>>>>>
>>>>> Please decide within yourself and move on.
>>>>>
>>>>
>>>> I am not saying that it is wrong. My point here is that adding this
>>>> particular flag into MMC platform data to differentiate a SDMA specific
>>>> feature which got introduced post certain SOC may not be needed. But you
>>>> can
>>>> always post your comments on the list which will be looked at by a wider
>>>> audience and finally the right patch will go in.
>>>
>>> Please see [1] for SOC specific feature handling. any reasons we can't
>>> handle it by adding a new feature?
>>>
>>> [1]
>>>
>>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/plat-omap/include/plat/cpu.h#l439
>>>
>>
>> Thanks. I can add a new feature here, but I see that the API is tied
>> to OMAP3, whereas the DMA feature is common
>> to 3630, OMAP4 and mostly everything after that. I can work on an
>> upgrade, but do you see that
>> as a dependency and done on the context of this patch ?
>> Regards,
>> Venkat.
>
> Yes, I am aware that the current APIs are tied to OMAP3, no reason that we
> cant introduce a OMAP version independent feature.. Yes, IMHO, this is an
> SOC specific feature that has no place in a platform data.. lets not misuse
> that.
> Regards,
> NM


Draft patch for this is here (Tested ok). I will post this as part of
the series if this looks ok...
This doesn't have all existing feature checks for OMAP4, but they seem to be not
needed/used anyway..

---
 arch/arm/mach-omap2/clock3xxx_data.c  |    2 -
 arch/arm/mach-omap2/id.c              |   25 +++++++++++++++--------
 arch/arm/plat-omap/include/plat/cpu.h |   36 +++++++++++++++++-----------------
 3 files changed, 36 insertions(+), 27 deletions(-)

Index: linux-omap-2.6/arch/arm/mach-omap2/id.c
===================================================================
--- linux-omap-2.6.orig/arch/arm/mach-omap2/id.c	2010-05-09
23:29:28.000000000 +0530
+++ linux-omap-2.6/arch/arm/mach-omap2/id.c	2010-05-10 17:39:29.000000000 +0530
@@ -28,7 +28,7 @@
 static struct omap_chip_id omap_chip;
 static unsigned int omap_revision;

-u32 omap3_features;
+u32 omap_features;

 unsigned int omap_rev(void)
 {
@@ -161,14 +161,14 @@
 #define OMAP3_CHECK_FEATURE(status,feat)				\
 	if (((status & OMAP3_ ##feat## _MASK) 				\
 		>> OMAP3_ ##feat## _SHIFT) != FEAT_ ##feat## _NONE) { 	\
-		omap3_features |= OMAP3_HAS_ ##feat;			\
+		omap_features |= OMAP_HAS_ ##feat;			\
 	}

 void __init omap3_check_features(void)
 {
 	u32 status;

-	omap3_features = 0;
+	omap_features = 0;

 	status = omap_ctrl_readl(OMAP3_CONTROL_OMAP_STATUS);

@@ -178,7 +178,7 @@
 	OMAP3_CHECK_FEATURE(status, NEON);
 	OMAP3_CHECK_FEATURE(status, ISP);
 	if (cpu_is_omap3630())
-		omap3_features |= OMAP3_HAS_192MHZ_CLK;
+		omap_features |= OMAP_HAS_192MHZ_CLK | OMAP_HAS_SDMA_DLOAD;

 	/*
 	 * TODO: Get additional info (where applicable)
@@ -267,6 +267,12 @@
 	}
 }

+void __init omap4_check_features(void)
+{
+	omap_features = OMAP_HAS_SDMA_DLOAD;
+
+}
+
 void __init omap4_check_revision(void)
 {
 	u32 idcode;
@@ -294,7 +300,7 @@
 }

 #define OMAP3_SHOW_FEATURE(feat)		\
-	if (omap3_has_ ##feat())		\
+	if (omap_has_ ##feat())		\
 		printk(#feat" ");

 void __init omap3_cpuinfo(void)
@@ -314,20 +320,20 @@
 		/*
 		 * AM35xx devices
 		 */
-		if (omap3_has_sgx()) {
+		if (omap_has_sgx()) {
 			omap_revision = OMAP3517_REV(rev);
 			strcpy(cpu_name, "AM3517");
 		} else {
 			/* Already set in omap3_check_revision() */
 			strcpy(cpu_name, "AM3505");
 		}
-	} else if (omap3_has_iva() && omap3_has_sgx()) {
+	} else if (omap_has_iva() && omap_has_sgx()) {
 		/* OMAP3430, OMAP3525, OMAP3515, OMAP3503 devices */
 		strcpy(cpu_name, "OMAP3430/3530");
-	} else if (omap3_has_iva()) {
+	} else if (omap_has_iva()) {
 		omap_revision = OMAP3525_REV(rev);
 		strcpy(cpu_name, "OMAP3525");
-	} else if (omap3_has_sgx()) {
+	} else if (omap_has_sgx()) {
 		omap_revision = OMAP3515_REV(rev);
 		strcpy(cpu_name, "OMAP3515");
 	} else {
@@ -386,6 +392,7 @@
 		return;
 	} else if (cpu_is_omap44xx()) {
 		omap4_check_revision();
+		omap4_check_features();
 		return;
 	} else {
 		pr_err("OMAP revision unknown, please fix!\n");
Index: linux-omap-2.6/arch/arm/plat-omap/include/plat/cpu.h
===================================================================
--- linux-omap-2.6.orig/arch/arm/plat-omap/include/plat/cpu.h	2010-05-09
23:24:15.000000000 +0530
+++ linux-omap-2.6/arch/arm/plat-omap/include/plat/cpu.h	2010-05-10
17:53:04.000000000 +0530
@@ -434,28 +434,30 @@
 void omap2_check_revision(void);

 /*
- * Runtime detection of OMAP3 features
+ * Runtime detection of OMAP features
  */
-extern u32 omap3_features;
+extern u32 omap_features;

-#define OMAP3_HAS_L2CACHE		BIT(0)
-#define OMAP3_HAS_IVA			BIT(1)
-#define OMAP3_HAS_SGX			BIT(2)
-#define OMAP3_HAS_NEON			BIT(3)
-#define OMAP3_HAS_ISP			BIT(4)
-#define OMAP3_HAS_192MHZ_CLK		BIT(5)
+#define OMAP_HAS_L2CACHE		BIT(0)
+#define OMAP_HAS_IVA			BIT(1)
+#define OMAP_HAS_SGX			BIT(2)
+#define OMAP_HAS_NEON			BIT(3)
+#define OMAP_HAS_ISP			BIT(4)
+#define OMAP_HAS_192MHZ_CLK		BIT(5)
+#define OMAP_HAS_SDMA_DLOAD		BIT(6)

-#define OMAP3_HAS_FEATURE(feat,flag)			\
-static inline unsigned int omap3_has_ ##feat(void)	\
+#define OMAP_HAS_FEATURE(feat, flag)			\
+static inline unsigned int omap_has_ ##feat(void)	\
 {							\
-	return (omap3_features & OMAP3_HAS_ ##flag);	\
+	return (omap_features & OMAP_HAS_ ##flag);	\
 }							\

-OMAP3_HAS_FEATURE(l2cache, L2CACHE)
-OMAP3_HAS_FEATURE(sgx, SGX)
-OMAP3_HAS_FEATURE(iva, IVA)
-OMAP3_HAS_FEATURE(neon, NEON)
-OMAP3_HAS_FEATURE(isp, ISP)
-OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
+OMAP_HAS_FEATURE(l2cache, L2CACHE)
+OMAP_HAS_FEATURE(sgx, SGX)
+OMAP_HAS_FEATURE(iva, IVA)
+OMAP_HAS_FEATURE(neon, NEON)
+OMAP_HAS_FEATURE(isp, ISP)
+OMAP_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
+OMAP_HAS_FEATURE(sdma_dload, SDMA_DLOAD)

 #endif
Index: linux-omap-2.6/arch/arm/mach-omap2/clock3xxx_data.c
===================================================================
--- linux-omap-2.6.orig/arch/arm/mach-omap2/clock3xxx_data.c	2010-05-10
14:41:39.000000000 +0530
+++ linux-omap-2.6/arch/arm/mach-omap2/clock3xxx_data.c	2010-05-10
14:41:50.000000000 +0530
@@ -3510,7 +3510,7 @@
 			cpu_clkflg |= CK_3430ES2;
 		}
 	}
-	if (omap3_has_192mhz_clk())
+	if (omap_has_192mhz_clk())
 		omap_96m_alwon_fck = omap_96m_alwon_fck_3630;

 	if (cpu_is_omap3630()) {
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Nishanth Menon]

On 05/10/2010 07:31 AM, Venkatraman S wrote:
> Nishanth Menon<menon.nishanth@gmail.com>  wrote:

>>>> Please see [1] for SOC specific feature handling. any reasons we can't
>>>> handle it by adding a new feature?
>>>>
>>>> [1]
>>>>
>>>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/plat-omap/include/plat/cpu.h#l439
>>>>
>>>
>>> Thanks. I can add a new feature here, but I see that the API is tied
>>> to OMAP3, whereas the DMA feature is common
>>> to 3630, OMAP4 and mostly everything after that. I can work on an
>>> upgrade, but do you see that
>>> as a dependency and done on the context of this patch ?
>>> Regards,
>>> Venkat.
>>
>> Yes, I am aware that the current APIs are tied to OMAP3, no reason that we
>> cant introduce a OMAP version independent feature.. Yes, IMHO, this is an
>> SOC specific feature that has no place in a platform data.. lets not misuse
>> that.
>> Regards,
>> NM
>
>
> Draft patch for this is here (Tested ok). I will post this as part of
> the series if this looks ok...
> This doesn't have all existing feature checks for OMAP4, but they seem to be not
> needed/used anyway..

NAK. DO NOT DO two logically exclusive things in a single patch:
a) you are switching omap3_features to omap_features
b) you are introducing sdma feature for the same.

please split and post to l-o for review as a proper patch.

>
> ---
>   arch/arm/mach-omap2/clock3xxx_data.c  |    2 -
>   arch/arm/mach-omap2/id.c              |   25 +++++++++++++++--------
>   arch/arm/plat-omap/include/plat/cpu.h |   36 +++++++++++++++++-----------------
>   3 files changed, 36 insertions(+), 27 deletions(-)
>
> Index: linux-omap-2.6/arch/arm/mach-omap2/id.c
> ===================================================================
> --- linux-omap-2.6.orig/arch/arm/mach-omap2/id.c	2010-05-09
> 23:29:28.000000000 +0530
> +++ linux-omap-2.6/arch/arm/mach-omap2/id.c	2010-05-10 17:39:29.000000000 +0530
> @@ -28,7 +28,7 @@
>   static struct omap_chip_id omap_chip;
>   static unsigned int omap_revision;
>
> -u32 omap3_features;
> +u32 omap_features;
>
>   unsigned int omap_rev(void)
>   {
> @@ -161,14 +161,14 @@
>   #define OMAP3_CHECK_FEATURE(status,feat)				\
>   	if (((status&  OMAP3_ ##feat## _MASK) 				\
>   		>>  OMAP3_ ##feat## _SHIFT) != FEAT_ ##feat## _NONE) { 	\
> -		omap3_features |= OMAP3_HAS_ ##feat;			\
> +		omap_features |= OMAP_HAS_ ##feat;			\
>   	}
>
>   void __init omap3_check_features(void)
>   {
>   	u32 status;
>
> -	omap3_features = 0;
> +	omap_features = 0;
>
>   	status = omap_ctrl_readl(OMAP3_CONTROL_OMAP_STATUS);
>
> @@ -178,7 +178,7 @@
>   	OMAP3_CHECK_FEATURE(status, NEON);
>   	OMAP3_CHECK_FEATURE(status, ISP);
>   	if (cpu_is_omap3630())
> -		omap3_features |= OMAP3_HAS_192MHZ_CLK;
> +		omap_features |= OMAP_HAS_192MHZ_CLK | OMAP_HAS_SDMA_DLOAD;
>
>   	/*
>   	 * TODO: Get additional info (where applicable)
> @@ -267,6 +267,12 @@
>   	}
>   }
>
> +void __init omap4_check_features(void)
> +{
> +	omap_features = OMAP_HAS_SDMA_DLOAD;
> +
> +}
> +
>   void __init omap4_check_revision(void)
>   {
>   	u32 idcode;
> @@ -294,7 +300,7 @@
>   }
>
>   #define OMAP3_SHOW_FEATURE(feat)		\
> -	if (omap3_has_ ##feat())		\
> +	if (omap_has_ ##feat())		\
>   		printk(#feat" ");
>
>   void __init omap3_cpuinfo(void)
> @@ -314,20 +320,20 @@
>   		/*
>   		 * AM35xx devices
>   		 */
> -		if (omap3_has_sgx()) {
> +		if (omap_has_sgx()) {
>   			omap_revision = OMAP3517_REV(rev);
>   			strcpy(cpu_name, "AM3517");
>   		} else {
>   			/* Already set in omap3_check_revision() */
>   			strcpy(cpu_name, "AM3505");
>   		}
> -	} else if (omap3_has_iva()&&  omap3_has_sgx()) {
> +	} else if (omap_has_iva()&&  omap_has_sgx()) {
>   		/* OMAP3430, OMAP3525, OMAP3515, OMAP3503 devices */
>   		strcpy(cpu_name, "OMAP3430/3530");
> -	} else if (omap3_has_iva()) {
> +	} else if (omap_has_iva()) {
>   		omap_revision = OMAP3525_REV(rev);
>   		strcpy(cpu_name, "OMAP3525");
> -	} else if (omap3_has_sgx()) {
> +	} else if (omap_has_sgx()) {
>   		omap_revision = OMAP3515_REV(rev);
>   		strcpy(cpu_name, "OMAP3515");
>   	} else {
> @@ -386,6 +392,7 @@
>   		return;
>   	} else if (cpu_is_omap44xx()) {
>   		omap4_check_revision();
> +		omap4_check_features();
>   		return;
>   	} else {
>   		pr_err("OMAP revision unknown, please fix!\n");
> Index: linux-omap-2.6/arch/arm/plat-omap/include/plat/cpu.h
> ===================================================================
> --- linux-omap-2.6.orig/arch/arm/plat-omap/include/plat/cpu.h	2010-05-09
> 23:24:15.000000000 +0530
> +++ linux-omap-2.6/arch/arm/plat-omap/include/plat/cpu.h	2010-05-10
> 17:53:04.000000000 +0530
> @@ -434,28 +434,30 @@
>   void omap2_check_revision(void);
>
>   /*
> - * Runtime detection of OMAP3 features
> + * Runtime detection of OMAP features
>    */
> -extern u32 omap3_features;
> +extern u32 omap_features;
>
> -#define OMAP3_HAS_L2CACHE		BIT(0)
> -#define OMAP3_HAS_IVA			BIT(1)
> -#define OMAP3_HAS_SGX			BIT(2)
> -#define OMAP3_HAS_NEON			BIT(3)
> -#define OMAP3_HAS_ISP			BIT(4)
> -#define OMAP3_HAS_192MHZ_CLK		BIT(5)
> +#define OMAP_HAS_L2CACHE		BIT(0)
> +#define OMAP_HAS_IVA			BIT(1)
> +#define OMAP_HAS_SGX			BIT(2)
> +#define OMAP_HAS_NEON			BIT(3)
> +#define OMAP_HAS_ISP			BIT(4)
> +#define OMAP_HAS_192MHZ_CLK		BIT(5)
> +#define OMAP_HAS_SDMA_DLOAD		BIT(6)
>
> -#define OMAP3_HAS_FEATURE(feat,flag)			\
> -static inline unsigned int omap3_has_ ##feat(void)	\
> +#define OMAP_HAS_FEATURE(feat, flag)			\
> +static inline unsigned int omap_has_ ##feat(void)	\
>   {							\
> -	return (omap3_features&  OMAP3_HAS_ ##flag);	\
> +	return (omap_features&  OMAP_HAS_ ##flag);	\
>   }							\
>
> -OMAP3_HAS_FEATURE(l2cache, L2CACHE)
> -OMAP3_HAS_FEATURE(sgx, SGX)
> -OMAP3_HAS_FEATURE(iva, IVA)
> -OMAP3_HAS_FEATURE(neon, NEON)
> -OMAP3_HAS_FEATURE(isp, ISP)
> -OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
> +OMAP_HAS_FEATURE(l2cache, L2CACHE)
> +OMAP_HAS_FEATURE(sgx, SGX)
> +OMAP_HAS_FEATURE(iva, IVA)
> +OMAP_HAS_FEATURE(neon, NEON)
> +OMAP_HAS_FEATURE(isp, ISP)
> +OMAP_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
> +OMAP_HAS_FEATURE(sdma_dload, SDMA_DLOAD)

>
>   #endif
> Index: linux-omap-2.6/arch/arm/mach-omap2/clock3xxx_data.c
> ===================================================================
> --- linux-omap-2.6.orig/arch/arm/mach-omap2/clock3xxx_data.c	2010-05-10
> 14:41:39.000000000 +0530
> +++ linux-omap-2.6/arch/arm/mach-omap2/clock3xxx_data.c	2010-05-10
> 14:41:50.000000000 +0530
> @@ -3510,7 +3510,7 @@
>   			cpu_clkflg |= CK_3430ES2;
>   		}
>   	}
> -	if (omap3_has_192mhz_clk())
> +	if (omap_has_192mhz_clk())
>   		omap_96m_alwon_fck = omap_96m_alwon_fck_3630;
>
>   	if (cpu_is_omap3630()) {

Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Venkatraman S]

Nishanth Menon <nm@ti.com> wrote:
> On 05/10/2010 07:31 AM, Venkatraman S wrote:
>>
>> Nishanth Menon<menon.nishanth@gmail.com>  wrote:
>
>>>>> Please see [1] for SOC specific feature handling. any reasons we can't
>>>>> handle it by adding a new feature?
>>>>>
>>>>> [1]
>>>>>
>>>>>
>>>>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/plat-omap/include/plat/cpu.h#l439
>>>>>
>>>>
>>>> Thanks. I can add a new feature here, but I see that the API is tied
>>>> to OMAP3, whereas the DMA feature is common
>>>> to 3630, OMAP4 and mostly everything after that. I can work on an
>>>> upgrade, but do you see that
>>>> as a dependency and done on the context of this patch ?
>>>> Regards,
>>>> Venkat.
>>>
>>> Yes, I am aware that the current APIs are tied to OMAP3, no reason that
>>> we
>>> cant introduce a OMAP version independent feature.. Yes, IMHO, this is an
>>> SOC specific feature that has no place in a platform data.. lets not
>>> misuse
>>> that.
>>> Regards,
>>> NM
>>

Just a note that I have updated my patch series based on all the
comments except that the 'features' framework has to be created to be
utilised by this patch series. That would be a separate patch, which I
am working on, and will post on monday.
Regards,
Venkat.

Re: [PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Venkatraman S]

Nishanth Menon <nm@ti.com> wrote:
> On 05/10/2010 07:31 AM, Venkatraman S wrote:
>>
>> Nishanth Menon<menon.nishanth@gmail.com>  wrote:
>
>>>>> Please see [1] for SOC specific feature handling. any reasons we can't
>>>>> handle it by adding a new feature?
>>>>>
>>>>> [1]
>>>>>
>>>>>
>>>>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/plat-omap/include/plat/cpu.h#l439
>>>>>
>>>>
>>>> Thanks. I can add a new feature here, but I see that the API is tied
>>>> to OMAP3, whereas the DMA feature is common
>>>> to 3630, OMAP4 and mostly everything after that. I can work on an
>>>> upgrade, but do you see that
>>>> as a dependency and done on the context of this patch ?
>>>> Regards,
>>>> Venkat.
>>>
>>> Yes, I am aware that the current APIs are tied to OMAP3, no reason that
>>> we
>>> cant introduce a OMAP version independent feature.. Yes, IMHO, this is an
>>> SOC specific feature that has no place in a platform data.. lets not
>>> misuse
>>> that.
>>> Regards,
>>> NM
>>

Just a note that I have updated my patch series based on all the
comments except that the 'features' framework has to be created to be
utilised by this patch series. That would be a separate patch, which I
am working on, and will post on monday.
Regards,
Venkat.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 2/2] omap hsmmc: adaptation of sdma descriptor autoloading feature

[Venkatraman S]

Nishanth Menon <nm@ti.com> wrote:
> On 05/10/2010 07:31 AM, Venkatraman S wrote:
>>
>> Nishanth Menon<menon.nishanth@gmail.com> ?wrote:
>
>>>>> Please see [1] for SOC specific feature handling. any reasons we can't
>>>>> handle it by adding a new feature?
>>>>>
>>>>> [1]
>>>>>
>>>>>
>>>>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/plat-omap/include/plat/cpu.h#l439
>>>>>
>>>>
>>>> Thanks. I can add a new feature here, but I see that the API is tied
>>>> to OMAP3, whereas the DMA feature is common
>>>> to 3630, OMAP4 and mostly everything after that. I can work on an
>>>> upgrade, but do you see that
>>>> as a dependency and done on the context of this patch ?
>>>> Regards,
>>>> Venkat.
>>>
>>> Yes, I am aware that the current APIs are tied to OMAP3, no reason that
>>> we
>>> cant introduce a OMAP version independent feature.. Yes, IMHO, this is an
>>> SOC specific feature that has no place in a platform data.. lets not
>>> misuse
>>> that.
>>> Regards,
>>> NM
>>

Just a note that I have updated my patch series based on all the
comments except that the 'features' framework has to be created to be
utilised by this patch series. That would be a separate patch, which I
am working on, and will post on monday.
Regards,
Venkat.