Re: [PATCH] platform/chrome: cros_ec_lpc: Separate host command and irq disable

[Lalithkumar Rajendran]

Adding chrome-platform to CC.


On Tue, Oct 10, 2023 at 12:03 PM Lalith Rajendran
<lalithkraj@xxxxxxxxxxxx> wrote:
>
> Both cros host command and irq disable were moved to suspend
> prepare stage from late suspend recently. This is causing EC
> to report MKBP event timeouts during suspend stress testing.
> When the MKBP event timeouts happen during suspend, subsequent
> wakeup of AP by EC using MKBP doesn't happen properly. Although
> there are other issues to debug here, this change move the irq
> disabling part back to late suspend stage which is a general
> suggestion from the suspend kernel documentaiton to do irq
> disable as late as possible.
>
> Signed-off-by: Lalith Rajendran <lalithkraj@xxxxxxxxxxxx>
> ---
>
>  drivers/platform/chrome/cros_ec.c     | 90 ++++++++++++++++++++++++---
>  drivers/platform/chrome/cros_ec.h     |  4 ++
>  drivers/platform/chrome/cros_ec_lpc.c | 21 +++++--
>  3 files changed, 101 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index 2b49155a9b35..6f520c13c0f3 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -317,16 +317,17 @@ EXPORT_SYMBOL(cros_ec_unregister);
>
>  #ifdef CONFIG_PM_SLEEP
>  /**
> - * cros_ec_suspend() - Handle a suspend operation for the ChromeOS EC device.
> + * cros_ec_suspend_prepare() - Handle a suspend prepare operation for the ChromeOS EC device.
>   * @ec_dev: Device to suspend.
>   *
> - * This can be called by drivers to handle a suspend event.
> + * This can be called by drivers to handle a suspend prepare stage of suspend.
> + * Drivers should either call cros_ec_supsend or call
> + * cros_ec_suspend_prepare and cros_ec_suspend_late.
>   *
>   * Return: 0 on success or negative error code.
>   */
> -int cros_ec_suspend(struct cros_ec_device *ec_dev)
> +int cros_ec_suspend_prepare(struct cros_ec_device *ec_dev)
>  {
> -       struct device *dev = ec_dev->dev;
>         int ret;
>         u8 sleep_event;
>
> @@ -338,7 +339,23 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
>         if (ret < 0)
>                 dev_dbg(ec_dev->dev, "Error %d sending suspend event to ec",
>                         ret);
> +       return 0;
> +}
> +EXPORT_SYMBOL(cros_ec_suspend_prepare);
>
> +/**
> + * cros_ec_suspend_late() - Handle a suspend late operation for the ChromeOS EC device.
> + * @ec_dev: Device to suspend.
> + *
> + * This can be called by drivers to handle a suspend late stage of suspend.
> + * Drivers should either call cros_ec_supsend or call
> + * cros_ec_suspend_prepare and cros_ec_suspend_late.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_suspend_late(struct cros_ec_device *ec_dev)
> +{
> +       struct device *dev = ec_dev->dev;
>         if (device_may_wakeup(dev))
>                 ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
>
> @@ -348,6 +365,24 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
>
>         return 0;
>  }
> +EXPORT_SYMBOL(cros_ec_suspend_late);
> +
> +/**
> + * cros_ec_suspend() - Handle a suspend operation for the ChromeOS EC device.
> + * @ec_dev: Device to suspend.
> + *
> + * This can be called by drivers to handle a suspend event.
> + * Drivers should either call cros_ec_supsend or call
> + * cros_ec_suspend_prepare and cros_ec_suspend_late.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_suspend(struct cros_ec_device *ec_dev)
> +{
> +       cros_ec_suspend_prepare(ec_dev);
> +       cros_ec_suspend_late(ec_dev);
> +       return 0;
> +}
>  EXPORT_SYMBOL(cros_ec_suspend);
>
>  static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
> @@ -365,21 +400,20 @@ static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
>  }
>
>  /**
> - * cros_ec_resume() - Handle a resume operation for the ChromeOS EC device.
> + * cros_ec_resume() - Handle a resume complete operation for the ChromeOS EC device.
>   * @ec_dev: Device to resume.
>   *
> - * This can be called by drivers to handle a resume event.
> + * This can be called by drivers to handle a resume complete stage of resume.
> + * Drivers should either call cros_ec_resume or call
> + * cros_ec_resume_early and cros_ec_resume_complete.
>   *
>   * Return: 0 on success or negative error code.
>   */
> -int cros_ec_resume(struct cros_ec_device *ec_dev)
> +int cros_ec_resume_complete(struct cros_ec_device *ec_dev)
>  {
>         int ret;
>         u8 sleep_event;
>
> -       ec_dev->suspended = false;
> -       enable_irq(ec_dev->irq);
> -
>         sleep_event = (!IS_ENABLED(CONFIG_ACPI) || pm_suspend_via_firmware()) ?
>                       HOST_SLEEP_EVENT_S3_RESUME :
>                       HOST_SLEEP_EVENT_S0IX_RESUME;
> @@ -388,6 +422,24 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
>         if (ret < 0)
>                 dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
>                         ret);
> +       return 0;
> +}
> +EXPORT_SYMBOL(cros_ec_resume_complete);
> +
> +/**
> + * cros_ec_resume_early() - Handle a resume early operation for the ChromeOS EC device.
> + * @ec_dev: Device to resume.
> + *
> + * This can be called by drivers to handle a resume early stage of resume.
> + * Drivers should either call cros_ec_resume or call
> + * cros_ec_resume_early and cros_ec_resume_complete.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_resume_early(struct cros_ec_device *ec_dev)
> +{
> +       ec_dev->suspended = false;
> +       enable_irq(ec_dev->irq);
>
>         if (ec_dev->wake_enabled) {
>                 disable_irq_wake(ec_dev->irq);
> @@ -402,6 +454,24 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
>
>         return 0;
>  }
> +EXPORT_SYMBOL(cros_ec_resume_early);
> +
> +/**
> + * cros_ec_resume() - Handle a resume operation for the ChromeOS EC device.
> + * @ec_dev: Device to resume.
> + *
> + * This can be called by drivers to handle a resume event.
> + * Drivers should either call cros_ec_resume or call
> + * cros_ec_resume_early and cros_ec_resume_complete.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_resume(struct cros_ec_device *ec_dev)
> +{
> +       cros_ec_resume_early(ec_dev);
> +       cros_ec_resume_complete(ec_dev);
> +       return 0;
> +}
>  EXPORT_SYMBOL(cros_ec_resume);
>
>  #endif
> diff --git a/drivers/platform/chrome/cros_ec.h b/drivers/platform/chrome/cros_ec.h
> index bbca0096868a..41defaa5e766 100644
> --- a/drivers/platform/chrome/cros_ec.h
> +++ b/drivers/platform/chrome/cros_ec.h
> @@ -14,7 +14,11 @@ int cros_ec_register(struct cros_ec_device *ec_dev);
>  void cros_ec_unregister(struct cros_ec_device *ec_dev);
>
>  int cros_ec_suspend(struct cros_ec_device *ec_dev);
> +int cros_ec_suspend_late(struct cros_ec_device *ec_dev);
> +int cros_ec_suspend_prepare(struct cros_ec_device *ec_dev);
>  int cros_ec_resume(struct cros_ec_device *ec_dev);
> +int cros_ec_resume_early(struct cros_ec_device *ec_dev);
> +int cros_ec_resume_complete(struct cros_ec_device *ec_dev);
>
>  irqreturn_t cros_ec_irq_thread(int irq, void *data);
>
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index 8982cf23e514..afb9f7dbb2ba 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -537,22 +537,35 @@ MODULE_DEVICE_TABLE(dmi, cros_ec_lpc_dmi_table);
>  static int cros_ec_lpc_prepare(struct device *dev)
>  {
>         struct cros_ec_device *ec_dev = dev_get_drvdata(dev);
> -
> -       return cros_ec_suspend(ec_dev);
> +       return cros_ec_suspend_prepare(ec_dev);
>  }
>
>  static void cros_ec_lpc_complete(struct device *dev)
>  {
>         struct cros_ec_device *ec_dev = dev_get_drvdata(dev);
> -       cros_ec_resume(ec_dev);
> +       cros_ec_resume_complete(ec_dev);
> +}
> +static int cros_ec_lpc_suspend_late(struct device *dev)
> +{
> +       struct cros_ec_device *ec_dev = dev_get_drvdata(dev);
> +
> +       return cros_ec_suspend_late(ec_dev);
> +}
> +
> +static int cros_ec_lpc_resume_early(struct device *dev)
> +{
> +       struct cros_ec_device *ec_dev = dev_get_drvdata(dev);
> +
> +       return cros_ec_resume_early(ec_dev);
>  }
>  #endif
>
>  static const struct dev_pm_ops cros_ec_lpc_pm_ops = {
>  #ifdef CONFIG_PM_SLEEP
>         .prepare = cros_ec_lpc_prepare,
> -       .complete = cros_ec_lpc_complete
> +       .complete = cros_ec_lpc_complete,
>  #endif
> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(cros_ec_lpc_suspend_late, cros_ec_lpc_resume_early)
>  };
>
>  static struct platform_driver cros_ec_lpc_driver = {
> --
> 2.42.0.609.gbb76f46606-goog
>