[linux-cirrus] Re: new ep93xx ethernet driver
- From: Lennert Buytenhek <buytenh@xxxxxxxxxxxxxx>
- To: Herbert Valerio Riedel <hvr@xxxxxxx>
- Date: Sat, 8 Jul 2006 12:56:21 +0200
On Sat, Jul 08, 2006 at 06:01:55AM +0200, Herbert Valerio Riedel wrote:
> > My TODO list for the ethernet driver is:
> > - tx restart on critical error
> > - on init, read PHY preamble suppress bit and program into MAC
> > - tx timeout handling (->tx_timeout, ->watchdog_timeo)
> > - sort out max mii clock, use phylib?
> > - use dev->base_addr / dev->irq?
> > - on link up, check duplex mode and program MAC accordingly (CRS handling)
> > - get rid of busy waits
> > - set pkt thresholds correctly
> > - ->phy_power() platform hook so that we can turn the PHY off when the
> > interface is down, to conserve power, on boards that support it (ts72xx
> > does)
> > - try cheating with unaligned buffers on rx or tx
> > - reduce static memory usage
>
> btw, have you thought about using the new generic PHY framework for
> reducing PHY related code duplication, ...? :-)
I've looked at phylib, and as far as I can see, adding support for it
would only increase code duplication, at least in the short term, until
phylib is sorted out a bit more. For example, the struct mii_if_info we
need to pass into generic_mii_ioctl() requires these mdio read/write
prototypes:
static int ep93xx_mdio_read(struct net_device *dev, int phy_id, int reg);
static void ep93xx_mdio_write(struct net_device *dev, int phy_id, int reg, int
data);
Whereas phylib's mii_bus abstraction requires:
int gfar_mdio_read(struct mii_bus *bus, int mii_id, int regnum);
int gfar_mdio_write(struct mii_bus *bus, int mii_id, int regnum, u16 value);
I had a quick look at phylib some days ago, and there's a bunch of
stuff that just isn't right. For example, this will happily make it
detect my 88E1111 and 88E1145 PHYs as 88E1101:
static struct phy_driver m88e1101_driver = {
.phy_id = 0x01410c00,
.phy_id_mask = 0xffffff00,
.name = "Marvell 88E1101",
Also, phylib uses disable_irq(), which isn't so very nice. (But hard to
avoid, I suppose.. :-/)
It doesn't really help that there aren't a lot of in-tree users of
phylib, so it's not getting a lot of code review as far as I can see.
In my original todo-list, there were these two phy-related items:
> > - sort out max mii clock, use phylib?
> > - ->phy_power() platform hook so that we can turn the PHY off when the
> > interface is down, to conserve power, on boards that support it (ts72xx
> > does)
For the first item, it would be useful if phylib knew about the
PHY-specific maximum MDIO clock rating, so that we could slow down
MDIO accesses for slow PHYs, and increase the clock rate for PHYs
that can handle higher MDIO rates. However, phylib doesn't store
this information currently, and the better option is probably to
make MDIO access preemptable so that we don't really care about
how long it takes.
For the second item, this would involve a per-board GPIO write or
CPLD register write, and wouldn't involve phylib at all.
cheers,
Lennert
Other related posts:
