Received-SPF: pass (google.com: domain of linux-kernel+bounces-10078-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: <20231220235459.2965548-1-markhas@chromium.org>
 <20231220165423.v2.22.Ieee574a0e94fbaae01fd6883ffe2ceeb98d7df28@changeid> <ZYRdjHVgES1odZAQ@smile.fi.intel.com>
In-Reply-To: <ZYRdjHVgES1odZAQ@smile.fi.intel.com>
From: Mark Hasemeyer <markhas@chromium.org>
Date: Fri, 22 Dec 2023 15:21:34 -0700
Message-ID: <CANg-bXAgiMB2Q_fHOc_BBm4pRXdVveu-Rs6uZrLCAjkzaBYm4w@mail.gmail.com>
Subject: Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to
 manage wakeirq
To: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Tzung-Bi Shih <tzungbi@kernel.org>, 
	Raul Rangel <rrangel@chromium.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Rob Herring <robh@kernel.org>, Sudeep Holla <sudeep.holla@arm.com>, 
	Benson Leung <bleung@chromium.org>, Bhanu Prakash Maiya <bhanumaiya@chromium.org>, 
	Chen-Yu Tsai <wenst@chromium.org>, Guenter Roeck <groeck@chromium.org>, Lee Jones <lee@kernel.org>, 
	Prashant Malani <pmalani@chromium.org>, Rob Barnes <robbarnes@google.com>, 
	Stephen Boyd <swboyd@chromium.org>, chrome-platform@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"

> > +     irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
> > +     if (irq > 0) {
> >               ec_dev->irq = irq;
> > -     else if (irq != -ENXIO) {
> > +             if (cros_ec_should_force_irq_wake_capable())
> > +                     irq_wake = true;
> > +             else
> > +                     irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
> > +             dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", irq, irq_wake);
> > +     } else if (irq != -ENXIO) {
> >               dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
> >               return irq;
> >       }
>
> Yeah, this is confusing now. Which one should I trust more: irq or irqres.start?
> What is the point to have irqres with this duplication?

irqres is needed to pull the wake capability of the irq. I agree tha
irq and irqres.start should have the same information. I chose irq
because it's more obvious what it represents over irqres.start. It's
also more concise.

>
> ...
>
> > -             dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
> > +             dev_err_probe(dev, ret, "couldn't register ec_dev (%d)\n", ret);
> >               return ret;
>
>                 return dev_err_probe(...);
>
> ...
>
> > +                     dev_err_probe(dev, ret, "Failed to init device for wakeup");
> > +                     return ret;
>

Tzung-Bi pointed out there are other areas of the driver that just use
dev_err() in the probe path. My vote is to drop the use of
dev_err_probe() to stay consistent with what exists. A separate patch
can be sent to modify the statements to use the
dev_err_probe() variant.

>
> ...
>
> > +     if (!np)
> > +             return;
>
> Why do you need this now?

It felt intuitive to return early from cros_ec_spi_dt_probe() if no
device node exists. This does not fix any known bugs though. Dropping
as irrelevant.

> >       ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
> >       if (!ret)
> >               ec_spi->end_of_msg_delay = val;
>
> > +     if (ec_dev->irq > 0 && of_property_read_bool(np, "wakeup-source")) {
> > +             ec_spi->irq_wake = true;
> > +             dev_dbg(&spi->dev, "IRQ: %i, wake_capable: %i\n", ec_dev->irq, ec_spi->irq_wake);
> > +     }
>
>         if (ret)
>                 return;
>
>         ec_spi->irq_wake = of_property_read_bool(np, "wakeup-source"));

The other interface drivers only analyze irq_wake if an irq exists. I
think that makes sense and would like to stay consistent.
Thinking:
ec_spi->irq_wake = spi->irq > 0 && of_property_read_bool(np, "wakeup-source"));

>         dev_dbg(&spi->dev, "IRQ: %i, wake_capable: %s\n", ec_dev->irq, str_yes_no(ec_spi->irq_wake));

> ...
>
> > @@ -78,6 +80,7 @@ struct cros_ec_uart {
> >       u32 baudrate;
> >       u8 flowcontrol;
> >       u32 irq;
> > +     bool irq_wake;
> >       struct response_info response;
> >  };
>
> Run `pahole` and amend respectively to avoid wasting memory.

No savings to be had, but we can defrag the holes from sizes 3 and 3,
to 2 and 4 by moving irq_wake above irq.

pahole --reorganize -C cros_ec_uart cros_ec_uart.o

struct cros_ec_uart {
        struct serdev_device *     serdev;               /*     0     8 */
        u32                        baudrate;             /*     8     4 */
        u8                         flowcontrol;          /*    12     1 */
        bool                       irq_wake;             /*    13     1 */

        /* XXX 2 bytes hole, try to pack */

        u32                        irq;                  /*    16     4 */

        /* XXX 4 bytes hole, try to pack */

        struct response_info       response;             /*    24    64 */

        /* size: 88, cachelines: 2, members: 6 */
        /* sum members: 82, holes: 2, sum holes: 6 */
        /* last cacheline: 24 bytes */
};

> > +     dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", ec_uart->irq, ec_uart->irq_wake);
>
> str_yes_no() from string_choices.h?

SGTM