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

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

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 <quic_jaehyoo@quicinc.com <mailto:quic_jaehyoo@quicinc.com>> wrote:
>>
>>     From: Graeme Gregory <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>>
>>     ---
>>       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
>>
>>