Re: [Question] m25p80 driver versus spi clock rate

Re: [Question] m25p80 driver versus spi clock rate

[Steven A. Falco]

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:
>> 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?

Re: [Question] m25p80 driver versus spi clock rate

[David Brownell]

On Tuesday 23 June 2009, Steven A. Falco wrote:
> 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.

Bits-per-word == 0 must be interpreted as == 8.

Simple bug in the ppc4xx code.  It currently rejects
values other than 8.

Speaking of spi_ppc4xx issues ... I still have an oldish
copy in my review queue, it needs something like the
appended patch.  (Plus something to accept bpw == 0.)
Is there a newer version?

- Dave


--- a/drivers/spi/spi_ppc4xx.c
+++ b/drivers/spi/spi_ppc4xx.c
@@ -61,9 +61,6 @@
 /* RxD ready */
 #define SPI_PPC4XX_SR_RBR	(0x80 >> 7)
 
-/* the spi->mode bits understood by this driver: */
-#define MODEBITS	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
-
 /* clock settings (SCP and CI) for various SPI modes */
 #define SPI_CLK_MODE0	SPI_PPC4XX_MODE_SCP
 #define SPI_CLK_MODE1	0
@@ -198,9 +195,6 @@ static int spi_ppc4xx_setup(struct spi_d
 	struct spi_ppc4xx_cs *cs = spi->controller_state;
 	int init = 0;
 
-	if (!spi->bits_per_word)
-		spi->bits_per_word = 8;
-
 	if (spi->bits_per_word != 8) {
 		dev_err(&spi->dev, "invalid bits-per-word (%d)\n",
 			spi->bits_per_word);
@@ -212,12 +206,6 @@ static int spi_ppc4xx_setup(struct spi_d
 		return -EINVAL;
 	}
 
-	if (spi->mode & ~MODEBITS) {
-		dev_dbg(&spi->dev, "setup: unsupported mode bits %x\n",
-			spi->mode & ~MODEBITS);
-		return -EINVAL;
-	}
-
 	if (cs == NULL) {
 		cs = kzalloc(sizeof *cs, GFP_KERNEL);
 		if (!cs)
@@ -268,10 +256,6 @@ static int spi_ppc4xx_setup(struct spi_d
 		}
 	}
 
-	dev_dbg(&spi->dev, "%s: mode %d, %u bpw, %d hz\n",
-		__func__, spi->mode, spi->bits_per_word,
-		spi->max_speed_hz);
-
 	return 0;
 }
 
@@ -442,6 +426,9 @@ static int __init spi_ppc4xx_of_probe(st
 		}
 	}
 
+	/* the spi->mode bits understood by this driver: */
+	master->modebits = SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST;
+
 	/* Setup the state for the bitbang driver */
 	bbp = &hw->bitbang;
 	bbp->master = hw->master;

Re: [Question] m25p80 driver versus spi clock rate

[Steven A. Falco]

David Brownell wrote:
> On Tuesday 23 June 2009, Steven A. Falco wrote:
>> 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.
> 
> Bits-per-word == 0 must be interpreted as == 8.
> 
> Simple bug in the ppc4xx code.  It currently rejects
> values other than 8.

Ok - I'll post a patch for that.  Your changes to bitbang_work look
good.  I'm not clear on why you first set do_setup = -1 but later
use do_setup = 1.  Perhaps they should both be "1".  Other than that,

Acked-by: Steven A. Falco <sfalco@harris.com>

> 
> Speaking of spi_ppc4xx issues ... I still have an oldish
> copy in my review queue, it needs something like the
> appended patch.  (Plus something to accept bpw == 0.)
> Is there a newer version?

That is a question for Stefan.  Perhaps when I post my patch
to the PPC list, we can move this further along...

	Steve

Re: [Question] m25p80 driver versus spi clock rate

[Stefan Roese]

On Wednesday 24 June 2009 16:25:20 Steven A. Falco wrote:
> > Speaking of spi_ppc4xx issues ... I still have an oldish
> > copy in my review queue, it needs something like the
> > appended patch.  (Plus something to accept bpw == 0.)
> > Is there a newer version?
>
> That is a question for Stefan.  Perhaps when I post my patch
> to the PPC list, we can move this further along...

I have to admit that I didn't find the time to rework the driver after David's 
latest review. Frankly, this could take some time since I'm currently busy 
with other tasks. So it would be great if someone else (Steven?) might pick up 
here and resubmit this driver so that we can get it finally included.

Thanks.

Best regards,
Stefan

Re: [Question] m25p80 driver versus spi clock rate

[Steven A. Falco]

Stefan Roese wrote:
> On Wednesday 24 June 2009 16:25:20 Steven A. Falco wrote:
>>> Speaking of spi_ppc4xx issues ... I still have an oldish
>>> copy in my review queue, it needs something like the
>>> appended patch.  (Plus something to accept bpw == 0.)
>>> Is there a newer version?
>> That is a question for Stefan.  Perhaps when I post my patch
>> to the PPC list, we can move this further along...
> 
> I have to admit that I didn't find the time to rework the driver after David's 
> latest review. Frankly, this could take some time since I'm currently busy 
> with other tasks. So it would be great if someone else (Steven?) might pick up 
> here and resubmit this driver so that we can get it finally included.
> 
> Thanks.
> 
> Best regards,
> Stefan

Ok - I'll take that on.

Please, both David and Stefan send me the latest versions
and/or patches you have, and I'll integrate them and post
to the PPC list.

	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?

Re: [Question] m25p80 driver versus spi clock rate

[Stefan Roese]

On Wednesday 24 June 2009 16:36:58 Steven A. Falco wrote:
> > I have to admit that I didn't find the time to rework the driver after
> > David's latest review. Frankly, this could take some time since I'm
> > currently busy with other tasks. So it would be great if someone else
> > (Steven?) might pick up here and resubmit this driver so that we can get
> > it finally included.
> >
> > Thanks.
> >
> > Best regards,
> > Stefan
>
> Ok - I'll take that on.

Great, thanks.

> Please, both David and Stefan send me the latest versions
> and/or patches you have, and I'll integrate them and post
> to the PPC list.

I just sent you my latest driver version (v6). Here a link to David's latest 
review:

http://article.gmane.org/gmane.linux.ports.ppc64.devel/55229

Best regards,
Stefan

Re: [Question] m25p80 driver versus spi clock rate

[David Brownell]

On Wednesday 24 June 2009, Steven A. Falco wrote:
> Your changes to bitbang_work look good.

You tested?


> I'm not clear on why you first set do_setup = -1 but later 
> use do_setup = 1.  Perhaps they should both be "1".  Other than that,
> 
> Acked-by: Steven A. Falco <sfalco@harris.com>

The "-1" is for the init path, "1" for per-transfer overrides;
this way it avoids some extra calls to set up the bits/speed.

Re: [Question] m25p80 driver versus spi clock rate

[Steven A. Falco]

David Brownell wrote:
> On Wednesday 24 June 2009, Steven A. Falco wrote:
>> Your changes to bitbang_work look good.
> 
> You tested?
> 

Yes - I built a kernel with your patch, combined with the changes I
just posted to linuxppc-dev@ozlabs.org as:

"[PATCH v1] Make spi_ppc4xx.c tolerate 0 bits-per-word and 0 speed_hz"

I was successful in operating both the m25p16 at 16 MHz and the AVR
at 240 KHz, as verified by oscilloscope.  So my "ack" includes testing.

> 
>> I'm not clear on why you first set do_setup = -1 but later 
>> use do_setup = 1.  Perhaps they should both be "1".  Other than that,
>>
>> Acked-by: Steven A. Falco <sfalco@harris.com>
> 
> The "-1" is for the init path, "1" for per-transfer overrides;
> this way it avoids some extra calls to set up the bits/speed.

Got it.  No further comments.  My "ack" stands.

I'll start looking at a revised version of the spi_ppc4xx.c driver,
integrating your comments.

	Steve