Received-SPF: pass (google.com: domain of linux-kernel+bounces-20055-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240102210820.2604667-1-markhas@chromium.org>
 <20240102140734.v4.24.Ieee574a0e94fbaae01fd6883ffe2ceeb98d7df28@changeid>
 <CAE-0n50zkwZ8nguUJcL1gjbuavhSU_rLxfGhanxB4YA7N34hLQ@mail.gmail.com>
 <CANg-bXByhaSngW2AAG9h6DYHpiTUvh8+yw3LPU6ZQSCb62M-wg@mail.gmail.com>
 <CAE-0n52u68wMHJGe8=jz4Y1y2=voycFEY15keebz9tPDDBgiqA@mail.gmail.com>
 <CANg-bXDzLJgWLuH8Xj4GLYG=AVfcbmi_EfrA7DaHj4F6i350DA@mail.gmail.com>
 <CAE-0n52365_AYgjaXyV7+oB8WgaJVp5oCUzdsq7NquZsR08XXw@mail.gmail.com>
 <CANg-bXAVm01qoGUL2hUN9=3eK5o6c6BKtTUL82WiLL8cxFeaLA@mail.gmail.com> <CAE-0n50nNXSybfAnOgAk4g8hOma7=yLG8+y9PpHaXXSUE-zfsw@mail.gmail.com>
In-Reply-To: <CAE-0n50nNXSybfAnOgAk4g8hOma7=yLG8+y9PpHaXXSUE-zfsw@mail.gmail.com>
From: Mark Hasemeyer <markhas@chromium.org>
Date: Mon, 8 Jan 2024 12:56:44 -0700
Message-ID: <CANg-bXAO0Pk8Lea-Ci+k56ucBDA+w-2+3L_uP6y2=-JihujXDg@mail.gmail.com>
Subject: Re: [PATCH v4 24/24] platform/chrome: cros_ec: Use PM subsystem to
 manage wakeirq
To: Stephen Boyd <swboyd@chromium.org>
Cc: LKML <linux-kernel@vger.kernel.org>, Sudeep Holla <sudeep.holla@arm.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, Rob Herring <robh@kernel.org>, 
	Andy Shevchenko <andriy.shevchenko@intel.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Raul Rangel <rrangel@chromium.org>, Tzung-Bi Shih <tzungbi@kernel.org>, 
	Benson Leung <bleung@chromium.org>, Bhanu Prakash Maiya <bhanumaiya@chromium.org>, 
	Chen-Yu Tsai <wenst@chromium.org>, Guenter Roeck <groeck@chromium.org>, 
	Prashant Malani <pmalani@chromium.org>, Rob Barnes <robbarnes@google.com>, 
	chrome-platform@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"

> Isn't this patch series making the wakeup-source DT property required
> for all existing DT nodes? I'm saying that the property is implicit
> based on the compatible string "google,cros-ec-{spi,rpmsg,uart}", so
> we shouldn't add the property explicitly. Just rely on the compatible
> string to convey the property's existence.

The current wording in 'wakeup-source.txt' states: "Nodes that
describe devices which has wakeup capability must contain a
"wakeup-source" boolean property." According to that wording, the
existing DTS does not match the expectation. This is what led me to
add the property. However, feedback from KML mentioned the wording may
be a little strong and it should be updated. Hence patch 04 in this
series.

I can revert the SPI driver to assume wake capability, which will no
longer make the wakeup-source property required. At that point,
leaving the property in the DTS simply provides an indication. Considering it
won't be required, I can drop the DTS patches that add the property.

> I'm no expert in ACPI so sorry if I'm misunderstanding. The driver
> unconditionally enables wake on the irq.

Yes.

> Most other chromebooks have
> added some other interrupt (GPE?) for wakeup purposes, which is
> different from the irq used for IO?

The GPE is used for wake and IO (It processes ACPI notify alerts).
AFAIK, the separate IRQ was introduced for latency reasons as the GPE
path was too slow.

> And this patch series tries to
> figure out if enable_irq_wake() is going to fail on those devices so it
> can only enable irq wake if the irq supports it? When does calling
> enable_irq_wake() not return an error to properly indicate that the irq
> can't wake? On skyrim devices, where presumably it needed to be marked
> in ACPI differently? Or does that platform really support wake on the
> irq, but we also have a GPE so enabling wake on the irq is not failing?

The patch series does two things:
1. Determines whether the irq should be enabled for wake, as opposed
to assuming (at least for LPC/ACPI).
2. Moves enable_irq_wake() logic to the PM subsystem.

Skyrim does _not_ support wake on irq. It uses a GPE. So the patch
series drops the assumption that irqwake should be enabled. Instead,
it polls the ACPI tables to determine whether or not the IRQ should be
enabled for wake.

> Having to backport 24 patches to fix a bug is not good.

Some of the patches were DTS related as a result of my interpretation
of 'wakeup-source.txt' (see above comment). Other patches are
tangential based on KML feedback to fix things that are orthogonal to
the bug itself.

> Can the driver
> look for both an IO interrupt and a GPE and then assume the GPE is for
> wakeup and the interrupt is for IO?

No, some boards need the IO based irq to wake, and may use both.

> > 3. Leave the existing solution
>
> How is 3 an option? I thought this patch series was fixing a bug.

I meant the solution in the existing patch train.