[PATCH] spi: spi_mpc8xxx.c: Fix QE mode Litte Endian

[PATCH] spi: spi_mpc8xxx.c: Fix QE mode Litte Endian

[Joakim Tjernlund]

QE mode uses Little Endian so words > 8 bits are byte swapped.
Workaround this by always enforcing wordsize 8 for words
> 8 bits. Unfortunately this will not work for LSB transfers
where wordsize is > 8 bits so disable these for now.

Also move the different quirks into its own function to keep
mpc8xxx_spi_setup_transfer() sane.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
---
 drivers/spi/spi_mpc8xxx.c |  101 +++++++++++++++++++++++++++++++--------------
 1 files changed, 70 insertions(+), 31 deletions(-)

diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index c312d0e..c85dbbb 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c
@@ -285,36 +285,12 @@ static void mpc8xxx_spi_chipselect(struct spi_device *spi, int value)
 	}
 }
 
-static
-int mpc8xxx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
+static int
+mspi_apply_cpu_mode_quirks(struct spi_mpc8xxx_cs *cs,
+			   struct spi_device *spi,
+			   struct mpc8xxx_spi *mpc8xxx_spi,
+			   int bits_per_word)
 {
-	struct mpc8xxx_spi *mpc8xxx_spi;
-	u8 bits_per_word, pm;
-	u32 hz;
-	struct spi_mpc8xxx_cs	*cs = spi->controller_state;
-
-	mpc8xxx_spi = spi_master_get_devdata(spi->master);
-
-	if (t) {
-		bits_per_word = t->bits_per_word;
-		hz = t->speed_hz;
-	} else {
-		bits_per_word = 0;
-		hz = 0;
-	}
-
-	/* spi_transfer level calls that work per-word */
-	if (!bits_per_word)
-		bits_per_word = spi->bits_per_word;
-
-	/* Make sure its a bit width we support [4..16, 32] */
-	if ((bits_per_word < 4)
-	    || ((bits_per_word > 16) && (bits_per_word != 32)))
-		return -EINVAL;
-
-	if (!hz)
-		hz = spi->max_speed_hz;
-
 	cs->rx_shift = 0;
 	cs->tx_shift = 0;
 	if (bits_per_word <= 8) {
@@ -338,19 +314,82 @@ int mpc8xxx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
 		return -EINVAL;
 
 	if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE &&
-			spi->mode & SPI_LSB_FIRST) {
+	    spi->mode & SPI_LSB_FIRST) {
 		cs->tx_shift = 0;
 		if (bits_per_word <= 8)
 			cs->rx_shift = 8;
 		else
 			cs->rx_shift = 0;
 	}
-
 	mpc8xxx_spi->rx_shift = cs->rx_shift;
 	mpc8xxx_spi->tx_shift = cs->tx_shift;
 	mpc8xxx_spi->get_rx = cs->get_rx;
 	mpc8xxx_spi->get_tx = cs->get_tx;
 
+	return bits_per_word;
+}
+
+static int
+mspi_apply_qe_mode_quirks(struct spi_mpc8xxx_cs *cs,
+			  struct spi_device *spi,
+			  int bits_per_word)
+{
+	/* QE uses Little Endian for words > 8
+	 * so transform all words > 8 into 8 bits
+	 * Unfortnatly that doesn't work for LSB so
+	 * reject these for now */
+	/* Note: 32 bits word, LSB works iff
+	 * tfcr/rfcr is set to CPMFCR_GBL */
+	if (spi->mode & SPI_LSB_FIRST &&
+	    bits_per_word > 8)
+		return -EINVAL;
+	if (bits_per_word > 8)
+		return 8; /* pretend its 8 bits */
+	return bits_per_word;
+}
+
+static
+int mpc8xxx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
+{
+	struct mpc8xxx_spi *mpc8xxx_spi;
+	int bits_per_word;
+	u8 pm;
+	u32 hz;
+	struct spi_mpc8xxx_cs	*cs = spi->controller_state;
+
+	mpc8xxx_spi = spi_master_get_devdata(spi->master);
+
+	if (t) {
+		bits_per_word = t->bits_per_word;
+		hz = t->speed_hz;
+	} else {
+		bits_per_word = 0;
+		hz = 0;
+	}
+
+	/* spi_transfer level calls that work per-word */
+	if (!bits_per_word)
+		bits_per_word = spi->bits_per_word;
+
+	/* Make sure its a bit width we support [4..16, 32] */
+	if ((bits_per_word < 4)
+	    || ((bits_per_word > 16) && (bits_per_word != 32)))
+		return -EINVAL;
+
+	if (!hz)
+		hz = spi->max_speed_hz;
+
+	if (!(mpc8xxx_spi->flags & SPI_CPM_MODE))
+		bits_per_word = mspi_apply_cpu_mode_quirks(cs, spi,
+							   mpc8xxx_spi,
+							   bits_per_word);
+	else if (mpc8xxx_spi->flags & SPI_QE)
+		bits_per_word = mspi_apply_qe_mode_quirks(cs, spi,
+							  bits_per_word);
+
+	if (bits_per_word < 0)
+		return bits_per_word;
+
 	if (bits_per_word == 32)
 		bits_per_word = 0;
 	else
-- 
1.6.4.4


------------------------------------------------------------------------------

[PATCH] spi: spidev_test.c: Make transfer size suitable for wordsize 32

[Joakim Tjernlund]

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
---
 Documentation/spi/spidev_test.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/Documentation/spi/spidev_test.c b/Documentation/spi/spidev_test.c
index 10abd37..3978e69 100644
--- a/Documentation/spi/spidev_test.c
+++ b/Documentation/spi/spidev_test.c
@@ -45,7 +45,6 @@ static void transfer(int fd)
 		0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
 		0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
 		0xDE, 0xAD, 0xBE, 0xEF, 0xBA, 0xAD,
-		0xF0, 0x0D,
 	};
 	uint8_t rx[ARRAY_SIZE(tx)] = {0, };
 	struct spi_ioc_transfer tr = {
-- 
1.6.4.4


------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: Fix QE mode Litte Endian

[Anton Vorontsov]

On Fri, May 14, 2010 at 11:14:26AM +0200, Joakim Tjernlund wrote:
> QE mode uses Little Endian so words > 8 bits are byte swapped.
> Workaround this by always enforcing wordsize 8 for words
> > 8 bits. Unfortunately this will not work for LSB transfers
> where wordsize is > 8 bits so disable these for now.
> 
> Also move the different quirks into its own function to keep
> mpc8xxx_spi_setup_transfer() sane.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
> ---

Looks OK, thanks!

Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

------------------------------------------------------------------------------

Re: [PATCH] spi: spidev_test.c: Make transfer size suitable for wordsize 32

[Anton Vorontsov]

On Fri, May 14, 2010 at 11:14:27AM +0200, Joakim Tjernlund wrote:
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>

Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Thanks!

> ---
>  Documentation/spi/spidev_test.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/spi/spidev_test.c b/Documentation/spi/spidev_test.c
> index 10abd37..3978e69 100644
> --- a/Documentation/spi/spidev_test.c
> +++ b/Documentation/spi/spidev_test.c
> @@ -45,7 +45,6 @@ static void transfer(int fd)
>  		0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
>  		0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
>  		0xDE, 0xAD, 0xBE, 0xEF, 0xBA, 0xAD,
> -		0xF0, 0x0D,
>  	};
>  	uint8_t rx[ARRAY_SIZE(tx)] = {0, };
>  	struct spi_ioc_transfer tr = {
> -- 
> 1.6.4.4

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

------------------------------------------------------------------------------

Re: [PATCH] spi: spidev_test.c: Make transfer size suitable for wordsize 32

[Grant Likely]

On Fri, May 14, 2010 at 6:22 AM, Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, May 14, 2010 at 11:14:27AM +0200, Joakim Tjernlund wrote:
>>
>> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
>
> Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

I don't think I'll apply this one.  I know it's just sample code, but
it still smells of masking a corner case.  If you really want to do
this, then I'd suggest adding a runtime switch to change the transfer
size.

g.

------------------------------------------------------------------------------

Re: [PATCH] spi: spidev_test.c: Make transfer size suitable for wordsize 32

[Joakim Tjernlund]

glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org wrote on 2010/05/22 10:39:38:
>
> On Fri, May 14, 2010 at 6:22 AM, Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Fri, May 14, 2010 at 11:14:27AM +0200, Joakim Tjernlund wrote:
> >>
> >> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
> >
> > Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> I don't think I'll apply this one.  I know it's just sample code, but
> it still smells of masking a corner case.  If you really want to do
> this, then I'd suggest adding a runtime switch to change the transfer
> size.

How so? The current version don't stand a chance to do any ops. with word size 32
as all transfers has to have a wordsize aligned len. This is not a corner case.

 Jocke


------------------------------------------------------------------------------

Re: [PATCH] spi: spidev_test.c: Make transfer size suitable for wordsize 32

[Grant Likely]

On Sat, May 22, 2010 at 8:34 AM, Joakim Tjernlund
<joakim.tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org> wrote:
> glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org wrote on 2010/05/22 10:39:38:
>>
>> On Fri, May 14, 2010 at 6:22 AM, Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > On Fri, May 14, 2010 at 11:14:27AM +0200, Joakim Tjernlund wrote:
>> >>
>> >> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
>> >
>> > Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> I don't think I'll apply this one.  I know it's just sample code, but
>> it still smells of masking a corner case.  If you really want to do
>> this, then I'd suggest adding a runtime switch to change the transfer
>> size.
>
> How so? The current version don't stand a chance to do any ops. with word size 32
> as all transfers has to have a wordsize aligned len. This is not a corner case.

but it also changes the transfer size for 16 and 8 bpw transfers to
something, for lack of a better word, more boring.  I'd rather see a
patch that clamps the transfers size based on the bpw setting.  Bonus
points for a patch that adds the ability to change the transfer size
and/or data from the command line.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

------------------------------------------------------------------------------

Re: [PATCH] spi: spidev_test.c: Make transfer size suitable for wordsize 32

[Joakim Tjernlund]

glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org wrote on 2010/05/25 21:41:15:
>
> On Sat, May 22, 2010 at 8:34 AM, Joakim Tjernlund
> <joakim.tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org> wrote:
> > glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org wrote on 2010/05/22 10:39:38:
> >>
> >> On Fri, May 14, 2010 at 6:22 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> >> > On Fri, May 14, 2010 at 11:14:27AM +0200, Joakim Tjernlund wrote:
> >> >>
> >> >> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
> >> >
> >> > Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >> I don't think I'll apply this one.  I know it's just sample code, but
> >> it still smells of masking a corner case.  If you really want to do
> >> this, then I'd suggest adding a runtime switch to change the transfer
> >> size.
> >
> > How so? The current version don't stand a chance to do any ops. with word size 32
> > as all transfers has to have a wordsize aligned len. This is not a corner case.
>
> but it also changes the transfer size for 16 and 8 bpw transfers to
> something, for lack of a better word, more boring.  I'd rather see a

That is a rather lame argument.

> patch that clamps the transfers size based on the bpw setting.  Bonus
> points for a patch that adds the ability to change the transfer size
> and/or data from the command line.

This is better though and yes it would be much better if one
could control the size. I didn't bother with that though as
I just wanted to fix the driver and the current one doesn't
work at all so I did the minimum fix to get it going.
I am fine either way whether you apply this patch or not, but
I won't work on fixing spidev_test until the next time I need
it though. Too much other stuff on my plate and the test eq. has
already been claimed by others.


------------------------------------------------------------------------------