From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:51257)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <chouteau@adacore.com>) id 1Uyjgf-0008Mm-RL
	for qemu-devel@nongnu.org; Mon, 15 Jul 2013 10:23:43 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <chouteau@adacore.com>) id 1Uyjge-00077u-0S
	for qemu-devel@nongnu.org; Mon, 15 Jul 2013 10:23:37 -0400
Message-ID: <51E405E4.7080406@adacore.com>
Date: Mon, 15 Jul 2013 16:23:32 +0200
From: Fabien Chouteau <chouteau@adacore.com>
MIME-Version: 1.0
References: <1373476202-11277-1-git-send-email-chouteau@adacore.com>
	<1373476202-11277-3-git-send-email-chouteau@adacore.com>
	<CAEgOgz6FvZPB2ZTin3gxAPbvkNwTbQN-dXiOQfzN3LLAy46urQ@mail.gmail.com>
In-Reply-To: <CAEgOgz6FvZPB2ZTin3gxAPbvkNwTbQN-dXiOQfzN3LLAy46urQ@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet
 Controller (eTSEC)
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, agraf@suse.de

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 <chouteau@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);

?


>> +
>> +    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?

>> +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