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:c...@kaod.org]
> Sent: Friday, May 17, 2019 12:08 PM
> To: Wim Vervoorn <wvervo...@eltan.com>; qemu-devel@nongnu.org;
> qemu-...@nongnu.org; Joel Stanley <j...@jms.id.au>; Andrew Jeffery
> <and...@aj.id.au>; Peter Maydell <peter.mayd...@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 <wvervo...@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
>>
>
>
