[U-Boot] [PATCH 3/4] net: phy: add support for Micrel's KSZ9021

[Troy Kisky]

On 1/29/2012 7:26 PM, Andy Fleming wrote:
> NAK, for reasons listed below.
>
> On Thu, Jan 26, 2012 at 4:21 PM, Troy Kisky
> <troy.kisky at boundarydevices.com>  wrote:
>> Add the gigabit phy KSZ9021.
>>
>> Signed-off-by: Troy Kisky<troy.kisky at boundarydevices.com>
> These commit messages are a bit over-brief. Please explain a bit more
> if there's anything non-trivial in the patch.
>
>
>> ---
>>   drivers/net/phy/Makefile         |    1 +
>>   drivers/net/phy/micrel_ksz9021.c |  124 ++++++++++++++++++++++++++++++++++++++
>>   drivers/net/phy/phy.c            |    3 +
>>   3 files changed, 128 insertions(+), 0 deletions(-)
>>   create mode 100644 drivers/net/phy/micrel_ksz9021.c
>>
> [...]
>
>> diff --git a/drivers/net/phy/micrel_ksz9021.c b/drivers/net/phy/micrel_ksz9021.c
>> +
>> +/* This is used to set board specific things like clock skew */
>> +unsigned short ksz9021_por_cmds[] = {
>> +#ifdef CONFIG_PHY_MICREL_KSZ9021_INIT_CMDS
>> +       CONFIG_PHY_MICREL_KSZ9021_INIT_CMDS
>> +#endif
>> +       0, 0
>> +};
>> +
>> +int ksz9021_send_phy_cmds(struct phy_device *phydev, unsigned short* p)
>> +{
>> +       for (;;) {
>> +               unsigned reg = *p++;
>> +               unsigned val = *p++;
>> +               if (reg == 0&&  val == 0)
>> +                       break;
>> +               if (reg<  32) {
>> +                       phy_write(phydev, MDIO_DEVAD_NONE, reg, val);
>> +               } else {
>> +                       phy_write(phydev, MDIO_DEVAD_NONE,
>> +                                       MII_KSZ9021_EXTENDED_CTRL, reg);
>> +                       phy_write(phydev, MDIO_DEVAD_NONE,
>> +                                       MII_KSZ9021_EXTENDED_DATAW, val);
>> +               }
>> +       }
>> +       return 0;
>> +}
>
> For instance, some wacky data-driven initializer function which
> totally works around any attempts the community might make at dealing
> with initialization in a transparent and functional way, and is
> essentially a whole (undocumented) API in and of itself.
This should be a separate patch including documentation. But that 
doesn't sound
sufficient for you.
>
> Basically, this is the sort of capability that all PHYs need, and if
> the current framework isn't sufficient to this PHY's needs, then we
> should look at modifying PHY Lib to provide better board-level
> initialization capabilities.
>
> An earlier version of something like PHY Lib had a similar type of
> mechanism, but a list of register writes is often insufficient to the
> task of performing necessary initialization. Sometimes, one needs to
> wait until a write takes effect, or do a different write based on link
> state information, or make other sorts of decisions.
I thought that was a clean method of keeping board specific things in 
the board.h file,
without needing to duplicate code in the board.c file.

Please suggest an alternative method. Right now, I'm just dead in the 
water and
cannot see the way forward. Do you want a more complete API? Or do you want
want this as a function in the board file?

I could use board_phy_config, but that would mean duplicated code in 
other board files.
The only sticking point is that board_phy_config is called after 
drv->config, not before or
instead of. If it was changed to instead of, I could add the call to 
phydev->drv->config(phydev)
to all existing board_phy_config functions. Currently that is

board/freescale/corenet_ds/eth_p4080.c
board/freescale/mpc8544ds/mpc8544ds.c

>
>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index eb55180..7e1e4b6 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -438,6 +438,9 @@ int phy_init(void)
>>   #ifdef CONFIG_PHY_MICREL
>>         phy_micrel_init();
>>   #endif
>> +#ifdef CONFIG_PHY_MICREL_KSZ9021
>> +       phy_ksz9021_init();
>> +#endif
> I believe we're sorting this list alphabetically....
>
> And now I see that this is actually a Micrel PHY. Why are you making a
> separate file for it? Put this code in the micrel.c file.

So, you want ifdefs in the micrel.c file?  That seems unnecessarily ugly.
The micrel file currently supports only the ksz804 and uses only
genphy_xxx functions. Perhaps renaming this file to micrel_ksz804.c
would be more appropriate. Alternatively, change the name to genphy.c
and change the structure to
static struct phy_driver genphy_driver = {
     .name = CONFIG_GENPHY_NAME,
     .uid = CONFIG_GENPHY_UID,
     .mask = CONFIG_GENPHY_MASK,
     .features = PHY_BASIC_FEATURES,
     .config = &genphy_config,
     .startup = &genphy_startup,
     .shutdown = &genphy_shutdown,
};

So that all phys which are generic can be easily defined by the board which
uses it.


I think phy_init should be changed to traverse a section created by the
linker so that all these ifdefs could disappear.  Something like

#define __phy_init_section   __attribute__ ((__section__ 
(".data.phy_init_section")))
typedef int(*phy_init_rtn)(void);

static phy_init_rtn ksz9021_phy_init_rtn __phy_init_section = 
phy_ksz9021_init;

extern phy_init_rtn __phy_init_section_start, __phy_init_section_end;

int phy_init(void)
{
     phy_init_rtn* rtn = &__phy_init_section_start;
     while (rtn < &__phy_init_section_end) {
         rtn();
         rtn++;
     }
}

But that is beyond the scope of this patch. Would you like a patch to 
convert to this
before this patch?

>
>>   #ifdef CONFIG_PHY_NATSEMI
>>         phy_natsemi_init();
>>   #endif