[v3,08/12] hw/nvram: NPCM7xx OTP device model

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         | 391 +++++++++++++++++++++++++++++++++
 include/hw/arm/npcm7xx.h       |   3 +
 include/hw/nvram/npcm7xx_otp.h |  93 ++++++++
 5 files changed, 520 insertions(+)
 create mode 100644 hw/nvram/npcm7xx_otp.c
 create mode 100644 include/hw/nvram/npcm7xx_otp.h

On 6/27/20 1:55 AM, 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         | 391 +++++++++++++++++++++++++++++++++
>  include/hw/arm/npcm7xx.h       |   3 +
>  include/hw/nvram/npcm7xx_otp.h |  93 ++++++++
>  5 files changed, 520 insertions(+)
>  create mode 100644 hw/nvram/npcm7xx_otp.c
>  create mode 100644 include/hw/nvram/npcm7xx_otp.h
> 
> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
> index f395fe84b6..825910a4bf 100644
> --- a/hw/arm/npcm7xx.c
> +++ b/hw/arm/npcm7xx.c
> @@ -36,6 +36,10 @@
>  #define NPCM7XX_MMIO_BA         (0x80000000)
>  #define NPCM7XX_MMIO_SZ         (0x7FFD0000)
>  
> +/* OTP key storage and fuse strap array */
> +#define NPCM7XX_OTP1_BA         (0xF0189000)
> +#define NPCM7XX_OTP2_BA         (0xF018A000)
> +
>  /* Core system modules. */
>  #define NPCM7XX_L2C_BA          (0xF03FC000)
>  #define NPCM7XX_CPUP_BA         (0xF03FE000)
> @@ -122,6 +126,15 @@ void npcm7xx_write_secondary_boot(ARMCPU *cpu, const struct arm_boot_info *info)
>                         NPCM7XX_SMP_LOADER_START);
>  }
>  
> +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));
> +}
> +
>  static qemu_irq npcm7xx_irq(NPCM7xxState *s, int n)
>  {
>      return qdev_get_gpio_in(DEVICE(&s->a9mpcore), n);
> @@ -142,6 +155,10 @@ static void npcm7xx_init(Object *obj)
>      object_property_add_alias(obj, "power-on-straps", OBJECT(&s->gcr),
>                                "power-on-straps");
>      object_initialize_child(obj, "clk", &s->clk, TYPE_NPCM7XX_CLK);
> +    object_initialize_child(obj, "otp1", &s->key_storage,
> +                            TYPE_NPCM7XX_KEY_STORAGE);
> +    object_initialize_child(obj, "otp2", &s->fuse_array,
> +                            TYPE_NPCM7XX_FUSE_ARRAY);
>  
>      for (i = 0; i < ARRAY_SIZE(s->tim); i++) {
>          object_initialize_child(obj, "tim[*]", &s->tim[i], TYPE_NPCM7XX_TIMER);
> @@ -225,6 +242,21 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
>      }
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->clk), 0, NPCM7XX_CLK_BA);
>  
> +    /* OTP key storage and fuse strap array */
> +    sysbus_realize(SYS_BUS_DEVICE(&s->key_storage), &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->key_storage), 0, NPCM7XX_OTP1_BA);
> +    sysbus_realize(SYS_BUS_DEVICE(&s->fuse_array), &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->fuse_array), 0, NPCM7XX_OTP2_BA);
> +    npcm7xx_init_fuses(s);
> +
>      /* Timer Modules (TIM) */
>      QEMU_BUILD_BUG_ON(ARRAY_SIZE(npcm7xx_tim_addr) != ARRAY_SIZE(s->tim));
>      for (i = 0; i < ARRAY_SIZE(s->tim); i++) {
> diff --git a/hw/nvram/Makefile.objs b/hw/nvram/Makefile.objs
> index f3ad921382..0270f0bbf7 100644
> --- a/hw/nvram/Makefile.objs
> +++ b/hw/nvram/Makefile.objs
> @@ -4,5 +4,6 @@ common-obj-$(CONFIG_AT24C) += eeprom_at24c.o
>  common-obj-y += fw_cfg.o
>  common-obj-$(CONFIG_CHRP_NVRAM) += chrp_nvram.o
>  common-obj-$(CONFIG_MAC_NVRAM) += mac_nvram.o
> +common-obj-$(CONFIG_NPCM7XX) += npcm7xx_otp.o
>  common-obj-$(CONFIG_NRF51_SOC) += nrf51_nvm.o
>  obj-$(CONFIG_PSERIES) += spapr_nvram.o
> diff --git a/hw/nvram/npcm7xx_otp.c b/hw/nvram/npcm7xx_otp.c
> new file mode 100644
> index 0000000000..71fc728a2d
> --- /dev/null
> +++ b/hw/nvram/npcm7xx_otp.c
> @@ -0,0 +1,391 @@
> +/*
> + * 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
> + * version 2 as published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "hw/nvram/npcm7xx_otp.h"
> +#include "qapi/error.h"
> +#include "qemu/bitops.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/units.h"
> +
> +/* Each module has 4 KiB of register space. Only a fraction of it is used. */
> +#define NPCM7XX_OTP_REGS_SIZE (4 * KiB)
> +
> +/* Register field definitions. */
> +#define FST_RIEN BIT(2)
> +#define FST_RDST BIT(1)
> +#define FST_RDY BIT(0)
> +#define FST_RO_MASK (FST_RDST | FST_RDY)
> +
> +#define FADDR_BYTEADDR(rv) extract32((rv), 0, 10)
> +#define FADDR_BITPOS(rv) extract32((rv), 10, 3)
> +
> +#define FDATA_CLEAR 0x00000001
> +
> +#define FCFG_FDIS BIT(31)
> +#define FCFG_FCFGLK_MASK 0x00ff0000
> +
> +#define FCTL_PROG_CMD1 0x00000001
> +#define FCTL_PROG_CMD2 0xbf79e5d0
> +#define FCTL_READ_CMD 0x00000002
> +
> +static uint8_t ecc_encode_nibble(uint8_t n)
> +{
> +    uint8_t result = n;
> +
> +    result |= (((n >> 0) & 1) ^ ((n >> 1) & 1)) << 4;
> +    result |= (((n >> 2) & 1) ^ ((n >> 3) & 1)) << 5;
> +    result |= (((n >> 0) & 1) ^ ((n >> 2) & 1)) << 6;
> +    result |= (((n >> 1) & 1) ^ ((n >> 3) & 1)) << 7;
> +
> +    return result;
> +}
> +
> +void npcm7xx_otp_array_write(NPCM7xxOTPState *s, const void *data,
> +                             unsigned int offset, unsigned int len)
> +{
> +    const uint8_t *src = data;
> +    uint8_t *dst = s->array + offset;
> +
> +    while (len-- > 0) {
> +        uint8_t c = *src++;
> +
> +        *dst++ = ecc_encode_nibble(extract8(c, 0, 4));
> +        *dst++ = ecc_encode_nibble(extract8(c, 4, 4));
> +    }
> +}
> +
> +/* Common register read handler for both OTP classes. */
> +static uint64_t npcm7xx_otp_read(NPCM7xxOTPState *s, NPCM7xxOTPRegister reg)
> +{
> +    uint32_t value = 0;
> +
> +    switch (reg) {
> +    case NPCM7XX_OTP_FST:
> +    case NPCM7XX_OTP_FADDR:
> +    case NPCM7XX_OTP_FDATA:
> +    case NPCM7XX_OTP_FCFG:
> +        value = s->regs[reg];
> +        break;
> +
> +    case NPCM7XX_OTP_FCTL:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: read from write-only FCTL register\n",
> +                      DEVICE(s)->canonical_path);
> +        break;
> +
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from invalid offset 0x%zx\n",
> +                      DEVICE(s)->canonical_path, reg * sizeof(uint32_t));
> +        break;
> +    }
> +
> +    return value;
> +}
> +
> +/* Read a byte from the OTP array into the data register. */
> +static void npcm7xx_otp_read_array(NPCM7xxOTPState *s)
> +{
> +    uint32_t faddr = s->regs[NPCM7XX_OTP_FADDR];
> +
> +    s->regs[NPCM7XX_OTP_FDATA] = s->array[FADDR_BYTEADDR(faddr)];
> +    s->regs[NPCM7XX_OTP_FST] |= FST_RDST | FST_RDY;
> +}
> +
> +/* Program a byte from the data register into the OTP array. */
> +static void npcm7xx_otp_program_array(NPCM7xxOTPState *s)
> +{
> +    uint32_t faddr = s->regs[NPCM7XX_OTP_FADDR];
> +
> +    /* Bits can only go 0->1, never 1->0. */
> +    s->array[FADDR_BYTEADDR(faddr)] |= (1U << FADDR_BITPOS(faddr));
> +    s->regs[NPCM7XX_OTP_FST] |= FST_RDST | FST_RDY;
> +}
> +
> +/* Compute the next value of the FCFG register. */
> +static uint32_t npcm7xx_otp_compute_fcfg(uint32_t cur_value, uint32_t new_value)
> +{
> +    uint32_t lock_mask;
> +    uint32_t value;
> +
> +    /*
> +     * FCFGLK holds sticky bits 16..23, indicating which bits in FPRGLK (8..15)
> +     * and FRDLK (0..7) that are read-only.
> +     */
> +    lock_mask = (cur_value & FCFG_FCFGLK_MASK) >> 8;
> +    lock_mask |= lock_mask >> 8;
> +    /* FDIS and FCFGLK bits are sticky (write 1 to set; can't clear). */
> +    value = cur_value & (FCFG_FDIS | FCFG_FCFGLK_MASK);
> +    /* Preserve read-only bits in FPRGLK and FRDLK */
> +    value |= cur_value & lock_mask;
> +    /* Set all bits that aren't read-only. */
> +    value |= new_value & ~lock_mask;
> +
> +    return value;
> +}
> +
> +/* 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);
> +        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;
> +}
> +
> +/* Register read handler specific to the fuse array OTP module. */
> +static uint64_t npcm7xx_fuse_array_read(void *opaque, hwaddr addr,
> +                                        unsigned int size)
> +{
> +    NPCM7xxOTPRegister reg = addr / sizeof(uint32_t);
> +    NPCM7xxOTPState *s = opaque;
> +    uint32_t value;
> +
> +    /*
> +     * Only the Fuse Strap register needs special handling; all other registers
> +     * work the same way for both kinds of OTP modules.
> +     */
> +    if (reg != NPCM7XX_OTP_FUSTRAP) {
> +        value = npcm7xx_otp_read(s, reg);
> +    } else {
> +        /* FUSTRAP is stored as three copies in the OTP array. */
> +        uint32_t fustrap[3];
> +
> +        memcpy(fustrap, &s->array[0], sizeof(fustrap));
> +
> +        /* Determine value by a majority vote on each bit. */
> +        value = (fustrap[0] & fustrap[1]) | (fustrap[0] & fustrap[2]) |
> +                (fustrap[1] & fustrap[2]);
> +    }
> +
> +    return value;
> +}
> +
> +/* Register write handler specific to the fuse array OTP module. */
> +static void npcm7xx_fuse_array_write(void *opaque, hwaddr addr, uint64_t v,
> +                                     unsigned int size)
> +{
> +    NPCM7xxOTPRegister reg = addr / sizeof(uint32_t);
> +    NPCM7xxOTPState *s = opaque;
> +
> +    /*
> +     * The Fuse Strap register is read-only. Other registers are handled by
> +     * common code.
> +     */
> +    if (reg != NPCM7XX_OTP_FUSTRAP) {
> +        npcm7xx_otp_write(s, reg, v);
> +    }
> +}
> +
> +static const MemoryRegionOps npcm7xx_fuse_array_ops = {
> +    .read       = npcm7xx_fuse_array_read,
> +    .write      = npcm7xx_fuse_array_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid      = {
> +        .min_access_size        = 4,
> +        .max_access_size        = 4,
> +        .unaligned              = false,
> +    },
> +};
> +
> +/* Register read handler specific to the key storage OTP module. */
> +static uint64_t npcm7xx_key_storage_read(void *opaque, hwaddr addr,
> +                                         unsigned int size)
> +{
> +    NPCM7xxOTPRegister reg = addr / sizeof(uint32_t);
> +    NPCM7xxOTPState *s = opaque;
> +
> +    /*
> +     * Only the Fuse Key Index register needs special handling; all other
> +     * registers work the same way for both kinds of OTP modules.
> +     */
> +    if (reg != NPCM7XX_OTP_FKEYIND) {
> +        return npcm7xx_otp_read(s, reg);
> +    }
> +
> +    qemu_log_mask(LOG_UNIMP, "%s: FKEYIND is not implemented\n", __func__);
> +
> +    return s->regs[NPCM7XX_OTP_FKEYIND];
> +}
> +
> +/* Register write handler specific to the key storage OTP module. */
> +static void npcm7xx_key_storage_write(void *opaque, hwaddr addr, uint64_t v,
> +                                      unsigned int size)
> +{
> +    NPCM7xxOTPRegister reg = addr / sizeof(uint32_t);
> +    NPCM7xxOTPState *s = opaque;
> +
> +    /*
> +     * Only the Fuse Key Index register needs special handling; all other
> +     * registers work the same way for both kinds of OTP modules.
> +     */
> +    if (reg != NPCM7XX_OTP_FKEYIND) {
> +        npcm7xx_otp_write(s, reg, v);
> +        return;
> +    }
> +
> +    qemu_log_mask(LOG_UNIMP, "%s: FKEYIND is not implemented\n", __func__);
> +
> +    s->regs[NPCM7XX_OTP_FKEYIND] = v;
> +}
> +
> +static const MemoryRegionOps npcm7xx_key_storage_ops = {
> +    .read       = npcm7xx_key_storage_read,
> +    .write      = npcm7xx_key_storage_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid      = {
> +        .min_access_size        = 4,
> +        .max_access_size        = 4,
> +        .unaligned              = false,
> +    },
> +};
> +
> +static void npcm7xx_otp_enter_reset(Object *obj, ResetType type)
> +{
> +    NPCM7xxOTPState *s = NPCM7XX_OTP(obj);
> +
> +    memset(s->regs, 0, sizeof(s->regs));
> +
> +    s->regs[NPCM7XX_OTP_FST] = 0x00000001;
> +    s->regs[NPCM7XX_OTP_FCFG] = 0x20000000;
> +}
> +
> +static void npcm7xx_otp_realize(DeviceState *dev, Error **errp)
> +{
> +    NPCM7xxOTPClass *oc = NPCM7XX_OTP_GET_CLASS(dev);
> +    NPCM7xxOTPState *s = NPCM7XX_OTP(dev);
> +    SysBusDevice *sbd = &s->parent;
> +
> +    s->array = g_malloc0(NPCM7XX_OTP_ARRAY_BYTES);

The possibility to reuse persistent OTP seems important.
This can be added later of course.

See simple example in hw/nvram/eeprom_at24c.c which use
a BlockBackend so the OTP content is not lost after reset
or migration.

> +
> +    memory_region_init_io(&s->mmio, OBJECT(s), oc->mmio_ops, s, "regs",
> +                          NPCM7XX_OTP_REGS_SIZE);
> +    sysbus_init_mmio(sbd, &s->mmio);
> +}
> +
> +static void npcm7xx_otp_class_init(ObjectClass *klass, void *data)
> +{
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +

Missing migration vmstate for NPCM7xxOTPState::regs[].

> +    dc->realize = npcm7xx_otp_realize;
> +    rc->phases.enter = npcm7xx_otp_enter_reset;
> +}
> +
> +static void npcm7xx_key_storage_class_init(ObjectClass *klass, void *data)
> +{
> +    NPCM7xxOTPClass *oc = NPCM7XX_OTP_CLASS(klass);
> +
> +    oc->mmio_ops = &npcm7xx_key_storage_ops;
> +}
> +
> +static void npcm7xx_fuse_array_class_init(ObjectClass *klass, void *data)
> +{
> +    NPCM7xxOTPClass *oc = NPCM7XX_OTP_CLASS(klass);
> +
> +    oc->mmio_ops = &npcm7xx_fuse_array_ops;
> +}
> +
> +static const TypeInfo npcm7xx_otp_types[] = {
> +    {
> +        .name = TYPE_NPCM7XX_OTP,
> +        .parent = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(NPCM7xxOTPState),
> +        .class_init = npcm7xx_otp_class_init,
> +        .abstract = true,
> +    },
> +    {
> +        .name = TYPE_NPCM7XX_KEY_STORAGE,
> +        .parent = TYPE_NPCM7XX_OTP,
> +        .class_init = npcm7xx_key_storage_class_init,
> +    },
> +    {
> +        .name = TYPE_NPCM7XX_FUSE_ARRAY,
> +        .parent = TYPE_NPCM7XX_OTP,
> +        .class_init = npcm7xx_fuse_array_class_init,
> +    },
> +};
> +DEFINE_TYPES(npcm7xx_otp_types);
> diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h
> index a241af0b8c..360cd965a7 100644
> --- a/include/hw/arm/npcm7xx.h
> +++ b/include/hw/arm/npcm7xx.h
> @@ -19,6 +19,7 @@
>  #include "hw/cpu/a9mpcore.h"
>  #include "hw/misc/npcm7xx_clk.h"
>  #include "hw/misc/npcm7xx_gcr.h"
> +#include "hw/nvram/npcm7xx_otp.h"
>  #include "hw/timer/npcm7xx_timer.h"
>  #include "target/arm/cpu.h"
>  
> @@ -62,6 +63,8 @@ typedef struct NPCM7xxState {
>      NPCM7xxGCRState     gcr;
>      NPCM7xxCLKState     clk;
>      NPCM7xxTimerCtrlState tim[3];
> +    NPCM7xxOTPState     key_storage;
> +    NPCM7xxOTPState     fuse_array;
>  } NPCM7xxState;
>  
>  #define TYPE_NPCM7XX    "npcm7xx"
> diff --git a/include/hw/nvram/npcm7xx_otp.h b/include/hw/nvram/npcm7xx_otp.h
> new file mode 100644
> index 0000000000..1127c48fef
> --- /dev/null
> +++ b/include/hw/nvram/npcm7xx_otp.h
> @@ -0,0 +1,93 @@
> +/*
> + * 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
> + * version 2 as published by the Free Software Foundation.
> + *
> + * 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)
> +
> +/**
> + * 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;
> +} 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)
> +
> +/**
> + * 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 */
>

On Fri, Jul 3, 2020 at 6:46 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 6/27/20 1:55 AM, Havard Skinnemoen wrote:
> > +static void npcm7xx_otp_realize(DeviceState *dev, Error **errp)
> > +{
> > +    NPCM7xxOTPClass *oc = NPCM7XX_OTP_GET_CLASS(dev);
> > +    NPCM7xxOTPState *s = NPCM7XX_OTP(dev);
> > +    SysBusDevice *sbd = &s->parent;
> > +
> > +    s->array = g_malloc0(NPCM7XX_OTP_ARRAY_BYTES);
>
> The possibility to reuse persistent OTP seems important.
> This can be added later of course.

Agree, it's an important part of the behavior of the module. But it's
not essential to be able to boot a BMC firmware image, so I left it
out initially.

> See simple example in hw/nvram/eeprom_at24c.c which use
> a BlockBackend so the OTP content is not lost after reset
> or migration.

I'll take a look at that, thanks!

> > +
> > +    memory_region_init_io(&s->mmio, OBJECT(s), oc->mmio_ops, s, "regs",
> > +                          NPCM7XX_OTP_REGS_SIZE);
> > +    sysbus_init_mmio(sbd, &s->mmio);
> > +}
> > +
> > +static void npcm7xx_otp_class_init(ObjectClass *klass, void *data)
> > +{
> > +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
>
> Missing migration vmstate for NPCM7xxOTPState::regs[].

Ah, you're right. This is probably true for most of the peripherals in
this series. I'll see if I can get it sorted out for the next
iteration.

On Fri, Jul 3, 2020 at 2:07 PM Havard Skinnemoen <hskinnemoen@google.com> wrote:
>
> On Fri, Jul 3, 2020 at 6:46 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 6/27/20 1:55 AM, Havard Skinnemoen wrote:
> > > +static void npcm7xx_otp_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    NPCM7xxOTPClass *oc = NPCM7XX_OTP_GET_CLASS(dev);
> > > +    NPCM7xxOTPState *s = NPCM7XX_OTP(dev);
> > > +    SysBusDevice *sbd = &s->parent;
> > > +
> > > +    s->array = g_malloc0(NPCM7XX_OTP_ARRAY_BYTES);
> >
> > The possibility to reuse persistent OTP seems important.
> > This can be added later of course.
>
> Agree, it's an important part of the behavior of the module. But it's
> not essential to be able to boot a BMC firmware image, so I left it
> out initially.
>
> > See simple example in hw/nvram/eeprom_at24c.c which use
> > a BlockBackend so the OTP content is not lost after reset
> > or migration.
>
> I'll take a look at that, thanks!
>
> > > +
> > > +    memory_region_init_io(&s->mmio, OBJECT(s), oc->mmio_ops, s, "regs",
> > > +                          NPCM7XX_OTP_REGS_SIZE);
> > > +    sysbus_init_mmio(sbd, &s->mmio);
> > > +}
> > > +
> > > +static void npcm7xx_otp_class_init(ObjectClass *klass, void *data)
> > > +{
> > > +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > +
> >
> > Missing migration vmstate for NPCM7xxOTPState::regs[].
>
> Ah, you're right. This is probably true for most of the peripherals in
> this series. I'll see if I can get it sorted out for the next
> iteration.

Btw, do you have any advice on how to test device migration? I'm not
planning to do actual migration of BMCs in the near future, so I'm not
sure if I'd notice if I get it wrong.

On Fri, Jul 3, 2020 at 11:19 PM Havard Skinnemoen
<hskinnemoen@google.com> wrote:
>
> On Fri, Jul 3, 2020 at 2:07 PM Havard Skinnemoen <hskinnemoen@google.com> wrote:
> >
> > On Fri, Jul 3, 2020 at 6:46 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > >
> > > On 6/27/20 1:55 AM, Havard Skinnemoen wrote:
> > > > +static void npcm7xx_otp_realize(DeviceState *dev, Error **errp)
> > > > +{
> > > > +    NPCM7xxOTPClass *oc = NPCM7XX_OTP_GET_CLASS(dev);
> > > > +    NPCM7xxOTPState *s = NPCM7XX_OTP(dev);
> > > > +    SysBusDevice *sbd = &s->parent;
> > > > +
> > > > +    s->array = g_malloc0(NPCM7XX_OTP_ARRAY_BYTES);
> > >
> > > The possibility to reuse persistent OTP seems important.
> > > This can be added later of course.
> >
> > Agree, it's an important part of the behavior of the module. But it's
> > not essential to be able to boot a BMC firmware image, so I left it
> > out initially.
> >
> > > See simple example in hw/nvram/eeprom_at24c.c which use
> > > a BlockBackend so the OTP content is not lost after reset
> > > or migration.
> >
> > I'll take a look at that, thanks!
> >
> > > > +
> > > > +    memory_region_init_io(&s->mmio, OBJECT(s), oc->mmio_ops, s, "regs",
> > > > +                          NPCM7XX_OTP_REGS_SIZE);
> > > > +    sysbus_init_mmio(sbd, &s->mmio);
> > > > +}
> > > > +
> > > > +static void npcm7xx_otp_class_init(ObjectClass *klass, void *data)
> > > > +{
> > > > +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> > > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > > +
> > >
> > > Missing migration vmstate for NPCM7xxOTPState::regs[].
> >
> > Ah, you're right. This is probably true for most of the peripherals in
> > this series. I'll see if I can get it sorted out for the next
> > iteration.
>
> Btw, do you have any advice on how to test device migration? I'm not
> planning to do actual migration of BMCs in the near future, so I'm not
> sure if I'd notice if I get it wrong.

Peter Maydell described it very simply here:
https://translatedcode.wordpress.com/category/debugging-tricks/

On 7/3/20 11:07 PM, Havard Skinnemoen wrote:
> On Fri, Jul 3, 2020 at 6:46 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 6/27/20 1:55 AM, Havard Skinnemoen wrote:
>>> +static void npcm7xx_otp_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    NPCM7xxOTPClass *oc = NPCM7XX_OTP_GET_CLASS(dev);
>>> +    NPCM7xxOTPState *s = NPCM7XX_OTP(dev);
>>> +    SysBusDevice *sbd = &s->parent;
>>> +
>>> +    s->array = g_malloc0(NPCM7XX_OTP_ARRAY_BYTES);
>>
>> The possibility to reuse persistent OTP seems important.
>> This can be added later of course.
> 
> Agree, it's an important part of the behavior of the module. But it's
> not essential to be able to boot a BMC firmware image, so I left it
> out initially.

This is not a problem until someone else try to use it. And OTP/NVRAM
are devices currently in hype. Meanwhile we have another technical debt.
I am just giving generic review comments.

What bugs me particularly is the OTP content zeroed at reset without
any warning. Maybe you can add something like:

  qemu_log_mask(LOG_UNIMP,
                "Persistence not supported, OTP content erased!\n");

in npcm7xx_otp_enter_reset().

> 
>> See simple example in hw/nvram/eeprom_at24c.c which use
>> a BlockBackend so the OTP content is not lost after reset
>> or migration.
> 
> I'll take a look at that, thanks!
> 
>>> +
>>> +    memory_region_init_io(&s->mmio, OBJECT(s), oc->mmio_ops, s, "regs",
>>> +                          NPCM7XX_OTP_REGS_SIZE);
>>> +    sysbus_init_mmio(sbd, &s->mmio);
>>> +}
>>> +
>>> +static void npcm7xx_otp_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +
>>
>> Missing migration vmstate for NPCM7xxOTPState::regs[].
> 
> Ah, you're right. This is probably true for most of the peripherals in
> this series. I'll see if I can get it sorted out for the next
> iteration.

IIUC Peter is ready to queue your series on Monday if you fix the
license, but he usually doesn't accept devices without vmstate via
his tree, you got lucky ;) See:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg643089.html

Adding the vmstate is not a hard task. If you need to respin,
I strongly recommend you to consider adding them.

Excellent work btw :)

Regards,

Phil.

On Fri, Jul 3, 2020 at 3:24 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/3/20 11:07 PM, Havard Skinnemoen wrote:
> > On Fri, Jul 3, 2020 at 6:46 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> On 6/27/20 1:55 AM, Havard Skinnemoen wrote:
> >>> +static void npcm7xx_otp_realize(DeviceState *dev, Error **errp)
> >>> +{
> >>> +    NPCM7xxOTPClass *oc = NPCM7XX_OTP_GET_CLASS(dev);
> >>> +    NPCM7xxOTPState *s = NPCM7XX_OTP(dev);
> >>> +    SysBusDevice *sbd = &s->parent;
> >>> +
> >>> +    s->array = g_malloc0(NPCM7XX_OTP_ARRAY_BYTES);
> >>
> >> The possibility to reuse persistent OTP seems important.
> >> This can be added later of course.
> >
> > Agree, it's an important part of the behavior of the module. But it's
> > not essential to be able to boot a BMC firmware image, so I left it
> > out initially.
>
> This is not a problem until someone else try to use it. And OTP/NVRAM
> are devices currently in hype. Meanwhile we have another technical debt.
> I am just giving generic review comments.
>
> What bugs me particularly is the OTP content zeroed at reset without
> any warning. Maybe you can add something like:
>
>   qemu_log_mask(LOG_UNIMP,
>                 "Persistence not supported, OTP content erased!\n");
>
> in npcm7xx_otp_enter_reset().

Ah, but it only zeroes the registers. The OTP array is only zeroed in realize().

But I see your point about persistence. I will work on that.

One possible complication is that real boards are expected to be
programmed a certain way, so some reasonable defaults need to be
loaded when the user doesn't specify an image (and perhaps also if the
user specifies an "uninitialized" image, if there's any way to detect
that). Would it make sense to add pc-bios blobs that can be loaded by
default, and used as a starting point or backing file for users that
need persistent OTP arrays? Or maybe just OR the factory-programmed
bits into the user-provided image, since that would match the OTP
nature of those bits.

> >
> >> See simple example in hw/nvram/eeprom_at24c.c which use
> >> a BlockBackend so the OTP content is not lost after reset
> >> or migration.
> >
> > I'll take a look at that, thanks!
> >
> >>> +
> >>> +    memory_region_init_io(&s->mmio, OBJECT(s), oc->mmio_ops, s, "regs",
> >>> +                          NPCM7XX_OTP_REGS_SIZE);
> >>> +    sysbus_init_mmio(sbd, &s->mmio);
> >>> +}
> >>> +
> >>> +static void npcm7xx_otp_class_init(ObjectClass *klass, void *data)
> >>> +{
> >>> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> >>> +    DeviceClass *dc = DEVICE_CLASS(klass);
> >>> +
> >>
> >> Missing migration vmstate for NPCM7xxOTPState::regs[].
> >
> > Ah, you're right. This is probably true for most of the peripherals in
> > this series. I'll see if I can get it sorted out for the next
> > iteration.
>
> IIUC Peter is ready to queue your series on Monday if you fix the
> license, but he usually doesn't accept devices without vmstate via
> his tree, you got lucky ;) See:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg643089.html
>
> Adding the vmstate is not a hard task. If you need to respin,
> I strongly recommend you to consider adding them.

Yeah, I'll add vmstate for v4. Thanks for the blog link, that looks
like what I need.