Re: [Qemu-devel] aspeed qemu question

["Cédric Le Goater" <clg@kaod.org>]

Hello Wim,

On 5/20/19 9:42 AM, Wim Vervoorn wrote:
> Hello Cédric,
> 
> It was not yet my intention to have this patch included in the qemu repo so I didn't pay attention to checking the indentation etc. So I misunderstood your suggestion. I am sorry about that.
> 
> I will address your remarks in a new patch.
> 
> Besides this I have another question.
> 
> First of all I don't think qemu should assert due to a lacking NIC definition on the commandline so that is why I think I am missing something.

I think there is another problem. 

In hw/net/ftgmac100.c, this line in ftgmac100_realize() should be removed :

	s->conf.peers.ncs[0] = nd_table[0].netdev;

I will send a fix.


> Secondly I have started qemu with various attempts to define a 2nd NIC all with the same result (an assert). The issue with this is that I am not 100% sure I am defining the NICs in the correct way. If you could provide me with a commandline that is known to be correct I can use that for testing.

You could use two devices such as : 

 -net nic,macaddr=C0:FF:EE:00:00:02,model=ftgmac100 -net user,id=netdev0 
 -net nic,macaddr=C0:FF:EE:00:00:04,model=ftgmac100 -net user,id=netdev1

Providing you have the correct DT, you should see something like :

[    3.628410] libphy: Fixed MDIO Bus: probed
[    3.631462] ftgmac100 1e660000.ethernet: Read MAC address c0:ff:ee:00:00:02 from chip
[    3.631835] ftgmac100 1e660000.ethernet: Using NCSI interface
[    3.646027] ftgmac100 1e660000.ethernet eth0: irq 19, mapped at (ptrval)
[    3.647308] ftgmac100 1e680000.ethernet: Read MAC address c0:ff:ee:00:00:04 from chip
[    4.120223] libphy: ftgmac100_mdio: probed
[    4.121590] RTL8211E Gigabit Ethernet 1e680000.ethernet--1:00: attached PHY driver [RTL8211E Gigabit Ethernet] (mii_bus:phy_addr=1e680000.ethernet--1:00, irq=POLL)
[    4.134884] ftgmac100 1e680000.ethernet eth1: irq 20, mapped at (ptrval)

Regards,

C.



> Thanks for you patience.
> 
> Best Regards,
> Wim Vervoorn
> 
> 
> -----Original Message-----
> From: Cédric Le Goater [mailto:clg@kaod.org] 
> Sent: Friday, May 17, 2019 12:08 PM
> To: Wim Vervoorn <wvervoorn@eltan.com>; qemu-devel@nongnu.org; qemu-arm@nongnu.org; Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Peter Maydell <peter.maydell@linaro.org>
> Subject: Re: aspeed qemu question
> 
> 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: 
>> <bbf9392b84f38531b89e4ea39e06210b99ec7a0c.1558078463.git.wvervoorn@elt
>> an.com>
>> From: Wim Vervoorn <wvervoorn@eltan.com>
>> 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
>>
> 
>