Hi Andrey For now I will remove this particular patch from my series and will send a separate patch addressing your comments.
Regards Gaurav Jain > -----Original Message----- > From: ZHIZHIKIN Andrey <andrey.zhizhi...@leica-geosystems.com> > Sent: Monday, January 10, 2022 7:31 PM > To: Gaurav Jain <gaurav.j...@nxp.com>; u-boot@lists.denx.de > Cc: Stefano Babic <sba...@denx.de>; Fabio Estevam <feste...@gmail.com>; > Peng Fan <peng....@nxp.com>; Simon Glass <s...@chromium.org>; Priyanka > Jain <priyanka.j...@nxp.com>; Ye Li <ye...@nxp.com>; Horia Geanta > <horia.gea...@nxp.com>; Ji Luo <ji....@nxp.com>; Franck Lenormand > <franck.lenorm...@nxp.com>; Silvano Di Ninno <silvano.dini...@nxp.com>; > Sahil Malhotra <sahil.malho...@nxp.com>; Pankaj Gupta > <pankaj.gu...@nxp.com>; Varun Sethi <v.se...@nxp.com>; dl-uboot-imx > <uboot-...@nxp.com>; Shengzhou Liu <shengzhou....@nxp.com>; Mingkai Hu > <mingkai...@nxp.com>; Rajesh Bhagat <rajesh.bha...@nxp.com>; Meenakshi > Aggarwal <meenakshi.aggar...@nxp.com>; Wasim Khan > <wasim.k...@nxp.com>; Alison Wang <alison.w...@nxp.com>; Pramod > Kumar <pramod.kuma...@nxp.com>; Andy Tang <andy.t...@nxp.com>; > Adrian Alonso <adrian.alo...@nxp.com>; Vladimir Oltean > <olte...@gmail.com>; mich...@walle.cc > Subject: [EXT] RE: [PATCH v8 10/15] crypto/fsl: Improve hwrng performance in > kernel > > Caution: EXT Email > > Hello Gaurav, > > Cc: Michael Walle > > > -----Original Message----- > > From: U-Boot <u-boot-boun...@lists.denx.de> On Behalf Of Gaurav Jain > > Sent: Monday, January 10, 2022 1:27 PM > > To: u-boot@lists.denx.de > > Cc: Stefano Babic <sba...@denx.de>; Fabio Estevam > > <feste...@gmail.com>; Peng Fan <peng....@nxp.com>; Simon Glass > > <s...@chromium.org>; Priyanka Jain <priyanka.j...@nxp.com>; Ye Li > > <ye...@nxp.com>; Horia Geanta <horia.gea...@nxp.com>; Ji Luo > > <ji....@nxp.com>; Franck Lenormand <franck.lenorm...@nxp.com>; Silvano > > Di Ninno <silvano.dini...@nxp.com>; Sahil malhotra > > <sahil.malho...@nxp.com>; Pankaj Gupta <pankaj.gu...@nxp.com>; Varun > > Sethi <v.se...@nxp.com>; NXP i . MX U-Boot Team <uboot-...@nxp.com>; > > Shengzhou Liu <shengzhou....@nxp.com>; Mingkai Hu > > <mingkai...@nxp.com>; Rajesh Bhagat <rajesh.bha...@nxp.com>; > Meenakshi > > Aggarwal <meenakshi.aggar...@nxp.com>; Wasim Khan > > <wasim.k...@nxp.com>; Alison Wang <alison.w...@nxp.com>; Pramod > Kumar > > <pramod.kuma...@nxp.com>; Tang Yuantian <andy.t...@nxp.com>; Adrian > > Alonso <adrian.alo...@nxp.com>; Vladimir Oltean <olte...@gmail.com> > > Subject: [PATCH v8 10/15] crypto/fsl: Improve hwrng performance in > > kernel > > > > From: Ye Li <ye...@nxp.com> > > > > RNG parameters are reconfigured. > > - For TRNG to generate 256 bits of entropy, RNG TRNG Seed Control register > > is configured to have reduced SAMP_SIZE from default 2500 to 512. it is > > number of entropy samples that will be taken during Entropy generation. > > - self-test registers(Monobit Limit, Poker Range, Run Length Limit) > > are synchronized with new RTSDCTL[SAMP_SIZE] of 512. > > > > TRNG time is caluculated based on sample size. > > Typo: caluculated -> calculated > > > time required to generate entropy is reduced and hwrng performance > > improved from 0.3 kB/s to 1.3 kB/s. > > Is there any degradation in passed/failed FIPS 140-2 test count? Can you > provide > some results from at least rngtest run? > > > > > Signed-off-by: Ye Li <ye...@nxp.com> > > Acked-by: Gaurav Jain <gaurav.j...@nxp.com>> > > --- > > drivers/crypto/fsl/jr.c | 102 +++++++++++++++++++++++++++++++++------- > > include/fsl_sec.h | 1 + > > 2 files changed, 87 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c index > > a84440ab10..e5346a84a4 100644 > > --- a/drivers/crypto/fsl/jr.c > > +++ b/drivers/crypto/fsl/jr.c > > @@ -603,30 +603,100 @@ static u8 get_rng_vid(ccsr_sec_t *sec) > > */ > > static void kick_trng(int ent_delay, ccsr_sec_t *sec) { > > + u32 samples = 512; /* number of bits to generate and test */ > > + u32 mono_min = 195; > > + u32 mono_max = 317; > > + u32 mono_range = mono_max - mono_min; > > + u32 poker_min = 1031; > > + u32 poker_max = 1600; > > + u32 poker_range = poker_max - poker_min + 1; > > + u32 retries = 2; > > + u32 lrun_max = 32; > > + s32 run_1_min = 27; > > + s32 run_1_max = 107; > > + s32 run_1_range = run_1_max - run_1_min; > > + s32 run_2_min = 7; > > + s32 run_2_max = 62; > > + s32 run_2_range = run_2_max - run_2_min; > > + s32 run_3_min = 0; > > + s32 run_3_max = 39; > > + s32 run_3_range = run_3_max - run_3_min; > > + s32 run_4_min = -1; > > + s32 run_4_max = 26; > > + s32 run_4_range = run_4_max - run_4_min; > > + s32 run_5_min = -1; > > + s32 run_5_max = 18; > > + s32 run_5_range = run_5_max - run_5_min; > > + s32 run_6_min = -1; > > + s32 run_6_max = 17; > > + s32 run_6_range = run_6_max - run_6_min; > > I have a feeling that this whole block of local variables can be simplified. > I'm not > sure it is required to list this so detailed. > > You can attempt to define those values in header file and use macros to > compute bound conditions, rather than allocating this on the stack here. > > > + u32 val; > > + > > struct rng4tst __iomem *rng = > > (struct rng4tst __iomem *)&sec->rng; > > - u32 val; > > > > - /* put RNG4 into program mode */ > > - sec_setbits32(&rng->rtmctl, RTMCTL_PRGM); > > - /* rtsdctl bits 0-15 contain "Entropy Delay, which defines the > > - * length (in system clocks) of each Entropy sample taken > > - * */ > > + /* Put RNG in program mode */ > > + /* Setting both RTMCTL:PRGM and RTMCTL:TRNG_ACC causes TRNG to > > + * properly invalidate the entropy in the entropy register and > > + * force re-generation. > > + */ > > + sec_setbits32(&rng->rtmctl, RTMCTL_PRGM | RTMCTL_ACC); > > + > > + /* Configure the RNG Entropy Delay > > + * Performance-wise, it does not make sense to > > + * set the delay to a value that is lower > > + * than the last one that worked (i.e. the state handles > > + * were instantiated properly. Thus, instead of wasting > > + * time trying to set the values controlling the sample > > + * frequency, the function simply returns. > > + */ > > val = sec_in32(&rng->rtsdctl); > > - val = (val & ~RTSDCTL_ENT_DLY_MASK) | > > - (ent_delay << RTSDCTL_ENT_DLY_SHIFT); > > + val &= RTSDCTL_ENT_DLY_MASK; > > + val >>= RTSDCTL_ENT_DLY_SHIFT; > > + if (ent_delay < val) { > > + /* Put RNG4 into run mode */ > > + sec_clrbits32(&rng->rtmctl, RTMCTL_PRGM | RTMCTL_ACC); > > + return; > > + } > > + > > + val = (ent_delay << RTSDCTL_ENT_DLY_SHIFT) | samples; > > sec_out32(&rng->rtsdctl, val); > > - /* min. freq. count, equal to 1/4 of the entropy sample length */ > > - sec_out32(&rng->rtfreqmin, ent_delay >> 2); > > - /* disable maximum frequency count */ > > - sec_out32(&rng->rtfreqmax, RTFRQMAX_DISABLE); > > + > > + /* > > + * Recommended margins (min,max) for freq. count: > > + * freq_mul = RO_freq / TRNG_clk_freq > > + * rtfrqmin = (ent_delay x freq_mul) >> 1; > > + * rtfrqmax = (ent_delay x freq_mul) << 3; > > + * Given current deployments of CAAM in i.MX SoCs, and to simplify > > + * the configuration, we consider [1,16] to be a safe interval > > Statement "... we consider ..." should be perhaps extended with justification > on > how this conclusion was made. Some test results (either here in the comment or > in commit message) would be beneficial to see. > > > + * for the freq_mul and the limits of the interval are used to compute > > + * rtfrqmin, rtfrqmax > > + */ > > + sec_out32(&rng->rtfreqmin, ent_delay >> 1); > > + sec_out32(&rng->rtfreqmax, ent_delay << 7); > > + > > + sec_out32(&rng->rtscmisc, (retries << 16) | lrun_max); > > + sec_out32(&rng->rtpkrmax, poker_max); > > + sec_out32(&rng->rtpkrrng, poker_range); > > + sec_out32(&rng->rsvd1[0], (mono_range << 16) | mono_max); > > + sec_out32(&rng->rsvd1[1], (run_1_range << 16) | run_1_max); > > + sec_out32(&rng->rsvd1[2], (run_2_range << 16) | run_2_max); > > + sec_out32(&rng->rsvd1[3], (run_3_range << 16) | run_3_max); > > + sec_out32(&rng->rsvd1[4], (run_4_range << 16) | run_4_max); > > + sec_out32(&rng->rsvd1[5], (run_5_range << 16) | run_5_max); > > + sec_out32(&rng->rsvd1[6], (run_6_range << 16) | run_6_max); > > Writing in reserved area is not a good idea. Those registers are defined in > HW, > can you please define them also in the header file? > > > + > > + val = sec_in32(&rng->rtmctl); > > /* > > - * select raw sampling in both entropy shifter > > + * Select raw sampling in both entropy shifter > > This change is not needed, otherwise please adjust all comments in original > file, > as for example comment below also starts with small letter... > > > * and statistical checker > > */ > > - sec_setbits32(&rng->rtmctl, RTMCTL_SAMP_MODE_RAW_ES_SC); > > The only usage of RTMCTL_SAMP_MODE_RAW_ES_SC is dropped here, please > remove it from include/fsl_sec.h as well. > > > - /* put RNG4 into run mode */ > > - sec_clrbits32(&rng->rtmctl, RTMCTL_PRGM); > > + val &= ~RTMCTL_SAMP_MODE_INVALID; > > + val |= RTMCTL_SAMP_MODE_RAW_ES_SC; > > + /* Put RNG4 into run mode */ > > + val &= ~(RTMCTL_PRGM | RTMCTL_ACC); > > BIT() macro for both new and existing defines? > > > + /*test with sample mode only */ > > + sec_out32(&rng->rtmctl, val); > > } > > > > static int rng_init(uint8_t sec_idx, ccsr_sec_t *sec) diff --git > > a/include/fsl_sec.h b/include/fsl_sec.h index 7b6e3e2c20..2b3239414a > > 100644 > > --- a/include/fsl_sec.h > > +++ b/include/fsl_sec.h > > @@ -34,6 +34,7 @@ > > #if CONFIG_SYS_FSL_SEC_COMPAT >= 4 > > /* RNG4 TRNG test registers */ > > struct rng4tst { > > +#define RTMCTL_ACC 0x20 > > #define RTMCTL_PRGM 0x00010000 /* 1 -> program mode, 0 -> run mode > */ > > #define RTMCTL_SAMP_MODE_VON_NEUMANN_ES_SC 0 /* use von > Neumann data in > > both entropy shifter > > and > > -- > > 2.17.1 > > -- andrey