| Message ID | GVAP278MB0662F98EAAFAD95645E7010A974C2@GVAP278MB0662.CHEP278.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low | expand |
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,
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;
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
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
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;
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(-)