| Message ID | 1437469005-3162-1-git-send-email-yao.yuan@freescale.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Vinod, After our discuss, I have already send the v2. Could you please take some times help to review this patch again? Thanks. Best Regards, Yuan Yao > -----Original Message----- > From: linux-arm-kernel [mailto:linux-arm-kernel-bounces@lists.infradead.org] > On Behalf Of Yuan Yao > Sent: Tuesday, July 21, 2015 4:57 PM > To: vinod.koul@intel.com; stefan@agner.ch; arnd@arndb.de > Cc: dmaengine@vger.kernel.org; dan.j.williams@intel.com; linux- > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: [PATCH v2] dmaengine: fsl-edma: add PM suspend/resume support > > This add power management suspend/resume support for the fsl-edma driver. > > eDMA acted as a basic function used by others. What it needs to do is the two > steps below to support power management. > > In fsl_edma_suspend_late: > Check whether the DMA chan is idle and if it is not idle, stop PM operation. > > In fsl_edma_resume_early: > Enable the eDMA and wait for being used. > > Signed-off-by: Yuan Yao <yao.yuan@freescale.com> > --- > drivers/dma/fsl-edma.c | 83 > ++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 80 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma/fsl-edma.c b/drivers/dma/fsl-edma.c index > 915eec3..cf8b06c 100644 > --- a/drivers/dma/fsl-edma.c > +++ b/drivers/dma/fsl-edma.c > @@ -116,6 +116,10 @@ > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \ > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | \ > BIT(DMA_SLAVE_BUSWIDTH_8_BYTES) > +enum fsl_edma_pm_state { > + RUNNING = 0, > + SUSPENDED, > +}; > > struct fsl_edma_hw_tcd { > __le32 saddr; > @@ -147,6 +151,9 @@ struct fsl_edma_slave_config { struct fsl_edma_chan { > struct virt_dma_chan vchan; > enum dma_status status; > + enum fsl_edma_pm_state pm_state; > + bool idle; > + u32 slave_id; > struct fsl_edma_engine *edma; > struct fsl_edma_desc *edesc; > struct fsl_edma_slave_config fsc; > @@ -298,6 +305,7 @@ static int fsl_edma_terminate_all(struct dma_chan > *chan) > spin_lock_irqsave(&fsl_chan->vchan.lock, flags); > fsl_edma_disable_request(fsl_chan); > fsl_chan->edesc = NULL; > + fsl_chan->idle = true; > vchan_get_all_descriptors(&fsl_chan->vchan, &head); > spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); > vchan_dma_desc_free_list(&fsl_chan->vchan, &head); @@ -313,6 > +321,7 @@ static int fsl_edma_pause(struct dma_chan *chan) > if (fsl_chan->edesc) { > fsl_edma_disable_request(fsl_chan); > fsl_chan->status = DMA_PAUSED; > + fsl_chan->idle = true; > } > spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); > return 0; > @@ -327,6 +336,7 @@ static int fsl_edma_resume(struct dma_chan *chan) > if (fsl_chan->edesc) { > fsl_edma_enable_request(fsl_chan); > fsl_chan->status = DMA_IN_PROGRESS; > + fsl_chan->idle = false; > } > spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); > return 0; > @@ -648,6 +658,7 @@ static void fsl_edma_xfer_desc(struct fsl_edma_chan > *fsl_chan) > fsl_edma_set_tcd_regs(fsl_chan, fsl_chan->edesc->tcd[0].vtcd); > fsl_edma_enable_request(fsl_chan); > fsl_chan->status = DMA_IN_PROGRESS; > + fsl_chan->idle = false; > } > > static irqreturn_t fsl_edma_tx_handler(int irq, void *dev_id) @@ -676,6 > +687,7 @@ static irqreturn_t fsl_edma_tx_handler(int irq, void *dev_id) > vchan_cookie_complete(&fsl_chan->edesc- > >vdesc); > fsl_chan->edesc = NULL; > fsl_chan->status = DMA_COMPLETE; > + fsl_chan->idle = true; > } else { > vchan_cyclic_callback(&fsl_chan->edesc- > >vdesc); > } > @@ -704,6 +716,7 @@ static irqreturn_t fsl_edma_err_handler(int irq, void > *dev_id) > edma_writeb(fsl_edma, EDMA_CERR_CERR(ch), > fsl_edma->membase + EDMA_CERR); > fsl_edma->chans[ch].status = DMA_ERROR; > + fsl_edma->chans[ch].idle = true; > } > } > return IRQ_HANDLED; > @@ -724,6 +737,12 @@ static void fsl_edma_issue_pending(struct dma_chan > *chan) > > spin_lock_irqsave(&fsl_chan->vchan.lock, flags); > > + if (unlikely(fsl_chan->pm_state != RUNNING)) { > + spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); > + /* cannot submit due to suspend */ > + return; > + } > + > if (vchan_issue_pending(&fsl_chan->vchan) && !fsl_chan->edesc) > fsl_edma_xfer_desc(fsl_chan); > > @@ -735,6 +754,7 @@ static struct dma_chan *fsl_edma_xlate(struct > of_phandle_args *dma_spec, { > struct fsl_edma_engine *fsl_edma = ofdma->of_dma_data; > struct dma_chan *chan, *_chan; > + struct fsl_edma_chan *fsl_chan; > unsigned long chans_per_mux = fsl_edma->n_chans / DMAMUX_NR; > > if (dma_spec->args_count != 2) > @@ -748,8 +768,10 @@ static struct dma_chan *fsl_edma_xlate(struct > of_phandle_args *dma_spec, > chan = dma_get_slave_channel(chan); > if (chan) { > chan->device->privatecnt++; > - fsl_edma_chan_mux(to_fsl_edma_chan(chan), > - dma_spec->args[1], true); > + fsl_chan = to_fsl_edma_chan(chan); > + fsl_chan->slave_id = dma_spec->args[1]; > + fsl_edma_chan_mux(fsl_chan, fsl_chan- > >slave_id, > + true); > mutex_unlock(&fsl_edma->fsl_edma_mutex); > return chan; > } > @@ -888,7 +910,9 @@ static int fsl_edma_probe(struct platform_device *pdev) > struct fsl_edma_chan *fsl_chan = &fsl_edma->chans[i]; > > fsl_chan->edma = fsl_edma; > - > + fsl_chan->pm_state = RUNNING; > + fsl_chan->slave_id = 0; > + fsl_chan->idle = true; > fsl_chan->vchan.desc_free = fsl_edma_free_desc; > vchan_init(&fsl_chan->vchan, &fsl_edma->dma_dev); > > @@ -959,6 +983,58 @@ static int fsl_edma_remove(struct platform_device > *pdev) > return 0; > } > > +static int fsl_edma_suspend_late(struct device *dev) { > + struct fsl_edma_engine *fsl_edma = dev_get_drvdata(dev); > + struct fsl_edma_chan *fsl_chan; > + unsigned long flags; > + int i; > + > + for (i = 0; i < fsl_edma->n_chans; i++) { > + fsl_chan = &fsl_edma->chans[i]; > + spin_lock_irqsave(&fsl_chan->vchan.lock, flags); > + /* Make sure chan is idle or can not into suspend. */ > + if (unlikely(!fsl_chan->idle)) > + goto out; > + fsl_chan->pm_state = SUSPENDED; > + spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); > + } > + return 0; > + > +out: > + for (; i >= 0; i--) { > + fsl_chan = &fsl_edma->chans[i]; > + fsl_chan->pm_state = RUNNING; > + spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); > + } > + return -EBUSY; > +} > + > +static int fsl_edma_resume_early(struct device *dev) { > + struct fsl_edma_engine *fsl_edma = dev_get_drvdata(dev); > + struct fsl_edma_chan *fsl_chan; > + int i; > + > + for (i = 0; i < fsl_edma->n_chans; i++) { > + fsl_chan = &fsl_edma->chans[i]; > + fsl_chan->pm_state = RUNNING; > + edma_writew(fsl_edma, 0x0, fsl_edma->membase + > EDMA_TCD_CSR(i)); > + if (fsl_chan->slave_id != 0) > + fsl_edma_chan_mux(fsl_chan, fsl_chan->slave_id, > true); > + } > + > + edma_writel(fsl_edma, EDMA_CR_ERGA | EDMA_CR_ERCA, > + fsl_edma->membase + EDMA_CR); > + > + return 0; > +} > + > +static const struct dev_pm_ops fsl_edma_pm_ops = { > + .suspend_late = fsl_edma_suspend_late, > + .resume_early = fsl_edma_resume_early, > +}; > + > static const struct of_device_id fsl_edma_dt_ids[] = { > { .compatible = "fsl,vf610-edma", }, > { /* sentinel */ } > @@ -969,6 +1045,7 @@ static struct platform_driver fsl_edma_driver = { > .driver = { > .name = "fsl-edma", > .of_match_table = fsl_edma_dt_ids, > + .pm = &fsl_edma_pm_ops, > }, > .probe = fsl_edma_probe, > .remove = fsl_edma_remove, > -- > 2.1.0.27.g96db324 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 21, 2015 at 3:56 AM, Yuan Yao <yao.yuan@freescale.com> wrote: > This add power management suspend/resume support for the fsl-edma > driver. > > eDMA acted as a basic function used by others. What it needs to do is > the two steps below to support power management. > > In fsl_edma_suspend_late: > Check whether the DMA chan is idle and if it is not idle, stop PM > operation. You should try to quiesce the device on suspend instead of depending on itself to be happen in idle and failing if it is not. Regards, Leo -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Leo, Thanks for your review. About those two methods for DMA suspend that you have mentioned. We have a lot of the discussions in other DMA driver like DMA for Freescale PowerPC. Finally, we think the device which used the DMA transmission service should cancel the transmission service in its suspend. So DMA in suspend should be idle. Once the DMA in late_suspend is not be idle, we think some driver haven't canceled the DMA transmission. So maybe something is error when other driver in suspend. In the case, we should return failed to stop PM. DMA should not make a choice for other drivers(which used DMA) to force stop DMA transmission. Thanks. Best Regards, Yuan Yao > -----Original Message----- > From: pku.leo@gmail.com [mailto:pku.leo@gmail.com] On Behalf Of Li Yang > Sent: Friday, August 14, 2015 4:58 AM > To: Yuan Yao-B46683 > Cc: Vinod Koul; stefan@agner.ch; Arnd Bergmann; Dan Williams; > dmaengine@vger.kernel.org; lkml; linux-arm-kernel@lists.infradead.org; linux- > pm@vger.kernel.org > Subject: Re: [PATCH v2] dmaengine: fsl-edma: add PM suspend/resume > support > > On Tue, Jul 21, 2015 at 3:56 AM, Yuan Yao <yao.yuan@freescale.com> wrote: > > This add power management suspend/resume support for the fsl-edma > > driver. > > > > eDMA acted as a basic function used by others. What it needs to do is > > the two steps below to support power management. > > > > In fsl_edma_suspend_late: > > Check whether the DMA chan is idle and if it is not idle, stop PM > > operation. > > You should try to quiesce the device on suspend instead of depending on itself > to be happen in idle and failing if it is not. > > Regards, > Leo
On Fri, Aug 14, 2015 at 1:24 AM, Yao Yuan <yao.yuan@freescale.com> wrote: > Hi Leo, > > Thanks for your review. > About those two methods for DMA suspend that you have mentioned. We have a lot of the discussions in other DMA driver like DMA for Freescale PowerPC. > > Finally, we think the device which used the DMA transmission service should cancel the transmission service in its suspend. > So DMA in suspend should be idle. If that's the case you should clearly state this in the commit message and in code, although I don't know if it is safe to make such assumption. There could be user of the DMA that doesn't track the completion of transfers. > > Once the DMA in late_suspend is not be idle, we think some driver haven't canceled the DMA transmission. So maybe something is error when other driver in suspend. > > In the case, we should return failed to stop PM. DMA should not make a choice for other drivers(which used DMA) to force stop DMA transmission. The suspend entrance should be terminated by wakeup events and only critical issues. I don't think we should just terminate the suspend entrance just because having on-going I/O without even try to stop it. > > Thanks. > > Best Regards, > Yuan Yao > >> -----Original Message----- >> From: pku.leo@gmail.com [mailto:pku.leo@gmail.com] On Behalf Of Li Yang >> Sent: Friday, August 14, 2015 4:58 AM >> To: Yuan Yao-B46683 >> Cc: Vinod Koul; stefan@agner.ch; Arnd Bergmann; Dan Williams; >> dmaengine@vger.kernel.org; lkml; linux-arm-kernel@lists.infradead.org; linux- >> pm@vger.kernel.org >> Subject: Re: [PATCH v2] dmaengine: fsl-edma: add PM suspend/resume >> support >> >> On Tue, Jul 21, 2015 at 3:56 AM, Yuan Yao <yao.yuan@freescale.com> wrote: >> > This add power management suspend/resume support for the fsl-edma >> > driver. >> > >> > eDMA acted as a basic function used by others. What it needs to do is >> > the two steps below to support power management. >> > >> > In fsl_edma_suspend_late: >> > Check whether the DMA chan is idle and if it is not idle, stop PM >> > operation. >> >> You should try to quiesce the device on suspend instead of depending on itself >> to be happen in idle and failing if it is not. >> >> Regards, >> Leo
On Sat, Aug 15, 2015 at 7:48 AM, pku.leo < pku.leo@gmail.com > wrote: > On Fri, Aug 14, 2015 at 1:24 AM, Yao Yuan <yao.yuan@freescale.com> wrote: > > Hi Leo, > > > > Thanks for your review. > > About those two methods for DMA suspend that you have mentioned. We > have a lot of the discussions in other DMA driver like DMA for Freescale > PowerPC. > > > > Finally, we think the device which used the DMA transmission service should > cancel the transmission service in its suspend. > > So DMA in suspend should be idle. > > If that's the case you should clearly state this in the commit message and in > code, although I don't know if it is safe to make such assumption. There could > be user of the DMA that doesn't track the completion of transfers. I think it should be safe. In my opinion, even some client(the user of the DMA) forget to cancel its DMA transmission, It will just lead to PM failed but no other system and data risk. Although we should first fix the behavior of the client. Once you are no need the DMA transmission, why not stop it? Is it right? > > > > Once the DMA in late_suspend is not be idle, we think some driver haven't > canceled the DMA transmission. So maybe something is error when other > driver in suspend. > > > > In the case, we should return failed to stop PM. DMA should not make a > choice for other drivers(which used DMA) to force stop DMA transmission. > > The suspend entrance should be terminated by wakeup events and only critical > issues. I don't think we should just terminate the suspend entrance just > because having on-going I/O without even try to stop it. The graceful behavior would be to for client to PAUSE or terminate and then suspend followed by DMA suspend. We need to rely on client doing the right thing here. The DMA should not make a decision instead of client. If the DMA is not idle in DMA suspend, it should be the client's issue. We don't know what the client really want to do, so just return the non-success value. I'm not sure my description is clear. So we may refer the discussion about the DMA PM support before. Such as "DMA: Freescale: add suspend resume functions for DMA driver" Like:https://lkml.org/lkml/2014/5/21/1 > > > > Thanks. > > > > Best Regards, > > Yuan Yao > > > >> -----Original Message----- > >> From: pku.leo@gmail.com [mailto:pku.leo@gmail.com] On Behalf Of Li > >> Yang > >> Sent: Friday, August 14, 2015 4:58 AM > >> To: Yuan Yao-B46683 > >> Cc: Vinod Koul; stefan@agner.ch; Arnd Bergmann; Dan Williams; > >> dmaengine@vger.kernel.org; lkml; > >> linux-arm-kernel@lists.infradead.org; linux- pm@vger.kernel.org > >> Subject: Re: [PATCH v2] dmaengine: fsl-edma: add PM suspend/resume > >> support > >> > >> On Tue, Jul 21, 2015 at 3:56 AM, Yuan Yao <yao.yuan@freescale.com> wrote: > >> > This add power management suspend/resume support for the fsl-edma > >> > driver. > >> > > >> > eDMA acted as a basic function used by others. What it needs to do > >> > is the two steps below to support power management. > >> > > >> > In fsl_edma_suspend_late: > >> > Check whether the DMA chan is idle and if it is not idle, stop PM > >> > operation. > >> > >> You should try to quiesce the device on suspend instead of depending > >> on itself to be happen in idle and failing if it is not. > >> > >> Regards, > >> Leo > > > > -- > - Leo
Hi Yao. On 17/08/15 13:59, Yao Yuan wrote: > On Sat, Aug 15, 2015 at 7:48 AM, pku.leo < pku.leo@gmail.com > wrote: >> On Fri, Aug 14, 2015 at 1:24 AM, Yao Yuan <yao.yuan@freescale.com> wrote: >>> Hi Leo, >>> >>> Thanks for your review. >>> About those two methods for DMA suspend that you have mentioned. We >> have a lot of the discussions in other DMA driver like DMA for Freescale >> PowerPC. >>> Finally, we think the device which used the DMA transmission service should >> cancel the transmission service in its suspend. >>> So DMA in suspend should be idle. >> If that's the case you should clearly state this in the commit message and in >> code, although I don't know if it is safe to make such assumption. There could >> be user of the DMA that doesn't track the completion of transfers. > I think it should be safe. In my opinion, even some client(the user of the DMA) forget to cancel its DMA transmission, > It will just lead to PM failed but no other system and data risk. > Although we should first fix the behavior of the client. > Once you are no need the DMA transmission, why not stop it? > > Is it right? Think of it from the end user perspective. Would you like your laptop (or whatever) to refuse to suspend because of this condition? The user may well expect that closing the lid on their laptop will reliably lead to it suspending to ram. Returning a failure here could result in a loss of data if the condition is not detected and the machine subsequently runs out of power. I do agree that whatever is submitting DMA should be stopped first; ideally this driver would always be idle because whatever producers of work exist would already have been quiesced and output flushed. Regards, Nigel -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Nigel, On Mon, Aug 17, 2015 at 2:49 PM, Nigel Cunningham < nigel@nigelcunningham.com.au > wrote: > On 17/08/15 13:59, Yao Yuan wrote: > > On Sat, Aug 15, 2015 at 7:48 AM, pku.leo < pku.leo@gmail.com > wrote: > >> On Fri, Aug 14, 2015 at 1:24 AM, Yao Yuan <yao.yuan@freescale.com> wrote: > >>> Hi Leo, > >>> > >>> Thanks for your review. > >>> About those two methods for DMA suspend that you have mentioned. > We > >> have a lot of the discussions in other DMA driver like DMA for > >> Freescale PowerPC. > >>> Finally, we think the device which used the DMA transmission service > >>> should > >> cancel the transmission service in its suspend. > >>> So DMA in suspend should be idle. > >> If that's the case you should clearly state this in the commit > >> message and in code, although I don't know if it is safe to make such > >> assumption. There could be user of the DMA that doesn't track the > completion of transfers. > > I think it should be safe. In my opinion, even some client(the user of > > the DMA) forget to cancel its DMA transmission, It will just lead to PM failed > but no other system and data risk. > > Although we should first fix the behavior of the client. > > Once you are no need the DMA transmission, why not stop it? > > > > Is it right? > Think of it from the end user perspective. Would you like your laptop (or > whatever) to refuse to suspend because of this condition? The user may well > expect that closing the lid on their laptop will reliably lead to it suspending to > ram. Returning a failure here could result in a loss of data if the condition is not > detected and the machine subsequently runs out of power. > Yes, the user may well expect that closing the lid on their laptop will reliably lead to it suspending to ram. So the client(the user of the DMA) must to PAUSE or terminate the DMA transmission. We need to rely on client doing the right thing here. The DMA should not make a decision instead of client. If the DMA is not idle in DMA suspend, it should be the client's issue. We don't know what the client really want to do, so just return the non-success value. > I do agree that whatever is submitting DMA should be stopped first; ideally this > driver would always be idle because whatever producers of work exist would > already have been quiesced and output flushed. >
On Mon, Aug 17, 2015 at 2:22 AM, Yao Yuan <yao.yuan@freescale.com> wrote: > Hi Nigel, > > On Mon, Aug 17, 2015 at 2:49 PM, Nigel Cunningham < nigel@nigelcunningham.com.au > wrote: >> On 17/08/15 13:59, Yao Yuan wrote: >> > On Sat, Aug 15, 2015 at 7:48 AM, pku.leo < pku.leo@gmail.com > wrote: >> >> On Fri, Aug 14, 2015 at 1:24 AM, Yao Yuan <yao.yuan@freescale.com> wrote: >> >>> Hi Leo, >> >>> >> >>> Thanks for your review. >> >>> About those two methods for DMA suspend that you have mentioned. >> We >> >> have a lot of the discussions in other DMA driver like DMA for >> >> Freescale PowerPC. >> >>> Finally, we think the device which used the DMA transmission service >> >>> should >> >> cancel the transmission service in its suspend. >> >>> So DMA in suspend should be idle. >> >> If that's the case you should clearly state this in the commit >> >> message and in code, although I don't know if it is safe to make such >> >> assumption. There could be user of the DMA that doesn't track the >> completion of transfers. >> > I think it should be safe. In my opinion, even some client(the user of >> > the DMA) forget to cancel its DMA transmission, It will just lead to PM failed >> but no other system and data risk. >> > Although we should first fix the behavior of the client. >> > Once you are no need the DMA transmission, why not stop it? >> > >> > Is it right? >> Think of it from the end user perspective. Would you like your laptop (or >> whatever) to refuse to suspend because of this condition? The user may well >> expect that closing the lid on their laptop will reliably lead to it suspending to >> ram. Returning a failure here could result in a loss of data if the condition is not >> detected and the machine subsequently runs out of power. >> > > Yes, the user may well expect that closing the lid on their laptop will reliably lead to it suspending to ram. > So the client(the user of the DMA) must to PAUSE or terminate the DMA transmission. > > We need to rely on client doing the right thing here. > The DMA should not make a decision instead of client. > If the DMA is not idle in DMA suspend, it should be the client's issue. > We don't know what the client really want to do, so just return the non-success value. The problem here is that neither the client nor the DMA controller driver should easily decide to stop the suspend entrance and rollback. I don't think the non-idle situation is serious enough to cause a rollback. You should do whatever can be done with the DMA controller(such as stop the controller and leave whatever to be done to the wake up) and continue with the suspend. Regards, Leo -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 17, 2015 at 02:10:46PM -0500, Li Yang wrote: > >> Think of it from the end user perspective. Would you like your laptop (or > >> whatever) to refuse to suspend because of this condition? The user may well > >> expect that closing the lid on their laptop will reliably lead to it suspending to > >> ram. Returning a failure here could result in a loss of data if the condition is not > >> detected and the machine subsequently runs out of power. > >> > > > > Yes, the user may well expect that closing the lid on their laptop will reliably lead to it suspending to ram. > > So the client(the user of the DMA) must to PAUSE or terminate the DMA transmission. > > > > We need to rely on client doing the right thing here. > > The DMA should not make a decision instead of client. > > If the DMA is not idle in DMA suspend, it should be the client's issue. > > We don't know what the client really want to do, so just return the non-success value. > > The problem here is that neither the client nor the DMA controller > driver should easily decide to stop the suspend entrance and rollback. > I don't think the non-idle situation is serious enough to cause a > rollback. You should do whatever can be done with the DMA > controller(such as stop the controller and leave whatever to be done > to the wake up) and continue with the suspend. Ideally yes client should suspend first and dmaengine driver then being idle when suspend is invoked. But we know we are in idle world! So, driver should ensure it suspends the active channels and then goes to suspend and restores that on resume
On Thu, Aug 20, 2015 at 12:08:00PM +0800, Vinod Koul wrote: > On Mon, Aug 17, 2015 at 02:10:46PM -0500, Li Yang wrote: > > >> Think of it from the end user perspective. Would you like your > > >> laptop (or > > >> whatever) to refuse to suspend because of this condition? The user > > >> may well expect that closing the lid on their laptop will reliably > > >> lead to it suspending to ram. Returning a failure here could result > > >> in a loss of data if the condition is not detected and the machine > subsequently runs out of power. > > >> > > > > > > Yes, the user may well expect that closing the lid on their laptop will reliably > lead to it suspending to ram. > > > So the client(the user of the DMA) must to PAUSE or terminate the DMA > transmission. > > > > > > We need to rely on client doing the right thing here. > > > The DMA should not make a decision instead of client. > > > If the DMA is not idle in DMA suspend, it should be the client's issue. > > > We don't know what the client really want to do, so just return the non- > success value. > > > > The problem here is that neither the client nor the DMA controller > > driver should easily decide to stop the suspend entrance and rollback. > > I don't think the non-idle situation is serious enough to cause a > > rollback. You should do whatever can be done with the DMA > > controller(such as stop the controller and leave whatever to be done > > to the wake up) and continue with the suspend. > > Ideally yes client should suspend first and dmaengine driver then being idle > when suspend is invoked. But we know we are in idle world! > So, driver should ensure it suspends the active channels and then goes to > suspend and restores that on resume > Hi Vinod, Hi Leo, Is there any other discussions? I think we can have the following solutions for DMA driver: 1, Force terminate the active channels in its suspend and then return. 2, DMA have to wait until the active channels idle. 3, Don't care about the active channels and direct return. 4, Return BUS BUSY error and stop PM progress. I prefer the last one, because I think client should make sure the channel is idle. But also the first one should be works. Can we have a conclusion and suggestion for which one is better? Thanks. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Thu, Aug 20, 2015 at 12:08:00PM +0800, Vinod Koul wrote: > > On Mon, Aug 17, 2015 at 02:10:46PM -0500, Li Yang wrote: > > > >> Think of it from the end user perspective. Would you like your > > > >> laptop (or > > > >> whatever) to refuse to suspend because of this condition? The > > > >> user may well expect that closing the lid on their laptop will > > > >> reliably lead to it suspending to ram. Returning a failure here > > > >> could result in a loss of data if the condition is not detected > > > >> and the machine > > subsequently runs out of power. > > > >> > > > > > > > > Yes, the user may well expect that closing the lid on their laptop > > > > will reliably > > lead to it suspending to ram. > > > > So the client(the user of the DMA) must to PAUSE or terminate the > > > > DMA > > transmission. > > > > > > > > We need to rely on client doing the right thing here. > > > > The DMA should not make a decision instead of client. > > > > If the DMA is not idle in DMA suspend, it should be the client's issue. > > > > We don't know what the client really want to do, so just return > > > > the non- > > success value. > > > > > > The problem here is that neither the client nor the DMA controller > > > driver should easily decide to stop the suspend entrance and rollback. > > > I don't think the non-idle situation is serious enough to cause a > > > rollback. You should do whatever can be done with the DMA > > > controller(such as stop the controller and leave whatever to be done > > > to the wake up) and continue with the suspend. > > > > Ideally yes client should suspend first and dmaengine driver then > > being idle when suspend is invoked. But we know we are in idle world! > > So, driver should ensure it suspends the active channels and then goes > > to suspend and restores that on resume > > > > Hi Vinod, > Hi Leo, > > Is there any other discussions? > > I think we can have the following solutions for DMA driver: > 1, Force terminate the active channels in its suspend and then return. > 2, DMA have to wait until the active channels idle. > 3, Don't care about the active channels and direct return. > 4, Return BUS BUSY error and stop PM progress. > > > I prefer the last one, because I think client should make sure the channel is idle. > > But also the first one should be works. I agree that client should be responsible for cleaning up the pending requests. My concern is that we shouldn't be triggering rewind by returning error if any client failed to do so. I would suggest to complain about it with a message and continue to do whatever we can do at the DMA driver level and continue with the suspend. So I would prefer the solution one. Regards, Leo -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/fsl-edma.c b/drivers/dma/fsl-edma.c index 915eec3..cf8b06c 100644 --- a/drivers/dma/fsl-edma.c +++ b/drivers/dma/fsl-edma.c @@ -116,6 +116,10 @@ BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \ BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | \ BIT(DMA_SLAVE_BUSWIDTH_8_BYTES) +enum fsl_edma_pm_state { + RUNNING = 0, + SUSPENDED, +}; struct fsl_edma_hw_tcd { __le32 saddr; @@ -147,6 +151,9 @@ struct fsl_edma_slave_config { struct fsl_edma_chan { struct virt_dma_chan vchan; enum dma_status status; + enum fsl_edma_pm_state pm_state; + bool idle; + u32 slave_id; struct fsl_edma_engine *edma; struct fsl_edma_desc *edesc; struct fsl_edma_slave_config fsc; @@ -298,6 +305,7 @@ static int fsl_edma_terminate_all(struct dma_chan *chan) spin_lock_irqsave(&fsl_chan->vchan.lock, flags); fsl_edma_disable_request(fsl_chan); fsl_chan->edesc = NULL; + fsl_chan->idle = true; vchan_get_all_descriptors(&fsl_chan->vchan, &head); spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); vchan_dma_desc_free_list(&fsl_chan->vchan, &head); @@ -313,6 +321,7 @@ static int fsl_edma_pause(struct dma_chan *chan) if (fsl_chan->edesc) { fsl_edma_disable_request(fsl_chan); fsl_chan->status = DMA_PAUSED; + fsl_chan->idle = true; } spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); return 0; @@ -327,6 +336,7 @@ static int fsl_edma_resume(struct dma_chan *chan) if (fsl_chan->edesc) { fsl_edma_enable_request(fsl_chan); fsl_chan->status = DMA_IN_PROGRESS; + fsl_chan->idle = false; } spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); return 0; @@ -648,6 +658,7 @@ static void fsl_edma_xfer_desc(struct fsl_edma_chan *fsl_chan) fsl_edma_set_tcd_regs(fsl_chan, fsl_chan->edesc->tcd[0].vtcd); fsl_edma_enable_request(fsl_chan); fsl_chan->status = DMA_IN_PROGRESS; + fsl_chan->idle = false; } static irqreturn_t fsl_edma_tx_handler(int irq, void *dev_id) @@ -676,6 +687,7 @@ static irqreturn_t fsl_edma_tx_handler(int irq, void *dev_id) vchan_cookie_complete(&fsl_chan->edesc->vdesc); fsl_chan->edesc = NULL; fsl_chan->status = DMA_COMPLETE; + fsl_chan->idle = true; } else { vchan_cyclic_callback(&fsl_chan->edesc->vdesc); } @@ -704,6 +716,7 @@ static irqreturn_t fsl_edma_err_handler(int irq, void *dev_id) edma_writeb(fsl_edma, EDMA_CERR_CERR(ch), fsl_edma->membase + EDMA_CERR); fsl_edma->chans[ch].status = DMA_ERROR; + fsl_edma->chans[ch].idle = true; } } return IRQ_HANDLED; @@ -724,6 +737,12 @@ static void fsl_edma_issue_pending(struct dma_chan *chan) spin_lock_irqsave(&fsl_chan->vchan.lock, flags); + if (unlikely(fsl_chan->pm_state != RUNNING)) { + spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); + /* cannot submit due to suspend */ + return; + } + if (vchan_issue_pending(&fsl_chan->vchan) && !fsl_chan->edesc) fsl_edma_xfer_desc(fsl_chan); @@ -735,6 +754,7 @@ static struct dma_chan *fsl_edma_xlate(struct of_phandle_args *dma_spec, { struct fsl_edma_engine *fsl_edma = ofdma->of_dma_data; struct dma_chan *chan, *_chan; + struct fsl_edma_chan *fsl_chan; unsigned long chans_per_mux = fsl_edma->n_chans / DMAMUX_NR; if (dma_spec->args_count != 2) @@ -748,8 +768,10 @@ static struct dma_chan *fsl_edma_xlate(struct of_phandle_args *dma_spec, chan = dma_get_slave_channel(chan); if (chan) { chan->device->privatecnt++; - fsl_edma_chan_mux(to_fsl_edma_chan(chan), - dma_spec->args[1], true); + fsl_chan = to_fsl_edma_chan(chan); + fsl_chan->slave_id = dma_spec->args[1]; + fsl_edma_chan_mux(fsl_chan, fsl_chan->slave_id, + true); mutex_unlock(&fsl_edma->fsl_edma_mutex); return chan; } @@ -888,7 +910,9 @@ static int fsl_edma_probe(struct platform_device *pdev) struct fsl_edma_chan *fsl_chan = &fsl_edma->chans[i]; fsl_chan->edma = fsl_edma; - + fsl_chan->pm_state = RUNNING; + fsl_chan->slave_id = 0; + fsl_chan->idle = true; fsl_chan->vchan.desc_free = fsl_edma_free_desc; vchan_init(&fsl_chan->vchan, &fsl_edma->dma_dev); @@ -959,6 +983,58 @@ static int fsl_edma_remove(struct platform_device *pdev) return 0; } +static int fsl_edma_suspend_late(struct device *dev) +{ + struct fsl_edma_engine *fsl_edma = dev_get_drvdata(dev); + struct fsl_edma_chan *fsl_chan; + unsigned long flags; + int i; + + for (i = 0; i < fsl_edma->n_chans; i++) { + fsl_chan = &fsl_edma->chans[i]; + spin_lock_irqsave(&fsl_chan->vchan.lock, flags); + /* Make sure chan is idle or can not into suspend. */ + if (unlikely(!fsl_chan->idle)) + goto out; + fsl_chan->pm_state = SUSPENDED; + spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); + } + return 0; + +out: + for (; i >= 0; i--) { + fsl_chan = &fsl_edma->chans[i]; + fsl_chan->pm_state = RUNNING; + spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags); + } + return -EBUSY; +} + +static int fsl_edma_resume_early(struct device *dev) +{ + struct fsl_edma_engine *fsl_edma = dev_get_drvdata(dev); + struct fsl_edma_chan *fsl_chan; + int i; + + for (i = 0; i < fsl_edma->n_chans; i++) { + fsl_chan = &fsl_edma->chans[i]; + fsl_chan->pm_state = RUNNING; + edma_writew(fsl_edma, 0x0, fsl_edma->membase + EDMA_TCD_CSR(i)); + if (fsl_chan->slave_id != 0) + fsl_edma_chan_mux(fsl_chan, fsl_chan->slave_id, true); + } + + edma_writel(fsl_edma, EDMA_CR_ERGA | EDMA_CR_ERCA, + fsl_edma->membase + EDMA_CR); + + return 0; +} + +static const struct dev_pm_ops fsl_edma_pm_ops = { + .suspend_late = fsl_edma_suspend_late, + .resume_early = fsl_edma_resume_early, +}; + static const struct of_device_id fsl_edma_dt_ids[] = { { .compatible = "fsl,vf610-edma", }, { /* sentinel */ } @@ -969,6 +1045,7 @@ static struct platform_driver fsl_edma_driver = { .driver = { .name = "fsl-edma", .of_match_table = fsl_edma_dt_ids, + .pm = &fsl_edma_pm_ops, }, .probe = fsl_edma_probe, .remove = fsl_edma_remove,
This add power management suspend/resume support for the fsl-edma driver. eDMA acted as a basic function used by others. What it needs to do is the two steps below to support power management. In fsl_edma_suspend_late: Check whether the DMA chan is idle and if it is not idle, stop PM operation. In fsl_edma_resume_early: Enable the eDMA and wait for being used. Signed-off-by: Yuan Yao <yao.yuan@freescale.com> --- drivers/dma/fsl-edma.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 3 deletions(-)