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

Introduce a QEMU device model for ASPEED's One-Time Programmable (OTP)
memory.

This model simulates a word-addressable OTP region used for secure
fuse storage. The OTP memory can operate with an internal memory
buffer.

The OTP model provides a memory-like interface through a dedicated
AddressSpace, allowing other device models (e.g., SBC) to issue
transactions as if accessing a memory-mapped region.

Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
 hw/misc/aspeed_otpmem.c         | 85 +++++++++++++++++++++++++++++++++
 hw/misc/meson.build             |  1 +
 include/hw/misc/aspeed_otpmem.h | 33 +++++++++++++
 3 files changed, 119 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..b13b87fae8
--- /dev/null
+++ b/hw/misc/aspeed_otpmem.c
@@ -0,0 +1,85 @@
+/*
+ *  ASPEED OTP (One-Time Programmable) memory
+ *
+ *  Copyright (C) 2025 Aspeed
+ *
+ *  SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "trace.h"
+#include "system/block-backend-global-state.h"
+#include "system/block-backend-io.h"
+#include "hw/misc/aspeed_otpmem.h"
+
+static uint64_t aspeed_otpmem_read(void *opaque, hwaddr offset, unsigned size)
+{
+    AspeedOTPMemState *s = opaque;
+    uint64_t val = 0;
+
+    memcpy(&val, s->storage + offset, size);
+
+    return val;
+}
+
+static void aspeed_otpmem_write(void *opaque, hwaddr offset,
+                                uint64_t val, unsigned size)
+{
+    AspeedOTPMemState *s = opaque;
+
+    memcpy(s->storage + offset, &val, size);
+}
+
+static const MemoryRegionOps aspeed_otpmem_ops = {
+    .read = aspeed_otpmem_read,
+    .write = aspeed_otpmem_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+};
+
+static void aspeed_otpmem_realize(DeviceState *dev, Error **errp)
+{
+    AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
+    const size_t size = OTPMEM_SIZE;
+    int i, num;
+    uint32_t *p;
+
+    s->storage = g_malloc(size);
+    if (!s->storage) {
+        error_setg(errp, "Failed to allocate OTP memory storage buffer");
+        return;
+    }
+
+    num = size / sizeof(uint32_t);
+    p = (uint32_t *)s->storage;
+    for (i = 0; i < num; i++) {
+        p[i] = (i % 2 == 0) ? 0x00000000 : 0xFFFFFFFF;
+    }
+
+    memory_region_init_io(&s->mmio, OBJECT(dev), &aspeed_otpmem_ops,
+                          s, "aspeed.otpmem", size);
+    address_space_init(&s->as, &s->mmio, NULL);
+}
+
+static void aspeed_otpmem_class_init(ObjectClass *klass, const void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    dc->realize = aspeed_otpmem_realize;
+}
+
+static const TypeInfo aspeed_otpmem_info = {
+    .name          = TYPE_ASPEED_OTPMEM,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(AspeedOTPMemState),
+    .class_init    = aspeed_otpmem_class_init,
+};
+
+static void aspeed_otpmem_register_types(void)
+{
+    type_register_static(&aspeed_otpmem_info);
+}
+
+type_init(aspeed_otpmem_register_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..a598e707a9
--- /dev/null
+++ b/include/hw/misc/aspeed_otpmem.h
@@ -0,0 +1,33 @@
+/*
+ *  ASPEED OTP (One-Time Programmable) memory
+ *
+ *  Copyright (C) 2025 Aspeed
+ *
+ *  SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef ASPEED_OTPMMEM_H
+#define ASPEED_OTPMMEM_H
+
+#include "system/memory.h"
+#include "hw/block/block.h"
+#include "system/memory.h"
+#include "system/address-spaces.h"
+
+#define OTPMEM_SIZE 0x4000
+#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedOTPMemState, ASPEED_OTPMEM)
+
+typedef struct AspeedOTPMemState {
+    DeviceState parent_obj;
+
+    uint64_t size;
+
+    AddressSpace as;
+
+    MemoryRegion mmio;
+
+    uint8_t *storage;
+} AspeedOTPMemState;
+
+#endif /* ASPEED_OTPMMEM_H */
-- 
2.43.0

On Tue, 24 Jun 2025 at 03:22, Kane Chen <kane_chen@aspeedtech.com> wrote:
>
> From: Kane-Chen-AS <kane_chen@aspeedtech.com>
>
> Introduce a QEMU device model for ASPEED's One-Time Programmable (OTP)
> memory.
>
> This model simulates a word-addressable OTP region used for secure
> fuse storage. The OTP memory can operate with an internal memory
> buffer.
>
> The OTP model provides a memory-like interface through a dedicated
> AddressSpace, allowing other device models (e.g., SBC) to issue
> transactions as if accessing a memory-mapped region.
>
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
>  hw/misc/aspeed_otpmem.c         | 85 +++++++++++++++++++++++++++++++++
>  hw/misc/meson.build             |  1 +
>  include/hw/misc/aspeed_otpmem.h | 33 +++++++++++++

Our other otp-type devices are in hw/nvram/.

You should probably look at whether there's any kind of
already existing conventions for these types of devices
that this one should follow. (There may be nothing
appropriate, but it's worth a look, so we don't end up
with half a dozen devices that are all fairly similar
but all do things in arbitrarily different ways.)

thanks
-- PMM

Hi Peter,

Thanks for your suggestion.

Yes, I did review the existing devices under `hw/nvram/`, including `bcm2835_otp.c`, `npcm7xx_otp.c`, and others. However, I encountered two main challenges:

1. Lack of file backend support
2. Inability to support one-time programming behavior from 1 -> 0 as well as 0 -> 1

For point 1, it's manageable — some existing devices do support a file backend, or we could introduce a middleware layer to add this functionality if needed.

Point 2 is a limitation of the existing mode. Most existing implementations are designed for OTP models that only support 0 -> 1 transitions. Supporting 1 -> 0 one-time programming would require significant modification. Even with a middleware layer, the logic would become quite complex, and the changes might be more intrusive than introducing a new device model.

That’s why I decided to implement a new model specifically for the Aspeed OTP device. That said, I did refer to the designs in `hw/nvram/` during development, and Cédric also pointed me to `pnv_pnor.c` and `m25p80.c`, which provided helpful reference points.

Please let me know if you have any concerns or further suggestions.

Best Regards,
Kane
> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Wednesday, June 25, 2025 6:26 PM
> To: Kane Chen <kane_chen@aspeedtech.com>
> Cc: Cédric Le Goater <clg@kaod.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>; Troy Lee <troy_lee@aspeedtech.com>
> Subject: Re: [RFC v6 1/3] hw/misc/aspeed_otp: Add ASPEED OTP memory
> device model
> 
> On Tue, 24 Jun 2025 at 03:22, Kane Chen <kane_chen@aspeedtech.com>
> wrote:
> >
> > From: Kane-Chen-AS <kane_chen@aspeedtech.com>
> >
> > Introduce a QEMU device model for ASPEED's One-Time Programmable
> (OTP)
> > memory.
> >
> > This model simulates a word-addressable OTP region used for secure
> > fuse storage. The OTP memory can operate with an internal memory
> > buffer.
> >
> > The OTP model provides a memory-like interface through a dedicated
> > AddressSpace, allowing other device models (e.g., SBC) to issue
> > transactions as if accessing a memory-mapped region.
> >
> > Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> > ---
> >  hw/misc/aspeed_otpmem.c         | 85
> +++++++++++++++++++++++++++++++++
> >  hw/misc/meson.build             |  1 +
> >  include/hw/misc/aspeed_otpmem.h | 33 +++++++++++++
> 
> Our other otp-type devices are in hw/nvram/.
> 
> You should probably look at whether there's any kind of already existing
> conventions for these types of devices that this one should follow. (There may
> be nothing appropriate, but it's worth a look, so we don't end up with half a
> dozen devices that are all fairly similar but all do things in arbitrarily different
> ways.)
> 
> thanks
> -- PMM

On 6/24/25 04:22, Kane Chen wrote:
> From: Kane-Chen-AS <kane_chen@aspeedtech.com>
> 
> Introduce a QEMU device model for ASPEED's One-Time Programmable (OTP)
> memory.
> 
> This model simulates a word-addressable OTP region used for secure
> fuse storage. The OTP memory can operate with an internal memory
> buffer.
> 
> The OTP model provides a memory-like interface through a dedicated
> AddressSpace, allowing other device models (e.g., SBC) to issue
> transactions as if accessing a memory-mapped region.
> 
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
>   hw/misc/aspeed_otpmem.c         | 85 +++++++++++++++++++++++++++++++++
>   hw/misc/meson.build             |  1 +
>   include/hw/misc/aspeed_otpmem.h | 33 +++++++++++++
>   3 files changed, 119 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..b13b87fae8
> --- /dev/null
> +++ b/hw/misc/aspeed_otpmem.c
> @@ -0,0 +1,85 @@
> +/*
> + *  ASPEED OTP (One-Time Programmable) memory
> + *
> + *  Copyright (C) 2025 Aspeed
> + *
> + *  SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "trace.h"
> +#include "system/block-backend-global-state.h"
> +#include "system/block-backend-io.h"
> +#include "hw/misc/aspeed_otpmem.h"
> +
> +static uint64_t aspeed_otpmem_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    AspeedOTPMemState *s = opaque;
> +    uint64_t val = 0;
> +
> +    memcpy(&val, s->storage + offset, size);
> +
> +    return val;
> +}
> +
> +static void aspeed_otpmem_write(void *opaque, hwaddr offset,
> +                                uint64_t val, unsigned size)
> +{
> +    AspeedOTPMemState *s = opaque;
> +
> +    memcpy(s->storage + offset, &val, size);
> +}
> +
> +static const MemoryRegionOps aspeed_otpmem_ops = {
> +    .read = aspeed_otpmem_read,
> +    .write = aspeed_otpmem_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +};
> +
> +static void aspeed_otpmem_realize(DeviceState *dev, Error **errp)
> +{
> +    AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
> +    const size_t size = OTPMEM_SIZE;

Why not use s->size instead ? and a device property ?

> +    int i, num;
> +    uint32_t *p;
> +
> +    s->storage = g_malloc(size);
> +    if (!s->storage) {

if g_malloc() fails, the application will terminate. There is no
need to test the returned pointer.

> +        error_setg(errp, "Failed to allocate OTP memory storage buffer");
> +        return;
> +    }
> +
> +    num = size / sizeof(uint32_t);
> +    p = (uint32_t *)s->storage;
> +    for (i = 0; i < num; i++) {
> +        p[i] = (i % 2 == 0) ? 0x00000000 : 0xFFFFFFFF;
> +    }

The above initialization could be done in a aspeed_otpmem_init_storage()routine.

I understand that you want the values set at runtime to be kept
after a machine/device reset.

> +    memory_region_init_io(&s->mmio, OBJECT(dev), &aspeed_otpmem_ops,
> +                          s, "aspeed.otpmem", size);
> +    address_space_init(&s->as, &s->mmio, NULL);
> +}
> +
> +static void aspeed_otpmem_class_init(ObjectClass *klass, const void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    dc->realize = aspeed_otpmem_realize;
> +}
> +
> +static const TypeInfo aspeed_otpmem_info = {
> +    .name          = TYPE_ASPEED_OTPMEM,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(AspeedOTPMemState),
> +    .class_init    = aspeed_otpmem_class_init,
> +};
> +
> +static void aspeed_otpmem_register_types(void)
> +{
> +    type_register_static(&aspeed_otpmem_info);
> +}
> +
> +type_init(aspeed_otpmem_register_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..a598e707a9
> --- /dev/null
> +++ b/include/hw/misc/aspeed_otpmem.h

Please add to your .git/config file :

[diff]
	orderFile = /path/to/qemu/scripts/git.orderfile


Thanks,

C.



> @@ -0,0 +1,33 @@
> +/*
> + *  ASPEED OTP (One-Time Programmable) memory
> + *
> + *  Copyright (C) 2025 Aspeed
> + *
> + *  SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef ASPEED_OTPMMEM_H
> +#define ASPEED_OTPMMEM_H
> +
> +#include "system/memory.h"
> +#include "hw/block/block.h"
> +#include "system/memory.h"
> +#include "system/address-spaces.h"
> +
> +#define OTPMEM_SIZE 0x4000
> +#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedOTPMemState, ASPEED_OTPMEM)
> +
> +typedef struct AspeedOTPMemState {
> +    DeviceState parent_obj;
> +
> +    uint64_t size;
> +
> +    AddressSpace as;
> +
> +    MemoryRegion mmio;
> +
> +    uint8_t *storage;
> +} AspeedOTPMemState;
> +
> +#endif /* ASPEED_OTPMMEM_H */

Hi Cédric,

Thanks for your review and helpful comments. I will update the code accordingly.

I also have a question regarding the OTP memory content initialization. Since the OTP memory only needs to be initialized once, I plan to move the initialization code into the reset function and use a flag to ensure it runs only once. This way, the memory contents will be retained across device or machine resets.

Please let me know if you have any concerns or suggestions about this approach.

Best Regards,
Kane
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Tuesday, June 24, 2025 2:28 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: [RFC v6 1/3] hw/misc/aspeed_otp: Add ASPEED OTP memory
> device model
> 
> On 6/24/25 04:22, Kane Chen wrote:
> > From: Kane-Chen-AS <kane_chen@aspeedtech.com>
> >
> > Introduce a QEMU device model for ASPEED's One-Time Programmable
> (OTP)
> > memory.
> >
> > This model simulates a word-addressable OTP region used for secure
> > fuse storage. The OTP memory can operate with an internal memory
> > buffer.
> >
> > The OTP model provides a memory-like interface through a dedicated
> > AddressSpace, allowing other device models (e.g., SBC) to issue
> > transactions as if accessing a memory-mapped region.
> >
> > Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> > ---
> >   hw/misc/aspeed_otpmem.c         | 85
> +++++++++++++++++++++++++++++++++
> >   hw/misc/meson.build             |  1 +
> >   include/hw/misc/aspeed_otpmem.h | 33 +++++++++++++
> >   3 files changed, 119 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..b13b87fae8
> > --- /dev/null
> > +++ b/hw/misc/aspeed_otpmem.c
> > @@ -0,0 +1,85 @@
> > +/*
> > + *  ASPEED OTP (One-Time Programmable) memory
> > + *
> > + *  Copyright (C) 2025 Aspeed
> > + *
> > + *  SPDX-License-Identifier: GPL-2.0-or-later  */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/log.h"
> > +#include "qapi/error.h"
> > +#include "trace.h"
> > +#include "system/block-backend-global-state.h"
> > +#include "system/block-backend-io.h"
> > +#include "hw/misc/aspeed_otpmem.h"
> > +
> > +static uint64_t aspeed_otpmem_read(void *opaque, hwaddr offset,
> > +unsigned size) {
> > +    AspeedOTPMemState *s = opaque;
> > +    uint64_t val = 0;
> > +
> > +    memcpy(&val, s->storage + offset, size);
> > +
> > +    return val;
> > +}
> > +
> > +static void aspeed_otpmem_write(void *opaque, hwaddr offset,
> > +                                uint64_t val, unsigned size) {
> > +    AspeedOTPMemState *s = opaque;
> > +
> > +    memcpy(s->storage + offset, &val, size); }
> > +
> > +static const MemoryRegionOps aspeed_otpmem_ops = {
> > +    .read = aspeed_otpmem_read,
> > +    .write = aspeed_otpmem_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .valid.min_access_size = 1,
> > +    .valid.max_access_size = 4,
> > +};
> > +
> > +static void aspeed_otpmem_realize(DeviceState *dev, Error **errp) {
> > +    AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
> > +    const size_t size = OTPMEM_SIZE;
> 
> Why not use s->size instead ? and a device property ?
> 
> > +    int i, num;
> > +    uint32_t *p;
> > +
> > +    s->storage = g_malloc(size);
> > +    if (!s->storage) {
> 
> if g_malloc() fails, the application will terminate. There is no need to test the
> returned pointer.
> 
> > +        error_setg(errp, "Failed to allocate OTP memory storage buffer");
> > +        return;
> > +    }
> > +
> > +    num = size / sizeof(uint32_t);
> > +    p = (uint32_t *)s->storage;
> > +    for (i = 0; i < num; i++) {
> > +        p[i] = (i % 2 == 0) ? 0x00000000 : 0xFFFFFFFF;
> > +    }
> 
> The above initialization could be done in a
> aspeed_otpmem_init_storage()routine.
> 
> I understand that you want the values set at runtime to be kept after a
> machine/device reset.
> 
> > +    memory_region_init_io(&s->mmio, OBJECT(dev),
> &aspeed_otpmem_ops,
> > +                          s, "aspeed.otpmem", size);
> > +    address_space_init(&s->as, &s->mmio, NULL); }
> > +
> > +static void aspeed_otpmem_class_init(ObjectClass *klass, const void
> > +*data) {
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    dc->realize = aspeed_otpmem_realize; }
> > +
> > +static const TypeInfo aspeed_otpmem_info = {
> > +    .name          = TYPE_ASPEED_OTPMEM,
> > +    .parent        = TYPE_DEVICE,
> > +    .instance_size = sizeof(AspeedOTPMemState),
> > +    .class_init    = aspeed_otpmem_class_init,
> > +};
> > +
> > +static void aspeed_otpmem_register_types(void)
> > +{
> > +    type_register_static(&aspeed_otpmem_info);
> > +}
> > +
> > +type_init(aspeed_otpmem_register_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..a598e707a9
> > --- /dev/null
> > +++ b/include/hw/misc/aspeed_otpmem.h
> 
> Please add to your .git/config file :
> 
> [diff]
> 	orderFile = /path/to/qemu/scripts/git.orderfile
> 
> 
> Thanks,
> 
> C.
> 
> 
> 
> > @@ -0,0 +1,33 @@
> > +/*
> > + *  ASPEED OTP (One-Time Programmable) memory
> > + *
> > + *  Copyright (C) 2025 Aspeed
> > + *
> > + *  SPDX-License-Identifier: GPL-2.0-or-later  */
> > +
> > +#ifndef ASPEED_OTPMMEM_H
> > +#define ASPEED_OTPMMEM_H
> > +
> > +#include "system/memory.h"
> > +#include "hw/block/block.h"
> > +#include "system/memory.h"
> > +#include "system/address-spaces.h"
> > +
> > +#define OTPMEM_SIZE 0x4000
> > +#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
> > +OBJECT_DECLARE_SIMPLE_TYPE(AspeedOTPMemState,
> ASPEED_OTPMEM)
> > +
> > +typedef struct AspeedOTPMemState {
> > +    DeviceState parent_obj;
> > +
> > +    uint64_t size;
> > +
> > +    AddressSpace as;
> > +
> > +    MemoryRegion mmio;
> > +
> > +    uint8_t *storage;
> > +} AspeedOTPMemState;
> > +
> > +#endif /* ASPEED_OTPMMEM_H */

Hello Kane,

On 6/25/25 07:59, Kane Chen wrote:
> Hi Cédric,
> 
> Thanks for your review and helpful comments. I will update the code accordingly.
> 
> I also have a question regarding the OTP memory content initialization. Since the OTP memory only needs to be initialized once, I plan to move the initialization code into the reset function 

It's not a reset function since the contents persist across resets.

> and use a flag to ensure it runs only once. This way, the memory contents will be retained across device or machine resets.

I would keep the contents initialization in the realize routine,
same as m25p80_realize() does :

         s->storage = blk_blockalign(NULL, s->size);
         memset(s->storage, 0xFF, s->size);

Thanks,

C.



> 
> Please let me know if you have any concerns or suggestions about this approach.
> 
> Best Regards,
> Kane
>> -----Original Message-----
>> From: Cédric Le Goater <clg@kaod.org>
>> Sent: Tuesday, June 24, 2025 2:28 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: [RFC v6 1/3] hw/misc/aspeed_otp: Add ASPEED OTP memory
>> device model
>>
>> On 6/24/25 04:22, Kane Chen wrote:
>>> From: Kane-Chen-AS <kane_chen@aspeedtech.com>
>>>
>>> Introduce a QEMU device model for ASPEED's One-Time Programmable
>> (OTP)
>>> memory.
>>>
>>> This model simulates a word-addressable OTP region used for secure
>>> fuse storage. The OTP memory can operate with an internal memory
>>> buffer.
>>>
>>> The OTP model provides a memory-like interface through a dedicated
>>> AddressSpace, allowing other device models (e.g., SBC) to issue
>>> transactions as if accessing a memory-mapped region.
>>>
>>> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
>>> ---
>>>    hw/misc/aspeed_otpmem.c         | 85
>> +++++++++++++++++++++++++++++++++
>>>    hw/misc/meson.build             |  1 +
>>>    include/hw/misc/aspeed_otpmem.h | 33 +++++++++++++
>>>    3 files changed, 119 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..b13b87fae8
>>> --- /dev/null
>>> +++ b/hw/misc/aspeed_otpmem.c
>>> @@ -0,0 +1,85 @@
>>> +/*
>>> + *  ASPEED OTP (One-Time Programmable) memory
>>> + *
>>> + *  Copyright (C) 2025 Aspeed
>>> + *
>>> + *  SPDX-License-Identifier: GPL-2.0-or-later  */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/log.h"
>>> +#include "qapi/error.h"
>>> +#include "trace.h"
>>> +#include "system/block-backend-global-state.h"
>>> +#include "system/block-backend-io.h"
>>> +#include "hw/misc/aspeed_otpmem.h"
>>> +
>>> +static uint64_t aspeed_otpmem_read(void *opaque, hwaddr offset,
>>> +unsigned size) {
>>> +    AspeedOTPMemState *s = opaque;
>>> +    uint64_t val = 0;
>>> +
>>> +    memcpy(&val, s->storage + offset, size);
>>> +
>>> +    return val;
>>> +}
>>> +
>>> +static void aspeed_otpmem_write(void *opaque, hwaddr offset,
>>> +                                uint64_t val, unsigned size) {
>>> +    AspeedOTPMemState *s = opaque;
>>> +
>>> +    memcpy(s->storage + offset, &val, size); }
>>> +
>>> +static const MemoryRegionOps aspeed_otpmem_ops = {
>>> +    .read = aspeed_otpmem_read,
>>> +    .write = aspeed_otpmem_write,
>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>> +    .valid.min_access_size = 1,
>>> +    .valid.max_access_size = 4,
>>> +};
>>> +
>>> +static void aspeed_otpmem_realize(DeviceState *dev, Error **errp) {
>>> +    AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
>>> +    const size_t size = OTPMEM_SIZE;
>>
>> Why not use s->size instead ? and a device property ?
>>
>>> +    int i, num;
>>> +    uint32_t *p;
>>> +
>>> +    s->storage = g_malloc(size);
>>> +    if (!s->storage) {
>>
>> if g_malloc() fails, the application will terminate. There is no need to test the
>> returned pointer.
>>
>>> +        error_setg(errp, "Failed to allocate OTP memory storage buffer");
>>> +        return;
>>> +    }
>>> +
>>> +    num = size / sizeof(uint32_t);
>>> +    p = (uint32_t *)s->storage;
>>> +    for (i = 0; i < num; i++) {
>>> +        p[i] = (i % 2 == 0) ? 0x00000000 : 0xFFFFFFFF;
>>> +    }
>>
>> The above initialization could be done in a
>> aspeed_otpmem_init_storage()routine.
>>
>> I understand that you want the values set at runtime to be kept after a
>> machine/device reset.
>>
>>> +    memory_region_init_io(&s->mmio, OBJECT(dev),
>> &aspeed_otpmem_ops,
>>> +                          s, "aspeed.otpmem", size);
>>> +    address_space_init(&s->as, &s->mmio, NULL); }
>>> +
>>> +static void aspeed_otpmem_class_init(ObjectClass *klass, const void
>>> +*data) {
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    dc->realize = aspeed_otpmem_realize; }
>>> +
>>> +static const TypeInfo aspeed_otpmem_info = {
>>> +    .name          = TYPE_ASPEED_OTPMEM,
>>> +    .parent        = TYPE_DEVICE,
>>> +    .instance_size = sizeof(AspeedOTPMemState),
>>> +    .class_init    = aspeed_otpmem_class_init,
>>> +};
>>> +
>>> +static void aspeed_otpmem_register_types(void)
>>> +{
>>> +    type_register_static(&aspeed_otpmem_info);
>>> +}
>>> +
>>> +type_init(aspeed_otpmem_register_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..a598e707a9
>>> --- /dev/null
>>> +++ b/include/hw/misc/aspeed_otpmem.h
>>
>> Please add to your .git/config file :
>>
>> [diff]
>> 	orderFile = /path/to/qemu/scripts/git.orderfile
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>> @@ -0,0 +1,33 @@
>>> +/*
>>> + *  ASPEED OTP (One-Time Programmable) memory
>>> + *
>>> + *  Copyright (C) 2025 Aspeed
>>> + *
>>> + *  SPDX-License-Identifier: GPL-2.0-or-later  */
>>> +
>>> +#ifndef ASPEED_OTPMMEM_H
>>> +#define ASPEED_OTPMMEM_H
>>> +
>>> +#include "system/memory.h"
>>> +#include "hw/block/block.h"
>>> +#include "system/memory.h"
>>> +#include "system/address-spaces.h"
>>> +
>>> +#define OTPMEM_SIZE 0x4000
>>> +#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedOTPMemState,
>> ASPEED_OTPMEM)
>>> +
>>> +typedef struct AspeedOTPMemState {
>>> +    DeviceState parent_obj;
>>> +
>>> +    uint64_t size;
>>> +
>>> +    AddressSpace as;
>>> +
>>> +    MemoryRegion mmio;
>>> +
>>> +    uint8_t *storage;
>>> +} AspeedOTPMemState;
>>> +
>>> +#endif /* ASPEED_OTPMMEM_H */
> 

Hi Cédric,

Thanks for your comments.

I have one more question regarding the use of the "RFC" tag. Initially, I marked the series as "RFC" to gather feedback on the OTP memory and SBC initialization design. Now that we're moving forward with more concrete code review, I'm wondering whether I should still use the "RFC" tag for the next version of the patch.

Would you recommend keeping the "RFC" tag, or is it appropriate to drop it at this point?

Best Regards,
Kane
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Wednesday, June 25, 2025 2:07 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: [RFC v6 1/3] hw/misc/aspeed_otp: Add ASPEED OTP memory
> device model
> 
> Hello Kane,
> 
> On 6/25/25 07:59, Kane Chen wrote:
> > Hi Cédric,
> >
> > Thanks for your review and helpful comments. I will update the code
> accordingly.
> >
> > I also have a question regarding the OTP memory content
> > initialization. Since the OTP memory only needs to be initialized
> > once, I plan to move the initialization code into the reset function
> 
> It's not a reset function since the contents persist across resets.
> 
> > and use a flag to ensure it runs only once. This way, the memory contents
> will be retained across device or machine resets.
> 
> I would keep the contents initialization in the realize routine, same as
> m25p80_realize() does :
> 
>          s->storage = blk_blockalign(NULL, s->size);
>          memset(s->storage, 0xFF, s->size);
> 
> Thanks,
> 
> C.
> 
> 
> 
> >
> > Please let me know if you have any concerns or suggestions about this
> approach.
> >
> > Best Regards,
> > Kane
> >> -----Original Message-----
> >> From: Cédric Le Goater <clg@kaod.org>
> >> Sent: Tuesday, June 24, 2025 2:28 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: [RFC v6 1/3] hw/misc/aspeed_otp: Add ASPEED OTP memory
> >> device model
> >>
> >> On 6/24/25 04:22, Kane Chen wrote:
> >>> From: Kane-Chen-AS <kane_chen@aspeedtech.com>
> >>>
> >>> Introduce a QEMU device model for ASPEED's One-Time Programmable
> >> (OTP)
> >>> memory.
> >>>
> >>> This model simulates a word-addressable OTP region used for secure
> >>> fuse storage. The OTP memory can operate with an internal memory
> >>> buffer.
> >>>
> >>> The OTP model provides a memory-like interface through a dedicated
> >>> AddressSpace, allowing other device models (e.g., SBC) to issue
> >>> transactions as if accessing a memory-mapped region.
> >>>
> >>> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> >>> ---
> >>>    hw/misc/aspeed_otpmem.c         | 85
> >> +++++++++++++++++++++++++++++++++
> >>>    hw/misc/meson.build             |  1 +
> >>>    include/hw/misc/aspeed_otpmem.h | 33 +++++++++++++
> >>>    3 files changed, 119 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..b13b87fae8
> >>> --- /dev/null
> >>> +++ b/hw/misc/aspeed_otpmem.c
> >>> @@ -0,0 +1,85 @@
> >>> +/*
> >>> + *  ASPEED OTP (One-Time Programmable) memory
> >>> + *
> >>> + *  Copyright (C) 2025 Aspeed
> >>> + *
> >>> + *  SPDX-License-Identifier: GPL-2.0-or-later  */
> >>> +
> >>> +#include "qemu/osdep.h"
> >>> +#include "qemu/log.h"
> >>> +#include "qapi/error.h"
> >>> +#include "trace.h"
> >>> +#include "system/block-backend-global-state.h"
> >>> +#include "system/block-backend-io.h"
> >>> +#include "hw/misc/aspeed_otpmem.h"
> >>> +
> >>> +static uint64_t aspeed_otpmem_read(void *opaque, hwaddr offset,
> >>> +unsigned size) {
> >>> +    AspeedOTPMemState *s = opaque;
> >>> +    uint64_t val = 0;
> >>> +
> >>> +    memcpy(&val, s->storage + offset, size);
> >>> +
> >>> +    return val;
> >>> +}
> >>> +
> >>> +static void aspeed_otpmem_write(void *opaque, hwaddr offset,
> >>> +                                uint64_t val, unsigned size) {
> >>> +    AspeedOTPMemState *s = opaque;
> >>> +
> >>> +    memcpy(s->storage + offset, &val, size); }
> >>> +
> >>> +static const MemoryRegionOps aspeed_otpmem_ops = {
> >>> +    .read = aspeed_otpmem_read,
> >>> +    .write = aspeed_otpmem_write,
> >>> +    .endianness = DEVICE_LITTLE_ENDIAN,
> >>> +    .valid.min_access_size = 1,
> >>> +    .valid.max_access_size = 4,
> >>> +};
> >>> +
> >>> +static void aspeed_otpmem_realize(DeviceState *dev, Error **errp) {
> >>> +    AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
> >>> +    const size_t size = OTPMEM_SIZE;
> >>
> >> Why not use s->size instead ? and a device property ?
> >>
> >>> +    int i, num;
> >>> +    uint32_t *p;
> >>> +
> >>> +    s->storage = g_malloc(size);
> >>> +    if (!s->storage) {
> >>
> >> if g_malloc() fails, the application will terminate. There is no need
> >> to test the returned pointer.
> >>
> >>> +        error_setg(errp, "Failed to allocate OTP memory storage
> buffer");
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    num = size / sizeof(uint32_t);
> >>> +    p = (uint32_t *)s->storage;
> >>> +    for (i = 0; i < num; i++) {
> >>> +        p[i] = (i % 2 == 0) ? 0x00000000 : 0xFFFFFFFF;
> >>> +    }
> >>
> >> The above initialization could be done in a
> >> aspeed_otpmem_init_storage()routine.
> >>
> >> I understand that you want the values set at runtime to be kept after
> >> a machine/device reset.
> >>
> >>> +    memory_region_init_io(&s->mmio, OBJECT(dev),
> >> &aspeed_otpmem_ops,
> >>> +                          s, "aspeed.otpmem", size);
> >>> +    address_space_init(&s->as, &s->mmio, NULL); }
> >>> +
> >>> +static void aspeed_otpmem_class_init(ObjectClass *klass, const void
> >>> +*data) {
> >>> +    DeviceClass *dc = DEVICE_CLASS(klass);
> >>> +    dc->realize = aspeed_otpmem_realize; }
> >>> +
> >>> +static const TypeInfo aspeed_otpmem_info = {
> >>> +    .name          = TYPE_ASPEED_OTPMEM,
> >>> +    .parent        = TYPE_DEVICE,
> >>> +    .instance_size = sizeof(AspeedOTPMemState),
> >>> +    .class_init    = aspeed_otpmem_class_init,
> >>> +};
> >>> +
> >>> +static void aspeed_otpmem_register_types(void)
> >>> +{
> >>> +    type_register_static(&aspeed_otpmem_info);
> >>> +}
> >>> +
> >>> +type_init(aspeed_otpmem_register_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..a598e707a9
> >>> --- /dev/null
> >>> +++ b/include/hw/misc/aspeed_otpmem.h
> >>
> >> Please add to your .git/config file :
> >>
> >> [diff]
> >> 	orderFile = /path/to/qemu/scripts/git.orderfile
> >>
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >>
> >>> @@ -0,0 +1,33 @@
> >>> +/*
> >>> + *  ASPEED OTP (One-Time Programmable) memory
> >>> + *
> >>> + *  Copyright (C) 2025 Aspeed
> >>> + *
> >>> + *  SPDX-License-Identifier: GPL-2.0-or-later  */
> >>> +
> >>> +#ifndef ASPEED_OTPMMEM_H
> >>> +#define ASPEED_OTPMMEM_H
> >>> +
> >>> +#include "system/memory.h"
> >>> +#include "hw/block/block.h"
> >>> +#include "system/memory.h"
> >>> +#include "system/address-spaces.h"
> >>> +
> >>> +#define OTPMEM_SIZE 0x4000
> >>> +#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
> >>> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedOTPMemState,
> >> ASPEED_OTPMEM)
> >>> +
> >>> +typedef struct AspeedOTPMemState {
> >>> +    DeviceState parent_obj;
> >>> +
> >>> +    uint64_t size;
> >>> +
> >>> +    AddressSpace as;
> >>> +
> >>> +    MemoryRegion mmio;
> >>> +
> >>> +    uint8_t *storage;
> >>> +} AspeedOTPMemState;
> >>> +
> >>> +#endif /* ASPEED_OTPMMEM_H */
> >

On 6/25/25 09:05, Kane Chen wrote:
> Hi Cédric,
> 
> Thanks for your comments.
> 
> I have one more question regarding the use of the "RFC" tag. Initially, I marked the series as "RFC" to gather feedback on the OTP memory and SBC initialization design. Now that we're moving forward with more concrete code review, I'm wondering whether I should still use the "RFC" tag for the next version of the patch.
> 
> Would you recommend keeping the "RFC" tag, or is it appropriate to drop it at this point?

It's fine to drop the RFC in the next version. Make it a v1 too.

Thanks,

C.