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:
- There's only one PHY in the whole system
- 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:
- 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