| Message ID | 20220526181344.2088-3-i.bornyakov@metrotek.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Microchip Polarfire FPGA manager | expand |
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. [...] > +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; > +}
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; >> +}
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.
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.
On 26/05/2022 19:13, Ivan Bornyakov wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Add support to the FPGA manager for programming Microchip Polarfire > FPGAs over slave SPI interface with .dat formatted bitsream image. > > Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru> > --- > drivers/fpga/Kconfig | 9 + > drivers/fpga/Makefile | 1 + > drivers/fpga/microchip-spi.c | 386 +++++++++++++++++++++++++++++++++++ > 3 files changed, 396 insertions(+) > create mode 100644 drivers/fpga/microchip-spi.c > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > index 26025dbab353..75806ef5c9ea 100644 > --- a/drivers/fpga/Kconfig > +++ b/drivers/fpga/Kconfig > @@ -248,4 +248,13 @@ config FPGA_MGR_VERSAL_FPGA > configure the programmable logic(PL). > > To compile this as a module, choose M here. > + > +config FPGA_MGR_MICROCHIP_SPI > + tristate "Microchip Polarfire SPI FPGA manager" > + depends on SPI > + help > + FPGA manager driver support for Microchip Polarfire FPGAs > + programming over slave SPI interface with .dat formatted > + bitstream image. > + > endif # FPGA > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile > index e32bfa90f968..5425a15892df 100644 > --- a/drivers/fpga/Makefile > +++ b/drivers/fpga/Makefile > @@ -19,6 +19,7 @@ obj-$(CONFIG_FPGA_MGR_XILINX_SPI) += xilinx-spi.o > obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o > obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += zynqmp-fpga.o > obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA) += versal-fpga.o > +obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI) += microchip-spi.o > obj-$(CONFIG_ALTERA_PR_IP_CORE) += altera-pr-ip-core.o > obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT) += altera-pr-ip-core-plat.o > > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c > new file mode 100644 > index 000000000000..7579b0de119f > --- /dev/null > +++ b/drivers/fpga/microchip-spi.c > @@ -0,0 +1,386 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Microchip Polarfire FPGA programming over slave SPI interface. > + */ > + > +#include <asm/unaligned.h> > +#include <linux/delay.h> > +#include <linux/fpga/fpga-mgr.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/spi/spi.h> > + > +#define MPF_SPI_ISC_ENABLE 0x0B > +#define MPF_SPI_ISC_DISABLE 0x0C > +#define MPF_SPI_READ_STATUS 0x00 > +#define MPF_SPI_READ_DATA 0x01 > +#define MPF_SPI_FRAME_INIT 0xAE > +#define MPF_SPI_FRAME 0xEE > +#define MPF_SPI_PRG_MODE 0x01 > +#define MPF_SPI_RELEASE 0x23 > + > +#define MPF_SPI_FRAME_SIZE 16 > + > +#define MPF_HEADER_SIZE_OFFSET 24 > +#define MPF_DATA_SIZE_OFFSET 55 > + > +#define MPF_LOOKUP_TABLE_RECORD_SIZE 9 > +#define MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET 0 > +#define MPF_LOOKUP_TABLE_BLOCK_START_OFFSET 1 > + > +#define MPF_COMPONENTS_SIZE_ID 5 > +#define MPF_BITSTREAM_ID 8 > + > +#define MPF_BITS_PER_COMPONENT_SIZE 22 > + > +#define MPF_STATUS_POLL_RETRIES 10000 > +#define MPF_STATUS_BUSY BIT(0) > +#define MPF_STATUS_READY BIT(1) > +#define MPF_STATUS_SPI_VIOLATION BIT(2) > +#define MPF_STATUS_SPI_ERROR BIT(3) > + > +struct mpf_priv { > + struct spi_device *spi; > + bool program_mode; > +}; > + > +static int mpf_read_status(struct spi_device *spi) > +{ > + u8 status = 0, status_command = MPF_SPI_READ_STATUS; > + /* > + * Two identical SPI transfers are used for status reading. > + * The reason is that the first one can be inadequate. > + * We ignore it completely and use the second one. > + */ > + struct spi_transfer xfers[] = { > + [0 ... 1] = { > + .tx_buf = &status_command, > + .rx_buf = &status, > + .len = 1, > + .cs_change = 1, > + } > + }; Hmm, I don't think that this is correct, or at least it is not correct from the polarfire /soc/ perspective. I was told that there was nothing different other than the envm between the programming for both devices - but this is another situation where I start to question that. When I run this code, ISC enable /never/ passes - failing due to timing out. I see something like this picture here: https://i.imgur.com/EKhd1S3.png You can see the 0x0B ISC enable coming through & then a status check after it. With the current code, the value of the "status" variable will be 0x0, given you are overwriting the first MISO value with the second. According to the hw guys, the spi hw status *should* only be returned on MISO in the first byte after SS goes low. If this is not the case for a non -soc part, which, as I said before, I don't have a board with the SPI programmer exposed for & I have been told is not the case then my comments can just be ignored entirely & I'll have some head scratching to do... Thanks, Conor. > + int ret = spi_sync_transfer(spi, xfers, 2); > + > + if ((status & MPF_STATUS_SPI_VIOLATION) || > + (status & MPF_STATUS_SPI_ERROR)) > + ret = -EIO; > + > + return ret ? : status; > +} > + > +static enum fpga_mgr_states mpf_ops_state(struct fpga_manager *mgr) > +{ > + struct mpf_priv *priv = mgr->priv; > + struct spi_device *spi; > + bool program_mode; > + int status; > + > + spi = priv->spi; > + program_mode = priv->program_mode; > + status = mpf_read_status(spi); > + > + if (!program_mode && !status) > + return FPGA_MGR_STATE_OPERATING; > + > + return FPGA_MGR_STATE_UNKNOWN; > +} > + > +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; > + 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; > +} > + > +/* Poll HW status until busy bit is cleared and mask bits are set. */ > +static int mpf_poll_status(struct spi_device *spi, u8 mask) > +{ > + int status, retries = MPF_STATUS_POLL_RETRIES; > + > + while (retries--) { > + status = mpf_read_status(spi); > + if (status < 0) > + return status; > + > + if (status & MPF_STATUS_BUSY) > + continue; > + > + if (!mask || (status & mask)) > + return status; > + } > + > + return -EBUSY; > +} > + > +static int mpf_spi_write(struct spi_device *spi, const void *buf, size_t buf_size) > +{ > + int status = mpf_poll_status(spi, 0); > + > + if (status < 0) > + return status; > + > + return spi_write(spi, buf, buf_size); > +} > + > +static int mpf_spi_write_then_read(struct spi_device *spi, > + const void *txbuf, size_t txbuf_size, > + void *rxbuf, size_t rxbuf_size) > +{ > + const u8 read_command[] = { MPF_SPI_READ_DATA }; > + int ret; > + > + ret = mpf_spi_write(spi, txbuf, txbuf_size); > + if (ret) > + return ret; > + > + ret = mpf_poll_status(spi, MPF_STATUS_READY); > + if (ret < 0) > + return ret; > + > + return spi_write_then_read(spi, read_command, sizeof(read_command), > + rxbuf, rxbuf_size); > +} > + > +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 = 0; > + 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: spi_ret %d, isc_ret %u\n", > + ret, isc_ret); > + return -EFAULT; > + } > + > + ret = mpf_spi_write(spi, program_mode, sizeof(program_mode)); > + if (ret) { > + dev_err(dev, "Failed to enter program mode: %d\n", ret); > + return ret; > + } > + > + priv->program_mode = true; > + > + return 0; > +} > + > +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)); > + if (ret) { > + dev_err(dev, "Failed to write bitstream frame %d/%zu\n", > + 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 > >
On Mon, May 30, 2022 at 11:22:26AM +0000, Conor.Dooley@microchip.com wrote: > On 26/05/2022 19:13, Ivan Bornyakov wrote: > > +static int mpf_read_status(struct spi_device *spi) > > +{ > > + u8 status = 0, status_command = MPF_SPI_READ_STATUS; > > + /* > > + * Two identical SPI transfers are used for status reading. > > + * The reason is that the first one can be inadequate. > > + * We ignore it completely and use the second one. > > + */ > > + struct spi_transfer xfers[] = { > > + [0 ... 1] = { > > + .tx_buf = &status_command, > > + .rx_buf = &status, > > + .len = 1, > > + .cs_change = 1, > > + } > > + }; > > Hmm, I don't think that this is correct, or at least it is not > correct from the polarfire /soc/ perspective. I was told that > there was nothing different other than the envm between the > programming for both devices - but this is another situation > where I start to question that. > > When I run this code, ISC enable /never/ passes - failing due > to timing out. I see something like this picture here: > https://i.imgur.com/EKhd1S3.png > You can see the 0x0B ISC enable coming through & then a status > check after it. > > With the current code, the value of the "status" variable will > be 0x0, given you are overwriting the first MISO value with the > second. According to the hw guys, the spi hw status *should* > only be returned on MISO in the first byte after SS goes low. > > If this is not the case for a non -soc part, which, as I said > before, I don't have a board with the SPI programmer exposed > for & I have been told is not the case then my comments can > just be ignored entirely & I'll have some head scratching to > do... > > Thanks, > Conor. > If I understood correctly, SS doesn't alter between two status reading transactions despite .cs_change = 1. May be adding some .cs_change_delay to spi_transfer struct can help with that? > > + int ret = spi_sync_transfer(spi, xfers, 2); > > + > > + if ((status & MPF_STATUS_SPI_VIOLATION) || > > + (status & MPF_STATUS_SPI_ERROR)) > > + ret = -EIO; > > + > > + return ret ? : status; > > +}
On 30/05/2022 13:07, Ivan Bornyakov wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, May 30, 2022 at 11:22:26AM +0000, Conor.Dooley@microchip.com wrote: >> On 26/05/2022 19:13, Ivan Bornyakov wrote: >>> +static int mpf_read_status(struct spi_device *spi) >>> +{ >>> + u8 status = 0, status_command = MPF_SPI_READ_STATUS; >>> + /* >>> + * Two identical SPI transfers are used for status reading. >>> + * The reason is that the first one can be inadequate. >>> + * We ignore it completely and use the second one. >>> + */ >>> + struct spi_transfer xfers[] = { >>> + [0 ... 1] = { >>> + .tx_buf = &status_command, >>> + .rx_buf = &status, >>> + .len = 1, >>> + .cs_change = 1, >>> + } >>> + }; >> >> Hmm, I don't think that this is correct, or at least it is not >> correct from the polarfire /soc/ perspective. I was told that >> there was nothing different other than the envm between the >> programming for both devices - but this is another situation >> where I start to question that. >> >> When I run this code, ISC enable /never/ passes - failing due >> to timing out. I see something like this picture here: >> https://i.imgur.com/EKhd1S3.png >> You can see the 0x0B ISC enable coming through & then a status >> check after it. >> >> With the current code, the value of the "status" variable will >> be 0x0, given you are overwriting the first MISO value with the >> second. According to the hw guys, the spi hw status *should* >> only be returned on MISO in the first byte after SS goes low. >> >> If this is not the case for a non -soc part, which, as I said >> before, I don't have a board with the SPI programmer exposed >> for & I have been told is not the case then my comments can >> just be ignored entirely & I'll have some head scratching to >> do... >> >> Thanks, >> Conor. >> > > If I understood correctly, SS doesn't alter between two status reading > transactions despite .cs_change = 1. May be adding some .cs_change_delay > to spi_transfer struct can help with that? D-oh - bug in the spi controller driver :) LGTM now, successfully programmed my PolarFire SoC with v12. I'd almost suggest adding a compatible for it too - but since the envm programming doesn't work I don't think that would be correct. Tested-by: Conor Dooley <conor.dooley@microchip.com> With a small comment about why it's using spi_sync_transfer(): Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > >>> + int ret = spi_sync_transfer(spi, xfers, 2); >>> + >>> + if ((status & MPF_STATUS_SPI_VIOLATION) || >>> + (status & MPF_STATUS_SPI_ERROR)) >>> + ret = -EIO; >>> + >>> + return ret ? : status; >>> +} >
On Mon, May 30, 2022 at 02:26:18PM +0000, Conor.Dooley@microchip.com wrote: > On 30/05/2022 13:07, Ivan Bornyakov wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Mon, May 30, 2022 at 11:22:26AM +0000, Conor.Dooley@microchip.com wrote: > >> On 26/05/2022 19:13, Ivan Bornyakov wrote: > >>> +static int mpf_read_status(struct spi_device *spi) > >>> +{ > >>> + u8 status = 0, status_command = MPF_SPI_READ_STATUS; > >>> + /* > >>> + * Two identical SPI transfers are used for status reading. > >>> + * The reason is that the first one can be inadequate. > >>> + * We ignore it completely and use the second one. > >>> + */ > >>> + struct spi_transfer xfers[] = { > >>> + [0 ... 1] = { > >>> + .tx_buf = &status_command, > >>> + .rx_buf = &status, > >>> + .len = 1, > >>> + .cs_change = 1, > >>> + } > >>> + }; > >> > >> Hmm, I don't think that this is correct, or at least it is not > >> correct from the polarfire /soc/ perspective. I was told that > >> there was nothing different other than the envm between the > >> programming for both devices - but this is another situation > >> where I start to question that. > >> > >> When I run this code, ISC enable /never/ passes - failing due > >> to timing out. I see something like this picture here: > >> https://i.imgur.com/EKhd1S3.png > >> You can see the 0x0B ISC enable coming through & then a status > >> check after it. > >> > >> With the current code, the value of the "status" variable will > >> be 0x0, given you are overwriting the first MISO value with the > >> second. According to the hw guys, the spi hw status *should* > >> only be returned on MISO in the first byte after SS goes low. > >> > >> If this is not the case for a non -soc part, which, as I said > >> before, I don't have a board with the SPI programmer exposed > >> for & I have been told is not the case then my comments can > >> just be ignored entirely & I'll have some head scratching to > >> do... > >> > >> Thanks, > >> Conor. > >> > > > > If I understood correctly, SS doesn't alter between two status reading > > transactions despite .cs_change = 1. May be adding some .cs_change_delay > > to spi_transfer struct can help with that? > > D-oh - bug in the spi controller driver :) So, no additional delay is needed? > LGTM now, successfully programmed my PolarFire SoC with v12. > I'd almost suggest adding a compatible for it too - but since > the envm programming doesn't work I don't think that would be > correct. > > Tested-by: Conor Dooley <conor.dooley@microchip.com> > > With a small comment about why it's using spi_sync_transfer(): > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > Thanks for your assistance, Conor! > > > >>> + int ret = spi_sync_transfer(spi, xfers, 2); > >>> + > >>> + if ((status & MPF_STATUS_SPI_VIOLATION) || > >>> + (status & MPF_STATUS_SPI_ERROR)) > >>> + ret = -EIO; > >>> + > >>> + return ret ? : status; > >>> +} > > >
On 30/05/2022 15:26, Ivan Bornyakov wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, May 30, 2022 at 02:26:18PM +0000, Conor.Dooley@microchip.com wrote: >> On 30/05/2022 13:07, Ivan Bornyakov wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On Mon, May 30, 2022 at 11:22:26AM +0000, Conor.Dooley@microchip.com wrote: >>>> On 26/05/2022 19:13, Ivan Bornyakov wrote: >>>>> +static int mpf_read_status(struct spi_device *spi) >>>>> +{ >>>>> + u8 status = 0, status_command = MPF_SPI_READ_STATUS; >>>>> + /* >>>>> + * Two identical SPI transfers are used for status reading. >>>>> + * The reason is that the first one can be inadequate. >>>>> + * We ignore it completely and use the second one. >>>>> + */ >>>>> + struct spi_transfer xfers[] = { >>>>> + [0 ... 1] = { >>>>> + .tx_buf = &status_command, >>>>> + .rx_buf = &status, >>>>> + .len = 1, >>>>> + .cs_change = 1, >>>>> + } >>>>> + }; >>>> >>>> Hmm, I don't think that this is correct, or at least it is not >>>> correct from the polarfire /soc/ perspective. I was told that >>>> there was nothing different other than the envm between the >>>> programming for both devices - but this is another situation >>>> where I start to question that. >>>> >>>> When I run this code, ISC enable /never/ passes - failing due >>>> to timing out. I see something like this picture here: >>>> https://i.imgur.com/EKhd1S3.png >>>> You can see the 0x0B ISC enable coming through & then a status >>>> check after it. >>>> >>>> With the current code, the value of the "status" variable will >>>> be 0x0, given you are overwriting the first MISO value with the >>>> second. According to the hw guys, the spi hw status *should* >>>> only be returned on MISO in the first byte after SS goes low. >>>> >>>> If this is not the case for a non -soc part, which, as I said >>>> before, I don't have a board with the SPI programmer exposed >>>> for & I have been told is not the case then my comments can >>>> just be ignored entirely & I'll have some head scratching to >>>> do... >>>> >>>> Thanks, >>>> Conor. >>>> >>> >>> If I understood correctly, SS doesn't alter between two status reading >>> transactions despite .cs_change = 1. May be adding some .cs_change_delay >>> to spi_transfer struct can help with that? >> >> D-oh - bug in the spi controller driver :) > > So, no additional delay is needed? Correct, programmed successfully without changing the delay. >> LGTM now, successfully programmed my PolarFire SoC with v12. Typo, this should be v13 >> I'd almost suggest adding a compatible for it too - but since >> the envm programming doesn't work I don't think that would be >> correct. >> >> Tested-by: Conor Dooley <conor.dooley@microchip.com> >> >> With a small comment about why it's using spi_sync_transfer(): >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >> > > Thanks for your assistance, Conor! > >>> >>>>> + int ret = spi_sync_transfer(spi, xfers, 2); >>>>> + >>>>> + if ((status & MPF_STATUS_SPI_VIOLATION) || >>>>> + (status & MPF_STATUS_SPI_ERROR)) >>>>> + ret = -EIO; >>>>> + >>>>> + return ret ? : status; >>>>> +} >>> >> >
One last item, sorry! On 30/05/2022 15:24, Conor Dooley wrote: > On 30/05/2022 13:07, Ivan Bornyakov wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On Mon, May 30, 2022 at 11:22:26AM +0000, Conor.Dooley@microchip.com wrote: >>> On 26/05/2022 19:13, Ivan Bornyakov wrote: >>>> +static int mpf_read_status(struct spi_device *spi) >>>> +{ >>>> + u8 status = 0, status_command = MPF_SPI_READ_STATUS; >>>> + /* >>>> + * Two identical SPI transfers are used for status reading. >>>> + * The reason is that the first one can be inadequate. >>>> + * We ignore it completely and use the second one. >>>> + */ >>>> + struct spi_transfer xfers[] = { >>>> + [0 ... 1] = { >>>> + .tx_buf = &status_command, >>>> + .rx_buf = &status, >>>> + .len = 1, >>>> + .cs_change = 1, Should cs_change be set for both messages or just the first? From reading the documentation, it looks like we only want it for the first one. https://elixir.bootlin.com/linux/latest/source/include/linux/spi/spi.h#L895 Thanks, Conor. >>>> + } >>>> + }; >>> >>> Hmm, I don't think that this is correct, or at least it is not >>> correct from the polarfire /soc/ perspective. I was told that >>> there was nothing different other than the envm between the >>> programming for both devices - but this is another situation >>> where I start to question that. >>> >>> When I run this code, ISC enable /never/ passes - failing due >>> to timing out. I see something like this picture here: >>> https://i.imgur.com/EKhd1S3.png >>> You can see the 0x0B ISC enable coming through & then a status >>> check after it. >>> >>> With the current code, the value of the "status" variable will >>> be 0x0, given you are overwriting the first MISO value with the >>> second. According to the hw guys, the spi hw status *should* >>> only be returned on MISO in the first byte after SS goes low. >>> >>> If this is not the case for a non -soc part, which, as I said >>> before, I don't have a board with the SPI programmer exposed >>> for & I have been told is not the case then my comments can >>> just be ignored entirely & I'll have some head scratching to >>> do... >>> >>> Thanks, >>> Conor. >>> >> >> If I understood correctly, SS doesn't alter between two status reading >> transactions despite .cs_change = 1. May be adding some .cs_change_delay >> to spi_transfer struct can help with that? > > D-oh - bug in the spi controller driver :) > LGTM now, successfully programmed my PolarFire SoC with v12. > I'd almost suggest adding a compatible for it too - but since > the envm programming doesn't work I don't think that would be > correct. > > Tested-by: Conor Dooley <conor.dooley@microchip.com> > > With a small comment about why it's using spi_sync_transfer(): > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > >> >>>> + int ret = spi_sync_transfer(spi, xfers, 2); >>>> + >>>> + if ((status & MPF_STATUS_SPI_VIOLATION) || >>>> + (status & MPF_STATUS_SPI_ERROR)) >>>> + ret = -EIO; >>>> + >>>> + return ret ? : status; >>>> +} >> >
On Tue, May 31, 2022 at 10:53:07AM +0000, Conor.Dooley@microchip.com wrote: > One last item, sorry! > > On 30/05/2022 15:24, Conor Dooley wrote: > > On 30/05/2022 13:07, Ivan Bornyakov wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> > >> On Mon, May 30, 2022 at 11:22:26AM +0000, Conor.Dooley@microchip.com wrote: > >>> On 26/05/2022 19:13, Ivan Bornyakov wrote: > >>>> +static int mpf_read_status(struct spi_device *spi) > >>>> +{ > >>>> + u8 status = 0, status_command = MPF_SPI_READ_STATUS; > >>>> + /* > >>>> + * Two identical SPI transfers are used for status reading. > >>>> + * The reason is that the first one can be inadequate. > >>>> + * We ignore it completely and use the second one. > >>>> + */ > >>>> + struct spi_transfer xfers[] = { > >>>> + [0 ... 1] = { > >>>> + .tx_buf = &status_command, > >>>> + .rx_buf = &status, > >>>> + .len = 1, > >>>> + .cs_change = 1, > > Should cs_change be set for both messages or just the first? > From reading the documentation, it looks like we only want it > for the first one. > > https://elixir.bootlin.com/linux/latest/source/include/linux/spi/spi.h#L895 > > Thanks, > Conor. > You're right, I've overlooked that moment. > >>>> + } > >>>> + }; > >>> > >>> Hmm, I don't think that this is correct, or at least it is not > >>> correct from the polarfire /soc/ perspective. I was told that > >>> there was nothing different other than the envm between the > >>> programming for both devices - but this is another situation > >>> where I start to question that. > >>> > >>> When I run this code, ISC enable /never/ passes - failing due > >>> to timing out. I see something like this picture here: > >>> https://i.imgur.com/EKhd1S3.png > >>> You can see the 0x0B ISC enable coming through & then a status > >>> check after it. > >>> > >>> With the current code, the value of the "status" variable will > >>> be 0x0, given you are overwriting the first MISO value with the > >>> second. According to the hw guys, the spi hw status *should* > >>> only be returned on MISO in the first byte after SS goes low. > >>> > >>> If this is not the case for a non -soc part, which, as I said > >>> before, I don't have a board with the SPI programmer exposed > >>> for & I have been told is not the case then my comments can > >>> just be ignored entirely & I'll have some head scratching to > >>> do... > >>> > >>> Thanks, > >>> Conor. > >>> > >> > >> If I understood correctly, SS doesn't alter between two status reading > >> transactions despite .cs_change = 1. May be adding some .cs_change_delay > >> to spi_transfer struct can help with that? > > > > D-oh - bug in the spi controller driver :) > > LGTM now, successfully programmed my PolarFire SoC with v12. > > I'd almost suggest adding a compatible for it too - but since > > the envm programming doesn't work I don't think that would be > > correct. > > > > Tested-by: Conor Dooley <conor.dooley@microchip.com> > > > > With a small comment about why it's using spi_sync_transfer(): > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > > >> > >>>> + int ret = spi_sync_transfer(spi, xfers, 2); > >>>> + > >>>> + if ((status & MPF_STATUS_SPI_VIOLATION) || > >>>> + (status & MPF_STATUS_SPI_ERROR)) > >>>> + ret = -EIO; > >>>> + > >>>> + return ret ? : status; > >>>> +} > >> > > >
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index 26025dbab353..75806ef5c9ea 100644 --- a/drivers/fpga/Kconfig +++ b/drivers/fpga/Kconfig @@ -248,4 +248,13 @@ config FPGA_MGR_VERSAL_FPGA configure the programmable logic(PL). To compile this as a module, choose M here. + +config FPGA_MGR_MICROCHIP_SPI + tristate "Microchip Polarfire SPI FPGA manager" + depends on SPI + help + FPGA manager driver support for Microchip Polarfire FPGAs + programming over slave SPI interface with .dat formatted + bitstream image. + endif # FPGA diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index e32bfa90f968..5425a15892df 100644 --- a/drivers/fpga/Makefile +++ b/drivers/fpga/Makefile @@ -19,6 +19,7 @@ obj-$(CONFIG_FPGA_MGR_XILINX_SPI) += xilinx-spi.o obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += zynqmp-fpga.o obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA) += versal-fpga.o +obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI) += microchip-spi.o obj-$(CONFIG_ALTERA_PR_IP_CORE) += altera-pr-ip-core.o obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT) += altera-pr-ip-core-plat.o diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c new file mode 100644 index 000000000000..7579b0de119f --- /dev/null +++ b/drivers/fpga/microchip-spi.c @@ -0,0 +1,386 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Microchip Polarfire FPGA programming over slave SPI interface. + */ + +#include <asm/unaligned.h> +#include <linux/delay.h> +#include <linux/fpga/fpga-mgr.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/spi/spi.h> + +#define MPF_SPI_ISC_ENABLE 0x0B +#define MPF_SPI_ISC_DISABLE 0x0C +#define MPF_SPI_READ_STATUS 0x00 +#define MPF_SPI_READ_DATA 0x01 +#define MPF_SPI_FRAME_INIT 0xAE +#define MPF_SPI_FRAME 0xEE +#define MPF_SPI_PRG_MODE 0x01 +#define MPF_SPI_RELEASE 0x23 + +#define MPF_SPI_FRAME_SIZE 16 + +#define MPF_HEADER_SIZE_OFFSET 24 +#define MPF_DATA_SIZE_OFFSET 55 + +#define MPF_LOOKUP_TABLE_RECORD_SIZE 9 +#define MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET 0 +#define MPF_LOOKUP_TABLE_BLOCK_START_OFFSET 1 + +#define MPF_COMPONENTS_SIZE_ID 5 +#define MPF_BITSTREAM_ID 8 + +#define MPF_BITS_PER_COMPONENT_SIZE 22 + +#define MPF_STATUS_POLL_RETRIES 10000 +#define MPF_STATUS_BUSY BIT(0) +#define MPF_STATUS_READY BIT(1) +#define MPF_STATUS_SPI_VIOLATION BIT(2) +#define MPF_STATUS_SPI_ERROR BIT(3) + +struct mpf_priv { + struct spi_device *spi; + bool program_mode; +}; + +static int mpf_read_status(struct spi_device *spi) +{ + u8 status = 0, status_command = MPF_SPI_READ_STATUS; + /* + * Two identical SPI transfers are used for status reading. + * The reason is that the first one can be inadequate. + * We ignore it completely and use the second one. + */ + 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); + + if ((status & MPF_STATUS_SPI_VIOLATION) || + (status & MPF_STATUS_SPI_ERROR)) + ret = -EIO; + + return ret ? : status; +} + +static enum fpga_mgr_states mpf_ops_state(struct fpga_manager *mgr) +{ + struct mpf_priv *priv = mgr->priv; + struct spi_device *spi; + bool program_mode; + int status; + + spi = priv->spi; + program_mode = priv->program_mode; + status = mpf_read_status(spi); + + if (!program_mode && !status) + return FPGA_MGR_STATE_OPERATING; + + return FPGA_MGR_STATE_UNKNOWN; +} + +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; + 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; +} + +/* Poll HW status until busy bit is cleared and mask bits are set. */ +static int mpf_poll_status(struct spi_device *spi, u8 mask) +{ + int status, retries = MPF_STATUS_POLL_RETRIES; + + while (retries--) { + status = mpf_read_status(spi); + if (status < 0) + return status; + + if (status & MPF_STATUS_BUSY) + continue; + + if (!mask || (status & mask)) + return status; + } + + return -EBUSY; +} + +static int mpf_spi_write(struct spi_device *spi, const void *buf, size_t buf_size) +{ + int status = mpf_poll_status(spi, 0); + + if (status < 0) + return status; + + return spi_write(spi, buf, buf_size); +} + +static int mpf_spi_write_then_read(struct spi_device *spi, + const void *txbuf, size_t txbuf_size, + void *rxbuf, size_t rxbuf_size) +{ + const u8 read_command[] = { MPF_SPI_READ_DATA }; + int ret; + + ret = mpf_spi_write(spi, txbuf, txbuf_size); + if (ret) + return ret; + + ret = mpf_poll_status(spi, MPF_STATUS_READY); + if (ret < 0) + return ret; + + return spi_write_then_read(spi, read_command, sizeof(read_command), + rxbuf, rxbuf_size); +} + +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 = 0; + 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: spi_ret %d, isc_ret %u\n", + ret, isc_ret); + return -EFAULT; + } + + ret = mpf_spi_write(spi, program_mode, sizeof(program_mode)); + if (ret) { + dev_err(dev, "Failed to enter program mode: %d\n", ret); + return ret; + } + + priv->program_mode = true; + + return 0; +} + +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)); + if (ret) { + dev_err(dev, "Failed to write bitstream frame %d/%zu\n", + 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");
Add support to the FPGA manager for programming Microchip Polarfire FPGAs over slave SPI interface with .dat formatted bitsream image. Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru> --- drivers/fpga/Kconfig | 9 + drivers/fpga/Makefile | 1 + drivers/fpga/microchip-spi.c | 386 +++++++++++++++++++++++++++++++++++ 3 files changed, 396 insertions(+) create mode 100644 drivers/fpga/microchip-spi.c