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

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

[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 raletively 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.

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

Signed-off-by: Kyle Tso <kyletso@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 3620efee2688..3001df7bd602 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -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,14 +2288,16 @@ 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]);
+		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(pdo_pps_apdo_max_voltage(pdo), 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);
 	}
-- 
2.20.0.rc2.403.gdbc3b29805-goog

Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Guenter Roeck]

On Thu, Dec 06, 2018 at 11:02:27AM +0800, Kyle Tso wrote:
> 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 raletively 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.
> 

Maybe a graphical description would help ?

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
> 
> Signed-off-by: Kyle Tso <kyletso@google.com>

Makes sense to me. I am a bit concerned that it might cause odd regressions,
though. Did you test it with a few adapters ?

With the expectation that you did,

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3620efee2688..3001df7bd602 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -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,14 +2288,16 @@ 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]);
> +		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(pdo_pps_apdo_max_voltage(pdo), 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);
>  	}
> -- 
> 2.20.0.rc2.403.gdbc3b29805-goog
> 

Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Heikki Krogerus]

On Thu, Dec 06, 2018 at 11:02:27AM +0800, Kyle Tso wrote:
> 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 raletively 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.
> 
> 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
> 
> Signed-off-by: Kyle Tso <kyletso@google.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3620efee2688..3001df7bd602 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -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,14 +2288,16 @@ 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]);
> +		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(pdo_pps_apdo_max_voltage(pdo), 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);
>  	}
> -- 
> 2.20.0.rc2.403.gdbc3b29805-goog

-- 
heikki

RE: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Adam Thomson]

On 06 December 2018 03:02, Kyle Tso wrote:

> 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
> raletively 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.

Is there a real world example of a sink requiring the 5V - 12V range? In that
scenario could we not add an additional sink capability which allows for this
range to be supported, and the current implementation should work just fine?

> 
> 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
> 
> Signed-off-by: Kyle Tso <kyletso@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3620efee2688..3001df7bd602 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -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,14 +2288,16 @@ 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]);
> +		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(pdo_pps_apdo_max_voltage(pdo), 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);
>  	}
> --
> 2.20.0.rc2.403.gdbc3b29805-goog

RE: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Adam Thomson]

On 10 December 2018 09:01, Adam Thomson wrote:

> On 06 December 2018 03:02, Kyle Tso wrote:
> 
> > 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 raletively 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.
> 
> Is there a real world example of a sink requiring the 5V - 12V range? In that
> scenario could we not add an additional sink capability which allows for this range
> to be supported, and the current implementation should work just fine?

Ok, I maybe should have waited until after my morning coffee to respond. So
because the lower limit on the sink side, is higher than the advertised source's
PPS minimum voltage it never gets selected? Personally I'd prefer to keep the
upper limit checking as is as I think that's an additional safety benefit
helping to prevent over-voltage scenarios. I think if a PPS APDO can supply up
to 11V then the system should be capable of handling that voltage, otherwise
it shouldn't be considered at all. The Source provides limits checking as well
to make sure the Sink doesn't request a value above the maximum voltage limit
for that selected APDO.

For the lower limit I'm more inclined to agree with allowing a higher minimum
on the sink side as that's less of a safety/damage issue as I understand it.
FWIW, what is the real world scenario? What happens if voltage drops below 5V?

> 
> >
> > 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
> >
> > Signed-off-by: Kyle Tso <kyletso@google.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c
> > b/drivers/usb/typec/tcpm/tcpm.c index 3620efee2688..3001df7bd602
> > 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -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,14 +2288,16 @@ 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]);
> > +		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(pdo_pps_apdo_max_voltage(pdo), 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);
> >  	}
> > --
> > 2.20.0.rc2.403.gdbc3b29805-goog
>

Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Kyle Tso]

On Fri, Dec 7, 2018 at 1:56 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Dec 06, 2018 at 11:02:27AM +0800, Kyle Tso wrote:
> > 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 raletively 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.
> >
>
> Maybe a graphical description would help ?
>
> Currently accepted:
>                 |--------- source -----|
>         |----------- sink ---------------|
>
> Currently not accepted:
>
>                         |--------- source -----|
>         |----------- sink ---------------|
>
>         |--------- source -----|
>                 |----------- sink ---------------|
>
>         |--------- source -----------------|
>                 |------ sink -------|
>

Sorry for late reply. I was on vacation.
Thanks for the suggestion. I will update the commit message.

> > 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
> >
> > Signed-off-by: Kyle Tso <kyletso@google.com>
>
> Makes sense to me. I am a bit concerned that it might cause odd regressions,
> though. Did you test it with a few adapters ?
>
> With the expectation that you did,
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>

Yes I have tested this patch with 3 different brands of adapters with
PPS capabilities.

thanks,
Kyle

Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Kyle Tso]

On Mon, Dec 10, 2018 at 7:36 PM Adam Thomson
<Adam.Thomson.Opensource@diasemi.com> wrote:
>
> On 10 December 2018 09:01, Adam Thomson wrote:
>
> > On 06 December 2018 03:02, Kyle Tso wrote:
> >
> > > 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 raletively 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.
> >
> > Is there a real world example of a sink requiring the 5V - 12V range? In that
> > scenario could we not add an additional sink capability which allows for this range
> > to be supported, and the current implementation should work just fine?
>
> Ok, I maybe should have waited until after my morning coffee to respond. So
> because the lower limit on the sink side, is higher than the advertised source's
> PPS minimum voltage it never gets selected? Personally I'd prefer to keep the
> upper limit checking as is as I think that's an additional safety benefit
> helping to prevent over-voltage scenarios. I think if a PPS APDO can supply up
> to 11V then the system should be capable of handling that voltage, otherwise
> it shouldn't be considered at all. The Source provides limits checking as well
> to make sure the Sink doesn't request a value above the maximum voltage limit
> for that selected APDO.
>

If the over-voltage occurs, it means:
1. the adapter malfunctioned. or
2. the code on the Sink accidentally requests a voltage level which is
over the limit of the Sink.

For 1., it is difficult to predict the behaviors of a malfunctioned
adapter. The over-voltage event may happen even if the Sink doesn't
select the APDO from this broken adapter.
For 2., it is difficult to predict the behaviors from the careless code as well.

> For the lower limit I'm more inclined to agree with allowing a higher minimum
> on the sink side as that's less of a safety/damage issue as I understand it.
> FWIW, what is the real world scenario? What happens if voltage drops below 5V?
>

Some products (in Sink mode) have under-voltage protection (the lower
bound might be around 3.8V - 4V before
the calculation of IR-drop) that will cause the disconnection.

thanks,
Kyle

RE: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Adam Thomson]

On 12 December 2018 02:47, Kyle Tso wrote:

> On Mon, Dec 10, 2018 at 7:36 PM Adam Thomson
> <Adam.Thomson.Opensource@diasemi.com> wrote:
> >
> > On 10 December 2018 09:01, Adam Thomson wrote:
> >
> > > On 06 December 2018 03:02, Kyle Tso wrote:
> > >
> > > > 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 raletively
> > > > 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.
> > >
> > > Is there a real world example of a sink requiring the 5V - 12V
> > > range? In that scenario could we not add an additional sink
> > > capability which allows for this range to be supported, and the current
> implementation should work just fine?
> >
> > Ok, I maybe should have waited until after my morning coffee to
> > respond. So because the lower limit on the sink side, is higher than
> > the advertised source's PPS minimum voltage it never gets selected?
> > Personally I'd prefer to keep the upper limit checking as is as I
> > think that's an additional safety benefit helping to prevent
> > over-voltage scenarios. I think if a PPS APDO can supply up to 11V
> > then the system should be capable of handling that voltage, otherwise
> > it shouldn't be considered at all. The Source provides limits checking
> > as well to make sure the Sink doesn't request a value above the maximum
> voltage limit for that selected APDO.
> >
> 
> If the over-voltage occurs, it means:
> 1. the adapter malfunctioned. or
> 2. the code on the Sink accidentally requests a voltage level which is over the limit
> of the Sink.
> 
> For 1., it is difficult to predict the behaviors of a malfunctioned adapter. The over-
> voltage event may happen even if the Sink doesn't select the APDO from this
> broken adapter.

Yes, I agree it's almost impossible to do anything from software to mitigate
this which is why the HW design has to have protection for this.

> For 2., it is difficult to predict the behaviors from the careless code as well.

Yes, that's also true, but if it's coded with the intention not to select an
option that's potentially higher than the system can handle then we're less
likely to fall foul of over-voltage scenarios in my opinion. By selecting a
PPS APDO with an upper threshold which falls within the board limits, assuming
the code were to accidentally request something higher than the PPS APDO maximum
then the Source should reject this. Just feels a little safer as we're talking
about controlling an external power source. At the end of the day though the
decision lies with the maintainers on this.

> > For the lower limit I'm more inclined to agree with allowing a higher
> > minimum on the sink side as that's less of a safety/damage issue as I
> understand it.
> > FWIW, what is the real world scenario? What happens if voltage drops below
> 5V?
> >
> 
> Some products (in Sink mode) have under-voltage protection (the lower bound
> might be around 3.8V - 4V before the calculation of IR-drop) that will cause the
> disconnection.

Ok, so the system would just stop charging, correct? I guess the calling code
to control the Source/adapter, via TCPM, wouldn't select a value below 4V in that
scenario anyway?

> 
> thanks,
> Kyle

Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Kyle Tso]

On Wed, Dec 12, 2018 at 6:15 PM Adam Thomson
<Adam.Thomson.Opensource@diasemi.com> wrote:
>
> On 12 December 2018 02:47, Kyle Tso wrote:
>
> > On Mon, Dec 10, 2018 at 7:36 PM Adam Thomson
> > <Adam.Thomson.Opensource@diasemi.com> wrote:
> > >
> > > On 10 December 2018 09:01, Adam Thomson wrote:
> > >
> > > > On 06 December 2018 03:02, Kyle Tso wrote:
> > > >
> > > > > 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 raletively
> > > > > 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.
> > > >
> > > > Is there a real world example of a sink requiring the 5V - 12V
> > > > range? In that scenario could we not add an additional sink
> > > > capability which allows for this range to be supported, and the current
> > implementation should work just fine?
> > >
> > > Ok, I maybe should have waited until after my morning coffee to
> > > respond. So because the lower limit on the sink side, is higher than
> > > the advertised source's PPS minimum voltage it never gets selected?
> > > Personally I'd prefer to keep the upper limit checking as is as I
> > > think that's an additional safety benefit helping to prevent
> > > over-voltage scenarios. I think if a PPS APDO can supply up to 11V
> > > then the system should be capable of handling that voltage, otherwise
> > > it shouldn't be considered at all. The Source provides limits checking
> > > as well to make sure the Sink doesn't request a value above the maximum
> > voltage limit for that selected APDO.
> > >
> >
> > If the over-voltage occurs, it means:
> > 1. the adapter malfunctioned. or
> > 2. the code on the Sink accidentally requests a voltage level which is over the limit
> > of the Sink.
> >
> > For 1., it is difficult to predict the behaviors of a malfunctioned adapter. The over-
> > voltage event may happen even if the Sink doesn't select the APDO from this
> > broken adapter.
>
> Yes, I agree it's almost impossible to do anything from software to mitigate
> this which is why the HW design has to have protection for this.
>
> > For 2., it is difficult to predict the behaviors from the careless code as well.
>
> Yes, that's also true, but if it's coded with the intention not to select an
> option that's potentially higher than the system can handle then we're less
> likely to fall foul of over-voltage scenarios in my opinion. By selecting a
> PPS APDO with an upper threshold which falls within the board limits, assuming
> the code were to accidentally request something higher than the PPS APDO maximum
> then the Source should reject this. Just feels a little safer as we're talking
> about controlling an external power source. At the end of the day though the
> decision lies with the maintainers on this.
>

The implementation of PPS in TCPM doesn't account for the decision of
the content
in each RDO. It is nearly fully passive and receives the key values
(voltage/current) from
external codes through the power_supply framework. There are already some basic
checks in TCPM which reject invalid requests from the external codes.

pps_data.min_volt
pps_data.max_volt
pps_data.max_curr

These values are set in tcpm_pd_select_pps_apdo() and they are
restricted within the
board limits (and vice versa if the limitation is on the source side).
I think it is enough
to protect it from careless external codes.

> > > For the lower limit I'm more inclined to agree with allowing a higher
> > > minimum on the sink side as that's less of a safety/damage issue as I
> > understand it.
> > > FWIW, what is the real world scenario? What happens if voltage drops below
> > 5V?
> > >
> >
> > Some products (in Sink mode) have under-voltage protection (the lower bound
> > might be around 3.8V - 4V before the calculation of IR-drop) that will cause the
> > disconnection.
>
> Ok, so the system would just stop charging, correct? I guess the calling code
> to control the Source/adapter, via TCPM, wouldn't select a value below 4V in that
> scenario anyway?

Yes, that's why I do these changes in this patch. It's weird if the
Sink claims its Sink
Capabilities with a wider range of voltage than it really can support.
But actually
many adapters (in market) have wider capabilities than this kind of Sink port.

thanks,
Kyle

RE: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Adam Thomson]

On 17 December 2018 12:45, Kyle Tso wrote:

> On Mon, Dec 17, 2018 at 8:23 PM Adam Thomson
> <Adam.Thomson.Opensource@diasemi.com> wrote:
> >
> > On 17 December 2018 02:48, Kyle Tso wrote:
> >
> > > 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 raletively wider range.
> > > For example, a Source PDO advertising 3.3V-11V@3A (9V
> >
> > relatively
> >
> 
> noted!
> Thanks for the correction. I will fix this in the next patch.
> 
> > > 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
> > >
> > > Signed-off-by: Kyle Tso <kyletso@google.com>
> > >
> > > ---
> > > Changelog since v1:
> > > - updated the commit message as suggested by Guenter Roeck
> > > <linux@roeck- us.net>
> > > ---
> > >  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
> > >  1 file changed, 17 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpm.c
> > > b/drivers/usb/typec/tcpm/tcpm.c index 3620efee2688..3001df7bd602
> > > 100644
> > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > @@ -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) {
> >
> > Sorry I didn't raise this before, but came to mind this morning when I
> > was considering your updates again. What happens if the Source validly
> > provides two PPS APDOs, for example:
> >
> >         3.3V - 11V, 3A (9V programmable)
> >         3.3V - 16V, 3A (15V programmable)
> >
> > and the sink APDO is:
> >
> >         5V - 9V, 3A
> >
> > I think the code here will now select the higher range (15V
> > programmable) as it gives a larger power output value, even if the
> > sink can only support a voltage that's far smaller. I really don't
> > think this is correct. *If* you are going to allow selection of PPS
> > APDOs that provide a larger voltage range than the Sink can actually
> > cope with, then I think you should at least select the lower of those advertised
> which fulfils the needs of the Sink.
> 
> Source:
> 3.3V - 11V, 3A (9V programmable)
> 3.3V - 16V, 3A (15V programmable)
> 
> Sink
> 5V - 9V, 3A
> 
> In this case, the Sink will select "3.3V - 11V, 3A (9V programmable)"
> because the
> "max_op_mv" when dealing with both Source Cap will be the same.
> 
> See line#2269, "max_op_mv" will be limited by "max_snk_mv" which is 9V.
> And in line#2273, "max_op_mv" (of the second src_cap) fails the check because
> "max_mv", which is equal to the "max_op_mv" of the previous src_cap, is the
> same as it.

Ok, yes I missed the re-calculation of src_mw on line 2270. Apologies.

> 
> 2267                             if (min_src_mv <= max_snk_mv &&
> 2268                                 max_src_mv >= min_snk_mv) {
> 2269                                     max_op_mv = min(max_src_mv,
> max_snk_mv);
> 2270                                     src_mw = (max_op_mv * src_ma) / 1000;
> 2271                                     /* Prefer higher voltages if
> available */
> 2272                                     if ((src_mw == max_mw &&
> 2273                                          max_op_mv > max_mv) ||
> 2274                                         src_mw > max_mw) {
> 2275                                             src_pdo = i;
> 2276                                             snk_pdo = j;
> 2277                                             max_mw = src_mw;
> 2278                                             max_mv = max_op_mv;
> 2279                                     }
> 2280                             }
> 
> >
> > >                                               src_pdo = i;
> > >                                               snk_pdo = j;
> > >                                               max_mw = src_mw;
> > > -                                             max_mv = max_src_mv;
> > > +                                             max_mv = max_op_mv;
> > >                                       }
> > >                               }
> > >                       }
> > > @@ -2285,14 +2288,16 @@ 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]);
> > > +             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(pdo_pps_apdo_max_voltage(pdo), 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);
> > >       }
> > > --
> > > 2.20.0.405.gbc1bbc6f85-goog
> >

Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Kyle Tso]

On Mon, Dec 17, 2018 at 8:45 PM Kyle Tso <kyletso@google.com> wrote:
>
> On Mon, Dec 17, 2018 at 8:23 PM Adam Thomson
> <Adam.Thomson.Opensource@diasemi.com> wrote:
> >
> > On 17 December 2018 02:48, Kyle Tso wrote:
> >
> > > 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
> > > raletively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V
> >
> > relatively
> >
>
> noted!
> Thanks for the correction. I will fix this in the next patch.
>
> > > 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
> > >
> > > Signed-off-by: Kyle Tso <kyletso@google.com>
> > >
> > > ---
> > > Changelog since v1:
> > > - updated the commit message as suggested by Guenter Roeck <linux@roeck-
> > > us.net>
> > > ---
> > >  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
> > >  1 file changed, 17 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > > index 3620efee2688..3001df7bd602 100644
> > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > @@ -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) {
> >
> > Sorry I didn't raise this before, but came to mind this morning when I was
> > considering your updates again. What happens if the Source validly provides two
> > PPS APDOs, for example:
> >
> >         3.3V - 11V, 3A (9V programmable)
> >         3.3V - 16V, 3A (15V programmable)
> >
> > and the sink APDO is:
> >
> >         5V - 9V, 3A
> >
> > I think the code here will now select the higher range (15V programmable) as it
> > gives a larger power output value, even if the sink can only support a voltage
> > that's far smaller. I really don't think this is correct. *If* you are going to
> > allow selection of PPS APDOs that provide a larger voltage range than the Sink
> > can actually cope with, then I think you should at least select the lower of
> > those advertised which fulfils the needs of the Sink.
>
> Source:
> 3.3V - 11V, 3A (9V programmable)
> 3.3V - 16V, 3A (15V programmable)
>
> Sink
> 5V - 9V, 3A
>
> In this case, the Sink will select "3.3V - 11V, 3A (9V programmable)"
> because the
> "max_op_mv" when dealing with both Source Cap will be the same.
>
> See line#2269, "max_op_mv" will be limited by "max_snk_mv" which is 9V.
> And in line#2273, "max_op_mv" (of the second src_cap) fails the check
> because "max_mv", which is equal to the "max_op_mv" of the previous src_cap,
> is the same as it.
>

Sorry, I should have explained more clearer here.
"max_op_mv" and "src_mw" are both limited to "max_snk_mv" (9V) in this
case. Therefore in the following "if" statement (line#2272 to line#2274), both
"src_mw" and "max_op_mv" remain the same as those regarding to
the previous src_cap. So the "if" statement will be "false".
==> select the previous src_cap

> 2267                             if (min_src_mv <= max_snk_mv &&
> 2268                                 max_src_mv >= min_snk_mv) {
> 2269                                     max_op_mv = min(max_src_mv,
> max_snk_mv);
> 2270                                     src_mw = (max_op_mv * src_ma) / 1000;
> 2271                                     /* Prefer higher voltages if
> available */
> 2272                                     if ((src_mw == max_mw &&
> 2273                                          max_op_mv > max_mv) ||
> 2274                                         src_mw > max_mw) {
> 2275                                             src_pdo = i;
> 2276                                             snk_pdo = j;
> 2277                                             max_mw = src_mw;
> 2278                                             max_mv = max_op_mv;
> 2279                                     }
> 2280                             }
>
> >
> > >                                               src_pdo = i;
> > >                                               snk_pdo = j;
> > >                                               max_mw = src_mw;
> > > -                                             max_mv = max_src_mv;
> > > +                                             max_mv = max_op_mv;
> > >                                       }
> > >                               }
> > >                       }
> > > @@ -2285,14 +2288,16 @@ 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]);
> > > +             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(pdo_pps_apdo_max_voltage(pdo), 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);
> > >       }
> > > --
> > > 2.20.0.405.gbc1bbc6f85-goog
> >

Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Kyle Tso]

On Mon, Dec 17, 2018 at 8:23 PM Adam Thomson
<Adam.Thomson.Opensource@diasemi.com> wrote:
>
> On 17 December 2018 02:48, Kyle Tso wrote:
>
> > 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
> > raletively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V
>
> relatively
>

noted!
Thanks for the correction. I will fix this in the next patch.

> > 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
> >
> > Signed-off-by: Kyle Tso <kyletso@google.com>
> >
> > ---
> > Changelog since v1:
> > - updated the commit message as suggested by Guenter Roeck <linux@roeck-
> > us.net>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 3620efee2688..3001df7bd602 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -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) {
>
> Sorry I didn't raise this before, but came to mind this morning when I was
> considering your updates again. What happens if the Source validly provides two
> PPS APDOs, for example:
>
>         3.3V - 11V, 3A (9V programmable)
>         3.3V - 16V, 3A (15V programmable)
>
> and the sink APDO is:
>
>         5V - 9V, 3A
>
> I think the code here will now select the higher range (15V programmable) as it
> gives a larger power output value, even if the sink can only support a voltage
> that's far smaller. I really don't think this is correct. *If* you are going to
> allow selection of PPS APDOs that provide a larger voltage range than the Sink
> can actually cope with, then I think you should at least select the lower of
> those advertised which fulfils the needs of the Sink.

Source:
3.3V - 11V, 3A (9V programmable)
3.3V - 16V, 3A (15V programmable)

Sink
5V - 9V, 3A

In this case, the Sink will select "3.3V - 11V, 3A (9V programmable)"
because the
"max_op_mv" when dealing with both Source Cap will be the same.

See line#2269, "max_op_mv" will be limited by "max_snk_mv" which is 9V.
And in line#2273, "max_op_mv" (of the second src_cap) fails the check
because "max_mv", which is equal to the "max_op_mv" of the previous src_cap,
is the same as it.

2267                             if (min_src_mv <= max_snk_mv &&
2268                                 max_src_mv >= min_snk_mv) {
2269                                     max_op_mv = min(max_src_mv,
max_snk_mv);
2270                                     src_mw = (max_op_mv * src_ma) / 1000;
2271                                     /* Prefer higher voltages if
available */
2272                                     if ((src_mw == max_mw &&
2273                                          max_op_mv > max_mv) ||
2274                                         src_mw > max_mw) {
2275                                             src_pdo = i;
2276                                             snk_pdo = j;
2277                                             max_mw = src_mw;
2278                                             max_mv = max_op_mv;
2279                                     }
2280                             }

>
> >                                               src_pdo = i;
> >                                               snk_pdo = j;
> >                                               max_mw = src_mw;
> > -                                             max_mv = max_src_mv;
> > +                                             max_mv = max_op_mv;
> >                                       }
> >                               }
> >                       }
> > @@ -2285,14 +2288,16 @@ 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]);
> > +             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(pdo_pps_apdo_max_voltage(pdo), 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);
> >       }
> > --
> > 2.20.0.405.gbc1bbc6f85-goog
>

Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Kyle Tso]

On Mon, Dec 17, 2018 at 7:36 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Mon, Dec 17, 2018 at 10:48:05AM +0800, Kyle Tso wrote:
> > 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 raletively 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
> >
> > Signed-off-by: Kyle Tso <kyletso@google.com>
>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>
> In case you'll do one more version, I have a minor comment about the
> style below, but no need to resend only because of that.
>
> > ---
> > Changelog since v1:
> > - updated the commit message as suggested by Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 3620efee2688..3001df7bd602 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -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,14 +2288,16 @@ 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]);
> > +             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(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt);
> > +                     min(port->pps_data.max_volt, port->pps_data.out_volt);
>
> While here, you could have aligned that like the above lines:
>
>         port->pps_data.out_volt = min(pdo_pps_apdo_max_voltage(pdo),
>                                       port->pps_data.out_volt);
>

Thanks Heikki Krogerus,
I will fix this (and the lines below as well) in the next (should be V3) patch.

> >               port->pps_data.op_curr =
> >                       min(port->pps_data.max_curr, port->pps_data.op_curr);
> >       }
> > --
> > 2.20.0.405.gbc1bbc6f85-goog
>
> thanks,
>
> --
> heikki

RE: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Adam Thomson]

On 17 December 2018 02:48, Kyle Tso wrote:

> 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
> raletively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V

relatively

> 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
> 
> Signed-off-by: Kyle Tso <kyletso@google.com>
> 
> ---
> Changelog since v1:
> - updated the commit message as suggested by Guenter Roeck <linux@roeck-
> us.net>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3620efee2688..3001df7bd602 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -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) {

Sorry I didn't raise this before, but came to mind this morning when I was
considering your updates again. What happens if the Source validly provides two
PPS APDOs, for example:

	3.3V - 11V, 3A (9V programmable)
	3.3V - 16V, 3A (15V programmable)

and the sink APDO is:

	5V - 9V, 3A

I think the code here will now select the higher range (15V programmable) as it
gives a larger power output value, even if the sink can only support a voltage
that's far smaller. I really don't think this is correct. *If* you are going to
allow selection of PPS APDOs that provide a larger voltage range than the Sink
can actually cope with, then I think you should at least select the lower of
those advertised which fulfils the needs of the Sink.

>  						src_pdo = i;
>  						snk_pdo = j;
>  						max_mw = src_mw;
> -						max_mv = max_src_mv;
> +						max_mv = max_op_mv;
>  					}
>  				}
>  			}
> @@ -2285,14 +2288,16 @@ 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]);
> +		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(pdo_pps_apdo_max_voltage(pdo), 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);
>  	}
> --
> 2.20.0.405.gbc1bbc6f85-goog

Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Heikki Krogerus]

On Mon, Dec 17, 2018 at 10:48:05AM +0800, Kyle Tso wrote:
> 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 raletively 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
> 
> Signed-off-by: Kyle Tso <kyletso@google.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

In case you'll do one more version, I have a minor comment about the
style below, but no need to resend only because of that.

> ---
> Changelog since v1:
> - updated the commit message as suggested by Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3620efee2688..3001df7bd602 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -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,14 +2288,16 @@ 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]);
> +		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(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt);
> +			min(port->pps_data.max_volt, port->pps_data.out_volt);

While here, you could have aligned that like the above lines:

	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);
>  	}
> -- 
> 2.20.0.405.gbc1bbc6f85-goog

thanks,

-- 
heikki

Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

[Guenter Roeck]

On 12/16/18 6:48 PM, Kyle Tso wrote:
> 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 raletively 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
> 
> Signed-off-by: Kyle Tso <kyletso@google.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> 
> ---
> Changelog since v1:
> - updated the commit message as suggested by Guenter Roeck <linux@roeck-us.net>
> ---
>   drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
>   1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3620efee2688..3001df7bd602 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -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,14 +2288,16 @@ 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]);
> +		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(pdo_pps_apdo_max_voltage(pdo), 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);
>   	}
> 

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

[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 raletively 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

Signed-off-by: Kyle Tso <kyletso@google.com>

---
Changelog since v1:
- updated the commit message as suggested by Guenter Roeck <linux@roeck-us.net>
---
 drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 3620efee2688..3001df7bd602 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -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,14 +2288,16 @@ 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]);
+		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(pdo_pps_apdo_max_voltage(pdo), 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);
 	}
-- 
2.20.0.405.gbc1bbc6f85-goog