diff mbox series

usb: typec: tpcm: Fix issues with power being removed during reset

Message ID 20240212-usb-fix-renegade-v1-1-22c43c88d635@kernel.org (mailing list archive)
State Accepted
Commit 69f89168b310878be82d7d97bc0d22068ad858c0
Headers show
Series usb: typec: tpcm: Fix issues with power being removed during reset | expand

Commit Message

Mark Brown Feb. 12, 2024, 6:42 p.m. UTC 
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ábor Stefanik identified in review
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(-)


---
base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
change-id: 20240212-usb-fix-renegade-837d35cfc0c2

Best regards,

Comments

Badhri Jagan Sridharan Feb. 13, 2024, 10:09 a.m. UTC | #1 
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 reverted.
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) ==
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 AM Mark Brown <broonie@kernel.org> wrote:
>
> 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ábor Stefanik identified in review
> 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 *port)
>                 break;
>         case PORT_RESET:
>                 tcpm_reset_port(port);
> -               tcpm_set_cc(port, TYPEC_CC_OPEN);
> +               tcpm_set_cc(port, tcpm_default_state(port) == SNK_UNATTACHED ?
> +                           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>
>
Mark Brown Feb. 13, 2024, 1:13 p.m. UTC | #2 
On Tue, Feb 13, 2024 at 02:09:16AM -0800, Badhri Jagan Sridharan wrote:
> Hi Mark,

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

> 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 reverted.
> 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.

I don't super care so long as the boards I care about continue to
function, I submitted this patch because the only response to my report
about the rk3399-roc-pc having been broken in mainline was a
confirmation that the failure was expected.  As I noted in the commit
log checking if there is an alternative power source does seem like a
viable option here, I am not particularly familiar with this code.
Greg KH Feb. 17, 2024, 3:54 p.m. UTC | #3 
On Mon, Feb 12, 2024 at 06:42:13PM +0000, Mark Brown wrote:
> 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ábor Stefanik identified in review
> 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 *port)
>  		break;
>  	case PORT_RESET:
>  		tcpm_reset_port(port);
> -		tcpm_set_cc(port, TYPEC_CC_OPEN);
> +		tcpm_set_cc(port, tcpm_default_state(port) == SNK_UNATTACHED ?
> +			    TYPEC_CC_RD : tcpm_rp_cc(port));
>  		tcpm_set_state(port, PORT_RESET_WAIT_OFF,
>  			       PD_T_ERROR_RECOVERY);
>  		break;
> 

Dueling reverts, fun!

:(

Badhri, can you either ack this, or submit your proposed change so as to
get this back working again?

thanks,

greg k-h
diff mbox series

Patch

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 *port)
 		break;
 	case PORT_RESET:
 		tcpm_reset_port(port);
-		tcpm_set_cc(port, TYPEC_CC_OPEN);
+		tcpm_set_cc(port, tcpm_default_state(port) == SNK_UNATTACHED ?
+			    TYPEC_CC_RD : tcpm_rp_cc(port));
 		tcpm_set_state(port, PORT_RESET_WAIT_OFF,
 			       PD_T_ERROR_RECOVERY);
 		break;