mbox series

[RFC,0/2] usb: Link USB devices with their USB Type-C partner counterparts

Message ID 20230822133205.2063210-1-heikki.krogerus@linux.intel.com (mailing list archive)
Headers show
Series usb: Link USB devices with their USB Type-C partner counterparts | expand

Message

Heikki Krogerus Aug. 22, 2023, 1:32 p.m. UTC
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(-)

Comments

Benson Leung Aug. 22, 2023, 1:58 p.m. UTC | #1
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
Douglas Gilbert Aug. 22, 2023, 2:52 p.m. UTC | #2
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
Benson Leung Aug. 23, 2023, 12:24 a.m. UTC | #3
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
Heikki Krogerus Aug. 23, 2023, 8:56 a.m. UTC | #4
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 Krogerus Aug. 23, 2023, 10:01 a.m. UTC | #5
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!
Douglas Gilbert Aug. 24, 2023, 4:51 p.m. UTC | #6
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,
>
Heikki Krogerus Aug. 28, 2023, 9:21 a.m. UTC | #7
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).
Douglas Gilbert Aug. 29, 2023, 6:42 p.m. UTC | #8
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
Heikki Krogerus Aug. 31, 2023, 12:25 p.m. UTC | #9
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,