Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220

[John Youn]

On 10/13/2016 4:36 PM, John Stultz wrote:
> From: Chen Yu <chenyu56@xxxxxxxxxx>
> 
> 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?

[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?

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.

Regards,
John