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 ...
[ message continues ]

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

 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

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


 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

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

 Hmm, you have reached the 80th column -- that causes problems sometimes,

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


 Hmm, out of curiosity -- does such enforcement play well with tools like ...
[ message continues ]

I didn't mention this before, but I did test these myself at least on a

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

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

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

[snip]

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


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>

--

[ message continues ]

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

 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

 Thanks for the hint -- it looks like your newly introduced
MII_BCM54XX_AUX_CTL macro could be used here.  Designers tend to reuse

 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

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

  Maciej
--

[ message continues ]

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>

--

[ message continues ]

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

[ message continues ]

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 

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

[ message continues ]