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

[Mike Frysinger]

On Thu, Apr 7, 2011 at 10:16 AM, Andy Fleming wrote:
> 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.

you should of course know that checkpatch is simply another tool.  it
does not get the final say on things.

however, you should still know that style issues in unrelated code
belongs in a sep changeset.  if you're changing the code anyways (by
moving/deleting/whatever), then that's one thing.  but this patch is
full of hunks where *only* style is mangled.

>>> +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.

i can live with the genphy_driver always being there as a safety net
(plus that code gets re-used quite a bit by other phy drivers), but i
dont think the 10G for everyone makes sense.  even if there was a
driver in the tree that used it right now, it doesnt make sense to
force onto everyone.

i'd venture to say that even today, 1GB is not that common.  certainly
not to the degree that 100mbit is.

>>> +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.

true, but the INIT_LIST_HEAD() call would still be useless runtime
overhead.  since you need to allocate/zero the data anyways, right ?

>>> +/* 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?

no, i really would like to keep the structure mirrored.  copy the file
and rip out the stuff you dont want.  after having dealt with other
network related headers which took the approach you have here, i can
say that what i'm suggesting makes things a lot easier (at least for
me).

also, please fix your e-mailer to properly wrap long lines.
-mike