Message-ID: <CAE-0n52365_AYgjaXyV7+oB8WgaJVp5oCUzdsq7NquZsR08XXw@mail.gmail.com>
Date: Wed, 3 Jan 2024 16:29:34 -0800
From: Stephen Boyd <swboyd@...omium.org>
To: Mark Hasemeyer <markhas@...omium.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Sudeep Holla <sudeep.holla@....com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Rob Herring <robh@...nel.org>, 
	Andy Shevchenko <andriy.shevchenko@...el.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, 
	Raul Rangel <rrangel@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>, 
	Benson Leung <bleung@...omium.org>, Bhanu Prakash Maiya <bhanumaiya@...omium.org>, 
	Chen-Yu Tsai <wenst@...omium.org>, Guenter Roeck <groeck@...omium.org>, 
	Prashant Malani <pmalani@...omium.org>, Rob Barnes <robbarnes@...gle.com>, 
	chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v4 24/24] platform/chrome: cros_ec: Use PM subsystem to
 manage wakeirq

Quoting Mark Hasemeyer (2024-01-03 14:25:25)
> > The DTS patch would go through the platform maintainer tree and be
> > pulled into the soc tree and sent up to mainline from there. The
> > platform/chrome patch would go through chrome platform tree and then to
> > mainline. The bisection hole will be real.
>
> I thought it would all get merged in the next merge window. How are
> bifurcations like this normally dealt with? Does one wait for the
> first series of patches to land in mainline before submitting
> dependent patches? That could take two merge windows.

It's usually fine to land in the respective maintainer trees because the
driver is written to be compatible with either version of the DT.

>
> > >
> > > > Does using the pm wakeirq logic require the use of 'wakeup-source'
> > > > property in DT? A quick glance makes me believe it isn't needed, so
> > > > please split that part out of this patch as well.
> > >
> > > No, 'wakeup-source' is not required, it provides an indication to the
> > > driver that the IRQ should be used for wake, which then enables the pm
> > > subsystem accordingly. If 'wakup-source' is not used, we're back to
> > > square one of making assumptions. Specifically in this case, it would
> > > be assumed that all SPI based EC's are wake capable.
> >
> > Is that the wrong assumption to make? My understanding of the DT
> > property is that it is used to signal that this device should be treated
> > as a wakeup source, when otherwise it isn't treated as one. In this
> > case, the device has always been treated as a wakeup source so adding
> > the property is redundant.
>
> For SPI, it's not the wrong assumption. I was trying to drop the
> assumption though to match ACPI/LPC behavior.

Ok. Is the EC always a wakeup source? Not the EC irq, the EC device.

>
> > Making the patch series dependent on the
> > property being present makes the driver break without the DT change. We
> > try to make drivers work with older DT files, sometimes by adding
> > backwards compatibility code, so the presence of the wakeup-source
> > property should not be required to make this work.
>
> Do we have use cases where Chromebooks are running older DTBs? I get
> the idea in theory, but don't grasp why it's needed here.

It's to make the kernel bisectable while the DTB and driver patches are
merged through different trees.

> Regardless,
> I can update the SPI code to assume a wake capable IRQ. But I'd like
> to keep the prerequisite device tree patches unless there's a good
> reason to drop them. Specifying 'wake-source' certainly doesn't hurt
> anything, and there are other improvements to the OF
> subsystem/documentation.

I don't see any harm in having wakeup-source in the binding, but I'd
prefer it is behind a different compatible string as optional, and
introduced when we need to have an EC that doesn't wake up the system.
Otherwise it's all future coding for a device that doesn't exist. It's
also a potential landmine if the driver patch is backported somewhere
without the DTS patch (maybe the DTS is not upstream?). Someone will
have to debug why wakeups aren't working anymore.

>
> > What is the goal of this patch series? Is it to allow disabling the
> > wakeup capability of the EC wake irq from userspace? I can see a
> > possible problem where we want to disable wakeup for the EC dynamically
> > because either it has no child devices that are wakeup sources (e.g. no
> > power button, no keyboard on ARM) or userspace has disabled all the
> > wakeup sources for those child devices at runtime. In that case, we
> > would want to keep the EC irq from waking up the system from suspend. Is
> > that what you're doing here?
>
> The root of this patch series stems from a bug where spurious wakes
> are seen on Skyrim.

Are all 24 patches needed to fix spurious wakeups? Why can't we do a DMI
match table for Skyrim devices and disable the wakeirq logic on that
platform? That would be a much more focused and targeted fix, no?

> Copying some wording from the DTS patches:
> "Some Chromebooks use a separate wake pin, while others overload the
> interrupt for wake and IO. With the current assumption, spurious wakes
> can occur on systems that use a separate wake pin. It is planned to
> update the driver to no longer assume that the EC interrupt pin should
> be enabled for wake."
>
> This patch series will allow us to disable the ec_sync pin as a wake
> source on Skyrim as it already uses a dedicated wake gpio.

Aha! This last sentence is the detail I've been looking for. Please put
these details in the commit text.

"Skyrim devices are experiencing spurious wakeups due to the EC driver
always enabling the irq as a wakeup source but on Skyrim devices the EC
wakeup signal is a dedicated gpio separate from the irq."

Please be direct and specific instead of writing in general terms.

Powered by blists - more mailing lists