Received-SPF: pass (google.com: domain of linux-kernel+bounces-63311-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240212-usb-fix-renegade-v1-1-22c43c88d635@kernel.org>
In-Reply-To: <20240212-usb-fix-renegade-v1-1-22c43c88d635@kernel.org>
From: Badhri Jagan Sridharan <badhri@google.com>
Date: Tue, 13 Feb 2024 02:09:16 -0800
Message-ID: <CAPTae5LOresaK3Epd4R_ifpx2kw21tubQDENg4OMrhJWsar-oA@mail.gmail.com>
Subject: Re: [PATCH] usb: typec: tpcm: Fix issues with power being removed
 during reset
To: Mark Brown <broonie@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>, Heikki Krogerus <heikki.krogerus@linux.intel.com>, 
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, =?UTF-8?Q?G=C3=A1bor_Stefanik?= <netrolller.3d@gmail.com>, 
	rdbabiera@google.com, amitsd@google.com, linux-usb@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Mark,

While HI-Zing CC pins disrupts power for batteryless devices, not
Hi-Zing CC pins would prevent clean error recovery for self powered
devices which is why "usb: typec: tcpm: fix cc role at port reset" was reve=
rted.
Please note that the breakage in error recovery behavior is a
regression as well.
Hi-Zing CC pins would make the port partner recognize it as disconnect
and will result in bringup the connection back cleanly.

How about leveraging "self-powered" device tree property and Hi-Zing
CC pins only when using "self-powered" ?
This should help devices which don't have batteries while NOT regressing
the error recovery behavior for the self powered devices.

--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -4897,7 +4897,11 @@ static void run_state_machine(struct tcpm_port *port=
)
                break;
        case PORT_RESET:
                tcpm_reset_port(port);
-               tcpm_set_cc(port, TYPEC_CC_OPEN);
+               if (port->self_powered)
+                       tcpm_set_cc(port, TYPEC_CC_OPEN);
+               else
+                       tcpm_set_cc(port, tcpm_default_state(port) =3D=3D
SNK_UNATTACHED ?
+                               TYPEC_CC_RD : tcpm_rp_cc(port));
                tcpm_set_state(port, PORT_RESET_WAIT_OFF,
                               PD_T_ERROR_RECOVERY);

Thanks,
Badhri


On Mon, Feb 12, 2024 at 10:42=E2=80=AFAM Mark Brown <broonie@kernel.org> wr=
ote:
>
> Since the merge of b717dfbf73e8 ("Revert "usb: typec: tcpm: fix
> cc role at port reset"") into mainline the LibreTech Renegade
> Elite/Firefly has died during boot, the main symptom observed in testing
> is a sudden stop in console output.  G=C3=A1bor Stefanik identified in re=
view
> that the patch would cause power to be removed from devices without
> batteries (like this board), observing that while the patch is correct
> according to the spec this appears to be an oversight in the spec.
>
> Given that the change makes previously working systems unusable let's
> revert it, there was some discussion of identifying systems that have
> alternative power and implementing the standards conforming behaviour in
> only that case.
>
> Fixes: b717dfbf73e8 ("Revert "usb: typec: tcpm: fix cc role at port reset=
"")
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.=
c
> index f7d7daa60c8d..a0978ed1a257 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4876,7 +4876,8 @@ static void run_state_machine(struct tcpm_port *por=
t)
>                 break;
>         case PORT_RESET:
>                 tcpm_reset_port(port);
> -               tcpm_set_cc(port, TYPEC_CC_OPEN);
> +               tcpm_set_cc(port, tcpm_default_state(port) =3D=3D SNK_UNA=
TTACHED ?
> +                           TYPEC_CC_RD : tcpm_rp_cc(port));
>                 tcpm_set_state(port, PORT_RESET_WAIT_OFF,
>                                PD_T_ERROR_RECOVERY);
>                 break;
>
> ---
> base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
> change-id: 20240212-usb-fix-renegade-837d35cfc0c2
>
> Best regards,
> --
> Mark Brown <broonie@kernel.org>
>