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

[Andy Fleming]

On Wed, Aug 22, 2012 at 2:59 PM, Troy Kisky
<troy.kisky at boundarydevices.com> wrote:
> On 8/20/2012 5:35 PM, Andy Fleming wrote:
>
>
>
> On Monday, August 20, 2012, Troy Kisky wrote:
>>
>> It is useful to be able to try a range of
>> possible phy addresses to connect.
>
>
>
> This seems like it just encourages a bad habit.
>
>
> Which is?


The bad habit is that many device drivers make an assumption about the
MDIO bus topology. These assumptions sit there and cause problems,
until one day someone does something new, and breaks the driver. The
only assumption I'm happy with is that the PHY won't change while the
interface is running. Though even that starts to break down in some
cases.


>
>
> How do you envision this working on a system with multiple Ethernet
> controllers? Or with more PHYs than Ethernet controllers?
>
>
> 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.


>
>
> While it is often the case that the PHY is the only one on a bus, I think
> it's a bad idea to codify that notion in the driver (I know, it was already
> like that).
>
>
> 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.

For a somewhat elaborate example of this, look at drivers/net/tsec.c.
tsec_standard_init() and tsec_eth_init().

Andy