Re: [PATCH v2 1/1] hw/arm/aspeed: Add fby35 machine type

[Peter Delevoryas <pdel@fb.com>]

> On May 25, 2022, at 1:41 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Hello Peter,
> 
>> On 5/4/22 00:59, Peter Delevoryas wrote:
>> Add the 'fby35-bmc' machine type based on the kernel DTS[1] and userspace
>> i2c setup scripts[2]. Undefined values are inherited from the AST2600-EVB.
>> Reference images can be found in Facebook OpenBMC Github Release assets
>> as "fby35.mtd". [3]
>> You can boot the reference images as follows (fby35 uses dual-flash):
>> qemu-system-arm -machine fby35-bmc \
>>     -drive file=fby35.mtd,format=raw,if=mtd \
>>     -drive file=fby35.mtd,format=raw,if=mtd \
>>     -nographic
>> [1] https://github.com/facebook/openbmc-linux/blob/412d5053258007117e94b1e36015aefc1301474b/arch/arm/boot/dts/aspeed-bmc-facebook-fby35.dts
>> [2] https://github.com/facebook/openbmc/blob/e2294ff5d31dd65c248fe396a385286d6d5c463d/meta-facebook/meta-fby35/recipes-fby35/plat-utils/files/setup-dev.sh
>> [3] https://github.com/facebook/openbmc/releases
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>> v2: Removed avocado test, updated commit message.
>>  hw/arm/aspeed.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 63 insertions(+)
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index a74c13ab0f..725c169488 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -21,6 +21,7 @@
>>  #include "hw/misc/led.h"
>>  #include "hw/qdev-properties.h"
>>  #include "sysemu/block-backend.h"
>> +#include "sysemu/reset.h"
>>  #include "hw/loader.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/units.h"
>> @@ -951,6 +952,35 @@ static void bletchley_bmc_i2c_init(AspeedMachineState *bmc)
>>      i2c_slave_create_simple(i2c[12], TYPE_PCA9552, 0x67);
>>  }
>>  +static void fby35_i2c_init(AspeedMachineState *bmc)
>> +{
>> +    AspeedSoCState *soc = &bmc->soc;
>> +    I2CBus *i2c[16];
>> +
>> +    for (int i = 0; i < 16; i++) {
>> +        i2c[i] = aspeed_i2c_get_bus(&soc->i2c, i);
>> +    }
>> +
>> +    i2c_slave_create_simple(i2c[2], TYPE_LM75, 0x4f);
>> +    i2c_slave_create_simple(i2c[8], TYPE_TMP421, 0x1f);
>> +    /* Hotswap controller is actually supposed to be mp5920 or ltc4282. */
>> +    i2c_slave_create_simple(i2c[11], "adm1272", 0x44);
>> +    i2c_slave_create_simple(i2c[12], TYPE_LM75, 0x4e);
>> +    i2c_slave_create_simple(i2c[12], TYPE_LM75, 0x4f);
>> +
>> +    aspeed_eeprom_init(i2c[4], 0x51, 128 * KiB);
>> +    aspeed_eeprom_init(i2c[6], 0x51, 128 * KiB);
>> +    aspeed_eeprom_init(i2c[8], 0x50, 32 * KiB);
>> +    aspeed_eeprom_init(i2c[11], 0x51, 128 * KiB);
>> +    aspeed_eeprom_init(i2c[11], 0x54, 128 * KiB);
>> +
>> +    /*
>> +     * TODO: There is a multi-master i2c connection to an AST1030 MiniBMC on
>> +     * buses 0, 1, 2, 3, and 9. Source address 0x10, target address 0x20 on
>> +     * each.
>> +     */
>> +}
>> +
>>  static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
>>  {
>>      return ASPEED_MACHINE(obj)->mmio_exec;
>> @@ -1293,6 +1323,35 @@ static void aspeed_machine_bletchley_class_init(ObjectClass *oc, void *data)
>>          aspeed_soc_num_cpus(amc->soc_name);
>>  }
>>  +static void fby35_reset(MachineState *state)
>> +{
>> +    AspeedMachineState *bmc = ASPEED_MACHINE(state);
>> +    AspeedGPIOState *gpio = &bmc->soc.gpio;
>> +
>> +    qemu_devices_reset();
>> +
>> +    /* Board ID */
>> +    object_property_set_bool(OBJECT(gpio), "gpioV4", true, &error_fatal);
>> +    object_property_set_bool(OBJECT(gpio), "gpioV5", true, &error_fatal);
>> +    object_property_set_bool(OBJECT(gpio), "gpioV6", true, &error_fatal);
>> +    object_property_set_bool(OBJECT(gpio), "gpioV7", false, &error_fatal);
>> +}
>> +
>> +static void aspeed_machine_fby35_class_init(ObjectClass *oc, void *data)
>> +{
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>> +
>> +    mc->desc       = "Facebook fby35 BMC (Cortex-A7)";
>> +    mc->reset      = fby35_reset;
>> +    amc->fmc_model = "mx66l1g45g";
>> +    amc->num_cs    = 2;
>> +    amc->macs_mask = ASPEED_MAC3_ON;
>> +    amc->i2c_init  = fby35_i2c_init;
>> +    /* FIXME: Replace this macro with something more general */
>> +    mc->default_ram_size = FUJI_BMC_RAM_SIZE;
>> +}
>> +
>>  #define AST1030_INTERNAL_FLASH_SIZE (1024 * 1024)
>>  /* Main SYSCLK frequency in Hz (200MHz) */
>>  #define SYSCLK_FRQ 200000000ULL
>> @@ -1411,6 +1470,10 @@ static const TypeInfo aspeed_machine_types[] = {
>>          .name          = MACHINE_TYPE_NAME("bletchley-bmc"),
>>          .parent        = TYPE_ASPEED_MACHINE,
>>          .class_init    = aspeed_machine_bletchley_class_init,
>> +    }, {
>> +        .name          = MACHINE_TYPE_NAME("fby35-bmc"),
>> +        .parent        = MACHINE_TYPE_NAME("ast2600-evb"),
> 
> Machine "fby35-bmc" inherits from the "ast2600-evb" machine definitions
> which means that any changes in "ast2600-evb" will silently modify
> "fby35-bmc". Is that what you want ?

When I wrote this, yes, that’s what I intended. i wanted to make the machine definition as simple as possible.

I don’t know what the HW straps should be, so I wanted to inherit them from the EVB.

 But thinking again, I’m not completely sure.

 I think for now, I’m ok with it, but if necessary, changes to the EVB should feel free to break things in this model. Once I know the HW strap values, maybe I could remove the dependency.

> 
> Thanks,
> 
> C.
> 
>> +        .class_init    = aspeed_machine_fby35_class_init,
>>      }, {
>>          .name           = MACHINE_TYPE_NAME("ast1030-evb"),
>>          .parent         = TYPE_ASPEED_MACHINE,
>