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

[Manu Gautam]

Hi Jack,

On 9/28/2017 10:23 PM, Jack Pham wrote:
> 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@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.
>> 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?

it looks like newer QUSB2 PHY doesn't have a register to read linestate.
QSCRATCH is the only option.
However, setting dp/dm wakeup interrupt polarity based on current linestate
isn't perfect either. It could race with any change in linestate while it was being
read, resulting in missed wakeup interrupt.
Same is the case with QMP PHY when trying to check for RX terminations on
suspend.
IMO PHY driver should get this info from platform glue or controller driver.
E.g. current speed -> SS, HS/FS, LS or NONE (if not in session).

Kishon,
What would you suggest here?
Should we add new calls e.g. phy_get/set_current_speed like::

diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 78bb0d7..41d9ec2 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -29,6 +29,14 @@ enum phy_mode {
ÂÂÂÂÂÂÂ PHY_MODE_USB_OTG,
Â};

+enum phy_speed {
+ÂÂÂÂÂÂ PHY_SPEED_INVALID,
+ÂÂÂÂÂÂ PHY_SPEED_USB_LS,
+ÂÂÂÂÂÂ PHY_SPEED_USB_FS_HS,
+ÂÂÂÂÂÂ PHY_SPEED_USB_SS,
+};
+
Â/**
 * struct phy_ops - set of function pointers for performing phy operations
 * @init: operation to be performed for initializing phy
@@ -45,6 +53,7 @@ struct phy_ops {
ÂÂÂÂÂÂÂ intÂÂÂÂ (*power_on)(struct phy *phy);
ÂÂÂÂÂÂÂ intÂÂÂÂ (*power_off)(struct phy *phy);
ÂÂÂÂÂÂÂ intÂÂÂÂ (*set_mode)(struct phy *phy, enum phy_mode mode);
+ÂÂÂÂÂÂ intÂÂÂÂ (*set_speed)(struct phy *phy, enum phy_speed speed);
ÂÂÂÂÂÂÂ intÂÂÂÂ (*reset)(struct phy *phy);
ÂÂÂÂÂÂÂ struct module *owner;
Â};


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

I was aiming 4.15 for PHY drivers at least.
This will be further simplified if dwc3 glue driver can pass current speed as well
to PHY drivers.

> Jack

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