net: fec_mxc: add 1000 Mbps selection

Signed-off-by Troy Kisky <[email protected]>

---
?drivers/net/fec_mxc.c | ? 35 ++++++++++++++++++++++++++++++----- ?drivers/net/fec_mxc.h | ? ?1 +
?2 files changed, 31 insertions(+), 5 deletions(-)

drivers/net/fec_mxc.c +14 -0

index 3fffe79..4d7a38d 100644
@@ -371,6 +371,20 @@ 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(miiphy_get_dev_by_name(dev->name),
+ ? ? ? ? ? ? ? ? ? ? ? fec->phy_id, dev, PHY_INTERFACE_MODE_RGMII);
+ ? ? ? fec->phydev = phydev;
+ ? ? ? if (phydev)
+ ? ? ? ? ? ? ? phy_config(phydev);
+#endif
+}
+

?/**
?* Start the FEC engine
?* @param[in] dev Our device to handle

drivers/net/fec_mxc.c +0 -0

@@ -427,9 +441,19 @@ static int fec_open(struct eth_device *edev)
 ? ? ? ?}

?#endif

- ? ? ? miiphy_wait_aneg(edev);
- ? ? ? speed = miiphy_speed(edev->name, fec->phy_id);
- ? ? ? miiphy_duplex(edev->name, fec->phy_id);
+ ? ? ? if (fec->phydev) {
+ ? ? ? ? ? ? ? /* Start up the PHY */

Quoted 33 lines · Troy Kisky

Signed-off-by Troy Kisky<[email protected]>

---
  drivers/net/fec_mxc.c |   35 ++++++++++++++++++++++++++++++-----
  drivers/net/fec_mxc.h |    1 +
  2 files changed, 31 insertions(+), 5 deletions(-)

drivers/net/fec_mxc.c +14 -0

index 3fffe79..4d7a38d 100644
@@ -371,6 +371,20 @@ 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(miiphy_get_dev_by_name(dev->name),
+                       fec->phy_id, dev, PHY_INTERFACE_MODE_RGMII);
+       fec->phydev = phydev;
+       if (phydev)
+               phy_config(phydev);
+#endif
+}
+
  /**
  * Start the FEC engine
  * @param[in] dev Our device to handle

@@ -427,9 +441,19 @@ static int fec_open(struct eth_device *edev)
        }

drivers/net/fec_mxc.c static int fec_open(struct eth_device *edev) · line 448

Quoted 10 lines

drivers/net/fec_mxc.c +5 -3

  #endif
 
-       miiphy_wait_aneg(edev);
-       speed = miiphy_speed(edev->name, fec->phy_id);
-       miiphy_duplex(edev->name, fec->phy_id);
+       if (fec->phydev) {
+               /* Start up the PHY */
+#ifdef CONFIG_PHYLIB
+               phy_startup(fec->phydev);
+#endif

The old miiphy code is not truly compatible with the new PHY Lib code. Please implement full support, rather than relying on the compatibility shim. It should be straightforward, just convert all of the miiphy-style calls into the newer mdio/phy calls.

Andy

Quoted 54 lines · Andy Fleming

Quoted 41 lines · On Thu, Jan 26, 2012 at 4:21 PM, Troy Kisky <[email protected]>…

Signed-off-by Troy Kisky<[email protected]>

---
?drivers/net/fec_mxc.c | ? 35 ++++++++++++++++++++++++++++++----- ?drivers/net/fec_mxc.h | ? ?1 +
?2 files changed, 31 insertions(+), 5 deletions(-)

drivers/net/fec_mxc.c +14 -0

index 3fffe79..4d7a38d 100644
@@ -371,6 +371,20 @@ 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(miiphy_get_dev_by_name(dev->name),
+ ? ? ? ? ? ? ? ? ? ? ? fec->phy_id, dev, PHY_INTERFACE_MODE_RGMII);
+ ? ? ? fec->phydev = phydev;
+ ? ? ? if (phydev)
+ ? ? ? ? ? ? ? phy_config(phydev);
+#endif
+}
+

?/**
?* Start the FEC engine
?* @param[in] dev Our device to handle

drivers/net/fec_mxc.c +0 -0

@@ -427,9 +441,19 @@ static int fec_open(struct eth_device *edev)
 ? ? ? ?}

?#endif

- ? ? ? miiphy_wait_aneg(edev);
- ? ? ? speed = miiphy_speed(edev->name, fec->phy_id);
- ? ? ? miiphy_duplex(edev->name, fec->phy_id);
+ ? ? ? if (fec->phydev) {
+ ? ? ? ? ? ? ? /* Start up the PHY */

drivers/net/fec_mxc.c static int fec_set_hwaddr(struct eth_device *dev) · line 385

Quoted 3 lines

drivers/net/fec_mxc.c +3 -0

+#ifdef CONFIG_PHYLIB
+ ? ? ? ? ? ? ? phy_startup(fec->phydev);
+#endif

The old miiphy code is not truly compatible with the new PHY Lib code. Please implement full support, rather than relying on the compatibility shim. It should be straightforward, just convert all of the miiphy-style calls into the newer mdio/phy calls.

Quoted 2 lines

Andy

How can I do this without running the risk of breaking existing boards? Surely, there should be an intermediate stage where both are supported until all boards are converted. Then, my "shim" can be safely removed.

Or am I entirely missing you point???
Sorry if I am. Please elaborate on how not to break existing boards which I cannot test.

Quoted 54 lines · Troy Kisky

Quoted 51 lines · Andy Fleming

Quoted 33 lines · Troy Kisky

Signed-off-by Troy Kisky<[email protected]>

---
  drivers/net/fec_mxc.c |   35 ++++++++++++++++++++++++++++++-----
  drivers/net/fec_mxc.h |    1 +
  2 files changed, 31 insertions(+), 5 deletions(-)

drivers/net/fec_mxc.c +14 -0

index 3fffe79..4d7a38d 100644
@@ -371,6 +371,20 @@ 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(miiphy_get_dev_by_name(dev->name),
+                       fec->phy_id, dev, PHY_INTERFACE_MODE_RGMII);
+       fec->phydev = phydev;
+       if (phydev)
+               phy_config(phydev);
+#endif
+}
+
  /**
  * Start the FEC engine
  * @param[in] dev Our device to handle

@@ -427,9 +441,19 @@ static int fec_open(struct eth_device *edev)
        }

drivers/net/fec_mxc.c static int fec_open(struct eth_device *edev) · line 448

Quoted 10 lines

drivers/net/fec_mxc.c +5 -3

  #endif
 
-       miiphy_wait_aneg(edev);
-       speed = miiphy_speed(edev->name, fec->phy_id);
-       miiphy_duplex(edev->name, fec->phy_id);
+       if (fec->phydev) {
+               /* Start up the PHY */
+#ifdef CONFIG_PHYLIB
+               phy_startup(fec->phydev);
+#endif

The old miiphy code is not truly compatible with the new PHY Lib code. Please implement full support, rather than relying on the compatibility shim. It should be straightforward, just convert all of the miiphy-style calls into the newer mdio/phy calls.

Quoted 2 lines

Andy

Quoted 7 lines

How can I do this without running the risk of breaking existing boards? Surely, there should be an intermediate stage where both are supported until all boards are converted. Then, my "shim" can be safely removed.

Or am I entirely missing you point???
Sorry if I am. Please elaborate on how not to break existing boards which I cannot test.

Well, it wouldn't take *too* much work to fix it for all boards (Looks like none of the boards do board-specific PHY things. All of the board-specific PHY code is contained, evilly, in the ethernet driver). However, I'm not going to suggest that you have to port the driver fully now. But you need to make the two separate. The proper PHY Lib functions may, at some point, assume that there's proper bus infrastructure behind the bus transactions, and that might cause the redirection through the miiphy shim to do weird things. So allow miiphy to exist, but add the proper mdio initialization code in parallel. The config options will pick which type is used.

The problem is that the shim was meant to allow all existing code to continue working while not duplicating too much code. It wasn't meant to allow new code to use the old mechanisms (miiphy_read/miiphy_write, etc). The PHY Lib functions ignore how the miiphy code works, and the miiphy code is unaware of how the PHY Lib code works, so there's potential for things to break horribly. In practice, it's probably fine, but I strongly prefer avoiding hybrid solutions (I think this is the third driver that has tried this, which prompted a patch to mark the miiphy code as deprecated).

Andy

+int ksz9021_send_phy_cmds(struct phy_device *phydev, unsigned short* p)

drivers/net/phy/micrel_ksz9021.c line 40

Quoted 2 lines · On Thursday 26 January 2012 17:21:44 Troy Kisky wrote:

drivers/net/phy/micrel_ksz9021.c new file +2 -0

+/* This is used to set board specific things like clock skew */
+unsigned short ksz9021_por_cmds[] = {

static const

Quoted 2 lines

+int ksz9021_send_phy_cmds(struct phy_device *phydev, unsigned short* p)

static

drivers/net/phy/micrel_ksz9021.c line 49

Quoted 2 lines

drivers/net/phy/micrel_ksz9021.c new file +1 -0

+	for (;;) {

personally, i'd prefer:

	while (1) {

-mike

Add the gigabit phy KSZ9021.

Signed-off-by Troy Kisky <[email protected]>

---

?drivers/net/phy/Makefile ? ? ? ? | ? ?1 +
?drivers/net/phy/micrel_ksz9021.c | ?124 ++++++++++++++++++++++++++++++++++++++
?drivers/net/phy/phy.c ? ? ? ? ? ?| ? ?3 +

?3 files changed, 128 insertions(+), 0 deletions(-) ?create mode 100644 drivers/net/phy/micrel_ksz9021.c

drivers/net/phy/micrel_ksz9021.c +17 -0

+
+/* This is used to set board specific things like clock skew */
+unsigned short ksz9021_por_cmds[] = {
+#ifdef CONFIG_PHY_MICREL_KSZ9021_INIT_CMDS
+ ? ? ? CONFIG_PHY_MICREL_KSZ9021_INIT_CMDS
+#endif
+ ? ? ? 0, 0
+};
+
+int ksz9021_send_phy_cmds(struct phy_device *phydev, unsigned short* p)
+{
+ ? ? ? for (;;) {
+ ? ? ? ? ? ? ? unsigned reg = *p++;
+ ? ? ? ? ? ? ? unsigned val = *p++;
+ ? ? ? ? ? ? ? if (reg == 0 && val == 0)
+ ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? if (reg < 32) {

?#ifdef CONFIG_PHY_NATSEMI
? ? ? ?phy_natsemi_init();
?#endif

NAK, for reasons listed below.

Quoted 3 lines · Troy Kisky

Add the gigabit phy KSZ9021.

Signed-off-by Troy Kisky<[email protected]>

These commit messages are a bit over-brief. Please explain a bit more if there's anything non-trivial in the patch.

Quoted 7 lines

---
  drivers/net/phy/Makefile         |    1 +
  drivers/net/phy/micrel_ksz9021.c |  124 ++++++++++++++++++++++++++++++++++++++
  drivers/net/phy/phy.c            |    3 +
  3 files changed, 128 insertions(+), 0 deletions(-)

create mode 100644 drivers/net/phy/micrel_ksz9021.c

[...]

Quoted 25 lines

drivers/net/phy/micrel_ksz9021.c +24 -0

+
+/* This is used to set board specific things like clock skew */
+unsigned short ksz9021_por_cmds[] = {
+#ifdef CONFIG_PHY_MICREL_KSZ9021_INIT_CMDS
+       CONFIG_PHY_MICREL_KSZ9021_INIT_CMDS
+#endif
+       0, 0
+};
+
+int ksz9021_send_phy_cmds(struct phy_device *phydev, unsigned short* p)
+{
+       for (;;) {
+               unsigned reg = *p++;
+               unsigned val = *p++;
+               if (reg == 0&&  val == 0)
+                       break;
+               if (reg<  32) {
+                       phy_write(phydev, MDIO_DEVAD_NONE, reg, val);
+               } else {
+                       phy_write(phydev, MDIO_DEVAD_NONE,
+                                       MII_KSZ9021_EXTENDED_CTRL, reg);
+                       phy_write(phydev, MDIO_DEVAD_NONE,
+                                       MII_KSZ9021_EXTENDED_DATAW, val);
+               }

drivers/net/phy/micrel_ksz9021.c line 64

Quoted 3 lines

drivers/net/phy/micrel_ksz9021.c +3 -0

+       }
+       return 0;
+}

For instance, some wacky data-driven initializer function which totally works around any attempts the community might make at dealing with initialization in a transparent and functional way, and is essentially a whole (undocumented) API in and of itself.

Basically, this is the sort of capability that all PHYs need, and if the current framework isn't sufficient to this PHY's needs, then we should look at modifying PHY Lib to provide better board-level initialization capabilities.

An earlier version of something like PHY Lib had a similar type of mechanism, but a list of register writes is often insufficient to the task of performing necessary initialization. Sometimes, one needs to wait until a write takes effect, or do a different write based on link state information, or make other sorts of decisions.

drivers/net/phy/phy.c int phy_init(void) · line 443

Quoted 13 lines

drivers/net/phy/phy.c +3 -0

index eb55180..7e1e4b6 100644
@@ -438,6 +438,9 @@ int phy_init(void)
  #ifdef CONFIG_PHY_MICREL
        phy_micrel_init();
  #endif
+#ifdef CONFIG_PHY_MICREL_KSZ9021
+       phy_ksz9021_init();
+#endif

I believe we're sorting this list alphabetically....

And now I see that this is actually a Micrel PHY. Why are you making a separate file for it? Put this code in the micrel.c file.

Quoted 4 lines

drivers/net/phy/phy.c +0 -0

  #ifdef CONFIG_PHY_NATSEMI
        phy_natsemi_init();
  #endif

Quoted 8 lines · Andy Fleming

NAK, for reasons listed below.

An earlier version of something like PHY Lib had a similar type of mechanism, but a list of register writes is often insufficient to the task of performing necessary initialization. Sometimes, one needs to wait until a write takes effect, or do a different write based on link state information, or make other sorts of decisions.

I thought that was a clean method of keeping board specific things in the board.h file,
without needing to duplicate code in the board.c file.

Please suggest an alternative method. Right now, I'm just dead in the water and
cannot see the way forward. Do you want a more complete API? Or do you want want this as a function in the board file?

I could use board_phy_config, but that would mean duplicated code in other board files.
The only sticking point is that board_phy_config is called after drv->config, not before or
instead of. If it was changed to instead of, I could add the call to phydev->drv->config(phydev)
to all existing board_phy_config functions. Currently that is

board/freescale/corenet_ds/eth_p4080.c
board/freescale/mpc8544ds/mpc8544ds.c

Quoted 18 lines

Quoted 12 lines

drivers/net/phy/phy.c +0 -0

index eb55180..7e1e4b6 100644
@@ -438,6 +438,9 @@ int phy_init(void)

?#ifdef CONFIG_PHY_MICREL
? ? ? ?phy_micrel_init();
?#endif

drivers/net/phy/phy.c int phy_init(void) · line 443

Quoted 3 lines

drivers/net/phy/phy.c +3 -0

+#ifdef CONFIG_PHY_MICREL_KSZ9021
+ ? ? ? phy_ksz9021_init();
+#endif

I believe we're sorting this list alphabetically....

Quoted 2 lines

And now I see that this is actually a Micrel PHY. Why are you making a separate file for it? Put this code in the micrel.c file.

So, you want ifdefs in the micrel.c file? ?That seems unnecessarily ugly. The micrel file currently supports only the ksz804 and uses only genphy_xxx functions. Perhaps renaming this file to micrel_ksz804.c would be more appropriate. Alternatively, change the name to genphy.c and change the structure to
static struct phy_driver genphy_driver = {
? ?.name = CONFIG_GENPHY_NAME,
? ?.uid = CONFIG_GENPHY_UID,
? ?.mask = CONFIG_GENPHY_MASK,
? ?.features = PHY_BASIC_FEATURES,
? ?.config = &genphy_config,
? ?.startup = &genphy_startup,
? ?.shutdown = &genphy_shutdown,
};

So that all phys which are generic can be easily defined by the board which uses it.

I think phy_init should be changed to traverse a section created by the linker so that all these ifdefs could disappear. ?Something like

#define __phy_init_section ? __attribute__ ((__section__ (".data.phy_init_section")))
typedef int(*phy_init_rtn)(void);

static phy_init_rtn ksz9021_phy_init_rtn __phy_init_section = phy_ksz9021_init;

extern phy_init_rtn __phy_init_section_start, __phy_init_section_end;

int phy_init(void)
{
? ?phy_init_rtn* rtn = &__phy_init_section_start;
? ?while (rtn < &__phy_init_section_end) {
? ? ? ?rtn();
? ? ? ?rtn++;
? ?}
}

But that is beyond the scope of this patch. Would you like a patch to convert to this
before this patch?

Quoted 8 lines · Troy Kisky

Quoted 7 lines · Andy Fleming

NAK, for reasons listed below.

An earlier version of something like PHY Lib had a similar type of mechanism, but a list of register writes is often insufficient to the task of performing necessary initialization. Sometimes, one needs to wait until a write takes effect, or do a different write based on link state information, or make other sorts of decisions.

Quoted 3 lines

I thought that was a clean method of keeping board specific things in the board.h file,
without needing to duplicate code in the board.c file.

It seems that way, but, as I said, it can lead to more problems in the future. It also makes the reasons for the initialization sequences much less comprehensible.

Quoted 15 lines

Please suggest an alternative method. Right now, I'm just dead in the water and
cannot see the way forward. Do you want a more complete API? Or do you want want this as a function in the board file?

I could use board_phy_config, but that would mean duplicated code in other board files.
The only sticking point is that board_phy_config is called after drv->config, not before or
instead of. If it was changed to instead of, I could add the call to phydev->drv->config(phydev)
to all existing board_phy_config functions. Currently that is

Quoted 2 lines

board/freescale/corenet_ds/eth_p4080.c
board/freescale/mpc8544ds/mpc8544ds.c

Calling board_phy_config() instead of drv->config() might be the most straightforward solution. You don't have to have duplicate code, though. If a given set of boards will have the same initialization sequence, then share the code between them. Also, as these sequences are all probably either enabling/disabling well-understood features, or configuration, you could put those functions in the PHY driver code, and your board code ends up looking like:

/* Muxing on bus adds extra delay on RX path */
ksz9021_set_rx_delay(phydev, 12ns);

/* New isolinear circuits go to 11 */
ksz9021_adjust_resonance_frequency(11);

/* Accidental feedback loop caused PHY to achieve homicidal rage */ ksz9021_disable_emotion_chip(phydev);

And there are other factors, which could be handled *generically* by the config() function for your PHY:

if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
    int cfg = phy_read(phydev, KSZ9021_CONFIG_SPECIAL);
    cfg |= KSZ021_SGMII_MODE;
    phy_write(phydev, KSZ9021_CONFIG_SPECIAL, cfg);
} else ...

That's the best solution, as it helps to contain knowledge about the PHY inside the driver. However, there are definitely times when the PHY requires some very specific configuration steps, which are best contained in board code.

Summary: Yes, I think it would be a good idea to convert all the current calls to phydev->drv->config to board_phy_config(), which should default to just calling drv->config().

Quoted 34 lines

Quoted 16 lines

Quoted 8 lines

drivers/net/phy/phy.c +0 -0

index eb55180..7e1e4b6 100644

drivers/net/phy/phy.c int phy_init(void) · line 443

Quoted 6 lines

drivers/net/phy/phy.c +3 -0

@@ -438,6 +438,9 @@ int phy_init(void)
  #ifdef CONFIG_PHY_MICREL
        phy_micrel_init();
  #endif
+#ifdef CONFIG_PHY_MICREL_KSZ9021
+       phy_ksz9021_init();
+#endif

I believe we're sorting this list alphabetically....

Quoted 2 lines

And now I see that this is actually a Micrel PHY. Why are you making a separate file for it? Put this code in the micrel.c file.

So, you want ifdefs in the micrel.c file? That seems unnecessarily ugly. The micrel file currently supports only the ksz804 and uses only genphy_xxx functions. Perhaps renaming this file to micrel_ksz804.c would be more appropriate. Alternatively, change the name to genphy.c and change the structure to
static struct phy_driver genphy_driver = {
.name = CONFIG_GENPHY_NAME,
.uid = CONFIG_GENPHY_UID,
.mask = CONFIG_GENPHY_MASK,
.features = PHY_BASIC_FEATURES,
.config =&genphy_config,
.startup =&genphy_startup,
.shutdown =&genphy_shutdown,
};

Quoted 2 lines

So that all phys which are generic can be easily defined by the board which uses it.

I meant you should follow the example of marvell.c or vitesse.c, and put all Micrel PHY code in the micrel.c file. Frequently, the different PHYs from the same companies share common features. We may divide them up differently, later, but, if anything, this driver should be the "micrel" driver, and the existing one is just a stub.

The idea of allowing the Micrel stub PHY to exist was so that someone might come along one day, and decide to add to its functionality. It seems likely that your code may get repurposed by some other developer to better support the 804 on her board.

Quoted 24 lines

I think phy_init should be changed to traverse a section created by the linker so that all these ifdefs could disappear. Something like

#define __phy_init_section __attribute__ ((__section__
(".data.phy_init_section")))
typedef int(*phy_init_rtn)(void);

static phy_init_rtn ksz9021_phy_init_rtn __phy_init_section = phy_ksz9021_init;

extern phy_init_rtn __phy_init_section_start, __phy_init_section_end;

int phy_init(void)

{
    phy_init_rtn* rtn =&__phy_init_section_start;
    while (rtn<  &__phy_init_section_end) {
        rtn();
        rtn++;
    }
}

Quoted 3 lines

But that is beyond the scope of this patch. Would you like a patch to convert to this
before this patch?

That's fine by me, though I think it's a bit overkill. As I said, there are other examples of collecting multiple PHY drivers into one file. It wastes a little space and time, but only a little, to my mind. If we want to be more precise, we can always add more specific #ifdefs. I just want to avoid a situation where we have hundreds or thousands of PHY drivers, and right now we've standardized on one init function and one file per PHY vendor.

Andy

Signed-off-by Troy Kisky <[email protected]>

drivers/net/fec_mxc.c +1 -0

index 3affda8..3fffe79 100644
@@ -378,6 +378,7 @@ static int fec_set_hwaddr(struct eth_device *dev)
 ?static int fec_open(struct eth_device *edev)
 ?{
 ? ? ? ?struct fec_priv *fec = (struct fec_priv *)edev->priv;
+ ? ? ? int speed;
 
 ? ? ? ?debug("fec_open: fec_open(dev)\n");

? ? ? ?/* full-duplex, heartbeat disabled */

drivers/net/fec_mxc.c +0 -0

@@ -427,8 +428,23 @@ static int fec_open(struct eth_device *edev)

?#endif

? ? ? ?miiphy_wait_aneg(edev);
- ? ? ? miiphy_speed(edev->name, fec->phy_id);
+ ? ? ? speed = miiphy_speed(edev->name, fec->phy_id); ? ? ? ?miiphy_duplex(edev->name, fec->phy_id); +#ifdef CONFIG_MX6Q