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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 20 Oct 2014 18:17:42 +0000
From:	Paul Zimmerman <Paul.Zimmerman@...opsys.com>
To:	Huang Rui <ray.huang@....com>, Felipe Balbi <balbi@...com>
CC:	Alan Stern <stern@...land.harvard.edu>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Vincent Wan <vincent.wan@....com>, Tony Li <tony.li@....com>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 16/16] usb: dwc3: enable usb suspend phy

> From: Huang Rui [mailto:ray.huang@....com]
> 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@...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@....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.

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