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

[<Conor.Dooley@microchip.com>]

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