> -----Original Message----- > From: svenkatr@xxxxxxxxx [mailto:svenkatr@xxxxxxxxx] On Behalf Of Venkatraman S > Sent: Wednesday, May 05, 2010 9:49 PM > To: Shilimkar, Santosh > Cc: linux-omap@xxxxxxxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxxxxx; > 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@xxxxxx> wrote: > > > >> -----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 > > 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-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
