Re: [SPAM] [PATCH v5 05/10] hw/nvram/aspeed_otp: Add OTP programming semantics and tracing

[Cédric Le Goater]

On 8/12/25 11:40, Kane Chen wrote:

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

Implement correct OTP programming behavior for Aspeed OTP:
- Support read-modify-write flow with one-way bit programming:
   * prog_bit uses 0s as the "to-be-programmed" mask.
   * Even-indexed words: 0->1, odd-indexed words: 1->0.
   * Reject non-programmable requests and log conflicts.
- Enable unaligned accesses in MemoryRegionOps.
   Since each OTP address maps to a 1DW (4B) or 2DW (8B) block in the
   backing store, upper-layer accesses may be unaligned to block
   boundaries.

This matches the irreversible, word-parity-dependent programming rules
of Aspeed SoCs and exposes changes via QEMU trace events.

Signed-off-by: Kane-Chen-AS <kane_c...@aspeedtech.com>

Reviewed-by: Cédric Le Goater <c...@redhat.com>

Thanks,

C.

---
  hw/nvram/aspeed_otp.c | 80 ++++++++++++++++++++++++++++++++++++++++++-
  hw/nvram/trace-events |  5 +++
  2 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/hw/nvram/aspeed_otp.c b/hw/nvram/aspeed_otp.c
index abb3731823..66b2bfea28 100644
--- a/hw/nvram/aspeed_otp.c
+++ b/hw/nvram/aspeed_otp.c
@@ -12,6 +12,7 @@
  #include "system/block-backend.h"
  #include "hw/qdev-properties.h"
  #include "hw/nvram/aspeed_otp.h"
+#include "hw/nvram/trace.h"

static uint64_t aspeed_otp_read(void *opaque, hwaddr offset, unsigned size)

  {
@@ -23,12 +24,87 @@ static uint64_t aspeed_otp_read(void *opaque, hwaddr 
offset, unsigned size)
      return val;
  }

+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 any bit can be programmed, accept the request */
+    has_programmable_bits = value ^ (~prog_bit);
+
+    if (programmed_bits) {
+        trace_aspeed_otp_prog_conflict(otp_addr, programmed_bits);
+        for (int i = 0; i < 32; ++i) {
+            if (programmed_bits & (1U << i)) {
+                trace_aspeed_otp_prog_bit(i);
+            }
+        }
+    }
+
+    return has_programmable_bits != 0;
+}
+
+static bool program_otpmem_data(void *opaque, uint32_t otp_addr,
+                             uint32_t prog_bit, uint32_t *value)
+{
+    AspeedOTPState *s = opaque;
+    bool is_odd = otp_addr & 1;
+    uint32_t otp_offset = otp_addr << 2;
+
+    memcpy(value, s->storage + otp_offset, sizeof(uint32_t));
+
+    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_otp_write(void *opaque, hwaddr otp_addr,
                                  uint64_t val, unsigned size)
  {
      AspeedOTPState *s = opaque;
+    uint32_t otp_offset, value;

- memcpy(s->storage + otp_addr, &val, size);

+    if (!program_otpmem_data(s, otp_addr, val, &value)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Failed to program data, value = %x, bit = %lx\n",
+                      __func__, value, val);
+        return;
+    }
+
+    otp_offset = otp_addr << 2;
+    memcpy(s->storage + otp_offset, &value, size);
+
+    if (s->blk) {
+        if (blk_pwrite(s->blk, otp_offset, size, &value, 0) < 0) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Failed to write %x to %x\n",
+                          __func__, value, otp_offset);
+
+            return;
+        }
+    }
+    trace_aspeed_otp_prog(otp_offset, val, value);
  }

static bool aspeed_otp_init_storage(AspeedOTPState *s, Error **errp)

@@ -63,6 +139,8 @@ static const MemoryRegionOps aspeed_otp_ops = {
      .endianness = DEVICE_LITTLE_ENDIAN,
      .valid.min_access_size = 1,
      .valid.max_access_size = 4,
+    .valid.unaligned = true,
+    .impl.unaligned = true
  };

static void aspeed_otp_realize(DeviceState *dev, Error **errp)

diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events
index 5e33b24d47..7084bf70d3 100644
--- a/hw/nvram/trace-events
+++ b/hw/nvram/trace-events
@@ -1,5 +1,10 @@
  # See docs/devel/tracing.rst for syntax documentation.

+# aspeed_otp.c

+aspeed_otp_prog(uint32_t addr, uint32_t prog_value, uint32_t value) "OTP Memory program: addr 0x%" 
PRIx32 " prog_value 0x%" PRIx32 " value 0x%" PRIx32
+aspeed_otp_prog_conflict(uint32_t addr, uint32_t bits) "Conflict at addr=0x%x, 
bits=0x%08x"
+aspeed_otp_prog_bit(int bit) "Programmed bit %d"
+
  # ds1225y.c
  nvram_read(uint32_t addr, uint32_t ret) "read addr %d: 0x%02x"
  nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%02x -> 
0x%02x"