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

[Cédric Le Goater]

Adding Markus,

On 6/23/22 19:37, Patrick Venture wrote:



On Thu, Jun 23, 2022 at 10:16 AM Cédric Le Goater mailto:c...@kaod.org>> wrote:

On 6/23/22 17:28, Patrick Venture wrote:
 >
 >
 > On Wed, Jun 22, 2022 at 10:48 AM Jae Hyun Yoo mailto:quic_jaeh...@quicinc.com> >> wrote:
 >
 >     From: Graeme Gregory mailto:quic_ggreg...@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 mailto:quic_ggreg...@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 
 that we 
still need to finish upstreaming:


      /* mb_fru */
     at24c_eeprom_init_one(npcm7xx_i2c_get_bus(soc, 5), 5, 0x50, 8192, 0);

     /* 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);


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.

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

[Cédric Le Goater]

On 6/22/22 19:28, Jae Hyun Yoo wrote:

From: Graeme Gregory 

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 




Reviewed-by: Cédric Le Goater 

Thanks,

C.


---
  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);
+}
+
  static void qcom_dc_scm_bmc_i2c_init(AspeedMachineState *bmc)
  {
  AspeedSoCState *soc = &bmc->soc;
  
  i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 15), "tmp105", 0x4d);
  
-uint8_t *eeprom_buf = g_malloc0(128 * 1024);

-if (eeprom_buf) {
-smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53,
-  eeprom_buf);
-}
+qcom_dc_scm_fru_init(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53, 128 * 1024);
  }
  
  static bool aspeed_get_mmio_exec(Object *obj, Error **errp)

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

[Jae Hyun Yoo]

On 6/23/2022 10:37 AM, Patrick Venture wrote:



On Thu, Jun 23, 2022 at 10:16 AM Cédric Le Goater > wrote:


On 6/23/22 17:28, Patrick Venture wrote:
 >
 >
 > On Wed, Jun 22, 2022 at 10:48 AM Jae Hyun Yoo
mailto:quic_jaeh...@quicinc.com>
>>
wrote:
 >
 >     From: Graeme Gregory mailto:quic_ggreg...@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 mailto:quic_ggreg...@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 
 
that we still need to finish upstreaming:



      /* mb_fru */
     at24c_eeprom_init_one(npcm7xx_i2c_get_bus(soc, 5), 5, 0x50, 8192, 0);

     /* 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);



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.


Okay. I found that the above code was tried to upstream earlier by Hao
but it's not completed yet and it's pending at below thread.

https://lore.kernel.org/qemu-devel/875ystigke.fsf...@dus

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

[Patrick Venture]

On Thu, Jun 23, 2022 at 10:16 AM Cédric Le Goater  wrote:

> On 6/23/22 17:28, Patrick Venture wrote:
> >
> >
> > On Wed, Jun 22, 2022 at 10:48 AM Jae Hyun Yoo  > wrote:
> >
> > From: Graeme Gregory  quic_ggreg...@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_ggreg...@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 that
we still need to finish upstreaming:


 /* mb_fru */
at24c_eeprom_init_one(npcm7xx_i2c_get_bus(soc, 5), 5, 0x50, 8192, 0);

/* 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);


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.



> Thanks,
>
> C.
>

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

[Cédric Le Goater]

On 6/23/22 17:28, Patrick Venture wrote:



On Wed, Jun 22, 2022 at 10:48 AM Jae Hyun Yoo mailto:quic_jaeh...@quicinc.com>> wrote:

From: Graeme Gregory mailto:quic_ggreg...@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 mailto:quic_ggreg...@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 ?

Thanks,

C.

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

[Cédric Le Goater]

On 6/23/22 18:34, Jae Hyun Yoo wrote:

Hello Patrick,

On 6/23/2022 8:28 AM, Patrick Venture wrote:



On Wed, Jun 22, 2022 at 10:48 AM Jae Hyun Yoo mailto:quic_jaeh...@quicinc.com>> wrote:

    From: Graeme Gregory mailto:quic_ggreg...@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 mailto:quic_ggreg...@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.


Okay. I agree with you that it can be more generic to be used for other
machines too. I'll rewrite this function in v2 to make it have below
shape.

static void
at24c_eeprom_init_from_drive(I2CBus *bus, uint8_t addr, uint32_t rsize,
  int bus, int unit)


Yes and if other non-Aspeed machines need it one day, we could export it.
There is no need for now to multiply the interface now.

Thanks,

C.



Thanks,
Jae


    +
  static void qcom_dc_scm_bmc_i2c_init(AspeedMachineState *bmc)
  {
      AspeedSoCState *soc = &bmc->soc;

      i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 15),
    "tmp105", 0x4d);

    -    uint8_t *eeprom_buf = g_malloc0(128 * 1024);
    -    if (eeprom_buf) {
    -        smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53,
    -                              eeprom_buf);
    -    }
    +    qcom_dc_scm_fru_init(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53,
    128 * 1024);
  }

  static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
    --     2.25.1

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

[Jae Hyun Yoo]

Hello Patrick,

On 6/23/2022 8:28 AM, Patrick Venture wrote:



On Wed, Jun 22, 2022 at 10:48 AM Jae Hyun Yoo > wrote:


From: Graeme Gregory mailto:quic_ggreg...@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 mailto:quic_ggreg...@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.


Okay. I agree with you that it can be more generic to be used for other
machines too. I'll rewrite this function in v2 to make it have below
shape.

static void
at24c_eeprom_init_from_drive(I2CBus *bus, uint8_t addr, uint32_t rsize,
 int bus, int unit)

Thanks,
Jae


+
  static void qcom_dc_scm_bmc_i2c_init(AspeedMachineState *bmc)
  {
      AspeedSoCState *soc = &bmc->soc;

      i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 15),
"tmp105", 0x4d);

-    uint8_t *eeprom_buf = g_malloc0(128 * 1024);
-    if (eeprom_buf) {
-        smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53,
-                              eeprom_buf);
-    }
+    qcom_dc_scm_fru_init(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53,
128 * 1024);
  }

  static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
-- 
2.25.1

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

[Patrick Venture]

On Wed, Jun 22, 2022 at 10:48 AM Jae Hyun Yoo 
wrote:

> From: Graeme Gregory 
>
> 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 
> ---
>  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.


> +
>  static void qcom_dc_scm_bmc_i2c_init(AspeedMachineState *bmc)
>  {
>  AspeedSoCState *soc = &bmc->soc;
>
>  i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 15), "tmp105",
> 0x4d);
>
> -uint8_t *eeprom_buf = g_malloc0(128 * 1024);
> -if (eeprom_buf) {
> -smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53,
> -  eeprom_buf);
> -}
> +qcom_dc_scm_fru_init(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53, 128 *
> 1024);
>  }
>
>  static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
> --
> 2.25.1
>
>
>

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

[Jae Hyun Yoo]

On 6/22/2022 10:28 PM, Joel Stanley wrote:

On Wed, 22 Jun 2022 at 17:29, Jae Hyun Yoo  wrote:


From: Graeme Gregory 

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 
---
  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);
+}
+
  static void qcom_dc_scm_bmc_i2c_init(AspeedMachineState *bmc)
  {
  AspeedSoCState *soc = &bmc->soc;

  i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 15), "tmp105", 
0x4d);

-uint8_t *eeprom_buf = g_malloc0(128 * 1024);
-if (eeprom_buf) {
-smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53,
-  eeprom_buf);
-}


Again, it's strange to see code that was just added being removed. If
you want the FRU to be in its own patch then remove the eeprom from
the previous patch.


I see. I'll remove it from the 2/9 patch.

Thanks,
Jae


+qcom_dc_scm_fru_init(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53, 128 * 1024);
  }

  static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
--
2.25.1

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

[Joel Stanley]

On Wed, 22 Jun 2022 at 17:29, Jae Hyun Yoo  wrote:
>
> From: Graeme Gregory 
>
> 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 
> ---
>  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);
> +}
> +
>  static void qcom_dc_scm_bmc_i2c_init(AspeedMachineState *bmc)
>  {
>  AspeedSoCState *soc = &bmc->soc;
>
>  i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 15), "tmp105", 
> 0x4d);
>
> -uint8_t *eeprom_buf = g_malloc0(128 * 1024);
> -if (eeprom_buf) {
> -smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53,
> -  eeprom_buf);
> -}

Again, it's strange to see code that was just added being removed. If
you want the FRU to be in its own patch then remove the eeprom from
the previous patch.

> +qcom_dc_scm_fru_init(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53, 128 * 
> 1024);
>  }
>
>  static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
> --
> 2.25.1
>