diff mbox

[08/83] staging: brcm80211: replaced #ifdef __mips__ sections by W_REG_FLUSH

Message ID 1306928768-7501-8-git-send-email-rvossen@broadcom.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Roland Vossen June 1, 2011, 11:44 a.m. UTC 
Code cleanup. A read-after-write construct is present in the code to ensure
write order for certain Broadcom chips. Those chips are: bcm4706, bcm4716,
bcm4717, bcm4718. All these chips contain a MIPS processor. This patch gets
rid of several #ifdef __mips__ sections by defining a new macro in a header
file. This patch does not introduce behavioral changes and is purely meant
for code cleanup. The __mips__ define will be made more specific in a future
patch.

Signed-off-by: Roland Vossen <rvossen@broadcom.com>
Reviewed-by: Arend van Spriel <arend@broadcom.com>
---
 .../staging/brcm80211/brcmsmac/phy/wlc_phy_cmn.c   |   63 ++++----------------
 drivers/staging/brcm80211/include/bcmutils.h       |   11 ++++
 2 files changed, 24 insertions(+), 50 deletions(-)

Comments

Jonas Gorski June 1, 2011, 12:27 p.m. UTC | #1 
On 1 June 2011 13:44, Roland Vossen <rvossen@broadcom.com> wrote:
> --- a/drivers/staging/brcm80211/include/bcmutils.h
> +++ b/drivers/staging/brcm80211/include/bcmutils.h
> @@ -366,6 +366,17 @@ extern void bcm_prpkt(const char *msg, struct sk_buff *p0);
>        } while (0)
>  #endif                         /* __BIG_ENDIAN */
>
> +#ifdef __mips__
> +/*
> + * bcm4716 (which includes 4717 & 4718), plus 4706 on PCIe can reorder
> + * transactions. As a fix, a read after write is performed on certain places
> + * in the code. Older chips and the newer 5357 family don't require this fix.
> + */

Checking for CONFIG_BCM47XX instead of __mips__ would be a bit cleaner
and less broad. Although the bcm47x6s are not supported by BCM47XX
yet, when the support gets added they will be caught by this without
affecting other mips platforms.
Perhaps a clean solution would be to introduce a quirk option/value in
the pci_bus or pci_dev the driver can use to decide at runtime whether
it needs this workaround or not. This would also allow other drivers
to check for it if they are also affected by it (even if the
probability of this is rather low - at least I haven't seen a bcm47x6
yet with anything other than a broadcom wifi connected to its pcie
;-).

Jonas
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roland Vossen June 1, 2011, 5:17 p.m. UTC | #2 
Hello Jonas,

> Checking for CONFIG_BCM47XX instead of __mips__ would be a bit cleaner
> and less broad. Although the bcm47x6s are not supported by BCM47XX
> yet, when the support gets added they will be caught by this without
> affecting other mips platforms.
> Perhaps a clean solution would be to introduce a quirk option/value in
> the pci_bus or pci_dev the driver can use to decide at runtime whether
> it needs this workaround or not. This would also allow other drivers
> to check for it if they are also affected by it (even if the
> probability of this is rather low - at least I haven't seen a bcm47x6
> yet with anything other than a broadcom wifi connected to its pcie
> ;-).

Further mips cleanup is in the pipeline, your suggestion is certainly a 
good one. A similar suggestion has been made by GregKH in the past.

However, the focus of the patch set is on code cleanup. So my intention 
is to rewrite code to make it cleaner, without changing existing behavior.

Your (and Gregs) suggestion have been noted down and will get a 
follow-up in the future.

Thanks for the feedback,

Roland.

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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/staging/brcm80211/brcmsmac/phy/wlc_phy_cmn.c b/drivers/staging/brcm80211/brcmsmac/phy/wlc_phy_cmn.c
index f45628a..07a5bcb 100644
--- a/drivers/staging/brcm80211/brcmsmac/phy/wlc_phy_cmn.c
+++ b/drivers/staging/brcm80211/brcmsmac/phy/wlc_phy_cmn.c
@@ -247,16 +247,10 @@  u16 read_radio_reg(phy_info_t *pi, u16 addr)
 	if ((D11REV_GE(pi->sh->corerev, 24)) ||
 	    (D11REV_IS(pi->sh->corerev, 22)
 	     && (pi->pubpi.phy_type != PHY_TYPE_SSN))) {
-		W_REG(&pi->regs->radioregaddr, addr);
-#ifdef __mips__
-		(void)R_REG(&pi->regs->radioregaddr);
-#endif
+		W_REG_FLUSH(&pi->regs->radioregaddr, addr);
 		data = R_REG(&pi->regs->radioregdata);
 	} else {
-		W_REG(&pi->regs->phy4waddr, addr);
-#ifdef __mips__
-		(void)R_REG(&pi->regs->phy4waddr);
-#endif
+		W_REG_FLUSH(&pi->regs->phy4waddr, addr);
 
 #ifdef __ARM_ARCH_4T__
 		__asm__(" .align 4 ");
@@ -281,16 +275,10 @@  void write_radio_reg(phy_info_t *pi, u16 addr, u16 val)
 	    (D11REV_IS(pi->sh->corerev, 22)
 	     && (pi->pubpi.phy_type != PHY_TYPE_SSN))) {
 
-		W_REG(&pi->regs->radioregaddr, addr);
-#ifdef __mips__
-		(void)R_REG(&pi->regs->radioregaddr);
-#endif
+		W_REG_FLUSH(&pi->regs->radioregaddr, addr);
 		W_REG(&pi->regs->radioregdata, val);
 	} else {
-		W_REG(&pi->regs->phy4waddr, addr);
-#ifdef __mips__
-		(void)R_REG(&pi->regs->phy4waddr);
-#endif
+		W_REG_FLUSH(&pi->regs->phy4waddr, addr);
 		W_REG(&pi->regs->phy4wdatalo, val);
 	}
 
@@ -312,29 +300,17 @@  static u32 read_radio_id(phy_info_t *pi)
 	if (D11REV_GE(pi->sh->corerev, 24)) {
 		u32 b0, b1, b2;
 
-		W_REG(&pi->regs->radioregaddr, 0);
-#ifdef __mips__
-		(void)R_REG(&pi->regs->radioregaddr);
-#endif
+		W_REG_FLUSH(&pi->regs->radioregaddr, 0);
 		b0 = (u32) R_REG(&pi->regs->radioregdata);
-		W_REG(&pi->regs->radioregaddr, 1);
-#ifdef __mips__
-		(void)R_REG(&pi->regs->radioregaddr);
-#endif
+		W_REG_FLUSH(&pi->regs->radioregaddr, 1);
 		b1 = (u32) R_REG(&pi->regs->radioregdata);
-		W_REG(&pi->regs->radioregaddr, 2);
-#ifdef __mips__
-		(void)R_REG(&pi->regs->radioregaddr);
-#endif
+		W_REG_FLUSH(&pi->regs->radioregaddr, 2);
 		b2 = (u32) R_REG(&pi->regs->radioregdata);
 
 		id = ((b0 & 0xf) << 28) | (((b2 << 8) | b1) << 12) | ((b0 >> 4)
 								      & 0xf);
 	} else {
-		W_REG(&pi->regs->phy4waddr, RADIO_IDCODE);
-#ifdef __mips__
-		(void)R_REG(&pi->regs->phy4waddr);
-#endif
+		W_REG_FLUSH(&pi->regs->phy4waddr, RADIO_IDCODE);
 		id = (u32) R_REG(&pi->regs->phy4wdatalo);
 		id |= (u32) R_REG(&pi->regs->phy4wdatahi) << 16;
 	}
@@ -397,10 +373,7 @@  u16 read_phy_reg(phy_info_t *pi, u16 addr)
 
 	regs = pi->regs;
 
-	W_REG(&regs->phyregaddr, addr);
-#ifdef __mips__
-	(void)R_REG(&regs->phyregaddr);
-#endif
+	W_REG_FLUSH(&regs->phyregaddr, addr);
 
 	pi->phy_wreg = 0;
 	return R_REG(&regs->phyregdata);
@@ -413,8 +386,7 @@  void write_phy_reg(phy_info_t *pi, u16 addr, u16 val)
 	regs = pi->regs;
 
 #ifdef __mips__
-	W_REG(&regs->phyregaddr, addr);
-	(void)R_REG(&regs->phyregaddr);
+	W_REG_FLUSH(&regs->phyregaddr, addr);
 	W_REG(&regs->phyregdata, val);
 	if (addr == 0x72)
 		(void)R_REG(&regs->phyregdata);
@@ -436,10 +408,7 @@  void and_phy_reg(phy_info_t *pi, u16 addr, u16 val)
 
 	regs = pi->regs;
 
-	W_REG(&regs->phyregaddr, addr);
-#ifdef __mips__
-	(void)R_REG(&regs->phyregaddr);
-#endif
+	W_REG_FLUSH(&regs->phyregaddr, addr);
 
 	W_REG(&regs->phyregdata, (R_REG(&regs->phyregdata) & val));
 	pi->phy_wreg = 0;
@@ -451,10 +420,7 @@  void or_phy_reg(phy_info_t *pi, u16 addr, u16 val)
 
 	regs = pi->regs;
 
-	W_REG(&regs->phyregaddr, addr);
-#ifdef __mips__
-	(void)R_REG(&regs->phyregaddr);
-#endif
+	W_REG_FLUSH(&regs->phyregaddr, addr);
 
 	W_REG(&regs->phyregdata, (R_REG(&regs->phyregdata) | val));
 	pi->phy_wreg = 0;
@@ -466,10 +432,7 @@  void mod_phy_reg(phy_info_t *pi, u16 addr, u16 mask, u16 val)
 
 	regs = pi->regs;
 
-	W_REG(&regs->phyregaddr, addr);
-#ifdef __mips__
-	(void)R_REG(&regs->phyregaddr);
-#endif
+	W_REG_FLUSH(&regs->phyregaddr, addr);
 
 	W_REG(&regs->phyregdata,
 	      ((R_REG(&regs->phyregdata) & ~mask) | (val & mask)));
diff --git a/drivers/staging/brcm80211/include/bcmutils.h b/drivers/staging/brcm80211/include/bcmutils.h
index 17683f2..d7f531e 100644
--- a/drivers/staging/brcm80211/include/bcmutils.h
+++ b/drivers/staging/brcm80211/include/bcmutils.h
@@ -366,6 +366,17 @@  extern void bcm_prpkt(const char *msg, struct sk_buff *p0);
 	} while (0)
 #endif				/* __BIG_ENDIAN */
 
+#ifdef __mips__
+/*
+ * bcm4716 (which includes 4717 & 4718), plus 4706 on PCIe can reorder
+ * transactions. As a fix, a read after write is performed on certain places
+ * in the code. Older chips and the newer 5357 family don't require this fix.
+ */
+#define W_REG_FLUSH(r, v)	({ W_REG((r), (v)); (void)R_REG(r); })
+#else
+#define W_REG_FLUSH(r, v)	W_REG((r), (v))
+#endif				/* __mips__ */
+
 #define AND_REG(r, v)	W_REG((r), R_REG(r) & (v))
 #define OR_REG(r, v)	W_REG((r), R_REG(r) | (v))