From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42000) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uytie-0004xw-71 for qemu-devel@nongnu.org; Mon, 15 Jul 2013 21:06:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uytid-0004xc-3I for qemu-devel@nongnu.org; Mon, 15 Jul 2013 21:06:20 -0400 Received: from mail-bk0-f41.google.com ([209.85.214.41]:62582) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uytic-0004xT-SE for qemu-devel@nongnu.org; Mon, 15 Jul 2013 21:06:19 -0400 Received: by mail-bk0-f41.google.com with SMTP id jc3so28142bkc.0 for ; Mon, 15 Jul 2013 18:06:17 -0700 (PDT) MIME-Version: 1.0 Sender: peter.crosthwaite@petalogix.com In-Reply-To: <51E405E4.7080406@adacore.com> References: <1373476202-11277-1-git-send-email-chouteau@adacore.com> <1373476202-11277-3-git-send-email-chouteau@adacore.com> <51E405E4.7080406@adacore.com> Date: Tue, 16 Jul 2013 11:06:17 +1000 Message-ID: From: Peter Crosthwaite Content-Type: text/plain; charset=ISO-8859-1 Subject: Re: [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabien Chouteau Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, agraf@suse.de Hi Fabien, On Tue, Jul 16, 2013 at 12:23 AM, Fabien Chouteau 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 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 >