Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230515142552.1.I17cae37888be3a8683911991602f18e482e7a621@changeid>
 <ZGQ9Y+vqWhQLHAQh@google.com> <CAMaBtwHxaevxLY7zWNDU8zbyWx=puLkeeRAjFtovvrA5pjtJ4w@mail.gmail.com>
 <ZGWBhEMmo2lStTg9@google.com>
In-Reply-To: <ZGWBhEMmo2lStTg9@google.com>
From:   Tim Van Patten <timvp@chromium.org>
Date:   Thu, 18 May 2023 10:47:23 -0600
Message-ID: <CAMaBtwFtE=vjuhVy7rw9zCe9WV0dRyeBWj88JH2j3bkbh2BkXA@mail.gmail.com>
Subject: Re: [PATCH] [v9] platform/chrome: cros_ec_lpc: Move host command to prepare/complete
To:     Tzung-Bi Shih <tzungbi@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, robbarnes@google.com,
        lalithkraj@google.com, rrangel@chromium.org,
        Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        chrome-platform@lists.linux.dev, Garrick Evans <garrick@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 17, 2023 at 7:38=E2=80=AFPM Tzung-Bi Shih <tzungbi@kernel.org> =
wrote:
>
> On Wed, May 17, 2023 at 09:56:59AM -0600, Tim Van Patten wrote:
> > > I can understand the patch wants to notify EC earlier/later when the =
system
> > suspend/resume.  But what is the issue addressed?  What happens if the
> > measurement of suspend/resume duration is not that accurate?
> >
> > Please see the following:
> > - b/206860487
> > - https://docs.google.com/document/d/1AgaZmG70bAKhZb-ZMbZT-TyY49zPoKuDD=
bD61dDBSTQ/edit?disco=3DAAAAws1enlw&usp_dm=3Dfalse
>
> I have no permission to access the doc.

Since you work at Google, please request permission to access it, so
the owners can grant it.

> Please put the context in the commit
> message.

Done in the next patchset.

> It's usually helpful if you could put the corresponding EC FW
> changes.

Why are you assuming there is EC FW with this change? Any and all of
the EC changes related to this have (obviously) landed long ago.

> > The issue is that we need the EC aware of the AP being in the process
> > of suspend/resume from start to finish, so we can accurately
> > determine:
> > - How long the process took to better gauge we're meeting ChromeOS requ=
irements.
> > - When the AP failed to complete the process, so we can collect data
> > and perform error recovery.
>
> Is it a new feature?

No, it's not.

> How could the *error* recovery do?

I don't understand what this is asking.

> > > What about other interfaces (i2c, spi, uart)?  Do they also need to c=
hange
> > the callbacks?
> >
> > We aren't concerned about those devices, because they aren't being
> > used on the devices we're seeing issues with. If devices using those
> > ECs want this change, they can pick it up as well, but we don't have
> > any way to test changes on those devices (whatever they may be).
>
> This doesn't sound good.  As I would suppose you are adding some new EC F=
W
> features regarding to EC_CMD_HOST_SLEEP_EVENT, you should consider the
> existing systems too.

Again, why are you assuming there is new EC FW for this? This is only
changing when an already-existing host command is being sent. Nothing
is being added or removed.

> What happens if a system uses older kernel (without this patch) to
> communicate with new EC FW via LPC?

The message is being sent regardless of whether this patch lands or
not. This patch just changes when they are sent, so there is no risk
from that perspective.

> How about adding a new EC host command for your purpose so that it won't
> affect the existing systems?  I knew this was discussed in some older ser=
ies
> but I didn't follow the thread.
>

No. The necessary host command already exists and is being sent. There
is no additional command being sent with this change. It is only
changing when the command is being sent.