: Check EEPROM before tweaking SERDES

PATCH In review Nick Kossifidis 5 participants 7 messages Jul 31, 2009 – Aug 4, 2009 Atheros ATH5K wireless driver

Share & tools

Nick Kossifidis

author

2009-07-31 18:34 UTC #0 lore raw

PATCH 1/4 : Check EEPROM before tweaking SERDES

* Read PCI-E infos offset from EEPROM and if it points to serdes section (0x40), enable serdes programming (further tweaking of serdes values during attach). This follows Legacy and Sam's HAL sources.

---
 drivers/net/wireless/ath/ath5k/attach.c |   56 +++++++++++++++++++------------
 drivers/net/wireless/ath/ath5k/eeprom.c |   10 +++++
 drivers/net/wireless/ath/ath5k/eeprom.h |    4 ++
 3 files changed, 48 insertions(+), 22 deletions(-)

drivers/net/wireless/ath/ath5k/attach.c +34 ~~-22~~

index 9a84d94..6263065 100644
@@ -253,28 +253,6 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
 	}
 
 	/*
-	 * Write PCI-E power save settings
-	 */
-	if ((ah->ah_version == AR5K_AR5212) && (pdev->is_pcie)) {
-		ath5k_hw_reg_write(ah, 0x9248fc00, AR5K_PCIE_SERDES);
-		ath5k_hw_reg_write(ah, 0x24924924, AR5K_PCIE_SERDES);
-		/* Shut off RX when elecidle is asserted */
-		ath5k_hw_reg_write(ah, 0x28000039, AR5K_PCIE_SERDES);
-		ath5k_hw_reg_write(ah, 0x53160824, AR5K_PCIE_SERDES);
-		/* TODO: EEPROM work */
-		ath5k_hw_reg_write(ah, 0xe5980579, AR5K_PCIE_SERDES);
-		/* Shut off PLL and CLKREQ active in L1 */
-		ath5k_hw_reg_write(ah, 0x001defff, AR5K_PCIE_SERDES);
-		/* Preserce other settings */
-		ath5k_hw_reg_write(ah, 0x1aaabe40, AR5K_PCIE_SERDES);
-		ath5k_hw_reg_write(ah, 0xbe105554, AR5K_PCIE_SERDES);
-		ath5k_hw_reg_write(ah, 0x000e3007, AR5K_PCIE_SERDES);
-		/* Reset SERDES to load new settings */
-		ath5k_hw_reg_write(ah, 0x00000000, AR5K_PCIE_SERDES_RESET);
-		mdelay(1);
-	}
-
-	/*
 	 * POST
 	 */
 	ret = ath5k_hw_post(ah);

@@ -295,6 +273,40 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
 		goto err_free;
 	}
 
+	/*
+	 * Write PCI-E power save settings
+	 */
+	if ((ah->ah_version == AR5K_AR5212) && (pdev->is_pcie)) {
+		struct ath5k_eeprom_info *ee = &ah->ah_capabilities.cap_eeprom;
+
+		ath5k_hw_reg_write(ah, 0x9248fc00, AR5K_PCIE_SERDES);
+		ath5k_hw_reg_write(ah, 0x24924924, AR5K_PCIE_SERDES);
+
+		/* Shut off RX when elecidle is asserted */
+		ath5k_hw_reg_write(ah, 0x28000039, AR5K_PCIE_SERDES);
+		ath5k_hw_reg_write(ah, 0x53160824, AR5K_PCIE_SERDES);
+
+		/* If serdes programing is enabled, increase PCI-E
+		 * tx power for systems with long trace from host
+		 * to minicard connector. */
+		if (ee->ee_serdes)
+			ath5k_hw_reg_write(ah, 0xe5980579, AR5K_PCIE_SERDES);
+		else
+			ath5k_hw_reg_write(ah, 0xf6800579, AR5K_PCIE_SERDES);
+
+		/* Shut off PLL and CLKREQ active in L1 */
+		ath5k_hw_reg_write(ah, 0x001defff, AR5K_PCIE_SERDES);
+
+		/* Preserve other settings */
+		ath5k_hw_reg_write(ah, 0x1aaabe40, AR5K_PCIE_SERDES);
+		ath5k_hw_reg_write(ah, 0xbe105554, AR5K_PCIE_SERDES);
+		ath5k_hw_reg_write(ah, 0x000e3007, AR5K_PCIE_SERDES);
+
+		/* Reset SERDES to load new settings */
+		ath5k_hw_reg_write(ah, 0x00000000, AR5K_PCIE_SERDES_RESET);
+		mdelay(1);
+	}
+
 	/* Get misc capabilities */
 	ret = ath5k_hw_set_capabilities(ah);
 	if (ret) {

drivers/net/wireless/ath/ath5k/eeprom.c +10 -0

index c56b494..8af477d 100644
@@ -167,6 +167,16 @@ ath5k_eeprom_init_header(struct ath5k_hw *ah)
 	ee->ee_rfkill_pin = (u8) AR5K_REG_MS(val, AR5K_EEPROM_RFKILL_GPIO_SEL);
 	ee->ee_rfkill_pol = val & AR5K_EEPROM_RFKILL_POLARITY ? true : false;
 
+	/* Check if PCIE_OFFSET points to PCIE_SERDES_SECTION
+	 * and enable serdes programming if needed.
+	 *
+	 * XXX: Serdes values seem to be fixed so
+	 * no need to read them here, we write them
+	 * during ath5k_hw_attach */
+	AR5K_EEPROM_READ(AR5K_EEPROM_PCIE_OFFSET, val);
+	ee->ee_serdes = (val == AR5K_EEPROM_PCIE_SERDES_SECTION) ?
+							true : false;
+
 	return 0;
 }

drivers/net/wireless/ath/ath5k/eeprom.h +4 -0

index 64be73a..0123f35 100644
@@ -19,6 +19,9 @@
 /*
  * Common ar5xxx EEPROM data offsets (set these on AR5K_EEPROM_BASE)
  */
+#define	AR5K_EEPROM_PCIE_OFFSET		0x02	/* Contains offset to PCI-E infos */
+#define	AR5K_EEPROM_PCIE_SERDES_SECTION	0x40	/* PCIE_OFFSET points here when
+						 * SERDES infos are present */
 #define AR5K_EEPROM_MAGIC		0x003d	/* EEPROM Magic number */
 #define AR5K_EEPROM_MAGIC_VALUE		0x5aa5	/* Default - found on EEPROM */
 #define AR5K_EEPROM_MAGIC_5212		0x0000145c /* 5212 */

@@ -391,6 +394,7 @@ struct ath5k_eeprom_info {
 	u8	ee_rfkill_pin;
 	bool	ee_rfkill_pol;
 	bool	ee_is_hb63;
+	bool	ee_serdes;
 	u16	ee_misc0;
 	u16	ee_misc1;
 	u16	ee_misc2;

Luis R. Rodriguez

2009-07-31 18:11 UTC #1 lore raw

Quoted 34 lines · Nick Kossifidis

---

 drivers/net/wireless/ath/ath5k/attach.c |   56 +++++++++++++++++++------------
 drivers/net/wireless/ath/ath5k/eeprom.c |   10 +++++
 drivers/net/wireless/ath/ath5k/eeprom.h |    4 ++
 3 files changed, 48 insertions(+), 22 deletions(-)

drivers/net/wireless/ath/ath5k/attach.c +0 ~~-15~~

index 9a84d94..6263065 100644
@@ -253,28 +253,6 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
        }
 
        /*
-        * Write PCI-E power save settings
-        */
-       if ((ah->ah_version == AR5K_AR5212) && (pdev->is_pcie)) {
-               ath5k_hw_reg_write(ah, 0x9248fc00, AR5K_PCIE_SERDES);
-               ath5k_hw_reg_write(ah, 0x24924924, AR5K_PCIE_SERDES);
-               /* Shut off RX when elecidle is asserted */
-               ath5k_hw_reg_write(ah, 0x28000039, AR5K_PCIE_SERDES);
-               ath5k_hw_reg_write(ah, 0x53160824, AR5K_PCIE_SERDES);
-               /* TODO: EEPROM work */
-               ath5k_hw_reg_write(ah, 0xe5980579, AR5K_PCIE_SERDES);
-               /* Shut off PLL and CLKREQ active in L1 */
-               ath5k_hw_reg_write(ah, 0x001defff, AR5K_PCIE_SERDES);
-               /* Preserce other settings */
-               ath5k_hw_reg_write(ah, 0x1aaabe40, AR5K_PCIE_SERDES);
-               ath5k_hw_reg_write(ah, 0xbe105554, AR5K_PCIE_SERDES);

drivers/net/wireless/ath/ath5k/attach.c struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version) · line 258

Quoted 10 lines

drivers/net/wireless/ath/ath5k/attach.c +0 -7

-               ath5k_hw_reg_write(ah, 0x000e3007, AR5K_PCIE_SERDES);
-               /* Reset SERDES to load new settings */
-               ath5k_hw_reg_write(ah, 0x00000000, AR5K_PCIE_SERDES_RESET);
-               mdelay(1);
-       }
-
-       /*
         * POST
         */
        ret = ath5k_hw_post(ah);

Can the changes for the code be done before the move, or after so that way the actual code changes to the section can be easily readable?

Quoted 10 lines

drivers/net/wireless/ath/ath5k/attach.c +6 -0

@@ -295,6 +273,40 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
                goto err_free;
        }
 
+       /*
+        * Write PCI-E power save settings
+        */
+       if ((ah->ah_version == AR5K_AR5212) && (pdev->is_pcie)) {
+               struct ath5k_eeprom_info *ee = &ah->ah_capabilities.cap_eeprom;
+

drivers/net/wireless/ath/ath5k/attach.c struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version) · line 291

Quoted 10 lines

drivers/net/wireless/ath/ath5k/attach.c +10 -0

+               ath5k_hw_reg_write(ah, 0x9248fc00, AR5K_PCIE_SERDES);
+               ath5k_hw_reg_write(ah, 0x24924924, AR5K_PCIE_SERDES);
+
+               /* Shut off RX when elecidle is asserted */
+               ath5k_hw_reg_write(ah, 0x28000039, AR5K_PCIE_SERDES);
+               ath5k_hw_reg_write(ah, 0x53160824, AR5K_PCIE_SERDES);
+
+               /* If serdes programing is enabled, increase PCI-E
+                * tx power for systems with long trace from host
+                * to minicard connector. */

This is just comment-picky mind using this style for comments for > 1 lines?

/*
* foo bleh buh
* bar
* qwerty
*/

Luis

Nick Kossifidis

maintainer

2009-07-31 18:18 UTC #2 lore raw

2009/7/31 Luis R. Rodriguez <[email protected]>:

Quoted 4 lines

Can the changes for the code be done before the move, or after so that way the actual code changes to the section can be easily readable?

Since we use EEPROM infos we must move this after eeprom initialization.

Quoted 10 lines

This is just comment-picky mind using this style for comments for > 1 lines?
/*
* foo bleh buh
* bar
* qwerty
*/
Luis

I use it mostly on large functions (such as attach/reset) to mark sections, we don't execute a single command here but a bunch of reg writes. I find it easier to read this way.

Signature · 3 lines

GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Luis R. Rodriguez

2009-07-31 18:20 UTC #3 lore raw

Quoted 7 lines · Nick Kossifidis

2009/7/31 Luis R. Rodriguez <[email protected]>:
Quoted 4 lines
Can the changes for the code be done before the move, or after so that way the actual code changes to the section can be easily readable?
Since we use EEPROM infos we must move this after eeprom initialization.

Yes, but my point is a move can be done in one patch, changes to that code in a separate patch, to help understand what is going on during patch review.

Luis

Nick Kossifidis

maintainer

2009-07-31 18:27 UTC #4 lore raw

2009/7/31 Luis R. Rodriguez <[email protected]>:

Quoted 7 lines · Nick Kossifidis

Quoted 6 lines
2009/7/31 Luis R. Rodriguez <[email protected]>:
Quoted 4 lines
Can the changes for the code be done before the move, or after so that way the actual code changes to the section can be easily readable?

Quoted 8 lines

Quoted 1 line
Since we use EEPROM infos we must move this after eeprom initialization.
Yes, but my point is a move can be done in one patch, changes to that code in a separate patch, to help understand what is going on during patch review.
Luis

I think this will pollute the logs, it's a very simple change and commented.

Signature · 3 lines

GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Kalle Valo

2009-08-01 18:37 UTC #5 lore raw

Quoted 5 lines · Nick Kossifidis

Quoted 3 lines
Yes, but my point is a move can be done in one patch, changes to that code in a separate patch, to help understand what is going on during patch review.
I think this will pollute the logs, it's a very simple change and commented.

I'm with Luis here. It makes a lot easier to read patches and find bugs if the patches do one thing at a time. Don't worry about the number of patches, usually that's not a problem.

Signature · 1 line

Kalle Valo

Bob Copeland

Acked-by on v1 1/4

2009-08-04 15:18 UTC #6 lore raw

In reply to Nick Kossifidis

Quoted 9 lines · Nick Kossifidis

+ /* Check if PCIE_OFFSET points to PCIE_SERDES_SECTION
+ * and enable serdes programming if needed.
+ *
+ * XXX: Serdes values seem to be fixed so
+ * no need to read them here, we write them
+ * during ath5k_hw_attach */
+ AR5K_EEPROM_READ(AR5K_EEPROM_PCIE_OFFSET, val);
+ ee->ee_serdes = (val == AR5K_EEPROM_PCIE_SERDES_SECTION) ?
+ true : false;

Minor nit, can just be:

Quoted 1 line

+ ee->ee_serdes = (val == AR5K_EEPROM_PCIE_SERDES_SECTION);

Just a matter of taste though, so:

Acked-by Bob Copeland <[email protected]>

Signature · 1 line

Bob Copeland %% www.bobcopeland.com