* [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts
@ 2023-08-22 13:32 Heikki Krogerus
2023-08-22 13:32 ` [RFC PATCH 1/2] usb: typec: Link enumerated USB devices with Type-C partner Heikki Krogerus
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Heikki Krogerus @ 2023-08-22 13:32 UTC (permalink / raw
To: Benson Leung; +Cc: Jameson Thies, Prashant Malani, Won Chung, linux-usb
Hi Benson,
RFC for now. I can't test these properly. If you guys could take over
this, I would much appreciated. I hope this is at least close to your
proposal.
With this (or something like it) you should be able to get
notification about USB connections and disconnections to your port
driver by implementing the new "attach" and "deattach" callbacks in
struct typec_partner_desc. The typec partner devices will also have
symlinks to the enumerated USB devices and vise versa.
I took a little shortcut and did not implement a proper device list.
Instead there is now only a member for the USB2 device and a member
for the USB3 device in struct typec_port, so with this only USB is
supported. But the API does not deal with struct usb_device, so
extending this to support other devices (TBT, Displayport, etc.) by
adding the actual device list should be fairly easy.
thanks,
Heikki Krogerus (2):
usb: typec: Link enumerated USB devices with Type-C partner
usb: Inform the USB Type-C class about enumerated devices
drivers/usb/core/hub.c | 4 ++
drivers/usb/core/hub.h | 3 +
drivers/usb/core/port.c | 19 +++++-
drivers/usb/typec/class.c | 108 +++++++++++++++++++++++++++++---
drivers/usb/typec/class.h | 16 +++++
drivers/usb/typec/port-mapper.c | 9 ++-
include/linux/usb/typec.h | 37 +++++++++++
7 files changed, 184 insertions(+), 12 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 1/2] usb: typec: Link enumerated USB devices with Type-C partner
2023-08-22 13:32 [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts Heikki Krogerus
@ 2023-08-22 13:32 ` Heikki Krogerus
2023-08-22 13:32 ` [RFC PATCH 2/2] usb: Inform the USB Type-C class about enumerated devices Heikki Krogerus
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2023-08-22 13:32 UTC (permalink / raw
To: Benson Leung; +Cc: Jameson Thies, Prashant Malani, Won Chung, linux-usb
Adding functions that USB hub code can use to inform the
Type-C class about connected USB devices.
Once taken into use, it will allow the Type-C port drivers
to power off components that are not needed, for example if
USB2 device is enumerated, everything that is only relevant
for USB3 (retimers, etc.), can be powered off.
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
drivers/usb/typec/class.c | 108 +++++++++++++++++++++++++++++---
drivers/usb/typec/class.h | 16 +++++
drivers/usb/typec/port-mapper.c | 9 ++-
include/linux/usb/typec.h | 37 +++++++++++
4 files changed, 160 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 9c1dbf3c00e0..2e0451bd336e 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -13,6 +13,7 @@
#include <linux/usb/pd_vdo.h>
#include <linux/usb/typec_mux.h>
#include <linux/usb/typec_retimer.h>
+#include <linux/usb.h>
#include "bus.h"
#include "class.h"
@@ -681,6 +682,33 @@ const struct device_type typec_partner_dev_type = {
.release = typec_partner_release,
};
+static void typec_partner_link_device(struct typec_partner *partner, struct device *dev)
+{
+ int ret;
+
+ ret = sysfs_create_link(&dev->kobj, &partner->dev.kobj, "typec");
+ if (ret)
+ return;
+
+ ret = sysfs_create_link(&partner->dev.kobj, &dev->kobj, dev_name(dev));
+ if (ret) {
+ sysfs_remove_link(&dev->kobj, "typec");
+ return;
+ }
+
+ if (partner->attach)
+ partner->attach(partner, dev);
+}
+
+static void typec_partner_unlink_device(struct typec_partner *partner, struct device *dev)
+{
+ sysfs_remove_link(&partner->dev.kobj, dev_name(dev));
+ sysfs_remove_link(&dev->kobj, "typec");
+
+ if (partner->deattach)
+ partner->deattach(partner, dev);
+}
+
/**
* typec_partner_set_identity - Report result from Discover Identity command
* @partner: The partner updated identity values
@@ -865,6 +893,8 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
partner->num_altmodes = -1;
partner->pd_revision = desc->pd_revision;
partner->svdm_version = port->cap->svdm_version;
+ partner->attach = desc->attach;
+ partner->deattach = desc->deattach;
if (desc->identity) {
/*
@@ -887,6 +917,11 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
return ERR_PTR(ret);
}
+ if (port->usb2_dev)
+ typec_partner_link_device(partner, port->usb2_dev);
+ if (port->usb3_dev)
+ typec_partner_link_device(partner, port->usb3_dev);
+
return partner;
}
EXPORT_SYMBOL_GPL(typec_register_partner);
@@ -899,8 +934,19 @@ EXPORT_SYMBOL_GPL(typec_register_partner);
*/
void typec_unregister_partner(struct typec_partner *partner)
{
- if (!IS_ERR_OR_NULL(partner))
- device_unregister(&partner->dev);
+ struct typec_port *port;
+
+ if (IS_ERR_OR_NULL(partner))
+ return;
+
+ port = to_typec_port(partner->dev.parent);
+
+ if (port->usb2_dev)
+ typec_partner_unlink_device(partner, port->usb2_dev);
+ if (port->usb3_dev)
+ typec_partner_unlink_device(partner, port->usb3_dev);
+
+ device_unregister(&partner->dev);
}
EXPORT_SYMBOL_GPL(typec_unregister_partner);
@@ -1775,6 +1821,50 @@ static int partner_match(struct device *dev, void *data)
return is_typec_partner(dev);
}
+static struct typec_partner *typec_get_partner(struct typec_port *port)
+{
+ struct device *dev;
+
+ dev = device_find_child(&port->dev, NULL, partner_match);
+ if (!dev)
+ return NULL;
+
+ return to_typec_partner(dev);
+}
+
+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);
+
+ if (udev->speed < USB_SPEED_SUPER)
+ port->usb2_dev = dev;
+ else
+ port->usb3_dev = dev;
+
+ if (partner) {
+ typec_partner_link_device(partner, dev);
+ put_device(&partner->dev);
+ }
+}
+
+static void typec_partner_deattach(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);
+
+ if (partner) {
+ typec_partner_unlink_device(partner, dev);
+ put_device(&partner->dev);
+ }
+
+ if (port->usb2_dev == dev)
+ port->usb2_dev = NULL;
+ else if (port->usb3_dev == dev)
+ port->usb3_dev = NULL;
+}
+
/**
* typec_set_data_role - Report data role change
* @port: The USB Type-C Port where the role was changed
@@ -1784,7 +1874,7 @@ static int partner_match(struct device *dev, void *data)
*/
void typec_set_data_role(struct typec_port *port, enum typec_data_role role)
{
- struct device *partner_dev;
+ struct typec_partner *partner;
if (port->data_role == role)
return;
@@ -1793,14 +1883,14 @@ void typec_set_data_role(struct typec_port *port, enum typec_data_role role)
sysfs_notify(&port->dev.kobj, NULL, "data_role");
kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
- partner_dev = device_find_child(&port->dev, NULL, partner_match);
- if (!partner_dev)
+ partner = typec_get_partner(port);
+ if (!partner)
return;
- if (to_typec_partner(partner_dev)->identity)
- typec_product_type_notify(partner_dev);
+ if (partner->identity)
+ typec_product_type_notify(&partner->dev);
- put_device(partner_dev);
+ put_device(&partner->dev);
}
EXPORT_SYMBOL_GPL(typec_set_data_role);
@@ -2251,6 +2341,8 @@ struct typec_port *typec_register_port(struct device *parent,
port->ops = cap->ops;
port->port_type = cap->type;
port->prefer_role = cap->prefer_role;
+ port->con.attach = typec_partner_attach;
+ port->con.deattach = typec_partner_deattach;
device_initialize(&port->dev);
port->dev.class = &typec_class;
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
index 673b2952b074..c36761ba3f59 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -8,6 +8,7 @@
struct typec_mux;
struct typec_switch;
+struct usb_device;
struct typec_plug {
struct device dev;
@@ -35,6 +36,9 @@ struct typec_partner {
enum usb_pd_svdm_ver svdm_version;
struct usb_power_delivery *pd;
+
+ void (*attach)(struct typec_partner *partner, struct device *dev);
+ void (*deattach)(struct typec_partner *partner, struct device *dev);
};
struct typec_port {
@@ -59,6 +63,18 @@ struct typec_port {
const struct typec_capability *cap;
const struct typec_operations *ops;
+
+ struct typec_connector con;
+
+ /*
+ * REVISIT: Only USB devices for now. If there are others, these need to
+ * be converted into a list.
+ *
+ * NOTE: These may be registered first before the typec_partner, so they
+ * will always have to be kept here instead of struct typec_partner.
+ */
+ struct device *usb2_dev;
+ struct device *usb3_dev;
};
#define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
diff --git a/drivers/usb/typec/port-mapper.c b/drivers/usb/typec/port-mapper.c
index a929e000d0e2..d42da5720a25 100644
--- a/drivers/usb/typec/port-mapper.c
+++ b/drivers/usb/typec/port-mapper.c
@@ -8,17 +8,22 @@
#include <linux/acpi.h>
#include <linux/component.h>
+#include <linux/usb.h>
#include "class.h"
static int typec_aggregate_bind(struct device *dev)
{
- return component_bind_all(dev, NULL);
+ struct typec_port *port = to_typec_port(dev);
+
+ return component_bind_all(dev, &port->con);
}
static void typec_aggregate_unbind(struct device *dev)
{
- component_unbind_all(dev, NULL);
+ struct typec_port *port = to_typec_port(dev);
+
+ component_unbind_all(dev, &port->con);
}
static const struct component_master_ops typec_aggregate_ops = {
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 8fa781207970..a05d6f6f2536 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -202,6 +202,8 @@ struct typec_cable_desc {
* @accessory: Audio, Debug or none.
* @identity: Discover Identity command data
* @pd_revision: USB Power Delivery Specification Revision if supported
+ * @attach: Notification about attached USB device
+ * @deattach: Notification about removed USB device
*
* Details about a partner that is attached to USB Type-C port. If @identity
* member exists when partner is registered, a directory named "identity" is
@@ -217,6 +219,9 @@ struct typec_partner_desc {
enum typec_accessory accessory;
struct usb_pd_identity *identity;
u16 pd_revision; /* 0300H = "3.0" */
+
+ void (*attach)(struct typec_partner *partner, struct device *dev);
+ void (*deattach)(struct typec_partner *partner, struct device *dev);
};
/**
@@ -335,4 +340,36 @@ 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);
+/**
+ * struct typec_connector - Representation of Type-C port for external drivers
+ * @attach: notification about device removal
+ * @deattach: notification about device removal
+ *
+ * Drivers that control the USB and other ports (DisplayPorts, etc.), that are
+ * connected to the Type-C connectors, can use these callbacks to inform the
+ * Type-C connector class about connections and disconnections. That information
+ * can then be used by the typec-port drivers to power on or off parts that are
+ * needed or not needed - as an example, in USB mode if USB2 device is
+ * enumerated, USB3 components (retimers, phys, and what have you) do not need
+ * to be powered on.
+ *
+ * The attached (enumerated) devices will be liked with the typec-partner device.
+ */
+struct typec_connector {
+ void (*attach)(struct typec_connector *con, struct device *dev);
+ void (*deattach)(struct typec_connector *con, struct device *dev);
+};
+
+static inline void typec_attach(struct typec_connector *con, struct device *dev)
+{
+ if (con && con->attach)
+ con->attach(con, dev);
+}
+
+static inline void typec_deattach(struct typec_connector *con, struct device *dev)
+{
+ if (con && con->deattach)
+ con->deattach(con, dev);
+}
+
#endif /* __LINUX_USB_TYPEC_H */
--
2.40.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH 2/2] usb: Inform the USB Type-C class about enumerated devices
2023-08-22 13:32 [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts Heikki Krogerus
2023-08-22 13:32 ` [RFC PATCH 1/2] usb: typec: Link enumerated USB devices with Type-C partner Heikki Krogerus
@ 2023-08-22 13:32 ` Heikki Krogerus
2023-08-22 13:58 ` [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts Benson Leung
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2023-08-22 13:32 UTC (permalink / raw
To: Benson Leung; +Cc: Jameson Thies, Prashant Malani, Won Chung, linux-usb
The Type-C port drivers can make PM related decitions based
on is the device USB3 or USB2.
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
drivers/usb/core/hub.c | 4 ++++
drivers/usb/core/hub.h | 3 +++
drivers/usb/core/port.c | 19 +++++++++++++++++--
3 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3c54b218301c..53f5b902ce7d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2266,6 +2266,8 @@ void usb_disconnect(struct usb_device **pdev)
*/
if (!test_and_set_bit(port1, hub->child_usage_bits))
pm_runtime_get_sync(&port_dev->dev);
+
+ typec_deattach(port_dev->connector, &udev->dev);
}
usb_remove_ep_devs(&udev->ep0);
@@ -2612,6 +2614,8 @@ int usb_new_device(struct usb_device *udev)
if (!test_and_set_bit(port1, hub->child_usage_bits))
pm_runtime_get_sync(&port_dev->dev);
+
+ typec_attach(port_dev->connector, &udev->dev);
}
(void) usb_create_ep_devs(&udev->dev, &udev->ep0, udev);
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 37897afd1b64..bba7b40173dd 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -14,6 +14,7 @@
#include <linux/usb.h>
#include <linux/usb/ch11.h>
#include <linux/usb/hcd.h>
+#include <linux/usb/typec.h>
#include "usb.h"
struct usb_hub {
@@ -82,6 +83,7 @@ struct usb_hub {
* @dev: generic device interface
* @port_owner: port's owner
* @peer: related usb2 and usb3 ports (share the same connector)
+ * @connector: USB Type-C connector
* @req: default pm qos request for hubs without port power control
* @connect_type: port's connect type
* @state: device state of the usb device attached to the port
@@ -100,6 +102,7 @@ struct usb_port {
struct device dev;
struct usb_dev_state *port_owner;
struct usb_port *peer;
+ struct typec_connector *connector;
struct dev_pm_qos_request *req;
enum usb_port_connect_type connect_type;
enum usb_device_state state;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 77be0dc28da9..c17e6edf0ed6 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -653,6 +653,7 @@ static void find_and_link_peer(struct usb_hub *hub, int port1)
static int connector_bind(struct device *dev, struct device *connector, void *data)
{
+ struct usb_port *port_dev = to_usb_port(dev);
int ret;
ret = sysfs_create_link(&dev->kobj, &connector->kobj, "connector");
@@ -660,16 +661,30 @@ static int connector_bind(struct device *dev, struct device *connector, void *da
return ret;
ret = sysfs_create_link(&connector->kobj, &dev->kobj, dev_name(dev));
- if (ret)
+ if (ret) {
sysfs_remove_link(&dev->kobj, "connector");
+ return ret;
+ }
+
+ port_dev->connector = data;
+
+ /*
+ * If the USB device was enumerated before this is called, infoming
+ * the Type-C connector about that device separately.
+ */
+ if (port_dev->child)
+ typec_attach(port_dev->connector, &port_dev->child->dev);
- return ret;
+ return 0;
}
static void connector_unbind(struct device *dev, struct device *connector, void *data)
{
+ struct usb_port *port_dev = to_usb_port(dev);
+
sysfs_remove_link(&connector->kobj, dev_name(dev));
sysfs_remove_link(&dev->kobj, "connector");
+ port_dev->connector = NULL;
}
static const struct component_ops connector_ops = {
--
2.40.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts
2023-08-22 13:32 [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts Heikki Krogerus
2023-08-22 13:32 ` [RFC PATCH 1/2] usb: typec: Link enumerated USB devices with Type-C partner Heikki Krogerus
2023-08-22 13:32 ` [RFC PATCH 2/2] usb: Inform the USB Type-C class about enumerated devices Heikki Krogerus
@ 2023-08-22 13:58 ` Benson Leung
2023-08-22 14:52 ` Douglas Gilbert
2023-08-23 0:24 ` Benson Leung
4 siblings, 0 replies; 12+ messages in thread
From: Benson Leung @ 2023-08-22 13:58 UTC (permalink / raw
To: Heikki Krogerus; +Cc: Jameson Thies, Prashant Malani, Won Chung, linux-usb
Hi Heikki,
On Tue, Aug 22, 2023 at 6:32 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Benson,
>
> RFC for now. I can't test these properly. If you guys could take over
> this, I would much appreciated. I hope this is at least close to your
> proposal.
Much appreciated for the quick turnaround. Yes, I can test this on our
systems that have subsystem linking enabled. (speaking of which, would
be interested in getting a sample of one of our Chromebook systems
that fully implement the typec subsystem linking)?
>
> With this (or something like it) you should be able to get
> notification about USB connections and disconnections to your port
> driver by implementing the new "attach" and "deattach" callbacks in
> struct typec_partner_desc. The typec partner devices will also have
> symlinks to the enumerated USB devices and vise versa.
>
Got it, i'll modify the cros_ec_typec driver to implement these
callbacks, and look for the new symlinks.
> I took a little shortcut and did not implement a proper device list.
> Instead there is now only a member for the USB2 device and a member
> for the USB3 device in struct typec_port, so with this only USB is
> supported. But the API does not deal with struct usb_device, so
> extending this to support other devices (TBT, Displayport, etc.) by
> adding the actual device list should be fairly easy.
Excellent! Thank you.
Benson
>
> thanks,
>
> Heikki Krogerus (2):
> usb: typec: Link enumerated USB devices with Type-C partner
> usb: Inform the USB Type-C class about enumerated devices
>
> drivers/usb/core/hub.c | 4 ++
> drivers/usb/core/hub.h | 3 +
> drivers/usb/core/port.c | 19 +++++-
> drivers/usb/typec/class.c | 108 +++++++++++++++++++++++++++++---
> drivers/usb/typec/class.h | 16 +++++
> drivers/usb/typec/port-mapper.c | 9 ++-
> include/linux/usb/typec.h | 37 +++++++++++
> 7 files changed, 184 insertions(+), 12 deletions(-)
>
> --
> 2.40.1
>
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts
2023-08-22 13:32 [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts Heikki Krogerus
` (2 preceding siblings ...)
2023-08-22 13:58 ` [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts Benson Leung
@ 2023-08-22 14:52 ` Douglas Gilbert
2023-08-23 8:56 ` Heikki Krogerus
2023-08-23 0:24 ` Benson Leung
4 siblings, 1 reply; 12+ messages in thread
From: Douglas Gilbert @ 2023-08-22 14:52 UTC (permalink / raw
To: Heikki Krogerus, Benson Leung
Cc: Jameson Thies, Prashant Malani, Won Chung, linux-usb
On 2023-08-22 09:32, Heikki Krogerus wrote:
> Hi Benson,
>
> RFC for now. I can't test these properly. If you guys could take over
> this, I would much appreciated. I hope this is at least close to your
> proposal.
>
> With this (or something like it) you should be able to get
> notification about USB connections and disconnections to your port
> driver by implementing the new "attach" and "deattach" callbacks in
> struct typec_partner_desc. The typec partner devices will also have
> symlinks to the enumerated USB devices and vise versa.
>
> I took a little shortcut and did not implement a proper device list.
> Instead there is now only a member for the USB2 device and a member
> for the USB3 device in struct typec_port, so with this only USB is
> supported. But the API does not deal with struct usb_device, so
> extending this to support other devices (TBT, Displayport, etc.) by
> adding the actual device list should be fairly easy.
On a related matter, I wonder why there aren't symlinks between typec ports
(under /sys/class/typec ) and/or the corresponding pd objects (under
/sys/class/usb_power_delivery ) to the related power_supply objects under
/sys/class/power_supply . For example under the latter directory I see:
$ ls | more
AC
BAT0
hidpp_battery_1
ucsi-source-psy-USBC000:001
ucsi-source-psy-USBC000:002
Those last two power supplies are obviously connected to typec port0 and port1
(but offset by 1). Those power_supply objects hold inaccurate data which I hope
will improve in time. Significantly power_supply objects don't seem to report
the direction of the power. Here is a little utility I have been working on
to report the USB Type-C port/pd disposition on my machine:
$ lsucpd
port0 [pd0] > {5V, 0.9A}
port1 [pd1] <<=== partner: [pd8]
My laptop (Thinkpad X13 G3) has two type-C ports and port1 is a sink with a
PD contract. I would like that second line to have 20V, 3.25A appended to it
but there are several issues:
- no typec or pd symlink to ucsi-source-psy-USBC000:002
- that power supply_object says it is online (correct) with a voltage_now:
5000000 uV (incorrect) and current_now: 3000000 uA (incorrect). See below.
ucsi-source-psy-USBC000:002 $ ls_name_value
current_max : 3250000
current_now : 3000000
online : 1
scope : Unknown
type : USB
uevent : <removed>
usb_type : C [PD] PD_PPS
voltage_max : 20000000
voltage_min : 5000000
voltage_now : 5000000
Doug Gilbert
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts
2023-08-22 13:32 [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts Heikki Krogerus
` (3 preceding siblings ...)
2023-08-22 14:52 ` Douglas Gilbert
@ 2023-08-23 0:24 ` Benson Leung
2023-08-23 10:01 ` Heikki Krogerus
4 siblings, 1 reply; 12+ messages in thread
From: Benson Leung @ 2023-08-23 0:24 UTC (permalink / raw
To: Heikki Krogerus; +Cc: Jameson Thies, Prashant Malani, Won Chung, linux-usb
Hi Heikki,
On Tue, Aug 22, 2023 at 6:32 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Benson,
>
> RFC for now. I can't test these properly. If you guys could take over
> this, I would much appreciated. I hope this is at least close to your
> proposal.
>
> With this (or something like it) you should be able to get
> notification about USB connections and disconnections to your port
> driver by implementing the new "attach" and "deattach" callbacks in
> struct typec_partner_desc. The typec partner devices will also have
> symlinks to the enumerated USB devices and vise versa.
>
> I took a little shortcut and did not implement a proper device list.
> Instead there is now only a member for the USB2 device and a member
> for the USB3 device in struct typec_port, so with this only USB is
> supported. But the API does not deal with struct usb_device, so
> extending this to support other devices (TBT, Displayport, etc.) by
> adding the actual device list should be fairly easy.
>
> thanks,
>
> Heikki Krogerus (2):
> usb: typec: Link enumerated USB devices with Type-C partner
> usb: Inform the USB Type-C class about enumerated devices
>
> drivers/usb/core/hub.c | 4 ++
> drivers/usb/core/hub.h | 3 +
> drivers/usb/core/port.c | 19 +++++-
> drivers/usb/typec/class.c | 108 +++++++++++++++++++++++++++++---
> drivers/usb/typec/class.h | 16 +++++
> drivers/usb/typec/port-mapper.c | 9 ++-
> include/linux/usb/typec.h | 37 +++++++++++
> 7 files changed, 184 insertions(+), 12 deletions(-)
>
> --
> 2.40.1
>
Tested-by: Benson Leung <bleung@chromium.org>
I picked these two changes back to my Brya/Redrix Chromebook which has
the PLD changes to link subsystems.
First I plugged in a USB-C to USB-A receptacle adapter with a USB3
thumbdrive into port0, and went to the port0-partner path.
redrix-rev3 /sys/class/typec/port0-partner # ls -lh
total 0
lrwxrwxrwx. 1 root root 0 Aug 22 17:16 2-1 ->
../../../../../../../0000:00:0d.0/usb2/2-1
-r--r--r--. 1 root root 4.0K Aug 22 17:14 accessory_mode
lrwxrwxrwx. 1 root root 0 Aug 22 17:14 device -> ../../port0
drwxr-xr-x. 2 root root 0 Aug 22 17:14 identity
drwxr-xr-x. 2 root root 0 Aug 22 17:14 power
lrwxrwxrwx. 1 root root 0 Aug 22 17:14 subsystem ->
../../../../../../../../../class/typec
-r--r--r--. 1 root root 4.0K Aug 22 17:14 supports_usb_power_delivery
-r--r--r--. 1 root root 4.0K Aug 22 17:14 type
-rw-r--r--. 1 root root 4.0K Aug 22 17:14 uevent
-r--r--r--. 1 root root 4.0K Aug 22 17:14 usb_power_delivery_revision
2-1 symlink appears, which is the SuperSpeed usb device associated
with the thumbdrive.
Unplugging the USB3 thumbdrive without unplugging the C-to-A adapter,
and then plugging in a USB2.0 security key:
redrix-rev3 /sys/class/typec/port0-partner # ls -lh
total 0
lrwxrwxrwx. 1 root root 0 Aug 22 17:19 3-1 ->
../../../../../../../0000:00:14.0/usb3/3-1
-r--r--r--. 1 root root 4.0K Aug 22 17:14 accessory_mode
lrwxrwxrwx. 1 root root 0 Aug 22 17:14 device -> ../../port0
drwxr-xr-x. 2 root root 0 Aug 22 17:14 identity
drwxr-xr-x. 2 root root 0 Aug 22 17:14 power
lrwxrwxrwx. 1 root root 0 Aug 22 17:14 subsystem ->
../../../../../../../../../class/typec
-r--r--r--. 1 root root 4.0K Aug 22 17:14 supports_usb_power_delivery
-r--r--r--. 1 root root 4.0K Aug 22 17:14 type
-rw-r--r--. 1 root root 4.0K Aug 22 17:14 uevent
-r--r--r--. 1 root root 4.0K Aug 22 17:14 usb_power_delivery_revision
2-1 node disappears. 3-1 appears
Unplugging the adapter, plugging in a USB4 hub:
redrix-rev3 /sys/class/typec/port0-partner # ls -lh
total 0
lrwxrwxrwx. 1 root root 0 Aug 22 17:21 2-1 ->
../../../../../../../0000:00:0d.0/usb2/2-1
lrwxrwxrwx. 1 root root 0 Aug 22 17:21 3-1 ->
../../../../../../../0000:00:14.0/usb3/3-1
-r--r--r--. 1 root root 4.0K Aug 22 17:21 accessory_mode
lrwxrwxrwx. 1 root root 0 Aug 22 17:21 device -> ../../port0
drwxr-xr-x. 2 root root 0 Aug 22 17:21 identity
-r--r--r--. 1 root root 4.0K Aug 22 17:21 number_of_alternate_modes
drwxr-xr-x. 5 root root 0 Aug 22 17:21 pd0
drwxr-xr-x. 4 root root 0 Aug 22 17:21 port0-partner.0
drwxr-xr-x. 2 root root 0 Aug 22 17:21 power
lrwxrwxrwx. 1 root root 0 Aug 22 17:21 subsystem ->
../../../../../../../../../class/typec
-r--r--r--. 1 root root 4.0K Aug 22 17:21 supports_usb_power_delivery
-r--r--r--. 1 root root 4.0K Aug 22 17:21 type
-rw-r--r--. 1 root root 4.0K Aug 22 17:21 uevent
lrwxrwxrwx. 1 root root 0 Aug 22 17:21 usb_power_delivery -> pd0
-r--r--r--. 1 root root 4.0K Aug 22 17:21 usb_power_delivery_revision
Both 2-1 and 3-1 are linked.
Thanks so much for this, Heikki! I can look a little closer at the
attach and deattach callbacks in our typec port driver in a little
while.
Benson
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts
2023-08-22 14:52 ` Douglas Gilbert
@ 2023-08-23 8:56 ` Heikki Krogerus
2023-08-24 16:51 ` Douglas Gilbert
0 siblings, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2023-08-23 8:56 UTC (permalink / raw
To: Douglas Gilbert
Cc: Benson Leung, Jameson Thies, Prashant Malani, Won Chung,
linux-usb
Hi Douglas,
On Tue, Aug 22, 2023 at 10:52:12AM -0400, Douglas Gilbert wrote:
> On 2023-08-22 09:32, Heikki Krogerus wrote:
> On a related matter, I wonder why there aren't symlinks between typec ports
> (under /sys/class/typec ) and/or the corresponding pd objects (under
> /sys/class/usb_power_delivery ) to the related power_supply objects under
> /sys/class/power_supply . For example under the latter directory I see:
> $ ls | more
> AC
> BAT0
> hidpp_battery_1
> ucsi-source-psy-USBC000:001
> ucsi-source-psy-USBC000:002
>
> Those last two power supplies are obviously connected to typec port0 and port1
> (but offset by 1). Those power_supply objects hold inaccurate data which I hope
> will improve in time. Significantly power_supply objects don't seem to report
> the direction of the power. Here is a little utility I have been working on
> to report the USB Type-C port/pd disposition on my machine:
> $ lsucpd
> port0 [pd0] > {5V, 0.9A}
> port1 [pd1] <<=== partner: [pd8]
>
> My laptop (Thinkpad X13 G3) has two type-C ports and port1 is a sink with a
> PD contract. I would like that second line to have 20V, 3.25A appended to it
> but there are several issues:
> - no typec or pd symlink to ucsi-source-psy-USBC000:002
> - that power supply_object says it is online (correct) with a voltage_now:
> 5000000 uV (incorrect) and current_now: 3000000 uA (incorrect). See below.
>
> ucsi-source-psy-USBC000:002 $ ls_name_value
> current_max : 3250000
> current_now : 3000000
> online : 1
> scope : Unknown
> type : USB
> uevent : <removed>
> usb_type : C [PD] PD_PPS
> voltage_max : 20000000
> voltage_min : 5000000
> voltage_now : 5000000
I'm glad you brought that up. The major problem with the Type-C power
supplies is that the Type-C connector class does not actually take
care of them. They are all registered by the device drivers, and all
of them seem to expose different kind of information. In your case the
power supplies are registered by the UCSI driver, and the above may
indicate a bug in that driver.
To improve the situation, I originally proposed that instead of
adding a separate device class for USB Power Delivery objects, we
would utilise the already existing power supply class. That proposal
was not seen acceptable by many (including Benson if I recall), and I
now tend to agree with that because of several reasons, starting from
the fact that USB PD objects supply other informations on top of power
delivery details (so completely unrelated to PM).
Even before that I had proposed that the Type-C connector class could
supply API for the drivers to take care of the registration of the
power supplies. I proposed that not only the Type-C ports should
register the power supplies but also the partners should represent
their own power supplies. That would make things much more clear for
the user space IMO. The port and partner would always create a "chain"
of supplies where the other is the supply the other the sink of power.
That is already supported by the power supply class. For some reason
this proposal was also not seen as a good idea at the time, but it may
be that I just failed to explain it properly.
Nevertheless, I still think that that is exactly how the Type-C power
supplies should be always presented - separate supplies for both ports
and partners - and that obviously the Type-C connector class should
take care of those supplies so that they always supply the same
information. Unfortunately I do not have any time at the moment to
work on this right now.
Br,
--
heikki
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts
2023-08-23 0:24 ` Benson Leung
@ 2023-08-23 10:01 ` Heikki Krogerus
0 siblings, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2023-08-23 10:01 UTC (permalink / raw
To: Benson Leung; +Cc: Jameson Thies, Prashant Malani, Won Chung, linux-usb
On Tue, Aug 22, 2023 at 05:24:44PM -0700, Benson Leung wrote:
> Hi Heikki,
>
> On Tue, Aug 22, 2023 at 6:32 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi Benson,
> >
> > RFC for now. I can't test these properly. If you guys could take over
> > this, I would much appreciated. I hope this is at least close to your
> > proposal.
> >
> > With this (or something like it) you should be able to get
> > notification about USB connections and disconnections to your port
> > driver by implementing the new "attach" and "deattach" callbacks in
> > struct typec_partner_desc. The typec partner devices will also have
> > symlinks to the enumerated USB devices and vise versa.
> >
> > I took a little shortcut and did not implement a proper device list.
> > Instead there is now only a member for the USB2 device and a member
> > for the USB3 device in struct typec_port, so with this only USB is
> > supported. But the API does not deal with struct usb_device, so
> > extending this to support other devices (TBT, Displayport, etc.) by
> > adding the actual device list should be fairly easy.
> >
> > thanks,
> >
> > Heikki Krogerus (2):
> > usb: typec: Link enumerated USB devices with Type-C partner
> > usb: Inform the USB Type-C class about enumerated devices
> >
> > drivers/usb/core/hub.c | 4 ++
> > drivers/usb/core/hub.h | 3 +
> > drivers/usb/core/port.c | 19 +++++-
> > drivers/usb/typec/class.c | 108 +++++++++++++++++++++++++++++---
> > drivers/usb/typec/class.h | 16 +++++
> > drivers/usb/typec/port-mapper.c | 9 ++-
> > include/linux/usb/typec.h | 37 +++++++++++
> > 7 files changed, 184 insertions(+), 12 deletions(-)
> >
> > --
> > 2.40.1
> >
>
> Tested-by: Benson Leung <bleung@chromium.org>
>
>
> I picked these two changes back to my Brya/Redrix Chromebook which has
> the PLD changes to link subsystems.
>
> First I plugged in a USB-C to USB-A receptacle adapter with a USB3
> thumbdrive into port0, and went to the port0-partner path.
>
> redrix-rev3 /sys/class/typec/port0-partner # ls -lh
> total 0
> lrwxrwxrwx. 1 root root 0 Aug 22 17:16 2-1 ->
> ../../../../../../../0000:00:0d.0/usb2/2-1
> -r--r--r--. 1 root root 4.0K Aug 22 17:14 accessory_mode
> lrwxrwxrwx. 1 root root 0 Aug 22 17:14 device -> ../../port0
> drwxr-xr-x. 2 root root 0 Aug 22 17:14 identity
> drwxr-xr-x. 2 root root 0 Aug 22 17:14 power
> lrwxrwxrwx. 1 root root 0 Aug 22 17:14 subsystem ->
> ../../../../../../../../../class/typec
> -r--r--r--. 1 root root 4.0K Aug 22 17:14 supports_usb_power_delivery
> -r--r--r--. 1 root root 4.0K Aug 22 17:14 type
> -rw-r--r--. 1 root root 4.0K Aug 22 17:14 uevent
> -r--r--r--. 1 root root 4.0K Aug 22 17:14 usb_power_delivery_revision
>
> 2-1 symlink appears, which is the SuperSpeed usb device associated
> with the thumbdrive.
> Unplugging the USB3 thumbdrive without unplugging the C-to-A adapter,
> and then plugging in a USB2.0 security key:
>
> redrix-rev3 /sys/class/typec/port0-partner # ls -lh
> total 0
> lrwxrwxrwx. 1 root root 0 Aug 22 17:19 3-1 ->
> ../../../../../../../0000:00:14.0/usb3/3-1
> -r--r--r--. 1 root root 4.0K Aug 22 17:14 accessory_mode
> lrwxrwxrwx. 1 root root 0 Aug 22 17:14 device -> ../../port0
> drwxr-xr-x. 2 root root 0 Aug 22 17:14 identity
> drwxr-xr-x. 2 root root 0 Aug 22 17:14 power
> lrwxrwxrwx. 1 root root 0 Aug 22 17:14 subsystem ->
> ../../../../../../../../../class/typec
> -r--r--r--. 1 root root 4.0K Aug 22 17:14 supports_usb_power_delivery
> -r--r--r--. 1 root root 4.0K Aug 22 17:14 type
> -rw-r--r--. 1 root root 4.0K Aug 22 17:14 uevent
> -r--r--r--. 1 root root 4.0K Aug 22 17:14 usb_power_delivery_revision
>
> 2-1 node disappears. 3-1 appears
>
> Unplugging the adapter, plugging in a USB4 hub:
> redrix-rev3 /sys/class/typec/port0-partner # ls -lh
> total 0
> lrwxrwxrwx. 1 root root 0 Aug 22 17:21 2-1 ->
> ../../../../../../../0000:00:0d.0/usb2/2-1
> lrwxrwxrwx. 1 root root 0 Aug 22 17:21 3-1 ->
> ../../../../../../../0000:00:14.0/usb3/3-1
> -r--r--r--. 1 root root 4.0K Aug 22 17:21 accessory_mode
> lrwxrwxrwx. 1 root root 0 Aug 22 17:21 device -> ../../port0
> drwxr-xr-x. 2 root root 0 Aug 22 17:21 identity
> -r--r--r--. 1 root root 4.0K Aug 22 17:21 number_of_alternate_modes
> drwxr-xr-x. 5 root root 0 Aug 22 17:21 pd0
> drwxr-xr-x. 4 root root 0 Aug 22 17:21 port0-partner.0
> drwxr-xr-x. 2 root root 0 Aug 22 17:21 power
> lrwxrwxrwx. 1 root root 0 Aug 22 17:21 subsystem ->
> ../../../../../../../../../class/typec
> -r--r--r--. 1 root root 4.0K Aug 22 17:21 supports_usb_power_delivery
> -r--r--r--. 1 root root 4.0K Aug 22 17:21 type
> -rw-r--r--. 1 root root 4.0K Aug 22 17:21 uevent
> lrwxrwxrwx. 1 root root 0 Aug 22 17:21 usb_power_delivery -> pd0
> -r--r--r--. 1 root root 4.0K Aug 22 17:21 usb_power_delivery_revision
>
> Both 2-1 and 3-1 are linked.
>
> Thanks so much for this, Heikki! I can look a little closer at the
> attach and deattach callbacks in our typec port driver in a little
> while.
Cool! Thank you!
--
heikki
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts
2023-08-23 8:56 ` Heikki Krogerus
@ 2023-08-24 16:51 ` Douglas Gilbert
2023-08-28 9:21 ` Heikki Krogerus
0 siblings, 1 reply; 12+ messages in thread
From: Douglas Gilbert @ 2023-08-24 16:51 UTC (permalink / raw
To: Heikki Krogerus
Cc: Benson Leung, Jameson Thies, Prashant Malani, Won Chung,
linux-usb
On 2023-08-23 04:56, Heikki Krogerus wrote:
> Hi Douglas,
>
> On Tue, Aug 22, 2023 at 10:52:12AM -0400, Douglas Gilbert wrote:
>> On 2023-08-22 09:32, Heikki Krogerus wrote:
>> On a related matter, I wonder why there aren't symlinks between typec ports
>> (under /sys/class/typec ) and/or the corresponding pd objects (under
>> /sys/class/usb_power_delivery ) to the related power_supply objects under
>> /sys/class/power_supply . For example under the latter directory I see:
>> $ ls | more
>> AC
>> BAT0
>> hidpp_battery_1
>> ucsi-source-psy-USBC000:001
>> ucsi-source-psy-USBC000:002
>>
>> Those last two power supplies are obviously connected to typec port0 and port1
>> (but offset by 1). Those power_supply objects hold inaccurate data which I hope
>> will improve in time. Significantly power_supply objects don't seem to report
>> the direction of the power. Here is a little utility I have been working on
>> to report the USB Type-C port/pd disposition on my machine:
>> $ lsucpd
>> port0 [pd0] > {5V, 0.9A}
>> port1 [pd1] <<=== partner: [pd8]
>>
>> My laptop (Thinkpad X13 G3) has two type-C ports and port1 is a sink with a
>> PD contract. I would like that second line to have 20V, 3.25A appended to it
>> but there are several issues:
>> - no typec or pd symlink to ucsi-source-psy-USBC000:002
>> - that power supply_object says it is online (correct) with a voltage_now:
>> 5000000 uV (incorrect) and current_now: 3000000 uA (incorrect). See below.
>>
>> ucsi-source-psy-USBC000:002 $ ls_name_value
>> current_max : 3250000
>> current_now : 3000000
>> online : 1
>> scope : Unknown
>> type : USB
>> uevent : <removed>
>> usb_type : C [PD] PD_PPS
>> voltage_max : 20000000
>> voltage_min : 5000000
>> voltage_now : 5000000
>
> I'm glad you brought that up. The major problem with the Type-C power
> supplies is that the Type-C connector class does not actually take
> care of them. They are all registered by the device drivers, and all
> of them seem to expose different kind of information. In your case the
> power supplies are registered by the UCSI driver, and the above may
> indicate a bug in that driver.
Hi,
Thanks for the background.
My X13 Gen 3 (i5-1240P) uses the typec_ucsi and ucsi_acpi modules. Some time
back in a post you explained how to use debugfs with ucsi. Following that
procedure, just after a 20 Volt PD contract is negotiated on port 0 I see:
# cat /sys/kernel/debug/tracing/trace
....
kworker/0:1-18718 [000] ..... 137813.407189: ucsi_connector_change:
port0 status: change=0000, opmode=5, connected=1, sourcing=0,
partner_flags=1, partner_type=1,
request_data_obj=1304b12c, BC status=1
That RDO is incorrect, the top nibble (1) is the index of the default Vsafe5v
PDO. The correct PDO index would be 4 in this case. The source is an Apple 140W
USB-C power adapter so I doubt that it is breaking any PD 3.0/3.1 protocol
rules.
According the a PD analyzer (km002c) only one Request is sent by the sink:
82 10 d6 59 87 43 which it decodes as "Pos: 4 Fixed: 20V, 4.7A" which is
Accepted and 200 ms later a PS RDY is sent by the source and Vbus
transitions from from 5.17 Volts to 20.4 Volts. So I can see no Request for
PDO index 1 being sent.
With acpi_listen the following traffic occurs just after the power adapter
is plugged into port 0:
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
ibm/hotkey LEN0268:00 00000080 00006032
ac_adapter ACPI0003:00 00000080 00000001
ac_adapter ACPI0003:00 00000080 00000001
ibm/hotkey LEN0268:00 00000080 00006030
thermal_zone LNXTHERM:00 00000081 00000000
ibm/hotkey LEN0268:00 00000080 00006030
thermal_zone LNXTHERM:00 00000081 00000000
Hope this helps if you find time to look at this.
Doug Gilbert
> To improve the situation, I originally proposed that instead of
> adding a separate device class for USB Power Delivery objects, we
> would utilise the already existing power supply class. That proposal
> was not seen acceptable by many (including Benson if I recall), and I
> now tend to agree with that because of several reasons, starting from
> the fact that USB PD objects supply other informations on top of power
> delivery details (so completely unrelated to PM).
>
> Even before that I had proposed that the Type-C connector class could
> supply API for the drivers to take care of the registration of the
> power supplies. I proposed that not only the Type-C ports should
> register the power supplies but also the partners should represent
> their own power supplies. That would make things much more clear for
> the user space IMO. The port and partner would always create a "chain"
> of supplies where the other is the supply the other the sink of power.
> That is already supported by the power supply class. For some reason
> this proposal was also not seen as a good idea at the time, but it may
> be that I just failed to explain it properly.
>
> Nevertheless, I still think that that is exactly how the Type-C power
> supplies should be always presented - separate supplies for both ports
> and partners - and that obviously the Type-C connector class should
> take care of those supplies so that they always supply the same
> information. Unfortunately I do not have any time at the moment to
> work on this right now.
>
> Br,
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts
2023-08-24 16:51 ` Douglas Gilbert
@ 2023-08-28 9:21 ` Heikki Krogerus
2023-08-29 18:42 ` Douglas Gilbert
0 siblings, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2023-08-28 9:21 UTC (permalink / raw
To: Douglas Gilbert
Cc: Benson Leung, Jameson Thies, Prashant Malani, Won Chung,
linux-usb
On Thu, Aug 24, 2023 at 12:51:04PM -0400, Douglas Gilbert wrote:
> On 2023-08-23 04:56, Heikki Krogerus wrote:
> > Hi Douglas,
> >
> > On Tue, Aug 22, 2023 at 10:52:12AM -0400, Douglas Gilbert wrote:
> > > On 2023-08-22 09:32, Heikki Krogerus wrote:
> > > On a related matter, I wonder why there aren't symlinks between typec ports
> > > (under /sys/class/typec ) and/or the corresponding pd objects (under
> > > /sys/class/usb_power_delivery ) to the related power_supply objects under
> > > /sys/class/power_supply . For example under the latter directory I see:
> > > $ ls | more
> > > AC
> > > BAT0
> > > hidpp_battery_1
> > > ucsi-source-psy-USBC000:001
> > > ucsi-source-psy-USBC000:002
> > >
> > > Those last two power supplies are obviously connected to typec port0 and port1
> > > (but offset by 1). Those power_supply objects hold inaccurate data which I hope
> > > will improve in time. Significantly power_supply objects don't seem to report
> > > the direction of the power. Here is a little utility I have been working on
> > > to report the USB Type-C port/pd disposition on my machine:
> > > $ lsucpd
> > > port0 [pd0] > {5V, 0.9A}
> > > port1 [pd1] <<=== partner: [pd8]
> > >
> > > My laptop (Thinkpad X13 G3) has two type-C ports and port1 is a sink with a
> > > PD contract. I would like that second line to have 20V, 3.25A appended to it
> > > but there are several issues:
> > > - no typec or pd symlink to ucsi-source-psy-USBC000:002
> > > - that power supply_object says it is online (correct) with a voltage_now:
> > > 5000000 uV (incorrect) and current_now: 3000000 uA (incorrect). See below.
> > >
> > > ucsi-source-psy-USBC000:002 $ ls_name_value
> > > current_max : 3250000
> > > current_now : 3000000
> > > online : 1
> > > scope : Unknown
> > > type : USB
> > > uevent : <removed>
> > > usb_type : C [PD] PD_PPS
> > > voltage_max : 20000000
> > > voltage_min : 5000000
> > > voltage_now : 5000000
> >
> > I'm glad you brought that up. The major problem with the Type-C power
> > supplies is that the Type-C connector class does not actually take
> > care of them. They are all registered by the device drivers, and all
> > of them seem to expose different kind of information. In your case the
> > power supplies are registered by the UCSI driver, and the above may
> > indicate a bug in that driver.
>
> Hi,
> Thanks for the background.
>
> My X13 Gen 3 (i5-1240P) uses the typec_ucsi and ucsi_acpi modules. Some time
> back in a post you explained how to use debugfs with ucsi. Following that
> procedure, just after a 20 Volt PD contract is negotiated on port 0 I see:
>
> # cat /sys/kernel/debug/tracing/trace
> ....
> kworker/0:1-18718 [000] ..... 137813.407189: ucsi_connector_change:
> port0 status: change=0000, opmode=5, connected=1, sourcing=0,
> partner_flags=1, partner_type=1,
> request_data_obj=1304b12c, BC status=1
>
> That RDO is incorrect, the top nibble (1) is the index of the default Vsafe5v
> PDO. The correct PDO index would be 4 in this case. The source is an Apple 140W
> USB-C power adapter so I doubt that it is breaking any PD 3.0/3.1 protocol
> rules.
The driver reads the RDO from the UCSI interface, so if it's wrong,
there is possibly a problem in the Embedded Controller firmware :-(.
> According the a PD analyzer (km002c) only one Request is sent by the sink:
> 82 10 d6 59 87 43 which it decodes as "Pos: 4 Fixed: 20V, 4.7A" which is
> Accepted and 200 ms later a PS RDY is sent by the source and Vbus
> transitions from from 5.17 Volts to 20.4 Volts. So I can see no Request for
> PDO index 1 being sent.
>
> With acpi_listen the following traffic occurs just after the power adapter
> is plugged into port 0:
> battery PNP0C0A:00 00000080 00000001
> battery PNP0C0A:00 00000080 00000001
> ibm/hotkey LEN0268:00 00000080 00006032
> ac_adapter ACPI0003:00 00000080 00000001
> ac_adapter ACPI0003:00 00000080 00000001
> ibm/hotkey LEN0268:00 00000080 00006030
> thermal_zone LNXTHERM:00 00000081 00000000
> ibm/hotkey LEN0268:00 00000080 00006030
> thermal_zone LNXTHERM:00 00000081 00000000
>
> Hope this helps if you find time to look at this.
Thank you. I'll try to reproduce the issue this week, but I don't have
that exact model of Thinkpad available I'm afraid (UCSI tends to
behave a little bit differently on every single platform).
--
heikki
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts
2023-08-28 9:21 ` Heikki Krogerus
@ 2023-08-29 18:42 ` Douglas Gilbert
2023-08-31 12:25 ` Heikki Krogerus
0 siblings, 1 reply; 12+ messages in thread
From: Douglas Gilbert @ 2023-08-29 18:42 UTC (permalink / raw
To: Heikki Krogerus
Cc: Benson Leung, Jameson Thies, Prashant Malani, Won Chung,
linux-usb
On 2023-08-28 05:21, Heikki Krogerus wrote:
> On Thu, Aug 24, 2023 at 12:51:04PM -0400, Douglas Gilbert wrote:
>> On 2023-08-23 04:56, Heikki Krogerus wrote:
>>> Hi Douglas,
>>>
>>> On Tue, Aug 22, 2023 at 10:52:12AM -0400, Douglas Gilbert wrote:
>>>> On 2023-08-22 09:32, Heikki Krogerus wrote:
>>>> On a related matter, I wonder why there aren't symlinks between typec ports
>>>> (under /sys/class/typec ) and/or the corresponding pd objects (under
>>>> /sys/class/usb_power_delivery ) to the related power_supply objects under
>>>> /sys/class/power_supply . For example under the latter directory I see:
>>>> $ ls | more
>>>> AC
>>>> BAT0
>>>> hidpp_battery_1
>>>> ucsi-source-psy-USBC000:001
>>>> ucsi-source-psy-USBC000:002
>>>>
>>>> Those last two power supplies are obviously connected to typec port0 and port1
>>>> (but offset by 1). Those power_supply objects hold inaccurate data which I hope
>>>> will improve in time. Significantly power_supply objects don't seem to report
>>>> the direction of the power. Here is a little utility I have been working on
>>>> to report the USB Type-C port/pd disposition on my machine:
>>>> $ lsucpd
>>>> port0 [pd0] > {5V, 0.9A}
>>>> port1 [pd1] <<=== partner: [pd8]
>>>>
>>>> My laptop (Thinkpad X13 G3) has two type-C ports and port1 is a sink with a
>>>> PD contract. I would like that second line to have 20V, 3.25A appended to it
>>>> but there are several issues:
>>>> - no typec or pd symlink to ucsi-source-psy-USBC000:002
>>>> - that power supply_object says it is online (correct) with a voltage_now:
>>>> 5000000 uV (incorrect) and current_now: 3000000 uA (incorrect). See below.
>>>>
>>>> ucsi-source-psy-USBC000:002 $ ls_name_value
>>>> current_max : 3250000
>>>> current_now : 3000000
>>>> online : 1
>>>> scope : Unknown
>>>> type : USB
>>>> uevent : <removed>
>>>> usb_type : C [PD] PD_PPS
>>>> voltage_max : 20000000
>>>> voltage_min : 5000000
>>>> voltage_now : 5000000
>>>
>>> I'm glad you brought that up. The major problem with the Type-C power
>>> supplies is that the Type-C connector class does not actually take
>>> care of them. They are all registered by the device drivers, and all
>>> of them seem to expose different kind of information. In your case the
>>> power supplies are registered by the UCSI driver, and the above may
>>> indicate a bug in that driver.
>>
>> Hi,
>> Thanks for the background.
>>
>> My X13 Gen 3 (i5-1240P) uses the typec_ucsi and ucsi_acpi modules. Some time
>> back in a post you explained how to use debugfs with ucsi. Following that
>> procedure, just after a 20 Volt PD contract is negotiated on port 0 I see:
>>
>> # cat /sys/kernel/debug/tracing/trace
>> ....
>> kworker/0:1-18718 [000] ..... 137813.407189: ucsi_connector_change:
>> port0 status: change=0000, opmode=5, connected=1, sourcing=0,
>> partner_flags=1, partner_type=1,
>> request_data_obj=1304b12c, BC status=1
>>
>> That RDO is incorrect, the top nibble (1) is the index of the default Vsafe5v
>> PDO. The correct PDO index would be 4 in this case. The source is an Apple 140W
>> USB-C power adapter so I doubt that it is breaking any PD 3.0/3.1 protocol
>> rules.
>
> The driver reads the RDO from the UCSI interface, so if it's wrong,
> there is possibly a problem in the Embedded Controller firmware :-(.
>
>> According the a PD analyzer (km002c) only one Request is sent by the sink:
>> 82 10 d6 59 87 43 which it decodes as "Pos: 4 Fixed: 20V, 4.7A" which is
>> Accepted and 200 ms later a PS RDY is sent by the source and Vbus
>> transitions from from 5.17 Volts to 20.4 Volts. So I can see no Request for
>> PDO index 1 being sent.
>>
>> With acpi_listen the following traffic occurs just after the power adapter
>> is plugged into port 0:
>> battery PNP0C0A:00 00000080 00000001
>> battery PNP0C0A:00 00000080 00000001
>> ibm/hotkey LEN0268:00 00000080 00006032
>> ac_adapter ACPI0003:00 00000080 00000001
>> ac_adapter ACPI0003:00 00000080 00000001
>> ibm/hotkey LEN0268:00 00000080 00006030
>> thermal_zone LNXTHERM:00 00000081 00000000
>> ibm/hotkey LEN0268:00 00000080 00006030
>> thermal_zone LNXTHERM:00 00000081 00000000
>>
>> Hope this helps if you find time to look at this.
>
> Thank you. I'll try to reproduce the issue this week, but I don't have
> that exact model of Thinkpad available I'm afraid (UCSI tends to
> behave a little bit differently on every single platform).
Could it be a CPU generation thing? My CPU is 12th generation (2022) and
there is another report of a Lenovo P15gen2 (11th generation 2021 I assume)
not reporting PDOs at all. I have an older Dell XPS 9380 which has an 8th
generation CPU (3 USB-C port and no Type A ports) that has no UCSI support.
So do PDOs and the active RDO get properly reported on 13th generation
CPUs?
Doug Gilbert
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts
2023-08-29 18:42 ` Douglas Gilbert
@ 2023-08-31 12:25 ` Heikki Krogerus
0 siblings, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2023-08-31 12:25 UTC (permalink / raw
To: Douglas Gilbert
Cc: Benson Leung, Jameson Thies, Prashant Malani, Won Chung,
linux-usb
On Tue, Aug 29, 2023 at 02:42:29PM -0400, Douglas Gilbert wrote:
> On 2023-08-28 05:21, Heikki Krogerus wrote:
> > On Thu, Aug 24, 2023 at 12:51:04PM -0400, Douglas Gilbert wrote:
> > > On 2023-08-23 04:56, Heikki Krogerus wrote:
> > > > Hi Douglas,
> > > >
> > > > On Tue, Aug 22, 2023 at 10:52:12AM -0400, Douglas Gilbert wrote:
> > > > > On 2023-08-22 09:32, Heikki Krogerus wrote:
> > > > > On a related matter, I wonder why there aren't symlinks between typec ports
> > > > > (under /sys/class/typec ) and/or the corresponding pd objects (under
> > > > > /sys/class/usb_power_delivery ) to the related power_supply objects under
> > > > > /sys/class/power_supply . For example under the latter directory I see:
> > > > > $ ls | more
> > > > > AC
> > > > > BAT0
> > > > > hidpp_battery_1
> > > > > ucsi-source-psy-USBC000:001
> > > > > ucsi-source-psy-USBC000:002
> > > > >
> > > > > Those last two power supplies are obviously connected to typec port0 and port1
> > > > > (but offset by 1). Those power_supply objects hold inaccurate data which I hope
> > > > > will improve in time. Significantly power_supply objects don't seem to report
> > > > > the direction of the power. Here is a little utility I have been working on
> > > > > to report the USB Type-C port/pd disposition on my machine:
> > > > > $ lsucpd
> > > > > port0 [pd0] > {5V, 0.9A}
> > > > > port1 [pd1] <<=== partner: [pd8]
> > > > >
> > > > > My laptop (Thinkpad X13 G3) has two type-C ports and port1 is a sink with a
> > > > > PD contract. I would like that second line to have 20V, 3.25A appended to it
> > > > > but there are several issues:
> > > > > - no typec or pd symlink to ucsi-source-psy-USBC000:002
> > > > > - that power supply_object says it is online (correct) with a voltage_now:
> > > > > 5000000 uV (incorrect) and current_now: 3000000 uA (incorrect). See below.
> > > > >
> > > > > ucsi-source-psy-USBC000:002 $ ls_name_value
> > > > > current_max : 3250000
> > > > > current_now : 3000000
> > > > > online : 1
> > > > > scope : Unknown
> > > > > type : USB
> > > > > uevent : <removed>
> > > > > usb_type : C [PD] PD_PPS
> > > > > voltage_max : 20000000
> > > > > voltage_min : 5000000
> > > > > voltage_now : 5000000
> > > >
> > > > I'm glad you brought that up. The major problem with the Type-C power
> > > > supplies is that the Type-C connector class does not actually take
> > > > care of them. They are all registered by the device drivers, and all
> > > > of them seem to expose different kind of information. In your case the
> > > > power supplies are registered by the UCSI driver, and the above may
> > > > indicate a bug in that driver.
> > >
> > > Hi,
> > > Thanks for the background.
> > >
> > > My X13 Gen 3 (i5-1240P) uses the typec_ucsi and ucsi_acpi modules. Some time
> > > back in a post you explained how to use debugfs with ucsi. Following that
> > > procedure, just after a 20 Volt PD contract is negotiated on port 0 I see:
> > >
> > > # cat /sys/kernel/debug/tracing/trace
> > > ....
> > > kworker/0:1-18718 [000] ..... 137813.407189: ucsi_connector_change:
> > > port0 status: change=0000, opmode=5, connected=1, sourcing=0,
> > > partner_flags=1, partner_type=1,
> > > request_data_obj=1304b12c, BC status=1
> > >
> > > That RDO is incorrect, the top nibble (1) is the index of the default Vsafe5v
> > > PDO. The correct PDO index would be 4 in this case. The source is an Apple 140W
> > > USB-C power adapter so I doubt that it is breaking any PD 3.0/3.1 protocol
> > > rules.
> >
> > The driver reads the RDO from the UCSI interface, so if it's wrong,
> > there is possibly a problem in the Embedded Controller firmware :-(.
> >
> > > According the a PD analyzer (km002c) only one Request is sent by the sink:
> > > 82 10 d6 59 87 43 which it decodes as "Pos: 4 Fixed: 20V, 4.7A" which is
> > > Accepted and 200 ms later a PS RDY is sent by the source and Vbus
> > > transitions from from 5.17 Volts to 20.4 Volts. So I can see no Request for
> > > PDO index 1 being sent.
> > >
> > > With acpi_listen the following traffic occurs just after the power adapter
> > > is plugged into port 0:
> > > battery PNP0C0A:00 00000080 00000001
> > > battery PNP0C0A:00 00000080 00000001
> > > ibm/hotkey LEN0268:00 00000080 00006032
> > > ac_adapter ACPI0003:00 00000080 00000001
> > > ac_adapter ACPI0003:00 00000080 00000001
> > > ibm/hotkey LEN0268:00 00000080 00006030
> > > thermal_zone LNXTHERM:00 00000081 00000000
> > > ibm/hotkey LEN0268:00 00000080 00006030
> > > thermal_zone LNXTHERM:00 00000081 00000000
> > >
> > > Hope this helps if you find time to look at this.
> >
> > Thank you. I'll try to reproduce the issue this week, but I don't have
> > that exact model of Thinkpad available I'm afraid (UCSI tends to
> > behave a little bit differently on every single platform).
>
> Could it be a CPU generation thing? My CPU is 12th generation (2022) and
> there is another report of a Lenovo P15gen2 (11th generation 2021 I assume)
> not reporting PDOs at all. I have an older Dell XPS 9380 which has an 8th
> generation CPU (3 USB-C port and no Type A ports) that has no UCSI support.
> So do PDOs and the active RDO get properly reported on 13th generation
> CPUs?
Probable not. It really depends on the embedded controller or PD
controller firmware, so ultimately the platform and product.
It could be that the reference embedded controller firmware from Intel
is only patched ones for every CPU generation (I don't know), but for
example Dell does not use Intel's reference embedded controller
firmware, or they at least patch it heavily for every product, so the
CPU gen should not play a huge role there.
Now some (most?) USB PD controllers also support UCSI natively, so the
EC firmware may be just a proxy between the OS and the PD controller.
The PD controller can be anything regardless of the CPU generation,
and theare are several vendors, and on top of that, the PD controller
firmwares may be customised for every single product line.
thanks,
--
heikki
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-08-31 12:25 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-22 13:32 [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts Heikki Krogerus
2023-08-22 13:32 ` [RFC PATCH 1/2] usb: typec: Link enumerated USB devices with Type-C partner Heikki Krogerus
2023-08-22 13:32 ` [RFC PATCH 2/2] usb: Inform the USB Type-C class about enumerated devices Heikki Krogerus
2023-08-22 13:58 ` [RFC PATCH 0/2] usb: Link USB devices with their USB Type-C partner counterparts Benson Leung
2023-08-22 14:52 ` Douglas Gilbert
2023-08-23 8:56 ` Heikki Krogerus
2023-08-24 16:51 ` Douglas Gilbert
2023-08-28 9:21 ` Heikki Krogerus
2023-08-29 18:42 ` Douglas Gilbert
2023-08-31 12:25 ` Heikki Krogerus
2023-08-23 0:24 ` Benson Leung
2023-08-23 10:01 ` Heikki Krogerus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).