| Message ID | 1413536021-4886-17-git-send-email-ray.huang@amd.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi, On Fri, Oct 17, 2014 at 04:53:41PM +0800, Huang Rui wrote: > AMD NL needs to suspend usb3 ss phy, but this doesn't enable on simulation > board. > > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > drivers/usb/dwc3/core.c | 7 ++++++- > drivers/usb/dwc3/dwc3-pci.c | 3 ++- > drivers/usb/dwc3/platform_data.h | 1 + > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 3ccfe41..4a98696 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -395,6 +395,9 @@ static void dwc3_phy_setup(struct dwc3 *dwc) > if (dwc->quirks & DWC3_QUIRK_TX_DEEPH) > reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(1); > > + if (dwc->quirks & DWC3_QUIRK_SUSPHY) should be: if (!dwc->suspend_usb3_phy_quirk) > + reg |= DWC3_GUSB3PIPECTL_SUSPHY; IIRC, databook asks us to set that bit anyway, so the quirk is disabling that bit. Am I missing something ? Paul ? > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c > index 146eb2f..71401a3 100644 > --- a/drivers/usb/dwc3/dwc3-pci.c > +++ b/drivers/usb/dwc3/dwc3-pci.c > @@ -157,7 +157,8 @@ static int dwc3_pci_probe(struct pci_dev *pci, > | DWC3_QUIRK_DEPOCHANGE > | DWC3_QUIRK_LFPSFILT > | DWC3_QUIRK_RX_DETOPOLL > - | DWC3_QUIRK_TX_DEEPH; > + | DWC3_QUIRK_TX_DEEPH > + | DWC3_QUIRK_SUSPHY; last patch
> From: Felipe Balbi [mailto:balbi@ti.com] > Sent: Friday, October 17, 2014 8:00 AM > > On Fri, Oct 17, 2014 at 04:53:41PM +0800, Huang Rui wrote: > > AMD NL needs to suspend usb3 ss phy, but this doesn't enable on simulation > > board. > > > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > --- > > drivers/usb/dwc3/core.c | 7 ++++++- > > drivers/usb/dwc3/dwc3-pci.c | 3 ++- > > drivers/usb/dwc3/platform_data.h | 1 + > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 3ccfe41..4a98696 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -395,6 +395,9 @@ static void dwc3_phy_setup(struct dwc3 *dwc) > > if (dwc->quirks & DWC3_QUIRK_TX_DEEPH) > > reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(1); > > > > + if (dwc->quirks & DWC3_QUIRK_SUSPHY) > > should be: > > if (!dwc->suspend_usb3_phy_quirk) > > > + reg |= DWC3_GUSB3PIPECTL_SUSPHY; > > IIRC, databook asks us to set that bit anyway, so the quirk is disabling > that bit. Am I missing something ? Paul ? It looks to me that Huang's patch is enabling that bit, not disabling it. Currently the driver does not set either DWC3_GUSB3PIPECTL_SUSPHY or DWC3_GUSB2PHYCFG_SUSPHY (unless it has been added by that big patch series you just posted). According to the databook, both of those bits should be set to 1 after the core initialization has completed. So I think the driver should be changed to enable both of those by default, and then maybe you want quirks to disable them if there is some platform out there which needs that.
Hi, On Fri, Oct 17, 2014 at 06:41:04PM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@ti.com] > > Sent: Friday, October 17, 2014 8:00 AM > > > > On Fri, Oct 17, 2014 at 04:53:41PM +0800, Huang Rui wrote: > > > AMD NL needs to suspend usb3 ss phy, but this doesn't enable on simulation > > > board. > > > > > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > > --- > > > drivers/usb/dwc3/core.c | 7 ++++++- > > > drivers/usb/dwc3/dwc3-pci.c | 3 ++- > > > drivers/usb/dwc3/platform_data.h | 1 + > > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index 3ccfe41..4a98696 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -395,6 +395,9 @@ static void dwc3_phy_setup(struct dwc3 *dwc) > > > if (dwc->quirks & DWC3_QUIRK_TX_DEEPH) > > > reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(1); > > > > > > + if (dwc->quirks & DWC3_QUIRK_SUSPHY) > > > > should be: > > > > if (!dwc->suspend_usb3_phy_quirk) > > > > > + reg |= DWC3_GUSB3PIPECTL_SUSPHY; > > > > IIRC, databook asks us to set that bit anyway, so the quirk is disabling > > that bit. Am I missing something ? Paul ? > > It looks to me that Huang's patch is enabling that bit, not disabling > it. right, but that's what's actually quirky right ? IIRC, Databook asks us to set usb2 and usb3 suspend phy bits and we're just not doing it. > Currently the driver does not set either DWC3_GUSB3PIPECTL_SUSPHY or > DWC3_GUSB2PHYCFG_SUSPHY (unless it has been added by that big patch > series you just posted). According to the databook, both of those > bits should be set to 1 after the core initialization has completed. there you go. So unless that quirk bit flag is set, we're safe of setting SUSPHY bit for everybody. > So I think the driver should be changed to enable both of those by > default, and then maybe you want quirks to disable them if there is > some platform out there which needs that. Yeah, that's what I thought too :-) Thanks for confirming
On Fri, Oct 17, 2014 at 01:48:19PM -0500, Felipe Balbi wrote: > Hi, > > On Fri, Oct 17, 2014 at 06:41:04PM +0000, Paul Zimmerman wrote: > > > From: Felipe Balbi [mailto:balbi@ti.com] > > > Sent: Friday, October 17, 2014 8:00 AM > > > > > > On Fri, Oct 17, 2014 at 04:53:41PM +0800, Huang Rui wrote: > > > > AMD NL needs to suspend usb3 ss phy, but this doesn't enable on simulation > > > > board. > > > > > > > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > > > --- > > > > drivers/usb/dwc3/core.c | 7 ++++++- > > > > drivers/usb/dwc3/dwc3-pci.c | 3 ++- > > > > drivers/usb/dwc3/platform_data.h | 1 + > > > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index 3ccfe41..4a98696 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -395,6 +395,9 @@ static void dwc3_phy_setup(struct dwc3 *dwc) > > > > if (dwc->quirks & DWC3_QUIRK_TX_DEEPH) > > > > reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(1); > > > > > > > > + if (dwc->quirks & DWC3_QUIRK_SUSPHY) > > > > > > should be: > > > > > > if (!dwc->suspend_usb3_phy_quirk) > > > > > > > + reg |= DWC3_GUSB3PIPECTL_SUSPHY; > > > > > > IIRC, databook asks us to set that bit anyway, so the quirk is disabling > > > that bit. Am I missing something ? Paul ? > > > > It looks to me that Huang's patch is enabling that bit, not disabling > > it. > > right, but that's what's actually quirky right ? IIRC, Databook asks us > to set usb2 and usb3 suspend phy bits and we're just not doing it. > > > Currently the driver does not set either DWC3_GUSB3PIPECTL_SUSPHY or > > DWC3_GUSB2PHYCFG_SUSPHY (unless it has been added by that big patch > > series you just posted). According to the databook, both of those > > bits should be set to 1 after the core initialization has completed. > > there you go. So unless that quirk bit flag is set, we're safe of > setting SUSPHY bit for everybody. > So I can update to set these two suspend phy bits defaultly in my next patch set, is it OK? :) Thanks, Rui > > So I think the driver should be changed to enable both of those by > > default, and then maybe you want quirks to disable them if there is > > some platform out there which needs that. > > Yeah, that's what I thought too :-) Thanks for confirming > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 20, 2014 at 04:41:54PM +0800, Huang Rui wrote: > On Fri, Oct 17, 2014 at 01:48:19PM -0500, Felipe Balbi wrote: > > Hi, > > > > On Fri, Oct 17, 2014 at 06:41:04PM +0000, Paul Zimmerman wrote: > > > > From: Felipe Balbi [mailto:balbi@ti.com] > > > > Sent: Friday, October 17, 2014 8:00 AM > > > > > > > > On Fri, Oct 17, 2014 at 04:53:41PM +0800, Huang Rui wrote: > > > > > AMD NL needs to suspend usb3 ss phy, but this doesn't enable on simulation > > > > > board. > > > > > > > > > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > > > > --- > > > > > drivers/usb/dwc3/core.c | 7 ++++++- > > > > > drivers/usb/dwc3/dwc3-pci.c | 3 ++- > > > > > drivers/usb/dwc3/platform_data.h | 1 + > > > > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > > index 3ccfe41..4a98696 100644 > > > > > --- a/drivers/usb/dwc3/core.c > > > > > +++ b/drivers/usb/dwc3/core.c > > > > > @@ -395,6 +395,9 @@ static void dwc3_phy_setup(struct dwc3 *dwc) > > > > > if (dwc->quirks & DWC3_QUIRK_TX_DEEPH) > > > > > reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(1); > > > > > > > > > > + if (dwc->quirks & DWC3_QUIRK_SUSPHY) > > > > > > > > should be: > > > > > > > > if (!dwc->suspend_usb3_phy_quirk) > > > > > > > > > + reg |= DWC3_GUSB3PIPECTL_SUSPHY; > > > > > > > > IIRC, databook asks us to set that bit anyway, so the quirk is disabling > > > > that bit. Am I missing something ? Paul ? > > > > > > It looks to me that Huang's patch is enabling that bit, not disabling > > > it. > > > > right, but that's what's actually quirky right ? IIRC, Databook asks us > > to set usb2 and usb3 suspend phy bits and we're just not doing it. > > > > > Currently the driver does not set either DWC3_GUSB3PIPECTL_SUSPHY or > > > DWC3_GUSB2PHYCFG_SUSPHY (unless it has been added by that big patch > > > series you just posted). According to the databook, both of those > > > bits should be set to 1 after the core initialization has completed. > > > > there you go. So unless that quirk bit flag is set, we're safe of > > setting SUSPHY bit for everybody. > > > I read the databook again, below words (DWC3_GUSB3PIPECTL_SUSPHY) is copied from databook: For DRD/OTG configurations, it is recommended that this bit is set to‘ 0’ during coreConsultant configuration. If it is set to ’1’, then the application should clear this bit after power-on reset. Application needs to set it to ’1’ after the core initialization is completed. For all other configurations, this bit can be set to ’1’ during core configuration. I see it's recommended to set '0' if on DRD/OTG configuration. Thanks, Rui -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PiBGcm9tOiBIdWFuZyBSdWkgW21haWx0bzpyYXkuaHVhbmdAYW1kLmNvbV0NCj4gU2VudDogTW9u ZGF5LCBPY3RvYmVyIDIwLCAyMDE0IDI6MDEgQU0NCj4gDQo+IE9uIE1vbiwgT2N0IDIwLCAyMDE0 IGF0IDA0OjQxOjU0UE0gKzA4MDAsIEh1YW5nIFJ1aSB3cm90ZToNCj4gPiBPbiBGcmksIE9jdCAx NywgMjAxNCBhdCAwMTo0ODoxOVBNIC0wNTAwLCBGZWxpcGUgQmFsYmkgd3JvdGU6DQo+ID4gPiBI aSwNCj4gPiA+DQo+ID4gPiBPbiBGcmksIE9jdCAxNywgMjAxNCBhdCAwNjo0MTowNFBNICswMDAw LCBQYXVsIFppbW1lcm1hbiB3cm90ZToNCj4gPiA+ID4gPiBGcm9tOiBGZWxpcGUgQmFsYmkgW21h aWx0bzpiYWxiaUB0aS5jb21dDQo+ID4gPiA+ID4gU2VudDogRnJpZGF5LCBPY3RvYmVyIDE3LCAy MDE0IDg6MDAgQU0NCj4gPiA+ID4gPg0KPiA+ID4gPiA+IE9uIEZyaSwgT2N0IDE3LCAyMDE0IGF0 IDA0OjUzOjQxUE0gKzA4MDAsIEh1YW5nIFJ1aSB3cm90ZToNCj4gPiA+ID4gPiA+IEFNRCBOTCBu ZWVkcyB0byBzdXNwZW5kIHVzYjMgc3MgcGh5LCBidXQgdGhpcyBkb2Vzbid0IGVuYWJsZSBvbiBz aW11bGF0aW9uDQo+ID4gPiA+ID4gPiBib2FyZC4NCj4gPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiBT aWduZWQtb2ZmLWJ5OiBIdWFuZyBSdWkgPHJheS5odWFuZ0BhbWQuY29tPg0KPiA+ID4gPiA+ID4g LS0tDQo+ID4gPiA+ID4gPiAgZHJpdmVycy91c2IvZHdjMy9jb3JlLmMgICAgICAgICAgfCA3ICsr KysrKy0NCj4gPiA+ID4gPiA+ICBkcml2ZXJzL3VzYi9kd2MzL2R3YzMtcGNpLmMgICAgICB8IDMg KystDQo+ID4gPiA+ID4gPiAgZHJpdmVycy91c2IvZHdjMy9wbGF0Zm9ybV9kYXRhLmggfCAxICsN Cj4gPiA+ID4gPiA+ICAzIGZpbGVzIGNoYW5nZWQsIDkgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlv bnMoLSkNCj4gPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy91c2Iv ZHdjMy9jb3JlLmMgYi9kcml2ZXJzL3VzYi9kd2MzL2NvcmUuYw0KPiA+ID4gPiA+ID4gaW5kZXgg M2NjZmU0MS4uNGE5ODY5NiAxMDA2NDQNCj4gPiA+ID4gPiA+IC0tLSBhL2RyaXZlcnMvdXNiL2R3 YzMvY29yZS5jDQo+ID4gPiA+ID4gPiArKysgYi9kcml2ZXJzL3VzYi9kd2MzL2NvcmUuYw0KPiA+ ID4gPiA+ID4gQEAgLTM5NSw2ICszOTUsOSBAQCBzdGF0aWMgdm9pZCBkd2MzX3BoeV9zZXR1cChz dHJ1Y3QgZHdjMyAqZHdjKQ0KPiA+ID4gPiA+ID4gIAlpZiAoZHdjLT5xdWlya3MgJiBEV0MzX1FV SVJLX1RYX0RFRVBIKQ0KPiA+ID4gPiA+ID4gIAkJcmVnIHw9IERXQzNfR1VTQjNQSVBFQ1RMX1RY X0RFRVBIKDEpOw0KPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+ICsJaWYgKGR3Yy0+cXVpcmtzICYg RFdDM19RVUlSS19TVVNQSFkpDQo+ID4gPiA+ID4NCj4gPiA+ID4gPiBzaG91bGQgYmU6DQo+ID4g PiA+ID4NCj4gPiA+ID4gPiAJaWYgKCFkd2MtPnN1c3BlbmRfdXNiM19waHlfcXVpcmspDQo+ID4g PiA+ID4NCj4gPiA+ID4gPiA+ICsJCXJlZyB8PSBEV0MzX0dVU0IzUElQRUNUTF9TVVNQSFk7DQo+ ID4gPiA+ID4NCj4gPiA+ID4gPiBJSVJDLCBkYXRhYm9vayBhc2tzIHVzIHRvIHNldCB0aGF0IGJp dCBhbnl3YXksIHNvIHRoZSBxdWlyayBpcyBkaXNhYmxpbmcNCj4gPiA+ID4gPiB0aGF0IGJpdC4g QW0gSSBtaXNzaW5nIHNvbWV0aGluZyA/IFBhdWwgPw0KPiA+ID4gPg0KPiA+ID4gPiBJdCBsb29r cyB0byBtZSB0aGF0IEh1YW5nJ3MgcGF0Y2ggaXMgZW5hYmxpbmcgdGhhdCBiaXQsIG5vdCBkaXNh YmxpbmcNCj4gPiA+ID4gaXQuDQo+ID4gPg0KPiA+ID4gcmlnaHQsIGJ1dCB0aGF0J3Mgd2hhdCdz IGFjdHVhbGx5IHF1aXJreSByaWdodCA/IElJUkMsIERhdGFib29rIGFza3MgdXMNCj4gPiA+IHRv IHNldCB1c2IyIGFuZCB1c2IzIHN1c3BlbmQgcGh5IGJpdHMgYW5kIHdlJ3JlIGp1c3Qgbm90IGRv aW5nIGl0Lg0KPiA+ID4NCj4gPiA+ID4gQ3VycmVudGx5IHRoZSBkcml2ZXIgZG9lcyBub3Qgc2V0 IGVpdGhlciBEV0MzX0dVU0IzUElQRUNUTF9TVVNQSFkgb3INCj4gPiA+ID4gRFdDM19HVVNCMlBI WUNGR19TVVNQSFkgKHVubGVzcyBpdCBoYXMgYmVlbiBhZGRlZCBieSB0aGF0IGJpZyBwYXRjaA0K PiA+ID4gPiBzZXJpZXMgeW91IGp1c3QgcG9zdGVkKS4gQWNjb3JkaW5nIHRvIHRoZSBkYXRhYm9v aywgYm90aCBvZiB0aG9zZQ0KPiA+ID4gPiBiaXRzIHNob3VsZCBiZSBzZXQgdG8gMSBhZnRlciB0 aGUgY29yZSBpbml0aWFsaXphdGlvbiBoYXMgY29tcGxldGVkLg0KPiA+ID4NCj4gPiA+IHRoZXJl IHlvdSBnby4gU28gdW5sZXNzIHRoYXQgcXVpcmsgYml0IGZsYWcgaXMgc2V0LCB3ZSdyZSBzYWZl IG9mDQo+ID4gPiBzZXR0aW5nIFNVU1BIWSBiaXQgZm9yIGV2ZXJ5Ym9keS4NCj4gPiA+DQo+ID4N Cj4gDQo+IEkgcmVhZCB0aGUgZGF0YWJvb2sgYWdhaW4sIGJlbG93IHdvcmRzIChEV0MzX0dVU0Iz UElQRUNUTF9TVVNQSFkpIGlzDQo+IGNvcGllZCBmcm9tIGRhdGFib29rOg0KPiANCj4gRm9yIERS RC9PVEcgY29uZmlndXJhdGlvbnMsIGl0IGlzIHJlY29tbWVuZGVkIHRoYXQgdGhpcyBiaXQgaXMg c2V0IHRv4oCYDQo+IDDigJkgZHVyaW5nIGNvcmVDb25zdWx0YW50IGNvbmZpZ3VyYXRpb24uIElm IGl0IGlzIHNldCB0byDigJkx4oCZLCB0aGVuIHRoZQ0KPiBhcHBsaWNhdGlvbiBzaG91bGQgY2xl YXIgdGhpcyBiaXQgYWZ0ZXIgcG93ZXItb24gcmVzZXQuIEFwcGxpY2F0aW9uDQo+IG5lZWRzIHRv IHNldCBpdCB0byDigJkx4oCZIGFmdGVyIHRoZSBjb3JlIGluaXRpYWxpemF0aW9uIGlzIGNvbXBs ZXRlZC4NCj4gRm9yIGFsbCBvdGhlciBjb25maWd1cmF0aW9ucywgdGhpcyBiaXQgY2FuIGJlIHNl dCB0byDigJkx4oCZIGR1cmluZyBjb3JlDQo+IGNvbmZpZ3VyYXRpb24uDQo+IA0KPiBJIHNlZSBp dCdzIHJlY29tbWVuZGVkIHRvIHNldCAnMCcgaWYgb24gRFJEL09URyBjb25maWd1cmF0aW9uLg0K DQpObywgaXQncyByZWNvbW1lbmRlZCB0byBzZXQgaXQgdG8gJzAnIGR1cmluZyBjb3JlQ29uc3Vs dGFudA0KY29uZmlndXJhdGlvbi4gVGhpcyBpcyBhIHBhcnQgb2YgdGhlIHN5bnRoZXNpcyBwcm9j ZXNzLCBpLmUuIHdoZW4NCmJ1aWxkaW5nIHRoZSBSVEwgZm9yIHRoZSBTb0MuIFRoaXMgZGV0ZXJt aW5lcyB3aGF0IHRoZSBkZWZhdWx0IHZhbHVlDQpvZiB0aGUgYml0IHdpbGwgYmUgd2hlbiB0aGUg Y29yZSBpcyByZXNldC4gQXQgcnVudGltZSBpdCBpcyBzdGlsbA0KcmVjb21tZW5kZWQgdG8gc2V0 IGl0IHRvICcxJyB3aGVuIGluIGRldmljZSBtb2RlLCBhZnRlciB0aGUgY29yZQ0KaW5pdGlhbGl6 YXRpb24gaXMgY29tcGxldGVkLg0KDQotLSANClBhdWwNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Oct 20, 2014 at 04:41:54PM +0800, Huang Rui wrote: > On Fri, Oct 17, 2014 at 01:48:19PM -0500, Felipe Balbi wrote: > > Hi, > > > > On Fri, Oct 17, 2014 at 06:41:04PM +0000, Paul Zimmerman wrote: > > > > From: Felipe Balbi [mailto:balbi@ti.com] > > > > Sent: Friday, October 17, 2014 8:00 AM > > > > > > > > On Fri, Oct 17, 2014 at 04:53:41PM +0800, Huang Rui wrote: > > > > > AMD NL needs to suspend usb3 ss phy, but this doesn't enable on simulation > > > > > board. > > > > > > > > > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > > > > --- > > > > > drivers/usb/dwc3/core.c | 7 ++++++- > > > > > drivers/usb/dwc3/dwc3-pci.c | 3 ++- > > > > > drivers/usb/dwc3/platform_data.h | 1 + > > > > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > > index 3ccfe41..4a98696 100644 > > > > > --- a/drivers/usb/dwc3/core.c > > > > > +++ b/drivers/usb/dwc3/core.c > > > > > @@ -395,6 +395,9 @@ static void dwc3_phy_setup(struct dwc3 *dwc) > > > > > if (dwc->quirks & DWC3_QUIRK_TX_DEEPH) > > > > > reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(1); > > > > > > > > > > + if (dwc->quirks & DWC3_QUIRK_SUSPHY) > > > > > > > > should be: > > > > > > > > if (!dwc->suspend_usb3_phy_quirk) > > > > > > > > > + reg |= DWC3_GUSB3PIPECTL_SUSPHY; > > > > > > > > IIRC, databook asks us to set that bit anyway, so the quirk is disabling > > > > that bit. Am I missing something ? Paul ? > > > > > > It looks to me that Huang's patch is enabling that bit, not disabling > > > it. > > > > right, but that's what's actually quirky right ? IIRC, Databook asks us > > to set usb2 and usb3 suspend phy bits and we're just not doing it. > > > > > Currently the driver does not set either DWC3_GUSB3PIPECTL_SUSPHY or > > > DWC3_GUSB2PHYCFG_SUSPHY (unless it has been added by that big patch > > > series you just posted). According to the databook, both of those > > > bits should be set to 1 after the core initialization has completed. > > > > there you go. So unless that quirk bit flag is set, we're safe of > > setting SUSPHY bit for everybody. > > > > So I can update to set these two suspend phy bits defaultly in my next > patch set, is it OK? :) We need to split this into two patches: patch 1 adds missing SUSPHY bit for all cores above revision 1.94a at the end of probe() patch 2 adds a quirk which AMD needs so that setting USB3_SUSPHY bit is conditional on that quirk.
On Mon, Oct 20, 2014 at 05:01:25PM +0800, Huang Rui wrote: > On Mon, Oct 20, 2014 at 04:41:54PM +0800, Huang Rui wrote: > > On Fri, Oct 17, 2014 at 01:48:19PM -0500, Felipe Balbi wrote: > > > Hi, > > > > > > On Fri, Oct 17, 2014 at 06:41:04PM +0000, Paul Zimmerman wrote: > > > > > From: Felipe Balbi [mailto:balbi@ti.com] > > > > > Sent: Friday, October 17, 2014 8:00 AM > > > > > > > > > > On Fri, Oct 17, 2014 at 04:53:41PM +0800, Huang Rui wrote: > > > > > > AMD NL needs to suspend usb3 ss phy, but this doesn't enable on simulation > > > > > > board. > > > > > > > > > > > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > > > > > --- > > > > > > drivers/usb/dwc3/core.c | 7 ++++++- > > > > > > drivers/usb/dwc3/dwc3-pci.c | 3 ++- > > > > > > drivers/usb/dwc3/platform_data.h | 1 + > > > > > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > > > index 3ccfe41..4a98696 100644 > > > > > > --- a/drivers/usb/dwc3/core.c > > > > > > +++ b/drivers/usb/dwc3/core.c > > > > > > @@ -395,6 +395,9 @@ static void dwc3_phy_setup(struct dwc3 *dwc) > > > > > > if (dwc->quirks & DWC3_QUIRK_TX_DEEPH) > > > > > > reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(1); > > > > > > > > > > > > + if (dwc->quirks & DWC3_QUIRK_SUSPHY) > > > > > > > > > > should be: > > > > > > > > > > if (!dwc->suspend_usb3_phy_quirk) > > > > > > > > > > > + reg |= DWC3_GUSB3PIPECTL_SUSPHY; > > > > > > > > > > IIRC, databook asks us to set that bit anyway, so the quirk is disabling > > > > > that bit. Am I missing something ? Paul ? > > > > > > > > It looks to me that Huang's patch is enabling that bit, not disabling > > > > it. > > > > > > right, but that's what's actually quirky right ? IIRC, Databook asks us > > > to set usb2 and usb3 suspend phy bits and we're just not doing it. > > > > > > > Currently the driver does not set either DWC3_GUSB3PIPECTL_SUSPHY or > > > > DWC3_GUSB2PHYCFG_SUSPHY (unless it has been added by that big patch > > > > series you just posted). According to the databook, both of those > > > > bits should be set to 1 after the core initialization has completed. > > > > > > there you go. So unless that quirk bit flag is set, we're safe of > > > setting SUSPHY bit for everybody. > > > > > > > I read the databook again, below words (DWC3_GUSB3PIPECTL_SUSPHY) is > copied from databook: > > For DRD/OTG configurations, it is recommended that this bit is set to‘ > 0’ during coreConsultant configuration. If it is set to ’1’, then the > application should clear this bit after power-on reset. Application > needs to set it to ’1’ after the core initialization is completed. > For all other configurations, this bit can be set to ’1’ during core > configuration. > > I see it's recommended to set '0' if on DRD/OTG configuration. 0 at the beginning of probe() sequence so core can communicate with its PHYs, then set it to one at the end of probe().
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 3ccfe41..4a98696 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -395,6 +395,9 @@ static void dwc3_phy_setup(struct dwc3 *dwc) if (dwc->quirks & DWC3_QUIRK_TX_DEEPH) reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(1); + if (dwc->quirks & DWC3_QUIRK_SUSPHY) + reg |= DWC3_GUSB3PIPECTL_SUSPHY; + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); mdelay(100); @@ -496,8 +499,10 @@ static int dwc3_core_init(struct dwc3 *dwc) dwc->is_fpga = true; } - if ((dwc->quirks & DWC3_QUIRK_AMD_NL) && dwc->is_fpga) + if ((dwc->quirks & DWC3_QUIRK_AMD_NL) && dwc->is_fpga) { dwc->quirks |= DWC3_QUIRK_DISSCRAMBLE; + dwc->quirks &= ~DWC3_QUIRK_SUSPHY; + } if (dwc->quirks & DWC3_QUIRK_DISSCRAMBLE) reg |= DWC3_GCTL_DISSCRAMBLE; diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c index 146eb2f..71401a3 100644 --- a/drivers/usb/dwc3/dwc3-pci.c +++ b/drivers/usb/dwc3/dwc3-pci.c @@ -157,7 +157,8 @@ static int dwc3_pci_probe(struct pci_dev *pci, | DWC3_QUIRK_DEPOCHANGE | DWC3_QUIRK_LFPSFILT | DWC3_QUIRK_RX_DETOPOLL - | DWC3_QUIRK_TX_DEEPH; + | DWC3_QUIRK_TX_DEEPH + | DWC3_QUIRK_SUSPHY; } ret = platform_device_add_resources(dwc3, res, ARRAY_SIZE(res)); diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h index 54f0e45..f68cd97 100644 --- a/drivers/usb/dwc3/platform_data.h +++ b/drivers/usb/dwc3/platform_data.h @@ -38,5 +38,6 @@ struct dwc3_platform_data { #define DWC3_QUIRK_LFPSFILT (1 << 7) #define DWC3_QUIRK_RX_DETOPOLL (1 << 8) #define DWC3_QUIRK_TX_DEEPH (1 << 9) +#define DWC3_QUIRK_SUSPHY (1 << 10) };
AMD NL needs to suspend usb3 ss phy, but this doesn't enable on simulation board. Signed-off-by: Huang Rui <ray.huang@amd.com> --- drivers/usb/dwc3/core.c | 7 ++++++- drivers/usb/dwc3/dwc3-pci.c | 3 ++- drivers/usb/dwc3/platform_data.h | 1 + 3 files changed, 9 insertions(+), 2 deletions(-)