Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices

[Joel Stanley]

On Mon, 18 May 2020 at 12:19, Cédric Le Goater <address@hidden> wrote:
>
> On 5/18/20 7:03 AM, Markus Armbruster wrote:
> > These devices are optional, and controlled by @nb_nics.
> > aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum
> > supported number.  aspeed_soc_ast2600_realize() and
> > aspeed_soc_realize() realize only the wanted number.  Works, although
> > it can leave unrealized devices hanging around in the QOM composition
> > tree.  Affects machines ast2500-evb, ast2600-evb, palmetto-bmc,
> > romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc.
> >
> > Make the init functions create only the wanted ones.  Visible in "info
> > qom-tree"; here's the change for ast2600-evb:
> >
> >      /machine (ast2600-evb-machine)
> >        [...]
> >        /soc (ast2600-a1)
> >          [...]
> >          /ftgmac100[0] (ftgmac100)
> >            /ftgmac100[0] (qemu:memory-region)
> >     -    /ftgmac100[1] (ftgmac100)
> >     -    /ftgmac100[2] (ftgmac100)
> >     -    /ftgmac100[3] (ftgmac100)
> >          /gpio (aspeed.gpio-ast2600)
> >          [...]
> >          /mii[0] (aspeed-mmi)
> >            /aspeed-mmi[0] (qemu:memory-region)
> >     -    /mii[1] (aspeed-mmi)
> >     -    /mii[2] (aspeed-mmi)
> >     -    /mii[3] (aspeed-mmi)
> >          /rtc (aspeed.rtc)
> >
> > I'm not sure creating @nb_nics devices makes sense.  How many does the
> > physical chip provide?
>
> The AST2400, AST2500 SoC have 2 macs and the AST2600 has 4. Each machine
> define the one it uses, generally MAC0 but the tacoma board uses MAC3.
>
> Shouldn't the model reflect the real address space independently from
> the NIC backends defined on the command line ?

Agreed, the MAC hardware is present in all instances of the AST2600,
so they should be present in qemu. Only some boards wire up a network
device to the other side.

It would be advantageous for us to be able to specify which device is
being connected to on the command line. Currently we do this by
connecting all devices up to the one we care about which is an ugly
workaround.

> How should we proceed in such cases ?
>
> Thanks,
>
> C.
>
> >
> > Cc: "Cédric Le Goater" <address@hidden>
> > Cc: Peter Maydell <address@hidden>
> > Cc: Andrew Jeffery <address@hidden>
> > Cc: Joel Stanley <address@hidden>
> > Cc: address@hidden
> > Signed-off-by: Markus Armbruster <address@hidden>
> > ---
> >  hw/arm/aspeed_ast2600.c | 2 +-
> >  hw/arm/aspeed_soc.c     | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index 71a0acfe26..0a6a77dd54 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -188,7 +188,7 @@ static void aspeed_soc_ast2600_init(Object *obj)
> >                                sizeof(s->wdt[i]), typename);
> >      }
> >
> > -    for (i = 0; i < sc->macs_num; i++) {
> > +    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
> >          sysbus_init_child_obj(obj, "ftgmac100[*]", 
> > OBJECT(&s->ftgmac100[i]),
> >                                sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
> >
> > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> > index cf6b6dd116..7ca860392a 100644
> > --- a/hw/arm/aspeed_soc.c
> > +++ b/hw/arm/aspeed_soc.c
> > @@ -203,7 +203,7 @@ static void aspeed_soc_init(Object *obj)
> >                                sizeof(s->wdt[i]), typename);
> >      }
> >
> > -    for (i = 0; i < sc->macs_num; i++) {
> > +    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
> >          sysbus_init_child_obj(obj, "ftgmac100[*]", 
> > OBJECT(&s->ftgmac100[i]),
> >                                sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
> >      }
> >
>