[Question] m25p80 driver versus spi clock rate

[Question] m25p80 driver versus spi clock rate

[Steven A. Falco]

I am trying to figure out how the mtd/devices/m25p80.c driver is supposed
to set the spi clock speed.  (Perhaps I'm making a bad assuption even to
think that it _should_ set the clock speed.  If so, please say so.)

I have three spi devices on a ppc400epx: two m25p16 chips and one Atmel
AVR uP.  The Atmel is handled from user-space, via spidev.  I am setting
the spi_ioc_transfer "speed_hz" parameter, and I see spidev change the
clock rate as I would expect.

However, m25p80.c doesn't seem to touch the clock rate.  Perhaps this is
because the PPC440epx uses "open firmware" device trees to specify the
speed and the m25p80 driver is not an OF driver.  Or as I said above,
perhaps the m25p80 driver isn't even supposed to set the clock rate.

The Atmel we are using cannot have its spi bus run faster than 250 KHz,
while the m25p16 devices can run up to 75 MHz (although the PPC440epx
tops out at 16.6 MHz).  So it is essential that I flip the clock rate
back and forth to maximize performance of the m25p16's while allowing
the Atmel to work.

So, if someone could comment on how the clock speed should be set, I
would appreciate it.  Clearly, I can put in a hack, but I'd rather
follow the existing intent.

	Thanks,
	Steve

Re: [Question] m25p80 driver versus spi clock rate

[Mike Frysinger]

On Mon, Jun 22, 2009 at 16:50, Steven A. Falco wrote:
> I am trying to figure out how the mtd/devices/m25p80.c driver is supposed
> to set the spi clock speed.  (Perhaps I'm making a bad assuption even to
> think that it _should_ set the clock speed.  If so, please say so.)

it shouldnt.  this is done in the board resources via the speed_hz
field of the spi_board_info struct on a per-spi device setting.
-mike

Re: [Question] m25p80 driver versus spi clock rate

[Steven A. Falco]

Mike Frysinger wrote:
> On Mon, Jun 22, 2009 at 16:50, Steven A. Falco wrote:
>> I am trying to figure out how the mtd/devices/m25p80.c driver is supposed
>> to set the spi clock speed.  (Perhaps I'm making a bad assuption even to
>> think that it _should_ set the clock speed.  If so, please say so.)
> 
> it shouldnt.  this is done in the board resources via the speed_hz
> field of the spi_board_info struct on a per-spi device setting.
> -mike

Thank you for the hint.  spi_board_info has a max_speed_hz field - it does
not have a speed_hz field.  The various platforms all seem to set
max_speed_hz, so perhaps that is what you meant to say.

In my case, max_speed_hz is being correctly set, but that doesn't seem to be
enough.  I have traced through the calling hierarchy, and this is what I got:

1) m25p80_read builds a spi_message, and calls spi_sync to do the transfer.

2) spi_sync calls spi_async.  I added some printk, and saw speed_hz=0 and
   max_speed_hz=50000000.  This is consistent with my platform setup (set via
   a dts file).

3) spi_async calls through pointer "transfer" to spi_bitbang_transfer (because
   the PPC4xx driver doesn't set its own transfer handler).

4) spi_bitbang_transfer calls spi_master_get_devdata, then enqueues the work

5) bitbang_work iterates through the queued transfers, and if speed_hz is
   non-zero, bitbang_work calls through setup_transfer to spi_ppc4xx_setupxfer
   which would set the divisor.  But, as noted in step 2, speed_hz=0, so the
   spi bus speed is not set.  Rather, it remains at whatever speed some other
   device chose.

Note that bitbang_work looks at speed_hz, not max_speed_hz.  So, I come back to
the same problem.  Somehow speed_hz must be set in order to make bitbang_work
call setup_transfer, yet the only place that seems to happen is in spidev.

Stefan - I believe you wrote the SPI_PPC4XX driver.  Can you comment on how
this is supposed to work?  My device tree has:

				m25p16@1 {
					compatible = "st,m25p80";
					reg = <1>;
					spi-max-frequency = <50000000>;
				};

				fpdlite@2 {
					compatible = "linux,spidev";
					reg = <2>;
					spi-max-frequency = <200000>;
				};

and the issue is that fpdlite@2 drops the frequency to 200000, but m25p16@1
does not raise the frequency to 50000000.

	Thanks,
	Steve

Re: [Question] m25p80 driver versus spi clock rate

[Mike Frysinger]

On Tue, Jun 23, 2009 at 14:41, Steven A. Falco wrote:
> Mike Frysinger wrote:
>> On Mon, Jun 22, 2009 at 16:50, Steven A. Falco wrote:
>>> I am trying to figure out how the mtd/devices/m25p80.c driver is supposed
>>> to set the spi clock speed.  (Perhaps I'm making a bad assuption even to
>>> think that it _should_ set the clock speed.  If so, please say so.)
>>
>> it shouldnt.  this is done in the board resources via the speed_hz
>> field of the spi_board_info struct on a per-spi device setting.
>
> Thank you for the hint.  spi_board_info has a max_speed_hz field - it does
> not have a speed_hz field.  The various platforms all seem to set
> max_speed_hz, so perhaps that is what you meant to say.
>
> In my case, max_speed_hz is being correctly set, but that doesn't seem to be
> enough.  I have traced through the calling hierarchy, and this is what I got:
>
> 1) m25p80_read builds a spi_message, and calls spi_sync to do the transfer.
>
> 2) spi_sync calls spi_async.  I added some printk, and saw speed_hz=0 and
>   max_speed_hz=50000000.  This is consistent with my platform setup (set via
>   a dts file).
>
> 3) spi_async calls through pointer "transfer" to spi_bitbang_transfer (because
>   the PPC4xx driver doesn't set its own transfer handler).
>
> 4) spi_bitbang_transfer calls spi_master_get_devdata, then enqueues the work
>
> 5) bitbang_work iterates through the queued transfers, and if speed_hz is
>   non-zero, bitbang_work calls through setup_transfer to spi_ppc4xx_setupxfer
>   which would set the divisor.  But, as noted in step 2, speed_hz=0, so the
>   spi bus speed is not set.  Rather, it remains at whatever speed some other
>   device chose.
>
> Note that bitbang_work looks at speed_hz, not max_speed_hz.  So, I come back to
> the same problem.  Somehow speed_hz must be set in order to make bitbang_work
> call setup_transfer, yet the only place that seems to happen is in spidev.

sounds like the bitbang SPI bus driver is broken.  if speed_hz is 0,
then the bus driver should fall back to the max_speed_hz from the spi
resources.
-mike

Re: [Question] m25p80 driver versus spi clock rate

[David Brownell]

On Tuesday 23 June 2009, Mike Frysinger wrote:
> On Tue, Jun 23, 2009 at 14:41, Steven A. Falco wrote:
> > Mike Frysinger wrote:
> >> On Mon, Jun 22, 2009 at 16:50, Steven A. Falco wrote:
> >>> I am trying to figure out how the mtd/devices/m25p80.c driver is supposed
> >>> to set the spi clock speed.  (Perhaps I'm making a bad assuption even to
> >>> think that it _should_ set the clock speed.  If so, please say so.)
> >>
> >> it shouldnt.  this is done in the board resources via the speed_hz
> >> field of the spi_board_info struct on a per-spi device setting.
> >
> > Thank you for the hint.  spi_board_info has a max_speed_hz field - it does
> > not have a speed_hz field.  The various platforms all seem to set
> > max_speed_hz, so perhaps that is what you meant to say.

Right.  A chip might support a much faster rate than is
achievable on a given board.  That's why setting the speed
limit is one of the duties of the board-specific setup code.


> > In my case, max_speed_hz is being correctly set, but that doesn't seem to be
> > enough.  I have traced through the calling hierarchy, and this is what I got:
> >
> > 1) m25p80_read builds a spi_message, and calls spi_sync to do the transfer.
> >
> > 2) spi_sync calls spi_async.  I added some printk, and saw speed_hz=0 and
> >   max_speed_hz=50000000.  This is consistent with my platform setup (set via
> >   a dts file).

What code are you talking about then?  Not the m25p80 code,
which just depends on platform code to have done its job.


> > 3) spi_async calls through pointer "transfer" to spi_bitbang_transfer (because
> >   the PPC4xx driver doesn't set its own transfer handler).
> >
> > 4) spi_bitbang_transfer calls spi_master_get_devdata, then enqueues the work
> >
> > 5) bitbang_work iterates through the queued transfers, and if speed_hz is
> >   non-zero, bitbang_work calls through setup_transfer to spi_ppc4xx_setupxfer
> >   which would set the divisor.  But, as noted in step 2, speed_hz=0, so the
> >   spi bus speed is not set.  Rather, it remains at whatever speed some other
> >   device chose.
> >
> > Note that bitbang_work looks at speed_hz, not max_speed_hz.  So, I come back to
> > the same problem.  Somehow speed_hz must be set in order to make bitbang_work
> > call setup_transfer, yet the only place that seems to happen is in spidev.
> 
> sounds like the bitbang SPI bus driver is broken.  if speed_hz is 0,
> then the bus driver should fall back to the max_speed_hz from the spi
> resources.

I just looked at that code, and didn't see any obvious issue.
It relies on initial setup to be correct, and then restores it
after any per-transfer override.

Maybe the problem is that the OF-to-SPI linkage is still borked.
Is it ensuring spi_setup() was called at device setup time?

- Dave




> -mike
> 
> 

Re: [Question] m25p80 driver versus spi clock rate

[Steven A. Falco]

David Brownell wrote:
> On Tuesday 23 June 2009, Mike Frysinger wrote:
>> On Tue, Jun 23, 2009 at 14:41, Steven A. Falco wrote:
>>> Mike Frysinger wrote:
>>>> On Mon, Jun 22, 2009 at 16:50, Steven A. Falco wrote:
>>> Note that bitbang_work looks at speed_hz, not max_speed_hz.  So, I come back to
>>> the same problem.  Somehow speed_hz must be set in order to make bitbang_work
>>> call setup_transfer, yet the only place that seems to happen is in spidev.
>> sounds like the bitbang SPI bus driver is broken.  if speed_hz is 0,
>> then the bus driver should fall back to the max_speed_hz from the spi
>> resources.
> 
> I just looked at that code, and didn't see any obvious issue.
> It relies on initial setup to be correct, and then restores it
> after any per-transfer override.

I have an m25p16 spi flash and an Atmel AVR on the spi bus.  These are
specified in a device tree (DTS file) - the m25p16 has a max frequency
of 50 MHz, and the AVR has a max frequency of 250 KHz.  When the kernel
boots, the m25p16 is registered to the m25p80 driver, and the AVR is
registered to the spidev (userland) driver.

As the devices are added, I see spi_ppc4xx_setup called for each one.
The first device discovered happens to be the m25p16, and so the bus
speed is first set to 50 MHz.  Next, the AVR happens to be discovered
and the bus is set to 250 KHz.

The max_speed_hz is set correctly for each device.  But, since the AVR
is listed second in the device tree, the spi bus winds up at the slower
clock rate of 250 KHz.  At this point, no actual transfers have been
done - we are just talking about initialization.

If I now access the m25p16 device, we go through the calling hierarchy
from my previous email.  As I said, speed_hz is 0, because m25p80
doesn't set it, max_speed_hz is 50 MHz as set by spi_ppc4xx_setup,
and the bus is still running at 250 KHz as described above.

bitbang_work never calls spi_ppc4xx_setupxfer, because speed_hz is 0.
So the transfer that should have happened at 50 MHz happens at 250 KHz.

> 
> Maybe the problem is that the OF-to-SPI linkage is still borked.
> Is it ensuring spi_setup() was called at device setup time?
> 

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.

I could reverse the order of the devices in the DTS file.  But then,
the bus would be initialized to 50 MHz.  Now, when I try to access
the AVR, and assuming I don't override the speed from userland, I will have
speed_hz=0, max_speed_hz=250 KHz, and the bus at 50 MHz.  Again, bitbang_work
won't call spi_ppc4xx_setupxfer because speed_hz=0, and the transaction will
fail, because the bus is at 50 MHz when it should be at 250 KHz.

This patch would fix it, by forcing bitbang_work to always set up the
transfer.  What do you think?

--- spi_bitbang.c.orig	2009-06-23 16:21:43.000000000 -0400
+++ spi_bitbang.c	2009-06-23 16:22:21.000000000 -0400
@@ -302,13 +302,10 @@
 
 		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;
-				if (!setup_transfer) {
-					status = -ENOPROTOOPT;
-					break;
-				}
+			setup_transfer = bitbang->setup_transfer;
+			if (!setup_transfer) {
+				status = -ENOPROTOOPT;
+				break;
 			}
 			if (setup_transfer) {
 				status = setup_transfer(spi, t);

Re: [Question] m25p80 driver versus spi clock rate

[David Brownell]

On Tuesday 23 June 2009, Steven A. Falco wrote:
> David Brownell wrote:
> > I just looked at that code, and didn't see any obvious issue.
> > It relies on initial setup to be correct, and then restores it
> > after any per-transfer override.

Summary of the key point below:  it does rely on that, but when
used with this ppc4xx driver that setup isn't getting done.

 
> I have an m25p16 spi flash and an Atmel AVR on the spi bus.  These are
> specified in a device tree (DTS file) - the m25p16 has a max frequency
> of 50 MHz, and the AVR has a max frequency of 250 KHz.  When the kernel
> boots, the m25p16 is registered to the m25p80 driver, and the AVR is
> registered to the spidev (userland) driver.
> 
> As the devices are added, I see spi_ppc4xx_setup called for each one.
> The first device discovered happens to be the m25p16, and so the bus
> speed is first set to 50 MHz.  Next, the AVR happens to be discovered
> and the bus is set to 250 KHz.
> 
> The max_speed_hz is set correctly for each device.  But, since the AVR
> is listed second in the device tree, the spi bus winds up at the slower
> clock rate of 250 KHz.  At this point, no actual transfers have been
> done - we are just talking about initialization.

Note the conceptual problem goof there too:  until a transfer to
some device is active, talking about "bus speed" is meaningless.


> If I now access the m25p16 device, we go through the calling hierarchy
> from my previous email.  As I said, speed_hz is 0, because m25p80
> doesn't set it, max_speed_hz is 50 MHz as set by spi_ppc4xx_setup,
> and the bus is still running at 250 KHz as described above.
> 
> bitbang_work never calls spi_ppc4xx_setupxfer, because speed_hz is 0.
> So the transfer that should have happened at 50 MHz happens at 250 KHz.
> 
> > 
> > Maybe the problem is that the OF-to-SPI linkage is still borked.
> > Is it ensuring spi_setup() was called at device setup time?
> > 
> 
> 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.

- 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

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