Hi Cédric,
> -----Original Message-----
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Monday, April 28, 2025 3:41 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 3/3] hw/arm: Integrate Aspeed OTP memory into
> AST10x0 and AST2600 SoCs
>
> On 4/23/25 04:56, Kane Chen wrote:
> > From: Kane-Chen-AS <kane_c...@aspeedtech.com>
> >
> > This patch wires up the OTP memory device (`aspeed.otpmem`) into the
> > AST1030 and AST2600 SoC models. The device is initialized, attached to
> > a backing block drive (`-drive id=otpmem`) and linked to the SBC
> > controller via a QOM link.
> >
> > The default OTP memory image can be generated using the following
> > command.
> > ```bash
> > for i in $(seq 1 2048); do
> > printf '\x00\x00\x00\x00\xff\xff\xff\xff'
> > done > otpmem.img
> > ```
> >
> > To load the OTP memory image into the guest, use:
> > ```bash
> > ./qemu-system-arm \
> > -drive id=otpmem,file=otpmem.img,if=none,format=raw \
> > ...
> > ```
>
> I thought we were going to implement the same method of the edk2 flash
> devices of the q35 machine. Setting a machine option would set the drive :
>
> qemu-system-arm -M ast2600-evb,otpmem=otpmem-drive \
> -blockdev
> node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
> ...
>
> Which is not what is proposed below.
>
I understand that using a machine option (e.g., -M ast2600-evb,otpmem=xxx)
to specify the OTP memory drive is similar to the modeling used for
flash devices in the Q35 machine. However, in the real ASPEED hardware,
the OTP memory is physically part of the Secure Boot Controller (SBC)
and is not designed to be removable or swappable. Allowing users to
specify the OTP memory through a machine option might imply otherwise,
which could be misleading compared to the actual hardware behavior.
That said, if maintaining consistency with QEMU’s device modeling
principles (as done for flash devices) is preferred over strict
hardware modeling fidelity, I am willing to adjust the implementation
accordingly.
Could you please confirm if you still prefer following the edk2 flash
model for OTP memory, despite the slight mismatch with hardware
behavior? Once confirmed, I will proceed with updating the object
hierarchy and the machine properties as discussed.
> > Note: Do not use the -snapshot option, or OTP data writes will not
> > persist to the image file.
> >
> > Signed-off-by: Kane-Chen-AS <kane_c...@aspeedtech.com>
> > ---
> > hw/arm/aspeed_ast10x0.c | 19 +++++++++++++++++++
> > hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
> > include/hw/arm/aspeed_soc.h | 2 ++
> > 3 files changed, 40 insertions(+)
> >
> > diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c index
> > ec329f4991..eaa70feb9f 100644
> > --- a/hw/arm/aspeed_ast10x0.c
> > +++ b/hw/arm/aspeed_ast10x0.c
> > @@ -15,6 +15,7 @@
> > #include "system/system.h"
> > #include "hw/qdev-clock.h"
> > #include "hw/misc/unimp.h"
> > +#include "system/block-backend-global-state.h"
> > #include "hw/arm/aspeed_soc.h"
> >
> > #define ASPEED_SOC_IOMEM_SIZE 0x00200000 @@ -156,6 +157,8 @@
> static
> > void aspeed_soc_ast1030_init(Object *obj)
> >
> > object_initialize_child(obj, "sbc", &s->sbc, TYPE_ASPEED_SBC);
> >
> > + object_initialize_child(obj, "otpmem", &s->otpmem,
> > + TYPE_ASPEED_OTPMEM);
> > +
>
> This belongs to AspeedSBC. See below.
>
> > for (i = 0; i < sc->wdts_num; i++) {
> > snprintf(typename, sizeof(typename), "aspeed.wdt-%s",
> socname);
> > object_initialize_child(obj, "wdt[*]", &s->wdt[i],
> > typename); @@ -194,6 +197,7 @@ static void
> aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
> > Error *err = NULL;
> > int i;
> > g_autofree char *sram_name = NULL;
> > + BlockBackend *blk;
> >
> > if (!clock_has_source(s->sysclk)) {
> > error_setg(errp, "sysclk clock must be wired up by the board
> > code"); @@ -359,6 +363,21 @@ static void
> aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
> >
> ASPEED_SMC_GET_CLASS(&s->spi[i])->flash_window_base);
> > }
> >
> > + /* OTP memory */
> > + blk = blk_by_name(ASPEED_OTPMEM_DRIVE);
> > + if (blk) {
> > + blk_set_perm(blk, BLK_PERM_CONSISTENT_READ |
> BLK_PERM_WRITE,
> > + 0, &error_fatal);
> > + qdev_prop_set_drive(DEVICE(&s->otpmem), "drive", blk);
> > +
> > + if (!sysbus_realize(SYS_BUS_DEVICE(&s->otpmem), errp)) {
> > + return;
> > + }
> > + /* Assign OTP memory to SBC */
> > + object_property_set_link(OBJECT(&s->sbc), "otpmem",
> > + OBJECT(&s->otpmem),
> &error_abort);
> > + }
> > +
>
> The "optmem" machine option should be pointing to "drive" option of the
> AspeedOTPMemState object in this object hierarchy :
>
> /machine (ast2600-evb-machine)
> /soc (ast2600-a3)
> /sbc (aspeed.sbc)
> /aspeed.sbc[0] (memory-region)
> /optmem (aspeed.otpmem) <- move otpmem there
>
> This will require using object_property_add_alias() in 2 or 3 levels.
>
> object_property_add_alias(OBJECT(parent), "optmem"
> OBJECT(child), "drive", &error_abort)
>
> Please try that instead and let's see the result.
>
I will move the otpmem as a child as the aspeed.sbc. For the "optmem" machine
option,
please help confirm which one is the expected behavior.
> Thanks,
>
> C.
>
>
Thanks again for your review and helpful guidance.
Best Regards,
Kane
