Hi Cédric,
> -----Original Message-----
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Monday, April 28, 2025 3:06 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: [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_c...@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_c...@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
