diff mbox series

[1/1] usb: pd: fix the SenderResponseTimer conform to specification

Message ID 20250105125251.5190-1-joswang1221@gmail.com (mailing list archive)
State New
Headers show
Series [1/1] usb: pd: fix the SenderResponseTimer conform to specification | expand

Commit Message

Jos Wang Jan. 5, 2025, 12:52 p.m. UTC 
From: Jos Wang <joswang@lenovo.com>

According to the USB PD3 CTS specification
(https://usb.org/document-library/
usb-power-delivery-compliance-test-specification-0/
USB_PD3_CTS_Q4_2024_OR.zip), the requirements for
tSenderResponse are different in PD2 and PD3 modes, see
Table 19 Timing Table & Calculations. For PD2 mode, the
tSenderResponse min 24ms and max 30ms; for PD3 mode, the
tSenderResponse min 27ms and max 33ms.

For the "TEST.PD.PROT.SRC.2 Get_Source_Cap No Request" test
item, after receiving the Source_Capabilities Message sent by
the UUT, the tester deliberately does not send a Request Message
in order to force the SenderResponse timer on the Source UUT to
timeout. The Tester checks that a Hard Reset is detected between
tSenderResponse min and max，the delay is between the last bit of
the GoodCRC Message EOP has been sent and the first bit of Hard
Reset SOP has been received. The current code does not distinguish
between PD2 and PD3 modes, and tSenderResponse defaults to 60ms.
This will cause this test item and the following tests to fail:
TEST.PD.PROT.SRC3.2 SenderResponseTimer Timeout
TEST.PD.PROT.SNK.6 SenderResponseTimer Timeout

Set the SenderResponseTimer timeout to 27ms to meet the PD2
and PD3 mode requirements.

Fixes: f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")
Cc: stable@vger.kernel.org
Signed-off-by: Jos Wang <joswang@lenovo.com>
---
 include/linux/usb/pd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Greg KH Jan. 5, 2025, 12:59 p.m. UTC | #1 
On Sun, Jan 05, 2025 at 08:52:51PM +0800, joswang wrote:
> From: Jos Wang <joswang@lenovo.com>
> 
> According to the USB PD3 CTS specification
> (https://usb.org/document-library/
> usb-power-delivery-compliance-test-specification-0/
> USB_PD3_CTS_Q4_2024_OR.zip), the requirements for

Please put urls on one line so that they can be linked to correctly.

> tSenderResponse are different in PD2 and PD3 modes, see
> Table 19 Timing Table & Calculations. For PD2 mode, the
> tSenderResponse min 24ms and max 30ms; for PD3 mode, the
> tSenderResponse min 27ms and max 33ms.
> 
> For the "TEST.PD.PROT.SRC.2 Get_Source_Cap No Request" test
> item, after receiving the Source_Capabilities Message sent by
> the UUT, the tester deliberately does not send a Request Message
> in order to force the SenderResponse timer on the Source UUT to
> timeout. The Tester checks that a Hard Reset is detected between
> tSenderResponse min and max，the delay is between the last bit of
> the GoodCRC Message EOP has been sent and the first bit of Hard
> Reset SOP has been received. The current code does not distinguish
> between PD2 and PD3 modes, and tSenderResponse defaults to 60ms.
> This will cause this test item and the following tests to fail:
> TEST.PD.PROT.SRC3.2 SenderResponseTimer Timeout
> TEST.PD.PROT.SNK.6 SenderResponseTimer Timeout
> 
> Set the SenderResponseTimer timeout to 27ms to meet the PD2
> and PD3 mode requirements.
> 
> Fixes: f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jos Wang <joswang@lenovo.com>
> ---
>  include/linux/usb/pd.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index 3068c3084eb6..99ca49bbf376 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -475,7 +475,7 @@ static inline unsigned int rdo_max_power(u32 rdo)
>  #define PD_T_NO_RESPONSE	5000	/* 4.5 - 5.5 seconds */
>  #define PD_T_DB_DETECT		10000	/* 10 - 15 seconds */
>  #define PD_T_SEND_SOURCE_CAP	150	/* 100 - 200 ms */
> -#define PD_T_SENDER_RESPONSE	60	/* 24 - 30 ms, relaxed */
> +#define PD_T_SENDER_RESPONSE	27	/* 24 - 30 ms */

Why 27 and not 30?  The comment seems odd here, right?

thanks,

greg k-h
Jos Wang Jan. 6, 2025, 1:25 p.m. UTC | #2 
On Sun, Jan 5, 2025 at 9:00 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sun, Jan 05, 2025 at 08:52:51PM +0800, joswang wrote:
> > From: Jos Wang <joswang@lenovo.com>
> >
> > According to the USB PD3 CTS specification
> > (https://usb.org/document-library/
> > usb-power-delivery-compliance-test-specification-0/
> > USB_PD3_CTS_Q4_2024_OR.zip), the requirements for
>
> Please put urls on one line so that they can be linked to correctly.
>

OK，Thanks

> > tSenderResponse are different in PD2 and PD3 modes, see
> > Table 19 Timing Table & Calculations. For PD2 mode, the
> > tSenderResponse min 24ms and max 30ms; for PD3 mode, the
> > tSenderResponse min 27ms and max 33ms.
> >
> > For the "TEST.PD.PROT.SRC.2 Get_Source_Cap No Request" test
> > item, after receiving the Source_Capabilities Message sent by
> > the UUT, the tester deliberately does not send a Request Message
> > in order to force the SenderResponse timer on the Source UUT to
> > timeout. The Tester checks that a Hard Reset is detected between
> > tSenderResponse min and max，the delay is between the last bit of
> > the GoodCRC Message EOP has been sent and the first bit of Hard
> > Reset SOP has been received. The current code does not distinguish
> > between PD2 and PD3 modes, and tSenderResponse defaults to 60ms.
> > This will cause this test item and the following tests to fail:
> > TEST.PD.PROT.SRC3.2 SenderResponseTimer Timeout
> > TEST.PD.PROT.SNK.6 SenderResponseTimer Timeout
> >
> > Set the SenderResponseTimer timeout to 27ms to meet the PD2
> > and PD3 mode requirements.
> >
> > Fixes: f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jos Wang <joswang@lenovo.com>
> > ---
> >  include/linux/usb/pd.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> > index 3068c3084eb6..99ca49bbf376 100644
> > --- a/include/linux/usb/pd.h
> > +++ b/include/linux/usb/pd.h
> > @@ -475,7 +475,7 @@ static inline unsigned int rdo_max_power(u32 rdo)
> >  #define PD_T_NO_RESPONSE     5000    /* 4.5 - 5.5 seconds */
> >  #define PD_T_DB_DETECT               10000   /* 10 - 15 seconds */
> >  #define PD_T_SEND_SOURCE_CAP 150     /* 100 - 200 ms */
> > -#define PD_T_SENDER_RESPONSE 60      /* 24 - 30 ms, relaxed */
> > +#define PD_T_SENDER_RESPONSE 27      /* 24 - 30 ms */
>
> Why 27 and not 30?  The comment seems odd here, right?
>

1、As mentioned in the commit message, "TEST.PD.PROT.SRC.2
Get_Source_Cap No Request" test item, after receiving the
Source_Capabilities Message sent by the UUT, the tester deliberately
does not send a Request Message in order to force the SenderResponse
timer on the Source UUT to timeout. The Tester checks that a Hard
Reset is detected between tSenderResponse min and max. Since it takes
time for the tcpm framework layer to initiate a Hard Reset (writing
the PD PHY register through I2C operation), setting tSenderResponse to
30ms (PD2.0 spec max) will cause this test item to fail in PD2.0 mode.

2、The comments here are indeed unreasonable, how about modifying it like this?
+#define PD_T_SENDER_RESPONSE 27 /* PD2.0 spec 24ms -30ms, PD3.1 spec
27ms - 33ms, setting 27ms meets the requirements of PD2.0 and PD3.1.
*/

3、This patch was originally discussed here
(https://patchwork.kernel.org/project/linux-usb/patch/20241222105239.2618-2-joswang1221@gmail.com/)
Dmitry Baryshkov suggested splitting the "[v2,2/2] usb: typec: tcpm:
fix the sender response time issue" patch and submitting it
separately.
(1) Set SenderResponse timer timeout to 27ms
(2) Make timeout depend on the PD version
(3) Make timeouts configurable via DT
Actually, I would prefer to merge (1) and (2) and submit them. Do you
have any better suggestions?

> thanks,
>
> greg k-h
Greg KH Jan. 7, 2025, 10:35 a.m. UTC | #3 
On Mon, Jan 06, 2025 at 09:25:17PM +0800, Jos Wang wrote:
> On Sun, Jan 5, 2025 at 9:00 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Jan 05, 2025 at 08:52:51PM +0800, joswang wrote:
> > > From: Jos Wang <joswang@lenovo.com>
> > >
> > > According to the USB PD3 CTS specification
> > > (https://usb.org/document-library/
> > > usb-power-delivery-compliance-test-specification-0/
> > > USB_PD3_CTS_Q4_2024_OR.zip), the requirements for
> >
> > Please put urls on one line so that they can be linked to correctly.
> >
> 
> OK，Thanks
> 
> > > tSenderResponse are different in PD2 and PD3 modes, see
> > > Table 19 Timing Table & Calculations. For PD2 mode, the
> > > tSenderResponse min 24ms and max 30ms; for PD3 mode, the
> > > tSenderResponse min 27ms and max 33ms.
> > >
> > > For the "TEST.PD.PROT.SRC.2 Get_Source_Cap No Request" test
> > > item, after receiving the Source_Capabilities Message sent by
> > > the UUT, the tester deliberately does not send a Request Message
> > > in order to force the SenderResponse timer on the Source UUT to
> > > timeout. The Tester checks that a Hard Reset is detected between
> > > tSenderResponse min and max，the delay is between the last bit of
> > > the GoodCRC Message EOP has been sent and the first bit of Hard
> > > Reset SOP has been received. The current code does not distinguish
> > > between PD2 and PD3 modes, and tSenderResponse defaults to 60ms.
> > > This will cause this test item and the following tests to fail:
> > > TEST.PD.PROT.SRC3.2 SenderResponseTimer Timeout
> > > TEST.PD.PROT.SNK.6 SenderResponseTimer Timeout
> > >
> > > Set the SenderResponseTimer timeout to 27ms to meet the PD2
> > > and PD3 mode requirements.
> > >
> > > Fixes: f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Jos Wang <joswang@lenovo.com>
> > > ---
> > >  include/linux/usb/pd.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> > > index 3068c3084eb6..99ca49bbf376 100644
> > > --- a/include/linux/usb/pd.h
> > > +++ b/include/linux/usb/pd.h
> > > @@ -475,7 +475,7 @@ static inline unsigned int rdo_max_power(u32 rdo)
> > >  #define PD_T_NO_RESPONSE     5000    /* 4.5 - 5.5 seconds */
> > >  #define PD_T_DB_DETECT               10000   /* 10 - 15 seconds */
> > >  #define PD_T_SEND_SOURCE_CAP 150     /* 100 - 200 ms */
> > > -#define PD_T_SENDER_RESPONSE 60      /* 24 - 30 ms, relaxed */
> > > +#define PD_T_SENDER_RESPONSE 27      /* 24 - 30 ms */
> >
> > Why 27 and not 30?  The comment seems odd here, right?
> >
> 
> 1、As mentioned in the commit message, "TEST.PD.PROT.SRC.2
> Get_Source_Cap No Request" test item, after receiving the
> Source_Capabilities Message sent by the UUT, the tester deliberately
> does not send a Request Message in order to force the SenderResponse
> timer on the Source UUT to timeout. The Tester checks that a Hard
> Reset is detected between tSenderResponse min and max. Since it takes
> time for the tcpm framework layer to initiate a Hard Reset (writing
> the PD PHY register through I2C operation), setting tSenderResponse to
> 30ms (PD2.0 spec max) will cause this test item to fail in PD2.0 mode.
> 
> 2、The comments here are indeed unreasonable, how about modifying it like this?
> +#define PD_T_SENDER_RESPONSE 27 /* PD2.0 spec 24ms -30ms, PD3.1 spec
> 27ms - 33ms, setting 27ms meets the requirements of PD2.0 and PD3.1.
> */

As it is, it needs to be changed to something else, so please pick
something that you would want to see if you were reading this code.

thanks,

greg k-h
Jos Wang Jan. 8, 2025, 2:49 p.m. UTC | #4 
On Sun, Jan 5, 2025 at 8:53 PM joswang <joswang1221@gmail.com> wrote:
>
> From: Jos Wang <joswang@lenovo.com>
>
> According to the USB PD3 CTS specification
> (https://usb.org/document-library/
> usb-power-delivery-compliance-test-specification-0/
> USB_PD3_CTS_Q4_2024_OR.zip), the requirements for
> tSenderResponse are different in PD2 and PD3 modes, see
> Table 19 Timing Table & Calculations. For PD2 mode, the
> tSenderResponse min 24ms and max 30ms; for PD3 mode, the
> tSenderResponse min 27ms and max 33ms.
>
> For the "TEST.PD.PROT.SRC.2 Get_Source_Cap No Request" test
> item, after receiving the Source_Capabilities Message sent by
> the UUT, the tester deliberately does not send a Request Message
> in order to force the SenderResponse timer on the Source UUT to
> timeout. The Tester checks that a Hard Reset is detected between
> tSenderResponse min and max，the delay is between the last bit of
> the GoodCRC Message EOP has been sent and the first bit of Hard
> Reset SOP has been received. The current code does not distinguish
> between PD2 and PD3 modes, and tSenderResponse defaults to 60ms.
> This will cause this test item and the following tests to fail:
> TEST.PD.PROT.SRC3.2 SenderResponseTimer Timeout
> TEST.PD.PROT.SNK.6 SenderResponseTimer Timeout
>
> Set the SenderResponseTimer timeout to 27ms to meet the PD2
> and PD3 mode requirements.
>
> Fixes: f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jos Wang <joswang@lenovo.com>
> ---
>  include/linux/usb/pd.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index 3068c3084eb6..99ca49bbf376 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -475,7 +475,7 @@ static inline unsigned int rdo_max_power(u32 rdo)
>  #define PD_T_NO_RESPONSE       5000    /* 4.5 - 5.5 seconds */
>  #define PD_T_DB_DETECT         10000   /* 10 - 15 seconds */
>  #define PD_T_SEND_SOURCE_CAP   150     /* 100 - 200 ms */
> -#define PD_T_SENDER_RESPONSE   60      /* 24 - 30 ms, relaxed */
> +#define PD_T_SENDER_RESPONSE   27      /* 24 - 30 ms */
>  #define PD_T_RECEIVER_RESPONSE 15      /* 15ms max */
>  #define PD_T_SOURCE_ACTIVITY   45
>  #define PD_T_SINK_ACTIVITY     135
> --
> 2.17.1
>

Hi Dmitry

Discuss here "https://patchwork.kernel.org/project/linux-usb/patch/20241222105239.2618-2-joswang1221@gmail.com/",
you suggest setting PD_T_SENDER_RESPONSE to 27ms.
1、In actual testing, setting the timeout to 27ms can solve the CTS
problem in PD2 mode. For PD3 mode, since 27ms is at the boundary, it
cannot solve the CTS problem well.
For example, in the test item “TEST.PD.PROT.SRC.3 SenderResponseTimer
Deadline (As a Sink, the Tester checks that the UUT accepts a Request
Message sent at the deadline limit of tSenderResponse min in reply to
a Source_Capabilities Message.)”, after the Tester (sink) receives the
Source_cap message, it will send a Request message 25.26 ms later. The
tcpm framework layer on the source side should take more than 2
seconds to receive and process the Request message，which means that a
Hard Reset may be sent before the Request message is fully processed.
2、The current modification is as follows:
#define PD_T_SENDER_RESPONSE 27 /* I am confused by the comments here,
such as 24ms - 30ms or 24ms - 33 ms or others */
There is no suitable comment here. In the next patch "Make timeout
depend on the PD version", this line of code may need to be modified
again.
In summary, I might consider including the following changes in one commit:
(1) include/linux/usb/pd.h
+#define PD_T_PD2_SENDER_RESPONSE 27 /* PD20 spec 24 - 30 ms */
+#define PD_T_PD3_SENDER_RESPONSE 30 /* PD30 spec 27 - 33 ms */
(2) drivers/usb/typec/tcpm/tcpm.c
Make timeout depend on the PD version
In fact, it is to patch "[v2,2/2] usb: typec: tcpm: fix the sender
response time issue", remove "Make timeouts configurable via DT"

Do you have a better idea?

Thanks

Jos Wang
diff mbox series

Patch

diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
index 3068c3084eb6..99ca49bbf376 100644
--- a/include/linux/usb/pd.h
+++ b/include/linux/usb/pd.h
@@ -475,7 +475,7 @@  static inline unsigned int rdo_max_power(u32 rdo)
 #define PD_T_NO_RESPONSE	5000	/* 4.5 - 5.5 seconds */
 #define PD_T_DB_DETECT		10000	/* 10 - 15 seconds */
 #define PD_T_SEND_SOURCE_CAP	150	/* 100 - 200 ms */
-#define PD_T_SENDER_RESPONSE	60	/* 24 - 30 ms, relaxed */
+#define PD_T_SENDER_RESPONSE	27	/* 24 - 30 ms */
 #define PD_T_RECEIVER_RESPONSE	15	/* 15ms max */
 #define PD_T_SOURCE_ACTIVITY	45
 #define PD_T_SINK_ACTIVITY	135