[2/2] spi: Add Qualcomm QUP SPI controller support

Commit Message

Ivan T. Ivanov Feb. 6, 2014, 4:57 p.m. UTC

From: "Ivan T. Ivanov" <[e-mail]>

Qualcomm Universal Peripheral (QUP) core is an 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
support up to 50MHz, up to four chip selects, and a programmable
data path from 4 bits to 32 bits; MODE0..3 protocols

Signed-off-by: Ivan T. Ivanov <[e-mail]>
Cc: Alok Chauhan <[e-mail]>
Cc: Gilad Avidov <[e-mail]>
Cc: Kiran Gunda <[e-mail]>
Cc: Sagar Dharia <[e-mail]>
---
 drivers/spi/Kconfig   |   14 +
 drivers/spi/Makefile  |    1 +
 drivers/spi/spi-qup.c |  898 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 913 insertions(+)
 create mode 100644 drivers/spi/spi-qup.c

Comments

Andy Gross Feb. 7, 2014, 7:39 a.m. UTC | #1

On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <[e-mail]>
> 
> Qualcomm Universal Peripheral (QUP) core is an 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
> support up to 50MHz, up to four chip selects, and a programmable
> data path from 4 bits to 32 bits; MODE0..3 protocols
> 
> Signed-off-by: Ivan T. Ivanov <[e-mail]>
> Cc: Alok Chauhan <[e-mail]>
> Cc: Gilad Avidov <[e-mail]>
> Cc: Kiran Gunda <[e-mail]>
> Cc: Sagar Dharia <[e-mail]>
> ---
>  drivers/spi/Kconfig   |   14 +
>  drivers/spi/Makefile  |    1 +
>  drivers/spi/spi-qup.c |  898 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 913 insertions(+)
>  create mode 100644 drivers/spi/spi-qup.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index ba9310b..bf8ce6b 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -381,6 +381,20 @@ config SPI_RSPI
>  	help
>  	  SPI driver for Renesas RSPI blocks.
>  
> +config SPI_QUP
> +	tristate "Qualcomm SPI Support with QUP interface"
> +	depends on ARCH_MSM

I'd change to ARCH_MSM_DT.  This ensures the OF component is there.

> +	help
> +	  Qualcomm Universal Peripheral (QUP) core is an 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 support up to 50MHz, up to four chip selects, and a
> +	  programmable data path from 4 bits to 32 bits; supports numerous
> +	  protocol variants.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called spi_qup.
> +
>  config SPI_S3C24XX
>  	tristate "Samsung S3C24XX series SPI"
>  	depends on ARCH_S3C24XX
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 95af48d..e598147 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -59,6 +59,7 @@ spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_PXADMA)	+= spi-pxa2xx-pxadma.o
>  spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_DMA)	+= spi-pxa2xx-dma.o
>  obj-$(CONFIG_SPI_PXA2XX)		+= spi-pxa2xx-platform.o
>  obj-$(CONFIG_SPI_PXA2XX_PCI)		+= spi-pxa2xx-pci.o
> +obj-$(CONFIG_SPI_QUP)			+= spi-qup.o
>  obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
>  obj-$(CONFIG_SPI_S3C24XX)		+= spi-s3c24xx-hw.o
>  spi-s3c24xx-hw-y			:= spi-s3c24xx.o
> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> new file mode 100644
> index 0000000..5eb5e8f
> --- /dev/null
> +++ b/drivers/spi/spi-qup.c
> @@ -0,0 +1,898 @@
> +/*
> + * Copyright (c) 2008-2014, The Linux foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License rev 2 and
> + * only rev 2 as published by the free Software foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or fITNESS fOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>

Remove this for now.  No runtime support.

> +#include <linux/spi/spi.h>
> +
> +#define QUP_CONFIG			0x0000
> +#define QUP_STATE			0x0004
> +#define QUP_IO_M_MODES			0x0008
> +#define QUP_SW_RESET			0x000c
> +#define QUP_OPERATIONAL			0x0018
> +#define QUP_ERROR_FLAGS			0x001c
> +#define QUP_ERROR_FLAGS_EN		0x0020
> +#define QUP_OPERATIONAL_MASK		0x0028
> +#define QUP_HW_VERSION			0x0030
> +#define QUP_MX_OUTPUT_CNT		0x0100
> +#define QUP_OUTPUT_FIFO			0x0110
> +#define QUP_MX_WRITE_CNT		0x0150
> +#define QUP_MX_INPUT_CNT		0x0200
> +#define QUP_MX_READ_CNT			0x0208
> +#define QUP_INPUT_FIFO			0x0218
> +
> +#define SPI_CONFIG			0x0300
> +#define SPI_IO_CONTROL			0x0304
> +#define SPI_ERROR_FLAGS			0x0308
> +#define SPI_ERROR_FLAGS_EN		0x030c
> +
> +/* QUP_CONFIG fields */
> +#define QUP_CONFIG_SPI_MODE		(1 << 8)
> +#define QUP_CONFIG_NO_INPUT		BIT(7)
> +#define QUP_CONFIG_NO_OUTPUT		BIT(6)
> +#define QUP_CONFIG_N			0x001f
> +
> +/* QUP_STATE fields */
> +#define QUP_STATE_VALID			BIT(2)
> +#define QUP_STATE_RESET			0
> +#define QUP_STATE_RUN			1
> +#define QUP_STATE_PAUSE			3
> +#define QUP_STATE_MASK			3
> +#define QUP_STATE_CLEAR			2
> +
> +#define QUP_HW_VERSION_2_1_1		0x20010001
> +
> +/* QUP_IO_M_MODES fields */
> +#define QUP_IO_M_PACK_EN		BIT(15)
> +#define QUP_IO_M_UNPACK_EN		BIT(14)
> +#define QUP_IO_M_INPUT_MODE_MASK_SHIFT	12
> +#define QUP_IO_M_OUTPUT_MODE_MASK_SHIFT	10
> +#define QUP_IO_M_INPUT_MODE_MASK	(3 << QUP_IO_M_INPUT_MODE_MASK_SHIFT)
> +#define QUP_IO_M_OUTPUT_MODE_MASK	(3 << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT)
> +
> +#define QUP_IO_M_OUTPUT_BLOCK_SIZE(x)	(((x) & (0x03 << 0)) >> 0)
> +#define QUP_IO_M_OUTPUT_FIFO_SIZE(x)	(((x) & (0x07 << 2)) >> 2)
> +#define QUP_IO_M_INPUT_BLOCK_SIZE(x)	(((x) & (0x03 << 5)) >> 5)
> +#define QUP_IO_M_INPUT_FIFO_SIZE(x)	(((x) & (0x07 << 7)) >> 7)
> +
> +#define QUP_IO_M_MODE_FIFO		0
> +#define QUP_IO_M_MODE_BLOCK		1
> +#define QUP_IO_M_MODE_DMOV		2
> +#define QUP_IO_M_MODE_BAM		3
> +
> +/* QUP_OPERATIONAL fields */
> +#define QUP_OP_MAX_INPUT_DONE_FLAG	BIT(11)
> +#define QUP_OP_MAX_OUTPUT_DONE_FLAG	BIT(10)
> +#define QUP_OP_IN_SERVICE_FLAG		BIT(9)
> +#define QUP_OP_OUT_SERVICE_FLAG		BIT(8)
> +#define QUP_OP_IN_FIFO_FULL		BIT(7)
> +#define QUP_OP_OUT_FIFO_FULL		BIT(6)
> +#define QUP_OP_IN_FIFO_NOT_EMPTY	BIT(5)
> +#define QUP_OP_OUT_FIFO_NOT_EMPTY	BIT(4)
> +
> +/* QUP_ERROR_FLAGS and QUP_ERROR_FLAGS_EN fields */
> +#define QUP_ERROR_OUTPUT_OVER_RUN	BIT(5)
> +#define QUP_ERROR_INPUT_UNDER_RUN	BIT(4)
> +#define QUP_ERROR_OUTPUT_UNDER_RUN	BIT(3)
> +#define QUP_ERROR_INPUT_OVER_RUN	BIT(2)
> +
> +/* SPI_CONFIG fields */
> +#define SPI_CONFIG_HS_MODE		BIT(10)
> +#define SPI_CONFIG_INPUT_FIRST		BIT(9)
> +#define SPI_CONFIG_LOOPBACK		BIT(8)
> +
> +/* SPI_IO_CONTROL fields */
> +#define SPI_IO_C_FORCE_CS		BIT(11)
> +#define SPI_IO_C_CLK_IDLE_HIGH		BIT(10)
> +#define SPI_IO_C_MX_CS_MODE		BIT(8)
> +#define SPI_IO_C_CS_N_POLARITY_0	BIT(4)
> +#define SPI_IO_C_CS_SELECT(x)		(((x) & 3) << 2)
> +#define SPI_IO_C_CS_SELECT_MASK		0x000c
> +#define SPI_IO_C_TRISTATE_CS		BIT(1)
> +#define SPI_IO_C_NO_TRI_STATE		BIT(0)
> +
> +/* SPI_ERROR_FLAGS and SPI_ERROR_FLAGS_EN fields */
> +#define SPI_ERROR_CLK_OVER_RUN		BIT(1)
> +#define SPI_ERROR_CLK_UNDER_RUN		BIT(0)
> +
> +#define SPI_NUM_CHIPSELECTS		4
> +
> +/* high speed mode is when bus rate is greater then 26MHz */
> +#define SPI_HS_MIN_RATE			26000000
> +
> +#define SPI_DELAY_THRESHOLD		1
> +#define SPI_DELAY_RETRY			10
> +
> +struct spi_qup_device {
> +	int bits_per_word;
> +	int chip_select;
> +	int speed_hz;
> +	u16 mode;
> +};
> +
> +struct spi_qup {
> +	void __iomem		*base;
> +	struct device		*dev;
> +	struct clk		*cclk;	/* core clock */
> +	struct clk		*iclk;	/* interface clock */
> +	int			irq;
> +	u32			max_speed_hz;
> +	u32			speed_hz;
> +
> +	int			in_fifo_sz;
> +	int			out_fifo_sz;
> +	int			in_blk_sz;
> +	int			out_blk_sz;
> +
> +	struct spi_transfer	*xfer;
> +	struct completion	done;
> +	int			error;
> +	int			bytes_per_word;
> +	int			tx_bytes;
> +	int			rx_bytes;
> +};
> +
> +
> +static inline bool spi_qup_is_valid_state(struct spi_qup *controller)
> +{
> +	u32 opstate = readl_relaxed(controller->base + QUP_STATE);
> +
> +	return opstate & QUP_STATE_VALID;
> +}
> +
> +static int spi_qup_set_state(struct spi_qup *controller, u32 state)
> +{
> +	unsigned long loop = 0;
> +	u32 cur_state;
> +
> +	cur_state = readl_relaxed(controller->base + QUP_STATE);
> +	/*
> +	 * Per spec: for PAUSE_STATE to RESET_STATE, two writes
> +	 * of (b10) are required
> +	 */
> +	if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
> +	    (state == QUP_STATE_RESET)) {
> +		writel_relaxed(QUP_STATE_CLEAR, controller->base + QUP_STATE);
> +		writel_relaxed(QUP_STATE_CLEAR, controller->base + QUP_STATE);
> +	} else {
> +		cur_state &= ~QUP_STATE_MASK;
> +		cur_state |= state;
> +		writel_relaxed(cur_state, controller->base + QUP_STATE);
> +	}
> +
> +	while (!spi_qup_is_valid_state(controller)) {
> +
> +		usleep_range(SPI_DELAY_THRESHOLD, SPI_DELAY_THRESHOLD * 2);
> +
> +		if (++loop > SPI_DELAY_RETRY)
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void spi_qup_deassert_cs(struct spi_qup *controller,
> +				struct spi_qup_device *chip)
> +{
> +	u32 iocontol, mask;
> +
> +	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
> +
> +	/* Disable auto CS toggle and use manual */
> +	iocontol &= ~SPI_IO_C_MX_CS_MODE;
> +	iocontol |= SPI_IO_C_FORCE_CS;
> +
> +	iocontol &= ~SPI_IO_C_CS_SELECT_MASK;
> +	iocontol |= SPI_IO_C_CS_SELECT(chip->chip_select);
> +
> +	mask = SPI_IO_C_CS_N_POLARITY_0 << chip->chip_select;
> +
> +	if (chip->mode & SPI_CS_HIGH)
> +		iocontol &= ~mask;
> +	else
> +		iocontol |= mask;
> +
> +	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
> +}
> +
> +static void spi_qup_assert_cs(struct spi_qup *controller,
> +			      struct spi_qup_device *chip)
> +{
> +	u32 iocontol, mask;
> +
> +	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
> +
> +	/* Disable auto CS toggle and use manual */
> +	iocontol &= ~SPI_IO_C_MX_CS_MODE;
> +	iocontol |= SPI_IO_C_FORCE_CS;
> +
> +	iocontol &= ~SPI_IO_C_CS_SELECT_MASK;
> +	iocontol |= SPI_IO_C_CS_SELECT(chip->chip_select);
> +
> +	mask = SPI_IO_C_CS_N_POLARITY_0 << chip->chip_select;
> +
> +	if (chip->mode & SPI_CS_HIGH)
> +		iocontol |= mask;
> +	else
> +		iocontol &= ~mask;
> +
> +	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
> +}
> +
> +static void spi_qup_fifo_read(struct spi_qup *controller,
> +			      struct spi_transfer *xfer)
> +{
> +	u8 *rx_buf = xfer->rx_buf;
> +	u32 word, state;
> +	int idx, shift;
> +
> +	while (controller->rx_bytes < xfer->len) {
> +
> +		state = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +		if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
> +			break;
> +
> +		word = readl_relaxed(controller->base + QUP_INPUT_FIFO);
> +
> +		for (idx = 0; idx < controller->bytes_per_word &&
> +		     controller->rx_bytes < xfer->len; idx++,
> +		     controller->rx_bytes++) {
> +
> +			if (!rx_buf)
> +				continue;
> +			/*
> +			 * The data format depends on bytes_per_word:
> +			 *  4 bytes: 0x12345678
> +			 *  2 bytes: 0x00001234
> +			 *  1 byte : 0x00000012
> +			 */
> +			shift = BITS_PER_BYTE;
> +			shift *= (controller->bytes_per_word - idx - 1);
> +			rx_buf[controller->rx_bytes] = word >> shift;
> +		}
> +	}
> +}
> +
> +static void spi_qup_fifo_write(struct spi_qup *controller,
> +			       struct spi_transfer *xfer)
> +{
> +	const u8 *tx_buf = xfer->tx_buf;
> +	u32 word, state, data;
> +	int idx;
> +
> +	while (controller->tx_bytes < xfer->len) {
> +
> +		state = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +		if (state & QUP_OP_OUT_FIFO_FULL)
> +			break;
> +
> +		word = 0;
> +		for (idx = 0; idx < controller->bytes_per_word &&
> +		     controller->tx_bytes < xfer->len; idx++,
> +		     controller->tx_bytes++) {
> +
> +			if (!tx_buf)
> +				continue;
> +
> +			data = tx_buf[controller->tx_bytes];
> +			word |= data << (BITS_PER_BYTE * (3 - idx));
> +		}
> +
> +		writel_relaxed(word, controller->base + QUP_OUTPUT_FIFO);
> +	}
> +}
> +
> +static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
> +{
> +	struct spi_qup *controller = dev_id;
> +	struct spi_transfer *xfer;
> +	u32 opflags, qup_err, spi_err;
> +
> +	xfer = controller->xfer;
> +
> +	qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> +	spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> +	opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +
> +	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> +	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> +	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> +
> +	if (!xfer)
> +		return IRQ_HANDLED;
> +
> +	if (qup_err) {
> +		if (qup_err & QUP_ERROR_OUTPUT_OVER_RUN)
> +			dev_warn(controller->dev, "OUTPUT_OVER_RUN\n");
> +		if (qup_err & QUP_ERROR_INPUT_UNDER_RUN)
> +			dev_warn(controller->dev, "INPUT_UNDER_RUN\n");
> +		if (qup_err & QUP_ERROR_OUTPUT_UNDER_RUN)
> +			dev_warn(controller->dev, "OUTPUT_UNDER_RUN\n");
> +		if (qup_err & QUP_ERROR_INPUT_OVER_RUN)
> +			dev_warn(controller->dev, "INPUT_OVER_RUN\n");
> +
> +		controller->error = -EIO;
> +	}
> +
> +	if (spi_err) {
> +		if (spi_err & SPI_ERROR_CLK_OVER_RUN)
> +			dev_warn(controller->dev, "CLK_OVER_RUN\n");
> +		if (spi_err & SPI_ERROR_CLK_UNDER_RUN)
> +			dev_warn(controller->dev, "CLK_UNDER_RUN\n");
> +
> +		controller->error = -EIO;
> +	}
> +
> +	if (opflags & QUP_OP_IN_SERVICE_FLAG) {
> +		writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
> +		       controller->base + QUP_OPERATIONAL);
> +		spi_qup_fifo_read(controller, xfer);
> +	}
> +
> +	if (opflags & QUP_OP_OUT_SERVICE_FLAG) {
> +		writel_relaxed(QUP_OP_OUT_SERVICE_FLAG,
> +		       controller->base + QUP_OPERATIONAL);
> +		spi_qup_fifo_write(controller, xfer);
> +	}
> +
> +	if (controller->rx_bytes == xfer->len ||
> +	    controller->error)
> +		complete(&controller->done);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int spi_qup_transfer_do(struct spi_qup *controller,
> +			       struct spi_qup_device *chip,
> +			       struct spi_transfer *xfer)
> +{
> +	unsigned long timeout;
> +	int ret = -EIO;
> +
> +	reinit_completion(&controller->done);
> +
> +	timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
> +	timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
> +	timeout = 100 * msecs_to_jiffies(timeout);
> +
> +	controller->rx_bytes = 0;
> +	controller->tx_bytes = 0;
> +	controller->error = 0;
> +	controller->xfer = xfer;
> +
> +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> +		dev_warn(controller->dev, "cannot set RUN state\n");
> +		goto exit;
> +	}
> +
> +	if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
> +		dev_warn(controller->dev, "cannot set PAUSE state\n");
> +		goto exit;
> +	}
> +
> +	spi_qup_fifo_write(controller, xfer);
> +
> +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> +		dev_warn(controller->dev, "cannot set EXECUTE state\n");
> +		goto exit;
> +	}
> +
> +	if (!wait_for_completion_timeout(&controller->done, timeout))
> +		ret = -ETIMEDOUT;
> +	else
> +		ret = controller->error;
> +exit:
> +	controller->xfer = NULL;

Should the manipulation of controller->xfer be protected by spinlock?

> +	controller->error = 0;
> +	controller->rx_bytes = 0;
> +	controller->tx_bytes = 0;
> +	spi_qup_set_state(controller, QUP_STATE_RESET);
> +	return ret;
> +}
> +
> +static int spi_qup_setup(struct spi_device *spi)
> +{
> +	struct spi_qup *controller = spi_master_get_devdata(spi->master);
> +	struct spi_qup_device *chip = spi_get_ctldata(spi);
> +
> +	if (spi->chip_select >= spi->master->num_chipselect) {
> +		dev_err(controller->dev, "invalid chip_select %d\n",
> +			spi->chip_select);
> +		return -EINVAL;
> +	}
> +
> +	if (spi->max_speed_hz > controller->max_speed_hz) {
> +		dev_err(controller->dev, "invalid max_speed_hz %d\n",
> +			spi->max_speed_hz);
> +		return -EINVAL;
> +	}
> +
> +	if (!chip) {
> +		/* First setup */
> +		chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +		if (!chip) {
> +			dev_err(controller->dev, "no memory for chip data\n");
> +			return -ENOMEM;
> +		}
> +
> +		spi_set_ctldata(spi, chip);
> +	}
> +
> +	return 0;
> +}
> +
> +static void spi_qup_cleanup(struct spi_device *spi)
> +{
> +	struct spi_qup_device *chip = spi_get_ctldata(spi);
> +
> +	if (!chip)
> +		return;
> +
> +	spi_set_ctldata(spi, NULL);
> +	kfree(chip);
> +}
> +
> +/* set clock freq, clock ramp, bits per work */
> +static int spi_qup_io_setup(struct spi_device *spi,
> +			  struct spi_transfer *xfer)
> +{
> +	struct spi_qup *controller = spi_master_get_devdata(spi->master);
> +	struct spi_qup_device *chip = spi_get_ctldata(spi);
> +	u32 iocontol, config, iomode, mode;
> +	int ret, n_words;
> +
> +	if (spi->mode & SPI_LOOP && xfer->len > controller->in_fifo_sz) {
> +		dev_err(controller->dev, "too big size for loopback %d > %d\n",
> +			xfer->len, controller->in_fifo_sz);
> +		return -EIO;
> +	}
> +
> +	chip->mode = spi->mode;
> +	chip->speed_hz = spi->max_speed_hz;
> +	if (xfer->speed_hz)
> +		chip->speed_hz = xfer->speed_hz;
> +
> +	if (controller->speed_hz != chip->speed_hz) {
> +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> +		if (ret) {
> +			dev_err(controller->dev, "fail to set frequency %d",
> +				chip->speed_hz);
> +			return -EIO;
> +		}
> +	}
> +
> +	controller->speed_hz = chip->speed_hz;
> +
> +	chip->bits_per_word = spi->bits_per_word;
> +	if (xfer->bits_per_word)
> +		chip->bits_per_word = xfer->bits_per_word;
> +
> +	if (chip->bits_per_word <= 8)
> +		controller->bytes_per_word = 1;
> +	else if (chip->bits_per_word <= 16)
> +		controller->bytes_per_word = 2;
> +	else
> +		controller->bytes_per_word = 4;
> +
> +	if (controller->bytes_per_word > xfer->len ||
> +	    xfer->len % controller->bytes_per_word != 0){
> +		/* No partial transfers */
> +		dev_err(controller->dev, "invalid len %d for %d bits\n",
> +			xfer->len, chip->bits_per_word);
> +		return -EIO;
> +	}
> +
> +	n_words = xfer->len / controller->bytes_per_word;
> +
> +	if (spi_qup_set_state(controller, QUP_STATE_RESET)) {
> +		dev_err(controller->dev, "cannot set RESET state\n");
> +		return -EIO;
> +	}
> +
> +	if (n_words <= controller->in_fifo_sz) {
> +		mode = QUP_IO_M_MODE_FIFO;
> +		writel_relaxed(n_words, controller->base + QUP_MX_READ_CNT);
> +		writel_relaxed(n_words, controller->base + QUP_MX_WRITE_CNT);
> +		/* must be zero for FIFO */
> +		writel_relaxed(0, controller->base + QUP_MX_INPUT_CNT);
> +		writel_relaxed(0, controller->base + QUP_MX_OUTPUT_CNT);
> +	} else {
> +		mode = QUP_IO_M_MODE_BLOCK;
> +		writel_relaxed(n_words, controller->base + QUP_MX_INPUT_CNT);
> +		writel_relaxed(n_words, controller->base + QUP_MX_OUTPUT_CNT);
> +		/* must be zero for BLOCK and BAM */
> +		writel_relaxed(0, controller->base + QUP_MX_READ_CNT);
> +		writel_relaxed(0, controller->base + QUP_MX_WRITE_CNT);
> +	}
> +
> +	iomode = readl_relaxed(controller->base + QUP_IO_M_MODES);
> +	/* Set input and output transfer mode */
> +	iomode &= ~(QUP_IO_M_INPUT_MODE_MASK | QUP_IO_M_OUTPUT_MODE_MASK);
> +	iomode &= ~(QUP_IO_M_PACK_EN | QUP_IO_M_UNPACK_EN);
> +	iomode |= (mode << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT);
> +	iomode |= (mode << QUP_IO_M_INPUT_MODE_MASK_SHIFT);
> +
> +	writel_relaxed(iomode, controller->base + QUP_IO_M_MODES);
> +
> +	config = readl_relaxed(controller->base + SPI_CONFIG);
> +
> +	if (chip->mode & SPI_LOOP)
> +		config |= SPI_CONFIG_LOOPBACK;
> +	else
> +		config &= ~SPI_CONFIG_LOOPBACK;
> +
> +	if (chip->mode & SPI_CPHA)
> +		config &= ~SPI_CONFIG_INPUT_FIRST;
> +	else
> +		config |= SPI_CONFIG_INPUT_FIRST;
> +
> +	/*
> +	 * HS_MODE improves signal stability for spi-clk high rates
> +	 * but is invalid in loop back mode.
> +	 */
> +	if ((controller->speed_hz >= SPI_HS_MIN_RATE) &&
> +	    !(chip->mode & SPI_LOOP))
> +		config |= SPI_CONFIG_HS_MODE;
> +	else
> +		config &= ~SPI_CONFIG_HS_MODE;
> +
> +	writel_relaxed(config, controller->base + SPI_CONFIG);
> +
> +	config = readl_relaxed(controller->base + QUP_CONFIG);
> +	config &= ~(QUP_CONFIG_NO_INPUT | QUP_CONFIG_NO_OUTPUT | QUP_CONFIG_N);
> +	config |= chip->bits_per_word - 1;
> +	config |= QUP_CONFIG_SPI_MODE;
> +	writel_relaxed(config, controller->base + QUP_CONFIG);
> +
> +	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
> +
> +	/* Disable auto CS toggle */
> +	iocontol &= ~SPI_IO_C_MX_CS_MODE;
> +
> +	if (chip->mode & SPI_CPOL)
> +		iocontol |= SPI_IO_C_CLK_IDLE_HIGH;
> +	else
> +		iocontol &= ~SPI_IO_C_CLK_IDLE_HIGH;
> +
> +	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
> +
> +	/*
> +	 * TODO: In BAM mode mask INPUT and OUTPUT service flags in
> +	 * to prevent IRQs on FIFO status change.
> +	 */

Remove the TODO.  Not necessary.  This stuff can be added when it becomes BAM
enabled.

> +	writel_relaxed(0, controller->base + QUP_OPERATIONAL_MASK);
> +
> +	return 0;
> +}
> +
> +static int spi_qup_transfer_one(struct spi_master *master,
> +				struct spi_message *msg)
> +{
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +	struct spi_qup_device *chip = spi_get_ctldata(msg->spi);
> +	struct spi_transfer *xfer;
> +	struct spi_device *spi;
> +	unsigned cs_change;
> +	int status;
> +
> +	spi = msg->spi;
> +	cs_change = 1;
> +	status = 0;
> +
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +
> +		status = spi_qup_io_setup(spi, xfer);
> +		if (status)
> +			break;
> +

no locking?  This whole code block needs to have some type of mutex_lock to keep
others from trouncing the hardware while you are doing this transfer.

> +		if (cs_change)
> +			spi_qup_assert_cs(controller, chip);

Should the CS be done outside the loop?  I'd expect the following sequence to
happen:
- change CS
- Loop and do some transfers
- deassert CS

In this code, you reinitialize and assert/deassert CS for every transaction.

> +
> +		cs_change = xfer->cs_change;
> +
> +		/* Do actual transfer */
> +		status = spi_qup_transfer_do(controller, chip, xfer);
> +		if (status)
> +			break;
> +
> +		msg->actual_length += xfer->len;
> +
> +		if (xfer->delay_usecs)
> +			udelay(xfer->delay_usecs);
> +
> +		if (cs_change)
> +			spi_qup_deassert_cs(controller, chip);
> +	}
> +
> +	if (status || !cs_change)
> +		spi_qup_deassert_cs(controller, chip);
> +
> +	msg->status = status;
> +	spi_finalize_current_message(master);
> +	return status;
> +}
> +
> +static int spi_qup_probe(struct platform_device *pdev)
> +{
> +	struct spi_master *master;
> +	struct clk *iclk, *cclk;
> +	struct spi_qup *controller;
> +	struct resource *res;
> +	struct device *dev;
> +	void __iomem *base;
> +	u32 data, max_freq, iomode;
> +	int ret, irq, size;
> +
> +	dev = &pdev->dev;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +
> +	if (irq < 0)
> +		return irq;
> +
> +	cclk = devm_clk_get(dev, "core");
> +	if (IS_ERR(cclk)) {
> +		dev_err(dev, "cannot get core clock\n");
No need to error print.  devm_clk_get already outputs something
> +		return PTR_ERR(cclk);
> +	}
> +
> +	iclk = devm_clk_get(dev, "iface");
> +	if (IS_ERR(iclk)) {
> +		dev_err(dev, "cannot get iface clock\n");

No need to error print.  devm_clk_get already outputs something

> +		return PTR_ERR(iclk);
> +	}
> +
> +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> +		max_freq = 19200000;

I'd set the default to 50MHz as that is the max supported by hardware.  I'd just
set max_freq declaration to 50MHz and then check the value if it is changed via
DT.

> +
> +	if (!max_freq) {
> +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> +		return -ENXIO;
> +	}

This is buggy.  Remove this and collapse into the of_property_read_u32 if
statement.  On non-zero, check the range for validity.

> +
> +	ret = clk_set_rate(cclk, max_freq);
> +	if (ret)
> +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);

Bail here?

> +
> +	ret = clk_prepare_enable(cclk);
> +	if (ret) {
> +		dev_err(dev, "cannot enable core clock\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(iclk);
> +	if (ret) {
> +		clk_disable_unprepare(cclk);
> +		dev_err(dev, "cannot enable iface clock\n");
> +		return ret;
> +	}
> +
> +	data = readl_relaxed(base + QUP_HW_VERSION);
> +
> +	if (data < QUP_HW_VERSION_2_1_1) {
> +		clk_disable_unprepare(cclk);
> +		clk_disable_unprepare(iclk);
> +		dev_err(dev, "v.%08x is not supported\n", data);
> +		return -ENXIO;
> +	}
> +
> +	master = spi_alloc_master(dev, sizeof(struct spi_qup));
> +	if (!master) {
> +		clk_disable_unprepare(cclk);
> +		clk_disable_unprepare(iclk);
> +		dev_err(dev, "cannot allocate master\n");
> +		return -ENOMEM;
> +	}
> +
> +	master->bus_num = pdev->id;
> +	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
> +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> +	master->setup = spi_qup_setup;
> +	master->cleanup = spi_qup_cleanup;
> +	master->transfer_one_message = spi_qup_transfer_one;
> +	master->dev.of_node = pdev->dev.of_node;
> +	master->auto_runtime_pm = true;

Remove this.  No runtime support

> +
> +	platform_set_drvdata(pdev, master);
> +
> +	controller = spi_master_get_devdata(master);
> +
> +	controller->dev  = dev;
> +	controller->base = base;
> +	controller->iclk = iclk;
> +	controller->cclk = cclk;
> +	controller->irq  = irq;
> +	controller->max_speed_hz = clk_get_rate(cclk);
> +	controller->speed_hz = controller->max_speed_hz;
> +
> +	init_completion(&controller->done);
> +
> +	iomode = readl_relaxed(base + QUP_IO_M_MODES);
> +
> +	size = QUP_IO_M_OUTPUT_BLOCK_SIZE(iomode);
> +	if (size)
> +		controller->out_blk_sz = size * 16;
> +	else
> +		controller->out_blk_sz = 4;
> +
> +	size = QUP_IO_M_INPUT_BLOCK_SIZE(iomode);
> +	if (size)
> +		controller->in_blk_sz = size * 16;
> +	else
> +		controller->in_blk_sz = 4;
> +
> +	size = QUP_IO_M_OUTPUT_FIFO_SIZE(iomode);
> +	controller->out_fifo_sz = controller->out_blk_sz * (2 << size);
> +
> +	size = QUP_IO_M_INPUT_FIFO_SIZE(iomode);
> +	controller->in_fifo_sz = controller->in_blk_sz * (2 << size);
> +
> +	dev_info(dev, "v.%08x IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
> +		 data, controller->in_blk_sz, controller->in_fifo_sz,
> +		 controller->out_blk_sz, controller->out_fifo_sz);
> +
> +	writel_relaxed(1, base + QUP_SW_RESET);
> +
> +	ret = spi_qup_set_state(controller, QUP_STATE_RESET);
> +	if (ret) {
> +		dev_err(dev, "cannot set RESET state\n");
> +		goto error;
> +	}
> +
> +	writel_relaxed(0, base + QUP_OPERATIONAL);
> +	writel_relaxed(0, base + QUP_IO_M_MODES);
> +	writel_relaxed(0, base + QUP_OPERATIONAL_MASK);
> +	writel_relaxed(SPI_ERROR_CLK_UNDER_RUN | SPI_ERROR_CLK_OVER_RUN,
> +		       base + SPI_ERROR_FLAGS_EN);
> +
> +	writel_relaxed(0, base + SPI_CONFIG);
> +	writel_relaxed(SPI_IO_C_NO_TRI_STATE, base + SPI_IO_CONTROL);
> +
> +	ret = devm_request_irq(dev, irq, spi_qup_qup_irq,
> +			       IRQF_TRIGGER_HIGH, pdev->name, controller);
> +	if (ret) {
> +		dev_err(dev, "cannot request IRQ %d\n", irq);

unnecessary print

> +		goto error;
> +	}
> +
> +	ret = devm_spi_register_master(dev, master);
> +	if (!ret) {
> +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> +		pm_runtime_use_autosuspend(dev);
> +		pm_runtime_set_active(dev);
> +		pm_runtime_enable(dev);

Remove all the runtime stuff.  not supported right now.

> +		return ret;
> +	}
> +error:
> +	clk_disable_unprepare(cclk);
> +	clk_disable_unprepare(iclk);
> +	spi_master_put(master);
> +	return ret;
> +}
> +
> +#ifdef CONFIG_PM_RUNTIME

Remove all the runtime stuff.  not supported right now.

> +static int spi_qup_pm_suspend_runtime(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	disable_irq(controller->irq);
> +	clk_disable_unprepare(controller->cclk);
> +	clk_disable_unprepare(controller->iclk);
> +	dev_dbg(device, "suspend runtime\n");
> +	return 0;
> +}
> +
> +static int spi_qup_pm_resume_runtime(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	clk_prepare_enable(controller->cclk);
> +	clk_prepare_enable(controller->iclk);
> +	enable_irq(controller->irq);
> +	dev_dbg(device, "resume runtime\n");
> +	return 0;
> +}
> +#endif /* CONFIG_PM_RUNTIME */
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int spi_qup_suspend(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +	int status;
> +
> +	status = spi_master_suspend(master);
> +	if (!status) {
> +		disable_irq(controller->irq);
> +		clk_disable_unprepare(controller->cclk);
> +		clk_disable_unprepare(controller->iclk);
> +	}
> +
> +	dev_dbg(device, "system suspend %d\n", status);
> +	return status;
> +}

Remove all the suspend/resume stuff.  not supported right now.

> +
> +static int spi_qup_resume(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +	int status;
> +
> +	clk_prepare_enable(controller->cclk);
> +	clk_prepare_enable(controller->iclk);
> +
> +	status = spi_master_resume(master);
> +
> +	dev_dbg(device, "system resume %d\n", status);
> +	return status;
> +}
> +#endif /* CONFIG_PM_SLEEP */

Remove all the suspend/resume stuff.  not supported right now.

> +
> +static int spi_qup_remove(struct platform_device *pdev)
> +{
> +	struct spi_master *master = dev_get_drvdata(&pdev->dev);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	pm_runtime_get_sync(&pdev->dev);
> +

Do we need to wait for any current transactions to complete
and do a devm_free_irq()?

> +	clk_disable_unprepare(controller->cclk);
> +	clk_disable_unprepare(controller->iclk);
> +
> +	pm_runtime_put_noidle(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	return 0;
> +}
> +
> +static struct of_device_id spi_qup_dt_match[] = {
> +	{ .compatible = "qcom,spi-qup-v2", },

Need compatible tags of qcom,spi-qup-v2.1.1 (msm8974 v1) or qcom,spi-qup-v2.2.1
(msm8974 v2)

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, spi_qup_dt_match);
> +
> +static const struct dev_pm_ops spi_qup_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(spi_qup_suspend, spi_qup_resume)
> +	SET_RUNTIME_PM_OPS(spi_qup_pm_suspend_runtime,
> +			   spi_qup_pm_resume_runtime,
> +			   NULL)
> +};

Remove this for now.

> +
> +static struct platform_driver spi_qup_driver = {
> +	.driver = {
> +		.name		= "spi_qup",
> +		.owner		= THIS_MODULE,
> +		.pm		= &spi_qup_dev_pm_ops,
> +		.of_match_table = spi_qup_dt_match,
> +	},
> +	.probe = spi_qup_probe,
> +	.remove = spi_qup_remove,
> +};
> +module_platform_driver(spi_qup_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION("0.4");
> +MODULE_ALIAS("platform:spi_qup");
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivan T. Ivanov Feb. 7, 2014, 9:52 a.m. UTC | #2

Hi Andy,

On Fri, 2014-02-07 at 01:39 -0600, Andy Gross wrote: 
> On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <[e-mail]>
> > 
> > Qualcomm Universal Peripheral (QUP) core is an 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
> > support up to 50MHz, up to four chip selects, and a programmable
> > data path from 4 bits to 32 bits; MODE0..3 protocols
> > 
> > Signed-off-by: Ivan T. Ivanov <[e-mail]>
> > Cc: Alok Chauhan <[e-mail]>
> > Cc: Gilad Avidov <[e-mail]>
> > Cc: Kiran Gunda <[e-mail]>
> > Cc: Sagar Dharia <[e-mail]>
> > ---
> >  drivers/spi/Kconfig   |   14 +
> >  drivers/spi/Makefile  |    1 +
> >  drivers/spi/spi-qup.c |  898 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 913 insertions(+)
> >  create mode 100644 drivers/spi/spi-qup.c
> > 
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index ba9310b..bf8ce6b 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -381,6 +381,20 @@ config SPI_RSPI
> >  	help
> >  	  SPI driver for Renesas RSPI blocks.
> >  
> > +config SPI_QUP
> > +	tristate "Qualcomm SPI Support with QUP interface"
> > +	depends on ARCH_MSM
> 
> I'd change to ARCH_MSM_DT.  This ensures the OF component is there.

Ok. will change.

> 
> > +	help
> > +	  Qualcomm Universal Peripheral (QUP) core is an 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 support up to 50MHz, up to four chip selects, and a
> > +	  programmable data path from 4 bits to 32 bits; supports numerous
> > +	  protocol variants.
> > +
> > +	  This driver can also be built as a module.  If so, the module
> > +	  will be called spi_qup.
> > +
> >  config SPI_S3C24XX
> >  	tristate "Samsung S3C24XX series SPI"
> >  	depends on ARCH_S3C24XX
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index 95af48d..e598147 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -59,6 +59,7 @@ spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_PXADMA)	+= spi-pxa2xx-pxadma.o
> >  spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_DMA)	+= spi-pxa2xx-dma.o
> >  obj-$(CONFIG_SPI_PXA2XX)		+= spi-pxa2xx-platform.o
> >  obj-$(CONFIG_SPI_PXA2XX_PCI)		+= spi-pxa2xx-pci.o
> > +obj-$(CONFIG_SPI_QUP)			+= spi-qup.o
> >  obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
> >  obj-$(CONFIG_SPI_S3C24XX)		+= spi-s3c24xx-hw.o
> >  spi-s3c24xx-hw-y			:= spi-s3c24xx.o
> > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> > new file mode 100644
> > index 0000000..5eb5e8f
> > --- /dev/null
> > +++ b/drivers/spi/spi-qup.c
> > @@ -0,0 +1,898 @@
> > +/*
> > + * Copyright (c) 2008-2014, The Linux foundation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License rev 2 and
> > + * only rev 2 as published by the free Software foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or fITNESS fOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> 
> Remove this for now.  No runtime support.

Did you see any particular issue with the implementation 
or this is just because this platform didn't have support 
for power management?  

> 
> > +#include <linux/spi/spi.h>
> > +

<snip>

> > +
> > +static int spi_qup_transfer_do(struct spi_qup *controller,
> > +			       struct spi_qup_device *chip,
> > +			       struct spi_transfer *xfer)
> > +{
> > +	unsigned long timeout;
> > +	int ret = -EIO;
> > +
> > +	reinit_completion(&controller->done);
> > +
> > +	timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
> > +	timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
> > +	timeout = 100 * msecs_to_jiffies(timeout);
> > +
> > +	controller->rx_bytes = 0;
> > +	controller->tx_bytes = 0;
> > +	controller->error = 0;
> > +	controller->xfer = xfer;
> > +
> > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > +		dev_warn(controller->dev, "cannot set RUN state\n");
> > +		goto exit;
> > +	}
> > +
> > +	if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
> > +		dev_warn(controller->dev, "cannot set PAUSE state\n");
> > +		goto exit;
> > +	}
> > +
> > +	spi_qup_fifo_write(controller, xfer);
> > +
> > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > +		dev_warn(controller->dev, "cannot set EXECUTE state\n");
> > +		goto exit;
> > +	}
> > +
> > +	if (!wait_for_completion_timeout(&controller->done, timeout))
> > +		ret = -ETIMEDOUT;
> > +	else
> > +		ret = controller->error;
> > +exit:
> > +	controller->xfer = NULL;
> 
> Should the manipulation of controller->xfer be protected by spinlock?

:-). Probably. I am wondering, could I avoid locking if firstly place
QUP into RESET state and then access these field. This should stop
all activities in it, right?
> 
> > +	controller->error = 0;
> > +	controller->rx_bytes = 0;
> > +	controller->tx_bytes = 0;
> > +	spi_qup_set_state(controller, QUP_STATE_RESET);
> > +	return ret;
> > +}
> > +

<snip>

> > +
> > +/* set clock freq, clock ramp, bits per work */
> > +static int spi_qup_io_setup(struct spi_device *spi,
> > +			  struct spi_transfer *xfer)
> > +{

<snip>

> > +
> > +	/*
> > +	 * TODO: In BAM mode mask INPUT and OUTPUT service flags in
> > +	 * to prevent IRQs on FIFO status change.
> > +	 */
> 
> Remove the TODO.  Not necessary.  This stuff can be added when it becomes BAM
> enabled.

Ok. 

> 
> > +	writel_relaxed(0, controller->base + QUP_OPERATIONAL_MASK);
> > +
> > +	return 0;
> > +}
> > +
> > +static int spi_qup_transfer_one(struct spi_master *master,
> > +				struct spi_message *msg)
> > +{
> > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > +	struct spi_qup_device *chip = spi_get_ctldata(msg->spi);
> > +	struct spi_transfer *xfer;
> > +	struct spi_device *spi;
> > +	unsigned cs_change;
> > +	int status;
> > +
> > +	spi = msg->spi;
> > +	cs_change = 1;
> > +	status = 0;
> > +
> > +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> > +
> > +		status = spi_qup_io_setup(spi, xfer);
> > +		if (status)
> > +			break;
> > +
> 
> no locking?  This whole code block needs to have some type of mutex_lock to keep
> others from trouncing the hardware while you are doing this transfer.

This is handled by SPI framework.

> 
> > +		if (cs_change)
> > +			spi_qup_assert_cs(controller, chip);
> 
> Should the CS be done outside the loop?  I'd expect the following sequence to
> happen:
> - change CS
> - Loop and do some transfers
> - deassert CS
> 
> In this code, you reinitialize and assert/deassert CS for every transaction.
> 
> > +
> > +		cs_change = xfer->cs_change;


Not exactly. It is allowed that CS goes inactive after every
transaction. This is how I read struct spi_transfer description.

> > +
> > +		/* Do actual transfer */
> > +		status = spi_qup_transfer_do(controller, chip, xfer);
> > +		if (status)
> > +			break;
> > +
> > +		msg->actual_length += xfer->len;
> > +
> > +		if (xfer->delay_usecs)
> > +			udelay(xfer->delay_usecs);
> > +
> > +		if (cs_change)
> > +			spi_qup_deassert_cs(controller, chip);
> > +	}
> > +
> > +	if (status || !cs_change)
> > +		spi_qup_deassert_cs(controller, chip);
> > +
> > +	msg->status = status;
> > +	spi_finalize_current_message(master);
> > +	return status;
> > +}
> > +
> > +static int spi_qup_probe(struct platform_device *pdev)
> > +{
> > +	struct spi_master *master;
> > +	struct clk *iclk, *cclk;
> > +	struct spi_qup *controller;
> > +	struct resource *res;
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	u32 data, max_freq, iomode;
> > +	int ret, irq, size;
> > +
> > +	dev = &pdev->dev;
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	cclk = devm_clk_get(dev, "core");
> > +	if (IS_ERR(cclk)) {
> > +		dev_err(dev, "cannot get core clock\n");
> No need to error print.  devm_clk_get already outputs something

Ok.

> > +		return PTR_ERR(cclk);
> > +	}
> > +
> > +	iclk = devm_clk_get(dev, "iface");
> > +	if (IS_ERR(iclk)) {
> > +		dev_err(dev, "cannot get iface clock\n");
> 
> No need to error print.  devm_clk_get already outputs something

Ok.

> 
> > +		return PTR_ERR(iclk);
> > +	}
> > +
> > +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> > +		max_freq = 19200000;
> 
> I'd set the default to 50MHz as that is the max supported by hardware.  I'd just
> set max_freq declaration to 50MHz and then check the value if it is changed via
> DT.

50MHz doesn't seems to be supported on all chip sets. Currently common
denominator on all chip sets, that I can see, is 19.2MHz. I have tried 
to test it with more than 19.2MHz on APQ8074 and it fails.

> 
> > +
> > +	if (!max_freq) {
> > +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> > +		return -ENXIO;
> > +	}
> 
> This is buggy.  Remove this and collapse into the of_property_read_u32 if
> statement.  On non-zero, check the range for validity.

True. Will fix.

> 
> > +
> > +	ret = clk_set_rate(cclk, max_freq);
> > +	if (ret)
> > +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> 
> Bail here?

I don't know. What will be the consequences if controller continue to
operate on its default rate?

> 
> > +
> > +	ret = clk_prepare_enable(cclk);
> > +	if (ret) {
> > +		dev_err(dev, "cannot enable core clock\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(iclk);
> > +	if (ret) {
> > +		clk_disable_unprepare(cclk);
> > +		dev_err(dev, "cannot enable iface clock\n");
> > +		return ret;
> > +	}
> > +
> > +	data = readl_relaxed(base + QUP_HW_VERSION);
> > +
> > +	if (data < QUP_HW_VERSION_2_1_1) {
> > +		clk_disable_unprepare(cclk);
> > +		clk_disable_unprepare(iclk);
> > +		dev_err(dev, "v.%08x is not supported\n", data);
> > +		return -ENXIO;
> > +	}
> > +
> > +	master = spi_alloc_master(dev, sizeof(struct spi_qup));
> > +	if (!master) {
> > +		clk_disable_unprepare(cclk);
> > +		clk_disable_unprepare(iclk);
> > +		dev_err(dev, "cannot allocate master\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	master->bus_num = pdev->id;
> > +	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
> > +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> > +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> > +	master->setup = spi_qup_setup;
> > +	master->cleanup = spi_qup_cleanup;
> > +	master->transfer_one_message = spi_qup_transfer_one;
> > +	master->dev.of_node = pdev->dev.of_node;
> > +	master->auto_runtime_pm = true;
> 
> Remove this.  No runtime support
> 
> > +
> > +	platform_set_drvdata(pdev, master);
> > +
> > +	controller = spi_master_get_devdata(master);
> > +
> > +	controller->dev  = dev;
> > +	controller->base = base;
> > +	controller->iclk = iclk;
> > +	controller->cclk = cclk;
> > +	controller->irq  = irq;
> > +	controller->max_speed_hz = clk_get_rate(cclk);
> > +	controller->speed_hz = controller->max_speed_hz;
> > +
> > +	init_completion(&controller->done);
> > +
> > +	iomode = readl_relaxed(base + QUP_IO_M_MODES);
> > +
> > +	size = QUP_IO_M_OUTPUT_BLOCK_SIZE(iomode);
> > +	if (size)
> > +		controller->out_blk_sz = size * 16;
> > +	else
> > +		controller->out_blk_sz = 4;
> > +
> > +	size = QUP_IO_M_INPUT_BLOCK_SIZE(iomode);
> > +	if (size)
> > +		controller->in_blk_sz = size * 16;
> > +	else
> > +		controller->in_blk_sz = 4;
> > +
> > +	size = QUP_IO_M_OUTPUT_FIFO_SIZE(iomode);
> > +	controller->out_fifo_sz = controller->out_blk_sz * (2 << size);
> > +
> > +	size = QUP_IO_M_INPUT_FIFO_SIZE(iomode);
> > +	controller->in_fifo_sz = controller->in_blk_sz * (2 << size);
> > +
> > +	dev_info(dev, "v.%08x IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
> > +		 data, controller->in_blk_sz, controller->in_fifo_sz,
> > +		 controller->out_blk_sz, controller->out_fifo_sz);
> > +
> > +	writel_relaxed(1, base + QUP_SW_RESET);
> > +
> > +	ret = spi_qup_set_state(controller, QUP_STATE_RESET);
> > +	if (ret) {
> > +		dev_err(dev, "cannot set RESET state\n");
> > +		goto error;
> > +	}
> > +
> > +	writel_relaxed(0, base + QUP_OPERATIONAL);
> > +	writel_relaxed(0, base + QUP_IO_M_MODES);
> > +	writel_relaxed(0, base + QUP_OPERATIONAL_MASK);
> > +	writel_relaxed(SPI_ERROR_CLK_UNDER_RUN | SPI_ERROR_CLK_OVER_RUN,
> > +		       base + SPI_ERROR_FLAGS_EN);
> > +
> > +	writel_relaxed(0, base + SPI_CONFIG);
> > +	writel_relaxed(SPI_IO_C_NO_TRI_STATE, base + SPI_IO_CONTROL);
> > +
> > +	ret = devm_request_irq(dev, irq, spi_qup_qup_irq,
> > +			       IRQF_TRIGGER_HIGH, pdev->name, controller);
> > +	if (ret) {
> > +		dev_err(dev, "cannot request IRQ %d\n", irq);
> 
> unnecessary print

Will remove.

> 
> > +		goto error;
> > +	}
> > +
> > +	ret = devm_spi_register_master(dev, master);
> > +	if (!ret) {
> > +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> > +		pm_runtime_use_autosuspend(dev);
> > +		pm_runtime_set_active(dev);
> > +		pm_runtime_enable(dev);
> 
> Remove all the runtime stuff.  not supported right now.
> 
> > +		return ret;
> > +	}
> > +error:
> > +	clk_disable_unprepare(cclk);
> > +	clk_disable_unprepare(iclk);
> > +	spi_master_put(master);
> > +	return ret;
> > +}
> > +

<snip>

> 
> > +
> > +static int spi_qup_remove(struct platform_device *pdev)
> > +{
> > +	struct spi_master *master = dev_get_drvdata(&pdev->dev);
> > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > +
> > +	pm_runtime_get_sync(&pdev->dev);
> > +
> 
> Do we need to wait for any current transactions to complete
> and do a devm_free_irq()?
> 
> > +	clk_disable_unprepare(controller->cclk);
> > +	clk_disable_unprepare(controller->iclk);

My understanding is:

Disabling clocks will timeout transaction, if any. Core Device driver
will call: devm_spi_unregister(), which will wait pending transactions
to complete and then remove the SPI master.

> > +
> > +	pm_runtime_put_noidle(&pdev->dev);
> > +	pm_runtime_disable(&pdev->dev);
> > +	return 0;
> > +}
> > +
> > +static struct of_device_id spi_qup_dt_match[] = {
> > +	{ .compatible = "qcom,spi-qup-v2", },
> 
> Need compatible tags of qcom,spi-qup-v2.1.1 (msm8974 v1) or qcom,spi-qup-v2.2.1
> (msm8974 v2)

I am not aware of the difference. My board report v.20020000. 
Is there difference of handling these controllers?


Thanks, 
Ivan

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[e-mail] Feb. 7, 2014, 4:34 p.m. UTC | #3

> From: "Ivan T. Ivanov" <[e-mail]>
>
> Qualcomm Universal Peripheral (QUP) core is an 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
> support up to 50MHz, up to four chip selects, and a programmable
> data path from 4 bits to 32 bits; MODE0..3 protocols
>
> Signed-off-by: Ivan T. Ivanov <[e-mail]>
> Cc: Alok Chauhan <[e-mail]>
> Cc: Gilad Avidov <[e-mail]>
> Cc: Kiran Gunda <[e-mail]>
> Cc: Sagar Dharia <[e-mail]>
> ---
>  drivers/spi/Kconfig   |   14 +
>  drivers/spi/Makefile  |    1 +
>  drivers/spi/spi-qup.c |  898
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 913 insertions(+)
>  create mode 100644 drivers/spi/spi-qup.c
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index ba9310b..bf8ce6b 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -381,6 +381,20 @@ config SPI_RSPI
>         help
>           SPI driver for Renesas RSPI blocks.
>
> +config SPI_QUP
> +       tristate "Qualcomm SPI Support with QUP interface"
> +       depends on ARCH_MSM
> +       help
> +         Qualcomm Universal Peripheral (QUP) core is an 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 support up to 50MHz, up to four chip selects, and a
> +         programmable data path from 4 bits to 32 bits; supports numerous
> +         protocol variants.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called spi_qup.
> +
>  config SPI_S3C24XX
>         tristate "Samsung S3C24XX series SPI"
>         depends on ARCH_S3C24XX
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 95af48d..e598147 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -59,6 +59,7 @@ spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_PXADMA)       +=
> spi-pxa2xx-pxadma.o
>  spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_DMA)   += spi-pxa2xx-dma.o
>  obj-$(CONFIG_SPI_PXA2XX)               += spi-pxa2xx-platform.o
>  obj-$(CONFIG_SPI_PXA2XX_PCI)           += spi-pxa2xx-pci.o
> +obj-$(CONFIG_SPI_QUP)                  += spi-qup.o
>  obj-$(CONFIG_SPI_RSPI)                 += spi-rspi.o
>  obj-$(CONFIG_SPI_S3C24XX)              += spi-s3c24xx-hw.o
>  spi-s3c24xx-hw-y                       := spi-s3c24xx.o
> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> new file mode 100644
> index 0000000..5eb5e8f
> --- /dev/null
> +++ b/drivers/spi/spi-qup.c
> @@ -0,0 +1,898 @@
> +/*
> + * Copyright (c) 2008-2014, The Linux foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License rev 2 and
> + * only rev 2 as published by the free Software foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or fITNESS fOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/spi/spi.h>
> +
> +#define QUP_CONFIG                     0x0000
> +#define QUP_STATE                      0x0004
> +#define QUP_IO_M_MODES                 0x0008
> +#define QUP_SW_RESET                   0x000c
> +#define QUP_OPERATIONAL                        0x0018
> +#define QUP_ERROR_FLAGS                        0x001c
> +#define QUP_ERROR_FLAGS_EN             0x0020
> +#define QUP_OPERATIONAL_MASK           0x0028
> +#define QUP_HW_VERSION                 0x0030
> +#define QUP_MX_OUTPUT_CNT              0x0100
> +#define QUP_OUTPUT_FIFO                        0x0110
> +#define QUP_MX_WRITE_CNT               0x0150
> +#define QUP_MX_INPUT_CNT               0x0200
> +#define QUP_MX_READ_CNT                        0x0208
> +#define QUP_INPUT_FIFO                 0x0218
> +
> +#define SPI_CONFIG                     0x0300
> +#define SPI_IO_CONTROL                 0x0304
> +#define SPI_ERROR_FLAGS                        0x0308
> +#define SPI_ERROR_FLAGS_EN             0x030c
> +
> +/* QUP_CONFIG fields */
> +#define QUP_CONFIG_SPI_MODE            (1 << 8)
> +#define QUP_CONFIG_NO_INPUT            BIT(7)
> +#define QUP_CONFIG_NO_OUTPUT           BIT(6)
> +#define QUP_CONFIG_N                   0x001f
> +
> +/* QUP_STATE fields */
> +#define QUP_STATE_VALID                        BIT(2)
> +#define QUP_STATE_RESET                        0
> +#define QUP_STATE_RUN                  1
> +#define QUP_STATE_PAUSE                        3
> +#define QUP_STATE_MASK                 3
> +#define QUP_STATE_CLEAR                        2
> +
> +#define QUP_HW_VERSION_2_1_1           0x20010001
> +
> +/* QUP_IO_M_MODES fields */
> +#define QUP_IO_M_PACK_EN               BIT(15)
> +#define QUP_IO_M_UNPACK_EN             BIT(14)
> +#define QUP_IO_M_INPUT_MODE_MASK_SHIFT 12
> +#define QUP_IO_M_OUTPUT_MODE_MASK_SHIFT        10
> +#define QUP_IO_M_INPUT_MODE_MASK       (3 <<
> QUP_IO_M_INPUT_MODE_MASK_SHIFT)
> +#define QUP_IO_M_OUTPUT_MODE_MASK      (3 <<
> QUP_IO_M_OUTPUT_MODE_MASK_SHIFT)
> +
> +#define QUP_IO_M_OUTPUT_BLOCK_SIZE(x)  (((x) & (0x03 << 0)) >> 0)
> +#define QUP_IO_M_OUTPUT_FIFO_SIZE(x)   (((x) & (0x07 << 2)) >> 2)
> +#define QUP_IO_M_INPUT_BLOCK_SIZE(x)   (((x) & (0x03 << 5)) >> 5)
> +#define QUP_IO_M_INPUT_FIFO_SIZE(x)    (((x) & (0x07 << 7)) >> 7)
> +
> +#define QUP_IO_M_MODE_FIFO             0
> +#define QUP_IO_M_MODE_BLOCK            1
> +#define QUP_IO_M_MODE_DMOV             2
> +#define QUP_IO_M_MODE_BAM              3
> +
> +/* QUP_OPERATIONAL fields */
> +#define QUP_OP_MAX_INPUT_DONE_FLAG     BIT(11)
> +#define QUP_OP_MAX_OUTPUT_DONE_FLAG    BIT(10)
> +#define QUP_OP_IN_SERVICE_FLAG         BIT(9)
> +#define QUP_OP_OUT_SERVICE_FLAG                BIT(8)
> +#define QUP_OP_IN_FIFO_FULL            BIT(7)
> +#define QUP_OP_OUT_FIFO_FULL           BIT(6)
> +#define QUP_OP_IN_FIFO_NOT_EMPTY       BIT(5)
> +#define QUP_OP_OUT_FIFO_NOT_EMPTY      BIT(4)
> +
> +/* QUP_ERROR_FLAGS and QUP_ERROR_FLAGS_EN fields */
> +#define QUP_ERROR_OUTPUT_OVER_RUN      BIT(5)
> +#define QUP_ERROR_INPUT_UNDER_RUN      BIT(4)
> +#define QUP_ERROR_OUTPUT_UNDER_RUN     BIT(3)
> +#define QUP_ERROR_INPUT_OVER_RUN       BIT(2)
> +
> +/* SPI_CONFIG fields */
> +#define SPI_CONFIG_HS_MODE             BIT(10)
> +#define SPI_CONFIG_INPUT_FIRST         BIT(9)
> +#define SPI_CONFIG_LOOPBACK            BIT(8)
> +
> +/* SPI_IO_CONTROL fields */
> +#define SPI_IO_C_FORCE_CS              BIT(11)
> +#define SPI_IO_C_CLK_IDLE_HIGH         BIT(10)
> +#define SPI_IO_C_MX_CS_MODE            BIT(8)
> +#define SPI_IO_C_CS_N_POLARITY_0       BIT(4)
> +#define SPI_IO_C_CS_SELECT(x)          (((x) & 3) << 2)
> +#define SPI_IO_C_CS_SELECT_MASK                0x000c
> +#define SPI_IO_C_TRISTATE_CS           BIT(1)
> +#define SPI_IO_C_NO_TRI_STATE          BIT(0)
> +
> +/* SPI_ERROR_FLAGS and SPI_ERROR_FLAGS_EN fields */
> +#define SPI_ERROR_CLK_OVER_RUN         BIT(1)
> +#define SPI_ERROR_CLK_UNDER_RUN                BIT(0)
> +
> +#define SPI_NUM_CHIPSELECTS            4
> +
> +/* high speed mode is when bus rate is greater then 26MHz */
> +#define SPI_HS_MIN_RATE                        26000000
> +
> +#define SPI_DELAY_THRESHOLD            1
> +#define SPI_DELAY_RETRY                        10
> +
> +struct spi_qup_device {
> +       int bits_per_word;
> +       int chip_select;
> +       int speed_hz;
> +       u16 mode;
> +};
> +
> +struct spi_qup {
> +       void __iomem            *base;
> +       struct device           *dev;
> +       struct clk              *cclk;  /* core clock */
> +       struct clk              *iclk;  /* interface clock */
> +       int                     irq;
> +       u32                     max_speed_hz;
> +       u32                     speed_hz;
> +
> +       int                     in_fifo_sz;
> +       int                     out_fifo_sz;
> +       int                     in_blk_sz;
> +       int                     out_blk_sz;
> +
> +       struct spi_transfer     *xfer;
> +       struct completion       done;
> +       int                     error;
> +       int                     bytes_per_word;
> +       int                     tx_bytes;
> +       int                     rx_bytes;
> +};
> +
> +
> +static inline bool spi_qup_is_valid_state(struct spi_qup *controller)
> +{
> +       u32 opstate = readl_relaxed(controller->base + QUP_STATE);
> +
> +       return opstate & QUP_STATE_VALID;
> +}
> +
> +static int spi_qup_set_state(struct spi_qup *controller, u32 state)
> +{
> +       unsigned long loop = 0;
> +       u32 cur_state;
> +
> +       cur_state = readl_relaxed(controller->base + QUP_STATE);
Make sure the state is valid before you read the current state.
> +       /*
> +        * Per spec: for PAUSE_STATE to RESET_STATE, two writes
> +        * of (b10) are required
> +        */
> +       if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
> +           (state == QUP_STATE_RESET)) {
> +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
> QUP_STATE);
> +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
> QUP_STATE);
> +       } else {
Make sure you don't transition from RESET to PAUSE.
> +               cur_state &= ~QUP_STATE_MASK;
> +               cur_state |= state;
> +               writel_relaxed(cur_state, controller->base + QUP_STATE);
> +       }
> +
> +       while (!spi_qup_is_valid_state(controller)) {
> +
> +               usleep_range(SPI_DELAY_THRESHOLD, SPI_DELAY_THRESHOLD *
> 2);
> +
> +               if (++loop > SPI_DELAY_RETRY)
> +                       return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static void spi_qup_deassert_cs(struct spi_qup *controller,
> +                               struct spi_qup_device *chip)
> +{
> +       u32 iocontol, mask;
> +
> +       iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
> +
> +       /* Disable auto CS toggle and use manual */
> +       iocontol &= ~SPI_IO_C_MX_CS_MODE;
> +       iocontol |= SPI_IO_C_FORCE_CS;
> +
> +       iocontol &= ~SPI_IO_C_CS_SELECT_MASK;
> +       iocontol |= SPI_IO_C_CS_SELECT(chip->chip_select);
> +
> +       mask = SPI_IO_C_CS_N_POLARITY_0 << chip->chip_select;
> +
> +       if (chip->mode & SPI_CS_HIGH)
> +               iocontol &= ~mask;
> +       else
> +               iocontol |= mask;
> +
> +       writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
> +}
> +
> +static void spi_qup_assert_cs(struct spi_qup *controller,
> +                             struct spi_qup_device *chip)
> +{
> +       u32 iocontol, mask;
> +
> +       iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
> +
> +       /* Disable auto CS toggle and use manual */
> +       iocontol &= ~SPI_IO_C_MX_CS_MODE;
> +       iocontol |= SPI_IO_C_FORCE_CS;
> +
> +       iocontol &= ~SPI_IO_C_CS_SELECT_MASK;
> +       iocontol |= SPI_IO_C_CS_SELECT(chip->chip_select);
> +
> +       mask = SPI_IO_C_CS_N_POLARITY_0 << chip->chip_select;
> +
> +       if (chip->mode & SPI_CS_HIGH)
> +               iocontol |= mask;
> +       else
> +               iocontol &= ~mask;
> +
> +       writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
> +}
> +
> +static void spi_qup_fifo_read(struct spi_qup *controller,
> +                             struct spi_transfer *xfer)
> +{
> +       u8 *rx_buf = xfer->rx_buf;
> +       u32 word, state;
> +       int idx, shift;
> +
> +       while (controller->rx_bytes < xfer->len) {
> +
> +               state = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +               if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
> +                       break;
> +
> +               word = readl_relaxed(controller->base + QUP_INPUT_FIFO);
> +
> +               for (idx = 0; idx < controller->bytes_per_word &&
> +                    controller->rx_bytes < xfer->len; idx++,
> +                    controller->rx_bytes++) {
> +
> +                       if (!rx_buf)
> +                               continue;
If there is no rx_buf just set rx_bytes to xfer->len and skip the loop
entirely.
> +                       /*
> +                        * The data format depends on bytes_per_word:
> +                        *  4 bytes: 0x12345678
> +                        *  2 bytes: 0x00001234
> +                        *  1 byte : 0x00000012
> +                        */
> +                       shift = BITS_PER_BYTE;
> +                       shift *= (controller->bytes_per_word - idx - 1);
> +                       rx_buf[controller->rx_bytes] = word >> shift;
> +               }
> +       }
> +}
> +
> +static void spi_qup_fifo_write(struct spi_qup *controller,
> +                              struct spi_transfer *xfer)
> +{
> +       const u8 *tx_buf = xfer->tx_buf;
> +       u32 word, state, data;
> +       int idx;
> +
> +       while (controller->tx_bytes < xfer->len) {
> +
> +               state = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +               if (state & QUP_OP_OUT_FIFO_FULL)
> +                       break;
> +
> +               word = 0;
> +               for (idx = 0; idx < controller->bytes_per_word &&
> +                    controller->tx_bytes < xfer->len; idx++,
> +                    controller->tx_bytes++) {
> +
> +                       if (!tx_buf)
> +                               continue;
Just set tx_bytes to xfer->len and return prior to entering loop.
> +
> +                       data = tx_buf[controller->tx_bytes];
> +                       word |= data << (BITS_PER_BYTE * (3 - idx));
> +               }
> +
> +               writel_relaxed(word, controller->base + QUP_OUTPUT_FIFO);
> +       }
> +}
> +
> +static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
> +{
> +       struct spi_qup *controller = dev_id;
> +       struct spi_transfer *xfer;
> +       u32 opflags, qup_err, spi_err;
> +
> +       xfer = controller->xfer;
> +
> +       qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> +       spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> +       opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +
> +       writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> +       writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> +       writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> +
> +       if (!xfer)
> +               return IRQ_HANDLED;
> +
> +       if (qup_err) {
> +               if (qup_err & QUP_ERROR_OUTPUT_OVER_RUN)
> +                       dev_warn(controller->dev, "OUTPUT_OVER_RUN\n");
> +               if (qup_err & QUP_ERROR_INPUT_UNDER_RUN)
> +                       dev_warn(controller->dev, "INPUT_UNDER_RUN\n");
> +               if (qup_err & QUP_ERROR_OUTPUT_UNDER_RUN)
> +                       dev_warn(controller->dev, "OUTPUT_UNDER_RUN\n");
> +               if (qup_err & QUP_ERROR_INPUT_OVER_RUN)
> +                       dev_warn(controller->dev, "INPUT_OVER_RUN\n");
> +
> +               controller->error = -EIO;
> +       }
> +
> +       if (spi_err) {
> +               if (spi_err & SPI_ERROR_CLK_OVER_RUN)
> +                       dev_warn(controller->dev, "CLK_OVER_RUN\n");
> +               if (spi_err & SPI_ERROR_CLK_UNDER_RUN)
> +                       dev_warn(controller->dev, "CLK_UNDER_RUN\n");
> +
> +               controller->error = -EIO;
> +       }
> +
> +       if (opflags & QUP_OP_IN_SERVICE_FLAG) {
> +               writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
> +                      controller->base + QUP_OPERATIONAL);
Write is not necessary since already cleared above.
> +               spi_qup_fifo_read(controller, xfer);
> +       }
> +
> +       if (opflags & QUP_OP_OUT_SERVICE_FLAG) {
> +               writel_relaxed(QUP_OP_OUT_SERVICE_FLAG,
> +                      controller->base + QUP_OPERATIONAL);
Write is not necessary since already cleared above.
> +               spi_qup_fifo_write(controller, xfer);
> +       }
> +
> +       if (controller->rx_bytes == xfer->len ||
> +           controller->error)
> +               complete(&controller->done);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int spi_qup_transfer_do(struct spi_qup *controller,
> +                              struct spi_qup_device *chip,
> +                              struct spi_transfer *xfer)
> +{
> +       unsigned long timeout;
> +       int ret = -EIO;
> +
> +       reinit_completion(&controller->done);
> +
> +       timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
> +       timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
> +       timeout = 100 * msecs_to_jiffies(timeout);
> +
> +       controller->rx_bytes = 0;
> +       controller->tx_bytes = 0;
> +       controller->error = 0;
> +       controller->xfer = xfer;
> +
> +       if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> +               dev_warn(controller->dev, "cannot set RUN state\n");
> +               goto exit;
> +       }
> +
> +       if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
> +               dev_warn(controller->dev, "cannot set PAUSE state\n");
> +               goto exit;
> +       }
> +
> +       spi_qup_fifo_write(controller, xfer);
> +
> +       if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> +               dev_warn(controller->dev, "cannot set EXECUTE state\n");
> +               goto exit;
> +       }
> +
> +       if (!wait_for_completion_timeout(&controller->done, timeout))
> +               ret = -ETIMEDOUT;
> +       else
> +               ret = controller->error;
> +exit:
> +       controller->xfer = NULL;
> +       controller->error = 0;
> +       controller->rx_bytes = 0;
> +       controller->tx_bytes = 0;
> +       spi_qup_set_state(controller, QUP_STATE_RESET);
> +       return ret;
> +}
> +
> +static int spi_qup_setup(struct spi_device *spi)
> +{
> +       struct spi_qup *controller = spi_master_get_devdata(spi->master);
> +       struct spi_qup_device *chip = spi_get_ctldata(spi);
> +
> +       if (spi->chip_select >= spi->master->num_chipselect) {
> +               dev_err(controller->dev, "invalid chip_select %d\n",
> +                       spi->chip_select);
> +               return -EINVAL;
> +       }
> +
> +       if (spi->max_speed_hz > controller->max_speed_hz) {
> +               dev_err(controller->dev, "invalid max_speed_hz %d\n",
> +                       spi->max_speed_hz);
> +               return -EINVAL;
> +       }
> +
> +       if (!chip) {
> +               /* First setup */
> +               chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +               if (!chip) {
> +                       dev_err(controller->dev, "no memory for chip
> data\n");
> +                       return -ENOMEM;
> +               }
> +
> +               spi_set_ctldata(spi, chip);
> +       }
> +
> +       return 0;
> +}
> +
> +static void spi_qup_cleanup(struct spi_device *spi)
> +{
> +       struct spi_qup_device *chip = spi_get_ctldata(spi);
> +
> +       if (!chip)
> +               return;
> +
> +       spi_set_ctldata(spi, NULL);
> +       kfree(chip);
> +}
> +
> +/* set clock freq, clock ramp, bits per work */
> +static int spi_qup_io_setup(struct spi_device *spi,
> +                         struct spi_transfer *xfer)
> +{
> +       struct spi_qup *controller = spi_master_get_devdata(spi->master);
> +       struct spi_qup_device *chip = spi_get_ctldata(spi);
> +       u32 iocontol, config, iomode, mode;
> +       int ret, n_words;
> +
> +       if (spi->mode & SPI_LOOP && xfer->len > controller->in_fifo_sz) {
> +               dev_err(controller->dev, "too big size for loopback %d >
> %d\n",
> +                       xfer->len, controller->in_fifo_sz);
> +               return -EIO;
> +       }
> +
> +       chip->mode = spi->mode;
> +       chip->speed_hz = spi->max_speed_hz;
> +       if (xfer->speed_hz)
> +               chip->speed_hz = xfer->speed_hz;
> +
> +       if (controller->speed_hz != chip->speed_hz) {
> +               ret = clk_set_rate(controller->cclk, chip->speed_hz);
> +               if (ret) {
> +                       dev_err(controller->dev, "fail to set frequency
> %d",
> +                               chip->speed_hz);
> +                       return -EIO;
> +               }
> +       }
> +
> +       controller->speed_hz = chip->speed_hz;
> +
> +       chip->bits_per_word = spi->bits_per_word;
> +       if (xfer->bits_per_word)
> +               chip->bits_per_word = xfer->bits_per_word;
> +
> +       if (chip->bits_per_word <= 8)
> +               controller->bytes_per_word = 1;
> +       else if (chip->bits_per_word <= 16)
> +               controller->bytes_per_word = 2;
> +       else
> +               controller->bytes_per_word = 4;
> +
> +       if (controller->bytes_per_word > xfer->len ||
> +           xfer->len % controller->bytes_per_word != 0){
> +               /* No partial transfers */
> +               dev_err(controller->dev, "invalid len %d for %d bits\n",
> +                       xfer->len, chip->bits_per_word);
> +               return -EIO;
> +       }
> +
> +       n_words = xfer->len / controller->bytes_per_word;
> +
> +       if (spi_qup_set_state(controller, QUP_STATE_RESET)) {
> +               dev_err(controller->dev, "cannot set RESET state\n");
> +               return -EIO;
> +       }
> +
> +       if (n_words <= controller->in_fifo_sz) {
> +               mode = QUP_IO_M_MODE_FIFO;
> +               writel_relaxed(n_words, controller->base +
> QUP_MX_READ_CNT);
> +               writel_relaxed(n_words, controller->base +
> QUP_MX_WRITE_CNT);
> +               /* must be zero for FIFO */
> +               writel_relaxed(0, controller->base + QUP_MX_INPUT_CNT);
> +               writel_relaxed(0, controller->base + QUP_MX_OUTPUT_CNT);
> +       } else {
> +               mode = QUP_IO_M_MODE_BLOCK;
> +               writel_relaxed(n_words, controller->base +
> QUP_MX_INPUT_CNT);
> +               writel_relaxed(n_words, controller->base +
> QUP_MX_OUTPUT_CNT);
> +               /* must be zero for BLOCK and BAM */
> +               writel_relaxed(0, controller->base + QUP_MX_READ_CNT);
> +               writel_relaxed(0, controller->base + QUP_MX_WRITE_CNT);
> +       }
> +
> +       iomode = readl_relaxed(controller->base + QUP_IO_M_MODES);
> +       /* Set input and output transfer mode */
> +       iomode &= ~(QUP_IO_M_INPUT_MODE_MASK | QUP_IO_M_OUTPUT_MODE_MASK);
> +       iomode &= ~(QUP_IO_M_PACK_EN | QUP_IO_M_UNPACK_EN);
> +       iomode |= (mode << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT);
> +       iomode |= (mode << QUP_IO_M_INPUT_MODE_MASK_SHIFT);
> +
> +       writel_relaxed(iomode, controller->base + QUP_IO_M_MODES);
> +
> +       config = readl_relaxed(controller->base + SPI_CONFIG);
> +
> +       if (chip->mode & SPI_LOOP)
> +               config |= SPI_CONFIG_LOOPBACK;
> +       else
> +               config &= ~SPI_CONFIG_LOOPBACK;
> +
> +       if (chip->mode & SPI_CPHA)
> +               config &= ~SPI_CONFIG_INPUT_FIRST;
> +       else
> +               config |= SPI_CONFIG_INPUT_FIRST;
> +
> +       /*
> +        * HS_MODE improves signal stability for spi-clk high rates
> +        * but is invalid in loop back mode.
> +        */
> +       if ((controller->speed_hz >= SPI_HS_MIN_RATE) &&
> +           !(chip->mode & SPI_LOOP))
> +               config |= SPI_CONFIG_HS_MODE;
> +       else
> +               config &= ~SPI_CONFIG_HS_MODE;
> +
> +       writel_relaxed(config, controller->base + SPI_CONFIG);
> +
> +       config = readl_relaxed(controller->base + QUP_CONFIG);
> +       config &= ~(QUP_CONFIG_NO_INPUT | QUP_CONFIG_NO_OUTPUT |
> QUP_CONFIG_N);
> +       config |= chip->bits_per_word - 1;
> +       config |= QUP_CONFIG_SPI_MODE;
> +       writel_relaxed(config, controller->base + QUP_CONFIG);
> +
> +       iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
> +
> +       /* Disable auto CS toggle */
> +       iocontol &= ~SPI_IO_C_MX_CS_MODE;
> +
> +       if (chip->mode & SPI_CPOL)
> +               iocontol |= SPI_IO_C_CLK_IDLE_HIGH;
> +       else
> +               iocontol &= ~SPI_IO_C_CLK_IDLE_HIGH;
> +
> +       writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
> +
> +       /*
> +        * TODO: In BAM mode mask INPUT and OUTPUT service flags in
> +        * to prevent IRQs on FIFO status change.
> +        */
Remove TODO
> +       writel_relaxed(0, controller->base + QUP_OPERATIONAL_MASK);
> +
> +       return 0;
> +}
> +
> +static int spi_qup_transfer_one(struct spi_master *master,
> +                               struct spi_message *msg)
> +{
> +       struct spi_qup *controller = spi_master_get_devdata(master);
> +       struct spi_qup_device *chip = spi_get_ctldata(msg->spi);
> +       struct spi_transfer *xfer;
> +       struct spi_device *spi;
> +       unsigned cs_change;
> +       int status;
> +
> +       spi = msg->spi;
> +       cs_change = 1;
> +       status = 0;
> +
> +       list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +
> +               status = spi_qup_io_setup(spi, xfer);
> +               if (status)
> +                       break;
> +
> +               if (cs_change)
> +                       spi_qup_assert_cs(controller, chip);
> +
> +               cs_change = xfer->cs_change;
> +
> +               /* Do actual transfer */
> +               status = spi_qup_transfer_do(controller, chip, xfer);
> +               if (status)
> +                       break;
> +
> +               msg->actual_length += xfer->len;
> +
> +               if (xfer->delay_usecs)
> +                       udelay(xfer->delay_usecs);
> +
> +               if (cs_change)
> +                       spi_qup_deassert_cs(controller, chip);
> +       }
> +
> +       if (status || !cs_change)
> +               spi_qup_deassert_cs(controller, chip);
> +
> +       msg->status = status;
> +       spi_finalize_current_message(master);
> +       return status;
> +}
> +
> +static int spi_qup_probe(struct platform_device *pdev)
> +{
> +       struct spi_master *master;
> +       struct clk *iclk, *cclk;
> +       struct spi_qup *controller;
> +       struct resource *res;
> +       struct device *dev;
> +       void __iomem *base;
> +       u32 data, max_freq, iomode;
> +       int ret, irq, size;
> +
> +       dev = &pdev->dev;
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       irq = platform_get_irq(pdev, 0);
> +
> +       if (irq < 0)
> +               return irq;
> +
> +       cclk = devm_clk_get(dev, "core");
> +       if (IS_ERR(cclk)) {
> +               dev_err(dev, "cannot get core clock\n");
> +               return PTR_ERR(cclk);
> +       }
> +
> +       iclk = devm_clk_get(dev, "iface");
> +       if (IS_ERR(iclk)) {
> +               dev_err(dev, "cannot get iface clock\n");
> +               return PTR_ERR(iclk);
> +       }
> +
> +       if (of_property_read_u32(dev->of_node, "spi-max-frequency",
> &max_freq))
> +               max_freq = 19200000;
> +
> +       if (!max_freq) {
> +               dev_err(dev, "invalid clock frequency %d\n", max_freq);
> +               return -ENXIO;
> +       }
> +
> +       ret = clk_set_rate(cclk, max_freq);
> +       if (ret)
> +               dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> +
> +       ret = clk_prepare_enable(cclk);
> +       if (ret) {
> +               dev_err(dev, "cannot enable core clock\n");
> +               return ret;
> +       }
> +
> +       ret = clk_prepare_enable(iclk);
> +       if (ret) {
> +               clk_disable_unprepare(cclk);
> +               dev_err(dev, "cannot enable iface clock\n");
> +               return ret;
> +       }
> +
> +       data = readl_relaxed(base + QUP_HW_VERSION);
> +
> +       if (data < QUP_HW_VERSION_2_1_1) {
> +               clk_disable_unprepare(cclk);
> +               clk_disable_unprepare(iclk);
> +               dev_err(dev, "v.%08x is not supported\n", data);
> +               return -ENXIO;
> +       }
> +
> +       master = spi_alloc_master(dev, sizeof(struct spi_qup));
> +       if (!master) {
> +               clk_disable_unprepare(cclk);
> +               clk_disable_unprepare(iclk);
> +               dev_err(dev, "cannot allocate master\n");
> +               return -ENOMEM;
> +       }
> +
> +       master->bus_num = pdev->id;
> +       master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
> +       master->num_chipselect = SPI_NUM_CHIPSELECTS;
> +       master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> +       master->setup = spi_qup_setup;
> +       master->cleanup = spi_qup_cleanup;
> +       master->transfer_one_message = spi_qup_transfer_one;
> +       master->dev.of_node = pdev->dev.of_node;
> +       master->auto_runtime_pm = true;
> +
> +       platform_set_drvdata(pdev, master);
> +
> +       controller = spi_master_get_devdata(master);
> +
> +       controller->dev  = dev;
> +       controller->base = base;
> +       controller->iclk = iclk;
> +       controller->cclk = cclk;
> +       controller->irq  = irq;
> +       controller->max_speed_hz = clk_get_rate(cclk);
> +       controller->speed_hz = controller->max_speed_hz;
> +
> +       init_completion(&controller->done);
> +
> +       iomode = readl_relaxed(base + QUP_IO_M_MODES);
> +
> +       size = QUP_IO_M_OUTPUT_BLOCK_SIZE(iomode);
> +       if (size)
> +               controller->out_blk_sz = size * 16;
> +       else
> +               controller->out_blk_sz = 4;
> +
> +       size = QUP_IO_M_INPUT_BLOCK_SIZE(iomode);
> +       if (size)
> +               controller->in_blk_sz = size * 16;
> +       else
> +               controller->in_blk_sz = 4;
> +
> +       size = QUP_IO_M_OUTPUT_FIFO_SIZE(iomode);
> +       controller->out_fifo_sz = controller->out_blk_sz * (2 << size);
> +
> +       size = QUP_IO_M_INPUT_FIFO_SIZE(iomode);
> +       controller->in_fifo_sz = controller->in_blk_sz * (2 << size);
> +
> +       dev_info(dev, "v.%08x IN:block:%d, fifo:%d, OUT:block:%d,
> fifo:%d\n",
> +                data, controller->in_blk_sz, controller->in_fifo_sz,
> +                controller->out_blk_sz, controller->out_fifo_sz);
> +
> +       writel_relaxed(1, base + QUP_SW_RESET);
> +
> +       ret = spi_qup_set_state(controller, QUP_STATE_RESET);
> +       if (ret) {
> +               dev_err(dev, "cannot set RESET state\n");
> +               goto error;
> +       }
> +
> +       writel_relaxed(0, base + QUP_OPERATIONAL);
> +       writel_relaxed(0, base + QUP_IO_M_MODES);
> +       writel_relaxed(0, base + QUP_OPERATIONAL_MASK);
> +       writel_relaxed(SPI_ERROR_CLK_UNDER_RUN | SPI_ERROR_CLK_OVER_RUN,
> +                      base + SPI_ERROR_FLAGS_EN);
> +
> +       writel_relaxed(0, base + SPI_CONFIG);
> +       writel_relaxed(SPI_IO_C_NO_TRI_STATE, base + SPI_IO_CONTROL);
> +
> +       ret = devm_request_irq(dev, irq, spi_qup_qup_irq,
> +                              IRQF_TRIGGER_HIGH, pdev->name, controller);
> +       if (ret) {
> +               dev_err(dev, "cannot request IRQ %d\n", irq);
> +               goto error;
> +       }
> +
> +       ret = devm_spi_register_master(dev, master);
> +       if (!ret) {
> +               pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> +               pm_runtime_use_autosuspend(dev);
> +               pm_runtime_set_active(dev);
> +               pm_runtime_enable(dev);
> +               return ret;
> +       }
> +error:
> +       clk_disable_unprepare(cclk);
> +       clk_disable_unprepare(iclk);
> +       spi_master_put(master);
> +       return ret;
> +}
> +
> +#ifdef CONFIG_PM_RUNTIME
> +static int spi_qup_pm_suspend_runtime(struct device *device)
> +{
> +       struct spi_master *master = dev_get_drvdata(device);
> +       struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +       disable_irq(controller->irq);
> +       clk_disable_unprepare(controller->cclk);
> +       clk_disable_unprepare(controller->iclk);
> +       dev_dbg(device, "suspend runtime\n");
> +       return 0;
> +}
> +
> +static int spi_qup_pm_resume_runtime(struct device *device)
> +{
> +       struct spi_master *master = dev_get_drvdata(device);
> +       struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +       clk_prepare_enable(controller->cclk);
> +       clk_prepare_enable(controller->iclk);
> +       enable_irq(controller->irq);
> +       dev_dbg(device, "resume runtime\n");
> +       return 0;
> +}
> +#endif /* CONFIG_PM_RUNTIME */
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int spi_qup_suspend(struct device *device)
> +{
> +       struct spi_master *master = dev_get_drvdata(device);
> +       struct spi_qup *controller = spi_master_get_devdata(master);
> +       int status;
> +
> +       status = spi_master_suspend(master);
> +       if (!status) {
> +               disable_irq(controller->irq);
> +               clk_disable_unprepare(controller->cclk);
> +               clk_disable_unprepare(controller->iclk);
> +       }
> +
> +       dev_dbg(device, "system suspend %d\n", status);
> +       return status;
> +}
> +
> +static int spi_qup_resume(struct device *device)
> +{
> +       struct spi_master *master = dev_get_drvdata(device);
> +       struct spi_qup *controller = spi_master_get_devdata(master);
> +       int status;
> +
> +       clk_prepare_enable(controller->cclk);
> +       clk_prepare_enable(controller->iclk);
> +
> +       status = spi_master_resume(master);
> +
> +       dev_dbg(device, "system resume %d\n", status);
> +       return status;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static int spi_qup_remove(struct platform_device *pdev)
> +{
> +       struct spi_master *master = dev_get_drvdata(&pdev->dev);
> +       struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +       pm_runtime_get_sync(&pdev->dev);
> +
> +       clk_disable_unprepare(controller->cclk);
> +       clk_disable_unprepare(controller->iclk);
> +
> +       pm_runtime_put_noidle(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);
> +       return 0;
> +}
> +
> +static struct of_device_id spi_qup_dt_match[] = {
> +       { .compatible = "qcom,spi-qup-v2", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, spi_qup_dt_match);
> +
> +static const struct dev_pm_ops spi_qup_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(spi_qup_suspend, spi_qup_resume)
> +       SET_RUNTIME_PM_OPS(spi_qup_pm_suspend_runtime,
> +                          spi_qup_pm_resume_runtime,
> +                          NULL)
> +};
> +
> +static struct platform_driver spi_qup_driver = {
> +       .driver = {
> +               .name           = "spi_qup",
> +               .owner          = THIS_MODULE,
> +               .pm             = &spi_qup_dev_pm_ops,
> +               .of_match_table = spi_qup_dt_match,
> +       },
> +       .probe = spi_qup_probe,
> +       .remove = spi_qup_remove,
> +};
> +module_platform_driver(spi_qup_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION("0.4");
> +MODULE_ALIAS("platform:spi_qup");



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Josh Cartwright Feb. 7, 2014, 4:51 p.m. UTC | #4

On Fri, Feb 07, 2014 at 01:39:52AM -0600, Andy Gross wrote:
> On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <[e-mail]>
> > 
> > Qualcomm Universal Peripheral (QUP) core is an 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
> > support up to 50MHz, up to four chip selects, and a programmable
> > data path from 4 bits to 32 bits; MODE0..3 protocols
> > 
> > Signed-off-by: Ivan T. Ivanov <[e-mail]>
> > Cc: Alok Chauhan <[e-mail]>
> > Cc: Gilad Avidov <[e-mail]>
> > Cc: Kiran Gunda <[e-mail]>
> > Cc: Sagar Dharia <[e-mail]>
> > ---
> >  drivers/spi/Kconfig   |   14 +
> >  drivers/spi/Makefile  |    1 +
> >  drivers/spi/spi-qup.c |  898 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 913 insertions(+)
> >  create mode 100644 drivers/spi/spi-qup.c
> > 
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index ba9310b..bf8ce6b 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -381,6 +381,20 @@ config SPI_RSPI
> >  	help
> >  	  SPI driver for Renesas RSPI blocks.
> >  
> > +config SPI_QUP
> > +	tristate "Qualcomm SPI Support with QUP interface"
> > +	depends on ARCH_MSM
> 
> I'd change to ARCH_MSM_DT.  This ensures the OF component is there.

I'd rather explicitly include the CONFIG_OF dependency, but I'm not too
opinionated.

	config SPI_QUP
		tristate "Qualcomm SPI Support with QUP interface"
		depends on OF
		depends on ARM
		depends on ARCH_MSM_DT || COMPILE_TEST

With Kumar's pending the ARCH_MSM_DT -> ARCH_QCOM rename, we'll
introduce a arm-soc/spi tree dependency here that we'll need to keep
track of.

Kumar-

How would you like to handle this?  Would it make sense for this to go
through the SPI tree with depending on ARCH_QCOM instead of ARCH_MSM_DT?

Mark Brown Feb. 7, 2014, 5:12 p.m. UTC | #5

On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:

This looks mostly good, there's a few odd things and missing use of
framework features.

> Qualcomm Universal Peripheral (QUP) core is an 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
> support up to 50MHz, up to four chip selects, and a programmable
> data path from 4 bits to 32 bits; MODE0..3 protocols

The grammar in this and the Kconfig text is a bit garbled, might want to
give it a once over (support -> supports for example).

> +static void spi_qup_deassert_cs(struct spi_qup *controller,
> +				struct spi_qup_device *chip)
> +{


> +	if (chip->mode & SPI_CS_HIGH)
> +		iocontol &= ~mask;
> +	else
> +		iocontol |= mask;

Implement a set_cs() operation and let the core worry about all this
for you as well as saving two implementations.

> +		word = 0;
> +		for (idx = 0; idx < controller->bytes_per_word &&
> +		     controller->tx_bytes < xfer->len; idx++,
> +		     controller->tx_bytes++) {
> +
> +			if (!tx_buf)
> +				continue;

Do you need to set the _MUST_TX flag?

> +	qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> +	spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> +	opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +
> +	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> +	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> +	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> +
> +	if (!xfer)
> +		return IRQ_HANDLED;

Are you sure?  It seems wrong to just ignore interrupts, some comments
would help explain why.

> +static int spi_qup_transfer_do(struct spi_qup *controller,
> +			       struct spi_qup_device *chip,
> +			       struct spi_transfer *xfer)

This looks like a transfer_one() function, please use the framework
features where you can.

> +	if (controller->speed_hz != chip->speed_hz) {
> +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> +		if (ret) {
> +			dev_err(controller->dev, "fail to set frequency %d",
> +				chip->speed_hz);
> +			return -EIO;
> +		}
> +	}

Is calling into the clock framework really so expensive that we need to
avoid doing it?  You also shouldn't be interacting with the hardware in
setup().

> +	if (chip->bits_per_word <= 8)
> +		controller->bytes_per_word = 1;
> +	else if (chip->bits_per_word <= 16)
> +		controller->bytes_per_word = 2;
> +	else
> +		controller->bytes_per_word = 4;

This looks like a switch statement, and looking at the above it's not
clear that the device actually supports anything other than whole bytes.
I'm not sure what that would mean from an API point of view.

> +static int spi_qup_transfer_one(struct spi_master *master,
> +				struct spi_message *msg)
> +{

This entire function can be removed, the core can do it for you.

> +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> +		max_freq = 19200000;
> +
> +	if (!max_freq) {
> +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> +		return -ENXIO;
> +	}
> +
> +	ret = clk_set_rate(cclk, max_freq);
> +	if (ret)
> +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);

You set the clock rate per transfer so why bother setting it here,
perhaps we support the rate the devices request but not this maximum
rate?

> +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);

Are you *sure* the device supports anything other than whole bytes?

> +	ret = devm_spi_register_master(dev, master);
> +	if (!ret) {
> +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> +		pm_runtime_use_autosuspend(dev);
> +		pm_runtime_set_active(dev);
> +		pm_runtime_enable(dev);
> +		return ret;
> +	}

This is really unclearly written, the success case looks like error
handling.

> +#ifdef CONFIG_PM_RUNTIME
> +static int spi_qup_pm_suspend_runtime(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	disable_irq(controller->irq);

Why do you need to disable the interrupt?  Will the hardware generate
spurious interrupts, if so some documentation is in order.

> +static int spi_qup_pm_resume_runtime(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	clk_prepare_enable(controller->cclk);
> +	clk_prepare_enable(controller->iclk);
> +	enable_irq(controller->irq);

No error checking here...

Mark Brown Feb. 7, 2014, 5:16 p.m. UTC | #6

On Fri, Feb 07, 2014 at 04:34:25PM -0000, [e-mail] wrote:
> > From: "Ivan T. Ivanov" <[e-mail]>
> >
> > Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> > provides a common data path (an output FIFO and an input FIFO)

Folks, please remember to delete irrelevant context from your e-mails,
it's easy for the reader not to see things if they have to go through
pages of quote to find one or two lines of new text.

Mark Brown Feb. 7, 2014, 5:18 p.m. UTC | #7

On Fri, Feb 07, 2014 at 10:51:27AM -0600, Josh Cartwright wrote:

> 	config SPI_QUP
> 		tristate "Qualcomm SPI Support with QUP interface"
> 		depends on OF
> 		depends on ARM

Does this really depend on ARM?  If so why?

> 		depends on ARCH_MSM_DT || COMPILE_TEST

> With Kumar's pending the ARCH_MSM_DT -> ARCH_QCOM rename, we'll
> introduce a arm-soc/spi tree dependency here that we'll need to keep
> track of.

It seems simpler to just depend on MSM_DT || ARCH_QCOM or whatever.

Josh Cartwright Feb. 7, 2014, 5:20 p.m. UTC | #8

Hey Mark-

On Fri, Feb 07, 2014 at 05:18:34PM +0000, Mark Brown wrote:
> On Fri, Feb 07, 2014 at 10:51:27AM -0600, Josh Cartwright wrote:
> 
> > 	config SPI_QUP
> > 		tristate "Qualcomm SPI Support with QUP interface"
> > 		depends on OF
> > 		depends on ARM
> 
> Does this really depend on ARM?  If so why?

The ARM dependency is there for the use of _relaxed io accessor
variants.

> > 		depends on ARCH_MSM_DT || COMPILE_TEST
> 
> > With Kumar's pending the ARCH_MSM_DT -> ARCH_QCOM rename, we'll
> > introduce a arm-soc/spi tree dependency here that we'll need to keep
> > track of.
> 
> It seems simpler to just depend on MSM_DT || ARCH_QCOM or whatever.

ARCH_MSM_DT is going away, so maybe this is the best option for the
short term (a later patch can remove ARCH_MSM_DT from here at some point
in the future).

  Josh

Mark Brown Feb. 7, 2014, 5:31 p.m. UTC | #9

On Fri, Feb 07, 2014 at 11:20:51AM -0600, Josh Cartwright wrote:
> On Fri, Feb 07, 2014 at 05:18:34PM +0000, Mark Brown wrote:
> > On Fri, Feb 07, 2014 at 10:51:27AM -0600, Josh Cartwright wrote:

> > > 	config SPI_QUP
> > > 		tristate "Qualcomm SPI Support with QUP interface"
> > > 		depends on OF
> > > 		depends on ARM

> > Does this really depend on ARM?  If so why?

> The ARM dependency is there for the use of _relaxed io accessor
> variants.

That's not ARM only and I thought we were getting generic versions of it
anyway?  ARMv8, MIPS, Microblaze, Hexagon and SH also define it.

Andy Gross Feb. 7, 2014, 5:32 p.m. UTC | #10

On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:
> 
> Hi Andy,
> 
> On Fri, 2014-02-07 at 01:39 -0600, Andy Gross wrote: 
> > On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> > > From: "Ivan T. Ivanov" <[e-mail]>
> > > 
> > > Qualcomm Universal Peripheral (QUP) core is an 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
> > > support up to 50MHz, up to four chip selects, and a programmable
> > > data path from 4 bits to 32 bits; MODE0..3 protocols
> > > 
> > > Signed-off-by: Ivan T. Ivanov <[e-mail]>
> > > Cc: Alok Chauhan <[e-mail]>
> > > Cc: Gilad Avidov <[e-mail]>
> > > Cc: Kiran Gunda <[e-mail]>
> > > Cc: Sagar Dharia <[e-mail]>
> > > ---
> > >  drivers/spi/Kconfig   |   14 +
> > >  drivers/spi/Makefile  |    1 +
> > >  drivers/spi/spi-qup.c |  898 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 913 insertions(+)
> > >  create mode 100644 drivers/spi/spi-qup.c
> > > 
> > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > > index ba9310b..bf8ce6b 100644
> > > --- a/drivers/spi/Kconfig
> > > +++ b/drivers/spi/Kconfig
> > > @@ -381,6 +381,20 @@ config SPI_RSPI
> > >  	help
> > >  	  SPI driver for Renesas RSPI blocks.
> > >  
> > > +config SPI_QUP
> > > +	tristate "Qualcomm SPI Support with QUP interface"
> > > +	depends on ARCH_MSM
> > 
> > I'd change to ARCH_MSM_DT.  This ensures the OF component is there.
> 
> Ok. will change.
> 
> > 
> > > +	help
> > > +	  Qualcomm Universal Peripheral (QUP) core is an 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 support up to 50MHz, up to four chip selects, and a
> > > +	  programmable data path from 4 bits to 32 bits; supports numerous
> > > +	  protocol variants.
> > > +
> > > +	  This driver can also be built as a module.  If so, the module
> > > +	  will be called spi_qup.
> > > +
> > >  config SPI_S3C24XX
> > >  	tristate "Samsung S3C24XX series SPI"
> > >  	depends on ARCH_S3C24XX
> > > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > > index 95af48d..e598147 100644
> > > --- a/drivers/spi/Makefile
> > > +++ b/drivers/spi/Makefile
> > > @@ -59,6 +59,7 @@ spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_PXADMA)	+= spi-pxa2xx-pxadma.o
> > >  spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_DMA)	+= spi-pxa2xx-dma.o
> > >  obj-$(CONFIG_SPI_PXA2XX)		+= spi-pxa2xx-platform.o
> > >  obj-$(CONFIG_SPI_PXA2XX_PCI)		+= spi-pxa2xx-pci.o
> > > +obj-$(CONFIG_SPI_QUP)			+= spi-qup.o
> > >  obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
> > >  obj-$(CONFIG_SPI_S3C24XX)		+= spi-s3c24xx-hw.o
> > >  spi-s3c24xx-hw-y			:= spi-s3c24xx.o
> > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> > > new file mode 100644
> > > index 0000000..5eb5e8f
> > > --- /dev/null
> > > +++ b/drivers/spi/spi-qup.c
> > > @@ -0,0 +1,898 @@
> > > +/*
> > > + * Copyright (c) 2008-2014, The Linux foundation. All rights reserved.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License rev 2 and
> > > + * only rev 2 as published by the free Software foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or fITNESS fOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/err.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/list.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > 
> > Remove this for now.  No runtime support.
> 
> Did you see any particular issue with the implementation 
> or this is just because this platform didn't have support 
> for power management?  
> 

The platform doesn't have support for PM right now.  So it's probably better to
remove all this and revisit later when it is in place.

> > > +#include <linux/spi/spi.h>
> > > +
> 
> <snip>
> 
> > > +
> > > +static int spi_qup_transfer_do(struct spi_qup *controller,
> > > +			       struct spi_qup_device *chip,
> > > +			       struct spi_transfer *xfer)
> > > +{
> > > +	unsigned long timeout;
> > > +	int ret = -EIO;
> > > +
> > > +	reinit_completion(&controller->done);
> > > +
> > > +	timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
> > > +	timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
> > > +	timeout = 100 * msecs_to_jiffies(timeout);
> > > +
> > > +	controller->rx_bytes = 0;
> > > +	controller->tx_bytes = 0;
> > > +	controller->error = 0;
> > > +	controller->xfer = xfer;
> > > +
> > > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > > +		dev_warn(controller->dev, "cannot set RUN state\n");
> > > +		goto exit;
> > > +	}
> > > +
> > > +	if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
> > > +		dev_warn(controller->dev, "cannot set PAUSE state\n");
> > > +		goto exit;
> > > +	}
> > > +
> > > +	spi_qup_fifo_write(controller, xfer);
> > > +
> > > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > > +		dev_warn(controller->dev, "cannot set EXECUTE state\n");
> > > +		goto exit;
> > > +	}
> > > +
> > > +	if (!wait_for_completion_timeout(&controller->done, timeout))
> > > +		ret = -ETIMEDOUT;
> > > +	else
> > > +		ret = controller->error;
> > > +exit:
> > > +	controller->xfer = NULL;
> > 
> > Should the manipulation of controller->xfer be protected by spinlock?
> 
> :-). Probably. I am wondering, could I avoid locking if firstly place
> QUP into RESET state and then access these field. This should stop
> all activities in it, right?

It's generally safest to not assume the hardware is going to do sane things.
I'm concerned about spurious IRQs.

> > 
> > > +	controller->error = 0;
> > > +	controller->rx_bytes = 0;
> > > +	controller->tx_bytes = 0;
> > > +	spi_qup_set_state(controller, QUP_STATE_RESET);
> > > +	return ret;
> > > +}
> > > +
> 
> <snip>
> 
> > > +
> > > +/* set clock freq, clock ramp, bits per work */
> > > +static int spi_qup_io_setup(struct spi_device *spi,
> > > +			  struct spi_transfer *xfer)
> > > +{
> 
> <snip>
> 
> > > +
> > > +	/*
> > > +	 * TODO: In BAM mode mask INPUT and OUTPUT service flags in
> > > +	 * to prevent IRQs on FIFO status change.
> > > +	 */
> > 
> > Remove the TODO.  Not necessary.  This stuff can be added when it becomes BAM
> > enabled.
> 
> Ok. 
> 
> > 
> > > +	writel_relaxed(0, controller->base + QUP_OPERATIONAL_MASK);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int spi_qup_transfer_one(struct spi_master *master,
> > > +				struct spi_message *msg)
> > > +{
> > > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > > +	struct spi_qup_device *chip = spi_get_ctldata(msg->spi);
> > > +	struct spi_transfer *xfer;
> > > +	struct spi_device *spi;
> > > +	unsigned cs_change;
> > > +	int status;
> > > +
> > > +	spi = msg->spi;
> > > +	cs_change = 1;
> > > +	status = 0;
> > > +
> > > +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> > > +
> > > +		status = spi_qup_io_setup(spi, xfer);
> > > +		if (status)
> > > +			break;
> > > +
> > 
> > no locking?  This whole code block needs to have some type of mutex_lock to keep
> > others from trouncing the hardware while you are doing this transfer.
> 
> This is handled by SPI framework.
> 

Ah I looked through that and didn't see it the first time.  But looking again, I
see it.  You're right, you can ignore this comment.

> > 
> > > +		if (cs_change)
> > > +			spi_qup_assert_cs(controller, chip);
> > 
> > Should the CS be done outside the loop?  I'd expect the following sequence to
> > happen:
> > - change CS
> > - Loop and do some transfers
> > - deassert CS
> > 
> > In this code, you reinitialize and assert/deassert CS for every transaction.
> > 
> > > +
> > > +		cs_change = xfer->cs_change;
> 
> 
> Not exactly. It is allowed that CS goes inactive after every
> transaction. This is how I read struct spi_transfer description.

Ah ok.  This is fine then.

> 
> > > +
> > > +		/* Do actual transfer */
> > > +		status = spi_qup_transfer_do(controller, chip, xfer);
> > > +		if (status)
> > > +			break;
> > > +
> > > +		msg->actual_length += xfer->len;
> > > +
> > > +		if (xfer->delay_usecs)
> > > +			udelay(xfer->delay_usecs);
> > > +
> > > +		if (cs_change)
> > > +			spi_qup_deassert_cs(controller, chip);
> > > +	}
> > > +
> > > +	if (status || !cs_change)
> > > +		spi_qup_deassert_cs(controller, chip);
> > > +
> > > +	msg->status = status;
> > > +	spi_finalize_current_message(master);
> > > +	return status;
> > > +}
> > > +
> > > +static int spi_qup_probe(struct platform_device *pdev)
> > > +{
> > > +	struct spi_master *master;
> > > +	struct clk *iclk, *cclk;
> > > +	struct spi_qup *controller;
> > > +	struct resource *res;
> > > +	struct device *dev;
> > > +	void __iomem *base;
> > > +	u32 data, max_freq, iomode;
> > > +	int ret, irq, size;
> > > +
> > > +	dev = &pdev->dev;
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	base = devm_ioremap_resource(dev, res);
> > > +	if (IS_ERR(base))
> > > +		return PTR_ERR(base);
> > > +
> > > +	irq = platform_get_irq(pdev, 0);
> > > +
> > > +	if (irq < 0)
> > > +		return irq;
> > > +
> > > +	cclk = devm_clk_get(dev, "core");
> > > +	if (IS_ERR(cclk)) {
> > > +		dev_err(dev, "cannot get core clock\n");
> > No need to error print.  devm_clk_get already outputs something
> 
> Ok.
> 
> > > +		return PTR_ERR(cclk);
> > > +	}
> > > +
> > > +	iclk = devm_clk_get(dev, "iface");
> > > +	if (IS_ERR(iclk)) {
> > > +		dev_err(dev, "cannot get iface clock\n");
> > 
> > No need to error print.  devm_clk_get already outputs something
> 
> Ok.
> 
> > 
> > > +		return PTR_ERR(iclk);
> > > +	}
> > > +
> > > +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> > > +		max_freq = 19200000;
> > 
> > I'd set the default to 50MHz as that is the max supported by hardware.  I'd just
> > set max_freq declaration to 50MHz and then check the value if it is changed via
> > DT.
> 
> 50MHz doesn't seems to be supported on all chip sets. Currently common
> denominator on all chip sets, that I can see, is 19.2MHz. I have tried 
> to test it with more than 19.2MHz on APQ8074 and it fails.
> 

I guess my stance is to set it to the hardware max supported frequency if it is
not specified.  If that needs to be lower on a board because of whatever reason,
they override it.

> > 
> > > +
> > > +	if (!max_freq) {
> > > +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> > > +		return -ENXIO;
> > > +	}
> > 
> > This is buggy.  Remove this and collapse into the of_property_read_u32 if
> > statement.  On non-zero, check the range for validity.
> 
> True. Will fix.
> 
> > 
> > > +
> > > +	ret = clk_set_rate(cclk, max_freq);
> > > +	if (ret)
> > > +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> > 
> > Bail here?
> 
> I don't know. What will be the consequences if controller continue to
> operate on its default rate?
> 

It is unclear.  But if you can't set the rate that is configured or if there is
a misconfiguration, it's probably better to exit the probe and catch it here.

> > > +
> > > +	ret = clk_prepare_enable(cclk);
> > > +	if (ret) {
> > > +		dev_err(dev, "cannot enable core clock\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(iclk);
> > > +	if (ret) {
> > > +		clk_disable_unprepare(cclk);
> > > +		dev_err(dev, "cannot enable iface clock\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	data = readl_relaxed(base + QUP_HW_VERSION);
> > > +
> > > +	if (data < QUP_HW_VERSION_2_1_1) {
> > > +		clk_disable_unprepare(cclk);
> > > +		clk_disable_unprepare(iclk);
> > > +		dev_err(dev, "v.%08x is not supported\n", data);
> > > +		return -ENXIO;
> > > +	}
> > > +
> > > +	master = spi_alloc_master(dev, sizeof(struct spi_qup));
> > > +	if (!master) {
> > > +		clk_disable_unprepare(cclk);
> > > +		clk_disable_unprepare(iclk);
> > > +		dev_err(dev, "cannot allocate master\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	master->bus_num = pdev->id;
> > > +	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
> > > +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> > > +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> > > +	master->setup = spi_qup_setup;
> > > +	master->cleanup = spi_qup_cleanup;
> > > +	master->transfer_one_message = spi_qup_transfer_one;
> > > +	master->dev.of_node = pdev->dev.of_node;
> > > +	master->auto_runtime_pm = true;
> > 
> > Remove this.  No runtime support
> > 
> > > +
> > > +	platform_set_drvdata(pdev, master);
> > > +
> > > +	controller = spi_master_get_devdata(master);
> > > +
> > > +	controller->dev  = dev;
> > > +	controller->base = base;
> > > +	controller->iclk = iclk;
> > > +	controller->cclk = cclk;
> > > +	controller->irq  = irq;
> > > +	controller->max_speed_hz = clk_get_rate(cclk);
> > > +	controller->speed_hz = controller->max_speed_hz;
> > > +
> > > +	init_completion(&controller->done);
> > > +
> > > +	iomode = readl_relaxed(base + QUP_IO_M_MODES);
> > > +
> > > +	size = QUP_IO_M_OUTPUT_BLOCK_SIZE(iomode);
> > > +	if (size)
> > > +		controller->out_blk_sz = size * 16;
> > > +	else
> > > +		controller->out_blk_sz = 4;
> > > +
> > > +	size = QUP_IO_M_INPUT_BLOCK_SIZE(iomode);
> > > +	if (size)
> > > +		controller->in_blk_sz = size * 16;
> > > +	else
> > > +		controller->in_blk_sz = 4;
> > > +
> > > +	size = QUP_IO_M_OUTPUT_FIFO_SIZE(iomode);
> > > +	controller->out_fifo_sz = controller->out_blk_sz * (2 << size);
> > > +
> > > +	size = QUP_IO_M_INPUT_FIFO_SIZE(iomode);
> > > +	controller->in_fifo_sz = controller->in_blk_sz * (2 << size);
> > > +
> > > +	dev_info(dev, "v.%08x IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
> > > +		 data, controller->in_blk_sz, controller->in_fifo_sz,
> > > +		 controller->out_blk_sz, controller->out_fifo_sz);
> > > +
> > > +	writel_relaxed(1, base + QUP_SW_RESET);
> > > +
> > > +	ret = spi_qup_set_state(controller, QUP_STATE_RESET);
> > > +	if (ret) {
> > > +		dev_err(dev, "cannot set RESET state\n");
> > > +		goto error;
> > > +	}
> > > +
> > > +	writel_relaxed(0, base + QUP_OPERATIONAL);
> > > +	writel_relaxed(0, base + QUP_IO_M_MODES);
> > > +	writel_relaxed(0, base + QUP_OPERATIONAL_MASK);
> > > +	writel_relaxed(SPI_ERROR_CLK_UNDER_RUN | SPI_ERROR_CLK_OVER_RUN,
> > > +		       base + SPI_ERROR_FLAGS_EN);
> > > +
> > > +	writel_relaxed(0, base + SPI_CONFIG);
> > > +	writel_relaxed(SPI_IO_C_NO_TRI_STATE, base + SPI_IO_CONTROL);
> > > +
> > > +	ret = devm_request_irq(dev, irq, spi_qup_qup_irq,
> > > +			       IRQF_TRIGGER_HIGH, pdev->name, controller);
> > > +	if (ret) {
> > > +		dev_err(dev, "cannot request IRQ %d\n", irq);
> > 
> > unnecessary print
> 
> Will remove.
> 
> > 
> > > +		goto error;
> > > +	}
> > > +
> > > +	ret = devm_spi_register_master(dev, master);
> > > +	if (!ret) {
> > > +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> > > +		pm_runtime_use_autosuspend(dev);
> > > +		pm_runtime_set_active(dev);
> > > +		pm_runtime_enable(dev);
> > 
> > Remove all the runtime stuff.  not supported right now.
> > 
> > > +		return ret;
> > > +	}
> > > +error:
> > > +	clk_disable_unprepare(cclk);
> > > +	clk_disable_unprepare(iclk);
> > > +	spi_master_put(master);
> > > +	return ret;
> > > +}
> > > +
> 
> <snip>
> 
> > 
> > > +
> > > +static int spi_qup_remove(struct platform_device *pdev)
> > > +{
> > > +	struct spi_master *master = dev_get_drvdata(&pdev->dev);
> > > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > > +
> > > +	pm_runtime_get_sync(&pdev->dev);
> > > +
> > 
> > Do we need to wait for any current transactions to complete
> > and do a devm_free_irq()?
> > 
> > > +	clk_disable_unprepare(controller->cclk);
> > > +	clk_disable_unprepare(controller->iclk);
> 
> My understanding is:
> 
> Disabling clocks will timeout transaction, if any. Core Device driver
> will call: devm_spi_unregister(), which will wait pending transactions
> to complete and then remove the SPI master.

Disabling clocks will confuse the hardware.  We cannot disable clocks while the
spi core is active and transferring data.

> 
> > > +
> > > +	pm_runtime_put_noidle(&pdev->dev);
> > > +	pm_runtime_disable(&pdev->dev);
> > > +	return 0;
> > > +}
> > > +
> > > +static struct of_device_id spi_qup_dt_match[] = {
> > > +	{ .compatible = "qcom,spi-qup-v2", },
> > 
> > Need compatible tags of qcom,spi-qup-v2.1.1 (msm8974 v1) or qcom,spi-qup-v2.2.1
> > (msm8974 v2)
> 
> I am not aware of the difference. My board report v.20020000. 
> Is there difference of handling these controllers?

There were some bug fixes between versions.  None of those affect SPI (that I
can tell), but it's better to be more descriptive and use the full versions in
the compatible tags.

> 
> 
> Thanks, 
> Ivan
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Josh Cartwright Feb. 7, 2014, 5:46 p.m. UTC | #11

On Fri, Feb 07, 2014 at 05:31:08PM +0000, Mark Brown wrote:
> On Fri, Feb 07, 2014 at 11:20:51AM -0600, Josh Cartwright wrote:
> > On Fri, Feb 07, 2014 at 05:18:34PM +0000, Mark Brown wrote:
> > > On Fri, Feb 07, 2014 at 10:51:27AM -0600, Josh Cartwright wrote:
> > > > 	config SPI_QUP
> > > > 		tristate "Qualcomm SPI Support with QUP interface"
> > > > 		depends on OF
> > > > 		depends on ARM
> 
> > > Does this really depend on ARM?  If so why?
> 
> > The ARM dependency is there for the use of _relaxed io accessor
> > variants.
> 
> That's not ARM only and I thought we were getting generic versions of it
> anyway?  ARMv8, MIPS, Microblaze, Hexagon and SH also define it.

Okay, that's fair.  I'm only vaguely familiar with the generic _relaxed
variants, but until they land, how do we appropriately declare the
dependency to prevent breaking COMPILE_TEST builds on architectures that
don't have them?  Or should we either bother?

Do we need to introduce a HAVE_RELAXED_IO_ACCESSORS selected by those
architectures with support?

Mark Brown Feb. 7, 2014, 5:52 p.m. UTC | #12

On Fri, Feb 07, 2014 at 11:32:07AM -0600, Andy Gross wrote:
> On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:

To repeat what I said in my earlier e-mail please delete irrelevant
context from your mails so any new content you are including is
discoverable.

> > Did you see any particular issue with the implementation 
> > or this is just because this platform didn't have support 
> > for power management?  

> The platform doesn't have support for PM right now.  So it's probably better to
> remove all this and revisit later when it is in place.

No, runtime PM does not require any platform support at all and is good
practice - look at what the driver is doing with it, it's useful as-is.

Mark Brown Feb. 7, 2014, 5:57 p.m. UTC | #13

On Fri, Feb 07, 2014 at 11:46:43AM -0600, Josh Cartwright wrote:
> On Fri, Feb 07, 2014 at 05:31:08PM +0000, Mark Brown wrote:

> > That's not ARM only and I thought we were getting generic versions of it
> > anyway?  ARMv8, MIPS, Microblaze, Hexagon and SH also define it.

> Okay, that's fair.  I'm only vaguely familiar with the generic _relaxed
> variants, but until they land, how do we appropriately declare the
> dependency to prevent breaking COMPILE_TEST builds on architectures that
> don't have them?  Or should we either bother?

> Do we need to introduce a HAVE_RELAXED_IO_ACCESSORS selected by those
> architectures with support?

I think that or just getting generic versions done would be the way
forwards.  Right now it's a bit of a shambles.

Andy Gross Feb. 7, 2014, 7:12 p.m. UTC | #14

On Fri, Feb 07, 2014 at 05:52:34PM +0000, Mark Brown wrote:
> On Fri, Feb 07, 2014 at 11:32:07AM -0600, Andy Gross wrote:
> > On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:
> 

[... snip ...]

> > The platform doesn't have support for PM right now.  So it's probably better to
> > remove all this and revisit later when it is in place.
> 
> No, runtime PM does not require any platform support at all and is good
> practice - look at what the driver is doing with it, it's useful as-is.

Fair enough.

Ivan T. Ivanov Feb. 10, 2014, 3:54 p.m. UTC | #15

Hi Daniel,

On Fri, 2014-02-07 at 16:34 +0000, [e-mail] wrote: 
> > From: "Ivan T. Ivanov" <[e-mail]>


<snip>

> > +
> > +static int spi_qup_set_state(struct spi_qup *controller, u32 state)
> > +{
> > +       unsigned long loop = 0;
> > +       u32 cur_state;
> > +
> > +       cur_state = readl_relaxed(controller->base + QUP_STATE);
> Make sure the state is valid before you read the current state.

Why? Controller is always left in valid state (after probe and every
transaction)? I know that CAF code contain this check, but now driver
is little bit different. I have made some tests and controller is
always in valid state in this point.

> > +       /*
> > +        * Per spec: for PAUSE_STATE to RESET_STATE, two writes
> > +        * of (b10) are required
> > +        */
> > +       if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
> > +           (state == QUP_STATE_RESET)) {
> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
> > QUP_STATE);
> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
> > QUP_STATE);
> > +       } else {
> Make sure you don't transition from RESET to PAUSE.

I don't see this to be handled in CAF code. Could you give me
more details, please?

Right now possible state transactions are:

* if everything is fine:
  RESET -> RUN -> PAUSE -> RUN -> RESET 
* in case of error: 
  RESET -> RUN -> RESET 
  RESET -> RUN -> PAUSE -> RESET

Please correct me if I am wrong.

> > +
> > +static void spi_qup_fifo_read(struct spi_qup *controller,
> > +                             struct spi_transfer *xfer)
> > +{
> > +       u8 *rx_buf = xfer->rx_buf;
> > +       u32 word, state;
> > +       int idx, shift;
> > +
> > +       while (controller->rx_bytes < xfer->len) {
> > +
> > +               state = readl_relaxed(controller->base + QUP_OPERATIONAL);
> > +               if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
> > +                       break;
> > +
> > +               word = readl_relaxed(controller->base + QUP_INPUT_FIFO);
> > +
> > +               for (idx = 0; idx < controller->bytes_per_word &&
> > +                    controller->rx_bytes < xfer->len; idx++,
> > +                    controller->rx_bytes++) {
> > +
> > +                       if (!rx_buf)
> > +                               continue;
> If there is no rx_buf just set rx_bytes to xfer->len and skip the loop
> entirely.

Well, FIFO buffer still should be drained, right?
Anyway. I am looking for better way to handle this.
Same applies for filling FIFO buffer.

> > +                       /*
> > +                        * The data format depends on bytes_per_word:
> > +                        *  4 bytes: 0x12345678
> > +                        *  2 bytes: 0x00001234
> > +                        *  1 byte : 0x00000012
> > +                        */
> > +                       shift = BITS_PER_BYTE;
> > +                       shift *= (controller->bytes_per_word - idx - 1);
> > +                       rx_buf[controller->rx_bytes] = word >> shift;
> > +               }
> > +       }
> > +}

<snip>

> > +static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
> > +{
> > +       struct spi_qup *controller = dev_id;
> > +       struct spi_transfer *xfer;
> > +       u32 opflags, qup_err, spi_err;
> > +
> > +       xfer = controller->xfer;
> > +
> > +       qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> > +       spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> > +       opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> > +
> > +       writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> > +       writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> > +       writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> > +
> > +       if (!xfer)
> > +               return IRQ_HANDLED;
> > +
> > +       if (qup_err) {
> > +               if (qup_err & QUP_ERROR_OUTPUT_OVER_RUN)
> > +                       dev_warn(controller->dev, "OUTPUT_OVER_RUN\n");
> > +               if (qup_err & QUP_ERROR_INPUT_UNDER_RUN)
> > +                       dev_warn(controller->dev, "INPUT_UNDER_RUN\n");
> > +               if (qup_err & QUP_ERROR_OUTPUT_UNDER_RUN)
> > +                       dev_warn(controller->dev, "OUTPUT_UNDER_RUN\n");
> > +               if (qup_err & QUP_ERROR_INPUT_OVER_RUN)
> > +                       dev_warn(controller->dev, "INPUT_OVER_RUN\n");
> > +
> > +               controller->error = -EIO;
> > +       }
> > +
> > +       if (spi_err) {
> > +               if (spi_err & SPI_ERROR_CLK_OVER_RUN)
> > +                       dev_warn(controller->dev, "CLK_OVER_RUN\n");
> > +               if (spi_err & SPI_ERROR_CLK_UNDER_RUN)
> > +                       dev_warn(controller->dev, "CLK_UNDER_RUN\n");
> > +
> > +               controller->error = -EIO;
> > +       }
> > +
> > +       if (opflags & QUP_OP_IN_SERVICE_FLAG) {
> > +               writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
> > +                      controller->base + QUP_OPERATIONAL);
> Write is not necessary since already cleared above.
> > +               spi_qup_fifo_read(controller, xfer);
> > +       }
> > +
> > +       if (opflags & QUP_OP_OUT_SERVICE_FLAG) {
> > +               writel_relaxed(QUP_OP_OUT_SERVICE_FLAG,
> > +                      controller->base + QUP_OPERATIONAL);
> Write is not necessary since already cleared above.

True. I have forgot to remove these writes. Will fix.

Regards,
Ivan

> > +               spi_qup_fifo_write(controller, xfer);
> > +       }
> > +
> > +       if (controller->rx_bytes == xfer->len ||
> > +           controller->error)
> > +               complete(&controller->done);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[e-mail] Feb. 10, 2014, 4:21 p.m. UTC | #16

Hi Ivan,
>
>> > +
>> > +static int spi_qup_set_state(struct spi_qup *controller, u32 state)
>> > +{
>> > +       unsigned long loop = 0;
>> > +       u32 cur_state;
>> > +
>> > +       cur_state = readl_relaxed(controller->base + QUP_STATE);
>> Make sure the state is valid before you read the current state.
>
> Why? Controller is always left in valid state (after probe and every
> transaction)? I know that CAF code contain this check, but now driver
> is little bit different. I have made some tests and controller is
> always in valid state in this point.

The hardware programming guide we recommends doing this.  I'd have to talk
to the hardware designers to know exactly the reason why.  I can follow up
on this if you'd like.

>
>> > +       /*
>> > +        * Per spec: for PAUSE_STATE to RESET_STATE, two writes
>> > +        * of (b10) are required
>> > +        */
>> > +       if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
>> > +           (state == QUP_STATE_RESET)) {
>> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
>> > QUP_STATE);
>> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
>> > QUP_STATE);
>> > +       } else {
>> Make sure you don't transition from RESET to PAUSE.
>
> I don't see this to be handled in CAF code. Could you give me
> more details, please?
>
> Right now possible state transactions are:
>
> * if everything is fine:
>   RESET -> RUN -> PAUSE -> RUN -> RESET
> * in case of error:
>   RESET -> RUN -> RESET
>   RESET -> RUN -> PAUSE -> RESET
>
> Please correct me if I am wrong.

According to the hardware documentation if the hardware is in the RESET
state and we try to transition to the PAUSE state the hardware behavior is
"undefined", which usually means bad things will happen.  Admittedly, if
the driver always follows the "valid" rules (the ones you've listed above)
it _should_ never happen.  However, it is _possible_ the hardware is in
the RESET state while the driver thinks it's in the RUN state and the
driver tries to put is in the PAUSE state.  In other words, I'd like to
err on the side of caution since the check doesn't really cost us
anything.

>
>> > +
>> > +static void spi_qup_fifo_read(struct spi_qup *controller,
>> > +                             struct spi_transfer *xfer)
>> > +{
>> > +       u8 *rx_buf = xfer->rx_buf;
>> > +       u32 word, state;
>> > +       int idx, shift;
>> > +
>> > +       while (controller->rx_bytes < xfer->len) {
>> > +
>> > +               state = readl_relaxed(controller->base +
>> QUP_OPERATIONAL);
>> > +               if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
>> > +                       break;
>> > +
>> > +               word = readl_relaxed(controller->base +
>> QUP_INPUT_FIFO);
>> > +
>> > +               for (idx = 0; idx < controller->bytes_per_word &&
>> > +                    controller->rx_bytes < xfer->len; idx++,
>> > +                    controller->rx_bytes++) {
>> > +
>> > +                       if (!rx_buf)
>> > +                               continue;
>> If there is no rx_buf just set rx_bytes to xfer->len and skip the loop
>> entirely.
>
> Well, FIFO buffer still should be drained, right?
> Anyway. I am looking for better way to handle this.
> Same applies for filling FIFO buffer.
>

Yes.  Still read from the FIFO but skip the for loop since we're just
adding bytes_per_word to the total rx_byte count one iteration at a time
and not doing anything with the data.

Thanks,
Dan


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivan T. Ivanov Feb. 10, 2014, 4:29 p.m. UTC | #17

Hi,

On Fri, 2014-02-07 at 17:12 +0000, Mark Brown wrote: 
> On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> 
> This looks mostly good, there's a few odd things and missing use of
> framework features.
> 
> > Qualcomm Universal Peripheral (QUP) core is an 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
> > support up to 50MHz, up to four chip selects, and a programmable
> > data path from 4 bits to 32 bits; MODE0..3 protocols
> 
> The grammar in this and the Kconfig text is a bit garbled, might want to
> give it a once over (support -> supports for example).

Ok

> 
> > +static void spi_qup_deassert_cs(struct spi_qup *controller,
> > +				struct spi_qup_device *chip)
> > +{
> 
> 
> > +	if (chip->mode & SPI_CS_HIGH)
> > +		iocontol &= ~mask;
> > +	else
> > +		iocontol |= mask;
> 
> Implement a set_cs() operation and let the core worry about all this
> for you as well as saving two implementations.

Nice. Will us it.

> 
> > +		word = 0;
> > +		for (idx = 0; idx < controller->bytes_per_word &&
> > +		     controller->tx_bytes < xfer->len; idx++,
> > +		     controller->tx_bytes++) {
> > +
> > +			if (!tx_buf)
> > +				continue;
> 
> Do you need to set the _MUST_TX flag?
> 
> > +	qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> > +	spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> > +	opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> > +
> > +	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> > +	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> > +	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> > +
> > +	if (!xfer)
> > +		return IRQ_HANDLED;
> 
> Are you sure?  It seems wrong to just ignore interrupts, some comments
> would help explain why.

Of course this should never happen. This was left from initial stage
of the implementation.

> 
> > +static int spi_qup_transfer_do(struct spi_qup *controller,
> > +			       struct spi_qup_device *chip,
> > +			       struct spi_transfer *xfer)
> 
> This looks like a transfer_one() function, please use the framework
> features where you can.

Sure, will do. Somehow I have missed this.

> 
> > +	if (controller->speed_hz != chip->speed_hz) {
> > +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> > +		if (ret) {
> > +			dev_err(controller->dev, "fail to set frequency %d",
> > +				chip->speed_hz);
> > +			return -EIO;
> > +		}
> > +	}
> 
> Is calling into the clock framework really so expensive that we need to
> avoid doing it?  

Probably not. But why to call it if the frequency is the same.


> You also shouldn't be interacting with the hardware in
> setup().

This is internal hw-setup, not master::setup() or I am
missing something?

> 
> > +	if (chip->bits_per_word <= 8)
> > +		controller->bytes_per_word = 1;
> > +	else if (chip->bits_per_word <= 16)
> > +		controller->bytes_per_word = 2;
> > +	else
> > +		controller->bytes_per_word = 4;
> 
> This looks like a switch statement, and looking at the above it's not
> clear that the device actually supports anything other than whole bytes.
> I'm not sure what that would mean from an API point of view.

SPI API didn't validate struct spi_transfer::len field.

The whole sniped looks like this:

	chip->bits_per_word = spi->bits_per_word;
	if (xfer->bits_per_word)
		chip->bits_per_word = xfer->bits_per_word;

	if (chip->bits_per_word <= 8)
		controller->bytes_per_word = 1;
	else if (chip->bits_per_word <= 16)
		controller->bytes_per_word = 2;
	else
		controller->bytes_per_word = 4;

	if (controller->bytes_per_word > xfer->len ||
	    xfer->len % controller->bytes_per_word != 0){
		/* No partial transfers */
		dev_err(controller->dev, "invalid len %d for %d bits\n",
			xfer->len, chip->bits_per_word);
		return -EIO;
	}

	n_words = xfer->len / controller->bytes_per_word;

'bytes_per_word' have to be power of 2. This is my understanding of
struct spi_transfer description. So I am discarding all transfers with
'len' non multiple of word size.


> 
> > +static int spi_qup_transfer_one(struct spi_master *master,
> > +				struct spi_message *msg)
> > +{
> 
> This entire function can be removed, the core can do it for you.

Sure, will use it.

> 
> > +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> > +		max_freq = 19200000;
> > +
> > +	if (!max_freq) {
> > +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> > +		return -ENXIO;
> > +	}
> > +
> > +	ret = clk_set_rate(cclk, max_freq);
> > +	if (ret)
> > +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> 
> You set the clock rate per transfer so why bother setting it here,

Only if differs from the current one.

> perhaps we support the rate the devices request but not this maximum
> rate?

Thats why it is just a warning. I will see how to handle this better.

> 
> > +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> > +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> 
> Are you *sure* the device supports anything other than whole bytes?

Yep. I see them on the scope.

> 
> > +	ret = devm_spi_register_master(dev, master);
> > +	if (!ret) {
> > +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> > +		pm_runtime_use_autosuspend(dev);
> > +		pm_runtime_set_active(dev);
> > +		pm_runtime_enable(dev);
> > +		return ret;
> > +	}
> 
> This is really unclearly written, the success case looks like error
> handling.

I suppose that if use a goto, I will have to do it consistently. 
Will rework it.

> 
> > +#ifdef CONFIG_PM_RUNTIME
> > +static int spi_qup_pm_suspend_runtime(struct device *device)
> > +{
> > +	struct spi_master *master = dev_get_drvdata(device);
> > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > +
> > +	disable_irq(controller->irq);
> 
> Why do you need to disable the interrupt?  Will the hardware generate
> spurious interrupts, if so some documentation is in order.

I don't know. Just extra protection? I will have t o find how to 
test this.

> 
> > +static int spi_qup_pm_resume_runtime(struct device *device)
> > +{
> > +	struct spi_master *master = dev_get_drvdata(device);
> > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > +
> > +	clk_prepare_enable(controller->cclk);
> > +	clk_prepare_enable(controller->iclk);
> > +	enable_irq(controller->irq);
> 
> No error checking here...

Will fix.

Thanks,
Ivan


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivan T. Ivanov Feb. 10, 2014, 4:55 p.m. UTC | #18

Hi Andy,

On Fri, 2014-02-07 at 11:32 -0600, Andy Gross wrote: 
> On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:
> > 

<snip>

> > > > +static int spi_qup_transfer_do(struct spi_qup *controller,
> > > > +			       struct spi_qup_device *chip,
> > > > +			       struct spi_transfer *xfer)
> > > > +{
> > > > +	unsigned long timeout;
> > > > +	int ret = -EIO;
> > > > +
> > > > +	reinit_completion(&controller->done);
> > > > +
> > > > +	timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
> > > > +	timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
> > > > +	timeout = 100 * msecs_to_jiffies(timeout);
> > > > +
> > > > +	controller->rx_bytes = 0;
> > > > +	controller->tx_bytes = 0;
> > > > +	controller->error = 0;
> > > > +	controller->xfer = xfer;
> > > > +
> > > > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > > > +		dev_warn(controller->dev, "cannot set RUN state\n");
> > > > +		goto exit;
> > > > +	}
> > > > +
> > > > +	if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
> > > > +		dev_warn(controller->dev, "cannot set PAUSE state\n");
> > > > +		goto exit;
> > > > +	}
> > > > +
> > > > +	spi_qup_fifo_write(controller, xfer);
> > > > +
> > > > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > > > +		dev_warn(controller->dev, "cannot set EXECUTE state\n");
> > > > +		goto exit;
> > > > +	}
> > > > +
> > > > +	if (!wait_for_completion_timeout(&controller->done, timeout))
> > > > +		ret = -ETIMEDOUT;
> > > > +	else
> > > > +		ret = controller->error;
> > > > +exit:
> > > > +	controller->xfer = NULL;
> > > 
> > > Should the manipulation of controller->xfer be protected by spinlock?
> > 
> > :-). Probably. I am wondering, could I avoid locking if firstly place
> > QUP into RESET state and then access these field. This should stop
> > all activities in it, right?
> 
> It's generally safest to not assume the hardware is going to do sane things.
> I'm concerned about spurious IRQs.

Ok, will add protection. 

<snip>

> > > > +
> > > > +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> > > > +		max_freq = 19200000;
> > > 
> > > I'd set the default to 50MHz as that is the max supported by hardware.  I'd just
> > > set max_freq declaration to 50MHz and then check the value if it is changed via
> > > DT.
> > 
> > 50MHz doesn't seems to be supported on all chip sets. Currently common
> > denominator on all chip sets, that I can see, is 19.2MHz. I have tried 
> > to test it with more than 19.2MHz on APQ8074 and it fails.
> > 
> 
> I guess my stance is to set it to the hardware max supported frequency if it is
> not specified.  If that needs to be lower on a board because of whatever reason,
> they override it.

Ok, I will do in this way.

<snip>

> > > 
> > > > +
> > > > +	ret = clk_set_rate(cclk, max_freq);
> > > > +	if (ret)
> > > > +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> > > 
> > > Bail here?
> > 
> > I don't know. What will be the consequences if controller continue to
> > operate on its default rate?
> > 
> 
> It is unclear.  But if you can't set the rate that is configured or if there is
> a misconfiguration, it's probably better to exit the probe and catch it here.


My preference is to delay clock speed change till first
SPI transfer. And use wherever transfer itself mandate.

<snip>

> > > > +
> > > > +static int spi_qup_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct spi_master *master = dev_get_drvdata(&pdev->dev);
> > > > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > > > +
> > > > +	pm_runtime_get_sync(&pdev->dev);
> > > > +
> > > 
> > > Do we need to wait for any current transactions to complete
> > > and do a devm_free_irq()?
> > > 
> > > > +	clk_disable_unprepare(controller->cclk);
> > > > +	clk_disable_unprepare(controller->iclk);
> > 
> > My understanding is:
> > 
> > Disabling clocks will timeout transaction, if any. Core Device driver
> > will call: devm_spi_unregister(), which will wait pending transactions
> > to complete and then remove the SPI master.
> 
> Disabling clocks will confuse the hardware.  We cannot disable clocks while the
> spi core is active and transferring data.

I could follow approach taken by other SPI drivers, just reset
controller and disable clocks.

> 
> > 
> > > > +
> > > > +	pm_runtime_put_noidle(&pdev->dev);
> > > > +	pm_runtime_disable(&pdev->dev);
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static struct of_device_id spi_qup_dt_match[] = {
> > > > +	{ .compatible = "qcom,spi-qup-v2", },
> > > 
> > > Need compatible tags of qcom,spi-qup-v2.1.1 (msm8974 v1) or qcom,spi-qup-v2.2.1
> > > (msm8974 v2)
> > 
> > I am not aware of the difference. My board report v.20020000. 
> > Is there difference of handling these controllers?
> 
> There were some bug fixes between versions.  None of those affect SPI (that I
> can tell), but it's better to be more descriptive and use the full versions in
> the compatible tags.

No strong preference here. Should I add qcom,spi-qup-v2.2.0, then? :-)

Regards,
Ivan


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Brown Feb. 10, 2014, 5:09 p.m. UTC | #19

On Mon, Feb 10, 2014 at 06:29:26PM +0200, Ivan T. Ivanov wrote:
> On Fri, 2014-02-07 at 17:12 +0000, Mark Brown wrote: 

> > > +	if (!xfer)
> > > +		return IRQ_HANDLED;

> > Are you sure?  It seems wrong to just ignore interrupts, some comments
> > would help explain why.

> Of course this should never happen. This was left from initial stage
> of the implementation.

OK, so reporting them as errors seems better then.

> > > +	if (controller->speed_hz != chip->speed_hz) {
> > > +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> > > +		if (ret) {
> > > +			dev_err(controller->dev, "fail to set frequency %d",
> > > +				chip->speed_hz);
> > > +			return -EIO;
> > > +		}
> > > +	}

> > Is calling into the clock framework really so expensive that we need to
> > avoid doing it?  

> Probably not. But why to call it if the frequency is the same.

It's less code that could go wrong and the check is already there in the
clock framework hopefully.

> > You also shouldn't be interacting with the hardware in
> > setup().

> This is internal hw-setup, not master::setup() or I am
> missing something?

The naming could probably be clearer then - config or something.

> > > +	if (chip->bits_per_word <= 8)
> > > +		controller->bytes_per_word = 1;
> > > +	else if (chip->bits_per_word <= 16)
> > > +		controller->bytes_per_word = 2;
> > > +	else
> > > +		controller->bytes_per_word = 4;

> > This looks like a switch statement, and looking at the above it's not
> > clear that the device actually supports anything other than whole bytes.
> > I'm not sure what that would mean from an API point of view.

> SPI API didn't validate struct spi_transfer::len field.

It's supposed to; if the validation is incomplete then that should be
fixed.

Ivan T. Ivanov Feb. 10, 2014, 5:11 p.m. UTC | #20

Hi Dan, 

On Mon, 2014-02-10 at 16:21 +0000, [e-mail] wrote: 
> Hi Ivan,
> >
> >> > +
> >> > +static int spi_qup_set_state(struct spi_qup *controller, u32 state)
> >> > +{
> >> > +       unsigned long loop = 0;
> >> > +       u32 cur_state;
> >> > +
> >> > +       cur_state = readl_relaxed(controller->base + QUP_STATE);
> >> Make sure the state is valid before you read the current state.
> >
> > Why? Controller is always left in valid state (after probe and every
> > transaction)? I know that CAF code contain this check, but now driver
> > is little bit different. I have made some tests and controller is
> > always in valid state in this point.
> 
> The hardware programming guide we recommends doing this.  I'd have to talk
> to the hardware designers to know exactly the reason why.  I can follow up
> on this if you'd like.
> 

Ok, thanks. I could add it back. Please, could you point me to place
in driver where this could happen.


> >
> >> > +       /*
> >> > +        * Per spec: for PAUSE_STATE to RESET_STATE, two writes
> >> > +        * of (b10) are required
> >> > +        */
> >> > +       if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
> >> > +           (state == QUP_STATE_RESET)) {
> >> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
> >> > QUP_STATE);
> >> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
> >> > QUP_STATE);
> >> > +       } else {
> >> Make sure you don't transition from RESET to PAUSE.
> >
> > I don't see this to be handled in CAF code. Could you give me
> > more details, please?
> >
> > Right now possible state transactions are:
> >
> > * if everything is fine:
> >   RESET -> RUN -> PAUSE -> RUN -> RESET
> > * in case of error:
> >   RESET -> RUN -> RESET
> >   RESET -> RUN -> PAUSE -> RESET
> >
> > Please correct me if I am wrong.
> 
> According to the hardware documentation if the hardware is in the RESET
> state and we try to transition to the PAUSE state the hardware behavior is
> "undefined", which usually means bad things will happen.  Admittedly, if
> the driver always follows the "valid" rules (the ones you've listed above)
> it _should_ never happen.  However, it is _possible_ the hardware is in
> the RESET state while the driver thinks it's in the RUN state and the
> driver tries to put is in the PAUSE state.  In other words, I'd like to
> err on the side of caution since the check doesn't really cost us
> anything.

Ok that is fine, but did you see where/how this could happen in
the current implementation. If this could happen I will like to fix it.

> 
> >
> >> > +
> >> > +static void spi_qup_fifo_read(struct spi_qup *controller,
> >> > +                             struct spi_transfer *xfer)
> >> > +{
> >> > +       u8 *rx_buf = xfer->rx_buf;
> >> > +       u32 word, state;
> >> > +       int idx, shift;
> >> > +
> >> > +       while (controller->rx_bytes < xfer->len) {
> >> > +
> >> > +               state = readl_relaxed(controller->base +
> >> QUP_OPERATIONAL);
> >> > +               if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
> >> > +                       break;
> >> > +
> >> > +               word = readl_relaxed(controller->base +
> >> QUP_INPUT_FIFO);
> >> > +
> >> > +               for (idx = 0; idx < controller->bytes_per_word &&
> >> > +                    controller->rx_bytes < xfer->len; idx++,
> >> > +                    controller->rx_bytes++) {
> >> > +
> >> > +                       if (!rx_buf)
> >> > +                               continue;
> >> If there is no rx_buf just set rx_bytes to xfer->len and skip the loop
> >> entirely.
> >
> > Well, FIFO buffer still should be drained, right?
> > Anyway. I am looking for better way to handle this.
> > Same applies for filling FIFO buffer.
> >
> 
> Yes.  Still read from the FIFO but skip the for loop since we're just
> adding bytes_per_word to the total rx_byte count one iteration at a time
> and not doing anything with the data.
> 

My point was: If I made rx_bytes equal xfer->len, outer while loop will
be terminated before FIFO was drained :-). I will see how to handle
this better.

Thanks,
Ivan

> Thanks,
> Dan
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[e-mail] Feb. 10, 2014, 5:41 p.m. UTC | #21

Hi Ivan,

>> >> > +static int spi_qup_set_state(struct spi_qup *controller, u32
>> state)
>> >> > +{
>> >> > +       unsigned long loop = 0;
>> >> > +       u32 cur_state;
>> >> > +
>> >> > +       cur_state = readl_relaxed(controller->base + QUP_STATE);
>> >> Make sure the state is valid before you read the current state.
>> >
>> > Why? Controller is always left in valid state (after probe and every
>> > transaction)? I know that CAF code contain this check, but now driver
>> > is little bit different. I have made some tests and controller is
>> > always in valid state in this point.
>>
>> The hardware programming guide we recommends doing this.  I'd have to
>> talk
>> to the hardware designers to know exactly the reason why.  I can follow
>> up
>> on this if you'd like.
>>
>
> Ok, thanks. I could add it back. Please, could you point me to place
> in driver where this could happen.
>
>
>> >
>> >> > +       /*
>> >> > +        * Per spec: for PAUSE_STATE to RESET_STATE, two writes
>> >> > +        * of (b10) are required
>> >> > +        */
>> >> > +       if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
>> >> > +           (state == QUP_STATE_RESET)) {
>> >> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
>> >> > QUP_STATE);
>> >> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
>> >> > QUP_STATE);
>> >> > +       } else {
>> >> Make sure you don't transition from RESET to PAUSE.
>> >
>> > I don't see this to be handled in CAF code. Could you give me
>> > more details, please?
>> >
>> > Right now possible state transactions are:
>> >
>> > * if everything is fine:
>> >   RESET -> RUN -> PAUSE -> RUN -> RESET
>> > * in case of error:
>> >   RESET -> RUN -> RESET
>> >   RESET -> RUN -> PAUSE -> RESET
>> >
>> > Please correct me if I am wrong.
>>
>> According to the hardware documentation if the hardware is in the RESET
>> state and we try to transition to the PAUSE state the hardware behavior
>> is
>> "undefined", which usually means bad things will happen.  Admittedly, if
>> the driver always follows the "valid" rules (the ones you've listed
>> above)
>> it _should_ never happen.  However, it is _possible_ the hardware is in
>> the RESET state while the driver thinks it's in the RUN state and the
>> driver tries to put is in the PAUSE state.  In other words, I'd like to
>> err on the side of caution since the check doesn't really cost us
>> anything.
>
> Ok that is fine, but did you see where/how this could happen in
> the current implementation. If this could happen I will like to fix it.

I don't see a problem in the current implementation that would cause you
to get in the RESET state without knowing it.  It's just my paranoia
kicking in.

>
>>
>> >
>> >> > +
>> >> > +static void spi_qup_fifo_read(struct spi_qup *controller,
>> >> > +                             struct spi_transfer *xfer)
>> >> > +{
>> >> > +       u8 *rx_buf = xfer->rx_buf;
>> >> > +       u32 word, state;
>> >> > +       int idx, shift;
>> >> > +
>> >> > +       while (controller->rx_bytes < xfer->len) {
>> >> > +
>> >> > +               state = readl_relaxed(controller->base +
>> >> QUP_OPERATIONAL);
>> >> > +               if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
>> >> > +                       break;
>> >> > +
>> >> > +               word = readl_relaxed(controller->base +
>> >> QUP_INPUT_FIFO);
>> >> > +
>> >> > +               for (idx = 0; idx < controller->bytes_per_word &&
>> >> > +                    controller->rx_bytes < xfer->len; idx++,
>> >> > +                    controller->rx_bytes++) {
>> >> > +
>> >> > +                       if (!rx_buf)
>> >> > +                               continue;
>> >> If there is no rx_buf just set rx_bytes to xfer->len and skip the
>> loop
>> >> entirely.
>> >
>> > Well, FIFO buffer still should be drained, right?
>> > Anyway. I am looking for better way to handle this.
>> > Same applies for filling FIFO buffer.
>> >
>>
>> Yes.  Still read from the FIFO but skip the for loop since we're just
>> adding bytes_per_word to the total rx_byte count one iteration at a time
>> and not doing anything with the data.
>>
>
> My point was: If I made rx_bytes equal xfer->len, outer while loop will
> be terminated before FIFO was drained :-). I will see how to handle
> this better.

You're right!  Had to read that snippet again.  Though, I think we can
skip draining the FIFO by setting the NO_INPUT bit in the QUP_CONFIG
register (and the NO_OUTPUT for writes).



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andy Gross Feb. 10, 2014, 5:47 p.m. UTC | #22

On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:

[....]

> > > > Bail here?
> > > 
> > > I don't know. What will be the consequences if controller continue to
> > > operate on its default rate?
> > > 
> > 
> > It is unclear.  But if you can't set the rate that is configured or if there is
> > a misconfiguration, it's probably better to exit the probe and catch it here.
> 
> 
> My preference is to delay clock speed change till first
> SPI transfer. And use wherever transfer itself mandate.
> 

That works.  My only concern is that it might be nice to catch a configuration
problem early rather than wait for the SPI transfer to fail continuously.

[....]

> > > My understanding is:
> > > 
> > > Disabling clocks will timeout transaction, if any. Core Device driver
> > > will call: devm_spi_unregister(), which will wait pending transactions
> > > to complete and then remove the SPI master.
> > 
> > Disabling clocks will confuse the hardware.  We cannot disable clocks while the
> > spi core is active and transferring data.
> 
> I could follow approach taken by other SPI drivers, just reset
> controller and disable clocks.

You have to wait until the hardware is in a sane state.  For the QUP, that means
in a RUN/PAUSE/RESET state.  It cannot be in transition when you cut the clocks.
The safest thing to do is to get the QUP into the RESET state and then cut the
clocks.

[.....]

> > > I am not aware of the difference. My board report v.20020000. 
> > > Is there difference of handling these controllers?
> > 
> > There were some bug fixes between versions.  None of those affect SPI (that I
> > can tell), but it's better to be more descriptive and use the full versions in
> > the compatible tags.
> 
> No strong preference here. Should I add qcom,spi-qup-v2.2.0, then? :-)

According to the documentation, there is no v2.2.0.  It appears there is some
disconnect between the specific HW revision and the documentation.  I'll see if
I can get some clarification from the hardware guys.  For now, I think the 2.1.1
and 2.2.1 tags are fine.

Ivan T. Ivanov Feb. 10, 2014, 7:41 p.m. UTC | #23

Hi,

On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
> On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
> 
> [....]
> 
> > > > > Bail here?
> > > > 
> > > > I don't know. What will be the consequences if controller continue to
> > > > operate on its default rate?
> > > > 
> > > 
> > > It is unclear.  But if you can't set the rate that is configured or if there is
> > > a misconfiguration, it's probably better to exit the probe and catch it here.
> > 
> > 
> > My preference is to delay clock speed change till first
> > SPI transfer. And use wherever transfer itself mandate.
> > 
> 
> That works.  My only concern is that it might be nice to catch a configuration
> problem early rather than wait for the SPI transfer to fail continuously.

If developer is skilled enough to know which version controller is,
(s)he will be able to put the right frequency constrain here :-)

> 
> [....]
> 
> > > > My understanding is:
> > > > 
> > > > Disabling clocks will timeout transaction, if any. Core Device driver
> > > > will call: devm_spi_unregister(), which will wait pending transactions
> > > > to complete and then remove the SPI master.
> > > 
> > > Disabling clocks will confuse the hardware.  We cannot disable clocks while the
> > > spi core is active and transferring data.
> > 
> > I could follow approach taken by other SPI drivers, just reset
> > controller and disable clocks.
> 
> You have to wait until the hardware is in a sane state.  For the QUP, that means
> in a RUN/PAUSE/RESET state.  It cannot be in transition when you cut the clocks.
> The safest thing to do is to get the QUP into the RESET state and then cut the
> clocks.
> 

Sure. will do.

> [.....]
> 
> > > > I am not aware of the difference. My board report v.20020000. 
> > > > Is there difference of handling these controllers?
> > > 
> > > There were some bug fixes between versions.  None of those affect SPI (that I
> > > can tell), but it's better to be more descriptive and use the full versions in
> > > the compatible tags.
> > 
> > No strong preference here. Should I add qcom,spi-qup-v2.2.0, then? :-)
> 
> According to the documentation, there is no v2.2.0.  It appears there is some
> disconnect between the specific HW revision and the documentation.  I'll see if
> I can get some clarification from the hardware guys.  For now, I think the 2.1.1
> and 2.2.1 tags are fine.

Ok. Thanks,
Ivan



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Courtney Cavin Feb. 10, 2014, 8:29 p.m. UTC | #24

On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote:
> 
> Hi,
> 
> On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
> > On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
> > 
> > [....]
> > 
> > > > > > Bail here?
> > > > > 
> > > > > I don't know. What will be the consequences if controller continue to
> > > > > operate on its default rate?
> > > > > 
> > > > 
> > > > It is unclear.  But if you can't set the rate that is configured or if there is
> > > > a misconfiguration, it's probably better to exit the probe and catch it here.
> > > 
> > > 
> > > My preference is to delay clock speed change till first
> > > SPI transfer. And use wherever transfer itself mandate.
> > > 
> > 
> > That works.  My only concern is that it might be nice to catch a configuration
> > problem early rather than wait for the SPI transfer to fail continuously.
> 
> If developer is skilled enough to know which version controller is,
> (s)he will be able to put the right frequency constrain here :-)

A developer doesn't have to have much skill at all to copy-paste DT
configurations around and muck with numbers....  I agree with Andy here,
early validation is a good idea here, at the very least, some sanity
checks.

-Courtney
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivan T. Ivanov Feb. 10, 2014, 8:59 p.m. UTC | #25

Hi, 

On Mon, 2014-02-10 at 12:29 -0800, Courtney Cavin wrote:
> On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote:
> > 
> > Hi,
> > 
> > On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
> > > On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
> > > 
> > > [....]
> > > 
> > > > > > > Bail here?
> > > > > > 
> > > > > > I don't know. What will be the consequences if controller continue to
> > > > > > operate on its default rate?
> > > > > > 
> > > > > 
> > > > > It is unclear.  But if you can't set the rate that is configured or if there is
> > > > > a misconfiguration, it's probably better to exit the probe and catch it here.
> > > > 
> > > > 
> > > > My preference is to delay clock speed change till first
> > > > SPI transfer. And use wherever transfer itself mandate.
> > > > 
> > > 
> > > That works.  My only concern is that it might be nice to catch a configuration
> > > problem early rather than wait for the SPI transfer to fail continuously.
> > 
> > If developer is skilled enough to know which version controller is,
> > (s)he will be able to put the right frequency constrain here :-)
> 
> A developer doesn't have to have much skill at all to copy-paste DT
> configurations around and muck with numbers....  I agree with Andy here,
> early validation is a good idea here, at the very least, some sanity
> checks.
> 

So, probably first variant with just warning will be good enough? 

Regards,
Ivan


> -Courtney


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Brown Feb. 10, 2014, 9:40 p.m. UTC | #26

On Mon, Feb 10, 2014 at 10:59:54PM +0200, Ivan T. Ivanov wrote:
> On Mon, 2014-02-10 at 12:29 -0800, Courtney Cavin wrote:

> > A developer doesn't have to have much skill at all to copy-paste DT
> > configurations around and muck with numbers....  I agree with Andy here,
> > early validation is a good idea here, at the very least, some sanity
> > checks.

> So, probably first variant with just warning will be good enough? 

I'm not sure it actually adds anything meaningful here - if the error
reporting isn't clear enough on use then that's probably an issue anyway
and we may never even use the default.

Ivan T. Ivanov Feb. 11, 2014, 3:46 p.m. UTC | #27

Hi, 

On Mon, 2014-02-10 at 12:29 -0800, Courtney Cavin wrote: 
> On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote:
> > 
> > Hi,
> > 
> > On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
> > > On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
> > > 
> > > [....]
> > > 
> > > > > > > Bail here?
> > > > > > 
> > > > > > I don't know. What will be the consequences if controller continue to
> > > > > > operate on its default rate?
> > > > > > 
> > > > > 
> > > > > It is unclear.  But if you can't set the rate that is configured or if there is
> > > > > a misconfiguration, it's probably better to exit the probe and catch it here.
> > > > 
> > > > 
> > > > My preference is to delay clock speed change till first
> > > > SPI transfer. And use wherever transfer itself mandate.
> > > > 
> > > 
> > > That works.  My only concern is that it might be nice to catch a configuration
> > > problem early rather than wait for the SPI transfer to fail continuously.
> > 
> > If developer is skilled enough to know which version controller is,
> > (s)he will be able to put the right frequency constrain here :-)
> 
> A developer doesn't have to have much skill at all to copy-paste DT
> configurations around and muck with numbers....  I agree with Andy here,
> early validation is a good idea here, at the very least, some sanity
> checks.

Actually, thinking more on this. Supplying SPI controller with, 
let say 50MHz, which is what success of clk_set_rate() means, doesn't
necessarily guaranteer that controller will be able to do transfers
properly, right? Setting frequency at this point didn't bring any
benefit. 

Regards,
Ivan


> 
> -Courtney


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ba9310b..bf8ce6b 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -381,6 +381,20 @@  config SPI_RSPI
 	help
 	  SPI driver for Renesas RSPI blocks.
 
+config SPI_QUP
+	tristate "Qualcomm SPI Support with QUP interface"
+	depends on ARCH_MSM
+	help
+	  Qualcomm Universal Peripheral (QUP) core is an 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 support up to 50MHz, up to four chip selects, and a
+	  programmable data path from 4 bits to 32 bits; supports numerous
+	  protocol variants.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called spi_qup.
+
 config SPI_S3C24XX
 	tristate "Samsung S3C24XX series SPI"
 	depends on ARCH_S3C24XX
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 95af48d..e598147 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -59,6 +59,7 @@  spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_PXADMA)	+= spi-pxa2xx-pxadma.o
 spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_DMA)	+= spi-pxa2xx-dma.o
 obj-$(CONFIG_SPI_PXA2XX)		+= spi-pxa2xx-platform.o
 obj-$(CONFIG_SPI_PXA2XX_PCI)		+= spi-pxa2xx-pci.o
+obj-$(CONFIG_SPI_QUP)			+= spi-qup.o
 obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi-s3c24xx-hw.o
 spi-s3c24xx-hw-y			:= spi-s3c24xx.o
diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
new file mode 100644
index 0000000..5eb5e8f
--- /dev/null
+++ b/drivers/spi/spi-qup.c
@@ -0,0 +1,898 @@ 
+/*
+ * Copyright (c) 2008-2014, The Linux foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License rev 2 and
+ * only rev 2 as published by the free Software foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or fITNESS fOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/spi/spi.h>
+
+#define QUP_CONFIG			0x0000
+#define QUP_STATE			0x0004
+#define QUP_IO_M_MODES			0x0008
+#define QUP_SW_RESET			0x000c
+#define QUP_OPERATIONAL			0x0018
+#define QUP_ERROR_FLAGS			0x001c
+#define QUP_ERROR_FLAGS_EN		0x0020
+#define QUP_OPERATIONAL_MASK		0x0028
+#define QUP_HW_VERSION			0x0030
+#define QUP_MX_OUTPUT_CNT		0x0100
+#define QUP_OUTPUT_FIFO			0x0110
+#define QUP_MX_WRITE_CNT		0x0150
+#define QUP_MX_INPUT_CNT		0x0200
+#define QUP_MX_READ_CNT			0x0208
+#define QUP_INPUT_FIFO			0x0218
+
+#define SPI_CONFIG			0x0300
+#define SPI_IO_CONTROL			0x0304
+#define SPI_ERROR_FLAGS			0x0308
+#define SPI_ERROR_FLAGS_EN		0x030c
+
+/* QUP_CONFIG fields */
+#define QUP_CONFIG_SPI_MODE		(1 << 8)
+#define QUP_CONFIG_NO_INPUT		BIT(7)
+#define QUP_CONFIG_NO_OUTPUT		BIT(6)
+#define QUP_CONFIG_N			0x001f
+
+/* QUP_STATE fields */
+#define QUP_STATE_VALID			BIT(2)
+#define QUP_STATE_RESET			0
+#define QUP_STATE_RUN			1
+#define QUP_STATE_PAUSE			3
+#define QUP_STATE_MASK			3
+#define QUP_STATE_CLEAR			2
+
+#define QUP_HW_VERSION_2_1_1		0x20010001
+
+/* QUP_IO_M_MODES fields */
+#define QUP_IO_M_PACK_EN		BIT(15)
+#define QUP_IO_M_UNPACK_EN		BIT(14)
+#define QUP_IO_M_INPUT_MODE_MASK_SHIFT	12
+#define QUP_IO_M_OUTPUT_MODE_MASK_SHIFT	10
+#define QUP_IO_M_INPUT_MODE_MASK	(3 << QUP_IO_M_INPUT_MODE_MASK_SHIFT)
+#define QUP_IO_M_OUTPUT_MODE_MASK	(3 << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT)
+
+#define QUP_IO_M_OUTPUT_BLOCK_SIZE(x)	(((x) & (0x03 << 0)) >> 0)
+#define QUP_IO_M_OUTPUT_FIFO_SIZE(x)	(((x) & (0x07 << 2)) >> 2)
+#define QUP_IO_M_INPUT_BLOCK_SIZE(x)	(((x) & (0x03 << 5)) >> 5)
+#define QUP_IO_M_INPUT_FIFO_SIZE(x)	(((x) & (0x07 << 7)) >> 7)
+
+#define QUP_IO_M_MODE_FIFO		0
+#define QUP_IO_M_MODE_BLOCK		1
+#define QUP_IO_M_MODE_DMOV		2
+#define QUP_IO_M_MODE_BAM		3
+
+/* QUP_OPERATIONAL fields */
+#define QUP_OP_MAX_INPUT_DONE_FLAG	BIT(11)
+#define QUP_OP_MAX_OUTPUT_DONE_FLAG	BIT(10)
+#define QUP_OP_IN_SERVICE_FLAG		BIT(9)
+#define QUP_OP_OUT_SERVICE_FLAG		BIT(8)
+#define QUP_OP_IN_FIFO_FULL		BIT(7)
+#define QUP_OP_OUT_FIFO_FULL		BIT(6)
+#define QUP_OP_IN_FIFO_NOT_EMPTY	BIT(5)
+#define QUP_OP_OUT_FIFO_NOT_EMPTY	BIT(4)
+
+/* QUP_ERROR_FLAGS and QUP_ERROR_FLAGS_EN fields */
+#define QUP_ERROR_OUTPUT_OVER_RUN	BIT(5)
+#define QUP_ERROR_INPUT_UNDER_RUN	BIT(4)
+#define QUP_ERROR_OUTPUT_UNDER_RUN	BIT(3)
+#define QUP_ERROR_INPUT_OVER_RUN	BIT(2)
+
+/* SPI_CONFIG fields */
+#define SPI_CONFIG_HS_MODE		BIT(10)
+#define SPI_CONFIG_INPUT_FIRST		BIT(9)
+#define SPI_CONFIG_LOOPBACK		BIT(8)
+
+/* SPI_IO_CONTROL fields */
+#define SPI_IO_C_FORCE_CS		BIT(11)
+#define SPI_IO_C_CLK_IDLE_HIGH		BIT(10)
+#define SPI_IO_C_MX_CS_MODE		BIT(8)
+#define SPI_IO_C_CS_N_POLARITY_0	BIT(4)
+#define SPI_IO_C_CS_SELECT(x)		(((x) & 3) << 2)
+#define SPI_IO_C_CS_SELECT_MASK		0x000c
+#define SPI_IO_C_TRISTATE_CS		BIT(1)
+#define SPI_IO_C_NO_TRI_STATE		BIT(0)
+
+/* SPI_ERROR_FLAGS and SPI_ERROR_FLAGS_EN fields */
+#define SPI_ERROR_CLK_OVER_RUN		BIT(1)
+#define SPI_ERROR_CLK_UNDER_RUN		BIT(0)
+
+#define SPI_NUM_CHIPSELECTS		4
+
+/* high speed mode is when bus rate is greater then 26MHz */
+#define SPI_HS_MIN_RATE			26000000
+
+#define SPI_DELAY_THRESHOLD		1
+#define SPI_DELAY_RETRY			10
+
+struct spi_qup_device {
+	int bits_per_word;
+	int chip_select;
+	int speed_hz;
+	u16 mode;
+};
+
+struct spi_qup {
+	void __iomem		*base;
+	struct device		*dev;
+	struct clk		*cclk;	/* core clock */
+	struct clk		*iclk;	/* interface clock */
+	int			irq;
+	u32			max_speed_hz;
+	u32			speed_hz;
+
+	int			in_fifo_sz;
+	int			out_fifo_sz;
+	int			in_blk_sz;
+	int			out_blk_sz;
+
+	struct spi_transfer	*xfer;
+	struct completion	done;
+	int			error;
+	int			bytes_per_word;
+	int			tx_bytes;
+	int			rx_bytes;
+};
+
+
+static inline bool spi_qup_is_valid_state(struct spi_qup *controller)
+{
+	u32 opstate = readl_relaxed(controller->base + QUP_STATE);
+
+	return opstate & QUP_STATE_VALID;
+}
+
+static int spi_qup_set_state(struct spi_qup *controller, u32 state)
+{
+	unsigned long loop = 0;
+	u32 cur_state;
+
+	cur_state = readl_relaxed(controller->base + QUP_STATE);
+	/*
+	 * Per spec: for PAUSE_STATE to RESET_STATE, two writes
+	 * of (b10) are required
+	 */
+	if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
+	    (state == QUP_STATE_RESET)) {
+		writel_relaxed(QUP_STATE_CLEAR, controller->base + QUP_STATE);
+		writel_relaxed(QUP_STATE_CLEAR, controller->base + QUP_STATE);
+	} else {
+		cur_state &= ~QUP_STATE_MASK;
+		cur_state |= state;
+		writel_relaxed(cur_state, controller->base + QUP_STATE);
+	}
+
+	while (!spi_qup_is_valid_state(controller)) {
+
+		usleep_range(SPI_DELAY_THRESHOLD, SPI_DELAY_THRESHOLD * 2);
+
+		if (++loop > SPI_DELAY_RETRY)
+			return -EIO;
+	}
+
+	return 0;
+}
+
+static void spi_qup_deassert_cs(struct spi_qup *controller,
+				struct spi_qup_device *chip)
+{
+	u32 iocontol, mask;
+
+	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
+
+	/* Disable auto CS toggle and use manual */
+	iocontol &= ~SPI_IO_C_MX_CS_MODE;
+	iocontol |= SPI_IO_C_FORCE_CS;
+
+	iocontol &= ~SPI_IO_C_CS_SELECT_MASK;
+	iocontol |= SPI_IO_C_CS_SELECT(chip->chip_select);
+
+	mask = SPI_IO_C_CS_N_POLARITY_0 << chip->chip_select;
+
+	if (chip->mode & SPI_CS_HIGH)
+		iocontol &= ~mask;
+	else
+		iocontol |= mask;
+
+	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
+}
+
+static void spi_qup_assert_cs(struct spi_qup *controller,
+			      struct spi_qup_device *chip)
+{
+	u32 iocontol, mask;
+
+	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
+
+	/* Disable auto CS toggle and use manual */
+	iocontol &= ~SPI_IO_C_MX_CS_MODE;
+	iocontol |= SPI_IO_C_FORCE_CS;
+
+	iocontol &= ~SPI_IO_C_CS_SELECT_MASK;
+	iocontol |= SPI_IO_C_CS_SELECT(chip->chip_select);
+
+	mask = SPI_IO_C_CS_N_POLARITY_0 << chip->chip_select;
+
+	if (chip->mode & SPI_CS_HIGH)
+		iocontol |= mask;
+	else
+		iocontol &= ~mask;
+
+	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
+}
+
+static void spi_qup_fifo_read(struct spi_qup *controller,
+			      struct spi_transfer *xfer)
+{
+	u8 *rx_buf = xfer->rx_buf;
+	u32 word, state;
+	int idx, shift;
+
+	while (controller->rx_bytes < xfer->len) {
+
+		state = readl_relaxed(controller->base + QUP_OPERATIONAL);
+		if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
+			break;
+
+		word = readl_relaxed(controller->base + QUP_INPUT_FIFO);
+
+		for (idx = 0; idx < controller->bytes_per_word &&
+		     controller->rx_bytes < xfer->len; idx++,
+		     controller->rx_bytes++) {
+
+			if (!rx_buf)
+				continue;
+			/*
+			 * The data format depends on bytes_per_word:
+			 *  4 bytes: 0x12345678
+			 *  2 bytes: 0x00001234
+			 *  1 byte : 0x00000012
+			 */
+			shift = BITS_PER_BYTE;
+			shift *= (controller->bytes_per_word - idx - 1);
+			rx_buf[controller->rx_bytes] = word >> shift;
+		}
+	}
+}
+
+static void spi_qup_fifo_write(struct spi_qup *controller,
+			       struct spi_transfer *xfer)
+{
+	const u8 *tx_buf = xfer->tx_buf;
+	u32 word, state, data;
+	int idx;
+
+	while (controller->tx_bytes < xfer->len) {
+
+		state = readl_relaxed(controller->base + QUP_OPERATIONAL);
+		if (state & QUP_OP_OUT_FIFO_FULL)
+			break;
+
+		word = 0;
+		for (idx = 0; idx < controller->bytes_per_word &&
+		     controller->tx_bytes < xfer->len; idx++,
+		     controller->tx_bytes++) {
+
+			if (!tx_buf)
+				continue;
+
+			data = tx_buf[controller->tx_bytes];
+			word |= data << (BITS_PER_BYTE * (3 - idx));
+		}
+
+		writel_relaxed(word, controller->base + QUP_OUTPUT_FIFO);
+	}
+}
+
+static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
+{
+	struct spi_qup *controller = dev_id;
+	struct spi_transfer *xfer;
+	u32 opflags, qup_err, spi_err;
+
+	xfer = controller->xfer;
+
+	qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
+	spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
+	opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
+
+	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
+	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
+	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
+
+	if (!xfer)
+		return IRQ_HANDLED;
+
+	if (qup_err) {
+		if (qup_err & QUP_ERROR_OUTPUT_OVER_RUN)
+			dev_warn(controller->dev, "OUTPUT_OVER_RUN\n");
+		if (qup_err & QUP_ERROR_INPUT_UNDER_RUN)
+			dev_warn(controller->dev, "INPUT_UNDER_RUN\n");
+		if (qup_err & QUP_ERROR_OUTPUT_UNDER_RUN)
+			dev_warn(controller->dev, "OUTPUT_UNDER_RUN\n");
+		if (qup_err & QUP_ERROR_INPUT_OVER_RUN)
+			dev_warn(controller->dev, "INPUT_OVER_RUN\n");
+
+		controller->error = -EIO;
+	}
+
+	if (spi_err) {
+		if (spi_err & SPI_ERROR_CLK_OVER_RUN)
+			dev_warn(controller->dev, "CLK_OVER_RUN\n");
+		if (spi_err & SPI_ERROR_CLK_UNDER_RUN)
+			dev_warn(controller->dev, "CLK_UNDER_RUN\n");
+
+		controller->error = -EIO;
+	}
+
+	if (opflags & QUP_OP_IN_SERVICE_FLAG) {
+		writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
+		       controller->base + QUP_OPERATIONAL);
+		spi_qup_fifo_read(controller, xfer);
+	}
+
+	if (opflags & QUP_OP_OUT_SERVICE_FLAG) {
+		writel_relaxed(QUP_OP_OUT_SERVICE_FLAG,
+		       controller->base + QUP_OPERATIONAL);
+		spi_qup_fifo_write(controller, xfer);
+	}
+
+	if (controller->rx_bytes == xfer->len ||
+	    controller->error)
+		complete(&controller->done);
+
+	return IRQ_HANDLED;
+}
+
+static int spi_qup_transfer_do(struct spi_qup *controller,
+			       struct spi_qup_device *chip,
+			       struct spi_transfer *xfer)
+{
+	unsigned long timeout;
+	int ret = -EIO;
+
+	reinit_completion(&controller->done);
+
+	timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
+	timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
+	timeout = 100 * msecs_to_jiffies(timeout);
+
+	controller->rx_bytes = 0;
+	controller->tx_bytes = 0;
+	controller->error = 0;
+	controller->xfer = xfer;
+
+	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
+		dev_warn(controller->dev, "cannot set RUN state\n");
+		goto exit;
+	}
+
+	if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
+		dev_warn(controller->dev, "cannot set PAUSE state\n");
+		goto exit;
+	}
+
+	spi_qup_fifo_write(controller, xfer);
+
+	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
+		dev_warn(controller->dev, "cannot set EXECUTE state\n");
+		goto exit;
+	}
+
+	if (!wait_for_completion_timeout(&controller->done, timeout))
+		ret = -ETIMEDOUT;
+	else
+		ret = controller->error;
+exit:
+	controller->xfer = NULL;
+	controller->error = 0;
+	controller->rx_bytes = 0;
+	controller->tx_bytes = 0;
+	spi_qup_set_state(controller, QUP_STATE_RESET);
+	return ret;
+}
+
+static int spi_qup_setup(struct spi_device *spi)
+{
+	struct spi_qup *controller = spi_master_get_devdata(spi->master);
+	struct spi_qup_device *chip = spi_get_ctldata(spi);
+
+	if (spi->chip_select >= spi->master->num_chipselect) {
+		dev_err(controller->dev, "invalid chip_select %d\n",
+			spi->chip_select);
+		return -EINVAL;
+	}
+
+	if (spi->max_speed_hz > controller->max_speed_hz) {
+		dev_err(controller->dev, "invalid max_speed_hz %d\n",
+			spi->max_speed_hz);
+		return -EINVAL;
+	}
+
+	if (!chip) {
+		/* First setup */
+		chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+		if (!chip) {
+			dev_err(controller->dev, "no memory for chip data\n");
+			return -ENOMEM;
+		}
+
+		spi_set_ctldata(spi, chip);
+	}
+
+	return 0;
+}
+
+static void spi_qup_cleanup(struct spi_device *spi)
+{
+	struct spi_qup_device *chip = spi_get_ctldata(spi);
+
+	if (!chip)
+		return;
+
+	spi_set_ctldata(spi, NULL);
+	kfree(chip);
+}
+
+/* set clock freq, clock ramp, bits per work */
+static int spi_qup_io_setup(struct spi_device *spi,
+			  struct spi_transfer *xfer)
+{
+	struct spi_qup *controller = spi_master_get_devdata(spi->master);
+	struct spi_qup_device *chip = spi_get_ctldata(spi);
+	u32 iocontol, config, iomode, mode;
+	int ret, n_words;
+
+	if (spi->mode & SPI_LOOP && xfer->len > controller->in_fifo_sz) {
+		dev_err(controller->dev, "too big size for loopback %d > %d\n",
+			xfer->len, controller->in_fifo_sz);
+		return -EIO;
+	}
+
+	chip->mode = spi->mode;
+	chip->speed_hz = spi->max_speed_hz;
+	if (xfer->speed_hz)
+		chip->speed_hz = xfer->speed_hz;
+
+	if (controller->speed_hz != chip->speed_hz) {
+		ret = clk_set_rate(controller->cclk, chip->speed_hz);
+		if (ret) {
+			dev_err(controller->dev, "fail to set frequency %d",
+				chip->speed_hz);
+			return -EIO;
+		}
+	}
+
+	controller->speed_hz = chip->speed_hz;
+
+	chip->bits_per_word = spi->bits_per_word;
+	if (xfer->bits_per_word)
+		chip->bits_per_word = xfer->bits_per_word;
+
+	if (chip->bits_per_word <= 8)
+		controller->bytes_per_word = 1;
+	else if (chip->bits_per_word <= 16)
+		controller->bytes_per_word = 2;
+	else
+		controller->bytes_per_word = 4;
+
+	if (controller->bytes_per_word > xfer->len ||
+	    xfer->len % controller->bytes_per_word != 0){
+		/* No partial transfers */
+		dev_err(controller->dev, "invalid len %d for %d bits\n",
+			xfer->len, chip->bits_per_word);
+		return -EIO;
+	}
+
+	n_words = xfer->len / controller->bytes_per_word;
+
+	if (spi_qup_set_state(controller, QUP_STATE_RESET)) {
+		dev_err(controller->dev, "cannot set RESET state\n");
+		return -EIO;
+	}
+
+	if (n_words <= controller->in_fifo_sz) {
+		mode = QUP_IO_M_MODE_FIFO;
+		writel_relaxed(n_words, controller->base + QUP_MX_READ_CNT);
+		writel_relaxed(n_words, controller->base + QUP_MX_WRITE_CNT);
+		/* must be zero for FIFO */
+		writel_relaxed(0, controller->base + QUP_MX_INPUT_CNT);
+		writel_relaxed(0, controller->base + QUP_MX_OUTPUT_CNT);
+	} else {
+		mode = QUP_IO_M_MODE_BLOCK;
+		writel_relaxed(n_words, controller->base + QUP_MX_INPUT_CNT);
+		writel_relaxed(n_words, controller->base + QUP_MX_OUTPUT_CNT);
+		/* must be zero for BLOCK and BAM */
+		writel_relaxed(0, controller->base + QUP_MX_READ_CNT);
+		writel_relaxed(0, controller->base + QUP_MX_WRITE_CNT);
+	}
+
+	iomode = readl_relaxed(controller->base + QUP_IO_M_MODES);
+	/* Set input and output transfer mode */
+	iomode &= ~(QUP_IO_M_INPUT_MODE_MASK | QUP_IO_M_OUTPUT_MODE_MASK);
+	iomode &= ~(QUP_IO_M_PACK_EN | QUP_IO_M_UNPACK_EN);
+	iomode |= (mode << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT);
+	iomode |= (mode << QUP_IO_M_INPUT_MODE_MASK_SHIFT);
+
+	writel_relaxed(iomode, controller->base + QUP_IO_M_MODES);
+
+	config = readl_relaxed(controller->base + SPI_CONFIG);
+
+	if (chip->mode & SPI_LOOP)
+		config |= SPI_CONFIG_LOOPBACK;
+	else
+		config &= ~SPI_CONFIG_LOOPBACK;
+
+	if (chip->mode & SPI_CPHA)
+		config &= ~SPI_CONFIG_INPUT_FIRST;
+	else
+		config |= SPI_CONFIG_INPUT_FIRST;
+
+	/*
+	 * HS_MODE improves signal stability for spi-clk high rates
+	 * but is invalid in loop back mode.
+	 */
+	if ((controller->speed_hz >= SPI_HS_MIN_RATE) &&
+	    !(chip->mode & SPI_LOOP))
+		config |= SPI_CONFIG_HS_MODE;
+	else
+		config &= ~SPI_CONFIG_HS_MODE;
+
+	writel_relaxed(config, controller->base + SPI_CONFIG);
+
+	config = readl_relaxed(controller->base + QUP_CONFIG);
+	config &= ~(QUP_CONFIG_NO_INPUT | QUP_CONFIG_NO_OUTPUT | QUP_CONFIG_N);
+	config |= chip->bits_per_word - 1;
+	config |= QUP_CONFIG_SPI_MODE;
+	writel_relaxed(config, controller->base + QUP_CONFIG);
+
+	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
+
+	/* Disable auto CS toggle */
+	iocontol &= ~SPI_IO_C_MX_CS_MODE;
+
+	if (chip->mode & SPI_CPOL)
+		iocontol |= SPI_IO_C_CLK_IDLE_HIGH;
+	else
+		iocontol &= ~SPI_IO_C_CLK_IDLE_HIGH;
+
+	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
+
+	/*
+	 * TODO: In BAM mode mask INPUT and OUTPUT service flags in
+	 * to prevent IRQs on FIFO status change.
+	 */
+	writel_relaxed(0, controller->base + QUP_OPERATIONAL_MASK);
+
+	return 0;
+}
+
+static int spi_qup_transfer_one(struct spi_master *master,
+				struct spi_message *msg)
+{
+	struct spi_qup *controller = spi_master_get_devdata(master);
+	struct spi_qup_device *chip = spi_get_ctldata(msg->spi);
+	struct spi_transfer *xfer;
+	struct spi_device *spi;
+	unsigned cs_change;
+	int status;
+
+	spi = msg->spi;
+	cs_change = 1;
+	status = 0;
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+
+		status = spi_qup_io_setup(spi, xfer);
+		if (status)
+			break;
+
+		if (cs_change)
+			spi_qup_assert_cs(controller, chip);
+
+		cs_change = xfer->cs_change;
+
+		/* Do actual transfer */
+		status = spi_qup_transfer_do(controller, chip, xfer);
+		if (status)
+			break;
+
+		msg->actual_length += xfer->len;
+
+		if (xfer->delay_usecs)
+			udelay(xfer->delay_usecs);
+
+		if (cs_change)
+			spi_qup_deassert_cs(controller, chip);
+	}
+
+	if (status || !cs_change)
+		spi_qup_deassert_cs(controller, chip);
+
+	msg->status = status;
+	spi_finalize_current_message(master);
+	return status;
+}
+
+static int spi_qup_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct clk *iclk, *cclk;
+	struct spi_qup *controller;
+	struct resource *res;
+	struct device *dev;
+	void __iomem *base;
+	u32 data, max_freq, iomode;
+	int ret, irq, size;
+
+	dev = &pdev->dev;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	irq = platform_get_irq(pdev, 0);
+
+	if (irq < 0)
+		return irq;
+
+	cclk = devm_clk_get(dev, "core");
+	if (IS_ERR(cclk)) {
+		dev_err(dev, "cannot get core clock\n");
+		return PTR_ERR(cclk);
+	}
+
+	iclk = devm_clk_get(dev, "iface");
+	if (IS_ERR(iclk)) {
+		dev_err(dev, "cannot get iface clock\n");
+		return PTR_ERR(iclk);
+	}
+
+	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
+		max_freq = 19200000;
+
+	if (!max_freq) {
+		dev_err(dev, "invalid clock frequency %d\n", max_freq);
+		return -ENXIO;
+	}
+
+	ret = clk_set_rate(cclk, max_freq);
+	if (ret)
+		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
+
+	ret = clk_prepare_enable(cclk);
+	if (ret) {
+		dev_err(dev, "cannot enable core clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(iclk);
+	if (ret) {
+		clk_disable_unprepare(cclk);
+		dev_err(dev, "cannot enable iface clock\n");
+		return ret;
+	}
+
+	data = readl_relaxed(base + QUP_HW_VERSION);
+
+	if (data < QUP_HW_VERSION_2_1_1) {
+		clk_disable_unprepare(cclk);
+		clk_disable_unprepare(iclk);
+		dev_err(dev, "v.%08x is not supported\n", data);
+		return -ENXIO;
+	}
+
+	master = spi_alloc_master(dev, sizeof(struct spi_qup));
+	if (!master) {
+		clk_disable_unprepare(cclk);
+		clk_disable_unprepare(iclk);
+		dev_err(dev, "cannot allocate master\n");
+		return -ENOMEM;
+	}
+
+	master->bus_num = pdev->id;
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
+	master->num_chipselect = SPI_NUM_CHIPSELECTS;
+	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
+	master->setup = spi_qup_setup;
+	master->cleanup = spi_qup_cleanup;
+	master->transfer_one_message = spi_qup_transfer_one;
+	master->dev.of_node = pdev->dev.of_node;
+	master->auto_runtime_pm = true;
+
+	platform_set_drvdata(pdev, master);
+
+	controller = spi_master_get_devdata(master);
+
+	controller->dev  = dev;
+	controller->base = base;
+	controller->iclk = iclk;
+	controller->cclk = cclk;
+	controller->irq  = irq;
+	controller->max_speed_hz = clk_get_rate(cclk);
+	controller->speed_hz = controller->max_speed_hz;
+
+	init_completion(&controller->done);
+
+	iomode = readl_relaxed(base + QUP_IO_M_MODES);
+
+	size = QUP_IO_M_OUTPUT_BLOCK_SIZE(iomode);
+	if (size)
+		controller->out_blk_sz = size * 16;
+	else
+		controller->out_blk_sz = 4;
+
+	size = QUP_IO_M_INPUT_BLOCK_SIZE(iomode);
+	if (size)
+		controller->in_blk_sz = size * 16;
+	else
+		controller->in_blk_sz = 4;
+
+	size = QUP_IO_M_OUTPUT_FIFO_SIZE(iomode);
+	controller->out_fifo_sz = controller->out_blk_sz * (2 << size);
+
+	size = QUP_IO_M_INPUT_FIFO_SIZE(iomode);
+	controller->in_fifo_sz = controller->in_blk_sz * (2 << size);
+
+	dev_info(dev, "v.%08x IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
+		 data, controller->in_blk_sz, controller->in_fifo_sz,
+		 controller->out_blk_sz, controller->out_fifo_sz);
+
+	writel_relaxed(1, base + QUP_SW_RESET);
+
+	ret = spi_qup_set_state(controller, QUP_STATE_RESET);
+	if (ret) {
+		dev_err(dev, "cannot set RESET state\n");
+		goto error;
+	}
+
+	writel_relaxed(0, base + QUP_OPERATIONAL);
+	writel_relaxed(0, base + QUP_IO_M_MODES);
+	writel_relaxed(0, base + QUP_OPERATIONAL_MASK);
+	writel_relaxed(SPI_ERROR_CLK_UNDER_RUN | SPI_ERROR_CLK_OVER_RUN,
+		       base + SPI_ERROR_FLAGS_EN);
+
+	writel_relaxed(0, base + SPI_CONFIG);
+	writel_relaxed(SPI_IO_C_NO_TRI_STATE, base + SPI_IO_CONTROL);
+
+	ret = devm_request_irq(dev, irq, spi_qup_qup_irq,
+			       IRQF_TRIGGER_HIGH, pdev->name, controller);
+	if (ret) {
+		dev_err(dev, "cannot request IRQ %d\n", irq);
+		goto error;
+	}
+
+	ret = devm_spi_register_master(dev, master);
+	if (!ret) {
+		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
+		pm_runtime_use_autosuspend(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+		return ret;
+	}
+error:
+	clk_disable_unprepare(cclk);
+	clk_disable_unprepare(iclk);
+	spi_master_put(master);
+	return ret;
+}
+
+#ifdef CONFIG_PM_RUNTIME
+static int spi_qup_pm_suspend_runtime(struct device *device)
+{
+	struct spi_master *master = dev_get_drvdata(device);
+	struct spi_qup *controller = spi_master_get_devdata(master);
+
+	disable_irq(controller->irq);
+	clk_disable_unprepare(controller->cclk);
+	clk_disable_unprepare(controller->iclk);
+	dev_dbg(device, "suspend runtime\n");
+	return 0;
+}
+
+static int spi_qup_pm_resume_runtime(struct device *device)
+{
+	struct spi_master *master = dev_get_drvdata(device);
+	struct spi_qup *controller = spi_master_get_devdata(master);
+
+	clk_prepare_enable(controller->cclk);
+	clk_prepare_enable(controller->iclk);
+	enable_irq(controller->irq);
+	dev_dbg(device, "resume runtime\n");
+	return 0;
+}
+#endif /* CONFIG_PM_RUNTIME */
+
+#ifdef CONFIG_PM_SLEEP
+static int spi_qup_suspend(struct device *device)
+{
+	struct spi_master *master = dev_get_drvdata(device);
+	struct spi_qup *controller = spi_master_get_devdata(master);
+	int status;
+
+	status = spi_master_suspend(master);
+	if (!status) {
+		disable_irq(controller->irq);
+		clk_disable_unprepare(controller->cclk);
+		clk_disable_unprepare(controller->iclk);
+	}
+
+	dev_dbg(device, "system suspend %d\n", status);
+	return status;
+}
+
+static int spi_qup_resume(struct device *device)
+{
+	struct spi_master *master = dev_get_drvdata(device);
+	struct spi_qup *controller = spi_master_get_devdata(master);
+	int status;
+
+	clk_prepare_enable(controller->cclk);
+	clk_prepare_enable(controller->iclk);
+
+	status = spi_master_resume(master);
+
+	dev_dbg(device, "system resume %d\n", status);
+	return status;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static int spi_qup_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = dev_get_drvdata(&pdev->dev);
+	struct spi_qup *controller = spi_master_get_devdata(master);
+
+	pm_runtime_get_sync(&pdev->dev);
+
+	clk_disable_unprepare(controller->cclk);
+	clk_disable_unprepare(controller->iclk);
+
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	return 0;
+}
+
+static struct of_device_id spi_qup_dt_match[] = {
+	{ .compatible = "qcom,spi-qup-v2", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, spi_qup_dt_match);
+
+static const struct dev_pm_ops spi_qup_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(spi_qup_suspend, spi_qup_resume)
+	SET_RUNTIME_PM_OPS(spi_qup_pm_suspend_runtime,
+			   spi_qup_pm_resume_runtime,
+			   NULL)
+};
+
+static struct platform_driver spi_qup_driver = {
+	.driver = {
+		.name		= "spi_qup",
+		.owner		= THIS_MODULE,
+		.pm		= &spi_qup_dev_pm_ops,
+		.of_match_table = spi_qup_dt_match,
+	},
+	.probe = spi_qup_probe,
+	.remove = spi_qup_remove,
+};
+module_platform_driver(spi_qup_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("0.4");
+MODULE_ALIAS("platform:spi_qup");