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

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

[Mark Hasemeyer]

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 <markhas@chromium.org>
---

 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

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

[Mark Hasemeyer]

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 <markhas@chromium.org>
---

 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

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

[Mark Hasemeyer]

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 <markhas@chromium.org>
---

 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

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

[Krzysztof Kozlowski]

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 <markhas@chromium.org>
> ---
> 

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

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

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

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

[Rob Herring]

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

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

[Rob Herring]

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 <markhas@chromium.org>
> ---
> 
>  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

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

[AngeloGioacchino Del Regno]

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 <markhas@chromium.org>

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

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

[Konrad Dybcio]

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 <markhas@chromium.org>
> 
> 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

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

[Mark Hasemeyer]

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

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

[Mark Hasemeyer]

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

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

[Rob Herring]

On Thu, Dec 14, 2023 at 3:04 PM Mark Hasemeyer <markhas@chromium.org> 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

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

[Rob Herring]

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

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

[Mark Hasemeyer]

On Fri, Dec 15, 2023 at 8:30 AM Rob Herring <robh@kernel.org> 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?

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

[Sudeep Holla]

On Fri, Dec 15, 2023 at 01:56:47PM -0700, Mark Hasemeyer wrote:
> On Fri, Dec 15, 2023 at 8:30 AM Rob Herring <robh@kernel.org> 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