Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755594AbcJVBAE (ORCPT ); Fri, 21 Oct 2016 21:00:04 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:38696 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754888AbcJVBAC (ORCPT ); Fri, 21 Oct 2016 21:00:02 -0400 Subject: Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220 To: John Youn , John Stultz , lkml References: <1476401397-26497-1-git-send-email-john.stultz@linaro.org> <1476401397-26497-3-git-send-email-john.stultz@linaro.org> <2c58c084-704e-3cac-70a1-c8b31661b3d6@synopsys.com> <3d9a84ee-f17e-e93a-17a2-d2dac3be9e58@huawei.com> <3ab39230-0d6b-509e-e26f-b7f7d8549369@synopsys.com> <07e16fd3-7e27-3346-4e25-b2baa6c15c46@huawei.com> <2bb28321-2d3f-8f9d-92ec-4fccc18c02d5@synopsys.com> CC: , Wei Xu , Guodong Xu , Amit Pundir , Rob Herring , Mark Rutland , Douglas Anderson , Greg Kroah-Hartman , "linux-usb@vger.kernel.org" From: Chen Yu Message-ID: <87617a6d-eb5a-5e9e-4369-7e07de2543ed@huawei.com> Date: Sat, 22 Oct 2016 08:56:23 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <2bb28321-2d3f-8f9d-92ec-4fccc18c02d5@synopsys.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.142.63.192] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6049 Lines: 162 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 >>>>>> >>>>>> 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.