On 30/03/2022 15:48, Ivan Bornyakov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi, Conor!
>
> On Wed, Mar 30, 2022 at 02:37:05PM +0000, Conor.Dooley@xxxxxxxxxxxxx wrote:
>> Hey Ivan,
>> Been testing this and generated a couple questions.
>> I've put them inline where they were relevant.
>> Thanks,
>> Conor.
>>
>> On 22/03/2022 19:15, Ivan Bornyakov wrote:
>>>
>>> ... snip ...
>>>
>>> +static int mpf_ops_write_init(struct fpga_manager *mgr,
>>> + struct fpga_image_info *info, const char *buf,
>>> + size_t count)
>>> +{
>>> + const u8 program_mode[] = { MPF_SPI_FRAME_INIT, MPF_SPI_PRG_MODE };
>>> + const u8 isc_en_command[] = { MPF_SPI_ISC_ENABLE };
>>> + struct mpf_priv *priv = mgr->priv;
>>> + struct device *dev = &mgr->dev;
>>> + struct spi_device *spi;
>>> + u32 isc_ret;
>>> + int ret;
>>> +
>>> + if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
>>> + dev_err(dev, "Partial reconfiguration is not supported\n");
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + spi = priv->spi;
>>> +
>>> + ret = mpf_spi_write_then_read(spi, isc_en_command, sizeof(isc_en_command),
>>> + &isc_ret, sizeof(isc_ret));
>>> + if (ret || isc_ret) {
>>> + dev_err(dev, "Failed to enable ISC: %d\n", ret ? : isc_ret);
>>> + return -EFAULT;
>>> + }
>>
>> So, my test board for this has had a PolarFire SoC, not a standard
>> PolarFire. I ran into some problems with the ISC enable code, due to
>> a sequence error. After sending the SPI_ISC_ENABLE, you then do a
>> poll_status_not_busy to hold until you see a STATUS_READY.
>> poll_status_not_busy does a w8r8 to request and then read the status,
>> and you expect a sequence as below:
>>
>> op: w w r w r
>> M: 0xB 0x0 0x0
>> S: 0x1 0x2
>>
>> I could not get past this check & it would just poll until the
>> timeout. What I saw on a protocol analyser was more like so:
>>
>> op: w w r w r
>> M: 0xB 0x0 0x0
>> S: 0x1 0x0 0x2 0x0
>>
>> So the read in that w8r8 would always get a zero back and then time out.
>> Changing the poll function (just for isc) to only read gave:
>>
>> op: w r r
>> M: 0xB 0x0 0x0
>> S: 0x1 0x2
>>
>> For the code after the ISC enable, I reverted to your implementation
>> of the poll function & the rest of the programming sequence ran.
>>
>> I spoke to the guys that wrote the HW about this, and they said that
>> reading the status back *as* the 0x0 the poll command is clocked in is
>> the expected behaviour.
>> They also said that MPF should work identically to an MPFS and I was unable
>> to find a documented difference between MPF and MPFS other than the envm,
>> which is an optional component anyway.
>>
>> But I can only assume that what you were doing worked for you, so if
>> you could possibly share some waveforms of the write_init sequence
>> that'd be great. Or if there is something that you think I am
>> overlooking, please let me know.
>>
>
> If you replace poll_status_not_busy() function with this code:
>
> 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;
> }
>
> Will it work for you? It is still works in my case.
Yeah, status checking after the ISC enable works for me with
this changed. However, the mpf_ops_state still uses w8r8 &
will need to be changed too.
Thanks,
Conor.
