[PATCH v2 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply

[PATCH v2 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply

[Badhri Jagan Sridharan]

tcpm_pd_build_request overwrites current_limit and supply_voltage
even before port partner accepts the requests. This leaves stale
values in current_limit and supply_voltage that get exported by
"tcpm-source-psy-". Solving this problem by caching the request
values of current limit/supply voltage in req_current_limit
and req_supply_voltage. current_limit/supply_voltage gets updated
once the port partner accepts the request.

Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
---
Changes since V1:
* Fixed typo as suggested by Guenter Roeck.
* Added Reviewed-by tags.
---
 drivers/usb/typec/tcpm/tcpm.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index ca1fc77697fc..4ea4b30ae885 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -389,7 +389,10 @@ struct tcpm_port {
 	unsigned int operating_snk_mw;
 	bool update_sink_caps;
 
-	/* Requested current / voltage */
+	/* Requested current / voltage to the port partner */
+	u32 req_current_limit;
+	u32 req_supply_voltage;
+	/* Actual current / voltage limit of the local port */
 	u32 current_limit;
 	u32 supply_voltage;
 
@@ -2435,8 +2438,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
 		case SNK_TRANSITION_SINK:
 			if (port->vbus_present) {
 				tcpm_set_current_limit(port,
-						       port->current_limit,
-						       port->supply_voltage);
+						       port->req_current_limit,
+						       port->req_supply_voltage);
 				port->explicit_contract = true;
 				tcpm_set_auto_vbus_discharge_threshold(port,
 								       TYPEC_PWR_MODE_PD,
@@ -2545,8 +2548,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
 			break;
 		case SNK_NEGOTIATE_PPS_CAPABILITIES:
 			port->pps_data.active = true;
-			port->supply_voltage = port->pps_data.out_volt;
-			port->current_limit = port->pps_data.op_curr;
+			port->req_supply_voltage = port->pps_data.out_volt;
+			port->req_current_limit = port->pps_data.op_curr;
 			tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
 			break;
 		case SOFT_RESET_SEND:
@@ -3195,8 +3198,8 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
 			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
 	}
 
-	port->current_limit = ma;
-	port->supply_voltage = mv;
+	port->req_current_limit = ma;
+	port->req_supply_voltage = mv;
 
 	return 0;
 }
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH v2 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply

[Badhri Jagan Sridharan]

tcpm_pd_select_pps_apdo overwrites port->pps_data.min_volt,
port->pps_data.max_volt, port->pps_data.max_curr even before
port partner accepts the requests. This leaves incorrect values
in current_limit and supply_voltage that get exported by
"tcpm-source-psy-". Solving this problem by caching the request
values in req_min_volt, req_max_volt, req_max_curr, req_out_volt,
req_op_curr. min_volt, max_volt, max_curr gets updated once the
partner accepts the request. current_limit, supply_voltage gets updated
once local port's tcpm enters SNK_TRANSITION_SINK when the accepted
current_limit and supply_voltage is enforced.

Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
---
Changes since V1:
* Moved to kerneldoc header as suggested by Greg KH.
* Added reviewed by tags.
---
 drivers/usb/typec/tcpm/tcpm.c | 88 +++++++++++++++++++++--------------
 1 file changed, 53 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 4ea4b30ae885..b4a40099d7e9 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -268,12 +268,27 @@ struct pd_mode_data {
 	struct typec_altmode_desc altmode_desc[ALTMODE_DISCOVERY_MAX];
 };
 
+/*
+ * @min_volt: Actual min voltage at the local port
+ * @req_min_volt: Requested min voltage to the port partner
+ * @max_volt: Actual max voltage at the local port
+ * @req_max_volt: Requested max voltage to the port partner
+ * @max_curr: Actual max current at the local port
+ * @req_max_curr: Requested max current of the port partner
+ * @req_out_volt: Requested output voltage to the port partner
+ * @req_op_curr: Requested operating current to the port partner
+ * @supported: Parter has atleast one APDO hence supports PPS
+ * @active: PPS mode is active
+ */
 struct pd_pps_data {
 	u32 min_volt;
+	u32 req_min_volt;
 	u32 max_volt;
+	u32 req_max_volt;
 	u32 max_curr;
-	u32 out_volt;
-	u32 op_curr;
+	u32 req_max_curr;
+	u32 req_out_volt;
+	u32 req_op_curr;
 	bool supported;
 	bool active;
 };
@@ -2498,8 +2513,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
 			break;
 		case SNK_NEGOTIATE_PPS_CAPABILITIES:
 			/* Revert data back from any requested PPS updates */
-			port->pps_data.out_volt = port->supply_voltage;
-			port->pps_data.op_curr = port->current_limit;
+			port->pps_data.req_out_volt = port->supply_voltage;
+			port->pps_data.req_op_curr = port->current_limit;
 			port->pps_status = (type == PD_CTRL_WAIT ?
 					    -EAGAIN : -EOPNOTSUPP);
 
@@ -2548,8 +2563,11 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
 			break;
 		case SNK_NEGOTIATE_PPS_CAPABILITIES:
 			port->pps_data.active = true;
-			port->req_supply_voltage = port->pps_data.out_volt;
-			port->req_current_limit = port->pps_data.op_curr;
+			port->pps_data.min_volt = port->pps_data.req_min_volt;
+			port->pps_data.max_volt = port->pps_data.req_max_volt;
+			port->pps_data.max_curr = port->pps_data.req_max_curr;
+			port->req_supply_voltage = port->pps_data.req_out_volt;
+			port->req_current_limit = port->pps_data.req_op_curr;
 			tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
 			break;
 		case SOFT_RESET_SEND:
@@ -3108,16 +3126,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
 		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(port->pps_data.max_volt,
-					      max(port->pps_data.min_volt,
-						  port->pps_data.out_volt));
-		port->pps_data.op_curr = min(port->pps_data.max_curr,
-					     port->pps_data.op_curr);
+		port->pps_data.req_min_volt = max(pdo_pps_apdo_min_voltage(src),
+						  pdo_pps_apdo_min_voltage(snk));
+		port->pps_data.req_max_volt = min(pdo_pps_apdo_max_voltage(src),
+						  pdo_pps_apdo_max_voltage(snk));
+		port->pps_data.req_max_curr = min_pps_apdo_current(src, snk);
+		port->pps_data.req_out_volt = min(port->pps_data.max_volt,
+						  max(port->pps_data.min_volt,
+						      port->pps_data.req_out_volt));
+		port->pps_data.req_op_curr = min(port->pps_data.max_curr,
+						 port->pps_data.req_op_curr);
 		power_supply_changed(port->psy);
 	}
 
@@ -3245,10 +3263,10 @@ static int tcpm_pd_build_pps_request(struct tcpm_port *port, u32 *rdo)
 			tcpm_log(port, "Invalid APDO selected!");
 			return -EINVAL;
 		}
-		max_mv = port->pps_data.max_volt;
-		max_ma = port->pps_data.max_curr;
-		out_mv = port->pps_data.out_volt;
-		op_ma = port->pps_data.op_curr;
+		max_mv = port->pps_data.req_max_volt;
+		max_ma = port->pps_data.req_max_curr;
+		out_mv = port->pps_data.req_out_volt;
+		op_ma = port->pps_data.req_op_curr;
 		break;
 	default:
 		tcpm_log(port, "Invalid PDO selected!");
@@ -3295,8 +3313,8 @@ static int tcpm_pd_build_pps_request(struct tcpm_port *port, u32 *rdo)
 	tcpm_log(port, "Requesting APDO %d: %u mV, %u mA",
 		 src_pdo_index, out_mv, op_ma);
 
-	port->pps_data.op_curr = op_ma;
-	port->pps_data.out_volt = out_mv;
+	port->pps_data.req_op_curr = op_ma;
+	port->pps_data.req_out_volt = out_mv;
 
 	return 0;
 }
@@ -5429,7 +5447,7 @@ static int tcpm_try_role(struct typec_port *p, int role)
 	return ret;
 }
 
-static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
+static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 req_op_curr)
 {
 	unsigned int target_mw;
 	int ret;
@@ -5447,12 +5465,12 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
 		goto port_unlock;
 	}
 
-	if (op_curr > port->pps_data.max_curr) {
+	if (req_op_curr > port->pps_data.max_curr) {
 		ret = -EINVAL;
 		goto port_unlock;
 	}
 
-	target_mw = (op_curr * port->pps_data.out_volt) / 1000;
+	target_mw = (req_op_curr * port->supply_voltage) / 1000;
 	if (target_mw < port->operating_snk_mw) {
 		ret = -EINVAL;
 		goto port_unlock;
@@ -5466,10 +5484,10 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
 	}
 
 	/* Round down operating current to align with PPS valid steps */
-	op_curr = op_curr - (op_curr % RDO_PROG_CURR_MA_STEP);
+	req_op_curr = req_op_curr - (req_op_curr % RDO_PROG_CURR_MA_STEP);
 
 	reinit_completion(&port->pps_complete);
-	port->pps_data.op_curr = op_curr;
+	port->pps_data.req_op_curr = req_op_curr;
 	port->pps_status = 0;
 	port->pps_pending = true;
 	mutex_unlock(&port->lock);
@@ -5490,7 +5508,7 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
 	return ret;
 }
 
-static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
+static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 req_out_volt)
 {
 	unsigned int target_mw;
 	int ret;
@@ -5508,13 +5526,13 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
 		goto port_unlock;
 	}
 
-	if (out_volt < port->pps_data.min_volt ||
-	    out_volt > port->pps_data.max_volt) {
+	if (req_out_volt < port->pps_data.min_volt ||
+	    req_out_volt > port->pps_data.max_volt) {
 		ret = -EINVAL;
 		goto port_unlock;
 	}
 
-	target_mw = (port->pps_data.op_curr * out_volt) / 1000;
+	target_mw = (port->current_limit * req_out_volt) / 1000;
 	if (target_mw < port->operating_snk_mw) {
 		ret = -EINVAL;
 		goto port_unlock;
@@ -5528,10 +5546,10 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
 	}
 
 	/* Round down output voltage to align with PPS valid steps */
-	out_volt = out_volt - (out_volt % RDO_PROG_VOLT_MV_STEP);
+	req_out_volt = req_out_volt - (req_out_volt % RDO_PROG_VOLT_MV_STEP);
 
 	reinit_completion(&port->pps_complete);
-	port->pps_data.out_volt = out_volt;
+	port->pps_data.req_out_volt = req_out_volt;
 	port->pps_status = 0;
 	port->pps_pending = true;
 	mutex_unlock(&port->lock);
@@ -5589,8 +5607,8 @@ static int tcpm_pps_activate(struct tcpm_port *port, bool activate)
 
 	/* Trigger PPS request or move back to standard PDO contract */
 	if (activate) {
-		port->pps_data.out_volt = port->supply_voltage;
-		port->pps_data.op_curr = port->current_limit;
+		port->pps_data.req_out_volt = port->supply_voltage;
+		port->pps_data.req_op_curr = port->current_limit;
 	}
 	mutex_unlock(&port->lock);
 
-- 
2.31.1.295.g9ea45b61b8-goog

Re: [PATCH v2 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply

[Heikki Krogerus]

On Wed, Apr 07, 2021 at 01:07:19PM -0700, Badhri Jagan Sridharan wrote:
> tcpm_pd_select_pps_apdo overwrites port->pps_data.min_volt,
> port->pps_data.max_volt, port->pps_data.max_curr even before
> port partner accepts the requests. This leaves incorrect values
> in current_limit and supply_voltage that get exported by
> "tcpm-source-psy-". Solving this problem by caching the request
> values in req_min_volt, req_max_volt, req_max_curr, req_out_volt,
> req_op_curr. min_volt, max_volt, max_curr gets updated once the
> partner accepts the request. current_limit, supply_voltage gets updated
> once local port's tcpm enters SNK_TRANSITION_SINK when the accepted
> current_limit and supply_voltage is enforced.
> 
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

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

> ---
> Changes since V1:
> * Moved to kerneldoc header as suggested by Greg KH.
> * Added reviewed by tags.
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 88 +++++++++++++++++++++--------------
>  1 file changed, 53 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 4ea4b30ae885..b4a40099d7e9 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -268,12 +268,27 @@ struct pd_mode_data {
>  	struct typec_altmode_desc altmode_desc[ALTMODE_DISCOVERY_MAX];
>  };
>  
> +/*
> + * @min_volt: Actual min voltage at the local port
> + * @req_min_volt: Requested min voltage to the port partner
> + * @max_volt: Actual max voltage at the local port
> + * @req_max_volt: Requested max voltage to the port partner
> + * @max_curr: Actual max current at the local port
> + * @req_max_curr: Requested max current of the port partner
> + * @req_out_volt: Requested output voltage to the port partner
> + * @req_op_curr: Requested operating current to the port partner
> + * @supported: Parter has atleast one APDO hence supports PPS
> + * @active: PPS mode is active
> + */
>  struct pd_pps_data {
>  	u32 min_volt;
> +	u32 req_min_volt;
>  	u32 max_volt;
> +	u32 req_max_volt;
>  	u32 max_curr;
> -	u32 out_volt;
> -	u32 op_curr;
> +	u32 req_max_curr;
> +	u32 req_out_volt;
> +	u32 req_op_curr;
>  	bool supported;
>  	bool active;
>  };
> @@ -2498,8 +2513,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
>  			break;
>  		case SNK_NEGOTIATE_PPS_CAPABILITIES:
>  			/* Revert data back from any requested PPS updates */
> -			port->pps_data.out_volt = port->supply_voltage;
> -			port->pps_data.op_curr = port->current_limit;
> +			port->pps_data.req_out_volt = port->supply_voltage;
> +			port->pps_data.req_op_curr = port->current_limit;
>  			port->pps_status = (type == PD_CTRL_WAIT ?
>  					    -EAGAIN : -EOPNOTSUPP);
>  
> @@ -2548,8 +2563,11 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
>  			break;
>  		case SNK_NEGOTIATE_PPS_CAPABILITIES:
>  			port->pps_data.active = true;
> -			port->req_supply_voltage = port->pps_data.out_volt;
> -			port->req_current_limit = port->pps_data.op_curr;
> +			port->pps_data.min_volt = port->pps_data.req_min_volt;
> +			port->pps_data.max_volt = port->pps_data.req_max_volt;
> +			port->pps_data.max_curr = port->pps_data.req_max_curr;
> +			port->req_supply_voltage = port->pps_data.req_out_volt;
> +			port->req_current_limit = port->pps_data.req_op_curr;
>  			tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
>  			break;
>  		case SOFT_RESET_SEND:
> @@ -3108,16 +3126,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
>  		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(port->pps_data.max_volt,
> -					      max(port->pps_data.min_volt,
> -						  port->pps_data.out_volt));
> -		port->pps_data.op_curr = min(port->pps_data.max_curr,
> -					     port->pps_data.op_curr);
> +		port->pps_data.req_min_volt = max(pdo_pps_apdo_min_voltage(src),
> +						  pdo_pps_apdo_min_voltage(snk));
> +		port->pps_data.req_max_volt = min(pdo_pps_apdo_max_voltage(src),
> +						  pdo_pps_apdo_max_voltage(snk));
> +		port->pps_data.req_max_curr = min_pps_apdo_current(src, snk);
> +		port->pps_data.req_out_volt = min(port->pps_data.max_volt,
> +						  max(port->pps_data.min_volt,
> +						      port->pps_data.req_out_volt));
> +		port->pps_data.req_op_curr = min(port->pps_data.max_curr,
> +						 port->pps_data.req_op_curr);
>  		power_supply_changed(port->psy);
>  	}
>  
> @@ -3245,10 +3263,10 @@ static int tcpm_pd_build_pps_request(struct tcpm_port *port, u32 *rdo)
>  			tcpm_log(port, "Invalid APDO selected!");
>  			return -EINVAL;
>  		}
> -		max_mv = port->pps_data.max_volt;
> -		max_ma = port->pps_data.max_curr;
> -		out_mv = port->pps_data.out_volt;
> -		op_ma = port->pps_data.op_curr;
> +		max_mv = port->pps_data.req_max_volt;
> +		max_ma = port->pps_data.req_max_curr;
> +		out_mv = port->pps_data.req_out_volt;
> +		op_ma = port->pps_data.req_op_curr;
>  		break;
>  	default:
>  		tcpm_log(port, "Invalid PDO selected!");
> @@ -3295,8 +3313,8 @@ static int tcpm_pd_build_pps_request(struct tcpm_port *port, u32 *rdo)
>  	tcpm_log(port, "Requesting APDO %d: %u mV, %u mA",
>  		 src_pdo_index, out_mv, op_ma);
>  
> -	port->pps_data.op_curr = op_ma;
> -	port->pps_data.out_volt = out_mv;
> +	port->pps_data.req_op_curr = op_ma;
> +	port->pps_data.req_out_volt = out_mv;
>  
>  	return 0;
>  }
> @@ -5429,7 +5447,7 @@ static int tcpm_try_role(struct typec_port *p, int role)
>  	return ret;
>  }
>  
> -static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
> +static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 req_op_curr)
>  {
>  	unsigned int target_mw;
>  	int ret;
> @@ -5447,12 +5465,12 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
>  		goto port_unlock;
>  	}
>  
> -	if (op_curr > port->pps_data.max_curr) {
> +	if (req_op_curr > port->pps_data.max_curr) {
>  		ret = -EINVAL;
>  		goto port_unlock;
>  	}
>  
> -	target_mw = (op_curr * port->pps_data.out_volt) / 1000;
> +	target_mw = (req_op_curr * port->supply_voltage) / 1000;
>  	if (target_mw < port->operating_snk_mw) {
>  		ret = -EINVAL;
>  		goto port_unlock;
> @@ -5466,10 +5484,10 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
>  	}
>  
>  	/* Round down operating current to align with PPS valid steps */
> -	op_curr = op_curr - (op_curr % RDO_PROG_CURR_MA_STEP);
> +	req_op_curr = req_op_curr - (req_op_curr % RDO_PROG_CURR_MA_STEP);
>  
>  	reinit_completion(&port->pps_complete);
> -	port->pps_data.op_curr = op_curr;
> +	port->pps_data.req_op_curr = req_op_curr;
>  	port->pps_status = 0;
>  	port->pps_pending = true;
>  	mutex_unlock(&port->lock);
> @@ -5490,7 +5508,7 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
>  	return ret;
>  }
>  
> -static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
> +static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 req_out_volt)
>  {
>  	unsigned int target_mw;
>  	int ret;
> @@ -5508,13 +5526,13 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
>  		goto port_unlock;
>  	}
>  
> -	if (out_volt < port->pps_data.min_volt ||
> -	    out_volt > port->pps_data.max_volt) {
> +	if (req_out_volt < port->pps_data.min_volt ||
> +	    req_out_volt > port->pps_data.max_volt) {
>  		ret = -EINVAL;
>  		goto port_unlock;
>  	}
>  
> -	target_mw = (port->pps_data.op_curr * out_volt) / 1000;
> +	target_mw = (port->current_limit * req_out_volt) / 1000;
>  	if (target_mw < port->operating_snk_mw) {
>  		ret = -EINVAL;
>  		goto port_unlock;
> @@ -5528,10 +5546,10 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
>  	}
>  
>  	/* Round down output voltage to align with PPS valid steps */
> -	out_volt = out_volt - (out_volt % RDO_PROG_VOLT_MV_STEP);
> +	req_out_volt = req_out_volt - (req_out_volt % RDO_PROG_VOLT_MV_STEP);
>  
>  	reinit_completion(&port->pps_complete);
> -	port->pps_data.out_volt = out_volt;
> +	port->pps_data.req_out_volt = req_out_volt;
>  	port->pps_status = 0;
>  	port->pps_pending = true;
>  	mutex_unlock(&port->lock);
> @@ -5589,8 +5607,8 @@ static int tcpm_pps_activate(struct tcpm_port *port, bool activate)
>  
>  	/* Trigger PPS request or move back to standard PDO contract */
>  	if (activate) {
> -		port->pps_data.out_volt = port->supply_voltage;
> -		port->pps_data.op_curr = port->current_limit;
> +		port->pps_data.req_out_volt = port->supply_voltage;
> +		port->pps_data.req_op_curr = port->current_limit;
>  	}
>  	mutex_unlock(&port->lock);
>  
> -- 
> 2.31.1.295.g9ea45b61b8-goog

-- 
heikki

Re: [PATCH v2 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply

[Guenter Roeck]

On 4/7/21 1:07 PM, Badhri Jagan Sridharan wrote:
> tcpm_pd_select_pps_apdo overwrites port->pps_data.min_volt,
> port->pps_data.max_volt, port->pps_data.max_curr even before
> port partner accepts the requests. This leaves incorrect values
> in current_limit and supply_voltage that get exported by
> "tcpm-source-psy-". Solving this problem by caching the request
> values in req_min_volt, req_max_volt, req_max_curr, req_out_volt,
> req_op_curr. min_volt, max_volt, max_curr gets updated once the
> partner accepts the request. current_limit, supply_voltage gets updated
> once local port's tcpm enters SNK_TRANSITION_SINK when the accepted
> current_limit and supply_voltage is enforced.
> 
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

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

> ---
> Changes since V1:
> * Moved to kerneldoc header as suggested by Greg KH.
> * Added reviewed by tags.
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 88 +++++++++++++++++++++--------------
>  1 file changed, 53 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 4ea4b30ae885..b4a40099d7e9 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -268,12 +268,27 @@ struct pd_mode_data {
>  	struct typec_altmode_desc altmode_desc[ALTMODE_DISCOVERY_MAX];
>  };
>  
> +/*
> + * @min_volt: Actual min voltage at the local port
> + * @req_min_volt: Requested min voltage to the port partner
> + * @max_volt: Actual max voltage at the local port
> + * @req_max_volt: Requested max voltage to the port partner
> + * @max_curr: Actual max current at the local port
> + * @req_max_curr: Requested max current of the port partner
> + * @req_out_volt: Requested output voltage to the port partner
> + * @req_op_curr: Requested operating current to the port partner
> + * @supported: Parter has atleast one APDO hence supports PPS
> + * @active: PPS mode is active
> + */
>  struct pd_pps_data {
>  	u32 min_volt;
> +	u32 req_min_volt;
>  	u32 max_volt;
> +	u32 req_max_volt;
>  	u32 max_curr;
> -	u32 out_volt;
> -	u32 op_curr;
> +	u32 req_max_curr;
> +	u32 req_out_volt;
> +	u32 req_op_curr;
>  	bool supported;
>  	bool active;
>  };
> @@ -2498,8 +2513,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
>  			break;
>  		case SNK_NEGOTIATE_PPS_CAPABILITIES:
>  			/* Revert data back from any requested PPS updates */
> -			port->pps_data.out_volt = port->supply_voltage;
> -			port->pps_data.op_curr = port->current_limit;
> +			port->pps_data.req_out_volt = port->supply_voltage;
> +			port->pps_data.req_op_curr = port->current_limit;
>  			port->pps_status = (type == PD_CTRL_WAIT ?
>  					    -EAGAIN : -EOPNOTSUPP);
>  
> @@ -2548,8 +2563,11 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
>  			break;
>  		case SNK_NEGOTIATE_PPS_CAPABILITIES:
>  			port->pps_data.active = true;
> -			port->req_supply_voltage = port->pps_data.out_volt;
> -			port->req_current_limit = port->pps_data.op_curr;
> +			port->pps_data.min_volt = port->pps_data.req_min_volt;
> +			port->pps_data.max_volt = port->pps_data.req_max_volt;
> +			port->pps_data.max_curr = port->pps_data.req_max_curr;
> +			port->req_supply_voltage = port->pps_data.req_out_volt;
> +			port->req_current_limit = port->pps_data.req_op_curr;
>  			tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
>  			break;
>  		case SOFT_RESET_SEND:
> @@ -3108,16 +3126,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
>  		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(port->pps_data.max_volt,
> -					      max(port->pps_data.min_volt,
> -						  port->pps_data.out_volt));
> -		port->pps_data.op_curr = min(port->pps_data.max_curr,
> -					     port->pps_data.op_curr);
> +		port->pps_data.req_min_volt = max(pdo_pps_apdo_min_voltage(src),
> +						  pdo_pps_apdo_min_voltage(snk));
> +		port->pps_data.req_max_volt = min(pdo_pps_apdo_max_voltage(src),
> +						  pdo_pps_apdo_max_voltage(snk));
> +		port->pps_data.req_max_curr = min_pps_apdo_current(src, snk);
> +		port->pps_data.req_out_volt = min(port->pps_data.max_volt,
> +						  max(port->pps_data.min_volt,
> +						      port->pps_data.req_out_volt));
> +		port->pps_data.req_op_curr = min(port->pps_data.max_curr,
> +						 port->pps_data.req_op_curr);
>  		power_supply_changed(port->psy);
>  	}
>  
> @@ -3245,10 +3263,10 @@ static int tcpm_pd_build_pps_request(struct tcpm_port *port, u32 *rdo)
>  			tcpm_log(port, "Invalid APDO selected!");
>  			return -EINVAL;
>  		}
> -		max_mv = port->pps_data.max_volt;
> -		max_ma = port->pps_data.max_curr;
> -		out_mv = port->pps_data.out_volt;
> -		op_ma = port->pps_data.op_curr;
> +		max_mv = port->pps_data.req_max_volt;
> +		max_ma = port->pps_data.req_max_curr;
> +		out_mv = port->pps_data.req_out_volt;
> +		op_ma = port->pps_data.req_op_curr;
>  		break;
>  	default:
>  		tcpm_log(port, "Invalid PDO selected!");
> @@ -3295,8 +3313,8 @@ static int tcpm_pd_build_pps_request(struct tcpm_port *port, u32 *rdo)
>  	tcpm_log(port, "Requesting APDO %d: %u mV, %u mA",
>  		 src_pdo_index, out_mv, op_ma);
>  
> -	port->pps_data.op_curr = op_ma;
> -	port->pps_data.out_volt = out_mv;
> +	port->pps_data.req_op_curr = op_ma;
> +	port->pps_data.req_out_volt = out_mv;
>  
>  	return 0;
>  }
> @@ -5429,7 +5447,7 @@ static int tcpm_try_role(struct typec_port *p, int role)
>  	return ret;
>  }
>  
> -static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
> +static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 req_op_curr)
>  {
>  	unsigned int target_mw;
>  	int ret;
> @@ -5447,12 +5465,12 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
>  		goto port_unlock;
>  	}
>  
> -	if (op_curr > port->pps_data.max_curr) {
> +	if (req_op_curr > port->pps_data.max_curr) {
>  		ret = -EINVAL;
>  		goto port_unlock;
>  	}
>  
> -	target_mw = (op_curr * port->pps_data.out_volt) / 1000;
> +	target_mw = (req_op_curr * port->supply_voltage) / 1000;
>  	if (target_mw < port->operating_snk_mw) {
>  		ret = -EINVAL;
>  		goto port_unlock;
> @@ -5466,10 +5484,10 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
>  	}
>  
>  	/* Round down operating current to align with PPS valid steps */
> -	op_curr = op_curr - (op_curr % RDO_PROG_CURR_MA_STEP);
> +	req_op_curr = req_op_curr - (req_op_curr % RDO_PROG_CURR_MA_STEP);
>  
>  	reinit_completion(&port->pps_complete);
> -	port->pps_data.op_curr = op_curr;
> +	port->pps_data.req_op_curr = req_op_curr;
>  	port->pps_status = 0;
>  	port->pps_pending = true;
>  	mutex_unlock(&port->lock);
> @@ -5490,7 +5508,7 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
>  	return ret;
>  }
>  
> -static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
> +static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 req_out_volt)
>  {
>  	unsigned int target_mw;
>  	int ret;
> @@ -5508,13 +5526,13 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
>  		goto port_unlock;
>  	}
>  
> -	if (out_volt < port->pps_data.min_volt ||
> -	    out_volt > port->pps_data.max_volt) {
> +	if (req_out_volt < port->pps_data.min_volt ||
> +	    req_out_volt > port->pps_data.max_volt) {
>  		ret = -EINVAL;
>  		goto port_unlock;
>  	}
>  
> -	target_mw = (port->pps_data.op_curr * out_volt) / 1000;
> +	target_mw = (port->current_limit * req_out_volt) / 1000;
>  	if (target_mw < port->operating_snk_mw) {
>  		ret = -EINVAL;
>  		goto port_unlock;
> @@ -5528,10 +5546,10 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
>  	}
>  
>  	/* Round down output voltage to align with PPS valid steps */
> -	out_volt = out_volt - (out_volt % RDO_PROG_VOLT_MV_STEP);
> +	req_out_volt = req_out_volt - (req_out_volt % RDO_PROG_VOLT_MV_STEP);
>  
>  	reinit_completion(&port->pps_complete);
> -	port->pps_data.out_volt = out_volt;
> +	port->pps_data.req_out_volt = req_out_volt;
>  	port->pps_status = 0;
>  	port->pps_pending = true;
>  	mutex_unlock(&port->lock);
> @@ -5589,8 +5607,8 @@ static int tcpm_pps_activate(struct tcpm_port *port, bool activate)
>  
>  	/* Trigger PPS request or move back to standard PDO contract */
>  	if (activate) {
> -		port->pps_data.out_volt = port->supply_voltage;
> -		port->pps_data.op_curr = port->current_limit;
> +		port->pps_data.req_out_volt = port->supply_voltage;
> +		port->pps_data.req_op_curr = port->current_limit;
>  	}
>  	mutex_unlock(&port->lock);
>  
> 

[PATCH v2 3/6] usb: typec: tcpm: update power supply once partner accepts

[Badhri Jagan Sridharan]

power_supply_changed needs to be called to notify clients
after the partner accepts the requested values for the pps
case.

Also, remove the redundant power_supply_changed at the end
of the tcpm_reset_port as power_supply_changed is already
called right after usb_type is changed.

Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
---
Changes since V1:
* Updated commit description to clarify Guenter Roeck's concern.
* Added Reviewed-by tags
---
 drivers/usb/typec/tcpm/tcpm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index b4a40099d7e9..d1d03ee90d8f 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -2568,6 +2568,7 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
 			port->pps_data.max_curr = port->pps_data.req_max_curr;
 			port->req_supply_voltage = port->pps_data.req_out_volt;
 			port->req_current_limit = port->pps_data.req_op_curr;
+			power_supply_changed(port->psy);
 			tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
 			break;
 		case SOFT_RESET_SEND:
@@ -3136,7 +3137,6 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
 						      port->pps_data.req_out_volt));
 		port->pps_data.req_op_curr = min(port->pps_data.max_curr,
 						 port->pps_data.req_op_curr);
-		power_supply_changed(port->psy);
 	}
 
 	return src_pdo;
@@ -3561,8 +3561,6 @@ static void tcpm_reset_port(struct tcpm_port *port)
 	port->sink_cap_done = false;
 	if (port->tcpc->enable_frs)
 		port->tcpc->enable_frs(port->tcpc, false);
-
-	power_supply_changed(port->psy);
 }
 
 static void tcpm_detach(struct tcpm_port *port)
-- 
2.31.1.295.g9ea45b61b8-goog

Re: [PATCH v2 3/6] usb: typec: tcpm: update power supply once partner accepts

[Heikki Krogerus]

On Wed, Apr 07, 2021 at 01:07:20PM -0700, Badhri Jagan Sridharan wrote:
> power_supply_changed needs to be called to notify clients
> after the partner accepts the requested values for the pps
> case.
> 
> Also, remove the redundant power_supply_changed at the end
> of the tcpm_reset_port as power_supply_changed is already
> called right after usb_type is changed.
> 
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

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

> ---
> Changes since V1:
> * Updated commit description to clarify Guenter Roeck's concern.
> * Added Reviewed-by tags
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index b4a40099d7e9..d1d03ee90d8f 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2568,6 +2568,7 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
>  			port->pps_data.max_curr = port->pps_data.req_max_curr;
>  			port->req_supply_voltage = port->pps_data.req_out_volt;
>  			port->req_current_limit = port->pps_data.req_op_curr;
> +			power_supply_changed(port->psy);
>  			tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
>  			break;
>  		case SOFT_RESET_SEND:
> @@ -3136,7 +3137,6 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
>  						      port->pps_data.req_out_volt));
>  		port->pps_data.req_op_curr = min(port->pps_data.max_curr,
>  						 port->pps_data.req_op_curr);
> -		power_supply_changed(port->psy);
>  	}
>  
>  	return src_pdo;
> @@ -3561,8 +3561,6 @@ static void tcpm_reset_port(struct tcpm_port *port)
>  	port->sink_cap_done = false;
>  	if (port->tcpc->enable_frs)
>  		port->tcpc->enable_frs(port->tcpc, false);
> -
> -	power_supply_changed(port->psy);
>  }
>  
>  static void tcpm_detach(struct tcpm_port *port)
> -- 
> 2.31.1.295.g9ea45b61b8-goog

-- 
heikki

Re: [PATCH v2 3/6] usb: typec: tcpm: update power supply once partner accepts

[Guenter Roeck]

On 4/7/21 1:07 PM, Badhri Jagan Sridharan wrote:
> power_supply_changed needs to be called to notify clients
> after the partner accepts the requested values for the pps
> case.
> 
> Also, remove the redundant power_supply_changed at the end
> of the tcpm_reset_port as power_supply_changed is already
> called right after usb_type is changed.
> 
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

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

> ---
> Changes since V1:
> * Updated commit description to clarify Guenter Roeck's concern.
> * Added Reviewed-by tags
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index b4a40099d7e9..d1d03ee90d8f 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2568,6 +2568,7 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
>  			port->pps_data.max_curr = port->pps_data.req_max_curr;
>  			port->req_supply_voltage = port->pps_data.req_out_volt;
>  			port->req_current_limit = port->pps_data.req_op_curr;
> +			power_supply_changed(port->psy);
>  			tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
>  			break;
>  		case SOFT_RESET_SEND:
> @@ -3136,7 +3137,6 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
>  						      port->pps_data.req_out_volt));
>  		port->pps_data.req_op_curr = min(port->pps_data.max_curr,
>  						 port->pps_data.req_op_curr);
> -		power_supply_changed(port->psy);
>  	}
>  
>  	return src_pdo;
> @@ -3561,8 +3561,6 @@ static void tcpm_reset_port(struct tcpm_port *port)
>  	port->sink_cap_done = false;
>  	if (port->tcpc->enable_frs)
>  		port->tcpc->enable_frs(port->tcpc, false);
> -
> -	power_supply_changed(port->psy);
>  }
>  
>  static void tcpm_detach(struct tcpm_port *port)
> 

[PATCH v2 4/6] usb: typec: tcpm: Honour pSnkStdby requirement during negotiation

[Badhri Jagan Sridharan]

From PD Spec:
The Sink Shall transition to Sink Standby before a positive or
negative voltage transition of VBUS. During Sink Standby
the Sink Shall reduce its power draw to pSnkStdby. This allows
the Source to manage the voltage transition as well as
supply sufficient operating current to the Sink to maintain PD
operation during the transition. The Sink Shall
complete this transition to Sink Standby within tSnkStdby
after evaluating the Accept Message from the Source. The
transition when returning to Sink operation from Sink Standby
Shall be completed within tSnkNewPower. The
pSnkStdby requirement Shall only apply if the Sink power draw
is higher than this level.

The above requirement needs to be met to prevent hard resets
from port partner.

Without the patch: (5V/3A during SNK_DISCOVERY all the way through
explicit contract)
[   95.711984] CC1: 0 -> 0, CC2: 0 -> 5 [state TOGGLING, polarity 0, connected]
[   95.712007] state change TOGGLING -> SNK_ATTACH_WAIT [rev3 NONE_AMS]
[   95.712017] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 170 ms [rev3 NONE_AMS]
[   95.837190] VBUS on
[   95.882075] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 170 ms]
[   95.882082] state change SNK_DEBOUNCED -> SNK_ATTACHED [rev3 NONE_AMS]
[   95.882086] polarity 1
[   95.883151] set_auto_vbus_discharge_threshold mode:0 pps_active:n vbus:5000 ret:0
[   95.883441] enable vbus discharge ret:0
[   95.883445] Requesting mux state 1, usb-role 2, orientation 2
[   95.883776] state change SNK_ATTACHED -> SNK_STARTUP [rev3 NONE_AMS]
[   95.883879] pending state change SNK_STARTUP -> SNK_DISCOVERY @ 500 ms [rev3 NONE_AMS]
[   96.038960] VBUS on
[   96.383939] state change SNK_STARTUP -> SNK_DISCOVERY [delayed 500 ms]
[   96.383946] Setting voltage/current limit 5000 mV 3000 mA
[   96.383961] vbus=0 charge:=1
[   96.386044] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
[   96.386309] pending state change SNK_WAIT_CAPABILITIES -> HARD_RESET_SEND @ 450 ms [rev3 NONE_AMS]
[   96.394404] PD RX, header: 0x2161 [1]
[   96.394408]  PDO 0: type 0, 5000 mV, 3000 mA [E]
[   96.394410]  PDO 1: type 0, 9000 mV, 2000 mA []
[   96.394412] state change SNK_WAIT_CAPABILITIES -> SNK_NEGOTIATE_CAPABILITIES [rev2 POWER_NEGOTIATION]
[   96.394416] Setting usb_comm capable false
[   96.395083] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
[   96.395089] Requesting PDO 1: 9000 mV, 2000 mA
[   96.395093] PD TX, header: 0x1042
[   96.397404] PD TX complete, status: 0
[   96.397424] pending state change SNK_NEGOTIATE_CAPABILITIES -> HARD_RESET_SEND @ 60 ms [rev2 POWER_NEGOTIATION]
[   96.400826] PD RX, header: 0x363 [1]
[   96.400829] state change SNK_NEGOTIATE_CAPABILITIES -> SNK_TRANSITION_SINK [rev2 POWER_NEGOTIATION]
[   96.400832] pending state change SNK_TRANSITION_SINK -> HARD_RESET_SEND @ 500 ms [rev2 POWER_NEGOTIATION]
[   96.577315] PD RX, header: 0x566 [1]
[   96.577321] Setting voltage/current limit 9000 mV 2000 mA
[   96.578363] set_auto_vbus_discharge_threshold mode:3 pps_active:n vbus:9000 ret:0
[   96.578370] state change SNK_TRANSITION_SINK -> SNK_READY [rev2 POWER_NEGOTIATION]

With the patch:
[  168.398573] CC1: 0 -> 0, CC2: 0 -> 5 [state TOGGLING, polarity 0, connected]
[  168.398605] state change TOGGLING -> SNK_ATTACH_WAIT [rev3 NONE_AMS]
[  168.398619] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 170 ms [rev3 NONE_AMS]
[  168.522348] VBUS on
[  168.568676] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 170 ms]
[  168.568684] state change SNK_DEBOUNCED -> SNK_ATTACHED [rev3 NONE_AMS]
[  168.568688] polarity 1
[  168.569867] set_auto_vbus_discharge_threshold mode:0 pps_active:n vbus:5000 ret:0
[  168.570158] enable vbus discharge ret:0
[  168.570161] Requesting mux state 1, usb-role 2, orientation 2
[  168.570504] state change SNK_ATTACHED -> SNK_STARTUP [rev3 NONE_AMS]
[  168.570634] pending state change SNK_STARTUP -> SNK_DISCOVERY @ 500 ms [rev3 NONE_AMS]
[  169.070689] state change SNK_STARTUP -> SNK_DISCOVERY [delayed 500 ms]
[  169.070695] Setting voltage/current limit 5000 mV 3000 mA
[  169.070702] vbus=0 charge:=1
[  169.072719] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
[  169.073145] pending state change SNK_WAIT_CAPABILITIES -> HARD_RESET_SEND @ 450 ms [rev3 NONE_AMS]
[  169.077162] PD RX, header: 0x2161 [1]
[  169.077172]  PDO 0: type 0, 5000 mV, 3000 mA [E]
[  169.077178]  PDO 1: type 0, 9000 mV, 2000 mA []
[  169.077183] state change SNK_WAIT_CAPABILITIES -> SNK_NEGOTIATE_CAPABILITIES [rev2 POWER_NEGOTIATION]
[  169.077191] Setting usb_comm capable false
[  169.077753] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
[  169.077759] Requesting PDO 1: 9000 mV, 2000 mA
[  169.077762] PD TX, header: 0x1042
[  169.079990] PD TX complete, status: 0
[  169.080013] pending state change SNK_NEGOTIATE_CAPABILITIES -> HARD_RESET_SEND @ 60 ms [rev2 POWER_NEGOTIATION]
[  169.083183] VBUS on
[  169.084195] PD RX, header: 0x363 [1]
[  169.084200] state change SNK_NEGOTIATE_CAPABILITIES -> SNK_TRANSITION_SINK [rev2 POWER_NEGOTIATION]
[  169.084206] Setting standby current 5000 mV @ 500 mA
[  169.084209] Setting voltage/current limit 5000 mV 500 mA
[  169.084220] pending state change SNK_TRANSITION_SINK -> HARD_RESET_SEND @ 500 ms [rev2 POWER_NEGOTIATION]
[  169.260222] PD RX, header: 0x566 [1]
[  169.260227] Setting voltage/current limit 9000 mV 2000 mA
[  169.261315] set_auto_vbus_discharge_threshold mode:3 pps_active:n vbus:9000 ret:0
[  169.261321] state change SNK_TRANSITION_SINK -> SNK_READY [rev2 POWER_NEGOTIATION]
[  169.261570] AMS POWER_NEGOTIATION finished

Fixes: f0690a25a140b ("staging: typec: USB Type-C Port Manager (tcpm)")
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 17 +++++++++++++++++
 include/linux/usb/pd.h        |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index d1d03ee90d8f..770b2edd9a04 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -4131,6 +4131,23 @@ static void run_state_machine(struct tcpm_port *port)
 		}
 		break;
 	case SNK_TRANSITION_SINK:
+		/* From the USB PD spec:
+		 * "The Sink Shall transition to Sink Standby before a positive or
+		 * negative voltage transition of VBUS. During Sink Standby
+		 * the Sink Shall reduce its power draw to pSnkStdby."
+		 *
+		 * This is not applicable to PPS though as the port can continue
+		 * to draw negotiated power without switching to standby.
+		 */
+		if (port->supply_voltage != port->req_supply_voltage && !port->pps_data.active &&
+		    port->current_limit * port->supply_voltage / 1000 > PD_P_SNK_STDBY_MW) {
+			u32 stdby_ma = port->supply_voltage ? PD_P_SNK_STDBY_MW * 1000 /
+				port->supply_voltage : 0;
+			tcpm_log(port, "Setting standby current %u mV @ %u mA",
+				 port->supply_voltage, stdby_ma);
+			tcpm_set_current_limit(port, stdby_ma, port->supply_voltage);
+		}
+		fallthrough;
 	case SNK_TRANSITION_SINK_VBUS:
 		tcpm_set_state(port, hard_reset_state(port),
 			       PD_T_PS_TRANSITION);
diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
index 70d681918d01..bf00259493e0 100644
--- a/include/linux/usb/pd.h
+++ b/include/linux/usb/pd.h
@@ -493,4 +493,6 @@ static inline unsigned int rdo_max_power(u32 rdo)
 #define PD_N_CAPS_COUNT		(PD_T_NO_RESPONSE / PD_T_SEND_SOURCE_CAP)
 #define PD_N_HARD_RESET_COUNT	2
 
+#define PD_P_SNK_STDBY_MW	2500	/* 2500 mW */
+
 #endif /* __LINUX_USB_PD_H */
-- 
2.31.1.295.g9ea45b61b8-goog

Re: [PATCH v2 4/6] usb: typec: tcpm: Honour pSnkStdby requirement during negotiation

[Heikki Krogerus]

On Wed, Apr 07, 2021 at 01:07:21PM -0700, Badhri Jagan Sridharan wrote:
> >From PD Spec:
> The Sink Shall transition to Sink Standby before a positive or
> negative voltage transition of VBUS. During Sink Standby
> the Sink Shall reduce its power draw to pSnkStdby. This allows
> the Source to manage the voltage transition as well as
> supply sufficient operating current to the Sink to maintain PD
> operation during the transition. The Sink Shall
> complete this transition to Sink Standby within tSnkStdby
> after evaluating the Accept Message from the Source. The
> transition when returning to Sink operation from Sink Standby
> Shall be completed within tSnkNewPower. The
> pSnkStdby requirement Shall only apply if the Sink power draw
> is higher than this level.
> 
> The above requirement needs to be met to prevent hard resets
> from port partner.
> 
> Without the patch: (5V/3A during SNK_DISCOVERY all the way through
> explicit contract)
> [   95.711984] CC1: 0 -> 0, CC2: 0 -> 5 [state TOGGLING, polarity 0, connected]
> [   95.712007] state change TOGGLING -> SNK_ATTACH_WAIT [rev3 NONE_AMS]
> [   95.712017] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 170 ms [rev3 NONE_AMS]
> [   95.837190] VBUS on
> [   95.882075] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 170 ms]
> [   95.882082] state change SNK_DEBOUNCED -> SNK_ATTACHED [rev3 NONE_AMS]
> [   95.882086] polarity 1
> [   95.883151] set_auto_vbus_discharge_threshold mode:0 pps_active:n vbus:5000 ret:0
> [   95.883441] enable vbus discharge ret:0
> [   95.883445] Requesting mux state 1, usb-role 2, orientation 2
> [   95.883776] state change SNK_ATTACHED -> SNK_STARTUP [rev3 NONE_AMS]
> [   95.883879] pending state change SNK_STARTUP -> SNK_DISCOVERY @ 500 ms [rev3 NONE_AMS]
> [   96.038960] VBUS on
> [   96.383939] state change SNK_STARTUP -> SNK_DISCOVERY [delayed 500 ms]
> [   96.383946] Setting voltage/current limit 5000 mV 3000 mA
> [   96.383961] vbus=0 charge:=1
> [   96.386044] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
> [   96.386309] pending state change SNK_WAIT_CAPABILITIES -> HARD_RESET_SEND @ 450 ms [rev3 NONE_AMS]
> [   96.394404] PD RX, header: 0x2161 [1]
> [   96.394408]  PDO 0: type 0, 5000 mV, 3000 mA [E]
> [   96.394410]  PDO 1: type 0, 9000 mV, 2000 mA []
> [   96.394412] state change SNK_WAIT_CAPABILITIES -> SNK_NEGOTIATE_CAPABILITIES [rev2 POWER_NEGOTIATION]
> [   96.394416] Setting usb_comm capable false
> [   96.395083] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
> [   96.395089] Requesting PDO 1: 9000 mV, 2000 mA
> [   96.395093] PD TX, header: 0x1042
> [   96.397404] PD TX complete, status: 0
> [   96.397424] pending state change SNK_NEGOTIATE_CAPABILITIES -> HARD_RESET_SEND @ 60 ms [rev2 POWER_NEGOTIATION]
> [   96.400826] PD RX, header: 0x363 [1]
> [   96.400829] state change SNK_NEGOTIATE_CAPABILITIES -> SNK_TRANSITION_SINK [rev2 POWER_NEGOTIATION]
> [   96.400832] pending state change SNK_TRANSITION_SINK -> HARD_RESET_SEND @ 500 ms [rev2 POWER_NEGOTIATION]
> [   96.577315] PD RX, header: 0x566 [1]
> [   96.577321] Setting voltage/current limit 9000 mV 2000 mA
> [   96.578363] set_auto_vbus_discharge_threshold mode:3 pps_active:n vbus:9000 ret:0
> [   96.578370] state change SNK_TRANSITION_SINK -> SNK_READY [rev2 POWER_NEGOTIATION]
> 
> With the patch:
> [  168.398573] CC1: 0 -> 0, CC2: 0 -> 5 [state TOGGLING, polarity 0, connected]
> [  168.398605] state change TOGGLING -> SNK_ATTACH_WAIT [rev3 NONE_AMS]
> [  168.398619] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 170 ms [rev3 NONE_AMS]
> [  168.522348] VBUS on
> [  168.568676] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 170 ms]
> [  168.568684] state change SNK_DEBOUNCED -> SNK_ATTACHED [rev3 NONE_AMS]
> [  168.568688] polarity 1
> [  168.569867] set_auto_vbus_discharge_threshold mode:0 pps_active:n vbus:5000 ret:0
> [  168.570158] enable vbus discharge ret:0
> [  168.570161] Requesting mux state 1, usb-role 2, orientation 2
> [  168.570504] state change SNK_ATTACHED -> SNK_STARTUP [rev3 NONE_AMS]
> [  168.570634] pending state change SNK_STARTUP -> SNK_DISCOVERY @ 500 ms [rev3 NONE_AMS]
> [  169.070689] state change SNK_STARTUP -> SNK_DISCOVERY [delayed 500 ms]
> [  169.070695] Setting voltage/current limit 5000 mV 3000 mA
> [  169.070702] vbus=0 charge:=1
> [  169.072719] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
> [  169.073145] pending state change SNK_WAIT_CAPABILITIES -> HARD_RESET_SEND @ 450 ms [rev3 NONE_AMS]
> [  169.077162] PD RX, header: 0x2161 [1]
> [  169.077172]  PDO 0: type 0, 5000 mV, 3000 mA [E]
> [  169.077178]  PDO 1: type 0, 9000 mV, 2000 mA []
> [  169.077183] state change SNK_WAIT_CAPABILITIES -> SNK_NEGOTIATE_CAPABILITIES [rev2 POWER_NEGOTIATION]
> [  169.077191] Setting usb_comm capable false
> [  169.077753] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
> [  169.077759] Requesting PDO 1: 9000 mV, 2000 mA
> [  169.077762] PD TX, header: 0x1042
> [  169.079990] PD TX complete, status: 0
> [  169.080013] pending state change SNK_NEGOTIATE_CAPABILITIES -> HARD_RESET_SEND @ 60 ms [rev2 POWER_NEGOTIATION]
> [  169.083183] VBUS on
> [  169.084195] PD RX, header: 0x363 [1]
> [  169.084200] state change SNK_NEGOTIATE_CAPABILITIES -> SNK_TRANSITION_SINK [rev2 POWER_NEGOTIATION]
> [  169.084206] Setting standby current 5000 mV @ 500 mA
> [  169.084209] Setting voltage/current limit 5000 mV 500 mA
> [  169.084220] pending state change SNK_TRANSITION_SINK -> HARD_RESET_SEND @ 500 ms [rev2 POWER_NEGOTIATION]
> [  169.260222] PD RX, header: 0x566 [1]
> [  169.260227] Setting voltage/current limit 9000 mV 2000 mA
> [  169.261315] set_auto_vbus_discharge_threshold mode:3 pps_active:n vbus:9000 ret:0
> [  169.261321] state change SNK_TRANSITION_SINK -> SNK_READY [rev2 POWER_NEGOTIATION]
> [  169.261570] AMS POWER_NEGOTIATION finished
> 
> Fixes: f0690a25a140b ("staging: typec: USB Type-C Port Manager (tcpm)")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 17 +++++++++++++++++
>  include/linux/usb/pd.h        |  2 ++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d1d03ee90d8f..770b2edd9a04 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4131,6 +4131,23 @@ static void run_state_machine(struct tcpm_port *port)
>  		}
>  		break;
>  	case SNK_TRANSITION_SINK:
> +		/* From the USB PD spec:
> +		 * "The Sink Shall transition to Sink Standby before a positive or
> +		 * negative voltage transition of VBUS. During Sink Standby
> +		 * the Sink Shall reduce its power draw to pSnkStdby."
> +		 *
> +		 * This is not applicable to PPS though as the port can continue
> +		 * to draw negotiated power without switching to standby.
> +		 */
> +		if (port->supply_voltage != port->req_supply_voltage && !port->pps_data.active &&
> +		    port->current_limit * port->supply_voltage / 1000 > PD_P_SNK_STDBY_MW) {
> +			u32 stdby_ma = port->supply_voltage ? PD_P_SNK_STDBY_MW * 1000 /
> +				port->supply_voltage : 0;

Looks like unnecessary condition to me. The first condition can not be
true if port->supply_voltage == 0. So I think that should be just:

                        u32 stdby_ma = PD_P_SNK_STDBY_MW * 1000 / port->supply_voltage;

Or did I miss something?

> +			tcpm_log(port, "Setting standby current %u mV @ %u mA",
> +				 port->supply_voltage, stdby_ma);
> +			tcpm_set_current_limit(port, stdby_ma, port->supply_voltage);
> +		}
> +		fallthrough;
>  	case SNK_TRANSITION_SINK_VBUS:
>  		tcpm_set_state(port, hard_reset_state(port),
>  			       PD_T_PS_TRANSITION);
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index 70d681918d01..bf00259493e0 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -493,4 +493,6 @@ static inline unsigned int rdo_max_power(u32 rdo)
>  #define PD_N_CAPS_COUNT		(PD_T_NO_RESPONSE / PD_T_SEND_SOURCE_CAP)
>  #define PD_N_HARD_RESET_COUNT	2
>  
> +#define PD_P_SNK_STDBY_MW	2500	/* 2500 mW */
> +
>  #endif /* __LINUX_USB_PD_H */
> -- 
> 2.31.1.295.g9ea45b61b8-goog

thanks,

-- 
heikki

Re: [PATCH v2 4/6] usb: typec: tcpm: Honour pSnkStdby requirement during negotiation

[Badhri Jagan Sridharan]

On Thu, Apr 8, 2021 at 12:48 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Apr 07, 2021 at 01:07:21PM -0700, Badhri Jagan Sridharan wrote:
> > >From PD Spec:
> > The Sink Shall transition to Sink Standby before a positive or
> > negative voltage transition of VBUS. During Sink Standby
> > the Sink Shall reduce its power draw to pSnkStdby. This allows
> > the Source to manage the voltage transition as well as
> > supply sufficient operating current to the Sink to maintain PD
> > operation during the transition. The Sink Shall
> > complete this transition to Sink Standby within tSnkStdby
> > after evaluating the Accept Message from the Source. The
> > transition when returning to Sink operation from Sink Standby
> > Shall be completed within tSnkNewPower. The
> > pSnkStdby requirement Shall only apply if the Sink power draw
> > is higher than this level.
> >
> > The above requirement needs to be met to prevent hard resets
> > from port partner.
> >
> > Without the patch: (5V/3A during SNK_DISCOVERY all the way through
> > explicit contract)
> > [   95.711984] CC1: 0 -> 0, CC2: 0 -> 5 [state TOGGLING, polarity 0, connected]
> > [   95.712007] state change TOGGLING -> SNK_ATTACH_WAIT [rev3 NONE_AMS]
> > [   95.712017] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 170 ms [rev3 NONE_AMS]
> > [   95.837190] VBUS on
> > [   95.882075] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 170 ms]
> > [   95.882082] state change SNK_DEBOUNCED -> SNK_ATTACHED [rev3 NONE_AMS]
> > [   95.882086] polarity 1
> > [   95.883151] set_auto_vbus_discharge_threshold mode:0 pps_active:n vbus:5000 ret:0
> > [   95.883441] enable vbus discharge ret:0
> > [   95.883445] Requesting mux state 1, usb-role 2, orientation 2
> > [   95.883776] state change SNK_ATTACHED -> SNK_STARTUP [rev3 NONE_AMS]
> > [   95.883879] pending state change SNK_STARTUP -> SNK_DISCOVERY @ 500 ms [rev3 NONE_AMS]
> > [   96.038960] VBUS on
> > [   96.383939] state change SNK_STARTUP -> SNK_DISCOVERY [delayed 500 ms]
> > [   96.383946] Setting voltage/current limit 5000 mV 3000 mA
> > [   96.383961] vbus=0 charge:=1
> > [   96.386044] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
> > [   96.386309] pending state change SNK_WAIT_CAPABILITIES -> HARD_RESET_SEND @ 450 ms [rev3 NONE_AMS]
> > [   96.394404] PD RX, header: 0x2161 [1]
> > [   96.394408]  PDO 0: type 0, 5000 mV, 3000 mA [E]
> > [   96.394410]  PDO 1: type 0, 9000 mV, 2000 mA []
> > [   96.394412] state change SNK_WAIT_CAPABILITIES -> SNK_NEGOTIATE_CAPABILITIES [rev2 POWER_NEGOTIATION]
> > [   96.394416] Setting usb_comm capable false
> > [   96.395083] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
> > [   96.395089] Requesting PDO 1: 9000 mV, 2000 mA
> > [   96.395093] PD TX, header: 0x1042
> > [   96.397404] PD TX complete, status: 0
> > [   96.397424] pending state change SNK_NEGOTIATE_CAPABILITIES -> HARD_RESET_SEND @ 60 ms [rev2 POWER_NEGOTIATION]
> > [   96.400826] PD RX, header: 0x363 [1]
> > [   96.400829] state change SNK_NEGOTIATE_CAPABILITIES -> SNK_TRANSITION_SINK [rev2 POWER_NEGOTIATION]
> > [   96.400832] pending state change SNK_TRANSITION_SINK -> HARD_RESET_SEND @ 500 ms [rev2 POWER_NEGOTIATION]
> > [   96.577315] PD RX, header: 0x566 [1]
> > [   96.577321] Setting voltage/current limit 9000 mV 2000 mA
> > [   96.578363] set_auto_vbus_discharge_threshold mode:3 pps_active:n vbus:9000 ret:0
> > [   96.578370] state change SNK_TRANSITION_SINK -> SNK_READY [rev2 POWER_NEGOTIATION]
> >
> > With the patch:
> > [  168.398573] CC1: 0 -> 0, CC2: 0 -> 5 [state TOGGLING, polarity 0, connected]
> > [  168.398605] state change TOGGLING -> SNK_ATTACH_WAIT [rev3 NONE_AMS]
> > [  168.398619] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 170 ms [rev3 NONE_AMS]
> > [  168.522348] VBUS on
> > [  168.568676] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 170 ms]
> > [  168.568684] state change SNK_DEBOUNCED -> SNK_ATTACHED [rev3 NONE_AMS]
> > [  168.568688] polarity 1
> > [  168.569867] set_auto_vbus_discharge_threshold mode:0 pps_active:n vbus:5000 ret:0
> > [  168.570158] enable vbus discharge ret:0
> > [  168.570161] Requesting mux state 1, usb-role 2, orientation 2
> > [  168.570504] state change SNK_ATTACHED -> SNK_STARTUP [rev3 NONE_AMS]
> > [  168.570634] pending state change SNK_STARTUP -> SNK_DISCOVERY @ 500 ms [rev3 NONE_AMS]
> > [  169.070689] state change SNK_STARTUP -> SNK_DISCOVERY [delayed 500 ms]
> > [  169.070695] Setting voltage/current limit 5000 mV 3000 mA
> > [  169.070702] vbus=0 charge:=1
> > [  169.072719] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
> > [  169.073145] pending state change SNK_WAIT_CAPABILITIES -> HARD_RESET_SEND @ 450 ms [rev3 NONE_AMS]
> > [  169.077162] PD RX, header: 0x2161 [1]
> > [  169.077172]  PDO 0: type 0, 5000 mV, 3000 mA [E]
> > [  169.077178]  PDO 1: type 0, 9000 mV, 2000 mA []
> > [  169.077183] state change SNK_WAIT_CAPABILITIES -> SNK_NEGOTIATE_CAPABILITIES [rev2 POWER_NEGOTIATION]
> > [  169.077191] Setting usb_comm capable false
> > [  169.077753] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
> > [  169.077759] Requesting PDO 1: 9000 mV, 2000 mA
> > [  169.077762] PD TX, header: 0x1042
> > [  169.079990] PD TX complete, status: 0
> > [  169.080013] pending state change SNK_NEGOTIATE_CAPABILITIES -> HARD_RESET_SEND @ 60 ms [rev2 POWER_NEGOTIATION]
> > [  169.083183] VBUS on
> > [  169.084195] PD RX, header: 0x363 [1]
> > [  169.084200] state change SNK_NEGOTIATE_CAPABILITIES -> SNK_TRANSITION_SINK [rev2 POWER_NEGOTIATION]
> > [  169.084206] Setting standby current 5000 mV @ 500 mA
> > [  169.084209] Setting voltage/current limit 5000 mV 500 mA
> > [  169.084220] pending state change SNK_TRANSITION_SINK -> HARD_RESET_SEND @ 500 ms [rev2 POWER_NEGOTIATION]
> > [  169.260222] PD RX, header: 0x566 [1]
> > [  169.260227] Setting voltage/current limit 9000 mV 2000 mA
> > [  169.261315] set_auto_vbus_discharge_threshold mode:3 pps_active:n vbus:9000 ret:0
> > [  169.261321] state change SNK_TRANSITION_SINK -> SNK_READY [rev2 POWER_NEGOTIATION]
> > [  169.261570] AMS POWER_NEGOTIATION finished
> >
> > Fixes: f0690a25a140b ("staging: typec: USB Type-C Port Manager (tcpm)")
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 17 +++++++++++++++++
> >  include/linux/usb/pd.h        |  2 ++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index d1d03ee90d8f..770b2edd9a04 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -4131,6 +4131,23 @@ static void run_state_machine(struct tcpm_port *port)
> >               }
> >               break;
> >       case SNK_TRANSITION_SINK:
> > +             /* From the USB PD spec:
> > +              * "The Sink Shall transition to Sink Standby before a positive or
> > +              * negative voltage transition of VBUS. During Sink Standby
> > +              * the Sink Shall reduce its power draw to pSnkStdby."
> > +              *
> > +              * This is not applicable to PPS though as the port can continue
> > +              * to draw negotiated power without switching to standby.
> > +              */
> > +             if (port->supply_voltage != port->req_supply_voltage && !port->pps_data.active &&
> > +                 port->current_limit * port->supply_voltage / 1000 > PD_P_SNK_STDBY_MW) {
> > +                     u32 stdby_ma = port->supply_voltage ? PD_P_SNK_STDBY_MW * 1000 /
> > +                             port->supply_voltage : 0;
>
> Looks like unnecessary condition to me. The first condition can not be
> true if port->supply_voltage == 0. So I think that should be just:
>
>                         u32 stdby_ma = PD_P_SNK_STDBY_MW * 1000 / port->supply_voltage;
>
> Or did I miss something?

You are right. That's indeed not necessary. I was wondering whether
port->supply_voltage would be
0 during the swap sequence. It doesn't seem to be. Updating in my next
version - V3. Thanks Heikki !

>
> > +                     tcpm_log(port, "Setting standby current %u mV @ %u mA",
> > +                              port->supply_voltage, stdby_ma);
> > +                     tcpm_set_current_limit(port, stdby_ma, port->supply_voltage);
> > +             }
> > +             fallthrough;
> >       case SNK_TRANSITION_SINK_VBUS:
> >               tcpm_set_state(port, hard_reset_state(port),
> >                              PD_T_PS_TRANSITION);
> > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> > index 70d681918d01..bf00259493e0 100644
> > --- a/include/linux/usb/pd.h
> > +++ b/include/linux/usb/pd.h
> > @@ -493,4 +493,6 @@ static inline unsigned int rdo_max_power(u32 rdo)
> >  #define PD_N_CAPS_COUNT              (PD_T_NO_RESPONSE / PD_T_SEND_SOURCE_CAP)
> >  #define PD_N_HARD_RESET_COUNT        2
> >
> > +#define PD_P_SNK_STDBY_MW    2500    /* 2500 mW */
> > +
> >  #endif /* __LINUX_USB_PD_H */
> > --
> > 2.31.1.295.g9ea45b61b8-goog
>
> thanks,
>
> --
> heikki

Re: [PATCH v2 4/6] usb: typec: tcpm: Honour pSnkStdby requirement during negotiation

[Guenter Roeck]

On 4/7/21 1:07 PM, Badhri Jagan Sridharan wrote:
>>From PD Spec:
> The Sink Shall transition to Sink Standby before a positive or
> negative voltage transition of VBUS. During Sink Standby
> the Sink Shall reduce its power draw to pSnkStdby. This allows
> the Source to manage the voltage transition as well as
> supply sufficient operating current to the Sink to maintain PD
> operation during the transition. The Sink Shall
> complete this transition to Sink Standby within tSnkStdby
> after evaluating the Accept Message from the Source. The
> transition when returning to Sink operation from Sink Standby
> Shall be completed within tSnkNewPower. The
> pSnkStdby requirement Shall only apply if the Sink power draw
> is higher than this level.
> 
> The above requirement needs to be met to prevent hard resets
> from port partner.
> 
> Without the patch: (5V/3A during SNK_DISCOVERY all the way through
> explicit contract)
> [   95.711984] CC1: 0 -> 0, CC2: 0 -> 5 [state TOGGLING, polarity 0, connected]
> [   95.712007] state change TOGGLING -> SNK_ATTACH_WAIT [rev3 NONE_AMS]
> [   95.712017] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 170 ms [rev3 NONE_AMS]
> [   95.837190] VBUS on
> [   95.882075] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 170 ms]
> [   95.882082] state change SNK_DEBOUNCED -> SNK_ATTACHED [rev3 NONE_AMS]
> [   95.882086] polarity 1
> [   95.883151] set_auto_vbus_discharge_threshold mode:0 pps_active:n vbus:5000 ret:0
> [   95.883441] enable vbus discharge ret:0
> [   95.883445] Requesting mux state 1, usb-role 2, orientation 2
> [   95.883776] state change SNK_ATTACHED -> SNK_STARTUP [rev3 NONE_AMS]
> [   95.883879] pending state change SNK_STARTUP -> SNK_DISCOVERY @ 500 ms [rev3 NONE_AMS]
> [   96.038960] VBUS on
> [   96.383939] state change SNK_STARTUP -> SNK_DISCOVERY [delayed 500 ms]
> [   96.383946] Setting voltage/current limit 5000 mV 3000 mA
> [   96.383961] vbus=0 charge:=1
> [   96.386044] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
> [   96.386309] pending state change SNK_WAIT_CAPABILITIES -> HARD_RESET_SEND @ 450 ms [rev3 NONE_AMS]
> [   96.394404] PD RX, header: 0x2161 [1]
> [   96.394408]  PDO 0: type 0, 5000 mV, 3000 mA [E]
> [   96.394410]  PDO 1: type 0, 9000 mV, 2000 mA []
> [   96.394412] state change SNK_WAIT_CAPABILITIES -> SNK_NEGOTIATE_CAPABILITIES [rev2 POWER_NEGOTIATION]
> [   96.394416] Setting usb_comm capable false
> [   96.395083] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
> [   96.395089] Requesting PDO 1: 9000 mV, 2000 mA
> [   96.395093] PD TX, header: 0x1042
> [   96.397404] PD TX complete, status: 0
> [   96.397424] pending state change SNK_NEGOTIATE_CAPABILITIES -> HARD_RESET_SEND @ 60 ms [rev2 POWER_NEGOTIATION]
> [   96.400826] PD RX, header: 0x363 [1]
> [   96.400829] state change SNK_NEGOTIATE_CAPABILITIES -> SNK_TRANSITION_SINK [rev2 POWER_NEGOTIATION]
> [   96.400832] pending state change SNK_TRANSITION_SINK -> HARD_RESET_SEND @ 500 ms [rev2 POWER_NEGOTIATION]
> [   96.577315] PD RX, header: 0x566 [1]
> [   96.577321] Setting voltage/current limit 9000 mV 2000 mA
> [   96.578363] set_auto_vbus_discharge_threshold mode:3 pps_active:n vbus:9000 ret:0
> [   96.578370] state change SNK_TRANSITION_SINK -> SNK_READY [rev2 POWER_NEGOTIATION]
> 
> With the patch:
> [  168.398573] CC1: 0 -> 0, CC2: 0 -> 5 [state TOGGLING, polarity 0, connected]
> [  168.398605] state change TOGGLING -> SNK_ATTACH_WAIT [rev3 NONE_AMS]
> [  168.398619] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 170 ms [rev3 NONE_AMS]
> [  168.522348] VBUS on
> [  168.568676] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 170 ms]
> [  168.568684] state change SNK_DEBOUNCED -> SNK_ATTACHED [rev3 NONE_AMS]
> [  168.568688] polarity 1
> [  168.569867] set_auto_vbus_discharge_threshold mode:0 pps_active:n vbus:5000 ret:0
> [  168.570158] enable vbus discharge ret:0
> [  168.570161] Requesting mux state 1, usb-role 2, orientation 2
> [  168.570504] state change SNK_ATTACHED -> SNK_STARTUP [rev3 NONE_AMS]
> [  168.570634] pending state change SNK_STARTUP -> SNK_DISCOVERY @ 500 ms [rev3 NONE_AMS]
> [  169.070689] state change SNK_STARTUP -> SNK_DISCOVERY [delayed 500 ms]
> [  169.070695] Setting voltage/current limit 5000 mV 3000 mA
> [  169.070702] vbus=0 charge:=1
> [  169.072719] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
> [  169.073145] pending state change SNK_WAIT_CAPABILITIES -> HARD_RESET_SEND @ 450 ms [rev3 NONE_AMS]
> [  169.077162] PD RX, header: 0x2161 [1]
> [  169.077172]  PDO 0: type 0, 5000 mV, 3000 mA [E]
> [  169.077178]  PDO 1: type 0, 9000 mV, 2000 mA []
> [  169.077183] state change SNK_WAIT_CAPABILITIES -> SNK_NEGOTIATE_CAPABILITIES [rev2 POWER_NEGOTIATION]
> [  169.077191] Setting usb_comm capable false
> [  169.077753] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
> [  169.077759] Requesting PDO 1: 9000 mV, 2000 mA
> [  169.077762] PD TX, header: 0x1042
> [  169.079990] PD TX complete, status: 0
> [  169.080013] pending state change SNK_NEGOTIATE_CAPABILITIES -> HARD_RESET_SEND @ 60 ms [rev2 POWER_NEGOTIATION]
> [  169.083183] VBUS on
> [  169.084195] PD RX, header: 0x363 [1]
> [  169.084200] state change SNK_NEGOTIATE_CAPABILITIES -> SNK_TRANSITION_SINK [rev2 POWER_NEGOTIATION]
> [  169.084206] Setting standby current 5000 mV @ 500 mA
> [  169.084209] Setting voltage/current limit 5000 mV 500 mA
> [  169.084220] pending state change SNK_TRANSITION_SINK -> HARD_RESET_SEND @ 500 ms [rev2 POWER_NEGOTIATION]
> [  169.260222] PD RX, header: 0x566 [1]
> [  169.260227] Setting voltage/current limit 9000 mV 2000 mA
> [  169.261315] set_auto_vbus_discharge_threshold mode:3 pps_active:n vbus:9000 ret:0
> [  169.261321] state change SNK_TRANSITION_SINK -> SNK_READY [rev2 POWER_NEGOTIATION]
> [  169.261570] AMS POWER_NEGOTIATION finished
> 
> Fixes: f0690a25a140b ("staging: typec: USB Type-C Port Manager (tcpm)")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>

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

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 17 +++++++++++++++++
>  include/linux/usb/pd.h        |  2 ++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d1d03ee90d8f..770b2edd9a04 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4131,6 +4131,23 @@ static void run_state_machine(struct tcpm_port *port)
>  		}
>  		break;
>  	case SNK_TRANSITION_SINK:
> +		/* From the USB PD spec:
> +		 * "The Sink Shall transition to Sink Standby before a positive or
> +		 * negative voltage transition of VBUS. During Sink Standby
> +		 * the Sink Shall reduce its power draw to pSnkStdby."
> +		 *
> +		 * This is not applicable to PPS though as the port can continue
> +		 * to draw negotiated power without switching to standby.
> +		 */
> +		if (port->supply_voltage != port->req_supply_voltage && !port->pps_data.active &&
> +		    port->current_limit * port->supply_voltage / 1000 > PD_P_SNK_STDBY_MW) {
> +			u32 stdby_ma = port->supply_voltage ? PD_P_SNK_STDBY_MW * 1000 /
> +				port->supply_voltage : 0;
> +			tcpm_log(port, "Setting standby current %u mV @ %u mA",
> +				 port->supply_voltage, stdby_ma);
> +			tcpm_set_current_limit(port, stdby_ma, port->supply_voltage);
> +		}
> +		fallthrough;
>  	case SNK_TRANSITION_SINK_VBUS:
>  		tcpm_set_state(port, hard_reset_state(port),
>  			       PD_T_PS_TRANSITION);
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index 70d681918d01..bf00259493e0 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -493,4 +493,6 @@ static inline unsigned int rdo_max_power(u32 rdo)
>  #define PD_N_CAPS_COUNT		(PD_T_NO_RESPONSE / PD_T_SEND_SOURCE_CAP)
>  #define PD_N_HARD_RESET_COUNT	2
>  
> +#define PD_P_SNK_STDBY_MW	2500	/* 2500 mW */
> +
>  #endif /* __LINUX_USB_PD_H */
> 

[PATCH v2 5/6] usb: typec: tcpm: Allow slow charging loops to comply to pSnkStby

[Badhri Jagan Sridharan]

When a PD charger advertising Rp-3.0 is connected to a sink port, the
sink port current limit would 3A, during SNK_DISCOVERY, till power
negotiation starts. Once the negotiation starts the power limit needs
to drop down to pSnkStby(500mA @ 5V) and to negotiated current limit
once the explicit contract is in place. Not all charging loops can ramp
up to 3A and drop down to 500mA within tSnkStdby which is 15ms. The port
partner might hard reset if tSnkStdby is not met.

To solve this problem, this patch introduces slow-charger-loop which
when set makes the port request PD_P_SNK_STDBY_MW upon entering
SNK_DISCOVERY(instead of 3A or the 1.5A during SNK_DISCOVERY) and the
actual currrent limit after RX of PD_CTRL_PSRDY for PD link or during
SNK_READY for non-pd link.

Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 770b2edd9a04..b5bed6866a2b 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -459,6 +459,12 @@ struct tcpm_port {
 	/* Auto vbus discharge status */
 	bool auto_vbus_discharge_enabled;
 
+	/*
+	 * When set, port requests PD_P_SNK_STDBY_MW upon entering SNK_DISCOVERY and
+	 * the actual currrent limit after RX of PD_CTRL_PSRDY for PD link,
+	 * SNK_READY for non-pd link.
+	 */
+	bool slow_charger_loop;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
 	struct mutex logbuffer_lock;	/* log buffer access lock */
@@ -4047,9 +4053,12 @@ static void run_state_machine(struct tcpm_port *port)
 		break;
 	case SNK_DISCOVERY:
 		if (port->vbus_present) {
-			tcpm_set_current_limit(port,
-					       tcpm_get_current_limit(port),
-					       5000);
+			u32 current_lim = (!port->slow_charger_loop ||
+					   (tcpm_get_current_limit(port) <=
+					    PD_P_SNK_STDBY_MW / 5)) ?
+				tcpm_get_current_limit(port) :
+				PD_P_SNK_STDBY_MW / 5;
+			tcpm_set_current_limit(port, current_lim, 5000);
 			tcpm_set_charge(port, true);
 			tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
 			break;
@@ -4161,6 +4170,8 @@ static void run_state_machine(struct tcpm_port *port)
 			port->pwr_opmode = TYPEC_PWR_MODE_PD;
 		}
 
+		if (!port->pd_capable && port->slow_charger_loop)
+			tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
 		tcpm_swap_complete(port, 0);
 		tcpm_typec_connect(port);
 		mod_enable_frs_delayed_work(port, 0);
@@ -5763,6 +5774,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 	port->typec_caps.type = ret;
 	port->port_type = port->typec_caps.type;
 
+	port->slow_charger_loop = fwnode_property_read_bool(fwnode, "slow-charger-loop");
 	if (port->port_type == TYPEC_PORT_SNK)
 		goto sink;
 
-- 
2.31.1.295.g9ea45b61b8-goog

Re: [PATCH v2 5/6] usb: typec: tcpm: Allow slow charging loops to comply to pSnkStby

[Heikki Krogerus]

On Wed, Apr 07, 2021 at 01:07:22PM -0700, Badhri Jagan Sridharan wrote:
> When a PD charger advertising Rp-3.0 is connected to a sink port, the
> sink port current limit would 3A, during SNK_DISCOVERY, till power
> negotiation starts. Once the negotiation starts the power limit needs
> to drop down to pSnkStby(500mA @ 5V) and to negotiated current limit
> once the explicit contract is in place. Not all charging loops can ramp
> up to 3A and drop down to 500mA within tSnkStdby which is 15ms. The port
> partner might hard reset if tSnkStdby is not met.
> 
> To solve this problem, this patch introduces slow-charger-loop which
> when set makes the port request PD_P_SNK_STDBY_MW upon entering
> SNK_DISCOVERY(instead of 3A or the 1.5A during SNK_DISCOVERY) and the
> actual currrent limit after RX of PD_CTRL_PSRDY for PD link or during
> SNK_READY for non-pd link.
> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 770b2edd9a04..b5bed6866a2b 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -459,6 +459,12 @@ struct tcpm_port {
>  	/* Auto vbus discharge status */
>  	bool auto_vbus_discharge_enabled;
>  
> +	/*
> +	 * When set, port requests PD_P_SNK_STDBY_MW upon entering SNK_DISCOVERY and
> +	 * the actual currrent limit after RX of PD_CTRL_PSRDY for PD link,
> +	 * SNK_READY for non-pd link.
> +	 */
> +	bool slow_charger_loop;
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry *dentry;
>  	struct mutex logbuffer_lock;	/* log buffer access lock */
> @@ -4047,9 +4053,12 @@ static void run_state_machine(struct tcpm_port *port)
>  		break;
>  	case SNK_DISCOVERY:
>  		if (port->vbus_present) {
> -			tcpm_set_current_limit(port,
> -					       tcpm_get_current_limit(port),
> -					       5000);
> +			u32 current_lim = (!port->slow_charger_loop ||
> +					   (tcpm_get_current_limit(port) <=
> +					    PD_P_SNK_STDBY_MW / 5)) ?
> +				tcpm_get_current_limit(port) :
> +				PD_P_SNK_STDBY_MW / 5;

Here the use of the ternary operator is not appropriate. Please try to
clean that up somehow. Maybe something like this would be better?

                        u32 current_lim = tcpm_get_current_limit(port);

			if (port->slow_charger_loop || (current_lim < PD_P_SNK_STDBY_MW / 5))
				current_lim = PD_P_SNK_STDBY_MW / 5;

> +			tcpm_set_current_limit(port, current_lim, 5000);
>  			tcpm_set_charge(port, true);
>  			tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
>  			break;
> @@ -4161,6 +4170,8 @@ static void run_state_machine(struct tcpm_port *port)
>  			port->pwr_opmode = TYPEC_PWR_MODE_PD;
>  		}
>  
> +		if (!port->pd_capable && port->slow_charger_loop)
> +			tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
>  		tcpm_swap_complete(port, 0);
>  		tcpm_typec_connect(port);
>  		mod_enable_frs_delayed_work(port, 0);
> @@ -5763,6 +5774,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
>  	port->typec_caps.type = ret;
>  	port->port_type = port->typec_caps.type;
>  
> +	port->slow_charger_loop = fwnode_property_read_bool(fwnode, "slow-charger-loop");
>  	if (port->port_type == TYPEC_PORT_SNK)
>  		goto sink;
>  
> -- 
> 2.31.1.295.g9ea45b61b8-goog

thanks,

-- 
heikki

Re: [PATCH v2 5/6] usb: typec: tcpm: Allow slow charging loops to comply to pSnkStby

[Heikki Krogerus]

> > @@ -4047,9 +4053,12 @@ static void run_state_machine(struct tcpm_port *port)
> >  		break;
> >  	case SNK_DISCOVERY:
> >  		if (port->vbus_present) {
> > -			tcpm_set_current_limit(port,
> > -					       tcpm_get_current_limit(port),
> > -					       5000);
> > +			u32 current_lim = (!port->slow_charger_loop ||
> > +					   (tcpm_get_current_limit(port) <=
> > +					    PD_P_SNK_STDBY_MW / 5)) ?
> > +				tcpm_get_current_limit(port) :
> > +				PD_P_SNK_STDBY_MW / 5;
> 
> Here the use of the ternary operator is not appropriate. Please try to
> clean that up somehow. Maybe something like this would be better?
> 
>                         u32 current_lim = tcpm_get_current_limit(port);
> 
> 			if (port->slow_charger_loop || (current_lim < PD_P_SNK_STDBY_MW / 5))
> 				current_lim = PD_P_SNK_STDBY_MW / 5;

Sorry, I mean:

			if (port->slow_charger_loop || (current_lim > PD_P_SNK_STDBY_MW / 5))
				current_lim = PD_P_SNK_STDBY_MW / 5;

thanks,

-- 
heikki

Re: [PATCH v2 5/6] usb: typec: tcpm: Allow slow charging loops to comply to pSnkStby

[Badhri Jagan Sridharan]

On Thu, Apr 8, 2021 at 1:22 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> > > @@ -4047,9 +4053,12 @@ static void run_state_machine(struct tcpm_port *port)
> > >             break;
> > >     case SNK_DISCOVERY:
> > >             if (port->vbus_present) {
> > > -                   tcpm_set_current_limit(port,
> > > -                                          tcpm_get_current_limit(port),
> > > -                                          5000);
> > > +                   u32 current_lim = (!port->slow_charger_loop ||
> > > +                                      (tcpm_get_current_limit(port) <=
> > > +                                       PD_P_SNK_STDBY_MW / 5)) ?
> > > +                           tcpm_get_current_limit(port) :
> > > +                           PD_P_SNK_STDBY_MW / 5;
> >
> > Here the use of the ternary operator is not appropriate. Please try to
> > clean that up somehow. Maybe something like this would be better?
> >
> >                         u32 current_lim = tcpm_get_current_limit(port);
> >
> >                       if (port->slow_charger_loop || (current_lim < PD_P_SNK_STDBY_MW / 5))
> >                               current_lim = PD_P_SNK_STDBY_MW / 5;
>
> Sorry, I mean:
>
>                         if (port->slow_charger_loop || (current_lim > PD_P_SNK_STDBY_MW / 5))
>                                 current_lim = PD_P_SNK_STDBY_MW / 5;

Ack. Updating in my next version: V3.

Thanks,
Badhri

>
> thanks,
>
> --
> heikki

[PATCH v2 6/6] Documentation: connector: Add slow-charger-loop definition

[Badhri Jagan Sridharan]

To allow slow charger loops to comply to pSnkStby requirement,
this patch introduces slow-charger-loop which when set makes
the port request PD_P_SNK_STDBY_MW upon entering SNK_DISCOVERY
(instead of 3A or the 1.5A during SNK_DISCOVERY) and the actual
currrent limit after RX of PD_CTRL_PSRDY for PD link or during
SNK_READY for non-pd link.

Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 .../devicetree/bindings/connector/usb-connector.yaml       | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml
index b6daedd62516..09ad3ad983a6 100644
--- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
+++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
@@ -197,6 +197,13 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [1, 2, 3]
 
+  slow-charger-loop:
+    description: Allows slow charging loops to comply to pSnkStby. When set makes
+      the port request pSnkStby(2.5W - 5V@500mA) upon entering SNK_DISCOVERY(instead
+      of 3A or the 1.5A during SNK_DISCOVERY) and the actual currrent limit after
+      reception of PS_Ready for PD link or during SNK_READY for non-pd link.
+    type: boolean
+
 required:
   - compatible
 
-- 
2.31.1.295.g9ea45b61b8-goog

Re: [PATCH v2 6/6] Documentation: connector: Add slow-charger-loop definition

[Rob Herring]

On Wed, Apr 07, 2021 at 01:07:23PM -0700, Badhri Jagan Sridharan wrote:
> To allow slow charger loops to comply to pSnkStby requirement,
> this patch introduces slow-charger-loop which when set makes
> the port request PD_P_SNK_STDBY_MW upon entering SNK_DISCOVERY
> (instead of 3A or the 1.5A during SNK_DISCOVERY) and the actual
> currrent limit after RX of PD_CTRL_PSRDY for PD link or during
> SNK_READY for non-pd link.

What are 'slow charger loops' and pSnkStby?

'dt-bindings: connector: ...' for the subject. Follow the pattern you 
see with 'git log --oneline' for a directory.

> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  .../devicetree/bindings/connector/usb-connector.yaml       | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> index b6daedd62516..09ad3ad983a6 100644
> --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> @@ -197,6 +197,13 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      enum: [1, 2, 3]
>  
> +  slow-charger-loop:
> +    description: Allows slow charging loops to comply to pSnkStby. When set makes
> +      the port request pSnkStby(2.5W - 5V@500mA) upon entering SNK_DISCOVERY(instead
> +      of 3A or the 1.5A during SNK_DISCOVERY) and the actual currrent limit after
> +      reception of PS_Ready for PD link or during SNK_READY for non-pd link.
> +    type: boolean
> +
>  required:
>    - compatible
>  
> -- 
> 2.31.1.295.g9ea45b61b8-goog
> 

Re: [PATCH v2 6/6] Documentation: connector: Add slow-charger-loop definition

[Badhri Jagan Sridharan]

Hi Rob,

Have clarified "slow charger loops" and pSnkStby in my V3 of the patch
and have updated the subject as well.
I was actually doing a git log where the last one showed up with the
following commit description.

commit 4b59b60d896f3ed94921974e916db091bc3a9ba8
Author: Kyle Tso <kyletso@google.com>
Date:   Fri Feb 12 15:37:43 2021 +0800

    Documentation: connector: Update the description of sink-vdos

    Remove the acronym "VDM" and replace it with the full name "Vendor
    Defined Message".

    Reviewed-by: Guenter Roeck <linux@roeck-us.net>
    Signed-off-by: Kyle Tso <kyletso@google.com>
    Link: https://lore.kernel.org/r/20210212073743.665038-1-kyletso@google.com
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks,
Badhri

On Fri, Apr 9, 2021 at 11:38 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Apr 07, 2021 at 01:07:23PM -0700, Badhri Jagan Sridharan wrote:
> > To allow slow charger loops to comply to pSnkStby requirement,
> > this patch introduces slow-charger-loop which when set makes
> > the port request PD_P_SNK_STDBY_MW upon entering SNK_DISCOVERY
> > (instead of 3A or the 1.5A during SNK_DISCOVERY) and the actual
> > currrent limit after RX of PD_CTRL_PSRDY for PD link or during
> > SNK_READY for non-pd link.
>
> What are 'slow charger loops' and pSnkStby?
>
> 'dt-bindings: connector: ...' for the subject. Follow the pattern you
> see with 'git log --oneline' for a directory.
>
> >
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  .../devicetree/bindings/connector/usb-connector.yaml       | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> > index b6daedd62516..09ad3ad983a6 100644
> > --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
> > +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> > @@ -197,6 +197,13 @@ properties:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      enum: [1, 2, 3]
> >
> > +  slow-charger-loop:
> > +    description: Allows slow charging loops to comply to pSnkStby. When set makes
> > +      the port request pSnkStby(2.5W - 5V@500mA) upon entering SNK_DISCOVERY(instead
> > +      of 3A or the 1.5A during SNK_DISCOVERY) and the actual currrent limit after
> > +      reception of PS_Ready for PD link or during SNK_READY for non-pd link.
> > +    type: boolean
> > +
> >  required:
> >    - compatible
> >
> > --
> > 2.31.1.295.g9ea45b61b8-goog
> >

Re: [PATCH v2 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply

[Heikki Krogerus]

On Wed, Apr 07, 2021 at 01:07:18PM -0700, Badhri Jagan Sridharan wrote:
> tcpm_pd_build_request overwrites current_limit and supply_voltage
> even before port partner accepts the requests. This leaves stale
> values in current_limit and supply_voltage that get exported by
> "tcpm-source-psy-". Solving this problem by caching the request
> values of current limit/supply voltage in req_current_limit
> and req_supply_voltage. current_limit/supply_voltage gets updated
> once the port partner accepts the request.
> 
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

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

> ---
> Changes since V1:
> * Fixed typo as suggested by Guenter Roeck.
> * Added Reviewed-by tags.
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index ca1fc77697fc..4ea4b30ae885 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -389,7 +389,10 @@ struct tcpm_port {
>  	unsigned int operating_snk_mw;
>  	bool update_sink_caps;
>  
> -	/* Requested current / voltage */
> +	/* Requested current / voltage to the port partner */
> +	u32 req_current_limit;
> +	u32 req_supply_voltage;
> +	/* Actual current / voltage limit of the local port */
>  	u32 current_limit;
>  	u32 supply_voltage;
>  
> @@ -2435,8 +2438,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
>  		case SNK_TRANSITION_SINK:
>  			if (port->vbus_present) {
>  				tcpm_set_current_limit(port,
> -						       port->current_limit,
> -						       port->supply_voltage);
> +						       port->req_current_limit,
> +						       port->req_supply_voltage);
>  				port->explicit_contract = true;
>  				tcpm_set_auto_vbus_discharge_threshold(port,
>  								       TYPEC_PWR_MODE_PD,
> @@ -2545,8 +2548,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
>  			break;
>  		case SNK_NEGOTIATE_PPS_CAPABILITIES:
>  			port->pps_data.active = true;
> -			port->supply_voltage = port->pps_data.out_volt;
> -			port->current_limit = port->pps_data.op_curr;
> +			port->req_supply_voltage = port->pps_data.out_volt;
> +			port->req_current_limit = port->pps_data.op_curr;
>  			tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
>  			break;
>  		case SOFT_RESET_SEND:
> @@ -3195,8 +3198,8 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>  			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>  	}
>  
> -	port->current_limit = ma;
> -	port->supply_voltage = mv;
> +	port->req_current_limit = ma;
> +	port->req_supply_voltage = mv;
>  
>  	return 0;
>  }
> -- 
> 2.31.1.295.g9ea45b61b8-goog

-- 
heikki