From 82bced99683343899dea4634452940b15d6374eb Mon Sep 17 00:00:00 2001
From: Leilk Liu <[email protected]>
Date: Mon, 17 Sep 2018 10:07:45 +0800
Subject: [PATCH v3 0/3] Add Mediatek SPI slave driver
v3:
1. Fix Rob Herring review comment about bindings.
2. remove unused variables in driver.
v2:
1. Fix Sean Wang review comment.
2. remove unused register and offset define.
3. update bindings file with assigned-clocks and assigned-clocks-parents.
v1:
This series are based on 4.19-rc1 and provide three patches to add mediatek spi slave driver.
Leilk Liu (3):
spis: mediatek: add bindings for Mediatek MT2712 soc platform
spis: mediatek: add spi slave for Mediatek MT2712
arm64: dts: Add spi slave dts
.../devicetree/bindings/spi/spi-slave-mt27xx.txt | 32 ++
arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 11 +
drivers/spi/Kconfig | 8 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-slave-mt27xx.c | 553 +++++++++++++++++++++
5 files changed, 605 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
create mode 100644 drivers/spi/spi-slave-mt27xx.c
--
1.9.1
This patch adds MT2712 spi slave into device tree.
Signed-off-by: Leilk Liu <[email protected]>
---
arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
index 75cc0f7..ee627a7 100644
--- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
@@ -301,6 +301,17 @@
status = "disabled";
};
+ spis1: spi@10013000 {
+ compatible = "mediatek,mt2712-spi-slave";
+ reg = <0 0x10013000 0 0x100>;
+ interrupts = <GIC_SPI 283 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&infracfg CLK_INFRA_AO_SPI1>;
+ clock-names = "spi";
+ assigned-clocks = <&topckgen CLK_TOP_SPISLV_SEL>;
+ assigned-clock-parents = <&topckgen CLK_TOP_UNIVPLL1_D2>;
+ status = "disabled";
+ };
+
apmixedsys: syscon@10209000 {
compatible = "mediatek,mt2712-apmixedsys", "syscon";
reg = <0 0x10209000 0 0x1000>;
--
1.7.9.5
This patch adds a DT binding documentation for the MT2712 soc.
Signed-off-by: Leilk Liu <[email protected]>
---
.../devicetree/bindings/spi/spi-slave-mt27xx.txt | 32 ++++++++++++++++++++
1 file changed, 32 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
diff --git a/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
new file mode 100644
index 0000000..09cb2c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
@@ -0,0 +1,32 @@
+Binding for MTK SPI Slave controller
+
+Required properties:
+- compatible: should be one of the following.
+ - mediatek,mt2712-spi-slave: for mt2712 platforms
+- reg: Address and length of the register set for the device.
+- interrupts: Should contain spi interrupt.
+- clocks: phandles to input clocks.
+ It's clock gate, and should be <&infracfg CLK_INFRA_AO_SPI1>.
+- clock-names: should be "spi" for the clock gate.
+
+Optional properties:
+- assigned-clocks: it's mux clock, should be <&topckgen CLK_TOP_SPISLV_SEL>.
+- assigned-clock-parents: parent of mux clock.
+ It's PLL, and should be on of the following.
+ - <&topckgen CLK_TOP_UNIVPLL1_D2>: specify parent clock 312MHZ.
+ It's the default one.
+ - <&topckgen CLK_TOP_UNIVPLL1_D4>: specify parent clock 156MHZ.
+ - <&topckgen CLK_TOP_UNIVPLL2_D4>: specify parent clock 104MHZ.
+ - <&topckgen CLK_TOP_UNIVPLL1_D8>: specify parent clock 78MHZ.
+
+Example:
+- SoC Specific Portion:
+spis1: spi@10013000 {
+ compatible = "mediatek,mt2712-spi-slave";
+ reg = <0 0x10013000 0 0x100>;
+ interrupts = <GIC_SPI 283 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&infracfg CLK_INFRA_AO_SPI1>;
+ clock-names = "spi";
+ assigned-clocks = <&topckgen CLK_TOP_SPISLV_SEL>;
+ assigned-clock-parents = <&topckgen CLK_TOP_UNIVPLL1_D2>;
+};
--
1.7.9.5
This patchs add basic spi slave for MT2712.
Signed-off-by: Leilk Liu <[email protected]>
---
drivers/spi/Kconfig | 8 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-slave-mt27xx.c | 553 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 562 insertions(+)
create mode 100644 drivers/spi/spi-slave-mt27xx.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 671d078..86ef6a5 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -802,6 +802,14 @@ config SPI_SLAVE
slave protocol handlers.
if SPI_SLAVE
+config SPI_SLAVE_MT27XX
+ tristate "MediaTek SPI slave device"
+ depends on ARCH_MEDIATEK || COMPILE_TEST
+ help
+ This selects the MediaTek(R) SPI slave device driver.
+ If you want to use MediaTek(R) SPI slave interface,
+ say Y or M here.If you are not sure, say N.
+ SPI slave drivers for Mediatek MT27XX series ARM SoCs.
config SPI_SLAVE_TIME
tristate "SPI slave handler reporting boot up time"
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index a90d559..a5e7b3c 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -109,5 +109,6 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA) += spi-xtensa-xtfpga.o
obj-$(CONFIG_SPI_ZYNQMP_GQSPI) += spi-zynqmp-gqspi.o
# SPI slave protocol handlers
+obj-$(CONFIG_SPI_SLAVE_MT27XX) += spi-slave-mt27xx.o
obj-$(CONFIG_SPI_SLAVE_TIME) += spi-slave-time.o
obj-$(CONFIG_SPI_SLAVE_SYSTEM_CONTROL) += spi-slave-system-control.o
diff --git a/drivers/spi/spi-slave-mt27xx.c b/drivers/spi/spi-slave-mt27xx.c
new file mode 100644
index 0000000..2e59339
--- /dev/null
+++ b/drivers/spi/spi-slave-mt27xx.c
@@ -0,0 +1,553 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (c) 2018 MediaTek Inc.
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/spi/spi.h>
+
+#define SPIS_IRQ_EN_REG 0x0
+#define SPIS_IRQ_CLR_REG 0x4
+#define SPIS_IRQ_ST_REG 0x8
+#define SPIS_IRQ_MASK_REG 0xc
+#define SPIS_CFG_REG 0x10
+#define SPIS_RX_DATA_REG 0x14
+#define SPIS_TX_DATA_REG 0x18
+#define SPIS_RX_DST_REG 0x1c
+#define SPIS_TX_SRC_REG 0x20
+#define SPIS_DMA_CFG_REG 0x30
+#define SPIS_SOFT_RST_REG 0x40
+
+/* SPIS_IRQ_EN_REG */
+#define DMA_DONE_EN BIT(7)
+#define DATA_DONE_EN BIT(2)
+#define RSTA_DONE_EN BIT(1)
+#define CMD_INVALID_EN BIT(0)
+
+/* SPIS_IRQ_ST_REG */
+#define DMA_DONE_ST BIT(7)
+#define DATA_DONE_ST BIT(2)
+#define RSTA_DONE_ST BIT(1)
+#define CMD_INVALID_ST BIT(0)
+
+/* SPIS_IRQ_MASK_REG */
+#define DMA_DONE_MASK BIT(7)
+#define DATA_DONE_MASK BIT(2)
+#define RSTA_DONE_MASK BIT(1)
+#define CMD_INVALID_MASK BIT(0)
+
+/* SPIS_CFG_REG */
+#define SPIS_TX_ENDIAN BIT(7)
+#define SPIS_RX_ENDIAN BIT(6)
+#define SPIS_TXMSBF BIT(5)
+#define SPIS_RXMSBF BIT(4)
+#define SPIS_CPHA BIT(3)
+#define SPIS_CPOL BIT(2)
+#define SPIS_TX_EN BIT(1)
+#define SPIS_RX_EN BIT(0)
+
+/* SPIS_DMA_CFG_REG */
+#define TX_DMA_TRIG_EN BIT(31)
+#define TX_DMA_EN BIT(30)
+#define RX_DMA_EN BIT(29)
+#define TX_DMA_LEN 0xfffff
+
+/* SPIS_SOFT_RST_REG */
+#define SPIS_DMA_ADDR_EN BIT(1)
+#define SPIS_SOFT_RST BIT(0)
+
+#define MTK_SPI_SLAVE_MAX_FIFO_SIZE 512U
+
+struct mtk_spi_slave {
+ struct device *dev;
+ void __iomem *base;
+ struct clk *spi_clk;
+ struct completion xfer_done;
+ struct spi_transfer *cur_transfer;
+ bool slave_aborted;
+};
+
+static const struct of_device_id mtk_spi_slave_of_match[] = {
+ { .compatible = "mediatek,mt2712-spi-slave", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mtk_spi_slave_of_match);
+
+static void mtk_spi_slave_disable_dma(struct mtk_spi_slave *mdata)
+{
+ u32 reg_val;
+
+ reg_val = readl(mdata->base + SPIS_DMA_CFG_REG);
+ reg_val &= ~RX_DMA_EN;
+ reg_val &= ~TX_DMA_EN;
+ writel(reg_val, mdata->base + SPIS_DMA_CFG_REG);
+}
+
+static void mtk_spi_slave_disable_xfer(struct mtk_spi_slave *mdata)
+{
+ u32 reg_val;
+
+ reg_val = readl(mdata->base + SPIS_CFG_REG);
+ reg_val &= ~SPIS_TX_EN;
+ reg_val &= ~SPIS_RX_EN;
+ writel(reg_val, mdata->base + SPIS_CFG_REG);
+}
+
+static int mtk_spi_slave_wait_for_completion(struct mtk_spi_slave *mdata)
+{
+ if (wait_for_completion_interruptible(&mdata->xfer_done) ||
+ mdata->slave_aborted) {
+ dev_err(mdata->dev, "interrupted\n");
+ return -EINTR;
+ }
+
+ return 0;
+}
+
+static int mtk_spi_slave_prepare_message(struct spi_controller *ctlr,
+ struct spi_message *msg)
+{
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+ struct spi_device *spi = msg->spi;
+ bool cpha, cpol;
+ u32 reg_val;
+
+ cpha = spi->mode & SPI_CPHA ? 1 : 0;
+ cpol = spi->mode & SPI_CPOL ? 1 : 0;
+
+ reg_val = readl(mdata->base + SPIS_CFG_REG);
+ if (cpha)
+ reg_val |= SPIS_CPHA;
+ else
+ reg_val &= ~SPIS_CPHA;
+ if (cpol)
+ reg_val |= SPIS_CPOL;
+ else
+ reg_val &= ~SPIS_CPOL;
+
+ if (spi->mode & SPI_LSB_FIRST)
+ reg_val &= ~(SPIS_TXMSBF | SPIS_RXMSBF);
+ else
+ reg_val |= SPIS_TXMSBF | SPIS_RXMSBF;
+
+ /* set the tx/rx endian */
+#ifdef __LITTLE_ENDIAN
+ reg_val &= ~SPIS_TX_ENDIAN;
+ reg_val &= ~SPIS_RX_ENDIAN;
+#else
+ reg_val |= SPIS_TX_ENDIAN;
+ reg_val |= SPIS_RX_ENDIAN;
+#endif
+ writel(reg_val, mdata->base + SPIS_CFG_REG);
+
+ return 0;
+}
+
+static int mtk_spi_slave_fifo_transfer(struct spi_controller *ctlr,
+ struct spi_device *spi,
+ struct spi_transfer *xfer)
+{
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+ int reg_val, cnt, remainder, ret;
+
+ writel(SPIS_SOFT_RST, mdata->base + SPIS_SOFT_RST_REG);
+
+ reg_val = readl(mdata->base + SPIS_CFG_REG);
+ if (xfer->rx_buf)
+ reg_val |= SPIS_RX_EN;
+ if (xfer->tx_buf)
+ reg_val |= SPIS_TX_EN;
+ writel(reg_val, mdata->base + SPIS_CFG_REG);
+
+ cnt = xfer->len / 4;
+ if (xfer->tx_buf)
+ iowrite32_rep(mdata->base + SPIS_TX_DATA_REG,
+ xfer->tx_buf, cnt);
+
+ remainder = xfer->len % 4;
+ if (xfer->tx_buf && remainder > 0) {
+ reg_val = 0;
+ memcpy(®_val, xfer->tx_buf + cnt * 4, remainder);
+ writel(reg_val, mdata->base + SPIS_TX_DATA_REG);
+ }
+
+ ret = mtk_spi_slave_wait_for_completion(mdata);
+ if (ret) {
+ mtk_spi_slave_disable_xfer(mdata);
+ writel(SPIS_SOFT_RST, mdata->base + SPIS_SOFT_RST_REG);
+ }
+
+ return ret;
+}
+
+static int mtk_spi_slave_dma_transfer(struct spi_controller *ctlr,
+ struct spi_device *spi,
+ struct spi_transfer *xfer)
+{
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+ struct device *dev = mdata->dev;
+ int reg_val, ret;
+
+ writel(SPIS_SOFT_RST, mdata->base + SPIS_SOFT_RST_REG);
+
+ if (xfer->tx_buf) {
+ xfer->tx_dma = dma_map_single(dev, (void *)xfer->tx_buf,
+ xfer->len, DMA_TO_DEVICE);
+ if (dma_mapping_error(dev, xfer->tx_dma)) {
+ ret = -ENOMEM;
+ goto disable_transfer;
+ }
+ }
+
+ if (xfer->rx_buf) {
+ xfer->rx_dma = dma_map_single(dev, (void *)xfer->rx_buf,
+ xfer->len, DMA_FROM_DEVICE);
+ if (dma_mapping_error(dev, xfer->rx_dma)) {
+ ret = -ENOMEM;
+ goto unmap_txdma;
+ }
+ }
+
+ writel(xfer->tx_dma, mdata->base + SPIS_TX_SRC_REG);
+ writel(xfer->rx_dma, mdata->base + SPIS_RX_DST_REG);
+
+ writel(SPIS_DMA_ADDR_EN, mdata->base + SPIS_SOFT_RST_REG);
+
+ /* enable config reg tx rx_enable */
+ reg_val = readl(mdata->base + SPIS_CFG_REG);
+ if (xfer->tx_buf)
+ reg_val |= SPIS_TX_EN;
+ if (xfer->rx_buf)
+ reg_val |= SPIS_RX_EN;
+ writel(reg_val, mdata->base + SPIS_CFG_REG);
+
+ /* config dma */
+ reg_val = 0;
+ reg_val |= (xfer->len - 1) & TX_DMA_LEN;
+ writel(reg_val, mdata->base + SPIS_DMA_CFG_REG);
+
+ reg_val = readl(mdata->base + SPIS_DMA_CFG_REG);
+ if (xfer->tx_buf)
+ reg_val |= TX_DMA_EN;
+ if (xfer->rx_buf)
+ reg_val |= RX_DMA_EN;
+ reg_val |= TX_DMA_TRIG_EN;
+ writel(reg_val, mdata->base + SPIS_DMA_CFG_REG);
+
+ ret = mtk_spi_slave_wait_for_completion(mdata);
+ if (ret)
+ goto unmap_rxdma;
+
+ return 0;
+
+unmap_rxdma:
+ if (xfer->rx_buf)
+ dma_unmap_single(dev, xfer->rx_dma,
+ xfer->len, DMA_FROM_DEVICE);
+
+unmap_txdma:
+ if (xfer->tx_buf)
+ dma_unmap_single(dev, xfer->tx_dma,
+ xfer->len, DMA_TO_DEVICE);
+
+disable_transfer:
+ mtk_spi_slave_disable_dma(mdata);
+ mtk_spi_slave_disable_xfer(mdata);
+ writel(SPIS_SOFT_RST, mdata->base + SPIS_SOFT_RST_REG);
+
+ return ret;
+}
+
+static int mtk_spi_slave_transfer_one(struct spi_controller *ctlr,
+ struct spi_device *spi,
+ struct spi_transfer *xfer)
+{
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+
+ reinit_completion(&mdata->xfer_done);
+ mdata->slave_aborted = false;
+ mdata->cur_transfer = xfer;
+
+ if (xfer->len > MTK_SPI_SLAVE_MAX_FIFO_SIZE)
+ return mtk_spi_slave_dma_transfer(ctlr, spi, xfer);
+ else
+ return mtk_spi_slave_fifo_transfer(ctlr, spi, xfer);
+}
+
+static int mtk_spi_slave_setup(struct spi_device *spi)
+{
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(spi->master);
+ u32 reg_val;
+
+ reg_val = DMA_DONE_EN | DATA_DONE_EN |
+ RSTA_DONE_EN | CMD_INVALID_EN;
+ writel(reg_val, mdata->base + SPIS_IRQ_EN_REG);
+
+ reg_val = DMA_DONE_MASK | DATA_DONE_MASK |
+ RSTA_DONE_MASK | CMD_INVALID_MASK;
+ writel(reg_val, mdata->base + SPIS_IRQ_MASK_REG);
+
+ mtk_spi_slave_disable_dma(mdata);
+ mtk_spi_slave_disable_xfer(mdata);
+
+ return 0;
+}
+
+static int mtk_slave_abort(struct spi_controller *ctlr)
+{
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+
+ mdata->slave_aborted = true;
+ complete(&mdata->xfer_done);
+
+ return 0;
+}
+
+static irqreturn_t mtk_spi_slave_interrupt(int irq, void *dev_id)
+{
+ struct spi_controller *ctlr = dev_id;
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+ struct spi_transfer *trans = mdata->cur_transfer;
+ u32 int_status, reg_val, cnt, remainder;
+
+ int_status = readl(mdata->base + SPIS_IRQ_ST_REG);
+ writel(int_status, mdata->base + SPIS_IRQ_CLR_REG);
+
+ if (!trans)
+ return IRQ_HANDLED;
+
+ if ((int_status & DMA_DONE_ST) &&
+ ((int_status & DATA_DONE_ST) ||
+ (int_status & RSTA_DONE_ST))) {
+ writel(SPIS_SOFT_RST, mdata->base + SPIS_SOFT_RST_REG);
+
+ if (trans->tx_buf)
+ dma_unmap_single(mdata->dev, trans->tx_dma,
+ trans->len, DMA_TO_DEVICE);
+ if (trans->rx_buf)
+ dma_unmap_single(mdata->dev, trans->rx_dma,
+ trans->len, DMA_FROM_DEVICE);
+
+ mtk_spi_slave_disable_dma(mdata);
+ mtk_spi_slave_disable_xfer(mdata);
+ }
+
+ if ((!(int_status & DMA_DONE_ST)) &&
+ ((int_status & DATA_DONE_ST) ||
+ (int_status & RSTA_DONE_ST))) {
+ cnt = trans->len / 4;
+ if (trans->rx_buf)
+ ioread32_rep(mdata->base + SPIS_RX_DATA_REG,
+ trans->rx_buf, cnt);
+ remainder = trans->len % 4;
+ if (trans->rx_buf && remainder > 0) {
+ reg_val = readl(mdata->base + SPIS_RX_DATA_REG);
+ memcpy(trans->rx_buf + (cnt * 4),
+ ®_val, remainder);
+ }
+
+ mtk_spi_slave_disable_xfer(mdata);
+ }
+
+ if (int_status & CMD_INVALID_ST)
+ dev_err(&ctlr->dev, "cmd invalid\n");
+
+ mdata->cur_transfer = NULL;
+ complete(&mdata->xfer_done);
+
+ return IRQ_HANDLED;
+}
+
+static int mtk_spi_slave_probe(struct platform_device *pdev)
+{
+ struct spi_controller *ctlr;
+ struct mtk_spi_slave *mdata;
+ struct resource *res;
+ int irq, ret;
+
+ ctlr = spi_alloc_slave(&pdev->dev, sizeof(*mdata));
+ if (!ctlr) {
+ dev_err(&pdev->dev, "failed to alloc spi slave\n");
+ return -ENOMEM;
+ }
+
+ ctlr->auto_runtime_pm = true;
+ ctlr->dev.of_node = pdev->dev.of_node;
+ ctlr->mode_bits = SPI_CPOL | SPI_CPHA;
+ ctlr->mode_bits |= SPI_LSB_FIRST;
+
+ ctlr->prepare_message = mtk_spi_slave_prepare_message;
+ ctlr->transfer_one = mtk_spi_slave_transfer_one;
+ ctlr->setup = mtk_spi_slave_setup;
+ ctlr->slave_abort = mtk_slave_abort;
+
+ mdata = spi_controller_get_devdata(ctlr);
+
+ platform_set_drvdata(pdev, ctlr);
+
+ init_completion(&mdata->xfer_done);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ ret = -ENODEV;
+ dev_err(&pdev->dev, "failed to determine base address\n");
+ goto err_put_ctlr;
+ }
+
+ mdata->dev = &pdev->dev;
+
+ mdata->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(mdata->base)) {
+ ret = PTR_ERR(mdata->base);
+ goto err_put_ctlr;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "failed to get irq (%d)\n", irq);
+ ret = irq;
+ goto err_put_ctlr;
+ }
+
+ ret = devm_request_irq(&pdev->dev, irq, mtk_spi_slave_interrupt,
+ IRQF_TRIGGER_NONE, dev_name(&pdev->dev), ctlr);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register irq (%d)\n", ret);
+ goto err_put_ctlr;
+ }
+
+ mdata->spi_clk = devm_clk_get(&pdev->dev, "spi");
+ if (IS_ERR(mdata->spi_clk)) {
+ ret = PTR_ERR(mdata->spi_clk);
+ dev_err(&pdev->dev, "failed to get spi-clk: %d\n", ret);
+ goto err_put_ctlr;
+ }
+
+ ret = clk_prepare_enable(mdata->spi_clk);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to enable spi_clk (%d)\n", ret);
+ goto err_put_ctlr;
+ }
+
+ pm_runtime_enable(&pdev->dev);
+
+ ret = devm_spi_register_controller(&pdev->dev, ctlr);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "failed to register slave controller(%d)\n", ret);
+ clk_disable_unprepare(mdata->spi_clk);
+ goto err_disable_runtime_pm;
+ }
+
+ clk_disable_unprepare(mdata->spi_clk);
+
+ return 0;
+
+err_disable_runtime_pm:
+ pm_runtime_disable(&pdev->dev);
+err_put_ctlr:
+ spi_controller_put(ctlr);
+
+ return ret;
+}
+
+static int mtk_spi_slave_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(&pdev->dev);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mtk_spi_slave_suspend(struct device *dev)
+{
+ struct spi_controller *ctlr = dev_get_drvdata(dev);
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+ int ret;
+
+ ret = spi_controller_suspend(ctlr);
+ if (ret)
+ return ret;
+
+ if (!pm_runtime_suspended(dev))
+ clk_disable_unprepare(mdata->spi_clk);
+
+ return ret;
+}
+
+static int mtk_spi_slave_resume(struct device *dev)
+{
+ struct spi_controller *ctlr = dev_get_drvdata(dev);
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+ int ret;
+
+ if (!pm_runtime_suspended(dev)) {
+ ret = clk_prepare_enable(mdata->spi_clk);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable spi_clk (%d)\n", ret);
+ return ret;
+ }
+ }
+
+ ret = spi_controller_resume(ctlr);
+ if (ret < 0)
+ clk_disable_unprepare(mdata->spi_clk);
+
+ return ret;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_PM
+static int mtk_spi_slave_runtime_suspend(struct device *dev)
+{
+ struct spi_controller *ctlr = dev_get_drvdata(dev);
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+
+ clk_disable_unprepare(mdata->spi_clk);
+
+ return 0;
+}
+
+static int mtk_spi_slave_runtime_resume(struct device *dev)
+{
+ struct spi_controller *ctlr = dev_get_drvdata(dev);
+ struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+ int ret;
+
+ ret = clk_prepare_enable(mdata->spi_clk);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable spi_clk (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops mtk_spi_slave_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(mtk_spi_slave_suspend, mtk_spi_slave_resume)
+ SET_RUNTIME_PM_OPS(mtk_spi_slave_runtime_suspend,
+ mtk_spi_slave_runtime_resume, NULL)
+};
+
+static struct platform_driver mtk_spi_slave_driver = {
+ .driver = {
+ .name = "mtk-spi-slave",
+ .pm = &mtk_spi_slave_pm,
+ .of_match_table = mtk_spi_slave_of_match,
+ },
+ .probe = mtk_spi_slave_probe,
+ .remove = mtk_spi_slave_remove,
+};
+
+module_platform_driver(mtk_spi_slave_driver);
+
+MODULE_DESCRIPTION("MTK SPI Slave Controller driver");
+MODULE_AUTHOR("Leilk Liu <[email protected]>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:mtk-spi-slave");
--
1.7.9.5
On Mon, Sep 17, 2018 at 10:19:21AM +0800, Leilk Liu wrote:
This looks overall pretty good, a few smallish comments below:
Please use subject lines matching the style for the subsystem. This
makes it easier for people to identify relevant patches.
> if SPI_SLAVE
> +config SPI_SLAVE_MT27XX
> + tristate "MediaTek SPI slave device"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + help
> + This selects the MediaTek(R) SPI slave device driver.
> + If you want to use MediaTek(R) SPI slave interface,
> + say Y or M here.If you are not sure, say N.
> + SPI slave drivers for Mediatek MT27XX series ARM SoCs.
>
> config SPI_SLAVE_TIME
This is a driver not a slave implementation, it should be with the other
drivers in the Kconfig. The slave implementations are for the
functionality that uses the drivers, not the drivers themselves.
> + /* set the tx/rx endian */
> +#ifdef __LITTLE_ENDIAN
> + reg_val &= ~SPIS_TX_ENDIAN;
> + reg_val &= ~SPIS_RX_ENDIAN;
> +#else
> + reg_val |= SPIS_TX_ENDIAN;
> + reg_val |= SPIS_RX_ENDIAN;
> +#endif
> + writel(reg_val, mdata->base + SPIS_CFG_REG);
This seems weird - it looks like it's changing the endianess of the
protocol based on the endianness the processor is running in. What's
going on here? I'd expect the driver to be just treating everything as
a byte stream and letting the protocol driver handle the endianness
issues, otherwise we might end up with double converions.
> + xfer->tx_dma = dma_map_single(dev, (void *)xfer->tx_buf,
> + xfer->len, DMA_TO_DEVICE);
Why is there a cast to void here? That's usually a sign that there's a
type safety issue, the whole point with void is that it's compatible
with any other pointer.
> +static irqreturn_t mtk_spi_slave_interrupt(int irq, void *dev_id)
> +{
> + struct spi_controller *ctlr = dev_id;
> + struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> + struct spi_transfer *trans = mdata->cur_transfer;
> + u32 int_status, reg_val, cnt, remainder;
> +
> + int_status = readl(mdata->base + SPIS_IRQ_ST_REG);
> + writel(int_status, mdata->base + SPIS_IRQ_CLR_REG);
> +
> + if (!trans)
> + return IRQ_HANDLED;
Are you sure that this is the right thing to do if we get a spurious
interrupt - the normal thing would be to return IRQ_NONE, possibly
logging a warning as well?
> + if (int_status & CMD_INVALID_ST)
> + dev_err(&ctlr->dev, "cmd invalid\n");
If there's an interrupt that doesn't have any of the interrupt status
flags set I'd expect to see a warning and probably IRQ_NONE being
returned. That way if the interrupt line is shared we work correctly
and if something goes wrong and the interrupt gets stuck on then the
core interrupt code's error handling can kick in.
Hi Mark,
Thanks for your comments.
On Tue, 2018-09-18 at 09:30 -0700, Mark Brown wrote:
> On Mon, Sep 17, 2018 at 10:19:21AM +0800, Leilk Liu wrote:
>
> This looks overall pretty good, a few smallish comments below:
>
> Please use subject lines matching the style for the subsystem. This
> makes it easier for people to identify relevant patches.
>
OK, I'll fix it, thanks.
> > if SPI_SLAVE
> > +config SPI_SLAVE_MT27XX
> > + tristate "MediaTek SPI slave device"
> > + depends on ARCH_MEDIATEK || COMPILE_TEST
> > + help
> > + This selects the MediaTek(R) SPI slave device driver.
> > + If you want to use MediaTek(R) SPI slave interface,
> > + say Y or M here.If you are not sure, say N.
> > + SPI slave drivers for Mediatek MT27XX series ARM SoCs.
> >
> > config SPI_SLAVE_TIME
>
> This is a driver not a slave implementation, it should be with the other
> drivers in the Kconfig. The slave implementations are for the
> functionality that uses the drivers, not the drivers themselves.
>
OK, I'll fix it, thanks.
> > + /* set the tx/rx endian */
> > +#ifdef __LITTLE_ENDIAN
> > + reg_val &= ~SPIS_TX_ENDIAN;
> > + reg_val &= ~SPIS_RX_ENDIAN;
> > +#else
> > + reg_val |= SPIS_TX_ENDIAN;
> > + reg_val |= SPIS_RX_ENDIAN;
> > +#endif
> > + writel(reg_val, mdata->base + SPIS_CFG_REG);
>
> This seems weird - it looks like it's changing the endianess of the
> protocol based on the endianness the processor is running in. What's
> going on here? I'd expect the driver to be just treating everything as
> a byte stream and letting the protocol driver handle the endianness
> issues, otherwise we might end up with double converions.
>
OK, I'll set the endian of SPI the same with the processor. Thanks.
> > + xfer->tx_dma = dma_map_single(dev, (void *)xfer->tx_buf,
> > + xfer->len, DMA_TO_DEVICE);
>
> Why is there a cast to void here? That's usually a sign that there's a
> type safety issue, the whole point with void is that it's compatible
> with any other pointer.
>
tx_buf is a const void*, here it need a void * for the dma mapping.
And I'll remove void * from dma_map_single((void *)rx_buf).
Thanks.
> > +static irqreturn_t mtk_spi_slave_interrupt(int irq, void *dev_id)
> > +{
> > + struct spi_controller *ctlr = dev_id;
> > + struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> > + struct spi_transfer *trans = mdata->cur_transfer;
> > + u32 int_status, reg_val, cnt, remainder;
> > +
> > + int_status = readl(mdata->base + SPIS_IRQ_ST_REG);
> > + writel(int_status, mdata->base + SPIS_IRQ_CLR_REG);
> > +
> > + if (!trans)
> > + return IRQ_HANDLED;
>
> Are you sure that this is the right thing to do if we get a spurious
> interrupt - the normal thing would be to return IRQ_NONE, possibly
> logging a warning as well?
>
OK, it should reture IRQ_NONE here, thanks.
> > + if (int_status & CMD_INVALID_ST)
> > + dev_err(&ctlr->dev, "cmd invalid\n");
>
> If there's an interrupt that doesn't have any of the interrupt status
> flags set I'd expect to see a warning and probably IRQ_NONE being
> returned. That way if the interrupt line is shared we work correctly
> and if something goes wrong and the interrupt gets stuck on then the
> core interrupt code's error handling can kick in.
OK, I'll fix it, thanks.
On Wed, 2018-09-26 at 17:33 -0500, Rob Herring wrote:
> On Mon, Sep 17, 2018 at 10:19:20AM +0800, Leilk Liu wrote:
> > This patch adds a DT binding documentation for the MT2712 soc.
> >
> > Signed-off-by: Leilk Liu <[email protected]>
> > ---
> > .../devicetree/bindings/spi/spi-slave-mt27xx.txt | 32 ++++++++++++++++++++
> > 1 file changed, 32 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
> >
> > diff --git a/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
> > new file mode 100644
> > index 0000000..09cb2c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
> > @@ -0,0 +1,32 @@
> > +Binding for MTK SPI Slave controller
> > +
> > +Required properties:
> > +- compatible: should be one of the following.
> > + - mediatek,mt2712-spi-slave: for mt2712 platforms
> > +- reg: Address and length of the register set for the device.
> > +- interrupts: Should contain spi interrupt.
> > +- clocks: phandles to input clocks.
> > + It's clock gate, and should be <&infracfg CLK_INFRA_AO_SPI1>.
> > +- clock-names: should be "spi" for the clock gate.
> > +
> > +Optional properties:
> > +- assigned-clocks: it's mux clock, should be <&topckgen CLK_TOP_SPISLV_SEL>.
> > +- assigned-clock-parents: parent of mux clock.
> > + It's PLL, and should be on of the following.
>
> s/on/one/
>
> With that fixed,
>
> Reviewed-by: Rob Herring <[email protected]>
Yes, it's a mistake, I'll fix it, thanks