On Fri, Jun 21, 2024 at 2:00 PM Lukasz Stelmach <l.stelmach@xxxxxxxxxxx> wrote: > > It was <2024-06-20 czw 18:13>, when Sam Protsenko wrote: > > On some Exynos chips like Exynos850 the access to Security Sub System > > (SSS) registers is protected with TrustZone, and therefore only possible > > from EL3 monitor software. The Linux kernel is running in EL1, so the > > only way for the driver to obtain TRNG data is via SMC calls to EL3 > > monitor. Implement such SMC operation and use it when EXYNOS_SMC flag is > > set in the corresponding chip driver data. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@xxxxxxxxxx> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> > > --- > > Changes in v3: > > - Added appropriate error messages for the case when init SMC command fails > > > > Changes in v2: > > - Used the "reversed Christmas tree" style in the variable declaration > > block in exynos_trng_do_read_smc() > > - Renamed .quirks to .flags in the driver structure > > - Added Krzysztof's R-b tag > > > > drivers/char/hw_random/exynos-trng.c | 140 +++++++++++++++++++++++++-- > > 1 file changed, 130 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/char/hw_random/exynos-trng.c b/drivers/char/hw_random/exynos-trng.c > > index 6ef2ee6c9804..9fa30583cc86 100644 > > --- a/drivers/char/hw_random/exynos-trng.c > > +++ b/drivers/char/hw_random/exynos-trng.c > > [...] > > > > @@ -103,6 +163,24 @@ static int exynos_trng_init(struct hwrng *rng) > > return 0; > > } > > > > +static int exynos_trng_init_smc(struct hwrng *rng) > > +{ > > + struct exynos_trng_dev *trng = (struct exynos_trng_dev *)rng->priv; > > + struct arm_smccc_res res; > > + int ret = 0; > > + > > + arm_smccc_smc(SMC_CMD_RANDOM, HWRNG_INIT, 0, 0, 0, 0, 0, 0, &res); > > + if (res.a0 != HWRNG_RET_OK) { > > + dev_err(trng->dev, "SMC command for TRNG init failed (%d)\n", > > + (int)res.a0); > > + ret = -EIO; > > + } > > + if ((int)res.a0 == -1) > > + dev_info(trng->dev, "Make sure LDFW is loaded by your BL\n"); > > This is good, thank you for adding it. It can be even better though, if > you don't skimp on message length (-; I mean, I know what BL is, I can > fingure what LDFW is because you have explained to me and I can see the > source code, but somewone who sees it for the first time will be only > slightly less surprised than with v2 error message only. Come on, you > can make this message twice as long and it will still fit in 80 characters (-; > Guess my OCD got in the way and I just didn't want to break the line :) But yeah, LDFW = Loadable Firmware, and BL = bootloader. There is an "ldfw" partition on eMMC, and I noticed Samsung usually uses LDFW term, so I figured it was not a big deal to throw that abbreviation at the user. But I totally agree on BL part, it might be confusing. I don't have any strong opinion on this one. If you are going to apply v3, can I kindly ask you to change that message the way you want it to be? > Don't change it if v3 is the last. If not, please, make it more verbose. > > > + > > + return ret; > > +} > > + > > > [...] > > > Kind regards, > -- > Łukasz Stelmach > Samsung R&D Institute Poland > Samsung Electronics