JZ4780 rng driver (draft).

[José Vázquez Fernández]

This is a draft of a JZ47xx rng driver. As base bcm2835-rng.c was used.
At least the Ingenic M150 seems that have the same registers in the same
addresses so it could be used for other than those mentioned SoCs.

Because I have a lot of doubts there are comments and commented code
from the base driver that maybe can be interesting. Also via-rng can
give some idea in case the randomness is not good enough.

Finally I have no idea how to write the dts node.

Pepe

[James Hogan]

Hi Jose,

Thanks for making the effort to get a driver for this.

The DT node would be extremely simple in this case, all you need is the
compatible string (matching the one in the driver), and reg to specify
the base address and size of the RNG register region.

Can I also suggest submitting this sort of thing as patches with
git format-patch / git send-email. Its good practice for submitting
upstream, can include the Kconfig and Makefile changes necessary to
actually use the driver, and is simpler for others to try out. Do ask if
you have Q's about that.

>
> Pepe

> /*
> * Random Number Generator driver for Ingenic Xburst

I suspect the RNG is actually part of the SoC rather than the core,
which would make it part of the JZ4780.

> *
> * Copyright 2015 (c) José Vázquez Fernández
> *
> * with the majority of the code coming from:
> *
> * Broadcom BCM2835 Random Number Generator support
> *
> * Copyright (c) 2010-2012 Broadcom. All rights reserved.
> * Copyright (c) 2013 Lubomir Rintel
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License ("GPL")
> * version 2, as published by the Free Software Foundation.
> */
>
> #include <linux/hw_random.h>
> #include <linux/init.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of_address.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/printk.h>
>
> // not sure about the registers addresses
> #define CGU_REG_RNG 0x100000dc
> //#define CGU_REG_RNG 0xdc

Register offsets should be relative to the block of registers it is
contained in. That way the driver can be used by future/past SoCs which
have the same RNG block but potentially at a different physical
address, by just specifying a different base address in the DT.

Sometimes that isn't straightforward when the SoC mixes lots of
different misc SoC registers in a single block, so it might make sense
to use mfd/syscon to access the registers via a single syscon device
representing the block of misc registers.

> #define CGU_REG_ERNG 0x100000d8
> //#define CGU_REG_ERNG 0xd8
>
> /* the initial numbers generated are "less random" so will be discarded */
> //#define RNG_WARMUP_COUNT 0x40000
>
> static int jz47xx_rng_read(struct hwrng *rng, u32 *data)
> {
> void __iomem *rng_base = (void __iomem *)rng->priv;
>
> #if 0
> while ((__raw_readl(rng_base + RNG_STATUS) >> 24) == 0) {
> if (!wait)
> return 0;
> cpu_relax();
> }
> #endif
> *data = __raw_readl(rng_base + CGU_REG_RNG);
> /* __raw_readl or readl? */

readl unless you have good reason to use __raw_ (like have a bi-endian
CPU and the registers always being native endian, which doesn't apply
here).

> }
>
> static struct hwrng jz47xx_rng_ops = {
> .name = "jz47xx",
> .read = jz47xx_rng_read,
> };
>
> static int jz47xx_rng_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct device_node *np = dev->of_node;
> void __iomem *rng_base;
> int err;
>
> /* map peripheral */
> rng_base = of_iomap(np, 0);

Can you use something more like this to make the driver more generic and
not depend directly on DT, as well as meaning you don't need to iounmap
it on failure or on removal?

struct resource *res;

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
rng_base = devm_ioremap_resource(dev, res);
if (IS_ERR(rng_base))
return PTR_ERR(rng_base);

> if (!rng_base) {
> dev_err(dev, "failed to remap rng regs");

You should have a newline at the end of the string.

> return -ENODEV;

-ENOMEM would be more appropriate.

> }
> jz47xx_rng_ops.priv = (unsigned long)rng_base;
>
> /* register driver */
> err = hwrng_register(&jz47xx_rng_ops);
> if (err) {
> dev_err(dev, "Xburst hwrng registration failed\n");
> iounmap(rng_base);

(i.e. this iounmap could be dropped if using the devm_ functions above)

> } else {

General Linux style is to handle the error cases sequentially, keeping
the successful / common case as unnested as possible, so probably best
to return err; in the error case, scrap the else, and return 0; at the
end on success.

> dev_info(dev, "Xburst hwrng registered\n");
>
> /* set warm-up count & enable */
> //__raw_writel(RNG_WARMUP_COUNT, rng_base + RNG_STATUS);

If the JZ hardware doesn't have this sort of register then fair enough,
but it'd be worth checking whether anything else should be done to
ensure the best randomness. E.g. should you avoid reading it too often
(like timeriomem-rng driver does)?

>
> /* set CGU_REG_ERNG register */
> __raw_writel(1, rng_base + CGU_REG_ERNG);
> }
> return err;
> }
>
> static int jz47xx_rng_remove(struct platform_device *pdev)
> {
> void __iomem *rng_base = (void __iomem *)jz47xx_rng_ops.priv;
>
> /* disable rng hardware */
> __raw_writel(0, rng_base + CGU_REG_ERNG);
>
> /* unregister driver */
> hwrng_unregister(&jz47xx_rng_ops);
> iounmap(rng_base);

(here too, scrap the iounmap if you use the devm_ function to map it in
the first place).

>
> return 0;
> }
>
> static const struct of_device_id jz47xx_rng_of_match[] = {
> { .compatible = "ingenic,jz47xx-rng", },

You also should add a binding document with an example DT node to
Documentation/devicetree/bindings/hwrng/, which tbh can be extremely
simple.

Cheers
James

> {},
> };
> MODULE_DEVICE_TABLE(of, jz47xx_rng_of_match);
>
> static struct platform_driver jz47xx_rng_driver = {
> .driver = {
> .name = "jz47xx-rng",
> .of_match_table = jz47xx_rng_of_match,
> },
> .probe = jz47xx_rng_probe,
> .remove = jz47xx_rng_remove,
> };
> module_platform_driver(jz47xx_rng_driver);
>
> MODULE_AUTHOR("José Vázquez Fernández <corol...@gmail.com>");
> MODULE_DESCRIPTION("jz47xx Random Number Generator (RNG) driver");
> MODULE_LICENSE("GPL v2");

[Pepe Vázquez Fernández]

Attached there is a diff with the changes advised by James Hogan.

It depends on MIPS instead JZ4780 because the M150 have the same
registers at the same addresses. I have no idea if other Ingenic SoCs
have those registers, but afaik the JZ4770 hasn't them.

I think that the driver, with minor changes and/or fixes, will work as
expected, but I can't do more because it exceeds my skills (the only
programming language I know well is PIC16/18 assembler).
Many thanks to James for his extremely useful hints.

This is the best I can do, so somebody will have to finish the driver.

Off topic: I guess that the JZ4780 has a crypto engine, but couldn't
find nothing about it.

Regards:

Pepe

[Michael McConville]

Pepe Vázquez Fernández wrote:
> Off topic: I guess that the JZ4780 has a crypto engine, but couldn't
> find nothing about it.

I think I remember seeing a vague reference to this in the docs. I'm
very interested. If anyone knows anything about it, please share.

[Pepe Vázquez Fernández]

I think that the crypto acceleration driver/s are provided only to the
customers, some developers or are mixed in the android sources. I
couldn't be able to find something about it/them in the M200 and X1000
kernel sources.

Pepe