[PATCH] net: dwc_eth_qos: Pad descriptors to cacheline size

[Stephen Warren]

On 4/29/20 1:56 PM, Marek Vasut wrote:
> On 4/29/20 9:51 PM, Stephen Warren wrote:
>> On 4/29/20 1:14 PM, Marek Vasut wrote:
>>> The DWMAC4 IP has the possibility to skip up to 7 64bit words after
>>> the descriptor, use this to pad the descriptors to cacheline size.
>>
>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>>
>>>  /* Descriptors */
>>> +/*
>>
>> Nit: Blank line between those two comments to match the existing style.
>>
>>> + * #if EQOS_DESCRIPTOR_SIZE < ARCH_DMA_MINALIGN fails if we use sizeof()
>>> + * or offsetof() to calculate descriptor size, since neither is allowed
>>> + * in C preprocessor macros, so just hard-code this to 16=4*sizeof(u32).
>>> + */
>>> +#define EQOS_DESCRIPTOR_SIZE	16
>>> +#define EQOS_DESCRIPTOR_PAD	((ARCH_DMA_MINALIGN - EQOS_DESCRIPTOR_SIZE) / 4)
>>> +struct eqos_desc {
>>> +	u32 des0;
>>> +	u32 des1;
>>> +	u32 des2;
>>> +	u32 des3;
>>> +	u32 __pad[EQOS_DESCRIPTOR_PAD];
>>> +};
>>
>> I think the padding needs to be optional based on the same condition in
>> the following existing warning logic, and the warning logic may need to
>> be removed/updated to account for the fact that padding is now being
>> used to avoid the issue:
>>
>> (quoting this from existing code)
>>> /*
>>>  * Warn if the cache-line size is larger than the descriptor size. In such
>>>  * cases the driver will likely fail because the CPU needs to flush the cache
>>>  * when requeuing RX buffers, therefore descriptors written by the hardware
>>>  * may be discarded. Architectures with full IO coherence, such as x86, do not
>>>  * experience this issue, and hence are excluded from this condition.
>>>  *
>>>  * This can be fixed by defining CONFIG_SYS_NONCACHED_MEMORY which will cause
>>>  * the driver to allocate descriptors from a pool of non-cached memory.
>>>  */
>>> #if EQOS_DESCRIPTOR_SIZE < ARCH_DMA_MINALIGN
>>> #if !defined(CONFIG_SYS_NONCACHED_MEMORY) && \
>>>         !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) && !defined(CONFIG_X86)
>>> #warning Cache line size is larger than descriptor size
>>> #endif
>>> #endif
>>
>> (back to quoting from the patch)
>>> -#define EQOS_DESCRIPTOR_WORDS	4
>>> -#define EQOS_DESCRIPTOR_SIZE	(EQOS_DESCRIPTOR_WORDS * 4)
>> ...
>>> -struct eqos_desc {
>>> -	u32 des0;
>>> -	u32 des1;
>>> -	u32 des2;
>>> -	u32 des3;
>>> -};
>>
>> This patch would be a lot smaller if those definitions were kept in
>> their existing location instead of moving them...
> 
> Could you at least test it on the tegra ?

This patch (applied on top of current u-boot/master) does cause network
failures on Jetson TX2 (Tegra186), which obviously uses this driver.

The docs for our version of the IP block do indicate that the DSL shift
is supported, so I think the patch should work. I wonder if it's because
of the bus width of our EQoS IP block? Our docs indicate that DSL is a
multiple of the bus width, so describes a skip of word/dword/lword count
based on 32/64/128 bus width. Perhaps the bus width needs to be
parametrized when calculating the value? Our docs don't seem to indicate
which bus width our HW uses:-( Aha, but changing the "/2" to "/4" in the
code in this patch which calculates DSL value does yield a working
system, so I guess we have a 128-bit bus width. And indeed this matches
a comment I wrote in the driver:

        /*
         * Burst length must be < 1/2 FIFO size.
         * FIFO size in tqs is encoded as (n / 256) - 1.
         * Each burst is n * 8 (PBLX8) * 16 (AXI width) == 128 bytes.

16 byte AXI width == 128 bit width.