[PATCH 0/1] spi: fsl_espi: fix din offset

[Tomas Alvarez Vanoli]

Hi

>What kind of issues do you see? And how does your patch resolves
>this? I guess we can assume the code was working correctly once.

The problem was that when calling spi_xfer like so:

`spi_xfer(spi_slv, bitlen, &dout, &din, SPI_XFER_ONCE);`

To read a read only part of chip with known values, I was getting garbage.

Digging deeper into the driver and adding print statements I was able to
determine that:

- fsl_espi_rx was getting the expected data and putting it in the correct
address (local din)

- The memcpy later in the code was taking data from a different address
(buffer + 2 * cmd_len) and putting it in the user pointer (the &din from my
call)

>Honestly, I don't understand that code either. ...

The switch at the start of the function determines the buffer and offset setup.
To be fair I only focused on my issue, SPI_XFER_ONCE which is the same as
SPI_XFER_BEGIN | SPI_XFER_END, and in a small transaction.

Inside the ONCE case, the driver allocates a buffer of double the transaction,
because it puts the tx data in the first half, and then will write the rx data
in the second half.

The case of only "SPI_XFER_END" is a bit more "cryptic" to put it nicely, and I
did not properly debug it if I am being honest.

However, for my case of the ONCE transfer, if you look at the start of the while
loop:

```
	while (num_chunks--) {
		if (data_in)
			din = buffer + rx_offset;
```

This means that if the user passed a pointer to get the read data back, then
we set a local "din" to the buffer we allocated at the start plus rx_offset.
In the "ONCE" case, this is the second half of the buffer. In the END case, this
"cmd_len".

This din pointer is what is passed to "fsl_espi_rx", where I had corroborated
that the chip data read was the expected.

When we get to later in the while loop, there's this:

```
		if (data_in) {
			memcpy(data_in, buffer + 2 * cmd_len, tran_len);
			if (*buffer == 0x0b) {
				data_in += tran_len;
				data_len -= tran_len;
				*(int *)buffer += tran_len;
			}
		}
```

where I made my change, in the memcpy source pointer. This is the code in charge
of copying the data back to the caller/user.

As you can see, for the ONCE case, cmd_len is 0, so it is taking the data at the
start of buffer (my tx data, not the rx data). Hopefully the bug I was
experiencing is clearer with this explanation.

As I mentally traced this code I realized of a better fix, hopefully you can
review this and/or test in on your board too.

Given that for the ONCE transfer cmd_len is 0, what could be changed is also:

diff --git a/drivers/spi/fsl_espi.c b/drivers/spi/fsl_espi.c
index b1586d12912..ef2ebd5840b 100644
--- a/drivers/spi/fsl_espi.c
+++ b/drivers/spi/fsl_espi.c
@@ -287,13 +287,13 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *data_out,
                break;
        case SPI_XFER_BEGIN | SPI_XFER_END:
                len = data_len;
-               buffer = (unsigned char *)malloc(len * 2);
+               buffer = (unsigned char *)malloc(len);
                if (!buffer) {
                        debug("SF: Failed to malloc memory.\n");
                        return 1;
                }
                memcpy(buffer, data_out, len);
-               rx_offset = len;
+               rx_offset = 0;
                cmd_len = 0;
                break;
        }

This theoretically should work and should also not affect the "END" case.

The two things that support this are that:

- The loop always writes out first, so the rx data can safely overwrite the tx
- cmd_len is set to 0 for the ONCE case, so data_in memcpy would take the rx
data from the right rx_offset (0)

Hopefully this explanation and my proposed alternative fix are helpful, I'm glad
someone else was able to test this, I had cc'ed someone from nxp hoping they
could shine some light on the code but their email does not exist anymore.

It would be good to know what flags the flash probe is passing to the driver,
to know if the problem actualyl arises from the switch statement at the start
or handling multiple chunks/blks. I can say for certain that for a single
chunk and signle block, the ONCE transfer was not working correctly for me.

Best Regards,
Tomas