Dear Ian,
On Sat, 2014-04-19 at 14:52 +0100, Ian Campbell wrote:
> - /* Invalidate only "status" field for the following check */
> - invalidate_dcache_range((unsigned long)&desc_p->txrx_status,
> - (unsigned long)&desc_p->txrx_status +
> - sizeof(desc_p->txrx_status));
> + /* Strictly we only need to invalidate the "status" field for
> + * the following check, but on some platforms we cannot
> + * invalidate only 4 bytes, so invalidate the the whole thing
> + * which is known to be DMA aligned. */
> + invalidate_dcache_range((unsigned long)desc_p,
> + (unsigned long)desc_p +
> + sizeof(struct dmamacdescr));
>
> /* Check if the descriptor is owned by CPU */
> if (desc_p->txrx_status & DESC_TXSTS_OWNBYDMA) {
Unfortunately I cannot recall exactly why I wanted to invalidate only
"status" field.
Now looking at this code I may assume that I wanted to save some CPU
cycles. Because:
1. We don't care about all other fields except "status". GMAC only
changes "status" field when it resets "OWNED_BY_DMA" flag and all other
fields CPU writes but not reads while sending packets.
2. We may save quite a few CPU cycles if only invalidating minimum
amount of bytes (remember each read from external memory may cost 100s
of cycles).
So I would advise:
1. Don't invalidate "sizeof(struct dmamacdescr)" but only
"roundup(sizeof(desc_p->txrx_status), ARCH_DMA_MINALIGN))".
2. In the following lines implements rounding as well:
============
/* Flush data to be sent */
flush_dcache_range((unsigned long)desc_p->dmamac_addr,
(unsigned long)desc_p->dmamac_addr + length);
============
We may be sure "desc_p->dmamac_addr" is properly aligned, but length
could be not-aligned. So I'd replace "length" with "roundup(length,
ARCH_DMA_MINALIGN)" as you did in 3rd patch.
3. Check carefully if there're other instances of probably unaligned
cache operations. I erroneously didn't care about alignment on cache
invalidation/flushing because my implementation of those cache
operations deals with non-aligned start/end internally within
invalidate/flush functions - which might be not that good even if it's
convenient for me.
4. Why don't you squeeze all 3 patches in 1 and name it like "fix
alignment issues with caches on some platforms"? Basically with all 3
patches you fix one and only issue and application of any one of those 3
patches doesn't solve your problem, right?
Regards,
Alexey
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
