Subject: Re: [PATCH v3 2/5] phy: da8xx-usb: Configure CFGCHIP2 to support OTG
 workaround
To: Alexandre Bailon <abailon@baylibre.com>, b-liu@ti.com,
        balbi@kernel.org
References: <1478186765-19840-1-git-send-email-abailon@baylibre.com>
 <1478186765-19840-3-git-send-email-abailon@baylibre.com>
Cc: kishon@ti.com, khilman@baylibre.com, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, nsekhar@ti.com
From: David Lechner <david@lechnology.com>
Message-ID: <669c9a45-a384-263e-458d-29e0bdbba6e4@lechnology.com>
Date: Thu, 3 Nov 2016 12:00:22 -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: <1478186765-19840-3-git-send-email-abailon@baylibre.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2356
Lines: 67

On 11/03/2016 10:26 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@baylibre.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;

I still think this is wrong that you set these bits but never clear 
them. For example, if you start in host mode, these bits will not bit 
set, but if you start in otg mode and switch to host mode, these bits 
will still be set.

And I still think that these bits (CFGCHIP2_SESENDEN | 
CFGCHIP2_VBDTCTEN) should just be enabled during driver probe rather 
than here since I don't know of a reason to turn them off.

>  		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;
>  }
>