| Message ID | 1386684373-24753-1-git-send-email-chiau.ee.chew@intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Tue, Dec 10, 2013 at 10:06:13PM +0800, Chew Chiau Ee wrote: > From: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > This is to disable/enable DW_DMAC hw during suspend/resume. > > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/dma/dw/pci.c | 33 +++++++++++++++++++++++++++++++++ > 1 files changed, 33 insertions(+), 0 deletions(-) > > diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c > index e89fc24..97bc3a2 100644 > --- a/drivers/dma/dw/pci.c > +++ b/drivers/dma/dw/pci.c > @@ -75,6 +75,36 @@ static void dw_pci_remove(struct pci_dev *pdev) > dev_warn(&pdev->dev, "can't remove device properly: %d\n", ret); > } > > +#ifdef CONFIG_PM_SLEEP > + > +static int dw_pci_suspend_noirq(struct device *dev) > +{ > + struct pci_dev *pci = to_pci_dev(dev); > + struct dw_dma_chip *chip = pci_get_drvdata(pci); > + > + return dw_dma_suspend(chip); > +}; > + > +static int dw_pci_resume_noirq(struct device *dev) > +{ > + struct pci_dev *pci = to_pci_dev(dev); > + struct dw_dma_chip *chip = pci_get_drvdata(pci); > + > + return dw_dma_resume(chip); > +}; > + > +#else /* !CONFIG_PM_SLEEP */ > + > +#define dw_pci_suspend_noirq NULL > +#define dw_pci_resume_noirq NULL > + > +#endif /* !CONFIG_PM_SLEEP */ How about SET_SYSTEM_SLEEP_PM_OPS instead? -- ~Vinod > + > +static const struct dev_pm_ops dw_pci_dev_pm_ops = { > + .suspend_noirq = dw_pci_suspend_noirq, > + .resume_noirq = dw_pci_resume_noirq, > +}; > + > static DEFINE_PCI_DEVICE_TABLE(dw_pci_id_table) = { > /* Medfield */ > { PCI_VDEVICE(INTEL, 0x0827), (kernel_ulong_t)&dw_pci_pdata }, > @@ -92,6 +122,9 @@ static struct pci_driver dw_pci_driver = { > .id_table = dw_pci_id_table, > .probe = dw_pci_probe, > .remove = dw_pci_remove, > + .driver = { > + .pm = &dw_pci_dev_pm_ops, > + }, > }; > > module_pci_driver(dw_pci_driver); > -- > 1.7.4.4 >
T24gVHVlLCAyMDEzLTEyLTEwIGF0IDE1OjQwICswNTMwLCBWaW5vZCBLb3VsIHdyb3RlOg0KPiBP biBUdWUsIERlYyAxMCwgMjAxMyBhdCAxMDowNjoxM1BNICswODAwLCBDaGV3IENoaWF1IEVlIHdy b3RlOg0KPiA+IEZyb206IENoZXcsIENoaWF1IEVlIDxjaGlhdS5lZS5jaGV3QGludGVsLmNvbT4N Cj4gPiANCj4gPiBUaGlzIGlzIHRvIGRpc2FibGUvZW5hYmxlIERXX0RNQUMgaHcgZHVyaW5nIHN1 c3BlbmQvcmVzdW1lLg0KPiA+IA0KPiA+IFNpZ25lZC1vZmYtYnk6IENoZXcsIENoaWF1IEVlIDxj aGlhdS5lZS5jaGV3QGludGVsLmNvbT4NCj4gPiBBY2tlZC1ieTogQW5keSBTaGV2Y2hlbmtvIDxh bmRyaXkuc2hldmNoZW5rb0BsaW51eC5pbnRlbC5jb20+DQo+ID4gLS0tDQo+ID4gIGRyaXZlcnMv ZG1hL2R3L3BjaS5jIHwgICAzMyArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysNCj4g PiAgMSBmaWxlcyBjaGFuZ2VkLCAzMyBpbnNlcnRpb25zKCspLCAwIGRlbGV0aW9ucygtKQ0KPiA+ IA0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2RtYS9kdy9wY2kuYyBiL2RyaXZlcnMvZG1hL2R3 L3BjaS5jDQo+ID4gaW5kZXggZTg5ZmMyNC4uOTdiYzNhMiAxMDA2NDQNCj4gPiAtLS0gYS9kcml2 ZXJzL2RtYS9kdy9wY2kuYw0KPiA+ICsrKyBiL2RyaXZlcnMvZG1hL2R3L3BjaS5jDQo+ID4gQEAg LTc1LDYgKzc1LDM2IEBAIHN0YXRpYyB2b2lkIGR3X3BjaV9yZW1vdmUoc3RydWN0IHBjaV9kZXYg KnBkZXYpDQo+ID4gIAkJZGV2X3dhcm4oJnBkZXYtPmRldiwgImNhbid0IHJlbW92ZSBkZXZpY2Ug cHJvcGVybHk6ICVkXG4iLCByZXQpOw0KPiA+ICB9DQo+ID4gIA0KPiA+ICsjaWZkZWYgQ09ORklH X1BNX1NMRUVQDQo+ID4gKw0KPiA+ICtzdGF0aWMgaW50IGR3X3BjaV9zdXNwZW5kX25vaXJxKHN0 cnVjdCBkZXZpY2UgKmRldikNCj4gPiArew0KPiA+ICsJc3RydWN0IHBjaV9kZXYgKnBjaSA9IHRv X3BjaV9kZXYoZGV2KTsNCj4gPiArCXN0cnVjdCBkd19kbWFfY2hpcCAqY2hpcCA9IHBjaV9nZXRf ZHJ2ZGF0YShwY2kpOw0KPiA+ICsNCj4gPiArCXJldHVybiBkd19kbWFfc3VzcGVuZChjaGlwKTsN Cj4gPiArfTsNCj4gPiArDQo+ID4gK3N0YXRpYyBpbnQgZHdfcGNpX3Jlc3VtZV9ub2lycShzdHJ1 Y3QgZGV2aWNlICpkZXYpDQo+ID4gK3sNCj4gPiArCXN0cnVjdCBwY2lfZGV2ICpwY2kgPSB0b19w Y2lfZGV2KGRldik7DQo+ID4gKwlzdHJ1Y3QgZHdfZG1hX2NoaXAgKmNoaXAgPSBwY2lfZ2V0X2Ry dmRhdGEocGNpKTsNCj4gPiArDQo+ID4gKwlyZXR1cm4gZHdfZG1hX3Jlc3VtZShjaGlwKTsNCj4g PiArfTsNCj4gPiArDQo+ID4gKyNlbHNlIC8qICFDT05GSUdfUE1fU0xFRVAgKi8NCj4gPiArDQo+ ID4gKyNkZWZpbmUgZHdfcGNpX3N1c3BlbmRfbm9pcnEJTlVMTA0KPiA+ICsjZGVmaW5lIGR3X3Bj aV9yZXN1bWVfbm9pcnEJTlVMTA0KPiA+ICsNCj4gPiArI2VuZGlmIC8qICFDT05GSUdfUE1fU0xF RVAgKi8NCj4gSG93IGFib3V0IFNFVF9TWVNURU1fU0xFRVBfUE1fT1BTIGluc3RlYWQ/DQoNClNv LCB3ZSBhcmUgdXNpbmcgKl9ub2lycSB2ZXJzaW9ucyBvZiB0aGUgZnVuY3Rpb25zIGhlcmUuIFdo YXQgaGFwcGVuZWQNCndoZW4gd2Ugc3dpdGNoIHRvIG5vcm1hbCBvbmVzPyBBbnkgc2lkZSBlZmZl Y3RzPw0KDQo+IA0KPiAtLQ0KPiB+Vmlub2QNCj4gPiArDQo+ID4gK3N0YXRpYyBjb25zdCBzdHJ1 Y3QgZGV2X3BtX29wcyBkd19wY2lfZGV2X3BtX29wcyA9IHsNCj4gPiArCS5zdXNwZW5kX25vaXJx ID0gZHdfcGNpX3N1c3BlbmRfbm9pcnEsDQo+ID4gKwkucmVzdW1lX25vaXJxID0gZHdfcGNpX3Jl c3VtZV9ub2lycSwNCj4gPiArfTsNCj4gPiArDQo+ID4gIHN0YXRpYyBERUZJTkVfUENJX0RFVklD RV9UQUJMRShkd19wY2lfaWRfdGFibGUpID0gew0KPiA+ICAJLyogTWVkZmllbGQgKi8NCj4gPiAg CXsgUENJX1ZERVZJQ0UoSU5URUwsIDB4MDgyNyksIChrZXJuZWxfdWxvbmdfdCkmZHdfcGNpX3Bk YXRhIH0sDQo+ID4gQEAgLTkyLDYgKzEyMiw5IEBAIHN0YXRpYyBzdHJ1Y3QgcGNpX2RyaXZlciBk d19wY2lfZHJpdmVyID0gew0KPiA+ICAJLmlkX3RhYmxlCT0gZHdfcGNpX2lkX3RhYmxlLA0KPiA+ ICAJLnByb2JlCQk9IGR3X3BjaV9wcm9iZSwNCj4gPiAgCS5yZW1vdmUJCT0gZHdfcGNpX3JlbW92 ZSwNCj4gPiArCS5kcml2ZXIJPSB7DQo+ID4gKwkJLnBtCT0gJmR3X3BjaV9kZXZfcG1fb3BzLA0K PiA+ICsJfSwNCj4gPiAgfTsNCj4gPiAgDQo+ID4gIG1vZHVsZV9wY2lfZHJpdmVyKGR3X3BjaV9k cml2ZXIpOw0KPiA+IC0tIA0KPiA+IDEuNy40LjQNCj4gPiANCj4gDQoNCi0tIA0KQW5keSBTaGV2 Y2hlbmtvIDxhbmRyaXkuc2hldmNoZW5rb0BpbnRlbC5jb20+DQpJbnRlbCBGaW5sYW5kIE95DQot LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0KSW50ZWwgRmlubGFuZCBPeQpSZWdpc3RlcmVkIEFkZHJlc3M6IFBMIDI4MSwg MDAxODEgSGVsc2lua2kgCkJ1c2luZXNzIElkZW50aXR5IENvZGU6IDAzNTc2MDYgLSA0IApEb21p Y2lsZWQgaW4gSGVsc2lua2kgCgpUaGlzIGUtbWFpbCBhbmQgYW55IGF0dGFjaG1lbnRzIG1heSBj b250YWluIGNvbmZpZGVudGlhbCBtYXRlcmlhbCBmb3IKdGhlIHNvbGUgdXNlIG9mIHRoZSBpbnRl bmRlZCByZWNpcGllbnQocykuIEFueSByZXZpZXcgb3IgZGlzdHJpYnV0aW9uCmJ5IG90aGVycyBp cyBzdHJpY3RseSBwcm9oaWJpdGVkLiBJZiB5b3UgYXJlIG5vdCB0aGUgaW50ZW5kZWQKcmVjaXBp ZW50LCBwbGVhc2UgY29udGFjdCB0aGUgc2VuZGVyIGFuZCBkZWxldGUgYWxsIGNvcGllcy4K -- 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
Vmlub2QsDQoNCkFzIG1lbnRpb25lZCBieSBBbmR5LCB3ZSBhcmUgdXNpbmcgKl9ub2lycSAgdmVy aW9uIG9mIHN1c3BlbmQvcmVzdW1lIFBNIGNhbGxiYWNrIHdoZXJlYnkgdGhlIGNhbGxiYWNrcyB3 b3VsZCBiZSBleGVjdXRlZCBhZnRlciBJUlEgaGFuZGxlcnMgaGF2ZSBiZWVuIGRpc2FibGVkLiBJ ZiB1c2luZyBTRVRfU1lTVEVNX1NMRUVQX1BNX09QUywgaXQgd291bGQgYmUgdGhlIG5vcm1hbCBz dXNwZW5kL3Jlc3VtZSBQTSBjYWxsYmFjay4gTG9va2luZyBhdCB0aGUgRGVzZ2lud2FyZSBETUFD IHBsYXRmb3JtIGNvZGUgKGRyaXZlcnMvZG1hL2R3L3BsYXRmb3JtLmMpLCBpdCBpcyB1c2luZyB0 aGUgKl9ub2lycSBzdXNwZW5kL3Jlc3VtZSBQTSBjYWxsYmFjay4gSXMgaXQgYWR2aXNhYmxlIHRv IHVzZSB0aGUgbm9ybWFsIHN1c3BlbmQvcmVzdW1lIFBNIGNhbGxiYWNrIGluc3RlYWQgb2YgKl9u b2lycSBzdXNwZW5kL1BNIGNhbGxiYWNrPyANCg0KVGhhbmtzLA0KQ2hpYXUgRWUNCg0KLS0tLS1P cmlnaW5hbCBNZXNzYWdlLS0tLS0NCkZyb206IFNoZXZjaGVua28sIEFuZHJpeSANClNlbnQ6IFR1 ZXNkYXksIERlY2VtYmVyIDEwLCAyMDEzIDc6NTcgUE0NClRvOiBLb3VsLCBWaW5vZA0KQ2M6IENo ZXcsIENoaWF1IEVlOyBWaXJlc2ggS3VtYXI7IEFuZHkgU2hldmNoZW5rbzsgV2lsbGlhbXMsIERh biBKOyBkbWFlbmdpbmVAdmdlci5rZXJuZWwub3JnOyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwu b3JnDQpTdWJqZWN0OiBSZTogW1BBVENIXSBkbWE6IGR3OiBBZGQgc3VzcGVuZCBhbmQgcmVzdW1l IGhhbmRsaW5nIGZvciBQQ0kgbW9kZSBEV19ETUFDLg0KDQpPbiBUdWUsIDIwMTMtMTItMTAgYXQg MTU6NDAgKzA1MzAsIFZpbm9kIEtvdWwgd3JvdGU6DQo+IE9uIFR1ZSwgRGVjIDEwLCAyMDEzIGF0 IDEwOjA2OjEzUE0gKzA4MDAsIENoZXcgQ2hpYXUgRWUgd3JvdGU6DQo+ID4gRnJvbTogQ2hldywg Q2hpYXUgRWUgPGNoaWF1LmVlLmNoZXdAaW50ZWwuY29tPg0KPiA+IA0KPiA+IFRoaXMgaXMgdG8g ZGlzYWJsZS9lbmFibGUgRFdfRE1BQyBodyBkdXJpbmcgc3VzcGVuZC9yZXN1bWUuDQo+ID4gDQo+ ID4gU2lnbmVkLW9mZi1ieTogQ2hldywgQ2hpYXUgRWUgPGNoaWF1LmVlLmNoZXdAaW50ZWwuY29t Pg0KPiA+IEFja2VkLWJ5OiBBbmR5IFNoZXZjaGVua28gPGFuZHJpeS5zaGV2Y2hlbmtvQGxpbnV4 LmludGVsLmNvbT4NCj4gPiAtLS0NCj4gPiAgZHJpdmVycy9kbWEvZHcvcGNpLmMgfCAgIDMzICsr KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKw0KPiA+ICAxIGZpbGVzIGNoYW5nZWQsIDMz IGluc2VydGlvbnMoKyksIDAgZGVsZXRpb25zKC0pDQo+ID4gDQo+ID4gZGlmZiAtLWdpdCBhL2Ry aXZlcnMvZG1hL2R3L3BjaS5jIGIvZHJpdmVycy9kbWEvZHcvcGNpLmMgaW5kZXggDQo+ID4gZTg5 ZmMyNC4uOTdiYzNhMiAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2RtYS9kdy9wY2kuYw0KPiA+ ICsrKyBiL2RyaXZlcnMvZG1hL2R3L3BjaS5jDQo+ID4gQEAgLTc1LDYgKzc1LDM2IEBAIHN0YXRp YyB2b2lkIGR3X3BjaV9yZW1vdmUoc3RydWN0IHBjaV9kZXYgKnBkZXYpDQo+ID4gIAkJZGV2X3dh cm4oJnBkZXYtPmRldiwgImNhbid0IHJlbW92ZSBkZXZpY2UgcHJvcGVybHk6ICVkXG4iLCByZXQp OyAgDQo+ID4gfQ0KPiA+ICANCj4gPiArI2lmZGVmIENPTkZJR19QTV9TTEVFUA0KPiA+ICsNCj4g PiArc3RhdGljIGludCBkd19wY2lfc3VzcGVuZF9ub2lycShzdHJ1Y3QgZGV2aWNlICpkZXYpIHsN Cj4gPiArCXN0cnVjdCBwY2lfZGV2ICpwY2kgPSB0b19wY2lfZGV2KGRldik7DQo+ID4gKwlzdHJ1 Y3QgZHdfZG1hX2NoaXAgKmNoaXAgPSBwY2lfZ2V0X2RydmRhdGEocGNpKTsNCj4gPiArDQo+ID4g KwlyZXR1cm4gZHdfZG1hX3N1c3BlbmQoY2hpcCk7DQo+ID4gK307DQo+ID4gKw0KPiA+ICtzdGF0 aWMgaW50IGR3X3BjaV9yZXN1bWVfbm9pcnEoc3RydWN0IGRldmljZSAqZGV2KSB7DQo+ID4gKwlz dHJ1Y3QgcGNpX2RldiAqcGNpID0gdG9fcGNpX2RldihkZXYpOw0KPiA+ICsJc3RydWN0IGR3X2Rt YV9jaGlwICpjaGlwID0gcGNpX2dldF9kcnZkYXRhKHBjaSk7DQo+ID4gKw0KPiA+ICsJcmV0dXJu IGR3X2RtYV9yZXN1bWUoY2hpcCk7DQo+ID4gK307DQo+ID4gKw0KPiA+ICsjZWxzZSAvKiAhQ09O RklHX1BNX1NMRUVQICovDQo+ID4gKw0KPiA+ICsjZGVmaW5lIGR3X3BjaV9zdXNwZW5kX25vaXJx CU5VTEwNCj4gPiArI2RlZmluZSBkd19wY2lfcmVzdW1lX25vaXJxCU5VTEwNCj4gPiArDQo+ID4g KyNlbmRpZiAvKiAhQ09ORklHX1BNX1NMRUVQICovDQo+IEhvdyBhYm91dCBTRVRfU1lTVEVNX1NM RUVQX1BNX09QUyBpbnN0ZWFkPw0KDQpTbywgd2UgYXJlIHVzaW5nICpfbm9pcnEgdmVyc2lvbnMg b2YgdGhlIGZ1bmN0aW9ucyBoZXJlLiBXaGF0IGhhcHBlbmVkIHdoZW4gd2Ugc3dpdGNoIHRvIG5v cm1hbCBvbmVzPyBBbnkgc2lkZSBlZmZlY3RzPw0KDQo+IA0KPiAtLQ0KPiB+Vmlub2QNCj4gPiAr DQo+ID4gK3N0YXRpYyBjb25zdCBzdHJ1Y3QgZGV2X3BtX29wcyBkd19wY2lfZGV2X3BtX29wcyA9 IHsNCj4gPiArCS5zdXNwZW5kX25vaXJxID0gZHdfcGNpX3N1c3BlbmRfbm9pcnEsDQo+ID4gKwku cmVzdW1lX25vaXJxID0gZHdfcGNpX3Jlc3VtZV9ub2lycSwgfTsNCj4gPiArDQo+ID4gIHN0YXRp YyBERUZJTkVfUENJX0RFVklDRV9UQUJMRShkd19wY2lfaWRfdGFibGUpID0gew0KPiA+ICAJLyog TWVkZmllbGQgKi8NCj4gPiAgCXsgUENJX1ZERVZJQ0UoSU5URUwsIDB4MDgyNyksIChrZXJuZWxf dWxvbmdfdCkmZHdfcGNpX3BkYXRhIH0sIEBAIA0KPiA+IC05Miw2ICsxMjIsOSBAQCBzdGF0aWMg c3RydWN0IHBjaV9kcml2ZXIgZHdfcGNpX2RyaXZlciA9IHsNCj4gPiAgCS5pZF90YWJsZQk9IGR3 X3BjaV9pZF90YWJsZSwNCj4gPiAgCS5wcm9iZQkJPSBkd19wY2lfcHJvYmUsDQo+ID4gIAkucmVt b3ZlCQk9IGR3X3BjaV9yZW1vdmUsDQo+ID4gKwkuZHJpdmVyCT0gew0KPiA+ICsJCS5wbQk9ICZk d19wY2lfZGV2X3BtX29wcywNCj4gPiArCX0sDQo+ID4gIH07DQo+ID4gIA0KPiA+ICBtb2R1bGVf cGNpX2RyaXZlcihkd19wY2lfZHJpdmVyKTsNCj4gPiAtLQ0KPiA+IDEuNy40LjQNCj4gPiANCj4g DQoNCi0tDQpBbmR5IFNoZXZjaGVua28gPGFuZHJpeS5zaGV2Y2hlbmtvQGludGVsLmNvbT4gSW50 ZWwgRmlubGFuZCBPeQ0K -- 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, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote: > Vinod, Please do *NOT* top post Please fix your MUA to wrap lines with 80chars. I have fix below to make it readble... > As mentioned by Andy, we are using *_noirq verion of suspend/resume PM > callback whereby the callbacks would be executed after IRQ handlers have been > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal > suspend/resume PM callback. Looking at the Desginware DMAC platform code > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM > callback. Is it advisable to use the normal suspend/resume PM callback instead > of *_noirq suspend/PM callback? i dont see a reason why we need the noirq versions -- ~Vinod > > Thanks, > Chiau Ee > > -----Original Message----- > From: Shevchenko, Andriy > Sent: Tuesday, December 10, 2013 7:57 PM > To: Koul, Vinod > Cc: Chew, Chiau Ee; Viresh Kumar; Andy Shevchenko; Williams, Dan J; dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC. > > On Tue, 2013-12-10 at 15:40 +0530, Vinod Koul wrote: > > On Tue, Dec 10, 2013 at 10:06:13PM +0800, Chew Chiau Ee wrote: > > > From: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > > > > > This is to disable/enable DW_DMAC hw during suspend/resume. > > > > > > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/dma/dw/pci.c | 33 +++++++++++++++++++++++++++++++++ > > > 1 files changed, 33 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c index > > > e89fc24..97bc3a2 100644 > > > --- a/drivers/dma/dw/pci.c > > > +++ b/drivers/dma/dw/pci.c > > > @@ -75,6 +75,36 @@ static void dw_pci_remove(struct pci_dev *pdev) > > > dev_warn(&pdev->dev, "can't remove device properly: %d\n", ret); > > > } > > > > > > +#ifdef CONFIG_PM_SLEEP > > > + > > > +static int dw_pci_suspend_noirq(struct device *dev) { > > > + struct pci_dev *pci = to_pci_dev(dev); > > > + struct dw_dma_chip *chip = pci_get_drvdata(pci); > > > + > > > + return dw_dma_suspend(chip); > > > +}; > > > + > > > +static int dw_pci_resume_noirq(struct device *dev) { > > > + struct pci_dev *pci = to_pci_dev(dev); > > > + struct dw_dma_chip *chip = pci_get_drvdata(pci); > > > + > > > + return dw_dma_resume(chip); > > > +}; > > > + > > > +#else /* !CONFIG_PM_SLEEP */ > > > + > > > +#define dw_pci_suspend_noirq NULL > > > +#define dw_pci_resume_noirq NULL > > > + > > > +#endif /* !CONFIG_PM_SLEEP */ > > How about SET_SYSTEM_SLEEP_PM_OPS instead? > > So, we are using *_noirq versions of the functions here. What happened when we switch to normal ones? Any side effects? > > > > > -- > > ~Vinod > > > + > > > +static const struct dev_pm_ops dw_pci_dev_pm_ops = { > > > + .suspend_noirq = dw_pci_suspend_noirq, > > > + .resume_noirq = dw_pci_resume_noirq, }; > > > + > > > static DEFINE_PCI_DEVICE_TABLE(dw_pci_id_table) = { > > > /* Medfield */ > > > { PCI_VDEVICE(INTEL, 0x0827), (kernel_ulong_t)&dw_pci_pdata }, @@ > > > -92,6 +122,9 @@ static struct pci_driver dw_pci_driver = { > > > .id_table = dw_pci_id_table, > > > .probe = dw_pci_probe, > > > .remove = dw_pci_remove, > > > + .driver = { > > > + .pm = &dw_pci_dev_pm_ops, > > > + }, > > > }; > > > > > > module_pci_driver(dw_pci_driver); > > > -- > > > 1.7.4.4 > > > > > > > -- > Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy
On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote: > On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote: > > As mentioned by Andy, we are using *_noirq verion of suspend/resume PM > > callback whereby the callbacks would be executed after IRQ handlers have been > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal > > suspend/resume PM callback. Looking at the Desginware DMAC platform code > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM > > callback. Is it advisable to use the normal suspend/resume PM callback instead > > of *_noirq suspend/PM callback? > > i dont see a reason why we need the noirq versions Okay. I imagine the following use case. For example we have compiled in DMA driver (dw_dmac) along with, for example, SPI driver. System was scheduled to go sleep. An order of calling IIUC might be DMA first, then SPI (since they are not in parent / child relations). What was happened when SPI would like to do a DMA transfer and DMA is going to sleep? I'm trying to understand if this is a case. > > > How about SET_SYSTEM_SLEEP_PM_OPS instead? > > > > So, we are using *_noirq versions of the functions here. What happened when we switch to normal ones? Any side effects?
On Thu, Dec 19, 2013 at 12:51:29PM +0200, Andy Shevchenko wrote: > On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote: > > On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote: > > > > As mentioned by Andy, we are using *_noirq verion of suspend/resume PM > > > callback whereby the callbacks would be executed after IRQ handlers have been > > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal > > > suspend/resume PM callback. Looking at the Desginware DMAC platform code > > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM > > > callback. Is it advisable to use the normal suspend/resume PM callback instead > > > of *_noirq suspend/PM callback? > > > > i dont see a reason why we need the noirq versions > > Okay. I imagine the following use case. > > For example we have compiled in DMA driver (dw_dmac) along with, for > example, SPI driver. > > System was scheduled to go sleep. > > An order of calling IIUC might be DMA first, then SPI (since they are > not in parent / child relations). > > What was happened when SPI would like to do a DMA transfer and DMA is > going to sleep? I'm trying to understand if this is a case. In that case how does no irq version help us? For these cases, I have been using suspend_late. Since the dmaengine driver is providing service to other clients (SPI), it needs to esnure that it suspends after SPI using suspend_late and resume using resume_early. That way dma is availble whenever the client is active -- ~Vinod > > > > > How about SET_SYSTEM_SLEEP_PM_OPS instead? > > > > > > So, we are using *_noirq versions of the functions here. What happened when we switch to normal ones? Any side effects? > > > > -- > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy >
On Mon, Jan 20, 2014 at 06:17:56PM +0530, Shevchenko, Andriy wrote: > On Mon, 2014-01-20 at 14:55 +0530, Vinod Koul wrote: > > On Thu, Dec 19, 2013 at 12:51:29PM +0200, Andy Shevchenko wrote: > > > On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote: > > > > On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote: > > > > > > > > As mentioned by Andy, we are using *_noirq verion of suspend/resume PM > > > > > callback whereby the callbacks would be executed after IRQ handlers have been > > > > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal > > > > > suspend/resume PM callback. Looking at the Desginware DMAC platform code > > > > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM > > > > > callback. Is it advisable to use the normal suspend/resume PM callback instead > > > > > of *_noirq suspend/PM callback? > > > > > > > > i dont see a reason why we need the noirq versions > > > > > > Okay. I imagine the following use case. > > > > > > For example we have compiled in DMA driver (dw_dmac) along with, for > > > example, SPI driver. > > > > > > System was scheduled to go sleep. > > > > > > An order of calling IIUC might be DMA first, then SPI (since they are > > > not in parent / child relations). > > > > > > What was happened when SPI would like to do a DMA transfer and DMA is > > > going to sleep? I'm trying to understand if this is a case. > > In that case how does no irq version help us? > > It guarantees that we have no user of DMA anymore, since there is no > interrupt going on. well how is that. It will gaurantee that there wont be interrupt. User can still submit a transaction or another transaction will be in progress... > > For these cases, I have been using suspend_late. Since the dmaengine driver is > > providing service to other clients (SPI), it needs to esnure that it suspends > > after SPI using suspend_late and resume using resume_early. That way dma is > > availble whenever the client is active > > suspend_late is working in context that interrupt handler may be > invoked. Thus, to have DMA driver be properly shut down we have to > wait / terminate possible ongoing transfer. Well client is already suspended via .suspend. So where is the transaction :) > > It seems for me all DMA drivers that are using > system .suspend()/.resume() are potentially buggy. Yup!
On Mon, 2014-01-20 at 14:55 +0530, Vinod Koul wrote: > On Thu, Dec 19, 2013 at 12:51:29PM +0200, Andy Shevchenko wrote: > > On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote: > > > On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote: > > > > > > As mentioned by Andy, we are using *_noirq verion of suspend/resume PM > > > > callback whereby the callbacks would be executed after IRQ handlers have been > > > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal > > > > suspend/resume PM callback. Looking at the Desginware DMAC platform code > > > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM > > > > callback. Is it advisable to use the normal suspend/resume PM callback instead > > > > of *_noirq suspend/PM callback? > > > > > > i dont see a reason why we need the noirq versions > > > > Okay. I imagine the following use case. > > > > For example we have compiled in DMA driver (dw_dmac) along with, for > > example, SPI driver. > > > > System was scheduled to go sleep. > > > > An order of calling IIUC might be DMA first, then SPI (since they are > > not in parent / child relations). > > > > What was happened when SPI would like to do a DMA transfer and DMA is > > going to sleep? I'm trying to understand if this is a case. > In that case how does no irq version help us? It guarantees that we have no user of DMA anymore, since there is no interrupt going on. > For these cases, I have been using suspend_late. Since the dmaengine driver is > providing service to other clients (SPI), it needs to esnure that it suspends > after SPI using suspend_late and resume using resume_early. That way dma is > availble whenever the client is active suspend_late is working in context that interrupt handler may be invoked. Thus, to have DMA driver be properly shut down we have to wait / terminate possible ongoing transfer. It seems for me all DMA drivers that are using system .suspend()/.resume() are potentially buggy. > > -- > ~Vinod > > > > > > > > How about SET_SYSTEM_SLEEP_PM_OPS instead? > > > > > > > > So, we are using *_noirq versions of the functions here. What happened when we switch to normal ones? Any side effects? > > > > > > > > -- > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Intel Finland Oy > > > -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Mon, 2014-01-20 at 17:37 +0530, Vinod Koul wrote: > On Mon, Jan 20, 2014 at 06:17:56PM +0530, Shevchenko, Andriy wrote: > > On Mon, 2014-01-20 at 14:55 +0530, Vinod Koul wrote: > > > On Thu, Dec 19, 2013 at 12:51:29PM +0200, Andy Shevchenko wrote: > > > > On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote: > > > > > On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote: > > > > > > > > > > As mentioned by Andy, we are using *_noirq verion of suspend/resume PM > > > > > > callback whereby the callbacks would be executed after IRQ handlers have been > > > > > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal > > > > > > suspend/resume PM callback. Looking at the Desginware DMAC platform code > > > > > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM > > > > > > callback. Is it advisable to use the normal suspend/resume PM callback instead > > > > > > of *_noirq suspend/PM callback? > > > > > > > > > > i dont see a reason why we need the noirq versions > > > > > > > > Okay. I imagine the following use case. > > > > > > > > For example we have compiled in DMA driver (dw_dmac) along with, for > > > > example, SPI driver. > > > > > > > > System was scheduled to go sleep. > > > > > > > > An order of calling IIUC might be DMA first, then SPI (since they are > > > > not in parent / child relations). > > > > > > > > What was happened when SPI would like to do a DMA transfer and DMA is > > > > going to sleep? I'm trying to understand if this is a case. > > > In that case how does no irq version help us? > > > > It guarantees that we have no user of DMA anymore, since there is no > > interrupt going on. > well how is that. It will gaurantee that there wont be interrupt. User can still > submit a transaction or another transaction will be in progress... This is how system suspend callback tree is called. First it calls .suspend() for all devices, then .suspend_late(), then .suspend_noirq(). There is set of assumptions per each callback round. After .suspend() the device must be quiescent. But... > > > For these cases, I have been using suspend_late. Since the dmaengine driver is > > > providing service to other clients (SPI), it needs to esnure that it suspends > > > after SPI using suspend_late and resume using resume_early. That way dma is > > > availble whenever the client is active > > > > suspend_late is working in context that interrupt handler may be > > invoked. Thus, to have DMA driver be properly shut down we have to > > wait / terminate possible ongoing transfer. > Well client is already suspended via .suspend. So where is the transaction :) ...as I already wrote before we have no parent-child relationship between DMA and, for example, SPI. That means we may possible have the case when SPI's .suspend() will be called later than DMA's one. > > It seems for me all DMA drivers that are using > > system .suspend()/.resume() are potentially buggy. > Yup! So, we have to decide what to do with them. .suspend_late() still seems for me not the best approach. *Or* we have to check for ongoing transaction and do something with it. *Or* just shut down the device and rely on DMA transaction initiator that it handles the terminated transaction properly. What is you opinion? -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Mon, Jan 20, 2014 at 07:38:32PM +0530, Shevchenko, Andriy wrote: > > > > > > > As mentioned by Andy, we are using *_noirq verion of suspend/resume PM > > > > > > > callback whereby the callbacks would be executed after IRQ handlers have been > > > > > > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal > > > > > > > suspend/resume PM callback. Looking at the Desginware DMAC platform code > > > > > > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM > > > > > > > callback. Is it advisable to use the normal suspend/resume PM callback instead > > > > > > > of *_noirq suspend/PM callback? > > > > > > > > > > > > i dont see a reason why we need the noirq versions > > > > > > > > > > Okay. I imagine the following use case. > > > > > > > > > > For example we have compiled in DMA driver (dw_dmac) along with, for > > > > > example, SPI driver. > > > > > > > > > > System was scheduled to go sleep. > > > > > > > > > > An order of calling IIUC might be DMA first, then SPI (since they are > > > > > not in parent / child relations). > > > > > > > > > > What was happened when SPI would like to do a DMA transfer and DMA is > > > > > going to sleep? I'm trying to understand if this is a case. > > > > In that case how does no irq version help us? > > > > > > It guarantees that we have no user of DMA anymore, since there is no > > > interrupt going on. > > well how is that. It will gaurantee that there wont be interrupt. User can still > > submit a transaction or another transaction will be in progress... > > This is how system suspend callback tree is called. > > First it calls .suspend() for all devices, then .suspend_late(), > then .suspend_noirq(). > > There is set of assumptions per each callback round. After .suspend() > the device must be quiescent. > > But... > > > > > For these cases, I have been using suspend_late. Since the dmaengine driver is > > > > providing service to other clients (SPI), it needs to esnure that it suspends > > > > after SPI using suspend_late and resume using resume_early. That way dma is > > > > availble whenever the client is active > > > > > > suspend_late is working in context that interrupt handler may be > > > invoked. Thus, to have DMA driver be properly shut down we have to > > > wait / terminate possible ongoing transfer. > > Well client is already suspended via .suspend. So where is the transaction :) > > ...as I already wrote before we have no parent-child relationship > between DMA and, for example, SPI. That means we may possible have the > case when SPI's .suspend() will be called later than DMA's one. > > > > It seems for me all DMA drivers that are using > > > system .suspend()/.resume() are potentially buggy. > > Yup! > > So, we have to decide what to do with them. .suspend_late() still seems > for me not the best approach. *Or* we have to check for ongoing > transaction and do something with it. *Or* just shut down the device and > rely on DMA transaction initiator that it handles the terminated > transaction properly. As you clearly said, we dont have a parent-child relatation though we have big dependency. I think this is true for DMA clients, i ran into similar situation with i2c few days back! So only think which can give us a good system behaviour would be clients getting suspended first and then then service providing subsystems. (same reason why we do dma driver loading and init much before others drivers) So yes in the .suspend callback of the client, it needs to 1) abort any transactions it has 2) make the client quiscent then the .late_suspend kicks in and suspend the core drivers like dma. This is how it has worked reliably for me in production systems. I am all ears if we have a better and cleaner apprach to this problem :)
On Sun, 2014-01-26 at 16:47 +0530, Vinod Koul wrote: > > > > > For these cases, I have been using suspend_late. Since the dmaengine driver is > > > > > providing service to other clients (SPI), it needs to esnure that it suspends > > > > > after SPI using suspend_late and resume using resume_early. That way dma is > > > > > availble whenever the client is active > > > > > > > > suspend_late is working in context that interrupt handler may be > > > > invoked. Thus, to have DMA driver be properly shut down we have to > > > > wait / terminate possible ongoing transfer. > > > Well client is already suspended via .suspend. So where is the transaction :) > > > > ...as I already wrote before we have no parent-child relationship > > between DMA and, for example, SPI. That means we may possible have the > > case when SPI's .suspend() will be called later than DMA's one. > > > > > > It seems for me all DMA drivers that are using > > > > system .suspend()/.resume() are potentially buggy. > > > Yup! > > > > So, we have to decide what to do with them. .suspend_late() still seems > > for me not the best approach. *Or* we have to check for ongoing > > transaction and do something with it. *Or* just shut down the device and > > rely on DMA transaction initiator that it handles the terminated > > transaction properly. > > As you clearly said, we dont have a parent-child relatation though we have big > dependency. I think this is true for DMA clients, i ran into similar situation > with i2c few days back! > > So only think which can give us a good system behaviour would be clients getting > suspended first and then then service providing subsystems. Agree. > (same reason why we > do dma driver loading and init much before others drivers) Yes, it would be done via deferred probe. > So yes in the .suspend callback of the client, it needs to > 1) abort any transactions it has > 2) make the client quiscent > > then the .late_suspend kicks in and suspend the core drivers like dma. > > This is how it has worked reliably for me in production systems. I am all ears > if we have a better and cleaner apprach to this problem :) Yes, summarize everything we discussed we have to: - provide suspend_late, resume_early callbacks in the DMA driver instead of *_noirq versions - ensure that all clients on our platforms follow the described scenario Chiau Ee, I think you may to change your patch accordingly.
> -----Original Message----- > From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com] > Sent: Monday, January 27, 2014 6:17 PM > To: Koul, Vinod > Cc: Andy Shevchenko; Chew, Chiau Ee; Viresh Kumar; Williams, Dan J; > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode > DW_DMAC. > > On Sun, 2014-01-26 at 16:47 +0530, Vinod Koul wrote: > > > > > > > For these cases, I have been using suspend_late. Since the > > > > > > dmaengine driver is providing service to other clients (SPI), > > > > > > it needs to esnure that it suspends after SPI using > > > > > > suspend_late and resume using resume_early. That way dma is > > > > > > availble whenever the client is active > > > > > > > > > > suspend_late is working in context that interrupt handler may be > > > > > invoked. Thus, to have DMA driver be properly shut down we have > > > > > to wait / terminate possible ongoing transfer. > > > > Well client is already suspended via .suspend. So where is the > > > > transaction :) > > > > > > ...as I already wrote before we have no parent-child relationship > > > between DMA and, for example, SPI. That means we may possible have > > > the case when SPI's .suspend() will be called later than DMA's one. > > > > > > > > It seems for me all DMA drivers that are using system > > > > > .suspend()/.resume() are potentially buggy. > > > > Yup! > > > > > > So, we have to decide what to do with them. .suspend_late() still > > > seems for me not the best approach. *Or* we have to check for > > > ongoing transaction and do something with it. *Or* just shut down > > > the device and rely on DMA transaction initiator that it handles the > > > terminated transaction properly. > > > > As you clearly said, we dont have a parent-child relatation though we > > have big dependency. I think this is true for DMA clients, i ran into > > similar situation with i2c few days back! > > > > So only think which can give us a good system behaviour would be > > clients getting suspended first and then then service providing subsystems. > > Agree. > > > (same reason why we > > do dma driver loading and init much before others drivers) > > Yes, it would be done via deferred probe. > > > So yes in the .suspend callback of the client, it needs to > > 1) abort any transactions it has > > 2) make the client quiscent > > > > then the .late_suspend kicks in and suspend the core drivers like dma. > > > > This is how it has worked reliably for me in production systems. I am > > all ears if we have a better and cleaner apprach to this problem :) > > Yes, summarize everything we discussed we have to: > - provide suspend_late, resume_early callbacks in the DMA driver instead of > *_noirq versions > - ensure that all clients on our platforms follow the described scenario > > Chiau Ee, I think you may to change your patch accordingly. Ok. Noted > > > -- > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy
diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c index e89fc24..97bc3a2 100644 --- a/drivers/dma/dw/pci.c +++ b/drivers/dma/dw/pci.c @@ -75,6 +75,36 @@ static void dw_pci_remove(struct pci_dev *pdev) dev_warn(&pdev->dev, "can't remove device properly: %d\n", ret); } +#ifdef CONFIG_PM_SLEEP + +static int dw_pci_suspend_noirq(struct device *dev) +{ + struct pci_dev *pci = to_pci_dev(dev); + struct dw_dma_chip *chip = pci_get_drvdata(pci); + + return dw_dma_suspend(chip); +}; + +static int dw_pci_resume_noirq(struct device *dev) +{ + struct pci_dev *pci = to_pci_dev(dev); + struct dw_dma_chip *chip = pci_get_drvdata(pci); + + return dw_dma_resume(chip); +}; + +#else /* !CONFIG_PM_SLEEP */ + +#define dw_pci_suspend_noirq NULL +#define dw_pci_resume_noirq NULL + +#endif /* !CONFIG_PM_SLEEP */ + +static const struct dev_pm_ops dw_pci_dev_pm_ops = { + .suspend_noirq = dw_pci_suspend_noirq, + .resume_noirq = dw_pci_resume_noirq, +}; + static DEFINE_PCI_DEVICE_TABLE(dw_pci_id_table) = { /* Medfield */ { PCI_VDEVICE(INTEL, 0x0827), (kernel_ulong_t)&dw_pci_pdata }, @@ -92,6 +122,9 @@ static struct pci_driver dw_pci_driver = { .id_table = dw_pci_id_table, .probe = dw_pci_probe, .remove = dw_pci_remove, + .driver = { + .pm = &dw_pci_dev_pm_ops, + }, }; module_pci_driver(dw_pci_driver);