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

[<Conor.Dooley@microchip.com>]

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?

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;
>> +}