[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Etherne
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC) |
Date: |
Tue, 16 Jul 2013 11:06:17 +1000 |
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
>
- [Qemu-ppc] [PATCH 0/2] Enhanced Three Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/10
- Re: [Qemu-ppc] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Scott Wood, 2013/07/15
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/16
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Alexander Graf, 2013/07/16
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/16
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Scott Wood, 2013/07/16
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/17
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Alexander Graf, 2013/07/17
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/17
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Scott Wood, 2013/07/16