Re: [PATCH v11 2/3] fpga: microchip-spi: add Microchip MPF FPGA manager

[Conor Dooley <mail@conchuod.ie>]

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@microchip.com 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.