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 v4:
-Rebase on linux-next
-See each patch for patch specific changes
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 | 90 ++++++++++++-------
drivers/base/property.c | 40 ++++++---
drivers/gpio/gpiolib-acpi.c | 28 ++++--
drivers/i2c/i2c-core-acpi.c | 43 ++++-----
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 | 40 +++++++--
drivers/platform/chrome/cros_ec_spi.c | 15 ++--
drivers/platform/chrome/cros_ec_uart.c | 14 ++-
include/linux/acpi.h | 25 +++---
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, 350 insertions(+), 172 deletions(-)
--
2.43.0.472.g3155946c3a-goog
In some cases, it would be nice to instantiate a struct resource with
custom flags. For example, creating an IRQ resource with a flag that
marks the interrupt as wake capable.
Add a set of macros to provide custom flag arguments.
Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-Update Andy's email to @linux.intel.com
-Add Reviewed-by tag
Changes in v3:
-New patch
include/linux/ioport.h | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index db7fe25f33700..a44e73ca058a8 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -163,28 +163,38 @@ enum {
.desc = IORES_DESC_NONE, \
}
+#define DEFINE_RES_IO_NAMED_FLAGS(_start, _size, _name, _flags) \
+ DEFINE_RES_NAMED((_start), (_size), (_name), (_flags) | IORESOURCE_IO)
#define DEFINE_RES_IO_NAMED(_start, _size, _name) \
- DEFINE_RES_NAMED((_start), (_size), (_name), IORESOURCE_IO)
+ DEFINE_RES_IO_NAMED_FLAGS((_start), (_size), (_name), 0)
#define DEFINE_RES_IO(_start, _size) \
DEFINE_RES_IO_NAMED((_start), (_size), NULL)
+#define DEFINE_RES_MEM_NAMED_FLAGS(_start, _size, _name, _flags) \
+ DEFINE_RES_NAMED((_start), (_size), (_name), (_flags) | IORESOURCE_MEM)
#define DEFINE_RES_MEM_NAMED(_start, _size, _name) \
- DEFINE_RES_NAMED((_start), (_size), (_name), IORESOURCE_MEM)
+ DEFINE_RES_MEM_NAMED_FLAGS((_start), (_size), (_name), 0)
#define DEFINE_RES_MEM(_start, _size) \
DEFINE_RES_MEM_NAMED((_start), (_size), NULL)
+#define DEFINE_RES_REG_NAMED_FLAGS(_start, _size, _name, _flags) \
+ DEFINE_RES_NAMED((_start), (_size), (_name), (_flags) | IORESOURCE_REG)
#define DEFINE_RES_REG_NAMED(_start, _size, _name) \
- DEFINE_RES_NAMED((_start), (_size), (_name), IORESOURCE_REG)
+ DEFINE_RES_REG_NAMED_FLAGS((_start), (_size), (_name), 0)
#define DEFINE_RES_REG(_start, _size) \
DEFINE_RES_REG_NAMED((_start), (_size), NULL)
+#define DEFINE_RES_IRQ_NAMED_FLAGS(_irq, _name, _flags) \
+ DEFINE_RES_NAMED((_irq), 1, (_name), (_flags) | IORESOURCE_IRQ)
#define DEFINE_RES_IRQ_NAMED(_irq, _name) \
- DEFINE_RES_NAMED((_irq), 1, (_name), IORESOURCE_IRQ)
+ DEFINE_RES_IRQ_NAMED_FLAGS((_irq), (_name), 0)
#define DEFINE_RES_IRQ(_irq) \
DEFINE_RES_IRQ_NAMED((_irq), NULL)
+#define DEFINE_RES_DMA_NAMED_FLAGS(_dma, _name, _flags) \
+ DEFINE_RES_NAMED((_dma), 1, (_name), (_flags) | IORESOURCE_DMA)
#define DEFINE_RES_DMA_NAMED(_dma, _name) \
- DEFINE_RES_NAMED((_dma), 1, (_name), IORESOURCE_DMA)
+ DEFINE_RES_DMA_NAMED_FLAGS((_dma), (_name), 0)
#define DEFINE_RES_DMA(_dma) \
DEFINE_RES_DMA_NAMED((_dma), NULL)
--
2.43.0.472.g3155946c3a-goog
Other information besides wake capability can be provided about GPIO
IRQs such as triggering, polarity, and sharability. Use resource flags
to provide this information to the caller if they want it.
This should keep the API more robust over time as flags are added,
modified, or removed. It also more closely matches acpi_irq_get() which
take a resource as an argument.
Rename the function to acpi_dev_get_gpio_irq_resource() to better
describe the function's new behavior.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-DEFINES_RES_NAMED->DEFINE_RES_IRQ_NAMED_FLAGS
-Indent fix
-Initialize struct resource on stack
-Remove ioport.h dependency in acpi.h
Changes in v3:
-Use DEFINE_RES_NAMED macro
-Add acpi_gpio_info.shareable doc
Changes in v2:
-Remove explicit cast to struct resource
-irq -> IRQ
drivers/gpio/gpiolib-acpi.c | 28 +++++++++++++++++++---------
drivers/i2c/i2c-core-acpi.c | 10 ++++++++--
include/linux/acpi.h | 25 +++++++++++--------------
3 files changed, 38 insertions(+), 25 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 88066826d8e5b..d14426c831187 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -12,6 +12,7 @@
#include <linux/errno.h>
#include <linux/export.h>
#include <linux/interrupt.h>
+#include <linux/ioport.h>
#include <linux/irq.h>
#include <linux/mutex.h>
#include <linux/pinctrl/pinctrl.h>
@@ -99,6 +100,7 @@ struct acpi_gpio_chip {
* @pin_config: pin bias as provided by ACPI
* @polarity: interrupt polarity as provided by ACPI
* @triggering: triggering type as provided by ACPI
+ * @shareable: share type as provided by ACPI (shared vs exclusive).
* @wake_capable: wake capability as provided by ACPI
* @debounce: debounce timeout as provided by ACPI
* @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping
@@ -111,6 +113,7 @@ struct acpi_gpio_info {
int polarity;
int triggering;
bool wake_capable;
+ bool shareable;
unsigned int debounce;
unsigned int quirks;
};
@@ -760,6 +763,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
lookup->info.debounce = agpio->debounce_timeout;
lookup->info.gpioint = gpioint;
lookup->info.wake_capable = acpi_gpio_irq_is_wake(&lookup->info.adev->dev, agpio);
+ lookup->info.shareable = agpio->shareable == ACPI_SHARED;
/*
* Polarity and triggering are only specified for GpioInt
@@ -1004,11 +1008,11 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
}
/**
- * acpi_dev_gpio_irq_wake_get_by() - Find GpioInt and translate it to Linux IRQ number
+ * acpi_dev_get_gpio_irq_resource() - Find GpioInt and populate resource struct
* @adev: pointer to a ACPI device to get IRQ from
* @name: optional name of GpioInt resource
* @index: index of GpioInt resource (starting from %0)
- * @wake_capable: Set to true if the IRQ is wake capable
+ * @r: pointer to resource to populate with IRQ information.
*
* If the device has one or more GpioInt resources, this function can be
* used to translate from the GPIO offset in the resource to the Linux IRQ
@@ -1023,10 +1027,12 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
* The GPIO is considered wake capable if the GpioInt resource specifies
* SharedAndWake or ExclusiveAndWake.
*
- * Return: Linux IRQ number (> %0) on success, negative errno on failure.
+ * IRQ number will be available in the resource structure.
+ *
+ * Return: 0 on success, negative errno on failure.
*/
-int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, int index,
- bool *wake_capable)
+int acpi_dev_get_gpio_irq_resource(struct acpi_device *adev, const char *name, int index,
+ struct resource *r)
{
int idx, i;
unsigned int irq_flags;
@@ -1045,6 +1051,7 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, in
if (info.gpioint && idx++ == index) {
unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
enum gpiod_flags dflags = GPIOD_ASIS;
+ unsigned long res_flags;
char label[32];
int irq;
@@ -1084,16 +1091,19 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, in
}
/* avoid suspend issues with GPIOs when systems are using S3 */
- if (wake_capable && acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
- *wake_capable = info.wake_capable;
+ if (info.wake_capable && !(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+ info.wake_capable = false;
- return irq;
+ res_flags = acpi_dev_irq_flags(info.triggering, info.polarity,
+ info.shareable, info.wake_capable);
+ *r = DEFINE_RES_IRQ_NAMED_FLAGS(irq, NULL, res_flags);
+ return 0;
}
}
return -ENOENT;
}
-EXPORT_SYMBOL_GPL(acpi_dev_gpio_irq_wake_get_by);
+EXPORT_SYMBOL_GPL(acpi_dev_get_gpio_irq_resource);
static acpi_status
acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index d6037a3286690..8126a87baf3d4 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -203,6 +203,7 @@ int i2c_acpi_get_irq(struct i2c_client *client, bool *wake_capable)
{
struct acpi_device *adev = ACPI_COMPANION(&client->dev);
struct list_head resource_list;
+ struct resource irqres;
struct i2c_acpi_irq_context irq_ctx = {
.irq = -ENOENT,
};
@@ -217,8 +218,13 @@ int i2c_acpi_get_irq(struct i2c_client *client, bool *wake_capable)
acpi_dev_free_resource_list(&resource_list);
- if (irq_ctx.irq == -ENOENT)
- irq_ctx.irq = acpi_dev_gpio_irq_wake_get(adev, 0, &irq_ctx.wake_capable);
+ if (irq_ctx.irq == -ENOENT) {
+ ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, &irqres);
+ if (ret)
+ return ret;
+ irq_ctx.irq = irqres.start;
+ irq_ctx.wake_capable = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
+ }
if (irq_ctx.irq < 0)
return irq_ctx.irq;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index b7165e52b3c68..a0cd733febe34 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -9,7 +9,6 @@
#define _LINUX_ACPI_H
#include <linux/errno.h>
-#include <linux/ioport.h> /* for struct resource */
#include <linux/resource_ext.h>
#include <linux/device.h>
#include <linux/mod_devicetable.h>
@@ -17,6 +16,7 @@
#include <linux/uuid.h>
#include <linux/node.h>
+struct resource;
struct irq_domain;
struct irq_domain_ops;
@@ -1232,8 +1232,8 @@ bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio);
bool acpi_gpio_get_io_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio);
-int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, int index,
- bool *wake_capable);
+int acpi_dev_get_gpio_irq_resource(struct acpi_device *adev, const char *name, int index,
+ struct resource *r);
#else
static inline bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio)
@@ -1245,28 +1245,25 @@ static inline bool acpi_gpio_get_io_resource(struct acpi_resource *ares,
{
return false;
}
-static inline int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name,
- int index, bool *wake_capable)
+static inline int acpi_dev_get_gpio_irq_resource(struct acpi_device *adev, const char *name,
+ int index, struct resource *r)
{
return -ENXIO;
}
#endif
-static inline int acpi_dev_gpio_irq_wake_get(struct acpi_device *adev, int index,
- bool *wake_capable)
+static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int index)
{
- return acpi_dev_gpio_irq_wake_get_by(adev, NULL, index, wake_capable);
-}
+ struct resource r = {};
+ int ret;
-static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name,
- int index)
-{
- return acpi_dev_gpio_irq_wake_get_by(adev, name, index, NULL);
+ ret = acpi_dev_get_gpio_irq_resource(adev, name, index, &r);
+ return ret ?: r.start;
}
static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
{
- return acpi_dev_gpio_irq_wake_get_by(adev, NULL, index, NULL);
+ return acpi_dev_gpio_irq_get_by(adev, NULL, index);
}
/* Device properties */
--
2.43.0.472.g3155946c3a-goog
The i2c_acpi_irq_context structure provides redundant information that
can be provided with struct resource.
Refactor i2c_acpi_get_irq() to use struct resource instead of struct
i2c_acpi_irq_context.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-Use Andy's @linux.intel.com email
-Remove blank line in commit message
-More error handling refactoring in i2c_acpi_get_irq()
-Remove struct i2c_acpi_irq_context as it's unused
Changes in v3:
-Add Suggested-by
-Check resource flags for valid irq
-Drop error pointer check
-Invert error checking logic in i2c_acpi_get_irq()
-Drop redundant 0 in struct resource init
-Drop unnecessary check for irq > 0 when setting I2C_CLIENT_WAKE
Changes in v2:
-New patch
drivers/i2c/i2c-core-acpi.c | 49 +++++++++++++------------------------
drivers/i2c/i2c-core-base.c | 6 ++---
drivers/i2c/i2c-core.h | 4 +--
3 files changed, 22 insertions(+), 37 deletions(-)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 8126a87baf3d4..4c3df540c2f4b 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -137,11 +137,6 @@ static const struct acpi_device_id i2c_acpi_ignored_device_ids[] = {
{}
};
-struct i2c_acpi_irq_context {
- int irq;
- bool wake_capable;
-};
-
static int i2c_acpi_do_lookup(struct acpi_device *adev,
struct i2c_acpi_lookup *lookup)
{
@@ -175,64 +170,54 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
static int i2c_acpi_add_irq_resource(struct acpi_resource *ares, void *data)
{
- struct i2c_acpi_irq_context *irq_ctx = data;
- struct resource r;
+ struct resource *r = data;
- if (irq_ctx->irq > 0)
+ if (r->flags)
return 1;
- if (!acpi_dev_resource_interrupt(ares, 0, &r))
+ if (!acpi_dev_resource_interrupt(ares, 0, r))
return 1;
- irq_ctx->irq = i2c_dev_irq_from_resources(&r, 1);
- irq_ctx->wake_capable = r.flags & IORESOURCE_IRQ_WAKECAPABLE;
+ i2c_dev_irq_from_resources(r, 1);
return 1; /* No need to add resource to the list */
}
/**
- * i2c_acpi_get_irq - get device IRQ number from ACPI
+ * i2c_acpi_get_irq - get device IRQ number from ACPI and populate resource
* @client: Pointer to the I2C client device
- * @wake_capable: Set to true if the IRQ is wake capable
+ * @r: resource with populated IRQ information
*
* Find the IRQ number used by a specific client device.
*
* Return: The IRQ number or an error code.
*/
-int i2c_acpi_get_irq(struct i2c_client *client, bool *wake_capable)
+int i2c_acpi_get_irq(struct i2c_client *client, struct resource *r)
{
struct acpi_device *adev = ACPI_COMPANION(&client->dev);
struct list_head resource_list;
- struct resource irqres;
- struct i2c_acpi_irq_context irq_ctx = {
- .irq = -ENOENT,
- };
int ret;
+ if (!r)
+ return -EINVAL;
+
INIT_LIST_HEAD(&resource_list);
ret = acpi_dev_get_resources(adev, &resource_list,
- i2c_acpi_add_irq_resource, &irq_ctx);
+ i2c_acpi_add_irq_resource, r);
if (ret < 0)
return ret;
acpi_dev_free_resource_list(&resource_list);
- if (irq_ctx.irq == -ENOENT) {
- ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, &irqres);
- if (ret)
- return ret;
- irq_ctx.irq = irqres.start;
- irq_ctx.wake_capable = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
- }
+ if (r->flags)
+ return r->start;
- if (irq_ctx.irq < 0)
- return irq_ctx.irq;
+ ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, r);
+ if (ret)
+ return ret;
- if (wake_capable)
- *wake_capable = irq_ctx.wake_capable;
-
- return irq_ctx.irq;
+ return r->start;
}
static int i2c_acpi_get_info(struct acpi_device *adev,
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 3bd48d4b6318f..0339c298ba50b 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -513,10 +513,10 @@ static int i2c_device_probe(struct device *dev)
if (irq == -EINVAL || irq == -ENODATA)
irq = of_irq_get(dev->of_node, 0);
} else if (ACPI_COMPANION(dev)) {
- bool wake_capable;
+ struct resource r = {};
- irq = i2c_acpi_get_irq(client, &wake_capable);
- if (irq > 0 && wake_capable)
+ irq = i2c_acpi_get_irq(client, &r);
+ if (r.flags & IORESOURCE_IRQ_WAKECAPABLE)
client->flags |= I2C_CLIENT_WAKE;
}
if (irq == -EPROBE_DEFER) {
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 05b8b8dfa9bdd..b5dc559c49d11 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -61,11 +61,11 @@ static inline int __i2c_check_suspended(struct i2c_adapter *adap)
#ifdef CONFIG_ACPI
void i2c_acpi_register_devices(struct i2c_adapter *adap);
-int i2c_acpi_get_irq(struct i2c_client *client, bool *wake_capable);
+int i2c_acpi_get_irq(struct i2c_client *client, struct resource *r);
#else /* CONFIG_ACPI */
static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
-static inline int i2c_acpi_get_irq(struct i2c_client *client, bool *wake_capable)
+static inline int i2c_acpi_get_irq(struct i2c_client *client, struct resource *r)
{
return 0;
}
--
2.43.0.472.g3155946c3a-goog
The wording in the current documentation is a little strong. The
intention was not to fix any particular interrupt as wakeup capable but
leave those details to the device. It wasn't intended to enforce any
rules as what can be or can't be a wakeup interrupt.
Soften the wording to not mandate that the 'wakeup-source' property be
used, and clarify what it means when an interrupt is marked (or not
marked) for wakeup.
Link: https://lore.kernel.org/all/ZYAjxxHcCOgDVMTQ@bogus/
Link: https://lore.kernel.org/all/CAL_Jsq+MYwOG40X26cYmO9EkZ9xqWrXDi03MaRfxnV-+VGkXWQ@mail.gmail.com/
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit title prefixes
Changes in v2:
-New patch
.../bindings/power/wakeup-source.txt | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/power/wakeup-source.txt b/Documentation/devicetree/bindings/power/wakeup-source.txt
index 697333a56d5e2..75bc20b95688f 100644
--- a/Documentation/devicetree/bindings/power/wakeup-source.txt
+++ b/Documentation/devicetree/bindings/power/wakeup-source.txt
@@ -3,16 +3,20 @@ Specifying wakeup capability for devices
Any device nodes
----------------
-Nodes that describe devices which has wakeup capability must contain an
+Nodes that describe devices which have wakeup capability may contain a
"wakeup-source" boolean property.
-Also, if device is marked as a wakeup source, then all the primary
-interrupt(s) can be used as wakeup interrupt(s).
+If the device is marked as a wakeup-source, interrupt wake capability depends
+on the device specific "interrupt-names" property. If no interrupts are labeled
+as wake capable, then it is up to the device to determine which interrupts can
+wake the system.
-However if the devices have dedicated interrupt as the wakeup source
-then they need to specify/identify the same using device specific
-interrupt name. In such cases only that interrupt can be used as wakeup
-interrupt.
+However if a device has a dedicated interrupt as the wakeup source, then it
+needs to specify/identify it using a device specific interrupt name. In such
+cases only that interrupt can be used as a wakeup interrupt.
+
+While various legacy interrupt names exist, new devices should use "wakeup" as
+the canonical interrupt name.
List of legacy properties and respective binding document
---------------------------------------------------------
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi | 1 +
arch/arm/boot/dts/nvidia/tegra124-venice2.dts | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi b/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi
index a2ee371802004..8125c1b3e8d79 100644
--- a/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi
+++ b/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi
@@ -338,6 +338,7 @@ cros_ec: cros-ec@0 {
interrupt-parent = <&gpio>;
interrupts = <TEGRA_GPIO(C, 7) IRQ_TYPE_LEVEL_LOW>;
reg = <0>;
+ wakeup-source;
google,cros-ec-spi-msg-delay = <2000>;
diff --git a/arch/arm/boot/dts/nvidia/tegra124-venice2.dts b/arch/arm/boot/dts/nvidia/tegra124-venice2.dts
index 3924ee385dee0..df98dc2a67b85 100644
--- a/arch/arm/boot/dts/nvidia/tegra124-venice2.dts
+++ b/arch/arm/boot/dts/nvidia/tegra124-venice2.dts
@@ -857,6 +857,7 @@ cros_ec: cros-ec@0 {
interrupt-parent = <&gpio>;
interrupts = <TEGRA_GPIO(C, 7) IRQ_TYPE_LEVEL_LOW>;
reg = <0>;
+ wakeup-source;
google,cros-ec-spi-msg-delay = <2000>;
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts b/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts
index 4e757b6e28e1c..3759742d38cac 100644
--- a/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts
@@ -967,6 +967,7 @@ cros_ec: cros-ec@0 {
reg = <0>;
spi-max-frequency = <3125000>;
google,has-vbc-nvram;
+ wakeup-source;
controller-data {
samsung,spi-feedback-delay = <1>;
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts b/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts
index f91bc4ae008e4..9bbbdce9103a6 100644
--- a/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts
@@ -949,6 +949,7 @@ cros_ec: cros-ec@0 {
reg = <0>;
spi-max-frequency = <3125000>;
google,has-vbc-nvram;
+ wakeup-source;
controller-data {
samsung,spi-feedback-delay = <1>;
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
index 8d614ac2c58ed..335aed42dc9e3 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
@@ -1155,6 +1155,7 @@ cros_ec: ec@0 {
spi-max-frequency = <12000000>;
interrupts-extended = <&pio 0 IRQ_TYPE_LEVEL_LOW>;
google,cros-ec-spi-msg-delay = <500>;
+ wakeup-source;
i2c_tunnel: i2c-tunnel0 {
compatible = "google,cros-ec-i2c-tunnel";
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi b/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi
index 092316be67f74..1554fe36e60fe 100644
--- a/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi
@@ -112,6 +112,7 @@ cros_ec: ec@0 {
pinctrl-names = "default";
pinctrl-0 = <&ec_int>;
spi-max-frequency = <3000000>;
+ wakeup-source;
i2c_tunnel: i2c-tunnel {
compatible = "google,cros-ec-i2c-tunnel";
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
index 5506de83f61d4..08261164ab18d 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
@@ -924,6 +924,7 @@ cros_ec: cros-ec@0 {
interrupts-extended = <&pio 151 IRQ_TYPE_LEVEL_LOW>;
pinctrl-names = "default";
pinctrl-0 = <&ec_ap_int_odl>;
+ wakeup-source;
i2c_tunnel: i2c-tunnel {
compatible = "google,cros-ec-i2c-tunnel";
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
index f2281250ac35d..ab44d382f757e 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
@@ -1332,6 +1332,7 @@ cros_ec: ec@0 {
spi-max-frequency = <3000000>;
pinctrl-names = "default";
pinctrl-0 = <&cros_ec_int>;
+ wakeup-source;
#address-cells = <1>;
#size-cells = <0>;
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm64/boot/dts/nvidia/tegra132-norrin.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts b/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts
index bbc2e9bef08da..14d58859bb55c 100644
--- a/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts
@@ -762,6 +762,7 @@ ec: cros-ec@0 {
interrupt-parent = <&gpio>;
interrupts = <TEGRA_GPIO(C, 7) IRQ_TYPE_LEVEL_LOW>;
reg = <0>;
+ wakeup-source;
google,cros-ec-spi-msg-delay = <2000>;
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
index bbdcd441c049d..2edb270d0bc2f 100644
--- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
@@ -1149,6 +1149,7 @@ cros_ec: ec@0 {
pinctrl-names = "default";
pinctrl-0 = <&cros_ec_int>;
spi-max-frequency = <3000000>;
+ wakeup-source;
keyboard-backlight {
compatible = "google,cros-kbd-led-backlight";
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Reviewed-by: Douglas Anderson <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-Add Douglas's Reviewed-by tag from v2 review
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 46aaeba286047..f3a6da8b28901 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -649,6 +649,7 @@ cros_ec: ec@0 {
pinctrl-names = "default";
pinctrl-0 = <&ap_ec_int_l>;
spi-max-frequency = <3000000>;
+ wakeup-source;
cros_ec_pwm: pwm {
compatible = "google,cros-ec-pwm";
--
2.43.0.472.g3155946c3a-goog
Add wake capability information to the IRQ resource. Wake capability is
assumed based on conventions provided in the devicetree wakeup-source
binding documentation. An interrupt is considered wake capable if the
following are true:
1. A wakeup-source property exits in the same device node as the
interrupt.
2. The IRQ is marked as dedicated by setting its interrupt-name to
"wakeup".
The wakeup-source documentation states that dedicated interrupts can use
device specific interrupt names and device drivers are still welcome to
use their own naming schemes. This API is provided as a helper if one is
willing to conform to the above conventions.
The ACPI subsystems already provides similar APIs that allow one to
query the wake capability of an IRQ. This brings closer feature parity
to the devicetree.
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-Add Rob's Reviewed-by tag from v2
-Ignored Andy's Reviewed-by tag per his request:
https://lore.kernel.org/all/[email protected]/
Changes in v3:
-Use DEFINE_RES_IRQ_NAMED_FLAGS macro
Changes in v2:
-Update logic to return true only if wakeup-source property and
"wakeup" interrupt-name are defined
-irq->IRQ, api->API
drivers/of/irq.c | 39 +++++++++++++++++++++++++++++++++++----
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 174900072c18c..cdecdc3515f88 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -383,11 +383,39 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
}
EXPORT_SYMBOL_GPL(of_irq_parse_one);
+/**
+ * __of_irq_wake_capable - Determine whether a given IRQ index is wake capable
+ *
+ * The IRQ is considered wake capable if the following are true:
+ * 1. wakeup-source property exists
+ * 2. provided IRQ index is labelled as a dedicated wakeirq
+ *
+ * This logic assumes the provided IRQ index is valid.
+ *
+ * @dev: pointer to device tree node
+ * @index: zero-based index of the IRQ
+ * Return: True if provided IRQ index for #dev is wake capable. False otherwise.
+ */
+static bool __of_irq_wake_capable(const struct device_node *dev, int index)
+{
+ int wakeindex;
+
+ if (!of_property_read_bool(dev, "wakeup-source"))
+ return false;
+
+ wakeindex = of_property_match_string(dev, "interrupt-names", "wakeup");
+ return wakeindex >= 0 && wakeindex == index;
+}
+
/**
* of_irq_to_resource - Decode a node's IRQ and return it as a resource
* @dev: pointer to device tree node
- * @index: zero-based index of the irq
+ * @index: zero-based index of the IRQ
* @r: pointer to resource structure to return result into.
+ *
+ * Return: Linux IRQ number on success, or 0 on the IRQ mapping failure, or
+ * -EPROBE_DEFER if the IRQ domain is not yet created, or error code in case
+ * of any other failure.
*/
int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
{
@@ -399,6 +427,7 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
/* Only dereference the resource if both the
* resource and the irq are valid. */
if (r && irq) {
+ u32 irq_flags;
const char *name = NULL;
memset(r, 0, sizeof(*r));
@@ -409,9 +438,11 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
of_property_read_string_index(dev, "interrupt-names", index,
&name);
- r->start = r->end = irq;
- r->flags = IORESOURCE_IRQ | irqd_get_trigger_type(irq_get_irq_data(irq));
- r->name = name ? name : of_node_full_name(dev);
+ irq_flags = irqd_get_trigger_type(irq_get_irq_data(irq));
+ if (__of_irq_wake_capable(dev, index))
+ irq_flags |= IORESOURCE_IRQ_WAKECAPABLE;
+
+ *r = DEFINE_RES_IRQ_NAMED_FLAGS(irq, name ?: of_node_full_name(dev), irq_flags);
}
return irq;
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Reviewed-by: Douglas Anderson <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-Add Douglas's Reviewed-by tag from v2 review
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 1 +
arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index 9ea6636125ad9..2ba4ea60cb147 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -548,6 +548,7 @@ cros_ec: ec@0 {
pinctrl-names = "default";
pinctrl-0 = <&ap_ec_int_l>;
spi-max-frequency = <3000000>;
+ wakeup-source;
cros_ec_pwm: pwm {
compatible = "google,cros-ec-pwm";
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
index ebae545c587c4..fbfac7534d3c6 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
@@ -19,6 +19,7 @@ cros_ec: ec@0 {
pinctrl-names = "default";
pinctrl-0 = <&ap_ec_int_l>;
spi-max-frequency = <3000000>;
+ wakeup-source;
cros_ec_pwm: pwm {
compatible = "google,cros-ec-pwm";
--
2.43.0.472.g3155946c3a-goog
Similar to of_irq_to_resource_table(), add a default implementation of
of_irq_to_resource() for systems that don't have CONFIG_OF_IRQ defined.
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-Add Rob's Ack tag from v2
include/linux/of_irq.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index d6d3eae2f1452..0d73b2ca92d31 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -34,8 +34,6 @@ static inline int of_irq_parse_oldworld(const struct device_node *device, int in
extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
extern unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
-extern int of_irq_to_resource(struct device_node *dev, int index,
- struct resource *r);
#ifdef CONFIG_OF_IRQ
extern void of_irq_init(const struct of_device_id *matches);
@@ -44,6 +42,7 @@ extern int of_irq_parse_one(struct device_node *device, int index,
extern int of_irq_count(struct device_node *dev);
extern int of_irq_get(struct device_node *dev, int index);
extern int of_irq_get_byname(struct device_node *dev, const char *name);
+extern int of_irq_to_resource(struct device_node *dev, int index, struct resource *r);
extern int of_irq_to_resource_table(struct device_node *dev,
struct resource *res, int nr_irqs);
extern struct device_node *of_irq_find_parent(struct device_node *child);
@@ -76,6 +75,11 @@ static inline int of_irq_get_byname(struct device_node *dev, const char *name)
{
return 0;
}
+static inline int of_irq_to_resource(struct device_node *dev, int index,
+ struct resource *r)
+{
+ return 0;
+}
static inline int of_irq_to_resource_table(struct device_node *dev,
struct resource *res, int nr_irqs)
{
--
2.43.0.472.g3155946c3a-goog
The extern keyword is implicit for function declarations.
Remove it where possible and adjust the line wrapping accordingly.
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-Add Rob's Ack tag from v2
Changes in v2:
-New patch
include/linux/of_irq.h | 35 +++++++++++++++++------------------
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 0d73b2ca92d31..a130dcbc4bb45 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -32,27 +32,26 @@ static inline int of_irq_parse_oldworld(const struct device_node *device, int in
}
#endif /* CONFIG_PPC32 && CONFIG_PPC_PMAC */
-extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
-extern unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
+int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
+unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
#ifdef CONFIG_OF_IRQ
-extern void of_irq_init(const struct of_device_id *matches);
-extern int of_irq_parse_one(struct device_node *device, int index,
- struct of_phandle_args *out_irq);
-extern int of_irq_count(struct device_node *dev);
-extern int of_irq_get(struct device_node *dev, int index);
-extern int of_irq_get_byname(struct device_node *dev, const char *name);
-extern int of_irq_to_resource(struct device_node *dev, int index, struct resource *r);
-extern int of_irq_to_resource_table(struct device_node *dev,
- struct resource *res, int nr_irqs);
-extern struct device_node *of_irq_find_parent(struct device_node *child);
-extern struct irq_domain *of_msi_get_domain(struct device *dev,
+void of_irq_init(const struct of_device_id *matches);
+int of_irq_parse_one(struct device_node *device, int index,
+ struct of_phandle_args *out_irq);
+int of_irq_count(struct device_node *dev);
+int of_irq_get(struct device_node *dev, int index);
+int of_irq_get_byname(struct device_node *dev, const char *name);
+int of_irq_to_resource(struct device_node *dev, int index, struct resource *r);
+int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
+ int nr_irqs);
+struct device_node *of_irq_find_parent(struct device_node *child);
+struct irq_domain *of_msi_get_domain(struct device *dev,
struct device_node *np,
enum irq_domain_bus_token token);
-extern struct irq_domain *of_msi_map_get_device_domain(struct device *dev,
- u32 id,
- u32 bus_token);
-extern void of_msi_configure(struct device *dev, struct device_node *np);
+struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 id,
+ u32 bus_token);
+void of_msi_configure(struct device *dev, struct device_node *np);
u32 of_msi_map_id(struct device *dev, struct device_node *msi_np, u32 id_in);
#else
static inline void of_irq_init(const struct of_device_id *matches)
@@ -117,7 +116,7 @@ static inline u32 of_msi_map_id(struct device *dev,
* implements it differently. However, the prototype is the same for all,
* so declare it here regardless of the CONFIG_OF_IRQ setting.
*/
-extern unsigned int irq_of_parse_and_map(struct device_node *node, int index);
+unsigned int irq_of_parse_and_map(struct device_node *node, int index);
#else /* !CONFIG_OF && !CONFIG_SPARC */
static inline unsigned int irq_of_parse_and_map(struct device_node *dev,
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Reviewed-by: Douglas Anderson <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-Add Douglas's Reviewed-by tag from v2 review
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
index 0ab5e8f53ac9f..e8276db9eabb2 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
@@ -852,6 +852,7 @@ cros_ec: ec@0 {
pinctrl-names = "default";
pinctrl-0 = <&ec_ap_int_l>;
spi-max-frequency = <3000000>;
+ wakeup-source;
cros_ec_pwm: pwm {
compatible = "google,cros-ec-pwm";
--
2.43.0.472.g3155946c3a-goog
The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.
Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake.
Add the wakeup-source property to all cros-ec-spi compatible device
nodes to signify to the driver that they should still be a valid wakeup
source.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
(no changes since v3)
Changes in v3:
-Update commit message to provide details of the motivation behind the
change
Changes in v2:
-Split by arch/soc
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index 789fd0dcc88ba..b5734e056aef1 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -603,6 +603,7 @@ cros_ec: ec@0 {
pinctrl-names = "default";
pinctrl-0 = <&ec_ap_int_l>;
spi-max-frequency = <3000000>;
+ wakeup-source;
i2c_tunnel: i2c-tunnel {
compatible = "google,cros-ec-i2c-tunnel";
--
2.43.0.472.g3155946c3a-goog
The underlying ACPI and OF subsystems provide their own APIs which
provide IRQ information as a struct resource. This allows callers to get
more information about the IRQ by looking at the resource flags. For
example, whether or not an IRQ is wake capable.
Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-Add Sakari's Reviewed-by tag from v2
-Remove ioport.h dependency in fwnode.h
-Use Andy's @linux.intel.com email
Changes in v3:
-Add Suggested-by tag
-Initialize struct resource to 0 on stack
-EXPORT_SYMBOL()->EXPORT_SYMBOL_GPL()
-Remove extra space in commit message
-Reformat fwnode_irq_get_resource() declaration
Changes in v2:
-New patch
drivers/acpi/property.c | 11 +++++------
drivers/base/property.c | 32 +++++++++++++++++++++++++-------
drivers/of/property.c | 8 ++++----
include/linux/fwnode.h | 8 +++++---
include/linux/property.h | 2 ++
5 files changed, 41 insertions(+), 20 deletions(-)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index a6ead5204046b..891fff5a16797 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1627,17 +1627,16 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
return 0;
}
-static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
- unsigned int index)
+static int acpi_fwnode_irq_get_resource(const struct fwnode_handle *fwnode, unsigned int index,
+ struct resource *r)
{
- struct resource res;
int ret;
- ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), index, &res);
+ ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), index, r);
if (ret)
return ret;
- return res.start;
+ return r->start;
}
#define DECLARE_ACPI_FWNODE_OPS(ops) \
@@ -1664,7 +1663,7 @@ static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
acpi_graph_get_remote_endpoint, \
.graph_get_port_parent = acpi_fwnode_get_parent, \
.graph_parse_endpoint = acpi_fwnode_graph_parse_endpoint, \
- .irq_get = acpi_fwnode_irq_get, \
+ .irq_get_resource = acpi_fwnode_irq_get_resource, \
}; \
EXPORT_SYMBOL_GPL(ops)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index a1b01ab420528..441899171d19d 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1046,6 +1046,29 @@ void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index)
}
EXPORT_SYMBOL(fwnode_iomap);
+/**
+ * fwnode_irq_get_resource - Get IRQ directly from a fwnode and populate
+ * the resource struct
+ * @fwnode: Pointer to the firmware node
+ * @index: Zero-based index of the IRQ
+ * @r: Pointer to resource to populate with IRQ information.
+ *
+ * Return: Linux IRQ number on success. Negative errno on failure.
+ */
+int fwnode_irq_get_resource(const struct fwnode_handle *fwnode, unsigned int index,
+ struct resource *r)
+{
+ int ret;
+
+ ret = fwnode_call_int_op(fwnode, irq_get_resource, index, r);
+ /* We treat mapping errors as invalid case */
+ if (ret == 0)
+ return -EINVAL;
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(fwnode_irq_get_resource);
+
/**
* fwnode_irq_get - Get IRQ directly from a fwnode
* @fwnode: Pointer to the firmware node
@@ -1055,14 +1078,9 @@ EXPORT_SYMBOL(fwnode_iomap);
*/
int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
{
- int ret;
+ struct resource r = {};
- ret = fwnode_call_int_op(fwnode, irq_get, index);
- /* We treat mapping errors as invalid case */
- if (ret == 0)
- return -EINVAL;
-
- return ret;
+ return fwnode_irq_get_resource(fwnode, index, &r);
}
EXPORT_SYMBOL(fwnode_irq_get);
diff --git a/drivers/of/property.c b/drivers/of/property.c
index afdaefbd03f61..864ea5fa5702b 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1425,10 +1425,10 @@ static void __iomem *of_fwnode_iomap(struct fwnode_handle *fwnode, int index)
#endif
}
-static int of_fwnode_irq_get(const struct fwnode_handle *fwnode,
- unsigned int index)
+static int of_fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
+ unsigned int index, struct resource *r)
{
- return of_irq_get(to_of_node(fwnode), index);
+ return of_irq_to_resource(to_of_node(fwnode), index, r);
}
static int of_fwnode_add_links(struct fwnode_handle *fwnode)
@@ -1469,7 +1469,7 @@ const struct fwnode_operations of_fwnode_ops = {
.graph_get_port_parent = of_fwnode_graph_get_port_parent,
.graph_parse_endpoint = of_fwnode_graph_parse_endpoint,
.iomap = of_fwnode_iomap,
- .irq_get = of_fwnode_irq_get,
+ .irq_get_resource = of_fwnode_irq_get_resource,
.add_links = of_fwnode_add_links,
};
EXPORT_SYMBOL_GPL(of_fwnode_ops);
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 2a72f55d26eb8..b82c9c072bcc9 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -9,12 +9,13 @@
#ifndef _LINUX_FWNODE_H_
#define _LINUX_FWNODE_H_
-#include <linux/types.h>
-#include <linux/list.h>
#include <linux/bits.h>
#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/types.h>
struct fwnode_operations;
+struct resource;
struct device;
/*
@@ -164,7 +165,8 @@ struct fwnode_operations {
int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode,
struct fwnode_endpoint *endpoint);
void __iomem *(*iomap)(struct fwnode_handle *fwnode, int index);
- int (*irq_get)(const struct fwnode_handle *fwnode, unsigned int index);
+ int (*irq_get_resource)(const struct fwnode_handle *fwnode,
+ unsigned int index, struct resource *r);
int (*add_links)(struct fwnode_handle *fwnode);
};
diff --git a/include/linux/property.h b/include/linux/property.h
index e6516d0b7d52a..685ba72a8ce9e 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -190,6 +190,8 @@ struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
void fwnode_handle_put(struct fwnode_handle *fwnode);
int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
+int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
+ unsigned int index, struct resource *r);
int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
unsigned int device_get_child_node_count(const struct device *dev);
--
2.43.0.472.g3155946c3a-goog
Some of the exported functions use EXPORT_SYMBOL() instead of
EXPORT_SYMBOL_GPL() and are inconsistent with the other exported
functions in the module. The underlying APCI/OF struct fwnode_operations
implementations are also exported via EXPORT_SYMBOL_GPL().
Update them to use the EXPORT_SYMBOL_GPL() macro.
Suggested-by: Sakari Ailus <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-EXPORT_SYMBOL->EXPORT_SYMBOL()
-Add Andy's Reviewed-by tag
Changes in v3:
-New patch
drivers/base/property.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 441899171d19d..4f686516cac82 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1044,7 +1044,7 @@ void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index)
{
return fwnode_call_ptr_op(fwnode, iomap, index);
}
-EXPORT_SYMBOL(fwnode_iomap);
+EXPORT_SYMBOL_GPL(fwnode_iomap);
/**
* fwnode_irq_get_resource - Get IRQ directly from a fwnode and populate
@@ -1082,7 +1082,7 @@ int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
return fwnode_irq_get_resource(fwnode, index, &r);
}
-EXPORT_SYMBOL(fwnode_irq_get);
+EXPORT_SYMBOL_GPL(fwnode_irq_get);
/**
* fwnode_irq_get_byname - Get IRQ from a fwnode using its name
@@ -1110,7 +1110,7 @@ int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name)
return fwnode_irq_get(fwnode, index);
}
-EXPORT_SYMBOL(fwnode_irq_get_byname);
+EXPORT_SYMBOL_GPL(fwnode_irq_get_byname);
/**
* fwnode_graph_get_next_endpoint - Get next endpoint firmware node
@@ -1355,7 +1355,7 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
return fwnode_call_int_op(fwnode, graph_parse_endpoint, endpoint);
}
-EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
+EXPORT_SYMBOL_GPL(fwnode_graph_parse_endpoint);
const void *device_get_match_data(const struct device *dev)
{
--
2.43.0.472.g3155946c3a-goog
Unify handling of ACPI, GPIO, devictree, and platform resource
interrupts in platform_get_irq_optional(). Each of these subsystems
provide their own APIs which provide IRQ information as a struct
resource. This simplifies the logic of the function and allows callers
to get more information about the IRQ by looking at the resource flags.
For example, whether or not an IRQ is wake capable.
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-platform_get_irq_optional() returns 0 on success
Changes in v3:
-Remove PTR_ERR check
-Move platform_res assignment
-Check for irq == 0 to trigger WARN msg
-Refactor error handling of acpi_dev_get_gpio_irq_resource() to be
consistent with fwnode_irq_get_resource()
-Remove extra blank lines
-Initialize struct resource on stack
Changes in v2:
-irq->IRQ
-Remove cast to struct resource
-Conform to get_optional() function naming
-Revert move of irq_get_irq_data()
-Add NULL check on struct resource*
-Use fwnode to retrieve IRQ for DT/ACPI
drivers/base/platform.c | 90 +++++++++++++++++++++------------
include/linux/platform_device.h | 3 ++
2 files changed, 61 insertions(+), 32 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 10c5779634182..8a42b48922e68 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -151,50 +151,45 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
#endif /* CONFIG_HAS_IOMEM */
/**
- * platform_get_irq_optional - get an optional IRQ for a device
+ * platform_get_irq_resource_optional - get an optional IRQ for a device and
+ * populate the resource struct
* @dev: platform device
* @num: IRQ number index
+ * @r: pointer to resource to populate with IRQ information.
*
* Gets an IRQ for a platform device. Device drivers should check the return
- * value for errors so as to not pass a negative integer value to the
- * request_irq() APIs. This is the same as platform_get_irq(), except that it
- * does not print an error message if an IRQ can not be obtained.
+ * value for errors. If no error is returned, the IRQ can be found in r->start.
*
* For example::
*
- * int irq = platform_get_irq_optional(pdev, 0);
- * if (irq < 0)
- * return irq;
+ * int res = platform_get_irq_resource_optional(pdev, 0, &res);
+ * if (!res)
+ * return res.start;
*
- * Return: non-zero IRQ number on success, negative error number on failure.
+ * Return: 0 on success, negative error number on failure.
*/
-int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+int platform_get_irq_resource_optional(struct platform_device *dev,
+ unsigned int num, struct resource *r)
{
int ret;
+
+ if (!r)
+ return -EINVAL;
+
#ifdef CONFIG_SPARC
/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
if (!dev || num >= dev->archdata.num_irqs)
goto out_not_found;
- ret = dev->archdata.irqs[num];
+ *r = DEFINE_RES_IRQ(dev->archdata.irqs[num]);
+ ret = 0;
goto out;
#else
struct fwnode_handle *fwnode = dev_fwnode(&dev->dev);
- struct resource *r;
- if (is_of_node(fwnode)) {
- ret = of_irq_get(to_of_node(fwnode), num);
- if (ret > 0 || ret == -EPROBE_DEFER)
- goto out;
- }
-
- r = platform_get_resource(dev, IORESOURCE_IRQ, num);
- if (is_acpi_device_node(fwnode)) {
- if (r && r->flags & IORESOURCE_DISABLED) {
- ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), num, r);
- if (ret)
- goto out;
- }
- }
+ ret = fwnode_irq_get_resource(fwnode, num, r);
+ ret = ret < 0 ? ret : 0;
+ if (!ret || ret == -EPROBE_DEFER)
+ goto out;
/*
* The resources may pass trigger flags to the irqs that need
@@ -202,7 +197,9 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
* IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
* settings.
*/
- if (r && r->flags & IORESOURCE_BITS) {
+ struct resource *platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
+
+ if (platform_res && platform_res->flags & IORESOURCE_BITS) {
struct irq_data *irqd;
irqd = irq_get_irq_data(r->start);
@@ -211,8 +208,9 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
}
- if (r) {
- ret = r->start;
+ if (platform_res) {
+ *r = *platform_res;
+ ret = 0;
goto out;
}
@@ -224,9 +222,9 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
* allows a common code path across either kind of resource.
*/
if (num == 0 && is_acpi_device_node(fwnode)) {
- ret = acpi_dev_gpio_irq_get(to_acpi_device_node(fwnode), num);
+ ret = acpi_dev_get_gpio_irq_resource(to_acpi_device_node(fwnode), NULL, num, r);
/* Our callers expect -ENXIO for missing IRQs. */
- if (ret >= 0 || ret == -EPROBE_DEFER)
+ if (!ret || ret == -EPROBE_DEFER)
goto out;
}
@@ -234,11 +232,11 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
out_not_found:
ret = -ENXIO;
out:
- if (WARN(!ret, "0 is an invalid IRQ number\n"))
+ if (WARN(!ret && !r->start, "0 is an invalid IRQ number\n"))
return -EINVAL;
return ret;
}
-EXPORT_SYMBOL_GPL(platform_get_irq_optional);
+EXPORT_SYMBOL_GPL(platform_get_irq_resource_optional);
/**
* platform_get_irq - get an IRQ for a device
@@ -270,6 +268,34 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
}
EXPORT_SYMBOL_GPL(platform_get_irq);
+/**
+ * platform_get_irq_optional - get an optional IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ *
+ * Gets an IRQ for a platform device. Device drivers should check the return
+ * value for errors so as to not pass a negative integer value to the
+ * request_irq() APIs. This is the same as platform_get_irq(), except that it
+ * does not print an error message if an IRQ can not be obtained.
+ *
+ * For example::
+ *
+ * int irq = platform_get_irq_optional(pdev, 0);
+ * if (irq < 0)
+ * return irq;
+ *
+ * Return: non-zero IRQ number on success, negative error number on failure.
+ */
+int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+{
+ int ret;
+ struct resource r = {};
+
+ ret = platform_get_irq_resource_optional(dev, num, &r);
+ return ret ?: r.start;
+}
+EXPORT_SYMBOL_GPL(platform_get_irq_optional);
+
/**
* platform_irq_count - Count the number of IRQs a platform device uses
* @dev: platform device
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 7a41c72c19591..2117f817d9c9c 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -102,6 +102,9 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
extern int platform_get_irq(struct platform_device *, unsigned int);
extern int platform_get_irq_optional(struct platform_device *, unsigned int);
+extern int platform_get_irq_resource_optional(struct platform_device *dev,
+ unsigned int num,
+ struct resource *r);
extern int platform_irq_count(struct platform_device *);
extern int devm_platform_get_irqs_affinity(struct platform_device *dev,
struct irq_affinity *affd,
--
2.43.0.472.g3155946c3a-goog
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.
Acked-by: Tzung-Bi Shih <[email protected]>
Signed-off-by: Mark Hasemeyer <[email protected]>
---
Changes in v4:
-Rebase on linux-next
-See each patch for patch specific changes
-Add Tzung-Bi's Ack tag
-Drop dev_err() during cros_ec_uart_probe()
-Initalize struct resource on stack
-Update error handling for platform_get_irq_resource_optional()
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 | 40 ++++++++++++++---
drivers/platform/chrome/cros_ec_spi.c | 15 ++++---
drivers/platform/chrome/cros_ec_uart.c | 14 ++++--
include/linux/platform_data/cros_ec_proto.h | 4 +-
5 files changed, 94 insertions(+), 27 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..0204593d7b1c9 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,8 +374,9 @@ 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 r = {};
u8 buf[2] = {};
- int irq, ret;
+ int ret;
/*
* The Framework Laptop (and possibly other non-ChromeOS devices)
@@ -428,12 +450,16 @@ 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)
- ec_dev->irq = irq;
- else if (irq != -ENXIO) {
- dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
- return irq;
+ ret = platform_get_irq_resource_optional(pdev, 0, &r);
+ if (!ret) {
+ ec_dev->irq = r.start;
+ if (should_force_irq_wake_capable())
+ ec_dev->irq_wake = true;
+ else
+ ec_dev->irq_wake = r.flags & IORESOURCE_IRQ_WAKECAPABLE;
+ } else if (ret != -ENXIO) {
+ dev_err(dev, "couldn't retrieve IRQ number (%d)\n", ret);
+ return ret;
}
ret = cros_ec_register(ec_dev);
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..5de346a078745 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,6 +306,7 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);
+ /* Register a new cros_ec device */
return cros_ec_register(ec_dev);
}
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
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 <[email protected]>
> Signed-off-by: Mark Hasemeyer <[email protected]>
> ---
[...]
> 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.
Il 02/01/24 22:07, Mark Hasemeyer ha scritto:
> The cros_ec driver currently assumes that cros-ec-spi compatible device
> nodes are a wakeup-source even though the wakeup-source property is not
> defined.
>
> Some Chromebooks use a separate wake pin, while others overload the
> interrupt for wake and IO. With the current assumption, spurious wakes
> can occur on systems that use a separate wake pin. It is planned to
> update the driver to no longer assume that the EC interrupt pin should
> be enabled for wake.
>
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to signify to the driver that they should still be a valid wakeup
> source.
>
> Signed-off-by: Mark Hasemeyer <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 02/01/24 22:07, Mark Hasemeyer ha scritto:
> The cros_ec driver currently assumes that cros-ec-spi compatible device
> nodes are a wakeup-source even though the wakeup-source property is not
> defined.
>
> Some Chromebooks use a separate wake pin, while others overload the
> interrupt for wake and IO. With the current assumption, spurious wakes
> can occur on systems that use a separate wake pin. It is planned to
> update the driver to no longer assume that the EC interrupt pin should
> be enabled for wake.
>
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to signify to the driver that they should still be a valid wakeup
> source.
>
> Signed-off-by: Mark Hasemeyer <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 02/01/24 22:07, Mark Hasemeyer ha scritto:
> The cros_ec driver currently assumes that cros-ec-spi compatible device
> nodes are a wakeup-source even though the wakeup-source property is not
> defined.
>
> Some Chromebooks use a separate wake pin, while others overload the
> interrupt for wake and IO. With the current assumption, spurious wakes
> can occur on systems that use a separate wake pin. It is planned to
> update the driver to no longer assume that the EC interrupt pin should
> be enabled for wake.
>
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to signify to the driver that they should still be a valid wakeup
> source.
>
> Signed-off-by: Mark Hasemeyer <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 02/01/24 22:07, Mark Hasemeyer ha scritto:
> The cros_ec driver currently assumes that cros-ec-spi compatible device
> nodes are a wakeup-source even though the wakeup-source property is not
> defined.
>
> Some Chromebooks use a separate wake pin, while others overload the
> interrupt for wake and IO. With the current assumption, spurious wakes
> can occur on systems that use a separate wake pin. It is planned to
> update the driver to no longer assume that the EC interrupt pin should
> be enabled for wake.
>
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to signify to the driver that they should still be a valid wakeup
> source.
>
> Signed-off-by: Mark Hasemeyer <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> 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.
The DMI quirk is used to list boards whose IRQ should be enabled for
wake, regardless of what their firmware says. Currently the driver
always enables the EC IRQ for wake anyway, so there shouldn't be a
need to backport the DMI quirk on its own. Splitting out the DMI quirk
into its own patch would break Brya/Brask's ability to wake if they
were to run a kernel w/o the DMI patch. I chose not to split it out to
keep the change atomic, and avoid introducing any regressions.
> > 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.
Correct, this patch doesn't make sense to backport on its own. It is
dependent on the entire patch series (more than just the DTS changes).
I'm not super familiar with how patches flow through different
maintainer trees. But I'd imagine this patch series makes its way to
torvalds/master in some sort of sane manner where earlier patches land
before later (dependent) ones?
> 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.
No, 'wakeup-source' is not required, it provides an indication to the
driver that the IRQ should be used for wake, which then enables the pm
subsystem accordingly. If 'wakup-source' is not used, we're back to
square one of making assumptions. Specifically in this case, it would
be assumed that all SPI based EC's are wake capable.
> 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.
I don't know if I'm convinced this should happen. I'm open to it, but
see my previous comments.
> > +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.
Ack.
>
> > + 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.
Sounds good :-)
> [...]
> > diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> > 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.
Ack.
>
> > };
> >
> > 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 don't know for sure. If so, the EC driver doesn't currently support
it because it assumes wake capability.
> 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.
We could introduce a custom binding? IMHO, using inverted logic like
that kind of goes against the grain with how ACPI and kernel are
currently structured. And I don't love how it would make the SPI EC
driver inverted from the other interfaces. I'd rather go back to just
assuming all SPI based EC's are wake capable. But even then, why
assume? This gives us flexibility to disable wakeirqs and drops
unnecessary assumptions. It also lays the groundwork for new boards
that may behave differently. For example, an ACPI based SPI EC.
Quoting Mark Hasemeyer (2024-01-03 09:47:17)
> > 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.
>
> The DMI quirk is used to list boards whose IRQ should be enabled for
> wake, regardless of what their firmware says. Currently the driver
> always enables the EC IRQ for wake anyway, so there shouldn't be a
> need to backport the DMI quirk on its own. Splitting out the DMI quirk
> into its own patch would break Brya/Brask's ability to wake if they
> were to run a kernel w/o the DMI patch. I chose not to split it out to
> keep the change atomic, and avoid introducing any regressions.
Ok, thanks for clarifying. I understand now that it is to workaround the
firmware on those boards where the driver didn't care before.
>
> > > 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.
>
> Correct, this patch doesn't make sense to backport on its own. It is
> dependent on the entire patch series (more than just the DTS changes).
> I'm not super familiar with how patches flow through different
> maintainer trees. But I'd imagine this patch series makes its way to
> torvalds/master in some sort of sane manner where earlier patches land
> before later (dependent) ones?
The DTS patch would go through the platform maintainer tree and be
pulled into the soc tree and sent up to mainline from there. The
platform/chrome patch would go through chrome platform tree and then to
mainline. The bisection hole will be real.
>
> > 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.
>
> No, 'wakeup-source' is not required, it provides an indication to the
> driver that the IRQ should be used for wake, which then enables the pm
> subsystem accordingly. If 'wakup-source' is not used, we're back to
> square one of making assumptions. Specifically in this case, it would
> be assumed that all SPI based EC's are wake capable.
Is that the wrong assumption to make? My understanding of the DT
property is that it is used to signal that this device should be treated
as a wakeup source, when otherwise it isn't treated as one. In this
case, the device has always been treated as a wakeup source so adding
the property is redundant. Making the patch series dependent on the
property being present makes the driver break without the DT change. We
try to make drivers work with older DT files, sometimes by adding
backwards compatibility code, so the presence of the wakeup-source
property should not be required to make this work.
>
> > 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.
>
> I don't know if I'm convinced this should happen. I'm open to it, but
> see my previous comments.
>
> > > 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 don't know for sure. If so, the EC driver doesn't currently support
> it because it assumes wake capability.
>
> > 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.
>
> We could introduce a custom binding? IMHO, using inverted logic like
> that kind of goes against the grain with how ACPI and kernel are
> currently structured. And I don't love how it would make the SPI EC
> driver inverted from the other interfaces. I'd rather go back to just
> assuming all SPI based EC's are wake capable. But even then, why
> assume? This gives us flexibility to disable wakeirqs and drops
> unnecessary assumptions. It also lays the groundwork for new boards
> that may behave differently. For example, an ACPI based SPI EC.
What is the goal of this patch series? Is it to allow disabling the
wakeup capability of the EC wake irq from userspace? I can see a
possible problem where we want to disable wakeup for the EC dynamically
because either it has no child devices that are wakeup sources (e.g. no
power button, no keyboard on ARM) or userspace has disabled all the
wakeup sources for those child devices at runtime. In that case, we
would want to keep the EC irq from waking up the system from suspend. Is
that what you're doing here?
> The DTS patch would go through the platform maintainer tree and be
> pulled into the soc tree and sent up to mainline from there. The
> platform/chrome patch would go through chrome platform tree and then to
> mainline. The bisection hole will be real.
I thought it would all get merged in the next merge window. How are
bifurcations like this normally dealt with? Does one wait for the
first series of patches to land in mainline before submitting
dependent patches? That could take two merge windows.
> >
> > > 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.
> >
> > No, 'wakeup-source' is not required, it provides an indication to the
> > driver that the IRQ should be used for wake, which then enables the pm
> > subsystem accordingly. If 'wakup-source' is not used, we're back to
> > square one of making assumptions. Specifically in this case, it would
> > be assumed that all SPI based EC's are wake capable.
>
> Is that the wrong assumption to make? My understanding of the DT
> property is that it is used to signal that this device should be treated
> as a wakeup source, when otherwise it isn't treated as one. In this
> case, the device has always been treated as a wakeup source so adding
> the property is redundant.
For SPI, it's not the wrong assumption. I was trying to drop the
assumption though to match ACPI/LPC behavior.
> Making the patch series dependent on the
> property being present makes the driver break without the DT change. We
> try to make drivers work with older DT files, sometimes by adding
> backwards compatibility code, so the presence of the wakeup-source
> property should not be required to make this work.
Do we have use cases where Chromebooks are running older DTBs? I get
the idea in theory, but don't grasp why it's needed here. Regardless,
I can update the SPI code to assume a wake capable IRQ. But I'd like
to keep the prerequisite device tree patches unless there's a good
reason to drop them. Specifying 'wake-source' certainly doesn't hurt
anything, and there are other improvements to the OF
subsystem/documentation.
> What is the goal of this patch series? Is it to allow disabling the
> wakeup capability of the EC wake irq from userspace? I can see a
> possible problem where we want to disable wakeup for the EC dynamically
> because either it has no child devices that are wakeup sources (e.g. no
> power button, no keyboard on ARM) or userspace has disabled all the
> wakeup sources for those child devices at runtime. In that case, we
> would want to keep the EC irq from waking up the system from suspend. Is
> that what you're doing here?
The root of this patch series stems from a bug where spurious wakes
are seen on Skyrim. Copying some wording from the DTS patches:
"Some Chromebooks use a separate wake pin, while others overload the
interrupt for wake and IO. With the current assumption, spurious wakes
can occur on systems that use a separate wake pin. It is planned to
update the driver to no longer assume that the EC interrupt pin should
be enabled for wake."
This patch series will allow us to disable the ec_sync pin as a wake
source on Skyrim as it already uses a dedicated wake gpio.
On Wed, Jan 3, 2024 at 3:25 PM Mark Hasemeyer <[email protected]> wrote:
>
> > The DTS patch would go through the platform maintainer tree and be
> > pulled into the soc tree and sent up to mainline from there. The
> > platform/chrome patch would go through chrome platform tree and then to
> > mainline. The bisection hole will be real.
>
> I thought it would all get merged in the next merge window. How are
> bifurcations like this normally dealt with? Does one wait for the
> first series of patches to land in mainline before submitting
> dependent patches? That could take two merge windows.
The DTS dependency problem must be an exception? How are other
dependency problems resolved? For example, this patch relies on
changes to 'platform_get_irq()' which is in drivers/base/platform.c,
which I imagine is in a different subsystem and gets merged into a
different maintainer's tree.
Quoting Mark Hasemeyer (2024-01-03 14:25:25)
> > The DTS patch would go through the platform maintainer tree and be
> > pulled into the soc tree and sent up to mainline from there. The
> > platform/chrome patch would go through chrome platform tree and then to
> > mainline. The bisection hole will be real.
>
> I thought it would all get merged in the next merge window. How are
> bifurcations like this normally dealt with? Does one wait for the
> first series of patches to land in mainline before submitting
> dependent patches? That could take two merge windows.
It's usually fine to land in the respective maintainer trees because the
driver is written to be compatible with either version of the DT.
>
> > >
> > > > 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.
> > >
> > > No, 'wakeup-source' is not required, it provides an indication to the
> > > driver that the IRQ should be used for wake, which then enables the pm
> > > subsystem accordingly. If 'wakup-source' is not used, we're back to
> > > square one of making assumptions. Specifically in this case, it would
> > > be assumed that all SPI based EC's are wake capable.
> >
> > Is that the wrong assumption to make? My understanding of the DT
> > property is that it is used to signal that this device should be treated
> > as a wakeup source, when otherwise it isn't treated as one. In this
> > case, the device has always been treated as a wakeup source so adding
> > the property is redundant.
>
> For SPI, it's not the wrong assumption. I was trying to drop the
> assumption though to match ACPI/LPC behavior.
Ok. Is the EC always a wakeup source? Not the EC irq, the EC device.
>
> > Making the patch series dependent on the
> > property being present makes the driver break without the DT change. We
> > try to make drivers work with older DT files, sometimes by adding
> > backwards compatibility code, so the presence of the wakeup-source
> > property should not be required to make this work.
>
> Do we have use cases where Chromebooks are running older DTBs? I get
> the idea in theory, but don't grasp why it's needed here.
It's to make the kernel bisectable while the DTB and driver patches are
merged through different trees.
> Regardless,
> I can update the SPI code to assume a wake capable IRQ. But I'd like
> to keep the prerequisite device tree patches unless there's a good
> reason to drop them. Specifying 'wake-source' certainly doesn't hurt
> anything, and there are other improvements to the OF
> subsystem/documentation.
I don't see any harm in having wakeup-source in the binding, but I'd
prefer it is behind a different compatible string as optional, and
introduced when we need to have an EC that doesn't wake up the system.
Otherwise it's all future coding for a device that doesn't exist. It's
also a potential landmine if the driver patch is backported somewhere
without the DTS patch (maybe the DTS is not upstream?). Someone will
have to debug why wakeups aren't working anymore.
>
> > What is the goal of this patch series? Is it to allow disabling the
> > wakeup capability of the EC wake irq from userspace? I can see a
> > possible problem where we want to disable wakeup for the EC dynamically
> > because either it has no child devices that are wakeup sources (e.g. no
> > power button, no keyboard on ARM) or userspace has disabled all the
> > wakeup sources for those child devices at runtime. In that case, we
> > would want to keep the EC irq from waking up the system from suspend. Is
> > that what you're doing here?
>
> The root of this patch series stems from a bug where spurious wakes
> are seen on Skyrim.
Are all 24 patches needed to fix spurious wakeups? Why can't we do a DMI
match table for Skyrim devices and disable the wakeirq logic on that
platform? That would be a much more focused and targeted fix, no?
> Copying some wording from the DTS patches:
> "Some Chromebooks use a separate wake pin, while others overload the
> interrupt for wake and IO. With the current assumption, spurious wakes
> can occur on systems that use a separate wake pin. It is planned to
> update the driver to no longer assume that the EC interrupt pin should
> be enabled for wake."
>
> This patch series will allow us to disable the ec_sync pin as a wake
> source on Skyrim as it already uses a dedicated wake gpio.
Aha! This last sentence is the detail I've been looking for. Please put
these details in the commit text.
"Skyrim devices are experiencing spurious wakeups due to the EC driver
always enabling the irq as a wakeup source but on Skyrim devices the EC
wakeup signal is a dedicated gpio separate from the irq."
Please be direct and specific instead of writing in general terms.
Quoting Mark Hasemeyer (2024-01-03 14:45:06)
> On Wed, Jan 3, 2024 at 3:25 PM Mark Hasemeyer <[email protected]> wrote:
> >
> > > The DTS patch would go through the platform maintainer tree and be
> > > pulled into the soc tree and sent up to mainline from there. The
> > > platform/chrome patch would go through chrome platform tree and then to
> > > mainline. The bisection hole will be real.
> >
> > I thought it would all get merged in the next merge window. How are
> > bifurcations like this normally dealt with? Does one wait for the
> > first series of patches to land in mainline before submitting
> > dependent patches? That could take two merge windows.
>
> The DTS dependency problem must be an exception? How are other
> dependency problems resolved? For example, this patch relies on
> changes to 'platform_get_irq()' which is in drivers/base/platform.c,
> which I imagine is in a different subsystem and gets merged into a
> different maintainer's tree.
Cross tree code dependencies like that are usually resolved by having a
maintainer ack the patch and another maintainer take the code parts.
DT bindings are not supposed to be changed in a way that would break
compatibility with existing code, hence the compatible property. It's a
backwards incompatible change to add the wakeup-source property to the
EC binding and make the driver rely on that property to maintain the
previous code behavior. That's why I keep saying that if you want to add
a wakeup-source property and make the driver act the same as before it
must be done with a new compatible string. Because the driver has always
set the device to wakeup, the compatible has implicitly conveyed that
the wakeup-source property is present.
> > > > > 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.
> > > >
> > > > No, 'wakeup-source' is not required, it provides an indication to the
> > > > driver that the IRQ should be used for wake, which then enables the pm
> > > > subsystem accordingly. If 'wakup-source' is not used, we're back to
> > > > square one of making assumptions. Specifically in this case, it would
> > > > be assumed that all SPI based EC's are wake capable.
> > >
> > > Is that the wrong assumption to make? My understanding of the DT
> > > property is that it is used to signal that this device should be treated
> > > as a wakeup source, when otherwise it isn't treated as one. In this
> > > case, the device has always been treated as a wakeup source so adding
> > > the property is redundant.
> >
> > For SPI, it's not the wrong assumption. I was trying to drop the
> > assumption though to match ACPI/LPC behavior.
>
> Ok. Is the EC always a wakeup source? Not the EC irq, the EC device.
Yes. The powerd daemon enables the EC for wake [1]. At least on ACPI systems.
[1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/81b23aeac510655f27e1d87b99b12c78146e7c37:src/platform2/power_manager/powerd/daemon.cc;l=611
> > Regardless,
> > I can update the SPI code to assume a wake capable IRQ. But I'd like
> > to keep the prerequisite device tree patches unless there's a good
> > reason to drop them. Specifying 'wake-source' certainly doesn't hurt
> > anything, and there are other improvements to the OF
> > subsystem/documentation.
>
> I don't see any harm in having wakeup-source in the binding, but I'd
> prefer it is behind a different compatible string as optional, and
> introduced when we need to have an EC that doesn't wake up the system.
The 'wakeup-source' property is already optional. See patch 04 in this
version of the series which updates the documentation surrounding the
property. Or are you suggesting a completely new string so that we can
be forever backward compatible? If that's the case, the property would
indeed have to have inverted logic signifying a device that is *not*
wakeup capable. It feels wrong to have to do something like that.
> Otherwise it's all future coding for a device that doesn't exist.
I agree adding 'wakeup-source' is future coding for a device that
doesn't exist. It does provide (pseudo) feature parity with ACPI
systems though. See my comment below about ACPI GpioInt/Interrupt
resources.
> It's
> also a potential landmine if the driver patch is backported somewhere
> without the DTS patch (maybe the DTS is not upstream?). Someone will
> have to debug why wakeups aren't working anymore.
I can change the SPI driver so it doesn't look for the 'wakeup-source'
property, keeping existing behavior where wakeirq is assumed. So there
should be no issues with backporting.
> > > What is the goal of this patch series? Is it to allow disabling the
> > > wakeup capability of the EC wake irq from userspace? I can see a
> > > possible problem where we want to disable wakeup for the EC dynamically
> > > because either it has no child devices that are wakeup sources (e.g. no
> > > power button, no keyboard on ARM) or userspace has disabled all the
> > > wakeup sources for those child devices at runtime. In that case, we
> > > would want to keep the EC irq from waking up the system from suspend. Is
> > > that what you're doing here?
> >
> > The root of this patch series stems from a bug where spurious wakes
> > are seen on Skyrim.
>
> Are all 24 patches needed to fix spurious wakeups? Why can't we do a DMI
> match table for Skyrim devices and disable the wakeirq logic on that
> platform? That would be a much more focused and targeted fix, no?
It would be more focused and targeted, but I don't think it's the
correct fix. Skyrim is not the quirk. The driver is incorrectly
enabling the IRQ for wake even though a GPE exists for the EC to wake the AP.
ACPI defines keywords to specify GpioInt and Interrupt
resources as wake capable, and until recently [2], we were not
flagging the respective resources correctly. Most ACPI Chromebooks
have a dedicated GPE for wake, and enabling the IRQ for wake is
unintentional IMHO.
[2] https://review.coreboot.org/c/coreboot/+/79373
> > Copying some wording from the DTS patches:
> > "Some Chromebooks use a separate wake pin, while others overload the
> > interrupt for wake and IO. With the current assumption, spurious wakes
> > can occur on systems that use a separate wake pin. It is planned to
> > update the driver to no longer assume that the EC interrupt pin should
> > be enabled for wake."
> >
> > This patch series will allow us to disable the ec_sync pin as a wake
> > source on Skyrim as it already uses a dedicated wake gpio.
>
> Aha! This last sentence is the detail I've been looking for. Please put
> these details in the commit text.
>
> "Skyrim devices are experiencing spurious wakeups due to the EC driver
> always enabling the irq as a wakeup source but on Skyrim devices the EC
> wakeup signal is a dedicated gpio separate from the irq."
>
> Please be direct and specific instead of writing in general terms.
Sure, I can update the commit text :-)
In summary, there are a lot of comments that suggest different
solutions. Here are the options I see:
1. Skyrim DMI quirk
2a. Update EC SPI driver to assume wake capable regardless of
'wakeup-source' property being present
2b. Remove 'wakeup-source' entries from DTS
3. Leave the existing solution
I'm arguing for 2a (without 2b). This keeps backward compatibility
while adding an indication that the EC is wake capable and keeps
closer feature parity with the ACPI/LPC interface. FWIW, others have
already reviewed/ack'd the dts patches.
On Tue, Jan 02, 2024 at 02:07:27PM -0700, Mark Hasemeyer wrote:
> The i2c_acpi_irq_context structure provides redundant information that
> can be provided with struct resource.
>
> Refactor i2c_acpi_get_irq() to use struct resource instead of struct
> i2c_acpi_irq_context.
...
> ret = acpi_dev_get_resources(adev, &resource_list,
> - i2c_acpi_add_irq_resource, &irq_ctx);
> + i2c_acpi_add_irq_resource, r);
Up to you, but you can make it a single line.
ret = acpi_dev_get_resources(adev, &resource_list, i2c_acpi_add_irq_resource, r);
> if (ret < 0)
> return ret;
...
> +++ b/drivers/i2c/i2c-core-base.c
> + struct resource r = {};
Needs ioport.h.
...
> + irq = i2c_acpi_get_irq(client, &r);
> + if (r.flags & IORESOURCE_IRQ_WAKECAPABLE)
Ditto.
> client->flags |= I2C_CLIENT_WAKE;
> }
...
> +++ b/drivers/i2c/i2c-core.h
> +int i2c_acpi_get_irq(struct i2c_client *client, struct resource *r);
> +static inline int i2c_acpi_get_irq(struct i2c_client *client, struct resource *r)
Needs a forward declaration (besides the inclusion block is a total mess
in this file).
--
With Best Regards,
Andy Shevchenko
On Tue, Jan 02, 2024 at 02:07:26PM -0700, Mark Hasemeyer wrote:
> Other information besides wake capability can be provided about GPIO
> IRQs such as triggering, polarity, and sharability. Use resource flags
> to provide this information to the caller if they want it.
>
> This should keep the API more robust over time as flags are added,
> modified, or removed. It also more closely matches acpi_irq_get() which
> take a resource as an argument.
>
> Rename the function to acpi_dev_get_gpio_irq_resource() to better
> describe the function's new behavior.
Missing blank line.
We put a commit message as
$SUMARY
...blank line...
$DESCRIPTION (can contain blank lines)
...blank line...
$TAG block (may not contain blank lines)
> Signed-off-by: Mark Hasemeyer <[email protected]>
...
> + unsigned long res_flags;
Why not calling it irq_flags?
...
> +struct resource;
This...
> + struct resource r = {};
> + return ret ?: r.start;
...does _not_ cover these cases.
Hence ioport.h must be included. Did I miss it?
--
With Best Regards,
Andy Shevchenko
On Tue, Jan 02, 2024 at 02:07:45PM -0700, Mark Hasemeyer wrote:
> The underlying ACPI and OF subsystems provide their own APIs which
> provide IRQ information as a struct resource. This allows callers to get
> more information about the IRQ by looking at the resource flags. For
> example, whether or not an IRQ is wake capable.
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Tue, Jan 02, 2024 at 02:07:47PM -0700, Mark Hasemeyer wrote:
> Unify handling of ACPI, GPIO, devictree, and platform resource
> interrupts in platform_get_irq_optional(). Each of these subsystems
> provide their own APIs which provide IRQ information as a struct
> resource. This simplifies the logic of the function and allows callers
> to get more information about the IRQ by looking at the resource flags.
> For example, whether or not an IRQ is wake capable.
...
> + ret = fwnode_irq_get_resource(fwnode, num, r);
I still prefer this not to return positive value. Since you _require_ @r to be
not NULL, i.e. valid, the returning positive value makes no sense.
> + ret = ret < 0 ? ret : 0;
> + if (!ret || ret == -EPROBE_DEFER)
> + goto out;
...
> + struct resource *platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
Same comment, please split definition and assignment.
--
With Best Regards,
Andy Shevchenko
Hi Mark,
On Tue, Jan 02, 2024 at 02:07:46PM -0700, Mark Hasemeyer wrote:
> Some of the exported functions use EXPORT_SYMBOL() instead of
> EXPORT_SYMBOL_GPL() and are inconsistent with the other exported
> functions in the module. The underlying APCI/OF struct fwnode_operations
> implementations are also exported via EXPORT_SYMBOL_GPL().
>
> Update them to use the EXPORT_SYMBOL_GPL() macro.
>
> Suggested-by: Sakari Ailus <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Mark Hasemeyer <[email protected]>
Thanks!
Reviewed-by: Sakari Ailus <[email protected]>
--
Regards,
Sakari Ailus
Quoting Mark Hasemeyer (2024-01-03 20:55:41)
> > > > > > 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.
> > > > >
> > > > > No, 'wakeup-source' is not required, it provides an indication to the
> > > > > driver that the IRQ should be used for wake, which then enables the pm
> > > > > subsystem accordingly. If 'wakup-source' is not used, we're back to
> > > > > square one of making assumptions. Specifically in this case, it would
> > > > > be assumed that all SPI based EC's are wake capable.
> > > >
> > > > Is that the wrong assumption to make? My understanding of the DT
> > > > property is that it is used to signal that this device should be treated
> > > > as a wakeup source, when otherwise it isn't treated as one. In this
> > > > case, the device has always been treated as a wakeup source so adding
> > > > the property is redundant.
> > >
> > > For SPI, it's not the wrong assumption. I was trying to drop the
> > > assumption though to match ACPI/LPC behavior.
> >
> > Ok. Is the EC always a wakeup source? Not the EC irq, the EC device.
>
> Yes. The powerd daemon enables the EC for wake [1]. At least on ACPI systems.
> [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/81b23aeac510655f27e1d87b99b12c78146e7c37:src/platform2/power_manager/powerd/daemon.cc;l=611
>
> > > Regardless,
> > > I can update the SPI code to assume a wake capable IRQ. But I'd like
> > > to keep the prerequisite device tree patches unless there's a good
> > > reason to drop them. Specifying 'wake-source' certainly doesn't hurt
> > > anything, and there are other improvements to the OF
> > > subsystem/documentation.
> >
> > I don't see any harm in having wakeup-source in the binding, but I'd
> > prefer it is behind a different compatible string as optional, and
> > introduced when we need to have an EC that doesn't wake up the system.
>
> The 'wakeup-source' property is already optional. See patch 04 in this
> version of the series which updates the documentation surrounding the
> property. Or are you suggesting a completely new string so that we can
> be forever backward compatible? If that's the case, the property would
> indeed have to have inverted logic signifying a device that is *not*
> wakeup capable. It feels wrong to have to do something like that.
Isn't this patch series making the wakeup-source DT property required
for all existing DT nodes? I'm saying that the property is implicit
based on the compatible string "google,cros-ec-{spi,rpmsg,uart}", so
we shouldn't add the property explicitly. Just rely on the compatible
string to convey the property's existence.
>
> > Otherwise it's all future coding for a device that doesn't exist.
>
> I agree adding 'wakeup-source' is future coding for a device that
> doesn't exist. It does provide (pseudo) feature parity with ACPI
> systems though. See my comment below about ACPI GpioInt/Interrupt
> resources.
>
> > It's
> > also a potential landmine if the driver patch is backported somewhere
> > without the DTS patch (maybe the DTS is not upstream?). Someone will
> > have to debug why wakeups aren't working anymore.
>
> I can change the SPI driver so it doesn't look for the 'wakeup-source'
> property, keeping existing behavior where wakeirq is assumed. So there
> should be no issues with backporting.
>
> > > > What is the goal of this patch series? Is it to allow disabling the
> > > > wakeup capability of the EC wake irq from userspace? I can see a
> > > > possible problem where we want to disable wakeup for the EC dynamically
> > > > because either it has no child devices that are wakeup sources (e.g. no
> > > > power button, no keyboard on ARM) or userspace has disabled all the
> > > > wakeup sources for those child devices at runtime. In that case, we
> > > > would want to keep the EC irq from waking up the system from suspend. Is
> > > > that what you're doing here?
> > >
> > > The root of this patch series stems from a bug where spurious wakes
> > > are seen on Skyrim.
> >
> > Are all 24 patches needed to fix spurious wakeups? Why can't we do a DMI
> > match table for Skyrim devices and disable the wakeirq logic on that
> > platform? That would be a much more focused and targeted fix, no?
>
> It would be more focused and targeted, but I don't think it's the
> correct fix. Skyrim is not the quirk. The driver is incorrectly
> enabling the IRQ for wake even though a GPE exists for the EC to wake the AP.
> ACPI defines keywords to specify GpioInt and Interrupt
> resources as wake capable, and until recently [2], we were not
> flagging the respective resources correctly. Most ACPI Chromebooks
> have a dedicated GPE for wake, and enabling the IRQ for wake is
> unintentional IMHO.
I'm no expert in ACPI so sorry if I'm misunderstanding. The driver
unconditionally enables wake on the irq. Most other chromebooks have
added some other interrupt (GPE?) for wakeup purposes, which is
different from the irq used for IO? And this patch series tries to
figure out if enable_irq_wake() is going to fail on those devices so it
can only enable irq wake if the irq supports it? When does calling
enable_irq_wake() not return an error to properly indicate that the irq
can't wake? On skyrim devices, where presumably it needed to be marked
in ACPI differently? Or does that platform really support wake on the
irq, but we also have a GPE so enabling wake on the irq is not failing?
Having to backport 24 patches to fix a bug is not good. Can the driver
look for both an IO interrupt and a GPE and then assume the GPE is for
wakeup and the interrupt is for IO?
> [2] https://review.coreboot.org/c/coreboot/+/79373
>
> > > Copying some wording from the DTS patches:
> > > "Some Chromebooks use a separate wake pin, while others overload the
> > > interrupt for wake and IO. With the current assumption, spurious wakes
> > > can occur on systems that use a separate wake pin. It is planned to
> > > update the driver to no longer assume that the EC interrupt pin should
> > > be enabled for wake."
> > >
> > > This patch series will allow us to disable the ec_sync pin as a wake
> > > source on Skyrim as it already uses a dedicated wake gpio.
> >
> > Aha! This last sentence is the detail I've been looking for. Please put
> > these details in the commit text.
> >
> > "Skyrim devices are experiencing spurious wakeups due to the EC driver
> > always enabling the irq as a wakeup source but on Skyrim devices the EC
> > wakeup signal is a dedicated gpio separate from the irq."
> >
> > Please be direct and specific instead of writing in general terms.
>
> Sure, I can update the commit text :-)
>
> In summary, there are a lot of comments that suggest different
> solutions. Here are the options I see:
> 1. Skyrim DMI quirk
> 2a. Update EC SPI driver to assume wake capable regardless of
> 'wakeup-source' property being present
> 2b. Remove 'wakeup-source' entries from DTS
One disconnect I have is that 'wakeup-source' isn't only talking about
the interrupt in DT. It's indicating that the device itself is a wakeup
source. The interrupt controller hierarchy decides which interrupts are
wakeup capable. It sounds like in ACPI the wakeup capability is encoded
in the interrupt descriptor? DT is different here. As I stated earlier,
the EC device has always been a wakeup source, so having the DT property
there is redundant.
> 3. Leave the existing solution
How is 3 an option? I thought this patch series was fixing a bug.
>
> I'm arguing for 2a (without 2b). This keeps backward compatibility
> while adding an indication that the EC is wake capable and keeps
> closer feature parity with the ACPI/LPC interface. FWIW, others have
> already reviewed/ack'd the dts patches.
> Missing blank line.
> We put a commit message as
>
> $SUMARY
> ...blank line...
> $DESCRIPTION (can contain blank lines)
> ...blank line...
> $TAG block (may not contain blank lines)
>
> > Signed-off-by: Mark Hasemeyer <[email protected]>
Looks like a nuance of patman I need to iron out.
>
> ...
>
> > + unsigned long res_flags;
>
> Why not calling it irq_flags?
irq_flags is already used within the same scope, although it's
declared at the top of the function. I'll move the declaration to the
scope where it's used and rename irq_flags -> irq_type, and irq_res ->
irq_flags.
> > +struct resource;
>
> This...
>
> > + struct resource r = {};
>
> > + return ret ?: r.start;
>
> ...does _not_ cover these cases.
>
> Hence ioport.h must be included. Did I miss it?
You're right. It didn't break the build, which means ioport.h must be
included indirectly. I'll add it back.
On Sat, Jan 6, 2024 at 7:56 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Jan 02, 2024 at 02:07:47PM -0700, Mark Hasemeyer wrote:
> > Unify handling of ACPI, GPIO, devictree, and platform resource
> > interrupts in platform_get_irq_optional(). Each of these subsystems
> > provide their own APIs which provide IRQ information as a struct
> > resource. This simplifies the logic of the function and allows callers
> > to get more information about the IRQ by looking at the resource flags.
> > For example, whether or not an IRQ is wake capable.
>
> ...
>
> > + ret = fwnode_irq_get_resource(fwnode, num, r);
>
> I still prefer this not to return positive value. Since you _require_ @r to be
> not NULL, i.e. valid, the returning positive value makes no sense.
>
> > + ret = ret < 0 ? ret : 0;
> > + if (!ret || ret == -EPROBE_DEFER)
> > + goto out;
I agree. But echoing my response from v3 patch 24:
- The fwnode patch is already reviewed and approved.
- The fwnode patch uses of_irq_to_resource() which already existed and
returns the irq number on success. The error handling translation will
just get pushed to the fwnode subsystem unless of_irq_to_resource() is
also modified which means updating 8 or so drivers that reference it.
I can either:
-Leave it
-Modify the fwnode subsystem to perform the error translation of
of_irq_to_resource()
-Modify the fwnode and OF subsystems and update all driver references
The fwnode and OF patches are already reviewed. I imagine coding
changes would imply dropping any Reviewed-by tags and requesting
another review?
I'd really prefer to not blow up the patch series anymore, but if you
feel strongly, we can come up with a solution.
>
> > + struct resource *platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
>
> Same comment, please split definition and assignment.
Will do.
> Isn't this patch series making the wakeup-source DT property required
> for all existing DT nodes? I'm saying that the property is implicit
> based on the compatible string "google,cros-ec-{spi,rpmsg,uart}", so
> we shouldn't add the property explicitly. Just rely on the compatible
> string to convey the property's existence.
The current wording in 'wakeup-source.txt' states: "Nodes that
describe devices which has wakeup capability must contain a
"wakeup-source" boolean property." According to that wording, the
existing DTS does not match the expectation. This is what led me to
add the property. However, feedback from KML mentioned the wording may
be a little strong and it should be updated. Hence patch 04 in this
series.
I can revert the SPI driver to assume wake capability, which will no
longer make the wakeup-source property required. At that point,
leaving the property in the DTS simply provides an indication. Considering it
won't be required, I can drop the DTS patches that add the property.
> I'm no expert in ACPI so sorry if I'm misunderstanding. The driver
> unconditionally enables wake on the irq.
Yes.
> Most other chromebooks have
> added some other interrupt (GPE?) for wakeup purposes, which is
> different from the irq used for IO?
The GPE is used for wake and IO (It processes ACPI notify alerts).
AFAIK, the separate IRQ was introduced for latency reasons as the GPE
path was too slow.
> And this patch series tries to
> figure out if enable_irq_wake() is going to fail on those devices so it
> can only enable irq wake if the irq supports it? When does calling
> enable_irq_wake() not return an error to properly indicate that the irq
> can't wake? On skyrim devices, where presumably it needed to be marked
> in ACPI differently? Or does that platform really support wake on the
> irq, but we also have a GPE so enabling wake on the irq is not failing?
The patch series does two things:
1. Determines whether the irq should be enabled for wake, as opposed
to assuming (at least for LPC/ACPI).
2. Moves enable_irq_wake() logic to the PM subsystem.
Skyrim does _not_ support wake on irq. It uses a GPE. So the patch
series drops the assumption that irqwake should be enabled. Instead,
it polls the ACPI tables to determine whether or not the IRQ should be
enabled for wake.
> Having to backport 24 patches to fix a bug is not good.
Some of the patches were DTS related as a result of my interpretation
of 'wakeup-source.txt' (see above comment). Other patches are
tangential based on KML feedback to fix things that are orthogonal to
the bug itself.
> Can the driver
> look for both an IO interrupt and a GPE and then assume the GPE is for
> wakeup and the interrupt is for IO?
No, some boards need the IO based irq to wake, and may use both.
> > 3. Leave the existing solution
>
> How is 3 an option? I thought this patch series was fixing a bug.
I meant the solution in the existing patch train.
Quoting Mark Hasemeyer (2024-01-08 11:56:44)
> > Isn't this patch series making the wakeup-source DT property required
> > for all existing DT nodes? I'm saying that the property is implicit
> > based on the compatible string "google,cros-ec-{spi,rpmsg,uart}", so
> > we shouldn't add the property explicitly. Just rely on the compatible
> > string to convey the property's existence.
>
> The current wording in 'wakeup-source.txt' states: "Nodes that
> describe devices which has wakeup capability must contain a
> "wakeup-source" boolean property." According to that wording, the
> existing DTS does not match the expectation. This is what led me to
> add the property. However, feedback from KML mentioned the wording may
> be a little strong and it should be updated. Hence patch 04 in this
> series.
>
> I can revert the SPI driver to assume wake capability, which will no
> longer make the wakeup-source property required. At that point,
> leaving the property in the DTS simply provides an indication. Considering it
> won't be required, I can drop the DTS patches that add the property.
>
> > I'm no expert in ACPI so sorry if I'm misunderstanding. The driver
> > unconditionally enables wake on the irq.
>
> Yes.
>
> > Most other chromebooks have
> > added some other interrupt (GPE?) for wakeup purposes, which is
> > different from the irq used for IO?
>
> The GPE is used for wake and IO (It processes ACPI notify alerts).
> AFAIK, the separate IRQ was introduced for latency reasons as the GPE
> path was too slow.
Alright, I don't know what ACPI notify alerts are so most likely that is
causing me confusion.
>
> > And this patch series tries to
> > figure out if enable_irq_wake() is going to fail on those devices so it
> > can only enable irq wake if the irq supports it? When does calling
> > enable_irq_wake() not return an error to properly indicate that the irq
> > can't wake? On skyrim devices, where presumably it needed to be marked
> > in ACPI differently? Or does that platform really support wake on the
> > irq, but we also have a GPE so enabling wake on the irq is not failing?
>
> The patch series does two things:
> 1. Determines whether the irq should be enabled for wake, as opposed
> to assuming (at least for LPC/ACPI).
> 2. Moves enable_irq_wake() logic to the PM subsystem.
>
> Skyrim does _not_ support wake on irq. It uses a GPE. So the patch
> series drops the assumption that irqwake should be enabled.
Does the call to enable_irq_wake() on skyrim succeed? It seems like the
driver considers failure to enable wake on the irq as the way to figure
out if the irq supports wakeup or not. I'm trying to understand why
anything needs to be changed.
> Instead,
> it polls the ACPI tables to determine whether or not the IRQ should be
> enabled for wake.
>
> > Having to backport 24 patches to fix a bug is not good.
>
> Some of the patches were DTS related as a result of my interpretation
> of 'wakeup-source.txt' (see above comment). Other patches are
> tangential based on KML feedback to fix things that are orthogonal to
> the bug itself.
Fair enough. The fix should be isolated and be early in the series so
that we don't need to backport the whole stack to fix a bug.
>
> > Can the driver
> > look for both an IO interrupt and a GPE and then assume the GPE is for
> > wakeup and the interrupt is for IO?
>
> No, some boards need the IO based irq to wake, and may use both.
Ok.
>
> > > 3. Leave the existing solution
> >
> > How is 3 an option? I thought this patch series was fixing a bug.
>
> I meant the solution in the existing patch train.
Got it.
On Tue, 02 Jan 2024 14:07:28 -0700, Mark Hasemeyer wrote:
> The wording in the current documentation is a little strong. The
> intention was not to fix any particular interrupt as wakeup capable but
> leave those details to the device. It wasn't intended to enforce any
> rules as what can be or can't be a wakeup interrupt.
>
> Soften the wording to not mandate that the 'wakeup-source' property be
> used, and clarify what it means when an interrupt is marked (or not
> marked) for wakeup.
>
> Link: https://lore.kernel.org/all/ZYAjxxHcCOgDVMTQ@bogus/
> Link: https://lore.kernel.org/all/CAL_Jsq+MYwOG40X26cYmO9EkZ9xqWrXDi03MaRfxnV-+VGkXWQ@mail.gmail.com/
> Signed-off-by: Mark Hasemeyer <[email protected]>
> ---
>
> (no changes since v3)
>
> Changes in v3:
> -Update commit title prefixes
>
> Changes in v2:
> -New patch
>
> .../bindings/power/wakeup-source.txt | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
Applied, thanks!
On Tue, 02 Jan 2024 14:07:45 -0700, Mark Hasemeyer wrote:
> The underlying ACPI and OF subsystems provide their own APIs which
> provide IRQ information as a struct resource. This allows callers to get
> more information about the IRQ by looking at the resource flags. For
> example, whether or not an IRQ is wake capable.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Sakari Ailus <[email protected]>
> Signed-off-by: Mark Hasemeyer <[email protected]>
> ---
>
> Changes in v4:
> -Add Sakari's Reviewed-by tag from v2
> -Remove ioport.h dependency in fwnode.h
> -Use Andy's @linux.intel.com email
>
> Changes in v3:
> -Add Suggested-by tag
> -Initialize struct resource to 0 on stack
> -EXPORT_SYMBOL()->EXPORT_SYMBOL_GPL()
> -Remove extra space in commit message
> -Reformat fwnode_irq_get_resource() declaration
>
> Changes in v2:
> -New patch
>
> drivers/acpi/property.c | 11 +++++------
> drivers/base/property.c | 32 +++++++++++++++++++++++++-------
> drivers/of/property.c | 8 ++++----
> include/linux/fwnode.h | 8 +++++---
> include/linux/property.h | 2 ++
> 5 files changed, 41 insertions(+), 20 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>
On Mon, Jan 08, 2024 at 12:09:10PM -0700, Mark Hasemeyer wrote:
> On Sat, Jan 6, 2024 at 7:56 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Tue, Jan 02, 2024 at 02:07:47PM -0700, Mark Hasemeyer wrote:
..
> > > + ret = fwnode_irq_get_resource(fwnode, num, r);
> >
> > I still prefer this not to return positive value. Since you _require_ @r to be
> > not NULL, i.e. valid, the returning positive value makes no sense.
> >
> > > + ret = ret < 0 ? ret : 0;
> > > + if (!ret || ret == -EPROBE_DEFER)
> > > + goto out;
>
> I agree. But echoing my response from v3 patch 24:
> - The fwnode patch is already reviewed and approved.
> - The fwnode patch uses of_irq_to_resource() which already existed and
> returns the irq number on success. The error handling translation will
> just get pushed to the fwnode subsystem unless of_irq_to_resource() is
> also modified which means updating 8 or so drivers that reference it.
>
> I can either:
> -Leave it
> -Modify the fwnode subsystem to perform the error translation of
> of_irq_to_resource()
> -Modify the fwnode and OF subsystems and update all driver references
>
> The fwnode and OF patches are already reviewed. I imagine coding
> changes would imply dropping any Reviewed-by tags and requesting
> another review?
> I'd really prefer to not blow up the patch series anymore, but if you
> feel strongly, we can come up with a solution.
fwnode is quite generic API and I won't fail it from day 1.
Yet we have already some deviations in fwnode/device vs. OF/ACPI cases
(first comes to mind is device_for_each_child_node() which assumes
"availability").
So, I would prefer fwnode API to not inherit issues/features of OF specific
code. Maybe this can be considered as "yes, please update it".
--
With Best Regards,
Andy Shevchenko
On 02/01/2024 22:07, 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.
>
Patch 9, 10, 11 and 12 applied to the mediatek tree.
Thanks!
> Changes in v4:
> -Rebase on linux-next
> -See each patch for patch specific changes
>
> 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 | 90 ++++++++++++-------
> drivers/base/property.c | 40 ++++++---
> drivers/gpio/gpiolib-acpi.c | 28 ++++--
> drivers/i2c/i2c-core-acpi.c | 43 ++++-----
> 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 | 40 +++++++--
> drivers/platform/chrome/cros_ec_spi.c | 15 ++--
> drivers/platform/chrome/cros_ec_uart.c | 14 ++-
> include/linux/acpi.h | 25 +++---
> 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, 350 insertions(+), 172 deletions(-)
>
On Tue, 02 Jan 2024 14:07:24 -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 <[email protected]>