Re: [PATCH v3 2/3] usb: dwc3: fix dcache flush range calculation

[Neil Armstrong <neil.armstrong@linaro.org>]

On 02/10/2024 16:55, Marek Vasut wrote:
> On 10/2/24 4:39 PM, Neil Armstrong wrote:
>> The current flush operation will omit doing a flush/invalidate on
>> the first and last bytes if the base address and size are not aligned
>> with DMA_MINALIGN.
>>
>> This causes operation failures Qualcomm platforms.
>>
>> Take in account the alignment and size of the buffer and also
>> flush the previous and last cacheline.
>>
>> Remove CACHELINE_SIZE which was the same as DMA_MINALIGN.
> 
> It isn't the same, CACHELINE_SIZE was set to CONFIG_SYS_CACHELINE_SIZE (CPU L1 cache cacheline length) while ARCH_DMA_MINALIGN is DMA engine alignment requirement (from times where there used to be one DMA engine on most devices). You likely want a max(CONFIG_SYS_CACHELINE_SIZE, dwc3-buffer-alignment-requirement) to really correctly align the buffer.

It is definitely true for platforms declaring dma_alloc_coherent() (arm, riscv, x86)
except nios2 but there's 0 chance dwc3 appears on a nios2 platform.

Neil

> 
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/usb/dwc3/io.h | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
>> index 04791d4c9be..a6c2bb0f47d 100644
>> --- a/drivers/usb/dwc3/io.h
>> +++ b/drivers/usb/dwc3/io.h
>> @@ -20,7 +20,6 @@
>>   #include <cpu_func.h>
>>   #include <asm/io.h>
>> -#define    CACHELINE_SIZE        CONFIG_SYS_CACHELINE_SIZE
>>   static inline u32 dwc3_readl(void __iomem *base, u32 offset)
>>   {
>>       unsigned long offs = offset - DWC3_GLOBALS_REGS_START;
>> @@ -50,6 +49,9 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
>>   static inline void dwc3_flush_cache(uintptr_t addr, int length)
>>   {
>> -    flush_dcache_range(addr, addr + ROUND(length, CACHELINE_SIZE));
>> +    uintptr_t start_addr = (uintptr_t)addr & ~(ARCH_DMA_MINALIGN - 1);
>> +    uintptr_t end_addr = ALIGN((uintptr_t)addr + length, ARCH_DMA_MINALIGN);
>> +
>> +    flush_dcache_range((unsigned long)start_addr, (unsigned long)end_addr);
>>   }
>