| Message ID | 20240325-ucsi-reset-delay-v1-1-d9e183e0f1e6@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: ucsi: Wait 20ms before retrying reset | expand |
On Mon, Mar 25, 2024 at 09:19:43PM +0000, Pavan Holla wrote: > The PPM might take time to process reset. Allow 20ms for the reset to > complete before issuing another reset. > > Signed-off-by: Pavan Holla <pholla@chromium.org> What commit id does this fix? Does it need to go to older kernels? > --- > There is a 20ms delay for a reset retry to complete. However, the first > reset attempt is expected to complete immediately after an async write > of the reset command. This patch adds 20ms between the async write and > the CCI read that expects the reset to be complete. The additional delay > also allows the PPM to settle after the first reset, which seems to be > the intention behind the original 20ms delay ( kernel v4.14 has a comment > regarding the same ) Why was the comment removed in newer kernels? Where does the magic 20ms number come from? What about systems that do not need that time delay, did things just slow down for them? thanks, greg k-h
On Tue, Mar 26, 2024 at 1:29 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Mar 25, 2024 at 09:19:43PM +0000, Pavan Holla wrote: > > The PPM might take time to process reset. Allow 20ms for the reset to > > complete before issuing another reset. > What commit id does this fix? Does it need to go to older kernels? This does not fix any commit. However, the time taken by a CCI read is insufficient for a ChromeOS EC and PDC to perform a reset. > > There is a 20ms delay for a reset retry to complete. However, the first > > reset attempt is expected to complete immediately after an async write > > of the reset command. This patch adds 20ms between the async write and > > the CCI read that expects the reset to be complete. The additional delay > > also allows the PPM to settle after the first reset, which seems to be > > the intention behind the original 20ms delay ( kernel v4.14 has a comment > > regarding the same ) > > Why was the comment removed in newer kernels? The comment was removed when the old UCSI API was removed in 2ede55468ca8cc236da66579359c2c406d4c1cba > Where does the magic 20ms number come from? What about systems that do > not need that time delay, did things just slow down for them? I am not sure how 20ms was decided upon. However, UCSI v1.2 has MIN_TIME_TO_RESPOND_WITH_BUSY=10ms. So, we need to provide at least 10ms for the PPM to respond with CCI busy. Indeed, this patch slows down other implementations by 20ms. UCSIv3 also defines a 200ms timeout for PPM_RESET. Hi Heikki, Do you think the right thing to do here is: 1) poll CCI ( poll duration TBD) for 20ms until busy is set or reset is complete. 2) poll CCI ( poll duration TBD) for 180ms until reset is complete if busy was set. 3) If either 1) or 2) timeout, retry the reset. If you agree with the approach, please advise a poll duration as well ( 20ms? ) Thanks, Pavan
Hi, Normally the driver does not retry the reset, so maybe you should just say "wait 20ms before reading the CCI after reset", or something like that. The idea here is to give the PPM time to actually update that field before reading it, right? On Tue, Mar 26, 2024 at 04:34:44PM -0700, Pavan Holla wrote: > On Tue, Mar 26, 2024 at 1:29 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Mar 25, 2024 at 09:19:43PM +0000, Pavan Holla wrote: > > > The PPM might take time to process reset. Allow 20ms for the reset to > > > complete before issuing another reset. > > What commit id does this fix? Does it need to go to older kernels? > > This does not fix any commit. However, the time taken by a CCI read is > insufficient for a ChromeOS EC and PDC to perform a reset. Perhaps you could put that to the commit message. > > > There is a 20ms delay for a reset retry to complete. However, the first > > > reset attempt is expected to complete immediately after an async write > > > of the reset command. This patch adds 20ms between the async write and > > > the CCI read that expects the reset to be complete. The additional delay > > > also allows the PPM to settle after the first reset, which seems to be > > > the intention behind the original 20ms delay ( kernel v4.14 has a comment > > > regarding the same ) > > > > Why was the comment removed in newer kernels? > > The comment was removed when the old UCSI API was removed in > 2ede55468ca8cc236da66579359c2c406d4c1cba > > > Where does the magic 20ms number come from? What about systems that do > > not need that time delay, did things just slow down for them? > > I am not sure how 20ms was decided upon. However, UCSI v1.2 has > MIN_TIME_TO_RESPOND_WITH_BUSY=10ms. So, we need to provide at least > 10ms for the PPM to respond with CCI busy. Indeed, this patch slows down other > implementations by 20ms. UCSIv3 also defines a 200ms timeout for PPM_RESET. It does not slow down other implementations. The delay has always been there before the RESET_COMPLETE bit is actually checked. The change here makes sense to me. Just rewrite the commit message. thanks,
On Wed, Mar 27, 2024 at 4:01 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Hi, > > Normally the driver does not retry the reset, so maybe you should just > say "wait 20ms before reading the CCI after reset", or something like > that. > > The idea here is to give the PPM time to actually update that field > before reading it, right? Yes, that's the idea. I will change the commit message in v2. > On Tue, Mar 26, 2024 at 04:34:44PM -0700, Pavan Holla wrote: > > On Tue, Mar 26, 2024 at 1:29 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, Mar 25, 2024 at 09:19:43PM +0000, Pavan Holla wrote: > > > > The PPM might take time to process reset. Allow 20ms for the reset to > > > > complete before issuing another reset. > > > What commit id does this fix? Does it need to go to older kernels? > > > > This does not fix any commit. However, the time taken by a CCI read is > > insufficient for a ChromeOS EC and PDC to perform a reset. > > Perhaps you could put that to the commit message. Will do. > > > > There is a 20ms delay for a reset retry to complete. However, the first > > > > reset attempt is expected to complete immediately after an async write > > > > of the reset command. This patch adds 20ms between the async write and > > > > the CCI read that expects the reset to be complete. The additional delay > > > > also allows the PPM to settle after the first reset, which seems to be > > > > the intention behind the original 20ms delay ( kernel v4.14 has a comment > > > > regarding the same ) > > > > > > Why was the comment removed in newer kernels? > > > > The comment was removed when the old UCSI API was removed in > > 2ede55468ca8cc236da66579359c2c406d4c1cba > > > > > Where does the magic 20ms number come from? What about systems that do > > > not need that time delay, did things just slow down for them? > > > > I am not sure how 20ms was decided upon. However, UCSI v1.2 has > > MIN_TIME_TO_RESPOND_WITH_BUSY=10ms. So, we need to provide at least > > 10ms for the PPM to respond with CCI busy. Indeed, this patch slows down other > > implementations by 20ms. UCSIv3 also defines a 200ms timeout for PPM_RESET. > > It does not slow down other implementations. The delay has always been > there before the RESET_COMPLETE bit is actually checked. Ah, maybe other PPM's don't set CCI.busy, and that is probably why a reset isn't retried for them. The UCSIv1 spec itself might have a typo in Table 4-2 whereCCI.busy is only allowed to be 0 for a PPM_RESET. However, figure 4-1 shows that a transition to busy is allowed from PPM Idle (Notification disabled). Moving the msleep(20) before the CCI read is probably a good idea anyway since it gives enough time for CCI.busy to be set. Should we also skip the retry if busy is set? > The change here makes sense to me. Just rewrite the commit message. Will do in v2 if I don't receive further feedback. Thanks, Pavan
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index cf52cb34d285..6b642c4c58b7 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -1280,6 +1280,9 @@ static int ucsi_reset_ppm(struct ucsi *ucsi) goto out; } + /* Give the PPM time to reset and stabilize */ + msleep(20); + ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci)); if (ret) goto out; @@ -1293,7 +1296,6 @@ static int ucsi_reset_ppm(struct ucsi *ucsi) goto out; } - msleep(20); } while (!(cci & UCSI_CCI_RESET_COMPLETE)); out:
The PPM might take time to process reset. Allow 20ms for the reset to complete before issuing another reset. Signed-off-by: Pavan Holla <pholla@chromium.org> --- There is a 20ms delay for a reset retry to complete. However, the first reset attempt is expected to complete immediately after an async write of the reset command. This patch adds 20ms between the async write and the CCI read that expects the reset to be complete. The additional delay also allows the PPM to settle after the first reset, which seems to be the intention behind the original 20ms delay ( kernel v4.14 has a comment regarding the same ) --- drivers/usb/typec/ucsi/ucsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) --- base-commit: 4cece764965020c22cff7665b18a012006359095 change-id: 20240325-ucsi-reset-delay-bdf6712455fd Best regards,