From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from devils.ext.ti.com ([198.47.26.153]:42883 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752234AbbJBOFK (ORCPT ); Fri, 2 Oct 2015 10:05:10 -0400 Date: Fri, 2 Oct 2015 09:05:02 -0500 From: Felipe Balbi To: John Youn CC: "balbi@ti.com" , "linux-usb@vger.kernel.org" , "david.fisher1@synopsys.com" , "stable@vger.kernel.org" Subject: Re: [PATCH 1/6] usb: dwc3: Support Synopsys USB 3.1 IP Message-ID: <20151002140502.GC5552@saruman.tx.rr.com> Reply-To: References: <20151002020355.GB2534@saruman.tx.rr.com> <2B3535C5ECE8B5419E3ECBE30077290901DC384A94@US01WEMBX2.internal.synopsys.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="KDt/GgjP6HVcx58l" Content-Disposition: inline In-Reply-To: <2B3535C5ECE8B5419E3ECBE30077290901DC384A94@US01WEMBX2.internal.synopsys.com> Sender: stable-owner@vger.kernel.org List-ID: --KDt/GgjP6HVcx58l Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable HBi, On Fri, Oct 02, 2015 at 03:09:57AM +0000, John Youn wrote: > On 10/1/2015 7:03 PM, Felipe Balbi wrote: > > Hi, > >=20 > > On Fri, Sep 04, 2015 at 07:15:10PM -0700, John Youn wrote: > >> This patch allows the dwc3 driver to run on the new Synopsys USB 3.1 > >> IP core, albeit in USB 3.0 mode only. > >> > >> The Synopsys USB 3.1 IP (DWC_usb31) retains mostly the same register > >> interface and programming model as the existing USB 3.0 controller IP > >> (DWC_usb3). However, the underlying IP is different and the GSNPSID > >> and version numbers are different. > >> > >> The DWC_usb31 version register is actually lower in value than the > >> full GSNPSID of the DWC_usb3 IP. So if we are on DWC_usb3 IP just > >> store the lower word of the GSNPSID instead of the full register. Then > >> adjust the revision constants to match. This will allow existing > >> revision checks to continue to work when running on the new IP. > >=20 > > I would rather not touch those constants at all. We can have the driver > > set a is_dwc31 flag and use if !is_dwc31 && rev < XYZ for the revision = checks. > >=20 > > It's more work, but it seems better IMO. >=20 > I initially did it like that but it got really messy and it would > make it harder to port back to stable kernels... except that this doesn't apply to stable kernels :-) Stable is strictly for regression fixes. We _do_ get the odd new device id into stable, but only w= hen it's really just a device id. dwc31 requires a bunch of other changes to ge= t it to work with current driver, it's not only a new device id, right ? > >> Finally add a documentation note about the revision numbering and > >> checking with regards to the old and new IP. Because these are > >> different IPs which will both continue to be supported, feature sets > >> and revisions checks may not sync-up across future versions. > >=20 > > and this is why I prefer to have the flag :-) We can run different revi= sion > > check methods depending if we're running on dwc3 or dwc31. >=20 > All of the existing checks apply to both. The mismatch condition > will be the exception. okay. Let's take one example: if (dwc->revision < DWC3_REVISION_220A) { reg |=3D DWC3_DCFG_SUPERSPEED; } else { ... So this is a check that's only valid for DWC3 because DWC3 was the one whic= h had this bug, not DWC31. In order to skip this for DWC31 we should have somethi= ng like: if (!dwc->is_dwc31 && dwc->revision < DWC3_REVISION_220A) { ... it looks awful, but this is only the case because SNPS made the revision of= the newer cores lower than the previous ones :-p if you used 0x55340000 for exa= mple, we wouldn't have this problem. Yeah, difficult choice. This is_dwc31 will look awful. How about using bit31 of the revision as a is_dwc31 flag ? We don't touch the older revisions and we're gonna be using a reserved bit as a flag. Then, the revision check wou= ld look like: reg =3D dwc3_readl(dwc->regs, DWC3_VER_NUMBER); =20 /** * NOTICE: we're using bit 31 as a "is dwc3.1" flag. This is really * just so dwc31 revisions are always larger than dwc3. */ reg |=3D DWC3_REVISION_IS_DWC31; > >> From now, any check based on a revision (for STARS, workarounds, and > >> new features) should take into consideration how it applies to both > >> the 3.1/3.0 IP and make the check accordingly. > >> > >> Cc: # v3.18+ > >=20 > > I really fail to how any of these patches in this series apply for stab= le. Care > > to explain ? >=20 > We have some prototyping products that are stuck on 3.18 stable > kernels and will continue to ship with that for some time. We'd > like to run the USB 3.1 controller on those platforms. Without > these version id and version number updates dwc3 will not work > with the USB 3.1 IP. >=20 > I think the plan is to update those platforms to 4.2 eventually. > So even then it will still need this patch. >=20 > Also it will help out any customers stuck on earlier kernels. >=20 > How would you advise we handle this, with the version id and > number changes? I have a feeling the answer to that will be that you will need to backport = your own patches :-( Or upgrade to the latest kernel around once your patches get merged. Would you care to explain why upgrading the kernel is so complex for this prototyping solution ? I suppose you're not using HAPS as a PCIe card on a common desktop PC, then ? > I can make the changes as you suggest but it might be a pain > to apply it to stable kernels. >=20 > The other patches in this series tagged for stable are to > support these same platforms on 3.18+. Either so that they > can work at all (such as missing PCI IDs) or to pass some > compliance tests (LPM flags in platform data, enblslpm quirk). >=20 >=20 >=20 > >=20 > >> Signed-off-by: John Youn > >> --- > >> drivers/usb/dwc3/core.c | 9 ++++++-- > >> drivers/usb/dwc3/core.h | 56 +++++++++++++++++++++++++++++++---------= --------- > >> 2 files changed, 43 insertions(+), 22 deletions(-) > >> > >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >> index c72c8c5..566cca1 100644 > >> --- a/drivers/usb/dwc3/core.c > >> +++ b/drivers/usb/dwc3/core.c > >> @@ -534,12 +534,17 @@ static int dwc3_core_init(struct dwc3 *dwc) > >> =20 > >> reg =3D dwc3_readl(dwc->regs, DWC3_GSNPSID); > >> /* This should read as U3 followed by revision number */ > >> - if ((reg & DWC3_GSNPSID_MASK) !=3D 0x55330000) { > >> + if ((reg & DWC3_GSNPSID_MASK) =3D=3D 0x55330000) { > >> + /* Detected DWC_usb3 IP */ > >> + dwc->revision =3D reg & DWC3_GSNPSREV_MASK; > >=20 > > leave the mask out of it and ... > >=20 > >> + } else if ((reg & DWC3_GSNPSID_MASK) =3D=3D 0x33310000) { > >> + /* Detected DWC_usb31 IP */ > >> + dwc->revision =3D dwc3_readl(dwc->regs, DWC3_VER_NUMBER); > >=20 > > set a dwc->is_dwc31 =3D true flag here. > >=20 > >> + } else { > >> dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n"); > >> ret =3D -ENODEV; > >> goto err0; > >> } > >> - dwc->revision =3D reg; > >> =20 > >> /* > >> * Write Linux Version Code to our GUID register so it's easy to fig= ure > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > >> index 9188745..7446467 100644 > >> --- a/drivers/usb/dwc3/core.h > >> +++ b/drivers/usb/dwc3/core.h > >> @@ -108,6 +108,9 @@ > >> #define DWC3_GPRTBIMAP_FS0 0xc188 > >> #define DWC3_GPRTBIMAP_FS1 0xc18c > >> =20 > >> +#define DWC3_VER_NUMBER 0xc1a0 > >> +#define DWC3_VER_TYPE 0xc1a4 > >=20 > > what is this VER_TYPE register ? >=20 > It gives the release type, EA, GA, etc. It's not used so I can > leave it out if you want. no, we can keep it here. Was just curious :-) > >> @@ -661,7 +664,8 @@ struct dwc3_scratchpad_array { > >> * @num_event_buffers: calculated number of event buffers > >> * @u1u2: only used on revisions <1.83a for workaround > >> * @maximum_speed: maximum speed requested (mainly for testing purpos= es) > >> - * @revision: revision register contents > >> + * @revision: the core revision. the contents will depend on the whet= her > >> + * this is a usb3 or usb31 core. > >> * @dr_mode: requested mode of operation > >> * @usb2_phy: pointer to USB2 PHY > >> * @usb3_phy: pointer to USB3 PHY > >> @@ -771,27 +775,39 @@ struct dwc3 { > >> u32 num_event_buffers; > >> u32 u1u2; > >> u32 maximum_speed; > >> + > >> + /* > >> + * All 3.1 IP version constants are greater than the 3.0 IP > >> + * version constants. This works for most version checks in > >> + * dwc3. However, in the future, this may not apply as > >> + * features may be developed on newer versions of the 3.0 IP > >> + * that are not in the 3.1 IP. > >> + */ > >> u32 revision; > >> =20 > >> -#define DWC3_REVISION_173A 0x5533173a > >> -#define DWC3_REVISION_175A 0x5533175a > >> -#define DWC3_REVISION_180A 0x5533180a > >> -#define DWC3_REVISION_183A 0x5533183a > >> -#define DWC3_REVISION_185A 0x5533185a > >> -#define DWC3_REVISION_187A 0x5533187a > >> -#define DWC3_REVISION_188A 0x5533188a > >> -#define DWC3_REVISION_190A 0x5533190a > >> -#define DWC3_REVISION_194A 0x5533194a > >> -#define DWC3_REVISION_200A 0x5533200a > >> -#define DWC3_REVISION_202A 0x5533202a > >> -#define DWC3_REVISION_210A 0x5533210a > >> -#define DWC3_REVISION_220A 0x5533220a > >> -#define DWC3_REVISION_230A 0x5533230a > >> -#define DWC3_REVISION_240A 0x5533240a > >> -#define DWC3_REVISION_250A 0x5533250a > >> -#define DWC3_REVISION_260A 0x5533260a > >> -#define DWC3_REVISION_270A 0x5533270a > >> -#define DWC3_REVISION_280A 0x5533280a > >=20 > > yeah, I'd rather not do all this. > >=20 > >> +/* DWC_usb3 revisions */ > >> +#define DWC3_REVISION_173A 0x173a > >> +#define DWC3_REVISION_175A 0x175a > >> +#define DWC3_REVISION_180A 0x180a > >> +#define DWC3_REVISION_183A 0x183a > >> +#define DWC3_REVISION_185A 0x185a > >> +#define DWC3_REVISION_187A 0x187a > >> +#define DWC3_REVISION_188A 0x188a > >> +#define DWC3_REVISION_190A 0x190a > >> +#define DWC3_REVISION_194A 0x194a > >> +#define DWC3_REVISION_200A 0x200a > >> +#define DWC3_REVISION_202A 0x202a > >> +#define DWC3_REVISION_210A 0x210a > >> +#define DWC3_REVISION_220A 0x220a > >> +#define DWC3_REVISION_230A 0x230a > >> +#define DWC3_REVISION_240A 0x240a > >> +#define DWC3_REVISION_250A 0x250a > >> +#define DWC3_REVISION_260A 0x260a > >> +#define DWC3_REVISION_270A 0x270a > >> +#define DWC3_REVISION_280A 0x280a > >> + > >> +/* DWC_usb31 revisions */ > >> +#define DWC3_USB31_REVISION_110A 0x3131302a > >=20 > > are you sure you tested this ? Above you check for 0x33310000 but here = you use > > 0x3131 ? What gives ? Also, it seems odd that revision 1.10a is actuall= y 3.02a > > in HW, is this really correct ? > >=20 >=20 > The one in the source file is wrong. I did run it but not sure > how it was working... maybe wrong bitfile. I'll look into it > and fix it. >=20 > The version value is actually ASCII using all 4 > bytes: "110*". The last 'a' is replaced with '*' in the register > as that indicates a documentation only change with no IP changes. and I suppose it's already too late to change that :-p It would've been gre= at to maintain the previous method and just make sure it's higher then dwc3. --=20 balbi --KDt/GgjP6HVcx58l Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWDo8OAAoJEIaOsuA1yqREL48P/j0sWOpnGdnhoctuIWsOzk20 TS2UUojRTGrNK9HYqHUGTjgCZxvqzWcGsXRZ8XKP63vqkNYfGa2M8CvO9osrYc2n Hl64L/c8VFPhpBhyLrOWm9sdO+ppwNg583BbzD/HXrP5jhT0sQNGqkceLHLXyMx6 FbEQvymY+cye6UjaOtHMxRBMHfGX9me7dDo8yDQXaEMAju/4CkM/4OhBOQPsOCu6 ezg1xfYMXnLDnaS3fYCNJ3P4lD1fW/cRrpFA1HysdoBuH0USlR9H2DqWUiBGVqRC erC6Vg/1Cx2TFiyDWMbfUuiWGSFh3kDkQ6+eh2A/SF5irs/A1+LqjzlzB99eekbP wVeBFYPVTRvpRvhuEqGyfp9Rx65WeoGCnaNWlHA2pj1VcOw4dmfyVOSu5oSfto4c +A6Ix+kajxGFDElrvBGPBk5RlsYTB1DoN6ddeA6r2athlat+InLaBWpRuIDWFIAq 7NE1BUnIP2fSeH3COJPx2iZaw1g8CNoZOytUzwdeZNYgOaYydO/NB9s/AxC1puOw CPcikHcyniQ906OKweXRy7BRApnS3hxp5UKaRrhRqfCQ0ATaane19/IZwRsY4JXL fIPQnbCBhFJ/G7tFXlAa+hJ0spsrzaiakRxGzJb92kSU9T5X7+wbuU1epHix4/fS 5VhUZEXGVRHR3fXkNADM =1c1m -----END PGP SIGNATURE----- --KDt/GgjP6HVcx58l--