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

[Stephen Boyd]

Quoting Mark Hasemeyer (2024-01-08 11:56:44)
> > 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.

Alright, I don't know what ACPI notify alerts are so most likely that is
causing me confusion.

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

Does the call to enable_irq_wake() on skyrim succeed? It seems like the
driver considers failure to enable wake on the irq as the way to figure
out if the irq supports wakeup or not. I'm trying to understand why
anything needs to be changed.

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

Fair enough. The fix should be isolated and be early in the series so
that we don't need to backport the whole stack to fix a bug.

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

Ok.

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

Got it.