From: Kane-Chen-AS <kane_chen@aspeedtech.com>

This introduces a new model for the ASPEED OTP (One-Time Programmable)
memory. The device is implemented as a `SysBusDevice` and provides an
abstracted interface for OTP read, write (program), and default value
initialization.

OTP content is backed by a block device and supports QEMU’s drive
infrastructure via the "drive" property.

Features:
- Enforces irreversible bit programming logic (0->1 or 1->0)
- Provides interface for SoC/secure controller integration
- Validates bounds and bit-level constraints
- Uses QEMU error handling conventions and logging

Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
 hw/misc/aspeed_otpmem.c         | 211 ++++++++++++++++++++++++++++++++
 hw/misc/meson.build             |   1 +
 include/hw/misc/aspeed_otpmem.h |  40 ++++++
 3 files changed, 252 insertions(+)
 create mode 100644 hw/misc/aspeed_otpmem.c
 create mode 100644 include/hw/misc/aspeed_otpmem.h

diff --git a/hw/misc/aspeed_otpmem.c b/hw/misc/aspeed_otpmem.c
new file mode 100644
index 0000000000..4f8f2827f7
--- /dev/null
+++ b/hw/misc/aspeed_otpmem.c
@@ -0,0 +1,211 @@
+/*
+ *  ASPEED OTP (One-Time Programmable) memory
+ *
+ *  Copyright (C) 2025 Aspeed
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/block/block.h"
+#include "hw/block/flash.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
+#include "system/block-backend.h"
+#include "qemu/log.h"
+#include "qemu/option.h"
+#include "hw/sysbus.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_otpmem.h"
+
+static const Property aspeed_otpmem_properties[] = {
+    DEFINE_PROP_DRIVE("drive", AspeedOTPMemState, blk),
+};
+
+static void aspeed_otpmem_read(void *opaque, uint32_t addr,
+                               uint32_t *out, Error **errp)
+{
+    AspeedOTPMemState *otp = ASPEED_OTPMEM(opaque);
+
+    assert(otp->blk);
+
+    if (out == NULL) {
+        error_setg(errp, "out is NULL");
+        return;
+    }
+
+    if (addr > (otp->max_size - 4)) {
+        error_setg(errp, "OTP memory 0x%x is exceeded", addr);
+        return;
+    }
+
+    if (blk_pread(otp->blk, (int64_t)addr, sizeof(uint32_t), out, 0) < 0) {
+        error_setg(errp, "Failed to read data 0x%x", addr);
+        return;
+    }
+    return;
+}
+
+static bool valid_program_data(uint32_t otp_addr,
+                                 uint32_t value, uint32_t prog_bit)
+{
+    uint32_t programmed_bits, has_programmable_bits;
+    bool is_odd = otp_addr & 1;
+
+    /*
+     * prog_bit uses 0s to indicate target bits to program:
+     *   - if OTP word is even-indexed, programmed bits flip 0->1
+     *   - if odd, bits flip 1->0
+     * Bit programming is one-way only and irreversible.
+     */
+    if (is_odd) {
+        programmed_bits = ~value & prog_bit;
+    } else {
+        programmed_bits = value & (~prog_bit);
+    }
+
+    /* If there is some bit can be programed, to accept the request */
+    has_programmable_bits = value ^ (~prog_bit);
+
+    if (programmed_bits) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Found programmed bits in addr %x\n",
+                      __func__, otp_addr);
+        for (int i = 0; i < 32; ++i) {
+            if (programmed_bits & (1U << i)) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "  Programmed bit %d\n",
+                              i);
+            }
+        }
+    }
+
+    return has_programmable_bits != 0;
+}
+
+static bool program_otpmem_data(void *opaque, uint32_t otp_addr,
+                             uint32_t prog_bit, uint32_t *value)
+{
+    AspeedOTPMemState *s = ASPEED_OTPMEM(opaque);
+    bool is_odd = otp_addr & 1;
+    uint32_t otp_offset = otp_addr << 2;
+
+    if (blk_pread(s->blk, (int64_t)otp_offset,
+                  sizeof(uint32_t), value, 0) < 0) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Failed to read data 0x%x\n",
+                      __func__, otp_offset);
+        return false;
+    }
+
+    if (!valid_program_data(otp_addr, *value, prog_bit)) {
+        return false;
+    }
+
+    if (is_odd) {
+        *value &= ~prog_bit;
+    } else {
+        *value |= ~prog_bit;
+    }
+
+    return true;
+}
+
+static void aspeed_otpmem_prog(void *s, uint32_t otp_addr,
+                               uint32_t data, Error **errp)
+{
+    AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
+    uint32_t otp_offset, value;
+
+    assert(otp->blk);
+
+    if (otp_addr > (otp->max_size >> 2)) {
+        error_setg(errp, "OTP memory 0x%x is exceeded", otp_addr);
+        return;
+    }
+
+    otp_offset = otp_addr << 2;
+    if (!program_otpmem_data(s, otp_addr, data, &value)) {
+        error_setg(errp, "Failed to program data");
+        return;
+    }
+
+    if (blk_pwrite(otp->blk, (int64_t)otp_offset,
+                   sizeof(value), &value, 0) < 0) {
+        error_setg(errp, "Failed to write data");
+    }
+
+    return;
+}
+
+static void aspeed_otpmem_set_default(void *s, uint32_t otp_offset,
+                                      uint32_t data, Error **errp)
+{
+    AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
+
+    if ((otp_offset + 4) > otp->max_size) {
+        error_setg(errp, "OTP memory 0x%x is exceeded", otp_offset);
+        return;
+    }
+
+    if (blk_pwrite(otp->blk, (int64_t)otp_offset,
+                   sizeof(data), &data, 0) < 0) {
+        error_setg(errp, "Failed to write data");
+    }
+    return;
+}
+
+static AspeedOTPMemOps aspeed_otpmem_ops = {
+    .read = aspeed_otpmem_read,
+    .prog = aspeed_otpmem_prog,
+    .set_default_value = aspeed_otpmem_set_default
+};
+
+static void aspeed_otpmem_realize(DeviceState *dev, Error **errp)
+{
+    AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
+
+    if (!s->blk) {
+        error_setg(&error_fatal, "OTP memory is not initialized");
+        return;
+    }
+
+    s->max_size = blk_getlength(s->blk);
+    if (s->max_size < 0 || (s->max_size % 4)) {
+        error_setg(&error_fatal,
+                   "Unexpected OTP memory size: %" PRId64 "",
+                   s->max_size);
+        return;
+    }
+
+    s->ops = &aspeed_otpmem_ops;
+
+    return;
+}
+
+static void aspeed_otpmem_system_reset(DeviceState *dev)
+{
+    return;
+}
+
+static void aspeed_otpmem_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    device_class_set_legacy_reset(dc, aspeed_otpmem_system_reset);
+    dc->realize = aspeed_otpmem_realize;
+    device_class_set_props(dc, aspeed_otpmem_properties);
+
+}
+
+static const TypeInfo aspeed_otpmem_types[] = {
+    {
+        .name           = TYPE_ASPEED_OTPMEM,
+        .parent         = TYPE_SYS_BUS_DEVICE,
+        .instance_size  = sizeof(AspeedOTPMemState),
+        .class_init     = aspeed_otpmem_class_init,
+    },
+};
+
+DEFINE_TYPES(aspeed_otpmem_types)
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 6d47de482c..ed1eaaa2ad 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -136,6 +136,7 @@ system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
   'aspeed_sbc.c',
   'aspeed_sdmc.c',
   'aspeed_xdma.c',
+  'aspeed_otpmem.c',
   'aspeed_peci.c',
   'aspeed_sli.c'))
 
diff --git a/include/hw/misc/aspeed_otpmem.h b/include/hw/misc/aspeed_otpmem.h
new file mode 100644
index 0000000000..11e2de70b6
--- /dev/null
+++ b/include/hw/misc/aspeed_otpmem.h
@@ -0,0 +1,40 @@
+/*
+ *  ASPEED OTP (One-Time Programmable) memory
+ *
+ *  Copyright (C) 2025 Aspeed
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef ASPEED_OTPMMEM_H
+#define ASPEED_OTPMMEM_H
+
+#include "hw/sysbus.h"
+#include "qapi/error.h"
+
+#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
+#define ASPEED_OTPMEM_DRIVE "otpmem"
+
+#define ASPEED_OTPMEM(obj) OBJECT_CHECK(AspeedOTPMemState, (obj), \
+                                        TYPE_ASPEED_OTPMEM)
+
+typedef struct AspeedOTPMemOps {
+    void (*read)(void *s, uint32_t addr, uint32_t *out, Error **errp);
+    void (*prog)(void *s, uint32_t addr, uint32_t data, Error **errp);
+    void (*set_default_value)(void *s, uint32_t otp_offset,
+                              uint32_t data, Error **errp);
+} AspeedOTPMemOps;
+
+typedef struct AspeedOTPMemState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion mmio;
+    BlockBackend *blk;
+    int64_t max_size;
+
+    AspeedOTPMemOps *ops;
+} AspeedOTPMemState;
+
+#endif /* ASPEED_OTPMMEM_H */
+
-- 
2.43.0

On 4/23/25 04:56, Kane Chen wrote:
> From: Kane-Chen-AS <kane_chen@aspeedtech.com>
> 
> This introduces a new model for the ASPEED OTP (One-Time Programmable)
> memory. The device is implemented as a `SysBusDevice` and provides an
> abstracted interface for OTP read, write (program), and default value
> initialization.
> 
> OTP content is backed by a block device and supports QEMU’s drive
> infrastructure via the "drive" property.
> 
> Features:
> - Enforces irreversible bit programming logic (0->1 or 1->0)
> - Provides interface for SoC/secure controller integration
> - Validates bounds and bit-level constraints
> - Uses QEMU error handling conventions and logging
> 
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
>   hw/misc/aspeed_otpmem.c         | 211 ++++++++++++++++++++++++++++++++
>   hw/misc/meson.build             |   1 +
>   include/hw/misc/aspeed_otpmem.h |  40 ++++++
>   3 files changed, 252 insertions(+)
>   create mode 100644 hw/misc/aspeed_otpmem.c
>   create mode 100644 include/hw/misc/aspeed_otpmem.h
> 
> diff --git a/hw/misc/aspeed_otpmem.c b/hw/misc/aspeed_otpmem.c
> new file mode 100644
> index 0000000000..4f8f2827f7
> --- /dev/null
> +++ b/hw/misc/aspeed_otpmem.c
> @@ -0,0 +1,211 @@
> +/*
> + *  ASPEED OTP (One-Time Programmable) memory
> + *
> + *  Copyright (C) 2025 Aspeed
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/block/block.h"
> +#include "hw/block/flash.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "system/block-backend.h"
> +#include "qemu/log.h"
> +#include "qemu/option.h"
> +#include "hw/sysbus.h"
> +#include "qemu/error-report.h"
> +#include "hw/misc/aspeed_otpmem.h"
> +
> +static const Property aspeed_otpmem_properties[] = {
> +    DEFINE_PROP_DRIVE("drive", AspeedOTPMemState, blk),
> +};

Usually the 'Property' array is defined just before the class_init
routine. Please move it there.

> +static void aspeed_otpmem_read(void *opaque, uint32_t addr,
> +                               uint32_t *out, Error **errp)

hmm, that's not a MemoryRegion handler. Why not ? I will check in the
following patches

> +{
> +    AspeedOTPMemState *otp = ASPEED_OTPMEM(opaque);
> +
> +    assert(otp->blk);

This check shouldn't be needed if the backend id initialized in the
realize routine.

> +    if (out == NULL) {
> +        error_setg(errp, "out is NULL");
> +        return;
> +    }
> +
> +    if (addr > (otp->max_size - 4)) {

Why a MemoryRegion, no need for this check.

> +        error_setg(errp, "OTP memory 0x%x is exceeded", addr);
> +        return;
> +    }
> +
> +    if (blk_pread(otp->blk, (int64_t)addr, sizeof(uint32_t), out, 0) < 0) {
> +        error_setg(errp, "Failed to read data 0x%x", addr);
> +        return;
> +    }
> +    return;
> +}
> +
> +static bool valid_program_data(uint32_t otp_addr,
> +                                 uint32_t value, uint32_t prog_bit)
> +{
> +    uint32_t programmed_bits, has_programmable_bits;
> +    bool is_odd = otp_addr & 1;
> +
> +    /*
> +     * prog_bit uses 0s to indicate target bits to program:
> +     *   - if OTP word is even-indexed, programmed bits flip 0->1
> +     *   - if odd, bits flip 1->0
> +     * Bit programming is one-way only and irreversible.
> +     */
> +    if (is_odd) {
> +        programmed_bits = ~value & prog_bit;
> +    } else {
> +        programmed_bits = value & (~prog_bit);
> +    }
> +
> +    /* If there is some bit can be programed, to accept the request */
> +    has_programmable_bits = value ^ (~prog_bit);
> +
> +    if (programmed_bits) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Found programmed bits in addr %x\n",
> +                      __func__, otp_addr);
> +        for (int i = 0; i < 32; ++i) {
> +            if (programmed_bits & (1U << i)) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "  Programmed bit %d\n",
> +                              i);
> +            }
> +        }
> +    }
> +
> +    return has_programmable_bits != 0;
> +}
> +
> +static bool program_otpmem_data(void *opaque, uint32_t otp_addr,
> +                             uint32_t prog_bit, uint32_t *value)
> +{
> +    AspeedOTPMemState *s = ASPEED_OTPMEM(opaque);
> +    bool is_odd = otp_addr & 1;
> +    uint32_t otp_offset = otp_addr << 2;
> +
> +    if (blk_pread(s->blk, (int64_t)otp_offset,
> +                  sizeof(uint32_t), value, 0) < 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Failed to read data 0x%x\n",
> +                      __func__, otp_offset);
> +        return false;
> +    }
> +
> +    if (!valid_program_data(otp_addr, *value, prog_bit)) {
> +        return false;
> +    }
> +
> +    if (is_odd) {
> +        *value &= ~prog_bit;
> +    } else {
> +        *value |= ~prog_bit;
> +    }
> +
> +    return true;
> +}
> +
> +static void aspeed_otpmem_prog(void *s, uint32_t otp_addr,
> +                               uint32_t data, Error **errp)
> +{
> +    AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
> +    uint32_t otp_offset, value;
> +
> +    assert(otp->blk);
> +
> +    if (otp_addr > (otp->max_size >> 2)) {
> +        error_setg(errp, "OTP memory 0x%x is exceeded", otp_addr);
> +        return;
> +    }
> +
> +    otp_offset = otp_addr << 2;
> +    if (!program_otpmem_data(s, otp_addr, data, &value)) {
> +        error_setg(errp, "Failed to program data");
> +        return;
> +    }
> +
> +    if (blk_pwrite(otp->blk, (int64_t)otp_offset,
> +                   sizeof(value), &value, 0) < 0) {
> +        error_setg(errp, "Failed to write data");
> +    }
> +
> +    return;
> +}
> +
> +static void aspeed_otpmem_set_default(void *s, uint32_t otp_offset,
> +                                      uint32_t data, Error **errp)
> +{
> +    AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
> +
> +    if ((otp_offset + 4) > otp->max_size) {
> +        error_setg(errp, "OTP memory 0x%x is exceeded", otp_offset);
> +        return;
> +    }
> +
> +    if (blk_pwrite(otp->blk, (int64_t)otp_offset,
> +                   sizeof(data), &data, 0) < 0) {
> +        error_setg(errp, "Failed to write data");
> +    }
> +    return;
> +}
> +
> +static AspeedOTPMemOps aspeed_otpmem_ops = {
> +    .read = aspeed_otpmem_read,
> +    .prog = aspeed_otpmem_prog,
> +    .set_default_value = aspeed_otpmem_set_default
> +};
> +
> +static void aspeed_otpmem_realize(DeviceState *dev, Error **errp)
> +{
> +    AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
> +
> +    if (!s->blk) {
> +        error_setg(&error_fatal, "OTP memory is not initialized");
> +        return;
> +    }
> +
> +    s->max_size = blk_getlength(s->blk);
> +    if (s->max_size < 0 || (s->max_size % 4)) {
> +        error_setg(&error_fatal,
> +                   "Unexpected OTP memory size: %" PRId64 "",
> +                   s->max_size);
> +        return;
> +    }
> +
> +    s->ops = &aspeed_otpmem_ops;

You should consider using an AddressSpace for the OTP transactions.

> +    return;
> +}
> +
> +static void aspeed_otpmem_system_reset(DeviceState *dev)
> +{
> +    return;
> +}
> +

Reset is empty. Please remove.


> +static void aspeed_otpmem_class_init(ObjectClass *klass, void *data)> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    device_class_set_legacy_reset(dc, aspeed_otpmem_system_reset);
> +    dc->realize = aspeed_otpmem_realize;
> +    device_class_set_props(dc, aspeed_otpmem_properties);
> +
> +}
> +
> +static const TypeInfo aspeed_otpmem_types[] = {
> +    {
> +        .name           = TYPE_ASPEED_OTPMEM,
> +        .parent         = TYPE_SYS_BUS_DEVICE,
> +        .instance_size  = sizeof(AspeedOTPMemState),
> +        .class_init     = aspeed_otpmem_class_init,
> +    },
> +};
> +
> +DEFINE_TYPES(aspeed_otpmem_types)
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 6d47de482c..ed1eaaa2ad 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -136,6 +136,7 @@ system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
>     'aspeed_sbc.c',
>     'aspeed_sdmc.c',
>     'aspeed_xdma.c',
> +  'aspeed_otpmem.c',
>     'aspeed_peci.c',
>     'aspeed_sli.c'))
>   
> diff --git a/include/hw/misc/aspeed_otpmem.h b/include/hw/misc/aspeed_otpmem.h
> new file mode 100644
> index 0000000000..11e2de70b6
> --- /dev/null
> +++ b/include/hw/misc/aspeed_otpmem.h
> @@ -0,0 +1,40 @@
> +/*
> + *  ASPEED OTP (One-Time Programmable) memory
> + *
> + *  Copyright (C) 2025 Aspeed
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef ASPEED_OTPMMEM_H
> +#define ASPEED_OTPMMEM_H
> +
> +#include "hw/sysbus.h"
> +#include "qapi/error.h"
> +
> +#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
> +#define ASPEED_OTPMEM_DRIVE "otpmem"

This ASPEED_OTPMEM_DRIVE definition looks wrong to me. What is it for ?

Thanks,

C.


> +
> +#define ASPEED_OTPMEM(obj) OBJECT_CHECK(AspeedOTPMemState, (obj), \
> +                                        TYPE_ASPEED_OTPMEM)
> +
> +typedef struct AspeedOTPMemOps {
> +    void (*read)(void *s, uint32_t addr, uint32_t *out, Error **errp);
> +    void (*prog)(void *s, uint32_t addr, uint32_t data, Error **errp);
> +    void (*set_default_value)(void *s, uint32_t otp_offset,
> +                              uint32_t data, Error **errp);
> +} AspeedOTPMemOps;
> +
> +typedef struct AspeedOTPMemState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion mmio;
> +    BlockBackend *blk;
> +    int64_t max_size;
> +
> +    AspeedOTPMemOps *ops;
> +} AspeedOTPMemState;
> +
> +#endif /* ASPEED_OTPMMEM_H */
> +

Hi Cédric,

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, April 28, 2025 3:06 PM
> To: Kane Chen <kane_chen@aspeedtech.com>; Peter Maydell
> <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy
> Lee <leetroy@gmail.com>; Jamin Lin <jamin_lin@aspeedtech.com>; Andrew
> Jeffery <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>; open
> list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>
> Subject: Re: [PATCH v3 1/3] hw/misc/aspeed_otp: Add Aspeed OTP memory
> device model
> 
> On 4/23/25 04:56, Kane Chen wrote:
> > From: Kane-Chen-AS <kane_chen@aspeedtech.com>
> >
> > This introduces a new model for the ASPEED OTP (One-Time Programmable)
> > memory. The device is implemented as a `SysBusDevice` and provides an
> > abstracted interface for OTP read, write (program), and default value
> > initialization.
> >
> > OTP content is backed by a block device and supports QEMU’s drive
> > infrastructure via the "drive" property.
> >
> > Features:
> > - Enforces irreversible bit programming logic (0->1 or 1->0)
> > - Provides interface for SoC/secure controller integration
> > - Validates bounds and bit-level constraints
> > - Uses QEMU error handling conventions and logging
> >
> > Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> > ---
> >   hw/misc/aspeed_otpmem.c         | 211
> ++++++++++++++++++++++++++++++++
> >   hw/misc/meson.build             |   1 +
> >   include/hw/misc/aspeed_otpmem.h |  40 ++++++
> >   3 files changed, 252 insertions(+)
> >   create mode 100644 hw/misc/aspeed_otpmem.c
> >   create mode 100644 include/hw/misc/aspeed_otpmem.h
> >
> > diff --git a/hw/misc/aspeed_otpmem.c b/hw/misc/aspeed_otpmem.c new
> > file mode 100644 index 0000000000..4f8f2827f7
> > --- /dev/null
> > +++ b/hw/misc/aspeed_otpmem.c
> > @@ -0,0 +1,211 @@
> > +/*
> > + *  ASPEED OTP (One-Time Programmable) memory
> > + *
> > + *  Copyright (C) 2025 Aspeed
> > + *
> > + * This code is licensed under the GPL version 2 or later.  See
> > + * the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/block/block.h"
> > +#include "hw/block/flash.h"
> > +#include "hw/qdev-properties.h"
> > +#include "hw/qdev-properties-system.h"
> > +#include "system/block-backend.h"
> > +#include "qemu/log.h"
> > +#include "qemu/option.h"
> > +#include "hw/sysbus.h"
> > +#include "qemu/error-report.h"
> > +#include "hw/misc/aspeed_otpmem.h"
> > +
> > +static const Property aspeed_otpmem_properties[] = {
> > +    DEFINE_PROP_DRIVE("drive", AspeedOTPMemState, blk), };
> 
> Usually the 'Property' array is defined just before the class_init routine. Please
> move it there.
> 
I will move the Property array to just before the class_init routine
in the next version of the patch.
> > +static void aspeed_otpmem_read(void *opaque, uint32_t addr,
> > +                               uint32_t *out, Error **errp)
> 
> hmm, that's not a MemoryRegion handler. Why not ? I will check in the
> following patches
> 
In the AST1030 and AST2600, the OTP memory is accessed through the SBC
(Secure Boot Controller) and cannot be accessed directly. Therefore,
I did not implement a MemoryRegion handler. If you have any concerns
regarding this design, please let me know.

> > +{
> > +    AspeedOTPMemState *otp = ASPEED_OTPMEM(opaque);
> > +
> > +    assert(otp->blk);
> 
> This check shouldn't be needed if the backend id initialized in the realize
> routine.
> 
> > +    if (out == NULL) {
> > +        error_setg(errp, "out is NULL");
> > +        return;
> > +    }
> > +
> > +    if (addr > (otp->max_size - 4)) {
> 
> Why a MemoryRegion, no need for this check.
I will remove the assert and the address boundary check code segments
you pointed out in the next version of the patch.

> 
> > +        error_setg(errp, "OTP memory 0x%x is exceeded", addr);
> > +        return;
> > +    }
> > +
> > +    if (blk_pread(otp->blk, (int64_t)addr, sizeof(uint32_t), out, 0) < 0) {
> > +        error_setg(errp, "Failed to read data 0x%x", addr);
> > +        return;
> > +    }
> > +    return;
> > +}
> > +
> > +static bool valid_program_data(uint32_t otp_addr,
> > +                                 uint32_t value, uint32_t prog_bit) {
> > +    uint32_t programmed_bits, has_programmable_bits;
> > +    bool is_odd = otp_addr & 1;
> > +
> > +    /*
> > +     * prog_bit uses 0s to indicate target bits to program:
> > +     *   - if OTP word is even-indexed, programmed bits flip 0->1
> > +     *   - if odd, bits flip 1->0
> > +     * Bit programming is one-way only and irreversible.
> > +     */
> > +    if (is_odd) {
> > +        programmed_bits = ~value & prog_bit;
> > +    } else {
> > +        programmed_bits = value & (~prog_bit);
> > +    }
> > +
> > +    /* If there is some bit can be programed, to accept the request */
> > +    has_programmable_bits = value ^ (~prog_bit);
> > +
> > +    if (programmed_bits) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Found programmed bits in addr %x\n",
> > +                      __func__, otp_addr);
> > +        for (int i = 0; i < 32; ++i) {
> > +            if (programmed_bits & (1U << i)) {
> > +                qemu_log_mask(LOG_GUEST_ERROR,
> > +                              "  Programmed bit %d\n",
> > +                              i);
> > +            }
> > +        }
> > +    }
> > +
> > +    return has_programmable_bits != 0; }
> > +
> > +static bool program_otpmem_data(void *opaque, uint32_t otp_addr,
> > +                             uint32_t prog_bit, uint32_t *value) {
> > +    AspeedOTPMemState *s = ASPEED_OTPMEM(opaque);
> > +    bool is_odd = otp_addr & 1;
> > +    uint32_t otp_offset = otp_addr << 2;
> > +
> > +    if (blk_pread(s->blk, (int64_t)otp_offset,
> > +                  sizeof(uint32_t), value, 0) < 0) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Failed to read data 0x%x\n",
> > +                      __func__, otp_offset);
> > +        return false;
> > +    }
> > +
> > +    if (!valid_program_data(otp_addr, *value, prog_bit)) {
> > +        return false;
> > +    }
> > +
> > +    if (is_odd) {
> > +        *value &= ~prog_bit;
> > +    } else {
> > +        *value |= ~prog_bit;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +static void aspeed_otpmem_prog(void *s, uint32_t otp_addr,
> > +                               uint32_t data, Error **errp) {
> > +    AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
> > +    uint32_t otp_offset, value;
> > +
> > +    assert(otp->blk);
> > +
> > +    if (otp_addr > (otp->max_size >> 2)) {
> > +        error_setg(errp, "OTP memory 0x%x is exceeded", otp_addr);
> > +        return;
> > +    }
> > +
> > +    otp_offset = otp_addr << 2;
> > +    if (!program_otpmem_data(s, otp_addr, data, &value)) {
> > +        error_setg(errp, "Failed to program data");
> > +        return;
> > +    }
> > +
> > +    if (blk_pwrite(otp->blk, (int64_t)otp_offset,
> > +                   sizeof(value), &value, 0) < 0) {
> > +        error_setg(errp, "Failed to write data");
> > +    }
> > +
> > +    return;
> > +}
> > +
> > +static void aspeed_otpmem_set_default(void *s, uint32_t otp_offset,
> > +                                      uint32_t data, Error **errp)
> {
> > +    AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
> > +
> > +    if ((otp_offset + 4) > otp->max_size) {
> > +        error_setg(errp, "OTP memory 0x%x is exceeded", otp_offset);
> > +        return;
> > +    }
> > +
> > +    if (blk_pwrite(otp->blk, (int64_t)otp_offset,
> > +                   sizeof(data), &data, 0) < 0) {
> > +        error_setg(errp, "Failed to write data");
> > +    }
> > +    return;
> > +}
> > +
> > +static AspeedOTPMemOps aspeed_otpmem_ops = {
> > +    .read = aspeed_otpmem_read,
> > +    .prog = aspeed_otpmem_prog,
> > +    .set_default_value = aspeed_otpmem_set_default };
> > +
> > +static void aspeed_otpmem_realize(DeviceState *dev, Error **errp) {
> > +    AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
> > +
> > +    if (!s->blk) {
> > +        error_setg(&error_fatal, "OTP memory is not initialized");
> > +        return;
> > +    }
> > +
> > +    s->max_size = blk_getlength(s->blk);
> > +    if (s->max_size < 0 || (s->max_size % 4)) {
> > +        error_setg(&error_fatal,
> > +                   "Unexpected OTP memory size: %" PRId64 "",
> > +                   s->max_size);
> > +        return;
> > +    }
> > +
> > +    s->ops = &aspeed_otpmem_ops;
> 
> You should consider using an AddressSpace for the OTP transactions.
> 
Regarding the use of ops function pointers: the OTP memory behaves
similarly to an efuse device (like xlnx-efuse.c), but with different
read/write rules. Additionally, the access behaviors differ across
different ASPEED SoCs (e.g., AST2700 vs AST2600/AST1030), so I chose
a flexible ops-based design for potential future extension. I will
also evaluate whether using an AddressSpace would fit the requirements
better.
> > +    return;
> > +}
> > +
> > +static void aspeed_otpmem_system_reset(DeviceState *dev) {
> > +    return;
> > +}
> > +
> 
> Reset is empty. Please remove.
I will remove the empty reset function in the next patch as suggested.
> 
> 
> > +static void aspeed_otpmem_class_init(ObjectClass *klass, void *data)> +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    device_class_set_legacy_reset(dc, aspeed_otpmem_system_reset);
> > +    dc->realize = aspeed_otpmem_realize;
> > +    device_class_set_props(dc, aspeed_otpmem_properties);
> > +
> > +}
> > +
> > +static const TypeInfo aspeed_otpmem_types[] = {
> > +    {
> > +        .name           = TYPE_ASPEED_OTPMEM,
> > +        .parent         = TYPE_SYS_BUS_DEVICE,
> > +        .instance_size  = sizeof(AspeedOTPMemState),
> > +        .class_init     = aspeed_otpmem_class_init,
> > +    },
> > +};
> > +
> > +DEFINE_TYPES(aspeed_otpmem_types)
> > diff --git a/hw/misc/meson.build b/hw/misc/meson.build index
> > 6d47de482c..ed1eaaa2ad 100644
> > --- a/hw/misc/meson.build
> > +++ b/hw/misc/meson.build
> > @@ -136,6 +136,7 @@ system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true:
> files(
> >     'aspeed_sbc.c',
> >     'aspeed_sdmc.c',
> >     'aspeed_xdma.c',
> > +  'aspeed_otpmem.c',
> >     'aspeed_peci.c',
> >     'aspeed_sli.c'))
> >
> > diff --git a/include/hw/misc/aspeed_otpmem.h
> > b/include/hw/misc/aspeed_otpmem.h new file mode 100644 index
> > 0000000000..11e2de70b6
> > --- /dev/null
> > +++ b/include/hw/misc/aspeed_otpmem.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + *  ASPEED OTP (One-Time Programmable) memory
> > + *
> > + *  Copyright (C) 2025 Aspeed
> > + *
> > + * This code is licensed under the GPL version 2 or later.  See
> > + * the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef ASPEED_OTPMMEM_H
> > +#define ASPEED_OTPMMEM_H
> > +
> > +#include "hw/sysbus.h"
> > +#include "qapi/error.h"
> > +
> > +#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
> > +#define ASPEED_OTPMEM_DRIVE "otpmem"
> 
> This ASPEED_OTPMEM_DRIVE definition looks wrong to me. What is it for ?
> 
The ASPEED_OTPMEM_DRIVE macro is actually used in a later patch
(patch 3: "hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs").
I will move this macro definition to the appropriate patch file.

> Thanks,
> 
> C.
> 
> 
> > +
> > +#define ASPEED_OTPMEM(obj) OBJECT_CHECK(AspeedOTPMemState, (obj),
> \
> > +                                        TYPE_ASPEED_OTPMEM)
> > +
> > +typedef struct AspeedOTPMemOps {
> > +    void (*read)(void *s, uint32_t addr, uint32_t *out, Error **errp);
> > +    void (*prog)(void *s, uint32_t addr, uint32_t data, Error **errp);
> > +    void (*set_default_value)(void *s, uint32_t otp_offset,
> > +                              uint32_t data, Error **errp); }
> > +AspeedOTPMemOps;
> > +
> > +typedef struct AspeedOTPMemState {
> > +    SysBusDevice parent_obj;
> > +
> > +    MemoryRegion mmio;
> > +    BlockBackend *blk;
> > +    int64_t max_size;
> > +
> > +    AspeedOTPMemOps *ops;
> > +} AspeedOTPMemState;
> > +
> > +#endif /* ASPEED_OTPMMEM_H */
> > +
Thanks again for your comments and review.

Best Regards,
Kane