[PATCH v2 0/3] spi: driver for Cirrus EP93xx SPI controller

[PATCH v2 0/3] spi: driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

Hello,

This is second, updated revision of the series. I tried to address all the
review comments given by H Hartley Sweeten, Ryan Mallon and Martin Guy.

I also tested these again with my TS-7260 board (it has EP9302 chip) using at25
and mmc_spi drivers.

Please review.

Thanks,
MW

Mika Westerberg (3):
  spi: implemented driver for Cirrus EP93xx SPI controller
  ep93xx: added chip revision reading function
  ep93xx: SPI driver platform support code

 arch/arm/mach-ep93xx/clock.c                    |   14 +
 arch/arm/mach-ep93xx/core.c                     |   56 ++
 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    |   10 +
 drivers/spi/Kconfig                             |   11 +
 drivers/spi/Makefile                            |    1 +
 drivers/spi/ep93xx_spi.c                        | 1063 +++++++++++++++++++++++
 8 files changed, 1190 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h
 create mode 100644 drivers/spi/ep93xx_spi.c

[PATCH v2 1/3] 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 currently supports only interrupt driven mode but in future we may add
polling mode support as well.

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                       | 1063 ++++++++++++++++++++++++
 4 files changed, 1109 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..17935f3
--- /dev/null
+++ b/drivers/spi/ep93xx_spi.c
@@ -0,0 +1,1063 @@
+/*
+ * Driver for Cirrus Logic EP93xx SPI controller.
+ *
+ * Copyright (c) 2010 Mika Westerberg
+ *
+ * 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/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
+
+/**
+ * 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
+ * @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
+ *
+ * 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;
+};
+
+/**
+ * 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)
+{
+	u16 regval;
+	int err;
+
+	err = clk_enable(espi->clk);
+	if (err)
+		return err;
+
+	regval = ep93xx_spi_read_u16(espi, SSPCR1);
+	regval |= SSPCR1_SSE;
+	ep93xx_spi_write_u16(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)
+{
+	u16 regval;
+
+	regval = ep93xx_spi_read_u16(espi, SSPCR1);
+	regval &= ~SSPCR1_SSE;
+	ep93xx_spi_write_u16(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_wait_busy() - waits SPI controller while it is busy
+ * @espi: ep93xx SPI controller struct
+ * @msecs: timeout in milliseconds to wait
+ *
+ * Function waits while SPI controller is busy in transferring frames. Returns
+ * %0 when controller is not busy anymore and %-ETIMEDOUT on timeout. Wait is
+ * implemented by busylooping so this function can also be called in atomic
+ * context.
+ */
+static int ep93xx_spi_wait_busy(const struct ep93xx_spi *espi,
+				unsigned long msecs)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(msecs);
+
+	while (time_before(jiffies, timeout)) {
+		if ((ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_BSY) == 0)
+			return 0;
+		cpu_relax();
+	}
+
+	return -ETIMEDOUT;
+}
+
+/**
+ * ep93xx_spi_calc_divisors() - calculates SPI clock divisors
+ * @espi: ep93xx SPI controller struct
+ * @chip: divisors are calculated for this chip
+ * @rate: maximum rate (in Hz) that this chip supports
+ *
+ * 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)
+{
+	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 = max_speed / (cpsr * (1 + scr))
+	 * where max_speed is same as SPI clock rate.
+	 *
+	 * 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 ((espi->max_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);
+}
+
+/**
+ * ep93xx_spi_process_transfer() - processes one SPI transfer
+ * @espi: ep93xx SPI controller struct
+ * @msg: current message
+ * @t: transfer to process
+ *
+ * This function processes one SPI transfer given in @t. Function waits until
+ * transfer is complete (may sleep) and updates @msg->status based on whether
+ * transfer was succesfully processed or not.
+ */
+static void ep93xx_spi_process_transfer(struct ep93xx_spi *espi,
+					struct spi_message *msg,
+					struct spi_transfer *t)
+{
+	struct ep93xx_spi_chip *chip = spi_get_ctldata(msg->spi);
+
+	msg->state = t;
+
+	/*
+	 * Handle any transfer specific settings if needed. We use
+	 * temporary chip settings here and restore original later when
+	 * the transfer is finished.
+	 */
+	if (t->speed_hz || t->bits_per_word) {
+		struct ep93xx_spi_chip tmp_chip = *chip;
+
+		if (t->speed_hz) {
+			int err;
+
+			err = ep93xx_spi_calc_divisors(espi, &tmp_chip,
+						       t->speed_hz);
+			if (err) {
+				dev_err(&espi->pdev->dev,
+					"failed to adjust speed\n");
+				msg->status = err;
+				return;
+			}
+		}
+
+		if (t->bits_per_word)
+			tmp_chip.dss = bits_per_word_to_dss(t->bits_per_word);
+
+		/*
+		 * Set up temporary new hw settings for this transfer.
+		 */
+		ep93xx_spi_chip_setup(espi, &tmp_chip);
+	}
+
+	espi->rx = 0;
+	espi->tx = 0;
+
+	/*
+	 * Now everything is set up for the current transfer. Enable
+	 * interrupts and wait for the transfer to complete.
+	 */
+	ep93xx_spi_enable_interrupts(espi);
+	wait_for_completion(&espi->wait);
+
+	/*
+	 * In case of error during transmit, we bail out from processing
+	 * the message.
+	 */
+	if (msg->status)
+		return;
+
+	/*
+	 * After this transfer is finished, perform any possible
+	 * post-transfer actions requested by the protocol driver.
+	 */
+	if (t->delay_usecs)
+		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) {
+		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);
+}
+
+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 ?: 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). In
+ * case of error negative errno is returned.
+ *
+ * Although this function is currently only used by interrupt handler it is
+ * possible that in future we support polling transfers as well: hence calling
+ * context can be thread or atomic.
+ */
+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;
+
+	/*
+	 * Write as long as TX FIFO is not full. After every write we check
+	 * whether RX FIFO has any new data in it (and in that case we read what
+	 * ever was in the RX FIFO).
+	 */
+	while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_TNF) &&
+		espi->tx < t->len) {
+		ep93xx_do_write(espi, t);
+
+		if (ep93xx_spi_wait_busy(espi, SPI_TIMEOUT)) {
+			msg->status = -ETIMEDOUT;
+			return msg->status;
+		}
+
+		while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
+			espi->rx < t->len) {
+			ep93xx_do_read(espi, t);
+		}
+	}
+
+	/* is transfer finished? */
+	if (espi->tx == t->len && espi->rx == t->len) {
+		msg->actual_length += t->len;
+		return t->len;
+	}
+
+	return 0;
+}
+
+/**
+ * ep93xx_spi_interrupt() - SPI interrupt handler
+ * @irq: IRQ number (not used)
+ * @dev_id: pointer to EP93xx controller struct
+ *
+ * This function handles TX/RX/ROR interrupts that come from the SPI controller.
+ * Returns %IRQ_HANDLED when interrupt was handled and %IRQ_NONE in case the
+ * @irq was not handled.
+ */
+static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id)
+{
+	struct ep93xx_spi *espi = dev_id;
+	u8 irq_status;
+
+	irq_status = ep93xx_spi_read_u8(espi, SSPIIR);
+
+	if (!(irq_status & (SSPIIR_RORIS | SSPIIR_TIS | SSPIIR_RIS)))
+		return IRQ_NONE; /* not for us */
+
+	/*
+	 * 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 * 255);
+	espi->info = info;
+	espi->pdev = pdev;
+
+	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;
+	}
+
+	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_unmap_regs;
+	}
+
+	error = request_irq(espi->irq, ep93xx_spi_interrupt, 0, "ep93xx-spi",
+			    espi);
+	if (error) {
+		dev_err(&pdev->dev, "failed to request rx 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;
+
+	error =	spi_register_master(master);
+	if (error)
+		goto fail_free_queue;
+
+	dev_info(&pdev->dev, "EP93xx SPI Controller at 0x%08lx irq %d\n",
+		 (unsigned long)res->start, espi->irq);
+
+	return 0;
+
+fail_free_queue:
+	destroy_workqueue(espi->wq);
+fail_free_irq:
+	free_irq(espi->irq, espi);
+fail_unmap_regs:
+	iounmap(espi->regs_base);
+fail_free_mem:
+	release_mem_region(res->start, resource_size(res));
+fail_put_clock:
+	clk_put(espi->clk);
+fail_release_master:
+	spi_master_put(master);
+	platform_set_drvdata(pdev, NULL);
+
+	return error;
+}
+
+static int __exit ep93xx_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct ep93xx_spi *espi = spi_master_get_devdata(master);
+	struct resource *res;
+
+	spin_lock_irq(&espi->lock);
+	espi->running = false;
+	spin_unlock_irq(&espi->lock);
+
+	destroy_workqueue(espi->wq);
+
+	/*
+	 * Complete remaining messages with %-ESHUTDOWN status.
+	 */
+	spin_lock_irq(&espi->lock);
+	while (!list_empty(&espi->msg_queue)) {
+		struct spi_message *msg;
+
+		msg = list_first_entry(&espi->msg_queue,
+				       struct spi_message, queue);
+		list_del_init(&msg->queue);
+		msg->status = -ESHUTDOWN;
+		spin_unlock_irq(&espi->lock);
+		msg->complete(msg->context);
+		spin_lock_irq(&espi->lock);
+	}
+	spin_unlock_irq(&espi->lock);
+
+	free_irq(espi->irq, espi);
+	iounmap(espi->regs_base);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(res->start, resource_size(res));
+	clk_put(espi->clk);
+	platform_set_drvdata(pdev, NULL);
+
+	spi_unregister_master(master);
+	return 0;
+}
+
+static struct platform_driver ep93xx_spi_driver = {
+	.driver		= {
+		.name	= "ep93xx-spi",
+		.owner	= THIS_MODULE,
+	},
+	.remove		= __exit_p(ep93xx_spi_remove),
+};
+
+static int __init ep93xx_spi_init(void)
+{
+	return platform_driver_probe(&ep93xx_spi_driver, ep93xx_spi_probe);
+}
+module_init(ep93xx_spi_init);
+
+static void __exit ep93xx_spi_exit(void)
+{
+	platform_driver_unregister(&ep93xx_spi_driver);
+}
+module_exit(ep93xx_spi_exit);
+
+MODULE_DESCRIPTION("EP93xx SPI Controller driver");
+MODULE_AUTHOR("Mika Westerberg <mika.westerberg@iki.fi>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ep93xx-spi");
-- 
1.5.6.5

[PATCH v2 2/3] ep93xx: added chip revision reading function

[Mika Westerberg]

Added a new function: ep93xx_chip_revision() which reads chip revision from the
sysconfig register.

Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
---
 arch/arm/mach-ep93xx/core.c                  |   14 ++++++++++++++
 arch/arm/mach-ep93xx/include/mach/platform.h |    8 ++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
index 90fb591..07572bb 100644
--- a/arch/arm/mach-ep93xx/core.c
+++ b/arch/arm/mach-ep93xx/core.c
@@ -222,6 +222,20 @@ void ep93xx_devcfg_set_clear(unsigned int set_bits, unsigned int clear_bits)
 }
 EXPORT_SYMBOL(ep93xx_devcfg_set_clear);
 
+/**
+ * ep93xx_chip_revision() - returns the EP93xx chip revision
+ *
+ * See <mach/platform.h> for more information.
+ */
+unsigned int ep93xx_chip_revision(void)
+{
+	unsigned int v;
+
+	v = __raw_readl(EP93XX_SYSCON_SYSCFG);
+	v &= EP93XX_SYSCON_SYSCFG_REV_MASK;
+	v >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
+	return v;
+}
 
 /*************************************************************************
  * EP93xx peripheral handling
diff --git a/arch/arm/mach-ep93xx/include/mach/platform.h b/arch/arm/mach-ep93xx/include/mach/platform.h
index c6dc14d..b663390 100644
--- a/arch/arm/mach-ep93xx/include/mach/platform.h
+++ b/arch/arm/mach-ep93xx/include/mach/platform.h
@@ -33,6 +33,14 @@ static inline void ep93xx_devcfg_clear_bits(unsigned int bits)
 	ep93xx_devcfg_set_clear(0x00, bits);
 }
 
+#define EP93XX_CHIP_REV_D0	3
+#define EP93XX_CHIP_REV_D1	4
+#define EP93XX_CHIP_REV_E0	5
+#define EP93XX_CHIP_REV_E1	6
+#define EP93XX_CHIP_REV_E2	7
+
+unsigned int ep93xx_chip_revision(void);
+
 void ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr);
 void ep93xx_register_i2c(struct i2c_gpio_platform_data *data,
 			 struct i2c_board_info *devices, int num);
-- 
1.5.6.5

[PATCH v2 3/3] 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.

Patch depends on the previous patch which adds function ep93xx_chip_revision().

Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
---
 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 07572bb..e25b1bd 100644
--- a/arch/arm/mach-ep93xx/core.c
+++ b/arch/arm/mach-ep93xx/core.c
@@ -377,6 +377,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 b663390..d3fb715 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
 {
@@ -42,6 +43,7 @@ static inline void ep93xx_devcfg_clear_bits(unsigned int bits)
 unsigned int ep93xx_chip_revision(void);
 
 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

[spi-devel-general] [PATCH v2 2/3] ep93xx: added chip revision reading function

[H Hartley Sweeten]

On Thursday, March 18, 2010 10:00 AM, Mika Westerberg wrote:
> Added a new function: ep93xx_chip_revision() which reads chip revision from the
> sysconfig register.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>

Hello Mika,

I'm ok with this part of your patch series.

Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>


> ---
>  arch/arm/mach-ep93xx/core.c                  |   14 ++++++++++++++
>  arch/arm/mach-ep93xx/include/mach/platform.h |    8 ++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> index 90fb591..07572bb 100644
> --- a/arch/arm/mach-ep93xx/core.c
> +++ b/arch/arm/mach-ep93xx/core.c
> @@ -222,6 +222,20 @@ void ep93xx_devcfg_set_clear(unsigned int set_bits, unsigned int clear_bits)
>  }
>  EXPORT_SYMBOL(ep93xx_devcfg_set_clear);
>  
> +/**
> + * ep93xx_chip_revision() - returns the EP93xx chip revision
> + *
> + * See <mach/platform.h> for more information.
> + */
> +unsigned int ep93xx_chip_revision(void)
> +{
> +	unsigned int v;
> +
> +	v = __raw_readl(EP93XX_SYSCON_SYSCFG);
> +	v &= EP93XX_SYSCON_SYSCFG_REV_MASK;
> +	v >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
> +	return v;
> +}
>  
>  /*************************************************************************
>   * EP93xx peripheral handling
> diff --git a/arch/arm/mach-ep93xx/include/mach/platform.h b/arch/arm/mach-ep93xx/include/mach/platform.h
> index c6dc14d..b663390 100644
> --- a/arch/arm/mach-ep93xx/include/mach/platform.h
> +++ b/arch/arm/mach-ep93xx/include/mach/platform.h
> @@ -33,6 +33,14 @@ static inline void ep93xx_devcfg_clear_bits(unsigned int bits)
>  	ep93xx_devcfg_set_clear(0x00, bits);
>  }
>  
> +#define EP93XX_CHIP_REV_D0	3
> +#define EP93XX_CHIP_REV_D1	4
> +#define EP93XX_CHIP_REV_E0	5
> +#define EP93XX_CHIP_REV_E1	6
> +#define EP93XX_CHIP_REV_E2	7
> +
> +unsigned int ep93xx_chip_revision(void);
> +
>  void ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr);
>  void ep93xx_register_i2c(struct i2c_gpio_platform_data *data,
>  			 struct i2c_board_info *devices, int num);

[spi-devel-general] [PATCH v2 2/3] ep93xx: added chip revision reading function

[Mika Westerberg]

On Thu, Mar 18, 2010 at 12:27:15PM -0500, H Hartley Sweeten wrote:
> On Thursday, March 18, 2010 10:00 AM, Mika Westerberg wrote:
> > Added a new function: ep93xx_chip_revision() which reads chip revision from the
> > sysconfig register.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> 
> Hello Mika,
> 
> I'm ok with this part of your patch series.
> 
> Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Hello,

Any comments on the v2 series of the driver? If it is OK could it be considered for .35?

Thanks,
MW

> 
> 
> > ---
> >  arch/arm/mach-ep93xx/core.c                  |   14 ++++++++++++++
> >  arch/arm/mach-ep93xx/include/mach/platform.h |    8 ++++++++
> >  2 files changed, 22 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> > index 90fb591..07572bb 100644
> > --- a/arch/arm/mach-ep93xx/core.c
> > +++ b/arch/arm/mach-ep93xx/core.c
> > @@ -222,6 +222,20 @@ void ep93xx_devcfg_set_clear(unsigned int set_bits, unsigned int clear_bits)
> >  }
> >  EXPORT_SYMBOL(ep93xx_devcfg_set_clear);
> >  
> > +/**
> > + * ep93xx_chip_revision() - returns the EP93xx chip revision
> > + *
> > + * See <mach/platform.h> for more information.
> > + */
> > +unsigned int ep93xx_chip_revision(void)
> > +{
> > +	unsigned int v;
> > +
> > +	v = __raw_readl(EP93XX_SYSCON_SYSCFG);
> > +	v &= EP93XX_SYSCON_SYSCFG_REV_MASK;
> > +	v >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
> > +	return v;
> > +}
> >  
> >  /*************************************************************************
> >   * EP93xx peripheral handling
> > diff --git a/arch/arm/mach-ep93xx/include/mach/platform.h b/arch/arm/mach-ep93xx/include/mach/platform.h
> > index c6dc14d..b663390 100644
> > --- a/arch/arm/mach-ep93xx/include/mach/platform.h
> > +++ b/arch/arm/mach-ep93xx/include/mach/platform.h
> > @@ -33,6 +33,14 @@ static inline void ep93xx_devcfg_clear_bits(unsigned int bits)
> >  	ep93xx_devcfg_set_clear(0x00, bits);
> >  }
> >  
> > +#define EP93XX_CHIP_REV_D0	3
> > +#define EP93XX_CHIP_REV_D1	4
> > +#define EP93XX_CHIP_REV_E0	5
> > +#define EP93XX_CHIP_REV_E1	6
> > +#define EP93XX_CHIP_REV_E2	7
> > +
> > +unsigned int ep93xx_chip_revision(void);
> > +
> >  void ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr);
> >  void ep93xx_register_i2c(struct i2c_gpio_platform_data *data,
> >  			 struct i2c_board_info *devices, int num);

[spi-devel-general] [PATCH v2 2/3] ep93xx: added chip revision reading function

[H Hartley Sweeten]

On Thursday, March 25, 2010 2:07 AM, Mika Westerberg wrote:
> On Thu, Mar 18, 2010 at 12:27:15PM -0500, H Hartley Sweeten wrote:
>> On Thursday, March 18, 2010 10:00 AM, Mika Westerberg wrote:
>>> Added a new function: ep93xx_chip_revision() which reads chip revision from the
>>> sysconfig register.
>>> 
>>> Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
>> 
>> Hello Mika,
>> 
>> I'm ok with this part of your patch series.
>> 
>> Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>
> Hello,
>
> Any comments on the v2 series of the driver? If it is OK could it be considered for .35?

Hello Mika,

This part (2/3) I am fine with.

If you want, you can add this as a separated patch to Russell's patch tracker
with my Acked-by above.


For parts 1 and 3.

I tried testing them in my setup with the mmc_spi.c and sst25l.c drivers.  Both
did not work.  For some reason the data read is always either 0x00 or 0xff.

I will try to get some time this week/weekend to look into this.

Regards,
Hartley

[spi-devel-general] [PATCH v2 2/3] ep93xx: added chip revision reading function

[Martin Guy]

On 3/25/10, H Hartley Sweeten <hartleys@visionengravers.com> wrote:
>  For parts 1 and 3.
>
>  I tried testing them in my setup with the mmc_spi.c and sst25l.c drivers.  Both
>  did not work.  For some reason the data read is always either 0x00 or 0xff.
>
>  I will try to get some time this week/weekend to look into this.

I got these working with the Sim.One board without modification.
The data rate when dd'ing from the block device is slightly slower
than the Cirrus Logic patches (222kB/s instead of 235kB/s), and I got
the same kind of write errors at the same frequency as we did with the
Cirrus code, but this seems to be some problem with our board. Unlike
the CIrrus code, it successfully retries failed multi-block read
requests using single-block reads.

In case it's useful to you, I attach the first hack of the
modifications I made to the board description file
I'm not sure it's right, and I haven't coded in the use of the
MMC-card-present GPIO interrupt line yet, but it "seems to work" ^TM

    M
-------------- next part --------------
A non-text attachment was scrubbed...
Name: simone-add-mmc_spi-2.6.34.patch
Type: text/x-diff
Size: 2072 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100326/1ffed33d/attachment.bin>

[PATCH v2 2/3] ep93xx: added chip revision reading function

[Martin Guy]

On 3/18/10, Mika Westerberg <mika.westerberg@iki.fi> wrote:
> Added a new function: ep93xx_chip_revision() which reads chip revision from the
>  sysconfig register.
>
>  diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
>  index 90fb591..07572bb 100644
>  --- a/arch/arm/mach-ep93xx/core.c
>  +++ b/arch/arm/mach-ep93xx/core.c
>  @@ -222,6 +222,20 @@ void ep93xx_devcfg_set_clear(unsigned int set_bits, unsigned int clear_bits)
>   }
>   EXPORT_SYMBOL(ep93xx_devcfg_set_clear);
>
>  +/**
>  + * ep93xx_chip_revision() - returns the EP93xx chip revision
>  + *
>  + * See <mach/platform.h> for more information.
>  + */
>  +unsigned int ep93xx_chip_revision(void)
>  +{
>  +       unsigned int v;
>  +
>  +       v = __raw_readl(EP93XX_SYSCON_SYSCFG);
>  +       v &= EP93XX_SYSCON_SYSCFG_REV_MASK;
>  +       v >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
>  +       return v;
>  +}

  The chip revision is normally placed in the global unsigned int
"system_rev", which is also reported by /proc/cpuinfo.
  It normally seems to be set from the atags passed by the bootloader
but uboot on this platform doesn't provide that, so it remains 0.
  The same applies to the very similar variables system_serial_low and
system_serial_high (which also remain 0 on this u-boot platform).
Shall we also set these from the hardware on ep93xx during thus giving
them non-zero values in /proc/cpuinfo too?
  As to where...? The other platforms that set these variables from
hardware queries do it in the board-specific files' MACHINE_START
function, but we'd have to do this in every board file, or have some
comon routine and modify every mach-ep93xx board file. The only common
routine all ep93xx board inits seem to have is ep93xx_init_devices.
This seems the least-resistance place to do this, but has nothing to
do with initlalizing devices of course. Is making another function in
toe core file and modifying every board's init function preferable?

Also, I have the simpler
    system_rev = *((unsigned int *)EP93XX_SYSCON_CHIP_ID) >> 28;
    system_serial_low = *((unsigned int *)EP93XX_SECURITY_UNIQID);
but maybe explicit readl and use of long defines are preferable.

Sorry, I'm not familiar with linux kernel best practice...

    M

[PATCH v2 2/3] ep93xx: added chip revision reading function

[Daniel Mack]

On Sat, Mar 20, 2010 at 06:07:50PM +0000, Martin Guy wrote:
>   The chip revision is normally placed in the global unsigned int
> "system_rev", which is also reported by /proc/cpuinfo.

No, that's the hardware revision number, which is different from the
chip revision number.

system_rev is also what is exported via /proc/cpuinfo because userland
software may need this information to adjust their runtime behaviour.

>   It normally seems to be set from the atags passed by the bootloader
> but uboot on this platform doesn't provide that, so it remains 0.

And this is exactly why it should be passed down from the bootloader.
Either the bootloader is special to each hardware revision (which is
command practice), or it is able to read the revision number from a
bunch of pull-up/down resistors.

>   The same applies to the very similar variables system_serial_low and
> system_serial_high (which also remain 0 on this u-boot platform).
> Shall we also set these from the hardware on ep93xx during thus giving
> them non-zero values in /proc/cpuinfo too?

If the ep93xx provides a unique serial number, then this could be an
idea. I would still only set it in case it has not been given by the
bootloader though.

Daniel

[PATCH v2 2/3] ep93xx: added chip revision reading function

[Martin Guy]

On 3/20/10, Daniel Mack <daniel@caiaq.de> wrote:
> On Sat, Mar 20, 2010 at 06:07:50PM +0000, Martin Guy wrote:
>  >   The chip revision is normally placed in the global unsigned int
>  > "system_rev", which is also reported by /proc/cpuinfo.
>
> No, that's the hardware revision number, which is different from the
>  chip revision number.

The board revision? Right, thanks. I don't see a place to report this
in cpuinfo, since it is the SoC revision, which is neither the ARM CPU
it contains nor the board that conatins the SoC.

> If the ep93xx provides a unique serial number, then this could be an
>  idea. I would still only set it in case it has not been given by the
>  bootloader though.

It does, world-unique, burned into the chip. Thanks again.

    M

[PATCH v2 2/3] ep93xx: added chip revision reading function

[H Hartley Sweeten]

On Saturday, March 20, 2010 11:41 AM, Martin Guy wrote:
> On 3/20/10, Daniel Mack <daniel@caiaq.de> wrote:
>> On Sat, Mar 20, 2010 at 06:07:50PM +0000, Martin Guy wrote:
>>>   The chip revision is normally placed in the global unsigned int
>>> "system_rev", which is also reported by /proc/cpuinfo.
>>
>> No, that's the hardware revision number, which is different from the
>>  chip revision number.
>
> The board revision? Right, thanks. I don't see a place to report this
> in cpuinfo, since it is the SoC revision, which is neither the ARM CPU
> it contains nor the board that conatins the SoC.
>
>> If the ep93xx provides a unique serial number, then this could be an
>>  idea. I would still only set it in case it has not been given by the
>>  bootloader though.
>
> It does, world-unique, burned into the chip. Thanks again.

Hello all,

Just to clarify things.

'system_rev' is an unsigned int exported from arch/arm/kernel/setup.c
It is filled in with the ATAG_REVISION data passed from the bootloader.
What this data actually means is specific to the bootloader.

'system_serial_low' and 'system_serial_high' are also unsigned int's
exported from arch/arm/kernel/setup.c.  These are filled in with the
ATAG_SERIAL data passed from the bootloader.  Again, what this data
actually means is specific to the bootloader.

These three values are exposed to the user via /proc/cpuinfo.  The
seq_operations that handle this are also in arch/arm/kernel/setup.c.

Mika's patch has nothing to do with these.  As already noted, the
chip revision needs to be determined in order to figure out the
correct internal frequency of the SSP peripheral clock.

This chip revision is currently not exposed to the user in any fashion.

The ep93xx unique serial number, also called the MaverickCrunch ID, can
be found in the Sercurity registers.  There are actually two of them.
One is a 32-bit unique ID the other is a 128-bit random ID.  Random
meaning random per chip not per boot since it is created by means of
fuseable links on the chip that are set during manufacture.  These
also are not currently exposed to the user in any fashion.

I had submitted a patch previously that adds an architecture specific
extension to /proc/cpuinfo that would allow exposing this information
on the ep93xx.  The same extension could be used on platforms like the
TS-7200 to show the jumper settings on the board.

If there is any interest I will re-post that patch series.

Regards,
Hartley

[PATCH v2 2/3] ep93xx: added chip revision reading function

[Martin Guy]

On 3/20/10, H Hartley Sweeten <hartleys@visionengravers.com> wrote:
>  The ep93xx unique serial number, also called the MaverickCrunch ID, can
>  be found in the Sercurity registers.  There are actually two of them.
>  One is a 32-bit unique ID the other is a 128-bit random ID.  Random
>  meaning random per chip not per boot since it is created by means of
>  fuseable links on the chip that are set during manufacture.  These
>  also are not currently exposed to the user in any fashion.
>
>  I had submitted a patch previously that adds an architecture specific
>  extension to /proc/cpuinfo that would allow exposing this information
>  on the ep93xx.  The same extension could be used on platforms like the
>  TS-7200 to show the jumper settings on the board.
>
>  If there is any interest I will re-post that patch series.

Thanks.

Yes, being able to see the unique ID is certainly useful. I use this
in userspace to set a different MAC address on each board for DHCP
purposes in a classroom of otherwise identical diskless X terminals
based on this chip.

    M

[PATCH v2 2/3] ep93xx: added chip revision reading function

[Mika Westerberg]

On Sat, Mar 20, 2010 at 06:07:50PM +0000, Martin Guy wrote:
> On 3/18/10, Mika Westerberg <mika.westerberg@iki.fi> wrote:
> > Added a new function: ep93xx_chip_revision() which reads chip revision from the
> >  sysconfig register.
> >
> >  diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> >  index 90fb591..07572bb 100644
> >  --- a/arch/arm/mach-ep93xx/core.c
> >  +++ b/arch/arm/mach-ep93xx/core.c
> >  @@ -222,6 +222,20 @@ void ep93xx_devcfg_set_clear(unsigned int set_bits, unsigned int clear_bits)
> >   }
> >   EXPORT_SYMBOL(ep93xx_devcfg_set_clear);
> >
> >  +/**
> >  + * ep93xx_chip_revision() - returns the EP93xx chip revision
> >  + *
> >  + * See <mach/platform.h> for more information.
> >  + */
> >  +unsigned int ep93xx_chip_revision(void)
> >  +{
> >  +       unsigned int v;
> >  +
> >  +       v = __raw_readl(EP93XX_SYSCON_SYSCFG);
> >  +       v &= EP93XX_SYSCON_SYSCFG_REV_MASK;
> >  +       v >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
> >  +       return v;
> >  +}
> 
>   The chip revision is normally placed in the global unsigned int
> "system_rev", which is also reported by /proc/cpuinfo.

Is system_rev same as cpu chip revision? I think it should contain
board revision number or something similar.

>   It normally seems to be set from the atags passed by the bootloader
> but uboot on this platform doesn't provide that, so it remains 0.

Neither my platform (uses redboot):

Hardware        : Technologic Systems TS-72xx SBC
Revision        : 0000
Serial          : 0000000000000000

>   The same applies to the very similar variables system_serial_low and
> system_serial_high (which also remain 0 on this u-boot platform).
> Shall we also set these from the hardware on ep93xx during thus giving
> them non-zero values in /proc/cpuinfo too?
>   As to where...? The other platforms that set these variables from
> hardware queries do it in the board-specific files' MACHINE_START
> function, but we'd have to do this in every board file, or have some
> comon routine and modify every mach-ep93xx board file. The only common
> routine all ep93xx board inits seem to have is ep93xx_init_devices.
> This seems the least-resistance place to do this, but has nothing to
> do with initlalizing devices of course. Is making another function in
> toe core file and modifying every board's init function preferable?
> 
> Also, I have the simpler
>     system_rev = *((unsigned int *)EP93XX_SYSCON_CHIP_ID) >> 28;
>     system_serial_low = *((unsigned int *)EP93XX_SECURITY_UNIQID);
> but maybe explicit readl and use of long defines are preferable.

I'm not the best guy to answer these. Maybe Hartley Sweeten or Ryan
Mallon have some ideas about this. I didn't even know that such
variable exists :)

Regards,
MW

[PATCH v2 2/3] ep93xx: added chip revision reading function

[H Hartley Sweeten]

On Saturday, March 20, 2010 11:31 AM, Mika Westerberg wrote:
> On Sat, Mar 20, 2010 at 06:07:50PM +0000, Martin Guy wrote:
>> On 3/18/10, Mika Westerberg <mika.westerberg@iki.fi> wrote:
>>> Added a new function: ep93xx_chip_revision() which reads chip revision from the
>>>  sysconfig register.
>>>
>>>  diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
>>>  index 90fb591..07572bb 100644
>>>  --- a/arch/arm/mach-ep93xx/core.c
>>>  +++ b/arch/arm/mach-ep93xx/core.c
>>>  @@ -222,6 +222,20 @@ void ep93xx_devcfg_set_clear(unsigned int set_bits, unsigned int clear_bits)
>>>   }
>>>   EXPORT_SYMBOL(ep93xx_devcfg_set_clear);
>>>
>>>  +/**
>>>  + * ep93xx_chip_revision() - returns the EP93xx chip revision
>>>  + *
>>>  + * See <mach/platform.h> for more information.
>>>  + */
>>>  +unsigned int ep93xx_chip_revision(void)
>>>  +{
>>>  +       unsigned int v;
>>>  +
>>>  +       v = __raw_readl(EP93XX_SYSCON_SYSCFG);
>>>  +       v &= EP93XX_SYSCON_SYSCFG_REV_MASK;
>>>  +       v >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
>>>  +       return v;
>>>  +}
>> 
>>   The chip revision is normally placed in the global unsigned int
>> "system_rev", which is also reported by /proc/cpuinfo.
>
> Is system_rev same as cpu chip revision? I think it should contain
> board revision number or something similar.

The system_rev != the chip revision.

>>   It normally seems to be set from the atags passed by the bootloader
>> but uboot on this platform doesn't provide that, so it remains 0.
>
> Neither my platform (uses redboot):
>
> Hardware        : Technologic Systems TS-72xx SBC
> Revision        : 0000
> Serial          : 0000000000000000
>
>>   The same applies to the very similar variables system_serial_low and
>> system_serial_high (which also remain 0 on this u-boot platform).
>> Shall we also set these from the hardware on ep93xx during thus giving
>> them non-zero values in /proc/cpuinfo too?
>>   As to where...? The other platforms that set these variables from
>> hardware queries do it in the board-specific files' MACHINE_START
>> function, but we'd have to do this in every board file, or have some
>> comon routine and modify every mach-ep93xx board file. The only common
>> routine all ep93xx board inits seem to have is ep93xx_init_devices.
>> This seems the least-resistance place to do this, but has nothing to
>> do with initlalizing devices of course. Is making another function in
>> toe core file and modifying every board's init function preferable?
>> 
>> Also, I have the simpler
>>     system_rev = *((unsigned int *)EP93XX_SYSCON_CHIP_ID) >> 28;
>>     system_serial_low = *((unsigned int *)EP93XX_SECURITY_UNIQID);
>> but maybe explicit readl and use of long defines are preferable.
>
> I'm not the best guy to answer these. Maybe Hartley Sweeten or Ryan
> Mallon have some ideas about this. I didn't even know that such
> variable exists :)

This is not a good idea.

system_rev, system_serial_high, and system_serial_low are all passed to
the kernel by the bootloader as ATAGs.  If your bootloader doesn't pass
the ATAGs it should be fixed.  Overriding the variables breaks what they
are intended for.

I have some patches for Redboot but Cirrus still has not sent in the FSF
paperwork to transfer the ep93xx port to eCos.  With those patches and
the /proc/cpuinfo extension patches I mentioned previously I get the
following on my system:

/ # cat /proc/cpuinfo
Processor       : ARM920T rev 0 (v4l)
BogoMIPS        : 99.53
Features        : swp half thumb crunch
CPU implementer : 0x41
CPU architecture: 4T
CPU variant     : 0x1
CPU part        : 0x920
CPU revision    : 0

Hardware        : Vision Engraving Systems EP9307
Revision        : 0001
Serial          : 4943410027260017

System Type     : iMARC

Unique ID       : 921040f8
Maverick Key    : 921040f85b6f0e7b76eb23d24309b3 OK
Silicon Rev     : E2
Watchdog        : active
Reset duration  : active
Boot mode       : normal async internal 16-bit
/ #

Regards,
Hartley

[PATCH v2 2/3] ep93xx: added chip revision reading function

[Mika Westerberg]

On Sat, Mar 20, 2010 at 02:42:26PM -0500, H Hartley Sweeten wrote:
> On Saturday, March 20, 2010 11:31 AM, Mika Westerberg wrote:
> > On Sat, Mar 20, 2010 at 06:07:50PM +0000, Martin Guy wrote:
> >> On 3/18/10, Mika Westerberg <mika.westerberg@iki.fi> wrote:
> >>> Added a new function: ep93xx_chip_revision() which reads chip revision from the
> >>>  sysconfig register.
> >>>
> >>>  diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> >>>  index 90fb591..07572bb 100644
> >>>  --- a/arch/arm/mach-ep93xx/core.c
> >>>  +++ b/arch/arm/mach-ep93xx/core.c
> >>>  @@ -222,6 +222,20 @@ void ep93xx_devcfg_set_clear(unsigned int set_bits, unsigned int clear_bits)
> >>>   }
> >>>   EXPORT_SYMBOL(ep93xx_devcfg_set_clear);
> >>>
> >>>  +/**
> >>>  + * ep93xx_chip_revision() - returns the EP93xx chip revision
> >>>  + *
> >>>  + * See <mach/platform.h> for more information.
> >>>  + */
> >>>  +unsigned int ep93xx_chip_revision(void)
> >>>  +{
> >>>  +       unsigned int v;
> >>>  +
> >>>  +       v = __raw_readl(EP93XX_SYSCON_SYSCFG);
> >>>  +       v &= EP93XX_SYSCON_SYSCFG_REV_MASK;
> >>>  +       v >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
> >>>  +       return v;
> >>>  +}
> >> 
> >>   The chip revision is normally placed in the global unsigned int
> >> "system_rev", which is also reported by /proc/cpuinfo.
> >
> > Is system_rev same as cpu chip revision? I think it should contain
> > board revision number or something similar.
> 
> The system_rev != the chip revision.
> 
> >>   It normally seems to be set from the atags passed by the bootloader
> >> but uboot on this platform doesn't provide that, so it remains 0.
> >
> > Neither my platform (uses redboot):
> >
> > Hardware        : Technologic Systems TS-72xx SBC
> > Revision        : 0000
> > Serial          : 0000000000000000
> >
> >>   The same applies to the very similar variables system_serial_low and
> >> system_serial_high (which also remain 0 on this u-boot platform).
> >> Shall we also set these from the hardware on ep93xx during thus giving
> >> them non-zero values in /proc/cpuinfo too?
> >>   As to where...? The other platforms that set these variables from
> >> hardware queries do it in the board-specific files' MACHINE_START
> >> function, but we'd have to do this in every board file, or have some
> >> comon routine and modify every mach-ep93xx board file. The only common
> >> routine all ep93xx board inits seem to have is ep93xx_init_devices.
> >> This seems the least-resistance place to do this, but has nothing to
> >> do with initlalizing devices of course. Is making another function in
> >> toe core file and modifying every board's init function preferable?
> >> 
> >> Also, I have the simpler
> >>     system_rev = *((unsigned int *)EP93XX_SYSCON_CHIP_ID) >> 28;
> >>     system_serial_low = *((unsigned int *)EP93XX_SECURITY_UNIQID);
> >> but maybe explicit readl and use of long defines are preferable.
> >
> > I'm not the best guy to answer these. Maybe Hartley Sweeten or Ryan
> > Mallon have some ideas about this. I didn't even know that such
> > variable exists :)
> 
> This is not a good idea.
> 
> system_rev, system_serial_high, and system_serial_low are all passed to
> the kernel by the bootloader as ATAGs.  If your bootloader doesn't pass
> the ATAGs it should be fixed.  Overriding the variables breaks what they
> are intended for.

Ok, thanks for clarification. It seems that in my platform, the bootloader
doesn't handle these correctly.

> I have some patches for Redboot but Cirrus still has not sent in the FSF
> paperwork to transfer the ep93xx port to eCos.  With those patches and
> the /proc/cpuinfo extension patches I mentioned previously I get the
> following on my system:
> 
> / # cat /proc/cpuinfo
> Processor       : ARM920T rev 0 (v4l)
> BogoMIPS        : 99.53
> Features        : swp half thumb crunch
> CPU implementer : 0x41
> CPU architecture: 4T
> CPU variant     : 0x1
> CPU part        : 0x920
> CPU revision    : 0
> 
> Hardware        : Vision Engraving Systems EP9307
> Revision        : 0001
> Serial          : 4943410027260017
> 
> System Type     : iMARC
> 
> Unique ID       : 921040f8
> Maverick Key    : 921040f85b6f0e7b76eb23d24309b3 OK
> Silicon Rev     : E2
> Watchdog        : active
> Reset duration  : active
> Boot mode       : normal async internal 16-bit
> / #

Cool! You should definitely repost your patches.

Regards,
MW

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 3/18/10, 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 currently supports only interrupt driven mode but in future we may add
>  polling mode support as well.
>
>  Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
>  ---

>  diff --git a/drivers/spi/ep93xx_spi.c b/drivers/spi/ep93xx_spi.c
>  new file mode 100644
>  index 0000000..17935f3
>  --- /dev/null
>  +++ b/drivers/spi/ep93xx_spi.c

>  +/**
>  + * ep93xx_spi_calc_divisors() - calculates SPI clock divisors
>  + * @espi: ep93xx SPI controller struct
>  + * @chip: divisors are calculated for this chip
>  + * @rate: maximum rate (in Hz) that this chip supports

Isn't that
>  + * @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)
>  +{
...
>  +       /*
>  +        * Calculate divisors so that we can get speed according the
>  +        * following formula:
>  +        *      rate = max_speed / (cpsr * (1 + scr))
>  +        * where max_speed is same as SPI clock rate.
>  +        *
>  +        * 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 ((espi->max_rate / (cpsr * (scr + 1))) < rate) {

Shouldn't that be

>  +                       if (clk_get_rate(espi->clk) / (cpsr * (scr + 1)) <= rate) {

since max_rate is the maximum usable rate, which is sspclk / 2, so the
existing code seems like it would set the divisors to give twice the
requested frequency.

Conversely, if an exact divisor of the master clock rate is requested
(such as 1843200), the "<" would give the next lower rate instead of
the requested rate so for the highest rates the doubling and this
halving would cancel out, while for lower rates you could get up to
twice the requested clock frequecy.

>  +static int __init ep93xx_spi_probe(struct platform_device *pdev)
>  +{
...
>  +       espi->max_rate = clk_get_rate(espi->clk) / 2;
>  +       espi->min_rate = clk_get_rate(espi->clk) / (254 * 255);

Isn't that
>  +       espi->min_rate = clk_get_rate(espi->clk) / (254 * 256);

since the output SPI clock frequency is sspclk / (cpsr * (1 + scr))
where cpsr = 2..254 step 2, scr = 1..255

I also notice that the code recalculates the clock divisors, programs
them ad resotres them for every transfer, even if the frequency etc is
always the same.
I don't know if this makes much difference to speed in normal
operation but it does make the debug output incredibly verbose,
halving transfer speeds when SPI debug is enabled.

Is there a suitable place to be lazy about this, so as to avoid doing
this if the transfer parameters are the same on successive operations?

Thanks

    M

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Thu, Mar 25, 2010 at 01:49:32PM +0000, Martin Guy wrote:
> On 3/18/10, 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 currently supports only interrupt driven mode but in future we may add
> >  polling mode support as well.
> >
> >  Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> >  ---
> 
> >  diff --git a/drivers/spi/ep93xx_spi.c b/drivers/spi/ep93xx_spi.c
> >  new file mode 100644
> >  index 0000000..17935f3
> >  --- /dev/null
> >  +++ b/drivers/spi/ep93xx_spi.c
> 
> >  +/**
> >  + * ep93xx_spi_calc_divisors() - calculates SPI clock divisors
> >  + * @espi: ep93xx SPI controller struct
> >  + * @chip: divisors are calculated for this chip
> >  + * @rate: maximum rate (in Hz) that this chip supports
> 
> Isn't that
> >  + * @rate: desired SPI output clock rate

Yes.

> 
> >  + *
> >  + * 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)
> >  +{
> ...
> >  +       /*
> >  +        * Calculate divisors so that we can get speed according the
> >  +        * following formula:
> >  +        *      rate = max_speed / (cpsr * (1 + scr))
> >  +        * where max_speed is same as SPI clock rate.
> >  +        *
> >  +        * 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 ((espi->max_rate / (cpsr * (scr + 1))) < rate) {
> 
> Shouldn't that be
> 
> >  +                       if (clk_get_rate(espi->clk) / (cpsr * (scr + 1)) <= rate) {
> 
> since max_rate is the maximum usable rate, which is sspclk / 2, so the
> existing code seems like it would set the divisors to give twice the
> requested frequency.
> 
> Conversely, if an exact divisor of the master clock rate is requested
> (such as 1843200), the "<" would give the next lower rate instead of
> the requested rate so for the highest rates the doubling and this
> halving would cancel out, while for lower rates you could get up to
> twice the requested clock frequecy.

Yes, you are right. Very good observation :)

> 
> >  +static int __init ep93xx_spi_probe(struct platform_device *pdev)
> >  +{
> ...
> >  +       espi->max_rate = clk_get_rate(espi->clk) / 2;
> >  +       espi->min_rate = clk_get_rate(espi->clk) / (254 * 255);
> 
> Isn't that
> >  +       espi->min_rate = clk_get_rate(espi->clk) / (254 * 256);
> 
> since the output SPI clock frequency is sspclk / (cpsr * (1 + scr))
> where cpsr = 2..254 step 2, scr = 1..255

Yes.

> I also notice that the code recalculates the clock divisors, programs
> them ad resotres them for every transfer, even if the frequency etc is
> always the same.

It shouldn't. This is checked in ep93xx_spi_setup() and clocks are
only calculated when speed is changed. Same thing is with
ep93xx_spi_process_transfer() where clocks are only calculated when
different transfer speed is requested (t->speed_hz != 0).

Thank you for your comments. I will fix these in the next revision
of the patch series.

Regards,
MW

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 3/25/10, 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 currently supports only interrupt driven mode but in future we may add
>  > >  polling mode support as well.

I've been staring more at this again and it looks (2 clock
strangenesses and extensive control reg setting apart) like good code.
I have another question: like the Cirrus driver, this takes 100% CPU
doing busy wait for the current transfer to complete.
Given that this driver is interrupt-based, is there any reason why it
can't do something else in the meanwhile?
Not that that's a reason not to include it in 2.6.35 - it works well
and we can think whether to make it more efficient in N+1...

   M

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Ryan Mallon]

Martin Guy wrote:
> On 3/25/10, 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 currently supports only interrupt driven mode but in future we may add
>>  > >  polling mode support as well.
> 
> I've been staring more at this again and it looks (2 clock
> strangenesses and extensive control reg setting apart) like good code.
> I have another question: like the Cirrus driver, this takes 100% CPU
> doing busy wait for the current transfer to complete.
> Given that this driver is interrupt-based, is there any reason why it
> can't do something else in the meanwhile?
> Not that that's a reason not to include it in 2.6.35 - it works well
> and we can think whether to make it more efficient in N+1...

I can't see anything outstanding in the driver which would make it do
that. I haven't had time to test the driver out yet, but it would be
nice to fix this before it gets committed. Hopefully it shouldn't be too
hard to fix. Any idea where the bulk of the time is being spent?

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

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

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Thu, Apr 01, 2010 at 01:15:20AM +0100, Martin Guy wrote:
> On 3/25/10, 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 currently supports only interrupt driven mode but in future we may add
> >  > >  polling mode support as well.
> 
> I've been staring more at this again and it looks (2 clock
> strangenesses and extensive control reg setting apart) like good code.

Thanks :)

> I have another question: like the Cirrus driver, this takes 100% CPU
> doing busy wait for the current transfer to complete.

Well it sleeps in ep93xx_spi_process_transfer() while current
transfer is complete. It shouldn't busy wait. At least that was not
my intention. If you have lot of data to transfer (for example while
using mmc_spi), the driver will be very busy.

But I can take a look on this and see if there is something wrong.

> Given that this driver is interrupt-based, is there any reason why it
> can't do something else in the meanwhile?

I had an idea that the driver could keep SPI controller interrupts
enabled and start next transfer in interrupt handler but I didn't
implement that yet, because first I wanted a driver that works.

> Not that that's a reason not to include it in 2.6.35 - it works well
> and we can think whether to make it more efficient in N+1...

Yeah, maybe we can optimize that later.

Thank you very much. It is good to hear that someone (else than me)
has been using this :)

Regards,
MW

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Thu, Apr 01, 2010 at 01:15:20AM +0100, Martin Guy wrote:
> On 3/25/10, 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 currently supports only interrupt driven mode but in future we may add
> >  > >  polling mode support as well.
> 
> I've been staring more at this again and it looks (2 clock
> strangenesses and extensive control reg setting apart) like good code.
> I have another question: like the Cirrus driver, this takes 100% CPU
> doing busy wait for the current transfer to complete.

Hi,

I tried to find out whether the driver did something wrong to get
into busylooping but couldn't find anything. In case of MMC/SD
cards, through mmc_spi, transfer sizes are >= 512 bytes at a time
and FIFO size is only 8 bytes (or words) so the CPU is expected to
be pretty busy serving interrupts from the SSP.

I'm about to post updated series with your comments addressed. I
also added support for polling transfers.

> Given that this driver is interrupt-based, is there any reason why it
> can't do something else in the meanwhile?
> Not that that's a reason not to include it in 2.6.35 - it works well
> and we can think whether to make it more efficient in N+1...

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/6/10, Mika Westerberg <mika.westerberg@iki.fi> wrote:
> On Thu, Apr 01, 2010 at 01:15:20AM +0100, Martin Guy wrote:
>  > I have another question: like the Cirrus driver, this takes 100% CPU
>  > doing busy wait for the current transfer to complete.
>
>  I tried to find out whether the driver did something wrong to get
>  into busylooping but couldn't find anything. In case of MMC/SD
>  cards, through mmc_spi, transfer sizes are >= 512 bytes at a time
>  and FIFO size is only 8 bytes (or words) so the CPU is expected to
>  be pretty busy serving interrupts from the SSP.

Right. that explains it.
The board I'm testing it with just has a single mmc/sd card on the ssp port.

The docs say "separate transmit and receive FIFO memory buffers, 16
bits wide, 8 locations deep".
Watching vmstat output with this driver, it gets 226 kB/sec reading
/dev/mmcblk0 at 100% system cpu usage with 2354 interrupts and 2724
context switches per second. I don't see how that relates to 8 bytes
or 8 halfwords per interrupt; it suggests 100 bytes transferred per
interrupt once you remove the 100/sec of clock tick irq.
Writing instead gets 228kB/sec incurring 1916 (read: 1815) irqs and
934 context switches.
Similarly, checking /proc/interrupts, writing 10MB to the raw device
incurs 83221 ep93xx-spi interrupts: one per 125 bytes transferred. Is
it maybe busy-waiting filling the output FIFO as fast as it is
emptying?

Here's the code I'm talking about:

static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
{
...
        /*
         * Write as long as TX FIFO is not full. After every write we check
         * whether RX FIFO has any new data in it (and in that case we read what
         * ever was in the RX FIFO).
         */
        while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_TNF) &&
                espi->tx < t->len) {
                ep93xx_do_write(espi, t);

                if (ep93xx_spi_wait_busy(espi, SPI_TIMEOUT)) {
                        msg->status = -ETIMEDOUT;
                        return msg->status;
                }

                while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
                        espi->rx < t->len) {
                        ep93xx_do_read(espi, t);
                }
        }
...

and ep93xx_spi_wait_busy() reads a device register in an busy loop
with a timeout:

static int ep93xx_spi_wait_busy(const struct ep93xx_spi *espi,
                                unsigned long msecs)
{
        unsigned long timeout = jiffies + msecs_to_jiffies(msecs);

        while (time_before(jiffies, timeout)) {
                if ((ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_BSY) == 0)
                        return 0;
                cpu_relax();
        }

        return -ETIMEDOUT;
}

The manual says that the BSY status bit means
"SSP is currently transmitting and / or receiving a frame
or the transmit FIFO is non-empty."
so this would make the code
- output one byte/halfword
- wait busily until the FIFO is empty of that one datum
- maybe read one received word/byte
- loop

This seems bizarre compared to what the hardware suggests:
We only have one interrupt, which is triggered when
- the TX buffer is less than half empty (ie there is room for 4 or more words)
- the RX buffer is more than half full (ie there are four or more
words to read from it)
- RX overrun
and there are status bits to tell us whether the TX and RX buffers are
full, empty or half-full (the half-full bits triggering the
interrupts).

My understanding ATM is that this version enables interrupts to start
a transfer, immediately gets interrupted because the transmit buffer
is less than half empty, and if it is supposed to be reading from the
card, tramsmits a 0 byte/halfword for every byte/halfword it is
supposed to be receiving.

If it's supposed to be sending, it pokes one byte/halfword into the
write FIFO, waits for that to have been completely transmitted and for
the FIFO to be empty, then checks if it should read anything, then
writes another byte/halfword etc. and so on.

This seems completely at odds with what the hardware suggests, which would be
WRITE: fill the TX buffer, then wait for half-empty IRQ.
READ: react to half-full IRQ by emptying the RX buffer.
with some hackery because if you receive a final 1, 2 or 3
byte/halfwords, you don't get an RX interrupt.

I've tried removing the busy-wait from the read-write loop and the
kernel hangs forever as soon as it looks at the SSP device. Worse, my
serial console is dead so I can't be more expicit.

Am I missing something here, or is the current strategy forced by
undocumented brokenness of the hardware?

Lastly, what is the meaning of conditional operator(s) in
        return t->bits_per_word ?: msg->spi->bits_per_word;
It compiles, but it's the first time I've seen this construct in 27
years of C programming. What is the "normal" syntax for this?

    M

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Tue, Apr 06, 2010 at 01:50:44PM +0100, Martin Guy wrote:
> On 4/6/10, Mika Westerberg <mika.westerberg@iki.fi> wrote:
> > On Thu, Apr 01, 2010 at 01:15:20AM +0100, Martin Guy wrote:
> >  > I have another question: like the Cirrus driver, this takes 100% CPU
> >  > doing busy wait for the current transfer to complete.
> >
> >  I tried to find out whether the driver did something wrong to get
> >  into busylooping but couldn't find anything. In case of MMC/SD
> >  cards, through mmc_spi, transfer sizes are >= 512 bytes at a time
> >  and FIFO size is only 8 bytes (or words) so the CPU is expected to
> >  be pretty busy serving interrupts from the SSP.
> 
> Right. that explains it.
> The board I'm testing it with just has a single mmc/sd card on the ssp port.
> 
> The docs say "separate transmit and receive FIFO memory buffers, 16
> bits wide, 8 locations deep".
> Watching vmstat output with this driver, it gets 226 kB/sec reading
> /dev/mmcblk0 at 100% system cpu usage with 2354 interrupts and 2724
> context switches per second. I don't see how that relates to 8 bytes
> or 8 halfwords per interrupt; it suggests 100 bytes transferred per
> interrupt once you remove the 100/sec of clock tick irq.
> Writing instead gets 228kB/sec incurring 1916 (read: 1815) irqs and
> 934 context switches.
> Similarly, checking /proc/interrupts, writing 10MB to the raw device
> incurs 83221 ep93xx-spi interrupts: one per 125 bytes transferred. Is
> it maybe busy-waiting filling the output FIFO as fast as it is
> emptying?

If you check the sizes of the transfers for the MMC case it is something like:

[ 1796.080000] ep93xx-spi ep93xx-spi: setup: mode 0, cpsr 2, scr 0, dss 7
[ 1796.080000] ep93xx-spi ep93xx-spi: setup: cr0 0x7
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_process_transfer: transferring 512 bytes
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_interrupt: handling interrupt
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_read_write: tx=512, rx=512
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_process_transfer: transferring 2 bytes
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_interrupt: handling interrupt
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_read_write: tx=2, rx=2
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_process_transfer: transferring 1 bytes
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_interrupt: handling interrupt
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_read_write: tx=1, rx=1

So size of the transfer varies, depending on what kind of message
is passed from mmc_spi to the driver. From the above, we can see
that ep93xx_spi_read_write() transfers 512 bytes before passing control
back to the calling interrupt handler.

I got the same results with polling enabled (this is with v3 which is not yet
posted).

> 
> Here's the code I'm talking about:
> 
> static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
> {
> ...
>         /*
>          * Write as long as TX FIFO is not full. After every write we check
>          * whether RX FIFO has any new data in it (and in that case we read what
>          * ever was in the RX FIFO).
>          */
>         while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_TNF) &&
>                 espi->tx < t->len) {
>                 ep93xx_do_write(espi, t);
> 
>                 if (ep93xx_spi_wait_busy(espi, SPI_TIMEOUT)) {
>                         msg->status = -ETIMEDOUT;
>                         return msg->status;
>                 }
> 
>                 while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
>                         espi->rx < t->len) {
>                         ep93xx_do_read(espi, t);
>                 }
>         }
> ...
> 
> and ep93xx_spi_wait_busy() reads a device register in an busy loop
> with a timeout:
> 
> static int ep93xx_spi_wait_busy(const struct ep93xx_spi *espi,
>                                 unsigned long msecs)
> {
>         unsigned long timeout = jiffies + msecs_to_jiffies(msecs);
> 
>         while (time_before(jiffies, timeout)) {
>                 if ((ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_BSY) == 0)
>                         return 0;
>                 cpu_relax();
>         }
> 
>         return -ETIMEDOUT;
> }
> 
> The manual says that the BSY status bit means
> "SSP is currently transmitting and / or receiving a frame
> or the transmit FIFO is non-empty."

I think that when the controller is enabled (SSE is set), it is
transmitting/receiving once there is data (TX). It even enables the
SPI output clock only when TX data is put into TX FIFO (I checked
this with oscilloscope).

I added debug print to this function (ep93xx_spi_wait_busy()) and
it loops only once. If I don't wait while the bit is cleared, the
controller hangs.

> so this would make the code
> - output one byte/halfword
> - wait busily until the FIFO is empty of that one datum
> - maybe read one received word/byte
> - loop
> 
> This seems bizarre compared to what the hardware suggests:
> We only have one interrupt, which is triggered when
> - the TX buffer is less than half empty (ie there is room for 4 or more words)
> - the RX buffer is more than half full (ie there are four or more
> words to read from it)
> - RX overrun
> and there are status bits to tell us whether the TX and RX buffers are
> full, empty or half-full (the half-full bits triggering the
> interrupts).
> 
> My understanding ATM is that this version enables interrupts to start
> a transfer, immediately gets interrupted because the transmit buffer
> is less than half empty, and if it is supposed to be reading from the
> card, tramsmits a 0 byte/halfword for every byte/halfword it is
> supposed to be receiving.
> 
> If it's supposed to be sending, it pokes one byte/halfword into the
> write FIFO, waits for that to have been completely transmitted and for
> the FIFO to be empty, then checks if it should read anything, then
> writes another byte/halfword etc. and so on.
> 
> This seems completely at odds with what the hardware suggests, which would be
> WRITE: fill the TX buffer, then wait for half-empty IRQ.
> READ: react to half-full IRQ by emptying the RX buffer.
> with some hackery because if you receive a final 1, 2 or 3
> byte/halfwords, you don't get an RX interrupt.

Since SPI is full-duplex, I think that it is enough to just perform
another transfer once we get RX/TX interrupt. This also makes the
driver simpler (yet not the most efficient).

At least if you see my comments above, the driver seems to do the
right thing (for example it transmitted one spi_transfer at a time
without busy looping).

> I've tried removing the busy-wait from the read-write loop and the
> kernel hangs forever as soon as it looks at the SSP device. Worse, my
> serial console is dead so I can't be more expicit.

Yup, I noticed same thing. Once BSY bit is cleared, RX FIFO can be
read without hangs (or something like that).

> Am I missing something here, or is the current strategy forced by
> undocumented brokenness of the hardware?

I'm not sure.

For me, current strategy seems clear:
	1. Fetch next message
		1.1 Enable the SPI controller
		1.2 Assert chip-select
		1.3 Fetch next transfer
			1.3.1 Enable interrupts (if not polling)
			1.3.2 Perform the transfer
				write/read to/from the FIFO
			1.3.2 Disable interrupts (if not polling)
			... (for all spi_transfers)
		1.4 De-assert chip-select
		1.5 Disable the SPI controller
		1.6 call msg->complete
		...

It isn't the most optimized but my purpose was to get it working first. Since I only
have one ep9302 board and 2 SPI devices, it limits my testing a bit. Thanks to you
I've got very valuable input related to the driver :)

If you have time, I suggest that add some printk()s to the
ep93xx_spi_wait_busy() and see whether it busy loops in your device
(or does some other weird things).

> Lastly, what is the meaning of conditional operator(s) in
>         return t->bits_per_word ?: msg->spi->bits_per_word;
> It compiles, but it's the first time I've seen this construct in 27
> years of C programming. What is the "normal" syntax for this?

It is an GCC extension:

http://gcc.gnu.org/onlinedocs/gcc-4.4.3/gcc/Conditionals.html#Conditionals

I've been using it once I saw it somewhere in the kernel source.

Thanks again,
MW

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/6/10, Mika Westerberg <mika.westerberg@iki.fi> wrote:
>  I only have one ep9302 board and 2 SPI devices

If you'd like one of the MMC-only boards to test on, I can ship it to
you - please provide smail mail address by private mail if that would
be welcome

>  > Lastly, what is the meaning of conditional operator(s) in
>  >         return t->bits_per_word ?: msg->spi->bits_per_word;
>  > It compiles, but it's the first time I've seen this construct in 27
>  > years of C programming. What is the "normal" syntax for this?
>
> It is an GCC extension:
>
>  http://gcc.gnu.org/onlinedocs/gcc-4.4.3/gcc/Conditionals.html#Conditionals
>
>  I've been using it once I saw it somewhere in the kernel source.

OK, I suggest we don't use "GNU extensions" if they can be avoided,
and program in C. The GCC monoculture is ending, thanks to llvm. and
those gnu-heads are not the most wise people on the planet. (OK, the
ANSI committe are worse, but.... :)

   M

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

Hi again

I've looked at this again, and v2 does send every complete block (up
to 512 bytes) before it returns from an interrupt. If you factor out
all the conditions that always have constant values, and those that
can never happen, from the central loop:

        /*
         * Write as long as TX FIFO is not full. After every write we check
         * whether RX FIFO has any new data in it (and in that case we read what
         * ever was in the RX FIFO).
         */
        while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_TNF) &&
                espi->tx < t->len) {
                ep93xx_do_write(espi, t);

                if (ep93xx_spi_wait_busy(espi, SPI_TIMEOUT)) {
                        msg->status = -ETIMEDOUT;
                        return msg->status;
                }

                while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
                        espi->rx < t->len) {
                        ep93xx_do_read(espi, t);
                }
        }

you end up with:

        while (espi->tx < t->len) {
                ep93xx_do_write(espi, t);

                while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_BSY);

                ep93xx_do_read(espi, t);
        }

This makes each interrupt routine take about 2ms to complete. I don't
know if that's unacceptably long or not.

I've coded up a version that works differently, by filling the TX FIFO
and returning from the IRQ routine - it then get interrupted again
when the TX FIFO is half empty (and the RX FIFO is half full), at
which point it empties and fills them
and waits for another interrupt.
The end of the transfer has to be handled specially because the
interrupt is held high as long as the TX FIFO is half empty or less,
so we have to busy-wait while the final four words are transmitted and
received, concluding the transfer.

The result is an increase in MMC card read or write speed from 232kB/s
to 315kB/s and a decrease in CPU usage from 100% to 55-60%, tested
with plain 1GB SD card.
A 2GB MLC card instead gets 323kB/sec in read and 319kB/sec in write (!).

SDHC cards don't work at all - they are rejected by the MMC-SPI driver
(eror -38, ENOSYS: Function not implemented), the same as happens with
the old CIrrus driver.

I've only changed the ep93xx_spi_read_write() function in your v2
interrupting driver: the new version is below. Would you see if it
works with your other SPI hardware?

I've tried also to handle transfers with word size > 8 bits but have
not been able to test these.

Cheers

    M

/**
 * 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). In
 * case of error negative errno is returned.
 *
 * Although this function is currently only used by interrupt handler it is
 * possible that in future we support polling transfers as well: hence calling
 * context can be thread or atomic.
 *
 * Both RX and TX sides have an 8-item-deep FIFO, and interrupts are caused by
 * - the transmit FIFO being half empty or less (ie room for four more words)
 * - the receive FIFO being half full or more (ie four or more words in it)
 * - receive overrun
 * This driver works by queuing the SPI transfer list and enabling interrupts
 * in the device. This immediately causes a first interrupt because the TX FIFO
 * is empty. We fill the TX FIFO and return. We are then interrupted again when
 * four words have been transmitted (and received), so we empty the RX and fill
 * the TX FIFOs until there are less than 4 words remaining to be transmitted.
 * At that point interrupts become useless because the TX IRQ remains set while
 * the last 4 words are being transmitted, and the RX IRQ won't trigger unless
 * there are 4 or more words in the RX FIFO. So at the end of the transfer we
 * simply busy-wait for the last words to be transmitted and received.
 *
 * It's tempting to use the TNF (Transmit buffer not full) status bit to know
 * when we have filled the TX FIFO but that makes for RX overruns because
 * it queues 8 words in the TC FIFO plus the word already being transmitted
 * for a total of 9, which is enough to overflow the RX FIFO.
 */
static int ep93xx_spi_read_write(struct ep93xx_spi *espi)
{
	struct spi_message *msg;
	struct spi_transfer *t;
	unsigned long flags;
	unsigned n;	/* Number of bytes to send to the TX FIFO */

	spin_lock_irqsave(&espi->lock, flags);
	msg = espi->current_msg;
	spin_unlock_irqrestore(&espi->lock, flags);

	t = msg->state;

	/* Read all received data */
	while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)
		ep93xx_do_read(espi, t);

	n = t->len - espi->tx;	/* Total number of bytes waiting to be
				 * sent to the TX FIFO for this transfer */

	switch (n) {
	default:
		/* This is the start or middle of a multi-word transfer:
		 * fill the TX FIFO as much as it is safe to do without
		 * risking RX overruns. To do this we ensure that the RX FIFO
		 * can end up with no more than 8 words in it by calculating
		 * how many words are in the RX/TX FIFOs and the TX/RX shift
		 * registers from the difference between the number of bytes
		 * we have already sent and received.
		 */
		{ unsigned words_in_flight =
			(espi->tx - espi->rx) / (bits_per_word(espi) > 8 ? 2 : 1);
		  unsigned space = 8 - words_in_flight;
		  if (n > space) n = space;
		}

		/* space and n are always >= 1 */
		do {
			ep93xx_do_write(espi, t);
		} while (--n > 0);
		break;

	case 1:
		/* Single-word transfers are common and not worth returning and
		 * causing another interrupt for. This also handles a single
		 * final word of a longer tranfer, which again is not worth
		 * causing another interrupt for.
		 * Just transmit it and wait for the reply.
		 */
		ep93xx_do_write(espi, t);
		/* Fall through to wait for and read the received word */
	case 0:
		/* All of this transfer's data has been sent to the TX FIFO.
		 * The TX and RX IRQs are now useless to tell us when the last
		 * few words have finished, so simply wait for the last 0-4
		 * words to be transmitted and for the corresponding RX data
		 * to come back in.
		 * We empty the RX FIFO while we are waiting, which is faster
		 * than first waiting and then emptying.
		 */
		{
			unsigned ssp_status;

			do {
				ssp_status = ep93xx_spi_read_u8(espi, SSPSR);
				if (ssp_status & SSPSR_RNE)
					ep93xx_do_read(espi, t);
			} while (ssp_status & SSPSR_BSY);
		}

		/* Finish emptying the RX FIFO. Even though we emptied it while
		 * waiting on BUSY, there can still be several (up to 3) words
		 * still waiting in it.  Not sure why!
		 */
		while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)
			ep93xx_do_read(espi, t);
		break;
	}

	/* is transfer finished? */
	if (espi->rx == t->len) {
		msg->actual_length += t->len;
		return t->len;
	}

	return 0;
}

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

Oops, I mean

-	unsigned n;	/* Number of bytes to send to the TX FIFO */
+	unsigned n;	/* Number of words to send to the TX FIFO */

and

-	n = t->len - espi->tx;	/* Total number of bytes waiting to be
-				 * sent to the TX FIFO for this transfer */
+	/* Number of words still to be sent to the TX FIFO */
+	n = (t->len - espi->tx) / (bits_per_word(espi) > 8 ? 2 : 1);

sorry about that.

    M

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Fri, Apr 09, 2010 at 06:56:49PM +0100, Martin Guy wrote:
> 
> you end up with:
> 
>         while (espi->tx < t->len) {
>                 ep93xx_do_write(espi, t);
> 
>                 while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_BSY);
> 
>                 ep93xx_do_read(espi, t);
>         }
> 
> This makes each interrupt routine take about 2ms to complete. I don't
> know if that's unacceptably long or not.

Yeah. It's probably bad thing to spend that much time in interrupt
handler.

> I've coded up a version that works differently, by filling the TX FIFO
> and returning from the IRQ routine - it then get interrupted again
> when the TX FIFO is half empty (and the RX FIFO is half full), at
> which point it empties and fills them
> and waits for another interrupt.
> The end of the transfer has to be handled specially because the
> interrupt is held high as long as the TX FIFO is half empty or less,
> so we have to busy-wait while the final four words are transmitted and
> received, concluding the transfer.
> 
> The result is an increase in MMC card read or write speed from 232kB/s
> to 315kB/s and a decrease in CPU usage from 100% to 55-60%, tested
> with plain 1GB SD card.
> A 2GB MLC card instead gets 323kB/sec in read and 319kB/sec in write (!).
> 
> SDHC cards don't work at all - they are rejected by the MMC-SPI driver
> (eror -38, ENOSYS: Function not implemented), the same as happens with
> the old CIrrus driver.
> 
> I've only changed the ep93xx_spi_read_write() function in your v2
> interrupting driver: the new version is below. Would you see if it
> works with your other SPI hardware?

I tested your version with few SD cards (one of them is SDHC) and
works without any problems (and performance is better). Also tested
with SPI EEPROM through at25 driver and it works well.

However, I would really like to have this code to be simpler and
to be suitable for polling as well. I wrote myself version that is
based on yours and also borrowed this fifo_level thingy from
amba-pl022 driver. I tested it with the same devices and performance
seems to be in par with your version.

Can you try following with your devices?

struct ep93xx_spi {
	...
	size_t	fifo_level;
};

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;

        if (espi->tx == 0 && espi->rx == 0)
                espi->fifo_level = 0;

        while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
                espi->rx < t->len) {
                ep93xx_do_read(espi, t);
                espi->fifo_level--;
        }

        while (espi->fifo_level < 8 && espi->tx < t->len) {
                ep93xx_do_write(espi, t);
                espi->fifo_level++;
        }

        /* is transfer finished? */
        if (espi->tx == t->len && espi->rx == t->len) {
                msg->actual_length += t->len;
                return t->len;
        }

        return 0;
}

Thanks,
MW

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

On 4/10/10, Mika Westerberg <mika.westerberg@iki.fi> wrote:
>  Can you try following with your devices?

Woo, nice one!
Same CPU usage (~57%) same throughput (315kb/s) and it's much more
elegant. :) Just using repeated interrupts to handle the last 4 words
instead of the nasty busy-wait loops seems fairer to other interrupts
too!

You can make it even smaller be removing some redundant checks
deriving from the fact that it's synchronous, so N writes always cause
N reads:

>         if (espi->tx == 0 && espi->rx == 0)
>                 espi->fifo_level = 0;

can be

         if (espi->tx == 0)
                 espi->fifo_level = 0;

,

>         while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
>                 espi->rx < t->len) {

         while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)) {

and

>         /* is transfer finished? */
>         if (espi->tx == t->len && espi->rx == t->len) {

         if (espi->rx == t->len) {

Cheers!

    M

[PATCH v2 1/3] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy]

and

-         if (espi->tx == 0 && espi->rx == 0)
-                 espi->fifo_level = 0;

since it's always created as 0 and is always reset to 0 by the end of a transfer

and if you like

          /* is transfer finished? */
-         if (espi->tx == t->len && espi->rx == t->len) {
+         if (espi->fifo_level == 0) {

to save a couple of instructions

    M