[PATCH v1 0/3] USB TCPM host (SRC) mode improvements
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(-)
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
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
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, };
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);elseret = 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
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);elseret = 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
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
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) {
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
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);elsetcpm_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
participants (4)
-
bigunclemax＠gmail.com -
Jonas Karlman -
Maxim Kiselev
-
Sebastian Reichel