Message-ID: <CANg-bXByhaSngW2AAG9h6DYHpiTUvh8+yw3LPU6ZQSCb62M-wg@mail.gmail.com>
Date: Wed, 3 Jan 2024 10:47:17 -0700
From: Mark Hasemeyer <markhas@...omium.org>
To: Stephen Boyd <swboyd@...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

> The DMI quirk looks to be fixing something. Most likely that should be
> backported to stable kernels as well? Please split the DMI match table
> part out of this so that it isn't mixed together with migrating the
> driver to use the pm wakeirq logic.

The DMI quirk is used to list boards whose IRQ should be enabled for
wake, regardless of what their firmware says. Currently the driver
always enables the EC IRQ for wake anyway, so there shouldn't be a
need to backport the DMI quirk on its own. Splitting out the DMI quirk
into its own patch would break Brya/Brask's ability to wake if they
were to run a kernel w/o the DMI patch. I chose not to split it out to
keep the change atomic, and avoid introducing any regressions.

> > For device tree base systems, it is not an issue as the relevant device
> > tree entries have been updated and DTS is built from source for each
> > ChromeOS update.
>
> It is still sorta an issue. It means that we can't backport this patch
> without also backporting the DTS patch to the kernel. Furthermore, DTS
> changes go through different maintainer trees, so having this patch in
> the kernel without the DTS patch means the bisection hole is possibly
> quite large.

Correct, this patch doesn't make sense to backport on its own. It is
dependent on the entire patch series (more than just the DTS changes).
I'm not super familiar with how patches flow through different
maintainer trees. But I'd imagine this patch series makes its way to
torvalds/master in some sort of sane manner where earlier patches land
before later (dependent) ones?

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

> We should see a patch
> for the DMI quirk, a patch to use wakeup-source (doubtful that we need
> it at all though), and a patch to use the pm wakeirq logic instead of
> hand rolling it again.

I don't know if I'm convinced this should happen. I'm open to it, but
see my previous comments.

> > +static int enable_irq_for_wake(struct cros_ec_device *ec_dev)
> > +{
> > +       struct device *dev = ec_dev->dev;
> > +       int ret = device_init_wakeup(dev, true);
> > +
> > +       if (ret) {
> > +               dev_err(dev, "Failed to enable device for wakeup");
>
> Missing newline on printk message.

Ack.

>
> > +               return ret;
> > +       }
> > +       ret = dev_pm_set_wake_irq(dev, ec_dev->irq);
> > +       if (ret)
> > +               device_init_wakeup(dev, false);
> > +
> > +       return ret;
>
> I'd rather see the code formatted like this:
>
>         int ret;
>
>         ret = device_init_wakeup(dev, true);
>         if (ret) {
>                 dev_err(...);
>                 return ret;
>         }
>
>         ret = dev_pm_set_wake_irq(...);
>         if (ret)
>                 device_init_wakeup(dev, false);
>
>         return ret;
>
> Mostly because the first 'if (ret)' requires me to hunt in the variable
> declaration part of the function to figure out what it is set to instead
> of looking at the line directly above.

Sounds good :-)


> [...]
> > diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> >  struct cros_ec_spi {
> >         struct spi_device *spi;
> > @@ -77,6 +79,7 @@ struct cros_ec_spi {
> >         unsigned int start_of_msg_delay;
> >         unsigned int end_of_msg_delay;
> >         struct kthread_worker *high_pri_worker;
> > +       bool irq_wake;
>
> This is only used in probe. Make it a local variable instead of another
> struct member to save memory.

Ack.

>
> >  };
> >
> >  typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev,
> > @@ -689,9 +692,10 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
> >         return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi);
> >  }
> >
> > -static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> > +static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi)
> >  {
> > -       struct device_node *np = dev->of_node;
> > +       struct spi_device *spi = ec_spi->spi;
> > +       struct device_node *np = spi->dev.of_node;
> >         u32 val;
> >         int ret;
> >
> > @@ -702,6 +706,8 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> >         ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
> >         if (!ret)
> >                 ec_spi->end_of_msg_delay = val;
> > +
> > +       ec_spi->irq_wake = spi->irq > 0 && of_property_present(np, "wakeup-source");
>
> Is there any EC SPI device that doesn't want to support wakeup?

I don't know for sure. If so, the EC driver doesn't currently support
it because it assumes wake capability.

> I'd
> prefer we introduce a new property or compatible string to indicate that
> wakeup isn't supported and otherwise always set irq_wake to true. That
> way DT can change in parallel with the driver instead of in lockstep.

We could introduce a custom binding? IMHO, using inverted logic like
that kind of goes against the grain with how ACPI and kernel are
currently structured. And I don't love how it would make the SPI EC
driver inverted from the other interfaces. I'd rather go back to just
assuming all SPI based EC's are wake capable. But even then, why
assume? This gives us flexibility to disable wakeirqs and drops
unnecessary assumptions. It also lays the groundwork for new boards
that may behave differently. For example, an ACPI based SPI EC.

Powered by blists - more mailing lists