Re: [PATCH 4/9] hw/arm/aspeed: add Qualcomm Firework machine and FRU device

[Jae Hyun Yoo]

On 6/24/2022 12:32 AM, Cédric Le Goater wrote:
> On 6/23/22 16:11, Jae Hyun Yoo wrote:
> > On 6/22/2022 11:43 PM, Cédric Le Goater wrote:
> > > On 6/22/22 19:28, Jae Hyun Yoo wrote:
> > > > From: Graeme Gregory <quic_ggregory@quicinc.com>
> > > >
> > > > Add base for Qualcomm Firework machine and add its FRU device which is
> > > > defined by DC-SCM to be fixed address 0x50.
> > > >
> > > > Signed-off-by: Graeme Gregory <quic_ggregory@quicinc.com>
> > > > ---
> > > >   hw/arm/aspeed.c | 53 
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > > >   1 file changed, 53 insertions(+)
> > > >
> > > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > > > index 36d6b2c33e48..0e6edd2be4fa 100644
> > > > --- a/hw/arm/aspeed.c
> > > > +++ b/hw/arm/aspeed.c
> > > > @@ -1017,6 +1017,35 @@ static void 
> > > > qcom_dc_scm_bmc_i2c_init(AspeedMachineState *bmc)
> > > >       qcom_dc_scm_fru_init(aspeed_i2c_get_bus(&soc->i2c, 15), 0x53, 
> > > > 128 * 1024);
> > > >   }
> > > > +static void qcom_firework_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, 1);
> > > > +
> > > > +    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_firework_i2c_init(AspeedMachineState *bmc)
> > > > +{
> > > > +    AspeedSoCState *soc = &bmc->soc;
> > > > +
> > > > +    /* Create the generic DC-SCM hardware */
> > > > +    qcom_dc_scm_bmc_i2c_init(bmc);
> > > > +
> > > > +    /* Now create the Firework specific hardware */
> > > > +
> > > > +    /* I2C4 */
> > > > +    qcom_firework_fru_init(aspeed_i2c_get_bus(&soc->i2c, 4), 0x50, 
> > > > 128 * 1024);
> > > > +}
> > > > +
> > > >   static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
> > > >   {
> > > >       return ASPEED_MACHINE(obj)->mmio_exec;
> > > > @@ -1489,6 +1518,26 @@ static void 
> > > > aspeed_machine_qcom_dc_scm_v1_class_init(ObjectClass *oc,
> > > >           aspeed_soc_num_cpus(amc->soc_name);
> > > >   };
> > > > +static void aspeed_machine_qcom_firework_class_init(ObjectClass *oc,
> > > > +                                                    void *data)
> > > > +{
> > > > +    MachineClass *mc = MACHINE_CLASS(oc);
> > > > +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> > > > +
> > > > +    mc->desc       = "Qualcomm DC-SCM V1/Firework BMC (Cortex A7)";
> > > > +    amc->soc_name  = "ast2600-a3";
> > > > +    amc->hw_strap1 = QCOM_DC_SCM_V1_BMC_HW_STRAP1;
> > > > +    amc->hw_strap2 = QCOM_DC_SCM_V1_BMC_HW_STRAP2;
> > > > +    amc->fmc_model = "n25q512a";
> > > > +    amc->spi_model = "n25q512a";
> > > > +    amc->num_cs    = 2;
> > > > +    amc->macs_mask = ASPEED_MAC2_ON | ASPEED_MAC3_ON;
> > > > +    amc->i2c_init  = qcom_dc_scm_firework_i2c_init;
> > > > +    mc->default_ram_size = 1 * GiB;
> > > > +    mc->default_cpus = mc->min_cpus = mc->max_cpus =
> > > > +        aspeed_soc_num_cpus(amc->soc_name);
> > > > +};
> > > > +
> > > >   static const TypeInfo aspeed_machine_types[] = {
> > > >       {
> > > >           .name          = MACHINE_TYPE_NAME("palmetto-bmc"),
> > > > @@ -1534,6 +1583,10 @@ static const TypeInfo aspeed_machine_types[] = {
> > > >           .name          = MACHINE_TYPE_NAME("qcom-dc-scm-v1-bmc"),
> > > >           .parent        = TYPE_ASPEED_MACHINE,
> > > >           .class_init    = aspeed_machine_qcom_dc_scm_v1_class_init,
> > > > +    }, {
> > > > +        .name          = MACHINE_TYPE_NAME("qcom-firework"),
> > >
> > > We should add the "-bmc" prefix to this machine name to be consistent
> > > with the other BMCs. A "qcom-firework" machine would model the whole
> > > system, host side included.
> >
> > Right, so I added the "-bmc" tag to "qcom-dc-scm-v1-bmc" as it's an
> > add-in card type board, and it can be attached to the "qcom-firework"
> > baseboard. The "qcom-firework" doesn't have the "-bmc" tag intentionally
> > since it doesn't have a bmc soc on it.
>
> These are the Aspeed machines, they only model the BMC side of the
> overall system.
>
> A "qcom-firework" machine would include the host SoC, possibly the
> service and management SoCs plus the BMC.
>
> As an example, see the fb35 machine being developed by Peter :
>
>    http://patchwork.ozlabs.org/project/qemu-devel/list/?series=306294
>
> or the PowerNV machines which use an embedded or external Aspeed BMCs

Okay. I'll add the '-bmc' suffix for the Firework's BMC part in this
series so that the 'qcom-firework' can be used for a machine that
has the host SoC. I'll submit v2 soon.

Thanks,
Jae

> Thanks,
>
> C.
>
> > Thanks,
> > Jae
> >
> > > Thanks,
> > >
> > > C.
> > >
> > > > +        .parent        = TYPE_ASPEED_MACHINE,
> > > > +        .class_init    = aspeed_machine_qcom_firework_class_init,
> > > >       }, {
> > > >           .name          = MACHINE_TYPE_NAME("fp5280g2-bmc"),
> > > >           .parent        = TYPE_ASPEED_MACHINE,