All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/6] Fixes for tcpm-source-psy- and pSnkStby
@ 2021-04-06  1:36 Badhri Jagan Sridharan
  2021-04-06  1:36 ` [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply Badhri Jagan Sridharan
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Badhri Jagan Sridharan @ 2021-04-06  1:36 UTC (permalink / raw)
  To: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, Rob Herring,
	Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso, Badhri Jagan Sridharan

Hi,

(1) and (2) in the series addresses the problem that Adam Thomson
had pointed out in:
https://lore.kernel.org/linux-usb/PR3PR10MB4142450638A8E07A33E475B080689@PR3PR10MB4142.EURPRD10.PROD.OUTLOOK.COM/

(3) updates the power_supply_changed based on changes
made through (1) and (2)

(4) (5) (6) makes TCPM comply pSnkStby requirement for both fast
and slow charging loops. This was also previously sent as part
of https://lore.kernel.org/patchwork/patch/1283928/

Since the patches were dependent on each other sending them this
way.

Badhri Jagan Sridharan (6):
  usb: typec: tcpm: Address incorrect values of tcpm psy for fixed
    supply
  usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply
  usb: typec: tcpm: update power supply once partner accepts
  usb: typec: tcpm: Honour pSnkStdby requirement during negotiation
  usb: typec: tcpm: Allow slow charging loops to comply to pSnkStby
  Documentation: connector: Add slow-charger-loop definition

 .../bindings/connector/usb-connector.yaml     |   7 +
 drivers/usb/typec/tcpm/tcpm.c                 | 136 ++++++++++++------
 include/linux/usb/pd.h                        |   2 +
 3 files changed, 99 insertions(+), 46 deletions(-)

-- 
2.31.0.208.g409f899ff0-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply
  2021-04-06  1:36 [PATCH v1 0/6] Fixes for tcpm-source-psy- and pSnkStby Badhri Jagan Sridharan
@ 2021-04-06  1:36 ` Badhri Jagan Sridharan
  2021-04-06  1:56   ` Guenter Roeck
  2021-04-07 16:04   ` Adam Thomson
  2021-04-06  1:36 ` [PATCH v1 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply Badhri Jagan Sridharan
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 16+ messages in thread
From: Badhri Jagan Sridharan @ 2021-04-06  1:36 UTC (permalink / raw)
  To: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, Rob Herring,
	Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso, 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>
---
 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..03eca5061132 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;
+	/* Acutal 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.0.208.g409f899ff0-goog


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v1 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply
  2021-04-06  1:36 [PATCH v1 0/6] Fixes for tcpm-source-psy- and pSnkStby Badhri Jagan Sridharan
  2021-04-06  1:36 ` [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply Badhri Jagan Sridharan
@ 2021-04-06  1:36 ` Badhri Jagan Sridharan
  2021-04-06 14:14   ` Greg Kroah-Hartman
  2021-04-07 16:07   ` Adam Thomson
  2021-04-06  1:36 ` [PATCH v1 3/6] usb: typec: tcpm: update power supply once partner accepts Badhri Jagan Sridharan
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 16+ messages in thread
From: Badhri Jagan Sridharan @ 2021-04-06  1:36 UTC (permalink / raw)
  To: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, Rob Herring,
	Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso, 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>
---
 drivers/usb/typec/tcpm/tcpm.c | 84 ++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 03eca5061132..d43774cc2ccf 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -269,11 +269,22 @@ struct pd_mode_data {
 };
 
 struct pd_pps_data {
+	/* Actual min voltage at the local port */
 	u32 min_volt;
+	/* Requested min voltage to the port partner */
+	u32 req_min_volt;
+	/* Actual max voltage at the local port */
 	u32 max_volt;
+	/* Requested max voltage to the port partner */
+	u32 req_max_volt;
+	/* Actual max current at the local port */
 	u32 max_curr;
-	u32 out_volt;
-	u32 op_curr;
+	/* Requested max current of the port partner */
+	u32 req_max_curr;
+	/* Requested output voltage to the port partner */
+	u32 req_out_volt;
+	/* Requested operating current to the port partner */
+	u32 req_op_curr;
 	bool supported;
 	bool active;
 };
@@ -2498,8 +2509,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 +2559,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 +3122,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 +3259,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 +3309,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 +5443,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 +5461,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 +5480,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 +5504,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 +5522,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 +5542,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 +5603,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.0.208.g409f899ff0-goog


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v1 3/6] usb: typec: tcpm: update power supply once partner accepts
  2021-04-06  1:36 [PATCH v1 0/6] Fixes for tcpm-source-psy- and pSnkStby Badhri Jagan Sridharan
  2021-04-06  1:36 ` [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply Badhri Jagan Sridharan
  2021-04-06  1:36 ` [PATCH v1 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply Badhri Jagan Sridharan
@ 2021-04-06  1:36 ` Badhri Jagan Sridharan
  2021-04-06  1:57   ` Guenter Roeck
  2021-04-07 16:08   ` Adam Thomson
  2021-04-06  1:36 ` [PATCH v1 4/6] usb: typec: tcpm: Honour pSnkStdby requirement during negotiation Badhri Jagan Sridharan
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 16+ messages in thread
From: Badhri Jagan Sridharan @ 2021-04-06  1:36 UTC (permalink / raw)
  To: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, Rob Herring,
	Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso, Badhri Jagan Sridharan

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

Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 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 d43774cc2ccf..7708b01009cb 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -2564,6 +2564,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:
@@ -3132,7 +3133,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;
@@ -3557,8 +3557,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.0.208.g409f899ff0-goog


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v1 4/6] usb: typec: tcpm: Honour pSnkStdby requirement during negotiation
  2021-04-06  1:36 [PATCH v1 0/6] Fixes for tcpm-source-psy- and pSnkStby Badhri Jagan Sridharan
                   ` (2 preceding siblings ...)
  2021-04-06  1:36 ` [PATCH v1 3/6] usb: typec: tcpm: update power supply once partner accepts Badhri Jagan Sridharan
@ 2021-04-06  1:36 ` Badhri Jagan Sridharan
  2021-04-06  1:36 ` [PATCH v1 5/6] usb: typec: tcpm: Allow slow charging loops to comply to pSnkStby Badhri Jagan Sridharan
  2021-04-06  1:36 ` [PATCH v1 6/6] Documentation: connector: Add slow-charger-loop definition Badhri Jagan Sridharan
  5 siblings, 0 replies; 16+ messages in thread
From: Badhri Jagan Sridharan @ 2021-04-06  1:36 UTC (permalink / raw)
  To: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, Rob Herring,
	Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso, 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 7708b01009cb..de9e57a7a929 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -4127,6 +4127,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.0.208.g409f899ff0-goog


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v1 5/6] usb: typec: tcpm: Allow slow charging loops to comply to pSnkStby
  2021-04-06  1:36 [PATCH v1 0/6] Fixes for tcpm-source-psy- and pSnkStby Badhri Jagan Sridharan
                   ` (3 preceding siblings ...)
  2021-04-06  1:36 ` [PATCH v1 4/6] usb: typec: tcpm: Honour pSnkStdby requirement during negotiation Badhri Jagan Sridharan
@ 2021-04-06  1:36 ` Badhri Jagan Sridharan
  2021-04-06  1:36 ` [PATCH v1 6/6] Documentation: connector: Add slow-charger-loop definition Badhri Jagan Sridharan
  5 siblings, 0 replies; 16+ messages in thread
From: Badhri Jagan Sridharan @ 2021-04-06  1:36 UTC (permalink / raw)
  To: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, Rob Herring,
	Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso, 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 de9e57a7a929..6aa314005f57 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -455,6 +455,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 */
@@ -4043,9 +4049,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;
@@ -4157,6 +4166,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);
@@ -5759,6 +5770,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.0.208.g409f899ff0-goog


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v1 6/6] Documentation: connector: Add slow-charger-loop definition
  2021-04-06  1:36 [PATCH v1 0/6] Fixes for tcpm-source-psy- and pSnkStby Badhri Jagan Sridharan
                   ` (4 preceding siblings ...)
  2021-04-06  1:36 ` [PATCH v1 5/6] usb: typec: tcpm: Allow slow charging loops to comply to pSnkStby Badhri Jagan Sridharan
@ 2021-04-06  1:36 ` Badhri Jagan Sridharan
  5 siblings, 0 replies; 16+ messages in thread
From: Badhri Jagan Sridharan @ 2021-04-06  1:36 UTC (permalink / raw)
  To: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, Rob Herring,
	Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso, 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.0.208.g409f899ff0-goog


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply
  2021-04-06  1:36 ` [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply Badhri Jagan Sridharan
@ 2021-04-06  1:56   ` Guenter Roeck
  2021-04-07 16:04   ` Adam Thomson
  1 sibling, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2021-04-06  1:56 UTC (permalink / raw)
  To: Badhri Jagan Sridharan, Heikki Krogerus, Greg Kroah-Hartman,
	Rob Herring, Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso

On 4/5/21 6:36 PM, 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>
> ---
>  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..03eca5061132 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;
> +	/* Acutal current / voltage limit of the local port */

Actual

Otherwise makes sense.

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

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 3/6] usb: typec: tcpm: update power supply once partner accepts
  2021-04-06  1:36 ` [PATCH v1 3/6] usb: typec: tcpm: update power supply once partner accepts Badhri Jagan Sridharan
@ 2021-04-06  1:57   ` Guenter Roeck
  2021-04-07 16:08   ` Adam Thomson
  1 sibling, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2021-04-06  1:57 UTC (permalink / raw)
  To: Badhri Jagan Sridharan, Heikki Krogerus, Greg Kroah-Hartman,
	Rob Herring, Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso

On 4/5/21 6:36 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.
> 
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  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 d43774cc2ccf..7708b01009cb 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2564,6 +2564,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:
> @@ -3132,7 +3133,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;
> @@ -3557,8 +3557,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);

The reason for this change is missing in the patch description,
or am I missing something ?

Thanks,
Guenter

>  }
>  
>  static void tcpm_detach(struct tcpm_port *port)
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply
  2021-04-06  1:36 ` [PATCH v1 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply Badhri Jagan Sridharan
@ 2021-04-06 14:14   ` Greg Kroah-Hartman
  2021-04-07 16:07   ` Adam Thomson
  1 sibling, 0 replies; 16+ messages in thread
From: Greg Kroah-Hartman @ 2021-04-06 14:14 UTC (permalink / raw)
  To: Badhri Jagan Sridharan
  Cc: Guenter Roeck, Heikki Krogerus, Rob Herring, Adam Thomson,
	linux-usb, linux-kernel, devicetree, Kyle Tso

On Mon, Apr 05, 2021 at 06:36:39PM -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>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 84 ++++++++++++++++++++---------------
>  1 file changed, 49 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 03eca5061132..d43774cc2ccf 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -269,11 +269,22 @@ struct pd_mode_data {
>  };
>  
>  struct pd_pps_data {
> +	/* Actual min voltage at the local port */
>  	u32 min_volt;
> +	/* Requested min voltage to the port partner */
> +	u32 req_min_volt;
> +	/* Actual max voltage at the local port */
>  	u32 max_volt;
> +	/* Requested max voltage to the port partner */
> +	u32 req_max_volt;
> +	/* Actual max current at the local port */
>  	u32 max_curr;
> -	u32 out_volt;
> -	u32 op_curr;
> +	/* Requested max current of the port partner */
> +	u32 req_max_curr;
> +	/* Requested output voltage to the port partner */
> +	u32 req_out_volt;
> +	/* Requested operating current to the port partner */
> +	u32 req_op_curr;

Shouldn't you just document this all properly in a kerneldoc header
right above the structure?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply
  2021-04-06  1:36 ` [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply Badhri Jagan Sridharan
  2021-04-06  1:56   ` Guenter Roeck
@ 2021-04-07 16:04   ` Adam Thomson
  2021-04-07 20:13     ` Badhri Jagan Sridharan
  1 sibling, 1 reply; 16+ messages in thread
From: Adam Thomson @ 2021-04-07 16:04 UTC (permalink / raw)
  To: Badhri Jagan Sridharan, Guenter Roeck, Heikki Krogerus,
	Greg Kroah-Hartman, Rob Herring, Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso

On 06 April 2021 02:37, 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>
> ---

Looks sensible, typo aside:

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

>  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..03eca5061132 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;
> +	/* Acutal 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.0.208.g409f899ff0-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v1 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply
  2021-04-06  1:36 ` [PATCH v1 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply Badhri Jagan Sridharan
  2021-04-06 14:14   ` Greg Kroah-Hartman
@ 2021-04-07 16:07   ` Adam Thomson
  2021-04-07 20:12     ` Badhri Jagan Sridharan
  1 sibling, 1 reply; 16+ messages in thread
From: Adam Thomson @ 2021-04-07 16:07 UTC (permalink / raw)
  To: Badhri Jagan Sridharan, Guenter Roeck, Heikki Krogerus,
	Greg Kroah-Hartman, Rob Herring, Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso

On 06 April 2021 02:37, 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>

>  drivers/usb/typec/tcpm/tcpm.c | 84 ++++++++++++++++++++---------------
>  1 file changed, 49 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 03eca5061132..d43774cc2ccf 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -269,11 +269,22 @@ struct pd_mode_data {
>  };
> 
>  struct pd_pps_data {
> +	/* Actual min voltage at the local port */
>  	u32 min_volt;
> +	/* Requested min voltage to the port partner */
> +	u32 req_min_volt;
> +	/* Actual max voltage at the local port */
>  	u32 max_volt;
> +	/* Requested max voltage to the port partner */
> +	u32 req_max_volt;
> +	/* Actual max current at the local port */
>  	u32 max_curr;
> -	u32 out_volt;
> -	u32 op_curr;
> +	/* Requested max current of the port partner */
> +	u32 req_max_curr;
> +	/* Requested output voltage to the port partner */
> +	u32 req_out_volt;
> +	/* Requested operating current to the port partner */
> +	u32 req_op_curr;
>  	bool supported;
>  	bool active;
>  };
> @@ -2498,8 +2509,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 +2559,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 +3122,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 +3259,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 +3309,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 +5443,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 +5461,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 +5480,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 +5504,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 +5522,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 +5542,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 +5603,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.0.208.g409f899ff0-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v1 3/6] usb: typec: tcpm: update power supply once partner accepts
  2021-04-06  1:36 ` [PATCH v1 3/6] usb: typec: tcpm: update power supply once partner accepts Badhri Jagan Sridharan
  2021-04-06  1:57   ` Guenter Roeck
@ 2021-04-07 16:08   ` Adam Thomson
  2021-04-07 20:11     ` Badhri Jagan Sridharan
  1 sibling, 1 reply; 16+ messages in thread
From: Adam Thomson @ 2021-04-07 16:08 UTC (permalink / raw)
  To: Badhri Jagan Sridharan, Guenter Roeck, Heikki Krogerus,
	Greg Kroah-Hartman, Rob Herring, Adam Thomson
  Cc: linux-usb, linux-kernel, devicetree, Kyle Tso

On 06 April 2021 02:37, 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.
> 
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through
> power_supply")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---

Missing commit information aside:

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

>  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 d43774cc2ccf..7708b01009cb 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2564,6 +2564,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:
> @@ -3132,7 +3133,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;
> @@ -3557,8 +3557,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.0.208.g409f899ff0-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 3/6] usb: typec: tcpm: update power supply once partner accepts
  2021-04-07 16:08   ` Adam Thomson
@ 2021-04-07 20:11     ` Badhri Jagan Sridharan
  0 siblings, 0 replies; 16+ messages in thread
From: Badhri Jagan Sridharan @ 2021-04-07 20:11 UTC (permalink / raw)
  To: Adam Thomson
  Cc: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, Rob Herring,
	linux-usb, linux-kernel, devicetree, Kyle Tso

>> The reason for this change is missing in the patch description,
>> or am I missing something ?

Ah yes forgot to state this in the commit description which I have
updated in V2.
Removed a redundant power_supply_changed as one was already made a
couple of lines before this one.

Thanks,
Badhri

On Wed, Apr 7, 2021 at 9:08 AM Adam Thomson
<Adam.Thomson.Opensource@diasemi.com> wrote:
>
> On 06 April 2021 02:37, 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.
> >
> > Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through
> > power_supply")
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
>
> Missing commit information aside:
>
> Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>
> >  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 d43774cc2ccf..7708b01009cb 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -2564,6 +2564,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:
> > @@ -3132,7 +3133,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;
> > @@ -3557,8 +3557,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.0.208.g409f899ff0-goog
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply
  2021-04-07 16:07   ` Adam Thomson
@ 2021-04-07 20:12     ` Badhri Jagan Sridharan
  0 siblings, 0 replies; 16+ messages in thread
From: Badhri Jagan Sridharan @ 2021-04-07 20:12 UTC (permalink / raw)
  To: Adam Thomson
  Cc: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, Rob Herring,
	linux-usb, linux-kernel, devicetree, Kyle Tso

Hi Greg,

Moved to kerneldoc header in V2.

Thanks,
Badhri

On Wed, Apr 7, 2021 at 9:07 AM Adam Thomson
<Adam.Thomson.Opensource@diasemi.com> wrote:
>
> On 06 April 2021 02:37, 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>
>
> >  drivers/usb/typec/tcpm/tcpm.c | 84 ++++++++++++++++++++---------------
> >  1 file changed, 49 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 03eca5061132..d43774cc2ccf 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -269,11 +269,22 @@ struct pd_mode_data {
> >  };
> >
> >  struct pd_pps_data {
> > +     /* Actual min voltage at the local port */
> >       u32 min_volt;
> > +     /* Requested min voltage to the port partner */
> > +     u32 req_min_volt;
> > +     /* Actual max voltage at the local port */
> >       u32 max_volt;
> > +     /* Requested max voltage to the port partner */
> > +     u32 req_max_volt;
> > +     /* Actual max current at the local port */
> >       u32 max_curr;
> > -     u32 out_volt;
> > -     u32 op_curr;
> > +     /* Requested max current of the port partner */
> > +     u32 req_max_curr;
> > +     /* Requested output voltage to the port partner */
> > +     u32 req_out_volt;
> > +     /* Requested operating current to the port partner */
> > +     u32 req_op_curr;
> >       bool supported;
> >       bool active;
> >  };
> > @@ -2498,8 +2509,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 +2559,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 +3122,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 +3259,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 +3309,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 +5443,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 +5461,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 +5480,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 +5504,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 +5522,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 +5542,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 +5603,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.0.208.g409f899ff0-goog
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply
  2021-04-07 16:04   ` Adam Thomson
@ 2021-04-07 20:13     ` Badhri Jagan Sridharan
  0 siblings, 0 replies; 16+ messages in thread
From: Badhri Jagan Sridharan @ 2021-04-07 20:13 UTC (permalink / raw)
  To: Adam Thomson
  Cc: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, Rob Herring,
	linux-usb, linux-kernel, devicetree, Kyle Tso

Hi Guenter and Adam,

Thanks for the reviews !
Fixed up the typo in V2.

Thanks,
Badhri

On Wed, Apr 7, 2021 at 9:04 AM Adam Thomson
<Adam.Thomson.Opensource@diasemi.com> wrote:
>
> On 06 April 2021 02:37, 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>
> > ---
>
> Looks sensible, typo aside:
>
> Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>
> >  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..03eca5061132 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;
> > +     /* Acutal 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.0.208.g409f899ff0-goog
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-04-07 20:14 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06  1:36 [PATCH v1 0/6] Fixes for tcpm-source-psy- and pSnkStby Badhri Jagan Sridharan
2021-04-06  1:36 ` [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply Badhri Jagan Sridharan
2021-04-06  1:56   ` Guenter Roeck
2021-04-07 16:04   ` Adam Thomson
2021-04-07 20:13     ` Badhri Jagan Sridharan
2021-04-06  1:36 ` [PATCH v1 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply Badhri Jagan Sridharan
2021-04-06 14:14   ` Greg Kroah-Hartman
2021-04-07 16:07   ` Adam Thomson
2021-04-07 20:12     ` Badhri Jagan Sridharan
2021-04-06  1:36 ` [PATCH v1 3/6] usb: typec: tcpm: update power supply once partner accepts Badhri Jagan Sridharan
2021-04-06  1:57   ` Guenter Roeck
2021-04-07 16:08   ` Adam Thomson
2021-04-07 20:11     ` Badhri Jagan Sridharan
2021-04-06  1:36 ` [PATCH v1 4/6] usb: typec: tcpm: Honour pSnkStdby requirement during negotiation Badhri Jagan Sridharan
2021-04-06  1:36 ` [PATCH v1 5/6] usb: typec: tcpm: Allow slow charging loops to comply to pSnkStby Badhri Jagan Sridharan
2021-04-06  1:36 ` [PATCH v1 6/6] Documentation: connector: Add slow-charger-loop definition Badhri Jagan Sridharan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.