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.
> 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 ;)
> 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.
> 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.
> 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.
> @@ -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.
> @@ -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.
> + 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 :)
> /* 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?
>
> + 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?
> @@ -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? ;)
> +#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
--
Rafał
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
