[PATCH v2 1/2] spi: spi-imx: enable dma support for escpi controller
Frank Li
lznuaa at gmail.com
Tue Jan 14 16:52:01 EST 2014
On Tue, Jan 14, 2014 at 12:02 AM, Shawn Guo <shawn.guo at linaro.org> wrote:
> The list linux-spi at vger.kernel.org should be copied.
>
> I also added Marek who has great experience of play DMA on SPI.
>
> On Sat, Jan 04, 2014 at 06:53:51AM +0800, Frank Li wrote:
>> After enable DMA
>>
>> spi-nor read speed is
>> dd if=/dev/mtd0 of=/dev/null bs=1M count=1
>> 1+0 records in
>> 1+0 records out
>> 1048576 bytes (1.0 MB) copied, 0.720402 s, 1.5 MB/s
>>
>> spi-nor write speed is
>> dd if=/dev/zero of=/dev/mtd0 bs=1M count=1
>> 1+0 records in
>> 1+0 records out
>> 1048576 bytes (1.0 MB) copied, 3.56044 s, 295 kB/s
>>
>> Before enable DMA
>>
>> spi-nor read speed is
>> dd if=/dev/mtd0 of=/dev/null bs=1M count=1
>> 1+0 records in
>> 1+0 records out
>> 1048576 bytes (1.0 MB) copied, 2.37717 s, 441 kB/s
>>
>> spi-nor write speed is
>>
>> dd if=/dev/zero of=/dev/mtd0 bs=1M count=1
>> 1+0 records in
>> 1+0 records out
>> 1048576 bytes (1.0 MB) copied, 4.83181 s, 217 kB/s
>>
>> Signed-off-by: Frank Li <Frank.Li at freescale.com>
>> ---
>> drivers/spi/spi-imx.c | 447 ++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 files changed, 441 insertions(+), 6 deletions(-)
>>
>> Change from v1.
>> 1. check if res is null at res = platform_get_resource(pdev, IORESOURCE_MEM, 0)
>> 2. fix failure transfer when len is not multiple watermark
>>
>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
>> index b80f2f7..1a9099a 100644
>> --- a/drivers/spi/spi-imx.c
>> +++ b/drivers/spi/spi-imx.c
>> @@ -39,6 +39,10 @@
>> #include <linux/of_gpio.h>
>>
>> #include <linux/platform_data/spi-imx.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/platform_data/dma-imx.h>
>> +#include <linux/dmaengine.h>
>> +
>
> Nit: we already have one blank in between.
>
>>
>> #define DRIVER_NAME "spi_imx"
>>
>> @@ -52,6 +56,9 @@
>> #define MXC_INT_RR (1 << 0) /* Receive data ready interrupt */
>> #define MXC_INT_TE (1 << 1) /* Transmit FIFO empty interrupt */
>>
>> +/* The maximum bytes that a sdma BD can transfer.*/
>> +#define MAX_SDMA_BD_BYTES (1 << 15)
>> +
>> struct spi_imx_config {
>> unsigned int speed_hz;
>> unsigned int bpw;
>> @@ -84,6 +91,7 @@ struct spi_imx_data {
>>
>> struct completion xfer_done;
>> void __iomem *base;
>> + resource_size_t mapbase;
>
> I think the following definition is more easy to understand.
>
> phys_addr_t pbase;
>
>> int irq;
>> struct clk *clk_per;
>> struct clk *clk_ipg;
>> @@ -92,6 +100,29 @@ struct spi_imx_data {
>> unsigned int count;
>> void (*tx)(struct spi_imx_data *);
>> void (*rx)(struct spi_imx_data *);
>> + int (*txrx_bufs)(struct spi_device *spi, struct spi_transfer *t);
>> + struct dma_chan *dma_chan_rx, *dma_chan_tx;
>
> Having them on two line fits better to the style used here.
>
>> + unsigned int dma_is_inited;
>
> All other lines use space in between. Why these two use tabs?
>
>> + struct device *dev;
>> +
>> + struct completion dma_rx_completion;
>> + struct completion dma_tx_completion;
>> +
>> + u8 *dma_rx_tmpbuf;
>> + unsigned int dma_rx_tmpbuf_size;
>> + unsigned int dma_rx_tmpbuf_phy_addr;
>
> Use type dma_addr_t for it?
>
>> +
>> + u8 *dma_tx_tmpbuf;
>> + unsigned int dma_tx_tmpbuf_size;
>> + unsigned int dma_tx_tmpbuf_phy_addr;
>> +
>> + unsigned int usedma;
>> + unsigned int dma_finished;
>> + /* SDMA wartermark */
>> + u32 rx_wml;
>> + u32 tx_wml;
>> + u32 rxt_wml;
>> +
>> void *rx_buf;
>> const void *tx_buf;
>> unsigned int txfifo; /* number of words pushed in tx FIFO */
>> @@ -185,6 +216,7 @@ static unsigned int spi_imx_clkdiv_2(unsigned int fin,
>> #define MX51_ECSPI_CTRL 0x08
>> #define MX51_ECSPI_CTRL_ENABLE (1 << 0)
>> #define MX51_ECSPI_CTRL_XCH (1 << 2)
>> +#define MX51_ECSPI_CTRL_SMC (1 << 3)
>
> You do not have to follow the wrong pattern. One space after '<<' is
> good enough.
>
>> #define MX51_ECSPI_CTRL_MODE_MASK (0xf << 4)
>> #define MX51_ECSPI_CTRL_POSTDIV_OFFSET 8
>> #define MX51_ECSPI_CTRL_PREDIV_OFFSET 12
>> @@ -202,9 +234,22 @@ static unsigned int spi_imx_clkdiv_2(unsigned int fin,
>> #define MX51_ECSPI_INT_TEEN (1 << 0)
>> #define MX51_ECSPI_INT_RREN (1 << 3)
>>
>> +#define MX51_ECSPI_DMA 0x14
>> +#define MX51_ECSPI_DMA_TX_WML_OFFSET 0
>> +#define MX51_ECSPI_DMA_TX_WML_MASK 0x3F
>
> It seems that the common pattern for hex value in this file is to use
> lowercase.
>
>> +#define MX51_ECSPI_DMA_RX_WML_OFFSET 16
>> +#define MX51_ECSPI_DMA_RX_WML_MASK (0x3F << 16)
>> +#define MX51_ECSPI_DMA_RXT_WML_OFFSET 24
>> +#define MX51_ECSPI_DMA_RXT_WML_MASK (0x3F << 16)
>
> Shouldn't it be (0x3F << 24)?
>
>> +
>> +#define MX51_ECSPI_DMA_TEDEN_OFFSET 7
>> +#define MX51_ECSPI_DMA_RXDEN_OFFSET 23
>> +#define MX51_ECSPI_DMA_RXTDEN_OFFSET 31
>
> Please use tabs to align these numbers in the same column.
>
>> +
>> #define MX51_ECSPI_STAT 0x18
>> #define MX51_ECSPI_STAT_RR (1 << 3)
>>
>> +#define MX51_ECSPI_TESTREG 0x20
>
> It's used nowhere?
>
>> /* MX51 eCSPI */
>> static unsigned int mx51_ecspi_clkdiv(unsigned int fin, unsigned int fspi)
>> {
>> @@ -255,16 +300,28 @@ static void __maybe_unused mx51_ecspi_trigger(struct spi_imx_data *spi_imx)
>> {
>> u32 reg;
>>
>> - reg = readl(spi_imx->base + MX51_ECSPI_CTRL);
>> - reg |= MX51_ECSPI_CTRL_XCH;
>> - writel(reg, spi_imx->base + MX51_ECSPI_CTRL);
>> + if (!spi_imx->usedma) {
>
> So the DMA support is added for imx51-ecspi type device only? If that's
> case, we should make it clear in commit log.
>
>> + reg = readl(spi_imx->base + MX51_ECSPI_CTRL);
>> + reg |= MX51_ECSPI_CTRL_XCH;
>> + writel(reg, spi_imx->base + MX51_ECSPI_CTRL);
>> + } else {
>> + if (!spi_imx->dma_finished) {
>> + reg = readl(spi_imx->base + MX51_ECSPI_CTRL);
>> + reg |= MX51_ECSPI_CTRL_SMC;
>> + writel(reg, spi_imx->base + MX51_ECSPI_CTRL);
>> + } else {
>> + reg = readl(spi_imx->base + MX51_ECSPI_CTRL);
>> + reg &= (~MX51_ECSPI_CTRL_SMC);
>
> Unnecessary parentheses.
>
>> + writel(reg, spi_imx->base + MX51_ECSPI_CTRL);
>> + }
>
> Wouldn't it make more sense to write it like below?
>
> reg = readl(spi_imx->base + MX51_ECSPI_CTRL);
> if (!spi_imx->dma_finished)
> reg |= MX51_ECSPI_CTRL_SMC;
> else
> reg &= ~MX51_ECSPI_CTRL_SMC;
> writel(reg, spi_imx->base + MX51_ECSPI_CTRL);
>
>> + }
>> }
>>
>> static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>> struct spi_imx_config *config)
>> {
>> - u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0;
>> -
>> + u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0, dma = 0;
>> + u32 tx_wml_cfg, rx_wml_cfg, rxt_wml_cfg;
>> /*
>> * The hardware seems to have a race condition when changing modes. The
>> * current assumption is that the selection of the channel arrives
>> @@ -297,6 +354,30 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>> writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
>> writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
>>
>> + /*
>> + * Configure the DMA register: setup the watermark
>> + * and enable DMA request.
>> + */
>> + if (spi_imx->dma_is_inited) {
>> + dma = readl(spi_imx->base + MX51_ECSPI_DMA);
>> +
>> + spi_imx->tx_wml = spi_imx_get_fifosize(spi_imx) / 2;
>> + spi_imx->rx_wml = spi_imx_get_fifosize(spi_imx) / 2;
>> + spi_imx->rxt_wml = spi_imx_get_fifosize(spi_imx) / 2;
>> + rx_wml_cfg = spi_imx->rx_wml << MX51_ECSPI_DMA_RX_WML_OFFSET;
>> + tx_wml_cfg = spi_imx->tx_wml << MX51_ECSPI_DMA_TX_WML_OFFSET;
>> + rxt_wml_cfg = spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET;
>> + dma = (dma & (~MX51_ECSPI_DMA_TX_WML_MASK)
>> + & (~MX51_ECSPI_DMA_RX_WML_MASK)
>> + & (~MX51_ECSPI_DMA_RXT_WML_MASK))
>
> Unnecessary parentheses.
>
>> + | rx_wml_cfg | tx_wml_cfg | rxt_wml_cfg
>> + | (1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
>> + | (1 << MX51_ECSPI_DMA_RXDEN_OFFSET)
>> + | (1 << MX51_ECSPI_DMA_RXTDEN_OFFSET);
>> +
>> + writel(dma, spi_imx->base + MX51_ECSPI_DMA);
>> + }
>> +
>> return 0;
>> }
>
> ... skip the dma code, and hope Marek can help review it ...
>
>> @@ -729,6 +1088,27 @@ static int spi_imx_transfer(struct spi_device *spi,
>> return transfer->len;
>> }
>>
>> +static int spi_imx_transfer(struct spi_device *spi,
>> + struct spi_transfer *transfer)
>> +{
>> + struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
>> +
>> + if (spi_imx->dma_chan_tx && spi_imx->dma_chan_rx) {
>> + /*
>> + * Don't use sdma when the size of data to be transfered is
>> + * lower then SDMA wartermark.
>> + */
>> + if ((transfer->len >= spi_imx->rx_wml) &&
>> + (transfer->len > spi_imx->tx_wml)) {
>> + spi_imx->usedma = 1;
>> + return spi_imx_sdma_transfer(spi, transfer);
>> + }
>> + }
>> +
>> + spi_imx->usedma = 0;
>> + return spi_imx_pio_transfer(spi, transfer);
>> +}
>> +
>> static int spi_imx_setup(struct spi_device *spi)
>> {
>> struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
>> @@ -778,6 +1158,56 @@ spi_imx_unprepare_message(struct spi_master *master, struct spi_message *msg)
>> return 0;
>> }
>>
>> +static int spi_imx_sdma_init(struct spi_imx_data *spi_imx)
>> +{
>> + struct dma_slave_config slave_config = {};
>> + struct device *dev = spi_imx->dev;
>> + int ret;
>> +
>> +
>> + /* Prepare for TX : */
>> + spi_imx->dma_chan_tx = dma_request_slave_channel(dev, "tx");
>> + if (!spi_imx->dma_chan_tx) {
>> + dev_err(dev, "cannot get the TX DMA channel!\n");
>
> So we will see this error message for each type of imx spi device,
> though the driver only supports DMA for imx51-ecspi.
>
> Shawn
How to handle this one? change to devinfo? Or check check if imx51-ecspi?
>
>> + ret = -EINVAL;
>> + goto err;
>> + }
>> +
>> + slave_config.direction = DMA_MEM_TO_DEV;
>> + slave_config.dst_addr = spi_imx->mapbase + MXC_CSPITXDATA;
>> + slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>> + slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx) / 2;
>> + ret = dmaengine_slave_config(spi_imx->dma_chan_tx, &slave_config);
>> + if (ret) {
>> + dev_err(dev, "error in TX dma configuration.");
>> + goto err;
>> + }
>> +
>> + /* Prepare for RX : */
>> + spi_imx->dma_chan_rx = dma_request_slave_channel(dev, "rx");
>> + if (!spi_imx->dma_chan_rx) {
>> + dev_dbg(dev, "cannot get the DMA channel.\n");
>> + ret = -EINVAL;
>> + goto err;
>> + }
>> +
>> + slave_config.direction = DMA_DEV_TO_MEM;
>> + slave_config.src_addr = spi_imx->mapbase + MXC_CSPIRXDATA;
>> + slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>> + slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx) / 2;
>> + ret = dmaengine_slave_config(spi_imx->dma_chan_rx, &slave_config);
>> + if (ret) {
>> + dev_err(dev, "error in RX dma configuration.\n");
>> + goto err;
>> + }
>> + spi_imx->dma_is_inited = 1;
>> +
>> + return 0;
>> +err:
>> + spi_imx_sdma_exit(spi_imx);
>> + return ret;
>> +}
>> +
>> static int spi_imx_probe(struct platform_device *pdev)
>> {
>> struct device_node *np = pdev->dev.of_node;
>> @@ -849,6 +1279,8 @@ static int spi_imx_probe(struct platform_device *pdev)
>> (struct spi_imx_devtype_data *) pdev->id_entry->driver_data;
>>
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (res)
>> + spi_imx->mapbase = res->start;
>> spi_imx->base = devm_ioremap_resource(&pdev->dev, res);
>> if (IS_ERR(spi_imx->base)) {
>> ret = PTR_ERR(spi_imx->base);
>> @@ -890,6 +1322,9 @@ static int spi_imx_probe(struct platform_device *pdev)
>>
>> spi_imx->spi_clk = clk_get_rate(spi_imx->clk_per);
>>
>> + spi_imx->dev = &pdev->dev;
>> + spi_imx_sdma_init(spi_imx);
>> +
>> spi_imx->devtype_data->reset(spi_imx);
>>
>> spi_imx->devtype_data->intctrl(spi_imx, 0);
>> --
>> 1.7.8
>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
More information about the linux-arm-kernel
mailing list