| Message ID | 1478732303-13718-9-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 9 Nov 2016, Jason Gunthorpe wrote: > socfpga just uses the CPU to memory copy the bitstream, so there is > no reason it needs contiguous kernel memory. Switch to use the sg > interface. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > drivers/fpga/socfpga.c | 56 +++++++++++++++++++++++++++++++++----------------- > 1 file changed, 37 insertions(+), 19 deletions(-) > > diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c > index 27d2ff28132c..f3f390b2eecf 100644 > --- a/drivers/fpga/socfpga.c > +++ b/drivers/fpga/socfpga.c > @@ -24,6 +24,7 @@ > #include <linux/of_address.h> > #include <linux/of_irq.h> > #include <linux/pm.h> > +#include <linux/scatterlist.h> > > /* Register offsets */ > #define SOCFPGA_FPGMGR_STAT_OFST 0x0 > @@ -408,10 +409,22 @@ static int socfpga_fpga_reset(struct fpga_manager *mgr) > * Prepare the FPGA to receive the configuration data. > */ > static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags, > - const char *buf, size_t count) > + struct sg_table *sgt) > { > struct socfpga_fpga_priv *priv = mgr->priv; > - int ret; > + struct scatterlist *sg; > + int ret, i; > + > + /* We use the CPU to read the bitstream 32 bits at a time, and thus > + * require alignment. > + */ > + for_each_sg(sgt->sgl, sg, sgt->nents, i) { > + if ((sg->offset % 4) != 0) { > + dev_err(&mgr->dev, > + "Invalid bitstream, chunks must be aligned\n"); > + return -EINVAL; > + } > + } > > if (flags & FPGA_MGR_PARTIAL_RECONFIG) { > dev_err(&mgr->dev, "Partial reconfiguration not supported.\n"); > @@ -440,40 +453,45 @@ static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags, > /* > * Step 9: write data to the FPGA data register > */ > -static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr, > - const char *buf, size_t count) > +static void socfpga_write_buf(struct socfpga_fpga_priv *priv, const u32 *buf, > + size_t count) > { > - struct socfpga_fpga_priv *priv = mgr->priv; > - u32 *buffer_32 = (u32 *)buf; > size_t i = 0; > > - if (count <= 0) > - return -EINVAL; > - > /* Write out the complete 32-bit chunks. */ > while (count >= sizeof(u32)) { > - socfpga_fpga_data_writel(priv, buffer_32[i++]); > + socfpga_fpga_data_writel(priv, buf[i++]); > count -= sizeof(u32); > } > > /* Write out remaining non 32-bit chunks. */ > switch (count) { > case 3: > - socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff); > + socfpga_fpga_data_writel(priv, buf[i++] & 0x00ffffff); > break; > case 2: > - socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff); > + socfpga_fpga_data_writel(priv, buf[i++] & 0x0000ffff); > break; > case 1: > - socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff); > - break; > - case 0: > + socfpga_fpga_data_writel(priv, buf[i++] & 0x000000ff); > break; > default: > - /* This will never happen. */ > - return -EFAULT; > + break; > } > +} > + > +static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr, > + struct sg_table *sgt) > +{ > + struct socfpga_fpga_priv *priv = mgr->priv; > + struct sg_mapping_iter miter; > + > + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); > + > + while (sg_miter_next(&miter)) > + socfpga_write_buf(priv, miter.addr, miter.length); > > + sg_miter_stop(&miter); > return 0; > } Hi Jason, Currently or soon we have 3 drivers that don't really use the sg interface natively. So this workaround ends up in each of them? That's a lot of duplicated code. Why can't this code be in the fpga-mgr.c core for drivers that aren't using sg (to minimizing duplication). I will test this when I get time, may not be this week. I just moved to a new building and lab and am in a course all week and so forth. Alan > > @@ -545,8 +563,8 @@ static enum fpga_mgr_states socfpga_fpga_ops_state(struct fpga_manager *mgr) > > static const struct fpga_manager_ops socfpga_fpga_ops = { > .state = socfpga_fpga_ops_state, > - .write_init = socfpga_fpga_ops_configure_init, > - .write = socfpga_fpga_ops_configure_write, > + .write_init_sg = socfpga_fpga_ops_configure_init, > + .write_sg = socfpga_fpga_ops_configure_write, > .write_complete = socfpga_fpga_ops_configure_complete, > }; > > -- > 2.1.4 > >
On Sun, Nov 13, 2016 at 05:19:34PM -0600, atull wrote: > Currently or soon we have 3 drivers that don't really use the sg > interface natively. So this workaround ends up in each of them? Thinking of the SG list as a workaround is not really right - the SG list is a way to pass memory stored in non-contiguous pages around, and the miter is a way to access them from the CPU. socfpga *does* use sg natively because it is happy to process the data from the CPU page-at-time. It just doesn't use DMA. > That's a lot of duplicated code. Why can't this code be in the > fpga-mgr.c core for drivers that aren't using sg (to minimizing > duplication). Sure, if it is a common pattern it is a good idea to lift it. I'd add a newop 'write_fragment' and a driver must define write_sg write_fragment, if write_fragment is used then the core supplies that loop. Is there a tree with these new drivers someplace? > I will test this when I get time, may not be this week. I just > moved to a new building and lab and am in a course all week and > so forth. Sure, I don't expect any problems, Zynq uses the same loop and it seems fine. Jason
On Mon, 14 Nov 2016, Jason Gunthorpe wrote: > On Sun, Nov 13, 2016 at 05:19:34PM -0600, atull wrote: > > > Currently or soon we have 3 drivers that don't really use the sg > > interface natively. So this workaround ends up in each of them? > > Thinking of the SG list as a workaround is not really right - the SG > list is a way to pass memory stored in non-contiguous pages around, > and the miter is a way to access them from the CPU. No, I ment the other way. The changes to socfpga.c are a workaround to the sg-centric interface. And other drivers will need the same workaround. But below I see you understand... > > socfpga *does* use sg natively because it is happy to process the data > from the CPU page-at-time. It just doesn't use DMA. > > > That's a lot of duplicated code. Why can't this code be in the > > fpga-mgr.c core for drivers that aren't using sg (to minimizing > > duplication). > > Sure, if it is a common pattern it is a good idea to lift it. > > I'd add a newop 'write_fragment' and a driver must define write_sg > write_fragment, if write_fragment is used then the core supplies > that loop. Sure, but isn't that just the old op? :) There may also be common code that you added to configure_init that should go in the core unless it's fpga-specific. > > Is there a tree with these new drivers someplace? The arria10 driver is on linux-next master branch. There are two others on the mailing list now. linux-next also contains other changes to the FPGA mgr API that will affect your patches in minor ways, so you should rebase you patches. > > > I will test this when I get time, may not be this week. I just > > moved to a new building and lab and am in a course all week and > > so forth. > > Sure, I don't expect any problems, Zynq uses the same loop and it > seems fine. > > Jason >
On Sun, Nov 13, 2016 at 10:02:00PM -0600, atull wrote: > > I'd add a newop 'write_fragment' and a driver must define write_sg > > write_fragment, if write_fragment is used then the core supplies > > that loop. > > Sure, but isn't that just the old op? :) I'm fine with that too, but it is semantically different since write is called multiple times in this new world.. All the drivers seem fine with that today so it is probably OK .. > There may also be common code that you added to configure_init > that should go in the core unless it's fpga-specific. Sure, the alignment test is easy enough to pull out.. Jason
On Tue, 15 Nov 2016, Jason Gunthorpe wrote: > On Sun, Nov 13, 2016 at 10:02:00PM -0600, atull wrote: > > > I'd add a newop 'write_fragment' and a driver must define write_sg > > > write_fragment, if write_fragment is used then the core supplies > > > that loop. > > > > Sure, but isn't that just the old op? :) > > I'm fine with that too, but it is semantically different since write > is called multiple times in this new world.. All the drivers seem fine > with that today so it is probably OK .. Not different. From 'fpga-mgr.txt': The programming sequence is: 1. .write_init 2. .write (may be called once or multiple times) 3. .write_complete The old write was be separate from write_init and write_complete because I figured that in the future someone may be streaming in the bitstream and not have the whole bitstream in memory. So that is keeping with the original intention that it could be called multiple times. The current API doesn't do that but the fpga manager ops are intended to support that from the start. Alan > > > There may also be common code that you added to configure_init > > that should go in the core unless it's fpga-specific. > > Sure, the alignment test is easy enough to pull out.. > > Jason >
On Tue, Nov 15, 2016 at 09:47:05AM -0600, atull wrote: > Not different. > > From 'fpga-mgr.txt': > The programming sequence is: > 1. .write_init > 2. .write (may be called once or multiple times) > 3. .write_complete > > The old write was be separate from write_init and write_complete > because I figured that in the future someone may be streaming in > the bitstream and not have the whole bitstream in memory. What is the point of this if write_init gets a copy of the buffer - what is that supposed to be? If you see things this way why are you opposed to patch 9? I'll change things around to call write multiple times and force the sg list into write_init, which seems like what you intended anyhow.. Jason
On Wed, 16 Nov 2016, Jason Gunthorpe wrote: > On Tue, Nov 15, 2016 at 09:47:05AM -0600, atull wrote: > > Not different. > > > > From 'fpga-mgr.txt': > > The programming sequence is: > > 1. .write_init > > 2. .write (may be called once or multiple times) > > 3. .write_complete > > > > The old write was be separate from write_init and write_complete > > because I figured that in the future someone may be streaming in > > the bitstream and not have the whole bitstream in memory. > > What is the point of this if write_init gets a copy of the buffer - > what is that supposed to be? Sometimes write_init needs to look at the header of the image. You can see that in the socfpga-a10.c (on linux-next/master) > > If you see things this way why are you opposed to patch 9? I'll change > things around to call write multiple times and force the sg list into > write_init, which seems like what you intended anyhow.. Not against it (and I do need to spend some more time looking at this stuff, this is coming at a busy time). My point there was that there was code that needed to go into the core so that the ICE40 and the cyclone spi driver that are on the mailing list won't have to have the same workaround that you were adding to the socfpga.c driver. Alan > > Jason >
On Wed, Nov 16, 2016 at 09:45:23AM -0600, atull wrote: > > What is the point of this if write_init gets a copy of the buffer - > > what is that supposed to be? > > Sometimes write_init needs to look at the header of the image. > You can see that in the socfpga-a10.c (on linux-next/master) I know what it is for, I'm asking what should it be if we are calling write_init multiple times. It feels like the driver needs to indicate the header length it wants to inspect and the core core needs to make that much of the bitstream available to write_init() before calling write(). Is that what you were thinking? > at this stuff, this is coming at a busy time). My point there > was that there was code that needed to go into the core so that > the ICE40 and the cyclone spi driver that are on the mailing > list won't have to have the same workaround that you were > adding to the socfpga.c driver. Sure, that is easy for write() - not clear on write_init sematics? I will send a revised series. I'd also like to close on the zynq bitfile verification patch, did you have any comments on that? Jason
On Wed, 16 Nov 2016, Jason Gunthorpe wrote: > On Wed, Nov 16, 2016 at 09:45:23AM -0600, atull wrote: > > > What is the point of this if write_init gets a copy of the buffer - > > > what is that supposed to be? > > > > Sometimes write_init needs to look at the header of the image. > > You can see that in the socfpga-a10.c (on linux-next/master) > > I know what it is for, I'm asking what should it be if we are calling > write_init multiple times. > > It feels like the driver needs to indicate the header length it wants > to inspect and the core core needs to make that much of the bitstream > available to write_init() before calling write(). > > Is that what you were thinking? That would make sense. socfpga-a10.c requires a certain amount of header in write_init, but the current API didn't have a way for socfgpa-a10.c to specify that to fpga-mgr.c core. Should probably happen during registration. If you have an idea about that, that's good, otherwise we'll get back to that separately. > > > at this stuff, this is coming at a busy time). My point there > > was that there was code that needed to go into the core so that > > the ICE40 and the cyclone spi driver that are on the mailing > > list won't have to have the same workaround that you were > > adding to the socfpga.c driver. > > Sure, that is easy for write() - not clear on write_init sematics? > I will send a revised series. > > I'd also like to close on the zynq bitfile verification patch, did you > have any comments on that? I think Joshua had some comments. Besides that, I'm ok with that patch. Alan > > Jason >
On Thu, 17 Nov 2016, atull wrote: > On Wed, 16 Nov 2016, Jason Gunthorpe wrote: > > > On Wed, Nov 16, 2016 at 09:45:23AM -0600, atull wrote: > > > > What is the point of this if write_init gets a copy of the buffer - > > > > what is that supposed to be? > > > > > > Sometimes write_init needs to look at the header of the image. > > > You can see that in the socfpga-a10.c (on linux-next/master) > > > > I know what it is for, I'm asking what should it be if we are calling > > write_init multiple times. > > > > It feels like the driver needs to indicate the header length it wants > > to inspect and the core core needs to make that much of the bitstream > > available to write_init() before calling write(). > > > > Is that what you were thinking? > > That would make sense. socfpga-a10.c requires a certain amount > of header in write_init, but the current API didn't have a way > for socfgpa-a10.c to specify that to fpga-mgr.c core. Should > probably happen during registration. If you have an idea about > that, that's good, otherwise we'll get back to that separately. > > > > > > at this stuff, this is coming at a busy time). My point there > > > was that there was code that needed to go into the core so that > > > the ICE40 and the cyclone spi driver that are on the mailing > > > list won't have to have the same workaround that you were > > > adding to the socfpga.c driver. > > > > Sure, that is easy for write() - not clear on write_init sematics? > > I will send a revised series. > > > > I'd also like to close on the zynq bitfile verification patch, did you > > have any comments on that? > > I think Joshua had some comments. Besides that, I'm ok with that > patch. > > Alan I didn't have any other specific issues with on the other zynq patches. For the sg patches, I don't have anything more to say that I haven't already said. I want to keep open the possibility of streaming an FPGA image in. In this case the FPGA image is not complete in memory and write() will be called multiple times. One case is where maybe hopefully the firmware layer can be extended to not allocate a large buffer (scattered or contiguous) and can stream in the image. If you have a nice way to move the workarounds that you had to add to socfpga.c into the core so they don't replicated in socfpga-a10.c, ICE40, cyclone5 spi and other new drivers that aren't using sg natively, that would be good. I think I've already said all this before so please figure that into he your v2 when you rebase onto linux-next. Alan > > > > > Jason > > >
diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c index 27d2ff28132c..f3f390b2eecf 100644 --- a/drivers/fpga/socfpga.c +++ b/drivers/fpga/socfpga.c @@ -24,6 +24,7 @@ #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/pm.h> +#include <linux/scatterlist.h> /* Register offsets */ #define SOCFPGA_FPGMGR_STAT_OFST 0x0 @@ -408,10 +409,22 @@ static int socfpga_fpga_reset(struct fpga_manager *mgr) * Prepare the FPGA to receive the configuration data. */ static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags, - const char *buf, size_t count) + struct sg_table *sgt) { struct socfpga_fpga_priv *priv = mgr->priv; - int ret; + struct scatterlist *sg; + int ret, i; + + /* We use the CPU to read the bitstream 32 bits at a time, and thus + * require alignment. + */ + for_each_sg(sgt->sgl, sg, sgt->nents, i) { + if ((sg->offset % 4) != 0) { + dev_err(&mgr->dev, + "Invalid bitstream, chunks must be aligned\n"); + return -EINVAL; + } + } if (flags & FPGA_MGR_PARTIAL_RECONFIG) { dev_err(&mgr->dev, "Partial reconfiguration not supported.\n"); @@ -440,40 +453,45 @@ static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags, /* * Step 9: write data to the FPGA data register */ -static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr, - const char *buf, size_t count) +static void socfpga_write_buf(struct socfpga_fpga_priv *priv, const u32 *buf, + size_t count) { - struct socfpga_fpga_priv *priv = mgr->priv; - u32 *buffer_32 = (u32 *)buf; size_t i = 0; - if (count <= 0) - return -EINVAL; - /* Write out the complete 32-bit chunks. */ while (count >= sizeof(u32)) { - socfpga_fpga_data_writel(priv, buffer_32[i++]); + socfpga_fpga_data_writel(priv, buf[i++]); count -= sizeof(u32); } /* Write out remaining non 32-bit chunks. */ switch (count) { case 3: - socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff); + socfpga_fpga_data_writel(priv, buf[i++] & 0x00ffffff); break; case 2: - socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff); + socfpga_fpga_data_writel(priv, buf[i++] & 0x0000ffff); break; case 1: - socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff); - break; - case 0: + socfpga_fpga_data_writel(priv, buf[i++] & 0x000000ff); break; default: - /* This will never happen. */ - return -EFAULT; + break; } +} + +static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr, + struct sg_table *sgt) +{ + struct socfpga_fpga_priv *priv = mgr->priv; + struct sg_mapping_iter miter; + + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); + + while (sg_miter_next(&miter)) + socfpga_write_buf(priv, miter.addr, miter.length); + sg_miter_stop(&miter); return 0; } @@ -545,8 +563,8 @@ static enum fpga_mgr_states socfpga_fpga_ops_state(struct fpga_manager *mgr) static const struct fpga_manager_ops socfpga_fpga_ops = { .state = socfpga_fpga_ops_state, - .write_init = socfpga_fpga_ops_configure_init, - .write = socfpga_fpga_ops_configure_write, + .write_init_sg = socfpga_fpga_ops_configure_init, + .write_sg = socfpga_fpga_ops_configure_write, .write_complete = socfpga_fpga_ops_configure_complete, };
socfpga just uses the CPU to memory copy the bitstream, so there is no reason it needs contiguous kernel memory. Switch to use the sg interface. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- drivers/fpga/socfpga.c | 56 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 19 deletions(-)