diff mbox

phylib: Add support for the LXT973 phy.

Message ID 20100602125527.GA20396@riccoc20.at.omicron.at
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Richard Cochran June 2, 2010, 12:55 p.m. UTC 
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(-)

Comments

Richard Cochran June 2, 2010, 1:07 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller June 2, 2010, 1:50 p.m. UTC | #2 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Cochran June 2, 2010, 3:08 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller June 2, 2010, 3:15 p.m. UTC | #4 
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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Fleming June 2, 2010, 7:32 p.m. UTC | #5 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Cochran June 3, 2010, 11:28 a.m. UTC | #6 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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