atmel_spi clock polarity

atmel_spi clock polarity

[Atsushi Nemoto]

I found that atmel_spi driver does not initialize clock polarity
correctly (except for at91rm9200 CS0 channel) in some case.

The atmel_spi driver uses gpio-controlled chipselect.  OTOH spi clock
signal is controlled by CSRn.CPOL bit, but this register controls
clock signal correctly only in 'real transfer' duration.  At the time
of cs_activate() call, CSRn.CPOL will be initialized correctly, but
the controller do not know which channel is to be used next, so clock
signal will stay at the inactive state of last transfer.  If clock
polarity of new transfer and last transfer was differ, new transfer
will start with wrong clock signal state.

For example, if you started SPI MODE 2 or 3 transfer after SPI MODE 0
or 1 transfer, the clock signal state at the assertion of chipselect
will be low.  Of course this will violates SPI transfer.


Here is my quick workaround for this problem.  It makes all CSRn.CPOL
match for the transfer before activating chipselect.  I'm not quite
sure my analysis is correct, and there might be better solution.
Could you give me any comments?


diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c
index 293b7ca..85687aa 100644
--- a/drivers/spi/atmel_spi.c
+++ b/drivers/spi/atmel_spi.c
@@ -87,6 +87,16 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
 	unsigned gpio = (unsigned) spi->controller_data;
 	unsigned active = spi->mode & SPI_CS_HIGH;
 	u32 mr;
+	int i;
+	u32 csr;
+	u32 cpol = (spi->mode & SPI_CPOL) ? SPI_BIT(CPOL) : 0;
+
+	/* Make sure clock polarity is correct */
+	for (i = 0; i < spi->master->num_chipselect; i++) {
+		csr = spi_readl(as, CSR0 + 4 * i);
+		if ((csr ^ cpol) & SPI_BIT(CPOL))
+			spi_writel(as, CSR0 + 4 * i, csr ^ SPI_BIT(CPOL));
+	}
 
 	mr = spi_readl(as, MR);
 	mr = SPI_BFINS(PCS, ~(1 << spi->chip_select), mr);

---
Atsushi Nemoto

Re: atmel_spi clock polarity

[Haavard Skinnemoen]

On Sat, 16 Feb 2008 22:32:52 +0900 (JST)
Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org> wrote:

> Here is my quick workaround for this problem.  It makes all CSRn.CPOL
> match for the transfer before activating chipselect.  I'm not quite
> sure my analysis is correct, and there might be better solution.
> Could you give me any comments?

I'm not sure if I fully understand what problem you're seeing. Is the
clock state wrong when the chip select is activated? If so, does the
patch below help?

Haavard

diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c
index 293b7ca..4f19b82 100644
--- a/drivers/spi/atmel_spi.c
+++ b/drivers/spi/atmel_spi.c
@@ -90,6 +90,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
 
 	mr = spi_readl(as, MR);
 	mr = SPI_BFINS(PCS, ~(1 << spi->chip_select), mr);
+	spi_writel(as, MR, mr);
 
 	dev_dbg(&spi->dev, "activate %u%s, mr %08x\n",
 			gpio, active ? " (high)" : "",
@@ -97,7 +98,6 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
 
 	if (!(cpu_is_at91rm9200() && spi->chip_select == 0))
 		gpio_set_value(gpio, active);
-	spi_writel(as, MR, mr);
 }
 
 static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi)

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: atmel_spi clock polarity

[Atsushi Nemoto]

On Mon, 18 Feb 2008 12:42:37 +0100, Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> > Here is my quick workaround for this problem.  It makes all CSRn.CPOL
> > match for the transfer before activating chipselect.  I'm not quite
> > sure my analysis is correct, and there might be better solution.
> > Could you give me any comments?
> 
> I'm not sure if I fully understand what problem you're seeing. Is the
> clock state wrong when the chip select is activated?

Yes.

> If so, does the patch below help?

Hmm... It might fix my problem.  But IIRC the clock state follows
CSRn.CPOL just before the real transfer.  Like this (previous transfer
was MODE 0, new transfer is MODE 3):

           T0                T1 T2

CS      ~~~|________________________________________________

CLK     ______________________|~|___|~~~|___|~~~|___|~~~|___

SO      ~~~~~~~~~~~~~~~~~~~~~~~~~~|___|~~~|___|~~~|___|~~~|_
                                   MSB

T0-T1 was relatively longer then T1-T2.  I suppose T1 is not the
point of updating MR register, but the point of starting DMA transfer.

Anyway, I will try your patch in a few days.

---
Atsushi Nemoto

Re: atmel_spi clock polarity

[Haavard Skinnemoen]

On Mon, 18 Feb 2008 23:12:43 +0900 (JST)
Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org> wrote:

> T0-T1 was relatively longer then T1-T2.  I suppose T1 is not the
> point of updating MR register, but the point of starting DMA transfer.

Aw. I see.

> Anyway, I will try your patch in a few days.

Ok, thanks. If it works, that would be great, but given your
description above I'm not sure if I dare hope for it.

Hmm...I suppose we could just use CSR0 for all transfers and not
bother updating MR. That might actually be cheaper than doing it
"the right way"...and allow us to support an arbitrary number of
chipselects instead of just four.

But I guess the AT91RM9200 won't be too happy about that...

Haavard

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: atmel_spi clock polarity

[Atsushi Nemoto]

On Mon, 18 Feb 2008 15:31:58 +0100, Haavard Skinnemoen <hskinnemoen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> > Anyway, I will try your patch in a few days.
> 
> Ok, thanks. If it works, that would be great, but given your
> description above I'm not sure if I dare hope for it.

Unfortunately it did not work.  The clock state did not change by
writing MR register.

---
Atsushi Nemoto

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: atmel_spi clock polarity

[Haavard Skinnemoen]

On Wed, 20 Feb 2008 14:21:09 +0900 (JST)
Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org> wrote:

> On Mon, 18 Feb 2008 15:31:58 +0100, Haavard Skinnemoen <hskinnemoen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> > > Anyway, I will try your patch in a few days.
> > 
> > Ok, thanks. If it works, that would be great, but given your
> > description above I'm not sure if I dare hope for it.
> 
> Unfortunately it did not work.  The clock state did not change by
> writing MR register.

Ok, thanks for testing.

In that case, I think the best fix is to let NPCS0 stay selected
permanently in MR and overwrite CSR0 with to the new slave's settings
before asserting CS. But that's a more complicated change, and I don't
know how it will affect the AT91RM9200 special cases.

So I suggest we merge your patch for 2.6.25, and try to optimize it
for 2.6.26.

David, do you want me to pass on the patch with my signoff or just ask
Andrew to add my Acked-by to the patch already in mm?

Haavard

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: atmel_spi clock polarity

[Atsushi Nemoto]

On Wed, 20 Feb 2008 10:34:46 +0100, Haavard Skinnemoen <hskinnemoen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> In that case, I think the best fix is to let NPCS0 stay selected
> permanently in MR and overwrite CSR0 with to the new slave's settings
> before asserting CS. But that's a more complicated change, and I don't
> know how it will affect the AT91RM9200 special cases.
> 
> So I suggest we merge your patch for 2.6.25, and try to optimize it
> for 2.6.26.

I absolutely agree.

> David, do you want me to pass on the patch with my signoff or just ask
> Andrew to add my Acked-by to the patch already in mm?

The patch in mm also lacks my Signed-off line.  I had thought Andrew
never take a patch without Signed-off line ;)

Should I resend my patch with my Signed-off and Haavard's Acked-by?

---
Atsushi Nemoto

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: atmel_spi clock polarity

[David Brownell]

On Wednesday 20 February 2008, Atsushi Nemoto wrote:
> > David, do you want me to pass on the patch with my signoff or just ask
> > Andrew to add my Acked-by to the patch already in mm?
> 
> The patch in mm also lacks my Signed-off line.  I had thought Andrew
> never take a patch without Signed-off line ;)
> 
> Should I resend my patch with my Signed-off and Haavard's Acked-by?

Good point.  I think that's the way to go.  

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: atmel_spi clock polarity

[David Brownell]

On Wednesday 20 February 2008, Haavard Skinnemoen wrote:
> > 
> > Unfortunately it did not work.  The clock state did not change by
> > writing MR register.
> 
> Ok, thanks for testing.
> 
> In that case, I think the best fix is to let NPCS0 stay selected
> permanently in MR and overwrite CSR0 with to the new slave's settings
> before asserting CS. But that's a more complicated change, and I don't
> know how it will affect the AT91RM9200 special cases.

The rm9200 special cases which, ISTR, still don't work right ...


> So I suggest we merge your patch for 2.6.25, and try to optimize it
> for 2.6.26.
> 
> David, do you want me to pass on the patch with my signoff or just ask
> Andrew to add my Acked-by to the patch already in mm?

It'd be good to let Andrew have your ack.  It's already in MM, so
if I don't need to sign off it's nice to have less to do there.  :)

- Dave

 



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: atmel_spi clock polarity

[David Brownell]

On Monday 18 February 2008, Atsushi Nemoto wrote:
> 	 IIRC the clock state follows
> CSRn.CPOL just before the real transfer.

No ... clock state should be valid *before* chipselect goes
active.  So I'm thinking the patch from Haavard is likely
the right change.


> Like this (previous transfer 
> was MODE 0, new transfer is MODE 3):
> 
>            T0                T1 T2
> 
> CS      ~~~|________________________________________________

So at T0, some chip is selected (and never deselected) ...

> 
> CLK     ______________________|~|___|~~~|___|~~~|___|~~~|___

... and at T1 CPOL is changed??  That's wrong.  There should
never be a partial clock period while a chipselect is active.
While it's inactive, sure -- no chip should care.


> 
> SO      ~~~~~~~~~~~~~~~~~~~~~~~~~~|___|~~~|___|~~~|___|~~~|_
>                                    MSB
> 
> T0-T1 was relatively longer then T1-T2.  I suppose T1 is not the
> point of updating MR register, but the point of starting DMA transfer.
> 

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: atmel_spi clock polarity

[Haavard Skinnemoen]

On Mon, 18 Feb 2008 11:57:56 -0800
David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:

> On Monday 18 February 2008, Atsushi Nemoto wrote:
> > 	 IIRC the clock state follows
> > CSRn.CPOL just before the real transfer.
> 
> No ... clock state should be valid *before* chipselect goes
> active.  So I'm thinking the patch from Haavard is likely
> the right change.

Hopefully it is...

> > CLK     ______________________|~|___|~~~|___|~~~|___|~~~|___
> 
> ... and at T1 CPOL is changed??  That's wrong.  There should
> never be a partial clock period while a chipselect is active.
> While it's inactive, sure -- no chip should care.

...but what I'm afraid of is that since we're using GPIO chipselects,
the controller may _think_ that no chip is selected and change the
clock polarity just before it would pull the chipselect low if we were
using automatic chipselects...

If that's the case, it would be a bit strange if it actually helped to
initialize all the CSRn registers, but it would also offer us a nice
workaround...

Atsushi, do you by any chance have debugging enabled? That would
explain the long delay from CS activation to the change of clock
polarity. Compared to printk() the DMA setup takes almost no time at
all.

If you can confirm that my patch helps, I think that's the one we want
in mainline.

Haavard

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: atmel_spi clock polarity

[Atsushi Nemoto]

On Mon, 18 Feb 2008 23:49:18 +0100, Haavard Skinnemoen <hskinnemoen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> > > CLK     ______________________|~|___|~~~|___|~~~|___|~~~|___
> > 
> > ... and at T1 CPOL is changed??  That's wrong.  There should
> > never be a partial clock period while a chipselect is active.
> > While it's inactive, sure -- no chip should care.
> 
> ...but what I'm afraid of is that since we're using GPIO chipselects,
> the controller may _think_ that no chip is selected and change the
> clock polarity just before it would pull the chipselect low if we were
> using automatic chipselects...

Yes.  That's I suppose.

> If that's the case, it would be a bit strange if it actually helped to
> initialize all the CSRn registers, but it would also offer us a nice
> workaround...

I suppose the clock state of the AT91 just follows _last_ transfer.
My patch (setting all CSRn.POL for next transfer) was based on that
assumption.

> Atsushi, do you by any chance have debugging enabled? That would
> explain the long delay from CS activation to the change of clock
> polarity. Compared to printk() the DMA setup takes almost no time at
> all.

No, I did not enabled debugging.

> If you can confirm that my patch helps, I think that's the one we want
> in mainline.

Unfortunately I had no time to try it today.  Hopefully tomorrow...

---
Atsushi Nemoto

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/