[PATCH V2] Add SPI driver support for GENI based QUP

[PATCH V2] Add SPI driver support for GENI based QUP

[Dilip Kota]

This patch series adds SPI driver support for GENI based QUP.
Also adding the device tree binding document for reference
as per reviewers request which was merged in Linus tree.

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

 drivers/spi/Kconfig               |  12 +
 drivers/spi/Makefile              |   1 +
 drivers/spi/spi-geni-qcom.c       | 685 ++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi-geni-qcom.h |  14 +
 4 files changed, 712 insertions(+)
 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 V2] 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.

Change-Id: Ib2c7feba5a2fc4015bc83319b233b4356f7d3190
Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Dilip Kota <dkota@codeaurora.org>
---
 drivers/spi/Kconfig               |  12 +
 drivers/spi/Makefile              |   1 +
 drivers/spi/spi-geni-qcom.c       | 685 ++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi-geni-qcom.h |  14 +
 4 files changed, 712 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..d4aa755 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 SPI controller with QUP interface"
+	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..95381b8
--- /dev/null
+++ b/drivers/spi/spi-geni-qcom.c
@@ -0,0 +1,685 @@
+// 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/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/spi/spi-geni-qcom.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;
+	int irq;
+	struct device *dev;
+	int rx_fifo_depth;
+	int tx_fifo_depth;
+	int tx_fifo_width;
+	int tx_wm;
+	bool setup;
+	u32 cur_speed_hz;
+	int cur_word_len;
+	unsigned int tx_rem_bytes;
+	unsigned int rx_rem_bytes;
+	struct spi_transfer *cur_xfer;
+	struct completion xfer_done;
+	int oversampling;
+};
+
+static int get_spi_clk_cfg(u32 speed_hz, struct spi_geni_master *mas,
+			int *clk_idx, int *clk_div)
+{
+	unsigned long sclk_freq;
+	struct geni_se *se = &mas->se;
+	int ret;
+
+	ret = geni_se_clk_freq_match(&mas->se,
+				(speed_hz * mas->oversampling), clk_idx,
+				&sclk_freq, true);
+	if (ret) {
+		dev_err(mas->dev, "%s: Failed(%d) to find src clk for 0x%x\n",
+						__func__, ret, speed_hz);
+		return ret;
+	}
+
+	*clk_div = ((sclk_freq / mas->oversampling) / speed_hz);
+	if (!(*clk_div)) {
+		dev_err(mas->dev, "%s:Err:sclk:%lu oversampling:%d speed:%u\n",
+			__func__, sclk_freq, mas->oversampling, speed_hz);
+		return -EINVAL;
+	}
+
+	dev_dbg(mas->dev, "%s: req %u sclk %lu, idx %d, div %d\n", __func__,
+				speed_hz, sclk_freq, *clk_idx, *clk_div);
+	ret = clk_set_rate(se->clk, sclk_freq);
+	if (ret)
+		dev_err(mas->dev, "%s: clk_set_rate failed %d\n",
+							__func__, ret);
+	return ret;
+}
+
+static void spi_setup_word_len(struct spi_geni_master *mas, u32 mode,
+						int bits_per_word)
+{
+	int pack_words = 1;
+	bool msb_first = (mode & SPI_LSB_FIRST) ? false : true;
+	struct geni_se *se = &mas->se;
+	u32 word_len;
+
+	word_len = readl_relaxed(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->tx_fifo_width % bits_per_word))
+		pack_words = mas->tx_fifo_width / bits_per_word;
+	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_relaxed(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;
+	u16 mode = spi_slv->mode;
+	u32 loopback_cfg = readl_relaxed(se->base + SE_SPI_LOOPBACK);
+	u32 cpol = readl_relaxed(se->base + SE_SPI_CPOL);
+	u32 cpha = readl_relaxed(se->base + SE_SPI_CPHA);
+	u32 demux_sel = 0;
+	u32 demux_output_inv = 0;
+	u32 clk_sel = 0;
+	u32 m_clk_cfg = 0;
+	int ret = 0;
+	int idx;
+	int div;
+	struct spi_geni_qcom_ctrl_data *delay_params = NULL;
+	u32 spi_delay_params = 0;
+
+	loopback_cfg &= ~LOOPBACK_MSK;
+	cpol &= ~CPOL;
+	cpha &= ~CPHA;
+
+	if (mode & SPI_LOOP)
+		loopback_cfg |= LOOPBACK_ENABLE;
+
+	if (mode & SPI_CPOL)
+		cpol |= CPOL;
+
+	if (mode & SPI_CPHA)
+		cpha |= CPHA;
+
+	if (spi_slv->mode & SPI_CS_HIGH)
+		demux_output_inv |= BIT(spi_slv->chip_select);
+
+	if (spi_slv->controller_data) {
+		u32 cs_clk_delay = 0;
+		u32 inter_words_delay = 0;
+
+		delay_params =
+		(struct spi_geni_qcom_ctrl_data *) spi_slv->controller_data;
+		cs_clk_delay =
+		(delay_params->spi_cs_clk_delay << SPI_CS_CLK_DELAY_SHFT)
+							& SPI_CS_CLK_DELAY_MSK;
+		inter_words_delay =
+			delay_params->spi_inter_words_delay &
+						SPI_INTER_WORDS_DELAY_MSK;
+		spi_delay_params =
+		(inter_words_delay | cs_clk_delay);
+	}
+
+	demux_sel = spi_slv->chip_select;
+	mas->cur_speed_hz = spi_slv->max_speed_hz;
+	mas->cur_word_len = 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_relaxed(loopback_cfg, se->base + SE_SPI_LOOPBACK);
+	writel_relaxed(demux_sel, se->base + SE_SPI_DEMUX_SEL);
+	writel_relaxed(cpha, se->base + SE_SPI_CPHA);
+	writel_relaxed(cpol, se->base + SE_SPI_CPOL);
+	writel_relaxed(demux_output_inv, se->base + SE_SPI_DEMUX_OUTPUT_INV);
+	writel_relaxed(clk_sel, se->base + SE_GENI_CLK_SEL);
+	writel_relaxed(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);
+	writel_relaxed(spi_delay_params, se->base + SE_SPI_DELAY_COUNTERS);
+	return 0;
+}
+
+static int spi_geni_prepare_message(struct spi_master *spi,
+					struct spi_message *spi_msg)
+{
+	int ret = 0;
+	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, "%s: Couldn't select mode %d", __func__, ret);
+		ret = -EINVAL;
+	}
+	return ret;
+}
+
+static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
+{
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+	struct geni_se *se = &mas->se;
+
+	if (!mas->setup) {
+		int proto = geni_se_read_proto(se);
+		unsigned int major;
+		unsigned int minor;
+		unsigned int step;
+
+		if (proto != GENI_SE_SPI) {
+			dev_err(mas->dev, "Invalid proto %d\n", proto);
+			return -ENXIO;
+		}
+		mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
+		mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
+		mas->tx_fifo_width = geni_se_get_tx_fifo_width(se);
+		geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));
+		mas->oversampling = 1;
+		/* Transmit an entire FIFO worth of data per IRQ */
+		mas->tx_wm = 1;
+		geni_se_get_wrapper_version(se, major, minor, step);
+		if ((major == 1) && (minor == 0))
+			mas->oversampling = 2;
+		mas->setup = 1;
+	}
+	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 m_param = 0;
+	struct geni_se *se = &mas->se;
+	u32 spi_tx_cfg = readl_relaxed(se->base + SE_SPI_TRANS_CFG);
+	u32 trans_len = 0;
+
+	if (xfer->bits_per_word != mas->cur_word_len) {
+		spi_setup_word_len(mas, mode, xfer->bits_per_word);
+		mas->cur_word_len = xfer->bits_per_word;
+	}
+
+	/* Speed and bits per word can be overridden per transfer */
+	if (xfer->speed_hz != mas->cur_speed_hz) {
+		int ret = 0;
+		u32 clk_sel = 0;
+		u32 m_clk_cfg = 0;
+		int idx = 0;
+		int div = 0;
+
+		ret = get_spi_clk_cfg(xfer->speed_hz, mas, &idx, &div);
+		if (ret) {
+			dev_err(mas->dev, "%s:Err setting clks:%d\n",
+								__func__, ret);
+			return;
+		}
+		mas->cur_speed_hz = xfer->speed_hz;
+		clk_sel |= (idx & CLK_SEL_MSK);
+		m_clk_cfg |= ((div << CLK_DIV_SHFT) | SER_CLK_EN);
+		writel_relaxed(clk_sel, se->base + SE_GENI_CLK_SEL);
+		writel_relaxed(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_word_len % MIN_WORD_LEN)) {
+		trans_len =
+			((xfer->len * BITS_PER_BYTE) /
+					mas->cur_word_len) & TRANS_LEN_MSK;
+	} else {
+		int bytes_per_word = (mas->cur_word_len / 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_relaxed(trans_len, se->base + SE_SPI_TX_TRANS_LEN);
+	}
+
+	if (m_cmd & SPI_RX_ONLY) {
+		writel_relaxed(trans_len, se->base + SE_SPI_RX_TRANS_LEN);
+		mas->rx_rem_bytes = xfer->len;
+	}
+	writel_relaxed(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
+	geni_se_setup_m_cmd(se, m_cmd, m_param);
+	if (m_cmd & SPI_TX_ONLY)
+		writel_relaxed(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;
+	struct geni_se *se = &mas->se;
+
+	reinit_completion(&mas->xfer_done);
+	geni_se_cancel_m_cmd(se);
+	writel_relaxed(0, se->base + SE_GENI_TX_WATERMARK_REG);
+	timeout = wait_for_completion_timeout(&mas->xfer_done, HZ);
+	if (!timeout) {
+		reinit_completion(&mas->xfer_done);
+		geni_se_abort_m_cmd(se);
+		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 void geni_spi_handle_tx(struct spi_geni_master *mas)
+{
+	int i = 0;
+	int tx_fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;
+	int max_bytes = 0;
+	const u8 *tx_buf;
+	struct geni_se *se = &mas->se;
+
+	if (!mas->cur_xfer)
+		return;
+
+	/*
+	 * For non-byte aligned bits-per-word values. (e.g 9)
+	 * The FIFO packing is set to 1 SPI word per FIFO word.
+	 * Assumption is that each SPI word will be accomodated in
+	 * ceil (bits_per_word / bits_per_byte)
+	 * and the next SPI word starts at the next byte.
+	 * In such cases, we can fit 1 SPI word per FIFO word so adjust the
+	 * max byte that can be sent per IRQ accordingly.
+	 */
+	if ((mas->tx_fifo_width % mas->cur_word_len))
+		max_bytes = (mas->tx_fifo_depth - mas->tx_wm) *
+				((mas->cur_word_len / BITS_PER_BYTE) + 1);
+	else
+		max_bytes = (mas->tx_fifo_depth - mas->tx_wm) * tx_fifo_width;
+	tx_buf = mas->cur_xfer->tx_buf;
+	tx_buf += (mas->cur_xfer->len - mas->tx_rem_bytes);
+	max_bytes = min_t(int, mas->tx_rem_bytes, max_bytes);
+	while (i < max_bytes) {
+		int j;
+		u32 fifo_word = 0;
+		u8 *fifo_byte;
+		int bytes_per_fifo = tx_fifo_width;
+		int bytes_to_write = 0;
+
+		if ((mas->tx_fifo_width % mas->cur_word_len))
+			bytes_per_fifo =
+				(mas->cur_word_len / BITS_PER_BYTE) + 1;
+		bytes_to_write = min_t(int, max_bytes - i, bytes_per_fifo);
+		fifo_byte = (u8 *)&fifo_word;
+		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_relaxed(0, se->base + SE_GENI_TX_WATERMARK_REG);
+}
+
+static void geni_spi_handle_rx(struct spi_geni_master *mas)
+{
+	int i = 0;
+	struct geni_se *se = &mas->se;
+	int fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;
+	u32 rx_fifo_status = readl_relaxed(se->base + SE_GENI_RX_FIFO_STATUS);
+	int rx_bytes = 0;
+	int rx_wc = 0;
+	u8 *rx_buf;
+
+	if (!mas->cur_xfer)
+		return;
+
+	rx_buf = mas->cur_xfer->rx_buf;
+	rx_wc = rx_fifo_status & RX_FIFO_WC_MSK;
+	if (rx_fifo_status & RX_LAST) {
+		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_wc -= 1;
+			rx_bytes += rx_last_byte_valid;
+		}
+	}
+
+	/*
+	 * For non-byte aligned bits-per-word values. (e.g 9)
+	 * The FIFO packing is set to 1 SPI word per FIFO word.
+	 * Assumption is that each SPI word will be accomodated in
+	 * ceil (bits_per_word / bits_per_byte)
+	 * and the next SPI word starts at the next byte.
+	 */
+	if (!(mas->tx_fifo_width % mas->cur_word_len))
+		rx_bytes += rx_wc * fifo_width;
+	else
+		rx_bytes += rx_wc *
+			((mas->cur_word_len / BITS_PER_BYTE) + 1);
+	rx_bytes = min_t(int, mas->rx_rem_bytes, rx_bytes);
+	rx_buf += (mas->cur_xfer->len - mas->rx_rem_bytes);
+	while (i < rx_bytes) {
+		u32 fifo_word = 0;
+		u8 *fifo_byte;
+		int bytes_per_fifo = fifo_width;
+		int read_bytes = 0;
+		int j;
+
+		if ((mas->tx_fifo_width % mas->cur_word_len))
+			bytes_per_fifo =
+				(mas->cur_word_len / BITS_PER_BYTE) + 1;
+		read_bytes = min_t(int, rx_bytes - i, bytes_per_fifo);
+		ioread32_rep(se->base + SE_GENI_RX_FIFOn, &fifo_word, 1);
+		fifo_byte = (u8 *)&fifo_word;
+		for (j = 0; j < read_bytes; 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 = 0;
+	irqreturn_t ret = IRQ_HANDLED;
+
+	if (pm_runtime_status_suspended(mas->dev)) {
+		ret = IRQ_NONE;
+		goto exit_geni_spi_irq;
+	}
+	m_irq = readl_relaxed(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) {
+		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_relaxed(0, se->base + SE_GENI_TX_WATERMARK_REG);
+			dev_err(mas->dev,
+				"%s:Premature Done.tx_rem%d bpw%d\n",
+				__func__, mas->tx_rem_bytes, mas->cur_word_len);
+		}
+		if (mas->rx_rem_bytes)
+			dev_err(mas->dev,
+				"%s:Premature Done.rx_rem%d bpw%d\n",
+				__func__, mas->rx_rem_bytes, mas->cur_word_len);
+	}
+
+	if ((m_irq & M_CMD_CANCEL_EN) || (m_irq & M_CMD_ABORT_EN))
+		complete(&mas->xfer_done);
+exit_geni_spi_irq:
+	writel_relaxed(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
+	return ret;
+}
+
+static int spi_geni_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct spi_master *spi;
+	struct spi_geni_master *spi_geni;
+	struct resource *res;
+	struct geni_se *se;
+
+	spi = spi_alloc_master(&pdev->dev, sizeof(struct spi_geni_master));
+	if (!spi) {
+		ret = -ENOMEM;
+		dev_err(&pdev->dev, "Failed to alloc spi struct\n");
+		goto spi_geni_probe_err;
+	}
+
+	platform_set_drvdata(pdev, spi);
+	spi_geni = spi_master_get_devdata(spi);
+	spi_geni->dev = &pdev->dev;
+	spi_geni->se.dev = &pdev->dev;
+	spi_geni->se.wrapper = dev_get_drvdata(pdev->dev.parent);
+	se = &spi_geni->se;
+
+	spi->bus_num = -1;
+	spi->dev.of_node = pdev->dev.of_node;
+	spi_geni->se.clk = devm_clk_get(&pdev->dev, "se");
+	if (IS_ERR(spi_geni->se.clk)) {
+		ret = PTR_ERR(spi_geni->se.clk);
+		dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
+		goto spi_geni_probe_err;
+	}
+
+	if (of_property_read_u32(pdev->dev.of_node, "spi-max-frequency",
+				&spi->max_speed_hz)) {
+		dev_err(&pdev->dev, "Max frequency not specified.\n");
+		ret = -ENXIO;
+		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;
+		dev_err(&pdev->dev, "Err IO mapping iomem\n");
+		goto spi_geni_probe_err;
+	}
+
+	spi_geni->irq = platform_get_irq(pdev, 0);
+	if (spi_geni->irq < 0) {
+		dev_err(&pdev->dev, "Err getting IRQ\n");
+		ret = spi_geni->irq;
+		goto spi_geni_probe_unmap;
+	}
+	ret = devm_request_irq(&pdev->dev, spi_geni->irq, geni_spi_isr,
+				IRQF_TRIGGER_HIGH, "spi_geni", spi);
+	if (ret) {
+		dev_err(&pdev->dev, "Request_irq failed:%d: err:%d\n",
+							spi_geni->irq, ret);
+		goto spi_geni_probe_unmap;
+	}
+
+	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->prepare_transfer_hardware = spi_geni_prepare_transfer_hardware;
+	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(&spi_geni->xfer_done);
+	pm_runtime_enable(&pdev->dev);
+	ret = devm_spi_register_master(&pdev->dev, spi);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register SPI master\n");
+		goto spi_geni_probe_unmap;
+	}
+
+	dev_dbg(&pdev->dev, "%s Probed\n", __func__);
+	return ret;
+spi_geni_probe_unmap:
+	devm_iounmap(&pdev->dev, se->base);
+spi_geni_probe_err:
+	spi_master_put(spi);
+	return ret;
+}
+
+static int spi_geni_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct spi_geni_master *spi_geni = spi_master_get_devdata(master);
+
+	geni_se_resources_off(&spi_geni->se);
+	pm_runtime_put_noidle(&pdev->dev);
+	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 *spi_geni = spi_master_get_devdata(spi);
+
+	return geni_se_resources_off(&spi_geni->se);
+}
+
+static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
+{
+	struct spi_master *spi = dev_get_drvdata(dev);
+	struct spi_geni_master *spi_geni = spi_master_get_devdata(spi);
+
+	return geni_se_resources_on(&spi_geni->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.

[PATCH V2] dt-bindings: soc: qcom: Add device tree binding for GENI SE

[Dilip Kota]

From: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>

Add device tree binding support for the QCOM GENI SE driver.

Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  | 119 +++++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
new file mode 100644
index 0000000..d330c73
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
@@ -0,0 +1,119 @@
+Qualcomm Technologies, Inc. GENI Serial Engine QUP Wrapper Controller
+
+Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP) wrapper
+is a programmable module for supporting a wide range of serial interfaces
+like UART, SPI, I2C, I3C, etc. A single QUP module can provide upto 8 Serial
+Interfaces, using its internal Serial Engines. The GENI Serial Engine QUP
+Wrapper controller is modeled as a node with zero or more child nodes each
+representing a serial engine.
+
+Required properties:
+- compatible:		Must be "qcom,geni-se-qup".
+- reg:			Must contain QUP register address and length.
+- clock-names:		Must contain "m-ahb" and "s-ahb".
+- clocks:		AHB clocks needed by the device.
+
+Required properties if child node exists:
+- #address-cells: 	Must be <1> for Serial Engine Address
+- #size-cells: 		Must be <1> for Serial Engine Address Size
+- ranges: 		Must be present
+
+Properties for children:
+
+A GENI based QUP wrapper controller node can contain 0 or more child nodes
+representing serial devices.  These serial devices can be a QCOM UART, I2C
+controller, SPI controller, or some combination of aforementioned devices.
+Please refer below the child node definitions for the supported serial
+interface protocols.
+
+Qualcomm Technologies Inc. GENI Serial Engine based I2C Controller
+
+Required properties:
+- compatible:		Must be "qcom,geni-i2c".
+- reg: 			Must contain QUP register address and length.
+- interrupts: 		Must contain I2C interrupt.
+- clock-names: 		Must contain "se".
+- clocks: 		Serial engine core clock needed by the device.
+- #address-cells:	Must be <1> for I2C device address.
+- #size-cells:		Must be <0> as I2C addresses have no size component.
+
+Optional property:
+- clock-frequency:	Desired I2C bus clock frequency in Hz.
+			When missing default to 400000Hz.
+
+Child nodes should conform to I2C bus binding as described in i2c.txt.
+
+Qualcomm Technologies Inc. GENI Serial Engine based UART Controller
+
+Required properties:
+- compatible:		Must be "qcom,geni-debug-uart".
+- reg: 			Must contain UART register location and length.
+- interrupts: 		Must contain UART core interrupts.
+- clock-names:		Must contain "se".
+- 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.
+- 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>.
+
+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:
+	geniqup@8c0000 {
+		compatible = "qcom,geni-se-qup";
+		reg = <0x8c0000 0x6000>;
+		clock-names = "m-ahb", "s-ahb";
+		clocks = <&clock_gcc GCC_QUPV3_WRAP_0_M_AHB_CLK>,
+			<&clock_gcc GCC_QUPV3_WRAP_0_S_AHB_CLK>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		i2c0: i2c@a94000 {
+			compatible = "qcom,geni-i2c";
+			reg = <0xa94000 0x4000>;
+			interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>;
+			clock-names = "se";
+			clocks = <&clock_gcc GCC_QUPV3_WRAP0_S5_CLK>;
+			pinctrl-names = "default", "sleep";
+			pinctrl-0 = <&qup_1_i2c_5_active>;
+			pinctrl-1 = <&qup_1_i2c_5_sleep>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		uart0: serial@a88000 {
+			compatible = "qcom,geni-debug-uart";
+			reg = <0xa88000 0x7000>;
+			interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
+			clock-names = "se";
+			clocks = <&clock_gcc GCC_QUPV3_WRAP0_S0_CLK>;
+			pinctrl-names = "default", "sleep";
+			pinctrl-0 = <&qup_1_uart_3_active>;
+			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>;
+			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

Re: [PATCH V2] dt-bindings: soc: qcom: Add device tree binding for GENI SE

[Doug Anderson]

Hi,

On Fri, Jul 20, 2018 at 4:51 AM, Dilip Kota <dkota@codeaurora.org> wrote:
> From: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>
> Add device tree binding support for the QCOM GENI SE driver.
>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
>  .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  | 119 +++++++++++++++++++++
>  1 file changed, 119 insertions(+)

Just a note to mention that this patch landed two months ago and can
already be found in mainline Linux.  See commit e9b02415565b
("dt-bindings: soc: qcom: Add device tree binding for GENI SE").

...I think Mark Brown made comments on v1 of the SPI driver that he
was surprised about how the bindings landed, but that doesn't mean you
should re-post a patch that is already in mainline--just point to the
patch that landed by git hash.  If something about the bindings needs
to change then post a patch that changes them.

-Doug

Re: [PATCH V2] Add SPI driver support for GENI based QUP

[Doug Anderson]

Hi,

On Fri, Jul 20, 2018 at 4:50 AM, Dilip Kota <dkota@codeaurora.org> wrote:
> This patch series adds SPI driver support for GENI based QUP.
> Also adding the device tree binding document for reference
> as per reviewers request which was merged in Linus tree.
>
> Girish Mahadevan (1):
>   spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
>
>  drivers/spi/Kconfig               |  12 +
>  drivers/spi/Makefile              |   1 +
>  drivers/spi/spi-geni-qcom.c       | 685 ++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi-geni-qcom.h |  14 +
>  4 files changed, 712 insertions(+)
>  create mode 100644 drivers/spi/spi-geni-qcom.c
>  create mode 100644 include/linux/spi/spi-geni-qcom.h

This looks like it was supposed to be a cover letter.  Typically
that's labelled as "patch 0" in the series.  Other handy traditions:

* The cover letter should contain a description of what changed
between this patch and the last one

* You only want a cover letter for a patch series with more than one
patch in it.  In your case you only have one patch so you should have
no cover letter.

* It's handy if your cover letter has prefixes that match the
subsystem just like the patch itself.  AKA: "spi: spi-geni-qcom:"


Personally I use "patman" to help me with sending out my patches.  See
<https://github.com/u-boot/u-boot/tree/master/tools/patman>.


-Doug

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

[kbuild test robot]

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

Hi Girish,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on spi/for-next]
[also build test WARNING on v4.18-rc5 next-20180720]
[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-spi-geni-qcom-Add-SPI-driver-support-for-GENI-based-QUP/20180721-034916
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from drivers/spi/spi-geni-qcom.c:11:0:
   drivers/spi/spi-geni-qcom.c: In function 'spi_geni_prepare_transfer_hardware':
   include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared (first use in this function); did you mean 'vm_region'?
     step = version & HW_VER_STEP_MASK; \
            ^
>> drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version'
      geni_se_get_wrapper_version(se, major, minor, step);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/qcom-geni-se.h:238:9: note: each undeclared identifier is reported only once for each function it appears in
     step = version & HW_VER_STEP_MASK; \
            ^
>> drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version'
      geni_se_get_wrapper_version(se, major, minor, step);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/geni_se_get_wrapper_version +256 drivers/spi/spi-geni-qcom.c

   233	
   234	static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
   235	{
   236		struct spi_geni_master *mas = spi_master_get_devdata(spi);
   237		struct geni_se *se = &mas->se;
   238	
   239		if (!mas->setup) {
   240			int proto = geni_se_read_proto(se);
   241			unsigned int major;
   242			unsigned int minor;
   243			unsigned int step;
   244	
   245			if (proto != GENI_SE_SPI) {
   246				dev_err(mas->dev, "Invalid proto %d\n", proto);
   247				return -ENXIO;
   248			}
   249			mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
   250			mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
   251			mas->tx_fifo_width = geni_se_get_tx_fifo_width(se);
   252			geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));
   253			mas->oversampling = 1;
   254			/* Transmit an entire FIFO worth of data per IRQ */
   255			mas->tx_wm = 1;
 > 256			geni_se_get_wrapper_version(se, major, minor, step);
   257			if ((major == 1) && (minor == 0))
   258				mas->oversampling = 2;
   259			mas->setup = 1;
   260		}
   261		return 0;
   262	}
   263	

---
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: 64141 bytes --]

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

[Doug Anderson]

>Hi,

On Fri, Jul 20, 2018 at 4:50 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.
>
> Change-Id: Ib2c7feba5a2fc4015bc83319b233b4356f7d3190
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>

Why is Girish here twice?  That doesn't seem right.  This is also an
awful lot of Signed-off-by tags.  Did they all contribute code here?
v1 of this patch only had girish.


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

Rob may have reviewed the bindings, but that doesn't mean he's happy
with the code.  I'd suggest removing his Reviewed-by


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

Are you certain that Stephen provided his Reviewed-by?  I don't see
it.  ...and, as you can see below, it seems like you ignored most of
Stephen's comments on v1.



> Signed-off-by: Dilip Kota <dkota@codeaurora.org>
> ---

I would have expected some version history here to say what changed
from v1 to v2.  See my comments on the cover letter and consider using
patman <https://github.com/u-boot/u-boot/tree/master/tools/patman> to
help you.


> +config SPI_QCOM_GENI
> +       tristate "Qualcomm SPI controller with QUP interface"

On v1 Stephen Boyd said:

> This is the same help text as the SPI_QUP config up above. Please make
> it different somehow by adding GENI or something like that instead of
> QUP?

More explicitly, please change the line to:
  tristate "Qualcomm SPI controller with GENI interface"


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

On v1 Stephen Boyd said:

> Drop "built-in"?


> +struct spi_geni_master {
> +       struct geni_se se;
> +       int irq;
> +       struct device *dev;
> +       int rx_fifo_depth;
> +       int tx_fifo_depth;
> +       int tx_fifo_width;
> +       int tx_wm;

In v1 Stephen said:

> All of these can be unsigned ints?


> +       bool setup;
> +       u32 cur_speed_hz;
> +       int cur_word_len;

In v1 Stephen said:

> unsigned?


> +       unsigned int tx_rem_bytes;
> +       unsigned int rx_rem_bytes;
> +       struct spi_transfer *cur_xfer;
> +       struct completion xfer_done;
> +       int oversampling;

In v1 Stephen said:

> unsigned?


> +static int get_spi_clk_cfg(u32 speed_hz, struct spi_geni_master *mas,

In v1 Stephen said:

> Why a u32? And not unsigned int?


> +       if (ret) {
> +               dev_err(mas->dev, "%s: Failed(%d) to find src clk for 0x%x\n",
> +                                               __func__, ret, speed_hz);

In v1 Stephen said:

> Frequency Hz printed in hex? I am not a hex computer! I hope!

...I'll further add that printing __func__ is generally discouraged
for dev_xx printouts.  They've already got the device name and that
together with the string should be enough.  Remove it.


> +static void spi_setup_word_len(struct spi_geni_master *mas, u32 mode,

In v1 Stephen said:

> mode is only u16 in spi_device struct. Maybe it would be better to pass
> the spi_device struct here and then pick out the mode from there.


> +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;
> +       u16 mode = spi_slv->mode;
> +       u32 loopback_cfg = readl_relaxed(se->base + SE_SPI_LOOPBACK);
> +       u32 cpol = readl_relaxed(se->base + SE_SPI_CPOL);
> +       u32 cpha = readl_relaxed(se->base + SE_SPI_CPHA);
> +       u32 demux_sel = 0;
> +       u32 demux_output_inv = 0;
> +       u32 clk_sel = 0;
> +       u32 m_clk_cfg = 0;
> +       int ret = 0;

In v1 Stephen said:

> Don't initialize variables and then overwrite them.


> +       demux_sel = spi_slv->chip_select;
> +       mas->cur_speed_hz = spi_slv->max_speed_hz;

In v1 Stephen said:

> Why can't you use clk_get_rate() instead? Or call clk_set_rate() with
> the rate you want the master clk to run at and then divide that down
> from there?

...there was a bunch of arguments here and I'm not sure I followed all
of them, but the net-net is that there should be no rate at the
controller level.  Each SPI slave should request its rate and that
should take effect for each transfer.  Basically just honor the rate
of the transfer in setup_fifo_xfer and be done with it.


> +       mas->cur_word_len = 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);

In v1 Stephen said:

> Just assign clk_sel instead of ORing it because it's initialized to 0
> up above.


> +       m_clk_cfg |= ((div << CLK_DIV_SHFT) | SER_CLK_EN);

In v1 Stephen said:

> Same story.


> +               mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
> +               mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
> +               mas->tx_fifo_width = geni_se_get_tx_fifo_width(se);
> +               geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));

In v1 Stephen said:

> Why 2? Drop extra parenthesis please.

Girish responded that the 2 is important and that he would add a
detailed comment, which I don't see.  I also still see parenthesis.


> +               mas->oversampling = 1;
> +               /* Transmit an entire FIFO worth of data per IRQ */
> +               mas->tx_wm = 1;
> +               geni_se_get_wrapper_version(se, major, minor, step);

In v1 Stephen said:

> Drop extra parenthesis.

I'll further add that I'd prefer it if you based your series atop
<https://patchwork.kernel.org/patch/10412417/> which changes the
prototype here.


> +       /*
> +        * 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))

In v1 Stephen said:

> Combine else and if here into an else if.


> +       mas->cur_xfer = xfer;
> +       if (m_cmd & SPI_TX_ONLY) {
> +               mas->tx_rem_bytes = xfer->len;
> +               writel_relaxed(trans_len, se->base + SE_SPI_TX_TRANS_LEN);
> +       }
> +
> +       if (m_cmd & SPI_RX_ONLY) {
> +               writel_relaxed(trans_len, se->base + SE_SPI_RX_TRANS_LEN);
> +               mas->rx_rem_bytes = xfer->len;
> +       }
> +       writel_relaxed(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
> +       geni_se_setup_m_cmd(se, m_cmd, m_param);
> +       if (m_cmd & SPI_TX_ONLY)
> +               writel_relaxed(mas->tx_wm, se->base + SE_GENI_TX_WATERMARK_REG);

In v1 Stephen said:

> This can't be combined with above m_cmd & SPI_TX_ONLY statement?

Girish said it can't.  ...but IMO if it really can't then please add a
comment in the code so someone doesn't later go back and try to
combine.


> +static void geni_spi_handle_tx(struct spi_geni_master *mas)
> +{
> +       int i = 0;
> +       int tx_fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;
> +       int max_bytes = 0;
> +       const u8 *tx_buf;
> +       struct geni_se *se = &mas->se;
> +
> +       if (!mas->cur_xfer)
> +               return;
> +
> +       /*
> +        * For non-byte aligned bits-per-word values. (e.g 9)
> +        * The FIFO packing is set to 1 SPI word per FIFO word.

In v1 Stephen said:

> Decapitalize "The"


> +        * Assumption is that each SPI word will be accomodated in
> +        * ceil (bits_per_word / bits_per_byte)
> +        * and the next SPI word starts at the next byte.
> +        * In such cases, we can fit 1 SPI word per FIFO word so adjust the
> +        * max byte that can be sent per IRQ accordingly.
> +        */
> +       if ((mas->tx_fifo_width % mas->cur_word_len))

In v1 Stephen said:

> Drop extra parenthesis please.


> +               max_bytes = (mas->tx_fifo_depth - mas->tx_wm) *
> +                               ((mas->cur_word_len / BITS_PER_BYTE) + 1);
> +       else
> +               max_bytes = (mas->tx_fifo_depth - mas->tx_wm) * tx_fifo_width;

In v1 Stephen said:

> The above if else needs braces, or it could be written as:
>
>   max_bytes = mas->tx_fifo_depth - mas->tx_wm;
>   if (mas->tx_fifo_width % mas->cur_word_len) {
>     max_bytes *= (mas->cur_word_len / BITS_PER_BYTE) + 1;
>   else
>     max_bytes *= tx_fifo_width;


> +       tx_buf = mas->cur_xfer->tx_buf;
> +       tx_buf += (mas->cur_xfer->len - mas->tx_rem_bytes);

In v1 Stephen said:

> Drop parenthesis.


> +       max_bytes = min_t(int, mas->tx_rem_bytes, max_bytes);

In v1 Stephen said:

> Why isn't max_bytes unsigned?


> +       while (i < max_bytes) {
> +               int j;
> +               u32 fifo_word = 0;
> +               u8 *fifo_byte;
> +               int bytes_per_fifo = tx_fifo_width;
> +               int bytes_to_write = 0;
> +
> +               if ((mas->tx_fifo_width % mas->cur_word_len))

In v1 Stephen said:

> Drop parenthesis.


> +                       bytes_per_fifo =
> +                               (mas->cur_word_len / BITS_PER_BYTE) + 1;
> +               bytes_to_write = min_t(int, max_bytes - i, bytes_per_fifo);

In v1 Stephen said:

> More things can be unsigned?


> +               fifo_byte = (u8 *)&fifo_word;
> +               for (j = 0; j < bytes_to_write; j++)
> +                       fifo_byte[j] = tx_buf[i++];

In v1 Stephen said:

> Why are we doing all this work to fill in a fifo byte at a time? tx_buf
> should be a buffer of bytes that we can iowrite32_rep() with directly.
> And then we could just run iowrite32_rep() with some count of bytes to
> write? I suppose we may run into problems with unaligned size buffers,
> but it sounds like that doesn't happen?

Girish had a response to this and Stephen had another question which
was never answered.  Specifically he asked:

> Have you tested out non-byte aligned word size transfers?

Please answer and also add a comment justifying why we're doing all
this work to fill a fifo a byte at a time.


> +static void geni_spi_handle_rx(struct spi_geni_master *mas)
> +{
> +       int i = 0;
> +       struct geni_se *se = &mas->se;
> +       int fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;
> +       u32 rx_fifo_status = readl_relaxed(se->base + SE_GENI_RX_FIFO_STATUS);
> +       int rx_bytes = 0;
> +       int rx_wc = 0;
> +       u8 *rx_buf;
> +
> +       if (!mas->cur_xfer)
> +               return;

In v1 Stephen said:

> This is an error condition? We should return IRQ_NONE in such a case in
> the irq handler? Or somehow indicate this up that we actually handled an
> rx or not so the irqhandler can do the right thing.


> +       /*
> +        * For non-byte aligned bits-per-word values. (e.g 9)
> +        * The FIFO packing is set to 1 SPI word per FIFO word.
> +        * Assumption is that each SPI word will be accomodated in
> +        * ceil (bits_per_word / bits_per_byte)
> +        * and the next SPI word starts at the next byte.
> +        */
> +       if (!(mas->tx_fifo_width % mas->cur_word_len))
> +               rx_bytes += rx_wc * fifo_width;
> +       else
> +               rx_bytes += rx_wc *
> +                       ((mas->cur_word_len / BITS_PER_BYTE) + 1);
> +       rx_bytes = min_t(int, mas->rx_rem_bytes, rx_bytes);

In v1 Stephen said:

> min_t is preferably avoided.


> +       rx_buf += (mas->cur_xfer->len - mas->rx_rem_bytes);

In v1 Stephen said:

> Drop parenthesis.


> +       while (i < rx_bytes) {
> +               u32 fifo_word = 0;
> +               u8 *fifo_byte;
> +               int bytes_per_fifo = fifo_width;
> +               int read_bytes = 0;
> +               int j;
> +
> +               if ((mas->tx_fifo_width % mas->cur_word_len))

In v1 Stephen said:

> Drop parenthesis.


> +static int spi_geni_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       struct spi_master *spi;
> +       struct spi_geni_master *spi_geni;
> +       struct resource *res;
> +       struct geni_se *se;
> +
> +       spi = spi_alloc_master(&pdev->dev, sizeof(struct spi_geni_master));
> +       if (!spi) {
> +               ret = -ENOMEM;
> +               dev_err(&pdev->dev, "Failed to alloc spi struct\n");

In v1 Stephen said:

> We don't need allocation error messages.



> +       platform_set_drvdata(pdev, spi);
> +       spi_geni = spi_master_get_devdata(spi);
> +       spi_geni->dev = &pdev->dev;
> +       spi_geni->se.dev = &pdev->dev;
> +       spi_geni->se.wrapper = dev_get_drvdata(pdev->dev.parent);
> +       se = &spi_geni->se;
> +
> +       spi->bus_num = -1;
> +       spi->dev.of_node = pdev->dev.of_node;

In v1 Mark Brown said:

> This is broken, the virtual SPI device does not exist in DT and this
> might break things.

...though I'm as confused as Girish was.  It seems like all the
drivers do what you're doing...  I didn't dig into the code though.


> +       spi_geni->se.clk = devm_clk_get(&pdev->dev, "se");
> +       if (IS_ERR(spi_geni->se.clk)) {
> +               ret = PTR_ERR(spi_geni->se.clk);
> +               dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
> +               goto spi_geni_probe_err;
> +       }
> +
> +       if (of_property_read_u32(pdev->dev.of_node, "spi-max-frequency",
> +                               &spi->max_speed_hz)) {

In v1 Stephen said:

> Why does this need to come from DT?

Please remove this concept.  No other SPI driver has it.


> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       se->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(se->base)) {
> +               ret = -ENOMEM;
> +               dev_err(&pdev->dev, "Err IO mapping iomem\n");

In v1 Stephen said:

> We don't need these error messages. devm_ioremap_resource() already does
> it.


> +       init_completion(&spi_geni->xfer_done);
> +       pm_runtime_enable(&pdev->dev);
> +       ret = devm_spi_register_master(&pdev->dev, spi);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to register SPI master\n");

In v1 Stephen said:

> Most likely spi_register_master() is going to print what went wrong so
> this print is not useful.


> +static int spi_geni_remove(struct platform_device *pdev)
> +{
> +       struct spi_master *master = platform_get_drvdata(pdev);
> +       struct spi_geni_master *spi_geni = spi_master_get_devdata(master);
> +
> +       geni_se_resources_off(&spi_geni->se);

Why would you need to call geni_se_resources_off()?  Isn't it handled
by pm_runtime?


> +       pm_runtime_put_noidle(&pdev->dev);

I'm curious: why would you have a "put" here?  Won't that result in an
imbalance since you don't exit probe with "get"?  ...or did pm_runtime
throw me for a loop again?


> 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 */

In v1 Stephen said:

> Why?


> +/*
> + * 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 {

In v1 Stephen said:

> What's the point of this header file and structure? This driver supports
> board files? Isn't everything DT now?

There was a bunch of discussion in followup threads but no closure and
I still don't understand why we need this.  Please remove it.

One note is that Girish seemed to justify this by saying it's useful
for "inter-transfer delays within a message".  Isn't this just the
"delay_usecs" within a transfer?  I think the answer here is to remove
the header file and structure and if you can later show where it's
used then we can always add it back in.


> +       u32 spi_cs_clk_delay;
> +       u32 spi_inter_words_delay;
> +};
> +
> +#endif /*__SPI_GENI_QCOM_HEADER___*/
> --

I'm not sure where to comment about this, so adding it to the end:

Between v1 and v2 you totally removed all the locking.  Presumably
this is because you didn't want to hold the lock in
handle_fifo_timeout() while waiting for the completion.  IMO taking
the lock out was the wrong thing to do.  You should keep it, but just
drop the lock before wait_for_completion_timeout() and add it back
afterwards.  Specifically you _don't_ want the IRQ and timeout code
stomping on each other.

---

I'll also mention that you just wasted quite a bit of reviewer
resources for me to repeat the refrain "you didn't address comments
from v1".  Reviewer time is precious.  It's better that you wasted my
time than Mark's, but there are still better things for me to do.
Please be more careful.  I realize that this is your first patch
posted but ignoring previous feedback is not a good start.  If you
want to see the v1 discussion, you can easily find it at:

https://patchwork.kernel.org/patch/10379299/


...since I spent so much time checking over what you did vs. v1, I
didn't actually have a chance to review v2 on its own merits.


-Doug

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

[Doug Anderson]

Hi,

On Fri, Jul 20, 2018 at 3:53 PM, Doug Anderson <dianders@chromium.org> wrote:
>> +               mas->oversampling = 1;
>> +               /* Transmit an entire FIFO worth of data per IRQ */
>> +               mas->tx_wm = 1;
>> +               geni_se_get_wrapper_version(se, major, minor, step);
>
> In v1 Stephen said:
>
>> Drop extra parenthesis.
>
> I'll further add that I'd prefer it if you based your series atop
> <https://patchwork.kernel.org/patch/10412417/> which changes the
> prototype here.

Whoops.  Extra parenthesis was actually referring to the line below
this one, AKA:


> +               if ((major == 1) && (minor == 0))
> +                       mas->oversampling = 2;

...that still applies.  ...but then ignore my comment suggesting that
you base your series atop Stephen's.  You already did that.  Certainly
you should mention somewhere in your email that your patch is based on
his.


-Doug

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3937 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.18-rc5 next-20180720]
[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-spi-geni-qcom-Add-SPI-driver-support-for-GENI-based-QUP/20180721-034916
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-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=ia64 

All errors (new ones prefixed by >>):

   In file included from drivers/spi/spi-geni-qcom.c:11:
   drivers/spi/spi-geni-qcom.c: In function 'spi_geni_prepare_transfer_hardware':
>> include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared (first use in this function); did you mean 'vm_region'?
     step = version & HW_VER_STEP_MASK; \
            ^~~~~~~
   drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version'
      geni_se_get_wrapper_version(se, major, minor, step);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/qcom-geni-se.h:238:9: note: each undeclared identifier is reported only once for each function it appears in
     step = version & HW_VER_STEP_MASK; \
            ^~~~~~~
   drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version'
      geni_se_get_wrapper_version(se, major, minor, step);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from drivers//spi/spi-geni-qcom.c:11:
   drivers//spi/spi-geni-qcom.c: In function 'spi_geni_prepare_transfer_hardware':
>> include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared (first use in this function); did you mean 'vm_region'?
     step = version & HW_VER_STEP_MASK; \
            ^~~~~~~
   drivers//spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version'
      geni_se_get_wrapper_version(se, major, minor, step);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/qcom-geni-se.h:238:9: note: each undeclared identifier is reported only once for each function it appears in
     step = version & HW_VER_STEP_MASK; \
            ^~~~~~~
   drivers//spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version'
      geni_se_get_wrapper_version(se, major, minor, step);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +238 include/linux/qcom-geni-se.h

eddac5af Karthikeyan Ramasubramanian 2018-03-30  231  
eddac5af Karthikeyan Ramasubramanian 2018-03-30  232  #define geni_se_get_wrapper_version(se, major, minor, step) do { \
eddac5af Karthikeyan Ramasubramanian 2018-03-30  233  	u32 ver; \
eddac5af Karthikeyan Ramasubramanian 2018-03-30  234  \
eddac5af Karthikeyan Ramasubramanian 2018-03-30  235  	ver = geni_se_get_qup_hw_version(se); \
eddac5af Karthikeyan Ramasubramanian 2018-03-30  236  	major = (ver & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT; \
eddac5af Karthikeyan Ramasubramanian 2018-03-30  237  	minor = (ver & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT; \
eddac5af Karthikeyan Ramasubramanian 2018-03-30 @238  	step = version & HW_VER_STEP_MASK; \
eddac5af Karthikeyan Ramasubramanian 2018-03-30  239  } while (0)
eddac5af Karthikeyan Ramasubramanian 2018-03-30  240  

:::::: The code at line 238 was first introduced by commit
:::::: eddac5af06546d2e7a0730e3dc02dde3dc91098a soc: qcom: Add GENI based QUP Wrapper driver

:::::: TO: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
:::::: CC: Andy Gross <andy.gross@linaro.org>

---
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: 50909 bytes --]

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

[dkota]

On 2018-07-21 05:07, kbuild test robot wrote:
> 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.18-rc5 next-20180720]
> [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-spi-geni-qcom-Add-SPI-driver-support-for-GENI-based-QUP/20180721-034916
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 
> for-next
> config: ia64-allmodconfig (attached as .config)
> compiler: ia64-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=ia64
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from drivers/spi/spi-geni-qcom.c:11:
>    drivers/spi/spi-geni-qcom.c: In function
> 'spi_geni_prepare_transfer_hardware':
>>> include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared 
>>> (first use in this function); did you mean 'vm_region'?
>      step = version & HW_VER_STEP_MASK; \
>             ^~~~~~~
>    drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro
> 'geni_se_get_wrapper_version'
>       geni_se_get_wrapper_version(se, major, minor, step);
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>    include/linux/qcom-geni-se.h:238:9: note: each undeclared
> identifier is reported only once for each function it appears in
>      step = version & HW_VER_STEP_MASK; \
>             ^~~~~~~
>    drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro
> 'geni_se_get_wrapper_version'
>       geni_se_get_wrapper_version(se, major, minor, step);
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> --
>    In file included from drivers//spi/spi-geni-qcom.c:11:
>    drivers//spi/spi-geni-qcom.c: In function
> 'spi_geni_prepare_transfer_hardware':
>>> include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared 
>>> (first use in this function); did you mean 'vm_region'?
>      step = version & HW_VER_STEP_MASK; \
>             ^~~~~~~
>    drivers//spi/spi-geni-qcom.c:256:3: note: in expansion of macro
> 'geni_se_get_wrapper_version'
>       geni_se_get_wrapper_version(se, major, minor, step);
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>    include/linux/qcom-geni-se.h:238:9: note: each undeclared
> identifier is reported only once for each function it appears in
>      step = version & HW_VER_STEP_MASK; \
>             ^~~~~~~
>    drivers//spi/spi-geni-qcom.c:256:3: note: in expansion of macro
> 'geni_se_get_wrapper_version'
>       geni_se_get_wrapper_version(se, major, minor, step);
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> vim +238 include/linux/qcom-geni-se.h
> 
> eddac5af Karthikeyan Ramasubramanian 2018-03-30  231
> eddac5af Karthikeyan Ramasubramanian 2018-03-30  232  #define
> geni_se_get_wrapper_version(se, major, minor, step) do { \
> eddac5af Karthikeyan Ramasubramanian 2018-03-30  233  	u32 ver; \
> eddac5af Karthikeyan Ramasubramanian 2018-03-30  234  \
> eddac5af Karthikeyan Ramasubramanian 2018-03-30  235  	ver =
> geni_se_get_qup_hw_version(se); \
> eddac5af Karthikeyan Ramasubramanian 2018-03-30  236  	major = (ver &
> HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT; \
> eddac5af Karthikeyan Ramasubramanian 2018-03-30  237  	minor = (ver &
> HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT; \
> eddac5af Karthikeyan Ramasubramanian 2018-03-30 @238  	step = version
> & HW_VER_STEP_MASK; \
> eddac5af Karthikeyan Ramasubramanian 2018-03-30  239  } while (0)
> eddac5af Karthikeyan Ramasubramanian 2018-03-30  240
> 
> :::::: The code at line 238 was first introduced by commit
> :::::: eddac5af06546d2e7a0730e3dc02dde3dc91098a soc: qcom: Add GENI
> based QUP Wrapper driver
> 
> :::::: TO: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> :::::: CC: Andy Gross <andy.gross@linaro.org>
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology 
> Center
> https://lists.01.org/pipermail/kbuild-all                   Intel 
> Corporation


HI,

For now please ignore the PATCHSET 2. I will upload a new PATCHSET 
addressing all the comments of patchset 1 with detailed description.
Sorry for the inconvenience.

Regards,
Dilip