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

[Troy Kisky]

On 1/30/2012 5:05 PM, Andy Fleming wrote:
> 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.

I'll take your word, as I have more difficulty imagining your future.

>
>
>> 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().
Can do.
Thanks for providing a direction.

>
>>
>>>
>>>> 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'm doubtful, since that code has not changed since your initial commit 
back in April.
Is anyone still using it today? Seems only powerPC platforms with 
CONFIG_TSEC_ENET
have had a chance. But I guess that is an irrelevant tangent.

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

Yes, if some code is shared. I does make sense to me to have both in the 
same file or
have a separate file with the shared code rather than cut-n-pasting the 
code.

However, a hard rule of only one file per vendor doesn't make sense to me.
If Micrel buys out vitesse will you combine the files? I doubt it.

On the other hand, as this is way way way down on the list of things I 
care about,
and seems much higher on your list, putting it in micrel.c is fine with me.


>
> Andy
>