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