| Message ID | 1310310203-12288-1-git-send-email-sundaram@ti.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
2011/7/10 Sundaram Raju <sundaram@ti.com>: > Added new dma_ctrl_cmd TI_DMA_STRIDE_CONFIG to pass the TI DMA > controller specific configurations on how a buffer must be walked > through and how data is picked for transfer based on a specified > pattern over the channel. > > The configuration passed is specific to the TI DMA controller used. > > Signed-off-by: Sundaram Raju <sundaram@ti.com> This is exactly how I think we should do this. Acked-by: Linus Walleij <linus.walleij@linaro.org> Thanks, Linus Walleij
On Mon, Jul 11, 2011 at 2:28 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > 2011/7/10 Sundaram Raju <sundaram@ti.com>: > >> Added new dma_ctrl_cmd TI_DMA_STRIDE_CONFIG to pass the TI DMA >> controller specific configurations on how a buffer must be walked >> through and how data is picked for transfer based on a specified >> pattern over the channel. >> >> The configuration passed is specific to the TI DMA controller used. ...and I suspect the slave device drivers that use TI DMA are not expected to ever work with other dmaengines? Likely the case, but just wondering out loud. >> Signed-off-by: Sundaram Raju <sundaram@ti.com> > > This is exactly how I think we should do this. > Acked-by: Linus Walleij <linus.walleij@linaro.org>
On Sun, Jul 10, 2011 at 8:33 PM, Sundaram Raju <sundaram@ti.com> wrote: > Added new dma_ctrl_cmd TI_DMA_STRIDE_CONFIG to pass the TI DMA > controller specific configurations on how a buffer must be walked > through and how data is picked for transfer based on a specified > pattern over the channel. > > The configuration passed is specific to the TI DMA controller used. > > Signed-off-by: Sundaram Raju <sundaram@ti.com> > --- > include/linux/dmaengine.h | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index eee7add..51dadc4 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -123,6 +123,10 @@ enum dma_ctrl_flags { > * command. > * @FSLDMA_EXTERNAL_START: this command will put the Freescale DMA controller > * into external start mode. > + * @TI_DMA_STRIDE_CONFIG: this command is only implemented by TI DMA > + * controllers that need to pass special configuration on how to walk through > + * the buffer to pick up data in a specified pattern to be transferred in > + * the channel. > */ > enum dma_ctrl_cmd { > DMA_TERMINATE_ALL, > @@ -130,6 +134,7 @@ enum dma_ctrl_cmd { > DMA_RESUME, > DMA_SLAVE_CONFIG, > FSLDMA_EXTERNAL_START, > + TI_DMA_STRIDE_CONFIG, > }; IMHO this isn't very correct. 1) Striding, in one form or other, is supported by other DMACs as well. The number will only increase in future. Are we to add <VENDOR>_DMA_STRIDE_CONFIG for each case ? 2) As Dan noted, client drivers are going to have ifdef hackery in order to be common to other SoCs. 3) TI may not have just one DMAC IP used in all the SoCs. So if you want vendor specific defines anyway, please atleast also add DMAC version to it. Something like > DMA_SLAVE_CONFIG, > FSLDMA_EXTERNAL_START, > + TI_DMA_v1_STRIDE_CONFIG,
On Mon, Jul 11, 2011 at 11:39 PM, Dan Williams <dan.j.williams@intel.com> wrote: > On Mon, Jul 11, 2011 at 2:28 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > ...and I suspect the slave device drivers that use TI DMA are not > expected to ever work with other dmaengines? Likely the case, but > just wondering out loud. Typically the OMAP/TI drivers are one-to-one with this specific DMA controller, but they *can* support controllers without stride options, and notice that striding will only be used for the display driver IIRC, pseudo-code: ret = dmaengine_device_control(chan, TI_DMA_STRIDE_CONFIG, (unsigned long) &my_stride_config); if (ret) { /* * OK no striding on this DMA engine, fall back to something else, * such as creating an SGlist which emulates the striding with one * sglist element per stride. */ } By injecting an error in the stride config path this can even be properly tested. So it will become an optional acceleration. Thanks, Linus Walleij
On Tue, Jul 12, 2011 at 6:17 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: > 1) Striding, in one form or other, is supported by other DMACs as well. > The number will only increase in future. > Are we to add <VENDOR>_DMA_STRIDE_CONFIG for each case ? If we are sure about this and striding will work in a similar way on all then let's have the enum named DMA_STRIDE_CONFIG and move the passed-in struct to <linux/dmaengine.h) then? Would that be: struct dma_stride_config { u32 read_bytes; u32 skip_bytes; }; Or something more complex? > 2) As Dan noted, client drivers are going to have ifdef hackery in > order to be common > to other SoCs. Don't think so, why? This is a runtime config entirely, and I just illustrated in mail to Dan how that can be handled by falling back to a sglist I believe? We can *maybe* even put the fallback code into dmaengine, so that an emulated sglist in place for the DMAengine is done automatically of the DMA controller does not support striding. > 3) TI may not have just one DMAC IP used in all the SoCs. So if you want > vendor specific defines anyway, please atleast also add DMAC version to it. > Something like >> DMA_SLAVE_CONFIG, >> FSLDMA_EXTERNAL_START, >> + TI_DMA_v1_STRIDE_CONFIG, Yep unless we make it generic DMA_STRIDE_CONFIG simply, this makes a lot of sense. Linus Walleij
> -----Original Message----- > From: Linus Walleij [mailto:linus.walleij@linaro.org] > Sent: Tuesday, July 12, 2011 3:28 PM > To: Dan Williams > Cc: Raju, Sundaram; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; davinci-linux-open-source@linux.davincidsp.com; > linux@arm.linux.org.uk; linux-omap@vger.kernel.org > Subject: Re: [PATCH] dmaengine: add dma_ctrl_cmd to pass buffer stride > configuration > > On Mon, Jul 11, 2011 at 11:39 PM, Dan Williams <dan.j.williams@intel.com> > wrote: > > On Mon, Jul 11, 2011 at 2:28 AM, Linus Walleij <linus.walleij@linaro.org> > wrote: > > > ...and I suspect the slave device drivers that use TI DMA are not > > expected to ever work with other dmaengines? Likely the case, but > > just wondering out loud. > > Typically the OMAP/TI drivers are one-to-one with this specific DMA > controller, but they *can* support controllers without stride options, and > notice that striding will only be used for the display driver IIRC, > pseudo-code: > > ret = dmaengine_device_control(chan, TI_DMA_STRIDE_CONFIG, > (unsigned long) &my_stride_config); > if (ret) { > /* > * OK no striding on this DMA engine, fall back to something else, > * such as creating an SGlist which emulates the striding with one > * sglist element per stride. > */ > } > > By injecting an error in the stride config path this can even be > properly tested. So it will become an optional acceleration. Yes, this is exactly what I also wanted to say. :) But if the client driver does not implement a fallback like this then that driver will work only with TI DMA and not any other dmaengines. (Mentioning this because, there are client drivers which are tightly coupled to this special configuration) Keeping this in mind, I had started the original discussion with the suggestion of modifying the existing prepare API and adding an extra argument to it, which can be used to pass special configuration. And I also wanted to generalize that configuration passed. Anyways that design also will come down to this same path, and instead of modifying the existing API signatures, I think this is the best way we can go. Regards, Sundaram
> -----Original Message----- > From: Linus Walleij [mailto:linus.walleij@linaro.org] > Sent: Tuesday, July 12, 2011 3:33 PM > To: Jassi Brar > Cc: Raju, Sundaram; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; davinci-linux-open-source@linux.davincidsp.com; > linux@arm.linux.org.uk; dan.j.williams@intel.com; linux-omap@vger.kernel.org > Subject: Re: [PATCH] dmaengine: add dma_ctrl_cmd to pass buffer stride > configuration > > On Tue, Jul 12, 2011 at 6:17 AM, Jassi Brar <jassisinghbrar@gmail.com> > wrote: > > > 1) Striding, in one form or other, is supported by other DMACs as well. > > The number will only increase in future. > > Are we to add <VENDOR>_DMA_STRIDE_CONFIG for each case ? > > If we are sure about this and striding will work in a similar way on all > then let's have the enum named DMA_STRIDE_CONFIG and move the > passed-in struct to <linux/dmaengine.h) then? > > Would that be: > > struct dma_stride_config { > u32 read_bytes; > u32 skip_bytes; > }; > > Or something more complex? > When I started this discussion on stride config, I received comments like this is too specific to TI DMAC, and there are not many DMACs which can do this. I actually wanted to generalize the configuration passed and put a comment on it similar to the one on top of dma_slave_config, which says | |/** <snip> | * The rationale for adding configuration information to this struct | * is as follows: if it is likely that most DMA slave controllers in | * the world will support the configuration option, then make it | * generic. If not: if it is fixed so that it be sent in static from | * the platform data, then prefer to do that. Else, if it is neither | * fixed at runtime, nor generic enough (such as bus mastership on | * some CPU family and whatnot) then create a custom slave config | * struct and pass that, then make this config a member of that | * struct, if applicable. | */ | If any other DMAC can do similar stride configuration, then we can generalize it. Till we generalize this stride configuration I think a custom configuration aligned between the client driver and the offload engine driver can be used. > > 2) As Dan noted, client drivers are going to have ifdef hackery in > > order to be common > > to other SoCs. > > Don't think so, why? This is a runtime config entirely, and I just illustrated > in mail to Dan how that can be handled by falling back to a sglist I believe? > > We can *maybe* even put the fallback code into dmaengine, so that an > emulated sglist in place for the DMAengine is done automatically of the > DMA controller does not support striding. > Good Idea. But the client might always have a better way to handle this fallback than this suggested fallback code in dmaengine, which will be a common implementation based on the received sg_list and the DMAC capabilities. If this is done then preference should be provided to the client's fallback implementation, if present. > > 3) TI may not have just one DMAC IP used in all the SoCs. So if you want > > vendor specific defines anyway, please atleast also add DMAC version to it. > > Something like > >> DMA_SLAVE_CONFIG, > >> FSLDMA_EXTERNAL_START, > >> + TI_DMA_v1_STRIDE_CONFIG, > > Yep unless we make it generic DMA_STRIDE_CONFIG simply, this makes > a lot of sense. > Okay, I can add one cmd for the EDMAC in DaVinci series of SoCs and one for SDMAC in OMAP series of SoCs. Regards, Sundaram
On Tue, Jul 12, 2011 at 12:56 PM, Raju, Sundaram <sundaram@ti.com> wrote: > [Me] >> [Jassi] >> > 3) TI may not have just one DMAC IP used in all the SoCs. So if you want >> > vendor specific defines anyway, please atleast also add DMAC version to it. >> > Something like >> >> DMA_SLAVE_CONFIG, >> >> FSLDMA_EXTERNAL_START, >> >> + TI_DMA_v1_STRIDE_CONFIG, >> >> Yep unless we make it generic DMA_STRIDE_CONFIG simply, this makes >> a lot of sense. > > Okay, I can add one cmd for the EDMAC in DaVinci series of SoCs and > one for SDMAC in OMAP series of SoCs. Wait, that's two different silicon blocks right? Then you already have proof that this spans more than one DMAC and then you can just go for a generic DMA_STRIDE_CONFIG from day one. That both are TI does not matter, if they are totally unrelated implementations. Yours, Linus Walleij
On Tue, Jul 12, 2011 at 3:33 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Jul 12, 2011 at 6:17 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: > >> 1) Striding, in one form or other, is supported by other DMACs as well. >> The number will only increase in future. >> Are we to add <VENDOR>_DMA_STRIDE_CONFIG for each case ? > > If we are sure about this and striding will work in a similar way on all > then let's have the enum named DMA_STRIDE_CONFIG and move the > passed-in struct to <linux/dmaengine.h) then? > > Would that be: > > struct dma_stride_config { > u32 read_bytes; > u32 skip_bytes; > }; > > Or something more complex? Well, I am not sure if striding needs any special treatment at all. Why not have client drivers prepare and submit sg-list. Let the DMAC drivers interpret/parse the sg-list and program it as strides if the h/w supports it. If anything, we should make preparation and submission of sg-list as efficient as possible. >> 2) As Dan noted, client drivers are going to have ifdef hackery in >> order to be common >> to other SoCs. > > Don't think so, why? This is a runtime config entirely, and I just illustrated > in mail to Dan how that can be handled by falling back to a sglist I believe? Runtime decision isn't neat either. What if a client driver is common to 'N' SoCs each with different DMACs ? We would need a switch construct !
> -----Original Message----- > From: Jassi Brar [mailto:jassisinghbrar@gmail.com] > Sent: Tuesday, July 12, 2011 4:51 PM > To: Linus Walleij > Cc: Raju, Sundaram; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; davinci-linux-open-source@linux.davincidsp.com; > linux@arm.linux.org.uk; dan.j.williams@intel.com; linux-omap@vger.kernel.org > Subject: Re: [PATCH] dmaengine: add dma_ctrl_cmd to pass buffer stride > configuration > > On Tue, Jul 12, 2011 at 3:33 PM, Linus Walleij <linus.walleij@linaro.org> > wrote: > > On Tue, Jul 12, 2011 at 6:17 AM, Jassi Brar <jassisinghbrar@gmail.com> > wrote: > > > >> 1) Striding, in one form or other, is supported by other DMACs as well. > >> The number will only increase in future. > >> Are we to add <VENDOR>_DMA_STRIDE_CONFIG for each case ? > > > > If we are sure about this and striding will work in a similar way on all > > then let's have the enum named DMA_STRIDE_CONFIG and move the > > passed-in struct to <linux/dmaengine.h) then? > > > > Would that be: > > > > struct dma_stride_config { > > u32 read_bytes; > > u32 skip_bytes; > > }; > > > > Or something more complex? > Well, I am not sure if striding needs any special treatment at all. > Why not have client drivers prepare and submit sg-list. > Let the DMAC drivers interpret/parse the sg-list and program it > as strides if the h/w supports it. > If anything, we should make preparation and submission of sg-list > as efficient as possible. Jassi, sg_lists describe only a bunch of disjoint buffers. But what if the DMAC can skip and read the bytes within each of the buffers in the sg_list? (like TI EDMAC and TI SDMAC) How can that information be passed to the offload engine driver from the client? ~Sundaram
On Tue, Jul 12, 2011 at 5:01 PM, Raju, Sundaram <sundaram@ti.com> wrote: >> -----Original Message----- >> From: Jassi Brar [mailto:jassisinghbrar@gmail.com] >> Sent: Tuesday, July 12, 2011 4:51 PM >> To: Linus Walleij >> Cc: Raju, Sundaram; linux-arm-kernel@lists.infradead.org; linux- >> kernel@vger.kernel.org; davinci-linux-open-source@linux.davincidsp.com; >> linux@arm.linux.org.uk; dan.j.williams@intel.com; linux-omap@vger.kernel.org >> Subject: Re: [PATCH] dmaengine: add dma_ctrl_cmd to pass buffer stride >> configuration >> >> On Tue, Jul 12, 2011 at 3:33 PM, Linus Walleij <linus.walleij@linaro.org> >> wrote: >> > On Tue, Jul 12, 2011 at 6:17 AM, Jassi Brar <jassisinghbrar@gmail.com> >> wrote: >> > >> >> 1) Striding, in one form or other, is supported by other DMACs as well. >> >> The number will only increase in future. >> >> Are we to add <VENDOR>_DMA_STRIDE_CONFIG for each case ? >> > >> > If we are sure about this and striding will work in a similar way on all >> > then let's have the enum named DMA_STRIDE_CONFIG and move the >> > passed-in struct to <linux/dmaengine.h) then? >> > >> > Would that be: >> > >> > struct dma_stride_config { >> > u32 read_bytes; >> > u32 skip_bytes; >> > }; >> > >> > Or something more complex? >> Well, I am not sure if striding needs any special treatment at all. >> Why not have client drivers prepare and submit sg-list. >> Let the DMAC drivers interpret/parse the sg-list and program it >> as strides if the h/w supports it. >> If anything, we should make preparation and submission of sg-list >> as efficient as possible. > Jassi, > > sg_lists describe only a bunch of disjoint buffers. But what if the > DMAC can skip and read the bytes within each of the buffers in > the sg_list? (like TI EDMAC and TI SDMAC) > How can that information be passed to the offload > engine driver from the client? > OK, I overlooked. We do need something new to handle these ultra-fine-grained sg-lists. But still we shouldn't add SoC specific API to the common sub-systems. Maybe a new api to pass fixed-format variable-length encoded message to the DMAC drivers? Which could be interpreted by DMAC drivers to extract all the needed xfer parameters from the 'header' section and instructions to program the xfers in the DMAC from the variable length body of the 'message' buffer. It might sound complicated but we can have helpers to make the job easy. Btw, the regular single/sg-list xfers could also be expressed by this method.
> -----Original Message----- > From: Jassi Brar [mailto:jassisinghbrar@gmail.com] > Sent: Tuesday, July 12, 2011 6:15 PM > To: Raju, Sundaram > Cc: Linus Walleij; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; davinci-linux-open-source@linux.davincidsp.com; > linux@arm.linux.org.uk; dan.j.williams@intel.com; linux-omap@vger.kernel.org > Subject: Re: [PATCH] dmaengine: add dma_ctrl_cmd to pass buffer stride > configuration > > On Tue, Jul 12, 2011 at 5:01 PM, Raju, Sundaram <sundaram@ti.com> wrote: > >> -----Original Message----- > >> From: Jassi Brar [mailto:jassisinghbrar@gmail.com] > >> Sent: Tuesday, July 12, 2011 4:51 PM > >> To: Linus Walleij > >> Cc: Raju, Sundaram; linux-arm-kernel@lists.infradead.org; linux- > >> kernel@vger.kernel.org; davinci-linux-open-source@linux.davincidsp.com; > >> linux@arm.linux.org.uk; dan.j.williams@intel.com; linux- > omap@vger.kernel.org > >> Subject: Re: [PATCH] dmaengine: add dma_ctrl_cmd to pass buffer stride > >> configuration > >> > >> On Tue, Jul 12, 2011 at 3:33 PM, Linus Walleij <linus.walleij@linaro.org> > >> wrote: > >> > On Tue, Jul 12, 2011 at 6:17 AM, Jassi Brar <jassisinghbrar@gmail.com> > >> wrote: > >> > > >> >> 1) Striding, in one form or other, is supported by other DMACs as well. > >> >> The number will only increase in future. > >> >> Are we to add <VENDOR>_DMA_STRIDE_CONFIG for each case ? > >> > > >> > If we are sure about this and striding will work in a similar way on all > >> > then let's have the enum named DMA_STRIDE_CONFIG and move the > >> > passed-in struct to <linux/dmaengine.h) then? > >> > > >> > Would that be: > >> > > >> > struct dma_stride_config { > >> > u32 read_bytes; > >> > u32 skip_bytes; > >> > }; > >> > > >> > Or something more complex? > >> Well, I am not sure if striding needs any special treatment at all. > >> Why not have client drivers prepare and submit sg-list. > >> Let the DMAC drivers interpret/parse the sg-list and program it > >> as strides if the h/w supports it. > >> If anything, we should make preparation and submission of sg-list > >> as efficient as possible. > > Jassi, > > > > sg_lists describe only a bunch of disjoint buffers. But what if the > > DMAC can skip and read the bytes within each of the buffers in > > the sg_list? (like TI EDMAC and TI SDMAC) > > How can that information be passed to the offload > > engine driver from the client? > > > OK, I overlooked. > We do need something new to handle these ultra-fine-grained sg-lists. > But still we shouldn't add SoC specific API to the common sub-systems. > > Maybe a new api to pass fixed-format variable-length encoded message > to the DMAC drivers? > Which could be interpreted by DMAC drivers to extract all the needed xfer > parameters from the 'header' section and instructions to program the xfers > in the DMAC from the variable length body of the 'message' buffer. > It might sound complicated but we can have helpers to make the job easy. > Btw, the regular single/sg-list xfers could also be expressed by this method. Do you expect this variable length body of the message to be DMAC independent? I don't think so. In that case how is this different from what we have here already? If it can be DMAC independent, can you illustrate more on how this can be done? But the point to note is, if this can be made DMAC independent then the control command we have also can be made DMAC independent by generalizing the configuration structure passed to it. ~ Sundaram
On Mon, Jul 18, 2011 at 1:21 PM, Raju, Sundaram <sundaram@ti.com> wrote: >> >> Maybe a new api to pass fixed-format variable-length encoded message >> to the DMAC drivers? >> Which could be interpreted by DMAC drivers to extract all the needed xfer >> parameters from the 'header' section and instructions to program the xfers >> in the DMAC from the variable length body of the 'message' buffer. >> It might sound complicated but we can have helpers to make the job easy. >> Btw, the regular single/sg-list xfers could also be expressed by this method. > > Do you expect this variable length body of the message to be DMAC > independent? I don't think so. In that case how is this different from what > we have here already? Yes, this whould be DMAC independent. > If it can be DMAC independent, can you illustrate more on how this can > be done? But the point to note is, if this can be made DMAC independent > then the control command we have also can be made DMAC independent > by generalizing the configuration structure passed to it. The 'header' I suggest, would in fact be a structure body, only an extra pointer would point to the 'instructions' to convey actual location and sizes of mico-xfers. I don't think it is possible to have general definition of such transfers fully within a structure. If I understand what you ask. I have just posted an RFC. I kept the terms same so that it is easier to understand. Please have a look. You are CC'ed too. Thanks, Jassi
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index eee7add..51dadc4 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -123,6 +123,10 @@ enum dma_ctrl_flags { * command. * @FSLDMA_EXTERNAL_START: this command will put the Freescale DMA controller * into external start mode. + * @TI_DMA_STRIDE_CONFIG: this command is only implemented by TI DMA + * controllers that need to pass special configuration on how to walk through + * the buffer to pick up data in a specified pattern to be transferred in + * the channel. */ enum dma_ctrl_cmd { DMA_TERMINATE_ALL, @@ -130,6 +134,7 @@ enum dma_ctrl_cmd { DMA_RESUME, DMA_SLAVE_CONFIG, FSLDMA_EXTERNAL_START, + TI_DMA_STRIDE_CONFIG, }; /**
Added new dma_ctrl_cmd TI_DMA_STRIDE_CONFIG to pass the TI DMA controller specific configurations on how a buffer must be walked through and how data is picked for transfer based on a specified pattern over the channel. The configuration passed is specific to the TI DMA controller used. Signed-off-by: Sundaram Raju <sundaram@ti.com> --- include/linux/dmaengine.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)