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

From: Sudeep Holla <sudeep.holla@arm.com>
To: Mark Hasemeyer <markhas@chromium.org>
Cc: Rob Herring <robh@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Raul Rangel <rrangel@chromium.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Frank Rowand <frowand.list@gmail.com>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v1 3/6] of: irq: add wake capable bit to of_irq_resource()
Date: Mon, 18 Dec 2023 10:49:43 +0000	[thread overview]
Message-ID: <ZYAjxxHcCOgDVMTQ@bogus> (raw)
In-Reply-To: <CANg-bXDfOp4e6WDx9JF5XyxjSvh-ctNsM1TTgr8N1NzfDvzpgw@mail.gmail.com>

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