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

[Conor.Dooley]

On 22/05/2022 21:22, Ivan Bornyakov wrote:
> On Mon, May 23, 2022 at 12:52:32AM +0800, Xu Yilun wrote:
>> On Fri, May 13, 2022 at 07:27:54PM +0300, Ivan Bornyakov wrote:
>>> Add support to the FPGA manager for programming Microchip Polarfire
>>> FPGAs over slave SPI interface with .dat formatted bitsream image.

---8<---

>>> +static int mpf_read_status(struct spi_device *spi)
>>> +{
>>> +	u8 status = 0, status_command = MPF_SPI_READ_STATUS;
>>> +	struct spi_transfer xfers[] = {
>>> +		[0 ... 1] = {
>>> +			.tx_buf = &status_command,
>>> +			.rx_buf = &status,
>>> +			.len = 1,
>>> +			.cs_change = 1,
>>> +		}
>>> +	};
>>> +	int ret = spi_sync_transfer(spi, xfers, 2);
>>
>> I remember it is spi_w8r8 for the first time, why we change to
>> spi_sync_transfer? They behavior differently on spi bus.
> 
> On v8 Conor reported that spi_w8r8 was not correct way to read the status,
> despite that my HW was giving reasonable result.
> 
> See https://lore.kernel.org/linux-fpga/7fcde9aa-c086-33e1-1619-04663bfeff85@xxxxxxxxxxxxx/#t

For some context, the spi slave programming was implemented to match the
jtag programming process. The status can't be read with w8r8() because it
comes directly from the hardware between the pins on the FPGA and the spi
slave & will start clocking it out onto MISO as soon as it has a clock
and its select is active.
I am not really sure how it worked that way for you.

> 
>>
>> And why we need to xfer the same message twice? If it relates to
>> some HW behavior, we'd better add some comments here.
>>
> 
> On v11 Conor reported that he observed inadequate status readings, and
> double status reads fixed that. There is also a hint in Microchip's
> "SPI-DirectC User Guide" that status should be read two times, but not a
> clear statement.
> 
> See https://lore.kernel.org/linux-fpga/4b752147-1a09-a4af-bc5d-3b132b84ef49@xxxxxxxxxxx/#t
> 
> Anyway, I'll add some words.

I'll be honest and say that the DirectC user guide is not great, I had
a look for that "hint" but it didn't stand out to me.
Unless that is you meant two screenshots of "checking hardware status"
at this URL in section 10's subsections?
https://onlinedocs.microchip.com/pr/GUID-835917AF-E521-4046-AD59-DCB458EB8466-en-US-1/index.html

If that is the case, that seems very insufficient to me. I will try to
get clarification & suggest that the documentation be improved if so.

---8<---

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

Sure, I'll take a look. If I can't answer I'll try to find out.

I've been really busy the last week (and will be this coming week too) so
while I did use v12, I have not been able to properly sit down and look
at it properly yet, so apologies for the radio silence from me. Hopefully
I will get a chance this week & can give either a review or ack etc.

Thanks,
Conor.

> 
>>> +		if (ret) {
>>> +			dev_err(dev, "Failed to write bitstream frame %d/%zd\n",
>>
>> %zu for size_t
>>
>>> +				i, count / MPF_SPI_FRAME_SIZE);
>>> +			return ret;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int mpf_ops_write_complete(struct fpga_manager *mgr,
>>> +				  struct fpga_image_info *info)
>>> +{
>>> +	const u8 isc_dis_command[] = { MPF_SPI_ISC_DISABLE };
>>> +	const u8 release_command[] = { MPF_SPI_RELEASE };
>>> +	struct mpf_priv *priv = mgr->priv;
>>> +	struct device *dev = &mgr->dev;
>>> +	struct spi_device *spi;
>>> +	int ret;
>>> +
>>> +	spi = priv->spi;
>>> +
>>> +	ret = mpf_spi_write(spi, isc_dis_command, sizeof(isc_dis_command));
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to disable ISC: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	usleep_range(1000, 2000);
>>> +
>>> +	ret = mpf_spi_write(spi, release_command, sizeof(release_command));
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to exit program mode: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	priv->program_mode = false;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct fpga_manager_ops mpf_ops = {
>>> +	.state = mpf_ops_state,
>>> +	.initial_header_size = 71,
>>> +	.parse_header = mpf_ops_parse_header,
>>> +	.write_init = mpf_ops_write_init,
>>> +	.write = mpf_ops_write,
>>> +	.write_complete = mpf_ops_write_complete,
>>> +};
>>> +
>>> +static int mpf_probe(struct spi_device *spi)
>>> +{
>>> +	struct device *dev = &spi->dev;
>>> +	struct fpga_manager *mgr;
>>> +	struct mpf_priv *priv;
>>> +
>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->spi = spi;
>>> +
>>> +	mgr = devm_fpga_mgr_register(dev, "Microchip Polarfire SPI FPGA Manager",
>>> +				     &mpf_ops, priv);
>>> +
>>> +	return PTR_ERR_OR_ZERO(mgr);
>>> +}
>>> +
>>> +static const struct spi_device_id mpf_spi_ids[] = {
>>> +	{ .name = "mpf-spi-fpga-mgr", },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(spi, mpf_spi_ids);
>>> +
>>> +#if IS_ENABLED(CONFIG_OF)
>>> +static const struct of_device_id mpf_of_ids[] = {
>>> +	{ .compatible = "microchip,mpf-spi-fpga-mgr" },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, mpf_of_ids);
>>> +#endif /* IS_ENABLED(CONFIG_OF) */
>>> +
>>> +static struct spi_driver mpf_driver = {
>>> +	.probe = mpf_probe,
>>> +	.id_table = mpf_spi_ids,
>>> +	.driver = {
>>> +		.name = "microchip_mpf_spi_fpga_mgr",
>>> +		.of_match_table = of_match_ptr(mpf_of_ids),
>>> +	},
>>> +};
>>> +
>>> +module_spi_driver(mpf_driver);
>>> +
>>> +MODULE_DESCRIPTION("Microchip Polarfire SPI FPGA Manager");
>>> +MODULE_LICENSE("GPL");
>>> -- 
>>> 2.35.1
>>>
>