From: Grant Likely <grant.likely@secretlab.ca>
To: Feng Tang <feng.tang@intel.com>
Cc: Erwin Authried <eauth@softsys.co.at>,
Andrew Morton <akpm@linux-foundation.org>,
Greg KH <greg@kroah.com>, David Brownell <david-b@pacbell.net>,
Alan Cox <alan@lxorguk.ukuu.org.uk>,
spi-devel-list <spi-devel-general@lists.sourceforge.net>,
"linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>
Subject: Re: [spi-devel-general] [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110
Date: Sun, 7 Mar 2010 21:12:34 -0700 [thread overview]
Message-ID: <fa686aa41003072012q51c57f7fidbc1b62b91969832@mail.gmail.com> (raw)
In-Reply-To: <20100308101149.6e874718@feng-i7>
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.
next prev parent reply other threads:[~2010-03-08 4:12 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-03-04 7:25 [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110 Feng Tang
2010-03-04 18:46 ` Masakazu Mokuno
2010-03-05 3:48 ` Feng Tang
2010-03-05 7:44 ` [spi-devel-general] " Erwin Authried
2010-03-08 2:11 ` Feng Tang
2010-03-08 4:12 ` Grant Likely [this message]
[not found] ` <fa686aa41003072012q51c57f7fidbc1b62b91969832-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-03-11 9:07 ` Feng Tang
2010-03-08 4:30 ` Grant Likely
2010-03-11 8:32 ` Feng Tang
-- strict thread matches above, loose matches on Subject: below --
2010-03-16 17:22 christian pellegrin
2010-03-17 2:35 ` Feng Tang
2010-03-17 7:39 ` christian pellegrin
2010-03-17 8:17 ` [spi-devel-general] " Erwin Authried
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fa686aa41003072012q51c57f7fidbc1b62b91969832@mail.gmail.com \
--to=grant.likely@secretlab.ca \
--cc=akpm@linux-foundation.org \
--cc=alan@lxorguk.ukuu.org.uk \
--cc=david-b@pacbell.net \
--cc=eauth@softsys.co.at \
--cc=feng.tang@intel.com \
--cc=greg@kroah.com \
--cc=linux-serial@vger.kernel.org \
--cc=spi-devel-general@lists.sourceforge.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).