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

[Heikki Krogerus]

Hi,

+Dan and Guenter

I'm sorry for the late reply. These slipped under my radar. I do
have a one more proposal below, and a few nits..

On Wed, Oct 18, 2017 at 01:22:48PM -0700, Badhri Jagan Sridharan wrote:
> At present, TCPM code assumes that local device supports
> variable/batt pdos and always selects the pdo with highest
> possible power within the board limit. This assumption
> might not hold good for all devices. To overcome this,
> this patch makes TCPM only accept a source_pdo when there is
> a matching sink pdo.
> 
> For Fixed pdos: The voltage should match between the
> incoming source_cap and the registered snk_pdo
> For Variable/Batt pdos: The incoming source_cap voltage
> range should fall within the registered snk_pdo's voltage
> range.
> 
> Also, when the cap_mismatch bit is set, the max_power/current
> should be set to the max_current/power of the sink_pdo.
> This is according to:
> 
> "If the Capability Mismatch bit is set to one
> The Maximum Operating Current/Power field may contain a value
> larger than the maximum current/power offered in the Source
> Capabilities message???s PDO as referenced by the Object position field.
> This enables the Sink to indicate that it requires more current/power
> than is being offered. If the Sink requires a different voltage this
> will be indicated by its Sink Capabilities message.
> 
> Signed-off-by: Badhri Jagan Sridharan <Badhri@xxxxxxxxxx>
> ---
> Changelog since v1:
> - Rebased the patch on top of drivers/usb/type/tcpm.c
> - Added duplicate pdo check for variable/batt pdo.
> - Fixed tabs as suggested by dan.carpenter@xxxxxxxxxx
> 
> Changelog since v2:
> - Rebase
> 
>  drivers/usb/typec/tcpm.c | 155 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 120 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index e3f8b3da46b0..1b37333dc63b 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -261,6 +261,9 @@ struct tcpm_port {
>  	unsigned int nr_src_pdo;
>  	u32 snk_pdo[PDO_MAX_OBJECTS];
>  	unsigned int nr_snk_pdo;
> +	unsigned int nr_fixed; /* number of fixed sink PDOs */
> +	unsigned int nr_var; /* number of variable sink PDOs */
> +	unsigned int nr_batt; /* number of battery sink PDOs */
>  	u32 snk_vdo[VDO_MAX_OBJECTS];
>  	unsigned int nr_snk_vdo;
>  
> @@ -1762,39 +1765,92 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
>  	return 0;
>  }
>  
> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
> +static void tcpm_pd_evaluate_pdo(struct tcpm_port *port, int src_pdo_index,
> +				 int snk_pdo_index, int *max_mw, int *max_mv,
> +				 int *selected_src_pdo,
> +				 int *matching_snk_pdo)
>  {
> -	unsigned int i, max_mw = 0, max_mv = 0;
> +	unsigned int mv = 0, ma = 0, mw = 0;
> +	u32 src_pdo = port->source_caps[src_pdo_index];
> +	u32 snk_pdo = port->snk_pdo[snk_pdo_index];
> +	enum pd_pdo_type type = pdo_type(src_pdo);
> +
> +	mv = (type == PDO_TYPE_FIXED) ? pdo_fixed_voltage(src_pdo) :
> +					pdo_min_voltage(src_pdo);
> +
> +	if (type == PDO_TYPE_BATT) {
> +		mw = min(pdo_max_power(src_pdo), pdo_max_power(snk_pdo));
> +	} else {
> +		ma = min(pdo_max_current(src_pdo), pdo_max_current(snk_pdo));
> +		mw = ma * mv / 1000;
> +	}
> +
> +	/* Perfer higher voltages if available */
> +	if (mw > *max_mw || (mw == *max_mw && mv > *max_mv)) {
> +		*selected_src_pdo = src_pdo_index;
> +		*matching_snk_pdo = snk_pdo_index;
> +		*max_mw = mw;
> +		*max_mv = mv;
> +	}
> +}
> +
> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo)
> +{
> +	unsigned int i, j, max_mw = 0, max_mv = 0;
>  	int ret = -EINVAL;
>  
>  	/*
> -	 * Select the source PDO providing the most power while staying within
> -	 * the board's voltage limits. Prefer PDO providing exp
> +	 * Select the source PDO providing the most power which has a
> +	 * matchig sink cap.
>  	 */
>  	for (i = 0; i < port->nr_source_caps; i++) {
>  		u32 pdo = port->source_caps[i];
>  		enum pd_pdo_type type = pdo_type(pdo);
> -		unsigned int mv, ma, mw;
> -
> -		if (type == PDO_TYPE_FIXED)
> -			mv = pdo_fixed_voltage(pdo);
> -		else
> -			mv = pdo_min_voltage(pdo);
> -
> -		if (type == PDO_TYPE_BATT) {
> -			mw = pdo_max_power(pdo);
> -		} else {
> -			ma = min(pdo_max_current(pdo),
> -				 port->max_snk_ma);
> -			mw = ma * mv / 1000;
> -		}
>  
> -		/* Perfer higher voltages if available */
> -		if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
> -		    mv <= port->max_snk_mv) {
> -			ret = i;
> -			max_mw = mw;
> -			max_mv = mv;
> +		if (type == PDO_TYPE_FIXED) {
> +			for (j = 0; j < port->nr_fixed; j++) {
> +				if (pdo_fixed_voltage(pdo) ==
> +				    pdo_fixed_voltage(port->snk_pdo[j])) {
> +					tcpm_pd_evaluate_pdo(port, i, j,
> +							     &max_mw, &max_mv,
> +							     &ret, sink_pdo);
> +					/* There could only be one fixed pdo
> +					 * at a specific voltage level.
> +					 * So breaking here.
> +					 */
> +					break;
> +				}
> +			}
> +		} else if (type == PDO_TYPE_BATT) {
> +			for (j = port->nr_fixed;
> +			     j < port->nr_fixed +
> +				 port->nr_batt;
> +			     j++) {
> +				if (pdo_min_voltage(pdo) >=
> +				     pdo_min_voltage(port->snk_pdo[j]) &&
> +				     pdo_max_voltage(pdo) <=
> +				     pdo_max_voltage(port->snk_pdo[j])) {
> +					tcpm_pd_evaluate_pdo(port, i, j,
> +							     &max_mw, &max_mv,
> +							     &ret, sink_pdo);
> +				}
> +			}
> +		} else if (type == PDO_TYPE_VAR) {
> +			for (j = port->nr_fixed +
> +				 port->nr_batt;
> +			     j < port->nr_fixed +
> +				 port->nr_batt +
> +				 port->nr_var;
> +			     j++) {
> +				if (pdo_min_voltage(pdo) >=
> +				     pdo_min_voltage(port->snk_pdo[j]) &&
> +				     pdo_max_voltage(pdo) <=
> +				     pdo_max_voltage(port->snk_pdo[j])) {
> +					tcpm_pd_evaluate_pdo(port, i, j,
> +							     &max_mw, &max_mv,
> +							     &ret, sink_pdo);
> +				}
> +			}
>  		}

Wouldn't it make sense to call tcpm_pd_evaluate_pdo() here ones and
run those nested loops there? You are already checking the type there
in any case.

>  	}
>  
> @@ -1806,13 +1862,14 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>  	unsigned int mv, ma, mw, flags;
>  	unsigned int max_ma, max_mw;
>  	enum pd_pdo_type type;
> -	int index;
> -	u32 pdo;
> +	int index, snk_pdo_index;
> +	u32 pdo, matching_snk_pdo;
>  
> -	index = tcpm_pd_select_pdo(port);
> +	index = tcpm_pd_select_pdo(port, &snk_pdo_index);
>  	if (index < 0)
>  		return -EINVAL;
>  	pdo = port->source_caps[index];
> +	matching_snk_pdo = port->snk_pdo[snk_pdo_index];
>  	type = pdo_type(pdo);
>  
>  	if (type == PDO_TYPE_FIXED)
> @@ -1820,26 +1877,32 @@ 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(pdo_max_power(pdo), pdo_max_power(matching_snk_pdo));
> +		ma = 1000 * mw / mv;
>  	} else {
>  		ma = min(pdo_max_current(pdo),
> -			 1000 * port->max_snk_mw / mv);
> +			 pdo_max_current(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) {
> +			if (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);
> +		}

How about:

                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);

Though, that does not make it any more readable. Just a proposal.

>  	}
>  
>  	tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
> @@ -3588,6 +3651,19 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>  }
>  EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>  
> +static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
> +				 enum pd_pdo_type type)

Please align that properly.

> +{
> +	int count = 0;
> +	int i;
> +
> +	for (i = 0; i < nr_pdo; i++) {
> +		if (pdo_type(pdo[i]) == type)
> +			count++;
> +	}
> +	return count;
> +}
> +
>  struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  {
>  	struct tcpm_port *port;
> @@ -3633,6 +3709,15 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  					  tcpc->config->nr_src_pdo);
>  	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo,
>  					  tcpc->config->nr_snk_pdo);
> +	port->nr_fixed =  nr_type_pdos(port->snk_pdo,
> +							port->nr_snk_pdo,
> +							PDO_TYPE_FIXED);

Ditto.

> +	port->nr_var = nr_type_pdos(port->snk_pdo,
> +						     port->nr_snk_pdo,
> +						     PDO_TYPE_VAR);

Ditto.

> +	port->nr_batt = nr_type_pdos(port->snk_pdo,
> +						      port->nr_snk_pdo,
> +						      PDO_TYPE_BATT);

Ditto.


Thanks,

-- 
heikki