[PATCH v1 0/3] USB TCPM host (SRC) mode improvements

[PATCH v1 0/3] USB TCPM host (SRC) mode improvements

[bigunclemax]

From: Maksim Kiselev <bigunclemax@gmail.com>

Hello,

First of all I want to thank Sebastian for the work he has done
to add TPCM support.

This series contains some improvements for issues I encountered when
Type-C port acts as a host (source).

The first issue is that the Type-C state machine gets stuck
in TOGGLING state.

The second is that in host mode fusb302 should control VBUS supply via
regulator, but this is not implemented yet.

Therefore, I'd like to discuss several patches that solve issues above.

P.S. Maybe this series may be combined with Sebastian's series
- "USB-PD TCPM improvements"

https://lists.denx.de/pipermail/u-boot/2024-October/570163.html

Best wishes,
Maksim

Maksim Kiselev (3):
  usb: tcpm: fusb302: add missing newline character to debug output
  usb: tcpm: fusb302: add support for set_vbus() callback.
  usb: tcpm: fix toggling in host (SRC) mode

 drivers/usb/tcpm/fusb302.c | 50 +++++++++++++++++++++++++++++++++++---
 drivers/usb/tcpm/tcpm.c    |  8 +++++-
 2 files changed, 53 insertions(+), 5 deletions(-)

-- 
2.45.2

[PATCH v1 1/3] usb: tcpm: fusb302: add missing newline character to debug output

[bigunclemax]

From: Maksim Kiselev <bigunclemax@gmail.com>

Add newline character in dev_dbg end.

Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
---
 drivers/usb/tcpm/fusb302.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/tcpm/fusb302.c b/drivers/usb/tcpm/fusb302.c
index fe93ff3d339..ee283782792 100644
--- a/drivers/usb/tcpm/fusb302.c
+++ b/drivers/usb/tcpm/fusb302.c
@@ -940,7 +940,7 @@ static int fusb302_get_src_cc_status(struct udevice *dev,
 		return ret;
 
 	fusb302_i2c_read(dev, FUSB_REG_SWITCHES0, &status0);
-	dev_dbg(dev, "get_src_cc_status switches: 0x%0x", status0);
+	dev_dbg(dev, "get_src_cc_status switches: 0x%0x\n", status0);
 
 	/* Step 2: Set compararator volt to differentiate between Open and Rd */
 	ret = fusb302_i2c_write(dev, FUSB_REG_MEASURE, rd_mda);
@@ -952,7 +952,7 @@ static int fusb302_get_src_cc_status(struct udevice *dev,
 	if (ret)
 		return ret;
 
-	dev_dbg(dev, "get_src_cc_status rd_mda status0: 0x%0x", status0);
+	dev_dbg(dev, "get_src_cc_status rd_mda status0: 0x%0x\n", status0);
 	if (status0 & FUSB_REG_STATUS0_COMP) {
 		*cc = TYPEC_CC_OPEN;
 		return 0;
@@ -968,7 +968,7 @@ static int fusb302_get_src_cc_status(struct udevice *dev,
 	if (ret)
 		return ret;
 
-	dev_dbg(dev, "get_src_cc_status ra_mda status0: 0x%0x", status0);
+	dev_dbg(dev, "get_src_cc_status ra_mda status0: 0x%0x\n", status0);
 	if (status0 & FUSB_REG_STATUS0_COMP)
 		*cc = TYPEC_CC_RD;
 	else
-- 
2.45.2

Re: [PATCH v1 1/3] usb: tcpm: fusb302: add missing newline character to debug output

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

Hi,

On Fri, Nov 01, 2024 at 01:34:50PM +0300, bigunclemax@gmail.com wrote:
> From: Maksim Kiselev <bigunclemax@gmail.com>
> 
> Add newline character in dev_dbg end.
> 
> Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
> ---

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/usb/tcpm/fusb302.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/tcpm/fusb302.c b/drivers/usb/tcpm/fusb302.c
> index fe93ff3d339..ee283782792 100644
> --- a/drivers/usb/tcpm/fusb302.c
> +++ b/drivers/usb/tcpm/fusb302.c
> @@ -940,7 +940,7 @@ static int fusb302_get_src_cc_status(struct udevice *dev,
>  		return ret;
>  
>  	fusb302_i2c_read(dev, FUSB_REG_SWITCHES0, &status0);
> -	dev_dbg(dev, "get_src_cc_status switches: 0x%0x", status0);
> +	dev_dbg(dev, "get_src_cc_status switches: 0x%0x\n", status0);
>  
>  	/* Step 2: Set compararator volt to differentiate between Open and Rd */
>  	ret = fusb302_i2c_write(dev, FUSB_REG_MEASURE, rd_mda);
> @@ -952,7 +952,7 @@ static int fusb302_get_src_cc_status(struct udevice *dev,
>  	if (ret)
>  		return ret;
>  
> -	dev_dbg(dev, "get_src_cc_status rd_mda status0: 0x%0x", status0);
> +	dev_dbg(dev, "get_src_cc_status rd_mda status0: 0x%0x\n", status0);
>  	if (status0 & FUSB_REG_STATUS0_COMP) {
>  		*cc = TYPEC_CC_OPEN;
>  		return 0;
> @@ -968,7 +968,7 @@ static int fusb302_get_src_cc_status(struct udevice *dev,
>  	if (ret)
>  		return ret;
>  
> -	dev_dbg(dev, "get_src_cc_status ra_mda status0: 0x%0x", status0);
> +	dev_dbg(dev, "get_src_cc_status ra_mda status0: 0x%0x\n", status0);
>  	if (status0 & FUSB_REG_STATUS0_COMP)
>  		*cc = TYPEC_CC_RD;
>  	else
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 2/3] usb: tcpm: fusb302: add support for set_vbus() callback.

[bigunclemax]

From: Maksim Kiselev <bigunclemax@gmail.com>

Add support for VBUS supply regulator.

When our type-c port acts as a host(SRC), this regulator
used for control VBUS supply.

Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
---
 drivers/usb/tcpm/fusb302.c | 44 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/tcpm/fusb302.c b/drivers/usb/tcpm/fusb302.c
index ee283782792..2a258d6429b 100644
--- a/drivers/usb/tcpm/fusb302.c
+++ b/drivers/usb/tcpm/fusb302.c
@@ -10,6 +10,7 @@
 #include <asm/gpio.h>
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <power/regulator.h>
 #include <dm/device_compat.h>
 #include <usb/tcpm.h>
 #include "fusb302_reg.h"
@@ -50,10 +51,13 @@ struct fusb302_chip {
 
 	/* port status */
 	bool vconn_on;
+	bool vbus_on;
 	bool vbus_present;
 	enum typec_cc_polarity cc_polarity;
 	enum typec_cc_status cc1;
 	enum typec_cc_status cc2;
+
+	struct udevice *vbus;
 };
 
 static int fusb302_i2c_write(struct udevice *dev, u8 address, u8 data)
@@ -506,7 +510,28 @@ done:
 
 static int fusb302_set_vbus(struct udevice *dev, bool on, bool charge)
 {
-	return 0;
+	struct fusb302_chip *chip = dev_get_priv(dev);
+	int ret = 0;
+
+	if (chip->vbus_on == on) {
+		dev_dbg(dev, "vbus is already %s\n", on ? "On" : "Off");
+	} else {
+		if (CONFIG_IS_ENABLED(DM_REGULATOR) && chip->vbus) {
+			if (on)
+				ret = regulator_set_enable(chip->vbus, true);
+			else
+				ret = regulator_set_enable(chip->vbus, false);
+			if (ret < 0) {
+				dev_dbg(dev, "cannot %s vbus regulator, ret=%d\n",
+					on ? "enable" : "disable", ret);
+				return ret;
+			}
+		}
+		chip->vbus_on = on;
+		dev_dbg(dev, "vbus := %s\n", on ? "On" : "Off");
+	}
+
+	return ret;
 }
 
 static int fusb302_pd_tx_flush(struct udevice *dev)
@@ -1293,6 +1318,22 @@ static int fusb302_get_connector_node(struct udevice *dev, ofnode *connector_nod
 	return 0;
 }
 
+static int fusb302_probe(struct udevice *dev)
+{
+	struct fusb302_chip *chip = dev_get_priv(dev);
+	int ret;
+
+	if (CONFIG_IS_ENABLED(DM_REGULATOR)) {
+		ret = device_get_supply_regulator(dev, "vbus-supply", &chip->vbus);
+		if (ret && ret != -ENOENT) {
+			dev_err(dev, "Failed to get vbus-supply regulator\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static struct dm_tcpm_ops fusb302_ops = {
 	.get_connector_node = fusb302_get_connector_node,
 	.init = fusb302_init,
@@ -1320,4 +1361,5 @@ U_BOOT_DRIVER(fusb302) = {
 	.of_match = fusb302_ids,
 	.ops = &fusb302_ops,
 	.priv_auto = sizeof(struct fusb302_chip),
+	.probe = fusb302_probe,
 };
-- 
2.45.2

Re: [PATCH v1 2/3] usb: tcpm: fusb302: add support for set_vbus() callback.

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3472 bytes --]

Hi,

On Fri, Nov 01, 2024 at 01:34:51PM +0300, bigunclemax@gmail.com wrote:
> From: Maksim Kiselev <bigunclemax@gmail.com>
> 
> Add support for VBUS supply regulator.
> 
> When our type-c port acts as a host(SRC), this regulator
> used for control VBUS supply.
> 
> Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
> ---
>  drivers/usb/tcpm/fusb302.c | 44 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/tcpm/fusb302.c b/drivers/usb/tcpm/fusb302.c
> index ee283782792..2a258d6429b 100644
> --- a/drivers/usb/tcpm/fusb302.c
> +++ b/drivers/usb/tcpm/fusb302.c
> @@ -10,6 +10,7 @@
>  #include <asm/gpio.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
> +#include <power/regulator.h>
>  #include <dm/device_compat.h>
>  #include <usb/tcpm.h>
>  #include "fusb302_reg.h"
> @@ -50,10 +51,13 @@ struct fusb302_chip {
>  
>  	/* port status */
>  	bool vconn_on;
> +	bool vbus_on;
>  	bool vbus_present;
>  	enum typec_cc_polarity cc_polarity;
>  	enum typec_cc_status cc1;
>  	enum typec_cc_status cc2;
> +
> +	struct udevice *vbus;
>  };
>  
>  static int fusb302_i2c_write(struct udevice *dev, u8 address, u8 data)
> @@ -506,7 +510,28 @@ done:
>  
>  static int fusb302_set_vbus(struct udevice *dev, bool on, bool charge)
>  {
> -	return 0;
> +	struct fusb302_chip *chip = dev_get_priv(dev);
> +	int ret = 0;
> +
> +	if (chip->vbus_on == on) {
> +		dev_dbg(dev, "vbus is already %s\n", on ? "On" : "Off");
> +	} else {
> +		if (CONFIG_IS_ENABLED(DM_REGULATOR) && chip->vbus) {

should be enough to check for chip->vbus, since the regulator
device should be NULL when DM_REGULATOR is disabled. If due to
some bug its not NULL regulator_set_enable would fail gracefully
with -ENOSYS, so that somebody can debug the problem.

> +			if (on)
> +				ret = regulator_set_enable(chip->vbus, true);
> +			else
> +				ret = regulator_set_enable(chip->vbus, false);

regulator_set_enable(chip->vbus, on);

> +			if (ret < 0) {
> +				dev_dbg(dev, "cannot %s vbus regulator, ret=%d\n",
> +					on ? "enable" : "disable", ret);

dev_err()?

> +				return ret;
> +			}
> +		}
> +		chip->vbus_on = on;
> +		dev_dbg(dev, "vbus := %s\n", on ? "On" : "Off");
> +	}
> +
> +	return ret;

I suggest converting this to

int ret;

if (chip->vbus_on == on) {
    dev_dbg(...);
    return 0;
}

... remaining code without extra indent ...

Otherwise LGTM.

-- Sebastian


>  }
>  
>  static int fusb302_pd_tx_flush(struct udevice *dev)
> @@ -1293,6 +1318,22 @@ static int fusb302_get_connector_node(struct udevice *dev, ofnode *connector_nod
>  	return 0;
>  }
>  
> +static int fusb302_probe(struct udevice *dev)
> +{
> +	struct fusb302_chip *chip = dev_get_priv(dev);
> +	int ret;
> +
> +	if (CONFIG_IS_ENABLED(DM_REGULATOR)) {
> +		ret = device_get_supply_regulator(dev, "vbus-supply", &chip->vbus);
> +		if (ret && ret != -ENOENT) {
> +			dev_err(dev, "Failed to get vbus-supply regulator\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static struct dm_tcpm_ops fusb302_ops = {
>  	.get_connector_node = fusb302_get_connector_node,
>  	.init = fusb302_init,
> @@ -1320,4 +1361,5 @@ U_BOOT_DRIVER(fusb302) = {
>  	.of_match = fusb302_ids,
>  	.ops = &fusb302_ops,
>  	.priv_auto = sizeof(struct fusb302_chip),
> +	.probe = fusb302_probe,
>  };
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/3] usb: tcpm: fusb302: add support for set_vbus() callback.

[Jonas Karlman]

Hi Maksim,

On 2024-11-01 19:17, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Nov 01, 2024 at 01:34:51PM +0300, bigunclemax@gmail.com wrote:
>> From: Maksim Kiselev <bigunclemax@gmail.com>
>>
>> Add support for VBUS supply regulator.
>>
>> When our type-c port acts as a host(SRC), this regulator
>> used for control VBUS supply.
>>
>> Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
>> ---
>>  drivers/usb/tcpm/fusb302.c | 44 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/tcpm/fusb302.c b/drivers/usb/tcpm/fusb302.c
>> index ee283782792..2a258d6429b 100644
>> --- a/drivers/usb/tcpm/fusb302.c
>> +++ b/drivers/usb/tcpm/fusb302.c
>> @@ -10,6 +10,7 @@
>>  #include <asm/gpio.h>
>>  #include <linux/delay.h>
>>  #include <linux/err.h>
>> +#include <power/regulator.h>
>>  #include <dm/device_compat.h>
>>  #include <usb/tcpm.h>
>>  #include "fusb302_reg.h"
>> @@ -50,10 +51,13 @@ struct fusb302_chip {
>>  
>>  	/* port status */
>>  	bool vconn_on;
>> +	bool vbus_on;
>>  	bool vbus_present;
>>  	enum typec_cc_polarity cc_polarity;
>>  	enum typec_cc_status cc1;
>>  	enum typec_cc_status cc2;
>> +
>> +	struct udevice *vbus;
>>  };
>>  
>>  static int fusb302_i2c_write(struct udevice *dev, u8 address, u8 data)
>> @@ -506,7 +510,28 @@ done:
>>  
>>  static int fusb302_set_vbus(struct udevice *dev, bool on, bool charge)
>>  {
>> -	return 0;
>> +	struct fusb302_chip *chip = dev_get_priv(dev);
>> +	int ret = 0;
>> +
>> +	if (chip->vbus_on == on) {
>> +		dev_dbg(dev, "vbus is already %s\n", on ? "On" : "Off");
>> +	} else {
>> +		if (CONFIG_IS_ENABLED(DM_REGULATOR) && chip->vbus) {
> 
> should be enough to check for chip->vbus, since the regulator
> device should be NULL when DM_REGULATOR is disabled. If due to
> some bug its not NULL regulator_set_enable would fail gracefully
> with -ENOSYS, so that somebody can debug the problem.
> 
>> +			if (on)
>> +				ret = regulator_set_enable(chip->vbus, true);
>> +			else
>> +				ret = regulator_set_enable(chip->vbus, false);
> 
> regulator_set_enable(chip->vbus, on);

Please use something like following:

  ret = regulator_set_enable_if_allowed(chip->vbus, on);
  if (ret && ret != -ENOSYS)

Should be safe when DM_REGULATOR disabled or when chip->vbus is NULL,
also work when regulator is always-on or already enabled.

> 
>> +			if (ret < 0) {
>> +				dev_dbg(dev, "cannot %s vbus regulator, ret=%d\n",
>> +					on ? "enable" : "disable", ret);
> 
> dev_err()?
> 
>> +				return ret;
>> +			}
>> +		}
>> +		chip->vbus_on = on;

vbus_on could probably be dropped, fixed and gpio regulators is
currently already reference counted. Or is the set_vbus ops not called
evenly and current state needs to be known?

>> +		dev_dbg(dev, "vbus := %s\n", on ? "On" : "Off");
>> +	}
>> +
>> +	return ret;
> 
> I suggest converting this to
> 
> int ret;
> 
> if (chip->vbus_on == on) {
>     dev_dbg(...);
>     return 0;
> }
> 
> ... remaining code without extra indent ...
> 
> Otherwise LGTM.
> 
> -- Sebastian
> 
> 
>>  }
>>  
>>  static int fusb302_pd_tx_flush(struct udevice *dev)
>> @@ -1293,6 +1318,22 @@ static int fusb302_get_connector_node(struct udevice *dev, ofnode *connector_nod
>>  	return 0;
>>  }
>>  
>> +static int fusb302_probe(struct udevice *dev)
>> +{
>> +	struct fusb302_chip *chip = dev_get_priv(dev);
>> +	int ret;
>> +
>> +	if (CONFIG_IS_ENABLED(DM_REGULATOR)) {
>> +		ret = device_get_supply_regulator(dev, "vbus-supply", &chip->vbus);
>> +		if (ret && ret != -ENOENT) {

You could probably just drop the DM_REGULATOR wrap and use:

  if (ret && ret != -ENOSYS && ret != -ENOENT) {

Regards,
Jonas

>> +			dev_err(dev, "Failed to get vbus-supply regulator\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static struct dm_tcpm_ops fusb302_ops = {
>>  	.get_connector_node = fusb302_get_connector_node,
>>  	.init = fusb302_init,
>> @@ -1320,4 +1361,5 @@ U_BOOT_DRIVER(fusb302) = {
>>  	.of_match = fusb302_ids,
>>  	.ops = &fusb302_ops,
>>  	.priv_auto = sizeof(struct fusb302_chip),
>> +	.probe = fusb302_probe,
>>  };
>> -- 
>> 2.45.2
>>

Re: [PATCH v1 2/3] usb: tcpm: fusb302: add support for set_vbus() callback.

[Maxim Kiselev]

Hi, Sebastian, Jonas

Thanks for your suggestions.
I'll fix it in v2.

вс, 3 нояб. 2024 г. в 02:22, Jonas Karlman <jonas@kwiboo.se>:
> >> +            chip->vbus_on = on;
>
> vbus_on could probably be dropped, fixed and gpio regulators is
> currently already reference counted. Or is the set_vbus ops not called
> evenly and current state needs to be known?

Yep, vbus_on could be dropped. It is only necessary to display dev_dbg messages,
which are similar to Linux fusb302_log.

So, do you suggest removing this?

Best wishes,
Maksim

[PATCH v1 3/3] usb: tcpm: fix toggling in host (SRC) mode

[bigunclemax]

From: Maksim Kiselev <bigunclemax@gmail.com>

PU\PD resistors on CC lines must be configured before running the TCPM
state machine.

Also, when the Type-C port acts as a host (SRC), the VBUS sould be enabled
only after the toggling has been completed. And we have to wait for
the corresponding IRQ to finish the toggling process. But this doesn't
happen because we exit from tcpm_poll_event() if VBUS != OK.

So, Let's check for VBUS state only when the port acts as a host (SRC)
to solve this problem.

Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
---
 drivers/usb/tcpm/tcpm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/tcpm/tcpm.c b/drivers/usb/tcpm/tcpm.c
index 0aee57cb2f4..c9f8dbdc795 100644
--- a/drivers/usb/tcpm/tcpm.c
+++ b/drivers/usb/tcpm/tcpm.c
@@ -2122,6 +2122,12 @@ static void tcpm_init(struct udevice *dev)
 	else
 		port->vbus_vsafe0v = true;
 
+	if (port->self_powered)
+		tcpm_set_cc(dev, TYPEC_CC_OPEN);
+	else
+		tcpm_set_cc(dev, tcpm_default_state(port) == SNK_UNATTACHED ?
+				TYPEC_CC_RD : tcpm_rp_cc(port));
+
 	tcpm_set_state(dev, tcpm_default_state(port), 0);
 
 	if (drvops->get_cc(dev, &cc1, &cc2) == 0)
@@ -2234,7 +2240,7 @@ static void tcpm_poll_event(struct udevice *dev)
 	const struct dm_tcpm_ops *drvops = dev_get_driver_ops(dev);
 	struct tcpm_port *port = dev_get_uclass_plat(dev);
 
-	if (!drvops->get_vbus(dev))
+	if (!drvops->get_vbus(dev) && (tcpm_default_state(port) == SNK_UNATTACHED))
 		return;
 
 	while (port->poll_event_cnt < TCPM_POLL_EVENT_TIME_OUT) {
-- 
2.45.2

Re: [PATCH v1 3/3] usb: tcpm: fix toggling in host (SRC) mode

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

Hi,

On Fri, Nov 01, 2024 at 01:34:52PM +0300, bigunclemax@gmail.com wrote:
> From: Maksim Kiselev <bigunclemax@gmail.com>
> 
> PU\PD resistors on CC lines must be configured before running the TCPM
> state machine.
> 
> Also, when the Type-C port acts as a host (SRC), the VBUS sould be enabled
> only after the toggling has been completed. And we have to wait for
> the corresponding IRQ to finish the toggling process. But this doesn't
> happen because we exit from tcpm_poll_event() if VBUS != OK.
> 
> So, Let's check for VBUS state only when the port acts as a host (SRC)
> to solve this problem.
> 
> Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
> ---

I will look into this patch next week and give it a try on the Rock
5B. As a first feedback the commit message seems to confuse data and
power roles. SRC (source) and SNK (sink) are for the power direction
while "host" and "device" are used for the data direction. With
USB-PD these are independent from each other. You seem to have the
old USB model in mind where e.g. a USB keyboard ("device") gets
power from the PC ("host"). But USB-PD allows setups like a USB-C
hub ("device") having its own power-supply and providing a laptop
("host") with power.

Greetings,

-- Sebastian

>  drivers/usb/tcpm/tcpm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/tcpm/tcpm.c b/drivers/usb/tcpm/tcpm.c
> index 0aee57cb2f4..c9f8dbdc795 100644
> --- a/drivers/usb/tcpm/tcpm.c
> +++ b/drivers/usb/tcpm/tcpm.c
> @@ -2122,6 +2122,12 @@ static void tcpm_init(struct udevice *dev)
>  	else
>  		port->vbus_vsafe0v = true;
>  
> +	if (port->self_powered)
> +		tcpm_set_cc(dev, TYPEC_CC_OPEN);
> +	else
> +		tcpm_set_cc(dev, tcpm_default_state(port) == SNK_UNATTACHED ?
> +				TYPEC_CC_RD : tcpm_rp_cc(port));
> +
>  	tcpm_set_state(dev, tcpm_default_state(port), 0);
>  
>  	if (drvops->get_cc(dev, &cc1, &cc2) == 0)
> @@ -2234,7 +2240,7 @@ static void tcpm_poll_event(struct udevice *dev)
>  	const struct dm_tcpm_ops *drvops = dev_get_driver_ops(dev);
>  	struct tcpm_port *port = dev_get_uclass_plat(dev);
>  
> -	if (!drvops->get_vbus(dev))
> +	if (!drvops->get_vbus(dev) && (tcpm_default_state(port) == SNK_UNATTACHED))
>  		return;
>  
>  	while (port->poll_event_cnt < TCPM_POLL_EVENT_TIME_OUT) {
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 3/3] usb: tcpm: fix toggling in host (SRC) mode

[Maxim Kiselev]

Hi,

пт, 1 нояб. 2024 г. в 21:32, Sebastian Reichel
<sebastian.reichel@collabora.com>:
>
> Hi,
>
> On Fri, Nov 01, 2024 at 01:34:52PM +0300, bigunclemax@gmail.com wrote:
> > From: Maksim Kiselev <bigunclemax@gmail.com>
> >
> > PU\PD resistors on CC lines must be configured before running the TCPM
> > state machine.
> >
> > Also, when the Type-C port acts as a host (SRC), the VBUS sould be enabled
> > only after the toggling has been completed. And we have to wait for
> > the corresponding IRQ to finish the toggling process. But this doesn't
> > happen because we exit from tcpm_poll_event() if VBUS != OK.
> >
> > So, Let's check for VBUS state only when the port acts as a host (SRC)
> > to solve this problem.
> >
> > Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
> > ---
>
> I will look into this patch next week and give it a try on the Rock
> 5B. As a first feedback the commit message seems to confuse data and
> power roles. SRC (source) and SNK (sink) are for the power direction
> while "host" and "device" are used for the data direction. With
> USB-PD these are independent from each other.

Yes, you're right. I'll drop a host mention from the commit message in
the next version.
This patch is for the case when our board acts as a source (SRC).

Best wishes,
Maksim

> You seem to have the
> old USB model in mind where e.g. a USB keyboard ("device") gets
> power from the PC ("host"). But USB-PD allows setups like a USB-C
> hub ("device") having its own power-supply and providing a laptop
> ("host") with power.
>
> Greetings,
>
> -- Sebastian
>
> >  drivers/usb/tcpm/tcpm.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/tcpm/tcpm.c b/drivers/usb/tcpm/tcpm.c
> > index 0aee57cb2f4..c9f8dbdc795 100644
> > --- a/drivers/usb/tcpm/tcpm.c
> > +++ b/drivers/usb/tcpm/tcpm.c
> > @@ -2122,6 +2122,12 @@ static void tcpm_init(struct udevice *dev)
> >       else
> >               port->vbus_vsafe0v = true;
> >
> > +     if (port->self_powered)
> > +             tcpm_set_cc(dev, TYPEC_CC_OPEN);
> > +     else
> > +             tcpm_set_cc(dev, tcpm_default_state(port) == SNK_UNATTACHED ?
> > +                             TYPEC_CC_RD : tcpm_rp_cc(port));
> > +
> >       tcpm_set_state(dev, tcpm_default_state(port), 0);
> >
> >       if (drvops->get_cc(dev, &cc1, &cc2) == 0)
> > @@ -2234,7 +2240,7 @@ static void tcpm_poll_event(struct udevice *dev)
> >       const struct dm_tcpm_ops *drvops = dev_get_driver_ops(dev);
> >       struct tcpm_port *port = dev_get_uclass_plat(dev);
> >
> > -     if (!drvops->get_vbus(dev))
> > +     if (!drvops->get_vbus(dev) && (tcpm_default_state(port) == SNK_UNATTACHED))
> >               return;
> >
> >       while (port->poll_event_cnt < TCPM_POLL_EVENT_TIME_OUT) {
> > --
> > 2.45.2
> >