mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
@ 2024-10-22 17:28 Yanik Fuchs
  2024-10-23  9:05 ` Heikki Krogerus
  2024-10-24  3:13 ` Amit Sunil Dhamne
  0 siblings, 2 replies; 5+ messages in thread
From: Yanik Fuchs @ 2024-10-22 17:28 UTC (permalink / raw)
  To: heikki.krogerus, gregkh, rdbabiera, linux, kyletso, badhri,
	xu.yang_2, amitsd
  Cc: linux-usb, linux-kernel

Good Evening

Here is a Patch to resolve an issue with TCPM if Vbus was never low.
Please consider that this is one of my first kernel pull requests, I may have missunderstood the process.

Freundliche Grüsse
Best regards


Yanik Fuchs

---

From 604b97b6394b5643394bc63d4ac691c098c99c40 Mon Sep 17 00:00:00 2001
From: yfu <yanikfuchs@me.com>
Date: Tue, 22 Oct 2024 18:23:18 +0200
Subject: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low

Before this patch, tcpm went into SOFT_RESET state, if Vbus was never low
resulting in Hard_Reset, if power supply does not support USB_PD Soft_Reset.

In order to prevent this, I remove the Vbus check completely, so that 
we go as well into the SNK_WAIT_CAPABILITIES_TIMEOUT state. There, we send 
PD_CTRL_GET_SOURCE_CAP which many power supply do support.
(122968f8dda8 usb: typec: tcpm: avoid resets for missing source capability messages)

Additionally, I added SOFT_RESET (instead of Hard_Reset) as Fallback solution
if we still not have gotten any capabilities. Hard_Reset is now only done, 
if PD_CTRL_GET_SOURCE_CAP and SOFT_RESET fail to get capabilities.
---
 drivers/usb/typec/tcpm/tcpm.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index fc619478200f..c7a59c9f78ee 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -5038,14 +5038,8 @@ static void run_state_machine(struct tcpm_port *port)
 		 * were already in a stable contract before this boot.
 		 * Do this only once.
 		 */
-		if (port->vbus_never_low) {
-			port->vbus_never_low = false;
-			tcpm_set_state(port, SNK_SOFT_RESET,
+		tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
 				       PD_T_SINK_WAIT_CAP);
-		} else {
-			tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
-				       PD_T_SINK_WAIT_CAP);
-		}
 		break;
 	case SNK_WAIT_CAPABILITIES_TIMEOUT:
 		/*
@@ -5064,7 +5058,7 @@ static void run_state_machine(struct tcpm_port *port)
 		 * according to the specification.
 		 */
 		if (tcpm_pd_send_control(port, PD_CTRL_GET_SOURCE_CAP, TCPC_TX_SOP))
-			tcpm_set_state_cond(port, hard_reset_state(port), 0);
+			tcpm_set_state_cond(port, SNK_SOFT_RESET, 0);
 		else
 			tcpm_set_state(port, hard_reset_state(port), PD_T_SINK_WAIT_CAP);
 		break;
-- 
2.34.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
  2024-10-22 17:28 [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low Yanik Fuchs
@ 2024-10-23  9:05 ` Heikki Krogerus
  2024-10-24  3:13 ` Amit Sunil Dhamne
  1 sibling, 0 replies; 5+ messages in thread
From: Heikki Krogerus @ 2024-10-23  9:05 UTC (permalink / raw)
  To: Yanik Fuchs
  Cc: gregkh, rdbabiera, linux, kyletso, badhri, xu.yang_2, amitsd,
	linux-usb, linux-kernel

Hi Yanik,

On Tue, Oct 22, 2024 at 05:28:51PM +0000, Yanik Fuchs wrote:
> Good Evening
> 
> Here is a Patch to resolve an issue with TCPM if Vbus was never low.
> Please consider that this is one of my first kernel pull requests, I may have missunderstood the process.

Welcome aboard :)

Thank you for the patch. Unfortunately it is not properly formatted.
As this is a patch, you can't really comment it like this here.
Instead you should put any additional comments...

> Freundliche Grüsse
> Best regards
> 
> 
> Yanik Fuchs
> 
> ---
> 
> >From 604b97b6394b5643394bc63d4ac691c098c99c40 Mon Sep 17 00:00:00 2001
> From: yfu <yanikfuchs@me.com>
> Date: Tue, 22 Oct 2024 18:23:18 +0200
> Subject: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
> 
> Before this patch, tcpm went into SOFT_RESET state, if Vbus was never low
> resulting in Hard_Reset, if power supply does not support USB_PD Soft_Reset.
> 
> In order to prevent this, I remove the Vbus check completely, so that 
> we go as well into the SNK_WAIT_CAPABILITIES_TIMEOUT state. There, we send 
> PD_CTRL_GET_SOURCE_CAP which many power supply do support.
> (122968f8dda8 usb: typec: tcpm: avoid resets for missing source capability messages)
> 
> Additionally, I added SOFT_RESET (instead of Hard_Reset) as Fallback solution
> if we still not have gotten any capabilities. Hard_Reset is now only done, 
> if PD_CTRL_GET_SOURCE_CAP and SOFT_RESET fail to get capabilities.
> ---

... here after those three lines. The proper format, and the whole
development process is documented here:
https://docs.kernel.org/process/development-process.html

You have also not signed your patch with a Signed-off-by tag. The
importance of the signature in patches is explained in the fifth
section of the development process documentation, here:
https://docs.kernel.org/process/5.Posting.html

>  drivers/usb/typec/tcpm/tcpm.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index fc619478200f..c7a59c9f78ee 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -5038,14 +5038,8 @@ static void run_state_machine(struct tcpm_port *port)
>  		 * were already in a stable contract before this boot.
>  		 * Do this only once.
>  		 */
> -		if (port->vbus_never_low) {
> -			port->vbus_never_low = false;
> -			tcpm_set_state(port, SNK_SOFT_RESET,
> +		tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
>  				       PD_T_SINK_WAIT_CAP);

Here you should fix the alignment of the code so it matches the
parentheses. You can use the scripts/checkpatch.pl script, which is
part of the kernel source, to detect this kind of issues in your code
by supplying your patch to it.

> -		} else {
> -			tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
> -				       PD_T_SINK_WAIT_CAP);
> -		}
>  		break;
>  	case SNK_WAIT_CAPABILITIES_TIMEOUT:
>  		/*
> @@ -5064,7 +5058,7 @@ static void run_state_machine(struct tcpm_port *port)
>  		 * according to the specification.
>  		 */
>  		if (tcpm_pd_send_control(port, PD_CTRL_GET_SOURCE_CAP, TCPC_TX_SOP))
> -			tcpm_set_state_cond(port, hard_reset_state(port), 0);
> +			tcpm_set_state_cond(port, SNK_SOFT_RESET, 0);
>  		else
>  			tcpm_set_state(port, hard_reset_state(port), PD_T_SINK_WAIT_CAP);
>  		break;
> -- 
> 2.34.1

Otherwise the code looks very good to me, but I can't yet say if the
change is appropriate. Let's fix the patch format first.

Br,

-- 
heikki

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
  2024-10-22 17:28 [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low Yanik Fuchs
  2024-10-23  9:05 ` Heikki Krogerus
@ 2024-10-24  3:13 ` Amit Sunil Dhamne
  2024-10-24  8:54   ` AW: " Yanik Fuchs
  1 sibling, 1 reply; 5+ messages in thread
From: Amit Sunil Dhamne @ 2024-10-24  3:13 UTC (permalink / raw)
  To: Yanik Fuchs, heikki.krogerus, gregkh, rdbabiera, linux, kyletso,
	badhri, xu.yang_2, sebastian.reichel
  Cc: linux-usb, linux-kernel

Hi Yanik,

On 10/22/24 10:28 AM, Yanik Fuchs wrote:
> Good Evening
>
> Here is a Patch to resolve an issue with TCPM if Vbus was never low.
> Please consider that this is one of my first kernel pull requests, I may have missunderstood the process.
>
> Freundliche Grüsse
> Best regards
>
>
> Yanik Fuchs
>
> ---
>
>  From 604b97b6394b5643394bc63d4ac691c098c99c40 Mon Sep 17 00:00:00 2001
> From: yfu <yanikfuchs@me.com>
> Date: Tue, 22 Oct 2024 18:23:18 +0200
> Subject: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
>
> Before this patch, tcpm went into SOFT_RESET state, if Vbus was never low
> resulting in Hard_Reset, if power supply does not support USB_PD Soft_Reset.
>
> In order to prevent this, I remove the Vbus check completely, so that
> we go as well into the SNK_WAIT_CAPABILITIES_TIMEOUT state. There, we send
> PD_CTRL_GET_SOURCE_CAP which many power supply do support.
> (122968f8dda8 usb: typec: tcpm: avoid resets for missing source capability messages)

Please refer to 
https://lore.kernel.org/all/20241024022233.3276995-1-amitsd@google.com/ as
122968f8dda8 is causing USB Type-C PD compliance failures.

>
> Additionally, I added SOFT_RESET (instead of Hard_Reset) as Fallback solution
> if we still not have gotten any capabilities. Hard_Reset is now only done,
> if PD_CTRL_GET_SOURCE_CAP and SOFT_RESET fail to get capabilities.
> ---
>   drivers/usb/typec/tcpm/tcpm.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index fc619478200f..c7a59c9f78ee 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -5038,14 +5038,8 @@ static void run_state_machine(struct tcpm_port *port)
>   		 * were already in a stable contract before this boot.
>   		 * Do this only once.
>   		 */
> -		if (port->vbus_never_low) {
> -			port->vbus_never_low = false;
> -			tcpm_set_state(port, SNK_SOFT_RESET,
> +		tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
>   				       PD_T_SINK_WAIT_CAP);
> -		} else {
> -			tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
> -				       PD_T_SINK_WAIT_CAP);
> -		}

Instead of deleting code, please restrict this behavior to non self 
powered battery case as this most likely break compliance and

may break actual use-cases for other users as a result. If you want you 
can move stuff around after

https://lore.kernel.org/all/20241024022233.3276995-1-amitsd@google.com/

gets accepted in the following way:

```

if (!port->self_powered) {

         tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT, 
PD_T_SINK_WAIT_CAP);

         break;

}


if (port->vbus_never_low) {

         tcpm_set_state(port, SNK_SOFT_RESET, PD_T_SINK_WAIT_CAP);

} else {

         tcpm_set_state(port, hard_reset_state(..), PD_T_SINK_WAIT_CAP);

}

```

This way you don't have to execute the SNK_SOFT_RESET flow for non self 
powered use-case.

Thanks,

Amit

>   		break;
>   	case SNK_WAIT_CAPABILITIES_TIMEOUT:
>   		/*
> @@ -5064,7 +5058,7 @@ static void run_state_machine(struct tcpm_port *port)
>   		 * according to the specification.
>   		 */
>   		if (tcpm_pd_send_control(port, PD_CTRL_GET_SOURCE_CAP, TCPC_TX_SOP))
> -			tcpm_set_state_cond(port, hard_reset_state(port), 0);
> +			tcpm_set_state_cond(port, SNK_SOFT_RESET, 0);
>   		else
>   			tcpm_set_state(port, hard_reset_state(port), PD_T_SINK_WAIT_CAP);
>   		break;

^ permalink raw reply	[flat|nested] 5+ messages in thread

* AW: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
  2024-10-24  3:13 ` Amit Sunil Dhamne
@ 2024-10-24  8:54   ` Yanik Fuchs
  2024-10-24  9:20     ` Yanik Fuchs
  0 siblings, 1 reply; 5+ messages in thread
From: Yanik Fuchs @ 2024-10-24  8:54 UTC (permalink / raw)
  To: Amit Sunil Dhamne, heikki.krogerus, gregkh, rdbabiera, linux,
	kyletso, badhri, xu.yang_2, sebastian.reichel
  Cc: linux-usb, linux-kernel

Hi Amit

If the setting "bus-powered" is not set, there is only one way, that Vbus does not go down in init (where we force a Hard_Reset, which sets CC to Open if not bus-powerded). The case where Vbus never goes down and setting Bus-powered is not set is only with a legacy cable/adabter. In that case, we do not have PD compatibility anyway, and have to end up in SNK_READY eventually.
So we do the SOFT_RESET if Vbus was never down out of historical reasons and is not needed anymore since tcpm is sending Get_Capabilities (the only reason we send SOFT_RESET if Vbus was never down, is to get capabilities of Source, and that we still do, if Get_Capabilities does not work).

As you correctly have pointed out, I have to fix a issue I spotted yesterday as well, that with this Patch, after two hard_resets and then after 3 SoftReset we seem not to end up in SNK_READY, but in SNK_UNNCONECTED -> I have to find a way to check if we try hard reset in SOFT_RESET state after leaving the WAIT_SINK_CAPABILITIES_TIMOUT and in that case send it to SNK_CONNECTED (same as if we would be in state WAIT_SINK_CAPABILITIES or WAIT_SINK_CAPABILITIES_TIMOUT).
Maybe we have to count softresets after requesing capabilities and reset it as soon as we are in SNK_READY

BTW:
I have a patch which I use in the 5.15 version of the driver, where I can get ptn5110 to init correctly without forcing the Hardreset and the manual CC_Change trigger (I changed default state of sink to SNK_DEBOUNCED (where Vbus is checked and if not true sends us to SNK_UNCONNECTED) and added a delay of one second to be shure that IRQ is running when state machine starts)

Freundliche Grüsse
Best regards


Freundliche Grüsse
Best regards


Freundliche Grüsse
Best regards





________________________________________
Von: Amit Sunil Dhamne <amitsd@google.com>
Gesendet: Donnerstag, 24. Oktober 2024 05:13
An: Yanik Fuchs <Yanik.fuchs@mbv.ch>; heikki.krogerus@linux.intel.com <heikki.krogerus@linux.intel.com>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>; rdbabiera@google.com <rdbabiera@google.com>; linux@roeck-us.net <linux@roeck-us.net>; kyletso@google.com <kyletso@google.com>; badhri@google.com <badhri@google.com>; xu.yang_2@nxp.com <xu.yang_2@nxp.com>; sebastian.reichel@collabora.com <sebastian.reichel@collabora.com>
Cc: linux-usb@vger.kernel.org <linux-usb@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Betreff: Re: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low

ACHTUNG: Dieses E-Mail stammt von einem externen Absender. Prüfe Anhänge oder Links sehr genau!


Hi Yanik,

On 10/22/24 10:28 AM, Yanik Fuchs wrote:
> Good Evening
>
> Here is a Patch to resolve an issue with TCPM if Vbus was never low.
> Please consider that this is one of my first kernel pull requests, I may have missunderstood the process.
>
> Freundliche Grüsse
> Best regards
>
>
> Yanik Fuchs
>
> ---
>
>  From 604b97b6394b5643394bc63d4ac691c098c99c40 Mon Sep 17 00:00:00 2001
> From: yfu <yanikfuchs@me.com>
> Date: Tue, 22 Oct 2024 18:23:18 +0200
> Subject: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
>
> Before this patch, tcpm went into SOFT_RESET state, if Vbus was never low
> resulting in Hard_Reset, if power supply does not support USB_PD Soft_Reset.
>
> In order to prevent this, I remove the Vbus check completely, so that
> we go as well into the SNK_WAIT_CAPABILITIES_TIMEOUT state. There, we send
> PD_CTRL_GET_SOURCE_CAP which many power supply do support.
> (122968f8dda8 usb: typec: tcpm: avoid resets for missing source capability messages)

Please refer to
https://lore.kernel.org/all/20241024022233.3276995-1-amitsd@google.com/ as
122968f8dda8 is causing USB Type-C PD compliance failures.

>
> Additionally, I added SOFT_RESET (instead of Hard_Reset) as Fallback solution
> if we still not have gotten any capabilities. Hard_Reset is now only done,
> if PD_CTRL_GET_SOURCE_CAP and SOFT_RESET fail to get capabilities.
> ---
>   drivers/usb/typec/tcpm/tcpm.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index fc619478200f..c7a59c9f78ee 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -5038,14 +5038,8 @@ static void run_state_machine(struct tcpm_port *port)
>                * were already in a stable contract before this boot.
>                * Do this only once.
>                */
> -             if (port->vbus_never_low) {
> -                     port->vbus_never_low = false;
> -                     tcpm_set_state(port, SNK_SOFT_RESET,
> +             tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
>                                      PD_T_SINK_WAIT_CAP);
> -             } else {
> -                     tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
> -                                    PD_T_SINK_WAIT_CAP);
> -             }

Instead of deleting code, please restrict this behavior to non self
powered battery case as this most likely break compliance and

may break actual use-cases for other users as a result. If you want you
can move stuff around after

https://lore.kernel.org/all/20241024022233.3276995-1-amitsd@google.com/

gets accepted in the following way:

```

if (!port->self_powered) {

         tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
PD_T_SINK_WAIT_CAP);

         break;

}


if (port->vbus_never_low) {

         tcpm_set_state(port, SNK_SOFT_RESET, PD_T_SINK_WAIT_CAP);

} else {

         tcpm_set_state(port, hard_reset_state(..), PD_T_SINK_WAIT_CAP);

}

```

This way you don't have to execute the SNK_SOFT_RESET flow for non self
powered use-case.

Thanks,

Amit

>               break;
>       case SNK_WAIT_CAPABILITIES_TIMEOUT:
>               /*
> @@ -5064,7 +5058,7 @@ static void run_state_machine(struct tcpm_port *port)
>                * according to the specification.
>                */
>               if (tcpm_pd_send_control(port, PD_CTRL_GET_SOURCE_CAP, TCPC_TX_SOP))
> -                     tcpm_set_state_cond(port, hard_reset_state(port), 0);
> +                     tcpm_set_state_cond(port, SNK_SOFT_RESET, 0);
>               else
>                       tcpm_set_state(port, hard_reset_state(port), PD_T_SINK_WAIT_CAP);
>               break;

^ permalink raw reply	[flat|nested] 5+ messages in thread

* AW: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
  2024-10-24  8:54   ` AW: " Yanik Fuchs
@ 2024-10-24  9:20     ` Yanik Fuchs
  0 siblings, 0 replies; 5+ messages in thread
From: Yanik Fuchs @ 2024-10-24  9:20 UTC (permalink / raw)
  To: Amit Sunil Dhamne, heikki.krogerus, gregkh, rdbabiera, linux,
	kyletso, badhri, xu.yang_2, sebastian.reichel
  Cc: linux-usb, linux-kernel

Hey all

Does anybody see a problem, if we in the function hard_reset_state() not check for the state it was send, but check if Vbus is present. If Vbus, we go to SNK_READY and if no Vbus we go to SNK_UNCONNECTED?

Freundliche Grüsse
Best regards





________________________________________
Von: Yanik Fuchs <Yanik.fuchs@mbv.ch>
Gesendet: Donnerstag, 24. Oktober 2024 10:54
An: Amit Sunil Dhamne <amitsd@google.com>; heikki.krogerus@linux.intel.com <heikki.krogerus@linux.intel.com>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>; rdbabiera@google.com <rdbabiera@google.com>; linux@roeck-us.net <linux@roeck-us.net>; kyletso@google.com <kyletso@google.com>; badhri@google.com <badhri@google.com>; xu.yang_2@nxp.com <xu.yang_2@nxp.com>; sebastian.reichel@collabora.com <sebastian.reichel@collabora.com>
Cc: linux-usb@vger.kernel.org <linux-usb@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Betreff: AW: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low

Hi Amit

If the setting "bus-powered" is not set, there is only one way, that Vbus does not go down in init (where we force a Hard_Reset, which sets CC to Open if not bus-powerded). The case where Vbus never goes down and setting Bus-powered is not set is only with a legacy cable/adabter. In that case, we do not have PD compatibility anyway, and have to end up in SNK_READY eventually.
So we do the SOFT_RESET if Vbus was never down out of historical reasons and is not needed anymore since tcpm is sending Get_Capabilities (the only reason we send SOFT_RESET if Vbus was never down, is to get capabilities of Source, and that we still do, if Get_Capabilities does not work).

As you correctly have pointed out, I have to fix a issue I spotted yesterday as well, that with this Patch, after two hard_resets and then after 3 SoftReset we seem not to end up in SNK_READY, but in SNK_UNNCONECTED -> I have to find a way to check if we try hard reset in SOFT_RESET state after leaving the WAIT_SINK_CAPABILITIES_TIMOUT and in that case send it to SNK_CONNECTED (same as if we would be in state WAIT_SINK_CAPABILITIES or WAIT_SINK_CAPABILITIES_TIMOUT).
Maybe we have to count softresets after requesing capabilities and reset it as soon as we are in SNK_READY

BTW:
I have a patch which I use in the 5.15 version of the driver, where I can get ptn5110 to init correctly without forcing the Hardreset and the manual CC_Change trigger (I changed default state of sink to SNK_DEBOUNCED (where Vbus is checked and if not true sends us to SNK_UNCONNECTED) and added a delay of one second to be shure that IRQ is running when state machine starts)

Freundliche Grüsse
Best regards


Freundliche Grüsse
Best regards


Freundliche Grüsse
Best regards





________________________________________
Von: Amit Sunil Dhamne <amitsd@google.com>
Gesendet: Donnerstag, 24. Oktober 2024 05:13
An: Yanik Fuchs <Yanik.fuchs@mbv.ch>; heikki.krogerus@linux.intel.com <heikki.krogerus@linux.intel.com>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>; rdbabiera@google.com <rdbabiera@google.com>; linux@roeck-us.net <linux@roeck-us.net>; kyletso@google.com <kyletso@google.com>; badhri@google.com <badhri@google.com>; xu.yang_2@nxp.com <xu.yang_2@nxp.com>; sebastian.reichel@collabora.com <sebastian.reichel@collabora.com>
Cc: linux-usb@vger.kernel.org <linux-usb@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Betreff: Re: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low

ACHTUNG: Dieses E-Mail stammt von einem externen Absender. Prüfe Anhänge oder Links sehr genau!


Hi Yanik,

On 10/22/24 10:28 AM, Yanik Fuchs wrote:
> Good Evening
>
> Here is a Patch to resolve an issue with TCPM if Vbus was never low.
> Please consider that this is one of my first kernel pull requests, I may have missunderstood the process.
>
> Freundliche Grüsse
> Best regards
>
>
> Yanik Fuchs
>
> ---
>
>  From 604b97b6394b5643394bc63d4ac691c098c99c40 Mon Sep 17 00:00:00 2001
> From: yfu <yanikfuchs@me.com>
> Date: Tue, 22 Oct 2024 18:23:18 +0200
> Subject: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
>
> Before this patch, tcpm went into SOFT_RESET state, if Vbus was never low
> resulting in Hard_Reset, if power supply does not support USB_PD Soft_Reset.
>
> In order to prevent this, I remove the Vbus check completely, so that
> we go as well into the SNK_WAIT_CAPABILITIES_TIMEOUT state. There, we send
> PD_CTRL_GET_SOURCE_CAP which many power supply do support.
> (122968f8dda8 usb: typec: tcpm: avoid resets for missing source capability messages)

Please refer to
https://lore.kernel.org/all/20241024022233.3276995-1-amitsd@google.com/ as
122968f8dda8 is causing USB Type-C PD compliance failures.

>
> Additionally, I added SOFT_RESET (instead of Hard_Reset) as Fallback solution
> if we still not have gotten any capabilities. Hard_Reset is now only done,
> if PD_CTRL_GET_SOURCE_CAP and SOFT_RESET fail to get capabilities.
> ---
>   drivers/usb/typec/tcpm/tcpm.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index fc619478200f..c7a59c9f78ee 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -5038,14 +5038,8 @@ static void run_state_machine(struct tcpm_port *port)
>                * were already in a stable contract before this boot.
>                * Do this only once.
>                */
> -             if (port->vbus_never_low) {
> -                     port->vbus_never_low = false;
> -                     tcpm_set_state(port, SNK_SOFT_RESET,
> +             tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
>                                      PD_T_SINK_WAIT_CAP);
> -             } else {
> -                     tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
> -                                    PD_T_SINK_WAIT_CAP);
> -             }

Instead of deleting code, please restrict this behavior to non self
powered battery case as this most likely break compliance and

may break actual use-cases for other users as a result. If you want you
can move stuff around after

https://lore.kernel.org/all/20241024022233.3276995-1-amitsd@google.com/

gets accepted in the following way:

```

if (!port->self_powered) {

         tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
PD_T_SINK_WAIT_CAP);

         break;

}


if (port->vbus_never_low) {

         tcpm_set_state(port, SNK_SOFT_RESET, PD_T_SINK_WAIT_CAP);

} else {

         tcpm_set_state(port, hard_reset_state(..), PD_T_SINK_WAIT_CAP);

}

```

This way you don't have to execute the SNK_SOFT_RESET flow for non self
powered use-case.

Thanks,

Amit

>               break;
>       case SNK_WAIT_CAPABILITIES_TIMEOUT:
>               /*
> @@ -5064,7 +5058,7 @@ static void run_state_machine(struct tcpm_port *port)
>                * according to the specification.
>                */
>               if (tcpm_pd_send_control(port, PD_CTRL_GET_SOURCE_CAP, TCPC_TX_SOP))
> -                     tcpm_set_state_cond(port, hard_reset_state(port), 0);
> +                     tcpm_set_state_cond(port, SNK_SOFT_RESET, 0);
>               else
>                       tcpm_set_state(port, hard_reset_state(port), PD_T_SINK_WAIT_CAP);
>               break;

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-10-24  9:20 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-22 17:28 [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low Yanik Fuchs
2024-10-23  9:05 ` Heikki Krogerus
2024-10-24  3:13 ` Amit Sunil Dhamne
2024-10-24  8:54   ` AW: " Yanik Fuchs
2024-10-24  9:20     ` Yanik Fuchs

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®