[PATCH] Fix alignment issues with DMA TX on BCM4331

[Rafał Miłecki]

W dniu 13 sierpnia 2011 22:18 użytkownik David Woodhouse
<dwmw2 at infradead.org> napisał:
> On Sat, 2011-08-13 at 21:06 +0200, Rafał Miłecki wrote:
>> W dniu 12 sierpnia 2011 12:27 użytkownik David Woodhouse
>> <dwmw2 at infradead.org> napisał:
>> > When the TX descriptor ring is not aligned to 8KiB on BCM4331, we have
>> > to write the full address to the TXINDEX register or the DMA engine gets
>> > confused after the first time we wrap round to slot zero.
>> >
>> > [ 7438.538945] Poked TX with address fe0 for slot 254
>> > [ 7438.539077] Acking gen reason 20000000
>> > [ 7438.539177] irq xmitstat 20fc1001 0000007f
>> > [ 7438.607861] Poked TX with address 0 for slot 0
>> > [ 7438.608567] Acking gen reason 20000000
>> > [ 7438.608668] irq xmitstat 20fe1001 00000080
>> > [ 7438.608709] irq xmitstat 20000011 00000080
>> > [ 7438.608724] b43-phy2 debug: Out of order TX status report on DMA ring 1. Expected 256, but got 0
>> > [ 7438.608739] irq xmitstat 20020011 00000080
>> > [ 7438.608750] b43-phy2 debug: Out of order TX status report on DMA ring 1. Expected 256, but got 2
>> > [ 7438.608765] irq xmitstat 20040011 00000080
>> >
>> > We write 0xff0 to the TXADDRLO register to see if the DMA engine is
>> > capable of unaligned operation. If it *is*, then it'll have this problem
>> > and we have to write the full address to TXINDEX. Comments in brcmsmac
>> > indicate that the low 13 bits are required.
>> >
>> > If we're doing this, we *also* have to write to TXCTL to enable the DMA
>> > engine *after* setting up the ring address.
>> >
>> > Signed-off-by: David Woodhouse <David.Woodhouse at intel.com>
>> > --
>> > I've made that change to the initialisation order of TXCTL vs.
>> > TXADDR{HI,LO} unconditional; is there a reason not to?
>> >
>> > diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
>> > index 82168f8..92dd6d9 100644
>> > --- a/drivers/net/wireless/b43/dma.c
>> > +++ b/drivers/net/wireless/b43/dma.c
>> > @@ -225,8 +225,10 @@ static void op64_fill_descriptor(struct b43_dmaring *ring,
>> >
>> >  static void op64_poke_tx(struct b43_dmaring *ring, int slot)
>> >  {
>> > -       b43_dma_write(ring, B43_DMA64_TXINDEX,
>> > -                     (u32) (slot * sizeof(struct b43_dmadesc64)));
>> > +       u32 indexval = slot * sizeof(struct b43_dmadesc64);
>> > +       if (ring->unaligned)
>> > +               indexval |= (u32)ring->dmabase;
>> > +       b43_dma_write(ring, B43_DMA64_TXINDEX, indexval);
>>
>> Shouldn't this bit or be a numerical sum? I mean: +=
>> Imagine you get dmabase like 0x1f310123 instead of some nice 0x1f310000
>
> You won't. The ring is always going to be aligned to its own size, so a
> simple mask is perfectly sufficient. Even if we go to an 8KiB ring
> that'll still be true.
>
> If we ever tried to use a ring that *wasn't* so aligned, I strongly
> suspect we'd get other hardware problems like the ones that this patch
> addresses.

I didn't know about such aligning. Of course my example 0x1f310123 was
silly, it was more realistic to take for ex. 0x1f310100. But if you
say kernel will always nicely align rings, OK then. Thanks for
teaching me sth new.

-- 
Rafał