On Tue, 4 Feb 2014 21:21:16 +0100 Marek Vasut marex@denx.de wrote:
On Tuesday, February 04, 2014 at 08:29:49 AM, Lukasz Majewski wrote:
Hi Marek,
On Monday, February 03, 2014 at 12:06:59 PM, Lukasz Majewski wrote:
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@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@samsung.com > > Cc: Marek Vasut marex@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
_Should_ not happen ... I am absolutelly positive someone will be bitten by such assumption. I think this assumption about buffer alignment should really be documented somewhere.
I will add verbose commit message for this.
The commit message will get lost as time moves on, this should be clearly documented in some doc/ or code comment.
Ok. I will add code comment.
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.
Document that please, I doubt this is documented anywhere, but it's clearly part of the API. Also, some checks might be put in place for the alignment , they might be in #ifdef DEBUG for all I care, but it would be nice to have such a check, since I'm worried someone will really be bitten.
I rely on the code embedded at cache_v7.c. It works and saved me a lot of trouble (since it always print "ERROR" when buffer alignment and size is wrong).
Also I think, that those checks shall be done at invalidate_* and flush_* cache routines, not at USB driver.
Not all CPUs are ARMv7 though.
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.
Yeah, for arm926ejs core as well. Maybe that check shall be shifted into the cache management routine prototypes somehow ... not all CPUs implement that check :-(
Maybe other ARM architectures shall have the cache management code updated to work in a similar way to cache_v7.c ?
Not all CPUs are ARM architecture ... there're others, you know ;-)
:-) ... but who cares about the rest :-)
To be serious (quite), I do believe that checking if unaligned cache flush/invalidation is performed, shall be handled at the code which is responsible for cache management.
Conceptually, it shall not be done at UDC code.
It complains with "ERROR" message when start or end address is not aligned (that is how I've discovered the unaligned buffers at UMS).
Yes.
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.
Got it.
I will emphasize the need of correct buffer allocation at commit message and add some comment to UDC in the v2.
Best regards, Lukasz Majewski