2019-03-22 12:31:57

by Kyle Tso

Subject: [PATCH v2] usb: typec: tcpm: collision avoidance

This patch provides the implementation of Collision Avoidance introduced
in PD3.0. The start of each Atomic Message Sequence (AMS) initiated by
the port will be denied if the current AMS is not interruptible. The
Source port will set the CC to SinkTxNG if it is going to initiate an
AMS, and SinkTxOk otherwise. Meanwhile, any AMS initiated by a Sink port
will be denied in TCPM if the port partner (Source) sets SinkTxNG except
for HARD_RESET and SOFT_RESET.

Signed-off-by: Kyle Tso <[email protected]>
---
Changelog since v1:
(suggested by Guenter Roeck <[email protected]>)
- removed macro tcpm_switch_state
- defined new macro support_ams
- added new function tcpm_ams_finish separated from tcpm_ams_start
and move tcpm_set_cc before tcpm_pd_transmit
- re-designed the log format in tcpm_set_state and tcpm_set_state_cond

drivers/usb/typec/tcpm/tcpm.c | 538 ++++++++++++++++++++++++++++++----
include/linux/usb/pd.h | 1 +
include/linux/usb/tcpm.h | 4 +
3 files changed, 483 insertions(+), 60 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 0f62db091d8d..c9242fcccf76 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -71,6 +71,8 @@
S(SNK_HARD_RESET_SINK_ON), \
\
S(SOFT_RESET), \
+ S(SRC_SOFT_RESET_WAIT_SNK_TX), \
+ S(SNK_SOFT_RESET), \
S(SOFT_RESET_SEND), \
\
S(DR_SWAP_ACCEPT), \
@@ -124,7 +126,45 @@
\
S(ERROR_RECOVERY), \
S(PORT_RESET), \
- S(PORT_RESET_WAIT_OFF)
+ S(PORT_RESET_WAIT_OFF), \
+ \
+ S(AMS_START)
+
+#define FOREACH_AMS(S) \
+ S(NONE_AMS), \
+ S(POWER_NEGOTIATION), \
+ S(GOTOMIN), \
+ S(SOFT_RESET_AMS), \
+ S(HARD_RESET), \
+ S(CABLE_RESET), \
+ S(GET_SOURCE_CAPABILITIES), \
+ S(GET_SINK_CAPABILITIES), \
+ S(POWER_ROLE_SWAP), \
+ S(FAST_ROLE_SWAP), \
+ S(DATA_ROLE_SWAP), \
+ S(VCONN_SWAP), \
+ S(SOURCE_ALERT), \
+ S(GETTING_SOURCE_EXTENDED_CAPABILITIES),\
+ S(GETTING_SOURCE_SINK_STATUS), \
+ S(GETTING_BATTERY_CAPABILITIES), \
+ S(GETTING_BATTERY_STATUS), \
+ S(GETTING_MANUFACTURER_INFORMATION), \
+ S(SECURITY), \
+ S(FIRMWARE_UPDATE), \
+ S(DISCOVER_IDENTITY), \
+ S(SOURCE_STARTUP_CABLE_PLUG_DISCOVER_IDENTITY), \
+ S(DISCOVER_SVIDS), \
+ S(DISCOVER_MODES), \
+ S(DFP_TO_UFP_ENTER_MODE), \
+ S(DFP_TO_UFP_EXIT_MODE), \
+ S(DFP_TO_CABLE_PLUG_ENTER_MODE), \
+ S(DFP_TO_CABLE_PLUG_EXIT_MODE), \
+ S(ATTENTION), \
+ S(BIST), \
+ S(UNSTRUCTURED_VDMS), \
+ S(STRUCTURED_VDMS), \
+ S(COUNTRY_INFO), \
+ S(COUNTRY_CODES)

#define GENERATE_ENUM(e) e
#define GENERATE_STRING(s) #s
@@ -137,6 +177,14 @@ static const char * const tcpm_states[] = {
FOREACH_STATE(GENERATE_STRING)
};

+enum tcpm_ams {
+ FOREACH_AMS(GENERATE_ENUM)
+};
+
+static const char * const tcpm_ams_str[] = {
+ FOREACH_AMS(GENERATE_STRING)
+};
+
enum vdm_states {
VDM_STATE_ERR_BUSY = -3,
VDM_STATE_ERR_SEND = -2,
@@ -146,6 +194,7 @@ enum vdm_states {
VDM_STATE_READY = 1,
VDM_STATE_BUSY = 2,
VDM_STATE_WAIT_RSP_BUSY = 3,
+ VDM_STATE_SEND_MESSAGE = 4,
};

enum pd_msg_request {
@@ -320,6 +369,11 @@ struct tcpm_port {
/* port belongs to a self powered device */
bool self_powered;

+ /* Collision Avoidance and Atomic Message Sequence */
+ enum tcpm_state upcoming_state;
+ enum tcpm_ams ams;
+ bool in_ams;
+
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
struct mutex logbuffer_lock; /* log buffer access lock */
@@ -371,6 +425,12 @@ struct pd_rx_event {
((port)->try_src_count == 0 && (port)->try_role == TYPEC_SOURCE && \
(port)->port_type == TYPEC_PORT_DRP)

+#define tcpm_sink_tx_ok(port) \
+ (tcpm_port_is_sink(port) && \
+ ((port)->cc1 == TYPEC_CC_RP_3_0 || (port)->cc2 == TYPEC_CC_RP_3_0))
+
+#define support_ams(port) ((port)->negotiated_rev >= PD_REV30)
+
static enum tcpm_state tcpm_default_state(struct tcpm_port *port)
{
if (port->port_type == TYPEC_PORT_DRP) {
@@ -600,6 +660,33 @@ static void tcpm_debugfs_exit(const struct tcpm_port *port) { }

#endif

+static void tcpm_set_cc(struct tcpm_port *port, enum typec_cc_status cc)
+{
+ tcpm_log(port, "cc:=%d", cc);
+ port->cc_req = cc;
+ port->tcpc->set_cc(port->tcpc, cc);
+}
+
+static int tcpm_ams_finish(struct tcpm_port *port)
+{
+ int ret = 0;
+
+ if (!support_ams(port)) {
+ port->upcoming_state = INVALID_STATE;
+ return -EOPNOTSUPP;
+ }
+
+ tcpm_log(port, "AMS %s finished", tcpm_ams_str[port->ams]);
+
+ if (port->pwr_role == TYPEC_SOURCE)
+ tcpm_set_cc(port, SINK_TX_OK);
+
+ port->in_ams = false;
+ port->ams = NONE_AMS;
+
+ return ret;
+}
+
static int tcpm_pd_transmit(struct tcpm_port *port,
enum tcpm_transmit_type type,
const struct pd_message *msg)
@@ -627,13 +714,29 @@ static int tcpm_pd_transmit(struct tcpm_port *port,
switch (port->tx_status) {
case TCPC_TX_SUCCESS:
port->message_id = (port->message_id + 1) & PD_HEADER_ID_MASK;
- return 0;
+ /*
+ * USB PD rev 3.0, 8.3.2.1.3:
+ * "... Note that every AMS is Interruptible until the first
+ * Message in the sequence has been successfully sent (GoodCRC
+ * Message received)."
+ */
+ if (support_ams(port) && port->ams != NONE_AMS)
+ port->in_ams = true;
+ break;
case TCPC_TX_DISCARDED:
- return -EAGAIN;
+ ret = -EAGAIN;
+ break;
case TCPC_TX_FAILED:
default:
- return -EIO;
+ ret = -EIO;
+ break;
}
+
+ /* Some AMS don't expect responses. Finish them here. */
+ if (port->ams == ATTENTION || port->ams == SOURCE_ALERT)
+ tcpm_ams_finish(port);
+
+ return ret;
}

void tcpm_pd_transmit_complete(struct tcpm_port *port,
@@ -883,17 +986,20 @@ static void tcpm_set_state(struct tcpm_port *port, enum tcpm_state state,
unsigned int delay_ms)
{
if (delay_ms) {
- tcpm_log(port, "pending state change %s -> %s @ %u ms",
- tcpm_states[port->state], tcpm_states[state],
- delay_ms);
+ tcpm_log(port, "pending state change %s -> %s @ %u ms%s%s",
+ tcpm_states[port->state], tcpm_states[state], delay_ms,
+ support_ams(port) ? " in AMS " : "",
+ support_ams(port) ? tcpm_ams_str[port->ams] : "");
port->delayed_state = state;
mod_delayed_work(port->wq, &port->state_machine,
msecs_to_jiffies(delay_ms));
port->delayed_runtime = jiffies + msecs_to_jiffies(delay_ms);
port->delay_ms = delay_ms;
} else {
- tcpm_log(port, "state change %s -> %s",
- tcpm_states[port->state], tcpm_states[state]);
+ tcpm_log(port, "state change %s -> %s%s%s",
+ tcpm_states[port->state], tcpm_states[state],
+ support_ams(port) ? " in AMS " : "",
+ support_ams(port) ? tcpm_ams_str[port->ams] : "");
port->delayed_state = INVALID_STATE;
port->prev_state = port->state;
port->state = state;
@@ -915,10 +1021,12 @@ static void tcpm_set_state_cond(struct tcpm_port *port, enum tcpm_state state,
tcpm_set_state(port, state, delay_ms);
else
tcpm_log(port,
- "skipped %sstate change %s -> %s [%u ms], context state %s",
+ "skipped %sstate change %s -> %s [%u ms], context state %s%s%s",
delay_ms ? "delayed " : "",
tcpm_states[port->state], tcpm_states[state],
- delay_ms, tcpm_states[port->enter_state]);
+ delay_ms, tcpm_states[port->enter_state],
+ support_ams(port) ? " in AMS " : "",
+ support_ams(port) ? tcpm_ams_str[port->ams] : "");
}

static void tcpm_queue_message(struct tcpm_port *port,
@@ -928,6 +1036,139 @@ static void tcpm_queue_message(struct tcpm_port *port,
mod_delayed_work(port->wq, &port->state_machine, 0);
}

+static bool tcpm_vdm_ams(struct tcpm_port *port)
+{
+ switch (port->ams) {
+ case DISCOVER_IDENTITY:
+ case SOURCE_STARTUP_CABLE_PLUG_DISCOVER_IDENTITY:
+ case DISCOVER_SVIDS:
+ case DISCOVER_MODES:
+ case DFP_TO_UFP_ENTER_MODE:
+ case DFP_TO_UFP_EXIT_MODE:
+ case DFP_TO_CABLE_PLUG_ENTER_MODE:
+ case DFP_TO_CABLE_PLUG_EXIT_MODE:
+ case ATTENTION:
+ case UNSTRUCTURED_VDMS:
+ case STRUCTURED_VDMS:
+ break;
+ default:
+ return false;
+ }
+
+ return true;
+}
+
+static bool tcpm_ams_interruptible(struct tcpm_port *port)
+{
+ switch (port->ams) {
+ /* Interruptible AMS */
+ case NONE_AMS:
+ case SECURITY:
+ case FIRMWARE_UPDATE:
+ case DISCOVER_IDENTITY:
+ case SOURCE_STARTUP_CABLE_PLUG_DISCOVER_IDENTITY:
+ case DISCOVER_SVIDS:
+ case DISCOVER_MODES:
+ case DFP_TO_UFP_ENTER_MODE:
+ case DFP_TO_UFP_EXIT_MODE:
+ case DFP_TO_CABLE_PLUG_ENTER_MODE:
+ case DFP_TO_CABLE_PLUG_EXIT_MODE:
+ case UNSTRUCTURED_VDMS:
+ case STRUCTURED_VDMS:
+ case COUNTRY_INFO:
+ case COUNTRY_CODES:
+ break;
+ /* Non-Interruptible AMS */
+ default:
+ if (port->in_ams)
+ return false;
+ break;
+ }
+
+ return true;
+}
+
+static int tcpm_ams_start(struct tcpm_port *port, enum tcpm_ams ams)
+{
+ int ret = 0;
+
+ if (port->negotiated_rev < PD_REV30) {
+ port->upcoming_state = INVALID_STATE;
+ return -EOPNOTSUPP;
+ }
+
+ tcpm_log(port, "AMS %s start", tcpm_ams_str[ams]);
+
+ if (!tcpm_ams_interruptible(port) && ams != HARD_RESET) {
+ port->upcoming_state = INVALID_STATE;
+ tcpm_log(port, "AMS %s not interruptible, aborting",
+ tcpm_ams_str[port->ams]);
+ return -EAGAIN;
+ }
+
+ if (port->pwr_role == TYPEC_SOURCE) {
+ enum typec_cc_status cc_req;
+
+ port->ams = ams;
+
+ if (ams == HARD_RESET) {
+ tcpm_set_cc(port, tcpm_rp_cc(port));
+ tcpm_pd_transmit(port, TCPC_TX_HARD_RESET, NULL);
+ tcpm_set_state(port, HARD_RESET_START, 0);
+ return 0;
+ } else if (ams == SOFT_RESET_AMS) {
+ if (!port->explicit_contract) {
+ port->upcoming_state = INVALID_STATE;
+ tcpm_set_cc(port, tcpm_rp_cc(port));
+ return 0;
+ }
+ } else if (tcpm_vdm_ams(port)) {
+ /* tSinkTx is enforced in vdm_run_state_machine */
+ tcpm_set_cc(port, SINK_TX_NG);
+ return 0;
+ }
+
+ cc_req = port->cc_req;
+ tcpm_set_cc(port, SINK_TX_NG);
+ if (port->state == SRC_READY ||
+ port->state == SRC_STARTUP ||
+ port->state == SRC_SOFT_RESET_WAIT_SNK_TX ||
+ port->state == SOFT_RESET ||
+ port->state == SOFT_RESET_SEND)
+ tcpm_set_state(port, AMS_START, cc_req == SINK_TX_OK ?
+ PD_T_SINK_TX : 0);
+ else
+ tcpm_set_state(port, SRC_READY, cc_req == SINK_TX_OK ?
+ PD_T_SINK_TX : 0);
+ } else {
+ if (!tcpm_sink_tx_ok(port) &&
+ ams != SOFT_RESET_AMS &&
+ ams != HARD_RESET) {
+ port->upcoming_state = INVALID_STATE;
+ tcpm_log(port, "Sink TX No Go");
+ return -EAGAIN;
+ }
+
+ port->ams = ams;
+
+ if (ams == HARD_RESET) {
+ tcpm_pd_transmit(port, TCPC_TX_HARD_RESET, NULL);
+ tcpm_set_state(port, HARD_RESET_START, 0);
+ return 0;
+ } else if (tcpm_vdm_ams(port)) {
+ return 0;
+ }
+
+ if (port->state == SNK_READY ||
+ port->state == SNK_SOFT_RESET)
+ tcpm_set_state(port, AMS_START, 0);
+ else
+ tcpm_set_state(port, SNK_READY, 0);
+ }
+
+ return ret;
+}
+
/*
* VDM/VDO handling functions
*/
@@ -1164,6 +1405,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
default:
break;
}
+ tcpm_ams_finish(port);
break;
case CMDT_RSP_NAK:
switch (cmd) {
@@ -1175,6 +1417,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
default:
break;
}
+ tcpm_ams_finish(port);
break;
default:
break;
@@ -1260,7 +1503,7 @@ static unsigned int vdm_ready_timeout(u32 vdm_hdr)
static void vdm_run_state_machine(struct tcpm_port *port)
{
struct pd_message msg;
- int i, res;
+ int i, res = 0;

switch (port->vdm_state) {
case VDM_STATE_READY:
@@ -1277,27 +1520,43 @@ static void vdm_run_state_machine(struct tcpm_port *port)
if (port->state != SRC_READY && port->state != SNK_READY)
break;

- /* Prepare and send VDM */
- memset(&msg, 0, sizeof(msg));
- msg.header = PD_HEADER_LE(PD_DATA_VENDOR_DEF,
- port->pwr_role,
- port->data_role,
- port->negotiated_rev,
- port->message_id, port->vdo_count);
- for (i = 0; i < port->vdo_count; i++)
- msg.payload[i] = cpu_to_le32(port->vdo_data[i]);
- res = tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
- if (res < 0) {
- port->vdm_state = VDM_STATE_ERR_SEND;
- } else {
- unsigned long timeout;
+ if (PD_VDO_CMDT(port->vdo_data[0]) == CMDT_INIT) {
+ switch (PD_VDO_CMD(port->vdo_data[0])) {
+ case CMD_DISCOVER_IDENT:
+ res = tcpm_ams_start(port, DISCOVER_IDENTITY);
+ break;
+ case CMD_DISCOVER_SVID:
+ res = tcpm_ams_start(port, DISCOVER_SVIDS);
+ break;
+ case CMD_DISCOVER_MODES:
+ res = tcpm_ams_start(port, DISCOVER_MODES);
+ break;
+ case CMD_ENTER_MODE:
+ res = tcpm_ams_start(port,
+ DFP_TO_UFP_ENTER_MODE);
+ break;
+ case CMD_EXIT_MODE:
+ res = tcpm_ams_start(port,
+ DFP_TO_UFP_EXIT_MODE);
+ break;
+ case CMD_ATTENTION:
+ res = tcpm_ams_start(port, ATTENTION);
+ break;
+ default:
+ res = -EOPNOTSUPP;
+ break;
+ }

- port->vdm_retries = 0;
- port->vdm_state = VDM_STATE_BUSY;
- timeout = vdm_ready_timeout(port->vdo_data[0]);
- mod_delayed_work(port->wq, &port->vdm_state_machine,
- timeout);
+ if (res == -EAGAIN)
+ return;
}
+
+ port->vdm_state = VDM_STATE_SEND_MESSAGE;
+ mod_delayed_work(port->wq, &port->vdm_state_machine,
+ (res != -EOPNOTSUPP) &&
+ (port->pwr_role == TYPEC_SOURCE) &&
+ (PD_VDO_CMDT(port->vdo_data[0]) == CMDT_INIT) ?
+ PD_T_SINK_TX : 0);
break;
case VDM_STATE_WAIT_RSP_BUSY:
port->vdo_data[0] = port->vdo_retry;
@@ -1306,6 +1565,8 @@ static void vdm_run_state_machine(struct tcpm_port *port)
break;
case VDM_STATE_BUSY:
port->vdm_state = VDM_STATE_ERR_TMOUT;
+ if (port->ams != NONE_AMS)
+ tcpm_ams_finish(port);
break;
case VDM_STATE_ERR_SEND:
/*
@@ -1318,6 +1579,30 @@ static void vdm_run_state_machine(struct tcpm_port *port)
tcpm_log(port, "VDM Tx error, retry");
port->vdm_retries++;
port->vdm_state = VDM_STATE_READY;
+ tcpm_ams_finish(port);
+ }
+ break;
+ case VDM_STATE_SEND_MESSAGE:
+ /* Prepare and send VDM */
+ memset(&msg, 0, sizeof(msg));
+ msg.header = PD_HEADER_LE(PD_DATA_VENDOR_DEF,
+ port->pwr_role,
+ port->data_role,
+ port->negotiated_rev,
+ port->message_id, port->vdo_count);
+ for (i = 0; i < port->vdo_count; i++)
+ msg.payload[i] = cpu_to_le32(port->vdo_data[i]);
+ res = tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
+ if (res < 0) {
+ port->vdm_state = VDM_STATE_ERR_SEND;
+ } else {
+ unsigned long timeout;
+
+ port->vdm_retries = 0;
+ port->vdm_state = VDM_STATE_BUSY;
+ timeout = vdm_ready_timeout(port->vdo_data[0]);
+ mod_delayed_work(port->wq, &port->vdm_state_machine,
+ timeout);
}
break;
default:
@@ -1341,7 +1626,8 @@ static void vdm_state_machine_work(struct work_struct *work)
prev_state = port->vdm_state;
vdm_run_state_machine(port);
} while (port->vdm_state != prev_state &&
- port->vdm_state != VDM_STATE_BUSY);
+ port->vdm_state != VDM_STATE_BUSY &&
+ port->vdm_state != VDM_STATE_SEND_MESSAGE);

mutex_unlock(&port->lock);
}
@@ -1671,6 +1957,7 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
{
enum pd_ctrl_msg_type type = pd_header_type_le(msg->header);
enum tcpm_state next_state;
+ int ret = 0;

switch (type) {
case PD_CTRL_GOOD_CRC:
@@ -1785,11 +2072,18 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
case SOFT_RESET_SEND:
port->message_id = 0;
port->rx_msgid = -1;
- if (port->pwr_role == TYPEC_SOURCE)
- next_state = SRC_SEND_CAPABILITIES;
- else
- next_state = SNK_WAIT_CAPABILITIES;
- tcpm_set_state(port, next_state, 0);
+ if (port->ams == SOFT_RESET_AMS)
+ tcpm_ams_finish(port);
+ if (port->pwr_role == TYPEC_SOURCE) {
+ port->upcoming_state = SRC_SEND_CAPABILITIES;
+ ret = tcpm_ams_start(port, POWER_NEGOTIATION);
+ if (ret == -EOPNOTSUPP)
+ tcpm_set_state(port,
+ SRC_SEND_CAPABILITIES,
+ 0);
+ } else {
+ tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
+ }
break;
case DR_SWAP_SEND:
tcpm_set_state(port, DR_SWAP_CHANGE_DR, 0);
@@ -2555,13 +2849,6 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
return false;
}

-static void tcpm_set_cc(struct tcpm_port *port, enum typec_cc_status cc)
-{
- tcpm_log(port, "cc:=%d", cc);
- port->cc_req = cc;
- port->tcpc->set_cc(port->tcpc, cc);
-}
-
static int tcpm_init_vbus(struct tcpm_port *port)
{
int ret;
@@ -2680,6 +2967,8 @@ static void tcpm_unregister_altmodes(struct tcpm_port *port)

static void tcpm_reset_port(struct tcpm_port *port)
{
+ port->in_ams = false;
+ port->ams = NONE_AMS;
tcpm_unregister_altmodes(port);
tcpm_typec_disconnect(port);
port->attached = false;
@@ -2843,6 +3132,7 @@ static void run_state_machine(struct tcpm_port *port)
int ret;
enum typec_pwr_opmode opmode;
unsigned int msecs;
+ enum tcpm_state upcoming_state;

port->enter_state = port->state;
switch (port->state) {
@@ -2945,7 +3235,14 @@ static void run_state_machine(struct tcpm_port *port)
port->message_id = 0;
port->rx_msgid = -1;
port->explicit_contract = false;
- tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0);
+ /* SNK -> SRC POWER/FAST_ROLE_SWAP finished */
+ if (port->ams == POWER_ROLE_SWAP ||
+ port->ams == FAST_ROLE_SWAP)
+ tcpm_ams_finish(port);
+ port->upcoming_state = SRC_SEND_CAPABILITIES;
+ ret = tcpm_ams_start(port, POWER_NEGOTIATION);
+ if (ret == -EOPNOTSUPP)
+ tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0);
break;
case SRC_SEND_CAPABILITIES:
port->caps_count++;
@@ -3003,6 +3300,19 @@ static void run_state_machine(struct tcpm_port *port)
tcpm_swap_complete(port, 0);
tcpm_typec_connect(port);

+ if (port->ams != NONE_AMS)
+ tcpm_ams_finish(port);
+ /*
+ * If previous AMS is interrupted, switch to the upcoming
+ * state.
+ */
+ upcoming_state = port->upcoming_state;
+ if (port->upcoming_state != INVALID_STATE) {
+ port->upcoming_state = INVALID_STATE;
+ tcpm_set_state(port, upcoming_state, 0);
+ break;
+ }
+
tcpm_check_send_discover(port);
/*
* 6.3.5
@@ -3120,6 +3430,12 @@ static void run_state_machine(struct tcpm_port *port)
port->message_id = 0;
port->rx_msgid = -1;
port->explicit_contract = false;
+
+ if (port->ams == POWER_ROLE_SWAP ||
+ port->ams == FAST_ROLE_SWAP)
+ /* SRC -> SNK POWER/FAST_ROLE_SWAP finished */
+ tcpm_ams_finish(port);
+
tcpm_set_state(port, SNK_DISCOVERY, 0);
break;
case SNK_DISCOVERY:
@@ -3169,7 +3485,7 @@ static void run_state_machine(struct tcpm_port *port)
*/
if (port->vbus_never_low) {
port->vbus_never_low = false;
- tcpm_set_state(port, SOFT_RESET_SEND,
+ tcpm_set_state(port, SNK_SOFT_RESET,
PD_T_SINK_WAIT_CAP);
} else {
tcpm_set_state(port, hard_reset_state(port),
@@ -3222,9 +3538,22 @@ static void run_state_machine(struct tcpm_port *port)

tcpm_swap_complete(port, 0);
tcpm_typec_connect(port);
- tcpm_check_send_discover(port);
tcpm_pps_complete(port, port->pps_status);

+ if (port->ams != NONE_AMS)
+ tcpm_ams_finish(port);
+ /*
+ * If previous AMS is interrupted, switch to the upcoming
+ * state.
+ */
+ upcoming_state = port->upcoming_state;
+ if (port->upcoming_state != INVALID_STATE) {
+ port->upcoming_state = INVALID_STATE;
+ tcpm_set_state(port, upcoming_state, 0);
+ break;
+ }
+
+ tcpm_check_send_discover(port);
power_supply_changed(port->psy);

break;
@@ -3246,8 +3575,18 @@ static void run_state_machine(struct tcpm_port *port)

/* Hard_Reset states */
case HARD_RESET_SEND:
- tcpm_pd_transmit(port, TCPC_TX_HARD_RESET, NULL);
- tcpm_set_state(port, HARD_RESET_START, 0);
+ if (port->ams != NONE_AMS)
+ tcpm_ams_finish(port);
+ /*
+ * State machine will be directed to HARD_RESET_START,
+ * thus set upcoming_state to INVALID_STATE.
+ */
+ port->upcoming_state = INVALID_STATE;
+ ret = tcpm_ams_start(port, HARD_RESET);
+ if (ret == -EOPNOTSUPP) {
+ tcpm_pd_transmit(port, TCPC_TX_HARD_RESET, NULL);
+ tcpm_set_state(port, HARD_RESET_START, 0);
+ }
break;
case HARD_RESET_START:
port->hard_reset_count++;
@@ -3269,6 +3608,8 @@ static void run_state_machine(struct tcpm_port *port)
break;
case SRC_HARD_RESET_VBUS_ON:
tcpm_set_vbus(port, true);
+ if (port->ams == HARD_RESET)
+ tcpm_ams_finish(port);
port->tcpc->set_pd_rx(port->tcpc, true);
tcpm_set_attached_state(port, true);
tcpm_set_state(port, SRC_UNATTACHED, PD_T_PS_SOURCE_ON);
@@ -3288,6 +3629,8 @@ static void run_state_machine(struct tcpm_port *port)
tcpm_set_state(port, SNK_HARD_RESET_SINK_ON, PD_T_SAFE_0V);
break;
case SNK_HARD_RESET_WAIT_VBUS:
+ if (port->ams == HARD_RESET)
+ tcpm_ams_finish(port);
/* Assume we're disconnected if VBUS doesn't come back. */
tcpm_set_state(port, SNK_UNATTACHED,
PD_T_SRC_RECOVER_MAX + PD_T_SRC_TURN_ON);
@@ -3315,6 +3658,8 @@ static void run_state_machine(struct tcpm_port *port)
5000);
tcpm_set_charge(port, true);
}
+ if (port->ams == HARD_RESET)
+ tcpm_ams_finish(port);
tcpm_set_attached_state(port, true);
tcpm_set_state(port, SNK_STARTUP, 0);
break;
@@ -3324,10 +3669,23 @@ static void run_state_machine(struct tcpm_port *port)
port->message_id = 0;
port->rx_msgid = -1;
tcpm_pd_send_control(port, PD_CTRL_ACCEPT);
- if (port->pwr_role == TYPEC_SOURCE)
- tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0);
- else
+ if (port->pwr_role == TYPEC_SOURCE) {
+ port->upcoming_state = SRC_SEND_CAPABILITIES;
+ ret = tcpm_ams_start(port, POWER_NEGOTIATION);
+ if (ret == -EOPNOTSUPP)
+ tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0);
+ } else {
tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
+ }
+ break;
+ case SRC_SOFT_RESET_WAIT_SNK_TX:
+ case SNK_SOFT_RESET:
+ if (port->ams != NONE_AMS)
+ tcpm_ams_finish(port);
+ port->upcoming_state = SOFT_RESET_SEND;
+ ret = tcpm_ams_start(port, SOFT_RESET_AMS);
+ if (ret == -EOPNOTSUPP)
+ tcpm_set_state(port, SOFT_RESET_SEND, 0);
break;
case SOFT_RESET_SEND:
port->message_id = 0;
@@ -3533,6 +3891,19 @@ static void run_state_machine(struct tcpm_port *port)
tcpm_default_state(port),
port->vbus_present ? PD_T_PS_SOURCE_OFF : 0);
break;
+
+ /* Collision Avoidance state */
+ case AMS_START:
+ if (port->upcoming_state == INVALID_STATE) {
+ tcpm_set_state(port, port->pwr_role == TYPEC_SOURCE ?
+ SRC_READY : SNK_READY, 0);
+ break;
+ }
+
+ upcoming_state = port->upcoming_state;
+ port->upcoming_state = INVALID_STATE;
+ tcpm_set_state(port, upcoming_state, 0);
+ break;
default:
WARN(1, "Unexpected port state %d\n", port->state);
break;
@@ -3859,6 +4230,8 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
static void _tcpm_pd_hard_reset(struct tcpm_port *port)
{
tcpm_log_force(port, "Received hard reset");
+ if (port->ams != NONE_AMS)
+ port->ams = NONE_AMS;
/*
* If we keep receiving hard reset requests, executing the hard reset
* must have failed. Revert to error recovery if that happens.
@@ -3976,7 +4349,15 @@ static int tcpm_dr_set(const struct typec_capability *cap,
port->non_pd_role_swap = true;
tcpm_set_state(port, PORT_RESET, 0);
} else {
- tcpm_set_state(port, DR_SWAP_SEND, 0);
+ port->upcoming_state = DR_SWAP_SEND;
+ ret = tcpm_ams_start(port, DATA_ROLE_SWAP);
+ if (ret == -EAGAIN) {
+ port->upcoming_state = INVALID_STATE;
+ goto port_unlock;
+ } else if (ret == -EOPNOTSUPP) {
+ port->upcoming_state = INVALID_STATE;
+ tcpm_set_state(port, DR_SWAP_SEND, 0);
+ }
}

port->swap_status = 0;
@@ -4023,10 +4404,19 @@ static int tcpm_pr_set(const struct typec_capability *cap,
goto port_unlock;
}

+ port->upcoming_state = PR_SWAP_SEND;
+ ret = tcpm_ams_start(port, POWER_ROLE_SWAP);
+ if (ret == -EAGAIN) {
+ port->upcoming_state = INVALID_STATE;
+ goto port_unlock;
+ } else if (ret == -EOPNOTSUPP) {
+ port->upcoming_state = INVALID_STATE;
+ tcpm_set_state(port, PR_SWAP_SEND, 0);
+ }
+
port->swap_status = 0;
port->swap_pending = true;
reinit_completion(&port->swap_complete);
- tcpm_set_state(port, PR_SWAP_SEND, 0);
mutex_unlock(&port->lock);

if (!wait_for_completion_timeout(&port->swap_complete,
@@ -4063,10 +4453,19 @@ static int tcpm_vconn_set(const struct typec_capability *cap,
goto port_unlock;
}

+ port->upcoming_state = VCONN_SWAP_SEND;
+ ret = tcpm_ams_start(port, VCONN_SWAP);
+ if (ret == -EAGAIN) {
+ port->upcoming_state = INVALID_STATE;
+ goto port_unlock;
+ } else if (ret == -EOPNOTSUPP) {
+ port->upcoming_state = INVALID_STATE;
+ tcpm_set_state(port, VCONN_SWAP_SEND, 0);
+ }
+
port->swap_status = 0;
port->swap_pending = true;
reinit_completion(&port->swap_complete);
- tcpm_set_state(port, VCONN_SWAP_SEND, 0);
mutex_unlock(&port->lock);

if (!wait_for_completion_timeout(&port->swap_complete,
@@ -4131,6 +4530,13 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
goto port_unlock;
}

+ port->upcoming_state = SNK_NEGOTIATE_PPS_CAPABILITIES;
+ ret = tcpm_ams_start(port, POWER_NEGOTIATION);
+ if (ret == -EAGAIN || ret == -EOPNOTSUPP) {
+ port->upcoming_state = INVALID_STATE;
+ goto port_unlock;
+ }
+
/* Round down operating current to align with PPS valid steps */
op_curr = op_curr - (op_curr % RDO_PROG_CURR_MA_STEP);

@@ -4138,7 +4544,6 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
port->pps_data.op_curr = op_curr;
port->pps_status = 0;
port->pps_pending = true;
- tcpm_set_state(port, SNK_NEGOTIATE_PPS_CAPABILITIES, 0);
mutex_unlock(&port->lock);

if (!wait_for_completion_timeout(&port->pps_complete,
@@ -4187,6 +4592,13 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
goto port_unlock;
}

+ port->upcoming_state = SNK_NEGOTIATE_PPS_CAPABILITIES;
+ ret = tcpm_ams_start(port, POWER_NEGOTIATION);
+ if (ret == -EAGAIN || ret == -EOPNOTSUPP) {
+ port->upcoming_state = INVALID_STATE;
+ goto port_unlock;
+ }
+
/* Round down output voltage to align with PPS valid steps */
out_volt = out_volt - (out_volt % RDO_PROG_VOLT_MV_STEP);

@@ -4194,7 +4606,6 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
port->pps_data.out_volt = out_volt;
port->pps_status = 0;
port->pps_pending = true;
- tcpm_set_state(port, SNK_NEGOTIATE_PPS_CAPABILITIES, 0);
mutex_unlock(&port->lock);

if (!wait_for_completion_timeout(&port->pps_complete,
@@ -4234,6 +4645,16 @@ static int tcpm_pps_activate(struct tcpm_port *port, bool activate)
goto port_unlock;
}

+ if (activate)
+ port->upcoming_state = SNK_NEGOTIATE_PPS_CAPABILITIES;
+ else
+ port->upcoming_state = SNK_NEGOTIATE_CAPABILITIES;
+ ret = tcpm_ams_start(port, POWER_NEGOTIATION);
+ if (ret == -EAGAIN || ret == -EOPNOTSUPP) {
+ port->upcoming_state = INVALID_STATE;
+ goto port_unlock;
+ }
+
reinit_completion(&port->pps_complete);
port->pps_status = 0;
port->pps_pending = true;
@@ -4242,9 +4663,6 @@ static int tcpm_pps_activate(struct tcpm_port *port, bool activate)
if (activate) {
port->pps_data.out_volt = port->supply_voltage;
port->pps_data.op_curr = port->current_limit;
- tcpm_set_state(port, SNK_NEGOTIATE_PPS_CAPABILITIES, 0);
- } else {
- tcpm_set_state(port, SNK_NEGOTIATE_CAPABILITIES, 0);
}
mutex_unlock(&port->lock);

diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
index f2162e0fe531..6296ef65dab6 100644
--- a/include/linux/usb/pd.h
+++ b/include/linux/usb/pd.h
@@ -451,6 +451,7 @@ static inline unsigned int rdo_max_power(u32 rdo)
#define PD_T_ERROR_RECOVERY 100 /* minimum 25 is insufficient */
#define PD_T_SRCSWAPSTDBY 625 /* Maximum of 650ms */
#define PD_T_NEWSRC 250 /* Maximum of 275ms */
+#define PD_T_SINK_TX 16 /* 16 - 20 ms */

#define PD_T_DRP_TRY 100 /* 75 - 150 ms */
#define PD_T_DRP_TRYWAIT 600 /* 400 - 800 ms */
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 0c532ca3f079..419929a10549 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -28,6 +28,10 @@ enum typec_cc_status {
TYPEC_CC_RP_3_0,
};

+/* Collision Avoidance */
+#define SINK_TX_NG TYPEC_CC_RP_1_5
+#define SINK_TX_OK TYPEC_CC_RP_3_0
+
enum typec_cc_polarity {
TYPEC_POLARITY_CC1,
TYPEC_POLARITY_CC2,
--
2.21.0.392.gf8f6787159e-goog

2019-04-04 14:15:02

by Heikki Krogerus

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

Hi,

On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
> This patch provides the implementation of Collision Avoidance introduced
> in PD3.0. The start of each Atomic Message Sequence (AMS) initiated by
> the port will be denied if the current AMS is not interruptible. The
> Source port will set the CC to SinkTxNG if it is going to initiate an
> AMS, and SinkTxOk otherwise. Meanwhile, any AMS initiated by a Sink port
> will be denied in TCPM if the port partner (Source) sets SinkTxNG except
> for HARD_RESET and SOFT_RESET.

I tested this with my GDBWin which has fusb302. When I plug-in
DisplayPort adapter, the partner device never gets registered, and I
see steady flow of warnings from fusb302:

[ 693.391176] Vconn is on during toggle start
[ 693.391250] WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562 fusb302_set_toggling+0x129/0x130 [fusb302]
[ 693.400293] Modules linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532 i915 typec intel_gtt intel_cht_int33fe
[ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W 5.1.0-rc3-heikki+ #17
[ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not detect charger type
[ 693.412278] Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017
[ 693.412283] Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm]
[ 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 [fusb302]
[ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6 c6 83 b0 01 00 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0 e8 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54 41 89 f4 55 53 48 8d
[ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS: 00010286
[ 693.439174] RAX: 0000000000000000 RBX: ffff888178080028 RCX: 0000000000000000
[ 693.442157] RDX: 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f
[ 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09: 00000000000202c0
[ 693.448100] R10: 0000010cb24a3d18 R11: 000000000000001e R12: ffff8881780801b0
[ 693.451086] R13: ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040
[ 693.454060] FS: 0000000000000000(0000) GS:ffff88817bb00000(0000) knlGS:0000000000000000
[ 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 693.462984] CR2: 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0
[ 693.465969] Call Trace:
[ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302]
[ 693.471894] tcpm_ams_start+0x1b8/0x2a0 [tcpm]
[ 693.474855] ? _cond_resched+0x10/0x20
[ 693.477807] tcpm_state_machine_work+0x57e/0x28f6 [tcpm]
[ 693.480776] ? tcpm_pd_event_handler+0x111/0x320 [tcpm]
[ 693.483743] process_one_work+0x1da/0x410
[ 693.486703] worker_thread+0x28/0x3c0
[ 693.489651] ? process_one_work+0x410/0x410
[ 693.492603] kthread+0x10b/0x130
[ 693.495548] ? kthread_create_on_node+0x60/0x60
[ 693.498506] ret_from_fork+0x1f/0x30


Here's the tcpm debugfs log:

[ 692.553000] CC1: 0 -> 2, CC2: 0 -> 1 [state DRP_TOGGLING, polarity 0, connected]
[ 692.553016] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
[ 692.553056] pending state change SRC_ATTACH_WAIT -> SNK_TRY @ 200 ms
[ 692.757402] state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
[ 692.757410] cc:=2
[ 692.761946] pending state change SNK_TRY -> SNK_TRY_WAIT @ 100 ms
[ 692.869308] state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
[ 692.869313] state change SNK_TRY_WAIT -> SRC_TRYWAIT
[ 692.869317] cc:=3
[ 692.873657] pending state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED @ 100 ms
[ 692.882524] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_TRYWAIT, polarity 0, connected]
[ 692.882537] state change SRC_TRYWAIT -> SRC_TRYWAIT_DEBOUNCE
[ 692.882567] pending state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED @ 200 ms
[ 693.085337] state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED [delayed 200 ms]
[ 693.085347] polarity 0
[ 693.085352] Requesting mux state 1, usb-role 1, orientation 1
[ 693.346845] vconn:=1
[ 693.347174] vbus:=1 charge=0
[ 693.358340] pending state change SRC_ATTACHED -> SRC_UNATTACHED @ 480 ms
[ 693.378702] VBUS on
[ 693.378711] state change SRC_ATTACHED -> SRC_STARTUP
[ 693.378741] AMS POWER_NEGOTIATION start
[ 693.378745] cc:=4
[ 693.505321] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
[ 693.505325] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
[ 693.505327] PD TX, header: 0x11a1
[ 693.613296] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
[ 693.613309] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
[ 693.765730] state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES [delayed 150 ms]
[ 693.765753] PD TX, header: 0x11a1
[ 693.770016] PD TX complete, status: 0
[ 693.770261] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES_TIMEOUT @ 150 ms in AMS POWER_NEGOTIATION
[ 693.775178] PD RX, header: 0x1042 [1]
[ 693.775195] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
[ 693.775236] Requested 5000 mV, 400 mA for 400 / 900 mA
[ 693.775244] PD TX, header: 0x363
[ 693.778253] PD TX complete, status: 0
[ 693.778363] pending state change SRC_NEGOTIATE_CAPABILITIES -> SRC_TRANSITION_SUPPLY @ 35 ms
[ 693.803463] Received hard reset
[ 693.803473] state change SRC_NEGOTIATE_CAPABILITIES -> HARD_RESET_START
[ 693.806323] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms
[ 693.837400] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
[ 693.837414] vconn:=1
[ 693.837426] vbus:=0 charge=0
[ 693.843380] Requesting mux state 1, usb-role 1, orientation 1
[ 693.844554] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms
[ 693.844575] VBUS off
[ 693.844580] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON
[ 693.844617] vbus:=1 charge=0
[ 693.850688] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms
[ 693.868706] VBUS on
[ 693.868713] state change SRC_HARD_RESET_VBUS_ON -> SRC_STARTUP
[ 693.868742] AMS POWER_NEGOTIATION start
[ 693.868749] cc:=4
[ 694.101422] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
[ 694.101425] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
[ 694.101428] PD TX, header: 0x11a1
[ 694.205301] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
[ 694.205317] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
[ 694.325808] Received hard reset
[ 694.325816] state change SRC_SEND_CAPABILITIES -> HARD_RESET_START in AMS NONE_AMS
[ 694.329208] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms in AMS NONE_AMS
[ 694.359394] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
[ 694.359401] vconn:=1
[ 694.359408] vbus:=0 charge=0
[ 694.366321] Requesting mux state 1, usb-role 1, orientation 1
[ 694.367685] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms in AMS NONE_AMS
[ 694.367700] VBUS off
[ 694.367704] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON in AMS NONE_AMS
[ 694.367721] vbus:=1 charge=0
[ 694.374175] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms in AMS NONE_AMS
[ 694.374194] CC1: 2 -> 0, CC2: 1 -> 0 [state SRC_HARD_RESET_VBUS_ON, polarity 0, disconnected]
[ 694.374201] state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED in AMS NONE_AMS
[ 694.631957] Start DRP toggling


thanks,

--
heikki

2019-04-05 13:43:10

by Guenter Roeck

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

On 4/4/19 7:13 AM, Heikki Krogerus wrote:
> Hi,
>
> On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
>> This patch provides the implementation of Collision Avoidance introduced
>> in PD3.0. The start of each Atomic Message Sequence (AMS) initiated by
>> the port will be denied if the current AMS is not interruptible. The
>> Source port will set the CC to SinkTxNG if it is going to initiate an
>> AMS, and SinkTxOk otherwise. Meanwhile, any AMS initiated by a Sink port
>> will be denied in TCPM if the port partner (Source) sets SinkTxNG except
>> for HARD_RESET and SOFT_RESET.
>
> I tested this with my GDBWin which has fusb302. When I plug-in
> DisplayPort adapter, the partner device never gets registered, and I
> see steady flow of warnings from fusb302:
>

FWIW, I made multiple attempts to review the patch. Each time I get stuck
after a while and notice that I don't understand what is going on.

Maybe the state machine needs a complete overhaul. It seems to have reached
a point where it is getting too complex to understand what is going on.

> [ 693.391176] Vconn is on during toggle start
> [ 693.391250] WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562 fusb302_set_toggling+0x129/0x130 [fusb302]
> [ 693.400293] Modules linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532 i915 typec intel_gtt intel_cht_int33fe
> [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W 5.1.0-rc3-heikki+ #17
> [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not detect charger type
> [ 693.412278] Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017
> [ 693.412283] Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm]
> [ 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 [fusb302]
> [ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6 c6 83 b0 01 00 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0 e8 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54 41 89 f4 55 53 48 8d
> [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS: 00010286
> [ 693.439174] RAX: 0000000000000000 RBX: ffff888178080028 RCX: 0000000000000000
> [ 693.442157] RDX: 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f
> [ 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09: 00000000000202c0
> [ 693.448100] R10: 0000010cb24a3d18 R11: 000000000000001e R12: ffff8881780801b0
> [ 693.451086] R13: ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040
> [ 693.454060] FS: 0000000000000000(0000) GS:ffff88817bb00000(0000) knlGS:0000000000000000
> [ 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 693.462984] CR2: 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0
> [ 693.465969] Call Trace:
> [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302]
> [ 693.471894] tcpm_ams_start+0x1b8/0x2a0 [tcpm]

tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter what. This causes
the fusb302 code to start toggling. As such, it may well attempt to start toggling
in the wrong state.

Guenter

> [ 693.474855] ? _cond_resched+0x10/0x20
> [ 693.477807] tcpm_state_machine_work+0x57e/0x28f6 [tcpm]
> [ 693.480776] ? tcpm_pd_event_handler+0x111/0x320 [tcpm]
> [ 693.483743] process_one_work+0x1da/0x410
> [ 693.486703] worker_thread+0x28/0x3c0
> [ 693.489651] ? process_one_work+0x410/0x410
> [ 693.492603] kthread+0x10b/0x130
> [ 693.495548] ? kthread_create_on_node+0x60/0x60
> [ 693.498506] ret_from_fork+0x1f/0x30
>
>
> Here's the tcpm debugfs log:
>
> [ 692.553000] CC1: 0 -> 2, CC2: 0 -> 1 [state DRP_TOGGLING, polarity 0, connected]
> [ 692.553016] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
> [ 692.553056] pending state change SRC_ATTACH_WAIT -> SNK_TRY @ 200 ms
> [ 692.757402] state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
> [ 692.757410] cc:=2
> [ 692.761946] pending state change SNK_TRY -> SNK_TRY_WAIT @ 100 ms
> [ 692.869308] state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
> [ 692.869313] state change SNK_TRY_WAIT -> SRC_TRYWAIT
> [ 692.869317] cc:=3
> [ 692.873657] pending state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED @ 100 ms
> [ 692.882524] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_TRYWAIT, polarity 0, connected]
> [ 692.882537] state change SRC_TRYWAIT -> SRC_TRYWAIT_DEBOUNCE
> [ 692.882567] pending state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED @ 200 ms
> [ 693.085337] state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED [delayed 200 ms]
> [ 693.085347] polarity 0
> [ 693.085352] Requesting mux state 1, usb-role 1, orientation 1
> [ 693.346845] vconn:=1
> [ 693.347174] vbus:=1 charge=0
> [ 693.358340] pending state change SRC_ATTACHED -> SRC_UNATTACHED @ 480 ms
> [ 693.378702] VBUS on
> [ 693.378711] state change SRC_ATTACHED -> SRC_STARTUP
> [ 693.378741] AMS POWER_NEGOTIATION start
> [ 693.378745] cc:=4
> [ 693.505321] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> [ 693.505325] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
> [ 693.505327] PD TX, header: 0x11a1
> [ 693.613296] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
> [ 693.613309] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
> [ 693.765730] state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES [delayed 150 ms]
> [ 693.765753] PD TX, header: 0x11a1
> [ 693.770016] PD TX complete, status: 0
> [ 693.770261] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES_TIMEOUT @ 150 ms in AMS POWER_NEGOTIATION
> [ 693.775178] PD RX, header: 0x1042 [1]
> [ 693.775195] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
> [ 693.775236] Requested 5000 mV, 400 mA for 400 / 900 mA
> [ 693.775244] PD TX, header: 0x363
> [ 693.778253] PD TX complete, status: 0
> [ 693.778363] pending state change SRC_NEGOTIATE_CAPABILITIES -> SRC_TRANSITION_SUPPLY @ 35 ms
> [ 693.803463] Received hard reset
> [ 693.803473] state change SRC_NEGOTIATE_CAPABILITIES -> HARD_RESET_START
> [ 693.806323] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms
> [ 693.837400] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
> [ 693.837414] vconn:=1
> [ 693.837426] vbus:=0 charge=0
> [ 693.843380] Requesting mux state 1, usb-role 1, orientation 1
> [ 693.844554] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms
> [ 693.844575] VBUS off
> [ 693.844580] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON
> [ 693.844617] vbus:=1 charge=0
> [ 693.850688] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms
> [ 693.868706] VBUS on
> [ 693.868713] state change SRC_HARD_RESET_VBUS_ON -> SRC_STARTUP
> [ 693.868742] AMS POWER_NEGOTIATION start
> [ 693.868749] cc:=4
> [ 694.101422] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> [ 694.101425] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
> [ 694.101428] PD TX, header: 0x11a1
> [ 694.205301] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
> [ 694.205317] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
> [ 694.325808] Received hard reset
> [ 694.325816] state change SRC_SEND_CAPABILITIES -> HARD_RESET_START in AMS NONE_AMS
> [ 694.329208] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms in AMS NONE_AMS
> [ 694.359394] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
> [ 694.359401] vconn:=1
> [ 694.359408] vbus:=0 charge=0
> [ 694.366321] Requesting mux state 1, usb-role 1, orientation 1
> [ 694.367685] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms in AMS NONE_AMS
> [ 694.367700] VBUS off
> [ 694.367704] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON in AMS NONE_AMS
> [ 694.367721] vbus:=1 charge=0
> [ 694.374175] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms in AMS NONE_AMS
> [ 694.374194] CC1: 2 -> 0, CC2: 1 -> 0 [state SRC_HARD_RESET_VBUS_ON, polarity 0, disconnected]
> [ 694.374201] state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED in AMS NONE_AMS
> [ 694.631957] Start DRP toggling
>
>
> thanks,
>

2019-04-08 14:18:55

by Kyle Tso

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck <[email protected]> wrote:
>
> On 4/4/19 7:13 AM, Heikki Krogerus wrote:
> > Hi,
> >
> > On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
> >> This patch provides the implementation of Collision Avoidance introduced
> >> in PD3.0. The start of each Atomic Message Sequence (AMS) initiated by
> >> the port will be denied if the current AMS is not interruptible. The
> >> Source port will set the CC to SinkTxNG if it is going to initiate an
> >> AMS, and SinkTxOk otherwise. Meanwhile, any AMS initiated by a Sink port
> >> will be denied in TCPM if the port partner (Source) sets SinkTxNG except
> >> for HARD_RESET and SOFT_RESET.
> >
> > I tested this with my GDBWin which has fusb302. When I plug-in
> > DisplayPort adapter, the partner device never gets registered, and I
> > see steady flow of warnings from fusb302:
> >
>
> FWIW, I made multiple attempts to review the patch. Each time I get stuck
> after a while and notice that I don't understand what is going on.
>
> Maybe the state machine needs a complete overhaul. It seems to have reached
> a point where it is getting too complex to understand what is going on.
>
> > [ 693.391176] Vconn is on during toggle start
> > [ 693.391250] WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562 fusb302_set_toggling+0x129/0x130 [fusb302]
> > [ 693.400293] Modules linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532 i915 typec intel_gtt intel_cht_int33fe
> > [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W 5.1.0-rc3-heikki+ #17
> > [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not detect charger type
> > [ 693.412278] Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017
> > [ 693.412283] Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm]
> > [ 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 [fusb302]
> > [ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6 c6 83 b0 01 00 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0 e8 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54 41 89 f4 55 53 48 8d
> > [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS: 00010286
> > [ 693.439174] RAX: 0000000000000000 RBX: ffff888178080028 RCX: 0000000000000000
> > [ 693.442157] RDX: 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f
> > [ 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09: 00000000000202c0
> > [ 693.448100] R10: 0000010cb24a3d18 R11: 000000000000001e R12: ffff8881780801b0
> > [ 693.451086] R13: ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040
> > [ 693.454060] FS: 0000000000000000(0000) GS:ffff88817bb00000(0000) knlGS:0000000000000000
> > [ 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 693.462984] CR2: 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0
> > [ 693.465969] Call Trace:
> > [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302]
> > [ 693.471894] tcpm_ams_start+0x1b8/0x2a0 [tcpm]
>
> tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter what. This causes
> the fusb302 code to start toggling. As such, it may well attempt to start toggling
> in the wrong state.
>
> Guenter
>

I read the fusb302 spec but failed to find the statement that says it
should "set toggling" when CC switches among default/medium/high.

quot from fusb302 spec:
"The FUSB302 allows the host software to change the charging current
capabilities of the port through the HOST_CUR control bits. If the
HOST_CUR bits are changed prior to attach, the FUSB302 automatically
indicates the programmed current capability when a device is attached.
If the current capabilities are changed after a device is attached,
the FUSB302 immediately changes the CC line to the programmed
capability."

Is it possible to skip fusb302_set_toggling() @ line#658 if
tcpm_set_cc() is called in order to switch the cc among
default/medium/high of Rp ?

the tcpm log on another PD controller:
[ 1215.709538] CC1: 0 -> 2, CC2: 0 -> 1 [state DRP_TOGGLING, polarity
0, connected]
[ 1215.709551] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
[ 1215.709576] pending state change SRC_ATTACH_WAIT -> SRC_ATTACHED @
200 ms
[ 1215.907465] state change SRC_ATTACH_WAIT -> SRC_ATTACHED [delayed
200 ms]
[ 1215.907670] polarity 0
[ 1215.908070] Requesting mux state 1, usb-role 1, orientation 1
[ 1215.909342] vconn:=1
[ 1215.909549] vbus:=1 charge=0
[ 1215.909633] Setting pd capable false
[ 1215.909637] pending state change SRC_ATTACHED -> SRC_UNATTACHED @
480 ms
[ 1215.909685] VBUS on
[ 1215.909687] state change SRC_ATTACHED -> SRC_STARTUP
[ 1215.914263] AMS POWER_NEGOTIATION start
[ 1215.914267] cc:=4
[ 1215.914288] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
[ 1215.914294] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS
POWER_NEGOTIATION
[ 1215.914298] PD TX, header: 0x11a1
[ 1215.919552] PD TX complete, status: 2
[ 1215.919577] pending state change SRC_SEND_CAPABILITIES ->
SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
[ 1216.071041] state change SRC_SEND_CAPABILITIES ->
SRC_SEND_CAPABILITIES [delayed 150 ms]
[ 1216.071049] PD TX, header: 0x11a1
[ 1216.072508] PD TX complete, status: 0
[ 1216.072535] Setting pd capable true
[ 1216.072645] pending state change SRC_SEND_CAPABILITIES ->
HARD_RESET_SEND @ 150 ms in AMS POWER_NEGOTIATION
[ 1216.073915] PD RX, header: 0x1042 [1]
[ 1216.073920] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
[ 1216.073941] Requested 5000 mV, 900 mA for 900 / 900 mA
[ 1216.073944] PD TX, header: 0x363
[ 1216.075281] PD TX complete, status: 0
[ 1216.075337] pending state change SRC_NEGOTIATE_CAPABILITIES ->
SRC_TRANSITION_SUPPLY @ 35 ms
[ 1216.113742] state change SRC_NEGOTIATE_CAPABILITIES ->
SRC_TRANSITION_SUPPLY [delayed 35 ms]
[ 1216.113752] PD TX, header: 0x566
[ 1216.116743] PD TX complete, status: 0
[ 1216.117175] state change SRC_TRANSITION_SUPPLY -> SRC_READY
[ 1216.118737] PD TX, header: 0x176f
[ 1216.120285] PD TX complete, status: 0
[ 1216.122455] PD RX, header: 0x524f [1]
[ 1216.122463] Rx VDM cmd 0xff008041 type 1 cmd 1 len 5
[ 1216.122511] Identity: 04b4:1120.0000
[ 1216.122531] PD TX, header: 0x196f
[ 1216.123965] PD TX complete, status: 0
[ 1216.125573] PD RX, header: 0x244f [1]
[ 1216.125578] Rx VDM cmd 0xff008042 type 1 cmd 2 len 2
[ 1216.125581] SVID 1: 0xff01
[ 1216.125603] PD TX, header: 0x1b6f
[ 1216.127074] PD TX complete, status: 0
[ 1216.128750] PD RX, header: 0x264f [1]
[ 1216.128754] Rx VDM cmd 0xff018043 type 1 cmd 3 len 2
[ 1216.128758] Alternate mode 0: SVID 0xff01, VDO 1: 0x00000405

> > [ 693.474855] ? _cond_resched+0x10/0x20
> > [ 693.477807] tcpm_state_machine_work+0x57e/0x28f6 [tcpm]
> > [ 693.480776] ? tcpm_pd_event_handler+0x111/0x320 [tcpm]
> > [ 693.483743] process_one_work+0x1da/0x410
> > [ 693.486703] worker_thread+0x28/0x3c0
> > [ 693.489651] ? process_one_work+0x410/0x410
> > [ 693.492603] kthread+0x10b/0x130
> > [ 693.495548] ? kthread_create_on_node+0x60/0x60
> > [ 693.498506] ret_from_fork+0x1f/0x30
> >
> >
> > Here's the tcpm debugfs log:
> >
> > [ 692.553000] CC1: 0 -> 2, CC2: 0 -> 1 [state DRP_TOGGLING, polarity 0, connected]
> > [ 692.553016] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
> > [ 692.553056] pending state change SRC_ATTACH_WAIT -> SNK_TRY @ 200 ms
> > [ 692.757402] state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
> > [ 692.757410] cc:=2
> > [ 692.761946] pending state change SNK_TRY -> SNK_TRY_WAIT @ 100 ms
> > [ 692.869308] state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
> > [ 692.869313] state change SNK_TRY_WAIT -> SRC_TRYWAIT
> > [ 692.869317] cc:=3
> > [ 692.873657] pending state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED @ 100 ms
> > [ 692.882524] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_TRYWAIT, polarity 0, connected]
> > [ 692.882537] state change SRC_TRYWAIT -> SRC_TRYWAIT_DEBOUNCE
> > [ 692.882567] pending state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED @ 200 ms
> > [ 693.085337] state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED [delayed 200 ms]
> > [ 693.085347] polarity 0
> > [ 693.085352] Requesting mux state 1, usb-role 1, orientation 1
> > [ 693.346845] vconn:=1
> > [ 693.347174] vbus:=1 charge=0
> > [ 693.358340] pending state change SRC_ATTACHED -> SRC_UNATTACHED @ 480 ms
> > [ 693.378702] VBUS on
> > [ 693.378711] state change SRC_ATTACHED -> SRC_STARTUP
> > [ 693.378741] AMS POWER_NEGOTIATION start
> > [ 693.378745] cc:=4
> > [ 693.505321] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> > [ 693.505325] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
> > [ 693.505327] PD TX, header: 0x11a1
> > [ 693.613296] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
> > [ 693.613309] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
> > [ 693.765730] state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES [delayed 150 ms]
> > [ 693.765753] PD TX, header: 0x11a1
> > [ 693.770016] PD TX complete, status: 0
> > [ 693.770261] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES_TIMEOUT @ 150 ms in AMS POWER_NEGOTIATION
> > [ 693.775178] PD RX, header: 0x1042 [1]
> > [ 693.775195] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
> > [ 693.775236] Requested 5000 mV, 400 mA for 400 / 900 mA
> > [ 693.775244] PD TX, header: 0x363
> > [ 693.778253] PD TX complete, status: 0
> > [ 693.778363] pending state change SRC_NEGOTIATE_CAPABILITIES -> SRC_TRANSITION_SUPPLY @ 35 ms
> > [ 693.803463] Received hard reset
> > [ 693.803473] state change SRC_NEGOTIATE_CAPABILITIES -> HARD_RESET_START
> > [ 693.806323] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms
> > [ 693.837400] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
> > [ 693.837414] vconn:=1
> > [ 693.837426] vbus:=0 charge=0
> > [ 693.843380] Requesting mux state 1, usb-role 1, orientation 1
> > [ 693.844554] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms
> > [ 693.844575] VBUS off
> > [ 693.844580] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON
> > [ 693.844617] vbus:=1 charge=0
> > [ 693.850688] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms
> > [ 693.868706] VBUS on
> > [ 693.868713] state change SRC_HARD_RESET_VBUS_ON -> SRC_STARTUP
> > [ 693.868742] AMS POWER_NEGOTIATION start
> > [ 693.868749] cc:=4
> > [ 694.101422] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> > [ 694.101425] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
> > [ 694.101428] PD TX, header: 0x11a1
> > [ 694.205301] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
> > [ 694.205317] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
> > [ 694.325808] Received hard reset
> > [ 694.325816] state change SRC_SEND_CAPABILITIES -> HARD_RESET_START in AMS NONE_AMS
> > [ 694.329208] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms in AMS NONE_AMS
> > [ 694.359394] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
> > [ 694.359401] vconn:=1
> > [ 694.359408] vbus:=0 charge=0
> > [ 694.366321] Requesting mux state 1, usb-role 1, orientation 1
> > [ 694.367685] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms in AMS NONE_AMS
> > [ 694.367700] VBUS off
> > [ 694.367704] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON in AMS NONE_AMS
> > [ 694.367721] vbus:=1 charge=0
> > [ 694.374175] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms in AMS NONE_AMS
> > [ 694.374194] CC1: 2 -> 0, CC2: 1 -> 0 [state SRC_HARD_RESET_VBUS_ON, polarity 0, disconnected]
> > [ 694.374201] state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED in AMS NONE_AMS
> > [ 694.631957] Start DRP toggling
> >
> >
> > thanks,
> >
>

2019-04-09 13:05:02

by Heikki Krogerus

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

+Hans

On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
> On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck <[email protected]> wrote:
> >
> > On 4/4/19 7:13 AM, Heikki Krogerus wrote:
> > > Hi,
> > >
> > > On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
> > >> This patch provides the implementation of Collision Avoidance introduced
> > >> in PD3.0. The start of each Atomic Message Sequence (AMS) initiated by
> > >> the port will be denied if the current AMS is not interruptible. The
> > >> Source port will set the CC to SinkTxNG if it is going to initiate an
> > >> AMS, and SinkTxOk otherwise. Meanwhile, any AMS initiated by a Sink port
> > >> will be denied in TCPM if the port partner (Source) sets SinkTxNG except
> > >> for HARD_RESET and SOFT_RESET.
> > >
> > > I tested this with my GDBWin which has fusb302. When I plug-in
> > > DisplayPort adapter, the partner device never gets registered, and I
> > > see steady flow of warnings from fusb302:
> > >
> >
> > FWIW, I made multiple attempts to review the patch. Each time I get stuck
> > after a while and notice that I don't understand what is going on.
> >
> > Maybe the state machine needs a complete overhaul. It seems to have reached
> > a point where it is getting too complex to understand what is going on.
> >
> > > [ 693.391176] Vconn is on during toggle start
> > > [ 693.391250] WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562 fusb302_set_toggling+0x129/0x130 [fusb302]
> > > [ 693.400293] Modules linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532 i915 typec intel_gtt intel_cht_int33fe
> > > [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W 5.1.0-rc3-heikki+ #17
> > > [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not detect charger type
> > > [ 693.412278] Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017
> > > [ 693.412283] Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm]
> > > [ 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 [fusb302]
> > > [ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6 c6 83 b0 01 00 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0 e8 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54 41 89 f4 55 53 48 8d
> > > [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS: 00010286
> > > [ 693.439174] RAX: 0000000000000000 RBX: ffff888178080028 RCX: 0000000000000000
> > > [ 693.442157] RDX: 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f
> > > [ 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09: 00000000000202c0
> > > [ 693.448100] R10: 0000010cb24a3d18 R11: 000000000000001e R12: ffff8881780801b0
> > > [ 693.451086] R13: ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040
> > > [ 693.454060] FS: 0000000000000000(0000) GS:ffff88817bb00000(0000) knlGS:0000000000000000
> > > [ 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [ 693.462984] CR2: 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0
> > > [ 693.465969] Call Trace:
> > > [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302]
> > > [ 693.471894] tcpm_ams_start+0x1b8/0x2a0 [tcpm]
> >
> > tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter what. This causes
> > the fusb302 code to start toggling. As such, it may well attempt to start toggling
> > in the wrong state.
> >
> > Guenter
> >
>
> I read the fusb302 spec but failed to find the statement that says it
> should "set toggling" when CC switches among default/medium/high.
>
> quot from fusb302 spec:
> "The FUSB302 allows the host software to change the charging current
> capabilities of the port through the HOST_CUR control bits. If the
> HOST_CUR bits are changed prior to attach, the FUSB302 automatically
> indicates the programmed current capability when a device is attached.
> If the current capabilities are changed after a device is attached,
> the FUSB302 immediately changes the CC line to the programmed
> capability."
>
> Is it possible to skip fusb302_set_toggling() @ line#658 if
> tcpm_set_cc() is called in order to switch the cc among
> default/medium/high of Rp ?
>
> the tcpm log on another PD controller:
> [ 1215.709538] CC1: 0 -> 2, CC2: 0 -> 1 [state DRP_TOGGLING, polarity
> 0, connected]
> [ 1215.709551] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
> [ 1215.709576] pending state change SRC_ATTACH_WAIT -> SRC_ATTACHED @
> 200 ms
> [ 1215.907465] state change SRC_ATTACH_WAIT -> SRC_ATTACHED [delayed
> 200 ms]
> [ 1215.907670] polarity 0
> [ 1215.908070] Requesting mux state 1, usb-role 1, orientation 1
> [ 1215.909342] vconn:=1
> [ 1215.909549] vbus:=1 charge=0
> [ 1215.909633] Setting pd capable false
> [ 1215.909637] pending state change SRC_ATTACHED -> SRC_UNATTACHED @
> 480 ms
> [ 1215.909685] VBUS on
> [ 1215.909687] state change SRC_ATTACHED -> SRC_STARTUP
> [ 1215.914263] AMS POWER_NEGOTIATION start
> [ 1215.914267] cc:=4
> [ 1215.914288] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> [ 1215.914294] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS
> POWER_NEGOTIATION
> [ 1215.914298] PD TX, header: 0x11a1
> [ 1215.919552] PD TX complete, status: 2
> [ 1215.919577] pending state change SRC_SEND_CAPABILITIES ->
> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
> [ 1216.071041] state change SRC_SEND_CAPABILITIES ->
> SRC_SEND_CAPABILITIES [delayed 150 ms]
> [ 1216.071049] PD TX, header: 0x11a1
> [ 1216.072508] PD TX complete, status: 0
> [ 1216.072535] Setting pd capable true
> [ 1216.072645] pending state change SRC_SEND_CAPABILITIES ->
> HARD_RESET_SEND @ 150 ms in AMS POWER_NEGOTIATION
> [ 1216.073915] PD RX, header: 0x1042 [1]
> [ 1216.073920] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
> [ 1216.073941] Requested 5000 mV, 900 mA for 900 / 900 mA
> [ 1216.073944] PD TX, header: 0x363
> [ 1216.075281] PD TX complete, status: 0
> [ 1216.075337] pending state change SRC_NEGOTIATE_CAPABILITIES ->
> SRC_TRANSITION_SUPPLY @ 35 ms
> [ 1216.113742] state change SRC_NEGOTIATE_CAPABILITIES ->
> SRC_TRANSITION_SUPPLY [delayed 35 ms]
> [ 1216.113752] PD TX, header: 0x566
> [ 1216.116743] PD TX complete, status: 0
> [ 1216.117175] state change SRC_TRANSITION_SUPPLY -> SRC_READY
> [ 1216.118737] PD TX, header: 0x176f
> [ 1216.120285] PD TX complete, status: 0
> [ 1216.122455] PD RX, header: 0x524f [1]
> [ 1216.122463] Rx VDM cmd 0xff008041 type 1 cmd 1 len 5
> [ 1216.122511] Identity: 04b4:1120.0000
> [ 1216.122531] PD TX, header: 0x196f
> [ 1216.123965] PD TX complete, status: 0
> [ 1216.125573] PD RX, header: 0x244f [1]
> [ 1216.125578] Rx VDM cmd 0xff008042 type 1 cmd 2 len 2
> [ 1216.125581] SVID 1: 0xff01
> [ 1216.125603] PD TX, header: 0x1b6f
> [ 1216.127074] PD TX complete, status: 0
> [ 1216.128750] PD RX, header: 0x264f [1]
> [ 1216.128754] Rx VDM cmd 0xff018043 type 1 cmd 3 len 2
> [ 1216.128758] Alternate mode 0: SVID 0xff01, VDO 1: 0x00000405
>
> > > [ 693.474855] ? _cond_resched+0x10/0x20
> > > [ 693.477807] tcpm_state_machine_work+0x57e/0x28f6 [tcpm]
> > > [ 693.480776] ? tcpm_pd_event_handler+0x111/0x320 [tcpm]
> > > [ 693.483743] process_one_work+0x1da/0x410
> > > [ 693.486703] worker_thread+0x28/0x3c0
> > > [ 693.489651] ? process_one_work+0x410/0x410
> > > [ 693.492603] kthread+0x10b/0x130
> > > [ 693.495548] ? kthread_create_on_node+0x60/0x60
> > > [ 693.498506] ret_from_fork+0x1f/0x30
> > >
> > >
> > > Here's the tcpm debugfs log:
> > >
> > > [ 692.553000] CC1: 0 -> 2, CC2: 0 -> 1 [state DRP_TOGGLING, polarity 0, connected]
> > > [ 692.553016] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
> > > [ 692.553056] pending state change SRC_ATTACH_WAIT -> SNK_TRY @ 200 ms
> > > [ 692.757402] state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
> > > [ 692.757410] cc:=2
> > > [ 692.761946] pending state change SNK_TRY -> SNK_TRY_WAIT @ 100 ms
> > > [ 692.869308] state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
> > > [ 692.869313] state change SNK_TRY_WAIT -> SRC_TRYWAIT
> > > [ 692.869317] cc:=3
> > > [ 692.873657] pending state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED @ 100 ms
> > > [ 692.882524] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_TRYWAIT, polarity 0, connected]
> > > [ 692.882537] state change SRC_TRYWAIT -> SRC_TRYWAIT_DEBOUNCE
> > > [ 692.882567] pending state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED @ 200 ms
> > > [ 693.085337] state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED [delayed 200 ms]
> > > [ 693.085347] polarity 0
> > > [ 693.085352] Requesting mux state 1, usb-role 1, orientation 1
> > > [ 693.346845] vconn:=1
> > > [ 693.347174] vbus:=1 charge=0
> > > [ 693.358340] pending state change SRC_ATTACHED -> SRC_UNATTACHED @ 480 ms
> > > [ 693.378702] VBUS on
> > > [ 693.378711] state change SRC_ATTACHED -> SRC_STARTUP
> > > [ 693.378741] AMS POWER_NEGOTIATION start
> > > [ 693.378745] cc:=4
> > > [ 693.505321] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> > > [ 693.505325] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
> > > [ 693.505327] PD TX, header: 0x11a1
> > > [ 693.613296] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
> > > [ 693.613309] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
> > > [ 693.765730] state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES [delayed 150 ms]
> > > [ 693.765753] PD TX, header: 0x11a1
> > > [ 693.770016] PD TX complete, status: 0
> > > [ 693.770261] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES_TIMEOUT @ 150 ms in AMS POWER_NEGOTIATION
> > > [ 693.775178] PD RX, header: 0x1042 [1]
> > > [ 693.775195] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
> > > [ 693.775236] Requested 5000 mV, 400 mA for 400 / 900 mA
> > > [ 693.775244] PD TX, header: 0x363
> > > [ 693.778253] PD TX complete, status: 0
> > > [ 693.778363] pending state change SRC_NEGOTIATE_CAPABILITIES -> SRC_TRANSITION_SUPPLY @ 35 ms
> > > [ 693.803463] Received hard reset
> > > [ 693.803473] state change SRC_NEGOTIATE_CAPABILITIES -> HARD_RESET_START
> > > [ 693.806323] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms
> > > [ 693.837400] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
> > > [ 693.837414] vconn:=1
> > > [ 693.837426] vbus:=0 charge=0
> > > [ 693.843380] Requesting mux state 1, usb-role 1, orientation 1
> > > [ 693.844554] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms
> > > [ 693.844575] VBUS off
> > > [ 693.844580] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON
> > > [ 693.844617] vbus:=1 charge=0
> > > [ 693.850688] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms
> > > [ 693.868706] VBUS on
> > > [ 693.868713] state change SRC_HARD_RESET_VBUS_ON -> SRC_STARTUP
> > > [ 693.868742] AMS POWER_NEGOTIATION start
> > > [ 693.868749] cc:=4
> > > [ 694.101422] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> > > [ 694.101425] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
> > > [ 694.101428] PD TX, header: 0x11a1
> > > [ 694.205301] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
> > > [ 694.205317] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
> > > [ 694.325808] Received hard reset
> > > [ 694.325816] state change SRC_SEND_CAPABILITIES -> HARD_RESET_START in AMS NONE_AMS
> > > [ 694.329208] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms in AMS NONE_AMS
> > > [ 694.359394] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
> > > [ 694.359401] vconn:=1
> > > [ 694.359408] vbus:=0 charge=0
> > > [ 694.366321] Requesting mux state 1, usb-role 1, orientation 1
> > > [ 694.367685] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms in AMS NONE_AMS
> > > [ 694.367700] VBUS off
> > > [ 694.367704] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON in AMS NONE_AMS
> > > [ 694.367721] vbus:=1 charge=0
> > > [ 694.374175] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms in AMS NONE_AMS
> > > [ 694.374194] CC1: 2 -> 0, CC2: 1 -> 0 [state SRC_HARD_RESET_VBUS_ON, polarity 0, disconnected]
> > > [ 694.374201] state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED in AMS NONE_AMS
> > > [ 694.631957] Start DRP toggling

thanks,

--
heikki

2019-04-09 13:08:02

by Heikki Krogerus

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote:
> +Hans
>
> On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
> > On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck <[email protected]> wrote:
> > >
> > > On 4/4/19 7:13 AM, Heikki Krogerus wrote:
> > > > Hi,
> > > >
> > > > On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
> > > >> This patch provides the implementation of Collision Avoidance introduced
> > > >> in PD3.0. The start of each Atomic Message Sequence (AMS) initiated by
> > > >> the port will be denied if the current AMS is not interruptible. The
> > > >> Source port will set the CC to SinkTxNG if it is going to initiate an
> > > >> AMS, and SinkTxOk otherwise. Meanwhile, any AMS initiated by a Sink port
> > > >> will be denied in TCPM if the port partner (Source) sets SinkTxNG except
> > > >> for HARD_RESET and SOFT_RESET.
> > > >
> > > > I tested this with my GDBWin which has fusb302. When I plug-in
> > > > DisplayPort adapter, the partner device never gets registered, and I
> > > > see steady flow of warnings from fusb302:
> > > >
> > >
> > > FWIW, I made multiple attempts to review the patch. Each time I get stuck
> > > after a while and notice that I don't understand what is going on.
> > >
> > > Maybe the state machine needs a complete overhaul. It seems to have reached
> > > a point where it is getting too complex to understand what is going on.
> > >
> > > > [ 693.391176] Vconn is on during toggle start
> > > > [ 693.391250] WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562 fusb302_set_toggling+0x129/0x130 [fusb302]
> > > > [ 693.400293] Modules linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532 i915 typec intel_gtt intel_cht_int33fe
> > > > [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W 5.1.0-rc3-heikki+ #17
> > > > [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not detect charger type
> > > > [ 693.412278] Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017
> > > > [ 693.412283] Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm]
> > > > [ 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 [fusb302]
> > > > [ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6 c6 83 b0 01 00 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0 e8 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54 41 89 f4 55 53 48 8d
> > > > [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS: 00010286
> > > > [ 693.439174] RAX: 0000000000000000 RBX: ffff888178080028 RCX: 0000000000000000
> > > > [ 693.442157] RDX: 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f
> > > > [ 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09: 00000000000202c0
> > > > [ 693.448100] R10: 0000010cb24a3d18 R11: 000000000000001e R12: ffff8881780801b0
> > > > [ 693.451086] R13: ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040
> > > > [ 693.454060] FS: 0000000000000000(0000) GS:ffff88817bb00000(0000) knlGS:0000000000000000
> > > > [ 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > [ 693.462984] CR2: 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0
> > > > [ 693.465969] Call Trace:
> > > > [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302]
> > > > [ 693.471894] tcpm_ams_start+0x1b8/0x2a0 [tcpm]
> > >
> > > tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter what. This causes
> > > the fusb302 code to start toggling. As such, it may well attempt to start toggling
> > > in the wrong state.
> > >
> > > Guenter
> > >
> >
> > I read the fusb302 spec but failed to find the statement that says it
> > should "set toggling" when CC switches among default/medium/high.
> >
> > quot from fusb302 spec:
> > "The FUSB302 allows the host software to change the charging current
> > capabilities of the port through the HOST_CUR control bits. If the
> > HOST_CUR bits are changed prior to attach, the FUSB302 automatically
> > indicates the programmed current capability when a device is attached.
> > If the current capabilities are changed after a device is attached,
> > the FUSB302 immediately changes the CC line to the programmed
> > capability."
> >
> > Is it possible to skip fusb302_set_toggling() @ line#658 if
> > tcpm_set_cc() is called in order to switch the cc among
> > default/medium/high of Rp ?

Hans, you introduced that in commit daf81d0137a9c ("usb: typec:
fusb302: Refactor / simplify tcpm_set_cc()"), so could you take a look
at this.

> > the tcpm log on another PD controller:
> > [ 1215.709538] CC1: 0 -> 2, CC2: 0 -> 1 [state DRP_TOGGLING, polarity
> > 0, connected]
> > [ 1215.709551] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
> > [ 1215.709576] pending state change SRC_ATTACH_WAIT -> SRC_ATTACHED @
> > 200 ms
> > [ 1215.907465] state change SRC_ATTACH_WAIT -> SRC_ATTACHED [delayed
> > 200 ms]
> > [ 1215.907670] polarity 0
> > [ 1215.908070] Requesting mux state 1, usb-role 1, orientation 1
> > [ 1215.909342] vconn:=1
> > [ 1215.909549] vbus:=1 charge=0
> > [ 1215.909633] Setting pd capable false
> > [ 1215.909637] pending state change SRC_ATTACHED -> SRC_UNATTACHED @
> > 480 ms
> > [ 1215.909685] VBUS on
> > [ 1215.909687] state change SRC_ATTACHED -> SRC_STARTUP
> > [ 1215.914263] AMS POWER_NEGOTIATION start
> > [ 1215.914267] cc:=4
> > [ 1215.914288] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> > [ 1215.914294] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS
> > POWER_NEGOTIATION
> > [ 1215.914298] PD TX, header: 0x11a1
> > [ 1215.919552] PD TX complete, status: 2
> > [ 1215.919577] pending state change SRC_SEND_CAPABILITIES ->
> > SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
> > [ 1216.071041] state change SRC_SEND_CAPABILITIES ->
> > SRC_SEND_CAPABILITIES [delayed 150 ms]
> > [ 1216.071049] PD TX, header: 0x11a1
> > [ 1216.072508] PD TX complete, status: 0
> > [ 1216.072535] Setting pd capable true
> > [ 1216.072645] pending state change SRC_SEND_CAPABILITIES ->
> > HARD_RESET_SEND @ 150 ms in AMS POWER_NEGOTIATION
> > [ 1216.073915] PD RX, header: 0x1042 [1]
> > [ 1216.073920] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
> > [ 1216.073941] Requested 5000 mV, 900 mA for 900 / 900 mA
> > [ 1216.073944] PD TX, header: 0x363
> > [ 1216.075281] PD TX complete, status: 0
> > [ 1216.075337] pending state change SRC_NEGOTIATE_CAPABILITIES ->
> > SRC_TRANSITION_SUPPLY @ 35 ms
> > [ 1216.113742] state change SRC_NEGOTIATE_CAPABILITIES ->
> > SRC_TRANSITION_SUPPLY [delayed 35 ms]
> > [ 1216.113752] PD TX, header: 0x566
> > [ 1216.116743] PD TX complete, status: 0
> > [ 1216.117175] state change SRC_TRANSITION_SUPPLY -> SRC_READY
> > [ 1216.118737] PD TX, header: 0x176f
> > [ 1216.120285] PD TX complete, status: 0
> > [ 1216.122455] PD RX, header: 0x524f [1]
> > [ 1216.122463] Rx VDM cmd 0xff008041 type 1 cmd 1 len 5
> > [ 1216.122511] Identity: 04b4:1120.0000
> > [ 1216.122531] PD TX, header: 0x196f
> > [ 1216.123965] PD TX complete, status: 0
> > [ 1216.125573] PD RX, header: 0x244f [1]
> > [ 1216.125578] Rx VDM cmd 0xff008042 type 1 cmd 2 len 2
> > [ 1216.125581] SVID 1: 0xff01
> > [ 1216.125603] PD TX, header: 0x1b6f
> > [ 1216.127074] PD TX complete, status: 0
> > [ 1216.128750] PD RX, header: 0x264f [1]
> > [ 1216.128754] Rx VDM cmd 0xff018043 type 1 cmd 3 len 2
> > [ 1216.128758] Alternate mode 0: SVID 0xff01, VDO 1: 0x00000405
> >
> > > > [ 693.474855] ? _cond_resched+0x10/0x20
> > > > [ 693.477807] tcpm_state_machine_work+0x57e/0x28f6 [tcpm]
> > > > [ 693.480776] ? tcpm_pd_event_handler+0x111/0x320 [tcpm]
> > > > [ 693.483743] process_one_work+0x1da/0x410
> > > > [ 693.486703] worker_thread+0x28/0x3c0
> > > > [ 693.489651] ? process_one_work+0x410/0x410
> > > > [ 693.492603] kthread+0x10b/0x130
> > > > [ 693.495548] ? kthread_create_on_node+0x60/0x60
> > > > [ 693.498506] ret_from_fork+0x1f/0x30
> > > >
> > > >
> > > > Here's the tcpm debugfs log:
> > > >
> > > > [ 692.553000] CC1: 0 -> 2, CC2: 0 -> 1 [state DRP_TOGGLING, polarity 0, connected]
> > > > [ 692.553016] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
> > > > [ 692.553056] pending state change SRC_ATTACH_WAIT -> SNK_TRY @ 200 ms
> > > > [ 692.757402] state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
> > > > [ 692.757410] cc:=2
> > > > [ 692.761946] pending state change SNK_TRY -> SNK_TRY_WAIT @ 100 ms
> > > > [ 692.869308] state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
> > > > [ 692.869313] state change SNK_TRY_WAIT -> SRC_TRYWAIT
> > > > [ 692.869317] cc:=3
> > > > [ 692.873657] pending state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED @ 100 ms
> > > > [ 692.882524] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_TRYWAIT, polarity 0, connected]
> > > > [ 692.882537] state change SRC_TRYWAIT -> SRC_TRYWAIT_DEBOUNCE
> > > > [ 692.882567] pending state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED @ 200 ms
> > > > [ 693.085337] state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED [delayed 200 ms]
> > > > [ 693.085347] polarity 0
> > > > [ 693.085352] Requesting mux state 1, usb-role 1, orientation 1
> > > > [ 693.346845] vconn:=1
> > > > [ 693.347174] vbus:=1 charge=0
> > > > [ 693.358340] pending state change SRC_ATTACHED -> SRC_UNATTACHED @ 480 ms
> > > > [ 693.378702] VBUS on
> > > > [ 693.378711] state change SRC_ATTACHED -> SRC_STARTUP
> > > > [ 693.378741] AMS POWER_NEGOTIATION start
> > > > [ 693.378745] cc:=4
> > > > [ 693.505321] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> > > > [ 693.505325] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
> > > > [ 693.505327] PD TX, header: 0x11a1
> > > > [ 693.613296] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
> > > > [ 693.613309] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
> > > > [ 693.765730] state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES [delayed 150 ms]
> > > > [ 693.765753] PD TX, header: 0x11a1
> > > > [ 693.770016] PD TX complete, status: 0
> > > > [ 693.770261] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES_TIMEOUT @ 150 ms in AMS POWER_NEGOTIATION
> > > > [ 693.775178] PD RX, header: 0x1042 [1]
> > > > [ 693.775195] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
> > > > [ 693.775236] Requested 5000 mV, 400 mA for 400 / 900 mA
> > > > [ 693.775244] PD TX, header: 0x363
> > > > [ 693.778253] PD TX complete, status: 0
> > > > [ 693.778363] pending state change SRC_NEGOTIATE_CAPABILITIES -> SRC_TRANSITION_SUPPLY @ 35 ms
> > > > [ 693.803463] Received hard reset
> > > > [ 693.803473] state change SRC_NEGOTIATE_CAPABILITIES -> HARD_RESET_START
> > > > [ 693.806323] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms
> > > > [ 693.837400] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
> > > > [ 693.837414] vconn:=1
> > > > [ 693.837426] vbus:=0 charge=0
> > > > [ 693.843380] Requesting mux state 1, usb-role 1, orientation 1
> > > > [ 693.844554] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms
> > > > [ 693.844575] VBUS off
> > > > [ 693.844580] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON
> > > > [ 693.844617] vbus:=1 charge=0
> > > > [ 693.850688] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms
> > > > [ 693.868706] VBUS on
> > > > [ 693.868713] state change SRC_HARD_RESET_VBUS_ON -> SRC_STARTUP
> > > > [ 693.868742] AMS POWER_NEGOTIATION start
> > > > [ 693.868749] cc:=4
> > > > [ 694.101422] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> > > > [ 694.101425] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
> > > > [ 694.101428] PD TX, header: 0x11a1
> > > > [ 694.205301] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
> > > > [ 694.205317] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
> > > > [ 694.325808] Received hard reset
> > > > [ 694.325816] state change SRC_SEND_CAPABILITIES -> HARD_RESET_START in AMS NONE_AMS
> > > > [ 694.329208] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms in AMS NONE_AMS
> > > > [ 694.359394] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
> > > > [ 694.359401] vconn:=1
> > > > [ 694.359408] vbus:=0 charge=0
> > > > [ 694.366321] Requesting mux state 1, usb-role 1, orientation 1
> > > > [ 694.367685] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms in AMS NONE_AMS
> > > > [ 694.367700] VBUS off
> > > > [ 694.367704] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON in AMS NONE_AMS
> > > > [ 694.367721] vbus:=1 charge=0
> > > > [ 694.374175] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms in AMS NONE_AMS
> > > > [ 694.374194] CC1: 2 -> 0, CC2: 1 -> 0 [state SRC_HARD_RESET_VBUS_ON, polarity 0, disconnected]
> > > > [ 694.374201] state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED in AMS NONE_AMS
> > > > [ 694.631957] Start DRP toggling

thanks,

--
heikki

2019-04-09 14:42:18

by Hans de Goede

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

Hi,

On 09-04-19 15:06, Heikki Krogerus wrote:
> On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote:
>> +Hans
>>
>> On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
>>> On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck <[email protected]> wrote:
>>>>
>>>> On 4/4/19 7:13 AM, Heikki Krogerus wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
>>>>>> This patch provides the implementation of Collision Avoidance introduced
>>>>>> in PD3.0. The start of each Atomic Message Sequence (AMS) initiated by
>>>>>> the port will be denied if the current AMS is not interruptible. The
>>>>>> Source port will set the CC to SinkTxNG if it is going to initiate an
>>>>>> AMS, and SinkTxOk otherwise. Meanwhile, any AMS initiated by a Sink port
>>>>>> will be denied in TCPM if the port partner (Source) sets SinkTxNG except
>>>>>> for HARD_RESET and SOFT_RESET.
>>>>>
>>>>> I tested this with my GDBWin which has fusb302. When I plug-in
>>>>> DisplayPort adapter, the partner device never gets registered, and I
>>>>> see steady flow of warnings from fusb302:
>>>>>
>>>>
>>>> FWIW, I made multiple attempts to review the patch. Each time I get stuck
>>>> after a while and notice that I don't understand what is going on.
>>>>
>>>> Maybe the state machine needs a complete overhaul. It seems to have reached
>>>> a point where it is getting too complex to understand what is going on.
>>>>
>>>>> [ 693.391176] Vconn is on during toggle start
>>>>> [ 693.391250] WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562 fusb302_set_toggling+0x129/0x130 [fusb302]
>>>>> [ 693.400293] Modules linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532 i915 typec intel_gtt intel_cht_int33fe
>>>>> [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W 5.1.0-rc3-heikki+ #17
>>>>> [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not detect charger type
>>>>> [ 693.412278] Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017
>>>>> [ 693.412283] Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm]
>>>>> [ 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 [fusb302]
>>>>> [ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6 c6 83 b0 01 00 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0 e8 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54 41 89 f4 55 53 48 8d
>>>>> [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS: 00010286
>>>>> [ 693.439174] RAX: 0000000000000000 RBX: ffff888178080028 RCX: 0000000000000000
>>>>> [ 693.442157] RDX: 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f
>>>>> [ 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09: 00000000000202c0
>>>>> [ 693.448100] R10: 0000010cb24a3d18 R11: 000000000000001e R12: ffff8881780801b0
>>>>> [ 693.451086] R13: ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040
>>>>> [ 693.454060] FS: 0000000000000000(0000) GS:ffff88817bb00000(0000) knlGS:0000000000000000
>>>>> [ 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> [ 693.462984] CR2: 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0
>>>>> [ 693.465969] Call Trace:
>>>>> [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302]
>>>>> [ 693.471894] tcpm_ams_start+0x1b8/0x2a0 [tcpm]
>>>>
>>>> tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter what. This causes
>>>> the fusb302 code to start toggling. As such, it may well attempt to start toggling
>>>> in the wrong state.
>>>>
>>>> Guenter
>>>>
>>>
>>> I read the fusb302 spec but failed to find the statement that says it
>>> should "set toggling" when CC switches among default/medium/high.
>>>
>>> quot from fusb302 spec:
>>> "The FUSB302 allows the host software to change the charging current
>>> capabilities of the port through the HOST_CUR control bits. If the
>>> HOST_CUR bits are changed prior to attach, the FUSB302 automatically
>>> indicates the programmed current capability when a device is attached.
>>> If the current capabilities are changed after a device is attached,
>>> the FUSB302 immediately changes the CC line to the programmed
>>> capability."
>>>
>>> Is it possible to skip fusb302_set_toggling() @ line#658 if
>>> tcpm_set_cc() is called in order to switch the cc among
>>> default/medium/high of Rp ?
>
> Hans, you introduced that in commit daf81d0137a9c ("usb: typec:
> fusb302: Refactor / simplify tcpm_set_cc()"), so could you take a look
> at this.

I do not believe that that commit introduces the fusb302_set_toggling()
as the subject of the commit says it just refactors things, the
set_toggling call was introduced by:

commit ea3b4d5523bc8("usb: typec: fusb302: Resolve fixed power role contract setup")

Before that:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564

tcpm_set_cc actually turned toggling off in all cases.

I've no doubt that Adam was seeing a real problem, but I've doubted if this
was the right fix before. I even had it reverted in my tree for a while, but
since in my use-cases so far it has not caused any problems I've not looked
into it further.

In the mean time the code has changed quite a bit though, so making
tcpm_set_cc() behave as it did before, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564

Will require writing something from scratch based on the new code which
mimicks the behaviour of the old code; and then we also need to fix Adam's
problem on top.

Regards,

Hans






>
>>> the tcpm log on another PD controller:
>>> [ 1215.709538] CC1: 0 -> 2, CC2: 0 -> 1 [state DRP_TOGGLING, polarity
>>> 0, connected]
>>> [ 1215.709551] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
>>> [ 1215.709576] pending state change SRC_ATTACH_WAIT -> SRC_ATTACHED @
>>> 200 ms
>>> [ 1215.907465] state change SRC_ATTACH_WAIT -> SRC_ATTACHED [delayed
>>> 200 ms]
>>> [ 1215.907670] polarity 0
>>> [ 1215.908070] Requesting mux state 1, usb-role 1, orientation 1
>>> [ 1215.909342] vconn:=1
>>> [ 1215.909549] vbus:=1 charge=0
>>> [ 1215.909633] Setting pd capable false
>>> [ 1215.909637] pending state change SRC_ATTACHED -> SRC_UNATTACHED @
>>> 480 ms
>>> [ 1215.909685] VBUS on
>>> [ 1215.909687] state change SRC_ATTACHED -> SRC_STARTUP
>>> [ 1215.914263] AMS POWER_NEGOTIATION start
>>> [ 1215.914267] cc:=4
>>> [ 1215.914288] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
>>> [ 1215.914294] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS
>>> POWER_NEGOTIATION
>>> [ 1215.914298] PD TX, header: 0x11a1
>>> [ 1215.919552] PD TX complete, status: 2
>>> [ 1215.919577] pending state change SRC_SEND_CAPABILITIES ->
>>> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
>>> [ 1216.071041] state change SRC_SEND_CAPABILITIES ->
>>> SRC_SEND_CAPABILITIES [delayed 150 ms]
>>> [ 1216.071049] PD TX, header: 0x11a1
>>> [ 1216.072508] PD TX complete, status: 0
>>> [ 1216.072535] Setting pd capable true
>>> [ 1216.072645] pending state change SRC_SEND_CAPABILITIES ->
>>> HARD_RESET_SEND @ 150 ms in AMS POWER_NEGOTIATION
>>> [ 1216.073915] PD RX, header: 0x1042 [1]
>>> [ 1216.073920] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
>>> [ 1216.073941] Requested 5000 mV, 900 mA for 900 / 900 mA
>>> [ 1216.073944] PD TX, header: 0x363
>>> [ 1216.075281] PD TX complete, status: 0
>>> [ 1216.075337] pending state change SRC_NEGOTIATE_CAPABILITIES ->
>>> SRC_TRANSITION_SUPPLY @ 35 ms
>>> [ 1216.113742] state change SRC_NEGOTIATE_CAPABILITIES ->
>>> SRC_TRANSITION_SUPPLY [delayed 35 ms]
>>> [ 1216.113752] PD TX, header: 0x566
>>> [ 1216.116743] PD TX complete, status: 0
>>> [ 1216.117175] state change SRC_TRANSITION_SUPPLY -> SRC_READY
>>> [ 1216.118737] PD TX, header: 0x176f
>>> [ 1216.120285] PD TX complete, status: 0
>>> [ 1216.122455] PD RX, header: 0x524f [1]
>>> [ 1216.122463] Rx VDM cmd 0xff008041 type 1 cmd 1 len 5
>>> [ 1216.122511] Identity: 04b4:1120.0000
>>> [ 1216.122531] PD TX, header: 0x196f
>>> [ 1216.123965] PD TX complete, status: 0
>>> [ 1216.125573] PD RX, header: 0x244f [1]
>>> [ 1216.125578] Rx VDM cmd 0xff008042 type 1 cmd 2 len 2
>>> [ 1216.125581] SVID 1: 0xff01
>>> [ 1216.125603] PD TX, header: 0x1b6f
>>> [ 1216.127074] PD TX complete, status: 0
>>> [ 1216.128750] PD RX, header: 0x264f [1]
>>> [ 1216.128754] Rx VDM cmd 0xff018043 type 1 cmd 3 len 2
>>> [ 1216.128758] Alternate mode 0: SVID 0xff01, VDO 1: 0x00000405
>>>
>>>>> [ 693.474855] ? _cond_resched+0x10/0x20
>>>>> [ 693.477807] tcpm_state_machine_work+0x57e/0x28f6 [tcpm]
>>>>> [ 693.480776] ? tcpm_pd_event_handler+0x111/0x320 [tcpm]
>>>>> [ 693.483743] process_one_work+0x1da/0x410
>>>>> [ 693.486703] worker_thread+0x28/0x3c0
>>>>> [ 693.489651] ? process_one_work+0x410/0x410
>>>>> [ 693.492603] kthread+0x10b/0x130
>>>>> [ 693.495548] ? kthread_create_on_node+0x60/0x60
>>>>> [ 693.498506] ret_from_fork+0x1f/0x30
>>>>>
>>>>>
>>>>> Here's the tcpm debugfs log:
>>>>>
>>>>> [ 692.553000] CC1: 0 -> 2, CC2: 0 -> 1 [state DRP_TOGGLING, polarity 0, connected]
>>>>> [ 692.553016] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
>>>>> [ 692.553056] pending state change SRC_ATTACH_WAIT -> SNK_TRY @ 200 ms
>>>>> [ 692.757402] state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
>>>>> [ 692.757410] cc:=2
>>>>> [ 692.761946] pending state change SNK_TRY -> SNK_TRY_WAIT @ 100 ms
>>>>> [ 692.869308] state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
>>>>> [ 692.869313] state change SNK_TRY_WAIT -> SRC_TRYWAIT
>>>>> [ 692.869317] cc:=3
>>>>> [ 692.873657] pending state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED @ 100 ms
>>>>> [ 692.882524] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_TRYWAIT, polarity 0, connected]
>>>>> [ 692.882537] state change SRC_TRYWAIT -> SRC_TRYWAIT_DEBOUNCE
>>>>> [ 692.882567] pending state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED @ 200 ms
>>>>> [ 693.085337] state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED [delayed 200 ms]
>>>>> [ 693.085347] polarity 0
>>>>> [ 693.085352] Requesting mux state 1, usb-role 1, orientation 1
>>>>> [ 693.346845] vconn:=1
>>>>> [ 693.347174] vbus:=1 charge=0
>>>>> [ 693.358340] pending state change SRC_ATTACHED -> SRC_UNATTACHED @ 480 ms
>>>>> [ 693.378702] VBUS on
>>>>> [ 693.378711] state change SRC_ATTACHED -> SRC_STARTUP
>>>>> [ 693.378741] AMS POWER_NEGOTIATION start
>>>>> [ 693.378745] cc:=4
>>>>> [ 693.505321] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
>>>>> [ 693.505325] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
>>>>> [ 693.505327] PD TX, header: 0x11a1
>>>>> [ 693.613296] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
>>>>> [ 693.613309] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
>>>>> [ 693.765730] state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES [delayed 150 ms]
>>>>> [ 693.765753] PD TX, header: 0x11a1
>>>>> [ 693.770016] PD TX complete, status: 0
>>>>> [ 693.770261] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES_TIMEOUT @ 150 ms in AMS POWER_NEGOTIATION
>>>>> [ 693.775178] PD RX, header: 0x1042 [1]
>>>>> [ 693.775195] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
>>>>> [ 693.775236] Requested 5000 mV, 400 mA for 400 / 900 mA
>>>>> [ 693.775244] PD TX, header: 0x363
>>>>> [ 693.778253] PD TX complete, status: 0
>>>>> [ 693.778363] pending state change SRC_NEGOTIATE_CAPABILITIES -> SRC_TRANSITION_SUPPLY @ 35 ms
>>>>> [ 693.803463] Received hard reset
>>>>> [ 693.803473] state change SRC_NEGOTIATE_CAPABILITIES -> HARD_RESET_START
>>>>> [ 693.806323] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms
>>>>> [ 693.837400] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
>>>>> [ 693.837414] vconn:=1
>>>>> [ 693.837426] vbus:=0 charge=0
>>>>> [ 693.843380] Requesting mux state 1, usb-role 1, orientation 1
>>>>> [ 693.844554] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms
>>>>> [ 693.844575] VBUS off
>>>>> [ 693.844580] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON
>>>>> [ 693.844617] vbus:=1 charge=0
>>>>> [ 693.850688] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms
>>>>> [ 693.868706] VBUS on
>>>>> [ 693.868713] state change SRC_HARD_RESET_VBUS_ON -> SRC_STARTUP
>>>>> [ 693.868742] AMS POWER_NEGOTIATION start
>>>>> [ 693.868749] cc:=4
>>>>> [ 694.101422] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
>>>>> [ 694.101425] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS POWER_NEGOTIATION
>>>>> [ 694.101428] PD TX, header: 0x11a1
>>>>> [ 694.205301] pending state change SRC_SEND_CAPABILITIES -> SRC_SEND_CAPABILITIES @ 150 ms in AMS POWER_NEGOTIATION
>>>>> [ 694.205317] CC1: 2 -> 2, CC2: 1 -> 1 [state SRC_SEND_CAPABILITIES, polarity 0, connected]
>>>>> [ 694.325808] Received hard reset
>>>>> [ 694.325816] state change SRC_SEND_CAPABILITIES -> HARD_RESET_START in AMS NONE_AMS
>>>>> [ 694.329208] pending state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF @ 30 ms in AMS NONE_AMS
>>>>> [ 694.359394] state change HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF [delayed 30 ms]
>>>>> [ 694.359401] vconn:=1
>>>>> [ 694.359408] vbus:=0 charge=0
>>>>> [ 694.366321] Requesting mux state 1, usb-role 1, orientation 1
>>>>> [ 694.367685] pending state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON @ 760 ms in AMS NONE_AMS
>>>>> [ 694.367700] VBUS off
>>>>> [ 694.367704] state change SRC_HARD_RESET_VBUS_OFF -> SRC_HARD_RESET_VBUS_ON in AMS NONE_AMS
>>>>> [ 694.367721] vbus:=1 charge=0
>>>>> [ 694.374175] pending state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED @ 480 ms in AMS NONE_AMS
>>>>> [ 694.374194] CC1: 2 -> 0, CC2: 1 -> 0 [state SRC_HARD_RESET_VBUS_ON, polarity 0, disconnected]
>>>>> [ 694.374201] state change SRC_HARD_RESET_VBUS_ON -> SRC_UNATTACHED in AMS NONE_AMS
>>>>> [ 694.631957] Start DRP toggling
>
> thanks,
>

2019-04-10 10:36:07

by Adam Thomson

Subject: RE: [PATCH v2] usb: typec: tcpm: collision avoidance

On 09 April 2019 15:41, Hans de Goede wrote:

> Hi,
>
> On 09-04-19 15:06, Heikki Krogerus wrote:
> > On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote:
> >> +Hans
> >>
> >> On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
> >>> On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck <[email protected]> wrote:
> >>>>
> >>>> On 4/4/19 7:13 AM, Heikki Krogerus wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
> >>>>>> This patch provides the implementation of Collision Avoidance
> >>>>>> introduced in PD3.0. The start of each Atomic Message Sequence
> >>>>>> (AMS) initiated by the port will be denied if the current AMS is
> >>>>>> not interruptible. The Source port will set the CC to SinkTxNG if
> >>>>>> it is going to initiate an AMS, and SinkTxOk otherwise.
> >>>>>> Meanwhile, any AMS initiated by a Sink port will be denied in
> >>>>>> TCPM if the port partner (Source) sets SinkTxNG except for HARD_RESET
> and SOFT_RESET.
> >>>>>
> >>>>> I tested this with my GDBWin which has fusb302. When I plug-in
> >>>>> DisplayPort adapter, the partner device never gets registered, and
> >>>>> I see steady flow of warnings from fusb302:
> >>>>>
> >>>>
> >>>> FWIW, I made multiple attempts to review the patch. Each time I get
> >>>> stuck after a while and notice that I don't understand what is going on.
> >>>>
> >>>> Maybe the state machine needs a complete overhaul. It seems to have
> >>>> reached a point where it is getting too complex to understand what is going
> on.
> >>>>
> >>>>> [ 693.391176] Vconn is on during toggle start [ 693.391250]
> >>>>> WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562
> >>>>> fusb302_set_toggling+0x129/0x130 [fusb302] [ 693.400293] Modules
> linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532 i915 typec
> intel_gtt intel_cht_int33fe
> >>>>> [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W
> 5.1.0-rc3-heikki+ #17
> >>>>> [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not detect
> >>>>> charger type [ 693.412278] Hardware name: Default string Default
> >>>>> string/Default string, BIOS 5.11 05/25/2017 [ 693.412283]
> >>>>> Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm] [
> >>>>> 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 [fusb302] [
> >>>>> 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6 c6 83 b0 01 00
> >>>>> 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0 e8
> >>>>> 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54 41 89
> >>>>> f4 55 53 48 8d [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS:
> >>>>> 00010286 [ 693.439174] RAX: 0000000000000000 RBX:
> >>>>> ffff888178080028 RCX: 0000000000000000 [ 693.442157] RDX:
> >>>>> 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f [
> >>>>> 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09:
> >>>>> 00000000000202c0 [ 693.448100] R10: 0000010cb24a3d18 R11:
> >>>>> 000000000000001e R12: ffff8881780801b0 [ 693.451086] R13:
> ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040 [ 693.454060] FS:
> 0000000000000000(0000) GS:ffff88817bb00000(0000) knlGS:0000000000000000 [
> 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 693.462984] CR2:
> 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0 [ 693.465969]
> Call Trace:
> >>>>> [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302] [ 693.471894]
> >>>>> tcpm_ams_start+0x1b8/0x2a0 [tcpm]
> >>>>
> >>>> tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter
> >>>> what. This causes the fusb302 code to start toggling. As such, it
> >>>> may well attempt to start toggling in the wrong state.
> >>>>
> >>>> Guenter
> >>>>
> >>>
> >>> I read the fusb302 spec but failed to find the statement that says
> >>> it should "set toggling" when CC switches among default/medium/high.
> >>>
> >>> quot from fusb302 spec:
> >>> "The FUSB302 allows the host software to change the charging current
> >>> capabilities of the port through the HOST_CUR control bits. If the
> >>> HOST_CUR bits are changed prior to attach, the FUSB302 automatically
> >>> indicates the programmed current capability when a device is attached.
> >>> If the current capabilities are changed after a device is attached,
> >>> the FUSB302 immediately changes the CC line to the programmed
> >>> capability."
> >>>
> >>> Is it possible to skip fusb302_set_toggling() @ line#658 if
> >>> tcpm_set_cc() is called in order to switch the cc among
> >>> default/medium/high of Rp ?
> >
> > Hans, you introduced that in commit daf81d0137a9c ("usb: typec:
> > fusb302: Refactor / simplify tcpm_set_cc()"), so could you take a look
> > at this.
>
> I do not believe that that commit introduces the fusb302_set_toggling() as the
> subject of the commit says it just refactors things, the set_toggling call was
> introduced by:
>
> commit ea3b4d5523bc8("usb: typec: fusb302: Resolve fixed power role contract
> setup")
>
> Before that:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/u
> sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
>
> tcpm_set_cc actually turned toggling off in all cases.
>
> I've no doubt that Adam was seeing a real problem, but I've doubted if this was
> the right fix before. I even had it reverted in my tree for a while, but since in my
> use-cases so far it has not caused any problems I've not looked into it further.

From my recollection, that was the only way to generate the necessary event from
fusb302 to indicate a connection, when the device was in a fixed role state
(i.e. only source or only sink). Without it the driver doesn't work in these
scenarios as there's no TOGDONE event generated by fusb302, so no eventual call
to 'tcpm_cc_change()' to tell TCPM that something has happened and move on the
state machine. Not all devices will be DRP so we have to account for this.

>
> In the mean time the code has changed quite a bit though, so making
> tcpm_set_cc() behave as it did before, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/u
> sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
>
> Will require writing something from scratch based on the new code which
> mimicks the behaviour of the old code; and then we also need to fix Adam's
> problem on top.
>
> Regards,
>
> Hans

2019-04-10 13:38:45

by Kyle Tso

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

On Wed, Apr 10, 2019 at 6:32 PM Adam Thomson
<[email protected]> wrote:
>
> On 09 April 2019 15:41, Hans de Goede wrote:
>
> > Hi,
> >
> > On 09-04-19 15:06, Heikki Krogerus wrote:
> > > On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote:
> > >> +Hans
> > >>
> > >> On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
> > >>> On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck <[email protected]> wrote:
> > >>>>
> > >>>> On 4/4/19 7:13 AM, Heikki Krogerus wrote:
> > >>>>> Hi,
> > >>>>>
> > >>>>> On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
> > >>>>>> This patch provides the implementation of Collision Avoidance
> > >>>>>> introduced in PD3.0. The start of each Atomic Message Sequence
> > >>>>>> (AMS) initiated by the port will be denied if the current AMS is
> > >>>>>> not interruptible. The Source port will set the CC to SinkTxNG if
> > >>>>>> it is going to initiate an AMS, and SinkTxOk otherwise.
> > >>>>>> Meanwhile, any AMS initiated by a Sink port will be denied in
> > >>>>>> TCPM if the port partner (Source) sets SinkTxNG except for HARD_RESET
> > and SOFT_RESET.
> > >>>>>
> > >>>>> I tested this with my GDBWin which has fusb302. When I plug-in
> > >>>>> DisplayPort adapter, the partner device never gets registered, and
> > >>>>> I see steady flow of warnings from fusb302:
> > >>>>>
> > >>>>
> > >>>> FWIW, I made multiple attempts to review the patch. Each time I get
> > >>>> stuck after a while and notice that I don't understand what is going on.
> > >>>>
> > >>>> Maybe the state machine needs a complete overhaul. It seems to have
> > >>>> reached a point where it is getting too complex to understand what is going
> > on.
> > >>>>
> > >>>>> [ 693.391176] Vconn is on during toggle start [ 693.391250]
> > >>>>> WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562
> > >>>>> fusb302_set_toggling+0x129/0x130 [fusb302] [ 693.400293] Modules
> > linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532 i915 typec
> > intel_gtt intel_cht_int33fe
> > >>>>> [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W
> > 5.1.0-rc3-heikki+ #17
> > >>>>> [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not detect
> > >>>>> charger type [ 693.412278] Hardware name: Default string Default
> > >>>>> string/Default string, BIOS 5.11 05/25/2017 [ 693.412283]
> > >>>>> Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm] [
> > >>>>> 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 [fusb302] [
> > >>>>> 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6 c6 83 b0 01 00
> > >>>>> 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0 e8
> > >>>>> 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54 41 89
> > >>>>> f4 55 53 48 8d [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS:
> > >>>>> 00010286 [ 693.439174] RAX: 0000000000000000 RBX:
> > >>>>> ffff888178080028 RCX: 0000000000000000 [ 693.442157] RDX:
> > >>>>> 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f [
> > >>>>> 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09:
> > >>>>> 00000000000202c0 [ 693.448100] R10: 0000010cb24a3d18 R11:
> > >>>>> 000000000000001e R12: ffff8881780801b0 [ 693.451086] R13:
> > ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040 [ 693.454060] FS:
> > 0000000000000000(0000) GS:ffff88817bb00000(0000) knlGS:0000000000000000 [
> > 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 693.462984] CR2:
> > 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0 [ 693.465969]
> > Call Trace:
> > >>>>> [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302] [ 693.471894]
> > >>>>> tcpm_ams_start+0x1b8/0x2a0 [tcpm]
> > >>>>
> > >>>> tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter
> > >>>> what. This causes the fusb302 code to start toggling. As such, it
> > >>>> may well attempt to start toggling in the wrong state.
> > >>>>
> > >>>> Guenter
> > >>>>
> > >>>
> > >>> I read the fusb302 spec but failed to find the statement that says
> > >>> it should "set toggling" when CC switches among default/medium/high.
> > >>>
> > >>> quot from fusb302 spec:
> > >>> "The FUSB302 allows the host software to change the charging current
> > >>> capabilities of the port through the HOST_CUR control bits. If the
> > >>> HOST_CUR bits are changed prior to attach, the FUSB302 automatically
> > >>> indicates the programmed current capability when a device is attached.
> > >>> If the current capabilities are changed after a device is attached,
> > >>> the FUSB302 immediately changes the CC line to the programmed
> > >>> capability."
> > >>>
> > >>> Is it possible to skip fusb302_set_toggling() @ line#658 if
> > >>> tcpm_set_cc() is called in order to switch the cc among
> > >>> default/medium/high of Rp ?
> > >
> > > Hans, you introduced that in commit daf81d0137a9c ("usb: typec:
> > > fusb302: Refactor / simplify tcpm_set_cc()"), so could you take a look
> > > at this.
> >
> > I do not believe that that commit introduces the fusb302_set_toggling() as the
> > subject of the commit says it just refactors things, the set_toggling call was
> > introduced by:
> >
> > commit ea3b4d5523bc8("usb: typec: fusb302: Resolve fixed power role contract
> > setup")
> >
> > Before that:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/u
> > sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
> >
> > tcpm_set_cc actually turned toggling off in all cases.
> >
> > I've no doubt that Adam was seeing a real problem, but I've doubted if this was
> > the right fix before. I even had it reverted in my tree for a while, but since in my
> > use-cases so far it has not caused any problems I've not looked into it further.
>
> From my recollection, that was the only way to generate the necessary event from
> fusb302 to indicate a connection, when the device was in a fixed role state
> (i.e. only source or only sink). Without it the driver doesn't work in these
> scenarios as there's no TOGDONE event generated by fusb302, so no eventual call
> to 'tcpm_cc_change()' to tell TCPM that something has happened and move on the
> state machine. Not all devices will be DRP so we have to account for this.
>

The switch among different Rp values on CC pins comes from TCPM and
after the switch finishes, TCPM doesn't need to update the CC status because
this kind of switch won't affect the state machine.

> >
> > In the mean time the code has changed quite a bit though, so making
> > tcpm_set_cc() behave as it did before, see:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/u
> > sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
> >
> > Will require writing something from scratch based on the new code which
> > mimicks the behaviour of the old code; and then we also need to fix Adam's
> > problem on top.
> >
> > Regards,
> >
> > Hans

I tried to fix this with below changes and it works.

============================================
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -110,6 +110,9 @@ struct fusb302_chip {
enum typec_cc_status cc2;
u32 snk_pdo[PDO_MAX_OBJECTS];

+ /* Local pin status */
+ enum typec_cc_status cc;
+
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
/* lock for log buffer access */
@@ -611,6 +614,19 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum
typec_cc_status cc)
enum toggling_mode mode;

mutex_lock(&chip->lock);
+ if ((chip->cc == TYPEC_CC_RP_DEF || chip->cc == TYPEC_CC_RP_1_5 ||
+ chip->cc == TYPEC_CC_RP_3_0) && (cc == TYPEC_CC_RP_DEF ||
+ cc == TYPEC_CC_RP_1_5 || cc == TYPEC_CC_RP_3_0)) {
+ ret = fusb302_set_src_current(chip, cc_src_current[cc]);
+ if (ret < 0) {
+ fusb302_log(chip, "cannot set src current %s, ret=%d\n",
+ typec_cc_status_name[cc], ret);
+ goto done;
+ }
+ fusb302_log(chip, "cc := %s", typec_cc_status_name[cc]);
+ goto rp_switch;
+ }
+
switch (cc) {
case TYPEC_CC_OPEN:
mode = TOGGLING_MODE_OFF;
@@ -659,6 +675,8 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum
typec_cc_status cc)
if (ret < 0)
fusb302_log(chip, "cannot set toggling mode, ret=%d", ret);

+rp_switch:
+ chip->cc = cc;
done:
mutex_unlock(&chip->lock);
============================================

TCPM logs:

[ 52.267343] CC1: 0 -> 1, CC2: 0 -> 2 [state DRP_TOGGLING, polarity
0, connected]
[ 52.267354] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
[ 52.267435] pending state change SRC_ATTACH_WAIT -> SNK_TRY @ 200 ms
[ 52.466852] state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
[ 52.466876] cc:=2
[ 52.471851] pending state change SNK_TRY -> SNK_TRY_WAIT @ 140 ms
[ 52.610244] state change SNK_TRY -> SNK_TRY_WAIT [delayed 140 ms]
[ 52.610256] state change SNK_TRY_WAIT -> SRC_TRYWAIT
[ 52.610298] cc:=3
[ 52.613262] pending state change SRC_TRYWAIT ->
SRC_TRYWAIT_UNATTACHED @ 140 ms
[ 52.625248] CC1: 1 -> 1, CC2: 2 -> 2 [state SRC_TRYWAIT, polarity
0, connected]
[ 52.625261] state change SRC_TRYWAIT -> SRC_TRYWAIT_DEBOUNCE
[ 52.625299] pending state change SRC_TRYWAIT_DEBOUNCE ->
SRC_ATTACHED @ 200 ms
[ 52.823417] state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED
[delayed 200 ms]
[ 52.823425] polarity 1
[ 52.823458] Requesting mux state 1, usb-role 1, orientation 2
[ 52.846919] vconn:=1
[ 52.854536] vbus:=1 charge=0
[ 52.893437] pending state change SRC_ATTACHED -> SRC_UNATTACHED @ 480 ms
[ 52.894715] VBUS on
[ 52.894718] state change SRC_ATTACHED -> SRC_STARTUP
[ 52.894733] AMS POWER_NEGOTIATION start
[ 52.894736] cc:=4
[ 52.895043] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
[ 52.895046] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS
POWER_NEGOTIATION
[ 52.895048] PD TX, header: 0x11a1
[ 52.910309] PD TX complete, status: 0
[ 52.910346] pending state change SRC_SEND_CAPABILITIES ->
HARD_RESET_SEND @ 150 ms in AMS POWER_NEGOTIATION
[ 52.912383] PD RX, header: 0x1042 [1]
[ 52.912387] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
[ 52.912398] Requested 5000 mV, 900 mA for 900 / 900 mA
[ 52.912400] PD TX, header: 0x363
[ 52.915725] PD TX complete, status: 0
[ 52.915765] pending state change SRC_NEGOTIATE_CAPABILITIES ->
SRC_TRANSITION_SUPPLY @ 35 ms
[ 52.950125] state change SRC_NEGOTIATE_CAPABILITIES ->
SRC_TRANSITION_SUPPLY [delayed 35 ms]
[ 52.950139] PD TX, header: 0x566
[ 52.953910] PD TX complete, status: 0
[ 52.955573] state change SRC_TRANSITION_SUPPLY -> SRC_READY
[ 52.957359] PD TX, header: 0x176f
[ 52.962785] PD TX complete, status: 0
[ 52.966026] PD RX, header: 0x524f [1]
[ 52.966037] Rx VDM cmd 0xff008041 type 1 cmd 1 len 5
[ 52.966081] Identity: 04b4:1120.0000
[ 52.966106] PD TX, header: 0x196f
[ 52.969875] PD TX complete, status: 0
[ 52.972298] PD RX, header: 0x244f [1]
[ 52.972304] Rx VDM cmd 0xff008042 type 1 cmd 2 len 2
[ 52.972308] SVID 1: 0xff01
[ 52.972340] PD TX, header: 0x1b6f
[ 52.976066] PD TX complete, status: 0
[ 52.986511] PD RX, header: 0x264f [1]
[ 52.986516] Rx VDM cmd 0xff018043 type 1 cmd 3 len 2
[ 52.986520] Alternate mode 0: SVID 0xff01, VDO 1: 0x00000405

2019-04-10 18:44:13

by Adam Thomson

Subject: RE: [PATCH v2] usb: typec: tcpm: collision avoidance

On 10 April 2019 16:45, Hans de Goede wrote:

> Hi Kyle,
>
> On 10-04-19 14:49, Kyle Tso wrote:
> > On Wed, Apr 10, 2019 at 6:32 PM Adam Thomson
> > <[email protected]> wrote:
> >>
> >> On 09 April 2019 15:41, Hans de Goede wrote:
> >>
> >>> Hi,
> >>>
> >>> On 09-04-19 15:06, Heikki Krogerus wrote:
> >>>> On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote:
> >>>>> +Hans
> >>>>>
> >>>>> On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
> >>>>>> On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck <[email protected]>
> wrote:
> >>>>>>>
> >>>>>>> On 4/4/19 7:13 AM, Heikki Krogerus wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
> >>>>>>>>> This patch provides the implementation of Collision Avoidance
> >>>>>>>>> introduced in PD3.0. The start of each Atomic Message Sequence
> >>>>>>>>> (AMS) initiated by the port will be denied if the current AMS
> >>>>>>>>> is not interruptible. The Source port will set the CC to
> >>>>>>>>> SinkTxNG if it is going to initiate an AMS, and SinkTxOk otherwise.
> >>>>>>>>> Meanwhile, any AMS initiated by a Sink port will be denied in
> >>>>>>>>> TCPM if the port partner (Source) sets SinkTxNG except for
> >>>>>>>>> HARD_RESET
> >>> and SOFT_RESET.
> >>>>>>>>
> >>>>>>>> I tested this with my GDBWin which has fusb302. When I plug-in
> >>>>>>>> DisplayPort adapter, the partner device never gets registered,
> >>>>>>>> and I see steady flow of warnings from fusb302:
> >>>>>>>>
> >>>>>>>
> >>>>>>> FWIW, I made multiple attempts to review the patch. Each time I
> >>>>>>> get stuck after a while and notice that I don't understand what is going
> on.
> >>>>>>>
> >>>>>>> Maybe the state machine needs a complete overhaul. It seems to
> >>>>>>> have reached a point where it is getting too complex to
> >>>>>>> understand what is going
> >>> on.
> >>>>>>>
> >>>>>>>> [ 693.391176] Vconn is on during toggle start [ 693.391250]
> >>>>>>>> WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562
> >>>>>>>> fusb302_set_toggling+0x129/0x130 [fusb302] [ 693.400293]
> >>>>>>>> Modules
> >>> linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532
> >>> i915 typec intel_gtt intel_cht_int33fe
> >>>>>>>> [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W
> >>> 5.1.0-rc3-heikki+ #17
> >>>>>>>> [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not
> >>>>>>>> detect charger type [ 693.412278] Hardware name: Default
> >>>>>>>> string Default string/Default string, BIOS 5.11 05/25/2017 [
> >>>>>>>> 693.412283]
> >>>>>>>> Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm] [
> >>>>>>>> 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130
> >>>>>>>> [fusb302] [ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6
> >>>>>>>> c6 83 b0 01 00
> >>>>>>>> 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0
> >>>>>>>> e8 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54
> >>>>>>>> 41 89
> >>>>>>>> f4 55 53 48 8d [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS:
> >>>>>>>> 00010286 [ 693.439174] RAX: 0000000000000000 RBX:
> >>>>>>>> ffff888178080028 RCX: 0000000000000000 [ 693.442157] RDX:
> >>>>>>>> 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f [
> >>>>>>>> 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09:
> >>>>>>>> 00000000000202c0 [ 693.448100] R10: 0000010cb24a3d18 R11:
> >>>>>>>> 000000000000001e R12: ffff8881780801b0 [ 693.451086] R13:
> >>> ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040 [ 693.454060]
> FS:
> >>> 0000000000000000(0000) GS:ffff88817bb00000(0000)
> >>> knlGS:0000000000000000 [ 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0:
> 0000000080050033 [ 693.462984] CR2:
> >>> 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0 [
> >>> 693.465969] Call Trace:
> >>>>>>>> [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302] [ 693.471894]
> >>>>>>>> tcpm_ams_start+0x1b8/0x2a0 [tcpm]
> >>>>>>>
> >>>>>>> tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter
> >>>>>>> what. This causes the fusb302 code to start toggling. As such,
> >>>>>>> it may well attempt to start toggling in the wrong state.
> >>>>>>>
> >>>>>>> Guenter
> >>>>>>>
> >>>>>>
> >>>>>> I read the fusb302 spec but failed to find the statement that
> >>>>>> says it should "set toggling" when CC switches among
> default/medium/high.
> >>>>>>
> >>>>>> quot from fusb302 spec:
> >>>>>> "The FUSB302 allows the host software to change the charging
> >>>>>> current capabilities of the port through the HOST_CUR control
> >>>>>> bits. If the HOST_CUR bits are changed prior to attach, the
> >>>>>> FUSB302 automatically indicates the programmed current capability
> when a device is attached.
> >>>>>> If the current capabilities are changed after a device is
> >>>>>> attached, the FUSB302 immediately changes the CC line to the
> >>>>>> programmed capability."
> >>>>>>
> >>>>>> Is it possible to skip fusb302_set_toggling() @ line#658 if
> >>>>>> tcpm_set_cc() is called in order to switch the cc among
> >>>>>> default/medium/high of Rp ?
> >>>>
> >>>> Hans, you introduced that in commit daf81d0137a9c ("usb: typec:
> >>>> fusb302: Refactor / simplify tcpm_set_cc()"), so could you take a
> >>>> look at this.
> >>>
> >>> I do not believe that that commit introduces the
> >>> fusb302_set_toggling() as the subject of the commit says it just
> >>> refactors things, the set_toggling call was introduced by:
> >>>
> >>> commit ea3b4d5523bc8("usb: typec: fusb302: Resolve fixed power role
> >>> contract
> >>> setup")
> >>>
> >>> Before that:
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t
> >>> ree/drivers/u
> >>> sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
> >>>
> >>> tcpm_set_cc actually turned toggling off in all cases.
> >>>
> >>> I've no doubt that Adam was seeing a real problem, but I've doubted
> >>> if this was the right fix before. I even had it reverted in my tree
> >>> for a while, but since in my use-cases so far it has not caused any problems
> I've not looked into it further.
> >>
> >> From my recollection, that was the only way to generate the
> >> necessary event from
> >> fusb302 to indicate a connection, when the device was in a fixed role
> >> state (i.e. only source or only sink). Without it the driver doesn't
> >> work in these scenarios as there's no TOGDONE event generated by
> >> fusb302, so no eventual call to 'tcpm_cc_change()' to tell TCPM that
> >> something has happened and move on the state machine. Not all devices will
> be DRP so we have to account for this.
> >>
> >
> > The switch among different Rp values on CC pins comes from TCPM and
> > after the switch finishes, TCPM doesn't need to update the CC status
> > because this kind of switch won't affect the state machine.
> >
> >>>
> >>> In the mean time the code has changed quite a bit though, so making
> >>> tcpm_set_cc() behave as it did before, see:
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t
> >>> ree/drivers/u
> >>> sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
> >>>
> >>> Will require writing something from scratch based on the new code
> >>> which mimicks the behaviour of the old code; and then we also need
> >>> to fix Adam's problem on top.
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >
> > I tried to fix this with below changes and it works.
> >
> > ============================================
> > --- a/drivers/usb/typec/tcpm/fusb302.c
> > +++ b/drivers/usb/typec/tcpm/fusb302.c
> > @@ -110,6 +110,9 @@ struct fusb302_chip {
> > enum typec_cc_status cc2;
> > u32 snk_pdo[PDO_MAX_OBJECTS];
> >
> > + /* Local pin status */
> > + enum typec_cc_status cc;
> > +
> > #ifdef CONFIG_DEBUG_FS
> > struct dentry *dentry;
> > /* lock for log buffer access */ @@ -611,6 +614,19 @@ static
> > int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
> > enum toggling_mode mode;
> >
> > mutex_lock(&chip->lock);
> > + if ((chip->cc == TYPEC_CC_RP_DEF || chip->cc == TYPEC_CC_RP_1_5 ||
> > + chip->cc == TYPEC_CC_RP_3_0) && (cc == TYPEC_CC_RP_DEF ||
> > + cc == TYPEC_CC_RP_1_5 || cc == TYPEC_CC_RP_3_0)) {
> > + ret = fusb302_set_src_current(chip, cc_src_current[cc]);
> > + if (ret < 0) {
> > + fusb302_log(chip, "cannot set src current %s, ret=%d\n",
> > + typec_cc_status_name[cc], ret);
> > + goto done;
> > + }
> > + fusb302_log(chip, "cc := %s", typec_cc_status_name[cc]);
> > + goto rp_switch;
> > + }
> > +
> > switch (cc) {
> > case TYPEC_CC_OPEN:
> > mode = TOGGLING_MODE_OFF; @@ -659,6 +675,8 @@ static
> > int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
> > if (ret < 0)
> > fusb302_log(chip, "cannot set toggling mode, ret=%d",
> > ret);
> >
> > +rp_switch:
> > + chip->cc = cc;
> > done:
> > mutex_unlock(&chip->lock);
>
>
> I understand what you are trying to do here and I agree that just changing the Cc
> pins this way should not start toggling. But I would rather go back to the
> functionality of tcpm_set_cc() from before commit ea3b4d5523bc8("usb: typec:
> fusb302: Resolve fixed power role contract setup")
>
> Starting toggling from tcpm_set_cc() just feels wrong; and currently power role
> swapping is broken with the fusb302, which IIRC used to work. I suspect this is
> related.
>
> I plan to write a patch tomorrow to functionally take tcpm_set_cc() back to the
> way it was before. This should fix your case and I hope this also fixes power-role
> swapping.
>
> This will re-introduce Adam Thomson's problem, but I have a feeling that that
> actually needs a fix in the tcpm.c code rather then at the fusb302 level.

To be clear here, the names TOGGLING_MODE_SNK and TOGGLING_MODE_SRC are a
misnomer from the HW spec for fusb302. The device isn't toggling anything as far
as I'm aware, so I don't necessarily agree with your point. It's a mechanism to
have the HW report when the CC line changes on connection. Without that we have
no reporting from the HW for the fixed role scenarios. I'm also not 100%
convinced yet that this is something to resolve in TCPM as the reporting
mechanism is there to kick-on the TCPM state machine. It just needs the device
driver to know when to do it, hence the reason for my change.

Think maybe this needs a little more consideration before breaking something
to fix something else. I would be interested to know though if you roll-back
does fix PR swap.

>
> Anyways first let me attempt to write the promised patch and then we will see
> from there.
>
> Regards,
>
> Hans

2019-04-10 18:50:54

by Hans de Goede

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

Hi,

On 10-04-19 18:14, Adam Thomson wrote:
> On 10 April 2019 16:45, Hans de Goede wrote:
>
>> Hi Kyle,
>>
>> On 10-04-19 14:49, Kyle Tso wrote:
>>> On Wed, Apr 10, 2019 at 6:32 PM Adam Thomson
>>> <[email protected]> wrote:
>>>>
>>>> On 09 April 2019 15:41, Hans de Goede wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> On 09-04-19 15:06, Heikki Krogerus wrote:
>>>>>> On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote:
>>>>>>> +Hans
>>>>>>>
>>>>>>> On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
>>>>>>>> On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck <[email protected]>
>> wrote:
>>>>>>>>>
>>>>>>>>> On 4/4/19 7:13 AM, Heikki Krogerus wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
>>>>>>>>>>> This patch provides the implementation of Collision Avoidance
>>>>>>>>>>> introduced in PD3.0. The start of each Atomic Message Sequence
>>>>>>>>>>> (AMS) initiated by the port will be denied if the current AMS
>>>>>>>>>>> is not interruptible. The Source port will set the CC to
>>>>>>>>>>> SinkTxNG if it is going to initiate an AMS, and SinkTxOk otherwise.
>>>>>>>>>>> Meanwhile, any AMS initiated by a Sink port will be denied in
>>>>>>>>>>> TCPM if the port partner (Source) sets SinkTxNG except for
>>>>>>>>>>> HARD_RESET
>>>>> and SOFT_RESET.
>>>>>>>>>>
>>>>>>>>>> I tested this with my GDBWin which has fusb302. When I plug-in
>>>>>>>>>> DisplayPort adapter, the partner device never gets registered,
>>>>>>>>>> and I see steady flow of warnings from fusb302:
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> FWIW, I made multiple attempts to review the patch. Each time I
>>>>>>>>> get stuck after a while and notice that I don't understand what is going
>> on.
>>>>>>>>>
>>>>>>>>> Maybe the state machine needs a complete overhaul. It seems to
>>>>>>>>> have reached a point where it is getting too complex to
>>>>>>>>> understand what is going
>>>>> on.
>>>>>>>>>
>>>>>>>>>> [ 693.391176] Vconn is on during toggle start [ 693.391250]
>>>>>>>>>> WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562
>>>>>>>>>> fusb302_set_toggling+0x129/0x130 [fusb302] [ 693.400293]
>>>>>>>>>> Modules
>>>>> linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532
>>>>> i915 typec intel_gtt intel_cht_int33fe
>>>>>>>>>> [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W
>>>>> 5.1.0-rc3-heikki+ #17
>>>>>>>>>> [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not
>>>>>>>>>> detect charger type [ 693.412278] Hardware name: Default
>>>>>>>>>> string Default string/Default string, BIOS 5.11 05/25/2017 [
>>>>>>>>>> 693.412283]
>>>>>>>>>> Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm] [
>>>>>>>>>> 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130
>>>>>>>>>> [fusb302] [ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6
>>>>>>>>>> c6 83 b0 01 00
>>>>>>>>>> 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0
>>>>>>>>>> e8 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54
>>>>>>>>>> 41 89
>>>>>>>>>> f4 55 53 48 8d [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS:
>>>>>>>>>> 00010286 [ 693.439174] RAX: 0000000000000000 RBX:
>>>>>>>>>> ffff888178080028 RCX: 0000000000000000 [ 693.442157] RDX:
>>>>>>>>>> 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f [
>>>>>>>>>> 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09:
>>>>>>>>>> 00000000000202c0 [ 693.448100] R10: 0000010cb24a3d18 R11:
>>>>>>>>>> 000000000000001e R12: ffff8881780801b0 [ 693.451086] R13:
>>>>> ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040 [ 693.454060]
>> FS:
>>>>> 0000000000000000(0000) GS:ffff88817bb00000(0000)
>>>>> knlGS:0000000000000000 [ 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0:
>> 0000000080050033 [ 693.462984] CR2:
>>>>> 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0 [
>>>>> 693.465969] Call Trace:
>>>>>>>>>> [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302] [ 693.471894]
>>>>>>>>>> tcpm_ams_start+0x1b8/0x2a0 [tcpm]
>>>>>>>>>
>>>>>>>>> tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter
>>>>>>>>> what. This causes the fusb302 code to start toggling. As such,
>>>>>>>>> it may well attempt to start toggling in the wrong state.
>>>>>>>>>
>>>>>>>>> Guenter
>>>>>>>>>
>>>>>>>>
>>>>>>>> I read the fusb302 spec but failed to find the statement that
>>>>>>>> says it should "set toggling" when CC switches among
>> default/medium/high.
>>>>>>>>
>>>>>>>> quot from fusb302 spec:
>>>>>>>> "The FUSB302 allows the host software to change the charging
>>>>>>>> current capabilities of the port through the HOST_CUR control
>>>>>>>> bits. If the HOST_CUR bits are changed prior to attach, the
>>>>>>>> FUSB302 automatically indicates the programmed current capability
>> when a device is attached.
>>>>>>>> If the current capabilities are changed after a device is
>>>>>>>> attached, the FUSB302 immediately changes the CC line to the
>>>>>>>> programmed capability."
>>>>>>>>
>>>>>>>> Is it possible to skip fusb302_set_toggling() @ line#658 if
>>>>>>>> tcpm_set_cc() is called in order to switch the cc among
>>>>>>>> default/medium/high of Rp ?
>>>>>>
>>>>>> Hans, you introduced that in commit daf81d0137a9c ("usb: typec:
>>>>>> fusb302: Refactor / simplify tcpm_set_cc()"), so could you take a
>>>>>> look at this.
>>>>>
>>>>> I do not believe that that commit introduces the
>>>>> fusb302_set_toggling() as the subject of the commit says it just
>>>>> refactors things, the set_toggling call was introduced by:
>>>>>
>>>>> commit ea3b4d5523bc8("usb: typec: fusb302: Resolve fixed power role
>>>>> contract
>>>>> setup")
>>>>>
>>>>> Before that:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t
>>>>> ree/drivers/u
>>>>> sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
>>>>>
>>>>> tcpm_set_cc actually turned toggling off in all cases.
>>>>>
>>>>> I've no doubt that Adam was seeing a real problem, but I've doubted
>>>>> if this was the right fix before. I even had it reverted in my tree
>>>>> for a while, but since in my use-cases so far it has not caused any problems
>> I've not looked into it further.
>>>>
>>>> From my recollection, that was the only way to generate the
>>>> necessary event from
>>>> fusb302 to indicate a connection, when the device was in a fixed role
>>>> state (i.e. only source or only sink). Without it the driver doesn't
>>>> work in these scenarios as there's no TOGDONE event generated by
>>>> fusb302, so no eventual call to 'tcpm_cc_change()' to tell TCPM that
>>>> something has happened and move on the state machine. Not all devices will
>> be DRP so we have to account for this.
>>>>
>>>
>>> The switch among different Rp values on CC pins comes from TCPM and
>>> after the switch finishes, TCPM doesn't need to update the CC status
>>> because this kind of switch won't affect the state machine.
>>>
>>>>>
>>>>> In the mean time the code has changed quite a bit though, so making
>>>>> tcpm_set_cc() behave as it did before, see:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t
>>>>> ree/drivers/u
>>>>> sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
>>>>>
>>>>> Will require writing something from scratch based on the new code
>>>>> which mimicks the behaviour of the old code; and then we also need
>>>>> to fix Adam's problem on top.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Hans
>>>
>>> I tried to fix this with below changes and it works.
>>>
>>> ============================================
>>> --- a/drivers/usb/typec/tcpm/fusb302.c
>>> +++ b/drivers/usb/typec/tcpm/fusb302.c
>>> @@ -110,6 +110,9 @@ struct fusb302_chip {
>>> enum typec_cc_status cc2;
>>> u32 snk_pdo[PDO_MAX_OBJECTS];
>>>
>>> + /* Local pin status */
>>> + enum typec_cc_status cc;
>>> +
>>> #ifdef CONFIG_DEBUG_FS
>>> struct dentry *dentry;
>>> /* lock for log buffer access */ @@ -611,6 +614,19 @@ static
>>> int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
>>> enum toggling_mode mode;
>>>
>>> mutex_lock(&chip->lock);
>>> + if ((chip->cc == TYPEC_CC_RP_DEF || chip->cc == TYPEC_CC_RP_1_5 ||
>>> + chip->cc == TYPEC_CC_RP_3_0) && (cc == TYPEC_CC_RP_DEF ||
>>> + cc == TYPEC_CC_RP_1_5 || cc == TYPEC_CC_RP_3_0)) {
>>> + ret = fusb302_set_src_current(chip, cc_src_current[cc]);
>>> + if (ret < 0) {
>>> + fusb302_log(chip, "cannot set src current %s, ret=%d\n",
>>> + typec_cc_status_name[cc], ret);
>>> + goto done;
>>> + }
>>> + fusb302_log(chip, "cc := %s", typec_cc_status_name[cc]);
>>> + goto rp_switch;
>>> + }
>>> +
>>> switch (cc) {
>>> case TYPEC_CC_OPEN:
>>> mode = TOGGLING_MODE_OFF; @@ -659,6 +675,8 @@ static
>>> int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
>>> if (ret < 0)
>>> fusb302_log(chip, "cannot set toggling mode, ret=%d",
>>> ret);
>>>
>>> +rp_switch:
>>> + chip->cc = cc;
>>> done:
>>> mutex_unlock(&chip->lock);
>>
>>
>> I understand what you are trying to do here and I agree that just changing the Cc
>> pins this way should not start toggling. But I would rather go back to the
>> functionality of tcpm_set_cc() from before commit ea3b4d5523bc8("usb: typec:
>> fusb302: Resolve fixed power role contract setup")
>>
>> Starting toggling from tcpm_set_cc() just feels wrong; and currently power role
>> swapping is broken with the fusb302, which IIRC used to work. I suspect this is
>> related.
>>
>> I plan to write a patch tomorrow to functionally take tcpm_set_cc() back to the
>> way it was before. This should fix your case and I hope this also fixes power-role
>> swapping.
>>
>> This will re-introduce Adam Thomson's problem, but I have a feeling that that
>> actually needs a fix in the tcpm.c code rather then at the fusb302 level.
>
> To be clear here, the names TOGGLING_MODE_SNK and TOGGLING_MODE_SRC are a
> misnomer from the HW spec for fusb302. The device isn't toggling anything as far
> as I'm aware, so I don't necessarily agree with your point.

If I understand the datasheet correctly:

"The FUSB302 has the capability to do autonomous
DRP toggle. In autonomous toggle the FUSB302
internally controls the PDWN1, PDWN2, PU_EN1 and
PU_EN2, MEAS_CC1 and MEAS_CC2 and implements
a fixed DRP toggle between presenting as a SRC and
presenting as a SNK. Alternately, it can present as a
SRC or SNK only and poll CC1 and CC2 continuously."

It is still attaching Rp resp Rd to CC1 or CC2 one at a time
to detect polarity, so it is still toggling, it just is not
doing dual-role toggling. This is also expected behavior for
a sink, a sink may not present Rd on both CC pins at the
same time, otherwise the source cannot detect the polarity
and the source also cannot detect if Vconn is necessary.

> It's a mechanism to
> have the HW report when the CC line changes on connection. Without that we have
> no reporting from the HW for the fixed role scenarios.

Not just connection, also polarity detection. Notice that
the tcpm framework / the driver also has a start_drp_toggling()
method. I think we may also need a start_srp_toggling function
just like it and call that from the SNK_UNATTACHED and
SRC_UNATTACHED states for single-role ports. I agree that we
need to start toggling when in those states, but tcpm_set_cc gets
called in a lot of other places where AFAIK we should NOT
restart toggling and your patch causes us to restart
toggling in those cases.

> I'm also not 100%
> convinced yet that this is something to resolve in TCPM as the reporting
> mechanism is there to kick-on the TCPM state machine. It just needs the device
> driver to know when to do it, hence the reason for my change.
>
> Think maybe this needs a little more consideration before breaking something
> to fix something else.

It is not my intention for the patch I plan to write to go
upstream as is, knowing that it will break your use-case.

Worst case we end up having 2 patches where your use-case
is broken in the intermediate state. But if we end up adding
a start_srp_toggling function as I suspect we may; then the
commit adding that may even be ordered before the other one,
so we can even avoid the broken intermediate state.

Regards,

Hans

2019-04-10 18:52:01

by Hans de Goede

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

> If I understand the datasheet correctly:
>
> "The FUSB302 has the capability to do autonomous
> DRP toggle. In autonomous toggle the FUSB302
> internally controls the PDWN1, PDWN2, PU_EN1 and
> PU_EN2, MEAS_CC1 and MEAS_CC2 and implements
> a fixed DRP toggle between presenting as a SRC and
> presenting as a SNK. Alternately, it can present as a
> SRC or SNK only and poll CC1 and CC2 continuously."
>
> It is still attaching Rp resp Rd to CC1 or CC2 one at a time
> to detect polarity, so it is still toggling, it just is not
> doing dual-role toggling. This is also expected behavior for
> a sink, a sink may not present Rd on both CC pins at the
> same time, otherwise the source cannot detect the polarity
> and the source also cannot detect if Vconn is necessary.

Erm the "a sink may not present Rd on both CC pins at the same time"
bit obviously is wrong, ignore that please.

2019-04-10 18:57:17

by Hans de Goede

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

Hi Kyle,

On 10-04-19 14:49, Kyle Tso wrote:
> On Wed, Apr 10, 2019 at 6:32 PM Adam Thomson
> <[email protected]> wrote:
>>
>> On 09 April 2019 15:41, Hans de Goede wrote:
>>
>>> Hi,
>>>
>>> On 09-04-19 15:06, Heikki Krogerus wrote:
>>>> On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote:
>>>>> +Hans
>>>>>
>>>>> On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
>>>>>> On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck <[email protected]> wrote:
>>>>>>>
>>>>>>> On 4/4/19 7:13 AM, Heikki Krogerus wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
>>>>>>>>> This patch provides the implementation of Collision Avoidance
>>>>>>>>> introduced in PD3.0. The start of each Atomic Message Sequence
>>>>>>>>> (AMS) initiated by the port will be denied if the current AMS is
>>>>>>>>> not interruptible. The Source port will set the CC to SinkTxNG if
>>>>>>>>> it is going to initiate an AMS, and SinkTxOk otherwise.
>>>>>>>>> Meanwhile, any AMS initiated by a Sink port will be denied in
>>>>>>>>> TCPM if the port partner (Source) sets SinkTxNG except for HARD_RESET
>>> and SOFT_RESET.
>>>>>>>>
>>>>>>>> I tested this with my GDBWin which has fusb302. When I plug-in
>>>>>>>> DisplayPort adapter, the partner device never gets registered, and
>>>>>>>> I see steady flow of warnings from fusb302:
>>>>>>>>
>>>>>>>
>>>>>>> FWIW, I made multiple attempts to review the patch. Each time I get
>>>>>>> stuck after a while and notice that I don't understand what is going on.
>>>>>>>
>>>>>>> Maybe the state machine needs a complete overhaul. It seems to have
>>>>>>> reached a point where it is getting too complex to understand what is going
>>> on.
>>>>>>>
>>>>>>>> [ 693.391176] Vconn is on during toggle start [ 693.391250]
>>>>>>>> WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562
>>>>>>>> fusb302_set_toggling+0x129/0x130 [fusb302] [ 693.400293] Modules
>>> linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532 i915 typec
>>> intel_gtt intel_cht_int33fe
>>>>>>>> [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W
>>> 5.1.0-rc3-heikki+ #17
>>>>>>>> [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not detect
>>>>>>>> charger type [ 693.412278] Hardware name: Default string Default
>>>>>>>> string/Default string, BIOS 5.11 05/25/2017 [ 693.412283]
>>>>>>>> Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm] [
>>>>>>>> 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 [fusb302] [
>>>>>>>> 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6 c6 83 b0 01 00
>>>>>>>> 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0 e8
>>>>>>>> 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54 41 89
>>>>>>>> f4 55 53 48 8d [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS:
>>>>>>>> 00010286 [ 693.439174] RAX: 0000000000000000 RBX:
>>>>>>>> ffff888178080028 RCX: 0000000000000000 [ 693.442157] RDX:
>>>>>>>> 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f [
>>>>>>>> 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09:
>>>>>>>> 00000000000202c0 [ 693.448100] R10: 0000010cb24a3d18 R11:
>>>>>>>> 000000000000001e R12: ffff8881780801b0 [ 693.451086] R13:
>>> ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040 [ 693.454060] FS:
>>> 0000000000000000(0000) GS:ffff88817bb00000(0000) knlGS:0000000000000000 [
>>> 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 693.462984] CR2:
>>> 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0 [ 693.465969]
>>> Call Trace:
>>>>>>>> [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302] [ 693.471894]
>>>>>>>> tcpm_ams_start+0x1b8/0x2a0 [tcpm]
>>>>>>>
>>>>>>> tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter
>>>>>>> what. This causes the fusb302 code to start toggling. As such, it
>>>>>>> may well attempt to start toggling in the wrong state.
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>>
>>>>>> I read the fusb302 spec but failed to find the statement that says
>>>>>> it should "set toggling" when CC switches among default/medium/high.
>>>>>>
>>>>>> quot from fusb302 spec:
>>>>>> "The FUSB302 allows the host software to change the charging current
>>>>>> capabilities of the port through the HOST_CUR control bits. If the
>>>>>> HOST_CUR bits are changed prior to attach, the FUSB302 automatically
>>>>>> indicates the programmed current capability when a device is attached.
>>>>>> If the current capabilities are changed after a device is attached,
>>>>>> the FUSB302 immediately changes the CC line to the programmed
>>>>>> capability."
>>>>>>
>>>>>> Is it possible to skip fusb302_set_toggling() @ line#658 if
>>>>>> tcpm_set_cc() is called in order to switch the cc among
>>>>>> default/medium/high of Rp ?
>>>>
>>>> Hans, you introduced that in commit daf81d0137a9c ("usb: typec:
>>>> fusb302: Refactor / simplify tcpm_set_cc()"), so could you take a look
>>>> at this.
>>>
>>> I do not believe that that commit introduces the fusb302_set_toggling() as the
>>> subject of the commit says it just refactors things, the set_toggling call was
>>> introduced by:
>>>
>>> commit ea3b4d5523bc8("usb: typec: fusb302: Resolve fixed power role contract
>>> setup")
>>>
>>> Before that:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/u
>>> sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
>>>
>>> tcpm_set_cc actually turned toggling off in all cases.
>>>
>>> I've no doubt that Adam was seeing a real problem, but I've doubted if this was
>>> the right fix before. I even had it reverted in my tree for a while, but since in my
>>> use-cases so far it has not caused any problems I've not looked into it further.
>>
>> From my recollection, that was the only way to generate the necessary event from
>> fusb302 to indicate a connection, when the device was in a fixed role state
>> (i.e. only source or only sink). Without it the driver doesn't work in these
>> scenarios as there's no TOGDONE event generated by fusb302, so no eventual call
>> to 'tcpm_cc_change()' to tell TCPM that something has happened and move on the
>> state machine. Not all devices will be DRP so we have to account for this.
>>
>
> The switch among different Rp values on CC pins comes from TCPM and
> after the switch finishes, TCPM doesn't need to update the CC status because
> this kind of switch won't affect the state machine.
>
>>>
>>> In the mean time the code has changed quite a bit though, so making
>>> tcpm_set_cc() behave as it did before, see:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/u
>>> sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
>>>
>>> Will require writing something from scratch based on the new code which
>>> mimicks the behaviour of the old code; and then we also need to fix Adam's
>>> problem on top.
>>>
>>> Regards,
>>>
>>> Hans
>
> I tried to fix this with below changes and it works.
>
> ============================================
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -110,6 +110,9 @@ struct fusb302_chip {
> enum typec_cc_status cc2;
> u32 snk_pdo[PDO_MAX_OBJECTS];
>
> + /* Local pin status */
> + enum typec_cc_status cc;
> +
> #ifdef CONFIG_DEBUG_FS
> struct dentry *dentry;
> /* lock for log buffer access */
> @@ -611,6 +614,19 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum
> typec_cc_status cc)
> enum toggling_mode mode;
>
> mutex_lock(&chip->lock);
> + if ((chip->cc == TYPEC_CC_RP_DEF || chip->cc == TYPEC_CC_RP_1_5 ||
> + chip->cc == TYPEC_CC_RP_3_0) && (cc == TYPEC_CC_RP_DEF ||
> + cc == TYPEC_CC_RP_1_5 || cc == TYPEC_CC_RP_3_0)) {
> + ret = fusb302_set_src_current(chip, cc_src_current[cc]);
> + if (ret < 0) {
> + fusb302_log(chip, "cannot set src current %s, ret=%d\n",
> + typec_cc_status_name[cc], ret);
> + goto done;
> + }
> + fusb302_log(chip, "cc := %s", typec_cc_status_name[cc]);
> + goto rp_switch;
> + }
> +
> switch (cc) {
> case TYPEC_CC_OPEN:
> mode = TOGGLING_MODE_OFF;
> @@ -659,6 +675,8 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum
> typec_cc_status cc)
> if (ret < 0)
> fusb302_log(chip, "cannot set toggling mode, ret=%d", ret);
>
> +rp_switch:
> + chip->cc = cc;
> done:
> mutex_unlock(&chip->lock);


I understand what you are trying to do here and I agree that just
changing the Cc pins this way should not start toggling. But I
would rather go back to the functionality of tcpm_set_cc() from
before commit ea3b4d5523bc8("usb: typec: fusb302: Resolve fixed power role contract setup")

Starting toggling from tcpm_set_cc() just feels wrong; and currently
power role swapping is broken with the fusb302, which IIRC used to
work. I suspect this is related.

I plan to write a patch tomorrow to functionally take tcpm_set_cc()
back to the way it was before. This should fix your case and
I hope this also fixes power-role swapping.

This will re-introduce Adam Thomson's problem, but I have a feeling
that that actually needs a fix in the tcpm.c code rather then at
the fusb302 level.

Anyways first let me attempt to write the promised patch and then
we will see from there.

Regards,

Hans





> ============================================
>
> TCPM logs:
>
> [ 52.267343] CC1: 0 -> 1, CC2: 0 -> 2 [state DRP_TOGGLING, polarity
> 0, connected]
> [ 52.267354] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
> [ 52.267435] pending state change SRC_ATTACH_WAIT -> SNK_TRY @ 200 ms
> [ 52.466852] state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
> [ 52.466876] cc:=2
> [ 52.471851] pending state change SNK_TRY -> SNK_TRY_WAIT @ 140 ms
> [ 52.610244] state change SNK_TRY -> SNK_TRY_WAIT [delayed 140 ms]
> [ 52.610256] state change SNK_TRY_WAIT -> SRC_TRYWAIT
> [ 52.610298] cc:=3
> [ 52.613262] pending state change SRC_TRYWAIT ->
> SRC_TRYWAIT_UNATTACHED @ 140 ms
> [ 52.625248] CC1: 1 -> 1, CC2: 2 -> 2 [state SRC_TRYWAIT, polarity
> 0, connected]
> [ 52.625261] state change SRC_TRYWAIT -> SRC_TRYWAIT_DEBOUNCE
> [ 52.625299] pending state change SRC_TRYWAIT_DEBOUNCE ->
> SRC_ATTACHED @ 200 ms
> [ 52.823417] state change SRC_TRYWAIT_DEBOUNCE -> SRC_ATTACHED
> [delayed 200 ms]
> [ 52.823425] polarity 1
> [ 52.823458] Requesting mux state 1, usb-role 1, orientation 2
> [ 52.846919] vconn:=1
> [ 52.854536] vbus:=1 charge=0
> [ 52.893437] pending state change SRC_ATTACHED -> SRC_UNATTACHED @ 480 ms
> [ 52.894715] VBUS on
> [ 52.894718] state change SRC_ATTACHED -> SRC_STARTUP
> [ 52.894733] AMS POWER_NEGOTIATION start
> [ 52.894736] cc:=4
> [ 52.895043] state change SRC_STARTUP -> AMS_START in AMS POWER_NEGOTIATION
> [ 52.895046] state change AMS_START -> SRC_SEND_CAPABILITIES in AMS
> POWER_NEGOTIATION
> [ 52.895048] PD TX, header: 0x11a1
> [ 52.910309] PD TX complete, status: 0
> [ 52.910346] pending state change SRC_SEND_CAPABILITIES ->
> HARD_RESET_SEND @ 150 ms in AMS POWER_NEGOTIATION
> [ 52.912383] PD RX, header: 0x1042 [1]
> [ 52.912387] state change SRC_SEND_CAPABILITIES -> SRC_NEGOTIATE_CAPABILITIES
> [ 52.912398] Requested 5000 mV, 900 mA for 900 / 900 mA
> [ 52.912400] PD TX, header: 0x363
> [ 52.915725] PD TX complete, status: 0
> [ 52.915765] pending state change SRC_NEGOTIATE_CAPABILITIES ->
> SRC_TRANSITION_SUPPLY @ 35 ms
> [ 52.950125] state change SRC_NEGOTIATE_CAPABILITIES ->
> SRC_TRANSITION_SUPPLY [delayed 35 ms]
> [ 52.950139] PD TX, header: 0x566
> [ 52.953910] PD TX complete, status: 0
> [ 52.955573] state change SRC_TRANSITION_SUPPLY -> SRC_READY
> [ 52.957359] PD TX, header: 0x176f
> [ 52.962785] PD TX complete, status: 0
> [ 52.966026] PD RX, header: 0x524f [1]
> [ 52.966037] Rx VDM cmd 0xff008041 type 1 cmd 1 len 5
> [ 52.966081] Identity: 04b4:1120.0000
> [ 52.966106] PD TX, header: 0x196f
> [ 52.969875] PD TX complete, status: 0
> [ 52.972298] PD RX, header: 0x244f [1]
> [ 52.972304] Rx VDM cmd 0xff008042 type 1 cmd 2 len 2
> [ 52.972308] SVID 1: 0xff01
> [ 52.972340] PD TX, header: 0x1b6f
> [ 52.976066] PD TX complete, status: 0
> [ 52.986511] PD RX, header: 0x264f [1]
> [ 52.986516] Rx VDM cmd 0xff018043 type 1 cmd 3 len 2
> [ 52.986520] Alternate mode 0: SVID 0xff01, VDO 1: 0x00000405
>

2019-04-11 09:08:02

by Adam Thomson

Subject: RE: [PATCH v2] usb: typec: tcpm: collision avoidance

On 10 April 2019 17:39, Hans de Goede wrote:

> On 10-04-19 18:14, Adam Thomson wrote:
> > On 10 April 2019 16:45, Hans de Goede wrote:
> >
> >> Hi Kyle,
> >>
> >> On 10-04-19 14:49, Kyle Tso wrote:
> >>> On Wed, Apr 10, 2019 at 6:32 PM Adam Thomson
> >>> <[email protected]> wrote:
> >>>>
> >>>> On 09 April 2019 15:41, Hans de Goede wrote:
> >>>>
> >>>>> Hi,
> >>>>>
> >>>>> On 09-04-19 15:06, Heikki Krogerus wrote:
> >>>>>> On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote:
> >>>>>>> +Hans
> >>>>>>>
> >>>>>>> On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
> >>>>>>>> On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck
> >>>>>>>> <[email protected]>
> >> wrote:
> >>>>>>>>>
> >>>>>>>>> On 4/4/19 7:13 AM, Heikki Krogerus wrote:
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
> >>>>>>>>>>> This patch provides the implementation of Collision
> >>>>>>>>>>> Avoidance introduced in PD3.0. The start of each Atomic
> >>>>>>>>>>> Message Sequence
> >>>>>>>>>>> (AMS) initiated by the port will be denied if the current
> >>>>>>>>>>> AMS is not interruptible. The Source port will set the CC to
> >>>>>>>>>>> SinkTxNG if it is going to initiate an AMS, and SinkTxOk otherwise.
> >>>>>>>>>>> Meanwhile, any AMS initiated by a Sink port will be denied
> >>>>>>>>>>> in TCPM if the port partner (Source) sets SinkTxNG except
> >>>>>>>>>>> for HARD_RESET
> >>>>> and SOFT_RESET.
> >>>>>>>>>>
> >>>>>>>>>> I tested this with my GDBWin which has fusb302. When I
> >>>>>>>>>> plug-in DisplayPort adapter, the partner device never gets
> >>>>>>>>>> registered, and I see steady flow of warnings from fusb302:
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> FWIW, I made multiple attempts to review the patch. Each time
> >>>>>>>>> I get stuck after a while and notice that I don't understand
> >>>>>>>>> what is going
> >> on.
> >>>>>>>>>
> >>>>>>>>> Maybe the state machine needs a complete overhaul. It seems to
> >>>>>>>>> have reached a point where it is getting too complex to
> >>>>>>>>> understand what is going
> >>>>> on.
> >>>>>>>>>
> >>>>>>>>>> [ 693.391176] Vconn is on during toggle start [ 693.391250]
> >>>>>>>>>> WARNING: CPU: 2 PID: 30 at
> >>>>>>>>>> drivers/usb/typec/tcpm/fusb302.c:562
> >>>>>>>>>> fusb302_set_toggling+0x129/0x130 [fusb302] [ 693.400293]
> >>>>>>>>>> Modules
> >>>>> linked in: intel_xhci_usb_role_switch fusb302 tcpm roles
> >>>>> pi3usb30532
> >>>>> i915 typec intel_gtt intel_cht_int33fe
> >>>>>>>>>> [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W
> >>>>> 5.1.0-rc3-heikki+ #17
> >>>>>>>>>> [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not
> >>>>>>>>>> detect charger type [ 693.412278] Hardware name: Default
> >>>>>>>>>> string Default string/Default string, BIOS 5.11 05/25/2017 [
> >>>>>>>>>> 693.412283]
> >>>>>>>>>> Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm] [
> >>>>>>>>>> 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130
> >>>>>>>>>> [fusb302] [ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78
> >>>>>>>>>> c6
> >>>>>>>>>> c6 83 b0 01 00
> >>>>>>>>>> 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21
> >>>>>>>>>> a0
> >>>>>>>>>> e8 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54
> >>>>>>>>>> 41 89
> >>>>>>>>>> f4 55 53 48 8d [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS:
> >>>>>>>>>> 00010286 [ 693.439174] RAX: 0000000000000000 RBX:
> >>>>>>>>>> ffff888178080028 RCX: 0000000000000000 [ 693.442157] RDX:
> >>>>>>>>>> 000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f
> >>>>>>>>>> [ 693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09:
> >>>>>>>>>> 00000000000202c0 [ 693.448100] R10: 0000010cb24a3d18 R11:
> >>>>>>>>>> 000000000000001e R12: ffff8881780801b0 [ 693.451086] R13:
> >>>>> ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040 [
> >>>>> 693.454060]
> >> FS:
> >>>>> 0000000000000000(0000) GS:ffff88817bb00000(0000)
> >>>>> knlGS:0000000000000000 [ 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0:
> >> 0000000080050033 [ 693.462984] CR2:
> >>>>> 00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0 [
> >>>>> 693.465969] Call Trace:
> >>>>>>>>>> [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302] [
> >>>>>>>>>> 693.471894]
> >>>>>>>>>> tcpm_ams_start+0x1b8/0x2a0 [tcpm]
> >>>>>>>>>
> >>>>>>>>> tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no
> >>>>>>>>> matter what. This causes the fusb302 code to start toggling.
> >>>>>>>>> As such, it may well attempt to start toggling in the wrong state.
> >>>>>>>>>
> >>>>>>>>> Guenter
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> I read the fusb302 spec but failed to find the statement that
> >>>>>>>> says it should "set toggling" when CC switches among
> >> default/medium/high.
> >>>>>>>>
> >>>>>>>> quot from fusb302 spec:
> >>>>>>>> "The FUSB302 allows the host software to change the charging
> >>>>>>>> current capabilities of the port through the HOST_CUR control
> >>>>>>>> bits. If the HOST_CUR bits are changed prior to attach, the
> >>>>>>>> FUSB302 automatically indicates the programmed current
> >>>>>>>> capability
> >> when a device is attached.
> >>>>>>>> If the current capabilities are changed after a device is
> >>>>>>>> attached, the FUSB302 immediately changes the CC line to the
> >>>>>>>> programmed capability."
> >>>>>>>>
> >>>>>>>> Is it possible to skip fusb302_set_toggling() @ line#658 if
> >>>>>>>> tcpm_set_cc() is called in order to switch the cc among
> >>>>>>>> default/medium/high of Rp ?
> >>>>>>
> >>>>>> Hans, you introduced that in commit daf81d0137a9c ("usb: typec:
> >>>>>> fusb302: Refactor / simplify tcpm_set_cc()"), so could you take a
> >>>>>> look at this.
> >>>>>
> >>>>> I do not believe that that commit introduces the
> >>>>> fusb302_set_toggling() as the subject of the commit says it just
> >>>>> refactors things, the set_toggling call was introduced by:
> >>>>>
> >>>>> commit ea3b4d5523bc8("usb: typec: fusb302: Resolve fixed power
> >>>>> role contract
> >>>>> setup")
> >>>>>
> >>>>> Before that:
> >>>>>
> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> >>>>> /t
> >>>>> ree/drivers/u
> >>>>> sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca56
> >>>>> 4
> >>>>>
> >>>>> tcpm_set_cc actually turned toggling off in all cases.
> >>>>>
> >>>>> I've no doubt that Adam was seeing a real problem, but I've
> >>>>> doubted if this was the right fix before. I even had it reverted
> >>>>> in my tree for a while, but since in my use-cases so far it has
> >>>>> not caused any problems
> >> I've not looked into it further.
> >>>>
> >>>> From my recollection, that was the only way to generate the
> >>>> necessary event from
> >>>> fusb302 to indicate a connection, when the device was in a fixed
> >>>> role state (i.e. only source or only sink). Without it the driver
> >>>> doesn't work in these scenarios as there's no TOGDONE event
> >>>> generated by fusb302, so no eventual call to 'tcpm_cc_change()' to
> >>>> tell TCPM that something has happened and move on the state
> >>>> machine. Not all devices will
> >> be DRP so we have to account for this.
> >>>>
> >>>
> >>> The switch among different Rp values on CC pins comes from TCPM and
> >>> after the switch finishes, TCPM doesn't need to update the CC status
> >>> because this kind of switch won't affect the state machine.
> >>>
> >>>>>
> >>>>> In the mean time the code has changed quite a bit though, so
> >>>>> making
> >>>>> tcpm_set_cc() behave as it did before, see:
> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> >>>>> /t
> >>>>> ree/drivers/u
> >>>>> sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca56
> >>>>> 4
> >>>>>
> >>>>> Will require writing something from scratch based on the new code
> >>>>> which mimicks the behaviour of the old code; and then we also need
> >>>>> to fix Adam's problem on top.
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>> Hans
> >>>
> >>> I tried to fix this with below changes and it works.
> >>>
> >>> ============================================
> >>> --- a/drivers/usb/typec/tcpm/fusb302.c
> >>> +++ b/drivers/usb/typec/tcpm/fusb302.c
> >>> @@ -110,6 +110,9 @@ struct fusb302_chip {
> >>> enum typec_cc_status cc2;
> >>> u32 snk_pdo[PDO_MAX_OBJECTS];
> >>>
> >>> + /* Local pin status */
> >>> + enum typec_cc_status cc;
> >>> +
> >>> #ifdef CONFIG_DEBUG_FS
> >>> struct dentry *dentry;
> >>> /* lock for log buffer access */ @@ -611,6 +614,19 @@
> >>> static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
> >>> enum toggling_mode mode;
> >>>
> >>> mutex_lock(&chip->lock);
> >>> + if ((chip->cc == TYPEC_CC_RP_DEF || chip->cc == TYPEC_CC_RP_1_5 ||
> >>> + chip->cc == TYPEC_CC_RP_3_0) && (cc == TYPEC_CC_RP_DEF ||
> >>> + cc == TYPEC_CC_RP_1_5 || cc == TYPEC_CC_RP_3_0)) {
> >>> + ret = fusb302_set_src_current(chip, cc_src_current[cc]);
> >>> + if (ret < 0) {
> >>> + fusb302_log(chip, "cannot set src current %s, ret=%d\n",
> >>> + typec_cc_status_name[cc], ret);
> >>> + goto done;
> >>> + }
> >>> + fusb302_log(chip, "cc := %s", typec_cc_status_name[cc]);
> >>> + goto rp_switch;
> >>> + }
> >>> +
> >>> switch (cc) {
> >>> case TYPEC_CC_OPEN:
> >>> mode = TOGGLING_MODE_OFF; @@ -659,6 +675,8 @@
> >>> static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
> >>> if (ret < 0)
> >>> fusb302_log(chip, "cannot set toggling mode,
> >>> ret=%d", ret);
> >>>
> >>> +rp_switch:
> >>> + chip->cc = cc;
> >>> done:
> >>> mutex_unlock(&chip->lock);
> >>
> >>
> >> I understand what you are trying to do here and I agree that just
> >> changing the Cc pins this way should not start toggling. But I would
> >> rather go back to the functionality of tcpm_set_cc() from before commit
> ea3b4d5523bc8("usb: typec:
> >> fusb302: Resolve fixed power role contract setup")
> >>
> >> Starting toggling from tcpm_set_cc() just feels wrong; and currently
> >> power role swapping is broken with the fusb302, which IIRC used to
> >> work. I suspect this is related.
> >>
> >> I plan to write a patch tomorrow to functionally take tcpm_set_cc()
> >> back to the way it was before. This should fix your case and I hope
> >> this also fixes power-role swapping.
> >>
> >> This will re-introduce Adam Thomson's problem, but I have a feeling
> >> that that actually needs a fix in the tcpm.c code rather then at the fusb302
> level.
> >
> > To be clear here, the names TOGGLING_MODE_SNK and
> TOGGLING_MODE_SRC
> > are a misnomer from the HW spec for fusb302. The device isn't toggling
> > anything as far as I'm aware, so I don't necessarily agree with your point.
>
> If I understand the datasheet correctly:
>
> "The FUSB302 has the capability to do autonomous DRP toggle. In autonomous
> toggle the FUSB302 internally controls the PDWN1, PDWN2, PU_EN1 and
> PU_EN2, MEAS_CC1 and MEAS_CC2 and implements a fixed DRP toggle between
> presenting as a SRC and presenting as a SNK. Alternately, it can present as a SRC
> or SNK only and poll CC1 and CC2 continuously."
>
> It is still attaching Rp resp Rd to CC1 or CC2 one at a time to detect polarity, so it is
> still toggling, it just is not doing dual-role toggling. This is also expected behavior
> for a sink, a sink may not present Rd on both CC pins at the same time, otherwise
> the source cannot detect the polarity and the source also cannot detect if Vconn
> is necessary.

Ok, it's maybe toggling in a slightly different sense as you're not toggling
roles but rather which CC pin you measure against. With regards to Rp being
toggled between CC1 and CC2, I don't actually see that mentioned in the
datasheet. Might be missing something though.

>
> > It's a mechanism to
> > have the HW report when the CC line changes on connection. Without
> > that we have no reporting from the HW for the fixed role scenarios.
>
> Not just connection, also polarity detection. Notice that the tcpm framework /
> the driver also has a start_drp_toggling() method. I think we may also need a
> start_srp_toggling function just like it and call that from the SNK_UNATTACHED
> and SRC_UNATTACHED states for single-role ports. I agree that we need to start
> toggling when in those states, but tcpm_set_cc gets called in a lot of other places
> where AFAIK we should NOT restart toggling and your patch causes us to restart
> toggling in those cases.

Yes, ok that's true although originally almost all calls to tcpm_set_cc() seemed
to relate to handling the detached -> attached state. That obviously now would
change with Kyle's updates. However I do also see the PR Swap called into this
function so may have been affected (your tests will verify that) although for
the state machine that again then drops into a detached state. If we do add
a generic function for this I'd suggest maybe a different name as I don't think
toggling sounds right here. Might be a bit confusing given that toggling was
originally used in the scope of swapping roles.

>
> > I'm also not 100%
> > convinced yet that this is something to resolve in TCPM as the
> > reporting mechanism is there to kick-on the TCPM state machine. It
> > just needs the device driver to know when to do it, hence the reason for my
> change.
> >
> > Think maybe this needs a little more consideration before breaking
> > something to fix something else.
>
> It is not my intention for the patch I plan to write to go upstream as is, knowing
> that it will break your use-case.
>
> Worst case we end up having 2 patches where your use-case is broken in the
> intermediate state. But if we end up adding a start_srp_toggling function as I
> suspect we may; then the commit adding that may even be ordered before the
> other one, so we can even avoid the broken intermediate state.

Ok, that's fine. Thanks.

>
> Regards,
>
> Hans

2019-04-13 20:41:32

by Hans de Goede

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

Hi,

On 10-04-19 18:38, Hans de Goede wrote:
> On 10-04-19 18:14, Adam Thomson wrote:
>> On 10 April 2019 16:45, Hans de Goede wrote:

<snip>

>>> Starting toggling from tcpm_set_cc() just feels wrong; and currently power role
>>> swapping is broken with the fusb302, which IIRC used to work. I suspect this is
>>> related.
>>>
>>> I plan to write a patch tomorrow to functionally take tcpm_set_cc() back to the
>>> way it was before. This should fix your case and I hope this also fixes power-role
>>> swapping.
>>>
>>> This will re-introduce Adam Thomson's problem, but I have a feeling that that
>>> actually needs a fix in the tcpm.c code rather then at the fusb302 level.
>>
>> To be clear here, the names TOGGLING_MODE_SNK and TOGGLING_MODE_SRC are a
>> misnomer from the HW spec for fusb302. The device isn't toggling anything as far
>> as I'm aware, so I don't necessarily agree with your point.
>
> If I understand the datasheet correctly:
>
> "The FUSB302 has the capability to do autonomous
> DRP toggle. In autonomous toggle the FUSB302
> internally controls the PDWN1, PDWN2, PU_EN1 and
> PU_EN2, MEAS_CC1 and MEAS_CC2 and implements
> a fixed DRP toggle between presenting as a SRC and
> presenting as a SNK. Alternately, it can present as a
> SRC or SNK only and poll CC1 and CC2 continuously."
>
> It is still attaching Rp resp Rd to CC1 or CC2 one at a time
> to detect polarity, so it is still toggling, it just is not
> doing dual-role toggling. This is also expected behavior for
> a sink, a sink may not present Rd on both CC pins at the
> same time, otherwise the source cannot detect the polarity
> and the source also cannot detect if Vconn is necessary.
>
>> It's a mechanism to
>> have the HW report when the CC line changes on connection. Without that we have
>> no reporting from the HW for the fixed role scenarios.
>
> Not just connection, also polarity detection. Notice that
> the tcpm framework / the driver also has a start_drp_toggling()
> method. I think we may also need a start_srp_toggling function
> just like it and call that from the SNK_UNATTACHED and
> SRC_UNATTACHED states for single-role ports. I agree that we
> need to start toggling when in those states, but tcpm_set_cc gets
> called in a lot of other places where AFAIK we should NOT
> restart toggling and your patch causes us to restart
> toggling in those cases.

Ok, so as I suspected, commit ea3b4d5523bc ("usb: typec: fusb302:
Resolve fixed power role contract setup") is what caused the
power-role swapping breakage I've been seeing.

So I've prepared a 3 patch series:

1) Add a new start_srp_connection_detect function which, when
implemented by the tcpc_dev, gets called instead of
start_drp_toggling for single role ports (SRPs)

2) Implement 1. for fusb302 to fix the SRP issue Adam was
seeing, without depending on set_cc starting "toggling"
or something like it for the fix

3) Revert commit ea3b4d5523bc, restoring power-role swap
functionality.

This should also fix the issue Kyle Tso was seeing when
trying to change from one Rp setting to another.

I'll send out the series right after this email.

Regards,

Hans

2019-04-15 10:04:40

by Adam Thomson

Subject: RE: [PATCH v2] usb: typec: tcpm: collision avoidance

On 13 April 2019 21:39, Hans de Goede wrote:

> On 10-04-19 18:38, Hans de Goede wrote:
> > On 10-04-19 18:14, Adam Thomson wrote:
> >> On 10 April 2019 16:45, Hans de Goede wrote:
>
> <snip>
>
> >>> Starting toggling from tcpm_set_cc() just feels wrong; and currently
> >>> power role swapping is broken with the fusb302, which IIRC used to
> >>> work. I suspect this is related.
> >>>
> >>> I plan to write a patch tomorrow to functionally take tcpm_set_cc()
> >>> back to the way it was before. This should fix your case and I hope
> >>> this also fixes power-role swapping.
> >>>
> >>> This will re-introduce Adam Thomson's problem, but I have a feeling
> >>> that that actually needs a fix in the tcpm.c code rather then at the fusb302
> level.
> >>
> >> To be clear here, the names TOGGLING_MODE_SNK and
> TOGGLING_MODE_SRC
> >> are a misnomer from the HW spec for fusb302. The device isn't
> >> toggling anything as far as I'm aware, so I don't necessarily agree with your
> point.
> >
> > If I understand the datasheet correctly:
> >
> > "The FUSB302 has the capability to do autonomous DRP toggle. In
> > autonomous toggle the FUSB302 internally controls the PDWN1, PDWN2,
> > PU_EN1 and PU_EN2, MEAS_CC1 and MEAS_CC2 and implements a fixed DRP
> > toggle between presenting as a SRC and presenting as a SNK.
> > Alternately, it can present as a SRC or SNK only and poll CC1 and CC2
> > continuously."
> >
> > It is still attaching Rp resp Rd to CC1 or CC2 one at a time to detect
> > polarity, so it is still toggling, it just is not doing dual-role
> > toggling. This is also expected behavior for a sink, a sink may not
> > present Rd on both CC pins at the same time, otherwise the source
> > cannot detect the polarity and the source also cannot detect if Vconn
> > is necessary.
> >
> >> It's a mechanism to
> >> have the HW report when the CC line changes on connection. Without
> >> that we have no reporting from the HW for the fixed role scenarios.
> >
> > Not just connection, also polarity detection. Notice that the tcpm
> > framework / the driver also has a start_drp_toggling() method. I think
> > we may also need a start_srp_toggling function just like it and call
> > that from the SNK_UNATTACHED and SRC_UNATTACHED states for single-role
> > ports. I agree that we need to start toggling when in those states,
> > but tcpm_set_cc gets called in a lot of other places where AFAIK we
> > should NOT restart toggling and your patch causes us to restart
> > toggling in those cases.
>
> Ok, so as I suspected, commit ea3b4d5523bc ("usb: typec: fusb302:
> Resolve fixed power role contract setup") is what caused the power-role
> swapping breakage I've been seeing.

Apologies for the breakage. Annoyed I didn't catch that when submitting that
patch. Thanks for looking to resolve this and will review your updates shortly.

> So I've prepared a 3 patch series:
>
> 1) Add a new start_srp_connection_detect function which, when implemented
> by the tcpc_dev, gets called instead of start_drp_toggling for single role ports
> (SRPs)
>
> 2) Implement 1. for fusb302 to fix the SRP issue Adam was seeing, without
> depending on set_cc starting "toggling"
> or something like it for the fix
>
> 3) Revert commit ea3b4d5523bc, restoring power-role swap functionality.
>
> This should also fix the issue Kyle Tso was seeing when trying to change from one
> Rp setting to another.
>
> I'll send out the series right after this email.
>
> Regards,
>
> Hans

2019-09-19 12:37:58

by Kyle Tso

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

Ping! Anyone still reviewing this patch?
I have another change related to AMS.
I will group them as a set and re-send it.

Regards,
Kyle Tso


On Mon, Apr 15, 2019 at 6:03 PM Adam Thomson
<[email protected]> wrote:
>
> On 13 April 2019 21:39, Hans de Goede wrote:
>
> > On 10-04-19 18:38, Hans de Goede wrote:
> > > On 10-04-19 18:14, Adam Thomson wrote:
> > >> On 10 April 2019 16:45, Hans de Goede wrote:
> >
> > <snip>
> >
> > >>> Starting toggling from tcpm_set_cc() just feels wrong; and currently
> > >>> power role swapping is broken with the fusb302, which IIRC used to
> > >>> work. I suspect this is related.
> > >>>
> > >>> I plan to write a patch tomorrow to functionally take tcpm_set_cc()
> > >>> back to the way it was before. This should fix your case and I hope
> > >>> this also fixes power-role swapping.
> > >>>
> > >>> This will re-introduce Adam Thomson's problem, but I have a feeling
> > >>> that that actually needs a fix in the tcpm.c code rather then at the fusb302
> > level.
> > >>
> > >> To be clear here, the names TOGGLING_MODE_SNK and
> > TOGGLING_MODE_SRC
> > >> are a misnomer from the HW spec for fusb302. The device isn't
> > >> toggling anything as far as I'm aware, so I don't necessarily agree with your
> > point.
> > >
> > > If I understand the datasheet correctly:
> > >
> > > "The FUSB302 has the capability to do autonomous DRP toggle. In
> > > autonomous toggle the FUSB302 internally controls the PDWN1, PDWN2,
> > > PU_EN1 and PU_EN2, MEAS_CC1 and MEAS_CC2 and implements a fixed DRP
> > > toggle between presenting as a SRC and presenting as a SNK.
> > > Alternately, it can present as a SRC or SNK only and poll CC1 and CC2
> > > continuously."
> > >
> > > It is still attaching Rp resp Rd to CC1 or CC2 one at a time to detect
> > > polarity, so it is still toggling, it just is not doing dual-role
> > > toggling. This is also expected behavior for a sink, a sink may not
> > > present Rd on both CC pins at the same time, otherwise the source
> > > cannot detect the polarity and the source also cannot detect if Vconn
> > > is necessary.
> > >
> > >> It's a mechanism to
> > >> have the HW report when the CC line changes on connection. Without
> > >> that we have no reporting from the HW for the fixed role scenarios.
> > >
> > > Not just connection, also polarity detection. Notice that the tcpm
> > > framework / the driver also has a start_drp_toggling() method. I think
> > > we may also need a start_srp_toggling function just like it and call
> > > that from the SNK_UNATTACHED and SRC_UNATTACHED states for single-role
> > > ports. I agree that we need to start toggling when in those states,
> > > but tcpm_set_cc gets called in a lot of other places where AFAIK we
> > > should NOT restart toggling and your patch causes us to restart
> > > toggling in those cases.
> >
> > Ok, so as I suspected, commit ea3b4d5523bc ("usb: typec: fusb302:
> > Resolve fixed power role contract setup") is what caused the power-role
> > swapping breakage I've been seeing.
>
> Apologies for the breakage. Annoyed I didn't catch that when submitting that
> patch. Thanks for looking to resolve this and will review your updates shortly.
>
> > So I've prepared a 3 patch series:
> >
> > 1) Add a new start_srp_connection_detect function which, when implemented
> > by the tcpc_dev, gets called instead of start_drp_toggling for single role ports
> > (SRPs)
> >
> > 2) Implement 1. for fusb302 to fix the SRP issue Adam was seeing, without
> > depending on set_cc starting "toggling"
> > or something like it for the fix
> >
> > 3) Revert commit ea3b4d5523bc, restoring power-role swap functionality.
> >
> > This should also fix the issue Kyle Tso was seeing when trying to change from one
> > Rp setting to another.
> >
> > I'll send out the series right after this email.
> >
> > Regards,
> >
> > Hans
>

2019-09-19 13:00:52

by Heikki Krogerus

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

On Thu, Sep 19, 2019 at 06:48:32PM +0800, Kyle Tso wrote:
> Ping! Anyone still reviewing this patch?
> I have another change related to AMS.
> I will group them as a set and re-send it.

Please rebase resend the patch. It does not apply any more.

thanks,

--
heikki

2019-09-19 13:58:56

by Guenter Roeck

Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

On 9/19/19 3:48 AM, Kyle Tso wrote:
> Ping! Anyone still reviewing this patch?

Which patch ? Hans' series was long since pushed.

Guenter

> I have another change related to AMS.
> I will group them as a set and re-send it.
>
> Regards,
> Kyle Tso
>
>
> On Mon, Apr 15, 2019 at 6:03 PM Adam Thomson
> <[email protected]> wrote:
>>
>> On 13 April 2019 21:39, Hans de Goede wrote:
>>
>>> On 10-04-19 18:38, Hans de Goede wrote:
>>>> On 10-04-19 18:14, Adam Thomson wrote:
>>>>> On 10 April 2019 16:45, Hans de Goede wrote:
>>>
>>> <snip>
>>>
>>>>>> Starting toggling from tcpm_set_cc() just feels wrong; and currently
>>>>>> power role swapping is broken with the fusb302, which IIRC used to
>>>>>> work. I suspect this is related.
>>>>>>
>>>>>> I plan to write a patch tomorrow to functionally take tcpm_set_cc()
>>>>>> back to the way it was before. This should fix your case and I hope
>>>>>> this also fixes power-role swapping.
>>>>>>
>>>>>> This will re-introduce Adam Thomson's problem, but I have a feeling
>>>>>> that that actually needs a fix in the tcpm.c code rather then at the fusb302
>>> level.
>>>>>
>>>>> To be clear here, the names TOGGLING_MODE_SNK and
>>> TOGGLING_MODE_SRC
>>>>> are a misnomer from the HW spec for fusb302. The device isn't
>>>>> toggling anything as far as I'm aware, so I don't necessarily agree with your
>>> point.
>>>>
>>>> If I understand the datasheet correctly:
>>>>
>>>> "The FUSB302 has the capability to do autonomous DRP toggle. In
>>>> autonomous toggle the FUSB302 internally controls the PDWN1, PDWN2,
>>>> PU_EN1 and PU_EN2, MEAS_CC1 and MEAS_CC2 and implements a fixed DRP
>>>> toggle between presenting as a SRC and presenting as a SNK.
>>>> Alternately, it can present as a SRC or SNK only and poll CC1 and CC2
>>>> continuously."
>>>>
>>>> It is still attaching Rp resp Rd to CC1 or CC2 one at a time to detect
>>>> polarity, so it is still toggling, it just is not doing dual-role
>>>> toggling. This is also expected behavior for a sink, a sink may not
>>>> present Rd on both CC pins at the same time, otherwise the source
>>>> cannot detect the polarity and the source also cannot detect if Vconn
>>>> is necessary.
>>>>
>>>>> It's a mechanism to
>>>>> have the HW report when the CC line changes on connection. Without
>>>>> that we have no reporting from the HW for the fixed role scenarios.
>>>>
>>>> Not just connection, also polarity detection. Notice that the tcpm
>>>> framework / the driver also has a start_drp_toggling() method. I think
>>>> we may also need a start_srp_toggling function just like it and call
>>>> that from the SNK_UNATTACHED and SRC_UNATTACHED states for single-role
>>>> ports. I agree that we need to start toggling when in those states,
>>>> but tcpm_set_cc gets called in a lot of other places where AFAIK we
>>>> should NOT restart toggling and your patch causes us to restart
>>>> toggling in those cases.
>>>
>>> Ok, so as I suspected, commit ea3b4d5523bc ("usb: typec: fusb302:
>>> Resolve fixed power role contract setup") is what caused the power-role
>>> swapping breakage I've been seeing.
>>
>> Apologies for the breakage. Annoyed I didn't catch that when submitting that
>> patch. Thanks for looking to resolve this and will review your updates shortly.
>>
>>> So I've prepared a 3 patch series:
>>>
>>> 1) Add a new start_srp_connection_detect function which, when implemented
>>> by the tcpc_dev, gets called instead of start_drp_toggling for single role ports
>>> (SRPs)
>>>
>>> 2) Implement 1. for fusb302 to fix the SRP issue Adam was seeing, without
>>> depending on set_cc starting "toggling"
>>> or something like it for the fix
>>>
>>> 3) Revert commit ea3b4d5523bc, restoring power-role swap functionality.
>>>
>>> This should also fix the issue Kyle Tso was seeing when trying to change from one
>>> Rp setting to another.
>>>
>>> I'll send out the series right after this email.
>>>
>>> Regards,
>>>
>>> Hans
>>
>