diff mbox series

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

Message ID 20210407200723.1914388-1-badhri@google.com
State New
Headers show
Series [v2,1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply | expand

Commit Message

Badhri Jagan Sridharan April 7, 2021, 8:07 p.m. UTC
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(-)

Comments

Heikki Krogerus April 8, 2021, 6:51 a.m. UTC | #1
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 Krogerus April 8, 2021, 7:47 a.m. UTC | #2
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,
Guenter Roeck April 10, 2021, 2:11 a.m. UTC | #3
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 */
>
Badhri Jagan Sridharan April 14, 2021, 12:58 a.m. UTC | #4
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
diff mbox series

Patch

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