Re: [PATCH] i.MX: add an emulation for RNGC

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 26 Dec 2019 at 17:52, Martin Kaiser <martin@kaiser.cx> wrote:
>
> Add an emulation for the RNGC random number generator and the compatible
> RNGB variant. These peripherals are included (at least) in imx25 and
> imx35 chipsets.
>
> The emulation supports the initial self test, reseeding the prng and
> reading random numbers.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>

Thanks for this patch; it looks good to me -- the only
major missing item is a VMStateDescription, which should
be easy to add. (I haven't checked the actual device
functionality against a datasheet, but it all looks plausible.)
There were also a couple of minor nits which I've listed inline below.

> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index ba898a5781..2b28f8c096 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -42,6 +42,7 @@ common-obj-$(CONFIG_IMX) += imx7_ccm.o
>  common-obj-$(CONFIG_IMX) += imx2_wdt.o
>  common-obj-$(CONFIG_IMX) += imx7_snvs.o
>  common-obj-$(CONFIG_IMX) += imx7_gpr.o
> +common-obj-$(CONFIG_IMX) += imx_rngc.o
>  common-obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
>  common-obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
>  common-obj-$(CONFIG_MAINSTONE) += mst_fpga.o
> diff --git a/hw/misc/imx_rngc.c b/hw/misc/imx_rngc.c
> new file mode 100644
> index 0000000000..f935ec46a6
> --- /dev/null
> +++ b/hw/misc/imx_rngc.c
> @@ -0,0 +1,262 @@
> +/*
> + * Freescale i.MX RNGC emulation
> + *
> + * Copyright (C) 2019 Martin Kaiser <martin@kaiser.cx>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + * This driver provides the minimum functionality to initialize and seed
> + * an rngc and to read random numbers. The rngb that is found in imx25
> + * chipsets is also supported.
> + */
> +
> +
> +#include "qemu/osdep.h"
> +#include "qemu/main-loop.h"

Do you really need main-loop.h ?

> +#include "qemu/module.h"
> +#include "qemu/log.h"
> +#include "qemu/guest-random.h"
> +#include "hw/irq.h"
> +#include "hw/misc/imx_rngc.h"

> +static void __imx_rngc_reset(IMXRNGCState *s)

Don't use __ prefixes for function names, please.

> +{
> +    s->op_self_test = OP_IDLE;
> +    s->op_seed = OP_IDLE;
> +    s->mask = 0;
> +    s->auto_seed = false;
> +}
> +
> +static void imx_rngc_write(void *opaque, hwaddr offset, uint64_t value,
> +                           unsigned size)
> +{
> +    IMXRNGCState *s = IMX_RNGC(opaque);
> +
> +    switch (offset) {
> +    case RNGC_COMMAND:
> +        if (value & RNGC_CMD_BIT_SW_RST) {
> +            __imx_rngc_reset(s);
> +        }
> +
> +        /*
> +         * For now, both CLR_ERR and CLR_INT clear the interrupt. We
> +         * don't report any erors yet.

"errors"

> +         */
> +        if (value & (RNGC_CMD_BIT_CLR_ERR | RNGC_CMD_BIT_CLR_INT)) {
> +            qemu_irq_lower(s->irq);
> +        }

> +static void imx_rngc_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = imx_rngc_realize;
> +    dc->reset = imx_rngc_reset;
> +    dc->desc = "i.MX RNGC";

You need also to set dc->vmsd to a VMStateDescription
which describes the variable state of the device that
needs to be migrated.

> +}
> +
> +static const TypeInfo imx_rngc_info = {
> +    .name          = TYPE_IMX_RNGC,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(IMXRNGCState),
> +    .class_init    = imx_rngc_class_init,
> +};
> +
> +static void imx_rngc_register_types(void)
> +{
> +    type_register_static(&imx_rngc_info);
> +}

thanks
-- PMM