[PATCH v2 0/2] qcom: Add a method to manually toggle the DP pullup on HS USB PHY

[PATCH v2 0/2] qcom: Add a method to manually toggle the DP pullup on HS USB PHY

[Bryan O'Donoghue]

V2:
I'm clearing out old patches in my tree and opted to rework the naming of
the relevant flag to align with the downstream name. Also the original set
was sent against a .txt description which has now moved onto .yaml.

I haven't retained Rob's Acked-by since its +18 months and the
Documentation patch applies to .yaml now not .txt

https://lore.kernel.org/all/20200528223458.GA804610@bogus/T/#mb20456db4a3d1cf608bb6335a1b6fa9dda8cb0cb
https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/usb/phy/phy-msm-usb.c#2736

V1:
On an MSM8939 we have a system behind both a type-c controller and a USB
Hub. VBUS is not connected to the PHY and no GPIO is available to signal
VBUS state to the USB controller and PHY.

In this case we've used USB role-switching to transition between
host and device mode.

The current code in qcom-usb-hs only touches the VBUS pullup control bits
if we have an extcon but, setting those bits is still required on the
example I gave of the MSM8939.

This series takes the downstream concept of a DT driven flag for the VBUS
pullup bits and applies it to upstream in the poweron/poweroff path of the
PHY.

I've opted to unset the bits on PHY poweroff though in downstream the bits
are only ever switched on if the flag is present, downstream never switches
the bits off again. I think though, setting the bits off on PHY power-off
is the right thing to do, so I've done it.

Bryan O'Donoghue (2):
  dt-bindings: phy: Add qcom,dp-manual-pullup description
  phy: qcom-usb-hs: Add qcom,dp-manual-pullup logic

 .../bindings/phy/qcom,usb-hs-phy.yaml         |  7 ++++
 drivers/phy/qualcomm/phy-qcom-usb-hs.c        | 36 +++++++++++++++++++
 2 files changed, 43 insertions(+)

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: phy: Add qcom,dp-manual-pullup description

[Bryan O'Donoghue]

The original Qualcomm driver for the HighSpeed USB PHY contains a flag
which tells the driver that the controller and PHY do not connect to VBUS.

In this case an external IC such as a Type-C port manager supplies VBUS and
the VBUS signal is not routed to the SoC. This means we cannot detect the
presence or absence of VBUS and cannot take action based on it.

Document the downstream boolean qcom,dp-manual-pullup to allow the HS PHY
implement the necessary logic.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
index aa97478dd0161..c55a59df71ad0 100644
--- a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
@@ -80,6 +80,13 @@ properties:
             the address is offset from the ULPI_EXT_VENDOR_SPECIFIC address
         - description: value
 
+  qcom,dp-manual-pullup:
+    type: boolean
+    description: This flag indicates to the HS USB PHY driver that it should
+                 enable or disable an internal pullup when powering on or
+                 powering off the HS PHY instead of toggling the value when VBUS
+                 is absent or present.
+
 required:
   - clocks
   - clock-names
-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: phy: Add qcom,dp-manual-pullup description

[Stephan Gerhold]

On Thu, Dec 29, 2022 at 06:34:09PM +0000, Bryan O'Donoghue wrote:
> The original Qualcomm driver for the HighSpeed USB PHY contains a flag
> which tells the driver that the controller and PHY do not connect to VBUS.
> 
> In this case an external IC such as a Type-C port manager supplies VBUS and
> the VBUS signal is not routed to the SoC. This means we cannot detect the
> presence or absence of VBUS and cannot take action based on it.
> 
> Document the downstream boolean qcom,dp-manual-pullup to allow the HS PHY
> implement the necessary logic.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

AFAIK it is not possible to route VBUS directly to the controller on
these SoCs so this property would likely be added to the SoC dtsi
(i.e. msm8916.dtsi and msm8939.dtsi) and used by all boards.

This means we could just bind this behavior to the existing SoC-specific
compatible (i.e. of_device_is_compatible(..., "qcom,usb-hs-phy-msm8916"))
and avoid having an extra property.

Thoughts?

Thanks,
Stephan

> ---
>  Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
> index aa97478dd0161..c55a59df71ad0 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
> @@ -80,6 +80,13 @@ properties:
>              the address is offset from the ULPI_EXT_VENDOR_SPECIFIC address
>          - description: value
>  
> +  qcom,dp-manual-pullup:
> +    type: boolean
> +    description: This flag indicates to the HS USB PHY driver that it should
> +                 enable or disable an internal pullup when powering on or
> +                 powering off the HS PHY instead of toggling the value when VBUS
> +                 is absent or present.
> +
>  required:
>    - clocks
>    - clock-names
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/2] dt-bindings: phy: Add qcom,dp-manual-pullup description

[Bryan O'Donoghue]

On 29/12/2022 18:57, Stephan Gerhold wrote:
> AFAIK it is not possible to route VBUS directly to the controller on
> these SoCs so this property would likely be added to the SoC dtsi
> (i.e. msm8916.dtsi and msm8939.dtsi) and used by all boards.

So db410c signals the SoC via GPIO 121 / USB_HS_ID

https://fccid.io/2AFQA-DB410C/Schematics/Schematics-2816094.pdf

Which causes ULPI_MISC_A_VBUSVLDEXT to be updated depending on the state 
VBUS.

But not ULPI_MISC_A_VBUSVLDEXTSEL this is the additional register that 
downstream updates when "VBUS is not routed to the controller"

I don't have a bit-level description of these registers at the moment 
so, I'm guessing that ULPI_MISC_A_VBUSVLDEXTSEL *is* being updated.

The reason for that is if I just set ULPI_MISC_A_VBUSVLDEXT then as a 
device a host never sees my SoC via the internal USB hub.

In other words, for me at any rate I need to see both

- ULPI_MISC_A_VBUSVLDEXT
- ULPI_MISC_A_VBUSVLDEXTSEL

to get the pullup to work and hence the Hub/Host to detect the 8939.

> This means we could just bind this behavior to the existing SoC-specific
> compatible (i.e. of_device_is_compatible(..., "qcom,usb-hs-phy-msm8916"))
> and avoid having an extra property.
> 
> Thoughts?

So. I'm OOO at the moment and didn't bring my db410c but TBH to me I 
don't see why we do this whole dance with the pullup on/off with VBUS.

The right thing to do is to run an experiment statically setting

- ULPI_MISC_A_VBUSVLDEXT
- ULPI_MISC_A_VBUSVLDEXTSEL

On/off at power on/off respectively on

- db410c
- My reference where I already know it works

I'm not really seeing the utility of - partially waggling one of two 
registers with VBUS.

Why not just push the pullup on with power-on and off with power-off..

Its worth an experiement if you have the time, if not I'll check it when 
I get back home.

---
bod

Re: [PATCH v2 1/2] dt-bindings: phy: Add qcom,dp-manual-pullup description

[Stephan Gerhold]

On Thu, Dec 29, 2022 at 07:48:46PM +0000, Bryan O'Donoghue wrote:
> On 29/12/2022 18:57, Stephan Gerhold wrote:
> > AFAIK it is not possible to route VBUS directly to the controller on
> > these SoCs so this property would likely be added to the SoC dtsi
> > (i.e. msm8916.dtsi and msm8939.dtsi) and used by all boards.
> 
> So db410c signals the SoC via GPIO 121 / USB_HS_ID
> 
> https://fccid.io/2AFQA-DB410C/Schematics/Schematics-2816094.pdf
> 
> Which causes ULPI_MISC_A_VBUSVLDEXT to be updated depending on the state
> VBUS.
> 

Correct. If I'm reading the DB410c schematic correctly the USB_HS_ID on
DB410c is actually derived from VBUS on the micro USB port (and not the
ID pin of the port as one would normally expect).

> But not ULPI_MISC_A_VBUSVLDEXTSEL this is the additional register that
> downstream updates when "VBUS is not routed to the controller"

AFAICT ULPI_MISC_A_VBUSVLDEXTSEL is set in qcom_usb_hs_phy_set_mode() if
the USB controller enables device mode? (And disabled again in host mode.)

> 
> I don't have a bit-level description of these registers at the moment so,
> I'm guessing that ULPI_MISC_A_VBUSVLDEXTSEL *is* being updated.
> 
> The reason for that is if I just set ULPI_MISC_A_VBUSVLDEXT then as a device
> a host never sees my SoC via the internal USB hub.
> 
> In other words, for me at any rate I need to see both
> 
> - ULPI_MISC_A_VBUSVLDEXT
> - ULPI_MISC_A_VBUSVLDEXTSEL
> 
> to get the pullup to work and hence the Hub/Host to detect the 8939.
> 

The bit-level description of this register in the public APQ8016E TRM
[1] is not very useful (page 1957):

VBUSVLDEXTSEL = "External VBUS valid select"
VBUSVLDEXT = "External VBUS valid indicator"

But I think VBUSVLDEXTSEL basically means "Use external VBUS state from
VBUSVLDEXT instead of internal detection". And VBUSVLDEXT then contains
the actual VBUS state.

The VBUS state is then probably used somewhere inside the USB controller
or PHY to enable necessary USB protocol things such as enabling this "DP
pull-up" (to be fair I have no idea how the USB protocol really works :)).

[1]: https://developer.qualcomm.com/download/sd410/snapdragon-410e-technical-reference-manual.pdf

> > This means we could just bind this behavior to the existing SoC-specific
> > compatible (i.e. of_device_is_compatible(..., "qcom,usb-hs-phy-msm8916"))
> > and avoid having an extra property.
> > 
> > Thoughts?
> 
> So. I'm OOO at the moment and didn't bring my db410c but TBH to me I don't
> see why we do this whole dance with the pullup on/off with VBUS.
> 
> The right thing to do is to run an experiment statically setting
> 
> - ULPI_MISC_A_VBUSVLDEXT
> - ULPI_MISC_A_VBUSVLDEXTSEL
> 
> On/off at power on/off respectively on
> 
> - db410c
> - My reference where I already know it works
> 
> I'm not really seeing the utility of - partially waggling one of two
> registers with VBUS.
> 
> Why not just push the pullup on with power-on and off with power-off..
> 

I don't feel qualified to comment on this. I'd just follow the
suggestion from the docs here to make VBUSVLDEXT effectively represent
the result of the external VBUS detection (see APQ8016E TRM [1] section
6.4.5.3.3 Software control for SESS_VALID, page 1927).

DB410c and other devices with USB hub are obviously a bit special, but
I think it works correctly for DB410c at the moment because its USB_HS_ID
GPIO basically indicates the incoming VBUS on the micro USB port.

There might be some funky design where it is completely impossible to
detect the incoming VBUS. In that case we have no choice but to force
the detected VBUS state on permanently.

Thanks,
Stephan

[1]: https://developer.qualcomm.com/download/sd410/snapdragon-410e-technical-reference-manual.pdf

Re: [PATCH v2 1/2] dt-bindings: phy: Add qcom,dp-manual-pullup description

[Krzysztof Kozlowski]

On 29/12/2022 19:34, Bryan O'Donoghue wrote:
> The original Qualcomm driver for the HighSpeed USB PHY contains a flag
> which tells the driver that the controller and PHY do not connect to VBUS.
> 
> In this case an external IC such as a Type-C port manager supplies VBUS and
> the VBUS signal is not routed to the SoC. This means we cannot detect the
> presence or absence of VBUS and cannot take action based on it.
> 
> Document the downstream boolean qcom,dp-manual-pullup to allow the HS PHY
> implement the necessary logic.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
> index aa97478dd0161..c55a59df71ad0 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
> @@ -80,6 +80,13 @@ properties:
>              the address is offset from the ULPI_EXT_VENDOR_SPECIFIC address
>          - description: value
>  
> +  qcom,dp-manual-pullup:
> +    type: boolean
> +    description: This flag indicates to the HS USB PHY driver that it should

Drop references to the driver and rephrase it to describe the controller
behavior/feature or the board layout/configuration, not the driver. The
same applies to property name - "manual pullup" is describing driver,
not hardware.

> +                 enable or disable an internal pullup when powering on or
> +                 powering off the HS PHY instead of toggling the value when VBUS
> +                 is absent or present.

Wrong indentation. It's two spaces.

> +
>  required:
>    - clocks
>    - clock-names

Best regards,
Krzysztof

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

[Bryan O'Donoghue]

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(+)

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

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

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

[Bryan O'Donoghue]

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

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

[Stephan Gerhold]

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

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

[Bryan O'Donoghue]

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

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

[Stephan Gerhold]

On Thu, Dec 29, 2022 at 09:05:23PM +0000, Bryan O'Donoghue wrote:
> 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.
> 

Then it doesn't seem to be a particularly complete schematic. :-)
PM8916 definitely has USB_IN pads (pad # N13, P13). :-)

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

[Bryan O'Donoghue]

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.

VBUS connects to VBUS_DET on the USB Hub - a USB2514BQFN36, there's no 
corresponding routing of VBUS onto a GPIO or USB_IN_x on the PMIC.

I assume there's some good reason the PCB designer did it this way .. 
there's enough of a reason behind it that Qcom put a workaround for the 
case where PCBs don't route the VBUS state to the SoC or PMIC.

---
bod

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

[Stephan Gerhold]

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

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

[Bryan O'Donoghue]

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

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

[Stephan Gerhold]

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