Re: [PATCH 3/9] hw/arm/aspeed: qcom-dc-scm-v1: add block backed FRU device

["Cédric Le Goater" <clg@kaod.org>]

Adding Markus,

On 6/23/22 19:37, Patrick Venture wrote:
> 
> 
> On Thu, Jun 23, 2022 at 10:16 AM Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>> wrote:
> 
>     On 6/23/22 17:28, Patrick Venture wrote:
>      >
>      >
>      > On Wed, Jun 22, 2022 at 10:48 AM Jae Hyun Yoo <quic_jaehyoo@quicinc.com <mailto:quic_jaehyoo@quicinc.com> <mailto:quic_jaehyoo@quicinc.com <mailto:quic_jaehyoo@quicinc.com>>> wrote:
>      >
>      >     From: Graeme Gregory <quic_ggregory@quicinc.com <mailto:quic_ggregory@quicinc.com> <mailto:quic_ggregory@quicinc.com <mailto:quic_ggregory@quicinc.com>>>
>      >
>      >     The FRU device uses the index 0 device on bus IF_NONE.
>      >
>      >     -drive file=$FRU,format=raw,if=none
>      >
>      >     file must match FRU size of 128k
>      >
>      >     Signed-off-by: Graeme Gregory <quic_ggregory@quicinc.com <mailto:quic_ggregory@quicinc.com> <mailto:quic_ggregory@quicinc.com <mailto:quic_ggregory@quicinc.com>>>
>      >     ---
>      >       hw/arm/aspeed.c | 22 +++++++++++++++++-----
>      >       1 file changed, 17 insertions(+), 5 deletions(-)
>      >
>      >     diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>      >     index 785cc543d046..36d6b2c33e48 100644
>      >     --- a/hw/arm/aspeed.c
>      >     +++ b/hw/arm/aspeed.c
>      >     @@ -992,17 +992,29 @@ static void fby35_i2c_init(AspeedMachineState *bmc)
>      >            */
>      >       }
>      >
>      >     +static void qcom_dc_scm_fru_init(I2CBus *bus, uint8_t addr, uint32_t rsize)
>      >     +{
>      >     +    I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
>      >     +    DeviceState *dev = DEVICE(i2c_dev);
>      >     +    /* Use First Index for DC-SCM FRU */
>      >     +    DriveInfo *dinfo = drive_get(IF_NONE, 0, 0);
>      >     +
>      >     +    qdev_prop_set_uint32(dev, "rom-size", rsize);
>      >     +
>      >     +    if (dinfo) {
>      >     +        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>      >     +    }
>      >     +
>      >     +    i2c_slave_realize_and_unref(i2c_dev, bus, &error_abort);
>      >     +}
>      >
>      >
>      > We've sent a similar patch up for the at24c but in its own file -- but looking at this, we could likely expand it to suite our cases as well - is there a reason it's named qcom_dc_scm_fru_init?  Presumably that's to handle the drive_get parameters.  If you make it use `drive_get(IF_NONE, bus, unit);` You'll be able to associate a drive via parameters like you aim to.
> 
> 
>     I have seen various attempts to populate the Aspeed eeproms with
>     data. The simplest is the g220a_bmc_i2c_init() approach. Some are
>     generating C code from the .bin files and compiling the result in
>     QEMU. Some were generating elf sections, linking them in QEMU and
>     passing the pointer as an eeprom buf.
> 
>     The drive approach is the best I think, but the device/drive
>     association depends on the drive order on the command line when
>     devices are created by the platform.
> 
>     We could may be extend the GenericLoaderState for eeproms ?
>     or introduce a routine which would look for a file under a (pc-bios)
>     per-machine directory and fill the eeprom buf with its contents ?
> 
>     Any idea ?
> 
> 
> So we have this in our eeprom_at24c module because we use it with Aspeed and Nuvoton:
> 
> void at24c_eeprom_init_one(I2CBus *i2c_bus, int bus, uint8_t addr,
>                             uint32_t rsize, int unit_number)
> {
>      I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
>      DeviceState *dev = DEVICE(i2c_dev);
>      BlockInterfaceType type = IF_NONE;
>      DriveInfo *dinfo;
> 
>      dinfo = drive_get(type, bus, unit_number);
>      if (dinfo) {
>          qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>      }
>      qdev_prop_set_uint32(dev, "rom-size", rsize);
>      i2c_slave_realize_and_unref(i2c_dev, i2c_bus, &error_abort);
> }
> 
> With this style call in the board:
> 
> snippet from downstream version of https://github.com/qemu/qemu/blob/master/hw/arm/npcm7xx_boards.c#L305 <https://github.com/qemu/qemu/blob/master/hw/arm/npcm7xx_boards.c#L305> that we still need to finish upstreaming:
> 
> <snip>
>       /* mb_fru */
>      at24c_eeprom_init_one(npcm7xx_i2c_get_bus(soc, 5), 5, 0x50, 8192, 0);
> <snip>
>      /* fan_fru */
>      at24c_eeprom_init_one(pca954x_i2c_get_bus(i2c_mux, 2), 5, 0x51, 8192, 1);
>      /* hsbp_fru */
>      at24c_eeprom_init_one(pca954x_i2c_get_bus(i2c_mux, 3), 5, 0x52, 8192, 2);
> <snip>
> 
> And then we call it with the bus and unit, the pair of those has to be unique for the drive parameter to work:
> 
> -drive file=/export/hda3/tmp/mb_fru@50,if=none,bus=5,unit=0,snapshot=off,format=raw
> -drive file=/export/hda3/tmp/fan_fru@51,if=none,bus=5,unit=1,snapshot=off,format=raw
> -drive file=/export/hda3/tmp/hsbp_fru@52,if=none,bus=5,unit=2,snapshot=off,format=raw
> 
> The above code snippet is what, I'm fairly certain we had sent up for review before, and we can send it again if you want.

You could. I am not sure this is the good direction but it would restart
the -drive topic.


Thanks,

C.