Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220

[Chen Yu]

On 2016/10/22 4:00, John Youn wrote:
> On 10/20/2016 5:43 PM, Chen Yu wrote:
>> On 2016/10/19 6:21, John Youn wrote:
>>> On 10/16/2016 7:42 PM, Chen Yu wrote:
>>>>
>>>>
>>>> On 2016/10/15 3:37, John Youn wrote:
>>>>> On 10/13/2016 4:36 PM, John Stultz wrote:
>>>>>> From: Chen Yu <chenyu56@xxxxxxxxxx>
>>>>>>
>>>>>> The Hi6220's usb controller is limited in that it does not
>>>>>> automatically autonegotiate the usb speed. Thus it requires a
>>>>>> quirk so that we can manually negotiate the best usb speed for
>>>>>> the attached device.
>>>>>
>>>>> Hi,
>>>>>
>>>>> Could you expand more on this by explaining what exactly is the
>>>>> limitation and the workaround?
>>>>>
>>>>
>>>> The USB host limitation of Hisilicon Hi6220 is full-speed and low-speed
>>>> devices can not be enumerated when gets plugged behind a hub.
>>>>
>>>>> [snip]
>>>>>
>>>>>> +/*
>>>>>> + * HPRT0_SPD_HIGH_SPEED: high speed
>>>>>> + * HPRT0_SPD_FULL_SPEED: full speed
>>>>>> + */
>>>>>> +static void dwc2_change_bus_speed(struct usb_hcd *hcd, int speed)
>>>>>> +{
>>>>>> +	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>>>>> +
>>>>>> +	if (hsotg->core_params->speed == speed)
>>>>>> +		return;
>>>>>> +
>>>>>> +	hsotg->core_params->speed = speed;
>>>>>> +	queue_work(hsotg->wq_otg, &hsotg->wf_otg);
>>>>>> +}
>>>>>> +
>>>>>> +static int dwc2_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
>>>>>> +{
>>>>>> +	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>>>>> +
>>>>>> +	if (!hsotg->change_speed_quirk)
>>>>>> +		return 1;
>>>>>> +
>>>>>> +	hsotg->device_count++;
>>>>>
>>>>> Why do you need to track the device count?
>>>>>
>>>>>> +	dev_info(hsotg->dev, "Device count is %u after alloc dev\n",
>>>>>> +			hsotg->device_count);
>>>>>> +
>>>>>> +	return 1;
>>>>>> +}
>>>>>> +
>>>>>> +static void dwc2_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
>>>>>> +{
>>>>>> +	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>>>>> +
>>>>>> +	if (!hsotg->change_speed_quirk)
>>>>>> +		return;
>>>>>> +
>>>>>> +	if (hsotg->device_count)
>>>>>> +		hsotg->device_count--;
>>>>>> +
>>>>>> +	dev_info(hsotg->dev, "Device count is %u after free dev\n",
>>>>>> +			hsotg->device_count);
>>>>>> +
>>>>>> +	if (hsotg->device_count == 1 && udev->parent &&
>>>>>> +			udev->parent->speed > USB_SPEED_UNKNOWN &&
>>>>>> +			udev->parent->speed < USB_SPEED_HIGH) {
>>>>>> +		dev_info(hsotg->dev, "Set speed to default high-speed\n");
>>>>>> +		dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
>>>>>> +	}
>>>>>> +}
>>>>>> +
>>>>>> +static int dwc2_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
>>>>>> +{
>>>>>> +	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>>>>> +
>>>>>> +	if (!hsotg->change_speed_quirk)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	if (udev->speed == USB_SPEED_HIGH) {
>>>>>> +		dev_info(hsotg->dev, "Set speed to high-speed\n");
>>>>>> +		dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
>>>>>> +	} else if (udev->speed == USB_SPEED_FULL
>>>>>> +			|| udev->speed == USB_SPEED_LOW) {
>>>>>> +		dev_info(hsotg->dev, "Set speed to full-speed\n");
>>>>>> +		dwc2_change_bus_speed(hcd, HPRT0_SPD_FULL_SPEED);
>>>>>> +	}
>>>>>
>>>>> It seems you are reinitializing the core every time a device is reset
>>>>> and the udev->speed does not match the core_param speed. But how is
>>>>> the udev->speed being set correctly if the hw cannot negotiate the
>>>>> speed in the first place?
>>>>>
>>>>
>>>> The hardware can negotiate the speed, but communication with a full-speed or
>>>> low-speed device behind a hub is the problem.
>>>>
>>>>> Also should it be for every device? What about if a device gets
>>>>> plugged in behind a hub? I don't think you want to execute this code
>>>>> in that case.
>>>>>
>>>>> This should only affect devices plugged into the root hub, correct?
>>>>> And the hsotg controller only has one root hub port. It seems things
>>>>> could be simplified a bit.
>>>>>
>>>>
>>>> The patch is initially written for Hikey Hi6220 board, and there is a
>>>> hub always connected to root hub, so the patch sets the configuration to
>>>> HPRT0_SPD_HIGH_SPEED when there is only one device(the hub).
>>>
>>> Ok, I see.
>>>
>>>>
>>>> Thanks for your suggestions, the patch needs modified in these aspect:
>>>> 1. Change the speed setting only when the device is behind a hub in dwc2_reset_device.
>>>
>>> I still think you will have issues with multiple devices. Since you
>>> have a built-in hub after root hub, it will always be behind the
>>> hub. So whenver you need to change speeds, it will always reset every
>>> device in the tree. Have you tested with multiple devices and also
>>> multiple levels of hubs?
>>>
>>
>> Yes, a full-speed device or low-speed device being plugged in will reset other devices
>> if current speed setting is not full-speed.
>> I have tested with multiple devices and also two levels of high-speed hubs,
>> the patch was working well for enumerating full-speed device and low-speed device.
>> Also it changed the speed to high when there was only one hub connected to root hub.
>>
>>
>>>> 2. Change the speed to HPRT0_SPD_HIGH_SPEED only when the last device is a hub.
>>>>
>>>> What do you think about the fix? Any suggestions will be appreciate!
>>>
>>> I'm not sure if any fix can work for all cases. Has this problem
>>> always been there?
>>
>> Yes, the problem is always there because Hi6220 can not support "Split Transaction".
> 
> So the problem is actually that the on board hub doesn't support
> splits? That's much different than your initial description and the
> workaround makes more sense in this context.
> 
> But why can't it support splits?
> 
> And how can an FS device pass enumeration if the hub doesn't support
> splits? I feel like there is still something missing.
> 
> Also, if you plug in a HS hub, and then an FS device behind that. That
> should work fine, no? So you wouldn't want to reset anything in that
> case.
> 

Hi6220 is a SOC of Hisilicon, not the board. The hub on board is a HS hub and support splits.
The limitation is from the Hi6220 chip.