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. 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.
Attachment:
signature.asc
Description: Digital signature
