Re: [PATCH v2 8/8] aspeed: Add AST2600 (BMC) to fby35

[Peter Delevoryas <pdel@fb.com>]

> On Jun 27, 2022, at 10:01 PM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> On 6/24/22 02:37, Peter Delevoryas wrote:
>> You can test booting the BMC with both '-device loader' and '-drive
>> file'. This is necessary because of how the fb-openbmc boot sequence
>> works (jump to 0x20000000 after U-Boot SPL).
>> 
>>     wget https://github.com/facebook/openbmc/releases/download/openbmc-e2294ff5d31d/fby35.mtd
>>     qemu-system-arm -machine fby35 -nographic \
>>         -device loader,file=fby35.mtd,addr=0,cpu-num=0 -drive file=fby35.mtd,format=raw,if=mtd
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>>  hw/arm/fby35.c | 39 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>> diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c
>> index dc1ae8e62a..1e24cbf3f8 100644
>> --- a/hw/arm/fby35.c
>> +++ b/hw/arm/fby35.c
>> @@ -21,17 +21,53 @@
>>   */
>>    #include "qemu/osdep.h"
>> +#include "qemu/units.h"
>> +#include "qapi/error.h"
>>  #include "hw/boards.h"
>> +#include "hw/arm/aspeed_soc.h"
>>    #define TYPE_FBY35 MACHINE_TYPE_NAME("fby35")
>>  OBJECT_DECLARE_SIMPLE_TYPE(Fby35State, FBY35);
>>    struct Fby35State {
>>      MachineState parent_obj;
>> +
>> +    MemoryRegion bmc_memory;
>> +    MemoryRegion bmc_dram;
>> +    MemoryRegion bmc_boot_rom;
>> +
>> +    AspeedSoCState bmc;
>>  };
>>  +static void fby35_bmc_init(Fby35State *s)
>> +{
>> +    uint32_t boot_rom_size;
>> +
>> +    memory_region_init(&s->bmc_memory, OBJECT(s), "bmc-memory", UINT64_MAX);
>> +    memory_region_init_ram(&s->bmc_dram, OBJECT(s), "bmc-dram", 2 * GiB, &error_abort);
>> +
>> +    object_initialize_child(OBJECT(s), "bmc", &s->bmc, "ast2600-a3");
>> +    object_property_set_int(OBJECT(&s->bmc), "ram-size", s->bmc_dram.size, &error_abort);
> 
> This fails to compile on some platforms :
> 
> ./hw/arm/fby35.c: In function ‘fby35_bmc_init’:
> ../hw/arm/fby35.c:50:69: error: incompatible type for argument 3 of ‘object_property_set_int’
>   50 |     object_property_set_int(OBJECT(&s->bmc), "ram-size", s->bmc_dram.size, &error_abort);
>      |                                                          ~~~~~~~~~~~^~~~~
>      |                                                                     |
>      |                                                                     Int128
> In file included from /builds/legoater/qemu/include/exec/memory.h:28,
>                 from /builds/legoater/qemu/include/hw/boards.h:6,
>                 from ../hw/arm/fby35.c:26:
> /builds/legoater/qemu/include/qom/object.h:1342:38: note: expected ‘int64_t’ {aka ‘long long int’} but argument is of type ‘Int128’
> 1342 |                              int64_t value, Error **errp);
>      |                              ~~~~~~~~^~~~~
> 
> 

Oh yikes, I’ll fix this. Hopefully without casting from Int128 to int64_t or defining a new constant.

> You don't need to resend the patches 1-7. I have pulled them in my branch
> for the next PR.

Got it, I’ll just resend this one as v3 then.

> 
>> +    object_property_set_link(OBJECT(&s->bmc), "memory", OBJECT(&s->bmc_memory), &error_abort);
>> +    object_property_set_link(OBJECT(&s->bmc), "dram", OBJECT(&s->bmc_dram), &error_abort);
>> +    object_property_set_int(OBJECT(&s->bmc), "hw-strap1", 0x000000C0, &error_abort);
>> +    object_property_set_int(OBJECT(&s->bmc), "hw-strap2", 0x00000003, &error_abort);
> 
> We could share common definitions with the BMC machine in .h file.

Good point, I’ll put then in aspeed.h then.

> 
>> +    object_property_set_int(OBJECT(&s->bmc), "uart-default", ASPEED_DEV_UART5, &error_abort);
>> +    qdev_realize(DEVICE(&s->bmc), NULL, &error_abort);
>> +
>> +    boot_rom_size = ASPEED_SMC_GET_CLASS(&s->bmc.fmc)->segments[0].size;
>> +
>> +    memory_region_init_rom(&s->bmc_boot_rom, OBJECT(s), "bmc-boot-rom", boot_rom_size, &error_abort);
>> +    memory_region_add_subregion(&s->bmc_memory, 0, &s->bmc_boot_rom);
>> +
>> +    aspeed_board_init_flashes(&s->bmc.fmc, "n25q00", 2, 0);
> 
> I am not totally convinced with the ROM because it complexifies how the
> machine is started but I haven't tried the other way using the loader
> either. It is my TODO list.

Yep, totally agree: if I can fix this soon I’ll send out a fix or something, otherwise
feel free to submit a change of your own.

Thanks for the review!

> 
> Thanks,
> 
> C.
> 
>> +}
>> +
>>  static void fby35_init(MachineState *machine)
>>  {
>> +    Fby35State *s = FBY35(machine);
>> +
>> +    fby35_bmc_init(s);
>>  }
>>    static void fby35_class_init(ObjectClass *oc, void *data)
>> @@ -40,6 +76,9 @@ static void fby35_class_init(ObjectClass *oc, void *data)
>>        mc->desc = "Meta Platforms fby35";
>>      mc->init = fby35_init;
>> +    mc->no_floppy = 1;
>> +    mc->no_cdrom = 1;
>> +    mc->min_cpus = mc->max_cpus = mc->default_cpus = 2;
>>  }
>>    static const TypeInfo fby35_types[] = {
>