Re: [Qemu-arm] aspeed qemu question

[Cédric Le Goater]

Hello Win,

On 5/17/19 9:46 AM, Wim Vervoorn wrote:
> Hello Cedríc,
> 
> Thanks for your response. I created and attached the patch. You are right the 
> code snipped turns out unreadable.

You should try to send the patch directly and not attached. Checkout 
the git send-email commands for it. See also :

https://wiki.qemu.org/Contribute/SubmitAPatch

> In the patch I enable the MAC's depending on the value in hwstrap1 just as in 
> the real hardware. In the Palmetto both nics will be enabled (just as in the 
> real palmetto). 
> 
> I added a 2nd AST2400 BMC the Eltan Piestewa Peak. Here I enabled the 2nd NIC 
> only.
> 
> What I see is that when I use Palmetto I get an assert in net/net.c line 256, 
> this is in the "qemu_net_client_setup()". I assume I have to prepare 
> something regarding the host side of the network implementation but I this 
> point I have no clue what and I have a hard time figuring out how the 
> networking architecture really works. 

Did you define two nics on the command line  ?

more comments below.

[ ... ] 

> From bbf9392b84f38531b89e4ea39e06210b99ec7a0c Mon Sep 17 00:00:00 2001
> Message-Id: <address@hidden>
> From: Wim Vervoorn <address@hidden>
> Date: Fri, 17 May 2019 09:26:16 +0200
> Subject: [PATCH] ASPEED BMC: Add support for 2nd NIC
> 
> Add support for 2nd NIC.
> 
> This solution is using the hwstrap1 value to enable disable the
> controllers.

OK. Let see how this shows in the code.

> The Palmetto leaves both NICs enabled while the Piestewa Peak only
> enables one.

You should send two different patches, one for the NIC, one for Piestewa 
Peak machine. 

> The Palmetto asserts in net/net.c line 256 when qemu starts so something
> must be missing to support the 2nd nic.

You need a "signed-off-by:" tag here

> ---
>  hw/arm/aspeed.c             | 26 ++++++++++++++++++++++
>  hw/arm/aspeed_soc.c         | 54 
> ++++++++++++++++++++++++++++++++-------------
>  include/hw/arm/aspeed_soc.h |  2 +-
>  3 files changed, 66 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> old mode 100644
> new mode 100755
> index 0203642..f957a5b
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -43,6 +43,23 @@ struct AspeedBoardState {
>          SCU_AST2400_HW_STRAP_SET_CLK_SOURCE(AST2400_CLK_48M_IN) |       \
>          SCU_HW_STRAP_VGA_CLASS_CODE |                                   \
>          SCU_HW_STRAP_LPC_RESET_PIN |                                    \
> +        SCU_HW_STRAP_MAC0_RGMII |                                       \
> +        SCU_HW_STRAP_MAC1_RGMII |                                       \
> +        SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_M_S_EN) |                \
> +        SCU_AST2400_HW_STRAP_SET_CPU_AHB_RATIO(AST2400_CPU_AHB_RATIO_2_1) | \
> +        SCU_HW_STRAP_SPI_WIDTH |                                        \
> +        SCU_HW_STRAP_VGA_SIZE_SET(VGA_16M_DRAM) |                       \
> +        SCU_AST2400_HW_STRAP_BOOT_MODE(AST2400_SPI_BOOT))
> +
> +/* Piestewa Peak hardware value:  */
> +#define ELTANPWP_BMC_HW_STRAP1 (                                        \
> +        SCU_AST2400_HW_STRAP_DRAM_SIZE(DRAM_SIZE_256MB) |               \
> +        SCU_AST2400_HW_STRAP_DRAM_CONFIG(2 /* DDR3 with CL=6, CWL=5 */) | \
> +        SCU_AST2400_HW_STRAP_ACPI_DIS |                                 \
> +        SCU_AST2400_HW_STRAP_SET_CLK_SOURCE(AST2400_CLK_48M_IN) |       \
> +        SCU_HW_STRAP_VGA_CLASS_CODE |                                   \
> +        SCU_HW_STRAP_LPC_RESET_PIN |                                    \
> +        SCU_HW_STRAP_MAC1_RGMII |                                       \
>          SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_M_S_EN) |                \
>          SCU_AST2400_HW_STRAP_SET_CPU_AHB_RATIO(AST2400_CPU_AHB_RATIO_2_1) | \
>          SCU_HW_STRAP_SPI_WIDTH |                                        \
> @@ -423,6 +440,15 @@ static const AspeedBoardConfig aspeed_boards[] = {
>          .num_cs    = 1,
>          .i2c_init  = palmetto_bmc_i2c_init,
>      }, {
> +        .name      = MACHINE_TYPE_NAME("eltanpwp-bmc"),
> +        .desc      = "Eltan Piestewa Peak BMC (ARM926EJ-S)",
> +        .soc_name  = "ast2400-a1",
> +        .hw_strap1 = ELTANPWP_BMC_HW_STRAP1,
> +        .fmc_model = "n25q256a",
> +        .spi_model = "mx25l25635e",

Are these the correct flash models for your board ? 

> +        .num_cs    = 1,
> +        .i2c_init  = palmetto_bmc_i2c_init,
> +    }, {
>          .name      = MACHINE_TYPE_NAME("ast2500-evb"),
>          .desc      = "Aspeed AST2500 EVB (ARM1176)",
>          .soc_name  = "ast2500-a1",
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> old mode 100644
> new mode 100755
> index 0f6e5be..8ed7902
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -184,9 +184,13 @@ static void aspeed_soc_init(Object *obj)
>                                         OBJECT(&s->scu), &error_abort);
>      }
>  
> -    object_initialize(&s->ftgmac100, sizeof(s->ftgmac100), TYPE_FTGMAC100);
> -    object_property_add_child(obj, "ftgmac100", OBJECT(&s->ftgmac100), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->ftgmac100), sysbus_get_default());
> +    object_initialize(&s->ftgmac100[0], sizeof(s->ftgmac100), 
> TYPE_FTGMAC100);
> +    object_property_add_child(obj, "ftgmac100[0]", OBJECT(&s->ftgmac100[0]), 
> NULL);
> +    qdev_set_parent_bus(DEVICE(&s->ftgmac100[0]), sysbus_get_default());
> +
> +    object_initialize(&s->ftgmac100[1], sizeof(s->ftgmac100), 
> TYPE_FTGMAC100);
> +    object_property_add_child(obj, "ftgmac100[1]", OBJECT(&s->ftgmac100[1]), 
> NULL);
> +    qdev_set_parent_bus(DEVICE(&s->ftgmac100[1]), sysbus_get_default());

using a loop would be better. The future Aspeed SoCs might have more than two
MACs.
>  
>      object_initialize(&s->ibt, sizeof(s->ibt), TYPE_ASPEED_IBT);
>      object_property_add_child(obj, "bt", OBJECT(&s->ibt), NULL);
> @@ -384,19 +388,39 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
> **errp)
>                          ASPEED_SOC_WDT_BASE + i * 0x20);
>      }
>  
> -    /* Net */
> -    qdev_set_nic_properties(DEVICE(&s->ftgmac100), &nd_table[0]);
> -    object_property_set_bool(OBJECT(&s->ftgmac100), true, "aspeed", &err);
> -    object_property_set_bool(OBJECT(&s->ftgmac100), true, "realized",
> -                             &local_err);
> -    error_propagate(&err, local_err);
> -    if (err) {
> -        error_propagate(errp, err);
> -        return;
> +    /* Net LAN 0*/
> +        qdev_set_nic_properties(DEVICE(&s->ftgmac100[0]), &nd_table[0]);
> +        object_property_set_bool(OBJECT(&s->ftgmac100[0]), true, "aspeed", 
> &err);

This is not the correct indentation. Please run checkpatch.pl on the patch.

> +    if (s->scu.hw_strap1 & SCU_HW_STRAP_MAC0_RGMII) {
> +        qemu_log("%s: LAN0 enabled\n", __func__);

we don't need this kind of information.

> +        object_property_set_bool(OBJECT(&s->ftgmac100[0]), true, "realized",
> +                                &local_err);
> +        error_propagate(&err, local_err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[0]), 0, 
> ASPEED_SOC_ETH1_BASE);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100[0]), 0,
> +                        qdev_get_gpio_in(DEVICE(&s->vic), 2));
> +    }
> +
> +    /* Net LAN 1*/
> +        qdev_set_nic_properties(DEVICE(&s->ftgmac100[1]), &nd_table[1]);
> +        object_property_set_bool(OBJECT(&s->ftgmac100[1]), true, "aspeed", 
> &err);
> +    if (s->scu.hw_strap1 & SCU_HW_STRAP_MAC1_RGMII) {
> +        qemu_log("%s: LAN1 enabled\n", __func__);
> +        object_property_set_bool(OBJECT(&s->ftgmac100[1]), true, "realized",
> +                                &local_err);
> +        error_propagate(&err, local_err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[1]), 0, 
> ASPEED_SOC_ETH2_BASE);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100[1]), 0,
> +                        qdev_get_gpio_in(DEVICE(&s->vic), 3));

I would use a loop such as : 

    for (i = 0; i < nb_nics; i++) {
        NICInfo *nd = &nd_table[i];

        /* test SCU ... */

        ...
    }

to realize the NICs.

>      }
> -    sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100), 0, ASPEED_SOC_ETH1_BASE);
> -    sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100), 0,
> -                       qdev_get_gpio_in(DEVICE(&s->vic), 2));
>  
>      /* iBT */
>      object_property_set_bool(OBJECT(&s->ibt), true, "realized", &err);
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> old mode 100644
> new mode 100755
> index 623223d..0799d61
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -47,7 +47,7 @@ typedef struct AspeedSoCState {
>      AspeedSMCState spi[ASPEED_SPIS_NUM];
>      AspeedSDMCState sdmc;
>      AspeedWDTState wdt[ASPEED_WDTS_NUM];
> -    FTGMAC100State ftgmac100;
> +    FTGMAC100State ftgmac100[2];

2 needs a macro.


I have a patchset currently being reviewed which should help you
to define the different addresses, interrupt numbers of the MACS.
Let me ping you when this is merge and then you can rebase. 

However, you can send a patch for your board definition. There
should not be any conflicts with this addition.

Thanks,

C.




>      AspeedIBTState ibt;
>      AspeedGPIOState gpio;
>      AspeedPWMState pwm;
> -- 
> 2.7.4
>