Re: [Qemu-devel] [PATCH 2/2] hw/arm/allwinner-a10: initialize EMAC

[Peter Crosthwaite]

On Fri, Jan 3, 2014 at 3:19 AM, Beniamino Galvani <address@hidden> wrote:
> On Thu, Jan 02, 2014 at 08:20:12PM +1000, Peter Crosthwaite wrote:
>> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <address@hidden> wrote:
>> > Signed-off-by: Beniamino Galvani <address@hidden>
>> > ---
>> >  hw/arm/allwinner-a10.c         |   20 ++++++++++++++++++++
>> >  include/hw/arm/allwinner-a10.h |    4 ++++
>> >  2 files changed, 24 insertions(+)
>> >
>> > diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
>> > index 4658e19..155e026 100644
>> > --- a/hw/arm/allwinner-a10.c
>> > +++ b/hw/arm/allwinner-a10.c
>> > @@ -22,6 +22,7 @@
>> >  static void aw_a10_init(Object *obj)
>> >  {
>> >      AwA10State *s = AW_A10(obj);
>> > +    DeviceState *dev;
>>
>> For consistency with surrounding code, you could just cast to DEVICE
>> include and drop the new local var.
>>
>
> Ok.
>
>> >
>> >      object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a8-" TYPE_ARM_CPU);
>> >      object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
>> > @@ -31,6 +32,14 @@ static void aw_a10_init(Object *obj)
>> >
>> >      object_initialize(&s->timer, sizeof(s->timer), TYPE_AW_A10_PIT);
>> >      qdev_set_parent_bus(DEVICE(&s->timer), sysbus_get_default());
>> > +
>> > +    if (nd_table[0].used) {
>>
>> So with SoC MACs, they are "always there". I think this conditional
>> should be removed, and the MAC is always instantiated. ...
>>
>> > +        qemu_check_nic_model(&nd_table[0], "allwinner_emac");
>> > +        object_initialize(&s->emac, sizeof(s->emac), TYPE_AW_EMAC);
>> > +        dev = DEVICE(&s->emac);
>> > +        qdev_set_nic_properties(dev, &nd_table[0]);
>>
>> ... and then only this is conditional on nd_table.used. The mac model
>> itself then of course needs to be hardened against a null netargs.
>>
>> > +        qdev_set_parent_bus(dev, sysbus_get_default());
>> > +    }
>> >  }
>> >
>
> So, if I understand correctly, the part under the condition should be:
>
>              qdev_set_parent_bus(dev, sysbus_get_default());
>

No, I meant the line above, qdev_set_nic_properties. This corresponds
to attaching the always-present NIC to an emulated network.

Regards,
Peter

> right?
>
>> >  static void aw_a10_realize(DeviceState *dev, Error **errp)
>> > @@ -76,6 +85,17 @@ static void aw_a10_realize(DeviceState *dev, Error 
>> > **errp)
>> >      sysbus_connect_irq(sysbusdev, 4, s->irq[67]);
>> >      sysbus_connect_irq(sysbusdev, 5, s->irq[68]);
>> >
>> > +    if (nd_table[0].used) {
>> > +        object_property_set_bool(OBJECT(&s->emac), true, "realized", 
>> > &err);
>> > +        if (err != NULL) {
>> > +            error_propagate(errp, err);
>> > +            return;
>> > +        }
>> > +        sysbusdev = SYS_BUS_DEVICE(&s->emac);
>> > +        sysbus_mmio_map(sysbusdev, 0, AW_A10_EMAC_BASE);
>> > +        sysbus_connect_irq(sysbusdev, 0, s->irq[55]);
>> > +    }
>> > +
>> >      serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, 
>> > s->irq[1],
>> >                     115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
>> >  }
>> > diff --git a/include/hw/arm/allwinner-a10.h 
>> > b/include/hw/arm/allwinner-a10.h
>> > index da36647..6ea5988 100644
>> > --- a/include/hw/arm/allwinner-a10.h
>> > +++ b/include/hw/arm/allwinner-a10.h
>> > @@ -6,6 +6,7 @@
>> >  #include "hw/arm/arm.h"
>> >  #include "hw/timer/allwinner-a10-pit.h"
>> >  #include "hw/intc/allwinner-a10-pic.h"
>> > +#include "hw/net/allwinner_emac.h"
>> >
>> >  #include "sysemu/sysemu.h"
>> >  #include "exec/address-spaces.h"
>> > @@ -14,9 +15,11 @@
>> >  #define AW_A10_PIC_REG_BASE     0x01c20400
>> >  #define AW_A10_PIT_REG_BASE     0x01c20c00
>> >  #define AW_A10_UART0_REG_BASE   0x01c28000
>> > +#define AW_A10_EMAC_BASE        0x01c0b000
>> >
>> >  #define AW_A10_SDRAM_BASE       0x40000000
>> >
>> > +
>>
>> This whitespace change intended?
>
> No, a leftover.
>
> Thanks,
> Beniamino
>