LinuxLists.cc - [PATCH 0/2] spi: Add Qualcomm QUP SPI controller support

2014-02-06 16:59:20

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

From: "Ivan T. Ivanov" <[email protected]>

Hi,

Following two patches are adding initial support for SPI controller
available in Qualcomm SoC's.

Controller initialization is based on spi_qsd driver available in
CAF repository.

Controller supports SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP modes,
up to 3 CS's and from 4 to 32 bits per word. SPI_LOOP mode is limited
to input FIFO buffer size.

Currently driver support only PIO mode, I am hopping to add also DMA
mode support with dmaengine patches developed by Andy.

Regards,
Ivan

Ivan T. Ivanov (2):
spi: qup: Add device tree bindings information
spi: Add Qualcomm QUP SPI controller support

.../devicetree/bindings/spi/qcom,spi-qup.txt | 86 ++
drivers/spi/Kconfig | 14 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-qup.c | 898 ++++++++++++++++++++
4 files changed, 999 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
create mode 100644 drivers/spi/spi-qup.c

--
1.7.9.5

2014-02-06 16:59:30

by Ivan T. Ivanov

[permalink] [raw]

Subject: [PATCH 1/2] spi: qup: Add device tree bindings information

From: "Ivan T. Ivanov" <[email protected]>

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

Signed-off-by: Ivan T. Ivanov <[email protected]>
---
.../devicetree/bindings/spi/qcom,spi-qup.txt | 86 ++++++++++++++++++++
1 file changed, 86 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qup.txt

diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
new file mode 100644
index 0000000..74565f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
@@ -0,0 +1,86 @@
+Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)
+
+The 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.
+
+Required properties:
+- compatible: Should contain "qcom,spi-qup-v2".
+- reg: Should contain base register location and length
+- interrupts: Interrupt number used by this controller
+
+- clocks: Should contain the core clock and the AHB clock.
+- clock-names: Should be "core" for the core clock and "iface" for the
+ AHB clock.
+
+- #address-cells: Number of cells required to define a chip select
+ address on the SPI bus. Should be set to 1.
+- #size-cells: Should be zero.
+
+Optional properties:
+- spi-max-frequency: Specifies maximum SPI clock frequency, Units - Hz. Definition
+ as per Documentation/devicetree/bindings/spi/spi-bus.txt
+
+SPI slave nodes must be children of the SPI master node and can
+contain properties described in Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Example:
+
+ spi_8: spi@f9964000 { /* BLSP2 QUP2 */
+
+ compatible = "qcom,spi-qup-v2";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0xf9964000 0x1000>;
+ interrupts = <0 102 0>;
+ spi-max-frequency = <19200000>;
+
+ clocks = <&gcc GCC_BLSP2_QUP2_SPI_APPS_CLK>, <&gcc GCC_BLSP2_AHB_CLK>;
+ clock-names = "core", "iface";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&spi8_default>;
+
+ device@0 {
+ compatible = "arm,pl022-dummy";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0>; /* Chip select 0 */
+ spi-max-frequency = <19200000>;
+ spi-cpol;
+ };
+
+ device@1 {
+ compatible = "arm,pl022-dummy";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <1>; /* Chip select 1 */
+ spi-max-frequency = <9600000>;
+ spi-cpha;
+ };
+
+ device@2 {
+ compatible = "arm,pl022-dummy";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <2>; /* Chip select 2 */
+ spi-max-frequency = <19200000>;
+ spi-cpol;
+ spi-cpha;
+ };
+
+ device@3 {
+ compatible = "arm,pl022-dummy";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <3>; /* Chip select 3 */
+ spi-max-frequency = <19200000>;
+ spi-cpol;
+ spi-cpha;
+ spi-cs-high;
+ };
+ };
+
--
1.7.9.5

2014-02-06 16:59:47

by Ivan T. Ivanov

[permalink] [raw]

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

From: "Ivan T. Ivanov" <[email protected]>

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 <[email protected]>
Cc: Alok Chauhan <[email protected]>
Cc: Gilad Avidov <[email protected]>
Cc: Kiran Gunda <[email protected]>
Cc: Sagar Dharia <[email protected]>
---
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);
+ /*
+ * 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");
--
1.7.9.5

2014-02-06 17:51:12

[permalink] [raw]

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

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

2014-02-10 17:09:15

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.

Attachments:

(No filename) (611.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2014-02-11 15:47:54

by Ivan T. Ivanov

[permalink] [raw]

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

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