[PATCH] PHYLIB: Add 1000Base-X support for Broadcom bcm5482

[PATCH] PHYLIB: Add 1000Base-X support for Broadcom bcm5482

[Nate Case]

Configure the BCM5482S secondary SerDes for 1000Base-X mode when the
appropriate dev_flags are passed in to phy_connect().  This is
needed when the PHY is used for fiber and backplane connections.

Signed-off-by: Nate Case <ncase@xes-inc.com>
---
Note: This contains a few "magic" numbers/bits which are documented
in the comments.  I neglected making #defines for all of these to
keep the change size down, but I'm willing to make them if people want
it enough.  Most are probably 5482 specific.

 drivers/net/phy/broadcom.c |  153 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 151 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 60c5cfe..a0ccb96 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -42,10 +42,83 @@
 #define MII_BCM54XX_INT_MDIX	0x2000	/* MDIX status change */
 #define MII_BCM54XX_INT_PSERR	0x4000	/* Pair swap error */
 
+#define MII_BCM54XX_RSV0	0x15	/* Reserved / Expansion registers */
+#define MII_BCM54XX_EXP		0x17	/* Expansion register access */
+#define MII_BCM54XX_EXP_SSD	0x0e00	/* Secondary SerDes select */
+#define MII_BCM54XX_EXP_ER	0x0f00	/* Expansion register select */
+#define MII_BCM54XX_EXP_NONE	0x0000	/* Exp register not selected */
+
+#define MII_BCM54XX_AUX_CTL	0x18
+
+#define MII_BCM54XX_SHD		0x1c	/* 0x1c shadow registers */
+#define MII_BCM54XX_SHD_WRITE	0x8000
+#define MII_BCM54XX_SHD_VAL(x)	((x & 0x1f) << 10)
+#define MII_BCM54XX_SHD_DATA(x)	((x & 0x3ff) << 0)
+
+/*
+ * Device flags for PHYs that can be configured for different operating
+ * modes
+ */
+#define PHY_BCM_FLAGS_VALID		0x80000000
+#define PHY_BCM_FLAGS_INTF_XAUI		0x00000020
+#define PHY_BCM_FLAGS_INTF_SGMII	0x00000010
+#define PHY_BCM_FLAGS_MODE_1000BX	0x00000002
+#define PHY_BCM_FLAGS_MODE_COPPER	0x00000001
+
 MODULE_DESCRIPTION("Broadcom PHY driver");
 MODULE_AUTHOR("Maciej W. Rozycki");
 MODULE_LICENSE("GPL");
 
+/*
+ * Indirect register access functions for the 1000BASE-T/100BASE-TX/10BASE-T
+ * 0x1c shadow registers
+ */
+static inline int
+bcm54xx_shadow_read(struct phy_device *phydev, u16 shadow)
+{
+	phy_write(phydev, MII_BCM54XX_SHD, MII_BCM54XX_SHD_VAL(shadow));
+	return MII_BCM54XX_SHD_DATA(phy_read(phydev, MII_BCM54XX_SHD));
+}
+
+static inline int
+bcm54xx_shadow_write(struct phy_device *phydev, u16 shadow, u16 val)
+{
+	return phy_write(phydev, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE
+		| MII_BCM54XX_SHD_VAL(shadow)
+		| MII_BCM54XX_SHD_DATA(val));
+}
+
+/*
+ * Indirect register access functions for the Expansion Registers
+ */
+static inline int
+bcm54xx_exp_read(struct phy_device *phydev, int sec_serdes, u8 regnum)
+{
+	int val;
+
+	phy_write(phydev, MII_BCM54XX_EXP,
+			(sec_serdes ? MII_BCM54XX_EXP_SSD : MII_BCM54XX_EXP_ER)
+			| regnum);
+	val = phy_read(phydev, MII_BCM54XX_RSV0);
+	phy_write(phydev, MII_BCM54XX_EXP, MII_BCM54XX_EXP_NONE | regnum);
+
+	return val;
+}
+
+static inline int
+bcm54xx_exp_write(struct phy_device *phydev, int sec_serdes, u8 regnum, u16 val)
+{
+	int ret;
+
+	phy_write(phydev, MII_BCM54XX_EXP,
+			(sec_serdes ? MII_BCM54XX_EXP_SSD : MII_BCM54XX_EXP_ER)
+			| regnum);
+	ret = phy_write(phydev, MII_BCM54XX_RSV0, val);
+	phy_write(phydev, MII_BCM54XX_EXP, MII_BCM54XX_EXP_NONE | regnum);
+
+	return ret;
+}
+
 static int bcm54xx_config_init(struct phy_device *phydev)
 {
 	int reg, err;
@@ -70,6 +143,82 @@ static int bcm54xx_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int bcm5482_config_init(struct phy_device *phydev)
+{
+	int err;
+
+	err = bcm54xx_config_init(phydev);
+
+	if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX) {
+		/*
+		 * Secondary SerDes control (shadow 10100):
+		 *   Enable secondary SerDes and its use as an LED source
+		 */
+		bcm54xx_shadow_write(phydev, 0x14,
+			bcm54xx_shadow_read(phydev, 0x14) | 0x9);
+
+		/*
+		 * Secondary SerDes SGMII Slave (0x15):
+		 *   Enable SGMII slave mode and auto-detection
+		 */
+		bcm54xx_exp_write(phydev, 1, 0x15,
+			bcm54xx_exp_read(phydev, 1, 0x15) | 0x3);
+
+		/*
+		 * Secondary SerDes MII control (0x00):
+		 *   Disable secondary SerDes powerdown
+		 */
+		bcm54xx_exp_write(phydev, 1, 0x00,
+			bcm54xx_exp_read(phydev, 1, 0x00) & ~0x0800);
+
+		/*
+		 * Mode Control register (shadow 11111):
+		 *   Select 1000BASE-X register set (primary SerDes)
+		 */
+		bcm54xx_shadow_write(phydev, 0x1f,
+			bcm54xx_shadow_read(phydev, 0x1f) | 0x1);
+
+		/*
+		 * LED selector 1 register (shadow 01101):
+		 *   LED1=ACTIVITYLED, LED3=LINKSPD[2]
+		 *   (Use LED1 as secondary SerDes ACTIVITY LED)
+		 */
+		bcm54xx_shadow_write(phydev, 0x0d, 0x013);
+
+		/*
+		 * Auto-negotiation doesn't seem to work quite right
+		 * in this mode, so we disable it and force it to the
+		 * right speed/duplex setting.  Only 'link status'
+		 * is important.
+		 */
+		phydev->autoneg = AUTONEG_DISABLE;
+		phydev->speed = SPEED_1000;
+		phydev->duplex = DUPLEX_FULL;
+	}
+
+	return err;
+}
+
+static int bcm5482_read_status(struct phy_device *phydev)
+{
+	int err;
+
+	err = genphy_read_status(phydev);
+
+	if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX) {
+		/*
+		 * Only link status matters for 1000Base-X mode, so force
+		 * 1000 Mbit/s full-duplex status
+		 */
+		if (phydev->link) {
+			phydev->speed = SPEED_1000;
+			phydev->duplex = DUPLEX_FULL;
+		}
+	}
+
+	return err;
+}
+
 static int bcm54xx_ack_interrupt(struct phy_device *phydev)
 {
 	int reg;
@@ -210,9 +359,9 @@ static struct phy_driver bcm5482_driver = {
 	.name		= "Broadcom BCM5482",
 	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
-	.config_init	= bcm54xx_config_init,
+	.config_init	= bcm5482_config_init,
 	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
+	.read_status	= bcm5482_read_status,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver 	= { .owner = THIS_MODULE },
-- 
1.5.4.4

Re: [PATCH] PHYLIB: Add 1000Base-X support for Broadcom bcm5482

[Maciej W. Rozycki]

Hi Nate,

> Configure the BCM5482S secondary SerDes for 1000Base-X mode when the
> appropriate dev_flags are passed in to phy_connect().  This is
> needed when the PHY is used for fiber and backplane connections.
> 
> Signed-off-by: Nate Case <ncase@xes-inc.com>
> ---

 Thanks for your changes -- they do not seem to break my system, but I
have no way to verify your additions at the run time as I only have
BCM5421 PHYs to try with.  A few comments follow.

> Note: This contains a few "magic" numbers/bits which are documented
> in the comments.  I neglected making #defines for all of these to
> keep the change size down, but I'm willing to make them if people want
> it enough.  Most are probably 5482 specific.

 Please do provide these macros!  The size does not matter (duh, what a
truism!).  What really matters is the meaning of the numbers.  What seems
obvious now, may not be such a couple of changes into the future.  
Besides, it is much more effective to grep, etc. for macros than for
numbers.

 We have magic numbers elsewhere, true, but they are results of
reverse-engineering, so the exact meaning is unknown.  Of course if you
have a possibility to document them properly, you are welcome to do so.
But for everything that's known, let's keep the code as it should be from
the beginning.

> +#define MII_BCM54XX_RSV0	0x15	/* Reserved / Expansion registers */

 Hmm, the name seems confusing -- is it the manufacturer's official name
of the register?  Based on the semantics I would call it
MII_BCM54XX_EXP_DATA or suchlike.

> +#define MII_BCM54XX_EXP		0x17	/* Expansion register access */

 And then perhaps MII_BCM54XX_EXP_SEL for consistency.

> +	return phy_write(phydev, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE
> +		| MII_BCM54XX_SHD_VAL(shadow)
> +		| MII_BCM54XX_SHD_DATA(val));

 Formatting, please -- Linux wants operators at the end of the line and 
indentation to mark where the continuation correlates to the line above.  
In this case either:

	return phy_write(phydev, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE |
						  MII_BCM54XX_SHD_VAL(shadow) |
						  MII_BCM54XX_SHD_DATA(val));

or:

	return phy_write(phydev, MII_BCM54XX_SHD,
			 MII_BCM54XX_SHD_WRITE | MII_BCM54XX_SHD_VAL(shadow) |
			 MII_BCM54XX_SHD_DATA(val));

I have a preference within the two and there are other acceptable
variations, but I won't influence you. ;-)

> +	phy_write(phydev, MII_BCM54XX_EXP,
> +			(sec_serdes ? MII_BCM54XX_EXP_SSD : MII_BCM54XX_EXP_ER)
> +			| regnum);

 Likewise:

	phy_write(phydev, MII_BCM54XX_EXP, (sec_serdes ? MII_BCM54XX_EXP_SSD :
						         MII_BCM54XX_EXP_ER) |
					   regnum);

or:

	phy_write(phydev, MII_BCM54XX_EXP,
		  (sec_serdes ? MII_BCM54XX_EXP_SSD : MII_BCM54XX_EXP_ER) |
		  regnum);


> +static inline int
> +bcm54xx_exp_write(struct phy_device *phydev, int sec_serdes, u8 regnum, u16 val)

 Hmm, you have reached the 80th column -- that causes problems sometimes,
I would wrap the arguments.

> +		bcm54xx_shadow_write(phydev, 0x14,
> +			bcm54xx_shadow_read(phydev, 0x14) | 0x9);

 Similarly:

		bcm54xx_shadow_write(phydev, 0x14,
				     bcm54xx_shadow_read(phydev, 0x14) | 0x9);

I think these all fit, but:

		reg = bcm54xx_shadow_read(phydev, 0x14);
		bcm54xx_shadow_write(phydev, 0x14, reg | 0x9);

would be good otherwise.

> +	if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX) {
> +		/*
> +		 * Only link status matters for 1000Base-X mode, so force
> +		 * 1000 Mbit/s full-duplex status
> +		 */
> +		if (phydev->link) {
> +			phydev->speed = SPEED_1000;
> +			phydev->duplex = DUPLEX_FULL;
> +		}
> +	}

 Hmm, out of curiosity -- does such enforcement play well with tools like 
ethtool or mii-tool?

  Maciej

Re: [PATCH] PHYLIB: Add 1000Base-X support for Broadcom bcm5482

[Nate Case]

On Wed, 2008-05-14 at 01:02 +0100, Maciej W. Rozycki wrote:
>  Thanks for your changes -- they do not seem to break my system, but I
> have no way to verify your additions at the run time as I only have
> BCM5421 PHYs to try with.  A few comments follow.

I didn't mention this before, but I did test these myself at least on a
board with a BCM5482S in both SGMII->Copper and 1000Base-X modes.

>  Please do provide these macros!  The size does not matter (duh, what a
> truism!).  What really matters is the meaning of the numbers.  What seems
> obvious now, may not be such a couple of changes into the future.  
> Besides, it is much more effective to grep, etc. for macros than for
> numbers.

I went ahead and revised my patch to include these.  I certainly don't
disagree with your reasoning in general.  Laziness is a big factor and
it felt faster/easier to get things merged if the change had a smaller
footprint :)

>  We have magic numbers elsewhere, true, but they are results of
> reverse-engineering, so the exact meaning is unknown.  Of course if you
> have a possibility to document them properly, you are welcome to do so.
> But for everything that's known, let's keep the code as it should be from
> the beginning.

FYI: My guess for the 5481 magic bits is that they are accessing shadow
values at 0x18 similar to the way my 5481 patch uses the 0x1c shadow
registers.  It looks the same as the 5482's "Misc Control
Register" (shadow value "111", which is where the 0x7 comes from).

> > +#define MII_BCM54XX_RSV0	0x15	/* Reserved / Expansion registers */
> 
>  Hmm, the name seems confusing -- is it the manufacturer's official name
> of the register?  Based on the semantics I would call it
> MII_BCM54XX_EXP_DATA or suchlike.

I'm not aware of any "official" manufacturer name for this register.
The register map has 0x15 as "reserved", but 0x15 is used for accessing
expansion registers.  I agree with your interpretation of how it's used
so I renamed it as you did in v2 of my patch.

> 
> > +#define MII_BCM54XX_EXP		0x17	/* Expansion register access */
> 
>  And then perhaps MII_BCM54XX_EXP_SEL for consistency.

Done

>  Formatting, please -- Linux wants operators at the end of the line and 
> indentation to mark where the continuation correlates to the line above.  
> In this case either:
[snip]

Done (this makes me wonder why Lindent doesn't pass "-nbbo" to indent
though).

>  Hmm, you have reached the 80th column -- that causes problems sometimes,
> I would wrap the arguments.

Done

>  Hmm, out of curiosity -- does such enforcement play well with tools like 
> ethtool or mii-tool?

I did test with 'ethtool' and link detection worked fine.  The only
catch is that it will show 1000 mbit/s even when there is no link (many
drivers report 10 mbit/s with no link present), but I wouldn't think we
should ever trust 'speed' without a link present.

v2 of the patch will follow shortly.

-- 
Nate Case <ncase@xes-inc.com>

Re: [PATCH] PHYLIB: Add 1000Base-X support for Broadcom bcm5482

[Maciej W. Rozycki]

On Wed, 14 May 2008, Nate Case wrote:

> I didn't mention this before, but I did test these myself at least on a
> board with a BCM5482S in both SGMII->Copper and 1000Base-X modes.

 I have assumed it, but wanted to point out to the others the only
additional testing I can do is to check whether it breaks support or not
for one of the other PHY chips handled by the driver.

> I went ahead and revised my patch to include these.  I certainly don't
> disagree with your reasoning in general.  Laziness is a big factor and
> it felt faster/easier to get things merged if the change had a smaller
> footprint :)

 Laziness is a virtue -- the main incentive for the humankind to make
progress after all -- but only if it does not make you work harder later
on. :-)

> FYI: My guess for the 5481 magic bits is that they are accessing shadow
> values at 0x18 similar to the way my 5481 patch uses the 0x1c shadow
> registers.  It looks the same as the 5482's "Misc Control
> Register" (shadow value "111", which is where the 0x7 comes from).

 Thanks for the hint -- it looks like your newly introduced
MII_BCM54XX_AUX_CTL macro could be used here.  Designers tend to reuse
building blocks in hardware, so chances are it is indeed the same.

> The register map has 0x15 as "reserved", but 0x15 is used for accessing
> expansion registers.  I agree with your interpretation of how it's used
> so I renamed it as you did in v2 of my patch.

 OK.  One note on this occasion: please keep the registers sorted by the
index.  I missed it with the original review, but the additional registers
at 0x15, 0x17 and 0x18 (and the values within) should be placed between
MII_BCM54XX_ESR at 0x11 and MII_BCM54XX_ISR at 0x1a.

> I did test with 'ethtool' and link detection worked fine.  The only
> catch is that it will show 1000 mbit/s even when there is no link (many
> drivers report 10 mbit/s with no link present), but I wouldn't think we
> should ever trust 'speed' without a link present.

 I was a bit worried how it plays with actually trying to force different,
perhaps incompatible, link parameters.

  Maciej

Re: [PATCH] PHYLIB: Add 1000Base-X support for Broadcom bcm5482

[Nate Case]

On Thu, 2008-05-15 at 00:15 +0100, Maciej W. Rozycki wrote:
>  OK.  One note on this occasion: please keep the registers sorted by the
> index.  I missed it with the original review, but the additional registers
> at 0x15, 0x17 and 0x18 (and the values within) should be placed between
> MII_BCM54XX_ESR at 0x11 and MII_BCM54XX_ISR at 0x1a.

Fixed this in patch v3 (coming up shortly).

>  I was a bit worried how it plays with actually trying to force different,
> perhaps incompatible, link parameters.

I didn't try forcing speeds from userspace, but the worst I can envision
happening is that after forcing 10mbit/s or half-duplex mode (for
example) it would still report 1000mbit/s FD.  The forcing itself would
probably not report an error, though you could argue that it should.  My
understanding is that the hardware will take the writes but ignore those
registers in 1000Base-X mode.

-- 
Nate Case <ncase@xes-inc.com>

Re: [PATCH] PHYLIB: Add 1000Base-X support for Broadcom bcm5482

[Andy Fleming]

On May 15, 2008, at 16:03, Nate Case wrote:

> On Thu, 2008-05-15 at 00:15 +0100, Maciej W. Rozycki wrote:
>> OK.  One note on this occasion: please keep the registers sorted by  
>> the
>> index.  I missed it with the original review, but the additional  
>> registers
>> at 0x15, 0x17 and 0x18 (and the values within) should be placed  
>> between
>> MII_BCM54XX_ESR at 0x11 and MII_BCM54XX_ISR at 0x1a.
>
> Fixed this in patch v3 (coming up shortly).
>
>> I was a bit worried how it plays with actually trying to force  
>> different,
>> perhaps incompatible, link parameters.
>
> I didn't try forcing speeds from userspace, but the worst I can  
> envision
> happening is that after forcing 10mbit/s or half-duplex mode (for
> example) it would still report 1000mbit/s FD.  The forcing itself  
> would
> probably not report an error, though you could argue that it  
> should.  My
> understanding is that the hardware will take the writes but ignore  
> those
> registers in 1000Base-X mode.

To get an error in these situations, you could change the phydev's  
supported modes to indicate only 1000BT is supported, and that  
autonegotiation has to be off.

Andy

Re: [PATCH] PHYLIB: Add 1000Base-X support for Broadcom bcm5482

[Maciej W. Rozycki]

On Thu, 15 May 2008, Nate Case wrote:

> Fixed this in patch v3 (coming up shortly).

 This one works for me as well.  However I have noticed a couple of
unnecessary "inline" annotations.  I think it is best to leave the
decision about inlining up to the compiler -- remember that for Linux
"inline" expands to "inline __attribute__((always_inline))".  I have
updated the patch on your behalf to save you the hassle with another
iteration and will send it separately straight away, together with a few
trivial formatting adjustments.  Let me know if you disagree with any of
these changes.

 While doing this I have checked the relevant Kconfig file and noticed the 
new PHY models are not mentioned -- I will send another patch separately.

> I didn't try forcing speeds from userspace, but the worst I can envision
> happening is that after forcing 10mbit/s or half-duplex mode (for
> example) it would still report 1000mbit/s FD.  The forcing itself would
> probably not report an error, though you could argue that it should.  My
> understanding is that the hardware will take the writes but ignore those
> registers in 1000Base-X mode.

 Please try when you have some opportunity.  Personally I think an error
is not strictly necessary as long as the status read back is correct.

  Maciej