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

[Peter Delevoryas]

On Wed, Jul 27, 2022 at 12:05:58PM +0200, Cédric Le Goater wrote:
> On 7/5/22 21:13, 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 <peter@pjd.dev>
> > ---
> >   hw/arm/fby35.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 41 insertions(+)
> > 
> > diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c
> > index 03b458584c..5c5224d374 100644
> > --- a/hw/arm/fby35.c
> > +++ b/hw/arm/fby35.c
> > @@ -6,17 +6,55 @@
> >    */
> >   #include "qemu/osdep.h"
> > +#include "qemu/units.h"
> > +#include "qapi/error.h"
> > +#include "sysemu/sysemu.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;
> >   };
> > +#define FBY35_BMC_RAM_SIZE (2 * GiB)
> > +
> > +static void fby35_bmc_init(Fby35State *s)
> > +{
> > +    memory_region_init(&s->bmc_memory, OBJECT(s), "bmc-memory", 
> > UINT64_MAX);
> > +    memory_region_init_ram(&s->bmc_dram, OBJECT(s), "bmc-dram",
> > +                           FBY35_BMC_RAM_SIZE, &error_abort);
> 
> A MachineState object is used as a owner of the RAM region and this
> should assert in memory_region_init_ram() :
> 
>     /* This will assert if owner is neither NULL nor a DeviceState.
>      * We only want the owner here for the purposes of defining a
>      * unique name for migration. TODO: Ideally we should implement
>      * a naming scheme for Objects which are not DeviceStates, in
>      * which case we can relax this restriction.
>      */
>     owner_dev = DEVICE(owner);
> 
> It went unnoticed until I started experimenting with some MachineState
> modifications. CONFIG_QOM_CAST_DEBUG needs to be defined to catch the
> error. I would have thought that CI was doing this check. It seems not,
> which is surprising.

Hmmm! I see, didn't realize this was a requirement. Thanks for catching it!

> 
> Anyhow, this needs a fix for 7.1 and I will work on it.

I see, yes, thanks!!

> 
> C.
> 
> > +
> > +    object_initialize_child(OBJECT(s), "bmc", &s->bmc, "ast2600-a3");
> > +    object_property_set_int(OBJECT(&s->bmc), "ram-size", 
> > FBY35_BMC_RAM_SIZE,
> > +                            &error_abort);
> > +    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);
> > +    aspeed_soc_uart_set_chr(&s->bmc, ASPEED_DEV_UART5, serial_hd(0));
> > +    qdev_realize(DEVICE(&s->bmc), NULL, &error_abort);
> > +
> > +    aspeed_board_init_flashes(&s->bmc.fmc, "n25q00", 2, 0);
> > +}
> > +
> >   static void fby35_init(MachineState *machine)
> >   {
> > +    Fby35State *s = FBY35(machine);
> > +
> > +    fby35_bmc_init(s);
> >   }
> >   static void fby35_class_init(ObjectClass *oc, void *data)
> > @@ -25,6 +63,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[] = {
>