Michael Buesch wrote: > On Thursday 15 November 2007 06:55:42 Larry Finger wrote: >> --- wireless-2.6.orig/drivers/net/wireless/b43/dma.c >> +++ wireless-2.6/drivers/net/wireless/b43/dma.c >> @@ -165,7 +165,7 @@ static void op64_fill_descriptor(struct >> addrhi = (((u64) dmaaddr >> 32) & ~SSB_DMA_TRANSLATION_MASK); >> addrext = (((u64) dmaaddr >> 32) & SSB_DMA_TRANSLATION_MASK) >> >> SSB_DMA_TRANSLATION_SHIFT; >> - addrhi |= ssb_dma_translation(ring->dev->dev); >> + addrhi |= (ssb_dma_translation(ring->dev->dev) << 1); >> if (slot == ring->nr_slots - 1) >> ctl0 |= B43_DMA64_DCTL0_DTABLEEND; >> if (start) > > Wait, this looks broken to me. It looks like the bug is more > inside of the ssb_dma_translation function, rather than here. Yes and no. In the DMA specs (http://bcm-v4.sipsolutions.net/802.11/DMA), a 2-bit "routing code" is used for the address portion of a descriptor. For 32-bit, a value of 1 indicates "Client Mode Translation". With a 64-bit device, a value of 2 is the valid routing code and 1 leads to DMA Errors. It seemed easier to take a 0/1 output from ssb_dma_translation and shift it for the 64-bit case than to teach ssb to distinguish between 32- and 64-bit DMA. BTW, we do have one other option - we could ignore the routing and address extension bits and _limit_ the user to a maximum of 0.5 Zettabytes! What is the probability that any machine will exceed that amount of memory within our lifetimes? But then, I never expected to see Terrabyte disk drives in a 3.5 inch package!! Have you noted that Broadcom uses a 2-bit routing code _AND_ a 2-bit address extension? Why they didn't use a flat 64-bit address is a mystery. I can, however, go either way - your drivers and your call. >> @@ -426,14 +426,15 @@ static inline >> static int alloc_ringmemory(struct b43_dmaring *ring) >> { >> struct device *dev = ring->dev->dev->dev; >> + int size = (ring->dma64) ? 8192 : B43_DMA_RINGMEMSIZE; > > Uhm, a page is also 4k in x86_64? > Why doesn't using a page here work? What does happen? You get random 4K/8K alignment and the driver fails with a Fatal DMA error for those with 4K alignment. >> @@ -636,18 +637,13 @@ static int dmacontroller_setup(struct b4 >> if (ring->dma64) { >> u64 ringbase = (u64) (ring->dmabase); >> >> - addrext = ((ringbase >> 32) & SSB_DMA_TRANSLATION_MASK) >> - >> SSB_DMA_TRANSLATION_SHIFT; >> - value = B43_DMA64_TXENABLE; >> - value |= (addrext << B43_DMA64_TXADDREXT_SHIFT) >> - & B43_DMA64_TXADDREXT_MASK; >> - b43_dma_write(ring, B43_DMA64_TXCTL, value); >> + b43_dma_write(ring, B43_DMA64_TXCTL, >> + B43_DMA64_TXENABLE); > > Ehm, why are you removing this? Unlike 32-bit DMA and the 64-bit descriptors, the 64-bit case uses two full 32-bit words to store the Descriptor Ring Address. No fancy packing of the upper bits into the control word are needed. >> b43_dma_write(ring, B43_DMA64_TXRINGLO, >> (ringbase & 0xFFFFFFFF)); >> b43_dma_write(ring, B43_DMA64_TXRINGHI, >> ((ringbase >> 32) & >> - ~SSB_DMA_TRANSLATION_MASK) >> - | trans); >> + 0xFFFFFFFF)); > > Huh? This mask is not needed. It is a carryover from when I was having a problem. This will become b43_dma_write(ring, B43_DMA64_TXRINGHI,(ringbase >> 32)); in the next version. >> } else { >> u32 ringbase = (u32) (ring->dmabase); >> >> @@ -668,20 +664,16 @@ static int dmacontroller_setup(struct b4 >> if (ring->dma64) { >> u64 ringbase = (u64) (ring->dmabase); >> >> - addrext = ((ringbase >> 32) & SSB_DMA_TRANSLATION_MASK) >> - >> SSB_DMA_TRANSLATION_SHIFT; >> - value = (ring->frameoffset << B43_DMA64_RXFROFF_SHIFT); >> - value |= B43_DMA64_RXENABLE; >> - value |= (addrext << B43_DMA64_RXADDREXT_SHIFT) >> - & B43_DMA64_RXADDREXT_MASK; >> + value = (ring->frameoffset << B43_DMA64_RXFROFF_SHIFT) >> + | B43_DMA64_RXENABLE; >> b43_dma_write(ring, B43_DMA64_RXCTL, value); >> b43_dma_write(ring, B43_DMA64_RXRINGLO, >> (ringbase & 0xFFFFFFFF)); >> b43_dma_write(ring, B43_DMA64_RXRINGHI, >> ((ringbase >> 32) & >> - ~SSB_DMA_TRANSLATION_MASK) >> - | trans); >> - b43_dma_write(ring, B43_DMA64_RXINDEX, 200); >> + 0xFFFFFFFF)); >> + b43_dma_write(ring, B43_DMA64_RXINDEX, ring->nr_slots * >> + sizeof(struct b43_dmadesc64)); > > Same here. Same change for the RX Descriptor Ring Address. >> @@ -695,11 +687,12 @@ static int dmacontroller_setup(struct b4 >> b43_dma_write(ring, B43_DMA32_RXRING, >> (ringbase & ~SSB_DMA_TRANSLATION_MASK) >> | trans); >> - b43_dma_write(ring, B43_DMA32_RXINDEX, 200); >> + b43_dma_write(ring, B43_DMA32_RXINDEX, ring->nr_slots * >> + sizeof(struct b43_dmadesc32)); > > I'm not sure why you do this change. It took me a while to figure out where the magic number of 200 came from, and what I needed for the 64-bit case. In fact I think the 200 is a bug and should be 0x200. To me, this change makes it clearer. >> Index: wireless-2.6/drivers/net/wireless/b43/dma.h >> =================================================================== >> --- wireless-2.6.orig/drivers/net/wireless/b43/dma.h >> +++ wireless-2.6/drivers/net/wireless/b43/dma.h >> @@ -260,6 +260,13 @@ static inline u32 b43_dma_read(struct b4 >> static inline >> void b43_dma_write(struct b43_dmaring *ring, u16 offset, u32 value) >> { >> + /* temporary debugging code */ >> + if (((offset == 8) || (offset == 0x28)) && ring->dma64 && >> + ((value & 0x1FFF) != 0)) { >> + printk(KERN_ERR "b43: bad desc ring address for 64-bit DMA" >> + " - offset, value: 0x%.2X 0x%.4X\n", offset, value); >> + dump_stack(); >> + } > > Ok, temporary. You know what that means :) In this case, it disappears before the patch goes to John! Larry - To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
