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

From: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
To: Patrick Venture <venture@google.com>, Hao Wu <wuhaotsh@google.com>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Cédric Le Goater" <clg@kaod.org>,
	"Titus Rwantare" <titusr@google.com>,
	"Andrew Jeffery" <andrew@aj.id.au>,
	"Joel Stanley" <joel@jms.id.au>,
	"Graeme Gregory" <quic_ggregory@quicinc.com>,
	"Maheswara Kurapati" <quic_mkurapat@quicinc.com>,
	qemu-arm <qemu-arm@nongnu.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>
Subject: Re: [PATCH 3/9] hw/arm/aspeed: qcom-dc-scm-v1: add block backed FRU device
Date: Thu, 23 Jun 2022 09:34:20 -0700	[thread overview]
Message-ID: <852d9c04-1ece-3b88-c8f2-d96d333f7bde@quicinc.com> (raw)
In-Reply-To: <CAO=notzsLW=F6QDu5mAhBCXNMocV1_qF3EazHYLoP_mu8e8QFg@mail.gmail.com>

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)

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
> 
> 