[PATCH] spi_sh_msiof: Fixed data sampling on the correct edge

[PATCH] spi_sh_msiof: Fixed data sampling on the correct edge

[Pietrek, Markus]

Hi,

it seems to me that the spi_sh_msiof.c driver configures REDG and TEDG wrongly. TEDG=0 outputs data at the **rising edge** of the clock and REDG=0 samples data at the **falling edge** of the clock. Therefore for SPI, TEDG must be equal to REDG, otherwise the last byte received is not sampled in the SPI mode 3

The SH7723 HW Reference Manual explains the setting in Figure 20.20 and Figure 20.21 ("SPI Clock and data timing")

diff --git a/drivers/spi/spi_sh_msiof.c b/drivers/spi/spi_sh_msiof.c
index 51e5e1d..a9fec2f 100644
--- a/drivers/spi/spi_sh_msiof.c
+++ b/drivers/spi/spi_sh_msiof.c
@@ -173,13 +173,11 @@ static void sh_msiof_spi_set_pin_regs(struct sh_msiof_spi_priv *p,
        int edge;

        /*
-        * CPOL CPHA     TSCKIZ RSCKIZ TEDG REDG(!)
-        *    0    0         10     10    1    0
-        *    0    1         10     10    0    1
-        *    1    0         11     11    0    1
-        *    1    1         11     11    1    0
-        *
-        * (!) Note: REDG is inverted recommended data sheet setting
+        * CPOL CPHA     TSCKIZ RSCKIZ TEDG REDG
+        *    0    0         10     10    1    1
+        *    0    1         10     10    0    0
+        *    1    0         11     11    0    0
+        *    1    1         11     11    1    1
         */

        sh_msiof_write(p, FCTR, 0);
@@ -193,7 +191,7 @@ static void sh_msiof_spi_set_pin_regs(struct sh_msiof_spi_priv *p,
        edge = cpol ? cpha : !cpha;

        tmp |= edge << 27; /* TEDG */
-       tmp |= !edge << 26; /* REDG */
+       tmp |= edge << 26; /* REDG */
        tmp |= (tx_hi_z ? 2 : 0) << 22; /* TXDIZ */
        sh_msiof_write(p, CTR, tmp);
 }
@@ -280,6 +278,9 @@ static void sh_msiof_spi_read_fifo_8(struct sh_msiof_spi_priv *p,

        for (k = 0; k < words; k++)
                buf_8[k] = sh_msiof_read(p, RFDR) >> fs;
+
+       for (k = 0; k < words; k++)
+               pr_err(" %02x\n", buf_8[k]);
 }

 static void sh_msiof_spi_read_fifo_16(struct sh_msiof_spi_priv *p,

_____________________________________

Amtsgericht Mannheim
HRB 110 300
Gesch?ftsf?hrer: Dieter Baur, Ramona Maurer
_____________________________________

Important Note:
- This e-mail may contain trade secrets or privileged, undisclosed or otherwise confidential information.
- If you have received this e-mail in error, you are hereby notified that any review, copying or distribution of it is strictly prohibited.
- Please inform us immediately and destroy the original transmittal.

Thank you for your cooperation.

Re: [PATCH] spi_sh_msiof: Fixed data sampling on the correct edge

[Magnus Damm]

Hi Markus,

On Thu, Jan 21, 2010 at 6:26 PM, Pietrek, Markus
<Markus.Pietrek@emtrion.de> wrote:
> it seems to me that the spi_sh_msiof.c driver configures REDG and TEDG wrongly. TEDG=0 outputs data at the **rising edge** of the clock and REDG=0 samples data at the **falling edge** of the clock. Therefore for SPI, TEDG must be equal to REDG, otherwise the last byte received is not sampled in the SPI mode 3
>
> The SH7723 HW Reference Manual explains the setting in Figure 20.20 and Figure 20.21 ("SPI Clock and data timing")

Nice to hear that someone is using the msiof driver! Your patch, is it
for a theoretical data sheet mismatch issue only?

I used your version of the register settings initially when hacked up
the driver, but after testing on my sh7724 board and looking at the
results in a scope I decided to use the inverted bit setting. I may be
wrong though, so please back up your patch with some real world test
results. =)

/ magnus

AW: [PATCH] spi_sh_msiof: Fixed data sampling on the correct edge

[Pietrek, Markus]

Hi Magnus,

of course there are real world examples on the SH7723 ;-) And I'm happy that a driver is now available otherwise I had to write it on my own.

I'm using spidev to communicate with a samsung oled display S6E63D6. This display requires the mode SPI_CPHA and SPI_CPOL.

Plain writing is fine with the old register setting (REDG=!TEDG). I can configure the display. But when reading the chip identifier, the last byte (of the identified 0x63D6) is never stored in the FIFO. I always read 0x6363, so the FIFO returns the 2nd last byte as the last byte.

With a logical analyzer I can proof that the device is sending the correct data. So it must be an issue of the sampling. And changing REDG/TEDG to the values recommended in the SH7723 HW Reference Manual fixed this issue.

Best regards,
Markus

> -----Ursprüngliche Nachricht-----
> Von: Magnus Damm [mailto:magnus.damm@gmail.com]
> Gesendet: Donnerstag, 21. Januar 2010 11:02
> An: Pietrek, Markus
> Cc: linux-sh@vger.kernel.org
> Betreff: Re: [PATCH] spi_sh_msiof: Fixed data sampling on the correct
> edge
>
> Hi Markus,
>
> On Thu, Jan 21, 2010 at 6:26 PM, Pietrek, Markus
> <Markus.Pietrek@emtrion.de> wrote:
> > it seems to me that the spi_sh_msiof.c driver configures REDG and
> TEDG wrongly. TEDG=0 outputs data at the **rising edge** of the clock
> and REDG=0 samples data at the **falling edge** of the clock.
> Therefore for SPI, TEDG must be equal to REDG, otherwise the last byte
> received is not sampled in the SPI mode 3
> >
> > The SH7723 HW Reference Manual explains the setting in Figure 20.20
> and Figure 20.21 ("SPI Clock and data timing")
>
> Nice to hear that someone is using the msiof driver! Your patch, is it
> for a theoretical data sheet mismatch issue only?
>
> I used your version of the register settings initially when hacked up
> the driver, but after testing on my sh7724 board and looking at the
> results in a scope I decided to use the inverted bit setting. I may be
> wrong though, so please back up your patch with some real world test
> results. =)
>
> / magnus

_____________________________________

Amtsgericht Mannheim
HRB 110 300
Geschäftsführer: Dieter Baur, Ramona Maurer
_____________________________________

Important Note:
- This e-mail may contain trade secrets or privileged, undisclosed or otherwise confidential information.
- If you have received this e-mail in error, you are hereby notified that any review, copying or distribution of it is strictly prohibited.
- Please inform us immediately and destroy the original transmittal.

Thank you for your cooperation.

Re: [PATCH] spi_sh_msiof: Fixed data sampling on the correct edge

[Magnus Damm]

Hi Markus,

On Thu, Jan 21, 2010 at 7:28 PM, Pietrek, Markus
<Markus.Pietrek@emtrion.de> wrote:
> Hi Magnus,
>
> of course there are real world examples on the SH7723 ;-) And I'm happy that a driver is now available otherwise I had to write it on my own.

Good to hear. I appreciate your patch!

> I'm using spidev to communicate with a samsung oled display S6E63D6. This display requires the mode SPI_CPHA and SPI_CPOL.
>
> Plain writing is fine with the old register setting (REDG=!TEDG). I can configure the display. But when reading the chip identifier, the last byte (of the identified 0x63D6) is never stored in the FIFO. I always read 0x6363, so the FIFO returns the 2nd last byte as the last byte.

Huh, is that so?

I tested this on my Ecovec sh7724 board with MMC-over-SPI and your
patch does not break my setup...

> With a logical analyzer I can proof that the device is sending the correct data. So it must be an issue of the sampling. And changing REDG/TEDG to the values recommended in the SH7723 HW Reference Manual fixed this issue.

.. and following the data sheet sounds like a good plan. Especially
since it fixes your issue.

Can you please send a patch without the debug printouts to
spi-devel-general and add my acked-by?

Acked-by: Magnus Damm <damm@opensource.se>

Thanks a lot!

/ magnus

Re: [PATCH] spi_sh_msiof: Fixed data sampling on the correct edge

[Paul Mundt]

On Wed, Jan 27, 2010 at 04:23:36PM +0900, Magnus Damm wrote:
> On Thu, Jan 21, 2010 at 7:28 PM, Pietrek, Markus
> <Markus.Pietrek@emtrion.de> wrote:
> > I'm using spidev to communicate with a samsung oled display S6E63D6.
> > This display requires the mode SPI_CPHA and SPI_CPOL.
> >
> > Plain writing is fine with the old register setting (REDG=!TEDG). I
> > can configure the display. But when reading the chip identifier, the
> > last byte (of the identified 0x63D6) is never stored in the FIFO. I
> > always read 0x6363, so the FIFO returns the 2nd last byte as the last
> > byte.
> 
> Huh, is that so?
> 
> I tested this on my Ecovec sh7724 board with MMC-over-SPI and your
> patch does not break my setup...
> 
> > With a logical analyzer I can proof that the device is sending the
> > correct data. So it must be an issue of the sampling. And changing
> > REDG/TEDG to the values recommended in the SH7723 HW Reference Manual
> > fixed this issue.
> 
> .. and following the data sheet sounds like a good plan. Especially
> since it fixes your issue.
> 
> Can you please send a patch without the debug printouts to
> spi-devel-general and add my acked-by?
> 
> Acked-by: Magnus Damm <damm@opensource.se>
> 
This looks like something we should handle for 2.6.33 anyways, so Markus
when you resubmit this make sure to provide your Signed-off-by as well,
then I'll batch it up with the next round of updates, thanks.