On Monday 25 February 2013 18:51:53 Rafał Miłecki wrote:
> 2013/2/25 Tijs Van Buggenhout <t...@able.be>:
> > I opted to introduce a CONFIG_BGMAC_UNALIGNED_ADDRESSING macro to
> > introduce
> > the changes to support unaligned addressing for the bgmac driver (it does
> > add some extra bytes in bgmac_dma_ring struct). This could be an option
> > in Kconfig (not yet in patch), but for now it is defined fix in bgmac.h.
> > The
> > implementation should work for both aligned and unaligned addressing when
> > CONFIG_BGMAC_UNALIGNED_ADDRESSING is defined.
>
> I don't think we should need CONFIG_BGMAC_UNALIGNED_ADDRESSING, it
> doesn't save a lot of size (when disabled) and makes configuration
> more confusing.
I'll get rid of the defines, and create another patch ;-)
> > If there is no need for en extra Kconfig option, let me know, I'll adjust
> > the patch. In the other case, I'll create an extra entry in Kconfig and
> > add it to the patch aswell. I created the patch from 3.6 kernel sources,
> > let me know if there are differences for the 3.8 kernel.
>
> Kernel 3.6 doesn't include bgmac at all ;)
Building from openwrt trunk, which configures a 3.6.11 kernel by default,
where bgmac is patched in?
> > I hope this can be tested on different hardware supporting dma
> > aligned/unaligned addressing, it should work on both. Any
> > comments/suggestions are welcome..
>
> I'll give it a try on my BCM4706s, just give me some time.
No problem, take your time ;-)
> > Note: Besides the changes for unaligned addressing, I corrected some types
> > for variables and formats. I also introduced an extra int j variable in
> > the second loop within dma initialisation of rx ring(s).. from my
> > understanding reusing the int i variable in the inner loop breaks the
> > outer loop if there would be more than one rx ring.
>
> That definitely should go in separated patches.
I'll separate them into different patches. The loop variable fix is already
settled
> > Signed-off-by: Tijs Van Buggenhout (t...@able.be)
>
> Your whole patch is malformed, there are extra line breaks and tabs
> were eaten by your mailer.
It's a cp&p from a cat'ed diff on console. Email client does a line wrap after
80 characters in plain text. Will try as attachment next time...
> > @@ -384,6 +388,10 @@
> >
> > u16 mmio_base;
> > struct bgmac_dma_desc *cpu_base;
> > dma_addr_t dma_base;
> >
> > +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING
> > + bool is_unaligned;
> > + u32 index_base;
>
> I'm pretty sure it's some duplication, we already store address somewhere
> else.
Correct, but has implicit 0 value, when it is not assigned. Avoiding if
conditions in the rest of the code.
> > @@ -156,6 +156,9 @@
> >
> > if (++ring->end >= BGMAC_TX_RING_SLOTS)
> >
> > ring->end = 0;
> >
> > bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_TX_INDEX,
> >
> > +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING
> > + ring->index_base +
> > +#endif
>
> Note: If we de-duplicate index_base we have to add that conditionally.
Indeed .. we could go for the bool is_unaligned, and write something like
'((is_unaligned ? lower_32_bits( ring->dma_base ) : 0 ) + ' or would you
prefer a normal if statement.
We could also go for a local variable index_base in the corresponding
functions, which is initialised to 0 or set to the correct value in case of
unalignment.
> > + if (ring->start == ring->end) {
> > + bgmac_warn(bgmac, "Ignore DMA TX free on empty ring
> > 0x%X\n", ring->mmio_base);
> > + return;
> > + }
> > +
>
> Is that supposed to happen? I've to analyze that part :)
We can leave it out.. it's an optimisation (BGMAC_DMA_TX_STATUS doesn't need
to be read). I saw the warning appear a few times on my screen today, so the
message print is better left out anyways.
> > /* The last slot that hardware didn't consume yet */
> > empty_slot = bgmac_read(bgmac, ring->mmio_base +
> > BGMAC_DMA_TX_STATUS);
> > empty_slot &= BGMAC_DMA_TX_STATDPTR;
> >
> > +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING
> > + empty_slot -= ring->index_base;
> > + empty_slot &= BGMAC_DMA_TX_STATDPTR;
> > +#endif
> >
> > empty_slot /= sizeof(struct bgmac_dma_desc);
>
> Do we need to mask that twice?
I kept as close to the broadcom sources as possible. Keeping in mind that
empty_slot is an u16, bgmac_read returns an u32 as is index_base, it's
probably best (for clarity) to write:
empty_slot = ( bgmac_read(bgmac, ring->mmio_base + BGMAC_DMA_TX_STATUS) &
BGMAC_DMA_TX_STATDPTR );
empty_slot -= ( ring->index_base & BGMAC_DMA_TX_STATDPTR )
but
empty_slot = (( bgmac_read(bgmac, ring->mmio_base + BGMAC_DMA_TX_STATUS) -
ring->index_base ) & BGMAC_DMA_TX_STATDPTR );
can avoid the extra mask. Should I include an extra type cast (u16)?
> > + if (((ring->start == 0) && (empty_slot > ring->end)) ||
> > + (empty_slot >= ring->num_slots)) {
> > + bgmac_err(bgmac, "Bogus current TX slot index %u (start
> > index: %u, end index: %u)\n",
> > + empty_slot, ring->start, ring->end);
> > + return;
> > + }
>
> Looks OK to add the check, I just wonder: did you actually hit that?
> Maybe we setup something incorrectly?
Nope, this should never occur.. If it is, then something is wrong in the setup
as you mention. The hardware will not recover from this.
But we need something to avoid that the while loop is executed, possibly
unmapping incorrect dma address / freeing some kernel memory, mess up the
ring->start value, and wake up the netif queue. The error can also be more
informative/correct than 'Hardware reported transmission for empty TX ring
slot'.
We saw packets leaving the device that contained data from memory that was not
owned by the driver (at high rates).
> > @@ -416,9 +439,15 @@
> >
> > ring = &bgmac->tx_ring[i];
> > ring->num_slots = BGMAC_TX_RING_SLOTS;
> > ring->mmio_base = ring_base[i];
> >
> > +#ifdef CONFIG_BGMAC_UNALIGNED_ADDRESSING
> > + if ((ring->is_unaligned = bgmac_dma_unaligned(bgmac, ring,
> > BGMAC_DMA_RING_TX)))
> > + bgmac_warn(bgmac, "TX on ring 0x%X supports
> > unaligned addressing\n",
> > + ring->mmio_base);
>
> Warn? ;)
I guess we can make this an informational message?
> > +#else
> >
> > if (bgmac_dma_unaligned(bgmac, ring, BGMAC_DMA_RING_TX))
> >
> > bgmac_warn(bgmac, "TX on ring 0x%X supports
> > unaligned
> >
> > addressing but this feature is not implemented\n",
> >
> > ring->mmio_base);
>
> There should be info like "not enabled" or sth, but it'll go anyway
> when removing CONFIG_BGMAC_UNALIGNED_ADDRESSING
Correct ;-)
Regards,
Tijs
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
