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@xxxxxxxxxxxxx.
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?
Thanks,
Conor.
>
> [...]
>
>
>> +static int mpf_ops_parse_header(struct fpga_manager *mgr,
>> + struct fpga_image_info *info,
>> + const char *buf, size_t count)
>> +{
>> + size_t component_size_byte_num, component_size_byte_off,
>> + components_size_start, bitstream_start, i,
>> + block_id_offset, block_start_offset;
>> + u8 header_size, blocks_num, block_id;
>
> I think component_size_byte_num, component_size_byte_off, i should be size_t
> are all simple numbers irrelated to data size, so maybe u32 is just good.
>
> Thanks,
> Yilun
>
>> + u32 block_start, component_size;
>> + u16 components_num;
>> +
>> + if (!buf) {
>> + dev_err(&mgr->dev, "Image buffer is not provided\n");
>> + return -EINVAL;
>> + }
>> +
>> + header_size = *(buf + MPF_HEADER_SIZE_OFFSET);
>> + if (header_size > count) {
>> + info->header_size = header_size;
>> + return -EAGAIN;
>> + }
>> +
>> + /*
>> + * Go through look-up table to find out where actual bitstream starts
>> + * and where sizes of components of the bitstream lies.
>> + */
>> + blocks_num = *(buf + header_size - 1);
>> + block_id_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET;
>> + block_start_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_START_OFFSET;
>> +
>> + header_size += blocks_num * MPF_LOOKUP_TABLE_RECORD_SIZE;
>> + if (header_size > count) {
>> + info->header_size = header_size;
>> + return -EAGAIN;
>> + }
>> +
>> + components_size_start = 0;
>> + bitstream_start = 0;
>> +
>> + while (blocks_num--) {
>> + block_id = *(buf + block_id_offset);
>> + block_start = get_unaligned_le32(buf + block_start_offset);
>> +
>> + switch (block_id) {
>> + case MPF_BITSTREAM_ID:
>> + info->header_size = bitstream_start = block_start;
>> + if (block_start > count)
>> + return -EAGAIN;
>> +
>> + break;
>> + case MPF_COMPONENTS_SIZE_ID:
>> + components_size_start = block_start;
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + if (bitstream_start && components_size_start)
>> + break;
>> +
>> + block_id_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
>> + block_start_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
>> + }
>> +
>> + if (!bitstream_start || !components_size_start) {
>> + dev_err(&mgr->dev, "Failed to parse header look-up table\n");
>> + return -EFAULT;
>> + }
>> +
>> + /*
>> + * Parse bitstream size.
>> + * Sizes of components of the bitstream are 22-bits long placed next
>> + * to each other. Image header should be extended by now up to where
>> + * actual bitstream starts, so no need for overflow check anymore.
>> + */
>> + components_num = get_unaligned_le16(buf + MPF_DATA_SIZE_OFFSET);
>> +
>> + for (i = 0; i < components_num; i++) {
>> + component_size_byte_num =
>> + (i * MPF_BITS_PER_COMPONENT_SIZE) / BITS_PER_BYTE;
>> + component_size_byte_off =
>> + (i * MPF_BITS_PER_COMPONENT_SIZE) % BITS_PER_BYTE;
>> +
>> + component_size = get_unaligned_le32(buf +
>> + components_size_start +
>> + component_size_byte_num);
>> + component_size >>= component_size_byte_off;
>> + component_size &= GENMASK(MPF_BITS_PER_COMPONENT_SIZE - 1, 0);
>> +
>> + info->data_size += component_size * MPF_SPI_FRAME_SIZE;
>> + }
>> +
>> + return 0;
>> +}
