Message-ID: <52da6a0c-6259-6d48-f927-07ce037bf584@baylibre.com>
Date:   Fri, 28 Oct 2016 14:39:07 +0200
From:   Alexandre Bailon <abailon@...libre.com>
To:     David Lechner <david@...hnology.com>, khilman@...libre.com,
        b-liu@...com, balbi@...nel.org
Cc:     linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround
 when phy in OTG mode

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@...libre.com>
>> ---
>>  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.

Powered by blists - more mailing lists