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

[Cédric Le Goater]

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);
      |                              ~~~~~~~~^~~~~


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

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

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

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[] = {