lkml.org 
[lkml]   [2016]   [Oct]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220
    From
    Date


    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

    \
     
     \ /
      Last update: 2016-10-17 04:42    [W:4.295 / U:0.576 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site