One last item, sorry!

On 30/05/2022 15:24, Conor Dooley wrote:

On 30/05/2022 13:07, Ivan Bornyakov wrote:

quoted

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

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

quoted

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,

Should cs_change be set for both messages or just the first?
 From reading the documentation, it looks like we only want it
for the first one.

https://elixir.bootlin.com/linux/latest/source/include/linux/spi/spi.h#L895

Thanks,
Conor.

quoted

quoted

quoted

+               }
+       };

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?

D-oh - bug in the spi controller driver :)
LGTM now, successfully programmed my PolarFire SoC with v12.
I'd almost suggest adding a compatible for it too - but since
the envm programming doesn't work I don't think that would be
correct.

Tested-by: Conor Dooley <conor.dooley@microchip.com>

With a small comment about why it's using spi_sync_transfer():
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

quoted

quoted

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