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

From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Felipe Balbi <balbi@kernel.org>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <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 22:15:39 +0000	[thread overview]
Message-ID: <0132b864-6b9a-5258-8c0f-b007f4138035@synopsys.com> (raw)
In-Reply-To: <87sglqvu2m.fsf@kernel.org>

Hi,

Felipe Balbi wrote:
> 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."

Ok.

>> @@ -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;
> 	}
>
> 	[...]
> }

Ok. I'll revise this.

Thanks!
Thinh