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".
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
