Thanks for the review. First I need to provide the context that why I changed the current working dma code for dw_spi driver: 1. The dw_spi driver in upstream only have PIO support, though dma code is written long time ago and verified with GTM501L spi modem. When we posted the dma patch to community several days ago, there is comments suggested to use the latest dma interfaces, but the problem is those dma changes were not even in Linus's tree or AC tree, but in only linux-next tree, including intel dma controller driver's change, so I have to generate the patch against linux-next tree to respond to community reviews quickly, even there is currently no place which put all these changes together.
2. Vinod sent out an email requested all MID drivers which consumer intel_mid_dma driver to update the dma interface, which is mainly about the new DMA_SLAVE_CONFIG interface. On Sat, 30 Oct 2010 02:35:53 +0800 "Koul, Vinod" <[email protected]> wrote: evice_pre > > > > Use dma_slave_config to dynamically set dma channel for each dma > > transaction, also use the dma device's device_prep_slave_sg() > > callback instead of the device_prep_dma_memcpy(). > I don't think the DMA controller you are using supports sg operation, > you will still need to call memcpy for single transfer. Emm, I'm not familiar with all the latest changes to upstream dma core, but in the sample code dma_test.c that you sent to me, it does use the device_prep_slave_sg() interface, in the slave_do_dma() function. And in linux-next tree, there is commit added devcie_prep_slave_sg: commit 576e3c394a6c427c9a1378ec88ef7eb97e731992 Author: Ramesh Babu K V <[email protected]> Date: Mon Oct 4 10:37:53 2010 +0000 intel_mid_dma: Add sg list support to DMA driver For a very high speed DMA various periphral devices need scatter-gather list support. The DMA hardware support link list items. This list can be circular also (adding new flag DMA_PREP_CIRCULAR_LIST) Right now this flag is in driver header and should be moved to dmaengine header file eventually Signed-off-by: Ramesh Babu K V <[email protected]> Signed-off-by: Vinod Koul <[email protected]> Signed-off-by: Dan Williams <[email protected]> My understanding of latest dma core change is: the memcpy interface is mainly used for transfer between memory and memory (no peripherals involved), and community suggest us to move to the device_prep_slave_sg(). > > Was this patch tested? > > > + rxs = &dw_dma->dmas_rx; > > + rxs->hs_mode = LNW_DMA_HW_HS; > > + rxs->cfg_mode = LNW_DMA_PER_TO_MEM; > > dws->rxchan->private = rxs; > Is this based on upstream bits, if so then you shouldn't be using > private variable. Calling device_control with DMA_SLAVE_CONFIG should > be enough > > > + /* 2. Prepare the TX dma transfer */ > > + txconf.direction = DMA_TO_DEVICE; > > + txconf.dst_addr = dws->dma_addr; > > + txconf.dst_maxburst = LNW_DMA_MSIZE_16; > Burst size of 16 for HW handshaking, a definite no from me. > This can't work :( Actually this setting worked since Jan 2009, and have been verified with GTM501L SPI modem and the latest IFX60x60 SPI modem. > > > + txchan->device->device_control(txchan, DMA_SLAVE_CONFIG, > > + (unsigned long) &txconf); > Okay, this is based on upstream bits, so please remove private. > > > + txdesc = txchan->device->device_prep_slave_sg(txchan, > > + &dws->tx_sgl, > > + 1, > > + DMA_TO_DEVICE, > > + DMA_PREP_INTERRUPT | > > DMA_COMPL_SKIP_DEST_UNMAP); > > + txdesc->callback = dw_spi_dma_done; > > + txdesc->callback_param = dws; > txdesc can be NULL here... You should handle that. Good point, I plan to submit the simplest change first, if the main framework is fine, then I will add the error handling code. Not only we need to check the txdesc, but also other places calling the generic DMA APIs > > > + /* 3. Prepare the RX dma transfer */ > > + rxconf.direction = DMA_FROM_DEVICE; > > + rxconf.src_addr = dws->dma_addr; > > + rxconf.src_maxburst = LNW_DMA_MSIZE_16; > This will not work > > > - if (rxdesc) > > - rxdesc->tx_submit(rxdesc); > > - if (txdesc) > > - txdesc->tx_submit(txdesc); > > - > > + rxdesc->tx_submit(rxdesc); > > + txdesc->tx_submit(txdesc); > Would prefer here the if ().... is not removed > > ~Vinod > _______________________________________________ MeeGo-kernel mailing list [email protected] http://lists.meego.com/listinfo/meego-kernel
