RE: [RFC/PATCH v3] usb: dwc3: Introduce OTG driver for dwc3

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Ido Shayevitz
> Sent: Wednesday, July 25, 2012 5:46 AM
> 
> This is first release of otg driver for the dwc3 Synopsys USB3 core.
> The otg driver implements the otg final state machine and control the
> activation of the device controller or host controller.
> 
> In this first implementation, only simple DRD mode is implemented,
> determine if A or B device according to the ID pin as reflected in the
> OSTS.ConIDSts field.
> 
> Signed-off-by: Ido Shayevitz <idos@xxxxxxxxxxxxxx>
> 
> ---
>  drivers/usb/dwc3/Kconfig     |    6 +-
>  drivers/usb/dwc3/Makefile    |    2 +
>  drivers/usb/dwc3/core.c      |   15 +-
>  drivers/usb/dwc3/core.h      |   54 +++++-
>  drivers/usb/dwc3/dwc3_otg.c  |  512 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/dwc3/dwc3_otg.h  |   38 +++
>  drivers/usb/dwc3/gadget.c    |   63 +++++
>  drivers/usb/host/xhci-plat.c |   21 ++
>  drivers/usb/host/xhci.c      |   13 +-
>  9 files changed, 711 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/usb/dwc3/dwc3_otg.c
>  create mode 100644 drivers/usb/dwc3/dwc3_otg.h
> 
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index d13c60f..0cc108d 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -1,9 +1,9 @@
>  config USB_DWC3
>  	tristate "DesignWare USB3 DRD Core Support"
> -	depends on (USB && USB_GADGET)
> +	depends on (USB || USB_GADGET)

Are you sure this is correct? How can a dual-role device work unless both
USB and USB_GADGET are set?

>  	select USB_OTG_UTILS
> -	select USB_GADGET_DUALSPEED
> -	select USB_GADGET_SUPERSPEED
> +	select USB_GADGET_DUALSPEED if USB_GADGET
> +	select USB_GADGET_SUPERSPEED if USB_GADGET
>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>  	help
>  	  Say Y or M here if your system has a Dual Role SuperSpeed
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index d441fe4..ffb3f55 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -1,11 +1,13 @@
>  ccflags-$(CONFIG_USB_DWC3_DEBUG)	:= -DDEBUG
>  ccflags-$(CONFIG_USB_DWC3_VERBOSE)	+= -DVERBOSE_DEBUG
> +ccflags-y += -Idrivers/usb/host
> 
>  obj-$(CONFIG_USB_DWC3)			+= dwc3.o
> 
>  dwc3-y					:= core.o
>  dwc3-y					+= host.o
>  dwc3-y					+= gadget.o ep0.o
> +dwc3-y					+= dwc3_otg.o
> 
>  ifneq ($(CONFIG_DEBUG_FS),)
>  	dwc3-y				+= debugfs.o
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c34452a..5343e39 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -517,15 +517,24 @@ static int __devinit dwc3_probe(struct platform_device *pdev)
>  		break;
>  	case DWC3_MODE_DRD:
>  		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> +		ret = dwc3_otg_init(dwc);
> +		if (ret) {
> +			dev_err(dev, "failed to initialize otg\n");
> +			goto err1;
> +		}
> +

You should follow the existing pattern for error handling in this
function. If there is an error, jump to the error exit, and put your
exit function there, instead of here.

>  		ret = dwc3_host_init(dwc);
>  		if (ret) {
>  			dev_err(dev, "failed to initialize host\n");
> +			dwc3_otg_exit(dwc);

Same here.

>  			goto err1;
>  		}
> 
>  		ret = dwc3_gadget_init(dwc);
>  		if (ret) {
>  			dev_err(dev, "failed to initialize gadget\n");
> +			dwc3_host_exit(dwc);
> +			dwc3_otg_exit(dwc);

Same here.

>  			goto err1;
>  		}
>  		break;

< snip >

> diff --git a/drivers/usb/dwc3/dwc3_otg.c b/drivers/usb/dwc3/dwc3_otg.c
> new file mode 100644
> index 0000000..b3bda11
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3_otg.c
> @@ -0,0 +1,512 @@
> +/**
> + * dwc3_otg.c - DesignWare USB3 DRD Controller OTG
> + *
> + * Copyright (c) 2012, Code Aurora Forum. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */

The other DWC3 files are dual-licensed, with both the GPL and BSD
licenses. You should use the same license here. Same thing for
dwc3_otg.h

< snip >

> +/**
> + * dwc3_otg_reset - reset dwc3 otg registers.
> + *
> + * @w: Pointer to the dwc3 otg workqueue
> + */
> +static void dwc3_otg_reset(struct dwc3_otg *dotg)
> +{
> +	/*
> +	 * OCFG[2] - OTG-Version = 0
> +	 * OCFG[1] - HNPCap = 0
> +	 * OCFG[0] - SRPCap = 0
> +	 */
> +	dwc3_writel(dotg->regs, DWC3_OCFG, 0x0);
> +
> +	/*
> +	 * OCTL[6] - PeriMode = 1
> +	 * OCTL[5] - PrtPwrCtl = 0
> +	 * OCTL[4] - HNPReq = 0
> +	 * OCTL[3] - SesReq = 0
> +	 * OCTL[2] - TermSelDLPulse = 0
> +	 * OCTL[1] - DevSetHNPEn = 0
> +	 * OCTL[0] - HstSetHNPEn = 0
> +	 */
> +	dwc3_writel(dotg->regs, DWC3_OCTL, 0x40);

This seems a little strange. Why not:

	dwc3_writel(dotg->regs, DWC3_OCTL, DWC3_OCTL_PERIMODE);

instead?

> +
> +	/* Clear all otg events (interrupts) indications  */
> +	dwc3_writel(dotg->regs, DWC3_OEVT, 0x1FFFFFF);

This should instead be something like:

	dwc3_writel(dotg->regs, DWC3_OEVT, DWC3_OEVT_CLEAR_ALL);

and then define DWC3_OEVT_CLEAR_ALL in a header file. You should
always avoid 'magic' constants in the C code.

-- 
Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux