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 ;-) > 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. This said I tested the throughput of the driver by doing a zmodem transfer and, as far as I remember, the results were pretty good. Please note that the transmission is already buffered in higher layer. Don't be fooled by the fact that tx_empty is correctly implemented in the actual driver: you *have* to implement it otherwise some things like termios call tcdrain() will be broken. For example applications that do RS485 rx/tx switching in user-space will go nuts without it. As far receiving is concerned ..... > the datasheet say it supports 230400~300 baud rate, then driver guy need make > it work as it claims, like copying 500 bytes string somewhere and paste it on > this console in 230400/600 rate. .... 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! >> I'm more than happy to work with you to have just one perfect (or >> almost perfect) driver instead of two of them half-done. See some >> comments inline. > Can't agree more, all I want is just a upstream driver which meets the basic > requirement. > I think upstream drivers have to meet the requirement of the average user not be tailored to specific user-cases (see the Android code inclusion debate). For example changing SPI speed to meet some rate of data flow is wrong. Nothing will assure you that you get the rate you asked: it all depends of the master SPI clock and the granularity resulting by its division. So I still think the best way is poll the T bit by using the SPI bus as little as possible. >> 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? > 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. >> 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. Bye, PS.: sorry to have missed you previous emails where some points where already discussed. Please CC me next time, unfortunately spi-devel mailing list carries quite a bit of spam and so I have to search its post in the trash folder. I realized this is the reason why I didn't see your previous posts :-/ -- Christian Pellegrin, see http://www.evolware.org/chri/ "Real Programmers don't play tennis, or any other sport which requires you to change clothes. Mountain climbing is OK, and Real Programmers wear their climbing boots to work in case a mountain should suddenly spring up in the middle of the computer room." -- 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
