[PATCH 0/2] spi: add wakeup handling to SPI core

[PATCH 0/2] spi: add wakeup handling to SPI core

[Dmitry Torokhov]

These patches add wakeup handling for SPI devices to SPI core, mirroring
I2C subsystem. Users can mark devices as "wakeup-source" in device tree
(or using static device properties) and SPI core will mark the primary
interrupt line as wakeirq and mark the device as wakeup enabled.

There is also an option to define a dedicated wake irq.


Dmitry Torokhov (2):
  spi: dt-bindings: spi-controller: add wakeup-source and interrupts
  spi: wire up wakeup-source/wakeirq handling

 .../bindings/spi/spi-controller.yaml          | 23 ++++++++++++++++
 drivers/spi/spi.c                             | 27 ++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Dmitry Torokhov]

This change adds bindings for wakeup-source and
interrupts/interrupt-names properties that can be used to configure
device as wakeup source on the system.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

---

 .../bindings/spi/spi-controller.yaml          | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 732339275848c..4e31a4bf6851c 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -78,6 +78,29 @@ patternProperties:
         description:
           Chip select used by the device.
 
+      interrupts:
+        items:
+          - description: main interrupt (attention) line.
+          - description: dedicated wakeup interrupt.
+        minItems: 1 # The wakeup interrupt is optional.
+        description:
+          Specifies interrupt lines a device is connected to. Typically a
+          device is wired to a single interrupt line that is used as
+          "attention" signal and also to wake up system when device is
+          set up as wakeup source. However on some systems a dedicated
+          wakeup line might be used.
+
+      interrupt-names:
+        items:
+          - const: irq
+          - const: wakeup
+        minItems: 1
+
+      wakeup-source:
+        description:
+          Device is capable of waking up the system.
+        type: boolean
+
       spi-3wire:
         $ref: /schemas/types.yaml#/definitions/flag
         description:
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH 2/2] spi: wire up wakeup-source/wakeirq handling

[Dmitry Torokhov]

Many SPI drivers need to configure their devices for waking up the
system. Instead of forcing them to reimplement wakeup handling, let's
mirror what I2C core is doing and handle "wakeup-source" device property
in SPI core.

Also, let's allow naming device's interrupt lines as "irq" and "wakeup"
and automatically configure the right one as wakeirq when device is
supposed to be a wakeup source.

Implementing this is very helpful for drivers that use both I2C and SPI
for transport, such as tsc2004/tsc2005.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

---

 drivers/spi/spi.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 50769a8475d66..baa7aac300d9f 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -22,6 +22,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/property.h>
 #include <linux/export.h>
 #include <linux/sched/rt.h>
@@ -394,13 +395,37 @@ static int spi_drv_probe(struct device *dev)
 		return ret;
 
 	if (dev->of_node) {
-		spi->irq = of_irq_get(dev->of_node, 0);
+		spi->irq = of_irq_get_byname(dev->of_node, "irq");
+		if (spi->irq == -EINVAL || spi->irq == -ENODATA)
+			spi->irq = of_irq_get(dev->of_node, 0);
 		if (spi->irq == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
 		if (spi->irq < 0)
 			spi->irq = 0;
 	}
 
+	if (device_property_read_bool(dev, "wakeup-source")) {
+		int wakeirq = -ENOENT;
+
+		if (dev->of_node) {
+			wakeirq = of_irq_get_byname(dev->of_node, "wakeup");
+			if (wakeirq == -EPROBE_DEFER)
+				return wakeirq;
+		}
+
+		device_init_wakeup(dev, true);
+
+		if (wakeirq > 0 && wakeirq != spi->irq)
+			ret = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
+		else if (spi->irq > 0)
+			ret = dev_pm_set_wake_irq(dev, spi->irq);
+		else
+			ret = 0;
+
+		if (ret)
+			dev_warn(dev, "failed to set up wakeup irq: %d\n", ret);
+	}
+
 	ret = dev_pm_domain_attach(dev, true);
 	if (ret)
 		return ret;
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Mark Brown]

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

On Mon, Nov 11, 2019 at 09:54:10PM -0800, Dmitry Torokhov wrote:

> +      interrupts:
> +        items:
> +          - description: main interrupt (attention) line.
> +          - description: dedicated wakeup interrupt.
> +        minItems: 1 # The wakeup interrupt is optional.
> +        description:
> +          Specifies interrupt lines a device is connected to. Typically a
> +          device is wired to a single interrupt line that is used as
> +          "attention" signal and also to wake up system when device is
> +          set up as wakeup source. However on some systems a dedicated
> +          wakeup line might be used.

> +      interrupt-names:
> +        items:
> +          - const: irq
> +          - const: wakeup
> +        minItems: 1

How will this interact with a SPI device that defines interrupts at the
device level, possibly more than one of them?  Especially if the device
has its own idea what the interrupts should be called.

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

Re: [PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Dmitry Torokhov]

On Tue, Nov 12, 2019 at 12:03:07PM +0000, Mark Brown wrote:
> On Mon, Nov 11, 2019 at 09:54:10PM -0800, Dmitry Torokhov wrote:
> 
> > +      interrupts:
> > +        items:
> > +          - description: main interrupt (attention) line.
> > +          - description: dedicated wakeup interrupt.
> > +        minItems: 1 # The wakeup interrupt is optional.
> > +        description:
> > +          Specifies interrupt lines a device is connected to. Typically a
> > +          device is wired to a single interrupt line that is used as
> > +          "attention" signal and also to wake up system when device is
> > +          set up as wakeup source. However on some systems a dedicated
> > +          wakeup line might be used.
> 
> > +      interrupt-names:
> > +        items:
> > +          - const: irq
> > +          - const: wakeup
> > +        minItems: 1
> 
> How will this interact with a SPI device that defines interrupts at the
> device level, possibly more than one of them?  Especially if the device
> has its own idea what the interrupts should be called.

My understanding that individual drivers should be able to override
whatever the default behavior core has configured, and the device can
establish their own mapping. We have this in I2C and I believe this
works well.

Is the concern about the device tree scheme or SPI core handling?

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Mark Brown]

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

On Tue, Nov 12, 2019 at 11:03:28AM -0800, Dmitry Torokhov wrote:
> On Tue, Nov 12, 2019 at 12:03:07PM +0000, Mark Brown wrote:
> > On Mon, Nov 11, 2019 at 09:54:10PM -0800, Dmitry Torokhov wrote:

> > > +      interrupts:
> > > +        items:
> > > +          - description: main interrupt (attention) line.
> > > +          - description: dedicated wakeup interrupt.
> > > +        minItems: 1 # The wakeup interrupt is optional.

> > > +      interrupt-names:
> > > +        items:
> > > +          - const: irq
> > > +          - const: wakeup
> > > +        minItems: 1

> > How will this interact with a SPI device that defines interrupts at the
> > device level, possibly more than one of them?  Especially if the device
> > has its own idea what the interrupts should be called.

> My understanding that individual drivers should be able to override
> whatever the default behavior core has configured, and the device can
> establish their own mapping. We have this in I2C and I believe this
> works well.

> Is the concern about the device tree scheme or SPI core handling?

Both really.

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

Re: [PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Dmitry Torokhov]

On Tue, Nov 12, 2019 at 07:15:47PM +0000, Mark Brown wrote:
> On Tue, Nov 12, 2019 at 11:03:28AM -0800, Dmitry Torokhov wrote:
> > On Tue, Nov 12, 2019 at 12:03:07PM +0000, Mark Brown wrote:
> > > On Mon, Nov 11, 2019 at 09:54:10PM -0800, Dmitry Torokhov wrote:
> 
> > > > +      interrupts:
> > > > +        items:
> > > > +          - description: main interrupt (attention) line.
> > > > +          - description: dedicated wakeup interrupt.
> > > > +        minItems: 1 # The wakeup interrupt is optional.
> 
> > > > +      interrupt-names:
> > > > +        items:
> > > > +          - const: irq
> > > > +          - const: wakeup
> > > > +        minItems: 1
> 
> > > How will this interact with a SPI device that defines interrupts at the
> > > device level, possibly more than one of them?  Especially if the device
> > > has its own idea what the interrupts should be called.
> 
> > My understanding that individual drivers should be able to override
> > whatever the default behavior core has configured, and the device can
> > establish their own mapping. We have this in I2C and I believe this
> > works well.
> 
> > Is the concern about the device tree scheme or SPI core handling?
> 
> Both really.

So as I mentioned, the driver is not forced to use the interrupt
supplied by the SPI core, and the worst thing is that the core
configures the main IRQ as wakeirq and driver would need to call
dev_pm_clear_wake_irq() before switching to correct one. I expect there
will be just a few drivers needing that and many more would benefit from
the default behavior and not needing to repeat the same boilerplate
code.

As far as scheme goes - I hope that Rob could confirm that we can
override number of interrupts and names in consumers of the binding, as
needed.

-- 
Dmitry

Re: [PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Mark Brown]

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

On Tue, Nov 12, 2019 at 11:36:53AM -0800, Dmitry Torokhov wrote:

> As far as scheme goes - I hope that Rob could confirm that we can
> override number of interrupts and names in consumers of the binding, as
> needed.

Yes, I think that's the main worry here - if there's issue with the
framework bit we should be able to sort that.

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

Re: [PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Rob Herring]

On Tue, Nov 12, 2019 at 11:36:53AM -0800, Dmitry Torokhov wrote:
> On Tue, Nov 12, 2019 at 07:15:47PM +0000, Mark Brown wrote:
> > On Tue, Nov 12, 2019 at 11:03:28AM -0800, Dmitry Torokhov wrote:
> > > On Tue, Nov 12, 2019 at 12:03:07PM +0000, Mark Brown wrote:
> > > > On Mon, Nov 11, 2019 at 09:54:10PM -0800, Dmitry Torokhov wrote:
> > 
> > > > > +      interrupts:
> > > > > +        items:
> > > > > +          - description: main interrupt (attention) line.
> > > > > +          - description: dedicated wakeup interrupt.
> > > > > +        minItems: 1 # The wakeup interrupt is optional.
> > 
> > > > > +      interrupt-names:
> > > > > +        items:
> > > > > +          - const: irq
> > > > > +          - const: wakeup
> > > > > +        minItems: 1
> > 
> > > > How will this interact with a SPI device that defines interrupts at the
> > > > device level, possibly more than one of them?  Especially if the device
> > > > has its own idea what the interrupts should be called.
> > 
> > > My understanding that individual drivers should be able to override
> > > whatever the default behavior core has configured, and the device can
> > > establish their own mapping. We have this in I2C and I believe this
> > > works well.
> > 
> > > Is the concern about the device tree scheme or SPI core handling?
> > 
> > Both really.
> 
> So as I mentioned, the driver is not forced to use the interrupt
> supplied by the SPI core, and the worst thing is that the core
> configures the main IRQ as wakeirq and driver would need to call
> dev_pm_clear_wake_irq() before switching to correct one. I expect there
> will be just a few drivers needing that and many more would benefit from
> the default behavior and not needing to repeat the same boilerplate
> code.
> 
> As far as scheme goes - I hope that Rob could confirm that we can
> override number of interrupts and names in consumers of the binding, as
> needed.

This won't work. A device schema doesn't override what's defined here, 
but just further constrains this schema.

You could define a "spi irq" schema which devices can include if they 
want to, but I don't think this pattern is that common to SPI devices. 
There's not any spec behind compared to say alert irq for SMBus. 

The 'wakeup' irq name is standardized (for DT), but that's not SPI 
specific. About all we could define there is 'wakeup-source' is boolean 
and if there is more than one interrupt, one should be named 'wakeup'.

Rob

Re: [PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Dmitry Torokhov]

On Thu, Nov 14, 2019 at 2:26 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Nov 12, 2019 at 11:36:53AM -0800, Dmitry Torokhov wrote:
> > On Tue, Nov 12, 2019 at 07:15:47PM +0000, Mark Brown wrote:
> > > On Tue, Nov 12, 2019 at 11:03:28AM -0800, Dmitry Torokhov wrote:
> > > > On Tue, Nov 12, 2019 at 12:03:07PM +0000, Mark Brown wrote:
> > > > > On Mon, Nov 11, 2019 at 09:54:10PM -0800, Dmitry Torokhov wrote:
> > >
> > > > > > +      interrupts:
> > > > > > +        items:
> > > > > > +          - description: main interrupt (attention) line.
> > > > > > +          - description: dedicated wakeup interrupt.
> > > > > > +        minItems: 1 # The wakeup interrupt is optional.
> > >
> > > > > > +      interrupt-names:
> > > > > > +        items:
> > > > > > +          - const: irq
> > > > > > +          - const: wakeup
> > > > > > +        minItems: 1
> > >
> > > > > How will this interact with a SPI device that defines interrupts at the
> > > > > device level, possibly more than one of them?  Especially if the device
> > > > > has its own idea what the interrupts should be called.
> > >
> > > > My understanding that individual drivers should be able to override
> > > > whatever the default behavior core has configured, and the device can
> > > > establish their own mapping. We have this in I2C and I believe this
> > > > works well.
> > >
> > > > Is the concern about the device tree scheme or SPI core handling?
> > >
> > > Both really.
> >
> > So as I mentioned, the driver is not forced to use the interrupt
> > supplied by the SPI core, and the worst thing is that the core
> > configures the main IRQ as wakeirq and driver would need to call
> > dev_pm_clear_wake_irq() before switching to correct one. I expect there
> > will be just a few drivers needing that and many more would benefit from
> > the default behavior and not needing to repeat the same boilerplate
> > code.
> >
> > As far as scheme goes - I hope that Rob could confirm that we can
> > override number of interrupts and names in consumers of the binding, as
> > needed.
>
> This won't work. A device schema doesn't override what's defined here,
> but just further constrains this schema.
>
> You could define a "spi irq" schema which devices can include if they
> want to, but I don't think this pattern is that common to SPI devices.
> There's not any spec behind compared to say alert irq for SMBus.
>
> The 'wakeup' irq name is standardized (for DT), but that's not SPI
> specific. About all we could define there is 'wakeup-source' is boolean
> and if there is more than one interrupt, one should be named 'wakeup'.

OK, so what I am hearing is "interrupt"/"interrupt-names" properties
should be defined in individual device's bindings, and wakeup-source
can stay in spi-controller.yaml, right?

And as far as SPI core goes, we can still do what I proposed, because
we already handle "first" interrupt as the default one and the drivers
can override as needed anyway...

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Rob Herring]

On Thu, Nov 14, 2019 at 5:09 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Thu, Nov 14, 2019 at 2:26 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Nov 12, 2019 at 11:36:53AM -0800, Dmitry Torokhov wrote:
> > > On Tue, Nov 12, 2019 at 07:15:47PM +0000, Mark Brown wrote:
> > > > On Tue, Nov 12, 2019 at 11:03:28AM -0800, Dmitry Torokhov wrote:
> > > > > On Tue, Nov 12, 2019 at 12:03:07PM +0000, Mark Brown wrote:
> > > > > > On Mon, Nov 11, 2019 at 09:54:10PM -0800, Dmitry Torokhov wrote:
> > > >
> > > > > > > +      interrupts:
> > > > > > > +        items:
> > > > > > > +          - description: main interrupt (attention) line.
> > > > > > > +          - description: dedicated wakeup interrupt.
> > > > > > > +        minItems: 1 # The wakeup interrupt is optional.
> > > >
> > > > > > > +      interrupt-names:
> > > > > > > +        items:
> > > > > > > +          - const: irq
> > > > > > > +          - const: wakeup
> > > > > > > +        minItems: 1
> > > >
> > > > > > How will this interact with a SPI device that defines interrupts at the
> > > > > > device level, possibly more than one of them?  Especially if the device
> > > > > > has its own idea what the interrupts should be called.
> > > >
> > > > > My understanding that individual drivers should be able to override
> > > > > whatever the default behavior core has configured, and the device can
> > > > > establish their own mapping. We have this in I2C and I believe this
> > > > > works well.
> > > >
> > > > > Is the concern about the device tree scheme or SPI core handling?
> > > >
> > > > Both really.
> > >
> > > So as I mentioned, the driver is not forced to use the interrupt
> > > supplied by the SPI core, and the worst thing is that the core
> > > configures the main IRQ as wakeirq and driver would need to call
> > > dev_pm_clear_wake_irq() before switching to correct one. I expect there
> > > will be just a few drivers needing that and many more would benefit from
> > > the default behavior and not needing to repeat the same boilerplate
> > > code.
> > >
> > > As far as scheme goes - I hope that Rob could confirm that we can
> > > override number of interrupts and names in consumers of the binding, as
> > > needed.
> >
> > This won't work. A device schema doesn't override what's defined here,
> > but just further constrains this schema.
> >
> > You could define a "spi irq" schema which devices can include if they
> > want to, but I don't think this pattern is that common to SPI devices.
> > There's not any spec behind compared to say alert irq for SMBus.
> >
> > The 'wakeup' irq name is standardized (for DT), but that's not SPI
> > specific. About all we could define there is 'wakeup-source' is boolean
> > and if there is more than one interrupt, one should be named 'wakeup'.
>
> OK, so what I am hearing is "interrupt"/"interrupt-names" properties
> should be defined in individual device's bindings, and wakeup-source
> can stay in spi-controller.yaml, right?

It could, but it's not SPI specific. I think we should convert
bindings/power/wakeup-source.txt instead. Something like this:

select: true

properties:
  wakeup-source:
    type: boolean
    description: ...

if:
  properties:
    interrupt-names:
      contains:
        const: wakeup
  required:
    - interrupt-names
then:
  required:
    - wakeup-source

dependencies:
  wakeup-source: [ interrupts ]


Rob

> And as far as SPI core goes, we can still do what I proposed, because
> we already handle "first" interrupt as the default one and the drivers
> can override as needed anyway...
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Mark Brown]

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

On Fri, Nov 15, 2019 at 07:52:22AM -0600, Rob Herring wrote:

> if:
>   properties:
>     interrupt-names:
>       contains:
>         const: wakeup
>   required:
>     - interrupt-names
> then:
>   required:
>     - wakeup-source

That seems to say that if we have a device that has an interrupt called
"wakeup" then it must be a wakeup source.  Is that desirable?  Being
able to wake the system is partly a property of the system as a whole
(the wakeup signal needs to be wired somewhere where it can wake things)
and a device might have a signal that could be used to wake the system,
may even be called "wakeup" by the device but for some reason isn't
wired suitably in a given system.

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

Re: [PATCH 1/2] spi: dt-bindings: spi-controller: add wakeup-source and interrupts

[Rob Herring]

On Fri, Nov 15, 2019 at 9:22 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Nov 15, 2019 at 07:52:22AM -0600, Rob Herring wrote:
>
> > if:
> >   properties:
> >     interrupt-names:
> >       contains:
> >         const: wakeup
> >   required:
> >     - interrupt-names
> > then:
> >   required:
> >     - wakeup-source
>
> That seems to say that if we have a device that has an interrupt called
> "wakeup" then it must be a wakeup source.  Is that desirable?  Being
> able to wake the system is partly a property of the system as a whole
> (the wakeup signal needs to be wired somewhere where it can wake things)
> and a device might have a signal that could be used to wake the system,
> may even be called "wakeup" by the device but for some reason isn't
> wired suitably in a given system.

Perhaps it is too strict. It would be useful as a "Did you forget
wakeup-source?" message, but we don't have a way to distinguish that.

Rob