| Message ID | 1350538335-29026-1-git-send-email-b32955@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2012-10-18 at 13:32 +0800, Huang Shijie wrote: > From: Huang Shijie <shijie8@gmail.com> > > [1] Why add these new DMA control commands? > In mx6q, the gpmi-nand driver is the only user of the APBH-DMA. The dma clock > is enabled when we have successfully requested a DMA channel. So even when > the gpmi-nand driver does not work, the dma clock(apbh-dma) still runs > in high speed (198MHz). To save some power, it is better to disable the dma > clock when the dma device, such as gpmi-nand, is not working anymore. > When the dma device becomes work again, enable the dma clock again. > > [2] add new DMA control commands: DMA_START/DMA_END > DMA_START: do some preprations to start the DMA engine, such as enable the > necessary clocks. > DMA_END: do some works to end the DMA engine, such as disable the > necessary clocks. > > [3] This patch does not change any logic in i2c-mxs driver and mxs-pcm driver. > But for gpmi-nand driver, we will enable the the clock only when we select > the nand chip, and want to do some real jobs with the nand chip. > For mxs-mmc driver, disable the dma clock in the suspend; and enable the > dma clock in the resume. > Why cant you do start (prepare clock etc) when you submit the descriptor to dmaengine. Can be done in tx_submit callback. Similarly remove the clock when dma transaction gets completed. I don't think we need a new API for this, it needs to be handled by driver on its own. > Signed-off-by: Huang Shijie <b32955@freescale.com> > --- > drivers/dma/mxs-dma.c | 17 +++++++++++++++++ > drivers/i2c/busses/i2c-mxs.c | 10 +++++++++- > drivers/mmc/host/mxs-mmc.c | 19 ++++++++++++++++--- > drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 10 ++++++++++ > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 10 ++++++---- > include/linux/dmaengine.h | 6 ++++++ > sound/soc/mxs/mxs-pcm.c | 12 ++++++++++++ > 7 files changed, 76 insertions(+), 8 deletions(-) > > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c > index 9f02e79..89286f4 100644 > --- a/drivers/dma/mxs-dma.c > +++ b/drivers/dma/mxs-dma.c > @@ -384,6 +384,8 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan) > /* the descriptor is ready */ > async_tx_ack(&mxs_chan->desc); > > + clk_disable_unprepare(mxs_dma->clk); > + > return 0; > > err_clk: > @@ -399,6 +401,14 @@ static void mxs_dma_free_chan_resources(struct dma_chan *chan) > { > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan); > struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma; > + int ret; > + > + ret = clk_prepare_enable(mxs_dma->clk); > + if (ret) { > + dev_err(mxs_dma->dma_device.dev, > + "failed in enabling the dma clock\n"); > + return; > + } > > mxs_dma_disable_chan(mxs_chan); > > @@ -597,9 +607,13 @@ static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, > unsigned long arg) > { > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan); > + struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma; > int ret = 0; > > switch (cmd) { > + case DMA_START: > + ret = clk_prepare_enable(mxs_dma->clk); > + break; > case DMA_TERMINATE_ALL: > mxs_dma_reset_chan(mxs_chan); > mxs_dma_disable_chan(mxs_chan); > @@ -610,6 +624,9 @@ static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, > case DMA_RESUME: > mxs_dma_resume_chan(mxs_chan); > break; > + case DMA_END: > + clk_disable_unprepare(mxs_dma->clk); > + break; > default: > ret = -ENOSYS; > } > diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c > index 1f58197..da1e881 100644 > --- a/drivers/i2c/busses/i2c-mxs.c > +++ b/drivers/i2c/busses/i2c-mxs.c > @@ -643,6 +643,12 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev) > dev_err(dev, "Failed to request dma\n"); > return -ENODEV; > } > + err = dmaengine_device_control(i2c->dmach, DMA_START, 0); > + if (err) { > + dma_release_channel(i2c->dmach); > + dev_err(dev, "Failed to start dma\n"); > + return err; > + } > } > > platform_set_drvdata(pdev, i2c); > @@ -680,8 +686,10 @@ static int __devexit mxs_i2c_remove(struct platform_device *pdev) > if (ret) > return -EBUSY; > > - if (i2c->dmach) > + if (i2c->dmach) { > + dmaengine_device_control(i2c->dmach, DMA_END, 0); > dma_release_channel(i2c->dmach); > + } > > writel(MXS_I2C_CTRL0_SFTRST, i2c->regs + MXS_I2C_CTRL0_SET); > > diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c > index 80d1e6d..aa91830 100644 > --- a/drivers/mmc/host/mxs-mmc.c > +++ b/drivers/mmc/host/mxs-mmc.c > @@ -676,6 +676,9 @@ static int mxs_mmc_probe(struct platform_device *pdev) > "%s: failed to request dma\n", __func__); > goto out_clk_put; > } > + ret = dmaengine_device_control(ssp->dmach, DMA_START, 0); > + if (ret) > + goto out_free_dma; > > /* set mmc core parameters */ > mmc->ops = &mxs_mmc_ops; > @@ -717,18 +720,20 @@ static int mxs_mmc_probe(struct platform_device *pdev) > ret = devm_request_irq(&pdev->dev, irq_err, mxs_mmc_irq_handler, 0, > DRIVER_NAME, host); > if (ret) > - goto out_free_dma; > + goto out_end_free_dma; > > spin_lock_init(&host->lock); > > ret = mmc_add_host(mmc); > if (ret) > - goto out_free_dma; > + goto out_end_free_dma; > > dev_info(mmc_dev(host->mmc), "initialized\n"); > > return 0; > > +out_end_free_dma: > + dmaengine_device_control(ssp->dmach, DMA_END, 0); > out_free_dma: > if (ssp->dmach) > dma_release_channel(ssp->dmach); > @@ -750,8 +755,10 @@ static int mxs_mmc_remove(struct platform_device *pdev) > > platform_set_drvdata(pdev, NULL); > > - if (ssp->dmach) > + if (ssp->dmach) { > + dmaengine_device_control(ssp->dmach, DMA_END, 0); > dma_release_channel(ssp->dmach); > + } > > clk_disable_unprepare(ssp->clk); > clk_put(ssp->clk); > @@ -772,6 +779,7 @@ static int mxs_mmc_suspend(struct device *dev) > ret = mmc_suspend_host(mmc); > > clk_disable_unprepare(ssp->clk); > + dmaengine_device_control(ssp->dmach, DMA_END, 0); > > return ret; > } > @@ -784,8 +792,13 @@ static int mxs_mmc_resume(struct device *dev) > int ret = 0; > > clk_prepare_enable(ssp->clk); > + ret = dmaengine_device_control(ssp->dmach, DMA_START, 0); > + if (ret) > + return ret; > > ret = mmc_resume_host(mmc); > + if (ret) > + dmaengine_device_control(ssp->dmach, DMA_END, 0); > > return ret; > } > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > index 3502acc..20ed3f3 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > @@ -958,6 +958,7 @@ void gpmi_begin(struct gpmi_nand_data *this) > uint32_t reg; > unsigned int dll_wait_time_in_us; > struct gpmi_nfc_hardware_timing hw; > + struct dma_chan *channel = get_dma_chan(this); > int ret; > > /* Enable the clock. */ > @@ -967,6 +968,12 @@ void gpmi_begin(struct gpmi_nand_data *this) > goto err_out; > } > > + ret = dmaengine_device_control(channel, DMA_START, 0); > + if (ret) { > + gpmi_disable_clk(this); > + goto err_out; > + } > + > /* Only initialize the timing once */ > if (this->flags & GPMI_TIMING_INIT_OK) > return; > @@ -1035,7 +1042,10 @@ err_out: > > void gpmi_end(struct gpmi_nand_data *this) > { > + struct dma_chan *channel = get_dma_chan(this); > + > gpmi_disable_clk(this); > + dmaengine_device_control(channel, DMA_END, 0); > } > > /* Clears a BCH interrupt. */ > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > index e2c56fc..5694d03 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > @@ -815,12 +815,14 @@ static void gpmi_select_chip(struct mtd_info *mtd, int chipnr) > struct nand_chip *chip = mtd->priv; > struct gpmi_nand_data *this = chip->priv; > > - if ((this->current_chip < 0) && (chipnr >= 0)) > + if ((this->current_chip < 0) && (chipnr >= 0)) { > + /* set the current_chip before we call gpmi_begin(). */ > + this->current_chip = chipnr; > gpmi_begin(this); > - else if ((this->current_chip >= 0) && (chipnr < 0)) > + } else if ((this->current_chip >= 0) && (chipnr < 0)) { > gpmi_end(this); > - > - this->current_chip = chipnr; > + this->current_chip = chipnr; > + } > } > > static void gpmi_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index d3201e4..79f864a 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -199,6 +199,8 @@ enum dma_ctrl_flags { > /** > * enum dma_ctrl_cmd - DMA operations that can optionally be exercised > * on a running channel. > + * @DMA_START: do some preprations to start the DMA engine, such as enable the > + * necessary clocks. > * @DMA_TERMINATE_ALL: terminate all ongoing transfers > * @DMA_PAUSE: pause ongoing transfers > * @DMA_RESUME: resume paused transfer > @@ -209,13 +211,17 @@ enum dma_ctrl_flags { > * command. > * @FSLDMA_EXTERNAL_START: this command will put the Freescale DMA controller > * into external start mode. > + * @DMA_END: do some works to end the DMA engine, such as disable the > + * necessary clocks. > */ > enum dma_ctrl_cmd { > + DMA_START, > DMA_TERMINATE_ALL, > DMA_PAUSE, > DMA_RESUME, > DMA_SLAVE_CONFIG, > FSLDMA_EXTERNAL_START, > + DMA_END, > }; > > /** > diff --git a/sound/soc/mxs/mxs-pcm.c b/sound/soc/mxs/mxs-pcm.c > index f82d766..cfcc30f 100644 > --- a/sound/soc/mxs/mxs-pcm.c > +++ b/sound/soc/mxs/mxs-pcm.c > @@ -92,6 +92,7 @@ static int snd_mxs_open(struct snd_pcm_substream *substream) > { > struct snd_soc_pcm_runtime *rtd = substream->private_data; > struct mxs_pcm_dma_data *pcm_dma_data; > + struct dma_chan *chan; > int ret; > > pcm_dma_data = kzalloc(sizeof(*pcm_dma_data), GFP_KERNEL); > @@ -107,6 +108,14 @@ static int snd_mxs_open(struct snd_pcm_substream *substream) > return ret; > } > > + chan = snd_dmaengine_pcm_get_chan(substream); > + ret = dmaengine_device_control(chan, DMA_START, (unsigned long)0); > + if (ret) { > + snd_dmaengine_pcm_close(substream); > + kfree(pcm_dma_data); > + return ret; > + } > + > snd_soc_set_runtime_hwparams(substream, &snd_mxs_hardware); > > snd_dmaengine_pcm_set_data(substream, pcm_dma_data); > @@ -117,7 +126,10 @@ static int snd_mxs_open(struct snd_pcm_substream *substream) > static int snd_mxs_close(struct snd_pcm_substream *substream) > { > struct mxs_pcm_dma_data *pcm_dma_data = snd_dmaengine_pcm_get_data(substream); > + struct dma_chan *chan; > > + chan = snd_dmaengine_pcm_get_chan(substream); > + dmaengine_device_control(chan, DMA_END, 0); > snd_dmaengine_pcm_close(substream); > kfree(pcm_dma_data); >
? 2012?10?18? 14:18, Vinod Koul ??: > Why cant you do start (prepare clock etc) when you submit the descriptor > to dmaengine. Can be done in tx_submit callback. > Similarly remove the clock when dma transaction gets completed. I ever thought this method too. But it will become low efficient in the following case: Assuming the gpmi-nand driver has to read out 1024 pages in one _SINGLE_ read operation. The gpmi-nand will submit the descriptor to dmaengine per page. So with your method, the system will repeat the enable/disable dma clock 1024 time. At every enable/disable dma clock, the system has to enable the clock chain and it's parents ... But with this patch, we only need to enable/disable dma clock one time, just at we select the nand chip. thanks Huang Shijie
Dear Huang Shijie, Why such massive CC ? > ? 2012?10?18? 14:18, Vinod Koul ??: > > Why cant you do start (prepare clock etc) when you submit the descriptor > > to dmaengine. Can be done in tx_submit callback. > > Similarly remove the clock when dma transaction gets completed. > > I ever thought this method too. > > But it will become low efficient in the following case: > > Assuming the gpmi-nand driver has to read out 1024 pages in one > _SINGLE_ read operation. > The gpmi-nand will submit the descriptor to dmaengine per page. It will? Then GPMI NAND is flat our broken ... again. > So with > your method, > the system will repeat the enable/disable dma clock 1024 time. Yes, it is the driver that's wrong. > At every > enable/disable dma clock, > the system has to enable the clock chain and it's parents ... > > But with this patch, we only need to enable/disable dma clock one time, > just at we select the nand chip. You are fixing a driver problem at a framework level, wrong. So, check how the MXS SPI driver handles descriptor chaining. Indeed, the Sigmatel screwed the DMA stuff good. But if you analyze the SPI driver, you'll notice a few important points that might come handy when you fix the GPMI NAND driver properly. The direction to take here is: 1) Implement DMA chaining into the GPMI NAND driver 2) You might need to do one PIO transfer to reconfigure the IP registers between each segment of the DMA chain (just as MXS SPI does it) 3) You might run out of DMA descriptors when doing too long chains -- so you might need to fix that part of the mxs DMA driver. > thanks > Huang Shijie Best regards, Marek Vasut
? 2012?10?18? 15:14, Marek Vasut ??: > Dear Huang Shijie, > > Why such massive CC ? > >> ? 2012?10?18? 14:18, Vinod Koul ??: >>> Why cant you do start (prepare clock etc) when you submit the descriptor >>> to dmaengine. Can be done in tx_submit callback. >>> Similarly remove the clock when dma transaction gets completed. >> I ever thought this method too. >> >> But it will become low efficient in the following case: >> >> Assuming the gpmi-nand driver has to read out 1024 pages in one >> _SINGLE_ read operation. >> The gpmi-nand will submit the descriptor to dmaengine per page. > It will? Then GPMI NAND is flat our broken ... again. yes. Please read the NAND chip spec about the comand READ PAGE(00h-30h) and the code nand_do_read_ops(). The nand chip limits us only read one page out one time. So the driver will submit the descriptor to dmaengine per page. >> So with >> your method, >> the system will repeat the enable/disable dma clock 1024 time. > Yes, it is the driver that's wrong. not the driver. >> At every >> enable/disable dma clock, >> the system has to enable the clock chain and it's parents ... >> >> But with this patch, we only need to enable/disable dma clock one time, >> just at we select the nand chip. > You are fixing a driver problem at a framework level, wrong. > > So, check how the MXS SPI driver handles descriptor chaining. Indeed, the > Sigmatel screwed the DMA stuff good. But if you analyze the SPI driver, you'll > notice a few important points that might come handy when you fix the GPMI NAND > driver properly. > > The direction to take here is: > 1) Implement DMA chaining into the GPMI NAND driver How can i implement the DMA chain if the nand chip READ-PAGE command gives us the one page limit? thanks Huang Shijie > 2) You might need to do one PIO transfer to reconfigure the IP registers between > each segment of the DMA chain (just as MXS SPI does it) > 3) You might run out of DMA descriptors when doing too long chains -- so you > might need to fix that part of the mxs DMA driver.
Dear Huang Shijie, > ? 2012?10?18? 15:14, Marek Vasut ??: > > Dear Huang Shijie, > > > > Why such massive CC ? > > > >> ? 2012?10?18? 14:18, Vinod Koul ??: > >>> Why cant you do start (prepare clock etc) when you submit the > >>> descriptor to dmaengine. Can be done in tx_submit callback. > >>> Similarly remove the clock when dma transaction gets completed. > >> > >> I ever thought this method too. > >> > >> But it will become low efficient in the following case: > >> Assuming the gpmi-nand driver has to read out 1024 pages in one > >> > >> _SINGLE_ read operation. > >> The gpmi-nand will submit the descriptor to dmaengine per page. > > > > It will? Then GPMI NAND is flat our broken ... again. > > yes. > > Please read the NAND chip spec about the comand READ PAGE(00h-30h) and > the code > nand_do_read_ops(). The nand chip limits us only read one page out one > time. So the driver will submit the descriptor to dmaengine per page. So we can't stream data from the chip? About time to adjust the MTD framework to allow that. Maybe implement a command queue? > >> So with > >> your method, > >> the system will repeat the enable/disable dma clock 1024 time. > > > > Yes, it is the driver that's wrong. > > not the driver. > > >> At every > >> enable/disable dma clock, > >> the system has to enable the clock chain and it's parents ... > >> > >> But with this patch, we only need to enable/disable dma clock one time, > >> just at we select the nand chip. > > > > You are fixing a driver problem at a framework level, wrong. > > > > So, check how the MXS SPI driver handles descriptor chaining. Indeed, the > > Sigmatel screwed the DMA stuff good. But if you analyze the SPI driver, > > you'll notice a few important points that might come handy when you fix > > the GPMI NAND driver properly. > > > > The direction to take here is: > > 1) Implement DMA chaining into the GPMI NAND driver > > How can i implement the DMA chain if the nand chip READ-PAGE command > gives us the one page limit? This smells like a time to extend the MTD api ? > thanks > Huang Shijie > > > 2) You might need to do one PIO transfer to reconfigure the IP registers > > between each segment of the DMA chain (just as MXS SPI does it) > > 3) You might run out of DMA descriptors when doing too long chains -- so > > you might need to fix that part of the mxs DMA driver. Best regards, Marek Vasut
? 2012?10?18? 16:16, Marek Vasut ??: > So we can't stream data from the chip? About time to adjust the MTD framework to > allow that. Maybe implement a command queue? > to Artem & David: is this possible to stream the data out with a command queue? thanks Huang Shijie
? 2012?10?18? 16:16, Marek Vasut ??: > So we can't stream data from the chip? About time to adjust the MTD framework to > allow that. Maybe implement a command queue? > IMHO, it's not possible. Because the READ-PAGE(00h-30h) command needs to check the busy status which means we have to stop in the middle, so we can not chain the all the read-pages DMA commands. thanks Huang Shijie
Dear Huang Shijie, > ? 2012?10?18? 16:16, Marek Vasut ??: > > So we can't stream data from the chip? About time to adjust the MTD > > framework to allow that. Maybe implement a command queue? > > IMHO, it's not possible. Because the READ-PAGE(00h-30h) command needs to > check the busy status > which means we have to stop in the middle, so we can not chain the all > the read-pages DMA commands. Can the DMA not branch? > thanks > Huang Shijie Best regards, Marek Vasut
On Thu, Oct 18, 2012 at 02:45:41PM +0800, Huang Shijie wrote: > ? 2012?10?18? 14:18, Vinod Koul ??: >> Why cant you do start (prepare clock etc) when you submit the descriptor >> to dmaengine. Can be done in tx_submit callback. >> Similarly remove the clock when dma transaction gets completed. > I ever thought this method too. > > But it will become low efficient in the following case: > > Assuming the gpmi-nand driver has to read out 1024 pages in one > _SINGLE_ read operation. > The gpmi-nand will submit the descriptor to dmaengine per page. So with > your method, > the system will repeat the enable/disable dma clock 1024 time. At every > enable/disable dma clock, > the system has to enable the clock chain and it's parents ... And what if you stop using clk_prepare_enable(), and prepare the clock when the channel is requested and only use clk_enable() in the tx_submit method?
? 2012?10?18? 16:52, Russell King - ARM Linux ??: > On Thu, Oct 18, 2012 at 02:45:41PM +0800, Huang Shijie wrote: >> ? 2012?10?18? 14:18, Vinod Koul ??: >>> Why cant you do start (prepare clock etc) when you submit the descriptor >>> to dmaengine. Can be done in tx_submit callback. >>> Similarly remove the clock when dma transaction gets completed. >> I ever thought this method too. >> >> But it will become low efficient in the following case: >> >> Assuming the gpmi-nand driver has to read out 1024 pages in one >> _SINGLE_ read operation. >> The gpmi-nand will submit the descriptor to dmaengine per page. So with >> your method, >> the system will repeat the enable/disable dma clock 1024 time. At every >> enable/disable dma clock, >> the system has to enable the clock chain and it's parents ... > And what if you stop using clk_prepare_enable(), and prepare the clock > when the channel is requested and only use clk_enable() in the tx_submit yes. it's a little better. There is nearly no difference between the clk_prepare_enable() and clk_enable() in actually. the clk_gate2_ops does not have any @->prepare. thanks Huang Shijie > method? >
? 2012?10?18? 16:49, Marek Vasut ??: > Dear Huang Shijie, > >> ? 2012?10?18? 16:16, Marek Vasut ??: >>> So we can't stream data from the chip? About time to adjust the MTD >>> framework to allow that. Maybe implement a command queue? >> IMHO, it's not possible. Because the READ-PAGE(00h-30h) command needs to >> check the busy status >> which means we have to stop in the middle, so we can not chain the all >> the read-pages DMA commands. > Can the DMA not branch? it's too complicated to the MTD layer, as well as the gpmi driver. Best Regards Huang Shijie
On 18 October 2012 12:15, Huang Shijie <b32955@freescale.com> wrote: > ? 2012?10?18? 14:18, Vinod Koul ??: > >> Why cant you do start (prepare clock etc) when you submit the descriptor >> to dmaengine. Can be done in tx_submit callback. >> Similarly remove the clock when dma transaction gets completed. > > I ever thought this method too. > > But it will become low efficient in the following case: > > Assuming the gpmi-nand driver has to read out 1024 pages in one _SINGLE_ > read operation. > The gpmi-nand will submit the descriptor to dmaengine per page. So with your > method, > the system will repeat the enable/disable dma clock 1024 time. At every > enable/disable dma clock, > the system has to enable the clock chain and it's parents ... > > But with this patch, we only need to enable/disable dma clock one time, just > at we select the nand chip. > If the clock is the dmac's property (not channels'), the toggling seems too aggressive. You could try using runtime_suspend/resume for clock disabling/enabling. How about employing autosuspend with a few ms delay?
Dear Huang Shijie, > ? 2012?10?18? 16:49, Marek Vasut ??: > > Dear Huang Shijie, > > > >> ? 2012?10?18? 16:16, Marek Vasut ??: > >>> So we can't stream data from the chip? About time to adjust the MTD > >>> framework to allow that. Maybe implement a command queue? > >> > >> IMHO, it's not possible. Because the READ-PAGE(00h-30h) command needs to > >> check the busy status > >> which means we have to stop in the middle, so we can not chain the all > >> the read-pages DMA commands. > > > > Can the DMA not branch? > > it's too complicated to the MTD layer, as well as the gpmi driver. Can you please elaborate ? Best regards, Marek Vasut
On Thu, Oct 18, 2012 at 6:51 AM, Marek Vasut <marex@denx.de> wrote: > Dear Huang Shijie, > >> ? 2012?10?18? 16:49, Marek Vasut ??: >> > Dear Huang Shijie, >> > >> >> ? 2012?10?18? 16:16, Marek Vasut ??: >> >>> So we can't stream data from the chip? About time to adjust the MTD >> >>> framework to allow that. Maybe implement a command queue? >> >> >> >> IMHO, it's not possible. Because the READ-PAGE(00h-30h) command needs to >> >> check the busy status >> >> which means we have to stop in the middle, so we can not chain the all >> >> the read-pages DMA commands. >> > >> > Can the DMA not branch? >> >> it's too complicated to the MTD layer, as well as the gpmi driver. > > Can you please elaborate ? could you read the nand spec about how to read a page out with the READ-PAGE(00-30) command, and tell me how to make the DMA branch? I really have no idea about how to make the DMA branch. thanks Huang Shijie . > Best regards, > Marek Vasut
On Thu, Oct 18, 2012 at 5:29 AM, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 18 October 2012 12:15, Huang Shijie <b32955@freescale.com> wrote: >> ? 2012?10?18? 14:18, Vinod Koul ??: >> >>> Why cant you do start (prepare clock etc) when you submit the descriptor >>> to dmaengine. Can be done in tx_submit callback. >>> Similarly remove the clock when dma transaction gets completed. >> >> I ever thought this method too. >> >> But it will become low efficient in the following case: >> >> Assuming the gpmi-nand driver has to read out 1024 pages in one _SINGLE_ >> read operation. >> The gpmi-nand will submit the descriptor to dmaengine per page. So with your >> method, >> the system will repeat the enable/disable dma clock 1024 time. At every >> enable/disable dma clock, >> the system has to enable the clock chain and it's parents ... >> >> But with this patch, we only need to enable/disable dma clock one time, just >> at we select the nand chip. >> > If the clock is the dmac's property (not channels'), the toggling > seems too aggressive. > You could try using runtime_suspend/resume for clock > disabling/enabling. How about employing autosuspend with a few ms > delay? Yes, employing the autosuspend is workable method too.o But it's a little more complicated then this patch. You have to create a thread or workqueue to do the job. What's more, I think other DMA engine may also meets the same issue as the mxs-dma, such as the imx-sdma. It's somehow a common issue to disable the clocks when no one use the DMA engine. Do you also suggest employing the autosuspend? Why not use a simple way to handle this issue? When the dma device want to use a DMA engine, it just needs to issue a DMA_START command. thanks Huang Shijie Huang Shijie
On 18 October 2012 20:48, Huang Shijie <shijie8@gmail.com> wrote: > On Thu, Oct 18, 2012 at 5:29 AM, Jassi Brar <jaswinder.singh@linaro.org> wrote: >> On 18 October 2012 12:15, Huang Shijie <b32955@freescale.com> wrote: >>> ? 2012?10?18? 14:18, Vinod Koul ??: >>> >>>> Why cant you do start (prepare clock etc) when you submit the descriptor >>>> to dmaengine. Can be done in tx_submit callback. >>>> Similarly remove the clock when dma transaction gets completed. >>> >>> I ever thought this method too. >>> >>> But it will become low efficient in the following case: >>> >>> Assuming the gpmi-nand driver has to read out 1024 pages in one _SINGLE_ >>> read operation. >>> The gpmi-nand will submit the descriptor to dmaengine per page. So with your >>> method, >>> the system will repeat the enable/disable dma clock 1024 time. At every >>> enable/disable dma clock, >>> the system has to enable the clock chain and it's parents ... >>> >>> But with this patch, we only need to enable/disable dma clock one time, just >>> at we select the nand chip. >>> >> If the clock is the dmac's property (not channels'), the toggling >> seems too aggressive. >> You could try using runtime_suspend/resume for clock >> disabling/enabling. How about employing autosuspend with a few ms >> delay? > > Yes, employing the autosuspend is workable method too.o > But it's a little more complicated then this patch. You have to create > a thread or workqueue to > do the job. > I am not sure why you would need to create any thread/workqueue ? Just pm_runtime_put_autosuspend() in drivers/ to see how it works.
On Thu, 2012-10-18 at 16:29 +0800, Huang Shijie wrote: > ? 2012?10?18? 16:16, Marek Vasut ??: > > So we can't stream data from the chip? About time to adjust the MTD framework to > > allow that. Maybe implement a command queue? > > > > to Artem & David: > is this possible to stream the data out with a command queue? Well, I think there was no HW which supported reading multiple pages with DMA before, are your sure yours support this? Usually reading a NAND page consists of 2 phases - loading the NAND array contents to a controller buffer, and then copying that to RAM. And the second part is something people sometimes do with DMA, AFAIU. If your HW supports this, sure the MTD framework can be re-worked. Or you may write your own 'mtd->_read()' / 'mtd->_write()' implementation.
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c index 9f02e79..89286f4 100644 --- a/drivers/dma/mxs-dma.c +++ b/drivers/dma/mxs-dma.c @@ -384,6 +384,8 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan) /* the descriptor is ready */ async_tx_ack(&mxs_chan->desc); + clk_disable_unprepare(mxs_dma->clk); + return 0; err_clk: @@ -399,6 +401,14 @@ static void mxs_dma_free_chan_resources(struct dma_chan *chan) { struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan); struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma; + int ret; + + ret = clk_prepare_enable(mxs_dma->clk); + if (ret) { + dev_err(mxs_dma->dma_device.dev, + "failed in enabling the dma clock\n"); + return; + } mxs_dma_disable_chan(mxs_chan); @@ -597,9 +607,13 @@ static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned long arg) { struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan); + struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma; int ret = 0; switch (cmd) { + case DMA_START: + ret = clk_prepare_enable(mxs_dma->clk); + break; case DMA_TERMINATE_ALL: mxs_dma_reset_chan(mxs_chan); mxs_dma_disable_chan(mxs_chan); @@ -610,6 +624,9 @@ static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, case DMA_RESUME: mxs_dma_resume_chan(mxs_chan); break; + case DMA_END: + clk_disable_unprepare(mxs_dma->clk); + break; default: ret = -ENOSYS; } diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c index 1f58197..da1e881 100644 --- a/drivers/i2c/busses/i2c-mxs.c +++ b/drivers/i2c/busses/i2c-mxs.c @@ -643,6 +643,12 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev) dev_err(dev, "Failed to request dma\n"); return -ENODEV; } + err = dmaengine_device_control(i2c->dmach, DMA_START, 0); + if (err) { + dma_release_channel(i2c->dmach); + dev_err(dev, "Failed to start dma\n"); + return err; + } } platform_set_drvdata(pdev, i2c); @@ -680,8 +686,10 @@ static int __devexit mxs_i2c_remove(struct platform_device *pdev) if (ret) return -EBUSY; - if (i2c->dmach) + if (i2c->dmach) { + dmaengine_device_control(i2c->dmach, DMA_END, 0); dma_release_channel(i2c->dmach); + } writel(MXS_I2C_CTRL0_SFTRST, i2c->regs + MXS_I2C_CTRL0_SET); diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c index 80d1e6d..aa91830 100644 --- a/drivers/mmc/host/mxs-mmc.c +++ b/drivers/mmc/host/mxs-mmc.c @@ -676,6 +676,9 @@ static int mxs_mmc_probe(struct platform_device *pdev) "%s: failed to request dma\n", __func__); goto out_clk_put; } + ret = dmaengine_device_control(ssp->dmach, DMA_START, 0); + if (ret) + goto out_free_dma; /* set mmc core parameters */ mmc->ops = &mxs_mmc_ops; @@ -717,18 +720,20 @@ static int mxs_mmc_probe(struct platform_device *pdev) ret = devm_request_irq(&pdev->dev, irq_err, mxs_mmc_irq_handler, 0, DRIVER_NAME, host); if (ret) - goto out_free_dma; + goto out_end_free_dma; spin_lock_init(&host->lock); ret = mmc_add_host(mmc); if (ret) - goto out_free_dma; + goto out_end_free_dma; dev_info(mmc_dev(host->mmc), "initialized\n"); return 0; +out_end_free_dma: + dmaengine_device_control(ssp->dmach, DMA_END, 0); out_free_dma: if (ssp->dmach) dma_release_channel(ssp->dmach); @@ -750,8 +755,10 @@ static int mxs_mmc_remove(struct platform_device *pdev) platform_set_drvdata(pdev, NULL); - if (ssp->dmach) + if (ssp->dmach) { + dmaengine_device_control(ssp->dmach, DMA_END, 0); dma_release_channel(ssp->dmach); + } clk_disable_unprepare(ssp->clk); clk_put(ssp->clk); @@ -772,6 +779,7 @@ static int mxs_mmc_suspend(struct device *dev) ret = mmc_suspend_host(mmc); clk_disable_unprepare(ssp->clk); + dmaengine_device_control(ssp->dmach, DMA_END, 0); return ret; } @@ -784,8 +792,13 @@ static int mxs_mmc_resume(struct device *dev) int ret = 0; clk_prepare_enable(ssp->clk); + ret = dmaengine_device_control(ssp->dmach, DMA_START, 0); + if (ret) + return ret; ret = mmc_resume_host(mmc); + if (ret) + dmaengine_device_control(ssp->dmach, DMA_END, 0); return ret; } diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c index 3502acc..20ed3f3 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c @@ -958,6 +958,7 @@ void gpmi_begin(struct gpmi_nand_data *this) uint32_t reg; unsigned int dll_wait_time_in_us; struct gpmi_nfc_hardware_timing hw; + struct dma_chan *channel = get_dma_chan(this); int ret; /* Enable the clock. */ @@ -967,6 +968,12 @@ void gpmi_begin(struct gpmi_nand_data *this) goto err_out; } + ret = dmaengine_device_control(channel, DMA_START, 0); + if (ret) { + gpmi_disable_clk(this); + goto err_out; + } + /* Only initialize the timing once */ if (this->flags & GPMI_TIMING_INIT_OK) return; @@ -1035,7 +1042,10 @@ err_out: void gpmi_end(struct gpmi_nand_data *this) { + struct dma_chan *channel = get_dma_chan(this); + gpmi_disable_clk(this); + dmaengine_device_control(channel, DMA_END, 0); } /* Clears a BCH interrupt. */ diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index e2c56fc..5694d03 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c @@ -815,12 +815,14 @@ static void gpmi_select_chip(struct mtd_info *mtd, int chipnr) struct nand_chip *chip = mtd->priv; struct gpmi_nand_data *this = chip->priv; - if ((this->current_chip < 0) && (chipnr >= 0)) + if ((this->current_chip < 0) && (chipnr >= 0)) { + /* set the current_chip before we call gpmi_begin(). */ + this->current_chip = chipnr; gpmi_begin(this); - else if ((this->current_chip >= 0) && (chipnr < 0)) + } else if ((this->current_chip >= 0) && (chipnr < 0)) { gpmi_end(this); - - this->current_chip = chipnr; + this->current_chip = chipnr; + } } static void gpmi_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index d3201e4..79f864a 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -199,6 +199,8 @@ enum dma_ctrl_flags { /** * enum dma_ctrl_cmd - DMA operations that can optionally be exercised * on a running channel. + * @DMA_START: do some preprations to start the DMA engine, such as enable the + * necessary clocks. * @DMA_TERMINATE_ALL: terminate all ongoing transfers * @DMA_PAUSE: pause ongoing transfers * @DMA_RESUME: resume paused transfer @@ -209,13 +211,17 @@ enum dma_ctrl_flags { * command. * @FSLDMA_EXTERNAL_START: this command will put the Freescale DMA controller * into external start mode. + * @DMA_END: do some works to end the DMA engine, such as disable the + * necessary clocks. */ enum dma_ctrl_cmd { + DMA_START, DMA_TERMINATE_ALL, DMA_PAUSE, DMA_RESUME, DMA_SLAVE_CONFIG, FSLDMA_EXTERNAL_START, + DMA_END, }; /** diff --git a/sound/soc/mxs/mxs-pcm.c b/sound/soc/mxs/mxs-pcm.c index f82d766..cfcc30f 100644 --- a/sound/soc/mxs/mxs-pcm.c +++ b/sound/soc/mxs/mxs-pcm.c @@ -92,6 +92,7 @@ static int snd_mxs_open(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct mxs_pcm_dma_data *pcm_dma_data; + struct dma_chan *chan; int ret; pcm_dma_data = kzalloc(sizeof(*pcm_dma_data), GFP_KERNEL); @@ -107,6 +108,14 @@ static int snd_mxs_open(struct snd_pcm_substream *substream) return ret; } + chan = snd_dmaengine_pcm_get_chan(substream); + ret = dmaengine_device_control(chan, DMA_START, (unsigned long)0); + if (ret) { + snd_dmaengine_pcm_close(substream); + kfree(pcm_dma_data); + return ret; + } + snd_soc_set_runtime_hwparams(substream, &snd_mxs_hardware); snd_dmaengine_pcm_set_data(substream, pcm_dma_data); @@ -117,7 +126,10 @@ static int snd_mxs_open(struct snd_pcm_substream *substream) static int snd_mxs_close(struct snd_pcm_substream *substream) { struct mxs_pcm_dma_data *pcm_dma_data = snd_dmaengine_pcm_get_data(substream); + struct dma_chan *chan; + chan = snd_dmaengine_pcm_get_chan(substream); + dmaengine_device_control(chan, DMA_END, 0); snd_dmaengine_pcm_close(substream); kfree(pcm_dma_data);