MIME-Version: 1.0
In-Reply-To: <02aa65e7-e967-055b-2af3-2e9b6ef77935@nvidia.com>
References: <20170908205011.77986-1-briannorris@chromium.org> <02aa65e7-e967-055b-2af3-2e9b6ef77935@nvidia.com>
From: Shawn N <shawnn@chromium.org>
Date: Tue, 19 Sep 2017 07:09:53 -0700
Message-ID: <CALaWCOMj0wQk5OfYOYqU_sZUt2SQBhy=HaP-qOiB5aMf9G8inw@mail.gmail.com>
Subject: Re: [PATCH v3] platform/chrome: Use proper protocol transfer function
To: Jon Hunter <jonathanh@nvidia.com>
Cc: Brian Norris <briannorris@chromium.org>,
        Olof Johansson <olof@lixom.net>, Benson Leung <bleung@chromium.org>,
        Lee Jones <lee.jones@linaro.org>, linux-kernel@vger.kernel.org,
        Doug Anderson <dianders@chromium.org>,
        Brian Norris <computersforpeace@gmail.com>,
        Javier Martinez Canillas <javier@osg.samsung.com>,
        Gwendal Grignou <gwendal@chromium.org>,
        Enric Balletbo <enric.balletbo@collabora.co.uk>,
        Tomeu Vizoso <tomeu.vizoso@collabora.com>,
        "linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3375
Lines: 90

On Tue, Sep 19, 2017 at 6:44 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> Hi Brian,
>
> On 08/09/17 21:50, Brian Norris wrote:
> > From: Shawn Nematbakhsh <shawnn@chromium.org>
> >
> > pkt_xfer should be used for protocol v3, and cmd_xfer otherwise. We had
> > one instance of these functions correct, but not the second, fall-back
> > case. We use the fall-back only when the first command returns an
> > IN_PROGRESS status, which is only used on some EC firmwares where we
> > don't want to constantly poll the bus, but instead back off and
> > sleep/retry for a little while.
> >
> > Fixes: 2c7589af3c4d ("mfd: cros_ec: add proto v3 skeleton")
> > Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
> > ---
> > v3:
> >  * Added Javier's reviewed tag
> >  * It's been > 8 months since [1], so why not? And hey, Benson's officially in
> >    MAINTAINERS now! Too bad no one told me.
> >
> >  [1] https://patchwork.kernel.org/patch/9450633/
> >
> >
> > v2:
> >  * Add Benson in 'To:'
> >  * make subject prefix more obvious
> >
> >  drivers/platform/chrome/cros_ec_proto.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index 8dfa7fcb1248..e7bbdf947bbc 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -60,12 +60,14 @@ static int send_command(struct cros_ec_device *ec_dev,
> >                       struct cros_ec_command *msg)
> >  {
> >       int ret;
> > +     int (*xfer_fxn)(struct cros_ec_device *ec, struct cros_ec_command *msg);
> >
> >       if (ec_dev->proto_version > 2)
> > -             ret = ec_dev->pkt_xfer(ec_dev, msg);
> > +             xfer_fxn = ec_dev->pkt_xfer;
> >       else
> > -             ret = ec_dev->cmd_xfer(ec_dev, msg);
> > +             xfer_fxn = ec_dev->cmd_xfer;
> >
> > +     ret = (*xfer_fxn)(ec_dev, msg);
> >       if (msg->result == EC_RES_IN_PROGRESS) {
> >               int i;
> >               struct cros_ec_command *status_msg;
> > @@ -88,7 +90,7 @@ static int send_command(struct cros_ec_device *ec_dev,
> >               for (i = 0; i < EC_COMMAND_RETRIES; i++) {
> >                       usleep_range(10000, 11000);
> >
> > -                     ret = ec_dev->cmd_xfer(ec_dev, status_msg);
> > +                     ret = (*xfer_fxn)(ec_dev, status_msg);
> >                       if (ret < 0)
> >                               break;
> >
>
> Tegra124 Nyan-Big is currently crashing during boot with -next [0] and
> bisect is pointing to this commit. Reverting the above on top of -next
> does allow the board to boot successfully. Looks like this board is
> proto_version 3 but I have not looked into this any further. Let me know
> if you have any thoughts.


Thanks for the bug report, I'll look into this today.

> [    1.502497] kernel BUG at drivers/platform/chrome/cros_ec_proto.c:34!
> 34 BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);

So, ec_dev->proto_version > 3? That doesn't seem right.

>
>
> Cheers
> Jon
>
> [0]
> https://nvtb.github.io//linux-next/test_next-20170919/20170918213034/boot/tegra124-nyan-big/tegra124-nyan-big/tegra_defconfig_log.txt
>
>
> --
> nvpublic