Re: [PATCH v2] platform/chrome: cros_ec: Send host event for prepare/complete

[Tim Van Patten]

[Resending in plain text mode]

Hi,

> Please be consistent.  Either way:
> - .prepare and .complete.
> - .prepare() and .complete().

I'll address this in the next version.

> The patch doesn't allow EC to log anything.  It makes AP emit more logs.

This patch changes when the EC outputs the host command that indicates
the AP is starting suspend and finishing resume, due to the change (in
this patch) when the AP sends that host command.   This makes the EC's
logs more accurate when correlating them with the AP's logs in regards
to when suspend is started and resume is completed.   Previously,
those events were sent when suspend/resume were already in progress.

We'd also like to keep the new logs emitted by the AP to make it
clearer when the AP is starting suspend and completing resume, so we
can correlate it with the EC logs more easily.   This should aid
debugging and timing analysis.   Since it only occurs during
suspend/resume, it shouldn't flood the logs and follows the logging of
other driver PM functions.

> I didn't see concerns in [1] have been addressed.

I replied to the first email stating why we want to keep the log
message (and reiterated it above).   What's the correct process to
indicate we don't want to make the change requested in [1]?

On Wed, Jul 13, 2022 at 12:05 PM Tim Van Patten <timvp@xxxxxxxxxx> wrote:
>
> Hi,
>
>> Please be consistent.  Either way:
>> - .prepare and .complete.
>> - .prepare() and .complete().
>
>
> I'll address this in the next version.
>
>> The patch doesn't allow EC to log anything.  It makes AP emit more logs.
>
>
> This patch changes when the EC outputs the host command that indicates the AP is starting suspend and finishing resume, due to the change (in this patch) when the AP sends that host command.   This makes the EC's logs more accurate when correlating them with the AP's logs in regards to when suspend is started and resume is completed.   Previously, those events were sent when suspend/resume were already in progress.
>
> We'd also like to keep the new logs emitted by the AP to make it clearer when the AP is starting suspend and completing resume, so we can correlate it with the EC logs more easily.   This should aid debugging and timing analysis.   Since it only occurs during suspend/resume, it shouldn't flood the logs and follows the logging of other driver PM functions.
>
>> I didn't see concerns in [1] have been addressed.
>
>
> I replied to the first email stating why we want to keep the log message (and reiterated it above).   What's the correct process to indicate we don't want to make the change requested in [1]?
>
>
> On Tue, Jul 12, 2022 at 8:58 PM Tzung-Bi Shih <tzungbi@xxxxxxxxxx> wrote:
>>
>> On Wed, Jul 06, 2022 at 08:51:39PM -0600, Tim Van Patten wrote:
>> > Update cros_ec_lpc_pm_ops to call cros_ec_lpc_suspend() during PM
>> > .prepare() and cros_ec_lpc_resume() during .complete. This allows the
>> > EC to log entry/exit of AP's suspend/resume more accurately.
>>
>> Please be consistent.  Either way:
>> - .prepare and .complete.
>> - .prepare() and .complete().
>>
>> The patch doesn't allow EC to log anything.  It makes AP emit more logs.
>>
>> On the related note, the commit subject is confusing.  The patch doesn't
>> send "host event".  "host event" is a terminology when EC wants to notify
>> AP something.  Also, s/cros_ec/cros_ec_lpcs/.
>>
>> > Changes in v2:
>> > - Include cros_ec_resume() return value in dev_info() output.
>> > - Guard setting .prepare/.complete with #ifdef CONFIG_PM_SLEEP.
>>
>> I didn't see concerns in [1] have been addressed.
>>
>> [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220701095421.1.I78ded92e416b55de31975686d34b2058d4761c07@changeid/#24920824
>
>
>
> --
>
> Tim Van Patten | ChromeOS | timvp@xxxxxxxxxx | (720) 432-0997



-- 

Tim Van Patten | ChromeOS | timvp@xxxxxxxxxx | (720) 432-0997