[PATCH V4 0/4] spi-geni-qcom: QUP SPI GENI driver and SPI device tree bindings

[PATCH V4 0/4] spi-geni-qcom: QUP SPI GENI driver and SPI device tree bindings

[Dilip Kota]

In this V4 series, submitted changes to remove SPI controller
max-frquency device tree binding as it is controller
specific property.

Move the GENI SE SPI controller device tree bindings
documentation to devicetree/bindings/spi/qcom,spi-geni-qcom.txt.

Address the reviewer comments to spi-geni-qcom.c in patch-V3.

Adds changes in spi-geni-qcom.c to plugin set_cs API to SPI framework.

Dilip Kota (3):
  dt-bindings: soc: qcom: Remove SPI controller maximum frequency entry
  dt-bindings: soc: qcom: GENI SE SPI controller device tree binding
  spi: spi-geni-qcom: Plugin API to assert and de-assert Chipselect

Girish Mahadevan (1):
  spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

 .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  |  29 +-
 .../devicetree/bindings/spi/qcom,spi-geni-qcom.txt |  39 ++
 drivers/spi/Kconfig                                |  12 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-geni-qcom.c                        | 680 +++++++++++++++++++++
 include/linux/spi/spi-geni-qcom.h                  |  14 +
 6 files changed, 748 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt
 create mode 100644 drivers/spi/spi-geni-qcom.c
 create mode 100644 include/linux/spi/spi-geni-qcom.h

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V4 1/4] dt-bindings: soc: qcom: Remove SPI controller maximum frequency binding

[Dilip Kota]

SPI controller driver should maintain the maximum frequency
of the controller instead of relying on device tree bindings.
Because maximum frequency is specific property of SPI
controller.

Signed-off-by: Dilip Kota <dkota@codeaurora.org>
---
 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
index 68b7d62..16467ed 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
@@ -60,7 +60,6 @@ Required properties:
 - interrupts:		Must contain SPI controller interrupts.
 - clock-names:		Must contain "se".
 - clocks:		Serial engine core clock needed by the device.
-- spi-max-frequency:	Specifies maximum SPI clock frequency, units - Hz.
 - #address-cells:	Must be <1> to define a chip select address on
 			the SPI bus.
 - #size-cells:		Must be <0>.
@@ -112,7 +111,6 @@ Example:
 			pinctrl-names = "default", "sleep";
 			pinctrl-0 = <&qup_1_spi_2_active>;
 			pinctrl-1 = <&qup_1_spi_2_sleep>;
-			spi-max-frequency = <19200000>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 		};
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V4 1/4] dt-bindings: soc: qcom: Remove SPI controller maximum frequency binding

[Doug Anderson]

Hi,

On Tue, Sep 18, 2018 at 11:08 AM Dilip Kota <dkota@codeaurora.org> wrote:
>
> SPI controller driver should maintain the maximum frequency
> of the controller instead of relying on device tree bindings.
> Because maximum frequency is specific property of SPI
> controller.
>
> Signed-off-by: Dilip Kota <dkota@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH V4 1/4] dt-bindings: soc: qcom: Remove SPI controller maximum frequency binding

[Stephen Boyd]

Quoting Dilip Kota (2018-09-18 11:07:23)
> SPI controller driver should maintain the maximum frequency
> of the controller instead of relying on device tree bindings.
> Because maximum frequency is specific property of SPI
> controller.
> 
> Signed-off-by: Dilip Kota <dkota@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH V4 1/4] dt-bindings: soc: qcom: Remove SPI controller maximum frequency binding

[Rob Herring]

On Tue, 18 Sep 2018 23:37:23 +0530, Dilip Kota wrote:
> SPI controller driver should maintain the maximum frequency
> of the controller instead of relying on device tree bindings.
> Because maximum frequency is specific property of SPI
> controller.
> 
> Signed-off-by: Dilip Kota <dkota@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt | 2 --
>  1 file changed, 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH V4 2/4] dt-bindings: soc: qcom: GENI SE SPI controller device tree binding

[Dilip Kota]

Move GENI SE SPI controller device-tree bindings
from devicetree/bindings/soc/qcom/qcom,geni-se.txt
to devicetree/bindings/spi/qcom,spi-geni-qcom.txt.

Signed-off-by: Dilip Kota <dkota@codeaurora.org>
---
 .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  | 27 ++-------------
 .../devicetree/bindings/spi/qcom,spi-geni-qcom.txt | 39 ++++++++++++++++++++++
 2 files changed, 41 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
index 16467ed..f0fbeda 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
@@ -53,19 +53,8 @@ Required properties:
 - clocks:		Serial engine core clock needed by the device.
 
 Qualcomm Technologies Inc. GENI Serial Engine based SPI Controller
-
-Required properties:
-- compatible:		Must contain "qcom,geni-spi".
-- reg:			Must contain SPI register location and length.
-- interrupts:		Must contain SPI controller interrupts.
-- clock-names:		Must contain "se".
-- clocks:		Serial engine core clock needed by the device.
-- #address-cells:	Must be <1> to define a chip select address on
-			the SPI bus.
-- #size-cells:		Must be <0>.
-
-SPI slave nodes must be children of the SPI master node and conform to SPI bus
-binding as described in Documentation/devicetree/bindings/spi/spi-bus.txt.
+node binding is described in
+Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt.
 
 Example:
 	geniqup@8c0000 {
@@ -102,16 +91,4 @@ Example:
 			pinctrl-1 = <&qup_1_uart_3_sleep>;
 		};
 
-		spi0: spi@a84000 {
-			compatible = "qcom,geni-spi";
-			reg = <0xa84000 0x4000>;
-			interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
-			clock-names = "se";
-			clocks = <&clock_gcc GCC_QUPV3_WRAP0_S0_CLK>;
-			pinctrl-names = "default", "sleep";
-			pinctrl-0 = <&qup_1_spi_2_active>;
-			pinctrl-1 = <&qup_1_spi_2_sleep>;
-			#address-cells = <1>;
-			#size-cells = <0>;
-		};
 	}
diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt
new file mode 100644
index 0000000..790311a
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt
@@ -0,0 +1,39 @@
+GENI based Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)
+
+The QUP v3 core is a GENI based AHB slave that provides a common data path
+(an output FIFO and an input FIFO) for serial peripheral interface (SPI)
+mini-core.
+
+SPI in master mode supports up to 50MHz, up to four chip selects, programmable
+data path from 4 bits to 32 bits and numerous protocol variants.
+
+Required properties:
+- compatible:		Must contain "qcom,geni-spi".
+- reg:			Must contain SPI register location and length.
+- interrupts:		Must contain SPI controller interrupts.
+- clock-names:		Must contain "se".
+- clocks:		Serial engine core clock needed by the device.
+- #address-cells:	Must be <1> to define a chip select address on
+			the SPI bus.
+- #size-cells:		Must be <0>.
+
+SPI Controller nodes must be child of GENI based Qualcomm Universal
+Peripharal. Please refer GENI based QUP wrapper controller node bindings
+described in Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt.
+
+SPI slave nodes must be children of the SPI master node and conform to SPI bus
+binding as described in Documentation/devicetree/bindings/spi/spi-bus.txt.
+
+Example:
+	spi0: spi@a84000 {
+		compatible = "qcom,geni-spi";
+		reg = <0xa84000 0x4000>;
+		interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
+		clock-names = "se";
+		clocks = <&clock_gcc GCC_QUPV3_WRAP0_S0_CLK>;
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&qup_1_spi_2_active>;
+		pinctrl-1 = <&qup_1_spi_2_sleep>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V4 2/4] dt-bindings: soc: qcom: GENI SE SPI controller device tree binding

[Doug Anderson]

Hi,
On Tue, Sep 18, 2018 at 11:08 AM Dilip Kota <dkota@codeaurora.org> wrote:
>
> Move GENI SE SPI controller device-tree bindings
> from devicetree/bindings/soc/qcom/qcom,geni-se.txt
> to devicetree/bindings/spi/qcom,spi-geni-qcom.txt.
>
> Signed-off-by: Dilip Kota <dkota@codeaurora.org>
> ---
>  .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  | 27 ++-------------
>  .../devicetree/bindings/spi/qcom,spi-geni-qcom.txt | 39 ++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 25 deletions(-)

IMO this matches what Mark Brown requested in a previous email and
seems like a good idea.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH V4 2/4] dt-bindings: soc: qcom: GENI SE SPI controller device tree binding

[Stephen Boyd]

Quoting Dilip Kota (2018-09-18 11:07:24)
> Move GENI SE SPI controller device-tree bindings
> from devicetree/bindings/soc/qcom/qcom,geni-se.txt
> to devicetree/bindings/spi/qcom,spi-geni-qcom.txt.
> 
> Signed-off-by: Dilip Kota <dkota@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH V4 2/4] dt-bindings: soc: qcom: GENI SE SPI controller device tree binding

[Rob Herring]

On Tue, 18 Sep 2018 23:37:24 +0530, Dilip Kota wrote:
> Move GENI SE SPI controller device-tree bindings
> from devicetree/bindings/soc/qcom/qcom,geni-se.txt
> to devicetree/bindings/spi/qcom,spi-geni-qcom.txt.
> 
> Signed-off-by: Dilip Kota <dkota@codeaurora.org>
> ---
>  .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  | 27 ++-------------
>  .../devicetree/bindings/spi/qcom,spi-geni-qcom.txt | 39 ++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 25 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH V4 3/4] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Dilip Kota]

From: Girish Mahadevan <girishm@codeaurora.org>

This driver supports GENI based SPI Controller in the Qualcomm SOCs. The
Qualcomm Generic Interface (GENI) is a programmable module supporting a
wide range of serial interfaces including SPI. This driver supports SPI
operations using FIFO mode of transfer.

Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
Signed-off-by: Dilip Kota <dkota@codeaurora.org>
---
Addressing all the reviewer commets given in Patchset1.
Summerizing the chages below:

	Move prepare_transfer_hardware to probe
	Fix the unbind failure
	Add Spinlock
	Use readl/writel() instead of _relaxed()
	Remove __func__ in dev_err
	Declare register variables as u32
	Don't initialize variables and then overwrite them
	Remove unused step
	Remove IRQ entry in geni struct
	Remove extra paranthesis
	Reframe probe errors
	Change the tx_fifo_width naming in handle_rx and handle_tx
	Declare spi_geni_master as 'mas' in complete driver.
	Correcting the punctuation in the comments.
	Adding comments in the code while setting Tx WaterMark Register.
	get_spi_clk_cfg() fixes
	Rewrite the geni_spi_handle_tx() and geni_spi_handle_rx()

Regarding the suspend/resume and loopback failures, i am working to update in next patch series
drivers/spi/Kconfig               |  12 +
 drivers/spi/Makefile              |   1 +
 drivers/spi/spi-geni-qcom.c       | 653 ++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi-geni-qcom.h |  14 +
 4 files changed, 680 insertions(+)
 create mode 100644 drivers/spi/spi-geni-qcom.c
 create mode 100644 include/linux/spi/spi-geni-qcom.h

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ad5d68e..8934df8 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -533,6 +533,18 @@ config SPI_QUP
 	  This driver can also be built as a module.  If so, the module
 	  will be called spi_qup.
 
+config SPI_QCOM_GENI
+	tristate "Qualcomm GENI based SPI controller"
+	depends on QCOM_GENI_SE
+	help
+	  This driver supports GENI serial engine based SPI controller in
+	  master mode on the Qualcomm Technologies Inc.'s SoCs. If you say
+	  yes to this option, support will be included for the built-in SPI
+	  interface on the Qualcomm Technologies Inc.'s SoCs.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called spi-geni-qcom.
+
 config SPI_S3C24XX
 	tristate "Samsung S3C24XX series SPI"
 	depends on ARCH_S3C24XX
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index cb1f437..98337cf 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -74,6 +74,7 @@ obj-$(CONFIG_SPI_PPC4xx)		+= spi-ppc4xx.o
 spi-pxa2xx-platform-objs		:= spi-pxa2xx.o spi-pxa2xx-dma.o
 obj-$(CONFIG_SPI_PXA2XX)		+= spi-pxa2xx-platform.o
 obj-$(CONFIG_SPI_PXA2XX_PCI)		+= spi-pxa2xx-pci.o
+obj-$(CONFIG_SPI_QCOM_GENI)		+= spi-geni-qcom.o
 obj-$(CONFIG_SPI_QUP)			+= spi-qup.o
 obj-$(CONFIG_SPI_ROCKCHIP)		+= spi-rockchip.o
 obj-$(CONFIG_SPI_RB4XX)			+= spi-rb4xx.o
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
new file mode 100644
index 0000000..949b853
--- /dev/null
+++ b/drivers/spi/spi-geni-qcom.c
@@ -0,0 +1,653 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/log2.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/qcom-geni-se.h>
+#include <linux/spi/spi.h>
+#include <linux/spinlock.h>
+
+/* SPI SE specific registers and respective register fields */
+#define SE_SPI_CPHA		0x224
+#define CPHA			BIT(0)
+
+#define SE_SPI_LOOPBACK		0x22c
+#define LOOPBACK_ENABLE		0x1
+#define NORMAL_MODE		0x0
+#define LOOPBACK_MSK		GENMASK(1, 0)
+
+#define SE_SPI_CPOL		0x230
+#define CPOL			BIT(2)
+
+#define SE_SPI_DEMUX_OUTPUT_INV	0x24c
+#define CS_DEMUX_OUTPUT_INV_MSK	GENMASK(3, 0)
+
+#define SE_SPI_DEMUX_SEL	0x250
+#define CS_DEMUX_OUTPUT_SEL	GENMASK(3, 0)
+
+#define SE_SPI_TRANS_CFG	0x25c
+#define CS_TOGGLE		BIT(0)
+
+#define SE_SPI_WORD_LEN		0x268
+#define WORD_LEN_MSK		GENMASK(9, 0)
+#define MIN_WORD_LEN		4
+
+#define SE_SPI_TX_TRANS_LEN	0x26c
+#define SE_SPI_RX_TRANS_LEN	0x270
+#define TRANS_LEN_MSK		GENMASK(23, 0)
+
+#define SE_SPI_PRE_POST_CMD_DLY	0x274
+
+#define SE_SPI_DELAY_COUNTERS	0x278
+#define SPI_INTER_WORDS_DELAY_MSK	GENMASK(9, 0)
+#define SPI_CS_CLK_DELAY_MSK		GENMASK(19, 10)
+#define SPI_CS_CLK_DELAY_SHFT		10
+
+/* M_CMD OP codes for SPI */
+#define SPI_TX_ONLY		1
+#define SPI_RX_ONLY		2
+#define SPI_FULL_DUPLEX		3
+#define SPI_TX_RX		7
+#define SPI_CS_ASSERT		8
+#define SPI_CS_DEASSERT		9
+#define SPI_SCK_ONLY		10
+/* M_CMD params for SPI */
+#define SPI_PRE_CMD_DELAY	BIT(0)
+#define TIMESTAMP_BEFORE	BIT(1)
+#define FRAGMENTATION		BIT(2)
+#define TIMESTAMP_AFTER		BIT(3)
+#define POST_CMD_DELAY		BIT(4)
+
+static irqreturn_t geni_spi_isr(int irq, void *data);
+
+struct spi_geni_master {
+	struct geni_se se;
+	struct device *dev;
+	u32 rx_fifo_depth;
+	u32 tx_fifo_depth;
+	u32 fifo_width_bits;
+	u32 tx_wm;
+	unsigned int cur_speed_hz;
+	unsigned int cur_bits_per_word;
+	unsigned int tx_rem_bytes;
+	unsigned int rx_rem_bytes;
+	struct spi_transfer *cur_xfer;
+	struct completion xfer_done;
+	unsigned int oversampling;
+	spinlock_t lock;
+};
+
+static int get_spi_clk_cfg(unsigned int speed_hz,
+			struct spi_geni_master *mas,
+			unsigned int *clk_idx,
+			unsigned int *clk_div)
+{
+	unsigned long sclk_freq;
+	unsigned int actual_hz;
+	struct geni_se *se = &mas->se;
+	int ret;
+
+	ret = geni_se_clk_freq_match(&mas->se,
+				speed_hz * mas->oversampling,
+				clk_idx, &sclk_freq, false);
+	if (ret) {
+		dev_err(mas->dev, "Failed(%d) to find src clk for %dHz\n",
+							ret, speed_hz);
+		return ret;
+	}
+
+	*clk_div = DIV_ROUND_UP(sclk_freq, mas->oversampling * speed_hz);
+	actual_hz = sclk_freq / (mas->oversampling * *clk_div);
+
+	dev_dbg(mas->dev, "req %u=>%u sclk %lu, idx %d, div %d\n", speed_hz,
+				actual_hz, sclk_freq, *clk_idx, *clk_div);
+	ret = clk_set_rate(se->clk, sclk_freq);
+	if (ret)
+		dev_err(mas->dev, "clk_set_rate failed %d\n", ret);
+	return ret;
+}
+
+static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
+					unsigned int bits_per_word)
+{
+	unsigned int pack_words;
+	bool msb_first = (mode & SPI_LSB_FIRST) ? false : true;
+	struct geni_se *se = &mas->se;
+	u32 word_len;
+
+	word_len = readl(se->base + SE_SPI_WORD_LEN);
+
+	/*
+	 * If bits_per_word isn't a byte aligned value, set the packing to be
+	 * 1 SPI word per FIFO word.
+	 */
+	if (!(mas->fifo_width_bits % bits_per_word))
+		pack_words = mas->fifo_width_bits / bits_per_word;
+	else
+		pack_words = 1;
+	word_len &= ~WORD_LEN_MSK;
+	word_len |= ((bits_per_word - MIN_WORD_LEN) & WORD_LEN_MSK);
+	geni_se_config_packing(&mas->se, bits_per_word, pack_words, msb_first,
+								true, true);
+	writel(word_len, se->base + SE_SPI_WORD_LEN);
+}
+
+static int setup_fifo_params(struct spi_device *spi_slv,
+					struct spi_master *spi)
+{
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+	struct geni_se *se = &mas->se;
+	u32 loopback_cfg, cpol, cpha, demux_output_inv;
+	u32 demux_sel, clk_sel, m_clk_cfg, idx, div;
+	int ret;
+
+	loopback_cfg = readl(se->base + SE_SPI_LOOPBACK);
+	cpol = readl(se->base + SE_SPI_CPOL);
+	cpha = readl(se->base + SE_SPI_CPHA);
+	demux_output_inv = 0;
+	loopback_cfg &= ~LOOPBACK_MSK;
+	cpol &= ~CPOL;
+	cpha &= ~CPHA;
+
+	if (spi_slv->mode & SPI_LOOP)
+		loopback_cfg |= LOOPBACK_ENABLE;
+
+	if (spi_slv->mode & SPI_CPOL)
+		cpol |= CPOL;
+
+	if (spi_slv->mode & SPI_CPHA)
+		cpha |= CPHA;
+
+	if (spi_slv->mode & SPI_CS_HIGH)
+		demux_output_inv = BIT(spi_slv->chip_select);
+
+	demux_sel = spi_slv->chip_select;
+	mas->cur_speed_hz = spi_slv->max_speed_hz;
+	mas->cur_bits_per_word = spi_slv->bits_per_word;
+
+	ret = get_spi_clk_cfg(mas->cur_speed_hz, mas, &idx, &div);
+	if (ret) {
+		dev_err(mas->dev, "Err setting clks ret(%d) for %d\n",
+							ret, mas->cur_speed_hz);
+		return ret;
+	}
+
+	clk_sel = idx & CLK_SEL_MSK;
+	m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN;
+	spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word);
+	writel(loopback_cfg, se->base + SE_SPI_LOOPBACK);
+	writel(demux_sel, se->base + SE_SPI_DEMUX_SEL);
+	writel(cpha, se->base + SE_SPI_CPHA);
+	writel(cpol, se->base + SE_SPI_CPOL);
+	writel(demux_output_inv, se->base + SE_SPI_DEMUX_OUTPUT_INV);
+	writel(clk_sel, se->base + SE_GENI_CLK_SEL);
+	writel(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);
+	return 0;
+}
+
+static int spi_geni_prepare_message(struct spi_master *spi,
+					struct spi_message *spi_msg)
+{
+	int ret;
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+	struct geni_se *se = &mas->se;
+
+	geni_se_select_mode(se, GENI_SE_FIFO);
+	reinit_completion(&mas->xfer_done);
+	ret = setup_fifo_params(spi_msg->spi, spi);
+	if (ret)
+		dev_err(mas->dev, "Couldn't select mode %d", ret);
+	return ret;
+}
+
+static int spi_geni_init(struct spi_geni_master *mas)
+{
+	struct geni_se *se = &mas->se;
+	unsigned int proto, major, minor, ver;
+
+	pm_runtime_get_sync(mas->dev);
+
+	proto = geni_se_read_proto(se);
+	if (proto != GENI_SE_SPI) {
+		dev_err(mas->dev, "Invalid proto %d\n", proto);
+		pm_runtime_put(mas->dev);
+		return -ENXIO;
+	}
+	mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
+	mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
+
+	/* Width of Tx and Rx FIFO is same */
+	mas->fifo_width_bits = geni_se_get_tx_fifo_width(se);
+
+	/*
+	 * Hardware programming guide suggests to configure
+	 * RX FIFO RFR level to fifo_depth-2.
+	 */
+	geni_se_init(se, 0x0, mas->tx_fifo_depth - 2);
+	/* Transmit an entire FIFO worth of data per IRQ */
+	mas->tx_wm = 1;
+	ver = geni_se_get_qup_hw_version(se);
+	major = GENI_SE_VERSION_MAJOR(ver);
+	minor = GENI_SE_VERSION_MINOR(ver);
+
+	if (major == 1 && minor == 0)
+		mas->oversampling = 2;
+	else
+		mas->oversampling = 1;
+
+	pm_runtime_put(mas->dev);
+	return 0;
+}
+
+static void setup_fifo_xfer(struct spi_transfer *xfer,
+				struct spi_geni_master *mas,
+				u16 mode, struct spi_master *spi)
+{
+	u32 m_cmd = 0, m_param = 0;
+	u32 spi_tx_cfg, trans_len;
+	struct geni_se *se = &mas->se;
+
+	spi_tx_cfg = readl(se->base + SE_SPI_TRANS_CFG);
+	if (xfer->bits_per_word != mas->cur_bits_per_word) {
+		spi_setup_word_len(mas, mode, xfer->bits_per_word);
+		mas->cur_bits_per_word = xfer->bits_per_word;
+	}
+
+	/* Speed and bits per word can be overridden per transfer */
+	if (xfer->speed_hz != mas->cur_speed_hz) {
+		int ret;
+		u32 clk_sel, m_clk_cfg;
+		unsigned int idx, div;
+
+		ret = get_spi_clk_cfg(xfer->speed_hz, mas, &idx, &div);
+		if (ret) {
+			dev_err(mas->dev, "Err setting clks:%d\n", ret);
+			return;
+		}
+		/*
+		 * SPI core clock gets configured with the requested frequency
+		 * or the frequency closer to the requested frequency.
+		 * For that reason requested frequency is stored in the
+		 * cur_speed_hz and referred in the consicutive transfer instead
+		 * of calling clk_get_rate() API.
+		 */
+		mas->cur_speed_hz = xfer->speed_hz;
+		clk_sel = idx & CLK_SEL_MSK;
+		m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN;
+		writel(clk_sel, se->base + SE_GENI_CLK_SEL);
+		writel(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);
+	}
+
+	mas->tx_rem_bytes = 0;
+	mas->rx_rem_bytes = 0;
+	if (xfer->tx_buf && xfer->rx_buf)
+		m_cmd = SPI_FULL_DUPLEX;
+	else if (xfer->tx_buf)
+		m_cmd = SPI_TX_ONLY;
+	else if (xfer->rx_buf)
+		m_cmd = SPI_RX_ONLY;
+
+	spi_tx_cfg &= ~CS_TOGGLE;
+	if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) {
+		trans_len =
+			(xfer->len * BITS_PER_BYTE /
+					mas->cur_bits_per_word) & TRANS_LEN_MSK;
+	} else {
+		unsigned int bytes_per_word =
+			mas->cur_bits_per_word / BITS_PER_BYTE + 1;
+
+		trans_len = (xfer->len / bytes_per_word) & TRANS_LEN_MSK;
+	}
+
+	/*
+	 * If CS change flag is set, then toggle the CS line in between
+	 * transfers and keep CS asserted after the last transfer.
+	 * Else if keep CS flag asserted in between transfers and de-assert
+	 * CS after the last message.
+	 */
+	if (xfer->cs_change) {
+		if (list_is_last(&xfer->transfer_list,
+				&spi->cur_msg->transfers))
+			m_param = FRAGMENTATION;
+	} else {
+		if (!list_is_last(&xfer->transfer_list,
+				&spi->cur_msg->transfers))
+			m_param = FRAGMENTATION;
+	}
+
+	mas->cur_xfer = xfer;
+	if (m_cmd & SPI_TX_ONLY) {
+		mas->tx_rem_bytes = xfer->len;
+		writel(trans_len, se->base + SE_SPI_TX_TRANS_LEN);
+	}
+
+	if (m_cmd & SPI_RX_ONLY) {
+		writel(trans_len, se->base + SE_SPI_RX_TRANS_LEN);
+		mas->rx_rem_bytes = xfer->len;
+	}
+	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
+	geni_se_setup_m_cmd(se, m_cmd, m_param);
+
+	/*
+	 * TX_WATERMARK_REG should be set after SPI configuration and
+	 * setting up GENI SE engine, as driver starts data transfer
+	 * for the watermark interrupt.
+	 */
+	if (m_cmd & SPI_TX_ONLY)
+		writel(mas->tx_wm, se->base + SE_GENI_TX_WATERMARK_REG);
+}
+
+static void handle_fifo_timeout(struct spi_master *spi,
+				struct spi_message *msg)
+{
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+	unsigned long timeout, flags;
+	struct geni_se *se = &mas->se;
+
+	spin_lock_irqsave(&mas->lock, flags);
+	reinit_completion(&mas->xfer_done);
+	geni_se_cancel_m_cmd(se);
+	writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
+	spin_unlock_irqrestore(&mas->lock, flags);
+	timeout = wait_for_completion_timeout(&mas->xfer_done, HZ);
+	if (!timeout) {
+		spin_lock_irqsave(&mas->lock, flags);
+		reinit_completion(&mas->xfer_done);
+		geni_se_abort_m_cmd(se);
+		spin_unlock_irqrestore(&mas->lock, flags);
+		timeout = wait_for_completion_timeout(&mas->xfer_done,
+								HZ);
+		if (!timeout)
+			dev_err(mas->dev,
+				"Failed to cancel/abort m_cmd\n");
+	}
+}
+
+static int spi_geni_transfer_one(struct spi_master *spi,
+				struct spi_device *slv,
+				struct spi_transfer *xfer)
+{
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+
+	setup_fifo_xfer(xfer, mas, slv->mode, spi);
+	return 1;
+}
+
+static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
+{
+	/*
+	 * Calculate how many bytes we'll put in each FIFO word.  If the
+	 * transfer words don't pack cleanly into a FIFO word we'll just put
+	 * one transfer word in each FIFO word.  If they do pack we'll pack 'em.
+	 */
+	if (mas->fifo_width_bits % mas->cur_bits_per_word)
+		return roundup_pow_of_two(DIV_ROUND_UP(mas->cur_bits_per_word,
+						       BITS_PER_BYTE));
+	else
+		return mas->fifo_width_bits / BITS_PER_BYTE;
+}
+
+static irqreturn_t geni_spi_handle_tx(struct spi_geni_master *mas)
+{
+	struct geni_se *se = &mas->se;
+	unsigned int max_bytes;
+	const u8 *tx_buf;
+	unsigned int bytes_per_fifo_word = geni_byte_per_fifo_word(mas);
+	unsigned int i = 0;
+
+	if (!mas->cur_xfer)
+		return IRQ_NONE;
+
+	max_bytes = (mas->tx_fifo_depth - mas->tx_wm) * bytes_per_fifo_word;
+	if (mas->tx_rem_bytes < max_bytes)
+		max_bytes = mas->tx_rem_bytes;
+
+	tx_buf = mas->cur_xfer->tx_buf + mas->cur_xfer->len - mas->tx_rem_bytes;
+	while (i < max_bytes) {
+		unsigned int j;
+		unsigned int bytes_to_write;
+		u32 fifo_word = 0;
+		u8 *fifo_byte = (u8 *)&fifo_word;
+
+		bytes_to_write = min(bytes_per_fifo_word, max_bytes - i);
+		for (j = 0; j < bytes_to_write; j++)
+			fifo_byte[j] = tx_buf[i++];
+		iowrite32_rep(se->base + SE_GENI_TX_FIFOn, &fifo_word, 1);
+	}
+	mas->tx_rem_bytes -= max_bytes;
+	if (!mas->tx_rem_bytes)
+		writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t geni_spi_handle_rx(struct spi_geni_master *mas)
+{
+	struct geni_se *se = &mas->se;
+	u32 rx_fifo_status;
+	unsigned int rx_bytes;
+	u8 *rx_buf;
+	unsigned int bytes_per_fifo_word = geni_byte_per_fifo_word(mas);
+	unsigned int i = 0;
+
+	if (!mas->cur_xfer)
+		return IRQ_NONE;
+
+	rx_fifo_status = readl(se->base + SE_GENI_RX_FIFO_STATUS);
+	rx_bytes = (rx_fifo_status & RX_FIFO_WC_MSK) * bytes_per_fifo_word;
+	if (rx_fifo_status & RX_LAST) {
+		unsigned int rx_last_byte_valid =
+			(rx_fifo_status & RX_LAST_BYTE_VALID_MSK)
+				>> RX_LAST_BYTE_VALID_SHFT;
+		if (rx_last_byte_valid && (rx_last_byte_valid < 4))
+			rx_bytes -= bytes_per_fifo_word - rx_last_byte_valid;
+	}
+	if (mas->rx_rem_bytes < rx_bytes)
+		rx_bytes = mas->rx_rem_bytes;
+
+	rx_buf = mas->cur_xfer->rx_buf + mas->cur_xfer->len - mas->rx_rem_bytes;
+	while (i < rx_bytes) {
+		u32 fifo_word = 0;
+		u8 *fifo_byte = (u8 *)&fifo_word;
+		unsigned int bytes_to_read;
+		unsigned int j;
+
+		bytes_to_read = min(bytes_per_fifo_word, rx_bytes - i);
+		ioread32_rep(se->base + SE_GENI_RX_FIFOn, &fifo_word, 1);
+		for (j = 0; j < bytes_to_read; j++)
+			rx_buf[i++] = fifo_byte[j];
+	}
+	mas->rx_rem_bytes -= rx_bytes;
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t geni_spi_isr(int irq, void *data)
+{
+	struct spi_master *spi = data;
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+	struct geni_se *se = &mas->se;
+	u32 m_irq;
+	unsigned long flags;
+	irqreturn_t ret = IRQ_HANDLED;
+
+	if (pm_runtime_status_suspended(mas->dev))
+		return IRQ_NONE;
+
+	spin_lock_irqsave(&mas->lock, flags);
+	m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);
+	if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))
+		ret = geni_spi_handle_rx(mas);
+
+	if (m_irq & M_TX_FIFO_WATERMARK_EN)
+		ret = geni_spi_handle_tx(mas);
+
+	if (m_irq & M_CMD_DONE_EN) {
+		spi_finalize_current_transfer(spi);
+		/*
+		 * If this happens, then a CMD_DONE came before all the Tx
+		 * buffer bytes were sent out. This is unusual, log this
+		 * condition and disable the WM interrupt to prevent the
+		 * system from stalling due an interrupt storm.
+		 * If this happens when all Rx bytes haven't been received, log
+		 * the condition.
+		 * The only known time this can happen is if bits_per_word != 8
+		 * and some registers that expect xfer lengths in num spi_words
+		 * weren't written correctly.
+		 */
+		if (mas->tx_rem_bytes) {
+			writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
+			dev_err(mas->dev, "Premature Done.tx_rem%d bpw%d\n",
+				mas->tx_rem_bytes, mas->cur_bits_per_word);
+		}
+		if (mas->rx_rem_bytes)
+			dev_err(mas->dev, "Premature Done.rx_rem%d bpw%d\n",
+				mas->rx_rem_bytes, mas->cur_bits_per_word);
+	}
+
+	if ((m_irq & M_CMD_CANCEL_EN) || (m_irq & M_CMD_ABORT_EN))
+		complete(&mas->xfer_done);
+
+	writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
+	spin_unlock_irqrestore(&mas->lock, flags);
+	return ret;
+}
+
+static int spi_geni_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct spi_master *spi;
+	struct spi_geni_master *mas;
+	struct resource *res;
+	struct geni_se *se;
+
+	spi = spi_alloc_master(&pdev->dev, sizeof(struct spi_geni_master));
+	if (!spi)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, spi);
+	mas = spi_master_get_devdata(spi);
+	mas->dev = &pdev->dev;
+	mas->se.dev = &pdev->dev;
+	mas->se.wrapper = dev_get_drvdata(pdev->dev.parent);
+	se = &mas->se;
+
+	spi->bus_num = -1;
+	spi->dev.of_node = pdev->dev.of_node;
+	mas->se.clk = devm_clk_get(&pdev->dev, "se");
+	if (IS_ERR(mas->se.clk)) {
+		ret = PTR_ERR(mas->se.clk);
+		dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
+		goto spi_geni_probe_err;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	se->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(se->base)) {
+		ret = -ENOMEM;
+		goto spi_geni_probe_err;
+	}
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
+		goto spi_geni_probe_err;
+	}
+	ret = devm_request_irq(&pdev->dev, ret, geni_spi_isr,
+				IRQF_TRIGGER_HIGH, "spi_geni", spi);
+	if (ret)
+		goto spi_geni_probe_err;
+
+	spi->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_CS_HIGH;
+	spi->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
+	spi->num_chipselect = 4;
+	spi->max_speed_hz = 50000000;
+	spi->prepare_message = spi_geni_prepare_message;
+	spi->transfer_one = spi_geni_transfer_one;
+	spi->auto_runtime_pm = true;
+	spi->handle_err = handle_fifo_timeout;
+
+	init_completion(&mas->xfer_done);
+	spin_lock_init(&mas->lock);
+	pm_runtime_enable(&pdev->dev);
+
+	ret = spi_geni_init(mas);
+	if (ret)
+		goto spi_geni_probe_runtime_disable;
+
+	ret = spi_register_master(spi);
+	if (ret)
+		goto spi_geni_probe_runtime_disable;
+
+	return 0;
+spi_geni_probe_runtime_disable:
+	pm_runtime_disable(&pdev->dev);
+spi_geni_probe_err:
+	spi_master_put(spi);
+	return ret;
+}
+
+static int spi_geni_remove(struct platform_device *pdev)
+{
+	struct spi_master *spi = platform_get_drvdata(pdev);
+
+	/* Unregister _before_ disabling pm_runtime() so we stop transfers */
+	spi_unregister_master(spi);
+
+	pm_runtime_disable(&pdev->dev);
+	return 0;
+}
+
+static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
+{
+	struct spi_master *spi = dev_get_drvdata(dev);
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+
+	return geni_se_resources_off(&mas->se);
+}
+
+static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
+{
+	struct spi_master *spi = dev_get_drvdata(dev);
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+
+	return geni_se_resources_on(&mas->se);
+}
+
+static int __maybe_unused spi_geni_suspend(struct device *dev)
+{
+	if (!pm_runtime_status_suspended(dev))
+		return -EBUSY;
+	return 0;
+}
+
+static const struct dev_pm_ops spi_geni_pm_ops = {
+	SET_RUNTIME_PM_OPS(spi_geni_runtime_suspend,
+					spi_geni_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(spi_geni_suspend, NULL)
+};
+
+static const struct of_device_id spi_geni_dt_match[] = {
+	{ .compatible = "qcom,geni-spi" },
+	{}
+};
+
+static struct platform_driver spi_geni_driver = {
+	.probe  = spi_geni_probe,
+	.remove = spi_geni_remove,
+	.driver = {
+		.name = "geni_spi",
+		.pm = &spi_geni_pm_ops,
+		.of_match_table = spi_geni_dt_match,
+	},
+};
+module_platform_driver(spi_geni_driver);
+
+MODULE_DESCRIPTION("SPI driver for GENI based QUP cores");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/spi/spi-geni-qcom.h b/include/linux/spi/spi-geni-qcom.h
new file mode 100644
index 0000000..dc95764
--- /dev/null
+++ b/include/linux/spi/spi-geni-qcom.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __SPI_GENI_QCOM_HEADER___
+#define __SPI_GENI_QCOM_HEADER___
+
+struct spi_geni_qcom_ctrl_data {
+	u32 spi_cs_clk_delay;
+	u32 spi_inter_words_delay;
+};
+
+#endif /*__SPI_GENI_QCOM_HEADER___*/
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V4 3/4] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Doug Anderson]

Hi,

On Tue, Sep 18, 2018 at 11:09 AM Dilip Kota <dkota@codeaurora.org> wrote:
>
> From: Girish Mahadevan <girishm@codeaurora.org>
>
> This driver supports GENI based SPI Controller in the Qualcomm SOCs. The
> Qualcomm Generic Interface (GENI) is a programmable module supporting a
> wide range of serial interfaces including SPI. This driver supports SPI
> operations using FIFO mode of transfer.
>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Dilip Kota <dkota@codeaurora.org>
> ---
> Addressing all the reviewer commets given in Patchset1.
> Summerizing the chages below:
>
>         Move prepare_transfer_hardware to probe
>         Fix the unbind failure
>         Add Spinlock
>         Use readl/writel() instead of _relaxed()
>         Remove __func__ in dev_err
>         Declare register variables as u32
>         Don't initialize variables and then overwrite them
>         Remove unused step
>         Remove IRQ entry in geni struct
>         Remove extra paranthesis
>         Reframe probe errors
>         Change the tx_fifo_width naming in handle_rx and handle_tx
>         Declare spi_geni_master as 'mas' in complete driver.
>         Correcting the punctuation in the comments.
>         Adding comments in the code while setting Tx WaterMark Register.
>         get_spi_clk_cfg() fixes
>         Rewrite the geni_spi_handle_tx() and geni_spi_handle_rx()
>
> Regarding the suspend/resume and loopback failures, i am working to update in next patch series
> drivers/spi/Kconfig               |  12 +
>  drivers/spi/Makefile              |   1 +
>  drivers/spi/spi-geni-qcom.c       | 653 ++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi-geni-qcom.h |  14 +
>  4 files changed, 680 insertions(+)

I've confirmed that all the feedback I'm aware of from previous
versions of this patch has now been addressed, except as noted above:

* suspend callback still looks a little wonky.
* loopback doesn't actually work

IMO it would be OK to land this patch and address the suspend callback
/ loopback issues in later patches, but obviously that's for Mark to
decide.

NOTE: for this patch to land it must be based on a tree that includes
the patch ("soc: qcom: geni: Make version macros simpler") [1] in
order to compile.  It would also be nice if it was also based upon the
patches ("soc: qcom: geni: Don't ignore clk_round_rate() errors in
geni_se_clk_tbl_get()") [2] and ("soc: qcom: geni:
geni_se_clk_freq_match() should always accept multiples") [3].  Those
aren't needed for things to compile but are technically needed for
correctness.  +Andy Gross has said he'll put that the above 3 patches
in his tree in a way that it would be easy to pull in

Mark: it would be super if you had a chance to take a look at this
patch now and see what you think.

[1] https://lkml.kernel.org/r/20180518224750.232742-1-swboyd@chromium.org
[2] https://lkml.kernel.org/r/20180906224906.93752-1-dianders@chromium.org
[3] https://lkml.kernel.org/r/20180906224906.93752-2-dianders@chromium.org

With all that said, I'm happy adding:

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>


-Doug

Re: [PATCH V4 3/4] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[kbuild test robot]

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

Hi Girish,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on v4.19-rc4 next-20180918]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dilip-Kota/spi-geni-qcom-QUP-SPI-GENI-driver-and-SPI-device-tree-bindings/20180919-043055
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   drivers/spi/spi-geni-qcom.c: In function 'spi_geni_init':
>> drivers/spi/spi-geni-qcom.c:236:10: error: implicit declaration of function 'GENI_SE_VERSION_MAJOR'; did you mean 'XCHAL_HW_VERSION_MAJOR'? [-Werror=implicit-function-declaration]
     major = GENI_SE_VERSION_MAJOR(ver);
             ^~~~~~~~~~~~~~~~~~~~~
             XCHAL_HW_VERSION_MAJOR
>> drivers/spi/spi-geni-qcom.c:237:10: error: implicit declaration of function 'GENI_SE_VERSION_MINOR'; did you mean 'XCHAL_HW_VERSION_MINOR'? [-Werror=implicit-function-declaration]
     minor = GENI_SE_VERSION_MINOR(ver);
             ^~~~~~~~~~~~~~~~~~~~~
             XCHAL_HW_VERSION_MINOR
   cc1: some warnings being treated as errors

vim +236 drivers/spi/spi-geni-qcom.c

   208	
   209	static int spi_geni_init(struct spi_geni_master *mas)
   210	{
   211		struct geni_se *se = &mas->se;
   212		unsigned int proto, major, minor, ver;
   213	
   214		pm_runtime_get_sync(mas->dev);
   215	
   216		proto = geni_se_read_proto(se);
   217		if (proto != GENI_SE_SPI) {
   218			dev_err(mas->dev, "Invalid proto %d\n", proto);
   219			pm_runtime_put(mas->dev);
   220			return -ENXIO;
   221		}
   222		mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
   223		mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
   224	
   225		/* Width of Tx and Rx FIFO is same */
   226		mas->fifo_width_bits = geni_se_get_tx_fifo_width(se);
   227	
   228		/*
   229		 * Hardware programming guide suggests to configure
   230		 * RX FIFO RFR level to fifo_depth-2.
   231		 */
   232		geni_se_init(se, 0x0, mas->tx_fifo_depth - 2);
   233		/* Transmit an entire FIFO worth of data per IRQ */
   234		mas->tx_wm = 1;
   235		ver = geni_se_get_qup_hw_version(se);
 > 236		major = GENI_SE_VERSION_MAJOR(ver);
 > 237		minor = GENI_SE_VERSION_MINOR(ver);
   238	
   239		if (major == 1 && minor == 0)
   240			mas->oversampling = 2;
   241		else
   242			mas->oversampling = 1;
   243	
   244		pm_runtime_put(mas->dev);
   245		return 0;
   246	}
   247	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55176 bytes --]

Re: [PATCH V4 3/4] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Stephen Boyd]

Quoting Dilip Kota (2018-09-18 11:07:25)
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> new file mode 100644
> index 0000000..949b853
> --- /dev/null
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -0,0 +1,653 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/qcom-geni-se.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spinlock.h>
> +
> +/* SPI SE specific registers and respective register fields */
> +#define SE_SPI_CPHA            0x224
> +#define CPHA                   BIT(0)
> +
> +#define SE_SPI_LOOPBACK                0x22c
> +#define LOOPBACK_ENABLE                0x1
> +#define NORMAL_MODE            0x0
> +#define LOOPBACK_MSK           GENMASK(1, 0)
> +
> +#define SE_SPI_CPOL            0x230
> +#define CPOL                   BIT(2)
> +
> +#define SE_SPI_DEMUX_OUTPUT_INV        0x24c
> +#define CS_DEMUX_OUTPUT_INV_MSK        GENMASK(3, 0)
> +
> +#define SE_SPI_DEMUX_SEL       0x250
> +#define CS_DEMUX_OUTPUT_SEL    GENMASK(3, 0)
> +
> +#define SE_SPI_TRANS_CFG       0x25c
> +#define CS_TOGGLE              BIT(0)
> +
> +#define SE_SPI_WORD_LEN                0x268
> +#define WORD_LEN_MSK           GENMASK(9, 0)
> +#define MIN_WORD_LEN           4
> +
> +#define SE_SPI_TX_TRANS_LEN    0x26c
> +#define SE_SPI_RX_TRANS_LEN    0x270
> +#define TRANS_LEN_MSK          GENMASK(23, 0)
> +
> +#define SE_SPI_PRE_POST_CMD_DLY        0x274
> +
> +#define SE_SPI_DELAY_COUNTERS  0x278
> +#define SPI_INTER_WORDS_DELAY_MSK      GENMASK(9, 0)
> +#define SPI_CS_CLK_DELAY_MSK           GENMASK(19, 10)
> +#define SPI_CS_CLK_DELAY_SHFT          10
> +
> +/* M_CMD OP codes for SPI */
> +#define SPI_TX_ONLY            1
> +#define SPI_RX_ONLY            2
> +#define SPI_FULL_DUPLEX                3
> +#define SPI_TX_RX              7
> +#define SPI_CS_ASSERT          8
> +#define SPI_CS_DEASSERT                9
> +#define SPI_SCK_ONLY           10
> +/* M_CMD params for SPI */
> +#define SPI_PRE_CMD_DELAY      BIT(0)
> +#define TIMESTAMP_BEFORE       BIT(1)
> +#define FRAGMENTATION          BIT(2)
> +#define TIMESTAMP_AFTER                BIT(3)
> +#define POST_CMD_DELAY         BIT(4)
> +
> +static irqreturn_t geni_spi_isr(int irq, void *data);

Does this need to be forward declared?

> +
> +struct spi_geni_master {
> +       struct geni_se se;
> +       struct device *dev;
> +       u32 rx_fifo_depth;

Is this used?

> +       u32 tx_fifo_depth;
> +       u32 fifo_width_bits;
> +       u32 tx_wm;
> +       unsigned int cur_speed_hz;

unsigned long for Hz? The clk framework uses that type.

> +       unsigned int cur_bits_per_word;
> +       unsigned int tx_rem_bytes;
> +       unsigned int rx_rem_bytes;
> +       struct spi_transfer *cur_xfer;

const?

> +       struct completion xfer_done;
> +       unsigned int oversampling;
> +       spinlock_t lock;
> +};
> +
[...]
> +
> +static int spi_geni_prepare_message(struct spi_master *spi,
> +                                       struct spi_message *spi_msg)
> +{
> +       int ret;
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +       struct geni_se *se = &mas->se;
> +
> +       geni_se_select_mode(se, GENI_SE_FIFO);
> +       reinit_completion(&mas->xfer_done);
> +       ret = setup_fifo_params(spi_msg->spi, spi);
> +       if (ret)
> +               dev_err(mas->dev, "Couldn't select mode %d", ret);

Missing newline on error print.

> +       return ret;
> +}
> +
[...]
> +
> +static void setup_fifo_xfer(struct spi_transfer *xfer,
> +                               struct spi_geni_master *mas,
> +                               u16 mode, struct spi_master *spi)
> +{
> +       u32 m_cmd = 0, m_param = 0;
> +       u32 spi_tx_cfg, trans_len;
> +       struct geni_se *se = &mas->se;
> +
> +       spi_tx_cfg = readl(se->base + SE_SPI_TRANS_CFG);
> +       if (xfer->bits_per_word != mas->cur_bits_per_word) {
> +               spi_setup_word_len(mas, mode, xfer->bits_per_word);
> +               mas->cur_bits_per_word = xfer->bits_per_word;
> +       }
> +
> +       /* Speed and bits per word can be overridden per transfer */
> +       if (xfer->speed_hz != mas->cur_speed_hz) {
> +               int ret;
> +               u32 clk_sel, m_clk_cfg;
> +               unsigned int idx, div;
> +
> +               ret = get_spi_clk_cfg(xfer->speed_hz, mas, &idx, &div);
> +               if (ret) {
> +                       dev_err(mas->dev, "Err setting clks:%d\n", ret);
> +                       return;
> +               }
> +               /*
> +                * SPI core clock gets configured with the requested frequency
> +                * or the frequency closer to the requested frequency.
> +                * For that reason requested frequency is stored in the
> +                * cur_speed_hz and referred in the consicutive transfer instead

s/consicutive/consecutive/

> +                * of calling clk_get_rate() API.
> +                */
> +               mas->cur_speed_hz = xfer->speed_hz;
> +               clk_sel = idx & CLK_SEL_MSK;
> +               m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN;
> +               writel(clk_sel, se->base + SE_GENI_CLK_SEL);
> +               writel(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);
> +       }
> +
> +       mas->tx_rem_bytes = 0;
> +       mas->rx_rem_bytes = 0;
> +       if (xfer->tx_buf && xfer->rx_buf)
> +               m_cmd = SPI_FULL_DUPLEX;
> +       else if (xfer->tx_buf)
> +               m_cmd = SPI_TX_ONLY;
> +       else if (xfer->rx_buf)
> +               m_cmd = SPI_RX_ONLY;
> +
> +       spi_tx_cfg &= ~CS_TOGGLE;
> +       if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) {
> +               trans_len =
> +                       (xfer->len * BITS_PER_BYTE /
> +                                       mas->cur_bits_per_word) & TRANS_LEN_MSK;
> +       } else {
> +               unsigned int bytes_per_word =
> +                       mas->cur_bits_per_word / BITS_PER_BYTE + 1;
> +
> +               trans_len = (xfer->len / bytes_per_word) & TRANS_LEN_MSK;
> +       }

Rename 'trans_len' to 'len' and shorten some lines by taking out the
mask to get:

       if (!(mas->cur_bits_per_word % MIN_WORD_LEN))
               len = xfer->len * BITS_PER_BYTE / mas->cur_bits_per_word
       else 
               len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
       len &= TRANS_LEN_MSK;

> +
> +       /*
> +        * If CS change flag is set, then toggle the CS line in between
> +        * transfers and keep CS asserted after the last transfer.
> +        * Else if keep CS flag asserted in between transfers and de-assert
> +        * CS after the last message.
> +        */
> +       if (xfer->cs_change) {
> +               if (list_is_last(&xfer->transfer_list,
> +                               &spi->cur_msg->transfers))
> +                       m_param = FRAGMENTATION;
> +       } else {
> +               if (!list_is_last(&xfer->transfer_list,
> +                               &spi->cur_msg->transfers))
> +                       m_param = FRAGMENTATION;
> +       }
> +
> +       mas->cur_xfer = xfer;
> +       if (m_cmd & SPI_TX_ONLY) {
> +               mas->tx_rem_bytes = xfer->len;
> +               writel(trans_len, se->base + SE_SPI_TX_TRANS_LEN);
> +       }
> +
> +       if (m_cmd & SPI_RX_ONLY) {
> +               writel(trans_len, se->base + SE_SPI_RX_TRANS_LEN);
> +               mas->rx_rem_bytes = xfer->len;
> +       }
> +       writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
> +       geni_se_setup_m_cmd(se, m_cmd, m_param);
> +
> +       /*
> +        * TX_WATERMARK_REG should be set after SPI configuration and
> +        * setting up GENI SE engine, as driver starts data transfer
> +        * for the watermark interrupt.
> +        */
> +       if (m_cmd & SPI_TX_ONLY)
> +               writel(mas->tx_wm, se->base + SE_GENI_TX_WATERMARK_REG);
> +}
> +
> +static void handle_fifo_timeout(struct spi_master *spi,
> +                               struct spi_message *msg)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +       unsigned long timeout, flags;
> +       struct geni_se *se = &mas->se;
> +
> +       spin_lock_irqsave(&mas->lock, flags);
> +       reinit_completion(&mas->xfer_done);
> +       geni_se_cancel_m_cmd(se);
> +       writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> +       spin_unlock_irqrestore(&mas->lock, flags);
> +       timeout = wait_for_completion_timeout(&mas->xfer_done, HZ);

Can you rename this time_left or completed? Then the if condition reads
properly as "if not time left" or "if not completed". And then invert
that logic so things aren't so indented?

	time_left = wait_for_completion_timeout(..)
	if (time_left)
		return;

	spin_lock_irqsave(&mas->lock, flags);
	reinit_completion(..)
	...

> +       if (!timeout) {
> +               spin_lock_irqsave(&mas->lock, flags);
> +               reinit_completion(&mas->xfer_done);
> +               geni_se_abort_m_cmd(se);
> +               spin_unlock_irqrestore(&mas->lock, flags);
> +               timeout = wait_for_completion_timeout(&mas->xfer_done,
> +                                                               HZ);
> +               if (!timeout)
> +                       dev_err(mas->dev,
> +                               "Failed to cancel/abort m_cmd\n");
> +       }
> +}
> +
> +static int spi_geni_transfer_one(struct spi_master *spi,
> +                               struct spi_device *slv,
> +                               struct spi_transfer *xfer)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +
> +       setup_fifo_xfer(xfer, mas, slv->mode, spi);
> +       return 1;
> +}
> +
> +static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
> +{
> +       /*
> +        * Calculate how many bytes we'll put in each FIFO word.  If the
> +        * transfer words don't pack cleanly into a FIFO word we'll just put
> +        * one transfer word in each FIFO word.  If they do pack we'll pack 'em.
> +        */
> +       if (mas->fifo_width_bits % mas->cur_bits_per_word)
> +               return roundup_pow_of_two(DIV_ROUND_UP(mas->cur_bits_per_word,
> +                                                      BITS_PER_BYTE));
> +       else
> +               return mas->fifo_width_bits / BITS_PER_BYTE;

Just do a return. No else please.

> +}
> +
> +static irqreturn_t geni_spi_handle_tx(struct spi_geni_master *mas)
> +{
> +       struct geni_se *se = &mas->se;
> +       unsigned int max_bytes;
> +       const u8 *tx_buf;
> +       unsigned int bytes_per_fifo_word = geni_byte_per_fifo_word(mas);
> +       unsigned int i = 0;
> +
> +       if (!mas->cur_xfer)
> +               return IRQ_NONE;
> +
> +       max_bytes = (mas->tx_fifo_depth - mas->tx_wm) * bytes_per_fifo_word;
> +       if (mas->tx_rem_bytes < max_bytes)
> +               max_bytes = mas->tx_rem_bytes;
> +
> +       tx_buf = mas->cur_xfer->tx_buf + mas->cur_xfer->len - mas->tx_rem_bytes;
> +       while (i < max_bytes) {
> +               unsigned int j;
> +               unsigned int bytes_to_write;
> +               u32 fifo_word = 0;
> +               u8 *fifo_byte = (u8 *)&fifo_word;
> +
> +               bytes_to_write = min(bytes_per_fifo_word, max_bytes - i);
> +               for (j = 0; j < bytes_to_write; j++)
> +                       fifo_byte[j] = tx_buf[i++];
> +               iowrite32_rep(se->base + SE_GENI_TX_FIFOn, &fifo_word, 1);
> +       }
> +       mas->tx_rem_bytes -= max_bytes;
> +       if (!mas->tx_rem_bytes)
> +               writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t geni_spi_handle_rx(struct spi_geni_master *mas)
> +{
> +       struct geni_se *se = &mas->se;
> +       u32 rx_fifo_status;
> +       unsigned int rx_bytes;
> +       u8 *rx_buf;
> +       unsigned int bytes_per_fifo_word = geni_byte_per_fifo_word(mas);
> +       unsigned int i = 0;
> +
> +       if (!mas->cur_xfer)
> +               return IRQ_NONE;
> +
> +       rx_fifo_status = readl(se->base + SE_GENI_RX_FIFO_STATUS);

Rename to 'status'? rx_fifo is implicit in the register macro.

> +       rx_bytes = (rx_fifo_status & RX_FIFO_WC_MSK) * bytes_per_fifo_word;
> +       if (rx_fifo_status & RX_LAST) {
> +               unsigned int rx_last_byte_valid =
> +                       (rx_fifo_status & RX_LAST_BYTE_VALID_MSK)
> +                               >> RX_LAST_BYTE_VALID_SHFT;

Hoist this variable into function scope? So then the line is:

		last_valid = status & RX_LAST_BYTE_VALID_MSK;
		last_valid >>= RX_LAST_BYTE_VALID_SHFT;

> +               if (rx_last_byte_valid && (rx_last_byte_valid < 4))

Drop useless parenthesis please.

> +                       rx_bytes -= bytes_per_fifo_word - rx_last_byte_valid;
> +       }
> +       if (mas->rx_rem_bytes < rx_bytes)
> +               rx_bytes = mas->rx_rem_bytes;
> +
> +       rx_buf = mas->cur_xfer->rx_buf + mas->cur_xfer->len - mas->rx_rem_bytes;
> +       while (i < rx_bytes) {
> +               u32 fifo_word = 0;
> +               u8 *fifo_byte = (u8 *)&fifo_word;
> +               unsigned int bytes_to_read;
> +               unsigned int j;
> +
> +               bytes_to_read = min(bytes_per_fifo_word, rx_bytes - i);
> +               ioread32_rep(se->base + SE_GENI_RX_FIFOn, &fifo_word, 1);
> +               for (j = 0; j < bytes_to_read; j++)
> +                       rx_buf[i++] = fifo_byte[j];
> +       }
> +       mas->rx_rem_bytes -= rx_bytes;
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t geni_spi_isr(int irq, void *data)
> +{
> +       struct spi_master *spi = data;
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +       struct geni_se *se = &mas->se;
> +       u32 m_irq;
> +       unsigned long flags;
> +       irqreturn_t ret = IRQ_HANDLED;
> +
> +       if (pm_runtime_status_suspended(mas->dev))
> +               return IRQ_NONE;
> +
> +       spin_lock_irqsave(&mas->lock, flags);
> +       m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);
> +       if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))
> +               ret = geni_spi_handle_rx(mas);
> +
> +       if (m_irq & M_TX_FIFO_WATERMARK_EN)
> +               ret = geni_spi_handle_tx(mas);

Is it possible for all three conditions above to happen in one
interrupt? I ask because 'ret' is overwritten and so what may have been
IRQ_HANDLED may become IRQ_NONE which will lead to confusion in the irq
layer. Maybe the handle tx/rx functions can return a bool, that gets
orred together each time so that we know if something handled an
interrupt?

> +
> +       if (m_irq & M_CMD_DONE_EN) {
> +               spi_finalize_current_transfer(spi);
> +               /*
> +                * If this happens, then a CMD_DONE came before all the Tx
> +                * buffer bytes were sent out. This is unusual, log this
> +                * condition and disable the WM interrupt to prevent the
> +                * system from stalling due an interrupt storm.
> +                * If this happens when all Rx bytes haven't been received, log
> +                * the condition.
> +                * The only known time this can happen is if bits_per_word != 8
> +                * and some registers that expect xfer lengths in num spi_words
> +                * weren't written correctly.
> +                */
> +               if (mas->tx_rem_bytes) {
> +                       writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> +                       dev_err(mas->dev, "Premature Done.tx_rem%d bpw%d\n",

Why isn't there a space after "Done."? And why is 'done' capitalized?
Should 'tx_rem' be 'tx_rem='?

> +                               mas->tx_rem_bytes, mas->cur_bits_per_word);
> +               }
> +               if (mas->rx_rem_bytes)
> +                       dev_err(mas->dev, "Premature Done.rx_rem%d bpw%d\n",
> +                               mas->rx_rem_bytes, mas->cur_bits_per_word);
> +       }
> +
> +       if ((m_irq & M_CMD_CANCEL_EN) || (m_irq & M_CMD_ABORT_EN))
> +               complete(&mas->xfer_done);
> +
> +       writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
> +       spin_unlock_irqrestore(&mas->lock, flags);
> +       return ret;
> +}
> +
> +static int spi_geni_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       struct spi_master *spi;
> +       struct spi_geni_master *mas;
> +       struct resource *res;
> +       struct geni_se *se;
> +
> +       spi = spi_alloc_master(&pdev->dev, sizeof(struct spi_geni_master));

sizeof(*mas) for code clarity?

> +       if (!spi)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, spi);
> +       mas = spi_master_get_devdata(spi);
> +       mas->dev = &pdev->dev;
> +       mas->se.dev = &pdev->dev;
> +       mas->se.wrapper = dev_get_drvdata(pdev->dev.parent);
> +       se = &mas->se;
> +
> +       spi->bus_num = -1;
> +       spi->dev.of_node = pdev->dev.of_node;
> +       mas->se.clk = devm_clk_get(&pdev->dev, "se");
> +       if (IS_ERR(mas->se.clk)) {
> +               ret = PTR_ERR(mas->se.clk);
> +               dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
> +               goto spi_geni_probe_err;
> +       }
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       se->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(se->base)) {
> +               ret = -ENOMEM;

		ret = PTR_ERR(se->base);

so we don't lose the error value.

> +               goto spi_geni_probe_err;
> +       }
> +
> +       ret = platform_get_irq(pdev, 0);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
> +               goto spi_geni_probe_err;
> +       }
> +       ret = devm_request_irq(&pdev->dev, ret, geni_spi_isr,
> +                               IRQF_TRIGGER_HIGH, "spi_geni", spi);
> +       if (ret)
> +               goto spi_geni_probe_err;

Can you request this irq as late as possible in the probe function? I
worry there may be some pending irq line set and then this will cause an
interrupt storm with IRQ_NONE returned because the device is runtime
suspended.

> +
> +       spi->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_CS_HIGH;
> +       spi->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> +       spi->num_chipselect = 4;
> +       spi->max_speed_hz = 50000000;
> +       spi->prepare_message = spi_geni_prepare_message;
> +       spi->transfer_one = spi_geni_transfer_one;
> +       spi->auto_runtime_pm = true;
> +       spi->handle_err = handle_fifo_timeout;
> +
> +       init_completion(&mas->xfer_done);
> +       spin_lock_init(&mas->lock);
> +       pm_runtime_enable(&pdev->dev);
> +
> +       ret = spi_geni_init(mas);
> +       if (ret)
> +               goto spi_geni_probe_runtime_disable;
> +
> +       ret = spi_register_master(spi);
> +       if (ret)
> +               goto spi_geni_probe_runtime_disable;
> +
> +       return 0;
> +spi_geni_probe_runtime_disable:
> +       pm_runtime_disable(&pdev->dev);
> +spi_geni_probe_err:
> +       spi_master_put(spi);
> +       return ret;
> +}
> +
> +static int spi_geni_remove(struct platform_device *pdev)
> +{
> +       struct spi_master *spi = platform_get_drvdata(pdev);
> +
> +       /* Unregister _before_ disabling pm_runtime() so we stop transfers */
> +       spi_unregister_master(spi);
> +
> +       pm_runtime_disable(&pdev->dev);
> +       return 0;
> +}
> +
> +static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
> +{
> +       struct spi_master *spi = dev_get_drvdata(dev);
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +
> +       return geni_se_resources_off(&mas->se);
> +}
> +
> +static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
> +{
> +       struct spi_master *spi = dev_get_drvdata(dev);
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +
> +       return geni_se_resources_on(&mas->se);
> +}
> +
> +static int __maybe_unused spi_geni_suspend(struct device *dev)
> +{
> +       if (!pm_runtime_status_suspended(dev))
> +               return -EBUSY;
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops spi_geni_pm_ops = {
> +       SET_RUNTIME_PM_OPS(spi_geni_runtime_suspend,
> +                                       spi_geni_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(spi_geni_suspend, NULL)
> +};
> +
> +static const struct of_device_id spi_geni_dt_match[] = {
> +       { .compatible = "qcom,geni-spi" },
> +       {}
> +};

Please add a MODULE_DEVICE_TABLE(of, spi_geni_dt_match) here.

> +
> +static struct platform_driver spi_geni_driver = {
> +       .probe  = spi_geni_probe,
> +       .remove = spi_geni_remove,
> +       .driver = {
> +               .name = "geni_spi",
> +               .pm = &spi_geni_pm_ops,
> +               .of_match_table = spi_geni_dt_match,
> +       },
> +};
> +module_platform_driver(spi_geni_driver);
> +
> +MODULE_DESCRIPTION("SPI driver for GENI based QUP cores");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/spi/spi-geni-qcom.h b/include/linux/spi/spi-geni-qcom.h
> new file mode 100644
> index 0000000..dc95764
> --- /dev/null
> +++ b/include/linux/spi/spi-geni-qcom.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef __SPI_GENI_QCOM_HEADER___
> +#define __SPI_GENI_QCOM_HEADER___
> +
> +struct spi_geni_qcom_ctrl_data {
> +       u32 spi_cs_clk_delay;
> +       u32 spi_inter_words_delay;

It was decided we still needed this? I don't see it in v3 so please
remove this file unless it's needed.

Re: [PATCH V4 3/4] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[dkota]

[...]
>> +#define TIMESTAMP_BEFORE       BIT(1)
>> +#define FRAGMENTATION          BIT(2)
>> +#define TIMESTAMP_AFTER                BIT(3)
>> +#define POST_CMD_DELAY         BIT(4)
>> +
>> +static irqreturn_t geni_spi_isr(int irq, void *data);
> 
> Does this need to be forward declared?
Not required, will remove it.
> 
>> +
>> +struct spi_geni_master {
>> +       struct geni_se se;
>> +       struct device *dev;
>> +       u32 rx_fifo_depth;
> 
> Is this used?
It can be removed.
> 
>> +       u32 tx_fifo_depth;
>> +       u32 fifo_width_bits;
>> +       u32 tx_wm;
>> +       unsigned int cur_speed_hz;
> 
> unsigned long for Hz? The clk framework uses that type.

cur_speed_hz stores the speed value requested as part of transfer (not 
the resultant or rounded off frequency got from clk framework. It is u32 
type, i will change cur_speed_hz to u32 type instead of unsigned long.
Code snippet:
         mas->cur_speed_hz = xfer->speed_hz;

> 
>> +       unsigned int cur_bits_per_word;
>> +       unsigned int tx_rem_bytes;
>> +       unsigned int rx_rem_bytes;
>> +       struct spi_transfer *cur_xfer;
> 
> const?
Yes can be marked as const, will update it.
> 
>> +       struct completion xfer_done;
>> +       unsigned int oversampling;
>> +       spinlock_t lock;
>> +};
>> +
> [...]
>> +
>> +static int spi_geni_prepare_message(struct spi_master *spi,
>> +                                       struct spi_message *spi_msg)
>> +{
>> +       int ret;
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +       struct geni_se *se = &mas->se;
>> +
>> +       geni_se_select_mode(se, GENI_SE_FIFO);
>> +       reinit_completion(&mas->xfer_done);
>> +       ret = setup_fifo_params(spi_msg->spi, spi);
>> +       if (ret)
>> +               dev_err(mas->dev, "Couldn't select mode %d", ret);
> 
> Missing newline on error print.
> 
Will add it.
>> +       return ret;
>> +}
>> +
> [...]
>> +
>> +static void setup_fifo_xfer(struct spi_transfer *xfer,
>> +                               struct spi_geni_master *mas,
>> +                               u16 mode, struct spi_master *spi)
>> +{
>> +       u32 m_cmd = 0, m_param = 0;
>> +       u32 spi_tx_cfg, trans_len;
>> +       struct geni_se *se = &mas->se;
>> +
>> +       spi_tx_cfg = readl(se->base + SE_SPI_TRANS_CFG);
>> +       if (xfer->bits_per_word != mas->cur_bits_per_word) {
>> +               spi_setup_word_len(mas, mode, xfer->bits_per_word);
>> +               mas->cur_bits_per_word = xfer->bits_per_word;
>> +       }
>> +
>> +       /* Speed and bits per word can be overridden per transfer */
>> +       if (xfer->speed_hz != mas->cur_speed_hz) {
>> +               int ret;
>> +               u32 clk_sel, m_clk_cfg;
>> +               unsigned int idx, div;
>> +
>> +               ret = get_spi_clk_cfg(xfer->speed_hz, mas, &idx, 
>> &div);
>> +               if (ret) {
>> +                       dev_err(mas->dev, "Err setting clks:%d\n", 
>> ret);
>> +                       return;
>> +               }
>> +               /*
>> +                * SPI core clock gets configured with the requested 
>> frequency
>> +                * or the frequency closer to the requested frequency.
>> +                * For that reason requested frequency is stored in 
>> the
>> +                * cur_speed_hz and referred in the consicutive 
>> transfer instead
> 
> s/consicutive/consecutive/
will correct it.
> 
>> +                * of calling clk_get_rate() API.
>> +                */
>> +               mas->cur_speed_hz = xfer->speed_hz;
>> +               clk_sel = idx & CLK_SEL_MSK;
>> +               m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN;
>> +               writel(clk_sel, se->base + SE_GENI_CLK_SEL);
>> +               writel(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);
>> +       }
>> +
>> +       mas->tx_rem_bytes = 0;
>> +       mas->rx_rem_bytes = 0;
>> +       if (xfer->tx_buf && xfer->rx_buf)
>> +               m_cmd = SPI_FULL_DUPLEX;
>> +       else if (xfer->tx_buf)
>> +               m_cmd = SPI_TX_ONLY;
>> +       else if (xfer->rx_buf)
>> +               m_cmd = SPI_RX_ONLY;
>> +
>> +       spi_tx_cfg &= ~CS_TOGGLE;
>> +       if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) {
>> +               trans_len =
>> +                       (xfer->len * BITS_PER_BYTE /
>> +                                       mas->cur_bits_per_word) & 
>> TRANS_LEN_MSK;
>> +       } else {
>> +               unsigned int bytes_per_word =
>> +                       mas->cur_bits_per_word / BITS_PER_BYTE + 1;
>> +
>> +               trans_len = (xfer->len / bytes_per_word) & 
>> TRANS_LEN_MSK;
>> +       }
> 
> Rename 'trans_len' to 'len' and shorten some lines by taking out the
> mask to get:
> 
>        if (!(mas->cur_bits_per_word % MIN_WORD_LEN))
>                len = xfer->len * BITS_PER_BYTE / mas->cur_bits_per_word
>        else
>                len = xfer->len / (mas->cur_bits_per_word / 
> BITS_PER_BYTE + 1);
>        len &= TRANS_LEN_MSK;
> 
OK
>> +
>> +       /*
>> +        * If CS change flag is set, then toggle the CS line in 
>> between
>> +        * transfers and keep CS asserted after the last transfer.
>> +        * Else if keep CS flag asserted in between transfers and 
>> de-assert
>> +        * CS after the last message.
>> +        */
>> +       if (xfer->cs_change) {
>> +               if (list_is_last(&xfer->transfer_list,
>> +                               &spi->cur_msg->transfers))
>> +                       m_param = FRAGMENTATION;
>> +       } else {
>> +               if (!list_is_last(&xfer->transfer_list,
>> +                               &spi->cur_msg->transfers))
>> +                       m_param = FRAGMENTATION;
>> +       }
>> +
>> +       mas->cur_xfer = xfer;
>> +       if (m_cmd & SPI_TX_ONLY) {
>> +               mas->tx_rem_bytes = xfer->len;
>> +               writel(trans_len, se->base + SE_SPI_TX_TRANS_LEN);
>> +       }
>> +
>> +       if (m_cmd & SPI_RX_ONLY) {
>> +               writel(trans_len, se->base + SE_SPI_RX_TRANS_LEN);
>> +               mas->rx_rem_bytes = xfer->len;
>> +       }
>> +       writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>> +       geni_se_setup_m_cmd(se, m_cmd, m_param);
>> +
>> +       /*
>> +        * TX_WATERMARK_REG should be set after SPI configuration and
>> +        * setting up GENI SE engine, as driver starts data transfer
>> +        * for the watermark interrupt.
>> +        */
>> +       if (m_cmd & SPI_TX_ONLY)
>> +               writel(mas->tx_wm, se->base + 
>> SE_GENI_TX_WATERMARK_REG);
>> +}
>> +
>> +static void handle_fifo_timeout(struct spi_master *spi,
>> +                               struct spi_message *msg)
>> +{
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +       unsigned long timeout, flags;
>> +       struct geni_se *se = &mas->se;
>> +
>> +       spin_lock_irqsave(&mas->lock, flags);
>> +       reinit_completion(&mas->xfer_done);
>> +       geni_se_cancel_m_cmd(se);
>> +       writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
>> +       spin_unlock_irqrestore(&mas->lock, flags);
>> +       timeout = wait_for_completion_timeout(&mas->xfer_done, HZ);
> 
> Can you rename this time_left or completed? Then the if condition reads
> properly as "if not time left" or "if not completed". And then invert
> that logic so things aren't so indented?
> 
> 	time_left = wait_for_completion_timeout(..)
> 	if (time_left)
> 		return;
> 
> 	spin_lock_irqsave(&mas->lock, flags);
> 	reinit_completion(..)
> 	...
> 
Ok
>> +       if (!timeout) {
>> +               spin_lock_irqsave(&mas->lock, flags);
>> +               reinit_completion(&mas->xfer_done);
>> +               geni_se_abort_m_cmd(se);
>> +               spin_unlock_irqrestore(&mas->lock, flags);
>> +               timeout = wait_for_completion_timeout(&mas->xfer_done,
>> +                                                               HZ);
>> +               if (!timeout)
>> +                       dev_err(mas->dev,
>> +                               "Failed to cancel/abort m_cmd\n");
>> +       }
>> +}
>> +
>> +static int spi_geni_transfer_one(struct spi_master *spi,
>> +                               struct spi_device *slv,
>> +                               struct spi_transfer *xfer)
>> +{
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +
>> +       setup_fifo_xfer(xfer, mas, slv->mode, spi);
>> +       return 1;
>> +}
>> +
>> +static unsigned int geni_byte_per_fifo_word(struct spi_geni_master 
>> *mas)
>> +{
>> +       /*
>> +        * Calculate how many bytes we'll put in each FIFO word.  If 
>> the
>> +        * transfer words don't pack cleanly into a FIFO word we'll 
>> just put
>> +        * one transfer word in each FIFO word.  If they do pack we'll 
>> pack 'em.
>> +        */
>> +       if (mas->fifo_width_bits % mas->cur_bits_per_word)
>> +               return 
>> roundup_pow_of_two(DIV_ROUND_UP(mas->cur_bits_per_word,
>> +                                                      
>> BITS_PER_BYTE));
>> +       else
>> +               return mas->fifo_width_bits / BITS_PER_BYTE;
> 
> Just do a return. No else please.
> 
Ok
>> +}
>> +
>> +static irqreturn_t geni_spi_handle_tx(struct spi_geni_master *mas)
>> +{
>> +       struct geni_se *se = &mas->se;
>> +       unsigned int max_bytes;
>> +       const u8 *tx_buf;
>> +       unsigned int bytes_per_fifo_word = 
>> geni_byte_per_fifo_word(mas);
>> +       unsigned int i = 0;
>> +
>> +       if (!mas->cur_xfer)
>> +               return IRQ_NONE;
>> +
>> +       max_bytes = (mas->tx_fifo_depth - mas->tx_wm) * 
>> bytes_per_fifo_word;
>> +       if (mas->tx_rem_bytes < max_bytes)
>> +               max_bytes = mas->tx_rem_bytes;
>> +
>> +       tx_buf = mas->cur_xfer->tx_buf + mas->cur_xfer->len - 
>> mas->tx_rem_bytes;
>> +       while (i < max_bytes) {
>> +               unsigned int j;
>> +               unsigned int bytes_to_write;
>> +               u32 fifo_word = 0;
>> +               u8 *fifo_byte = (u8 *)&fifo_word;
>> +
>> +               bytes_to_write = min(bytes_per_fifo_word, max_bytes - 
>> i);
>> +               for (j = 0; j < bytes_to_write; j++)
>> +                       fifo_byte[j] = tx_buf[i++];
>> +               iowrite32_rep(se->base + SE_GENI_TX_FIFOn, &fifo_word, 
>> 1);
>> +       }
>> +       mas->tx_rem_bytes -= max_bytes;
>> +       if (!mas->tx_rem_bytes)
>> +               writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t geni_spi_handle_rx(struct spi_geni_master *mas)
>> +{
>> +       struct geni_se *se = &mas->se;
>> +       u32 rx_fifo_status;
>> +       unsigned int rx_bytes;
>> +       u8 *rx_buf;
>> +       unsigned int bytes_per_fifo_word = 
>> geni_byte_per_fifo_word(mas);
>> +       unsigned int i = 0;
>> +
>> +       if (!mas->cur_xfer)
>> +               return IRQ_NONE;
>> +
>> +       rx_fifo_status = readl(se->base + SE_GENI_RX_FIFO_STATUS);
> 
> Rename to 'status'? rx_fifo is implicit in the register macro.
Declared as "rx_fifo_status" for code readability.

> 
>> +       rx_bytes = (rx_fifo_status & RX_FIFO_WC_MSK) * 
>> bytes_per_fifo_word;
>> +       if (rx_fifo_status & RX_LAST) {
>> +               unsigned int rx_last_byte_valid =
>> +                       (rx_fifo_status & RX_LAST_BYTE_VALID_MSK)
>> +                               >> RX_LAST_BYTE_VALID_SHFT;
> 
> Hoist this variable into function scope? So then the line is:
> 
> 		last_valid = status & RX_LAST_BYTE_VALID_MSK;
> 		last_valid >>= RX_LAST_BYTE_VALID_SHFT;
> 
Ok
>> +               if (rx_last_byte_valid && (rx_last_byte_valid < 4))
> 
> Drop useless parenthesis please.
> 
Ok
>> +                       rx_bytes -= bytes_per_fifo_word - 
>> rx_last_byte_valid;
>> +       }
>> +       if (mas->rx_rem_bytes < rx_bytes)
>> +               rx_bytes = mas->rx_rem_bytes;
>> +
>> +       rx_buf = mas->cur_xfer->rx_buf + mas->cur_xfer->len - 
>> mas->rx_rem_bytes;
>> +       while (i < rx_bytes) {
>> +               u32 fifo_word = 0;
>> +               u8 *fifo_byte = (u8 *)&fifo_word;
>> +               unsigned int bytes_to_read;
>> +               unsigned int j;
>> +
>> +               bytes_to_read = min(bytes_per_fifo_word, rx_bytes - 
>> i);
>> +               ioread32_rep(se->base + SE_GENI_RX_FIFOn, &fifo_word, 
>> 1);
>> +               for (j = 0; j < bytes_to_read; j++)
>> +                       rx_buf[i++] = fifo_byte[j];
>> +       }
>> +       mas->rx_rem_bytes -= rx_bytes;
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t geni_spi_isr(int irq, void *data)
>> +{
>> +       struct spi_master *spi = data;
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +       struct geni_se *se = &mas->se;
>> +       u32 m_irq;
>> +       unsigned long flags;
>> +       irqreturn_t ret = IRQ_HANDLED;
>> +
>> +       if (pm_runtime_status_suspended(mas->dev))
>> +               return IRQ_NONE;
>> +
>> +       spin_lock_irqsave(&mas->lock, flags);
>> +       m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);
>> +       if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & 
>> M_RX_FIFO_LAST_EN))
>> +               ret = geni_spi_handle_rx(mas);
>> +
>> +       if (m_irq & M_TX_FIFO_WATERMARK_EN)
>> +               ret = geni_spi_handle_tx(mas);
> 
> Is it possible for all three conditions above to happen in one
> interrupt? I ask because 'ret' is overwritten and so what may have been
> IRQ_HANDLED may become IRQ_NONE which will lead to confusion in the irq
> layer. Maybe the handle tx/rx functions can return a bool, that gets
> orred together each time so that we know if something handled an
> interrupt?
> 
Will check it again by running fullduplex transfer and update.
>> +
>> +       if (m_irq & M_CMD_DONE_EN) {
>> +               spi_finalize_current_transfer(spi);
>> +               /*
>> +                * If this happens, then a CMD_DONE came before all 
>> the Tx
>> +                * buffer bytes were sent out. This is unusual, log 
>> this
>> +                * condition and disable the WM interrupt to prevent 
>> the
>> +                * system from stalling due an interrupt storm.
>> +                * If this happens when all Rx bytes haven't been 
>> received, log
>> +                * the condition.
>> +                * The only known time this can happen is if 
>> bits_per_word != 8
>> +                * and some registers that expect xfer lengths in num 
>> spi_words
>> +                * weren't written correctly.
>> +                */
>> +               if (mas->tx_rem_bytes) {
>> +                       writel(0, se->base + 
>> SE_GENI_TX_WATERMARK_REG);
>> +                       dev_err(mas->dev, "Premature Done.tx_rem%d 
>> bpw%d\n",
> 
> Why isn't there a space after "Done."? And why is 'done' capitalized?
> Should 'tx_rem' be 'tx_rem='?
Yes, will update.
> 
>> +                               mas->tx_rem_bytes, 
>> mas->cur_bits_per_word);
>> +               }
>> +               if (mas->rx_rem_bytes)
>> +                       dev_err(mas->dev, "Premature Done.rx_rem%d 
>> bpw%d\n",
>> +                               mas->rx_rem_bytes, 
>> mas->cur_bits_per_word);
>> +       }
>> +
>> +       if ((m_irq & M_CMD_CANCEL_EN) || (m_irq & M_CMD_ABORT_EN))
>> +               complete(&mas->xfer_done);
>> +
>> +       writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
>> +       spin_unlock_irqrestore(&mas->lock, flags);
>> +       return ret;
>> +}
>> +
>> +static int spi_geni_probe(struct platform_device *pdev)
>> +{
>> +       int ret;
>> +       struct spi_master *spi;
>> +       struct spi_geni_master *mas;
>> +       struct resource *res;
>> +       struct geni_se *se;
>> +
>> +       spi = spi_alloc_master(&pdev->dev, sizeof(struct 
>> spi_geni_master));
> 
> sizeof(*mas) for code clarity?
> 
Ok
>> +       if (!spi)
>> +               return -ENOMEM;
>> +
>> +       platform_set_drvdata(pdev, spi);
>> +       mas = spi_master_get_devdata(spi);
>> +       mas->dev = &pdev->dev;
>> +       mas->se.dev = &pdev->dev;
>> +       mas->se.wrapper = dev_get_drvdata(pdev->dev.parent);
>> +       se = &mas->se;
>> +
>> +       spi->bus_num = -1;
>> +       spi->dev.of_node = pdev->dev.of_node;
>> +       mas->se.clk = devm_clk_get(&pdev->dev, "se");
>> +       if (IS_ERR(mas->se.clk)) {
>> +               ret = PTR_ERR(mas->se.clk);
>> +               dev_err(&pdev->dev, "Err getting SE Core clk %d\n", 
>> ret);
>> +               goto spi_geni_probe_err;
>> +       }
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       se->base = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(se->base)) {
>> +               ret = -ENOMEM;
> 
> 		ret = PTR_ERR(se->base);
> 
> so we don't lose the error value.
> 
Ok
>> +               goto spi_geni_probe_err;
>> +       }
>> +
>> +       ret = platform_get_irq(pdev, 0);
>> +       if (ret < 0) {
>> +               dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
>> +               goto spi_geni_probe_err;
>> +       }
>> +       ret = devm_request_irq(&pdev->dev, ret, geni_spi_isr,
>> +                               IRQF_TRIGGER_HIGH, "spi_geni", spi);
>> +       if (ret)
>> +               goto spi_geni_probe_err;
> 
> Can you request this irq as late as possible in the probe function? I
> worry there may be some pending irq line set and then this will cause 
> an
> interrupt storm with IRQ_NONE returned because the device is runtime
> suspended.
> 
Ok; will call it after spi_register_master()
>> +
>> +       spi->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_CS_HIGH;
>> +       spi->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
>> +       spi->num_chipselect = 4;
>> +       spi->max_speed_hz = 50000000;
>> +       spi->prepare_message = spi_geni_prepare_message;
>> +       spi->transfer_one = spi_geni_transfer_one;
>> +       spi->auto_runtime_pm = true;
>> +       spi->handle_err = handle_fifo_timeout;
>> +
>> +       init_completion(&mas->xfer_done);
>> +       spin_lock_init(&mas->lock);
>> +       pm_runtime_enable(&pdev->dev);
>> +
>> +       ret = spi_geni_init(mas);
>> +       if (ret)
>> +               goto spi_geni_probe_runtime_disable;
>> +
>> +       ret = spi_register_master(spi);
>> +       if (ret)
>> +               goto spi_geni_probe_runtime_disable;
>> +
>> +       return 0;
>> +spi_geni_probe_runtime_disable:
>> +       pm_runtime_disable(&pdev->dev);
>> +spi_geni_probe_err:
>> +       spi_master_put(spi);
>> +       return ret;
>> +}
>> +
>> +static int spi_geni_remove(struct platform_device *pdev)
>> +{
>> +       struct spi_master *spi = platform_get_drvdata(pdev);
>> +
>> +       /* Unregister _before_ disabling pm_runtime() so we stop 
>> transfers */
>> +       spi_unregister_master(spi);
>> +
>> +       pm_runtime_disable(&pdev->dev);
>> +       return 0;
>> +}
>> +
>> +static int __maybe_unused spi_geni_runtime_suspend(struct device 
>> *dev)
>> +{
>> +       struct spi_master *spi = dev_get_drvdata(dev);
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +
>> +       return geni_se_resources_off(&mas->se);
>> +}
>> +
>> +static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
>> +{
>> +       struct spi_master *spi = dev_get_drvdata(dev);
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +
>> +       return geni_se_resources_on(&mas->se);
>> +}
>> +
>> +static int __maybe_unused spi_geni_suspend(struct device *dev)
>> +{
>> +       if (!pm_runtime_status_suspended(dev))
>> +               return -EBUSY;
>> +       return 0;
>> +}
>> +
>> +static const struct dev_pm_ops spi_geni_pm_ops = {
>> +       SET_RUNTIME_PM_OPS(spi_geni_runtime_suspend,
>> +                                       spi_geni_runtime_resume, NULL)
>> +       SET_SYSTEM_SLEEP_PM_OPS(spi_geni_suspend, NULL)
>> +};
>> +
>> +static const struct of_device_id spi_geni_dt_match[] = {
>> +       { .compatible = "qcom,geni-spi" },
>> +       {}
>> +};
> 
> Please add a MODULE_DEVICE_TABLE(of, spi_geni_dt_match) here.
> 
Ok
>> +
>> +static struct platform_driver spi_geni_driver = {
>> +       .probe  = spi_geni_probe,
>> +       .remove = spi_geni_remove,
>> +       .driver = {
>> +               .name = "geni_spi",
>> +               .pm = &spi_geni_pm_ops,
>> +               .of_match_table = spi_geni_dt_match,
>> +       },
>> +};
>> +module_platform_driver(spi_geni_driver);
>> +
>> +MODULE_DESCRIPTION("SPI driver for GENI based QUP cores");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/spi/spi-geni-qcom.h 
>> b/include/linux/spi/spi-geni-qcom.h
>> new file mode 100644
>> index 0000000..dc95764
>> --- /dev/null
>> +++ b/include/linux/spi/spi-geni-qcom.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights 
>> reserved.
>> + */
>> +
>> +#ifndef __SPI_GENI_QCOM_HEADER___
>> +#define __SPI_GENI_QCOM_HEADER___
>> +
>> +struct spi_geni_qcom_ctrl_data {
>> +       u32 spi_cs_clk_delay;
>> +       u32 spi_inter_words_delay;
> 
> It was decided we still needed this? I don't see it in v3 so please
> remove this file unless it's needed.
Its not required; I will remove it.

Along with these changes, i will submit the fixes of loopback issue and 
suspend/resume issue.

--Dilip

Re: [PATCH V4 3/4] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Doug Anderson]

Hi,

On Tue, Sep 25, 2018 at 12:36 PM <dkota@codeaurora.org> wrote:
> >> +       unsigned int cur_speed_hz;
> >
> > unsigned long for Hz? The clk framework uses that type.
>
> cur_speed_hz stores the speed value requested as part of transfer (not
> the resultant or rounded off frequency got from clk framework. It is u32
> type, i will change cur_speed_hz to u32 type instead of unsigned long.
> Code snippet:
>          mas->cur_speed_hz = xfer->speed_hz;

Change it to "unsigned long" anyway to match the clock framework.  In
theory maybe the "xfer" structure will be updated eventually.


-Doug

Re: [PATCH V4 3/4] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[dkota]

On 2018-09-26 01:24, Doug Anderson wrote:
> Hi,
> 
> On Tue, Sep 25, 2018 at 12:36 PM <dkota@codeaurora.org> wrote:
>> >> +       unsigned int cur_speed_hz;
>> >
>> > unsigned long for Hz? The clk framework uses that type.
>> 
>> cur_speed_hz stores the speed value requested as part of transfer (not
>> the resultant or rounded off frequency got from clk framework. It is 
>> u32
>> type, i will change cur_speed_hz to u32 type instead of unsigned long.
>> Code snippet:
>>          mas->cur_speed_hz = xfer->speed_hz;
> 
> Change it to "unsigned long" anyway to match the clock framework.  In
> theory maybe the "xfer" structure will be updated eventually.
Ok. WIll update it
> 


>> Is it possible for all three conditions above to happen in one
>> interrupt? I ask because 'ret' is overwritten and so what may have 
>> been
>> IRQ_HANDLED may become IRQ_NONE which will lead to confusion in the 
>> irq
>> layer. Maybe the handle tx/rx functions can return a bool, that gets
>> orred together each time so that we know if something handled an
>> interrupt?
>> 
> Will check it again by running fullduplex transfer and update.
Added the changes in V5 to ensure return value doesn't get overwritten.

--Dilip

[PATCH V4 4/4] spi: spi-geni-qcom: Plugin API to assert and de-assert Chipselect

[Dilip Kota]

Plugin set cs API to the SPI framework so that
framework can do chipselect assert de-assert
during  SPI transfers.

Signed-off-by: Dilip Kota <dkota@codeaurora.org>
---
 drivers/spi/spi-geni-qcom.c | 65 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 46 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 949b853..5cfb4f2 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -66,6 +66,13 @@
 
 static irqreturn_t geni_spi_isr(int irq, void *data);
 
+/* SPI M_COMMAND OPCODE */
+enum spi_mcmd_code {
+	CMD_NONE,
+	CMD_XFER,
+	CMD_CS,
+};
+
 struct spi_geni_master {
 	struct geni_se se;
 	struct device *dev;
@@ -81,8 +88,12 @@ struct spi_geni_master {
 	struct completion xfer_done;
 	unsigned int oversampling;
 	spinlock_t lock;
+	unsigned int cur_mcmd;
 };
 
+static void handle_fifo_timeout(struct spi_master *spi,
+				struct spi_message *msg);
+
 static int get_spi_clk_cfg(unsigned int speed_hz,
 			struct spi_geni_master *mas,
 			unsigned int *clk_idx,
@@ -113,6 +124,31 @@ static int get_spi_clk_cfg(unsigned int speed_hz,
 	return ret;
 }
 
+static void spi_geni_set_cs(struct spi_device *slv, bool set_flag)
+{
+	struct spi_geni_master *mas = spi_master_get_devdata(slv->master);
+	struct spi_master *spi = dev_get_drvdata(mas->dev);
+	struct geni_se *se = &mas->se;
+	unsigned long timeout;
+
+	reinit_completion(&mas->xfer_done);
+	pm_runtime_get_sync(mas->dev);
+	if (!(slv->mode & SPI_CS_HIGH))
+		set_flag = !set_flag;
+
+	mas->cur_mcmd = CMD_CS;
+	if (set_flag)
+		geni_se_setup_m_cmd(se, SPI_CS_ASSERT, 0);
+	else
+		geni_se_setup_m_cmd(se, SPI_CS_DEASSERT, 0);
+
+	timeout = wait_for_completion_timeout(&mas->xfer_done, HZ);
+	if (!timeout)
+		handle_fifo_timeout(spi, NULL);
+
+	pm_runtime_put(mas->dev);
+}
+
 static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
 					unsigned int bits_per_word)
 {
@@ -249,7 +285,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 				struct spi_geni_master *mas,
 				u16 mode, struct spi_master *spi)
 {
-	u32 m_cmd = 0, m_param = 0;
+	u32 m_cmd = 0;
 	u32 spi_tx_cfg, trans_len;
 	struct geni_se *se = &mas->se;
 
@@ -305,21 +341,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 		trans_len = (xfer->len / bytes_per_word) & TRANS_LEN_MSK;
 	}
 
-	/*
-	 * If CS change flag is set, then toggle the CS line in between
-	 * transfers and keep CS asserted after the last transfer.
-	 * Else if keep CS flag asserted in between transfers and de-assert
-	 * CS after the last message.
-	 */
-	if (xfer->cs_change) {
-		if (list_is_last(&xfer->transfer_list,
-				&spi->cur_msg->transfers))
-			m_param = FRAGMENTATION;
-	} else {
-		if (!list_is_last(&xfer->transfer_list,
-				&spi->cur_msg->transfers))
-			m_param = FRAGMENTATION;
-	}
+
 
 	mas->cur_xfer = xfer;
 	if (m_cmd & SPI_TX_ONLY) {
@@ -332,8 +354,8 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 		mas->rx_rem_bytes = xfer->len;
 	}
 	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
-	geni_se_setup_m_cmd(se, m_cmd, m_param);
-
+	mas->cur_mcmd = CMD_XFER;
+	geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
 	/*
 	 * TX_WATERMARK_REG should be set after SPI configuration and
 	 * setting up GENI SE engine, as driver starts data transfer
@@ -489,7 +511,11 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 		ret = geni_spi_handle_tx(mas);
 
 	if (m_irq & M_CMD_DONE_EN) {
-		spi_finalize_current_transfer(spi);
+		if (mas->cur_mcmd == CMD_XFER)
+			spi_finalize_current_transfer(spi);
+		else if (mas->cur_mcmd == CMD_CS)
+			complete(&mas->xfer_done);
+		mas->cur_mcmd = CMD_NONE;
 		/*
 		 * If this happens, then a CMD_DONE came before all the Tx
 		 * buffer bytes were sent out. This is unusual, log this
@@ -572,6 +598,7 @@ static int spi_geni_probe(struct platform_device *pdev)
 	spi->transfer_one = spi_geni_transfer_one;
 	spi->auto_runtime_pm = true;
 	spi->handle_err = handle_fifo_timeout;
+	spi->set_cs = spi_geni_set_cs;
 
 	init_completion(&mas->xfer_done);
 	spin_lock_init(&mas->lock);
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V4 4/4] spi: spi-geni-qcom: Plugin API to assert and de-assert Chipselect

[Doug Anderson]

Hi,

On Tue, Sep 18, 2018 at 11:09 AM Dilip Kota <dkota@codeaurora.org> wrote:
>
> Plugin set cs API to the SPI framework so that
> framework can do chipselect assert de-assert
> during  SPI transfers.
>
> Signed-off-by: Dilip Kota <dkota@codeaurora.org>
> ---
>  drivers/spi/spi-geni-qcom.c | 65 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 46 insertions(+), 19 deletions(-)

As I mentioned in out-of-band I don't see a huge reason why this can't
be squashed into patch #3.  Logically patch #3 has a bug where it
can't handle setting the chip select without a non-zero transfer and
that bug isn't fixed until patch #4.  ...but if folks really feel like
it should be a separate patch then I won't object too loudly since the
end result is the same.

...so feel free to add:

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>


-Doug