Hi Paul, On Thu, July 26, 2012 4:38 pm, Paul Zimmerman wrote: >> 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? If the USB_DWC3 is not selected then even dwc3/core.c is not being built and core.c supports also non DRD cores (depends on DWC3_MODE), so we want to build the DWC3 if there is at least host support or a gadget support. In case that the DWC_MODE is DRD but USB is not on, then dwc3/host.c will add the xHCI device, but will be no xHCI driver (xhci-plat) that will probe on this device. >> 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. I think I followed the pattern for error handling in this function. Please see that it is the same as "case DWC3_MODE_HOST" or "case DWC3_MODE_DEVICE" >> ret = dwc3_host_init(dwc); >> if (ret) { >> dev_err(dev, "failed to initialize host\n"); >> + dwc3_otg_exit(dwc); > > Same here. Here I was thinking of moving "dwc3_otg_exit()" to the error handling section, but I think it will make the error handling section too clumsy since eventhough the mode is DWC3_MODE_DRD, in this point I want to exit only the otg and not the host or gadget. So this should have a new error label that will be located between err1 and err2. On the other hand if the error will happen later, as the "goto err2" statement below in this function, then if the mode is DRD I will want to activate the exit function of also the host and gadget but not doing the otg exit again (the new label) and then do err1... So it becomes clumsy. Anyway let me know if you think of a simple way to organize the error handling section. >> 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. > 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 I see, I will check again before uploading a new patch version. Thanks ! > < 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? Good idea, will fix that. >> + >> + /* 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. Yes, good idea. I will also define this as follow: #define DWC3_OEVT_CLEAR_ALL (~DWC3_OEVT_DEVICEMODE) Because I see that Synopsys keep adding events to this register, for example in 2.20a there are 28 bits of events now... > -- > 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 > Thanks for the review, Ido -- Consultant for Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum -- 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
