Re: [PATCH] usb: typec: ucsi: Wait 20ms before retrying reset

[Pavan Holla]

On Wed, Mar 27, 2024 at 4:01 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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