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@xxxxxxxxxxxxxxx
> Signed-off-by: RD Babiera <rdbabiera@xxxxxxxxxx>
> ---
> 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,
--
heikki
