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