Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965255AbcJ2DMB (ORCPT ); Fri, 28 Oct 2016 23:12:01 -0400 Received: from devils.ext.ti.com ([198.47.26.153]:60325 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751223AbcJ2DL7 (ORCPT ); Fri, 28 Oct 2016 23:11:59 -0400 Date: Fri, 28 Oct 2016 22:11:05 -0500 From: Bin Liu To: David Lechner CC: Alexandre Bailon , , , , Subject: Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode Message-ID: <20161029031105.GA27731@uda0271908> Mail-Followup-To: Bin Liu , David Lechner , Alexandre Bailon , khilman@baylibre.com, balbi@kernel.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org References: <1477479503-5131-1-git-send-email-abailon@baylibre.com> <1477479503-5131-4-git-send-email-abailon@baylibre.com> <4727b41c-ab42-7066-8863-a3799920b79e@lechnology.com> <52da6a0c-6259-6d48-f927-07ce037bf584@baylibre.com> <00080607-ce69-b984-a9f3-68ef61193b7f@lechnology.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <00080607-ce69-b984-a9f3-68ef61193b7f@lechnology.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9043 Lines: 191 On Fri, Oct 28, 2016 at 12:11:21PM -0500, David Lechner wrote: > On 10/28/2016 07:39 AM, Alexandre Bailon wrote: > >On 10/28/2016 04:56 AM, David Lechner wrote: > >>On 10/26/2016 05:58 AM, Alexandre Bailon wrote: > >>>When the phy is forced in host mode, only the first hot plug and > >>>hot remove works. That is actually because the driver execute the > >>>OTG workaround, whereas it is not applicable in host or device mode. > >>>Indeed, to work correctly, the VBUS sense and session end comparator > >>>must be enabled, what is only possible when the phy is in OTG mode. > >>>Only execute the workaround if the phy is in OTG mode. > >>> > >>>Signed-off-by: Alexandre Bailon > >>>--- > >>> drivers/usb/musb/da8xx.c | 11 +++++++++++ > >>> 1 file changed, 11 insertions(+) > >>> > >>>diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c > >>>index 6749aa1..b8a6b65 100644 > >>>--- a/drivers/usb/musb/da8xx.c > >>>+++ b/drivers/usb/musb/da8xx.c > >>>@@ -145,6 +145,17 @@ static void otg_timer(unsigned long _musb) > >>> unsigned long flags; > >>> > >>> /* > >>>+ * We should only execute the OTG workaround when the phy is in OTG > >>>+ * mode. The workaround require the VBUS sense and the session end > >>>+ * comparator to be enabled, what is only possible if the phy is in > >>>+ * OTG mode. As the workaround is only required to detect if the > >>>+ * controller must act as host or device, we can safely exit OTG is > >>>+ * not in use. > >>>+ */ > >>>+ if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) > >> > >>musb->port_mode is not valid if we have changed the mode via sysfs. It > >>only reflects the mode set during driver probe. > >> > >>Furthermore, this breaks the host mode completely for me. The first hot > >>plug is not even detected. > >> > >>>+ return; > >>>+ > >>>+ /* > >>> * We poll because DaVinci's won't expose several OTG-critical > >>> * status change events (from the transceiver) otherwise. > >>> */ > >>> > >> > >> > >>The way this is working for me (on AM1808) is this: > >> > >>The problem is not that the OTG workaround is being used. The problem is > >>that after disconnect, the VBUSDRV is turned off. If you look at the > >>handler for DA8XX_INTR_DRVVBUS in da8xx_musb_interrupt(), you will see > >>that if VBUSDRV is off, then drvvbus == 0, which puts the musb state > >>back to device mode. > >> > >>I also ran into a similar problem a while back[1] that if you use a > >>self-powered device in host mode, it immediately becomes disconnected. > >>This is for the exact same reason. When a port detects a self-powered > >>device, it turns of VBUSDRV, which triggers the DA8XX_INTR_DRVVBUS > >>interrupt. As we have seen above, this takes the port out of host mode. > >> > >>The workaround that I have found that seems to fix both cases is to add > >>and else if statement that toggles the PHY host override when we are > >>forcing host mode and the VBUSDRV is turned off. > >I like this workaround. > >> > >>Here is a partial diff of drivers/usb/musb/da8xx.c to show what I mean: > >> > >>@@ -304,10 +309,14 @@ static irqreturn_t da8xx_musb_interrupt(int irq, > >>void *hci) > >> * Also, DRVVBUS pulses for SRP (but not at 5 V)... > >> */ > >> if (status & (DA8XX_INTR_DRVVBUS << DA8XX_INTR_USB_SHIFT)) { > >>+ struct da8xx_glue *glue = > >>+ dev_get_drvdata(musb->controller->parent); > >> int drvvbus = musb_readl(reg_base, DA8XX_USB_STAT_REG); > >> void __iomem *mregs = musb->mregs; > >> u8 devctl = musb_readb(mregs, MUSB_DEVCTL); > >>- int err; > >>+ int cfgchip2, err; > >>+ > >>+ regmap_read(glue->cfgchip, CFGCHIP(2), &cfgchip2); > >> > >> err = musb->int_usb & MUSB_INTR_VBUSERROR; > >> if (err) { > >>@@ -332,10 +341,25 @@ static irqreturn_t da8xx_musb_interrupt(int irq, > >>void *hci) > >> musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE; > >> portstate(musb->port1_status |= > >>USB_PORT_STAT_POWER); > >> del_timer(&otg_workaround); > >>+ } else if ((cfgchip2 & CFGCHIP2_OTGMODE_MASK) > >>+ == CFGCHIP2_OTGMODE_FORCE_HOST) { > >>+ /* > >>+ * If we are forcing host mode, VBUSDRV is > >>turned off > >>+ * after a device is disconnected. We need to > >>toggle the > >>+ * VBUS/ID override to trigger turn it back on, > >>which > >>+ * has the effect of triggering > >>DA8XX_INTR_DRVVBUS again. > >>+ */ > >>+ regmap_write_bits(glue->cfgchip, CFGCHIP(2), > >>+ CFGCHIP2_OTGMODE_MASK, > >>+ CFGCHIP2_OTGMODE_NO_OVERRIDE); > >>+ regmap_write_bits(glue->cfgchip, CFGCHIP(2), > >>+ CFGCHIP2_OTGMODE_MASK, > >>+ CFGCHIP2_OTGMODE_FORCE_HOST); > >> } else { > >> musb->is_active = 0; > >> MUSB_DEV_MODE(musb); > >> > >I haven't thought to this workaround. > >Actually, my goal with this patch was to prevent VBUSDRV to be turned > >off. When I hit the issues, I captured some traces and these traces > >let think VBUSDRV is turned off when the OTG workaround clear > >the bit MUSB_DEVCTL_SESSION. > > > > > After having slept on this, I am realizing that the "correct" thing > to do here is highly hardware dependent. If you look at musb_probe() > in musb_core.c, you will see the true purpose of musb->port_mode. In > host mode, it only sets up a host device, in peripheral mode, it > only sets up a peripheral device and in otg mode, it sets up both. > > So really, this musb->port_mode setting does not say anything about > what the hardware is actually capable of. It is just telling which > devices you want registered in the kernel. (As a side note, this > means that the dr_mode property is really not appropriate for device > tree in your other patch series - even though many existing USB > devices use dr_mode anyway). > > The CFGCHIP2_OTGMODE_* options are to work around hardware > deficiencies. They are only needed when a port is missing the > required external circuitry to function correctly. I think it is > wrong to assume that if someone selects a specific musb->port_mode > then they need to enable one of CFGCHIP2_OTGMODE_FORCE_*. > > If the port has the proper circuitry for OTG, then one should be > able to select any of host, peripheral or otg mode without needing > to set any of CFGCHIP2_OTGMODE_FORCE_*. > > So, I think if we want to be able to use CFGCHIP2_OTGMODE_FORCE_* > for special cases, then we need to add a module parameter (or this > might fit in device tree if we can figure out how to express it as > "describing the hardware"). The parameter will basically say > "override PHY VBUS/ID in host mode if and only if this parameter is > enabled". We could also have a parameter for peripheral mode that > says "override PHY VBUS/ID in peripheral mode if and only if this > parameter is enabled". Module parameters are no longer acceptable, but we can introduce quirk flags to solve this. > > As an example, on LEGO MINDSTORMS EV3, the USB port is wired for > peripheral only. There is nothing connected to the VBUSDRV or ID > pins. Furthermore, the VBUS pin is only connected to the USB jack > and there is not a way to generate VBUS power. So, we can set > musb->port_mode = MUSB_PORT_MODE_GADGET and everything will work as > expected as long as we don't set CFGCHIP2_OTGMODE_FORCE_PERIPHERAL. > Overriding the PHY breaks VBUS sense and we never get back to b_idle > after a device disconnect. (In fact, the only time one would ever > need to set CFGCHIP2_OTGMODE_FORCE_PERIPHERAL is if the VBUS pin is > not connected at all and/or the ID pin is hardwired to ground). > > If we want to be crazy though and be able to switch between host and > peripheral mode anyway, even though the required circuits are > missing, we can set musb->port_mode = MUSB_PORT_MODE_OTG. Then we > can write "host" to the "mode" sysfs attribute to force the port > into host mode. However, in order for this to work, it requires that > CFGCHIP2_OTGMODE_FORCE_HOST is set because of the missing circuitry > for host mode. You have to supply your own external VBUS, but it > does work. > > TL;DR > > I think you fill find that if we remove the da8xx_musb_set_mode() > callback completely, that both host and peripheral mode will work > for you. Overriding the PHY is only needed for unusual cases, like > my example where we are forcing host mode when the required > circuitry is missing. TL;DR, but it all sounds similar to that in the musb_dsps glue, you might find some ideas from there. Regards, -Bin.