[U-Boot] [PATCH 2/3] net/designware: invalidate entire descriptor in dw_eth_send

[Alexey Brodkin]

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