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

[Andy Fleming]

On Mon, Jan 30, 2012 at 3:30 PM, Troy Kisky
<troy.kisky at boundarydevices.com> wrote:
> On 1/29/2012 7:26 PM, Andy Fleming wrote:
>>
>> NAK, for reasons listed below.
>>
>> 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.


It seems that way, but, as I said, it can lead to more problems in the
future. It also makes the reasons for the initialization sequences
much less comprehensible.


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


Calling board_phy_config() instead of drv->config() might be the most
straightforward solution. You don't have to have duplicate code,
though. If a given set of boards will have the same initialization
sequence, then share the code between them. Also, as these sequences
are all probably either enabling/disabling well-understood features,
or configuration, you could put those functions in the PHY driver
code, and your board code ends up looking like:

/* Muxing on bus adds extra delay on RX path */
ksz9021_set_rx_delay(phydev, 12ns);

/* New isolinear circuits go to 11 */
ksz9021_adjust_resonance_frequency(11);

/* Accidental feedback loop caused PHY to achieve homicidal rage */
ksz9021_disable_emotion_chip(phydev);


And there are other factors, which could be handled *generically* by
the config() function for your PHY:

if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
   int cfg = phy_read(phydev, KSZ9021_CONFIG_SPECIAL);
   cfg |= KSZ021_SGMII_MODE;
   phy_write(phydev, KSZ9021_CONFIG_SPECIAL, cfg);
} else ...


That's the best solution, as it helps to contain knowledge about the
PHY inside the driver. However, there are definitely times when the
PHY requires some very specific configuration steps, which are best
contained in board code.

Summary: Yes, I think it would be a good idea to convert all the
current calls to phydev->drv->config to board_phy_config(), which
should default to just calling drv->config().

>
>
>>
>>
>>> 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 meant you should follow the example of marvell.c or vitesse.c, and
put all Micrel PHY code in the micrel.c file. Frequently, the
different PHYs from the same companies share common features. We may
divide them up differently, later, but, if anything, this driver
should be the "micrel" driver, and the existing one is just a stub.

The idea of allowing the Micrel stub PHY to exist was so that someone
might come along one day, and decide to add to its functionality. It
seems likely that your code may get repurposed by some other developer
to better support the 804 on her board.


>
>
> 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?


That's fine by me, though I think it's a bit overkill. As I said,
there are other examples of collecting multiple PHY drivers into one
file. It wastes a little space and time, but only a little, to my
mind. If we want to be more precise, we can always add more specific
#ifdefs. I just want to avoid a situation where we have hundreds or
thousands of PHY drivers, and right now we've standardized on one init
function and one file per PHY vendor.

Andy