Add support for reporting Source and Sink Capabilities
Power Data Object (PDO) property. These are reported by USB
Power Delivery (PD) capable power sources.
Signed-off-by: Prashant Malani <[email protected]>
---
Documentation/ABI/testing/sysfs-class-power | 30 +++++++++++++++++++++
drivers/power/supply/power_supply_sysfs.c | 18 ++++++++++++-
include/linux/power_supply.h | 5 ++++
3 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index ca830c6cd809..90d4198e6dfb 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -562,6 +562,36 @@ Description:
"Unknown", "SDP", "DCP", "CDP", "ACA", "C", "PD",
"PD_DRP", "PD_PPS", "BrickID"
+What: /sys/class/power_supply/<supply_name>/source_cap_pdos
+Date: September 2021
+Contact: [email protected]
+Description:
+ Reports the Source Capabilities Power Data Objects (PDO) reported by the USB
+ PD-capable power source. 13 PDOs are listed. PDOs 1-7 represent the Source Caps
+ for devices which only support Standard Power Range (SPR), whereas PDOs 8-13
+ are for Extended Power Range (EPR) capable sources.
+ NOTE: The EPR Source Caps message is a superset of the Source Cap message, so on
+ SPR-only sources, PDOs 8-13 will be 0.
+
+ Access: Read-Only
+
+ Valid values: Represented as a list of 13 32-bit PDO objects in hexadecimal format.
+
+What: /sys/class/power_supply/<supply_name>/sink_cap_pdos
+Date: September 2021
+Contact: [email protected]
+Description:
+ Reports the Sink Capabilities Power Data Objects (PDO) reported by the USB
+ PD-capable power source. 13 PDOs are listed. PDOs 1-7 represent the Sink Caps
+ for devices which only support Standard Power Range (SPR), whereas PDOs 8-13
+ are for Extended Power Range (EPR) capable sinks.
+ NOTE: The EPR Sink Caps message is a superset of the Sink Cap message, so on
+ SPR-only sinks, PDOs 8-13 will be 0.
+
+ Access: Read-Only
+
+ Valid values: Represented as a list of 13 32-bit PDO objects in hexadecimal format.
+
**Device Specific Properties**
What: /sys/class/power/ds2760-battery.*/charge_now
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index c3d7cbcd4fad..9d17d3376949 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -211,6 +211,9 @@ static struct power_supply_attr power_supply_attrs[] = {
POWER_SUPPLY_ATTR(MODEL_NAME),
POWER_SUPPLY_ATTR(MANUFACTURER),
POWER_SUPPLY_ATTR(SERIAL_NUMBER),
+ /* Array properties */
+ POWER_SUPPLY_ATTR(SINK_CAP_PDOS),
+ POWER_SUPPLY_ATTR(SOURCE_CAP_PDOS),
};
static struct attribute *
@@ -267,7 +270,11 @@ static ssize_t power_supply_show_property(struct device *dev,
struct power_supply *psy = dev_get_drvdata(dev);
struct power_supply_attr *ps_attr = to_ps_attr(attr);
enum power_supply_property psp = dev_attr_psp(attr);
- union power_supply_propval value;
+ union power_supply_propval value = {
+ .pdos = {0}
+ };
+ size_t count;
+ int i;
if (psp == POWER_SUPPLY_PROP_TYPE) {
value.intval = psy->desc->type;
@@ -299,6 +306,15 @@ static ssize_t power_supply_show_property(struct device *dev,
case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
ret = sprintf(buf, "%s\n", value.strval);
break;
+ case POWER_SUPPLY_PROP_SINK_CAP_PDOS:
+ case POWER_SUPPLY_PROP_SOURCE_CAP_PDOS:
+ ret = 0;
+ for (i = 0; i < PDO_MAX_OBJECTS; i++) {
+ count = sprintf(buf, "0x%08x\n", value.pdos[i]);
+ buf += count;
+ ret += count;
+ }
+ break;
default:
ret = sprintf(buf, "%d\n", value.intval);
}
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 9ca1f120a211..a53c8fa4c1c9 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -17,6 +17,7 @@
#include <linux/leds.h>
#include <linux/spinlock.h>
#include <linux/notifier.h>
+#include <linux/usb/pd.h>
/*
* All voltages, currents, charges, energies, time and temperatures in uV,
@@ -171,6 +172,9 @@ enum power_supply_property {
POWER_SUPPLY_PROP_MODEL_NAME,
POWER_SUPPLY_PROP_MANUFACTURER,
POWER_SUPPLY_PROP_SERIAL_NUMBER,
+ /* Array properties */
+ POWER_SUPPLY_PROP_SINK_CAP_PDOS,
+ POWER_SUPPLY_PROP_SOURCE_CAP_PDOS,
};
enum power_supply_type {
@@ -209,6 +213,7 @@ enum power_supply_notifier_events {
union power_supply_propval {
int intval;
const char *strval;
+ u32 pdos[PDO_MAX_OBJECTS];
};
struct device_node;
--
2.33.0.153.gba50c8fa24-goog
Hi Prashant,
On Thu, Sep 02, 2021 at 02:35:00PM -0700, Prashant Malani wrote:
> Add support for reporting Source and Sink Capabilities
> Power Data Object (PDO) property. These are reported by USB
> Power Delivery (PD) capable power sources.
>
> Signed-off-by: Prashant Malani <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-class-power | 30 +++++++++++++++++++++
> drivers/power/supply/power_supply_sysfs.c | 18 ++++++++++++-
> include/linux/power_supply.h | 5 ++++
> 3 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> index ca830c6cd809..90d4198e6dfb 100644
> --- a/Documentation/ABI/testing/sysfs-class-power
> +++ b/Documentation/ABI/testing/sysfs-class-power
> @@ -562,6 +562,36 @@ Description:
> "Unknown", "SDP", "DCP", "CDP", "ACA", "C", "PD",
> "PD_DRP", "PD_PPS", "BrickID"
>
> +What: /sys/class/power_supply/<supply_name>/source_cap_pdos
> +Date: September 2021
> +Contact: [email protected]
> +Description:
> + Reports the Source Capabilities Power Data Objects (PDO) reported by the USB
> + PD-capable power source. 13 PDOs are listed. PDOs 1-7 represent the Source Caps
> + for devices which only support Standard Power Range (SPR), whereas PDOs 8-13
> + are for Extended Power Range (EPR) capable sources.
> + NOTE: The EPR Source Caps message is a superset of the Source Cap message, so on
> + SPR-only sources, PDOs 8-13 will be 0.
> +
> + Access: Read-Only
> +
> + Valid values: Represented as a list of 13 32-bit PDO objects in hexadecimal format.
> +
> +What: /sys/class/power_supply/<supply_name>/sink_cap_pdos
> +Date: September 2021
> +Contact: [email protected]
> +Description:
> + Reports the Sink Capabilities Power Data Objects (PDO) reported by the USB
> + PD-capable power source. 13 PDOs are listed. PDOs 1-7 represent the Sink Caps
> + for devices which only support Standard Power Range (SPR), whereas PDOs 8-13
> + are for Extended Power Range (EPR) capable sinks.
> + NOTE: The EPR Sink Caps message is a superset of the Sink Cap message, so on
> + SPR-only sinks, PDOs 8-13 will be 0.
> +
> + Access: Read-Only
> +
> + Valid values: Represented as a list of 13 32-bit PDO objects in hexadecimal format.
My plan is to register a separate power supply for each PDO. So for
every source PDO and every sink PDO a port has in its capabilities,
you'll have a separate power supply registered, and the same for the
partner when it's connected. With every connection there should always
be one active (online) source psy and active sink psy (if the port is
source, one of the port's source psys will be active and the partner
will have the active sink psy, or vise versa - depending on the role).
Each PDO represents a "Power Supply" so to me that approach just
makes the most sense. It will require a bit of work in kernel, however
in user space it should mean that we only have a single new attribute
file for the power supplies named "pdo" that returns a single PDO.
Let me know if you guys see any obvious problems with the idea.
Otherwise, that is how we really need to do this. That will make
things much more clear in user space. I have a feeling it will make
things easier in kernel as well in the long run.
Adding Adam and Guenter. It would be good if you guys could comment
the idea as well.
thanks,
--
heikki
On 13 September 2021 14:30, Heikki Krogerus wrote:
> > Add support for reporting Source and Sink Capabilities
> > Power Data Object (PDO) property. These are reported by USB
> > Power Delivery (PD) capable power sources.
> >
> > Signed-off-by: Prashant Malani <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-class-power | 30 +++++++++++++++++++++
> > drivers/power/supply/power_supply_sysfs.c | 18 ++++++++++++-
> > include/linux/power_supply.h | 5 ++++
> > 3 files changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-power
> b/Documentation/ABI/testing/sysfs-class-power
> > index ca830c6cd809..90d4198e6dfb 100644
> > --- a/Documentation/ABI/testing/sysfs-class-power
> > +++ b/Documentation/ABI/testing/sysfs-class-power
> > @@ -562,6 +562,36 @@ Description:
> > "Unknown", "SDP", "DCP", "CDP", "ACA", "C", "PD",
> > "PD_DRP", "PD_PPS", "BrickID"
> >
> > +What:
> /sys/class/power_supply/<supply_name>/source_cap_pdos
> > +Date: September 2021
> > +Contact: [email protected]
> > +Description:
> > + Reports the Source Capabilities Power Data Objects (PDO)
> reported by the USB
> > + PD-capable power source. 13 PDOs are listed. PDOs 1-7 represent
> the Source Caps
> > + for devices which only support Standard Power Range (SPR),
> whereas PDOs 8-13
> > + are for Extended Power Range (EPR) capable sources.
> > + NOTE: The EPR Source Caps message is a superset of the Source
> Cap message, so on
> > + SPR-only sources, PDOs 8-13 will be 0.
> > +
> > + Access: Read-Only
> > +
> > + Valid values: Represented as a list of 13 32-bit PDO objects in
> hexadecimal format.
> > +
> > +What:
> /sys/class/power_supply/<supply_name>/sink_cap_pdos
> > +Date: September 2021
> > +Contact: [email protected]
> > +Description:
> > + Reports the Sink Capabilities Power Data Objects (PDO) reported
> by the USB
> > + PD-capable power source. 13 PDOs are listed. PDOs 1-7 represent
> the Sink Caps
> > + for devices which only support Standard Power Range (SPR),
> whereas PDOs 8-13
> > + are for Extended Power Range (EPR) capable sinks.
> > + NOTE: The EPR Sink Caps message is a superset of the Sink Cap
> message, so on
> > + SPR-only sinks, PDOs 8-13 will be 0.
> > +
> > + Access: Read-Only
> > +
> > + Valid values: Represented as a list of 13 32-bit PDO objects in
> hexadecimal format.
>
> My plan is to register a separate power supply for each PDO. So for
> every source PDO and every sink PDO a port has in its capabilities,
> you'll have a separate power supply registered, and the same for the
> partner when it's connected. With every connection there should always
> be one active (online) source psy and active sink psy (if the port is
> source, one of the port's source psys will be active and the partner
> will have the active sink psy, or vise versa - depending on the role).
>
> Each PDO represents a "Power Supply" so to me that approach just
> makes the most sense. It will require a bit of work in kernel, however
> in user space it should mean that we only have a single new attribute
> file for the power supplies named "pdo" that returns a single PDO.
>
> Let me know if you guys see any obvious problems with the idea.
> Otherwise, that is how we really need to do this. That will make
> things much more clear in user space. I have a feeling it will make
> things easier in kernel as well in the long run.
>
> Adding Adam and Guenter. It would be good if you guys could comment
> the idea as well.
Hi Heikki,
Thanks for CCing me. My two pence worth is that I always envisaged the PSY
representation as being 1 PSY for 1 power source. I consider this in a
similar manner to the Regulator framework, where 1 regulator can support a range
of voltages and currents, but this is covered by 1 regulator instance as it's
just a single output. For USB-PD we have a number of options for voltage/current
combos, including PPS which is even lower granularity, but it's still only one
port. I get the feeling that having PSY instances for each and every PDO might
be a little confusing and these will never be concurrent.
However, I'd be keen to understand further and see what restrictions/issues are
currently present as I probably don't have a complete view of this right now. I
wouldn't want to dismiss something out of turn, especially when you obviously
have good reason to suggest such an approach.
Hi Heikki,
On Mon, Sep 13, 2021 at 04:30:08PM +0300, Heikki Krogerus wrote:
> My plan is to register a separate power supply for each PDO. So for
> every source PDO and every sink PDO a port has in its capabilities,
> you'll have a separate power supply registered, and the same for the
> partner when it's connected. With every connection there should always
> be one active (online) source psy and active sink psy (if the port is
> source, one of the port's source psys will be active and the partner
> will have the active sink psy, or vise versa - depending on the role).
>
> Each PDO represents a "Power Supply" so to me that approach just
> makes the most sense. It will require a bit of work in kernel, however
> in user space it should mean that we only have a single new attribute
> file for the power supplies named "pdo" that returns a single PDO.
There's a few downsides to this approach (one psy for each pdo).
The PDOs returned by Source Capabilities and Sink Capabilities do not just
contain possible Voltage, Current, and Power combinations, but they also contain
additional information in the form of properties.
In the Fixed Supply PDO, the following bits convey information about the supply
or sink (See USB PD Spec R3.1 V1.0 Table 6-9):
* B29 - Dual-Role Power
* B28 - USB Suspend Supported
* B27 - Unconstrained Power
* B26 - USB Communications Capable
* B25 - Dual-Role Data
* B24 - Unchunked Extended Messages Supported
* B23 - EPR Mode Capable
These bits exist in every single 32-bit Fixed Supply PDO, however,
Section 6.4.1.2.2 requires that they be appropriately set in the vSafe5V Fixed
PDO (which is required for all sources and sinks), and set to 0 in all other
PDOs in the caps.
> Since all USB Providers support vSafe5V, the required vSafe5V Fixed Supply
> Power Data Object is also used to convey additional information that is
> returned in bits 29 through 25. All other Fixed Supply Power Data Objects
> Shall set bits 29…22 to zero.
So, splitting out a particular port partner or port's PDOs into individual
power supplies runs the risk of the information above not properly being
attributed to the actual power supply.
For example, if you're connected to a 18W power supply that has a vSafe5V PDO,
and a 9V Fixed PDO, and you're operating at 18W, the 9V Fixed PDO will be Active
but the inactive vSafe5V PDO has information in those higher order bits that
remain relevant.
Just something to keep in mind.
>
> Let me know if you guys see any obvious problems with the idea.
> Otherwise, that is how we really need to do this. That will make
> things much more clear in user space. I have a feeling it will make
> things easier in kernel as well in the long run.
>
> Adding Adam and Guenter. It would be good if you guys could comment
> the idea as well.
I'm supportive of using a separate PSY to represent the different power roles
of a particular port and port-partner, however. If you're connected to a USB-C
power bank, you should see two objects for that partner, one for when the
power bank is in source role, and one when the power bank is in sink role.
Even when you're in one role or the other, you should still be able to read
information from the other role in order to make an informed decision whether
you want to power role swap.
Thanks so much!
Benson
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]
Hi Adam and Heikki,
On Mon, Sep 13, 2021 at 03:15:46PM +0000, Adam Thomson wrote:
> On 13 September 2021 14:30, Heikki Krogerus wrote:
> >
> > My plan is to register a separate power supply for each PDO. So for
> > every source PDO and every sink PDO a port has in its capabilities,
> > you'll have a separate power supply registered, and the same for the
> > partner when it's connected. With every connection there should always
> > be one active (online) source psy and active sink psy (if the port is
> > source, one of the port's source psys will be active and the partner
> > will have the active sink psy, or vise versa - depending on the role).
> >
> > Each PDO represents a "Power Supply" so to me that approach just
> > makes the most sense. It will require a bit of work in kernel, however
> > in user space it should mean that we only have a single new attribute
> > file for the power supplies named "pdo" that returns a single PDO.
> >
> > Let me know if you guys see any obvious problems with the idea.
> > Otherwise, that is how we really need to do this. That will make
> > things much more clear in user space. I have a feeling it will make
> > things easier in kernel as well in the long run.
> >
> > Adding Adam and Guenter. It would be good if you guys could comment
> > the idea as well.
>
> Hi Heikki,
>
> Thanks for CCing me. My two pence worth is that I always envisaged the PSY
> representation as being 1 PSY for 1 power source. I consider this in a
> similar manner to the Regulator framework, where 1 regulator can support a range
> of voltages and currents, but this is covered by 1 regulator instance as it's
> just a single output. For USB-PD we have a number of options for voltage/current
> combos, including PPS which is even lower granularity, but it's still only one
> port. I get the feeling that having PSY instances for each and every PDO might
> be a little confusing and these will never be concurrent.
>
> However, I'd be keen to understand further and see what restrictions/issues are
> currently present as I probably don't have a complete view of this right now. I
> wouldn't want to dismiss something out of turn, especially when you obviously
> have good reason to suggest such an approach.
I thought of one more potential downside to one-psy-per-pdo:
Remember that a source or sink's Capabilities may dynamically change without
a port disconnect, and this could make one-psy-per-pdo much more chatty with
power supply deletions and re-registrations on load balancing events.
At basically any time, a power source may send a new SRC_CAP to the sink which
adjusts, deletes, or adds to the list of PDOs, without the connection state
machine registering a disconnect.
In a real world case, I have a charger in my kitchen that has 2 USB-C ports
and supports a total of 30W output. When one device is plugged in:
5V 3A, 9V 3A, 15V 2A
However, when two devices are plugged in, each sees:
5V 3A
The load balancing event would result in two power supply deletions, whereas
if it were a single psy per power supply (incorporating the list of PDO choices)
it would just be a single PROP_CHANGED event.
It seems cleaner to me to have deletions and additions only possible when the
thing is unplugged or plugged.
Thanks,
Benson
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]
Mon, Sep 13, 2021 at 03:15:46PM +0000, Adam Thomson kirjoitti:
> On 13 September 2021 14:30, Heikki Krogerus wrote:
>
> > > Add support for reporting Source and Sink Capabilities
> > > Power Data Object (PDO) property. These are reported by USB
> > > Power Delivery (PD) capable power sources.
> > >
> > > Signed-off-by: Prashant Malani <[email protected]>
> > > ---
> > > Documentation/ABI/testing/sysfs-class-power | 30 +++++++++++++++++++++
> > > drivers/power/supply/power_supply_sysfs.c | 18 ++++++++++++-
> > > include/linux/power_supply.h | 5 ++++
> > > 3 files changed, 52 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-class-power
> > b/Documentation/ABI/testing/sysfs-class-power
> > > index ca830c6cd809..90d4198e6dfb 100644
> > > --- a/Documentation/ABI/testing/sysfs-class-power
> > > +++ b/Documentation/ABI/testing/sysfs-class-power
> > > @@ -562,6 +562,36 @@ Description:
> > > "Unknown", "SDP", "DCP", "CDP", "ACA", "C", "PD",
> > > "PD_DRP", "PD_PPS", "BrickID"
> > >
> > > +What:
> > /sys/class/power_supply/<supply_name>/source_cap_pdos
> > > +Date: September 2021
> > > +Contact: [email protected]
> > > +Description:
> > > + Reports the Source Capabilities Power Data Objects (PDO)
> > reported by the USB
> > > + PD-capable power source. 13 PDOs are listed. PDOs 1-7 represent
> > the Source Caps
> > > + for devices which only support Standard Power Range (SPR),
> > whereas PDOs 8-13
> > > + are for Extended Power Range (EPR) capable sources.
> > > + NOTE: The EPR Source Caps message is a superset of the Source
> > Cap message, so on
> > > + SPR-only sources, PDOs 8-13 will be 0.
> > > +
> > > + Access: Read-Only
> > > +
> > > + Valid values: Represented as a list of 13 32-bit PDO objects in
> > hexadecimal format.
> > > +
> > > +What:
> > /sys/class/power_supply/<supply_name>/sink_cap_pdos
> > > +Date: September 2021
> > > +Contact: [email protected]
> > > +Description:
> > > + Reports the Sink Capabilities Power Data Objects (PDO) reported
> > by the USB
> > > + PD-capable power source. 13 PDOs are listed. PDOs 1-7 represent
> > the Sink Caps
> > > + for devices which only support Standard Power Range (SPR),
> > whereas PDOs 8-13
> > > + are for Extended Power Range (EPR) capable sinks.
> > > + NOTE: The EPR Sink Caps message is a superset of the Sink Cap
> > message, so on
> > > + SPR-only sinks, PDOs 8-13 will be 0.
> > > +
> > > + Access: Read-Only
> > > +
> > > + Valid values: Represented as a list of 13 32-bit PDO objects in
> > hexadecimal format.
> >
> > My plan is to register a separate power supply for each PDO. So for
> > every source PDO and every sink PDO a port has in its capabilities,
> > you'll have a separate power supply registered, and the same for the
> > partner when it's connected. With every connection there should always
> > be one active (online) source psy and active sink psy (if the port is
> > source, one of the port's source psys will be active and the partner
> > will have the active sink psy, or vise versa - depending on the role).
> >
> > Each PDO represents a "Power Supply" so to me that approach just
> > makes the most sense. It will require a bit of work in kernel, however
> > in user space it should mean that we only have a single new attribute
> > file for the power supplies named "pdo" that returns a single PDO.
> >
> > Let me know if you guys see any obvious problems with the idea.
> > Otherwise, that is how we really need to do this. That will make
> > things much more clear in user space. I have a feeling it will make
> > things easier in kernel as well in the long run.
> >
> > Adding Adam and Guenter. It would be good if you guys could comment
> > the idea as well.
>
> Hi Heikki,
>
> Thanks for CCing me. My two pence worth is that I always envisaged the PSY
> representation as being 1 PSY for 1 power source. I consider this in a
> similar manner to the Regulator framework, where 1 regulator can support a range
> of voltages and currents, but this is covered by 1 regulator instance as it's
> just a single output. For USB-PD we have a number of options for voltage/current
> combos, including PPS which is even lower granularity, but it's still only one
> port. I get the feeling that having PSY instances for each and every PDO might
> be a little confusing and these will never be concurrent.
>
> However, I'd be keen to understand further and see what restrictions/issues are
> currently present as I probably don't have a complete view of this right now. I
> wouldn't want to dismiss something out of turn, especially when you obviously
> have good reason to suggest such an approach.
I'm not proposing that we drop the port-psys. I'm sorry if I've been
unclear about that. The port-psy we can not drop because of several
reasons. For starters, we still can not assume that USB PD is always
supported.
What I'm trying to propose is that we take advantage of the
power-supply framework by building a "dynamic" hierarchy of power
supplies that supply each other in order to represent the actual
situation as closely as possible. For example, a port-psy that is
supplied by port-Fixed-sink-psy that is supplied by
port-partner-Fixed-source (that is supplied by port-partner-psy).
Something like that. The only "static" part in the hierarchy is the
port-psy, as everything else about it can change, even without
disconnection.
So the port-psy always either supplies another psy or is supplied by
another psy in this hierarchy, depending on the role of the port. But
most importantly, the properties of the port-psy itself are _newer_
adjustable - they are read-only. The psy that supplies the port-psy
can be adjustable, if it's for example PPS, but not the port-psy
itself.
The problem with having only a single psy per port (and possibly
partners) is that it does not work well enough when the capabilities
change, and the capabilities can really change at any moment, we don't
need to disconnect for that to happen - simply by plugging another
device to another port can change the power budget for your port and
change your capabilities. The biggest problem is when we loose the
ability to adjust the values if we for example loose the PPS that we
were using in the middle of operation. The single psy has to attempt
to handle the situation by adjusting something like the ranges of the
properties, because it can't change the actual property set itself.
That is hacky, and to be honest, a little bit risky, because it leaves
us at the mercy of programmers completely unnecessarily.
With my proposal, if the capabilities change, it only means we rebuild
the psy hierarchy, and that's it. Nothing else needs to be done in
kernel, and all changes are super visible and clear in user space.
thanks,
--
heikki
Mon, Sep 13, 2021 at 12:30:58PM -0700, Benson Leung kirjoitti:
> Hi Adam and Heikki,
>
> On Mon, Sep 13, 2021 at 03:15:46PM +0000, Adam Thomson wrote:
> > On 13 September 2021 14:30, Heikki Krogerus wrote:
> > >
> > > My plan is to register a separate power supply for each PDO. So for
> > > every source PDO and every sink PDO a port has in its capabilities,
> > > you'll have a separate power supply registered, and the same for the
> > > partner when it's connected. With every connection there should always
> > > be one active (online) source psy and active sink psy (if the port is
> > > source, one of the port's source psys will be active and the partner
> > > will have the active sink psy, or vise versa - depending on the role).
> > >
> > > Each PDO represents a "Power Supply" so to me that approach just
> > > makes the most sense. It will require a bit of work in kernel, however
> > > in user space it should mean that we only have a single new attribute
> > > file for the power supplies named "pdo" that returns a single PDO.
> > >
> > > Let me know if you guys see any obvious problems with the idea.
> > > Otherwise, that is how we really need to do this. That will make
> > > things much more clear in user space. I have a feeling it will make
> > > things easier in kernel as well in the long run.
> > >
> > > Adding Adam and Guenter. It would be good if you guys could comment
> > > the idea as well.
> >
> > Hi Heikki,
> >
> > Thanks for CCing me. My two pence worth is that I always envisaged the PSY
> > representation as being 1 PSY for 1 power source. I consider this in a
> > similar manner to the Regulator framework, where 1 regulator can support a range
> > of voltages and currents, but this is covered by 1 regulator instance as it's
> > just a single output. For USB-PD we have a number of options for voltage/current
> > combos, including PPS which is even lower granularity, but it's still only one
> > port. I get the feeling that having PSY instances for each and every PDO might
> > be a little confusing and these will never be concurrent.
> >
> > However, I'd be keen to understand further and see what restrictions/issues are
> > currently present as I probably don't have a complete view of this right now. I
> > wouldn't want to dismiss something out of turn, especially when you obviously
> > have good reason to suggest such an approach.
>
> I thought of one more potential downside to one-psy-per-pdo:
>
> Remember that a source or sink's Capabilities may dynamically change without
> a port disconnect, and this could make one-psy-per-pdo much more chatty with
> power supply deletions and re-registrations on load balancing events.
>
> At basically any time, a power source may send a new SRC_CAP to the sink which
> adjusts, deletes, or adds to the list of PDOs, without the connection state
> machine registering a disconnect.
>
> In a real world case, I have a charger in my kitchen that has 2 USB-C ports
> and supports a total of 30W output. When one device is plugged in:
> 5V 3A, 9V 3A, 15V 2A
> However, when two devices are plugged in, each sees:
> 5V 3A
>
> The load balancing event would result in two power supply deletions, whereas
> if it were a single psy per power supply (incorporating the list of PDO choices)
> it would just be a single PROP_CHANGED event.
>
> It seems cleaner to me to have deletions and additions only possible when the
> thing is unplugged or plugged.
I just argued to Adam that because the capabilities can change in
reality at any time, just like you pointed out here, using a psy
hierarchy instead of trying to handle everything with a single psy is
not only more clear, it's actually safer, and definitely less "hacky"
approach.
I don't really see why would it be a problem to unregister and
register the psys in the hierarchy be a problem?
thanks,
--
heikki
Mon, Sep 13, 2021 at 10:40:08AM -0700, Benson Leung kirjoitti:
> Hi Heikki,
>
> On Mon, Sep 13, 2021 at 04:30:08PM +0300, Heikki Krogerus wrote:
> > My plan is to register a separate power supply for each PDO. So for
> > every source PDO and every sink PDO a port has in its capabilities,
> > you'll have a separate power supply registered, and the same for the
> > partner when it's connected. With every connection there should always
> > be one active (online) source psy and active sink psy (if the port is
> > source, one of the port's source psys will be active and the partner
> > will have the active sink psy, or vise versa - depending on the role).
> >
> > Each PDO represents a "Power Supply" so to me that approach just
> > makes the most sense. It will require a bit of work in kernel, however
> > in user space it should mean that we only have a single new attribute
> > file for the power supplies named "pdo" that returns a single PDO.
>
> There's a few downsides to this approach (one psy for each pdo).
>
> The PDOs returned by Source Capabilities and Sink Capabilities do not just
> contain possible Voltage, Current, and Power combinations, but they also contain
> additional information in the form of properties.
>
> In the Fixed Supply PDO, the following bits convey information about the supply
> or sink (See USB PD Spec R3.1 V1.0 Table 6-9):
>
> * B29 - Dual-Role Power
> * B28 - USB Suspend Supported
> * B27 - Unconstrained Power
> * B26 - USB Communications Capable
> * B25 - Dual-Role Data
> * B24 - Unchunked Extended Messages Supported
> * B23 - EPR Mode Capable
>
> These bits exist in every single 32-bit Fixed Supply PDO, however,
> Section 6.4.1.2.2 requires that they be appropriately set in the vSafe5V Fixed
> PDO (which is required for all sources and sinks), and set to 0 in all other
> PDOs in the caps.
>
> > Since all USB Providers support vSafe5V, the required vSafe5V Fixed Supply
> > Power Data Object is also used to convey additional information that is
> > returned in bits 29 through 25. All other Fixed Supply Power Data Objects
> > Shall set bits 29…22 to zero.
>
> So, splitting out a particular port partner or port's PDOs into individual
> power supplies runs the risk of the information above not properly being
> attributed to the actual power supply.
>
> For example, if you're connected to a 18W power supply that has a vSafe5V PDO,
> and a 9V Fixed PDO, and you're operating at 18W, the 9V Fixed PDO will be Active
> but the inactive vSafe5V PDO has information in those higher order bits that
> remain relevant.
>
> Just something to keep in mind.
The section 6.4.1 dictates that the Capabilities Message (source or
sink) shall always contain the vSafe5V Fixes Supply object and that
it's always the first object. Therefore we should expect it to also
represent the first source and/or sink psy under the port/partner psy.
It would probable be easiest to just expose the missing details from
those extra bits in vSafe5V object under the typec port and partner
devices by providing separate sysfs files for them if needed.
thanks,
--
heikki
Hi Heikki,
On Tue, Sep 14, 2021 at 01:14:02PM +0300, Heikki Krogerus wrote:
> Mon, Sep 13, 2021 at 03:15:46PM +0000, Adam Thomson kirjoitti:
> >
> > Hi Heikki,
> >
> > Thanks for CCing me. My two pence worth is that I always envisaged the PSY
> > representation as being 1 PSY for 1 power source. I consider this in a
> > similar manner to the Regulator framework, where 1 regulator can support a range
> > of voltages and currents, but this is covered by 1 regulator instance as it's
> > just a single output. For USB-PD we have a number of options for voltage/current
> > combos, including PPS which is even lower granularity, but it's still only one
> > port. I get the feeling that having PSY instances for each and every PDO might
> > be a little confusing and these will never be concurrent.
> >
> > However, I'd be keen to understand further and see what restrictions/issues are
> > currently present as I probably don't have a complete view of this right now. I
> > wouldn't want to dismiss something out of turn, especially when you obviously
> > have good reason to suggest such an approach.
>
> I'm not proposing that we drop the port-psys. I'm sorry if I've been
> unclear about that. The port-psy we can not drop because of several
> reasons. For starters, we still can not assume that USB PD is always
> supported.
>
> What I'm trying to propose is that we take advantage of the
> power-supply framework by building a "dynamic" hierarchy of power
> supplies that supply each other in order to represent the actual
> situation as closely as possible. For example, a port-psy that is
> supplied by port-Fixed-sink-psy that is supplied by
> port-partner-Fixed-source (that is supplied by port-partner-psy).
> Something like that. The only "static" part in the hierarchy is the
> port-psy, as everything else about it can change, even without
> disconnection.
>
> So the port-psy always either supplies another psy or is supplied by
> another psy in this hierarchy, depending on the role of the port. But
> most importantly, the properties of the port-psy itself are _newer_
> adjustable - they are read-only. The psy that supplies the port-psy
> can be adjustable, if it's for example PPS, but not the port-psy
> itself.
>
> The problem with having only a single psy per port (and possibly
> partners) is that it does not work well enough when the capabilities
> change, and the capabilities can really change at any moment, we don't
> need to disconnect for that to happen - simply by plugging another
> device to another port can change the power budget for your port and
> change your capabilities. The biggest problem is when we loose the
> ability to adjust the values if we for example loose the PPS that we
> were using in the middle of operation. The single psy has to attempt
> to handle the situation by adjusting something like the ranges of the
> properties, because it can't change the actual property set itself.
> That is hacky, and to be honest, a little bit risky, because it leaves
> us at the mercy of programmers completely unnecessarily.
>
> With my proposal, if the capabilities change, it only means we rebuild
> the psy hierarchy, and that's it. Nothing else needs to be done in
> kernel, and all changes are super visible and clear in user space.
>
Thanks for providing the clarification. So you're proposing a port-psy and a
port-partner-psy that are connected to each other (one supplying the other).
If PD is not present, those two will exist per port and partner, and there
will be information about Type-C current (and possibly BC 1.2 and other
methods?)
Do you have an example hierarchy you could share that explains what it would
look like in /sys/class/power_supply with PD with Source Caps and Sink Caps on
both sides?
I think this all makes sense if the connector class is a read interface
for this info. Have you considered how the type-c connector class and this pd
psy support will handle dynamic PDO changes for advertisement FROM the ports?
For example, let's say you wanted the kernel and user to manage two USB-C ports
with higher power support (meaning, 5V, 9V, 15V, 20V capable), but then your
kernel and user needs to edit the Source Caps on the fly based on load
balancing.
If caps are represented as a group of psys together, how do you as a kernel
and user create an modify the set of Source_Caps you put out on a port?
Thanks,
Benson
> thanks,
>
> --
> heikki
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]
On Thu, Sep 16, 2021 at 12:22:55AM -0700, Benson Leung wrote:
> Hi Heikki,
>
> On Tue, Sep 14, 2021 at 01:14:02PM +0300, Heikki Krogerus wrote:
> > Mon, Sep 13, 2021 at 03:15:46PM +0000, Adam Thomson kirjoitti:
> > >
> > > Hi Heikki,
> > >
> > > Thanks for CCing me. My two pence worth is that I always envisaged the PSY
> > > representation as being 1 PSY for 1 power source. I consider this in a
> > > similar manner to the Regulator framework, where 1 regulator can support a range
> > > of voltages and currents, but this is covered by 1 regulator instance as it's
> > > just a single output. For USB-PD we have a number of options for voltage/current
> > > combos, including PPS which is even lower granularity, but it's still only one
> > > port. I get the feeling that having PSY instances for each and every PDO might
> > > be a little confusing and these will never be concurrent.
> > >
> > > However, I'd be keen to understand further and see what restrictions/issues are
> > > currently present as I probably don't have a complete view of this right now. I
> > > wouldn't want to dismiss something out of turn, especially when you obviously
> > > have good reason to suggest such an approach.
> >
> > I'm not proposing that we drop the port-psys. I'm sorry if I've been
> > unclear about that. The port-psy we can not drop because of several
> > reasons. For starters, we still can not assume that USB PD is always
> > supported.
> >
> > What I'm trying to propose is that we take advantage of the
> > power-supply framework by building a "dynamic" hierarchy of power
> > supplies that supply each other in order to represent the actual
> > situation as closely as possible. For example, a port-psy that is
> > supplied by port-Fixed-sink-psy that is supplied by
> > port-partner-Fixed-source (that is supplied by port-partner-psy).
> > Something like that. The only "static" part in the hierarchy is the
> > port-psy, as everything else about it can change, even without
> > disconnection.
> >
> > So the port-psy always either supplies another psy or is supplied by
> > another psy in this hierarchy, depending on the role of the port. But
> > most importantly, the properties of the port-psy itself are _newer_
> > adjustable - they are read-only. The psy that supplies the port-psy
> > can be adjustable, if it's for example PPS, but not the port-psy
> > itself.
> >
> > The problem with having only a single psy per port (and possibly
> > partners) is that it does not work well enough when the capabilities
> > change, and the capabilities can really change at any moment, we don't
> > need to disconnect for that to happen - simply by plugging another
> > device to another port can change the power budget for your port and
> > change your capabilities. The biggest problem is when we loose the
> > ability to adjust the values if we for example loose the PPS that we
> > were using in the middle of operation. The single psy has to attempt
> > to handle the situation by adjusting something like the ranges of the
> > properties, because it can't change the actual property set itself.
> > That is hacky, and to be honest, a little bit risky, because it leaves
> > us at the mercy of programmers completely unnecessarily.
> >
> > With my proposal, if the capabilities change, it only means we rebuild
> > the psy hierarchy, and that's it. Nothing else needs to be done in
> > kernel, and all changes are super visible and clear in user space.
> >
>
> Thanks for providing the clarification. So you're proposing a port-psy and a
> port-partner-psy that are connected to each other (one supplying the other).
> If PD is not present, those two will exist per port and partner, and there
> will be information about Type-C current (and possibly BC 1.2 and other
> methods?)
Yes, exactly.
> Do you have an example hierarchy you could share that explains what it would
> look like in /sys/class/power_supply with PD with Source Caps and Sink Caps on
> both sides?
I don't yet, but I'll prepare something. I did notice already that the
power supply class does not seem to display the suppliers nor
supplicants in sysfs. But we can always improve the class.
I probable should not talk about "hierarchy". Maybe taking about
simply "chain" of power supplies is more correct?
> I think this all makes sense if the connector class is a read interface
> for this info. Have you considered how the type-c connector class and this pd
> psy support will handle dynamic PDO changes for advertisement FROM the ports?
>
> For example, let's say you wanted the kernel and user to manage two USB-C ports
> with higher power support (meaning, 5V, 9V, 15V, 20V capable), but then your
> kernel and user needs to edit the Source Caps on the fly based on load
> balancing.
>
> If caps are represented as a group of psys together, how do you as a kernel
> and user create an modify the set of Source_Caps you put out on a port?
My idea is to utilise the "present" property with the ports. You would
always display all the possible supplies, but only the ones that you
expose in your current capabilities will be present.
The idea is also that the ports are always supplied by normal power
supplies of type "battery", "AC" and what have you. Those you can use
to see the maximum power your port can get, and to determine the
currently available power by checking the other ports that consume the
same supply.
So if you need more power for one port, you most likely will need to
attempt to adjust the power levels of the source PDO power supplies of
the other ports that share the base supply (like battery), or possibly
disable them, and that way enable (make present) more source supplies
for your port. That is the idea, but I admit I have not thought of
everything, so I'm not completely sure would it work exactly like
that, but the power balancing should in any case be possible with the
chain of power supplies one way or the other.
I hope I understood your question correctly, and I hope I was able to
give you an answer :-)
thanks,
--
heikki
On 16 September 2021 11:23, Heikki Krogerus wrote:
> > Thanks for providing the clarification. So you're proposing a port-psy and a
> > port-partner-psy that are connected to each other (one supplying the other).
> > If PD is not present, those two will exist per port and partner, and there
> > will be information about Type-C current (and possibly BC 1.2 and other
> > methods?)
>
> Yes, exactly.
>
> > Do you have an example hierarchy you could share that explains what it would
> > look like in /sys/class/power_supply with PD with Source Caps and Sink Caps on
> > both sides?
>
> I don't yet, but I'll prepare something. I did notice already that the
> power supply class does not seem to display the suppliers nor
> supplicants in sysfs. But we can always improve the class.
>
> I probable should not talk about "hierarchy". Maybe taking about
> simply "chain" of power supplies is more correct?
>
> > I think this all makes sense if the connector class is a read interface
> > for this info. Have you considered how the type-c connector class and this pd
> > psy support will handle dynamic PDO changes for advertisement FROM the
> ports?
> >
> > For example, let's say you wanted the kernel and user to manage two USB-C
> ports
> > with higher power support (meaning, 5V, 9V, 15V, 20V capable), but then your
> > kernel and user needs to edit the Source Caps on the fly based on load
> > balancing.
> >
> > If caps are represented as a group of psys together, how do you as a kernel
> > and user create an modify the set of Source_Caps you put out on a port?
>
> My idea is to utilise the "present" property with the ports. You would
> always display all the possible supplies, but only the ones that you
> expose in your current capabilities will be present.
>
> The idea is also that the ports are always supplied by normal power
> supplies of type "battery", "AC" and what have you. Those you can use
> to see the maximum power your port can get, and to determine the
> currently available power by checking the other ports that consume the
> same supply.
>
> So if you need more power for one port, you most likely will need to
> attempt to adjust the power levels of the source PDO power supplies of
> the other ports that share the base supply (like battery), or possibly
> disable them, and that way enable (make present) more source supplies
> for your port. That is the idea, but I admit I have not thought of
> everything, so I'm not completely sure would it work exactly like
> that, but the power balancing should in any case be possible with the
> chain of power supplies one way or the other.
>
> I hope I understood your question correctly, and I hope I was able to
> give you an answer :-)
Thanks for the additional updates. So is the intention here to move control of
PDO selection away from TCPM, or add more flexibility to it? As I understand it
from previous efforts around all of this, the intention was that TCPM makes the
decision as to which PDO to select (and which APDO for PPS) based on the offered
capabilities of the source and the sink capabilities which are described in FW.
Am just trying to envisage the use-cases here and how the kernel/user is
involved in selecting PDOs/voltages/currents.
IIRC there used to be functions for updating source/sink capabilities but these
never had users and were subsequently removed. I guess this would be essentially
the functional replacement for those APIs?
Personally, I think the idea of source/sink PSY instances supplying each other
seems reasonable. Right now we represent the external PD/Type-C supply (partner)
through TCPM as a PSY instance which is always present for the associated port,
although obviously when no source partner exists we're marked as offline. For
the partner side I'm guessing the PSY instance will be dynamically
created/destroyed? From previous experience PSY classes have tended to be
statically included so would be interested in seeing what this looks like in
reality.
Am still unsure on using PSY to expose individual PDOs though and this still
feels quite heavyweight. I assume we're not wanting to expose everything in PDOs
really, just the voltage/current info? Feels like we should be able to do this
with individual properties which a user can be notified of changes to through
the normal prop notifier, rather than a collection of PSY class instances.
However, I'm happy to be convinced the other way so will await further
details. :)
On Thu, Sep 16, 2021 at 7:12 AM Adam Thomson
<[email protected]> wrote:
>
> On 16 September 2021 11:23, Heikki Krogerus wrote:
>
> > > Thanks for providing the clarification. So you're proposing a port-psy and a
> > > port-partner-psy that are connected to each other (one supplying the other).
> > > If PD is not present, those two will exist per port and partner, and there
> > > will be information about Type-C current (and possibly BC 1.2 and other
> > > methods?)
> >
> > Yes, exactly.
> >
> > > Do you have an example hierarchy you could share that explains what it would
> > > look like in /sys/class/power_supply with PD with Source Caps and Sink Caps on
> > > both sides?
> >
> > I don't yet, but I'll prepare something. I did notice already that the
> > power supply class does not seem to display the suppliers nor
> > supplicants in sysfs. But we can always improve the class.
> >
> > I probable should not talk about "hierarchy". Maybe taking about
> > simply "chain" of power supplies is more correct?
> >
> > > I think this all makes sense if the connector class is a read interface
> > > for this info. Have you considered how the type-c connector class and this pd
> > > psy support will handle dynamic PDO changes for advertisement FROM the
> > ports?
> > >
> > > For example, let's say you wanted the kernel and user to manage two USB-C
> > ports
> > > with higher power support (meaning, 5V, 9V, 15V, 20V capable), but then your
> > > kernel and user needs to edit the Source Caps on the fly based on load
> > > balancing.
Adding a few more instances.
Editing Source Caps on the fly is also applicable for handheld devices
with just one port !
For instance, the phone might want to conserve power and limit the
power supplied to the port partner by adjusting the source caps to
limit battery drain based on the system conditions.
The sink caps can potentially change on the fly as well based on the
charging phase the handheld device is in. For instance, in the last
phase of charging (say when the battery is charged to greater than
80%), it would not make much sense to step down voltage(power losses
due to conversion) from greater than 5V to battery voltage(say ~4.4V).
> > >
> > > If caps are represented as a group of psys together, how do you as a kernel
> > > and user create an modify the set of Source_Caps you put out on a port?
> >
> > My idea is to utilise the "present" property with the ports. You would
> > always display all the possible supplies, but only the ones that you
> > expose in your current capabilities will be present.
> >
> > The idea is also that the ports are always supplied by normal power
> > supplies of type "battery", "AC" and what have you. Those you can use
> > to see the maximum power your port can get, and to determine the
> > currently available power by checking the other ports that consume the
> > same supply.
> >
> > So if you need more power for one port, you most likely will need to
> > attempt to adjust the power levels of the source PDO power supplies of
> > the other ports that share the base supply (like battery), or possibly
> > disable them, and that way enable (make present) more source supplies
> > for your port. That is the idea, but I admit I have not thought of
> > everything, so I'm not completely sure would it work exactly like
> > that, but the power balancing should in any case be possible with the
> > chain of power supplies one way or the other.
> >
> > I hope I understood your question correctly, and I hope I was able to
> > give you an answer :-)
>
> Thanks for the additional updates. So is the intention here to move control of
> PDO selection away from TCPM, or add more flexibility to it? As I understand it
> from previous efforts around all of this, the intention was that TCPM makes the
> decision as to which PDO to select (and which APDO for PPS) based on the offered
> capabilities of the source and the sink capabilities which are described in FW.
> Am just trying to envisage the use-cases here and how the kernel/user is
> involved in selecting PDOs/voltages/currents.
>
> IIRC there used to be functions for updating source/sink capabilities but these
> never had users and were subsequently removed. I guess this would be essentially
> the functional replacement for those APIs?
>
> Personally, I think the idea of source/sink PSY instances supplying each other
> seems reasonable. Right now we represent the external PD/Type-C supply (partner)
> through TCPM as a PSY instance which is always present for the associated port,
> although obviously when no source partner exists we're marked as offline. For
> the partner side I'm guessing the PSY instance will be dynamically
> created/destroyed? From previous experience PSY classes have tended to be
> statically included so would be interested in seeing what this looks like in
> reality.
>
> Am still unsure on using PSY to expose individual PDOs though and this still
> feels quite heavyweight. I assume we're not wanting to expose everything in PDOs
> really, just the voltage/current info? Feels like we should be able to do this
> with individual properties which a user can be notified of changes to through
> the normal prop notifier, rather than a collection of PSY class instances.
> However, I'm happy to be convinced the other way so will await further
> details. :)
On Fri, Sep 17, 2021 at 6:25 AM Badhri Jagan Sridharan
<[email protected]> wrote:
>
> On Thu, Sep 16, 2021 at 7:12 AM Adam Thomson
> <[email protected]> wrote:
> >
> > On 16 September 2021 11:23, Heikki Krogerus wrote:
> >
> > > > Thanks for providing the clarification. So you're proposing a port-psy and a
> > > > port-partner-psy that are connected to each other (one supplying the other).
> > > > If PD is not present, those two will exist per port and partner, and there
> > > > will be information about Type-C current (and possibly BC 1.2 and other
> > > > methods?)
> > >
> > > Yes, exactly.
As Benson mentioned PDOs contain more than power details like USB
Suspend indicator etc or Type-C only devices as Badhri mentioned here
may not integrate well with PSY class. Additionally, it is also
important to consider cable properties here for power as they also
have a role to play in the power limits and necessitates change of
existing PDOs or power limits. ( Type-C Monitor charging a computing
system does not have captive cables)
Given too many possibilities, would an approach similar to
gadgetfs/configfs or cpu scaling say like "type-configfs" or "typec
scaling" ABI framework that allows USB=C port management under one
path /sys/class/typec that allows:
- Provision to manage USB-C port power ( Power supply class should
still represent power contract established, as remaining
characteristics are nested with functional aspects and relevant on a
power contract failure )
+ dynamically change Rp ( Rp(default) is required to enter USB suspend)
+ Set PDO Policy ( PPS, Fixed, etc)
+ Give back power
+ Expose complete PDO ( As we do for VDOs)
+ Change USB Suspend flag
- Provision for extended messages
+ Provides additional details regarding ports like Get Status etc.
This shall allow us to take system level decisions.
- Provision to manage USB-C modes
+ Provision to enter modes as provided by interface standards like UCSI
With this user tools like Chrome OS "typecd" be able to use a single
class and its ABIs to manage USB-C port power and mode. Kindly correct
me if I am missing something here.
On Thu, Sep 16, 2021 at 02:12:23PM +0000, Adam Thomson wrote:
> On 16 September 2021 11:23, Heikki Krogerus wrote:
>
> > > Thanks for providing the clarification. So you're proposing a port-psy and a
> > > port-partner-psy that are connected to each other (one supplying the other).
> > > If PD is not present, those two will exist per port and partner, and there
> > > will be information about Type-C current (and possibly BC 1.2 and other
> > > methods?)
> >
> > Yes, exactly.
> >
> > > Do you have an example hierarchy you could share that explains what it would
> > > look like in /sys/class/power_supply with PD with Source Caps and Sink Caps on
> > > both sides?
> >
> > I don't yet, but I'll prepare something. I did notice already that the
> > power supply class does not seem to display the suppliers nor
> > supplicants in sysfs. But we can always improve the class.
> >
> > I probable should not talk about "hierarchy". Maybe taking about
> > simply "chain" of power supplies is more correct?
> >
> > > I think this all makes sense if the connector class is a read interface
> > > for this info. Have you considered how the type-c connector class and this pd
> > > psy support will handle dynamic PDO changes for advertisement FROM the
> > ports?
> > >
> > > For example, let's say you wanted the kernel and user to manage two USB-C
> > ports
> > > with higher power support (meaning, 5V, 9V, 15V, 20V capable), but then your
> > > kernel and user needs to edit the Source Caps on the fly based on load
> > > balancing.
> > >
> > > If caps are represented as a group of psys together, how do you as a kernel
> > > and user create an modify the set of Source_Caps you put out on a port?
> >
> > My idea is to utilise the "present" property with the ports. You would
> > always display all the possible supplies, but only the ones that you
> > expose in your current capabilities will be present.
> >
> > The idea is also that the ports are always supplied by normal power
> > supplies of type "battery", "AC" and what have you. Those you can use
> > to see the maximum power your port can get, and to determine the
> > currently available power by checking the other ports that consume the
> > same supply.
Going back here a bit... It now looks like this second part is not
possible, which sucks, but it only means registering a separate object
(psy) for each PDO is even more important.
> > So if you need more power for one port, you most likely will need to
> > attempt to adjust the power levels of the source PDO power supplies of
> > the other ports that share the base supply (like battery), or possibly
> > disable them, and that way enable (make present) more source supplies
> > for your port. That is the idea, but I admit I have not thought of
> > everything, so I'm not completely sure would it work exactly like
> > that, but the power balancing should in any case be possible with the
> > chain of power supplies one way or the other.
> >
> > I hope I understood your question correctly, and I hope I was able to
> > give you an answer :-)
>
> Thanks for the additional updates. So is the intention here to move control of
> PDO selection away from TCPM, or add more flexibility to it? As I understand it
> from previous efforts around all of this, the intention was that TCPM makes the
> decision as to which PDO to select (and which APDO for PPS) based on the offered
> capabilities of the source and the sink capabilities which are described in FW.
> Am just trying to envisage the use-cases here and how the kernel/user is
> involved in selecting PDOs/voltages/currents.
If we can leave the decision about the selection to TCPM, that would
be great! I'm not against that at all. As I said, I have not though
through the control aspect. Right now I'm mostly concerned about how
we expose the information to the user. The only reason why I have
considered the control part at all is because how ever we decide to
expose the information to the user, it has to work with control as
well.
> IIRC there used to be functions for updating source/sink capabilities but these
> never had users and were subsequently removed. I guess this would be essentially
> the functional replacement for those APIs?
>
> Personally, I think the idea of source/sink PSY instances supplying each other
> seems reasonable. Right now we represent the external PD/Type-C supply (partner)
> through TCPM as a PSY instance which is always present for the associated port,
> although obviously when no source partner exists we're marked as offline. For
> the partner side I'm guessing the PSY instance will be dynamically
> created/destroyed? From previous experience PSY classes have tended to be
> statically included so would be interested in seeing what this looks like in
> reality.
If there is anything that should be improved in the PSY class itself
(and I'm sure there's plenty), then we improve it.
> Am still unsure on using PSY to expose individual PDOs though and this still
> feels quite heavyweight. I assume we're not wanting to expose everything in PDOs
> really, just the voltage/current info? Feels like we should be able to do this
> with individual properties which a user can be notified of changes to through
> the normal prop notifier, rather than a collection of PSY class instances.
> However, I'm happy to be convinced the other way so will await further
> details. :)
The final PSYs and the supply chains they create as well as the
individual properties I'm more than happy to talk about, but having a
separate object for the smallest thing that we can see (PDO) is the
right thing to do here IMO. Trying to concatenate things into single
objects especially in sysfs, despite how nice it always would seem,
has taken me to the brink of disaster in the past far too many times.
In this case we don't need to take the risk of having to duplicated
information or in worst case deprecate something that is also exposed
to the sysfs in the future.
So the question is not why should we registers every individual PDO
separately. The question is, why shouldn't we do that? And saying that
it's "heavyweight" I'm afraid is not good enough. :-)
Right now I simply don't see any other way that would be as flexible,
or that could even handle all the different platforms in uniform way
(this needs to work with TCPM, UCSI, and everything else), as
registering separate object (psy) for every single PDO.
thanks,
--
heikki
Hi Rajaram,
I'm sorry for the late reply.
On Mon, Sep 20, 2021 at 06:50:22PM +0530, Rajaram R wrote:
> On Fri, Sep 17, 2021 at 6:25 AM Badhri Jagan Sridharan
> <[email protected]> wrote:
> >
> > On Thu, Sep 16, 2021 at 7:12 AM Adam Thomson
> > <[email protected]> wrote:
> > >
> > > On 16 September 2021 11:23, Heikki Krogerus wrote:
> > >
> > > > > Thanks for providing the clarification. So you're proposing a port-psy and a
> > > > > port-partner-psy that are connected to each other (one supplying the other).
> > > > > If PD is not present, those two will exist per port and partner, and there
> > > > > will be information about Type-C current (and possibly BC 1.2 and other
> > > > > methods?)
> > > >
> > > > Yes, exactly.
>
>
> As Benson mentioned PDOs contain more than power details like USB
> Suspend indicator etc or Type-C only devices as Badhri mentioned here
> may not integrate well with PSY class. Additionally, it is also
> important to consider cable properties here for power as they also
> have a role to play in the power limits and necessitates change of
> existing PDOs or power limits. ( Type-C Monitor charging a computing
> system does not have captive cables)
>
> Given too many possibilities, would an approach similar to
> gadgetfs/configfs or cpu scaling say like "type-configfs" or "typec
> scaling" ABI framework that allows USB=C port management under one
> path /sys/class/typec that allows:
>
> - Provision to manage USB-C port power ( Power supply class should
> still represent power contract established, as remaining
> characteristics are nested with functional aspects and relevant on a
> power contract failure )
> + dynamically change Rp ( Rp(default) is required to enter USB suspend)
> + Set PDO Policy ( PPS, Fixed, etc)
> + Give back power
> + Expose complete PDO ( As we do for VDOs)
> + Change USB Suspend flag
>
> - Provision for extended messages
> + Provides additional details regarding ports like Get Status etc.
> This shall allow us to take system level decisions.
>
> - Provision to manage USB-C modes
> + Provision to enter modes as provided by interface standards like UCSI
>
> With this user tools like Chrome OS "typecd" be able to use a single
> class and its ABIs to manage USB-C port power and mode. Kindly correct
> me if I am missing something here.
So I agree that we should have secondary interface to the USB Type-C
ports, cables and partners, and I think the secondary interface should
be "usbpdfs", something similar to usbfs, that you can use to tap into
the protocol layer of the PD stack.
We have to interpret things especially with UCSI, since we can't even
communicate raw VDOs with UCSI, but it will still not be a huge
problem.
I'm quite certain that we should be able to solve a lot of the control
related problems that we now have (so basically lack of control) with
it, but more importantly we would then not need to figure out what is
the correct way to represent every single thing in sysfs in order to
utilise it.
I would imagine this could then at least ideally be the only interface
that also the typecd would need to deal with.
thanks,
--
heikki
On 21 September 2021 11:54, Heikki Krogerus wrote:
> If we can leave the decision about the selection to TCPM, that would
> be great! I'm not against that at all. As I said, I have not though
> through the control aspect. Right now I'm mostly concerned about how
> we expose the information to the user. The only reason why I have
> considered the control part at all is because how ever we decide to
> expose the information to the user, it has to work with control as
> well.
Well part of the discussion has to be about the role that the user plays in
the control. What does and doesn't need to be controlled further up the stack,
and what will be taken care of by, for example, TCPM? Surely that dictates to
some degree what and how we expose all of this? Right now we have a simple means
to read and control voltages and currents through a PSY class, without the need
for the user to know any details of what a PDO/APDO is. Do we continue with
abstracting away to the user or instead let the user decipher this itself and
decide? Am just trying to understand the needs going forward.
> The final PSYs and the supply chains they create as well as the
> individual properties I'm more than happy to talk about, but having a
> separate object for the smallest thing that we can see (PDO) is the
> right thing to do here IMO. Trying to concatenate things into single
> objects especially in sysfs, despite how nice it always would seem,
> has taken me to the brink of disaster in the past far too many times.
>
> In this case we don't need to take the risk of having to duplicated
> information or in worst case deprecate something that is also exposed
> to the sysfs in the future.
>
> So the question is not why should we registers every individual PDO
> separately. The question is, why shouldn't we do that? And saying that
> it's "heavyweight" I'm afraid is not good enough. :-)
That was my initial feeling on the suggestion based on the idea of a PSY per PDO
and I still don't feel that fits as your creating a whole class of resources
to expose something that's pretty small. To me the PSY represents the source as
whole, and the PDOs are simply options/configurations for that source. If we're
needing to expose PDOs then I don't disagree with separating them out
individually and I certainly wouldn't want that all concatenated as one
property. However I think something like dynamically generated properties
might be a nicer solution to expose each PDO, or even groups of properties if
you wanted to split PDOs even further into constituent parts to the user.
Honestly though I'm not really against anything right now. I'm still trying to
build a better view for myself as to how this needs to be used in the future.
Hi folks,
Thanks for the comments and discussion on this RFC series.
On Fri, Sep 24, 2021 at 8:38 AM Adam Thomson
<[email protected]> wrote:
>
> On 21 September 2021 11:54, Heikki Krogerus wrote:
>
> > If we can leave the decision about the selection to TCPM, that would
> > be great! I'm not against that at all. As I said, I have not though
> > through the control aspect. Right now I'm mostly concerned about how
> > we expose the information to the user. The only reason why I have
> > considered the control part at all is because how ever we decide to
> > expose the information to the user, it has to work with control as
> > well.
>
> Well part of the discussion has to be about the role that the user plays in
> the control. What does and doesn't need to be controlled further up the stack,
> and what will be taken care of by, for example, TCPM? Surely that dictates to
> some degree what and how we expose all of this? Right now we have a simple means
> to read and control voltages and currents through a PSY class, without the need
> for the user to know any details of what a PDO/APDO is. Do we continue with
> abstracting away to the user or instead let the user decipher this itself and
> decide? Am just trying to understand the needs going forward.
>
> > The final PSYs and the supply chains they create as well as the
> > individual properties I'm more than happy to talk about, but having a
> > separate object for the smallest thing that we can see (PDO) is the
> > right thing to do here IMO. Trying to concatenate things into single
> > objects especially in sysfs, despite how nice it always would seem,
> > has taken me to the brink of disaster in the past far too many times.
> >
> > In this case we don't need to take the risk of having to duplicated
> > information or in worst case deprecate something that is also exposed
> > to the sysfs in the future.
> >
> > So the question is not why should we registers every individual PDO
> > separately. The question is, why shouldn't we do that? And saying that
> > it's "heavyweight" I'm afraid is not good enough. :-)
>
> That was my initial feeling on the suggestion based on the idea of a PSY per PDO
> and I still don't feel that fits as your creating a whole class of resources
> to expose something that's pretty small. To me the PSY represents the source as
> whole, and the PDOs are simply options/configurations for that source. If we're
> needing to expose PDOs then I don't disagree with separating them out
> individually and I certainly wouldn't want that all concatenated as one
> property. However I think something like dynamically generated properties
> might be a nicer solution to expose each PDO, or even groups of properties if
> you wanted to split PDOs even further into constituent parts to the user.
To downscope this issue for the time being, one of our immediate goals
is to expose the PDOs
to userspace for metrics reporting and potentially for some power
policy control through other
channels (like Chrome OS Embedded Controller).
Would it be acceptable to revise this series to drop the power supply
support for now (since I don't yet
see a consensus on how to implement it for the partner), and just add
sysfs nodes for each PDO ?
This would be akin to how it's being done for identity VDOs right now.
So we would have :
/sys/class/typec/<port>-partner/source_pdos/pdo{1-13}
and
/sys/class/typec/<port>-partner/sink_pdos/pdo{1-13}
and similarly for the port device.
If we want to add additional parsing of the Fixed Supply PDO into
individual properties for the partner/port,
those can of course be added later.
WDYT?
Thanks,
On Thu, Oct 07, 2021 at 03:32:27PM -0700, Prashant Malani wrote:
> Hi folks,
>
> Thanks for the comments and discussion on this RFC series.
>
> On Fri, Sep 24, 2021 at 8:38 AM Adam Thomson
> <[email protected]> wrote:
> >
> > On 21 September 2021 11:54, Heikki Krogerus wrote:
> >
> > > If we can leave the decision about the selection to TCPM, that would
> > > be great! I'm not against that at all. As I said, I have not though
> > > through the control aspect. Right now I'm mostly concerned about how
> > > we expose the information to the user. The only reason why I have
> > > considered the control part at all is because how ever we decide to
> > > expose the information to the user, it has to work with control as
> > > well.
> >
> > Well part of the discussion has to be about the role that the user plays in
> > the control. What does and doesn't need to be controlled further up the stack,
> > and what will be taken care of by, for example, TCPM? Surely that dictates to
> > some degree what and how we expose all of this? Right now we have a simple means
> > to read and control voltages and currents through a PSY class, without the need
> > for the user to know any details of what a PDO/APDO is. Do we continue with
> > abstracting away to the user or instead let the user decipher this itself and
> > decide? Am just trying to understand the needs going forward.
> >
> > > The final PSYs and the supply chains they create as well as the
> > > individual properties I'm more than happy to talk about, but having a
> > > separate object for the smallest thing that we can see (PDO) is the
> > > right thing to do here IMO. Trying to concatenate things into single
> > > objects especially in sysfs, despite how nice it always would seem,
> > > has taken me to the brink of disaster in the past far too many times.
> > >
> > > In this case we don't need to take the risk of having to duplicated
> > > information or in worst case deprecate something that is also exposed
> > > to the sysfs in the future.
> > >
> > > So the question is not why should we registers every individual PDO
> > > separately. The question is, why shouldn't we do that? And saying that
> > > it's "heavyweight" I'm afraid is not good enough. :-)
> >
> > That was my initial feeling on the suggestion based on the idea of a PSY per PDO
> > and I still don't feel that fits as your creating a whole class of resources
> > to expose something that's pretty small. To me the PSY represents the source as
> > whole, and the PDOs are simply options/configurations for that source. If we're
> > needing to expose PDOs then I don't disagree with separating them out
> > individually and I certainly wouldn't want that all concatenated as one
> > property. However I think something like dynamically generated properties
> > might be a nicer solution to expose each PDO, or even groups of properties if
> > you wanted to split PDOs even further into constituent parts to the user.
>
> To downscope this issue for the time being, one of our immediate goals
> is to expose the PDOs
> to userspace for metrics reporting and potentially for some power
> policy control through other
> channels (like Chrome OS Embedded Controller).
>
> Would it be acceptable to revise this series to drop the power supply
> support for now (since I don't yet
> see a consensus on how to implement it for the partner), and just add
> sysfs nodes for each PDO ?
> This would be akin to how it's being done for identity VDOs right now.
>
> So we would have :
>
> /sys/class/typec/<port>-partner/source_pdos/pdo{1-13}
>
> and
>
> /sys/class/typec/<port>-partner/sink_pdos/pdo{1-13}
>
> and similarly for the port device.
>
> If we want to add additional parsing of the Fixed Supply PDO into
> individual properties for the partner/port,
> those can of course be added later.
>
> WDYT?
I don't think we should use sysfs to expose and control any of these
objects. It does not really matter under which subsystem we are
working. Sysfs is just the wrong interface for this kind of data.
I'm now preparing a proof-of-concept patches where I create character
device for every USB PD capable device (port, plug and partner). The
idea is that we could use those char devices to tap into the USB PD
protocol directly. Right now I'm thinking the nodes would look like
this (with the first Type-C port):
/dev/pd0/port
/dev/pd0/plug0 - you only get this node with full featured cables
/dev/pd0/plug1 - ditto
/dev/pd0/partner - and this is here only if you are connected
So in this case you would use those char devices to send the actual
Get_Source_Cap and Get_Sink_Cap messages to get the PDOs.
The problem is that it's not going to be possible to always support
every type of command. For example with UCSI we are pretty much
limited to the capability control messages. But I still think this is
the right way to do this.
Let me know what you think.
thanks,
--
heikki
Hi Heikki,
On Fri, Oct 8, 2021 at 4:10 AM Heikki Krogerus
<[email protected]> wrote:
>
> On Thu, Oct 07, 2021 at 03:32:27PM -0700, Prashant Malani wrote:
> > Hi folks,
> >
> > Thanks for the comments and discussion on this RFC series.
> >
> > On Fri, Sep 24, 2021 at 8:38 AM Adam Thomson
> > <[email protected]> wrote:
> > >
> > > On 21 September 2021 11:54, Heikki Krogerus wrote:
> > >
> > > > If we can leave the decision about the selection to TCPM, that would
> > > > be great! I'm not against that at all. As I said, I have not though
> > > > through the control aspect. Right now I'm mostly concerned about how
> > > > we expose the information to the user. The only reason why I have
> > > > considered the control part at all is because how ever we decide to
> > > > expose the information to the user, it has to work with control as
> > > > well.
> > >
> > > Well part of the discussion has to be about the role that the user plays in
> > > the control. What does and doesn't need to be controlled further up the stack,
> > > and what will be taken care of by, for example, TCPM? Surely that dictates to
> > > some degree what and how we expose all of this? Right now we have a simple means
> > > to read and control voltages and currents through a PSY class, without the need
> > > for the user to know any details of what a PDO/APDO is. Do we continue with
> > > abstracting away to the user or instead let the user decipher this itself and
> > > decide? Am just trying to understand the needs going forward.
> > >
> > > > The final PSYs and the supply chains they create as well as the
> > > > individual properties I'm more than happy to talk about, but having a
> > > > separate object for the smallest thing that we can see (PDO) is the
> > > > right thing to do here IMO. Trying to concatenate things into single
> > > > objects especially in sysfs, despite how nice it always would seem,
> > > > has taken me to the brink of disaster in the past far too many times.
> > > >
> > > > In this case we don't need to take the risk of having to duplicated
> > > > information or in worst case deprecate something that is also exposed
> > > > to the sysfs in the future.
> > > >
> > > > So the question is not why should we registers every individual PDO
> > > > separately. The question is, why shouldn't we do that? And saying that
> > > > it's "heavyweight" I'm afraid is not good enough. :-)
> > >
> > > That was my initial feeling on the suggestion based on the idea of a PSY per PDO
> > > and I still don't feel that fits as your creating a whole class of resources
> > > to expose something that's pretty small. To me the PSY represents the source as
> > > whole, and the PDOs are simply options/configurations for that source. If we're
> > > needing to expose PDOs then I don't disagree with separating them out
> > > individually and I certainly wouldn't want that all concatenated as one
> > > property. However I think something like dynamically generated properties
> > > might be a nicer solution to expose each PDO, or even groups of properties if
> > > you wanted to split PDOs even further into constituent parts to the user.
> >
> > To downscope this issue for the time being, one of our immediate goals
> > is to expose the PDOs
> > to userspace for metrics reporting and potentially for some power
> > policy control through other
> > channels (like Chrome OS Embedded Controller).
> >
> > Would it be acceptable to revise this series to drop the power supply
> > support for now (since I don't yet
> > see a consensus on how to implement it for the partner), and just add
> > sysfs nodes for each PDO ?
> > This would be akin to how it's being done for identity VDOs right now.
> >
> > So we would have :
> >
> > /sys/class/typec/<port>-partner/source_pdos/pdo{1-13}
> >
> > and
> >
> > /sys/class/typec/<port>-partner/sink_pdos/pdo{1-13}
> >
> > and similarly for the port device.
> >
> > If we want to add additional parsing of the Fixed Supply PDO into
> > individual properties for the partner/port,
> > those can of course be added later.
> >
> > WDYT?
>
> I don't think we should use sysfs to expose and control any of these
> objects. It does not really matter under which subsystem we are
> working. Sysfs is just the wrong interface for this kind of data.
>
> I'm now preparing a proof-of-concept patches where I create character
> device for every USB PD capable device (port, plug and partner). The
> idea is that we could use those char devices to tap into the USB PD
> protocol directly. Right now I'm thinking the nodes would look like
> this (with the first Type-C port):
>
> /dev/pd0/port
> /dev/pd0/plug0 - you only get this node with full featured cables
> /dev/pd0/plug1 - ditto
> /dev/pd0/partner - and this is here only if you are connected
>
> So in this case you would use those char devices to send the actual
> Get_Source_Cap and Get_Sink_Cap messages to get the PDOs.
Interesting. Is this ABI going to need to be defined explicitly, or is the plan
to just mimic the PD protocol messages as much as possible?
I'm assuming the PDOs themselves will still need to be stored in the connector
class port/partner data structures, right?
>
> The problem is that it's not going to be possible to always support
> every type of command. For example with UCSI we are pretty much
> limited to the capability control messages. But I still think this is
> the right way to do this.
>
> Let me know what you think.
Sounds good. I look forward to trying out your series when it's ready.
Best regards,
-Prashant
On 08 October 2021 12:10, Heikki Krogerus wrote:
> > To downscope this issue for the time being, one of our immediate goals
> > is to expose the PDOs
> > to userspace for metrics reporting and potentially for some power
> > policy control through other
> > channels (like Chrome OS Embedded Controller).
> >
> > Would it be acceptable to revise this series to drop the power supply
> > support for now (since I don't yet
> > see a consensus on how to implement it for the partner), and just add
> > sysfs nodes for each PDO ?
> > This would be akin to how it's being done for identity VDOs right now.
> >
> > So we would have :
> >
> > /sys/class/typec/<port>-partner/source_pdos/pdo{1-13}
> >
> > and
> >
> > /sys/class/typec/<port>-partner/sink_pdos/pdo{1-13}
> >
> > and similarly for the port device.
> >
> > If we want to add additional parsing of the Fixed Supply PDO into
> > individual properties for the partner/port,
> > those can of course be added later.
> >
> > WDYT?
>
> I don't think we should use sysfs to expose and control any of these
> objects. It does not really matter under which subsystem we are
> working. Sysfs is just the wrong interface for this kind of data.
>
> I'm now preparing a proof-of-concept patches where I create character
> device for every USB PD capable device (port, plug and partner). The
> idea is that we could use those char devices to tap into the USB PD
> protocol directly. Right now I'm thinking the nodes would look like
> this (with the first Type-C port):
>
> /dev/pd0/port
> /dev/pd0/plug0 - you only get this node with full featured cables
> /dev/pd0/plug1 - ditto
> /dev/pd0/partner - and this is here only if you are connected
>
> So in this case you would use those char devices to send the actual
> Get_Source_Cap and Get_Sink_Cap messages to get the PDOs.
>
> The problem is that it's not going to be possible to always support
> every type of command. For example with UCSI we are pretty much
> limited to the capability control messages. But I still think this is
> the right way to do this.
>
> Let me know what you think.
My two pence worth; this feels like a more appropriate mechanism to access that
data. Look forward to seeing the POC.