[v5,1/5] usb: typec: tcpm: pdo matching optimization

[Guenter Roeck <linux@roeck-us.net>]

On Fri, Apr 20, 2018 at 06:13:30PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Mon, Apr 16, 2018 at 02:54:33PM +0800, Li Jun wrote:
> > This patch is a combination of commit 57e6f0d7b804
> > ("typec: tcpm: Only request matching pdos") and source
> > pdo selection optimization based on it, instead of only
> > compare between the same pdo type of sink and source,
> > we should check source pdo voltage range is within the
> > voltage range of one sink pdo.
> > 
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> 
> I don't see any problem with this, but I hope Guenter has time to
> check it.
> 

I don't see anything wrong, but then the changes are so complex that
I can only hope they were well tested. Unfortunately, I won't have
time to do such testing myself.

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

> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> > ---
> >  drivers/usb/typec/tcpm.c | 105 ++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 67 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > index 677d121..048b953 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -1772,39 +1772,63 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
> >  	return 0;
> >  }
> >  
> > -static int tcpm_pd_select_pdo(struct tcpm_port *port)
> > +#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
> > +#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
> > +
> > +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
> > +			      int *src_pdo)
> >  {
> > -	unsigned int i, max_mw = 0, max_mv = 0;
> > +	unsigned int i, j, max_src_mv = 0, min_src_mv = 0, max_mw = 0,
> > +		     max_mv = 0, src_mw = 0, src_ma = 0, max_snk_mv = 0,
> > +		     min_snk_mv = 0;
> 
> Don't resend because of this comment, but just for the record, you are
> declaring here so many variables at once that it is not easy to see
> what you are introducing (at least for me). It is OK to declare every
> variable even separately.
> 
> >  	int ret = -EINVAL;
> >  
> >  	/*
> > -	 * Select the source PDO providing the most power while staying within
> > -	 * the board's voltage limits. Prefer PDO providing exp
> > +	 * Select the source PDO providing the most power which has a
> > +	 * matchig sink cap.
> >  	 */
> >  	for (i = 0; i < port->nr_source_caps; i++) {
> >  		u32 pdo = port->source_caps[i];
> >  		enum pd_pdo_type type = pdo_type(pdo);
> > -		unsigned int mv, ma, mw;
> >  
> > -		if (type == PDO_TYPE_FIXED)
> > -			mv = pdo_fixed_voltage(pdo);
> > -		else
> > -			mv = pdo_min_voltage(pdo);
> > +		if (type == PDO_TYPE_FIXED) {
> > +			max_src_mv = pdo_fixed_voltage(pdo);
> > +			min_src_mv = max_src_mv;
> > +		} else {
> > +			max_src_mv = pdo_max_voltage(pdo);
> > +			min_src_mv = pdo_min_voltage(pdo);
> > +		}
> >  
> >  		if (type == PDO_TYPE_BATT) {
> > -			mw = pdo_max_power(pdo);
> > +			src_mw = pdo_max_power(pdo);
> >  		} else {
> > -			ma = min(pdo_max_current(pdo),
> > -				 port->max_snk_ma);
> > -			mw = ma * mv / 1000;
> > +			src_ma = pdo_max_current(pdo);
> > +			src_mw = src_ma * min_src_mv / 1000;
> >  		}
> >  
> > -		/* Perfer higher voltages if available */
> > -		if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
> > -		    mv <= port->max_snk_mv) {
> > -			ret = i;
> > -			max_mw = mw;
> > -			max_mv = mv;
> > +		for (j = 0; j < port->nr_snk_pdo; j++) {
> > +			pdo = port->snk_pdo[j];
> > +
> > +			if (pdo_type(pdo) == PDO_TYPE_FIXED) {
> > +				min_snk_mv = pdo_fixed_voltage(pdo);
> > +				max_snk_mv = pdo_fixed_voltage(pdo);
> > +			} else {
> > +				min_snk_mv = pdo_min_voltage(pdo);
> > +				max_snk_mv = pdo_max_voltage(pdo);
> > +			}
> > +
> > +			if (max_src_mv <= max_snk_mv &&
> > +				min_src_mv >= min_snk_mv) {
> > +				/* Prefer higher voltages if available */
> > +				if ((src_mw == max_mw && min_src_mv > max_mv) ||
> > +							src_mw > max_mw) {
> > +					*src_pdo = i;
> > +					*sink_pdo = j;
> > +					max_mw = src_mw;
> > +					max_mv = min_src_mv;
> > +					ret = 0;
> > +				}
> > +			}
> >  		}
> >  	}
> >  
> > @@ -1816,13 +1840,16 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
> >  	unsigned int mv, ma, mw, flags;
> >  	unsigned int max_ma, max_mw;
> >  	enum pd_pdo_type type;
> > -	int index;
> > -	u32 pdo;
> > +	int src_pdo_index, snk_pdo_index;
> > +	u32 pdo, matching_snk_pdo;
> > +	int ret;
> >  
> > -	index = tcpm_pd_select_pdo(port);
> > -	if (index < 0)
> > -		return -EINVAL;
> > -	pdo = port->source_caps[index];
> > +	ret = tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pdo = port->source_caps[src_pdo_index];
> > +	matching_snk_pdo = port->snk_pdo[snk_pdo_index];
> >  	type = pdo_type(pdo);
> >  
> >  	if (type == PDO_TYPE_FIXED)
> > @@ -1830,26 +1857,28 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
> >  	else
> >  		mv = pdo_min_voltage(pdo);
> >  
> > -	/* Select maximum available current within the board's power limit */
> > +	/* Select maximum available current within the sink pdo's limit */
> >  	if (type == PDO_TYPE_BATT) {
> > -		mw = pdo_max_power(pdo);
> > -		ma = 1000 * min(mw, port->max_snk_mw) / mv;
> > +		mw = min_power(pdo, matching_snk_pdo);
> > +		ma = 1000 * mw / mv;
> >  	} else {
> > -		ma = min(pdo_max_current(pdo),
> > -			 1000 * port->max_snk_mw / mv);
> > +		ma = min_current(pdo, matching_snk_pdo);
> > +		mw = ma * mv / 1000;
> >  	}
> > -	ma = min(ma, port->max_snk_ma);
> >  
> >  	flags = RDO_USB_COMM | RDO_NO_SUSPEND;
> >  
> >  	/* Set mismatch bit if offered power is less than operating power */
> > -	mw = ma * mv / 1000;
> >  	max_ma = ma;
> >  	max_mw = mw;
> >  	if (mw < port->operating_snk_mw) {
> >  		flags |= RDO_CAP_MISMATCH;
> > -		max_mw = port->operating_snk_mw;
> > -		max_ma = max_mw * 1000 / mv;
> > +		if (type == PDO_TYPE_BATT &&
> > +		    (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
> > +			max_mw = pdo_max_power(matching_snk_pdo);
> > +		else if (pdo_max_current(matching_snk_pdo) >
> > +			 pdo_max_current(pdo))
> > +			max_ma = pdo_max_current(matching_snk_pdo);
> >  	}
> >  
> >  	tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
> > @@ -1858,16 +1887,16 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
> >  		 port->polarity);
> >  
> >  	if (type == PDO_TYPE_BATT) {
> > -		*rdo = RDO_BATT(index + 1, mw, max_mw, flags);
> > +		*rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
> >  
> >  		tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
> > -			 index, mv, mw,
> > +			 src_pdo_index, mv, mw,
> >  			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
> >  	} else {
> > -		*rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
> > +		*rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
> >  
> >  		tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
> > -			 index, mv, ma,
> > +			 src_pdo_index, mv, ma,
> >  			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
> >  	}
> >  
> > -- 
> > 2.7.4
> 
> -- 
> heikki
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html