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

[Manu Gautam <mgautam@codeaurora.org>]

Hi Jack,


On 9/28/2017 12:46 AM, Jack Pham wrote:
> 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.

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.

>> - 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.


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