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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: svenkatr@xxxxxxxxx [mailto:svenkatr@xxxxxxxxx] On Behalf Of Venkatraman S
> Sent: Thursday, April 29, 2010 11:05 PM
> To: linux-omap@xxxxxxxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxx>
> 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@xxxxxx>
> CC: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> CC: Madhusudhan C <madhu.cr@xxxxxx>
> CC: Shilimkar Santosh <santosh.shilimkar@xxxxxx>
> CC: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux