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

[Stephan Gerhold]

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@xxxxxxxxxx>

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
>