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 feng.t...@intel.com 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 a...@linux.intel.com -- 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 ___ 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 a...@lxorguk.ukuu.org.uk wrote: On Wed, 24 Feb 2010 13:11:30 +0800 Feng Tang feng.t...@intel.com 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 a...@linux.intel.com Ack for the SPI side Acked-by: Grant Likely grant.lik...@secretlab.ca g. -- 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 ___ 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 esch...@online.de 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 grant.lik...@secretlab.ca 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#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 ___ 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 feng.t...@intel.com 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 feng.t...@intel.com 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#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 ___ 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 a...@linux-foundation.org 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#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 ___ 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#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 ___ 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#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 ___ 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 davi...@pacbell.net 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#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 ___ 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 Thu, 25 Feb 2010 12:47:13 +0800 David Brownell davi...@pacbell.net 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#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 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general