[PATCH v3 0/2] add optee support for broadcom NS3 soc

[PATCH v3 0/2] add optee support for broadcom NS3 soc

[Rayagonda Kokatanur]

This is fourth patch set series prepared on top of third
patch set ("add FIT image support for broadcom NS3 soc").

This patch adds optee support.

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.

 -Address slef review comments,
  Remove optee dt node as this file is no longer in sync with linux.

Changes from v1:
 -Address review comments from Thomas Fitzsimmons,
  Expand the bnxt full form.

 -Address review comments from Simon Glass,
  Move c file from board/broadcom/bcmns3/chimp_optee.c to 
  drivers/tee/broadcom,
  Move header file from include/brcm/chimp.h to include/broadcom/chimp.h 

Vikas Gupta (2):
  drivers: tee: broadcom: add optee based bnxt fw load driver
  configs: ns3: enable tee and optee driver

 configs/bcm_ns3_defconfig          |   5 +-
 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 +++
 7 files changed, 214 insertions(+), 1 deletion(-)
 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

-- 
2.17.1

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

[Rayagonda Kokatanur]

From: Vikas Gupta <vikas.gupta@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@broadcom.com>
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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;
+		}
+	}
+
+	return 0;
+}
+
+static int init_arg(struct tee_invoke_arg *arg, u32 func)
+{
+	if (get_open_session(&chimp_data))
+		return -EINVAL;
+
+	memset(arg, 0, sizeof(struct tee_invoke_arg));
+	arg->func = func;
+	arg->session = chimp_data.session;
+
+	return 0;
+}
+
+int chimp_handshake_status_optee(u32 timeout, u32 *hs)
+{
+	struct tee_invoke_arg arg;
+	struct tee_param param[1];
+	int ret;
+
+	ret = init_arg(&arg, TEE_CHIMP_HANDSHAKE_STATUS);
+	if (ret < 0)
+		return ret;
+
+	param[0].attr = TEE_PARAM_ATTR_TYPE_VALUE_INOUT;
+	param[0].u.value.a = timeout;
+
+	ret = tee_invoke_func(chimp_data.tee, &arg, ARRAY_SIZE(param), param);
+	if (ret < 0) {
+		printf("Handshake status command failed\n");
+		goto out;
+	}
+
+	switch (arg.ret) {
+	case TEE_SUCCESS:
+		*hs = param[0].u.value.a;
+		ret =  0;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out:
+	tee_close_session(chimp_data.tee, chimp_data.session);
+	chimp_data.tee = NULL;
+
+	return ret;
+}
+
+int chimp_health_status_optee(u32 *health)
+{
+	struct tee_invoke_arg arg;
+	struct tee_param param[1];
+	int ret;
+
+	ret = init_arg(&arg, TEE_CHIMP_HEALTH_STATUS);
+	if (ret < 0)
+		return ret;
+
+	param[0].attr = TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+
+	ret = tee_invoke_func(chimp_data.tee, &arg, ARRAY_SIZE(param), param);
+	if (ret < 0) {
+		printf("Helath status command failed\n");
+		goto out;
+	}
+
+	switch (arg.ret) {
+	case TEE_SUCCESS:
+		*health = param[0].u.value.a;
+		ret =  0;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out:
+	tee_close_session(chimp_data.tee, chimp_data.session);
+	chimp_data.tee = NULL;
+
+	return ret;
+}
+
+int chimp_fastboot_optee(void)
+{
+	struct tee_invoke_arg arg;
+	int ret;
+
+	ret = init_arg(&arg, TEE_CHIMP_FASTBOOT);
+	if (ret < 0)
+		return ret;
+
+	ret = tee_invoke_func(chimp_data.tee, &arg, 0, NULL);
+	if (ret < 0) {
+		printf("Chimp boot_fail\n");
+		goto out;
+	}
+
+	switch (arg.ret) {
+	case TEE_SUCCESS:
+		ret = 0;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out:
+	tee_close_session(chimp_data.tee, chimp_data.session);
+	chimp_data.tee = NULL;
+
+	return ret;
+}
+#else
+int chimp_handshake_status_optee(u32 timeout, u32 *status)
+{
+	printf("ChiMP handshake status fail (OPTEE not enabled)\n");
+	return -EINVAL;
+}
+
+int chimp_health_status_optee(u32 *status)
+{
+	printf("ChiMP health status fail (OPTEE not enabled)\n");
+	return -EINVAL;
+}
+
+int chimp_fastboot_optee(void)
+{
+	printf("ChiMP secure boot fail (OPTEE not enabled)\n");
+	return -EINVAL;
+}
+#endif /* CONFIG_CHIMP_OPTEE */
diff --git a/include/broadcom/chimp.h b/include/broadcom/chimp.h
new file mode 100644
index 0000000000..2397e13da0
--- /dev/null
+++ b/include/broadcom/chimp.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2020 Broadcom.
+ *
+ */
+
+#ifndef __CHIMP_H__
+#define __CHIMP_H__
+
+#include <linux/compiler.h>
+
+int chimp_fastboot_optee(void);
+int chimp_health_status_optee(u32 *status);
+int chimp_handshake_status_optee(u32 timeout, u32 *hstatus);
+
+#endif
-- 
2.17.1

[PATCH v3 2/2] configs: ns3: enable tee and optee driver

[Rayagonda Kokatanur]

From: Vikas Gupta <vikas.gupta@broadcom.com>

Enable tee and optee drivers.

Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 configs/bcm_ns3_defconfig | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/configs/bcm_ns3_defconfig b/configs/bcm_ns3_defconfig
index b52c6d7d93..13fe9d439e 100644
--- a/configs/bcm_ns3_defconfig
+++ b/configs/bcm_ns3_defconfig
@@ -4,12 +4,12 @@ CONFIG_TARGET_BCMNS3=y
 CONFIG_SYS_TEXT_BASE=0xFF000000
 CONFIG_ENV_SIZE=0x80000
 CONFIG_NR_DRAM_BANKS=2
-CONFIG_OF_BOARD_SETUP=y
 CONFIG_FIT=y
 CONFIG_FIT_SIGNATURE=y
 CONFIG_FIT_SIGNATURE_MAX_SIZE=0x20000000
 CONFIG_FIT_VERBOSE=y
 CONFIG_LEGACY_IMAGE_FORMAT=y
+CONFIG_OF_BOARD_SETUP=y
 CONFIG_LOGLEVEL=7
 CONFIG_SILENT_CONSOLE=y
 CONFIG_SILENT_U_BOOT_ONLY=y
@@ -42,6 +42,9 @@ CONFIG_PINCTRL=y
 CONFIG_PINCTRL_SINGLE=y
 CONFIG_DM_SERIAL=y
 CONFIG_SYS_NS16550=y
+CONFIG_TEE=y
+CONFIG_OPTEE=y
+# CONFIG_OPTEE_TA_AVB is not set
 # CONFIG_WATCHDOG is not set
 CONFIG_WDT=y
 CONFIG_WDT_SP805=y
-- 
2.17.1

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

[Simon Glass]

Hi Rayagonda,

On Wed, 10 Jun 2020 at 05:15, Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> From: Vikas Gupta <vikas.gupta@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@broadcom.com>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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?

> diff --git a/include/broadcom/chimp.h b/include/broadcom/chimp.h
> new file mode 100644
> index 0000000000..2397e13da0
> --- /dev/null
> +++ b/include/broadcom/chimp.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2020 Broadcom.
> + *
> + */
> +
> +#ifndef __CHIMP_H__
> +#define __CHIMP_H__
> +
> +#include <linux/compiler.h>
> +
> +int chimp_fastboot_optee(void);
> +int chimp_health_status_optee(u32 *status);
> +int chimp_handshake_status_optee(u32 timeout, u32 *hstatus);

Please add comments for each function .

> +
> +#endif
> --
> 2.17.1
>

Regards,
Simon

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

[Rayagonda Kokatanur]

Hi Simon,

On Fri, Jun 26, 2020 at 6:42 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rayagonda,
>
> On Wed, 10 Jun 2020 at 05:15, Rayagonda Kokatanur
> <rayagonda.kokatanur@broadcom.com> wrote:
> >
> > From: Vikas Gupta <vikas.gupta@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@broadcom.com>
> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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.

>
> > diff --git a/include/broadcom/chimp.h b/include/broadcom/chimp.h
> > new file mode 100644
> > index 0000000000..2397e13da0
> > --- /dev/null
> > +++ b/include/broadcom/chimp.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2020 Broadcom.
> > + *
> > + */
> > +
> > +#ifndef __CHIMP_H__
> > +#define __CHIMP_H__
> > +
> > +#include <linux/compiler.h>
> > +
> > +int chimp_fastboot_optee(void);
> > +int chimp_health_status_optee(u32 *status);
> > +int chimp_handshake_status_optee(u32 timeout, u32 *hstatus);
>
> Please add comments for each function .

Thank you, will add comments.

Regards,
Rayagonda

>
> > +
> > +#endif
> > --
> > 2.17.1
> >
>
> Regards,
> Simon

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

[Simon Glass]

Hi Rayagonda,

On Sun, 28 Jun 2020 at 21:08, Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> Hi Simon,
>
> On Fri, Jun 26, 2020 at 6:42 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Rayagonda,
> >
> > On Wed, 10 Jun 2020 at 05:15, Rayagonda Kokatanur
> > <rayagonda.kokatanur@broadcom.com> wrote:
> > >
> > > From: Vikas Gupta <vikas.gupta@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@broadcom.com>
> > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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

Regards,
Simon


Regards,
Simomn

[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@chromium.org> wrote:
>
> Hi Rayagonda,
>
> On Sun, 28 Jun 2020 at 21:08, Rayagonda Kokatanur
> <rayagonda.kokatanur@broadcom.com> wrote:
> >
> > Hi Simon,
> >
> > On Fri, Jun 26, 2020 at 6:42 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Rayagonda,
> > >
> > > On Wed, 10 Jun 2020 at 05:15, Rayagonda Kokatanur
> > > <rayagonda.kokatanur@broadcom.com> wrote:
> > > >
> > > > From: Vikas Gupta <vikas.gupta@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@broadcom.com>
> > > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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