[PATCH v2 1/5] usb: tcpm: add core framework
Marek Vasut
marex at denx.de
Sat Jul 6 02:41:04 CEST 2024
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 !
More information about the U-Boot
mailing list