Messages in this thread |  | | Date | Thu, 18 May 2023 09:38:12 +0800 | | From | Tzung-Bi Shih <> | | Subject | Re: [PATCH] [v9] platform/chrome: cros_ec_lpc: Move host command to prepare/complete |
| |
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@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 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.
|  |