[PATCH 1/2 v7] typec: tcpm: Validate source and sink caps

[PATCH 1/2 v7] typec: tcpm: Validate source and sink caps

[Badhri Jagan Sridharan]

The source and sink caps should follow the following rules.
This patch validates whether the src_caps/snk_caps adheres
to it.

6.4.1 Capabilities Message
A Capabilities message (Source Capabilities message or Sink
Capabilities message) shall have at least one Power
Data Object for vSafe5V. The Capabilities message shall also
contain the sending Port’s information followed by up to
6 additional Power Data Objects. Power Data Objects in a
Capabilities message shall be sent in the following order:

1. The vSafe5V Fixed Supply Object shall always be the first object.
2. The remaining Fixed Supply Objects, if present, shall be sent
   in voltage order; lowest to highest.
3. The Battery Supply Objects, if present shall be sent in Minimum
   Voltage order; lowest to highest.
4. The Variable Supply (non-battery) Objects, if present, shall be
   sent in Minimum Voltage order; lowest to highest.

Errors in source/sink_caps of the local port will prevent
the port registration. Whereas, errors in source caps of partner
device would only log them.

Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>

---
Changelog since v1:
- rebased on top drivers/usb/typec/tcpm.c as suggested by
  gregkh@linuxfoundation.org
- renamed nr_snk_*_pdo as suggested by dan.carpenter@oracle.com
- removed stale comment as suggested by linux@roeck-us.net
- removed the tests for nr_snk_*_pdo as suggested by
  dan.carpenter@oracle.com
- Fixed sytling as suggested by dan.carpenter@oracle.com
- renamed tcpm_get_nr_type_pdos to nr_type_pdos as suggested by
  dan.carpenter@oracle.com
- fixed nr_type_pdos as suggested by dan.carpenter@oracle.com
- tcpm_pd_select_pdo now checks for all matching variable/batt pdos
  instead of the first matching one.

Changelog since v2:
- refactored error messages as suggested by
  heikki.krogerus@linux.intel.com

Changelog since v3:
- fixed formatting errors as suggested by
  heikki.krogerus@linux.intel.com

Changelog since v4:
- Reusing the macro

Changelog since v5:
- Moved to enum for pdo_cap_err messages as suggested by
 heikki.krogerus@linux.intel.com
- Ack by heikki.krogerus@linux.intel.com

Changelog since v6:
- Added Reviewed-by: Guenter Roeck <linux@roeck-us.net>

 drivers/usb/typec/tcpm.c | 130 +++++++++++++++++++++++++++++++++++++++++++----
 include/linux/usb/pd.h   |   2 +
 include/linux/usb/tcpm.h |  16 +++---
 3 files changed, 131 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index c166fc77dfb8..8b637a4b474b 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -1247,6 +1247,100 @@ static void vdm_state_machine_work(struct work_struct *work)
 	mutex_unlock(&port->lock);
 }
 
+enum pdo_err {
+	PDO_NO_ERR,
+	PDO_ERR_NO_VSAFE5V,
+	PDO_ERR_VSAFE5V_NOT_FIRST,
+	PDO_ERR_PDO_TYPE_NOT_IN_ORDER,
+	PDO_ERR_FIXED_NOT_SORTED,
+	PDO_ERR_VARIABLE_BATT_NOT_SORTED,
+	PDO_ERR_DUPE_PDO,
+};
+
+static const char * const pdo_err_msg[] = {
+	[PDO_ERR_NO_VSAFE5V] =
+	" err: source/sink caps should atleast have vSafe5V",
+	[PDO_ERR_VSAFE5V_NOT_FIRST] =
+	" err: vSafe5V Fixed Supply Object Shall always be the first object",
+	[PDO_ERR_PDO_TYPE_NOT_IN_ORDER] =
+	" err: PDOs should be in the following order: Fixed; Battery; Variable",
+	[PDO_ERR_FIXED_NOT_SORTED] =
+	" err: Fixed supply pdos should be in increasing order of their fixed voltage",
+	[PDO_ERR_VARIABLE_BATT_NOT_SORTED] =
+	" err: Variable/Battery supply pdos should be in increasing order of their minimum voltage",
+	[PDO_ERR_DUPE_PDO] =
+	" err: Variable/Batt supply pdos cannot have same min/max voltage",
+};
+
+static enum pdo_err tcpm_caps_err(struct tcpm_port *port, const u32 *pdo,
+				  unsigned int nr_pdo)
+{
+	unsigned int i;
+
+	/* Should at least contain vSafe5v */
+	if (nr_pdo < 1)
+		return PDO_ERR_NO_VSAFE5V;
+
+	/* The vSafe5V Fixed Supply Object Shall always be the first object */
+	if (pdo_type(pdo[0]) != PDO_TYPE_FIXED ||
+	    pdo_fixed_voltage(pdo[0]) != VSAFE5V)
+		return PDO_ERR_VSAFE5V_NOT_FIRST;
+
+	for (i = 1; i < nr_pdo; i++) {
+		if (pdo_type(pdo[i]) < pdo_type(pdo[i - 1])) {
+			return PDO_ERR_PDO_TYPE_NOT_IN_ORDER;
+		} else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1])) {
+			enum pd_pdo_type type = pdo_type(pdo[i]);
+
+			switch (type) {
+			/*
+			 * The remaining Fixed Supply Objects, if
+			 * present, shall be sent in voltage order;
+			 * lowest to highest.
+			 */
+			case PDO_TYPE_FIXED:
+				if (pdo_fixed_voltage(pdo[i]) <=
+				    pdo_fixed_voltage(pdo[i - 1]))
+					return PDO_ERR_FIXED_NOT_SORTED;
+				break;
+			/*
+			 * The Battery Supply Objects and Variable
+			 * supply, if present shall be sent in Minimum
+			 * Voltage order; lowest to highest.
+			 */
+			case PDO_TYPE_VAR:
+			case PDO_TYPE_BATT:
+				if (pdo_min_voltage(pdo[i]) <
+				    pdo_min_voltage(pdo[i - 1]))
+					return PDO_ERR_VARIABLE_BATT_NOT_SORTED;
+				else if ((pdo_min_voltage(pdo[i]) ==
+					  pdo_min_voltage(pdo[i - 1])) &&
+					 (pdo_max_voltage(pdo[i]) ==
+					  pdo_min_voltage(pdo[i - 1])))
+					return PDO_ERR_DUPE_PDO;
+				break;
+			default:
+				tcpm_log_force(port, " Unknown pdo type");
+			}
+		}
+	}
+
+	return PDO_NO_ERR;
+}
+
+static int tcpm_validate_caps(struct tcpm_port *port, const u32 *pdo,
+			      unsigned int nr_pdo)
+{
+	enum pdo_err err_index = tcpm_caps_err(port, pdo, nr_pdo);
+
+	if (err_index != PDO_NO_ERR) {
+		tcpm_log_force(port, " %s", pdo_err_msg[err_index]);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * PD (data, control) command handling functions
  */
@@ -1269,6 +1363,9 @@ static void tcpm_pd_data_request(struct tcpm_port *port,
 
 		tcpm_log_source_caps(port);
 
+		tcpm_validate_caps(port, port->source_caps,
+				   port->nr_source_caps);
+
 		/*
 		 * This message may be received even if VBUS is not
 		 * present. This is quite unexpected; see USB PD
@@ -3435,9 +3532,12 @@ static int tcpm_copy_vdos(u32 *dest_vdo, const u32 *src_vdo,
 	return nr_vdo;
 }
 
-void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
-				     unsigned int nr_pdo)
+int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
+				    unsigned int nr_pdo)
 {
+	if (tcpm_validate_caps(port, pdo, nr_pdo))
+		return -EINVAL;
+
 	mutex_lock(&port->lock);
 	port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, pdo, nr_pdo);
 	switch (port->state) {
@@ -3457,16 +3557,20 @@ void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
 		break;
 	}
 	mutex_unlock(&port->lock);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(tcpm_update_source_capabilities);
 
-void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
-				   unsigned int nr_pdo,
-				   unsigned int max_snk_mv,
-				   unsigned int max_snk_ma,
-				   unsigned int max_snk_mw,
-				   unsigned int operating_snk_mw)
+int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
+				  unsigned int nr_pdo,
+				  unsigned int max_snk_mv,
+				  unsigned int max_snk_ma,
+				  unsigned int max_snk_mw,
+				  unsigned int operating_snk_mw)
 {
+	if (tcpm_validate_caps(port, pdo, nr_pdo))
+		return -EINVAL;
+
 	mutex_lock(&port->lock);
 	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, pdo, nr_pdo);
 	port->max_snk_mv = max_snk_mv;
@@ -3485,6 +3589,7 @@ void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
 		break;
 	}
 	mutex_unlock(&port->lock);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
 
@@ -3520,7 +3625,15 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 
 	init_completion(&port->tx_complete);
 	init_completion(&port->swap_complete);
+	tcpm_debugfs_init(port);
 
+	if (tcpm_validate_caps(port, tcpc->config->src_pdo,
+			       tcpc->config->nr_src_pdo) ||
+	    tcpm_validate_caps(port, tcpc->config->snk_pdo,
+			       tcpc->config->nr_snk_pdo)) {
+		err = -EINVAL;
+		goto out_destroy_wq;
+	}
 	port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, tcpc->config->src_pdo,
 					  tcpc->config->nr_src_pdo);
 	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo,
@@ -3575,7 +3688,6 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 		}
 	}
 
-	tcpm_debugfs_init(port);
 	mutex_lock(&port->lock);
 	tcpm_init(port);
 	mutex_unlock(&port->lock);
diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
index e00051ced806..b3d41d7409b3 100644
--- a/include/linux/usb/pd.h
+++ b/include/linux/usb/pd.h
@@ -148,6 +148,8 @@ enum pd_pdo_type {
 	(PDO_TYPE(PDO_TYPE_FIXED) | (flags) |		\
 	 PDO_FIXED_VOLT(mv) | PDO_FIXED_CURR(ma))
 
+#define VSAFE5V 5000 /* mv units */
+
 #define PDO_BATT_MAX_VOLT_SHIFT	20	/* 50mV units */
 #define PDO_BATT_MIN_VOLT_SHIFT	10	/* 50mV units */
 #define PDO_BATT_MAX_PWR_SHIFT	0	/* 250mW units */
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 073197f0d2bb..ca1c0b57f03f 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -183,14 +183,14 @@ struct tcpm_port;
 struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc);
 void tcpm_unregister_port(struct tcpm_port *port);
 
-void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
-				     unsigned int nr_pdo);
-void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
-				   unsigned int nr_pdo,
-				   unsigned int max_snk_mv,
-				   unsigned int max_snk_ma,
-				   unsigned int max_snk_mw,
-				   unsigned int operating_snk_mw);
+int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
+				    unsigned int nr_pdo);
+int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
+				  unsigned int nr_pdo,
+				  unsigned int max_snk_mv,
+				  unsigned int max_snk_ma,
+				  unsigned int max_snk_mw,
+				  unsigned int operating_snk_mw);
 
 void tcpm_vbus_change(struct tcpm_port *port);
 void tcpm_cc_change(struct tcpm_port *port);
-- 
2.15.0.448.gf294e3d99a-goog

[PATCH 2/2 v7] typec: tcpm: Only request matching pdos

[Badhri Jagan Sridharan]

At present, TCPM code assumes that local device supports
variable/batt pdos and always selects the pdo with highest
possible power within the board limit. This assumption
might not hold good for all devices. To overcome this,
this patch makes TCPM only accept a source_pdo when there is
a matching sink pdo.

For Fixed pdos: The voltage should match between the
incoming source_cap and the registered snk_pdo
For Variable/Batt pdos: The incoming source_cap voltage
range should fall within the registered snk_pdo's voltage
range.

Also, when the cap_mismatch bit is set, the max_power/current
should be set to the max_current/power of the sink_pdo.
This is according to:

"If the Capability Mismatch bit is set to one
The Maximum Operating Current/Power field may contain a value
larger than the maximum current/power offered in the Source
Capabilities message’s PDO as referenced by the Object position field.
This enables the Sink to indicate that it requires more current/power
than is being offered. If the Sink requires a different voltage this
will be indicated by its Sink Capabilities message.

Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>

---
Changelog since v1:
- Rebased the patch on top of drivers/usb/type/tcpm.c
- Added duplicate pdo check for variable/batt pdo.
- Fixed tabs as suggested by dan.carpenter@oracle.com

Changelog since v2:
- Rebase

Changelog since v3:
- Refactored code fixed formatting issues as suggested
  by heikki.krogerus@linux.intel.com

Changelog since v4:
- Rebase

Changelog since v5:
- handling the sink pdo index as pointer argument in tcpm_pd_select_pdo
- ACK by heikki.krogerus@linux.intel.com

Changelog since v6:
- Added Reviewed-by: Guenter Roeck <linux@roeck-us.net>

 drivers/usb/typec/tcpm.c | 163 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 121 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 8b637a4b474b..f4d563ee7690 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -252,6 +252,9 @@ struct tcpm_port {
 	unsigned int nr_src_pdo;
 	u32 snk_pdo[PDO_MAX_OBJECTS];
 	unsigned int nr_snk_pdo;
+	unsigned int nr_fixed; /* number of fixed sink PDOs */
+	unsigned int nr_var; /* number of variable sink PDOs */
+	unsigned int nr_batt; /* number of battery sink PDOs */
 	u32 snk_vdo[VDO_MAX_OBJECTS];
 	unsigned int nr_snk_vdo;
 
@@ -1767,39 +1770,90 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
 	return 0;
 }
 
-static int tcpm_pd_select_pdo(struct tcpm_port *port)
+#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
+#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
+
+static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
+			      int *src_pdo)
 {
-	unsigned int i, max_mw = 0, max_mv = 0;
+	unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
 	int ret = -EINVAL;
 
 	/*
-	 * Select the source PDO providing the most power while staying within
-	 * the board's voltage limits. Prefer PDO providing exp
+	 * Select the source PDO providing the most power which has a
+	 * matchig sink cap.
 	 */
 	for (i = 0; i < port->nr_source_caps; i++) {
 		u32 pdo = port->source_caps[i];
 		enum pd_pdo_type type = pdo_type(pdo);
-		unsigned int mv, ma, mw;
 
-		if (type == PDO_TYPE_FIXED)
-			mv = pdo_fixed_voltage(pdo);
-		else
-			mv = pdo_min_voltage(pdo);
-
-		if (type == PDO_TYPE_BATT) {
-			mw = pdo_max_power(pdo);
-		} else {
-			ma = min(pdo_max_current(pdo),
-				 port->max_snk_ma);
-			mw = ma * mv / 1000;
-		}
-
-		/* Perfer higher voltages if available */
-		if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
-		    mv <= port->max_snk_mv) {
-			ret = i;
-			max_mw = mw;
-			max_mv = mv;
+		if (type == PDO_TYPE_FIXED) {
+			for (j = 0; j < port->nr_fixed; j++) {
+				if (pdo_fixed_voltage(pdo) ==
+				    pdo_fixed_voltage(port->snk_pdo[j])) {
+					ma = min_current(pdo, port->snk_pdo[j]);
+					mv = pdo_fixed_voltage(pdo);
+					mw = ma * mv / 1000;
+					if (mw > max_mw ||
+					    (mw == max_mw && mv > max_mv)) {
+						ret = 0;
+						*src_pdo = i;
+						*sink_pdo = j;
+						max_mw = mw;
+						max_mv = mv;
+					}
+					/* There could only be one fixed pdo
+					 * at a specific voltage level.
+					 * So breaking here.
+					 */
+					break;
+				}
+			}
+		} else if (type == PDO_TYPE_BATT) {
+			for (j = port->nr_fixed;
+			     j < port->nr_fixed +
+				 port->nr_batt;
+			     j++) {
+				if (pdo_min_voltage(pdo) >=
+				     pdo_min_voltage(port->snk_pdo[j]) &&
+				     pdo_max_voltage(pdo) <=
+				     pdo_max_voltage(port->snk_pdo[j])) {
+					mw = min_power(pdo, port->snk_pdo[j]);
+					mv = pdo_min_voltage(pdo);
+					if (mw > max_mw ||
+					    (mw == max_mw && mv > max_mv)) {
+						ret = 0;
+						*src_pdo = i;
+						*sink_pdo = j;
+						max_mw = mw;
+						max_mv = mv;
+					}
+				}
+			}
+		} else if (type == PDO_TYPE_VAR) {
+			for (j = port->nr_fixed +
+				 port->nr_batt;
+			     j < port->nr_fixed +
+				 port->nr_batt +
+				 port->nr_var;
+			     j++) {
+				if (pdo_min_voltage(pdo) >=
+				     pdo_min_voltage(port->snk_pdo[j]) &&
+				     pdo_max_voltage(pdo) <=
+				     pdo_max_voltage(port->snk_pdo[j])) {
+					ma = min_current(pdo, port->snk_pdo[j]);
+					mv = pdo_min_voltage(pdo);
+					mw = ma * mv / 1000;
+					if (mw > max_mw ||
+					    (mw == max_mw && mv > max_mv)) {
+						ret = 0;
+						*src_pdo = i;
+						*sink_pdo = j;
+						max_mw = mw;
+						max_mv = mv;
+					}
+				}
+			}
 		}
 	}
 
@@ -1811,13 +1865,14 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
 	unsigned int mv, ma, mw, flags;
 	unsigned int max_ma, max_mw;
 	enum pd_pdo_type type;
-	int index;
-	u32 pdo;
+	int src_pdo_index, snk_pdo_index;
+	u32 pdo, matching_snk_pdo;
 
-	index = tcpm_pd_select_pdo(port);
-	if (index < 0)
+	if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
 		return -EINVAL;
-	pdo = port->source_caps[index];
+
+	pdo = port->source_caps[src_pdo_index];
+	matching_snk_pdo = port->snk_pdo[snk_pdo_index];
 	type = pdo_type(pdo);
 
 	if (type == PDO_TYPE_FIXED)
@@ -1825,26 +1880,28 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
 	else
 		mv = pdo_min_voltage(pdo);
 
-	/* Select maximum available current within the board's power limit */
+	/* Select maximum available current within the sink pdo's limit */
 	if (type == PDO_TYPE_BATT) {
-		mw = pdo_max_power(pdo);
-		ma = 1000 * min(mw, port->max_snk_mw) / mv;
+		mw = min_power(pdo, matching_snk_pdo);
+		ma = 1000 * mw / mv;
 	} else {
-		ma = min(pdo_max_current(pdo),
-			 1000 * port->max_snk_mw / mv);
+		ma = min_current(pdo, matching_snk_pdo);
+		mw = ma * mv / 1000;
 	}
-	ma = min(ma, port->max_snk_ma);
 
 	flags = RDO_USB_COMM | RDO_NO_SUSPEND;
 
 	/* Set mismatch bit if offered power is less than operating power */
-	mw = ma * mv / 1000;
 	max_ma = ma;
 	max_mw = mw;
 	if (mw < port->operating_snk_mw) {
 		flags |= RDO_CAP_MISMATCH;
-		max_mw = port->operating_snk_mw;
-		max_ma = max_mw * 1000 / mv;
+		if (type == PDO_TYPE_BATT &&
+		    (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
+			max_mw = pdo_max_power(matching_snk_pdo);
+		else if (pdo_max_current(matching_snk_pdo) >
+			 pdo_max_current(pdo))
+			max_ma = pdo_max_current(matching_snk_pdo);
 	}
 
 	tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
@@ -1853,16 +1910,16 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
 		 port->polarity);
 
 	if (type == PDO_TYPE_BATT) {
-		*rdo = RDO_BATT(index + 1, mw, max_mw, flags);
+		*rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
 
 		tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
-			 index, mv, mw,
+			 src_pdo_index, mv, mw,
 			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
 	} else {
-		*rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
+		*rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
 
 		tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
-			 index, mv, ma,
+			 src_pdo_index, mv, ma,
 			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
 	}
 
@@ -3593,6 +3650,19 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
 }
 EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
 
+static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
+			enum pd_pdo_type type)
+{
+	int count = 0;
+	int i;
+
+	for (i = 0; i < nr_pdo; i++) {
+		if (pdo_type(pdo[i]) == type)
+			count++;
+	}
+	return count;
+}
+
 struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 {
 	struct tcpm_port *port;
@@ -3638,6 +3708,15 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 					  tcpc->config->nr_src_pdo);
 	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo,
 					  tcpc->config->nr_snk_pdo);
+	port->nr_fixed =  nr_type_pdos(port->snk_pdo,
+				       port->nr_snk_pdo,
+				       PDO_TYPE_FIXED);
+	port->nr_var = nr_type_pdos(port->snk_pdo,
+				    port->nr_snk_pdo,
+				    PDO_TYPE_VAR);
+	port->nr_batt = nr_type_pdos(port->snk_pdo,
+				     port->nr_snk_pdo,
+				     PDO_TYPE_BATT);
 	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config->snk_vdo,
 					  tcpc->config->nr_snk_vdo);
 
-- 
2.15.0.448.gf294e3d99a-goog

RE: [PATCH 2/2 v7] typec: tcpm: Only request matching pdos

[Adam Thomson]

On 16 November 2017 01:02, Badhri Jagan Sridharan wrote:

> At present, TCPM code assumes that local device supports
> variable/batt pdos and always selects the pdo with highest
> possible power within the board limit. This assumption
> might not hold good for all devices. To overcome this,
> this patch makes TCPM only accept a source_pdo when there is
> a matching sink pdo.
> 
> For Fixed pdos: The voltage should match between the
> incoming source_cap and the registered snk_pdo
> For Variable/Batt pdos: The incoming source_cap voltage
> range should fall within the registered snk_pdo's voltage
> range.
> 
> Also, when the cap_mismatch bit is set, the max_power/current
> should be set to the max_current/power of the sink_pdo.
> This is according to:
> 
> "If the Capability Mismatch bit is set to one
> The Maximum Operating Current/Power field may contain a value
> larger than the maximum current/power offered in the Source
> Capabilities message’s PDO as referenced by the Object position field.
> This enables the Sink to indicate that it requires more current/power
> than is being offered. If the Sink requires a different voltage this
> will be indicated by its Sink Capabilities message.

Hi Badhri,

I have some queries on this change. At the moment the src/snk PDOs are hard
coded in the relevant PD controller driver (e.g. fusb302.c), and the only way
to update these is to call the tcpm_update_source_capabilities() or
tcpm_update_sink_capabilities() functions. However there are no real world
examples in the kernel where this happens. Where are these functions likely to
be called from, as wherever that is it will need a reference to the port?
Actually should we be adding DT bindings to set supported src/snk PDOs from FW,
if you're taking this approach to PDO selection?

This patch also seemingly leaves 'max_snk_mv', 'max_snk_ma' and 'max_snk_mv'
redundant, although those values can be configured from a PD controller driver
(e.g. fusb302 actually supports DT bindings which allow these to be set through
FW). Now these DT bindings are basically made redundant by your change as they
have no impact. Are we expecting these to be used again in the future, or should
these be removed? FYI, as part of my PPS patch set I have been using them as
part of the PPS APDO selection criteria, based on TCPM code prior to your
modifications, as for PPS we're interested in a wide range of voltages and
currents but want to stay within the platforms limits.

> 
> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> ---
> Changelog since v1:
> - Rebased the patch on top of drivers/usb/type/tcpm.c
> - Added duplicate pdo check for variable/batt pdo.
> - Fixed tabs as suggested by dan.carpenter@oracle.com
> 
> Changelog since v2:
> - Rebase
> 
> Changelog since v3:
> - Refactored code fixed formatting issues as suggested
>   by heikki.krogerus@linux.intel.com
> 
> Changelog since v4:
> - Rebase
> 
> Changelog since v5:
> - handling the sink pdo index as pointer argument in tcpm_pd_select_pdo
> - ACK by heikki.krogerus@linux.intel.com
> 
> Changelog since v6:
> - Added Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
>  drivers/usb/typec/tcpm.c | 163 +++++++++++++++++++++++++++++++++++-----------
> -
>  1 file changed, 121 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 8b637a4b474b..f4d563ee7690 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -252,6 +252,9 @@ struct tcpm_port {
>  	unsigned int nr_src_pdo;
>  	u32 snk_pdo[PDO_MAX_OBJECTS];
>  	unsigned int nr_snk_pdo;
> +	unsigned int nr_fixed; /* number of fixed sink PDOs */
> +	unsigned int nr_var; /* number of variable sink PDOs */
> +	unsigned int nr_batt; /* number of battery sink PDOs */
>  	u32 snk_vdo[VDO_MAX_OBJECTS];
>  	unsigned int nr_snk_vdo;
> 
> @@ -1767,39 +1770,90 @@ static int tcpm_pd_check_request(struct tcpm_port
> *port)
>  	return 0;
>  }
> 
> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
> +#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
> +#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
> +
> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
> +			      int *src_pdo)
>  {
> -	unsigned int i, max_mw = 0, max_mv = 0;
> +	unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
>  	int ret = -EINVAL;
> 
>  	/*
> -	 * Select the source PDO providing the most power while staying within
> -	 * the board's voltage limits. Prefer PDO providing exp
> +	 * Select the source PDO providing the most power which has a
> +	 * matchig sink cap.
>  	 */
>  	for (i = 0; i < port->nr_source_caps; i++) {
>  		u32 pdo = port->source_caps[i];
>  		enum pd_pdo_type type = pdo_type(pdo);
> -		unsigned int mv, ma, mw;
> 
> -		if (type == PDO_TYPE_FIXED)
> -			mv = pdo_fixed_voltage(pdo);
> -		else
> -			mv = pdo_min_voltage(pdo);
> -
> -		if (type == PDO_TYPE_BATT) {
> -			mw = pdo_max_power(pdo);
> -		} else {
> -			ma = min(pdo_max_current(pdo),
> -				 port->max_snk_ma);
> -			mw = ma * mv / 1000;
> -		}
> -
> -		/* Perfer higher voltages if available */
> -		if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
> -		    mv <= port->max_snk_mv) {
> -			ret = i;
> -			max_mw = mw;
> -			max_mv = mv;
> +		if (type == PDO_TYPE_FIXED) {
> +			for (j = 0; j < port->nr_fixed; j++) {
> +				if (pdo_fixed_voltage(pdo) ==
> +				    pdo_fixed_voltage(port->snk_pdo[j])) {
> +					ma = min_current(pdo, port->snk_pdo[j]);
> +					mv = pdo_fixed_voltage(pdo);
> +					mw = ma * mv / 1000;
> +					if (mw > max_mw ||
> +					    (mw == max_mw && mv > max_mv)) {
> +						ret = 0;
> +						*src_pdo = i;
> +						*sink_pdo = j;
> +						max_mw = mw;
> +						max_mv = mv;
> +					}
> +					/* There could only be one fixed pdo
> +					 * at a specific voltage level.
> +					 * So breaking here.
> +					 */
> +					break;
> +				}
> +			}
> +		} else if (type == PDO_TYPE_BATT) {
> +			for (j = port->nr_fixed;
> +			     j < port->nr_fixed +
> +				 port->nr_batt;
> +			     j++) {
> +				if (pdo_min_voltage(pdo) >=
> +				     pdo_min_voltage(port->snk_pdo[j]) &&
> +				     pdo_max_voltage(pdo) <=
> +				     pdo_max_voltage(port->snk_pdo[j])) {
> +					mw = min_power(pdo, port->snk_pdo[j]);
> +					mv = pdo_min_voltage(pdo);
> +					if (mw > max_mw ||
> +					    (mw == max_mw && mv > max_mv)) {
> +						ret = 0;
> +						*src_pdo = i;
> +						*sink_pdo = j;
> +						max_mw = mw;
> +						max_mv = mv;
> +					}
> +				}
> +			}
> +		} else if (type == PDO_TYPE_VAR) {
> +			for (j = port->nr_fixed +
> +				 port->nr_batt;
> +			     j < port->nr_fixed +
> +				 port->nr_batt +
> +				 port->nr_var;
> +			     j++) {
> +				if (pdo_min_voltage(pdo) >=
> +				     pdo_min_voltage(port->snk_pdo[j]) &&
> +				     pdo_max_voltage(pdo) <=
> +				     pdo_max_voltage(port->snk_pdo[j])) {
> +					ma = min_current(pdo, port->snk_pdo[j]);
> +					mv = pdo_min_voltage(pdo);
> +					mw = ma * mv / 1000;
> +					if (mw > max_mw ||
> +					    (mw == max_mw && mv > max_mv)) {
> +						ret = 0;
> +						*src_pdo = i;
> +						*sink_pdo = j;
> +						max_mw = mw;
> +						max_mv = mv;
> +					}
> +				}
> +			}
>  		}
>  	}
> 
> @@ -1811,13 +1865,14 @@ static int tcpm_pd_build_request(struct tcpm_port
> *port, u32 *rdo)
>  	unsigned int mv, ma, mw, flags;
>  	unsigned int max_ma, max_mw;
>  	enum pd_pdo_type type;
> -	int index;
> -	u32 pdo;
> +	int src_pdo_index, snk_pdo_index;
> +	u32 pdo, matching_snk_pdo;
> 
> -	index = tcpm_pd_select_pdo(port);
> -	if (index < 0)
> +	if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
>  		return -EINVAL;
> -	pdo = port->source_caps[index];
> +
> +	pdo = port->source_caps[src_pdo_index];
> +	matching_snk_pdo = port->snk_pdo[snk_pdo_index];
>  	type = pdo_type(pdo);
> 
>  	if (type == PDO_TYPE_FIXED)
> @@ -1825,26 +1880,28 @@ static int tcpm_pd_build_request(struct tcpm_port
> *port, u32 *rdo)
>  	else
>  		mv = pdo_min_voltage(pdo);
> 
> -	/* Select maximum available current within the board's power limit */
> +	/* Select maximum available current within the sink pdo's limit */
>  	if (type == PDO_TYPE_BATT) {
> -		mw = pdo_max_power(pdo);
> -		ma = 1000 * min(mw, port->max_snk_mw) / mv;
> +		mw = min_power(pdo, matching_snk_pdo);
> +		ma = 1000 * mw / mv;
>  	} else {
> -		ma = min(pdo_max_current(pdo),
> -			 1000 * port->max_snk_mw / mv);
> +		ma = min_current(pdo, matching_snk_pdo);
> +		mw = ma * mv / 1000;
>  	}
> -	ma = min(ma, port->max_snk_ma);
> 
>  	flags = RDO_USB_COMM | RDO_NO_SUSPEND;
> 
>  	/* Set mismatch bit if offered power is less than operating power */
> -	mw = ma * mv / 1000;
>  	max_ma = ma;
>  	max_mw = mw;
>  	if (mw < port->operating_snk_mw) {
>  		flags |= RDO_CAP_MISMATCH;
> -		max_mw = port->operating_snk_mw;
> -		max_ma = max_mw * 1000 / mv;
> +		if (type == PDO_TYPE_BATT &&
> +		    (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
> +			max_mw = pdo_max_power(matching_snk_pdo);
> +		else if (pdo_max_current(matching_snk_pdo) >
> +			 pdo_max_current(pdo))
> +			max_ma = pdo_max_current(matching_snk_pdo);
>  	}
> 
>  	tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
> @@ -1853,16 +1910,16 @@ static int tcpm_pd_build_request(struct tcpm_port
> *port, u32 *rdo)
>  		 port->polarity);
> 
>  	if (type == PDO_TYPE_BATT) {
> -		*rdo = RDO_BATT(index + 1, mw, max_mw, flags);
> +		*rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
> 
>  		tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
> -			 index, mv, mw,
> +			 src_pdo_index, mv, mw,
>  			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>  	} else {
> -		*rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
> +		*rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
> 
>  		tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
> -			 index, mv, ma,
> +			 src_pdo_index, mv, ma,
>  			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>  	}
> 
> @@ -3593,6 +3650,19 @@ int tcpm_update_sink_capabilities(struct tcpm_port
> *port, const u32 *pdo,
>  }
>  EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
> 
> +static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
> +			enum pd_pdo_type type)
> +{
> +	int count = 0;
> +	int i;
> +
> +	for (i = 0; i < nr_pdo; i++) {
> +		if (pdo_type(pdo[i]) == type)
> +			count++;
> +	}
> +	return count;
> +}
> +
>  struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  {
>  	struct tcpm_port *port;
> @@ -3638,6 +3708,15 @@ struct tcpm_port *tcpm_register_port(struct device
> *dev, struct tcpc_dev *tcpc)
>  					  tcpc->config->nr_src_pdo);
>  	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config-
> >snk_pdo,
>  					  tcpc->config->nr_snk_pdo);
> +	port->nr_fixed =  nr_type_pdos(port->snk_pdo,
> +				       port->nr_snk_pdo,
> +				       PDO_TYPE_FIXED);
> +	port->nr_var = nr_type_pdos(port->snk_pdo,
> +				    port->nr_snk_pdo,
> +				    PDO_TYPE_VAR);
> +	port->nr_batt = nr_type_pdos(port->snk_pdo,
> +				     port->nr_snk_pdo,
> +				     PDO_TYPE_BATT);
>  	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config-
> >snk_vdo,
>  					  tcpc->config->nr_snk_vdo);
> 
> --
> 2.15.0.448.gf294e3d99a-goog
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v7] typec: tcpm: Only request matching pdos

[Badhri Jagan Sridharan]

On Thu, Nov 23, 2017 at 3:10 AM, Adam Thomson
<Adam.Thomson.Opensource@diasemi.com> wrote:
> On 16 November 2017 01:02, Badhri Jagan Sridharan wrote:
>
>> At present, TCPM code assumes that local device supports
>> variable/batt pdos and always selects the pdo with highest
>> possible power within the board limit. This assumption
>> might not hold good for all devices. To overcome this,
>> this patch makes TCPM only accept a source_pdo when there is
>> a matching sink pdo.
>>
>> For Fixed pdos: The voltage should match between the
>> incoming source_cap and the registered snk_pdo
>> For Variable/Batt pdos: The incoming source_cap voltage
>> range should fall within the registered snk_pdo's voltage
>> range.
>>
>> Also, when the cap_mismatch bit is set, the max_power/current
>> should be set to the max_current/power of the sink_pdo.
>> This is according to:
>>
>> "If the Capability Mismatch bit is set to one
>> The Maximum Operating Current/Power field may contain a value
>> larger than the maximum current/power offered in the Source
>> Capabilities message’s PDO as referenced by the Object position field.
>> This enables the Sink to indicate that it requires more current/power
>> than is being offered. If the Sink requires a different voltage this
>> will be indicated by its Sink Capabilities message.
>
> Hi Badhri,
>
> I have some queries on this change. At the moment the src/snk PDOs are hard
> coded in the relevant PD controller driver (e.g. fusb302.c), and the only way
> to update these is to call the tcpm_update_source_capabilities() or
> tcpm_update_sink_capabilities() functions. However there are no real world

Good point ! But I dont see any code which calls this either. I believe
Guenter added this. Guenter do you know ?

> examples in the kernel where this happens. Where are these functions likely to
> be called from, as wherever that is it will need a reference to the port?
> Actually should we be adding DT bindings to set supported src/snk PDOs from FW,
> if you're taking this approach to PDO selection?
>
> This patch also seemingly leaves 'max_snk_mv', 'max_snk_ma' and 'max_snk_mv'
> redundant, although those values can be configured from a PD controller driver
> (e.g. fusb302 actually supports DT bindings which allow these to be set through
> FW). Now these DT bindings are basically made redundant by your change as they
> have no impact. Are we expecting these to be used again in the future, or should

Yes, I think  'max_snk_mv', 'max_snk_ma' and 'max_snk_mw' etc should be removed.
The problem here is that maintaining these values implies that tcpm is
not going to
request pdo based on the sink_caps that are published. All these
values can be derived from
the sink_pdo objects that were declared, hence, they are redundant,
I will update the patch to remove this.

> these be removed? FYI, as part of my PPS patch set I have been using them as
> part of the PPS APDO selection criteria, based on TCPM code prior to your
> modifications, as for PPS we're interested in a wide range of voltages and
> currents but want to stay within the platforms limits.

Arent you defining a new PDO type similar to PDO_FIXED, PDO_VARIABLE etc ?
If so values such as " 'max_snk_mv', 'max_snk_ma' and
'max_snk_mw' " should be part of the APDO object right ? So why would
you still need
 'max_snk_mv', 'max_snk_ma' and 'max_snk_mw'  ?

>
>>
>> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
>> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>> ---
>> Changelog since v1:
>> - Rebased the patch on top of drivers/usb/type/tcpm.c
>> - Added duplicate pdo check for variable/batt pdo.
>> - Fixed tabs as suggested by dan.carpenter@oracle.com
>>
>> Changelog since v2:
>> - Rebase
>>
>> Changelog since v3:
>> - Refactored code fixed formatting issues as suggested
>>   by heikki.krogerus@linux.intel.com
>>
>> Changelog since v4:
>> - Rebase
>>
>> Changelog since v5:
>> - handling the sink pdo index as pointer argument in tcpm_pd_select_pdo
>> - ACK by heikki.krogerus@linux.intel.com
>>
>> Changelog since v6:
>> - Added Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>>  drivers/usb/typec/tcpm.c | 163 +++++++++++++++++++++++++++++++++++-----------
>> -
>>  1 file changed, 121 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>> index 8b637a4b474b..f4d563ee7690 100644
>> --- a/drivers/usb/typec/tcpm.c
>> +++ b/drivers/usb/typec/tcpm.c
>> @@ -252,6 +252,9 @@ struct tcpm_port {
>>       unsigned int nr_src_pdo;
>>       u32 snk_pdo[PDO_MAX_OBJECTS];
>>       unsigned int nr_snk_pdo;
>> +     unsigned int nr_fixed; /* number of fixed sink PDOs */
>> +     unsigned int nr_var; /* number of variable sink PDOs */
>> +     unsigned int nr_batt; /* number of battery sink PDOs */
>>       u32 snk_vdo[VDO_MAX_OBJECTS];
>>       unsigned int nr_snk_vdo;
>>
>> @@ -1767,39 +1770,90 @@ static int tcpm_pd_check_request(struct tcpm_port
>> *port)
>>       return 0;
>>  }
>>
>> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
>> +#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
>> +#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
>> +
>> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
>> +                           int *src_pdo)
>>  {
>> -     unsigned int i, max_mw = 0, max_mv = 0;
>> +     unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
>>       int ret = -EINVAL;
>>
>>       /*
>> -      * Select the source PDO providing the most power while staying within
>> -      * the board's voltage limits. Prefer PDO providing exp
>> +      * Select the source PDO providing the most power which has a
>> +      * matchig sink cap.
>>        */
>>       for (i = 0; i < port->nr_source_caps; i++) {
>>               u32 pdo = port->source_caps[i];
>>               enum pd_pdo_type type = pdo_type(pdo);
>> -             unsigned int mv, ma, mw;
>>
>> -             if (type == PDO_TYPE_FIXED)
>> -                     mv = pdo_fixed_voltage(pdo);
>> -             else
>> -                     mv = pdo_min_voltage(pdo);
>> -
>> -             if (type == PDO_TYPE_BATT) {
>> -                     mw = pdo_max_power(pdo);
>> -             } else {
>> -                     ma = min(pdo_max_current(pdo),
>> -                              port->max_snk_ma);
>> -                     mw = ma * mv / 1000;
>> -             }
>> -
>> -             /* Perfer higher voltages if available */
>> -             if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
>> -                 mv <= port->max_snk_mv) {
>> -                     ret = i;
>> -                     max_mw = mw;
>> -                     max_mv = mv;
>> +             if (type == PDO_TYPE_FIXED) {
>> +                     for (j = 0; j < port->nr_fixed; j++) {
>> +                             if (pdo_fixed_voltage(pdo) ==
>> +                                 pdo_fixed_voltage(port->snk_pdo[j])) {
>> +                                     ma = min_current(pdo, port->snk_pdo[j]);
>> +                                     mv = pdo_fixed_voltage(pdo);
>> +                                     mw = ma * mv / 1000;
>> +                                     if (mw > max_mw ||
>> +                                         (mw == max_mw && mv > max_mv)) {
>> +                                             ret = 0;
>> +                                             *src_pdo = i;
>> +                                             *sink_pdo = j;
>> +                                             max_mw = mw;
>> +                                             max_mv = mv;
>> +                                     }
>> +                                     /* There could only be one fixed pdo
>> +                                      * at a specific voltage level.
>> +                                      * So breaking here.
>> +                                      */
>> +                                     break;
>> +                             }
>> +                     }
>> +             } else if (type == PDO_TYPE_BATT) {
>> +                     for (j = port->nr_fixed;
>> +                          j < port->nr_fixed +
>> +                              port->nr_batt;
>> +                          j++) {
>> +                             if (pdo_min_voltage(pdo) >=
>> +                                  pdo_min_voltage(port->snk_pdo[j]) &&
>> +                                  pdo_max_voltage(pdo) <=
>> +                                  pdo_max_voltage(port->snk_pdo[j])) {
>> +                                     mw = min_power(pdo, port->snk_pdo[j]);
>> +                                     mv = pdo_min_voltage(pdo);
>> +                                     if (mw > max_mw ||
>> +                                         (mw == max_mw && mv > max_mv)) {
>> +                                             ret = 0;
>> +                                             *src_pdo = i;
>> +                                             *sink_pdo = j;
>> +                                             max_mw = mw;
>> +                                             max_mv = mv;
>> +                                     }
>> +                             }
>> +                     }
>> +             } else if (type == PDO_TYPE_VAR) {
>> +                     for (j = port->nr_fixed +
>> +                              port->nr_batt;
>> +                          j < port->nr_fixed +
>> +                              port->nr_batt +
>> +                              port->nr_var;
>> +                          j++) {
>> +                             if (pdo_min_voltage(pdo) >=
>> +                                  pdo_min_voltage(port->snk_pdo[j]) &&
>> +                                  pdo_max_voltage(pdo) <=
>> +                                  pdo_max_voltage(port->snk_pdo[j])) {
>> +                                     ma = min_current(pdo, port->snk_pdo[j]);
>> +                                     mv = pdo_min_voltage(pdo);
>> +                                     mw = ma * mv / 1000;
>> +                                     if (mw > max_mw ||
>> +                                         (mw == max_mw && mv > max_mv)) {
>> +                                             ret = 0;
>> +                                             *src_pdo = i;
>> +                                             *sink_pdo = j;
>> +                                             max_mw = mw;
>> +                                             max_mv = mv;
>> +                                     }
>> +                             }
>> +                     }
>>               }
>>       }
>>
>> @@ -1811,13 +1865,14 @@ static int tcpm_pd_build_request(struct tcpm_port
>> *port, u32 *rdo)
>>       unsigned int mv, ma, mw, flags;
>>       unsigned int max_ma, max_mw;
>>       enum pd_pdo_type type;
>> -     int index;
>> -     u32 pdo;
>> +     int src_pdo_index, snk_pdo_index;
>> +     u32 pdo, matching_snk_pdo;
>>
>> -     index = tcpm_pd_select_pdo(port);
>> -     if (index < 0)
>> +     if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
>>               return -EINVAL;
>> -     pdo = port->source_caps[index];
>> +
>> +     pdo = port->source_caps[src_pdo_index];
>> +     matching_snk_pdo = port->snk_pdo[snk_pdo_index];
>>       type = pdo_type(pdo);
>>
>>       if (type == PDO_TYPE_FIXED)
>> @@ -1825,26 +1880,28 @@ static int tcpm_pd_build_request(struct tcpm_port
>> *port, u32 *rdo)
>>       else
>>               mv = pdo_min_voltage(pdo);
>>
>> -     /* Select maximum available current within the board's power limit */
>> +     /* Select maximum available current within the sink pdo's limit */
>>       if (type == PDO_TYPE_BATT) {
>> -             mw = pdo_max_power(pdo);
>> -             ma = 1000 * min(mw, port->max_snk_mw) / mv;
>> +             mw = min_power(pdo, matching_snk_pdo);
>> +             ma = 1000 * mw / mv;
>>       } else {
>> -             ma = min(pdo_max_current(pdo),
>> -                      1000 * port->max_snk_mw / mv);
>> +             ma = min_current(pdo, matching_snk_pdo);
>> +             mw = ma * mv / 1000;
>>       }
>> -     ma = min(ma, port->max_snk_ma);
>>
>>       flags = RDO_USB_COMM | RDO_NO_SUSPEND;
>>
>>       /* Set mismatch bit if offered power is less than operating power */
>> -     mw = ma * mv / 1000;
>>       max_ma = ma;
>>       max_mw = mw;
>>       if (mw < port->operating_snk_mw) {
>>               flags |= RDO_CAP_MISMATCH;
>> -             max_mw = port->operating_snk_mw;
>> -             max_ma = max_mw * 1000 / mv;
>> +             if (type == PDO_TYPE_BATT &&
>> +                 (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
>> +                     max_mw = pdo_max_power(matching_snk_pdo);
>> +             else if (pdo_max_current(matching_snk_pdo) >
>> +                      pdo_max_current(pdo))
>> +                     max_ma = pdo_max_current(matching_snk_pdo);
>>       }
>>
>>       tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
>> @@ -1853,16 +1910,16 @@ static int tcpm_pd_build_request(struct tcpm_port
>> *port, u32 *rdo)
>>                port->polarity);
>>
>>       if (type == PDO_TYPE_BATT) {
>> -             *rdo = RDO_BATT(index + 1, mw, max_mw, flags);
>> +             *rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
>>
>>               tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
>> -                      index, mv, mw,
>> +                      src_pdo_index, mv, mw,
>>                        flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>>       } else {
>> -             *rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
>> +             *rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
>>
>>               tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
>> -                      index, mv, ma,
>> +                      src_pdo_index, mv, ma,
>>                        flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>>       }
>>
>> @@ -3593,6 +3650,19 @@ int tcpm_update_sink_capabilities(struct tcpm_port
>> *port, const u32 *pdo,
>>  }
>>  EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>>
>> +static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
>> +                     enum pd_pdo_type type)
>> +{
>> +     int count = 0;
>> +     int i;
>> +
>> +     for (i = 0; i < nr_pdo; i++) {
>> +             if (pdo_type(pdo[i]) == type)
>> +                     count++;
>> +     }
>> +     return count;
>> +}
>> +
>>  struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>>  {
>>       struct tcpm_port *port;
>> @@ -3638,6 +3708,15 @@ struct tcpm_port *tcpm_register_port(struct device
>> *dev, struct tcpc_dev *tcpc)
>>                                         tcpc->config->nr_src_pdo);
>>       port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config-
>> >snk_pdo,
>>                                         tcpc->config->nr_snk_pdo);
>> +     port->nr_fixed =  nr_type_pdos(port->snk_pdo,
>> +                                    port->nr_snk_pdo,
>> +                                    PDO_TYPE_FIXED);
>> +     port->nr_var = nr_type_pdos(port->snk_pdo,
>> +                                 port->nr_snk_pdo,
>> +                                 PDO_TYPE_VAR);
>> +     port->nr_batt = nr_type_pdos(port->snk_pdo,
>> +                                  port->nr_snk_pdo,
>> +                                  PDO_TYPE_BATT);
>>       port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config-
>> >snk_vdo,
>>                                         tcpc->config->nr_snk_vdo);
>>
>> --
>> 2.15.0.448.gf294e3d99a-goog
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v7] typec: tcpm: Only request matching pdos

[Guenter Roeck]

On 11/27/2017 05:38 PM, Badhri Jagan Sridharan wrote:
> On Thu, Nov 23, 2017 at 3:10 AM, Adam Thomson
> <Adam.Thomson.Opensource@diasemi.com> wrote:
>> On 16 November 2017 01:02, Badhri Jagan Sridharan wrote:
>>
>>> At present, TCPM code assumes that local device supports
>>> variable/batt pdos and always selects the pdo with highest
>>> possible power within the board limit. This assumption
>>> might not hold good for all devices. To overcome this,
>>> this patch makes TCPM only accept a source_pdo when there is
>>> a matching sink pdo.
>>>
>>> For Fixed pdos: The voltage should match between the
>>> incoming source_cap and the registered snk_pdo
>>> For Variable/Batt pdos: The incoming source_cap voltage
>>> range should fall within the registered snk_pdo's voltage
>>> range.
>>>
>>> Also, when the cap_mismatch bit is set, the max_power/current
>>> should be set to the max_current/power of the sink_pdo.
>>> This is according to:
>>>
>>> "If the Capability Mismatch bit is set to one
>>> The Maximum Operating Current/Power field may contain a value
>>> larger than the maximum current/power offered in the Source
>>> Capabilities message’s PDO as referenced by the Object position field.
>>> This enables the Sink to indicate that it requires more current/power
>>> than is being offered. If the Sink requires a different voltage this
>>> will be indicated by its Sink Capabilities message.
>>
>> Hi Badhri,
>>
>> I have some queries on this change. At the moment the src/snk PDOs are hard
>> coded in the relevant PD controller driver (e.g. fusb302.c), and the only way
>> to update these is to call the tcpm_update_source_capabilities() or
>> tcpm_update_sink_capabilities() functions. However there are no real world
> 
> Good point ! But I dont see any code which calls this either. I believe
> Guenter added this. Guenter do you know ?
> 

A source might change its power capabilities dynamically. Practical example is
a system with two USB-C ports. If one is in use, it may support 3A output power.
If two are in use, it may support 1.5A each. This isn't currently used in the
kernel, but some of our devices have this behavior, so I thought it would be
prudent to have support for it. The same is true for a sink, which may change
its power requirements at runtime (same situation - a device is inserted at
the second USB port) and ask for more or less power.

Is that a problem ? I won't object if you want to take it out because there is
no current user, but you should keep in mind that those are valid use cases,
and that the code should at least be extensible (ie the patch taking it away
should be revertible) to support those use cases.

Guenter

>> examples in the kernel where this happens. Where are these functions likely to
>> be called from, as wherever that is it will need a reference to the port?
>> Actually should we be adding DT bindings to set supported src/snk PDOs from FW,
>> if you're taking this approach to PDO selection?
>>
>> This patch also seemingly leaves 'max_snk_mv', 'max_snk_ma' and 'max_snk_mv'
>> redundant, although those values can be configured from a PD controller driver
>> (e.g. fusb302 actually supports DT bindings which allow these to be set through
>> FW). Now these DT bindings are basically made redundant by your change as they
>> have no impact. Are we expecting these to be used again in the future, or should
> 
> Yes, I think  'max_snk_mv', 'max_snk_ma' and 'max_snk_mw' etc should be removed.
> The problem here is that maintaining these values implies that tcpm is
> not going to
> request pdo based on the sink_caps that are published. All these
> values can be derived from
> the sink_pdo objects that were declared, hence, they are redundant,
> I will update the patch to remove this.
> 
>> these be removed? FYI, as part of my PPS patch set I have been using them as
>> part of the PPS APDO selection criteria, based on TCPM code prior to your
>> modifications, as for PPS we're interested in a wide range of voltages and
>> currents but want to stay within the platforms limits.
> 
> Arent you defining a new PDO type similar to PDO_FIXED, PDO_VARIABLE etc ?
> If so values such as " 'max_snk_mv', 'max_snk_ma' and
> 'max_snk_mw' " should be part of the APDO object right ? So why would
> you still need
>   'max_snk_mv', 'max_snk_ma' and 'max_snk_mw'  ?
> 
>>
>>>
>>> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
>>> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>>
>>> ---
>>> Changelog since v1:
>>> - Rebased the patch on top of drivers/usb/type/tcpm.c
>>> - Added duplicate pdo check for variable/batt pdo.
>>> - Fixed tabs as suggested by dan.carpenter@oracle.com
>>>
>>> Changelog since v2:
>>> - Rebase
>>>
>>> Changelog since v3:
>>> - Refactored code fixed formatting issues as suggested
>>>    by heikki.krogerus@linux.intel.com
>>>
>>> Changelog since v4:
>>> - Rebase
>>>
>>> Changelog since v5:
>>> - handling the sink pdo index as pointer argument in tcpm_pd_select_pdo
>>> - ACK by heikki.krogerus@linux.intel.com
>>>
>>> Changelog since v6:
>>> - Added Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>>
>>>   drivers/usb/typec/tcpm.c | 163 +++++++++++++++++++++++++++++++++++-----------
>>> -
>>>   1 file changed, 121 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>>> index 8b637a4b474b..f4d563ee7690 100644
>>> --- a/drivers/usb/typec/tcpm.c
>>> +++ b/drivers/usb/typec/tcpm.c
>>> @@ -252,6 +252,9 @@ struct tcpm_port {
>>>        unsigned int nr_src_pdo;
>>>        u32 snk_pdo[PDO_MAX_OBJECTS];
>>>        unsigned int nr_snk_pdo;
>>> +     unsigned int nr_fixed; /* number of fixed sink PDOs */
>>> +     unsigned int nr_var; /* number of variable sink PDOs */
>>> +     unsigned int nr_batt; /* number of battery sink PDOs */
>>>        u32 snk_vdo[VDO_MAX_OBJECTS];
>>>        unsigned int nr_snk_vdo;
>>>
>>> @@ -1767,39 +1770,90 @@ static int tcpm_pd_check_request(struct tcpm_port
>>> *port)
>>>        return 0;
>>>   }
>>>
>>> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
>>> +#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
>>> +#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
>>> +
>>> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
>>> +                           int *src_pdo)
>>>   {
>>> -     unsigned int i, max_mw = 0, max_mv = 0;
>>> +     unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
>>>        int ret = -EINVAL;
>>>
>>>        /*
>>> -      * Select the source PDO providing the most power while staying within
>>> -      * the board's voltage limits. Prefer PDO providing exp
>>> +      * Select the source PDO providing the most power which has a
>>> +      * matchig sink cap.
>>>         */
>>>        for (i = 0; i < port->nr_source_caps; i++) {
>>>                u32 pdo = port->source_caps[i];
>>>                enum pd_pdo_type type = pdo_type(pdo);
>>> -             unsigned int mv, ma, mw;
>>>
>>> -             if (type == PDO_TYPE_FIXED)
>>> -                     mv = pdo_fixed_voltage(pdo);
>>> -             else
>>> -                     mv = pdo_min_voltage(pdo);
>>> -
>>> -             if (type == PDO_TYPE_BATT) {
>>> -                     mw = pdo_max_power(pdo);
>>> -             } else {
>>> -                     ma = min(pdo_max_current(pdo),
>>> -                              port->max_snk_ma);
>>> -                     mw = ma * mv / 1000;
>>> -             }
>>> -
>>> -             /* Perfer higher voltages if available */
>>> -             if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
>>> -                 mv <= port->max_snk_mv) {
>>> -                     ret = i;
>>> -                     max_mw = mw;
>>> -                     max_mv = mv;
>>> +             if (type == PDO_TYPE_FIXED) {
>>> +                     for (j = 0; j < port->nr_fixed; j++) {
>>> +                             if (pdo_fixed_voltage(pdo) ==
>>> +                                 pdo_fixed_voltage(port->snk_pdo[j])) {
>>> +                                     ma = min_current(pdo, port->snk_pdo[j]);
>>> +                                     mv = pdo_fixed_voltage(pdo);
>>> +                                     mw = ma * mv / 1000;
>>> +                                     if (mw > max_mw ||
>>> +                                         (mw == max_mw && mv > max_mv)) {
>>> +                                             ret = 0;
>>> +                                             *src_pdo = i;
>>> +                                             *sink_pdo = j;
>>> +                                             max_mw = mw;
>>> +                                             max_mv = mv;
>>> +                                     }
>>> +                                     /* There could only be one fixed pdo
>>> +                                      * at a specific voltage level.
>>> +                                      * So breaking here.
>>> +                                      */
>>> +                                     break;
>>> +                             }
>>> +                     }
>>> +             } else if (type == PDO_TYPE_BATT) {
>>> +                     for (j = port->nr_fixed;
>>> +                          j < port->nr_fixed +
>>> +                              port->nr_batt;
>>> +                          j++) {
>>> +                             if (pdo_min_voltage(pdo) >=
>>> +                                  pdo_min_voltage(port->snk_pdo[j]) &&
>>> +                                  pdo_max_voltage(pdo) <=
>>> +                                  pdo_max_voltage(port->snk_pdo[j])) {
>>> +                                     mw = min_power(pdo, port->snk_pdo[j]);
>>> +                                     mv = pdo_min_voltage(pdo);
>>> +                                     if (mw > max_mw ||
>>> +                                         (mw == max_mw && mv > max_mv)) {
>>> +                                             ret = 0;
>>> +                                             *src_pdo = i;
>>> +                                             *sink_pdo = j;
>>> +                                             max_mw = mw;
>>> +                                             max_mv = mv;
>>> +                                     }
>>> +                             }
>>> +                     }
>>> +             } else if (type == PDO_TYPE_VAR) {
>>> +                     for (j = port->nr_fixed +
>>> +                              port->nr_batt;
>>> +                          j < port->nr_fixed +
>>> +                              port->nr_batt +
>>> +                              port->nr_var;
>>> +                          j++) {
>>> +                             if (pdo_min_voltage(pdo) >=
>>> +                                  pdo_min_voltage(port->snk_pdo[j]) &&
>>> +                                  pdo_max_voltage(pdo) <=
>>> +                                  pdo_max_voltage(port->snk_pdo[j])) {
>>> +                                     ma = min_current(pdo, port->snk_pdo[j]);
>>> +                                     mv = pdo_min_voltage(pdo);
>>> +                                     mw = ma * mv / 1000;
>>> +                                     if (mw > max_mw ||
>>> +                                         (mw == max_mw && mv > max_mv)) {
>>> +                                             ret = 0;
>>> +                                             *src_pdo = i;
>>> +                                             *sink_pdo = j;
>>> +                                             max_mw = mw;
>>> +                                             max_mv = mv;
>>> +                                     }
>>> +                             }
>>> +                     }
>>>                }
>>>        }
>>>
>>> @@ -1811,13 +1865,14 @@ static int tcpm_pd_build_request(struct tcpm_port
>>> *port, u32 *rdo)
>>>        unsigned int mv, ma, mw, flags;
>>>        unsigned int max_ma, max_mw;
>>>        enum pd_pdo_type type;
>>> -     int index;
>>> -     u32 pdo;
>>> +     int src_pdo_index, snk_pdo_index;
>>> +     u32 pdo, matching_snk_pdo;
>>>
>>> -     index = tcpm_pd_select_pdo(port);
>>> -     if (index < 0)
>>> +     if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
>>>                return -EINVAL;
>>> -     pdo = port->source_caps[index];
>>> +
>>> +     pdo = port->source_caps[src_pdo_index];
>>> +     matching_snk_pdo = port->snk_pdo[snk_pdo_index];
>>>        type = pdo_type(pdo);
>>>
>>>        if (type == PDO_TYPE_FIXED)
>>> @@ -1825,26 +1880,28 @@ static int tcpm_pd_build_request(struct tcpm_port
>>> *port, u32 *rdo)
>>>        else
>>>                mv = pdo_min_voltage(pdo);
>>>
>>> -     /* Select maximum available current within the board's power limit */
>>> +     /* Select maximum available current within the sink pdo's limit */
>>>        if (type == PDO_TYPE_BATT) {
>>> -             mw = pdo_max_power(pdo);
>>> -             ma = 1000 * min(mw, port->max_snk_mw) / mv;
>>> +             mw = min_power(pdo, matching_snk_pdo);
>>> +             ma = 1000 * mw / mv;
>>>        } else {
>>> -             ma = min(pdo_max_current(pdo),
>>> -                      1000 * port->max_snk_mw / mv);
>>> +             ma = min_current(pdo, matching_snk_pdo);
>>> +             mw = ma * mv / 1000;
>>>        }
>>> -     ma = min(ma, port->max_snk_ma);
>>>
>>>        flags = RDO_USB_COMM | RDO_NO_SUSPEND;
>>>
>>>        /* Set mismatch bit if offered power is less than operating power */
>>> -     mw = ma * mv / 1000;
>>>        max_ma = ma;
>>>        max_mw = mw;
>>>        if (mw < port->operating_snk_mw) {
>>>                flags |= RDO_CAP_MISMATCH;
>>> -             max_mw = port->operating_snk_mw;
>>> -             max_ma = max_mw * 1000 / mv;
>>> +             if (type == PDO_TYPE_BATT &&
>>> +                 (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
>>> +                     max_mw = pdo_max_power(matching_snk_pdo);
>>> +             else if (pdo_max_current(matching_snk_pdo) >
>>> +                      pdo_max_current(pdo))
>>> +                     max_ma = pdo_max_current(matching_snk_pdo);
>>>        }
>>>
>>>        tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
>>> @@ -1853,16 +1910,16 @@ static int tcpm_pd_build_request(struct tcpm_port
>>> *port, u32 *rdo)
>>>                 port->polarity);
>>>
>>>        if (type == PDO_TYPE_BATT) {
>>> -             *rdo = RDO_BATT(index + 1, mw, max_mw, flags);
>>> +             *rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
>>>
>>>                tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
>>> -                      index, mv, mw,
>>> +                      src_pdo_index, mv, mw,
>>>                         flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>>>        } else {
>>> -             *rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
>>> +             *rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
>>>
>>>                tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
>>> -                      index, mv, ma,
>>> +                      src_pdo_index, mv, ma,
>>>                         flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>>>        }
>>>
>>> @@ -3593,6 +3650,19 @@ int tcpm_update_sink_capabilities(struct tcpm_port
>>> *port, const u32 *pdo,
>>>   }
>>>   EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>>>
>>> +static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
>>> +                     enum pd_pdo_type type)
>>> +{
>>> +     int count = 0;
>>> +     int i;
>>> +
>>> +     for (i = 0; i < nr_pdo; i++) {
>>> +             if (pdo_type(pdo[i]) == type)
>>> +                     count++;
>>> +     }
>>> +     return count;
>>> +}
>>> +
>>>   struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>>>   {
>>>        struct tcpm_port *port;
>>> @@ -3638,6 +3708,15 @@ struct tcpm_port *tcpm_register_port(struct device
>>> *dev, struct tcpc_dev *tcpc)
>>>                                          tcpc->config->nr_src_pdo);
>>>        port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config-
>>>> snk_pdo,
>>>                                          tcpc->config->nr_snk_pdo);
>>> +     port->nr_fixed =  nr_type_pdos(port->snk_pdo,
>>> +                                    port->nr_snk_pdo,
>>> +                                    PDO_TYPE_FIXED);
>>> +     port->nr_var = nr_type_pdos(port->snk_pdo,
>>> +                                 port->nr_snk_pdo,
>>> +                                 PDO_TYPE_VAR);
>>> +     port->nr_batt = nr_type_pdos(port->snk_pdo,
>>> +                                  port->nr_snk_pdo,
>>> +                                  PDO_TYPE_BATT);
>>>        port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config-
>>>> snk_vdo,
>>>                                          tcpc->config->nr_snk_vdo);
>>>
>>> --
>>> 2.15.0.448.gf294e3d99a-goog
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH 2/2 v7] typec: tcpm: Only request matching pdos

[Adam Thomson]

On 28 November 2017 01:38, Badhri Jagan Sridharan wrote:

> > examples in the kernel where this happens. Where are these functions likely to
> > be called from, as wherever that is it will need a reference to the port?
> > Actually should we be adding DT bindings to set supported src/snk PDOs from FW,
> > if you're taking this approach to PDO selection?
> >
> > This patch also seemingly leaves 'max_snk_mv', 'max_snk_ma' and 'max_snk_mv'
> > redundant, although those values can be configured from a PD controller driver
> > (e.g. fusb302 actually supports DT bindings which allow these to be set through
> > FW). Now these DT bindings are basically made redundant by your change as they
> > have no impact. Are we expecting these to be used again in the future, or should
> 
> Yes, I think  'max_snk_mv', 'max_snk_ma' and 'max_snk_mw' etc should be
> removed.
> The problem here is that maintaining these values implies that tcpm is
> not going to
> request pdo based on the sink_caps that are published. All these
> values can be derived from
> the sink_pdo objects that were declared, hence, they are redundant,
> I will update the patch to remove this.

I have no problem really with this approach, other than right now with your
patch there's no way to actually set the PDOs other than the 2 functions to
update source and sink capabilities. Previously you had the option, at least
through the fusb302 driver, to configure the max_snk_* values from DT, but your
patch obviously changes this behaviour. I think we need a FW based method of
configuring these at startup at least, as with your current patch the values
being used are hard coded. As this is generic for TCPM I would guess DT bindings
(and maybe equivalent ACPI properties as well I guess) would be a sensible
approach.
 
> > these be removed? FYI, as part of my PPS patch set I have been using them as
> > part of the PPS APDO selection criteria, based on TCPM code prior to your
> > modifications, as for PPS we're interested in a wide range of voltages and
> > currents but want to stay within the platforms limits.
> 
> Arent you defining a new PDO type similar to PDO_FIXED, PDO_VARIABLE etc ?
> If so values such as " 'max_snk_mv', 'max_snk_ma' and
> 'max_snk_mw' " should be part of the APDO object right ? So why would
> you still need
>  'max_snk_mv', 'max_snk_ma' and 'max_snk_mw'  ?

My initial implementation was based on the approach before your changes, and
actually for PPS this to me made sense, at least from the sink side. We are
dealing with a range of voltages and currents so the important points are the
maximum values. However, if we're now making decisions based on sink PDOs then
I can look at adapting.

RE: [PATCH 2/2 v7] typec: tcpm: Only request matching pdos

[Adam Thomson]

On 28 November 2017 02:07, Guenter Roeck wrote:

> On 11/27/2017 05:38 PM, Badhri Jagan Sridharan wrote:
> > On Thu, Nov 23, 2017 at 3:10 AM, Adam Thomson
> > <Adam.Thomson.Opensource@diasemi.com> wrote:
> >> On 16 November 2017 01:02, Badhri Jagan Sridharan wrote:
> >>
> >>> At present, TCPM code assumes that local device supports
> >>> variable/batt pdos and always selects the pdo with highest
> >>> possible power within the board limit. This assumption
> >>> might not hold good for all devices. To overcome this,
> >>> this patch makes TCPM only accept a source_pdo when there is
> >>> a matching sink pdo.
> >>>
> >>> For Fixed pdos: The voltage should match between the
> >>> incoming source_cap and the registered snk_pdo
> >>> For Variable/Batt pdos: The incoming source_cap voltage
> >>> range should fall within the registered snk_pdo's voltage
> >>> range.
> >>>
> >>> Also, when the cap_mismatch bit is set, the max_power/current
> >>> should be set to the max_current/power of the sink_pdo.
> >>> This is according to:
> >>>
> >>> "If the Capability Mismatch bit is set to one
> >>> The Maximum Operating Current/Power field may contain a value
> >>> larger than the maximum current/power offered in the Source
> >>> Capabilities message’s PDO as referenced by the Object position field.
> >>> This enables the Sink to indicate that it requires more current/power
> >>> than is being offered. If the Sink requires a different voltage this
> >>> will be indicated by its Sink Capabilities message.
> >>
> >> Hi Badhri,
> >>
> >> I have some queries on this change. At the moment the src/snk PDOs are hard
> >> coded in the relevant PD controller driver (e.g. fusb302.c), and the only way
> >> to update these is to call the tcpm_update_source_capabilities() or
> >> tcpm_update_sink_capabilities() functions. However there are no real world
> >
> > Good point ! But I dont see any code which calls this either. I believe
> > Guenter added this. Guenter do you know ?
> >
> 
> A source might change its power capabilities dynamically. Practical example is
> a system with two USB-C ports. If one is in use, it may support 3A output power.
> If two are in use, it may support 1.5A each. This isn't currently used in the
> kernel, but some of our devices have this behavior, so I thought it would be
> prudent to have support for it. The same is true for a sink, which may change
> its power requirements at runtime (same situation - a device is inserted at
> the second USB port) and ask for more or less power.
> 
> Is that a problem ? I won't object if you want to take it out because there is
> no current user, but you should keep in mind that those are valid use cases,
> and that the code should at least be extensible (ie the patch taking it away
> should be revertible) to support those use cases.

No, I have no desire to remove it and definitely see the valid use-cases. I was
curious though as to where these functions might be called from within the
kernel as wherever that is it will need to have the ability to retrieve the port
instance to be able to call these functions.