It was <2017-11-23 czw 17:31>, when Krzysztof Kozlowski wrote: > On Thu, Nov 23, 2017 at 4:09 PM, Łukasz Stelmach <l.stelmach@xxxxxxxxxxx> wrote: >> Add support for True Random Number Generator found in Samsung Exynos >> 5250+ SoCs. >> >> Signed-off-by: Łukasz Stelmach <l.stelmach@xxxxxxxxxxx> >> --- >> MAINTAINERS | 7 + >> drivers/char/hw_random/Kconfig | 12 ++ >> drivers/char/hw_random/Makefile | 1 + >> drivers/char/hw_random/exynos-trng.c | 256 +++++++++++++++++++++++++++++++++++ >> 4 files changed, 276 insertions(+) >> create mode 100644 drivers/char/hw_random/exynos-trng.c >> [...] >> endif # HW_RANDOM > > Thanks for working on TRNG. > > 1. I was rather thinking about extending existing exynos-rng.c [1] so > it would be using TRNG as seed for PRNG as this gives you much more > random data. Instead you developed totally separate driver which has > its own benefits - one can choose which interface he wants. Although > it is a little bit duplication. As far as I can tell, these are two different devices. However, PRNG shares hardware with the hash engine. Indeed there is a hardware to connect TRNG and PRNG, but, IMHO, it might be hard to model that dependency in kernel. To me it seems easier to read TRNG (or /dev/random) and and write the result to PRNG manually (in software). > 2. I understand that in your current version of TRNG driver there is > no conflict with PRNG and they can work both at the same time? I guess so. I expect no problems as long as TRNG_SEED_START is not set in the HASH_SEED_CONF register. > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/exynos-rng.c > >> + >> + 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; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + trng->mem = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(trng->mem)) { >> + dev_err(&pdev->dev, "Couldn't map IO resources.\n"); >> + ret = PTR_ERR(trng->mem); >> + goto err_ioremap; >> + } >> + >> + pm_runtime_enable(&pdev->dev); >> + ret = pm_runtime_get_sync(&pdev->dev); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d.\n", ret); >> + goto err_ioremap; > > Missing pm_runtime_disable? Added a separate label down below. >> + dev_err(&pdev->dev, "Couldn't get clock.\n"); >> + ret = PTR_ERR(trng->clk); >> + goto err_clock; >> + } >> + >> + ret = clk_prepare_enable(trng->clk); >> + if (ret) { >> + dev_err(&pdev->dev, "Unable to enable the clk: %d.\n", ret); >> + goto err_clock; >> + } >> + >> + ret = hwrng_register(&trng->rng); >> + if (ret) { >> + dev_err(&pdev->dev, "Couldn't register hwrng device.\n"); >> + goto err_register; >> + } >> + >> + dev_info(&pdev->dev, "Exynos True Random Number Generator.\n"); >> + >> + return 0; >> + >> +err_register: >> + clk_disable_unprepare(trng->clk); >> + >> +err_clock: >> + trng->mem = NULL; > > Why this has to be null-ified? I found this pattern in omap-rng.c. Removed. I'll wait a little bit more before posting v2. -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics
Attachment:
signature.asc
Description: PGP signature
