Add Mediatek SPI driver

Add Mediatek SPI driver

[leilk.liu]

Mediatek SPI BUS controller has 3 hardware restrictions:
1. Hw has the restriction that in one transfer, length must be a multiple of
   1024, when it's greater than 1024bytes.
2. Hw tx/rx have 4bytes aligned restriction.
3. For MT8173 IC: RX must enable TX, then TX transfer dummy data; TX don't need
   to enable RX.
Some workarounds are done in SPI driver code base on v4.1-rc1.

[PATCH 1/3] dt-bindings: ARM: Mediatek: Document devicetree bindings for spi bus

[leilk.liu]

From: Leilk Liu <leilk.liu@mediatek.com>

Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
---
 .../devicetree/bindings/spi/spi-mt65xx.txt         | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-mt65xx.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-mt65xx.txt b/Documentation/devicetree/bindings/spi/spi-mt65xx.txt
new file mode 100644
index 0000000..04c28fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-mt65xx.txt
@@ -0,0 +1,32 @@
+MTK SPI device
+
+Required properties:
+- compatible: should be one of the following.
+    - mediatek,mt8173-spi: for mt8173 platforms
+    - mediatek,mt8135-spi: for mt8135 platforms
+    - mediatek,mt6589-spi: for mt6589 platforms
+
+- reg: Address and length of the register set for the device
+
+- interrupts: Should contain spi interrupt
+
+- clock-names: tuple listing input clock names.
+	Required elements: "main"
+
+- clocks: phandles to input clocks.
+
+- pad-select: should specify spi pad used, only required for MT8173.
+	This value should be 0~3.
+
+Example:
+
+- SoC Specific Portion:
+spi: spi@1100a000 {
+	compatible = "mediatek,mt8173-spi";
+	reg = <0 0x1100a000 0 0x1000>;
+	interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_LOW>;
+	clocks = <&pericfg PERI_SPI0>;
+	clock-names = "main";
+	pad-select = <1>;
+	status = "disabled";
+};
-- 
1.8.1.1.dirty

[PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[leilk.liu]

From: Leilk Liu <leilk.liu@mediatek.com>

This patch adds basic spi bus for MT8173.

Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
---
 drivers/spi/Kconfig      |  10 +
 drivers/spi/Makefile     |   1 +
 drivers/spi/spi-mt65xx.c | 622 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 633 insertions(+)
 create mode 100644 drivers/spi/spi-mt65xx.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 198f96b..01239e0 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -324,6 +324,16 @@ config SPI_MESON_SPIFC
 	  This enables master mode support for the SPIFC (SPI flash
 	  controller) available in Amlogic Meson SoCs.
 
+config SPI_MT65XX
+	tristate "MediaTek SPI controller"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	select SPI_BITBANG
+	help
+	  This selects the MediaTek(R) SPI bus driver.
+	  If you want to use MediaTek(R) SPI interface,
+	  say Y or M here.If you are not sure, say N.
+	  SPI drivers for Mediatek mt65XX series ARM SoCs.
+
 config SPI_OC_TINY
 	tristate "OpenCores tiny SPI"
 	depends on GPIOLIB
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d8cbf65..ab332ef 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_SPI_MESON_SPIFC)		+= spi-meson-spifc.o
 obj-$(CONFIG_SPI_MPC512x_PSC)		+= spi-mpc512x-psc.o
 obj-$(CONFIG_SPI_MPC52xx_PSC)		+= spi-mpc52xx-psc.o
 obj-$(CONFIG_SPI_MPC52xx)		+= spi-mpc52xx.o
+obj-$(CONFIG_SPI_MT65XX)                += spi-mt65xx.o
 obj-$(CONFIG_SPI_MXS)			+= spi-mxs.o
 obj-$(CONFIG_SPI_NUC900)		+= spi-nuc900.o
 obj-$(CONFIG_SPI_OC_TINY)		+= spi-oc-tiny.o
diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
new file mode 100644
index 0000000..92c119d
--- /dev/null
+++ b/drivers/spi/spi-mt65xx.c
@@ -0,0 +1,622 @@
+/*
+ * Copyright (c) 2015 MediaTek Inc.
+ * Author: Leilk Liu <leilk.liu@mediatek.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/ioport.h>
+#include <linux/errno.h>
+#include <linux/spi/spi.h>
+#include <linux/workqueue.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/sched.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/kernel.h>
+#include <linux/spi/spi_bitbang.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+
+#define SPI_CFG0_REG                      0x0000
+#define SPI_CFG1_REG                      0x0004
+#define SPI_TX_SRC_REG                    0x0008
+#define SPI_RX_DST_REG                    0x000c
+#define SPI_CMD_REG                       0x0018
+#define SPI_STATUS0_REG                   0x001c
+#define SPI_PAD_SEL_REG                   0x0024
+
+#define SPI_CFG0_SCK_HIGH_OFFSET          0
+#define SPI_CFG0_SCK_LOW_OFFSET           8
+#define SPI_CFG0_CS_HOLD_OFFSET           16
+#define SPI_CFG0_CS_SETUP_OFFSET          24
+
+#define SPI_CFG0_SCK_HIGH_MASK            0xff
+#define SPI_CFG0_SCK_LOW_MASK             0xff00
+#define SPI_CFG0_CS_HOLD_MASK             0xff0000
+#define SPI_CFG0_CS_SETUP_MASK            0xff000000
+
+#define SPI_CFG1_CS_IDLE_OFFSET           0
+#define SPI_CFG1_PACKET_LOOP_OFFSET       8
+#define SPI_CFG1_PACKET_LENGTH_OFFSET     16
+#define SPI_CFG1_GET_TICK_DLY_OFFSET      30
+
+#define SPI_CFG1_CS_IDLE_MASK             0xff
+#define SPI_CFG1_PACKET_LOOP_MASK         0xff00
+#define SPI_CFG1_PACKET_LENGTH_MASK       0x3ff0000
+#define SPI_CFG1_GET_TICK_DLY_MASK        0xc0000000
+
+#define SPI_CMD_ACT_OFFSET                0
+#define SPI_CMD_RESUME_OFFSET             1
+#define SPI_CMD_RST_OFFSET                2
+#define SPI_CMD_PAUSE_EN_OFFSET           4
+#define SPI_CMD_DEASSERT_OFFSET           5
+#define SPI_CMD_CPHA_OFFSET               8
+#define SPI_CMD_CPOL_OFFSET               9
+#define SPI_CMD_RX_DMA_OFFSET             10
+#define SPI_CMD_TX_DMA_OFFSET             11
+#define SPI_CMD_TXMSBF_OFFSET             12
+#define SPI_CMD_RXMSBF_OFFSET             13
+#define SPI_CMD_RX_ENDIAN_OFFSET          14
+#define SPI_CMD_TX_ENDIAN_OFFSET          15
+#define SPI_CMD_FINISH_IE_OFFSET          16
+#define SPI_CMD_PAUSE_IE_OFFSET           17
+
+#define SPI_CMD_RESUME_MASK               0x2
+#define SPI_CMD_RST_MASK                  0x4
+#define SPI_CMD_PAUSE_EN_MASK             0x10
+#define SPI_CMD_DEASSERT_MASK             0x20
+#define SPI_CMD_CPHA_MASK                 0x100
+#define SPI_CMD_CPOL_MASK                 0x200
+#define SPI_CMD_RX_DMA_MASK               0x400
+#define SPI_CMD_TX_DMA_MASK               0x800
+#define SPI_CMD_TXMSBF_MASK               0x1000
+#define SPI_CMD_RXMSBF_MASK               0x2000
+#define SPI_CMD_RX_ENDIAN_MASK            0x4000
+#define SPI_CMD_TX_ENDIAN_MASK            0x8000
+#define SPI_CMD_FINISH_IE_MASK            0x10000
+
+#define COMPAT_MT6589			(0x1 << 0)
+#define COMPAT_MT8173			(0x1 << 1)
+
+#define MT8173_MAX_PAD_SEL 3
+
+#define IDLE 0
+#define INPROGRESS 1
+#define PAUSED 2
+
+#define PACKET_SIZE 1024
+
+struct mtk_chip_config {
+	u32 setuptime;
+	u32 holdtime;
+	u32 high_time;
+	u32 low_time;
+	u32 cs_idletime;
+	u32 tx_mlsb;
+	u32 rx_mlsb;
+	u32 tx_endian;
+	u32 rx_endian;
+	u32 pause;
+	u32 finish_intr;
+	u32 deassert;
+	u32 tckdly;
+};
+
+struct mtk_spi_ddata {
+	struct spi_bitbang bitbang;
+	void __iomem *base;
+	u32 irq;
+	u32 state;
+	u32 platform_compat;
+	u32 pad_sel;
+	struct clk *clk;
+
+	const u8 *tx_buf;
+	u8 *rx_buf;
+	u32 tx_len, rx_len;
+	struct completion done;
+};
+
+/*
+ * A piece of default chip info unless the platform
+ * supplies it.
+ */
+static const struct mtk_chip_config mtk_default_chip_info = {
+	.setuptime = 10,
+	.holdtime = 12,
+	.high_time = 6,
+	.low_time = 6,
+	.cs_idletime = 12,
+	.rx_mlsb = 1,
+	.tx_mlsb = 1,
+	.tx_endian = 0,
+	.rx_endian = 0,
+	.pause = 0,
+	.finish_intr = 1,
+	.deassert = 0,
+	.tckdly = 0,
+};
+
+static const struct of_device_id mtk_spi_of_match[] = {
+	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
+	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
+	{}
+};
+MODULE_DEVICE_TABLE(of, mtk_spi_of_match);
+
+static void mtk_spi_reset(struct mtk_spi_ddata *mdata)
+{
+	u32 reg_val;
+
+	/*set the software reset bit in SPI_CMD_REG.*/
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_RST_MASK;
+	reg_val |= 1 << SPI_CMD_RST_OFFSET;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_RST_MASK;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+}
+
+static void mtk_set_pause_bit(struct mtk_spi_ddata *mdata)
+{
+	u32 reg_val;
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val |= 1 << SPI_CMD_PAUSE_EN_OFFSET;
+	reg_val |= 1 << SPI_CMD_PAUSE_IE_OFFSET;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+}
+
+static void mtk_clear_pause_bit(struct mtk_spi_ddata *mdata)
+{
+	u32 reg_val;
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_PAUSE_EN_MASK;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+}
+
+static int mtk_spi_config(struct mtk_spi_ddata *mdata,
+			  struct mtk_chip_config *chip_config)
+{
+	u32 reg_val;
+
+	/* set the timing */
+	reg_val = readl(mdata->base + SPI_CFG0_REG);
+	reg_val &= ~(SPI_CFG0_SCK_HIGH_MASK | SPI_CFG0_SCK_LOW_MASK);
+	reg_val &= ~(SPI_CFG0_CS_HOLD_MASK | SPI_CFG0_CS_SETUP_MASK);
+	reg_val |= ((chip_config->high_time - 1) << SPI_CFG0_SCK_HIGH_OFFSET);
+	reg_val |= ((chip_config->low_time - 1) << SPI_CFG0_SCK_LOW_OFFSET);
+	reg_val |= ((chip_config->holdtime - 1) << SPI_CFG0_CS_HOLD_OFFSET);
+	reg_val |= ((chip_config->setuptime - 1) << SPI_CFG0_CS_SETUP_OFFSET);
+	writel(reg_val, mdata->base + SPI_CFG0_REG);
+
+	reg_val = readl(mdata->base + SPI_CFG1_REG);
+	reg_val &= ~SPI_CFG1_CS_IDLE_MASK;
+	reg_val |= ((chip_config->cs_idletime - 1) << SPI_CFG1_CS_IDLE_OFFSET);
+	reg_val &= ~SPI_CFG1_GET_TICK_DLY_MASK;
+	reg_val |= ((chip_config->tckdly) << SPI_CFG1_GET_TICK_DLY_OFFSET);
+	writel(reg_val, mdata->base + SPI_CFG1_REG);
+
+	/* set the mlsbx and mlsbtx */
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~(SPI_CMD_TX_ENDIAN_MASK | SPI_CMD_RX_ENDIAN_MASK);
+	reg_val &= ~(SPI_CMD_TXMSBF_MASK | SPI_CMD_RXMSBF_MASK);
+	reg_val |= (chip_config->tx_mlsb << SPI_CMD_TXMSBF_OFFSET);
+	reg_val |= (chip_config->rx_mlsb << SPI_CMD_RXMSBF_OFFSET);
+	reg_val |= (chip_config->tx_endian << SPI_CMD_TX_ENDIAN_OFFSET);
+	reg_val |= (chip_config->rx_endian << SPI_CMD_RX_ENDIAN_OFFSET);
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+
+	/* set finish and pause interrupt always enable */
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_FINISH_IE_MASK;
+	reg_val |= (chip_config->finish_intr << SPI_CMD_FINISH_IE_OFFSET);
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val |= 1 << SPI_CMD_TX_DMA_OFFSET;
+	reg_val |= 1 << SPI_CMD_RX_DMA_OFFSET;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+
+	/* set deassert mode */
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_DEASSERT_MASK;
+	reg_val |= (chip_config->deassert << SPI_CMD_DEASSERT_OFFSET);
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+
+	/* pad select */
+	if (mdata->platform_compat & COMPAT_MT8173)
+		writel(mdata->pad_sel, mdata->base + SPI_PAD_SEL_REG);
+
+	return 0;
+}
+
+static int mtk_spi_setup_transfer(struct spi_device *spi,
+				  struct spi_transfer *t)
+{
+	u32 reg_val;
+	struct spi_master *master = spi->master;
+	struct mtk_spi_ddata *mdata = spi_master_get_devdata(master);
+	struct spi_message *m = master->cur_msg;
+	struct mtk_chip_config *chip_config;
+
+	u8 cpha	= spi->mode & SPI_CPHA ? 1 : 0;
+	u8 cpol = spi->mode & SPI_CPOL ? 1 : 0;
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~(SPI_CMD_CPHA_MASK | SPI_CMD_CPOL_MASK);
+	reg_val |= (cpha << SPI_CMD_CPHA_OFFSET);
+	reg_val |= (cpol << SPI_CMD_CPOL_OFFSET);
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+
+	if (t->cs_change) {
+		if (!(list_is_last(&t->transfer_list, &m->transfers)))
+			mdata->state = IDLE;
+	} else {
+		mdata->state = IDLE;
+		mtk_spi_reset(mdata);
+	}
+
+	chip_config = (struct mtk_chip_config *)spi->controller_data;
+	if (!chip_config) {
+		chip_config = (void *)&mtk_default_chip_info;
+		spi->controller_data = chip_config;
+		mdata->state = IDLE;
+	}
+
+	mtk_spi_config(mdata, chip_config);
+
+	return 0;
+}
+
+static void mtk_spi_chipselect(struct spi_device *spi, int is_on)
+{
+	struct mtk_spi_ddata *mdata = spi_master_get_devdata(spi->master);
+
+	switch (is_on) {
+	case BITBANG_CS_ACTIVE:
+		mtk_set_pause_bit(mdata);
+		break;
+	case BITBANG_CS_INACTIVE:
+		mtk_clear_pause_bit(mdata);
+		break;
+	}
+}
+
+static void mtk_spi_start_transfer(struct mtk_spi_ddata *mdata)
+{
+	u32 reg_val;
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val |= 1 << SPI_CMD_ACT_OFFSET;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+}
+
+static void mtk_spi_resume_transfer(struct mtk_spi_ddata *mdata)
+{
+	u32 reg_val;
+
+	reg_val = readl(mdata->base + SPI_CMD_REG);
+	reg_val &= ~SPI_CMD_RESUME_MASK;
+	reg_val |= 1 << SPI_CMD_RESUME_OFFSET;
+	writel(reg_val, mdata->base + SPI_CMD_REG);
+}
+
+static int mtk_spi_setup_packet(struct mtk_spi_ddata *mdata,
+				struct spi_transfer *xfer)
+{
+	struct device *dev = &mdata->bitbang.master->dev;
+	u32 packet_size, packet_loop, reg_val;
+
+	packet_size = min_t(unsigned, xfer->len, PACKET_SIZE);
+
+	/* mtk hw has the restriction that xfer len must be a multiple of 1024,
+	 * when it is greater than 1024bytes.
+	 */
+	if (xfer->len % packet_size) {
+		dev_err(dev, "ERROR!The lens must be a multiple of %d, your len %d\n",
+			PACKET_SIZE, xfer->len);
+		return -EINVAL;
+	}
+
+	packet_loop = xfer->len / packet_size;
+
+	reg_val = readl(mdata->base + SPI_CFG1_REG);
+	reg_val &= ~(SPI_CFG1_PACKET_LENGTH_MASK + SPI_CFG1_PACKET_LOOP_MASK);
+	reg_val |= (packet_size - 1) << SPI_CFG1_PACKET_LENGTH_OFFSET;
+	reg_val |= (packet_loop - 1) << SPI_CFG1_PACKET_LOOP_OFFSET;
+	writel(reg_val, mdata->base + SPI_CFG1_REG);
+
+	return 0;
+}
+
+static int mtk_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *xfer)
+{
+	struct spi_master *master = spi->master;
+	struct mtk_spi_ddata *mdata = spi_master_get_devdata(master);
+	struct device *dev = &mdata->bitbang.master->dev;
+	int cmd, ret;
+
+	/* mtk spi hw tx/rx have 4bytes aligned restriction,
+	 * so kmalloc tx/rx buffer to workaround here.
+	 */
+	mdata->tx_buf = NULL;
+	mdata->rx_buf = NULL;
+	if (xfer->tx_buf) {
+		mdata->tx_buf = kmalloc(xfer->len, GFP_KERNEL);
+		if (!mdata->tx_buf) {
+			dev_err(dev, "malloc tx_buf failed.\n");
+			ret = -ENOMEM;
+			goto err_free;
+		}
+		memcpy((void *)mdata->tx_buf, xfer->tx_buf, xfer->len);
+	}
+	if (xfer->rx_buf) {
+		mdata->rx_buf = kmalloc(xfer->len, GFP_KERNEL);
+		if (!mdata->rx_buf) {
+			dev_err(dev, "malloc rx_buf failed.\n");
+			ret = -ENOMEM;
+			goto err_free;
+		}
+	}
+
+	reinit_completion(&mdata->done);
+
+	xfer->tx_dma = DMA_ERROR_CODE;
+	xfer->rx_dma = DMA_ERROR_CODE;
+	if (xfer->tx_buf) {
+		xfer->tx_dma = dma_map_single(dev, (void *)mdata->tx_buf,
+					      xfer->len, DMA_TO_DEVICE);
+		if (dma_mapping_error(dev, xfer->tx_dma)) {
+			dev_err(dev, "dma mapping tx_buf error.\n");
+			ret = -ENOMEM;
+			goto err_free;
+		}
+	}
+	if (xfer->rx_buf) {
+		xfer->rx_dma = dma_map_single(dev, mdata->rx_buf,
+					      xfer->len, DMA_FROM_DEVICE);
+		if (dma_mapping_error(dev, xfer->rx_dma)) {
+			if (xfer->tx_buf)
+				dma_unmap_single(dev, xfer->tx_dma,
+						 xfer->len, DMA_TO_DEVICE);
+			dev_err(dev, "dma mapping rx_buf error.\n");
+			ret = -ENOMEM;
+			goto err_free;
+		}
+	}
+
+	ret = mtk_spi_setup_packet(mdata, xfer);
+	if (ret != 0)
+		goto err_free;
+
+	/* Here is mt8173 HW issue: RX must enable TX, then TX transfer
+	 * dummy data; TX don't need to enable RX. so enable TX dma for
+	 * RX to workaround.
+	 */
+	cmd = readl(mdata->base + SPI_CMD_REG);
+	if (xfer->tx_buf || (mdata->platform_compat & COMPAT_MT8173))
+		cmd |= 1 << SPI_CMD_TX_DMA_OFFSET;
+	if (xfer->rx_buf)
+		cmd |= 1 << SPI_CMD_RX_DMA_OFFSET;
+	writel(cmd, mdata->base + SPI_CMD_REG);
+
+	/* set up the DMA bus address */
+	if (xfer->tx_dma != DMA_ERROR_CODE)
+		writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
+	if (xfer->rx_dma != DMA_ERROR_CODE)
+		writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
+
+	if (mdata->state == IDLE)
+		mtk_spi_start_transfer(mdata);
+	else if (mdata->state == PAUSED)
+		mtk_spi_resume_transfer(mdata);
+	else
+		mdata->state = INPROGRESS;
+
+	wait_for_completion(&mdata->done);
+
+	if (xfer->tx_dma != DMA_ERROR_CODE) {
+		dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE);
+		xfer->tx_dma = DMA_ERROR_CODE;
+	}
+	if (xfer->rx_dma != DMA_ERROR_CODE) {
+		dma_unmap_single(dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE);
+		xfer->rx_dma = DMA_ERROR_CODE;
+	}
+
+	/* spi disable dma */
+	cmd = readl(mdata->base + SPI_CMD_REG);
+	cmd &= ~SPI_CMD_TX_DMA_MASK;
+	cmd &= ~SPI_CMD_RX_DMA_MASK;
+	writel(cmd, mdata->base + SPI_CMD_REG);
+
+	if (xfer->rx_buf)
+		memcpy(xfer->rx_buf, mdata->rx_buf, xfer->len);
+
+	ret = xfer->len;
+
+err_free:
+	kfree(mdata->tx_buf);
+	kfree(mdata->rx_buf);
+	return ret;
+}
+
+static irqreturn_t mtk_spi_interrupt(int irq, void *dev_id)
+{
+	struct mtk_spi_ddata *mdata = dev_id;
+	u32 reg_val;
+
+	reg_val = readl(mdata->base + SPI_STATUS0_REG);
+	if (reg_val & 0x2)
+		mdata->state = PAUSED;
+	else
+		mdata->state = IDLE;
+	complete(&mdata->done);
+
+	return IRQ_HANDLED;
+}
+
+static unsigned long mtk_get_device_prop(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+
+	match = of_match_node(mtk_spi_of_match, pdev->dev.of_node);
+	return (unsigned long)match->data;
+}
+
+static int mtk_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct mtk_spi_ddata *mdata;
+	struct resource *res;
+	int	ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct mtk_spi_ddata));
+	if (!master) {
+		dev_err(&pdev->dev, "failed to alloc spi master\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, master);
+
+	master->dev.of_node = pdev->dev.of_node;
+	master->bus_num = pdev->id;
+	master->num_chipselect = 1;
+	master->mode_bits = SPI_CPOL | SPI_CPHA;
+
+	mdata = spi_master_get_devdata(master);
+
+	mdata->bitbang.master = master;
+	mdata->bitbang.chipselect = mtk_spi_chipselect;
+	mdata->bitbang.setup_transfer = mtk_spi_setup_transfer;
+	mdata->bitbang.txrx_bufs = mtk_spi_txrx_bufs;
+	mdata->platform_compat = mtk_get_device_prop(pdev);
+
+	if (mdata->platform_compat & COMPAT_MT8173) {
+		ret = of_property_read_u32(pdev->dev.of_node, "pad-select",
+					   &mdata->pad_sel);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to read pad select: %d\n",
+				ret);
+			goto err;
+		}
+
+		if (mdata->pad_sel > MT8173_MAX_PAD_SEL) {
+			dev_err(&pdev->dev, "wrong pad-select: %u\n",
+				mdata->pad_sel);
+			goto err;
+		}
+	}
+
+	init_completion(&mdata->done);
+
+	mdata->clk = devm_clk_get(&pdev->dev, "main");
+	if (IS_ERR(mdata->clk)) {
+		ret = PTR_ERR(mdata->clk);
+		dev_err(&pdev->dev, "failed to get clock: %d\n", ret);
+		goto err;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		ret = -ENODEV;
+		dev_err(&pdev->dev, "failed to determine base address\n");
+		goto err;
+	}
+
+	mdata->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(mdata->base)) {
+		ret = PTR_ERR(mdata->base);
+		goto err;
+	}
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to get irq (%d)\n", ret);
+		goto err;
+	}
+
+	mdata->irq = ret;
+
+	if (!pdev->dev.dma_mask)
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+
+	mdata->bitbang.master->dev.dma_mask = pdev->dev.dma_mask;
+
+	ret = clk_prepare_enable(mdata->clk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to enable clock (%d)\n", ret);
+		goto err;
+	}
+
+	ret = devm_request_irq(&pdev->dev, mdata->irq, mtk_spi_interrupt,
+			       IRQF_TRIGGER_NONE, dev_name(&pdev->dev), mdata);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register irq (%d)\n", ret);
+		goto err_disable_clk;
+	}
+
+	ret = spi_bitbang_start(&mdata->bitbang);
+	if (ret) {
+		dev_err(&pdev->dev, "spi_bitbang_start failed (%d)\n", ret);
+err_disable_clk:
+		clk_disable_unprepare(mdata->clk);
+err:
+		spi_master_put(master);
+	}
+
+	return ret;
+}
+
+static int mtk_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct mtk_spi_ddata *mdata = spi_master_get_devdata(master);
+
+	spi_bitbang_stop(&mdata->bitbang);
+	mtk_spi_reset(mdata);
+	clk_disable_unprepare(mdata->clk);
+	spi_master_put(master);
+
+	return 0;
+}
+
+struct platform_driver mtk_spi_driver = {
+	.driver = {
+		.name = "mtk-spi",
+		.of_match_table = mtk_spi_of_match,
+	},
+	.probe = mtk_spi_probe,
+	.remove = mtk_spi_remove,
+};
+
+module_platform_driver(mtk_spi_driver);
+
+MODULE_DESCRIPTION("MTK SPI Controller driver");
+MODULE_AUTHOR("Leilk Liu <leilk.liu@mediatek.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform: mtk_spi");
-- 
1.8.1.1.dirty

[PATCH 3/3] arm64: dts: Add spi bus dts

[leilk.liu]

From: Leilk Liu <leilk.liu@mediatek.com>

This patch adds MT8173 spi bus controllers into device tree.

Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 3fca624..4ff7c24 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -258,6 +258,15 @@
 			clock-names = "baud", "bus";
 			status = "disabled";
 		};
+
+		spi: spi@1100a000 {
+			compatible = "mediatek,mt8173-spi";
+			reg = <0 0x1100a000 0 0x1000>;
+			interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_LOW>;
+			clocks = <&pericfg CLK_PERI_SPI0>;
+			clock-names = "main";
+			status = "disabled";
+		};
 	};
 
 };
-- 
1.8.1.1.dirty

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

On Fri, May 08, 2015 at 04:55:42PM +0800, leilk.liu@mediatek.com wrote:
> From: Leilk Liu <leilk.liu@mediatek.com>
> 
> This patch adds basic spi bus for MT8173.

Please try to only CC relevant people on patches, you've got a very
broad CC list here and I'm not sure I understand why everyone is on it.
Sending people irrelevant patches adds to the volume of mail people have
to handle which takes time away from other things.

> +config SPI_MT65XX
> +	tristate "MediaTek SPI controller"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	select SPI_BITBANG

Unless your controller is geniunely bitbanging you shouldn't be using
the bitbang framework, modern drivers should implement set_cs() and
transfer_one() instead.  You should also be using the core helpers for
DMA mapping.

The driver looks basically good other than this, it shouldn't be too
much work (and ought to save you some code) to refactor to the modern
interfaces.

> +#define IDLE 0
> +#define INPROGRESS 1
> +#define PAUSED 2
> +
> +#define PACKET_SIZE 1024

You should namespace the constants you're using to avoid clashes with
headers.

> +static const struct of_device_id mtk_spi_of_match[] = {
> +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);

There were three compatible strings listed in the DT binding but only
two here.

> +	/*set the software reset bit in SPI_CMD_REG.*/

/* Spaces please */

> +static unsigned long mtk_get_device_prop(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +
> +	match = of_match_node(mtk_spi_of_match, pdev->dev.of_node);
> +	return (unsigned long)match->data;
> +}

This wrapper doesn't seem to be doing a lot and will crash if we somehow
manage to get the driver loaded without a match (eg, if someone tries to
instantiate as a regular platform device).

> +	ret = clk_prepare_enable(mdata->clk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to enable clock (%d)\n", ret);
> +		goto err;
> +	}

I'd expect to see runtime PM callbacks to enable and disable this clock
in order to minimise power consumption.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[leilk liu]

Thanks for your review.

On Fri, 2015-05-08 at 18:53 +0100, Mark Brown wrote:
> On Fri, May 08, 2015 at 04:55:42PM +0800, leilk.liu@mediatek.com wrote:
> > From: Leilk Liu <leilk.liu@mediatek.com>
> > 
> > This patch adds basic spi bus for MT8173.
> 
> Please try to only CC relevant people on patches, you've got a very
> broad CC list here and I'm not sure I understand why everyone is on it.
> Sending people irrelevant patches adds to the volume of mail people have
> to handle which takes time away from other things.
> 

I use scripts/get_maintainer.pl on the patches and keep all maintainers.
I'll further refine the list in the future.

> > +config SPI_MT65XX
> > +	tristate "MediaTek SPI controller"
> > +	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	select SPI_BITBANG
> 
> Unless your controller is geniunely bitbanging you shouldn't be using
> the bitbang framework, modern drivers should implement set_cs() and
> transfer_one() instead.  You should also be using the core helpers for
> DMA mapping.
> 

I will implement set_cs() and transfer_one() instead in next patch.

Could you tell me more details about "You should also be using the core
helpers for DMA mapping"?

> The driver looks basically good other than this, it shouldn't be too
> much work (and ought to save you some code) to refactor to the modern
> interfaces.
> 
> > +#define IDLE 0
> > +#define INPROGRESS 1
> > +#define PAUSED 2
> > +
> > +#define PACKET_SIZE 1024
> 
> You should namespace the constants you're using to avoid clashes with
> headers.
> 
> > +static const struct of_device_id mtk_spi_of_match[] = {
> > +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);
> 
> There were three compatible strings listed in the DT binding but only
> two here.
> 

MT6589 and MT8135 is compatible; 
For MT8135 IC, it can use the follow way in dts to probe:
  compatible = "mediatek,mt8135-spi", 
	       "mediatek,mt6589-spi";
And I test it's ok on MT8135 platform. So I add struct of_device_id
mtk_spi_of_match like this in spi driver code.

> > +	/*set the software reset bit in SPI_CMD_REG.*/
> 
> /* Spaces please */
> 
> > +static unsigned long mtk_get_device_prop(struct platform_device *pdev)
> > +{
> > +	const struct of_device_id *match;
> > +
> > +	match = of_match_node(mtk_spi_of_match, pdev->dev.of_node);
> > +	return (unsigned long)match->data;
> > +}
> 
> This wrapper doesn't seem to be doing a lot and will crash if we somehow
> manage to get the driver loaded without a match (eg, if someone tries to
> instantiate as a regular platform device).
> 
> > +	ret = clk_prepare_enable(mdata->clk);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to enable clock (%d)\n", ret);
> > +		goto err;
> > +	}
> 
> I'd expect to see runtime PM callbacks to enable and disable this clock
> in order to minimise power consumption.

In the case of other review comments, I will also fix them in next/OK.

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1203 bytes --]

On Tue, May 12, 2015 at 08:39:16PM +0800, leilk liu wrote:
> On Fri, 2015-05-08 at 18:53 +0100, Mark Brown wrote:
> > On Fri, May 08, 2015 at 04:55:42PM +0800, leilk.liu@mediatek.com wrote:

> Could you tell me more details about "You should also be using the core
> helpers for DMA mapping"?

Implement can_dma() - look for drivers providing that for examples.

> > > +static const struct of_device_id mtk_spi_of_match[] = {
> > > +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > > +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> > > +	{}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);

> > There were three compatible strings listed in the DT binding but only
> > two here.

> MT6589 and MT8135 is compatible; 
> For MT8135 IC, it can use the follow way in dts to probe:
>   compatible = "mediatek,mt8135-spi", 
> 	       "mediatek,mt6589-spi";

> And I test it's ok on MT8135 platform. So I add struct of_device_id
> mtk_spi_of_match like this in spi driver code.

You should list all the compatibles documented in the binding here, if
some of them are the same just have them map to a single constant.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Yingjoe Chen]

On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:
> On Tue, May 12, 2015 at 08:39:16PM +0800, leilk liu wrote:
<...>
> > > > +static const struct of_device_id mtk_spi_of_match[] = {
> > > > +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > > > +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> > > > +	{}
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);
> 
> > > There were three compatible strings listed in the DT binding but only
> > > two here.
> 
> > MT6589 and MT8135 is compatible; 
> > For MT8135 IC, it can use the follow way in dts to probe:
> >   compatible = "mediatek,mt8135-spi", 
> > 	       "mediatek,mt6589-spi";
> 
> > And I test it's ok on MT8135 platform. So I add struct of_device_id
> > mtk_spi_of_match like this in spi driver code.
> 
> You should list all the compatibles documented in the binding here, if
> some of them are the same just have them map to a single constant.

Hi Mark,

Just for clarification. If we want to add spi support for a new soc, say
mt8127, which we think is compatible to mt6589. Since it may turn out we
need special handling for this soc latter, it is suggested to write
compatible like this in mt8127.dtsi:

   compatible = "mediatek,mt8127-spi",
 	        "mediatek,mt6589-spi";

Device tree binding should list all possible compatible string in .dts,
so we'll have to add that to binding as well.

     - mediatek,mt6589-spi: for mt6589 platforms
+    - mediatek,mt8127-spi: for mt8127 platforms

Then we'll also need to add this to source code to reflect this.

	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
+	{ .compatible = "mediatek,mt8127-spi", .data = (void *)COMPAT_MT6589},

This seems to introduce lots of trivial patches just to add a compatible
device to me...

Joe.C

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

On Wed, May 13, 2015 at 05:26:06PM +0800, Yingjoe Chen wrote:
> On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:

> > > > There were three compatible strings listed in the DT binding but only
> > > > two here.

> > > And I test it's ok on MT8135 platform. So I add struct of_device_id
> > > mtk_spi_of_match like this in spi driver code.

> > You should list all the compatibles documented in the binding here, if
> > some of them are the same just have them map to a single constant.

> Just for clarification. If we want to add spi support for a new soc, say
> mt8127, which we think is compatible to mt6589. Since it may turn out we
> need special handling for this soc latter, it is suggested to write
> compatible like this in mt8127.dtsi:

>    compatible = "mediatek,mt8127-spi",
>  	        "mediatek,mt6589-spi";

> Device tree binding should list all possible compatible string in .dts,
> so we'll have to add that to binding as well.

>      - mediatek,mt6589-spi: for mt6589 platforms
> +    - mediatek,mt8127-spi: for mt8127 platforms

> Then we'll also need to add this to source code to reflect this.

> 	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> +	{ .compatible = "mediatek,mt8127-spi", .data = (void *)COMPAT_MT6589},

> This seems to introduce lots of trivial patches just to add a compatible
> device to me...

Yes, but that's how DT works and as you say these patches are all
trivial so it's not like they take any appreciable effort.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Yingjoe Chen]

On Wed, 2015-05-13 at 12:10 +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 05:26:06PM +0800, Yingjoe Chen wrote:
> > On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:
> 
> > > > > There were three compatible strings listed in the DT binding but only
> > > > > two here.
> 
> > > > And I test it's ok on MT8135 platform. So I add struct of_device_id
> > > > mtk_spi_of_match like this in spi driver code.
> 
> > > You should list all the compatibles documented in the binding here, if
> > > some of them are the same just have them map to a single constant.
> 
> > Just for clarification. If we want to add spi support for a new soc, say
> > mt8127, which we think is compatible to mt6589. Since it may turn out we
> > need special handling for this soc latter, it is suggested to write
> > compatible like this in mt8127.dtsi:
> 
> >    compatible = "mediatek,mt8127-spi",
> >  	        "mediatek,mt6589-spi";
> 
> > Device tree binding should list all possible compatible string in .dts,
> > so we'll have to add that to binding as well.
> 
> >      - mediatek,mt6589-spi: for mt6589 platforms
> > +    - mediatek,mt8127-spi: for mt8127 platforms
> 
> > Then we'll also need to add this to source code to reflect this.
> 
> > 	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > +	{ .compatible = "mediatek,mt8127-spi", .data = (void *)COMPAT_MT6589},
> 
> > This seems to introduce lots of trivial patches just to add a compatible
> > device to me...
> 
> Yes, but that's how DT works and as you say these patches are all
> trivial so it's not like they take any appreciable effort.

Yes, that's shouldn't be a big effort.
Thanks for clarification.

Joe.C

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[leilk liu]

Dear Mark,

Thanks for your reply.

On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:
> On Tue, May 12, 2015 at 08:39:16PM +0800, leilk liu wrote:
> > On Fri, 2015-05-08 at 18:53 +0100, Mark Brown wrote:
> > > On Fri, May 08, 2015 at 04:55:42PM +0800, leilk.liu@mediatek.com wrote:
> 
> > Could you tell me more details about "You should also be using the core
> > helpers for DMA mapping"?
> 
> Implement can_dma() - look for drivers providing that for examples.
> 

MTK spi hardware uses the dmaengine in spi controller. According to
datasheet, spi driver just need to enable dma register bit and write a
physical address to relevant dma address register, so I think it may be
complex while the driver supports can_dma.

> > > > +static const struct of_device_id mtk_spi_of_match[] = {
> > > > +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > > > +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> > > > +	{}
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);
> 
> > > There were three compatible strings listed in the DT binding but only
> > > two here.
> 
> > MT6589 and MT8135 is compatible; 
> > For MT8135 IC, it can use the follow way in dts to probe:
> >   compatible = "mediatek,mt8135-spi", 
> > 	       "mediatek,mt6589-spi";
> 
> > And I test it's ok on MT8135 platform. So I add struct of_device_id
> > mtk_spi_of_match like this in spi driver code.
> 
> You should list all the compatibles documented in the binding here, if
> some of them are the same just have them map to a single constant.

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On Fri, May 15, 2015 at 03:38:42PM +0800, leilk liu wrote:
> On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:

> > Implement can_dma() - look for drivers providing that for examples.

> MTK spi hardware uses the dmaengine in spi controller. According to
> datasheet, spi driver just need to enable dma register bit and write a
> physical address to relevant dma address register, so I think it may be
> complex while the driver supports can_dma.

That's how a very large proportion of devices that work with DMA are
done - why would this be complicated?  All can_dma() does is report if
DMA is possible.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Eddie Huang]

Hi Mark,

On Fri, 2015-05-15 at 17:25 +0800, Mark Brown wrote:
> On Fri, May 15, 2015 at 03:38:42PM +0800, leilk liu wrote:
> > On Tue, 2015-05-12 at 17:05 +0100, Mark Brown wrote:
> 
> > > Implement can_dma() - look for drivers providing that for examples.
> 
> > MTK spi hardware uses the dmaengine in spi controller. According to
> > datasheet, spi driver just need to enable dma register bit and write a
> > physical address to relevant dma address register, so I think it may be
> > complex while the driver supports can_dma.
> 
> That's how a very large proportion of devices that work with DMA are
> done - why would this be complicated?  All can_dma() does is report if
> DMA is possible.

In include/linux/spi/spi.h, it describes if can_dma() exists and returns
true, dma_tx and dma_rx must be set.But Medaitek SPI controller has its
own dma hardware, which means this dma resides in the same base address
range with SPI controller, and only used by SPI, so we don't implement
generic DMA driver, such that can't provide dma channel and assign to
dmx_tx, dmx_rx parameter. We think it's strange to implement generic dma
driver for dma that only used by specific hardware.Can we just provide
can_dma() function and return false ? But I think it's a little odd that
there actually has dma. So can we just skip can_dma() function let it be
NULL ?

Eddie
Thanks
 

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1461 bytes --]

On Mon, Jun 08, 2015 at 06:15:46PM +0800, Eddie Huang wrote:
> On Fri, 2015-05-15 at 17:25 +0800, Mark Brown wrote:

> > That's how a very large proportion of devices that work with DMA are
> > done - why would this be complicated?  All can_dma() does is report if
> > DMA is possible.

> In include/linux/spi/spi.h, it describes if can_dma() exists and returns
> true, dma_tx and dma_rx must be set.But Medaitek SPI controller has its
> own dma hardware, which means this dma resides in the same base address
> range with SPI controller, and only used by SPI, so we don't implement
> generic DMA driver, such that can't provide dma channel and assign to
> dmx_tx, dmx_rx parameter. We think it's strange to implement generic dma
> driver for dma that only used by specific hardware.Can we just provide
> can_dma() function and return false ? But I think it's a little odd that
> there actually has dma. So can we just skip can_dma() function let it be
> NULL ?

If it's simply the unavailbility of a struct dma_chan we must be able to
get a better solution than just open coding all the DMA mapping and
unmapping in the driver.  The only thing we actually use the channel for
is to get the device we need to use to do the mapping and unmapping,
either we need a way for devices to provide their own channels easily or
a way for SPI drivers to specify a device here instead of a channel.
The latter seems easier if a bit clunky (with having to work with both).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Eddie Huang]

Hi Mark,

On Mon, 2015-06-08 at 18:59 +0100, Mark Brown wrote:
> On Mon, Jun 08, 2015 at 06:15:46PM +0800, Eddie Huang wrote:
> > On Fri, 2015-05-15 at 17:25 +0800, Mark Brown wrote:
> 
> > > That's how a very large proportion of devices that work with DMA are
> > > done - why would this be complicated?  All can_dma() does is report if
> > > DMA is possible.
> 
> > In include/linux/spi/spi.h, it describes if can_dma() exists and returns
> > true, dma_tx and dma_rx must be set.But Medaitek SPI controller has its
> > own dma hardware, which means this dma resides in the same base address
> > range with SPI controller, and only used by SPI, so we don't implement
> > generic DMA driver, such that can't provide dma channel and assign to
> > dmx_tx, dmx_rx parameter. We think it's strange to implement generic dma
> > driver for dma that only used by specific hardware.Can we just provide
> > can_dma() function and return false ? But I think it's a little odd that
> > there actually has dma. So can we just skip can_dma() function let it be
> > NULL ?
> 
> If it's simply the unavailbility of a struct dma_chan we must be able to
> get a better solution than just open coding all the DMA mapping and
> unmapping in the driver.  The only thing we actually use the channel for
> is to get the device we need to use to do the mapping and unmapping,
> either we need a way for devices to provide their own channels easily or
> a way for SPI drivers to specify a device here instead of a channel.
> The latter seems easier if a bit clunky (with having to work with both).

I list two ways you mention.
Pesudo code of your first suggestion:

static int mtk_spi_probe(struct platform_device *pdev) {
  struct dma_chan *tx_chan;
  struct dma_device *tx_dma;

  tx_chan = devm_kzalloc(&pdev->dev, sizeof(*tx_chan), GFP_KERNEL);
  tx_dma = devm_kzalloc(&pdev->dev, sizeof(*tx_dma), GFP_KERNEL);

  tx_dma->dev = &pdev->dev;
  tx_chan->device = tx_dma;
  master->dma_tx = tx_chan;
  ...
}

Modification of your second suggestion:

--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -539,8 +539,8 @@ static int __spi_map_msg(struct spi_master *master,
struct spi_message *msg)
        if (!master->can_dma)
                return 0;

-       tx_dev = master->dma_tx->device->dev;
-       rx_dev = master->dma_rx->device->dev;
+       tx_dev = master->dma_tx ? master->dma_tx->device->dev :
master->dev;
+       rx_dev = master->dma_rx ? master->dma_rx->device->dev : master-


Is this what you want ? Actually, I don't like first one at all.


Eddie
Thanks

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

On Tue, Jun 09, 2015 at 06:05:21PM +0800, Eddie Huang wrote:

> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -539,8 +539,8 @@ static int __spi_map_msg(struct spi_master *master,
> struct spi_message *msg)
>         if (!master->can_dma)
>                 return 0;

> -       tx_dev = master->dma_tx->device->dev;
> -       rx_dev = master->dma_rx->device->dev;
> +       tx_dev = master->dma_tx ? master->dma_tx->device->dev :
> master->dev;
> +       rx_dev = master->dma_rx ? master->dma_rx->device->dev : master-

> Is this what you want ? Actually, I don't like first one at all.

Not quite what I'd been thinking of - we can't just pick the device in
the core safely, the device might be a MFD or have some other
restriction that needs us to use a separate struct device.  However most
of those cases are likely to point towards implementing a dmaengine
device so probably the above will work for most cases and is fine.  Can
you send a proper patch please?

Please don't use the ternery operator, though.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Eddie Huang]

Hi Mark,

On Tue, 2015-06-09 at 11:39 +0100, Mark Brown wrote:
> On Tue, Jun 09, 2015 at 06:05:21PM +0800, Eddie Huang wrote:
> 
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -539,8 +539,8 @@ static int __spi_map_msg(struct spi_master *master,
> > struct spi_message *msg)
> >         if (!master->can_dma)
> >                 return 0;
> 
> > -       tx_dev = master->dma_tx->device->dev;
> > -       rx_dev = master->dma_rx->device->dev;
> > +       tx_dev = master->dma_tx ? master->dma_tx->device->dev :
> > master->dev;
> > +       rx_dev = master->dma_rx ? master->dma_rx->device->dev : master-
> 
> > Is this what you want ? Actually, I don't like first one at all.
> 
> Not quite what I'd been thinking of - we can't just pick the device in
> the core safely, the device might be a MFD or have some other
> restriction that needs us to use a separate struct device.  However most
> of those cases are likely to point towards implementing a dmaengine
> device so probably the above will work for most cases and is fine.  Can
> you send a proper patch please?

Sure, we will send this with new MTK SPI driver, such that can verify
it.

> 
> Please don't use the ternery operator, though.
OK, will fix it

Eddie
Thanks

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Eddie Huang]

Hi Mark,

On Wed, 2015-06-10 at 16:06 +0800, Eddie Huang wrote:
> On Tue, 2015-06-09 at 11:39 +0100, Mark Brown wrote:
> > On Tue, Jun 09, 2015 at 06:05:21PM +0800, Eddie Huang wrote:
> > 
> > > --- a/drivers/spi/spi.c
> > > +++ b/drivers/spi/spi.c
> > > @@ -539,8 +539,8 @@ static int __spi_map_msg(struct spi_master *master,
> > > struct spi_message *msg)
> > >         if (!master->can_dma)
> > >                 return 0;
> > 
> > > -       tx_dev = master->dma_tx->device->dev;
> > > -       rx_dev = master->dma_rx->device->dev;
> > > +       tx_dev = master->dma_tx ? master->dma_tx->device->dev :
> > > master->dev;
> > > +       rx_dev = master->dma_rx ? master->dma_rx->device->dev : master-
> > 
> > > Is this what you want ? Actually, I don't like first one at all.
> > 
> > Not quite what I'd been thinking of - we can't just pick the device in
> > the core safely, the device might be a MFD or have some other
> > restriction that needs us to use a separate struct device.  However most
> > of those cases are likely to point towards implementing a dmaengine
> > device so probably the above will work for most cases and is fine.  Can
> > you send a proper patch please?
> 

After further investigate, we found problem when use can_dma() to
implement spi driver.

If can_dma() return true in __spi_map_msg() function, it will map data
buffer to sg_table, then pass this sg_table to transfer function. In
transfer function, spi hardware can do tx and rx at the same time (full
duplex), and the data size must be the same.

Here comes the problem, although total length of tx, rx is the same,
each entry in rx and tx scatterlist may not be the same (in the case
data buffer allocate from vmalloc). Other vendor have dmaengine driver
to send entry-by-entry automatically, so it is ok due to total length is
the same.But mediatek hw can only send tx and rx scatterlist entry one
by on, which means in full duplex, mediatek SPI hardware need send each
entry separately, will cause full duplex fail because tx/rx entry size
or entry number may not be the same.

The problem is not dma map discuss earlier, it is mediatek spi hardware
limitation that can't support scatterlist dma transfer in full-duplex
mode. We can only support both tx and rx has the same size continuous
memory data in full-duplex mode. I don't know whether should modify
spi.c to support mediatek spi, or we just return can_dma() false and do
transfer one continuous data in transfer function.

By the way, I think maybe it is better to change can_dma() to
can_dma_sg().

Eddie
Thanks

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

On Wed, Jun 17, 2015 at 05:08:03PM +0800, Eddie Huang wrote:

> Here comes the problem, although total length of tx, rx is the same,
> each entry in rx and tx scatterlist may not be the same (in the case
> data buffer allocate from vmalloc). Other vendor have dmaengine driver
> to send entry-by-entry automatically, so it is ok due to total length is
> the same.But mediatek hw can only send tx and rx scatterlist entry one
> by on, which means in full duplex, mediatek SPI hardware need send each
> entry separately, will cause full duplex fail because tx/rx entry size
> or entry number may not be the same.

I don't see why this is a problem - your driver is going to have to do
more work to overcome the limitations of the hardware but surely it's
just a question of how you parse the scatterlists (or rewriting them if
that's appropriate)?

> The problem is not dma map discuss earlier, it is mediatek spi hardware
> limitation that can't support scatterlist dma transfer in full-duplex
> mode. We can only support both tx and rx has the same size continuous
> memory data in full-duplex mode. I don't know whether should modify
> spi.c to support mediatek spi, or we just return can_dma() false and do
> transfer one continuous data in transfer function.

> By the way, I think maybe it is better to change can_dma() to
> can_dma_sg().

Don't you just need to handle the scatterlists in page sized chunks
here?  There's nothing about passing information about the memory that
was mapped around in a scatterlist that means you have to pass the whole
list to the hardware at once - at heart the scatterlist is just a
convenient structure for passing around where the data to be transferred
is.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Eddie Huang]

On Wed, 2015-06-17 at 13:47 +0100, Mark Brown wrote:
> On Wed, Jun 17, 2015 at 05:08:03PM +0800, Eddie Huang wrote:
> 
> > Here comes the problem, although total length of tx, rx is the same,
> > each entry in rx and tx scatterlist may not be the same (in the case
> > data buffer allocate from vmalloc). Other vendor have dmaengine driver
> > to send entry-by-entry automatically, so it is ok due to total length is
> > the same.But mediatek hw can only send tx and rx scatterlist entry one
> > by on, which means in full duplex, mediatek SPI hardware need send each
> > entry separately, will cause full duplex fail because tx/rx entry size
> > or entry number may not be the same.
> 
> I don't see why this is a problem - your driver is going to have to do
> more work to overcome the limitations of the hardware but surely it's
> just a question of how you parse the scatterlists (or rewriting them if
> that's appropriate)?
> 
> > The problem is not dma map discuss earlier, it is mediatek spi hardware
> > limitation that can't support scatterlist dma transfer in full-duplex
> > mode. We can only support both tx and rx has the same size continuous
> > memory data in full-duplex mode. I don't know whether should modify
> > spi.c to support mediatek spi, or we just return can_dma() false and do
> > transfer one continuous data in transfer function.
> 
> > By the way, I think maybe it is better to change can_dma() to So 
> > can_dma_sg().
> 
> Don't you just need to handle the scatterlists in page sized chunks
> here?  There's nothing about passing information about the memory that
> was mapped around in a scatterlist that means you have to pass the whole
> list to the hardware at once - at heart the scatterlist is just a
> convenient structure for passing around where the data to be transferred
> is.

Our hardware limitation is: we don't have separate dma tx, rx channel
with transfer finish interrupt, only have spi trigger operation.So the
mediatek SPI dma full duplex operation steps are:
1. Set TX DMA address.
2. Set RX DMA address.
3. Set length (this step assume TX, RX are the same size).
4. Set TX DMA enable, RX DMA enable bit in spi config register. (not
trigger DMA, just told spi use dma)
5. Trigger spi operations.
6. Wait spi operations finish interrupt.

If tx scatterlist per list data size are 128, 4096, 256. rx scatterlist
per list data size are 128, 4096, 256. So we need to go through above
steps three times. If tx scatterlists per list data size are 128, 4096,
256. rx scatterlists per list data size are 256, 4096, 128. If we start
sending first entry, tx size is 128, rx size is 256, this will cause
hardware malfunction because tx, rx data length are not the same.

The solution I think is copy scatterlist data into one single buffer in
mediatek spi transfer function, but I think this is odd because
__spi_map_msg() map single buffer into scatterlist, then our driver map
scatterlist into single buffer again. I hope this explaination is more
clear than before.

Eddie
Thanks

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1800 bytes --]

On Wed, Jun 17, 2015 at 10:10:51PM +0800, Eddie Huang wrote:

> Our hardware limitation is: we don't have separate dma tx, rx channel
> with transfer finish interrupt, only have spi trigger operation.So the
> mediatek SPI dma full duplex operation steps are:
> 1. Set TX DMA address.
> 2. Set RX DMA address.
> 3. Set length (this step assume TX, RX are the same size).
> 4. Set TX DMA enable, RX DMA enable bit in spi config register. (not
> trigger DMA, just told spi use dma)
> 5. Trigger spi operations.
> 6. Wait spi operations finish interrupt.

Sure, that's what I understood.

> If tx scatterlist per list data size are 128, 4096, 256. rx scatterlist
> per list data size are 128, 4096, 256. So we need to go through above
> steps three times. If tx scatterlists per list data size are 128, 4096,
> 256. rx scatterlists per list data size are 256, 4096, 128. If we start
> sending first entry, tx size is 128, rx size is 256, this will cause
> hardware malfunction because tx, rx data length are not the same.

> The solution I think is copy scatterlist data into one single buffer in
> mediatek spi transfer function, but I think this is odd because
> __spi_map_msg() map single buffer into scatterlist, then our driver map
> scatterlist into single buffer again. I hope this explaination is more
> clear than before.

To repeat what I said in my last mail: there's no need to use the
scatterlists as-is, your driver can do whatever set of DMA transfers it
likes to keep the lengths of each transfer the same.  Attempting to
linearise the transfers in memory isn't going to work unless you
allocate physically contiguous memory (which could get painful) and will
add substantial overhead.

For example with your above example you could split the transfers up to
be 128, 128, 3968, 128, 128.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

[Eddie Huang]

On Wed, 2015-06-17 at 17:35 +0100, Mark Brown wrote:
> On Wed, Jun 17, 2015 at 10:10:51PM +0800, Eddie Huang wrote:
> 
> > Our hardware limitation is: we don't have separate dma tx, rx channel
> > with transfer finish interrupt, only have spi trigger operation.So the
> > mediatek SPI dma full duplex operation steps are:
> > 1. Set TX DMA address.
> > 2. Set RX DMA address.
> > 3. Set length (this step assume TX, RX are the same size).
> > 4. Set TX DMA enable, RX DMA enable bit in spi config register. (not
> > trigger DMA, just told spi use dma)
> > 5. Trigger spi operations.
> > 6. Wait spi operations finish interrupt.
> 
> Sure, that's what I understood.
> 
> > If tx scatterlist per list data size are 128, 4096, 256. rx scatterlist
> > per list data size are 128, 4096, 256. So we need to go through above
> > steps three times. If tx scatterlists per list data size are 128, 4096,
> > 256. rx scatterlists per list data size are 256, 4096, 128. If we start
> > sending first entry, tx size is 128, rx size is 256, this will cause
> > hardware malfunction because tx, rx data length are not the same.
> 
> > The solution I think is copy scatterlist data into one single buffer in
> > mediatek spi transfer function, but I think this is odd because
> > __spi_map_msg() map single buffer into scatterlist, then our driver map
> > scatterlist into single buffer again. I hope this explaination is more
> > clear than before.
> 
> To repeat what I said in my last mail: there's no need to use the
> scatterlists as-is, your driver can do whatever set of DMA transfers it
> likes to keep the lengths of each transfer the same.  Attempting to
> linearise the transfers in memory isn't going to work unless you
> allocate physically contiguous memory (which could get painful) and will
> add substantial overhead.
> 
> For example with your above example you could split the transfers up to
> be 128, 128, 3968, 128, 128.

This is a workable way.
Thanks your suggestion.We will try to implement this,

Eddie
Thanks