[PATCH v3 00/24] Improve IRQ wake capability reporting and update the cros_ec driver to use it

[PATCH v3 00/24] Improve IRQ wake capability reporting and update the cros_ec driver to use it

[Mark Hasemeyer]

Currently the cros_ec driver assumes that its associated interrupt is
wake capable. This is an incorrect assumption as some Chromebooks use a
separate wake pin, while others overload the interrupt for wake and IO.
This patch train updates the driver to query the underlying ACPI/DT data
to determine whether or not the IRQ should be enabled for wake.

Both the device tree and ACPI systems have methods for reporting IRQ
wake capability. In device tree based systems, a node can advertise
itself as a 'wakeup-source'. In ACPI based systems, GpioInt and
Interrupt resource descriptors can use the 'SharedAndWake' or
'ExclusiveAndWake' share types.

Some logic is added to the platform, ACPI, and DT subsystems to more
easily pipe wakeirq information up to the driver.

Changes in v3:
-Rebase on linux-next
-See each patch for patch specific changes

Changes in v2:
-Rebase on linux-next
-Add cover letter
-See each patch for patch specific changes

Mark Hasemeyer (24):
  resource: Add DEFINE_RES_*_NAMED_FLAGS macro
  gpiolib: acpi: Modify acpi_dev_irq_wake_get_by() to use resource
  i2c: acpi: Modify i2c_acpi_get_irq() to use resource
  dt-bindings: power: Clarify wording for wakeup-source property
  ARM: dts: tegra: Enable cros-ec-spi as wake source
  ARM: dts: rockchip: rk3288: Enable cros-ec-spi as wake source
  ARM: dts: samsung: exynos5420: Enable cros-ec-spi as wake source
  ARM: dts: samsung: exynos5800: Enable cros-ec-spi as wake source
  arm64: dts: mediatek: mt8173: Enable cros-ec-spi as wake source
  arm64: dts: mediatek: mt8183: Enable cros-ec-spi as wake source
  arm64: dts: mediatek: mt8192: Enable cros-ec-spi as wake source
  arm64: dts: mediatek: mt8195: Enable cros-ec-spi as wake source
  arm64: dts: tegra: Enable cros-ec-spi as wake source
  arm64: dts: qcom: sc7180: Enable cros-ec-spi as wake source
  arm64: dts: qcom: sc7280: Enable cros-ec-spi as wake source
  arm64: dts: qcom: sdm845: Enable cros-ec-spi as wake source
  arm64: dts: rockchip: rk3399: Enable cros-ec-spi as wake source
  of: irq: add wake capable bit to of_irq_resource()
  of: irq: Add default implementation for of_irq_to_resource()
  of: irq: Remove extern from function declarations
  device property: Modify fwnode irq_get() to use resource
  device property: Update functions to use EXPORT_SYMBOL_GPL
  platform: Modify platform_get_irq_optional() to use resource
  platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

 .../bindings/power/wakeup-source.txt          | 18 +++--
 arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi   |  1 +
 arch/arm/boot/dts/nvidia/tegra124-venice2.dts |  1 +
 .../rockchip/rk3288-veyron-chromebook.dtsi    |  1 +
 .../boot/dts/samsung/exynos5420-peach-pit.dts |  1 +
 .../boot/dts/samsung/exynos5800-peach-pi.dts  |  1 +
 arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi  |  1 +
 .../arm64/boot/dts/mediatek/mt8183-kukui.dtsi |  1 +
 .../boot/dts/mediatek/mt8192-asurada.dtsi     |  1 +
 .../boot/dts/mediatek/mt8195-cherry.dtsi      |  1 +
 .../arm64/boot/dts/nvidia/tegra132-norrin.dts |  1 +
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  |  1 +
 .../arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  1 +
 .../arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi |  1 +
 arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi    |  1 +
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  |  1 +
 drivers/acpi/property.c                       | 11 ++-
 drivers/base/platform.c                       | 70 +++++++++++++------
 drivers/base/property.c                       | 32 +++++++--
 drivers/gpio/gpiolib-acpi.c                   | 27 ++++---
 drivers/i2c/i2c-core-acpi.c                   | 37 +++++-----
 drivers/i2c/i2c-core-base.c                   |  6 +-
 drivers/i2c/i2c-core.h                        |  4 +-
 drivers/of/irq.c                              | 39 +++++++++--
 drivers/of/property.c                         |  8 +--
 drivers/platform/chrome/cros_ec.c             | 48 ++++++++++---
 drivers/platform/chrome/cros_ec_lpc.c         | 32 ++++++++-
 drivers/platform/chrome/cros_ec_spi.c         | 15 ++--
 drivers/platform/chrome/cros_ec_uart.c        | 22 ++++--
 include/linux/acpi.h                          | 23 +++---
 include/linux/fwnode.h                        |  8 ++-
 include/linux/ioport.h                        | 20 ++++--
 include/linux/of_irq.h                        | 41 ++++++-----
 include/linux/platform_data/cros_ec_proto.h   |  4 +-
 include/linux/platform_device.h               |  3 +
 include/linux/property.h                      |  2 +
 36 files changed, 336 insertions(+), 149 deletions(-)

-- 
2.43.0.472.g3155946c3a-goog

[PATCH v3 24/24] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Mark Hasemeyer]

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.
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.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v3:
-Rebase on linux-next
-See each patch for patch specific changes
-Remove MODULE_DEVICE_TABLE
-Drop "cros_ec _" prefix from should_force_irq_wake_capable()
-Drop use of dev_err_probe() to be consistent with existing conventions
in the driver
-Drop *spi argument from cros_ec_spi_dt_probe()
-Drop null device_node check from cros_ec_spi_dt_probe()
-Add trailing commas to DMI table
-Drop redundant "!= NULL" in should_force_irq_wake_capable()
-Use str_yes_no() to print irq wake capability
-Move irqwake handling from the interface specific modules to cros_ec.c

Changes in v2:
-Rebase on linux-next
-Add cover letter
-See each patch for patch specific changes
-Look for 'wakeup-source' property in cros_ec_spi.c

 drivers/platform/chrome/cros_ec.c           | 48 +++++++++++++++++----
 drivers/platform/chrome/cros_ec_lpc.c       | 32 ++++++++++++--
 drivers/platform/chrome/cros_ec_spi.c       | 15 ++++---
 drivers/platform/chrome/cros_ec_uart.c      | 22 +++++++---
 include/linux/platform_data/cros_ec_proto.h |  4 +-
 5 files changed, 97 insertions(+), 24 deletions(-)

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");
+		return ret;
+	}
+	ret = dev_pm_set_wake_irq(dev, ec_dev->irq);
+	if (ret)
+		device_init_wakeup(dev, false);
+
+	return ret;
+}
+
+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,
+			str_yes_no(ec_dev->irq_wake));
+		if (ec_dev->irq_wake) {
+			err = enable_irq_for_wake(ec_dev);
+			if (err)
+				goto exit;
+		}
 	}
 
 	/* Register a platform device for the main EC instance */
@@ -313,6 +350,8 @@ EXPORT_SYMBOL(cros_ec_register);
  */
 void cros_ec_unregister(struct cros_ec_device *ec_dev)
 {
+	if (ec_dev->irq_wake)
+		disable_irq_for_wake(ec_dev);
 	platform_device_unregister(ec_dev->pd);
 	platform_device_unregister(ec_dev->ec);
 	mutex_destroy(&ec_dev->lock);
@@ -353,12 +392,6 @@ EXPORT_SYMBOL(cros_ec_suspend_prepare);
 
 static void cros_ec_disable_irq(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);
-	else
-		ec_dev->wake_enabled = false;
-
 	disable_irq(ec_dev->irq);
 	ec_dev->suspended = true;
 }
@@ -440,9 +473,6 @@ static void cros_ec_enable_irq(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);
-
 	/*
 	 * Let the mfd devices know about events that occur during
 	 * suspend. This way the clients know what to do with them.
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index f0f3d3d561572..193a48e3f8f8e 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -48,6 +48,27 @@ struct lpc_driver_ops {
 
 static struct lpc_driver_ops cros_ec_lpc_ops = { };
 
+static const struct dmi_system_id untrusted_fw_irq_wake_capable[] = {
+	{
+		.ident = "Brya",
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brya"),
+		},
+	},
+	{
+		.ident = "Brask",
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brask"),
+		},
+	},
+	{ }
+};
+
+static bool should_force_irq_wake_capable(void)
+{
+	return dmi_first_match(untrusted_fw_irq_wake_capable);
+}
+
 /*
  * A generic instance of the read function of struct lpc_driver_ops, used for
  * the LPC EC.
@@ -353,6 +374,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 	struct acpi_device *adev;
 	acpi_status status;
 	struct cros_ec_device *ec_dev;
+	struct resource irqres;
 	u8 buf[2] = {};
 	int irq, ret;
 
@@ -428,10 +450,14 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 	 * Some boards do not have an IRQ allotted for cros_ec_lpc,
 	 * which makes ENXIO an expected (and safe) scenario.
 	 */
-	irq = platform_get_irq_optional(pdev, 0);
-	if (irq > 0)
+	irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
+	if (irq > 0) {
 		ec_dev->irq = irq;
-	else if (irq != -ENXIO) {
+		if (should_force_irq_wake_capable())
+			ec_dev->irq_wake = true;
+		else
+			ec_dev->irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
+	} else if (irq != -ENXIO) {
 		dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
 		return irq;
 	}
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;
 };
 
 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");
 }
 
 static void cros_ec_spi_high_pri_release(void *worker)
@@ -754,12 +760,13 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	/* Check for any DT properties */
-	cros_ec_spi_dt_probe(ec_spi, dev);
+	cros_ec_spi_dt_probe(ec_spi);
 
 	spi_set_drvdata(spi, ec_dev);
 	ec_dev->dev = dev;
 	ec_dev->priv = ec_spi;
 	ec_dev->irq = spi->irq;
+	ec_dev->irq_wake = ec_spi->irq_wake;
 	ec_dev->cmd_xfer = cros_ec_cmd_xfer_spi;
 	ec_dev->pkt_xfer = cros_ec_pkt_xfer_spi;
 	ec_dev->phys_name = dev_name(&ec_spi->spi->dev);
@@ -780,8 +787,6 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 		return err;
 	}
 
-	device_init_wakeup(&spi->dev, true);
-
 	return 0;
 }
 
diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
index 68d80559fddc2..5330ccdf9b35c 100644
--- a/drivers/platform/chrome/cros_ec_uart.c
+++ b/drivers/platform/chrome/cros_ec_uart.c
@@ -69,6 +69,7 @@ struct response_info {
  * @serdev:		serdev uart device we are connected to.
  * @baudrate:		UART baudrate of attached EC device.
  * @flowcontrol:	UART flowcontrol of attached device.
+ * @irq_wake:		Whether or not irq assertion should wake the system.
  * @irq:		Linux IRQ number of associated serial device.
  * @response:		Response info passing between cros_ec_uart_pkt_xfer()
  *			and cros_ec_uart_rx_bytes()
@@ -77,6 +78,7 @@ struct cros_ec_uart {
 	struct serdev_device *serdev;
 	u32 baudrate;
 	u8 flowcontrol;
+	bool irq_wake;
 	u32 irq;
 	struct response_info response;
 };
@@ -224,8 +226,10 @@ static int cros_ec_uart_resource(struct acpi_resource *ares, void *data)
 static int cros_ec_uart_acpi_probe(struct cros_ec_uart *ec_uart)
 {
 	int ret;
+	struct resource irqres;
 	LIST_HEAD(resources);
-	struct acpi_device *adev = ACPI_COMPANION(&ec_uart->serdev->dev);
+	struct device *dev = &ec_uart->serdev->dev;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
 
 	ret = acpi_dev_get_resources(adev, &resources, cros_ec_uart_resource, ec_uart);
 	if (ret < 0)
@@ -234,12 +238,12 @@ static int cros_ec_uart_acpi_probe(struct cros_ec_uart *ec_uart)
 	acpi_dev_free_resource_list(&resources);
 
 	/* Retrieve GpioInt and translate it to Linux IRQ number */
-	ret = acpi_dev_gpio_irq_get(adev, 0);
+	ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, &irqres);
 	if (ret < 0)
 		return ret;
 
-	ec_uart->irq = ret;
-	dev_dbg(&ec_uart->serdev->dev, "IRQ number %d\n", ec_uart->irq);
+	ec_uart->irq = irqres.start;
+	ec_uart->irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
 
 	return 0;
 }
@@ -293,6 +297,7 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
 	ec_dev->dev = dev;
 	ec_dev->priv = ec_uart;
 	ec_dev->irq = ec_uart->irq;
+	ec_dev->irq_wake = ec_uart->irq_wake;
 	ec_dev->cmd_xfer = NULL;
 	ec_dev->pkt_xfer = cros_ec_uart_pkt_xfer;
 	ec_dev->din_size = sizeof(struct ec_host_response) +
@@ -301,7 +306,14 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
 
 	serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);
 
-	return cros_ec_register(ec_dev);
+	/* Register a new cros_ec device */
+	ret = cros_ec_register(ec_dev);
+	if (ret) {
+		dev_err(dev, "Couldn't register ec_dev (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
 }
 
 static void cros_ec_uart_remove(struct serdev_device *serdev)
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 8865e350c12a5..0fb2781b602d6 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -100,6 +100,7 @@ struct cros_ec_command {
  * @proto_version: The protocol version used for this device.
  * @priv: Private data.
  * @irq: Interrupt to use.
+ * @irq_wake: Whether or not irq assertion should wake the system.
  * @id: Device id.
  * @din: Input buffer (for data from EC). This buffer will always be
  *       dword-aligned and include enough space for up to 7 word-alignment
@@ -115,7 +116,6 @@ struct cros_ec_command {
  *        performance advantage to using dword.
  * @din_size: Size of din buffer to allocate (zero to use static din).
  * @dout_size: Size of dout buffer to allocate (zero to use static dout).
- * @wake_enabled: True if this device can wake the system from sleep.
  * @suspended: True if this device had been suspended.
  * @cmd_xfer: Send command to EC and get response.
  *            Returns the number of bytes received if the communication
@@ -169,11 +169,11 @@ struct cros_ec_device {
 	u16 proto_version;
 	void *priv;
 	int irq;
+	bool irq_wake;
 	u8 *din;
 	u8 *dout;
 	int din_size;
 	int dout_size;
-	bool wake_enabled;
 	bool suspended;
 	int (*cmd_xfer)(struct cros_ec_device *ec,
 			struct cros_ec_command *msg);
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH v3 24/24] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Tzung-Bi Shih]

On Tue, Dec 26, 2023 at 12:21:28PM -0700, Mark Hasemeyer wrote:
> 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.
> 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.
> 
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>

The patch overall looks good to me.

With some minor comments:
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

> diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
[...]
>  static int cros_ec_uart_acpi_probe(struct cros_ec_uart *ec_uart)
>  {
[...]
>  	/* Retrieve GpioInt and translate it to Linux IRQ number */
> -	ret = acpi_dev_gpio_irq_get(adev, 0);
> +	ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, &irqres);
>  	if (ret < 0)
>  		return ret;
>  
> -	ec_uart->irq = ret;
> -	dev_dbg(&ec_uart->serdev->dev, "IRQ number %d\n", ec_uart->irq);
> +	ec_uart->irq = irqres.start;

How about keep using `ret`?

> @@ -301,7 +306,14 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
>  
>  	serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);
>  
> -	return cros_ec_register(ec_dev);
> +	/* Register a new cros_ec device */
> +	ret = cros_ec_register(ec_dev);
> +	if (ret) {
> +		dev_err(dev, "Couldn't register ec_dev (%d)\n", ret);
> +		return ret;
> +	}

It doesn't need the change after moving device_init_wakeup() and
dev_pm_set_wake_irq() into cros_ec_register().

Drop it?

Re: [PATCH v3 24/24] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Andy Shevchenko]

On Tue, Dec 26, 2023 at 12:21:28PM -0700, Mark Hasemeyer wrote:
> 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.
> 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.

...

>  	acpi_status status;
>  	struct cros_ec_device *ec_dev;
> +	struct resource irqres;

	struct resource irqres = {};

?

>  	u8 buf[2] = {};
>  	int irq, ret;

...

> -	irq = platform_get_irq_optional(pdev, 0);
> -	if (irq > 0)
> +	irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
> +	if (irq > 0) {
>  		ec_dev->irq = irq;
> -	else if (irq != -ENXIO) {
> +		if (should_force_irq_wake_capable())
> +			ec_dev->irq_wake = true;
> +		else
> +			ec_dev->irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
> +	} else if (irq != -ENXIO) {
>  		dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
>  		return irq;
>  	}

Still I do not like ambiguity behind irq > 0 vs. irqres.start.

For this, and if needed others, return plain error.
Seems I gave the tag for the previous patch, consider
that tag conditional (it seems I missed this).

...

>  	u16 proto_version;
>  	void *priv;
>  	int irq;
> +	bool irq_wake;
>  	u8 *din;
>  	u8 *dout;
>  	int din_size;
>  	int dout_size;
> -	bool wake_enabled;
>  	bool suspended;
>  	int (*cmd_xfer)(struct cros_ec_device *ec,
>  			struct cros_ec_command *msg);

Have you run pahole on this (before and after)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 24/24] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Mark Hasemeyer]

> >       /* Retrieve GpioInt and translate it to Linux IRQ number */
> > -     ret = acpi_dev_gpio_irq_get(adev, 0);
> > +     ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, &irqres);
> >       if (ret < 0)
> >               return ret;
> >
> > -     ec_uart->irq = ret;
> > -     dev_dbg(&ec_uart->serdev->dev, "IRQ number %d\n", ec_uart->irq);
> > +     ec_uart->irq = irqres.start;
>
> How about keep using `ret`?

The return value for 'acpi_dev_get_gpio_irq_resource' is different: 0
on success, negative errno on failure.

> > @@ -301,7 +306,14 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
> >
> >       serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);
> >
> > -     return cros_ec_register(ec_dev);
> > +     /* Register a new cros_ec device */
> > +     ret = cros_ec_register(ec_dev);
> > +     if (ret) {
> > +             dev_err(dev, "Couldn't register ec_dev (%d)\n", ret);
> > +             return ret;
> > +     }
>
> It doesn't need the change after moving device_init_wakeup() and
> dev_pm_set_wake_irq() into cros_ec_register().
>
> Drop it?

Will do.

Re: [PATCH v3 24/24] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Mark Hasemeyer]

> > -     irq = platform_get_irq_optional(pdev, 0);
> > -     if (irq > 0)
> > +     irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
> > +     if (irq > 0) {
> >               ec_dev->irq = irq;
> > -     else if (irq != -ENXIO) {
> > +             if (should_force_irq_wake_capable())
> > +                     ec_dev->irq_wake = true;
> > +             else
> > +                     ec_dev->irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
> > +     } else if (irq != -ENXIO) {
> >               dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
> >               return irq;
> >       }
>
> Still I do not like ambiguity behind irq > 0 vs. irqres.start.
>
> For this, and if needed others, return plain error.
> Seems I gave the tag for the previous patch, consider
> that tag conditional (it seems I missed this).

What "others" do you mean? Modify platform_get_irq_resource_optional()
to return success/err? Or do you mean to modify all irq resource based
functions? of_irq_to_resource() already existed and returns the irq
number on success. Modifying it would mean updating all references to
it, in addition to modifying the fwnode abstraction layer (and its
references). I'm open to modifying
platform_get_irq_resource_optional(), but would like to avoid blowing
up this patch train any further.

> ...
>
> >       u16 proto_version;
> >       void *priv;
> >       int irq;
> > +     bool irq_wake;
> >       u8 *din;
> >       u8 *dout;
> >       int din_size;
> >       int dout_size;
> > -     bool wake_enabled;
> >       bool suspended;
> >       int (*cmd_xfer)(struct cros_ec_device *ec,
> >                       struct cros_ec_command *msg);
>
> Have you run pahole on this (before and after)?

Yes I did. The structure is not fully optimized, but this change keps
the overall size unchanged (328 bytes).

Re: [PATCH v3 24/24] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Tzung-Bi Shih]

On Wed, Dec 27, 2023 at 07:34:58PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 26, 2023 at 12:21:28PM -0700, Mark Hasemeyer wrote:
> > -	irq = platform_get_irq_optional(pdev, 0);
> > -	if (irq > 0)
> > +	irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
> > +	if (irq > 0) {
> >  		ec_dev->irq = irq;
> > -	else if (irq != -ENXIO) {
> > +		if (should_force_irq_wake_capable())
> > +			ec_dev->irq_wake = true;
> > +		else
> > +			ec_dev->irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
> > +	} else if (irq != -ENXIO) {
> >  		dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
> >  		return irq;
> >  	}
> 
> Still I do not like ambiguity behind irq > 0 vs. irqres.start.
> 
> For this, and if needed others, return plain error.
> Seems I gave the tag for the previous patch, consider
> that tag conditional (it seems I missed this).

On a related note, I was confusing a while because of the differences:

platform_get_irq_optional() and platform_get_irq_resource_optional():
Return: non-zero IRQ number on success, negative error number on failure.

acpi_dev_get_gpio_irq_resource():
Return: 0 on success, negative errno on failure.

acpi_dev_gpio_irq_get():
Return: Linux IRQ number (> %0) on success, negative errno on failure.


How about let platform_get_irq_resource_optional():
- Return 0 on success and negative errno on failure.
- The callee needs to retrieve the IRQ number from irqres.start.
?

Re: (subset) [PATCH v3 00/24] Improve IRQ wake capability reporting and update the cros_ec driver to use it

[Bjorn Andersson]

On Tue, 26 Dec 2023 12:21:04 -0700, Mark Hasemeyer wrote:
> Currently the cros_ec driver assumes that its associated interrupt is
> wake capable. This is an incorrect assumption as some Chromebooks use a
> separate wake pin, while others overload the interrupt for wake and IO.
> This patch train updates the driver to query the underlying ACPI/DT data
> to determine whether or not the IRQ should be enabled for wake.
> 
> Both the device tree and ACPI systems have methods for reporting IRQ
> wake capability. In device tree based systems, a node can advertise
> itself as a 'wakeup-source'. In ACPI based systems, GpioInt and
> Interrupt resource descriptors can use the 'SharedAndWake' or
> 'ExclusiveAndWake' share types.
> 
> [...]

Applied, thanks!

[14/24] arm64: dts: qcom: sc7180: Enable cros-ec-spi as wake source
        commit: f172a341ec1f66bac2866720931594e81f02ad4d
[15/24] arm64: dts: qcom: sc7280: Enable cros-ec-spi as wake source
        commit: a4b28b9ecc99673da875e214b1a06f1e0f0a24fa
[16/24] arm64: dts: qcom: sdm845: Enable cros-ec-spi as wake source
        commit: a7baa25bfbfdcd4e76414f29ab43317ded8d3e6e

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: [PATCH v3 00/24] Improve IRQ wake capability reporting and update the cros_ec driver to use it

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

On Tue Dec 26, 2023 at 8:21 PM CET, Mark Hasemeyer wrote:
> Currently the cros_ec driver assumes that its associated interrupt is
> wake capable. This is an incorrect assumption as some Chromebooks use a
> separate wake pin, while others overload the interrupt for wake and IO.
> This patch train updates the driver to query the underlying ACPI/DT data
> to determine whether or not the IRQ should be enabled for wake.
>
> Both the device tree and ACPI systems have methods for reporting IRQ
> wake capability. In device tree based systems, a node can advertise
> itself as a 'wakeup-source'. In ACPI based systems, GpioInt and
> Interrupt resource descriptors can use the 'SharedAndWake' or
> 'ExclusiveAndWake' share types.
>
> Some logic is added to the platform, ACPI, and DT subsystems to more
> easily pipe wakeirq information up to the driver.
>
> Changes in v3:
> -Rebase on linux-next
> -See each patch for patch specific changes
>
> Changes in v2:
> -Rebase on linux-next
> -Add cover letter
> -See each patch for patch specific changes
>
> Mark Hasemeyer (24):
[...]
>   ARM: dts: tegra: Enable cros-ec-spi as wake source
[...]
>   arm64: dts: tegra: Enable cros-ec-spi as wake source
[...]

Both patches applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]