On Thu, Jan 26, 2012 at 4:21 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:

Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com

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

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 3fffe79..4d7a38d 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -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)        }  #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

On Mon, Jan 30, 2012 at 8:13 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:

On 1/29/2012 7:04 PM, Andy Fleming wrote:

...

On Thu, Jan 26, 2012 at 4:21 PM, Troy Kisky troy.kisky@boundarydevices.com  wrote:

...

Signed-off-by: Troy Kiskytroy.kisky@boundarydevices.com

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

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 3fffe79..4d7a38d 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -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)        }  #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

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

On 1/29/2012 7:26 PM, Andy Fleming wrote:

NAK, for reasons listed below.

On Thu, Jan 26, 2012 at 4:21 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:

...

Add the gigabit phy KSZ9021.

Signed-off-by: Troy Kiskytroy.kisky@boundarydevices.com

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

...

---

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

[...]

...

diff --git a/drivers/net/phy/micrel_ksz9021.c b/drivers/net/phy/micrel_ksz9021.c

+/* 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);
  

•           }
  

•   }
  

•   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.

This should be a separate patch including documentation. But that doesn't sound sufficient for you.

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.

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

...

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index eb55180..7e1e4b6 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -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.

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?

...

#ifdef CONFIG_PHY_NATSEMI phy_natsemi_init(); #endif

On 1/30/2012 5:05 PM, Andy Fleming wrote:

On Mon, Jan 30, 2012 at 3:30 PM, Troy Kisky troy.kisky@boundarydevices.com wrote:

...

On 1/29/2012 7:26 PM, Andy Fleming wrote:

...

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.

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.

I'll take your word, as I have more difficulty imagining your future.

...

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

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().

Can do. Thanks for providing a direction.

...

...

...

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index eb55180..7e1e4b6 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -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.

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

I'm doubtful, since that code has not changed since your initial commit back in April. Is anyone still using it today? Seems only powerPC platforms with CONFIG_TSEC_ENET have had a chance. But I guess that is an irrelevant tangent.

...

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?

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.

Yes, if some code is shared. I does make sense to me to have both in the same file or have a separate file with the shared code rather than cut-n-pasting the code.

However, a hard rule of only one file per vendor doesn't make sense to me. If Micrel buys out vitesse will you combine the files? I doubt it.

On the other hand, as this is way way way down on the list of things I care about, and seems much higher on your list, putting it in micrel.c is fine with me.

Andy