Re: [PATCH] [v9] platform/chrome: cros_ec_lpc: Move host command to prepare/complete

[Tzung-Bi Shih]

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-TyY49zPoKuDDbD61dDBSTQ/edit?disco=AAAAws1enlw&usp_dm=false

I have no permission to access the doc.  Please put the context in the commit
message.  It's usually helpful if you could put the corresponding EC FW
changes.

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

Is it a new feature?  How could the *error* recovery do?

> > It seems prepare() is a more general callback.  It could be followed by
> suspend(), freeze(), or poweroff()[1].  Do we expect the change?  For example,
> 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 = suspend_fn, \
> .resume_early = resume_fn, \
> .freeze_late = suspend_fn, \
> .thaw_early = resume_fn, \
> .poweroff_late = suspend_fn, \  <<---- here
> .restore_early = 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.

I see.  There is still slightly change in disabling/enabling IRQ[2].  But I
think it would be fine as long as they are symmetric.

[2]: https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/platform/chrome/cros_ec.c#L351

> > What about other interfaces (i2c, spi, uart)?  Do they also need to change
> 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 FW
features regarding to EC_CMD_HOST_SLEEP_EVENT, you should consider the
existing systems too.

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

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 series
but I didn't follow the thread.

> On Tue, May 16, 2023 at 8:35 PM Tzung-Bi Shih <tzungbi@xxxxxxxxxx> 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 the
> > > 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 system
[...]

Please don't top-posting next time.