[PATCH v3 0/4] usb: typec: USB Modes

[PATCH v3 0/4] usb: typec: USB Modes

[Heikki Krogerus]

Hi,

Changed since v2:
- Renamed the callbacks - Abhishek


Changed since v1:
- Leaving the usb_mode file hidden if the usb_ capabilities are not known - Dmitriy
- Fixed kernel doc warning - kernel test robot


This was originally a proposal of mine that I posted back in 2019 as
an RFC [1].

The goal is to expose the USB Mode - USB Mode as defined in the USB
Type-C and USB Power Delivery specifications - to the user via sysfs.

With ports the mode will show the supported and the default USB Mode,
which is the mode that is (or should be) used in the next Enter_USB
Message.

With the partners the mode will show similarly the supported USB
modes, and also the active mode. Changing the active mode should cause
Data Reset (as described in the USB Power Delivery spec.). This should
happen even if there is no active USB Mode (in Alternate Mode).

I've modified the original RFC patches based on comments I've received
(offline). The major change is that I renamed the sysfs attribute file
for the port from "usb_mode" to "usb_capability".

I'm taking the USB mode in use in the UCSI driver with these, but I
did not add support for the "Set USB" UCSI command yet. I have no way
of testing it. Also, I don't really like that we have to construct the
Enter_USB Message from the abstracted information that UCSI supplies.
The interface should supply the Enter_USB Message that was used as is.

[1] https://lore.kernel.org/linux-usb/20191230152857.43917-1-heikki.krogerus@linux.intel.com/

Heikki Krogerus (4):
  usb: typec: Add attribute file showing the supported USB modes of the
    port
  usb: typec: Add attribute file showing the USB Modes of the partner
  usb: typec: ucsi: Supply the USB capabilities to the ports
  usb: typec: ucsi: Add support for the partner USB Modes

 Documentation/ABI/testing/sysfs-class-typec |  27 +++
 drivers/usb/typec/class.c                   | 204 +++++++++++++++++++-
 drivers/usb/typec/class.h                   |   3 +
 drivers/usb/typec/ucsi/ucsi.c               |  15 ++
 drivers/usb/typec/ucsi/ucsi.h               |  10 +
 include/linux/usb/typec.h                   |  22 +++
 6 files changed, 277 insertions(+), 4 deletions(-)

-- 
2.45.2

[PATCH v3 1/4] usb: typec: Add attribute file showing the supported USB modes of the port

[Heikki Krogerus]

This attribute file, named "usb_capability", will show the
supported USB modes, which are USB 2.0, USB 3.2 and USB4.
These modes are defined in the USB Type-C (R2.0) and USB
Power Delivery (R3.0 V2.0) Specifications.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-class-typec | 13 ++++
 drivers/usb/typec/class.c                   | 81 +++++++++++++++++++++
 drivers/usb/typec/class.h                   |  1 +
 include/linux/usb/typec.h                   | 17 +++++
 4 files changed, 112 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
index 281b995beb05..7c307f02d99e 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -149,6 +149,19 @@ Description:
 		advertise to the partner. The currently used capabilities are in
 		brackets. Selection happens by writing to the file.
 
+What:		/sys/class/typec/<port>/usb_capability
+Date:		May 2024
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:	Lists the supported USB Modes. The default USB mode that is used
+		next time with the Enter_USB Message is in brackets. The default
+		mode can be changed by writing to the file when supported by the
+		driver.
+
+		Valid values:
+		- usb2 (USB 2.0)
+		- usb3 (USB 3.2)
+		- usb4 (USB4)
+
 USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
 
 What:		/sys/class/typec/<port>-partner/accessory_mode
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 9262fcd4144f..ea9ee47bb246 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -219,6 +219,13 @@ static ssize_t usb_power_delivery_revision_show(struct device *dev,
 						char *buf);
 static DEVICE_ATTR_RO(usb_power_delivery_revision);
 
+static const char * const usb_modes[] = {
+	[USB_MODE_NONE] = "none",
+	[USB_MODE_USB2] = "usb2",
+	[USB_MODE_USB3] = "usb3",
+	[USB_MODE_USB4] = "usb4"
+};
+
 /* ------------------------------------------------------------------------- */
 /* Alternate Modes */
 
@@ -1289,6 +1296,67 @@ EXPORT_SYMBOL_GPL(typec_unregister_cable);
 /* ------------------------------------------------------------------------- */
 /* USB Type-C ports */
 
+/**
+ * typec_port_set_usb_mode - Set the operational USB mode for the port
+ * @port: USB Type-C port
+ * @mode: USB Mode (USB2, USB3 or USB4)
+ *
+ * @mode will be used with the next Enter_USB message. Existing connections are
+ * not affected.
+ */
+void typec_port_set_usb_mode(struct typec_port *port, enum usb_mode mode)
+{
+	port->usb_mode = mode;
+}
+EXPORT_SYMBOL_GPL(typec_port_set_usb_mode);
+
+static ssize_t
+usb_capability_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	enum usb_mode mode = to_typec_port(dev)->usb_mode;
+	u8 cap = to_typec_port(dev)->cap->usb_capability;
+	int len = 0;
+	int i;
+
+	for (i = USB_MODE_USB2; i < USB_MODE_USB4 + 1; i++) {
+		if (!(BIT(i - 1) & cap))
+			continue;
+
+		if (i == mode)
+			len += sysfs_emit_at(buf, len, "[%s] ", usb_modes[i]);
+		else
+			len += sysfs_emit_at(buf, len, "%s ", usb_modes[i]);
+	}
+
+	buf[len - 1] = '\n';
+	return len;
+}
+
+static ssize_t
+usb_capability_store(struct device *dev, struct device_attribute *attr,
+		     const char *buf, size_t size)
+{
+	struct typec_port *port = to_typec_port(dev);
+	int ret = 0;
+	int mode;
+
+	if (!port->ops || !port->ops->default_usb_mode_set)
+		return -EOPNOTSUPP;
+
+	mode = sysfs_match_string(usb_modes, buf);
+	if (mode < 0)
+		return mode;
+
+	ret = port->ops->default_usb_mode_set(port, mode);
+	if (ret)
+		return ret;
+
+	port->usb_mode = mode;
+
+	return size;
+}
+static DEVICE_ATTR_RW(usb_capability);
+
 /**
  * typec_port_set_usb_power_delivery - Assign USB PD for port.
  * @port: USB Type-C port.
@@ -1757,6 +1825,7 @@ static struct attribute *typec_attrs[] = {
 	&dev_attr_vconn_source.attr,
 	&dev_attr_port_type.attr,
 	&dev_attr_orientation.attr,
+	&dev_attr_usb_capability.attr,
 	NULL,
 };
 
@@ -1790,6 +1859,11 @@ static umode_t typec_attr_is_visible(struct kobject *kobj,
 		if (port->cap->orientation_aware)
 			return 0444;
 		return 0;
+	} else if (attr == &dev_attr_usb_capability.attr) {
+		if (!port->cap->usb_capability)
+			return 0;
+		if (!port->ops || !port->ops->default_usb_mode_set)
+			return 0444;
 	}
 
 	return attr->mode;
@@ -2428,6 +2502,13 @@ struct typec_port *typec_register_port(struct device *parent,
 	port->con.attach = typec_partner_attach;
 	port->con.deattach = typec_partner_deattach;
 
+	if (cap->usb_capability & USB_CAPABILITY_USB4)
+		port->usb_mode = USB_MODE_USB4;
+	else if (cap->usb_capability & USB_CAPABILITY_USB3)
+		port->usb_mode = USB_MODE_USB3;
+	else if (cap->usb_capability & USB_CAPABILITY_USB2)
+		port->usb_mode = USB_MODE_USB2;
+
 	device_initialize(&port->dev);
 	port->dev.class = &typec_class;
 	port->dev.parent = parent;
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
index 7485cdb9dd20..85bc50aa54f7 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -55,6 +55,7 @@ struct typec_port {
 	enum typec_role			vconn_role;
 	enum typec_pwr_opmode		pwr_opmode;
 	enum typec_port_type		port_type;
+	enum usb_mode			usb_mode;
 	struct mutex			port_type_lock;
 
 	enum typec_orientation		orientation;
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 549275f8ac1b..f7edced5b10b 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -87,6 +87,17 @@ enum typec_orientation {
 	TYPEC_ORIENTATION_REVERSE,
 };
 
+enum usb_mode {
+	USB_MODE_NONE,
+	USB_MODE_USB2,
+	USB_MODE_USB3,
+	USB_MODE_USB4
+};
+
+#define USB_CAPABILITY_USB2	BIT(0)
+#define USB_CAPABILITY_USB3	BIT(1)
+#define USB_CAPABILITY_USB4	BIT(2)
+
 /*
  * struct enter_usb_data - Enter_USB Message details
  * @eudo: Enter_USB Data Object
@@ -240,6 +251,7 @@ struct typec_partner_desc {
  * @port_type_set: Set port type
  * @pd_get: Get available USB Power Delivery Capabilities.
  * @pd_set: Set USB Power Delivery Capabilities.
+ * @default_usb_mode_set: USB Mode to be used by default with Enter_USB Message
  */
 struct typec_operations {
 	int (*try_role)(struct typec_port *port, int role);
@@ -250,6 +262,7 @@ struct typec_operations {
 			     enum typec_port_type type);
 	struct usb_power_delivery **(*pd_get)(struct typec_port *port);
 	int (*pd_set)(struct typec_port *port, struct usb_power_delivery *pd);
+	int (*default_usb_mode_set)(struct typec_port *port, enum usb_mode mode);
 };
 
 enum usb_pd_svdm_ver {
@@ -267,6 +280,7 @@ enum usb_pd_svdm_ver {
  * @svdm_version: USB PD Structured VDM version if supported
  * @prefer_role: Initial role preference (DRP ports).
  * @accessory: Supported Accessory Modes
+ * @usb_capability: Supported USB Modes
  * @fwnode: Optional fwnode of the port
  * @driver_data: Private pointer for driver specific info
  * @pd: Optional USB Power Delivery Support
@@ -283,6 +297,7 @@ struct typec_capability {
 	int			prefer_role;
 	enum typec_accessory	accessory[TYPEC_MAX_ACCESSORY];
 	unsigned int		orientation_aware:1;
+	u8			usb_capability;
 
 	struct fwnode_handle	*fwnode;
 	void			*driver_data;
@@ -350,6 +365,8 @@ int typec_port_set_usb_power_delivery(struct typec_port *port, struct usb_power_
 int typec_partner_set_usb_power_delivery(struct typec_partner *partner,
 					 struct usb_power_delivery *pd);
 
+void typec_port_set_usb_mode(struct typec_port *port, enum usb_mode mode);
+
 /**
  * struct typec_connector - Representation of Type-C port for external drivers
  * @attach: notification about device removal
-- 
2.45.2

[PATCH v3 2/4] usb: typec: Add attribute file showing the USB Modes of the partner

[Heikki Krogerus]

This attribute file shows the supported USB modes (USB 2.0,
USB 3.0 and USB4) of the partner, and the currently active
mode.

The active mode is determined primarily by checking the
speed of the enumerated USB device. When USB Power Delivery
is supported, the active USB mode should be always the mode
that was used with the Enter_USB Message, regardless of the
result of the USB enumeration. The port drivers can
separately assign the mode with a dedicated API.

If USB Power Delivery Identity is supplied for the partner
device, the supported modes are extracted from it.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-class-typec |  14 +++
 drivers/usb/typec/class.c                   | 123 +++++++++++++++++++-
 drivers/usb/typec/class.h                   |   2 +
 include/linux/usb/typec.h                   |   5 +
 4 files changed, 140 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
index 7c307f02d99e..a3afe04b2688 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -233,6 +233,20 @@ Description:
 		directory exists, it will have an attribute file for every VDO
 		in Discover Identity command result.
 
+What:		/sys/class/typec/<port>-partner/usb_mode
+Date:		February 2024
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:	The USB Modes that the partner device supports. The active mode
+		is displayed in brackets. The active USB mode can be changed by
+		writing to this file when the port driver is able to send Data
+		Reset Message to the partner. That requires USB Power Delivery
+		contract between the partner and the port.
+
+		Valid values:
+		- usb2 (USB 2.0)
+		- usb3 (USB 3.2)
+		- usb4 (USB4)
+
 USB Type-C cable devices (eg. /sys/class/typec/port0-cable/)
 
 Note: Electronically Marked Cables will have a device also for one cable plug
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index ea9ee47bb246..a6fedafc9c86 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -618,6 +618,74 @@ EXPORT_SYMBOL_GPL(typec_unregister_altmode);
 /* ------------------------------------------------------------------------- */
 /* Type-C Partners */
 
+/**
+ * typec_partner_set_usb_mode - Assign active USB Mode for the partner
+ * @partner: USB Type-C partner
+ * @mode: USB Mode (USB2, USB3 or USB4)
+ *
+ * The port drivers can use this function to assign the active USB Mode to
+ * @partner. The USB Mode can change for example due to Data Reset.
+ */
+void typec_partner_set_usb_mode(struct typec_partner *partner, enum usb_mode mode)
+{
+	if (!partner || partner->usb_mode == mode)
+		return;
+
+	partner->usb_capability |= BIT(mode - 1);
+	partner->usb_mode = mode;
+	sysfs_notify(&partner->dev.kobj, NULL, "usb_mode");
+}
+EXPORT_SYMBOL_GPL(typec_partner_set_usb_mode);
+
+static ssize_t
+usb_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct typec_partner *partner = to_typec_partner(dev);
+	int len = 0;
+	int i;
+
+	for (i = USB_MODE_USB2; i < USB_MODE_USB4 + 1; i++) {
+		if (!(BIT(i - 1) & partner->usb_capability))
+			continue;
+
+		if (i == partner->usb_mode)
+			len += sysfs_emit_at(buf, len, "[%s] ", usb_modes[i]);
+		else
+			len += sysfs_emit_at(buf, len, "%s ", usb_modes[i]);
+	}
+
+	buf[len - 1] = '\n';
+	return len;
+}
+
+static ssize_t usb_mode_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct typec_partner *partner = to_typec_partner(dev);
+	struct typec_port *port = to_typec_port(dev->parent);
+	int mode;
+	int ret;
+
+	if (!port->ops || !port->ops->enter_usb_mode)
+		return -EOPNOTSUPP;
+
+	mode = sysfs_match_string(usb_modes, buf);
+	if (mode < 0)
+		return mode;
+
+	if (mode == partner->usb_mode)
+		return size;
+
+	ret = port->ops->enter_usb_mode(port, mode);
+	if (ret)
+		return ret;
+
+	typec_partner_set_usb_mode(partner, mode);
+
+	return size;
+}
+static DEVICE_ATTR_RW(usb_mode);
+
 static ssize_t accessory_mode_show(struct device *dev,
 				   struct device_attribute *attr,
 				   char *buf)
@@ -664,6 +732,7 @@ static struct attribute *typec_partner_attrs[] = {
 	&dev_attr_supports_usb_power_delivery.attr,
 	&dev_attr_number_of_alternate_modes.attr,
 	&dev_attr_type.attr,
+	&dev_attr_usb_mode.attr,
 	&dev_attr_usb_power_delivery_revision.attr,
 	NULL
 };
@@ -671,6 +740,14 @@ static struct attribute *typec_partner_attrs[] = {
 static umode_t typec_partner_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
 {
 	struct typec_partner *partner = to_typec_partner(kobj_to_dev(kobj));
+	struct typec_port *port = to_typec_port(partner->dev.parent);
+
+	if (attr == &dev_attr_usb_mode.attr) {
+		if (!partner->usb_capability)
+			return 0;
+		if (!port->ops || !port->ops->enter_usb_mode)
+			return 0444;
+	}
 
 	if (attr == &dev_attr_number_of_alternate_modes.attr) {
 		if (partner->num_altmodes < 0)
@@ -744,10 +821,33 @@ static void typec_partner_unlink_device(struct typec_partner *partner, struct de
  */
 int typec_partner_set_identity(struct typec_partner *partner)
 {
-	if (!partner->identity)
+	u8 usb_capability = partner->usb_capability;
+	struct device *dev = &partner->dev;
+	struct usb_pd_identity *id;
+
+	id = get_pd_identity(dev);
+	if (!id)
 		return -EINVAL;
 
-	typec_report_identity(&partner->dev);
+	if (to_typec_port(dev->parent)->data_role == TYPEC_HOST)  {
+		u32 devcap = PD_VDO_UFP_DEVCAP(id->vdo[0]);
+
+		if (devcap & (DEV_USB2_CAPABLE | DEV_USB2_BILLBOARD))
+			usb_capability |= USB_CAPABILITY_USB2;
+		if (devcap & DEV_USB3_CAPABLE)
+			usb_capability |= USB_CAPABILITY_USB3;
+		if (devcap & DEV_USB4_CAPABLE)
+			usb_capability |= USB_CAPABILITY_USB4;
+	} else {
+		usb_capability = PD_VDO_DFP_HOSTCAP(id->vdo[0]);
+	}
+
+	if (partner->usb_capability != usb_capability) {
+		partner->usb_capability = usb_capability;
+		sysfs_notify(&dev->kobj, NULL, "usb_mode");
+	}
+
+	typec_report_identity(dev);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(typec_partner_set_identity);
@@ -917,6 +1017,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
 	partner->usb_pd = desc->usb_pd;
 	partner->accessory = desc->accessory;
 	partner->num_altmodes = -1;
+	partner->usb_capability = desc->usb_capability;
 	partner->pd_revision = desc->pd_revision;
 	partner->svdm_version = port->cap->svdm_version;
 	partner->attach = desc->attach;
@@ -936,6 +1037,15 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
 	partner->dev.type = &typec_partner_dev_type;
 	dev_set_name(&partner->dev, "%s-partner", dev_name(&port->dev));
 
+	if (port->usb2_dev) {
+		partner->usb_capability |= USB_CAPABILITY_USB2;
+		partner->usb_mode = USB_MODE_USB2;
+	}
+	if (port->usb3_dev) {
+		partner->usb_capability |= USB_CAPABILITY_USB2 | USB_CAPABILITY_USB3;
+		partner->usb_mode = USB_MODE_USB3;
+	}
+
 	ret = device_register(&partner->dev);
 	if (ret) {
 		dev_err(&port->dev, "failed to register partner (%d)\n", ret);
@@ -1935,13 +2045,18 @@ static void typec_partner_attach(struct typec_connector *con, struct device *dev
 	struct typec_port *port = container_of(con, struct typec_port, con);
 	struct typec_partner *partner = typec_get_partner(port);
 	struct usb_device *udev = to_usb_device(dev);
+	enum usb_mode usb_mode;
 
-	if (udev->speed < USB_SPEED_SUPER)
+	if (udev->speed < USB_SPEED_SUPER) {
+		usb_mode = USB_MODE_USB2;
 		port->usb2_dev = dev;
-	else
+	} else {
+		usb_mode = USB_MODE_USB3;
 		port->usb3_dev = dev;
+	}
 
 	if (partner) {
+		typec_partner_set_usb_mode(partner, usb_mode);
 		typec_partner_link_device(partner, dev);
 		put_device(&partner->dev);
 	}
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
index 85bc50aa54f7..b3076a24ad2e 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -35,6 +35,8 @@ struct typec_partner {
 	int				num_altmodes;
 	u16				pd_revision; /* 0300H = "3.0" */
 	enum usb_pd_svdm_ver		svdm_version;
+	enum usb_mode			usb_mode;
+	u8				usb_capability;
 
 	struct usb_power_delivery	*pd;
 
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index f7edced5b10b..d616b8807000 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -220,6 +220,7 @@ struct typec_cable_desc {
  * @accessory: Audio, Debug or none.
  * @identity: Discover Identity command data
  * @pd_revision: USB Power Delivery Specification Revision if supported
+ * @usb_capability: Supported USB Modes
  * @attach: Notification about attached USB device
  * @deattach: Notification about removed USB device
  *
@@ -237,6 +238,7 @@ struct typec_partner_desc {
 	enum typec_accessory	accessory;
 	struct usb_pd_identity	*identity;
 	u16			pd_revision; /* 0300H = "3.0" */
+	u8			usb_capability;
 
 	void (*attach)(struct typec_partner *partner, struct device *dev);
 	void (*deattach)(struct typec_partner *partner, struct device *dev);
@@ -252,6 +254,7 @@ struct typec_partner_desc {
  * @pd_get: Get available USB Power Delivery Capabilities.
  * @pd_set: Set USB Power Delivery Capabilities.
  * @default_usb_mode_set: USB Mode to be used by default with Enter_USB Message
+ * @enter_usb_mode: Change the active USB Mode
  */
 struct typec_operations {
 	int (*try_role)(struct typec_port *port, int role);
@@ -263,6 +266,7 @@ struct typec_operations {
 	struct usb_power_delivery **(*pd_get)(struct typec_port *port);
 	int (*pd_set)(struct typec_port *port, struct usb_power_delivery *pd);
 	int (*default_usb_mode_set)(struct typec_port *port, enum usb_mode mode);
+	int (*enter_usb_mode)(struct typec_port *port, enum usb_mode mode);
 };
 
 enum usb_pd_svdm_ver {
@@ -365,6 +369,7 @@ int typec_port_set_usb_power_delivery(struct typec_port *port, struct usb_power_
 int typec_partner_set_usb_power_delivery(struct typec_partner *partner,
 					 struct usb_power_delivery *pd);
 
+void typec_partner_set_usb_mode(struct typec_partner *partner, enum usb_mode usb_mode);
 void typec_port_set_usb_mode(struct typec_port *port, enum usb_mode mode);
 
 /**
-- 
2.45.2

[PATCH v3 3/4] usb: typec: ucsi: Supply the USB capabilities to the ports

[Heikki Krogerus]

The USB capabilities can be extracted from the response to
the Get Connector Capability command. USB2 and USB3 support
can be checked from the Operation Mode field, and USB4
support from the Extended Operation Mode field.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 7 +++++++
 drivers/usb/typec/ucsi/ucsi.h | 8 ++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index e0f3925e401b..13c739d334c4 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1588,6 +1588,13 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
 	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
 		*accessory = TYPEC_ACCESSORY_DEBUG;
 
+	if (UCSI_CONCAP_USB2_SUPPORT(con))
+		cap->usb_capability |= USB_CAPABILITY_USB2;
+	if (UCSI_CONCAP_USB3_SUPPORT(con))
+		cap->usb_capability |= USB_CAPABILITY_USB3;
+	if (UCSI_CONCAP_USB4_SUPPORT(con))
+		cap->usb_capability |= USB_CAPABILITY_USB4;
+
 	cap->driver_data = con;
 	cap->ops = &ucsi_ops;
 
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 1cf5aad4c23a..a1f4b9b568c8 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -286,6 +286,14 @@ struct ucsi_connector_capability {
 	UCSI_SPEC_REVISION_TO_BCD(UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_))
 } __packed;
 
+#define UCSI_CONCAP_USB2_SUPPORT(_con_) ((_con_)->cap.op_mode & UCSI_CONCAP_OPMODE_USB2)
+#define UCSI_CONCAP_USB3_SUPPORT(_con_) ((_con_)->cap.op_mode & UCSI_CONCAP_OPMODE_USB3)
+#define UCSI_CONCAP_USB4_SUPPORT(_con_)					\
+	((_con_)->ucsi->version >= UCSI_VERSION_2_0 &&			\
+	 ((_con_)->cap.flags & (UCSI_CONCAP_EX_OP_MODE_USB4_GEN2 |	\
+				UCSI_CONCAP_EX_OP_MODE_USB4_GEN3 |	\
+				UCSI_CONCAP_EX_OP_MODE_USB4_GEN4)))
+
 struct ucsi_altmode {
 	u16 svid;
 	u32 mid;
-- 
2.45.2

[PATCH v3 4/4] usb: typec: ucsi: Add support for the partner USB Modes

[Heikki Krogerus]

UCSI does not share the contents of the Enter_USB Message
that was used, so the active mode still has to be always
determined from the enumerated USB device. However, after
UCSI v2.0 it is possible to check separately is USB4 the
active mode.

So with USB2 and USB3 the mode is always determined from the
result of the USB enumeration, and when USB4 USB Mode is
active, UCSI driver can assign the mode directly.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 8 ++++++++
 drivers/usb/typec/ucsi/ucsi.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 13c739d334c4..804f7f9b35ea 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1057,6 +1057,14 @@ static int ucsi_register_partner(struct ucsi_connector *con)
 
 	con->partner = partner;
 
+	if (con->ucsi->version >= UCSI_VERSION_3_0)
+		if (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) &
+		    UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN4)
+			typec_partner_set_usb_mode(partner, USB_MODE_USB4);
+	if (con->ucsi->version >= UCSI_VERSION_2_0)
+		if (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) &
+		    UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN3)
+			typec_partner_set_usb_mode(partner, USB_MODE_USB4);
 	return 0;
 }
 
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index a1f4b9b568c8..d850073e8d0a 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -346,6 +346,8 @@ struct ucsi_connector_status {
 #define UCSI_CONSTAT_PARTNER_FLAGS(_f_)		(((_f_) & GENMASK(12, 5)) >> 5)
 #define   UCSI_CONSTAT_PARTNER_FLAG_USB		1
 #define   UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE	2
+#define   UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN3	4
+#define   UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN4	8
 #define UCSI_CONSTAT_PARTNER_TYPE(_f_)		(((_f_) & GENMASK(15, 13)) >> 13)
 #define   UCSI_CONSTAT_PARTNER_TYPE_DFP		1
 #define   UCSI_CONSTAT_PARTNER_TYPE_UFP		2
-- 
2.45.2

Re: [PATCH v3 1/4] usb: typec: Add attribute file showing the supported USB modes of the port

[Greg Kroah-Hartman]

On Fri, Oct 11, 2024 at 03:43:59PM +0300, Heikki Krogerus wrote:
> +What:		/sys/class/typec/<port>/usb_capability
> +Date:		May 2024

It's a bit past May 2024 :)

> +static ssize_t
> +usb_capability_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	enum usb_mode mode = to_typec_port(dev)->usb_mode;
> +	u8 cap = to_typec_port(dev)->cap->usb_capability;
> +	int len = 0;
> +	int i;
> +
> +	for (i = USB_MODE_USB2; i < USB_MODE_USB4 + 1; i++) {
> +		if (!(BIT(i - 1) & cap))
> +			continue;
> +
> +		if (i == mode)
> +			len += sysfs_emit_at(buf, len, "[%s] ", usb_modes[i]);
> +		else
> +			len += sysfs_emit_at(buf, len, "%s ", usb_modes[i]);
> +	}
> +
> +	buf[len - 1] = '\n';

Nit, shouldn't this be another call to sysfs_emit_at()?

> @@ -240,6 +251,7 @@ struct typec_partner_desc {
>   * @port_type_set: Set port type
>   * @pd_get: Get available USB Power Delivery Capabilities.
>   * @pd_set: Set USB Power Delivery Capabilities.
> + * @default_usb_mode_set: USB Mode to be used by default with Enter_USB Message
>   */
>  struct typec_operations {
>  	int (*try_role)(struct typec_port *port, int role);
> @@ -250,6 +262,7 @@ struct typec_operations {
>  			     enum typec_port_type type);
>  	struct usb_power_delivery **(*pd_get)(struct typec_port *port);
>  	int (*pd_set)(struct typec_port *port, struct usb_power_delivery *pd);
> +	int (*default_usb_mode_set)(struct typec_port *port, enum usb_mode mode);

Naming is hard, but usually it's "noun_verb" so wouldn't this be just
"mode_set_default"?  We know it's usb :)

But why default, why not just "mode_set"?  or "set_mode" given you have
"try_role" here, but then you have "pd_set".  Ick, I don't know, it's
your code, so your call, nevermind...

thanks,

greg k-h

Re: [PATCH v3 2/4] usb: typec: Add attribute file showing the USB Modes of the partner

[Greg Kroah-Hartman]

On Fri, Oct 11, 2024 at 03:44:00PM +0300, Heikki Krogerus wrote:
> This attribute file shows the supported USB modes (USB 2.0,
> USB 3.0 and USB4) of the partner, and the currently active
> mode.
> 
> The active mode is determined primarily by checking the
> speed of the enumerated USB device. When USB Power Delivery
> is supported, the active USB mode should be always the mode
> that was used with the Enter_USB Message, regardless of the
> result of the USB enumeration. The port drivers can
> separately assign the mode with a dedicated API.
> 
> If USB Power Delivery Identity is supplied for the partner
> device, the supported modes are extracted from it.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  14 +++
>  drivers/usb/typec/class.c                   | 123 +++++++++++++++++++-
>  drivers/usb/typec/class.h                   |   2 +
>  include/linux/usb/typec.h                   |   5 +
>  4 files changed, 140 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index 7c307f02d99e..a3afe04b2688 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -233,6 +233,20 @@ Description:
>  		directory exists, it will have an attribute file for every VDO
>  		in Discover Identity command result.
>  
> +What:		/sys/class/typec/<port>-partner/usb_mode
> +Date:		February 2024

It's later than this :)


> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:	The USB Modes that the partner device supports. The active mode
> +		is displayed in brackets. The active USB mode can be changed by
> +		writing to this file when the port driver is able to send Data
> +		Reset Message to the partner. That requires USB Power Delivery
> +		contract between the partner and the port.
> +
> +		Valid values:
> +		- usb2 (USB 2.0)
> +		- usb3 (USB 3.2)
> +		- usb4 (USB4)

We should probably add all of this info to 'lsusb' one of these days.
I'll add it to my todo list...

> +
>  USB Type-C cable devices (eg. /sys/class/typec/port0-cable/)
>  
>  Note: Electronically Marked Cables will have a device also for one cable plug
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index ea9ee47bb246..a6fedafc9c86 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -618,6 +618,74 @@ EXPORT_SYMBOL_GPL(typec_unregister_altmode);
>  /* ------------------------------------------------------------------------- */
>  /* Type-C Partners */
>  
> +/**
> + * typec_partner_set_usb_mode - Assign active USB Mode for the partner
> + * @partner: USB Type-C partner
> + * @mode: USB Mode (USB2, USB3 or USB4)
> + *
> + * The port drivers can use this function to assign the active USB Mode to
> + * @partner. The USB Mode can change for example due to Data Reset.
> + */
> +void typec_partner_set_usb_mode(struct typec_partner *partner, enum usb_mode mode)
> +{
> +	if (!partner || partner->usb_mode == mode)
> +		return;
> +
> +	partner->usb_capability |= BIT(mode - 1);
> +	partner->usb_mode = mode;
> +	sysfs_notify(&partner->dev.kobj, NULL, "usb_mode");

Who is listening for this and what are they going to do with the
information?

> +}
> +EXPORT_SYMBOL_GPL(typec_partner_set_usb_mode);
> +
> +static ssize_t
> +usb_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct typec_partner *partner = to_typec_partner(dev);
> +	int len = 0;
> +	int i;
> +
> +	for (i = USB_MODE_USB2; i < USB_MODE_USB4 + 1; i++) {
> +		if (!(BIT(i - 1) & partner->usb_capability))
> +			continue;
> +
> +		if (i == partner->usb_mode)
> +			len += sysfs_emit_at(buf, len, "[%s] ", usb_modes[i]);
> +		else
> +			len += sysfs_emit_at(buf, len, "%s ", usb_modes[i]);
> +	}
> +
> +	buf[len - 1] = '\n';

Again, sysfs_emit_at()?

thanks,

greg k-h

Re: [PATCH v3 4/4] usb: typec: ucsi: Add support for the partner USB Modes

[Greg Kroah-Hartman]

On Fri, Oct 11, 2024 at 03:44:02PM +0300, Heikki Krogerus wrote:
> UCSI does not share the contents of the Enter_USB Message
> that was used, so the active mode still has to be always
> determined from the enumerated USB device. However, after
> UCSI v2.0 it is possible to check separately is USB4 the
> active mode.
> 
> So with USB2 and USB3 the mode is always determined from the
> result of the USB enumeration, and when USB4 USB Mode is
> active, UCSI driver can assign the mode directly.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 8 ++++++++
>  drivers/usb/typec/ucsi/ucsi.h | 2 ++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 13c739d334c4..804f7f9b35ea 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1057,6 +1057,14 @@ static int ucsi_register_partner(struct ucsi_connector *con)
>  
>  	con->partner = partner;
>  
> +	if (con->ucsi->version >= UCSI_VERSION_3_0)
> +		if (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) &
> +		    UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN4)
> +			typec_partner_set_usb_mode(partner, USB_MODE_USB4);
> +	if (con->ucsi->version >= UCSI_VERSION_2_0)
> +		if (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) &
> +		    UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN3)
> +			typec_partner_set_usb_mode(partner, USB_MODE_USB4);

Will this cause multiple set mode uevents to go out if both of these are
true?

thanks,

greg k-h

Re: [PATCH v3 1/4] usb: typec: Add attribute file showing the supported USB modes of the port

[Heikki Krogerus]

On Fri, Oct 11, 2024 at 03:07:21PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 11, 2024 at 03:43:59PM +0300, Heikki Krogerus wrote:
> > +What:		/sys/class/typec/<port>/usb_capability
> > +Date:		May 2024
> 
> It's a bit past May 2024 :)

Yes. I'm sorry about that.

> > +static ssize_t
> > +usb_capability_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	enum usb_mode mode = to_typec_port(dev)->usb_mode;
> > +	u8 cap = to_typec_port(dev)->cap->usb_capability;
> > +	int len = 0;
> > +	int i;
> > +
> > +	for (i = USB_MODE_USB2; i < USB_MODE_USB4 + 1; i++) {
> > +		if (!(BIT(i - 1) & cap))
> > +			continue;
> > +
> > +		if (i == mode)
> > +			len += sysfs_emit_at(buf, len, "[%s] ", usb_modes[i]);
> > +		else
> > +			len += sysfs_emit_at(buf, len, "%s ", usb_modes[i]);
> > +	}
> > +
> > +	buf[len - 1] = '\n';
> 
> Nit, shouldn't this be another call to sysfs_emit_at()?

Yes.

> > @@ -240,6 +251,7 @@ struct typec_partner_desc {
> >   * @port_type_set: Set port type
> >   * @pd_get: Get available USB Power Delivery Capabilities.
> >   * @pd_set: Set USB Power Delivery Capabilities.
> > + * @default_usb_mode_set: USB Mode to be used by default with Enter_USB Message
> >   */
> >  struct typec_operations {
> >  	int (*try_role)(struct typec_port *port, int role);
> > @@ -250,6 +262,7 @@ struct typec_operations {
> >  			     enum typec_port_type type);
> >  	struct usb_power_delivery **(*pd_get)(struct typec_port *port);
> >  	int (*pd_set)(struct typec_port *port, struct usb_power_delivery *pd);
> > +	int (*default_usb_mode_set)(struct typec_port *port, enum usb_mode mode);
> 
> Naming is hard, but usually it's "noun_verb" so wouldn't this be just
> "mode_set_default"?  We know it's usb :)
> 
> But why default, why not just "mode_set"?  or "set_mode" given you have
> "try_role" here, but then you have "pd_set".  Ick, I don't know, it's
> your code, so your call, nevermind...

I think I'll just change it this back to the way it was in the last
version, and drop "default" from the name.

thanks,

-- 
heikki

Re: [PATCH v3 2/4] usb: typec: Add attribute file showing the USB Modes of the partner

[Heikki Krogerus]

On Fri, Oct 11, 2024 at 03:09:57PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 11, 2024 at 03:44:00PM +0300, Heikki Krogerus wrote:
> > This attribute file shows the supported USB modes (USB 2.0,
> > USB 3.0 and USB4) of the partner, and the currently active
> > mode.
> > 
> > The active mode is determined primarily by checking the
> > speed of the enumerated USB device. When USB Power Delivery
> > is supported, the active USB mode should be always the mode
> > that was used with the Enter_USB Message, regardless of the
> > result of the USB enumeration. The port drivers can
> > separately assign the mode with a dedicated API.
> > 
> > If USB Power Delivery Identity is supplied for the partner
> > device, the supported modes are extracted from it.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-typec |  14 +++
> >  drivers/usb/typec/class.c                   | 123 +++++++++++++++++++-
> >  drivers/usb/typec/class.h                   |   2 +
> >  include/linux/usb/typec.h                   |   5 +
> >  4 files changed, 140 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > index 7c307f02d99e..a3afe04b2688 100644
> > --- a/Documentation/ABI/testing/sysfs-class-typec
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -233,6 +233,20 @@ Description:
> >  		directory exists, it will have an attribute file for every VDO
> >  		in Discover Identity command result.
> >  
> > +What:		/sys/class/typec/<port>-partner/usb_mode
> > +Date:		February 2024
> 
> It's later than this :)

Indeed. I'm sorry.

> 
> > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > +Description:	The USB Modes that the partner device supports. The active mode
> > +		is displayed in brackets. The active USB mode can be changed by
> > +		writing to this file when the port driver is able to send Data
> > +		Reset Message to the partner. That requires USB Power Delivery
> > +		contract between the partner and the port.
> > +
> > +		Valid values:
> > +		- usb2 (USB 2.0)
> > +		- usb3 (USB 3.2)
> > +		- usb4 (USB4)
> 
> We should probably add all of this info to 'lsusb' one of these days.
> I'll add it to my todo list...
> 
> > +
> >  USB Type-C cable devices (eg. /sys/class/typec/port0-cable/)
> >  
> >  Note: Electronically Marked Cables will have a device also for one cable plug
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index ea9ee47bb246..a6fedafc9c86 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -618,6 +618,74 @@ EXPORT_SYMBOL_GPL(typec_unregister_altmode);
> >  /* ------------------------------------------------------------------------- */
> >  /* Type-C Partners */
> >  
> > +/**
> > + * typec_partner_set_usb_mode - Assign active USB Mode for the partner
> > + * @partner: USB Type-C partner
> > + * @mode: USB Mode (USB2, USB3 or USB4)
> > + *
> > + * The port drivers can use this function to assign the active USB Mode to
> > + * @partner. The USB Mode can change for example due to Data Reset.
> > + */
> > +void typec_partner_set_usb_mode(struct typec_partner *partner, enum usb_mode mode)
> > +{
> > +	if (!partner || partner->usb_mode == mode)
> > +		return;
> > +
> > +	partner->usb_capability |= BIT(mode - 1);
> > +	partner->usb_mode = mode;
> > +	sysfs_notify(&partner->dev.kobj, NULL, "usb_mode");
> 
> Who is listening for this and what are they going to do with the
> information?

I'll drop it, unless Abhishek, you guys would have use for it. Let me
know. I'll send v4 next week.

> > +}
> > +EXPORT_SYMBOL_GPL(typec_partner_set_usb_mode);
> > +
> > +static ssize_t
> > +usb_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct typec_partner *partner = to_typec_partner(dev);
> > +	int len = 0;
> > +	int i;
> > +
> > +	for (i = USB_MODE_USB2; i < USB_MODE_USB4 + 1; i++) {
> > +		if (!(BIT(i - 1) & partner->usb_capability))
> > +			continue;
> > +
> > +		if (i == partner->usb_mode)
> > +			len += sysfs_emit_at(buf, len, "[%s] ", usb_modes[i]);
> > +		else
> > +			len += sysfs_emit_at(buf, len, "%s ", usb_modes[i]);
> > +	}
> > +
> > +	buf[len - 1] = '\n';
> 
> Again, sysfs_emit_at()?

Yes. These are going back to the internal review. Too many mistakes.
Sorry.

thanks,

-- 
heikki

Re: [PATCH v3 4/4] usb: typec: ucsi: Add support for the partner USB Modes

[Heikki Krogerus]

On Fri, Oct 11, 2024 at 03:11:01PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 11, 2024 at 03:44:02PM +0300, Heikki Krogerus wrote:
> > UCSI does not share the contents of the Enter_USB Message
> > that was used, so the active mode still has to be always
> > determined from the enumerated USB device. However, after
> > UCSI v2.0 it is possible to check separately is USB4 the
> > active mode.
> > 
> > So with USB2 and USB3 the mode is always determined from the
> > result of the USB enumeration, and when USB4 USB Mode is
> > active, UCSI driver can assign the mode directly.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/typec/ucsi/ucsi.c | 8 ++++++++
> >  drivers/usb/typec/ucsi/ucsi.h | 2 ++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index 13c739d334c4..804f7f9b35ea 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -1057,6 +1057,14 @@ static int ucsi_register_partner(struct ucsi_connector *con)
> >  
> >  	con->partner = partner;
> >  
> > +	if (con->ucsi->version >= UCSI_VERSION_3_0)
> > +		if (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) &
> > +		    UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN4)
> > +			typec_partner_set_usb_mode(partner, USB_MODE_USB4);
> > +	if (con->ucsi->version >= UCSI_VERSION_2_0)
> > +		if (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) &
> > +		    UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN3)
> > +			typec_partner_set_usb_mode(partner, USB_MODE_USB4);
> 
> Will this cause multiple set mode uevents to go out if both of these are
> true?

That should not be possible, but I'll change it to if else.

thanks,

-- 
heikki

Re: [PATCH v3 1/4] usb: typec: Add attribute file showing the supported USB modes of the port

[Abhishek Pandit-Subedi]

On Fri, Oct 11, 2024 at 6:59 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Fri, Oct 11, 2024 at 03:07:21PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Oct 11, 2024 at 03:43:59PM +0300, Heikki Krogerus wrote:
> > > +What:              /sys/class/typec/<port>/usb_capability
> > > +Date:              May 2024
> >
> > It's a bit past May 2024 :)
>
> Yes. I'm sorry about that.
>
> > > +static ssize_t
> > > +usb_capability_show(struct device *dev, struct device_attribute *attr, char *buf)
> > > +{
> > > +   enum usb_mode mode = to_typec_port(dev)->usb_mode;
> > > +   u8 cap = to_typec_port(dev)->cap->usb_capability;
> > > +   int len = 0;
> > > +   int i;
> > > +
> > > +   for (i = USB_MODE_USB2; i < USB_MODE_USB4 + 1; i++) {
> > > +           if (!(BIT(i - 1) & cap))
> > > +                   continue;
> > > +
> > > +           if (i == mode)
> > > +                   len += sysfs_emit_at(buf, len, "[%s] ", usb_modes[i]);
> > > +           else
> > > +                   len += sysfs_emit_at(buf, len, "%s ", usb_modes[i]);
> > > +   }
> > > +
> > > +   buf[len - 1] = '\n';
> >
> > Nit, shouldn't this be another call to sysfs_emit_at()?
>
> Yes.
>
> > > @@ -240,6 +251,7 @@ struct typec_partner_desc {
> > >   * @port_type_set: Set port type
> > >   * @pd_get: Get available USB Power Delivery Capabilities.
> > >   * @pd_set: Set USB Power Delivery Capabilities.
> > > + * @default_usb_mode_set: USB Mode to be used by default with Enter_USB Message
> > >   */
> > >  struct typec_operations {
> > >     int (*try_role)(struct typec_port *port, int role);
> > > @@ -250,6 +262,7 @@ struct typec_operations {
> > >                          enum typec_port_type type);
> > >     struct usb_power_delivery **(*pd_get)(struct typec_port *port);
> > >     int (*pd_set)(struct typec_port *port, struct usb_power_delivery *pd);
> > > +   int (*default_usb_mode_set)(struct typec_port *port, enum usb_mode mode);
> >
> > Naming is hard, but usually it's "noun_verb" so wouldn't this be just
> > "mode_set_default"?  We know it's usb :)
> >
> > But why default, why not just "mode_set"?  or "set_mode" given you have
> > "try_role" here, but then you have "pd_set".  Ick, I don't know, it's
> > your code, so your call, nevermind...
>
> I think I'll just change it this back to the way it was in the last
> version, and drop "default" from the name.

What's being set underneath is what USB mode to enter on the next
reset or attach -- i.e. the default USB mode to enter. So appropriate
naming here may be one of usb_set_default, usb_set_next. Dropping
"usb" makes less sense vs dropping "mode", which could also refer to
alternate modes so I'd prefer we don't call it mode_set.

>
> thanks,
>
> --
> heikki

Re: [PATCH v3 2/4] usb: typec: Add attribute file showing the USB Modes of the partner

[Abhishek Pandit-Subedi]

On Fri, Oct 11, 2024 at 7:04 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Fri, Oct 11, 2024 at 03:09:57PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Oct 11, 2024 at 03:44:00PM +0300, Heikki Krogerus wrote:
> > > This attribute file shows the supported USB modes (USB 2.0,
> > > USB 3.0 and USB4) of the partner, and the currently active
> > > mode.
> > >
> > > The active mode is determined primarily by checking the
> > > speed of the enumerated USB device. When USB Power Delivery
> > > is supported, the active USB mode should be always the mode
> > > that was used with the Enter_USB Message, regardless of the
> > > result of the USB enumeration. The port drivers can
> > > separately assign the mode with a dedicated API.
> > >
> > > If USB Power Delivery Identity is supplied for the partner
> > > device, the supported modes are extracted from it.
> > >
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-class-typec |  14 +++
> > >  drivers/usb/typec/class.c                   | 123 +++++++++++++++++++-
> > >  drivers/usb/typec/class.h                   |   2 +
> > >  include/linux/usb/typec.h                   |   5 +
> > >  4 files changed, 140 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > > index 7c307f02d99e..a3afe04b2688 100644
> > > --- a/Documentation/ABI/testing/sysfs-class-typec
> > > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > > @@ -233,6 +233,20 @@ Description:
> > >             directory exists, it will have an attribute file for every VDO
> > >             in Discover Identity command result.
> > >
> > > +What:              /sys/class/typec/<port>-partner/usb_mode
> > > +Date:              February 2024
> >
> > It's later than this :)
>
> Indeed. I'm sorry.
>
> >
> > > +Contact:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > +Description:       The USB Modes that the partner device supports. The active mode
> > > +           is displayed in brackets. The active USB mode can be changed by
> > > +           writing to this file when the port driver is able to send Data
> > > +           Reset Message to the partner. That requires USB Power Delivery
> > > +           contract between the partner and the port.
> > > +
> > > +           Valid values:
> > > +           - usb2 (USB 2.0)
> > > +           - usb3 (USB 3.2)
> > > +           - usb4 (USB4)
> >
> > We should probably add all of this info to 'lsusb' one of these days.
> > I'll add it to my todo list...
> >
> > > +
> > >  USB Type-C cable devices (eg. /sys/class/typec/port0-cable/)
> > >
> > >  Note: Electronically Marked Cables will have a device also for one cable plug
> > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > > index ea9ee47bb246..a6fedafc9c86 100644
> > > --- a/drivers/usb/typec/class.c
> > > +++ b/drivers/usb/typec/class.c
> > > @@ -618,6 +618,74 @@ EXPORT_SYMBOL_GPL(typec_unregister_altmode);
> > >  /* ------------------------------------------------------------------------- */
> > >  /* Type-C Partners */
> > >
> > > +/**
> > > + * typec_partner_set_usb_mode - Assign active USB Mode for the partner
> > > + * @partner: USB Type-C partner
> > > + * @mode: USB Mode (USB2, USB3 or USB4)
> > > + *
> > > + * The port drivers can use this function to assign the active USB Mode to
> > > + * @partner. The USB Mode can change for example due to Data Reset.
> > > + */
> > > +void typec_partner_set_usb_mode(struct typec_partner *partner, enum usb_mode mode)
> > > +{
> > > +   if (!partner || partner->usb_mode == mode)
> > > +           return;
> > > +
> > > +   partner->usb_capability |= BIT(mode - 1);
> > > +   partner->usb_mode = mode;
> > > +   sysfs_notify(&partner->dev.kobj, NULL, "usb_mode");
> >
> > Who is listening for this and what are they going to do with the
> > information?
>
> I'll drop it, unless Abhishek, you guys would have use for it. Let me
> know. I'll send v4 next week.

I think you are ok to remove this. We would care about this value when
registering the partner and when activating a specific USB mode. With
the latter, we can just depend on the synchronous nature of
usb_mode_store (i.e. we can check the result after writing to the
sysfs file).

>
> > > +}
> > > +EXPORT_SYMBOL_GPL(typec_partner_set_usb_mode);
> > > +
> > > +static ssize_t
> > > +usb_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> > > +{
> > > +   struct typec_partner *partner = to_typec_partner(dev);
> > > +   int len = 0;
> > > +   int i;
> > > +
> > > +   for (i = USB_MODE_USB2; i < USB_MODE_USB4 + 1; i++) {
> > > +           if (!(BIT(i - 1) & partner->usb_capability))
> > > +                   continue;
> > > +
> > > +           if (i == partner->usb_mode)
> > > +                   len += sysfs_emit_at(buf, len, "[%s] ", usb_modes[i]);
> > > +           else
> > > +                   len += sysfs_emit_at(buf, len, "%s ", usb_modes[i]);
> > > +   }
> > > +
> > > +   buf[len - 1] = '\n';
> >
> > Again, sysfs_emit_at()?
>
> Yes. These are going back to the internal review. Too many mistakes.
> Sorry.
>
> thanks,
>
> --
> heikki

Re: [PATCH v3 1/4] usb: typec: Add attribute file showing the supported USB modes of the port

[Heikki Krogerus]

> > > > @@ -240,6 +251,7 @@ struct typec_partner_desc {
> > > >   * @port_type_set: Set port type
> > > >   * @pd_get: Get available USB Power Delivery Capabilities.
> > > >   * @pd_set: Set USB Power Delivery Capabilities.
> > > > + * @default_usb_mode_set: USB Mode to be used by default with Enter_USB Message
> > > >   */
> > > >  struct typec_operations {
> > > >     int (*try_role)(struct typec_port *port, int role);
> > > > @@ -250,6 +262,7 @@ struct typec_operations {
> > > >                          enum typec_port_type type);
> > > >     struct usb_power_delivery **(*pd_get)(struct typec_port *port);
> > > >     int (*pd_set)(struct typec_port *port, struct usb_power_delivery *pd);
> > > > +   int (*default_usb_mode_set)(struct typec_port *port, enum usb_mode mode);
> > >
> > > Naming is hard, but usually it's "noun_verb" so wouldn't this be just
> > > "mode_set_default"?  We know it's usb :)
> > >
> > > But why default, why not just "mode_set"?  or "set_mode" given you have
> > > "try_role" here, but then you have "pd_set".  Ick, I don't know, it's
> > > your code, so your call, nevermind...
> >
> > I think I'll just change it this back to the way it was in the last
> > version, and drop "default" from the name.
> 
> What's being set underneath is what USB mode to enter on the next
> reset or attach -- i.e. the default USB mode to enter. So appropriate
> naming here may be one of usb_set_default, usb_set_next. Dropping
> "usb" makes less sense vs dropping "mode", which could also refer to
> alternate modes so I'd prefer we don't call it mode_set.

I'll keep it the way it is now. This is kernel internal stuff, so we
can always change it later.

thanks,

-- 
heikki

Re: [PATCH v3 2/4] usb: typec: Add attribute file showing the USB Modes of the partner

[Heikki Krogerus]

> > > > +void typec_partner_set_usb_mode(struct typec_partner *partner, enum usb_mode mode)
> > > > +{
> > > > +   if (!partner || partner->usb_mode == mode)
> > > > +           return;
> > > > +
> > > > +   partner->usb_capability |= BIT(mode - 1);
> > > > +   partner->usb_mode = mode;
> > > > +   sysfs_notify(&partner->dev.kobj, NULL, "usb_mode");
> > >
> > > Who is listening for this and what are they going to do with the
> > > information?
> >
> > I'll drop it, unless Abhishek, you guys would have use for it. Let me
> > know. I'll send v4 next week.
> 
> I think you are ok to remove this. We would care about this value when
> registering the partner and when activating a specific USB mode. With
> the latter, we can just depend on the synchronous nature of
> usb_mode_store (i.e. we can check the result after writing to the
> sysfs file).

Okay. I'll drop it.

thanks,

-- 
heikki