qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] arm/aspeed: Rework NIC attachment


From: Cédric Le Goater
Subject: Re: [PATCH] arm/aspeed: Rework NIC attachment
Date: Thu, 21 May 2020 09:36:05 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0

On 5/20/20 5:43 PM, Markus Armbruster wrote:
> Cédric Le Goater <address@hidden> writes:
> 
>> On 5/20/20 8:34 AM, Markus Armbruster wrote:
>>> Cédric Le Goater <address@hidden> 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 <address@hidden>
>>>> ---
>>>>
>>>>  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/address@hidden/
>>>>  
>>>>  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(),
>>>
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]