Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver

[Jean-Christophe DUBOIS]

On 05/05/2013 03:31 PM, Andreas Färber wrote:
> Am 05.05.2013 15:14, schrieb Jean-Christophe DUBOIS:
> > On 05/05/2013 05:11 AM, Peter Crosthwaite wrote:
> > > Hi JC,
> > >
> > > On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
> > > <address@hidden>  wrote:
> > > > This is based on the mcf_fec.c FEC implementation for ColdFire.
> Note that ColdFire is one of the least maintained targets in QEMU...

Well, that's too bad.

I actually believe the ColdFire FEC implementation would not work in 
actual QEMU (but I don't plan to build a ColdFire Kernel and RootFS to 
find out).

> > > > +static int imx_fec_can_receive(NetClientState *nc)
> > > > +{
> > > > +    imx_fec_state *s = qemu_get_nic_opaque(nc);
> > > > +
> > > Andreas, do we care about QOM casts coming from random void* opaques?
> Generally no. If we ever switch to C++ or some other OO language we'll
> have to touch casts anyway.
>
> Peter, please note that I usually don't have time to read through all
> patches - noticed this inline question incidentally only.
>
> > OK, I am going to do a QOM cast anyway.
> You should check how frequently this function is called - the current
> OBJECT_CHECK() implementation does at least one string comparison, so
> I'd suggest to avoid it here. That is, if the type passed in as opaque
> matches the type assigned here (thinking of interfaces).

Hum, ... you are going to be unhappy with my new patch.

> > > > +    return s->rx_enabled;
> > > > +}
> [snip]
>
> Not knowing this piece of hardware, might it make sense to call it
> "imx-fec" when there's another FEC for ColdFire?

I am calling it "imx.fec" in the new version of the file (to be 
equivalent to "imx.i2c" I did for I2C).

>   Or are they the same
> thing and should be unified?
Well, they are certainly very similar but are different on a few points. 
In particular the "buffer descriptors" are reversed between i.MX and 
Coldfire. Linux does a compile time decision about these buffer 
descriptors but I don't know how this would work with Qemu (can you make 
a different compile time decision for 2 different targets?).

We could also implement a more cumbersome run time decision but I wanted 
to keep simple.

And anyway I don't have Coldfire (cross compilers, rootfs, ...) setup 
around to test.

> Regards,
> Andreas