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

[Cédric Le Goater]

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 ?  

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);
>      }
>