| Message ID | 1307630701-9170-13-git-send-email-rvossen@broadcom.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 9 June 2011 16:44, Roland Vossen <rvossen@broadcom.com> wrote: > From: Arend van Spriel <arend@broadcom.com> > > The source file dma.c used a macro definition that indicated a big endian > platform. The linux tree has its own macro determined by architecture and/or > kernel configuration. This byteorder macro is __BIG_ENDIAN. This is now used > in dma.c. While this replacement is technically correct, the check itself is wrong and should be fixed instead. Currently it assumes that __mips__ means Broadcom MIPS; but if it isn't, then there's likely no SI_SDRAM_SWAPPED and any access there will in best case do nothing (but more like hang the system). Use either CONFIG_BCM47XX or CONFIG_BCM63XX to check for the BMIPS platforms this is probably intended for (I'm not 100% sure if BCM63XX has the swapped memory space - better check with your xDSL people ;-). 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
On 06/10/2011 01:53 AM, Jonas Gorski wrote: > On 9 June 2011 16:44, Roland Vossen<rvossen@broadcom.com> wrote: >> From: Arend van Spriel<arend@broadcom.com> >> >> The source file dma.c used a macro definition that indicated a big endian >> platform. The linux tree has its own macro determined by architecture and/or >> kernel configuration. This byteorder macro is __BIG_ENDIAN. This is now used >> in dma.c. > While this replacement is technically correct, the check itself is > wrong and should be fixed instead. Currently it assumes that __mips__ > means Broadcom MIPS; but if it isn't, then there's likely no > SI_SDRAM_SWAPPED and any access there will in best case do nothing > (but more like hang the system). > > Use either CONFIG_BCM47XX or CONFIG_BCM63XX to check for the BMIPS > platforms this is probably intended for (I'm not 100% sure if BCM63XX > has the swapped memory space - better check with your xDSL people ;-). Hi Jonas, You are right as with previous mips related patches. Consider this a first step which does not change functionality but aims to use the proper linux provided macros instead of our own incarnations. You can be assured that your feedback is on our work list. Gr. AvS
Hi Arend, On 10 June 2011 11:52, Arend van Spriel <arend@broadcom.com> wrote: > Hi Jonas, > > You are right as with previous mips related patches. Consider this a first > step which does not change functionality but aims to use the proper linux > provided macros instead of our own incarnations. Actually as far as I can tell this changes (non-)functionality: With the broken big endiness check the driver probably just doesn't work properly on MIPS BE systems, because some endianess conversion is missing*. With the endianess check fixed, the special handling for the Broadcom MIPS get's enabled, potentially crashing the system (or changing random registers), thus making it *more* broken for non BMIPS systems. Therefore I'd rather consider this a regression. Especially since (AFAIK) the BMIPS systems that can take advantage of this aren't supported in Linux (yet). If it were just making things less efficient, I wouldn't be complaining (at least not that much ;-). If you can say that this is a code path that isn't taken anyway, then my point is moot (for now). I don't know the driver enough to tell that. Jonas *assuming brcm80211 depends on this on BE systems and it isn't just an optimization -- 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
On Fri, Jun 10, 2011 at 04:17:04AM -0700, Jonas Gorski wrote: > Hi Arend, > > On 10 June 2011 11:52, Arend van Spriel <arend@broadcom.com> wrote: > > Hi Jonas, > > > > You are right as with previous mips related patches. Consider this a first > > step which does not change functionality but aims to use the proper linux > > provided macros instead of our own incarnations. > > Actually as far as I can tell this changes (non-)functionality: With > the broken big endiness check the driver probably just doesn't work > properly on MIPS BE systems, because some endianess conversion is > missing*. With the endianess check fixed, the special handling for the > Broadcom MIPS get's enabled, potentially crashing the system (or > changing random registers), thus making it *more* broken for non BMIPS > systems. Therefore I'd rather consider this a regression. Especially > since (AFAIK) the BMIPS systems that can take advantage of this aren't > supported in Linux (yet). > > If it were just making things less efficient, I wouldn't be > complaining (at least not that much ;-). If you can say that this is a > code path that isn't taken anyway, then my point is moot (for now). I > don't know the driver enough to tell that. > > Jonas > > *assuming brcm80211 depends on this on BE systems and it isn't just an > optimization Greg, Can you drop this patch and take the rest in the series? The other patches don't depend on this one. Thanks, - Henry -- 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
On Tue, Jun 14, 2011 at 10:52:20AM -0700, Henry Ptasinski wrote: > On Fri, Jun 10, 2011 at 04:17:04AM -0700, Jonas Gorski wrote: > > Hi Arend, > > > > On 10 June 2011 11:52, Arend van Spriel <arend@broadcom.com> wrote: > > > Hi Jonas, > > > > > > You are right as with previous mips related patches. Consider this a first > > > step which does not change functionality but aims to use the proper linux > > > provided macros instead of our own incarnations. > > > > Actually as far as I can tell this changes (non-)functionality: With > > the broken big endiness check the driver probably just doesn't work > > properly on MIPS BE systems, because some endianess conversion is > > missing*. With the endianess check fixed, the special handling for the > > Broadcom MIPS get's enabled, potentially crashing the system (or > > changing random registers), thus making it *more* broken for non BMIPS > > systems. Therefore I'd rather consider this a regression. Especially > > since (AFAIK) the BMIPS systems that can take advantage of this aren't > > supported in Linux (yet). > > > > If it were just making things less efficient, I wouldn't be > > complaining (at least not that much ;-). If you can say that this is a > > code path that isn't taken anyway, then my point is moot (for now). I > > don't know the driver enough to tell that. > > > > Jonas > > > > *assuming brcm80211 depends on this on BE systems and it isn't just an > > optimization > > > Greg, > > Can you drop this patch and take the rest in the series? The other patches > don't depend on this one. Will do. -- 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 --git a/drivers/staging/brcm80211/brcmsmac/dma.c b/drivers/staging/brcm80211/brcmsmac/dma.c index adf364c..9a9f0e7 100644 --- a/drivers/staging/brcm80211/brcmsmac/dma.c +++ b/drivers/staging/brcm80211/brcmsmac/dma.c @@ -507,9 +507,9 @@ struct dma_pub *dma_attach(char *name, struct si_pub *sih, di->dataoffsetlow = di->ddoffsetlow; di->dataoffsethigh = di->ddoffsethigh; } -#if defined(__mips__) && defined(IL_BIGENDIAN) +#if defined(__mips__) && defined(__BIG_ENDIAN) di->dataoffsetlow = di->dataoffsetlow + SI_SDRAM_SWAPPED; -#endif /* defined(__mips__) && defined(IL_BIGENDIAN) */ +#endif /* defined(__mips__) && defined(__BIG_ENDIAN) */ /* WAR64450 : DMACtl.Addr ext fields are not supported in SDIOD core. */ if ((ai_coreid(sih) == SDIOD_CORE_ID) && ((ai_corerev(sih) > 0) && (ai_corerev(sih) <= 2))) @@ -624,12 +624,12 @@ dma64_dd_upd(dma_info_t *di, dma64dd_t *ddring, dmaaddr_t pa, uint outidx, u32 ctrl2 = bufcount & D64_CTRL2_BC_MASK; /* PCI bus with big(>1G) physical address, use address extension */ -#if defined(__mips__) && defined(IL_BIGENDIAN) +#if defined(__mips__) && defined(__BIG_ENDIAN) if ((di->dataoffsetlow == SI_SDRAM_SWAPPED) || !(PHYSADDRLO(pa) & PCI32ADDR_HIGH)) { #else if ((di->dataoffsetlow == 0) || !(PHYSADDRLO(pa) & PCI32ADDR_HIGH)) { -#endif /* defined(__mips__) && defined(IL_BIGENDIAN) */ +#endif /* defined(__mips__) && defined(__BIG_ENDIAN) */ W_SM(&ddring[outidx].addrlow, BUS_SWAP32(PHYSADDRLO(pa) + di->dataoffsetlow));