Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Christophe Leroy]

Hello,

I'm wondering how to solve the following issue:

On powerpc CPM soc, SPI handles words in reversed byte order. Therefore 
when some drivers like the GPIO max7301 driver request 16 bits data, 
data is sent in wrong byte order.

$ git grep bits_per_word gpio-max7301.c
gpio-max7301.c: /* bits_per_word cannot be configured in platform data */
gpio-max7301.c:         spi->bits_per_word = 16;

We could do as in spi-fsl-spi.c SPI driver for the QE soc and force 8 
bits transfer at all time:

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;
}



But this would be a pitty because 8 bits mode is far less efficient and 
requires much lower transfer speed on powerpc CPM soc.

I have some (out of tree for now) driver that is used to load a FPGA 
through SPI and has to transfer as much as 300 kbytes data. That driver 
knows that words have to be submitted in reversed byte order and is able 
to use the SPI driver at full speed in 16 bits transfer mode, but when I 
use max7301 GPIO driver it is a problem.

What could be the best solution to force 8 bits transfer on generic 
drivers like GPIO max7301 while still allowing aware drivers like my 
FPGA loader to use 16 bits mode by reversing words by itself ?

Thanks
Christophe

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Tue, Mar 07, 2023 at 06:21:36PM +0000, Christophe Leroy wrote:
> 
> On powerpc CPM soc, SPI handles words in reversed byte order. Therefore 
> when some drivers like the GPIO max7301 driver request 16 bits data, 
> data is sent in wrong byte order.

Clearly if the device is byte swapping the data incorrectly then you'll
need to do an extra swap in the CPU before sending to the device, or get
the DMA controller to do it if that's a feature it has, when using the
16 bit mode.

> We could do as in spi-fsl-spi.c SPI driver for the QE soc and force 8 
> bits transfer at all time:

Right, that's the simplest thing.

> What could be the best solution to force 8 bits transfer on generic 
> drivers like GPIO max7301 while still allowing aware drivers like my 
> FPGA loader to use 16 bits mode by reversing words by itself ?

If your FPGA loader is already reversing words you may as well just move
it into the controller driver.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Christophe Leroy]

Le 07/03/2023 à 19:35, Mark Brown a écrit :
> On Tue, Mar 07, 2023 at 06:21:36PM +0000, Christophe Leroy wrote:
>>
>> On powerpc CPM soc, SPI handles words in reversed byte order. Therefore
>> when some drivers like the GPIO max7301 driver request 16 bits data,
>> data is sent in wrong byte order.
> 
> Clearly if the device is byte swapping the data incorrectly then you'll
> need to do an extra swap in the CPU before sending to the device, or get
> the DMA controller to do it if that's a feature it has, when using the
> 16 bit mode.
> 
>> We could do as in spi-fsl-spi.c SPI driver for the QE soc and force 8
>> bits transfer at all time:
> 
> Right, that's the simplest thing.

Yes it is for the general case, but I need to be able to continue using 
that reverting 16 bits mode for some special cases like my FPGA loader.

> 
>> What could be the best solution to force 8 bits transfer on generic
>> drivers like GPIO max7301 while still allowing aware drivers like my
>> FPGA loader to use 16 bits mode by reversing words by itself ?
> 
> If your FPGA loader is already reversing words you may as well just move
> it into the controller driver.

Well, my loader reverts words "in place" because it is the one who has 
allocated and prepared the DMA buffer. But if we move that swaping into 
the SPI controller driver, it won't be possible to do it "in place" 
because the controller cannot know it the caller plans to re-use data 
after. And I'm not sure about the performance if we start having the 
controler to start kmallocing new DMA buffers on the fly.

What about a new flag at SPI device registration to tell the SPI 
subsystem that this SPI device driver wants "raw/native 16 bits" data 
being sent as-is, that my loader would set in order to tell the 
controler "I have prepared the data, send it in your native 16 bits 
order mode" ?
And for all devices not setting that flags, the controller would 
fallback to 8 bits mode.

Thanks
Christophe

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

On Tue, Mar 07, 2023 at 07:01:54PM +0000, Christophe Leroy wrote:

> What about a new flag at SPI device registration to tell the SPI 
> subsystem that this SPI device driver wants "raw/native 16 bits" data 
> being sent as-is, that my loader would set in order to tell the 
> controler "I have prepared the data, send it in your native 16 bits 
> order mode" ?
> And for all devices not setting that flags, the controller would 
> fallback to 8 bits mode.

Oh, so the issue is that your controller is *not* swapping data?  In
that case if 16 bit transfers are more efficient and a buffer formatted
for 8 bit transfers is already in the correct format then why not just
tell the controller to use 16 bit words where possible?  Nothing outside
the controller cares about anything other than the memory and wire
formats, if the controller correctly performs an 8 bit transfer when
programmed for 16 bit words and it's faster then just do that.  That
will work a lot better in general, drivers just sending a byte stream
aren't going to (and shouldn't) ask for anything other than 8 bit words.

Word size configuration should only be used by a client driver when it
wants things rewriting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Christophe Leroy]

Le 07/03/2023 à 20:19, Mark Brown a écrit :
> On Tue, Mar 07, 2023 at 07:01:54PM +0000, Christophe Leroy wrote:
> 
>> What about a new flag at SPI device registration to tell the SPI
>> subsystem that this SPI device driver wants "raw/native 16 bits" data
>> being sent as-is, that my loader would set in order to tell the
>> controler "I have prepared the data, send it in your native 16 bits
>> order mode" ?
>> And for all devices not setting that flags, the controller would
>> fallback to 8 bits mode.
> 
> Oh, so the issue is that your controller is *not* swapping data?  In
> that case if 16 bit transfers are more efficient and a buffer formatted
> for 8 bit transfers is already in the correct format then why not just
> tell the controller to use 16 bit words where possible?  Nothing outside
> the controller cares about anything other than the memory and wire
> formats, if the controller correctly performs an 8 bit transfer when
> programmed for 16 bit words and it's faster then just do that.  That
> will work a lot better in general, drivers just sending a byte stream
> aren't going to (and shouldn't) ask for anything other than 8 bit words.
> 
> Word size configuration should only be used by a client driver when it
> wants things rewriting.

No no, 8 bits mode is slower, or should I say it consumes more CPU for 
the same clock rate, which means we have to use slower rate in order to 
not saturate the RISC controller of the Communication Processor.

Well I not sure what you mean by swapping / not swapping data. Powerpc 
8xx is natively a big endian processor like all PPC32. But its 
Communication Processor (CPM) is apparently fetching data as little 
endian when told to perform transfer of 16 bits word on SPI.

So, my problem really is the GPIO MAX7301 driver which requests 16 bits 
transfers, because then the SPI controller sends the 2 bytes in reversed 
order. Do I understand correctly that from your point of view, that 
driver shouldn't request a 16 bits tranfer ? It is done here, in the 
max7301_probe() function, 
https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/gpio/gpio-max7301.c#L50

Because if I clamp the CPM SPI driver to 8 bits transfers, then I cannot 
anymore perform 16 bits transfer for loading my FPGA, then it means I 
must reduce data rate then loading the FPGA takes ages.

Christophe

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

On Tue, Mar 07, 2023 at 07:43:42PM +0000, Christophe Leroy wrote:
> Le 07/03/2023 à 20:19, Mark Brown a écrit :

> > Oh, so the issue is that your controller is *not* swapping data?  In
> > that case if 16 bit transfers are more efficient and a buffer formatted
> > for 8 bit transfers is already in the correct format then why not just
> > tell the controller to use 16 bit words where possible?  Nothing outside

...

> No no, 8 bits mode is slower, or should I say it consumes more CPU for 
> the same clock rate, which means we have to use slower rate in order to 
> not saturate the RISC controller of the Communication Processor.

Please read what I wrote above.

> Well I not sure what you mean by swapping / not swapping data. Powerpc 
> 8xx is natively a big endian processor like all PPC32. But its 
> Communication Processor (CPM) is apparently fetching data as little 
> endian when told to perform transfer of 16 bits word on SPI.

The default wire format for SPI is big endian (MSB first), as covered in
spi.h:

 * In-memory data values are always in native CPU byte order, translated
 * from the wire byte order (big-endian except with SPI_LSB_FIRST).  So
 * for example when bits_per_word is sixteen, buffers are 2N bytes long
 * (@len = 2N) and hold N sixteen bit words in CPU byte order.

LSB_FIRST has only one in tree user other than spidev so I'd question
how often it's correctly implemented.

> So, my problem really is the GPIO MAX7301 driver which requests 16 bits 
> transfers, because then the SPI controller sends the 2 bytes in reversed 
> order. Do I understand correctly that from your point of view, that 
> driver shouldn't request a 16 bits tranfer ? It is done here, in the 
> max7301_probe() function, 
> https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/gpio/gpio-max7301.c#L50

It would certainly improve interoperability with controllers to request
8 bit, but so long as the driver is reading and writing data in the
expected format it should work perfectly well.  Looking at the lack of
any endianness handling in the driver that doesn't seem to be the case
though, it's just handling data in CPU endian format which isn't
portable.

> Because if I clamp the CPM SPI driver to 8 bits transfers, then I cannot 
> anymore perform 16 bits transfer for loading my FPGA, then it means I 
> must reduce data rate then loading the FPGA takes ages.

Why?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Christophe Leroy]

Le 07/03/2023 à 22:10, Mark Brown a écrit :
> On Tue, Mar 07, 2023 at 07:43:42PM +0000, Christophe Leroy wrote:
>> Le 07/03/2023 à 20:19, Mark Brown a écrit :
> 
>>> Oh, so the issue is that your controller is *not* swapping data?  In
>>> that case if 16 bit transfers are more efficient and a buffer formatted
>>> for 8 bit transfers is already in the correct format then why not just
>>> tell the controller to use 16 bit words where possible?  Nothing outside
> 
> ...
> 
>> No no, 8 bits mode is slower, or should I say it consumes more CPU for
>> the same clock rate, which means we have to use slower rate in order to
>> not saturate the RISC controller of the Communication Processor.
> 
> Please read what I wrote above.

Ah sorry, I mis-read your text.

So yes, the problem is that the controller *is* swapping data: if I 
format the buffer to have 0x12 0x34 and tells the controller to send it 
as a 16 bits word, it will send 0x3412 whereas if I tell it to send as 8 
bits words, it will send 0x12 then 0x34.

So a driver like MAX7301 which wants to send 0x1234 as a 16-bits word 
will write 0x1234 into the buffer. The powerpc 8xx being big endian, I 
get 0x12 0x34 into the buffer then the controller sends word 0x3412.

> 
>> Well I not sure what you mean by swapping / not swapping data. Powerpc
>> 8xx is natively a big endian processor like all PPC32. But its
>> Communication Processor (CPM) is apparently fetching data as little
>> endian when told to perform transfer of 16 bits word on SPI.
> 
> The default wire format for SPI is big endian (MSB first), as covered in
> spi.h:
> 
>   * In-memory data values are always in native CPU byte order, translated
>   * from the wire byte order (big-endian except with SPI_LSB_FIRST).  So
>   * for example when bits_per_word is sixteen, buffers are 2N bytes long
>   * (@len = 2N) and hold N sixteen bit words in CPU byte order.
> 
> LSB_FIRST has only one in tree user other than spidev so I'd question
> how often it's correctly implemented.

Well, ok, I have no problem with the wire byte order, only with how the 
controller fetches the data from the DMA buffer. I'm not sure what I 
write is clear, is it ?

> 
>> So, my problem really is the GPIO MAX7301 driver which requests 16 bits
>> transfers, because then the SPI controller sends the 2 bytes in reversed
>> order. Do I understand correctly that from your point of view, that
>> driver shouldn't request a 16 bits tranfer ? It is done here, in the
>> max7301_probe() function,
>> https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/gpio/gpio-max7301.c#L50
> 
> It would certainly improve interoperability with controllers to request
> 8 bit, but so long as the driver is reading and writing data in the
> expected format it should work perfectly well.  Looking at the lack of
> any endianness handling in the driver that doesn't seem to be the case
> though, it's just handling data in CPU endian format which isn't
> portable.

I agree, it should work but it doesn't.
When I change max7301_probe() to set bits_per_word to 8 instead of 16 it 
works.

> 
>> Because if I clamp the CPM SPI driver to 8 bits transfers, then I cannot
>> anymore perform 16 bits transfer for loading my FPGA, then it means I
>> must reduce data rate then loading the FPGA takes ages.
> 
> Why?

Maybe what I wrote here isn't clear. What I mean is that if I modify CPM 
SPI controller driver to always use 8 bits mode (as done for the QE SPI 
controller via function mspi_apply_qe_mode_quirks() ), then it will work 
but I will not be able to use the same speed as in 16 bits transfer mode.

So, to sum up, the solutions I see:
A/ Force CPM SPI controller to always use 8 bits mode.
B/ Implement a flag to allow a SPI consumer to say "please perform 
tranfer with your bogus but performant 16 bits mode, I have worked 
around the data order for you"
C/ Implement a byte-swapping in the CPM SPI controller when a consumer 
asks for a 16-bits data transfer
D/ Modify MAX7301 GPIO driver to use 8 bits words instead of 16 bits words.

Solution A will degrade performance by forcing transfer rate reduction.
Solution B looks like a "home made" solution.
Solution C means copy-and-swap into a newly allocated DMA buffer.
Solution D is .... the best one ?

Any other idea ?

Which solution would you prefer ?

Thanks
Christophe

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 6370 bytes --]

On Tue, Mar 07, 2023 at 09:40:09PM +0000, Christophe Leroy wrote:
> Le 07/03/2023 à 22:10, Mark Brown a écrit :
> > On Tue, Mar 07, 2023 at 07:43:42PM +0000, Christophe Leroy wrote:
> >> Le 07/03/2023 à 20:19, Mark Brown a écrit :

> So yes, the problem is that the controller *is* swapping data: if I 
> format the buffer to have 0x12 0x34 and tells the controller to send it 
> as a 16 bits word, it will send 0x3412 whereas if I tell it to send as 8 
> bits words, it will send 0x12 then 0x34.

> So a driver like MAX7301 which wants to send 0x1234 as a 16-bits word 
> will write 0x1234 into the buffer. The powerpc 8xx being big endian, I 
> get 0x12 0x34 into the buffer then the controller sends word 0x3412.

So the CPU is big endian, memory has a big endian word in it and the
controller is then sending as little endian?  That would mean that the
driver for the controller is implementing SPI_LSB_FIRST mode without
being asked (IIRC there is an option to run these systems LE, I don't
know if the controller is just always little endian or always byte
swapping).  That seems like a bug in the driver for the controller.

> >> Well I not sure what you mean by swapping / not swapping data. Powerpc
> >> 8xx is natively a big endian processor like all PPC32. But its
> >> Communication Processor (CPM) is apparently fetching data as little
> >> endian when told to perform transfer of 16 bits word on SPI.

> > The default wire format for SPI is big endian (MSB first), as covered in
> > spi.h:

> >   * In-memory data values are always in native CPU byte order, translated
> >   * from the wire byte order (big-endian except with SPI_LSB_FIRST).  So
> >   * for example when bits_per_word is sixteen, buffers are 2N bytes long
> >   * (@len = 2N) and hold N sixteen bit words in CPU byte order.

> > LSB_FIRST has only one in tree user other than spidev so I'd question
> > how often it's correctly implemented.

> Well, ok, I have no problem with the wire byte order, only with how the 
> controller fetches the data from the DMA buffer. I'm not sure what I 
> write is clear, is it ?

You have a problem with the wire byte order not being what was
requested - the data should always be big endian unless otherwise
specified.

> >> So, my problem really is the GPIO MAX7301 driver which requests 16 bits
> >> transfers, because then the SPI controller sends the 2 bytes in reversed
> >> order. Do I understand correctly that from your point of view, that
> >> driver shouldn't request a 16 bits tranfer ? It is done here, in the
> >> max7301_probe() function,
> >> https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/gpio/gpio-max7301.c#L50

> > It would certainly improve interoperability with controllers to request
> > 8 bit, but so long as the driver is reading and writing data in the
> > expected format it should work perfectly well.  Looking at the lack of
> > any endianness handling in the driver that doesn't seem to be the case
> > though, it's just handling data in CPU endian format which isn't
> > portable.

> I agree, it should work but it doesn't.
> When I change max7301_probe() to set bits_per_word to 8 instead of 16 it 
> works.

So the driver should also work on a little endian system configured for
16 bit words - looking at the code I would only expect it to work on
one endianness of system.

> >> Because if I clamp the CPM SPI driver to 8 bits transfers, then I cannot
> >> anymore perform 16 bits transfer for loading my FPGA, then it means I
> >> must reduce data rate then loading the FPGA takes ages.

> > Why?

> Maybe what I wrote here isn't clear. What I mean is that if I modify CPM 
> SPI controller driver to always use 8 bits mode (as done for the QE SPI 
> controller via function mspi_apply_qe_mode_quirks() ), then it will work 
> but I will not be able to use the same speed as in 16 bits transfer mode.

Why would we tell the controller to run in 8 bit mode when you don't
need to?  What the driver tells the controller just needs to produce the
correct results given the request from the client driver, it doesn't
need to be what the client driver asked for.

> So, to sum up, the solutions I see:
> A/ Force CPM SPI controller to always use 8 bits mode.

SPI controller driver should do whatever is the most efficient thing
for the transfers it was asked to perform.

> B/ Implement a flag to allow a SPI consumer to say "please perform 
> tranfer with your bogus but performant 16 bits mode, I have worked 
> around the data order for you"

SPI_LSB_FIRST already exists, however unless a device really is
operating with words I'd really suggest not using this since it just
adds confusion and portability problems. 

> C/ Implement a byte-swapping in the CPM SPI controller when a consumer 
> asks for a 16-bits data transfer

If the device doesn't request SPI_LSB_FIRST.

> D/ Modify MAX7301 GPIO driver to use 8 bits words instead of 16 bits words.

That seems like it's needed anyway AFAICT, the driver is assuming a
particular endianness.  It could also be modified to format data in a
way that'll work with either endianness but that seems more confusing.

If your FPGA image is just a byte stream then I'd expect the driver for
it to be using 8 bit mode.  It is unclear to me if something has pre
swapped the image to contort it through whatever set of endianness
assumptions are going on here, you seem to want it to be byte swapped
but that seems like a surprising format for a FPGA image to be generated
in which makes me think something swapped the image earlier on?  AFAICT
what you're describing is a FPGA image which is a sequence of little
endian 16 bit words which need to be sent to the FPGA as a sequence of
big endian 16 bit words.

> Solution A will degrade performance by forcing transfer rate reduction.
> Solution B looks like a "home made" solution.
> Solution C means copy-and-swap into a newly allocated DMA buffer.
> Solution D is .... the best one ?

> Any other idea ?

> Which solution would you prefer ?

It seems like it'd be a lot clearer if the FPGA driver worked with a
byte stream, then the controller driver can work out the most efficient
way to actually perform that transfer.  At the minute I'm not clear what
it's actually doing here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Christophe Leroy]

[-- Attachment #1: Type: text/plain, Size: 7391 bytes --]



Le 07/03/2023 à 23:16, Mark Brown a écrit :
> On Tue, Mar 07, 2023 at 09:40:09PM +0000, Christophe Leroy wrote:
>> Le 07/03/2023 à 22:10, Mark Brown a écrit :
>>> On Tue, Mar 07, 2023 at 07:43:42PM +0000, Christophe Leroy wrote:
>>>> Le 07/03/2023 à 20:19, Mark Brown a écrit :
> 
>> So yes, the problem is that the controller *is* swapping data: if I
>> format the buffer to have 0x12 0x34 and tells the controller to send it
>> as a 16 bits word, it will send 0x3412 whereas if I tell it to send as 8
>> bits words, it will send 0x12 then 0x34.
> 
>> So a driver like MAX7301 which wants to send 0x1234 as a 16-bits word
>> will write 0x1234 into the buffer. The powerpc 8xx being big endian, I
>> get 0x12 0x34 into the buffer then the controller sends word 0x3412.
> 
> So the CPU is big endian, memory has a big endian word in it and the
> controller is then sending as little endian?  That would mean that the
> driver for the controller is implementing SPI_LSB_FIRST mode without
> being asked (IIRC there is an option to run these systems LE, I don't
> know if the controller is just always little endian or always byte
> swapping).  That seems like a bug in the driver for the controller.

What does SPI_LSB_FIRST stands for ? From what you say, shall I 
understand that it means Least Significant _Byte_ first ?

I'm asking because as far as I can see, for the controller SPI_LSB_FIRST 
means Least Significant _bit_ first. Is that wrong ?

> 
>>>> Well I not sure what you mean by swapping / not swapping data. Powerpc
>>>> 8xx is natively a big endian processor like all PPC32. But its
>>>> Communication Processor (CPM) is apparently fetching data as little
>>>> endian when told to perform transfer of 16 bits word on SPI.
> 
>>> The default wire format for SPI is big endian (MSB first), as covered in
>>> spi.h:
> 
>>>    * In-memory data values are always in native CPU byte order, translated
>>>    * from the wire byte order (big-endian except with SPI_LSB_FIRST).  So
>>>    * for example when bits_per_word is sixteen, buffers are 2N bytes long
>>>    * (@len = 2N) and hold N sixteen bit words in CPU byte order.
> 
>>> LSB_FIRST has only one in tree user other than spidev so I'd question
>>> how often it's correctly implemented.
> 
>> Well, ok, I have no problem with the wire byte order, only with how the
>> controller fetches the data from the DMA buffer. I'm not sure what I
>> write is clear, is it ?
> 
> You have a problem with the wire byte order not being what was
> requested - the data should always be big endian unless otherwise
> specified.

The attached screen capture is an extract of the controller reference 
manual. When SPI_LSB_FIRST is requested, REV bit is set to 0. Otherwise 
REV bit is set to 1.

Hope it helps clarify how the controller works.

> 
>>>> So, my problem really is the GPIO MAX7301 driver which requests 16 bits
>>>> transfers, because then the SPI controller sends the 2 bytes in reversed
>>>> order. Do I understand correctly that from your point of view, that
>>>> driver shouldn't request a 16 bits tranfer ? It is done here, in the
>>>> max7301_probe() function,
>>>> https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/gpio/gpio-max7301.c#L50
> 
>>> It would certainly improve interoperability with controllers to request
>>> 8 bit, but so long as the driver is reading and writing data in the
>>> expected format it should work perfectly well.  Looking at the lack of
>>> any endianness handling in the driver that doesn't seem to be the case
>>> though, it's just handling data in CPU endian format which isn't
>>> portable.
> 
>> I agree, it should work but it doesn't.
>> When I change max7301_probe() to set bits_per_word to 8 instead of 16 it
>> works.
> 
> So the driver should also work on a little endian system configured for
> 16 bit words - looking at the code I would only expect it to work on
> one endianness of system.
> 
>>>> Because if I clamp the CPM SPI driver to 8 bits transfers, then I cannot
>>>> anymore perform 16 bits transfer for loading my FPGA, then it means I
>>>> must reduce data rate then loading the FPGA takes ages.
> 
>>> Why?
> 
>> Maybe what I wrote here isn't clear. What I mean is that if I modify CPM
>> SPI controller driver to always use 8 bits mode (as done for the QE SPI
>> controller via function mspi_apply_qe_mode_quirks() ), then it will work
>> but I will not be able to use the same speed as in 16 bits transfer mode.
> 
> Why would we tell the controller to run in 8 bit mode when you don't
> need to?  What the driver tells the controller just needs to produce the
> correct results given the request from the client driver, it doesn't
> need to be what the client driver asked for.
> 
>> So, to sum up, the solutions I see:
>> A/ Force CPM SPI controller to always use 8 bits mode.
> 
> SPI controller driver should do whatever is the most efficient thing
> for the transfers it was asked to perform.
> 
>> B/ Implement a flag to allow a SPI consumer to say "please perform
>> tranfer with your bogus but performant 16 bits mode, I have worked
>> around the data order for you"
> 
> SPI_LSB_FIRST already exists, however unless a device really is
> operating with words I'd really suggest not using this since it just
> adds confusion and portability problems.
> 
>> C/ Implement a byte-swapping in the CPM SPI controller when a consumer
>> asks for a 16-bits data transfer
> 
> If the device doesn't request SPI_LSB_FIRST.
> 
>> D/ Modify MAX7301 GPIO driver to use 8 bits words instead of 16 bits words.
> 
> That seems like it's needed anyway AFAICT, the driver is assuming a
> particular endianness.  It could also be modified to format data in a
> way that'll work with either endianness but that seems more confusing.
> 
> If your FPGA image is just a byte stream then I'd expect the driver for
> it to be using 8 bit mode.  It is unclear to me if something has pre
> swapped the image to contort it through whatever set of endianness
> assumptions are going on here, you seem to want it to be byte swapped
> but that seems like a surprising format for a FPGA image to be generated
> in which makes me think something swapped the image earlier on?  AFAICT
> what you're describing is a FPGA image which is a sequence of little
> endian 16 bit words which need to be sent to the FPGA as a sequence of
> big endian 16 bit words.

My FPGA image is a byte stream, you are right it could/should use 8 bit 
more. Using 16 bit mode was a trick to be able to send faster as the 
controler is more efficient in 16 bits mode.

> 
>> Solution A will degrade performance by forcing transfer rate reduction.
>> Solution B looks like a "home made" solution.
>> Solution C means copy-and-swap into a newly allocated DMA buffer.
>> Solution D is .... the best one ?
> 
>> Any other idea ?
> 
>> Which solution would you prefer ?
> 
> It seems like it'd be a lot clearer if the FPGA driver worked with a
> byte stream, then the controller driver can work out the most efficient
> way to actually perform that transfer.  At the minute I'm not clear what
> it's actually doing here.

Yes I agree, let's see if I can implement byte swapping in the SPI 
controller driver.

Thanks for your help,
Christophe

[-- Attachment #2: spi_fsl_examples.png --]
[-- Type: image/png, Size: 138646 bytes --]

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Mon, Mar 13, 2023 at 09:55:25AM +0000, Christophe Leroy wrote:
> Le 07/03/2023 à 23:16, Mark Brown a écrit :

> > So the CPU is big endian, memory has a big endian word in it and the
> > controller is then sending as little endian?  That would mean that the
> > driver for the controller is implementing SPI_LSB_FIRST mode without
> > being asked (IIRC there is an option to run these systems LE, I don't
> > know if the controller is just always little endian or always byte
> > swapping).  That seems like a bug in the driver for the controller.

> What does SPI_LSB_FIRST stands for ? From what you say, shall I 
> understand that it means Least Significant _Byte_ first ?

> I'm asking because as far as I can see, for the controller SPI_LSB_FIRST 
> means Least Significant _bit_ first. Is that wrong ?

Ah, you're right - it's bit rather than byte.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Christophe Leroy]

Hi Mark,

Le 07/03/2023 à 23:16, Mark Brown a écrit :
> On Tue, Mar 07, 2023 at 09:40:09PM +0000, Christophe Leroy wrote:
>>>    * In-memory data values are always in native CPU byte order, translated
>>>    * from the wire byte order (big-endian except with SPI_LSB_FIRST).  So
>>>    * for example when bits_per_word is sixteen, buffers are 2N bytes long
>>>    * (@len = 2N) and hold N sixteen bit words in CPU byte order.
> 
>>> LSB_FIRST has only one in tree user other than spidev so I'd question
>>> how often it's correctly implemented.
> 
>> Well, ok, I have no problem with the wire byte order, only with how the
>> controller fetches the data from the DMA buffer. I'm not sure what I
>> write is clear, is it ?
> 
> You have a problem with the wire byte order not being what was
> requested - the data should always be big endian unless otherwise
> specified.
> 

Looking at another driver I'm even more puzzled. That's driver 
drivers/leds/leds-dac124s085.c

It also sets spi->bits_per_word = 16 , but is uses cpu_to_le16() to 
prepare that data. So this one should then work with any host 
endianness, but why the hell is it doing cpu_to_le16() if the data 
should be big endian ?

To be honnest, I'm really not sure anymore what is the way to go.

Christophe

Re: Looking for a solution for CPM FSL SPI driver that swaps 16 bits words

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On Mon, Mar 13, 2023 at 10:33:10PM +0000, Christophe Leroy wrote:

> Looking at another driver I'm even more puzzled. That's driver 
> drivers/leds/leds-dac124s085.c

> It also sets spi->bits_per_word = 16 , but is uses cpu_to_le16() to 
> prepare that data. So this one should then work with any host 
> endianness, but why the hell is it doing cpu_to_le16() if the data 
> should be big endian ?

I wouldn't take some random client driver as being particulary
meaningful - people often abuse APIs.  I wouldn't be sure that that
endianness conversion was doing anything on the system that the driver
was developed on.

> To be honnest, I'm really not sure anymore what is the way to go.

The expected behaviour is very clearly documented, I've quoted that
documentation at you and recommended that anything being done for
performance reasons be done in the controller driver.  I'm really not
sure what else to tell you, especially given that if the client driver
uses a byte stream (which is what it wants) this all becomes purely
internal to the controller
driver.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]