| Message ID | 1492725802-17457-1-git-send-email-agust@denx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Anatolij, On 04/20/2017 03:03 PM, Anatolij Gustschin wrote: > To avoid bit-swapping in the driver the stream could be prepared > as bit-reversed. This is indicated by a flag in the fpga_image_info > struct. Check the flag and only bitswap if needed. Also fix build > warning with min() macro. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > > Hi Joshua, > > I'm using cyclone-ps-spi v9 with this patch and another patch here [1]. > Could you please merge it when submitting v10 series? > > Thanks, > Anatolij > > [1] https://patchwork.kernel.org/patch/9691381 > > drivers/fpga/cyclone-ps-spi.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c > index 56c3853..43eb124 100644 > --- a/drivers/fpga/cyclone-ps-spi.c > +++ b/drivers/fpga/cyclone-ps-spi.c > @@ -30,6 +30,7 @@ struct cyclonespi_conf { > struct gpio_desc *config; > struct gpio_desc *status; > struct spi_device *spi; > + u32 info_flags; > }; > > static const struct of_device_id of_ef_match[] = { > @@ -55,6 +56,8 @@ static int cyclonespi_write_init(struct fpga_manager *mgr, > struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; > int i; > > + conf->info_flags = info->flags; > + > if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) { > dev_err(&mgr->dev, "Partial reconfiguration not supported.\n"); > return -EINVAL; > @@ -98,9 +101,11 @@ static int cyclonespi_write(struct fpga_manager *mgr, const char *buf, > > while (fw_data < fw_data_end) { > int ret; > - size_t stride = min(fw_data_end - fw_data, SZ_4K); > + size_t stride = min_t(size_t, fw_data_end - fw_data, SZ_4K); > + > + if (!(conf->info_flags & FPGA_MGR_SPI_BITSTREAM_LSB_FIRST)) > + rev_buf((char *)fw_data, stride); > > - rev_buf(fw_data, stride); > ret = spi_write(conf->spi, fw_data, stride); > if (ret) { > dev_err(&mgr->dev, "spi error in firmware write: %d\n", Looks good to me. I'll see about testing and trying again to merge it next week. Thanks, Joshua -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, April 21, 2017 12:03:22 AM PDT Anatolij Gustschin wrote: > To avoid bit-swapping in the driver the stream could be prepared > as bit-reversed. This is indicated by a flag in the fpga_image_info > struct. Check the flag and only bitswap if needed. Also fix build > warning with min() macro. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > > Hi Joshua, > > I'm using cyclone-ps-spi v9 with this patch and another patch here [1]. > Could you please merge it when submitting v10 series? > > Thanks, > Anatolij > > [1] https://patchwork.kernel.org/patch/9691381 > > drivers/fpga/cyclone-ps-spi.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c > index 56c3853..43eb124 100644 > --- a/drivers/fpga/cyclone-ps-spi.c > +++ b/drivers/fpga/cyclone-ps-spi.c > @@ -30,6 +30,7 @@ struct cyclonespi_conf { > struct gpio_desc *config; > struct gpio_desc *status; > struct spi_device *spi; > + u32 info_flags; > }; > > static const struct of_device_id of_ef_match[] = { > @@ -55,6 +56,8 @@ static int cyclonespi_write_init(struct fpga_manager *mgr, > struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; int i; > > + conf->info_flags = info->flags; > + > if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) { > dev_err(&mgr->dev, "Partial reconfiguration not supported.\n"); > return -EINVAL; > @@ -98,9 +101,11 @@ static int cyclonespi_write(struct fpga_manager *mgr, > const char *buf, > > while (fw_data < fw_data_end) { > int ret; > - size_t stride = min(fw_data_end - fw_data, SZ_4K); > + size_t stride = min_t(size_t, fw_data_end - fw_data, SZ_4K); > + > + if (!(conf->info_flags & FPGA_MGR_SPI_BITSTREAM_LSB_FIRST)) > + rev_buf((char *)fw_data, stride); > Now testing... I think the logic here is inverted. if FPGA_MGR_SPI_BITSTREAM_LSB_FIRST (which is the native format for cyclone, but not for Arria), reverse the bits. > - rev_buf(fw_data, stride); > ret = spi_write(conf->spi, fw_data, stride); > if (ret) { > dev_err(&mgr->dev, "spi error in firmware write: %d\n",
Hi, On Wed, 17 May 2017 07:50:25 -0700 joshua.clayton@uniwest.com joshua.clayton@uniwest.com wrote: ... >> while (fw_data < fw_data_end) { >> int ret; >> - size_t stride = min(fw_data_end - fw_data, SZ_4K); >> + size_t stride = min_t(size_t, fw_data_end - fw_data, SZ_4K); >> + >> + if (!(conf->info_flags & FPGA_MGR_SPI_BITSTREAM_LSB_FIRST)) >> + rev_buf((char *)fw_data, stride); >> >Now testing... I think the logic here is inverted. >if FPGA_MGR_SPI_BITSTREAM_LSB_FIRST (which is the native format for cyclone, >but not for Arria), reverse the bits. When this flag is _not_ set (default), then the rev_buf() is invoked, like in you driver version in patch v9. Maybe the meaning of FPGA_MGR_SPI_BITSTREAM_LSB_FIRST is not clear? You do not have to set this flag, only users who post-processed their bitstreams after creation must set this flag (some users do not want to bitreverse bitstream data while sending in the driver, so they reverse the bit order in the bitstream files. This flag is for such users. Arria 10 expects LSB first for each data byte, too (like Cyclone-V and Stratix-V). There is no difference. Thanks, Anatolij -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, May 17, 2017 10:33:49 PM PDT Anatolij Gustschin wrote: > Hi, > > On Wed, 17 May 2017 07:50:25 -0700 > joshua.clayton@uniwest.com joshua.clayton@uniwest.com wrote: > ... > > >> while (fw_data < fw_data_end) { > >> > >> int ret; > >> > >> - size_t stride = min(fw_data_end - fw_data, SZ_4K); > >> + size_t stride = min_t(size_t, fw_data_end - fw_data, SZ_4K); > >> + > >> + if (!(conf->info_flags & FPGA_MGR_SPI_BITSTREAM_LSB_FIRST)) > >> + rev_buf((char *)fw_data, stride); > > > >Now testing... I think the logic here is inverted. > >if FPGA_MGR_SPI_BITSTREAM_LSB_FIRST (which is the native format for > >cyclone, but not for Arria), reverse the bits. > > When this flag is _not_ set (default), then the rev_buf() is invoked, > like in you driver version in patch v9. > > Maybe the meaning of FPGA_MGR_SPI_BITSTREAM_LSB_FIRST is not clear? Exactly! I was confused between the rbf bitstream, which is I guess not lsb first, and the chip, which wants it fed that way. > You do not have to set this flag, only users who post-processed their > bitstreams after creation must set this flag (some users do not want > to bitreverse bitstream data while sending in the driver, so they > reverse the bit order in the bitstream files. This flag is for such > users. ...and there is one other scenario in which it would not be needed: If the SPI device supports LSB_FIRST operations, bit reversal can be done by the SPI chipset, rather than the fpga manager. > > Arria 10 expects LSB first for each data byte, too (like Cyclone-V > and Stratix-V). There is no difference. > Thanks for that clarification. > Thanks, > > Anatolij
diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c index 56c3853..43eb124 100644 --- a/drivers/fpga/cyclone-ps-spi.c +++ b/drivers/fpga/cyclone-ps-spi.c @@ -30,6 +30,7 @@ struct cyclonespi_conf { struct gpio_desc *config; struct gpio_desc *status; struct spi_device *spi; + u32 info_flags; }; static const struct of_device_id of_ef_match[] = { @@ -55,6 +56,8 @@ static int cyclonespi_write_init(struct fpga_manager *mgr, struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; int i; + conf->info_flags = info->flags; + if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) { dev_err(&mgr->dev, "Partial reconfiguration not supported.\n"); return -EINVAL; @@ -98,9 +101,11 @@ static int cyclonespi_write(struct fpga_manager *mgr, const char *buf, while (fw_data < fw_data_end) { int ret; - size_t stride = min(fw_data_end - fw_data, SZ_4K); + size_t stride = min_t(size_t, fw_data_end - fw_data, SZ_4K); + + if (!(conf->info_flags & FPGA_MGR_SPI_BITSTREAM_LSB_FIRST)) + rev_buf((char *)fw_data, stride); - rev_buf(fw_data, stride); ret = spi_write(conf->spi, fw_data, stride); if (ret) { dev_err(&mgr->dev, "spi error in firmware write: %d\n",
To avoid bit-swapping in the driver the stream could be prepared as bit-reversed. This is indicated by a flag in the fpga_image_info struct. Check the flag and only bitswap if needed. Also fix build warning with min() macro. Signed-off-by: Anatolij Gustschin <agust@denx.de> --- Hi Joshua, I'm using cyclone-ps-spi v9 with this patch and another patch here [1]. Could you please merge it when submitting v10 series? Thanks, Anatolij [1] https://patchwork.kernel.org/patch/9691381 drivers/fpga/cyclone-ps-spi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)