Re: [RFC PATCH 07/14] usb: dwc3: gadget: Set lane count and lsm

From: Felipe Balbi <balbi@kernel.org>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	linux-usb@vger.kernel.org
Cc: John Youn <John.Youn@synopsys.com>
Subject: Re: [RFC PATCH 07/14] usb: dwc3: gadget: Set lane count and lsm
Date: Thu, 12 Dec 2019 10:14:57 +0200	[thread overview]
Message-ID: <87sglqvu2m.fsf@kernel.org> (raw)
In-Reply-To: <808d8ad39fc584e2f85e20148d56bd49c50aebf6.1576118671.git.thinhn@synopsys.com>

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

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:

> DWC_usb32 supports dual-lane at different transfer rate. This patch
> initializes the controller to use the maximum support transfer rate
> describes by the dwc3 device property of lane count and lane speed
> mantissa in Gbps.

Perhaps this should read as:

	"DWC_usb32 supports dual-lane at different transfer rate. This
	patch initializes the controller to use the maximum supported
	transfer rate described by the dwc3 device property of lane
	count and lane speed mantissa in Gbps."

> @@ -1424,6 +1428,38 @@ static void dwc3_check_params(struct dwc3 *dwc)
>  
>  		break;
>  	}
> +
> +	switch (dwc->maximum_lsm) {
> +	case 5:
> +		break;
> +	case 10:
> +		if (dwc->maximum_speed == USB_SPEED_SUPER)
> +			dev_err(dev, "invalid maximum_lsm parameter %d\n",
> +				dwc->maximum_lsm);
> +		/* Fall Through */
> +	default:
> +		if (dwc->maximum_speed == USB_SPEED_SUPER)
> +			dwc->maximum_lsm = 5;
> +		else if (dwc->maximum_speed > USB_SPEED_SUPER)
> +			dwc->maximum_lsm = 10;
> +		break;
> +	}
> +
> +	switch (dwc->maximum_lanes) {
> +	case 1:
> +	case 2:
> +		break;
> +	default:
> +		if (dwc->maximum_lanes > 2)
> +			dev_err(dev, "invalid number of lanes %d\n",
> +				dwc->maximum_lanes);
> +
> +		if (dwc3_is_usb32(dwc) &&
> +		    dwc->maximum_speed == USB_SPEED_SUPER_PLUS)
> +			dwc->maximum_lanes = 2;
> +		else
> +			dwc->maximum_lanes = 1;
> +	}
>  }
>  
>  static int dwc3_probe(struct platform_device *pdev)
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 7fde3c7da543..8e729d4cd5bd 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -376,6 +376,8 @@
>  #define DWC3_GUCTL2_RST_ACTBITLATER		BIT(14)
>  
>  /* Device Configuration Register */
> +#define DWC3_DCFG_NUMLANES(n)	(((n) & 0x3) << 30) /* DWC_usb32 only */
> +
>  #define DWC3_DCFG_DEVADDR(addr)	((addr) << 3)
>  #define DWC3_DCFG_DEVADDR_MASK	DWC3_DCFG_DEVADDR(0x7f)
>  
> @@ -449,6 +451,8 @@
>  #define DWC3_DEVTEN_USBRSTEN		BIT(1)
>  #define DWC3_DEVTEN_DISCONNEVTEN	BIT(0)
>  
> +#define DWC3_DSTS_CONNLANES(n)		(((n) >> 30) & 0x3) /* DWC_usb32 only */
> +
>  /* Device Status Register */
>  #define DWC3_DSTS_DCNRD			BIT(29)
>  
> @@ -946,6 +950,8 @@ struct dwc3_scratchpad_array {
>   * @nr_scratch: number of scratch buffers
>   * @u1u2: only used on revisions <1.83a for workaround
>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
> + * @maximum_lsm: maximum lane speed mantissa in Gbps
> + * @maximum_lanes: maximum lane count
>   * @ip: controller's ID
>   * @revision: controller's version of an IP
>   * @version_type: VERSIONTYPE register contents, a sub release of a revision
> @@ -973,6 +979,7 @@ struct dwc3_scratchpad_array {
>   * @ep0state: state of endpoint zero
>   * @link_state: link state
>   * @speed: device speed (super, high, full, low)
> + * @lane_count: number of connected lanes
>   * @hwparams: copy of hwparams registers
>   * @root: debugfs root folder pointer
>   * @regset: debugfs pointer to regdump file
> @@ -1100,6 +1107,8 @@ struct dwc3 {
>  	u32			nr_scratch;
>  	u32			u1u2;
>  	u32			maximum_speed;
> +	u8			maximum_lsm;
> +	u8			maximum_lanes;
>  
>  	u32			ip;
>  
> @@ -1159,6 +1168,7 @@ struct dwc3 {
>  	u8			u1pel;
>  
>  	u8			speed;
> +	u8			lane_count;
>  
>  	u8			num_eps;
>  
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a6d562e208a9..c31144af3261 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2183,6 +2183,53 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  }
>  
> +static void dwc3_gadget_set_sublink_attr(struct usb_gadget *g,
> +					 unsigned int lane_count,
> +					 unsigned int lsm)
> +{
> +	struct dwc3	*dwc = gadget_to_dwc(g);
> +	unsigned int	lanes;
> +	unsigned long	flags;
> +	u32		reg;
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	if (dwc->maximum_speed <= USB_SPEED_SUPER) {
> +		/* Fall back to maximum speed supported by HW */
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		dwc3_gadget_set_speed(g, dwc->maximum_speed);
> +		spin_lock_irqsave(&dwc->lock, flags);

it looks like we should extract a __dwc3_gadget_set_speed() to avoid the
possible race here:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a9aba716bf80..e317b696029e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2118,14 +2118,11 @@ static void dwc3_gadget_config_params(struct usb_gadget *g,
 				cpu_to_le16(DWC3_DEFAULT_U2_DEV_EXIT_LAT);
 }
 
-static void dwc3_gadget_set_speed(struct usb_gadget *g,
+static void __dwc3_gadget_set_speed(struct dwc3 *dwc,
 				  enum usb_device_speed speed)
 {
-	struct dwc3		*dwc = gadget_to_dwc(g);
-	unsigned long		flags;
 	u32			reg;
 
-	spin_lock_irqsave(&dwc->lock, flags);
 	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
 	reg &= ~(DWC3_DCFG_SPEED_MASK);
 
@@ -2175,7 +2172,16 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
 		}
 	}
 	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
+}
+
+static void dwc3_gadget_set_speed(struct usb_gadget *g,
+				  enum usb_device_speed speed)
+{
+	struct dwc3		*dwc = gadget_to_dwc(g);
+	unsigned long		flags;
 
+	spin_lock_irqsave(&dwc->lock, flags);
+	__dwc3_gadget_set_speed(dwc, speed);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 }
 
Then your patch would look like:

static void dwc3_gadget_set_sublink_attr(struct usb_gadget *g,
					 unsigned int lane_count,
					 unsigned int lsm)
{
	struct dwc3	*dwc = gadget_to_dwc(g);
	unsigned int	lanes;
	unsigned long	flags;
	u32		reg;

	spin_lock_irqsave(&dwc->lock, flags);
	if (dwc->maximum_speed <= USB_SPEED_SUPER) {
		/* Fall back to maximum speed supported by HW */
		__dwc3_gadget_set_speed(dwc, dwc->maximum_speed);
		goto done;
	}

	[...]
}

-- 
balbi

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