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

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

[Mika Westerberg]

Hello,

This is third revision of the driver. Thanks to Martin Guy who tested and
reviewed the code.

Changes since v2:
	- corrected spi clock rate calculation
	- interrupt handling is now more efficient
	- driver now supports polling mode as well, this can be selected with
	  'transfer_method' module parameter.
	- controller is disabled in probe function
	- some cosmetic changes

I have been testing this on my TS-7260 board (ep9302 based) 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                     |   42 +
 arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h |    1 +
 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h  |   34 +
 arch/arm/mach-ep93xx/include/mach/platform.h    |    2 +
 drivers/spi/Kconfig                             |   11 +
 drivers/spi/Makefile                            |    1 +
 drivers/spi/ep93xx_spi.c                        | 1113 +++++++++++++++++++++++
 8 files changed, 1218 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 v3 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).

Driver supports polling and interrupt based transfer methods. This can be
selected with module parameter 'transfer_method'.

Signed-off-by: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>
---
 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h |   34 +
 drivers/spi/Kconfig                            |   11 +
 drivers/spi/Makefile                           |    1 +
 drivers/spi/ep93xx_spi.c                       | 1113 ++++++++++++++++++++++++
 4 files changed, 1159 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..fe93d2e
--- /dev/null
+++ b/arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h
@@ -0,0 +1,34 @@
+#ifndef __ASM_MACH_EP93XX_SPI_H
+#define __ASM_MACH_EP93XX_SPI_H
+
+/**
+ * struct ep93xx_spi_info - EP93xx specific SPI descriptor
+ * @num_chipselect: number of chip select GPIOs on this board
+ * @cs_control: chip select control function
+ * @data: pointer to data that is passed to @cs_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 GPIOs that are
+ * allocated in board support files during board initialization.
+ */
+struct ep93xx_spi_info {
+	int	num_chipselect;
+	/*
+	 * cs_control() - control board chipselect GPIO lines
+	 * @cs: chip select to control
+	 * @value: value to set the chip select line to
+	 * @data: optional data
+	 *
+	 * This function is used to control board specific GPIO lines that
+	 * are allocated as SPI chipselects. @value is either %0 or %1. It
+	 * is possibled to pass additional information using @data.
+	 *
+	 * This function is called from thread context and can sleep if
+	 * necessary.
+	 */
+	void	(*cs_control)(unsigned cs, unsigned value, void *data);
+	void	*data;
+};
+
+#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..7273611
--- /dev/null
+++ b/drivers/spi/ep93xx_spi.c
@@ -0,0 +1,1113 @@
+/*
+ * 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		100
+/* maximum depth of RX/TX FIFO */
+#define SPI_FIFO_SIZE		8
+
+enum {
+	EP93XX_SPI_METHOD_POLL,
+	EP93XX_SPI_METHOD_INTERRUPT,
+	/* TODO: waiting for DMA support */
+};
+
+static int transfer_method = EP93XX_SPI_METHOD_INTERRUPT;
+module_param(transfer_method, int, S_IRUGO);
+MODULE_PARM_DESC(transfer_method,
+	"Used transfer method (0=poll, 1=interrupt [default])");
+
+/**
+ * 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
+ * @info: platform specific info
+ * @clk: clock for the controller
+ * @regs_base: pointer to ioremap()'d registers
+ * @irq: IRQ number used by the driver. If @irq is %0, we are in polled mode.
+ * @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.
+ * @transfer: method that performs the actual data transfer
+ *
+ * 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;
+	const struct ep93xx_spi_info	*info;
+	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				(*transfer)(struct ep93xx_spi *);
+};
+
+/**
+ * struct ep93xx_spi_chip - SPI device hardware settings
+ * @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)
+ * @mode: chip SPI mode (see also &struct spi_device)
+ *
+ * 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 {
+	unsigned long	rate;
+	u8		div_cpsr;
+	u8		div_scr;
+	u8		dss;
+	u8		mode;
+};
+
+/* 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_flush() - flushes SPI RX FIFO
+ * @espi: ep93xx SPI controller struct
+ * @msecs: timeout in milliseconds to wait
+ *
+ * This function flushes RX FIFO. Returns %0 in case of success and %-ETIMEDOUT
+ * when timeout occured. Wait is implemented in busylooping so this function can
+ * also be called from atomic context.
+ */
+static int ep93xx_spi_flush(const struct ep93xx_spi *espi, unsigned long msecs)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(msecs);
+
+	while (ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_RNE) {
+		if (time_after(jiffies, timeout))
+			return -ETIMEDOUT;
+
+		ep93xx_spi_read_u16(espi, SSPDR);
+	}
+
+	return 0;
+}
+
+/**
+ * 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 GPIO chipselect line as specified in @spi->chip_select in
+ * board specific manner (calls @info->cs_control()).
+ *
+ * Note that this function is called from a thread context and can sleep.
+ */
+static void ep93xx_spi_select_device(const struct ep93xx_spi *espi,
+				     struct spi_device *spi)
+{
+	const struct ep93xx_spi_info *info = espi->info;
+	unsigned value = (spi->mode & SPI_CS_HIGH) ? 1 : 0;
+
+	info->cs_control(spi->chip_select, value, info->data);
+}
+
+/**
+ * ep93xx_spi_deselect_device() - deselects given SPI device
+ * @espi: ep93xx SPI controller struct
+ * @spi: SPI device to deselect
+ *
+ * Function deasserts GPIO chipselect line as specified in @spi->chip_select in
+ * board specific manner (calls @info->cs_control()).
+ *
+ * Note that this function is called from a thread context and can sleep.
+ */
+static void ep93xx_spi_deselect_device(const struct ep93xx_spi *espi,
+				       struct spi_device *spi)
+{
+	const struct ep93xx_spi_info *info = espi->info;
+	unsigned value = (spi->mode & SPI_CS_HIGH) ? 0 : 1;
+
+	info->cs_control(spi->chip_select, value, info->data);
+}
+
+/**
+ * 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);
+	}
+
+	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->mode = spi->mode;
+	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;
+
+	spin_lock_irqsave(&espi->lock, flags);
+	if (!espi->running) {
+		spin_unlock_irqrestore(&espi->lock, flags);
+		return -ESHUTDOWN;
+	}
+	spin_unlock_irqrestore(&espi->lock, flags);
+
+	/* 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);
+	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->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->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);
+}
+
+static void u8_writer(struct ep93xx_spi *espi,
+		      const struct spi_transfer *t)
+{
+	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 u8_reader(struct ep93xx_spi *espi,
+		      struct spi_transfer *t)
+{
+	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);
+}
+
+static void u16_writer(struct ep93xx_spi *espi,
+		       const struct spi_transfer *t)
+{
+	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);
+}
+
+static void u16_reader(struct ep93xx_spi *espi,
+		       struct spi_transfer *t)
+{
+	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);
+}
+
+/**
+ * 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_writer(espi, t);
+	else
+		u8_writer(espi, t);
+}
+
+static void ep93xx_do_read(struct ep93xx_spi *espi,
+			   struct spi_transfer *t)
+{
+	if (bits_per_word(espi) > 8)
+		u16_reader(espi, t);
+	else
+		u8_reader(espi, t);
+}
+
+/**
+ * ep93xx_spi_read_write() - perform next RX/TX transfer
+ * @espi: ep93xx SPI controller struct
+ *
+ * This function transfers next bytes (or 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.
+ *
+ * Can be called from any context.
+ */
+static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
+{
+	struct spi_message *msg;
+	struct spi_transfer *t;
+	unsigned long flags;
+
+	spin_lock_irqsave(&espi->lock, flags);
+	msg = espi->current_msg;
+	spin_unlock_irqrestore(&espi->lock, flags);
+
+	t = msg->state;
+
+	/*
+	 * We explicitly handle FIFO level here. This way we don't have to check
+	 * TX FIFO status using %SSPSR_TNF bit which may cause RX FIFO overruns.
+	 */
+	if (espi->tx == 0 && espi->rx == 0)
+		espi->fifo_level = 0;
+
+	/* read all received data */
+	while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
+		espi->rx < t->len) {
+		ep93xx_do_read(espi, t);
+		espi->fifo_level--;
+	}
+
+	/* write as long as FIFO has room */
+	while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
+		ep93xx_do_write(espi, t);
+		espi->fifo_level++;
+	}
+
+	/* are we done yet? */
+	if (espi->tx == t->len && espi->rx == t->len) {
+		msg->actual_length += t->len;
+		return t->len;
+	}
+
+	return 0;
+}
+
+/**
+ * ep93xx_spi_polling_transfer() - implement polling mode transfer
+ * @espi: ep93xx SPI controller struct
+ *
+ * Function performs one polling mode transfer. When this function is finished,
+ * the whole transfer should be completed, or failed in case of error.
+ */
+static void ep93xx_spi_polling_transfer(struct ep93xx_spi *espi)
+{
+	while (!ep93xx_spi_read_write(espi))
+		cpu_relax();
+}
+
+/**
+ * ep93xx_spi_polling_transfer() - implement interrupt based transfer
+ * @espi: ep93xx SPI controller struct
+ *
+ * Function performs one interrupt based transfer. When this function is
+ * finished, the whole transfer should be completed, or failed in case of error.
+ */
+static void ep93xx_spi_interrupt_transfer(struct ep93xx_spi *espi)
+{
+	ep93xx_spi_enable_interrupts(espi);
+	wait_for_completion(&espi->wait);
+}
+
+/**
+ * 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. Perform the actual
+	 * transfer depending on the method.
+	 */
+	espi->transfer(espi);
+
+	/*
+	 * 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)
+		udelay(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)
+{
+	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.
+	 */
+	err = ep93xx_spi_flush(espi, SPI_TIMEOUT);
+	if (err) {
+		dev_warn(&espi->pdev->dev, "timeout while flushing RX fifo\n");
+		msg->status = err;
+		return;
+	}
+
+	/*
+	 * 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) {
+		cond_resched();
+		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);
+}
+
+/**
+ * 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");
+
+		spin_lock(&espi->lock);
+		espi->current_msg->status = -EIO;
+		spin_unlock(&espi->lock);
+	} 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->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->info = info;
+	espi->pdev = pdev;
+
+	switch (transfer_method) {
+	case EP93XX_SPI_METHOD_POLL:
+		espi->transfer = ep93xx_spi_polling_transfer;
+		break;
+
+	case EP93XX_SPI_METHOD_INTERRUPT:
+		espi->transfer = ep93xx_spi_interrupt_transfer;
+		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;
+		}
+		break;
+
+	default:
+		dev_err(&pdev->dev, "invalid transfer method %d given\n",
+			transfer_method);
+		error = -EINVAL;
+		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;
+	}
+
+	if (espi->irq) {
+		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)
+		goto fail_free_queue;
+
+	dev_info(&pdev->dev, "EP93xx SPI Controller at 0x%08lx\n",
+		 (unsigned long)res->start);
+	if (espi->irq)
+		dev_info(&pdev->dev, "using irq %d\n",  espi->irq);
+
+	return 0;
+
+fail_free_queue:
+	destroy_workqueue(espi->wq);
+fail_free_irq:
+	if (espi->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);
+
+	if (espi->irq)
+		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 v3 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Grant Likely]

Hi Mika,

Thanks for the patch.  Comments below.

g.

On Tue, Apr 13, 2010 at 7:10 AM, Mika Westerberg <mika.westerberg@iki.fi> wrote:
> This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
> in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).
>
> Driver supports polling and interrupt based transfer methods. This can be
> selected with module parameter 'transfer_method'.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> ---
>  arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h |   34 +
>  drivers/spi/Kconfig                            |   11 +
>  drivers/spi/Makefile                           |    1 +
>  drivers/spi/ep93xx_spi.c                       | 1113 ++++++++++++++++++++++++
>  4 files changed, 1159 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..fe93d2e
> --- /dev/null
> +++ b/arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h
> @@ -0,0 +1,34 @@
> +#ifndef __ASM_MACH_EP93XX_SPI_H
> +#define __ASM_MACH_EP93XX_SPI_H
> +
> +/**
> + * struct ep93xx_spi_info - EP93xx specific SPI descriptor
> + * @num_chipselect: number of chip select GPIOs on this board
> + * @cs_control: chip select control function
> + * @data: pointer to data that is passed to @cs_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 GPIOs that are
> + * allocated in board support files during board initialization.
> + */
> +struct ep93xx_spi_info {
> +       int     num_chipselect;
> +       /*
> +        * cs_control() - control board chipselect GPIO lines
> +        * @cs: chip select to control
> +        * @value: value to set the chip select line to
> +        * @data: optional data
> +        *
> +        * This function is used to control board specific GPIO lines that
> +        * are allocated as SPI chipselects. @value is either %0 or %1. It
> +        * is possibled to pass additional information using @data.
> +        *
> +        * This function is called from thread context and can sleep if
> +        * necessary.
> +        */
> +       void    (*cs_control)(unsigned cs, unsigned value, void *data);
> +       void    *data;
> +};
> +
> +#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..7273611
> --- /dev/null
> +++ b/drivers/spi/ep93xx_spi.c
> @@ -0,0 +1,1113 @@
> +/*
> + * 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            100
> +/* maximum depth of RX/TX FIFO */
> +#define SPI_FIFO_SIZE          8
> +
> +enum {
> +       EP93XX_SPI_METHOD_POLL,
> +       EP93XX_SPI_METHOD_INTERRUPT,
> +       /* TODO: waiting for DMA support */
> +};
> +
> +static int transfer_method = EP93XX_SPI_METHOD_INTERRUPT;
> +module_param(transfer_method, int, S_IRUGO);
> +MODULE_PARM_DESC(transfer_method,
> +       "Used transfer method (0=poll, 1=interrupt [default])");

Should this really be a module parameter?  Would it be better to
expose this knob as part of pdata or in sysfs?  (Assuming there even
is a real need to manipulate this control)

> +
> +/**
> + * 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
> + * @info: platform specific info
> + * @clk: clock for the controller
> + * @regs_base: pointer to ioremap()'d registers
> + * @irq: IRQ number used by the driver. If @irq is %0, we are in polled mode.
> + * @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.
> + * @transfer: method that performs the actual data transfer
> + *
> + * 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;
> +       const struct ep93xx_spi_info    *info;

You probably don't need to keep the info pointer around.  It isn't
interesting once you've extracted the data out of it.

> +       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                            (*transfer)(struct ep93xx_spi *);
> +};
> +
> +/**
> + * struct ep93xx_spi_chip - SPI device hardware settings
> + * @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)
> + * @mode: chip SPI mode (see also &struct spi_device)
> + *
> + * 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 {
> +       unsigned long   rate;
> +       u8              div_cpsr;
> +       u8              div_scr;
> +       u8              dss;
> +       u8              mode;
> +};
> +
> +/* 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_flush() - flushes SPI RX FIFO
> + * @espi: ep93xx SPI controller struct
> + * @msecs: timeout in milliseconds to wait
> + *
> + * This function flushes RX FIFO. Returns %0 in case of success and %-ETIMEDOUT
> + * when timeout occured. Wait is implemented in busylooping so this function can
> + * also be called from atomic context.
> + */
> +static int ep93xx_spi_flush(const struct ep93xx_spi *espi, unsigned long msecs)
> +{
> +       unsigned long timeout = jiffies + msecs_to_jiffies(msecs);
> +
> +       while (ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_RNE) {
> +               if (time_after(jiffies, timeout))
> +                       return -ETIMEDOUT;

Ouch.  With the current code the driver could busy wait for 100ms!  If
flushing takes time, then don't busywait.  Bail out to some form of
deferred work.  Let the kernel get on with something else.

> +
> +               ep93xx_spi_read_u16(espi, SSPDR);
> +       }
> +
> +       return 0;
> +}

This function is only called in one place; should it just be made a
part of ep93xx_spi_process_message?

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

Non-deterministic.  Big nested loops mean a lot of calculations within
 ep93xx_spi_process_transfer, and calculated values aren't cached if
they get replaced.  If you can, it would be better if you can
algebraically calculate the factors instead of an iterative loop.
It's requires more thinking at the outset, but it is worth the effort.

> +                       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 GPIO chipselect line as specified in @spi->chip_select in
> + * board specific manner (calls @info->cs_control()).
> + *
> + * Note that this function is called from a thread context and can sleep.
> + */
> +static void ep93xx_spi_select_device(const struct ep93xx_spi *espi,
> +                                    struct spi_device *spi)
> +{
> +       const struct ep93xx_spi_info *info = espi->info;
> +       unsigned value = (spi->mode & SPI_CS_HIGH) ? 1 : 0;
> +
> +       info->cs_control(spi->chip_select, value, info->data);
> +}
> +
> +/**
> + * ep93xx_spi_deselect_device() - deselects given SPI device
> + * @espi: ep93xx SPI controller struct
> + * @spi: SPI device to deselect
> + *
> + * Function deasserts GPIO chipselect line as specified in @spi->chip_select in
> + * board specific manner (calls @info->cs_control()).
> + *
> + * Note that this function is called from a thread context and can sleep.
> + */
> +static void ep93xx_spi_deselect_device(const struct ep93xx_spi *espi,
> +                                      struct spi_device *spi)
> +{
> +       const struct ep93xx_spi_info *info = espi->info;
> +       unsigned value = (spi->mode & SPI_CS_HIGH) ? 0 : 1;
> +
> +       info->cs_control(spi->chip_select, value, info->data);
> +}

These 2 functions are identical, other than the test being reversed.
The code can be a lot more concise here.

> +
> +/**
> + * 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);
> +       }
> +
> +       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->mode = spi->mode;
> +       chip->dss = bits_per_word_to_dss(spi->bits_per_word);

Why is mode and dss getting copied into chip?  chip should probable
instead have a reference back to its assigned spi device and the
original values should be used.

> +
> +       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;
> +
> +       spin_lock_irqsave(&espi->lock, flags);
> +       if (!espi->running) {
> +               spin_unlock_irqrestore(&espi->lock, flags);
> +               return -ESHUTDOWN;
> +       }
> +       spin_unlock_irqrestore(&espi->lock, flags);

The spin lock isn't protecting anything here.  The value of running
can change either immediately before or immediately after the critical
region.  Reading espi->running without the spinlock will have exactly
the same behaviour.

If the value of running matters when processing this function, then
the function needs to continue to hold the spin lock while the message
is being processed and then added to the message queue at the end of
the function.

However, the for_each_entry loop below is safe to process before
grabbing the lock, so you can defer grabbing the lock and checking
->running until just before the message is enqueued.

> +
> +       /* 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);
> +       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->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->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);
> +}
> +
> +static void u8_writer(struct ep93xx_spi *espi,
> +                     const struct spi_transfer *t)
> +{
> +       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 u8_reader(struct ep93xx_spi *espi,
> +                     struct spi_transfer *t)
> +{
> +       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);
> +}
> +
> +static void u16_writer(struct ep93xx_spi *espi,
> +                      const struct spi_transfer *t)
> +{
> +       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);
> +}
> +
> +static void u16_reader(struct ep93xx_spi *espi,
> +                      struct spi_transfer *t)
> +{
> +       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);
> +}

These 4 functions are only used once each.  Just merge them into the caller.

> +
> +/**
> + * 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_writer(espi, t);
> +       else
> +               u8_writer(espi, t);
> +}
> +
> +static void ep93xx_do_read(struct ep93xx_spi *espi,
> +                          struct spi_transfer *t)
> +{
> +       if (bits_per_word(espi) > 8)
> +               u16_reader(espi, t);
> +       else
> +               u8_reader(espi, t);
> +}
> +
> +/**
> + * ep93xx_spi_read_write() - perform next RX/TX transfer
> + * @espi: ep93xx SPI controller struct
> + *
> + * This function transfers next bytes (or 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.
> + *
> + * Can be called from any context.
> + */
> +static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
> +{
> +       struct spi_message *msg;
> +       struct spi_transfer *t;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&espi->lock, flags);
> +       msg = espi->current_msg;
> +       spin_unlock_irqrestore(&espi->lock, flags);

Again, this spin lock isn't protecting anything.  Plain single reads
can always be considered to be atomic.  Review the locking model, but
I suspect the lock needs to be held over the entire function.

> +
> +       t = msg->state;
> +
> +       /*
> +        * We explicitly handle FIFO level here. This way we don't have to check
> +        * TX FIFO status using %SSPSR_TNF bit which may cause RX FIFO overruns.
> +        */
> +       if (espi->tx == 0 && espi->rx == 0)
> +               espi->fifo_level = 0;
> +
> +       /* read all received data */
> +       while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
> +               espi->rx < t->len) {
> +               ep93xx_do_read(espi, t);
> +               espi->fifo_level--;
> +       }

Another busywait loop.  Could end up burning an awful lot of cycles
here.  A state machine might be more suitable for this driver so that
processing can be punted to deferred work when the FIFO gets either
empty or full.

> +
> +       /* write as long as FIFO has room */
> +       while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
> +               ep93xx_do_write(espi, t);
> +               espi->fifo_level++;
> +       }
> +
> +       /* are we done yet? */
> +       if (espi->tx == t->len && espi->rx == t->len) {
> +               msg->actual_length += t->len;
> +               return t->len;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * ep93xx_spi_polling_transfer() - implement polling mode transfer
> + * @espi: ep93xx SPI controller struct
> + *
> + * Function performs one polling mode transfer. When this function is finished,
> + * the whole transfer should be completed, or failed in case of error.
> + */
> +static void ep93xx_spi_polling_transfer(struct ep93xx_spi *espi)
> +{
> +       while (!ep93xx_spi_read_write(espi))
> +               cpu_relax();

And if it never finishes?  No exit path on error.

> +}
> +
> +/**
> + * ep93xx_spi_polling_transfer() - implement interrupt based transfer

Comment doesn't match

> + * @espi: ep93xx SPI controller struct
> + *
> + * Function performs one interrupt based transfer. When this function is
> + * finished, the whole transfer should be completed, or failed in case of error.
> + */
> +static void ep93xx_spi_interrupt_transfer(struct ep93xx_spi *espi)
> +{
> +       ep93xx_spi_enable_interrupts(espi);
> +       wait_for_completion(&espi->wait);
> +}
> +
> +/**
> + * 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. Perform the actual
> +        * transfer depending on the method.
> +        */
> +       espi->transfer(espi);
> +
> +       /*
> +        * 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)
> +               udelay(t->delay_usecs);

Consider sleeping instead.  busywaits increase latency.

> +       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)
> +{
> +       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.
> +        */
> +       err = ep93xx_spi_flush(espi, SPI_TIMEOUT);
> +       if (err) {
> +               dev_warn(&espi->pdev->dev, "timeout while flushing RX fifo\n");
> +               msg->status = err;
> +               return;
> +       }
> +
> +       /*
> +        * 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) {
> +               cond_resched();

Why is this necessary?

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

I'm nervous about the locking.  I'm not convinced that it covers all
the important cases.  Are you able to describe what the locks protect,
and why they are taken when they are?  That would help me evaluate
what the driver should be doing.

> +
> +       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);
> +}
> +
> +/**
> + * 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");
> +
> +               spin_lock(&espi->lock);
> +               espi->current_msg->status = -EIO;
> +               spin_unlock(&espi->lock);
> +       } 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->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->info = info;
> +       espi->pdev = pdev;
> +
> +       switch (transfer_method) {
> +       case EP93XX_SPI_METHOD_POLL:
> +               espi->transfer = ep93xx_spi_polling_transfer;
> +               break;
> +
> +       case EP93XX_SPI_METHOD_INTERRUPT:
> +               espi->transfer = ep93xx_spi_interrupt_transfer;
> +               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;
> +               }
> +               break;
> +
> +       default:
> +               dev_err(&pdev->dev, "invalid transfer method %d given\n",
> +                       transfer_method);
> +               error = -EINVAL;
> +               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;
> +       }
> +
> +       if (espi->irq) {
> +               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)
> +               goto fail_free_queue;
> +
> +       dev_info(&pdev->dev, "EP93xx SPI Controller at 0x%08lx\n",
> +                (unsigned long)res->start);
> +       if (espi->irq)
> +               dev_info(&pdev->dev, "using irq %d\n",  espi->irq);
> +
> +       return 0;
> +
> +fail_free_queue:
> +       destroy_workqueue(espi->wq);
> +fail_free_irq:
> +       if (espi->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);
> +
> +       if (espi->irq)
> +               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@iki.fi>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ep93xx-spi");
> --
> 1.5.6.5
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

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

[Mika Westerberg]

On Fri, Apr 16, 2010 at 11:30:45PM -0700, Grant Likely wrote:
> Thanks for the patch.  Comments below.

Thanks for the comments. See below my replies.

> g.
> 
> On Tue, Apr 13, 2010 at 7:10 AM, Mika Westerberg <mika.westerberg@iki.fi> wrote:
> > This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
> > in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).
> >
> > Driver supports polling and interrupt based transfer methods. This can be
> > selected with module parameter 'transfer_method'.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> > ---
> >  arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h |   34 +
> >  drivers/spi/Kconfig                            |   11 +
> >  drivers/spi/Makefile                           |    1 +
> >  drivers/spi/ep93xx_spi.c                       | 1113 ++++++++++++++++++++++++
> >  4 files changed, 1159 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..fe93d2e
> > --- /dev/null
> > +++ b/arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h
> > @@ -0,0 +1,34 @@
> > +#ifndef __ASM_MACH_EP93XX_SPI_H
> > +#define __ASM_MACH_EP93XX_SPI_H
> > +
> > +/**
> > + * struct ep93xx_spi_info - EP93xx specific SPI descriptor
> > + * @num_chipselect: number of chip select GPIOs on this board
> > + * @cs_control: chip select control function
> > + * @data: pointer to data that is passed to @cs_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 GPIOs that are
> > + * allocated in board support files during board initialization.
> > + */
> > +struct ep93xx_spi_info {
> > +       int     num_chipselect;
> > +       /*
> > +        * cs_control() - control board chipselect GPIO lines
> > +        * @cs: chip select to control
> > +        * @value: value to set the chip select line to
> > +        * @data: optional data
> > +        *
> > +        * This function is used to control board specific GPIO lines that
> > +        * are allocated as SPI chipselects. @value is either %0 or %1. It
> > +        * is possibled to pass additional information using @data.
> > +        *
> > +        * This function is called from thread context and can sleep if
> > +        * necessary.
> > +        */
> > +       void    (*cs_control)(unsigned cs, unsigned value, void *data);
> > +       void    *data;
> > +};
> > +
> > +#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..7273611
> > --- /dev/null
> > +++ b/drivers/spi/ep93xx_spi.c
> > @@ -0,0 +1,1113 @@
> > +/*
> > + * 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            100
> > +/* maximum depth of RX/TX FIFO */
> > +#define SPI_FIFO_SIZE          8
> > +
> > +enum {
> > +       EP93XX_SPI_METHOD_POLL,
> > +       EP93XX_SPI_METHOD_INTERRUPT,
> > +       /* TODO: waiting for DMA support */
> > +};
> > +
> > +static int transfer_method = EP93XX_SPI_METHOD_INTERRUPT;
> > +module_param(transfer_method, int, S_IRUGO);
> > +MODULE_PARM_DESC(transfer_method,
> > +       "Used transfer method (0=poll, 1=interrupt [default])");
> 
> Should this really be a module parameter?  Would it be better to
> expose this knob as part of pdata or in sysfs?  (Assuming there even
> is a real need to manipulate this control)

Hard to say. At least it was easier to test both modes by just
loading the module with correct parameter but it could possibly be
placed in pdata also, if required.

> 
> > +
> > +/**
> > + * 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
> > + * @info: platform specific info
> > + * @clk: clock for the controller
> > + * @regs_base: pointer to ioremap()'d registers
> > + * @irq: IRQ number used by the driver. If @irq is %0, we are in polled mode.
> > + * @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.
> > + * @transfer: method that performs the actual data transfer
> > + *
> > + * 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;
> > +       const struct ep93xx_spi_info    *info;
> 
> You probably don't need to keep the info pointer around.  It isn't
> interesting once you've extracted the data out of it.

Ok.

> 
> > +       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                            (*transfer)(struct ep93xx_spi *);
> > +};
> > +
> > +/**
> > + * struct ep93xx_spi_chip - SPI device hardware settings
> > + * @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)
> > + * @mode: chip SPI mode (see also &struct spi_device)
> > + *
> > + * 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 {
> > +       unsigned long   rate;
> > +       u8              div_cpsr;
> > +       u8              div_scr;
> > +       u8              dss;
> > +       u8              mode;
> > +};
> > +
> > +/* 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_flush() - flushes SPI RX FIFO
> > + * @espi: ep93xx SPI controller struct
> > + * @msecs: timeout in milliseconds to wait
> > + *
> > + * This function flushes RX FIFO. Returns %0 in case of success and %-ETIMEDOUT
> > + * when timeout occured. Wait is implemented in busylooping so this function can
> > + * also be called from atomic context.
> > + */
> > +static int ep93xx_spi_flush(const struct ep93xx_spi *espi, unsigned long msecs)
> > +{
> > +       unsigned long timeout = jiffies + msecs_to_jiffies(msecs);
> > +
> > +       while (ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_RNE) {
> > +               if (time_after(jiffies, timeout))
> > +                       return -ETIMEDOUT;
> 
> Ouch.  With the current code the driver could busy wait for 100ms!  If
> flushing takes time, then don't busywait.  Bail out to some form of
> deferred work.  Let the kernel get on with something else.

I've never witnessed flushing to take this whole 100ms. Timeout is
there just to make sure that this ends eventually. I can change it
to some lower value (say few msecs or something like that).

> 
> > +
> > +               ep93xx_spi_read_u16(espi, SSPDR);
> > +       }
> > +
> > +       return 0;
> > +}
> 
> This function is only called in one place; should it just be made a
> part of ep93xx_spi_process_message?

Yes. I'll do that.

> 
> > +
> > +/**
> > + * 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++) {
> 
> Non-deterministic.  Big nested loops mean a lot of calculations within
>  ep93xx_spi_process_transfer, and calculated values aren't cached if
> they get replaced.  If you can, it would be better if you can
> algebraically calculate the factors instead of an iterative loop.
> It's requires more thinking at the outset, but it is worth the effort.

Is it normal to change speed per transfer? ep93xx_spi_process_transfer()
only calls this if speed changes. That's why I used those loops
here; the assumption is that it is only called at setup time (and
even there only when speed changes).

For example in my test system using mmc_spi it was called total 3 times.

> 
> > +                       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 GPIO chipselect line as specified in @spi->chip_select in
> > + * board specific manner (calls @info->cs_control()).
> > + *
> > + * Note that this function is called from a thread context and can sleep.
> > + */
> > +static void ep93xx_spi_select_device(const struct ep93xx_spi *espi,
> > +                                    struct spi_device *spi)
> > +{
> > +       const struct ep93xx_spi_info *info = espi->info;
> > +       unsigned value = (spi->mode & SPI_CS_HIGH) ? 1 : 0;
> > +
> > +       info->cs_control(spi->chip_select, value, info->data);
> > +}
> > +
> > +/**
> > + * ep93xx_spi_deselect_device() - deselects given SPI device
> > + * @espi: ep93xx SPI controller struct
> > + * @spi: SPI device to deselect
> > + *
> > + * Function deasserts GPIO chipselect line as specified in @spi->chip_select in
> > + * board specific manner (calls @info->cs_control()).
> > + *
> > + * Note that this function is called from a thread context and can sleep.
> > + */
> > +static void ep93xx_spi_deselect_device(const struct ep93xx_spi *espi,
> > +                                      struct spi_device *spi)
> > +{
> > +       const struct ep93xx_spi_info *info = espi->info;
> > +       unsigned value = (spi->mode & SPI_CS_HIGH) ? 0 : 1;
> > +
> > +       info->cs_control(spi->chip_select, value, info->data);
> > +}
> 
> These 2 functions are identical, other than the test being reversed.
> The code can be a lot more concise here.

Ok.

> 
> > +
> > +/**
> > + * 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);
> > +       }
> > +
> > +       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->mode = spi->mode;
> > +       chip->dss = bits_per_word_to_dss(spi->bits_per_word);
> 
> Why is mode and dss getting copied into chip?  chip should probable
> instead have a reference back to its assigned spi device and the
> original values should be used.

This is done because if there are transfer specific settings (for
example bits per word changes) we can just make temporary copy of
chip and change those values and then restore back the original,
once the transfer is finished (see ep93xx_spi_process_transfer()).

Mode, however is redundant as it cannot be changed per transfer (or
even per message). I will drop that from the struct.

> 
> > +
> > +       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;
> > +
> > +       spin_lock_irqsave(&espi->lock, flags);
> > +       if (!espi->running) {
> > +               spin_unlock_irqrestore(&espi->lock, flags);
> > +               return -ESHUTDOWN;
> > +       }
> > +       spin_unlock_irqrestore(&espi->lock, flags);
> 
> The spin lock isn't protecting anything here.  The value of running
> can change either immediately before or immediately after the critical
> region.  Reading espi->running without the spinlock will have exactly
> the same behaviour.
> 
> If the value of running matters when processing this function, then
> the function needs to continue to hold the spin lock while the message
> is being processed and then added to the message queue at the end of
> the function.

Ah, you are right. Motivation here was to make sure that the driver isn't
shutting down.

> However, the for_each_entry loop below is safe to process before
> grabbing the lock, so you can defer grabbing the lock and checking
> ->running until just before the message is enqueued.

Ok. Thanks.

> 
> > +
> > +       /* 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);
> > +       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->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->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);
> > +}
> > +
> > +static void u8_writer(struct ep93xx_spi *espi,
> > +                     const struct spi_transfer *t)
> > +{
> > +       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 u8_reader(struct ep93xx_spi *espi,
> > +                     struct spi_transfer *t)
> > +{
> > +       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);
> > +}
> > +
> > +static void u16_writer(struct ep93xx_spi *espi,
> > +                      const struct spi_transfer *t)
> > +{
> > +       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);
> > +}
> > +
> > +static void u16_reader(struct ep93xx_spi *espi,
> > +                      struct spi_transfer *t)
> > +{
> > +       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);
> > +}
> 
> These 4 functions are only used once each.  Just merge them into the caller.

Will do.

> 
> > +
> > +/**
> > + * 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_writer(espi, t);
> > +       else
> > +               u8_writer(espi, t);
> > +}
> > +
> > +static void ep93xx_do_read(struct ep93xx_spi *espi,
> > +                          struct spi_transfer *t)
> > +{
> > +       if (bits_per_word(espi) > 8)
> > +               u16_reader(espi, t);
> > +       else
> > +               u8_reader(espi, t);
> > +}
> > +
> > +/**
> > + * ep93xx_spi_read_write() - perform next RX/TX transfer
> > + * @espi: ep93xx SPI controller struct
> > + *
> > + * This function transfers next bytes (or 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.
> > + *
> > + * Can be called from any context.
> > + */
> > +static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
> > +{
> > +       struct spi_message *msg;
> > +       struct spi_transfer *t;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&espi->lock, flags);
> > +       msg = espi->current_msg;
> > +       spin_unlock_irqrestore(&espi->lock, flags);
> 
> Again, this spin lock isn't protecting anything.  Plain single reads
> can always be considered to be atomic.  Review the locking model, but
> I suspect the lock needs to be held over the entire function.

Ok. I'll re-check the locking model. But see also my explanation below.

> 
> > +
> > +       t = msg->state;
> > +
> > +       /*
> > +        * We explicitly handle FIFO level here. This way we don't have to check
> > +        * TX FIFO status using %SSPSR_TNF bit which may cause RX FIFO overruns.
> > +        */
> > +       if (espi->tx == 0 && espi->rx == 0)
> > +               espi->fifo_level = 0;
> > +
> > +       /* read all received data */
> > +       while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
> > +               espi->rx < t->len) {
> > +               ep93xx_do_read(espi, t);
> > +               espi->fifo_level--;
> > +       }
> 
> Another busywait loop.  Could end up burning an awful lot of cycles
> here.  A state machine might be more suitable for this driver so that
> processing can be punted to deferred work when the FIFO gets either
> empty or full.

FIFO size is max 8 frames so there should be 8 iterations when the
FIFO is full. So is it enought to to add check that we only read
max 8 frames from the FIFO?

> > +
> > +       /* write as long as FIFO has room */
> > +       while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
> > +               ep93xx_do_write(espi, t);
> > +               espi->fifo_level++;
> > +       }
> > +
> > +       /* are we done yet? */
> > +       if (espi->tx == t->len && espi->rx == t->len) {
> > +               msg->actual_length += t->len;
> > +               return t->len;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * ep93xx_spi_polling_transfer() - implement polling mode transfer
> > + * @espi: ep93xx SPI controller struct
> > + *
> > + * Function performs one polling mode transfer. When this function is finished,
> > + * the whole transfer should be completed, or failed in case of error.
> > + */
> > +static void ep93xx_spi_polling_transfer(struct ep93xx_spi *espi)
> > +{
> > +       while (!ep93xx_spi_read_write(espi))
> > +               cpu_relax();
> 
> And if it never finishes?  No exit path on error.

Well with current implementation of ep93xx_spi_read_write() it
cannot fail.  I guess some sort of timeout could be used here. Also
we can take ROR (receive overrun) interrupt and report that back.

> 
> > +}
> > +
> > +/**
> > + * ep93xx_spi_polling_transfer() - implement interrupt based transfer
> 
> Comment doesn't match

Copy paste error, will fix.

> 
> > + * @espi: ep93xx SPI controller struct
> > + *
> > + * Function performs one interrupt based transfer. When this function is
> > + * finished, the whole transfer should be completed, or failed in case of error.
> > + */
> > +static void ep93xx_spi_interrupt_transfer(struct ep93xx_spi *espi)
> > +{
> > +       ep93xx_spi_enable_interrupts(espi);
> > +       wait_for_completion(&espi->wait);
> > +}
> > +
> > +/**
> > + * 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. Perform the actual
> > +        * transfer depending on the method.
> > +        */
> > +       espi->transfer(espi);
> > +
> > +       /*
> > +        * 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)
> > +               udelay(t->delay_usecs);
> 
> Consider sleeping instead.  busywaits increase latency.

Ok.

> 
> > +       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)
> > +{
> > +       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.
> > +        */
> > +       err = ep93xx_spi_flush(espi, SPI_TIMEOUT);
> > +       if (err) {
> > +               dev_warn(&espi->pdev->dev, "timeout while flushing RX fifo\n");
> > +               msg->status = err;
> > +               return;
> > +       }
> > +
> > +       /*
> > +        * 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) {
> > +               cond_resched();
> 
> Why is this necessary?

Well it isn't strictly necessary. This is just one of the places where we can
voluntarily relinguish the cpu for a while. I can remove that if necessary.

> 
> > +               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);
> 
> I'm nervous about the locking.  I'm not convinced that it covers all
> the important cases.  Are you able to describe what the locks protect,
> and why they are taken when they are?  That would help me evaluate
> what the driver should be doing.

Ok. here is a description of the locking model (sorry if this is
bit messy):

Lock should protect following fields:
 	running
 	current_msg
	msg_queue

(rest of the fields are basically only set once, except rx, tx and
fifo_level)

espi->msg_queue needs protection because there might be multiple
threads adding to the queue.

espi->running signals whether the driver is processing messages or
not. It can be changed when module is unloading, hence it needs
protection.

espi->current_msg is used to hold pointer to currently processed
message. This message has been removed from the espi->msg_queue.
It also indicates whether we can proceed with the next message. If
it is NULL we are allowed to process the next message.

Assumption is that when espi->current_msg is not NULL, message is
processed and we won't call ep93xx_spi_process_message(). This
allows us to use espi->rx, espi->tx, and espi->fifo_level without
locks.

Thanks,
MW

> 
> > +
> > +       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);
> > +}
> > +
> > +/**
> > + * 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");
> > +
> > +               spin_lock(&espi->lock);
> > +               espi->current_msg->status = -EIO;
> > +               spin_unlock(&espi->lock);
> > +       } 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->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->info = info;
> > +       espi->pdev = pdev;
> > +
> > +       switch (transfer_method) {
> > +       case EP93XX_SPI_METHOD_POLL:
> > +               espi->transfer = ep93xx_spi_polling_transfer;
> > +               break;
> > +
> > +       case EP93XX_SPI_METHOD_INTERRUPT:
> > +               espi->transfer = ep93xx_spi_interrupt_transfer;
> > +               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;
> > +               }
> > +               break;
> > +
> > +       default:
> > +               dev_err(&pdev->dev, "invalid transfer method %d given\n",
> > +                       transfer_method);
> > +               error = -EINVAL;
> > +               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;
> > +       }
> > +
> > +       if (espi->irq) {
> > +               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)
> > +               goto fail_free_queue;
> > +
> > +       dev_info(&pdev->dev, "EP93xx SPI Controller at 0x%08lx\n",
> > +                (unsigned long)res->start);
> > +       if (espi->irq)
> > +               dev_info(&pdev->dev, "using irq %d\n",  espi->irq);
> > +
> > +       return 0;
> > +
> > +fail_free_queue:
> > +       destroy_workqueue(espi->wq);
> > +fail_free_irq:
> > +       if (espi->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);
> > +
> > +       if (espi->irq)
> > +               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@iki.fi>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:ep93xx-spi");
> > --
> > 1.5.6.5
> >
> >
> 
> 
> 
> -- 
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.

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

[Grant Likely]

On Sat, Apr 17, 2010 at 5:00 AM, Mika Westerberg <mika.westerberg@iki.fi> wrote:
> On Fri, Apr 16, 2010 at 11:30:45PM -0700, Grant Likely wrote:
>> Thanks for the patch.  Comments below.
>
> Thanks for the comments. See below my replies.

>> On Tue, Apr 13, 2010 at 7:10 AM, Mika Westerberg <mika.westerberg@iki.fi> wrote:
>> > +static int transfer_method = EP93XX_SPI_METHOD_INTERRUPT;
>> > +module_param(transfer_method, int, S_IRUGO);
>> > +MODULE_PARM_DESC(transfer_method,
>> > +       "Used transfer method (0=poll, 1=interrupt [default])");
>>
>> Should this really be a module parameter?  Would it be better to
>> expose this knob as part of pdata or in sysfs?  (Assuming there even
>> is a real need to manipulate this control)
>
> Hard to say. At least it was easier to test both modes by just
> loading the module with correct parameter but it could possibly be
> placed in pdata also, if required.

Is it the sort of thing that can/should be changed at runtime (sysfs)?
 Is there the possibility of multiple instances of this device on a
single SoC?  I'm not strictly opposed to these lines, but I ask to
make sure they are the right abstraction.

>> > +       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                            (*transfer)(struct ep93xx_spi *);
>> > +};
>> > +
>> > +/**
>> > + * struct ep93xx_spi_chip - SPI device hardware settings
>> > + * @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)
>> > + * @mode: chip SPI mode (see also &struct spi_device)
>> > + *
>> > + * 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 {
>> > +       unsigned long   rate;
>> > +       u8              div_cpsr;
>> > +       u8              div_scr;
>> > +       u8              dss;
>> > +       u8              mode;
>> > +};
>> > +
>> > +/* 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_flush() - flushes SPI RX FIFO
>> > + * @espi: ep93xx SPI controller struct
>> > + * @msecs: timeout in milliseconds to wait
>> > + *
>> > + * This function flushes RX FIFO. Returns %0 in case of success and %-ETIMEDOUT
>> > + * when timeout occured. Wait is implemented in busylooping so this function can
>> > + * also be called from atomic context.
>> > + */
>> > +static int ep93xx_spi_flush(const struct ep93xx_spi *espi, unsigned long msecs)
>> > +{
>> > +       unsigned long timeout = jiffies + msecs_to_jiffies(msecs);
>> > +
>> > +       while (ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_RNE) {
>> > +               if (time_after(jiffies, timeout))
>> > +                       return -ETIMEDOUT;
>>
>> Ouch.  With the current code the driver could busy wait for 100ms!  If
>> flushing takes time, then don't busywait.  Bail out to some form of
>> deferred work.  Let the kernel get on with something else.
>
> I've never witnessed flushing to take this whole 100ms. Timeout is
> there just to make sure that this ends eventually. I can change it
> to some lower value (say few msecs or something like that).

Even with a lower value, the driver is still busywaiting which is bad
for latency.  If I see a lot of busywaiting, then I look very closely
at refactoring the driver into a state machine that can easily defer
work.

>> > +
>> > +/**
>> > + * 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++) {
>>
>> Non-deterministic.  Big nested loops mean a lot of calculations within
>>  ep93xx_spi_process_transfer, and calculated values aren't cached if
>> they get replaced.  If you can, it would be better if you can
>> algebraically calculate the factors instead of an iterative loop.
>> It's requires more thinking at the outset, but it is worth the effort.
>
> Is it normal to change speed per transfer? ep93xx_spi_process_transfer()
> only calls this if speed changes. That's why I used those loops
> here; the assumption is that it is only called at setup time (and
> even there only when speed changes).
>
> For example in my test system using mmc_spi it was called total 3 times.

But it is a concern if you have two spi devices with different
constraints attached to the same bus.

I'm not going to reject the driver over this, but you should be aware
of it and it probably should be fixed.

>> > +       chip->mode = spi->mode;
>> > +       chip->dss = bits_per_word_to_dss(spi->bits_per_word);
>>
>> Why is mode and dss getting copied into chip?  chip should probable
>> instead have a reference back to its assigned spi device and the
>> original values should be used.
>
> This is done because if there are transfer specific settings (for
> example bits per word changes) we can just make temporary copy of
> chip and change those values and then restore back the original,
> once the transfer is finished (see ep93xx_spi_process_transfer()).
>
> Mode, however is redundant as it cannot be changed per transfer (or
> even per message). I will drop that from the struct.

ok

>> > +       /* read all received data */
>> > +       while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
>> > +               espi->rx < t->len) {
>> > +               ep93xx_do_read(espi, t);
>> > +               espi->fifo_level--;
>> > +       }
>>
>> Another busywait loop.  Could end up burning an awful lot of cycles
>> here.  A state machine might be more suitable for this driver so that
>> processing can be punted to deferred work when the FIFO gets either
>> empty or full.
>
> FIFO size is max 8 frames so there should be 8 iterations when the
> FIFO is full. So is it enought to to add check that we only read
> max 8 frames from the FIFO?

If you do it right, FIFO size shouldn't matter.  The driver should
easily be able to read however many are available and defer until more
is ready.

>
>> > +
>> > +       /* write as long as FIFO has room */
>> > +       while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
>> > +               ep93xx_do_write(espi, t);
>> > +               espi->fifo_level++;
>> > +       }
>> > +
>> > +       /* are we done yet? */
>> > +       if (espi->tx == t->len && espi->rx == t->len) {
>> > +               msg->actual_length += t->len;
>> > +               return t->len;
>> > +       }
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +/**
>> > + * ep93xx_spi_polling_transfer() - implement polling mode transfer
>> > + * @espi: ep93xx SPI controller struct
>> > + *
>> > + * Function performs one polling mode transfer. When this function is finished,
>> > + * the whole transfer should be completed, or failed in case of error.
>> > + */
>> > +static void ep93xx_spi_polling_transfer(struct ep93xx_spi *espi)
>> > +{
>> > +       while (!ep93xx_spi_read_write(espi))
>> > +               cpu_relax();
>>
>> And if it never finishes?  No exit path on error.
>
> Well with current implementation of ep93xx_spi_read_write() it
> cannot fail.  I guess some sort of timeout could be used here. Also
> we can take ROR (receive overrun) interrupt and report that back.

In cases like this, if you've got a guaranteed no-fail path, then a
comment stating explicitly that is helpful to the reader.  :-)

>> > +       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)
>> > +{
>> > +       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.
>> > +        */
>> > +       err = ep93xx_spi_flush(espi, SPI_TIMEOUT);
>> > +       if (err) {
>> > +               dev_warn(&espi->pdev->dev, "timeout while flushing RX fifo\n");
>> > +               msg->status = err;
>> > +               return;
>> > +       }
>> > +
>> > +       /*
>> > +        * 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) {
>> > +               cond_resched();
>>
>> Why is this necessary?
>
> Well it isn't strictly necessary. This is just one of the places where we can
> voluntarily relinguish the cpu for a while. I can remove that if necessary.

I'd remove it.  There aren't very many places where the driver would
legitimately want to do this, especially if you reduce the
busywaiting.  Trust the scheduler to make sure you're process doesn't
get too much time.

>> > +       list_del_init(&msg->queue);
>> > +       espi->current_msg = msg;
>> > +       spin_unlock_irq(&espi->lock);
>>
>> I'm nervous about the locking.  I'm not convinced that it covers all
>> the important cases.  Are you able to describe what the locks protect,
>> and why they are taken when they are?  That would help me evaluate
>> what the driver should be doing.
>
> Ok. here is a description of the locking model (sorry if this is
> bit messy):
>
> Lock should protect following fields:
>        running
>        current_msg
>        msg_queue
>
> (rest of the fields are basically only set once, except rx, tx and
> fifo_level)
>
> espi->msg_queue needs protection because there might be multiple
> threads adding to the queue.
>
> espi->running signals whether the driver is processing messages or
> not. It can be changed when module is unloading, hence it needs
> protection.
>
> espi->current_msg is used to hold pointer to currently processed
> message. This message has been removed from the espi->msg_queue.
> It also indicates whether we can proceed with the next message. If
> it is NULL we are allowed to process the next message.
>
> Assumption is that when espi->current_msg is not NULL, message is
> processed and we won't call ep93xx_spi_process_message(). This
> allows us to use espi->rx, espi->tx, and espi->fifo_level without
> locks.

So you need to make sure that the lock is held over the entire region
between checking ->running, checking ->current_msg, and actually
modifying the queue or setting current_msg, right?

Cheers,
g.

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

[Martin Guy]

On 4/19/10, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>  >> > +static int ep93xx_spi_flush(const struct ep93xx_spi *espi, unsigned long msecs)
>  >> > +{
>  >> > +       unsigned long timeout = jiffies + msecs_to_jiffies(msecs);
>  >> > +
>  >> > +       while (ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_RNE) {
>  >> > +               if (time_after(jiffies, timeout))
>  >> > +                       return -ETIMEDOUT;
>  >>
>  >> Ouch.  With the current code the driver could busy wait for 100ms!  If
>  >> flushing takes time, then don't busywait.  Bail out to some form of
>  >> deferred work.  Let the kernel get on with something else.
>  >
>  > I've never witnessed flushing to take this whole 100ms. Timeout is
>  > there just to make sure that this ends eventually. I can change it
>  > to some lower value (say few msecs or something like that).
>
>  Even with a lower value, the driver is still busywaiting which is bad
>  for latency.  If I see a lot of busywaiting, then I look very closely
>  at refactoring the driver into a state machine that can easily defer
>  work.

This function and all busywaiting should disappear now that the
interrupt-driven mode only interrupts once per four words instead of
doing a whole block transfer inside a single interrupts call.

>  >> > + * ep93xx_spi_calc_divisors() - calculates SPI clock divisors
>  >> > +
>  >> > +       /*
>  >> > +        * 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++) {
>  >>
>  >> Non-deterministic.
Eh? Some new meaning of "non-deterministic" that I wasn'f familiar with? :)

Yes, it does look gross, but max clock rate is 3.7Mhz (7.4 in rev E2
silicon), so if your slowest device is 400kHz, in practice it only
runs the inner loop from 0 to less than 10.
How slow do SPI devices go in reality? The 254*256 divisor gives 100Hz.

>  >> algebraically calculate the factors instead of an iterative loop.

>  > FIFO size is max 8 frames so there should be 8 iterations when the
>  > FIFO is full. So is it enought to to add check that we only read
>  > max 8 frames from the FIFO?
>
>  If you do it right, FIFO size shouldn't matter.  The driver should
>  easily be able to read however many are available and defer until more
>  is ready.

Again, with the new interrupt-handling code you have to know the FIFO
size because if you regulate the TX filling with the FIFO_NOT_FULL_YET
status bit you end up with 8 words in the TX FIFO plus one word being
transmitted already and that makes 9, which can  overflow the receive
buffer (I've seen this happening in practice).

    M

------------------------------------------------------------------------------
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 v3 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Grant Likely]

On Mon, Apr 19, 2010 at 12:52 PM, Martin Guy <martinwguy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 4/19/10, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>>  >> > +       /*
>>  >> > +        * 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++) {
>>  >>
>>  >> Non-deterministic.
> Eh? Some new meaning of "non-deterministic" that I wasn'f familiar with? :)
>
> Yes, it does look gross, but max clock rate is 3.7Mhz (7.4 in rev E2
> silicon), so if your slowest device is 400kHz, in practice it only
> runs the inner loop from 0 to less than 10.
> How slow do SPI devices go in reality? The 254*256 divisor gives 100Hz.

Fair enough.  s/non-deterministic/gross/  :-p  I just prefer to see
analytic solutions over numerical ones.  :-)  As I said, I won't
object to the driver because of these lines.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

------------------------------------------------------------------------------
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 v3 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/19/10, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>  >>  >> > +       for (cpsr = 2; cpsr <= 254; cpsr += 2) {
>  >>  >> > +               for (scr = 0; scr <= 255; scr++) {
>  >>  >>
>  >>  >> Non-deterministic.
>  > Eh? Some new meaning of "non-deterministic" that I wasn'f familiar with? :)
>  >
>  > Yes, it does look gross, but max clock rate is 3.7Mhz (7.4 in rev E2
>  > silicon), so if your slowest device is 400kHz, in practice it only
>  > runs the inner loop from 0 to less than 10.
>  > How slow do SPI devices go in reality? The 254*256 divisor gives 100Hz.
>
> Fair enough.  s/non-deterministic/gross/  :-p  I just prefer to see
>  analytic solutions over numerical ones.  :-)  As I said, I won't
>  object to the driver because of these lines.

Absolutely :D
Hey, that's not my code, that comes from Cirrus' code..

Analytical solutions are nicer, yes.
Especially when they get the answer right.
I can see that that loop gets the answer right, whereas once we go
into integer division it is very easy to write code that looks right
but which gets the answer out by one

More to the point, they're probably faster if the calculation happns
once per transfer.
Only "probably" - this chip doesn't have a divide instruction, so
integer division causes a function call to a soft math routine. Would
you like to suggest a code fragment that achieves the desired effect
using only + - * and logical operators and that you have tested in a
loop from 1 to N against the "stupid" version?

So my question stands "what is the slowest SPI device", to know
whether it is worth worrying about, and whether I want to spend a
morning writing an analytical method that is both actually faster and
is sure get the answer right. Otherwise we end up driving devices at
the wrong clock rate.

    M

------------------------------------------------------------------------------
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 v3 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/19/10, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>  >> > +       /* read all received data */
>  >> > +       while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
>  >> > +               espi->rx < t->len) {
>  >> > +               ep93xx_do_read(espi, t);
>  >> > +               espi->fifo_level--;
>  >> > +       }
>  >>
>  >> Another busywait loop.  Could end up burning an awful lot of cycles
>  >> here.  A state machine might be more suitable for this driver so that
>  >> processing can be punted to deferred work when the FIFO gets either
>  >> empty or full.
>  >
>  > FIFO size is max 8 frames so there should be 8 iterations when the
>  > FIFO is full. So is it enought to to add check that we only read
>  > max 8 frames from the FIFO?
>
>  If you do it right, FIFO size shouldn't matter.  The driver should
>  easily be able to read however many are available and defer until more
>  is ready.

Sorry, I completely missed the point at the end of the last post.

This is not a busywait loop. It takes bytes out of the RX FIFO until
it is empty, which is all useful work. There is also no way to tell
just from our internal variables how many bytes are in the RX FIFO as
far as I know, so the only way is to check the Receiver-Not-Empty
status bit (which is what this code does).

However, the
>  >> > +               espi->rx < t->len) {
is redundant, since it will always be true when the first condition is
true, though the compiler cannot see optimise it out.

If you're afraid that a caller might pass the driver a receive buffer
shorter than than the transmit buffer, it would be better to check
that once when the transfer is requested, not once per byte received!

The same goes for the other checks for conditions that can only ever
be true or only ever false that I mentioned in my remarks on earlier
patches, and other optimizations. We are at the byte-per-byte level of
a device driver for a slow CPU, where efficiency is at a premium.

     M

------------------------------------------------------------------------------
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 v3 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Grant Likely]

On Mon, Apr 19, 2010 at 1:16 PM, Martin Guy <martinwguy@gmail.com> wrote:
> On 4/19/10, Grant Likely <grant.likely@secretlab.ca> wrote:
>>  >> > +       /* read all received data */
>>  >> > +       while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
>>  >> > +               espi->rx < t->len) {
>>  >> > +               ep93xx_do_read(espi, t);
>>  >> > +               espi->fifo_level--;
>>  >> > +       }
>>  >>
>>  >> Another busywait loop.  Could end up burning an awful lot of cycles
>>  >> here.  A state machine might be more suitable for this driver so that
>>  >> processing can be punted to deferred work when the FIFO gets either
>>  >> empty or full.
>>  >
>>  > FIFO size is max 8 frames so there should be 8 iterations when the
>>  > FIFO is full. So is it enought to to add check that we only read
>>  > max 8 frames from the FIFO?
>>
>>  If you do it right, FIFO size shouldn't matter.  The driver should
>>  easily be able to read however many are available and defer until more
>>  is ready.
>
> Sorry, I completely missed the point at the end of the last post.
>
> This is not a busywait loop. It takes bytes out of the RX FIFO until
> it is empty, which is all useful work. There is also no way to tell
> just from our internal variables how many bytes are in the RX FIFO as
> far as I know, so the only way is to check the Receiver-Not-Empty
> status bit (which is what this code does).

Yeah, you're right.  I was scanning through the code too quickly and
jumped to conclusions without actually checking.  Sorry about the
noise.

g.

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

[Mika Westerberg]

On Mon, Apr 19, 2010 at 12:02:39PM -0600, Grant Likely wrote:
> On Sat, Apr 17, 2010 at 5:00 AM, Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org> wrote:
> > On Fri, Apr 16, 2010 at 11:30:45PM -0700, Grant Likely wrote:
> >> Thanks for the patch.  Comments below.
> >
> > Thanks for the comments. See below my replies.
> 
> >> On Tue, Apr 13, 2010 at 7:10 AM, Mika Westerberg <mika.westerberg@iki.fi> wrote:
> >> > +static int transfer_method = EP93XX_SPI_METHOD_INTERRUPT;
> >> > +module_param(transfer_method, int, S_IRUGO);
> >> > +MODULE_PARM_DESC(transfer_method,
> >> > +       "Used transfer method (0=poll, 1=interrupt [default])");
> >>
> >> Should this really be a module parameter?  Would it be better to
> >> expose this knob as part of pdata or in sysfs?  (Assuming there even
> >> is a real need to manipulate this control)
> >
> > Hard to say. At least it was easier to test both modes by just
> > loading the module with correct parameter but it could possibly be
> > placed in pdata also, if required.
> 
> Is it the sort of thing that can/should be changed at runtime (sysfs)?
>  Is there the possibility of multiple instances of this device on a
> single SoC?  I'm not strictly opposed to these lines, but I ask to
> make sure they are the right abstraction.

I've actually decided to drop the whole polling mode.  So this
parameter will be gone in next revision.

> 
> >> > +       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                            (*transfer)(struct ep93xx_spi *);
> >> > +};
> >> > +
> >> > +/**
> >> > + * struct ep93xx_spi_chip - SPI device hardware settings
> >> > + * @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)
> >> > + * @mode: chip SPI mode (see also &struct spi_device)
> >> > + *
> >> > + * 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 {
> >> > +       unsigned long   rate;
> >> > +       u8              div_cpsr;
> >> > +       u8              div_scr;
> >> > +       u8              dss;
> >> > +       u8              mode;
> >> > +};
> >> > +
> >> > +/* 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_flush() - flushes SPI RX FIFO
> >> > + * @espi: ep93xx SPI controller struct
> >> > + * @msecs: timeout in milliseconds to wait
> >> > + *
> >> > + * This function flushes RX FIFO. Returns %0 in case of success and %-ETIMEDOUT
> >> > + * when timeout occured. Wait is implemented in busylooping so this function can
> >> > + * also be called from atomic context.
> >> > + */
> >> > +static int ep93xx_spi_flush(const struct ep93xx_spi *espi, unsigned long msecs)
> >> > +{
> >> > +       unsigned long timeout = jiffies + msecs_to_jiffies(msecs);
> >> > +
> >> > +       while (ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_RNE) {
> >> > +               if (time_after(jiffies, timeout))
> >> > +                       return -ETIMEDOUT;
> >>
> >> Ouch.  With the current code the driver could busy wait for 100ms!  If
> >> flushing takes time, then don't busywait.  Bail out to some form of
> >> deferred work.  Let the kernel get on with something else.
> >
> > I've never witnessed flushing to take this whole 100ms. Timeout is
> > there just to make sure that this ends eventually. I can change it
> > to some lower value (say few msecs or something like that).
> 
> Even with a lower value, the driver is still busywaiting which is bad
> for latency.  If I see a lot of busywaiting, then I look very closely
> at refactoring the driver into a state machine that can easily defer
> work.

RNE (RX FIFO not empty) bit is set only when there is stuff in the
FIFO. So it should be safe to read from. In normal cases FIFO is
just cleared and no timeout happens. It shouldn't busywait because
for every iteration it reads next frame from the FIFO.

Timeout was added there just to make sure that if the controller
goes mad we don't hang there forever.

But if you can explain little more detail how to implement this with
a state machine which can defer work I can implement that then.

> >> > +
> >> > +/**
> >> > + * 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++) {
> >>
> >> Non-deterministic.  Big nested loops mean a lot of calculations within
> >>  ep93xx_spi_process_transfer, and calculated values aren't cached if
> >> they get replaced.  If you can, it would be better if you can
> >> algebraically calculate the factors instead of an iterative loop.
> >> It's requires more thinking at the outset, but it is worth the effort.
> >
> > Is it normal to change speed per transfer? ep93xx_spi_process_transfer()
> > only calls this if speed changes. That's why I used those loops
> > here; the assumption is that it is only called at setup time (and
> > even there only when speed changes).
> >
> > For example in my test system using mmc_spi it was called total 3 times.
> 
> But it is a concern if you have two spi devices with different
> constraints attached to the same bus.
> 
> I'm not going to reject the driver over this, but you should be aware
> of it and it probably should be fixed.

OK.

> 
> >> > +       chip->mode = spi->mode;
> >> > +       chip->dss = bits_per_word_to_dss(spi->bits_per_word);
> >>
> >> Why is mode and dss getting copied into chip?  chip should probable
> >> instead have a reference back to its assigned spi device and the
> >> original values should be used.
> >
> > This is done because if there are transfer specific settings (for
> > example bits per word changes) we can just make temporary copy of
> > chip and change those values and then restore back the original,
> > once the transfer is finished (see ep93xx_spi_process_transfer()).
> >
> > Mode, however is redundant as it cannot be changed per transfer (or
> > even per message). I will drop that from the struct.
> 
> ok
> 
> >> > +       /* read all received data */
> >> > +       while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
> >> > +               espi->rx < t->len) {
> >> > +               ep93xx_do_read(espi, t);
> >> > +               espi->fifo_level--;
> >> > +       }
> >>
> >> Another busywait loop.  Could end up burning an awful lot of cycles
> >> here.  A state machine might be more suitable for this driver so that
> >> processing can be punted to deferred work when the FIFO gets either
> >> empty or full.
> >
> > FIFO size is max 8 frames so there should be 8 iterations when the
> > FIFO is full. So is it enought to to add check that we only read
> > max 8 frames from the FIFO?
> 
> If you do it right, FIFO size shouldn't matter.  The driver should
> easily be able to read however many are available and defer until more
> is ready.

Agreed. There is no need for FIFO size checking in this particular loop.

Checking the RNE bit is enough. The driver reads as many frames as there
are available.

I'll also remove those useless checks as Martin commented in his previous
mail.

> >
> >> > +
> >> > +       /* write as long as FIFO has room */
> >> > +       while (espi->fifo_level < SPI_FIFO_SIZE && espi->tx < t->len) {
> >> > +               ep93xx_do_write(espi, t);
> >> > +               espi->fifo_level++;
> >> > +       }
> >> > +
> >> > +       /* are we done yet? */
> >> > +       if (espi->tx == t->len && espi->rx == t->len) {
> >> > +               msg->actual_length += t->len;
> >> > +               return t->len;
> >> > +       }
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +/**
> >> > + * ep93xx_spi_polling_transfer() - implement polling mode transfer
> >> > + * @espi: ep93xx SPI controller struct
> >> > + *
> >> > + * Function performs one polling mode transfer. When this function is finished,
> >> > + * the whole transfer should be completed, or failed in case of error.
> >> > + */
> >> > +static void ep93xx_spi_polling_transfer(struct ep93xx_spi *espi)
> >> > +{
> >> > +       while (!ep93xx_spi_read_write(espi))
> >> > +               cpu_relax();
> >>
> >> And if it never finishes?  No exit path on error.
> >
> > Well with current implementation of ep93xx_spi_read_write() it
> > cannot fail.  I guess some sort of timeout could be used here. Also
> > we can take ROR (receive overrun) interrupt and report that back.
> 
> In cases like this, if you've got a guaranteed no-fail path, then a
> comment stating explicitly that is helpful to the reader.  :-)

Yeah sorry about that.

I'll drop the polling mode so this function is gone in next revision.

> 
> >> > +       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)
> >> > +{
> >> > +       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.
> >> > +        */
> >> > +       err = ep93xx_spi_flush(espi, SPI_TIMEOUT);
> >> > +       if (err) {
> >> > +               dev_warn(&espi->pdev->dev, "timeout while flushing RX fifo\n");
> >> > +               msg->status = err;
> >> > +               return;
> >> > +       }
> >> > +
> >> > +       /*
> >> > +        * 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) {
> >> > +               cond_resched();
> >>
> >> Why is this necessary?
> >
> > Well it isn't strictly necessary. This is just one of the places where we can
> > voluntarily relinguish the cpu for a while. I can remove that if necessary.
> 
> I'd remove it.  There aren't very many places where the driver would
> legitimately want to do this, especially if you reduce the
> busywaiting.  Trust the scheduler to make sure you're process doesn't
> get too much time.

OK.

> 
> >> > +       list_del_init(&msg->queue);
> >> > +       espi->current_msg = msg;
> >> > +       spin_unlock_irq(&espi->lock);
> >>
> >> I'm nervous about the locking.  I'm not convinced that it covers all
> >> the important cases.  Are you able to describe what the locks protect,
> >> and why they are taken when they are?  That would help me evaluate
> >> what the driver should be doing.
> >
> > Ok. here is a description of the locking model (sorry if this is
> > bit messy):
> >
> > Lock should protect following fields:
> >        running
> >        current_msg
> >        msg_queue
> >
> > (rest of the fields are basically only set once, except rx, tx and
> > fifo_level)
> >
> > espi->msg_queue needs protection because there might be multiple
> > threads adding to the queue.
> >
> > espi->running signals whether the driver is processing messages or
> > not. It can be changed when module is unloading, hence it needs
> > protection.
> >
> > espi->current_msg is used to hold pointer to currently processed
> > message. This message has been removed from the espi->msg_queue.
> > It also indicates whether we can proceed with the next message. If
> > it is NULL we are allowed to process the next message.
> >
> > Assumption is that when espi->current_msg is not NULL, message is
> > processed and we won't call ep93xx_spi_process_message(). This
> > allows us to use espi->rx, espi->tx, and espi->fifo_level without
> > locks.
> 
> So you need to make sure that the lock is held over the entire region
> between checking ->running, checking ->current_msg, and actually
> modifying the queue or setting current_msg, right?

Right. I think it is currently like that, no?

Thank you very much for your comments. I'll try to post next revision
later today.

Regards,
MW

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

[PATCH v3 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                     |   42 +++++++++++++++++++++++
 arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h |    1 +
 arch/arm/mach-ep93xx/include/mach/platform.h    |    2 +
 4 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index 5f80092..daaa799 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 / 2,
+};
 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..08b7747 100644
--- a/arch/arm/mach-ep93xx/core.c
+++ b/arch/arm/mach-ep93xx/core.c
@@ -363,6 +363,48 @@ void __init ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr)
 	platform_device_register(&ep93xx_eth_device);
 }
 
+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,
+	.num_resources	= ARRAY_SIZE(ep93xx_spi_resources),
+	.resource	= ep93xx_spi_resources,
+};
+
+/**
+ * ep93xx_register_spi() - registers spi platform device
+ * @info: ep93xx board specific spi info
+ *
+ * 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_device.dev.platform_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 v3 2/2] ep93xx: SPI driver platform support code

[H Hartley Sweeten]

On Tuesday, April 13, 2010 7:10 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>
> ---
>  arch/arm/mach-ep93xx/clock.c                    |   14 +++++++
>  arch/arm/mach-ep93xx/core.c                     |   42 +++++++++++++++++++++++
>  arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h |    1 +
>  arch/arm/mach-ep93xx/include/mach/platform.h    |    2 +
>  4 files changed, 59 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
> index 5f80092..daaa799 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 / 2,

Please change to:

	.rate		= EP93XX_EXT_CLK_RATE,

Under the assumption that all newer systems use Rev E2 silicon.

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

That makes this:

	if (ep93xx_chip_revision() < EP93XX_CHIP_REV_E2)
		clk_spi.rate /= 2;

This just makes sure there are no rounding errors due to the / then * in case
someone has a goofy system and changes the definition of EP93XX_EXT_CLK_RATE.

> +
>  	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..08b7747 100644
> --- a/arch/arm/mach-ep93xx/core.c
> +++ b/arch/arm/mach-ep93xx/core.c
> @@ -363,6 +363,48 @@ void __init ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr)
>  	platform_device_register(&ep93xx_eth_device);
>  }

Please add:

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,

And add:

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

Change to:

* @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_device.dev.platform_data = info;

Then change this to:

	ep93xx_spi_master_data = *info;

The platform init should then declare the 'info' as __initdata.

This makes sure that the data in the platform init is discarded after startup
is complete.

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

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 v3 0/2] spi: driver for Cirrus EP93xx SPI controller

[H Hartley Sweeten]

On Tuesday, April 13, 2010 7:10 AM, Mika Westerberg wrote:
> Hello,
> 
> This is third revision of the driver. Thanks to Martin Guy who tested and
> reviewed the code.
> 
> Changes since v2:
> 	- corrected spi clock rate calculation
> 	- interrupt handling is now more efficient
> 	- driver now supports polling mode as well, this can be selected with
> 	  'transfer_method' module parameter.
> 	- controller is disabled in probe function
> 	- some cosmetic changes
> 
> I have been testing this on my TS-7260 board (ep9302 based) 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
> 

Hello Mika,

I finally was able to get this working on my system.  Not sure what the issue
was earlier.

The only thing I don't like right off hand is the chip select handling.

The way it currently is done you are limited to using only the built-in GPIO's
of the EP93xx.  This prohibits a system from using an external i2c/spi/etc.
gpio expander to provide more chip selects.  And if a platform is not setup
correctly, the BUG_ON in cs_to_gpio in your example for the tx72xx is pretty
nasty.

Ryan and I worked out a runtime setup/cleanup for the spi device chip selects
in the spi driver I have in my tree.  I will take a look at it and see how
much trouble it will be to implement in your driver.

Anyway, now that I have your driver functioning I will be able to actually
provide some feedback to you.

Regards,
Hartley
------------------------------------------------------------------------------
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

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

[Mika Westerberg]

On Fri, Apr 16, 2010 at 01:28:05PM -0500, H Hartley Sweeten wrote:
> 
> I finally was able to get this working on my system.  Not sure what the issue
> was earlier.

Great :)

> The only thing I don't like right off hand is the chip select handling.
> 
> The way it currently is done you are limited to using only the built-in GPIO's
> of the EP93xx.  This prohibits a system from using an external i2c/spi/etc.
> gpio expander to provide more chip selects.  And if a platform is not setup
> correctly, the BUG_ON in cs_to_gpio in your example for the tx72xx is pretty
> nasty.

Yes the example was just a hack.

However, I'm not sure what you mean by limited only to built-in GPIOs? Currently
the driver does:

        info->cs_control(spi->chip_select, value, info->data);

when it wants to assert/deassert the chip select. I don't see how this is
limited to built-in GPIOs only (maybe I'm missing something). Now it is in
responsibility of platform board files to allocate necessary chipselect lines,
and translate 'spi->chip_select' and 'value' to something meaningful.

> Ryan and I worked out a runtime setup/cleanup for the spi device chip selects
> in the spi driver I have in my tree.  I will take a look at it and see how
> much trouble it will be to implement in your driver.

Ok.

> Anyway, now that I have your driver functioning I will be able to actually
> provide some feedback to you.

That sounds good.

Thanks,
MW

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

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

[Martin Guy]

(sorry for the duplicate, Mika, I forgot to include the list)

On 4/17/10, Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org> wrote:
>  However, I'm not sure what you mean by limited only to built-in GPIOs? Currently
>  the driver does:
>
>         info->cs_control(spi->chip_select, value, info->data);
>
>  when it wants to assert/deassert the chip select.

There's a further strangeness when a board has a single SPI device not
using any GPIO chip select. At present, clients seem to need to
declare that they have 1 chip select and to provide an empty
cs_control function that does nothing. For example:

static void null_cs_control(unsigned cs, unsigned value, void *data) {
       /* Sim.One only has MMC card and no GPIO chip select logic */
}

static struct ep93xx_spi_info simone_spi_info = {
       .num_chipselect = 1,
       .cs_control     = null_cs_control,
       .data           = NULL,
};
...
static void __init simone_init_machine(void)
{
...
       spi_register_board_info(simone_spi_board_info,
ARRAY_SIZE(simone_spi_board_info));
       ep93xx_register_spi(&simone_spi_info);
...

Allowing cs_control to be NULL would do away with the empty function, but
when I just tried with num_chipselect = 0 the device did not appear. There
may be a better solution.

   M

------------------------------------------------------------------------------
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 v3 0/2] spi: driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Sat, Apr 17, 2010 at 07:43:23PM +0100, Martin Guy wrote:
> 
> There's a further strangeness when a board has a single SPI device not
> using any GPIO chip select. At present, clients seem to need to
> declare that they have 1 chip select and to provide an empty
> cs_control function that does nothing. For example:
> 
> static void null_cs_control(unsigned cs, unsigned value, void *data) {
>        /* Sim.One only has MMC card and no GPIO chip select logic */
> }
> 
> static struct ep93xx_spi_info simone_spi_info = {
>        .num_chipselect = 1,
>        .cs_control     = null_cs_control,
>        .data           = NULL,
> };
> ...
> static void __init simone_init_machine(void)
> {
> ...
>        spi_register_board_info(simone_spi_board_info,
> ARRAY_SIZE(simone_spi_board_info));
>        ep93xx_register_spi(&simone_spi_info);
> ...
> 
> Allowing cs_control to be NULL would do away with the empty function, but
> when I just tried with num_chipselect = 0 the device did not appear. There
> may be a better solution.

There is a check in spi_master_register() where num_chipselect should be at
least 1. This is probably why it failed in your case.

However, I can add check to the driver that NULL cs_control is ok.

Thanks,
MW

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

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

[H Hartley Sweeten]

On Friday, April 16, 2010 9:59 PM, Mika Westerberg wrote:
> On Fri, Apr 16, 2010 at 01:28:05PM -0500, H Hartley Sweeten wrote:
>> 
>> I finally was able to get this working on my system.  Not sure what the issue
>> was earlier.
>
> Great :)
>
>> The only thing I don't like right off hand is the chip select handling.
>> 
>> The way it currently is done you are limited to using only the built-in GPIO's
>> of the EP93xx.  This prohibits a system from using an external i2c/spi/etc.
>> gpio expander to provide more chip selects.  And if a platform is not setup
>> correctly, the BUG_ON in cs_to_gpio in your example for the tx72xx is pretty
>> nasty.
>
> Yes the example was just a hack.
>
> However, I'm not sure what you mean by limited only to built-in GPIOs? Currently
> the driver does:
>
>        info->cs_control(spi->chip_select, value, info->data);
>
> when it wants to assert/deassert the chip select. I don't see how this is
> limited to built-in GPIOs only (maybe I'm missing something). Now it is in
> responsibility of platform board files to allocate necessary chipselect lines,
> and translate 'spi->chip_select' and 'value' to something meaningful.

The way you are currently handling the chip select limits you to the built-in
GPIO's because the 'gpio_request' happens early in the platform init before any
external gpio expanders might be available.

The best example is if your first SPI device is actually a GPIO expander that
is chip selected by a built-in GPIO.  This GPIO expander is then used to provide
the chip selects for additional SPI devices on the bus.

>> Ryan and I worked out a runtime setup/cleanup for the spi device chip selects
>> in the spi driver I have in my tree.  I will take a look at it and see how
>> much trouble it will be to implement in your driver.
>
> Ok.

I have a patch almost working.

Have you had time to implement any of the changes that Grant Likely proposed?
One of them dealt with the ep93xx_spi_{de}select_device functions.  Before I
post my proposed changes I would like to make sure it is based on your most
current code.

BTW, my patch will also handle Martin Guy's issue with using a null chip select.
Actually, the chip-select isn't really null since the SFRMOUT line is still
asserted (active-low) during any SPI transfer but the effect is still the same.

>> Anyway, now that I have your driver functioning I will be able to actually
>> provide some feedback to you.
>
> That sounds good.

Regards,
Hartley
------------------------------------------------------------------------------
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

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

[Mika Westerberg]

On Mon, Apr 19, 2010 at 12:04:30PM -0500, H Hartley Sweeten wrote:
> On Friday, April 16, 2010 9:59 PM, Mika Westerberg wrote:
...
> > when it wants to assert/deassert the chip select. I don't see how this is
> > limited to built-in GPIOs only (maybe I'm missing something). Now it is in
> > responsibility of platform board files to allocate necessary chipselect lines,
> > and translate 'spi->chip_select' and 'value' to something meaningful.
> 
> The way you are currently handling the chip select limits you to the built-in
> GPIO's because the 'gpio_request' happens early in the platform init before any
> external gpio expanders might be available.
> 
> The best example is if your first SPI device is actually a GPIO expander that
> is chip selected by a built-in GPIO.  This GPIO expander is then used to provide
> the chip selects for additional SPI devices on the bus.

Ok.. got it. I didn't consider that one.

> >> Ryan and I worked out a runtime setup/cleanup for the spi device chip selects
> >> in the spi driver I have in my tree.  I will take a look at it and see how
> >> much trouble it will be to implement in your driver.
> >
> > Ok.
> 
> I have a patch almost working.
> 
> Have you had time to implement any of the changes that Grant Likely proposed?

Mostly yes. I'm still considering whether I should drop the polling mode.

> One of them dealt with the ep93xx_spi_{de}select_device functions.  Before I
> post my proposed changes I would like to make sure it is based on your most
> current code.

I changed chipselect functions to be something like (espi->cs_control is copied
from info structure):

        if (espi->cs_control)
                espi->cs_control(spi->chip_select, !!(spi->mode & SPI_CS_HIGH),
                                 espi->cs_data);

I'm not exactly sure whether this is what Grant meant, however.

> BTW, my patch will also handle Martin Guy's issue with using a null chip select.

Yeah. I also have that fixed.

Do you want me to send next revision so that you can base your proposed changes
on top of that or how to proceed?

> Actually, the chip-select isn't really null since the SFRMOUT line is still
> asserted (active-low) during any SPI transfer but the effect is still the same.

Too bad that SFRMOUT cannot be used as GPIO, we could then use it as proper
software controlled chipselect ;)

Thanks,
MW

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

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

[H Hartley Sweeten]

On Monday, April 19, 2010 10:45 AM, Mika Westerberg wrote:
> On Mon, Apr 19, 2010 at 12:04:30PM -0500, H Hartley Sweeten wrote:
>> On Friday, April 16, 2010 9:59 PM, Mika Westerberg wrote:
>...
>>> when it wants to assert/deassert the chip select. I don't see how this is
>>> limited to built-in GPIOs only (maybe I'm missing something). Now it is in
>>> responsibility of platform board files to allocate necessary chipselect lines,
>>> and translate 'spi->chip_select' and 'value' to something meaningful.
>> 
>> The way you are currently handling the chip select limits you to the built-in
>> GPIO's because the 'gpio_request' happens early in the platform init before any
>> external gpio expanders might be available.
>> 
>> The best example is if your first SPI device is actually a GPIO expander that
>> is chip selected by a built-in GPIO.  This GPIO expander is then used to provide
>> the chip selects for additional SPI devices on the bus.
>
> Ok.. got it. I didn't consider that one.

I didn't originally either... Ryan pointed it out. ;-)

>>>> Ryan and I worked out a runtime setup/cleanup for the spi device chip selects
>>>> in the spi driver I have in my tree.  I will take a look at it and see how
>>>> much trouble it will be to implement in your driver.
>>>
>>> Ok.
>>
>> I have a patch almost working.
>> 
>> Have you had time to implement any of the changes that Grant Likely proposed?
>
> Mostly yes. I'm still considering whether I should drop the polling mode.

If interrupt support works well, and it seems to, the polling mode is really not
needed.  I would say drop it.

>> One of them dealt with the ep93xx_spi_{de}select_device functions.  Before I
>> post my proposed changes I would like to make sure it is based on your most
>> current code.
>
> I changed chipselect functions to be something like (espi->cs_control is copied
> from info structure):
>
>        if (espi->cs_control)
>                espi->cs_control(spi->chip_select, !!(spi->mode & SPI_CS_HIGH),
>                                 espi->cs_data);
>
> I'm not exactly sure whether this is what Grant meant, however.

I think you meant 'info->data' not 'espi->cs_data'.

Anyway, I think that is basically what Grant meant.  But, I would change the
cs_control to:

void (*cs_control)(struct spi_device *spi, int value);

The 'chip_select' can be extracted in the cs_control function directly from the
'spi' data.  And, I can't see a real use for the 'info->data' field, your current 
patchset doesn't use that field anyway.

>> BTW, my patch will also handle Martin Guy's issue with using a null chip select.
>
> Yeah. I also have that fixed.
>
> Do you want me to send next revision so that you can base your proposed changes
> on top of that or how to proceed?

When you have all the other comments covered please resubmit the patchset.  I will
base my proposed changes on that.

>> Actually, the chip-select isn't really null since the SFRMOUT line is still
>> asserted (active-low) during any SPI transfer but the effect is still the same.
>
> Too bad that SFRMOUT cannot be used as GPIO, we could then use it as proper
> software controlled chipselect ;)

Probably limited foresight from the Cirrus people...  They must have assumed that
only one SPI device would be connected to the ep93xx.  Actually, the edb93xx
designs use the SFRMOUT to gate the GPIO chip select to the device.

Also, remember that the ep39xx SSP peripheral can be used in three different modes.
For Motorola SPI and National Semiconductor Microwire formats the SFRMOUT is basically
an active-low chip select.  But for Texas Instruments synchronous serial frame format
the pin is pulsed for one serial clock period prior to the transmission of each frame.

Regards,
Hartley
------------------------------------------------------------------------------
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