Date: Tue, 17 Apr 2012 18:23:06 -0700 From: Adrian Chadd <adrian@freebsd.org> To: freebsd-wireless@freebsd.org Subject: [RFC] disable hardware register byteswap, fixes Merlin AR9220 on AR71xx Message-ID: <CAJ-VmokLuTCjJ_on9-Ejw7mEAmMhk075hqG4CnyFgtKaFLOFaA@mail.gmail.com>
index | next in thread | raw e-mail
[-- Attachment #1 --]
Hi all,
After quite a bit of hackery, hair tearing and other assorted stuff, I
finally nailed the AR9220 + AR71xx instability that we've all been
seeing.
It seems to be a combination of:
* incorrect power supplies being used (yes, I found that a board would
work fine until you started interacting with the NIC, at which point
it'd throw an immediate parity error - but then subsequent register
poking via the debugger would come out fine);
* _maybe_ some PCI bus glue issues? The PCI glue now looks a lot more
like what's in Linux/Atheros code.
* hardware register content byteswapping.
The last one is a bit unfortunate. Here's a patch that pushes the
register space byte swapping back into software, rather than hardware.
I've given it a good thrashing on my AR71xx boards with various AR9220
NICs and they all work a-ok now, with no panics and no "weird stuff."
I have no idea (yet) whether it's a timing issue with the PCI
NIC/bus/controller, whether the hardware swizzling is just plain
broken in some situations .. but what I don't want to do is turn all
of this off without understanding what the root cause is.
So if you've been dying (heh) to play with FreeBSD-HEAD on the
DIR-825, or any other atheros MIPS + AR9220 board, here's your chance.
I've had it rock solid on my AP96 and Routerstation / Routerstation
pro boards for the last couple of days. The DIR-825 is based on the
AP96 design (different switch PHY) so I would be very surprised to
find it unstable.
I may soon just give in and commit it, with a long explanation as to
why I'm disabling it. If people would like it resurrected for whatever
reason then the patch(es) will be in SVN history.
As a data point - ath5k, ath9k and the Atheros reference driver all
have the hardware register byteswap disabled. I'd rather not disable
it without having firm evidence it's broken. I may also tidy up the
register access stuff and make it all a compile time option.
Thanks,
Adrian
[-- Attachment #2 --]
Index: sys/dev/ath/ath_hal/ar5416/ar5416_reset.c
===================================================================
--- sys/dev/ath/ath_hal/ar5416/ar5416_reset.c (revision 234369)
+++ sys/dev/ath/ath_hal/ar5416/ar5416_reset.c (working copy)
@@ -1387,16 +1387,15 @@
if (type == HAL_RESET_COLD) {
if (isBigEndian()) {
/*
- * Set CFG, little-endian for register
- * and descriptor accesses.
+ * Set CFG, little-endian for descriptor accesses.
*/
- mask = INIT_CONFIG_STATUS | AR_CFG_SWRD | AR_CFG_SWRG;
+ mask = INIT_CONFIG_STATUS | AR_CFG_SWRD;
#ifndef AH_NEED_DESC_SWAP
mask |= AR_CFG_SWTD;
#endif
HALDEBUG(ah, HAL_DEBUG_RESET,
"%s Applying descriptor swap\n", __func__);
- OS_REG_WRITE(ah, AR_CFG, LE_READ_4(&mask));
+ OS_REG_WRITE(ah, AR_CFG, mask);
} else
OS_REG_WRITE(ah, AR_CFG, INIT_CONFIG_STATUS);
}
Index: sys/dev/ath/ath_hal/ar5210/ar5210_reset.c
===================================================================
--- sys/dev/ath/ath_hal/ar5210/ar5210_reset.c (revision 234369)
+++ sys/dev/ath/ath_hal/ar5210/ar5210_reset.c (working copy)
@@ -594,12 +594,10 @@
if ((resetMask & AR_RC_RMAC) == 0) {
if (isBigEndian()) {
/*
- * Set CFG, little-endian for register
- * and descriptor accesses.
+ * Set CFG, little-endian for descriptor accesses.
*/
- mask = INIT_CONFIG_STATUS |
- AR_CFG_SWTD | AR_CFG_SWRD | AR_CFG_SWRG;
- OS_REG_WRITE(ah, AR_CFG, LE_READ_4(&mask));
+ mask = INIT_CONFIG_STATUS | AR_CFG_SWTD | AR_CFG_SWRD;
+ OS_REG_WRITE(ah, AR_CFG, mask);
} else
OS_REG_WRITE(ah, AR_CFG, INIT_CONFIG_STATUS);
}
Index: sys/dev/ath/ath_hal/ar5211/ar5211_reset.c
===================================================================
--- sys/dev/ath/ath_hal/ar5211/ar5211_reset.c (revision 234369)
+++ sys/dev/ath/ath_hal/ar5211/ar5211_reset.c (working copy)
@@ -764,12 +764,10 @@
if ((resetMask & AR_RC_MAC) == 0) {
if (isBigEndian()) {
/*
- * Set CFG, little-endian for register
- * and descriptor accesses.
+ * Set CFG, little-endian for descriptor accesses.
*/
- mask = INIT_CONFIG_STATUS |
- AR_CFG_SWTD | AR_CFG_SWRD | AR_CFG_SWRG;
- OS_REG_WRITE(ah, AR_CFG, LE_READ_4(&mask));
+ mask = INIT_CONFIG_STATUS | AR_CFG_SWTD | AR_CFG_SWRD;
+ OS_REG_WRITE(ah, AR_CFG, mask);
} else
OS_REG_WRITE(ah, AR_CFG, INIT_CONFIG_STATUS);
}
Index: sys/dev/ath/ath_hal/ar5212/ar5212_reset.c
===================================================================
--- sys/dev/ath/ath_hal/ar5212/ar5212_reset.c (revision 234369)
+++ sys/dev/ath/ath_hal/ar5212/ar5212_reset.c (working copy)
@@ -1273,14 +1273,13 @@
if ((resetMask & AR_RC_MAC) == 0) {
if (isBigEndian()) {
/*
- * Set CFG, little-endian for register
- * and descriptor accesses.
+ * Set CFG, little-endian for descriptor accesses.
*/
- mask = INIT_CONFIG_STATUS | AR_CFG_SWRD | AR_CFG_SWRG;
+ mask = INIT_CONFIG_STATUS | AR_CFG_SWRD;
#ifndef AH_NEED_DESC_SWAP
mask |= AR_CFG_SWTD;
#endif
- OS_REG_WRITE(ah, AR_CFG, LE_READ_4(&mask));
+ OS_REG_WRITE(ah, AR_CFG, mask);
} else
OS_REG_WRITE(ah, AR_CFG, INIT_CONFIG_STATUS);
if (ar5212SetPowerMode(ah, HAL_PM_AWAKE, AH_TRUE))
Index: sys/dev/ath/ath_hal/ar5312/ar5312_reset.c
===================================================================
--- sys/dev/ath/ath_hal/ar5312/ar5312_reset.c (revision 234369)
+++ sys/dev/ath/ath_hal/ar5312/ar5312_reset.c (working copy)
@@ -740,8 +740,7 @@
if ((resetMask & AR_RC_MAC) == 0) {
if (isBigEndian()) {
/*
- * Set CFG, little-endian for register
- * and descriptor accesses.
+ * Set CFG, little-endian for descriptor accesses.
*/
#ifdef AH_NEED_DESC_SWAP
mask = INIT_CONFIG_STATUS | AR_CFG_SWRD;
Index: sys/dev/ath/ah_osdep.c
===================================================================
--- sys/dev/ath/ah_osdep.c (revision 234369)
+++ sys/dev/ath/ah_osdep.c (working copy)
@@ -267,12 +267,7 @@
}
if (ah->ah_config.ah_serialise_reg_war)
mtx_lock_spin(&ah_regser_mtx);
-#if _BYTE_ORDER == _BIG_ENDIAN
- if (OS_REG_UNSWAPPED(reg))
- bus_space_write_4(tag, h, reg, val);
- else
-#endif
- bus_space_write_stream_4(tag, h, reg, val);
+ bus_space_write_4(tag, h, reg, val);
if (ah->ah_config.ah_serialise_reg_war)
mtx_unlock_spin(&ah_regser_mtx);
}
@@ -286,12 +281,11 @@
if (ah->ah_config.ah_serialise_reg_war)
mtx_lock_spin(&ah_regser_mtx);
-#if _BYTE_ORDER == _BIG_ENDIAN
- if (OS_REG_UNSWAPPED(reg))
- val = bus_space_read_4(tag, h, reg);
- else
-#endif
- val = bus_space_read_stream_4(tag, h, reg);
+ val = bus_space_read_4(tag, h, reg);
+
+ if (val == 0xdeadc0de)
+ ath_hal_printf(ah, "%s: reg=0x%x, val=0xdeadc0de!\n", __func__, reg);
+
if (ah->ah_config.ah_serialise_reg_war)
mtx_unlock_spin(&ah_regser_mtx);
if (ath_hal_alq) {
@@ -343,12 +337,7 @@
if (ah->ah_config.ah_serialise_reg_war)
mtx_lock_spin(&ah_regser_mtx);
-#if _BYTE_ORDER == _BIG_ENDIAN
- if (OS_REG_UNSWAPPED(reg))
- bus_space_write_4(tag, h, reg, val);
- else
-#endif
- bus_space_write_stream_4(tag, h, reg, val);
+ bus_space_write_4(tag, h, reg, val);
if (ah->ah_config.ah_serialise_reg_war)
mtx_unlock_spin(&ah_regser_mtx);
}
@@ -362,12 +351,7 @@
if (ah->ah_config.ah_serialise_reg_war)
mtx_lock_spin(&ah_regser_mtx);
-#if _BYTE_ORDER == _BIG_ENDIAN
- if (OS_REG_UNSWAPPED(reg))
- val = bus_space_read_4(tag, h, reg);
- else
-#endif
- val = bus_space_read_stream_4(tag, h, reg);
+ val = bus_space_read_4(tag, h, reg);
if (ah->ah_config.ah_serialise_reg_war)
mtx_unlock_spin(&ah_regser_mtx);
return val;
Index: sys/dev/ath/ah_osdep.h
===================================================================
--- sys/dev/ath/ah_osdep.h (revision 234369)
+++ sys/dev/ath/ah_osdep.h (working copy)
@@ -97,39 +97,13 @@
extern void ath_hal_reg_write(struct ath_hal *ah, u_int reg, u_int32_t val);
extern u_int32_t ath_hal_reg_read(struct ath_hal *ah, u_int reg);
#else
-/*
- * The hardware registers are native little-endian byte order.
- * Big-endian hosts are handled by enabling hardware byte-swap
- * of register reads and writes at reset. But the PCI clock
- * domain registers are not byte swapped! Thus, on big-endian
- * platforms we have to explicitly byte-swap those registers.
- * Most of this code is collapsed at compile time because the
- * register values are constants.
- */
-#if _BYTE_ORDER == _BIG_ENDIAN
-#define OS_REG_WRITE(_ah, _reg, _val) do { \
- if (OS_REG_UNSWAPPED(_reg)) \
- bus_space_write_4((bus_space_tag_t)(_ah)->ah_st, \
- (bus_space_handle_t)(_ah)->ah_sh, (_reg), (_val)); \
- else \
- bus_space_write_stream_4((bus_space_tag_t)(_ah)->ah_st, \
- (bus_space_handle_t)(_ah)->ah_sh, (_reg), (_val)); \
-} while (0)
-#define OS_REG_READ(_ah, _reg) \
- (OS_REG_UNSWAPPED(_reg) ? \
- bus_space_read_4((bus_space_tag_t)(_ah)->ah_st, \
- (bus_space_handle_t)(_ah)->ah_sh, (_reg)) : \
- bus_space_read_stream_4((bus_space_tag_t)(_ah)->ah_st, \
- (bus_space_handle_t)(_ah)->ah_sh, (_reg)))
-#else /* _BYTE_ORDER == _LITTLE_ENDIAN */
#define OS_REG_WRITE(_ah, _reg, _val) \
bus_space_write_4((bus_space_tag_t)(_ah)->ah_st, \
(bus_space_handle_t)(_ah)->ah_sh, (_reg), (_val))
#define OS_REG_READ(_ah, _reg) \
bus_space_read_4((bus_space_tag_t)(_ah)->ah_st, \
(bus_space_handle_t)(_ah)->ah_sh, (_reg))
-#endif /* _BYTE_ORDER */
-#endif /* AH_DEBUG || AH_REGFUNC || AH_DEBUG_ALQ */
+#endif
#ifdef AH_DEBUG_ALQ
extern void OS_MARK(struct ath_hal *, u_int id, u_int32_t value);
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-VmokLuTCjJ_on9-Ejw7mEAmMhk075hqG4CnyFgtKaFLOFaA>