[PATCH] phylib: Add support for the LXT973 phy.

[PATCH] phylib: Add support for the LXT973 phy.

[Richard Cochran]

This patch implements a work around for Erratum 5, "3.3 V Fiber Speed
Selection." If the hardware wiring does not respect this erratum, then
fiber optic mode will not work properly.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 drivers/net/phy/lxt.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 50 insertions(+), 1 deletions(-)

diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index 057ecaa..c3de582 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -53,6 +53,9 @@
 
 #define MII_LXT971_ISR		19  /* Interrupt Status Register */
 
+/* register definitions for the 973 */
+#define MII_LXT973_PCR 16 /* Port Configuration Register */
+#define PCR_FIBER_SELECT 1
 
 MODULE_DESCRIPTION("Intel LXT PHY driver");
 MODULE_AUTHOR("Andy Fleming");
@@ -119,6 +122,33 @@ static int lxt971_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static int lxt973_probe(struct phy_device *phydev)
+{
+	int val = phy_read(phydev, MII_LXT973_PCR);
+
+	if (val & PCR_FIBER_SELECT) {
+		/*
+		 * If fiber is selected, then the only correct setting
+		 * is 100Mbps, full duplex, and auto negotiation off.
+		 */
+		val = phy_read(phydev, MII_BMCR);
+		val |= (BMCR_SPEED100 | BMCR_FULLDPLX);
+		val &= ~BMCR_ANENABLE;
+		phy_write(phydev, MII_BMCR, val);
+		/* Remember that the port is in fiber mode. */
+		phydev->priv = lxt973_probe;
+	} else {
+		phydev->priv = NULL;
+	}
+	return 0;
+}
+
+static int lxt973_config_aneg(struct phy_device *phydev)
+{
+	/* Do nothing if port is in fiber mode. */
+	return phydev->priv ? 0 : genphy_config_aneg(phydev);
+}
+
 static struct phy_driver lxt970_driver = {
 	.phy_id		= 0x78100000,
 	.name		= "LXT970",
@@ -146,6 +176,18 @@ static struct phy_driver lxt971_driver = {
 	.driver 	= { .owner = THIS_MODULE,},
 };
 
+static struct phy_driver lxt973_driver = {
+	.phy_id		= 0x00137a10,
+	.name		= "LXT973",
+	.phy_id_mask	= 0xfffffff0,
+	.features	= PHY_BASIC_FEATURES,
+	.flags		= 0,
+	.probe		= lxt973_probe,
+	.config_aneg	= lxt973_config_aneg,
+	.read_status	= genphy_read_status,
+	.driver 	= { .owner = THIS_MODULE,},
+};
+
 static int __init lxt_init(void)
 {
 	int ret;
@@ -157,9 +199,15 @@ static int __init lxt_init(void)
 	ret = phy_driver_register(&lxt971_driver);
 	if (ret)
 		goto err2;
+
+	ret = phy_driver_register(&lxt973_driver);
+	if (ret)
+		goto err3;
 	return 0;
 
- err2:	
+ err3:
+	phy_driver_unregister(&lxt971_driver);
+ err2:
 	phy_driver_unregister(&lxt970_driver);
  err1:
 	return ret;
@@ -169,6 +217,7 @@ static void __exit lxt_exit(void)
 {
 	phy_driver_unregister(&lxt970_driver);
 	phy_driver_unregister(&lxt971_driver);
+	phy_driver_unregister(&lxt973_driver);
 }
 
 module_init(lxt_init);
-- 
1.6.3.3

Re: [PATCH] phylib: Add support for the LXT973 phy.

[Andy Fleming]

On Mon, May 31, 2010 at 8:09 AM, Richard Cochran
<richardcochran@gmail.com> wrote:
> This patch implements a work around for Erratum 5, "3.3 V Fiber Speed
> Selection." If the hardware wiring does not respect this erratum, then
> fiber optic mode will not work properly.
>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>


>
> +static int lxt973_probe(struct phy_device *phydev)
> +{
> +       int val = phy_read(phydev, MII_LXT973_PCR);
> +
> +       if (val & PCR_FIBER_SELECT) {
> +               /*
> +                * If fiber is selected, then the only correct setting
> +                * is 100Mbps, full duplex, and auto negotiation off.
> +                */
> +               val = phy_read(phydev, MII_BMCR);
> +               val |= (BMCR_SPEED100 | BMCR_FULLDPLX);
> +               val &= ~BMCR_ANENABLE;
> +               phy_write(phydev, MII_BMCR, val);
> +               /* Remember that the port is in fiber mode. */
> +               phydev->priv = lxt973_probe;


That's a bit hacky.  There is a dev_flags field, which could be used
for this.  Probably, we should add a more general way of saying what
sort of port this is.  But don't use the presence and absence of
"priv", as it could one day get used for a different purpose, and this
seems like it would leave us open to strange bugs.

Also, is this erratum true for all lxt973 models, or is it fixed in
some revisions?


Andy

Re: [PATCH] phylib: Add support for the LXT973 phy.

[Richard Cochran]

On Tue, Jun 01, 2010 at 05:39:22PM -0500, Andy Fleming wrote:
> That's a bit hacky.  There is a dev_flags field, which could be used
> for this.  Probably, we should add a more general way of saying what
> sort of port this is.  But don't use the presence and absence of
> "priv", as it could one day get used for a different purpose, and this
> seems like it would leave us open to strange bugs.

Okay, I changed it.

At first, I was worried about using 'dev_flags' because I couldn't
tell exactly who may write to this field. Looking at tg.c and
broadcom.c, it appears that the MAC drivers may also write this
field. In contrast, the 'priv' field is surely private.

> Also, is this erratum true for all lxt973 models, or is it fixed in
> some revisions?

The documentation http://www.cortina-systems.com/products/download/266
says, "Status: This erratum has been previously fixed." However, I
could not find a reference to when this was fixed.

Richard

Date: Wed, 2 Jun 2010 13:47:02 +0200
Subject: [PATCH] phylib: Add support for the LXT973 phy.

This patch implements a work around for Erratum 5, "3.3 V Fiber Speed
Selection." If the hardware wiring does not respect this erratum, then
fiber optic mode will not work properly.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 drivers/net/phy/lxt.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index 8ee929b..4f97fdc 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -53,6 +53,9 @@
 
 #define MII_LXT971_ISR		19  /* Interrupt Status Register */
 
+/* register definitions for the 973 */
+#define MII_LXT973_PCR 16 /* Port Configuration Register */
+#define PCR_FIBER_SELECT 1
 
 MODULE_DESCRIPTION("Intel LXT PHY driver");
 MODULE_AUTHOR("Andy Fleming");
@@ -119,6 +122,34 @@ static int lxt971_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static int lxt973_probe(struct phy_device *phydev)
+{
+	int val = phy_read(phydev, MII_LXT973_PCR);
+
+	if (val & PCR_FIBER_SELECT) {
+		/*
+		 * If fiber is selected, then the only correct setting
+		 * is 100Mbps, full duplex, and auto negotiation off.
+		 */
+		val = phy_read(phydev, MII_BMCR);
+		val |= (BMCR_SPEED100 | BMCR_FULLDPLX);
+		val &= ~BMCR_ANENABLE;
+		phy_write(phydev, MII_BMCR, val);
+		/* Remember that the port is in fiber mode. */
+		phydev->dev_flags = PCR_FIBER_SELECT;
+	} else {
+		phydev->dev_flags = 0;
+	}
+	return 0;
+}
+
+static int lxt973_config_aneg(struct phy_device *phydev)
+{
+	/* Do nothing if port is in fiber mode. */
+	return PCR_FIBER_SELECT == phydev->dev_flags ?
+		0 : genphy_config_aneg(phydev);
+}
+
 static struct phy_driver lxt970_driver = {
 	.phy_id		= 0x78100000,
 	.name		= "LXT970",
@@ -146,6 +177,18 @@ static struct phy_driver lxt971_driver = {
 	.driver 	= { .owner = THIS_MODULE,},
 };
 
+static struct phy_driver lxt973_driver = {
+	.phy_id		= 0x00137a10,
+	.name		= "LXT973",
+	.phy_id_mask	= 0xfffffff0,
+	.features	= PHY_BASIC_FEATURES,
+	.flags		= 0,
+	.probe		= lxt973_probe,
+	.config_aneg	= lxt973_config_aneg,
+	.read_status	= genphy_read_status,
+	.driver		= { .owner = THIS_MODULE,},
+};
+
 static int __init lxt_init(void)
 {
 	int ret;
@@ -157,9 +200,15 @@ static int __init lxt_init(void)
 	ret = phy_driver_register(&lxt971_driver);
 	if (ret)
 		goto err2;
+
+	ret = phy_driver_register(&lxt973_driver);
+	if (ret)
+		goto err3;
 	return 0;
 
- err2:	
+ err3:
+	phy_driver_unregister(&lxt971_driver);
+ err2:
 	phy_driver_unregister(&lxt970_driver);
  err1:
 	return ret;
@@ -169,6 +218,7 @@ static void __exit lxt_exit(void)
 {
 	phy_driver_unregister(&lxt970_driver);
 	phy_driver_unregister(&lxt971_driver);
+	phy_driver_unregister(&lxt973_driver);
 }
 
 module_init(lxt_init);
-- 
1.6.3.3

Re: [PATCH] phylib: Add support for the LXT973 phy.

[Richard Cochran]

On Wed, Jun 02, 2010 at 02:55:27PM +0200, Richard Cochran wrote:
> On Tue, Jun 01, 2010 at 05:39:22PM -0500, Andy Fleming wrote:
> > Also, is this erratum true for all lxt973 models, or is it fixed in
> > some revisions?
> 
> The documentation http://www.cortina-systems.com/products/download/266
> says, "Status: This erratum has been previously fixed." However, I
> could not find a reference to when this was fixed.

In any case, the PHY only supports 100 Mbps when in fiber mode, so the
fix is always safe to use.

Richard

Re: [PATCH] phylib: Add support for the LXT973 phy.

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Wed, 2 Jun 2010 14:55:27 +0200

> On Tue, Jun 01, 2010 at 05:39:22PM -0500, Andy Fleming wrote:
>> That's a bit hacky.  There is a dev_flags field, which could be used
>> for this.  Probably, we should add a more general way of saying what
>> sort of port this is.  But don't use the presence and absence of
>> "priv", as it could one day get used for a different purpose, and this
>> seems like it would leave us open to strange bugs.
> 
> Okay, I changed it.
> 
> At first, I was worried about using 'dev_flags' because I couldn't
> tell exactly who may write to this field. Looking at tg.c and
> broadcom.c, it appears that the MAC drivers may also write this
> field. In contrast, the 'priv' field is surely private.

No, I think using dev_flags is absolutely the wrong way to do about
this.

phy_device->priv "could one day get used for a different purpose"?
What in the world are you smoking Andy?

It's clearly a private state pointer for the PHY driver to use,
full stop.  There is absolutely no ambiguity of what this value
is and what it is used for and who owns it.  The comments in the
layout of struct phy_device state this clearly as well.

On the other hand, ->dev_flags is an entirely different matter.  It's
set based upon arguments passed into PHY driver interface attach calls
and used in other various ways by the generic PHY library code.

This is entirely different from ->priv which is not touched at all
by the generic PHY code, and thus ->priv is much safer to use for
private purposes like Richard's case here.

Richard, please respin your patch so that you're using the ->priv
field like in your original patch.

Thanks.

Re: [PATCH] phylib: Add support for the LXT973 phy.

[Richard Cochran]

On Wed, Jun 02, 2010 at 06:50:17AM -0700, David Miller wrote:
> phy_device->priv "could one day get used for a different purpose"?
> What in the world are you smoking Andy?

I think he meant that the 'priv' pointer may one day be used to point
to some dynamically allocated data structure.

> It's clearly a private state pointer for the PHY driver to use,
> full stop.  There is absolutely no ambiguity of what this value
> is and what it is used for and who owns it.  The comments in the
> layout of struct phy_device state this clearly as well.
...
>
> Richard, please respin your patch so that you're using the ->priv
> field like in your original patch.

Well, is it okay to just use the first patch?

The fix needs just one bit of data, and I thought that (ab)using the
'priv' pointer would be okay, if a bit hacky. If, over time, the
driver needs more private data, it should be clear enought that the
existing bit "fiber selected" will also appear in the data structure.

Otherwise, the driver would have to allocate a structure with one
field, just to remember one bit.

Richard

Re: [PATCH] phylib: Add support for the LXT973 phy.

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Wed, 2 Jun 2010 17:08:51 +0200

> Well, is it okay to just use the first patch?

Wasn't there another change you were asked to make that got included
in the v2 patch?

Re: [PATCH] phylib: Add support for the LXT973 phy.

[Richard Cochran]

On Wed, Jun 02, 2010 at 08:15:12AM -0700, David Miller wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> Date: Wed, 2 Jun 2010 17:08:51 +0200
> 
> > Well, is it okay to just use the first patch?
> 
> Wasn't there another change you were asked to make that got included
> in the v2 patch?

No, I don't think so. He did ask whether the erratum has been fixed.
I will implement the 'port' field that he has suggested.

Richard

Re: [PATCH] phylib: Add support for the LXT973 phy.

[Andy Fleming]

On Wed, Jun 2, 2010 at 8:50 AM, David Miller <davem@davemloft.net> wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> Date: Wed, 2 Jun 2010 14:55:27 +0200
>
>> On Tue, Jun 01, 2010 at 05:39:22PM -0500, Andy Fleming wrote:
>>> That's a bit hacky.  There is a dev_flags field, which could be used
>>> for this.  Probably, we should add a more general way of saying what
>>> sort of port this is.  But don't use the presence and absence of
>>> "priv", as it could one day get used for a different purpose, and this
>>> seems like it would leave us open to strange bugs.
>>
>> Okay, I changed it.
>>
>> At first, I was worried about using 'dev_flags' because I couldn't
>> tell exactly who may write to this field. Looking at tg.c and
>> broadcom.c, it appears that the MAC drivers may also write this
>> field. In contrast, the 'priv' field is surely private.
>
> No, I think using dev_flags is absolutely the wrong way to do about
> this.

Yeah, I was clearly not thinking clearly.  dev_flags will be
overwritten, and is not meant for this.  I believe, what we should do
is add a "port" field to the PHY device, and if PCR_FIBER_SELECT is
set, then set the port field to PORT_FIBRE.  I'm not entirely clear on
the semantics of that field in the ethtool cmd, but it seems like the
right idea.


>
> phy_device->priv "could one day get used for a different purpose"?
> What in the world are you smoking Andy?
>
> It's clearly a private state pointer for the PHY driver to use,
> full stop.  There is absolutely no ambiguity of what this value
> is and what it is used for and who owns it.  The comments in the
> layout of struct phy_device state this clearly as well.

Yes, it's private, and I would be fine with using priv, I just didn't
think that it was correct to assign priv to point at the probe
function as a mechanism for indicating that fiber is being used.  I
believe that code should be more explicit than that.  priv is meant to
point at private data, not to indicate an arbitrary attribute of the
link by virtue of being non-null.

Andy

Re: [PATCH] phylib: Add support for the LXT973 phy.

[Richard Cochran]

On Wed, Jun 02, 2010 at 02:32:11PM -0500, Andy Fleming wrote:
> Yeah, I was clearly not thinking clearly.  dev_flags will be
> overwritten, and is not meant for this.  I believe, what we should do
> is add a "port" field to the PHY device, and if PCR_FIBER_SELECT is
> set, then set the port field to PORT_FIBRE.  I'm not entirely clear on
> the semantics of that field in the ethtool cmd, but it seems like the
> right idea.

Here is another try. Is that more like it?

Richard


This patch implements a work around for Erratum 5, "3.3 V Fiber Speed
Selection." If the hardware wiring does not respect this erratum, then
fiber optic mode will not work properly.

As part of the fix, the patch introduces a new field 'port_flags' into
the 'struct phy_device'. This field allows phy drivers to describe
fixed attributes of the port. Only phy drivers should write this field.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 drivers/net/phy/lxt.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/phy.h   |    8 +++++++
 2 files changed, 59 insertions(+), 1 deletions(-)

diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index 8ee929b..ef4a320 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -53,6 +53,9 @@
 
 #define MII_LXT971_ISR		19  /* Interrupt Status Register */
 
+/* register definitions for the 973 */
+#define MII_LXT973_PCR 16 /* Port Configuration Register */
+#define PCR_FIBER_SELECT 1
 
 MODULE_DESCRIPTION("Intel LXT PHY driver");
 MODULE_AUTHOR("Andy Fleming");
@@ -119,6 +122,34 @@ static int lxt971_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static int lxt973_probe(struct phy_device *phydev)
+{
+	int val = phy_read(phydev, MII_LXT973_PCR);
+
+	if (val & PCR_FIBER_SELECT) {
+		/*
+		 * If fiber is selected, then the only correct setting
+		 * is 100Mbps, full duplex, and auto negotiation off.
+		 */
+		val = phy_read(phydev, MII_BMCR);
+		val |= (BMCR_SPEED100 | BMCR_FULLDPLX);
+		val &= ~BMCR_ANENABLE;
+		phy_write(phydev, MII_BMCR, val);
+		/* Remember that the port is in fiber mode. */
+		phydev->port_flags |= PHY_PORT_FIBER;
+	} else {
+		phydev->port_flags &= ~PHY_PORT_FIBER;
+	}
+	return 0;
+}
+
+static int lxt973_config_aneg(struct phy_device *phydev)
+{
+	/* Do nothing if port is in fiber mode. */
+	return phydev->port_flags & PHY_PORT_FIBER ?
+		0 : genphy_config_aneg(phydev);
+}
+
 static struct phy_driver lxt970_driver = {
 	.phy_id		= 0x78100000,
 	.name		= "LXT970",
@@ -146,6 +177,18 @@ static struct phy_driver lxt971_driver = {
 	.driver 	= { .owner = THIS_MODULE,},
 };
 
+static struct phy_driver lxt973_driver = {
+	.phy_id		= 0x00137a10,
+	.name		= "LXT973",
+	.phy_id_mask	= 0xfffffff0,
+	.features	= PHY_BASIC_FEATURES,
+	.flags		= 0,
+	.probe		= lxt973_probe,
+	.config_aneg	= lxt973_config_aneg,
+	.read_status	= genphy_read_status,
+	.driver		= { .owner = THIS_MODULE,},
+};
+
 static int __init lxt_init(void)
 {
 	int ret;
@@ -157,9 +200,15 @@ static int __init lxt_init(void)
 	ret = phy_driver_register(&lxt971_driver);
 	if (ret)
 		goto err2;
+
+	ret = phy_driver_register(&lxt973_driver);
+	if (ret)
+		goto err3;
 	return 0;
 
- err2:	
+ err3:
+	phy_driver_unregister(&lxt971_driver);
+ err2:
 	phy_driver_unregister(&lxt970_driver);
  err1:
 	return ret;
@@ -169,6 +218,7 @@ static void __exit lxt_exit(void)
 {
 	phy_driver_unregister(&lxt970_driver);
 	phy_driver_unregister(&lxt971_driver);
+	phy_driver_unregister(&lxt973_driver);
 }
 
 module_init(lxt_init);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1c75b6b..602228c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -234,6 +234,11 @@ enum phy_state {
 	PHY_RESUMING
 };
 
+/*
+ * PHY_PORT_xxx: flags to describe the port's fixed attributes.
+ */
+#define PHY_PORT_FIBER 0x00000001 /* Port has a fiber optic transceiver */
+
 /* phy_device: An instance of a PHY
  *
  * drv: Pointer to the driver for this PHY instance
@@ -246,6 +251,7 @@ enum phy_state {
  * link_timeout: The number of timer firings to wait before the
  * giving up on the current attempt at acquiring a link
  * irq: IRQ number of the PHY's interrupt (-1 if none)
+ * port_flags: Bit field of PHY_PORT_xxx flags
  * phy_timer: The timer for handling the state machine
  * phy_queue: A work_queue for the interrupt
  * attached_dev: The attached enet driver's device instance ptr
@@ -314,6 +320,8 @@ struct phy_device {
 	 */
 	int irq;
 
+	int port_flags;
+
 	/* private data pointer */
 	/* For use by PHYs to maintain extra state */
 	void *priv;
-- 
1.6.3.3

Re: [PATCH] phylib: Add support for the LXT973 phy.

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Sat, 5 Jun 2010 16:00:17 +0200

> On Wed, Jun 02, 2010 at 02:32:11PM -0500, Andy Fleming wrote:
>> Yeah, I was clearly not thinking clearly.  dev_flags will be
>> overwritten, and is not meant for this.  I believe, what we should do
>> is add a "port" field to the PHY device, and if PCR_FIBER_SELECT is
>> set, then set the port field to PORT_FIBRE.  I'm not entirely clear on
>> the semantics of that field in the ethtool cmd, but it seems like the
>> right idea.
> 
> Here is another try. Is that more like it?

I think this is overkill.

One, and only one, PHY driver wants to maintain a boolean state and
we're adding a full 32-bit flags member to the generic PHY device
struct to accomodate this?

Andy if you have ideas to use that state via ethtool or whatever in
the future, you come back to us when the future arrives and you've
implemented the code in the generic PHY lib code to do that stuff.

As it stands right now, that code doesn't exist so accomodating it is
just silly.

I also think worrying about the phy_dev->priv pointer being misused in
the future inside of this driver is a completely bogus argument by any
measure.

We have many cases all over the kernel tree, in drivers and elsewhere,
where we encode integer states in what are normally pointer values
when we need to maintain a small piece of state and don't need to do a
full blown memory allocation to allocate a piece of extra memory to
hold that state.

Richard, please just resubmit your original patch and I will apply it.

Thanks.

Re: [PATCH] phylib: Add support for the LXT973 phy.

[Andy Fleming]

On Mon, Jun 7, 2010 at 3:18 AM, David Miller <davem@davemloft.net> wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> Date: Sat, 5 Jun 2010 16:00:17 +0200
>
>> On Wed, Jun 02, 2010 at 02:32:11PM -0500, Andy Fleming wrote:
>>> Yeah, I was clearly not thinking clearly.  dev_flags will be
>>> overwritten, and is not meant for this.  I believe, what we should do
>>> is add a "port" field to the PHY device, and if PCR_FIBER_SELECT is
>>> set, then set the port field to PORT_FIBRE.  I'm not entirely clear on
>>> the semantics of that field in the ethtool cmd, but it seems like the
>>> right idea.
>>
>> Here is another try. Is that more like it?
>
> I think this is overkill.

Well, I meant for it to be the same as the ethtool one, and not a flags field.

>
> One, and only one, PHY driver wants to maintain a boolean state and
> we're adding a full 32-bit flags member to the generic PHY device
> struct to accomodate this?


To be fair, this is a generic problem, with a simple solution.  I was
suggesting that a tiny patch would pave the way for others to follow
suit.  Instead, now a tiny patch will do something strange and
incomprehensible, and then will be changed later when some arbitrary
threshold is reached of PHY drivers needing to know what type of port
they are hooked up to.


>
> Andy if you have ideas to use that state via ethtool or whatever in
> the future, you come back to us when the future arrives and you've
> implemented the code in the generic PHY lib code to do that stuff.

Is there some reason for resistance to taking small steps in the right
direction of an obviously good destination (recording the port type)?
At the very least, can I ask that instead of assigning phydev->priv to
the address of the probe function, that we do something like:

phydev->priv = (void *) PCR_FIBER_SELECT;

And then check to make sure it is that value?  It's still hacky
(IMHO), but at least it's self-documenting.


>
> As it stands right now, that code doesn't exist so accomodating it is
> just silly.
>
> I also think worrying about the phy_dev->priv pointer being misused in
> the future inside of this driver is a completely bogus argument by any
> measure.
>
> We have many cases all over the kernel tree, in drivers and elsewhere,
> where we encode integer states in what are normally pointer values
> when we need to maintain a small piece of state and don't need to do a
> full blown memory allocation to allocate a piece of extra memory to
> hold that state.


I would consider it a case of last resort, for when you need to avoid
a memory allocation for performance reasons, and you must use a
generic structure for a non-generic task.  This is something detected
in slow-path code, and is a generic task, so we're only hitting 1/3 of
those conditions.  I won't speculate as to how many of those other
cases in the tree I would find annoying.  ;)

Andy

Re: [PATCH] phylib: Add support for the LXT973 phy.

[Richard Cochran]

On Mon, Jun 07, 2010 at 01:18:35AM -0700, David Miller wrote:
> 
> Richard, please just resubmit your original patch and I will apply it.

Okay, here it is.

Thanks,
Richard


This patch implements a work around for Erratum 5, "3.3 V Fiber Speed
Selection." If the hardware wiring does not respect this erratum, then
fiber optic mode will not work properly.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 drivers/net/phy/lxt.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 50 insertions(+), 1 deletions(-)

diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index 057ecaa..c3de582 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -53,6 +53,9 @@
 
 #define MII_LXT971_ISR		19  /* Interrupt Status Register */
 
+/* register definitions for the 973 */
+#define MII_LXT973_PCR 16 /* Port Configuration Register */
+#define PCR_FIBER_SELECT 1
 
 MODULE_DESCRIPTION("Intel LXT PHY driver");
 MODULE_AUTHOR("Andy Fleming");
@@ -119,6 +122,33 @@ static int lxt971_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static int lxt973_probe(struct phy_device *phydev)
+{
+	int val = phy_read(phydev, MII_LXT973_PCR);
+
+	if (val & PCR_FIBER_SELECT) {
+		/*
+		 * If fiber is selected, then the only correct setting
+		 * is 100Mbps, full duplex, and auto negotiation off.
+		 */
+		val = phy_read(phydev, MII_BMCR);
+		val |= (BMCR_SPEED100 | BMCR_FULLDPLX);
+		val &= ~BMCR_ANENABLE;
+		phy_write(phydev, MII_BMCR, val);
+		/* Remember that the port is in fiber mode. */
+		phydev->priv = lxt973_probe;
+	} else {
+		phydev->priv = NULL;
+	}
+	return 0;
+}
+
+static int lxt973_config_aneg(struct phy_device *phydev)
+{
+	/* Do nothing if port is in fiber mode. */
+	return phydev->priv ? 0 : genphy_config_aneg(phydev);
+}
+
 static struct phy_driver lxt970_driver = {
 	.phy_id		= 0x78100000,
 	.name		= "LXT970",
@@ -146,6 +176,18 @@ static struct phy_driver lxt971_driver = {
 	.driver 	= { .owner = THIS_MODULE,},
 };
 
+static struct phy_driver lxt973_driver = {
+	.phy_id		= 0x00137a10,
+	.name		= "LXT973",
+	.phy_id_mask	= 0xfffffff0,
+	.features	= PHY_BASIC_FEATURES,
+	.flags		= 0,
+	.probe		= lxt973_probe,
+	.config_aneg	= lxt973_config_aneg,
+	.read_status	= genphy_read_status,
+	.driver 	= { .owner = THIS_MODULE,},
+};
+
 static int __init lxt_init(void)
 {
 	int ret;
@@ -157,9 +199,15 @@ static int __init lxt_init(void)
 	ret = phy_driver_register(&lxt971_driver);
 	if (ret)
 		goto err2;
+
+	ret = phy_driver_register(&lxt973_driver);
+	if (ret)
+		goto err3;
 	return 0;
 
- err2:	
+ err3:
+	phy_driver_unregister(&lxt971_driver);
+ err2:
 	phy_driver_unregister(&lxt970_driver);
  err1:
 	return ret;
@@ -169,6 +217,7 @@ static void __exit lxt_exit(void)
 {
 	phy_driver_unregister(&lxt970_driver);
 	phy_driver_unregister(&lxt971_driver);
+	phy_driver_unregister(&lxt973_driver);
 }
 
 module_init(lxt_init);
-- 
1.6.3.3

Re: [PATCH] phylib: Add support for the LXT973 phy.

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Mon, 7 Jun 2010 17:39:32 +0200

> This patch implements a work around for Erratum 5, "3.3 V Fiber Speed
> Selection." If the hardware wiring does not respect this erratum, then
> fiber optic mode will not work properly.
> 
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>

Applied, thanks.

[PATCH] phylib: Add autoload support for the LXT973 phy.

[David Woodhouse]

Commit e13647c1 (phylib: Add support for the LXT973 phy.) added a new ID
but neglected to also add it to the MODULE_DEVICE_TABLE.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
When I did this stuff, I did wonder if we should make this happen
automatically somehow. I pondered some dirty macro hack in
phy_driver_register() which would do it somehow, but couldn't come up
with anything that'd work.

Removing the phy_id and phy_id_mask from struct phy_driver and having a
pointer to a match table would suck, since each driver only really
matches one device/mask. (Even where a single C file has multiple
drivers, they often differ in some methods or flags.)

The best option I can come up with right now, is probably to remove
phy_id and phy_id_mask from phy_driver and put a pointer to the driver
into the ID table, and take the ID table as the argument to
phy_driver_register(). I'm not sure I like that very much though -- I'd
prefer that we just remember to update the table and don't need to be
forced :)

(Another cheap option is to pass the ID table as an extra argument to
the existing phy_device_register(), I suppose, and it can just print a
warning if it doesn't find the same phy_id and phy_id_mask in the table)

diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index dbd0034..29c39ff 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -226,6 +226,7 @@ module_exit(lxt_exit);
 static struct mdio_device_id lxt_tbl[] = {
 	{ 0x78100000, 0xfffffff0 },
 	{ 0x001378e0, 0xfffffff0 },
+	{ 0x00137a10, 0xfffffff0 },
 	{ }
 };
 

Re: [PATCH] phylib: Add autoload support for the LXT973 phy.

[Richard Cochran]

On Tue, Jun 22, 2010 at 01:38:13PM +0100, David Woodhouse wrote:
> prefer that we just remember to update the table and don't need to be
> forced :)

Oops, and thanks for catching this.

>  static struct mdio_device_id lxt_tbl[] = {
>  	{ 0x78100000, 0xfffffff0 },
>  	{ 0x001378e0, 0xfffffff0 },
> +	{ 0x00137a10, 0xfffffff0 },
>  	{ }
>  };

Question about the whole PHY MODULE_DEVICE_TABLE system:

I recently posted a phy driver for the National Semiconductor
DP83640. During development, I used drivers/net/arm/ixp4xx_eth.c as
the MAC driver, which was linked into the kernel (not a module). I
noticed that the phy driver's probe function only gets called if the
phy driver is also statically linked, but not when it is loaded as a
module.

Is this the correct behavior?

Thanks,

Richard

Re: [PATCH] phylib: Add autoload support for the LXT973 phy.

[David Woodhouse]

On Wed, 2010-06-23 at 07:37 +0200, Richard Cochran wrote:
> 
> Question about the whole PHY MODULE_DEVICE_TABLE system:
> 
> I recently posted a phy driver for the National Semiconductor
> DP83640. During development, I used drivers/net/arm/ixp4xx_eth.c as
> the MAC driver, which was linked into the kernel (not a module). I
> noticed that the phy driver's probe function only gets called if the
> phy driver is also statically linked, but not when it is loaded as a
> module.
>
> Is this the correct behavior? 

Hm, that seems like the _expected_ behaviour, certainly. The MAC driver
will probe its device at boot time, and will issue a request_module() to
load the a specific PHY driver if there is one. When no such module
turns up (which it won't if you have no file system mounted yet), it'll
just fall back to the generic PHY support.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] phylib: Add autoload support for the LXT973 phy.

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Tue, 22 Jun 2010 13:38:13 +0100

> Commit e13647c1 (phylib: Add support for the LXT973 phy.) added a new ID
> but neglected to also add it to the MODULE_DEVICE_TABLE.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Applied, thanks.

> When I did this stuff, I did wonder if we should make this happen
> automatically somehow. I pondered some dirty macro hack in
> phy_driver_register() which would do it somehow, but couldn't come up
> with anything that'd work.
> 
> Removing the phy_id and phy_id_mask from struct phy_driver and having a
> pointer to a match table would suck, since each driver only really
> matches one device/mask. (Even where a single C file has multiple
> drivers, they often differ in some methods or flags.)
> 
> The best option I can come up with right now, is probably to remove
> phy_id and phy_id_mask from phy_driver and put a pointer to the driver
> into the ID table, and take the ID table as the argument to
> phy_driver_register(). I'm not sure I like that very much though -- I'd
> prefer that we just remember to update the table and don't need to be
> forced :)
> 
> (Another cheap option is to pass the ID table as an extra argument to
> the existing phy_device_register(), I suppose, and it can just print a
> warning if it doesn't find the same phy_id and phy_id_mask in the table)

As our experience shows, people aren't remembering to do it so we have
to do something hard handed to make sure this doesn't break.

A compile time error out is the best, but if that is too hard or ugly
and we do it at run time then we should fail the register (not just
print a warning) if the table is incomplete.

Otherwise we run into cases where a developer adds several new IDs,
forgets some of the table entries, but only tries testing the ones he
did remember to add and doesn't notice the warning message.