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

[Manu Gautam <mgautam@codeaurora.org>]

Hi Kishon

On 10/5/2017 2:38 PM, Manu Gautam wrote:
> Hi Jack,
>
> On 9/28/2017 10:23 PM, Jack Pham wrote:
>>
>>>>>> +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;
>  };
>

@Kishon,
Let me know if we can add set_speed to phy_ops. We need this for glue
driver to notify PHY of current connection speed to enable appropriate
wakeup interrupts.


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