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

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

[Erwin Authried]

Am Donnerstag, den 04.03.2010, 15:25 +0800 schrieb Feng Tang:
> Hi All,
> 
> Here is a driver for Maxim 3110 SPI-UART device, please help to review.
> 
> It has been validated with Designware SPI controller (drivers/spi: dw_spi.c &
> dw_spi_pci.c). It supports polling and IRQ mode, supports batch read, and
> provides a console.
> 
> change log:
>  since v5:
>         * Make the spi data buffer DMA safe, thanks to Mssakazu Mokuno, 
>           David Brownell and Grant Likely for pointing the bug out
>  since v4:
>         * Add explanation why not using DMA
>         * Fix a bug in max3110_read_multi()
>  since v3:
>         * Remove the Designware controller related stuff
>         * Remove some initialization code which should be done in the platform
>           setup code
>         * Add a missing change for drivers/serial/Makefile
>  since v2:
>         * Address comments from Andrew Morton
>         * Use test/set_bit to avoid race condition for SMP/preempt case
>         * Fix bug related with endian order
>  since v1:
>         * Address comments from Alan Cox
>         * Use a "use_irq" module parameter to runtime check whether
>           to use IRQ mode
>         * Add the suspend/resume implementation
> 
> Thanks!
> Feng

Hi,

the only thing that I still don't like is the way of sending characters.
Basically, the spi rate is set to the actual baud rate when characters
are transmitted to the uart, and multiple characters are sent without
looking at the status of the uart.

The spi rate and the uart clock aren't synchronized, what happens if the
spi rate is slightly higher than the MAX3100's baud rate clock?
In addition, if there are other devices on the spi bus, the bus will be
occupied unnecessarily long, especially when low baudrates are used.

One other small cosmetic thing:
in serial_m3110_tx_empty(), TIOCSER_TEMT should be returned instead of
1.

-Erwin

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

[Feng Tang]

On Fri, 5 Mar 2010 15:44:50 +0800
Erwin Authried <eauth@softsys.co.at> wrote:

> 
> The spi rate and the uart clock aren't synchronized, what happens if
> the spi rate is slightly higher than the MAX3100's baud rate clock?
> In addition, if there are other devices on the spi bus, the bus will
> be occupied unnecessarily long, especially when low baudrates are
> used.
Hi Erwin,

Yep, here you touched a key point of the driver.

In the early version of our driver, we used a fixed spi rate which is a
little owner than its own working rate (3.684MHz) as a spi controller can
hardly get a divided rate exactly same as 3110's clock , with that we can only
handle one word per spi transfer, and have to add a udelay function for each
transfer, and we even have to calculate the delay time for all kinds of the
UART baud rate it's working with. struct spi_transfer has a member "deay_usecs"
just for this case.

One key point for SPI devices is they can't be accessed by CPU directly and has
to go though tasklet or workqueue for each spi transfer. To improve performance,
we chose to use buffer read/write to let one transfer contain more data, and use
dynamic spi rate by setting "spi_transfer->speed_hz" for each baud rate
accordingly. spi rate is set by spi controller drivers based on the "speed_hz"
number and they should chose a lower spi rate than 3110's baud rate if can't find
a same rate. For a spi controller support multiple devices including 3110, the
delay from 3110 is inevitable no matter we use a explicit udelay or the dynamic
spi rate.

Till here, I have to say the "spi_transfer" of spi core is well designed, which
provides bits_per_word/delay_usecs/speed_hz of great flexibility for developing
spi device/controller drivers

Thanks,
Feng


> 
> One other small cosmetic thing:
> in serial_m3110_tx_empty(), TIOCSER_TEMT should be returned instead of
> 1.
> 
> -Erwin
> 
> 
> 

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

[Grant Likely]

On Sun, Mar 7, 2010 at 7:11 PM, Feng Tang <feng.tang@intel.com> wrote:
> On Fri, 5 Mar 2010 15:44:50 +0800
> Erwin Authried <eauth@softsys.co.at> wrote:
>
>>
>> The spi rate and the uart clock aren't synchronized, what happens if
>> the spi rate is slightly higher than the MAX3100's baud rate clock?
>> In addition, if there are other devices on the spi bus, the bus will
>> be occupied unnecessarily long, especially when low baudrates are
>> used.
> Hi Erwin,
>
> Yep, here you touched a key point of the driver.
>
> In the early version of our driver, we used a fixed spi rate which is a
> little owner than its own working rate (3.684MHz) as a spi controller can
> hardly get a divided rate exactly same as 3110's clock , with that we can only
> handle one word per spi transfer, and have to add a udelay function for each
> transfer, and we even have to calculate the delay time for all kinds of the
> UART baud rate it's working with. struct spi_transfer has a member "deay_usecs"
> just for this case.
>
> One key point for SPI devices is they can't be accessed by CPU directly and has
> to go though tasklet or workqueue for each spi transfer. To improve performance,
> we chose to use buffer read/write to let one transfer contain more data, and use
> dynamic spi rate by setting "spi_transfer->speed_hz" for each baud rate
> accordingly. spi rate is set by spi controller drivers based on the "speed_hz"
> number and they should chose a lower spi rate than 3110's baud rate if can't find
> a same rate. For a spi controller support multiple devices including 3110, the
> delay from 3110 is inevitable no matter we use a explicit udelay or the dynamic
> spi rate.
>
> Till here, I have to say the "spi_transfer" of spi core is well designed, which
> provides bits_per_word/delay_usecs/speed_hz of great flexibility for developing
> spi device/controller drivers

You're abusing the speed_hz setting of the SPI controller.  By
clocking down the SPI bus, you're effectively using the SPI completion
interrupt as a high resolution timer to schedule your character
transmissions so that you don't overrun the max3110 which has no tx
fifo.  Using a udelay is also the wrong thing to do because it chews
up CPU cycles to achieve the same result.

The correct thing to do is to either use a high resolution timer, or
the IRQ from the MAX3110 itself to schedule your spi transfers, and
set the SPI rate to the highest frequency that the MAX3110 can handle
so that your driver neither hogs the SPI bus, nor wastes CPU cycles.
You can improve the performance of the driver by using spi_async()
instead of spi_sync() it enqueue the transfers so that you can handle
everything at IRQ context and you don't need to drop into a workqueue
to process the data.

g.

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

[christian pellegrin]

On Thu, Mar 4, 2010 at 8:22 AM,
<spi-devel-general-request-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> wrote:

>
> Hi All,
>

Hi, first of all let me apologize that I haven't see your previous
versions of the driver. I completely missed them. I am the author of
the max3100 driver. As far as I can see the max3110 is just a max3100
with the TTL to EIA level converter. So, first of all, may I ask why
you did start a new driver from scratch? I'm quite happy with the
current one, the only problem I see is that it uses worqueues which
are scheduled like normal processes and so under load their latency is
*big*. I wanted to move to threaded interrupts (otherwise you have to
use some ugly tricks to give workqueue threads real-time scheduling
class) but was waiting for my beta testers^H^H^H^H^H^H^H^H^H^H^H^H^H
customers to move to modern kernel that support them. Anyway the patch
is trivial and I can provide it if there is some interest. A quick
glance at your patch shows you are using the same sched_other threads
so you will face the same problems. I saw you added console capability
which isn't present in current driver, but it's easy to add (I didn't
implement it initially because I don't see a good idea using the
max3100 for initial bring-up of an embedded system because it relies
on the SPI subsystem, worqueues, scheduler and such complicated
things). Anyway keep in mind that the MAX3100 is rather low-end UART
so don't push it too much.

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.

> + * 1. From Max3110 spec, the Rx FIFO has 8 words, while the Tx FIFO only has
> + *    1 word. If SPI master controller doesn't support sclk frequency change,
> + *    then the char need be sent out one by one with some delay

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.

> + *
> + * 2. Currently only RX availabe interrrupt is used

yeah, this is one of the reasons why MAX31x0 sucks: when you change
configuration of interrupts the  RX buffer is cleared. So you cannot
activate/deactivate TX empty interrupt as needed.

> +static int use_irq = 1;
> +module_param(use_irq, int, 0444);
> +MODULE_PARM_DESC(use_irq, "Whether using Max3110's IRQ capability");

I don't think it's a good idea using an UART in polling mode, it just
kills the system. Just pretend this is not possible otherwise some
hardware guys will pester us to do this just to save an electric wire.

> +       memset(&x, 0, sizeof x);
> +       x.len = len;
> +       x.tx_buf = txbuf;
> +       x.rx_buf = rxbuf;

why don't initialize this like:

        struct spi_transfer x = {
                .tx_buf = txbuf,
                .rx_buf = rxbuf,
                .len = len,
        };


> +       buf = kmalloc(8, GFP_KERNEL | GFP_DMA);

you do kmalloc and kfree in routines that are called in quite tight
loops: I guess it's an overkill for performance.

> +static unsigned int serial_m3110_tx_empty(struct uart_port *port)
> +{
> +       return 1;
> +}
> +
> +static void serial_m3110_stop_tx(struct uart_port *port)
> +{
> +       return;
> +}
> +
> +static void serial_m3110_stop_rx(struct uart_port *port)
> +{
> +       return;
> +}

are you sure it's sane to just ignore these from higher level?

> +       u8 recv_buf[512], *pbuf;

is this sane to allocate 512 byte on the stack?

> +               wait_event_interruptible(*wq,
> +                               max->flags || kthread_should_stop());
> +               test_and_set_bit(0, &max->mthread_up);

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.

> +/* Don't handle hw handshaking */
> +static unsigned int serial_m3110_get_mctrl(struct uart_port *port)
> +{
> +       return TIOCM_DSR | TIOCM_CAR | TIOCM_DSR;
> +}
> +
> +static void serial_m3110_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{
> +}
> +

this is quite bad because the MAX3100 is rather slow.

Bye!

-- 
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."

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

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

[Feng Tang]

Hi Christian,

Thanks for taking time to review the driver. Please see my answers inline

- Feng
On Wed, 17 Mar 2010 01:22:46 +0800
christian pellegrin <chripell@gmail.com> wrote:

> On Thu, Mar 4, 2010 at 8:22 AM,
> <spi-devel-general-request@lists.sourceforge.net> wrote:
> 
> >
> > Hi All,
> >
> 
> Hi, first of all let me apologize that I haven't see your previous
> versions of the driver. I completely missed them. I am the author of
> the max3100 driver. As far as I can see the max3110 is just a max3100
> with the TTL to EIA level converter. So, first of all, may I ask why
> you did start a new driver from scratch? I'm quite happy with the
I don't want to reinvent the wheel, as I have many more tasks to handle :)
My 3110 driver started to work in 2008(surely very basic at that time) before
your driver is posted on public. And the reasons I still posted driver here are:
1. It provides a console
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 

If we can cooperate to make current 3100 driver meet our expectation, I'm more
than happy to drop my 3110 one. I'd rather submit some bug fix for it than
posting 6 or more versions of code :)

> current one, the only problem I see is that it uses worqueues which
> are scheduled like normal processes and so under load their latency is
> *big*. I wanted to move to threaded interrupts (otherwise you have to
> use some ugly tricks to give workqueue threads real-time scheduling
> class) but was waiting for my beta testers^H^H^H^H^H^H^H^H^H^H^H^H^H
> customers to move to modern kernel that support them. Anyway the patch
> is trivial and I can provide it if there is some interest. A quick
> glance at your patch shows you are using the same sched_other threads
> so you will face the same problems. I saw you added console capability
> which isn't present in current driver, but it's easy to add (I didn't
> implement it initially because I don't see a good idea using the
> max3100 for initial bring-up of an embedded system because it relies
> on the SPI subsystem, worqueues, scheduler and such complicated
> things). Anyway keep in mind that the MAX3100 is rather low-end UART
> so don't push it too much.
It's not me push it too much, but I was pushed too much :)
We can't make the decision how the HW is used on all kinds of platforms. For
ours, we heavily rely on it as a sole console, and basic requirement is:
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.

For the kernel early boot phase debug, I actually had another 3110
early_printk patch which can work right after the kernel starts

> 
> 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.

> 
> > + * 1. From Max3110 spec, the Rx FIFO has 8 words, while the Tx
> > FIFO only has
> > + *    1 word. If SPI master controller doesn't support sclk
> > frequency change,
> > + *    then the char need be sent out one by one with some delay
> 
> 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

> 
> > + *
> > + * 2. Currently only RX availabe interrrupt is used
> 
> yeah, this is one of the reasons why MAX31x0 sucks: when you change
> configuration of interrupts the  RX buffer is cleared. So you cannot
> activate/deactivate TX empty interrupt as needed.
> 
> > +static int use_irq = 1;
> > +module_param(use_irq, int, 0444);
> > +MODULE_PARM_DESC(use_irq, "Whether using Max3110's IRQ
> > capability");
> 
> I don't think it's a good idea using an UART in polling mode, it just
> kills the system. Just pretend this is not possible otherwise some
> hardware guys will pester us to do this just to save an electric wire.
Yes, like Grant and others have mentioned, I plan to use spi->irq as the
condition, platform code which initialize the board_info should clear the
irq to 0 if they won't use IRQ for 3110

	if (spi->irq)
		request_irq(spi->irq,...);
	else
		start_read_thread;

> 
> 
> > +       buf = kmalloc(8, GFP_KERNEL | GFP_DMA);
> 
> you do kmalloc and kfree in routines that are called in quite tight
> loops: I guess it's an overkill for performance.

Overkill is just the word I have used when replying comments on my
earlier version :)

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.

> 
> are you sure it's sane to just ignore these from higher level?
> 
> > +       u8 recv_buf[512], *pbuf;
> 
> is this sane to allocate 512 byte on the stack?

The function is called only in driver's own main thread and reader thread
context, 512 should be safe enough. Also the reason I don't use
kzalloc/kfree is the same as you mentioned: performance

> 
> > +               wait_event_interruptible(*wq,
> > +                               max->flags ||
> > kthread_should_stop());
> > +               test_and_set_bit(0, &max->mthread_up);
> 
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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 .....

> 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."

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

[Erwin Authried]

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

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

[Feng Tang]

On Wed, 17 Mar 2010 15:39:23 +0800
christian pellegrin <chripell@gmail.com> wrote:

> 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 ;-)

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