[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]

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.

[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

.

[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.