Re: [PATCH v4 08/12] hw/nvram: NPCM7xx OTP device model

[Philippe Mathieu-Daudé]

On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
> This supports reading and writing OTP fuses and keys. Only fuse reading
> has been tested. Protection is not implemented.
> 
> Reviewed-by: Avi Fishman <avi.fishman@nuvoton.com>
> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> ---
>  hw/arm/npcm7xx.c               |  32 +++
>  hw/nvram/Makefile.objs         |   1 +
>  hw/nvram/npcm7xx_otp.c         | 405 +++++++++++++++++++++++++++++++++
>  include/hw/arm/npcm7xx.h       |   3 +
>  include/hw/nvram/npcm7xx_otp.h |  94 ++++++++
>  5 files changed, 535 insertions(+)
>  create mode 100644 hw/nvram/npcm7xx_otp.c
>  create mode 100644 include/hw/nvram/npcm7xx_otp.h
> 
...
> +static void npcm7xx_init_fuses(NPCM7xxState *s)
> +{
> +    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
> +    uint32_t value;
> +
> +    value = tswap32(nc->disabled_modules);
> +    npcm7xx_otp_array_write(&s->fuse_array, &value, 64, sizeof(value));

What is magic offset 64 for?

...
> diff --git a/hw/nvram/npcm7xx_otp.c b/hw/nvram/npcm7xx_otp.c
> new file mode 100644
> index 0000000000..18908bc839
> --- /dev/null
> +++ b/hw/nvram/npcm7xx_otp.c
...
> +/* Common register write handler for both OTP classes. */
> +static void npcm7xx_otp_write(NPCM7xxOTPState *s, NPCM7xxOTPRegister reg,
> +                              uint32_t value)
> +{
> +    switch (reg) {
> +    case NPCM7XX_OTP_FST:
> +        /* RDST is cleared by writing 1 to it. */
> +        if (value & FST_RDST) {
> +            s->regs[NPCM7XX_OTP_FST] &= ~FST_RDST;
> +        }
> +        /* Preserve read-only and write-one-to-clear bits */
> +        value =
> +            (value & ~FST_RO_MASK) | (s->regs[NPCM7XX_OTP_FST] & 
> FST_RO_MASK);

Trivial to review as:

           value &= ~FST_RO_MASK;
           value |= s->regs[NPCM7XX_OTP_FST] & FST_RO_MASK;

> +        break;
> +
> +    case NPCM7XX_OTP_FADDR:
> +        break;
> +
> +    case NPCM7XX_OTP_FDATA:
> +        /*
> +         * This register is cleared by writing a magic value to it; no other
> +         * values can be written.
> +         */
> +        if (value == FDATA_CLEAR) {
> +            value = 0;
> +        } else {
> +            value = s->regs[NPCM7XX_OTP_FDATA];
> +        }
> +        break;
> +
> +    case NPCM7XX_OTP_FCFG:
> +        value = npcm7xx_otp_compute_fcfg(s->regs[NPCM7XX_OTP_FCFG], value);
> +        break;
> +
> +    case NPCM7XX_OTP_FCTL:
> +        switch (value) {
> +        case FCTL_READ_CMD:
> +            npcm7xx_otp_read_array(s);
> +            break;
> +
> +        case FCTL_PROG_CMD1:
> +            /*
> +             * Programming requires writing two separate magic values to this
> +             * register; this is the first one. Just store it so it can be
> +             * verified later when the second magic value is received.
> +             */
> +            break;
> +
> +        case FCTL_PROG_CMD2:
> +            /*
> +             * Only initiate programming if we received the first half of the
> +             * command immediately before this one.
> +             */
> +            if (s->regs[NPCM7XX_OTP_FCTL] == FCTL_PROG_CMD1) {
> +                npcm7xx_otp_program_array(s);
> +            }
> +            break;
> +
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: unrecognized FCNTL value 0x%" PRIx32 "\n",
> +                          DEVICE(s)->canonical_path, value);
> +            break;
> +        }
> +        if (value != FCTL_PROG_CMD1) {
> +            value = 0;
> +        }
> +        break;
> +
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to invalid offset 0x%zx\n",
> +                      DEVICE(s)->canonical_path, reg * sizeof(uint32_t));
> +        return;
> +    }
> +
> +    s->regs[reg] = value;
> +}
...
> diff --git a/include/hw/nvram/npcm7xx_otp.h b/include/hw/nvram/npcm7xx_otp.h
> new file mode 100644
> index 0000000000..b52330dadc
> --- /dev/null
> +++ b/include/hw/nvram/npcm7xx_otp.h
> @@ -0,0 +1,94 @@
> +/*
> + * Nuvoton NPCM7xx OTP (Fuse Array) Interface
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +#ifndef NPCM7XX_OTP_H
> +#define NPCM7XX_OTP_H
> +
> +#include "exec/memory.h"
> +#include "hw/sysbus.h"
> +
> +/* Each OTP module holds 8192 bits of one-time programmable storage */
> +#define NPCM7XX_OTP_ARRAY_BITS (8192)
> +#define NPCM7XX_OTP_ARRAY_BYTES (NPCM7XX_OTP_ARRAY_BITS / 8)

You could replace 8 by BITS_PER_BYTE.

> +
> +/**
> + * enum NPCM7xxOTPRegister - 32-bit register indices.
> + */
> +typedef enum NPCM7xxOTPRegister {
> +    NPCM7XX_OTP_FST,
> +    NPCM7XX_OTP_FADDR,
> +    NPCM7XX_OTP_FDATA,
> +    NPCM7XX_OTP_FCFG,
> +    /* Offset 0x10 is FKEYIND in OTP1, FUSTRAP in OTP2 */
> +    NPCM7XX_OTP_FKEYIND = 0x0010 / sizeof(uint32_t),
> +    NPCM7XX_OTP_FUSTRAP = 0x0010 / sizeof(uint32_t),
> +    NPCM7XX_OTP_FCTL,
> +    NPCM7XX_OTP_NR_REGS,
> +} NPCM7xxOTPRegister;
> +
> +/**
> + * struct NPCM7xxOTPState - Device state for one OTP module.
> + * @parent: System bus device.
> + * @mmio: Memory region through which registers are accessed.
> + * @regs: Register contents.
> + * @array: OTP storage array.
> + */
> +typedef struct NPCM7xxOTPState {
> +    SysBusDevice parent;
> +
> +    MemoryRegion mmio;
> +    uint32_t regs[NPCM7XX_OTP_NR_REGS];
> +    uint8_t array[NPCM7XX_OTP_ARRAY_BYTES];
> +} NPCM7xxOTPState;
> +
> +#define TYPE_NPCM7XX_OTP "npcm7xx-otp"
> +#define NPCM7XX_OTP(obj) OBJECT_CHECK(NPCM7xxOTPState, (obj), 
> TYPE_NPCM7XX_OTP)
> +
> +#define TYPE_NPCM7XX_KEY_STORAGE "npcm7xx-key-storage"
> +#define TYPE_NPCM7XX_FUSE_ARRAY "npcm7xx-fuse-array"
> +
> +/**
> + * struct NPCM7xxOTPClass - OTP module class.
> + * @parent: System bus device class.
> + * @mmio_ops: MMIO register operations for this type of module.
> + *
> + * The two OTP modules (key-storage and fuse-array) have slightly different
> + * behavior, so we give them different MMIO register operations.
> + */
> +typedef struct NPCM7xxOTPClass {
> +    SysBusDeviceClass parent;
> +
> +    const MemoryRegionOps *mmio_ops;
> +} NPCM7xxOTPClass;
> +
> +#define NPCM7XX_OTP_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(NPCM7xxOTPClass, (klass), TYPE_NPCM7XX_OTP)
> +#define NPCM7XX_OTP_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(NPCM7xxOTPClass, (obj), TYPE_NPCM7XX_OTP)

If nothing outside of the model implementation requires accessing
the class fields (which is certainly the case here, no code out of
npcm7xx_otp.c should access mmio_ops directly), then I recommend
to keep the class definitions local to the single source file where
it is used. This also makes this header simpler to look at.

Very high code quality, so I just made nitpicking comments.
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +
> +/**
> + * npcm7xx_otp_array_write - ECC encode and write data to OTP array.
> + * @s: OTP module.
> + * @data: Data to be encoded and written.
> + * @offset: Offset of first byte to be written in the OTP array.
> + * @len: Number of bytes before ECC encoding.
> + *
> + * Each nibble of data is encoded into a byte, so the number of bytes written
> + * to the array will be @len * 2.
> + */
> +extern void npcm7xx_otp_array_write(NPCM7xxOTPState *s, const void *data,
> +                                    unsigned int offset, unsigned int len);
> +
> +#endif /* NPCM7XX_OTP_H */
>