diff mbox series

[v5,01/16] crypto/fsl: Add support for CAAM Job ring driver model

Message ID 20211115070014.17586-2-gaurav.jain@nxp.com
State Changes Requested
Delegated to: Stefano Babic
Headers show
Series Add CAAM driver model support | expand

Commit Message

Gaurav Jain Nov. 15, 2021, 6:59 a.m. UTC 
added device tree support for job ring driver.
sec is initialized based on job ring information processed
from device tree.

Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
Reviewed-by: Ye Li <ye.li@nxp.com>
---
 cmd/Kconfig                 |   1 +
 drivers/crypto/fsl/Kconfig  |   7 +
 drivers/crypto/fsl/Makefile |   4 +-
 drivers/crypto/fsl/jr.c     | 316 +++++++++++++++++++++++-------------
 drivers/crypto/fsl/jr.h     |  14 ++
 5 files changed, 232 insertions(+), 110 deletions(-)

Comments

Michael Walle Nov. 16, 2021, 11:01 a.m. UTC | #1 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5b30b13e43..2b24672505 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2009,6 +2009,7 @@ config CMD_AES
>  
>  config CMD_BLOB
>  	bool "Enable the 'blob' command"
> +	select FSL_BLOB

this looks wrong, because CMD_BLOB sounds like a generic command but it
will automatically select FSL_BLOB which in turn sounds freescale specific.
Looking at the help text, this command is (at least at the moment) freescale
specific, but the code seems to be generic and the blob_encap() and
blob_decap() are weak functions, thus they could be implemented in a
different way and not just by fsl_blob.c.

I don't think this should automatically select FSL_BLOB.

Also, shouldn't this be an uclass with encap and decap ops?

>  	depends on !MX6ULL && !MX6SLL && !MX6SL
>  	select IMX_HAB if ARCH_MX6 || ARCH_MX7 || ARCH_MX7ULP || ARCH_IMX8M
>  	help
> diff --git a/drivers/crypto/fsl/Kconfig b/drivers/crypto/fsl/Kconfig
> index 94ff540111..ab59d516f8 100644
> --- a/drivers/crypto/fsl/Kconfig
> +++ b/drivers/crypto/fsl/Kconfig
> @@ -66,4 +66,11 @@ config FSL_CAAM_RNG
>  	  using the prediction resistance flag which means the DRGB is
>  	  reseeded from the TRNG every time random data is generated.
>  
> +config FSL_BLOB
> +        bool "Enable Blob Encap/Decap, Blob KEK support"

wrong indendation?

> +	help
> +	  Enable support for the hardware based crytographic blob encap/decap
> +	  module of the CAAM. blobs can be safely placed into non-volatile
> +	  storage. blobs can only be decapsulated by the SoC that created it.
> +	  Enable support for blob key encryption key generation.
>  endif
ZHIZHIKIN Andrey Nov. 16, 2021, 3:54 p.m. UTC | #2 
Hello Gaurav,

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Gaurav Jain
> Sent: Monday, November 15, 2021 8:00 AM
> To: u-boot@lists.denx.de
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; Peng Fan
> <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka Jain
> <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> <franck.lenormand@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil
> malhotra <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> Sethi <V.Sethi@nxp.com>; NXP i . MX U-Boot Team <uboot-imx@nxp.com>; Shengzhou
> Liu <Shengzhou.Liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Rajesh Bhagat
> <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim
> Khan <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> <pramod.kumar_1@nxp.com>; Tang Yuantian <andy.tang@nxp.com>; Adrian Alonso
> <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>; Gaurav Jain
> <gaurav.jain@nxp.com>
> Subject: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring driver model
> 
> 
> added device tree support for job ring driver.
> sec is initialized based on job ring information processed
> from device tree.
> 
> Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> Reviewed-by: Ye Li <ye.li@nxp.com>
> ---
>  cmd/Kconfig                 |   1 +
>  drivers/crypto/fsl/Kconfig  |   7 +
>  drivers/crypto/fsl/Makefile |   4 +-
>  drivers/crypto/fsl/jr.c     | 316 +++++++++++++++++++++++-------------
>  drivers/crypto/fsl/jr.h     |  14 ++
>  5 files changed, 232 insertions(+), 110 deletions(-)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5b30b13e43..2b24672505 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2009,6 +2009,7 @@ config CMD_AES
> 
>  config CMD_BLOB
>         bool "Enable the 'blob' command"
> +       select FSL_BLOB
>         depends on !MX6ULL && !MX6SLL && !MX6SL
>         select IMX_HAB if ARCH_MX6 || ARCH_MX7 || ARCH_MX7ULP || ARCH_IMX8M
>         help
> diff --git a/drivers/crypto/fsl/Kconfig b/drivers/crypto/fsl/Kconfig
> index 94ff540111..ab59d516f8 100644
> --- a/drivers/crypto/fsl/Kconfig
> +++ b/drivers/crypto/fsl/Kconfig
> @@ -66,4 +66,11 @@ config FSL_CAAM_RNG
>           using the prediction resistance flag which means the DRGB is
>           reseeded from the TRNG every time random data is generated.
> 
> +config FSL_BLOB
> +        bool "Enable Blob Encap/Decap, Blob KEK support"
> +       help
> +         Enable support for the hardware based crytographic blob encap/decap
> +         module of the CAAM. blobs can be safely placed into non-volatile
> +         storage. blobs can only be decapsulated by the SoC that created it.
> +         Enable support for blob key encryption key generation.
>  endif
> diff --git a/drivers/crypto/fsl/Makefile b/drivers/crypto/fsl/Makefile
> index f9c3ccecfc..738535b8e4 100644
> --- a/drivers/crypto/fsl/Makefile
> +++ b/drivers/crypto/fsl/Makefile
> @@ -1,10 +1,12 @@
>  # SPDX-License-Identifier: GPL-2.0+
>  #
>  # Copyright 2014 Freescale Semiconductor, Inc.
> +# Copyright 2021 NXP
> 
>  obj-y += sec.o
>  obj-$(CONFIG_FSL_CAAM) += jr.o fsl_hash.o jobdesc.o error.o
> -obj-$(CONFIG_CMD_BLOB)$(CONFIG_IMX_CAAM_DEK_ENCAP) += fsl_blob.o
> +obj-$(CONFIG_FSL_BLOB) += fsl_blob.o
> +obj-$(CONFIG_IMX_CAAM_DEK_ENCAP) += fsl_blob.o
>  obj-$(CONFIG_RSA_FREESCALE_EXP) += fsl_rsa.o
>  obj-$(CONFIG_FSL_CAAM_RNG) += rng.o
>  obj-$(CONFIG_FSL_MFGPROT) += fsl_mfgprot.o
> diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
> index 22b649219e..eea2225a1e 100644
> --- a/drivers/crypto/fsl/jr.c
> +++ b/drivers/crypto/fsl/jr.c
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
>   * Copyright 2008-2014 Freescale Semiconductor, Inc.
> - * Copyright 2018 NXP
> + * Copyright 2018, 2021 NXP
>   *
>   * Based on CAAM driver in drivers/crypto/caam in Linux
>   */
> @@ -11,7 +11,6 @@
>  #include <linux/kernel.h>
>  #include <log.h>
>  #include <malloc.h>
> -#include "fsl_sec.h"
>  #include "jr.h"
>  #include "jobdesc.h"
>  #include "desc_constr.h"
> @@ -21,8 +20,11 @@
>  #include <asm/cache.h>
>  #include <asm/fsl_pamu.h>
>  #endif
> +#include <dm.h>
>  #include <dm/lists.h>
>  #include <linux/delay.h>
> +#include <dm/root.h>
> +#include <dm/device-internal.h>
> 
>  #define CIRC_CNT(head, tail, size)     (((head) - (tail)) & (size - 1))
>  #define CIRC_SPACE(head, tail, size)   CIRC_CNT((tail), (head) + 1, (size))
> @@ -35,20 +37,30 @@ uint32_t sec_offset[CONFIG_SYS_FSL_MAX_NUM_OF_SEC] = {
>  #endif
>  };
> 
> +#if CONFIG_IS_ENABLED(DM)
> +struct udevice *caam_dev;
> +#else
>  #define SEC_ADDR(idx)  \
>         (ulong)((CONFIG_SYS_FSL_SEC_ADDR + sec_offset[idx]))
> 
>  #define SEC_JR0_ADDR(idx)      \
>         (ulong)(SEC_ADDR(idx) + \
>          (CONFIG_SYS_FSL_JR0_OFFSET - CONFIG_SYS_FSL_SEC_OFFSET))
> +struct caam_regs caam_st;
> +#endif
> 
> -struct jobring jr0[CONFIG_SYS_FSL_MAX_NUM_OF_SEC];
> +static inline u32 jr_start_reg(u8 jrid)
> +{
> +       return (1 << jrid);
> +}
> 
> -static inline void start_jr0(uint8_t sec_idx)
> +#ifndef CONFIG_ARCH_IMX8
> +static inline void start_jr(struct caam_regs *caam)
>  {
> -       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
> +       ccsr_sec_t *sec = caam->sec;
>         u32 ctpr_ms = sec_in32(&sec->ctpr_ms);
>         u32 scfgr = sec_in32(&sec->scfgr);
> +       u32 jrstart = jr_start_reg(caam->jrid);
> 
>         if (ctpr_ms & SEC_CTPR_MS_VIRT_EN_INCL) {
>                 /* VIRT_EN_INCL = 1 & VIRT_EN_POR = 1 or
> @@ -56,23 +68,17 @@ static inline void start_jr0(uint8_t sec_idx)
>                  */
>                 if ((ctpr_ms & SEC_CTPR_MS_VIRT_EN_POR) ||
>                     (scfgr & SEC_SCFGR_VIRT_EN))
> -                       sec_out32(&sec->jrstartr, CONFIG_JRSTARTR_JR0);
> +                       sec_out32(&sec->jrstartr, jrstart);
>         } else {
>                 /* VIRT_EN_INCL = 0 && VIRT_EN_POR_VALUE = 1 */
>                 if (ctpr_ms & SEC_CTPR_MS_VIRT_EN_POR)
> -                       sec_out32(&sec->jrstartr, CONFIG_JRSTARTR_JR0);
> +                       sec_out32(&sec->jrstartr, jrstart);
>         }
>  }
> +#endif
> 
> -static inline void jr_reset_liodn(uint8_t sec_idx)
> -{
> -       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
> -       sec_out32(&sec->jrliodnr[0].ls, 0);
> -}
> -
> -static inline void jr_disable_irq(uint8_t sec_idx)
> +static inline void jr_disable_irq(struct jr_regs *regs)
>  {
> -       struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
>         uint32_t jrcfg = sec_in32(&regs->jrcfg1);
> 
>         jrcfg = jrcfg | JR_INTMASK;
> @@ -80,10 +86,10 @@ static inline void jr_disable_irq(uint8_t sec_idx)
>         sec_out32(&regs->jrcfg1, jrcfg);
>  }
> 
> -static void jr_initregs(uint8_t sec_idx)
> +static void jr_initregs(uint8_t sec_idx, struct caam_regs *caam)
>  {
> -       struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> -       struct jobring *jr = &jr0[sec_idx];
> +       struct jr_regs *regs = caam->regs;
> +       struct jobring *jr = &caam->jr[sec_idx];
>         caam_dma_addr_t ip_base = virt_to_phys((void *)jr->input_ring);
>         caam_dma_addr_t op_base = virt_to_phys((void *)jr->output_ring);
> 
> @@ -103,16 +109,16 @@ static void jr_initregs(uint8_t sec_idx)
>         sec_out32(&regs->irs, JR_SIZE);
> 
>         if (!jr->irq)
> -               jr_disable_irq(sec_idx);
> +               jr_disable_irq(regs);
>  }
> 
> -static int jr_init(uint8_t sec_idx)
> +static int jr_init(uint8_t sec_idx, struct caam_regs *caam)
>  {
> -       struct jobring *jr = &jr0[sec_idx];
> +       struct jobring *jr = &caam->jr[sec_idx];
> 
>         memset(jr, 0, sizeof(struct jobring));
> 
> -       jr->jq_id = DEFAULT_JR_ID;
> +       jr->jq_id = caam->jrid;
>         jr->irq = DEFAULT_IRQ;
> 
>  #ifdef CONFIG_FSL_CORENET
> @@ -134,53 +140,10 @@ static int jr_init(uint8_t sec_idx)
>         memset(jr->input_ring, 0, JR_SIZE * sizeof(caam_dma_addr_t));
>         memset(jr->output_ring, 0, jr->op_size);
> 
> -       start_jr0(sec_idx);
> -
> -       jr_initregs(sec_idx);
> -
> -       return 0;
> -}
> -
> -static int jr_sw_cleanup(uint8_t sec_idx)
> -{
> -       struct jobring *jr = &jr0[sec_idx];
> -
> -       jr->head = 0;
> -       jr->tail = 0;
> -       jr->read_idx = 0;
> -       jr->write_idx = 0;
> -       memset(jr->info, 0, sizeof(jr->info));
> -       memset(jr->input_ring, 0, jr->size * sizeof(caam_dma_addr_t));
> -       memset(jr->output_ring, 0, jr->size * sizeof(struct op_ring));
> -
> -       return 0;
> -}
> -
> -static int jr_hw_reset(uint8_t sec_idx)
> -{
> -       struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> -       uint32_t timeout = 100000;
> -       uint32_t jrint, jrcr;
> -
> -       sec_out32(&regs->jrcr, JRCR_RESET);
> -       do {
> -               jrint = sec_in32(&regs->jrint);
> -       } while (((jrint & JRINT_ERR_HALT_MASK) ==
> -                 JRINT_ERR_HALT_INPROGRESS) && --timeout);
> -
> -       jrint = sec_in32(&regs->jrint);
> -       if (((jrint & JRINT_ERR_HALT_MASK) !=
> -            JRINT_ERR_HALT_INPROGRESS) && timeout == 0)
> -               return -1;
> -
> -       timeout = 100000;
> -       sec_out32(&regs->jrcr, JRCR_RESET);
> -       do {
> -               jrcr = sec_in32(&regs->jrcr);
> -       } while ((jrcr & JRCR_RESET) && --timeout);
> -
> -       if (timeout == 0)
> -               return -1;
> +#ifndef CONFIG_ARCH_IMX8
> +       start_jr(caam);
> +#endif
> +       jr_initregs(sec_idx, caam);
> 
>         return 0;
>  }
> @@ -188,10 +151,10 @@ static int jr_hw_reset(uint8_t sec_idx)
>  /* -1 --- error, can't enqueue -- no space available */
>  static int jr_enqueue(uint32_t *desc_addr,
>                void (*callback)(uint32_t status, void *arg),
> -              void *arg, uint8_t sec_idx)
> +              void *arg, uint8_t sec_idx, struct caam_regs *caam)
>  {
> -       struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> -       struct jobring *jr = &jr0[sec_idx];
> +       struct jr_regs *regs = caam->regs;
> +       struct jobring *jr = &caam->jr[sec_idx];
>         int head = jr->head;
>         uint32_t desc_word;
>         int length = desc_len(desc_addr);
> @@ -263,10 +226,10 @@ static int jr_enqueue(uint32_t *desc_addr,
>         return 0;
>  }
> 
> -static int jr_dequeue(int sec_idx)
> +static int jr_dequeue(int sec_idx, struct caam_regs *caam)
>  {
> -       struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> -       struct jobring *jr = &jr0[sec_idx];
> +       struct jr_regs *regs = caam->regs;
> +       struct jobring *jr = &caam->jr[sec_idx];
>         int head = jr->head;
>         int tail = jr->tail;
>         int idx, i, found;
> @@ -349,14 +312,18 @@ static void desc_done(uint32_t status, void *arg)
>  {
>         struct result *x = arg;
>         x->status = status;
> -#ifndef CONFIG_SPL_BUILD
>         caam_jr_strstatus(status);
> -#endif
>         x->done = 1;
>  }
> 
>  static inline int run_descriptor_jr_idx(uint32_t *desc, uint8_t sec_idx)
>  {
> +       struct caam_regs *caam;
> +#if CONFIG_IS_ENABLED(DM)
> +       caam = dev_get_priv(caam_dev);
> +#else
> +       caam = &caam_st;
> +#endif
>         unsigned long long timeval = 0;
>         unsigned long long timeout = CONFIG_USEC_DEQ_TIMEOUT;
>         struct result op;
> @@ -364,7 +331,7 @@ static inline int run_descriptor_jr_idx(uint32_t *desc,
> uint8_t sec_idx)
> 
>         memset(&op, 0, sizeof(op));
> 
> -       ret = jr_enqueue(desc, desc_done, &op, sec_idx);
> +       ret = jr_enqueue(desc, desc_done, &op, sec_idx, caam);
>         if (ret) {
>                 debug("Error in SEC enq\n");
>                 ret = JQ_ENQ_ERR;
> @@ -375,7 +342,7 @@ static inline int run_descriptor_jr_idx(uint32_t *desc,
> uint8_t sec_idx)
>                 udelay(1);
>                 timeval += 1;
> 
> -               ret = jr_dequeue(sec_idx);
> +               ret = jr_dequeue(sec_idx, caam);
>                 if (ret) {
>                         debug("Error in SEC deq\n");
>                         ret = JQ_DEQ_ERR;
> @@ -402,13 +369,63 @@ int run_descriptor_jr(uint32_t *desc)
>         return run_descriptor_jr_idx(desc, 0);
>  }
> 
> +#ifndef CONFIG_ARCH_IMX8
> +static int jr_sw_cleanup(uint8_t sec_idx, struct caam_regs *caam)
> +{
> +       struct jobring *jr = &caam->jr[sec_idx];
> +
> +       jr->head = 0;
> +       jr->tail = 0;
> +       jr->read_idx = 0;
> +       jr->write_idx = 0;
> +       memset(jr->info, 0, sizeof(jr->info));
> +       memset(jr->input_ring, 0, jr->size * sizeof(caam_dma_addr_t));
> +       memset(jr->output_ring, 0, jr->size * sizeof(struct op_ring));
> +
> +       return 0;
> +}
> +
> +static int jr_hw_reset(struct jr_regs *regs)
> +{
> +       uint32_t timeout = 100000;
> +       uint32_t jrint, jrcr;
> +
> +       sec_out32(&regs->jrcr, JRCR_RESET);
> +       do {
> +               jrint = sec_in32(&regs->jrint);
> +       } while (((jrint & JRINT_ERR_HALT_MASK) ==
> +                 JRINT_ERR_HALT_INPROGRESS) && --timeout);
> +
> +       jrint = sec_in32(&regs->jrint);
> +       if (((jrint & JRINT_ERR_HALT_MASK) !=
> +            JRINT_ERR_HALT_INPROGRESS) && timeout == 0)
> +               return -1;
> +
> +       timeout = 100000;
> +       sec_out32(&regs->jrcr, JRCR_RESET);
> +       do {
> +               jrcr = sec_in32(&regs->jrcr);
> +       } while ((jrcr & JRCR_RESET) && --timeout);
> +
> +       if (timeout == 0)
> +               return -1;
> +
> +       return 0;
> +}
> +
>  static inline int jr_reset_sec(uint8_t sec_idx)
>  {
> -       if (jr_hw_reset(sec_idx) < 0)
> +       struct caam_regs *caam;
> +#if CONFIG_IS_ENABLED(DM)
> +       caam = dev_get_priv(caam_dev);
> +#else
> +       caam = &caam_st;
> +#endif
> +       if (jr_hw_reset(caam->regs) < 0)
>                 return -1;
> 
>         /* Clean up the jobring structure maintained by software */
> -       jr_sw_cleanup(sec_idx);
> +       jr_sw_cleanup(sec_idx, caam);
> 
>         return 0;
>  }
> @@ -418,9 +435,15 @@ int jr_reset(void)
>         return jr_reset_sec(0);
>  }
> 
> -static inline int sec_reset_idx(uint8_t sec_idx)
> +int sec_reset(void)
>  {
> -       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
> +       struct caam_regs *caam;
> +#if CONFIG_IS_ENABLED(DM)
> +       caam = dev_get_priv(caam_dev);
> +#else
> +       caam = &caam_st;
> +#endif
> +       ccsr_sec_t *sec = caam->sec;
>         uint32_t mcfgr = sec_in32(&sec->mcfgr);
>         uint32_t timeout = 100000;
> 
> @@ -446,11 +469,7 @@ static inline int sec_reset_idx(uint8_t sec_idx)
> 
>         return 0;
>  }
> -int sec_reset(void)
> -{
> -       return sec_reset_idx(0);
> -}
> -#ifndef CONFIG_SPL_BUILD
> +
>  static int deinstantiate_rng(u8 sec_idx, int state_handle_mask)
>  {
>         u32 *desc;
> @@ -496,12 +515,11 @@ static int deinstantiate_rng(u8 sec_idx, int
> state_handle_mask)
>         return ret;
>  }
> 
> -static int instantiate_rng(u8 sec_idx, int gen_sk)
> +static int instantiate_rng(uint8_t sec_idx, ccsr_sec_t *sec, int gen_sk)
>  {
>         u32 *desc;
>         u32 rdsta_val;
>         int ret = 0, sh_idx, size;
> -       ccsr_sec_t __iomem *sec = (ccsr_sec_t __iomem *)SEC_ADDR(sec_idx);
>         struct rng4tst __iomem *rng =
>                         (struct rng4tst __iomem *)&sec->rng;
> 
> @@ -554,9 +572,8 @@ static int instantiate_rng(u8 sec_idx, int gen_sk)
>         return ret;
>  }
> 
> -static u8 get_rng_vid(uint8_t sec_idx)
> +static u8 get_rng_vid(ccsr_sec_t *sec)
>  {
> -       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
>         u8 vid;
> 
>         if (caam_get_era() < 10) {
> @@ -574,9 +591,8 @@ static u8 get_rng_vid(uint8_t sec_idx)
>   * By default, the TRNG runs for 200 clocks per sample;
>   * 1200 clocks per sample generates better entropy.
>   */
> -static void kick_trng(int ent_delay, uint8_t sec_idx)
> +static void kick_trng(int ent_delay, ccsr_sec_t *sec)
>  {
> -       ccsr_sec_t __iomem *sec = (ccsr_sec_t __iomem *)SEC_ADDR(sec_idx);
>         struct rng4tst __iomem *rng =
>                         (struct rng4tst __iomem *)&sec->rng;
>         u32 val;
> @@ -603,10 +619,9 @@ static void kick_trng(int ent_delay, uint8_t sec_idx)
>         sec_clrbits32(&rng->rtmctl, RTMCTL_PRGM);
>  }
> 
> -static int rng_init(uint8_t sec_idx)
> +static int rng_init(uint8_t sec_idx, ccsr_sec_t *sec)
>  {
>         int ret, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
> -       ccsr_sec_t __iomem *sec = (ccsr_sec_t __iomem *)SEC_ADDR(sec_idx);
>         struct rng4tst __iomem *rng =
>                         (struct rng4tst __iomem *)&sec->rng;
>         u32 inst_handles;
> @@ -624,7 +639,7 @@ static int rng_init(uint8_t sec_idx)
>                  * the TRNG parameters.
>                  */
>                 if (!inst_handles) {
> -                       kick_trng(ent_delay, sec_idx);
> +                       kick_trng(ent_delay, sec);
>                         ent_delay += 400;
>                 }
>                 /*
> @@ -634,7 +649,7 @@ static int rng_init(uint8_t sec_idx)
>                  * interval, leading to a sucessful initialization of
>                  * the RNG.
>                  */
> -               ret = instantiate_rng(sec_idx, gen_sk);
> +               ret = instantiate_rng(sec_idx, sec, gen_sk);
>         } while ((ret == -1) && (ent_delay < RTSDCTL_ENT_DLY_MAX));
>         if (ret) {
>                 printf("SEC%u:  Failed to instantiate RNG\n", sec_idx);
> @@ -647,12 +662,29 @@ static int rng_init(uint8_t sec_idx)
>         return ret;
>  }
>  #endif
> +
>  int sec_init_idx(uint8_t sec_idx)
>  {
> -       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
> -       uint32_t mcr = sec_in32(&sec->mcfgr);
>         int ret = 0;
> -
> +       struct caam_regs *caam;
> +#if CONFIG_IS_ENABLED(DM)
> +       if (caam_dev == NULL) {
> +               printf("caam_jr: caam not found\n");
> +               return -1;
> +       }
> +       caam = dev_get_priv(caam_dev);
> +#else
> +       caam_st.sec = (void *)SEC_ADDR(sec_idx);
> +       caam_st.regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> +       caam_st.jrid = 0;
> +       caam = &caam_st;
> +#endif
> +#ifndef CONFIG_ARCH_IMX8
> +       ccsr_sec_t *sec = caam->sec;
> +       uint32_t mcr = sec_in32(&sec->mcfgr);
> +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
> +       uint32_t jrdid_ms = 0;
> +#endif
>  #ifdef CONFIG_FSL_CORENET
>         uint32_t liodnr;
>         uint32_t liodn_ns;
> @@ -682,6 +714,11 @@ int sec_init_idx(uint8_t sec_idx)
>         mcr |= (1 << MCFGR_PS_SHIFT);
>  #endif
>         sec_out32(&sec->mcfgr, mcr);
> +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)

This would effectively reserve the JR0 on _all_ i.MX8M derivatives is S World.

Current implementation only has JR0 reserved in S World on imx8mm derivative,
but this new addition extends this to imx8mn, imx8mp and imx8mq.

I'm wondering about several points here:
1. Why does current implementation on have this reservation done on imx8mm and
   where does this happen? None of the code pieces suggests that it is done in
   U-Boot, is it performed in BootROM?
2. What is the intention of having JR0 reserved for all derivatives? Is this
   the part of a bigger change that stretches across different SW components
   (e.g. ATF, OP-TEE, etc.)? If that is the case - then a more detailed
   description would be appreciated here.

ATF code already accounts for this reservation in commit:
a83a7c65e ("TEE-639 plat: imx8m: Do not release JR0 to NS if HAB is using it") [1],
but there is no description on why is this required though.

If this is required for HAB feature, then the question is: should it be kept in
S World when U-Boot starts, or SPL can release it after the binary is verified
and crypto facilities are not in use anymore?

> +       jrdid_ms = JRDID_MS_TZ_OWN | JRDID_MS_PRIM_TZ | JRDID_MS_PRIM_DID;

What is the intention of setting JRDID_MS_PRIM_TZ? Isn't setting JRDID_MS_TZ_OWN
would be sufficient here?

> +       sec_out32(&sec->jrliodnr[caam->jrid].ms, jrdid_ms);
> +#endif
> +       jr_reset();
> 
>  #ifdef CONFIG_FSL_CORENET
>  #ifdef CONFIG_SPL_BUILD
> @@ -693,25 +730,26 @@ int sec_init_idx(uint8_t sec_idx)
>         liodn_ns = CONFIG_SPL_JR0_LIODN_NS & JRNSLIODN_MASK;
>         liodn_s = CONFIG_SPL_JR0_LIODN_S & JRSLIODN_MASK;
> 
> -       liodnr = sec_in32(&sec->jrliodnr[0].ls) &
> +       liodnr = sec_in32(&sec->jrliodnr[caam->jrid].ls) &
>                  ~(JRNSLIODN_MASK | JRSLIODN_MASK);
>         liodnr = liodnr |
>                  (liodn_ns << JRNSLIODN_SHIFT) |
>                  (liodn_s << JRSLIODN_SHIFT);
> -       sec_out32(&sec->jrliodnr[0].ls, liodnr);
> +       sec_out32(&sec->jrliodnr[caam->jrid].ls, liodnr);
>  #else
> -       liodnr = sec_in32(&sec->jrliodnr[0].ls);
> +       liodnr = sec_in32(&sec->jrliodnr[caam->jrid].ls);
>         liodn_ns = (liodnr & JRNSLIODN_MASK) >> JRNSLIODN_SHIFT;
>         liodn_s = (liodnr & JRSLIODN_MASK) >> JRSLIODN_SHIFT;
>  #endif
>  #endif
> -
> -       ret = jr_init(sec_idx);
> +#endif
> +       ret = jr_init(sec_idx, caam);
>         if (ret < 0) {
>                 printf("SEC%u:  initialization failed\n", sec_idx);
>                 return -1;
>         }
> 
> +#ifndef CONFIG_ARCH_IMX8
>  #ifdef CONFIG_FSL_CORENET
>         ret = sec_config_pamu_table(liodn_ns, liodn_s);
>         if (ret < 0)
> @@ -719,9 +757,9 @@ int sec_init_idx(uint8_t sec_idx)
> 
>         pamu_enable();
>  #endif
> -#ifndef CONFIG_SPL_BUILD
> -       if (get_rng_vid(sec_idx) >= 4) {
> -               if (rng_init(sec_idx) < 0) {
> +
> +       if (get_rng_vid(caam->sec) >= 4) {
> +               if (rng_init(sec_idx, caam->sec) < 0) {
>                         printf("SEC%u:  RNG instantiation failed\n", sec_idx);
>                         return -1;
>                 }
> @@ -743,3 +781,63 @@ int sec_init(void)
>  {
>         return sec_init_idx(0);
>  }
> +
> +#if CONFIG_IS_ENABLED(DM)
> +static int caam_jr_probe(struct udevice *dev)
> +{
> +       struct caam_regs *caam = dev_get_priv(dev);
> +       fdt_addr_t addr;
> +       ofnode node;
> +       unsigned int jr_node = 0;
> +
> +       caam_dev = dev;
> +
> +       addr = dev_read_addr(dev);
> +       if (addr == FDT_ADDR_T_NONE) {
> +               printf("caam_jr: crypto not found\n");
> +               return -EINVAL;
> +       }
> +       caam->sec = (ccsr_sec_t *)(uintptr_t)addr;
> +       caam->regs = (struct jr_regs *)caam->sec;
> +
> +       /* Check for enabled job ring node */
> +       ofnode_for_each_subnode(node, dev_ofnode(dev)) {
> +               if (!ofnode_is_available(node)) {
> +                       continue;
> +               }
> +               jr_node = ofnode_read_u32_default(node, "reg", -1);
> +               if (jr_node > 0) {
> +                       caam->regs = (struct jr_regs *)((ulong)caam->sec +
> jr_node);
> +                       while (!(jr_node & 0x0F)) {
> +                               jr_node = jr_node >> 4;
> +                       }
> +                       caam->jrid = jr_node - 1;
> +                       break;
> +               }
> +       }
> +
> +       if (sec_init())
> +               printf("\nsec_init failed!\n");
> +
> +       return 0;
> +}
> +
> +static int caam_jr_bind(struct udevice *dev)
> +{
> +       return 0;
> +}
> +
> +static const struct udevice_id caam_jr_match[] = {
> +       { .compatible = "fsl,sec-v4.0" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(caam_jr) = {
> +       .name           = "caam_jr",
> +       .id             = UCLASS_MISC,
> +       .of_match       = caam_jr_match,
> +       .bind           = caam_jr_bind,
> +       .probe          = caam_jr_probe,
> +       .priv_auto      = sizeof(struct caam_regs),
> +};
> +#endif
> diff --git a/drivers/crypto/fsl/jr.h b/drivers/crypto/fsl/jr.h
> index 1047aa772c..43cb5e0753 100644
> --- a/drivers/crypto/fsl/jr.h
> +++ b/drivers/crypto/fsl/jr.h
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0+ */
>  /*
>   * Copyright 2008-2014 Freescale Semiconductor, Inc.
> + * Copyright 2021 NXP
>   *
>   */
> 
> @@ -8,7 +9,9 @@
>  #define __JR_H
> 
>  #include <linux/compiler.h>
> +#include "fsl_sec.h"
>  #include "type.h"
> +#include <misc.h>
> 
>  #define JR_SIZE 4
>  /* Timeout currently defined as 10 sec */
> @@ -35,6 +38,10 @@
>  #define JRSLIODN_SHIFT         0
>  #define JRSLIODN_MASK          0x00000fff
> 
> +#define JRDID_MS_PRIM_DID      1
> +#define JRDID_MS_PRIM_TZ       (1 << 4)
> +#define JRDID_MS_TZ_OWN                (1 << 15)

Maybe use BIT() macro here?

> +
>  #define JQ_DEQ_ERR             -1
>  #define JQ_DEQ_TO_ERR          -2
>  #define JQ_ENQ_ERR             -3
> @@ -102,6 +109,13 @@ struct result {
>         uint32_t status;
>  };
> 
> +struct caam_regs {
> +       ccsr_sec_t *sec;
> +       struct jr_regs *regs;
> +       u8 jrid;
> +       struct jobring jr[CONFIG_SYS_FSL_MAX_NUM_OF_SEC];
> +};
> +
>  void caam_jr_strstatus(u32 status);
>  int run_descriptor_jr(uint32_t *desc);
> 
> --
> 2.17.1

-- andrey

Link: [1]: https://source.codeaurora.org/external/imx/imx-atf/commit/?id=a83a7c65ea4e7b41d5c8fb129bac9caa89053d5e
Gaurav Jain Nov. 17, 2021, 11:25 a.m. UTC | #3 
Hello Andrey

> -----Original Message-----
> From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> Sent: Tuesday, November 16, 2021 9:24 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>;
> Peng Fan <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka
> Jain <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> <franck.lenormand@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>;
> Sahil Malhotra <sahil.malhotra@nxp.com>; Pankaj Gupta
> <pankaj.gupta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; dl-uboot-imx
> <uboot-imx@nxp.com>; Shengzhou Liu <shengzhou.liu@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; Rajesh Bhagat <rajesh.bhagat@nxp.com>; Meenakshi
> Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim Khan
> <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod
> Kumar <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>;
> Adrian Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> Subject: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> driver model
> 
> Caution: EXT Email
> 
> Hello Gaurav,
> 
> > -----Original Message-----
> > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Gaurav Jain
> > Sent: Monday, November 15, 2021 8:00 AM
> > To: u-boot@lists.denx.de
> > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>; Silvano
> > Di Ninno <silvano.dininno@nxp.com>; Sahil malhotra
> > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> > Sethi <V.Sethi@nxp.com>; NXP i . MX U-Boot Team <uboot-imx@nxp.com>;
> > Shengzhou Liu <Shengzhou.Liu@nxp.com>; Mingkai Hu
> > <mingkai.hu@nxp.com>; Rajesh Bhagat <rajesh.bhagat@nxp.com>;
> Meenakshi
> > Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim Khan
> > <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod
> Kumar
> > <pramod.kumar_1@nxp.com>; Tang Yuantian <andy.tang@nxp.com>; Adrian
> > Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>;
> > Gaurav Jain <gaurav.jain@nxp.com>
> > Subject: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> > driver model
> >
> >
> > added device tree support for job ring driver.
> > sec is initialized based on job ring information processed from device
> > tree.
> >
> > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > Reviewed-by: Ye Li <ye.li@nxp.com>
> > ---
> >  cmd/Kconfig                 |   1 +
> >  drivers/crypto/fsl/Kconfig  |   7 +
> >  drivers/crypto/fsl/Makefile |   4 +-
> >  drivers/crypto/fsl/jr.c     | 316 +++++++++++++++++++++++-------------
> >  drivers/crypto/fsl/jr.h     |  14 ++
> >  5 files changed, 232 insertions(+), 110 deletions(-)
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig index 5b30b13e43..2b24672505
> > 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -2009,6 +2009,7 @@ config CMD_AES
> >
> >  config CMD_BLOB
> >         bool "Enable the 'blob' command"
> > +       select FSL_BLOB
> >         depends on !MX6ULL && !MX6SLL && !MX6SL
> >         select IMX_HAB if ARCH_MX6 || ARCH_MX7 || ARCH_MX7ULP ||
> ARCH_IMX8M
> >         help
> > diff --git a/drivers/crypto/fsl/Kconfig b/drivers/crypto/fsl/Kconfig
> > index 94ff540111..ab59d516f8 100644
> > --- a/drivers/crypto/fsl/Kconfig
> > +++ b/drivers/crypto/fsl/Kconfig
> > @@ -66,4 +66,11 @@ config FSL_CAAM_RNG
> >           using the prediction resistance flag which means the DRGB is
> >           reseeded from the TRNG every time random data is generated.
> >
> > +config FSL_BLOB
> > +        bool "Enable Blob Encap/Decap, Blob KEK support"
> > +       help
> > +         Enable support for the hardware based crytographic blob encap/decap
> > +         module of the CAAM. blobs can be safely placed into non-volatile
> > +         storage. blobs can only be decapsulated by the SoC that created it.
> > +         Enable support for blob key encryption key generation.
> >  endif
> > diff --git a/drivers/crypto/fsl/Makefile b/drivers/crypto/fsl/Makefile
> > index f9c3ccecfc..738535b8e4 100644
> > --- a/drivers/crypto/fsl/Makefile
> > +++ b/drivers/crypto/fsl/Makefile
> > @@ -1,10 +1,12 @@
> >  # SPDX-License-Identifier: GPL-2.0+
> >  #
> >  # Copyright 2014 Freescale Semiconductor, Inc.
> > +# Copyright 2021 NXP
> >
> >  obj-y += sec.o
> >  obj-$(CONFIG_FSL_CAAM) += jr.o fsl_hash.o jobdesc.o error.o
> > -obj-$(CONFIG_CMD_BLOB)$(CONFIG_IMX_CAAM_DEK_ENCAP) += fsl_blob.o
> > +obj-$(CONFIG_FSL_BLOB) += fsl_blob.o
> > +obj-$(CONFIG_IMX_CAAM_DEK_ENCAP) += fsl_blob.o
> >  obj-$(CONFIG_RSA_FREESCALE_EXP) += fsl_rsa.o
> >  obj-$(CONFIG_FSL_CAAM_RNG) += rng.o
> >  obj-$(CONFIG_FSL_MFGPROT) += fsl_mfgprot.o diff --git
> > a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c index
> > 22b649219e..eea2225a1e 100644
> > --- a/drivers/crypto/fsl/jr.c
> > +++ b/drivers/crypto/fsl/jr.c
> > @@ -1,7 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0+
> >  /*
> >   * Copyright 2008-2014 Freescale Semiconductor, Inc.
> > - * Copyright 2018 NXP
> > + * Copyright 2018, 2021 NXP
> >   *
> >   * Based on CAAM driver in drivers/crypto/caam in Linux
> >   */
> > @@ -11,7 +11,6 @@
> >  #include <linux/kernel.h>
> >  #include <log.h>
> >  #include <malloc.h>
> > -#include "fsl_sec.h"
> >  #include "jr.h"
> >  #include "jobdesc.h"
> >  #include "desc_constr.h"
> > @@ -21,8 +20,11 @@
> >  #include <asm/cache.h>
> >  #include <asm/fsl_pamu.h>
> >  #endif
> > +#include <dm.h>
> >  #include <dm/lists.h>
> >  #include <linux/delay.h>
> > +#include <dm/root.h>
> > +#include <dm/device-internal.h>
> >
> >  #define CIRC_CNT(head, tail, size)     (((head) - (tail)) & (size - 1))
> >  #define CIRC_SPACE(head, tail, size)   CIRC_CNT((tail), (head) + 1, (size))
> > @@ -35,20 +37,30 @@ uint32_t
> sec_offset[CONFIG_SYS_FSL_MAX_NUM_OF_SEC]
> > = {  #endif  };
> >
> > +#if CONFIG_IS_ENABLED(DM)
> > +struct udevice *caam_dev;
> > +#else
> >  #define SEC_ADDR(idx)  \
> >         (ulong)((CONFIG_SYS_FSL_SEC_ADDR + sec_offset[idx]))
> >
> >  #define SEC_JR0_ADDR(idx)      \
> >         (ulong)(SEC_ADDR(idx) + \
> >          (CONFIG_SYS_FSL_JR0_OFFSET - CONFIG_SYS_FSL_SEC_OFFSET))
> > +struct caam_regs caam_st;
> > +#endif
> >
> > -struct jobring jr0[CONFIG_SYS_FSL_MAX_NUM_OF_SEC];
> > +static inline u32 jr_start_reg(u8 jrid) {
> > +       return (1 << jrid);
> > +}
> >
> > -static inline void start_jr0(uint8_t sec_idx)
> > +#ifndef CONFIG_ARCH_IMX8
> > +static inline void start_jr(struct caam_regs *caam)
> >  {
> > -       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
> > +       ccsr_sec_t *sec = caam->sec;
> >         u32 ctpr_ms = sec_in32(&sec->ctpr_ms);
> >         u32 scfgr = sec_in32(&sec->scfgr);
> > +       u32 jrstart = jr_start_reg(caam->jrid);
> >
> >         if (ctpr_ms & SEC_CTPR_MS_VIRT_EN_INCL) {
> >                 /* VIRT_EN_INCL = 1 & VIRT_EN_POR = 1 or @@ -56,23
> > +68,17 @@ static inline void start_jr0(uint8_t sec_idx)
> >                  */
> >                 if ((ctpr_ms & SEC_CTPR_MS_VIRT_EN_POR) ||
> >                     (scfgr & SEC_SCFGR_VIRT_EN))
> > -                       sec_out32(&sec->jrstartr, CONFIG_JRSTARTR_JR0);
> > +                       sec_out32(&sec->jrstartr, jrstart);
> >         } else {
> >                 /* VIRT_EN_INCL = 0 && VIRT_EN_POR_VALUE = 1 */
> >                 if (ctpr_ms & SEC_CTPR_MS_VIRT_EN_POR)
> > -                       sec_out32(&sec->jrstartr, CONFIG_JRSTARTR_JR0);
> > +                       sec_out32(&sec->jrstartr, jrstart);
> >         }
> >  }
> > +#endif
> >
> > -static inline void jr_reset_liodn(uint8_t sec_idx) -{
> > -       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
> > -       sec_out32(&sec->jrliodnr[0].ls, 0);
> > -}
> > -
> > -static inline void jr_disable_irq(uint8_t sec_idx)
> > +static inline void jr_disable_irq(struct jr_regs *regs)
> >  {
> > -       struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> >         uint32_t jrcfg = sec_in32(&regs->jrcfg1);
> >
> >         jrcfg = jrcfg | JR_INTMASK;
> > @@ -80,10 +86,10 @@ static inline void jr_disable_irq(uint8_t sec_idx)
> >         sec_out32(&regs->jrcfg1, jrcfg);  }
> >
> > -static void jr_initregs(uint8_t sec_idx)
> > +static void jr_initregs(uint8_t sec_idx, struct caam_regs *caam)
> >  {
> > -       struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> > -       struct jobring *jr = &jr0[sec_idx];
> > +       struct jr_regs *regs = caam->regs;
> > +       struct jobring *jr = &caam->jr[sec_idx];
> >         caam_dma_addr_t ip_base = virt_to_phys((void *)jr->input_ring);
> >         caam_dma_addr_t op_base = virt_to_phys((void
> > *)jr->output_ring);
> >
> > @@ -103,16 +109,16 @@ static void jr_initregs(uint8_t sec_idx)
> >         sec_out32(&regs->irs, JR_SIZE);
> >
> >         if (!jr->irq)
> > -               jr_disable_irq(sec_idx);
> > +               jr_disable_irq(regs);
> >  }
> >
> > -static int jr_init(uint8_t sec_idx)
> > +static int jr_init(uint8_t sec_idx, struct caam_regs *caam)
> >  {
> > -       struct jobring *jr = &jr0[sec_idx];
> > +       struct jobring *jr = &caam->jr[sec_idx];
> >
> >         memset(jr, 0, sizeof(struct jobring));
> >
> > -       jr->jq_id = DEFAULT_JR_ID;
> > +       jr->jq_id = caam->jrid;
> >         jr->irq = DEFAULT_IRQ;
> >
> >  #ifdef CONFIG_FSL_CORENET
> > @@ -134,53 +140,10 @@ static int jr_init(uint8_t sec_idx)
> >         memset(jr->input_ring, 0, JR_SIZE * sizeof(caam_dma_addr_t));
> >         memset(jr->output_ring, 0, jr->op_size);
> >
> > -       start_jr0(sec_idx);
> > -
> > -       jr_initregs(sec_idx);
> > -
> > -       return 0;
> > -}
> > -
> > -static int jr_sw_cleanup(uint8_t sec_idx) -{
> > -       struct jobring *jr = &jr0[sec_idx];
> > -
> > -       jr->head = 0;
> > -       jr->tail = 0;
> > -       jr->read_idx = 0;
> > -       jr->write_idx = 0;
> > -       memset(jr->info, 0, sizeof(jr->info));
> > -       memset(jr->input_ring, 0, jr->size * sizeof(caam_dma_addr_t));
> > -       memset(jr->output_ring, 0, jr->size * sizeof(struct op_ring));
> > -
> > -       return 0;
> > -}
> > -
> > -static int jr_hw_reset(uint8_t sec_idx) -{
> > -       struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> > -       uint32_t timeout = 100000;
> > -       uint32_t jrint, jrcr;
> > -
> > -       sec_out32(&regs->jrcr, JRCR_RESET);
> > -       do {
> > -               jrint = sec_in32(&regs->jrint);
> > -       } while (((jrint & JRINT_ERR_HALT_MASK) ==
> > -                 JRINT_ERR_HALT_INPROGRESS) && --timeout);
> > -
> > -       jrint = sec_in32(&regs->jrint);
> > -       if (((jrint & JRINT_ERR_HALT_MASK) !=
> > -            JRINT_ERR_HALT_INPROGRESS) && timeout == 0)
> > -               return -1;
> > -
> > -       timeout = 100000;
> > -       sec_out32(&regs->jrcr, JRCR_RESET);
> > -       do {
> > -               jrcr = sec_in32(&regs->jrcr);
> > -       } while ((jrcr & JRCR_RESET) && --timeout);
> > -
> > -       if (timeout == 0)
> > -               return -1;
> > +#ifndef CONFIG_ARCH_IMX8
> > +       start_jr(caam);
> > +#endif
> > +       jr_initregs(sec_idx, caam);
> >
> >         return 0;
> >  }
> > @@ -188,10 +151,10 @@ static int jr_hw_reset(uint8_t sec_idx)
> >  /* -1 --- error, can't enqueue -- no space available */  static int
> > jr_enqueue(uint32_t *desc_addr,
> >                void (*callback)(uint32_t status, void *arg),
> > -              void *arg, uint8_t sec_idx)
> > +              void *arg, uint8_t sec_idx, struct caam_regs *caam)
> >  {
> > -       struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> > -       struct jobring *jr = &jr0[sec_idx];
> > +       struct jr_regs *regs = caam->regs;
> > +       struct jobring *jr = &caam->jr[sec_idx];
> >         int head = jr->head;
> >         uint32_t desc_word;
> >         int length = desc_len(desc_addr); @@ -263,10 +226,10 @@ static
> > int jr_enqueue(uint32_t *desc_addr,
> >         return 0;
> >  }
> >
> > -static int jr_dequeue(int sec_idx)
> > +static int jr_dequeue(int sec_idx, struct caam_regs *caam)
> >  {
> > -       struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> > -       struct jobring *jr = &jr0[sec_idx];
> > +       struct jr_regs *regs = caam->regs;
> > +       struct jobring *jr = &caam->jr[sec_idx];
> >         int head = jr->head;
> >         int tail = jr->tail;
> >         int idx, i, found;
> > @@ -349,14 +312,18 @@ static void desc_done(uint32_t status, void
> > *arg)  {
> >         struct result *x = arg;
> >         x->status = status;
> > -#ifndef CONFIG_SPL_BUILD
> >         caam_jr_strstatus(status);
> > -#endif
> >         x->done = 1;
> >  }
> >
> >  static inline int run_descriptor_jr_idx(uint32_t *desc, uint8_t
> > sec_idx)  {
> > +       struct caam_regs *caam;
> > +#if CONFIG_IS_ENABLED(DM)
> > +       caam = dev_get_priv(caam_dev); #else
> > +       caam = &caam_st;
> > +#endif
> >         unsigned long long timeval = 0;
> >         unsigned long long timeout = CONFIG_USEC_DEQ_TIMEOUT;
> >         struct result op;
> > @@ -364,7 +331,7 @@ static inline int run_descriptor_jr_idx(uint32_t
> > *desc, uint8_t sec_idx)
> >
> >         memset(&op, 0, sizeof(op));
> >
> > -       ret = jr_enqueue(desc, desc_done, &op, sec_idx);
> > +       ret = jr_enqueue(desc, desc_done, &op, sec_idx, caam);
> >         if (ret) {
> >                 debug("Error in SEC enq\n");
> >                 ret = JQ_ENQ_ERR;
> > @@ -375,7 +342,7 @@ static inline int run_descriptor_jr_idx(uint32_t
> > *desc, uint8_t sec_idx)
> >                 udelay(1);
> >                 timeval += 1;
> >
> > -               ret = jr_dequeue(sec_idx);
> > +               ret = jr_dequeue(sec_idx, caam);
> >                 if (ret) {
> >                         debug("Error in SEC deq\n");
> >                         ret = JQ_DEQ_ERR; @@ -402,13 +369,63 @@ int
> > run_descriptor_jr(uint32_t *desc)
> >         return run_descriptor_jr_idx(desc, 0);  }
> >
> > +#ifndef CONFIG_ARCH_IMX8
> > +static int jr_sw_cleanup(uint8_t sec_idx, struct caam_regs *caam) {
> > +       struct jobring *jr = &caam->jr[sec_idx];
> > +
> > +       jr->head = 0;
> > +       jr->tail = 0;
> > +       jr->read_idx = 0;
> > +       jr->write_idx = 0;
> > +       memset(jr->info, 0, sizeof(jr->info));
> > +       memset(jr->input_ring, 0, jr->size * sizeof(caam_dma_addr_t));
> > +       memset(jr->output_ring, 0, jr->size * sizeof(struct op_ring));
> > +
> > +       return 0;
> > +}
> > +
> > +static int jr_hw_reset(struct jr_regs *regs) {
> > +       uint32_t timeout = 100000;
> > +       uint32_t jrint, jrcr;
> > +
> > +       sec_out32(&regs->jrcr, JRCR_RESET);
> > +       do {
> > +               jrint = sec_in32(&regs->jrint);
> > +       } while (((jrint & JRINT_ERR_HALT_MASK) ==
> > +                 JRINT_ERR_HALT_INPROGRESS) && --timeout);
> > +
> > +       jrint = sec_in32(&regs->jrint);
> > +       if (((jrint & JRINT_ERR_HALT_MASK) !=
> > +            JRINT_ERR_HALT_INPROGRESS) && timeout == 0)
> > +               return -1;
> > +
> > +       timeout = 100000;
> > +       sec_out32(&regs->jrcr, JRCR_RESET);
> > +       do {
> > +               jrcr = sec_in32(&regs->jrcr);
> > +       } while ((jrcr & JRCR_RESET) && --timeout);
> > +
> > +       if (timeout == 0)
> > +               return -1;
> > +
> > +       return 0;
> > +}
> > +
> >  static inline int jr_reset_sec(uint8_t sec_idx)  {
> > -       if (jr_hw_reset(sec_idx) < 0)
> > +       struct caam_regs *caam;
> > +#if CONFIG_IS_ENABLED(DM)
> > +       caam = dev_get_priv(caam_dev); #else
> > +       caam = &caam_st;
> > +#endif
> > +       if (jr_hw_reset(caam->regs) < 0)
> >                 return -1;
> >
> >         /* Clean up the jobring structure maintained by software */
> > -       jr_sw_cleanup(sec_idx);
> > +       jr_sw_cleanup(sec_idx, caam);
> >
> >         return 0;
> >  }
> > @@ -418,9 +435,15 @@ int jr_reset(void)
> >         return jr_reset_sec(0);
> >  }
> >
> > -static inline int sec_reset_idx(uint8_t sec_idx)
> > +int sec_reset(void)
> >  {
> > -       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
> > +       struct caam_regs *caam;
> > +#if CONFIG_IS_ENABLED(DM)
> > +       caam = dev_get_priv(caam_dev); #else
> > +       caam = &caam_st;
> > +#endif
> > +       ccsr_sec_t *sec = caam->sec;
> >         uint32_t mcfgr = sec_in32(&sec->mcfgr);
> >         uint32_t timeout = 100000;
> >
> > @@ -446,11 +469,7 @@ static inline int sec_reset_idx(uint8_t sec_idx)
> >
> >         return 0;
> >  }
> > -int sec_reset(void)
> > -{
> > -       return sec_reset_idx(0);
> > -}
> > -#ifndef CONFIG_SPL_BUILD
> > +
> >  static int deinstantiate_rng(u8 sec_idx, int state_handle_mask)  {
> >         u32 *desc;
> > @@ -496,12 +515,11 @@ static int deinstantiate_rng(u8 sec_idx, int
> > state_handle_mask)
> >         return ret;
> >  }
> >
> > -static int instantiate_rng(u8 sec_idx, int gen_sk)
> > +static int instantiate_rng(uint8_t sec_idx, ccsr_sec_t *sec, int
> > +gen_sk)
> >  {
> >         u32 *desc;
> >         u32 rdsta_val;
> >         int ret = 0, sh_idx, size;
> > -       ccsr_sec_t __iomem *sec = (ccsr_sec_t __iomem *)SEC_ADDR(sec_idx);
> >         struct rng4tst __iomem *rng =
> >                         (struct rng4tst __iomem *)&sec->rng;
> >
> > @@ -554,9 +572,8 @@ static int instantiate_rng(u8 sec_idx, int gen_sk)
> >         return ret;
> >  }
> >
> > -static u8 get_rng_vid(uint8_t sec_idx)
> > +static u8 get_rng_vid(ccsr_sec_t *sec)
> >  {
> > -       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
> >         u8 vid;
> >
> >         if (caam_get_era() < 10) {
> > @@ -574,9 +591,8 @@ static u8 get_rng_vid(uint8_t sec_idx)
> >   * By default, the TRNG runs for 200 clocks per sample;
> >   * 1200 clocks per sample generates better entropy.
> >   */
> > -static void kick_trng(int ent_delay, uint8_t sec_idx)
> > +static void kick_trng(int ent_delay, ccsr_sec_t *sec)
> >  {
> > -       ccsr_sec_t __iomem *sec = (ccsr_sec_t __iomem *)SEC_ADDR(sec_idx);
> >         struct rng4tst __iomem *rng =
> >                         (struct rng4tst __iomem *)&sec->rng;
> >         u32 val;
> > @@ -603,10 +619,9 @@ static void kick_trng(int ent_delay, uint8_t sec_idx)
> >         sec_clrbits32(&rng->rtmctl, RTMCTL_PRGM);  }
> >
> > -static int rng_init(uint8_t sec_idx)
> > +static int rng_init(uint8_t sec_idx, ccsr_sec_t *sec)
> >  {
> >         int ret, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
> > -       ccsr_sec_t __iomem *sec = (ccsr_sec_t __iomem *)SEC_ADDR(sec_idx);
> >         struct rng4tst __iomem *rng =
> >                         (struct rng4tst __iomem *)&sec->rng;
> >         u32 inst_handles;
> > @@ -624,7 +639,7 @@ static int rng_init(uint8_t sec_idx)
> >                  * the TRNG parameters.
> >                  */
> >                 if (!inst_handles) {
> > -                       kick_trng(ent_delay, sec_idx);
> > +                       kick_trng(ent_delay, sec);
> >                         ent_delay += 400;
> >                 }
> >                 /*
> > @@ -634,7 +649,7 @@ static int rng_init(uint8_t sec_idx)
> >                  * interval, leading to a sucessful initialization of
> >                  * the RNG.
> >                  */
> > -               ret = instantiate_rng(sec_idx, gen_sk);
> > +               ret = instantiate_rng(sec_idx, sec, gen_sk);
> >         } while ((ret == -1) && (ent_delay < RTSDCTL_ENT_DLY_MAX));
> >         if (ret) {
> >                 printf("SEC%u:  Failed to instantiate RNG\n",
> > sec_idx); @@ -647,12 +662,29 @@ static int rng_init(uint8_t sec_idx)
> >         return ret;
> >  }
> >  #endif
> > +
> >  int sec_init_idx(uint8_t sec_idx)
> >  {
> > -       ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
> > -       uint32_t mcr = sec_in32(&sec->mcfgr);
> >         int ret = 0;
> > -
> > +       struct caam_regs *caam;
> > +#if CONFIG_IS_ENABLED(DM)
> > +       if (caam_dev == NULL) {
> > +               printf("caam_jr: caam not found\n");
> > +               return -1;
> > +       }
> > +       caam = dev_get_priv(caam_dev); #else
> > +       caam_st.sec = (void *)SEC_ADDR(sec_idx);
> > +       caam_st.regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> > +       caam_st.jrid = 0;
> > +       caam = &caam_st;
> > +#endif
> > +#ifndef CONFIG_ARCH_IMX8
> > +       ccsr_sec_t *sec = caam->sec;
> > +       uint32_t mcr = sec_in32(&sec->mcfgr); #if
> > +defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
> > +       uint32_t jrdid_ms = 0;
> > +#endif
> >  #ifdef CONFIG_FSL_CORENET
> >         uint32_t liodnr;
> >         uint32_t liodn_ns;
> > @@ -682,6 +714,11 @@ int sec_init_idx(uint8_t sec_idx)
> >         mcr |= (1 << MCFGR_PS_SHIFT);
> >  #endif
> >         sec_out32(&sec->mcfgr, mcr);
> > +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
> 
> This would effectively reserve the JR0 on _all_ i.MX8M derivatives is S World.
This code is to set any JR DID in SPL so that the job ring can be configured. 

> 
> Current implementation only has JR0 reserved in S World on imx8mm derivative,
> but this new addition extends this to imx8mn, imx8mp and imx8mq.
Current implementation do not initialize CAAM for i.MX8M derivatives. It is not based on driver model approach and only using JR0.
With New implementation CAAM is enabled for i.MX8M derivative. Any JR whose DID is written in ATF, can be used in Uboot.
JR0 is reserved for HAB so JR1 will be used for all i.MX8M derivatives.

> 
> I'm wondering about several points here:
> 1. Why does current implementation on have this reservation done on imx8mm
> and
>    where does this happen? None of the code pieces suggests that it is done in
>    U-Boot, is it performed in BootROM?

I cannot see if current implementation(SPL/Uboot) has reservation done for imx8mm.
In ATF, we are reserving the JR0.

> 2. What is the intention of having JR0 reserved for all derivatives? Is this
>    the part of a bigger change that stretches across different SW components
>    (e.g. ATF, OP-TEE, etc.)? If that is the case - then a more detailed
>    description would be appreciated here.
> 
> ATF code already accounts for this reservation in commit:
> a83a7c65e ("TEE-639 plat: imx8m: Do not release JR0 to NS if HAB is using it")
> [1], but there is no description on why is this required though.
> 
> If this is required for HAB feature, then the question is: should it be kept in S
> World when U-Boot starts, or SPL can release it after the binary is verified and
> crypto facilities are not in use anymore?

Commit: a83a7c65e reserves JR0 for HAB and not released to NS but JR1, JR2 are released to NS.
HAB uses JR0 for secure boot on all i.MX8M derivatives. Uboot calls HAB API for authenticating kernel.

> 
> > +       jrdid_ms = JRDID_MS_TZ_OWN | JRDID_MS_PRIM_TZ |
> > + JRDID_MS_PRIM_DID;
> 
> What is the intention of setting JRDID_MS_PRIM_TZ? Isn't setting
> JRDID_MS_TZ_OWN would be sufficient here?

PRIM_TZ bit is set to 1 to indicate that only SecureWorld can
access registers in that Job Ring's register page

> 
> > +       sec_out32(&sec->jrliodnr[caam->jrid].ms, jrdid_ms); #endif
> > +       jr_reset();
> >
> >  #ifdef CONFIG_FSL_CORENET
> >  #ifdef CONFIG_SPL_BUILD
> > @@ -693,25 +730,26 @@ int sec_init_idx(uint8_t sec_idx)
> >         liodn_ns = CONFIG_SPL_JR0_LIODN_NS & JRNSLIODN_MASK;
> >         liodn_s = CONFIG_SPL_JR0_LIODN_S & JRSLIODN_MASK;
> >
> > -       liodnr = sec_in32(&sec->jrliodnr[0].ls) &
> > +       liodnr = sec_in32(&sec->jrliodnr[caam->jrid].ls) &
> >                  ~(JRNSLIODN_MASK | JRSLIODN_MASK);
> >         liodnr = liodnr |
> >                  (liodn_ns << JRNSLIODN_SHIFT) |
> >                  (liodn_s << JRSLIODN_SHIFT);
> > -       sec_out32(&sec->jrliodnr[0].ls, liodnr);
> > +       sec_out32(&sec->jrliodnr[caam->jrid].ls, liodnr);
> >  #else
> > -       liodnr = sec_in32(&sec->jrliodnr[0].ls);
> > +       liodnr = sec_in32(&sec->jrliodnr[caam->jrid].ls);
> >         liodn_ns = (liodnr & JRNSLIODN_MASK) >> JRNSLIODN_SHIFT;
> >         liodn_s = (liodnr & JRSLIODN_MASK) >> JRSLIODN_SHIFT;  #endif
> > #endif
> > -
> > -       ret = jr_init(sec_idx);
> > +#endif
> > +       ret = jr_init(sec_idx, caam);
> >         if (ret < 0) {
> >                 printf("SEC%u:  initialization failed\n", sec_idx);
> >                 return -1;
> >         }
> >
> > +#ifndef CONFIG_ARCH_IMX8
> >  #ifdef CONFIG_FSL_CORENET
> >         ret = sec_config_pamu_table(liodn_ns, liodn_s);
> >         if (ret < 0)
> > @@ -719,9 +757,9 @@ int sec_init_idx(uint8_t sec_idx)
> >
> >         pamu_enable();
> >  #endif
> > -#ifndef CONFIG_SPL_BUILD
> > -       if (get_rng_vid(sec_idx) >= 4) {
> > -               if (rng_init(sec_idx) < 0) {
> > +
> > +       if (get_rng_vid(caam->sec) >= 4) {
> > +               if (rng_init(sec_idx, caam->sec) < 0) {
> >                         printf("SEC%u:  RNG instantiation failed\n", sec_idx);
> >                         return -1;
> >                 }
> > @@ -743,3 +781,63 @@ int sec_init(void)  {
> >         return sec_init_idx(0);
> >  }
> > +
> > +#if CONFIG_IS_ENABLED(DM)
> > +static int caam_jr_probe(struct udevice *dev) {
> > +       struct caam_regs *caam = dev_get_priv(dev);
> > +       fdt_addr_t addr;
> > +       ofnode node;
> > +       unsigned int jr_node = 0;
> > +
> > +       caam_dev = dev;
> > +
> > +       addr = dev_read_addr(dev);
> > +       if (addr == FDT_ADDR_T_NONE) {
> > +               printf("caam_jr: crypto not found\n");
> > +               return -EINVAL;
> > +       }
> > +       caam->sec = (ccsr_sec_t *)(uintptr_t)addr;
> > +       caam->regs = (struct jr_regs *)caam->sec;
> > +
> > +       /* Check for enabled job ring node */
> > +       ofnode_for_each_subnode(node, dev_ofnode(dev)) {
> > +               if (!ofnode_is_available(node)) {
> > +                       continue;
> > +               }
> > +               jr_node = ofnode_read_u32_default(node, "reg", -1);
> > +               if (jr_node > 0) {
> > +                       caam->regs = (struct jr_regs
> > + *)((ulong)caam->sec +
> > jr_node);
> > +                       while (!(jr_node & 0x0F)) {
> > +                               jr_node = jr_node >> 4;
> > +                       }
> > +                       caam->jrid = jr_node - 1;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (sec_init())
> > +               printf("\nsec_init failed!\n");
> > +
> > +       return 0;
> > +}
> > +
> > +static int caam_jr_bind(struct udevice *dev) {
> > +       return 0;
> > +}
> > +
> > +static const struct udevice_id caam_jr_match[] = {
> > +       { .compatible = "fsl,sec-v4.0" },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(caam_jr) = {
> > +       .name           = "caam_jr",
> > +       .id             = UCLASS_MISC,
> > +       .of_match       = caam_jr_match,
> > +       .bind           = caam_jr_bind,
> > +       .probe          = caam_jr_probe,
> > +       .priv_auto      = sizeof(struct caam_regs),
> > +};
> > +#endif
> > diff --git a/drivers/crypto/fsl/jr.h b/drivers/crypto/fsl/jr.h index
> > 1047aa772c..43cb5e0753 100644
> > --- a/drivers/crypto/fsl/jr.h
> > +++ b/drivers/crypto/fsl/jr.h
> > @@ -1,6 +1,7 @@
> >  /* SPDX-License-Identifier: GPL-2.0+ */
> >  /*
> >   * Copyright 2008-2014 Freescale Semiconductor, Inc.
> > + * Copyright 2021 NXP
> >   *
> >   */
> >
> > @@ -8,7 +9,9 @@
> >  #define __JR_H
> >
> >  #include <linux/compiler.h>
> > +#include "fsl_sec.h"
> >  #include "type.h"
> > +#include <misc.h>
> >
> >  #define JR_SIZE 4
> >  /* Timeout currently defined as 10 sec */ @@ -35,6 +38,10 @@
> >  #define JRSLIODN_SHIFT         0
> >  #define JRSLIODN_MASK          0x00000fff
> >
> > +#define JRDID_MS_PRIM_DID      1
> > +#define JRDID_MS_PRIM_TZ       (1 << 4)
> > +#define JRDID_MS_TZ_OWN                (1 << 15)
> 
> Maybe use BIT() macro here?
Will do the change in next version of this patch series.

Regards
Gaurav Jain
> 
> > +
> >  #define JQ_DEQ_ERR             -1
> >  #define JQ_DEQ_TO_ERR          -2
> >  #define JQ_ENQ_ERR             -3
> > @@ -102,6 +109,13 @@ struct result {
> >         uint32_t status;
> >  };
> >
> > +struct caam_regs {
> > +       ccsr_sec_t *sec;
> > +       struct jr_regs *regs;
> > +       u8 jrid;
> > +       struct jobring jr[CONFIG_SYS_FSL_MAX_NUM_OF_SEC];
> > +};
> > +
> >  void caam_jr_strstatus(u32 status);
> >  int run_descriptor_jr(uint32_t *desc);
> >
> > --
> > 2.17.1
> 
> -- andrey
> 
> Link: [1]:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.c
> odeaurora.org%2Fexternal%2Fimx%2Fimx-
> atf%2Fcommit%2F%3Fid%3Da83a7c65ea4e7b41d5c8fb129bac9caa89053d5e&a
> mp;data=04%7C01%7Cgaurav.jain%40nxp.com%7C1b6edcabe31e4b9cae3d08d
> 9a9195296%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637726748
> 521538374%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=ANasYQwEH
> %2BEFyBbbWn8dBk2HcvwYdFr3QHXUAu74SIg%3D&amp;reserved=0
ZHIZHIKIN Andrey Nov. 17, 2021, 1:02 p.m. UTC | #4 
Hello Gaurav,

> -----Original Message-----
> From: Gaurav Jain <gaurav.jain@nxp.com>
> Sent: Wednesday, November 17, 2021 12:26 PM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; u-
> boot@lists.denx.de
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; Peng Fan
> <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka Jain
> <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> <franck.lenormand@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil
> Malhotra <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>; Shengzhou Liu
> <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Rajesh Bhagat
> <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim
> Khan <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>; Adrian Alonso
> <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> driver model
> 
> 
> Hello Andrey
> 
> > -----Original Message-----
> > From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> > Sent: Tuesday, November 16, 2021 9:24 PM
> > To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>;
> > Peng Fan <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka
> > Jain <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> > <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> > <franck.lenormand@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>;
> > Sahil Malhotra <sahil.malhotra@nxp.com>; Pankaj Gupta
> > <pankaj.gupta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; dl-uboot-imx
> > <uboot-imx@nxp.com>; Shengzhou Liu <shengzhou.liu@nxp.com>; Mingkai Hu
> > <mingkai.hu@nxp.com>; Rajesh Bhagat <rajesh.bhagat@nxp.com>; Meenakshi
> > Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim Khan
> > <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod
> > Kumar <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>;
> > Adrian Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> > Subject: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> > driver model
> >
> > Caution: EXT Email
> >
> > Hello Gaurav,
> >
> > > -----Original Message-----
> > > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Gaurav Jain
> > > Sent: Monday, November 15, 2021 8:00 AM
> > > To: u-boot@lists.denx.de
> > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>; Silvano
> > > Di Ninno <silvano.dininno@nxp.com>; Sahil malhotra
> > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> > > Sethi <V.Sethi@nxp.com>; NXP i . MX U-Boot Team <uboot-imx@nxp.com>;
> > > Shengzhou Liu <Shengzhou.Liu@nxp.com>; Mingkai Hu
> > > <mingkai.hu@nxp.com>; Rajesh Bhagat <rajesh.bhagat@nxp.com>;
> > Meenakshi
> > > Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim Khan
> > > <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod
> > Kumar
> > > <pramod.kumar_1@nxp.com>; Tang Yuantian <andy.tang@nxp.com>; Adrian
> > > Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>;
> > > Gaurav Jain <gaurav.jain@nxp.com>
> > > Subject: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> > > driver model
> > >
> > >
> > > added device tree support for job ring driver.
> > > sec is initialized based on job ring information processed from device
> > > tree.
> > >
> > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > > Reviewed-by: Ye Li <ye.li@nxp.com>
> > > ---
> > >  cmd/Kconfig                 |   1 +
> > >  drivers/crypto/fsl/Kconfig  |   7 +
> > >  drivers/crypto/fsl/Makefile |   4 +-
> > >  drivers/crypto/fsl/jr.c     | 316 +++++++++++++++++++++++-------------
> > >  drivers/crypto/fsl/jr.h     |  14 ++
> > >  5 files changed, 232 insertions(+), 110 deletions(-)
> > >

[snip]

> > >         sec_out32(&sec->mcfgr, mcr);
> > > +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
> >
> > This would effectively reserve the JR0 on _all_ i.MX8M derivatives is S World.
> This code is to set any JR DID in SPL so that the job ring can be configured.
> 
> >
> > Current implementation only has JR0 reserved in S World on imx8mm derivative,
> > but this new addition extends this to imx8mn, imx8mp and imx8mq.
> Current implementation do not initialize CAAM for i.MX8M derivatives. It is not
> based on driver model approach and only using JR0.

OK, but then I do not have on explanation on why do I see following results from
reading JRaDID_MS registers on imx8m derivatives:
- imx8mm:
	JR0DID_MS = 0x8011
	JR1DID_MS = 0x0
	JR2DID_MS = 0x0
- imx8mn:
	JR0DID_MS = 0x0
	JR1DID_MS = 0x0
	JR2DID_MS = 0x0
- imx8mp:
	JR0DID_MS = 0x0
	JR1DID_MS = 0x0
	JR2DID_MS = 0x0

This readout is taken at Kernel boot, and it clearly shows that only JR0 has
TZ_OWN, PRIM_TZ and PRIM_DID bits set, and it is only done on imx8mm.

> With New implementation CAAM is enabled for i.MX8M derivative. Any JR whose DID
> is written in ATF, can be used in Uboot.
> JR0 is reserved for HAB so JR1 will be used for all i.MX8M derivatives.
> 
> >
> > I'm wondering about several points here:
> > 1. Why does current implementation on have this reservation done on imx8mm
> > and
> >    where does this happen? None of the code pieces suggests that it is done in
> >    U-Boot, is it performed in BootROM?
> 
> I cannot see if current implementation(SPL/Uboot) has reservation done for
> imx8mm.
> In ATF, we are reserving the JR0.

I was not able to identify which part of ATF code is responsible to program
JR0DID_MS on imx8mm, the only thing I saw was the part where the JR0 is held
in S World *if* the JR0DID_MS is set to 0x8011. Can you point out where is this
performed in ATF code?

If it is not in the ATF, then my question above still stands: which component
(HW or SW) programs JR0DID_MS, and why is it only done on imx8mm derivative?

> 
> > 2. What is the intention of having JR0 reserved for all derivatives? Is this
> >    the part of a bigger change that stretches across different SW components
> >    (e.g. ATF, OP-TEE, etc.)? If that is the case - then a more detailed
> >    description would be appreciated here.
> >
> > ATF code already accounts for this reservation in commit:
> > a83a7c65e ("TEE-639 plat: imx8m: Do not release JR0 to NS if HAB is using it")
> > [1], but there is no description on why is this required though.
> >
> > If this is required for HAB feature, then the question is: should it be kept in
> S
> > World when U-Boot starts, or SPL can release it after the binary is verified
> and
> > crypto facilities are not in use anymore?
> 
> Commit: a83a7c65e reserves JR0 for HAB and not released to NS but JR1, JR2 are
> released to NS.

Then I believe this change should be in-sync with ATF implementation, because of
the fact that your change can have any arbitrary JR to be held in S World.

What would happen if for example JR1 is programmed with TZ_OWN, but ATF releases
it to NS world? Can it be used by Kernel afterwards? Or should the node be
disabled here so that Kernel does not even see JR1 during boot?

So far, ATF only examines the JR0DID_MS content, and not all the others...

> HAB uses JR0 for secure boot on all i.MX8M derivatives. Uboot calls HAB API for
> authenticating kernel.

This implies then that the JR0 is permanently held in S World and stays there for
entire device powercycle and cannot be reclaimed in NS World? In this case, the
DT node should be completely removed from DTB file so no SW entity can even see
it (as it is in a total possession of HW mechanisms). 

> 
> >
> > > +       jrdid_ms = JRDID_MS_TZ_OWN | JRDID_MS_PRIM_TZ |
> > > + JRDID_MS_PRIM_DID;
> >
> > What is the intention of setting JRDID_MS_PRIM_TZ? Isn't setting
> > JRDID_MS_TZ_OWN would be sufficient here?
> 
> PRIM_TZ bit is set to 1 to indicate that only SecureWorld can
> access registers in that Job Ring's register page

But would it not be enough just to set TZ_OWN? If I read SRM correct: only
TZ_OWN is enough to hold the JR in S World.

> 

[snip]

> >

-- andrey
ZHIZHIKIN Andrey Nov. 17, 2021, 8:19 p.m. UTC | #5 
Hello Gaurav,

> -----Original Message-----
> From: ZHIZHIKIN Andrey
> Sent: Wednesday, November 17, 2021 2:03 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; Peng Fan
> <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka Jain
> <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> <franck.lenormand@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil
> Malhotra <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>; Shengzhou Liu
> <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Rajesh Bhagat
> <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim
> Khan <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>; Adrian Alonso
> <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> driver model
> 
> Hello Gaurav,
> 
> > -----Original Message-----
> > From: Gaurav Jain <gaurav.jain@nxp.com>
> > Sent: Wednesday, November 17, 2021 12:26 PM
> > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; u-
> > boot@lists.denx.de
> > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; Peng
> Fan
> > <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka Jain
> > <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> > <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> > <franck.lenormand@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil
> > Malhotra <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> > Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>; Shengzhou Liu
> > <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Rajesh Bhagat
> > <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim
> > Khan <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> > <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>; Adrian Alonso
> > <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> > Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job
> ring
> > driver model
> >
> >
> > Hello Andrey
> >
> > > -----Original Message-----
> > > From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> > > Sent: Tuesday, November 16, 2021 9:24 PM
> > > To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>;
> > > Peng Fan <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka
> > > Jain <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> > > <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> > > <franck.lenormand@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>;
> > > Sahil Malhotra <sahil.malhotra@nxp.com>; Pankaj Gupta
> > > <pankaj.gupta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; dl-uboot-imx
> > > <uboot-imx@nxp.com>; Shengzhou Liu <shengzhou.liu@nxp.com>; Mingkai Hu
> > > <mingkai.hu@nxp.com>; Rajesh Bhagat <rajesh.bhagat@nxp.com>; Meenakshi
> > > Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim Khan
> > > <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod
> > > Kumar <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>;
> > > Adrian Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> > > Subject: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> > > driver model
> > >
> > > Caution: EXT Email
> > >
> > > Hello Gaurav,
> > >
> > > > -----Original Message-----
> > > > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Gaurav Jain
> > > > Sent: Monday, November 15, 2021 8:00 AM
> > > > To: u-boot@lists.denx.de
> > > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > > > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > > > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>; Silvano
> > > > Di Ninno <silvano.dininno@nxp.com>; Sahil malhotra
> > > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> > > > Sethi <V.Sethi@nxp.com>; NXP i . MX U-Boot Team <uboot-imx@nxp.com>;
> > > > Shengzhou Liu <Shengzhou.Liu@nxp.com>; Mingkai Hu
> > > > <mingkai.hu@nxp.com>; Rajesh Bhagat <rajesh.bhagat@nxp.com>;
> > > Meenakshi
> > > > Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim Khan
> > > > <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod
> > > Kumar
> > > > <pramod.kumar_1@nxp.com>; Tang Yuantian <andy.tang@nxp.com>; Adrian
> > > > Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>;
> > > > Gaurav Jain <gaurav.jain@nxp.com>
> > > > Subject: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> > > > driver model
> > > >
> > > >
> > > > added device tree support for job ring driver.
> > > > sec is initialized based on job ring information processed from device
> > > > tree.
> > > >
> > > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > > > Reviewed-by: Ye Li <ye.li@nxp.com>
> > > > ---
> > > >  cmd/Kconfig                 |   1 +
> > > >  drivers/crypto/fsl/Kconfig  |   7 +
> > > >  drivers/crypto/fsl/Makefile |   4 +-
> > > >  drivers/crypto/fsl/jr.c     | 316 +++++++++++++++++++++++-------------
> > > >  drivers/crypto/fsl/jr.h     |  14 ++
> > > >  5 files changed, 232 insertions(+), 110 deletions(-)
> > > >
> 
> [snip]
> 
> > > >         sec_out32(&sec->mcfgr, mcr);
> > > > +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
> > >
> > > This would effectively reserve the JR0 on _all_ i.MX8M derivatives is S
> World.
> > This code is to set any JR DID in SPL so that the job ring can be configured.
> >
> > >
> > > Current implementation only has JR0 reserved in S World on imx8mm derivative,
> > > but this new addition extends this to imx8mn, imx8mp and imx8mq.
> > Current implementation do not initialize CAAM for i.MX8M derivatives. It is not
> > based on driver model approach and only using JR0.
> 
> OK, but then I do not have on explanation on why do I see following results from
> reading JRaDID_MS registers on imx8m derivatives:
> - imx8mm:
> 	JR0DID_MS = 0x8011
> 	JR1DID_MS = 0x0
> 	JR2DID_MS = 0x0
> - imx8mn:
> 	JR0DID_MS = 0x0
> 	JR1DID_MS = 0x0
> 	JR2DID_MS = 0x0
> - imx8mp:
> 	JR0DID_MS = 0x0
> 	JR1DID_MS = 0x0
> 	JR2DID_MS = 0x0

I'd have to correct the readout above, I've posted the data which was not 100% accurate.

Here is the actual one:
- imx8mm:
	JR0DID_MS = 0x8011
	JR1DID_MS = 0x1
	JR2DID_MS = 0x1
- imx8mn:
	JR0DID_MS = 0x1
	JR1DID_MS = 0x1
	JR2DID_MS = 0x1
- imx8mp:
	JR0DID_MS = 0x0
	JR1DID_MS = 0x0
	JR2DID_MS = 0x0

It does suggests the following:
- Mini does have JR0 reserved in S World, JR1 and JR2 are released
  to NS World with DID programmed. According to the new logic in
  the patch - this should allow Mini to utilize HAB feature.
- Nano does have all JR released in NS World, which suggests that
  HAB is not available for it, correct?
- Plus does not have DID programmed in *any* JR devices, which
  fails the RNG initialization during Kernel start since DEC0
  cannot be initialized, but it is required to prime RNG via
  direct descriptor execution in DEC0. This means that all Crypto
  facilities are currently unavailable on Plus, correct? Does
  any of patches in this series suggests the fix for this? Is there
  simply power missing?

I would appreciate if you can comment on the rest of my points as they are still opened. 

> 
> This readout is taken at Kernel boot, and it clearly shows that only JR0 has
> TZ_OWN, PRIM_TZ and PRIM_DID bits set, and it is only done on imx8mm.
> 
> > With New implementation CAAM is enabled for i.MX8M derivative. Any JR whose DID
> > is written in ATF, can be used in Uboot.
> > JR0 is reserved for HAB so JR1 will be used for all i.MX8M derivatives.
> >
> > >
> > > I'm wondering about several points here:
> > > 1. Why does current implementation on have this reservation done on imx8mm
> > > and
> > >    where does this happen? None of the code pieces suggests that it is done
> in
> > >    U-Boot, is it performed in BootROM?
> >
> > I cannot see if current implementation(SPL/Uboot) has reservation done for
> > imx8mm.
> > In ATF, we are reserving the JR0.
> 
> I was not able to identify which part of ATF code is responsible to program
> JR0DID_MS on imx8mm, the only thing I saw was the part where the JR0 is held
> in S World *if* the JR0DID_MS is set to 0x8011. Can you point out where is this
> performed in ATF code?
> 
> If it is not in the ATF, then my question above still stands: which component
> (HW or SW) programs JR0DID_MS, and why is it only done on imx8mm derivative?
> 
> >
> > > 2. What is the intention of having JR0 reserved for all derivatives? Is this
> > >    the part of a bigger change that stretches across different SW components
> > >    (e.g. ATF, OP-TEE, etc.)? If that is the case - then a more detailed
> > >    description would be appreciated here.
> > >
> > > ATF code already accounts for this reservation in commit:
> > > a83a7c65e ("TEE-639 plat: imx8m: Do not release JR0 to NS if HAB is using
> it")
> > > [1], but there is no description on why is this required though.
> > >
> > > If this is required for HAB feature, then the question is: should it be kept
> in
> > S
> > > World when U-Boot starts, or SPL can release it after the binary is verified
> > and
> > > crypto facilities are not in use anymore?
> >
> > Commit: a83a7c65e reserves JR0 for HAB and not released to NS but JR1, JR2 are
> > released to NS.
> 
> Then I believe this change should be in-sync with ATF implementation, because of
> the fact that your change can have any arbitrary JR to be held in S World.
> 
> What would happen if for example JR1 is programmed with TZ_OWN, but ATF releases
> it to NS world? Can it be used by Kernel afterwards? Or should the node be
> disabled here so that Kernel does not even see JR1 during boot?
> 
> So far, ATF only examines the JR0DID_MS content, and not all the others...
> 
> > HAB uses JR0 for secure boot on all i.MX8M derivatives. Uboot calls HAB API for
> > authenticating kernel.
> 
> This implies then that the JR0 is permanently held in S World and stays there for
> entire device powercycle and cannot be reclaimed in NS World? In this case, the
> DT node should be completely removed from DTB file so no SW entity can even see
> it (as it is in a total possession of HW mechanisms).
> 
> >
> > >
> > > > +       jrdid_ms = JRDID_MS_TZ_OWN | JRDID_MS_PRIM_TZ |
> > > > + JRDID_MS_PRIM_DID;
> > >
> > > What is the intention of setting JRDID_MS_PRIM_TZ? Isn't setting
> > > JRDID_MS_TZ_OWN would be sufficient here?
> >
> > PRIM_TZ bit is set to 1 to indicate that only SecureWorld can
> > access registers in that Job Ring's register page
> 
> But would it not be enough just to set TZ_OWN? If I read SRM correct: only
> TZ_OWN is enough to hold the JR in S World.
> 
> >
> 
> [snip]
> 
> > >
> 
> -- andrey

Cc: Michael Walle

-- andrey
Gaurav Jain Nov. 22, 2021, 7:29 a.m. UTC | #6 
Hello Andrey

> -----Original Message-----
> From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> Sent: Wednesday, November 17, 2021 6:33 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>;
> Peng Fan <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka
> Jain <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> <franck.lenormand@nxp.com>; Silvano Di Ninno
> <silvano.dininno@nxp.com>; Sahil Malhotra <sahil.malhotra@nxp.com>;
> Pankaj Gupta <pankaj.gupta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> dl-uboot-imx <uboot-imx@nxp.com>; Shengzhou Liu
> <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Rajesh
> Bhagat <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>; Adrian
> Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job
> ring driver model
> 
> Caution: EXT Email
> 
> Hello Gaurav,
> 
> > -----Original Message-----
> > From: Gaurav Jain <gaurav.jain@nxp.com>
> > Sent: Wednesday, November 17, 2021 12:26 PM
> > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; u-
> > boot@lists.denx.de
> > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>;
> Silvano
> > Di Ninno <silvano.dininno@nxp.com>; Sahil Malhotra
> > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>;
> Varun
> > Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>;
> Shengzhou
> > Liu <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>;
> Rajesh
> > Bhagat <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> Alison
> > Wang <alison.wang@nxp.com>; Pramod Kumar
> <pramod.kumar_1@nxp.com>;
> > Andy Tang <andy.tang@nxp.com>; Adrian Alonso
> <adrian.alonso@nxp.com>;
> > Vladimir Oltean <olteanv@gmail.com>
> > Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for
> > CAAM Job ring driver model
> >
> >
> > Hello Andrey
> >
> > > -----Original Message-----
> > > From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> > > Sent: Tuesday, November 16, 2021 9:24 PM
> > > To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>;
> > > Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil Malhotra
> > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>;
> Varun
> > > Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>;
> Shengzhou
> > > Liu <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>;
> Rajesh
> > > Bhagat <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal
> > > <meenakshi.aggarwal@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> > > Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> > > <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>;
> Adrian
> > > Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> > > Subject: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM
> > > Job ring driver model
> > >
> > > Caution: EXT Email
> > >
> > > Hello Gaurav,
> > >
> > > > -----Original Message-----
> > > > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Gaurav
> > > > Jain
> > > > Sent: Monday, November 15, 2021 8:00 AM
> > > > To: u-boot@lists.denx.de
> > > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > > > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > > > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>;
> > > > Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil malhotra
> > > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>;
> > > > Varun Sethi <V.Sethi@nxp.com>; NXP i . MX U-Boot Team
> > > > <uboot-imx@nxp.com>; Shengzhou Liu <Shengzhou.Liu@nxp.com>;
> > > > Mingkai Hu <mingkai.hu@nxp.com>; Rajesh Bhagat
> > > > <rajesh.bhagat@nxp.com>;
> > > Meenakshi
> > > > Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim Khan
> > > > <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>;
> Pramod
> > > Kumar
> > > > <pramod.kumar_1@nxp.com>; Tang Yuantian <andy.tang@nxp.com>;
> > > > Adrian Alonso <adrian.alonso@nxp.com>; Vladimir Oltean
> > > > <olteanv@gmail.com>; Gaurav Jain <gaurav.jain@nxp.com>
> > > > Subject: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job
> > > > ring driver model
> > > >
> > > >
> > > > added device tree support for job ring driver.
> > > > sec is initialized based on job ring information processed from
> > > > device tree.
> > > >
> > > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > > > Reviewed-by: Ye Li <ye.li@nxp.com>
> > > > ---
> > > >  cmd/Kconfig                 |   1 +
> > > >  drivers/crypto/fsl/Kconfig  |   7 +
> > > >  drivers/crypto/fsl/Makefile |   4 +-
> > > >  drivers/crypto/fsl/jr.c     | 316 +++++++++++++++++++++++-------------
> > > >  drivers/crypto/fsl/jr.h     |  14 ++
> > > >  5 files changed, 232 insertions(+), 110 deletions(-)
> > > >
> 
> [snip]
> 
> > > >         sec_out32(&sec->mcfgr, mcr);
> > > > +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
> > >
> > > This would effectively reserve the JR0 on _all_ i.MX8M derivatives is S
> World.
> > This code is to set any JR DID in SPL so that the job ring can be configured.
> >
> > >
> > > Current implementation only has JR0 reserved in S World on imx8mm
> > > derivative, but this new addition extends this to imx8mn, imx8mp and
> imx8mq.
> > Current implementation do not initialize CAAM for i.MX8M derivatives.
> > It is not based on driver model approach and only using JR0.
> 
> OK, but then I do not have on explanation on why do I see following results
> from reading JRaDID_MS registers on imx8m derivatives:
> - imx8mm:
>         JR0DID_MS = 0x8011
>         JR1DID_MS = 0x0
>         JR2DID_MS = 0x0
> - imx8mn:
>         JR0DID_MS = 0x0
>         JR1DID_MS = 0x0
>         JR2DID_MS = 0x0
> - imx8mp:
>         JR0DID_MS = 0x0
>         JR1DID_MS = 0x0
>         JR2DID_MS = 0x0
> 
> This readout is taken at Kernel boot, and it clearly shows that only JR0 has
> TZ_OWN, PRIM_TZ and PRIM_DID bits set, and it is only done on imx8mm.

HAB is a code that is part of the ROM code which set the JR DID for all i.mx8M.
I took the dumps on SPL boot which actually shows the JR DID set by HAB.
Dump taken by you on kernel boot does not show the values set by ROM.
IMX8MM
JR0DID_MS = 0x8011
JR1DID_MS = 0x8011
JR2DID_MS = 0x0

IMX8MN
JR0DID_MS = 0x8011
JR1DID_MS = 0x8011
JR2DID_MS = 0x0

IMX8MP
JR0DID_MS = 0x8011
JR1DID_MS = 0x8011
JR2DID_MS = 0x0
> 
> > With New implementation CAAM is enabled for i.MX8M derivative. Any JR
> > whose DID is written in ATF, can be used in Uboot.
> > JR0 is reserved for HAB so JR1 will be used for all i.MX8M derivatives.
> >
> > >
> > > I'm wondering about several points here:
> > > 1. Why does current implementation on have this reservation done on
> > > imx8mm and
> > >    where does this happen? None of the code pieces suggests that it is
> done in
> > >    U-Boot, is it performed in BootROM?
> >
> > I cannot see if current implementation(SPL/Uboot) has reservation done
> > for imx8mm.
> > In ATF, we are reserving the JR0.
> 
> I was not able to identify which part of ATF code is responsible to program
> JR0DID_MS on imx8mm, the only thing I saw was the part where the JR0 is
> held in S World *if* the JR0DID_MS is set to 0x8011. Can you point out where
> is this performed in ATF code?
> 
> If it is not in the ATF, then my question above still stands: which component
> (HW or SW) programs JR0DID_MS, and why is it only done on imx8mm
> derivative?
HAB which is part of the ROM code sets the JR DID for all i.mx8M.
> 
> >
> > > 2. What is the intention of having JR0 reserved for all derivatives? Is this
> > >    the part of a bigger change that stretches across different SW
> components
> > >    (e.g. ATF, OP-TEE, etc.)? If that is the case - then a more detailed
> > >    description would be appreciated here.
> > >
> > > ATF code already accounts for this reservation in commit:
> > > a83a7c65e ("TEE-639 plat: imx8m: Do not release JR0 to NS if HAB is
> > > using it") [1], but there is no description on why is this required though.
> > >
> > > If this is required for HAB feature, then the question is: should it
> > > be kept in
> > S
> > > World when U-Boot starts, or SPL can release it after the binary is
> > > verified
> > and
> > > crypto facilities are not in use anymore?
> >
> > Commit: a83a7c65e reserves JR0 for HAB and not released to NS but JR1,
> > JR2 are released to NS.
> 
> Then I believe this change should be in-sync with ATF implementation,
> because of the fact that your change can have any arbitrary JR to be held in S
> World.
> 
> What would happen if for example JR1 is programmed with TZ_OWN, but
> ATF releases it to NS world? Can it be used by Kernel afterwards? Or should
> the node be disabled here so that Kernel does not even see JR1 during boot?
>
Since JR0 is marked as disabled in DT, so SPL is only accessing single job ring and setting the JR1 DID as 0x8011.
After SPL boots successfully, ATF is releasing JR1 and JR2 to NS by modifying the JRDID_MS as 0x1.
Uboot is also accessing single jobring which is JR1.
JR0 is only reserved for secure boot.

> So far, ATF only examines the JR0DID_MS content, and not all the others...
> 
> > HAB uses JR0 for secure boot on all i.MX8M derivatives. Uboot calls
> > HAB API for authenticating kernel.
> 
> This implies then that the JR0 is permanently held in S World and stays there
> for entire device powercycle and cannot be reclaimed in NS World?
Yes JR0 is held in S world.

 In this
> case, the DT node should be completely removed from DTB file so no SW
> entity can even see it (as it is in a total possession of HW mechanisms).
> 
We can consider this change after this patch series is merged.
Currently I have disabled the JR0 in device tree.

> >
> > >
> > > > +       jrdid_ms = JRDID_MS_TZ_OWN | JRDID_MS_PRIM_TZ |
> > > > + JRDID_MS_PRIM_DID;
> > >
> > > What is the intention of setting JRDID_MS_PRIM_TZ? Isn't setting
> > > JRDID_MS_TZ_OWN would be sufficient here?
> >
> > PRIM_TZ bit is set to 1 to indicate that only SecureWorld can access
> > registers in that Job Ring's register page
> 
> But would it not be enough just to set TZ_OWN? If I read SRM correct: only
> TZ_OWN is enough to hold the JR in S World.
> 
HAB is also setting 0x8011 as JR DID. It is better to be in sync with HAB.

Regards
Gaurav Jain

> >
> 
> [snip]
> 
> > >
> 
> -- andrey
ZHIZHIKIN Andrey Nov. 22, 2021, 5:20 p.m. UTC | #7 
Hello Gaurav,

> -----Original Message-----
> From: Gaurav Jain <gaurav.jain@nxp.com>
> Sent: Monday, November 22, 2021 8:29 AM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; u-
> boot@lists.denx.de
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; Peng Fan
> <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka Jain
> <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> <franck.lenormand@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil
> Malhotra <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>; Shengzhou Liu
> <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Rajesh Bhagat
> <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim
> Khan <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>; Adrian Alonso
> <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> driver model
> 
> 
> Hello Andrey
> 
> > -----Original Message-----
> > From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> > Sent: Wednesday, November 17, 2021 6:33 PM
> > To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>;
> > Peng Fan <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka
> > Jain <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> > <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> > <franck.lenormand@nxp.com>; Silvano Di Ninno
> > <silvano.dininno@nxp.com>; Sahil Malhotra <sahil.malhotra@nxp.com>;
> > Pankaj Gupta <pankaj.gupta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> > dl-uboot-imx <uboot-imx@nxp.com>; Shengzhou Liu
> > <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Rajesh
> > Bhagat <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> > Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> > <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>; Adrian
> > Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> > Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job
> > ring driver model
> >
> > Caution: EXT Email
> >
> > Hello Gaurav,
> >
> > > -----Original Message-----
> > > From: Gaurav Jain <gaurav.jain@nxp.com>
> > > Sent: Wednesday, November 17, 2021 12:26 PM
> > > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; u-
> > > boot@lists.denx.de
> > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>;
> > Silvano
> > > Di Ninno <silvano.dininno@nxp.com>; Sahil Malhotra
> > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>;
> > Varun
> > > Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>;
> > Shengzhou
> > > Liu <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>;
> > Rajesh
> > > Bhagat <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal
> > > <meenakshi.aggarwal@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> > Alison
> > > Wang <alison.wang@nxp.com>; Pramod Kumar
> > <pramod.kumar_1@nxp.com>;
> > > Andy Tang <andy.tang@nxp.com>; Adrian Alonso
> > <adrian.alonso@nxp.com>;
> > > Vladimir Oltean <olteanv@gmail.com>
> > > Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for
> > > CAAM Job ring driver model
> > >
> > >
> > > Hello Andrey
> > >
> > > > -----Original Message-----
> > > > From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> > > > Sent: Tuesday, November 16, 2021 9:24 PM
> > > > To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> > > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > > > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > > > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>;
> > > > Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil Malhotra
> > > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>;
> > Varun
> > > > Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>;
> > Shengzhou
> > > > Liu <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>;
> > Rajesh
> > > > Bhagat <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal
> > > > <meenakshi.aggarwal@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> > > > Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> > > > <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>;
> > Adrian
> > > > Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> > > > Subject: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM
> > > > Job ring driver model
> > > >
> > > > Caution: EXT Email
> > > >
> > > > Hello Gaurav,
> > > >
> > > > > -----Original Message-----
> > > > > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Gaurav
> > > > > Jain
> > > > > Sent: Monday, November 15, 2021 8:00 AM
> > > > > To: u-boot@lists.denx.de
> > > > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > > > > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > > > > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>;
> > > > > Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil malhotra
> > > > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>;
> > > > > Varun Sethi <V.Sethi@nxp.com>; NXP i . MX U-Boot Team
> > > > > <uboot-imx@nxp.com>; Shengzhou Liu <Shengzhou.Liu@nxp.com>;
> > > > > Mingkai Hu <mingkai.hu@nxp.com>; Rajesh Bhagat
> > > > > <rajesh.bhagat@nxp.com>;
> > > > Meenakshi
> > > > > Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim Khan
> > > > > <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>;
> > Pramod
> > > > Kumar
> > > > > <pramod.kumar_1@nxp.com>; Tang Yuantian <andy.tang@nxp.com>;
> > > > > Adrian Alonso <adrian.alonso@nxp.com>; Vladimir Oltean
> > > > > <olteanv@gmail.com>; Gaurav Jain <gaurav.jain@nxp.com>
> > > > > Subject: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job
> > > > > ring driver model
> > > > >
> > > > >
> > > > > added device tree support for job ring driver.
> > > > > sec is initialized based on job ring information processed from
> > > > > device tree.
> > > > >
> > > > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > > > > Reviewed-by: Ye Li <ye.li@nxp.com>
> > > > > ---
> > > > >  cmd/Kconfig                 |   1 +
> > > > >  drivers/crypto/fsl/Kconfig  |   7 +
> > > > >  drivers/crypto/fsl/Makefile |   4 +-
> > > > >  drivers/crypto/fsl/jr.c     | 316 +++++++++++++++++++++++-------------
> > > > >  drivers/crypto/fsl/jr.h     |  14 ++
> > > > >  5 files changed, 232 insertions(+), 110 deletions(-)
> > > > >
> >
> > [snip]
> >
> > > > >         sec_out32(&sec->mcfgr, mcr);
> > > > > +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
> > > >
> > > > This would effectively reserve the JR0 on _all_ i.MX8M derivatives is S
> > World.
> > > This code is to set any JR DID in SPL so that the job ring can be configured.
> > >
> > > >
> > > > Current implementation only has JR0 reserved in S World on imx8mm
> > > > derivative, but this new addition extends this to imx8mn, imx8mp and
> > imx8mq.
> > > Current implementation do not initialize CAAM for i.MX8M derivatives.
> > > It is not based on driver model approach and only using JR0.
> >
> > OK, but then I do not have on explanation on why do I see following results
> > from reading JRaDID_MS registers on imx8m derivatives:
> > - imx8mm:
> >         JR0DID_MS = 0x8011
> >         JR1DID_MS = 0x0
> >         JR2DID_MS = 0x0
> > - imx8mn:
> >         JR0DID_MS = 0x0
> >         JR1DID_MS = 0x0
> >         JR2DID_MS = 0x0
> > - imx8mp:
> >         JR0DID_MS = 0x0
> >         JR1DID_MS = 0x0
> >         JR2DID_MS = 0x0
> >
> > This readout is taken at Kernel boot, and it clearly shows that only JR0 has
> > TZ_OWN, PRIM_TZ and PRIM_DID bits set, and it is only done on imx8mm.
> 
> HAB is a code that is part of the ROM code which set the JR DID for all i.mx8M.
> I took the dumps on SPL boot which actually shows the JR DID set by HAB.
> Dump taken by you on kernel boot does not show the values set by ROM.
> IMX8MM
> JR0DID_MS = 0x8011
> JR1DID_MS = 0x8011
> JR2DID_MS = 0x0
> 
> IMX8MN
> JR0DID_MS = 0x8011
> JR1DID_MS = 0x8011
> JR2DID_MS = 0x0
> 
> IMX8MP
> JR0DID_MS = 0x8011
> JR1DID_MS = 0x8011
> JR2DID_MS = 0x0

This is an interesting piece of information, thanks a lot for the readout! So
it does look like that BootROM on all derivatives reserves JR0 and JR1 at the
beginning, letting the ATF to release only JR1 to NS world...

Does IMX8MQ have the same reservation as well?

> >
> > > With New implementation CAAM is enabled for i.MX8M derivative. Any JR
> > > whose DID is written in ATF, can be used in Uboot.
> > > JR0 is reserved for HAB so JR1 will be used for all i.MX8M derivatives.
> > >
> > > >
> > > > I'm wondering about several points here:
> > > > 1. Why does current implementation on have this reservation done on
> > > > imx8mm and
> > > >    where does this happen? None of the code pieces suggests that it is
> > done in
> > > >    U-Boot, is it performed in BootROM?
> > >
> > > I cannot see if current implementation(SPL/Uboot) has reservation done
> > > for imx8mm.
> > > In ATF, we are reserving the JR0.
> >
> > I was not able to identify which part of ATF code is responsible to program
> > JR0DID_MS on imx8mm, the only thing I saw was the part where the JR0 is
> > held in S World *if* the JR0DID_MS is set to 0x8011. Can you point out where
> > is this performed in ATF code?
> >
> > If it is not in the ATF, then my question above still stands: which component
> > (HW or SW) programs JR0DID_MS, and why is it only done on imx8mm
> > derivative?
> HAB which is part of the ROM code sets the JR DID for all i.mx8M.
> >
> > >
> > > > 2. What is the intention of having JR0 reserved for all derivatives? Is
> this
> > > >    the part of a bigger change that stretches across different SW
> > components
> > > >    (e.g. ATF, OP-TEE, etc.)? If that is the case - then a more detailed
> > > >    description would be appreciated here.
> > > >
> > > > ATF code already accounts for this reservation in commit:
> > > > a83a7c65e ("TEE-639 plat: imx8m: Do not release JR0 to NS if HAB is
> > > > using it") [1], but there is no description on why is this required though.
> > > >
> > > > If this is required for HAB feature, then the question is: should it
> > > > be kept in
> > > S
> > > > World when U-Boot starts, or SPL can release it after the binary is
> > > > verified
> > > and
> > > > crypto facilities are not in use anymore?
> > >
> > > Commit: a83a7c65e reserves JR0 for HAB and not released to NS but JR1,
> > > JR2 are released to NS.
> >
> > Then I believe this change should be in-sync with ATF implementation,
> > because of the fact that your change can have any arbitrary JR to be held in S
> > World.
> >
> > What would happen if for example JR1 is programmed with TZ_OWN, but
> > ATF releases it to NS world? Can it be used by Kernel afterwards? Or should
> > the node be disabled here so that Kernel does not even see JR1 during boot?
> >
> Since JR0 is marked as disabled in DT, so SPL is only accessing single job ring
> and setting the JR1 DID as 0x8011.
> After SPL boots successfully, ATF is releasing JR1 and JR2 to NS by modifying the
> JRDID_MS as 0x1.
> Uboot is also accessing single jobring which is JR1.
> JR0 is only reserved for secure boot.

Is it safe to assume that JR1 is then accessible from both S and NS Worlds?

If that is the case, then that would actually mean that JRx status on DT should
be set as following:

&sec_jr0 {
	status = "disabled";
	secure-status = "okay";
};

&sec_jr1 {
	secure-status = "okay";
};

&sec_jr2 {
	secure-status = "disabled";
};

This would effectively mean:
JR0 - S-only,
JR1 - visible in both
JR2 - NS-only

Please note, that as this configuration is applicable to both Kernel and U-Boot -
the above block should be defined in Kernel DT for all i.MX8M derivatives, and
picked up with the next U-Boot DTB re-sync.

As I'm working on V3 for CAAM clean-up in the Kernel [1] - I can submit those
configuration changes, but I would need a confirmation from you if this is an
expected JR configuration, and whether IMX8MQ have the same setup.

> 
> > So far, ATF only examines the JR0DID_MS content, and not all the others...
> >
> > > HAB uses JR0 for secure boot on all i.MX8M derivatives. Uboot calls
> > > HAB API for authenticating kernel.
> >
> > This implies then that the JR0 is permanently held in S World and stays there
> > for entire device powercycle and cannot be reclaimed in NS World?
> Yes JR0 is held in S world.
> 
>  In this
> > case, the DT node should be completely removed from DTB file so no SW
> > entity can even see it (as it is in a total possession of HW mechanisms).
> >
> We can consider this change after this patch series is merged.
> Currently I have disabled the JR0 in device tree.

I guess with the proposed DT configuration this point would be covered as
well, isn't it? There would be no need to remove the node, as it would be
marked disabled in NS and enabled in S Worlds. I believe it is better to
set the status as I proposed, because that information in DT is transparent
for everyone (removing node raises questions regarding HW availability to me).

> 
> > >
> > > >
> > > > > +       jrdid_ms = JRDID_MS_TZ_OWN | JRDID_MS_PRIM_TZ |
> > > > > + JRDID_MS_PRIM_DID;
> > > >
> > > > What is the intention of setting JRDID_MS_PRIM_TZ? Isn't setting
> > > > JRDID_MS_TZ_OWN would be sufficient here?
> > >
> > > PRIM_TZ bit is set to 1 to indicate that only SecureWorld can access
> > > registers in that Job Ring's register page
> >
> > But would it not be enough just to set TZ_OWN? If I read SRM correct: only
> > TZ_OWN is enough to hold the JR in S World.
> >
> HAB is also setting 0x8011 as JR DID. It is better to be in sync with HAB.

Do you know what is the reason for HAB to set PRIM_TZ bit? Is there any
specific reason for this?

> 
> Regards
> Gaurav Jain
> 
> > >
> >
> > [snip]
> >
> > > >
> >
> > -- andrey

-- andrey

Link: [1]: https://lore.kernel.org/lkml/20211111164601.13135-1-andrey.zhizhikin@leica-geosystems.com/
Gaurav Jain Nov. 23, 2021, 7:22 a.m. UTC | #8 
Hello Andrey

> -----Original Message-----
> From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> Sent: Monday, November 22, 2021 10:51 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>;
> Peng Fan <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka
> Jain <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> <franck.lenormand@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>;
> Sahil Malhotra <sahil.malhotra@nxp.com>; Pankaj Gupta
> <pankaj.gupta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; dl-uboot-imx
> <uboot-imx@nxp.com>; Shengzhou Liu <shengzhou.liu@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; Rajesh Bhagat <rajesh.bhagat@nxp.com>; Meenakshi
> Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim Khan
> <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod
> Kumar <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>;
> Adrian Alonso <adrian.alonso@nxp.com>; Vladimir Oltean
> <olteanv@gmail.com>; Michael Walle <michael@walle.cc>
> Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job
> ring driver model
> 
> Caution: EXT Email
> 
> Hello Gaurav,
> 
> > -----Original Message-----
> > From: Gaurav Jain <gaurav.jain@nxp.com>
> > Sent: Monday, November 22, 2021 8:29 AM
> > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; u-
> > boot@lists.denx.de
> > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>; Silvano
> > Di Ninno <silvano.dininno@nxp.com>; Sahil Malhotra
> > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> > Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>; Shengzhou
> > Liu <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Rajesh
> > Bhagat <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> Alison
> > Wang <alison.wang@nxp.com>; Pramod Kumar
> <pramod.kumar_1@nxp.com>;
> > Andy Tang <andy.tang@nxp.com>; Adrian Alonso <adrian.alonso@nxp.com>;
> > Vladimir Oltean <olteanv@gmail.com>
> > Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for
> > CAAM Job ring driver model
> >
> >
> > Hello Andrey
> >
> > > -----Original Message-----
> > > From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> > > Sent: Wednesday, November 17, 2021 6:33 PM
> > > To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>;
> > > Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil Malhotra
> > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> > > Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>; Shengzhou
> > > Liu <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Rajesh
> > > Bhagat <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal
> > > <meenakshi.aggarwal@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> > > Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> > > <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>; Adrian
> > > Alonso <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>
> > > Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for
> > > CAAM Job ring driver model
> > >
> > > Caution: EXT Email
> > >
> > > Hello Gaurav,
> > >
> > > > -----Original Message-----
> > > > From: Gaurav Jain <gaurav.jain@nxp.com>
> > > > Sent: Wednesday, November 17, 2021 12:26 PM
> > > > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; u-
> > > > boot@lists.denx.de
> > > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > > > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > > > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>;
> > > Silvano
> > > > Di Ninno <silvano.dininno@nxp.com>; Sahil Malhotra
> > > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>;
> > > Varun
> > > > Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>;
> > > Shengzhou
> > > > Liu <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>;
> > > Rajesh
> > > > Bhagat <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal
> > > > <meenakshi.aggarwal@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> > > Alison
> > > > Wang <alison.wang@nxp.com>; Pramod Kumar
> > > <pramod.kumar_1@nxp.com>;
> > > > Andy Tang <andy.tang@nxp.com>; Adrian Alonso
> > > <adrian.alonso@nxp.com>;
> > > > Vladimir Oltean <olteanv@gmail.com>
> > > > Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support
> > > > for CAAM Job ring driver model
> > > >
> > > >
> > > > Hello Andrey
> > > >
> > > > > -----Original Message-----
> > > > > From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> > > > > Sent: Tuesday, November 16, 2021 9:24 PM
> > > > > To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de
> > > > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye Li
> > > > > <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji Luo
> > > > > <ji.luo@nxp.com>; Franck Lenormand <franck.lenormand@nxp.com>;
> > > > > Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil Malhotra
> > > > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>;
> > > Varun
> > > > > Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>;
> > > Shengzhou
> > > > > Liu <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>;
> > > Rajesh
> > > > > Bhagat <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal
> > > > > <meenakshi.aggarwal@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> > > > > Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> > > > > <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>;
> > > Adrian
> > > > > Alonso <adrian.alonso@nxp.com>; Vladimir Oltean
> > > > > <olteanv@gmail.com>
> > > > > Subject: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for
> > > > > CAAM Job ring driver model
> > > > >
> > > > > Caution: EXT Email
> > > > >
> > > > > Hello Gaurav,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of
> > > > > > Gaurav Jain
> > > > > > Sent: Monday, November 15, 2021 8:00 AM
> > > > > > To: u-boot@lists.denx.de
> > > > > > Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam
> > > > > > <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; Simon Glass
> > > > > > <sjg@chromium.org>; Priyanka Jain <priyanka.jain@nxp.com>; Ye
> > > > > > Li <ye.li@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Ji
> > > > > > Luo <ji.luo@nxp.com>; Franck Lenormand
> > > > > > <franck.lenormand@nxp.com>; Silvano Di Ninno
> > > > > > <silvano.dininno@nxp.com>; Sahil malhotra
> > > > > > <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>;
> > > > > > Varun Sethi <V.Sethi@nxp.com>; NXP i . MX U-Boot Team
> > > > > > <uboot-imx@nxp.com>; Shengzhou Liu <Shengzhou.Liu@nxp.com>;
> > > > > > Mingkai Hu <mingkai.hu@nxp.com>; Rajesh Bhagat
> > > > > > <rajesh.bhagat@nxp.com>;
> > > > > Meenakshi
> > > > > > Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim Khan
> > > > > > <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>;
> > > Pramod
> > > > > Kumar
> > > > > > <pramod.kumar_1@nxp.com>; Tang Yuantian <andy.tang@nxp.com>;
> > > > > > Adrian Alonso <adrian.alonso@nxp.com>; Vladimir Oltean
> > > > > > <olteanv@gmail.com>; Gaurav Jain <gaurav.jain@nxp.com>
> > > > > > Subject: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job
> > > > > > ring driver model
> > > > > >
> > > > > >
> > > > > > added device tree support for job ring driver.
> > > > > > sec is initialized based on job ring information processed
> > > > > > from device tree.
> > > > > >
> > > > > > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > > > > > Reviewed-by: Ye Li <ye.li@nxp.com>
> > > > > > ---
> > > > > >  cmd/Kconfig                 |   1 +
> > > > > >  drivers/crypto/fsl/Kconfig  |   7 +
> > > > > >  drivers/crypto/fsl/Makefile |   4 +-
> > > > > >  drivers/crypto/fsl/jr.c     | 316 +++++++++++++++++++++++-------------
> > > > > >  drivers/crypto/fsl/jr.h     |  14 ++
> > > > > >  5 files changed, 232 insertions(+), 110 deletions(-)
> > > > > >
> > >
> > > [snip]
> > >
> > > > > >         sec_out32(&sec->mcfgr, mcr);
> > > > > > +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
> > > > >
> > > > > This would effectively reserve the JR0 on _all_ i.MX8M
> > > > > derivatives is S
> > > World.
> > > > This code is to set any JR DID in SPL so that the job ring can be configured.
> > > >
> > > > >
> > > > > Current implementation only has JR0 reserved in S World on
> > > > > imx8mm derivative, but this new addition extends this to imx8mn,
> > > > > imx8mp and
> > > imx8mq.
> > > > Current implementation do not initialize CAAM for i.MX8M derivatives.
> > > > It is not based on driver model approach and only using JR0.
> > >
> > > OK, but then I do not have on explanation on why do I see following
> > > results from reading JRaDID_MS registers on imx8m derivatives:
> > > - imx8mm:
> > >         JR0DID_MS = 0x8011
> > >         JR1DID_MS = 0x0
> > >         JR2DID_MS = 0x0
> > > - imx8mn:
> > >         JR0DID_MS = 0x0
> > >         JR1DID_MS = 0x0
> > >         JR2DID_MS = 0x0
> > > - imx8mp:
> > >         JR0DID_MS = 0x0
> > >         JR1DID_MS = 0x0
> > >         JR2DID_MS = 0x0
> > >
> > > This readout is taken at Kernel boot, and it clearly shows that only
> > > JR0 has TZ_OWN, PRIM_TZ and PRIM_DID bits set, and it is only done on
> imx8mm.
> >
> > HAB is a code that is part of the ROM code which set the JR DID for all i.mx8M.
> > I took the dumps on SPL boot which actually shows the JR DID set by HAB.
> > Dump taken by you on kernel boot does not show the values set by ROM.
> > IMX8MM
> > JR0DID_MS = 0x8011
> > JR1DID_MS = 0x8011
> > JR2DID_MS = 0x0
> >
> > IMX8MN
> > JR0DID_MS = 0x8011
> > JR1DID_MS = 0x8011
> > JR2DID_MS = 0x0
> >
> > IMX8MP
> > JR0DID_MS = 0x8011
> > JR1DID_MS = 0x8011
> > JR2DID_MS = 0x0
> 
> This is an interesting piece of information, thanks a lot for the readout! So it
> does look like that BootROM on all derivatives reserves JR0 and JR1 at the
> beginning, letting the ATF to release only JR1 to NS world...
> 
> Does IMX8MQ have the same reservation as well?
> 
> > >
> > > > With New implementation CAAM is enabled for i.MX8M derivative. Any
> > > > JR whose DID is written in ATF, can be used in Uboot.
> > > > JR0 is reserved for HAB so JR1 will be used for all i.MX8M derivatives.
> > > >
> > > > >
> > > > > I'm wondering about several points here:
> > > > > 1. Why does current implementation on have this reservation done
> > > > > on imx8mm and
> > > > >    where does this happen? None of the code pieces suggests that
> > > > > it is
> > > done in
> > > > >    U-Boot, is it performed in BootROM?
> > > >
> > > > I cannot see if current implementation(SPL/Uboot) has reservation
> > > > done for imx8mm.
> > > > In ATF, we are reserving the JR0.
> > >
> > > I was not able to identify which part of ATF code is responsible to
> > > program JR0DID_MS on imx8mm, the only thing I saw was the part where
> > > the JR0 is held in S World *if* the JR0DID_MS is set to 0x8011. Can
> > > you point out where is this performed in ATF code?
> > >
> > > If it is not in the ATF, then my question above still stands: which
> > > component (HW or SW) programs JR0DID_MS, and why is it only done on
> > > imx8mm derivative?
> > HAB which is part of the ROM code sets the JR DID for all i.mx8M.
> > >
> > > >
> > > > > 2. What is the intention of having JR0 reserved for all
> > > > > derivatives? Is
> > this
> > > > >    the part of a bigger change that stretches across different
> > > > > SW
> > > components
> > > > >    (e.g. ATF, OP-TEE, etc.)? If that is the case - then a more detailed
> > > > >    description would be appreciated here.
> > > > >
> > > > > ATF code already accounts for this reservation in commit:
> > > > > a83a7c65e ("TEE-639 plat: imx8m: Do not release JR0 to NS if HAB
> > > > > is using it") [1], but there is no description on why is this required though.
> > > > >
> > > > > If this is required for HAB feature, then the question is:
> > > > > should it be kept in
> > > > S
> > > > > World when U-Boot starts, or SPL can release it after the binary
> > > > > is verified
> > > > and
> > > > > crypto facilities are not in use anymore?
> > > >
> > > > Commit: a83a7c65e reserves JR0 for HAB and not released to NS but
> > > > JR1,
> > > > JR2 are released to NS.
> > >
> > > Then I believe this change should be in-sync with ATF
> > > implementation, because of the fact that your change can have any
> > > arbitrary JR to be held in S World.
> > >
> > > What would happen if for example JR1 is programmed with TZ_OWN, but
> > > ATF releases it to NS world? Can it be used by Kernel afterwards? Or
> > > should the node be disabled here so that Kernel does not even see JR1 during
> boot?
> > >
> > Since JR0 is marked as disabled in DT, so SPL is only accessing single
> > job ring and setting the JR1 DID as 0x8011.
> > After SPL boots successfully, ATF is releasing JR1 and JR2 to NS by
> > modifying the JRDID_MS as 0x1.
> > Uboot is also accessing single jobring which is JR1.
> > JR0 is only reserved for secure boot.
> 
> Is it safe to assume that JR1 is then accessible from both S and NS Worlds?
> 
> If that is the case, then that would actually mean that JRx status on DT should be
> set as following:
> 
> &sec_jr0 {
>         status = "disabled";
>         secure-status = "okay";
> };
> 
> &sec_jr1 {
>         secure-status = "okay";
> };
> 
> &sec_jr2 {
>         secure-status = "disabled";
> };
> 
> This would effectively mean:
> JR0 - S-only,
> JR1 - visible in both
> JR2 - NS-only
> 
> Please note, that as this configuration is applicable to both Kernel and U-Boot -
> the above block should be defined in Kernel DT for all i.MX8M derivatives, and
> picked up with the next U-Boot DTB re-sync.
> 
> As I'm working on V3 for CAAM clean-up in the Kernel [1] - I can submit those
> configuration changes, but I would need a confirmation from you if this is an
> expected JR configuration, and whether IMX8MQ have the same setup.
> 
IMX8MQ has same values.
JR0DID_MS = 0x8011
JR1DID_MS = 0x8011
JR2DID_MS = 0x0

For now we are only reserving JR0 for secure boot. JR1 DID is later modified in ATF to 0x1. JR2 can be used by OPTEE which is secure and can set the DID before accessing the JR2.
Setting secure-status as disabled for JR2 could break OPTEE.
"secure-status" property is not used in uboot CAAM driver code so how this is going to affect the caam driver working in SPL/Uboot?
I am not sure about the kernel caam driver how secure-status is processed. For kernel JR configuration I cannot confirm.
I would suggest to take the opinion from kernel caam maintainers as well.

> >
> > > So far, ATF only examines the JR0DID_MS content, and not all the others...
> > >
> > > > HAB uses JR0 for secure boot on all i.MX8M derivatives. Uboot
> > > > calls HAB API for authenticating kernel.
> > >
> > > This implies then that the JR0 is permanently held in S World and
> > > stays there for entire device powercycle and cannot be reclaimed in NS
> World?
> > Yes JR0 is held in S world.
> >
> >  In this
> > > case, the DT node should be completely removed from DTB file so no
> > > SW entity can even see it (as it is in a total possession of HW mechanisms).
> > >
> > We can consider this change after this patch series is merged.
> > Currently I have disabled the JR0 in device tree.
> 
> I guess with the proposed DT configuration this point would be covered as well,
> isn't it? There would be no need to remove the node, as it would be marked
> disabled in NS and enabled in S Worlds. I believe it is better to set the status as I
> proposed, because that information in DT is transparent for everyone (removing
> node raises questions regarding HW availability to me).

CAAM driver is used in spl, atf, optee, uboot, kernel.
Spl and uboot can work with JR1 only. For other components it will be good to have their opinion.

> 
> >
> > > >
> > > > >
> > > > > > +       jrdid_ms = JRDID_MS_TZ_OWN | JRDID_MS_PRIM_TZ |
> > > > > > + JRDID_MS_PRIM_DID;
> > > > >
> > > > > What is the intention of setting JRDID_MS_PRIM_TZ? Isn't setting
> > > > > JRDID_MS_TZ_OWN would be sufficient here?
> > > >
> > > > PRIM_TZ bit is set to 1 to indicate that only SecureWorld can
> > > > access registers in that Job Ring's register page
> > >
> > > But would it not be enough just to set TZ_OWN? If I read SRM
> > > correct: only TZ_OWN is enough to hold the JR in S World.
> > >
> > HAB is also setting 0x8011 as JR DID. It is better to be in sync with HAB.
> 
> Do you know what is the reason for HAB to set PRIM_TZ bit? Is there any
> specific reason for this?

To restrict JR register page access to Secure World, PRIM_TZ bit is set.
So later in ATF we can decide which JobRing to release to NS.

Regards
Gaurav Jain
> 
> >
> > Regards
> > Gaurav Jain
> >
> > > >
> > >
> > > [snip]
> > >
> > > > >
> > >
> > > -- andrey
> 
> -- andrey
> 
> Link: [1]:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kern
> el.org%2Flkml%2F20211111164601.13135-1-andrey.zhizhikin%40leica-
> geosystems.com%2F&amp;data=04%7C01%7Cgaurav.jain%40nxp.com%7C2266
> 10fc0dd44d2324b408d9addc6523%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637731984370210324%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&a
> mp;sdata=SMTu0Nn0SCYFQ0H6IxLo%2F9p4AkbG%2FS1E%2BD7ojMx52WQ%3D
> &amp;reserved=0
ZHIZHIKIN Andrey Nov. 23, 2021, 9:11 a.m. UTC | #9 
Hello Gaurav,

> -----Original Message-----
> From: Gaurav Jain <gaurav.jain@nxp.com>
> Sent: Tuesday, November 23, 2021 8:22 AM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; u-
> boot@lists.denx.de
> Cc: Stefano Babic <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; Peng Fan
> <peng.fan@nxp.com>; Simon Glass <sjg@chromium.org>; Priyanka Jain
> <priyanka.jain@nxp.com>; Ye Li <ye.li@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>; Franck Lenormand
> <franck.lenormand@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>; Sahil
> Malhotra <sahil.malhotra@nxp.com>; Pankaj Gupta <pankaj.gupta@nxp.com>; Varun
> Sethi <V.Sethi@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>; Shengzhou Liu
> <shengzhou.liu@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Rajesh Bhagat
> <rajesh.bhagat@nxp.com>; Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Wasim
> Khan <wasim.khan@nxp.com>; Alison Wang <alison.wang@nxp.com>; Pramod Kumar
> <pramod.kumar_1@nxp.com>; Andy Tang <andy.tang@nxp.com>; Adrian Alonso
> <adrian.alonso@nxp.com>; Vladimir Oltean <olteanv@gmail.com>; Michael Walle
> <michael@walle.cc>
> Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> driver model
> 
> 
> Hello Andrey
> 

[snip]

> > >
> > > HAB is a code that is part of the ROM code which set the JR DID for all
> i.mx8M.
> > > I took the dumps on SPL boot which actually shows the JR DID set by HAB.
> > > Dump taken by you on kernel boot does not show the values set by ROM.
> > > IMX8MM
> > > JR0DID_MS = 0x8011
> > > JR1DID_MS = 0x8011
> > > JR2DID_MS = 0x0
> > >
> > > IMX8MN
> > > JR0DID_MS = 0x8011
> > > JR1DID_MS = 0x8011
> > > JR2DID_MS = 0x0
> > >
> > > IMX8MP
> > > JR0DID_MS = 0x8011
> > > JR1DID_MS = 0x8011
> > > JR2DID_MS = 0x0
> >
> > This is an interesting piece of information, thanks a lot for the readout! So
> it
> > does look like that BootROM on all derivatives reserves JR0 and JR1 at the
> > beginning, letting the ATF to release only JR1 to NS world...
> >
> > Does IMX8MQ have the same reservation as well?
> >
> > > >
> > > > > With New implementation CAAM is enabled for i.MX8M derivative. Any
> > > > > JR whose DID is written in ATF, can be used in Uboot.
> > > > > JR0 is reserved for HAB so JR1 will be used for all i.MX8M derivatives.
> > > > >
> > > > > >
> > > > > > I'm wondering about several points here:
> > > > > > 1. Why does current implementation on have this reservation done
> > > > > > on imx8mm and
> > > > > >    where does this happen? None of the code pieces suggests that
> > > > > > it is
> > > > done in
> > > > > >    U-Boot, is it performed in BootROM?
> > > > >
> > > > > I cannot see if current implementation(SPL/Uboot) has reservation
> > > > > done for imx8mm.
> > > > > In ATF, we are reserving the JR0.
> > > >
> > > > I was not able to identify which part of ATF code is responsible to
> > > > program JR0DID_MS on imx8mm, the only thing I saw was the part where
> > > > the JR0 is held in S World *if* the JR0DID_MS is set to 0x8011. Can
> > > > you point out where is this performed in ATF code?
> > > >
> > > > If it is not in the ATF, then my question above still stands: which
> > > > component (HW or SW) programs JR0DID_MS, and why is it only done on
> > > > imx8mm derivative?
> > > HAB which is part of the ROM code sets the JR DID for all i.mx8M.
> > > >
> > > > >
> > > > > > 2. What is the intention of having JR0 reserved for all
> > > > > > derivatives? Is
> > > this
> > > > > >    the part of a bigger change that stretches across different
> > > > > > SW
> > > > components
> > > > > >    (e.g. ATF, OP-TEE, etc.)? If that is the case - then a more detailed
> > > > > >    description would be appreciated here.
> > > > > >
> > > > > > ATF code already accounts for this reservation in commit:
> > > > > > a83a7c65e ("TEE-639 plat: imx8m: Do not release JR0 to NS if HAB
> > > > > > is using it") [1], but there is no description on why is this required
> though.
> > > > > >
> > > > > > If this is required for HAB feature, then the question is:
> > > > > > should it be kept in
> > > > > S
> > > > > > World when U-Boot starts, or SPL can release it after the binary
> > > > > > is verified
> > > > > and
> > > > > > crypto facilities are not in use anymore?
> > > > >
> > > > > Commit: a83a7c65e reserves JR0 for HAB and not released to NS but
> > > > > JR1,
> > > > > JR2 are released to NS.
> > > >
> > > > Then I believe this change should be in-sync with ATF
> > > > implementation, because of the fact that your change can have any
> > > > arbitrary JR to be held in S World.
> > > >
> > > > What would happen if for example JR1 is programmed with TZ_OWN, but
> > > > ATF releases it to NS world? Can it be used by Kernel afterwards? Or
> > > > should the node be disabled here so that Kernel does not even see JR1
> during
> > boot?
> > > >
> > > Since JR0 is marked as disabled in DT, so SPL is only accessing single
> > > job ring and setting the JR1 DID as 0x8011.
> > > After SPL boots successfully, ATF is releasing JR1 and JR2 to NS by
> > > modifying the JRDID_MS as 0x1.
> > > Uboot is also accessing single jobring which is JR1.
> > > JR0 is only reserved for secure boot.
> >
> > Is it safe to assume that JR1 is then accessible from both S and NS Worlds?
> >
> > If that is the case, then that would actually mean that JRx status on DT should
> be
> > set as following:
> >
> > &sec_jr0 {
> >         status = "disabled";
> >         secure-status = "okay";
> > };
> >
> > &sec_jr1 {
> >         secure-status = "okay";
> > };
> >
> > &sec_jr2 {
> >         secure-status = "disabled";
> > };
> >
> > This would effectively mean:
> > JR0 - S-only,
> > JR1 - visible in both
> > JR2 - NS-only
> >
> > Please note, that as this configuration is applicable to both Kernel and U-Boot
> -
> > the above block should be defined in Kernel DT for all i.MX8M derivatives, and
> > picked up with the next U-Boot DTB re-sync.
> >
> > As I'm working on V3 for CAAM clean-up in the Kernel [1] - I can submit those
> > configuration changes, but I would need a confirmation from you if this is an
> > expected JR configuration, and whether IMX8MQ have the same setup.
> >
> IMX8MQ has same values.
> JR0DID_MS = 0x8011
> JR1DID_MS = 0x8011
> JR2DID_MS = 0x0
> 
> For now we are only reserving JR0 for secure boot. JR1 DID is later modified in
> ATF to 0x1. JR2 can be used by OPTEE which is secure and can set the DID before
> accessing the JR2.
> Setting secure-status as disabled for JR2 could break OPTEE.

I see no trouble here, as OPTEE does set the "secure-status" by itself if the
resource should be exclusively reserved in S World via dt_enable_secure_status()
call. What OPTEE does check is the "status" binding to identify which JR is
available, and setting secure-status = "disabled" does not imply status = "disabled".
JR device inquiry and reservation is done in caam_hal_cfg_get_jobring_dt() call,
see [1]. 

> "secure-status" property is not used in uboot CAAM driver code so how this is
> going to affect the caam driver working in SPL/Uboot?

The above snippet I proposed should be introduced in the Kernel DT, and then picked
up by U-Boot a the re-sync. It would not affect the U-Boot in any way, since the
"secure-status" property is not processed in it.

ATF uses register readout to identify which JR is held in S World, there is no impact
there as well.

OPTEE uses internal functions to set the proper secure-status, so it is beneficial to
Introduce DT bindings that it sets.

Kernel currently does not look at "secure-status" as well as U-Boot, and I'm not sure
if it is relevant for the moment.

Moreover, above snippet does reflect how the SW entities are seeing HW configurations
which comes out of the reset, isn't it?

> I am not sure about the kernel caam driver how secure-status is processed. For
> kernel JR configuration I cannot confirm.
> I would suggest to take the opinion from kernel caam maintainers as well.

I guess Horia can comment here regarding the above proposed status.

> 
> > >
> > > > So far, ATF only examines the JR0DID_MS content, and not all the others...
> > > >
> > > > > HAB uses JR0 for secure boot on all i.MX8M derivatives. Uboot
> > > > > calls HAB API for authenticating kernel.
> > > >
> > > > This implies then that the JR0 is permanently held in S World and
> > > > stays there for entire device powercycle and cannot be reclaimed in NS
> > World?
> > > Yes JR0 is held in S world.
> > >
> > >  In this
> > > > case, the DT node should be completely removed from DTB file so no
> > > > SW entity can even see it (as it is in a total possession of HW
> mechanisms).
> > > >
> > > We can consider this change after this patch series is merged.
> > > Currently I have disabled the JR0 in device tree.
> >
> > I guess with the proposed DT configuration this point would be covered as well,
> > isn't it? There would be no need to remove the node, as it would be marked
> > disabled in NS and enabled in S Worlds. I believe it is better to set the
> status as I
> > proposed, because that information in DT is transparent for everyone (removing
> > node raises questions regarding HW availability to me).
> 
> CAAM driver is used in spl, atf, optee, uboot, kernel.
> Spl and uboot can work with JR1 only. For other components it will be good to
> have their opinion.
> 
> >
> > >
> > > > >
> > > > > >
> > > > > > > +       jrdid_ms = JRDID_MS_TZ_OWN | JRDID_MS_PRIM_TZ |
> > > > > > > + JRDID_MS_PRIM_DID;
> > > > > >
> > > > > > What is the intention of setting JRDID_MS_PRIM_TZ? Isn't setting
> > > > > > JRDID_MS_TZ_OWN would be sufficient here?
> > > > >
> > > > > PRIM_TZ bit is set to 1 to indicate that only SecureWorld can
> > > > > access registers in that Job Ring's register page
> > > >
> > > > But would it not be enough just to set TZ_OWN? If I read SRM
> > > > correct: only TZ_OWN is enough to hold the JR in S World.
> > > >
> > > HAB is also setting 0x8011 as JR DID. It is better to be in sync with HAB.
> >
> > Do you know what is the reason for HAB to set PRIM_TZ bit? Is there any
> > specific reason for this?
> 
> To restrict JR register page access to Secure World, PRIM_TZ bit is set.
> So later in ATF we can decide which JobRing to release to NS.
> 
> Regards
> Gaurav Jain
> >
> > >
> > > Regards
> > > Gaurav Jain
> > >
> > > > >
> > > >
> > > > [snip]
> > > >
> > > > > >
> > > >
> > > > -- andrey
> >
> > -- andrey
> >

-- andrey

Link: [1]: https://github.com/OP-TEE/optee_os/blob/fd140f7eebbbee0c80f681b8bc1aad4b81f60194/core/drivers/crypto/caam/hal/common/hal_cfg_dt.c#L93
Gaurav Jain Nov. 30, 2021, 10:07 a.m. UTC | #10 
Hello Michael

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: Tuesday, November 16, 2021 4:32 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>
> Cc: Shengzhou Liu <shengzhou.liu@nxp.com>; Varun Sethi
> <V.Sethi@nxp.com>; Adrian Alonso <adrian.alonso@nxp.com>; Alison Wang
> <alison.wang@nxp.com>; Andy Tang <andy.tang@nxp.com>;
> festevam@gmail.com; Franck Lenormand <franck.lenormand@nxp.com>;
> Horia Geanta <horia.geanta@nxp.com>; Ji Luo <ji.luo@nxp.com>;
> Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; olteanv@gmail.com; Pankaj Gupta
> <pankaj.gupta@nxp.com>; Peng Fan <peng.fan@nxp.com>; Pramod Kumar
> <pramod.kumar_1@nxp.com>; Priyanka Jain <priyanka.jain@nxp.com>;
> Rajesh Bhagat <rajesh.bhagat@nxp.com>; Sahil Malhotra
> <sahil.malhotra@nxp.com>; sbabic@denx.de; Silvano Di Ninno
> <silvano.dininno@nxp.com>; sjg@chromium.org; u-boot@lists.denx.de; dl-
> uboot-imx <uboot-imx@nxp.com>; Wasim Khan <wasim.khan@nxp.com>;
> Ye Li <ye.li@nxp.com>; Michael Walle <michael@walle.cc>
> Subject: [EXT] Re: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job
> ring driver model
> 
> Caution: EXT Email
> 
> > diff --git a/cmd/Kconfig b/cmd/Kconfig index 5b30b13e43..2b24672505
> > 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -2009,6 +2009,7 @@ config CMD_AES
> >
> >  config CMD_BLOB
> >       bool "Enable the 'blob' command"
> > +     select FSL_BLOB
> 
> this looks wrong, because CMD_BLOB sounds like a generic command but it
> will automatically select FSL_BLOB which in turn sounds freescale specific.
> Looking at the help text, this command is (at least at the moment) freescale
> specific, but the code seems to be generic and the blob_encap() and
> blob_decap() are weak functions, thus they could be implemented in a
> different way and not just by fsl_blob.c.
> 
> I don't think this should automatically select FSL_BLOB.
Ok.. will change in next version of this series.
> 
> Also, shouldn't this be an uclass with encap and decap ops?

I agree with your suggestion. but in the context of current patch series this is not required immediately.
Will test encap and decap function after converting as uclass ops and send a separate patch.

> 
> >       depends on !MX6ULL && !MX6SLL && !MX6SL
> >       select IMX_HAB if ARCH_MX6 || ARCH_MX7 || ARCH_MX7ULP ||
> ARCH_IMX8M
> >       help
> > diff --git a/drivers/crypto/fsl/Kconfig b/drivers/crypto/fsl/Kconfig
> > index 94ff540111..ab59d516f8 100644
> > --- a/drivers/crypto/fsl/Kconfig
> > +++ b/drivers/crypto/fsl/Kconfig
> > @@ -66,4 +66,11 @@ config FSL_CAAM_RNG
> >         using the prediction resistance flag which means the DRGB is
> >         reseeded from the TRNG every time random data is generated.
> >
> > +config FSL_BLOB
> > +        bool "Enable Blob Encap/Decap, Blob KEK support"
> 
> wrong indendation?
Will be addressed in next version..

> 
> > +     help
> > +       Enable support for the hardware based crytographic blob
> encap/decap
> > +       module of the CAAM. blobs can be safely placed into non-volatile
> > +       storage. blobs can only be decapsulated by the SoC that created it.
> > +       Enable support for blob key encryption key generation.
> >  endif
diff mbox series

Patch

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 5b30b13e43..2b24672505 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2009,6 +2009,7 @@  config CMD_AES
 
 config CMD_BLOB
 	bool "Enable the 'blob' command"
+	select FSL_BLOB
 	depends on !MX6ULL && !MX6SLL && !MX6SL
 	select IMX_HAB if ARCH_MX6 || ARCH_MX7 || ARCH_MX7ULP || ARCH_IMX8M
 	help
diff --git a/drivers/crypto/fsl/Kconfig b/drivers/crypto/fsl/Kconfig
index 94ff540111..ab59d516f8 100644
--- a/drivers/crypto/fsl/Kconfig
+++ b/drivers/crypto/fsl/Kconfig
@@ -66,4 +66,11 @@  config FSL_CAAM_RNG
 	  using the prediction resistance flag which means the DRGB is
 	  reseeded from the TRNG every time random data is generated.
 
+config FSL_BLOB
+        bool "Enable Blob Encap/Decap, Blob KEK support"
+	help
+	  Enable support for the hardware based crytographic blob encap/decap
+	  module of the CAAM. blobs can be safely placed into non-volatile
+	  storage. blobs can only be decapsulated by the SoC that created it.
+	  Enable support for blob key encryption key generation.
 endif
diff --git a/drivers/crypto/fsl/Makefile b/drivers/crypto/fsl/Makefile
index f9c3ccecfc..738535b8e4 100644
--- a/drivers/crypto/fsl/Makefile
+++ b/drivers/crypto/fsl/Makefile
@@ -1,10 +1,12 @@ 
 # SPDX-License-Identifier: GPL-2.0+
 #
 # Copyright 2014 Freescale Semiconductor, Inc.
+# Copyright 2021 NXP
 
 obj-y += sec.o
 obj-$(CONFIG_FSL_CAAM) += jr.o fsl_hash.o jobdesc.o error.o
-obj-$(CONFIG_CMD_BLOB)$(CONFIG_IMX_CAAM_DEK_ENCAP) += fsl_blob.o
+obj-$(CONFIG_FSL_BLOB) += fsl_blob.o
+obj-$(CONFIG_IMX_CAAM_DEK_ENCAP) += fsl_blob.o
 obj-$(CONFIG_RSA_FREESCALE_EXP) += fsl_rsa.o
 obj-$(CONFIG_FSL_CAAM_RNG) += rng.o
 obj-$(CONFIG_FSL_MFGPROT) += fsl_mfgprot.o
diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index 22b649219e..eea2225a1e 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -1,7 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0+
 /*
  * Copyright 2008-2014 Freescale Semiconductor, Inc.
- * Copyright 2018 NXP
+ * Copyright 2018, 2021 NXP
  *
  * Based on CAAM driver in drivers/crypto/caam in Linux
  */
@@ -11,7 +11,6 @@ 
 #include <linux/kernel.h>
 #include <log.h>
 #include <malloc.h>
-#include "fsl_sec.h"
 #include "jr.h"
 #include "jobdesc.h"
 #include "desc_constr.h"
@@ -21,8 +20,11 @@ 
 #include <asm/cache.h>
 #include <asm/fsl_pamu.h>
 #endif
+#include <dm.h>
 #include <dm/lists.h>
 #include <linux/delay.h>
+#include <dm/root.h>
+#include <dm/device-internal.h>
 
 #define CIRC_CNT(head, tail, size)	(((head) - (tail)) & (size - 1))
 #define CIRC_SPACE(head, tail, size)	CIRC_CNT((tail), (head) + 1, (size))
@@ -35,20 +37,30 @@  uint32_t sec_offset[CONFIG_SYS_FSL_MAX_NUM_OF_SEC] = {
 #endif
 };
 
+#if CONFIG_IS_ENABLED(DM)
+struct udevice *caam_dev;
+#else
 #define SEC_ADDR(idx)	\
 	(ulong)((CONFIG_SYS_FSL_SEC_ADDR + sec_offset[idx]))
 
 #define SEC_JR0_ADDR(idx)	\
 	(ulong)(SEC_ADDR(idx) +	\
 	 (CONFIG_SYS_FSL_JR0_OFFSET - CONFIG_SYS_FSL_SEC_OFFSET))
+struct caam_regs caam_st;
+#endif
 
-struct jobring jr0[CONFIG_SYS_FSL_MAX_NUM_OF_SEC];
+static inline u32 jr_start_reg(u8 jrid)
+{
+	return (1 << jrid);
+}
 
-static inline void start_jr0(uint8_t sec_idx)
+#ifndef CONFIG_ARCH_IMX8
+static inline void start_jr(struct caam_regs *caam)
 {
-	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
+	ccsr_sec_t *sec = caam->sec;
 	u32 ctpr_ms = sec_in32(&sec->ctpr_ms);
 	u32 scfgr = sec_in32(&sec->scfgr);
+	u32 jrstart = jr_start_reg(caam->jrid);
 
 	if (ctpr_ms & SEC_CTPR_MS_VIRT_EN_INCL) {
 		/* VIRT_EN_INCL = 1 & VIRT_EN_POR = 1 or
@@ -56,23 +68,17 @@  static inline void start_jr0(uint8_t sec_idx)
 		 */
 		if ((ctpr_ms & SEC_CTPR_MS_VIRT_EN_POR) ||
 		    (scfgr & SEC_SCFGR_VIRT_EN))
-			sec_out32(&sec->jrstartr, CONFIG_JRSTARTR_JR0);
+			sec_out32(&sec->jrstartr, jrstart);
 	} else {
 		/* VIRT_EN_INCL = 0 && VIRT_EN_POR_VALUE = 1 */
 		if (ctpr_ms & SEC_CTPR_MS_VIRT_EN_POR)
-			sec_out32(&sec->jrstartr, CONFIG_JRSTARTR_JR0);
+			sec_out32(&sec->jrstartr, jrstart);
 	}
 }
+#endif
 
-static inline void jr_reset_liodn(uint8_t sec_idx)
-{
-	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
-	sec_out32(&sec->jrliodnr[0].ls, 0);
-}
-
-static inline void jr_disable_irq(uint8_t sec_idx)
+static inline void jr_disable_irq(struct jr_regs *regs)
 {
-	struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
 	uint32_t jrcfg = sec_in32(&regs->jrcfg1);
 
 	jrcfg = jrcfg | JR_INTMASK;
@@ -80,10 +86,10 @@  static inline void jr_disable_irq(uint8_t sec_idx)
 	sec_out32(&regs->jrcfg1, jrcfg);
 }
 
-static void jr_initregs(uint8_t sec_idx)
+static void jr_initregs(uint8_t sec_idx, struct caam_regs *caam)
 {
-	struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
-	struct jobring *jr = &jr0[sec_idx];
+	struct jr_regs *regs = caam->regs;
+	struct jobring *jr = &caam->jr[sec_idx];
 	caam_dma_addr_t ip_base = virt_to_phys((void *)jr->input_ring);
 	caam_dma_addr_t op_base = virt_to_phys((void *)jr->output_ring);
 
@@ -103,16 +109,16 @@  static void jr_initregs(uint8_t sec_idx)
 	sec_out32(&regs->irs, JR_SIZE);
 
 	if (!jr->irq)
-		jr_disable_irq(sec_idx);
+		jr_disable_irq(regs);
 }
 
-static int jr_init(uint8_t sec_idx)
+static int jr_init(uint8_t sec_idx, struct caam_regs *caam)
 {
-	struct jobring *jr = &jr0[sec_idx];
+	struct jobring *jr = &caam->jr[sec_idx];
 
 	memset(jr, 0, sizeof(struct jobring));
 
-	jr->jq_id = DEFAULT_JR_ID;
+	jr->jq_id = caam->jrid;
 	jr->irq = DEFAULT_IRQ;
 
 #ifdef CONFIG_FSL_CORENET
@@ -134,53 +140,10 @@  static int jr_init(uint8_t sec_idx)
 	memset(jr->input_ring, 0, JR_SIZE * sizeof(caam_dma_addr_t));
 	memset(jr->output_ring, 0, jr->op_size);
 
-	start_jr0(sec_idx);
-
-	jr_initregs(sec_idx);
-
-	return 0;
-}
-
-static int jr_sw_cleanup(uint8_t sec_idx)
-{
-	struct jobring *jr = &jr0[sec_idx];
-
-	jr->head = 0;
-	jr->tail = 0;
-	jr->read_idx = 0;
-	jr->write_idx = 0;
-	memset(jr->info, 0, sizeof(jr->info));
-	memset(jr->input_ring, 0, jr->size * sizeof(caam_dma_addr_t));
-	memset(jr->output_ring, 0, jr->size * sizeof(struct op_ring));
-
-	return 0;
-}
-
-static int jr_hw_reset(uint8_t sec_idx)
-{
-	struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
-	uint32_t timeout = 100000;
-	uint32_t jrint, jrcr;
-
-	sec_out32(&regs->jrcr, JRCR_RESET);
-	do {
-		jrint = sec_in32(&regs->jrint);
-	} while (((jrint & JRINT_ERR_HALT_MASK) ==
-		  JRINT_ERR_HALT_INPROGRESS) && --timeout);
-
-	jrint = sec_in32(&regs->jrint);
-	if (((jrint & JRINT_ERR_HALT_MASK) !=
-	     JRINT_ERR_HALT_INPROGRESS) && timeout == 0)
-		return -1;
-
-	timeout = 100000;
-	sec_out32(&regs->jrcr, JRCR_RESET);
-	do {
-		jrcr = sec_in32(&regs->jrcr);
-	} while ((jrcr & JRCR_RESET) && --timeout);
-
-	if (timeout == 0)
-		return -1;
+#ifndef CONFIG_ARCH_IMX8
+	start_jr(caam);
+#endif
+	jr_initregs(sec_idx, caam);
 
 	return 0;
 }
@@ -188,10 +151,10 @@  static int jr_hw_reset(uint8_t sec_idx)
 /* -1 --- error, can't enqueue -- no space available */
 static int jr_enqueue(uint32_t *desc_addr,
 	       void (*callback)(uint32_t status, void *arg),
-	       void *arg, uint8_t sec_idx)
+	       void *arg, uint8_t sec_idx, struct caam_regs *caam)
 {
-	struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
-	struct jobring *jr = &jr0[sec_idx];
+	struct jr_regs *regs = caam->regs;
+	struct jobring *jr = &caam->jr[sec_idx];
 	int head = jr->head;
 	uint32_t desc_word;
 	int length = desc_len(desc_addr);
@@ -263,10 +226,10 @@  static int jr_enqueue(uint32_t *desc_addr,
 	return 0;
 }
 
-static int jr_dequeue(int sec_idx)
+static int jr_dequeue(int sec_idx, struct caam_regs *caam)
 {
-	struct jr_regs *regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
-	struct jobring *jr = &jr0[sec_idx];
+	struct jr_regs *regs = caam->regs;
+	struct jobring *jr = &caam->jr[sec_idx];
 	int head = jr->head;
 	int tail = jr->tail;
 	int idx, i, found;
@@ -349,14 +312,18 @@  static void desc_done(uint32_t status, void *arg)
 {
 	struct result *x = arg;
 	x->status = status;
-#ifndef CONFIG_SPL_BUILD
 	caam_jr_strstatus(status);
-#endif
 	x->done = 1;
 }
 
 static inline int run_descriptor_jr_idx(uint32_t *desc, uint8_t sec_idx)
 {
+	struct caam_regs *caam;
+#if CONFIG_IS_ENABLED(DM)
+	caam = dev_get_priv(caam_dev);
+#else
+	caam = &caam_st;
+#endif
 	unsigned long long timeval = 0;
 	unsigned long long timeout = CONFIG_USEC_DEQ_TIMEOUT;
 	struct result op;
@@ -364,7 +331,7 @@  static inline int run_descriptor_jr_idx(uint32_t *desc, uint8_t sec_idx)
 
 	memset(&op, 0, sizeof(op));
 
-	ret = jr_enqueue(desc, desc_done, &op, sec_idx);
+	ret = jr_enqueue(desc, desc_done, &op, sec_idx, caam);
 	if (ret) {
 		debug("Error in SEC enq\n");
 		ret = JQ_ENQ_ERR;
@@ -375,7 +342,7 @@  static inline int run_descriptor_jr_idx(uint32_t *desc, uint8_t sec_idx)
 		udelay(1);
 		timeval += 1;
 
-		ret = jr_dequeue(sec_idx);
+		ret = jr_dequeue(sec_idx, caam);
 		if (ret) {
 			debug("Error in SEC deq\n");
 			ret = JQ_DEQ_ERR;
@@ -402,13 +369,63 @@  int run_descriptor_jr(uint32_t *desc)
 	return run_descriptor_jr_idx(desc, 0);
 }
 
+#ifndef CONFIG_ARCH_IMX8
+static int jr_sw_cleanup(uint8_t sec_idx, struct caam_regs *caam)
+{
+	struct jobring *jr = &caam->jr[sec_idx];
+
+	jr->head = 0;
+	jr->tail = 0;
+	jr->read_idx = 0;
+	jr->write_idx = 0;
+	memset(jr->info, 0, sizeof(jr->info));
+	memset(jr->input_ring, 0, jr->size * sizeof(caam_dma_addr_t));
+	memset(jr->output_ring, 0, jr->size * sizeof(struct op_ring));
+
+	return 0;
+}
+
+static int jr_hw_reset(struct jr_regs *regs)
+{
+	uint32_t timeout = 100000;
+	uint32_t jrint, jrcr;
+
+	sec_out32(&regs->jrcr, JRCR_RESET);
+	do {
+		jrint = sec_in32(&regs->jrint);
+	} while (((jrint & JRINT_ERR_HALT_MASK) ==
+		  JRINT_ERR_HALT_INPROGRESS) && --timeout);
+
+	jrint = sec_in32(&regs->jrint);
+	if (((jrint & JRINT_ERR_HALT_MASK) !=
+	     JRINT_ERR_HALT_INPROGRESS) && timeout == 0)
+		return -1;
+
+	timeout = 100000;
+	sec_out32(&regs->jrcr, JRCR_RESET);
+	do {
+		jrcr = sec_in32(&regs->jrcr);
+	} while ((jrcr & JRCR_RESET) && --timeout);
+
+	if (timeout == 0)
+		return -1;
+
+	return 0;
+}
+
 static inline int jr_reset_sec(uint8_t sec_idx)
 {
-	if (jr_hw_reset(sec_idx) < 0)
+	struct caam_regs *caam;
+#if CONFIG_IS_ENABLED(DM)
+	caam = dev_get_priv(caam_dev);
+#else
+	caam = &caam_st;
+#endif
+	if (jr_hw_reset(caam->regs) < 0)
 		return -1;
 
 	/* Clean up the jobring structure maintained by software */
-	jr_sw_cleanup(sec_idx);
+	jr_sw_cleanup(sec_idx, caam);
 
 	return 0;
 }
@@ -418,9 +435,15 @@  int jr_reset(void)
 	return jr_reset_sec(0);
 }
 
-static inline int sec_reset_idx(uint8_t sec_idx)
+int sec_reset(void)
 {
-	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
+	struct caam_regs *caam;
+#if CONFIG_IS_ENABLED(DM)
+	caam = dev_get_priv(caam_dev);
+#else
+	caam = &caam_st;
+#endif
+	ccsr_sec_t *sec = caam->sec;
 	uint32_t mcfgr = sec_in32(&sec->mcfgr);
 	uint32_t timeout = 100000;
 
@@ -446,11 +469,7 @@  static inline int sec_reset_idx(uint8_t sec_idx)
 
 	return 0;
 }
-int sec_reset(void)
-{
-	return sec_reset_idx(0);
-}
-#ifndef CONFIG_SPL_BUILD
+
 static int deinstantiate_rng(u8 sec_idx, int state_handle_mask)
 {
 	u32 *desc;
@@ -496,12 +515,11 @@  static int deinstantiate_rng(u8 sec_idx, int state_handle_mask)
 	return ret;
 }
 
-static int instantiate_rng(u8 sec_idx, int gen_sk)
+static int instantiate_rng(uint8_t sec_idx, ccsr_sec_t *sec, int gen_sk)
 {
 	u32 *desc;
 	u32 rdsta_val;
 	int ret = 0, sh_idx, size;
-	ccsr_sec_t __iomem *sec = (ccsr_sec_t __iomem *)SEC_ADDR(sec_idx);
 	struct rng4tst __iomem *rng =
 			(struct rng4tst __iomem *)&sec->rng;
 
@@ -554,9 +572,8 @@  static int instantiate_rng(u8 sec_idx, int gen_sk)
 	return ret;
 }
 
-static u8 get_rng_vid(uint8_t sec_idx)
+static u8 get_rng_vid(ccsr_sec_t *sec)
 {
-	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
 	u8 vid;
 
 	if (caam_get_era() < 10) {
@@ -574,9 +591,8 @@  static u8 get_rng_vid(uint8_t sec_idx)
  * By default, the TRNG runs for 200 clocks per sample;
  * 1200 clocks per sample generates better entropy.
  */
-static void kick_trng(int ent_delay, uint8_t sec_idx)
+static void kick_trng(int ent_delay, ccsr_sec_t *sec)
 {
-	ccsr_sec_t __iomem *sec = (ccsr_sec_t __iomem *)SEC_ADDR(sec_idx);
 	struct rng4tst __iomem *rng =
 			(struct rng4tst __iomem *)&sec->rng;
 	u32 val;
@@ -603,10 +619,9 @@  static void kick_trng(int ent_delay, uint8_t sec_idx)
 	sec_clrbits32(&rng->rtmctl, RTMCTL_PRGM);
 }
 
-static int rng_init(uint8_t sec_idx)
+static int rng_init(uint8_t sec_idx, ccsr_sec_t *sec)
 {
 	int ret, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
-	ccsr_sec_t __iomem *sec = (ccsr_sec_t __iomem *)SEC_ADDR(sec_idx);
 	struct rng4tst __iomem *rng =
 			(struct rng4tst __iomem *)&sec->rng;
 	u32 inst_handles;
@@ -624,7 +639,7 @@  static int rng_init(uint8_t sec_idx)
 		 * the TRNG parameters.
 		 */
 		if (!inst_handles) {
-			kick_trng(ent_delay, sec_idx);
+			kick_trng(ent_delay, sec);
 			ent_delay += 400;
 		}
 		/*
@@ -634,7 +649,7 @@  static int rng_init(uint8_t sec_idx)
 		 * interval, leading to a sucessful initialization of
 		 * the RNG.
 		 */
-		ret = instantiate_rng(sec_idx, gen_sk);
+		ret = instantiate_rng(sec_idx, sec, gen_sk);
 	} while ((ret == -1) && (ent_delay < RTSDCTL_ENT_DLY_MAX));
 	if (ret) {
 		printf("SEC%u:  Failed to instantiate RNG\n", sec_idx);
@@ -647,12 +662,29 @@  static int rng_init(uint8_t sec_idx)
 	return ret;
 }
 #endif
+
 int sec_init_idx(uint8_t sec_idx)
 {
-	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
-	uint32_t mcr = sec_in32(&sec->mcfgr);
 	int ret = 0;
-
+	struct caam_regs *caam;
+#if CONFIG_IS_ENABLED(DM)
+	if (caam_dev == NULL) {
+		printf("caam_jr: caam not found\n");
+		return -1;
+	}
+	caam = dev_get_priv(caam_dev);
+#else
+	caam_st.sec = (void *)SEC_ADDR(sec_idx);
+	caam_st.regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
+	caam_st.jrid = 0;
+	caam = &caam_st;
+#endif
+#ifndef CONFIG_ARCH_IMX8
+	ccsr_sec_t *sec = caam->sec;
+	uint32_t mcr = sec_in32(&sec->mcfgr);
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
+	uint32_t jrdid_ms = 0;
+#endif
 #ifdef CONFIG_FSL_CORENET
 	uint32_t liodnr;
 	uint32_t liodn_ns;
@@ -682,6 +714,11 @@  int sec_init_idx(uint8_t sec_idx)
 	mcr |= (1 << MCFGR_PS_SHIFT);
 #endif
 	sec_out32(&sec->mcfgr, mcr);
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
+	jrdid_ms = JRDID_MS_TZ_OWN | JRDID_MS_PRIM_TZ | JRDID_MS_PRIM_DID;
+	sec_out32(&sec->jrliodnr[caam->jrid].ms, jrdid_ms);
+#endif
+	jr_reset();
 
 #ifdef CONFIG_FSL_CORENET
 #ifdef CONFIG_SPL_BUILD
@@ -693,25 +730,26 @@  int sec_init_idx(uint8_t sec_idx)
 	liodn_ns = CONFIG_SPL_JR0_LIODN_NS & JRNSLIODN_MASK;
 	liodn_s = CONFIG_SPL_JR0_LIODN_S & JRSLIODN_MASK;
 
-	liodnr = sec_in32(&sec->jrliodnr[0].ls) &
+	liodnr = sec_in32(&sec->jrliodnr[caam->jrid].ls) &
 		 ~(JRNSLIODN_MASK | JRSLIODN_MASK);
 	liodnr = liodnr |
 		 (liodn_ns << JRNSLIODN_SHIFT) |
 		 (liodn_s << JRSLIODN_SHIFT);
-	sec_out32(&sec->jrliodnr[0].ls, liodnr);
+	sec_out32(&sec->jrliodnr[caam->jrid].ls, liodnr);
 #else
-	liodnr = sec_in32(&sec->jrliodnr[0].ls);
+	liodnr = sec_in32(&sec->jrliodnr[caam->jrid].ls);
 	liodn_ns = (liodnr & JRNSLIODN_MASK) >> JRNSLIODN_SHIFT;
 	liodn_s = (liodnr & JRSLIODN_MASK) >> JRSLIODN_SHIFT;
 #endif
 #endif
-
-	ret = jr_init(sec_idx);
+#endif
+	ret = jr_init(sec_idx, caam);
 	if (ret < 0) {
 		printf("SEC%u:  initialization failed\n", sec_idx);
 		return -1;
 	}
 
+#ifndef CONFIG_ARCH_IMX8
 #ifdef CONFIG_FSL_CORENET
 	ret = sec_config_pamu_table(liodn_ns, liodn_s);
 	if (ret < 0)
@@ -719,9 +757,9 @@  int sec_init_idx(uint8_t sec_idx)
 
 	pamu_enable();
 #endif
-#ifndef CONFIG_SPL_BUILD
-	if (get_rng_vid(sec_idx) >= 4) {
-		if (rng_init(sec_idx) < 0) {
+
+	if (get_rng_vid(caam->sec) >= 4) {
+		if (rng_init(sec_idx, caam->sec) < 0) {
 			printf("SEC%u:  RNG instantiation failed\n", sec_idx);
 			return -1;
 		}
@@ -743,3 +781,63 @@  int sec_init(void)
 {
 	return sec_init_idx(0);
 }
+
+#if CONFIG_IS_ENABLED(DM)
+static int caam_jr_probe(struct udevice *dev)
+{
+	struct caam_regs *caam = dev_get_priv(dev);
+	fdt_addr_t addr;
+	ofnode node;
+	unsigned int jr_node = 0;
+
+	caam_dev = dev;
+
+	addr = dev_read_addr(dev);
+	if (addr == FDT_ADDR_T_NONE) {
+		printf("caam_jr: crypto not found\n");
+		return -EINVAL;
+	}
+	caam->sec = (ccsr_sec_t *)(uintptr_t)addr;
+	caam->regs = (struct jr_regs *)caam->sec;
+
+	/* Check for enabled job ring node */
+	ofnode_for_each_subnode(node, dev_ofnode(dev)) {
+		if (!ofnode_is_available(node)) {
+			continue;
+		}
+		jr_node = ofnode_read_u32_default(node, "reg", -1);
+		if (jr_node > 0) {
+			caam->regs = (struct jr_regs *)((ulong)caam->sec + jr_node);
+			while (!(jr_node & 0x0F)) {
+				jr_node = jr_node >> 4;
+			}
+			caam->jrid = jr_node - 1;
+			break;
+		}
+	}
+
+	if (sec_init())
+		printf("\nsec_init failed!\n");
+
+	return 0;
+}
+
+static int caam_jr_bind(struct udevice *dev)
+{
+	return 0;
+}
+
+static const struct udevice_id caam_jr_match[] = {
+	{ .compatible = "fsl,sec-v4.0" },
+	{ }
+};
+
+U_BOOT_DRIVER(caam_jr) = {
+	.name		= "caam_jr",
+	.id		= UCLASS_MISC,
+	.of_match	= caam_jr_match,
+	.bind		= caam_jr_bind,
+	.probe		= caam_jr_probe,
+	.priv_auto	= sizeof(struct caam_regs),
+};
+#endif
diff --git a/drivers/crypto/fsl/jr.h b/drivers/crypto/fsl/jr.h
index 1047aa772c..43cb5e0753 100644
--- a/drivers/crypto/fsl/jr.h
+++ b/drivers/crypto/fsl/jr.h
@@ -1,6 +1,7 @@ 
 /* SPDX-License-Identifier: GPL-2.0+ */
 /*
  * Copyright 2008-2014 Freescale Semiconductor, Inc.
+ * Copyright 2021 NXP
  *
  */
 
@@ -8,7 +9,9 @@ 
 #define __JR_H
 
 #include <linux/compiler.h>
+#include "fsl_sec.h"
 #include "type.h"
+#include <misc.h>
 
 #define JR_SIZE 4
 /* Timeout currently defined as 10 sec */
@@ -35,6 +38,10 @@ 
 #define JRSLIODN_SHIFT		0
 #define JRSLIODN_MASK		0x00000fff
 
+#define JRDID_MS_PRIM_DID	1
+#define JRDID_MS_PRIM_TZ	(1 << 4)
+#define JRDID_MS_TZ_OWN		(1 << 15)
+
 #define JQ_DEQ_ERR		-1
 #define JQ_DEQ_TO_ERR		-2
 #define JQ_ENQ_ERR		-3
@@ -102,6 +109,13 @@  struct result {
 	uint32_t status;
 };
 
+struct caam_regs {
+	ccsr_sec_t *sec;
+	struct jr_regs *regs;
+	u8 jrid;
+	struct jobring jr[CONFIG_SYS_FSL_MAX_NUM_OF_SEC];
+};
+
 void caam_jr_strstatus(u32 status);
 int run_descriptor_jr(uint32_t *desc);