spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[linux]

After upgrade from kernel 6.5.11 to 6.6.1 the spi-devices on my hw 
colibri-imx6dl and verdin-imx8mm are not working anymore (TPM2 and SPI-SRAM).

Analyzing the problem showed that the 2 commits introduced the problem:

spi: Increase imx51 ecspi burst length based on transfer length
15a6af94a2779d5dfb42ee4bfac858ea8e964a3f

spi: imx: Take in account bits per word instead of assuming 8-bits
5f66db08cbd3ca471c66bacb0282902c79db9274

Reverting the commits solved the problem.

The analyse with the logic-analyser showed a wrong number of transmitted  
bytes and wrong data.
When I try to send 127 Byte with a incrementing pattern (0x01,0x02,0x03,..),
504 Bytes are sent (0x00,0x00,0x01 0x00,0x00,0x00,0x02, 0x00,0x00,0x00,0x03).
We tested with different sizes and patterns, all are not ok.
While analysing the configuration and code I was not able to see any obvious
mistake.

Has someone else discovered such misbehaviour or has any idea what is wrong?
Best Regards
Stefan

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Linux regression tracking (Thorsten Leemhuis)]

[CCing people that worked on the relevant commits as well as the
regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

On 18.11.23 15:25, linux@bigler.io wrote:
> After upgrade from kernel 6.5.11 to 6.6.1 the spi-devices on my hw 
> colibri-imx6dl and verdin-imx8mm are not working anymore (TPM2 and SPI-SRAM).
> 
> Analyzing the problem showed that the 2 commits introduced the problem:
> 
> spi: Increase imx51 ecspi burst length based on transfer length
> 15a6af94a2779d5dfb42ee4bfac858ea8e964a3f
> 
> spi: imx: Take in account bits per word instead of assuming 8-bits
> 5f66db08cbd3ca471c66bacb0282902c79db9274
> 
> Reverting the commits solved the problem.
> 
> The analyse with the logic-analyser showed a wrong number of transmitted  
> bytes and wrong data.
> When I try to send 127 Byte with a incrementing pattern (0x01,0x02,0x03,..),
> 504 Bytes are sent (0x00,0x00,0x01 0x00,0x00,0x00,0x02, 0x00,0x00,0x00,0x03).
> We tested with different sizes and patterns, all are not ok.
> While analysing the configuration and code I was not able to see any obvious
> mistake.
> 
> Has someone else discovered such misbehaviour or has any idea what is wrong?
> Best Regards

Thx for the report!

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

To ensure the issue doesn't fall through the cracks unnoticed, I'm
adding it to regzbot, the Linux kernel regression tracking bot:

#regzbot ^introduced 15a6af94a27
#regzbot title spi: spi-devices on colibri-imx6dl and verdin-imx8mm are
not working anymore
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Thorsten Leemhuis]

On 18.11.23 16:13, Linux regression tracking (Thorsten Leemhuis) wrote:
> [CCing people that worked on the relevant commits as well as the
> regression list, as it should be in the loop for regressions:
> https://docs.kernel.org/admin-guide/reporting-regressions.html]

Sorry for the spam, hit "send" before actually adding Stefan and Mark,
which now are among the recipients.

> On 18.11.23 15:25, linux@bigler.io wrote:
>> After upgrade from kernel 6.5.11 to 6.6.1 the spi-devices on my hw 
>> colibri-imx6dl and verdin-imx8mm are not working anymore (TPM2 and SPI-SRAM).
>>
>> Analyzing the problem showed that the 2 commits introduced the problem:
>>
>> spi: Increase imx51 ecspi burst length based on transfer length
>> 15a6af94a2779d5dfb42ee4bfac858ea8e964a3f
>>
>> spi: imx: Take in account bits per word instead of assuming 8-bits
>> 5f66db08cbd3ca471c66bacb0282902c79db9274
>>
>> Reverting the commits solved the problem.
>>
>> The analyse with the logic-analyser showed a wrong number of transmitted  
>> bytes and wrong data.
>> When I try to send 127 Byte with a incrementing pattern (0x01,0x02,0x03,..),
>> 504 Bytes are sent (0x00,0x00,0x01 0x00,0x00,0x00,0x02, 0x00,0x00,0x00,0x03).
>> We tested with different sizes and patterns, all are not ok.
>> While analysing the configuration and code I was not able to see any obvious
>> mistake.
>>
>> Has someone else discovered such misbehaviour or has any idea what is wrong?
>> Best Regards
> 
> Thx for the report!
> 
> [TLDR: I'm adding this report to the list of tracked Linux kernel
> regressions; the text you find below is based on a few templates
> paragraphs you might have encountered already in similar form.
> See link in footer if these mails annoy you.]
> 
> To ensure the issue doesn't fall through the cracks unnoticed, I'm
> adding it to regzbot, the Linux kernel regression tracking bot:
> 
> #regzbot ^introduced 15a6af94a27
> #regzbot title spi: spi-devices on colibri-imx6dl and verdin-imx8mm are
> not working anymore
> #regzbot ignore-activity
> 
> This isn't a regression? This issue or a fix for it are already
> discussed somewhere else? It was fixed already? You want to clarify when
> the regression started to happen? Or point out I got the title or
> something else totally wrong? Then just reply and tell me -- ideally
> while also telling regzbot about it, as explained by the page listed in
> the footer of this mail.
> 
> Developers: When fixing the issue, remember to add 'Link:' tags pointing
> to the report (the parent of this mail). See page linked in footer for
> details.
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> That page also explains what to do if mails like this annoy you.

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[linux]

Hi 

I checked the code an printed the used parameters, the ‘bits_per_word’ is set to 8.

Best Regards
Stefan Bigler

Am 2023-11-18T17:54:26.000+01:00 hat Stefan Moring <stefan.moring@technolution.nl> geschrieben:
>  Hi Thorsten,
> 
> Looking at the data of your test, it appears as if your ‘bits_per_word’ are
> set to 32, where you would want 8.
> 
> Can you verify this?
> 
> Kind regards,
> 
> Stefan Moring
> 
> Op za 18 nov. 2023 om 16:15 schreef Thorsten Leemhuis <
> regressions@leemhuis.info>
> 
> 
> >    On 18.11.23 16:13, Linux regression tracking (Thorsten Leemhuis) wrote:
> > 
> > >     [CCing people that worked on the relevant commits as well as the
> > >   regression list, as it should be in the loop for regressions:
> > >   https://docs.kernel.org/admin-guide/reporting-regressions.html]
> >  
> >  Sorry for the spam, hit "send" before actually adding Stefan and Mark,
> >  which now are among the recipients.
> > 
> > 
> > >     On 18.11.23 15:25, linux@bigler.io wrote:
> > > 
> > > >     After upgrade from kernel 6.5.11 to 6.6.1 the spi-devices on my hw
> > > >   colibri-imx6dl and verdin-imx8mm are not working anymore (TPM2 and
> > >   SPI-SRAM).
> > > 
> > > >  
> > > > >    
> > > > >   Analyzing the problem showed that the 2 commits introduced the problem:
> > > > >  
> > > > >   spi: Increase imx51 ecspi burst length based on transfer length
> > > > >   15a6af94a2779d5dfb42ee4bfac858ea8e964a3f
> > > > >  
> > > > >   spi: imx: Take in account bits per word instead of assuming 8-bits
> > > > >   5f66db08cbd3ca471c66bacb0282902c79db9274
> > > > >  
> > > > >   Reverting the commits solved the problem.
> > > > >  
> > > > >   The analyse with the logic-analyser showed a wrong number of
> > > >   transmitted
> > > > 
> > > > >  
> > > > > >     bytes and wrong data.
> > > > > >   When I try to send 127 Byte with a incrementing pattern
> > > > >   (0x01,0x02,0x03,..),
> > > > > 
> > > > > >  
> > > > > > >     504 Bytes are sent (0x00,0x00,0x01 0x00,0x00,0x00,0x02,
> > > > > >   0x00,0x00,0x00,0x03).
> > > > > > 
> > > > > > >  
> > > > > > > >     We tested with different sizes and patterns, all are not ok.
> > > > > > > >   While analysing the configuration and code I was not able to see any
> > > > > > >   obvious
> > > > > > > 
> > > > > > > >  
> > > > > > > > >     mistake.
> > > > > > > > >  
> > > > > > > > >   Has someone else discovered such misbehaviour or has any idea what is
> > > > > > > >   wrong?
> > > > > > > > 
> > > > > > > > >  
> > > > > > > > > >     Best Regards
> > > > > > > > >   
> > > > > > > > >   Thx for the report!
> > > > > > > > >  
> > > > > > > > >   [TLDR: I'm adding this report to the list of tracked Linux kernel
> > > > > > > > >   regressions; the text you find below is based on a few templates
> > > > > > > > >   paragraphs you might have encountered already in similar form.
> > > > > > > > >   See link in footer if these mails annoy you.]
> > > > > > > > >  
> > > > > > > > >   To ensure the issue doesn't fall through the cracks unnoticed, I'm
> > > > > > > > >   adding it to regzbot, the Linux kernel regression tracking bot:
> > > > > > > > >  
> > > > > > > > >   #regzbot ^introduced 15a6af94a27
> > > > > > > > >   #regzbot title spi: spi-devices on colibri-imx6dl and verdin-imx8mm are
> > > > > > > > >   not working anymore
> > > > > > > > >   #regzbot ignore-activity
> > > > > > > > >  
> > > > > > > > >   This isn't a regression? This issue or a fix for it are already
> > > > > > > > >   discussed somewhere else? It was fixed already? You want to clarify when
> > > > > > > > >   the regression started to happen? Or point out I got the title or
> > > > > > > > >   something else totally wrong? Then just reply and tell me -- ideally
> > > > > > > > >   while also telling regzbot about it, as explained by the page listed in
> > > > > > > > >   the footer of this mail.
> > > > > > > > >  
> > > > > > > > >   Developers: When fixing the issue, remember to add 'Link:' tags pointing
> > > > > > > > >   to the report (the parent of this mail). See page linked in footer for
> > > > > > > > >   details.
> > > > > > > > >  
> > > > > > > > >   Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> > > > > > > > >   --
> > > > > > > > >   Everything you wanna know about Linux kernel regression tracking:
> > > > > > > > >   https://linux-regtracking.leemhuis.info/about/#tldr
> > > > > > > > >   That page also explains what to do if mails like this annoy you.
> > > > > > > >  
> > > > > > > > 
> > > > > > > >

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[linux]

Hi Stefan

Thanks for analyzing the problem and sorry for the delay, I had to simplify the code to a minimum so that I can send to you.
Now I was able to use spidev.
I have an environment with yocto kirstone 4.0.10.
Load the spi-dma (imx-sdma 302c0000.dma-controller: loaded firmware 4.5), run the spi_imx and the spidev as kenelmodule.

I run the code on a Toradex Verdin Development Board and use the imx8mm-verdin-nonwifi-dev.dts

To add the spidev I patched imx8mm-verdin.dtsi
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
index 6f0811587142..262500940adc 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
@@ -209,6 +209,15 @@ &ecspi2 {
        cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_ecspi2>;
+
+       spidev@0{
+               compatible = "micron,spi-authenta";
+               reg = <0>;
+               #address-cells = <1>;
+               #size-cells = <0>;
+               spi-max-frequency = <20000000>;
+               status = "okay";
+       };
 };

as a spidev test program I used 
https://raw.githubusercontent.com/raspberrypi/linux/rpi-3.10.y/Documentation/spi/spidev_test.c

I changed the transmitted data
diff --git a/recipes-spi/spidev/files/spidev_test.c b/recipes-spi/spidev/files/spidev_test.c
index 16feda9..6056ffd 100644
--- a/recipes-spi/spidev/files/spidev_test.c
+++ b/recipes-spi/spidev/files/spidev_test.c
@@ -39,13 +39,22 @@ static void transfer(int fd)
 {
        int ret;
        uint8_t tx[] = {
-               0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
-               0x40, 0x00, 0x00, 0x00, 0x00, 0x95,
-               0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
-               0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
-               0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
-               0xDE, 0xAD, 0xBE, 0xEF, 0xBA, 0xAD,
-               0xF0, 0x0D,
+               0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
+        0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, // 16
+        0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18,
+        0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, // 32
+        0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28,
+        0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x20, // 48
+        0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38,
+        0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f, 0x40, // 64
+        0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
+        0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, // 80
+        0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18,
+        0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, // 96
+        0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28,
+        0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x20, // 112
+        0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38,
+        0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f,       // 127
        };
        uint8_t rx[ARRAY_SIZE(tx)] = {0, };

sending the content showed again the bad data
spidev_test --device /dev/spidev1.0 --speed 20000000 --bpw 8

0x00,0x00,0x01,   
0x00,0x00,0x00,0x02,
0x00,0x00,0x00,0x03,
0x00,0x00,0x00,0x04,

I you need more information let me know.

Best Regards
Stefan Bigler

Am 2023-11-19T08:52:54.000+01:00 hat Stefan Moring <stefan.moring@technolution.nl> geschrieben:
>  Hi Stefan,
> 
> Can you maybe share me your test code? I can try to reproduce it tomorrow.
> 
> Kind regards,
> 
> Stefan Moring

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Francesco Dolcini]

On Mon, Nov 20, 2023 at 09:27:10AM +0100, linux@bigler.io wrote:
> Load the spi-dma (imx-sdma 302c0000.dma-controller: loaded firmware
> 4.5), run the spi_imx and the spidev as kenelmodule.
> 
> I run the code on a Toradex Verdin Development Board and use the
> imx8mm-verdin-nonwifi-dev.dts
> 
> To add the spidev I patched imx8mm-verdin.dtsi
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> index 6f0811587142..262500940adc 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> @@ -209,6 +209,15 @@ &ecspi2 {
>         cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_ecspi2>;
> +
> +       spidev@0{
> +               compatible = "micron,spi-authenta";
> +               reg = <0>;
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               spi-max-frequency = <20000000>;
> +               status = "okay";
> +       };
>  };
> 
> as a spidev test program I used 
> https://raw.githubusercontent.com/raspberrypi/linux/rpi-3.10.y/Documentation/spi/spidev_test.c

Would you mind doing the following tests looping the SPI interface MISO/MOSI?

```
cd /tmp

dd if=/dev/urandom of=4k-spi-test-data.bin bs=1 count=4k
spidev_test -D "$device" -s 4000000 -i 4k-spi-test-data.bin -o 4k-spi-test-result.bin

dd if=/dev/urandom of=16bytes-spi-test-data.bin bs=1 count=16
spidev_test -D "$device" -s 16000 -i 16bytes-spi-test-data.bin -o 16bytes-spi-test-result.bin
```

with "$device" being your actual spidev device?

Those tests are passing on 6.7.0-rc2 for me.

Francesco

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[linux]

Hi Francesco

I run the test and it worked fine. The transmitted data matches with the recived data, no diff.

BUT!!! on the MDIO it is completly different !!!
When I request to send 4kB Data I measure on the line 16384Byte.

The data is again   3Dummy Bytes followed by the databyte.
So the error symetic on send and recive

Regards Stefan

Am 2023-11-20T09:47:48.000+01:00 hat Francesco Dolcini <francesco@dolcini.it> geschrieben:
>  On Mon, Nov 20, 2023 at 09:27:10AM +0100, linux@bigler.io wrote:
> 
> >    Load the spi-dma (imx-sdma 302c0000.dma-controller: loaded firmware
> >  4.5), run the spi_imx and the spidev as kenelmodule.
> >  
> >  I run the code on a Toradex Verdin Development Board and use the
> >  imx8mm-verdin-nonwifi-dev.dts
> >  
> >  To add the spidev I patched imx8mm-verdin.dtsi
> >  diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> >  index 6f0811587142..262500940adc 100644
> >  --- a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> >  +++ b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> >  @@ -209,6 +209,15 @@ &ecspi2 {
> >          cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
> >          pinctrl-names = "default";
> >          pinctrl-0 = <&pinctrl_ecspi2>;
> >  +
> >  +       spidev@0{
> >  +               compatible = "micron,spi-authenta";
> >  +               reg = <0>;
> >  +               #address-cells = <1>;
> >  +               #size-cells = <0>;
> >  +               spi-max-frequency = <20000000>;
> >  +               status = "okay";
> >  +       };
> >   };
> >  
> >  as a spidev test program I used 
> >  https://raw.githubusercontent.com/raspberrypi/linux/rpi-3.10.y/Documentation/spi/spidev_test.c
>  
> Would you mind doing the following tests looping the SPI interface MISO/MOSI?
> 
> ```
> cd /tmp
> 
> dd if=/dev/urandom of=4k-spi-test-data.bin bs=1 count=4k
> spidev_test -D "$device" -s 4000000 -i 4k-spi-test-data.bin -o 4k-spi-test-result.bin
> 
> dd if=/dev/urandom of=16bytes-spi-test-data.bin bs=1 count=16
> spidev_test -D "$device" -s 16000 -i 16bytes-spi-test-data.bin -o 16bytes-spi-test-result.bin
> ```
> 
> with "$device" being your actual spidev device?
> 
> Those tests are passing on 6.7.0-rc2 for me.
> 
Francesco

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Francesco Dolcini]

On Mon, Nov 20, 2023 at 10:33:38AM +0100, linux@bigler.io wrote:
> I run the test and it worked fine. The transmitted data matches with
> the recived data, no diff.
ok, thanks for confirming. At least we are on the same page.

> BUT!!! on the MDIO it is completly different !!!
> When I request to send 4kB Data I measure on the line 16384Byte.
> 
> The data is again   3Dummy Bytes followed by the databyte.
> So the error symetic on send and recive

ok, that's annoying. I wonder if the loopback test could be improved to
spot such kind of issues, maybe we could verify if the execution time
is reasonable given the SPI clock frequency and the payload size ...

Francesco

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[linux]

Hi Stefan

I added my printing 
@@ -655,12 +658,25 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
 
        /* Clear BL field and set the right value */
        ctrl &= ~MX51_ECSPI_CTRL_BL_MASK;
+       dev_info(spi_imx->dev, "%s: count=%d, bits_per_word=%d\n",
+            __FUNCTION__, spi_imx->count, spi_imx->bits_per_word);
+
        if (spi_imx->target_mode && is_imx53_ecspi(spi_imx))
                ctrl |= (spi_imx->target_burst * 8 - 1)
                        << MX51_ECSPI_CTRL_BL_OFFSET;

and run the test with the burst enabled 
/var/volatile/tmp# spidev_test --device /dev/spidev1.0 --speed 20000000 --bpw 8 -i 127bytes-spi-test-data.bin -o 127bytes-spi-test-result.bin
[  802.729824] spi_imx 30830000.spi: mx51_ecspi_prepare_transfer: count=127, bits_per_word=8
-> transmitted 504 Bytes on the line

then I run it with burst disabled
spidev_test --device /dev/spidev1.0 --speed 20000000 --bpw 8 -i 127bytes-spi-test-data.bin -o 127bytes-spi-test-result.bin
[  316.683807] spi_imx 30830000.spi: mx51_ecspi_prepare_transfer: count=0, bits_per_word=8
-> transmitted 127 Bytes on the line

The count is different 
- for burst-enabled  127
- for burst-disable 0

concerning the sdma-firmware I checked on 
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/imx/sdma
but the latest I found was 4.5

Regards
Stefan


Am 2023-11-20T16:30:15.000+01:00 hat Stefan Moring <stefan.moring@technolution.nl> geschrieben:
>  Hi Stefan,
> 
> I have been trying to reproduce your error on my iMX8M, scoping the SPI
> lines, but everything works as expected. I don't have the exact same board,
> but I don't think that should be a problem. One difference I noticed is
> that my imx-sdma firmware is version 4.6.
> 
> Can you verify the values used for the transfer,  spi_imx->count and
> spi_imx->bits_per_word inside the mx51_ecpsi_prepare_transfer() method?
> Those are the only two things that changed in the commits. Maybe compare
> them to the working version?
> 
> Kind regards,
> 
> Stefan Moring
> 
> Op ma 20 nov 2023 om 11:18 schreef Francesco Dolcini <francesco@dolcini.it>:
> 
> 
> >    On Mon, Nov 20, 2023 at 10:33:38AM +0100, linux@bigler.io wrote:
> > 
> > >     I run the test and it worked fine. The transmitted data matches with
> > >   the recived data, no diff.
> >   ok, thanks for confirming. At least we are on the same page.
> > 
> > 
> > >     BUT!!! on the MDIO it is completly different !!!
> > >   When I request to send 4kB Data I measure on the line 16384Byte.
> > >  
> > >   The data is again   3Dummy Bytes followed by the databyte.
> > >   So the error symetic on send and recive
> >  
> >  ok, that's annoying. I wonder if the loopback test could be improved to
> >  spot such kind of issues, maybe we could verify if the execution time
> >  is reasonable given the SPI clock frequency and the payload size ...
> > 
> >  Francesco
> > 
> 
> 
>

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Francesco Dolcini]

On Mon, Nov 20, 2023 at 04:30:15PM +0100, Stefan Moring wrote:
> Can you verify the values used for the transfer,  spi_imx->count and spi_imx->
> bits_per_word inside the mx51_ecpsi_prepare_transfer() method? Those are the
> only two things that changed in the commits. Maybe compare them to the working
> version?

I would suggest to bisect the issue to the actual commit that
introduced the regression, I do not think this was done yet.

Francesco

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Thorsten Leemhuis]

On 20.11.23 18:48, Francesco Dolcini wrote:
> On Mon, Nov 20, 2023 at 04:30:15PM +0100, Stefan Moring wrote:
>> Can you verify the values used for the transfer,  spi_imx->count and spi_imx->
>> bits_per_word inside the mx51_ecpsi_prepare_transfer() method? Those are the
>> only two things that changed in the commits. Maybe compare them to the working
>> version?
> 
> I would suggest to bisect the issue to the actual commit that
> introduced the regression, I do not think this was done yet.

I think it was. To quote

https://lore.kernel.org/all/8a415902c751cdbb4b20ce76569216ed@mail.infomaniak.com/

"'"
After upgrade from kernel 6.5.11 to 6.6.1 the spi-devices on my hw
colibri-imx6dl and verdin-imx8mm are not working anymore (TPM2 and
SPI-SRAM).

Analyzing the problem showed that the 2 commits introduced the problem:

spi: Increase imx51 ecspi burst length based on transfer length
15a6af94a2779d5dfb42ee4bfac858ea8e964a3f

spi: imx: Take in account bits per word instead of assuming 8-bits
5f66db08cbd3ca471c66bacb0282902c79db9274

Reverting the commits solved the problem.
"'"

Or am I missing something here?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Francesco Dolcini]

On Tue, Nov 21, 2023 at 10:06:51AM +0100, Thorsten Leemhuis wrote:
> On 20.11.23 18:48, Francesco Dolcini wrote:
> > On Mon, Nov 20, 2023 at 04:30:15PM +0100, Stefan Moring wrote:
> >> Can you verify the values used for the transfer,  spi_imx->count and spi_imx->
> >> bits_per_word inside the mx51_ecpsi_prepare_transfer() method? Those are the
> >> only two things that changed in the commits. Maybe compare them to the working
> >> version?
> > 
> > I would suggest to bisect the issue to the actual commit that
> > introduced the regression, I do not think this was done yet.
> 
> I think it was. To quote
Whoops, you are right.

> spi: Increase imx51 ecspi burst length based on transfer length
> 15a6af94a2779d5dfb42ee4bfac858ea8e964a3f
> 
> spi: imx: Take in account bits per word instead of assuming 8-bits
> 5f66db08cbd3ca471c66bacb0282902c79db9274

Do we know which one of those two commits introduces this regression?

Francesco

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Francesco Dolcini]

On Tue, Nov 21, 2023 at 10:16:57AM +0100, Francesco Dolcini wrote:
> On Tue, Nov 21, 2023 at 10:06:51AM +0100, Thorsten Leemhuis wrote:
> > spi: Increase imx51 ecspi burst length based on transfer length
> > 15a6af94a2779d5dfb42ee4bfac858ea8e964a3f
> > 
> > spi: imx: Take in account bits per word instead of assuming 8-bits
> > 5f66db08cbd3ca471c66bacb0282902c79db9274
> 
> Do we know which one of those two commits introduces this regression?

Whoops again. The second one is a fix for the first one, so my question
does not make any sense, forget about this, sorry.

Francesco

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[linux]

Hi

At least in my test-case the  commit is NOT introducing this regression, because the bits_per_word is 8, so the result is the same
spi: imx: Take in account bits per word instead of assuming 8-bits
5f66db08cbd3ca471c66bacb0282902c79db9274

I do not have the latest  mx-sdma firmware can you tell me where I get it. On 
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/imx/sdma
the latest I found was 4.5

I tried to debug the code but it's hard vor me to understand where the problem could be.
I saw then I disable the dma with set

imx51_ecspi_devtype_data {
.has_dmamode = false,
}
the SPI is working fine.

Should I do some more tests, do some loggings.. 
Please let me know

Regards 
Stefan Bigler

Am 2023-11-21T10:16:57.000+01:00 hat Francesco Dolcini <francesco@dolcini.it> geschrieben:
>  On Tue, Nov 21, 2023 at 10:06:51AM +0100, Thorsten Leemhuis wrote:
> 
> >    On 20.11.23 18:48, Francesco Dolcini wrote:
> > 
> > >     On Mon, Nov 20, 2023 at 04:30:15PM +0100, Stefan Moring wrote:
> > > 
> > > >     Can you verify the values used for the transfer,  spi_imx->count and spi_imx->
> > > >   bits_per_word inside the mx51_ecpsi_prepare_transfer() method? Those are the
> > > >   only two things that changed in the commits. Maybe compare them to the working
> > > >   version?
> > >    
> > >   I would suggest to bisect the issue to the actual commit that
> > >   introduced the regression, I do not think this was done yet.
> >   
> >  I think it was. To quote
>  Whoops, you are right.
> 
> 
> >    spi: Increase imx51 ecspi burst length based on transfer length
> >  15a6af94a2779d5dfb42ee4bfac858ea8e964a3f
> >  
> >  spi: imx: Take in account bits per word instead of assuming 8-bits
> >  5f66db08cbd3ca471c66bacb0282902c79db9274
>  
> Do we know which one of those two commits introduces this regression?
> 
Francesco

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[linux]

Hi 

i did now  my test with the latest imx-sdma  firmware 4.6 from NXP
https://www.nxp.com/lgfiles/NMG/MAD/YOCTO/firmware-imx-8.18.bin
There is no improvement.
What is the HW that you use? What type of imx8?

Regards Stefan Bigler

Am 2023-11-21T11:34:26.000+01:00 hat  <linux@bigler.io> geschrieben:
>  Hi
> 
> At least in my test-case the  commit is NOT introducing this regression, because the bits_per_word is 8, so the result is the same
> spi: imx: Take in account bits per word instead of assuming 8-bits
> 5f66db08cbd3ca471c66bacb0282902c79db9274
> 
> I do not have the latest  mx-sdma firmware can you tell me where I get it. On 
> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/imx/sdma
> the latest I found was 4.5
> 
> I tried to debug the code but it's hard vor me to understand where the problem could be.
> I saw then I disable the dma with set
> 
> imx51_ecspi_devtype_data {
> .has_dmamode = false,
> }
> the SPI is working fine.
> 
> Should I do some more tests, do some loggings.. 
> Please let me know
> 
> Regards 
> Stefan Bigler

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Benjamin Bigler]

Hello,

I'm uncertain whether this is the appropriate place to respond, but I
am encountering a similar issue on a phyboard-polis-rdk (imx8mm) with
Linux 6.7-rc2. My system uses the imx-sdma firmware version 4.6.

The problem is with the TPM, which fails to function correctly,
displaying the error message:
"tpm tpm0: TPM in field failure mode, requires firmware upgrade."

The issue is identical to the one reported by Stefan. It can be
resolved by reverting commits 15a6af94a2779d5dfb42ee4bfac858ea8e964a3f
and 5f66db08cbd3ca471c66bacb0282902c79db9274, after which the TPM works
again.

Additionally, I've conducted the tests which Francesco mentioned:

> ```
> cd /tmp
> 
> dd if=/dev/urandom of=4k-spi-test-data.bin bs=1 count=4k
> spidev_test -D "$device" -s 4000000 -i 4k-spi-test-data.bin -o 4k-
spi-test-result.bin
> 
> dd if=/dev/urandom of=16bytes-spi-test-data.bin bs=1 count=16
> spidev_test -D "$device" -s 16000 -i 16bytes-spi-test-data.bin -o
16bytes-spi-test-result.bin
> ```

The readback data is correct but on MOSI/MISO there are 16384 bytes.

Please let me know if I can do some more tests.

Best regards,

Benjamin Bigler

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Benjamin Bigler]

Hi

I did some debugging and I think the problem is that in this case bits_per_word is 8. So in
spi_imx_dma_configure the buswidth is set to DMA_SLAVE_BUSWIDTH_1_BYTE. But in
mx51_ecspi_prepare_transfer the BURST_LENGTH is now set to (spi_imx->count * spi_imx->bits_per_word
- 1)
before 15a6af94a2779d5dfb42ee4bfac858ea8e964a3f it was (spi_imx->bits_per_word - 1). Now the spi
transmits 4 byte per byte except for the first word. I added the following patch and it worked again

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 498e35c8db2c..f514966e2ada 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -659,11 +659,22 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
 		ctrl |= (spi_imx->target_burst * 8 - 1)
 			<< MX51_ECSPI_CTRL_BL_OFFSET;
 	else {
-		if (spi_imx->count >= 512)
-			ctrl |= 0xFFF << MX51_ECSPI_CTRL_BL_OFFSET;
-		else
-			ctrl |= (spi_imx->count * spi_imx->bits_per_word - 1)
+		if (spi_imx->usedma)
+			ctrl |= (spi_imx->bits_per_word *
+					 spi_imx_bytes_per_word(
+						 spi_imx->bits_per_word) -
+				 1)
 				<< MX51_ECSPI_CTRL_BL_OFFSET;
+		else {
+			if (spi_imx->count >= MX51_ECSPI_CTRL_MAX_BURST)
+				ctrl |= (MX51_ECSPI_CTRL_MAX_BURST - 1)
+					<< MX51_ECSPI_CTRL_BL_OFFSET;
+			else
+				ctrl |= (spi_imx->count *
+						 spi_imx->bits_per_word -
+					 1)
+					<< MX51_ECSPI_CTRL_BL_OFFSET;
+		}
 	}
 
 	/* set clock speed */

Best regards,
Benjamin Bigler

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[linux]

Hi 

Thank you Benjamin for the patch.
I have tested the patch and is works fine.

Best regards
Stefan Bigler

Am 2023-11-26T14:19:56.000+01:00 hat Benjamin Bigler <benjamin@bigler.one> geschrieben:
>  Hi
> 
> I did some debugging and I think the problem is that in this case bits_per_word is 8. So in
> spi_imx_dma_configure the buswidth is set to DMA_SLAVE_BUSWIDTH_1_BYTE. But in
> mx51_ecspi_prepare_transfer the BURST_LENGTH is now set to (spi_imx->count * spi_imx->bits_per_word
> - 1)
> before 15a6af94a2779d5dfb42ee4bfac858ea8e964a3f it was (spi_imx->bits_per_word - 1). Now the spi
> transmits 4 byte per byte except for the first word. I added the following patch and it worked again
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 498e35c8db2c..f514966e2ada 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -659,11 +659,22 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
>  		ctrl |= (spi_imx->target_burst * 8 - 1)
>  			<< MX51_ECSPI_CTRL_BL_OFFSET;
>  	else {
> -		if (spi_imx->count &gt;= 512)
> -			ctrl |= 0xFFF << MX51_ECSPI_CTRL_BL_OFFSET;
> -		else
> -			ctrl |= (spi_imx->count * spi_imx->bits_per_word - 1)
> +		if (spi_imx->usedma)
> +			ctrl |= (spi_imx->bits_per_word *
> +					 spi_imx_bytes_per_word(
> +						 spi_imx->bits_per_word) -
> +				 1)
>  				<< MX51_ECSPI_CTRL_BL_OFFSET;
> +		else {
> +			if (spi_imx->count &gt;= MX51_ECSPI_CTRL_MAX_BURST)
> +				ctrl |= (MX51_ECSPI_CTRL_MAX_BURST - 1)
> +					<< MX51_ECSPI_CTRL_BL_OFFSET;
> +			else
> +				ctrl |= (spi_imx->count *
> +						 spi_imx->bits_per_word -
> +					 1)
> +					<< MX51_ECSPI_CTRL_BL_OFFSET;
> +		}
>  	}
>  
>  	/* set clock speed */
> 
> Best regards,
Benjamin Bigler

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Linux regression tracking (Thorsten Leemhuis)]

On 27.11.23 10:09, linux@bigler.io wrote:
> 
> Thank you Benjamin for the patch.
> I have tested the patch and is works fine.

That's great.

Benjamin, what's the status of the fix for this regression? I tried to
find a proper submission for review, but didn't find one. Am I missing
something or did that fall through the cracks?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

> Am 2023-11-26T14:19:56.000+01:00 hat Benjamin Bigler <benjamin@bigler.one> geschrieben:
>>  Hi
>>
>> I did some debugging and I think the problem is that in this case bits_per_word is 8. So in
>> spi_imx_dma_configure the buswidth is set to DMA_SLAVE_BUSWIDTH_1_BYTE. But in
>> mx51_ecspi_prepare_transfer the BURST_LENGTH is now set to (spi_imx->count * spi_imx->bits_per_word
>> - 1)
>> before 15a6af94a2779d5dfb42ee4bfac858ea8e964a3f it was (spi_imx->bits_per_word - 1). Now the spi
>> transmits 4 byte per byte except for the first word. I added the following patch and it worked again
>>
>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
>> index 498e35c8db2c..f514966e2ada 100644
>> --- a/drivers/spi/spi-imx.c
>> +++ b/drivers/spi/spi-imx.c
>> @@ -659,11 +659,22 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
>>  		ctrl |= (spi_imx->target_burst * 8 - 1)
>>  			<< MX51_ECSPI_CTRL_BL_OFFSET;
>>  	else {
>> -		if (spi_imx->count &gt;= 512)
>> -			ctrl |= 0xFFF << MX51_ECSPI_CTRL_BL_OFFSET;
>> -		else
>> -			ctrl |= (spi_imx->count * spi_imx->bits_per_word - 1)
>> +		if (spi_imx->usedma)
>> +			ctrl |= (spi_imx->bits_per_word *
>> +					 spi_imx_bytes_per_word(
>> +						 spi_imx->bits_per_word) -
>> +				 1)
>>  				<< MX51_ECSPI_CTRL_BL_OFFSET;
>> +		else {
>> +			if (spi_imx->count &gt;= MX51_ECSPI_CTRL_MAX_BURST)
>> +				ctrl |= (MX51_ECSPI_CTRL_MAX_BURST - 1)
>> +					<< MX51_ECSPI_CTRL_BL_OFFSET;
>> +			else
>> +				ctrl |= (spi_imx->count *
>> +						 spi_imx->bits_per_word -
>> +					 1)
>> +					<< MX51_ECSPI_CTRL_BL_OFFSET;
>> +		}
>>  	}
>>  
>>  	/* set clock speed */
>>
>> Best regards,
> Benjamin Bigler
> 
> 

Re: spi: imx: Increase imx51 ecspi burst length fails on imx6dl and imx8mm

[Benjamin Bigler]

> > On 27.11.23 10:09, linux@bigler.io wrote:
> > > > 
> > > > Thank you Benjamin for the patch.
> > > > I have tested the patch and is works fine.
> > 
> > That's great.
> > 
> > Benjamin, what's the status of the fix for this regression? I tried to
> > find a proper submission for review, but didn't find one. Am I missing
> > something or did that fall through the cracks?

I haven't send a proper patch yet. I will read through the guidelines and
submit one on the weekend.

> > 
> > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> > --
> > Everything you wanna know about Linux kernel regression tracking:
> > https://linux-regtracking.leemhuis.info/about/#tldr
> > If I did something stupid, please tell me, as explained on that page.
> > 
> > #regzbot poke
> > 
> 

Best regards,
Benjamin Bigler