Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode

[Bin Liu]

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 <abailon@xxxxxxxxxxxx>
> >>>---
> >>> 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.