[PATCH 0/3] Add Mediatek SPI slave driver

[PATCH 0/3] Add Mediatek SPI slave driver

[Leilk Liu]

>From 5ba1b2c0cd279255b49ecc8fa18c04ceb292a214 Mon Sep 17 00:00:00 2001
From: Leilk Liu <leilk.liu@mediatek.com>
Date: Tue, 28 Aug 2018 14:18:56 +0800
Subject: [PATCH 0/3] Add Mediatek SPI slave driver

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   |   39 ++
 arch/arm64/boot/dts/mediatek/mt2712e.dtsi          |   12 +
 drivers/spi/Kconfig                                |    8 +
 drivers/spi/Makefile                               |    1 +
 drivers/spi/spi-slave-mt27xx.c                     |  613 ++++++++++++++++++++
 5 files changed, 673 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
 create mode 100644 drivers/spi/spi-slave-mt27xx.c

-- 
1.7.9.5

[PATCH 1/3] spis: mediatek: add bindings for Mediatek MT2712 soc platform

[Leilk Liu]

This patch adds a DT binding documentation for the MT2712 soc.

Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
---
 .../devicetree/bindings/spi/spi-slave-mt27xx.txt   |   39 ++++++++++++++++++++
 1 file changed, 39 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..dcb8934
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
@@ -0,0 +1,39 @@
+Binding for MTK SPI Slave controller
+
+Required properties:
+- compatible: should be one of the following.
+    - mediatek,mt2712-spi: for mt2712 platforms
+
+- reg: Address and length of the register set for the device
+
+- interrupts: Should contain spi interrupt
+
+- clocks: phandles to input clocks.
+  The first should be one of the following. It's PLL.
+   -  <&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.
+  The second should be <&topckgen CLK_TOP_SPISLV_SEL>. It's clock mux.
+  The third is <&infracfg CLK_INFRA_AO_SPI1>. It's clock gate.
+
+- clock-names: shall be "parent-clk" for the parent clock, "sel-clk" for the
+  muxes clock, and "spi-clk" for the clock gate.
+
+- spi-slave: Empty property indicating the SPI controller is used in slave mode.
+
+Example:
+
+- SoC Specific Portion:
+spis: spi at 10013000 {
+	compatible = "mediatek,mt2712-spi-slave";
+	reg = <0 0x10013000 0 0x100>;
+	interrupts = <GIC_SPI 283 IRQ_TYPE_LEVEL_LOW>;
+	clocks = <&topckgen CLK_TOP_UNIVPLL1_D2>,
+		<&topckgen CLK_TOP_SPISLV_SEL>,
+		<&infracfg CLK_INFRA_AO_SPI1>;
+	clock-names = "parent-clk", "sel-clk", "spi-clk";
+	spi-slave;
+	status = "disabled";
+};
-- 
1.7.9.5

[PATCH 1/3] spis: mediatek: add bindings for Mediatek MT2712 soc platform

[Matthias Brugger]

On 28/08/18 08:28, Leilk Liu wrote:
> This patch adds a DT binding documentation for the MT2712 soc.
> 
> Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
> ---
>  .../devicetree/bindings/spi/spi-slave-mt27xx.txt   |   39 ++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt

Do I understand correctly that mt6xxx, mt7xxx and mt8xxx SoCs have a totally
different architecture then mt27xx so they will need a totally different
binding. If not, then please rename the binding description file and the driver.

Regards,
Matthias

> 
> 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..dcb8934
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
> @@ -0,0 +1,39 @@
> +Binding for MTK SPI Slave controller
> +
> +Required properties:
> +- compatible: should be one of the following.
> +    - mediatek,mt2712-spi: for mt2712 platforms
> +
> +- reg: Address and length of the register set for the device
> +
> +- interrupts: Should contain spi interrupt
> +
> +- clocks: phandles to input clocks.
> +  The first should be one of the following. It's PLL.
> +   -  <&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.
> +  The second should be <&topckgen CLK_TOP_SPISLV_SEL>. It's clock mux.
> +  The third is <&infracfg CLK_INFRA_AO_SPI1>. It's clock gate.
> +
> +- clock-names: shall be "parent-clk" for the parent clock, "sel-clk" for the
> +  muxes clock, and "spi-clk" for the clock gate.
> +
> +- spi-slave: Empty property indicating the SPI controller is used in slave mode.
> +
> +Example:
> +
> +- SoC Specific Portion:
> +spis: spi at 10013000 {
> +	compatible = "mediatek,mt2712-spi-slave";
> +	reg = <0 0x10013000 0 0x100>;
> +	interrupts = <GIC_SPI 283 IRQ_TYPE_LEVEL_LOW>;
> +	clocks = <&topckgen CLK_TOP_UNIVPLL1_D2>,
> +		<&topckgen CLK_TOP_SPISLV_SEL>,
> +		<&infracfg CLK_INFRA_AO_SPI1>;
> +	clock-names = "parent-clk", "sel-clk", "spi-clk";
> +	spi-slave;
> +	status = "disabled";
> +};
> 

[SPAM]Re: [PATCH 1/3] spis: mediatek: add bindings for Mediatek MT2712 soc platform

[lei liu]

On Tue, 2018-08-28 at 17:44 +0200, Matthias Brugger wrote:
> 
> On 28/08/18 08:28, Leilk Liu wrote:
> > This patch adds a DT binding documentation for the MT2712 soc.
> > 
> > Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
> > ---
> >  .../devicetree/bindings/spi/spi-slave-mt27xx.txt   |   39 ++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
> 
> Do I understand correctly that mt6xxx, mt7xxx and mt8xxx SoCs have a totally
> different architecture then mt27xx so they will need a totally different
> binding. If not, then please rename the binding description file and the driver.
> 
> Regards,
> Matthias
> 

This series are for spi slave, and mt27xx spi slave design has a
entirely different architecture with mt6xxx, mt7xxx and mt8xxx SoCs.

> > 
> > 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..dcb8934
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
> > @@ -0,0 +1,39 @@
> > +Binding for MTK SPI Slave controller
> > +
> > +Required properties:
> > +- compatible: should be one of the following.
> > +    - mediatek,mt2712-spi: for mt2712 platforms
> > +
> > +- reg: Address and length of the register set for the device
> > +
> > +- interrupts: Should contain spi interrupt
> > +
> > +- clocks: phandles to input clocks.
> > +  The first should be one of the following. It's PLL.
> > +   -  <&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.
> > +  The second should be <&topckgen CLK_TOP_SPISLV_SEL>. It's clock mux.
> > +  The third is <&infracfg CLK_INFRA_AO_SPI1>. It's clock gate.
> > +
> > +- clock-names: shall be "parent-clk" for the parent clock, "sel-clk" for the
> > +  muxes clock, and "spi-clk" for the clock gate.
> > +
> > +- spi-slave: Empty property indicating the SPI controller is used in slave mode.
> > +
> > +Example:
> > +
> > +- SoC Specific Portion:
> > +spis: spi at 10013000 {
> > +	compatible = "mediatek,mt2712-spi-slave";
> > +	reg = <0 0x10013000 0 0x100>;
> > +	interrupts = <GIC_SPI 283 IRQ_TYPE_LEVEL_LOW>;
> > +	clocks = <&topckgen CLK_TOP_UNIVPLL1_D2>,
> > +		<&topckgen CLK_TOP_SPISLV_SEL>,
> > +		<&infracfg CLK_INFRA_AO_SPI1>;
> > +	clock-names = "parent-clk", "sel-clk", "spi-clk";
> > +	spi-slave;
> > +	status = "disabled";
> > +};
> > 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 1/3] spis: mediatek: add bindings for Mediatek MT2712 soc platform

[Rob Herring]

On Tue, Aug 28, 2018 at 02:28:03PM +0800, Leilk Liu wrote:
> This patch adds a DT binding documentation for the MT2712 soc.
> 
> Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
> ---
>  .../devicetree/bindings/spi/spi-slave-mt27xx.txt   |   39 ++++++++++++++++++++
>  1 file changed, 39 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..dcb8934
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
> @@ -0,0 +1,39 @@
> +Binding for MTK SPI Slave controller

Only does slave mode? If not, then the file name and doc should be just 
for the SPI controller (master and slave). The "spi-slave" property 
selects the mode. If it is only slave mode, then you don't need the 
spi-slave property.


> +
> +Required properties:
> +- compatible: should be one of the following.
> +    - mediatek,mt2712-spi: for mt2712 platforms
> +
> +- reg: Address and length of the register set for the device
> +
> +- interrupts: Should contain spi interrupt
> +
> +- clocks: phandles to input clocks.
> +  The first should be one of the following. It's PLL.
> +   -  <&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.
> +  The second should be <&topckgen CLK_TOP_SPISLV_SEL>. It's clock mux.
> +  The third is <&infracfg CLK_INFRA_AO_SPI1>. It's clock gate.
> +
> +- clock-names: shall be "parent-clk" for the parent clock, "sel-clk" for the
> +  muxes clock, and "spi-clk" for the clock gate.

"-clk" is redundant.

Is the parent clock actually connected to the block? The assigned-clocks 
properties are used for constraints on parent clocks.

> +
> +- spi-slave: Empty property indicating the SPI controller is used in slave mode.
> +
> +Example:
> +
> +- SoC Specific Portion:
> +spis: spi at 10013000 {
> +	compatible = "mediatek,mt2712-spi-slave";
> +	reg = <0 0x10013000 0 0x100>;
> +	interrupts = <GIC_SPI 283 IRQ_TYPE_LEVEL_LOW>;
> +	clocks = <&topckgen CLK_TOP_UNIVPLL1_D2>,
> +		<&topckgen CLK_TOP_SPISLV_SEL>,
> +		<&infracfg CLK_INFRA_AO_SPI1>;
> +	clock-names = "parent-clk", "sel-clk", "spi-clk";
> +	spi-slave;
> +	status = "disabled";

Don't should status in examples.

> +};
> -- 
> 1.7.9.5
> 

[PATCH 1/3] spis: mediatek: add bindings for Mediatek MT2712 soc platform

[lei liu]

On Tue, 2018-09-04 at 08:18 -0500, Rob Herring wrote:
> On Tue, Aug 28, 2018 at 02:28:03PM +0800, Leilk Liu wrote:
> > This patch adds a DT binding documentation for the MT2712 soc.
> > 
> > Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
> > ---
> >  .../devicetree/bindings/spi/spi-slave-mt27xx.txt   |   39 ++++++++++++++++++++
> >  1 file changed, 39 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..dcb8934
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt
> > @@ -0,0 +1,39 @@
> > +Binding for MTK SPI Slave controller
> 
> Only does slave mode? If not, then the file name and doc should be just 
> for the SPI controller (master and slave). The "spi-slave" property 
> selects the mode. If it is only slave mode, then you don't need the 
> spi-slave property.
> 

yes, this is only for slave mode, and I'll remove spi-slave property,
thanks.

> 
> > +
> > +Required properties:
> > +- compatible: should be one of the following.
> > +    - mediatek,mt2712-spi: for mt2712 platforms
> > +
> > +- reg: Address and length of the register set for the device
> > +
> > +- interrupts: Should contain spi interrupt
> > +
> > +- clocks: phandles to input clocks.
> > +  The first should be one of the following. It's PLL.
> > +   -  <&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.
> > +  The second should be <&topckgen CLK_TOP_SPISLV_SEL>. It's clock mux.
> > +  The third is <&infracfg CLK_INFRA_AO_SPI1>. It's clock gate.
> > +
> > +- clock-names: shall be "parent-clk" for the parent clock, "sel-clk" for the
> > +  muxes clock, and "spi-clk" for the clock gate.
> 
> "-clk" is redundant.
> 

ok, I'll fix it, thanks.

> Is the parent clock actually connected to the block? The assigned-clocks 
> properties are used for constraints on parent clocks.
> 

Thanks for your comment.
The parent and sel clk can select the source clk of spi module.
And I already use assigned-clocks and assigned-clocks-parents to set
them in patch v2.

> > +
> > +- spi-slave: Empty property indicating the SPI controller is used in slave mode.
> > +
> > +Example:
> > +
> > +- SoC Specific Portion:
> > +spis: spi at 10013000 {
> > +	compatible = "mediatek,mt2712-spi-slave";
> > +	reg = <0 0x10013000 0 0x100>;
> > +	interrupts = <GIC_SPI 283 IRQ_TYPE_LEVEL_LOW>;
> > +	clocks = <&topckgen CLK_TOP_UNIVPLL1_D2>,
> > +		<&topckgen CLK_TOP_SPISLV_SEL>,
> > +		<&infracfg CLK_INFRA_AO_SPI1>;
> > +	clock-names = "parent-clk", "sel-clk", "spi-clk";
> > +	spi-slave;
> > +	status = "disabled";
> 
> Don't should status in examples.
> 

ok, I'll fix it, thanks

> > +};
> > -- 
> > 1.7.9.5
> > 
> 

[PATCH 2/3] spis: mediatek: add spi slave for Mediatek MT2712

[Leilk Liu]

This patchs add basic spi slave for MT2712.

Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
---
 drivers/spi/Kconfig            |    8 +
 drivers/spi/Makefile           |    1 +
 drivers/spi/spi-slave-mt27xx.c |  613 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 622 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..2aa12f3
--- /dev/null
+++ b/drivers/spi/spi-slave-mt27xx.c
@@ -0,0 +1,613 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright (c) 2018 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/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_RX_CMD_REG		0x24
+#define SPIS_FIFO_ST_REG	0x28
+#define SPIS_MON_SEL_REG	0x2c
+#define SPIS_DMA_CFG_REG	0x30
+#define SPIS_FIFO_THR_REG	0x34
+#define SPIS_DEBUG_ST_REG	0x38
+#define SPIS_BYTE_CNT_REG	0x3c
+#define SPIS_SOFT_RST_REG	0x40
+
+/* SPIS_IRQ_EN_REG */
+#define DMA_DONE_EN		BIT(7)
+#define TX_FIFO_EMPTY_EN	BIT(6)
+#define TX_FIFO_FULL_EN		BIT(5)
+#define RX_FIFO_EMPTY_EN	BIT(4)
+#define RX_FIFO_FULL_EN		BIT(3)
+#define DATA_DONE_EN		BIT(2)
+#define RSTA_DONE_EN		BIT(1)
+#define CMD_INVALID_EN		BIT(0)
+
+/* SPIS_IRQ_CLR_REG */
+#define DMA_DONE_CLR		BIT(7)
+#define TX_FIFO_EMPTY_CLR	BIT(6)
+#define TX_FIFO_FULL_CLR	BIT(5)
+#define RX_FIFO_EMPTY_CLR	BIT(4)
+#define RX_FIFO_FULL_CLR	BIT(3)
+#define DATA_DONE_CLR		BIT(2)
+#define RSTA_DONE_CLR		BIT(1)
+#define CMD_INVALID_CLR		BIT(0)
+
+/* SPIS_IRQ_ST_REG */
+#define DMA_DONE_ST		BIT(7)
+#define TX_FIFO_EMPTY_ST	BIT(6)
+#define TX_FIFO_FULL_ST		BIT(5)
+#define RX_FIFO_EMPTY_ST	BIT(4)
+#define RX_FIFO_FULL_ST		BIT(3)
+#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 *parent_clk, *sel_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) {
+		pr_err("interrupted\n");
+		return -EINTR;
+	}
+
+	return 0;
+}
+
+static int mtk_spi_slave_prepare_message(struct spi_controller *ctlr,
+					 struct spi_message *msg)
+{
+	u16 cpha, cpol;
+	u32 reg_val;
+	struct spi_device *spi = msg->spi;
+	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+
+	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)
+{
+	int reg_val, cnt, remainder, ret;
+	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+
+	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(&reg_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)
+{
+	int reg_val, ret;
+	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+	struct device *dev = mdata->dev;
+
+	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 &= ~TX_DMA_LEN;
+	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)
+{
+	u32 reg_val;
+	struct mtk_spi_slave *mdata = spi_controller_get_devdata(spi->master);
+
+	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)
+{
+	u32 cmd, int_status, reg_val, cnt, remainder;
+	struct spi_controller *ctlr = dev_id;
+	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+	struct spi_transfer *trans = mdata->cur_transfer;
+
+	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);
+
+		mtk_spi_slave_disable_dma(mdata);
+		mtk_spi_slave_disable_xfer(mdata);
+
+		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);
+	}
+
+	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),
+			       &reg_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;
+	const struct of_device_id *of_id;
+	struct resource *res;
+	int i, 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->parent_clk = devm_clk_get(&pdev->dev, "parent-clk");
+	if (IS_ERR(mdata->parent_clk)) {
+		ret = PTR_ERR(mdata->parent_clk);
+		dev_err(&pdev->dev, "failed to get parent-clk: %d\n", ret);
+		goto err_put_ctlr;
+	}
+
+	mdata->sel_clk = devm_clk_get(&pdev->dev, "sel-clk");
+	if (IS_ERR(mdata->sel_clk)) {
+		ret = PTR_ERR(mdata->sel_clk);
+		dev_err(&pdev->dev, "failed to get sel-clk: %d\n", ret);
+		goto err_put_ctlr;
+	}
+
+	mdata->spi_clk = devm_clk_get(&pdev->dev, "spi-clk");
+	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;
+	}
+
+	ret = clk_set_parent(mdata->sel_clk, mdata->parent_clk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to clk_set_parent (%d)\n", ret);
+		goto err_disable_clk;
+	}
+
+	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);
+		goto err_disable_runtime_pm;
+	}
+
+	clk_disable_unprepare(mdata->spi_clk);
+
+	return 0;
+
+err_disable_runtime_pm:
+	pm_runtime_disable(&pdev->dev);
+err_disable_clk:
+	clk_disable_unprepare(mdata->spi_clk);
+err_put_ctlr:
+	spi_controller_put(ctlr);
+
+	return ret;
+}
+
+static int mtk_spi_slave_remove(struct platform_device *pdev)
+{
+	struct spi_controller *ctlr = platform_get_drvdata(pdev);
+
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mtk_spi_slave_suspend(struct device *dev)
+{
+	int ret;
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+
+	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)
+{
+	int ret;
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
+
+	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 <leilk.liu@mediatek.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:mtk-spi-slave");
-- 
1.7.9.5

[SPAM][PATCH 2/3] spis: mediatek: add spi slave for Mediatek MT2712

[Sean Wang]

Hi,

a few comments are added inline

On Tue, 2018-08-28 at 14:28 +0800, Leilk Liu wrote:
> This patchs add basic spi slave for MT2712.
> 
> Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
> ---
>  drivers/spi/Kconfig            |    8 +
>  drivers/spi/Makefile           |    1 +
>  drivers/spi/spi-slave-mt27xx.c |  613 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 622 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..2aa12f3
> --- /dev/null
> +++ b/drivers/spi/spi-slave-mt27xx.c
> @@ -0,0 +1,613 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Copyright (c) 2018 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.
> +

you can drop long disclaimer because you already had SPDX style license

> +#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_RX_CMD_REG		0x24
> +#define SPIS_FIFO_ST_REG	0x28
> +#define SPIS_MON_SEL_REG	0x2c
> +#define SPIS_DMA_CFG_REG	0x30
> +#define SPIS_FIFO_THR_REG	0x34
> +#define SPIS_DEBUG_ST_REG	0x38
> +#define SPIS_BYTE_CNT_REG	0x3c
> +#define SPIS_SOFT_RST_REG	0x40
> +
> +/* SPIS_IRQ_EN_REG */
> +#define DMA_DONE_EN		BIT(7)
> +#define TX_FIFO_EMPTY_EN	BIT(6)
> +#define TX_FIFO_FULL_EN		BIT(5)
> +#define RX_FIFO_EMPTY_EN	BIT(4)
> +#define RX_FIFO_FULL_EN		BIT(3)
> +#define DATA_DONE_EN		BIT(2)
> +#define RSTA_DONE_EN		BIT(1)
> +#define CMD_INVALID_EN		BIT(0)
> +
> +/* SPIS_IRQ_CLR_REG */
> +#define DMA_DONE_CLR		BIT(7)
> +#define TX_FIFO_EMPTY_CLR	BIT(6)
> +#define TX_FIFO_FULL_CLR	BIT(5)
> +#define RX_FIFO_EMPTY_CLR	BIT(4)
> +#define RX_FIFO_FULL_CLR	BIT(3)
> +#define DATA_DONE_CLR		BIT(2)
> +#define RSTA_DONE_CLR		BIT(1)
> +#define CMD_INVALID_CLR		BIT(0)
> +
> +/* SPIS_IRQ_ST_REG */
> +#define DMA_DONE_ST		BIT(7)
> +#define TX_FIFO_EMPTY_ST	BIT(6)
> +#define TX_FIFO_FULL_ST		BIT(5)
> +#define RX_FIFO_EMPTY_ST	BIT(4)
> +#define RX_FIFO_FULL_ST		BIT(3)
> +#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 *parent_clk, *sel_clk, *spi_clk;

consider that parent clk should be configured in dts
by these property assigned-clocks, assigned-clock-parents. in that way, the driver
don't bother with what the parent and sel clock are and becomes more configurable.

> +	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) {
> +		pr_err("interrupted\n");

dev_err?


> +		return -EINTR;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_spi_slave_prepare_message(struct spi_controller *ctlr,
> +					 struct spi_message *msg)
> +{
> +	u16 cpha, cpol;

bool?

> +	u32 reg_val;
> +	struct spi_device *spi = msg->spi;
> +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> +

how about using reverse xmas tree declarations for be readable? that means ordering declarations longest to shortest

> +	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)
> +{
> +	int reg_val, cnt, remainder, ret;
> +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> +

reverse xmas tree declarations ?

> +	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(&reg_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)
> +{
> +	int reg_val, ret;
> +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> +	struct device *dev = mdata->dev;
> +
> +	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 &= ~TX_DMA_LEN;

it seems it no needs the extra clear bit

> +	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)
> +{
> +	u32 reg_val;
> +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(spi->master);

reverse xmas tree declarations ?

> +	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)
> +{
> +	u32 cmd, int_status, reg_val, cnt, remainder;
> +	struct spi_controller *ctlr = dev_id;
> +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> +	struct spi_transfer *trans = mdata->cur_transfer;
> +
> +	int_status = readl(mdata->base + SPIS_IRQ_ST_REG);
> +	writel(int_status, mdata->base + SPIS_IRQ_CLR_REG);

I supposed you already have disabled these bit the isr can't handle

> +	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);
> +
> +		mtk_spi_slave_disable_dma(mdata);
> +		mtk_spi_slave_disable_xfer(mdata);
> +
> +		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);
> +	}
> +
> +	if ((!(int_status & DMA_DONE_ST)) &&
> +	    ((int_status & DATA_DONE_ST) ||
> +	    (int_status & RSTA_DONE_ST))) {

is the condition both for dma and fifo mode ?

and it seems there is a missing disable_xfer for fifo mode

> +		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),

parentheses around cnt * 4 is not needed, and no parentheses should be clearer enough

> +			       &reg_val, remainder);
> +		}
> +
> +		mtk_spi_slave_disable_xfer(mdata);

a little confused you put disable xfer last at fifo mode, but put first at dma mode

> +	}
> +
> +	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;
> +	const struct of_device_id *of_id;
> +	struct resource *res;
> +	int i, irq, ret;
> +

reverse xmas tree declarations ?

> +	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->parent_clk = devm_clk_get(&pdev->dev, "parent-clk");
> +	if (IS_ERR(mdata->parent_clk)) {
> +		ret = PTR_ERR(mdata->parent_clk);
> +		dev_err(&pdev->dev, "failed to get parent-clk: %d\n", ret);
> +		goto err_put_ctlr;
> +	}
> +
> +	mdata->sel_clk = devm_clk_get(&pdev->dev, "sel-clk");
> +	if (IS_ERR(mdata->sel_clk)) {
> +		ret = PTR_ERR(mdata->sel_clk);
> +		dev_err(&pdev->dev, "failed to get sel-clk: %d\n", ret);
> +		goto err_put_ctlr;
> +	}
> +
> +	mdata->spi_clk = devm_clk_get(&pdev->dev, "spi-clk");
> +	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;
> +	}
> +
> +	ret = clk_set_parent(mdata->sel_clk, mdata->parent_clk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to clk_set_parent (%d)\n", ret);
> +		goto err_disable_clk;
> +	}
> +

consider more about parent and sel clock assignment in the dts, that way would
help the driver to be portable on each soc

> +	pm_runtime_enable(&pdev->dev);

disable interrupt bits the isr doesn't support to

> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to register slave controller(%d)\n", ret);
> +		goto err_disable_runtime_pm;
> +	}
> +
> +	clk_disable_unprepare(mdata->spi_clk);
> +
> +	return 0;
> +
> +err_disable_runtime_pm:
> +	pm_runtime_disable(&pdev->dev);
> +err_disable_clk:
> +	clk_disable_unprepare(mdata->spi_clk);
> +err_put_ctlr:
> +	spi_controller_put(ctlr);
> +
> +	return ret;
> +}
> +
> +static int mtk_spi_slave_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mtk_spi_slave_suspend(struct device *dev)
> +{
> +	int ret;
> +	struct spi_controller *ctlr = dev_get_drvdata(dev);
> +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> +
> +	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)
> +{
> +	int ret;
> +	struct spi_controller *ctlr = dev_get_drvdata(dev);
> +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> +
> +	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 <leilk.liu@mediatek.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:mtk-spi-slave");

[SPAM][PATCH 2/3] spis: mediatek: add spi slave for Mediatek MT2712

[lei liu]

On Fri, 2018-08-31 at 10:41 +0800, Sean Wang wrote:
> Hi,
> 
> a few comments are added inline
> 
> On Tue, 2018-08-28 at 14:28 +0800, Leilk Liu wrote:
> > This patchs add basic spi slave for MT2712.
> > 
> > Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
> > ---
> >  drivers/spi/Kconfig            |    8 +
> >  drivers/spi/Makefile           |    1 +
> >  drivers/spi/spi-slave-mt27xx.c |  613 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 622 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..2aa12f3
> > --- /dev/null
> > +++ b/drivers/spi/spi-slave-mt27xx.c
> > @@ -0,0 +1,613 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +//
> > +// Copyright (c) 2018 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.
> > +
> 
> you can drop long disclaimer because you already had SPDX style license
> 
OK, I'll fix it, thanks

> > +#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_RX_CMD_REG		0x24
> > +#define SPIS_FIFO_ST_REG	0x28
> > +#define SPIS_MON_SEL_REG	0x2c
> > +#define SPIS_DMA_CFG_REG	0x30
> > +#define SPIS_FIFO_THR_REG	0x34
> > +#define SPIS_DEBUG_ST_REG	0x38
> > +#define SPIS_BYTE_CNT_REG	0x3c
> > +#define SPIS_SOFT_RST_REG	0x40
> > +
> > +/* SPIS_IRQ_EN_REG */
> > +#define DMA_DONE_EN		BIT(7)
> > +#define TX_FIFO_EMPTY_EN	BIT(6)
> > +#define TX_FIFO_FULL_EN		BIT(5)
> > +#define RX_FIFO_EMPTY_EN	BIT(4)
> > +#define RX_FIFO_FULL_EN		BIT(3)
> > +#define DATA_DONE_EN		BIT(2)
> > +#define RSTA_DONE_EN		BIT(1)
> > +#define CMD_INVALID_EN		BIT(0)
> > +
> > +/* SPIS_IRQ_CLR_REG */
> > +#define DMA_DONE_CLR		BIT(7)
> > +#define TX_FIFO_EMPTY_CLR	BIT(6)
> > +#define TX_FIFO_FULL_CLR	BIT(5)
> > +#define RX_FIFO_EMPTY_CLR	BIT(4)
> > +#define RX_FIFO_FULL_CLR	BIT(3)
> > +#define DATA_DONE_CLR		BIT(2)
> > +#define RSTA_DONE_CLR		BIT(1)
> > +#define CMD_INVALID_CLR		BIT(0)
> > +
> > +/* SPIS_IRQ_ST_REG */
> > +#define DMA_DONE_ST		BIT(7)
> > +#define TX_FIFO_EMPTY_ST	BIT(6)
> > +#define TX_FIFO_FULL_ST		BIT(5)
> > +#define RX_FIFO_EMPTY_ST	BIT(4)
> > +#define RX_FIFO_FULL_ST		BIT(3)
> > +#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 *parent_clk, *sel_clk, *spi_clk;
> 
> consider that parent clk should be configured in dts
> by these property assigned-clocks, assigned-clock-parents. in that way, the driver
> don't bother with what the parent and sel clock are and becomes more configurable.
> 
OK, I'll fix it, thanks

> > +	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) {
> > +		pr_err("interrupted\n");
> 
> dev_err?
> 
OK, I'll fix it, thanks

> 
> > +		return -EINTR;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_spi_slave_prepare_message(struct spi_controller *ctlr,
> > +					 struct spi_message *msg)
> > +{
> > +	u16 cpha, cpol;
> 
> bool?
> 
OK, I'll fix it, thanks

> > +	u32 reg_val;
> > +	struct spi_device *spi = msg->spi;
> > +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> > +
> 
> how about using reverse xmas tree declarations for be readable? that means ordering declarations longest to shortest
> 
OK, I'll fix it, thanks

> > +	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)
> > +{
> > +	int reg_val, cnt, remainder, ret;
> > +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> > +
> 
> reverse xmas tree declarations ?
> 
OK, I'll fix it, thanks

> > +	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(&reg_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)
> > +{
> > +	int reg_val, ret;
> > +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> > +	struct device *dev = mdata->dev;
> > +
> > +	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 &= ~TX_DMA_LEN;
> 
> it seems it no needs the extra clear bit
> 
OK, I'll fix it, thanks

> > +	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)
> > +{
> > +	u32 reg_val;
> > +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(spi->master);
> 
> reverse xmas tree declarations ?
> 
OK, I'll fix it, thanks

> > +	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)
> > +{
> > +	u32 cmd, int_status, reg_val, cnt, remainder;
> > +	struct spi_controller *ctlr = dev_id;
> > +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> > +	struct spi_transfer *trans = mdata->cur_transfer;
> > +
> > +	int_status = readl(mdata->base + SPIS_IRQ_ST_REG);
> > +	writel(int_status, mdata->base + SPIS_IRQ_CLR_REG);
> 
> I supposed you already have disabled these bit the isr can't handle
> 
yes. It's disabled in mtk_spi_slave_setup() which would be called when
probe.

> > +	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);
> > +
> > +		mtk_spi_slave_disable_dma(mdata);
> > +		mtk_spi_slave_disable_xfer(mdata);
> > +
> > +		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);
> > +	}
> > +
> > +	if ((!(int_status & DMA_DONE_ST)) &&
> > +	    ((int_status & DATA_DONE_ST) ||
> > +	    (int_status & RSTA_DONE_ST))) {
> 
> is the condition both for dma and fifo mode ?
> 
> and it seems there is a missing disable_xfer for fifo mode
> 
this condition is for transfer data or read status by dma mode.
if it's dma mode of transfer data, the HW assert the status:DMA_DONE_ST
& DATA_DONE_ST.
if it's dma mode of read status, the HW assert the status:DMA_DONE_ST &
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),
> 
> parentheses around cnt * 4 is not needed, and no parentheses should be clearer enough
> 
OK, I'll fix it, thanks

> > +			       &reg_val, remainder);
> > +		}
> > +
> > +		mtk_spi_slave_disable_xfer(mdata);
> 
> a little confused you put disable xfer last at fifo mode, but put first at dma mode
> 
OK, I'll also put disable xfer last at dma mode. thanks.

> > +	}
> > +
> > +	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;
> > +	const struct of_device_id *of_id;
> > +	struct resource *res;
> > +	int i, irq, ret;
> > +
> 
> reverse xmas tree declarations ?
> 
OK, I'll fix it, thanks

> > +	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->parent_clk = devm_clk_get(&pdev->dev, "parent-clk");
> > +	if (IS_ERR(mdata->parent_clk)) {
> > +		ret = PTR_ERR(mdata->parent_clk);
> > +		dev_err(&pdev->dev, "failed to get parent-clk: %d\n", ret);
> > +		goto err_put_ctlr;
> > +	}
> > +
> > +	mdata->sel_clk = devm_clk_get(&pdev->dev, "sel-clk");
> > +	if (IS_ERR(mdata->sel_clk)) {
> > +		ret = PTR_ERR(mdata->sel_clk);
> > +		dev_err(&pdev->dev, "failed to get sel-clk: %d\n", ret);
> > +		goto err_put_ctlr;
> > +	}
> > +
> > +	mdata->spi_clk = devm_clk_get(&pdev->dev, "spi-clk");
> > +	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;
> > +	}
> > +
> > +	ret = clk_set_parent(mdata->sel_clk, mdata->parent_clk);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to clk_set_parent (%d)\n", ret);
> > +		goto err_disable_clk;
> > +	}
> > +
> 
> consider more about parent and sel clock assignment in the dts, that way would
> help the driver to be portable on each soc
> 
OK, I'll fix it, thanks

> > +	pm_runtime_enable(&pdev->dev);
> 
> disable interrupt bits the isr doesn't support to
> 
> > +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> > +	if (ret) {
> > +		dev_err(&pdev->dev,
> > +			"failed to register slave controller(%d)\n", ret);
> > +		goto err_disable_runtime_pm;
> > +	}
> > +
> > +	clk_disable_unprepare(mdata->spi_clk);
> > +
> > +	return 0;
> > +
> > +err_disable_runtime_pm:
> > +	pm_runtime_disable(&pdev->dev);
> > +err_disable_clk:
> > +	clk_disable_unprepare(mdata->spi_clk);
> > +err_put_ctlr:
> > +	spi_controller_put(ctlr);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mtk_spi_slave_remove(struct platform_device *pdev)
> > +{
> > +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> > +
> > +	pm_runtime_disable(&pdev->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mtk_spi_slave_suspend(struct device *dev)
> > +{
> > +	int ret;
> > +	struct spi_controller *ctlr = dev_get_drvdata(dev);
> > +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> > +
> > +	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)
> > +{
> > +	int ret;
> > +	struct spi_controller *ctlr = dev_get_drvdata(dev);
> > +	struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> > +
> > +	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 <leilk.liu@mediatek.com>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:mtk-spi-slave");
> 
> 
> 

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

[Leilk Liu]

This patch adds MT2712 spi slave into device tree.

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

diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
index 75cc0f7..064419b 100644
--- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
@@ -301,6 +301,18 @@
 		status = "disabled";
 	};
 
+	spis1: spi at 10013000 {
+		compatible = "mediatek,mt2712-spi-slave";
+		reg = <0 0x10013000 0 0x100>;
+		interrupts = <GIC_SPI 283 IRQ_TYPE_LEVEL_LOW>;
+		clocks = <&topckgen CLK_TOP_UNIVPLL1_D2>,
+			<&topckgen CLK_TOP_SPISLV_SEL>,
+			<&infracfg CLK_INFRA_AO_SPI1>;
+		clock-names = "parent-clk", "sel-clk", "spi-clk";
+		spi-slave;
+		status = "disabled";
+	};
+
 	apmixedsys: syscon at 10209000 {
 		compatible = "mediatek,mt2712-apmixedsys", "syscon";
 		reg = <0 0x10209000 0 0x1000>;
-- 
1.7.9.5