Re: Net - new API for handling PHY devices

[Gary Thomas <gary at mlbassoc dot com>]

Andrew,

These are all reasonable questions/comments - that's why I said
it was a first cut :-)

On Wed, 2003-08-20 at 01:37, Andrew Lunn wrote:
> On Tue, Aug 19, 2003 at 11:27:36AM -0600, Gary Thomas wrote:
> > Still rough, but at least a framework for handling these devices.
> 
> > Index: devs/eth/phy/current/ChangeLog
> > +cdl_package CYGPKG_DEVS_ETH_PHY {
> > +    display       "Ethernet transciever (PHY) support"
> > +    description   "API for ethernet PHY devices"
> 
> CYGPKG_DEVS_ETH_PHY seems a rather generic name, when you are only
> supporting one way of talking to the phy. This code uses bit banging,
> but i could imagine some ethernet chips with built in shift registers
> which you just need to give bytes or words to. Maybe you should call
> this package .._ETH_PHY_BITBANG, and leave .._ETH_PHY for a generic
> layer which goes over all possible PHY access packages?
> 

I intend to add this soon.  I added this layer to handle the one
set of devices that I was working on, which happens to need the
bit banging.  I'm now working on another which does it differently,
so expect to see that fixed soon.

> > +
> > +    parent        CYGPKG_IO_ETH_DRIVERS
> > +
> > +    include_dir   cyg/io
> 
> If the parent is in the io tree and the include files go into the io
> tree, it seems better to me to put this package also in the io src tree.
> 

I think this is correct as is - it follows the pattern of other
ethernet devices (generic and specific).

> > +// Transceiver mode
> > +#define PHY_BMCR             0x00    // Register number
> > +#define PHY_BMCR_RESET       0x8000
> > +#define PHY_BMCR_LOOPBACK    0x4000
> > +#define PHY_BMCR_100MB       0x2000
> > +#define PHY_BMCR_AUTO_NEG    0x1000
> > +#define PHY_BMCR_POWER_DOWN  0x0800
> > +#define PHY_BMCR_ISOLATE     0x0400
> > +#define PHY_BMCR_RESTART     0x0200
> > +#define PHY_BMCR_FULL_DUPLEX 0x0100
> > +#define PHY_BMCR_COLL_TEST   0x0080
> 
> Where have these come from? Are they part of some standard, or
> specific to one PHY? There are some obvious things missing, eg 1000MB,
> UTP, BNC. AUI.  

Indeed.  These match the most common (like the LXT97x), but I should
make it a little "richer"

> 
> > +// Physical device access - defined by hardware instance
> > +typedef struct {
> > +    void (*init)(void);
> > +    void (*set_data)(int);
> > +    int  (*get_data)(void);
> > +    void (*set_clock)(int);
> > +    void (*set_dir)(int);
> > +} eth_phy_access_t;
> 
> Some of the device drivers support multiple instances of devices, eg
> the i82559. The phy access functions should also support multiple
> devices. Its necessary to add an extra parameter to all these
> functions, struct eth_drv_sc *sc. 

Sorry, no, I needed to do this the other way around.  Look at how
it's done on the Rattler.  I actually put the pointer to this table
(instance) in the "sc" table for each device.  This was necessary 
because the functions have to be unique.  Also, doing it this way
makes coding of the "bit banging" routines simpler and more efficient.

> 
> > +externC void _eth_phy_init(eth_phy_access_t *f);
> > +externC void _eth_phy_write(eth_phy_access_t *f, int reg, int unit, unsigned short data);
> > +externC bool _eth_phy_read(eth_phy_access_t *f, int reg, int unit, unsigned short *val);
> 
> These are the external API functions from the package. Shouldn't they
> have cyg prefixes?
> 

Probably.

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