[v2,2/2] phy: qcom-usb-hs: Add qcom,dp-manual-pullup logic

Downstream has a flag called qcom,dp-manual-pullup which informs the
downstream driver if it should toggle ULPI_MISC_A_VBUSVLDEXTSEL and
ULPI_MISC_A_VBUSVLDEXT.

Downstream states:

"qcom,dp-manual-pullup: If present, vbus is not routed to USB
        controller/phy and controller driver therefore enables pull-up
        explicitly before starting controller using usbcmd run/stop bit."

Working with a system that has both an external Type-C port controller and
an internal USB Hub results in a situation where VBUS is not connected to
the SoC.

In this case we still need to set the DP pullup.

This patch enables and disables the DP pullup on PHY power_on and power_off
respectively if the DT has declared the bool "qcom,enable-vbus-pullup"
effectively replicating the downstream logic to the same effect.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-usb-hs.c | 36 ++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

On Thu, Dec 29, 2022 at 06:34:10PM +0000, Bryan O'Donoghue wrote:
> Downstream has a flag called qcom,dp-manual-pullup which informs the
> downstream driver if it should toggle ULPI_MISC_A_VBUSVLDEXTSEL and
> ULPI_MISC_A_VBUSVLDEXT.
> 
> Downstream states:
> 
> "qcom,dp-manual-pullup: If present, vbus is not routed to USB
>         controller/phy and controller driver therefore enables pull-up
>         explicitly before starting controller using usbcmd run/stop bit."
> 
> Working with a system that has both an external Type-C port controller and
> an internal USB Hub results in a situation where VBUS is not connected to
> the SoC.
> 
> In this case we still need to set the DP pullup.
> 
> This patch enables and disables the DP pullup on PHY power_on and power_off
> respectively if the DT has declared the bool "qcom,enable-vbus-pullup"
> effectively replicating the downstream logic to the same effect.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

I think ideally you would add an extcon device (or equivalent, e.g.
power supply) somewhere in your Type-C setup so that the VBUS state is
set correctly and does not need to be forced permanently. The chipidea
USB driver will also benefit from this because AFAICT it uses that
information to power down the USB PHY and controller entirely when
in device mode and there is no USB cable connected.

But I agree that setting up a proper extcon device can be difficult,
especially during early bring-up where you just want USB to work and
don't care much about power saving. So I still think that making USB
work without extcon (like your patch does) is a useful change.

For the implementation, I think this flag should effectively emulate the
logic that is currently used if you assign an extcon, except that the
VBUS state is permanently forced active. Right now your
qcom_usb_hs_phy_enable_dp_pullup() function kind of duplicates logic
that already exists in the driver, which already has code to set
VBUSVLDEXTSEL and VBUSVLDEXT.

qcom_usb_hs_phy_set_mode() should be modified to use the else branch (as
if vbus_edev was set), which will set VBUSVLDEXTSEL for you. Without
extcon you currently use the if (!uphy->vbus_edev) branch, which will
enable the internal VBUS detection in the PHY (the ULPI_INT_SESS_VALID
code). This does not make any sense IMO if VBUS is not connected to the
SoC.

And then you basically just need to call qcom_usb_hs_phy_vbus_notifier()
to set the VBUSVLDEXT forcefully (rather than relying on the extcon
detection).

An alternative that I've occasionally used for early bring-up is to
simply use a dummy extcon driver [1] that permanently reports active
VBUS. The end result is the same. While it's clearly a hack perhaps this
makes it a bit more clear that ideally you really should try to assign
an extcon device, to avoid keeping the USB controller and PHY on
permanently.

Thanks,
Stephan

[1]: https://github.com/msm8916-mainline/linux/commit/3d029e6d4303e125aa013c501308d2d98e3cdc89

> ---
>  drivers/phy/qualcomm/phy-qcom-usb-hs.c | 36 ++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs.c b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
> index 53e46c220a3aa..45c94f6722c66 100644
> --- a/drivers/phy/qualcomm/phy-qcom-usb-hs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
> @@ -37,6 +37,7 @@ struct qcom_usb_hs_phy {
>  	struct ulpi_seq *init_seq;
>  	struct extcon_dev *vbus_edev;
>  	struct notifier_block vbus_notify;
> +	u8 enable_dp_pullup:1;
>  };
>  
>  static int qcom_usb_hs_phy_set_mode(struct phy *phy,
> @@ -105,6 +106,23 @@ qcom_usb_hs_phy_vbus_notifier(struct notifier_block *nb, unsigned long event,
>  	return ulpi_write(uphy->ulpi, addr, ULPI_MISC_A_VBUSVLDEXT);
>  }
>  
> +static int qcom_usb_hs_phy_enable_dp_pullup(struct ulpi *ulpi, bool enable)
> +{
> +	u8 addr;
> +	int ret;
> +
> +	if (enable)
> +		addr = ULPI_SET(ULPI_MISC_A);
> +	else
> +		addr = ULPI_CLR(ULPI_MISC_A);
> +
> +	ret = ulpi_write(ulpi, addr, ULPI_MISC_A_VBUSVLDEXTSEL);
> +	if (ret)
> +		return ret;
> +
> +	return ulpi_write(ulpi, addr, ULPI_MISC_A_VBUSVLDEXT);
> +}
> +
>  static int qcom_usb_hs_phy_power_on(struct phy *phy)
>  {
>  	struct qcom_usb_hs_phy *uphy = phy_get_drvdata(phy);
> @@ -154,6 +172,12 @@ static int qcom_usb_hs_phy_power_on(struct phy *phy)
>  			goto err_ulpi;
>  	}
>  
> +	if (uphy->enable_dp_pullup) {
> +		ret = qcom_usb_hs_phy_enable_dp_pullup(ulpi, true);
> +		if (ret)
> +			goto err_ulpi;
> +	}
> +
>  	if (uphy->vbus_edev) {
>  		state = extcon_get_state(uphy->vbus_edev, EXTCON_USB);
>  		/* setup initial state */
> @@ -180,10 +204,19 @@ static int qcom_usb_hs_phy_power_on(struct phy *phy)
>  static int qcom_usb_hs_phy_power_off(struct phy *phy)
>  {
>  	struct qcom_usb_hs_phy *uphy = phy_get_drvdata(phy);
> +	struct ulpi *ulpi = uphy->ulpi;
> +	int ret;
>  
>  	if (uphy->vbus_edev)
>  		extcon_unregister_notifier(uphy->vbus_edev, EXTCON_USB,
>  					   &uphy->vbus_notify);
> +
> +	if (uphy->enable_dp_pullup) {
> +		ret = qcom_usb_hs_phy_enable_dp_pullup(ulpi, false);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	regulator_disable(uphy->v3p3);
>  	regulator_disable(uphy->v1p8);
>  	clk_disable_unprepare(uphy->sleep_clk);
> @@ -229,6 +262,9 @@ static int qcom_usb_hs_phy_probe(struct ulpi *ulpi)
>  	/* NUL terminate */
>  	uphy->init_seq[size / 2].addr = uphy->init_seq[size / 2].val = 0;
>  
> +	if (of_property_read_bool(ulpi->dev.of_node, "qcom,dp-manual-pullup"))
> +		uphy->enable_dp_pullup = 1;
> +
>  	uphy->ref_clk = clk = devm_clk_get(&ulpi->dev, "ref");
>  	if (IS_ERR(clk))
>  		return PTR_ERR(clk);
> -- 
> 2.34.1
>

On 29/12/2022 19:45, Stephan Gerhold wrote:
> For the implementation, I think this flag should effectively emulate the
> logic that is currently used if you assign an extcon

Actually that's fine, its a better solution than pushing the pullup 
constantly as I just suggested in my previous email, there's also zero 
potential impact to existing boards this way, so, that's obviously better.

---
bod

On 29/12/2022 21:02, Stephan Gerhold wrote:
> On Thu, Dec 29, 2022 at 08:45:15PM +0100, Stephan Gerhold wrote:
>> On Thu, Dec 29, 2022 at 06:34:10PM +0000, Bryan O'Donoghue wrote:
>>> Downstream has a flag called qcom,dp-manual-pullup which informs the
>>> downstream driver if it should toggle ULPI_MISC_A_VBUSVLDEXTSEL and
>>> ULPI_MISC_A_VBUSVLDEXT.
>>>
>>> Downstream states:
>>>
>>> "qcom,dp-manual-pullup: If present, vbus is not routed to USB
>>>          controller/phy and controller driver therefore enables pull-up
>>>          explicitly before starting controller using usbcmd run/stop bit."
>>>
>>> Working with a system that has both an external Type-C port controller and
>>> an internal USB Hub results in a situation where VBUS is not connected to
>>> the SoC.
>>>
>>> In this case we still need to set the DP pullup.
>>>
>>> This patch enables and disables the DP pullup on PHY power_on and power_off
>>> respectively if the DT has declared the bool "qcom,enable-vbus-pullup"
>>> effectively replicating the downstream logic to the same effect.
>>>
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> [...]
>>
>> An alternative that I've occasionally used for early bring-up is to
>> simply use a dummy extcon driver [1] that permanently reports active
>> VBUS. The end result is the same. While it's clearly a hack perhaps this
>> makes it a bit more clear that ideally you really should try to assign
>> an extcon device, to avoid keeping the USB controller and PHY on
>> permanently.
>>
> 
> BTW another nice trick that should work in all almost all cases is to
> use the state of the PMIC USB_IN pin as VBUS detection. All battery-
> powered devices I have seen route the USB VBUS to PM8916 USB_IN. And
> even many boards like DB410c seem to do that or at least permanently
> supply 5V there. In all these cases the &pm8916_usbin extcon will
> report a VBUS state that should make USB work.
> 
> Have you tried using that on your MSM8939 board with the Type-C setup?
> 
> &pm8916_usbin {
> 	status = "okay";
> };
> 
> &usb {
> 	status = "okay";
> 	usb-role-switch;
> 	extcon = <&pm8916_usbin>;
> };
> 
> &usb_hs_phy {
> 	extcon = <&pm8916_usbin>;
> };
> 
> Stephan

I checked USBIN before my last email reply.

Its possible its connected but its not there on the 8939 schematic I have.

---
bod

On Thu, Dec 29, 2022 at 09:20:03PM +0000, Bryan O'Donoghue wrote:
> On 29/12/2022 21:11, Stephan Gerhold wrote:
> > Then it doesn't seem to be a particularly complete schematic. 😄
> > PM8916 definitely has USB_IN pads (pad # N13, P13). 😄
> 
> Let me check again.
> 
> No sorry USB_IN_0 and USB_IN_1 are connected to +5V in my schematic, I did
> check USB_IN I just forgot why it wasn't usable... +5v not VBUS is what its
> connected to.
> 

That is still good enough to replace qcom,dp-manual-pullup though.

If you have +5V connected to USB_IN then &pm8916_usbin should report
active VBUS permanently. Set it as extcon for &usb and &usb_hs_phy and
you should have VBUSVLDEXT "pulled-up" like in your patch here, without
any driver changes. :-)

It's a bit of a hack of course, which probably deserves a comment in the
device tree. But since it should not make any functional difference
compared to this patch the approach might be easier than getting this
patch finalized & accepted.

I leave that up to you :)

Stephan

On 29/12/2022 21:41, Stephan Gerhold wrote:
> That is still good enough to replace qcom,dp-manual-pullup though.

But there's no requirement to tie USB_IN_x high if VBUS is not connected 
to it.

---
bod

On Thu, Dec 29, 2022 at 11:38:04PM +0000, Bryan O'Donoghue wrote:
> On 29/12/2022 21:41, Stephan Gerhold wrote:
> > That is still good enough to replace qcom,dp-manual-pullup though.
> 
> But there's no requirement to tie USB_IN_x high if VBUS is not connected to
> it.
> 

I've yet to find any device where USB_IN is not connected in a usable way.

But as I said I leave it up to you - it would be nice to have a proper
solution for devices where VBUS state cannot be determined at all. Those
devices should be rare though so if &pm8916_usbin works in your case
maybe the time would be better invested into other open problems right
now. :)

Thanks,
Stephan