[U-Boot] [PATCH 4/6] usb:udc:samsung: Zero copy approach for data passed to Samsung's UDC driver

[Lukasz Majewski]

Hi Marek,

> On Saturday, February 01, 2014 at 12:05:29 PM, Lukasz Majewski wrote:
> > On Sat, 1 Feb 2014 03:55:20 +0100
> > 
> > Marek Vasut <marex at denx.de> wrote:
> > > On Friday, January 31, 2014 at 01:16:27 PM, Lukasz Majewski wrote:
> > > > The Samsung's UDC driver is not anymore copying data from USB
> > > > requests to data aligned internal buffers. Now it works
> > > > directly in data allocated in the upper layers like UMS, DFU,
> > > > THOR.
> > > > 
> > > > This change is possible since those gadgets now take care to
> > > > allocate buffers aligned to cache line
> > > > (CONFIG_SYS_CACHELINE_SIZE ).
> > > > 
> > > > Previously the UDC needed to copy this data to internal aligned
> > > > buffer to prevent from unaligned access exceptions.
> > > > 
> > > > Test condition
> > > > - test HW + measurement: Trats - Exynos4210 rev.1
> > > > - test HW Trats2 - Exynos4412 rev.1
> > > > 400 MiB compressed rootfs image download with `thor 0 mmc 0`
> > > > 
> > > > Measurement:
> > > > Transmission speed: 27.04 MiB/s
> > > > 
> > > > Change-Id: I1df1fbafc72ec703f0367ddee3fedf3a3f5523ed
> > > > Signed-off-by: Lukasz Majewski <l.majewski at samsung.com>
> > > > Cc: Marek Vasut <marex at denx.de>
> > > 
> > > You should use ROUND_UP(), not ROUND() throughout the patch.
> > > Otherwise you might fail to flush/invalidate the last little bit
> > > of data in some cacheline.
> > 
> > I might overlooked something, so please correct me if needed.
> > 
> > I allocate buffers in gadgets which are aligned to cache line with
> > starting address and its size is a multiplication of cache line size
> > (so I will not trash data allocated next to it when I invalidate
> > cache).
> > 
> > In the code I'm using ROUND to invalidate/flush more data than
> > needed (ROUND(176, 32) = 192). I'm prepared for this since buffer
> > in gadget is properly allocated (with DEFINE_CACHE_ALIGN_BUFFER()
> > which uses roundup() internally).
> 
> The problem is in case you receive buffer which is aligned to
> cacheline with it's start, but is [(k * cacheline_size) +
> (cacheline_size / 2) - 1] big. I think it's unlikely, but if this
> happens, you will get corruption, right ? 

Let's suppose, that I will receive 2063B = [(64 * 32) + 16 -1] from UDC.
If the passed buffer was exactly 2063 B in size, then we would have
here a data corruption.

However this situation will not happen since the buffer at gadget is
allocated with DEFINE_CACHE_ALIGN_BUFFER() or is an aligned
multiplication of cache line size (like 1MiB).

I think, that it is the responsibility of gadget developer to allocate
buffers with proper alignment and size.

> You might actually want to
> check for this condition and throw a warning in such a case.

The check is already implemented at ./arch/arm/cpu/armv7/cache_v7.c.

It complains with "ERROR" message when start or end address is not
aligned (that is how I've discovered the unaligned buffers at UMS).

> 
> I understand your argument with trying to not trash data, but then
> you will get a corruption during transfer, right ?

After applying those patches, the cache management would be performed
when the USB request is completed (in the UDC).

The only requirement for UDC is the correctly allocated buffer at
gadget.

> 
> Best regards,
> Marek Vasut



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group