[U-Boot] [PATCH v2 4/6] Create PHY Lib for U-Boot

[Andy Fleming]

On Apr 6, 2011, at 6:09 PM, Mike Frysinger wrote:

> On Tuesday, April 05, 2011 17:59:52 Andy Fleming wrote:
>> -#define debug(fmt,args...)	printf (fmt ,##args)
>> +#define debug(fmt, args...)	printf(fmt, ##args)
> 
> it'd be nice if all these unrelated formatting changes werent intermingled.  
> but i guess too hard for you to untangle now.  make it a pita to pick out what 
> are functional changes and what is pure noise.


A lot of that's due to checkpatch errors that hit on lines I changed, so I went through and changed all instances.


> 
>> +struct mii_dev *mdio_alloc(void)
>> +{
>> +	struct mii_dev *bus;
>> +
>> +	bus = malloc(sizeof(*bus));
>> +	if (!bus)
>> +		return bus;
>> +
>> +	memset(bus, 0, sizeof(*bus));
>> +
>> +	bus->name = malloc(MDIO_NAME_LEN);
> 
> considering the name len is hardcoded at build time, it'd be nice to inline 
> that into the struct itself to avoid having to do multiple mallocs.  but i 
> guess it might be hard to keep working with the legacy code ?  or maybe not if 
> you just grep the tree to make sure no one using legacy code has a name longer 
> than 31 bytes ...


Hmmm... I can go look, but it's really hard to make sure I've found all instances of ->name that are miiphy-related.


> 
>> +struct phy_device *mdio_phydev_for_ethname(const char *ethname)
>> ...
>> +			if ((!bus->phymap[i]) || (!bus->phymap[i]->dev))
> 
> useless paren around both expressions here
> 
>> --- /dev/null
>> +++ b/drivers/net/phy/phy.c
>> 
>> +int phy_read(struct phy_device *phydev, int devad, int regnum)
>> +{
>> +	struct mii_dev *bus = phydev->bus;
>> +
>> +	return bus->read(bus, phydev->addr, devad, regnum);
>> +}
>> +
>> +int phy_write(struct phy_device *phydev, int devad, int regnum, u16 val)
>> +{
>> +	struct mii_dev *bus = phydev->bus;
>> +
>> +	return bus->write(bus, phydev->addr, devad, regnum, val);
>> +}
> 
> seems like it'd make sense for these to be inlines in the phy header


Sure.


> 
>> +static struct phy_driver gen10g_driver = {
>> +	.uid		= 0xffffffff,
>> +	.mask		= 0xffffffff,
>> +	.name		= "Generic 10G PHY",
>> +	.features	= 0,
>> +	.config		= gen10g_config,
>> +	.startup	= gen10g_startup,
>> +	.shutdown	= gen10g_shutdown,
>> +};
> 
> this probably should be split out into a dedicated phy driver.  you might care 
> about it, but i cant think of any board atm where i would use this.  i imagine 
> for most people, it's simply useless bloat.
> 
>> +static struct phy_driver genphy_driver = {
>> +	.uid		= 0xffffffff,
>> +	.mask		= 0xffffffff,
>> +	.name		= "Generic PHY",
>> +	.features	= 0,
>> +	.config		= genphy_config,
>> +	.startup	= genphy_startup,
>> +	.shutdown	= genphy_shutdown,
>> +};
> 
> i think this should be split too for the board maintainers who know exactly 
> why phy they're going to have in their system.


Hmmm... I'm of two minds about this.  Admittedly, the 10G stuff is currently floating unused, and will only be slightly less so when I can get our 10G driver pushed out, but these drivers are meant to serve as the backstop for the driver detection process.  So I can see board authors eliminating the drivers because they've properly enabled the one driver they need, but I will need to rework the connect code to exit more cleanly if they did it wrong.

As a second note, if you look carefully at the follow-on driver code, you'll see that (for genphy, at least), it's not actually much bloat if you use most drivers.  The genphy_startup and genphy_shutdown functions are used all of the time by the drivers, as is genphy_config_aneg().

I also hope to add some more interesting generic 10G code at some point, which would make the 10G driver less bloaty as well.  Well, more bloated, but also more useful.  I can add a CONFIG which controls the existence of 10G support.  I'll take a look at what would be needed to cut out the generic drivers (via CONFIG option), but I suspect that all you'll save is the phy_driver struct, itself, plus the genphy_config() function.  The rest are needed by various PHY drivers.


> 
>> +static struct list_head phy_drivers;
>> +
>> +int phy_init(void)
>> +{
>> +	INIT_LIST_HEAD(&phy_drivers);
>> +
>> +	return 0;
>> +}
> 
> isnt there a macro for declaring/initializing a list structure statically ?  
> then we could avoid this useless phy_init() call.


This will do all of the driver initialization, it just looks useless in this patch.


> 
>> +/* Indicates what features are supported by the interface. */
>> +#define SUPPORTED_10baseT_Half		(1 << 0)
>> +#define SUPPORTED_10baseT_Full		(1 << 1)
>> +#define SUPPORTED_100baseT_Half		(1 << 2)
> 
> this stuff looks suspiciously like it was copy & pasted from linux/ethtool.h.  
> why not just copy the file over instead of creating your own custom variant ?

Well, I'm torn.  It is definitely copied.  But include/linux/ethtool.h is 796 lines, and I've copied less than 100 of them.  Most of the header is concerned with the ethtool command, which is definitely not appropriate for U-Boot.  I should add attribution, definitely.  Maybe what should be done is to take those PHY-specific bits, and move them to a separate header in Linux, and then copy *that* header to u-boot.  Perhaps just copy them into mii.h, which is already reflected in U-Boot?

Andy