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