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

2016-10-21 Thread Chen Yu 
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, >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

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

2016-10-21 Thread Chen Yu 
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, >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

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

2016-10-21 Thread John Youn 
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, >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

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

2016-10-21 Thread John Youn 
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, >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

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

2016-10-20 Thread Chen Yu 
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, >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

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

2016-10-20 Thread Chen Yu 
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, >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

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

2016-10-18 Thread John Youn 
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, >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

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

2016-10-18 Thread John Youn 
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, >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

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

2016-10-16 Thread Chen Yu 


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, >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

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

2016-10-16 Thread Chen Yu 


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, >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

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

2016-10-14 Thread John Youn 
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?

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

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

2016-10-14 Thread John Youn 
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?

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

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

2016-10-14 Thread John Stultz 
On Fri, Oct 14, 2016 at 8:00 AM, Rob Herring  wrote:
> On Thu, Oct 13, 2016 at 6:29 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.
>>
>> Cc: Wei Xu 
>> Cc: Guodong Xu 
>> Cc: Amit Pundir 
>> Cc: Rob Herring 
>> Cc: Mark Rutland 
>> Cc: John Youn 
>> Cc: Douglas Anderson 
>> Cc: Greg Kroah-Hartman 
>> Cc: linux-...@vger.kernel.org
>> Signed-off-by: Chen Yu 
>> Signed-off-by: John Stultz 
>> ---
>>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  1 +
>>  drivers/usb/dwc2/core.h   |  7 +++
>>  drivers/usb/dwc2/hcd.c| 75 
>> +++
>>  drivers/usb/dwc2/platform.c   |  3 ++
>>  4 files changed, 86 insertions(+)
>>
[snip]
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index 8e1728b..21c328b 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -585,6 +585,9 @@ static int dwc2_driver_probe(struct platform_device *dev)
>> dev_dbg(>dev, "mapped PA %08lx to VA %p\n",
>> (unsigned long)res->start, hsotg->regs);
>>
>> +   hsotg->change_speed_quirk = device_property_read_bool(>dev,
>> +   "hi6220,change_speed_quirk");
>
> Can't this be determined from the hi6220's compatible string?

Ah. Good suggestion! I'm moving the quirk field to the core_params.
Should avoid any dts or binding changes.

thanks
-john

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

2016-10-14 Thread John Stultz 
On Fri, Oct 14, 2016 at 8:00 AM, Rob Herring  wrote:
> On Thu, Oct 13, 2016 at 6:29 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.
>>
>> Cc: Wei Xu 
>> Cc: Guodong Xu 
>> Cc: Amit Pundir 
>> Cc: Rob Herring 
>> Cc: Mark Rutland 
>> Cc: John Youn 
>> Cc: Douglas Anderson 
>> Cc: Greg Kroah-Hartman 
>> Cc: linux-...@vger.kernel.org
>> Signed-off-by: Chen Yu 
>> Signed-off-by: John Stultz 
>> ---
>>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  1 +
>>  drivers/usb/dwc2/core.h   |  7 +++
>>  drivers/usb/dwc2/hcd.c| 75 
>> +++
>>  drivers/usb/dwc2/platform.c   |  3 ++
>>  4 files changed, 86 insertions(+)
>>
[snip]
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index 8e1728b..21c328b 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -585,6 +585,9 @@ static int dwc2_driver_probe(struct platform_device *dev)
>> dev_dbg(>dev, "mapped PA %08lx to VA %p\n",
>> (unsigned long)res->start, hsotg->regs);
>>
>> +   hsotg->change_speed_quirk = device_property_read_bool(>dev,
>> +   "hi6220,change_speed_quirk");
>
> Can't this be determined from the hi6220's compatible string?

Ah. Good suggestion! I'm moving the quirk field to the core_params.
Should avoid any dts or binding changes.

thanks
-john

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

2016-10-14 Thread Rob Herring 
On Thu, Oct 13, 2016 at 6:29 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.
>
> Cc: Wei Xu 
> Cc: Guodong Xu 
> Cc: Amit Pundir 
> Cc: Rob Herring 
> Cc: Mark Rutland 
> Cc: John Youn 
> Cc: Douglas Anderson 
> Cc: Greg Kroah-Hartman 
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Chen Yu 
> Signed-off-by: John Stultz 
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  1 +
>  drivers/usb/dwc2/core.h   |  7 +++
>  drivers/usb/dwc2/hcd.c| 75 
> +++
>  drivers/usb/dwc2/platform.c   |  3 ++
>  4 files changed, 86 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 17839db..2c8f435 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -752,6 +752,7 @@
> g-np-tx-fifo-size = <128>;
> g-tx-fifo-size = <128 128 128 128 128 128>;
> interrupts = <0 77 0x4>;
> +   hi6220,change_speed_quirk;
> };
>
> mailbox: mailbox@f751 {
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 2a21a04..24fea73 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -823,6 +823,10 @@ struct dwc2_hregs_backup {
>   * @frame_list_sz:  Frame list size
>   * @desc_gen_cache: Kmem cache for generic descriptors
>   * @desc_hsisoc_cache:  Kmem cache for hs isochronous descriptors
> + * @change_speed_quirk: Change speed configuration to DWC2_SPEED_PARAM_FULL
> + *  while full speed device connect. And change speed
> + *  back to DWC2_SPEED_PARAM_HIGH while device is gone.
> + * @device_count:   Number of devices connect to root hub
>   *
>   * These are for peripheral mode:
>   *
> @@ -951,6 +955,9 @@ struct dwc2_hsotg {
> struct kmem_cache *desc_gen_cache;
> struct kmem_cache *desc_hsisoc_cache;
>
> +   int change_speed_quirk;
> +   unsigned int device_count;
> +
>  #ifdef DEBUG
> u32 frrem_samples;
> u64 frrem_accum;
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index b374e60..663033e 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4868,6 +4868,75 @@ static void _dwc2_hcd_clear_tt_buffer_complete(struct 
> usb_hcd *hcd,
> spin_unlock_irqrestore(>lock, flags);
>  }
>
> +/*
> + * 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, >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++;
> +   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

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

2016-10-14 Thread Rob Herring 
On Thu, Oct 13, 2016 at 6:29 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.
>
> Cc: Wei Xu 
> Cc: Guodong Xu 
> Cc: Amit Pundir 
> Cc: Rob Herring 
> Cc: Mark Rutland 
> Cc: John Youn 
> Cc: Douglas Anderson 
> Cc: Greg Kroah-Hartman 
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Chen Yu 
> Signed-off-by: John Stultz 
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  1 +
>  drivers/usb/dwc2/core.h   |  7 +++
>  drivers/usb/dwc2/hcd.c| 75 
> +++
>  drivers/usb/dwc2/platform.c   |  3 ++
>  4 files changed, 86 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 17839db..2c8f435 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -752,6 +752,7 @@
> g-np-tx-fifo-size = <128>;
> g-tx-fifo-size = <128 128 128 128 128 128>;
> interrupts = <0 77 0x4>;
> +   hi6220,change_speed_quirk;
> };
>
> mailbox: mailbox@f751 {
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 2a21a04..24fea73 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -823,6 +823,10 @@ struct dwc2_hregs_backup {
>   * @frame_list_sz:  Frame list size
>   * @desc_gen_cache: Kmem cache for generic descriptors
>   * @desc_hsisoc_cache:  Kmem cache for hs isochronous descriptors
> + * @change_speed_quirk: Change speed configuration to DWC2_SPEED_PARAM_FULL
> + *  while full speed device connect. And change speed
> + *  back to DWC2_SPEED_PARAM_HIGH while device is gone.
> + * @device_count:   Number of devices connect to root hub
>   *
>   * These are for peripheral mode:
>   *
> @@ -951,6 +955,9 @@ struct dwc2_hsotg {
> struct kmem_cache *desc_gen_cache;
> struct kmem_cache *desc_hsisoc_cache;
>
> +   int change_speed_quirk;
> +   unsigned int device_count;
> +
>  #ifdef DEBUG
> u32 frrem_samples;
> u64 frrem_accum;
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index b374e60..663033e 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4868,6 +4868,75 @@ static void _dwc2_hcd_clear_tt_buffer_complete(struct 
> usb_hcd *hcd,
> spin_unlock_irqrestore(>lock, flags);
>  }
>
> +/*
> + * 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, >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++;
> +   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);
> +   }
> +
> +   return 0;
> +}
> +
>

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

2016-10-13 Thread John Stultz 
On Thu, Oct 13, 2016 at 4:29 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.
>
> Cc: Wei Xu 
> Cc: Guodong Xu 
> Cc: Amit Pundir 
> Cc: Rob Herring 
> Cc: Mark Rutland 
> Cc: John Youn 
> Cc: Douglas Anderson 
> Cc: Greg Kroah-Hartman 
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Chen Yu 
> Signed-off-by: John Stultz 
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  1 +
>  drivers/usb/dwc2/core.h   |  7 +++
>  drivers/usb/dwc2/hcd.c| 75 
> +++
>  drivers/usb/dwc2/platform.c   |  3 ++
>  4 files changed, 86 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 17839db..2c8f435 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -752,6 +752,7 @@
> g-np-tx-fifo-size = <128>;
> g-tx-fifo-size = <128 128 128 128 128 128>;
> interrupts = <0 77 0x4>;
> +   hi6220,change_speed_quirk;


I also realize I'm missing the DT binding documentation for this, but
I wanted to get a rough idea if this approach was ok first.
On the next submission (if there aren't major objections to the
approach) I can split the binding documentation and dts change into
different patches.

thanks
-john

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

2016-10-13 Thread John Stultz 
On Thu, Oct 13, 2016 at 4:29 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.
>
> Cc: Wei Xu 
> Cc: Guodong Xu 
> Cc: Amit Pundir 
> Cc: Rob Herring 
> Cc: Mark Rutland 
> Cc: John Youn 
> Cc: Douglas Anderson 
> Cc: Greg Kroah-Hartman 
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Chen Yu 
> Signed-off-by: John Stultz 
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  1 +
>  drivers/usb/dwc2/core.h   |  7 +++
>  drivers/usb/dwc2/hcd.c| 75 
> +++
>  drivers/usb/dwc2/platform.c   |  3 ++
>  4 files changed, 86 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 17839db..2c8f435 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -752,6 +752,7 @@
> g-np-tx-fifo-size = <128>;
> g-tx-fifo-size = <128 128 128 128 128 128>;
> interrupts = <0 77 0x4>;
> +   hi6220,change_speed_quirk;


I also realize I'm missing the DT binding documentation for this, but
I wanted to get a rough idea if this approach was ok first.
On the next submission (if there aren't major objections to the
approach) I can split the binding documentation and dts change into
different patches.

thanks
-john

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

2016-10-13 Thread John Stultz 
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.

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: Mark Rutland 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Greg Kroah-Hartman 
Cc: linux-...@vger.kernel.org
Signed-off-by: Chen Yu 
Signed-off-by: John Stultz 
---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  1 +
 drivers/usb/dwc2/core.h   |  7 +++
 drivers/usb/dwc2/hcd.c| 75 +++
 drivers/usb/dwc2/platform.c   |  3 ++
 4 files changed, 86 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi 
b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 17839db..2c8f435 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -752,6 +752,7 @@
g-np-tx-fifo-size = <128>;
g-tx-fifo-size = <128 128 128 128 128 128>;
interrupts = <0 77 0x4>;
+   hi6220,change_speed_quirk;
};
 
mailbox: mailbox@f751 {
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 2a21a04..24fea73 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -823,6 +823,10 @@ struct dwc2_hregs_backup {
  * @frame_list_sz:  Frame list size
  * @desc_gen_cache: Kmem cache for generic descriptors
  * @desc_hsisoc_cache:  Kmem cache for hs isochronous descriptors
+ * @change_speed_quirk: Change speed configuration to DWC2_SPEED_PARAM_FULL
+ *  while full speed device connect. And change speed
+ *  back to DWC2_SPEED_PARAM_HIGH while device is gone.
+ * @device_count:   Number of devices connect to root hub
  *
  * These are for peripheral mode:
  *
@@ -951,6 +955,9 @@ struct dwc2_hsotg {
struct kmem_cache *desc_gen_cache;
struct kmem_cache *desc_hsisoc_cache;
 
+   int change_speed_quirk;
+   unsigned int device_count;
+
 #ifdef DEBUG
u32 frrem_samples;
u64 frrem_accum;
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index b374e60..663033e 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4868,6 +4868,75 @@ static void _dwc2_hcd_clear_tt_buffer_complete(struct 
usb_hcd *hcd,
spin_unlock_irqrestore(>lock, flags);
 }
 
+/*
+ * 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, >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++;
+   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);
+   }
+
+   return 0;
+}
+
 static struct hc_driver dwc2_hc_driver = {
.description = "dwc2_hsotg",

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

2016-10-13 Thread John Stultz 
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.

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: Mark Rutland 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Greg Kroah-Hartman 
Cc: linux-...@vger.kernel.org
Signed-off-by: Chen Yu 
Signed-off-by: John Stultz 
---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  1 +
 drivers/usb/dwc2/core.h   |  7 +++
 drivers/usb/dwc2/hcd.c| 75 +++
 drivers/usb/dwc2/platform.c   |  3 ++
 4 files changed, 86 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi 
b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 17839db..2c8f435 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -752,6 +752,7 @@
g-np-tx-fifo-size = <128>;
g-tx-fifo-size = <128 128 128 128 128 128>;
interrupts = <0 77 0x4>;
+   hi6220,change_speed_quirk;
};
 
mailbox: mailbox@f751 {
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 2a21a04..24fea73 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -823,6 +823,10 @@ struct dwc2_hregs_backup {
  * @frame_list_sz:  Frame list size
  * @desc_gen_cache: Kmem cache for generic descriptors
  * @desc_hsisoc_cache:  Kmem cache for hs isochronous descriptors
+ * @change_speed_quirk: Change speed configuration to DWC2_SPEED_PARAM_FULL
+ *  while full speed device connect. And change speed
+ *  back to DWC2_SPEED_PARAM_HIGH while device is gone.
+ * @device_count:   Number of devices connect to root hub
  *
  * These are for peripheral mode:
  *
@@ -951,6 +955,9 @@ struct dwc2_hsotg {
struct kmem_cache *desc_gen_cache;
struct kmem_cache *desc_hsisoc_cache;
 
+   int change_speed_quirk;
+   unsigned int device_count;
+
 #ifdef DEBUG
u32 frrem_samples;
u64 frrem_accum;
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index b374e60..663033e 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4868,6 +4868,75 @@ static void _dwc2_hcd_clear_tt_buffer_complete(struct 
usb_hcd *hcd,
spin_unlock_irqrestore(>lock, flags);
 }
 
+/*
+ * 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, >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++;
+   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);
+   }
+
+   return 0;
+}
+
 static struct hc_driver dwc2_hc_driver = {
.description = "dwc2_hsotg",
.product_desc = "DWC OTG Controller",
@@ -5023,6 +5092,12 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
dev_warn(hsotg->dev, "can't set coherent DMA mask\n");
}
 
+   if (hsotg->change_speed_quirk) {