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

[Peter Crosthwaite]

Hi Fabien,

On Tue, Jul 16, 2013 at 12:23 AM, Fabien Chouteau <address@hidden> 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 <address@hidden> 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
>