LinuxLists.cc - [PATCH v1 1/6] gpiolib: acpi: Modify acpi_dev_irq_wake_get

[permalink] [raw]

Subject: [PATCH v1 2/6] arm: arm64: dts: Enable cros-ec-spi as wake source

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.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <[email protected]>
---

arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi | 1 +
arch/arm/boot/dts/nvidia/tegra124-venice2.dts | 1 +
arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi | 1 +
arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts | 1 +
arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts | 1 +
arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 1 +
arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 +
arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi | 1 +
arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi | 1 +
arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 1 +
arch/arm64/boot/dts/nvidia/tegra132-norrin.dts | 1 +
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 +
arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 1 +
arch/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 +
16 files changed, 16 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>;

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";
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>;
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>;
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
index 4dd21dd317026..f0395da659a86 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
@@ -1168,6 +1168,7 @@ cros_ec: ec@0 {
interrupt-parent = <&pio>;
interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
google,cros-ec-spi-msg-delay = <500>;
+ wakeup-source;

i2c_tunnel: i2c-tunnel0 {
compatible = "google,cros-ec-i2c-tunnel";
diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
index 44647d462e20b..359859f23b1fd 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
@@ -1013,6 +1013,7 @@ cros_ec: cros-ec@0 {
interrupts = <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";
diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
index 5f62dc83013f0..74c534d475cb0 100644
--- a/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
@@ -1918,6 +1918,7 @@ cros_ec: ec@0 {
interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
pinctrl-names = "default";
pinctrl-0 = <&ec_ap_int>;
+ wakeup-source;

i2c_tunnel: i2c-tunnel {
compatible = "google,cros-ec-i2c-tunnel";
diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
index a29da53d17894..4594287d60926 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
@@ -1454,6 +1454,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>;
diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
index 37a3e9de90ff7..a5ace1b02c3d2 100644
--- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
@@ -1034,6 +1034,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";
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>;

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 5a33e16a8b677..e6a2ed0463997 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -650,6 +650,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-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";
diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
index f86e7acdfd99f..d8eb45662c931 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
@@ -838,6 +838,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";
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index c9bf1d5c3a426..69a0b34f0615b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -602,6 +602,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

2023-12-13 18:02:09

[permalink] [raw]

Subject: [PATCH v1 3/6] of: irq: add wake capable bit to of_irq_resource()

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. No dedicated irq is defined, or 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 feature parity to the
devicetree.

Signed-off-by: Mark Hasemeyer <[email protected]>
---

drivers/of/irq.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 174900072c18c..633711bc32953 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. no dedicated wakeirq exists OR provided irq index is 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
* @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)
{
@@ -411,6 +439,8 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)

r->start = r->end = irq;
r->flags = IORESOURCE_IRQ | irqd_get_trigger_type(irq_get_irq_data(irq));
+ if (__of_irq_wake_capable(dev, index))
+ r->flags |= IORESOURCE_IRQ_WAKECAPABLE;
r->name = name ? name : of_node_full_name(dev);
}

--
2.43.0.472.g3155946c3a-goog

2023-12-13 18:02:11

[permalink] [raw]

Subject: [PATCH v1 4/6] of: irq: Add default implementation for of_irq_to_resource()

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.

Signed-off-by: Mark Hasemeyer <[email protected]>
---

include/linux/of_irq.h | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index d6d3eae2f1452..817c7510082cb 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,8 @@ 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 +76,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

2023-12-13 18:02:27

[permalink] [raw]

Subject: [PATCH v1 5/6] platform: Modify platform_get_irq_optional() to use resource

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.

Rename the function to platform_get_irq_resource() to better describe
the function's new behavior.

Signed-off-by: Mark Hasemeyer <[email protected]>
---

drivers/base/platform.c | 78 ++++++++++++++++++---------------
include/linux/platform_device.h | 9 +++-
2 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 76bfcba250039..6b58bde776d4f 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -151,9 +151,10 @@ 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 - get an IRQ for a device and populate resource struct
* @dev: platform device
* @num: IRQ number index
+ * @r: pointer to resource to populate with irq information. It is not modified on failure.
*
* 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
@@ -162,59 +163,47 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
*
* For example::
*
- * int irq = platform_get_irq_optional(pdev, 0);
+ * int irq = platform_get_irq_resource(pdev, 0, &res);
* 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 platform_get_irq_resource(struct platform_device *dev, unsigned int num, struct resource *r)
{
int ret;
#ifdef CONFIG_SPARC
/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
if (!dev || num >= dev->archdata.num_irqs)
- goto out_not_found;
+ return -ENXIO;
ret = dev->archdata.irqs[num];
+ if (ret >= 0)
+ *r = (struct resource)DEFINE_RES_IRQ(ret);
goto out;
#else
- struct resource *r;
+ struct resource *platform_res;

if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
- ret = of_irq_get(dev->dev.of_node, num);
+ ret = of_irq_to_resource(dev->dev.of_node, num, r);
if (ret > 0 || ret == -EPROBE_DEFER)
goto out;
}

- r = platform_get_resource(dev, IORESOURCE_IRQ, num);
- if (has_acpi_companion(&dev->dev)) {
- if (r && r->flags & IORESOURCE_DISABLED) {
- ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
- if (ret)
- goto out;
- }
- }
-
- /*
- * The resources may pass trigger flags to the irqs that need
- * to be set up. It so happens that the trigger flags for
- * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
- * settings.
- */
- if (r && r->flags & IORESOURCE_BITS) {
- struct irq_data *irqd;
-
- irqd = irq_get_irq_data(r->start);
- if (!irqd)
- goto out_not_found;
- irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
- }
-
- if (r) {
+ platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
+ if (platform_res && !(platform_res->flags & IORESOURCE_DISABLED)) {
+ *r = *platform_res;
ret = r->start;
goto out;
}

+ if (has_acpi_companion(&dev->dev)) {
+ ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
+ if (!ret || ret == -EPROBE_DEFER) {
+ ret = ret ?: r->start;
+ goto out;
+ }
+ }
+
/*
* For the index 0 interrupt, allow falling back to GpioInt
* resources. While a device could have both Interrupt and GpioInt
@@ -223,21 +212,38 @@ 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 && has_acpi_companion(&dev->dev)) {
- ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
+ ret = acpi_dev_get_gpio_irq_resource(ACPI_COMPANION(&dev->dev), NULL,
+ num, r);
/* Our callers expect -ENXIO for missing IRQs. */
- if (ret >= 0 || ret == -EPROBE_DEFER)
+ if (!ret || ret == -EPROBE_DEFER) {
+ ret = ret ?: r->start;
goto out;
+ }
}
-
#endif
-out_not_found:
ret = -ENXIO;
out:
if (WARN(!ret, "0 is an invalid IRQ number\n"))
return -EINVAL;
+
+ /*
+ * The resources may pass trigger flags to the irqs that need
+ * to be set up. It so happens that the trigger flags for
+ * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
+ * settings.
+ */
+ if (ret > 0 && r->flags & IORESOURCE_BITS) {
+ struct irq_data *irqd;
+
+ irqd = irq_get_irq_data(r->start);
+ if (!irqd)
+ ret = -ENXIO;
+ else
+ irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
+ }
return ret;
}
-EXPORT_SYMBOL_GPL(platform_get_irq_optional);
+EXPORT_SYMBOL_GPL(platform_get_irq_resource);

/**
* platform_get_irq - get an IRQ for a device
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 7a41c72c19591..cdaddab4d9241 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -101,7 +101,14 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
#endif

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(struct platform_device *dev, unsigned int num,
+ struct resource *r);
+static inline int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+{
+ struct resource r;
+
+ return platform_get_irq_resource(dev, num, &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

2023-12-13 18:02:37

[permalink] [raw]

Subject: [PATCH v1 6/6] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

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.

The wake capability of the interrupt is provided by the underlying
firmware/device tree. Some Chromebooks rely solely on the ec_sync pin to
wake the AP but they do not specify the interrupt as wake capable in the
ACPI _CRS or device tree. 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 irq wake logic was added on an interface basis (lpc, spi, uart) as
opposed to adding it to cros_ec.c because the i2c subsystem already does
this work on our behalf.

Signed-off-by: Mark Hasemeyer <[email protected]>
---

drivers/platform/chrome/cros_ec.c | 9 ----
drivers/platform/chrome/cros_ec_lpc.c | 51 +++++++++++++++++++--
drivers/platform/chrome/cros_ec_spi.c | 48 +++++++++++++++----
drivers/platform/chrome/cros_ec_uart.c | 34 ++++++++++++--
include/linux/platform_data/cros_ec_proto.h | 2 -
5 files changed, 116 insertions(+), 28 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index badc68bbae8cc..f24d2f2084399 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -353,12 +353,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 +434,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 ed498278a223f..d5beae1b779d3 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -21,6 +21,7 @@
#include <linux/platform_data/cros_ec_commands.h>
#include <linux/platform_data/cros_ec_proto.h>
#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
#include <linux/printk.h>
#include <linux/reboot.h>
#include <linux/suspend.h>
@@ -50,6 +51,27 @@ static struct lpc_driver_ops cros_ec_lpc_ops = { };

static struct platform_device *pdev_extcon;

+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")
+ }
+ }
+}
+MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);
+
+static bool cros_ec_should_force_irq_wake_capable(void)
+{
+ return dmi_first_match(untrusted_fw_irq_wake_capable) != NULL;
+}
+
/*
* A generic instance of the read function of struct lpc_driver_ops, used for
* the LPC EC.
@@ -352,9 +374,11 @@ static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data)
static int cros_ec_lpc_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
+ bool irq_wake = false;
struct acpi_device *adev;
acpi_status status;
struct cros_ec_device *ec_dev;
+ struct resource irqres;
u8 buf[2] = {};
int irq, ret;

@@ -430,20 +454,36 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
* Some boards do not have an IRQ allotted for cros_ec_lpc,
* which makes ENXIO an expected (and safe) scenario.
*/
- irq = platform_get_irq_optional(pdev, 0);
- if (irq > 0)
+ irq = platform_get_irq_resource(pdev, 0, &irqres);
+ if (irq > 0) {
ec_dev->irq = irq;
- else if (irq != -ENXIO) {
+ if (cros_ec_should_force_irq_wake_capable())
+ irq_wake = true;
+ else
+ irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
+ dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", irq, irq_wake);
+ } else if (irq != -ENXIO) {
dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
return irq;
}

ret = cros_ec_register(ec_dev);
if (ret) {
- dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
+ dev_err_probe(dev, ret, "couldn't register ec_dev (%d)\n", ret);
return ret;
}

+ if (irq_wake) {
+ ret = device_init_wakeup(dev, true);
+ if (ret) {
+ dev_err_probe(dev, ret, "Failed to init device for wakeup");
+ return ret;
+ }
+ ret = dev_pm_set_wake_irq(dev, irq);
+ if (ret)
+ return ret;
+ }
+
/*
* Connect a notify handler to process MKBP messages if we have a
* companion ACPI device.
@@ -472,6 +512,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
static int cros_ec_lpc_remove(struct platform_device *pdev)
{
struct cros_ec_device *ec_dev = platform_get_drvdata(pdev);
+ struct device *dev = ec_dev->dev;
struct acpi_device *adev;

platform_device_unregister(pdev_extcon);
@@ -481,6 +522,8 @@ static int cros_ec_lpc_remove(struct platform_device *pdev)
acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
cros_ec_lpc_acpi_notify);

+ dev_pm_clear_wake_irq(dev);
+ device_init_wakeup(dev, false);
cros_ec_unregister(ec_dev);

return 0;
diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 3e88cc92e8192..f99296ab0b8ec 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -7,9 +7,11 @@
#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>
+#include <linux/pm_wakeirq.h>
#include <linux/slab.h>
#include <linux/spi/spi.h>
#include <uapi/linux/sched/types.h>
@@ -70,6 +72,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 +80,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,12 +693,17 @@ 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 spi_device *spi)
{
- struct device_node *np = dev->of_node;
+ struct cros_ec_device *ec_dev = spi_get_drvdata(spi);
+ struct device_node *np = spi->dev.of_node;
+ struct resource irqres;
u32 val;
int ret;

+ if (!np)
+ return;
+
ret = of_property_read_u32(np, "google,cros-ec-spi-pre-delay", &val);
if (!ret)
ec_spi->start_of_msg_delay = val;
@@ -702,6 +711,17 @@ 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;
+
+ if (ec_dev->irq > 0) {
+ ret = of_irq_to_resource(np, 0, &irqres);
+ if (ret != ec_dev->irq) {
+ dev_err(&spi->dev, "irq mismatch detecting wake capability(%d != %d)\n",
+ ret, ec_dev->irq);
+ return;
+ }
+ ec_spi->irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
+ dev_dbg(&spi->dev, "IRQ: %i, wake_capable: %i\n", ec_dev->irq, ec_spi->irq_wake);
+ }
}

static void cros_ec_spi_high_pri_release(void *worker)
@@ -753,9 +773,6 @@ static int cros_ec_spi_probe(struct spi_device *spi)
if (!ec_dev)
return -ENOMEM;

- /* Check for any DT properties */
- cros_ec_spi_dt_probe(ec_spi, dev);
-
spi_set_drvdata(spi, ec_dev);
ec_dev->dev = dev;
ec_dev->priv = ec_spi;
@@ -768,6 +785,9 @@ static int cros_ec_spi_probe(struct spi_device *spi)
sizeof(struct ec_response_get_protocol_info);
ec_dev->dout_size = sizeof(struct ec_host_request);

+ /* Check for any DT properties */
+ cros_ec_spi_dt_probe(ec_spi, spi);
+
ec_spi->last_transfer_ns = ktime_get_ns();

err = cros_ec_spi_devm_high_pri_alloc(dev, ec_spi);
@@ -776,19 +796,31 @@ static int cros_ec_spi_probe(struct spi_device *spi)

err = cros_ec_register(ec_dev);
if (err) {
- dev_err(dev, "cannot register EC\n");
+ dev_err_probe(dev, err, "cannot register EC\n");
return err;
}

- device_init_wakeup(&spi->dev, true);
+ if (ec_spi->irq_wake) {
+ err = device_init_wakeup(dev, true);
+ if (err) {
+ dev_err_probe(dev, err, "Failed to init device for wakeup\n");
+ return err;
+ }
+ err = dev_pm_set_wake_irq(dev, ec_dev->irq);
+ if (err)
+ dev_err_probe(dev, err, "Failed to set irq(%d) for wake\n", ec_dev->irq);
+ }

- return 0;
+ return err;
}

static void cros_ec_spi_remove(struct spi_device *spi)
{
struct cros_ec_device *ec_dev = spi_get_drvdata(spi);
+ struct device *dev = ec_dev->dev;

+ dev_pm_clear_wake_irq(dev);
+ device_init_wakeup(dev, false);
cros_ec_unregister(ec_dev);
}

diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
index 788246559bbba..54255f523d8b7 100644
--- a/drivers/platform/chrome/cros_ec_uart.c
+++ b/drivers/platform/chrome/cros_ec_uart.c
@@ -13,6 +13,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/pm_wakeirq.h>
#include <linux/serdev.h>
#include <linux/slab.h>
#include <uapi/linux/sched/types.h>
@@ -70,6 +71,7 @@ struct response_info {
* @baudrate: UART baudrate of attached EC device.
* @flowcontrol: UART flowcontrol of attached device.
* @irq: Linux IRQ number of associated serial device.
+ * @irq_wake: Whether or not irq assertion should wake the system.
* @response: Response info passing between cros_ec_uart_pkt_xfer()
* and cros_ec_uart_rx_bytes()
*/
@@ -78,6 +80,7 @@ struct cros_ec_uart {
u32 baudrate;
u8 flowcontrol;
u32 irq;
+ bool irq_wake;
struct response_info response;
};

@@ -225,8 +228,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)
@@ -235,12 +240,13 @@ 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;
+ dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", ec_uart->irq, ec_uart->irq_wake);

return 0;
}
@@ -302,13 +308,31 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)

serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);

- return cros_ec_register(ec_dev);
+ /* Register a new cros_ec device */
+ ret = cros_ec_register(ec_dev);
+ if (ret) {
+ dev_err(dev, "Couldn't register ec_dev (%d)\n", ret);
+ return ret;
+ }
+
+ if (ec_uart->irq_wake) {
+ ret = device_init_wakeup(dev, true);
+ if (ret) {
+ dev_err_probe(dev, ret, "Failed to init device for wakeup");
+ return ret;
+ }
+ ret = dev_pm_set_wake_irq(dev, ec_uart->irq);
+ }
+ return ret;
}

static void cros_ec_uart_remove(struct serdev_device *serdev)
{
struct cros_ec_device *ec_dev = serdev_device_get_drvdata(serdev);
+ struct device *dev = ec_dev->dev;

+ dev_pm_clear_wake_irq(dev);
+ device_init_wakeup(dev, false);
cros_ec_unregister(ec_dev);
};

diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 4f9f756bc17ce..a9d952a0f6eda 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -115,7 +115,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
@@ -173,7 +172,6 @@ struct cros_ec_device {
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

2023-12-13 18:08:18

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v1 2/6] arm: arm64: dts: Enable cros-ec-spi as wake source

On 13/12/2023 19:00, Mark Hasemeyer wrote:
> 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.
>
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to match expected behavior.
>
> Signed-off-by: Mark Hasemeyer <[email protected]>
> ---
>

I did not get any other patches in the set, so no clue what's there...
but for this patch: please split per subarch. At least Samsung bits.

Best regards,
Krzysztof

2023-12-13 19:35:14

[permalink] [raw]

Subject: Re: [PATCH v1 1/6] gpiolib: acpi: Modify acpi_dev_irq_wake_get_by to use resource

On Wed, Dec 13, 2023 at 11:00:19AM -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

acpi_irq_get()

> take a resource as an argument.
>
> Rename the function to acpi_dev_get_gpio_irq_resource to better describe

acpi_dev_get_gpio_irq_resource()

> the function's new behavior.

...

> + * @r: pointer to resource to populate with irq information. It is not modified on failure.

IRQ

I don't think the second remark is even needed. It's usual approach, i.e.
we expect no changes in the output if error condition is met.

...

> + * Irq number will be available in the resource structure.

IRQ

...

> + *r = (struct resource)DEFINE_RES_IRQ(irq);

Why do you need "(struct resource)" annotation?

...

> + struct resource irqres;
> struct i2c_acpi_irq_context irq_ctx = {
> .irq = -ENOENT,
> };

Hmm... I'm wondering if we can reuse irqres as a context to the respective
lookup calls.

--
With Best Regards,
Andy Shevchenko

2023-12-13 19:41:05

[permalink] [raw]

Subject: Re: [PATCH v1 1/6] gpiolib: acpi: Modify acpi_dev_irq_wake_get_by to use resource

2023-12-13 19:44:39

[permalink] [raw]

Subject: Re: [PATCH v1 3/6] of: irq: add wake capable bit to of_irq_resource()

On Wed, Dec 13, 2023 at 11:00:21AM -0700, Mark Hasemeyer wrote:
> Add wake capability information to the irq resource. Wake capability is

IRQ

> 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. No dedicated irq is defined, or the irq is marked as dedicated by

IRQ

> 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

API

> willing to conform to the above conventions.
>
> The ACPI subsystems already provides similar apis that allow one to

APIs

> query the wake capability of an irq. This brings feature parity to the
> devicetree.

...

> +/**
> + * __of_irq_wake_capable - Determine whether a given irq index is wake capable

IRQ

> + * The irq is considered wake capable if the following are true:

IRQ

> + * 1. wakeup-source property exists
> + * 2. no dedicated wakeirq exists OR provided irq index is a dedicated wakeirq

IRQ

> + * This logic assumes the provided irq index is valid.

IRQ

> + * @dev: pointer to device tree node
> + * @index: zero-based index of the irq

IRQ

> + * Return: True if provided irq index for #dev is wake capable. False otherwise.

IRQ
@dev

> + */

...

> /**
> * 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
> * @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.
> */

You see, your new text is even inconsistent with the existing one...

--
With Best Regards,
Andy Shevchenko

2023-12-13 19:46:22

[permalink] [raw]

Subject: Re: [PATCH v1 4/6] of: irq: Add default implementation for of_irq_to_resource()

On Wed, Dec 13, 2023 at 11:00:22AM -0700, Mark Hasemeyer wrote:
> 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.

...

> +extern int of_irq_to_resource(struct device_node *dev, int index,
> + struct resource *r);

It looks like you may put this on a single line as this file has longer lines.
Note you do not need "extern" for function. You may even update the header
to drop all of them at once.

--
With Best Regards,
Andy Shevchenko

2023-12-13 19:53:27

[permalink] [raw]

Subject: Re: [PATCH v1 5/6] platform: Modify platform_get_irq_optional() to use resource

2023-12-13 22:04:55

[permalink] [raw]

Subject: Re: [PATCH v1 5/6] platform: Modify platform_get_irq_optional() to use resource

On Wed, Dec 13, 2023 at 11:00:23AM -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.
>
> Rename the function to platform_get_irq_resource() to better describe
> the function's new behavior.

This is misleading as the original function is still there.

The get_optional() functions are designed to not print an error message
where as the non-optional variant will. You've broken that pattern here
in that there is no platform_get_irq_resource_optional() (at least named
that because your implementation is that since there is no error
message).

What about versions equivalent to platform_get_irq_byname()
and platform_get_irq_byname_optional(), though I guess we need users
first.

>
> Signed-off-by: Mark Hasemeyer <[email protected]>
> ---
>
> drivers/base/platform.c | 78 ++++++++++++++++++---------------
> include/linux/platform_device.h | 9 +++-
> 2 files changed, 50 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 76bfcba250039..6b58bde776d4f 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -151,9 +151,10 @@ 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 - get an IRQ for a device and populate resource struct
> * @dev: platform device
> * @num: IRQ number index
> + * @r: pointer to resource to populate with irq information. It is not modified on failure.
> *
> * 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
> @@ -162,59 +163,47 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
> *
> * For example::
> *
> - * int irq = platform_get_irq_optional(pdev, 0);
> + * int irq = platform_get_irq_resource(pdev, 0, &res);
> * 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 platform_get_irq_resource(struct platform_device *dev, unsigned int num, struct resource *r)
> {
> int ret;
> #ifdef CONFIG_SPARC
> /* sparc does not have irqs represented as IORESOURCE_IRQ resources */
> if (!dev || num >= dev->archdata.num_irqs)
> - goto out_not_found;
> + return -ENXIO;
> ret = dev->archdata.irqs[num];
> + if (ret >= 0)
> + *r = (struct resource)DEFINE_RES_IRQ(ret);
> goto out;
> #else
> - struct resource *r;
> + struct resource *platform_res;
>
> if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
> - ret = of_irq_get(dev->dev.of_node, num);
> + ret = of_irq_to_resource(dev->dev.of_node, num, r);
> if (ret > 0 || ret == -EPROBE_DEFER)
> goto out;
> }
>
> - r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> - if (has_acpi_companion(&dev->dev)) {
> - if (r && r->flags & IORESOURCE_DISABLED) {
> - ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> - if (ret)
> - goto out;
> - }
> - }
> -
> - /*
> - * The resources may pass trigger flags to the irqs that need
> - * to be set up. It so happens that the trigger flags for
> - * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
> - * settings.
> - */
> - if (r && r->flags & IORESOURCE_BITS) {
> - struct irq_data *irqd;
> -
> - irqd = irq_get_irq_data(r->start);
> - if (!irqd)
> - goto out_not_found;
> - irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> - }
> -
> - if (r) {
> + platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
> + if (platform_res && !(platform_res->flags & IORESOURCE_DISABLED)) {
> + *r = *platform_res;
> ret = r->start;
> goto out;
> }
>
> + if (has_acpi_companion(&dev->dev)) {
> + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> + if (!ret || ret == -EPROBE_DEFER) {
> + ret = ret ?: r->start;
> + goto out;
> + }
> + }
> +
> /*
> * For the index 0 interrupt, allow falling back to GpioInt
> * resources. While a device could have both Interrupt and GpioInt
> @@ -223,21 +212,38 @@ 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 && has_acpi_companion(&dev->dev)) {
> - ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> + ret = acpi_dev_get_gpio_irq_resource(ACPI_COMPANION(&dev->dev), NULL,
> + num, r);
> /* Our callers expect -ENXIO for missing IRQs. */
> - if (ret >= 0 || ret == -EPROBE_DEFER)
> + if (!ret || ret == -EPROBE_DEFER) {
> + ret = ret ?: r->start;
> goto out;
> + }
> }
> -
> #endif
> -out_not_found:
> ret = -ENXIO;
> out:
> if (WARN(!ret, "0 is an invalid IRQ number\n"))
> return -EINVAL;
> +
> + /*
> + * The resources may pass trigger flags to the irqs that need
> + * to be set up. It so happens that the trigger flags for
> + * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
> + * settings.
> + */
> + if (ret > 0 && r->flags & IORESOURCE_BITS) {
> + struct irq_data *irqd;
> +
> + irqd = irq_get_irq_data(r->start);
> + if (!irqd)
> + ret = -ENXIO;
> + else
> + irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);

We were not doing any of this in the DT or Sparc cases before. It's
probably just redundant for DT. It might break Sparc.

Rob

2023-12-13 22:11:40

[permalink] [raw]

Subject: Re: [PATCH v1 2/6] arm: arm64: dts: Enable cros-ec-spi as wake source

On Wed, Dec 13, 2023 at 11:00:20AM -0700, Mark Hasemeyer wrote:
> 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.

If a device knows it is wakeup capable, why do you need a property too?
I haven't looked closely enough, but it smells like after patch 6, these
properties would be required for wakeup? That would be an ABI break.

Rob

2023-12-13 22:20:36

[permalink] [raw]

Subject: Re: [PATCH v1 3/6] of: irq: add wake capable bit to of_irq_resource()

On Wed, Dec 13, 2023 at 11:00:21AM -0700, Mark Hasemeyer wrote:
> 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. No dedicated irq is defined, or 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 feature parity to the
> devicetree.
>
> Signed-off-by: Mark Hasemeyer <[email protected]>
> ---
>
> drivers/of/irq.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 174900072c18c..633711bc32953 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. no dedicated wakeirq exists OR provided irq index is 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;

If a device has multiple interrupts, but none named "wakeup" you are
saying all the interrupts are wakeup capable. That's not right though.
Only the device knows which interrupts are wakeup capable. You need:

return wakeindex >= 0 && wakeindex == index;

Rob

2023-12-14 03:10:00

by Tzung-Bi Shih

[permalink] [raw]

Subject: Re: [PATCH v1 6/6] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

On Wed, Dec 13, 2023 at 11:00:24AM -0700, Mark Hasemeyer wrote:
> The cros ec driver is manually managing the wake irq by calling
> enable_irq_wake/disable_irq_wake during suspend/resume.
>
> Modify the driver to use the power management subsystem to manage the
> wakeirq.
>
> The wake capability of the interrupt is provided by the underlying
> firmware/device tree. Some Chromebooks rely solely on the ec_sync pin to
> wake the AP but they do not specify the interrupt as wake capable in the
> ACPI _CRS or device tree. 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 irq wake logic was added on an interface basis (lpc, spi, uart) as
> opposed to adding it to cros_ec.c because the i2c subsystem already does
> this work on our behalf.

It looks to me the patch combines multiple changes. Please separate them into
smaller pieces for reviewing easier.

Another confusing about the patch subject: "6/6"? Is it a series? I don't
see the cover letter.

2023-12-14 10:57:27

by AngeloGioacchino Del Regno

[permalink] [raw]

Subject: Re: [PATCH v1 2/6] arm: arm64: dts: Enable cros-ec-spi as wake source

Il 13/12/23 19:00, 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.
>
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to match expected behavior.
>
> Signed-off-by: Mark Hasemeyer <[email protected]>

I received only patch [2/6] - please send the entire series to the relevant
maintainers, as otherwise it's difficult to understand what's going on.

As for this patch alone:
1. arch/arm stuff goes to a different commit
2. I would prefer if you split per-arch and per-SoC.

Regards,
Angelo

2023-12-14 11:53:47

by Konrad Dybcio

[permalink] [raw]

Subject: Re: [PATCH v1 2/6] arm: arm64: dts: Enable cros-ec-spi as wake source

On 12/14/23 11:55, AngeloGioacchino Del Regno wrote:
> Il 13/12/23 19:00, 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.
>>
>> Add the wakeup-source property to all cros-ec-spi compatible device
>> nodes to match expected behavior.
>>
>> Signed-off-by: Mark Hasemeyer <[email protected]>
>
> I received only patch [2/6] - please send the entire series to the relevant
> maintainers, as otherwise it's difficult to understand what's going on.
>
> As for this patch alone:
> 1. arch/arm stuff goes to a different commit
> 2. I would prefer if you split per-arch and per-SoC.
+1, otherwise *somebody* will get merge conflicts that - even
if trivial - take additional time to resolve :(

Konrad

2023-12-14 20:57:47

[permalink] [raw]

Subject: Re: [PATCH v1 1/6] gpiolib: acpi: Modify acpi_dev_irq_wake_get_by to use resource

> > + *r = (struct resource)DEFINE_RES_IRQ(irq);
>
> Why do you need "(struct resource)" annotation?

I don't. I originally started working on this patch on a kernel
version that didn't have 52c4d11f1dce ("resource: Convert
DEFINE_RES_NAMED() to be compound literal").

> > + struct resource irqres;
> > struct i2c_acpi_irq_context irq_ctx = {
> > .irq = -ENOENT,
> > };
>
> Hmm... I'm wondering if we can reuse irqres as a context to the respective
> lookup calls.

I'll see if I can safely remove it.

2023-12-14 21:05:33

[permalink] [raw]

Subject: Re: [PATCH v1 2/6] arm: arm64: dts: Enable cros-ec-spi as wake source

> If a device knows it is wakeup capable, why do you need a property too?

I'm referencing:
https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
"Nodes that describe devices which has wakeup capability must contain
an "wakeup-source" boolean property."

Currently the driver assumes the device is wake capable without
parsing the device tree, which is an incorrect assumption as wake
capability should not be enabled on some cros_ec systems.

> I haven't looked closely enough, but it smells like after patch 6, these
> properties would be required for wakeup? That would be an ABI break.

Agreed. In this case, the driver is a ChromeOS related driver and DTS
is built from source for each OS update.
For more context, I will make sure to CC you (and everyone else) and
include a cover letter in the next series version.

2023-12-14 21:06:01

[permalink] [raw]

Subject: Re: [PATCH v1 3/6] of: irq: add wake capable bit to of_irq_resource()

> If a device has multiple interrupts, but none named "wakeup" you are
> saying all the interrupts are wakeup capable. That's not right though.
> Only the device knows which interrupts are wakeup capable. You need:
>
> return wakeindex >= 0 && wakeindex == index;

I was assuming logic described in the DT bindings:
https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
"Also, if device is marked as a wakeup source, then all the primary
interrupt(s) can be used as wakeup interrupt(s)."

2023-12-14 22:20:56

[permalink] [raw]

Subject: Re: [PATCH v1 2/6] arm: arm64: dts: Enable cros-ec-spi as wake source

On Thu, Dec 14, 2023 at 3:04 PM Mark Hasemeyer <[email protected]> wrote:
>
> > If a device knows it is wakeup capable, why do you need a property too?
>
> I'm referencing:
> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
> "Nodes that describe devices which has wakeup capability must contain
> an "wakeup-source" boolean property."

That's probably too strongly worded because wakeup capable devices
existed (and still exist) before this binding was created. Powerpc for
example doesn't use it.

> Currently the driver assumes the device is wake capable without
> parsing the device tree, which is an incorrect assumption as wake
> capability should not be enabled on some cros_ec systems.
>
> > I haven't looked closely enough, but it smells like after patch 6, these
> > properties would be required for wakeup? That would be an ABI break.
>
> Agreed. In this case, the driver is a ChromeOS related driver and DTS
> is built from source for each OS update.
> For more context, I will make sure to CC you (and everyone else) and
> include a cover letter in the next series version.

Please explain in the patches with an ABI break why it doesn't matter.

Rob

2023-12-15 15:30:48

[permalink] [raw]

Subject: Re: [PATCH v1 3/6] of: irq: add wake capable bit to of_irq_resource()

On Thu, Dec 14, 2023 at 02:05:16PM -0700, Mark Hasemeyer wrote:
> > If a device has multiple interrupts, but none named "wakeup" you are
> > saying all the interrupts are wakeup capable. That's not right though.
> > Only the device knows which interrupts are wakeup capable. You need:
> >
> > return wakeindex >= 0 && wakeindex == index;
>
> I was assuming logic described in the DT bindings:
> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
> "Also, if device is marked as a wakeup source, then all the primary
> interrupt(s) can be used as wakeup interrupt(s)."

Also not the best wording I think.

Which interrupts are primary interrupts?

If we can't determine which interrupt, then we should just leave it up
to the device.

Rob

2023-12-15 20:57:10

[permalink] [raw]

Subject: Re: [PATCH v1 3/6] of: irq: add wake capable bit to of_irq_resource()

On Fri, Dec 15, 2023 at 8:30 AM Rob Herring <[email protected]> wrote:
>
> On Thu, Dec 14, 2023 at 02:05:16PM -0700, Mark Hasemeyer wrote:
> > > If a device has multiple interrupts, but none named "wakeup" you are
> > > saying all the interrupts are wakeup capable. That's not right though.
> > > Only the device knows which interrupts are wakeup capable. You need:
> > >
> > > return wakeindex >= 0 && wakeindex == index;
> >
> > I was assuming logic described in the DT bindings:
> > https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
> > "Also, if device is marked as a wakeup source, then all the primary
> > interrupt(s) can be used as wakeup interrupt(s)."
>
> Also not the best wording I think.
>
> Which interrupts are primary interrupts?
>
> If we can't determine which interrupt, then we should just leave it up
> to the device.
>
> Rob

+Sudeep who authored the documentation and Rob Ack'd: a68eee4c748c
("Documentation: devicetree: standardize/consolidate on "wakeup-source"
property")

I think what Rob is suggesting more closely matches what ACPI supports: where
interrupt resources are individually marked as wake capable. The binding
documentation should be updated though.

Something like:
```
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 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.
```

Parts of the kernel (I2C, bluetooth, MMC) assume "wakeup" as the
interrupt-name. I added some wording to clarify the assumption.

Thoughts?

2023-12-15 21:02:25

[permalink] [raw]

Subject: Re: [PATCH v1 6/6] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

> It looks to me the patch combines multiple changes. Please separate them into
> smaller pieces for reviewing easier.

Unfortunately, I don't see an easy way to break up this commit without
breaking EC wake functionality along the way.

> Another confusing about the patch subject: "6/6"? Is it a series? I don't
> see the cover letter.

Sorry about that! I'll make sure to add a cover letter and CC you on
the next series version.

2023-12-18 10:49:56

by Sudeep Holla

[permalink] [raw]

Subject: Re: [PATCH v1 3/6] of: irq: add wake capable bit to of_irq_resource()

On Fri, Dec 15, 2023 at 01:56:47PM -0700, Mark Hasemeyer wrote:
> On Fri, Dec 15, 2023 at 8:30 AM Rob Herring <[email protected]> wrote:
> >
> > On Thu, Dec 14, 2023 at 02:05:16PM -0700, Mark Hasemeyer wrote:
> > > > If a device has multiple interrupts, but none named "wakeup" you are
> > > > saying all the interrupts are wakeup capable. That's not right though.
> > > > Only the device knows which interrupts are wakeup capable. You need:
> > > >

Agreed.

> > > > return wakeindex >= 0 && wakeindex == index;
> > >
> > > I was assuming logic described in the DT bindings:
> > > https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
> > > "Also, if device is marked as a wakeup source, then all the primary
> > > interrupt(s) can be used as wakeup interrupt(s)."

At the time it was written, the intention was not to fix any particular
interrupt as wakeup but leave those details to the device. Also this was
just to consolidate various variation of similar bindings/support for the
same wake feature. It didn't enforce any rules as what can be or can't be
wakeup interrupt.

> >
> > Also not the best wording I think.
> >
> > Which interrupts are primary interrupts?
> >
> > If we can't determine which interrupt, then we should just leave it up
> > to the device.
> >

Again I agree with this.

> +Sudeep who authored the documentation and Rob Ack'd: a68eee4c748c
> ("Documentation: devicetree: standardize/consolidate on "wakeup-source"
> property")
>
> I think what Rob is suggesting more closely matches what ACPI supports: where
> interrupt resources are individually marked as wake capable. The binding
> documentation should be updated though.
>
> Something like:
> ```
> 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 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.
> ```
>
> Parts of the kernel (I2C, bluetooth, MMC) assume "wakeup" as the
> interrupt-name. I added some wording to clarify the assumption.
>
> Thoughts?

Above wordings sounds good to me.

--
Regards,
Sudeep

2023-12-18 20:23:57

[permalink] [raw]

Subject: Re: [PATCH v1 5/6] platform: Modify platform_get_irq_optional() to use resource

> > + *r = (struct resource)DEFINE_RES_IRQ(ret);
>
> Why is the annotation needed?

It's not needed anymore. I'll remove it.

> > - struct resource *r;
> > + struct resource *platform_res;
> >
> > if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
> > - ret = of_irq_get(dev->dev.of_node, num);
> > + ret = of_irq_to_resource(dev->dev.of_node, num, r);
> > if (ret > 0 || ret == -EPROBE_DEFER)
> > goto out;
> > }
>
>
> > + if (has_acpi_companion(&dev->dev)) {
> > + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> > + if (!ret || ret == -EPROBE_DEFER) {
> > + ret = ret ?: r->start;
> > + goto out;
> > + }
> > + }
>
> Consider combine the above to use fwnode_irq_get() in the separate prerequisite
> change.

I like the idea. It doesn't look like 'struct fwnode_operations'
provides a way to retrieve information in a 'struct resource' format.
Perhaps this could be followed up on in a separate patch train?

>
> > + irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
>
> NIH resource_type().
>
> > + }
> > return ret;

Does NIH mean "not invented here"? resource_type() masks
IORESOURCE_TYPE_BITS, not IORESOURCE_BITS. I'm not quite sure what you
mean here.

2023-12-19 15:11:15