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),
};
