On Mon, May 30, 2022 at 11:22:26AM +0000, Conor.Dooley@microchip.com wrote:

On 26/05/2022 19:13, Ivan Bornyakov wrote:

quoted

+static int mpf_read_status(struct spi_device *spi)
+{
+       u8 status = 0, status_command = MPF_SPI_READ_STATUS;
+       /*
+        * Two identical SPI transfers are used for status reading.
+        * The reason is that the first one can be inadequate.
+        * We ignore it completely and use the second one.
+        */
+       struct spi_transfer xfers[] = {
+               [0 ... 1] = {
+                       .tx_buf = &status_command,
+                       .rx_buf = &status,
+                       .len = 1,
+                       .cs_change = 1,
+               }
+       };

Hmm, I don't think that this is correct, or at least it is not
correct from the polarfire /soc/ perspective. I was told that
there was nothing different other than the envm between the
programming for both devices - but this is another situation
where I start to question that.

When I run this code, ISC enable /never/ passes - failing due
to timing out. I see something like this picture here:
https://i.imgur.com/EKhd1S3.png
You can see the 0x0B ISC enable coming through & then a status
check after it.

With the current code, the value of the "status" variable will
be 0x0, given you are overwriting the first MISO value with the
second. According to the hw guys, the spi hw status *should*
only be returned on MISO in the first byte after SS goes low.

If this is not the case for a non -soc part, which, as I said
before, I don't have a board with the SPI programmer exposed
for & I have been told is not the case then my comments can
just be ignored entirely & I'll have some head scratching to
do...

Thanks,
Conor.

If I understood correctly, SS doesn't alter between two status reading
transactions despite .cs_change = 1. May be adding some .cs_change_delay
to spi_transfer struct can help with that?

quoted

+       int ret = spi_sync_transfer(spi, xfers, 2);
+
+       if ((status & MPF_STATUS_SPI_VIOLATION) ||
+           (status & MPF_STATUS_SPI_ERROR))
+               ret = -EIO;
+
+       return ret ? : status;
+}