[U-Boot] [PATCH 1/4] net: fec_mxc: add 1000 Mbps selection

[U-Boot] [PATCH 1/4] net: fec_mxc: add 1000 Mbps selection

[Troy Kisky]

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/fec_mxc.c |   18 +++++++++++++++++-
 drivers/net/fec_mxc.h |    2 ++
 2 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
index 3affda8..3fffe79 100644
--- a/drivers/net/fec_mxc.c
+++ b/drivers/net/fec_mxc.c
@@ -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 */
@@ -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
+	{
+		u32 ecr = readl(&fec->eth->ecntrl) & ~FEC_ECNTRL_SPEED;
+		u32 rcr = (readl(&fec->eth->r_cntrl) &
+				~(FEC_RCNTRL_RMII | FEC_RCNTRL_RMII_10T)) |
+				FEC_RCNTRL_RGMII | FEC_RCNTRL_MII_MODE;
+		if (speed == _1000BASET)
+			ecr |= FEC_ECNTRL_SPEED;
+		else if (speed != _100BASET)
+			rcr |= FEC_RCNTRL_RMII_10T;
+		writel(ecr, &fec->eth->ecntrl);
+		writel(rcr, &fec->eth->r_cntrl);
+	}
+#endif
+	debug("%s:Speed=%i\n", __func__, speed);
 
 	/*
 	 * Enable SmartDMA receive task
diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h
index 39337bf..1d6ab06 100644
--- a/drivers/net/fec_mxc.h
+++ b/drivers/net/fec_mxc.h
@@ -198,6 +198,7 @@ struct ethernet_regs {
 #define FEC_RCNTRL_FCE			0x00000020
 #define FEC_RCNTRL_RGMII		0x00000040
 #define FEC_RCNTRL_RMII			0x00000100
+#define FEC_RCNTRL_RMII_10T		0x00000200
 
 #define FEC_TCNTRL_GTS			0x00000001
 #define FEC_TCNTRL_HBC			0x00000002
@@ -207,6 +208,7 @@ struct ethernet_regs {
 
 #define FEC_ECNTRL_RESET		0x00000001	/* reset the FEC */
 #define FEC_ECNTRL_ETHER_EN		0x00000002	/* enable the FEC */
+#define FEC_ECNTRL_SPEED		0x00000020
 #define FEC_ECNTRL_DBSWAP		0x00000100
 
 #define FEC_X_WMRK_STRFWD		0x00000100
-- 
1.7.5.4

[U-Boot] [PATCH 2/4] net: fec_mxc: add PHYLIB support

[Troy Kisky]

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
+		speed = fec->phydev->speed;
+	} else {
+		miiphy_wait_aneg(edev);
+		speed = miiphy_speed(edev->name, fec->phy_id);
+		miiphy_duplex(edev->name, fec->phy_id);
+	}
+
+
 #ifdef CONFIG_MX6Q
 	{
 		u32 ecr = readl(&fec->eth->ecntrl) & ~FEC_ECNTRL_SPEED;
@@ -556,7 +580,7 @@ static int fec_init(struct eth_device *dev, bd_t* bd)
 	fec_tbd_init(fec);
 
 
-	if (fec->xcv_type != SEVENWIRE)
+	if (!fec->phydev && fec->xcv_type != SEVENWIRE)
 		miiphy_restart_aneg(dev);
 
 	fec_open(dev);
@@ -842,7 +866,8 @@ static int fec_probe(bd_t *bd, int dev_id, int phy_id, uint32_t base_addr)
 		debug("got MAC address from fuse: %pM\n", ethaddr);
 		memcpy(edev->enetaddr, ethaddr, 6);
 	}
-
+	/* Configure phy */
+	fec_eth_phy_config(edev);
 	return ret;
 
 err3:
diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h
index 1d6ab06..36125b5 100644
--- a/drivers/net/fec_mxc.h
+++ b/drivers/net/fec_mxc.h
@@ -286,6 +286,7 @@ struct fec_priv {
 	void *base_ptr;
 	int dev_id;
 	int phy_id;
+	struct phy_device *phydev;
 	int (*mii_postcall)(int);
 };
 
-- 
1.7.5.4

[U-Boot] [PATCH 2/4] net: fec_mxc: add PHYLIB support

[Andy Fleming]

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

[U-Boot] [PATCH 2/4] net: fec_mxc: add PHYLIB support

[Troy Kisky]

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

Thanks
Troy

[U-Boot] [PATCH 2/4] net: fec_mxc: add PHYLIB support

[Andy Fleming]

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

[U-Boot] [PATCH 2/4] net: fec_mxc: add PHYLIB support

[Troy Kisky]

On 1/31/2012 5:29 PM, Andy Fleming wrote:
> 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 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
>>>
>> 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
>

I think I followed you that time.

Thanks
Troy

[U-Boot] [PATCH 3/4] net: phy: add support for Micrel's KSZ9021

[Troy Kisky]

Add the gigabit phy KSZ9021.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 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/Makefile b/drivers/net/phy/Makefile
index feced39..86ec3c5 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -36,6 +36,7 @@ COBJS-$(CONFIG_PHY_DAVICOM) += davicom.o
 COBJS-$(CONFIG_PHY_LXT) += lxt.o
 COBJS-$(CONFIG_PHY_MARVELL) += marvell.o
 COBJS-$(CONFIG_PHY_MICREL) += micrel.o
+COBJS-$(CONFIG_PHY_MICREL_KSZ9021) += micrel_ksz9021.o
 COBJS-$(CONFIG_PHY_NATSEMI) += natsemi.o
 COBJS-$(CONFIG_PHY_REALTEK) += realtek.o
 COBJS-$(CONFIG_PHY_SMSC) += smsc.o
diff --git a/drivers/net/phy/micrel_ksz9021.c b/drivers/net/phy/micrel_ksz9021.c
new file mode 100644
index 0000000..f17b798
--- /dev/null
+++ b/drivers/net/phy/micrel_ksz9021.c
@@ -0,0 +1,124 @@
+/*
+ * Micrel KSZ9021 PHY driver
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ *
+ * Copyright 2012 Boundary Devices
+ *
+ */
+#include <config.h>
+#include <common.h>
+#include <phy.h>
+
+/* ksz9021 PHY Registers */
+#define MII_KSZ9021_EXTENDED_CTRL	0x0b
+#define MII_KSZ9021_EXTENDED_DATAW	0x0c
+#define MII_KSZ9021_PHY_CTL		0x1f
+#define MIIM_KSZ9021_PHYCTL_1000	(1 << 6)
+#define MIIM_KSZ9021_PHYCTL_100		(1 << 5)
+#define MIIM_KSZ9021_PHYCTL_10		(1 << 4)
+#define MIIM_KSZ9021_PHYCTL_DUPLEX	(1 << 3)
+
+#define CTRL1000_PREFER_MASTER		(1 << 10)
+#define CTRL1000_CONFIG_MASTER		(1 << 11)
+#define CTRL1000_MANUAL_CONFIG		(1 << 12)
+
+/* 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;
+}
+
+/* Micrel ksz9021 */
+static int ksz9021_config(struct phy_device *phydev)
+{
+	unsigned ctrl1000 = 0;
+	const unsigned master = CTRL1000_PREFER_MASTER |
+			CTRL1000_CONFIG_MASTER | CTRL1000_MANUAL_CONFIG;
+	unsigned features = phydev->drv->features;
+
+	ksz9021_send_phy_cmds(phydev, ksz9021_por_cmds);
+	if (getenv("disable_giga"))
+		features &= ~(SUPPORTED_1000baseT_Half |
+				SUPPORTED_1000baseT_Full);
+	/* force master mode for 1000BaseT due to chip errata */
+	if (features & SUPPORTED_1000baseT_Half)
+		ctrl1000 |= ADVERTISE_1000HALF | master;
+	if (features & SUPPORTED_1000baseT_Full)
+		ctrl1000 |= ADVERTISE_1000FULL | master;
+	phydev->advertising = phydev->supported = features;
+	phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000, ctrl1000);
+	genphy_config_aneg(phydev);
+	genphy_restart_aneg(phydev);
+	return 0;
+}
+
+static int ksz9021_startup(struct phy_device *phydev)
+{
+	unsigned phy_ctl;
+	genphy_update_link(phydev);
+	phy_ctl = phy_read(phydev, MDIO_DEVAD_NONE, MII_KSZ9021_PHY_CTL);
+
+	if (phy_ctl & MIIM_KSZ9021_PHYCTL_DUPLEX)
+		phydev->duplex = DUPLEX_FULL;
+	else
+		phydev->duplex = DUPLEX_HALF;
+
+	if (phy_ctl & MIIM_KSZ9021_PHYCTL_1000)
+		phydev->speed = SPEED_1000;
+	else if (phy_ctl & MIIM_KSZ9021_PHYCTL_100)
+		phydev->speed = SPEED_100;
+	else if (phy_ctl & MIIM_KSZ9021_PHYCTL_10)
+		phydev->speed = SPEED_10;
+	return 0;
+}
+
+static struct phy_driver ksz9021_driver = {
+	.name = "Micrel ksz9021",
+	.uid  = 0x221610,
+	.mask = 0xfffff0,
+	.features = PHY_GBIT_FEATURES,
+	.config = &ksz9021_config,
+	.startup = &ksz9021_startup,
+	.shutdown = &genphy_shutdown,
+};
+
+int phy_ksz9021_init(void)
+{
+	phy_register(&ksz9021_driver);
+	return 0;
+}
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
 #ifdef CONFIG_PHY_NATSEMI
 	phy_natsemi_init();
 #endif
-- 
1.7.5.4

[U-Boot] [PATCH 3/4] net: phy: add support for Micrel's KSZ9021

[Mike Frysinger]

On Thursday 26 January 2012 17:21:44 Troy Kisky wrote:
> +/* This is used to set board specific things like clock skew */
> +unsigned short ksz9021_por_cmds[] = {

static const

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

static

> +	for (;;) {

personally, i'd prefer:
	while (1) {
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120126/4b3d2373/attachment.pgp>

[U-Boot] [PATCH 3/4] net: phy: add support for Micrel's KSZ9021

[Troy Kisky]

On 1/26/2012 7:54 PM, Mike Frysinger wrote:
> On Thursday 26 January 2012 17:21:44 Troy Kisky wrote:
>> +/* This is used to set board specific things like clock skew */
>> +unsigned short ksz9021_por_cmds[] = {
> static const

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

Thanks
>
>> +	for (;;) {
> personally, i'd prefer:
> 	while (1) {
> -mike
I used to prefer while (1), but then Microsoft started warning about it. 
I doubt GCC ever makes the same decision,
but it is easier to use the same style. And I can't see a downside.

see
http://msdn.microsoft.com/en-us/library/6t66728h%28VS.80%29.aspx

[U-Boot] [PATCH 3/4] net: phy: add support for Micrel's KSZ9021

[Andy Fleming]

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

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.


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

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

[U-Boot] [PATCH 3/4] net: phy: add support for Micrel's KSZ9021

[Troy Kisky]

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

[U-Boot] [PATCH 3/4] net: phy: add support for Micrel's KSZ9021

[Andy Fleming]

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.


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

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

Andy

[U-Boot] [PATCH 3/4] net: phy: add support for Micrel's KSZ9021

[Troy Kisky]

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
>

[U-Boot] [PATCH 4/4] i.mx6q: mx6qsabrelite: Update the network configuration

[Troy Kisky]

Enable the usage of PHY_MICREL_KSZ9021, and
minimize the tx clock delay.

There is an issue with 1000 baseTx mode on early revs
of the SabreLite boards. The center tap pin 9 of the mag RJ45
USB combo was connected to the 3.3 filtered supply. Letting
this pin float solved the problem. Symptoms of the problem
were packets with many extra zeroes tacked on the end, and random
bit flips causing a high rate of CRC errors. 10/100 baseTx worked
fine on all revs. To disable 1000 baseTx for these boards, simply
define the environment variable disable_giga. ie.

setenv disable_giga 1

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 board/freescale/mx6qsabrelite/mx6qsabrelite.c |   43 +-----------------------
 include/configs/mx6qsabrelite.h               |    7 ++++
 2 files changed, 9 insertions(+), 41 deletions(-)

diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
index e6c12a5..77fb79c 100644
--- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
+++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
@@ -187,53 +187,14 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
-#define MII_1000BASET_CTRL		0x9
-#define MII_EXTENDED_CTRL		0xb
-#define MII_EXTENDED_DATAW		0xc
-
-int fecmxc_mii_postcall(int phy)
-{
-	/* prefer master mode */
-	miiphy_write("FEC", phy, MII_1000BASET_CTRL, 0x0f00);
-
-	/* min rx data delay */
-	miiphy_write("FEC", phy, MII_EXTENDED_CTRL, 0x8105);
-	miiphy_write("FEC", phy, MII_EXTENDED_DATAW, 0x0000);
-
-	/* max rx/tx clock delay, min rx/tx control delay */
-	miiphy_write("FEC", phy, MII_EXTENDED_CTRL, 0x8104);
-	miiphy_write("FEC", phy, MII_EXTENDED_DATAW, 0xf0f0);
-	miiphy_write("FEC", phy, MII_EXTENDED_CTRL, 0x104);
-
-	return 0;
-}
-
 int board_eth_init(bd_t *bis)
 {
-	struct eth_device *dev;
 	int ret;
-
 	setup_iomux_enet();
-
 	ret = cpu_eth_init(bis);
-	if (ret) {
+	if (ret)
 		printf("FEC MXC: %s:failed\n", __func__);
-		return ret;
-	}
-
-	dev = eth_get_dev_by_name("FEC");
-	if (!dev) {
-		printf("FEC MXC: Unable to get FEC device entry\n");
-		return -EINVAL;
-	}
-
-	ret = fecmxc_register_mii_postcall(dev, fecmxc_mii_postcall);
-	if (ret) {
-		printf("FEC MXC: Unable to register FEC mii postcall\n");
-		return ret;
-	}
-
-	return 0;
+	return ret;
 }
 
 int board_early_init_f(void)
diff --git a/include/configs/mx6qsabrelite.h b/include/configs/mx6qsabrelite.h
index 86b25d9..cbae4c3 100644
--- a/include/configs/mx6qsabrelite.h
+++ b/include/configs/mx6qsabrelite.h
@@ -66,6 +66,13 @@
 #define	CONFIG_FEC_XCV_TYPE		RGMII
 #define CONFIG_ETHPRIME			"FEC"
 #define CONFIG_FEC_MXC_PHYADDR		6
+#define CONFIG_PHYLIB
+#define CONFIG_PHY_MICREL_KSZ9021
+#define CONFIG_PHY_MICREL_KSZ9021_INIT_CMDS	\
+	0x8105, 0x0000,		/* min rx data delay */	\
+	0x8106, 0x0000,		/* min tx data delay */	\
+	0x8104, 0xf0f0,		/* max rx/tx clock delay, min rx/tx control */
+
 
 /* allow to overwrite serial and ethaddr */
 #define CONFIG_ENV_OVERWRITE
-- 
1.7.5.4

[U-Boot] [PATCH 1/4] net: fec_mxc: add 1000 Mbps selection

[Dirk Behme]

On 26.01.2012 23:21, Troy Kisky wrote:
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

Whole patch series:

Acked-by: Dirk Behme <dirk.behme@de.bosch.com>

Many thanks!

Dirk

[U-Boot] [PATCH 1/4] net: fec_mxc: add 1000 Mbps selection

[Andy Fleming]

> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index 3affda8..3fffe79 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -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 */
> @@ -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

What does this ifdef mean? Can you come up with a name that reflects
the actual configuration difference (ie - it supports gigabit, or it
has the extended whatsit register). When, invariably, the MX7Q (or
whatever) comes out, some unfortunate soul is going to have to figure
out which of the various ifdefs applies *only* to MX6Q, and which
apply also to the new chip.

Even better if this is something that can be determined at runtime.

Andy