Subject: Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed
 negotiation for Hisilicon Hi6220
To: John Youn <John.Youn@synopsys.com>,
        John Stultz <john.stultz@linaro.org>,
        lkml <linux-kernel@vger.kernel.org>
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>
CC: <wangbinghui@hisilicon.com>, Wei Xu <xuwei5@hisilicon.com>,
        Guodong Xu <guodong.xu@linaro.org>,
        Amit Pundir <amit.pundir@linaro.org>, Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Douglas Anderson <dianders@chromium.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
From: Chen Yu <chenyu56@huawei.com>
Message-ID: <3d9a84ee-f17e-e93a-17a2-d2dac3be9e58@huawei.com>
Date: Mon, 17 Oct 2016 10:41:38 +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: <2c58c084-704e-3cac-70a1-c8b31661b3d6@synopsys.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3854
Lines: 121


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