[RFC PATCH 0/3] Type C partner power supply and PDO support.

[RFC PATCH 0/3] Type C partner power supply and PDO support.

[Prashant Malani]

Hi,

We'd like to expose the USB PD Power Source/Sink capabilities provided
by a partner (SOP) to userspace. Towards that end,
here is a short series which adds the capabilities to the power supply
class. We also add a function to the Type C connector class
to register a power supply device for a connected partner.

I'm sending this out as an RFC to get some initial feedback from the
relevant groups. Once we can settle on an approach, I can resend the
series with a sample implementation in the Chrome OS Type C port driver
(cros-ec-typec).

Thanks!

Prashant Malani (3):
  usb: pd: Increase max PDO objects to 13
  power: supply: Add support for PDOs props
  usb: typec: Add partner power registration call

 Documentation/ABI/testing/sysfs-class-power | 30 +++++++++++++++++++++
 drivers/power/supply/power_supply_sysfs.c   | 18 ++++++++++++-
 drivers/usb/typec/class.c                   | 18 ++++++++++++-
 drivers/usb/typec/class.h                   |  2 ++
 include/linux/power_supply.h                |  5 ++++
 include/linux/usb/pd.h                      |  8 +++++-
 include/linux/usb/typec.h                   |  5 ++++
 7 files changed, 83 insertions(+), 3 deletions(-)

-- 
2.33.0.153.gba50c8fa24-goog

[RFC PATCH 1/3] usb: pd: Increase max PDO objects to 13

[Prashant Malani]

Increase the max number of PDO objects to 13, to accommodate the extra
PDOs added as a part of EPR (Extended Power Range) operation introduced
in the USB PD Spec Rev 3.1, v 1.0. See Figure 6-54 for details.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 include/linux/usb/pd.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
index 96b7ff66f074..7e8bdca1ce6e 100644
--- a/include/linux/usb/pd.h
+++ b/include/linux/usb/pd.h
@@ -201,7 +201,13 @@ struct pd_message {
 } __packed;
 
 /* PDO: Power Data Object */
-#define PDO_MAX_OBJECTS		7
+
+/*
+ * The EPR (Extended Power Range) structure is a superset of the SPR (Standard Power Range)
+ * capabilities structure, so set the max number of PDOs to 13 instead of 7. On SPR-only systems,
+ * objects 8 through 13 will just be empty.
+ */
+#define PDO_MAX_OBJECTS		13
 
 enum pd_pdo_type {
 	PDO_TYPE_FIXED = 0,
-- 
2.33.0.153.gba50c8fa24-goog

[RFC PATCH 2/3] power: supply: Add support for PDOs props

[Prashant Malani]

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.
+
 **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

[RFC PATCH 3/3] usb: typec: Add partner power registration call

[Prashant Malani]

Add a function to register a power supply device for a partner. Also,
ensure that the registered power supply gets unregistered when the
partner is removed.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/usb/typec/class.c | 18 +++++++++++++++++-
 drivers/usb/typec/class.h |  2 ++
 include/linux/usb/typec.h |  5 +++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index aeef453aa658..14a898440342 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -845,11 +845,27 @@ EXPORT_SYMBOL_GPL(typec_register_partner);
  */
 void typec_unregister_partner(struct typec_partner *partner)
 {
-	if (!IS_ERR_OR_NULL(partner))
+	if (!IS_ERR_OR_NULL(partner)) {
+		power_supply_unregister(partner->psy);
 		device_unregister(&partner->dev);
+	}
 }
 EXPORT_SYMBOL_GPL(typec_unregister_partner);
 
+int typec_partner_register_psy(struct typec_partner *partner, const struct power_supply_desc *desc,
+			       const struct power_supply_config *cfg)
+{
+	partner->psy = power_supply_register(&partner->dev, desc, cfg);
+	if (IS_ERR(partner->psy)) {
+		dev_err(&partner->dev, "failed to register partner power supply (%ld)\n",
+				PTR_ERR(partner->psy));
+		return PTR_ERR(partner->psy);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(typec_partner_register_psy);
+
 /* ------------------------------------------------------------------------- */
 /* Type-C Cable Plugs */
 
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
index aef03eb7e152..b75b0f22d982 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -4,6 +4,7 @@
 #define __USB_TYPEC_CLASS__
 
 #include <linux/device.h>
+#include <linux/power_supply.h>
 #include <linux/usb/typec.h>
 
 struct typec_mux;
@@ -33,6 +34,7 @@ struct typec_partner {
 	int				num_altmodes;
 	u16				pd_revision; /* 0300H = "3.0" */
 	enum usb_pd_svdm_ver		svdm_version;
+	struct power_supply		*psy;
 };
 
 struct typec_port {
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index e2e44bb1dad8..905527dab78c 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -22,6 +22,9 @@ struct typec_altmode_ops;
 struct fwnode_handle;
 struct device;
 
+struct power_supply_desc;
+struct power_supply_config;
+
 enum typec_port_type {
 	TYPEC_PORT_SRC,
 	TYPEC_PORT_SNK,
@@ -132,6 +135,8 @@ int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmod
 struct typec_altmode
 *typec_partner_register_altmode(struct typec_partner *partner,
 				const struct typec_altmode_desc *desc);
+int typec_partner_register_psy(struct typec_partner *partner, const struct power_supply_desc *desc,
+			       const struct power_supply_config *cfg);
 int typec_plug_set_num_altmodes(struct typec_plug *plug, int num_altmodes);
 struct typec_altmode
 *typec_plug_register_altmode(struct typec_plug *plug,
-- 
2.33.0.153.gba50c8fa24-goog

Re: [RFC PATCH 1/3] usb: pd: Increase max PDO objects to 13

[Jack Pham]

Hi Prashant,

On Thu, Sep 02, 2021 at 02:34:58PM -0700, Prashant Malani wrote:
> Increase the max number of PDO objects to 13, to accommodate the extra
> PDOs added as a part of EPR (Extended Power Range) operation introduced
> in the USB PD Spec Rev 3.1, v 1.0. See Figure 6-54 for details.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  include/linux/usb/pd.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index 96b7ff66f074..7e8bdca1ce6e 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -201,7 +201,13 @@ struct pd_message {
>  } __packed;
>  
>  /* PDO: Power Data Object */
> -#define PDO_MAX_OBJECTS		7
> +
> +/*
> + * The EPR (Extended Power Range) structure is a superset of the SPR (Standard Power Range)
> + * capabilities structure, so set the max number of PDOs to 13 instead of 7. On SPR-only systems,
> + * objects 8 through 13 will just be empty.
> + */
> +#define PDO_MAX_OBJECTS		13

Hmm this might break the recent change I made to UCSI in commit
1f4642b72be7 ("usb: typec: ucsi: Retrieve all the PDOs instead of just
the first 4").

 520 static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
 521 {
 522         int ret;
 523
 524         /* UCSI max payload means only getting at most 4 PDOs at a time */
 525         ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
 526         if (ret < 0)
 527                 return;
 528
 529         con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
 530         if (con->num_pdos < UCSI_MAX_PDOS)
 531                 return;
 532
 533         /* get the remaining PDOs, if any */
 534         ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
 535                             PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
				 ^^^^^^^^^^^^^^^
This routine calls the UCSI GET_PDOS command for up to 4 PDOs at a time
since that's the most the return payload can carry.  Currently this
assumes that we'd only need to request the PPM at most twice to retrieve
all the PDOs for up to a maximum of 7 (first request for 4 then again if
needed for the remaining 3).  I'm not sure if any existing UCSI FW would
be updatable to support more than 7 PDOs in the future, much less
support EPR.  In fact, current UCSI 1.2 spec [1] Table 4-34 mentions PDO
offset valid values are 0-7 and anything else "shall not be used", so I
don't know how UCSI will eventually cope with EPR without a spec update.

So if this macro changes to 13 then this call would result in a call to
the UCSI GET_PDOS command passing num_pdos == 13-4 = 9 which would
probably result in an error from the PPM FW.  So we might need to retain
the maximum value of 7 PDOs at least for UCSI here.  Maybe that means
this UCSI driver needs to carry its own definition of
UCSI_MAX_TOTAL_PDOS=7 instead of using PDO_MAX_OBJECTS?

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH 1/3] usb: pd: Increase max PDO objects to 13

[Jack Pham]

On Thu, Sep 02, 2021 at 11:47:01PM -0700, Jack Pham wrote:
> Hi Prashant,
> 
> On Thu, Sep 02, 2021 at 02:34:58PM -0700, Prashant Malani wrote:
> > Increase the max number of PDO objects to 13, to accommodate the extra
> > PDOs added as a part of EPR (Extended Power Range) operation introduced
> > in the USB PD Spec Rev 3.1, v 1.0. See Figure 6-54 for details.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  include/linux/usb/pd.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> > index 96b7ff66f074..7e8bdca1ce6e 100644
> > --- a/include/linux/usb/pd.h
> > +++ b/include/linux/usb/pd.h
> > @@ -201,7 +201,13 @@ struct pd_message {
> >  } __packed;
> >  
> >  /* PDO: Power Data Object */
> > -#define PDO_MAX_OBJECTS		7
> > +
> > +/*
> > + * The EPR (Extended Power Range) structure is a superset of the SPR (Standard Power Range)
> > + * capabilities structure, so set the max number of PDOs to 13 instead of 7. On SPR-only systems,
> > + * objects 8 through 13 will just be empty.
> > + */
> > +#define PDO_MAX_OBJECTS		13
> 
> Hmm this might break the recent change I made to UCSI in commit
> 1f4642b72be7 ("usb: typec: ucsi: Retrieve all the PDOs instead of just
> the first 4").
> 
>  520 static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
>  521 {
>  522         int ret;
>  523
>  524         /* UCSI max payload means only getting at most 4 PDOs at a time */
>  525         ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
>  526         if (ret < 0)
>  527                 return;
>  528
>  529         con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
>  530         if (con->num_pdos < UCSI_MAX_PDOS)
>  531                 return;
>  532
>  533         /* get the remaining PDOs, if any */
>  534         ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
>  535                             PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> 				 ^^^^^^^^^^^^^^^
> This routine calls the UCSI GET_PDOS command for up to 4 PDOs at a time
> since that's the most the return payload can carry.  Currently this
> assumes that we'd only need to request the PPM at most twice to retrieve
> all the PDOs for up to a maximum of 7 (first request for 4 then again if
> needed for the remaining 3).  I'm not sure if any existing UCSI FW would
> be updatable to support more than 7 PDOs in the future, much less
> support EPR.  In fact, current UCSI 1.2 spec [1] Table 4-34 mentions PDO

Sorry, forgot the footnote with the link to the spec:
[1] https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/usb-type-c-ucsi-spec.pdf

> offset valid values are 0-7 and anything else "shall not be used", so I
> don't know how UCSI will eventually cope with EPR without a spec update.
> 
> So if this macro changes to 13 then this call would result in a call to
> the UCSI GET_PDOS command passing num_pdos == 13-4 = 9 which would
> probably result in an error from the PPM FW.  So we might need to retain
> the maximum value of 7 PDOs at least for UCSI here.  Maybe that means
> this UCSI driver needs to carry its own definition of
> UCSI_MAX_TOTAL_PDOS=7 instead of using PDO_MAX_OBJECTS?
> 
> Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH 1/3] usb: pd: Increase max PDO objects to 13

[Prashant Malani]

Hi Jack,

Thanks for taking a look at the patch.

On Fri, Sep 3, 2021 at 11:05 AM Jack Pham <jackp@codeaurora.org> wrote:
>
> On Thu, Sep 02, 2021 at 11:47:01PM -0700, Jack Pham wrote:
> > Hi Prashant,
> >
> > On Thu, Sep 02, 2021 at 02:34:58PM -0700, Prashant Malani wrote:
> > > Increase the max number of PDO objects to 13, to accommodate the extra
> > > PDOs added as a part of EPR (Extended Power Range) operation introduced
> > > in the USB PD Spec Rev 3.1, v 1.0. See Figure 6-54 for details.
> > >
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > ---
> > >  include/linux/usb/pd.h | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> > > index 96b7ff66f074..7e8bdca1ce6e 100644
> > > --- a/include/linux/usb/pd.h
> > > +++ b/include/linux/usb/pd.h
> > > @@ -201,7 +201,13 @@ struct pd_message {
> > >  } __packed;
> > >
> > >  /* PDO: Power Data Object */
> > > -#define PDO_MAX_OBJECTS            7
> > > +
> > > +/*
> > > + * The EPR (Extended Power Range) structure is a superset of the SPR (Standard Power Range)
> > > + * capabilities structure, so set the max number of PDOs to 13 instead of 7. On SPR-only systems,
> > > + * objects 8 through 13 will just be empty.
> > > + */
> > > +#define PDO_MAX_OBJECTS            13
> >
> > Hmm this might break the recent change I made to UCSI in commit
> > 1f4642b72be7 ("usb: typec: ucsi: Retrieve all the PDOs instead of just
> > the first 4").
> >
> >  520 static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
> >  521 {
> >  522         int ret;
> >  523
> >  524         /* UCSI max payload means only getting at most 4 PDOs at a time */
> >  525         ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
> >  526         if (ret < 0)
> >  527                 return;
> >  528
> >  529         con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> >  530         if (con->num_pdos < UCSI_MAX_PDOS)
> >  531                 return;
> >  532
> >  533         /* get the remaining PDOs, if any */
> >  534         ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
> >  535                             PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> >                                ^^^^^^^^^^^^^^^
> > This routine calls the UCSI GET_PDOS command for up to 4 PDOs at a time
> > since that's the most the return payload can carry.  Currently this
> > assumes that we'd only need to request the PPM at most twice to retrieve
> > all the PDOs for up to a maximum of 7 (first request for 4 then again if
> > needed for the remaining 3).  I'm not sure if any existing UCSI FW would
> > be updatable to support more than 7 PDOs in the future, much less
> > support EPR.  In fact, current UCSI 1.2 spec [1] Table 4-34 mentions PDO
>
> Sorry, forgot the footnote with the link to the spec:
> [1] https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/usb-type-c-ucsi-spec.pdf
>
> > offset valid values are 0-7 and anything else "shall not be used", so I
> > don't know how UCSI will eventually cope with EPR without a spec update.
> >
> > So if this macro changes to 13 then this call would result in a call to
> > the UCSI GET_PDOS command passing num_pdos == 13-4 = 9 which would
> > probably result in an error from the PPM FW.  So we might need to retain
> > the maximum value of 7 PDOs at least for UCSI here.  Maybe that means
> > this UCSI driver needs to carry its own definition of
> > UCSI_MAX_TOTAL_PDOS=7 instead of using PDO_MAX_OBJECTS?

Thanks for pointing this out. We can perhaps just add another macro
for EPR_PDO_MAX_OBJECTS, and leave the current macro as is for now.

Best regards,

Re: [RFC PATCH 1/3] usb: pd: Increase max PDO objects to 13

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 4085 bytes --]

Hi Jack,

On Thu, Sep 02, 2021 at 11:47:01PM -0700, Jack Pham wrote:
> Hi Prashant,
> 
> On Thu, Sep 02, 2021 at 02:34:58PM -0700, Prashant Malani wrote:
> > Increase the max number of PDO objects to 13, to accommodate the extra
> > PDOs added as a part of EPR (Extended Power Range) operation introduced
> > in the USB PD Spec Rev 3.1, v 1.0. See Figure 6-54 for details.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  include/linux/usb/pd.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> > index 96b7ff66f074..7e8bdca1ce6e 100644
> > --- a/include/linux/usb/pd.h
> > +++ b/include/linux/usb/pd.h
> > @@ -201,7 +201,13 @@ struct pd_message {
> >  } __packed;
> >  
> >  /* PDO: Power Data Object */
> > -#define PDO_MAX_OBJECTS		7
> > +
> > +/*
> > + * The EPR (Extended Power Range) structure is a superset of the SPR (Standard Power Range)
> > + * capabilities structure, so set the max number of PDOs to 13 instead of 7. On SPR-only systems,
> > + * objects 8 through 13 will just be empty.
> > + */
> > +#define PDO_MAX_OBJECTS		13
> 
> Hmm this might break the recent change I made to UCSI in commit
> 1f4642b72be7 ("usb: typec: ucsi: Retrieve all the PDOs instead of just
> the first 4").
> 
>  520 static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
>  521 {
>  522         int ret;
>  523
>  524         /* UCSI max payload means only getting at most 4 PDOs at a time */
>  525         ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
>  526         if (ret < 0)
>  527                 return;
>  528
>  529         con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
>  530         if (con->num_pdos < UCSI_MAX_PDOS)
>  531                 return;
>  532
>  533         /* get the remaining PDOs, if any */
>  534         ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
>  535                             PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> 				 ^^^^^^^^^^^^^^^
> This routine calls the UCSI GET_PDOS command for up to 4 PDOs at a time
> since that's the most the return payload can carry.  Currently this
> assumes that we'd only need to request the PPM at most twice to retrieve
> all the PDOs for up to a maximum of 7 (first request for 4 then again if
> needed for the remaining 3).  I'm not sure if any existing UCSI FW would
> be updatable to support more than 7 PDOs in the future, much less
> support EPR.  In fact, current UCSI 1.2 spec [1] Table 4-34 mentions PDO
> offset valid values are 0-7 and anything else "shall not be used", so I
> don't know how UCSI will eventually cope with EPR without a spec update.
> 

I've had a conversation with Dmitriy Berchanskiy at Intel (the UCSI WG Chair)
about this, and it sounds like the UCSI spec is planned on being revved
(post R2.0) in order to support the additional messages and expanded structures
of USB PD R3.1 around EPR.

> So if this macro changes to 13 then this call would result in a call to
> the UCSI GET_PDOS command passing num_pdos == 13-4 = 9 which would
> probably result in an error from the PPM FW.  So we might need to retain
> the maximum value of 7 PDOs at least for UCSI here.  Maybe that means
> this UCSI driver needs to carry its own definition of
> UCSI_MAX_TOTAL_PDOS=7 instead of using PDO_MAX_OBJECTS?
> 

Prashant mentioned this as well, but maybe it makes sense to define a separate
EPR_PDO_MAX_OBJECTS to handle the EPR case, as there are completely separate
underlying PD messages (EPR_Source_Capabilities) where we expect up to 13
objects, and the classic SPR Source and Sink capabilities will still have the
7 object limit.

Thanks,
Benson

> Jack
> -- 
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 1/3] usb: pd: Increase max PDO objects to 13

[Jack Pham]

Hi Benson,

On Tue, Sep 07, 2021 at 04:28:53PM -0700, Benson Leung wrote:
> Hi Jack,
> 
> On Thu, Sep 02, 2021 at 11:47:01PM -0700, Jack Pham wrote:
> > Hi Prashant,
> > 
> > On Thu, Sep 02, 2021 at 02:34:58PM -0700, Prashant Malani wrote:
> > > Increase the max number of PDO objects to 13, to accommodate the extra
> > > PDOs added as a part of EPR (Extended Power Range) operation introduced
> > > in the USB PD Spec Rev 3.1, v 1.0. See Figure 6-54 for details.
> > > 
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > ---
> > >  include/linux/usb/pd.h | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> > > index 96b7ff66f074..7e8bdca1ce6e 100644
> > > --- a/include/linux/usb/pd.h
> > > +++ b/include/linux/usb/pd.h
> > > @@ -201,7 +201,13 @@ struct pd_message {
> > >  } __packed;
> > >  
> > >  /* PDO: Power Data Object */
> > > -#define PDO_MAX_OBJECTS		7
> > > +
> > > +/*
> > > + * The EPR (Extended Power Range) structure is a superset of the SPR (Standard Power Range)
> > > + * capabilities structure, so set the max number of PDOs to 13 instead of 7. On SPR-only systems,
> > > + * objects 8 through 13 will just be empty.
> > > + */
> > > +#define PDO_MAX_OBJECTS		13
> > 
> > Hmm this might break the recent change I made to UCSI in commit
> > 1f4642b72be7 ("usb: typec: ucsi: Retrieve all the PDOs instead of just
> > the first 4").
> > 
> >  520 static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
> >  521 {
> >  522         int ret;
> >  523
> >  524         /* UCSI max payload means only getting at most 4 PDOs at a time */
> >  525         ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
> >  526         if (ret < 0)
> >  527                 return;
> >  528
> >  529         con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> >  530         if (con->num_pdos < UCSI_MAX_PDOS)
> >  531                 return;
> >  532
> >  533         /* get the remaining PDOs, if any */
> >  534         ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
> >  535                             PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> > 				 ^^^^^^^^^^^^^^^
> > This routine calls the UCSI GET_PDOS command for up to 4 PDOs at a time
> > since that's the most the return payload can carry.  Currently this
> > assumes that we'd only need to request the PPM at most twice to retrieve
> > all the PDOs for up to a maximum of 7 (first request for 4 then again if
> > needed for the remaining 3).  I'm not sure if any existing UCSI FW would
> > be updatable to support more than 7 PDOs in the future, much less
> > support EPR.  In fact, current UCSI 1.2 spec [1] Table 4-34 mentions PDO
> > offset valid values are 0-7 and anything else "shall not be used", so I
> > don't know how UCSI will eventually cope with EPR without a spec update.
> > 
> 
> I've had a conversation with Dmitriy Berchanskiy at Intel (the UCSI WG Chair)
> about this, and it sounds like the UCSI spec is planned on being revved
> (post R2.0) in order to support the additional messages and expanded structures
> of USB PD R3.1 around EPR.

Good to know! Look forward to seeing it once it's ready.

I've access to the current R2.0 draft as well, and it looks like there's
gonna be a bit of work to update this driver to support it.  The big
standout is the data structure format change to accommodate much larger
payloads.  So I guess that bridge will be constructed when we get there,
both for 2.0 and later for EPR.

> > So if this macro changes to 13 then this call would result in a call to
> > the UCSI GET_PDOS command passing num_pdos == 13-4 = 9 which would
> > probably result in an error from the PPM FW.  So we might need to retain
> > the maximum value of 7 PDOs at least for UCSI here.  Maybe that means
> > this UCSI driver needs to carry its own definition of
> > UCSI_MAX_TOTAL_PDOS=7 instead of using PDO_MAX_OBJECTS?
> > 
> 
> Prashant mentioned this as well, but maybe it makes sense to define a separate
> EPR_PDO_MAX_OBJECTS to handle the EPR case, as there are completely separate
> underlying PD messages (EPR_Source_Capabilities) where we expect up to 13
> objects, and the classic SPR Source and Sink capabilities will still have the
> 7 object limit.

Sounds good to me FWIW.  Plus this will even avoid unnecessarily
bloating TCPM's internal {source,sink}_caps and {src,snk}_pdo arrays
(an additional 96 bytes) prematurely before that driver is ready to be
updated to handle EPR with all the new messages and states needed.

Thanks,
Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Heikki Krogerus]

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,

-- 
heikki

RE: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Adam Thomson]

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.

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 3531 bytes --]

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.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 3480 bytes --]

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.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Heikki Krogerus]

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,

-- 
heikki

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Heikki Krogerus]

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

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Heikki Krogerus]

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

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 4667 bytes --]

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.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Heikki Krogerus]

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

RE: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Adam Thomson]

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. :)

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Badhri Jagan Sridharan]

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. :)

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Rajaram R]

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.

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Heikki Krogerus]

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

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Heikki Krogerus]

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,

-- 
heikki

RE: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Adam Thomson]

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.

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Prashant Malani]

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,

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Heikki Krogerus]

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,

-- 
heikki

Re: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Prashant Malani]

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

RE: [RFC PATCH 2/3] power: supply: Add support for PDOs props

[Adam Thomson]

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.