On Mon, Jan 16, 2023 at 01:42:48PM +0100, Cédric Le Goater wrote: > On 1/14/23 18:01, Peter Delevoryas wrote: > > Signed-off-by: Peter Delevoryas <pe...@pjd.dev> > > --- > > hw/arm/aspeed.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > > index c929c61d582a..4ac8ff11a835 100644 > > --- a/hw/arm/aspeed.c > > +++ b/hw/arm/aspeed.c > > @@ -922,6 +922,52 @@ static void bletchley_bmc_i2c_init(AspeedMachineState > > *bmc) > > i2c_slave_create_simple(i2c[12], TYPE_PCA9552, 0x67); > > } > > +static const uint8_t fby35_bmc_fruid[] = { > > + 0x01, 0x00, 0x00, 0x01, 0x0d, 0x00, 0x00, 0xf1, 0x01, 0x0c, 0x00, 0x36, > > + 0xe6, 0xd0, 0xc6, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0xd2, 0x42, 0x4d, > > + 0x43, 0x20, 0x53, 0x74, 0x6f, 0x72, 0x61, 0x67, 0x65, 0x20, 0x4d, 0x6f, > > + 0x64, 0x75, 0x6c, 0x65, 0xcd, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, > > + 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0xce, 0x58, 0x58, 0x58, 0x58, 0x58, > > + 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0xc3, 0x31, 0x2e, > > + 0x30, 0xc9, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0xd2, > > + 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, > > + 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0xc1, 0x39, 0x01, 0x0c, 0x00, 0xc6, > > + 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0xd2, 0x59, 0x6f, 0x73, 0x65, 0x6d, > > + 0x69, 0x74, 0x65, 0x20, 0x56, 0x33, 0x2e, 0x35, 0x20, 0x45, 0x56, 0x54, > > + 0x32, 0xce, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, > > + 0x58, 0x58, 0x58, 0x58, 0xc4, 0x45, 0x56, 0x54, 0x32, 0xcd, 0x58, 0x58, > > + 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0xc7, > > + 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0xc3, 0x31, 0x2e, 0x30, 0xc9, > > + 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0x58, 0xc8, 0x43, 0x6f, > > + 0x6e, 0x66, 0x69, 0x67, 0x20, 0x41, 0xc1, 0x45, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > +}; > > > I would introduce a new aspeed_eeprom.c file for these definitions because > each machine could have its own set of eeproms and aspeed.c is already big > enough.
+1 > > > static void fby35_i2c_init(AspeedMachineState *bmc) > > { > > AspeedSoCState *soc = &bmc->soc; > > @@ -1363,6 +1409,9 @@ static void fby35_reset(MachineState *state, > > ShutdownCause reason) > > object_property_set_bool(OBJECT(gpio), "gpioB3", false, &error_fatal); > > object_property_set_bool(OBJECT(gpio), "gpioB4", false, &error_fatal); > > object_property_set_bool(OBJECT(gpio), "gpioB5", false, &error_fatal); > > + > > + at24c_eeprom_write(aspeed_i2c_get_bus(&bmc->soc.i2c, 11), > > + 0x54, 0, fby35_bmc_fruid, sizeof(fby35_bmc_fruid)); > > } > > That's one way to model the default reset values of the eeprom, we would > loose any writes though. > > I think we should have a reset_data buffer instead, which would be used > at realize time to set the initial data, if there are no drive backend, > and at reset if !writable. Something like smbus_eeprom_init_one() does > without a proper property API. I actually did it this way downstream[1], but I thought it would be controversial without using properties, and I wasn't sure how to model it with properties. [1] https://github.com/facebook/openbmc/blob/b98f10b7967ebce1f97e14a9a5d4c14f9f7d4c55/common/recipes-devtools/qemu/qemu/0014-hw-nvram-at24c-Add-static-memory-init-option.patch > > We would need some new interface to set a property for a constant buffer > of uint<>_t values. I don't know how complex that would be. It could be > useful to other models to define the init state of registers. Would DEFINE_PROP_ARRAY not work? Won't the properties be constant after qdev_realize()? Or if that's not preferable, DEFINE_PROP_LINK (uint8_t *reset_data) + DEFINE_PROP_UINT32 (reset_data_size)? Otherwise, if the property stuff is too uncertain, I'd be happy just adding an adhoc API through the helper like smbus_eeprom_init_one does. > > Thanks, > > C. > > > static void aspeed_machine_fby35_class_init(ObjectClass *oc, void *data) >