diff mbox series

[v1] usb: typec: tcpm: set initial svdm version based on pd revision

Message ID 20230726020810.1408299-1-rdbabiera@google.com (mailing list archive)
State Superseded
Headers show
Series [v1] usb: typec: tcpm: set initial svdm version based on pd revision | expand

Commit Message

RD Babiera July 26, 2023, 2:08 a.m. UTC 
When sending Discover Identity messages to a Port Partner that uses Power
Delivery v2 and SVDM v1, we currently send PD v2 messages with SVDM v2.0,
expecting the port partner to respond with its highest supported SVDM
version as stated in Section 6.4.4.2.3 in the Power Delivery v3
specification. However, sending SVDM v2 to some Power Delivery v2 port
partners results in a NAK whereas sending SVDM v1 does not.

NAK messages can be handled by the initiator (PD v3 section 6.4.4.2.5.1),
and one solution could be to resend Discover Identity on a lower SVDM
version if possible. But, Section 6.4.4.3 of PD v2 states that "A NAK
response Should be taken as an indication not to retry that particular
Command."

Instead, we can set the SVDM version to the maximum one supported by the
negotiated PD revision. When operating in PD v2, this obeys Section
6.4.4.2.3, which states the SVDM field "Shall be set to zero to indicate
Version 1.0." In PD v3, the SVDM field "Shall be set to 01b to indicate
Version 2.0."

Fixes: c34e85fa69b9 ("usb: typec: tcpm: Send DISCOVER_IDENTITY from dedicated work")
Cc: stable@vger.kernel.org
Signed-off-by: RD Babiera <rdbabiera@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)


base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c

Comments

Heikki Krogerus July 31, 2023, 11:16 a.m. UTC | #1 
Hi,

I'm sorry to keep you waiting.

On Wed, Jul 26, 2023 at 02:08:07AM +0000, RD Babiera wrote:
> When sending Discover Identity messages to a Port Partner that uses Power
> Delivery v2 and SVDM v1, we currently send PD v2 messages with SVDM v2.0,
> expecting the port partner to respond with its highest supported SVDM
> version as stated in Section 6.4.4.2.3 in the Power Delivery v3
> specification. However, sending SVDM v2 to some Power Delivery v2 port
> partners results in a NAK whereas sending SVDM v1 does not.
> 
> NAK messages can be handled by the initiator (PD v3 section 6.4.4.2.5.1),
> and one solution could be to resend Discover Identity on a lower SVDM
> version if possible. But, Section 6.4.4.3 of PD v2 states that "A NAK
> response Should be taken as an indication not to retry that particular
> Command."
> 
> Instead, we can set the SVDM version to the maximum one supported by the
> negotiated PD revision. When operating in PD v2, this obeys Section
> 6.4.4.2.3, which states the SVDM field "Shall be set to zero to indicate
> Version 1.0." In PD v3, the SVDM field "Shall be set to 01b to indicate
> Version 2.0."

This makes sense to me, but couple of nitpicks below.

> Fixes: c34e85fa69b9 ("usb: typec: tcpm: Send DISCOVER_IDENTITY from dedicated work")
> Cc: stable@vger.kernel.org
> Signed-off-by: RD Babiera <rdbabiera@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 829d75ebab42..5b0a428fcf5d 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -3928,6 +3928,31 @@ static enum typec_cc_status tcpm_pwr_opmode_to_rp(enum typec_pwr_opmode opmode)
>  	}
>  }
>  
> +static void tcpm_set_initial_svdm_version(struct tcpm_port *port)
> +{
> +	switch (port->negotiated_rev) {
> +	case PD_REV30:
> +		break;
> +	/*
> +	 * 6.4.4.2.3 Structured VDM Version
> +	 * 2.0 states "At this time, there is only one version (1.0) defined.
> +	 * This field Shall be set to zero to indicate Version 1.0."
> +	 * 3.0 states "This field Shall be set to 01b to indicate Version 2.0."
> +	 * To ensure that we follow the Power Delivery revision we are currently
> +	 * operating on, downgrade the SVDM version to the highest one supported
> +	 * by the Power Delivery revision.
> +	 */
> +	case PD_REV20:
> +		typec_partner_set_svdm_version(port->partner,
> +					       SVDM_VER_1_0);

One line is enough.

> +		break;
> +	default:
> +		typec_partner_set_svdm_version(port->partner,
> +					       SVDM_VER_1_0);

Ditto.

> +		break;
> +	}
> +}
> +
>  static void run_state_machine(struct tcpm_port *port)
>  {
>  	int ret;
> @@ -4165,9 +4190,10 @@ static void run_state_machine(struct tcpm_port *port)
>  		 * For now, this driver only supports SOP for DISCOVER_IDENTITY, thus using
>  		 * port->explicit_contract to decide whether to send the command.
>  		 */
> -		if (port->explicit_contract)
> +		if (port->explicit_contract) {
> +			tcpm_set_initial_svdm_version(port);
>  			mod_send_discover_delayed_work(port, 0);
> -		else
> +		} else
>  			port->send_discover = false;

The else statement needs to be wrapped in curly brackets in this case
since the if statement had them.

>  		/*
> @@ -4455,9 +4481,10 @@ static void run_state_machine(struct tcpm_port *port)
>  		 * For now, this driver only supports SOP for DISCOVER_IDENTITY, thus using
>  		 * port->explicit_contract.
>  		 */
> -		if (port->explicit_contract)
> +		if (port->explicit_contract) {
> +			tcpm_set_initial_svdm_version(port);
>  			mod_send_discover_delayed_work(port, 0);
> -		else
> +		} else
>  			port->send_discover = false;

Ditto.

thanks,
RD Babiera July 31, 2023, 4:55 p.m. UTC | #2 
On Mon, Jul 31, 2023 at 4:21 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> This makes sense to me, but couple of nitpicks below.

Will fix, thanks for the feedback!

--
Best,
RD
diff mbox series

Patch

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 829d75ebab42..5b0a428fcf5d 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -3928,6 +3928,31 @@  static enum typec_cc_status tcpm_pwr_opmode_to_rp(enum typec_pwr_opmode opmode)
 	}
 }
 
+static void tcpm_set_initial_svdm_version(struct tcpm_port *port)
+{
+	switch (port->negotiated_rev) {
+	case PD_REV30:
+		break;
+	/*
+	 * 6.4.4.2.3 Structured VDM Version
+	 * 2.0 states "At this time, there is only one version (1.0) defined.
+	 * This field Shall be set to zero to indicate Version 1.0."
+	 * 3.0 states "This field Shall be set to 01b to indicate Version 2.0."
+	 * To ensure that we follow the Power Delivery revision we are currently
+	 * operating on, downgrade the SVDM version to the highest one supported
+	 * by the Power Delivery revision.
+	 */
+	case PD_REV20:
+		typec_partner_set_svdm_version(port->partner,
+					       SVDM_VER_1_0);
+		break;
+	default:
+		typec_partner_set_svdm_version(port->partner,
+					       SVDM_VER_1_0);
+		break;
+	}
+}
+
 static void run_state_machine(struct tcpm_port *port)
 {
 	int ret;
@@ -4165,9 +4190,10 @@  static void run_state_machine(struct tcpm_port *port)
 		 * For now, this driver only supports SOP for DISCOVER_IDENTITY, thus using
 		 * port->explicit_contract to decide whether to send the command.
 		 */
-		if (port->explicit_contract)
+		if (port->explicit_contract) {
+			tcpm_set_initial_svdm_version(port);
 			mod_send_discover_delayed_work(port, 0);
-		else
+		} else
 			port->send_discover = false;
 
 		/*
@@ -4455,9 +4481,10 @@  static void run_state_machine(struct tcpm_port *port)
 		 * For now, this driver only supports SOP for DISCOVER_IDENTITY, thus using
 		 * port->explicit_contract.
 		 */
-		if (port->explicit_contract)
+		if (port->explicit_contract) {
+			tcpm_set_initial_svdm_version(port);
 			mod_send_discover_delayed_work(port, 0);
-		else
+		} else
 			port->send_discover = false;
 
 		power_supply_changed(port->psy);