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

[<Conor.Dooley@microchip.com>]

On 29/05/2022 19:51, Ivan Bornyakov wrote:
> On Sun, May 29, 2022 at 01:03:10PM +0000, Conor.Dooley@microchip.com wrote:
>> On 29/05/2022 13:39, Xu Yilun wrote:
>>> On Thu, May 26, 2022 at 09:13:43PM +0300, Ivan Bornyakov wrote:
>>>> Add support to the FPGA manager for programming Microchip Polarfire
>>>> FPGAs over slave SPI interface with .dat formatted bitsream image.
>>>
>>> From previous mail thread, there are still some hardware operations yet
>>> to be clarified, so I may need a Reviewed-by from Conor.Dooley@microchip.com.
>>
>> Yeah, was really busy last week. Planning on giving this version a go
>> tomorrow. I *think* I explained the reason the status check needed to be a
>> sync_transfer() but it hasn't been reflected in a comment yet.
>>
>> I didn't know the answers to the two other questions & passed them on to the
>> designers of the hardware blocks - but both are traveling so not got a
>> response yet. There's one bit of clarification I'd like from your:
>>
>>>>> +static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
>>>>> +{
>>>>> +	u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
>>>>> +	struct mpf_priv *priv = mgr->priv;
>>>>> +	struct device *dev = &mgr->dev;
>>>>> +	struct spi_device *spi;
>>>>> +	int ret, i;
>>>>> +
>>>>> +	if (count % MPF_SPI_FRAME_SIZE) {
>>>>> +		dev_err(dev, "Bitstream size is not a multiple of %d\n",
>>>>> +			MPF_SPI_FRAME_SIZE);
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	spi = priv->spi;
>>>>> +
>>>>> +	for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
>>>>> +		memcpy(tmp_buf + 1, buf + i * MPF_SPI_FRAME_SIZE,
>>>>> +		       MPF_SPI_FRAME_SIZE);
>>>>> +
>>>>> +		ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
>>>>
>>>> As I mentioned before, is it possible we use spi_sync_transfer to avoid
>>>> memcpy the whole bitstream?
>>>
>>> Unfortunately, I didn't succeed with spi_sunc_transfer here. May be
>>> Conor or other folks with more insight on Microchip's HW would be able
>>> to eliminate this memcpy...
>>
>> I understood this as being a question about why it was required to check
>> the status of the hardware between frames of the bitstream rather than
>> using spi_sync_transfer() to copy each frame back to back.
>>
>> Is that correct?
> 
> No.
> The issue here is memcpy() a bitstream data frame to temporary buffer
> before sending it to the device.
> The reason for memcpy() is that we need to send to the device 17 bytes:
> 1st byte 0xEE and next 16 bytes - bitstream data.
> Possible solution to eliminate memcpy() is to use spi_sync_transfer()
> instead of spi_write() for sending bitstream frames, like so:
> 
> diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> index 7579b0de119f..bf62ee7fd630 100644
> --- a/drivers/fpga/microchip-spi.c
> +++ b/drivers/fpga/microchip-spi.c
> @@ -270,7 +270,8 @@ static int mpf_ops_write_init(struct fpga_manager *mgr,
> 
>  static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
>  {
> -       u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
> +       u8 spi_frame_command = MPF_SPI_FRAME;
> +       struct spi_transfer xfers[2] = { 0 };
>         struct mpf_priv *priv = mgr->priv;
>         struct device *dev = &mgr->dev;
>         struct spi_device *spi;
> @@ -285,10 +286,15 @@ static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count
>         spi = priv->spi;
> 
>         for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
> -               memcpy(tmp_buf + 1, buf + i * MPF_SPI_FRAME_SIZE,
> -                      MPF_SPI_FRAME_SIZE);
> +               xfers[0].tx_buf = &spi_frame_command;
> +               xfers[0].len = 1;
> +               xfers[1].tx_buf = buf + i * MPF_SPI_FRAME_SIZE;
> +               xfers[1].len = MPF_SPI_FRAME_SIZE;
> +
> +               ret = mpf_poll_status(spi, 0);
> +               if (ret >= 0)
> +                       ret = spi_sync_transfer(spi, xfers, 2);
> 
> -               ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
>                 if (ret) {
>                         dev_err(dev, "Failed to write bitstream frame %d/%zu\n",
>                                 i, count / MPF_SPI_FRAME_SIZE);
> 
> Note that this is not a working solution.

Hmm, I'll take a look again. I did quickly do something like this
last Monday when I was trying to figure out what was meant, but I
omitted the mpf_poll_status() and that was enough to screw it up.
I'll take another look w/ this snippet.
Thanks,
Conor.