Re: [spi-devel-general] [PATCH v5] serial: spi: add spi-uart driver for Maxim 3110
On Tuesday 02 March 2010, Feng Tang wrote: + * 3. This driver doesn't support work with a spi cotnroller in DMA mode, As Grant said: That's a bug ... one that will randomly kick in based on whether the underlying SPI controller driver happens to use DMA for a given transaction. -- 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 v5] serial: spi: add spi-uart driver for Maxim 3110
On Wed, 3 Mar 2010 12:51:58 +0800 David Brownell davi...@pacbell.net wrote: On Tuesday 02 March 2010, Feng Tang wrote: + * 3. This driver doesn't support work with a spi cotnroller in DMA mode, As Grant said: That's a bug ... one that will randomly kick in based on whether the underlying SPI controller driver happens to use DMA for a given transaction. From this perspective, yes, it's a bug that this driver use non DMA-safe IO buffer, which prevents it to be portable for all kinds of controllers, and I can make it DMA safe. But I still don't think it's a good idea to use DMA for Max3110 for performance reason, I know there is some spi controller who only works in DMA mode. Here comes another idea, can we add a capability flag in struct spi_master indicating the master supporting poll or dma or both. Also we add similar bits in struct spi_transfer indicating the this transfer wants to be handled in poll or dma mode. For spi controller driver, it can claim its capability when registering as a spi_master, for a spi device driver, it can select the working mode based on spi_master's capability and set the working mode in struct spi_transfer. Then controller and device don't need to guess what the other is capable, and make the info public. Any thoughts? 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
Re: [spi-devel-general] [PATCH v5] serial: spi: add spi-uart driver for Maxim 3110
On Tuesday 02 March 2010, Feng Tang wrote: On Wed, 3 Mar 2010 12:51:58 +0800 David Brownell davi...@pacbell.net wrote: On Tuesday 02 March 2010, Feng Tang wrote: + * 3. This driver doesn't support work with a spi cotnroller in DMA mode, As Grant said: That's a bug ... one that will randomly kick in based on whether the underlying SPI controller driver happens to use DMA for a given transaction. From this perspective, yes, it's a bug that this driver use non DMA-safe IO buffer, which prevents it to be portable for all kinds of controllers, Yes, that's a bug ... you're using a programming interface which has portability as a core goal. In an operating system kernel which also has such a goal. and I can make it DMA safe. That's the right solution. Here comes another idea, can we add a capability flag in struct spi_master indicating the master supporting poll or dma or both. Also we add similar bits in struct spi_transfer indicating the this transfer wants to be handled in poll or dma mode. Let's not do either of those. There's no need to introduce such complexity, or to enable the associated new failure modes and bugs. Much simpler to just use DMA-safe I/O buffers in the first place ... which is what pretty much every driver stack in Linux already expects or requires. - 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] [PATCH v5] serial: spi: add spi-uart driver for Maxim 3110
Here comes another idea, can we add a capability flag in struct spi_master indicating the master supporting poll or dma or both. Also we add similar bits in struct spi_transfer indicating the this transfer wants to be handled in poll or dma mode. Let's not do either of those. There's no need to introduce such complexity, or to enable the associated new failure modes and bugs. This idea has nothing to do with the dma-safe problem you pointed out, I will make the buffer dma safe anyway. What I proposed just wants to provide some flexibility for protocol device drivers, it will use dma-safe buffer always, but it prefer not to use DMA for its one-word transfers, and hope to have a choice to do that. For current existing controller and device drivers, they can simply ignore the new working mode setting in struct spi_master and spi_transfer and leave them as 0. Thanks, Feng Much simpler to just use DMA-safe I/O buffers in the first place ... which is what pretty much every driver stack in Linux already expects or requires. - 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] [PATCH v5] serial: spi: add spi-uart driver for Maxim 3110
On Tue, Mar 2, 2010 at 7:57 PM, 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 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 [...] + * + * 3. This driver doesn't support work with a spi cotnroller in DMA mode, and + * the reason for not using DMA is: + * a) Maxim 3110 is a low speed UART device, whose tx/rx buffer are very few, + * and using DMA may be over-killing when working as a system console(imaging + * we edit a text file on it) + * b) Handling only one 16b word transfer will be very common, but non-32b + * aligned DMA operation is not supported by all kinds of DMA controllers + * + * Some spi controller drivers like Pxa2xx, Designware have option for device + * driver to chose to work in poll or DMA mode. And platform driver which + * enumerates Max3110 device should leverage this option to not use DMA. Between this, reading through the other comments, and the existence of the max3100 driver, I'm less and less comfortable with this driver. First, if this driver gets merged, then it is quite likely that neither you or the 3100 author will get around to merging them. Second, as others have pointed out, this driver is making assumptions about the SPI bus that it has no business making. The fact that it doesn't work with DMA isn't a design choice, it is a bug and whether or not it is a low speed uart device is completely beside the point. I'm generally not involved with the serial device drivers, but FWIW, I'm no longer in favor of this driver getting merged. If you really want to get it merged, then I recommend asking Greg to pick it up into staging so that there is some imperative to fix it up and merge it with the max3100 driver. 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 v5] serial: spi: add spi-uart driver for Maxim 3110
On Tuesday 02 March 2010, Feng Tang wrote: Here comes another idea, can we add a capability flag in struct spi_master indicating the master supporting poll or dma or both. Also we add similar bits in struct spi_transfer indicating the this transfer wants to be handled in poll or dma mode. Let's not do either of those. There's no need to introduce such complexity, or to enable the associated new failure modes and bugs. This idea has nothing to do with the dma-safe problem you pointed out, Your comment has nothing to do with my response. Are you implying that your suggestions do *NOT* introduce complexity, including new failure modes and thus the possibility of new confusing types of bugs? I will make the buffer dma safe anyway. Good ... What I proposed just wants to provide some flexibility for protocol device drivers, it will use dma-safe buffer always, but it prefer not to use DMA for its one-word transfers, That kind of heuristic is something the SPI controller driver could already apply if it's a good tradeoff on that hardware. (Considering also the extra added complexity, failure modes, and potential for new flavors of bug...) Of course, drivers like the max3110 are high enough in the driver stack that they can only guess about such tradeoffs. (And thus most drivers will likely guess wrong...) and hope to have a choice to do that. For current existing controller and device drivers, they can simply ignore the new working mode setting in struct spi_master and spi_transfer and leave them as 0. If someone decided to update a SPI controller driver to avoid DMA in some cases, in favor of PIO, they could already code such heuristics without needing your proposed hinting from upper layers. The result in the low-level driver would be just to use a different test (maybe is this a one-word transfer? instead of checking your per-transfer use PIO? hint) before kicking in whatever logic you think would improve performance (by eliminating DMA setup and teardown costs, including cache cleaning). - 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
