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
