[PATCH v3 1/2] drivers: tee: broadcom: add optee based bnxt fw load driver

[Rayagonda Kokatanur]

Hi Simon,

On Mon, Jun 29, 2020 at 10:56 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Rayagonda,
>
> On Sun, 28 Jun 2020 at 21:08, Rayagonda Kokatanur
> <rayagonda.kokatanur at broadcom.com> wrote:
> >
> > Hi Simon,
> >
> > On Fri, Jun 26, 2020 at 6:42 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Rayagonda,
> > >
> > > On Wed, 10 Jun 2020 at 05:15, Rayagonda Kokatanur
> > > <rayagonda.kokatanur at broadcom.com> wrote:
> > > >
> > > > From: Vikas Gupta <vikas.gupta at broadcom.com>
> > > >
> > > > Add optee based bnxt fw load driver.
> > > > bnxt is Broadcom NetXtreme controller Ethernet card.
> > > > This driver is used to load bnxt firmware binary using OpTEE.
> > > >
> > > > Signed-off-by: Vikas Gupta <vikas.gupta at broadcom.com>
> > > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com>
> > > > ---
> > > > Changes from v2:
> > > >  -Address review comments from Simon,
> > > >   Remove own return code and use standard error code.
> > > >   Take out common lines from different functions and move them
> > > >   into common static function.
> > > >   Remove include <common.h> as its not required.
> > > >   Move functions with printf from header file into c file.
> > > >
> > > >  drivers/tee/Kconfig                |   1 +
> > > >  drivers/tee/Makefile               |   1 +
> > > >  drivers/tee/broadcom/Kconfig       |   7 ++
> > > >  drivers/tee/broadcom/Makefile      |   3 +
> > > >  drivers/tee/broadcom/chimp_optee.c | 182 +++++++++++++++++++++++++++++
> > > >  include/broadcom/chimp.h           |  16 +++
> > > >  6 files changed, 210 insertions(+)
> > > >  create mode 100644 drivers/tee/broadcom/Kconfig
> > > >  create mode 100644 drivers/tee/broadcom/Makefile
> > > >  create mode 100644 drivers/tee/broadcom/chimp_optee.c
> > > >  create mode 100644 include/broadcom/chimp.h
> > > >
> > > > diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
> > > > index 5c0c89043f..5ca5a0836c 100644
> > > > --- a/drivers/tee/Kconfig
> > > > +++ b/drivers/tee/Kconfig
> > > > @@ -29,6 +29,7 @@ config SANDBOX_TEE
> > > >           "avb" commands.
> > > >
> > > >  source "drivers/tee/optee/Kconfig"
> > > > +source "drivers/tee/broadcom/Kconfig"
> > > >
> > > >  endmenu
> > > >
> > > > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> > > > index f72c68c09f..5c8ffdbce8 100644
> > > > --- a/drivers/tee/Makefile
> > > > +++ b/drivers/tee/Makefile
> > > > @@ -3,3 +3,4 @@
> > > >  obj-y += tee-uclass.o
> > > >  obj-$(CONFIG_SANDBOX) += sandbox.o
> > > >  obj-$(CONFIG_OPTEE) += optee/
> > > > +obj-y += broadcom/
> > > > diff --git a/drivers/tee/broadcom/Kconfig b/drivers/tee/broadcom/Kconfig
> > > > new file mode 100644
> > > > index 0000000000..ce95072d4e
> > > > --- /dev/null
> > > > +++ b/drivers/tee/broadcom/Kconfig
> > > > @@ -0,0 +1,7 @@
> > > > +config CHIMP_OPTEE
> > > > +       bool "Enable secure ChiMP firmware loading"
> > > > +       depends on OPTEE
> > > > +       default y
> > > > +       help
> > > > +         This driver is used to load bnxt firmware binary using OpTEE.
> > > > +         bnxt is Broadcom NetXtreme controller Ethernet card.
> > > > diff --git a/drivers/tee/broadcom/Makefile b/drivers/tee/broadcom/Makefile
> > > > new file mode 100644
> > > > index 0000000000..cb3cef16df
> > > > --- /dev/null
> > > > +++ b/drivers/tee/broadcom/Makefile
> > > > @@ -0,0 +1,3 @@
> > > > +# SPDX-License-Identifier: GPL-2.0+
> > > > +
> > > > +obj-y += chimp_optee.o
> > > > diff --git a/drivers/tee/broadcom/chimp_optee.c b/drivers/tee/broadcom/chimp_optee.c
> > > > new file mode 100644
> > > > index 0000000000..4ca1b59c6a
> > > > --- /dev/null
> > > > +++ b/drivers/tee/broadcom/chimp_optee.c
> > > > @@ -0,0 +1,182 @@
> > > > +// SPDX-License-Identifier: BSD-2-Clause
> > > > +/*
> > > > + * Copyright 2020 Broadcom.
> > > > + */
> > > > +
> > > > +#include <common.h>
> > > > +#include <tee.h>
> > > > +#include <broadcom/chimp.h>
> > > > +
> > > > +#ifdef CONFIG_CHIMP_OPTEE
> > > > +
> > > > +#define CHMIP_BOOT_UUID { 0x6272636D, 0x2019, 0x0716, \
> > > > +                  { 0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49 } }
> > > > +
> > > > +enum {
> > > > +       TEE_CHIMP_FASTBOOT = 0,
> > > > +       TEE_CHIMP_HEALTH_STATUS,
> > > > +       TEE_CHIMP_HANDSHAKE_STATUS,
> > > > +} tee_chmip_cmd;
> > > > +
> > > > +struct bcm_chimp_data {
> > > > +       struct udevice *tee;
> > > > +       u32 session;
> > > > +} chimp_data;
> > > > +
> > > > +static int get_open_session(struct bcm_chimp_data *b_data)
> > > > +{
> > > > +       struct udevice *tee = NULL;
> > > > +
> > > > +       while (!b_data->tee) {
> > > > +               const struct tee_optee_ta_uuid uuid = CHMIP_BOOT_UUID;
> > > > +               struct tee_open_session_arg arg;
> > > > +               int rc;
> > > > +
> > > > +               tee = tee_find_device(tee, NULL, NULL, NULL);
> > > > +               if (!tee)
> > > > +                       return -ENODEV;
> > > > +
> > > > +               memset(&arg, 0, sizeof(arg));
> > > > +               tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
> > > > +               rc = tee_open_session(tee, &arg, 0, NULL);
> > > > +               if (!rc) {
> > > > +                       b_data->tee = tee;
> > > > +                       b_data->session = arg.session;
> > > > +               }
> > > > +       }
> > > > +
> > >
> > > Is this looping forever? Should you return -ETIMEDOUT using
> > > get_timer() to check this?
> >
> > Its not an infinite loop.
> > Within the loop we are looking for our device.
> > Once all device iteration is over loop will exit or if we find our
> > device it will exit.
>
> What doesn't it find it first time? What happens in the system that
> stops that from working?
>
> I think you should:
> - move tee_find_device() out of the loop
> - add a comment as to why tee_open_session() takes a while to work
> - add a timeout if the expected time is exceeded

Thank you for your detailed review, loop is not required. Will modify
code and push new patch.

Regards,
Rayagonda

>
> Regards,
> Simon
>
>
> Regards,
> Simomn