DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 5A2F26071B
Date: Wed, 27 Sep 2017 12:16:43 -0700
From: Jack Pham <jackp@codeaurora.org>
To: Manu Gautam <mgautam@codeaurora.org>
Cc: Kishon Vijay Abraham I <kishon@ti.com>,
        Felipe Balbi <balbi@kernel.org>, linux-arm-msm@vger.kernel.org,
        linux-usb@vger.kernel.org, Vivek Gautam <vivek.gautam@codeaurora.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        "open list:GENERIC PHY FRAMEWORK" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 14/17] phy: qcom-qusb2: Set vbus sw-override signal in
 device mode
Message-ID: <20170927191643.GC12812@usblab-sd-06.qualcomm.com>
References: <1506502753-27408-1-git-send-email-mgautam@codeaurora.org>
 <1506502753-27408-15-git-send-email-mgautam@codeaurora.org>
 <20170927175741.GA12812@usblab-sd-06.qualcomm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170927175741.GA12812@usblab-sd-06.qualcomm.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2411
Lines: 55

On Wed, Sep 27, 2017 at 10:57:41AM -0700, Jack Pham wrote:
> Hi Manu,
> 
> On Wed, Sep 27, 2017 at 02:29:10PM +0530, Manu Gautam wrote:
> > VBUS signal coming from PHY must be asserted in device for
> > controller to start operation or assert pull-up. For some
> > platforms where VBUS line is not connected to PHY there is
> > HS_PHY_CTRL register in QSCRATCH wrapper that can be used
> > by software to override VBUS signal going to controller.
> > 
> > Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> > ---
> >  
> > +static int qusb2_phy_set_mode(struct phy *phy, enum phy_mode mode)
> > +{
> > +	struct qusb2_phy *qphy = phy_get_drvdata(phy);
> > +
> > +	qphy->mode = mode;
> > +
> > +	/* Update VBUS override in qscratch register */
> > +	if (qphy->qscratch_base) {
> > +		if (mode == PHY_MODE_USB_DEVICE)
> > +			qusb2_setbits(qphy->qscratch_base, QSCRATCH_HS_PHY_CTRL,
> > +				      UTMI_OTG_VBUS_VALID | SW_SESSVLD_SEL);
> > +		else
> > +			qusb2_clrbits(qphy->qscratch_base, QSCRATCH_HS_PHY_CTRL,
> > +				      UTMI_OTG_VBUS_VALID | SW_SESSVLD_SEL);
> 
> Wouldn't this be better off handled in the controller glue driver? Two
> reasons I think this patch is unattractive:
> 
> - qscratch_base is part of the controller's register space. Your later
>   patch 16/17 ("phy: qcom-qmp: Override lane0_power_present signal in
>   device mode") does a similar thing and hence both drivers have to
>   ioremap() the same register resource while at the same time avoiding
>   request_mem_region() (called by devm_ioremap_resource) to allow it to
>   be mapped in both places.
> 
> - VBUS override bit becomes asserted simply because the mode is changed
>   to device mode but this is irrespective of the actual VBUS state. This
>   could break some test setups which perform a logical disconnect by
>   switching off/on VBUS while leaving data lines connected. Controller
>   would go merrily along thinking it is still attached to the host.
> 
> Instead maybe this could be tied to EXTCON_USB handling in the glue
> driver; though it would need to be an additional notifier on top of
> dwc3/drd.c which already handles extcon for host/device mode.

That is to say, we'd probably need to split out dwc3-qcom from
dwc3-of-simple.c into its own driver (again) in order to add this.

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project