| Message ID | 20210902213500.3795948-3-pmalani@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Type C partner power supply and PDO support. | expand |
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 <pmalani@chromium.org> > --- > 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: linux-pm@vger.kernel.org > +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: linux-pm@vger.kernel.org > +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,
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 <pmalani@chromium.org> > > --- > > 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: linux-pm@vger.kernel.org > > +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: linux-pm@vger.kernel.org > > +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
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
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 <pmalani@chromium.org> > > > --- > > > 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: linux-pm@vger.kernel.org > > > +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: linux-pm@vger.kernel.org > > > +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,
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,
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,
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
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,
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 <Adam.Thomson.Opensource@diasemi.com> 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 <badhri@google.com> wrote: > > On Thu, Sep 16, 2021 at 7:12 AM Adam Thomson > <Adam.Thomson.Opensource@diasemi.com> 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,
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 > <badhri@google.com> wrote: > > > > On Thu, Sep 16, 2021 at 7:12 AM Adam Thomson > > <Adam.Thomson.Opensource@diasemi.com> 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,
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 <Adam.Thomson.Opensource@diasemi.com> 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 > <Adam.Thomson.Opensource@diasemi.com> 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,
Hi Heikki, On Fri, Oct 8, 2021 at 4:10 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> 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 > > <Adam.Thomson.Opensource@diasemi.com> 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.
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: linux-pm@vger.kernel.org +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: linux-pm@vger.kernel.org +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;
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 <pmalani@chromium.org> --- 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(-)