| Message ID | 20240724-u-boot-dwc3-gadget-dcache-fixup-v2-2-65836d699a71@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | dwc3: gadget: properly fix cache operations | expand |
On 7/24/24 5:48 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. > > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/usb/dwc3/io.h | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h > index 04791d4c9be..1aaf5413c6d 100644 > --- a/drivers/usb/dwc3/io.h > +++ b/drivers/usb/dwc3/io.h > @@ -50,6 +50,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(start_addr, end_addr); include/cpu_func.h:void flush_dcache_range(unsigned long start, unsigned long stop); So you shouldn't be feeding this function uintptr_t , right ? Also, why change CACHELINE_SIZE to ARCH_DMA_MINALIGN ?
On 01/10/2024 17:21, Marek Vasut wrote: > On 7/24/24 5:48 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. >> >> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/usb/dwc3/io.h | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h >> index 04791d4c9be..1aaf5413c6d 100644 >> --- a/drivers/usb/dwc3/io.h >> +++ b/drivers/usb/dwc3/io.h >> @@ -50,6 +50,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(start_addr, end_addr); > include/cpu_func.h:void flush_dcache_range(unsigned long start, unsigned long stop); > > So you shouldn't be feeding this function uintptr_t , right ? uintptr_t is the right type to perform pointer calculation, I should probably cast it to unsigned long for flush_dcache_range. I'll do this in v2. > > Also, why change CACHELINE_SIZE to ARCH_DMA_MINALIGN ? Because all the memory allocated for DMA was done with dma_alloc_coherent() which uses ARCH_DMA_MINALIGN directly or indirectly. I should probably remove CACHELINE_SIZE in the same patch. Thanks, Neil
On 10/1/24 5:32 PM, Neil Armstrong wrote: > On 01/10/2024 17:21, Marek Vasut wrote: >> On 7/24/24 5:48 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. >>> >>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> drivers/usb/dwc3/io.h | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h >>> index 04791d4c9be..1aaf5413c6d 100644 >>> --- a/drivers/usb/dwc3/io.h >>> +++ b/drivers/usb/dwc3/io.h >>> @@ -50,6 +50,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(start_addr, end_addr); >> include/cpu_func.h:void flush_dcache_range(unsigned long start, >> unsigned long stop); >> >> So you shouldn't be feeding this function uintptr_t , right ? > > uintptr_t is the right type to perform pointer calculation, > I should probably cast it to unsigned long for flush_dcache_range. Better fix flush_dcache_range(), using unsigned long there is not great, but that's for separate series from this bugfix. > I'll do this in v2. > >> >> Also, why change CACHELINE_SIZE to ARCH_DMA_MINALIGN ? > > Because all the memory allocated for DMA was done with dma_alloc_coherent() > which uses ARCH_DMA_MINALIGN directly or indirectly. > > I should probably remove CACHELINE_SIZE in the same patch. And document this in the commit message, that's useful information. Thanks
On 01/10/2024 17:47, Marek Vasut wrote: > On 10/1/24 5:32 PM, Neil Armstrong wrote: >> On 01/10/2024 17:21, Marek Vasut wrote: >>> On 7/24/24 5:48 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. >>>> >>>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> drivers/usb/dwc3/io.h | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h >>>> index 04791d4c9be..1aaf5413c6d 100644 >>>> --- a/drivers/usb/dwc3/io.h >>>> +++ b/drivers/usb/dwc3/io.h >>>> @@ -50,6 +50,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(start_addr, end_addr); >>> include/cpu_func.h:void flush_dcache_range(unsigned long start, unsigned long stop); >>> >>> So you shouldn't be feeding this function uintptr_t , right ? >> >> uintptr_t is the right type to perform pointer calculation, >> I should probably cast it to unsigned long for flush_dcache_range. > > Better fix flush_dcache_range(), using unsigned long there is not great, but that's for separate series from this bugfix. Since uintptr_t is a typedef to unsigned long, it won't change anything. > >> I'll do this in v2. >> >>> >>> Also, why change CACHELINE_SIZE to ARCH_DMA_MINALIGN ? >> >> Because all the memory allocated for DMA was done with dma_alloc_coherent() >> which uses ARCH_DMA_MINALIGN directly or indirectly. >> >> I should probably remove CACHELINE_SIZE in the same patch. > And document this in the commit message, that's useful information. Done, thx > > Thanks
diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h index 04791d4c9be..1aaf5413c6d 100644 --- a/drivers/usb/dwc3/io.h +++ b/drivers/usb/dwc3/io.h @@ -50,6 +50,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(start_addr, end_addr); } #endif /* __DRIVERS_USB_DWC3_IO_H */