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
