| Message ID | 20240620231339.1574-6-semen.protsenko@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 10bb6ac8f86f4b65ef8d227504868a61e0bcb148 |
| Headers | show |
| Series | hwrng: exynos: Add support for Exynos850 | expand |
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@linaro.org> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > 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 (-; Don't change it if v3 is the last. If not, please, make it more verbose. > + > + return ret; > +} > + [...] Kind regards,
On Fri, Jun 21, 2024 at 2:00 PM Lukasz Stelmach <l.stelmach@samsung.com> 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@linaro.org> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > 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
It was <2024-06-21 pią 14:40>, when Sam Protsenko wrote: > On Fri, Jun 21, 2024 at 2:00 PM Lukasz Stelmach <l.stelmach@samsung.com> 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@linaro.org> >> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> > --- >> > 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? I guess Olivia or Herbert will be applying it. Let me try… How about: "Check if your bootloader loads the firmware (SMC) part of the driver." >> 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 >
On Fri, Jun 21, 2024 at 5:11 PM Lukasz Stelmach <l.stelmach@samsung.com> wrote: [snip] > >> 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? > > I guess Olivia or Herbert will be applying it. Let me try… How about: > > "Check if your bootloader loads the firmware (SMC) part of the driver." > Much better. Thanks, Lukasz! > >> Don't change it if v3 is the last. If not, please, make it more verbose. > >>
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 @@ -10,6 +10,7 @@ * Krzysztof Kozłowski <krzk@kernel.org> */ +#include <linux/arm-smccc.h> #include <linux/clk.h> #include <linux/crypto.h> #include <linux/delay.h> @@ -22,6 +23,7 @@ #include <linux/mod_devicetable.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/property.h> #define EXYNOS_TRNG_CLKDIV 0x0 @@ -44,16 +46,41 @@ #define EXYNOS_TRNG_FIFO_LEN 8 #define EXYNOS_TRNG_CLOCK_RATE 500000 +/* Driver feature flags */ +#define EXYNOS_SMC BIT(0) + +#define EXYNOS_SMC_CALL_VAL(func_num) \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_SMC_32, \ + ARM_SMCCC_OWNER_SIP, \ + func_num) + +/* SMC command for DTRNG access */ +#define SMC_CMD_RANDOM EXYNOS_SMC_CALL_VAL(0x1012) + +/* SMC_CMD_RANDOM: arguments */ +#define HWRNG_INIT 0x0 +#define HWRNG_EXIT 0x1 +#define HWRNG_GET_DATA 0x2 +#define HWRNG_RESUME 0x3 + +/* SMC_CMD_RANDOM: return values */ +#define HWRNG_RET_OK 0x0 +#define HWRNG_RET_RETRY_ERROR 0x2 + +#define HWRNG_MAX_TRIES 100 + struct exynos_trng_dev { struct device *dev; void __iomem *mem; struct clk *clk; /* operating clock */ struct clk *pclk; /* bus clock */ struct hwrng rng; + unsigned long flags; }; -static int exynos_trng_do_read(struct hwrng *rng, void *data, size_t max, - bool wait) +static int exynos_trng_do_read_reg(struct hwrng *rng, void *data, size_t max, + bool wait) { struct exynos_trng_dev *trng = (struct exynos_trng_dev *)rng->priv; int val; @@ -70,7 +97,40 @@ static int exynos_trng_do_read(struct hwrng *rng, void *data, size_t max, return max; } -static int exynos_trng_init(struct hwrng *rng) +static int exynos_trng_do_read_smc(struct hwrng *rng, void *data, size_t max, + bool wait) +{ + struct arm_smccc_res res; + unsigned int copied = 0; + u32 *buf = data; + int tries = 0; + + while (copied < max) { + arm_smccc_smc(SMC_CMD_RANDOM, HWRNG_GET_DATA, 0, 0, 0, 0, 0, 0, + &res); + switch (res.a0) { + case HWRNG_RET_OK: + *buf++ = res.a2; + *buf++ = res.a3; + copied += 8; + tries = 0; + break; + case HWRNG_RET_RETRY_ERROR: + if (!wait) + return copied; + if (++tries >= HWRNG_MAX_TRIES) + return copied; + cond_resched(); + break; + default: + return -EIO; + } + } + + return copied; +} + +static int exynos_trng_init_reg(struct hwrng *rng) { struct exynos_trng_dev *trng = (struct exynos_trng_dev *)rng->priv; unsigned long sss_rate; @@ -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"); + + return ret; +} + static int exynos_trng_probe(struct platform_device *pdev) { struct exynos_trng_dev *trng; @@ -112,21 +190,29 @@ static int exynos_trng_probe(struct platform_device *pdev) if (!trng) return ret; + platform_set_drvdata(pdev, trng); + trng->dev = &pdev->dev; + + trng->flags = (unsigned long)device_get_match_data(&pdev->dev); + trng->rng.name = devm_kstrdup(&pdev->dev, dev_name(&pdev->dev), GFP_KERNEL); if (!trng->rng.name) return ret; - trng->rng.init = exynos_trng_init; - trng->rng.read = exynos_trng_do_read; trng->rng.priv = (unsigned long)trng; - platform_set_drvdata(pdev, trng); - trng->dev = &pdev->dev; + if (trng->flags & EXYNOS_SMC) { + trng->rng.init = exynos_trng_init_smc; + trng->rng.read = exynos_trng_do_read_smc; + } else { + trng->rng.init = exynos_trng_init_reg; + trng->rng.read = exynos_trng_do_read_reg; - trng->mem = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(trng->mem)) - return PTR_ERR(trng->mem); + trng->mem = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(trng->mem)) + return PTR_ERR(trng->mem); + } pm_runtime_enable(&pdev->dev); ret = pm_runtime_resume_and_get(&pdev->dev); @@ -170,12 +256,31 @@ static int exynos_trng_probe(struct platform_device *pdev) static void exynos_trng_remove(struct platform_device *pdev) { + struct exynos_trng_dev *trng = platform_get_drvdata(pdev); + + if (trng->flags & EXYNOS_SMC) { + struct arm_smccc_res res; + + arm_smccc_smc(SMC_CMD_RANDOM, HWRNG_EXIT, 0, 0, 0, 0, 0, 0, + &res); + } + pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); } static int exynos_trng_suspend(struct device *dev) { + struct exynos_trng_dev *trng = dev_get_drvdata(dev); + struct arm_smccc_res res; + + if (trng->flags & EXYNOS_SMC) { + arm_smccc_smc(SMC_CMD_RANDOM, HWRNG_EXIT, 0, 0, 0, 0, 0, 0, + &res); + if (res.a0 != HWRNG_RET_OK) + return -EIO; + } + pm_runtime_put_sync(dev); return 0; @@ -183,6 +288,7 @@ static int exynos_trng_suspend(struct device *dev) static int exynos_trng_resume(struct device *dev) { + struct exynos_trng_dev *trng = dev_get_drvdata(dev); int ret; ret = pm_runtime_resume_and_get(dev); @@ -191,6 +297,20 @@ static int exynos_trng_resume(struct device *dev) return ret; } + if (trng->flags & EXYNOS_SMC) { + struct arm_smccc_res res; + + arm_smccc_smc(SMC_CMD_RANDOM, HWRNG_RESUME, 0, 0, 0, 0, 0, 0, + &res); + if (res.a0 != HWRNG_RET_OK) + return -EIO; + + arm_smccc_smc(SMC_CMD_RANDOM, HWRNG_INIT, 0, 0, 0, 0, 0, 0, + &res); + if (res.a0 != HWRNG_RET_OK) + return -EIO; + } + return 0; }