Received-SPF: pass (google.com: domain of linux-kernel+bounces-122142-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240325-ucsi-reset-delay-v1-1-d9e183e0f1e6@chromium.org>
 <2024032624-subtitle-crisped-f4f1@gregkh> <CAB2FV=4Z1W1HSba50KaB3rR4=Ussb5RWPwUArr0_=3pFwxpAhA@mail.gmail.com>
 <ZgP8hqNXuMdkp7A5@kuha.fi.intel.com>
In-Reply-To: <ZgP8hqNXuMdkp7A5@kuha.fi.intel.com>
From: Pavan Holla <pholla@chromium.org>
Date: Wed, 27 Mar 2024 16:12:51 -0700
Message-ID: <CAB2FV=5pDxSiAGtYDUEbRLb-OtGP8d6duAP6+yD6eFdcRsC1Fg@mail.gmail.com>
Subject: Re: [PATCH] usb: typec: ucsi: Wait 20ms before retrying reset
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org, 
	linux-kernel@vger.kernel.org, 
	Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 27, 2024 at 4:01=E2=80=AFAM 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=E2=80=AFAM 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 f=
irst
> > > > reset attempt is expected to complete immediately after an async wr=
ite
> > > > 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 c=
omment
> > > > 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=3D10ms. So, we need to provide at least
> > 10ms for the PPM to respond with CCI busy. Indeed, this patch slows dow=
n other
> > implementations by 20ms. UCSIv3 also defines a 200ms timeout for PPM_RE=
SET.
>
> 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