Re: [spi-devel-general] [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Erwin Authried <eauth@softsys.co.at>]

Am Mittwoch, den 17.03.2010, 08:39 +0100 schrieb christian pellegrin:
> On Wed, Mar 17, 2010 at 3:35 AM, Feng Tang <feng.tang@intel.com> 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 .....

Hi,

tx_empty isn't that easy to implement if there is no way to get the
tx-empty status from the uart, like with the MAX3100. There are several
serial drivers that simply return TIOCSER_TEMT. I know it's really bad
for applications like RS485...
For this reason, and due to the lack of large of tx/rx fifos in the
MAX3100, I'm using Atmel AVRs where you can easily implement larger
fifos and reliable HW flow control in the firmware. Besides, the cost is
just a fraction of the MAX3100. 

-Erwin
> 
> > 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 :-/
> 
-- 
Dipl.-Ing. Erwin Authried
Softwareentwicklung und Systemdesign