[U-Boot] [PATCH V1 1/3] phy: add phy_connect_by_mask
It is useful to be able to try a range of possible phy addresses to connect.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com
--- v2: no change --- drivers/net/phy/phy.c | 108 +++++++++++++++++++++++++++++++------------------ include/phy.h | 2 + 2 files changed, 71 insertions(+), 39 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index baef60f..a22d2e0 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -31,6 +31,7 @@ #include <miiphy.h> #include <phy.h> #include <errno.h> +#include <linux/err.h>
/* Generic PHY support and helper functions */
@@ -573,6 +574,61 @@ int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id) return 0; }
+static struct phy_device *create_phy_by_mask(struct mii_dev *bus, + unsigned phy_mask, int devad, phy_interface_t interface) +{ + u32 phy_id = 0xffffffff; + while (phy_mask) { + int addr = ffs(phy_mask) - 1; + int r = get_phy_id(bus, addr, devad, &phy_id); + if (r < 0) + return ERR_PTR(r); + /* If the PHY ID is mostly f's, we didn't find anything */ + if ((phy_id & 0x1fffffff) != 0x1fffffff) + return phy_device_create(bus, addr, phy_id, interface); + phy_mask &= ~(1 << addr); + } + return NULL; +} + +static struct phy_device *search_for_existing_phy(struct mii_dev *bus, + unsigned phy_mask, phy_interface_t interface) +{ + /* If we have one, return the existing device, with new interface */ + while (phy_mask) { + int addr = ffs(phy_mask) - 1; + if (bus->phymap[addr]) { + bus->phymap[addr]->interface = interface; + return bus->phymap[addr]; + } + phy_mask &= ~(1 << addr); + } + return NULL; +} + +static struct phy_device *get_phy_device_by_mask(struct mii_dev *bus, + unsigned phy_mask, phy_interface_t interface) +{ + int i; + struct phy_device *phydev; + + phydev = search_for_existing_phy(bus, phy_mask, interface); + if (phydev) + return phydev; + /* Try Standard (ie Clause 22) access */ + /* Otherwise we have to try Clause 45 */ + for (i = 0; i < 5; i++) { + phydev = create_phy_by_mask(bus, phy_mask, + i ? i : MDIO_DEVAD_NONE, interface); + if (IS_ERR(phydev)) + return NULL; + if (phydev) + return phydev; + } + printf("Phy not found\n"); + return phy_device_create(bus, ffs(phy_mask) - 1, 0xffffffff, interface); +} + /** * get_phy_device - reads the specified PHY device and returns its @phy_device struct * @bus: the target MII bus @@ -584,38 +640,7 @@ int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id) struct phy_device *get_phy_device(struct mii_dev *bus, int addr, phy_interface_t interface) { - u32 phy_id = 0x1fffffff; - int i; - int r; - - /* If we have one, return the existing device, with new interface */ - if (bus->phymap[addr]) { - bus->phymap[addr]->interface = interface; - - return bus->phymap[addr]; - } - - /* Try Standard (ie Clause 22) access */ - r = get_phy_id(bus, addr, MDIO_DEVAD_NONE, &phy_id); - if (r) - return NULL; - - /* If the PHY ID is mostly f's, we didn't find anything */ - if ((phy_id & 0x1fffffff) != 0x1fffffff) - return phy_device_create(bus, addr, phy_id, interface); - - /* Otherwise we have to try Clause 45 */ - for (i = 1; i < 5; i++) { - r = get_phy_id(bus, addr, i, &phy_id); - if (r) - return NULL; - - /* If the phy_id is mostly Fs, there is no device there */ - if ((phy_id & 0x1fffffff) != 0x1fffffff) - break; - } - - return phy_device_create(bus, addr, phy_id, interface); + return get_phy_device_by_mask(bus, 1 << addr, interface); }
int phy_reset(struct phy_device *phydev) @@ -688,9 +713,8 @@ int miiphy_reset(const char *devname, unsigned char addr) return phy_reset(phydev); }
-struct phy_device *phy_connect(struct mii_dev *bus, int addr, - struct eth_device *dev, - phy_interface_t interface) +struct phy_device *phy_connect_by_mask(struct mii_dev *bus, unsigned phy_mask, + struct eth_device *dev, phy_interface_t interface) { struct phy_device *phydev;
@@ -701,11 +725,11 @@ struct phy_device *phy_connect(struct mii_dev *bus, int addr, /* Wait 15ms to make sure the PHY has come out of hard reset */ udelay(15000);
- phydev = get_phy_device(bus, addr, interface); + phydev = get_phy_device_by_mask(bus, phy_mask, interface);
if (!phydev) { - printf("Could not get PHY for %s:%d\n", bus->name, addr); - + printf("Could not get PHY for %s: phy mask %x\n", + bus->name, phy_mask); return NULL; }
@@ -714,7 +738,7 @@ struct phy_device *phy_connect(struct mii_dev *bus, int addr,
if (phydev->dev) printf("%s:%d is connected to %s. Reconnecting to %s\n", - bus->name, addr, phydev->dev->name, dev->name); + bus->name, phydev->addr, phydev->dev->name, dev->name);
phydev->dev = dev;
@@ -723,6 +747,12 @@ struct phy_device *phy_connect(struct mii_dev *bus, int addr, return phydev; }
+struct phy_device *phy_connect(struct mii_dev *bus, int addr, + struct eth_device *dev, phy_interface_t interface) +{ + return phy_connect_by_mask(bus, 1 << addr, dev, interface); +} + /* * Start the PHY. Returns 0 on success, or a negative error code. */ diff --git a/include/phy.h b/include/phy.h index 3c30f11..aea462f 100644 --- a/include/phy.h +++ b/include/phy.h @@ -202,6 +202,8 @@ int phy_reset(struct phy_device *phydev); struct phy_device *phy_connect(struct mii_dev *bus, int addr, struct eth_device *dev, phy_interface_t interface); +struct phy_device *phy_connect_by_mask(struct mii_dev *bus, unsigned phy_mask, + struct eth_device *dev, phy_interface_t interface); int phy_startup(struct phy_device *phydev); int phy_config(struct phy_device *phydev); int phy_shutdown(struct phy_device *phydev);
Allow board config files to list a range of possible phy addresses, in case the exact phy address is not certain.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com
--- v2: add README.fec_mxc for config options currently used in fec_mxc --- doc/README.fec_mxc | 34 ++++++++++++++++++++++++++++++++++ drivers/net/fec_mxc.c | 21 +++++++++++++++------ drivers/net/fec_mxc.h | 3 ++- 3 files changed, 51 insertions(+), 7 deletions(-) create mode 100644 doc/README.fec_mxc
diff --git a/doc/README.fec_mxc b/doc/README.fec_mxc new file mode 100644 index 0000000..1365b96 --- /dev/null +++ b/doc/README.fec_mxc @@ -0,0 +1,34 @@ +U-boot config options used in fec_mxc.c + +CONFIG_FEC_MXC + Selects fec_mxc.c to be compiled into u-boot. + +CONFIG_MII + Must be defined if CONFIG_FEC_MXC is defined. + +CONFIG_FEC_XCV_TYPE + Defaults to MII100 for 100 Base-tx. + RGMII selects 1000 Base-tx reduced pin count interface. + RMII selects 100 Base-tx reduced pin count interface. + +CONFIG_FEC_MXC_SWAP_PACKET + Forced on iff MX28. + Swaps the bytes order of all words(4 byte units) in the packet. + This should not be specified by a board file. It is cpu specific. + +CONFIG_PHYLIB + fec_mxc supports PHYLIB and should be used for new boards. + +CONFIG_FEC_MXC_NO_ANEG + Relevant only if PHYLIB not used. Skips auto-negotiation restart. + +CONFIG_FEC_MXC_MULTI + Initializes multiple ethernet interfaces using this driver. + +CONFIG_FEC_MXC_PHYMASK + Relevant only if CONFIG_FEC_MXC_MULTI is not set. + Selects a range of phy addresses to try to connect. + +CONFIG_FEC_MXC_PHYADDR + Relevant only if CONFIG_FEC_MXC_MULTI and CONFIG_FEC_MXC_PHYMASK are + not set. Selects the exact phy address that should be connected. diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index fbfc842..4af4976 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -391,7 +391,7 @@ static void fec_eth_phy_config(struct eth_device *dev) struct fec_priv *fec = (struct fec_priv *)dev->priv; struct phy_device *phydev;
- phydev = phy_connect(fec->bus, fec->phy_id, dev, + phydev = phy_connect_by_mask(fec->bus, fec->phy_mask, dev, PHY_INTERFACE_MODE_RGMII); if (phydev) { fec->phydev = phydev; @@ -898,7 +898,8 @@ static int fec_recv(struct eth_device *dev) return len; }
-static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) +static int fec_probe(bd_t *bd, int dev_id, unsigned phy_mask, + uint32_t base_addr) { struct eth_device *edev; struct fec_priv *fec; @@ -958,8 +959,11 @@ static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) sprintf(edev->name, "FEC%i", dev_id); fec->dev_id = dev_id; } - fec->phy_id = phy_id; - +#ifdef CONFIG_PHYLIB + fec->phy_mask = phy_mask; +#else + fec->phy_id = ffs(phy_mask) - 1; +#endif bus = mdio_alloc(); if (!bus) { printf("mdio_alloc failed\n"); @@ -1008,9 +1012,14 @@ err1: int fecmxc_initialize(bd_t *bd) { int lout = 1; +#ifdef CONFIG_FEC_MXC_PHYMASK + unsigned phy_mask = CONFIG_FEC_MXC_PHYMASK; +#else + unsigned phy_mask = 1 << CONFIG_FEC_MXC_PHYADDR; +#endif
debug("eth_init: fec_probe(bd)\n"); - lout = fec_probe(bd, -1, CONFIG_FEC_MXC_PHYADDR, IMX_FEC_BASE); + lout = fec_probe(bd, -1, phy_mask, IMX_FEC_BASE);
return lout; } @@ -1021,7 +1030,7 @@ int fecmxc_initialize_multi(bd_t *bd, int dev_id, int phy_id, uint32_t addr) int lout = 1;
debug("eth_init: fec_probe(bd, %i, %i) @ %08x\n", dev_id, phy_id, addr); - lout = fec_probe(bd, dev_id, phy_id, addr); + lout = fec_probe(bd, dev_id, 1 << phy_id, addr);
return lout; } diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h index 852b2e0..a0f50af 100644 --- a/drivers/net/fec_mxc.h +++ b/drivers/net/fec_mxc.h @@ -268,11 +268,12 @@ struct fec_priv { bd_t *bd; uint8_t *tdb_ptr; int dev_id; - int phy_id; struct mii_dev *bus; #ifdef CONFIG_PHYLIB + int phy_mask; struct phy_device *phydev; #else + int phy_id; int (*mii_postcall)(int); #endif };
Different ethernet jacks parts can result in the Micrel KSZ9021 getting a different phy address. Let's support all parts.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com
---- v2: no change --- include/configs/mx6qsabrelite.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/configs/mx6qsabrelite.h b/include/configs/mx6qsabrelite.h index 782abc8..44b8c7f 100644 --- a/include/configs/mx6qsabrelite.h +++ b/include/configs/mx6qsabrelite.h @@ -99,7 +99,7 @@ #define IMX_FEC_BASE ENET_BASE_ADDR #define CONFIG_FEC_XCV_TYPE RGMII #define CONFIG_ETHPRIME "FEC" -#define CONFIG_FEC_MXC_PHYADDR 6 +#define CONFIG_FEC_MXC_PHYMASK (0xf << 4) /* scan phy 4,5,6,7 */ #define CONFIG_PHYLIB #define CONFIG_PHY_MICREL #define CONFIG_PHY_MICREL_KSZ9021
On 8/20/2012 3:48 PM, Troy Kisky wrote:
It is useful to be able to try a range of possible phy addresses to connect.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com
v2: no change
Sorry, this series should of course say PATCH V2
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. How do you envision this working on a system with multiple Ethernet controllers? Or with more PHYs than Ethernet controllers? 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).
It's best if the driver make the reasonable assumption that its PHY address is known when it comes up, and let the board code, which can be aware that the PHY may exist in varying locations, search for the PHY. With that approach, the driver won't have to change when some board designer makes the PHY topology even stranger, and I would support a PHYLIB function to do searching much as you've specified. But the board-specific code needs to be able to tell the driver definitively which PHY belongs to it.
Andy
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?
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.
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?
It's best if the driver make the reasonable assumption that its PHY address is known when it comes up, and let the board code, which can be aware that the PHY may exist in varying locations, search for the PHY.
I agree. That's why I put #define CONFIG_FEC_MXC_PHYMASK (0xf << 4) /* scan phy 4,5,6,7 */
in the boards config file.
With that approach, the driver won't have to change when some board designer makes the PHY topology even stranger, and I would support a PHYLIB function to do searching much as you've specified. But the board-specific code needs to be able to tell the driver definitively which PHY belongs to it.
Please, will either you or Joe (or both!!!,) provide more specific directions as I am currently floundering.
Thanks Troy
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
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
On 8/22/2012 1:40 PM, Andy Fleming wrote:
- 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
Thanks for providing some direction. I think I know what you are after now.
Troy
Hi Troy,
On Wed, Aug 22, 2012 at 4:21 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:
On 8/22/2012 1:40 PM, Andy Fleming wrote:
- 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
Thanks for providing some direction. I think I know what you are after now.
Are you planning to update this before the release?
Thanks, -Joe
On 9/26/2012 10:26 AM, Joe Hershberger wrote:
Hi Troy,
On Wed, Aug 22, 2012 at 4:21 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:
On 8/22/2012 1:40 PM, Andy Fleming wrote:
- 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
Thanks for providing some direction. I think I know what you are after now.
Are you planning to update this before the release?
Thanks, -Joe
Probably not. I'll try to get to it next week, but I don't think it belongs in the coming release. And it might need still another iteration.
Thanks Troy
On 08/20/2012 05: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. How do you envision this working on a system with multiple Ethernet controllers? Or with more PHYs than Ethernet controllers? 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).
It's best if the driver make the reasonable assumption that its PHY address is known when it comes up, and let the board code, which can be aware that the PHY may exist in varying locations, search for the PHY.
A mask with a single bit set is about as specific as you can get, right?
Are you asking for something more extensible, like a callback into board-specific code?
With that approach, the driver won't have to change when some board designer makes the PHY topology even stranger, and I would support a PHYLIB function to do searching much as you've specified.
The primary reason for this change is that it's not only the board designer, but also the purchasing manager or manufacturer who can change the address.
Since the PHY addresses are often driven by the state of LEDs on an ethernet connector, swapping part numbers can result in different PHY addresses. We run into this on our Nitrogen6X boards where PoE-enabled jacks result in a different PHY address from a standard jack.
But the board-specific code needs to be able to tell the driver definitively which PHY belongs to it.
Andy
On Wed, Aug 22, 2012 at 3:11 PM, Eric Nelson eric.nelson@boundarydevices.com wrote:
On 08/20/2012 05:35 PM, Andy Fleming wrote:
On Monday, August 20, 2012, Troy Kisky wrote:
It's best if the driver make the reasonable assumption that its PHY address is known when it comes up, and let the board code, which can be aware that the PHY may exist in varying locations, search for the PHY.
A mask with a single bit set is about as specific as you can get, right?
Are you asking for something more extensible, like a callback into board-specific code?
More like having the board code call into the driver, to tell it: Here's where to find your PHY.
With that approach, the driver won't have to change when some board designer makes the PHY topology even stranger, and I would support a PHYLIB function to do searching much as you've specified.
The primary reason for this change is that it's not only the board designer, but also the purchasing manager or manufacturer who can change the address.
Since the PHY addresses are often driven by the state of LEDs on an ethernet connector, swapping part numbers can result in different PHY addresses. We run into this on our Nitrogen6X boards where PoE-enabled jacks result in a different PHY address from a standard jack.
Oh, I understand, fully. My argument is actually that it's even worse than that. Imagine that there's a scenario where there's a valid PHY at 4 AND 6, but under certain circumstances, your controller is hooked up to 6. With the mask solution, the driver will always connect to 4 (assuming your mask includes 4 and 6). So now, to solve that, you have to add more logic, but because the knowledge of the PHY is entirely encoded in the ethernet driver and the board config file, there's no way to insert logic, except in the driver.
I think that many people will find the need to search the bus, and a mask like the one in this patch is a very good way. But the driver shouldn't do the searching, IMO.
Andy
On 08/22/2012 01:50 PM, Andy Fleming wrote:
On Wed, Aug 22, 2012 at 3:11 PM, Eric Nelson eric.nelson@boundarydevices.com wrote:
On 08/20/2012 05:35 PM, Andy Fleming wrote:
On Monday, August 20, 2012, Troy Kisky wrote:
It's best if the driver make the reasonable assumption that its PHY address is known when it comes up, and let the board code, which can be aware that the PHY may exist in varying locations, search for the PHY.
A mask with a single bit set is about as specific as you can get, right?
Are you asking for something more extensible, like a callback into board-specific code?
More like having the board code call into the driver, to tell it: Here's where to find your PHY.
With that approach, the driver won't have to change when some board designer makes the PHY topology even stranger, and I would support a PHYLIB function to do searching much as you've specified.
The primary reason for this change is that it's not only the board designer, but also the purchasing manager or manufacturer who can change the address.
Since the PHY addresses are often driven by the state of LEDs on an ethernet connector, swapping part numbers can result in different PHY addresses. We run into this on our Nitrogen6X boards where PoE-enabled jacks result in a different PHY address from a standard jack.
Oh, I understand, fully. My argument is actually that it's even worse than that. Imagine that there's a scenario where there's a valid PHY at 4 AND 6, but under certain circumstances, your controller is hooked up to 6. With the mask solution, the driver will always connect to 4 (assuming your mask includes 4 and 6). So now, to solve that, you have to add more logic, but because the knowledge of the PHY is entirely encoded in the ethernet driver and the board config file, there's no way to insert logic, except in the driver.
I think that many people will find the need to search the bus, and a mask like the one in this patch is a very good way. But the driver shouldn't do the searching, IMO.
Thanks for the clarification Andy. I didn't understand where you were headed and it's now clear.
Regards,
Eric
This series tries to separate the mii regsisters from the ethernet registers as suggested by Andy Fleming. Then, mx6qsabrelite is changed to find the phy address from the possibles 4-7.
The V3 series is very different from V2.
Troy Kisky (9): doc/README.fec_mxc: add documentation net: fec_mxc: delete CONFIG_FEC_MXC_MULTI net: fec_mxc: change fec_mii_setspeed parameter net: fec_mxc: have fecmxc_initialize call fecmxc_initialize_multi phy: add phy_find_by_mask/phy_connect_dev net: fec_mxc: use fec_set_dev_name to set name net: fec_mxc: only call phy_connect in fec_probe net: fec_mxc: get phydev before fec_probe mx6qsabrelite: search mii phy address 4-7
board/freescale/mx6qsabrelite/mx6qsabrelite.c | 24 +++- doc/README.fec_mxc | 27 ++++ drivers/net/fec_mxc.c | 164 ++++++++++++++----------- drivers/net/fec_mxc.h | 2 +- drivers/net/phy/phy.c | 128 +++++++++++-------- include/configs/m28evk.h | 1 - include/configs/mx28evk.h | 1 - include/configs/sc_sps_1.h | 1 - include/netdev.h | 7 ++ include/phy.h | 3 + 10 files changed, 227 insertions(+), 131 deletions(-) create mode 100644 doc/README.fec_mxc
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com
--- v2: add README.fec_mxc for config options currently used in fec_mxc --- doc/README.fec_mxc | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 doc/README.fec_mxc
diff --git a/doc/README.fec_mxc b/doc/README.fec_mxc new file mode 100644 index 0000000..72a1d59 --- /dev/null +++ b/doc/README.fec_mxc @@ -0,0 +1,27 @@ +U-boot config options used in fec_mxc.c + +CONFIG_FEC_MXC + Selects fec_mxc.c to be compiled into u-boot. + +CONFIG_MII + Must be defined if CONFIG_FEC_MXC is defined. + +CONFIG_FEC_XCV_TYPE + Defaults to MII100 for 100 Base-tx. + RGMII selects 1000 Base-tx reduced pin count interface. + RMII selects 100 Base-tx reduced pin count interface. + +CONFIG_FEC_MXC_SWAP_PACKET + Forced on iff MX28. + Swaps the bytes order of all words(4 byte units) in the packet. + This should not be specified by a board file. It is cpu specific. + +CONFIG_PHYLIB + fec_mxc supports PHYLIB and should be used for new boards. + +CONFIG_FEC_MXC_NO_ANEG + Relevant only if PHYLIB not used. Skips auto-negotiation restart. + +CONFIG_FEC_MXC_PHYADDR + Optional, selects the exact phy address that should be connected + and function fecmxc_initialize will try to initialize it.
It is more logical to test for CONFIG_FEC_MXC_PHYADDR to determine whether to define the function fecmxc_initialize.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com --- drivers/net/fec_mxc.c | 2 +- include/configs/m28evk.h | 1 - include/configs/mx28evk.h | 1 - include/configs/sc_sps_1.h | 1 - 4 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 3e232c7..6596ceb 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -1021,7 +1021,7 @@ err1: return ret; }
-#ifndef CONFIG_FEC_MXC_MULTI +#ifdef CONFIG_FEC_MXC_PHYADDR int fecmxc_initialize(bd_t *bd) { int lout = 1; diff --git a/include/configs/m28evk.h b/include/configs/m28evk.h index bdbb820..2e8a236 100644 --- a/include/configs/m28evk.h +++ b/include/configs/m28evk.h @@ -189,7 +189,6 @@ #ifdef CONFIG_CMD_NET #define CONFIG_ETHPRIME "FEC0" #define CONFIG_FEC_MXC -#define CONFIG_FEC_MXC_MULTI #define CONFIG_MII #define CONFIG_FEC_XCV_TYPE RMII #endif diff --git a/include/configs/mx28evk.h b/include/configs/mx28evk.h index 7cdbec6..8178aa8 100644 --- a/include/configs/mx28evk.h +++ b/include/configs/mx28evk.h @@ -163,7 +163,6 @@ #define CONFIG_NET_MULTI #define CONFIG_ETHPRIME "FEC0" #define CONFIG_FEC_MXC -#define CONFIG_FEC_MXC_MULTI #define CONFIG_MII #define CONFIG_FEC_XCV_TYPE RMII #define CONFIG_MX28_FEC_MAC_IN_OCOTP diff --git a/include/configs/sc_sps_1.h b/include/configs/sc_sps_1.h index f5dc393..4177e3d 100644 --- a/include/configs/sc_sps_1.h +++ b/include/configs/sc_sps_1.h @@ -158,7 +158,6 @@ #ifdef CONFIG_CMD_NET #define CONFIG_ETHPRIME "FEC0" #define CONFIG_FEC_MXC -#define CONFIG_FEC_MXC_MULTI #define CONFIG_MII #define CONFIG_DISCOVER_PHY #define CONFIG_FEC_XCV_TYPE RMII
Only the hardware ethernet registers are needed for this function, so don't pass the more general structure. I'm trying to separate MII and fec.
This also fixes MX28 fec_mii_setspeed use on secondary ethernet port
This was found by inspection of the code and should be checked on real hardware.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com --- drivers/net/fec_mxc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 6596ceb..eb89e57 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -135,15 +135,15 @@ static int fec_mdio_read(struct ethernet_regs *eth, uint8_t phyAddr, return val; }
-static void fec_mii_setspeed(struct fec_priv *fec) +static void fec_mii_setspeed(struct ethernet_regs *eth) { /* * Set MII_SPEED = (1/(mii_speed * 2)) * System Clock * and do not drop the Preamble. */ writel((((imx_get_fecclk() / 1000000) + 2) / 5) << 1, - &fec->eth->mii_speed); - debug("%s: mii_speed %08x\n", __func__, readl(&fec->eth->mii_speed)); + ð->mii_speed); + debug("%s: mii_speed %08x\n", __func__, readl(ð->mii_speed)); }
static int fec_mdio_write(struct ethernet_regs *eth, uint8_t phyAddr, @@ -611,7 +611,7 @@ static int fec_init(struct eth_device *dev, bd_t* bd) fec_reg_setup(fec);
if (fec->xcv_type != SEVENWIRE) - fec_mii_setspeed(fec); + fec_mii_setspeed(fec->bus->priv);
/* * Set Opcode/Pause Duration Register @@ -966,7 +966,6 @@ static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) }
fec_reg_setup(fec); - fec_mii_setspeed(fec);
if (dev_id == -1) { sprintf(edev->name, "FEC"); @@ -995,6 +994,7 @@ static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) #else bus->priv = fec->eth; #endif + fec_mii_setspeed(bus->priv); ret = mdio_register(bus); if (ret) { printf("mdio_register failed\n");
Having only one call to fec_probe will ease the changing of its parameters.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com --- drivers/net/fec_mxc.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index eb89e57..f7384ad 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -1021,27 +1021,19 @@ err1: return ret; }
-#ifdef CONFIG_FEC_MXC_PHYADDR -int fecmxc_initialize(bd_t *bd) -{ - int lout = 1; - - debug("eth_init: fec_probe(bd)\n"); - lout = fec_probe(bd, -1, CONFIG_FEC_MXC_PHYADDR, IMX_FEC_BASE); - - return lout; -} -#endif - int fecmxc_initialize_multi(bd_t *bd, int dev_id, int phy_id, uint32_t addr) { - int lout = 1; - debug("eth_init: fec_probe(bd, %i, %i) @ %08x\n", dev_id, phy_id, addr); - lout = fec_probe(bd, dev_id, phy_id, addr); + return fec_probe(bd, dev_id, phy_id, addr); +}
- return lout; +#ifdef CONFIG_FEC_MXC_PHYADDR +int fecmxc_initialize(bd_t *bd) +{ + return fecmxc_initialize_multi(bd, -1, CONFIG_FEC_MXC_PHYADDR, + IMX_FEC_BASE); } +#endif
#ifndef CONFIG_PHYLIB int fecmxc_register_mii_postcall(struct eth_device *dev, int (*cb)(int))
It is useful to be able to try a range of possible phy addresses to connect.
Also, an ethernet device is not required to use phy_find_by_mask leading to better separation of mii vs ethernet, as suggested by Andy Fleming.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com
--- v2: no change --- drivers/net/phy/phy.c | 128 ++++++++++++++++++++++++++++++------------------- include/phy.h | 3 ++ 2 files changed, 81 insertions(+), 50 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index baef60f..c51b309 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -31,6 +31,7 @@ #include <miiphy.h> #include <phy.h> #include <errno.h> +#include <linux/err.h>
/* Generic PHY support and helper functions */
@@ -573,6 +574,61 @@ int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id) return 0; }
+static struct phy_device *create_phy_by_mask(struct mii_dev *bus, + unsigned phy_mask, int devad, phy_interface_t interface) +{ + u32 phy_id = 0xffffffff; + while (phy_mask) { + int addr = ffs(phy_mask) - 1; + int r = get_phy_id(bus, addr, devad, &phy_id); + if (r < 0) + return ERR_PTR(r); + /* If the PHY ID is mostly f's, we didn't find anything */ + if ((phy_id & 0x1fffffff) != 0x1fffffff) + return phy_device_create(bus, addr, phy_id, interface); + phy_mask &= ~(1 << addr); + } + return NULL; +} + +static struct phy_device *search_for_existing_phy(struct mii_dev *bus, + unsigned phy_mask, phy_interface_t interface) +{ + /* If we have one, return the existing device, with new interface */ + while (phy_mask) { + int addr = ffs(phy_mask) - 1; + if (bus->phymap[addr]) { + bus->phymap[addr]->interface = interface; + return bus->phymap[addr]; + } + phy_mask &= ~(1 << addr); + } + return NULL; +} + +static struct phy_device *get_phy_device_by_mask(struct mii_dev *bus, + unsigned phy_mask, phy_interface_t interface) +{ + int i; + struct phy_device *phydev; + + phydev = search_for_existing_phy(bus, phy_mask, interface); + if (phydev) + return phydev; + /* Try Standard (ie Clause 22) access */ + /* Otherwise we have to try Clause 45 */ + for (i = 0; i < 5; i++) { + phydev = create_phy_by_mask(bus, phy_mask, + i ? i : MDIO_DEVAD_NONE, interface); + if (IS_ERR(phydev)) + return NULL; + if (phydev) + return phydev; + } + printf("Phy not found\n"); + return phy_device_create(bus, ffs(phy_mask) - 1, 0xffffffff, interface); +} + /** * get_phy_device - reads the specified PHY device and returns its @phy_device struct * @bus: the target MII bus @@ -584,38 +640,7 @@ int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id) struct phy_device *get_phy_device(struct mii_dev *bus, int addr, phy_interface_t interface) { - u32 phy_id = 0x1fffffff; - int i; - int r; - - /* If we have one, return the existing device, with new interface */ - if (bus->phymap[addr]) { - bus->phymap[addr]->interface = interface; - - return bus->phymap[addr]; - } - - /* Try Standard (ie Clause 22) access */ - r = get_phy_id(bus, addr, MDIO_DEVAD_NONE, &phy_id); - if (r) - return NULL; - - /* If the PHY ID is mostly f's, we didn't find anything */ - if ((phy_id & 0x1fffffff) != 0x1fffffff) - return phy_device_create(bus, addr, phy_id, interface); - - /* Otherwise we have to try Clause 45 */ - for (i = 1; i < 5; i++) { - r = get_phy_id(bus, addr, i, &phy_id); - if (r) - return NULL; - - /* If the phy_id is mostly Fs, there is no device there */ - if ((phy_id & 0x1fffffff) != 0x1fffffff) - break; - } - - return phy_device_create(bus, addr, phy_id, interface); + return get_phy_device_by_mask(bus, 1 << addr, interface); }
int phy_reset(struct phy_device *phydev) @@ -688,38 +713,41 @@ int miiphy_reset(const char *devname, unsigned char addr) return phy_reset(phydev); }
-struct phy_device *phy_connect(struct mii_dev *bus, int addr, - struct eth_device *dev, - phy_interface_t interface) +struct phy_device *phy_find_by_mask(struct mii_dev *bus, unsigned phy_mask, + phy_interface_t interface) { - struct phy_device *phydev; - /* Reset the bus */ if (bus->reset) bus->reset(bus);
/* Wait 15ms to make sure the PHY has come out of hard reset */ udelay(15000); + return get_phy_device_by_mask(bus, phy_mask, interface); +}
- phydev = get_phy_device(bus, addr, interface); - - if (!phydev) { - printf("Could not get PHY for %s:%d\n", bus->name, addr); - - return NULL; - } - +void phy_connect_dev(struct phy_device *phydev, struct eth_device *dev) +{ /* Soft Reset the PHY */ phy_reset(phydev); - - if (phydev->dev) + if (phydev->dev) { printf("%s:%d is connected to %s. Reconnecting to %s\n", - bus->name, addr, phydev->dev->name, dev->name); - + phydev->bus->name, phydev->addr, + phydev->dev->name, dev->name); + } phydev->dev = dev; - debug("%s connected to %s\n", dev->name, phydev->drv->name); +} + +struct phy_device *phy_connect(struct mii_dev *bus, int addr, + struct eth_device *dev, phy_interface_t interface) +{ + struct phy_device *phydev;
+ phydev = phy_find_by_mask(bus, 1 << addr, interface); + if (phydev) + phy_connect_dev(phydev, dev); + else + printf("Could not get PHY for %s: addr %d\n", bus->name, addr); return phydev; }
diff --git a/include/phy.h b/include/phy.h index 3c30f11..58ca273 100644 --- a/include/phy.h +++ b/include/phy.h @@ -199,6 +199,9 @@ static inline int is_10g_interface(phy_interface_t interface)
int phy_init(void); int phy_reset(struct phy_device *phydev); +struct phy_device *phy_find_by_mask(struct mii_dev *bus, unsigned phy_mask, + phy_interface_t interface); +void phy_connect_dev(struct phy_device *phydev, struct eth_device *dev); struct phy_device *phy_connect(struct mii_dev *bus, int addr, struct eth_device *dev, phy_interface_t interface);
This allows us to create the phydev before calling fec_probe in later patch.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com --- drivers/net/fec_mxc.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index f7384ad..7e27210 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -915,6 +915,11 @@ static int fec_recv(struct eth_device *dev) return len; }
+static void fec_set_dev_name(char *dest, int dev_id) +{ + sprintf(dest, (dev_id == -1) ? "FEC" : "FEC%i", dev_id); +} + static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) { struct eth_device *edev; @@ -967,13 +972,8 @@ static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr)
fec_reg_setup(fec);
- if (dev_id == -1) { - sprintf(edev->name, "FEC"); - fec->dev_id = 0; - } else { - sprintf(edev->name, "FEC%i", dev_id); - fec->dev_id = dev_id; - } + fec_set_dev_name(edev->name, dev_id); + fec->dev_id = (dev_id == -1) ? 0 : dev_id; fec->phy_id = phy_id;
bus = mdio_alloc(); @@ -984,7 +984,7 @@ static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) } bus->read = fec_phy_read; bus->write = fec_phy_write; - sprintf(bus->name, edev->name); + fec_set_dev_name(bus->name, dev_id); #ifdef CONFIG_MX28 /* * The i.MX28 has two ethernet interfaces, but they are not equal.
This allows us to create the phydev before calling fec_probe in later patch.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com --- drivers/net/fec_mxc.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 7e27210..913c561 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -392,21 +392,6 @@ static int fec_set_hwaddr(struct eth_device *dev) return 0; }
-static void fec_eth_phy_config(struct eth_device *dev) -{ -#ifdef CONFIG_PHYLIB - struct fec_priv *fec = (struct fec_priv *)dev->priv; - struct phy_device *phydev; - - phydev = phy_connect(fec->bus, fec->phy_id, dev, - PHY_INTERFACE_MODE_RGMII); - if (phydev) { - fec->phydev = phydev; - phy_config(phydev); - } -#endif -} - /* * Do initial configuration of the FEC registers */ @@ -511,9 +496,7 @@ static int fec_open(struct eth_device *edev) #endif
#ifdef CONFIG_PHYLIB - if (!fec->phydev) - fec_eth_phy_config(edev); - if (fec->phydev) { + { /* Start up the PHY */ int ret = phy_startup(fec->phydev);
@@ -523,8 +506,6 @@ static int fec_open(struct eth_device *edev) return ret; } speed = fec->phydev->speed; - } else { - speed = _100BASET; } #else miiphy_wait_aneg(edev); @@ -922,6 +903,7 @@ static void fec_set_dev_name(char *dest, int dev_id)
static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) { + struct phy_device *phydev; struct eth_device *edev; struct fec_priv *fec; struct mii_dev *bus; @@ -1010,7 +992,16 @@ static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) memcpy(edev->enetaddr, ethaddr, 6); } /* Configure phy */ - fec_eth_phy_config(edev); +#ifdef CONFIG_PHYLIB + phydev = phy_connect(fec->bus, phy_id, edev, PHY_INTERFACE_MODE_RGMII); + if (!phydev) { + free(bus); + ret = -ENOMEM; + goto err3; + } + fec->phydev = phydev; + phy_config(phydev); +#endif return ret;
err3:
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com --- drivers/net/fec_mxc.c | 117 +++++++++++++++++++++++++++++++------------------ drivers/net/fec_mxc.h | 2 +- include/netdev.h | 7 +++ 3 files changed, 83 insertions(+), 43 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 913c561..4dbcdca 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -901,12 +901,16 @@ static void fec_set_dev_name(char *dest, int dev_id) sprintf(dest, (dev_id == -1) ? "FEC" : "FEC%i", dev_id); }
-static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) +#ifdef CONFIG_PHYLIB +int fec_probe(bd_t *bd, int dev_id, uint32_t base_addr, + struct mii_dev *bus, struct phy_device *phydev) +#else +static int fec_probe(bd_t *bd, int dev_id, uint32_t base_addr, + struct mii_dev *bus, int phy_id) +#endif { - struct phy_device *phydev; struct eth_device *edev; struct fec_priv *fec; - struct mii_dev *bus; unsigned char ethaddr[6]; uint32_t start; int ret = 0; @@ -953,69 +957,98 @@ static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr) }
fec_reg_setup(fec); - fec_set_dev_name(edev->name, dev_id); fec->dev_id = (dev_id == -1) ? 0 : dev_id; + fec->bus = bus; + fec_mii_setspeed(bus->priv); +#ifdef CONFIG_PHYLIB + fec->phydev = phydev; + phy_connect_dev(phydev, edev); + /* Configure phy */ + phy_config(phydev); +#else fec->phy_id = phy_id; +#endif + eth_register(edev); + + if (fec_get_hwaddr(edev, dev_id, ethaddr) == 0) { + debug("got MAC%d address from fuse: %pM\n", dev_id, ethaddr); + memcpy(edev->enetaddr, ethaddr, 6); + } + return ret; +err3: + free(fec); +err2: + free(edev); +err1: + return ret; +} + +struct mii_dev *fec_get_miibus(uint32_t base_addr, int dev_id) +{ + struct ethernet_regs *eth = (struct ethernet_regs *)base_addr; + struct mii_dev *bus; + int ret;
bus = mdio_alloc(); if (!bus) { printf("mdio_alloc failed\n"); - ret = -ENOMEM; - goto err3; + return NULL; } bus->read = fec_phy_read; bus->write = fec_phy_write; + bus->priv = eth; fec_set_dev_name(bus->name, dev_id); + + ret = mdio_register(bus); + if (ret) { + printf("mdio_register failed\n"); + free(bus); + return NULL; + } + fec_mii_setspeed(eth); + return bus; +} + +int fecmxc_initialize_multi(bd_t *bd, int dev_id, int phy_id, uint32_t addr) +{ + uint32_t base_mii; + struct mii_dev *bus = NULL; +#ifdef CONFIG_PHYLIB + struct phy_device *phydev = NULL; +#endif + int ret; + #ifdef CONFIG_MX28 /* * The i.MX28 has two ethernet interfaces, but they are not equal. * Only the first one can access the MDIO bus. */ - bus->priv = (struct ethernet_regs *)MXS_ENET0_BASE; + base_mii = MXS_ENET0_BASE; #else - bus->priv = fec->eth; + base_mii = addr; #endif - fec_mii_setspeed(bus->priv); - ret = mdio_register(bus); - if (ret) { - printf("mdio_register failed\n"); - free(bus); - ret = -ENOMEM; - goto err3; - } - fec->bus = bus; - eth_register(edev); - - if (fec_get_hwaddr(edev, dev_id, ethaddr) == 0) { - debug("got MAC%d address from fuse: %pM\n", dev_id, ethaddr); - memcpy(edev->enetaddr, ethaddr, 6); - } - /* Configure phy */ + debug("eth_init: fec_probe(bd, %i, %i) @ %08x\n", dev_id, phy_id, addr); + bus = fec_get_miibus(base_mii, dev_id); + if (!bus) + return -ENOMEM; #ifdef CONFIG_PHYLIB - phydev = phy_connect(fec->bus, phy_id, edev, PHY_INTERFACE_MODE_RGMII); + phydev = phy_find_by_mask(bus, 1 << phy_id, PHY_INTERFACE_MODE_RGMII); if (!phydev) { free(bus); - ret = -ENOMEM; - goto err3; + return -ENOMEM; } - fec->phydev = phydev; - phy_config(phydev); + ret = fec_probe(bd, dev_id, addr, bus, phydev); +#else + ret = fec_probe(bd, dev_id, addr, bus, phy_id); #endif + if (ret) { +#ifdef CONFIG_PHYLIB + free(phydev); +#endif + free(bus); + } return ret; - -err3: - free(fec); -err2: - free(edev); -err1: - return ret; -} - -int fecmxc_initialize_multi(bd_t *bd, int dev_id, int phy_id, uint32_t addr) -{ - debug("eth_init: fec_probe(bd, %i, %i) @ %08x\n", dev_id, phy_id, addr); - return fec_probe(bd, dev_id, phy_id, addr); }
#ifdef CONFIG_FEC_MXC_PHYADDR diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h index 203285a..b8f0da3 100644 --- a/drivers/net/fec_mxc.h +++ b/drivers/net/fec_mxc.h @@ -271,11 +271,11 @@ struct fec_priv { bd_t *bd; uint8_t *tdb_ptr; int dev_id; - int phy_id; struct mii_dev *bus; #ifdef CONFIG_PHYLIB struct phy_device *phydev; #else + int phy_id; int (*mii_postcall)(int); #endif }; diff --git a/include/netdev.h b/include/netdev.h index b8d303d..62aef79 100644 --- a/include/netdev.h +++ b/include/netdev.h @@ -204,9 +204,16 @@ struct mv88e61xx_config { int mv88e61xx_switch_initialize(struct mv88e61xx_config *swconfig); #endif /* CONFIG_MV88E61XX_SWITCH */
+struct mii_dev *fec_get_miibus(uint32_t base_addr, int dev_id); +#ifdef CONFIG_PHYLIB +struct phy_device; +int fec_probe(bd_t *bd, int dev_id, uint32_t base_addr, + struct mii_dev *bus, struct phy_device *phydev); +#else /* * Allow FEC to fine-tune MII configuration on boards which require this. */ int fecmxc_register_mii_postcall(struct eth_device *dev, int (*cb)(int)); +#endif
#endif /* _NETDEV_H_ */
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com --- board/freescale/mx6qsabrelite/mx6qsabrelite.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c b/board/freescale/mx6qsabrelite/mx6qsabrelite.c index af6f917..f010d0c 100644 --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c @@ -33,6 +33,7 @@ #include <asm/imx-common/boot_mode.h> #include <mmc.h> #include <fsl_esdhc.h> +#include <malloc.h> #include <micrel.h> #include <miiphy.h> #include <netdev.h> @@ -338,14 +339,31 @@ int board_phy_config(struct phy_device *phydev)
int board_eth_init(bd_t *bis) { + uint32_t base = IMX_FEC_BASE; + struct mii_dev *bus = NULL; + struct phy_device *phydev = NULL; int ret;
setup_iomux_enet();
- ret = cpu_eth_init(bis); - if (ret) +#ifdef CONFIG_FEC_MXC + bus = fec_get_miibus(base, -1); + if (!bus) + return 0; + /* scan phy 4,5,6,7 */ + phydev = phy_find_by_mask(bus, (0xf << 4), PHY_INTERFACE_MODE_RGMII); + if (!phydev) { + free(bus); + return 0; + } + printf("using phy at %d\n", phydev->addr); + ret = fec_probe(bis, -1, base, bus, phydev); + if (ret) { printf("FEC MXC: %s:failed\n", __func__); - + free(phydev); + free(bus); + } +#endif return 0; }
On 23/10/2012 04:40, Troy Kisky wrote:
This series tries to separate the mii regsisters from the ethernet registers as suggested by Andy Fleming. Then, mx6qsabrelite is changed to find the phy address from the possibles 4-7.
The V3 series is very different from V2.
Troy Kisky (9): doc/README.fec_mxc: add documentation net: fec_mxc: delete CONFIG_FEC_MXC_MULTI net: fec_mxc: change fec_mii_setspeed parameter net: fec_mxc: have fecmxc_initialize call fecmxc_initialize_multi phy: add phy_find_by_mask/phy_connect_dev net: fec_mxc: use fec_set_dev_name to set name net: fec_mxc: only call phy_connect in fec_probe net: fec_mxc: get phydev before fec_probe mx6qsabrelite: search mii phy address 4-7
Hi Joe,
board/freescale/mx6qsabrelite/mx6qsabrelite.c | 24 +++- doc/README.fec_mxc | 27 ++++ drivers/net/fec_mxc.c | 164 ++++++++++++++----------- drivers/net/fec_mxc.h | 2 +- drivers/net/phy/phy.c | 128 +++++++++++-------- include/configs/m28evk.h | 1 - include/configs/mx28evk.h | 1 - include/configs/sc_sps_1.h | 1 - include/netdev.h | 7 ++ include/phy.h | 3 + 10 files changed, 227 insertions(+), 131 deletions(-) create mode 100644 doc/README.fec_mxc
Patches are assigned to me in patchworks. Do you have any issue with this patchset ? If it is ok for you, I will apply it to u-boot-imx.
Best regards, Stefano
On 11/10/2012 12:28 AM, Stefano Babic wrote:
On 23/10/2012 04:40, Troy Kisky wrote:
This series tries to separate the mii regsisters from the ethernet registers as suggested by Andy Fleming. Then, mx6qsabrelite is changed to find the phy address from the possibles 4-7.
The V3 series is very different from V2.
Troy Kisky (9): doc/README.fec_mxc: add documentation net: fec_mxc: delete CONFIG_FEC_MXC_MULTI net: fec_mxc: change fec_mii_setspeed parameter net: fec_mxc: have fecmxc_initialize call fecmxc_initialize_multi phy: add phy_find_by_mask/phy_connect_dev net: fec_mxc: use fec_set_dev_name to set name net: fec_mxc: only call phy_connect in fec_probe net: fec_mxc: get phydev before fec_probe mx6qsabrelite: search mii phy address 4-7
Hi Joe,
board/freescale/mx6qsabrelite/mx6qsabrelite.c | 24 +++- doc/README.fec_mxc | 27 ++++ drivers/net/fec_mxc.c | 164 ++++++++++++++----------- drivers/net/fec_mxc.h | 2 +- drivers/net/phy/phy.c | 128 +++++++++++-------- include/configs/m28evk.h | 1 - include/configs/mx28evk.h | 1 - include/configs/sc_sps_1.h | 1 - include/netdev.h | 7 ++ include/phy.h | 3 + 10 files changed, 227 insertions(+), 131 deletions(-) create mode 100644 doc/README.fec_mxc
Patches are assigned to me in patchworks. Do you have any issue with this patchset ? If it is ok for you, I will apply it to u-boot-imx.
Best regards, Stefano
Who should I pester to see some progress on this ?
Thanks Troy
On 23/01/2013 00:48, Troy Kisky wrote:
On 11/10/2012 12:28 AM, Stefano Babic wrote:
On 23/10/2012 04:40, Troy Kisky wrote:
This series tries to separate the mii regsisters from the ethernet registers as suggested by Andy Fleming. Then, mx6qsabrelite is changed to find the phy address from the possibles 4-7.
The V3 series is very different from V2.
Troy Kisky (9): doc/README.fec_mxc: add documentation net: fec_mxc: delete CONFIG_FEC_MXC_MULTI net: fec_mxc: change fec_mii_setspeed parameter net: fec_mxc: have fecmxc_initialize call fecmxc_initialize_multi phy: add phy_find_by_mask/phy_connect_dev net: fec_mxc: use fec_set_dev_name to set name net: fec_mxc: only call phy_connect in fec_probe net: fec_mxc: get phydev before fec_probe mx6qsabrelite: search mii phy address 4-7
Hi Joe,
board/freescale/mx6qsabrelite/mx6qsabrelite.c | 24 +++- doc/README.fec_mxc | 27 ++++ drivers/net/fec_mxc.c | 164 ++++++++++++++----------- drivers/net/fec_mxc.h | 2 +- drivers/net/phy/phy.c | 128 +++++++++++-------- include/configs/m28evk.h | 1 - include/configs/mx28evk.h | 1 - include/configs/sc_sps_1.h | 1 - include/netdev.h | 7 ++ include/phy.h | 3 + 10 files changed, 227 insertions(+), 131 deletions(-) create mode 100644 doc/README.fec_mxc
Patches are assigned to me in patchworks. Do you have any issue with this patchset ? If it is ok for you, I will apply it to u-boot-imx.
Best regards, Stefano
Who should I pester to see some progress on this ?
Hi Troy,
you're right, this patchset stalls - but on the other side, I have not found any comment that should avoid that it can be merged.
If nobody complains, I propose I go on and I try to merge it into u-boot-imx. Here my first attempt:
as far as I can see, the patchset breaks mx6qsabreauto and mx35 boards. Generally speking, it breaks boards like flea3 that define CONFIG_PHYLIB in their config file.
undefined reference to `phy_connect_dev' drivers/net/libnet.o: In function `fecmxc_initialize_multi':
I think the patchset assumes that boards have its own board_eth_init(), making the required phy initialization, but most boards call only cpu_eth_init().
I have also a general question : it seems to me that the interface for PHY is set always to RMII - what about boards using MII ?
Best regards, Stefano Babic
On 1/23/2013 1:48 AM, Stefano Babic wrote:
On 23/01/2013 00:48, Troy Kisky wrote:
On 11/10/2012 12:28 AM, Stefano Babic wrote:
On 23/10/2012 04:40, Troy Kisky wrote:
This series tries to separate the mii regsisters from the ethernet registers as suggested by Andy Fleming. Then, mx6qsabrelite is changed to find the phy address from the possibles 4-7.
The V3 series is very different from V2.
Troy Kisky (9): doc/README.fec_mxc: add documentation net: fec_mxc: delete CONFIG_FEC_MXC_MULTI net: fec_mxc: change fec_mii_setspeed parameter net: fec_mxc: have fecmxc_initialize call fecmxc_initialize_multi phy: add phy_find_by_mask/phy_connect_dev net: fec_mxc: use fec_set_dev_name to set name net: fec_mxc: only call phy_connect in fec_probe net: fec_mxc: get phydev before fec_probe mx6qsabrelite: search mii phy address 4-7
Hi Joe,
board/freescale/mx6qsabrelite/mx6qsabrelite.c | 24 +++- doc/README.fec_mxc | 27 ++++ drivers/net/fec_mxc.c | 164 ++++++++++++++----------- drivers/net/fec_mxc.h | 2 +- drivers/net/phy/phy.c | 128 +++++++++++-------- include/configs/m28evk.h | 1 - include/configs/mx28evk.h | 1 - include/configs/sc_sps_1.h | 1 - include/netdev.h | 7 ++ include/phy.h | 3 + 10 files changed, 227 insertions(+), 131 deletions(-) create mode 100644 doc/README.fec_mxc
Patches are assigned to me in patchworks. Do you have any issue with this patchset ? If it is ok for you, I will apply it to u-boot-imx.
Best regards, Stefano
Who should I pester to see some progress on this ?
Hi Troy,
you're right, this patchset stalls - but on the other side, I have not found any comment that should avoid that it can be merged.
If nobody complains, I propose I go on and I try to merge it into u-boot-imx. Here my first attempt:
as far as I can see, the patchset breaks mx6qsabreauto and mx35 boards. Generally speking, it breaks boards like flea3 that define CONFIG_PHYLIB in their config file.
undefined reference to `phy_connect_dev' drivers/net/libnet.o: In function `fecmxc_initialize_multi':
I think the patchset assumes that boards have its own board_eth_init(), making the required phy initialization, but most boards call only cpu_eth_init().
Strange, my ./MAKEALL -a arm finished fine
--------------------- SUMMARY ---------------------------- Boards compiled: 297 Boards with warnings but no errors: 3 ( VCMA9 smdk2410 cam_enc_4xx ) ----------------------------------------------------------
Did you apply "[PATCH V3 5/9] phy: add phy_find_by_mask/phy_connect_dev"
I have also a general question : it seems to me that the interface for PHY is set always to RMII - what about boards using MII ?
I could have swore that I saw a patch to fix this? But it does look wrong currently. But perhaps I'm thinking of 9d2d924a net: fec_mxc: Fix setting of RCR for xMII which doesn't change the gasket setting for mx53
Best regards, Stefano Babic
On 23/01/2013 20:06, Troy Kisky wrote:
Hi Troy,
you're right, this patchset stalls - but on the other side, I have not found any comment that should avoid that it can be merged.
If nobody complains, I propose I go on and I try to merge it into u-boot-imx. Here my first attempt:
as far as I can see, the patchset breaks mx6qsabreauto and mx35 boards. Generally speking, it breaks boards like flea3 that define CONFIG_PHYLIB in their config file.
undefined reference to `phy_connect_dev' drivers/net/libnet.o: In function `fecmxc_initialize_multi':
I think the patchset assumes that boards have its own board_eth_init(), making the required phy initialization, but most boards call only cpu_eth_init().
Strange, my ./MAKEALL -a arm finished fine
Surely something is missing on my tree
--------------------- SUMMARY ---------------------------- Boards compiled: 297 Boards with warnings but no errors: 3 ( VCMA9 smdk2410 cam_enc_4xx )
Did you apply "[PATCH V3 5/9] phy: add phy_find_by_mask/phy_connect_dev"
That is ! It is assigned to Joe, and I have not found soon in patchwork. Now I can build. Joe, this patch is really network-related and is in your competence area, but if you do not complain I will apply together with the rest to avoid to break some i.MX boards.
Best regards, Stefano Babic
On 1/23/2013 1:48 AM, Stefano Babic wrote:
On 23/01/2013 00:48, Troy Kisky wrote:
On 11/10/2012 12:28 AM, Stefano Babic wrote:
I have also a general question : it seems to me that the interface for PHY is set always to RMII - what about boards using MII ?
Best regards, Stefano Babic
This was the patch I was thinking of....
On 4/19/2012 1:55 AM, Timo Ketola wrote:
Gasket needs a different configuration for 10BaseT than for higher speeds.
Signed-off-by: Timo Ketolatimo@exertus.fi
Changes in v4:
- Rewrapped commit message
Changes in v2:
Dropped patches 2 and 3 so this one changed from 5 to 3
Rebased to u-boot-imx next
Removed the remove of 'miiphy_duplex' call
Changed 'speed == _100BASET' to 'speed != _10BASET' to not to break _1000BASET
Changed configuration option to put gasket into RMII mode from !CONFIG_MII to CONFIG_RMII. I'm not too sure how this should be done though. !CONFIG_MII is normally used for this but its original purpose was to enable MII *management* interface, I think...
drivers/net/fec_mxc.c | 43
++++++++++++++++++++++++------------------- 1 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 824a199..48a69d4 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -440,6 +440,22 @@ static int fec_open(struct eth_device *edev) */ writel(readl(&fec->eth->ecntrl) | FEC_ECNTRL_ETHER_EN, &fec->eth->ecntrl); +#ifdef CONFIG_PHYLIB
- if (!fec->phydev)
fec_eth_phy_config(edev);- if (fec->phydev) {
/* Start up the PHY */phy_startup(fec->phydev);speed = fec->phydev->speed;- } else {
speed = _100BASET;- }
+#else
- miiphy_wait_aneg(edev);
- speed = miiphy_speed(edev->name, fec->phy_id);
- miiphy_duplex(edev->name, fec->phy_id);
+#endif
- #if defined(CONFIG_MX25) || defined(CONFIG_MX53) udelay(100); /*
@@ -453,9 +469,14 @@ static int fec_open(struct eth_device *edev) while (readw(&fec->eth->miigsk_enr)& MIIGSK_ENR_READY) udelay(2);
-#if !defined(CONFIG_MII)
- /* configure gasket for RMII, 50 MHz, no loopback, and no echo */
- writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr);
+#if defined(CONFIG_RMII)
- if (speed != _10BASET)
/* configure gasket for RMII, 50MHz, no loopback, and no echo */- writew(MIIGSK_CFGR_IF_MODE_RMII,&fec->eth->miigsk_cfgr);
- else
/* configure gasket for RMII, 5MHz, no loopback, and no echo */writew(MIIGSK_CFGR_IF_MODE_RMII | MIIGSK_CFGR_FRCONT,&fec->eth->miigsk_cfgr);writew(MIIGSK_CFGR_IF_MODE_MII,&fec->eth->miigsk_cfgr); @@ -474,22 +495,6 @@ static int fec_open(struct eth_device *edev) } #endif
Can you fix 10BASET for non-reduced pin count boards as well?
Thanks Troy
On 10/22/2012 7:40 PM, Troy Kisky wrote:
This series tries to separate the mii regsisters from the ethernet registers as suggested by Andy Fleming. Then, mx6qsabrelite is changed to find the phy address from the possibles 4-7.
The V3 series is very different from V2.
Troy Kisky (9): doc/README.fec_mxc: add documentation net: fec_mxc: delete CONFIG_FEC_MXC_MULTI net: fec_mxc: change fec_mii_setspeed parameter net: fec_mxc: have fecmxc_initialize call fecmxc_initialize_multi phy: add phy_find_by_mask/phy_connect_dev net: fec_mxc: use fec_set_dev_name to set name net: fec_mxc: only call phy_connect in fec_probe net: fec_mxc: get phydev before fec_probe mx6qsabrelite: search mii phy address 4-7
board/freescale/mx6qsabrelite/mx6qsabrelite.c | 24 +++- doc/README.fec_mxc | 27 ++++ drivers/net/fec_mxc.c | 164 ++++++++++++++----------- drivers/net/fec_mxc.h | 2 +- drivers/net/phy/phy.c | 128 +++++++++++-------- include/configs/m28evk.h | 1 - include/configs/mx28evk.h | 1 - include/configs/sc_sps_1.h | 1 - include/netdev.h | 7 ++ include/phy.h | 3 + 10 files changed, 227 insertions(+), 131 deletions(-) create mode 100644 doc/README.fec_mxc
Ping on this series....
Is there anything I need to fix?
Thanks Troy
On 23/10/2012 04:40, Troy Kisky wrote:
This series tries to separate the mii regsisters from the ethernet registers as suggested by Andy Fleming. Then, mx6qsabrelite is changed to find the phy address from the possibles 4-7.
The V3 series is very different from V2.
Applied to u-boot-imx, thanks.
Best regards, Stefano Babic
participants (5)
-
Andy Fleming -
Eric Nelson -
Joe Hershberger -
Stefano Babic -
Troy Kisky