* [PATCH] arm/aspeed: Rework NIC attachment
@ 2020-05-19 16:19 Cédric Le Goater
2020-05-20 6:34 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Cédric Le Goater @ 2020-05-19 16:19 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, qemu-devel, Markus Armbruster, qemu-arm,
Joel Stanley, Philippe Mathieu-Daudé, Cédric Le Goater
The AST2400 and AST2500 SoCs have two MACs but only the first MAC0 is
active on the Aspeed machines using these SoCs. The AST2600 has four
MACs. The AST2600 EVB machine activates MAC1, MAC2 and MAC3 and the
Tacoma BMC machine activates MAC2.
Introduce a bit-field property "macs-mask" under the Aspeed SoC model
to link the active MACs of the machine being started with the available
network devices.
Inactive MACs will have no peer and QEMU will warn the user with :
qemu-system-arm: warning: nic ftgmac100.0 has no peer
qemu-system-arm: warning: nic ftgmac100.1 has no peer
qemu-system-arm: warning: nic ftgmac100.3 has no peer
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
To be applied on top of patch "arm/aspeed: Compute the number of CPUs
from the SoC definition"
http://patchwork.ozlabs.org/project/qemu-devel/patch/20200519091631.1006073-1-clg@kaod.org/
include/hw/arm/aspeed.h | 1 +
include/hw/arm/aspeed_soc.h | 6 ++++++
hw/arm/aspeed.c | 6 ++++++
hw/arm/aspeed_ast2600.c | 11 ++++++++---
hw/arm/aspeed_soc.c | 10 ++++++++--
5 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index 18521484b90e..842dff485f5b 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -39,6 +39,7 @@ typedef struct AspeedMachineClass {
const char *fmc_model;
const char *spi_model;
uint32_t num_cs;
+ uint32_t macs_mask;
void (*i2c_init)(AspeedBoardState *bmc);
} AspeedMachineClass;
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 914115f3ef77..fdb9e05bc47c 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -34,6 +34,11 @@
#define ASPEED_CPUS_NUM 2
#define ASPEED_MACS_NUM 4
+#define ASPEED_MAC0_ON (1 << 0)
+#define ASPEED_MAC1_ON (1 << 1)
+#define ASPEED_MAC2_ON (1 << 2)
+#define ASPEED_MAC3_ON (1 << 3)
+
typedef struct AspeedSoCState {
/*< private >*/
DeviceState parent;
@@ -55,6 +60,7 @@ typedef struct AspeedSoCState {
AspeedSDMCState sdmc;
AspeedWDTState wdt[ASPEED_WDTS_NUM];
FTGMAC100State ftgmac100[ASPEED_MACS_NUM];
+ uint32_t macs_mask;
AspeedMiiState mii[ASPEED_MACS_NUM];
AspeedGPIOState gpio;
AspeedGPIOState gpio_1_8v;
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6f8f4b88f8ab..79c683864d7e 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -283,6 +283,8 @@ static void aspeed_machine_init(MachineState *machine)
&error_abort);
object_property_set_int(OBJECT(&bmc->soc), amc->num_cs, "num-cs",
&error_abort);
+ object_property_set_int(OBJECT(&bmc->soc), amc->macs_mask, "macs-mask",
+ &error_abort);
object_property_set_link(OBJECT(&bmc->soc), OBJECT(&bmc->ram_container),
"dram", &error_abort);
if (machine->kernel_filename) {
@@ -556,12 +558,14 @@ static int aspeed_soc_num_cpus(const char *soc_name)
static void aspeed_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
+ AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
mc->init = aspeed_machine_init;
mc->no_floppy = 1;
mc->no_cdrom = 1;
mc->no_parallel = 1;
mc->default_ram_id = "ram";
+ amc->macs_mask = ASPEED_MAC0_ON;
aspeed_machine_class_props_init(oc);
}
@@ -680,6 +684,7 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
amc->fmc_model = "w25q512jv";
amc->spi_model = "mx66u51235f";
amc->num_cs = 1;
+ amc->macs_mask = ASPEED_MAC1_ON | ASPEED_MAC2_ON | ASPEED_MAC3_ON;
amc->i2c_init = ast2600_evb_i2c_init;
mc->default_ram_size = 1 * GiB;
mc->default_cpus = mc->min_cpus = mc->max_cpus =
@@ -698,6 +703,7 @@ static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data)
amc->fmc_model = "mx66l1g45g";
amc->spi_model = "mx66l1g45g";
amc->num_cs = 2;
+ amc->macs_mask = ASPEED_MAC2_ON;
amc->i2c_init = witherspoon_bmc_i2c_init; /* Same board layout */
mc->default_ram_size = 1 * GiB;
mc->default_cpus = mc->min_cpus = mc->max_cpus =
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 114b94f8f44d..fa85122f6d78 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -247,6 +247,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
Error *err = NULL, *local_err = NULL;
qemu_irq irq;
+ NICInfo *nd = &nd_table[0];
/* IO space */
create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM],
@@ -462,8 +463,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
}
/* Net */
- for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
- qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]);
+ for (i = 0; i < sc->macs_num; i++) {
+ if ((s->macs_mask & (1 << i)) && nd->used) {
+ qemu_check_nic_model(nd, TYPE_FTGMAC100);
+ qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd);
+ nd++;
+ }
object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
&err);
object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized",
@@ -471,7 +476,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
error_propagate(&err, local_err);
if (err) {
error_propagate(errp, err);
- return;
+ return;
}
sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
sc->memmap[ASPEED_ETH1 + i]);
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 984d29087dce..d2c6a5760790 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -234,6 +234,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
AspeedSoCState *s = ASPEED_SOC(dev);
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
Error *err = NULL, *local_err = NULL;
+ NICInfo *nd = &nd_table[0];
/* IO space */
create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM],
@@ -405,8 +406,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
}
/* Net */
- for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
- qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]);
+ for (i = 0; i < sc->macs_num; i++) {
+ if ((s->macs_mask & (1 << i)) && nd->used) {
+ qemu_check_nic_model(nd, TYPE_FTGMAC100);
+ qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd);
+ nd++;
+ }
object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
&err);
object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized",
@@ -455,6 +460,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
aspeed_soc_get_irq(s, ASPEED_SDHCI));
}
static Property aspeed_soc_properties[] = {
+ DEFINE_PROP_UINT32("macs-mask", AspeedSoCState, macs_mask, 0x1),
DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
MemoryRegion *),
DEFINE_PROP_END_OF_LIST(),
--
2.25.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] arm/aspeed: Rework NIC attachment 2020-05-19 16:19 [PATCH] arm/aspeed: Rework NIC attachment Cédric Le Goater @ 2020-05-20 6:34 ` Markus Armbruster 2020-05-20 7:01 ` Cédric Le Goater 0 siblings, 1 reply; 6+ messages in thread From: Markus Armbruster @ 2020-05-20 6:34 UTC (permalink / raw) To: Cédric Le Goater Cc: Peter Maydell, Andrew Jeffery, qemu-devel, Markus Armbruster, qemu-arm, Joel Stanley, Philippe Mathieu-Daudé Cédric Le Goater <clg@kaod.org> writes: > The AST2400 and AST2500 SoCs have two MACs but only the first MAC0 is > active on the Aspeed machines using these SoCs. The AST2600 has four > MACs. The AST2600 EVB machine activates MAC1, MAC2 and MAC3 and the > Tacoma BMC machine activates MAC2. > > Introduce a bit-field property "macs-mask" under the Aspeed SoC model > to link the active MACs of the machine being started with the available > network devices. > > Inactive MACs will have no peer and QEMU will warn the user with : > > qemu-system-arm: warning: nic ftgmac100.0 has no peer > qemu-system-arm: warning: nic ftgmac100.1 has no peer > qemu-system-arm: warning: nic ftgmac100.3 has no peer I can't reproduce this warning. What's your exact command line? > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > > To be applied on top of patch "arm/aspeed: Compute the number of CPUs > from the SoC definition" > > http://patchwork.ozlabs.org/project/qemu-devel/patch/20200519091631.1006073-1-clg@kaod.org/ > > include/hw/arm/aspeed.h | 1 + > include/hw/arm/aspeed_soc.h | 6 ++++++ > hw/arm/aspeed.c | 6 ++++++ > hw/arm/aspeed_ast2600.c | 11 ++++++++--- > hw/arm/aspeed_soc.c | 10 ++++++++-- > 5 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h > index 18521484b90e..842dff485f5b 100644 > --- a/include/hw/arm/aspeed.h > +++ b/include/hw/arm/aspeed.h > @@ -39,6 +39,7 @@ typedef struct AspeedMachineClass { > const char *fmc_model; > const char *spi_model; > uint32_t num_cs; > + uint32_t macs_mask; > void (*i2c_init)(AspeedBoardState *bmc); > } AspeedMachineClass; > > diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h > index 914115f3ef77..fdb9e05bc47c 100644 > --- a/include/hw/arm/aspeed_soc.h > +++ b/include/hw/arm/aspeed_soc.h > @@ -34,6 +34,11 @@ > #define ASPEED_CPUS_NUM 2 > #define ASPEED_MACS_NUM 4 > > +#define ASPEED_MAC0_ON (1 << 0) > +#define ASPEED_MAC1_ON (1 << 1) > +#define ASPEED_MAC2_ON (1 << 2) > +#define ASPEED_MAC3_ON (1 << 3) > + > typedef struct AspeedSoCState { > /*< private >*/ > DeviceState parent; > @@ -55,6 +60,7 @@ typedef struct AspeedSoCState { > AspeedSDMCState sdmc; > AspeedWDTState wdt[ASPEED_WDTS_NUM]; > FTGMAC100State ftgmac100[ASPEED_MACS_NUM]; > + uint32_t macs_mask; What's the purpose of this member? When and how would it be different from AspeedMachineClass's macs_mask? > AspeedMiiState mii[ASPEED_MACS_NUM]; > AspeedGPIOState gpio; > AspeedGPIOState gpio_1_8v; > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index 6f8f4b88f8ab..79c683864d7e 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -283,6 +283,8 @@ static void aspeed_machine_init(MachineState *machine) > &error_abort); > object_property_set_int(OBJECT(&bmc->soc), amc->num_cs, "num-cs", > &error_abort); > + object_property_set_int(OBJECT(&bmc->soc), amc->macs_mask, "macs-mask", > + &error_abort); > object_property_set_link(OBJECT(&bmc->soc), OBJECT(&bmc->ram_container), > "dram", &error_abort); > if (machine->kernel_filename) { > @@ -556,12 +558,14 @@ static int aspeed_soc_num_cpus(const char *soc_name) > static void aspeed_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc); > > mc->init = aspeed_machine_init; > mc->no_floppy = 1; > mc->no_cdrom = 1; > mc->no_parallel = 1; > mc->default_ram_id = "ram"; > + amc->macs_mask = ASPEED_MAC0_ON; > > aspeed_machine_class_props_init(oc); > } > @@ -680,6 +684,7 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data) > amc->fmc_model = "w25q512jv"; > amc->spi_model = "mx66u51235f"; > amc->num_cs = 1; > + amc->macs_mask = ASPEED_MAC1_ON | ASPEED_MAC2_ON | ASPEED_MAC3_ON; > amc->i2c_init = ast2600_evb_i2c_init; > mc->default_ram_size = 1 * GiB; > mc->default_cpus = mc->min_cpus = mc->max_cpus = > @@ -698,6 +703,7 @@ static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data) > amc->fmc_model = "mx66l1g45g"; > amc->spi_model = "mx66l1g45g"; > amc->num_cs = 2; > + amc->macs_mask = ASPEED_MAC2_ON; > amc->i2c_init = witherspoon_bmc_i2c_init; /* Same board layout */ > mc->default_ram_size = 1 * GiB; > mc->default_cpus = mc->min_cpus = mc->max_cpus = > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c > index 114b94f8f44d..fa85122f6d78 100644 > --- a/hw/arm/aspeed_ast2600.c > +++ b/hw/arm/aspeed_ast2600.c > @@ -247,6 +247,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > Error *err = NULL, *local_err = NULL; > qemu_irq irq; > + NICInfo *nd = &nd_table[0]; > > /* IO space */ > create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM], > @@ -462,8 +463,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) > } > > /* Net */ > - for (i = 0; i < nb_nics && i < sc->macs_num; i++) { > - qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]); > + for (i = 0; i < sc->macs_num; i++) { > + if ((s->macs_mask & (1 << i)) && nd->used) { > + qemu_check_nic_model(nd, TYPE_FTGMAC100); > + qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd); > + nd++; > + } > object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed", > &err); > object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized", > @@ -471,7 +476,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) > error_propagate(&err, local_err); > if (err) { > error_propagate(errp, err); > - return; > + return; > } > sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0, > sc->memmap[ASPEED_ETH1 + i]); > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c > index 984d29087dce..d2c6a5760790 100644 > --- a/hw/arm/aspeed_soc.c > +++ b/hw/arm/aspeed_soc.c > @@ -234,6 +234,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) > AspeedSoCState *s = ASPEED_SOC(dev); > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > Error *err = NULL, *local_err = NULL; > + NICInfo *nd = &nd_table[0]; > > /* IO space */ > create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM], > @@ -405,8 +406,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) > } > > /* Net */ > - for (i = 0; i < nb_nics && i < sc->macs_num; i++) { > - qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]); > + for (i = 0; i < sc->macs_num; i++) { > + if ((s->macs_mask & (1 << i)) && nd->used) { > + qemu_check_nic_model(nd, TYPE_FTGMAC100); > + qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd); > + nd++; > + } > object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed", > &err); > object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized", > @@ -455,6 +460,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) > aspeed_soc_get_irq(s, ASPEED_SDHCI)); > } > static Property aspeed_soc_properties[] = { > + DEFINE_PROP_UINT32("macs-mask", AspeedSoCState, macs_mask, 0x1), > DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION, > MemoryRegion *), > DEFINE_PROP_END_OF_LIST(), ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm/aspeed: Rework NIC attachment 2020-05-20 6:34 ` Markus Armbruster @ 2020-05-20 7:01 ` Cédric Le Goater 2020-05-20 15:43 ` Markus Armbruster 0 siblings, 1 reply; 6+ messages in thread From: Cédric Le Goater @ 2020-05-20 7:01 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Andrew Jeffery, qemu-devel, qemu-arm, Joel Stanley, Philippe Mathieu-Daudé On 5/20/20 8:34 AM, Markus Armbruster wrote: > Cédric Le Goater <clg@kaod.org> writes: > >> The AST2400 and AST2500 SoCs have two MACs but only the first MAC0 is >> active on the Aspeed machines using these SoCs. The AST2600 has four >> MACs. The AST2600 EVB machine activates MAC1, MAC2 and MAC3 and the >> Tacoma BMC machine activates MAC2. >> >> Introduce a bit-field property "macs-mask" under the Aspeed SoC model >> to link the active MACs of the machine being started with the available >> network devices. >> >> Inactive MACs will have no peer and QEMU will warn the user with : >> >> qemu-system-arm: warning: nic ftgmac100.0 has no peer >> qemu-system-arm: warning: nic ftgmac100.1 has no peer >> qemu-system-arm: warning: nic ftgmac100.3 has no peer > > I can't reproduce this warning. What's your exact command line? Get a witherspoon-tacoma flash image : $ wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,label=builder,target=witherspoon-tacoma/lastSuccessfulBuild/artifact/deploy/images/witherspoon-tacoma/flash-witherspoon-tacoma Run : $ qemu-system-arm -M tacoma-bmc -nic user -drive file=./flash-witherspoon-tacoma,format=raw,if=mtd -nographic -nodefaults -serial mon:stdio qemu-system-arm: warning: nic ftgmac100.0 has no peer qemu-system-arm: warning: nic ftgmac100.1 has no peer qemu-system-arm: warning: nic ftgmac100.3 has no peer > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> >> To be applied on top of patch "arm/aspeed: Compute the number of CPUs >> from the SoC definition" >> >> http://patchwork.ozlabs.org/project/qemu-devel/patch/20200519091631.1006073-1-clg@kaod.org/ >> >> include/hw/arm/aspeed.h | 1 + >> include/hw/arm/aspeed_soc.h | 6 ++++++ >> hw/arm/aspeed.c | 6 ++++++ >> hw/arm/aspeed_ast2600.c | 11 ++++++++--- >> hw/arm/aspeed_soc.c | 10 ++++++++-- >> 5 files changed, 29 insertions(+), 5 deletions(-) >> >> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h >> index 18521484b90e..842dff485f5b 100644 >> --- a/include/hw/arm/aspeed.h >> +++ b/include/hw/arm/aspeed.h >> @@ -39,6 +39,7 @@ typedef struct AspeedMachineClass { >> const char *fmc_model; >> const char *spi_model; >> uint32_t num_cs; >> + uint32_t macs_mask; >> void (*i2c_init)(AspeedBoardState *bmc); >> } AspeedMachineClass; >> >> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h >> index 914115f3ef77..fdb9e05bc47c 100644 >> --- a/include/hw/arm/aspeed_soc.h >> +++ b/include/hw/arm/aspeed_soc.h >> @@ -34,6 +34,11 @@ >> #define ASPEED_CPUS_NUM 2 >> #define ASPEED_MACS_NUM 4 >> >> +#define ASPEED_MAC0_ON (1 << 0) >> +#define ASPEED_MAC1_ON (1 << 1) >> +#define ASPEED_MAC2_ON (1 << 2) >> +#define ASPEED_MAC3_ON (1 << 3) >> + >> typedef struct AspeedSoCState { >> /*< private >*/ >> DeviceState parent; >> @@ -55,6 +60,7 @@ typedef struct AspeedSoCState { >> AspeedSDMCState sdmc; >> AspeedWDTState wdt[ASPEED_WDTS_NUM]; >> FTGMAC100State ftgmac100[ASPEED_MACS_NUM]; >> + uint32_t macs_mask; > > What's the purpose of this member? When and how would it be different > from AspeedMachineClass's macs_mask? Each machine activates a different set of MACs even if using the same SoC. So, the SoC macs_mask is overiden when the machine initializes the SoC in aspeed_machine_init(). That said, I think the default SoC macs_mask should be all MACS, a value of 0xFFFFFFFF would be fine, and not only the first MAC as this patch does. > >> AspeedMiiState mii[ASPEED_MACS_NUM]; >> AspeedGPIOState gpio; >> AspeedGPIOState gpio_1_8v; >> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >> index 6f8f4b88f8ab..79c683864d7e 100644 >> --- a/hw/arm/aspeed.c >> +++ b/hw/arm/aspeed.c >> @@ -283,6 +283,8 @@ static void aspeed_machine_init(MachineState *machine) >> &error_abort); >> object_property_set_int(OBJECT(&bmc->soc), amc->num_cs, "num-cs", >> &error_abort); >> + object_property_set_int(OBJECT(&bmc->soc), amc->macs_mask, "macs-mask", >> + &error_abort); >> object_property_set_link(OBJECT(&bmc->soc), OBJECT(&bmc->ram_container), >> "dram", &error_abort); >> if (machine->kernel_filename) { >> @@ -556,12 +558,14 @@ static int aspeed_soc_num_cpus(const char *soc_name) >> static void aspeed_machine_class_init(ObjectClass *oc, void *data) >> { >> MachineClass *mc = MACHINE_CLASS(oc); >> + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc); >> >> mc->init = aspeed_machine_init; >> mc->no_floppy = 1; >> mc->no_cdrom = 1; >> mc->no_parallel = 1; >> mc->default_ram_id = "ram"; >> + amc->macs_mask = ASPEED_MAC0_ON; >> >> aspeed_machine_class_props_init(oc); >> } >> @@ -680,6 +684,7 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data) >> amc->fmc_model = "w25q512jv"; >> amc->spi_model = "mx66u51235f"; >> amc->num_cs = 1; >> + amc->macs_mask = ASPEED_MAC1_ON | ASPEED_MAC2_ON | ASPEED_MAC3_ON; >> amc->i2c_init = ast2600_evb_i2c_init; >> mc->default_ram_size = 1 * GiB; >> mc->default_cpus = mc->min_cpus = mc->max_cpus = >> @@ -698,6 +703,7 @@ static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data) >> amc->fmc_model = "mx66l1g45g"; >> amc->spi_model = "mx66l1g45g"; >> amc->num_cs = 2; >> + amc->macs_mask = ASPEED_MAC2_ON; >> amc->i2c_init = witherspoon_bmc_i2c_init; /* Same board layout */ >> mc->default_ram_size = 1 * GiB; >> mc->default_cpus = mc->min_cpus = mc->max_cpus = >> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c >> index 114b94f8f44d..fa85122f6d78 100644 >> --- a/hw/arm/aspeed_ast2600.c >> +++ b/hw/arm/aspeed_ast2600.c >> @@ -247,6 +247,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); >> Error *err = NULL, *local_err = NULL; >> qemu_irq irq; >> + NICInfo *nd = &nd_table[0]; >> >> /* IO space */ >> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM], >> @@ -462,8 +463,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >> } >> >> /* Net */ >> - for (i = 0; i < nb_nics && i < sc->macs_num; i++) { >> - qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]); >> + for (i = 0; i < sc->macs_num; i++) { >> + if ((s->macs_mask & (1 << i)) && nd->used) { >> + qemu_check_nic_model(nd, TYPE_FTGMAC100); >> + qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd); >> + nd++; >> + } >> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed", >> &err); >> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized", >> @@ -471,7 +476,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >> error_propagate(&err, local_err); >> if (err) { >> error_propagate(errp, err); >> - return; >> + return; >> } >> sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0, >> sc->memmap[ASPEED_ETH1 + i]); >> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c >> index 984d29087dce..d2c6a5760790 100644 >> --- a/hw/arm/aspeed_soc.c >> +++ b/hw/arm/aspeed_soc.c >> @@ -234,6 +234,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >> AspeedSoCState *s = ASPEED_SOC(dev); >> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); >> Error *err = NULL, *local_err = NULL; >> + NICInfo *nd = &nd_table[0]; >> >> /* IO space */ >> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM], >> @@ -405,8 +406,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >> } >> >> /* Net */ >> - for (i = 0; i < nb_nics && i < sc->macs_num; i++) { >> - qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]); >> + for (i = 0; i < sc->macs_num; i++) { >> + if ((s->macs_mask & (1 << i)) && nd->used) { >> + qemu_check_nic_model(nd, TYPE_FTGMAC100); >> + qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd); >> + nd++; >> + } >> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed", >> &err); >> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized", >> @@ -455,6 +460,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >> aspeed_soc_get_irq(s, ASPEED_SDHCI)); >> } >> static Property aspeed_soc_properties[] = { >> + DEFINE_PROP_UINT32("macs-mask", AspeedSoCState, macs_mask, 0x1), >> DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION, >> MemoryRegion *), >> DEFINE_PROP_END_OF_LIST(), > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm/aspeed: Rework NIC attachment 2020-05-20 7:01 ` Cédric Le Goater @ 2020-05-20 15:43 ` Markus Armbruster 2020-05-21 7:36 ` Cédric Le Goater 0 siblings, 1 reply; 6+ messages in thread From: Markus Armbruster @ 2020-05-20 15:43 UTC (permalink / raw) To: Cédric Le Goater Cc: Peter Maydell, Andrew Jeffery, qemu-devel, qemu-arm, Joel Stanley, Philippe Mathieu-Daudé Cédric Le Goater <clg@kaod.org> writes: > On 5/20/20 8:34 AM, Markus Armbruster wrote: >> Cédric Le Goater <clg@kaod.org> writes: >> >>> The AST2400 and AST2500 SoCs have two MACs but only the first MAC0 is >>> active on the Aspeed machines using these SoCs. The AST2600 has four >>> MACs. The AST2600 EVB machine activates MAC1, MAC2 and MAC3 and the >>> Tacoma BMC machine activates MAC2. >>> >>> Introduce a bit-field property "macs-mask" under the Aspeed SoC model >>> to link the active MACs of the machine being started with the available >>> network devices. >>> >>> Inactive MACs will have no peer and QEMU will warn the user with : >>> >>> qemu-system-arm: warning: nic ftgmac100.0 has no peer >>> qemu-system-arm: warning: nic ftgmac100.1 has no peer >>> qemu-system-arm: warning: nic ftgmac100.3 has no peer >> >> I can't reproduce this warning. What's your exact command line? > > Get a witherspoon-tacoma flash image : > > $ wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,label=builder,target=witherspoon-tacoma/lastSuccessfulBuild/artifact/deploy/images/witherspoon-tacoma/flash-witherspoon-tacoma > > Run : > > $ qemu-system-arm -M tacoma-bmc -nic user -drive file=./flash-witherspoon-tacoma,format=raw,if=mtd -nographic -nodefaults -serial mon:stdio > qemu-system-arm: warning: nic ftgmac100.0 has no peer > qemu-system-arm: warning: nic ftgmac100.1 has no peer > qemu-system-arm: warning: nic ftgmac100.3 has no peer I must have run the wrong binary somehow. >> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> >>> To be applied on top of patch "arm/aspeed: Compute the number of CPUs >>> from the SoC definition" >>> >>> http://patchwork.ozlabs.org/project/qemu-devel/patch/20200519091631.1006073-1-clg@kaod.org/ >>> >>> include/hw/arm/aspeed.h | 1 + >>> include/hw/arm/aspeed_soc.h | 6 ++++++ >>> hw/arm/aspeed.c | 6 ++++++ >>> hw/arm/aspeed_ast2600.c | 11 ++++++++--- >>> hw/arm/aspeed_soc.c | 10 ++++++++-- >>> 5 files changed, 29 insertions(+), 5 deletions(-) >>> >>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h >>> index 18521484b90e..842dff485f5b 100644 >>> --- a/include/hw/arm/aspeed.h >>> +++ b/include/hw/arm/aspeed.h >>> @@ -39,6 +39,7 @@ typedef struct AspeedMachineClass { >>> const char *fmc_model; >>> const char *spi_model; >>> uint32_t num_cs; >>> + uint32_t macs_mask; >>> void (*i2c_init)(AspeedBoardState *bmc); >>> } AspeedMachineClass; >>> >>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h >>> index 914115f3ef77..fdb9e05bc47c 100644 >>> --- a/include/hw/arm/aspeed_soc.h >>> +++ b/include/hw/arm/aspeed_soc.h >>> @@ -34,6 +34,11 @@ >>> #define ASPEED_CPUS_NUM 2 >>> #define ASPEED_MACS_NUM 4 >>> >>> +#define ASPEED_MAC0_ON (1 << 0) >>> +#define ASPEED_MAC1_ON (1 << 1) >>> +#define ASPEED_MAC2_ON (1 << 2) >>> +#define ASPEED_MAC3_ON (1 << 3) >>> + >>> typedef struct AspeedSoCState { >>> /*< private >*/ >>> DeviceState parent; >>> @@ -55,6 +60,7 @@ typedef struct AspeedSoCState { >>> AspeedSDMCState sdmc; >>> AspeedWDTState wdt[ASPEED_WDTS_NUM]; >>> FTGMAC100State ftgmac100[ASPEED_MACS_NUM]; >>> + uint32_t macs_mask; >> >> What's the purpose of this member? When and how would it be different >> from AspeedMachineClass's macs_mask? > > Each machine activates a different set of MACs even if using the same SoC. > So, the SoC macs_mask is overiden when the machine initializes the SoC in > aspeed_machine_init(). Let me try to rephrase my question below. > That said, I think the default SoC macs_mask should be all MACS, a value > of 0xFFFFFFFF would be fine, and not only the first MAC as this patch does. > >>> AspeedMiiState mii[ASPEED_MACS_NUM]; >>> AspeedGPIOState gpio; >>> AspeedGPIOState gpio_1_8v; >>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >>> index 6f8f4b88f8ab..79c683864d7e 100644 >>> --- a/hw/arm/aspeed.c >>> +++ b/hw/arm/aspeed.c >>> @@ -283,6 +283,8 @@ static void aspeed_machine_init(MachineState *machine) >>> &error_abort); >>> object_property_set_int(OBJECT(&bmc->soc), amc->num_cs, "num-cs", >>> &error_abort); >>> + object_property_set_int(OBJECT(&bmc->soc), amc->macs_mask, "macs-mask", >>> + &error_abort); Here, you set AspeedSocState member macs_mask to AspeedMachineClass member macs_mask. >>> object_property_set_link(OBJECT(&bmc->soc), OBJECT(&bmc->ram_container), >>> "dram", &error_abort); >>> if (machine->kernel_filename) { >>> @@ -556,12 +558,14 @@ static int aspeed_soc_num_cpus(const char *soc_name) >>> static void aspeed_machine_class_init(ObjectClass *oc, void *data) >>> { >>> MachineClass *mc = MACHINE_CLASS(oc); >>> + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc); >>> >>> mc->init = aspeed_machine_init; >>> mc->no_floppy = 1; >>> mc->no_cdrom = 1; >>> mc->no_parallel = 1; >>> mc->default_ram_id = "ram"; >>> + amc->macs_mask = ASPEED_MAC0_ON; Abstract base type's .class_init() sets AspeedMachineClass's macs_mask to "just first one". >>> >>> aspeed_machine_class_props_init(oc); >>> } >>> @@ -680,6 +684,7 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data) >>> amc->fmc_model = "w25q512jv"; >>> amc->spi_model = "mx66u51235f"; >>> amc->num_cs = 1; >>> + amc->macs_mask = ASPEED_MAC1_ON | ASPEED_MAC2_ON | ASPEED_MAC3_ON; >>> amc->i2c_init = ast2600_evb_i2c_init; >>> mc->default_ram_size = 1 * GiB; >>> mc->default_cpus = mc->min_cpus = mc->max_cpus = >>> @@ -698,6 +703,7 @@ static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data) >>> amc->fmc_model = "mx66l1g45g"; >>> amc->spi_model = "mx66l1g45g"; >>> amc->num_cs = 2; >>> + amc->macs_mask = ASPEED_MAC2_ON; >>> amc->i2c_init = witherspoon_bmc_i2c_init; /* Same board layout */ >>> mc->default_ram_size = 1 * GiB; >>> mc->default_cpus = mc->min_cpus = mc->max_cpus = Two concrete types' .class_init() override this default. Fine with me. >>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c >>> index 114b94f8f44d..fa85122f6d78 100644 >>> --- a/hw/arm/aspeed_ast2600.c >>> +++ b/hw/arm/aspeed_ast2600.c >>> @@ -247,6 +247,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >>> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); >>> Error *err = NULL, *local_err = NULL; >>> qemu_irq irq; >>> + NICInfo *nd = &nd_table[0]; >>> >>> /* IO space */ >>> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM], >>> @@ -462,8 +463,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >>> } >>> >>> /* Net */ >>> - for (i = 0; i < nb_nics && i < sc->macs_num; i++) { >>> - qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]); >>> + for (i = 0; i < sc->macs_num; i++) { >>> + if ((s->macs_mask & (1 << i)) && nd->used) { This checks AspeedSocState's member. >>> + qemu_check_nic_model(nd, TYPE_FTGMAC100); >>> + qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd); >>> + nd++; >>> + } >>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed", >>> &err); >>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized", >>> @@ -471,7 +476,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >>> error_propagate(&err, local_err); >>> if (err) { >>> error_propagate(errp, err); >>> - return; >>> + return; >>> } >>> sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0, >>> sc->memmap[ASPEED_ETH1 + i]); >>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c >>> index 984d29087dce..d2c6a5760790 100644 >>> --- a/hw/arm/aspeed_soc.c >>> +++ b/hw/arm/aspeed_soc.c >>> @@ -234,6 +234,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >>> AspeedSoCState *s = ASPEED_SOC(dev); >>> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); >>> Error *err = NULL, *local_err = NULL; >>> + NICInfo *nd = &nd_table[0]; >>> >>> /* IO space */ >>> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM], >>> @@ -405,8 +406,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >>> } >>> >>> /* Net */ >>> - for (i = 0; i < nb_nics && i < sc->macs_num; i++) { >>> - qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]); >>> + for (i = 0; i < sc->macs_num; i++) { >>> + if ((s->macs_mask & (1 << i)) && nd->used) { Likewise. Now my rephrased question: why do you need to store macs_mask both in AspeedSoCState and in AspeedMachineClass? As far as I can tell, their values are the same for any given machine. >>> + qemu_check_nic_model(nd, TYPE_FTGMAC100); >>> + qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd); >>> + nd++; >>> + } >>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed", >>> &err); >>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized", >>> @@ -455,6 +460,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >>> aspeed_soc_get_irq(s, ASPEED_SDHCI)); >>> } >>> static Property aspeed_soc_properties[] = { >>> + DEFINE_PROP_UINT32("macs-mask", AspeedSoCState, macs_mask, 0x1), Shouldn't this be ASPEED_MAC0_ON rather than 0x1? Hmm, isn't aspeed_machine_class_init()'s amc->macs_mask = ASPEED_MAC0_ON redundant with this? >>> DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION, >>> MemoryRegion *), >>> DEFINE_PROP_END_OF_LIST(), >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm/aspeed: Rework NIC attachment 2020-05-20 15:43 ` Markus Armbruster @ 2020-05-21 7:36 ` Cédric Le Goater 2020-05-25 7:09 ` Markus Armbruster 0 siblings, 1 reply; 6+ messages in thread From: Cédric Le Goater @ 2020-05-21 7:36 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Andrew Jeffery, qemu-devel, qemu-arm, Joel Stanley, Philippe Mathieu-Daudé On 5/20/20 5:43 PM, Markus Armbruster wrote: > Cédric Le Goater <clg@kaod.org> writes: > >> On 5/20/20 8:34 AM, Markus Armbruster wrote: >>> Cédric Le Goater <clg@kaod.org> writes: >>> >>>> The AST2400 and AST2500 SoCs have two MACs but only the first MAC0 is >>>> active on the Aspeed machines using these SoCs. The AST2600 has four >>>> MACs. The AST2600 EVB machine activates MAC1, MAC2 and MAC3 and the >>>> Tacoma BMC machine activates MAC2. >>>> >>>> Introduce a bit-field property "macs-mask" under the Aspeed SoC model >>>> to link the active MACs of the machine being started with the available >>>> network devices. >>>> >>>> Inactive MACs will have no peer and QEMU will warn the user with : >>>> >>>> qemu-system-arm: warning: nic ftgmac100.0 has no peer >>>> qemu-system-arm: warning: nic ftgmac100.1 has no peer >>>> qemu-system-arm: warning: nic ftgmac100.3 has no peer >>> >>> I can't reproduce this warning. What's your exact command line? >> >> Get a witherspoon-tacoma flash image : >> >> $ wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,label=builder,target=witherspoon-tacoma/lastSuccessfulBuild/artifact/deploy/images/witherspoon-tacoma/flash-witherspoon-tacoma >> >> Run : >> >> $ qemu-system-arm -M tacoma-bmc -nic user -drive file=./flash-witherspoon-tacoma,format=raw,if=mtd -nographic -nodefaults -serial mon:stdio >> qemu-system-arm: warning: nic ftgmac100.0 has no peer >> qemu-system-arm: warning: nic ftgmac100.1 has no peer >> qemu-system-arm: warning: nic ftgmac100.3 has no peer > > I must have run the wrong binary somehow. > >>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>>> >>>> To be applied on top of patch "arm/aspeed: Compute the number of CPUs >>>> from the SoC definition" >>>> >>>> http://patchwork.ozlabs.org/project/qemu-devel/patch/20200519091631.1006073-1-clg@kaod.org/ >>>> >>>> include/hw/arm/aspeed.h | 1 + >>>> include/hw/arm/aspeed_soc.h | 6 ++++++ >>>> hw/arm/aspeed.c | 6 ++++++ >>>> hw/arm/aspeed_ast2600.c | 11 ++++++++--- >>>> hw/arm/aspeed_soc.c | 10 ++++++++-- >>>> 5 files changed, 29 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h >>>> index 18521484b90e..842dff485f5b 100644 >>>> --- a/include/hw/arm/aspeed.h >>>> +++ b/include/hw/arm/aspeed.h >>>> @@ -39,6 +39,7 @@ typedef struct AspeedMachineClass { >>>> const char *fmc_model; >>>> const char *spi_model; >>>> uint32_t num_cs; >>>> + uint32_t macs_mask; >>>> void (*i2c_init)(AspeedBoardState *bmc); >>>> } AspeedMachineClass; >>>> >>>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h >>>> index 914115f3ef77..fdb9e05bc47c 100644 >>>> --- a/include/hw/arm/aspeed_soc.h >>>> +++ b/include/hw/arm/aspeed_soc.h >>>> @@ -34,6 +34,11 @@ >>>> #define ASPEED_CPUS_NUM 2 >>>> #define ASPEED_MACS_NUM 4 >>>> >>>> +#define ASPEED_MAC0_ON (1 << 0) >>>> +#define ASPEED_MAC1_ON (1 << 1) >>>> +#define ASPEED_MAC2_ON (1 << 2) >>>> +#define ASPEED_MAC3_ON (1 << 3) >>>> + >>>> typedef struct AspeedSoCState { >>>> /*< private >*/ >>>> DeviceState parent; >>>> @@ -55,6 +60,7 @@ typedef struct AspeedSoCState { >>>> AspeedSDMCState sdmc; >>>> AspeedWDTState wdt[ASPEED_WDTS_NUM]; >>>> FTGMAC100State ftgmac100[ASPEED_MACS_NUM]; >>>> + uint32_t macs_mask; >>> >>> What's the purpose of this member? When and how would it be different >>> from AspeedMachineClass's macs_mask? >> >> Each machine activates a different set of MACs even if using the same SoC. >> So, the SoC macs_mask is overiden when the machine initializes the SoC in >> aspeed_machine_init(). > > Let me try to rephrase my question below. > >> That said, I think the default SoC macs_mask should be all MACS, a value >> of 0xFFFFFFFF would be fine, and not only the first MAC as this patch does. >> >>>> AspeedMiiState mii[ASPEED_MACS_NUM]; >>>> AspeedGPIOState gpio; >>>> AspeedGPIOState gpio_1_8v; >>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >>>> index 6f8f4b88f8ab..79c683864d7e 100644 >>>> --- a/hw/arm/aspeed.c >>>> +++ b/hw/arm/aspeed.c >>>> @@ -283,6 +283,8 @@ static void aspeed_machine_init(MachineState *machine) >>>> &error_abort); >>>> object_property_set_int(OBJECT(&bmc->soc), amc->num_cs, "num-cs", >>>> &error_abort); >>>> + object_property_set_int(OBJECT(&bmc->soc), amc->macs_mask, "macs-mask", >>>> + &error_abort); > > Here, you set AspeedSocState member macs_mask to AspeedMachineClass > member macs_mask. yes. aspeed_machine_init() is common to all machines. AspeedMachineClass gathers the differences. > >>>> object_property_set_link(OBJECT(&bmc->soc), OBJECT(&bmc->ram_container), >>>> "dram", &error_abort); >>>> if (machine->kernel_filename) { >>>> @@ -556,12 +558,14 @@ static int aspeed_soc_num_cpus(const char *soc_name) >>>> static void aspeed_machine_class_init(ObjectClass *oc, void *data) >>>> { >>>> MachineClass *mc = MACHINE_CLASS(oc); >>>> + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc); >>>> >>>> mc->init = aspeed_machine_init; >>>> mc->no_floppy = 1; >>>> mc->no_cdrom = 1; >>>> mc->no_parallel = 1; >>>> mc->default_ram_id = "ram"; >>>> + amc->macs_mask = ASPEED_MAC0_ON; > > Abstract base type's .class_init() sets AspeedMachineClass's macs_mask > to "just first one". Yes. This covers all AST2400 and AST2500 machines. Only the AST2600 use a different set of MACs. > >>>> >>>> aspeed_machine_class_props_init(oc); >>>> } >>>> @@ -680,6 +684,7 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data) >>>> amc->fmc_model = "w25q512jv"; >>>> amc->spi_model = "mx66u51235f"; >>>> amc->num_cs = 1; >>>> + amc->macs_mask = ASPEED_MAC1_ON | ASPEED_MAC2_ON | ASPEED_MAC3_ON; >>>> amc->i2c_init = ast2600_evb_i2c_init; >>>> mc->default_ram_size = 1 * GiB; >>>> mc->default_cpus = mc->min_cpus = mc->max_cpus = >>>> @@ -698,6 +703,7 @@ static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data) >>>> amc->fmc_model = "mx66l1g45g"; >>>> amc->spi_model = "mx66l1g45g"; >>>> amc->num_cs = 2; >>>> + amc->macs_mask = ASPEED_MAC2_ON; >>>> amc->i2c_init = witherspoon_bmc_i2c_init; /* Same board layout */ >>>> mc->default_ram_size = 1 * GiB; >>>> mc->default_cpus = mc->min_cpus = mc->max_cpus = > > Two concrete types' .class_init() override this default. yes. > Fine with me. > >>>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c >>>> index 114b94f8f44d..fa85122f6d78 100644 >>>> --- a/hw/arm/aspeed_ast2600.c >>>> +++ b/hw/arm/aspeed_ast2600.c >>>> @@ -247,6 +247,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >>>> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); >>>> Error *err = NULL, *local_err = NULL; >>>> qemu_irq irq; >>>> + NICInfo *nd = &nd_table[0]; >>>> >>>> /* IO space */ >>>> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM], >>>> @@ -462,8 +463,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >>>> } >>>> >>>> /* Net */ >>>> - for (i = 0; i < nb_nics && i < sc->macs_num; i++) { >>>> - qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]); >>>> + for (i = 0; i < sc->macs_num; i++) { >>>> + if ((s->macs_mask & (1 << i)) && nd->used) { > > This checks AspeedSocState's member. > >>>> + qemu_check_nic_model(nd, TYPE_FTGMAC100); >>>> + qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd); >>>> + nd++; >>>> + } >>>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed", >>>> &err); >>>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized", >>>> @@ -471,7 +476,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >>>> error_propagate(&err, local_err); >>>> if (err) { >>>> error_propagate(errp, err); >>>> - return; >>>> + return; >>>> } >>>> sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0, >>>> sc->memmap[ASPEED_ETH1 + i]); >>>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c >>>> index 984d29087dce..d2c6a5760790 100644 >>>> --- a/hw/arm/aspeed_soc.c >>>> +++ b/hw/arm/aspeed_soc.c >>>> @@ -234,6 +234,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >>>> AspeedSoCState *s = ASPEED_SOC(dev); >>>> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); >>>> Error *err = NULL, *local_err = NULL; >>>> + NICInfo *nd = &nd_table[0]; >>>> >>>> /* IO space */ >>>> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM], >>>> @@ -405,8 +406,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >>>> } >>>> >>>> /* Net */ >>>> - for (i = 0; i < nb_nics && i < sc->macs_num; i++) { >>>> - qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]); >>>> + for (i = 0; i < sc->macs_num; i++) { >>>> + if ((s->macs_mask & (1 << i)) && nd->used) { > > Likewise. > > Now my rephrased question: why do you need to store macs_mask both in > AspeedSoCState and in AspeedMachineClass? As far as I can tell, their > values are the same for any given machine. yes, they are the same because the parent machine "propagates" the top level configuration to all sub-devices. The SoC is one of these and its initial settings are generic and do not make assumption on the machine characteristics. The general case is "use MAC0" only, which works for all AST2400 and AST2500 machines but the AST2600 machines have a different configuration. I agree there is some redundancy in this design pattern. What do you have in mind ? > >>>> + qemu_check_nic_model(nd, TYPE_FTGMAC100); >>>> + qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd); >>>> + nd++; >>>> + } >>>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed", >>>> &err); >>>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized", >>>> @@ -455,6 +460,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >>>> aspeed_soc_get_irq(s, ASPEED_SDHCI)); >>>> } >>>> static Property aspeed_soc_properties[] = { >>>> + DEFINE_PROP_UINT32("macs-mask", AspeedSoCState, macs_mask, 0x1), > > Shouldn't this be ASPEED_MAC0_ON rather than 0x1? yes. or 0x0 ? and let the machine fully in charge of the configuration. > Hmm, isn't aspeed_machine_class_init()'s amc->macs_mask = ASPEED_MAC0_ON > redundant with this? yes. So 0x0 is better. Thanks, C. > >>>> DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION, >>>> MemoryRegion *), >>>> DEFINE_PROP_END_OF_LIST(), >>> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm/aspeed: Rework NIC attachment 2020-05-21 7:36 ` Cédric Le Goater @ 2020-05-25 7:09 ` Markus Armbruster 0 siblings, 0 replies; 6+ messages in thread From: Markus Armbruster @ 2020-05-25 7:09 UTC (permalink / raw) To: Cédric Le Goater Cc: Peter Maydell, Andrew Jeffery, qemu-devel, qemu-arm, Joel Stanley, Philippe Mathieu-Daudé Cédric Le Goater <clg@kaod.org> writes: > On 5/20/20 5:43 PM, Markus Armbruster wrote: >> Cédric Le Goater <clg@kaod.org> writes: >> >>> On 5/20/20 8:34 AM, Markus Armbruster wrote: >>>> Cédric Le Goater <clg@kaod.org> writes: >>>> >>>>> The AST2400 and AST2500 SoCs have two MACs but only the first MAC0 is >>>>> active on the Aspeed machines using these SoCs. The AST2600 has four >>>>> MACs. The AST2600 EVB machine activates MAC1, MAC2 and MAC3 and the >>>>> Tacoma BMC machine activates MAC2. >>>>> >>>>> Introduce a bit-field property "macs-mask" under the Aspeed SoC model >>>>> to link the active MACs of the machine being started with the available >>>>> network devices. >>>>> >>>>> Inactive MACs will have no peer and QEMU will warn the user with : >>>>> >>>>> qemu-system-arm: warning: nic ftgmac100.0 has no peer >>>>> qemu-system-arm: warning: nic ftgmac100.1 has no peer >>>>> qemu-system-arm: warning: nic ftgmac100.3 has no peer >>>> >>>> I can't reproduce this warning. What's your exact command line? >>> >>> Get a witherspoon-tacoma flash image : >>> >>> $ wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,label=builder,target=witherspoon-tacoma/lastSuccessfulBuild/artifact/deploy/images/witherspoon-tacoma/flash-witherspoon-tacoma >>> >>> Run : >>> >>> $ qemu-system-arm -M tacoma-bmc -nic user -drive file=./flash-witherspoon-tacoma,format=raw,if=mtd -nographic -nodefaults -serial mon:stdio >>> qemu-system-arm: warning: nic ftgmac100.0 has no peer >>> qemu-system-arm: warning: nic ftgmac100.1 has no peer >>> qemu-system-arm: warning: nic ftgmac100.3 has no peer >> >> I must have run the wrong binary somehow. >> >>>> >>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>>> --- >>>>> >>>>> To be applied on top of patch "arm/aspeed: Compute the number of CPUs >>>>> from the SoC definition" >>>>> >>>>> http://patchwork.ozlabs.org/project/qemu-devel/patch/20200519091631.1006073-1-clg@kaod.org/ >>>>> >>>>> include/hw/arm/aspeed.h | 1 + >>>>> include/hw/arm/aspeed_soc.h | 6 ++++++ >>>>> hw/arm/aspeed.c | 6 ++++++ >>>>> hw/arm/aspeed_ast2600.c | 11 ++++++++--- >>>>> hw/arm/aspeed_soc.c | 10 ++++++++-- >>>>> 5 files changed, 29 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h >>>>> index 18521484b90e..842dff485f5b 100644 >>>>> --- a/include/hw/arm/aspeed.h >>>>> +++ b/include/hw/arm/aspeed.h >>>>> @@ -39,6 +39,7 @@ typedef struct AspeedMachineClass { >>>>> const char *fmc_model; >>>>> const char *spi_model; >>>>> uint32_t num_cs; >>>>> + uint32_t macs_mask; >>>>> void (*i2c_init)(AspeedBoardState *bmc); >>>>> } AspeedMachineClass; >>>>> >>>>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h >>>>> index 914115f3ef77..fdb9e05bc47c 100644 >>>>> --- a/include/hw/arm/aspeed_soc.h >>>>> +++ b/include/hw/arm/aspeed_soc.h >>>>> @@ -34,6 +34,11 @@ >>>>> #define ASPEED_CPUS_NUM 2 >>>>> #define ASPEED_MACS_NUM 4 >>>>> >>>>> +#define ASPEED_MAC0_ON (1 << 0) >>>>> +#define ASPEED_MAC1_ON (1 << 1) >>>>> +#define ASPEED_MAC2_ON (1 << 2) >>>>> +#define ASPEED_MAC3_ON (1 << 3) >>>>> + >>>>> typedef struct AspeedSoCState { >>>>> /*< private >*/ >>>>> DeviceState parent; >>>>> @@ -55,6 +60,7 @@ typedef struct AspeedSoCState { >>>>> AspeedSDMCState sdmc; >>>>> AspeedWDTState wdt[ASPEED_WDTS_NUM]; >>>>> FTGMAC100State ftgmac100[ASPEED_MACS_NUM]; >>>>> + uint32_t macs_mask; >>>> >>>> What's the purpose of this member? When and how would it be different >>>> from AspeedMachineClass's macs_mask? >>> >>> Each machine activates a different set of MACs even if using the same SoC. >>> So, the SoC macs_mask is overiden when the machine initializes the SoC in >>> aspeed_machine_init(). >> >> Let me try to rephrase my question below. >> >>> That said, I think the default SoC macs_mask should be all MACS, a value >>> of 0xFFFFFFFF would be fine, and not only the first MAC as this patch does. >>> >>>>> AspeedMiiState mii[ASPEED_MACS_NUM]; >>>>> AspeedGPIOState gpio; >>>>> AspeedGPIOState gpio_1_8v; >>>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >>>>> index 6f8f4b88f8ab..79c683864d7e 100644 >>>>> --- a/hw/arm/aspeed.c >>>>> +++ b/hw/arm/aspeed.c >>>>> @@ -283,6 +283,8 @@ static void aspeed_machine_init(MachineState *machine) >>>>> &error_abort); >>>>> object_property_set_int(OBJECT(&bmc->soc), amc->num_cs, "num-cs", >>>>> &error_abort); >>>>> + object_property_set_int(OBJECT(&bmc->soc), amc->macs_mask, "macs-mask", >>>>> + &error_abort); >> >> Here, you set AspeedSocState member macs_mask to AspeedMachineClass >> member macs_mask. > > yes. aspeed_machine_init() is common to all machines. AspeedMachineClass > gathers the differences. > >> >>>>> object_property_set_link(OBJECT(&bmc->soc), OBJECT(&bmc->ram_container), >>>>> "dram", &error_abort); >>>>> if (machine->kernel_filename) { >>>>> @@ -556,12 +558,14 @@ static int aspeed_soc_num_cpus(const char *soc_name) >>>>> static void aspeed_machine_class_init(ObjectClass *oc, void *data) >>>>> { >>>>> MachineClass *mc = MACHINE_CLASS(oc); >>>>> + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc); >>>>> >>>>> mc->init = aspeed_machine_init; >>>>> mc->no_floppy = 1; >>>>> mc->no_cdrom = 1; >>>>> mc->no_parallel = 1; >>>>> mc->default_ram_id = "ram"; >>>>> + amc->macs_mask = ASPEED_MAC0_ON; >> >> Abstract base type's .class_init() sets AspeedMachineClass's macs_mask >> to "just first one". > > Yes. This covers all AST2400 and AST2500 machines. Only the AST2600 > use a different set of MACs. > >> >>>>> >>>>> aspeed_machine_class_props_init(oc); >>>>> } >>>>> @@ -680,6 +684,7 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data) >>>>> amc->fmc_model = "w25q512jv"; >>>>> amc->spi_model = "mx66u51235f"; >>>>> amc->num_cs = 1; >>>>> + amc->macs_mask = ASPEED_MAC1_ON | ASPEED_MAC2_ON | ASPEED_MAC3_ON; >>>>> amc->i2c_init = ast2600_evb_i2c_init; >>>>> mc->default_ram_size = 1 * GiB; >>>>> mc->default_cpus = mc->min_cpus = mc->max_cpus = >>>>> @@ -698,6 +703,7 @@ static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data) >>>>> amc->fmc_model = "mx66l1g45g"; >>>>> amc->spi_model = "mx66l1g45g"; >>>>> amc->num_cs = 2; >>>>> + amc->macs_mask = ASPEED_MAC2_ON; >>>>> amc->i2c_init = witherspoon_bmc_i2c_init; /* Same board layout */ >>>>> mc->default_ram_size = 1 * GiB; >>>>> mc->default_cpus = mc->min_cpus = mc->max_cpus = >> >> Two concrete types' .class_init() override this default. > > yes. > >> Fine with me. >> >>>>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c >>>>> index 114b94f8f44d..fa85122f6d78 100644 >>>>> --- a/hw/arm/aspeed_ast2600.c >>>>> +++ b/hw/arm/aspeed_ast2600.c >>>>> @@ -247,6 +247,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >>>>> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); >>>>> Error *err = NULL, *local_err = NULL; >>>>> qemu_irq irq; >>>>> + NICInfo *nd = &nd_table[0]; >>>>> >>>>> /* IO space */ >>>>> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM], >>>>> @@ -462,8 +463,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >>>>> } >>>>> >>>>> /* Net */ >>>>> - for (i = 0; i < nb_nics && i < sc->macs_num; i++) { >>>>> - qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]); >>>>> + for (i = 0; i < sc->macs_num; i++) { >>>>> + if ((s->macs_mask & (1 << i)) && nd->used) { >> >> This checks AspeedSocState's member. >> >>>>> + qemu_check_nic_model(nd, TYPE_FTGMAC100); >>>>> + qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd); >>>>> + nd++; >>>>> + } >>>>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed", >>>>> &err); >>>>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized", >>>>> @@ -471,7 +476,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >>>>> error_propagate(&err, local_err); >>>>> if (err) { >>>>> error_propagate(errp, err); >>>>> - return; >>>>> + return; >>>>> } >>>>> sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0, >>>>> sc->memmap[ASPEED_ETH1 + i]); >>>>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c >>>>> index 984d29087dce..d2c6a5760790 100644 >>>>> --- a/hw/arm/aspeed_soc.c >>>>> +++ b/hw/arm/aspeed_soc.c >>>>> @@ -234,6 +234,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >>>>> AspeedSoCState *s = ASPEED_SOC(dev); >>>>> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); >>>>> Error *err = NULL, *local_err = NULL; >>>>> + NICInfo *nd = &nd_table[0]; >>>>> >>>>> /* IO space */ >>>>> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM], >>>>> @@ -405,8 +406,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >>>>> } >>>>> >>>>> /* Net */ >>>>> - for (i = 0; i < nb_nics && i < sc->macs_num; i++) { >>>>> - qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]); >>>>> + for (i = 0; i < sc->macs_num; i++) { >>>>> + if ((s->macs_mask & (1 << i)) && nd->used) { >> >> Likewise. >> >> Now my rephrased question: why do you need to store macs_mask both in >> AspeedSoCState and in AspeedMachineClass? As far as I can tell, their >> values are the same for any given machine. > > yes, they are the same because the parent machine "propagates" the top > level configuration to all sub-devices. The SoC is one of these and its > initial settings are generic and do not make assumption on the machine > characteristics. > > The general case is "use MAC0" only, which works for all AST2400 and > AST2500 machines but the AST2600 machines have a different configuration. > > I agree there is some redundancy in this design pattern. What do you > have in mind ? I'd drop AspeedSoCState's copy of @macs_mask. That way, readers don't have to trace the data flow to find out that the two copies of @macs_mask are actually identical. This is not a demand. Having two copies isn't wrong. It's just mildly confusing. >>>>> + qemu_check_nic_model(nd, TYPE_FTGMAC100); >>>>> + qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd); >>>>> + nd++; >>>>> + } >>>>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed", >>>>> &err); >>>>> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized", >>>>> @@ -455,6 +460,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp) >>>>> aspeed_soc_get_irq(s, ASPEED_SDHCI)); >>>>> } >>>>> static Property aspeed_soc_properties[] = { >>>>> + DEFINE_PROP_UINT32("macs-mask", AspeedSoCState, macs_mask, 0x1), >> >> Shouldn't this be ASPEED_MAC0_ON rather than 0x1? > > yes. or 0x0 ? and let the machine fully in charge of the configuration. Works for me. >> Hmm, isn't aspeed_machine_class_init()'s amc->macs_mask = ASPEED_MAC0_ON >> redundant with this? > > yes. So 0x0 is better. > > Thanks, > > C. > >> >>>>> DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION, >>>>> MemoryRegion *), >>>>> DEFINE_PROP_END_OF_LIST(), >>>> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-25 7:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-05-19 16:19 [PATCH] arm/aspeed: Rework NIC attachment Cédric Le Goater 2020-05-20 6:34 ` Markus Armbruster 2020-05-20 7:01 ` Cédric Le Goater 2020-05-20 15:43 ` Markus Armbruster 2020-05-21 7:36 ` Cédric Le Goater 2020-05-25 7:09 ` Markus Armbruster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).