Re: [PATCH v2] usb: typec: tcpm: set initial svdm version based on pd revision

[Heikki Krogerus]

On Mon, Jul 31, 2023 at 04:59:23PM +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."
> 
> Fixes: c34e85fa69b9 ("usb: typec: tcpm: Send DISCOVER_IDENTITY from dedicated work")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: RD Babiera <rdbabiera@xxxxxxxxxx>

Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

> ---
> Changes since v1:
> * Fixed styling errors.
> ---
>  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..5024354a0fe0 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -3928,6 +3928,29 @@ 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,10 +4188,12 @@ 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;
> +		}
>  
>  		/*
>  		 * 6.3.5
> @@ -4455,10 +4480,12 @@ 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);
>  		break;
> 
> base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
> -- 
> 2.41.0.585.gd2178a4bd4-goog

-- 
heikki