Re: [PATCH v2 14/17] phy: qcom-qusb2: Set vbus sw-override signal in device mode

[Jack Pham]

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@xxxxxxxxxxxxxx>
> > ---
> >  
> > +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