[4.16,REGRESSION,fix] Revert "typec: tcpm: Only request matching pdos"

[4.16,REGRESSION,fix] Revert "typec: tcpm: Only request matching pdos"

[Heikki Krogerus]

On Tue, Mar 06, 2018 at 10:50:05AM +0100, Hans de Goede wrote:
> Commit 57e6f0d7b804 ("typec: tcpm: Only request matching pdos") is causing
> a regression, before this commit e.g. the GPD win and GPD pocket devices
> were charging at 9V 3A with a PD charger, now they are instead slowly
> discharging  at 5V 0.4A, as this commit causes the ports max_snk_mv/ma/mw
> settings to be completely ignored.
> 
> Arguably the way to fix this would be to add a PDO_VAR() describing the
> voltage range to the snk_caps of boards which can handle any voltage in
> their range, but the "typec: tcpm: Only request matching pdos" commit
> looks at the type of PDO advertised by the source/charger and if that
> is fixed (as it typically is) only compairs against PDO_FIXED entries
> in the snk_caps so supporting a range of voltage would require adding a
> PDO_FIXED entry for *every possible* voltage to snk_caps.
> 
> AFAICT there is no reason why a fixed source_cap cannot be matched against
> a variable snk_cap, so at a minimum the commit should be rewritten to
> support that.
> 
> For now lets revert the "typec: tcpm: Only request matching pdos" commit,
> fixing the regression.
> 
> Cc: Badhri Jagan Sridharan <badhri@google.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

You are correct. The patch should be rewritten.

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


Thanks,

[4.16,REGRESSION,fix] Revert "typec: tcpm: Only request matching pdos"

[Jun Li]

Hi,
> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org
> [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Heikki Krogerus
> Sent: 2018年3月6日 20:33
> To: Hans de Goede <hdegoede@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Guenter Roeck
> <linux@roeck-us.net>; linux-usb@vger.kernel.org; Badhri Jagan Sridharan
> <badhri@google.com>
> Subject: Re: [PATCH 4.16 REGRESSION fix] Revert "typec: tcpm: Only request
> matching pdos"
> 
> On Tue, Mar 06, 2018 at 10:50:05AM +0100, Hans de Goede wrote:
> > Commit 57e6f0d7b804 ("typec: tcpm: Only request matching pdos") is
> > causing a regression, before this commit e.g. the GPD win and GPD
> > pocket devices were charging at 9V 3A with a PD charger, now they are
> > instead slowly discharging  at 5V 0.4A, as this commit causes the
> > ports max_snk_mv/ma/mw settings to be completely ignored.
> >

So max_snk_mv/ma/mw settings are back, I am dealing with PD properties
definition, so want to ask for long term, should we keep them, or use the
reverting patch way(only compare sink PDOs Vs source PDOs) but fix the
problem with it.

Thanks
Jun Li

> > Arguably the way to fix this would be to add a PDO_VAR() describing
> > the voltage range to the snk_caps of boards which can handle any
> > voltage in their range, but the "typec: tcpm: Only request matching
> > pdos" commit looks at the type of PDO advertised by the source/charger
> > and if that is fixed (as it typically is) only compairs against
> > PDO_FIXED entries in the snk_caps so supporting a range of voltage
> > would require adding a PDO_FIXED entry for *every possible* voltage to
> snk_caps.
> >
> > AFAICT there is no reason why a fixed source_cap cannot be matched
> > against a variable snk_cap, so at a minimum the commit should be
> > rewritten to support that.
> >
> > For now lets revert the "typec: tcpm: Only request matching pdos"
> > commit, fixing the regression.
> >
> > Cc: Badhri Jagan Sridharan <badhri@google.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> You are correct. The patch should be rewritten.
> 
> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> 
> Thanks,
> 
> --
> 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
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.k
> ernel.org%2Fmajordomo-info.html&data=02%7C01%7Cjun.li%40nxp.com%7C
> 6cd4e7a122244982de5e08d5835e618c%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C636559363813086942&sdata=nZw4ta%2Frro44iG%2FfH
> bhUrhOKZBdE16JvgSYUsb71qzg%3D&reserved=0

[4.16,REGRESSION,fix] Revert "typec: tcpm: Only request matching pdos"

[Opensource [Adam Thomson]]

On 06 March 2018 09:50, Hans de Goede wrote:

> Commit 57e6f0d7b804 ("typec: tcpm: Only request matching pdos") is causing
> a regression, before this commit e.g. the GPD win and GPD pocket devices
> were charging at 9V 3A with a PD charger, now they are instead slowly
> discharging  at 5V 0.4A, as this commit causes the ports max_snk_mv/ma/mw
> settings to be completely ignored.
> 
> Arguably the way to fix this would be to add a PDO_VAR() describing the
> voltage range to the snk_caps of boards which can handle any voltage in
> their range, but the "typec: tcpm: Only request matching pdos" commit
> looks at the type of PDO advertised by the source/charger and if that
> is fixed (as it typically is) only compairs against PDO_FIXED entries
> in the snk_caps so supporting a range of voltage would require adding a
> PDO_FIXED entry for *every possible* voltage to snk_caps.
> 
> AFAICT there is no reason why a fixed source_cap cannot be matched against
> a variable snk_cap, so at a minimum the commit should be rewritten to
> support that.
> 
> For now lets revert the "typec: tcpm: Only request matching pdos" commit,
> fixing the regression.
> 
> Cc: Badhri Jagan Sridharan <badhri@google.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Thanks Hans. Sadly I alluded to this problem before the patch was pulled in but
this was seemingly missed:

 https://lkml.org/lkml/2017/11/28/186

FWIW, Acked-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> ---
>  drivers/usb/typec/tcpm.c | 163 ++++++++++++-----------------------------------
>  1 file changed, 42 insertions(+), 121 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index bfcaf6618a1f..735a6dd74c20 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -254,9 +254,6 @@ 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;
> 
> @@ -1786,90 +1783,39 @@ static int tcpm_pd_check_request(struct tcpm_port
> *port)
>  	return 0;
>  }
> 
> -#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)
> +static int tcpm_pd_select_pdo(struct tcpm_port *port)
>  {
> -	unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
> +	unsigned int i, max_mw = 0, max_mv = 0;
>  	int ret = -EINVAL;
> 
>  	/*
> -	 * Select the source PDO providing the most power which has a
> -	 * matchig sink cap.
> +	 * Select the source PDO providing the most power while staying within
> +	 * the board's voltage limits. Prefer PDO providing exp
>  	 */
>  	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) {
> -			for (j = 0; j < port->nr_fixed; j++) {
> -				if (pdo_fixed_voltage(pdo) ==
> -				    pdo_fixed_voltage(port->snk_pdo[j])) {
> -					ma = min_current(pdo, port->snk_pdo[j]);
> -					mv = pdo_fixed_voltage(pdo);
> -					mw = ma * mv / 1000;
> -					if (mw > max_mw ||
> -					    (mw == max_mw && mv > max_mv)) {
> -						ret = 0;
> -						*src_pdo = i;
> -						*sink_pdo = j;
> -						max_mw = mw;
> -						max_mv = mv;
> -					}
> -					/* 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])) {
> -					mw = min_power(pdo, port->snk_pdo[j]);
> -					mv = pdo_min_voltage(pdo);
> -					if (mw > max_mw ||
> -					    (mw == max_mw && mv > max_mv)) {
> -						ret = 0;
> -						*src_pdo = i;
> -						*sink_pdo = j;
> -						max_mw = mw;
> -						max_mv = mv;
> -					}
> -				}
> -			}
> -		} 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])) {
> -					ma = min_current(pdo, port->snk_pdo[j]);
> -					mv = pdo_min_voltage(pdo);
> -					mw = ma * mv / 1000;
> -					if (mw > max_mw ||
> -					    (mw == max_mw && mv > max_mv)) {
> -						ret = 0;
> -						*src_pdo = i;
> -						*sink_pdo = j;
> -						max_mw = mw;
> -						max_mv = mv;
> -					}
> -				}
> -			}
> +		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;
>  		}
>  	}
> 
> @@ -1881,14 +1827,13 @@ 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 src_pdo_index, snk_pdo_index;
> -	u32 pdo, matching_snk_pdo;
> +	int index;
> +	u32 pdo;
> 
> -	if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
> +	index = tcpm_pd_select_pdo(port);
> +	if (index < 0)
>  		return -EINVAL;
> -
> -	pdo = port->source_caps[src_pdo_index];
> -	matching_snk_pdo = port->snk_pdo[snk_pdo_index];
> +	pdo = port->source_caps[index];
>  	type = pdo_type(pdo);
> 
>  	if (type == PDO_TYPE_FIXED)
> @@ -1896,28 +1841,26 @@ static int tcpm_pd_build_request(struct tcpm_port
> *port, u32 *rdo)
>  	else
>  		mv = pdo_min_voltage(pdo);
> 
> -	/* Select maximum available current within the sink pdo's limit */
> +	/* Select maximum available current within the board's power limit */
>  	if (type == PDO_TYPE_BATT) {
> -		mw = min_power(pdo, matching_snk_pdo);
> -		ma = 1000 * mw / mv;
> +		mw = pdo_max_power(pdo);
> +		ma = 1000 * min(mw, port->max_snk_mw) / mv;
>  	} else {
> -		ma = min_current(pdo, matching_snk_pdo);
> -		mw = ma * mv / 1000;
> +		ma = min(pdo_max_current(pdo),
> +			 1000 * port->max_snk_mw / mv);
>  	}
> +	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;
> -		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);
> +		max_mw = port->operating_snk_mw;
> +		max_ma = max_mw * 1000 / mv;
>  	}
> 
>  	tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
> @@ -1926,16 +1869,16 @@ static int tcpm_pd_build_request(struct tcpm_port
> *port, u32 *rdo)
>  		 port->polarity);
> 
>  	if (type == PDO_TYPE_BATT) {
> -		*rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
> +		*rdo = RDO_BATT(index + 1, mw, max_mw, flags);
> 
>  		tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
> -			 src_pdo_index, mv, mw,
> +			 index, mv, mw,
>  			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>  	} else {
> -		*rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
> +		*rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
> 
>  		tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
> -			 src_pdo_index, mv, ma,
> +			 index, mv, ma,
>  			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>  	}
> 
> @@ -3667,19 +3610,6 @@ 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)
> -{
> -	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;
> @@ -3725,15 +3655,6 @@ 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);
> -	port->nr_var = nr_type_pdos(port->snk_pdo,
> -				    port->nr_snk_pdo,
> -				    PDO_TYPE_VAR);
> -	port->nr_batt = nr_type_pdos(port->snk_pdo,
> -				     port->nr_snk_pdo,
> -				     PDO_TYPE_BATT);
>  	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config-
> >snk_vdo,
>  					  tcpc->config->nr_snk_vdo);
> 
> --
> 2.14.3
> 
> --
> 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
---
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

[4.16,REGRESSION,fix] Revert "typec: tcpm: Only request matching pdos"

[Guenter Roeck]

On 03/06/2018 01:50 AM, Hans de Goede wrote:
> Commit 57e6f0d7b804 ("typec: tcpm: Only request matching pdos") is causing
> a regression, before this commit e.g. the GPD win and GPD pocket devices
> were charging at 9V 3A with a PD charger, now they are instead slowly
> discharging  at 5V 0.4A, as this commit causes the ports max_snk_mv/ma/mw
> settings to be completely ignored.
> 
> Arguably the way to fix this would be to add a PDO_VAR() describing the
> voltage range to the snk_caps of boards which can handle any voltage in
> their range, but the "typec: tcpm: Only request matching pdos" commit
> looks at the type of PDO advertised by the source/charger and if that
> is fixed (as it typically is) only compairs against PDO_FIXED entries
> in the snk_caps so supporting a range of voltage would require adding a
> PDO_FIXED entry for *every possible* voltage to snk_caps.
> 
> AFAICT there is no reason why a fixed source_cap cannot be matched against
> a variable snk_cap, so at a minimum the commit should be rewritten to
> support that.
> 
> For now lets revert the "typec: tcpm: Only request matching pdos" commit,
> fixing the regression.
> 
> Cc: Badhri Jagan Sridharan <badhri@google.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

> ---
>   drivers/usb/typec/tcpm.c | 163 ++++++++++++-----------------------------------
>   1 file changed, 42 insertions(+), 121 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index bfcaf6618a1f..735a6dd74c20 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -254,9 +254,6 @@ 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;
>   
> @@ -1786,90 +1783,39 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
>   	return 0;
>   }
>   
> -#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)
> +static int tcpm_pd_select_pdo(struct tcpm_port *port)
>   {
> -	unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
> +	unsigned int i, max_mw = 0, max_mv = 0;
>   	int ret = -EINVAL;
>   
>   	/*
> -	 * Select the source PDO providing the most power which has a
> -	 * matchig sink cap.
> +	 * Select the source PDO providing the most power while staying within
> +	 * the board's voltage limits. Prefer PDO providing exp
>   	 */
>   	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) {
> -			for (j = 0; j < port->nr_fixed; j++) {
> -				if (pdo_fixed_voltage(pdo) ==
> -				    pdo_fixed_voltage(port->snk_pdo[j])) {
> -					ma = min_current(pdo, port->snk_pdo[j]);
> -					mv = pdo_fixed_voltage(pdo);
> -					mw = ma * mv / 1000;
> -					if (mw > max_mw ||
> -					    (mw == max_mw && mv > max_mv)) {
> -						ret = 0;
> -						*src_pdo = i;
> -						*sink_pdo = j;
> -						max_mw = mw;
> -						max_mv = mv;
> -					}
> -					/* 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])) {
> -					mw = min_power(pdo, port->snk_pdo[j]);
> -					mv = pdo_min_voltage(pdo);
> -					if (mw > max_mw ||
> -					    (mw == max_mw && mv > max_mv)) {
> -						ret = 0;
> -						*src_pdo = i;
> -						*sink_pdo = j;
> -						max_mw = mw;
> -						max_mv = mv;
> -					}
> -				}
> -			}
> -		} 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])) {
> -					ma = min_current(pdo, port->snk_pdo[j]);
> -					mv = pdo_min_voltage(pdo);
> -					mw = ma * mv / 1000;
> -					if (mw > max_mw ||
> -					    (mw == max_mw && mv > max_mv)) {
> -						ret = 0;
> -						*src_pdo = i;
> -						*sink_pdo = j;
> -						max_mw = mw;
> -						max_mv = mv;
> -					}
> -				}
> -			}
> +		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;
>   		}
>   	}
>   
> @@ -1881,14 +1827,13 @@ 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 src_pdo_index, snk_pdo_index;
> -	u32 pdo, matching_snk_pdo;
> +	int index;
> +	u32 pdo;
>   
> -	if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
> +	index = tcpm_pd_select_pdo(port);
> +	if (index < 0)
>   		return -EINVAL;
> -
> -	pdo = port->source_caps[src_pdo_index];
> -	matching_snk_pdo = port->snk_pdo[snk_pdo_index];
> +	pdo = port->source_caps[index];
>   	type = pdo_type(pdo);
>   
>   	if (type == PDO_TYPE_FIXED)
> @@ -1896,28 +1841,26 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>   	else
>   		mv = pdo_min_voltage(pdo);
>   
> -	/* Select maximum available current within the sink pdo's limit */
> +	/* Select maximum available current within the board's power limit */
>   	if (type == PDO_TYPE_BATT) {
> -		mw = min_power(pdo, matching_snk_pdo);
> -		ma = 1000 * mw / mv;
> +		mw = pdo_max_power(pdo);
> +		ma = 1000 * min(mw, port->max_snk_mw) / mv;
>   	} else {
> -		ma = min_current(pdo, matching_snk_pdo);
> -		mw = ma * mv / 1000;
> +		ma = min(pdo_max_current(pdo),
> +			 1000 * port->max_snk_mw / mv);
>   	}
> +	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;
> -		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);
> +		max_mw = port->operating_snk_mw;
> +		max_ma = max_mw * 1000 / mv;
>   	}
>   
>   	tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
> @@ -1926,16 +1869,16 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>   		 port->polarity);
>   
>   	if (type == PDO_TYPE_BATT) {
> -		*rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
> +		*rdo = RDO_BATT(index + 1, mw, max_mw, flags);
>   
>   		tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
> -			 src_pdo_index, mv, mw,
> +			 index, mv, mw,
>   			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>   	} else {
> -		*rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
> +		*rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
>   
>   		tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
> -			 src_pdo_index, mv, ma,
> +			 index, mv, ma,
>   			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>   	}
>   
> @@ -3667,19 +3610,6 @@ 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)
> -{
> -	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;
> @@ -3725,15 +3655,6 @@ 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);
> -	port->nr_var = nr_type_pdos(port->snk_pdo,
> -				    port->nr_snk_pdo,
> -				    PDO_TYPE_VAR);
> -	port->nr_batt = nr_type_pdos(port->snk_pdo,
> -				     port->nr_snk_pdo,
> -				     PDO_TYPE_BATT);
>   	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config->snk_vdo,
>   					  tcpc->config->nr_snk_vdo);
>   
>
---
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

[4.16,REGRESSION,fix] Revert "typec: tcpm: Only request matching pdos"

[Hans de Goede]

Commit 57e6f0d7b804 ("typec: tcpm: Only request matching pdos") is causing
a regression, before this commit e.g. the GPD win and GPD pocket devices
were charging at 9V 3A with a PD charger, now they are instead slowly
discharging  at 5V 0.4A, as this commit causes the ports max_snk_mv/ma/mw
settings to be completely ignored.

Arguably the way to fix this would be to add a PDO_VAR() describing the
voltage range to the snk_caps of boards which can handle any voltage in
their range, but the "typec: tcpm: Only request matching pdos" commit
looks at the type of PDO advertised by the source/charger and if that
is fixed (as it typically is) only compairs against PDO_FIXED entries
in the snk_caps so supporting a range of voltage would require adding a
PDO_FIXED entry for *every possible* voltage to snk_caps.

AFAICT there is no reason why a fixed source_cap cannot be matched against
a variable snk_cap, so at a minimum the commit should be rewritten to
support that.

For now lets revert the "typec: tcpm: Only request matching pdos" commit,
fixing the regression.

Cc: Badhri Jagan Sridharan <badhri@google.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm.c | 163 ++++++++++++-----------------------------------
 1 file changed, 42 insertions(+), 121 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index bfcaf6618a1f..735a6dd74c20 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -254,9 +254,6 @@ 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;
 
@@ -1786,90 +1783,39 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
 	return 0;
 }
 
-#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)
+static int tcpm_pd_select_pdo(struct tcpm_port *port)
 {
-	unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
+	unsigned int i, max_mw = 0, max_mv = 0;
 	int ret = -EINVAL;
 
 	/*
-	 * Select the source PDO providing the most power which has a
-	 * matchig sink cap.
+	 * Select the source PDO providing the most power while staying within
+	 * the board's voltage limits. Prefer PDO providing exp
 	 */
 	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) {
-			for (j = 0; j < port->nr_fixed; j++) {
-				if (pdo_fixed_voltage(pdo) ==
-				    pdo_fixed_voltage(port->snk_pdo[j])) {
-					ma = min_current(pdo, port->snk_pdo[j]);
-					mv = pdo_fixed_voltage(pdo);
-					mw = ma * mv / 1000;
-					if (mw > max_mw ||
-					    (mw == max_mw && mv > max_mv)) {
-						ret = 0;
-						*src_pdo = i;
-						*sink_pdo = j;
-						max_mw = mw;
-						max_mv = mv;
-					}
-					/* 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])) {
-					mw = min_power(pdo, port->snk_pdo[j]);
-					mv = pdo_min_voltage(pdo);
-					if (mw > max_mw ||
-					    (mw == max_mw && mv > max_mv)) {
-						ret = 0;
-						*src_pdo = i;
-						*sink_pdo = j;
-						max_mw = mw;
-						max_mv = mv;
-					}
-				}
-			}
-		} 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])) {
-					ma = min_current(pdo, port->snk_pdo[j]);
-					mv = pdo_min_voltage(pdo);
-					mw = ma * mv / 1000;
-					if (mw > max_mw ||
-					    (mw == max_mw && mv > max_mv)) {
-						ret = 0;
-						*src_pdo = i;
-						*sink_pdo = j;
-						max_mw = mw;
-						max_mv = mv;
-					}
-				}
-			}
+		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;
 		}
 	}
 
@@ -1881,14 +1827,13 @@ 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 src_pdo_index, snk_pdo_index;
-	u32 pdo, matching_snk_pdo;
+	int index;
+	u32 pdo;
 
-	if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
+	index = tcpm_pd_select_pdo(port);
+	if (index < 0)
 		return -EINVAL;
-
-	pdo = port->source_caps[src_pdo_index];
-	matching_snk_pdo = port->snk_pdo[snk_pdo_index];
+	pdo = port->source_caps[index];
 	type = pdo_type(pdo);
 
 	if (type == PDO_TYPE_FIXED)
@@ -1896,28 +1841,26 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
 	else
 		mv = pdo_min_voltage(pdo);
 
-	/* Select maximum available current within the sink pdo's limit */
+	/* Select maximum available current within the board's power limit */
 	if (type == PDO_TYPE_BATT) {
-		mw = min_power(pdo, matching_snk_pdo);
-		ma = 1000 * mw / mv;
+		mw = pdo_max_power(pdo);
+		ma = 1000 * min(mw, port->max_snk_mw) / mv;
 	} else {
-		ma = min_current(pdo, matching_snk_pdo);
-		mw = ma * mv / 1000;
+		ma = min(pdo_max_current(pdo),
+			 1000 * port->max_snk_mw / mv);
 	}
+	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;
-		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);
+		max_mw = port->operating_snk_mw;
+		max_ma = max_mw * 1000 / mv;
 	}
 
 	tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
@@ -1926,16 +1869,16 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
 		 port->polarity);
 
 	if (type == PDO_TYPE_BATT) {
-		*rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
+		*rdo = RDO_BATT(index + 1, mw, max_mw, flags);
 
 		tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
-			 src_pdo_index, mv, mw,
+			 index, mv, mw,
 			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
 	} else {
-		*rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
+		*rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
 
 		tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
-			 src_pdo_index, mv, ma,
+			 index, mv, ma,
 			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
 	}
 
@@ -3667,19 +3610,6 @@ 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)
-{
-	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;
@@ -3725,15 +3655,6 @@ 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);
-	port->nr_var = nr_type_pdos(port->snk_pdo,
-				    port->nr_snk_pdo,
-				    PDO_TYPE_VAR);
-	port->nr_batt = nr_type_pdos(port->snk_pdo,
-				     port->nr_snk_pdo,
-				     PDO_TYPE_BATT);
 	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config->snk_vdo,
 					  tcpc->config->nr_snk_vdo);