On 4/29/20 11:25 PM, Stephen Warren wrote: > 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.
That's what I was afraid of. > 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? The STM32MP1 docs say that the shift is in 64bit words, maybe that actually translate into bus words rather than 8 bytes indeed ? btw what is your ARCH_DMA_MINALIGN on t186, 64 ? I'm worried that if something has it set to 128 or more, then this DSL padding won't be able to cover it, since it can only skip up to 7 64bit words. > 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. So how do we parametrize that, based on compatible string I guess ? We already have params for the tegra186 and stm32mp1 variants of the IP, so adding one more should be OK. Also, thanks for looking into it. -- Best regards, Marek Vasut