From patchwork Fri Jan 25 09:40:10 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexander Shishkin X-Patchwork-Id: 2042321 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork2.kernel.org (Postfix) with ESMTP id B31D2DF223 for ; Fri, 25 Jan 2013 09:41:41 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TyfkK-00087x-NZ; Fri, 25 Jan 2013 09:38:52 +0000 Received: from mga03.intel.com ([143.182.124.21]) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TyfkI-00087R-Bc for linux-arm-kernel@lists.infradead.org; Fri, 25 Jan 2013 09:38:51 +0000 Received: from azsmga001.ch.intel.com ([10.2.17.19]) by azsmga101.ch.intel.com with ESMTP; 25 Jan 2013 01:38:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.84,537,1355126400"; d="scan'208";a="248289590" Received: from um.fi.intel.com (HELO localhost) ([10.237.72.164]) by azsmga001.ch.intel.com with ESMTP; 25 Jan 2013 01:38:42 -0800 From: Alexander Shishkin To: Peter Chen Subject: Re: [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect In-Reply-To: <20130125062827.GD29795@nchen-desktop> References: <1358733418-17969-1-git-send-email-peter.chen@freescale.com> <1358733418-17969-4-git-send-email-peter.chen@freescale.com> <87y5fimwma.fsf@ashishki-desk.ger.corp.intel.com> <20130125062827.GD29795@nchen-desktop> User-Agent: Notmuch/0.12+187~ga2502b0 (http://notmuchmail.org) Emacs/23.4.1 (x86_64-pc-linux-gnu) Date: Fri, 25 Jan 2013 11:40:10 +0200 Message-ID: <87wqv1bnyd.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130125_043850_795861_FA5D2009 X-CRM114-Status: GOOD ( 30.19 ) X-Spam-Score: -4.6 (----) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-4.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- 3.0 KHOP_BIG_TO_CC Sent to 10+ recipients instaed of Bcc or a list -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [143.182.124.21 listed in list.dnswl.org] -0.7 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: marex@denx.de, m.grzeschik@pengutronix.de, gregkh@linuxfoundation.org, pkondeti@codeaurora.org, linux-usb@vger.kernel.org, balbi@ti.com, mkl@pengutronix.de, kernel@pengutronix.de, matt@genesi-usa.com, maxime.ripard@free-electrons.com, shawn.guo@linaro.org, festevam@gmail.com, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org Peter Chen writes: > On Thu, Jan 24, 2013 at 05:25:17PM +0200, Alexander Shishkin wrote: >> Peter Chen writes: >> >> > The main design flow is the same with msm otg driver, that is the id and >> > vbus interrupt are handled at core driver, others are handled by >> > individual drivers. >> > >> > - At former design, when switch usb role from device->host, it will call >> > udc_stop, it will remove the gadget driver, so when switch role >> > from host->device, it can't add gadget driver any more. >> > At new design, when role switch occurs, the gadget just calls >> > usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as >> > reset controller, it will not free any device/gadget structure >> > >> > - Add vbus connect and disconnect to core interrupt handler, it can >> > notify udc driver by calling usb_gadget_vbus_disconnect >> > /usb_gadget_vbus_connect. >> > >> > Signed-off-by: Peter Chen >> >> [snip] >> >> > @@ -483,6 +614,17 @@ static int ci_hdrc_probe(struct platform_device *pdev) >> > goto rm_wq; >> > } >> > >> > + otgsc = hw_read(ci, OP_OTGSC, ~0); >> > + /* >> > + * if it is device mode: >> > + * - Enable vbus detect >> > + * - If it has already connected to host, notify udc >> > + */ >> > + if (ci->role == CI_ROLE_GADGET) { >> > + ci_enable_otg_interrupt(ci, OTGSC_BSVIE); >> > + ci_handle_vbus_change(ci); >> > + } >> > + >> >> Actually, this doesn't work, neither here, nor in udc_start(), where the >> next patch moves it. If you start in host role with A plug, unplug it, >> plug B and load a gadget module, > > When we unplug A, device's role start will be called, that is > udc_id_switch_for_device in my patch, it will enable vbus interrupt. > Then, when we plug B, there will be an vbus interrupt, then the > ci->vbus_active will be set, then when we load a gadget module, > the enumeration will begin like I said at last email. What doesn't happen is the reset into device mode (unless you have _REGS_SHARED set, which by the way seems a bit misleading) and we're still doing nothing till the vbus interrupt comes. So the REGS_SHARED is the real problem in this case. Now, looking at ci13xxx_{start,stop}(), I'm seeing some more code duplication. What do think about something like this (untested): ---cut--- ---cut--- That's on top of your patchset. Then, I notice there's a mess with locking around _gadget_stop_activity(), but that's out of scope of this patchset, of course. > Ok, I say "fully tested" means I tested cases for my development, which > includes: host/device is plugged during boots up, device/host switch, > host only controller, load gadget before/next cable plugs in. If you > think it is not enough, I will skip "fully" when sends next version patch. Sounds good, but it makes sense to mention that it's fully tested on imx. Regards, --- Alex diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index dcac77c..38446de 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -1351,6 +1351,28 @@ static const struct usb_ep_ops usb_ep_ops = { .fifo_flush = ep_fifo_flush, }; +static int hw_device_connect(struct ci13xxx *ci, int connect) +{ + int ret; + + if (connect) { + pm_runtime_get_sync(&ci->gadget.dev); + ret = hw_device_reset(ci, USBMODE_CM_DC); + hw_device_state(ci, ci->ep0out->qh.dma); + dev_dbg(ci->dev, "Connected to host\n"); + } else { + hw_device_state(ci, 0); + if (ci->platdata->notify_event) + ci->platdata->notify_event(ci, + CI13XXX_CONTROLLER_STOPPED_EVENT); + ret = _gadget_stop_activity(&ci->gadget); + pm_runtime_put_sync(&ci->gadget.dev); + dev_dbg(ci->dev, "Disconnected from host\n"); + } + + return ret; +} + /****************************************************************************** * GADGET block *****************************************************************************/ @@ -1358,7 +1380,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active) { struct ci13xxx *ci = container_of(_gadget, struct ci13xxx, gadget); unsigned long flags; - int gadget_ready = 0; + int gadget_ready = 0, ret = 0; spin_lock_irqsave(&ci->lock, flags); ci->vbus_active = is_active; @@ -1366,24 +1388,10 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active) gadget_ready = 1; spin_unlock_irqrestore(&ci->lock, flags); - if (gadget_ready) { - if (is_active) { - pm_runtime_get_sync(&_gadget->dev); - hw_device_reset(ci, USBMODE_CM_DC); - hw_device_state(ci, ci->ep0out->qh.dma); - dev_dbg(ci->dev, "Connected to host\n"); - } else { - hw_device_state(ci, 0); - if (ci->platdata->notify_event) - ci->platdata->notify_event(ci, - CI13XXX_CONTROLLER_STOPPED_EVENT); - _gadget_stop_activity(&ci->gadget); - pm_runtime_put_sync(&_gadget->dev); - dev_dbg(ci->dev, "Disconnected from host\n"); - } - } + if (gadget_ready) + ret = hw_device_connect(ci, is_active); - return 0; + return ret; } static int ci13xxx_wakeup(struct usb_gadget *_gadget) @@ -1546,20 +1554,9 @@ static int ci13xxx_start(struct usb_gadget *gadget, spin_lock_irqsave(&ci->lock, flags); ci->driver = driver; - pm_runtime_get_sync(&ci->gadget.dev); - if (ci->vbus_active) { - if (ci->platdata->flags & CI13XXX_REGS_SHARED) - hw_device_reset(ci, USBMODE_CM_DC); - } else { - pm_runtime_put_sync(&ci->gadget.dev); - goto done; - } + if (ci->vbus_active) + retval = hw_device_connect(ci, 1); - retval = hw_device_state(ci, ci->ep0out->qh.dma); - if (retval) - pm_runtime_put_sync(&ci->gadget.dev); - - done: spin_unlock_irqrestore(&ci->lock, flags); return retval; } @@ -1572,24 +1569,18 @@ static int ci13xxx_stop(struct usb_gadget *gadget, { struct ci13xxx *ci = container_of(gadget, struct ci13xxx, gadget); unsigned long flags; + int ret = 0; spin_lock_irqsave(&ci->lock, flags); if (ci->vbus_active) { - hw_device_state(ci, 0); - if (ci->platdata->notify_event) - ci->platdata->notify_event(ci, - CI13XXX_CONTROLLER_STOPPED_EVENT); ci->driver = NULL; spin_unlock_irqrestore(&ci->lock, flags); - _gadget_stop_activity(&ci->gadget); - spin_lock_irqsave(&ci->lock, flags); - pm_runtime_put(&ci->gadget.dev); - } - - spin_unlock_irqrestore(&ci->lock, flags); + ret = hw_device_connect(ci, 0); + } else + spin_unlock_irqrestore(&ci->lock, flags); - return 0; + return ret; } /******************************************************************************