Re: [PATCH 1/6] usb: dwc3: Support Synopsys USB 3.1 IP

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 10397 bytes --]

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,
> > 
> > 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.
> > 
> > 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.
> > 
> > It's more work, but it seems better IMO.
> 
> 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 when
it's really just a device id. dwc31 requires a bunch of other changes to get 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.
> > 
> > and this is why I prefer to have the flag :-) We can run different revision
> > check methods depending if we're running on dwc3 or dwc31.
> 
> 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 |= DWC3_DCFG_SUPERSPEED;
	} else {
	...

So this is a check that's only valid for DWC3 because DWC3 was the one which had
this bug, not DWC31. In order to skip this for DWC31 we should have something
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 example,
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 would
look like:

     reg = dwc3_readl(dwc->regs, DWC3_VER_NUMBER);
     
     /**
      * 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 |= 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: <stable@vger.kernel.org> # v3.18+
> > 
> > I really fail to how any of these patches in this series apply for stable. Care
> > to explain ?
> 
> 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.
> 
> I think the plan is to update those platforms to 4.2 eventually.
> So even then it will still need this patch.
> 
> Also it will help out any customers stuck on earlier kernels.
> 
> 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.
> 
> 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).
> 
> 
> 
> > 
> >> Signed-off-by: John Youn <johnyoun@synopsys.com>
> >> ---
> >>  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)
> >>  
> >>  	reg = dwc3_readl(dwc->regs, DWC3_GSNPSID);
> >>  	/* This should read as U3 followed by revision number */
> >> -	if ((reg & DWC3_GSNPSID_MASK) != 0x55330000) {
> >> +	if ((reg & DWC3_GSNPSID_MASK) == 0x55330000) {
> >> +		/* Detected DWC_usb3 IP */
> >> +		dwc->revision = reg & DWC3_GSNPSREV_MASK;
> > 
> > leave the mask out of it and ...
> > 
> >> +	} else if ((reg & DWC3_GSNPSID_MASK) == 0x33310000) {
> >> +		/* Detected DWC_usb31 IP */
> >> +		dwc->revision = dwc3_readl(dwc->regs, DWC3_VER_NUMBER);
> > 
> > set a dwc->is_dwc31 = true flag here.
> > 
> >> +	} else {
> >>  		dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
> >>  		ret = -ENODEV;
> >>  		goto err0;
> >>  	}
> >> -	dwc->revision = reg;
> >>  
> >>  	/*
> >>  	 * Write Linux Version Code to our GUID register so it's easy to figure
> >> 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
> >>  
> >> +#define DWC3_VER_NUMBER		0xc1a0
> >> +#define DWC3_VER_TYPE		0xc1a4
> > 
> > what is this VER_TYPE register ?
> 
> 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 purposes)
> >> - * @revision: revision register contents
> >> + * @revision: the core revision. the contents will depend on the whether
> >> + *            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;
> >>  
> >> -#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
> > 
> > yeah, I'd rather not do all this.
> > 
> >> +/* 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
> > 
> > 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 actually 3.02a
> > in HW, is this really correct ?
> > 
> 
> 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.
> 
> 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 great to
maintain the previous method and just make sure it's higher then dwc3.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]