Hi Cédric,
> -----Original Message-----
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Monday, April 28, 2025 3:21 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 2/3] hw/misc/aspeed_sbc: Connect Aspeed OTP
> memory device to SBC controller
>
> On 4/23/25 04:56, Kane Chen wrote:
> > From: Kane-Chen-AS <kane_c...@aspeedtech.com>
> >
> > This patch integrates the `aspeed.otpmem` device with the ASPEED
> > Secure Boot Controller (SBC). The SBC now accepts an OTP backend via a
> > QOM link property ("otpmem"), enabling internal access to OTP content
> > for controller-specific logic.
> >
> > This connection provides the foundation for future enhancements
> > involving fuse storage, device configuration, or secure manufacturing
> > data provisioning.
> >
> > Signed-off-by: Kane-Chen-AS <kane_c...@aspeedtech.com>
> > ---
> > hw/misc/aspeed_sbc.c | 146
> +++++++++++++++++++++++++++++++++++
> > include/hw/misc/aspeed_sbc.h | 15 ++++
> > 2 files changed, 161 insertions(+)
> >
> > diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c index
> > e4a6bd1581..f0ce7bbdf0 100644
> > --- a/hw/misc/aspeed_sbc.c
> > +++ b/hw/misc/aspeed_sbc.c
> > @@ -17,7 +17,11 @@
> > #include "migration/vmstate.h"
> >
> > #define R_PROT (0x000 / 4)
> > +#define R_CMD (0x004 / 4)
> > +#define R_ADDR (0x010 / 4)
> > #define R_STATUS (0x014 / 4)
> > +#define R_CAMP1 (0x020 / 4)
> > +#define R_CAMP2 (0x024 / 4)
> > #define R_QSR (0x040 / 4)
> >
> > /* R_STATUS */
> > @@ -57,6 +61,143 @@ static uint64_t aspeed_sbc_read(void *opaque,
> hwaddr addr, unsigned int size)
> > return s->regs[addr];
> > }
> >
> > +static void aspeed_sbc_otpmem_read(void *opaque)
>
> You could improve this prototype. How about :
>
> bool aspeed_sbc_otpmem_read(AspeedSBCState *s, uint32_t otp_addr, Error
> *errp)
>
> same below.
Sure. I will update the function prototype in the next patch, as you suggested.
>
> > +{
> > + AspeedSBCState *s = ASPEED_SBC(opaque);
> > + uint32_t otp_addr, data, otp_offset;
> > + bool is_data = false;
> > + Error *local_err = NULL;
> > +
> > + assert(s->otpmem);
> > +
> > + otp_addr = s->regs[R_ADDR];
> > + if (otp_addr < OTP_DATA_DWORD_COUNT) {
> > + is_data = true;
> > + } else if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Invalid OTP addr 0x%x\n",
> > + __func__, otp_addr);
> > + return;
> > + }
> > + otp_offset = otp_addr << 2;
> > +
> > + s->otpmem->ops->read(s->otpmem, otp_offset, &data, &local_err);
> > + if (local_err) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Failed to read data 0x%x, %s\n",
> > + __func__, otp_offset,
> > + error_get_pretty(local_err));
> > + error_free(local_err);
> > + return;> + }
>
> Please use an AddressSpace. See aspeed_smc for an example.
>
I will rework the code to use an AddressSpace for OTP memory access.
Thanks for pointing me to the aspeed_smc example — it’s very helpful.
> > + s->regs[R_CAMP1] = data;
> > +
> > + if (is_data) {
> > + s->otpmem->ops->read(s->otpmem, otp_offset + 4, &data,
> &local_err);
> > + if (local_err) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Failed to read data 0x%x, %s\n",
> > + __func__, otp_offset,
> > + error_get_pretty(local_err));
> > + error_free(local_err);
> > + return;
> > + }
> > + s->regs[R_CAMP2] = data;
> > + }
> > +}
> > +
> > +static void mr_handler(uint32_t otp_addr, uint32_t data)
>
> data is unused
I will remove it in the next patch.
>
> > +{
> > + switch (otp_addr) {
> > + case MODE_REGISTER:
> > + case MODE_REGISTER_A:
> > + case MODE_REGISTER_B:
> > + /* HW behavior, do nothing here */
> > + break;
> > + default:
> > + qemu_log_mask(LOG_GUEST_ERROR,
>
> alignment issue.
>
I will adjust it in the next patch.
> > + "%s: Unsupported address 0x%x\n",
> > + __func__, otp_addr);
> > + return;
> > + }
> > +}
> > +
> > +static void aspeed_sbc_otpmem_write(void *opaque) {
> > + AspeedSBCState *s = ASPEED_SBC(opaque);
> > + uint32_t otp_addr, data;
> > +
> > + otp_addr = s->regs[R_ADDR];
> > + data = s->regs[R_CAMP1];
> > +
> > + if (otp_addr == 0) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: ignore write program bit request\n",
> > + __func__);
> > + } else if (otp_addr >= MODE_REGISTER) {
> > + mr_handler(otp_addr, data);
> > + } else {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Unhandled OTP write address 0x%x\n",
> > + __func__, otp_addr);
> > + }
> > +}
> > +
> > +static void aspeed_sbc_otpmem_prog(void *opaque) {
> > + AspeedSBCState *s = ASPEED_SBC(opaque);
> > + uint32_t otp_addr, value;
> > + Error *local_err = NULL;
> > +
> > + assert(s->otpmem);
> > +
> > + otp_addr = s->regs[R_ADDR];
> > + value = s->regs[R_CAMP1];
> > + if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Invalid OTP addr 0x%x\n",
> > + __func__, otp_addr);
> > + return;
> > + }
> > +
> > + s->otpmem->ops->prog(s->otpmem, otp_addr, value, &local_err);
> > + if (local_err) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Failed to program data 0x%x to 0x%x,
> %s\n",
> > + __func__, value, otp_addr,
> > + error_get_pretty(local_err));
> > + error_free(local_err);
> > + return;
> > + }
> > +}
> > +
> > +static void aspeed_sbc_handle_command(void *opaque, uint32_t cmd) {
> > + AspeedSBCState *s = ASPEED_SBC(opaque);
> > +
> > + s->regs[R_STATUS] &= ~(OTP_MEM_IDLE | OTP_IDLE);
> > +
> > + switch (cmd) {
> > + case READ_CMD:
> > + aspeed_sbc_otpmem_read(s);
> > + break;
> > + case WRITE_CMD:
> > + aspeed_sbc_otpmem_write(s);
> > + break;
> > + case PROG_CMD:
> > + aspeed_sbc_otpmem_prog(s);
> > + break;
> > + default:
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Unknown command 0x%x\n",
> > + __func__, cmd);
> > + break;
> > + }
> > +
> > + s->regs[R_STATUS] |= (OTP_MEM_IDLE | OTP_IDLE); }
> > +
> > +
> > static void aspeed_sbc_write(void *opaque, hwaddr addr, uint64_t data,
> > unsigned int size)
> > {
> > @@ -78,6 +219,9 @@ static void aspeed_sbc_write(void *opaque, hwaddr
> addr, uint64_t data,
> > "%s: write to read only register 0x%"
> HWADDR_PRIx "\n",
> > __func__, addr << 2);
> > return;
> > + case R_CMD:
> > + aspeed_sbc_handle_command(opaque, data);
> > + return;
> > default:
> > break;
> > }
> > @@ -139,6 +283,8 @@ static const VMStateDescription
> vmstate_aspeed_sbc = {
> > static const Property aspeed_sbc_properties[] = {
> > DEFINE_PROP_BOOL("emmc-abr", AspeedSBCState, emmc_abr, 0),
> > DEFINE_PROP_UINT32("signing-settings", AspeedSBCState,
> > signing_settings, 0),
> > + DEFINE_PROP_LINK("otpmem", AspeedSBCState, otpmem,
> > + TYPE_ASPEED_OTPMEM, AspeedOTPMemState
> *),
>
> hmm, no.
>
> Instead AspeedOTPMemState should be a child object of AspeedSBCState, only
> created and realized for ast2600/1030 Soc. This means you will probably
> need a class attribute in AspeedSBCClass.
I will remove the DEFINE_PROP_LINK("otpmem", ...) approach and instead
manage AspeedOTPMemState as a child object of AspeedSBCState.
>
>
>
> > };
> >
> > static void aspeed_sbc_class_init(ObjectClass *klass, void *data)
> > diff --git a/include/hw/misc/aspeed_sbc.h
> > b/include/hw/misc/aspeed_sbc.h index 405e6782b9..8ae59d977e 100644
> > --- a/include/hw/misc/aspeed_sbc.h
> > +++ b/include/hw/misc/aspeed_sbc.h
> > @@ -10,6 +10,7 @@
> > #define ASPEED_SBC_H
> >
> > #include "hw/sysbus.h"
> > +#include "hw/misc/aspeed_otpmem.h"
> >
> > #define TYPE_ASPEED_SBC "aspeed.sbc"
> > #define TYPE_ASPEED_AST2600_SBC TYPE_ASPEED_SBC "-ast2600"
> > @@ -27,6 +28,18 @@ OBJECT_DECLARE_TYPE(AspeedSBCState,
> AspeedSBCClass, ASPEED_SBC)
> > #define QSR_SHA384 (0x2 << 10)
> > #define QSR_SHA512 (0x3 << 10)
> >
> > +#define READ_CMD (0x23b1e361)
> > +#define WRITE_CMD (0x23b1e362)
> > +#define PROG_CMD (0x23b1e364)
> > +
> > +#define OTP_DATA_DWORD_COUNT (0x800)
> > +#define OTP_TOTAL_DWORD_COUNT (0x1000)
> > +#define OTP_FILE_SIZE (OTP_TOTAL_DWORD_COUNT *
> sizeof(uint32_t))
> > +
> > +#define MODE_REGISTER (0x1000)
> > +#define MODE_REGISTER_A (0x3000)
> > +#define MODE_REGISTER_B (0x5000)
> > +
>
> These define belong to the implementation : aspeed_sbc.c.
>
I will move them to aspeed_sbc.c.
>
> Thanks,
>
> C.
>
>
> > struct AspeedSBCState {
> > SysBusDevice parent;
> >
> > @@ -36,6 +49,8 @@ struct AspeedSBCState {
> > MemoryRegion iomem;
> >
> > uint32_t regs[ASPEED_SBC_NR_REGS];
> > +
> > + AspeedOTPMemState *otpmem;
> > };
> >
> > struct AspeedSBCClass {
Thanks again for your comments and review.
Best Regards,
Kane
