linux-kernel - Re: [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Oct 2016 18:43:05 +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,
        Kishon Vijay Abraham I <kishon@...com>
Subject: Re: [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG
 workaround

On 10/26/2016 05:40 PM, David Lechner wrote:
> On 10/26/2016 05:58 AM, Alexandre Bailon wrote:
>> If we configure the da8xx OTG phy in OTG mode, neither device or host
>> mode will work. That is because the PHY is not able to detect and notify
>> the driver that value of ID pin changed.
>> To work despite this hardware limitation, the da8xx glue implement a
>> workaround.
>> But to work, the workaround require the VBUS sense and the session end
>> comparator to enabled.
>> Enable them if the phy is configured in OTG mode.
>>
>> Signed-off-by: Alexandre Bailon <abailon@...libre.com>
>> ---
>>  drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
>> index 32ae78c..fd39292 100644
>> --- a/drivers/phy/phy-da8xx-usb.c
>> +++ b/drivers/phy/phy-da8xx-usb.c
>> @@ -93,24 +93,31 @@ static int da8xx_usb20_phy_power_off(struct phy *phy)
>>  static int da8xx_usb20_phy_set_mode(struct phy *phy, enum phy_mode mode)
>>  {
>>      struct da8xx_usb_phy *d_phy = phy_get_drvdata(phy);
>> +    int ret;
>>      u32 val;
>>
>> +    ret = regmap_read(d_phy->regmap, CFGCHIP(2), &val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    val &= ~CFGCHIP2_OTGMODE_MASK;
>> +
>>      switch (mode) {
>>      case PHY_MODE_USB_HOST:        /* Force VBUS valid, ID = 0 */
>> -        val = CFGCHIP2_OTGMODE_FORCE_HOST;
>> +        val |= CFGCHIP2_OTGMODE_FORCE_HOST;
>>          break;
>>      case PHY_MODE_USB_DEVICE:    /* Force VBUS valid, ID = 1 */
>> -        val = CFGCHIP2_OTGMODE_FORCE_DEVICE;
>> +        val |= CFGCHIP2_OTGMODE_FORCE_DEVICE;
>>          break;
>>      case PHY_MODE_USB_OTG:    /* Don't override the VBUS/ID
>> comparators */
>> -        val = CFGCHIP2_OTGMODE_NO_OVERRIDE;
>> +        val |= CFGCHIP2_OTGMODE_NO_OVERRIDE |
>> +            CFGCHIP2_SESENDEN | CFGCHIP2_VBDTCTEN;
> 
> The AM1808 TRM indicates that CFGCHIP2_SESENDEN and CFGCHIP2_VBDTCTEN
> (and CFGCHIP2_DATPOL) should be on for normal operation of the USB 2.0
> PHY. They do not appear to be associated with the OTG mode specifically.
I agree but the function assigned to these bit is highly tied to OTG.
And in TI BSP, there is a comment that let me think that is only needed
for OTG.
/*
 * We have to override VBUS/ID signals when MUSB is configured into the
 * host-only mode -- ID pin will float if no cable is connected, so the
 * controller won't be able to drive VBUS thinking that it's a B-device.
 * Otherwise, we want to use the OTG mode and enable VBUS comparators.
 */
	cfgchip2 &= ~CFGCHIP2_OTGMODE;
#ifdef	CONFIG_USB_MUSB_HOST
	cfgchip2 |=  CFGCHIP2_FORCE_HOST;
#else
	cfgchip2 |=  CFGCHIP2_SESENDEN | CFGCHIP2_VBDTCTEN;
#endif
> 
> It seems to me that it would be more appropriate to set these in
> da8xx_usb20_phy_power_on() instead of here in da8xx_usb20_phy_set_mode().
I tried both the phy forced in device or host mode with theses bit set
or clear and I haven't see any issues, so I think we could set them in
da8xx_usb20_phy_power_on() as you suggested.
> 
> 
>>          break;
>>      default:
>>          return -EINVAL;
>>      }
>>
>> -    regmap_write_bits(d_phy->regmap, CFGCHIP(2), CFGCHIP2_OTGMODE_MASK,
>> -              val);
>> +    regmap_write(d_phy->regmap, CFGCHIP(2), val);
>>
>>      return 0;
>>  }
>>
> 
> Also cc'ing phy maintainer since this is a phy driver.
> 
>