Date: Tue, 2 Jan 2024 16:46:15 -0800
From: Stephen Boyd <swboyd@...omium.org>
To: LKML <linux-kernel@...r.kernel.org>, Mark Hasemeyer <markhas@...omium.org>
Cc: Sudeep Holla <sudeep.holla@....com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Rob Herring <robh@...nel.org>, 
	Andy Shevchenko <andriy.shevchenko@...el.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, 
	Raul Rangel <rrangel@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>, 
	Benson Leung <bleung@...omium.org>, Bhanu Prakash Maiya <bhanumaiya@...omium.org>, 
	Chen-Yu Tsai <wenst@...omium.org>, Guenter Roeck <groeck@...omium.org>, 
	Prashant Malani <pmalani@...omium.org>, Rob Barnes <robbarnes@...gle.com>, 
	chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v4 24/24] platform/chrome: cros_ec: Use PM subsystem to
 manage wakeirq

Quoting Mark Hasemeyer (2024-01-02 13:07:48)
> The cros ec driver is manually managing the wake IRQ by calling
> enable_irq_wake()/disable_irq_wake() during suspend/resume.
>
> Modify the driver to use the power management subsystem to manage the
> wakeirq.
>
> Rather than assuming that the IRQ is wake capable, use the underlying
> firmware/device tree to determine whether or not to enable it as a wake
> source. Some Chromebooks rely solely on the ec_sync pin to wake the AP
> but do not specify the interrupt as wake capable in the ACPI _CRS. For
> LPC/ACPI based systems a DMI quirk is introduced listing boards whose
> firmware should not be trusted to provide correct wake capable values.

The DMI quirk looks to be fixing something. Most likely that should be
backported to stable kernels as well? Please split the DMI match table
part out of this so that it isn't mixed together with migrating the
driver to use the pm wakeirq logic.

> For device tree base systems, it is not an issue as the relevant device
> tree entries have been updated and DTS is built from source for each
> ChromeOS update.

It is still sorta an issue. It means that we can't backport this patch
without also backporting the DTS patch to the kernel. Furthermore, DTS
changes go through different maintainer trees, so having this patch in
the kernel without the DTS patch means the bisection hole is possibly
quite large.

Does using the pm wakeirq logic require the use of 'wakeup-source'
property in DT? A quick glance makes me believe it isn't needed, so
please split that part out of this patch as well. We should see a patch
for the DMI quirk, a patch to use wakeup-source (doubtful that we need
it at all though), and a patch to use the pm wakeirq logic instead of
hand rolling it again.

>
> Acked-by: Tzung-Bi Shih <tzungbi@...nel.org>
> Signed-off-by: Mark Hasemeyer <markhas@...omium.org>
> ---
[...]
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index badc68bbae8cc..080b479f39a94 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -15,6 +15,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/cros_ec_commands.h>
>  #include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/pm_wakeirq.h>
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>
> @@ -168,6 +169,35 @@ static int cros_ec_ready_event(struct notifier_block *nb,
>         return NOTIFY_DONE;
>  }
>
> +static int enable_irq_for_wake(struct cros_ec_device *ec_dev)
> +{
> +       struct device *dev = ec_dev->dev;
> +       int ret = device_init_wakeup(dev, true);
> +
> +       if (ret) {
> +               dev_err(dev, "Failed to enable device for wakeup");

Missing newline on printk message.

> +               return ret;
> +       }
> +       ret = dev_pm_set_wake_irq(dev, ec_dev->irq);
> +       if (ret)
> +               device_init_wakeup(dev, false);
> +
> +       return ret;

I'd rather see the code formatted like this:

	int ret;

	ret = device_init_wakeup(dev, true);
	if (ret) {
		dev_err(...);
		return ret;
	}

	ret = dev_pm_set_wake_irq(...);
	if (ret)
		device_init_wakeup(dev, false);
	
	return ret;

Mostly because the first 'if (ret)' requires me to hunt in the variable
declaration part of the function to figure out what it is set to instead
of looking at the line directly above.

> +}
> +
> +static int disable_irq_for_wake(struct cros_ec_device *ec_dev)
> +{
> +       int ret;
> +       struct device *dev = ec_dev->dev;
> +
> +       dev_pm_clear_wake_irq(dev);
> +       ret = device_init_wakeup(dev, false);
> +       if (ret)
> +               dev_err(dev, "Failed to disable device for wakeup");
> +
> +       return ret;
> +}
> +
>  /**
>   * cros_ec_register() - Register a new ChromeOS EC, using the provided info.
>   * @ec_dev: Device to register.
> @@ -221,6 +251,13 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>                                 ec_dev->irq, err);
>                         goto exit;
>                 }
> +               dev_dbg(dev, "IRQ: %i, wake_capable: %s\n", ec_dev->irq,

This one has a newline :)

> +                       str_yes_no(ec_dev->irq_wake));
> +               if (ec_dev->irq_wake) {
> +                       err = enable_irq_for_wake(ec_dev);
> +                       if (err)
> +                               goto exit;
> +               }
>         }
[...]
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> index 3e88cc92e8192..102cdc3d1956d 100644
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -7,6 +7,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/platform_data/cros_ec_commands.h>
>  #include <linux/platform_data/cros_ec_proto.h>
>  #include <linux/platform_device.h>
> @@ -70,6 +71,7 @@
>   * @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
>   *      is sent when we want to turn off CS at the end of a transaction.
>   * @high_pri_worker: Used to schedule high priority work.
> + * @irq_wake: Whether or not irq assertion should wake the system.
>   */
>  struct cros_ec_spi {
>         struct spi_device *spi;
> @@ -77,6 +79,7 @@ struct cros_ec_spi {
>         unsigned int start_of_msg_delay;
>         unsigned int end_of_msg_delay;
>         struct kthread_worker *high_pri_worker;
> +       bool irq_wake;

This is only used in probe. Make it a local variable instead of another
struct member to save memory.

>  };
>
>  typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev,
> @@ -689,9 +692,10 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
>         return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi);
>  }
>
> -static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> +static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi)
>  {
> -       struct device_node *np = dev->of_node;
> +       struct spi_device *spi = ec_spi->spi;
> +       struct device_node *np = spi->dev.of_node;
>         u32 val;
>         int ret;
>
> @@ -702,6 +706,8 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
>         ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
>         if (!ret)
>                 ec_spi->end_of_msg_delay = val;
> +
> +       ec_spi->irq_wake = spi->irq > 0 && of_property_present(np, "wakeup-source");

Is there any EC SPI device that doesn't want to support wakeup? I'd
prefer we introduce a new property or compatible string to indicate that
wakeup isn't supported and otherwise always set irq_wake to true. That
way DT can change in parallel with the driver instead of in lockstep.

Powered by blists - more mailing lists