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 <c...@kaod.org>
> Sent: Tuesday, June 24, 2025 2:28 PM
> To: Kane Chen <kane_c...@aspeedtech.com>; Peter Maydell
> <peter.mayd...@linaro.org>; Steven Lee <steven_...@aspeedtech.com>; Troy
> Lee <leet...@gmail.com>; Jamin Lin <jamin_...@aspeedtech.com>; Andrew
> Jeffery <and...@codeconstruct.com.au>; Joel Stanley <j...@jms.id.au>;
> open list:ASPEED BMCs <qemu-...@nongnu.org>; open list:All patches CC
> here <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_...@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_c...@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_c...@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 */
