On 09/05/2022 19:56, Conor Dooley wrote:
On 09/05/2022 18:16, Ivan Bornyakov wrote:
On Mon, May 09, 2022 at 11:41:18AM +0000, Conor.Dooley@xxxxxxxxxxxxx wrote:
Hey Ivan, one comment below.
Thanks,
Conor.
On 07/05/2022 08:43, Ivan Bornyakov wrote:
... snip ...
+static int mpf_read_status(struct spi_device *spi)
+{
+ u8 status, status_command = MPF_SPI_READ_STATUS;
+ struct spi_transfer xfer = {
+ .tx_buf = &status_command,
+ .rx_buf = &status,
+ .len = 1,
+ };
+ int ret = spi_sync_transfer(spi, &xfer, 1);
+
+ if ((status & MPF_STATUS_SPI_VIOLATION) ||
+ (status & MPF_STATUS_SPI_ERROR))
+ ret = -EIO;
+
+ return ret ? : status;
+}
+
... snip ...
+
+static int poll_status_not_busy(struct spi_device *spi, u8 mask)
+{
+ int status, timeout = MPF_STATUS_POLL_TIMEOUT;
+
+ while (timeout--) {
+ status = mpf_read_status(spi);
+ if (status < 0 ||
+ (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
+ return status;
+
+ usleep_range(1000, 2000);
+ }
+
+ return -EBUSY;
+}
Is there a reason you changed this from the snippet you sent me
in the responses to version 8:
static int poll_status_not_busy(struct spi_device *spi, u8 mask)
{
u8 status, status_command = MPF_SPI_READ_STATUS;
int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
struct spi_transfer xfer = {
.tx_buf = &status_command,
.rx_buf = &status,
.len = 1,
};
while (timeout--) {
ret = spi_sync_transfer(spi, &xfer, 1);
if (ret < 0)
return ret;
if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
return status;
usleep_range(1000, 2000);
}
return -EBUSY;
}
With the current version, I hit the "Failed to write bitstream
frame" check in mpf_ops_write at random points in the transfer.
Replacing poll_status_not_busy with the above allows it to run
to completion.
In my eyes they are equivalent, aren't they?
I was in a bit of a rush today & didn't have time to do proper
debugging, I'll put some debug code in tomorrow and try to find
exactly what is different between the two.
Off the top of my head, since I don't have a board on me to test,
the only difference I can see is that with the snippet you only
checked if spi_sync_transfer was negative whereas now you check
if it has a value at all w/ that ternary operator.
But even that seems like it *shouldn't* be the problem, since ret
should contain -errno or zero, right?
Either way, I will do some digging tomorrow.
I put a printk("status %x, ret %d", status, ret); into the failure
path of mpf_read_status() & it looks like a status 0xA is being
returned - error & ready? That seems like a very odd combo to be
getting back out of it. It shouldn't be dodgy driver/connection
either, b/c that's what I see if I connect my protocol analyser:
https://i.imgur.com/VbjgfCk.png
That's mosi (hex), ss, sclk, mosi, miso (hex), miso in descending
order.
I think what was happening was with the snippet you returned one
of the following: -EBUSY, ret (aka -errno) or status. Since status
is positive, the checks in mpf_spi_write.*() saw nothing wrong at
all and programming continued despite there being a problem.
The new version fixes this by returning -EIO rather than status from
poll_status_not_busy().
I wish I had a socketable PolarFire so I could investigate further,
but this looks like it might a be hardware issue somewhere on my
end?
So ye, sorry for the noise and carry on! I'll try tofind what is to
blame for it.
Thanks,
Conor.
