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>
In-Reply-To: <ZGQ9Y+vqWhQLHAQh@google.com>
From:   Tim Van Patten <timvp@chromium.org>
Date:   Wed, 17 May 2023 09:56:59 -0600
Message-ID: <CAMaBtwHxaevxLY7zWNDU8zbyWx=puLkeeRAjFtovvrA5pjtJ4w@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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

> I can understand the patch wants to notify EC earlier/later when the syst=
em
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-TyY49zPoKuDDbD61=
dDBSTQ/edit?disco=3DAAAAws1enlw&usp_dm=3Dfalse

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 requirem=
ents.
- When the AP failed to complete the process, so we can collect data
and perform error recovery.

If the EC isn't monitoring the entire process, then:
- We get an inaccurate representation of when the suspend/resume
started/finished.
- The AP can fail during the gaps in monitoring and the EC will be
unable to detect and recover from it.

> It seems prepare() is a more general callback.  It could be followed by
suspend(), freeze(), or poweroff()[1].  Do we expect the change?  For examp=
le,
the system is going to power off but EC gets notification about the system
should be going to suspend.  Same as complete().

Please reference the implementation of SET_LATE_SYSTEM_SLEEP_PM_OPS
and see that we were already calling it in the poweroff path:

#define SET_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
.suspend_late =3D suspend_fn, \
.resume_early =3D resume_fn, \
.freeze_late =3D suspend_fn, \
.thaw_early =3D resume_fn, \
.poweroff_late =3D suspend_fn, \  <<---- here
.restore_early =3D resume_fn,

* @poweroff_late: Continue operations started by @poweroff(). Analogous to
* @suspend_late(), but it need not save the device's settings in memory.

So, there is unlikely to be any functional difference with this change
in terms of poweroff.

> What about other interfaces (i2c, spi, uart)?  Do they also need to chang=
e
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).

On Tue, May 16, 2023 at 8:35=E2=80=AFPM Tzung-Bi Shih <tzungbi@kernel.org> =
wrote:
>
> On Mon, May 15, 2023 at 02:25:52PM -0600, Tim Van Patten wrote:
> > Update cros_ec_lpc_pm_ops to call cros_ec_lpc_prepare() during PM
> > .prepare() and cros_ec_lpc_complete() during .complete(). This moves th=
e
> > host command that the AP sends and allows the EC to log entry/exit of
> > AP's suspend/resume more accurately.
>
> I can understand the patch wants to notify EC earlier/later when the syst=
em
> suspend/resume.  But what is the issue addressed?  What happens if the
> measurement of suspend/resume duration is not that accurate?
>
> Copied from my previous mail:
> * Should it move the callbacks?
>   * Is it appropriate to call cros_ec_suspend() when PM is still in prepa=
re
>     phase and call cros_ec_resume() when PM is already in complete phase?
>
> It seems prepare() is a more general callback.  It could be followed by
> suspend(), freeze(), or poweroff()[1].  Do we expect the change?  For exa=
mple,
> the system is going to power off but EC gets notification about the syste=
m
> should be going to suspend.  Same as complete().
>
> Moreover, cros_ec_suspend() and cros_ec_resume() do more than just notify=
 EC.
> E.g. [2].
>
> What about other interfaces (i2c, spi, uart)?  Do they also need to chang=
e
> the callbacks?
>
> [1]: https://elixir.bootlin.com/linux/v6.4-rc1/source/include/linux/pm.h#=
L74
> [2]: https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/platform/ch=
rome/cros_ec.c#L351
>
> > Changes in v9:
> > - Remove log statements.
> > - Ignore return value from cros_ec_resume().
>
> The change logs are not part of commit message.  They should put after "-=
--".