[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. >> + >> +#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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
