[PATCH] spi: Add support for the OpenCores SPI controller.

[PATCH] spi: Add support for the OpenCores SPI controller.

[Thierry Reding]

This patch adds a platform device driver that supports the OpenCores SPI
controller.

The driver expects two resources: an IORESOURCE_MEM resource defining the
core's memory-mapped registers and an IORESOURCE_IRQ for the associated
interrupt. It also requires a clock, "spi-master-clk", used to compute the
clock divider.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

---
 drivers/spi/Kconfig       |    5 +
 drivers/spi/Makefile      |    1 +
 drivers/spi/spioc.c       |  528 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spioc.h |   25 +++
 4 files changed, 559 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 83a185d..ff76d29 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -151,6 +151,11 @@ config SPI_MPC83xx
 	  technology. This driver uses a simple set of shift registers for data
 	  (opposed to the CPM based descriptor model).
 
+config SPI_OCORES
+	tristate "OpenCores SPI Controller"
+	help
+	  This enables using the OpenCores SPI controller.
+
 config SPI_OMAP_UWIRE
 	tristate "OMAP1 MicroWire"
 	depends on ARCH_OMAP1
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 5d04519..7802d0c 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
 obj-$(CONFIG_SPI_TXX9)			+= spi_txx9.o
 obj-$(CONFIG_SPI_XILINX)		+= xilinx_spi.o
 obj-$(CONFIG_SPI_SH_SCI)		+= spi_sh_sci.o
+obj-$(CONFIG_SPI_OCORES)		+= spioc.o
 # 	... add above this line ...
 
 # SPI protocol drivers (device/link on bus)
diff --git a/drivers/spi/spioc.c b/drivers/spi/spioc.c
new file mode 100644
index 0000000..2ec2c66
--- /dev/null
+++ b/drivers/spi/spioc.c
@@ -0,0 +1,528 @@
+/*
+ * linux/drivers/spi/spioc.c
+ *
+ * Copyright (C) 2007-2008 Avionic Design Development GmbH
+ * Copyright (C) 2008-2009 Avionic Design GmbH
+ *
+ * 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.
+ *
+ * Written by Thierry Reding <thierry.reding@avionic-design.de>
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spioc.h>
+
+/* register definitions */
+#define SPIOC_RX(i)	(i * 4)
+#define SPIOC_TX(i)	(i * 4)
+#define SPIOC_CTRL	0x10
+#define SPIOC_DIV	0x14
+#define SPIOC_SS	0x18
+
+/* SPIOC_CTRL register */
+#define CTRL_LEN(x)	((x < 128) ? x : 0)
+#define CTRL_BUSY	(1 <<  8)
+#define CTRL_RXNEG	(1 <<  9)
+#define CTRL_TXNEG	(1 << 10)
+#define CTRL_LSB	(1 << 11)
+#define CTRL_IE		(1 << 12)
+#define CTRL_ASS	(1 << 13)
+
+/**
+ * struct spioc - driver-specific context information
+ * @master:	SPI master device
+ * @info:	pointer to platform data
+ * @clk:	SPI master clock
+ * @irq:	SPI controller interrupt
+ * @mmio:	physical I/O memory resource
+ * @base:	base of memory-mapped I/O
+ * @message:	current SPI message
+ * @transfer:	current transfer of current SPI message
+ * @nx:		number of bytes sent/received for current transfer
+ * @queue:	SPI message queue
+ */
+struct spioc {
+	struct spi_master *master;
+	struct spioc_info *info;
+	struct clk *clk;
+	int irq;
+
+	struct resource *mmio;
+	void __iomem *base;
+
+	struct spi_message *message;
+	struct spi_transfer *transfer;
+	unsigned long nx;
+
+	struct list_head queue;
+	struct workqueue_struct *workqueue;
+	struct work_struct process_messages;
+	struct tasklet_struct process_transfers;
+	struct completion complete;
+	spinlock_t lock;
+};
+
+static inline u32 spioc_read(struct spioc *spioc, unsigned long offset)
+{
+	return ioread32(spioc->base + offset);
+}
+
+static inline void spioc_write(struct spioc *spioc, unsigned offset,
+		u32 value)
+{
+	iowrite32(value, spioc->base + offset);
+}
+
+static void spioc_chipselect(struct spioc *master, struct spi_device *spi)
+{
+	if (spi)
+		spioc_write(master, SPIOC_SS, 1 << spi->chip_select);
+	else
+		spioc_write(master, SPIOC_SS, 0);
+}
+
+/* count is assumed to be less than or equal to the maximum number of bytes
+ * that can be transferred in one go */
+static void spioc_copy_tx(struct spioc *spioc, const void *src, size_t count)
+{
+	u32 val = 0;
+	int i;
+
+	for (i = 0; i < count; i++) {
+		int rem = count - i;
+		int reg = (rem - 1) / 4;
+		int ofs = (rem - 1) % 4;
+
+		val |= (((u8 *)src)[i] & 0xff) << (ofs * 8);
+		if (!ofs) {
+			spioc_write(spioc, SPIOC_TX(reg), val);
+			val = 0;
+		}
+	}
+}
+
+static void spioc_copy_rx(struct spioc *spioc, void *dest, size_t count)
+{
+	u32 val = 0;
+	int i;
+
+	for (i = 0; i < count; i++) {
+		int rem = count - i;
+		int reg = (rem - 1) / 4;
+		int ofs = (rem - 1) % 4;
+
+		if ((i == 0) || (rem % 4 == 0))
+			val = spioc_read(spioc, SPIOC_RX(reg));
+
+		((u8 *)dest)[i] = (val >> (ofs * 8)) & 0xff;
+	}
+}
+
+static void process_messages(struct work_struct *work)
+{
+	struct spioc *spioc =
+		container_of(work, struct spioc, process_messages);
+	unsigned long flags;
+
+	spin_lock_irqsave(&spioc->lock, flags);
+
+	/* obtain next message */
+	if (list_empty(&spioc->queue)) {
+		spin_unlock_irqrestore(&spioc->lock, flags);
+		return;
+	}
+
+	spioc->message = list_entry(spioc->queue.next, struct spi_message,
+			queue);
+	list_del_init(&spioc->message->queue);
+
+	/* process transfers */
+	tasklet_schedule(&spioc->process_transfers);
+	spin_unlock_irqrestore(&spioc->lock, flags);
+}
+
+static void process_transfers(unsigned long data)
+{
+	struct spioc *spioc = (struct spioc *)data;
+	struct spi_transfer *transfer = spioc->transfer;
+	size_t rem;
+	u32 ctrl;
+
+	/* if this is the start of a message, get a pointer to the first
+	 * transfer */
+	if (!transfer || (spioc->nx >= transfer->len)) {
+		if (!transfer) {
+			transfer = list_entry(spioc->message->transfers.next,
+					struct spi_transfer, transfer_list);
+			spioc_chipselect(spioc, spioc->message->spi);
+		} else {
+			struct list_head *next = transfer->transfer_list.next;
+
+			if (next != &spioc->message->transfers) {
+				transfer = list_entry(next,
+						struct spi_transfer,
+						transfer_list);
+			} else {
+				spioc->message->actual_length = spioc->nx;
+				complete(spioc->message->context);
+				spioc->transfer = NULL;
+				spioc_chipselect(spioc, NULL);
+				return;
+			}
+		}
+
+		spioc->transfer = transfer;
+		spioc->nx = 0;
+	}
+
+	/* write data to registers */
+	rem = min_t(size_t, transfer->len - spioc->nx, 16);
+	if (transfer->tx_buf)
+		spioc_copy_tx(spioc, transfer->tx_buf + spioc->nx, rem);
+
+	/* read control register */
+	ctrl  = spioc_read(spioc, SPIOC_CTRL);
+	ctrl &= ~CTRL_LEN(127);    /* clear length bits */
+	ctrl |= CTRL_IE            /* assert interrupt on completion */
+	     |  CTRL_LEN(rem * 8); /* set word length */
+	spioc_write(spioc, SPIOC_CTRL, ctrl);
+
+	/* start transfer */
+	ctrl |= CTRL_BUSY;
+	spioc_write(spioc, SPIOC_CTRL, ctrl);
+}
+
+static int spioc_setup(struct spi_device *spi)
+{
+	struct spioc *spioc = spi_master_get_devdata(spi->master);
+	unsigned long clkdiv = 0x0000ffff;
+	u32 ctrl = spioc_read(spioc, SPIOC_CTRL);
+
+	/* make sure we're not busy */
+	ctrl &= ~CTRL_BUSY;
+
+	if (!spi->bits_per_word)
+		spi->bits_per_word = 8;
+
+	if (spi->mode & SPI_LSB_FIRST)
+		ctrl |=  CTRL_LSB;
+	else
+		ctrl &= ~CTRL_LSB;
+
+	/* adapt to clock polarity and phase */
+	if (spi->mode & SPI_CPOL) {
+		if (spi->mode & SPI_CPHA) {
+			ctrl |=  CTRL_TXNEG;
+			ctrl &= ~CTRL_RXNEG;
+		} else {
+			ctrl &= ~CTRL_TXNEG;
+			ctrl |=  CTRL_RXNEG;
+		}
+	} else {
+		if (spi->mode & SPI_CPHA) {
+			ctrl &= ~CTRL_TXNEG;
+			ctrl |=  CTRL_RXNEG;
+		} else {
+			ctrl |=  CTRL_TXNEG;
+			ctrl &= ~CTRL_RXNEG;
+		}
+	}
+
+	/* set the clock divider */
+	if (spi->max_speed_hz)
+		clkdiv = DIV_ROUND_UP(clk_get_rate(spioc->clk),
+				2 * spi->max_speed_hz) - 1;
+
+	if (clkdiv > 0x0000ffff)
+		clkdiv = 0x0000ffff;
+
+	spioc_write(spioc, SPIOC_DIV, clkdiv);
+	spioc_write(spioc, SPIOC_CTRL, ctrl);
+
+	/* deassert chip-select */
+	spioc_chipselect(spioc, NULL);
+
+	return 0;
+}
+
+static int spioc_transfer(struct spi_device *spi, struct spi_message *message)
+{
+	struct spi_master *master = spi->master;
+	struct spioc *spioc = spi_master_get_devdata(master);
+	unsigned long flags;
+
+	spin_lock_irqsave(&spioc->lock, flags);
+
+	list_add_tail(&message->queue, &spioc->queue);
+	queue_work(spioc->workqueue, &spioc->process_messages);
+
+	spin_unlock_irqrestore(&spioc->lock, flags);
+	return 0;
+}
+
+static void spioc_cleanup(struct spi_device *spi)
+{
+}
+
+static irqreturn_t spioc_interrupt(int irq, void *dev_id)
+{
+	struct spi_master *master = (struct spi_master *)dev_id;
+	struct spioc *spioc = spi_master_get_devdata(master);
+	struct spi_transfer *transfer = spioc->transfer;
+	size_t rem;
+
+	if (!transfer)
+		return IRQ_NONE;
+
+	/* read data from registers */
+	rem = min_t(size_t, transfer->len - spioc->nx, 16);
+	if (transfer->rx_buf)
+		spioc_copy_rx(spioc, transfer->rx_buf + spioc->nx, rem);
+	spioc->nx += rem;
+
+	tasklet_schedule(&spioc->process_transfers);
+	return IRQ_HANDLED;
+}
+
+static int init_queue(struct spi_master *master)
+{
+	struct spioc *spioc = spi_master_get_devdata(master);
+
+	INIT_LIST_HEAD(&spioc->queue);
+
+	/* initialize transfer processing tasklet */
+	tasklet_init(&spioc->process_transfers, process_transfers,
+			(unsigned long)spioc);
+
+	/* initialize message workqueue */
+	INIT_WORK(&spioc->process_messages, process_messages);
+	spioc->workqueue = create_singlethread_workqueue(
+			master->dev.bus_id);
+	if (!spioc->workqueue)
+		return -EBUSY;
+
+	return 0;
+}
+
+static int start_queue(struct spi_master *master)
+{
+	struct spioc *spioc = spi_master_get_devdata(master);
+
+	WARN_ON(spioc->message  != NULL);
+	WARN_ON(spioc->transfer != NULL);
+
+	spioc->message  = NULL;
+	spioc->transfer = NULL;
+
+	queue_work(spioc->workqueue, &spioc->process_messages);
+	return 0;
+}
+
+static int stop_queue(struct spi_master *master)
+{
+	return 0;
+}
+
+static int destroy_queue(struct spi_master *master)
+{
+	struct spioc *spioc = spi_master_get_devdata(master);
+	int retval = 0;
+
+	retval = stop_queue(master);
+	if (retval)
+		return retval;
+
+	destroy_workqueue(spioc->workqueue);
+	return 0;
+}
+
+static int __devinit spioc_probe(struct platform_device *pdev)
+{
+	struct resource *res = NULL;
+	void __iomem *mmio = NULL;
+	int retval = 0, irq;
+	struct spi_master *master = NULL;
+	struct spioc *spioc = NULL;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "MMIO resource not defined\n");
+		return -ENXIO;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "IRQ not defined\n");
+		return -ENXIO;
+	}
+
+	res = request_mem_region(res->start, res->end - res->start + 1,
+			res->name);
+	if (!res) {
+		dev_err(&pdev->dev, "unable to request memory region\n");
+		return -ENXIO;
+	}
+
+	mmio = ioremap_nocache(res->start, res->end - res->start + 1);
+	if (!mmio) {
+		dev_err(&pdev->dev, "can't remap I/O region\n");
+		retval = -ENXIO;
+		goto release;
+	}
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct spioc));
+	if (!master) {
+		dev_err(&pdev->dev, "unable to allocate SPI master\n");
+		retval = -ENOMEM;
+		goto unmap;
+	}
+
+	master->setup = spioc_setup;
+	master->transfer = spioc_transfer;
+	master->cleanup = spioc_cleanup;
+
+	spioc = spi_master_get_devdata(master);
+	if (!spioc) {
+		dev_err(&master->dev, "no controller data\n");
+		retval = -ENOMEM;
+		goto free;
+	}
+
+	spioc->master = master;
+	spioc->info = pdev->dev.platform_data;
+	spioc->irq = irq;
+	spioc->mmio = res;
+	spioc->base = mmio;
+	spioc->message = NULL;
+	spioc->transfer = NULL;
+	spin_lock_init(&spioc->lock);
+
+	spioc->clk = clk_get(NULL, "spi-master-clk");
+	if (IS_ERR(spioc->clk)) {
+		dev_err(&master->dev, "unable to get SPI master clock\n");
+		retval = PTR_ERR(spioc->clk);
+		spioc->clk = NULL;
+		goto free;
+	}
+
+	retval = init_queue(master);
+	if (retval) {
+		dev_err(&master->dev, "unable to initialize workqueue\n");
+		goto free;
+	}
+
+	retval = start_queue(master);
+	if (retval) {
+		dev_err(&master->dev, "unable to start workqueue\n");
+		goto free;
+	}
+
+	init_completion(&spioc->complete);
+
+	retval = request_irq(irq, spioc_interrupt, IRQF_SHARED, "spioc",
+			master);
+	if (retval) {
+		dev_err(&master->dev, "unable to install handler for "
+				"IRQ%d\n", irq);
+		goto free;
+	}
+
+	/* set SPI bus number and number of chipselects */
+	master->bus_num        = spioc->info->bus_num;
+	master->num_chipselect = spioc->info->num_chipselect;
+
+	retval = spi_register_master(master);
+	if (retval) {
+		dev_err(&master->dev, "unable to register SPI master\n");
+		goto free_irq;
+	}
+
+	dev_info(&master->dev, "SPI master registered\n");
+	platform_set_drvdata(pdev, master);
+
+out:
+	return retval;
+free_irq:
+	free_irq(irq, master);
+free:
+	(void)destroy_queue(master);
+	spi_master_put(master);
+
+	if (spioc && spioc->clk && !IS_ERR(spioc->clk))
+		clk_put(spioc->clk);
+unmap:
+	if (mmio)
+		iounmap(mmio);
+release:
+	if (res)
+		release_mem_region(res->start, res->end - res->start + 1);
+	goto out;
+}
+
+static int __devexit spioc_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct spioc *spioc = spi_master_get_devdata(master);
+	struct resource *res = spioc->mmio;
+
+	spi_master_get(master);
+	platform_set_drvdata(pdev, NULL);
+	spi_unregister_master(master);
+	destroy_queue(master);
+	free_irq(spioc->irq, master);
+	iounmap(spioc->base);
+	release_mem_region(res->start, res->end - res->start + 1);
+	clk_put(spioc->clk);
+	spi_master_put(master);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int spioc_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	return -ENOSYS;
+}
+
+static int spioc_resume(struct platform_device *pdev)
+{
+	return -ENOSYS;
+}
+#else
+#define spioc_suspend NULL
+#define spioc_resume  NULL
+#endif /* CONFIG_PM */
+
+static struct platform_driver spioc_driver = {
+	.probe = spioc_probe,
+	.remove = __devexit_p(spioc_remove),
+	.suspend = spioc_suspend,
+	.resume = spioc_resume,
+	.driver = {
+		.name  = "spioc",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init spioc_init(void)
+{
+	return platform_driver_register(&spioc_driver);
+}
+
+static void __exit spioc_exit(void)
+{
+	platform_driver_unregister(&spioc_driver);
+}
+
+module_init(spioc_init);
+module_exit(spioc_exit);
+
+MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>");
+MODULE_DESCRIPTION("OpenCores SPI controller driver");
+MODULE_LICENSE("GPL v2");
+
diff --git a/include/linux/spi/spioc.h b/include/linux/spi/spioc.h
new file mode 100644
index 0000000..1a4d7ad
--- /dev/null
+++ b/include/linux/spi/spioc.h
@@ -0,0 +1,25 @@
+/*
+ * linux/include/linux/spi/spioc.h
+ *
+ * Copyright (C) 2007-2008 Avionic Design Development GmbH
+ * Copyright (C) 2008-2009 Avionic Design GmbH
+ *
+ * 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.
+ *
+ * Written by Thierry Reding <thierry.reding@avionic-design.de>
+ */
+
+#ifndef LINUX_SPI_SPIOC_H
+#define LINUX_SPI_SPIOC_H
+
+#include <linux/spi/spi.h>
+
+struct spioc_info {
+	s16 bus_num;
+	u16 num_chipselect;
+};
+
+#endif /* !LINUX_SPI_SPIOC_H */
+
-- 
tg: (cb7b158..) adx/spi/spioc (depends on: adx/master)

Re: [PATCH] spi: Add support for the OpenCores SPI controller.

[Florian Fainelli]

Hi Thierry,

Le Thursday 26 March 2009 09:07:53 Thierry Reding, vous avez écrit :
> This patch adds a platform device driver that supports the OpenCores SPI
> controller.
>
> The driver expects two resources: an IORESOURCE_MEM resource defining the
> core's memory-mapped registers and an IORESOURCE_IRQ for the associated
> interrupt. It also requires a clock, "spi-master-clk", used to compute the
> clock divider.

Thanks again for submitting this ;)

>
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

Acked-by: Florian Fainelli <florian@openwrt.org>

-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

Re: [PATCH] spi: Add support for the OpenCores SPI controller.

[David Brownell]

Hmm ... Open Hardware, Free Software.  Sounds like a good match!

This driver has some all-too-common failings though:

 (a) doesn't correctly reject invalid device configurations
     in the setup() method

 (b) doesn't correctly reject unsupported per-message options
     in the transfer() method

 (c) the setup() method will trash concurrent transfers, since
     it wrongly assumes there's only one device

There are other minor nits too.


On Thursday 26 March 2009, Thierry Reding wrote:

> +struct spioc {
> +	struct spi_master *master;
> +	struct spioc_info *info;

Nothing in "info" is really needed after probe(); no
point in recording it here.


> static void process_transfers(unsigned long data)

... buggy (by inference), see below


> +static int spioc_setup(struct spi_device *spi)
> +{

Hmm, you seem to have disregarded the Documentation/spi/spi-summary
text describing what this does:

	Unless each SPI slave has its own configuration registers, don't
	change them right away ... otherwise drivers could corrupt I/O
	that's in progress for other SPI devices.

Because:


> +	struct spioc *spioc = spi_master_get_devdata(spi->master);
> +	unsigned long clkdiv = 0x0000ffff;
> +	u32 ctrl = spioc_read(spioc, SPIOC_CTRL);

CTRL is a controller-global register, not a per-chipselect one ...

> +
> +	/* make sure we're not busy */
> +	ctrl &= ~CTRL_BUSY;
> +
> +	if (!spi->bits_per_word)
> +		spi->bits_per_word = 8;
> +
> +	if (spi->mode & SPI_LSB_FIRST)
> +		ctrl |=  CTRL_LSB;
> +	else
> +		ctrl &= ~CTRL_LSB;
> +
> +	/* adapt to clock polarity and phase */
> +	if (spi->mode & SPI_CPOL) {
> +		if (spi->mode & SPI_CPHA) {
> +			ctrl |=  CTRL_TXNEG;
> +			ctrl &= ~CTRL_RXNEG;
> +		} else {
> +			ctrl &= ~CTRL_TXNEG;
> +			ctrl |=  CTRL_RXNEG;
> +		}
> +	} else {
> +		if (spi->mode & SPI_CPHA) {
> +			ctrl &= ~CTRL_TXNEG;
> +			ctrl |=  CTRL_RXNEG;
> +		} else {
> +			ctrl |=  CTRL_TXNEG;
> +			ctrl &= ~CTRL_RXNEG;
> +		}
> +	}

Notice how you're accepting *all* spi->mode bits, instead of
insisting that the only bits set there be ones you support.

Best that you do something along the lines of

/* mask of all SPI options this driver currently handles */
#define MODEBITS (SPI_CPOL|SPI_CPHA|SPI_LSB_FIRST)

	if (spi->mode & ~MODEBITS)
		return -EINVAL;

near the top of this routine.

> +
> +	/* set the clock divider */
> +	if (spi->max_speed_hz)
> +		clkdiv = DIV_ROUND_UP(clk_get_rate(spioc->clk),
> +				2 * spi->max_speed_hz) - 1;
> +
> +	if (clkdiv > 0x0000ffff)
> +		clkdiv = 0x0000ffff;

Shouldn't you just be failing setup() if the slowest clock rate
you support is still too fast for this peripheral?


> +
> +	spioc_write(spioc, SPIOC_DIV, clkdiv);
> +	spioc_write(spioc, SPIOC_CTRL, ctrl);

... here you reprogram parameters in use by the current
transfer.

Probably what you should do instead is compute the values to
be used for those two registers, and save them in a struct
referenced through spi_device.controller_state before you
start any individual transfer.

However, your process_transfers() currently seems to presume
a global setup model; that will need to change.  Minimally
you should set the transfer parameters for the current device.

And it will need to cope with per-transfer overrides for the
word size and bitrate, unless you reject them when messages
get submitted.


> +
> +	/* deassert chip-select */
> +	spioc_chipselect(spioc, NULL);
> +
> +	return 0;
> +}
> +
> +static int spioc_transfer(struct spi_device *spi, struct spi_message *message)
> +{
> +	struct spi_master *master = spi->master;
> +	struct spioc *spioc = spi_master_get_devdata(master);
> +	unsigned long flags;

Here you're accepting all messages, even ones that rely on
mechanisms you don't support.  You should either implement
the per-transfer wordsize, bitrate, and chipselect overrides;
or else scan the transfer list of this message to make sure
that no transfer requires them.

> +
> +	spin_lock_irqsave(&spioc->lock, flags);
> +
> +	list_add_tail(&message->queue, &spioc->queue);
> +	queue_work(spioc->workqueue, &spioc->process_messages);
> +
> +	spin_unlock_irqrestore(&spioc->lock, flags);
> +	return 0;
> +}
> +
> +static void spioc_cleanup(struct spi_device *spi)
> +{

This would be where you should kfree() the struct holding
per-device register settings that your setup() should allocate.

Having such NOP methods should be a sign that something
is wrong...

> +}


> +static int __devinit spioc_probe(struct platform_device *pdev)
> +{
> +	...
> +
> +	res = request_mem_region(res->start, res->end - res->start + 1,
> +			res->name);

I think resource_size(resource) should be used here and in the ioremap
call below.

> +	if (!res) {
> +		dev_err(&pdev->dev, "unable to request memory region\n");
> +		return -ENXIO;
> +	}
> +
> +	mmio = ioremap_nocache(res->start, res->end - res->start + 1);
> +	if (!mmio) {
> +		dev_err(&pdev->dev, "can't remap I/O region\n");
> +		retval = -ENXIO;
> +		goto release;
> +	}
> +	...


> +#ifdef CONFIG_PM
> +static int spioc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	return -ENOSYS;
> +}
> +
> +static int spioc_resume(struct platform_device *pdev)
> +{
> +	return -ENOSYS;
> +}

Why are you aborting system suspend transitions instead of
just not supporting PM?  Better IMO to depend on !PM if
this is trying to prevent nasty stuff from happening.


> +
> +struct spioc_info {
> +	s16 bus_num;

Better IMO to just use platform_device.id for this.

> +	u16 num_chipselect;
> +};
> +
> +#endif /* !LINUX_SPI_SPIOC_H */
> +
> 

Re: [PATCH] spi: Add support for the OpenCores SPI controller.

[David Brownell]

Hmm ... Open Hardware, Free Software.  Sounds like a good match!

This driver has some all-too-common failings though:

 (a) doesn't correctly reject invalid device configurations
     in the setup() method

 (b) doesn't correctly reject unsupported per-message options
     in the transfer() method

 (c) the setup() method will trash concurrent transfers, since
     it wrongly assumes there's only one device

There are other minor nits too.


On Thursday 26 March 2009, Thierry Reding wrote:

> +struct spioc {
> +	struct spi_master *master;
> +	struct spioc_info *info;

Nothing in "info" is really needed after probe(); no
point in recording it here.


> static void process_transfers(unsigned long data)

... buggy (by inference), see below


> +static int spioc_setup(struct spi_device *spi)
> +{

Hmm, you seem to have disregarded the Documentation/spi/spi-summary
text describing what this does:

	Unless each SPI slave has its own configuration registers, don't
	change them right away ... otherwise drivers could corrupt I/O
	that's in progress for other SPI devices.

Because:


> +	struct spioc *spioc = spi_master_get_devdata(spi->master);
> +	unsigned long clkdiv = 0x0000ffff;
> +	u32 ctrl = spioc_read(spioc, SPIOC_CTRL);

CTRL is a controller-global register, not a per-chipselect one ...

> +
> +	/* make sure we're not busy */
> +	ctrl &= ~CTRL_BUSY;
> +
> +	if (!spi->bits_per_word)
> +		spi->bits_per_word = 8;
> +
> +	if (spi->mode & SPI_LSB_FIRST)
> +		ctrl |=  CTRL_LSB;
> +	else
> +		ctrl &= ~CTRL_LSB;
> +
> +	/* adapt to clock polarity and phase */
> +	if (spi->mode & SPI_CPOL) {
> +		if (spi->mode & SPI_CPHA) {
> +			ctrl |=  CTRL_TXNEG;
> +			ctrl &= ~CTRL_RXNEG;
> +		} else {
> +			ctrl &= ~CTRL_TXNEG;
> +			ctrl |=  CTRL_RXNEG;
> +		}
> +	} else {
> +		if (spi->mode & SPI_CPHA) {
> +			ctrl &= ~CTRL_TXNEG;
> +			ctrl |=  CTRL_RXNEG;
> +		} else {
> +			ctrl |=  CTRL_TXNEG;
> +			ctrl &= ~CTRL_RXNEG;
> +		}
> +	}

Notice how you're accepting *all* spi->mode bits, instead of
insisting that the only bits set there be ones you support.

Best that you do something along the lines of

/* mask of all SPI options this driver currently handles */
#define MODEBITS (SPI_CPOL|SPI_CPHA|SPI_LSB_FIRST)

	if (spi->mode & ~MODEBITS)
		return -EINVAL;

near the top of this routine.

> +
> +	/* set the clock divider */
> +	if (spi->max_speed_hz)
> +		clkdiv = DIV_ROUND_UP(clk_get_rate(spioc->clk),
> +				2 * spi->max_speed_hz) - 1;
> +
> +	if (clkdiv > 0x0000ffff)
> +		clkdiv = 0x0000ffff;

Shouldn't you just be failing setup() if the slowest clock rate
you support is still too fast for this peripheral?


> +
> +	spioc_write(spioc, SPIOC_DIV, clkdiv);
> +	spioc_write(spioc, SPIOC_CTRL, ctrl);

... here you reprogram parameters in use by the current
transfer.

Probably what you should do instead is compute the values to
be used for those two registers, and save them in a struct
referenced through spi_device.controller_state before you
start any individual transfer.

However, your process_transfers() currently seems to presume
a global setup model; that will need to change.  Minimally
you should set the transfer parameters for the current device.

And it will need to cope with per-transfer overrides for the
word size and bitrate, unless you reject them when messages
get submitted.


> +
> +	/* deassert chip-select */
> +	spioc_chipselect(spioc, NULL);
> +
> +	return 0;
> +}
> +
> +static int spioc_transfer(struct spi_device *spi, struct spi_message *message)
> +{
> +	struct spi_master *master = spi->master;
> +	struct spioc *spioc = spi_master_get_devdata(master);
> +	unsigned long flags;

Here you're accepting all messages, even ones that rely on
mechanisms you don't support.  You should either implement
the per-transfer wordsize, bitrate, and chipselect overrides;
or else scan the transfer list of this message to make sure
that no transfer requires them.

> +
> +	spin_lock_irqsave(&spioc->lock, flags);
> +
> +	list_add_tail(&message->queue, &spioc->queue);
> +	queue_work(spioc->workqueue, &spioc->process_messages);
> +
> +	spin_unlock_irqrestore(&spioc->lock, flags);
> +	return 0;
> +}
> +
> +static void spioc_cleanup(struct spi_device *spi)
> +{

This would be where you should kfree() the struct holding
per-device register settings that your setup() should allocate.

Having such NOP methods should be a sign that something
is wrong...

> +}


> +static int __devinit spioc_probe(struct platform_device *pdev)
> +{
> +	...
> +
> +	res = request_mem_region(res->start, res->end - res->start + 1,
> +			res->name);

I think resource_size(resource) should be used here and in the ioremap
call below.

> +	if (!res) {
> +		dev_err(&pdev->dev, "unable to request memory region\n");
> +		return -ENXIO;
> +	}
> +
> +	mmio = ioremap_nocache(res->start, res->end - res->start + 1);
> +	if (!mmio) {
> +		dev_err(&pdev->dev, "can't remap I/O region\n");
> +		retval = -ENXIO;
> +		goto release;
> +	}
> +	...


> +#ifdef CONFIG_PM
> +static int spioc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	return -ENOSYS;
> +}
> +
> +static int spioc_resume(struct platform_device *pdev)
> +{
> +	return -ENOSYS;
> +}

Why are you aborting system suspend transitions instead of
just not supporting PM?  Better IMO to depend on !PM if
this is trying to prevent nasty stuff from happening.


> +
> +struct spioc_info {
> +	s16 bus_num;

Better IMO to just use platform_device.id for this.

> +	u16 num_chipselect;
> +};
> +
> +#endif /* !LINUX_SPI_SPIOC_H */
> +
> 

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

[PATCH v2] spi: Add support for the OpenCores SPI controller.

[Thierry Reding]

This patch adds a platform device driver that supports the OpenCores SPI
controller.

The driver expects two resources: an IORESOURCE_MEM resource defining the
core's memory-mapped registers and an IORESOURCE_IRQ for the associated
interrupt. It also requires a clock, "spi-master-clk", used to compute the
clock divider.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

---
 drivers/spi/Kconfig  |    5 +
 drivers/spi/Makefile |    1 +
 drivers/spi/spioc.c  |  633 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 639 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 83a185d..ff76d29 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -151,6 +151,11 @@ config SPI_MPC83xx
 	  technology. This driver uses a simple set of shift registers for data
 	  (opposed to the CPM based descriptor model).
 
+config SPI_OCORES
+	tristate "OpenCores SPI Controller"
+	help
+	  This enables using the OpenCores SPI controller.
+
 config SPI_OMAP_UWIRE
 	tristate "OMAP1 MicroWire"
 	depends on ARCH_OMAP1
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 5d04519..7802d0c 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
 obj-$(CONFIG_SPI_TXX9)			+= spi_txx9.o
 obj-$(CONFIG_SPI_XILINX)		+= xilinx_spi.o
 obj-$(CONFIG_SPI_SH_SCI)		+= spi_sh_sci.o
+obj-$(CONFIG_SPI_OCORES)		+= spioc.o
 # 	... add above this line ...
 
 # SPI protocol drivers (device/link on bus)
diff --git a/drivers/spi/spioc.c b/drivers/spi/spioc.c
new file mode 100644
index 0000000..231d43c
--- /dev/null
+++ b/drivers/spi/spioc.c
@@ -0,0 +1,633 @@
+/*
+ * linux/drivers/spi/spioc.c
+ *
+ * Copyright (C) 2007-2008 Avionic Design Development GmbH
+ * Copyright (C) 2008-2009 Avionic Design GmbH
+ *
+ * Partially inspired by code from linux/drivers/spi/pxa2xx_spi.c.
+ *
+ * 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.
+ *
+ * Written by Thierry Reding <thierry.reding@avionic-design.de>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+
+/* register definitions */
+#define	SPIOC_RX(i)	(i * 4)
+#define	SPIOC_TX(i)	(i * 4)
+#define	SPIOC_CTRL	0x10
+#define	SPIOC_DIV	0x14
+#define	SPIOC_SS	0x18
+
+/* SPIOC_CTRL register */
+#define	CTRL_LEN(x)	((x < 128) ? x : 0)
+#define	CTRL_BUSY	(1 <<  8)
+#define	CTRL_RXNEG	(1 <<  9)
+#define	CTRL_TXNEG	(1 << 10)
+#define	CTRL_LSB	(1 << 11)
+#define	CTRL_IE		(1 << 12)
+#define	CTRL_ASS	(1 << 13)
+
+#define	SPIOC_MAX_LEN	((unsigned int)16)
+
+static const u32 clock_mode[4] = {
+	[SPI_MODE_0] = CTRL_TXNEG,
+	[SPI_MODE_1] = CTRL_RXNEG,
+	[SPI_MODE_2] = CTRL_TXNEG,
+	[SPI_MODE_3] = CTRL_RXNEG,
+};
+
+/* valid SPI mode bits */
+#define	MODEBITS	(SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST)
+
+struct spioc_ctldata {
+	u32 ctrl;
+	u32 div;
+};
+
+struct spioc {
+	struct spi_master *master;
+	void __iomem *base;
+	struct clk *clk;
+	int irq;
+
+	struct workqueue_struct *workqueue;
+	struct work_struct work;
+	struct tasklet_struct tasklet;
+
+	struct list_head queue;
+	struct completion msg_done;
+	unsigned int state;
+	unsigned int busy;
+	spinlock_t lock;
+
+	struct spi_device *slave;
+	struct spi_message *message;
+	struct spi_transfer *transfer;
+	unsigned int cur_pos;
+	unsigned int cur_len;
+};
+
+/* queue states */
+#define	QUEUE_STOPPED	0
+#define	QUEUE_RUNNING	1
+
+static inline u32 spioc_read(struct spioc *spioc, unsigned long offset)
+{
+	return ioread32(spioc->base + offset);
+}
+
+static inline void spioc_write(struct spioc *spioc, unsigned offset,
+		u32 value)
+{
+	iowrite32(value, spioc->base + offset);
+}
+
+static u32 spioc_get_clkdiv(struct spioc *spioc, unsigned long speed)
+{
+	unsigned long rate = clk_get_rate(spioc->clk);
+	return DIV_ROUND_UP(rate, 2 * speed) - 1;
+}
+
+static void spioc_chipselect(struct spioc *spioc, struct spi_device *spi)
+{
+	if (spi != spioc->slave) {
+		u32 ss = spi ? (1 << spi->chip_select) : 0;
+		spioc_write(spioc, SPIOC_SS, ss);
+		spioc->slave = spi;
+	}
+}
+
+static void spioc_copy_tx(struct spioc *spioc)
+{
+	const void *src;
+	u32 val = 0;
+	int i;
+
+	if (!spioc->transfer->tx_buf)
+		return;
+
+	src = spioc->transfer->tx_buf + spioc->cur_pos;
+
+	for (i = 0; i < spioc->cur_len; i++) {
+		int rem = spioc->cur_len - i;
+		int reg = (rem - 1) / 4;
+		int ofs = (rem - 1) % 4;
+
+		val |= (((u8 *)src)[i] & 0xff) << (ofs * 8);
+		if (!ofs) {
+			spioc_write(spioc, SPIOC_TX(reg), val);
+			val = 0;
+		}
+	}
+}
+
+static void spioc_copy_rx(struct spioc *spioc)
+{
+	void *dest;
+	u32 val = 0;
+	int i;
+
+	if (!spioc->transfer->rx_buf)
+		return;
+
+	dest = spioc->transfer->rx_buf + spioc->cur_pos;
+
+	for (i = 0; i < spioc->cur_len; i++) {
+		int rem = spioc->cur_len - i;
+		int reg = (rem - 1) / 4;
+		int ofs = (rem - 1) % 4;
+
+		if ((i == 0) || (rem % 4 == 0))
+			val = spioc_read(spioc, SPIOC_RX(reg));
+
+		((u8 *)dest)[i] = (val >> (ofs * 8)) & 0xff;
+	}
+}
+
+static inline struct spi_transfer *next_transfer(struct list_head *head)
+{
+	return list_entry(head->next, struct spi_transfer, transfer_list);
+}
+
+static void finish_message(struct spioc *spioc, int ec)
+{
+	struct spi_message *message = spioc->message;
+
+	spioc->transfer = NULL;
+	spioc->message = NULL;
+
+	message->status = ec;
+
+	if (message->complete)
+		message->complete(message->context);
+}
+
+static void queue_message(struct spioc *spioc)
+{
+	if (spioc->state == QUEUE_RUNNING)
+		queue_work(spioc->workqueue, &spioc->work);
+
+	if (spioc->state == QUEUE_STOPPED)
+		complete(&spioc->msg_done);
+}
+
+static struct spi_transfer *continue_message(struct spioc *spioc)
+{
+	struct spi_transfer *transfer = spioc->transfer;
+	struct spi_message *message = spioc->message;
+	unsigned long flags;
+
+	if (!transfer)
+		return next_transfer(&message->transfers);
+
+	if (transfer->transfer_list.next != &message->transfers)
+		return next_transfer(&transfer->transfer_list);
+
+	spin_lock_irqsave(&spioc->lock, flags);
+	finish_message(spioc, 0);
+	queue_message(spioc);
+	spin_unlock_irqrestore(&spioc->lock, flags);
+
+	return NULL;
+}
+
+static void process_transfers(unsigned long data)
+{
+	struct spioc *spioc = (struct spioc *)data;
+	struct spi_transfer *transfer;
+	struct spi_message *message;
+	struct spioc_ctldata *ctl;
+	u32 ctrl = 0;
+	u32 div = 0;
+
+	WARN_ON(spioc->message == NULL);
+	message = spioc->message;
+	transfer = spioc->transfer;
+
+	/* finish up the last partial transfer */
+	if (transfer) {
+		spioc_copy_rx(spioc);
+		spioc->message->actual_length += spioc->cur_len;
+		spioc->cur_pos += spioc->cur_len;
+	}
+
+	/* proceed to next (or first) transfer in message */
+	if (!transfer || (spioc->cur_pos >= transfer->len)) {
+		if (transfer) {
+			if (transfer->delay_usecs)
+				udelay(transfer->delay_usecs);
+
+			if (!transfer->cs_change)
+				spioc_chipselect(spioc, NULL);
+		}
+
+		transfer = continue_message(spioc);
+		if (!transfer)
+			return;
+
+		spioc->transfer = transfer;
+		spioc->cur_pos = 0;
+		spioc->cur_len = 0;
+	}
+
+	ctl = spi_get_ctldata(message->spi);
+	div = ctl->div;
+
+	if (transfer->speed_hz) {
+		div = spioc_get_clkdiv(spioc, transfer->speed_hz);
+		if (div > 0xffff) {
+			finish_message(spioc, -EIO);
+			return;
+		}
+	}
+
+	spioc->cur_len = min(transfer->len - spioc->cur_pos, SPIOC_MAX_LEN);
+	spioc_copy_tx(spioc);
+
+	ctrl = ctl->ctrl;
+	ctrl |= CTRL_LEN(spioc->cur_len * 8);
+	ctrl |= CTRL_BUSY;
+	ctrl |= CTRL_IE;
+
+	spioc_chipselect(spioc, spioc->message->spi);
+	spioc_write(spioc, SPIOC_DIV, div);
+	spioc_write(spioc, SPIOC_CTRL, ctrl);
+}
+
+static void process_messages(struct work_struct *work)
+{
+	struct spioc *spioc = container_of(work, struct spioc, work);
+	struct spi_message *message;
+	unsigned long flags;
+
+	WARN_ON(spioc->message != NULL);
+
+	spin_lock_irqsave(&spioc->lock, flags);
+
+	if (list_empty(&spioc->queue)) {
+		spioc->busy = 0;
+		spin_unlock_irqrestore(&spioc->lock, flags);
+		return;
+	}
+
+	message = list_entry(spioc->queue.next, struct spi_message, queue);
+	list_del_init(&message->queue);
+	spioc->message = message;
+	tasklet_schedule(&spioc->tasklet);
+	spioc->busy = 1;
+
+	spin_unlock_irqrestore(&spioc->lock, flags);
+}
+
+static int spioc_setup(struct spi_device *spi)
+{
+	struct spioc *spioc = spi_master_get_devdata(spi->master);
+	struct spioc_ctldata *ctl = spi_get_ctldata(spi);
+	u32 div = 0;
+
+	if (spi->mode & ~MODEBITS)
+		return -EINVAL;
+
+	if (!spi->max_speed_hz)
+		return -EINVAL;
+
+	div = spioc_get_clkdiv(spioc, spi->max_speed_hz);
+	if (div > 0xffff)
+		return -EINVAL;
+
+	if (!ctl) {
+		ctl = kzalloc(sizeof(*ctl), GFP_KERNEL);
+		if (!ctl)
+			return -EINVAL;
+
+		spi_set_ctldata(spi, ctl);
+	}
+
+	ctl->div = div;
+	ctl->ctrl = 0;
+
+	if (spi->mode & SPI_LSB_FIRST)
+		ctl->ctrl |= CTRL_LSB;
+
+	ctl->ctrl |= clock_mode[spi->mode & 0x3];
+
+	return 0;
+}
+
+static int spioc_transfer(struct spi_device *spi, struct spi_message *message)
+{
+	struct spioc *spioc = spi_master_get_devdata(spi->master);
+	unsigned long flags = 0;
+
+	spin_lock_irqsave(&spioc->lock, flags);
+
+	if (spioc->state == QUEUE_STOPPED) {
+		spin_unlock_irqrestore(&spioc->lock, flags);
+		return -ESHUTDOWN;
+	}
+
+	message->status = -EINPROGRESS;
+	message->actual_length = 0;
+
+	list_add_tail(&message->queue, &spioc->queue);
+
+	if ((spioc->state == QUEUE_RUNNING) && !spioc->busy)
+		queue_work(spioc->workqueue, &spioc->work);
+
+	spin_unlock_irqrestore(&spioc->lock, flags);
+
+	return 0;
+}
+
+static void spioc_cleanup(struct spi_device *spi)
+{
+	struct spioc_ctldata *ctl = spi_get_ctldata(spi);
+	spi_set_ctldata(spi, NULL);
+	kfree(ctl);
+}
+
+static irqreturn_t spioc_interrupt(int irq, void *dev_id)
+{
+	struct spi_master *master = dev_id;
+	struct spioc *spioc;
+
+	if (!dev_id)
+		return IRQ_NONE;
+
+	spioc = spi_master_get_devdata(master);
+	tasklet_schedule(&spioc->tasklet);
+
+	return IRQ_HANDLED;
+}
+
+static int init_queue(struct spi_master *master)
+{
+	struct spioc *spioc = spi_master_get_devdata(master);
+
+	spioc->workqueue = create_workqueue(dev_name(&master->dev));
+	if (!spioc->workqueue)
+		return -ENOMEM;
+
+	INIT_WORK(&spioc->work, process_messages);
+	tasklet_init(&spioc->tasklet, process_transfers,
+			(unsigned long)spioc);
+
+	INIT_LIST_HEAD(&spioc->queue);
+	init_completion(&spioc->msg_done);
+	spin_lock_init(&spioc->lock);
+
+	spioc->state = QUEUE_STOPPED;
+	spioc->busy = 0;
+
+	return 0;
+}
+
+static int start_queue(struct spi_master *master)
+{
+	struct spioc *spioc = spi_master_get_devdata(master);
+	unsigned long flags;
+
+	spin_lock_irqsave(&spioc->lock, flags);
+
+	if ((spioc->state == QUEUE_RUNNING) || spioc->busy) {
+		spin_unlock_irqrestore(&spioc->lock, flags);
+		return -EBUSY;
+	}
+
+	spioc->state = QUEUE_RUNNING;
+	spioc->message = NULL;
+	spioc->transfer = NULL;
+
+	spin_unlock_irqrestore(&spioc->lock, flags);
+
+	queue_work(spioc->workqueue, &spioc->work);
+	return 0;
+}
+
+static int stop_queue(struct spi_master *master)
+{
+	struct spioc *spioc = spi_master_get_devdata(master);
+	unsigned long flags;
+	unsigned int empty;
+	unsigned int busy;
+
+	spin_lock_irqsave(&spioc->lock, flags);
+
+	empty = list_empty(&spioc->queue);
+	spioc->state = QUEUE_STOPPED;
+	busy = spioc->busy;
+
+	spin_unlock_irqrestore(&spioc->lock, flags);
+
+	if (!empty && busy)
+		wait_for_completion(&spioc->msg_done);
+
+	return 0;
+}
+
+static int destroy_queue(struct spi_master *master)
+{
+	struct spioc *spioc = spi_master_get_devdata(master);
+	int ret;
+
+	ret = stop_queue(master);
+	if (ret < 0)
+		return ret;
+
+	destroy_workqueue(spioc->workqueue);
+
+	return 0;
+}
+
+static int __devinit spioc_probe(struct platform_device *pdev)
+{
+	struct resource *res = NULL;
+	int irq = 0;
+	void __iomem *mmio = NULL;
+	struct spi_master *master = NULL;
+	struct spioc *spioc = NULL;
+	int ret = 0;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "MMIO resource not defined\n");
+		return -ENXIO;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "IRQ not defined\n");
+		return -ENXIO;
+	}
+
+	res = devm_request_mem_region(&pdev->dev, res->start,
+			resource_size(res), res->name);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to request memory region\n");
+		return -ENXIO;
+	}
+
+	mmio = devm_ioremap_nocache(&pdev->dev, res->start,
+			resource_size(res));
+	if (!mmio) {
+		dev_err(&pdev->dev, "failed to remap I/O region\n");
+		ret = -ENXIO;
+		goto free;
+	}
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct spioc));
+	if (!master) {
+		dev_err(&pdev->dev, "failed to allocate SPI master\n");
+		ret = -ENOMEM;
+		goto free;
+	}
+
+	master->setup = spioc_setup;
+	master->transfer = spioc_transfer;
+	master->cleanup = spioc_cleanup;
+
+	spioc = spi_master_get_devdata(master);
+	if (!spioc) {
+		ret = -ENXIO;
+		goto free;
+	}
+
+	spioc->master = master;
+	spioc->base = mmio;
+	spioc->irq = irq;
+	spioc->slave = NULL;
+
+	ret = init_queue(master);
+	if (ret < 0) {
+		dev_err(&master->dev, "failed to initialize message queue\n");
+		goto free;
+	}
+
+	ret = start_queue(master);
+	if (ret < 0) {
+		dev_err(&master->dev, "failed to start message queue\n");
+		goto free;
+	}
+
+	spioc->clk = clk_get(NULL, "spi-master-clk");
+	if (IS_ERR(spioc->clk)) {
+		dev_err(&master->dev, "SPI master clock undefined\n");
+		ret = PTR_ERR(spioc->clk);
+		spioc->clk = NULL;
+		goto free;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, spioc_interrupt, IRQF_SHARED,
+			"spioc", master);
+	if (ret) {
+		dev_err(&master->dev, "failed to install handler for "
+				"IRQ%d\n", irq);
+		goto put_clock;
+	}
+
+	/* set SPI bus number and number of chipselects */
+	master->bus_num = pdev->id;
+	master->num_chipselect = 8;
+
+	ret = spi_register_master(master);
+	if (ret) {
+		dev_err(&master->dev, "failed to register SPI master\n");
+		goto put_clock;
+	}
+
+	platform_set_drvdata(pdev, master);
+	return ret;
+
+put_clock:
+	clk_put(spioc->clk);
+free:
+	spi_master_put(master);
+	return ret;
+}
+
+static int __devexit spioc_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct spioc *spioc = spi_master_get_devdata(master);
+
+	spi_master_get(master);
+	platform_set_drvdata(pdev, NULL);
+	spi_unregister_master(master);
+	destroy_queue(master);
+	clk_put(spioc->clk);
+	spi_master_put(master);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int spioc_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct spi_master *master;
+	int ret = 0;
+
+	master = platform_get_drvdata(pdev);
+
+	ret = stop_queue(master);
+	if (ret < 0)
+		dev_err(&master->dev, "failed to stop queue\n");
+
+	return ret;
+}
+
+static int spioc_resume(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	int ret = 0;
+
+	master = platform_get_drvdata(pdev);
+
+	ret = start_queue(master);
+	if (ret < 0)
+		dev_err(&master->dev, "failed to start queue\n");
+
+	return ret;
+}
+#else
+#define spioc_suspend NULL
+#define spioc_resume  NULL
+#endif /* CONFIG_PM */
+
+static struct platform_driver spioc_driver = {
+	.probe = spioc_probe,
+	.remove = __devexit_p(spioc_remove),
+	.suspend = spioc_suspend,
+	.resume = spioc_resume,
+	.driver = {
+		.name  = "spioc",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init spioc_init(void)
+{
+	return platform_driver_register(&spioc_driver);
+}
+
+static void __exit spioc_exit(void)
+{
+	platform_driver_unregister(&spioc_driver);
+}
+
+module_init(spioc_init);
+module_exit(spioc_exit);
+
+MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>");
+MODULE_DESCRIPTION("OpenCores SPI controller driver");
+MODULE_LICENSE("GPL v2");
+
-- 
tg: (f2deb5b..) adx/spi/spioc (depends on: adx/master)

Re: [spi-devel-general] [PATCH v2] spi: Add support for the OpenCores SPI controller.

[Thierry Reding]

* Thierry Reding wrote:
> This patch adds a platform device driver that supports the OpenCores SPI
> controller.
> 
> The driver expects two resources: an IORESOURCE_MEM resource defining the
> core's memory-mapped registers and an IORESOURCE_IRQ for the associated
> interrupt. It also requires a clock, "spi-master-clk", used to compute the
> clock divider.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
[snip]

This second version is pretty much a rewrite. Some notes about the most
important changes:

  * uses per-chip states to allow more slaves to use the controller
    concurrently
  * rejects invalid device configurations during setup
  * rejects invalid per-message and per-transfer options
  * queues messages so that they can be processed one after another
      - this also provides for a way to handle power-management
  * omits the spioc.h (and with it the platform data structure):
      - uses the platform_device.id for the bus number
      - always uses 8 chipselects because that's the maximum that the core
        supports

I couldn't really find a way to implement per-transfer overrides for the
word size because the controller simply has no concept of word sizes. Is it
in such cases still necessary to hardwire the word size to 8 bits?

Thierry

Re: [PATCH v2] spi: Add support for the OpenCores SPI controller.

[Thierry Reding]

* Thierry Reding wrote:
> This patch adds a platform device driver that supports the OpenCores SPI
> controller.
> 
> The driver expects two resources: an IORESOURCE_MEM resource defining the
> core's memory-mapped registers and an IORESOURCE_IRQ for the associated
> interrupt. It also requires a clock, "spi-master-clk", used to compute the
> clock divider.
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
[snip]

This second version is pretty much a rewrite. Some notes about the most
important changes:

  * uses per-chip states to allow more slaves to use the controller
    concurrently
  * rejects invalid device configurations during setup
  * rejects invalid per-message and per-transfer options
  * queues messages so that they can be processed one after another
      - this also provides for a way to handle power-management
  * omits the spioc.h (and with it the platform data structure):
      - uses the platform_device.id for the bus number
      - always uses 8 chipselects because that's the maximum that the core
        supports

I couldn't really find a way to implement per-transfer overrides for the
word size because the controller simply has no concept of word sizes. Is it
in such cases still necessary to hardwire the word size to 8 bits?

Thierry


------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf

Re: [spi-devel-general] [PATCH v2] spi: Add support for the OpenCores SPI controller.

[David Brownell]

On Tuesday 28 April 2009, Thierry Reding wrote:
> This second version is pretty much a rewrite.

That happens sometimes...


> Some notes about the most 
> important changes:
> 
>   * uses per-chip states to allow more slaves to use the controller
>     concurrently
>   * rejects invalid device configurations during setup
>   * rejects invalid per-message and per-transfer options
>   * queues messages so that they can be processed one after another
>       - this also provides for a way to handle power-management
>   * omits the spioc.h (and with it the platform data structure):
>       - uses the platform_device.id for the bus number
>       - always uses 8 chipselects because that's the maximum that the core
>         supports

All that sounds good.

 
> I couldn't really find a way to implement per-transfer overrides for the
> word size because the controller simply has no concept of word sizes. Is it
> in such cases still necessary to hardwire the word size to 8 bits?

Is this the http://www.opencores.org/?do=project&who=spi core?
Its summary says "Variable length of transfer word up to 32 bits";
does that mean "configurable when core is synthesized" instead of
truly "variable"?

Re: [PATCH v2] spi: Add support for the OpenCores SPI controller.

[David Brownell]

On Tuesday 28 April 2009, Thierry Reding wrote:
> This second version is pretty much a rewrite.

That happens sometimes...


> Some notes about the most 
> important changes:
> 
>   * uses per-chip states to allow more slaves to use the controller
>     concurrently
>   * rejects invalid device configurations during setup
>   * rejects invalid per-message and per-transfer options
>   * queues messages so that they can be processed one after another
>       - this also provides for a way to handle power-management
>   * omits the spioc.h (and with it the platform data structure):
>       - uses the platform_device.id for the bus number
>       - always uses 8 chipselects because that's the maximum that the core
>         supports

All that sounds good.

 
> I couldn't really find a way to implement per-transfer overrides for the
> word size because the controller simply has no concept of word sizes. Is it
> in such cases still necessary to hardwire the word size to 8 bits?

Is this the http://www.opencores.org/?do=project&who=spi core?
Its summary says "Variable length of transfer word up to 32 bits";
does that mean "configurable when core is synthesized" instead of
truly "variable"?

------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf

Re: [spi-devel-general] [PATCH v2] spi: Add support for the OpenCores SPI controller.

[Thierry Reding]

* David Brownell wrote:
> On Tuesday 28 April 2009, Thierry Reding wrote:
> > This second version is pretty much a rewrite.
> 
> That happens sometimes...
> 
> 
> > Some notes about the most 
> > important changes:
> > 
> >   * uses per-chip states to allow more slaves to use the controller
> >     concurrently
> >   * rejects invalid device configurations during setup
> >   * rejects invalid per-message and per-transfer options
> >   * queues messages so that they can be processed one after another
> >       - this also provides for a way to handle power-management
> >   * omits the spioc.h (and with it the platform data structure):
> >       - uses the platform_device.id for the bus number
> >       - always uses 8 chipselects because that's the maximum that the core
> >         supports
> 
> All that sounds good.
> 
>  
> > I couldn't really find a way to implement per-transfer overrides for the
> > word size because the controller simply has no concept of word sizes. Is it
> > in such cases still necessary to hardwire the word size to 8 bits?
> 
> Is this the http://www.opencores.org/?do=project&who=spi core?

Yes, it is.

> Its summary says "Variable length of transfer word up to 32 bits";
> does that mean "configurable when core is synthesized" instead of
> truly "variable"?

That summary seems out-dated. The variable length of transfer word is
actually the maximum length of a single transfer and is 128 bits in the
latest version. So you get 4 registers, each 32 bits wide into which you
program the data you want to transfer. Then you set the number of bits of
that transfer so the core knows which registers and what bits of those
registers to shift out serially.

I'm not sure whether this is supposed to be the same as the word size. If it
is it would mean that a single transfer can always only transfer one word.
Which is kind of inefficient, I would think.

Thierry

Re: [spi-devel-general] [PATCH v2] spi: Add support for the OpenCores SPI controller.

[Florian Fainelli]

Le Tuesday 28 April 2009 14:20:11 Thierry Reding, vous avez écrit :
> * David Brownell wrote:
> > On Tuesday 28 April 2009, Thierry Reding wrote:
> > > This second version is pretty much a rewrite.
> >
> > That happens sometimes...
> >
> > > Some notes about the most
> > > important changes:
> > >
> > >   * uses per-chip states to allow more slaves to use the controller
> > >     concurrently
> > >   * rejects invalid device configurations during setup
> > >   * rejects invalid per-message and per-transfer options
> > >   * queues messages so that they can be processed one after another
> > >       - this also provides for a way to handle power-management
> > >   * omits the spioc.h (and with it the platform data structure):
> > >       - uses the platform_device.id for the bus number
> > >       - always uses 8 chipselects because that's the maximum that the
> > > core supports
> >
> > All that sounds good.
> >
> > > I couldn't really find a way to implement per-transfer overrides for
> > > the word size because the controller simply has no concept of word
> > > sizes. Is it in such cases still necessary to hardwire the word size to
> > > 8 bits?
> >
> > Is this the http://www.opencores.org/?do=project&who=spi core?
>
> Yes, it is.
>
> > Its summary says "Variable length of transfer word up to 32 bits";
> > does that mean "configurable when core is synthesized" instead of
> > truly "variable"?

This is indeed configured at synthesis time.

>
> That summary seems out-dated. The variable length of transfer word is
> actually the maximum length of a single transfer and is 128 bits in the
> latest version. So you get 4 registers, each 32 bits wide into which you
> program the data you want to transfer. Then you set the number of bits of
> that transfer so the core knows which registers and what bits of those
> registers to shift out serially.
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

Re: [spi-devel-general] [PATCH v2] spi: Add support for the OpenCores SPI controller.

[David Brownell]

On Tuesday 28 April 2009, Florian Fainelli wrote:
> > > Is this the http://www.opencores.org/?do=project&who=spi core?
> >
> > Yes, it is.
> >
> > > Its summary says "Variable length of transfer word up to 32 bits";
> > > does that mean "configurable when core is synthesized" instead of
> > > truly "variable"?
> 
> This is indeed configured at synthesis time.

Now I'm confused again.  Thierry says (below) that the number of bits
can be set per-"transfer".

Now, I can easily understand that a *maximum* would be configured
at synthesis time ... if there's a 32-bit CPU or DMA engine, it'd
make very limited sense to interact using 128-bit I/O words.

Is there both a configurable maximum, *and* a word-size setting that
can be changed on the fly?  That's what I would expect; it's what
most other designs do.  The only time I've seen fixed "you must use
N-bit words" designs is on cost-eradicated 8-bit microcontrollers.

- Dave


> > That summary seems out-dated. The variable length of transfer word is
> > actually the maximum length of a single transfer and is 128 bits in the
> > latest version. So you get 4 registers, each 32 bits wide into which you
> > program the data you want to transfer. Then you set the number of bits of
> > that transfer so the core knows which registers and what bits of those
> > registers to shift out serially.

Re: [spi-devel-general] [PATCH v2] spi: Add support for the OpenCores SPI controller.

[David Brownell]

On Tuesday 28 April 2009, Thierry Reding wrote:
> > > I couldn't really find a way to implement per-transfer overrides for the
> > > word size because the controller simply has no concept of word sizes. Is it
> > > in such cases still necessary to hardwire the word size to 8 bits?
> > 
> > Is this the http://www.opencores.org/?do=project&who=spi core?
> 
> Yes, it is.
> 
> > Its summary says "Variable length of transfer word up to 32 bits";
> > does that mean "configurable when core is synthesized" instead of
> > truly "variable"?
> 
> That summary seems out-dated. The variable length of transfer word is
> actually the maximum length of a single transfer and is 128 bits in the
> latest version.

So long as they don't couple "transfer" with chipselect activation
and then de-activation, that's normal.

128 bits is pretty big, but it should make no difference to the slave
whether the host thinks of its data as one 128-bit word, sixteen 8-bit
words, one 9-bit word followed by a 119-bit one, or whatever.

Unless the design is broken, so that you can't send words without
flapping the chipselect.  That would surprise me.


> I'm not sure whether this is supposed to be the same as the word size. If it
> is it would mean that a single transfer can always only transfer one word.
> Which is kind of inefficient, I would think.

A "struct spi_transfer" should include a arbitrary number of
such words.  If the word size is over 8 bits, all the usual
byte ordering concerns come into play.  You may optimize the
register I/O however you like, so long as the bits on the
wire come out in the right sequence.

Ignoring clock options, the canonical SPI transfer starts by
activating a chip select, then clocking out an arbitrary number
of bits (clocking *in* one bit for each one clocked out), and
then de-activating chipselect.  Those bits are usually viewed
as a sequence of various-size words ... not necesarily all
the same size.  Example, some LCD controllers use 9-bit command
words followed by pixel data encoded in bytes.

Now, how the bits get to/from the controller is an area where
silicon can optimize.  For example, it's common to offload
that work to a DMA controller that can do burst operations
to keep the data bus efficiency high ... and to have a FIFO
in there, so those bursts can be bigger than the word size.

- Dave

Re: [PATCH v2] spi: Add support for the OpenCores SPI controller.

[David Brownell]

On Tuesday 28 April 2009, Thierry Reding wrote:
> > > I couldn't really find a way to implement per-transfer overrides for the
> > > word size because the controller simply has no concept of word sizes. Is it
> > > in such cases still necessary to hardwire the word size to 8 bits?
> > 
> > Is this the http://www.opencores.org/?do=project&who=spi core?
> 
> Yes, it is.
> 
> > Its summary says "Variable length of transfer word up to 32 bits";
> > does that mean "configurable when core is synthesized" instead of
> > truly "variable"?
> 
> That summary seems out-dated. The variable length of transfer word is
> actually the maximum length of a single transfer and is 128 bits in the
> latest version.

So long as they don't couple "transfer" with chipselect activation
and then de-activation, that's normal.

128 bits is pretty big, but it should make no difference to the slave
whether the host thinks of its data as one 128-bit word, sixteen 8-bit
words, one 9-bit word followed by a 119-bit one, or whatever.

Unless the design is broken, so that you can't send words without
flapping the chipselect.  That would surprise me.


> I'm not sure whether this is supposed to be the same as the word size. If it
> is it would mean that a single transfer can always only transfer one word.
> Which is kind of inefficient, I would think.

A "struct spi_transfer" should include a arbitrary number of
such words.  If the word size is over 8 bits, all the usual
byte ordering concerns come into play.  You may optimize the
register I/O however you like, so long as the bits on the
wire come out in the right sequence.

Ignoring clock options, the canonical SPI transfer starts by
activating a chip select, then clocking out an arbitrary number
of bits (clocking *in* one bit for each one clocked out), and
then de-activating chipselect.  Those bits are usually viewed
as a sequence of various-size words ... not necesarily all
the same size.  Example, some LCD controllers use 9-bit command
words followed by pixel data encoded in bytes.

Now, how the bits get to/from the controller is an area where
silicon can optimize.  For example, it's common to offload
that work to a DMA controller that can do burst operations
to keep the data bus efficiency high ... and to have a FIFO
in there, so those bursts can be bigger than the word size.

- Dave

------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf

Re: [spi-devel-general] [PATCH v2] spi: Add support for the OpenCores SPI controller.

[Thierry Reding]

* David Brownell wrote:
> On Tuesday 28 April 2009, Thierry Reding wrote:
> > > > I couldn't really find a way to implement per-transfer overrides for the
> > > > word size because the controller simply has no concept of word sizes. Is it
> > > > in such cases still necessary to hardwire the word size to 8 bits?
> > > 
> > > Is this the http://www.opencores.org/?do=project&who=spi core?
> > 
> > Yes, it is.
> > 
> > > Its summary says "Variable length of transfer word up to 32 bits";
> > > does that mean "configurable when core is synthesized" instead of
> > > truly "variable"?
> > 
> > That summary seems out-dated. The variable length of transfer word is
> > actually the maximum length of a single transfer and is 128 bits in the
> > latest version.
> 
> So long as they don't couple "transfer" with chipselect activation
> and then de-activation, that's normal.
> 
> 128 bits is pretty big, but it should make no difference to the slave
> whether the host thinks of its data as one 128-bit word, sixteen 8-bit
> words, one 9-bit word followed by a 119-bit one, or whatever.
> 
> Unless the design is broken, so that you can't send words without
> flapping the chipselect.  That would surprise me.

So far, what I've been doing is just copy the transmission buffer in blocks
of 16 bytes byte-by-byte to the transmission registers (concatenating groups
of four bytes), then start the transfer. After the transfer I copy the bytes
back in the same manner to the receive buffer. I was assuming that the
buffers would be in the correct byte order at that point anyway.

This core actually allows manually setting the chipselect. It also has a mode
that automatically sets or clears the chipselect for each single "transfer"
(meaning up to those 128 bits). But as you say, that wouldn't be normal
behavior and breaks things.

> > I'm not sure whether this is supposed to be the same as the word size. If it
> > is it would mean that a single transfer can always only transfer one word.
> > Which is kind of inefficient, I would think.
> 
> A "struct spi_transfer" should include a arbitrary number of
> such words.  If the word size is over 8 bits, all the usual
> byte ordering concerns come into play.  You may optimize the
> register I/O however you like, so long as the bits on the
> wire come out in the right sequence.

What byte ordering are the transmit and receive buffers supposed to be in,
then? Native? Always big endian?

> Ignoring clock options, the canonical SPI transfer starts by
> activating a chip select, then clocking out an arbitrary number
> of bits (clocking *in* one bit for each one clocked out), and
> then de-activating chipselect.  Those bits are usually viewed
> as a sequence of various-size words ... not necesarily all
> the same size.  Example, some LCD controllers use 9-bit command
> words followed by pixel data encoded in bytes.
> 
> Now, how the bits get to/from the controller is an area where
> silicon can optimize.  For example, it's common to offload
> that work to a DMA controller that can do burst operations
> to keep the data bus efficiency high ... and to have a FIFO
> in there, so those bursts can be bigger than the word size.

I haven't experimented with using DMA transfers to this particular core
because there's currently no support for the generic DMA API on the CPU
we use.

> - Dave

Thierry

Re: [spi-devel-general] [PATCH v2] spi: Add support for the OpenCores SPI controller.

[Thierry Reding]

* David Brownell wrote:
> On Tuesday 28 April 2009, Florian Fainelli wrote:
> > > > Is this the http://www.opencores.org/?do=project&who=spi core?
> > >
> > > Yes, it is.
> > >
> > > > Its summary says "Variable length of transfer word up to 32 bits";
> > > > does that mean "configurable when core is synthesized" instead of
> > > > truly "variable"?
> > 
> > This is indeed configured at synthesis time.
> 
> Now I'm confused again.  Thierry says (below) that the number of bits
> can be set per-"transfer".
> 
> Now, I can easily understand that a *maximum* would be configured
> at synthesis time ... if there's a 32-bit CPU or DMA engine, it'd
> make very limited sense to interact using 128-bit I/O words.

I can't really comment on the synthesis because I'm not involved with that
part. What I was saying that the core provides a field in the control
register which defines the number of bits to transfer from/to the
transmit/receive registers. The maximum number of bits that can be specified
in this way is 128.

> Is there both a configurable maximum, *and* a word-size setting that
> can be changed on the fly?  That's what I would expect; it's what
> most other designs do.  The only time I've seen fixed "you must use
> N-bit words" designs is on cost-eradicated 8-bit microcontrollers.

Perhaps that maximum number of bits that can be set through the control
register is what can be configured at synthesis time.

> - Dave

Thierry

Re: [spi-devel-general] [PATCH v2] spi: Add support for the OpenCores SPI controller.

[Florian Fainelli]

Le Wednesday 29 April 2009 08:31:04 Thierry Reding, vous avez écrit :
> * David Brownell wrote:
> > On Tuesday 28 April 2009, Florian Fainelli wrote:
> > > > > Is this the http://www.opencores.org/?do=project&who=spi core?
> > > >
> > > > Yes, it is.
> > > >
> > > > > Its summary says "Variable length of transfer word up to 32 bits";
> > > > > does that mean "configurable when core is synthesized" instead of
> > > > > truly "variable"?
> > >
> > > This is indeed configured at synthesis time.
> >
> > Now I'm confused again.  Thierry says (below) that the number of bits
> > can be set per-"transfer".
> >
> > Now, I can easily understand that a *maximum* would be configured
> > at synthesis time ... if there's a 32-bit CPU or DMA engine, it'd
> > make very limited sense to interact using 128-bit I/O words.

The maximum size of the FIFO is configured at synthesis time should have made 
this clear before, sorry for the confusion.

>
> I can't really comment on the synthesis because I'm not involved with that
> part. What I was saying that the core provides a field in the control
> register which defines the number of bits to transfer from/to the
> transmit/receive registers. The maximum number of bits that can be
> specified in this way is 128.

Yes, which matches the FIFO size configured in the IP at synthesis time.

>
> > Is there both a configurable maximum, *and* a word-size setting that
> > can be changed on the fly?  That's what I would expect; it's what
> > most other designs do.  The only time I've seen fixed "you must use
> > N-bit words" designs is on cost-eradicated 8-bit microcontrollers.
>
> Perhaps that maximum number of bits that can be set through the control
> register is what can be configured at synthesis time.

Provided that your FIFO is 128-bits, you can of course ask the core to do up 
to the FIFO-size transfers for instance provided that you do not exceed the 
size of the FIFO. The later can obviously not be changed on-the-fly since 
physical resources of the FPGA for this should be reserved at synthesis time. 
Of course, one could use partial reconfiguration to increase the size, but 
that's slightly off-topic ;)
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------