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

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

[Alok Chauhan]

This patch series adds the driver for GENI based Qualcomm Universal
Peripheral (QUP) Serial Peripheral Interface (SPI) and SPI device tree
bindings.

An overview of the GENI SE SPI controller device tree components are in
patch 2 and 1. Patch 3 adds the SPI driver for GENI QUP HW.

changes from v4:
- Patch 1/3 and 2/3 are unchanged
- squashed patch 4/4 into 3/4 as suggested
- Patch 3/3 changes are follows:
        * Add SPI M_COMMAND OPCODE to handle different geni command
	  handling
        * Remove forward declaration of ISR
        * Remove unused variable rx_fifo_depth in spi_master structure
        * Declare cur_speed_hz as unsigned long to match clock framework
        * Declare cur_xfer as const pointer
        * Newline in error print
        * Correct consecutive spelling
        * Rename trans_len to len and restructure the lines in
	  setup_fifo_xfer()
        * Rename timeout to time_left and restructure the
	  handle_fifo_timeout()
        * Add check for '0' bytes transfer as part of
	  spi_geni_transfer_one()
        * Correct if-else check in geni_byte_per_fifo_word()
        * Remove NULL current transfer check in
	  geni_spi_handle_tx()/geni_spi_handle_rx() and make these
	  functions as void.
        * Hoist rx_last_byte_valid variable into function scope
        * Remove RT check in ISR and add cur_mcmd handling
        * Correct the error prints in ISR
        * In spi_alloc_master() pass 2nd arg as sizeof(*mas) for code
	  clarity
        * Use ret = PTR_ERR(se->base) for devm_ioremap_resource() err
	  return
        * Move request irq code to probe() and used request_irq() in place
	  of devm_request_irq()
        * Rewrite suspend/resume function
        * Add MODULE_DEVICE_TABLE(of, spi_geni_dt_match);
        * Remove include/linux/spi/spi-geni-qcom.h file
	

Dilip Kota (2):
  dt-bindings: soc: qcom: Remove SPI controller maximum frequency
    binding
  dt-bindings: soc: qcom: GENI SE SPI controller device tree binding

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                        | 703 +++++++++++++++++++++
 5 files changed, 757 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt
 create mode 100644 drivers/spi/spi-geni-qcom.c

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Alok Chauhan]

From: Dilip Kota <dkota@codeaurora.org>

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: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Alok Chauhan <alokc@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 ff92e5a..b9d0c21 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>;
 		};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Alok Chauhan]

From: Dilip Kota <dkota@codeaurora.org>

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: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Alok Chauhan <alokc@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 b9d0c21..dab7ca9 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>;
+	};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Alok Chauhan]

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>
Signed-off-by: Alok Chauhan <alokc@codeaurora.org>
---
 drivers/spi/Kconfig         |  12 +
 drivers/spi/Makefile        |   1 +
 drivers/spi/spi-geni-qcom.c | 703 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 716 insertions(+)
 create mode 100644 drivers/spi/spi-geni-qcom.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 671d078..51edc76 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 a90d559..b057c9c 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..6432ecc
--- /dev/null
+++ b/drivers/spi/spi-geni-qcom.c
@@ -0,0 +1,703 @@
+// 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)
+
+/* SPI M_COMMAND OPCODE */
+enum spi_mcmd_code {
+	CMD_NONE,
+	CMD_XFER,
+	CMD_CS,
+	CMD_CANCEL,
+};
+
+
+struct spi_geni_master {
+	struct geni_se se;
+	struct device *dev;
+	u32 tx_fifo_depth;
+	u32 fifo_width_bits;
+	u32 tx_wm;
+	unsigned long cur_speed_hz;
+	unsigned int cur_bits_per_word;
+	unsigned int tx_rem_bytes;
+	unsigned int rx_rem_bytes;
+	const struct spi_transfer *cur_xfer;
+	struct completion xfer_done;
+	unsigned int oversampling;
+	spinlock_t lock;
+	unsigned int cur_mcmd;
+	int irq;
+};
+
+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,
+			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_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)
+{
+	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 %ld\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\n", 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);
+
+	/* 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;
+	u32 spi_tx_cfg, 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 consecutive 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))
+		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;
+
+	mas->cur_xfer = xfer;
+	if (m_cmd & SPI_TX_ONLY) {
+		mas->tx_rem_bytes = xfer->len;
+		writel(len, se->base + SE_SPI_TX_TRANS_LEN);
+	}
+
+	if (m_cmd & SPI_RX_ONLY) {
+		writel(len, se->base + SE_SPI_RX_TRANS_LEN);
+		mas->rx_rem_bytes = xfer->len;
+	}
+	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
+	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
+	 * 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 time_left, flags;
+	struct geni_se *se = &mas->se;
+
+	spin_lock_irqsave(&mas->lock, flags);
+	reinit_completion(&mas->xfer_done);
+	mas->cur_mcmd = CMD_CANCEL;
+	geni_se_cancel_m_cmd(se);
+	writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
+	spin_unlock_irqrestore(&mas->lock, flags);
+	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);
+	if (time_left)
+		return;
+
+	spin_lock_irqsave(&mas->lock, flags);
+	reinit_completion(&mas->xfer_done);
+	geni_se_abort_m_cmd(se);
+	spin_unlock_irqrestore(&mas->lock, flags);
+	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);
+	if (!time_left)
+		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);
+
+	/* Terminate and return success for 0 byte length transfer */
+	if (!xfer->len)
+		return 0;
+
+	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));
+
+	return mas->fifo_width_bits / BITS_PER_BYTE;
+}
+
+static void 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;
+
+	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);
+}
+
+static void geni_spi_handle_rx(struct spi_geni_master *mas)
+{
+	struct geni_se *se = &mas->se;
+	u32 rx_fifo_status;
+	unsigned int rx_bytes;
+	unsigned int rx_last_byte_valid;
+	u8 *rx_buf;
+	unsigned int bytes_per_fifo_word = geni_byte_per_fifo_word(mas);
+	unsigned int i = 0;
+
+	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) {
+		rx_last_byte_valid = rx_fifo_status & RX_LAST_BYTE_VALID_MSK;
+		rx_last_byte_valid >>= 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;
+}
+
+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 (mas->cur_mcmd == CMD_NONE)
+		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))
+		geni_spi_handle_rx(mas);
+
+	if (m_irq & M_TX_FIFO_WATERMARK_EN)
+		geni_spi_handle_tx(mas);
+
+	if (m_irq & M_CMD_DONE_EN) {
+		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
+		 * 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)) {
+		mas->cur_mcmd = CMD_NONE;
+		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(*mas));
+	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 = PTR_ERR(se->base);
+		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;
+	spi->set_cs = spi_geni_set_cs;
+
+	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;
+
+	mas->irq = platform_get_irq(pdev, 0);
+	if (mas->irq < 0) {
+		ret = mas->irq;
+		dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
+		goto spi_geni_probe_runtime_disable;
+	}
+
+	ret = request_irq(mas->irq, geni_spi_isr,
+			IRQF_TRIGGER_HIGH, "spi_geni", spi);
+	if (ret)
+		goto spi_geni_probe_runtime_disable;
+
+	ret = spi_register_master(spi);
+	if (ret)
+		goto spi_geni_probe_free_irq;
+
+	return 0;
+spi_geni_probe_free_irq:
+	free_irq(mas->irq, spi);
+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);
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+
+	/* Unregister _before_ disabling pm_runtime() so we stop transfers */
+	spi_unregister_master(spi);
+
+	free_irq(mas->irq, 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)
+{
+	struct spi_master *spi = dev_get_drvdata(dev);
+	int ret;
+
+	ret = spi_master_suspend(spi);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		spi_master_resume(spi);
+
+	return ret;
+}
+
+static int __maybe_unused spi_geni_resume(struct device *dev)
+{
+	struct spi_master *spi = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		return ret;
+
+	ret = spi_master_resume(spi);
+	if (ret)
+		pm_runtime_force_suspend(dev);
+
+	return ret;
+}
+
+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, spi_geni_resume)
+};
+
+static const struct of_device_id spi_geni_dt_match[] = {
+	{ .compatible = "qcom,geni-spi" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, spi_geni_dt_match);
+
+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");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Doug Anderson]

Hi,

On Wed, Oct 3, 2018 at 6:45 AM Alok Chauhan <alokc@codeaurora.org> wrote:
> +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 (mas->cur_mcmd == CMD_NONE)
> +               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))
> +               geni_spi_handle_rx(mas);
> +
> +       if (m_irq & M_TX_FIFO_WATERMARK_EN)
> +               geni_spi_handle_tx(mas);
> +
> +       if (m_irq & M_CMD_DONE_EN) {
> +               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
> +                * 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)) {
> +               mas->cur_mcmd = CMD_NONE;
> +               complete(&mas->xfer_done);
> +       }
> +
> +       writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
> +       spin_unlock_irqrestore(&mas->lock, flags);
> +       return ret;

nit: you no longer need the "ret" variable.  Just return IRQ_HANDLED
here and remove the "ret" local variable from this function.  It'd be
nice if you put a blank line before the return too.


I'm not convinced it's worth spinning the patch to fix that one nit,
but if you spin it for some other reason please fix it.  I believe
this fixes all outstanding feedback that I'm aware of.  Thus:

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

-Doug

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

[Stephen Boyd]

Quoting Alok Chauhan (2018-10-03 06:44:25)
>

I just have a handful of nitpicks which can be fixed later in
follow-ups. Otherwise:

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

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> new file mode 100644
> index 0000000..6432ecc
> --- /dev/null
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -0,0 +1,703 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> +
[...]
> +#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)
> +
> +/* SPI M_COMMAND OPCODE */
> +enum spi_mcmd_code {

Nitpick: rename spi_m_cmd_opcode and drop the comment?

> +       CMD_NONE,
> +       CMD_XFER,
> +       CMD_CS,
> +       CMD_CANCEL,
> +};
> +
> +

Nitpick: Drop double newline.

> +struct spi_geni_master {
> +       struct geni_se se;
> +       struct device *dev;
> +       u32 tx_fifo_depth;
> +       u32 fifo_width_bits;
> +       u32 tx_wm;
> +       unsigned long cur_speed_hz;
> +       unsigned int cur_bits_per_word;
> +       unsigned int tx_rem_bytes;
> +       unsigned int rx_rem_bytes;
> +       const struct spi_transfer *cur_xfer;
> +       struct completion xfer_done;
> +       unsigned int oversampling;
> +       spinlock_t lock;
> +       unsigned int cur_mcmd;

Niptick: Use the enum?

> +       int irq;
> +};
> +
> +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,
> +                       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_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);

Nitpick: s/timeout/time_left/

> +       if (!timeout)
> +               handle_fifo_timeout(spi, NULL);
> +
> +       pm_runtime_put(mas->dev);
> +}
> +
[...]
> +
> +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 (mas->cur_mcmd == CMD_NONE)
> +               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))
> +               geni_spi_handle_rx(mas);
> +
> +       if (m_irq & M_TX_FIFO_WATERMARK_EN)
> +               geni_spi_handle_tx(mas);
> +
> +       if (m_irq & M_CMD_DONE_EN) {
> +               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
> +                * 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)) {
> +               mas->cur_mcmd = CMD_NONE;
> +               complete(&mas->xfer_done);
> +       }
> +
> +       writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
> +       spin_unlock_irqrestore(&mas->lock, flags);
> +       return ret;

Nitpick: Now this always returns IRQ_HANDLED, and we assign ret just to
do that. Perhaps only return IRQ_HANDLED if one of the above if
conditions is taken?

> +}
> +
> +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(*mas));
> +       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 = PTR_ERR(se->base);
> +               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;
> +       spi->set_cs = spi_geni_set_cs;
> +
> +       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;
> +
> +       mas->irq = platform_get_irq(pdev, 0);
> +       if (mas->irq < 0) {
> +               ret = mas->irq;
> +               dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
> +               goto spi_geni_probe_runtime_disable;
> +       }

Nitpick: If you got the irq earlier before allocating anything then nothing has
to be put on failure path.

> +
> +       ret = request_irq(mas->irq, geni_spi_isr,
> +                       IRQF_TRIGGER_HIGH, "spi_geni", spi);
> +       if (ret)
> +               goto spi_geni_probe_runtime_disable;
> +
> +       ret = spi_register_master(spi);
> +       if (ret)
> +               goto spi_geni_probe_free_irq;
> +
> +       return 0;
> +spi_geni_probe_free_irq:
> +       free_irq(mas->irq, spi);
> +spi_geni_probe_runtime_disable:
> +       pm_runtime_disable(&pdev->dev);
> +spi_geni_probe_err:
> +       spi_master_put(spi);
> +       return ret;
> +}

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

[Doug Anderson]

Hi,

On Mon, Oct 8, 2018 at 4:43 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > +       mas->irq = platform_get_irq(pdev, 0);
> > +       if (mas->irq < 0) {
> > +               ret = mas->irq;
> > +               dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
> > +               goto spi_geni_probe_runtime_disable;
> > +       }
>
> Nitpick: If you got the irq earlier before allocating anything then nothing has
> to be put on failure path.

I think this might have been in response to previous feedback from you
suggesting that we should get the irq as late as possible.  Ah, here
ya go:

https://lkml.kernel.org/r/153780767551.119890.9339380838620508307@swboyd.mtv.corp.google.com

> 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.

I'd rather handle the failure path then get the IRQ too early.

-Doug

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

[Stephen Boyd]

Quoting Doug Anderson (2018-10-08 16:52:36)
> Hi,
> 
> On Mon, Oct 8, 2018 at 4:43 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > +       mas->irq = platform_get_irq(pdev, 0);
> > > +       if (mas->irq < 0) {
> > > +               ret = mas->irq;
> > > +               dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
> > > +               goto spi_geni_probe_runtime_disable;
> > > +       }
> >
> > Nitpick: If you got the irq earlier before allocating anything then nothing has
> > to be put on failure path.
> 
> I think this might have been in response to previous feedback from you
> suggesting that we should get the irq as late as possible.  Ah, here
> ya go:
> 
> https://lkml.kernel.org/r/153780767551.119890.9339380838620508307@swboyd.mtv.corp.google.com

Yes I suggested we register for the irq as last as possible, but I
didn't suggest calling platform_get_irq() this late. It could be called
earlier so that if there isn't any irq then nothing to do besides return
failure.

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

[Doug Anderson]

Hi,

On Tue, Oct 9, 2018 at 9:12 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2018-10-08 16:52:36)
> > Hi,
> >
> > On Mon, Oct 8, 2018 at 4:43 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > +       mas->irq = platform_get_irq(pdev, 0);
> > > > +       if (mas->irq < 0) {
> > > > +               ret = mas->irq;
> > > > +               dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
> > > > +               goto spi_geni_probe_runtime_disable;
> > > > +       }
> > >
> > > Nitpick: If you got the irq earlier before allocating anything then nothing has
> > > to be put on failure path.
> >
> > I think this might have been in response to previous feedback from you
> > suggesting that we should get the irq as late as possible.  Ah, here
> > ya go:
> >
> > https://lkml.kernel.org/r/153780767551.119890.9339380838620508307@swboyd.mtv.corp.google.com
>
> Yes I suggested we register for the irq as last as possible, but I
> didn't suggest calling platform_get_irq() this late. It could be called
> earlier so that if there isn't any irq then nothing to do besides return
> failure.

Ah, you're suggesting separating the platform_get_irq() and the
request_irq() so that we call platform_get_irq() as the first thing in
the function but don't do the request_irq() until later.  Yeah, that
could be done and I guess if you feel strongly about it I wouldn't
object to the change, but I don't feel it buys us a lot and I kind of
like keeping the two next to each other.  Specifically why I don't
think it buys us a lot:

1. You still need the "dev_err" print, right?  platform_get_irq()
doesn't automatically print errors for you I think.

2. You now need a local variable "irq".  By putting the
platform_get_irq() before the memory allocation you now can't store it
directly in mas->irq.  We could try using "ret" as a temporary
variable but that seems worse in this case since it'd be a bit
fragile.

3. You don't get rid of any error labels / error handling so we don't
really save any code

When I tried this my diffstat says 8 lines added and 7 removed, so a
net increase in LOC FWIW.  I'm relying in gmail so my patch will be
whitespace-damaged (sigh), but you can find a clean one at:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e0325d618e209c22379e3a4269c14627b19243a8%5E%21/#F0

...the basic idea is this though:

@@ -543,6 +543,13 @@
   struct spi_geni_master *mas;
   struct resource *res;
   struct geni_se *se;
+  int irq;
+
+  irq = platform_get_irq(pdev, 0);
+  if (irq < 0) {
+    dev_err(&pdev->dev, "Err getting IRQ %d\n", irq);
+    return irq;
+  }

   spi = spi_alloc_master(&pdev->dev, sizeof(*mas));
   if (!spi)
@@ -553,6 +560,7 @@
   mas->dev = &pdev->dev;
   mas->se.dev = &pdev->dev;
   mas->se.wrapper = dev_get_drvdata(pdev->dev.parent);
+  mas->irq = irq;
   se = &mas->se;

   spi->bus_num = -1;
@@ -589,13 +597,6 @@
   if (ret)
     goto spi_geni_probe_runtime_disable;

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


-Doug

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

[Stephen Boyd]

Quoting Doug Anderson (2018-10-09 10:48:55)
> 
> Ah, you're suggesting separating the platform_get_irq() and the
> request_irq() so that we call platform_get_irq() as the first thing in
> the function but don't do the request_irq() until later.  Yeah, that
> could be done and I guess if you feel strongly about it I wouldn't
> object to the change, but I don't feel it buys us a lot and I kind of
> like keeping the two next to each other.  Specifically why I don't
> think it buys us a lot:
> 
> 1. You still need the "dev_err" print, right?  platform_get_irq()
> doesn't automatically print errors for you I think.

I usually leave it out. Who cares? Maybe we should throw a dev_err()
into platform_get_irq() on failure so we can keep drivers cleaner and
reduce a bunch of "can't find my IRQ" messages throughout the kernel.

> 
> 2. You now need a local variable "irq".  By putting the
> platform_get_irq() before the memory allocation you now can't store it
> directly in mas->irq.  We could try using "ret" as a temporary
> variable but that seems worse in this case since it'd be a bit
> fragile.
> 
> 3. You don't get rid of any error labels / error handling so we don't
> really save any code
> 
> When I tried this my diffstat says 8 lines added and 7 removed, so a
> net increase in LOC FWIW.  I'm relying in gmail so my patch will be
> whitespace-damaged (sigh), but you can find a clean one at:
> 
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e0325d618e209c22379e3a4269c14627b19243a8%5E%21/#F0
> 
> ...the basic idea is this though:
> 

Thanks! Here's an updated patch that I haven't compile tested in any way
that hoists up the IO mapping part too, which shows that the 'se' local
variable is almost entirely useless.

 drivers/spi/spi-geni-qcom.c | 49 ++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 6432ecc4e2ca..917707448578 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -538,11 +538,30 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 
 static int spi_geni_probe(struct platform_device *pdev)
 {
-	int ret;
+	int ret, irq;
 	struct spi_master *spi;
 	struct spi_geni_master *mas;
 	struct resource *res;
-	struct geni_se *se;
+	void __iomem *base;
+	struct clk *clk;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Err getting IRQ %d\n", irq);
+		return irq;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	clk = devm_clk_get(&pdev->dev, "se");
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(mas->se.clk);
+		dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
+		return ret;
+	}
 
 	spi = spi_alloc_master(&pdev->dev, sizeof(*mas));
 	if (!spi)
@@ -550,27 +569,15 @@ static int spi_geni_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, spi);
 	mas = spi_master_get_devdata(spi);
+	mas->irq = irq;
 	mas->dev = &pdev->dev;
 	mas->se.dev = &pdev->dev;
 	mas->se.wrapper = dev_get_drvdata(pdev->dev.parent);
-	se = &mas->se;
+	mas->se.base = base;
+	mas->se.clk = clk;
 
 	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 = PTR_ERR(se->base);
-		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;
@@ -589,13 +596,6 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (ret)
 		goto spi_geni_probe_runtime_disable;
 
-	mas->irq = platform_get_irq(pdev, 0);
-	if (mas->irq < 0) {
-		ret = mas->irq;
-		dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
-		goto spi_geni_probe_runtime_disable;
-	}
-
 	ret = request_irq(mas->irq, geni_spi_isr,
 			IRQF_TRIGGER_HIGH, "spi_geni", spi);
 	if (ret)
@@ -610,7 +610,6 @@ static int spi_geni_probe(struct platform_device *pdev)
 	free_irq(mas->irq, spi);
 spi_geni_probe_runtime_disable:
 	pm_runtime_disable(&pdev->dev);
-spi_geni_probe_err:
 	spi_master_put(spi);
 	return ret;
 }

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

[Doug Anderson]

Hi,
On Tue, Oct 9, 2018 at 12:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2018-10-09 10:48:55)
> >
> > Ah, you're suggesting separating the platform_get_irq() and the
> > request_irq() so that we call platform_get_irq() as the first thing in
> > the function but don't do the request_irq() until later.  Yeah, that
> > could be done and I guess if you feel strongly about it I wouldn't
> > object to the change, but I don't feel it buys us a lot and I kind of
> > like keeping the two next to each other.  Specifically why I don't
> > think it buys us a lot:
> >
> > 1. You still need the "dev_err" print, right?  platform_get_irq()
> > doesn't automatically print errors for you I think.
>
> I usually leave it out. Who cares? Maybe we should throw a dev_err()
> into platform_get_irq() on failure so we can keep drivers cleaner and
> reduce a bunch of "can't find my IRQ" messages throughout the kernel.

Yeah, all the boilerplate code is annoying.  ...of course by hoisting
it up then you get a whole bunch of people that have "optional" IRQs
suddenly getting error messages spewed which is also no good.  IMO the
convention of Linux drivers I've always reviewed is to print errors
like this, so unless that changes my vote is to follow convention.


> > 2. You now need a local variable "irq".  By putting the
> > platform_get_irq() before the memory allocation you now can't store it
> > directly in mas->irq.  We could try using "ret" as a temporary
> > variable but that seems worse in this case since it'd be a bit
> > fragile.
> >
> > 3. You don't get rid of any error labels / error handling so we don't
> > really save any code
> >
> > When I tried this my diffstat says 8 lines added and 7 removed, so a
> > net increase in LOC FWIW.  I'm relying in gmail so my patch will be
> > whitespace-damaged (sigh), but you can find a clean one at:
> >
> > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e0325d618e209c22379e3a4269c14627b19243a8%5E%21/#F0
> >
> > ...the basic idea is this though:
> >
>
> Thanks! Here's an updated patch that I haven't compile tested in any way
> that hoists up the IO mapping part too, which shows that the 'se' local
> variable is almost entirely useless.

Yeah, I'd be all for getting rid of "se".  I'm still not really seeing
the benefit of hoisting all the rest of the stuff up.  Do you feel
strongly about it?

In any case I think we've both said that all of our comments so far
are just nits and could be addressed in a followup patch.  Unless Mark
Brown wants these nits fixed ahead of time or has other changes he'd
like, I don't think we're expecting another spin of this patch from
Alok or Dilip, right?  We'd just expect them to post some follow-up
patches after Mark lands it?


-Doug

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

[Stephen Boyd]

Quoting Doug Anderson (2018-10-09 14:18:26)
> Hi,
> On Tue, Oct 9, 2018 at 12:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Doug Anderson (2018-10-09 10:48:55)
> > >
> > > Ah, you're suggesting separating the platform_get_irq() and the
> > > request_irq() so that we call platform_get_irq() as the first thing in
> > > the function but don't do the request_irq() until later.  Yeah, that
> > > could be done and I guess if you feel strongly about it I wouldn't
> > > object to the change, but I don't feel it buys us a lot and I kind of
> > > like keeping the two next to each other.  Specifically why I don't
> > > think it buys us a lot:
> > >
> > > 1. You still need the "dev_err" print, right?  platform_get_irq()
> > > doesn't automatically print errors for you I think.
> >
> > I usually leave it out. Who cares? Maybe we should throw a dev_err()
> > into platform_get_irq() on failure so we can keep drivers cleaner and
> > reduce a bunch of "can't find my IRQ" messages throughout the kernel.
> 
> Yeah, all the boilerplate code is annoying.  ...of course by hoisting
> it up then you get a whole bunch of people that have "optional" IRQs
> suddenly getting error messages spewed which is also no good.  IMO the
> convention of Linux drivers I've always reviewed is to print errors
> like this, so unless that changes my vote is to follow convention.
> 
> 
> > > 2. You now need a local variable "irq".  By putting the
> > > platform_get_irq() before the memory allocation you now can't store it
> > > directly in mas->irq.  We could try using "ret" as a temporary
> > > variable but that seems worse in this case since it'd be a bit
> > > fragile.
> > >
> > > 3. You don't get rid of any error labels / error handling so we don't
> > > really save any code
> > >
> > > When I tried this my diffstat says 8 lines added and 7 removed, so a
> > > net increase in LOC FWIW.  I'm relying in gmail so my patch will be
> > > whitespace-damaged (sigh), but you can find a clean one at:
> > >
> > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e0325d618e209c22379e3a4269c14627b19243a8%5E%21/#F0
> > >
> > > ...the basic idea is this though:
> > >
> >
> > Thanks! Here's an updated patch that I haven't compile tested in any way
> > that hoists up the IO mapping part too, which shows that the 'se' local
> > variable is almost entirely useless.
> 
> Yeah, I'd be all for getting rid of "se".  I'm still not really seeing
> the benefit of hoisting all the rest of the stuff up.  Do you feel
> strongly about it?
> 
> In any case I think we've both said that all of our comments so far
> are just nits and could be addressed in a followup patch.  Unless Mark
> Brown wants these nits fixed ahead of time or has other changes he'd
> like, I don't think we're expecting another spin of this patch from
> Alok or Dilip, right?  We'd just expect them to post some follow-up
> patches after Mark lands it?
> 
> 

Yes this is all nits territory. I don't really care too much, but the
patch is already written, so might as well roll it all in and make
things shiny.

Time to get back to real work :P

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

[alokc]

>> +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 (mas->cur_mcmd == CMD_NONE)
>> +               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))
>> +               geni_spi_handle_rx(mas);
>> +
>> +       if (m_irq & M_TX_FIFO_WATERMARK_EN)
>> +               geni_spi_handle_tx(mas);
>> +
>> +       if (m_irq & M_CMD_DONE_EN) {
>> +               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
>> +                * 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)) {
>> +               mas->cur_mcmd = CMD_NONE;
>> +               complete(&mas->xfer_done);
>> +       }
>> +
>> +       writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
>> +       spin_unlock_irqrestore(&mas->lock, flags);
>> +       return ret;
> 
> Nitpick: Now this always returns IRQ_HANDLED, and we assign ret just to
> do that. Perhaps only return IRQ_HANDLED if one of the above if
> conditions is taken?

Not always. If ISR get triggered without setting proper cur_mcmd then it 
will return IRQ_NONE.