Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934273AbcJZQnO (ORCPT ); Wed, 26 Oct 2016 12:43:14 -0400 Received: from mail-wm0-f46.google.com ([74.125.82.46]:36755 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932914AbcJZQnM (ORCPT ); Wed, 26 Oct 2016 12:43:12 -0400 Subject: Re: [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround To: David Lechner , khilman@baylibre.com, b-liu@ti.com, balbi@kernel.org References: <1477479503-5131-1-git-send-email-abailon@baylibre.com> <1477479503-5131-3-git-send-email-abailon@baylibre.com> <8e83a891-5088-9f14-021f-67e90d2c17ca@lechnology.com> Cc: linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, Kishon Vijay Abraham I From: Alexandre Bailon Message-ID: <1abb9599-3249-a560-ecd7-dd9f60386813@baylibre.com> Date: Wed, 26 Oct 2016 18:43:05 +0200 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: <8e83a891-5088-9f14-021f-67e90d2c17ca@lechnology.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3420 Lines: 90 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 >> --- >> 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. > >