Subject: Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround
 when phy in OTG mode
To: Alexandre Bailon <abailon@baylibre.com>, khilman@baylibre.com,
        b-liu@ti.com, balbi@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>
Cc: linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
From: David Lechner <david@lechnology.com>
Message-ID: <00080607-ce69-b984-a9f3-68ef61193b7f@lechnology.com>
Date: Fri, 28 Oct 2016 12:11:21 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <52da6a0c-6259-6d48-f927-07ce037bf584@baylibre.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 8547
Lines: 178

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@baylibre.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.
>


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".

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.