Re: [PATCH] TXx9 SPI controller driver

Re: [PATCH] TXx9 SPI controller driver

[David Brownell]

On Friday 22 June 2007, Atsushi Nemoto wrote:
> This is a driver for SPI controller built into TXx9 MIPS SoCs.

Looks mostly pretty good.  I made a few minor changes/cleanups
in the appended version, notably:
 - checking for spi->mode bits this code doesn't understand;
 - updating to match latest patches;

Note that if gpio_set_value() needs an mmiowb(), that seems like
a bug in this platform's  GPIO code; other platforms don't require
I/O barriers after GPIO calls.  Comments?

Also:

> +	/* calc real speed */
> +	n = (c->baseclk + spi->max_speed_hz - 1) / spi->max_speed_hz;
> +	if (n < 1)
> +		n = 1;
> +	else if (n > 0xff)
> +		return -EINVAL;
> +	spi->max_speed_hz = c->baseclk / n;

That's not right -- given the current API definitions.  The max_speed_hz
value should be read-only to controller drivers.

On the other hand, I've also wondered how the actual rate should be
reported to drivers, and that approach might be a reasonable solution
to that problem.  Can you discuss this proposed change on the spi
development list?



> +static int txx9spi_work_one(struct txx9spi *c, struct spi_message *m)
> +{
> +	...
> +	nsecs = 100 + 1000000000 / spi->max_speed_hz / 2;

... which is why we can't just strike that line changing max_speed_hz.
But please use only a single division in that expression, and explain
what the value is used for.  (A good variable name would do wonders.
Is this "nsecs per fortnight", or what?)

I suspect "nsecs" and the actual bitrate should probably be stored
with the other spi->controller_state data.


> +	list_for_each_entry (t, &m->transfers, transfer_list) {
> +		const void *txbuf = t->tx_buf;
> +		void *rxbuf = t->rx_buf;
> +		u32 data;
> +		unsigned int len = t->len;
> +		unsigned int wsize = spi->bits_per_word >> 3; /* in bytes */

But on the other hand, here you're ignoring t->max_speed_hz as
well as t->bits_per_word.  If you do that, you must check those
values in txx9spi_transfer() and reject message which include
transfers using those per-transfer overrides.

Given that this controller only seems to allow 8 or 16 bit
transfers, you're going to need checks in the transfer() path
even if you do support the per-transfer overrides someday.


> +		if ((!txbuf && !rxbuf && len) || (len & (wsize - 1))) {

That's confusing.  But it's also something that belongs with
per-transfer checks -- doing it here is needlessly late.


> +			status = -EINVAL;
> +			break;
> +		}
> +		if (cs_change) {
> +			txx9spi_cs_func(spi, 1);
> +			ndelay(nsecs);

Since cs_change is initialized to one, you're always delaying
after chipselect.  Now, it happens to be true that with CPOL=0
there must be a half clock delay between setting the MOSI value
and starting the first clock.  And maybe adding an extra delay
isn't harmful if CPOL=1.  But again -- comment please.  Say why
this is done in software when one would think the hardware would
do such things automatically.  There's a chip erratum??


> +		}
> +		cs_change = t->cs_change;
> +		while (len) {
> +			unsigned int count = SPI_FIFO_SIZE;
> +			int i;
> +			u32 cr0;
> +			if (len < count * wsize)
> +				count = len / wsize;
> +			/* now tx must be idle... */
> +			while (!(txx9spi_rd(c, TXx9_SPSR) & TXx9_SPSR_SIDLE))
> +				;
> +			cr0 = txx9spi_rd(c, TXx9_SPCR0);
> +			cr0 &= ~TXx9_SPCR0_RXIFL_MASK;
> +			cr0 |= (count - 1) << 12;
> +			/* enable rx intr */
> +			cr0 |= TXx9_SPCR0_RBSIE;
> +			txx9spi_wr(c, cr0, TXx9_SPCR0);
> +			/* send */
> +			for (i = 0; i < count; i++) {
> +				data = txbuf ? (wsize == 1 ?
> +						*(const u8 *)txbuf :
> +						*(const u16 *)txbuf) : 0;

Double "?:" expressions == confusing.  Please use at most one.

> +				txx9spi_wr(c, data, TXx9_SPDR);
> +				if (txbuf)
> +					txbuf += wsize;
> +			}
> +			/* wait all rx data */
> +			wait_event(c->waitq,
> +				   txx9spi_rd(c, TXx9_SPSR) & TXx9_SPSR_RBSI);
> +			/* receive */
> +			for (i = 0; i < count; i++) {
> +				data = txx9spi_rd(c, TXx9_SPDR);
> +				if (rxbuf) {
> +					if (wsize == 1)
> +						*(u8 *)rxbuf = data;
> +					else
> +						*(u16 *)rxbuf = data;
> +					rxbuf += wsize;
> +				}
> +			}
> +			len -= count * wsize;
> +		}
> +		m->actual_length += t->len;
> +		if (t->delay_usecs)
> +			udelay(t->delay_usecs);
> +
> +		if (!cs_change)
> +			continue;
> +		if (t->transfer_list.next == &m->transfers)
> +			break;
> +		/* sometimes a short mid-message deselect of the chip
> +		 * may be needed to terminate a mode or command
> +		 */

Admittedly the length of the deselect isn't specified, but this
looks goofy.  Just deselect immediately, and let driver use the
delay_usecs value if they need more.  Then if you want to stay
deselected for at least one full clock then try

	ndelay(min(2*nsec, 1000))

to more closely match other drivers' usage of udelay(1) here.


> +		ndelay(nsecs);
> +		txx9spi_cs_func(spi, 0);
> +		ndelay(nsecs);
> +	}
> +
> +	m->status = status;
> +	m->complete(m->context);
> +
> +	/* normally deactivate chipselect ... unless no error and
> +	 * cs_change has hinted that the next message will probably
> +	 * be for this chip too.
> +	 */
> +	if (!(status == 0 && cs_change)) {
> +		ndelay(nsecs);

Again:  if drivers need an extra delay, that's what delay_usec is for.

> +		txx9spi_cs_func(spi, 0);
> +		ndelay(nsecs);

... and I don't see why this delay is needed at all.  If your
hardware needs extra delays around chipselect toggles, surely
that should be built into your cs_func()?

But it looks to me like you have a bug here too.  You're trying
to implement this behavioral hint, which is fine, but you're
not recording that you did it.  So if the next message goes
to a different chip, both chips will be selected at the same
time -- right?  Bug... quickest for you to ignore this hint.


> +	}
> +
> +	/* enter config mode */
> +	txx9spi_wr(c, mcr | TXx9_SPMCR_CONFIG | TXx9_SPMCR_BCLR, TXx9_SPMCR);
> +	return status;
> +}
> +


> +
> +static int __init txx9spi_probe(struct platform_device *dev)
> +{
> +	struct spi_master *master;
> +	struct txx9spi *c;
> +	struct resource *res;
> +	int ret = -ENODEV;
> +	u32 mcr;
> +
> +	master = spi_alloc_master(&dev->dev, sizeof(*c));
> +	if (!master)
> +		return ret;
> +	c = spi_master_get_devdata(master);
> +	platform_set_drvdata(dev, master);
> +
> +	INIT_WORK(&c->work, txx9spi_work);
> +	spin_lock_init(&c->lock);
> +	INIT_LIST_HEAD(&c->queue);
> +	init_waitqueue_head(&c->waitq);
> +
> +	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		goto put_and_exit;
> +	if (res->start >= TXX9_DIRECTMAP_BASE)
> +		c->membase = (void __iomem *)(unsigned long)(int)res->start;
> +	else {
> +		c->membase = ioremap(res->start, res->end - res->start + 1);
> +		c->mapped = 1;
> +	}

That looks plain wrong.  Maybe it reflects a platform-level bug,
but ioremap(res->start) should Just Work even when it performs
an identity mapping on a given system.  Remove this ugly code.
Always map.


> +
> +	/* enter config mode */
> +	mcr = txx9spi_rd(c, TXx9_SPMCR);
> +	mcr &= ~(TXx9_SPMCR_OPMODE | TXx9_SPMCR_SPSTP | TXx9_SPMCR_BCLR);
> +	txx9spi_wr(c, mcr | TXx9_SPMCR_CONFIG | TXx9_SPMCR_BCLR, TXx9_SPMCR);
> +
> +	c->irq = platform_get_irq(dev, 0);
> +	if (c->irq < 0)
> +		goto res_and_exit;
> +	c->baseclk = platform_get_irq_byname(dev, "baseclk");
> +	if (c->baseclk < 0)
> +		goto res_and_exit;

Yeech!!   If you're getting a clock, then clk_get(dev, "baseclk").
That's very clearly not an IRQ.  If you need values that don't fit
into the current resource framework, either extend that framework
or use platform_data to pass the values.

In this case, extending the ioresource framework would be the wrong
answer, since the clock framework is there to handle clocks.  If
this platform doesn't support the clock framework, then you must
use platform_data to pass such nonstandard clock data.



> +	ret = request_irq(c->irq, txx9spi_interrupt, 0, dev->name, c);
> +	if (ret)
> +		goto res_and_exit;
> +
> +	c->workqueue = create_singlethread_workqueue(master->cdev.dev->bus_id);
> +	if (!c->workqueue)
> +		goto irq_and_exit;
> +
> +	dev_info(&dev->dev, "at 0x%llx, irq %d, %dMHz\n",
> +		 (unsigned long long)res->start, c->irq,
> +		 (c->baseclk + 500000) / 1000000);
> +
> +	master->bus_num = dev->id;
> +	master->setup = txx9spi_setup;
> +	master->transfer = txx9spi_transfer;
> +	master->cleanup = txx9spi_cleanup;

Something else that seems incorrect here is the lack of setup
for master->num_chipselect.  It seems that you're using the
spi->chip_select value as a GPIO number.  That's unusual, but
not wrong.  Just set num_chipselect to the number of GPIOs on
the platform.  Otherwise, just map from chipselect to GPIO
like the other drivers do.  Such mappings fit naturally into
platform_data.

The fact that this works at all is a temporary bug in the
SPI core.

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] TXx9 SPI controller driver

[David Brownell]

On Friday 22 June 2007, David Brownell wrote:
> On Friday 22 June 2007, Atsushi Nemoto wrote:
> > This is a driver for SPI controller built into TXx9 MIPS SoCs.
> 
> Looks mostly pretty good.  I made a few minor changes/cleanups
> in the appended version, notably:
>  - checking for spi->mode bits this code doesn't understand;
>  - updating to match latest patches;
> 
> Note that if gpio_set_value() needs an mmiowb(), that seems like
> a bug in this platform's  GPIO code; other platforms don't require
> I/O barriers after GPIO calls.  Comments?
> 
> Also:
> 

... yeah, -ENOPATCH, sorry.  And the minor whitespace fixes.

One more comment:  surely platform_driver_probe() would be
appropriate here?

=======	CUT HERE
From: Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org>

This is a driver for SPI controller built into TXx9 MIPS SoCs.
This driver is derived from arch/mips/tx4938/toshiba_rbtx4938/spi_txx9.c.

Signed-off-by: Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org>
---
 drivers/spi/Kconfig    |    6 
 drivers/spi/Makefile   |    1 
 drivers/spi/spi_txx9.c |  434 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 441 insertions(+)

--- g26.orig/drivers/spi/Kconfig	2007-06-20 20:15:51.000000000 -0700
+++ g26/drivers/spi/Kconfig	2007-06-22 09:01:03.000000000 -0700
@@ -175,6 +175,12 @@ config SPI_S3C24XX_GPIO
 	  the inbuilt hardware cannot provide the transfer mode, or
 	  where the board is using non hardware connected pins.
 
+config SPI_TXX9
+	tristate "Toshiba TXx9 SPI controller"
+	depends on SPI_MASTER && GENERIC_GPIO && CPU_TX49XX
+	help
+	  SPI driver for Toshiba TXx9 MIPS SoCs
+
 config SPI_XILINX
 	tristate "Xilinx SPI controller"
 	depends on SPI_MASTER && XILINX_VIRTEX && EXPERIMENTAL
--- g26.orig/drivers/spi/Makefile	2007-06-20 20:15:50.000000000 -0700
+++ g26/drivers/spi/Makefile	2007-06-22 09:00:34.000000000 -0700
@@ -24,6 +24,7 @@ obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52x
 obj-$(CONFIG_SPI_MPC83xx)		+= spi_mpc83xx.o
 obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
+obj-$(CONFIG_SPI_TXX9)			+= spi_txx9.o
 obj-$(CONFIG_SPI_XILINX)		+= xilinx_spi.o
 # 	... add above this line ...
 
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ g26/drivers/spi/spi_txx9.c	2007-06-22 11:48:28.000000000 -0700
@@ -0,0 +1,434 @@
+/*
+ * spi_tx99.c - TXx9 SPI controller driver.
+ *
+ * Based on linux/arch/mips/tx4938/toshiba_rbtx4938/spi_txx9.c
+ * Copyright (C) 2000-2001 Toshiba Corporation
+ *
+ * 2003-2005 (c) MontaVista Software, Inc. This file is licensed under the
+ * terms of the GNU General Public License version 2. This program is
+ * licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ *
+ * Support for TX4938 in 2.6 - Manish Lachwani (mlachwani-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org)
+ *
+ * Convert to generic SPI framework - Atsushi Nemoto (anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org)
+ */
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+#include <linux/spi/spi.h>
+#include <asm/gpio.h>
+
+
+#define SPI_FIFO_SIZE 4
+
+#define TXx9_SPMCR		0x00
+#define TXx9_SPCR0		0x04
+#define TXx9_SPCR1		0x08
+#define TXx9_SPFS		0x0c
+#define TXx9_SPSR		0x14
+#define TXx9_SPDR		0x18
+
+/* SPMCR : SPI Master Control */
+#define TXx9_SPMCR_OPMODE	0xc0
+#define TXx9_SPMCR_CONFIG	0x40
+#define TXx9_SPMCR_ACTIVE	0x80
+#define TXx9_SPMCR_SPSTP	0x02
+#define TXx9_SPMCR_BCLR		0x01
+
+/* SPCR0 : SPI Control 0 */
+#define TXx9_SPCR0_TXIFL_MASK	0xc000
+#define TXx9_SPCR0_RXIFL_MASK	0x3000
+#define TXx9_SPCR0_SIDIE	0x0800
+#define TXx9_SPCR0_SOEIE	0x0400
+#define TXx9_SPCR0_RBSIE	0x0200
+#define TXx9_SPCR0_TBSIE	0x0100
+#define TXx9_SPCR0_IFSPSE	0x0010
+#define TXx9_SPCR0_SBOS		0x0004
+#define TXx9_SPCR0_SPHA		0x0002
+#define TXx9_SPCR0_SPOL		0x0001
+
+/* SPSR : SPI Status */
+#define TXx9_SPSR_TBSI		0x8000
+#define TXx9_SPSR_RBSI		0x4000
+#define TXx9_SPSR_TBS_MASK	0x3800
+#define TXx9_SPSR_RBS_MASK	0x0700
+#define TXx9_SPSR_SPOE		0x0080
+#define TXx9_SPSR_IFSD		0x0008
+#define TXx9_SPSR_SIDLE		0x0004
+#define TXx9_SPSR_STRDY		0x0002
+#define TXx9_SPSR_SRRDY		0x0001
+
+
+struct txx9spi {
+	struct workqueue_struct	*workqueue;
+	struct work_struct work;
+	spinlock_t lock;	/* protect 'queue' */
+	struct list_head queue;
+	wait_queue_head_t waitq;
+	void __iomem *membase;
+	int mapped;
+	int irq;
+	int baseclk;
+};
+
+struct txx9spi_cs {
+	u32 cr1;
+};
+
+static u32 txx9spi_rd(struct txx9spi *c, int reg)
+{
+	return __raw_readl(c->membase + reg);
+}
+static void txx9spi_wr(struct txx9spi *c, u32 val, int reg)
+{
+	__raw_writel(val, c->membase + reg);
+}
+
+static void txx9spi_cs_func(struct spi_device *spi, int on)
+{
+	if (spi->mode & SPI_CS_HIGH)
+		on = !on;
+	gpio_set_value(spi->chip_select, !on);
+	mmiowb();
+}
+
+/* the spi->mode bits understood by this driver: */
+#define MODEBITS	(SPI_CS_HIGH|SPI_CPOL|SPI_CPHA)
+
+static int txx9spi_setup(struct spi_device *spi)
+{
+	struct txx9spi *c = spi_master_get_devdata(spi->master);
+	struct txx9spi_cs *cs = spi->controller_state;
+	unsigned int n;
+
+	if (spi->mode & ~MODEBITS)
+		return -EINVAL;
+
+	if (!spi->max_speed_hz)
+		return -EINVAL;
+
+	if (gpio_direction_output(spi->chip_select,
+				  !(spi->mode & SPI_CS_HIGH))) {
+		dev_err(&spi->dev, "Cannot setup GPIO for chipselect.\n");
+		return -EINVAL;
+	}
+
+	if (!cs) {
+		cs = kmalloc(sizeof(*cs), GFP_KERNEL);
+		if (!cs)
+			return -ENOMEM;
+		spi->controller_state = cs;
+	}
+
+	if (spi->bits_per_word == 0)
+		spi->bits_per_word = 8;
+	else if (spi->bits_per_word != 8 && spi->bits_per_word != 16)
+		return -EINVAL;
+
+	/* calc real speed */
+	n = (c->baseclk + spi->max_speed_hz - 1) / spi->max_speed_hz;
+	if (n < 1)
+		n = 1;
+	else if (n > 0xff)
+		return -EINVAL;
+	spi->max_speed_hz = c->baseclk / n;
+
+	cs->cr1 = (n << 8) | spi->bits_per_word;
+	return 0;
+}
+
+static irqreturn_t txx9spi_interrupt(int irq, void *dev_id)
+{
+	struct txx9spi *c = dev_id;
+
+	/* disable rx intr */
+	txx9spi_wr(c, txx9spi_rd(c, TXx9_SPCR0) & ~TXx9_SPCR0_RBSIE,
+		   TXx9_SPCR0);
+	wake_up(&c->waitq);
+	return IRQ_HANDLED;
+}
+
+static int txx9spi_work_one(struct txx9spi *c, struct spi_message *m)
+{
+	struct spi_device *spi = m->spi;
+	struct txx9spi_cs *cs = spi->controller_state;
+	struct spi_transfer *t;
+	unsigned int nsecs;
+	unsigned int cs_change;
+	int status;
+	u32 mcr;
+
+	mcr = txx9spi_rd(c, TXx9_SPMCR);
+	if (unlikely((mcr & TXx9_SPMCR_OPMODE) == TXx9_SPMCR_ACTIVE)) {
+		dev_err(&spi->dev, "Bad mode.\n");
+		return -EIO;
+	}
+
+	/* enter config mode */
+	mcr &= ~(TXx9_SPMCR_OPMODE | TXx9_SPMCR_SPSTP | TXx9_SPMCR_BCLR);
+	txx9spi_wr(c, mcr | TXx9_SPMCR_CONFIG | TXx9_SPMCR_BCLR, TXx9_SPMCR);
+	txx9spi_wr(c, TXx9_SPCR0_SBOS |
+		   ((spi->mode & SPI_CPOL) ? TXx9_SPCR0_SPOL : 0) |
+		   ((spi->mode & SPI_CPHA) ? TXx9_SPCR0_SPHA : 0) |
+		   0x08,
+		   TXx9_SPCR0);
+	txx9spi_wr(c, cs->cr1, TXx9_SPCR1);
+	/* enter active mode */
+	txx9spi_wr(c, mcr | TXx9_SPMCR_ACTIVE, TXx9_SPMCR);
+
+	cs_change = 1;
+	status = 0;
+	nsecs = 100 + 1000000000 / spi->max_speed_hz / 2;
+	list_for_each_entry (t, &m->transfers, transfer_list) {
+		const void *txbuf = t->tx_buf;
+		void *rxbuf = t->rx_buf;
+		u32 data;
+		unsigned int len = t->len;
+		unsigned int wsize = spi->bits_per_word >> 3; /* in bytes */
+		if ((!txbuf && !rxbuf && len) || (len & (wsize - 1))) {
+			status = -EINVAL;
+			break;
+		}
+		if (cs_change) {
+			txx9spi_cs_func(spi, 1);
+			ndelay(nsecs);
+		}
+		cs_change = t->cs_change;
+		while (len) {
+			unsigned int count = SPI_FIFO_SIZE;
+			int i;
+			u32 cr0;
+			if (len < count * wsize)
+				count = len / wsize;
+			/* now tx must be idle... */
+			while (!(txx9spi_rd(c, TXx9_SPSR) & TXx9_SPSR_SIDLE))
+				;
+			cr0 = txx9spi_rd(c, TXx9_SPCR0);
+			cr0 &= ~TXx9_SPCR0_RXIFL_MASK;
+			cr0 |= (count - 1) << 12;
+			/* enable rx intr */
+			cr0 |= TXx9_SPCR0_RBSIE;
+			txx9spi_wr(c, cr0, TXx9_SPCR0);
+			/* send */
+			for (i = 0; i < count; i++) {
+				data = txbuf ? (wsize == 1 ?
+						*(const u8 *)txbuf :
+						*(const u16 *)txbuf) : 0;
+				txx9spi_wr(c, data, TXx9_SPDR);
+				if (txbuf)
+					txbuf += wsize;
+			}
+			/* wait all rx data */
+			wait_event(c->waitq,
+				   txx9spi_rd(c, TXx9_SPSR) & TXx9_SPSR_RBSI);
+			/* receive */
+			for (i = 0; i < count; i++) {
+				data = txx9spi_rd(c, TXx9_SPDR);
+				if (rxbuf) {
+					if (wsize == 1)
+						*(u8 *)rxbuf = data;
+					else
+						*(u16 *)rxbuf = data;
+					rxbuf += wsize;
+				}
+			}
+			len -= count * wsize;
+		}
+		m->actual_length += t->len;
+		if (t->delay_usecs)
+			udelay(t->delay_usecs);
+
+		if (!cs_change)
+			continue;
+		if (t->transfer_list.next == &m->transfers)
+			break;
+		/* sometimes a short mid-message deselect of the chip
+		 * may be needed to terminate a mode or command
+		 */
+		ndelay(nsecs);
+		txx9spi_cs_func(spi, 0);
+		ndelay(nsecs);
+	}
+
+	m->status = status;
+	m->complete(m->context);
+
+	/* normally deactivate chipselect ... unless no error and
+	 * cs_change has hinted that the next message will probably
+	 * be for this chip too.
+	 */
+	if (!(status == 0 && cs_change)) {
+		ndelay(nsecs);
+		txx9spi_cs_func(spi, 0);
+		ndelay(nsecs);
+	}
+
+	/* enter config mode */
+	txx9spi_wr(c, mcr | TXx9_SPMCR_CONFIG | TXx9_SPMCR_BCLR, TXx9_SPMCR);
+	return status;
+}
+
+static void txx9spi_work(struct work_struct *work)
+{
+	struct txx9spi *c = container_of(work, struct txx9spi, work);
+	unsigned long flags;
+
+	spin_lock_irqsave(&c->lock, flags);
+	while (!list_empty(&c->queue)) {
+		struct spi_message *m;
+
+		m = container_of(c->queue.next, struct spi_message, queue);
+		list_del_init(&m->queue);
+		spin_unlock_irqrestore(&c->lock, flags);
+
+		txx9spi_work_one(c, m);
+
+		spin_lock_irqsave(&c->lock, flags);
+	}
+	spin_unlock_irqrestore(&c->lock, flags);
+}
+
+static int txx9spi_transfer(struct spi_device *spi, struct spi_message *m)
+{
+	struct spi_master *master = spi->master;
+	struct txx9spi *c = spi_master_get_devdata(master);
+	unsigned long flags;
+
+	m->actual_length = 0;
+	spin_lock_irqsave(&c->lock, flags);
+	list_add_tail(&m->queue, &c->queue);
+	queue_work(c->workqueue, &c->work);
+	spin_unlock_irqrestore(&c->lock, flags);
+
+	return 0;
+}
+
+static void txx9spi_cleanup(struct spi_device *spi)
+{
+	kfree(spi->controller_state);
+}
+
+#ifdef CONFIG_64BIT
+#define TXX9_DIRECTMAP_BASE	0xfff000000ul
+#else
+#define TXX9_DIRECTMAP_BASE	0xff000000ul
+#endif
+
+static int __init txx9spi_probe(struct platform_device *dev)
+{
+	struct spi_master *master;
+	struct txx9spi *c;
+	struct resource *res;
+	int ret = -ENODEV;
+	u32 mcr;
+
+	master = spi_alloc_master(&dev->dev, sizeof(*c));
+	if (!master)
+		return ret;
+	c = spi_master_get_devdata(master);
+	platform_set_drvdata(dev, master);
+
+	INIT_WORK(&c->work, txx9spi_work);
+	spin_lock_init(&c->lock);
+	INIT_LIST_HEAD(&c->queue);
+	init_waitqueue_head(&c->waitq);
+
+	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (!res)
+		goto put_and_exit;
+	if (res->start >= TXX9_DIRECTMAP_BASE)
+		c->membase = (void __iomem *)(unsigned long)(int)res->start;
+	else {
+		c->membase = ioremap(res->start, res->end - res->start + 1);
+		c->mapped = 1;
+	}
+
+	/* enter config mode */
+	mcr = txx9spi_rd(c, TXx9_SPMCR);
+	mcr &= ~(TXx9_SPMCR_OPMODE | TXx9_SPMCR_SPSTP | TXx9_SPMCR_BCLR);
+	txx9spi_wr(c, mcr | TXx9_SPMCR_CONFIG | TXx9_SPMCR_BCLR, TXx9_SPMCR);
+
+	c->irq = platform_get_irq(dev, 0);
+	if (c->irq < 0)
+		goto res_and_exit;
+	c->baseclk = platform_get_irq_byname(dev, "baseclk");
+	if (c->baseclk < 0)
+		goto res_and_exit;
+	ret = request_irq(c->irq, txx9spi_interrupt, 0, dev->name, c);
+	if (ret)
+		goto res_and_exit;
+
+	c->workqueue = create_singlethread_workqueue(master->cdev.dev->bus_id);
+	if (!c->workqueue)
+		goto irq_and_exit;
+
+	dev_info(&dev->dev, "at 0x%llx, irq %d, %dMHz\n",
+		 (unsigned long long)res->start, c->irq,
+		 (c->baseclk + 500000) / 1000000);
+
+	master->bus_num = dev->id;
+	master->setup = txx9spi_setup;
+	master->transfer = txx9spi_transfer;
+	master->cleanup = txx9spi_cleanup;
+
+	ret = spi_register_master(master);
+	if (ret)
+		goto wq_and_exit;
+	return 0;
+ wq_and_exit:
+	destroy_workqueue(c->workqueue);
+ irq_and_exit:
+	free_irq(c->irq, c);
+ res_and_exit:
+	if (c->mapped)
+		iounmap(c->membase);
+ put_and_exit:
+	platform_set_drvdata(dev, NULL);
+	spi_master_put(master);
+	return ret;
+}
+
+static int __exit txx9spi_remove(struct platform_device *dev)
+{
+	struct spi_master *master = spi_master_get(platform_get_drvdata(dev));
+	struct txx9spi *c = spi_master_get_devdata(master);
+
+	spi_unregister_master(master);
+	platform_set_drvdata(dev, NULL);
+	destroy_workqueue(c->workqueue);
+	free_irq(c->irq, c);
+	if (c->mapped)
+		iounmap(c->membase);
+	spi_master_put(master);
+	return 0;
+}
+
+static struct platform_driver txx9spi_driver = {
+	.probe = txx9spi_probe,
+	.remove = __exit_p(txx9spi_remove),
+	.driver = {
+		.name = "txx9spi",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init txx9spi_init(void)
+{
+	return platform_driver_register(&txx9spi_driver);
+}
+subsys_initcall(txx9spi_init);
+
+static void __exit txx9spi_exit(void)
+{
+	platform_driver_unregister(&txx9spi_driver);
+}
+module_exit(txx9spi_exit);
+
+MODULE_DESCRIPTION("TXx9 SPI Driver");
+MODULE_LICENSE("GPL");



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] TXx9 SPI controller driver

[Atsushi Nemoto]

On Fri, 22 Jun 2007 11:51:24 -0700, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > > This is a driver for SPI controller built into TXx9 MIPS SoCs.
> > 
> > Looks mostly pretty good.  I made a few minor changes/cleanups
> > in the appended version, notably:
> >  - checking for spi->mode bits this code doesn't understand;
> >  - updating to match latest patches;
> > 
> > Note that if gpio_set_value() needs an mmiowb(), that seems like
> > a bug in this platform's  GPIO code; other platforms don't require
> > I/O barriers after GPIO calls.  Comments?
> > 
> > Also:
> > 
> 
> ... yeah, -ENOPATCH, sorry.  And the minor whitespace fixes.
> 
> One more comment:  surely platform_driver_probe() would be
> appropriate here?

Thank you for excellent review!  I'll look at each comments surely
will update the driver but it may take a few days.

For now, I'm quite sure your patch is OK for me except for one thing:

> + * spi_tx99.c - TXx9 SPI controller driver.

Name it spi_txx9.c, please ;)

And for mmiowb() issue, I put it just only I was not sure whether
gpio_set_value() guarantee I/O barrier.  Now I see i2c-gpio.c, etc. do
not have such barriers.  I will remove these barriers and fix platform
gpio codes.

---
Atsushi Nemoto

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [spi-devel-general] [PATCH] TXx9 SPI controller driver

[David Brownell]

On Saturday 23 June 2007, Atsushi Nemoto wrote:

> Thank you for excellent review!  I'll look at each comments surely
> will update the driver but it may take a few days.

That's fine.

 
> For now, I'm quite sure your patch is OK for me except for one thing:
> 
> > + * spi_tx99.c - TXx9 SPI controller driver.
> 
> Name it spi_txx9.c, please ;)

Sorry, typo!  ... please fix when you resubmit.

 
> And for mmiowb() issue, I put it just only I was not sure whether
> gpio_set_value() guarantee I/O barrier.  Now I see i2c-gpio.c, etc. do
> not have such barriers.  I will remove these barriers and fix platform
> gpio codes.

I don't think this is a case where there'd be a benefit to
allowing non-barriered implementations, and thus requiring
all portable code to include platform-neutral I/O barriers.
I don't know that such neutral primitives actually exist...

I'll update the GPIO docs to make that clear, unless you
have some strong argument to the contrary.

- Dave

Re: [PATCH] TXx9 SPI controller driver

[Atsushi Nemoto]

On Sat, 23 Jun 2007 09:09:51 -0700, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > And for mmiowb() issue, I put it just only I was not sure whether
> > gpio_set_value() guarantee I/O barrier.  Now I see i2c-gpio.c, etc. do
> > not have such barriers.  I will remove these barriers and fix platform
> > gpio codes.
> 
> I don't think this is a case where there'd be a benefit to
> allowing non-barriered implementations, and thus requiring
> all portable code to include platform-neutral I/O barriers.
> I don't know that such neutral primitives actually exist...
> 
> I'll update the GPIO docs to make that clear, unless you
> have some strong argument to the contrary.

No contrary argument.  Some guide to writers of GPIO implementations
about the I/O barriers should be appreciated.

---
Atsushi Nemoto

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] TXx9 SPI controller driver

[Atsushi Nemoto]

On Fri, 22 Jun 2007 11:03:18 -0700, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > +	if (res->start >= TXX9_DIRECTMAP_BASE)
> > +		c->membase = (void __iomem *)(unsigned long)(int)res->start;
> > +	else {
> > +		c->membase = ioremap(res->start, res->end - res->start + 1);
> > +		c->mapped = 1;
> > +	}
> 
> That looks plain wrong.  Maybe it reflects a platform-level bug,
> but ioremap(res->start) should Just Work even when it performs
> an identity mapping on a given system.  Remove this ugly code.
> Always map.

Ralf, (as I said some time ago) TX39XX and TX49XX have "reserved"
segment in CKSEG3 area.  0xff000000-0xff3fffff on TX49XX and
0xff000000-0xfffeffff on TX39XX are reserved (unmapped, uncached).
Controllers on these SoCs are placed in this segment.

If ioremap()/iounmap() could handle these special case, I can remove
this hack in this driver.

Is something like this acceptable?

include/asm-mips/io.h:
static inline void __iomem * __ioremap_mode(phys_t offset, unsigned long size,
	unsigned long flags)
{
	void __iomem *addr = plat_ioremap(offset, size, flags);
	if (addr)
		return addr;
	...
}

static inline void iounmap(const volatile void __iomem *addr)
{
	if (plat_iounmap(addr))
		returnl
	...
}

include/asm-mips/mach-generic/ioremap.h:
static inline void __iomem *plat_ioremap(phys_t offset, unsigned long size,
	unsigned long flags)
{
	return NULL;
}

static inline int plat_iounmap(const volatile void __iomem *addr)
{
	return 0;
}

---
Atsushi Nemoto

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] TXx9 SPI controller driver

[Atsushi Nemoto]

On Fri, 22 Jun 2007 11:03:18 -0700, David Brownell <david-b@pacbell.net> wrote:
> > +	/* calc real speed */
> > +	n = (c->baseclk + spi->max_speed_hz - 1) / spi->max_speed_hz;
> > +	if (n < 1)
> > +		n = 1;
> > +	else if (n > 0xff)
> > +		return -EINVAL;
> > +	spi->max_speed_hz = c->baseclk / n;
> 
> That's not right -- given the current API definitions.  The max_speed_hz
> value should be read-only to controller drivers.
> 
> On the other hand, I've also wondered how the actual rate should be
> reported to drivers, and that approach might be a reasonable solution
> to that problem.  Can you discuss this proposed change on the spi
> development list?

As for this driver, anyway I should move this calculation to the
transfer function to handle per-transfer speed_hz, so there is no
serious requirement to modify max_speed_hz here.

I agree reporting the actual speed to the protocol driver might be
useful, but I should think a bit more.

> > +static int txx9spi_work_one(struct txx9spi *c, struct spi_message *m)
> > +{
> > +	...
> > +	nsecs = 100 + 1000000000 / spi->max_speed_hz / 2;
> 
> ... which is why we can't just strike that line changing max_speed_hz.
> But please use only a single division in that expression, and explain
> what the value is used for.  (A good variable name would do wonders.
> Is this "nsecs per fortnight", or what?)
> 
> I suspect "nsecs" and the actual bitrate should probably be stored
> with the other spi->controller_state data.

These nsecs delays are all comes from spi_bitbang driver I referred
when writing this driver.  As you already found, this controller does
not have dedicated chipselect signals and generic GPIO is used for
them.  As the controller do nothing about CS, the driver should take
care of CS setup/hold/recovery time.

I will move these delays to cs_func() and add some comments.

> > +	list_for_each_entry (t, &m->transfers, transfer_list) {
> > +		const void *txbuf = t->tx_buf;
> > +		void *rxbuf = t->rx_buf;
> > +		u32 data;
> > +		unsigned int len = t->len;
> > +		unsigned int wsize = spi->bits_per_word >> 3; /* in bytes */
> 
> But on the other hand, here you're ignoring t->max_speed_hz as
> well as t->bits_per_word.  If you do that, you must check those
> values in txx9spi_transfer() and reject message which include
> transfers using those per-transfer overrides.
> 
> Given that this controller only seems to allow 8 or 16 bit
> transfers, you're going to need checks in the transfer() path
> even if you do support the per-transfer overrides someday.

Oh I had missed these per-transfer parameters.  I will add some
checking for them.

> > +		if ((!txbuf && !rxbuf && len) || (len & (wsize - 1))) {
> 
> That's confusing.  But it's also something that belongs with
> per-transfer checks -- doing it here is needlessly late.

Sure.  I will move it before actual transfer.

> > +				data = txbuf ? (wsize == 1 ?
> > +						*(const u8 *)txbuf :
> > +						*(const u16 *)txbuf) : 0;
> 
> Double "?:" expressions == confusing.  Please use at most one.

Will fix.

> > +	if (!(status == 0 && cs_change)) {
> > +		ndelay(nsecs);
> 
> Again:  if drivers need an extra delay, that's what delay_usec is for.
> 
> > +		txx9spi_cs_func(spi, 0);
> > +		ndelay(nsecs);
> 
> ... and I don't see why this delay is needed at all.  If your
> hardware needs extra delays around chipselect toggles, surely
> that should be built into your cs_func()?
> 
> But it looks to me like you have a bug here too.  You're trying
> to implement this behavioral hint, which is fine, but you're
> not recording that you did it.  So if the next message goes
> to a different chip, both chips will be selected at the same
> time -- right?  Bug... quickest for you to ignore this hint.

Oh it should be a bug.  I will fix by recording last chipselect.

> > +	if (res->start >= TXX9_DIRECTMAP_BASE)
> > +		c->membase = (void __iomem *)(unsigned long)(int)res->start;
> > +	else {
> > +		c->membase = ioremap(res->start, res->end - res->start + 1);
> > +		c->mapped = 1;
> > +	}
> 
> That looks plain wrong.  Maybe it reflects a platform-level bug,
> but ioremap(res->start) should Just Work even when it performs
> an identity mapping on a given system.  Remove this ugly code.
> Always map.

Yes this was a hack...  I will try to make MIPS ioremap() can handle
this special case.

> > +	c->baseclk = platform_get_irq_byname(dev, "baseclk");
> > +	if (c->baseclk < 0)
> > +		goto res_and_exit;
> 
> Yeech!!   If you're getting a clock, then clk_get(dev, "baseclk").
> That's very clearly not an IRQ.  If you need values that don't fit
> into the current resource framework, either extend that framework
> or use platform_data to pass the values.
> 
> In this case, extending the ioresource framework would be the wrong
> answer, since the clock framework is there to handle clocks.  If
> this platform doesn't support the clock framework, then you must
> use platform_data to pass such nonstandard clock data.

Another hack.  Good to know about the clock framework.  That's what
I wanted.  I will try to implement them for MIPS.

> > +	master->bus_num = dev->id;
> > +	master->setup = txx9spi_setup;
> > +	master->transfer = txx9spi_transfer;
> > +	master->cleanup = txx9spi_cleanup;
> 
> Something else that seems incorrect here is the lack of setup
> for master->num_chipselect.  It seems that you're using the
> spi->chip_select value as a GPIO number.  That's unusual, but
> not wrong.  Just set num_chipselect to the number of GPIOs on
> the platform.  Otherwise, just map from chipselect to GPIO
> like the other drivers do.  Such mappings fit naturally into
> platform_data.
> 
> The fact that this works at all is a temporary bug in the
> SPI core.

Yes, I'm using GPIO number as chipselect value.  This controller
itself does not have any constraint about chipselect, I wanted to set
num_chipselect as "unlimited".  Though providing max GPIO number or
mapping table might be safe, I think just using chipselect number for
GPIO as is would be simple and enough.  I will do explicitly
initialize num_chipselect as 0 and add a comment for it.


I will send an updated patch in a few days.  Thank you.
---
Atsushi Nemoto