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@xxxxxxxxxxxxxxx> # 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@xxxxxxxxxxxx>
> >> ---
> >> 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:
signature.asc
Description: PGP signature
