Message-ID: <54EB6820.2040300@gmail.com>
Date:	Mon, 23 Feb 2015 09:49:20 -0800
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Tobias Waldekranz <tobias@...dekranz.com>
CC:	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/2] dsa: mv88e6131: support fixed PHYs

On 23/02/15 03:13, Tobias Waldekranz wrote:
> On Sat, Feb 21, 2015 at 10:56:25AM -0800, Florian Fainelli wrote:
>> 2015-02-21 2:30 GMT-08:00 Tobias Waldekranz <tobias@...dekranz.com>:
>>> On Thu, Feb 12, 2015 at 08:13:28AM -0800, Florian Fainelli wrote:
>>>> 2015-02-12 6:13 GMT-08:00 Tobias Waldekranz <tobias@...dekranz.com>:
>>>>> Statically setup the PCS Control on the MAC to match the fixed PHY.
>>>>
>>>> bcm_sf2 supports both fixed PHYs and regular PHYs, yet we do not need
>>>> to get access to the fixed PHY status from the adjust_link callback
>>>> because you could implement a separate fixed_link_update callback for
>>>> that purpose.
>>>>
>>>> Did not that work for you?
>>>>
>>>
>>> That was my first approach and it worked fine. The only issue I saw
>>> was that the callback was continously called at each poll cycle even
>>> though the link state had not changed.
>>
>> Right, we poll using this callback fairly often. Just like the PHY
>> libraries adjust_link, drivers are responsible for implementing
>> "caching" and avoiding the callback to be invoked too frequently.
>>
>>>
>>> So then I implemented the same check for updates that was in the
>>> regular adjust_link callback. But before I submitted that version of
>>> the patch I looked att the sf2 code, and it seemed as though this code
>>> uses the callback to update the phy status based on the chip state and
>>> not the other way around. Did I misunderstand the code?
>>
>> It is a two step process:
>>
>> - fixed_link_update updates the fixed PHY's status member to reflect
>> the HW changes (link change mostly), updates the PHY device
>> speed/duplex/pause parameters
>>
>> - adjust_link reads the PHY device speed/duplex/pause and applies
>> these to the HW registers
> 
> Right, in my case I just need to do an initial config according to the
> fixed settings which are read from the device-tree.
> 
> In the case where there is a real PHY attached to the switch, an
> internal machine will poll the PHY and setup the MAC accordingly. So
> HW will take care of step 2 for me.

Even if this is not a real PHY device, but e.g: a MAC-to-MAC type of
connection? Does the PHY PPU rely on being able to issue MDIO accesses?

> 
>>>
>>> Not wanting to break your code, I went with this approach instead. But
>>> if you're fine with it, I'm more than happy to go with that version.
>>
>> I think it is a little cleaner since the adjust_link callback does not
>> need to know what kind of PHY device it is dealing with, while the
>> fixed_link_update() one only takes care of fixed PHYs.
> 
> In my case I do need to know, since I want the switch's phy polling
> unit to do the work when possible. Maybe I should just rethink the
> whole thing and do the setup at probe-time using some other method.

You could do that, but it might be a little cumbersome, something
simpler could be just to have an initialization flag that ensures that
both of these callbacks run for at least once, and then do nothing?

> 
> I will get back to you with a better solution :)
> 
>> Thanks
>> -- 
>> Florian
> 


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists