| Message ID | 20231213110009.v1.3.I29b26a7f3b80fac0a618707446a10b6cc974fdaf@changeid |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | None | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
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...
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
> 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)."
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
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?
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
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); }
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(+)