linux-kernel - Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150723145532.GC21984@saruman.tx.rr.com>
Date:	Thu, 23 Jul 2015 09:55:32 -0500
From:	Felipe Balbi <balbi@...com>
To:	Nikhil Badola <nikhil.badola@...escale.com>
CC:	<linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	<linux-omap@...r.kernel.org>, <balbi@...com>,
	<gregkh@...uxfoundation.org>
Subject: Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk

Hi,

On Thu, Jul 23, 2015 at 03:41:35PM +0530, Nikhil Badola wrote:
> Add adjust_frame_length_quirk for writing to fladj register
> which adjusts (micro)frame length to value provided by
> "snps,configure-fladj" property thus avoiding USB 2.0 devices
> to time-out over a longer run
> 
> Signed-off-by: Nikhil Badola <nikhil.badola@...escale.com>

just like the other patch, I won't take this without a glue layer making
use of it.

> ---
>  drivers/usb/dwc3/core.c | 12 ++++++++++++
>  drivers/usb/dwc3/core.h |  7 +++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 5c110d8..72ba025 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -779,6 +779,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  	u8			lpm_nyet_threshold;
>  	u8			tx_de_emphasis;
>  	u8			hird_threshold;
> +	u32			fladj_value;
>  
>  	int			ret;
>  
> @@ -886,6 +887,12 @@ static int dwc3_probe(struct platform_device *pdev)
>  				&tx_de_emphasis);
>  		of_property_read_string(node, "snps,hsphy_interface",
>  					&dwc->hsphy_interface);
> +		ret = of_property_read_u32(node, "snps,configure-fladj",

This is not the correct name for the property, it should be something
like 'snps,quirk-frame-length-adjustment'. Quirk because this is only
needed when the 30MHz sideband signal is invalid for some reason.

This is also something that's only needed for host side operation, so
consider what would you do if you weren't using dwc3, if all you had was
XHCI.

I hope freescale will fix this silicon bug.

One extra thing: everything that can be done via DT, should be available
for pdata users as well.

> +					   &fladj_value);
> +		if (!ret)
> +			dwc->adjust_frame_length_quirk = 1;
> +		else
> +			dwc->adjust_frame_length_quirk = 0;

this flag is unnecessary. Just initialize fladj_value to 0 and check for
that:

>  	} else if (pdata) {
>  		dwc->maximum_speed = pdata->maximum_speed;
>  		dwc->has_lpm_erratum = pdata->has_lpm_erratum;
> @@ -957,6 +964,11 @@ static int dwc3_probe(struct platform_device *pdev)
>  		goto err1;
>  	}
>  
> +	/* Adjust Frame Length */
> +	if (dwc->adjust_frame_length_quirk)

	if (fladj) {
		u32 reg;
		u32 dft;

		reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
		dft = reg & 0x3f; /* needs a mask macro */

		if (!dev_WARN_ONCE(dwc->dev, dft == fladj,
			"request value same as default, ignoring\n")) {
			reg &= ~0x3f; /* needs a mask macro */
			reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
				DWC3_GFLADJ_30MHZ(fladj_value);

			dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
		}
	}

> +		dwc3_writel(dwc->regs, DWC3_GFLADJ, GFLADJ_30MHZ_REG_SEL |
> +			    GFLADJ_30MHZ(fladj_value));
> +
>  	if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
>  		dwc->dr_mode = USB_DR_MODE_HOST;
>  	else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 0447788..b7a5119 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -124,6 +124,7 @@
>  #define DWC3_GEVNTCOUNT(n)	(0xc40c + (n * 0x10))
>  
>  #define DWC3_GHWPARAMS8		0xc600
> +#define DWC3_GFLADJ		0xc630
>  
>  /* Device Registers */
>  #define DWC3_DCFG		0xc700
> @@ -234,6 +235,10 @@
>  /* Global HWPARAMS6 Register */
>  #define DWC3_GHWPARAMS6_EN_FPGA			(1 << 7)
>  
> +/* Global Frame Length Adjustment Register */
> +#define GFLADJ_30MHZ_REG_SEL		(1 << 7)

always prepend with DWC3_ like *all* other macros in this file.

Also, match docs to ease grepping. This should be called
DWC3_GFLADJ_30MHZ_SDBND_SEL

> +#define GFLADJ_30MHZ(n)			((n) & 0x3f)

> +
>  /* Device Configuration Register */
>  #define DWC3_DCFG_DEVADDR(addr)	((addr) << 3)
>  #define DWC3_DCFG_DEVADDR_MASK	DWC3_DCFG_DEVADDR(0x7f)
> @@ -712,6 +717,7 @@ struct dwc3_scratchpad_array {
>   * @rx_detect_poll_quirk: set if we enable rx_detect to polling lfps quirk
>   * @dis_u3_susphy_quirk: set if we disable usb3 suspend phy
>   * @dis_u2_susphy_quirk: set if we disable usb2 suspend phy
> + * @adjust_frame_length_quirk: enables post-silicon frame length adjustment
>   * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
>   * @tx_de_emphasis: Tx de-emphasis value
>   * 	0	- -6dB de-emphasis
> @@ -841,6 +847,7 @@ struct dwc3 {
>  	unsigned		rx_detect_poll_quirk:1;
>  	unsigned		dis_u3_susphy_quirk:1;
>  	unsigned		dis_u2_susphy_quirk:1;
> +	unsigned		adjust_frame_length_quirk:1;

unnecessary flag

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)