On Wed, 17 Mar 2010 15:39:23 +0800 christian pellegrin <chripell@xxxxxxxxx> wrote: > On Wed, Mar 17, 2010 at 3:35 AM, Feng Tang <feng.tang@xxxxxxxxx> > wrote: > > Hi Christian, > > Hi, > > > your driver is posted on public. And the reasons I still posted > > driver here are: 1. It provides a console > > I'll provide a patch to the mainline driver for the console. Let me > just finish my daily job duties ;-) Cool!! I can test that on my platform. > > > 2. It use buffer read/write to meet our performance goal, max3110 > > doesn't have good FIFO, but spi controller have and we can leverage > > that for performance > > I think this line of thinking is wrong. SPI generic layer is well .... > generic. For example the platform I use is the S3C24xx which has a SPI > shift register depth of just one byte. And using DMA is not an option > because the setup of a DMA transfer takes tens of microseconds. > Additionally the SPI driver in mainline is based on bit-banging which > uses workqueues and so is limited by the fact that it fights with > other sched_other processes. Unfortunately an alternative driver which > was posted never got mainlined and we, who need it, have to maintain > it ourself. But this is another story, the bottom line is that we have > to cope with the most basic SPI support we can meet. I'm no your side suggesting not to use DMA for 3110/3100, but David and others have a valid point, we don't know what kind of SPI controller that this driver will work with, and the driver has to be as general and compatible as possible. > > .... this is exactly the problem I was facing: loosing chars on RX. I > tracked the problem to a too long latency in awakening of the > workqueue (the RX FIFO of the MAX3100 overflows). I will post shortly > a patch that moves this to threaded interrupts which solved the > problem on the S3C platform I mentioned above working at 115200. A > quick and dirty fix for workqueues is posted here: > http://www.evolware.org/chri/paciugo/index.html. Anyway it has to do > with scheduling of processes so many more factors are concerned, like > HZ and preemption being enabled. > > I will post some tests together with the patch, if you have some > benchmarks you like to be run don't hesitate to send them. Thanks in > advance! heh, I don't have benchmarks but simply the copy/paste stuff and using zmodem sometimes. > > >> I don't think this is a good idea to change speed. Just check the T > >> bit to see when the TX buffer is empty and send at the maximum > >> available speed. So the usage of the SPI bus is better. > > Yes, checking T bit is a good idea for single word transfer model, > > but it doesn't help when using buffer read/write > > I really don't understand why, can you elaborate on this? Checking T bit has to be done in word level, send one word out, read one back and check T bit inside one spi xfer. Why not use the buffer model, the HW has a 8 words RX buffer, why not read 8 words back in one spi xfer, saving a lot of system cost. And if we make the spi clk slightly slower than the baud rate, there will be no TX overflow problem. > > > I didn't use kmalloc/kfree in my earlier submission, but other > > developers mentioned the buffer allocated here need be DMA safe, as > > 3110 may work with many kinds of SPI controller, some of them only > > supports DMA mode. > > > > why you don't preallocate buffer for SPI transfers that are DMA-safe? > For example the mcp251x driver > (http://lxr.free-electrons.com/source/drivers/net/can/mcp251x.c?v=2.6.34-rc1) > does this. Some function will be called in several places, say re-entered, hard to use a preallocate buffer. > > >> I guess you are using mthread_up to avoid awakening the main thread > >> too many times. But this makes sense? Anyway because the awakening > >> and the setting of the bit is not atomic it won't work anyway. > > Why this can't work? the test/set_bit are atomic operations. This > > bit check is not exclusive, it just try to save some waking up, > > though waking a already running thread doesn't cost too much as > > mentioned by Andrew > > First of all I think that the cost of re-awakening an already running > thread is lower than the logic of testing bits. My note was just > nit-picking: there is a window between when the task was woken-up and > where you set the bit, so it doesn't guarantee you that you aren't > awakening the thread more than once anyway. OK, got your point now. test/set_bit's cost depends on the platform, and my simple test using perf tool showing it cost 10us level to wake a running thread. Anyway, re-wake it does no harm :) Thanks, Feng -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
