Thanks for the valuable inputs. I have made some modifications to the davinci host controller driver and to the MMC frame work. I will send the patches soon and also ensure to follow the community guidelines from now on.
-Phaneedra Kumar.A 2009/7/29 Nori, Sekhar <[email protected]> > Hello Phaneendra, > > On Wed, Jul 29, 2009 at 18:26:49, Phaneendra kumar wrote: > > > > Hi all, > > > > This patch will add SDIO support to the DM355 host controller driver. > > > > I have verified this on DM355 EVM board in both DMA and PIO modes. And i > have used open source libertas driver for verifying the SDIO functionality. > > > > Signed-off-by: Phaneendra Kumar <[email protected]> > > ----- > > diff --git a/drivers/mmc/host/davinci_mmc.c > b/drivers/mmc/host/davinci_mmc.c > > index 8907b72..0744059 100644 > > --- a/drivers/mmc/host/davinci_mmc.c > > +++ b/drivers/mmc/host/davinci_mmc.c > > @@ -31,6 +31,8 @@ > [..] > > +/* DAVINCI_SDIOCTL definitions */ > > +#define SDIOCTL_RDWTRQ_SET (1 << 0) > > +#define SDIOCTL_RDWTCR_SET (1 << 0) > > + > > +/* DAVINCI_SDIOST0 definitions */ > > +#define SDIOST0_DAT1_HI (1 << 0) > > +#define SDIOST0_INTPRD (1 << 1) > > +#define SDIOST0_RDWTST (1 << 2) > > + > > +/* DAVINCI_SDIOIEN definitions */ > > +#define SDIOIEN_IOINTEN (1 << 0) > > +#define SDIOIEN_RWSEN (1 << 1) > > + > > +/* DAVINCI_SDIOIST definitions */ > > +#define SDIOIST_IOINT (1 << 0) > > +#define SDIOIST_RWS (1 << 1) > > The comments are superfluous also please use BIT(x) > > > + > > /* > > * One scatterlist dma "segment" is at most MAX_CCNT rw_threshold units, > > * and we handle up to NR_SG segments. MMC_BLOCK_BOUNCE kicks in only > > @@ -181,6 +200,10 @@ struct mmc_davinci_host { > > u32 rxdma, txdma; > > bool use_dma; > > bool do_dma; > > + struct timer_list sdio_timer; > > + u32 sdioInt; > > CamelCase.. > > > + /* For sdio irq enabling and disabling */ > > + spinlock_t sdio_lock; > > Why spin lock just to disable SDIO irq? May be comment > needs correction? > > > > > /* Scatterlist DMA uses one or more parameter RAM entries: > > * the main one (associated with rxdma or txdma) plus zero or > > @@ -202,6 +225,41 @@ struct mmc_davinci_host { > > unsigned ns_in_one_cycle; > > }; > > > > +static void mmc_enable_sdio_irq(struct mmc_host *mmc, int enable); > > Declaring a static function should not typically > be needed. > > > + > > +static inline void davinci_sdio_intr_chck(struct mmc_davinci_host *host) > > +{ > > + if (host->mmc->caps & MMC_CAP_SDIO_IRQ) { > > + if (!((readl(host->base + DAVINCI_SDIOST0)) & > SDIOST0_DAT1_HI) > > + && host->sdioInt) { > > May be using host->sdioInt as the first operand to && > will save a register read in case sdioInt is not enabled? > > > + writel(SDIOIST_IOINT, host->base + > DAVINCI_SDIOIST); > > + mmc_signal_sdio_irq(host->mmc); > > + } > > + } > > +} > > + > > +static inline void davinci_cmd_dat_reset(struct mmc_davinci_host *host) > > +{ > > + u32 temp = 0; > > + > > + if (host->mmc->card) { > > + if (mmc_card_sdio(host->mmc->card)) { > > + temp = readl(host->base + DAVINCI_MMCCTL); > > + writel(temp | MMCCTL_CMDRST | MMCCTL_DATRST, > > + host->base + DAVINCI_MMCCTL); > > + > > + temp &= ~(MMCCTL_CMDRST | MMCCTL_DATRST); > > + writel(temp, host->base + DAVINCI_MMCCTL); > > + } > > + } > > +} > > + > > +static void davinci_sdio_timer(unsigned long data) > > +{ > > + struct mmc_davinci_host *host = (struct mmc_davinci_host *)data; > > + > > + davinci_sdio_intr_chck(host); > > Can eliminate 'host' variable. > > > +} > > > > /* PIO only */ > > static void mmc_davinci_sg_to_buf(struct mmc_davinci_host *host) > > @@ -387,6 +445,15 @@ static void mmc_davinci_dma_cb(unsigned channel, u16 > ch_status, void *data) > > if (DMA_COMPLETE != ch_status) { > > struct mmc_davinci_host *host = data; > > > > + if (!(host->data)) { > > + printk(KERN_ERR "DMA Event Miss / NULL Transfr\n"); > > + edma_stop(host->txdma); > > + edma_clean_channel(host->txdma); > > + edma_stop(host->rxdma); > > + edma_clean_channel(host->rxdma); > > + return; > > + } > > + > > /* Currently means: DMA Event Missed, or "null" transfer > > * request was seen. In the future, TC errors (like bad > > * addresses) might be presented too. > > @@ -664,6 +731,14 @@ mmc_davinci_prepare_data(struct mmc_davinci_host > *host, struct mmc_request *req) > > host->buffer = NULL; > > host->bytes_left = data->blocks * data->blksz; > > > > + if (host->mmc->card) { > > + if (mmc_card_sdio(host->mmc->card)) { > > + if (data->blksz == 64) { > > + mdelay(5); > > + } > > Can you include a comment explaining why the delay > is needed? Please include a reference to spru section > if applicable. > > > + } > > + } > > + > > /* For now we try to use DMA whenever we won't need partial FIFO > > * reads or writes, either for the whole transfer (as tested here) > > * or for any individual scatterlist segment (tested when we call > > @@ -706,6 +781,8 @@ static void mmc_davinci_request(struct mmc_host *mmc, > struct mmc_request *req) > > return; > > } > > > > + davinci_cmd_dat_reset(host); > > + > > host->do_dma = 0; > > mmc_davinci_prepare_data(host, req); > > mmc_davinci_start_command(host, req->cmd); > > @@ -826,12 +903,9 @@ static void mmc_davinci_set_ios(struct mmc_host > *mmc, struct mmc_ios *ios) > > static void > > mmc_davinci_xfer_done(struct mmc_davinci_host *host, struct mmc_data > *data) > > { > > - host->data = NULL; > > - host->data_dir = DAVINCI_MMC_DATADIR_NONE; > > + davinci_abort_dma(host); > > > > if (host->do_dma) { > > - davinci_abort_dma(host); > > - > > dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len, > > (data->flags & MMC_DATA_WRITE) > > ? DMA_TO_DEVICE > > @@ -839,11 +913,18 @@ mmc_davinci_xfer_done(struct mmc_davinci_host > *host, struct mmc_data *data) > > host->do_dma = false; > > } > > > > + host->data = NULL; > > + host->data_dir = DAVINCI_MMC_DATADIR_NONE; > > + > > + davinci_cmd_dat_reset(host); > > + > > if (!data->stop || (host->cmd && host->cmd->error)) { > > mmc_request_done(host->mmc, data->mrq); > > writel(0, host->base + DAVINCI_MMCIM); > > } else > > mmc_davinci_start_command(host, data->stop); > > + > > + davinci_sdio_intr_chck(host); > > } > > > > static void mmc_davinci_cmd_done(struct mmc_davinci_host *host, > > @@ -870,6 +951,7 @@ static void mmc_davinci_cmd_done(struct > mmc_davinci_host *host, > > mmc_request_done(host->mmc, cmd->mrq); > > writel(0, host->base + DAVINCI_MMCIM); > > } > > + davinci_sdio_intr_chck(host); > > } > > > > static void > > @@ -895,6 +977,17 @@ static irqreturn_t mmc_davinci_irq(int irq, void > *dev_id) > > int end_transfer = 0; > > struct mmc_data *data = host->data; > > > > + if (host->mmc->caps & MMC_CAP_SDIO_IRQ) { > > + status = readl(host->base + DAVINCI_SDIOIST); > > + if (status & SDIOIST_IOINT) { > > + dev_dbg(mmc_dev(host->mmc), > > + "SDIO interrupt status %x\n", > status); > > + writel(status | SDIOIST_IOINT, > > + host->base + DAVINCI_SDIOIST); > > + mmc_signal_sdio_irq(host->mmc); > > + } > > + } > > + > > if (host->cmd == NULL && host->data == NULL) { > > status = readl(host->base + DAVINCI_MMCST0); > > dev_dbg(mmc_dev(host->mmc), > > @@ -904,6 +997,7 @@ static irqreturn_t mmc_davinci_irq(int irq, void > *dev_id) > > return IRQ_NONE; > > } > > > > + davinci_sdio_intr_chck(host); > > status = readl(host->base + DAVINCI_MMCST0); > > qstatus = status; > > > > @@ -1031,11 +1125,41 @@ static int mmc_davinci_get_ro(struct mmc_host > *mmc) > > return config->get_ro(pdev->id); > > } > > > > +static void mmc_enable_sdio_irq(struct mmc_host *mmc, int enable) > > +{ > > + struct mmc_davinci_host *host = mmc_priv(mmc); > > + unsigned long flags; > > + > > + spin_lock_irqsave(&host->sdio_lock, flags); > > + > > + if (enable) { > > + if (!((readl(host->base + DAVINCI_SDIOST0)) > > + & SDIOST0_DAT1_HI)) { > > + writel(SDIOIST_IOINT, > > + host->base + DAVINCI_SDIOIST); > > + mmc_signal_sdio_irq(host->mmc); > > + }else { > > + host->sdioInt = 1; > > + mod_timer(&host->sdio_timer, jiffies + (HZ/100)); > > + writel(readl(host->base + DAVINCI_SDIOIEN) | > > + SDIOIEN_IOINTEN, host->base + > DAVINCI_SDIOIEN); > > + } > > + } else { > > + host->sdioInt = 0; > > + del_timer(&host->sdio_timer); > > + writel(readl(host->base + DAVINCI_SDIOIEN) & > ~SDIOIEN_IOINTEN, > > + host->base + DAVINCI_SDIOIEN); > > + } > > Why not setup & add the timer here itself if enable == TRUE? > > I don't understand the hardware, but a timer task every jiffy > will affect power management support in future. > > > + > > + spin_unlock_irqrestore(&host->sdio_lock, flags); > > +} > > + > > static struct mmc_host_ops mmc_davinci_ops = { > > .request = mmc_davinci_request, > > .set_ios = mmc_davinci_set_ios, > > .get_cd = mmc_davinci_get_cd, > > .get_ro = mmc_davinci_get_ro, > > + .enable_sdio_irq = mmc_enable_sdio_irq, > > }; > > > > > /*----------------------------------------------------------------------*/ > > @@ -1132,6 +1256,12 @@ static int __init davinci_mmcsd_probe(struct > platform_device *pdev) > > /* REVISIT: someday, support IRQ-driven card detection. */ > > mmc->caps |= MMC_CAP_NEEDS_POLL; > > > > + mmc->caps |= MMC_CAP_SDIO_IRQ; > > + spin_lock_init(&host->sdio_lock); > > + setup_timer(&host->sdio_timer, davinci_sdio_timer, > > + (unsigned long)host); > > Does the timer need to be deleted on module exit or > is that taken care by mmc_enable_sdio_irq() itself? > > Thanks, > Sekhar > > > + host->sdioInt = 0; > > + > > if (!pdata || pdata->wires == 4 || pdata->wires == 0) > > mmc->caps |= MMC_CAP_4_BIT_DATA; > > > > _______________________________________________ > > Davinci-linux-open-source mailing list > > [email protected] > > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source > > > > >
_______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
