[PATCH v4 0/2] spi: driver for Cirrus EP93xx SPI controller

[PATCH v4 0/2] spi: driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

Hello,

This is fourth iteration of the driver. I tried to address most of the review
comments I received.

Changes to previous version:
	- dropped polling mode, there is no real use for that
	- changes according review comments by Grant Likely, Martin Guy,
	  and H. Hartley Sweeten

Previous version can be found here (with review comments):
	http://lists.infradead.org/pipermail/linux-arm-kernel/2010-April/012941.html

Let's hope I didn't forget anything.

Tested with my TS-7260 board with mmc_spi and at25 drivers.

Note that patch 2/2 depends on patch that is already in Russell's patch
tracking system:
	http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=5998/1

Thanks,
MW

Mika Westerberg (2):
  spi: implemented driver for Cirrus EP93xx SPI controller
  ep93xx: SPI driver platform support code

 arch/arm/mach-ep93xx/clock.c                    |   14 +
 arch/arm/mach-ep93xx/core.c                     |   48 ++
 arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h |    1 +
 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h  |   32 +
 arch/arm/mach-ep93xx/include/mach/platform.h    |    2 +
 drivers/spi/Kconfig                             |   11 +
 drivers/spi/Makefile                            |    1 +
 drivers/spi/ep93xx_spi.c                        |  992 +++++++++++++++++++++++
 8 files changed, 1101 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h
 create mode 100644 drivers/spi/ep93xx_spi.c


------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

[PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).

Signed-off-by: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>
---
 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h |   32 +
 drivers/spi/Kconfig                            |   11 +
 drivers/spi/Makefile                           |    1 +
 drivers/spi/ep93xx_spi.c                       |  992 ++++++++++++++++++++++++
 4 files changed, 1036 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h
 create mode 100644 drivers/spi/ep93xx_spi.c

diff --git a/arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h b/arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h
new file mode 100644
index 0000000..98935d8
--- /dev/null
+++ b/arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h
@@ -0,0 +1,32 @@
+#ifndef __ASM_MACH_EP93XX_SPI_H
+#define __ASM_MACH_EP93XX_SPI_H
+
+struct spi_device;
+
+/**
+ * struct ep93xx_spi_info - EP93xx specific SPI descriptor
+ * @num_chipselect: number of chip selects on this board, must be
+ *                  at least one
+ * @cs_control: chip select control function. Can be %NULL if not needed.
+ *
+ * This structure is passed from board support files to EP93xx SPI controller
+ * driver. It provides callback hook to control chip select lines that are
+ * allocated in board support files during the board initialization.
+ */
+struct ep93xx_spi_info {
+	int	num_chipselect;
+	/*
+	 * cs_control() - control board chipselect GPIO lines
+	 * @spi: SPI device whose chipselect is controlled
+	 * @value: value to set the chip select line to
+	 *
+	 * This function is used to control board specific chip select lines.
+	 * @value is either %0 or %1.
+	 *
+	 * This function is called from thread context and can sleep if
+	 * necessary.
+	 */
+	void	(*cs_control)(struct spi_device *spi, int value);
+};
+
+#endif /* __ASM_MACH_EP93XX_SPI_H */
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index a191fa2..5852340 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -117,6 +117,17 @@ config SPI_DAVINCI
 	help
 	  SPI master controller for DaVinci and DA8xx SPI modules.
 
+config SPI_EP93XX
+	tristate "Cirrus Logic EP93xx SPI controller"
+	depends on ARCH_EP93XX
+	help
+	  This enables using the Cirrus EP93xx SPI controller in master
+	  mode. This driver supports EP9301, EP9302, EP9307, EP9312 and EP9315
+	  chips.
+
+	  To compile this driver as a module, choose M here. The module will be
+	  called ep93xx_spi.
+
 config SPI_GPIO
 	tristate "GPIO-based bitbanging SPI Master"
 	depends on GENERIC_GPIO
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d7d0f89..377f845 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_SPI_DAVINCI)		+= davinci_spi.o
 obj-$(CONFIG_SPI_DESIGNWARE)		+= dw_spi.o
 obj-$(CONFIG_SPI_DW_PCI)		+= dw_spi_pci.o
 obj-$(CONFIG_SPI_DW_MMIO)		+= dw_spi_mmio.o
+obj-$(CONFIG_SPI_EP93XX)		+= ep93xx_spi.o
 obj-$(CONFIG_SPI_GPIO)			+= spi_gpio.o
 obj-$(CONFIG_SPI_IMX)			+= spi_imx.o
 obj-$(CONFIG_SPI_LM70_LLP)		+= spi_lm70llp.o
diff --git a/drivers/spi/ep93xx_spi.c b/drivers/spi/ep93xx_spi.c
new file mode 100644
index 0000000..310032d
--- /dev/null
+++ b/drivers/spi/ep93xx_spi.c
@@ -0,0 +1,992 @@
+/*
+ * Driver for Cirrus Logic EP93xx SPI controller.
+ *
+ * Copyright (c) 2010 Mika Westerberg
+ *
+ * Explicit FIFO handling code was inspired by amba-pl022 driver.
+ *
+ * For more information about the SPI controller see documentation on Cirrus
+ * Logic web site:
+ *     http://www.cirrus.com/en/pubs/manual/EP93xx_Users_Guide_UM1.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/bitops.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/workqueue.h>
+#include <linux/sched.h>
+#include <linux/spi/spi.h>
+
+#include <mach/ep93xx_spi.h>
+
+#define SSPCR0			0x0000
+#define SSPCR0_MODE_SHIFT	6
+#define SSPCR0_SCR_SHIFT	8
+
+#define SSPCR1			0x0004
+#define SSPCR1_RIE		BIT(0)
+#define SSPCR1_TIE		BIT(1)
+#define SSPCR1_RORIE		BIT(2)
+#define SSPCR1_LBM		BIT(3)
+#define SSPCR1_SSE		BIT(4)
+#define SSPCR1_MS		BIT(5)
+#define SSPCR1_SOD		BIT(6)
+
+#define SSPDR			0x0008
+
+#define SSPSR			0x000c
+#define SSPSR_TFE		BIT(0)
+#define SSPSR_TNF		BIT(1)
+#define SSPSR_RNE		BIT(2)
+#define SSPSR_RFF		BIT(3)
+#define SSPSR_BSY		BIT(4)
+#define SSPCPSR			0x0010
+
+#define SSPIIR			0x0014
+#define SSPIIR_RIS		BIT(0)
+#define SSPIIR_TIS		BIT(1)
+#define SSPIIR_RORIS		BIT(2)
+#define SSPICR			SSPIIR
+
+/* timeout in milliseconds */
+#define SPI_TIMEOUT		5
+/* maximum depth of RX/TX FIFO */
+#define SPI_FIFO_SIZE		8
+
+/**
+ * struct ep93xx_spi - EP93xx SPI controller structure
+ * @lock: spinlock that protects concurrent accesses to fields @running,
+ *        @current_msg and @msg_queue
+ * @pdev: pointer to platform device
+ * @clk: clock for the controller
+ * @regs_base: pointer to ioremap()'d registers
+ * @irq: IRQ number used by the driver
+ * @min_rate: minimum clock rate (in Hz) supported by the controller
+ * @max_rate: maximum clock rate (in Hz) supported by the controller
+ * @running: is the queue running
+ * @msg_queue: queue for the messages
+ * @wq: workqueue used by the driver
+ * @msg_work: work that is queued for the driver
+ * @wait: wait here until given transfer is completed
+ * @current_msg: message that is currently processed (or %NULL if none)
+ * @spi_dev: pointer to device that was previously selected
+ * @tx: current byte in transfer to transmit
+ * @rx: current byte in transfer to receive
+ * @fifo_level: how full is FIFO (%0..%SPI_FIFO_SIZE - %1). Receiving one
+ *              frame decreases this level and sending one frame increases it.
+ * @cs_control: chip select control function
+ *
+ * This structure holds EP93xx SPI controller specific information. When
+ * @running is %true, driver accepts transfer requests from protocol drivers.
+ * @current_msg is used to hold pointer to the message that is currently
+ * processed. If @current_msg is %NULL, it means that no processing is going
+ * on.
+ *
+ * Most of the fields are only written once and they can be accessed without
+ * taking the @lock. Fields that are accessed concurrently are: @current_msg,
+ * @running and @msg_queue.
+ */
+struct ep93xx_spi {
+	spinlock_t			lock;
+	const struct platform_device	*pdev;
+	struct clk			*clk;
+	void __iomem			*regs_base;
+	int				irq;
+	unsigned long			min_rate;
+	unsigned long			max_rate;
+	bool				running;
+	struct workqueue_struct		*wq;
+	struct work_struct		msg_work;
+	struct completion		wait;
+	struct list_head		msg_queue;
+	struct spi_message		*current_msg;
+	size_t				tx;
+	size_t				rx;
+	size_t				fifo_level;
+	void				(*cs_control)(struct spi_device *, int);
+};
+
+/**
+ * struct ep93xx_spi_chip - SPI device hardware settings
+ * @spi: back pointer to the SPI device
+ * @rate: max rate in hz this chip supports
+ * @div_cpsr: cpsr (pre-scaler) divider
+ * @div_scr: scr divider
+ * @dss: bits per word (4 - 16 bits)
+ *
+ * This structure is used to store hardware register specific settings for each
+ * SPI device. Settings are written to hardware by function
+ * ep93xx_spi_chip_setup().
+ */
+struct ep93xx_spi_chip {
+	const struct spi_device	*spi;
+	unsigned long		rate;
+	u8			div_cpsr;
+	u8			div_scr;
+	u8			dss;
+};
+
+/* converts bits per word to CR0.DSS value */
+#define bits_per_word_to_dss(bpw)	((bpw) - 1)
+
+static inline void
+ep93xx_spi_write_u8(const struct ep93xx_spi *espi, u16 reg, u8 value)
+{
+	__raw_writeb(value, espi->regs_base + reg);
+}
+
+static inline u8
+ep93xx_spi_read_u8(const struct ep93xx_spi *spi, u16 reg)
+{
+	return __raw_readb(spi->regs_base + reg);
+}
+
+static inline void
+ep93xx_spi_write_u16(const struct ep93xx_spi *espi, u16 reg, u16 value)
+{
+	__raw_writew(value, espi->regs_base + reg);
+}
+
+static inline u16
+ep93xx_spi_read_u16(const struct ep93xx_spi *spi, u16 reg)
+{
+	return __raw_readw(spi->regs_base + reg);
+}
+
+/**
+ * ep93xx_spi_enable() - enables the SPI controller and clock
+ * @espi: ep93xx SPI controller struct
+ *
+ * This function enables the SPI controller and its clock. Returns %0 in case
+ * of success and negative error in case if failure.
+ */
+static int ep93xx_spi_enable(const struct ep93xx_spi *espi)
+{
+	u8 regval;
+	int err;
+
+	err = clk_enable(espi->clk);
+	if (err)
+		return err;
+
+	regval = ep93xx_spi_read_u8(espi, SSPCR1);
+	regval |= SSPCR1_SSE;
+	ep93xx_spi_write_u8(espi, SSPCR1, regval);
+
+	return 0;
+}
+
+/**
+ * ep93xx_spi_disable() - disables the SPI controller and clock
+ * @espi: ep93xx SPI controller struct
+ *
+ * Function disables SPI controller and its clock.
+ */
+static void ep93xx_spi_disable(const struct ep93xx_spi *espi)
+{
+	u8 regval;
+
+	regval = ep93xx_spi_read_u8(espi, SSPCR1);
+	regval &= ~SSPCR1_SSE;
+	ep93xx_spi_write_u8(espi, SSPCR1, regval);
+
+	clk_disable(espi->clk);
+}
+
+/**
+ * ep93xx_spi_enable_interrupts() - enables all SPI interrupts
+ * @espi: ep93xx SPI controller struct
+ *
+ * Enables all SPI interrupts: receive overrun (ROR), transmit, and receive.
+ */
+static inline void
+ep93xx_spi_enable_interrupts(const struct ep93xx_spi *espi)
+{
+	u8 regval;
+
+	regval = ep93xx_spi_read_u8(espi, SSPCR1);
+	regval |= (SSPCR1_RORIE | SSPCR1_TIE | SSPCR1_RIE);
+	ep93xx_spi_write_u8(espi, SSPCR1, regval);
+}
+
+/**
+ * ep93xx_spi_disable_interrupts() - disables all SPI interrupts
+ * @espi: ep93xx SPI controller struct
+ *
+ * Disables all SPI interrupts.
+ */
+static inline void
+ep93xx_spi_disable_interrupts(const struct ep93xx_spi *espi)
+{
+	u8 regval;
+
+	regval = ep93xx_spi_read_u8(espi, SSPCR1);
+	regval &= ~(SSPCR1_RORIE | SSPCR1_TIE | SSPCR1_RIE);
+	ep93xx_spi_write_u8(espi, SSPCR1, regval);
+}
+
+/**
+ * ep93xx_spi_calc_divisors() - calculates SPI clock divisors
+ * @espi: ep93xx SPI controller struct
+ * @chip: divisors are calculated for this chip
+ * @rate: desired SPI output clock rate
+ *
+ * Function calculates cpsr (clock pre-scaler) and scr divisors based on
+ * given @rate and places them to @chip->div_cpsr and @chip->div_scr. If,
+ * for some reason, divisors cannot be calculated nothing is stored and
+ * %-EINVAL is returned.
+ */
+static int ep93xx_spi_calc_divisors(const struct ep93xx_spi *espi,
+				    struct ep93xx_spi_chip *chip,
+				    unsigned long rate)
+{
+	unsigned long spi_clk_rate = clk_get_rate(espi->clk);
+	int cpsr, scr;
+
+	/*
+	 * Make sure that max value is between values supported by the
+	 * controller. Note that minimum value is already checked in
+	 * ep93xx_spi_transfer().
+	 */
+	rate = clamp(rate, espi->min_rate, espi->max_rate);
+
+	/*
+	 * Calculate divisors so that we can get speed according the
+	 * following formula:
+	 *	rate = spi_clock_rate / (cpsr * (1 + scr))
+	 *
+	 * cpsr must be even number and starts from 2, scr can be any number
+	 * between 0 and 255.
+	 */
+	for (cpsr = 2; cpsr <= 254; cpsr += 2) {
+		for (scr = 0; scr <= 255; scr++) {
+			if ((spi_clk_rate / (cpsr * (scr + 1))) <= rate) {
+				chip->div_scr = (u8)scr;
+				chip->div_cpsr = (u8)cpsr;
+				return 0;
+			}
+		}
+	}
+
+	return -EINVAL;
+}
+
+/**
+ * ep93xx_spi_select_device() - select given SPI device
+ * @espi: ep93xx SPI controller struct
+ * @spi: SPI device to select
+ *
+ * Function asserts chipselect line as specified in @spi->chip_select in board
+ * specific manner.
+ *
+ * Note that this function is called from a thread context and can sleep.
+ */
+static inline void ep93xx_spi_select_device(const struct ep93xx_spi *espi,
+					    struct spi_device *spi)
+{
+	if (espi->cs_control)
+		espi->cs_control(spi, !!(spi->mode & SPI_CS_HIGH));
+}
+
+/**
+ * ep93xx_spi_deselect_device() - deselects given SPI device
+ * @espi: ep93xx SPI controller struct
+ * @spi: SPI device to deselect
+ *
+ * Function deasserts chipselect line as specified in @spi->chip_select in board
+ * specific manner.
+ *
+ * Note that this function is called from a thread context and can sleep.
+ */
+static inline void ep93xx_spi_deselect_device(const struct ep93xx_spi *espi,
+					      struct spi_device *spi)
+{
+	if (espi->cs_control)
+		espi->cs_control(spi, !(spi->mode & SPI_CS_HIGH));
+}
+
+/**
+ * ep93xx_spi_setup() - setup an SPI device
+ * @spi: SPI device to setup
+ *
+ * This function sets up SPI device mode, speed etc. Can be called multiple
+ * times for a single device. Returns %0 in case of success, negative error in
+ * case of failure. When this function returns success, the device is
+ * deselected.
+ */
+static int ep93xx_spi_setup(struct spi_device *spi)
+{
+	struct ep93xx_spi *espi = spi_master_get_devdata(spi->master);
+	struct ep93xx_spi_chip *chip;
+
+	if (spi->bits_per_word < 4 || spi->bits_per_word > 16) {
+		dev_err(&espi->pdev->dev, "invalid bits per word %d\n",
+			spi->bits_per_word);
+		return -EINVAL;
+	}
+
+	chip = spi_get_ctldata(spi);
+	if (!chip) {
+		chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+		if (!chip)
+			return -ENOMEM;
+
+		spi_set_ctldata(spi, chip);
+		chip->spi = spi;
+	}
+
+	if (spi->max_speed_hz != chip->rate) {
+		int err;
+
+		err = ep93xx_spi_calc_divisors(espi, chip, spi->max_speed_hz);
+		if (err != 0) {
+			spi_set_ctldata(spi, NULL);
+			kfree(chip);
+			return err;
+		}
+		chip->rate = spi->max_speed_hz;
+	}
+
+	chip->dss = bits_per_word_to_dss(spi->bits_per_word);
+
+	ep93xx_spi_deselect_device(espi, spi);
+	return 0;
+}
+
+/**
+ * ep93xx_spi_transfer() - queue message to be transferred
+ * @spi: target SPI device
+ * @msg: message to be transferred
+ *
+ * This function is called by SPI device drivers when they are going to transfer
+ * a new message. It simply puts the message in the queue and schedules
+ * workqueue to perform the actual transfer later on.
+ *
+ * Returns %0 on success and negative error in case of failure.
+ */
+static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message *msg)
+{
+	struct ep93xx_spi *espi = spi_master_get_devdata(spi->master);
+	struct spi_transfer *t;
+	unsigned long flags;
+
+	if (!msg || !msg->complete)
+		return -EINVAL;
+
+	/* first validate each transfer */
+	list_for_each_entry(t, &msg->transfers, transfer_list) {
+		if (t->bits_per_word) {
+			if (t->bits_per_word < 4 || t->bits_per_word > 16)
+				return -EINVAL;
+		}
+		if (t->speed_hz && t->speed_hz < espi->min_rate)
+				return -EINVAL;
+	}
+
+	/*
+	 * Now that we own the message, let's initialize it so that it is
+	 * suitable for us. We use @msg->status to signal whether there was
+	 * error in transfer and @msg->state is used to hold pointer to the
+	 * current transfer (or %NULL if no active current transfer).
+	 */
+	msg->state = NULL;
+	msg->status = 0;
+	msg->actual_length = 0;
+
+	spin_lock_irqsave(&espi->lock, flags);
+	if (!espi->running) {
+		spin_unlock_irqrestore(&espi->lock, flags);
+		return -ESHUTDOWN;
+	}
+	list_add_tail(&msg->queue, &espi->msg_queue);
+	queue_work(espi->wq, &espi->msg_work);
+	spin_unlock_irqrestore(&espi->lock, flags);
+
+	return 0;
+}
+
+/**
+ * ep93xx_spi_cleanup() - cleans up master controller specific state
+ * @spi: SPI device to cleanup
+ *
+ * This function releases master controller specific state for given @spi
+ * device.
+ */
+static void ep93xx_spi_cleanup(struct spi_device *spi)
+{
+	struct ep93xx_spi_chip *chip;
+
+	chip = spi_get_ctldata(spi);
+	if (chip) {
+		spi_set_ctldata(spi, NULL);
+		kfree(chip);
+	}
+}
+
+/**
+ * ep93xx_spi_chip_setup() - configures hardware according to given @chip
+ * @espi: ep93xx SPI controller struct
+ * @chip: chip specific settings
+ *
+ * This function sets up the actual hardware registers with settings given in
+ * @chip. Note that no validation is done so make sure that callers validate
+ * settings before calling this.
+ */
+static void ep93xx_spi_chip_setup(const struct ep93xx_spi *espi,
+				  const struct ep93xx_spi_chip *chip)
+{
+	u16 cr0;
+
+	cr0 = chip->div_scr << SSPCR0_SCR_SHIFT;
+	cr0 |= (chip->spi->mode & (SPI_CPHA|SPI_CPOL)) << SSPCR0_MODE_SHIFT;
+	cr0 |= chip->dss;
+
+	dev_dbg(&espi->pdev->dev, "setup: mode %d, cpsr %d, scr %d, dss %d\n",
+		chip->spi->mode, chip->div_cpsr, chip->div_scr, chip->dss);
+	dev_dbg(&espi->pdev->dev, "setup: cr0 %#x", cr0);
+
+	ep93xx_spi_write_u8(espi, SSPCPSR, chip->div_cpsr);
+	ep93xx_spi_write_u16(espi, SSPCR0, cr0);
+}
+
+/**
+ * ep93xx_spi_process_transfer() - processes one SPI transfer
+ * @espi: ep93xx SPI controller struct
+ * @msg: current message
+ * @t: transfer to process
+ *
+ * This function processes one SPI transfer given in @t. Function waits until
+ * transfer is complete (may sleep) and updates @msg->status based on whether
+ * transfer was succesfully processed or not.
+ */
+static void ep93xx_spi_process_transfer(struct ep93xx_spi *espi,
+					struct spi_message *msg,
+					struct spi_transfer *t)
+{
+	struct ep93xx_spi_chip *chip = spi_get_ctldata(msg->spi);
+
+	msg->state = t;
+
+	/*
+	 * Handle any transfer specific settings if needed. We use
+	 * temporary chip settings here and restore original later when
+	 * the transfer is finished.
+	 */
+	if (t->speed_hz || t->bits_per_word) {
+		struct ep93xx_spi_chip tmp_chip = *chip;
+
+		if (t->speed_hz) {
+			int err;
+
+			err = ep93xx_spi_calc_divisors(espi, &tmp_chip,
+						       t->speed_hz);
+			if (err) {
+				dev_err(&espi->pdev->dev,
+					"failed to adjust speed\n");
+				msg->status = err;
+				return;
+			}
+		}
+
+		if (t->bits_per_word)
+			tmp_chip.dss = bits_per_word_to_dss(t->bits_per_word);
+
+		/*
+		 * Set up temporary new hw settings for this transfer.
+		 */
+		ep93xx_spi_chip_setup(espi, &tmp_chip);
+	}
+
+	espi->rx = 0;
+	espi->tx = 0;
+
+	/*
+	 * Now everything is set up for the current transfer. Enable interrupts
+	 * and wait for the transfer to complete.
+	 */
+	ep93xx_spi_enable_interrupts(espi);
+	wait_for_completion(&espi->wait);
+
+	/*
+	 * In case of error during transmit, we bail out from processing
+	 * the message.
+	 */
+	if (msg->status)
+		return;
+
+	/*
+	 * After this transfer is finished, perform any possible
+	 * post-transfer actions requested by the protocol driver.
+	 */
+	if (t->delay_usecs) {
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		schedule_timeout(usecs_to_jiffies(t->delay_usecs));
+	}
+	if (t->cs_change) {
+		/*
+		 * In case protocol driver is asking us to drop the
+		 * chipselect briefly, we let the scheduler to handle
+		 * any "delay" here.
+		 */
+		if (!list_is_last(&t->transfer_list, &msg->transfers)) {
+			ep93xx_spi_deselect_device(espi, msg->spi);
+			cond_resched();
+			ep93xx_spi_select_device(espi, msg->spi);
+		}
+	}
+
+	if (t->speed_hz || t->bits_per_word)
+		ep93xx_spi_chip_setup(espi, chip);
+}
+
+/*
+ * ep93xx_spi_process_message() - process one SPI message
+ * @espi: ep93xx SPI controller struct
+ * @msg: message to process
+ *
+ * This function processes a single SPI message. We go through all transfers in
+ * the message and pass them to ep93xx_spi_process_transfer(). Chipselect is
+ * asserted during the whole message (unless per transfer cs_change is set).
+ *
+ * @msg->status contains %0 in case of success or negative error code in case of
+ * failure.
+ */
+static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
+				       struct spi_message *msg)
+{
+	unsigned long timeout;
+	struct spi_transfer *t;
+	int err;
+
+	/*
+	 * Enable the SPI controller and its clock.
+	 */
+	err = ep93xx_spi_enable(espi);
+	if (err) {
+		dev_err(&espi->pdev->dev, "failed to enable SPI controller\n");
+		msg->status = err;
+		return;
+	}
+
+	/*
+	 * Just to be sure: flush any data from RX FIFO.
+	 */
+	timeout = jiffies + msecs_to_jiffies(SPI_TIMEOUT);
+	while (ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_RNE) {
+		if (time_after(jiffies, timeout)) {
+			dev_warn(&espi->pdev->dev,
+				 "timeout while flushing RX fifo\n");
+			msg->status = -ETIMEDOUT;
+			return;
+		}
+		ep93xx_spi_read_u16(espi, SSPDR);
+	}
+
+	/*
+	 * We explicitly handle FIFO level. This way we don't have to check TX
+	 * FIFO status using %SSPSR_TNF bit which may cause RX FIFO overruns.
+	 */
+	espi->fifo_level = 0;
+
+	/*
+	 * Update SPI controller registers according to spi device and assert
+	 * the chipselect.
+	 */
+	ep93xx_spi_chip_setup(espi, spi_get_ctldata(msg->spi));
+	ep93xx_spi_select_device(espi, msg->spi);
+
+	list_for_each_entry(t, &msg->transfers, transfer_list) {
+		ep93xx_spi_process_transfer(espi, msg, t);
+		if (msg->status)
+			break;
+	}
+
+	/*
+	 * Now the whole message is transferred (or failed for some reason). We
+	 * deselect the device and disable the SPI controller.
+	 */
+	ep93xx_spi_deselect_device(espi, msg->spi);
+	ep93xx_spi_disable(espi);
+}
+
+#define work_to_espi(work) (container_of((work), struct ep93xx_spi, msg_work))
+
+/**
+ * ep93xx_spi_work() - EP93xx SPI workqueue worker function
+ * @work: work struct
+ *
+ * Workqueue worker function. This function is called when there are new
+ * SPI messages to be processed. Message is taken out from the queue and then
+ * passed to ep93xx_spi_process_message().
+ *
+ * After message is transferred, protocol driver is notified by calling
+ * @msg->complete(). In case of error, @msg->status is set to negative error
+ * number, otherwise it contains zero (and @msg->actual_length is updated).
+ */
+static void ep93xx_spi_work(struct work_struct *work)
+{
+	struct ep93xx_spi *espi = work_to_espi(work);
+	struct spi_message *msg;
+
+	spin_lock_irq(&espi->lock);
+	if (!espi->running || espi->current_msg ||
+		list_empty(&espi->msg_queue)) {
+		spin_unlock_irq(&espi->lock);
+		return;
+	}
+	msg = list_first_entry(&espi->msg_queue, struct spi_message, queue);
+	list_del_init(&msg->queue);
+	espi->current_msg = msg;
+	spin_unlock_irq(&espi->lock);
+
+	ep93xx_spi_process_message(espi, msg);
+
+	/*
+	 * Update the current message and re-schedule ourselves if there are
+	 * more messages in the queue.
+	 */
+	spin_lock_irq(&espi->lock);
+	espi->current_msg = NULL;
+	if (espi->running && !list_empty(&espi->msg_queue))
+		queue_work(espi->wq, &espi->msg_work);
+	spin_unlock_irq(&espi->lock);
+
+	/* notify the protocol driver that we are done with this message */
+	msg->complete(msg->context);
+}
+
+/**
+ * bits_per_word() - returns bits per word for current message
+ */
+static inline int bits_per_word(const struct ep93xx_spi *espi)
+{
+	struct spi_message *msg = espi->current_msg;
+	struct spi_transfer *t = msg->state;
+
+	return t->bits_per_word ? t->bits_per_word : msg->spi->bits_per_word;
+}
+
+static void ep93xx_do_write(struct ep93xx_spi *espi, struct spi_transfer *t)
+{
+	if (bits_per_word(espi) > 8) {
+		u16 tx_val = 0;
+
+		if (t->tx_buf)
+			tx_val = ((u16 *)t->tx_buf)[espi->tx];
+		ep93xx_spi_write_u16(espi, SSPDR, tx_val);
+		espi->tx += sizeof(tx_val);
+	} else {
+		u8 tx_val = 0;
+
+		if (t->tx_buf)
+			tx_val = ((u8 *)t->tx_buf)[espi->tx];
+		ep93xx_spi_write_u8(espi, SSPDR, tx_val);
+		espi->tx += sizeof(tx_val);
+	}
+}
+
+static void ep93xx_do_read(struct ep93xx_spi *espi, struct spi_transfer *t)
+{
+	if (bits_per_word(espi) > 8) {
+		u16 rx_val;
+
+		rx_val = ep93xx_spi_read_u16(espi, SSPDR);
+		if (t->rx_buf)
+			((u16 *)t->rx_buf)[espi->rx] = rx_val;
+		espi->rx += sizeof(rx_val);
+	} else {
+		u8 rx_val;
+
+		rx_val = ep93xx_spi_read_u8(espi, SSPDR);
+		if (t->rx_buf)
+			((u8 *)t->rx_buf)[espi->rx] = rx_val;
+		espi->rx += sizeof(rx_val);
+	}
+}
+
+/**
+ * ep93xx_spi_read_write() - perform next RX/TX transfer
+ * @espi: ep93xx SPI controller struct
+ *
+ * This function transfers next bytes (or half-words) to/from RX/TX FIFOs. If
+ * called several times, the whole transfer will be completed. Returns %0 when
+ * current transfer was not yet completed otherwise length of the transfer
+ * (>%0). When this function is finished, RX FIFO should be empty and TX FIFO
+ * should be full.
+ */
+static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
+{
+	struct spi_message *msg = espi->current_msg;
+	struct spi_transfer *t = msg->state;
+
+	/* read as long as RX FIFO has frames in it */
+	while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)) {
+		ep93xx_do_read(espi, t);
+		espi->fifo_level--;
+	}
+
+	/* write as long as TX FIFO has room */
+	while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
+		ep93xx_do_write(espi, t);
+		espi->fifo_level++;
+	}
+
+	if (espi->rx == t->len) {
+		msg->actual_length += t->len;
+		return t->len;
+	}
+
+	return 0;
+}
+
+/**
+ * ep93xx_spi_interrupt() - SPI interrupt handler
+ * @irq: IRQ number (not used)
+ * @dev_id: pointer to EP93xx controller struct
+ *
+ * This function handles TX/RX/ROR interrupts that come from the SPI controller.
+ * Returns %IRQ_HANDLED when interrupt was handled and %IRQ_NONE in case the
+ * @irq was not handled.
+ */
+static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
+{
+	struct ep93xx_spi *espi = dev_id;
+	u8 irq_status;
+
+	irq_status = ep93xx_spi_read_u8(espi, SSPIIR);
+
+	if (!(irq_status & (SSPIIR_RORIS | SSPIIR_TIS | SSPIIR_RIS)))
+		return IRQ_NONE; /* not for us */
+
+	/* clear the interrupt */
+	ep93xx_spi_write_u8(espi, SSPICR, 0);
+
+	/*
+	 * If we got ROR (receive overrun) interrupt we know that something is
+	 * wrong. Just abort the message.
+	 */
+	if (unlikely(irq_status & SSPIIR_RORIS)) {
+		dev_warn(&espi->pdev->dev,
+			 "receive overrun, aborting the message\n");
+		espi->current_msg->status = -EIO;
+	} else {
+		/*
+		 * Interrupt is either RX (RIS) or TX (TIS). For both cases we
+		 * simply execute next data transfer.
+		 */
+		if (!ep93xx_spi_read_write(espi)) {
+			/*
+			 * In normal case, there still is some processing left
+			 * for current transfer. Let's wait for the next
+			 * interrupt then.
+			 */
+			return IRQ_HANDLED;
+		}
+	}
+
+	/*
+	 * Current transfer is finished, either with error or with success. In
+	 * any case we disable interrupts and notify the worker to handle
+	 * any post-processing of the message.
+	 */
+	ep93xx_spi_disable_interrupts(espi);
+	complete(&espi->wait);
+	return IRQ_HANDLED;
+}
+
+static int __init ep93xx_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct ep93xx_spi_info *info;
+	struct ep93xx_spi *espi;
+	struct resource *res;
+	int error;
+
+	info = pdev->dev.platform_data;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*espi));
+	if (!master) {
+		dev_err(&pdev->dev, "failed to allocate spi master\n");
+		return -ENOMEM;
+	}
+
+	master->setup = ep93xx_spi_setup;
+	master->transfer = ep93xx_spi_transfer;
+	master->cleanup = ep93xx_spi_cleanup;
+	master->bus_num = 0;
+	master->num_chipselect = info->num_chipselect;
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
+
+	platform_set_drvdata(pdev, master);
+
+	espi = spi_master_get_devdata(master);
+	espi->cs_control = info->cs_control;
+
+	espi->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(espi->clk)) {
+		dev_err(&pdev->dev, "unable to get spi clock\n");
+		error = PTR_ERR(espi->clk);
+		goto fail_release_master;
+	}
+
+	spin_lock_init(&espi->lock);
+	init_completion(&espi->wait);
+
+	/*
+	 * Calculate maximum and minimum supported clock rates
+	 * for the controller.
+	 */
+	espi->max_rate = clk_get_rate(espi->clk) / 2;
+	espi->min_rate = clk_get_rate(espi->clk) / (254 * 256);
+	espi->pdev = pdev;
+
+	espi->irq = platform_get_irq(pdev, 0);
+	if (espi->irq < 0) {
+		error = -EBUSY;
+		dev_err(&pdev->dev, "failed to get irq resources\n");
+		goto fail_put_clock;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "unable to get iomem resource\n");
+		error = -ENODEV;
+		goto fail_put_clock;
+	}
+
+	res = request_mem_region(res->start, resource_size(res), pdev->name);
+	if (!res) {
+		dev_err(&pdev->dev, "unable to request iomem resources\n");
+		error = -EBUSY;
+		goto fail_put_clock;
+	}
+
+	espi->regs_base = ioremap(res->start, resource_size(res));
+	if (!espi->regs_base) {
+		dev_err(&pdev->dev, "failed to map resources\n");
+		error = -ENODEV;
+		goto fail_free_mem;
+	}
+
+	error = request_irq(espi->irq, ep93xx_spi_interrupt, 0,
+			    "ep93xx-spi", espi);
+	if (error) {
+		dev_err(&pdev->dev, "failed to request irq\n");
+		goto fail_unmap_regs;
+	}
+
+	espi->wq = create_singlethread_workqueue("ep93xx_spid");
+	if (!espi->wq) {
+		dev_err(&pdev->dev, "unable to create workqueue\n");
+		goto fail_free_irq;
+	}
+	INIT_WORK(&espi->msg_work, ep93xx_spi_work);
+	INIT_LIST_HEAD(&espi->msg_queue);
+	espi->running = true;
+
+	/* make sure that the hardware is disabled */
+	ep93xx_spi_write_u8(espi, SSPCR1, 0);
+
+	error =	spi_register_master(master);
+	if (error) {
+		dev_err(&pdev->dev, "failed to register SPI master\n");
+		goto fail_free_queue;
+	}
+
+	dev_info(&pdev->dev, "EP93xx SPI Controller at 0x%08lx irq %d\n",
+		 (unsigned long)res->start, espi->irq);
+
+	return 0;
+
+fail_free_queue:
+	destroy_workqueue(espi->wq);
+fail_free_irq:
+	free_irq(espi->irq, espi);
+fail_unmap_regs:
+	iounmap(espi->regs_base);
+fail_free_mem:
+	release_mem_region(res->start, resource_size(res));
+fail_put_clock:
+	clk_put(espi->clk);
+fail_release_master:
+	spi_master_put(master);
+	platform_set_drvdata(pdev, NULL);
+
+	return error;
+}
+
+static int __exit ep93xx_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct ep93xx_spi *espi = spi_master_get_devdata(master);
+	struct resource *res;
+
+	spin_lock_irq(&espi->lock);
+	espi->running = false;
+	spin_unlock_irq(&espi->lock);
+
+	destroy_workqueue(espi->wq);
+
+	/*
+	 * Complete remaining messages with %-ESHUTDOWN status.
+	 */
+	spin_lock_irq(&espi->lock);
+	while (!list_empty(&espi->msg_queue)) {
+		struct spi_message *msg;
+
+		msg = list_first_entry(&espi->msg_queue,
+				       struct spi_message, queue);
+		list_del_init(&msg->queue);
+		msg->status = -ESHUTDOWN;
+		spin_unlock_irq(&espi->lock);
+		msg->complete(msg->context);
+		spin_lock_irq(&espi->lock);
+	}
+	spin_unlock_irq(&espi->lock);
+
+	free_irq(espi->irq, espi);
+	iounmap(espi->regs_base);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(res->start, resource_size(res));
+	clk_put(espi->clk);
+	platform_set_drvdata(pdev, NULL);
+
+	spi_unregister_master(master);
+	return 0;
+}
+
+static struct platform_driver ep93xx_spi_driver = {
+	.driver		= {
+		.name	= "ep93xx-spi",
+		.owner	= THIS_MODULE,
+	},
+	.remove		= __exit_p(ep93xx_spi_remove),
+};
+
+static int __init ep93xx_spi_init(void)
+{
+	return platform_driver_probe(&ep93xx_spi_driver, ep93xx_spi_probe);
+}
+module_init(ep93xx_spi_init);
+
+static void __exit ep93xx_spi_exit(void)
+{
+	platform_driver_unregister(&ep93xx_spi_driver);
+}
+module_exit(ep93xx_spi_exit);
+
+MODULE_DESCRIPTION("EP93xx SPI Controller driver");
+MODULE_AUTHOR("Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ep93xx-spi");
-- 
1.5.6.5


------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Tuesday, April 20, 2010 8:12 AM, Mika Westerberg wrote:
> This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
> in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).
> 
> Signed-off-by: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>

Mika,

I'm still looking this over but I have one quick comment.

> +/**
> + * ep93xx_spi_select_device() - select given SPI device
> + * @espi: ep93xx SPI controller struct
> + * @spi: SPI device to select
> + *
> + * Function asserts chipselect line as specified in @spi->chip_select in board
> + * specific manner.
> + *
> + * Note that this function is called from a thread context and can sleep.
> + */
> +static inline void ep93xx_spi_select_device(const struct ep93xx_spi *espi,
> +					    struct spi_device *spi)
> +{
> +	if (espi->cs_control)
> +		espi->cs_control(spi, !!(spi->mode & SPI_CS_HIGH));
> +}
> +
> +/**
> + * ep93xx_spi_deselect_device() - deselects given SPI device
> + * @espi: ep93xx SPI controller struct
> + * @spi: SPI device to deselect
> + *
> + * Function deasserts chipselect line as specified in @spi->chip_select in board
> + * specific manner.
> + *
> + * Note that this function is called from a thread context and can sleep.
> + */
> +static inline void ep93xx_spi_deselect_device(const struct ep93xx_spi *espi,
> +					      struct spi_device *spi)
> +{
> +	if (espi->cs_control)
> +		espi->cs_control(spi, !(spi->mode & SPI_CS_HIGH));
> +}

These two functions can be combined.

/**
 * ep93xx_spi_chip_select() - select/deselect a given SPI device
 * @espi: ep93xx SPI controller struct
 * @spi: SPI device to select
 * @assert: flag to assert the chip select line
 *
 * Note that this function is called from a thread context and can sleep.
 */
static void ep93xx_spi_chip_select(const struct ep93xx_spi *espi,
					     struct spi_device *spi, int assert)
{
	int value = (spi->mode & SPI_CS_HIGH) ? !!assert : !assert;

	if (espi->cs_control)
		espi->cs_control(spi, value);
}

Then just do:

ep93xx_spi_select_device(espi, msg->spi, 1);	/* assert the spi chip select */

ep93xx_spi_select_device(espi, msg->spi, 0);	/* deassert the spi chip select */

Regards,
Hartley
------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Tue, Apr 20, 2010 at 12:24:26PM -0500, H Hartley Sweeten wrote:
> On Tuesday, April 20, 2010 8:12 AM, Mika Westerberg wrote:
> > This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
> > in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>
> 
> Mika,
> 
> I'm still looking this over but I have one quick comment.
> 
> > +/**
> > + * ep93xx_spi_select_device() - select given SPI device
> > + * @espi: ep93xx SPI controller struct
> > + * @spi: SPI device to select
> > + *
> > + * Function asserts chipselect line as specified in @spi->chip_select in board
> > + * specific manner.
> > + *
> > + * Note that this function is called from a thread context and can sleep.
> > + */
> > +static inline void ep93xx_spi_select_device(const struct ep93xx_spi *espi,
> > +					    struct spi_device *spi)
> > +{
> > +	if (espi->cs_control)
> > +		espi->cs_control(spi, !!(spi->mode & SPI_CS_HIGH));
> > +}
> > +
> > +/**
> > + * ep93xx_spi_deselect_device() - deselects given SPI device
> > + * @espi: ep93xx SPI controller struct
> > + * @spi: SPI device to deselect
> > + *
> > + * Function deasserts chipselect line as specified in @spi->chip_select in board
> > + * specific manner.
> > + *
> > + * Note that this function is called from a thread context and can sleep.
> > + */
> > +static inline void ep93xx_spi_deselect_device(const struct ep93xx_spi *espi,
> > +					      struct spi_device *spi)
> > +{
> > +	if (espi->cs_control)
> > +		espi->cs_control(spi, !(spi->mode & SPI_CS_HIGH));
> > +}
> 
> These two functions can be combined.
> 
> /**
>  * ep93xx_spi_chip_select() - select/deselect a given SPI device
>  * @espi: ep93xx SPI controller struct
>  * @spi: SPI device to select
>  * @assert: flag to assert the chip select line
>  *
>  * Note that this function is called from a thread context and can sleep.
>  */
> static void ep93xx_spi_chip_select(const struct ep93xx_spi *espi,
> 					     struct spi_device *spi, int assert)
> {
> 	int value = (spi->mode & SPI_CS_HIGH) ? !!assert : !assert;
> 
> 	if (espi->cs_control)
> 		espi->cs_control(spi, value);
> }
> 
> Then just do:
> 
> ep93xx_spi_select_device(espi, msg->spi, 1);	/* assert the spi chip select */
> 
> ep93xx_spi_select_device(espi, msg->spi, 0);	/* deassert the spi chip select */


I think it is more readable to do:

	ep93xx_spi_select_device(espi, msg->spi);

and

	ep93xx_spi_deselect_device(espi, msg->spi);

It can be seen from the function names what we are doing.

Thanks,
MW

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Wednesday, April 21, 2010 12:16 AM, Mika Westerberg wrote:
>> 
>> These two functions can be combined.
>> 
>> /**
>>  * ep93xx_spi_chip_select() - select/deselect a given SPI device
>>  * @espi: ep93xx SPI controller struct
>>  * @spi: SPI device to select
>>  * @assert: flag to assert the chip select line
>>  *
>>  * Note that this function is called from a thread context and can sleep.
>>  */
>> static void ep93xx_spi_chip_select(const struct ep93xx_spi *espi,
>> 					     struct spi_device *spi, int assert)
>> {
>> 	int value = (spi->mode & SPI_CS_HIGH) ? !!assert : !assert;
>> 
>> 	if (espi->cs_control)
>> 		espi->cs_control(spi, value);
>> }
>> 
>> Then just do:
>> 
>> ep93xx_spi_select_device(espi, msg->spi, 1);	/* assert the spi chip select */
>> 
>> ep93xx_spi_select_device(espi, msg->spi, 0);	/* deassert the spi chip select */
>
>
> I think it is more readable to do:
>
>	ep93xx_spi_select_device(espi, msg->spi);
>
> and
>
>	ep93xx_spi_deselect_device(espi, msg->spi);
>
> It can be seen from the function names what we are doing.

So rename the combined function to ep93xx_spi_cs_control and pass the "control"
parameter as a bool; true = selected, false = deselected.

Having the two almost identical functions is a bit redundant.  It also gives you
two places to have to maintain basically the same operation.  I think this is
what Grant was referring to with his previous comment.

Regards,
Hartley
------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Wed, Apr 21, 2010 at 11:47:13AM -0500, H Hartley Sweeten wrote:
> On Wednesday, April 21, 2010 12:16 AM, Mika Westerberg wrote:
> > I think it is more readable to do:
> >
> >	ep93xx_spi_select_device(espi, msg->spi);
> >
> > and
> >
> >	ep93xx_spi_deselect_device(espi, msg->spi);
> >
> > It can be seen from the function names what we are doing.
> 
> So rename the combined function to ep93xx_spi_cs_control and pass the "control"
> parameter as a bool; true = selected, false = deselected.
> 
> Having the two almost identical functions is a bit redundant.  It also gives you
> two places to have to maintain basically the same operation.  I think this is
> what Grant was referring to with his previous comment.

Agreed. Will do.

Thanks,
MW

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

Mika,

I have added some debug messages to the driver trying to figure out
how to chain the transfers in a message together in order to keep
the SFRM signal asserted for the entire message.  I still haven't
worked out a good solution but I did notice something else.

First, every spi transaction, including a single byte transfer, is
going to generate at least two interrupts.  One when the interrupts
are first enabled because the TX FIFO is empty.  And a second when
that byte has been transferred and the TX FIFO is again empty.

The first interrupt can be prevented by priming the TX FIFO before
enabling the interrupts.  All you need to do is call ep93xx_spi_read_write
right before ep93xx_spi_enable_interrupts.

Second, at high clock rates the RX FIFO will actually start to
fill as you are putting data into the TX FIFO.  If you add an inner
reader loop to the writer loop in ep93xx_spi_read_write you can
take advantage of this and reduce the number of interrupts generated
for large transfers.

For instance the mmc_spi driver regularly does 3 transfer messages
where the last transfer is a 512 byte read.  With your current v4
driver this message averages 69 interrupts to complete.  By adding
the inner reader it reduces the number of interrupts to an average
of 40.

Regards,
Hartley

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Wed, Apr 21, 2010 at 09:47:14PM -0500, H Hartley Sweeten wrote:
[...]
> First, every spi transaction, including a single byte transfer, is
> going to generate at least two interrupts.  One when the interrupts
> are first enabled because the TX FIFO is empty.  And a second when
> that byte has been transferred and the TX FIFO is again empty.
> 
> The first interrupt can be prevented by priming the TX FIFO before
> enabling the interrupts.  All you need to do is call ep93xx_spi_read_write
> right before ep93xx_spi_enable_interrupts.

OK thanks.

> Second, at high clock rates the RX FIFO will actually start to
> fill as you are putting data into the TX FIFO.  If you add an inner
> reader loop to the writer loop in ep93xx_spi_read_write you can
> take advantage of this and reduce the number of interrupts generated
> for large transfers.

I originally had inner read loop in the first version of the driver
but it wasn't functioning as it should (this was pointed out by
Martin). But now when we have explicit FIFO size handling, I think
that it can be added there again.

Thanks,
MW

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/22/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>  I have added some debug messages to the driver trying to figure out
>  how to chain the transfers in a message together in order to keep
>  the SFRM signal asserted for the entire message.  I still haven't
>  worked out a good solution but I did notice something else.
>
>  First, every spi transaction, including a single byte transfer, is
>  going to generate at least two interrupts.  One when the interrupts
>  are first enabled because the TX FIFO is empty.  And a second when
>  that byte has been transferred and the TX FIFO is again empty.
>
>  The first interrupt can be prevented by priming the TX FIFO before
>  enabling the interrupts.  All you need to do is call ep93xx_spi_read_write
>  right before ep93xx_spi_enable_interrupts.


>  Second, at high clock rates the RX FIFO will actually start to
>  fill as you are putting data into the TX FIFO.  If you add an inner
>  reader loop to the writer loop in ep93xx_spi_read_write you can
>  take advantage of this and reduce the number of interrupts generated
>  for large transfers.
>
>  For instance the mmc_spi driver regularly does 3 transfer messages
>  where the last transfer is a 512 byte read.  With your current v4
>  driver this message averages 69 interrupts to complete.  By adding
>  the inner reader it reduces the number of interrupts to an average
>  of 40.

I assume you mean something like

        /* read as long as RX FIFO has frames in it */
        while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) {
                ep93xx_do_read(espi, t);
                espi->fifo_level--;
        }

        /* write as long as TX FIFO has room */
        while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
                ep93xx_do_write(espi, t);
                espi->fifo_level++;
+
+               while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) {
+                       ep93xx_do_read(espi, t);
+                       espi->fifo_level--;
+               }
        }

Well, you're gonna love this :)

I've run some timing tests on this, and it turns out to give the same
data transfer speed but take more system CPU time: 64% instead of 60%,
i.e it is slower.

You are seeing a lower interrupt count, not because more useful work
is being done in each interrupt, but only because the extra code in
the inner loop makes the interrupt routine slower. Bear with me...

I'm using v4 with mika's continuity hack compiled for speed not space
and averaging the vmstat "sy" output column over 50 seconds while
block-reading 20 meg with dd from /dev/mmcblk0 to /dev/null using a
fairly old Sandisk 1MB SD card and clocking at 3.7 MHz.
All tests report 367 or 368 kB/sec; the variability of the system time
average in successive runs is about 1%

Figures are (commas separate values from successive runs)
v4 with Mika's hack:
     61.9,60.2%  234724,234727 irq/MB
with "while" reader in write loop:
     63.4,64.5%  197772,97801 irq/MB
with "if" reader in while loop:
     61.9,62.5%  197845,197847  irq/MB
with an "if" reader after the write loop:
     66.0,67.1%  278207,277166 irq/MB
with a "while" reader after the write loop:
     67.0%  275000 irq/MB

Further, on a suspicion about the silliness of the per-transfer
bits_per_word being checked right down the bottom of the lowest lever
byte-read/writing routine instead of once per transfer, I split
ep93xx_spi_read and _write into 8 and 16-bit variants, and checked
t->bits_per_word once outside the read and write loops.

This gives:
     60.0% 59.3%  285906 irq/MB

Note: it takes *more* interrupts to transfer a megabyte when the
interrupt routine is more efficient!

The reason is that almost all of those interrupts happen while waiting
for the last 4, 3, 2 or 1 bytes of a transfer to arrive in the RX
FIFO. On this dreadful chip there is no interrupt mechanism to say
"the TX buffer is empty and the received data has finished arriving".
There is only "the TX buffer is half empty or less" and "the RX buffer
is half full or more", so repeated interrupts now perform the function
that the old busy-wait loop performed in v1.

This is another good reason to unite multiple transfers into one
continuous stream, to have less of this effective busy-waiting with
interrupts spinning madly.

It now occurs to me that another step in CPU efficiency could be had
by abusing the receive-fifo-is-half-full interrupt to signal the
completion of a transfer. This would only work for transfers of four
or more words and would need some careful jiggery-pokery at end of
transfer to turn the tx-fifo-is-half-empty interrupt enable off and
ensure that exactly four words would end up in the RX FIFO.
It's a horrible thought, and I suspect that the DMA engine is the real answer.

   M

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/22/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>  First, every spi transaction, including a single byte transfer, is
>  going to generate at least two interrupts.  One when the interrupts
>  are first enabled because the TX FIFO is empty.  And a second when
>  that byte has been transferred and the TX FIFO is again empty.
>
>  The first interrupt can be prevented by priming the TX FIFO before
>  enabling the interrupts.  All you need to do is call ep93xx_spi_read_write
>  right before ep93xx_spi_enable_interrupts.

Nice. That increases the data throughput from 367 to 372 kB/sec and
reduces the CPU usage from 61 to 60.2% for large transfers

    M

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

Martin,

I'm replying to both of your previous messages.

On Thursday, April 22, 2010 7:28 AM, Martin Guy wrote:
> On 4/22/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>>  First, every spi transaction, including a single byte transfer, is
>>  going to generate at least two interrupts.  One when the interrupts
>>  are first enabled because the TX FIFO is empty.  And a second when
>>  that byte has been transferred and the TX FIFO is again empty.
>>
>>  The first interrupt can be prevented by priming the TX FIFO before
>>  enabling the interrupts.  All you need to do is call ep93xx_spi_read_write
>>  right before ep93xx_spi_enable_interrupts.
>
> Nice. That increases the data throughput from 367 to 372 kB/sec and
> reduces the CPU usage from 61 to 60.2% for large transfers

I figured "priming the fifo" before turning the transfer over to the
interrupt handler would help.

On Thursday, April 22, 2010 7:12 AM, Martin Guy wrote:
>>  Second, at high clock rates the RX FIFO will actually start to
>>  fill as you are putting data into the TX FIFO.  If you add an inner
>>  reader loop to the writer loop in ep93xx_spi_read_write you can
>>  take advantage of this and reduce the number of interrupts generated
>>  for large transfers.
>>
>>  For instance the mmc_spi driver regularly does 3 transfer messages
>>  where the last transfer is a 512 byte read.  With your current v4
>>  driver this message averages 69 interrupts to complete.  By adding
>>  the inner reader it reduces the number of interrupts to an average
>>  of 40.
>
> I assume you mean something like
>
>        /* read as long as RX FIFO has frames in it */
>        while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) {
>                ep93xx_do_read(espi, t);
>                espi->fifo_level--;
>        }
>
>        /* write as long as TX FIFO has room */
>        while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
>                ep93xx_do_write(espi, t);
>                espi->fifo_level++;
>+
>+               while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) {
>+                       ep93xx_do_read(espi, t);
>+                       espi->fifo_level--;
>+               }
>        }

Exactly what I did.

> Well, you're gonna love this :)
>
> I've run some timing tests on this, and it turns out to give the same
> data transfer speed but take more system CPU time: 64% instead of 60%,
> i.e it is slower.
>
> You are seeing a lower interrupt count, not because more useful work
> is being done in each interrupt, but only because the extra code in
> the inner loop makes the interrupt routine slower. Bear with me...
> 
> I'm using v4 with mika's continuity hack compiled for speed not space
> and averaging the vmstat "sy" output column over 50 seconds while
> block-reading 20 meg with dd from /dev/mmcblk0 to /dev/null using a
> fairly old Sandisk 1MB SD card and clocking at 3.7 MHz.
> All tests report 367 or 368 kB/sec; the variability of the system time
> average in successive runs is about 1%
> 
> Figures are (commas separate values from successive runs)
> v4 with Mika's hack:
>      61.9,60.2%  234724,234727 irq/MB
> with "while" reader in write loop:
>      63.4,64.5%  197772,97801 irq/MB
> with "if" reader in while loop:
>      61.9,62.5%  197845,197847  irq/MB
> with an "if" reader after the write loop:
>      66.0,67.1%  278207,277166 irq/MB
> with a "while" reader after the write loop:
>      67.0%  275000 irq/MB

I am surprised my this :-(.  I thought getting rid of the extra
interrupt context switches would help the data transfer time.  Oh well...

> Further, on a suspicion about the silliness of the per-transfer
> bits_per_word being checked right down the bottom of the lowest lever
> byte-read/writing routine instead of once per transfer, I split
> ep93xx_spi_read and _write into 8 and 16-bit variants, and checked
> t->bits_per_word once outside the read and write loops.
> 
> This gives:
>      60.0% 59.3%  285906 irq/MB

It looks like splitting the 8/16-bit read/write routines does help
with cpu usage compared to the tests above.  Was the data transfer
(kB/sec) any better?

> Note: it takes *more* interrupts to transfer a megabyte when the
> interrupt routine is more efficient!
> 
> The reason is that almost all of those interrupts happen while waiting
> for the last 4, 3, 2 or 1 bytes of a transfer to arrive in the RX
> FIFO. On this dreadful chip there is no interrupt mechanism to say
> "the TX buffer is empty and the received data has finished arriving".
> There is only "the TX buffer is half empty or less" and "the RX buffer
> is half full or more", so repeated interrupts now perform the function
> that the old busy-wait loop performed in v1.

Agree.  Kind of a pain...

> This is another good reason to unite multiple transfers into one
> continuous stream, to have less of this effective busy-waiting with
> interrupts spinning madly.

Yes.  Too bad we can't also combine multiple messages that might be in
the queue...

But, if this gets to complicated it might not be worth holding off
getting it merged, at least into linux-next or possibly into the
staging branch.  At that point it will be easier to provide patches.
I still have one hanging for Mika to expand the chip select options.

> It now occurs to me that another step in CPU efficiency could be had
> by abusing the receive-fifo-is-half-full interrupt to signal the
> completion of a transfer. This would only work for transfers of four
> or more words and would need some careful jiggery-pokery at end of
> transfer to turn the tx-fifo-is-half-empty interrupt enable off and
> ensure that exactly four words would end up in the RX FIFO.

That might work but I think it could break horribly.  The "jiggery-pokery"
could end up being pretty messy.

But, the tx-fifo-is-half-empty might be what is needed to handle the
multiple transfer merging.  We know the driver is finished transfering
the data to the fifo when (espi->tx == t->len).  At this point we are
just wating for the last (fifo_level) bytes to come in on the rx fifo,
this could be anything from 1 to 8 bytes.  If it's <= 4 the next interrupt
will be a tx-fifo-is-half-empty which is killing us trying to pull those
last bytes out of the rx fifo.  So how about something like this at the
end of ep93xx_spi_read_write:

	if (can_continue && espi->tx == t->len) {
		if (fifo_level < SPI_FIFO_SIZE/2) {
			/*
			 * Pull the next transfer out of the queue and
			 * use it to finish filling the tx fifo.  The
			 * next interrupt will complete this transfer.
			 * We then need to continue with the next transfer.
			 */
		}
	}

> It's a horrible thought, and I suspect that the DMA engine is the real answer.

The DMA engine might help but it's not here yet....

Regards,
Hartley

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/22/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>  > Further, on a suspicion about the silliness of the per-transfer
>  > bits_per_word being checked right down the bottom of the lowest lever
>  > byte-read/writing routine instead of once per transfer, I split
>  > ep93xx_spi_read and _write into 8 and 16-bit variants, and checked
>  > t->bits_per_word once outside the read and write loops.
>  >
>  > This gives:
>  >      60.0% 59.3%  285906 irq/MB
>
>
> It looks like splitting the 8/16-bit read/write routines does help
>  with cpu usage compared to the tests above.  Was the data transfer
>  (kB/sec) any better?

No, exactly the same.

>From a code poin tof view, one of the routines is just
if (bits > 8) {
   this16
} else {
   that8
}
so no loss in splitting it. You need two copies of the read and write
loops in the read_write routine, though it's only 8 extra lines of
code for a 1% CPU increase.
I don't have strong feelings about this it way.

> Yes.  Too bad we can't also combine multiple messages that might be in
>  the queue...

I wonder if it might be easier, if there are several parts to a
message, to just cook up  one big new transfer, copy the data into it
and just do that. It would mean copying every data block of course
since in the MMC/SD case they are 512+2+1.

>  But, if this gets to complicated it might not be worth holding off
>  getting it merged
Possibly. It depends on the timing of the merge windows. The current
code works with some devices.

>  > It now occurs to me that another step in CPU efficiency could be had
>  > by abusing the receive-fifo-is-half-full interrupt to signal the
>  > completion of a transfer. This would only work for transfers of four
>  > or more words and would need some careful jiggery-pokery at end of
>  > transfer to turn the tx-fifo-is-half-empty interrupt enable off and
>  > ensure that exactly four words would end up in the RX FIFO.
>
> That might work but I think it could break horribly.  The "jiggery-pokery"
>  could end up being pretty messy.

Yes. It is looking nasty and it would make a mess of the place where
the continuity stuff needs to happen to achieve actual functionality
improvements.

To get some idea of the potential wins I've been instrumenting the
code today to see how many interrupts it takes to receive the last 4
words
At 3.7MHz I'm seeing up to 3 interrupts waiting for the final draining
to happen, with an average of 1.36 (1 would be the perfect figure)
At 400kHz it takes up to 26 with an average of 9.92

Me, I only really care about the SD card case, where a half dozen
useless interrupts per 512-byte block is not going to impact on the
CPU usage much. In fact, by making the interrupt routine more complex
it may even end up being more cpu hungry in the end.

On the other hand, slow clock-speed devices are using needless high
CPU, especially if the transfer sizes are small as is typical with
command-response devices.

>  But, the tx-fifo-is-half-empty might be what is needed to handle the
>  multiple transfer merging.  We know the driver is finished transfering
>  the data to the fifo when (espi->tx == t->len).  At this point we are
>  just wating for the last (fifo_level) bytes to come in on the rx fifo,
>  this could be anything from 1 to 8 bytes.

Another instrumentation show that yes, at 3.7MHz, the data read on
each interrupt is between 1 and 8 words. I'm surprised it gets to 8
full - maybe when the interrupt is requested while some other IRQ
(ether? clock tick?) is already in progress? I'm instrumenting in a
way that shouldn't change the timing characteristics of the transfer
(ie a tiny printk's only after end of transfer)

>  > It's a horrible thought, and I suspect that the DMA engine is the real answer.
>
> The DMA engine might help but it's not here yet....

Yes. What I mean is that the prospect of that happening somewhere in
the future reduces the importance of doing hairy jiggery pokery with
the interrupt version.

     M

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Tuesday, April 20, 2010 8:12 AM, Mika Westerberg wrote:
>
> This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
> in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).
> 
> Signed-off-by: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>

Mika,

I discovered one gotcha with this driver.  Bear with me through this...

1. ep93xx_spi_process_message is called by the workqueue worker function to handle
   a spi_message queued in by the ep93xx_spi_transfer function.  Before starting
   the actual transfer the chip select to the device is asserted.
2. ep93xx_spi_process_transfer processes each transfer in the spi_message, one at a
   time, and does a wait_for_completion on each transfer.
3. When the entire spi_message has been transferred the chip select to the device
   is deasserted.

The problem is if a hardware design uses the SFRM1 pin as part of the chip select
logic.  The EP93xx User's Guide states that "the SFRMOUT signal behaves as a slave
Select" in Motorola SPI mode.  This signal behaves differently depending on the
SPI_MODE_* used.

Modes 0 and 2:
  SFRMOUT is forced HIGH during idle periods.
  Start of transmission, SFRMOUT is driven LOW.
  Single word transmission, SFRMOUT is returned to its idle HIGH state one
    SCLKOUT period after the last bit has been captured.
  Back-to-back transmissions, SFRMOUT signal is pulsed HIGH between each
    data word transfer.  On completion the SFRMOUT pin returns to its idle
    HIGH state one SCLKOUT period after the last bit has been captured.

Modes 1 and 3:
  SFRMOUT is forced HIGH during idle periods.
  Start of transmission, SFRMOUT is driven LOW.
  Single word transmission, SFRMOUT is returned to its idle HIGH state one
    SCLKOUT period after the last bit has been captured.
  Back-to-back transmissions, SFRMOUT signal is held LOW between each
    data word transfer.  On completion the SFRMOUT pin returns to its idle
    HIGH state one SCLKOUT period after the last bit has been captured.

So, since each transfer does a wait_for_completion, all the data is transmitted
which causes the SFRMOUT pin to go HIGH between each transfer in the message.

For devices like the sst25lf040a SPI Flash on the EDB93xx boards this causes a
problem.  To read data from that device you need to send a two part message.  The
first part is a write transfer with the one byte command and three byte address.
This is then followed by a read transfer to get the data.  But since the SFRMOUT
pin goes high this ends the command.

The driver Ryan and I worked on actually does work with the sst25lf040a on the
EDB93xx boards.  But now that I know what the issue is, it was actually a race
condition.  Sometimes it worked and sometimes it didn't...

I would think the Sim.One board (Martin?) would have the same issue with the mmc
card since that design uses the SFRMOUT pin directly for the chip select.

Is there anyway to keep the transfers going in a muiltipart message?

Regards,
Hartley
------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/20/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
> On Tuesday, April 20, 2010 8:12 AM, Mika Westerberg wrote:
> > This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
>  > in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).
>
>  I discovered one gotcha with this driver.  Bear with me through this...
>
>  1. ep93xx_spi_process_message is called by the workqueue worker function to handle
>    a spi_message queued in by the ep93xx_spi_transfer function.  Before starting
>    the actual transfer the chip select to the device is asserted.
>  2. ep93xx_spi_process_transfer processes each transfer in the spi_message, one at a
>    time, and does a wait_for_completion on each transfer.
>  3. When the entire spi_message has been transferred the chip select to the device
>    is deasserted.
>
>  The problem is if a hardware design uses the SFRM1 pin as part of the chip select
>  logic.  The EP93xx User's Guide states that "the SFRMOUT signal behaves as a slave
>  Select" in Motorola SPI mode.  This signal behaves differently depending on the
>  SPI_MODE_* used.
>
>  Modes 0 and 2:
>   SFRMOUT is forced HIGH during idle periods.
>   Start of transmission, SFRMOUT is driven LOW.
>   Single word transmission, SFRMOUT is returned to its idle HIGH state one
>     SCLKOUT period after the last bit has been captured.
>   Back-to-back transmissions, SFRMOUT signal is pulsed HIGH between each
>     data word transfer.  On completion the SFRMOUT pin returns to its idle
>     HIGH state one SCLKOUT period after the last bit has been captured.
>
>  Modes 1 and 3:
>   SFRMOUT is forced HIGH during idle periods.
>   Start of transmission, SFRMOUT is driven LOW.
>   Single word transmission, SFRMOUT is returned to its idle HIGH state one
>     SCLKOUT period after the last bit has been captured.
>   Back-to-back transmissions, SFRMOUT signal is held LOW between each
>     data word transfer.  On completion the SFRMOUT pin returns to its idle
>     HIGH state one SCLKOUT period after the last bit has been captured.
>
>  So, since each transfer does a wait_for_completion, all the data is transmitted
>  which causes the SFRMOUT pin to go HIGH between each transfer in the message.
>
>  For devices like the sst25lf040a SPI Flash on the EDB93xx boards this causes a
>  problem.  To read data from that device you need to send a two part message.  The
>  first part is a write transfer with the one byte command and three byte address.
>  This is then followed by a read transfer to get the data.  But since the SFRMOUT
>  pin goes high this ends the command.
>
>  The driver Ryan and I worked on actually does work with the sst25lf040a on the
>  EDB93xx boards.  But now that I know what the issue is, it was actually a race
>  condition.  Sometimes it worked and sometimes it didn't...
>
>  I would think the Sim.One board (Martin?) would have the same issue with the mmc
>  card since that design uses the SFRMOUT pin directly for the chip select.
>
>  Is there anyway to keep the transfers going in a muiltipart message?

I just changed the SPI mode selection, which is in the board
definition file, from Mode 0 to Mode 3 (the two modes that sample data
on the rising clock edge, as required by MMC/SD). It dramatically
increases linear block read throughput:
MMC: from 307kB/sec to 353kB/sec
SD: from 317 to 365
SD MLC: from 327 to 378
with 82000 interrupts per second and 60% cpu usage

presumably because the poor card isn't being deselected once every byte.

   M

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Tuesday, April 20, 2010 4:55 PM, Martin Guy wrote:
> On 4/20/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>> On Tuesday, April 20, 2010 8:12 AM, Mika Westerberg wrote:
>>> This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
>>> in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).
>>
>> I discovered one gotcha with this driver.  Bear with me through this...
>>
>> 1. ep93xx_spi_process_message is called by the workqueue worker function to handle
>>    a spi_message queued in by the ep93xx_spi_transfer function.  Before starting
>>    the actual transfer the chip select to the device is asserted.
>> 2. ep93xx_spi_process_transfer processes each transfer in the spi_message, one at a
>>    time, and does a wait_for_completion on each transfer.
>> 3. When the entire spi_message has been transferred the chip select to the device
>>    is deasserted.
>>
>> The problem is if a hardware design uses the SFRM1 pin as part of the chip select
>> logic.  The EP93xx User's Guide states that "the SFRMOUT signal behaves as a slave
>> Select" in Motorola SPI mode.  This signal behaves differently depending on the
>> SPI_MODE_* used.
>>
>> Modes 0 and 2:
>>  SFRMOUT is forced HIGH during idle periods.
>>  Start of transmission, SFRMOUT is driven LOW.
>>  Single word transmission, SFRMOUT is returned to its idle HIGH state one
>>    SCLKOUT period after the last bit has been captured.
>>  Back-to-back transmissions, SFRMOUT signal is pulsed HIGH between each
>>    data word transfer.  On completion the SFRMOUT pin returns to its idle
>>    HIGH state one SCLKOUT period after the last bit has been captured.
>>
>> Modes 1 and 3:
>>  SFRMOUT is forced HIGH during idle periods.
>>  Start of transmission, SFRMOUT is driven LOW.
>>  Single word transmission, SFRMOUT is returned to its idle HIGH state one
>>    SCLKOUT period after the last bit has been captured.
>>  Back-to-back transmissions, SFRMOUT signal is held LOW between each
>>    data word transfer.  On completion the SFRMOUT pin returns to its idle
>>    HIGH state one SCLKOUT period after the last bit has been captured.
>>
>> So, since each transfer does a wait_for_completion, all the data is transmitted
>> which causes the SFRMOUT pin to go HIGH between each transfer in the message.
>>
>> For devices like the sst25lf040a SPI Flash on the EDB93xx boards this causes a
>> problem.  To read data from that device you need to send a two part message.  The
>> first part is a write transfer with the one byte command and three byte address.
>> This is then followed by a read transfer to get the data.  But since the SFRMOUT
>> pin goes high this ends the command.
>>
>> The driver Ryan and I worked on actually does work with the sst25lf040a on the
>> EDB93xx boards.  But now that I know what the issue is, it was actually a race
>> condition.  Sometimes it worked and sometimes it didn't...
>>
>> I would think the Sim.One board (Martin?) would have the same issue with the mmc
>> card since that design uses the SFRMOUT pin directly for the chip select.
>>
>> Is there anyway to keep the transfers going in a muiltipart message?
>
> I just changed the SPI mode selection, which is in the board
> definition file, from Mode 0 to Mode 3 (the two modes that sample data
> on the rising clock edge, as required by MMC/SD). It dramatically
> increases linear block read throughput:
> MMC: from 307kB/sec to 353kB/sec
> SD: from 317 to 365
> SD MLC: from 327 to 378
> with 82000 interrupts per second and 60% cpu usage
>
> presumably because the poor card isn't being deselected once every byte.

Yes, Mode 3 should perform better since the protocol is not enforcing the
chip select deassertion between every byte.

You should still be getting a chip select deassertion between every transfer
in a multipart message since the Sim.One uses the SFRM1 line for the chip select.
I haven't actually looked through the mmc_spi driver to see if this happens.

Is there any possibility you could look at the actual bus with a logic analyzer?

My concern with the comments above is that currently this driver will not work
on the EDB93xx boards with the SPI Flash device.  I did hack one of my boards
to remove the SFRM1 signal from the chip select and it does "fix" the problem.
But, that's kind of an ugly solution for most users.

Regards,
Hartley

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Tuesday, April 20, 2010 5:11 PM, H Hartley Sweeten wrote:
> On Tuesday, April 20, 2010 4:55 PM, Martin Guy wrote:
>> On 4/20/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:

[snip]

>>> Is there anyway to keep the transfers going in a muiltipart message?
>>
>> I just changed the SPI mode selection, which is in the board
>> definition file, from Mode 0 to Mode 3 (the two modes that sample data
>> on the rising clock edge, as required by MMC/SD). It dramatically
>> increases linear block read throughput:
>> MMC: from 307kB/sec to 353kB/sec
>> SD: from 317 to 365
>> SD MLC: from 327 to 378
>> with 82000 interrupts per second and 60% cpu usage
>>
>> presumably because the poor card isn't being deselected once every byte.
>
> Yes, Mode 3 should perform better since the protocol is not enforcing the
> chip select deassertion between every byte.
>
> You should still be getting a chip select deassertion between every transfer
> in a multipart message since the Sim.One uses the SFRM1 line for the chip select.
> I haven't actually looked through the mmc_spi driver to see if this happens.
>
> Is there any possibility you could look at the actual bus with a logic analyzer?
>
> My concern with the comments above is that currently this driver will not work
> on the EDB93xx boards with the SPI Flash device.  I did hack one of my boards
> to remove the SFRM1 signal from the chip select and it does "fix" the problem.
> But, that's kind of an ugly solution for most users.

Just for the record.  Here are the (approximate) logic analyzer timings for the
Read-ID command to the SPI Flash (Mode 3, 20MHz max speed):

     Assertion of GPIO chip select: 0us
Assertion of SFRM1 (start of xfer): +22.5us
       4 byte Read-ID command xfer: +4.5us
    SFRM1 deasserts at end of xfer
Assertion of SFRM1 (start of xfer): +26.45us
      2 byte ManID/DevID read xfer: +2.25us
    SFRM1 deasserts at end of xfer
   Deassertion of GPIO chip select: +10.25us

So the entire 4+2 byte message took 65.95us, of which the actual data was only
6.75us.  The "dead-time" between the two spi transfers was 40.1% of the total
xfer time.

Regards,
Hartley


------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/21/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
> On Tuesday, April 20, 2010 4:55 PM, Martin Guy wrote:
>  > On 4/20/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>  >> So, since each transfer does a wait_for_completion, all the data is transmitted
>  >> which causes the SFRMOUT pin to go HIGH between each transfer in the message.
>  >> ...
>  >> I would think the Sim.One board (Martin?) would have the same issue with the mmc
>  >> card since that design uses the SFRMOUT pin directly for the chip select.
>  >>
>  >> Is there anyway to keep the transfers going in a muiltipart message?
>
>  You should still be getting a chip select deassertion between every transfer
>  in a multipart message since the Sim.One uses the SFRM1 line for the chip select.
>  I haven't actually looked through the mmc_spi driver to see if this happens.

OK, can I see if understand what you're saying, that
ep93xx_spi_process_message() calls ep93xx_spi_process_transfer()
multiple times using:

        list_for_each_entry(t, &msg->transfers, transfer_list) {
                ep93xx_spi_process_transfer(espi, msg, t);
 ...
and process_transfer() waits for each message block to be completely
transmitted and received, so a transaction that consists of several
messages will (or may) cause the device to be deselected between parts
of the same message, which may cause a failure.

I have noticed on card insertion, the last line of:

mmc0: problem reading switch capabilities, performance might suffer.
mmc0: host does not support reading read-only switch. assuming write-enable.
mmc0: new SD card on SPI
mmcblk0: mmc0:0000 SD    952 MiB
 mmcblk0: p1 p2 p4
mmcblk0: retrying using single block read

and the SDHC cards I have don't work at all, spewing tons of:
mmcblk0: error -38 sending status comand
mmcblk0: error -38 sending read/write command, response 0x4, card status 0xff04
end_request: I/O error, dev mmcblk0, sector 7744509

>  Is there any possibility you could look at the actual bus with a logic analyzer?
Not easily, but it seems a likely cause.
To prevent card deselection mid-message I think we would need to
handle multi-transfer messages by making the start of transfers 2..N
continuous with the end of transfers 1..N-1 instead of draining the
reply from each one before starting the next.

Am I on the right track?

    M

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Tuesday, April 20, 2010 6:10 PM, Martin Guy wrote:
> On 4/21/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>> On Tuesday, April 20, 2010 4:55 PM, Martin Guy wrote:
>>> On 4/20/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>>>> So, since each transfer does a wait_for_completion, all the data is transmitted
>>>> which causes the SFRMOUT pin to go HIGH between each transfer in the message.
>>>> ...
>>>> I would think the Sim.One board (Martin?) would have the same issue with the mmc
>>>> card since that design uses the SFRMOUT pin directly for the chip select.
>>>>
>>>> Is there anyway to keep the transfers going in a muiltipart message?
>>
>>  You should still be getting a chip select deassertion between every transfer
>>  in a multipart message since the Sim.One uses the SFRM1 line for the chip select.
>>  I haven't actually looked through the mmc_spi driver to see if this happens.
>
> OK, can I see if understand what you're saying, that
> ep93xx_spi_process_message() calls ep93xx_spi_process_transfer()
> multiple times using:
>
>        list_for_each_entry(t, &msg->transfers, transfer_list) {
>                ep93xx_spi_process_transfer(espi, msg, t);
> ...
> and process_transfer() waits for each message block to be completely
> transmitted and received, so a transaction that consists of several
> messages will (or may) cause the device to be deselected between parts
> of the same message, which may cause a failure.

Exactly.

> I have noticed on card insertion, the last line of:
> 
> mmc0: problem reading switch capabilities, performance might suffer.
> mmc0: host does not support reading read-only switch. assuming write-enable.
> mmc0: new SD card on SPI
> mmcblk0: mmc0:0000 SD    952 MiB
>  mmcblk0: p1 p2 p4
> mmcblk0: retrying using single block read

That message might be related to the SFRM1 line being deasserted.

You could try cutting the trace going to pin 1 (CS pin) of the mmc socket and
jumpering it to one of the EGPIO pins on JP15 (the LCD header) or JP16 (the
keypad header).  You will of course need to modify your platform init to use
the new chip select line.

> and the SDHC cards I have don't work at all, spewing tons of:
> mmcblk0: error -38 sending status comand
> mmcblk0: error -38 sending read/write command, response 0x4, card status 0xff04
> end_request: I/O error, dev mmcblk0, sector 7744509

I haven't seen those error messages before.  Of course I don't have any SDHC
cards... ;-)

>>  Is there any possibility you could look at the actual bus with a logic analyzer?
> Not easily, but it seems a likely cause.
> To prevent card deselection mid-message I think we would need to
> handle multi-transfer messages by making the start of transfers 2..N
> continuous with the end of transfers 1..N-1 instead of draining the
> reply from each one before starting the next.
> 
> Am I on the right track?

Yes.  I'm just not sure how to implement this.

Regards,
Hartley
------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Tue, Apr 20, 2010 at 08:52:26PM -0500, H Hartley Sweeten wrote:
> On Tuesday, April 20, 2010 6:10 PM, Martin Guy wrote:
> > I have noticed on card insertion, the last line of:
> > 
> > mmc0: problem reading switch capabilities, performance might suffer.
> > mmc0: host does not support reading read-only switch. assuming write-enable.
> > mmc0: new SD card on SPI
> > mmcblk0: mmc0:0000 SD    952 MiB
> >  mmcblk0: p1 p2 p4
> > mmcblk0: retrying using single block read
> 
> That message might be related to the SFRM1 line being deasserted.

This message comes, depending on the card you are using.

It is easily reproduced with following command:

	# fdisk -l

Problem there seems to be that fdisk (and also part of block layer that reads
partition table) seeks to the end of the disk and reads last block. I remember
reading some mail thread about the matter and it could be that some cards
do read-ahead which causes the response message to return failure (address error
as a response to STOP message which is completely bogus).

You can enable MMC debugging and see if this is the case with your card also.

I actually debugged this further some time ago and after failure if card status
register is read, there is no failure at all (card is in correct state).

> You could try cutting the trace going to pin 1 (CS pin) of the mmc socket and
> jumpering it to one of the EGPIO pins on JP15 (the LCD header) or JP16 (the
> keypad header).  You will of course need to modify your platform init to use
> the new chip select line.
> 
> > and the SDHC cards I have don't work at all, spewing tons of:
> > mmcblk0: error -38 sending status comand
> > mmcblk0: error -38 sending read/write command, response 0x4, card status 0xff04
> > end_request: I/O error, dev mmcblk0, sector 7744509
> 
> I haven't seen those error messages before.  Of course I don't have any SDHC
> cards... ;-)

With my TS-7260 board, SDHC cards work (but it uses GPIO lines).

MW

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Tue, Apr 20, 2010 at 08:52:26PM -0500, H Hartley Sweeten wrote:
> On Tuesday, April 20, 2010 6:10 PM, Martin Guy wrote:
> > Not easily, but it seems a likely cause.
> > To prevent card deselection mid-message I think we would need to
> > handle multi-transfer messages by making the start of transfers 2..N
> > continuous with the end of transfers 1..N-1 instead of draining the
> > reply from each one before starting the next.
> > 
> > Am I on the right track?
> 
> Yes.  I'm just not sure how to implement this.

I made a hack that allows transfer handler to continue to the next transfer directly
if there is no special per transfer settings in the message. Could you try that out
and report whether SFRMOUT works better with it?

Note that this is just to make sure that we are on the right track.

Patch is against v4 of the driver.

Thanks,
MW

diff --git a/drivers/spi/ep93xx_spi.c b/drivers/spi/ep93xx_spi.c
index 310032d..11dcdd1 100644
--- a/drivers/spi/ep93xx_spi.c
+++ b/drivers/spi/ep93xx_spi.c
@@ -106,6 +106,8 @@ struct ep93xx_spi {
 	unsigned long			min_rate;
 	unsigned long			max_rate;
 	bool				running;
+	bool				can_continue;
+	struct spi_transfer 		*last_transfer;
 	struct workqueue_struct		*wq;
 	struct work_struct		msg_work;
 	struct completion		wait;
@@ -380,6 +382,7 @@ static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message *msg)
 	struct ep93xx_spi *espi = spi_master_get_devdata(spi->master);
 	struct spi_transfer *t;
 	unsigned long flags;
+	bool can_continue = true;
 
 	if (!msg || !msg->complete)
 		return -EINVAL;
@@ -387,11 +390,21 @@ static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message *msg)
 	/* first validate each transfer */
 	list_for_each_entry(t, &msg->transfers, transfer_list) {
 		if (t->bits_per_word) {
+			can_continue = false;
 			if (t->bits_per_word < 4 || t->bits_per_word > 16)
 				return -EINVAL;
 		}
-		if (t->speed_hz && t->speed_hz < espi->min_rate)
+		if (t->speed_hz) {
+			can_continue = false;
+			if (t->speed_hz < espi->min_rate)
 				return -EINVAL;
+		}
+		if (t->cs_change &&
+			!list_is_last(&t->transfer_list, &msg->transfers)) {
+			can_continue = false;
+		}
+		if (t->delay_usecs)
+			can_continue = false;
 	}
 
 	/*
@@ -400,7 +413,7 @@ static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message *msg)
 	 * error in transfer and @msg->state is used to hold pointer to the
 	 * current transfer (or %NULL if no active current transfer).
 	 */
-	msg->state = NULL;
+	msg->state = (void *)can_continue;
 	msg->status = 0;
 	msg->actual_length = 0;
 
@@ -606,10 +619,33 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
 	ep93xx_spi_chip_setup(espi, spi_get_ctldata(msg->spi));
 	ep93xx_spi_select_device(espi, msg->spi);
 
-	list_for_each_entry(t, &msg->transfers, transfer_list) {
-		ep93xx_spi_process_transfer(espi, msg, t);
-		if (msg->status)
-			break;
+	if (msg->state) {
+		espi->can_continue = true;
+		espi->last_transfer = list_entry(msg->transfers.prev, struct spi_transfer,
+						 transfer_list);
+	} else {
+		espi->can_continue = false;
+		espi->last_transfer = NULL;
+	}
+
+	/*
+	 * In case there is no transfer specific settings in this message we
+	 * can transfer the whole message with interrupts. Otherwise we need
+	 * to perform transfer specific stuff in thread context.
+	 */
+	if (espi->can_continue) {
+		msg->state = list_first_entry(&msg->transfers, struct spi_transfer,
+					      transfer_list);
+		espi->rx = 0;
+		espi->tx = 0;
+		ep93xx_spi_enable_interrupts(espi);
+		wait_for_completion(&espi->wait);
+	} else {
+		list_for_each_entry(t, &msg->transfers, transfer_list) {
+			ep93xx_spi_process_transfer(espi, msg, t);
+			if (msg->status)
+				break;
+		}
 	}
 
 	/*
@@ -792,6 +828,24 @@ static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
 			 * interrupt then.
 			 */
 			return IRQ_HANDLED;
+		} else {
+			/*
+			 * If we can continue directly to the next transfer, do
+			 * that now.
+			 */
+			if (espi->can_continue) {
+				struct spi_message *msg = espi->current_msg;
+				struct spi_transfer *t = msg->state;
+
+				if (t != espi->last_transfer) {
+					msg->state = list_entry(t->transfer_list.next, struct spi_transfer,
+								transfer_list);
+					espi->rx = 0;
+					espi->tx = 0;
+					return IRQ_HANDLED;
+				}
+			}
+			/* otherwise we are ready */
 		}
 	}
 

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

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

On Wednesday, April 21, 2010 3:47 AM, Mika Westerberg wrote:
> On Tue, Apr 20, 2010 at 08:52:26PM -0500, H Hartley Sweeten wrote:
>> On Tuesday, April 20, 2010 6:10 PM, Martin Guy wrote:
>>> Not easily, but it seems a likely cause.
>>> To prevent card deselection mid-message I think we would need to
>>> handle multi-transfer messages by making the start of transfers 2..N
>>> continuous with the end of transfers 1..N-1 instead of draining the
>>> reply from each one before starting the next.
>>> 
>>> Am I on the right track?
>> 
>> Yes.  I'm just not sure how to implement this.
>
> I made a hack that allows transfer handler to continue to the next transfer directly
> if there is no special per transfer settings in the message. Could you try that out
> and report whether SFRMOUT works better with it?
>
> Note that this is just to make sure that we are on the right track.
>
> Patch is against v4 of the driver.
>
> Thanks,
> MW
>
> diff --git a/drivers/spi/ep93xx_spi.c b/drivers/spi/ep93xx_spi.c
> index 310032d..11dcdd1 100644
> --- a/drivers/spi/ep93xx_spi.c
> +++ b/drivers/spi/ep93xx_spi.c
> @@ -106,6 +106,8 @@ struct ep93xx_spi {
>  	unsigned long			min_rate;
>  	unsigned long			max_rate;
>  	bool				running;
> +	bool				can_continue;
> +	struct spi_transfer 		*last_transfer;
>  	struct workqueue_struct		*wq;
>  	struct work_struct		msg_work;
>  	struct completion		wait;
> @@ -380,6 +382,7 @@ static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message *msg)
>  	struct ep93xx_spi *espi = spi_master_get_devdata(spi->master);
>  	struct spi_transfer *t;
>  	unsigned long flags;
> +	bool can_continue = true;
>  
>  	if (!msg || !msg->complete)
>  		return -EINVAL;
> @@ -387,11 +390,21 @@ static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message *msg)
>  	/* first validate each transfer */
>  	list_for_each_entry(t, &msg->transfers, transfer_list) {
>  		if (t->bits_per_word) {
> +			can_continue = false;
>  			if (t->bits_per_word < 4 || t->bits_per_word > 16)
>  				return -EINVAL;
>  		}
> -		if (t->speed_hz && t->speed_hz < espi->min_rate)
> +		if (t->speed_hz) {
> +			can_continue = false;
> +			if (t->speed_hz < espi->min_rate)
>  				return -EINVAL;
> +		}
> +		if (t->cs_change &&
> +			!list_is_last(&t->transfer_list, &msg->transfers)) {
> +			can_continue = false;
> +		}
> +		if (t->delay_usecs)
> +			can_continue = false;
>  	}
>  
>  	/*
> @@ -400,7 +413,7 @@ static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message *msg)
>  	 * error in transfer and @msg->state is used to hold pointer to the
>  	 * current transfer (or %NULL if no active current transfer).
>  	 */
> -	msg->state = NULL;
> +	msg->state = (void *)can_continue;
>  	msg->status = 0;
>  	msg->actual_length = 0;
>  
> @@ -606,10 +619,33 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
>  	ep93xx_spi_chip_setup(espi, spi_get_ctldata(msg->spi));
>  	ep93xx_spi_select_device(espi, msg->spi);
>  
> -	list_for_each_entry(t, &msg->transfers, transfer_list) {
> -		ep93xx_spi_process_transfer(espi, msg, t);
> -		if (msg->status)
> -			break;
> +	if (msg->state) {
> +		espi->can_continue = true;
> +		espi->last_transfer = list_entry(msg->transfers.prev, struct spi_transfer,
> +						 transfer_list);
> +	} else {
> +		espi->can_continue = false;
> +		espi->last_transfer = NULL;
> +	}
> +
> +	/*
> +	 * In case there is no transfer specific settings in this message we
> +	 * can transfer the whole message with interrupts. Otherwise we need
> +	 * to perform transfer specific stuff in thread context.
> +	 */
> +	if (espi->can_continue) {
> +		msg->state = list_first_entry(&msg->transfers, struct spi_transfer,
> +					      transfer_list);
> +		espi->rx = 0;
> +		espi->tx = 0;
> +		ep93xx_spi_enable_interrupts(espi);
> +		wait_for_completion(&espi->wait);
> +	} else {
> +		list_for_each_entry(t, &msg->transfers, transfer_list) {
> +			ep93xx_spi_process_transfer(espi, msg, t);
> +			if (msg->status)
> +				break;
> +		}
>  	}
>  
>  	/*
> @@ -792,6 +828,24 @@ static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
>  			 * interrupt then.
>  			 */
>  			return IRQ_HANDLED;
> +		} else {
> +			/*
> +			 * If we can continue directly to the next transfer, do
> +			 * that now.
> +			 */
> +			if (espi->can_continue) {
> +				struct spi_message *msg = espi->current_msg;
> +				struct spi_transfer *t = msg->state;
> +
> +				if (t != espi->last_transfer) {
> +					msg->state = list_entry(t->transfer_list.next, struct spi_transfer,
> +								transfer_list);
> +					espi->rx = 0;
> +					espi->tx = 0;
> +					return IRQ_HANDLED;
> +				}
> +			}
> +			/* otherwise we are ready */
>  		}
>  	}
 
Same results are your v4 driver.  But, I think your on the right track.

I think the problem is in the ep93xx_spi_read_write routine.  That function
returns 0 as long as there is still data left in the current transfer.  The
only time it doesn't return 0, which will cause the can_continue, is when:

	if (espi->rx == t->len) {
		msg->actual_length += t->len;
		return t->len;
	}

At this point the tx and rx fifos will both be empty, which causes the SSP
peripheral to raise the SFRM signal.

A picture is worth a thousand words... Attached is a logic analyzer capture
of the Read-ID command and response from the SPI Flash.  EGPIO7 is my chip
select to the flash.  The first 4 SPI MOSI (Tx) blocks are the 0x90, 0x00,
0x00, 0x00 command to the flash.  You will notice that the SFRM line is
asserted for those bytes.  A bit later are the 2 SPI MISO (Rx) responses
from the flash with the ManID/DevID, again with the SFRM line asserted.

If the can_continue stuff worked correctly the second block should be right
with the first block.

I'm going to try hacking the spi flash driver to send the command/response
as a 6-byte full duplex message to make sure it works.

Regards,
Hartley

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #3: Type: text/plain, Size: 210 bytes --]

_______________________________________________
spi-devel-general mailing list
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Wed, Apr 21, 2010 at 01:00:56PM -0500, H Hartley Sweeten wrote:
> Same results are your v4 driver.  But, I think your on the right track.

Thanks for testing.

> I think the problem is in the ep93xx_spi_read_write routine.  That function
> returns 0 as long as there is still data left in the current transfer.  The
> only time it doesn't return 0, which will cause the can_continue, is when:
> 
> 	if (espi->rx == t->len) {
> 		msg->actual_length += t->len;
> 		return t->len;
> 	}
> 
> At this point the tx and rx fifos will both be empty, which causes the SSP
> peripheral to raise the SFRM signal.

True.

> A picture is worth a thousand words... Attached is a logic analyzer capture
> of the Read-ID command and response from the SPI Flash.  EGPIO7 is my chip
> select to the flash.  The first 4 SPI MOSI (Tx) blocks are the 0x90, 0x00,
> 0x00, 0x00 command to the flash.  You will notice that the SFRM line is
> asserted for those bytes.  A bit later are the 2 SPI MISO (Rx) responses
> from the flash with the ManID/DevID, again with the SFRM line asserted.

Yeah, it clearly shows that SFRMOUT is deasserted between transfers :(

I will try to work out some sort of hack which will keep the FIFOs from
emptying.

MW

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Ryan Mallon]

Mika Westerberg wrote:
> On Wed, Apr 21, 2010 at 01:00:56PM -0500, H Hartley Sweeten wrote:
>> Same results are your v4 driver.  But, I think your on the right track.
> 
> Thanks for testing.
> 
>> I think the problem is in the ep93xx_spi_read_write routine.  That function
>> returns 0 as long as there is still data left in the current transfer.  The
>> only time it doesn't return 0, which will cause the can_continue, is when:
>>
>> 	if (espi->rx == t->len) {
>> 		msg->actual_length += t->len;
>> 		return t->len;
>> 	}
>>
>> At this point the tx and rx fifos will both be empty, which causes the SSP
>> peripheral to raise the SFRM signal.
> 
> True.
> 
>> A picture is worth a thousand words... Attached is a logic analyzer capture
>> of the Read-ID command and response from the SPI Flash.  EGPIO7 is my chip
>> select to the flash.  The first 4 SPI MOSI (Tx) blocks are the 0x90, 0x00,
>> 0x00, 0x00 command to the flash.  You will notice that the SFRM line is
>> asserted for those bytes.  A bit later are the 2 SPI MISO (Rx) responses
>> from the flash with the ManID/DevID, again with the SFRM line asserted.
> 
> Yeah, it clearly shows that SFRMOUT is deasserted between transfers :(
> 
> I will try to work out some sort of hack which will keep the FIFOs from
> emptying.
> 
> MW

I still have not found time to test this, but I think this is the
problem we had we resulted in us using polling mode rather than
interrupts. If the spi fifo empties then it will deassert the frame
signal. By using polling code we could ensure that the fifos would not
drain at the incorrect time. The other solution is to use an externally
drivable signal for the frame (such as a gpio), but this is obviously
not possible on some existing hardware.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan-7Wk5F4Od5/oYd5yxfr4S2w@public.gmane.org         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Wed, Apr 21, 2010 at 01:00:56PM -0500, H Hartley Sweeten wrote:
>  
> Same results are your v4 driver.  But, I think your on the right track.
> 
> I think the problem is in the ep93xx_spi_read_write routine.  That function
> returns 0 as long as there is still data left in the current transfer.  The
> only time it doesn't return 0, which will cause the can_continue, is when:
> 
> 	if (espi->rx == t->len) {
> 		msg->actual_length += t->len;
> 		return t->len;
> 	}
> 
> At this point the tx and rx fifos will both be empty, which causes the SSP
> peripheral to raise the SFRM signal.

Hi again,

I crafted up next hack. It includes also your priming the FIFO
finding (thanks for that). Following patch is against v4 version
of the driver.

It performs another read/write after given transfer is finished.
I'm hoping that it could make it before SFRMOUT is deasserted.

Could you test this in your setup?

Thanks,
MW

diff --git a/drivers/spi/ep93xx_spi.c b/drivers/spi/ep93xx_spi.c
index 310032d..7a5c89e 100644
--- a/drivers/spi/ep93xx_spi.c
+++ b/drivers/spi/ep93xx_spi.c
@@ -105,6 +105,7 @@ struct ep93xx_spi {
 	int				irq;
 	unsigned long			min_rate;
 	unsigned long			max_rate;
+	bool				continuous;
 	bool				running;
 	struct workqueue_struct		*wq;
 	struct work_struct		msg_work;
@@ -380,6 +381,7 @@ static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message *msg)
 	struct ep93xx_spi *espi = spi_master_get_devdata(spi->master);
 	struct spi_transfer *t;
 	unsigned long flags;
+	bool continuous = true;
 
 	if (!msg || !msg->complete)
 		return -EINVAL;
@@ -389,9 +391,19 @@ static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message *msg)
 		if (t->bits_per_word) {
 			if (t->bits_per_word < 4 || t->bits_per_word > 16)
 				return -EINVAL;
+			continuous = false;
 		}
-		if (t->speed_hz && t->speed_hz < espi->min_rate)
+		if (t->speed_hz) {
+			if (t->speed_hz < espi->min_rate)
 				return -EINVAL;
+			continuous = false;
+		}
+		if (t->cs_change) {
+			if (!list_is_last(&t->transfer_list, &msg->transfers))
+				continuous = false;
+		}
+		if (t->delay_usecs)
+			continuous = false;
 	}
 
 	/*
@@ -399,8 +411,11 @@ static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message *msg)
 	 * suitable for us. We use @msg->status to signal whether there was
 	 * error in transfer and @msg->state is used to hold pointer to the
 	 * current transfer (or %NULL if no active current transfer).
+	 *
+	 * We hold continuation information in @msg->state while it is not
+	 * processed.
 	 */
-	msg->state = NULL;
+	msg->state = (void *)continuous;
 	msg->status = 0;
 	msg->actual_length = 0;
 
@@ -550,6 +565,91 @@ static void ep93xx_spi_process_transfer(struct ep93xx_spi *espi,
 		ep93xx_spi_chip_setup(espi, chip);
 }
 
+/**
+ * bits_per_word() - returns bits per word for current message
+ */
+static inline int bits_per_word(const struct ep93xx_spi *espi)
+{
+	struct spi_message *msg = espi->current_msg;
+	struct spi_transfer *t = msg->state;
+
+	return t->bits_per_word ? t->bits_per_word : msg->spi->bits_per_word;
+}
+
+static void ep93xx_do_write(struct ep93xx_spi *espi, struct spi_transfer *t)
+{
+	if (bits_per_word(espi) > 8) {
+		u16 tx_val = 0;
+
+		if (t->tx_buf)
+			tx_val = ((u16 *)t->tx_buf)[espi->tx];
+		ep93xx_spi_write_u16(espi, SSPDR, tx_val);
+		espi->tx += sizeof(tx_val);
+	} else {
+		u8 tx_val = 0;
+
+		if (t->tx_buf)
+			tx_val = ((u8 *)t->tx_buf)[espi->tx];
+		ep93xx_spi_write_u8(espi, SSPDR, tx_val);
+		espi->tx += sizeof(tx_val);
+	}
+}
+
+static void ep93xx_do_read(struct ep93xx_spi *espi, struct spi_transfer *t)
+{
+	if (bits_per_word(espi) > 8) {
+		u16 rx_val;
+
+		rx_val = ep93xx_spi_read_u16(espi, SSPDR);
+		if (t->rx_buf)
+			((u16 *)t->rx_buf)[espi->rx] = rx_val;
+		espi->rx += sizeof(rx_val);
+	} else {
+		u8 rx_val;
+
+		rx_val = ep93xx_spi_read_u8(espi, SSPDR);
+		if (t->rx_buf)
+			((u8 *)t->rx_buf)[espi->rx] = rx_val;
+		espi->rx += sizeof(rx_val);
+	}
+}
+
+/**
+ * ep93xx_spi_read_write() - perform next RX/TX transfer
+ * @espi: ep93xx SPI controller struct
+ *
+ * This function transfers next bytes (or half-words) to/from RX/TX FIFOs. If
+ * called several times, the whole transfer will be completed. Returns %0 when
+ * current transfer was not yet completed otherwise length of the transfer
+ * (>%0). When this function is finished, RX FIFO should be empty and TX FIFO
+ * should be full.
+ */
+static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
+{
+	struct spi_message *msg = espi->current_msg;
+	struct spi_transfer *t = msg->state;
+
+	/* read as long as RX FIFO has frames in it */
+	while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)) {
+		ep93xx_do_read(espi, t);
+		espi->fifo_level--;
+	}
+
+	/* write as long as TX FIFO has room */
+	while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
+		ep93xx_do_write(espi, t);
+		espi->fifo_level++;
+	}
+
+	if (espi->rx == t->len) {
+		msg->actual_length += t->len;
+		return t->len;
+	}
+
+	return 0;
+}
+
+
 /*
  * ep93xx_spi_process_message() - process one SPI message
  * @espi: ep93xx SPI controller struct
@@ -569,6 +669,8 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
 	struct spi_transfer *t;
 	int err;
 
+	espi->continuous = (bool)msg->state;
+
 	/*
 	 * Enable the SPI controller and its clock.
 	 */
@@ -606,12 +708,43 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
 	ep93xx_spi_chip_setup(espi, spi_get_ctldata(msg->spi));
 	ep93xx_spi_select_device(espi, msg->spi);
 
-	list_for_each_entry(t, &msg->transfers, transfer_list) {
-		ep93xx_spi_process_transfer(espi, msg, t);
-		if (msg->status)
-			break;
+	if (espi->continuous) {
+		/*
+		 * We can transfer all the transfers continuously.
+		 */
+		espi->rx = 0;
+		espi->tx = 0;
+		msg->state = list_first_entry(&msg->transfers, struct spi_transfer,
+					transfer_list);
+
+		/* prime the first transfer */
+		if (ep93xx_spi_read_write(espi)) {
+			/*
+			 * Transfer was completed. Pick next one and continue
+			 * if necessary.
+			 */
+			t = msg->state;
+			if (list_is_last(&t->transfer_list, &msg->transfers))
+				goto done;
+
+			espi->rx = 0;
+			espi->tx = 0;
+			msg->state = list_entry(t->transfer_list.next,
+						struct spi_transfer,
+						transfer_list);
+		}
+
+		ep93xx_spi_enable_interrupts(espi);
+		wait_for_completion(&espi->wait);
+	} else {
+		list_for_each_entry(t, &msg->transfers, transfer_list) {
+			ep93xx_spi_process_transfer(espi, msg, t);
+			if (msg->status)
+				break;
+		}
 	}
 
+done:
 	/*
 	 * Now the whole message is transferred (or failed for some reason). We
 	 * deselect the device and disable the SPI controller.
@@ -667,90 +800,6 @@ static void ep93xx_spi_work(struct work_struct *work)
 }
 
 /**
- * bits_per_word() - returns bits per word for current message
- */
-static inline int bits_per_word(const struct ep93xx_spi *espi)
-{
-	struct spi_message *msg = espi->current_msg;
-	struct spi_transfer *t = msg->state;
-
-	return t->bits_per_word ? t->bits_per_word : msg->spi->bits_per_word;
-}
-
-static void ep93xx_do_write(struct ep93xx_spi *espi, struct spi_transfer *t)
-{
-	if (bits_per_word(espi) > 8) {
-		u16 tx_val = 0;
-
-		if (t->tx_buf)
-			tx_val = ((u16 *)t->tx_buf)[espi->tx];
-		ep93xx_spi_write_u16(espi, SSPDR, tx_val);
-		espi->tx += sizeof(tx_val);
-	} else {
-		u8 tx_val = 0;
-
-		if (t->tx_buf)
-			tx_val = ((u8 *)t->tx_buf)[espi->tx];
-		ep93xx_spi_write_u8(espi, SSPDR, tx_val);
-		espi->tx += sizeof(tx_val);
-	}
-}
-
-static void ep93xx_do_read(struct ep93xx_spi *espi, struct spi_transfer *t)
-{
-	if (bits_per_word(espi) > 8) {
-		u16 rx_val;
-
-		rx_val = ep93xx_spi_read_u16(espi, SSPDR);
-		if (t->rx_buf)
-			((u16 *)t->rx_buf)[espi->rx] = rx_val;
-		espi->rx += sizeof(rx_val);
-	} else {
-		u8 rx_val;
-
-		rx_val = ep93xx_spi_read_u8(espi, SSPDR);
-		if (t->rx_buf)
-			((u8 *)t->rx_buf)[espi->rx] = rx_val;
-		espi->rx += sizeof(rx_val);
-	}
-}
-
-/**
- * ep93xx_spi_read_write() - perform next RX/TX transfer
- * @espi: ep93xx SPI controller struct
- *
- * This function transfers next bytes (or half-words) to/from RX/TX FIFOs. If
- * called several times, the whole transfer will be completed. Returns %0 when
- * current transfer was not yet completed otherwise length of the transfer
- * (>%0). When this function is finished, RX FIFO should be empty and TX FIFO
- * should be full.
- */
-static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
-{
-	struct spi_message *msg = espi->current_msg;
-	struct spi_transfer *t = msg->state;
-
-	/* read as long as RX FIFO has frames in it */
-	while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)) {
-		ep93xx_do_read(espi, t);
-		espi->fifo_level--;
-	}
-
-	/* write as long as TX FIFO has room */
-	while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
-		ep93xx_do_write(espi, t);
-		espi->fifo_level++;
-	}
-
-	if (espi->rx == t->len) {
-		msg->actual_length += t->len;
-		return t->len;
-	}
-
-	return 0;
-}
-
-/**
  * ep93xx_spi_interrupt() - SPI interrupt handler
  * @irq: IRQ number (not used)
  * @dev_id: pointer to EP93xx controller struct
@@ -792,6 +841,27 @@ static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
 			 * interrupt then.
 			 */
 			return IRQ_HANDLED;
+		} else {
+			struct spi_message *msg = espi->current_msg;
+			struct spi_transfer *t = msg->state;
+
+			if (!espi->continuous)
+				goto done;
+
+			if (!list_is_last(&t->transfer_list, &msg->transfers)) {
+				/*
+				 * Pick next transfer and refill the FIFO.
+				 */
+				msg->state = list_entry(t->transfer_list.next,
+							struct spi_transfer,
+							transfer_list);
+				espi->rx = 0;
+				espi->tx = 0;
+
+				ep93xx_spi_read_write(espi);
+				return IRQ_HANDLED;
+			}
+			/* we are done here */
 		}
 	}
 
@@ -800,6 +870,7 @@ static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
 	 * any case we disable interrupts and notify the worker to handle
 	 * any post-processing of the message.
 	 */
+done:
 	ep93xx_spi_disable_interrupts(espi);
 	complete(&espi->wait);
 	return IRQ_HANDLED;

------------------------------------------------------------------------------

RE: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Thursday, April 22, 2010 10:55 AM, Mika Westerberg wrote:
> On Wed, Apr 21, 2010 at 01:00:56PM -0500, H Hartley Sweeten wrote:
>>  
>> Same results are your v4 driver.  But, I think your on the right track.
>> 
>> I think the problem is in the ep93xx_spi_read_write routine.  That function
>> returns 0 as long as there is still data left in the current transfer.  The
>> only time it doesn't return 0, which will cause the can_continue, is when:
>> 
>> 	if (espi->rx == t->len) {
>> 		msg->actual_length += t->len;
>> 		return t->len;
>> 	}
>> 
>> At this point the tx and rx fifos will both be empty, which causes the SSP
>> peripheral to raise the SFRM signal.
>
> Hi again,
>
> I crafted up next hack. It includes also your priming the FIFO
> finding (thanks for that). 

Great!

> Following patch is against v4 version of the driver.
>
> It performs another read/write after given transfer is finished.
> I'm hoping that it could make it before SFRMOUT is deasserted.
> 
> Could you test this in your setup?

Same results.

I hacked your driver to allow me to toggle a gpio when the interrupt routine
starts and exits.

I captured the 4 byte write/2 byte read caused by the spi_write_then_read in
the sst25l driver when it does the Read-ID command.  The chip select goes low
followed by the first 4 byte transfer of the message due to the fifo's getting
primed.  This transfer actually doesn't finish since the rx data has not been
received.  So the interrupts just get enabled as normal.

The data starts transferring when the first byte is placed in the tx fifo.  It
takes about 4.45us to send/receive the 4 bytes.  Approximately 15.9us after the
last byte has been received the interrupt routine starts.  It then takes about
5.7us for the second 2 byte transfer to start.  So there is a total of about
21.6us between the two transfers where SFRM is deasserted.

Let me know if you want the picture posted.

[snip]

> +/**
> + * ep93xx_spi_read_write() - perform next RX/TX transfer
> + * @espi: ep93xx SPI controller struct
> + *
> + * This function transfers next bytes (or half-words) to/from RX/TX FIFOs. If
> + * called several times, the whole transfer will be completed. Returns %0 when
> + * current transfer was not yet completed otherwise length of the transfer
> + * (>%0). When this function is finished, RX FIFO should be empty and TX FIFO
> + * should be full.
> + */
> +static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
> +{
> +	struct spi_message *msg = espi->current_msg;
> +	struct spi_transfer *t = msg->state;
> +
> +	/* read as long as RX FIFO has frames in it */
> +	while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)) {
> +		ep93xx_do_read(espi, t);
> +		espi->fifo_level--;
> +	}
> +
> +	/* write as long as TX FIFO has room */
> +	while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
> +		ep93xx_do_write(espi, t);
> +		espi->fifo_level++;
> +	}
> +
> +	if (espi->rx == t->len) {
> +		msg->actual_length += t->len;
> +		return t->len;
> +	}
> +
> +	return 0;
> +}
> +
> +
>  /*
>   * ep93xx_spi_process_message() - process one SPI message
>   * @espi: ep93xx SPI controller struct
> @@ -569,6 +669,8 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
>  	struct spi_transfer *t;
>  	int err;
>  
> +	espi->continuous = (bool)msg->state;
> +
>  	/*
>  	 * Enable the SPI controller and its clock.
>  	 */
> @@ -606,12 +708,43 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
>  	ep93xx_spi_chip_setup(espi, spi_get_ctldata(msg->spi));
>  	ep93xx_spi_select_device(espi, msg->spi);
>  
> -	list_for_each_entry(t, &msg->transfers, transfer_list) {
> -		ep93xx_spi_process_transfer(espi, msg, t);
> -		if (msg->status)
> -			break;
> +	if (espi->continuous) {
> +		/*
> +		 * We can transfer all the transfers continuously.
> +		 */
> +		espi->rx = 0;
> +		espi->tx = 0;
> +		msg->state = list_first_entry(&msg->transfers, struct spi_transfer,
> +					transfer_list);
> +
> +		/* prime the first transfer */
> +		if (ep93xx_spi_read_write(espi)) {

On the first read/write for a transfer this condition can never succeed.

The ep93xx_spi_read_write function first empties the rx fifo then fills the
tx fifo.  Putting any data in the tx fifo means that you are going to have to
come back to empty the rx fifo.

So, the block below never executes.

> +			/*
> +			 * Transfer was completed. Pick next one and continue
> +			 * if necessary.
> +			 */
> +			t = msg->state;
> +			if (list_is_last(&t->transfer_list, &msg->transfers))
> +				goto done;
> +
> +			espi->rx = 0;
> +			espi->tx = 0;
> +			msg->state = list_entry(t->transfer_list.next,
> +						struct spi_transfer,
> +						transfer_list);
> +		}
> +
> +		ep93xx_spi_enable_interrupts(espi);
> +		wait_for_completion(&espi->wait);

Even if the first read/write did succeed you are not starting the second transfer.
This is just passing the operation off to the interrupt routine using the second
transfer in the message.

[snip]

> -/**
>   * ep93xx_spi_interrupt() - SPI interrupt handler
>   * @irq: IRQ number (not used)
>   * @dev_id: pointer to EP93xx controller struct
> @@ -792,6 +841,27 @@ static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
>  			 * interrupt then.
>  			 */
>  			return IRQ_HANDLED;
> +		} else {
> +			struct spi_message *msg = espi->current_msg;
> +			struct spi_transfer *t = msg->state;
> +
> +			if (!espi->continuous)
> +				goto done;
> +
> +			if (!list_is_last(&t->transfer_list, &msg->transfers)) {
> +				/*
> +				 * Pick next transfer and refill the FIFO.
> +				 */
> +				msg->state = list_entry(t->transfer_list.next,
> +							struct spi_transfer,
> +							transfer_list);
> +				espi->rx = 0;
> +				espi->tx = 0;
> +
> +				ep93xx_spi_read_write(espi);
> +				return IRQ_HANDLED;
> +			}

Here the N transfer has been completed and we are trying to continue with the N+1
transfer.  This should work but I need to figure out a way to capture it with my
logic analyzer.  The only problem is if the N transfer is complete that means the
tx and rx fifos are empty so the SFRM signal is going to be deasserted.  The N+1
transfer might start really soon after but there will still be a glitch in the SFRM
signal.

> +			/* we are done here */

Regards,
Hartley

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Thu, Apr 22, 2010 at 03:43:24PM -0500, H Hartley Sweeten wrote:
> On Thursday, April 22, 2010 10:55 AM, Mika Westerberg wrote:
> > Could you test this in your setup?
> 
> Same results.

OK. thanks for testing.

> I hacked your driver to allow me to toggle a gpio when the interrupt routine
> starts and exits.
> 
> I captured the 4 byte write/2 byte read caused by the spi_write_then_read in
> the sst25l driver when it does the Read-ID command.  The chip select goes low
> followed by the first 4 byte transfer of the message due to the fifo's getting
> primed.  This transfer actually doesn't finish since the rx data has not been
> received.  So the interrupts just get enabled as normal.
> 
> The data starts transferring when the first byte is placed in the tx fifo.  It
> takes about 4.45us to send/receive the 4 bytes.  Approximately 15.9us after the
> last byte has been received the interrupt routine starts.  It then takes about
> 5.7us for the second 2 byte transfer to start.  So there is a total of about
> 21.6us between the two transfers where SFRM is deasserted.

If I understand this correctly, for this particular message it might have worked if
the priming is not done? What I mean is that if we start the transfer in interrupt
handler and after the transfer is finished, immediately continue with the next one
(like in the patch) could it make SFRM to stay asserted (at least shorten the gap)?

> >  /*
> >   * ep93xx_spi_process_message() - process one SPI message
> >   * @espi: ep93xx SPI controller struct
> > @@ -569,6 +669,8 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
> >  	struct spi_transfer *t;
> >  	int err;
> >  
> > +	espi->continuous = (bool)msg->state;
> > +
> >  	/*
> >  	 * Enable the SPI controller and its clock.
> >  	 */
> > @@ -606,12 +708,43 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
> >  	ep93xx_spi_chip_setup(espi, spi_get_ctldata(msg->spi));
> >  	ep93xx_spi_select_device(espi, msg->spi);
> >  
> > -	list_for_each_entry(t, &msg->transfers, transfer_list) {
> > -		ep93xx_spi_process_transfer(espi, msg, t);
> > -		if (msg->status)
> > -			break;
> > +	if (espi->continuous) {
> > +		/*
> > +		 * We can transfer all the transfers continuously.
> > +		 */
> > +		espi->rx = 0;
> > +		espi->tx = 0;
> > +		msg->state = list_first_entry(&msg->transfers, struct spi_transfer,
> > +					transfer_list);
> > +
> > +		/* prime the first transfer */
> > +		if (ep93xx_spi_read_write(espi)) {
> 
> On the first read/write for a transfer this condition can never succeed.
> 
> The ep93xx_spi_read_write function first empties the rx fifo then fills the
> tx fifo.  Putting any data in the tx fifo means that you are going to have to
> come back to empty the rx fifo.
> 
> So, the block below never executes.

I also figured that out after I sent the hack to you...

> > +			/*
> > +			 * Transfer was completed. Pick next one and continue
> > +			 * if necessary.
> > +			 */
> > +			t = msg->state;
> > +			if (list_is_last(&t->transfer_list, &msg->transfers))
> > +				goto done;
> > +
> > +			espi->rx = 0;
> > +			espi->tx = 0;
> > +			msg->state = list_entry(t->transfer_list.next,
> > +						struct spi_transfer,
> > +						transfer_list);
> > +		}
> > +
> > +		ep93xx_spi_enable_interrupts(espi);
> > +		wait_for_completion(&espi->wait);
> 
> Even if the first read/write did succeed you are not starting the second transfer.
> This is just passing the operation off to the interrupt routine using the second
> transfer in the message.

Right.

> > -/**
> >   * ep93xx_spi_interrupt() - SPI interrupt handler
> >   * @irq: IRQ number (not used)
> >   * @dev_id: pointer to EP93xx controller struct
> > @@ -792,6 +841,27 @@ static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
> >  			 * interrupt then.
> >  			 */
> >  			return IRQ_HANDLED;
> > +		} else {
> > +			struct spi_message *msg = espi->current_msg;
> > +			struct spi_transfer *t = msg->state;
> > +
> > +			if (!espi->continuous)
> > +				goto done;
> > +
> > +			if (!list_is_last(&t->transfer_list, &msg->transfers)) {
> > +				/*
> > +				 * Pick next transfer and refill the FIFO.
> > +				 */
> > +				msg->state = list_entry(t->transfer_list.next,
> > +							struct spi_transfer,
> > +							transfer_list);
> > +				espi->rx = 0;
> > +				espi->tx = 0;
> > +
> > +				ep93xx_spi_read_write(espi);
> > +				return IRQ_HANDLED;
> > +			}
> 
> Here the N transfer has been completed and we are trying to continue with the N+1
> transfer.  This should work but I need to figure out a way to capture it with my
> logic analyzer.  The only problem is if the N transfer is complete that means the
> tx and rx fifos are empty so the SFRM signal is going to be deasserted.  The N+1
> transfer might start really soon after but there will still be a glitch in the SFRM
> signal.

It would be great if you manage to capture it with the logic analyzer.
Even if we get small glitch it means that we are going into right
direction (then we just need to figure out how to keep the FIFO
from emptying).

Other alternative is to use polling mode in v3 of the driver and
see if it works better.

Thanks,
MW

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Thursday, April 22, 2010 10:20 PM, Mika Westerberg wrote:
> On Thu, Apr 22, 2010 at 03:43:24PM -0500, H Hartley Sweeten wrote:
>> On Thursday, April 22, 2010 10:55 AM, Mika Westerberg wrote:
>>> Could you test this in your setup?
>> 
>> Same results.
>
> OK. thanks for testing.
>
>> I hacked your driver to allow me to toggle a gpio when the interrupt routine
>> starts and exits.
>> 
>> I captured the 4 byte write/2 byte read caused by the spi_write_then_read in
>> the sst25l driver when it does the Read-ID command.  The chip select goes low
>> followed by the first 4 byte transfer of the message due to the fifo's getting
>> primed.  This transfer actually doesn't finish since the rx data has not been
>> received.  So the interrupts just get enabled as normal.
>> 
>> The data starts transferring when the first byte is placed in the tx fifo.  It
>> takes about 4.45us to send/receive the 4 bytes.  Approximately 15.9us after the
>> last byte has been received the interrupt routine starts.  It then takes about
>> 5.7us for the second 2 byte transfer to start.  So there is a total of about
>> 21.6us between the two transfers where SFRM is deasserted.
>
> If I understand this correctly, for this particular message it might have worked if
> the priming is not done? What I mean is that if we start the transfer in interrupt
> handler and after the transfer is finished, immediately continue with the next one
> (like in the patch) could it make SFRM to stay asserted (at least shorten the gap)?

Hmmm.. It might have.  I didn't test that before I started hacking...

>>>  /*
>>>   * ep93xx_spi_process_message() - process one SPI message
>>>   * @espi: ep93xx SPI controller struct
>>> @@ -569,6 +669,8 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
>>>  	struct spi_transfer *t;
>>>  	int err;
>>>  
>>> +	espi->continuous = (bool)msg->state;
>>> +
>>>  	/*
>>>  	 * Enable the SPI controller and its clock.
>>>  	 */
>>> @@ -606,12 +708,43 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
>>>  	ep93xx_spi_chip_setup(espi, spi_get_ctldata(msg->spi));
>>>  	ep93xx_spi_select_device(espi, msg->spi);
>>>  
>>> -	list_for_each_entry(t, &msg->transfers, transfer_list) {
>>> -		ep93xx_spi_process_transfer(espi, msg, t);
>>> -		if (msg->status)
>>> -			break;
>>> +	if (espi->continuous) {
>>> +		/*
>>> +		 * We can transfer all the transfers continuously.
>>> +		 */
>>> +		espi->rx = 0;
>>> +		espi->tx = 0;
>>> +		msg->state = list_first_entry(&msg->transfers, struct spi_transfer,
>>> +					transfer_list);
>>> +
>>> +		/* prime the first transfer */
>>> +		if (ep93xx_spi_read_write(espi)) {
>> 
>> On the first read/write for a transfer this condition can never succeed.
>> 
>> The ep93xx_spi_read_write function first empties the rx fifo then fills the
>> tx fifo.  Putting any data in the tx fifo means that you are going to have to
>> come back to empty the rx fifo.
>> 
>> So, the block below never executes.
>
> I also figured that out after I sent the hack to you...
>
>>> +			/*
>>> +			 * Transfer was completed. Pick next one and continue
>>> +			 * if necessary.
>>> +			 */
>>> +			t = msg->state;
>>> +			if (list_is_last(&t->transfer_list, &msg->transfers))
>>> +				goto done;
>>> +
>>> +			espi->rx = 0;
>>> +			espi->tx = 0;
>>> +			msg->state = list_entry(t->transfer_list.next,
>>> +						struct spi_transfer,
>>> +						transfer_list);
>>> +		}
>>> +
>>> +		ep93xx_spi_enable_interrupts(espi);
>>> +		wait_for_completion(&espi->wait);
>> 
>> Even if the first read/write did succeed you are not starting the second transfer.
>> This is just passing the operation off to the interrupt routine using the second
>> transfer in the message.
>
> Right.
>
>>> -/**
>>>   * ep93xx_spi_interrupt() - SPI interrupt handler
>>>   * @irq: IRQ number (not used)
>>>   * @dev_id: pointer to EP93xx controller struct
>>> @@ -792,6 +841,27 @@ static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
>>>  			 * interrupt then.
>>>  			 */
>>>  			return IRQ_HANDLED;
>>> +		} else {
>>> +			struct spi_message *msg = espi->current_msg;
>>> +			struct spi_transfer *t = msg->state;
>>> +
>>> +			if (!espi->continuous)
>>> +				goto done;
>>> +
>>> +			if (!list_is_last(&t->transfer_list, &msg->transfers)) {
>>> +				/*
>>> +				 * Pick next transfer and refill the FIFO.
>>> +				 */
>>> +				msg->state = list_entry(t->transfer_list.next,
>>> +							struct spi_transfer,
>>> +							transfer_list);
>>> +				espi->rx = 0;
>>> +				espi->tx = 0;
>>> +
>>> +				ep93xx_spi_read_write(espi);
>>> +				return IRQ_HANDLED;
>>> +			}
>>
>> Here the N transfer has been completed and we are trying to continue with the N+1
>> transfer.  This should work but I need to figure out a way to capture it with my
>> logic analyzer.  The only problem is if the N transfer is complete that means the
>> tx and rx fifos are empty so the SFRM signal is going to be deasserted.  The N+1
>> transfer might start really soon after but there will still be a glitch in the SFRM
>> signal.
>
> It would be great if you manage to capture it with the logic analyzer.
> Even if we get small glitch it means that we are going into right
> direction (then we just need to figure out how to keep the FIFO
> from emptying).

I still haven't been able to catch this.

> Other alternative is to use polling mode in v3 of the driver and
> see if it works better.

I think the interrupt mode is still the better way to go.

I was able to hack your driver to keep the continuous transfer going in a multi-transaction
message.  Unfortunately I don't have an easy way to generate a diff to send the necessary
patch.  The hack does work with the sst25l driver.  It correctly detects the end of the
first 4 byte transfer and immediately chains in the second 2 byte transfer.  It ends up
looking like this:

WWWWwwIRRRRFCRRFi

Where:
  'W' is a write for the current transfer
  'w' is a write for the next transfer
  'I' is the start of the interrupt
  'R' is a read for the current transfer
  'F' is the completion of the current transfer
  'C' is the chain to the next transfer
  'i' is the end of the interrupt

So, at the start of the message the fifo's get primed with the 4+2 of the entire message.
Then interrupts are enabled.  When the interrupt occurs the first 4 byte transfer is read.
When that completes the interrupt routine chains to the next transfer and then reads the
next 2 bytes.  When that completes the message is finished so the interrupts get disabled
and espi->wait is completed.

Unfortunately is seems to break the mmc_spi driver for some reason.  I still haven't
figured out why.

If you can post your latest updates to the v4 driver I will create a patch against it
so others can test this also.

Regards,
Hartley
------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Friday, April 23, 2010 10:24 AM, H Hartley Sweeten wrote:
> I was able to hack your driver to keep the continuous transfer going in a
> multi-transaction message.  Unfortunately I don't have an easy way to
> generate a diff to send the necessary patch.  The hack does work with the
> sst25l driver.  It correctly detects the end of the first 4 byte transfer
> and immediately chains in the second 2 byte transfer.  It ends up looking
> like this:

Sorry for the length of this message.  I want to give as much detail as
possible so hopefully someone has an idea on this... ;-)


I modified my hack a bit so that the message transactions are dumped at
the end of all transactions in the message.  I'm using Mika's hack to
determine if the message could be continuous then displaying a debug
about the message.  By setting the 'continuous' flag to false right after
that it disables my hack to make the transaction continuous so the
message gets processed in the normal fashion.

The debug output decodes as:
  S   a transfer specific setting was made (in ep93xx_spi_process_transfer)
      NOTE: I have never actually seen this happen.
  +   start of a transfer (in ep93xx_spi_process_fransfer).
  -   end of a transfer (in ep93xx_spi_process_fransfer).
  I   entered ep93xx_spi_interrupt
  I   exited ep93xx_spi_interrupt
  C   chained to the next transfer in a continuous transaction
  nR  the n number of fifo reads (in ep93xx_spi_read_write) or just 'R' if
      only 1 byte is read; !8R if more than 8 reads were done (should never
      happen).
  nW  the n number of fifo writes (in ep93xx_spi_read_write) or just 'W' if
      only 1 byte is written; !8W if more than 8 writes were done (should 
      never happen).
  nw  the n number of fifo writes from the next transfer (in 
      ep93xx_spi_read_write) or just 'w' if only 1 byte is written; !8w if
      more than 8 writes were done (should never happen).  NOTE: this only
      occurs with doing a continuous transaction.
  F   the transfer is complete ((espi->rx == t->len) in ep93xx_spi_read_write)


For the sst25l driver, with continuous transactions disabled, I get this for
the Read-ID command (this is with SPI_DEBUG and MMC_DEBUG enabled):

spi spi0.0: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
ep93xx-spi ep93xx-spi.0: 2 part continuous transaction (6 bytes) for sst25l
ep93xx-spi ep93xx-spi.0: Done: +4WI4RFi-+2WI2RFi-
sst25l spi0.0: sst25lf040a (512 KiB)
Creating 3 MTD partitions on "SPI Flash":
0x000000000000-0x000000001000 : "SPI bootstrap"
0x000000001000-0x000000002000 : "Bootstrap config"
0x000000002000-0x000000080000 : "unallocated"
ep93xx-spi ep93xx-spi.0: registered child spi0.0

With continuous transactions enabled its:

spi spi0.0: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
ep93xx-spi ep93xx-spi.0: 2 part continuous transaction (6 bytes) for sst25l
ep93xx-spi ep93xx-spi.0: Done: 4W2wI4RFC2RFi
sst25l spi0.0: sst25lf040a (512 KiB)
Creating 3 MTD partitions on "SPI Flash":
0x000000000000-0x000000001000 : "SPI bootstrap"
0x000000001000-0x000000002000 : "Bootstrap config"
0x000000002000-0x000000080000 : "unallocated"
ep93xx-spi ep93xx-spi.0: registered child spi0.0

The '+' and '-' are missing since the transfer is started in ep93xx_spi_process_messange
and then handed off to the interrupt routine without first calling ep93xx_spi_process_transfer.
You can see the 2 bytes of the second transfer getting chained to the first 4 bytes
during the priming of the fifo.  Then after the interrupt the first 4 reads finish
the first transaction.  The next transaction is then chained in and also finishes
(2 reads) then the interrupt completes.  Message done!

With the mmc_spi driver the output is WAY to long to post. But, with continuous
transactions disabled the card works fine.  With continuous transactions enabled
it doesn't for some reason.

I have looked over the output a number of times and it appears the continuous transfer
works correctly.  There are 3 that happen when the mmc_spi driver has the spi clock
set to 400000 Hz max, I believe this is while the upper layers are detecting/configuring
the card.  One happens after a CMD10 and is a "mmc_spi: read block, 16 bytes" and is a
16+2 byte message.  This is followed by a CMD9 then another "mmc_spi: read block, 16 bytes"
which is another 16+2 byte message.  Then there is a CMD55 and a CMD51.  The CMD51 does
a 9 byte message then a 1 byte message followed by a "mmc_spi: read block, 8 bytes".
That's followed by 4 separate 1 byte messages then an 8+2 byte message.  All of these
n+m messages appear to work fine as a continuous transfer.

Shortly after this the following is output:

mmc0: new SD card on SPI
mmcblk0: mmc0:0000 S064B 60.6 MiB (ro)
 mmcblk0:

Right after that there is a CMD18.  I think this is where the upper layer is trying to
read the partition table.  This is where it getting interesting compared to the non-
continuous transfer.

mmcblk0: mmc0:0000 S064B 60.6 MiB (ro)
 mmcblk0:
mmc0: starting CMD18 arg 00000000 flags 000000b5
mmc0:     blksz 512 blocks 8 flags 00000200 tsac 100 ms nsac 10000
mmc0:     CMD12 arg 00000000 flags 0000049d
mmc_spi spi0.1:   mmc_spi: CMD18, resp R1
ep93xx-spi ep93xx-spi.0: 1 part transaction (9 bytes) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +8WI8RWiIRFi-
ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
mmc_spi spi0.1:     mmc_spi: read block, 512 bytes
ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
ep93xx-spi ep93xx-spi.0: 3 part continuous transaction (515 bytes) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: 8WI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
                               I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R2wFC2RwFCiIRFi
mmc_spi spi0.1:     mmc_spi: read block, 512 bytes
ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
ep93xx-spi ep93xx-spi.0: 3 part continuous transaction (515 bytes) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: 8WI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
					 I8R2wFC2RwFCiIRFi
mmc_spi spi0.1: read - crc error: crc_val=0x0000, computed=0x8258 len=512
mmc_spi spi0.1: read status -84
mmc_spi spi0.1:   mmc_spi: CMD12, resp R1B
ep93xx-spi ep93xx-spi.0: 1 part transaction (29 bytes) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +8WI8R8WiI8R8WiI8R5WiI5RFi-
mmc_spi spi0.1: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
mmc0: req done (CMD18): 0: 00000000 00000000 00000000 00000000
mmc0:     512 bytes transferred: -84
mmc0:     (CMD12): 0: 00000000 00000000 00000000 00000000
mmcblk0: retrying using single block read

At this point everything just goes nuts.  There are a bunch of CMD17 then CMD13.

mmc0: starting CMD17 arg 00000000 flags 000000b5
mmc0:     blksz 512 blocks 1 flags 00000200 tsac 100 ms nsac 10000
mmc_spi spi0.1:   mmc_spi: CMD17, resp R1
ep93xx-spi ep93xx-spi.0: 1 part transaction (9 bytes) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +8WI8RWiIRFi-
mmc_spi spi0.1:   ... CMD17 response SPI_R1: resp 0004 00000000
mmc_spi spi0.1: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
mmc0: req done (CMD17): -38: 00000004 00000000 00000000 00000000
mmc0:     0 bytes transferred: 0
mmc0: starting CMD13 arg 00000000 flags 00000195
mmc_spi spi0.1:   mmc_spi: CMD13, resp R2/R5
ep93xx-spi ep93xx-spi.0: 1 part transaction (18 bytes) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +8WI8R8WiI8R2WiI2RFi-
mmc_spi spi0.1:   ... CMD13 response SPI_R2/R5: resp 0004 00000000
mmc_spi spi0.1: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
mmc0: req done (CMD13): -38: 00000004 00000000 00000000 00000000
mmcblk0: error -38 sending status comand
mmcblk0: error -38 sending read/write command, response 0x4, card status 0x4
end_request: I/O error, dev mmcblk0, sector 0

The only difference appears the be the arg for CMD17 and the end_request output:

mmc0: starting CMD17 arg 00000000 flags 000000b5
...
end_request: I/O error, dev mmcblk0, sector 0
mmc0: starting CMD17 arg 00000200 flags 000000b5
...
end_request: I/O error, dev mmcblk0, sector 1
mmc0: starting CMD17 arg 00000400 flags 000000b5
...
end_request: I/O error, dev mmcblk0, sector 2
mmc0: starting CMD17 arg 00000600 flags 000000b5
...
end_request: I/O error, dev mmcblk0, sector 3
mmc0: starting CMD17 arg 00000800 flags 000000b5
...
end_request: I/O error, dev mmcblk0, sector 4
mmc0: starting CMD17 arg 00000a00 flags 000000b5
...
end_request: I/O error, dev mmcblk0, sector 5
mmc0: starting CMD17 arg 00000c00 flags 000000b5
...
end_request: I/O error, dev mmcblk0, sector 6
mmc0: starting CMD17 arg 00000e00 flags 000000b5
...
end_request: I/O error, dev mmcblk0, sector 7

Then there is a:

Buffer I/O error on device mmcblk0, logical block 0
mmc0: starting CMD18 arg 00000000 flags 000000b5
mmc0:     blksz 512 blocks 8 flags 00000200 tsac 100 ms nsac 10000
mmc0:     CMD12 arg 00000000 flags 0000049d
mmc_spi spi0.1:   mmc_spi: CMD18, resp R1
ep93xx-spi ep93xx-spi.0: 1 part transaction (9 bytes) for mmc_spi
ep93xx-spi ep93xx-spi.0: Done: +8WI8RWiIRFi-
mmc_spi spi0.1:   ... CMD18 response SPI_R1: resp 0004 00000000
mmc_spi spi0.1: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
mmc0: req done (CMD18): -38: 00000004 00000000 00000000 00000000
mmc0:     0 bytes transferred: 0
mmc0:     (CMD12): 0: 00000000 00000000 00000000 00000000
mmcblk0: retrying using single block read

Followed by all the CMD17 stuff above again.  Then it ends with:

Buffer I/O error on device mmcblk0, logical block 0
 unable to read partition table

I can't see any reason why the continuous transfer would cause a
crc error.  Even if it did I can't see why the single block reads
would all fail, all of then are composed of 1 transfer messages.

Anyone have some suggestions?

Again, sorry for the long message.

Regards,
Hartley

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/24/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
> On Friday, April 23, 2010 10:24 AM, H Hartley Sweeten wrote:
>  > I was able to hack your driver to keep the continuous transfer going in a
>  > multi-transaction message.

>  I modified my hack a bit so that the message transactions are dumped at
>  the end of all transactions in the message.

Yes, you cannot issue printk's while the transfer is active, because
that totally changes the timing.

>  I'm using Mika's hack to determine if the message could be continuous

I've thought about this some more, and the "continuity check" seems to
done in the wrong place. Instead of having the loop_for_all thing at
the top level, then having a race to keep the transfer going inside
the interrupt routine, the structure needs to change so that the
looping construct is initialised at the top level, but steps from one
transfer to the next inside the interrupt routine, the same way as it
is the irw routine that steps from one word to the next of a transfer.
This implies adding the "looping" variable into the struct.

But it gets worse. To react in time, we must get from the half-empty
interrupt being asserted to the point where we refill the TX FIFO
within 4 (maybe 5?) words. 32 bits at 3.7MHz is about 8.5 us. With E2
silicon at 7.4MHz this is 4 us. If we don't react in time, for example
because another interrupt routing is already in progress, the TX
buffer empties, and SFRMOUT goes high in the middle of a transfer.
Does this happen in practice? Well, you can check for it in the IRQ
routine: if a transfer has been started, there is more to send but the
device has gone idle, then we have not reacted quickly enough and
SFRMOUT will have gone high:

static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
{
        struct spi_message *msg = espi->current_msg;
        struct spi_transfer *t = msg->state;

        /* read as long as RX FIFO has frames in it */
        while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)) {
                ep93xx_do_read(espi, t);
                espi->fifo_level--;
        }

        if (espi->tx  >0 && espi->tx < t->len&&
!(ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_BSY)) {
                /* More to transmit but device has gone idle means that
                 * SFRMOUT will have gone high */
                printk("ep93xx-spi: Underrun\n");
        }

        /* write as long as TX FIFO has room */
        while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
                ep93xx_do_write(espi, t);
                espi->fifo_level++;
        }



 then displaying a debug
>  about the message.  By setting the 'continuous' flag to false right after
>  that it disables my hack to make the transaction continuous so the
>  message gets processed in the normal fashion.
>
>  The debug output decodes as:
>   S   a transfer specific setting was made (in ep93xx_spi_process_transfer)
>       NOTE: I have never actually seen this happen.
>   +   start of a transfer (in ep93xx_spi_process_fransfer).
>   -   end of a transfer (in ep93xx_spi_process_fransfer).
>   I   entered ep93xx_spi_interrupt
>   I   exited ep93xx_spi_interrupt
>   C   chained to the next transfer in a continuous transaction
>   nR  the n number of fifo reads (in ep93xx_spi_read_write) or just 'R' if
>       only 1 byte is read; !8R if more than 8 reads were done (should never
>       happen).
>   nW  the n number of fifo writes (in ep93xx_spi_read_write) or just 'W' if
>       only 1 byte is written; !8W if more than 8 writes were done (should
>       never happen).
>   nw  the n number of fifo writes from the next transfer (in
>       ep93xx_spi_read_write) or just 'w' if only 1 byte is written; !8w if
>       more than 8 writes were done (should never happen).  NOTE: this only
>       occurs with doing a continuous transaction.
>   F   the transfer is complete ((espi->rx == t->len) in ep93xx_spi_read_write)
>
>
>  For the sst25l driver, with continuous transactions disabled, I get this for
>  the Read-ID command (this is with SPI_DEBUG and MMC_DEBUG enabled):
>
>  spi spi0.0: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
>  ep93xx-spi ep93xx-spi.0: 2 part continuous transaction (6 bytes) for sst25l
>  ep93xx-spi ep93xx-spi.0: Done: +4WI4RFi-+2WI2RFi-
>  sst25l spi0.0: sst25lf040a (512 KiB)
>  Creating 3 MTD partitions on "SPI Flash":
>  0x000000000000-0x000000001000 : "SPI bootstrap"
>  0x000000001000-0x000000002000 : "Bootstrap config"
>  0x000000002000-0x000000080000 : "unallocated"
>  ep93xx-spi ep93xx-spi.0: registered child spi0.0
>
>  With continuous transactions enabled its:
>
>  spi spi0.0: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
>  ep93xx-spi ep93xx-spi.0: 2 part continuous transaction (6 bytes) for sst25l
>  ep93xx-spi ep93xx-spi.0: Done: 4W2wI4RFC2RFi
>  sst25l spi0.0: sst25lf040a (512 KiB)
>  Creating 3 MTD partitions on "SPI Flash":
>  0x000000000000-0x000000001000 : "SPI bootstrap"
>  0x000000001000-0x000000002000 : "Bootstrap config"
>  0x000000002000-0x000000080000 : "unallocated"
>  ep93xx-spi ep93xx-spi.0: registered child spi0.0
>
>  The '+' and '-' are missing since the transfer is started in ep93xx_spi_process_messange
>  and then handed off to the interrupt routine without first calling ep93xx_spi_process_transfer.
>  You can see the 2 bytes of the second transfer getting chained to the first 4 bytes
>  during the priming of the fifo.  Then after the interrupt the first 4 reads finish
>  the first transaction.  The next transaction is then chained in and also finishes
>  (2 reads) then the interrupt completes.  Message done!
>
>  With the mmc_spi driver the output is WAY to long to post. But, with continuous
>  transactions disabled the card works fine.  With continuous transactions enabled
>  it doesn't for some reason.
>
>  I have looked over the output a number of times and it appears the continuous transfer
>  works correctly.  There are 3 that happen when the mmc_spi driver has the spi clock
>  set to 400000 Hz max, I believe this is while the upper layers are detecting/configuring
>  the card.  One happens after a CMD10 and is a "mmc_spi: read block, 16 bytes" and is a
>  16+2 byte message.  This is followed by a CMD9 then another "mmc_spi: read block, 16 bytes"
>  which is another 16+2 byte message.  Then there is a CMD55 and a CMD51.  The CMD51 does
>  a 9 byte message then a 1 byte message followed by a "mmc_spi: read block, 8 bytes".
>  That's followed by 4 separate 1 byte messages then an 8+2 byte message.  All of these
>  n+m messages appear to work fine as a continuous transfer.
>
>  Shortly after this the following is output:
>
>
>  mmc0: new SD card on SPI
>
> mmcblk0: mmc0:0000 S064B 60.6 MiB (ro)
>   mmcblk0:
>
>  Right after that there is a CMD18.  I think this is where the upper layer is trying to
>  read the partition table.  This is where it getting interesting compared to the non-
>  continuous transfer.
>
>  mmcblk0: mmc0:0000 S064B 60.6 MiB (ro)
>   mmcblk0:
>  mmc0: starting CMD18 arg 00000000 flags 000000b5
>  mmc0:     blksz 512 blocks 8 flags 00000200 tsac 100 ms nsac 10000
>  mmc0:     CMD12 arg 00000000 flags 0000049d
>  mmc_spi spi0.1:   mmc_spi: CMD18, resp R1
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (9 bytes) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +8WI8RWiIRFi-
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
>  mmc_spi spi0.1:     mmc_spi: read block, 512 bytes
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
>  ep93xx-spi ep93xx-spi.0: 3 part continuous transaction (515 bytes) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: 8WI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R2wFC2RwFCiIRFi
>  mmc_spi spi0.1:     mmc_spi: read block, 512 bytes
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (1 byte) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +WIRFi-
>  ep93xx-spi ep93xx-spi.0: 3 part continuous transaction (515 bytes) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: 8WI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8WiI8R8Wi
>                                          I8R2wFC2RwFCiIRFi
>  mmc_spi spi0.1: read - crc error: crc_val=0x0000, computed=0x8258 len=512
>  mmc_spi spi0.1: read status -84
>  mmc_spi spi0.1:   mmc_spi: CMD12, resp R1B
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (29 bytes) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +8WI8R8WiI8R8WiI8R5WiI5RFi-
>  mmc_spi spi0.1: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
>  mmc0: req done (CMD18): 0: 00000000 00000000 00000000 00000000
>  mmc0:     512 bytes transferred: -84
>  mmc0:     (CMD12): 0: 00000000 00000000 00000000 00000000
>
> mmcblk0: retrying using single block read
>
>
> At this point everything just goes nuts.  There are a bunch of CMD17 then CMD13.
>
>  mmc0: starting CMD17 arg 00000000 flags 000000b5
>  mmc0:     blksz 512 blocks 1 flags 00000200 tsac 100 ms nsac 10000
>  mmc_spi spi0.1:   mmc_spi: CMD17, resp R1
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (9 bytes) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +8WI8RWiIRFi-
>  mmc_spi spi0.1:   ... CMD17 response SPI_R1: resp 0004 00000000
>  mmc_spi spi0.1: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
>  mmc0: req done (CMD17): -38: 00000004 00000000 00000000 00000000
>  mmc0:     0 bytes transferred: 0
>  mmc0: starting CMD13 arg 00000000 flags 00000195
>  mmc_spi spi0.1:   mmc_spi: CMD13, resp R2/R5
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (18 bytes) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +8WI8R8WiI8R2WiI2RFi-
>  mmc_spi spi0.1:   ... CMD13 response SPI_R2/R5: resp 0004 00000000
>  mmc_spi spi0.1: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
>  mmc0: req done (CMD13): -38: 00000004 00000000 00000000 00000000
>
> mmcblk0: error -38 sending status comand
>
> mmcblk0: error -38 sending read/write command, response 0x4, card status 0x4
>  end_request: I/O error, dev mmcblk0, sector 0
>
>  The only difference appears the be the arg for CMD17 and the end_request output:
>
>  mmc0: starting CMD17 arg 00000000 flags 000000b5
>  ...
>  end_request: I/O error, dev mmcblk0, sector 0
>  mmc0: starting CMD17 arg 00000200 flags 000000b5
>  ...
>  end_request: I/O error, dev mmcblk0, sector 1
>  mmc0: starting CMD17 arg 00000400 flags 000000b5
>  ...
>  end_request: I/O error, dev mmcblk0, sector 2
>  mmc0: starting CMD17 arg 00000600 flags 000000b5
>  ...
>  end_request: I/O error, dev mmcblk0, sector 3
>  mmc0: starting CMD17 arg 00000800 flags 000000b5
>  ...
>  end_request: I/O error, dev mmcblk0, sector 4
>  mmc0: starting CMD17 arg 00000a00 flags 000000b5
>  ...
>  end_request: I/O error, dev mmcblk0, sector 5
>  mmc0: starting CMD17 arg 00000c00 flags 000000b5
>  ...
>  end_request: I/O error, dev mmcblk0, sector 6
>  mmc0: starting CMD17 arg 00000e00 flags 000000b5
>  ...
>
> end_request: I/O error, dev mmcblk0, sector 7
>
>
> Then there is a:
>
>  Buffer I/O error on device mmcblk0, logical block 0
>  mmc0: starting CMD18 arg 00000000 flags 000000b5
>  mmc0:     blksz 512 blocks 8 flags 00000200 tsac 100 ms nsac 10000
>  mmc0:     CMD12 arg 00000000 flags 0000049d
>  mmc_spi spi0.1:   mmc_spi: CMD18, resp R1
>  ep93xx-spi ep93xx-spi.0: 1 part transaction (9 bytes) for mmc_spi
>  ep93xx-spi ep93xx-spi.0: Done: +8WI8RWiIRFi-
>  mmc_spi spi0.1:   ... CMD18 response SPI_R1: resp 0004 00000000
>  mmc_spi spi0.1: setup mode 3, 8 bits/w, 20000000 Hz max --> 0
>  mmc0: req done (CMD18): -38: 00000004 00000000 00000000 00000000
>  mmc0:     0 bytes transferred: 0
>  mmc0:     (CMD12): 0: 00000000 00000000 00000000 00000000
>
> mmcblk0: retrying using single block read
>
>
> Followed by all the CMD17 stuff above again.  Then it ends with:
>
>  Buffer I/O error on device mmcblk0, logical block 0
>   unable to read partition table
>
>  I can't see any reason why the continuous transfer would cause a
>  crc error.  Even if it did I can't see why the single block reads
>  would all fail, all of then are composed of 1 transfer messages.
>
>  Anyone have some suggestions?
>
>  Again, sorry for the long message.
>
>  Regards,
>
> Hartley
>

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

Sorry about the incomplete message. Finger trouble.

On 4/25/10, Martin Guy <martinwguy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
...
>  SFRMOUT will have gone high:

>         if (espi->tx  >0 && espi->tx < t->len
               && !(ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_BSY)) {
>                 /* More to transmit but device has gone idle means that
>                  * SFRMOUT will have gone high */
>                 printk("ep93xx-spi: Underrun\n");
>         }

I've also done a version that doesn't printk() in the middle of it,
which affects timing, but counts the underruns and reports at end of
transfer. The result is that every 512-byte block underruns at least
once, sometimes twice.

I'll run a few more tests, e.g. dropping the clock rate, but at this
point I'd be inclined to declare it impossible to keep SFRMOUT low
during a transfer, and just use the simple non-continuous code with a
comment at the top of the file that explains why you can't use SFRMOUT
as a chip select for devices that require CS to remain asserted for
the duration of a transfer.

    M

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Sunday, April 25, 2010 2:39 AM, Martin Guy wrote:
>
> Sorry about the incomplete message. Finger trouble.
>
> On 4/25/10, Martin Guy <martinwguy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> ...
>>  SFRMOUT will have gone high:
>
>>         if (espi->tx  >0 && espi->tx < t->len
>>               && !(ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_BSY)) {
>>                 /* More to transmit but device has gone idle means that
>>                  * SFRMOUT will have gone high */
>>                 printk("ep93xx-spi: Underrun\n");
>>         }
>
> I've also done a version that doesn't printk() in the middle of it,
> which affects timing, but counts the underruns and reports at end of
> transfer. The result is that every 512-byte block underruns at least
> once, sometimes twice.

I haven't actually looked for the underrun by checking the SSPSR_BSY
bit but my guess is actually underruns a lot more than that.  Depends
on where you are doing the check above on if you actually see it.

During the 512+2+1 message that is sent when the mmc_spi driver is
doing a block read, the 512 byte transfer goes like this:

1) 8 writes, prime the tx fifo
	SFRMOUT asserts when the first bit of the first byte starts 
	transmitting
2) Interrupt
3) 8 reads, we must have emptied the tx fifo and filled the rx fifo
	SFRMOUT de-asserts after the the last bit of the last byte is
	received
4) 8 writes, fifo's are empty so we re-fill them
	SFRMOUT asserts when the first bit of the first byte starts 
	transmitting
5) end of interupt
6) go back to 2

Basically, anytime the espi->fifo_level == 8 and we can do an 8 byte
read you have to assume that the SFRMOUT signal has already been de-
asserted.

This matches what I am seeing with the logic analyzer.

I have modifed my board so that I now have an external GPIO for the chip
select.  That chip select goes low then shortly after, the SFRMOUT signal
goes low and 8 bytes are pumped out then SFRMOUT goes back high.  A bit 
later the interrupt happens, indicated by another GPIO I toggle in the
interrupt routine.  Right before the interrupt ends I again see SFRMOUT
go low and 8 more bytes are pumped out.  The interrupt actually completes
before the data is all pumped out.  But again SFRMOUT gets de-asserted
before the next interrupt occurs.  This keeps happening until all the
data in the message is finally transferred, at which point the external
GPIO chip select is finally de-asserted.

> I'll run a few more tests, e.g. dropping the clock rate, but at this
> point I'd be inclined to declare it impossible to keep SFRMOUT low
> during a transfer, and just use the simple non-continuous code with a
> comment at the top of the file that explains why you can't use SFRMOUT
> as a chip select for devices that require CS to remain asserted for
> the duration of a transfer.

With a slow enough clock you can probably get to a point where SFRMOUT
will stay deasserted during the entire 512 byte transfer.  But it would
still de-assert during the switch to the next transfer in the message.

Regardless, I tend to agree with you.  Because of the chip design, it's
impossible (or at least you can't guarantee) to keep the SFRMOUT low
during a transfer.  Some devices can live with this others can't.  Oh well.

A Documentation/spi/ep93xx_spi.txt really should be created to note this
limitation as well as showing an example of the platform support needed to
use this driver.

Regards,
Hartley
------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

Hi,

I just answer to this last message as it seems that continuous
transfer is really not a 100% solid solution; It makes things more
complicated than need to be and benefit for that is probably not
worth the additional complexity. Feel free to disagree :)

On Sun, Apr 25, 2010 at 03:25:16PM -0500, H Hartley Sweeten wrote:
> On Sunday, April 25, 2010 2:39 AM, Martin Guy wrote:
> > I'll run a few more tests, e.g. dropping the clock rate, but at this
> > point I'd be inclined to declare it impossible to keep SFRMOUT low
> > during a transfer, and just use the simple non-continuous code with a
> > comment at the top of the file that explains why you can't use SFRMOUT
> > as a chip select for devices that require CS to remain asserted for
> > the duration of a transfer.
> 
> With a slow enough clock you can probably get to a point where SFRMOUT
> will stay deasserted during the entire 512 byte transfer.  But it would
> still de-assert during the switch to the next transfer in the message.
> 
> Regardless, I tend to agree with you.  Because of the chip design, it's
> impossible (or at least you can't guarantee) to keep the SFRMOUT low
> during a transfer.  Some devices can live with this others can't.  Oh well.

For the MMC over SPI at least we really cannot ever make fully
compliant solution with SFRMOUT because, during init phase protocol
sends CMD0 (reset) with chipselect active high (SPI_CS_HIGH). This
forces SD/MMC card to SPI mode. However, many cards seem to work
without any chipselect logic at all (with Sim.One for example).

> A Documentation/spi/ep93xx_spi.txt really should be created to note this
> limitation as well as showing an example of the platform support needed to
> use this driver.

Do we all agree that we can live with GPIOs as chipselect? Or do
we continue investigation to make it work with boards that use
SFRMOUT as CS?

I have following plans for v5 (in case we don't care about SFRMOUT).
Before doing the changes I would like to hear your opinnion.

	- prime the FIFO first in every transfer (Hartley's suggestion)
	- address review comments for v4
	- add Martin's patch for clearing SSPICR only if we get ROR
	- add necessary documentation under Documentation/spi/ep93xx_spi.txt

Thanks,
MW

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Monday, April 26, 2010 3:03 AM, Mika Westerberg wrote:
>
> Hi,
>
> I just answer to this last message as it seems that continuous
> transfer is really not a 100% solid solution; It makes things more
> complicated than need to be and benefit for that is probably not
> worth the additional complexity. Feel free to disagree :)

I agree.  Trying to make the transfer continuous is not solid and
does complicate the code.  Drop it.

> On Sun, Apr 25, 2010 at 03:25:16PM -0500, H Hartley Sweeten wrote:
>> On Sunday, April 25, 2010 2:39 AM, Martin Guy wrote:
>>> I'll run a few more tests, e.g. dropping the clock rate, but at this
>>> point I'd be inclined to declare it impossible to keep SFRMOUT low
>>> during a transfer, and just use the simple non-continuous code with a
>>> comment at the top of the file that explains why you can't use SFRMOUT
>>> as a chip select for devices that require CS to remain asserted for
>>> the duration of a transfer.
>> 
>> With a slow enough clock you can probably get to a point where SFRMOUT
>> will stay deasserted during the entire 512 byte transfer.  But it would
>> still de-assert during the switch to the next transfer in the message.
>> 
>> Regardless, I tend to agree with you.  Because of the chip design, it's
>> impossible (or at least you can't guarantee) to keep the SFRMOUT low
>> during a transfer.  Some devices can live with this others can't.  Oh well.
>
> For the MMC over SPI at least we really cannot ever make fully
> compliant solution with SFRMOUT because, during init phase protocol
> sends CMD0 (reset) with chipselect active high (SPI_CS_HIGH). This
> forces SD/MMC card to SPI mode. However, many cards seem to work
> without any chipselect logic at all (with Sim.One for example).

Good point.  Any driver that requests SPI_CS_HIGH will be "broken" by using
the SFRMOUT signal for the chip select.  I think the Sim.One board test
have just been lucky with the mmc/sc-cards.  There are probably many cards
that will not work correctly on that platform.

> A Documentation/spi/ep93xx_spi.txt really should be created to note this
> limitation as well as showing an example of the platform support needed to
> use this driver.

> Do we all agree that we can live with GPIOs as chipselect? Or do
> we continue investigation to make it work with boards that use
> SFRMOUT as CS?

I think we can live with the GPIOs for chip selects.

> I have following plans for v5 (in case we don't care about SFRMOUT).
> Before doing the changes I would like to hear your opinnion.
> 
> 	- prime the FIFO first in every transfer (Hartley's suggestion)

I think this helps regardless.  I makes sure the hardware is actually
doing something when a transfer starts instead of waiting for the first
interrupt to get things going.

> 	- address review comments for v4
> 	- add Martin's patch for clearing SSPICR only if we get ROR

Moving the interrupt clear into the unlikely() path probably makes sense.
It keeps the code from having to do the unnecessary memory write every time.

Also, is the IRQ_NONE check before that really necessary?  The IRQ_EP93XX_SSP
is not a chained interrupt so if ep93xx_spi_interrupt gets called I would
assume the interrupt must be valid.

> 	- add necessary documentation under Documentation/spi/ep93xx_spi.txt

Sounds good.

Once you post the v5 version I will test it then send you a patch for allowing
non-built-in GPIOs to be used for chip selects.

Regards,
Hartley

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Sun, Apr 25, 2010 at 03:25:16PM -0500, H Hartley Sweeten wrote:
> With a slow enough clock you can probably get to a point where SFRMOUT
> will stay deasserted during the entire 512 byte transfer.  But it would
> still de-assert during the switch to the next transfer in the message.
> 
> Regardless, I tend to agree with you.  Because of the chip design, it's
> impossible (or at least you can't guarantee) to keep the SFRMOUT low
> during a transfer.  Some devices can live with this others can't.  Oh well.

One more thing I forgot to mention. With DMA there are 2 buffers per channel which
means that the controller can continue right to the next transfer so SFRMOUT might
work better.

Regards,
MW

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/25/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>  During the 512+2+1 message that is sent when the mmc_spi driver is
>  doing a block read, the 512 byte transfer goes like this:
>
>  1) 8 writes, prime the tx fifo
>         SFRMOUT asserts when the first bit of the first byte starts
>         transmitting
>  2) Interrupt
>  3) 8 reads, we must have emptied the tx fifo and filled the rx fifo
>         SFRMOUT de-asserts after the the last bit of the last byte is
>         received

Are you printk-ing the diagnostics in the irq routine?
If so, that slows it down enough to ensure every 8-word block underruns.

Here are stats for 512-word transfers from SD card at 3.7 MHz; figures are
number of underruns/total number of reads and the distribution of
words read per interrupt [0..8], collected during the transfer and
printed once the transfer is over)

1/102 [1,0,2,0,0,100,0,0,1]
1/102 [1,0,2,0,0,100,0,0,1]
1/102 [1,0,2,0,0,100,0,0,1]
2/104 [1,1,0,2,21,76,3,1,2]
3/102 [1,1,1,1,11,85,1,1,3]
4/100 [1,0,1,1,7,83,3,2,4]

while 1- and 2-word transfers seem to be caught on the second interrupt.
[1,2,0,0,0,0,0,0,0]
[1,1,0,0,0,0,0,0,0]


>  I have modifed my board so that I now have an external GPIO for the chip
>  select.
Yes, we'll have to do that too.

>  > I'll run a few more tests, e.g. dropping the clock rate
>
> With a slow enough clock you can probably get to a point where SFRMOUT
>  will stay deasserted during the entire 512 byte transfer

I've tried it down to 400kHz and transfers still underrun (!)

So it does seem that SFRMOUT cannot be used as a chip select unless we
revert to the old busy-wait-for-whole-transfer code, which causes 100%
CPU usage during transfers.
and 1ms-long pauses (for 512 words at 3.7MHz).

This suggests that we can only handle SFRMOUT used as chip select
correctly by reinstating polling mode, adding the continuous transfer
code and allowing the board definition code to specify that polling
mode be used for such devices.
(much as I hate polling mode :)

>  But it would
>  still de-assert during the switch to the next transfer in the message.

Am I right in thinking that the GPIO chip select currently remains
asserted as it should during a multi-transfer message?

    M

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Monday, April 26, 2010 7:36 AM, Martin Guy wrote:
> On 4/25/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>>  During the 512+2+1 message that is sent when the mmc_spi driver is
>>  doing a block read, the 512 byte transfer goes like this:
>>
>>  1) 8 writes, prime the tx fifo
>>         SFRMOUT asserts when the first bit of the first byte starts
>>         transmitting
>>  2) Interrupt
>>  3) 8 reads, we must have emptied the tx fifo and filled the rx fifo
>>         SFRMOUT de-asserts after the the last bit of the last byte is
>>         received
>
> Are you printk-ing the diagnostics in the irq routine?
> If so, that slows it down enough to ensure every 8-word block underruns.

No, all my printk's happen outside the interrupt context.

> Here are stats for 512-word transfers from SD card at 3.7 MHz; figures are
> number of underruns/total number of reads and the distribution of
> words read per interrupt [0..8], collected during the transfer and
> printed once the transfer is over)
> 
> 1/102 [1,0,2,0,0,100,0,0,1]
> 1/102 [1,0,2,0,0,100,0,0,1]
> 1/102 [1,0,2,0,0,100,0,0,1]
> 2/104 [1,1,0,2,21,76,3,1,2]
> 3/102 [1,1,1,1,11,85,1,1,3]
> 4/100 [1,0,1,1,7,83,3,2,4]
> 
> while 1- and 2-word transfers seem to be caught on the second interrupt.
> [1,2,0,0,0,0,0,0,0]
> [1,1,0,0,0,0,0,0,0]

External stuff could be causing the differences.  In my testing, I only have
the mmc_spi driver running since I was trying to capture it on the logic
analyzer.  Ethernet, etc. are all shutdown.

>> I have modifed my board so that I now have an external GPIO for the chip
>> select.
> Yes, we'll have to do that too.

>>> I'll run a few more tests, e.g. dropping the clock rate
>>
>> With a slow enough clock you can probably get to a point where SFRMOUT
>>  will stay deasserted during the entire 512 byte transfer
>
> I've tried it down to 400kHz and transfers still underrun (!)
>
> So it does seem that SFRMOUT cannot be used as a chip select unless we
> revert to the old busy-wait-for-whole-transfer code, which causes 100%
> CPU usage during transfers.
> and 1ms-long pauses (for 512 words at 3.7MHz).
>
> This suggests that we can only handle SFRMOUT used as chip select
> correctly by reinstating polling mode, adding the continuous transfer
> code and allowing the board definition code to specify that polling
> mode be used for such devices.
> (much as I hate polling mode :)

I don't think we should worry about polling mode for now.  With the
external GPIO chip select we don't have to worry about what the SFRMOUT
signal is doing.

>>  But it would
>  still de-assert during the switch to the next transfer in the message.
>
> Am I right in thinking that the GPIO chip select currently remains
> asserted as it should during a multi-transfer message?

Yes.  It gets asserted at the start and de-asserted at the end of
ep93xx_spi_process_message.

Actually, I think your supposed to check the cs_change flag at the end
of the message to see if the protocol is hinting to leave the chip
select asserted for the next message.  Of course this also means you need
to verify that the next message is actually for the same device.

Regards,
Hartley
------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Tue, Apr 20, 2010 at 05:16:10PM -0500, H Hartley Sweeten wrote:
> On Tuesday, April 20, 2010 8:12 AM, Mika Westerberg wrote:
> >
> > This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
> > in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> 
> Mika,
> 
> I discovered one gotcha with this driver.  Bear with me through this...
> 
> 1. ep93xx_spi_process_message is called by the workqueue worker function to handle
>    a spi_message queued in by the ep93xx_spi_transfer function.  Before starting
>    the actual transfer the chip select to the device is asserted.
> 2. ep93xx_spi_process_transfer processes each transfer in the spi_message, one at a
>    time, and does a wait_for_completion on each transfer.
> 3. When the entire spi_message has been transferred the chip select to the device
>    is deasserted.

Yes. There is possibility that last transfer ->cs_change is set which can be used
by SPI controller drivers to keep the chip selected in case next message is going
to the same device.

However, I haven't implemented that as it is optional (but trivial to add afterwards).

> The problem is if a hardware design uses the SFRM1 pin as part of the chip select
> logic.  The EP93xx User's Guide states that "the SFRMOUT signal behaves as a slave
> Select" in Motorola SPI mode.  This signal behaves differently depending on the
> SPI_MODE_* used.
> 
> Modes 0 and 2:
>   SFRMOUT is forced HIGH during idle periods.
>   Start of transmission, SFRMOUT is driven LOW.
>   Single word transmission, SFRMOUT is returned to its idle HIGH state one
>     SCLKOUT period after the last bit has been captured.
>   Back-to-back transmissions, SFRMOUT signal is pulsed HIGH between each
>     data word transfer.  On completion the SFRMOUT pin returns to its idle
>     HIGH state one SCLKOUT period after the last bit has been captured.
> 
> Modes 1 and 3:
>   SFRMOUT is forced HIGH during idle periods.
>   Start of transmission, SFRMOUT is driven LOW.
>   Single word transmission, SFRMOUT is returned to its idle HIGH state one
>     SCLKOUT period after the last bit has been captured.
>   Back-to-back transmissions, SFRMOUT signal is held LOW between each
>     data word transfer.  On completion the SFRMOUT pin returns to its idle
>     HIGH state one SCLKOUT period after the last bit has been captured.
> 
> So, since each transfer does a wait_for_completion, all the data is transmitted
> which causes the SFRMOUT pin to go HIGH between each transfer in the message.

Yes, unfortunately. SFRMOUT cannot be software controlled so it is
not suitable for general purpose chipselect. This was the sole
purpose of using GPIO lines for chipselects.

> For devices like the sst25lf040a SPI Flash on the EDB93xx boards this causes a
> problem.  To read data from that device you need to send a two part message.  The
> first part is a write transfer with the one byte command and three byte address.
> This is then followed by a read transfer to get the data.  But since the SFRMOUT
> pin goes high this ends the command.
> 
> The driver Ryan and I worked on actually does work with the sst25lf040a on the
> EDB93xx boards.  But now that I know what the issue is, it was actually a race
> condition.  Sometimes it worked and sometimes it didn't...
> 
> I would think the Sim.One board (Martin?) would have the same issue with the mmc
> card since that design uses the SFRMOUT pin directly for the chip select.
> 
> Is there anyway to keep the transfers going in a muiltipart message?

I'm not exactly sure but "polling" mode sounds something that could
work around that. It just need to be sure that it transmits
continuously to keep the SFRMOUT asserted.

You could try v3 of the driver with:
	# modprobe ep93xx-spi transfer_method=0

and see if it works there.

An alternative would be that interrupt handler schedules tasklet which then
takes next transfer immediately once current transfer is finished.

Thanks,
MW

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Tuesday, April 20, 2010 11:37 PM, Mika Westerberg wrote:
> On Tue, Apr 20, 2010 at 05:16:10PM -0500, H Hartley Sweeten wrote:
>> On Tuesday, April 20, 2010 8:12 AM, Mika Westerberg wrote:
>>>
>>> This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
>>> in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).
>>> 
>>> Signed-off-by: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>
>> 
>> Mika,
>> 
>> I discovered one gotcha with this driver.  Bear with me through this...
>> 
>> 1. ep93xx_spi_process_message is called by the workqueue worker function to handle
>>    a spi_message queued in by the ep93xx_spi_transfer function.  Before starting
>>    the actual transfer the chip select to the device is asserted.
>> 2. ep93xx_spi_process_transfer processes each transfer in the spi_message, one at a
>>    time, and does a wait_for_completion on each transfer.
>> 3. When the entire spi_message has been transferred the chip select to the device
>>    is deasserted.
>
> Yes. There is possibility that last transfer ->cs_change is set which can be used
> by SPI controller drivers to keep the chip selected in case next message is going
> to the same device.
>
> However, I haven't implemented that as it is optional (but trivial to add afterwards).

It should be implemented.  The mmc_spi driver basically requires this.  Refer to this
comment in the mmc_spi driver:

 * - We tell the controller to keep the chipselect active from the
 *   beginning of an mmc_host_ops.request until the end.  So beware
 *   of SPI controller drivers that mis-handle the cs_change flag!
 *
 *   However, many cards seem OK with chipselect flapping up/down
 *   during that time ... at least on unshared bus segments.

>> The problem is if a hardware design uses the SFRM1 pin as part of the chip select
>> logic.  The EP93xx User's Guide states that "the SFRMOUT signal behaves as a slave
>> Select" in Motorola SPI mode.  This signal behaves differently depending on the
>> SPI_MODE_* used.
>> 
>> Modes 0 and 2:
>>   SFRMOUT is forced HIGH during idle periods.
>>   Start of transmission, SFRMOUT is driven LOW.
>>   Single word transmission, SFRMOUT is returned to its idle HIGH state one
>>     SCLKOUT period after the last bit has been captured.
>>   Back-to-back transmissions, SFRMOUT signal is pulsed HIGH between each
>>     data word transfer.  On completion the SFRMOUT pin returns to its idle
>>     HIGH state one SCLKOUT period after the last bit has been captured.
>> 
>> Modes 1 and 3:
>>   SFRMOUT is forced HIGH during idle periods.
>>   Start of transmission, SFRMOUT is driven LOW.
>>   Single word transmission, SFRMOUT is returned to its idle HIGH state one
>>     SCLKOUT period after the last bit has been captured.
>>   Back-to-back transmissions, SFRMOUT signal is held LOW between each
>>     data word transfer.  On completion the SFRMOUT pin returns to its idle
>>     HIGH state one SCLKOUT period after the last bit has been captured.
>> 
>> So, since each transfer does a wait_for_completion, all the data is transmitted
>> which causes the SFRMOUT pin to go HIGH between each transfer in the message.
>
> Yes, unfortunately. SFRMOUT cannot be software controlled so it is
> not suitable for general purpose chipselect. This was the sole
> purpose of using GPIO lines for chipselects.

I agree.  But existing ep93xx designs, the EDB93xx boards and Sim.One for
instance, use the SFRMOUT either directly for the chip select or as part of
the logic generating the chip select.  If possible it would be nice to handle
this.

>> For devices like the sst25lf040a SPI Flash on the EDB93xx boards this causes a
>> problem.  To read data from that device you need to send a two part message.  The
>> first part is a write transfer with the one byte command and three byte address.
>> This is then followed by a read transfer to get the data.  But since the SFRMOUT
>> pin goes high this ends the command.>
>> 
>> The driver Ryan and I worked on actually does work with the sst25lf040a on the
>> EDB93xx boards.  But now that I know what the issue is, it was actually a race
>> condition.  Sometimes it worked and sometimes it didn't...
>> 
>> I would think the Sim.One board (Martin?) would have the same issue with the mmc
>> card since that design uses the SFRMOUT pin directly for the chip select.
>> 
>> Is there anyway to keep the transfers going in a muiltipart message?
>
> I'm not exactly sure but "polling" mode sounds something that could
> work around that. It just need to be sure that it transmits
> continuously to keep the SFRMOUT asserted.
>
> You could try v3 of the driver with:
>	# modprobe ep93xx-spi transfer_method=0
>
> and see if it works there.

I'll try to test this later.  The driver Ryan and I had used polling and it
worked but I think there was a race condition in it which occasionally broke
the transfer.

> An alternative would be that interrupt handler schedules tasklet which then
> takes next transfer immediately once current transfer is finished.

I think you would need to:

1. detect when all the transmit data in a transfer has been sent to the fifo
2. if there is another transfer pending, and a cs_change has not been requested,
   get it and start pumping it's transmit data into the fifo
3. read the remaining data for the first transfer then complete it
4. the pending transfer, which is already started, becomes the current transfer

Does this sound reasonable?

Regards,
Hartley


------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

Hi, another little fix:

EP93xx User's Manual -> Synchronous Serial Port -> Registers

SSPIIR description:
"Read Only
Note: A write to this register clears the receive overrun interrupt,
regardless of the data
      value written."

It doesn't affect the RIS/TIS ones, which are caused by the state of the device.

so

static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
{
...
        if (!(irq_status & (SSPIIR_RORIS | SSPIIR_TIS | SSPIIR_RIS)))
                return IRQ_NONE; /* not for us */

-       /* clear the interrupt */
-       ep93xx_spi_write_u8(espi, SSPICR, 0);

        /*
         * If we got ROR (receive overrun) interrupt we know that something is
         * wrong. Just abort the message.
         */
        if (unlikely(irq_status & SSPIIR_RORIS)) {
+               /* clear the overrun interrupt */
+               ep93xx_spi_write_u8(espi, SSPICR, 0);
                dev_warn(&espi->pdev->dev,
                         "receive overrun, aborting the message\n");
                espi->current_msg->status = -EIO;
        } else {

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Saturday, April 24, 2010 11:15 AM, Martin Guy wrote:
> Hi, another little fix:
> 
> EP93xx User's Manual -> Synchronous Serial Port -> Registers
> 
> SSPIIR description:
> "Read Only
> Note: A write to this register clears the receive overrun interrupt,
> regardless of the data
>       value written."
> 
> It doesn't affect the RIS/TIS ones, which are caused by the state of the device.
> 
> so
> 
> static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
> {
> ...
>         if (!(irq_status & (SSPIIR_RORIS | SSPIIR_TIS | SSPIIR_RIS)))
>                 return IRQ_NONE; /* not for us */
> 
> -       /* clear the interrupt */
> -       ep93xx_spi_write_u8(espi, SSPICR, 0);
> 
>         /*
>          * If we got ROR (receive overrun) interrupt we know that something is
>          * wrong. Just abort the message.
>          */
>         if (unlikely(irq_status & SSPIIR_RORIS)) {
> +               /* clear the overrun interrupt */
> +               ep93xx_spi_write_u8(espi, SSPICR, 0);
>                 dev_warn(&espi->pdev->dev,
>                          "receive overrun, aborting the message\n");
>                 espi->current_msg->status = -EIO;
>         } else {

With this patch the interrupts only get cleared if it's a ROR interrupt.

Do the RIS/TIS interrupts get cleared by the upper layers?  If so, do we
really need to clear the ROR interrupt here?

Regards,
Hartley
------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Sun, Apr 25, 2010 at 02:55:08PM -0500, H Hartley Sweeten wrote:
> On Saturday, April 24, 2010 11:15 AM, Martin Guy wrote:
> > static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
> > {
> > ...
> >         if (!(irq_status & (SSPIIR_RORIS | SSPIIR_TIS | SSPIIR_RIS)))
> >                 return IRQ_NONE; /* not for us */
> > 
> > -       /* clear the interrupt */
> > -       ep93xx_spi_write_u8(espi, SSPICR, 0);
> > 
> >         /*
> >          * If we got ROR (receive overrun) interrupt we know that something is
> >          * wrong. Just abort the message.
> >          */
> >         if (unlikely(irq_status & SSPIIR_RORIS)) {
> > +               /* clear the overrun interrupt */
> > +               ep93xx_spi_write_u8(espi, SSPICR, 0);
> >                 dev_warn(&espi->pdev->dev,
> >                          "receive overrun, aborting the message\n");
> >                 espi->current_msg->status = -EIO;
> >         } else {
> 
> With this patch the interrupts only get cleared if it's a ROR interrupt.
> 
> Do the RIS/TIS interrupts get cleared by the upper layers?  If so, do we
> really need to clear the ROR interrupt here?

I believe it is not necessary but doesn't hurt either so I would keep it.

Regards,
MW

------------------------------------------------------------------------------

Re: [PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/25/10, H Hartley Sweeten <hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org> wrote:
>  > -       /* clear the interrupt */
>  > -       ep93xx_spi_write_u8(espi, SSPICR, 0);
>  >
>  >         /*
>  >          * If we got ROR (receive overrun) interrupt we know that something is
>  >          * wrong. Just abort the message.
>  >          */
>  >         if (unlikely(irq_status & SSPIIR_RORIS)) {
>  > +               /* clear the overrun interrupt */
>  > +               ep93xx_spi_write_u8(espi, SSPICR, 0);
>  >                 dev_warn(&espi->pdev->dev,
>  >                          "receive overrun, aborting the message\n");
>  >                 espi->current_msg->status = -EIO;
>  >         } else {
>
> With this patch the interrupts only get cleared if it's a ROR interrupt.
>
>  Do the RIS/TIS interrupts get cleared by the upper layers?  If so, do we
>  really need to clear the ROR interrupt here?

RX/TX interrupts are cleared by emptying the RX FIFO or filling the TX
FIFO to less/more than half full. There is no explicit clearing of the
IRQ.

The ROR interrupt instead remains active until you explicitly clear it
by writing (anything( to SSPICR, so it would remain on forever
otherwise.

    M

------------------------------------------------------------------------------

[PATCH v4 2/2] ep93xx: SPI driver platform support code

[Mika Westerberg]

This patch adds platform side support code for EP93xx SPI driver. This includes
clock, resources and muxing. There is a new function: ep93xx_register_spi() that
can be used by board support code to register new SPI devices for the board.

This patch depends on following ARM patch:
	5998/1 ep93xx: added chip revision reading function

Signed-off-by: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>
---
 arch/arm/mach-ep93xx/clock.c                    |   14 +++++++
 arch/arm/mach-ep93xx/core.c                     |   48 +++++++++++++++++++++++
 arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h |    1 +
 arch/arm/mach-ep93xx/include/mach/platform.h    |    2 +
 4 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index 5f80092..5986fae 100644
--- a/arch/arm/mach-ep93xx/clock.c
+++ b/arch/arm/mach-ep93xx/clock.c
@@ -96,6 +96,10 @@ static struct clk clk_keypad = {
 	.enable_mask	= EP93XX_SYSCON_KEYTCHCLKDIV_KEN,
 	.set_rate	= set_keytchclk_rate,
 };
+static struct clk clk_spi = {
+	.parent		= &clk_xtali,
+	.rate		= EP93XX_EXT_CLK_RATE,
+};
 static struct clk clk_pwm = {
 	.parent		= &clk_xtali,
 	.rate		= EP93XX_EXT_CLK_RATE,
@@ -186,6 +190,7 @@ static struct clk_lookup clocks[] = {
 	INIT_CK("ep93xx-ohci",		NULL,		&clk_usb_host),
 	INIT_CK("ep93xx-keypad",	NULL,		&clk_keypad),
 	INIT_CK("ep93xx-fb",		NULL,		&clk_video),
+	INIT_CK("ep93xx-spi",		NULL,		&clk_spi),
 	INIT_CK(NULL,			"pwm_clk",	&clk_pwm),
 	INIT_CK(NULL,			"m2p0",		&clk_m2p0),
 	INIT_CK(NULL,			"m2p1",		&clk_m2p1),
@@ -473,6 +478,14 @@ static int __init ep93xx_clock_init(void)
 	/* Initialize the pll2 derived clocks */
 	clk_usb_host.rate = clk_pll2.rate / (((value >> 28) & 0xf) + 1);
 
+	/*
+	 * EP93xx SSP clock rate was doubled in version E2. For more information
+	 * see:
+	 *     http://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf
+	 */
+	if (ep93xx_chip_revision() < EP93XX_CHIP_REV_E2)
+		clk_spi.rate /= 2;
+
 	pr_info("PLL1 running at %ld MHz, PLL2 at %ld MHz\n",
 		clk_pll1.rate / 1000000, clk_pll2.rate / 1000000);
 	pr_info("FCLK %ld MHz, HCLK %ld MHz, PCLK %ld MHz\n",
@@ -480,6 +493,7 @@ static int __init ep93xx_clock_init(void)
 		clk_p.rate / 1000000);
 
 	clkdev_add_table(clocks, ARRAY_SIZE(clocks));
+
 	return 0;
 }
 arch_initcall(ep93xx_clock_init);
diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
index 90fb591..e237309 100644
--- a/arch/arm/mach-ep93xx/core.c
+++ b/arch/arm/mach-ep93xx/core.c
@@ -35,6 +35,7 @@
 #include <mach/hardware.h>
 #include <mach/fb.h>
 #include <mach/ep93xx_keypad.h>
+#include <mach/ep93xx_spi.h>
 
 #include <asm/mach/map.h>
 #include <asm/mach/time.h>
@@ -363,6 +364,53 @@ void __init ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr)
 	platform_device_register(&ep93xx_eth_device);
 }
 
+static struct ep93xx_spi_info ep93xx_spi_master_data;
+
+static struct resource ep93xx_spi_resources[] = {
+	{
+		.start	= EP93XX_SPI_PHYS_BASE,
+		.end	= EP93XX_SPI_PHYS_BASE + 0x18 - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	{
+		.start	= IRQ_EP93XX_SSP,
+		.end	= IRQ_EP93XX_SSP,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device ep93xx_spi_device = {
+	.name		= "ep93xx-spi",
+	.id		= -1,
+	.dev		= {
+		.platform_data = &ep93xx_spi_master_data,
+	},
+	.num_resources	= ARRAY_SIZE(ep93xx_spi_resources),
+	.resource	= ep93xx_spi_resources,
+};
+
+/**
+ * ep93xx_register_spi() - registers spi platform device
+ * @info: ep93xx board specific spi master info (__initdata)
+ *
+ * This function registers platform device for the EP93xx SPI controller and
+ * also makes sure that SPI pins are muxed so that I2S is not using those
+ * pins. Caller should allocate necessary GPIO lines and register any SPI
+ * devices before calling this (see also spi_register_board_info()).
+ *
+ * Returns %0 in success and negative value in case of failure.
+ */
+int __init ep93xx_register_spi(struct ep93xx_spi_info *info)
+{
+	/*
+	 * When SPI is used, we need to make sure that I2S is muxed off from
+	 * SPI pins.
+	 */
+	ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_I2SONSSP);
+
+	ep93xx_spi_master_data = *info;
+	return platform_device_register(&ep93xx_spi_device);
+}
 
 /*************************************************************************
  * EP93xx i2c peripheral handling
diff --git a/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h b/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
index 93e2ecc..b1e096f 100644
--- a/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
+++ b/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
@@ -106,6 +106,7 @@
 
 #define EP93XX_AAC_BASE			EP93XX_APB_IOMEM(0x00080000)
 
+#define EP93XX_SPI_PHYS_BASE		EP93XX_APB_PHYS(0x000a0000)
 #define EP93XX_SPI_BASE			EP93XX_APB_IOMEM(0x000a0000)
 
 #define EP93XX_IRDA_BASE		EP93XX_APB_IOMEM(0x000b0000)
diff --git a/arch/arm/mach-ep93xx/include/mach/platform.h b/arch/arm/mach-ep93xx/include/mach/platform.h
index c6dc14d..5816581 100644
--- a/arch/arm/mach-ep93xx/include/mach/platform.h
+++ b/arch/arm/mach-ep93xx/include/mach/platform.h
@@ -9,6 +9,7 @@ struct i2c_board_info;
 struct platform_device;
 struct ep93xxfb_mach_info;
 struct ep93xx_keypad_platform_data;
+struct ep93xx_spi_info;
 
 struct ep93xx_eth_data
 {
@@ -34,6 +35,7 @@ static inline void ep93xx_devcfg_clear_bits(unsigned int bits)
 }
 
 void ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr);
+int ep93xx_register_spi(struct ep93xx_spi_info *info);
 void ep93xx_register_i2c(struct i2c_gpio_platform_data *data,
 			 struct i2c_board_info *devices, int num);
 void ep93xx_register_fb(struct ep93xxfb_mach_info *data);
-- 
1.5.6.5


------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH v4 2/2] ep93xx: SPI driver platform support code

[H Hartley Sweeten]

On Tuesday, April 20, 2010 8:12 AM, Mika Westerberg wrote:
>
> This patch adds platform side support code for EP93xx SPI driver. This includes
> clock, resources and muxing. There is a new function: ep93xx_register_spi() that
> can be used by board support code to register new SPI devices for the board.
>
> This patch depends on following ARM patch:
> 	5998/1 ep93xx: added chip revision reading function
>
> Signed-off-by: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>

Mika,

This is looking really good.  A couple comments below.

> ---
>  arch/arm/mach-ep93xx/clock.c                    |   14 +++++++
>  arch/arm/mach-ep93xx/core.c                     |   48 +++++++++++++++++++++++
>  arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h |    1 +
>  arch/arm/mach-ep93xx/include/mach/platform.h    |    2 +
>  4 files changed, 65 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
> index 5f80092..5986fae 100644
> --- a/arch/arm/mach-ep93xx/clock.c
> +++ b/arch/arm/mach-ep93xx/clock.c
> @@ -96,6 +96,10 @@ static struct clk clk_keypad = {
>  	.enable_mask	= EP93XX_SYSCON_KEYTCHCLKDIV_KEN,
>  	.set_rate	= set_keytchclk_rate,
>  };
> +static struct clk clk_spi = {
> +	.parent		= &clk_xtali,
> +	.rate		= EP93XX_EXT_CLK_RATE,

Good.

> +};
>  static struct clk clk_pwm = {
>  	.parent		= &clk_xtali,
>  	.rate		= EP93XX_EXT_CLK_RATE,
> @@ -186,6 +190,7 @@ static struct clk_lookup clocks[] = {
>  	INIT_CK("ep93xx-ohci",		NULL,		&clk_usb_host),
>  	INIT_CK("ep93xx-keypad",	NULL,		&clk_keypad),
>  	INIT_CK("ep93xx-fb",		NULL,		&clk_video),
> +	INIT_CK("ep93xx-spi",		NULL,		&clk_spi),

Comment on this farther below...

>  	INIT_CK(NULL,			"pwm_clk",	&clk_pwm),
>  	INIT_CK(NULL,			"m2p0",		&clk_m2p0),
>  	INIT_CK(NULL,			"m2p1",		&clk_m2p1),
> @@ -473,6 +478,14 @@ static int __init ep93xx_clock_init(void)
>  	/* Initialize the pll2 derived clocks */
>  	clk_usb_host.rate = clk_pll2.rate / (((value >> 28) & 0xf) + 1);
>  
> +	/*
> +	 * EP93xx SSP clock rate was doubled in version E2. For more information
> +	 * see:
> +	 *     http://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf
> +	 */
> +	if (ep93xx_chip_revision() < EP93XX_CHIP_REV_E2)
> +		clk_spi.rate /= 2;

Good.

> +
>  	pr_info("PLL1 running at %ld MHz, PLL2 at %ld MHz\n",
>  		clk_pll1.rate / 1000000, clk_pll2.rate / 1000000);
>  	pr_info("FCLK %ld MHz, HCLK %ld MHz, PCLK %ld MHz\n",
> @@ -480,6 +493,7 @@ static int __init ep93xx_clock_init(void)
>  		clk_p.rate / 1000000);
>  
>  	clkdev_add_table(clocks, ARRAY_SIZE(clocks));
> +
>  	return 0;
>  }
>  arch_initcall(ep93xx_clock_init);
> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> index 90fb591..e237309 100644
> --- a/arch/arm/mach-ep93xx/core.c
> +++ b/arch/arm/mach-ep93xx/core.c
> @@ -35,6 +35,7 @@
>  #include <mach/hardware.h>
>  #include <mach/fb.h>
>  #include <mach/ep93xx_keypad.h>
> +#include <mach/ep93xx_spi.h>
>  
>  #include <asm/mach/map.h>
>  #include <asm/mach/time.h>
> @@ -363,6 +364,53 @@ void __init ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr)
>  	platform_device_register(&ep93xx_eth_device);
>  }
>  
> +static struct ep93xx_spi_info ep93xx_spi_master_data;
> +
> +static struct resource ep93xx_spi_resources[] = {
> +	{
> +		.start	= EP93XX_SPI_PHYS_BASE,
> +		.end	= EP93XX_SPI_PHYS_BASE + 0x18 - 1,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	{
> +		.start	= IRQ_EP93XX_SSP,
> +		.end	= IRQ_EP93XX_SSP,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct platform_device ep93xx_spi_device = {
> +	.name		= "ep93xx-spi",
> +	.id		= -1,

This field is normally copied to the 'master->bus_num' field in the spi driver.  In
the API a -1 bus_num indicates a dynamically assigned bus ID.  It would probably be
cleaner to set the id here to 0 and change the line in the driver from:

+	master->bus_num = 0;

To

+	master->bus_num = pdev->id;

This will change the dev_id for the spi clock to 'ep93xx-spi.0' so the INIT_CK above
will need to be changed also.

> +	.dev		= {
> +		.platform_data = &ep93xx_spi_master_data,
> +	},
> +	.num_resources	= ARRAY_SIZE(ep93xx_spi_resources),
> +	.resource	= ep93xx_spi_resources,
> +};
> +
> +/**
> + * ep93xx_register_spi() - registers spi platform device
> + * @info: ep93xx board specific spi master info (__initdata)
> + *
> + * This function registers platform device for the EP93xx SPI controller and
> + * also makes sure that SPI pins are muxed so that I2S is not using those
> + * pins. Caller should allocate necessary GPIO lines and register any SPI
> + * devices before calling this (see also spi_register_board_info()).

It will be a bit cleaner to just pass the spi_board_info to this function and
do the spi_register_board_info here.  Look at the ep93xx_register_i2c function.

> + *
> + * Returns %0 in success and negative value in case of failure.
> + */
> +int __init ep93xx_register_spi(struct ep93xx_spi_info *info)

I'm not sure if the return value is necessary since this is all during init.
None of the other registration routines bother checking the platform_device_register
return value.  You can probably just drop it here and make this a 'void'

> +{
> +	/*
> +	 * When SPI is used, we need to make sure that I2S is muxed off from
> +	 * SPI pins.
> +	 */
> +	ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_I2SONSSP);
> +
> +	ep93xx_spi_master_data = *info;
> +	return platform_device_register(&ep93xx_spi_device);
> +}
>  
>  /*************************************************************************
>   * EP93xx i2c peripheral handling
> diff --git a/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h b/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> index 93e2ecc..b1e096f 100644
> --- a/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> +++ b/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> @@ -106,6 +106,7 @@
>  
>  #define EP93XX_AAC_BASE			EP93XX_APB_IOMEM(0x00080000)
>  
> +#define EP93XX_SPI_PHYS_BASE		EP93XX_APB_PHYS(0x000a0000)
>  #define EP93XX_SPI_BASE			EP93XX_APB_IOMEM(0x000a0000)
>  
>  #define EP93XX_IRDA_BASE		EP93XX_APB_IOMEM(0x000b0000)
> diff --git a/arch/arm/mach-ep93xx/include/mach/platform.h b/arch/arm/mach-ep93xx/include/mach/platform.h
> index c6dc14d..5816581 100644
> --- a/arch/arm/mach-ep93xx/include/mach/platform.h
> +++ b/arch/arm/mach-ep93xx/include/mach/platform.h
> @@ -9,6 +9,7 @@ struct i2c_board_info;
>  struct platform_device;
>  struct ep93xxfb_mach_info;
>  struct ep93xx_keypad_platform_data;
> +struct ep93xx_spi_info;
>  
>  struct ep93xx_eth_data
>  {
> @@ -34,6 +35,7 @@ static inline void ep93xx_devcfg_clear_bits(unsigned int bits)
>  }
>  
>  void ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr);
> +int ep93xx_register_spi(struct ep93xx_spi_info *info);
>  void ep93xx_register_i2c(struct i2c_gpio_platform_data *data,
>  			 struct i2c_board_info *devices, int num);
>  void ep93xx_register_fb(struct ep93xxfb_mach_info *data);

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev