[Meego-kernel] [PATCH 2/2] spi/dw_spi: Change the dma interface following dmaengine core's change
feng.tang at intel.com
Sun Oct 31 19:30:22 PDT 2010
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" <vinod.koul at intel.com> wrote:
> > 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:
Author: Ramesh Babu K V <ramesh.b.k.v at intel.com>
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 <ramesh.b.k.v at intel.com>
Signed-off-by: Vinod Koul <vinod.koul at intel.com>
Signed-off-by: Dan Williams <dan.j.williams at intel.com>
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
> > + /* 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
More information about the MeeGo-kernel