Re: [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC)

[Fabien Chouteau <chouteau@adacore.com>]

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