Sorry to cross-post this to linuxppc-...@ozlabs.org in the middle
of the story. I started this in linux-...@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:
>> 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
> - Dave
>
>
> ---
> drivers/spi/spi_bitbang.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> --- a/drivers/spi/spi_bitbang.c
> +++ b/drivers/spi/spi_bitbang.c
> @@ -258,6 +258,11 @@ static void bitbang_work(struct work_str
> struct spi_bitbang *bitbang =
> container_of(work, struct spi_bitbang, work);
> unsigned long flags;
> + int do_setup = -1;
> + int (*setup_transfer)(struct spi_device *,
> + struct spi_transfer *);
> +
> + setup_transfer = bitbang->setup_transfer;
>
> spin_lock_irqsave(&bitbang->lock, flags);
> bitbang->busy = 1;
> @@ -269,8 +274,6 @@ static void bitbang_work(struct work_str
> unsigned tmp;
> unsigned cs_change;
> int status;
> - int (*setup_transfer)(struct spi_device *,
> - struct spi_transfer *);
>
> m = container_of(bitbang->queue.next, struct spi_message,
> queue);
> @@ -286,19 +289,19 @@ static void bitbang_work(struct work_str
> tmp = 0;
> cs_change = (spi != bitbang->exclusive);
> status = 0;
> - setup_transfer = NULL;
>
> list_for_each_entry (t, &m->transfers, transfer_list) {
>
> - /* override or restore speed and wordsize */
> - if (t->speed_hz || t->bits_per_word) {
> - setup_transfer = bitbang->setup_transfer;
> + /* override speed or wordsize? */
> + if (t->speed_hz || t->bits_per_word)
> + do_setup = 1;
> +
> + /* init or override transfer params */
> + if (do_setup != 0) {
> if (!setup_transfer) {
> status = -ENOPROTOOPT;
> break;
> }
> - }
> - if (setup_transfer) {
> status = setup_transfer(spi, t);
> if (status < 0)
> break;
> @@ -362,8 +365,9 @@ static void bitbang_work(struct work_str
> m->status = status;
>
> /* restore speed and wordsize */
> - if (setup_transfer)
> + if (do_setup == 1)
> setup_transfer(spi, NULL);
> + do_setup = 0;
>
> /* normally deactivate chipselect ... unless no error and
> * cs_change has hinted that the next message will probably
--
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?
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
