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:

quoted

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:

quoted

... 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.