Hi Fabien, On Tue, Jul 16, 2013 at 12:23 AM, Fabien Chouteau <chout...@adacore.com> wrote: > On 07/15/2013 04:00 AM, Peter Crosthwaite wrote: >> Hi Fabien, >> > > Hi Peter, > >> On Thu, Jul 11, 2013 at 3:10 AM, Fabien Chouteau <chout...@adacore.com> >> wrote: >>> +#ifdef DEBUG_REGISTER >>> + printf("Write 0x%08x @ 0x" TARGET_FMT_plx" val:0x%08x->0x%08x : %s >>> (%s)\n", >>> + (unsigned int)value, addr, before, reg->value, reg->name, >>> reg->desc); >> >> Last I knew, printf was a bad idea for error messages due to monitor >> interference issue and nographic mode. But qemu_log or fprintf(stderr, >> are both better alternatives. >> > > Fixed. > >>> +static int etsec_can_receive(NetClientState *nc) >>> +{ >>> + /* Yes we always can\ */ >>> + return 1; >> >> As a general rule this is a bad idea. Multiple ethernet controllers in >> QEMU have tried this and had issues (particularly with the UBOOT >> bootloader) with mass packet droppage. But You have access to the >> information needed (the if conditions in rx_ring_write) to implement >> this it seems. >> > > Fixed. > >>> +static int etsec_init(SysBusDevice *dev) >>> +{ >>> + eTSEC *etsec = FROM_SYSBUS(typeof(*etsec), dev); >> >> Define and use QOM cast macros instead, FROM_FOO macros are deprecated. >> > > Something like: > > #define TYPE_ETSEC_COMMON "eTSEC" > #define ETSEC_COMMON(obj) \ > OBJECT_CHECK(eTSEC, (obj), TYPE_ETSEC_COMMON) > > > static int etsec_init(SysBusDevice *dev) > { > eTSEC *etsec = ETSEC_COMMON(dev); > > ? >
Yes thats the one. > >>> + >>> + memory_region_init_io(&etsec->io_area, OBJECT(etsec), &etsec_ops, >>> etsec, >>> + "eTSEC", 0x1000); >> >> Constant size memory_region_init_io should be migrated to the Object::Init >> fm. >> > > What is Object::Init()? Do you have an example? > hw/dma/xilinx_axidma.c - xilinx_axidma_init() and xilinx_axidma_realize() has an example of splitting init task between early and late. Note the memory_region_init_io is in the _init. Regards, Peter >>> +static void etsec_class_init(ObjectClass *klass, void *data) >>> +{ >>> + DeviceClass *dc = DEVICE_CLASS(klass); >>> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); >>> + >>> + k->init = etsec_init; >> >> SysBusDevice::init in depracated. Please use Device::realize instead. >> > > Fixed. > >>> +#define ACC_RW 1 /* Read/Write */ >>> +#define ACC_RO 2 /* Read Only */ >>> +#define ACC_WO 3 /* Write Only */ >>> +#define ACC_w1c 4 /* Write 1 to clear */ >> >> ACC_W1C. Would it be cleaner with an enum instead? >> > Fixed. > >>> +#ifdef HEX_DUMP >>> + >>> +static void hex_dump(FILE *f, const uint8_t *buf, int size) >> >> can you just use qemu_hexdump? >> >> check util/hexdump.c >> > > Didn't know there was one. Fixed. > > Thanks for the review. > > -- > Fabien Chouteau >