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

[Felipe Balbi <balbi@ti.com>]

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

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.

> 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.

> 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 ?

> 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 ?

> @@ -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 ?

-- 
balbi

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