From: Peter Delevoryas <peter@pjd.dev>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: patrick@stwcx.xyz, peter.maydell@linaro.org, andrew@aj.id.au,
joel@jms.id.au, hskinnemoen@google.com, kfting@nuvoton.com,
qemu-arm@nongnu.org, qemu-devel@nongnu.org,
"Philippe Mathieu-Daudé" <philmd@linaro.org>
Subject: Re: [PATCH 6/6] hw/arm/aspeed: Init fby35 BMC FRUID EEPROM
Date: Mon, 16 Jan 2023 09:19:26 -0800 [thread overview]
Message-ID: <Y8WHHrGicyqr9hzP@pdel-mbp> (raw)
In-Reply-To: <3131a986-1db0-8ab2-12cd-11aa32ea29e0@kaod.org>
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 <peter@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)
>
next prev parent reply other threads:[~2023-01-16 17:20 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-14 17:01 [PATCH 0/6] hw/nvram/eeprom_at24c: Cleanup + FRUID EEPROM init example Peter Delevoryas
2023-01-14 17:01 ` [PATCH 1/6] hw/nvram/eeprom_at24c: Add header w/ init helper Peter Delevoryas
2023-01-16 11:13 ` Cédric Le Goater
2023-01-16 12:23 ` Philippe Mathieu-Daudé
2023-01-16 17:21 ` Peter Delevoryas
2023-01-14 17:01 ` [PATCH 2/6] hw/arm/aspeed: Remove local copy of at24c_eeprom_init Peter Delevoryas
2023-01-16 11:13 ` Cédric Le Goater
2023-01-16 12:24 ` Philippe Mathieu-Daudé
2023-01-16 17:21 ` Peter Delevoryas
2023-01-14 17:01 ` [PATCH 3/6] hw/arm/aspeed: Replace aspeed_eeprom_init with at24c_eeprom_init Peter Delevoryas
2023-01-16 11:14 ` Cédric Le Goater
2023-01-16 12:25 ` Philippe Mathieu-Daudé
2023-01-14 17:01 ` [PATCH 4/6] hw/arm/npcm7xx: Remove local copy of at24c_eeprom_init Peter Delevoryas
2023-01-16 11:14 ` Cédric Le Goater
2023-01-16 12:26 ` Philippe Mathieu-Daudé
2023-01-14 17:01 ` [PATCH 5/6] hw/nvram/eeprom_at24c: Add I2C write helper Peter Delevoryas
2023-01-14 17:01 ` [PATCH 6/6] hw/arm/aspeed: Init fby35 BMC FRUID EEPROM Peter Delevoryas
2023-01-16 12:30 ` Philippe Mathieu-Daudé
2023-01-16 17:23 ` Peter Delevoryas
2023-01-17 6:47 ` Philippe Mathieu-Daudé
2023-01-17 17:49 ` Peter Delevoryas
2023-01-16 12:42 ` Cédric Le Goater
2023-01-16 17:19 ` Peter Delevoryas [this message]
2023-01-14 17:07 ` [PATCH 0/6] hw/nvram/eeprom_at24c: Cleanup + FRUID EEPROM init example Peter Delevoryas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y8WHHrGicyqr9hzP@pdel-mbp \
--to=peter@pjd.dev \
--cc=andrew@aj.id.au \
--cc=clg@kaod.org \
--cc=hskinnemoen@google.com \
--cc=joel@jms.id.au \
--cc=kfting@nuvoton.com \
--cc=patrick@stwcx.xyz \
--cc=peter.maydell@linaro.org \
--cc=philmd@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).