On 6/4/24 6:33 PM, Sebastian Reichel wrote: Hi,
sorry for the abysmal delay
diff --git a/cmd/tcpm.c b/cmd/tcpm.c new file mode 100644 index 000000000000..37cc3ed6a3b4 --- /dev/null +++ b/cmd/tcpm.c @@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * (C) Copyright 2024 Collabora + */ + +#include <command.h> +#include <errno.h> +#include <dm.h> +#include <dm/uclass-internal.h> +#include <usb/tcpm.h> + +#define LIMIT_DEV 32 +#define LIMIT_PARENT 20 + +static struct udevice *currdev; + +static int failure(int ret) +{ + printf("Error: %d (%s)\n", ret, errno_str(ret));
This seems unused, please drop.
+ return CMD_RET_FAILURE; +} + +static int do_dev(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) +{ + char *name; + int ret = -ENODEV; + + switch (argc) { + case 2: + name = argv[1]; + ret = tcpm_get(name, &currdev); + if (ret) { + printf("Can't get TCPM: %s!\n", name); + return failure(ret); + } + case 1: + if (!currdev) { + printf("TCPM device is not set!\n\n"); + return CMD_RET_USAGE; + } + + printf("dev: %d @ %s\n", dev_seq(currdev), currdev->name);
Can you replace the printing with dev_*() or log_*() ? [...]
+int do_print_info(struct udevice *dev) +{ + enum typec_orientation orientation = tcpm_get_orientation(dev); + const char *state = tcpm_get_state(dev); + int pd_rev = tcpm_get_pd_rev(dev); + int mv = tcpm_get_voltage(dev); + int ma = tcpm_get_current(dev); + enum typec_role pwr_role = tcpm_get_pwr_role(dev); + enum typec_data_role data_role = tcpm_get_data_role(dev); + bool connected = tcpm_is_connected(dev); + + if (connected) {
Invert the condition here to reduce indent: if (!connected) { printf("TCPM State...) return 0; } printf printf ... return 0;
+ printf("Orientation: %s\n", typec_orientation_name[orientation]); + printf("PD Revision: %s\n", typec_pd_rev_name[pd_rev]); + printf("Power Role: %s\n", typec_role_name[pwr_role]); + printf("Data Role: %s\n", typec_data_role_name[data_role]); + printf("Voltage: %2d.%03d V\n", mv / 1000, mv % 1000); + printf("Current: %2d.%03d A\n", ma / 1000, ma % 1000); + } else { + printf("TCPM State: %s\n", state); + } + + return 0; +}
[...]
+static int tcpm_pd_transmit(struct udevice *dev, + enum tcpm_transmit_type type, + const struct pd_message *msg) +{ + const struct dm_tcpm_ops *drvops = dev_get_driver_ops(dev); + struct tcpm_port *port = dev_get_uclass_plat(dev); + int ret; + int timeout = PD_T_TCPC_TX_TIMEOUT; + + if (msg) + dev_dbg(dev, "TCPM: PD TX, header: %#x\n", + le16_to_cpu(msg->header)); + else + dev_dbg(dev, "TCPM: PD TX, type: %#x\n", type); + + port->tx_complete = false; + ret = drvops->pd_transmit(dev, type, msg, port->negotiated_rev); + if (ret < 0) + return ret; + + while ((timeout > 0) && (!port->tx_complete)) { + drvops->poll_event(dev); + udelay(1000); + timeout--; + tcpm_check_and_run_delayed_work(dev); + }
Can this use e.g. read_poll_timeout() with custom op() implementation ? Would that make sense ?
+ if (!timeout) { + dev_err(dev, "TCPM: PD transmit data timeout\n"); + return -ETIMEDOUT; + } + + switch (port->tx_status) { + case TCPC_TX_SUCCESS: + port->message_id = (port->message_id + 1) & PD_HEADER_ID_MASK; + break; + case TCPC_TX_DISCARDED: + ret = -EAGAIN; + break; + case TCPC_TX_FAILED: + default: + ret = -EIO; + break; + } + + return ret; +} + +void tcpm_pd_transmit_complete(struct udevice *dev, + enum tcpm_transmit_status status)
How much of this can be static functions ?
+{ + struct tcpm_port *port = dev_get_uclass_plat(dev); + + dev_dbg(dev, "TCPM: PD TX complete, status: %u\n", status); + tcpm_reset_event_cnt(dev); + port->tx_status = status; + port->tx_complete = true; +}
[...]
+static void tcpm_pd_ctrl_request(struct udevice *dev, + const struct pd_message *msg) +{ + enum pd_ctrl_msg_type type = pd_header_type_le(msg->header); + struct tcpm_port *port = dev_get_uclass_plat(dev); + enum tcpm_state next_state; + + switch (type) {
[...]
+ case PD_CTRL_DR_SWAP: + if (port->port_type != TYPEC_PORT_DRP) { + tcpm_queue_message(dev, PD_MSG_CTRL_REJECT); + break; + } + /* + * XXX
What's this, some sort of FIXME ?
+ * 6.3.9: If an alternate mode is active, a request to swap + * alternate modes shall trigger a port reset. + */ + switch (port->state) { + case SRC_READY: + case SNK_READY: + tcpm_set_state(dev, DR_SWAP_ACCEPT, 0); + break; + default: + tcpm_queue_message(dev, PD_MSG_CTRL_WAIT); + break; + } + break;
[...]
+static bool tcpm_send_queued_message(struct udevice *dev) +{ + struct tcpm_port *port = dev_get_uclass_plat(dev); + enum pd_msg_request queued_message; + + do { + queued_message = port->queued_message; + port->queued_message = PD_MSG_NONE; + + switch (queued_message) { + case PD_MSG_CTRL_WAIT: + tcpm_pd_send_control(dev, PD_CTRL_WAIT); + break; + case PD_MSG_CTRL_REJECT: + tcpm_pd_send_control(dev, PD_CTRL_REJECT); + break; + case PD_MSG_CTRL_NOT_SUPP: + tcpm_pd_send_control(dev, PD_CTRL_NOT_SUPP); + break; + case PD_MSG_DATA_SINK_CAP: + tcpm_pd_send_sink_caps(dev); + break; + case PD_MSG_DATA_SOURCE_CAP: + tcpm_pd_send_source_caps(dev); + break; + default: + break; + } + } while (port->queued_message != PD_MSG_NONE);
Can this possibly run indefinitelly ? Should there be some limit on the number of processed messages ?
+ return false; +}
[...]
+void tcpm_poll_event(struct udevice *dev)
Can this be static void ?
+{ + const struct dm_tcpm_ops *drvops = dev_get_driver_ops(dev); + struct tcpm_port *port = dev_get_uclass_plat(dev); + + if (!drvops->get_vbus(dev)) + return; + + while (port->poll_event_cnt < TCPM_POLL_EVENT_TIME_OUT) { + if (!port->wait_dr_swap_message && + (port->state == SNK_READY || port->state == SRC_READY)) + break; + + drvops->poll_event(dev); + port->poll_event_cnt++; + udelay(500); + tcpm_check_and_run_delayed_work(dev); + } + + if (port->state != SNK_READY && port->state != SRC_READY) + dev_warn(dev, "TCPM: exit in state %s\n", + tcpm_states[port->state]); + + /* + * At this time, call the callback function of the respective pd chip + * to enter the low-power mode. In order to reduce the time spent on + * the PD chip driver as much as possible, the tcpm framework does not + * fully process the communication initiated by the device,so it should + * be noted that we can disable the internal oscillator, etc., but do + * not turn off the power of the transceiver module, otherwise the + * self-powered Type-C device will initiate a Message(eg: self-powered + * Type-C hub initiates a SINK capability request(PD_CTRL_GET_SINK_CAP)) + * and the pd chip cannot reply to GoodCRC, causing the self-powered Type-C + * device to switch vbus to vSafe5v, or even turn off vbus. + */ + if (drvops->enter_low_power_mode) {
Invert the condition here to reduce indent: if (!drvops->enter_low_power_mode) return;
+ if (drvops->enter_low_power_mode(dev, port->attached, + port->pd_capable)) + dev_err(dev, "TCPM: failed to enter low power\n"); + else + dev_info(dev, "TCPM: PD chip enter low power mode\n"); + } +}
A lot of code, but a real pleasure to read it. Thanks !