Skip to contentSkip to conversation

usb: typec: tcpm: Extend the matching rules on PPS APDO selection

PATCH In review Kyle Tso 3 participants 3 messages Dec 17, 2018 – Dec 18, 2018

Subsystems (3)
lore
Share & tools
lore

Kyle Tso

author
#0 lore raw

PATCH v3 1/1 usb: typec: tcpm: Extend the matching rules on PPS APDO selection

Current matching rules ensure that the voltage range of selected Source Capability is entirely within the range defined in one of the Sink Capabilities. This is reasonable but not practical because Sink may not support wide range of voltage when sinking power while Source could advertise its capabilities in relatively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V Prog of Fixed Nominal Voltage) will not be selected if the Sink requires 5V-12V@3A PPS power. However, the Sink could work well if the requested voltage range in RDOs is 5V-11V@3A.

Currently accepted:

		|--------- source -----|
	|----------- sink ---------------|

Currently not accepted:

			|--------- source -----|
	|----------- sink ---------------|
	|--------- source -----|
		|----------- sink ---------------|
	|--------- source -----------------|
		|------ sink -------|

To improve the usability, change the matching rules to what listed below:
a. The Source PDO is selectable if any portion of the voltage range
overlaps one of the Sink PDO's voltage range.
b. The maximum operational voltage will be the lower one between the
selected Source PDO and the matching Sink PDO.
c. The maximum power will be the maximum operational voltage times the
maximum current defined in the selected Source PDO
d. Select the Source PDO with the highest maximum power

---
Changelog since v1:
- updated the commit message as suggested by Guenter Roeck <[email protected]>

Changelog since v2:
- fixed the coding style problems as suggested by Heikki Krogerus <[email protected]>
- Corrected a wrong word in the commit message as suggested by Adam Thomson <[email protected]>

---
 drivers/usb/typec/tcpm/tcpm.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +20 -15
index 3620efee2688..4bc29b586698 100644
@@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
 	unsigned int i, j, max_mw = 0, max_mv = 0;
 	unsigned int min_src_mv, max_src_mv, src_ma, src_mw;
 	unsigned int min_snk_mv, max_snk_mv;
-	u32 pdo;
+	unsigned int max_op_mv;
+	u32 pdo, src, snk;
 	unsigned int src_pdo = 0, snk_pdo = 0;
 
 	/*
@@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
 					continue;
 				}
 
-				if (max_src_mv <= max_snk_mv &&
-				    min_src_mv >= min_snk_mv) {
+				if (min_src_mv <= max_snk_mv &&
+				    max_src_mv >= min_snk_mv) {
+					max_op_mv = min(max_src_mv, max_snk_mv);
+					src_mw = (max_op_mv * src_ma) / 1000;
 					/* Prefer higher voltages if available */
 					if ((src_mw == max_mw &&
-					     min_src_mv > max_mv) ||
+					     max_op_mv > max_mv) ||
 					    src_mw > max_mw) {
 						src_pdo = i;
 						snk_pdo = j;
 						max_mw = src_mw;
-						max_mv = max_src_mv;
+						max_mv = max_op_mv;
 					}
 				}
 			}
@@ -2285,16 +2288,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
 	}
 
 	if (src_pdo) {
-		pdo = port->source_caps[src_pdo];
-
-		port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo);
-		port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo);
-		port->pps_data.max_curr =
-			min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]);
-		port->pps_data.out_volt =
-			min(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt);
-		port->pps_data.op_curr =
-			min(port->pps_data.max_curr, port->pps_data.op_curr);
+		src = port->source_caps[src_pdo];
+		snk = port->snk_pdo[snk_pdo];
+
+		port->pps_data.min_volt = max(pdo_pps_apdo_min_voltage(src),
+					      pdo_pps_apdo_min_voltage(snk));
+		port->pps_data.max_volt = min(pdo_pps_apdo_max_voltage(src),
+					      pdo_pps_apdo_max_voltage(snk));
+		port->pps_data.max_curr = min_pps_apdo_current(src, snk);
+		port->pps_data.out_volt = min(port->pps_data.max_volt,
+					      port->pps_data.out_volt);
+		port->pps_data.op_curr = min(port->pps_data.max_curr,
+					     port->pps_data.op_curr);
 	}
 
 	return src_pdo;
Acked-by on v3 1/1
#1 lore raw
Quoted 33 lines · Kyle Tso

Current matching rules ensure that the voltage range of selected Source Capability is entirely within the range defined in one of the Sink Capabilities. This is reasonable but not practical because Sink may not support wide range of voltage when sinking power while Source could advertise its capabilities in relatively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V Prog of Fixed Nominal Voltage) will not be selected if the Sink requires 5V- 12V@3A PPS power. However, the Sink could work well if the requested voltage range in RDOs is 5V-11V@3A.

Currently accepted:

		|--------- source -----|
	|----------- sink ---------------|

Currently not accepted:

			|--------- source -----|
	|----------- sink ---------------|
	|--------- source -----|
		|----------- sink ---------------|
	|--------- source -----------------|
		|------ sink -------|

To improve the usability, change the matching rules to what listed below:
a. The Source PDO is selectable if any portion of the voltage range
overlaps one of the Sink PDO's voltage range.
b. The maximum operational voltage will be the lower one between the
selected Source PDO and the matching Sink PDO.
c. The maximum power will be the maximum operational voltage times the
maximum current defined in the selected Source PDO d. Select the Source PDO
with the highest maximum power

Quoted 1 line
Quoted 91 lines

---
Changelog since v1:
- updated the commit message as suggested by Guenter Roeck <linux@roeck- us.net>

Changelog since v2:
- fixed the coding style problems as suggested by Heikki Krogerus <[email protected]>
- Corrected a wrong word in the commit message as suggested by Adam Thomson
<[email protected]>

---
 drivers/usb/typec/tcpm/tcpm.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +2 -1
index 3620efee2688..4bc29b586698 100644
@@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
 tcpm_port *port)
 	unsigned int i, j, max_mw = 0, max_mv = 0;
 	unsigned int min_src_mv, max_src_mv, src_ma, src_mw;
 	unsigned int min_snk_mv, max_snk_mv;
-	u32 pdo;
+	unsigned int max_op_mv;
+	u32 pdo, src, snk;
 	unsigned int src_pdo = 0, snk_pdo = 0;
 
drivers/usb/typec/tcpm/tcpm.c +6 -4
 	/*
@@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
 tcpm_port *port)
 					continue;
 				}
 
-				if (max_src_mv <= max_snk_mv &&
-				    min_src_mv >= min_snk_mv) {
+				if (min_src_mv <= max_snk_mv &&
+				    max_src_mv >= min_snk_mv) {
+					max_op_mv = min(max_src_mv,
 max_snk_mv);
+					src_mw = (max_op_mv * src_ma) / 1000;
 					/* Prefer higher voltages if available */
 					if ((src_mw == max_mw &&
-					     min_src_mv > max_mv) ||
+					     max_op_mv > max_mv) ||
 					    src_mw > max_mw) {
 						src_pdo = i;
 						snk_pdo = j;
 						max_mw = src_mw;
-						max_mv = max_src_mv;
+						max_mv = max_op_mv;
 					}
drivers/usb/typec/tcpm/tcpm.c +6 -10
 				}
 			}
@@ -2285,16 +2288,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
 tcpm_port *port)
 	}
 
 	if (src_pdo) {
-		pdo = port->source_caps[src_pdo];
-
-		port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo);
-		port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo);
-		port->pps_data.max_curr =
-			min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]);
-		port->pps_data.out_volt =
-			min(pdo_pps_apdo_max_voltage(pdo), port-
 >pps_data.out_volt);
-		port->pps_data.op_curr =
-			min(port->pps_data.max_curr, port->pps_data.op_curr);
+		src = port->source_caps[src_pdo];
+		snk = port->snk_pdo[snk_pdo];
+
+		port->pps_data.min_volt =
 max(pdo_pps_apdo_min_voltage(src),
+					      pdo_pps_apdo_min_voltage(snk));
+		port->pps_data.max_volt =

min(pdo_pps_apdo_max_voltage(src),

drivers/usb/typec/tcpm/tcpm.c +6 -1
+					      pdo_pps_apdo_max_voltage(snk));
+		port->pps_data.max_curr = min_pps_apdo_current(src, snk);
+		port->pps_data.out_volt = min(port->pps_data.max_volt,
+					      port->pps_data.out_volt);
+		port->pps_data.op_curr = min(port->pps_data.max_curr,
+					     port->pps_data.op_curr);
 	}
 
 	return src_pdo;
--

2.20.0.405.gbc1bbc6f85-goog

maintainer Reviewed-by on v3 1/1
#2 lore raw
In reply to Kyle Tso
Quoted 34 lines · Kyle Tso

Current matching rules ensure that the voltage range of selected Source Capability is entirely within the range defined in one of the Sink Capabilities. This is reasonable but not practical because Sink may not support wide range of voltage when sinking power while Source could advertise its capabilities in relatively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V Prog of Fixed Nominal Voltage) will not be selected if the Sink requires 5V-12V@3A PPS power. However, the Sink could work well if the requested voltage range in RDOs is 5V-11V@3A.

Currently accepted:

		|--------- source -----|
	|----------- sink ---------------|

Currently not accepted:

			|--------- source -----|
	|----------- sink ---------------|
	|--------- source -----|
		|----------- sink ---------------|
	|--------- source -----------------|
		|------ sink -------|

To improve the usability, change the matching rules to what listed below:
a. The Source PDO is selectable if any portion of the voltage range
overlaps one of the Sink PDO's voltage range.
b. The maximum operational voltage will be the lower one between the
selected Source PDO and the matching Sink PDO.
c. The maximum power will be the maximum operational voltage times the
maximum current defined in the selected Source PDO
d. Select the Source PDO with the highest maximum power

Quoted 1 line
Quoted 81 lines

---
Changelog since v1:
- updated the commit message as suggested by Guenter Roeck <[email protected]>

Changelog since v2:
- fixed the coding style problems as suggested by Heikki Krogerus <[email protected]>
- Corrected a wrong word in the commit message as suggested by Adam Thomson <[email protected]>

---
 drivers/usb/typec/tcpm/tcpm.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +20 -15
index 3620efee2688..4bc29b586698 100644
@@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
 	unsigned int i, j, max_mw = 0, max_mv = 0;
 	unsigned int min_src_mv, max_src_mv, src_ma, src_mw;
 	unsigned int min_snk_mv, max_snk_mv;
-	u32 pdo;
+	unsigned int max_op_mv;
+	u32 pdo, src, snk;
 	unsigned int src_pdo = 0, snk_pdo = 0;
 
 	/*
@@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
 					continue;
 				}
 
-				if (max_src_mv <= max_snk_mv &&
-				    min_src_mv >= min_snk_mv) {
+				if (min_src_mv <= max_snk_mv &&
+				    max_src_mv >= min_snk_mv) {
+					max_op_mv = min(max_src_mv, max_snk_mv);
+					src_mw = (max_op_mv * src_ma) / 1000;
 					/* Prefer higher voltages if available */
 					if ((src_mw == max_mw &&
-					     min_src_mv > max_mv) ||
+					     max_op_mv > max_mv) ||
 					    src_mw > max_mw) {
 						src_pdo = i;
 						snk_pdo = j;
 						max_mw = src_mw;
-						max_mv = max_src_mv;
+						max_mv = max_op_mv;
 					}
 				}
 			}
@@ -2285,16 +2288,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
 	}
 
 	if (src_pdo) {
-		pdo = port->source_caps[src_pdo];
-
-		port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo);
-		port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo);
-		port->pps_data.max_curr =
-			min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]);
-		port->pps_data.out_volt =
-			min(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt);
-		port->pps_data.op_curr =
-			min(port->pps_data.max_curr, port->pps_data.op_curr);
+		src = port->source_caps[src_pdo];
+		snk = port->snk_pdo[snk_pdo];
+
+		port->pps_data.min_volt = max(pdo_pps_apdo_min_voltage(src),
+					      pdo_pps_apdo_min_voltage(snk));
+		port->pps_data.max_volt = min(pdo_pps_apdo_max_voltage(src),
+					      pdo_pps_apdo_max_voltage(snk));
+		port->pps_data.max_curr = min_pps_apdo_current(src, snk);
+		port->pps_data.out_volt = min(port->pps_data.max_volt,
+					      port->pps_data.out_volt);
+		port->pps_data.op_curr = min(port->pps_data.max_curr,
+					     port->pps_data.op_curr);
 	}
 
 	return src_pdo;
Signature · 1 line
2.20.0.405.gbc1bbc6f85-goog

thanks,