RE: [PATCH v2 16/16] usb: dwc3: enable usb suspend phy

[Paul Zimmerman]

> From: Huang Rui [mailto:ray.huang@xxxxxxx]
> Sent: Monday, October 20, 2014 2:01 AM
> 
> 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@xxxxxx]
> > > > > 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@xxxxxxx>
> > > > > > ---
> > > > > >  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.

No, it's recommended to set it to '0' during coreConsultant
configuration. This is a part of the synthesis process, i.e. when
building the RTL for the SoC. This determines what the default value
of the bit will be when the core is reset. At runtime it is still
recommended to set it to '1' when in device mode, after the core
initialization is completed.

-- 
Paul

N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå