Re: [ECOS] Re: Fwd: RealTek 8139 ethernet driver

[Gary Thomas <gary at mlbassoc dot com>]

On Thu, 2003-07-31 at 01:29, Eric Doenges wrote:
> Gary Thomas wrote:
> 
> > For the most part, this code is reasonably well structured.  The few
> > things I saw at first glance were:
> >  * Lots of uses of "info->XXX".  The use of a structure with pointers
> >    like this, but the name "info" for the pointer could be improved.
> >    Much can be determined by reading code if it's obvious what things
> >    point to (and the name of the pointer object can help here)
> 
> How about 'deviceInfo' then ?

Better, but perhaps even more descriptive. For example, the 82559
driver (arguably not the best driver, but useful) uses something like
  "struct i82559* p_i82559"
Then uses of p_i82559-> pretty clearly mean "the pointer to the info
kept about this 82559 device".

> 
> >  * I'd prefer that you tone-down the rhetoric and criticism of the 
> >    RealTek documentation.  Granted this may be poor, but pointing it
> >    out in a [permanent] public place won't make it better and may 
> >    actually discourage the vendor from making such documents public
> >    at all!
> 
> If they don't make their documentation public, they are going to sell
> less chips, so I don't see this happening. This is not the WiFi market
> where you can charge $25k for the privilige of being allowed to design
> with your chipset. However, I do agree that I should tone down the
> language (not out of respect for RealTek's feelings, but for my own
> reputation)

This argument, while seemingly valid from our perspective, holds no
sway with the hardware manufacturers.  In reality, the only way to keep
them cooperating is to be as nice to them [in public] as possible.

> 
> >  * I think there is some confusion about CYG_PHYSICAL_ADDRESS().  This
> >    is definitely needed, but there may also be a need to translate from
> >    CPU addresses to PCI addresses (on some systems, the address space
> >    differs depending on which "side" of the PCI bus the activity takes
> >    place and often by more than just a MMU-style mapping).
> 
> There is definitely confusion on my part. Unfortunately, I couldn't find
> anything on this in either the offical eCos documentation or
> Anthony Massa's book "Embedded Software Development with eCos", so I had
> to go by trial and error and use what works on my platform. I'm open to
> suggestions on how to do it right on all platforms.
> 
> >  * I don't think that the PCI lookup table belongs in the code specific
> >    to the use/instance.  The PCI code(s) [and thus boards] supported by
> >    the driver will be the same whether this driver is used by a PC or
> >    an ARM based target.
> 
> I agree with you in principle, but since vendor and device ID can be set
> by the serial eeprom connected to the 8139, I'm not 100% shure about
> this. There seem to be some vendors out there who don't understand
> the whole point behind PCI device and vendor IDs and use whatever they
> fancy.

That would be an indication of broken hardware (IMHO).  Nonetheless, 
which devices (IDs) your driver is ready to handle is an attribute of
the generic driver, not the specific instance, so I still think this 
table belongs there.

> 
> >  * There is no need to make devs/eth/rltk/8139/current/include/if_8139.h
> >    public - put it in the /src directory where it can be used by the
> >    driver.
> 
> Ok.
> 
> >  * Look again at the calling sequence for the "control" function.  It
> >    only should return 0 or 1 - not EINVAL and ESUCCESS.  There's no
> >    need for these to be used by a hardware driver.
> 
> I basically copied the code from the Intel 82559 driver, which returns
> 0, -1 and -2. (Rereading the eCos documentation gives me the impression
> you are right and the Intel driver is wrong =8^)

More than possible and we try to fix things when we can.  I'll look into
this.

> 
> >  * "poll()" is exactly "deliver()" except used by non-interrupt driven
> >    environments.  Look at some other drivers to see how this is handled.
> 
> I've looked at the Intel 82559 driver, and it seems to be doing more or
> less what I'm doing.

If you look at other drivers you'll find a much cleaner structure.  Your
driver (and probably the Intel one as well) over-complicate this.

> 
> >  * When an interrupt occurs, it is normally preferable to mask the
> >    interrupt using HAL_INTERRUPT_MASK() (or cyg_drv_interrupt_mask())
> >    rather than fiddling with the interrupt registers on the chip.  
> >    Again, there are many examples of how this is done.
> 
> I disagree. We are talking about PCI interrupts, which may be shared
> (and are, on a number of platforms). If I disable the complete
> interrupt, I disable all devices sharing that interrupt pin. I don't
> think it's acceptable to do this when the interrupt can only be
> reenabled from code run in a thread.
> 

This would be a philosophical difference between us then - I believe in
using the HAL interfaces whenever possible.  Not to worry, this wouldn't
keep (me, at least) from accepting the driver into the sources.

Thanks for your efforts.  I look forward to seeing your changes.

-- 
Gary Thomas <gary@mlbassoc.com>
MLB Associates


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss