Hi Cédric,
> -----Original Message-----
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Tuesday, May 27, 2025 4:12 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 v4 2/3] hw/misc/aspeed_sbc: Connect ASPEED OTP
> memory device to SBC controller
>
> On 5/27/25 09:46, Kane Chen wrote:
> > Hi Cédric,
> >
> >> -----Original Message-----
> >> From: Cédric Le Goater <c...@kaod.org>
> >> Sent: Tuesday, May 27, 2025 3:02 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 v4 2/3] hw/misc/aspeed_sbc: Connect ASPEED OTP
> >> memory device to SBC controller
> >>
> >> On 5/12/25 11:10, Kane Chen wrote:
> >>> From: Kane-Chen-AS <kane_c...@aspeedtech.com>
> >>>
> >>> Integrate the aspeed.otpmem backend with the ASPEED Secure Boot
> >>> Controller (SBC).
> >>>
> >>> This patch adds command handling support in the SBC to read and
> >>> program the connected OTP memory using READ, WRITE, and PROG
> >> commands.
> >>> It enables basic interaction with OTP content for secure boot or
> >>> fuse
> >> modeling logic.
> >>>
> >>> Tracepoints are used to monitor command activity and unsupported paths.
> >>>
> >>> The following QOM hierarchy illustrates how OTP is connected:
> >>>
> >>> /machine (ast1030-evb-machine)
> >>> /soc (ast1030-a1)
> >>> /sbc (aspeed.sbc-ast10X0)
> >>> /aspeed.sbc[0] (memory-region)
> >>> /otpmem (aspeed.otpmem)
> >>> /aspeed.otpmem.backend[0] (memory-region)
> >>>
> >>> Signed-off-by: Kane-Chen-AS <kane_c...@aspeedtech.com>
> >>> ---
> >>> hw/misc/aspeed_sbc.c | 179
> >> +++++++++++++++++++++++++++++++++++
> >>> hw/misc/trace-events | 5 +
> >>> include/hw/misc/aspeed_sbc.h | 5 +
> >>> 3 files changed, 189 insertions(+)
> >>>
> >>> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c index
> >>> a7d101ba71..237a8499d9 100644
> >>> --- a/hw/misc/aspeed_sbc.c
> >>> +++ b/hw/misc/aspeed_sbc.c
> >>> @@ -15,9 +15,14 @@
> >>> #include "hw/misc/aspeed_sbc.h"
> >>> #include "qapi/error.h"
> >>> #include "migration/vmstate.h"
> >>> +#include "trace.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 */
> >>> @@ -41,6 +46,19 @@
> >>> #define QSR_RSA_MASK (0x3 << 12)
> >>> #define QSR_HASH_MASK (0x3 << 10)
> >>>
> >>> +typedef enum {
> >>> + SBC_OTP_CMD_READ = 0x23b1e361,
> >>> + SBC_OTP_CMD_WRITE = 0x23b1e362,
> >>> + SBC_OTP_CMD_PROG = 0x23b1e364,
> >>> +} SBC_OTP_Command;
> >>> +
> >>> +#define OTP_DATA_DWORD_COUNT (0x800)
> >>> +#define OTP_TOTAL_DWORD_COUNT (0x1000)
> >>> +
> >>> +#define MODE_REGISTER (0x1000)
> >>> +#define MODE_REGISTER_A (0x3000)
> >>> +#define MODE_REGISTER_B (0x5000)
> >>> +
> >>> static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr,
> >>> unsigned int
> >> size)
> >>> {
> >>> AspeedSBCState *s = ASPEED_SBC(opaque); @@ -57,6 +75,143
> @@
> >>> static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned
> >>> int
> >> size)
> >>> return s->regs[addr];
> >>> }
> >>>
> >>> +static bool aspeed_sbc_otpmem_read(AspeedSBCState *s,
> >>> + uint32_t otp_addr, Error
> **errp)
> >> {
> >>> + uint32_t data = 0, otp_offset;
> >>> + bool is_data = false;
> >>> + AspeedSBCClass *sc = ASPEED_SBC_GET_CLASS(s);
> >>> + const AspeedOTPMemOps *otp_ops;
> >>> +
> >>> + if (sc->has_otpmem == false) {
> >>> + trace_aspeed_sbc_otpmem_state("disabled");
> >>> + return true;
> >>> + }
> >>> +
> >>> + otp_ops = aspeed_otpmem_get_ops(&s->otpmem);
> >>> +
> >>> + if (otp_addr < OTP_DATA_DWORD_COUNT) {
> >>> + is_data = true;
> >>> + } else if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
> >>> + error_setg(errp, "Invalid OTP addr 0x%x", otp_addr);
> >>> + return false;
> >>> + }
> >>> + otp_offset = otp_addr << 2;
> >>> +
> >>> + data = otp_ops->read(&s->otpmem, otp_offset, errp);
> >>
> >>
> >> why not call directly :
> >>
> >> ret = address_space_read(&s->otpmem.as, addr,
> >> MEMTXATTRS_UNSPECIFIED,
> >> (uint8_t *)&val, sizeof(val));
> >> if (ret != MEMTX_OK) {
> >> error_setg(errp, "Failed to read data from 0x%x", addr);
> >> return OTPMEM_ERR_MAGIC;
> >> }
> >>
> >> The otp_ops business looks useless to me. Same for the write and prog
> >> handlers.
> >
> > The OTP memory has some regions that are protected from guest access,
> > so I use a custom read function to handle that behavior.
>
> Why not fail the transaction on the address_space then ?
>
> > The write path
> > also has some special handling (see aspeed_otpmem_prog in patch 1),
> > which is why I'm using AspeedOTPMemOps for both. If you have concerns
> > or comments about this implementation, please let me know.
> If you want to keep routine program_otpmem_data at a low level, under the
> AspeedOTPMem model, it could be implemented using the drive backend
> directly instead of doing address_space transactions.
> You could also move all the code from aspeed_otpmem_prog() code in
> aspeed_sbc_otpmem_prog(). As you wish.
>
> I don't see any reason to keep AspeedOTPMemOps.
>
Got it. I think I misunderstood how address_space works. You're referring
to something like pnv_pnor_ops, right? I'll update the code and remove
AspeedOTPMemOps.
Thanks for the suggestion!
> Thanks,
>
> C.
Best regards,
Kane
