Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <da1e5125-de6b-11a8-a52d-7e6e5f45ab70@conchuod.ie>
Date:   Mon, 9 May 2022 19:56:15 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v11 2/3] fpga: microchip-spi: add Microchip MPF FPGA
 manager
Content-Language: en-US
To:     Ivan Bornyakov <i.bornyakov@metrotek.ru>,
        Conor.Dooley@microchip.com
Cc:     mdf@kernel.org, hao.wu@intel.com, yilun.xu@intel.com,
        trix@redhat.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-fpga@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        system@metrotek.ru
References: <20220507074304.11144-1-i.bornyakov@metrotek.ru>
 <20220507074304.11144-3-i.bornyakov@metrotek.ru>
 <bd5cb37b-ee56-f6d5-2d98-c08566b60728@microchip.com>
 <20220509171621.zk4owxwlngxjodgz@x260>
From:   Conor Dooley <mail@conchuod.ie>
In-Reply-To: <20220509171621.zk4owxwlngxjodgz@x260>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

Thanks,
Conor.