Received-SPF: pass (google.com: domain of linux-kernel+bounces-120708-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 27 Mar 2024 13:01:26 +0200
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
To: Pavan Holla <pholla@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Subject: Re: [PATCH] usb: typec: ucsi: Wait 20ms before retrying reset
Message-ID: <ZgP8hqNXuMdkp7A5@kuha.fi.intel.com>
References: <20240325-ucsi-reset-delay-v1-1-d9e183e0f1e6@chromium.org>
 <2024032624-subtitle-crisped-f4f1@gregkh>
 <CAB2FV=4Z1W1HSba50KaB3rR4=Ussb5RWPwUArr0_=3pFwxpAhA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAB2FV=4Z1W1HSba50KaB3rR4=Ussb5RWPwUArr0_=3pFwxpAhA@mail.gmail.com>

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,

-- 
heikki