On Sun, Jan 26, 2014 at 12:42:59AM +0100, Andreas Färber wrote:
> Am 20.01.2014 00:25, schrieb Beniamino Galvani:
> > Signed-off-by: Beniamino Galvani <b.galv...@gmail.com>
> > ---
> > hw/arm/allwinner-a10.c | 16 ++++++++++++++++
> > hw/arm/cubieboard.c | 7 +++++++
> > include/hw/arm/allwinner-a10.h | 3 +++
> > 3 files changed, 26 insertions(+)
> >
> > diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> > index 4658e19..416cd49 100644
> > --- a/hw/arm/allwinner-a10.c
> > +++ b/hw/arm/allwinner-a10.c
> > @@ -31,6 +31,13 @@ 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());
> > +
> > + object_initialize(&s->emac, sizeof(s->emac), TYPE_AW_EMAC);
> > + qdev_set_parent_bus(DEVICE(&s->emac), sysbus_get_default());
> > + if (nd_table[0].used) {
> > + qemu_check_nic_model(&nd_table[0], "allwinner_emac");
>
> Please adopt new-style names with dashes, i.e. "allwinner-emac". Peter
> C.'s comment wrt TYPE_* still applies though.
Ok.
>
> > + qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
>
> Since you're using DEVICE() twice, you should consider using a local
> variable like you did for sysbusdev below. It's a one-time
> initialization though, so not mandatory.
For readability and also, as suggested by Peter in a previous review,
consistency with nearby code I'd prefer to keep the two casts.
>
> > + }
> > }
> >
> > static void aw_a10_realize(DeviceState *dev, Error **errp)
> > @@ -76,6 +83,15 @@ 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]);
> >
> > + 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/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
> > index 3fcb6d2..b3f8f51 100644
> > --- a/hw/arm/cubieboard.c
> > +++ b/hw/arm/cubieboard.c
> > @@ -36,6 +36,13 @@ static void cubieboard_init(QEMUMachineInitArgs *args)
> > Error *err = NULL;
> >
> > s->a10 = AW_A10(object_new(TYPE_AW_A10));
> > +
> > + object_property_set_int(OBJECT(&s->a10->emac), 1, "phyaddr", &err);
>
> "phy-addr"?
Other network adapters are using "phyaddr" so I simply copied that.
But I agree, "phy-addr" sounds better. I will change it in v4.
>
> > + if (err != NULL) {
> > + error_report("Couldn't set phy address: %s\n",
> > error_get_pretty(err));
>
> error_report() always without trailing \n.
>
> > + exit(1);
> > + }
> > +
> > object_property_set_bool(OBJECT(s->a10), true, "realized", &err);
> > if (err != NULL) {
> > error_report("Couldn't realize Allwinner A10: %s\n",
>
> Bad example. ;) Fixes welcome.
I will fix both.
Beniamino
>
> Cheers,
> Andreas
>
> > diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
> > index da36647..01a189b 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,6 +15,7 @@
> > #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
> >
> > @@ -29,6 +31,7 @@ typedef struct AwA10State {
> > qemu_irq irq[AW_A10_PIC_INT_NR];
> > AwA10PITState timer;
> > AwA10PICState intc;
> > + AwEmacState emac;
> > } AwA10State;
> >
> > #define ALLWINNER_H_
> >
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
