> -----Original Message----- > From: Cédric Le Goater <[email protected]> > Sent: Monday, August 31, 2026 7:10 PM > To: Kane Chen <[email protected]>; Peter Maydell > <[email protected]>; Steven Lee <[email protected]>; Troy > Lee <[email protected]>; Jamin Lin <[email protected]>; Andrew > Jeffery <[email protected]>; Joel Stanley <[email protected]>; open > list:ASPEED BMCs <[email protected]>; open list:All patches CC here > <[email protected]> > Cc: Troy Lee <[email protected]> > Subject: Re: [PATCH v1 1/2] hw/misc/aspeed_sbc: Derive ABR state from OTP > config straps > > On 8/31/26 07:37, Kane Chen wrote: > > The eMMC ABR (Alternate Boot Recovery) enable state was controlled by > > a fixed "emmc-abr" machine property, and the logic reflecting it in > > R_STATUS was broken so ABR could never actually be reported as > > enabled. > > > > Instead of relying on the property, read the ABR strap value directly > > from the OTP configuration space and derive the enable state from it, > > matching how the real hardware determines ABR. The now-unused > > "emmc-abr" property is removed. > > This is an ABI breakage. It was never used and broken. I guess we are fine. > > > Signed-off-by: Kane-Chen-AS <[email protected]> > > --- > > include/hw/misc/aspeed_sbc.h | 1 - > > hw/misc/aspeed_sbc.c | 35 > ++++++++++++++++++++++++++++++++--- > > 2 files changed, 32 insertions(+), 4 deletions(-) > > > > diff --git a/include/hw/misc/aspeed_sbc.h > > b/include/hw/misc/aspeed_sbc.h index 07c7c22a86..7152497b2a 100644 > > --- a/include/hw/misc/aspeed_sbc.h > > +++ b/include/hw/misc/aspeed_sbc.h > > @@ -32,7 +32,6 @@ OBJECT_DECLARE_TYPE(AspeedSBCState, > AspeedSBCClass, ASPEED_SBC) > > struct AspeedSBCState { > > SysBusDevice parent; > > > > - bool emmc_abr; > > uint32_t signing_settings; > > > > MemoryRegion iomem; > > diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c index > > 1dfcf14e5b..5d4da39d30 100644 > > --- a/hw/misc/aspeed_sbc.c > > +++ b/hw/misc/aspeed_sbc.c > > @@ -60,6 +60,9 @@ > > #define MODE_REGISTER_A (0x3000) > > #define MODE_REGISTER_B (0x5000) > > > > +/* OTP Address */ > > +#define OTP_CFG0 (0x800) > > + > > I think that several OTP defines describing the OTP address space would be > better placed in aspeed_otp.h : > > #define OTP_MEMORY_SIZE 0x4000 > > #define OTP_DATA_DWORD_COUNT (0x800) > #define OTP_TOTAL_DWORD_COUNT (0x1000) > > /* OTP Address */ > #define OTP_CFG0 (0x800) > > Please explain the various regions. > > also, does this test makes sense : > > if (otp_addr >= OTP_TOTAL_DWORD_COUNT) { > qemu_log_mask(LOG_GUEST_ERROR, > "Invalid OTP addr 0x%x\n", > otp_addr); > return false; > } > > since the transaction on the address_space should fail. Or it could be > > Please rework aspeed_sbc_otp_read() which duplicate the same code twice. > > > static bool aspeed_otp_read_raw(AspeedSBCState *s, uint32_t otp_addr, > uint32_t *value) > { > uint32_t otp_offset = otp_addr << 2; > > if (address_space_read(&s->otp.as, otp_offset, > MEMTXATTRS_UNSPECIFIED, > value, sizeof(*value)) != MEMTX_OK) { > qemu_log_mask(LOG_GUEST_ERROR, > "Failed to read OTP memory, addr = %x\n", > otp_addr); > return false; > } > > return true; > } > > static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int > size) > > { > > AspeedSBCState *s = ASPEED_SBC(opaque); @@ -261,17 +264,44 > @@ > > static const MemoryRegionOps aspeed_sbc_ops = { > > }, > > }; > > > > +static bool aspeed_get_abr_state(AspeedSBCState *s) > > aspeed_sbc_otp_get_abr_state() > > Shouln't this be done in the OP model instead. > > > +{ > > + uint32_t value; > > + int i; > > + bool enable = false; > > + int config_offset; > > + > > + /* > > + * ABR is a strap setting, and each strap setting consists of six > > + * sub-values. Read all sub-values to retrieve the latest setting. > > + */ > > + for (i = 17; i < 28; i += 2) { > > sigh. What are these magic values ... ? Please explain. > > > + config_offset = OTP_CFG0; > > + config_offset |= (i / 8) * 0x200; > > + config_offset |= (i % 8) * 0x2; > > + > > + aspeed_sbc_otp_read(s, config_offset); > > May be introduce a aspeed_sbc_otp_read_config() helper. > > > + value = s->regs[R_CAMP1]; > > + enable ^= (value >> 11) & 0x1; > > + } > > + > > + return enable; > > +} > > + > > static void aspeed_sbc_reset_hold(Object *obj, ResetType type) > > { > > AspeedSBCState *s = ASPEED_SBC(obj); > > + bool abr; > > > > memset(s->regs, 0, sizeof(s->regs)); > > > > + abr = aspeed_get_abr_state(s); > > This creates an implicit ordering dependency: works because OTP is a QOM > child of the SBC, so its reset runs first. > > So, can't we do that at OTP realize time instead ? OTP content is non-volatile > and doesn't change across resets, so reading it once at realize time is > sufficient. > > > + > > /* Set secure boot enabled with RSA4096_SHA256 and enable eMMC > ABR */ > > s->regs[R_STATUS] = OTP_IDLE | OTP_MEM_IDLE; > > > > - if (s->emmc_abr) { > > - s->regs[R_STATUS] &= ABR_EN; > > + if (abr) { > > + s->regs[R_STATUS] |= ABR_EN; > > } > > > > if (s->signing_settings) { > > @@ -323,7 +353,6 @@ 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), > > }; > > Hi Cédric,
Thanks for your review and comments. I will address the comments regarding the OTP address definitions, boundary checks, and code cleanup. Regarding the interaction between the SBC and OTP models, I would like to clarify the expected behavior before making further changes. On real hardware, some OTP settings are reflected in SBC registers. For example, the eMMC ABR enable state and secure boot enable state are reflected in the SBC STATUS register. However, the OTP configuration can also be programmed at runtime. Therefore, reading the OTP configuration only once at realize/reset time may leave the SBC STATUS register out of sync if the OTP configuration is changed afterward. I see two possible approaches to keep the SBC STATUS register synchronized with the OTP configuration: Update the SBC STATUS register at reset and whenever the relevant OTP configuration is programmed. This would keep the cached STATUS value synchronized with the latest OTP configuration. Derive the relevant SBC STATUS bits directly from the OTP configuration whenever the STATUS register is read. This avoids maintaining a cached copy of the OTP-derived state. Could you please advise which approach would be preferred? Best Regards, Kane
