Re: [Question] m25p80 driver versus spi clock rate

Sorry to cross-post this to linuxppc-dev@ozlabs.org in the middle
of the story.  I started this in linux-mtd@lists.infradead.org, but
there are OF issues here, and I'd like the PPC folks to be aware of
the issues.

David Brownell wrote:

On Tuesday 23 June 2009, Steven A. Falco wrote:

quoted

David Brownell wrote:
The linkage appears correct - max_speed_hz is set correctly for each
device.  The problem is that bitbang_work won't call spi_ppc4xx_setupxfer
unless speed_hz is non-zero, and m25p80 has no way to alter speed_hz.

Or alternatively:  that bitbang_work is missing an initial
call to setup_xfer before the loop *starts* its work...

I think the issue is that few other users have used this
code with multiple devices, which had such mismatches in
device speed that they would have noticed this bug.

See if the below patch resolves this issue.

Fascinating.  I now get a fatal error:

m25p80 spi0.0: invalid bits-per-word (0)

This message comes from spi_ppc4xx_setupxfer.  I believe your patch
is doing what you intended (i.e. forcing an initial call to
spi_ppc4xx_setupxfer), but it exposes an OF / SPI linkage problem.

Namely, of_register_spi_devices does not support a bits-per-word
property, so bits-per-word is zero.

Since we had never called spi_ppc4xx_setupxfer for the m25p80, we
never saw this until now...

Here is part of spi_ppc4xx_setupxfer:

	/*
	 * Allow platform reduce the interrupt load on the CPU during SPI
	 * transfers. We do not target maximum performance, but rather allow
	 * platform to limit SPI bus frequency and interrupt rate.
	 */
	bpw = t ? t->bits_per_word : spi->bits_per_word;
	cs->speed_hz = t ? min(t->speed_hz, spi->max_speed_hz) :
		spi->max_speed_hz;

	if (bpw != 8) {
		dev_err(&spi->dev, "invalid bits-per-word (%d)\n", bpw);
		return -EINVAL;
	}

	if (cs->speed_hz == 0) {
		dev_err(&spi->dev, "invalid speed_hz (must be non-zero)\n");
		return -EINVAL;
	}

Actually, the problem is not entirely with of_register_spi_devices.
bitbang_work will call spi_ppc4xx_setupxfer with a non-null 
spi_transfer.  So, the above code will always set bpw based on
t->bits_per_word.  If t->bits_per_word is zero, it wouldn't even matter
if of_register_spi_devices set spi->bits_per_word, because the
transfer would override it.

How about:

	bpw = t && t->bits_per_word ? t->bits_per_word : spi->bits_per_word;

Now, t->bits_per_word would have to be non-zero in order to override
spi->bits_per_word.

We would still need a patch to of_register_spi_devices to allow (require)
a bits-per-word property, along with device tree patches to add the
property.  But that should take care of it.

I'm adding the ppc list as a CC, since this is turning into an OF
discussion.

	Steve

-- 
A: Because it makes the logic of the discussion difficult to follow.
Q: Why shouldn't I top post?
A: No.
Q: Should I top post?