Message-ID: <CAE-0n53UXmNZ9GUfh0A+F3NyJd2QqDL1GFaf-FtaJ2EidzBMeA@mail.gmail.com>
Date:   Mon, 21 Mar 2022 21:21:07 +0100
From:   Stephen Boyd <swboyd@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Benson Leung <bleung@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        chrome-platform@...ts.linux.dev,
        Guenter Roeck <groeck@...omium.org>,
        Craig Hesling <hesling@...omium.org>,
        Tom Hughes <tomhughes@...omium.org>,
        Alexandru M Stan <amstan@...omium.org>,
        Tzung-Bi Shih <tzungbi@...nel.org>,
        Matthias Kaehlcke <mka@...omium.org>
Subject: Re: [PATCH v4 3/3] platform/chrome: cros_ec_spi: Boot fingerprint
 processor during probe

Quoting Doug Anderson (2022-03-21 13:04:20)
> Hi,
>
> On Mon, Mar 21, 2022 at 12:11 PM Stephen Boyd <swboyd@...omium.org> wrote:
> >
> > Add gpio control to this driver so that the fingerprint device can be
> > booted if the BIOS isn't doing it already. This eases bringup of new
> > hardware as we don't have to wait for the BIOS to be ready, supports
> > kexec where the GPIOs may not be configured by the previous boot stage,
> > and is all around good hygiene because we control GPIOs for this device
> > from the device driver.
> >
> > Cc: Guenter Roeck <groeck@...omium.org>
> > Cc: Douglas Anderson <dianders@...omium.org>
> > Cc: Craig Hesling <hesling@...omium.org>
> > Cc: Tom Hughes <tomhughes@...omium.org>
> > Cc: Alexandru M Stan <amstan@...omium.org>
> > Cc: Tzung-Bi Shih <tzungbi@...nel.org>
> > Reviewed-by: Matthias Kaehlcke <mka@...omium.org>
> > Signed-off-by: Stephen Boyd <swboyd@...omium.org>
> > ---
> >  drivers/platform/chrome/cros_ec_spi.c | 42 +++++++++++++++++++++++++--
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> > index 51b64b392c51..92518f90f86e 100644
> > --- a/drivers/platform/chrome/cros_ec_spi.c
> > +++ b/drivers/platform/chrome/cros_ec_spi.c
> > @@ -4,6 +4,7 @@
> >  // Copyright (C) 2012 Google, Inc
> >
> >  #include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > @@ -690,11 +691,13 @@ 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 int cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> >  {
> >         struct device_node *np = dev->of_node;
> >         u32 val;
> >         int ret;
> > +       struct gpio_desc *boot0;
> > +       struct gpio_desc *reset;
> >
> >         ret = of_property_read_u32(np, "google,cros-ec-spi-pre-delay", &val);
> >         if (!ret)
> > @@ -703,6 +706,37 @@ 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;
> > +
> > +       if (!of_device_is_compatible(np, "google,cros-ec-fp"))
> > +               return 0;
> > +
> > +       boot0 = devm_gpiod_get(dev, "boot0", 0);
>
> I think that the last parameter to devm_gpiod_get() is better
> described by "GPIOD_ASIS", right? Same for the other one below.

Yes, 0 is GPIOD_ASIS.

>
>
> > +       if (IS_ERR(boot0))
> > +               return PTR_ERR(boot0);
> > +
> > +       reset = devm_gpiod_get(dev, "reset", 0);
> > +       if (IS_ERR(reset))
> > +               return PTR_ERR(reset);
> > +
> > +       /*
> > +        * Take the FPMCU out of reset and wait for it to boot if it's in
> > +        * bootloader mode or held in reset. This isn't the normal flow because
> > +        * typically the BIOS has already powered on the device to avoid the
> > +        * multi-second delay waiting for the FPMCU to boot and be responsive.
> > +        */
> > +       if (gpiod_get_value(boot0) || gpiod_get_value(reset)) {
>
> I believe that the above two calls are illegal as documented. The file
> `Documentation/driver-api/gpio/consumer.rst` says that if you use
> `GPIOD_ASIS` to get the GPIO that "The direction must be set later
> with one of the dedicated functions.". The "must" there is important.
>
> Oh, and further down it appears to be even more explicit and says "Be
> aware that there is no default direction for GPIOs. Therefore, **using
> a GPIO without setting its direction first is illegal and will result
> in undefined behavior!**".
>
> I assume that "get" counts as using?
>
> I think this sorta gets into some of the limitations of the GPIO APIs
> in Linux that try to make sure that they work on a "lowest common
> denominator" GPIO controller. I don't think they promise that
> "get_value" while in output mode is legal across all GPIO controllers.
>
> Maybe a solution is to at least add a comment saying that the code
> will only work on GPIO controllers that will let you get the value
> back if it's an output?

Ugh, yeah. I don't have a good way to detect that it is already booted
by the BIOS then, besides adding some other DT property that can
indicate we need to boot it, like "google,needs-boot", or have a
different compatible string for this device when it is powered off.

Or we could try to communicate with the device and if it fails to
respond we get the gpios and do the boot sequence before giving up on
probe. I don't see much value in that though so I'm leaning towards
dropping this patch.

>
>
> > +               /* Boot0 is sampled on reset deassertion */
> > +               gpiod_set_value(boot0, 0);
> > +               gpiod_set_value(reset, 1);
>
> Those two calls are almost certainly illegal / not guaranteed to work
> without setting a direction, at least in the general case. Luckily I
> think it's easy to just change both of them to
> "gpiod_direction_output", which takes a value.
>
> Actually, even on Qualcomm hardware I don't think those will work if
> the boot direction was input, will they? They'll set the value that
> _will_ be driven but won't cause it to actually be driven, right?
>

Correct.

Powered by blists - more mailing lists