[U-Boot] [PATCH V1 1/3] phy: add phy_connect_by_mask

[Joe Hershberger <joe.hershberger@gmail.com>]

Hi Andy,

On Wed, Aug 22, 2012 at 3:40 PM, Andy Fleming <afleming@gmail.com> wrote:
> On Wed, Aug 22, 2012 at 2:59 PM, Troy Kisky
> <troy.kisky@boundarydevices.com> wrote:
>> On 8/20/2012 5:35 PM, Andy Fleming wrote:
>> The same way it works currently. I removed no features.
>
>
> Agreed. But the way it works currently is bad. Many drivers do this:
>
> include/configs/board.h:
> #define MY_PHY_ADDR x
>
> drivers/net/myenet.c:
>
> ...
> phydev = phy_connect(blah, blah, MY_PHY_ADDR);
>
> This is a bad idea, because it encodes the implicit assumptions that:
>
> 1) There's only one PHY in the whole system
> 2) The PHY address can be known at compile time
>
> Later, when someone adds a second ethernet controller, a frequent
> solution is then to make a MY_PHY_ADDR1, MY_PHY_ADDR2, and then add
> some logic to the driver to pick which one to use. In general, this
> makes the driver brittle, as adding and rearranging controllers is
> fairly easy for logic designers, who don't have to care whether their
> new logic will continue to operate the old chip.
>
> Alternatively, when someone causes the PHY address to vary such that
> assumption #2 is violated, it's not uncommon to solve it by searching
> the bus. But this further entrenches assumption #1.
>
>

Just to underscore this, I'm currently working on a product with 2
MACs and 2 PHYs... both PHYs share the MDIO bus of the first MAC.
It's convenient for hardware since they only have to use up pins for
one MDIO bus.  I definitely want to get to a point where supporting
this sort of topology is cleaner and easier.

>> So, should I fix something before this patch?
>
> My thought is that your solution is quite elegant, but further
> entrenches the assumption that there will be only one ethernet
> controller. In my mind, the best way to solve this is:
>
> 1) Modify the driver so that the PHY address is passed in from board
> initialization code programmatically. As a nod to the effort of doing
> so for all boards, you can create a default value (ie - as it was),
> that can be overridden by board code.
> 2) Modify the search function to look for a valid PHY for a given
> mask, and return the address of that PHY
> 3) Add code to the board file which passes in the mask to the search
> function, and then passes the resulting PHY address to the driver.

I think this sounds good.

-Joe