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

[Jack Pham <jackp@codeaurora.org>]

Hi Manu,

On Thu, Sep 28, 2017 at 09:30:38AM +0530, Manu Gautam wrote:
> On 9/28/2017 12:46 AM, Jack Pham wrote:
> > On Wed, Sep 27, 2017 at 10:57:41AM -0700, Jack Pham wrote:
> >> 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.
> 
> Right. There is one more reason why qusb2 driver needs qscratch:
> - During runtime suspend, it has to check linestate to set correct  polarity for dp/dm
>   wakeup interrupt in order to detect disconnect/resume ion LS and FS/HS modes.

Ugh, oh yeah. The way I understand we did it in our downstream driver
is still to have the controller driver read the linestate but then pass
the information via additional set_mode() flags which the PHY driver
could use to correctly arm the interrupt trigger polarity.

An alternative would be to access a couple of the debug QUSB2PHY
registers that also provide a reading of the current UTMI linestate. The
HPG mentions them vaguely, and I can't remember if we tested that
interface or not. Assuming it works, would that be preferable to reading
a non-PHY register here?

> >> - 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.
> 
> Yes, dwc3/drd.c currently deals with only EXTCON_USB_HOST. So, for platforms
> where role swap happens using only Vbus or single GPIO this should take care of.
> 
> 
> > 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
> 
> However, I agree that more appropriate place for lane0-pwr-present and
> vbus override update is dwc3 glue driver. Since we don't have one right now,
> 
> IMO once we have dwc3-qcom driver in place, this handling can be moved from
> PHY to glue driver. Until then we can use this approach to get USB device mode
> working on qcom platforms which are using dwc3-of-simple.c e.g. sdm820
> dragonboard.

Could that be done in this series too? IMO better to get it right in one
shot. Is this aimed for 4.15?

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