Re: [PATCH v2 4/5] hw/arm/aspeed: Add aspeed_eeprom.c

[Peter Delevoryas]

On Tue, Jan 17, 2023 at 08:39:06AM +0100, Cédric Le Goater wrote:
> On 1/17/23 00:56, Peter Delevoryas wrote:
> > - Create aspeed_eeprom.c and aspeed_eeprom.h
> > - Include aspeed_eeprom.c in CONFIG_ASPEED meson source files
> > - Include aspeed_eeprom.h in aspeed.c
> > - Add fby35_bmc_fruid data
> > - Use new at24c_eeprom_init_rom helper to initialize BMC FRUID EEPROM with 
> > data
> >    from aspeed_eeprom.c
> > 
> > wget 
> > https://github.com/facebook/openbmc/releases/download/openbmc-e2294ff5d31d/fby35.mtd
> > qemu-system-aarch64 -machine fby35-bmc -nographic -mtdblock fby35.mtd
> > ...
> > user: root
> > pass: 0penBmc
> > ...
> > root@bmc-oob:~# fruid-util bmc
> > 
> > FRU Information           : BMC
> > ---------------           : ------------------
> > Board Mfg Date            : Mon Jan 10 21:42:00 2022
> > Board Mfg                 : XXXXXX
> > Board Product             : BMC Storage Module
> > Board Serial              : XXXXXXXXXXXXX
> > Board Part Number         : XXXXXXXXXXXXXX
> > Board FRU ID              : 1.0
> > Board Custom Data 1       : XXXXXXXXX
> > Board Custom Data 2       : XXXXXXXXXXXXXXXXXX
> > Product Manufacturer      : XXXXXX
> > Product Name              : Yosemite V3.5 EVT2
> > Product Part Number       : XXXXXXXXXXXXXX
> > Product Version           : EVT2
> > Product Serial            : XXXXXXXXXXXXX
> > Product Asset Tag         : XXXXXXX
> > Product FRU ID            : 1.0
> > Product Custom Data 1     : XXXXXXXXX
> > Product Custom Data 2     : Config A
> > 
> > Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> One little comment below,
> 
> > ---
> >   hw/arm/aspeed.c        |  4 +++-
> >   hw/arm/aspeed_eeprom.c | 51 ++++++++++++++++++++++++++++++++++++++++++
> >   hw/arm/aspeed_eeprom.h | 11 +++++++++
> >   hw/arm/meson.build     |  1 +
> >   4 files changed, 66 insertions(+), 1 deletion(-)
> >   create mode 100644 hw/arm/aspeed_eeprom.c
> >   create mode 100644 hw/arm/aspeed_eeprom.h
> > 
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index c929c61d582a..11e423db4538 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -14,6 +14,7 @@
> >   #include "hw/arm/boot.h"
> >   #include "hw/arm/aspeed.h"
> >   #include "hw/arm/aspeed_soc.h"
> > +#include "hw/arm/aspeed_eeprom.h"
> >   #include "hw/i2c/i2c_mux_pca954x.h"
> >   #include "hw/i2c/smbus_eeprom.h"
> >   #include "hw/misc/pca9552.h"
> > @@ -942,7 +943,8 @@ static void fby35_i2c_init(AspeedMachineState *bmc)
> >       at24c_eeprom_init(i2c[6], 0x51, 128 * KiB);
> >       at24c_eeprom_init(i2c[8], 0x50, 32 * KiB);
> >       at24c_eeprom_init(i2c[11], 0x51, 128 * KiB);
> > -    at24c_eeprom_init(i2c[11], 0x54, 128 * KiB);
> > +    at24c_eeprom_init_rom(i2c[11], 0x54, 128 * KiB, fby35_bmc_fruid,
> > +                          fby35_bmc_fruid_size);
> >       /*
> >        * TODO: There is a multi-master i2c connection to an AST1030 MiniBMC 
> > on
> > diff --git a/hw/arm/aspeed_eeprom.c b/hw/arm/aspeed_eeprom.c
> > new file mode 100644
> > index 000000000000..a5ffa959927b
> > --- /dev/null
> > +++ b/hw/arm/aspeed_eeprom.c
> > @@ -0,0 +1,51 @@
> > +/* Copyright (c) Meta Platforms, Inc. and affiliates. */
> > +
> > +#include "aspeed_eeprom.h"
> > +
> > +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,
> > +};
> > +
> > +const uint32_t fby35_bmc_fruid_size = sizeof(fby35_bmc_fruid);
> > diff --git a/hw/arm/aspeed_eeprom.h b/hw/arm/aspeed_eeprom.h
> > new file mode 100644
> > index 000000000000..89860e37d007
> > --- /dev/null
> > +++ b/hw/arm/aspeed_eeprom.h
> > @@ -0,0 +1,11 @@
> > +/* Copyright (c) Meta Platforms, Inc. and affiliates. */
> > +
> > +#ifndef ASPEED_EEPROM_H
> > +#define ASPEED_EEPROM_H
> > +
> > +#include "qemu/osdep.h"
> > +
> > +extern const uint8_t fby35_bmc_fruid[];
> 
> 
> may be define the array with an explicit size to avoid the size variable ?
> I don't see any good solution.

Yeah whatever seems most natural to you guys. Explicit size sounds fine to me,
it's easy enough to check the size using compiler errors.

> 
> Thanks,
> 
> C.
> 
> 
> 
> > +extern const uint32_t fby35_bmc_fruid_size;
> > +
> > +#endif
> > diff --git a/hw/arm/meson.build b/hw/arm/meson.build
> > index 76d4d650e42e..f70e8cfd4545 100644
> > --- a/hw/arm/meson.build
> > +++ b/hw/arm/meson.build
> > @@ -53,6 +53,7 @@ arm_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
> >     'aspeed.c',
> >     'aspeed_ast2600.c',
> >     'aspeed_ast10x0.c',
> > +  'aspeed_eeprom.c',
> >     'fby35.c'))
> >   arm_ss.add(when: 'CONFIG_MPS2', if_true: files('mps2.c'))
> >   arm_ss.add(when: 'CONFIG_MPS2', if_true: files('mps2-tz.c'))
>