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

From: Rob Herring
Date: Fri Oct 14 2016 - 11:00:41 EST

On Thu, Oct 13, 2016 at 6:29 PM, John Stultz <john.stultz@xxxxxxxxxx> 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.
>
> Cc: Wei Xu <xuwei5@xxxxxxxxxxxxx>
> Cc: Guodong Xu <guodong.xu@xxxxxxxxxx>
> Cc: Amit Pundir <amit.pundir@xxxxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: John Youn <johnyoun@xxxxxxxxxxxx>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-usb@xxxxxxxxxxxxxxx
> Signed-off-by: Chen Yu <chenyu56@xxxxxxxxxx>
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
> 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@f7510000 {
> 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&low 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(&hsotg->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, &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++;
> + 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) {
> + dwc2_hc_driver.alloc_dev = dwc2_alloc_dev;
> + dwc2_hc_driver.free_dev = dwc2_free_dev;
> + dwc2_hc_driver.reset_device = dwc2_reset_device;
> + }
> +
> hcd = usb_create_hcd(&dwc2_hc_driver, hsotg->dev, dev_name(hsotg->dev));
> if (!hcd)
> goto error1;
> 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->dev, "mapped PA %08lx to VA %p\n",
> (unsigned long)res->start, hsotg->regs);
>
> + hsotg->change_speed_quirk = device_property_read_bool(&dev->dev,
> + "hi6220,change_speed_quirk");

Can't this be determined from the hi6220's compatible string?

In any case, "hi6220" is not a vendor. This should be
hisilicon,change... and don't use '_'. And to bikeshed things,
"no-speed-change-quirk".

Rob