Re: [PATCH] TXx9 SPI controller driver

* Re: [PATCH] TXx9 SPI controller driver
       [not found] ` <20070622.232111.36926005.anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org>
@ 2007-06-22 18:03   ` David Brownell
       [not found]     ` <200706221103.19761.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
  2007-06-25 16:12     ` Atsushi Nemoto
  0 siblings, 2 replies; 7+ messages in thread
From: David Brownell @ 2007-06-22 18:03 UTC (permalink / raw)
  To: Atsushi Nemoto
  Cc: sshtylyov-hkdhdckH98+B+jHODAdFcQ,
	linux-mips-6z/3iImG2C8G8FEW9MqTrA,
	spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	ralf-6z/3iImG2C8G8FEW9MqTrA, mlachwani-Igf4POYTYCDQT0dZR+AlfA

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/

^ permalink raw reply	[flat|nested] 7+ messages in thread