On 10/2/2015 7:05 AM, Felipe Balbi wrote:
> 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 ?
My understanding is that small fixes, new device IDs, and quirks
are some of the things appropriate to submit to stable. I think
everything here tagged for stable is in one of those categories.
This patch is the minimal change required just to get the driver
loaded and running with this new hardware. I would put it in the
same category of change as a new device ID. We just have a new
version ID that needs to be checked against so that the probe
doesn't fail.
We don't plan to do any more than this for older kernels, for
example to add support for USB 3.1 and gen 2 speed.
>
>>>> 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;
>
I'm good with that.
>>>> 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 don't know the technical reasons why. One platform is ARM based
and using the 3.18 Linaro stable kernel. Another uses our ARC
platform which is also on 3.18.
We're trying to avoid the situation where where we have to ship
patches or maintain a separate kernel tree.
Do you have any objections to these going into stable?
Regards,
John
>
>> 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.
>
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html