Re: [RFC PATCH 3/6] spi: spi-s3c64xx: Add coherent buffers for DMA transfers

["Łukasz Czerwiński" <l.czerwinski@samsung.com>]

On 09/10/2013 02:02 PM, Mark Brown wrote:
> On Tue, Sep 10, 2013 at 01:23:45PM +0200, Łukasz Czerwiński wrote:
>> On 09/09/2013 04:45 PM, Mark Brown wrote:
>
>>> This seems like a very low limit to have, consider things like firmware
>>> downloads for example.  It seems reasonable to have a preallocated small
>>> buffer but there should be some fallback for larger transfer sizes.
>
>> I have tested my modification with different buffer sizes with
>> S5C73M3 driver (355560 B upload). I obtained following upload times:
>
>> 16kB buffer:
>> 	- 83.972 ms
>> 	- 79.196 ms
>> 	- 79.432 ms
>> 128kB buffer:
>> 	- 74.449 ms
>> 	- 80.719 ms
>> 	- 75.599 ms
>
>> For 256kB I obtained similar results as in 128kB case. Normally
>> non-interrupted SPI transfer should take 56ms (50MHz). Performance
>> loss is approximately about 6% between 16kB and 128KB buffer. I my
>> opinion there are no need to use bigger buffers.
>
> You're missing the point here.  This requires the client driver to know
> the maximum transfer size that the controller supports and split the
> transfer up for it.  This isn't something that the SPI API supports now
> and while it would be useful to add for controllers that are genuinely
> limited in transfer size it doesn't seem sane to just randomly impose a
> limit to make life easier for software.  Not all applications will be
> able to split their transfers up in the first place and it seems like
> the wrong place to put that knowledge.
>
> What I would expect to see the driver doing here is either allocating
> a larger buffer on demand or (probably more reliably given the need for
> contiguous physical memory) transparently splitting larger transfers.
> A super smart implementation would overlap the copy of blocks with the
> transfers of preceeding blocks, and there should be something we can do
> to avoid having to copy data that's already in suitable memory.
>
Thanks for your comments.
I will try implement transparently splitting larger transfer.

> Ideally this would all be factored out into the core based on limit
> information but that's probably not required immediately.  Though now I
> write that I'm wondering how many other devices should be doing this but
> aren't.  The DMA mapping of transfer buffers doesn't seem terribly
> device specific...
>
>> I propose add extra module parameter which allows to change buffer size.
>
> No, module parameters are not sensible for modern code.  If you wanted
> runtime configuration a sysfs tunable would be better though I do think
> that should be tuning optimisation, not a hard limit on the transfer
> size.
>
ok

Thanks
Lukasz