Re: [spi-devel-general] [RFC][PATCH] serial: spi: add spi-uart driver for Maxim 3110
On Thu, 25 Feb 2010 12:47:13 +0800 David Brownell wrote: > On Tuesday 29 December 2009, Erwin Authried wrote: > > I think there's no need for a MAX3100 **and** a MAX3110 driver, > > this is just confusing. The MAX3110 driver is identical to the > > MAX3100 from the software view, it is simply a MAX3100 with > > transceivers added to the chip. If there's any improvement, that > > should be merged into the existing MAX3100 driver. > > Assuming that's true ... who will resolve the issue? > Hi David, I've answered Erwin's comments before in v1 submission cycle, following is the quote: "I agree there should not be 2 drivers cover 1 family of HW, so this is a RFC. I've thinked about merge with current 3100 code, but it depends on one char per spi_transfer, while my driver relys on batch data transfer for efficiency. Another key point is the console, SPI UART can't be directly accessed by CPU, so every spi_transfer will go through tasklet/workqueue, which makes supporting printk a big part of my driver." I really did consider about that, but has no good clue, so I think better to shape my driver first. Thanks, Feng -- Download Intel® 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 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [RFC][PATCH v3] serial: spi: add spi-uart driver for Maxim 3110
On Thu, 25 Feb 2010 12:43:14 +0800
David Brownell wrote:
> On Tuesday 23 February 2010, Feng Tang wrote:
> >
> > +config SERIAL_MAX3110
> > + tristate "SPI UART driver for Max3110"
> > + select SERIAL_CORE
> > + select SERIAL_CORE_CONSOLE
>
> Shouldn't that depend on SPI_MASTER? As it stands, you're
> permitting it to build on systems that you *know* don't have
> a prayer of running this code ... or often, even finishing
> the build.
>
>
> > + help
> > + This is the UART protocol driver for MAX3110 device
> > +
> > +config MAX3110_DESIGNWARE
>
> Having this depend on a specific underlying SPI master controller
> is not a good thing. It should instead be written so that it
> runs correctly on *any* controller which exports the standard SPI
> programming interface.
>
>
>
> > + boolean "Enable Max3110 to work with Designware controller"
> > + default y
> > + depends on SERIAL_MAX3110
> > +
>
> That is, stuff like this, from your max3110 driver:
>
> > +
> > +#ifdef CONFIG_MAX3110_DESIGNWARE
> > +static struct dw_spi_chip spi_uart = {
> > + .poll_mode = 1,
> > + .enable_dma = 0,
> > + .type = SPI_FRF_SPI,
> > +};
> > +#endif
>
> Is completely irrelevant for other hardware ... or else
> (as with DMA, which should be "enabled by default") is
> relevant, but shouldn't be parameterized.
>
>
> > + spi->mode = SPI_MODE_0;
> > + spi->bits_per_word = 16;
> > +#ifdef CONFIG_MAX3110_DESIGNWARE
> > + spi->controller_data = &spi_uart;
> > +#endif
>
> That all looks fishy too. SPI_MODE should have
> been set up as part of device creation. Ditto
> any spi->controller_data ... normally, that all
> gets set up as part of board-specific setup.
>
> Normally one goes to a lot of effort to keep
> such board-specific code out of drivers.
Good point about the DW controller specific data, I'll remove them.
For those "bits_per_word" setting, I think we can put it here instead of
the board initialization code, as many types of boards can leverage the
setting here as it only works in 16b mode.
Thanks,
Feng
>
> - Dave
>
--
Download Intel® 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
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [RFC][PATCH] serial: spi: add spi-uart driver for Maxim 3110
On Tuesday 29 December 2009, Erwin Authried wrote: > I think there's no need for a MAX3100 **and** a MAX3110 driver, this is > just confusing. The MAX3110 driver is identical to the MAX3100 from the > software view, it is simply a MAX3100 with transceivers added to the > chip. If there's any improvement, that should be merged into the > existing MAX3100 driver. Assuming that's true ... who will resolve the issue? -- Download Intel® 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 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [RFC][PATCH v3] serial: spi: add spi-uart driver for Maxim 3110
On Tuesday 23 February 2010, Feng Tang wrote:
>
> +config SERIAL_MAX3110
> + tristate "SPI UART driver for Max3110"
> + select SERIAL_CORE
> + select SERIAL_CORE_CONSOLE
Shouldn't that depend on SPI_MASTER? As it stands, you're
permitting it to build on systems that you *know* don't have
a prayer of running this code ... or often, even finishing
the build.
> + help
> + This is the UART protocol driver for MAX3110 device
> +
> +config MAX3110_DESIGNWARE
Having this depend on a specific underlying SPI master controller
is not a good thing. It should instead be written so that it
runs correctly on *any* controller which exports the standard SPI
programming interface.
> + boolean "Enable Max3110 to work with Designware controller"
> + default y
> + depends on SERIAL_MAX3110
> +
That is, stuff like this, from your max3110 driver:
> +
> +#ifdef CONFIG_MAX3110_DESIGNWARE
> +static struct dw_spi_chip spi_uart = {
> + .poll_mode = 1,
> + .enable_dma = 0,
> + .type = SPI_FRF_SPI,
> +};
> +#endif
Is completely irrelevant for other hardware ... or else
(as with DMA, which should be "enabled by default") is
relevant, but shouldn't be parameterized.
> + spi->mode = SPI_MODE_0;
> + spi->bits_per_word = 16;
> +#ifdef CONFIG_MAX3110_DESIGNWARE
> + spi->controller_data = &spi_uart;
> +#endif
That all looks fishy too. SPI_MODE should have
been set up as part of device creation. Ditto
any spi->controller_data ... normally, that all
gets set up as part of board-specific setup.
Normally one goes to a lot of effort to keep
such board-specific code out of drivers.
- Dave
--
Download Intel® 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
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [RFC][PATCH v3] serial: spi: add spi-uart driver for Maxim 3110
On Thu, 25 Feb 2010 07:18:32 +0800
Andrew Morton wrote:
> > ...
> >
> > +static int max3110_main_thread(void *_max)
> > +{
> > + struct uart_max3110 *max = _max;
> > + wait_queue_head_t *wq = &max->wq;
> > + int ret = 0;
> > + struct circ_buf *xmit = &max->con_xmit;
> > +
> > + init_waitqueue_head(wq);
> > + pr_info(PR_FMT "start main thread\n");
> > +
> > + do {
> > + wait_event_interruptible(*wq,
> > + max->flags ||
> > kthread_should_stop());
> > + set_bit(0, &max->mthread_up);
> > +
> > + if (use_irq && test_bit(M3110_IRQ_PENDING,
> > &max->flags)) {
> > + max3110_con_receive(max);
> > + clear_bit(M3110_IRQ_PENDING, &max->flags);
> > + }
> > +
> > + /* First handle console output */
> > + if (test_bit(M3110_CON_TX_NEED, &max->flags)) {
> > + send_circ_buf(max, xmit);
> > + clear_bit(M3110_CON_TX_NEED, &max->flags);
> > + }
> > +
> > + /* Handle uart output */
> > + if (test_bit(M3110_UART_TX_NEED, &max->flags)) {
> > + transmit_char(max);
> > + clear_bit(M3110_UART_TX_NEED, &max->flags);
> > + }
> > + clear_bit(0, &max->mthread_up);
> > + } while (!kthread_should_stop());
> > +
> > + return ret;
> > +}
> > +
> > +static irqreturn_t serial_m3110_irq(int irq, void *dev_id)
> > +{
> > + struct uart_max3110 *max = dev_id;
> > +
> > + /* max3110's irq is a falling edge, not level triggered,
> > +* so no need to disable the irq */
> > + set_bit(M3110_IRQ_PENDING, &max->flags);
> > +
> > + if (!test_bit(0, &max->mthread_up))
> > + wake_up_process(max->main_thread);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> The manipulation of mthread_up here (and in several other places) is
> clearly quite racy. If you hit that race, the thread will not wake up
> and the driver will sit there not doing anything until some other
> event happens.
>
> This is perhaps fixable with test_and_set_bit() and
> test_and_clear_bit() (need to think about that) or, of course, by
> adding locking.
>
> But a simpler fix is just to delete mthread_up altogether.
> wake_up_process() on a running process is an OK thing to do and
> hopefully isn't terribly slow.
Yes, wake_up_process won't harm a running process, our driver's case
is a little special, the console's write() func may call
wake_up_process() for every character in the buffer, thus we will
try to avoid to call it. mthread_up can't be removed as it is also
referenced in read_thread.
I prefer to use the test_and_set/clear_bit for "mthread_up".
Thanks,
Feng
diff --git a/drivers/serial/max3110.c b/drivers/serial/max3110.c
index e8c44fa..d5bd71f 100644
--- a/drivers/serial/max3110.c
+++ b/drivers/serial/max3110.c
@@ -400,7 +400,7 @@ static int max3110_main_thread(void *_max)
do {
wait_event_interruptible(*wq,
max->flags || kthread_should_stop());
- set_bit(0, &max->mthread_up);
+ test_and_set_bit(0, &max->mthread_up);
if (use_irq && test_bit(M3110_IRQ_PENDING, &max->flags)) {
max3110_con_receive(max);
@@ -418,7 +418,7 @@ static int max3110_main_thread(void *_max)
transmit_char(max);
clear_bit(M3110_UART_TX_NEED, &max->flags);
}
- clear_bit(0, &max->mthread_up);
+ test_and_clear_bit(0, &max->mthread_up);
} while (!kthread_should_stop());
return ret;
--
Download Intel® 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
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [RFC][PATCH v3] serial: spi: add spi-uart driver for Maxim 3110
On Wed, 24 Feb 2010 13:11:30 +0800 Feng Tang wrote:
> 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 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
>
> >From 93455ea6fbf0bfb6494b88ea83296f26f29f7eee Mon Sep 17 00:00:00 2001
> From: Feng Tang
> Date: Tue, 23 Feb 2010 17:52:00 +0800
> Subject: [PATCH] serial: spi: add spi-uart driver for Maxim 3110
>
> This is the driver for Max3110 SPI-UART device, which connect
> to host with SPI interface. It supports baud rates from 300 to
> 230400, and supports both polling and IRQ mode, as well as
> providing a console for system use
>
> Its datasheet could be found here:
> http://datasheets.maxim-ic.com/en/ds/MAX3110E-MAX3111E.pdf
>
> ...
>
> +static int max3110_main_thread(void *_max)
> +{
> + struct uart_max3110 *max = _max;
> + wait_queue_head_t *wq = &max->wq;
> + int ret = 0;
> + struct circ_buf *xmit = &max->con_xmit;
> +
> + init_waitqueue_head(wq);
> + pr_info(PR_FMT "start main thread\n");
> +
> + do {
> + wait_event_interruptible(*wq,
> + max->flags || kthread_should_stop());
> + set_bit(0, &max->mthread_up);
> +
> + if (use_irq && test_bit(M3110_IRQ_PENDING, &max->flags)) {
> + max3110_con_receive(max);
> + clear_bit(M3110_IRQ_PENDING, &max->flags);
> + }
> +
> + /* First handle console output */
> + if (test_bit(M3110_CON_TX_NEED, &max->flags)) {
> + send_circ_buf(max, xmit);
> + clear_bit(M3110_CON_TX_NEED, &max->flags);
> + }
> +
> + /* Handle uart output */
> + if (test_bit(M3110_UART_TX_NEED, &max->flags)) {
> + transmit_char(max);
> + clear_bit(M3110_UART_TX_NEED, &max->flags);
> + }
> + clear_bit(0, &max->mthread_up);
> + } while (!kthread_should_stop());
> +
> + return ret;
> +}
> +
> +static irqreturn_t serial_m3110_irq(int irq, void *dev_id)
> +{
> + struct uart_max3110 *max = dev_id;
> +
> + /* max3110's irq is a falling edge, not level triggered,
> + * so no need to disable the irq */
> + set_bit(M3110_IRQ_PENDING, &max->flags);
> +
> + if (!test_bit(0, &max->mthread_up))
> + wake_up_process(max->main_thread);
> +
> + return IRQ_HANDLED;
> +}
The manipulation of mthread_up here (and in several other places) is
clearly quite racy. If you hit that race, the thread will not wake up
and the driver will sit there not doing anything until some other event
happens.
This is perhaps fixable with test_and_set_bit() and
test_and_clear_bit() (need to think about that) or, of course, by
adding locking.
But a simpler fix is just to delete mthread_up altogether. wake_up_process()
on a running process is an OK thing to do and hopefully isn't terribly slow.
> +/* If don't use RX IRQ, then need a thread to polling read */
> +static int max3110_read_thread(void *_max)
> +{
> + struct uart_max3110 *max = _max;
> +
> + pr_info(PR_FMT "start read thread\n");
> + do {
> + if (!test_bit(0, &max->mthread_up))
> + max3110_con_receive(max);
> +
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout_interruptible(HZ / 20);
The set_current_state(TASK_INTERRUPTIBLE) is unneeded.
> + } while (!kthread_should_stop());
> +
> + return 0;
> +}
--
Download Intel® 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
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [PATCH 1/2] spi/mmc_spi: SPI bus locking API, using mutex
On Wed, Feb 17, 2010 at 2:41 PM, Mike Frysinger wrote: > On Wed, Feb 17, 2010 at 16:07, Grant Likely wrote: >> Also, even if I agreed with the premise that a cookie is needed for >> deciding who can use the bus when locked, it is still a good idea to >> use a different API when working with a locked bus. Locking issues >> are subtle, and driver authors *must understand what they are doing*. >> Using a different API for talking to a locked bus is one of the things >> that makes absolute sense to make sure that drivers know which regions >> have the bus locked, and which do not. > > these sort of statements are always made yet it doesnt seem to prevent > bugs. of course people *should know what they're doing*, but > *reality* is that people make mistakes and bugs get merged all the > time. a little proactive protection goes a long way. i dont think > adding a cookie would increase overhead all that much. Heh. Alright. Then I think I'd like to see a follow-on patch that adds the cookie checking so that it can be reviewed independently of all the locking additions. Cheers, g. -- Download Intel® 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 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [PATCH 1/2 V2] spi/mmc_spi: SPI bus locking API, using mutex
Hey Ernst. Good patch, thank you. The merge window is going to open any moment now, and this patch hasn't had any linux-next exposure, so I'm not going to pick it up for 2.6.34. Instead, I'll wait until after the merge window closes and add it to my next branch so that it gets lots of exposure before the 2.6.35 merge window. Since this is a pretty core change, I'd like to have as much feedback and testing on it as possible before I commit to pushing it into mainline. Anyone who is following along and is interested, please test these two patches and let me know if they work for you. Ping me after Linus tags v2.6.34-rc1 to make sure I don't forget to put it in my -next branch. Some comments below... On Thu, Feb 18, 2010 at 5:10 AM, Ernst Schwab wrote: > I integrated your changes in V2, I hope I did not miss anything. > > On Wed, 17 Feb 2010 13:03:04 -0700 > Grant Likely wrote: >> > extern int spi_setup(struct spi_device *spi); >> > extern int spi_async(struct spi_device *spi, struct spi_message *message); >> > +extern int >> > +spi_async_locked(struct spi_device *spi, struct spi_message *message); >> >> Please keep return parameters and annotations on the same line as the >> function name (makes grep results more useful). You can break lines >> between parameters. > > Hm, the prototype for spi_alloc_master in the same file let me assume, > that this should be the style used in this header file. Normally, I > wouldn't do a linebreak there, ever... Yeah, things are inconsistent, but in this case both styles are present in the file, and when grepping I find it more important to have the annotations than the parameters immediately visable. > > Something more important: > > I have concerns on the use of the spin lock around the > __spi_async() call, as Mike Frysinger already remarked. > > Why do we 'need' this spin lock: > - by definition, spi_async can be called from any context > " > * Context: any (irqs may be blocked, etc) > * > * This call may be used in_irq and other contexts which can't sleep, > * as well as from task contexts which can sleep. > " > - we are not sure that spi_async isn't used reentrant (e.g. interrupt > service routine and normal context > - __spi_async calls the master->transfer() function which is not required > to be reentrant (or is it?) Um, I'm not sure I follow the argument. spin locks are safe to obtain in any context. Mutexes are the ones that cannot be held at atomic context. The spin lock does more than just protect the bus_is_locked flag. It also prevent overlap between process and IRQ context running in the relevant code. Basically the spin_lock_irqsave() either defers all IRQ processing in uniprocessor, or has other processors spin in SMP. I think the spinlock should always be held when calling the bus drivers ->transfer() hook. That will also allow us to remove driver-specific locking from the bus drivers. Otherwise we've got an inconsistent locking scheme which is asking for trouble down the road. > > But: This all is true for the original design, there seems to be no > protection against accidental reentrant use of spi_async(). spi_async() is intended to be callable from atomic context so that IRQ handers can trigger a new transfer without waiting around for it to complete. It is also totally valid for both process and IRQ contexts to call spi_async() at the same time, which is exactly why the spinlock should be required. Hmmm. Are you concerned about a code path where an spi_async() call will call the bus driver, which will end up calling spi_async() on itself? I cannot think of a use case where that would be desirable. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- Download Intel® 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 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [RFC][PATCH v3] serial: spi: add spi-uart driver for Maxim 3110
On Wed, Feb 24, 2010 at 3:44 AM, Alan Cox wrote: > On Wed, 24 Feb 2010 13:11:30 +0800 > Feng Tang wrote: > >> 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. > > Ack for the serial side > > Acked-by: Alan Cox Ack for the SPI side Acked-by: Grant Likely g. -- Download Intel® 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 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [RFC][PATCH v3] serial: spi: add spi-uart driver for Maxim 3110
On Wed, 24 Feb 2010 13:11:30 +0800 Feng Tang wrote: > 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. Ack for the serial side Acked-by: Alan Cox -- Download Intel® 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 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
