Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936195AbcJRWVd (ORCPT ); Tue, 18 Oct 2016 18:21:33 -0400 Received: from smtprelay.synopsys.com ([198.182.47.9]:52222 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932719AbcJRWV0 (ORCPT ); Tue, 18 Oct 2016 18:21:26 -0400 Subject: Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220 To: Chen Yu , 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> From: John Youn CC: "wangbinghui@hisilicon.com" , Wei Xu , Guodong Xu , Amit Pundir , Rob Herring , Mark Rutland , Douglas Anderson , "Greg Kroah-Hartman" , "linux-usb@vger.kernel.org" Message-ID: <3ab39230-0d6b-509e-e26f-b7f7d8549369@synopsys.com> Date: Tue, 18 Oct 2016 15:21:18 -0700 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: <3d9a84ee-f17e-e93a-17a2-d2dac3be9e58@huawei.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.9.139.89] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4413 Lines: 135 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? > 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? Regards, John