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@xxxxxxxxxx>
> > ---
> >  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
>