Messages in this thread |  | | Subject | Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220 | From | Chen Yu <> | Date | Mon, 17 Oct 2016 10:41:38 +0800 |
| |
On 2016/10/15 3:37, John Youn wrote: > On 10/13/2016 4:36 PM, John Stultz wrote: >> From: Chen Yu <chenyu56@huawei.com> >> >> 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).
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. 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!
thanks - Chen Yu
|  |