| Message ID | 1397809071-5353-9-git-send-email-hongbo.zhang@freescale.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Vinod Koul |
| Headers | show |
On Fri, Apr 18, 2014 at 04:17:51PM +0800, hongbo.zhang@freescale.com wrote: > From: Hongbo Zhang <hongbo.zhang@freescale.com> > > This patch adds suspend resume functions for Freescale DMA driver. > .prepare callback is used to stop further descriptors from being added into the > pending queue, and also issue pending queues into execution if there is any. > .suspend callback makes sure all the pending jobs are cleaned up and all the > channels are idle, and save the mode registers. > .resume callback re-initializes the channels by restore the mode registers. > > + > +static const struct dev_pm_ops fsldma_pm_ops = { > + .prepare = fsldma_prepare, > + .suspend = fsldma_suspend, > + .resume = fsldma_resume, > +}; I think this is not correct. We discussed this sometime back on list. The DMAengine drivers should use late resume and early suspend to ensure they get suspended after clients (who should use normal ones) and resume before them
On 05/03/2014 12:46 AM, Vinod Koul wrote: > On Fri, Apr 18, 2014 at 04:17:51PM +0800, hongbo.zhang@freescale.com wrote: >> From: Hongbo Zhang <hongbo.zhang@freescale.com> >> >> This patch adds suspend resume functions for Freescale DMA driver. >> .prepare callback is used to stop further descriptors from being added into the >> pending queue, and also issue pending queues into execution if there is any. >> .suspend callback makes sure all the pending jobs are cleaned up and all the >> channels are idle, and save the mode registers. >> .resume callback re-initializes the channels by restore the mode registers. >> >> + >> +static const struct dev_pm_ops fsldma_pm_ops = { >> + .prepare = fsldma_prepare, >> + .suspend = fsldma_suspend, >> + .resume = fsldma_resume, >> +}; > I think this is not correct. We discussed this sometime back on list. The > DMAengine drivers should use late resume and early suspend to ensure they get > suspended after clients (who should use normal ones) and resume before them > OK, will update it like this: use .suspend to take place of current .prepare use .suspend_late to take place of current .suspend use .resume_early to take place of current .resume -- 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
T24gU3VuLCAyMDE0LTA1LTA0IGF0IDE4OjIyICswODAwLCBIb25nYm8gWmhhbmcgd3JvdGU6DQo+ IE9uIDA1LzAzLzIwMTQgMTI6NDYgQU0sIFZpbm9kIEtvdWwgd3JvdGU6DQo+ID4gT24gRnJpLCBB cHIgMTgsIDIwMTQgYXQgMDQ6MTc6NTFQTSArMDgwMCwgaG9uZ2JvLnpoYW5nQGZyZWVzY2FsZS5j b20gd3JvdGU6DQo+ID4+IEZyb206IEhvbmdibyBaaGFuZyA8aG9uZ2JvLnpoYW5nQGZyZWVzY2Fs ZS5jb20+DQo+ID4+DQo+ID4+IFRoaXMgcGF0Y2ggYWRkcyBzdXNwZW5kIHJlc3VtZSBmdW5jdGlv bnMgZm9yIEZyZWVzY2FsZSBETUEgZHJpdmVyLg0KPiA+PiAucHJlcGFyZSBjYWxsYmFjayBpcyB1 c2VkIHRvIHN0b3AgZnVydGhlciBkZXNjcmlwdG9ycyBmcm9tIGJlaW5nIGFkZGVkIGludG8gdGhl DQo+ID4+IHBlbmRpbmcgcXVldWUsIGFuZCBhbHNvIGlzc3VlIHBlbmRpbmcgcXVldWVzIGludG8g ZXhlY3V0aW9uIGlmIHRoZXJlIGlzIGFueS4NCj4gPj4gLnN1c3BlbmQgY2FsbGJhY2sgbWFrZXMg c3VyZSBhbGwgdGhlIHBlbmRpbmcgam9icyBhcmUgY2xlYW5lZCB1cCBhbmQgYWxsIHRoZQ0KPiA+ PiBjaGFubmVscyBhcmUgaWRsZSwgYW5kIHNhdmUgdGhlIG1vZGUgcmVnaXN0ZXJzLg0KPiA+PiAu cmVzdW1lIGNhbGxiYWNrIHJlLWluaXRpYWxpemVzIHRoZSBjaGFubmVscyBieSByZXN0b3JlIHRo ZSBtb2RlIHJlZ2lzdGVycy4NCj4gPj4NCj4gPj4gKw0KPiA+PiArc3RhdGljIGNvbnN0IHN0cnVj dCBkZXZfcG1fb3BzIGZzbGRtYV9wbV9vcHMgPSB7DQo+ID4+ICsJLnByZXBhcmUJPSBmc2xkbWFf cHJlcGFyZSwNCj4gPj4gKwkuc3VzcGVuZAk9IGZzbGRtYV9zdXNwZW5kLA0KPiA+PiArCS5yZXN1 bWUJCT0gZnNsZG1hX3Jlc3VtZSwNCj4gPj4gK307DQo+ID4gSSB0aGluayB0aGlzIGlzIG5vdCBj b3JyZWN0LiBXZSBkaXNjdXNzZWQgdGhpcyBzb21ldGltZSBiYWNrIG9uIGxpc3QuIFRoZQ0KPiA+ IERNQWVuZ2luZSBkcml2ZXJzIHNob3VsZCB1c2UgbGF0ZSByZXN1bWUgYW5kIGVhcmx5IHN1c3Bl bmQgdG8gZW5zdXJlIHRoZXkgZ2V0DQo+ID4gc3VzcGVuZGVkIGFmdGVyIGNsaWVudHMgKHdobyBz aG91bGQgdXNlIG5vcm1hbCBvbmVzKSBhbmQgcmVzdW1lIGJlZm9yZSB0aGVtDQo+ID4NCj4gT0ss IHdpbGwgdXBkYXRlIGl0IGxpa2UgdGhpczoNCj4gdXNlIC5zdXNwZW5kIHRvIHRha2UgcGxhY2Ug b2YgY3VycmVudCAucHJlcGFyZQ0KDQpDb3VsZCB5b3UgcmVtb3ZlIHRoaXMgYXQgYWxsPw0KDQpB bnN3ZXJpbmcgdG8geW91ciBwcmV2aW91cyBzdGF0ZW1lbnRzIEkgY291bGQgc2F5IHRoYXQuDQpE ZXZpY2UgZHJpdmVycyAoRE1BYyB1c2VycykgdGhhdCBkb24ndCBpbXBsZW1lbnQgLnN1c3BlbmQg Y2FsbGJhY2sgYXJlDQpvbiB0aGVpciBvd24gd2l0aCB0cm91YmxlcywgeW91IGhhdmUgbm90IHRv IGNhcmUgYWJvdXQgdGhlbSBpbiB0aGUgRE1BDQpkcml2ZXIuDQoNCj4gdXNlIC5zdXNwZW5kX2xh dGUgdG8gdGFrZSBwbGFjZSBvZiBjdXJyZW50IC5zdXNwZW5kDQo+IHVzZSAucmVzdW1lX2Vhcmx5 IHRvIHRha2UgcGxhY2Ugb2YgY3VycmVudCAucmVzdW1lDQo+IA0KPiANCj4gDQo+IC0tDQo+IFRv IHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBk bWFlbmdpbmUiIGluDQo+IHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5r ZXJuZWwub3JnDQo+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5v cmcvbWFqb3Jkb21vLWluZm8uaHRtbA0KDQoNCi0tIA0KQW5keSBTaGV2Y2hlbmtvIDxhbmRyaXku c2hldmNoZW5rb0BpbnRlbC5jb20+DQpJbnRlbCBGaW5sYW5kIE95DQotLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0KSW50 ZWwgRmlubGFuZCBPeQpSZWdpc3RlcmVkIEFkZHJlc3M6IFBMIDI4MSwgMDAxODEgSGVsc2lua2kg CkJ1c2luZXNzIElkZW50aXR5IENvZGU6IDAzNTc2MDYgLSA0IApEb21pY2lsZWQgaW4gSGVsc2lu a2kgCgpUaGlzIGUtbWFpbCBhbmQgYW55IGF0dGFjaG1lbnRzIG1heSBjb250YWluIGNvbmZpZGVu dGlhbCBtYXRlcmlhbCBmb3IKdGhlIHNvbGUgdXNlIG9mIHRoZSBpbnRlbmRlZCByZWNpcGllbnQo cykuIEFueSByZXZpZXcgb3IgZGlzdHJpYnV0aW9uCmJ5IG90aGVycyBpcyBzdHJpY3RseSBwcm9o aWJpdGVkLiBJZiB5b3UgYXJlIG5vdCB0aGUgaW50ZW5kZWQKcmVjaXBpZW50LCBwbGVhc2UgY29u dGFjdCB0aGUgc2VuZGVyIGFuZCBkZWxldGUgYWxsIGNvcGllcy4K -- 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 05/07/2014 04:31 PM, Shevchenko, Andriy wrote: > On Sun, 2014-05-04 at 18:22 +0800, Hongbo Zhang wrote: >> On 05/03/2014 12:46 AM, Vinod Koul wrote: >>> On Fri, Apr 18, 2014 at 04:17:51PM +0800, hongbo.zhang@freescale.com wrote: >>>> From: Hongbo Zhang <hongbo.zhang@freescale.com> >>>> >>>> This patch adds suspend resume functions for Freescale DMA driver. >>>> .prepare callback is used to stop further descriptors from being added into the >>>> pending queue, and also issue pending queues into execution if there is any. >>>> .suspend callback makes sure all the pending jobs are cleaned up and all the >>>> channels are idle, and save the mode registers. >>>> .resume callback re-initializes the channels by restore the mode registers. >>>> >>>> + >>>> +static const struct dev_pm_ops fsldma_pm_ops = { >>>> + .prepare = fsldma_prepare, >>>> + .suspend = fsldma_suspend, >>>> + .resume = fsldma_resume, >>>> +}; >>> I think this is not correct. We discussed this sometime back on list. The >>> DMAengine drivers should use late resume and early suspend to ensure they get >>> suspended after clients (who should use normal ones) and resume before them >>> >> OK, will update it like this: >> use .suspend to take place of current .prepare > Could you remove this at all? > > Answering to your previous statements I could say that. > Device drivers (DMAc users) that don't implement .suspend callback are > on their own with troubles, you have not to care about them in the DMA > driver. Thanks for pointing out this issue. Then how to handle the descriptors in the pending list if there is any? a. let them finished. but if the DMA user has already suspended prior DMA controller, it is meaningless somehow and may even ask for trouble. b. don't touch them. after resume these pending descriptors could be executed, it is also meaningless because the resumed DMA user may in different state from before being suspended. c. delete them. should we do this? is is a bit crude? d. return a non-success value then the whole suspend process is reversed, e.g. suspend fails. I've looked through some dma drivers, most of them is in case b. >> use .suspend_late to take place of current .suspend >> use .resume_early to take place of current .resume >> >> >> >> -- >> 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 > -- 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, May 08, 2014 at 05:52:37PM +0800, Hongbo Zhang wrote: > > On 05/07/2014 04:31 PM, Shevchenko, Andriy wrote: > >On Sun, 2014-05-04 at 18:22 +0800, Hongbo Zhang wrote: > >>On 05/03/2014 12:46 AM, Vinod Koul wrote: > >>>On Fri, Apr 18, 2014 at 04:17:51PM +0800, hongbo.zhang@freescale.com wrote: > >>>>From: Hongbo Zhang <hongbo.zhang@freescale.com> > >>>> > >>>>This patch adds suspend resume functions for Freescale DMA driver. > >>>>.prepare callback is used to stop further descriptors from being added into the > >>>>pending queue, and also issue pending queues into execution if there is any. > >>>>.suspend callback makes sure all the pending jobs are cleaned up and all the > >>>>channels are idle, and save the mode registers. > >>>>.resume callback re-initializes the channels by restore the mode registers. > >>>> > >>>>+ > >>>>+static const struct dev_pm_ops fsldma_pm_ops = { > >>>>+ .prepare = fsldma_prepare, > >>>>+ .suspend = fsldma_suspend, > >>>>+ .resume = fsldma_resume, > >>>>+}; > >>>I think this is not correct. We discussed this sometime back on list. The > >>>DMAengine drivers should use late resume and early suspend to ensure they get > >>>suspended after clients (who should use normal ones) and resume before them > >>> > >>OK, will update it like this: > >>use .suspend to take place of current .prepare > >Could you remove this at all? > > > >Answering to your previous statements I could say that. > >Device drivers (DMAc users) that don't implement .suspend callback are > >on their own with troubles, you have not to care about them in the DMA > >driver. > > Thanks for pointing out this issue. > Then how to handle the descriptors in the pending list if there is any? > a. let them finished. > but if the DMA user has already suspended prior DMA controller, > it is meaningless somehow and may even ask for trouble. > b. don't touch them. > after resume these pending descriptors could be executed, it is > also meaningless because the resumed DMA user may in different state > from before being suspended. > c. delete them. > should we do this? is is a bit crude? > d. return a non-success value > then the whole suspend process is reversed, e.g. suspend fails. > I've looked through some dma drivers, most of them is in case b. Yes and that makese sense. In calssic suspend case we maybe in middle so graceful behaviour would be to for client to PAUSE or terminate and then suspend followed by DMA suspend. You need to rely on client doing the right thing here
On 05/21/2014 11:45 AM, Vinod Koul wrote: > On Thu, May 08, 2014 at 05:52:37PM +0800, Hongbo Zhang wrote: >> On 05/07/2014 04:31 PM, Shevchenko, Andriy wrote: >>> On Sun, 2014-05-04 at 18:22 +0800, Hongbo Zhang wrote: >>>> On 05/03/2014 12:46 AM, Vinod Koul wrote: >>>>> On Fri, Apr 18, 2014 at 04:17:51PM +0800, hongbo.zhang@freescale.com wrote: >>>>>> From: Hongbo Zhang <hongbo.zhang@freescale.com> >>>>>> >>>>>> This patch adds suspend resume functions for Freescale DMA driver. >>>>>> .prepare callback is used to stop further descriptors from being added into the >>>>>> pending queue, and also issue pending queues into execution if there is any. >>>>>> .suspend callback makes sure all the pending jobs are cleaned up and all the >>>>>> channels are idle, and save the mode registers. >>>>>> .resume callback re-initializes the channels by restore the mode registers. >>>>>> >>>>>> + >>>>>> +static const struct dev_pm_ops fsldma_pm_ops = { >>>>>> + .prepare = fsldma_prepare, >>>>>> + .suspend = fsldma_suspend, >>>>>> + .resume = fsldma_resume, >>>>>> +}; >>>>> I think this is not correct. We discussed this sometime back on list. The >>>>> DMAengine drivers should use late resume and early suspend to ensure they get >>>>> suspended after clients (who should use normal ones) and resume before them >>>>> >>>> OK, will update it like this: >>>> use .suspend to take place of current .prepare >>> Could you remove this at all? >>> >>> Answering to your previous statements I could say that. >>> Device drivers (DMAc users) that don't implement .suspend callback are >>> on their own with troubles, you have not to care about them in the DMA >>> driver. >> Thanks for pointing out this issue. >> Then how to handle the descriptors in the pending list if there is any? >> a. let them finished. >> but if the DMA user has already suspended prior DMA controller, >> it is meaningless somehow and may even ask for trouble. >> b. don't touch them. >> after resume these pending descriptors could be executed, it is >> also meaningless because the resumed DMA user may in different state >> from before being suspended. >> c. delete them. >> should we do this? is is a bit crude? >> d. return a non-success value >> then the whole suspend process is reversed, e.g. suspend fails. >> I've looked through some dma drivers, most of them is in case b. > Yes and that makese sense. > > In calssic suspend case we maybe in middle so graceful behaviour would be to for > client to PAUSE or terminate and then suspend followed by DMA suspend. > You need to rely on client doing the right thing here > OK, will resend this 6/8, 7/8 and 8/8 for another iteration, and will let the current 6/8 to be the last one for being reviewed and merged easier. -- 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/fsldma.c b/drivers/dma/fsldma.c index 6e1c9b3..836fc27 100644 --- a/drivers/dma/fsldma.c +++ b/drivers/dma/fsldma.c @@ -400,6 +400,14 @@ static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor *tx) spin_lock_bh(&chan->desc_lock); +#ifdef CONFIG_PM + if (unlikely(chan->pm_state != RUNNING)) { + chan_dbg(chan, "cannot submit due to suspend\n"); + spin_unlock_bh(&chan->desc_lock); + return -1; + } +#endif + /* * assign cookies to all of the software descriptors * that make up this transaction @@ -1312,6 +1320,9 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev, INIT_LIST_HEAD(&chan->ld_running); INIT_LIST_HEAD(&chan->ld_completed); chan->idle = true; +#ifdef CONFIG_PM + chan->pm_state = RUNNING; +#endif chan->common.device = &fdev->common; dma_cookie_init(&chan->common); @@ -1451,6 +1462,92 @@ static int fsldma_of_remove(struct platform_device *op) return 0; } +#ifdef CONFIG_PM +static int fsldma_prepare(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct fsldma_device *fdev = platform_get_drvdata(pdev); + struct fsldma_chan *chan; + int i; + + for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) { + chan = fdev->chan[i]; + if (!chan) + continue; + + spin_lock_bh(&chan->desc_lock); + chan->pm_state = SUSPENDING; + if (!list_empty(&chan->ld_pending)) + fsl_chan_xfer_ld_queue(chan); + spin_unlock_bh(&chan->desc_lock); + } + + return 0; +} + +static int fsldma_suspend(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct fsldma_device *fdev = platform_get_drvdata(pdev); + struct fsldma_chan *chan; + int i; + + for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) { + chan = fdev->chan[i]; + if (!chan) + continue; + + spin_lock_bh(&chan->desc_lock); + if (!chan->idle) + goto out; + chan->regs_save.mr = get_mr(chan); + chan->pm_state = SUSPENDED; + spin_unlock_bh(&chan->desc_lock); + } + return 0; + +out: + for (; i >= 0; i--) { + chan = fdev->chan[i]; + if (!chan) + continue; + chan->pm_state = RUNNING; + spin_unlock_bh(&chan->desc_lock); + } + return -EBUSY; +} + +static int fsldma_resume(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct fsldma_device *fdev = platform_get_drvdata(pdev); + struct fsldma_chan *chan; + u32 mode; + int i; + + for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) { + chan = fdev->chan[i]; + if (!chan) + continue; + + spin_lock_bh(&chan->desc_lock); + mode = chan->regs_save.mr + & ~FSL_DMA_MR_CS & ~FSL_DMA_MR_CC & ~FSL_DMA_MR_CA; + set_mr(chan, mode); + chan->pm_state = RUNNING; + spin_unlock_bh(&chan->desc_lock); + } + + return 0; +} + +static const struct dev_pm_ops fsldma_pm_ops = { + .prepare = fsldma_prepare, + .suspend = fsldma_suspend, + .resume = fsldma_resume, +}; +#endif + static const struct of_device_id fsldma_of_ids[] = { { .compatible = "fsl,elo3-dma", }, { .compatible = "fsl,eloplus-dma", }, @@ -1463,6 +1560,9 @@ static struct platform_driver fsldma_of_driver = { .name = "fsl-elo-dma", .owner = THIS_MODULE, .of_match_table = fsldma_of_ids, +#ifdef CONFIG_PM + .pm = &fsldma_pm_ops, +#endif }, .probe = fsldma_of_probe, .remove = fsldma_of_remove, diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h index ec19517..eecaf9e 100644 --- a/drivers/dma/fsldma.h +++ b/drivers/dma/fsldma.h @@ -134,6 +134,18 @@ struct fsldma_device { #define FSL_DMA_CHAN_PAUSE_EXT 0x00001000 #define FSL_DMA_CHAN_START_EXT 0x00002000 +#ifdef CONFIG_PM +struct fsldma_chan_regs_save { + u32 mr; +}; + +enum fsldma_pm_state { + RUNNING = 0, + SUSPENDING, + SUSPENDED, +}; +#endif + struct fsldma_chan { char name[8]; /* Channel name */ struct fsldma_chan_regs __iomem *regs; @@ -161,6 +173,10 @@ struct fsldma_chan { struct tasklet_struct tasklet; u32 feature; bool idle; /* DMA controller is idle */ +#ifdef CONFIG_PM + struct fsldma_chan_regs_save regs_save; + enum fsldma_pm_state pm_state; +#endif void (*toggle_ext_pause)(struct fsldma_chan *fsl_chan, int enable); void (*toggle_ext_start)(struct fsldma_chan *fsl_chan, int enable);