Re: [PATCH v4 24/24] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Mark Hasemeyer]

> > > > > 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.

Yes. The powerd daemon enables the EC for wake [1]. At least on ACPI systems.
[1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/81b23aeac510655f27e1d87b99b12c78146e7c37:src/platform2/power_manager/powerd/daemon.cc;l=611

> > 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.

The 'wakeup-source' property is already optional. See patch 04 in this
version of the series which updates the documentation surrounding the
property. Or are you suggesting a completely new string so that we can
be forever backward compatible? If that's the case, the property would
indeed have to have inverted logic signifying a device that is *not*
wakeup capable. It feels wrong to have to do something like that.

> Otherwise it's all future coding for a device that doesn't exist.

I agree adding 'wakeup-source' is future coding for a device that
doesn't exist. It does provide (pseudo) feature parity with ACPI
systems though. See my comment below about ACPI GpioInt/Interrupt
resources.

> 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.

I can change the SPI driver so it doesn't look for the 'wakeup-source'
property, keeping existing behavior where wakeirq is assumed. So there
should be no issues with backporting.

> > > 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?

It would be more focused and targeted, but I don't think it's the
correct fix. Skyrim is not the quirk. The driver is incorrectly
enabling the IRQ for wake even though a GPE exists for the EC to wake the AP.
ACPI defines keywords to specify GpioInt and Interrupt
resources as wake capable, and until recently [2], we were not
flagging the respective resources correctly. Most ACPI Chromebooks
have a dedicated GPE for wake, and enabling the IRQ for wake is
unintentional IMHO.
[2] https://review.coreboot.org/c/coreboot/+/79373

> > 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.

Sure, I can update the commit text :-)

In summary, there are a lot of comments that suggest different
solutions. Here are the options I see:
1. Skyrim DMI quirk
2a. Update EC SPI driver to assume wake capable regardless of
'wakeup-source' property being present
2b. Remove 'wakeup-source' entries from DTS
3. Leave the existing solution

I'm arguing for 2a (without 2b). This keeps backward compatibility
while adding an indication that the EC is wake capable and keeps
closer feature parity with the ACPI/LPC interface. FWIW, others have
already reviewed/ack'd the dts patches.